On Fri, Jan 21, 2011 at 11:38 PM, Jamie Iles <jamie@xxxxxxxxxxxxx> wrote:
> On Wed, Jan 12, 2011 at 10:46:28AM +0530, viresh kumar wrote:
>> > + Â Â Â 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;
>> }
>
> Hmm, looking back at this again, I wonder if the correct thing to do is
> just add a dependency in Kconfig for the driver on HAVE_CLK like
> drivers/spi/dw_spi_mmio and ditch the ability to set the rate through
> platform data? ÂThe platform has to at least provide clk_get(),
> clk_put() and clk_get_rate() at a minimum or the driver won't even
> build...
Yes, this has to be done from compilation point of view, we need
HAVE_CLK in kconfig.
>
> This would make things a lot simpler and it's not as if this is going to
> break anything as there aren't currently any users of the driver (and
> the one that will be does support the clk framework).
>
Ok.
--
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
