From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philipp Zabel Subject: Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line Date: Mon, 29 Jul 2019 12:18:58 +0200 Message-ID: <1564395538.7633.7.camel@pengutronix.de> References: <20190726184045.14669-1-jernej.skrabec@siol.net> <20190726184045.14669-3-jernej.skrabec@siol.net> <20190729063630.rn325whatfnc3m7n@pengutronix.de> <20190729071218.bukw7vxilqy523k3@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20190729071218.bukw7vxilqy523k3@pengutronix.de> Sender: linux-kernel-owner@vger.kernel.org To: Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= , Chen-Yu Tsai Cc: Jernej Skrabec , Thierry Reding , Maxime Ripard , Rob Herring , Mark Rutland , linux-pwm@vger.kernel.org, devicetree , linux-arm-kernel , linux-kernel , linux-sunxi List-Id: devicetree@vger.kernel.org Hi, On Mon, 2019-07-29 at 09:12 +0200, Uwe Kleine-König wrote: > Hello, > > On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote: > > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König > > wrote: > > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote: > > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev) > > > > if (IS_ERR(pwm->clk)) > > > > return PTR_ERR(pwm->clk); > > > > > > > > + if (pwm->data->has_reset) { > > > > + pwm->rst = devm_reset_control_get(&pdev->dev, NULL); > > > > + if (IS_ERR(pwm->rst)) > > > > + return PTR_ERR(pwm->rst); > > > > + > > > > + reset_control_deassert(pwm->rst); > > > > + } > > > > + > > > > > > I wonder why there is a need to track if a given chip needs a reset > > > line. I'd just use devm_reset_control_get_optional() and drop the > > > .has_reset member in struct sun4i_pwm_data. > > > > Because it's not optional for this platform, i.e. it won't work if > > the reset control (or clk, in the next patch) is somehow missing from > > the device tree. > > If the device tree is wrong it is considered ok that the driver doesn't > behave correctly. So this is not a problem you need (or should) care > about. I agree with this. Catching missing DT properties and other device tree validation is not the job of device drivers. The _optional request variants were introduced to simplify drivers that require the reset line on some platforms and not on others. I would ask to explicitly state whether the driver needs full control over the moment of (de)assertion of the reset signal, or whether the only requirement is that the reset signal stays deasserted while the PWM driver is active, by using devm_reset_control_get_optional_exclusive or devm_reset_control_get_optional_shared to request the reset control. regards Philipp