Message-ID: <20190625191950.2rvvputus4uavyjn@pengutronix.de>
Date:   Tue, 25 Jun 2019 21:19:50 +0200
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Paul Cercueil <paul@...pouillou.net>,
        Lee Jones <lee.jones@...aro.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        od@...c.me, linux-pwm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH] backlight: pwm_bl: Set pin to sleep state when powered
 down

On Tue, Jun 25, 2019 at 11:58:21AM +0200, Thierry Reding wrote:
> On Tue, Jun 25, 2019 at 09:42:20AM +0200, Uwe Kleine-König wrote:
> > On Wed, May 22, 2019 at 06:34:28PM +0200, Paul Cercueil wrote:
> > > When the driver probes, the PWM pin is automatically configured to its
> > > default state, which should be the "pwm" function. However, at this
> > > point we don't know the actual level of the pin, which may be active or
> > > inactive. As a result, if the driver probes without enabling the
> > > backlight, the PWM pin might be active, and the backlight would be
> > > lit way before being officially enabled.
> > 
> > I'm not sure I understand the problem completely here. Let me try to
> > summarize the problem you solve here:
> > 
> > The backlight device's default pinctrl contains the PWM function of the
> > PWM pin. As the PWM is (or at least might be) in an undefined state the
> > default pinctrl should only be switched to when it's clear if the
> > backlight should be on or off.
> > 
> > So you use the "init"-pinctrl to keep the PWM pin in some (undriven?)
> > state and by switching to "sleep" you prevent "default" getting active.
> > 
> > Did I get this right? If not, please correct me.
> > 
> > What is the PWM pin configured to in "init" in your case? Is the pinctrl
> > just empty? Or is it a gpio-mode (together with a gpio-hog)?
> > 
> > My thoughts to this is are:
> > 
> >  a) This is a general problem that applies (I think) to most if not all
> >     PWM consumers. If the PWM drives a motor, or makes your mobile
> >     vibrate, or drives an LED, or a clk, the PWM shouldn't start
> >     to do something before its consumer is ready.
> 
> Yes, it shouldn't start before it is explicitly told to do so by the
> consumer. One exception is if the PWM was already set up by firmware
> to run. So I think in general terms we always want the PWM to remain
> in the current state upon probe.

In the end this means that also pinmuxing should not be touched when the
PWM device probes.

> The atomic PWM API was designed with that in mind. The original use-
> case was to allow seamlessly taking over from a PWM regulator. In order
> to do so, the driver needs to be able to read back the hardware state
> and *not* initialize the PWM to some default state.
> 
> I think that same approach can be extended to backlights. The driver's
> probe needs to determine what the current state of the backlight is and
> preferable not touch it. And that basically propagates all the way to
> the display driver, which ultimately needs to determine whether or not
> the display configuration (including the backlight) is enabled.

Are you ambitious enough to handle cases like: PWM is running (maybe
because it cannot be disabled), but the pin is muxed to an "unconnected"
configuration such that the pin doesn't oscillate? In this case you'd
need an inspection function for pinmuxing.

> >  b) Thierry made it quite clear[1] that the PWM pin should be configured
> >     in a pinctrl of the pwm device, not the backlight (or more general:
> >     the consumer) device.
> > 
> > While I don't entirely agree with b) I think that even a) alone
> > justifies to think a bit more about the problem and preferably come up
> > with a solution that helps other consumers, too. Ideally if the
> > bootloader sets up the PWM to do something sensible, probing the
> > lowlevel PWM driver and the consumer driver should not interfere with
> > the bootloader's intention until the situation reaches a controlled
> > state. (I.e. if the backlight was left on by the bootloader to show a
> > nice logo, it should not flicker until a userspace program takes over
> > the display device.)
> 
> Yes, exactly that.
> 
> > A PWM is special in contrast to other devices as its intended behaviour
> > is only fixed once a consumer is present. Without a consumer it is
> > unknown if the PWM is inverted or not. And so the common approach that
> > pinctrl is setup by the device core only doesn't work without drawbacks
> > for PWMs.
> 
> Actually I don't think PWMs are special in this regard. A GPIO, for
> example, can also be active-low or active-high, and without a consumer
> there's not enough context to determine which one it should be.

Right, PWMs are more similar to GPIOs than to (say) backlight devices.
With your request to configure the pinmux for a PWM pin with the PWM
device instead of its consumer you're making some things more difficult.
For GPIOs it's quite common that they are muxed from their consumer
because there the same problems are present.

Best regards
Uwe

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

Powered by blists - more mailing lists