On 25-02-26 17:34:50, Uwe Kleine-König wrote:
> Hello,
>
> 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.
Now, the pwm_bl is not aware of this limitation and will request duty cycle values that
go above 4.26MHz.
>
> > 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().
Also, the pwm_get_state_hw() doesn't cache the state either.
This results in always having to call pwm_get_state_hw() before calling
pwm_apply_might_sleep().
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.
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?
I can do that, but the concern from [1] still stands.
>
> > + if (ret && ret != -EOPNOTSUPP) {
> > + dev_err(&pdev->dev, "failed to get PWM HW state");
> > + goto err_alloc;
> > + }
> > +
> > memset(&props, 0, sizeof(struct backlight_properties));
> >
> > if (data->levels) {
>
> Best regards
> Uwe
Thanks for reviewing,
Abel
