| Message ID | 20191019083556.19466-4-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backlight: gpio: simplify the driver | expand |
On Sat, Oct 19, 2019 at 10:35:50AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > The GPIO backlight driver currently requests the line 'as is', without > acively setting its direction. This can lead to problems: if the line > is in input mode by default, we won't be able to drive it later when > updating the status and also reading its initial value doesn't make > sense for backlight setting. > > Request the line 'as is' initially, so that we can read its value > without affecting it but then change the direction to output explicitly > when setting the initial brightness. > > Also: check the current direction and only read the value if it's output. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Intent looks good to me but... > --- > drivers/video/backlight/gpio_backlight.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c > index 3955b513f2f8..a36ac3a45b81 100644 > --- a/drivers/video/backlight/gpio_backlight.c > +++ b/drivers/video/backlight/gpio_backlight.c > @@ -25,9 +25,8 @@ struct gpio_backlight { > int def_value; > }; > > -static int gpio_backlight_update_status(struct backlight_device *bl) > +static int gpio_backlight_get_curr_brightness(struct backlight_device *bl) This function does not get the current brightness (e.g. what the hardware is currently doing). Given we've just nuked the function that *did* get the current brightness from the hardware this isn't an acceptable name. Would like something like calc_brightness() or get_next_brightness(). Daniel.
pon., 21 paź 2019 o 12:45 Daniel Thompson <daniel.thompson@linaro.org> napisał(a): > > On Sat, Oct 19, 2019 at 10:35:50AM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > The GPIO backlight driver currently requests the line 'as is', without > > acively setting its direction. This can lead to problems: if the line > > is in input mode by default, we won't be able to drive it later when > > updating the status and also reading its initial value doesn't make > > sense for backlight setting. > > > > Request the line 'as is' initially, so that we can read its value > > without affecting it but then change the direction to output explicitly > > when setting the initial brightness. > > > > Also: check the current direction and only read the value if it's output. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Intent looks good to me but... > > > --- > > drivers/video/backlight/gpio_backlight.c | 23 ++++++++++++++++++----- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c > > index 3955b513f2f8..a36ac3a45b81 100644 > > --- a/drivers/video/backlight/gpio_backlight.c > > +++ b/drivers/video/backlight/gpio_backlight.c > > @@ -25,9 +25,8 @@ struct gpio_backlight { > > int def_value; > > }; > > > > -static int gpio_backlight_update_status(struct backlight_device *bl) > > +static int gpio_backlight_get_curr_brightness(struct backlight_device *bl) > > This function does not get the current brightness (e.g. what the > hardware is currently doing). Given we've just nuked the function that > *did* get the current brightness from the hardware this isn't an > acceptable name. > > Would like something like calc_brightness() or get_next_brightness(). > Fair enough, the latter sounds good in this case. Bart
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c index 3955b513f2f8..a36ac3a45b81 100644 --- a/drivers/video/backlight/gpio_backlight.c +++ b/drivers/video/backlight/gpio_backlight.c @@ -25,9 +25,8 @@ struct gpio_backlight { int def_value; }; -static int gpio_backlight_update_status(struct backlight_device *bl) +static int gpio_backlight_get_curr_brightness(struct backlight_device *bl) { - struct gpio_backlight *gbl = bl_get_data(bl); int brightness = bl->props.brightness; if (bl->props.power != FB_BLANK_UNBLANK || @@ -35,6 +34,14 @@ static int gpio_backlight_update_status(struct backlight_device *bl) bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK)) brightness = 0; + return brightness; +} + +static int gpio_backlight_update_status(struct backlight_device *bl) +{ + struct gpio_backlight *gbl = bl_get_data(bl); + int brightness = gpio_backlight_get_curr_brightness(bl); + gpiod_set_value_cansleep(gbl->gpiod, brightness); return 0; @@ -85,7 +92,8 @@ static int gpio_backlight_initial_power_state(struct gpio_backlight *gbl) return gbl->def_value ? FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN; /* if the enable GPIO is disabled, do not enable the backlight */ - if (gpiod_get_value_cansleep(gbl->gpiod) == 0) + if (gpiod_get_direction(gbl->gpiod) == 0 && + gpiod_get_value_cansleep(gbl->gpiod) == 0) return FB_BLANK_POWERDOWN; return FB_BLANK_UNBLANK; @@ -98,7 +106,7 @@ static int gpio_backlight_probe(struct platform_device *pdev) struct backlight_properties props; struct backlight_device *bl; struct gpio_backlight *gbl; - int ret; + int ret, init_brightness; gbl = devm_kzalloc(&pdev->dev, sizeof(*gbl), GFP_KERNEL); if (gbl == NULL) @@ -151,7 +159,12 @@ static int gpio_backlight_probe(struct platform_device *pdev) bl->props.power = gpio_backlight_initial_power_state(gbl); bl->props.brightness = 1; - backlight_update_status(bl); + init_brightness = gpio_backlight_get_curr_brightness(bl); + ret = gpiod_direction_output(gbl->gpiod, init_brightness); + if (ret) { + dev_err(&pdev->dev, "failed to set initial brightness\n"); + return ret; + } platform_set_drvdata(pdev, bl); return 0;