| Message ID | 1435738921-25027-9-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Thierry
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.
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). Technically I think the most proper equivalent here would be to set the default state's period to data->pwm_period_ns, but I don't think that's proper to do. Perhaps since this is only relevant to boards where the backlight device is created from board setup code we don't have to care so much about messing up the initial state because either the board setup code has been carefully written to match what the bootloader set up, or because they don't match at all, in which case we don't have to worry anyway. Of course the right thing to do would be to replace all initialization of the data->pwm_period_ns by proper PWM lookup tables, but that's proven difficult in the past since very few people still have access to hardware where that code gets executed. Thierry
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. > > Technically I think the most proper equivalent here would be to set the > default state's period to data->pwm_period_ns, but I don't think that's > proper to do. Perhaps since this is only relevant to boards where the > backlight device is created from board setup code we don't have to care > so much about messing up the initial state because either the board > setup code has been carefully written to match what the bootloader set > up, or because they don't match at all, in which case we don't have to > worry anyway. IMHO, if we had to support default period values for non DT boards, the proper way would be to pass something in the PWM platform data and let the PWM driver (or PWM core) initialize the default PWM state. This way the PWM user could rely on the pwm_get_default_period() helper to extract the default period value.
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. > > Technically I think the most proper equivalent here would be to set the > > default state's period to data->pwm_period_ns, but I don't think that's > > proper to do. Perhaps since this is only relevant to boards where the > > backlight device is created from board setup code we don't have to care > > so much about messing up the initial state because either the board > > setup code has been carefully written to match what the bootloader set > > up, or because they don't match at all, in which case we don't have to > > worry anyway. > > IMHO, if we had to support default period values for non DT boards, the > proper way would be to pass something in the PWM platform data and let > the PWM driver (or PWM core) initialize the default PWM state. > This way the PWM user could rely on the pwm_get_default_period() helper > to extract the default period value. Yes, that's what PWM lookup tables are meant to address. I tried to convert existing users a number of times, but never got any replies and since it's board code I couldn't merge this through the PWM tree... Thierry
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.
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
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);
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(-)