Re: [PATCH] backlight: pwm_bl: apply the initial backlight state with sane defaults

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3533 bytes --]

Hi Uwe

On Tue, Sep 09, 2025 at 03:49:22PM +0200, Uwe Kleine-König wrote:
>On Fri, Aug 01, 2025 at 08:32:20AM +0200, Uwe Kleine-König wrote:
>> On Thu, Jul 31, 2025 at 10:47:18AM +0200, Michael Grzeschik wrote:
>> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> > index 237d3d3f3bb1a..5924e0b9f01e7 100644
>> > --- a/drivers/video/backlight/pwm_bl.c
>> > +++ b/drivers/video/backlight/pwm_bl.c
>> > @@ -518,13 +518,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>> >  	if (!state.period && (data->pwm_period_ns > 0))
>> >  		state.period = data->pwm_period_ns;
>> >
>> > -	ret = pwm_apply_might_sleep(pb->pwm, &state);
>> > -	if (ret) {
>> > -		dev_err_probe(&pdev->dev, ret,
>> > -			      "failed to apply initial PWM state");
>> > -		goto err_alloc;
>> > -	}
>> > -
>> >  	memset(&props, 0, sizeof(struct backlight_properties));
>> >
>> >  	if (data->levels) {
>> > @@ -582,6 +575,15 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>> >  	pb->lth_brightness = data->lth_brightness * (div_u64(state.period,
>> >  				pb->scale));
>> >
>> > +	state.duty_cycle = compute_duty_cycle(pb, data->dft_brightness, &state);
>> > +
>> > +	ret = pwm_apply_might_sleep(pb->pwm, &state);
>> > +	if (ret) {
>> > +		dev_err_probe(&pdev->dev, ret,
>> > +			      "failed to apply initial PWM state");
>> > +		goto err_alloc;
>> > +	}
>> > +
>>
>> I wonder why the PWM is updated at all in .probe(). Wouldn't it be the
>> natural thing to keep the PWM configured as it was (in its reset default
>> state or how the bootloader set it up)?
>>
>> Orthogonal to your change, while looking at the driver I wondered about:
>>
>>         bl = backlight_device_register(dev_name(&pdev->dev), &pdev->dev, pb,
>>                                        &pwm_backlight_ops, &props);
>>         if (IS_ERR(bl)) {
>>                 ret = dev_err_probe(&pdev->dev, PTR_ERR(bl),
>>                                     "failed to register backlight\n");
>>                 goto err_alloc;
>>         }
>>
>>         if (data->dft_brightness > data->max_brightness) {
>>                 dev_warn(&pdev->dev,
>>                          "invalid default brightness level: %u, using %u\n",
>>                          data->dft_brightness, data->max_brightness);
>>                 data->dft_brightness = data->max_brightness;
>>         }
>>
>>         bl->props.brightness = data->dft_brightness;
>>         bl->props.power = pwm_backlight_initial_power_state(pb);
>>         backlight_update_status(bl);
>>
>> Shoudn't setting data->dft_brightness, bl->props.brightness and
>> bl->props.power better happen before backlight_device_register()? Also
>> calling backlight_update_status() after backlight_device_register()
>> seems wrong to me, I'd claim the backend driver shouldn't call that.
>
>Do you intend to work on this orthogonal feedback? If not, I'll put it
>on my todo list.

Oh, yes, the orthogonal feedback. Thanks for the reminder of this
actually intended patch not being ready. I will work on this first. If you
like, please take the opurtunity to fix the issue you found.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]