Grant, Thanks for the review. Comments inlined.
On Sat, Aug 14, 2010 at 4:39 AM, Grant Likely <[email protected]> wrote: > On Fri, Aug 13, 2010 at 8:05 AM, Charulatha V <[email protected]> wrote: >> From: Govindraj.R <[email protected]> >> >> This patch converts the McSPI driver to use pm_runtime apis while >> implementing it in HWMOD FW way. > > Hi Govindraj, > > Some comments below. Short version is that this patch seems to lump a > number of (mostly) unrelated changes (the register map, HWMOD stuff, > and runtime pm) that makes it hard to review and understand. I could > use some help understanding what the intent of the patch is, and it > would also help to split it up. > > Cheers, > g. Sure, I will split this patch as below. 1.) reg map update 2.) hwmod update(like removing dma macros etc.) + runtime (if split, git bisect can break) (Since clock handling will be done with hwmod) <snip> >> + [OMAP2_MCSPI_RX0] = 0x13c, >> + [OMAP2_MCSPI_HL_REV] = 0x000, >> + [OMAP2_MCSPI_HL_HWINFO] = 0x004, >> + [OMAP2_MCSPI_HL_SYSCONFIG] = 0x010, >> +}; > > Other than an 0x100 offset for omap4 and the addition of revision > registers, these two register maps are identical. Is it really worth > having a complete register map table for two things that aren't really > different? It looks overengineered. > > Personally I'd handle this by having two base address pointers; the > regs pointer and the info/revision pointer. omap4 would have the > revision base set, and omap2 would not. > Yes will have a single table for register offset, pass a 0x100(probably named as offset deviation) value from platform data to be added while read/write to registers for omap4 and others can have 0 <snip> >> + pdata->num_cs = 2; >> + pdata->mode = OMAP2_MCSPI_MASTER; >> + pdata->dma_mode = 1; >> + pdata->force_cs_mode = 0; >> + pdata->fifo_depth = 0; >> + break; >> + case 2: >> + pdata->num_cs = 2; >> + break; >> + case 3: >> + pdata->num_cs = 1; >> + break; >> + } > > HWMOD appears to have the purpose of making device instantiation data > driven. If so, shouldn't these parameters also be extracted from the > HWMOD data? Yes these params can be passed from hwmod data This has to part of dev attribute data that can be passed from hwmod data. <snip> >> - mcspi->ick = clk_get(&pdev->dev, "ick"); >> - if (IS_ERR(mcspi->ick)) { >> - dev_dbg(&pdev->dev, "can't get mcspi_ick\n"); >> - status = PTR_ERR(mcspi->ick); >> - goto err1a; >> - } >> - mcspi->fck = clk_get(&pdev->dev, "fck"); >> - if (IS_ERR(mcspi->fck)) { >> - dev_dbg(&pdev->dev, "can't get mcspi_fck\n"); >> - status = PTR_ERR(mcspi->fck); >> - goto err2; >> - } >> - > > I'm obviously missing some context here. The driver doesn't seem to > manage the clocks anymore. What code does now? hwmod layer is handling all clocks now. <snip> >> + >> static struct platform_driver omap2_mcspi_driver = { >> .driver = { >> - .name = "omap2_mcspi", >> - .owner = THIS_MODULE, >> + .name = "omap2_mcspi", >> + .owner = THIS_MODULE, >> + .pm = &omap_mcspi_dev_pm_ops, > > nit: Unrelated whitespace changes. > Will correct this in next version. --- Regards, Govindraj.R ------------------------------------------------------------------------------ This SF.net Dev2Dev email is sponsored by: Show off your parallel programming skills. Enter the Intel(R) Threading Challenge 2010. http://p.sf.net/sfu/intel-thread-sfd _______________________________________________ spi-devel-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/spi-devel-general
