Hello Abel,
On Thu, Feb 27, 2025 at 03:07:21PM +0200, Abel Vesa wrote:
> On 25-02-26 17:34:50, Uwe Kleine-König wrote:
> > On Wed, Feb 26, 2025 at 05:31:08PM +0200, Abel Vesa wrote:
> > > The current implementation assumes that the PWM provider will be able to
> > > meet the requested period, but that is not always the case. Some PWM
> > > providers have limited HW configuration capabilities and can only
> > > provide a period that is somewhat close to the requested one. This
> > > simply means that the duty cycle requested might either be above the
> > > PWM's maximum value or the 100% duty cycle is never reached.
> >
> > If you request a state with 100% relative duty cycle you should get 100%
> > unless the hardware cannot do that. Which PWM hardware are you using?
> > Which requests are you actually doing that don't match your expectation?
>
> The PWM hardware is Qualcomm PMK8550 PMIC. The way the duty cycle is
> controlled is described in the following comment found in lpg_calc_freq
> of the leds-qcom-lpg driver:
>
> /*
> * The PWM period is determined by:
> *
> * resolution * pre_div * 2^M
> * period = --------------------------
> * refclk
> *
> * Resolution = 2^9 bits for PWM or
> * 2^{8, 9, 10, 11, 12, 13, 14, 15} bits for high resolution PWM
> * pre_div = {1, 3, 5, 6} and
> * M = [0..7].
> *
> * This allows for periods between 27uS and 384s for PWM channels and periods between
> * 3uS and 24576s for high resolution PWMs.
> * The PWM framework wants a period of equal or lower length than requested,
> * reject anything below minimum period.
> */
>
> So if we request a period of 5MHz, that will not ever be reached no matter what config
> is used. Instead, the 4.26 MHz is selected as closest possible.
The trace in the other mail thread suggest that you asked for a period
of 5 ms, not 5 MHz. And that results in a period of 4.26 ms.
> Now, the pwm_bl is not aware of this limitation and will request duty cycle values that
> go above 4.26MHz.
It requests .period = 5 ms + .duty_cycle = 5 ms. This is fine, and
according to the trace this results in both values becoming 4.26 ms in
real life. Seems fine to me.
> > > This could be easily fixed if the pwm_apply*() API family would allow
> > > overriding the period within the PWM state that's used for providing the
> > > duty cycle. But that is currently not the case.
> >
> > I don't understand what you mean here.
>
> What I was trying to say is that the PWM generic framework currently doesn't
> allow overriding the PWM state's period with one provided by the consumer,
> when calling pwm_apply_might_sleep().
Either I still don't understand what you want, or that is impossible or
useless. If you target .period = 5 ms and the hardware can only do 4.26
ms, why would you want to override period to 5 ms?
> Also, the pwm_get_state_hw() doesn't cache the state either.
*shrug*.
> This results in always having to call pwm_get_state_hw() before calling
> pwm_apply_might_sleep().
I cannot follow this conclusion. At least one of us two didn't
understand some detail yet.
> On top of that, pwm_get_state_hw() doesn't default to the cached value if the
> provider doesn't implement the ->get_state() op.
If it did that, the consumer wouldn't know if the request was
implemented exactly or if there is no way to read back the actual
configuration.
> Please correct me if I'm wrong about these.
>
> >
> > > So easiest fix here is to read back the period from the PWM provider via
> > > the provider's ->get_state() op, if implemented, which should provide the
> > > best matched period. Do this on probe after the first ->pwm_apply() op has
> > > been done, which will allow the provider to determine the best match
> > > period based on available configuration knobs. From there on, the
> > > backlight will use the best matched period, since the driver's internal
> > > PWM state is now synced up with the one from provider.
> > > [...]
> > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > index 237d3d3f3bb1a6d713c5f6ec3198af772bf1268c..71a3e9cd8844095e85c01b194d7466978f1ca78e 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -525,6 +525,17 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > goto err_alloc;
> > > }
> > >
> > > + /*
> > > + * The actual period might differ from the requested one due to HW
> > > + * limitations, so sync up the period with one determined by the
> > > + * provider driver.
> > > + */
> > > + ret = pwm_get_state_hw(pb->pwm, &pb->pwm->state);
> >
> > As a consumer you're not supposed to write to &pb->pwm->state. That's a
> > layer violation. Please call pwm_get_state_hw() with a struct pwm_state
> > that you own and save the relevant parts in your driver data.
>
> Yep, that is indeed wrong. Maybe making the pwm opaque might be a good idea as well.
>
> [1] Calling pwm_get_state_hw() would be wrong if the provider doesn't implement the ->get_state(),
> as I mentioned above.
>
> But are you suggesting we replace all calls to pwm_get_state() with
> pwm_get_state_hw() in pwm_bl?
No, I still didn't understand the problem you want to fix here. So I'm
not suggesting anything yet.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
