From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [PATCH v4 1/3] watchdog: add rza_wdt driver Date: Thu, 2 Mar 2017 10:39:49 -0800 Message-ID: <20170302183949.GA2047@roeck-us.net> References: <20170302135746.30550-1-chris.brandt@renesas.com> <20170302135746.30550-2-chris.brandt@renesas.com> <20170302171148.GA28554@roeck-us.net> <20170302174855.GD28554@roeck-us.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-watchdog-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Chris Brandt Cc: Wim Van Sebroeck , Sebastian Reichel , Rob Herring , Mark Rutland , Simon Horman , Geert Uytterhoeven , "linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: devicetree@vger.kernel.org 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 -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bh-25.webhostbox.net ([208.91.199.152]:46254 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751917AbdCBSkS (ORCPT ); Thu, 2 Mar 2017 13:40:18 -0500 Date: Thu, 2 Mar 2017 10:39:49 -0800 From: Guenter Roeck To: Chris Brandt Cc: Wim Van Sebroeck , Sebastian Reichel , Rob Herring , Mark Rutland , Simon Horman , Geert Uytterhoeven , "linux-renesas-soc@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-watchdog@vger.kernel.org" Subject: Re: [PATCH v4 1/3] watchdog: add rza_wdt driver Message-ID: <20170302183949.GA2047@roeck-us.net> References: <20170302135746.30550-1-chris.brandt@renesas.com> <20170302135746.30550-2-chris.brandt@renesas.com> <20170302171148.GA28554@roeck-us.net> <20170302174855.GD28554@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org 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