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

From: Enric Balletbo i Serra
Date: Tue Aug 07 2018 - 05:38:12 EST

Hi Daniel,

On 30/07/18 13:12, Daniel Thompson wrote:
> On Fri, Jul 27, 2018 at 05:11:21PM +0200, 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>
>> ---
>>
>> Changes in v2:
>> - Do not force the PWM be off in the first call to pwm_apply_state.
>> - Delayed applying the state until we know what the period is.
>> - Removed pb->period as after the conversion is not needed.
>
> Re-reading this I have spotted a couple of things I probably could have
> mentioned against v1... sorry.
>
> I think it's looking good though, I expect to be able to ack v3.
>
>
>> drivers/video/backlight/pwm_bl.c | 71 ++++++++++++++++++--------------
>> 1 file changed, 41 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index bdfcc0a71db1..dd1cb29b5332 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -28,7 +28,6 @@
>> struct pwm_bl_data {
>> struct pwm_device *pwm;
>> struct device *dev;
>> - unsigned int period;
>> unsigned int lth_brightness;
>> unsigned int *levels;
>> bool enabled;
>> @@ -46,7 +45,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 +57,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 +71,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 +82,10 @@ 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.duty_cycle = 0;
>> + pwm_apply_state(pb->pwm, &state);
>
> This is an in exact conversion because this code ignores a failure to
> set the duty cycle to zero whilst pwm_apply_state() does not.
>
> This would only matter if pwm_config() returns an error and given that a
> PWM which does not support a duty cycle of zero is permitted to adjust
> zero to the smallest supported value there is no *need* for a driver to
> return an error here. In other words... this is a subtle change of
> behaviour and perhaps (even probably) irrelevant.
>
> However I'm still interested whether you did any work to confirm or
> deny whether drivers that reports error on zero duty cycle actually
> exist.
>

Interesting, actually I don't have a use case for this, and I think that there
is nothing in the kernel. I know that some devices (like chromebook minnie and
jaq) the pwm must be >= 1% or 3% for the first non-zero value but I don't know
any where 0 is a problem.

>
>> regulator_disable(pb->power_supply);
>> pb->enabled = false;
>> @@ -89,14 +94,17 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>> static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
>> {
>> unsigned int lth = pb->lth_brightness;
>> + struct pwm_state state;
>> u64 duty_cycle;
>>
>> + pwm_get_state(pb->pwm, &state);
>> +
>> if (pb->levels)
>> duty_cycle = pb->levels[brightness];
>> else
>> duty_cycle = brightness;
>>
>> - duty_cycle *= pb->period - lth;
>> + duty_cycle *= state.period - lth;
>> do_div(duty_cycle, pb->scale);
>>
>> return duty_cycle + lth;
>> @@ -106,6 +114,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 +127,12 @@ 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);
>
> In principle the same subtle change applies here... but if pwm_config()
> reported an error here then the backlight probably didn't work before
> your change either so less need to worry about it!
>
>
>> - pwm_backlight_power_on(pb, brightness);
>> + pwm_get_state(pb->pwm, &state);
>> + state.duty_cycle = duty_cycle;
>> + if (!state.enabled)
>> + pwm_backlight_power_on(pb, &state);
>
> It verges towards nit picking but I don't really like the way a half updated
> state is shared between ...update_status and ...power_on.
>
> I'd rather it looked something like:
>
> pwm_get_state(pb->pwm, &state);
> if (!state.enabled) {
> pwm_backlight_power_on(pb); <-- no sharing here,
> make on match off
> } else {
> pwm_backlight_update_duty_cycle(pb, &state, brightness);
> pwm_apply_state(pb->pwm, &state);
> }
>
> (and have pwm_backlight_power_on() also call ...update_duty_cycle too)
>
> Thoughts?

What about something like this:

static int pwm_backlight_update_status(struct backlight_device *bl)
{
...

if (brightness > 0) {
pwm_get_state(pb->pwm, &state);
/* we can get rid of duty_cycle temporal variable */
state.duty_cycle = compute_duty_cycle(pb, brightness);
pwm_apply_state(pb->pwm, &state);
pwm_backlight_power_on(pb);
} else
pwm_backlight_power_off(pb);
...
}

static void pwm_backlight_power_on(struct pwm_bl_data *pb)
{
struct pwm_state state;

pwm_get_state(pb->pwm, &state);

if (state.enabled)
return;

...

state.enabled = true;
pwm_apply_state(pb->pwm, &state);

...
}

static void pwm_backlight_power_off(struct pwm_bl_data *pb)
{
struct pwm_state state;

...

pwm_get_state(pb->pwm, &state);
state.enabled = false;
state.duty_cycle = 0;
pwm_apply_state(pb->pwm, &state);

...
}

And I think that we can get rid of pb->enabled variable.

Best regards,
Enric

>
>
> Daniel.
>