Re: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Mar 02, 2017 at 06:22:17PM +0000, Chris Brandt wrote:
> On Thursday, March 02, 2017, Guenter Roeck wrote:
> > > > > > The rate check should probably be here to avoid situations where
> > > > > > rate < 16384.
> > > > >
> > > > > Do I need that if it's technically not possible to have a 'rate'
> > > > > less
> > > > than 25MHz?
> > > > >
> > > > > These watchdogs HW are always feed directly from the peripheral
> > > > > clock and there is no such thing as a 16kHz peripheral block an
> > > > > any Renesas
> > > > SoC.
> > > > >
> > > > Following that line of argument, can clk_get_rate() ever return 0 ?
> > >
> > > In the DT binding, it says that a clock source is required to be present.
> > >
> > > If the user leaves out the "clocks =", then devm_clk_get will fail.
> > >
> > > If the user puts in some crazy value for "clocks = ", then maybe you
> > > could get
> > > 0 (assuming there is a valid clock node they made by themselves
> > > somewhere that runs at 0Hz).
> > > But in that extreme case, I think they deserve to have it crash and
> > > burn because who knows what they are doing.
> > >
> > 
> > But then there could also be a clock source with a rate of less than 16
> > kHz, as wrong as it may be ?
> > 
> > Anyway, I disagree about the crash and burn. It isn't as if this would be
> > really fatal except for the watchdog driver. Bad data in devicetree should
> > not result in a system crash.
> 
> OK. I will put the check in. Something like:
> 
> 	rate = clk_get_rate(priv->clk); 
> 	if (rate < 16384) {
> 		dev_err(&pdev->dev, "invalid clock specified\n");
> 		return -ENOENT;
> 	}
> 
> 
Sounds good. Maybe display the wrong rate as well ?
Not a strong opinion though.

Thanks,
Guenter

> > > > That is the point of devm_ functions. It also means that you won't
> > > > need a remove function if you also use devm_watchdog_register_device().
> > >
> > > OK.
> > > I see that only 1 driver is using devm_watchdog_register_device
> > > (wdat_wdt.c), so maybe that is a new method.
> > >
> > Yes, it is quite new. Still, you are a bit behind. I count 19 users in the
> > mainline kernel.
> 
> OK, I see now.
> 
> 
> Thank you,
> Chris