Hi Andrew,
>> > +config IMGPDC_WDT
>> > + tristate "Imagination Technologies PDC Watchdog Timer"
>> > + depends on HAS_IOMEM
>>
>> We probably don't want to make this visible for everyone with
>> HAS_IOMEM, so perhaps MIPS || (HAS_IOMEM && COMPILE_TEST)? Do we want
>> to make this available for META as well?
> yeh, can I suggest something like this:
> depends on HAS_IOMEM
> depends on METAG || MIPS || COMPILE_TEST
> More intuitive/scalable to have them separate IMO, since technically it
> needs HAS_IOMEM regardless of platform (even if the listed platforms are
> guaranteed to select HAS_IOMEM).
Shall we go ahead with what James Hogan is suggesting here ?
>>
>> > --- /dev/null
>> > +++ b/drivers/watchdog/imgpdc_wdt.c
>>
>> > +#ifdef CONFIG_COMMON_CLK
>> > +#define to_pdc_wdt(_nb) container_of(_nb, struct pdc_wdt_dev, clk_nb)
>> > +
>> > +static int img_pdc_wdt_clk_notify(struct notifier_block *nb,
>> > + unsigned long action, void *data)
>> > +{
>> > + struct pdc_wdt_dev *wdt = to_pdc_wdt(nb);
>> > +
>> > + switch (action) {
>> > + case POST_RATE_CHANGE:
>> > + if (wdt->clk_rate == data->new_rate)
>> > + return NOTIFY_OK;
>> > +
>> > + /* We use the clock rate to calculate the max timeout */
>> > + if (data->new_rate < 1 ||
>> > + data->new_rate > PDC_MAX_WD_CLK_RATE) {
>> > + dev_err(wdt->dev, "invalid clock rate\n");
>> > + return NOTIFY_BAD;
>> > + }
>> > + wdt->clk_rate = data->new_rate;
>> > + if (order_base_2(wdt->clk_rate) == 0)
>> > + wdt->min_delay = 0;
>> > + else
>> > + wdt->min_delay = order_base_2(wdt->clk_rate) - 1;
>> > +
>> > + wdt->wdt_dev.max_timeout =
>> > + (1 << (PDC_WD_MAX_DELAY - wdt->min_delay));
>> > + ret = watchdog_init_timeout(&wdt->wdt_dev, timeout, wdt->dev);
>> > + if (ret < 0)
>> > + wdt->wdt_dev.timeout = wdt->wdt_dev.max_timeout;
>> > +
>> > + pdc_wdt_stop(&wdt->wdt_dev);
>> > + pdc_wdt_set_timeout(&wdt->wdt_dev, wdt->wdt_dev.timeout);
>> > + pdc_wdt_start(&wdt->wdt_dev);
>> > + break;
>> > + default:
>> > + break;
>> > + }
>> > + return NOTIFY_OK;
>> > +}
>> > +#endif
>>
>> Do we really expect the watchdog clock to change out from under us? I
>> think we can just get the rate in probe() and set the timeout bounds
>> based on that - no one outside the watchdog driver should be messing
>> with the watchdog clock, right? I know the samsung driver does
>> something similar to this, but I believe that's because the watchdog
>> clock is derived from the CPU clock.
> I agree (sorry if I implied wdt needed to be dynamic as well as reading
> the [probably constant] rate before).
> Note, dynamic changes did happen on TZ1090, but only just before suspend
> (since the hardware would change the rate regardless of what software
> did when the power actually went down, so it at least allowed drivers to
> put their workarounds in place). I presume watchdog timers aren't meant
> to be active during suspend though, in which case even for TZ1090 it
> probably doesn't need to expect changes.
>From the comments above both from Andrew & James Hogan, I understand that
we don't need clock rate-change notifier callback for Watchdog driver.
Please correct me if I am wrong.
> Cheers
> James
Thanks,
Naidu.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
