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.
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
