Re: [PATCH 2/5] pwm: jz4740: Fix pin level of disabled TCU2 channels, part 2

[Paul Cercueil]

Le mardi 29 novembre 2022 à 17:24 +0100, Uwe Kleine-König a écrit :
> Hello Paul,
> 
> On Tue, Nov 29, 2022 at 12:25:56PM +0000, Paul Cercueil wrote:
> > Hi Uwe,
> > 
> > Le lun. 28 nov. 2022 à 15:39:11 +0100, Uwe Kleine-König
> > <u.kleine-koenig@xxxxxxxxxxxxxx> a écrit :
> > > Hello,
> > > 
> > > On Tue, Oct 25, 2022 at 11:10:46AM +0100, Paul Cercueil wrote:
> > > > > Note that for disabled PWMs there is no official guaranty
> > > > > about the pin
> > > > > state. So it would be ok (but admittedly not great) to
> > > > > simplify the
> > > > > driver and accept that the pinstate is active while the PWM
> > > > > is off.
> > > > > IMHO this is also better than a glitch.
> > > > > 
> > > > > If a consumer wants the PWM to be in its inactive state, they
> > > > > should
> > > > > not disable it.
> > > > 
> > > > Completely disagree. I absolutely do not want the backlight to
> > > > go full
> > > > bright mode when the PWM pin is disabled. And disabling the
> > > > backlight is a
> > > > thing (for screen blanking and during mode changes).
> > > 
> > > For some hardwares there is no pretty choice. So the gist is: If
> > > the
> > > backlight driver wants to ensure that the PWM pin is driven to
> > > its
> > > inactive level, it should use:
> > > 
> > >         pwm_apply(pwm, { .period = ..., .duty_cycle = 0, .enabled
> > > = true });
> > > 
> > > and better not
> > > 
> > >         pwm_apply(pwm, { ..., .enabled = false });
> > 
> > Well that sounds pretty stupid to me; why doesn't the PWM subsystem
> > enforce
> > that the pins must be driven to their inactive level when the PWM
> > function
> > is disabled?
> > 
> > Then for such hardware you describe, the corresponding PWM
> > driver could itself apply a duty_cycle = 0 if that's what it takes
> > to get an
> > inactive state.
> 
> Let's assume we claim that on disable the pin is driven to the
> inactive level.
> 
> The (bad) effect is that for a use case where the pin state doesn't
> matter (e.g. a backlight where the power regulator is off), the PWM
> keeps running even though it could be disabled and so save some
> power.
> 
> So to make this use case properly supported, we need another flag in
> struct pwm_state that allows the consumer to tell the lowlevel driver
> that it's ok to disable the hardware even with the output being UB.
> Let's call this new flag "spam" and the pin is allowed to do whatever
> it
> wants with .spam = false.
> 
> After that you can realize that applying any state with:
> 
>         .duty_cycle = A,
>         .period = B,
>         .polarity = C,
>         .enabled = false,
>         .spam = true,
> 
> semantically (i.e. just looking at the output) has the same effect as
> 
>         .duty_cycle = 0,
>         .period = $something,
>         .polarity = C,
>         .enabled = true,
>         .spam = true,
> 
> So having .enabled doesn't add to the expressiveness of pwm_apply(),
> because you can specify any configuration without having to resort to
> .enabled = false. So the enabled member of struct pwm_state can be
> dropped.
> 
> Then we end up with the exact scenario we have now, just that the
> flag
> that specifies if the output should be held in the inactive state has
> a
> bad name.

If I follow you, then it means that the PWM backlight driver pwm_bl.c
should set state.enabled=true in pwm_backlight_power_off() to make sure
that the pin is inactive?

-Paul