Grant,
Thanks for the review. Comments inlined.
On Sat, Aug 14, 2010 at 4:39 AM, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> On Fri, Aug 13, 2010 at 8:05 AM, Charulatha V <charu@xxxxxx> wrote:
>> From: Govindraj.R <govindraj.raja@xxxxxx>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
