Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: "Vokáč Michal" <Michal.Vokac@ysoft.com>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
	"Lukasz Majewski" <l.majewski@majess.pl>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"Fabio Estevam" <fabio.estevam@nxp.com>,
	"Lothar Waßmann" <LW@karo-electronics.de>,
	"Linus Walleij" <linus.walleij@linaro.org>
Subject: Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state
Date: Thu, 22 Nov 2018 17:17:48 +0100	[thread overview]
Message-ID: <20181122161748.2ud7f26xkww5g24m@pengutronix.de> (raw)
In-Reply-To: <20181122150338.GA24661@ulmo>

Hello Thierry,

On Thu, Nov 22, 2018 at 04:03:38PM +0100, Thierry Reding wrote:
> On Sun, Nov 18, 2018 at 09:08:15PM +0100, Uwe Kleine-König wrote:
> > Thinking a bit about this it doesn't really matter for the consumer if
> > the pin stays in the idle level because there is a pull into the right
> > direction and the PWM is high-Z or if the PWM pulls actively in the
> > right direction. Also for pwm_config(pwm, 0, 100) the PWM could disable
> > its output in the presence of a pull, so the property says true that the
> > effects of pwm_config(pwm, 0, 100) and pwm_disable(pwm) should be the
> > same. And so my claim that pwm_disable is a part of the API that
> > doesn't give any value stays true.
> 
> I still think there's a slight difference there. Granted the effect on
> the consumer is the same whether you disable or set the duty-cycle to
> zero because the power output is the same. pwm_disable() is still more
> explicit, though, so it may involve more than just setting the duty-
> cycle.

If pwm_disable() vs. duty-cycle=0 doesn't make a difference to the
consumer, who do we do a favour by providing pwm_disable()?

> > > But again, why should PWMs be special? You turn off all other resources
> > > when you no longer need them, right? If you power your panel with a
> > > regulator, then when the panel is disabled you want to disable the
> > > regulator, right? Similarily if you don't use your I2C controller you
> > > want to turn off the clock that drives it, right? This is the same for
> > > any resource in your system: if you no longer need it, disable it. The
> > > fact that "disabling" the PWM is not straightforward on i.MX doesn't
> > > mean that we should simply ignore it.
> > 
> > I don't say we should ignore it. I say we shouldn't disable the hardware
> > if the consumer calls pwm_disable() if disabling the hardware results in
> > a state that shouldn't happen on pwm_disable().
> 
> That's backwards. If disabling the hardware results in a state that
> shouldn't happen when you disable the hardware, that just means that
> you're doing something wrong. When you do something wrong you fix it.

For you pwm_disable must imply disabling the PWM hardware. As you cannot
have both (disable the hardware and keep the pin on the idle level
without involving pinmux and GPIO) on i.MX in my eyes it is sensible to
drop one requirement from the framework's expectations to make it
possible for i.MX to fulfill them.

As there is no advantage for anyone to force .disable() to actually
disable the hardware (apart from a little power saving that probably
isn't even measurable) that's IMHO the one to drop.

> If the pin goes to the wrong level when you disable the hardware, the
> natural fix for the problem is to make sure the pin stays at the right
> level.

Ack. And you can do this either in an easy variant or in a more
complicated one.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |