Re: [PATCH] backlight: pwm_bl: switch to using "atomic" PWM API

[Enric Balletbo i Serra]

Hi Daniel,

Thanks for reviewing the patch.

On 27/07/18 13:32, Daniel Thompson wrote:
> On 26/07/18 10:15, Enric Balletbo i Serra wrote:
>> The "atomic" API allows us to configure PWM period and duty_cycle and
>> enable it in one call.
>>
>> The patch also moves the pwm_init_state just before any use of the
>> pwm_state struct, this fixes a potential bug where pwm_get_state
>> can be called before pwm_init_state.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>> ---
>>
>> Â drivers/video/backlight/pwm_bl.c | 48 ++++++++++++++++++++------------
>> Â 1 file changed, 30 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index bdfcc0a71db1..2c734d55d607 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -46,7 +46,8 @@ struct pwm_bl_data {
>> ÂÂÂÂÂ voidÂÂÂÂÂÂÂÂÂÂÂ (*exit)(struct device *);
>> Â };
>> Â -static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>> +static void pwm_backlight_power_on(struct pwm_bl_data *pb,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct pwm_state *state)
>> Â {
>> ÂÂÂÂÂ int err;
>> Â @@ -57,7 +58,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb,
>> int brightness)
>> ÂÂÂÂÂ if (err < 0)
>> ÂÂÂÂÂÂÂÂÂ dev_err(pb->dev, "failed to enable power supply\n");
>> Â -ÂÂÂ pwm_enable(pb->pwm);
>> +ÂÂÂ state->enabled = true;
>> +ÂÂÂ pwm_apply_state(pb->pwm, state);
>> Â ÂÂÂÂÂ if (pb->post_pwm_on_delay)
>> ÂÂÂÂÂÂÂÂÂ msleep(pb->post_pwm_on_delay);
>> @@ -70,6 +72,8 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb,
>> int brightness)
>> Â Â static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>> Â {
>> +ÂÂÂ struct pwm_state state;
>> +
>> ÂÂÂÂÂ if (!pb->enabled)
>> ÂÂÂÂÂÂÂÂÂ return;
>> Â @@ -79,8 +83,11 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>> ÂÂÂÂÂ if (pb->pwm_off_delay)
>> ÂÂÂÂÂÂÂÂÂ msleep(pb->pwm_off_delay);
>> Â -ÂÂÂ pwm_config(pb->pwm, 0, pb->period);
>> -ÂÂÂ pwm_disable(pb->pwm);
>> +ÂÂÂ pwm_get_state(pb->pwm, &state);
>> +ÂÂÂ state.enabled = false;
>> +ÂÂÂ state.period = pb->period;
>> +ÂÂÂ state.duty_cycle = 0;
>> +ÂÂÂ pwm_apply_state(pb->pwm, &state);
>> Â ÂÂÂÂÂ regulator_disable(pb->power_supply);
>> ÂÂÂÂÂ pb->enabled = false;
>> @@ -106,6 +113,7 @@ static int pwm_backlight_update_status(struct
>> backlight_device *bl)
>> Â {
>> ÂÂÂÂÂ struct pwm_bl_data *pb = bl_get_data(bl);
>> ÂÂÂÂÂ int brightness = bl->props.brightness;
>> +ÂÂÂ struct pwm_state state;
>> ÂÂÂÂÂ int duty_cycle;
>> Â ÂÂÂÂÂ if (bl->props.power != FB_BLANK_UNBLANK ||
>> @@ -118,8 +126,13 @@ static int pwm_backlight_update_status(struct
>> backlight_device *bl)
>> Â ÂÂÂÂÂ if (brightness > 0) {
>> ÂÂÂÂÂÂÂÂÂ duty_cycle = compute_duty_cycle(pb, brightness);
>> -ÂÂÂÂÂÂÂ pwm_config(pb->pwm, duty_cycle, pb->period);
>> -ÂÂÂÂÂÂÂ pwm_backlight_power_on(pb, brightness);
>> +ÂÂÂÂÂÂÂ pwm_get_state(pb->pwm, &state);
>> +ÂÂÂÂÂÂÂ state.duty_cycle = duty_cycle;
>> +ÂÂÂÂÂÂÂ state.period = pb->period;
>> +ÂÂÂÂÂÂÂ if (!state.enabled)
>> +ÂÂÂÂÂÂÂÂÂÂÂ pwm_backlight_power_on(pb, &state);
>> +ÂÂÂÂÂÂÂ else
>> +ÂÂÂÂÂÂÂÂÂÂÂ pwm_apply_state(pb->pwm, &state);
>> ÂÂÂÂÂ } else
>> ÂÂÂÂÂÂÂÂÂ pwm_backlight_power_off(pb);
>> Â @@ -447,7 +460,6 @@ static int pwm_backlight_probe(struct platform_device
>> *pdev)
>> ÂÂÂÂÂ struct device_node *node = pdev->dev.of_node;
>> ÂÂÂÂÂ struct pwm_bl_data *pb;
>> ÂÂÂÂÂ struct pwm_state state;
>> -ÂÂÂ struct pwm_args pargs;
>> ÂÂÂÂÂ unsigned int i;
>> ÂÂÂÂÂ int ret;
>> Â @@ -539,10 +551,17 @@ static int pwm_backlight_probe(struct platform_device
>> *pdev)
>> Â ÂÂÂÂÂ dev_dbg(&pdev->dev, "got pwm for backlight\n");
>> Â -ÂÂÂ if (!data->levels) {
>> -ÂÂÂÂÂÂÂ /* Get the PWM period (in nanoseconds) */
>> -ÂÂÂÂÂÂÂ pwm_get_state(pb->pwm, &state);
>> +ÂÂÂ /* Sync up PWM state and ensure it is off. */
>> +ÂÂÂ pwm_init_state(pb->pwm, &state);
>> +ÂÂÂ state.enabled = false;
>> +ÂÂÂ ret = pwm_apply_state(pb->pwm, &state);
> 
> Why do we ensure the PWM is off? Does this cause backlight flickers or make some
> of the code in pwm_backlight_initial_power_state() unreachable?
> 

No, I think that I can just remove that line.

> 
>> +ÂÂÂ if (ret) {
>> +ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
>> +ÂÂÂÂÂÂÂÂÂÂÂ ret);
>> +ÂÂÂÂÂÂÂ goto err_alloc;
>> +ÂÂÂ }
>> Â +ÂÂÂ if (!data->levels) {
>> ÂÂÂÂÂÂÂÂÂ ret = pwm_backlight_brightness_default(&pdev->dev, data,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ state.period);
>> ÂÂÂÂÂÂÂÂÂ if (ret < 0) {
>> @@ -559,20 +578,13 @@ static int pwm_backlight_probe(struct platform_device
>> *pdev)
>> ÂÂÂÂÂÂÂÂÂ pb->levels = data->levels;
>> ÂÂÂÂÂ }
>> Â -ÂÂÂ /*
>> -ÂÂÂÂ * FIXME: pwm_apply_args() should be removed when switching to
>> -ÂÂÂÂ * the atomic PWM API.
>> -ÂÂÂÂ */
>> -ÂÂÂ pwm_apply_args(pb->pwm);
>> -
>> ÂÂÂÂÂ /*
>> ÂÂÂÂÂÂ * The DT case will set the pwm_period_ns field to 0 and store the
>> ÂÂÂÂÂÂ * period, parsed from the DT, in the PWM device. For the non-DT case,
>> ÂÂÂÂÂÂ * set the period from platform data if it has not already been set
>> ÂÂÂÂÂÂ * via the PWM lookup table.
>> ÂÂÂÂÂÂ */
>> -ÂÂÂ pwm_get_args(pb->pwm, &pargs);
>> -ÂÂÂ pb->period = pargs.period;
>> +ÂÂÂ pb->period = state.period;
>> ÂÂÂÂÂ if (!pb->period && (data->pwm_period_ns > 0))
>> ÂÂÂÂÂÂÂÂÂ pb->period = data->pwm_period_ns;
> 
> Could we have delayed applying the state until we know what the period is
> supposed to be? No other call to pwm_apply_state() has its error value
> checked... so if there are problems with the period we could detect them here.
> 

Yes, I can move this code before 'if (!data->levels)' and call pwm_apply_state
after.

> Note also that we can guarantee the period is set before the probe completes
> then I think pb->period could be removed entirely. It was only really being
> carried around to help with calls to pwm_config() and these no longer exist.
> 

Right, I think that I can also remove pb->period. I'll send a second version soon.

Thanks,
 Enric
> 
> Daniel.
>