On Tue, Aug 07, 2018 at 11:38:04AM +0200, Enric Balletbo i Serra wrote:
> 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);
> ...
> }
This reads very well. I'm happy to go with this approach.
> 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.
> >
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
