Re: [RFC PATCH 08/15] backlight: pwm_bl: remove useless call to pwm_set_period

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4089 bytes --]

On Mon, Jul 20, 2015 at 11:57:04AM +0200, Boris Brezillon wrote:
> On Mon, 20 Jul 2015 11:10:04 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Mon, Jul 20, 2015 at 10:50:03AM +0200, Boris Brezillon wrote:
> > > On Mon, 20 Jul 2015 10:36:50 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > 
> > > > On Mon, Jul 20, 2015 at 10:21:43AM +0200, Boris Brezillon wrote:
> > > > > On Mon, 20 Jul 2015 10:16:00 +0200
> > > > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > 
> > > > > > On Wed, Jul 01, 2015 at 10:21:54AM +0200, Boris Brezillon wrote:
> > > > > > > The PWM period will be set when calling pwm_config. Remove this useless
> > > > > > > call to pwm_set_period, which might mess up with the initial PWM state
> > > > > > > once we have added proper support for PWM init state retrieval.
> > > > > > > 
> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > > > ---
> > > > > > >  drivers/video/backlight/pwm_bl.c | 4 +---
> > > > > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > > index ae498c1..fe5597c 100644
> > > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > > @@ -295,10 +295,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > > > > > >  	 * via the PWM lookup table.
> > > > > > >  	 */
> > > > > > >  	pb->period = pwm_get_default_period(pb->pwm);
> > > > > > > -	if (!pb->period && (data->pwm_period_ns > 0)) {
> > > > > > > +	if (!pb->period && (data->pwm_period_ns > 0))
> > > > > > >  		pb->period = data->pwm_period_ns;
> > > > > > > -		pwm_set_period(pb->pwm, data->pwm_period_ns);
> > > > > > > -	}
> > > > > > >  
> > > > > > >  	pb->lth_brightness = data->lth_brightness * (pb->period / pb->scale);
> > > > > > 
> > > > > > As far as I remember this line is there in order to pass in a period if
> > > > > > the backlight driver is initialized from board setup files. In such a
> > > > > > case there won't be an period associated with the PWM channel in the
> > > > > > first place.
> > > > > > 
> > > > > > I think even with the introduction of a default period, we'd be missing
> > > > > > out on the board setup case because there is no standard place where it
> > > > > > is being set, so it must come from the platform data.
> > > > > 
> > > > > AFAICT, we don't need to explicitly set the period when probing the
> > > > > backlight device, because it will be set next time we call
> > > > > pwm_config(), and since we're passing pb->period when calling
> > > > > pwm_config() everything should be fine.
> > > > 
> > > > Calling pwm_set_period() is still good for consistency. Consider for
> > > > example what happens if after the driver were to call pwm_get_period().
> > > > It would return some more or less random value (likely 0 or whatever it
> > > > had been set to by an earlier user).
> > > 
> > > Yes, that's true in general, but in this specific driver
> > > pwm_get_period() is never called, and the driver only relies on the
> > > pb->period value.
> > 
> > Perhaps that's something that should change. If the PWM core has all
> > this infrastructure there should be no need for the backlight driver to
> > keep it's own copy of that variable.
> 
> Yes, probably. In any case, I don't think we want PWM users to be able
> to mess up with the current or default PWM state, that's why I was
> planning on making the pwm_set_default_xxx helpers private to PWM
> drivers and core infrastructure.
> 
> Also note that if we keep this assignment it should at least be changed
> to a pwm_set_default_period() so that it does not override the current
> PWM state.

I think we should be able to live without the assignment. Perhaps when
replacing it, add a comment saying that this is for very legacy cases
only and that PWM lookup tables are the right way to fix this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]