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