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
