RE: [PATCH] watchdog: rza_wdt: Extend clock sources for RZ/A2

[Chris Brandt <Chris.Brandt@renesas.com>]

Hi Geert,

On Thursday, September 06, 2018, Geert Uytterhoeven wrote:
> Thanks for your patch!

Thank you for your review.

> "timeout * rate" may overflow
> DIV_ROUND_UP()?
OK.

> > +       pr_debug("%s: timeout set to %d (WTCNT=%d)\n", __func__,
> 
> %u for unsigned.
OK.


> > +       if (of_device_is_compatible(np, "renesas,r7s9210-wdt"))
> > +               priv->ext_cks = true;
> 
> That's not the proper way to support multiple devices.
> Please add an entry for "renesas,r7s9210-wdt" to rza_wdt_of_match[].
> "renesas,r7s9210-wdt" must be documented in the DT bindings.
> 
> I suggest storing cks in rza_wdt_of_match[].data, and retrieving it with
> of_device_get_match_data() in your probe function, so you can use that to
> differentiate, 

OK, I will change to that.

> and get rid of the ext_cks flag.

I still need to keep track if it's a RZ/A1 or RZ/A2 (ext_cks) for 
functions outside of probe (rza_wdt_calc_timeout).

> BTW, is the RZ/A2 WDT compatible with the RZ/A1 WDT, i.e. does it work
> with the unmodified driver? If not, "renesas,rza-wdt" must not be used as
> a
> fallback.

I would say it is not, because the dividers are different (meaning the 
calculated timeouts would not be correct).

Thanks,
Chris