Jamie,
On Wed, Jan 12, 2011 at 1:54 PM, Jamie Iles <jamie@xxxxxxxxxxxxx> wrote:
>> I don't know why, but checkpatch used to give few errors which it is
>> not giving now.
>> Like:
>> - Mixing spaces and tabs
>> - Line over 80 columns.
>>
>> There are few places in this patch where i have seen these issues, but
>> checkpatch doesn't
>> report them at all.
>
> Hmm, the only lines over 80 chars are printk lines and I can't see any
> with mixed spaces and tabs...
There are few in file drivers/watchdog/dw_wdt.c
line 40-45, 127, 133, 156, 167, 204, 216, 315, 319.
I can check it easily as i have used following in my vimrc file:
au FileType c match error /\s\+$\|\%>80v.\+\|[ ][ ]\+\|\n\n\n\+\|,[^
]\|^[ ]\+[^\*]\|(\s\+\|\s\+)/
au FileType c highlight error ctermbg=red guibg=red ctermfg=blue guifg=blue
au FileType h match error /\s\+$\|\%>80v.\+\|[ ][ ]\+\|\n\n\n\+\|,[^
]\|^[ ]\+[^\*]\|(\s\+\|\s\+)/
au FileType h highlight error ctermbg=red guibg=red ctermfg=blue guifg=blue
and so these sections are highlighted.
(...)
>> > + Â Â Â dw_wdt.clk = clk_get(&pdev->dev, NULL);
>> > + Â Â Â if (IS_ERR(dw_wdt.clk))
>> > + Â Â Â Â Â Â Â return -ENODEV;
>> > +
>> > + Â Â Â ret = clk_enable(dw_wdt.clk);
>>
>> should we call this routine if clk is NULL? Probably we will get error here.
>> But as we discussed earlier, we should support machines who don't support clk
>> framework. So, shouldn't we check clk for NULL here and call this
>> routine only if we have a valid clk pointer, otherwise continue
>> instead of returning error.
>
> I think this is right, it's perfectly valid for a platform to return
> NULL as a clk and you can pass that to clk_enable().
>
I agree on the first part: NULL can be returned from clk_get(), but
passing NULL to clk_enable and clk_get_rate doesn't look fine to me.
As they will always return error in that case and code will return from below
error check. That's why i suggest we should check for NULL before
clk_enable/disable and clk_get_rate.
What do you say?
>> > + Â Â Â if (ret)
>> > + Â Â Â Â Â Â Â goto out_put_clk;
>> > +
>> > + Â Â Â /*
>> > + Â Â Â Â* The timeout period of the watchdog is derived from the input clock
>> > + Â Â Â Â* frequency. ÂFor platforms that don't have a clk for the watchdog,
>> > + Â Â Â Â* they can specify the WDT clock rate through the clk_rate field of
>> > + Â Â Â Â* the struct dw_wdt_platform_data platform data.
>> > + Â Â Â Â*/
>> > + Â Â Â if (pdata && pdata->clk_rate > 0)
>> > + Â Â Â Â Â Â Â dw_wdt.clk_rate = pdata->clk_rate;
>> > + Â Â Â else
>> > + Â Â Â Â Â Â Â dw_wdt.clk_rate = clk_get_rate(dw_wdt.clk);
>> > +
>>
>> I know it doesn't make sense for a platform to have support for clk framework
>> and pass rate through plat data. But this code will always take
>> pdata->rate, if it is passed.
>> Wouldn't it be better if we reverse the sequence.
>>
>> if (clk)
>> Ârate = clk_get_rate(...);
>> else {
>> Âpdata = pdev->dev.platform_data;
>> Âif (pdata)
>> Â rate = pdata->rate;
>> }
>>
>> and then following code
>
> That's what I did first (without the test that clk is not NULL) but
> switched it the other way round so that the platform can override the
> frequency by setting platform_data->clk_rate to nonzero. ÂThat just
> seems a little more flexible and easy for testing but I'm happy to
> switch the order if you feel that's important.
>
Problem will occur if rate is dynamically changed and we are still believing
on platform code's clk_rate.
Would be better if we switch order. i.e. give priority to clk_get_rate over
pdata->rate.
>> > diff --git a/include/linux/platform_data/dw_wdt.h b/include/linux/platform_data/dw_wdt.h
>> > new file mode 100644
>> > index 0000000..0af10ef
>> > --- /dev/null
>> > +++ b/include/linux/platform_data/dw_wdt.h
>>
>> It looks you have created a separate generic folder platform_data.
>> But generally we put these files directly into existing folders.
>> I am not quite sure if that's fine or we should avoid this.
>
> Yes, this was suggested by Greg K-H to stop include/linux getting filled
> with platform data (https://lkml.org/lkml/2011/1/7/323).
>
Oh. That's good.
--
viresh
ST Microelectronics
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
