[v4,7/7] backlight: gpio: pull gpio_backlight_initial_power_state() into probe
diff mbox series

Message ID 20191001125837.4472-8-brgl@bgdev.pl
State New
Headers show
Series
  • backlight: gpio: simplify the driver
Related show

Commit Message

Bartosz Golaszewski Oct. 1, 2019, 12:58 p.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The probe function in the gpio-backlight driver is quite short. If we
pull gpio_backlight_initial_power_state() into probe we can drop two
more fields from struct gpio_backlight and shrink the driver code.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/video/backlight/gpio_backlight.c | 36 ++++++++----------------
 1 file changed, 12 insertions(+), 24 deletions(-)

Comments

Daniel Thompson Oct. 2, 2019, 10:33 a.m. UTC | #1
On Tue, Oct 01, 2019 at 02:58:37PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The probe function in the gpio-backlight driver is quite short. If we
> pull gpio_backlight_initial_power_state() into probe we can drop two
> more fields from struct gpio_backlight and shrink the driver code.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/video/backlight/gpio_backlight.c | 36 ++++++++----------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> index 6247687b6330..37ec184f0c5c 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -17,11 +17,8 @@
>  #include <linux/slab.h>
>  
>  struct gpio_backlight {
> -	struct device *dev;
>  	struct device *fbdev;
> -
>  	struct gpio_desc *gpiod;
> -	int def_value;
>  };
>  
>  static int gpio_backlight_update_status(struct backlight_device *bl)
> @@ -53,41 +50,24 @@ static const struct backlight_ops gpio_backlight_ops = {
>  	.check_fb	= gpio_backlight_check_fb,
>  };
>  
> -static int gpio_backlight_initial_power_state(struct gpio_backlight *gbl)

I'm inclined to view deleting this function as removing a comment (e.g.
the function name helps us to read the code)!

Removing the variables from the context structure is good but why not
just pass them to the function and let the compiler decided whether or
not to inline.


Daniel.


> -{
> -	struct device_node *node = gbl->dev->of_node;
> -
> -	/* Not booted with device tree or no phandle link to the node */
> -	if (!node || !node->phandle)
> -		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)
> -		return FB_BLANK_POWERDOWN;
> -
> -	return FB_BLANK_UNBLANK;
> -}
> -
> -
>  static int gpio_backlight_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev);
> +	struct device_node *of_node = dev->of_node;
>  	struct backlight_properties props;
>  	struct backlight_device *bl;
>  	struct gpio_backlight *gbl;
> -	int ret;
> +	int ret, def_value;
>  
>  	gbl = devm_kzalloc(dev, sizeof(*gbl), GFP_KERNEL);
>  	if (gbl == NULL)
>  		return -ENOMEM;
>  
> -	gbl->dev = dev;
> -
>  	if (pdata)
>  		gbl->fbdev = pdata->fbdev;
>  
> -	gbl->def_value = device_property_read_bool(dev, "default-on");
> +	def_value = device_property_read_bool(dev, "default-on");
>  
>  	gbl->gpiod = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
>  	if (IS_ERR(gbl->gpiod)) {
> @@ -109,7 +89,15 @@ static int gpio_backlight_probe(struct platform_device *pdev)
>  		return PTR_ERR(bl);
>  	}
>  
> -	bl->props.power = gpio_backlight_initial_power_state(gbl);
> +	/* Not booted with device tree or no phandle link to the node */
> +	if (!of_node || !of_node->phandle)
> +		bl->props.power = def_value ? FB_BLANK_UNBLANK
> +					    : FB_BLANK_POWERDOWN;
> +	else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
> +		bl->props.power = FB_BLANK_POWERDOWN;
> +	else
> +		bl->props.power = FB_BLANK_UNBLANK;
> +
>  	bl->props.brightness = 1;
>  
>  	backlight_update_status(bl);
> -- 
> 2.23.0
>
Bartosz Golaszewski Oct. 2, 2019, 11:46 a.m. UTC | #2
śr., 2 paź 2019 o 12:33 Daniel Thompson <daniel.thompson@linaro.org> napisał(a):
>
> On Tue, Oct 01, 2019 at 02:58:37PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The probe function in the gpio-backlight driver is quite short. If we
> > pull gpio_backlight_initial_power_state() into probe we can drop two
> > more fields from struct gpio_backlight and shrink the driver code.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  drivers/video/backlight/gpio_backlight.c | 36 ++++++++----------------
> >  1 file changed, 12 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> > index 6247687b6330..37ec184f0c5c 100644
> > --- a/drivers/video/backlight/gpio_backlight.c
> > +++ b/drivers/video/backlight/gpio_backlight.c
> > @@ -17,11 +17,8 @@
> >  #include <linux/slab.h>
> >
> >  struct gpio_backlight {
> > -     struct device *dev;
> >       struct device *fbdev;
> > -
> >       struct gpio_desc *gpiod;
> > -     int def_value;
> >  };
> >
> >  static int gpio_backlight_update_status(struct backlight_device *bl)
> > @@ -53,41 +50,24 @@ static const struct backlight_ops gpio_backlight_ops = {
> >       .check_fb       = gpio_backlight_check_fb,
> >  };
> >
> > -static int gpio_backlight_initial_power_state(struct gpio_backlight *gbl)
>
> I'm inclined to view deleting this function as removing a comment (e.g.
> the function name helps us to read the code)!
>

Right, but why not just add a comment then? The probe function is 50
lines long, there's really no need to split it. This will get inlined
anyway too.

Bart

> Removing the variables from the context structure is good but why not
> just pass them to the function and let the compiler decided whether or
> not to inline.
>
>
> Daniel.
>
>
> > -{
> > -     struct device_node *node = gbl->dev->of_node;
> > -
> > -     /* Not booted with device tree or no phandle link to the node */
> > -     if (!node || !node->phandle)
> > -             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)
> > -             return FB_BLANK_POWERDOWN;
> > -
> > -     return FB_BLANK_UNBLANK;
> > -}
> > -
> > -
> >  static int gpio_backlight_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> >       struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev);
> > +     struct device_node *of_node = dev->of_node;
> >       struct backlight_properties props;
> >       struct backlight_device *bl;
> >       struct gpio_backlight *gbl;
> > -     int ret;
> > +     int ret, def_value;
> >
> >       gbl = devm_kzalloc(dev, sizeof(*gbl), GFP_KERNEL);
> >       if (gbl == NULL)
> >               return -ENOMEM;
> >
> > -     gbl->dev = dev;
> > -
> >       if (pdata)
> >               gbl->fbdev = pdata->fbdev;
> >
> > -     gbl->def_value = device_property_read_bool(dev, "default-on");
> > +     def_value = device_property_read_bool(dev, "default-on");
> >
> >       gbl->gpiod = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> >       if (IS_ERR(gbl->gpiod)) {
> > @@ -109,7 +89,15 @@ static int gpio_backlight_probe(struct platform_device *pdev)
> >               return PTR_ERR(bl);
> >       }
> >
> > -     bl->props.power = gpio_backlight_initial_power_state(gbl);
> > +     /* Not booted with device tree or no phandle link to the node */
> > +     if (!of_node || !of_node->phandle)
> > +             bl->props.power = def_value ? FB_BLANK_UNBLANK
> > +                                         : FB_BLANK_POWERDOWN;
> > +     else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
> > +             bl->props.power = FB_BLANK_POWERDOWN;
> > +     else
> > +             bl->props.power = FB_BLANK_UNBLANK;
> > +
> >       bl->props.brightness = 1;
> >
> >       backlight_update_status(bl);
> > --
> > 2.23.0
> >
Daniel Thompson Oct. 2, 2019, 2:40 p.m. UTC | #3
On Wed, Oct 02, 2019 at 01:46:17PM +0200, Bartosz Golaszewski wrote:
> śr., 2 paź 2019 o 12:33 Daniel Thompson <daniel.thompson@linaro.org> napisał(a):
> >
> > On Tue, Oct 01, 2019 at 02:58:37PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > The probe function in the gpio-backlight driver is quite short. If we
> > > pull gpio_backlight_initial_power_state() into probe we can drop two
> > > more fields from struct gpio_backlight and shrink the driver code.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > >  drivers/video/backlight/gpio_backlight.c | 36 ++++++++----------------
> > >  1 file changed, 12 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> > > index 6247687b6330..37ec184f0c5c 100644
> > > --- a/drivers/video/backlight/gpio_backlight.c
> > > +++ b/drivers/video/backlight/gpio_backlight.c
> > > @@ -17,11 +17,8 @@
> > >  #include <linux/slab.h>
> > >
> > >  struct gpio_backlight {
> > > -     struct device *dev;
> > >       struct device *fbdev;
> > > -
> > >       struct gpio_desc *gpiod;
> > > -     int def_value;
> > >  };
> > >
> > >  static int gpio_backlight_update_status(struct backlight_device *bl)
> > > @@ -53,41 +50,24 @@ static const struct backlight_ops gpio_backlight_ops = {
> > >       .check_fb       = gpio_backlight_check_fb,
> > >  };
> > >
> > > -static int gpio_backlight_initial_power_state(struct gpio_backlight *gbl)
> >
> > I'm inclined to view deleting this function as removing a comment (e.g.
> > the function name helps us to read the code)!
> >
> 
> Right, but why not just add a comment then?

I guess you could add a comment but keeping it pulled out in a function
makes it easier to compare against equivalent code in other drivers
(such as pwm_bl).


Daniel.


> The probe function is 50
> lines long, there's really no need to split it. This will get inlined
> anyway too.
> 
> Bart
> 
> > Removing the variables from the context structure is good but why not
> > just pass them to the function and let the compiler decided whether or
> > not to inline.
> >
> >
> > Daniel.
> >
> >
> > > -{
> > > -     struct device_node *node = gbl->dev->of_node;
> > > -
> > > -     /* Not booted with device tree or no phandle link to the node */
> > > -     if (!node || !node->phandle)
> > > -             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)
> > > -             return FB_BLANK_POWERDOWN;
> > > -
> > > -     return FB_BLANK_UNBLANK;
> > > -}
> > > -
> > > -
> > >  static int gpio_backlight_probe(struct platform_device *pdev)
> > >  {
> > >       struct device *dev = &pdev->dev;
> > >       struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev);
> > > +     struct device_node *of_node = dev->of_node;
> > >       struct backlight_properties props;
> > >       struct backlight_device *bl;
> > >       struct gpio_backlight *gbl;
> > > -     int ret;
> > > +     int ret, def_value;
> > >
> > >       gbl = devm_kzalloc(dev, sizeof(*gbl), GFP_KERNEL);
> > >       if (gbl == NULL)
> > >               return -ENOMEM;
> > >
> > > -     gbl->dev = dev;
> > > -
> > >       if (pdata)
> > >               gbl->fbdev = pdata->fbdev;
> > >
> > > -     gbl->def_value = device_property_read_bool(dev, "default-on");
> > > +     def_value = device_property_read_bool(dev, "default-on");
> > >
> > >       gbl->gpiod = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> > >       if (IS_ERR(gbl->gpiod)) {
> > > @@ -109,7 +89,15 @@ static int gpio_backlight_probe(struct platform_device *pdev)
> > >               return PTR_ERR(bl);
> > >       }
> > >
> > > -     bl->props.power = gpio_backlight_initial_power_state(gbl);
> > > +     /* Not booted with device tree or no phandle link to the node */
> > > +     if (!of_node || !of_node->phandle)
> > > +             bl->props.power = def_value ? FB_BLANK_UNBLANK
> > > +                                         : FB_BLANK_POWERDOWN;
> > > +     else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
> > > +             bl->props.power = FB_BLANK_POWERDOWN;
> > > +     else
> > > +             bl->props.power = FB_BLANK_UNBLANK;
> > > +
> > >       bl->props.brightness = 1;
> > >
> > >       backlight_update_status(bl);
> > > --
> > > 2.23.0
> > >
Bartosz Golaszewski Oct. 3, 2019, 8:42 a.m. UTC | #4
śr., 2 paź 2019 o 16:40 Daniel Thompson <daniel.thompson@linaro.org> napisał(a):
>
> On Wed, Oct 02, 2019 at 01:46:17PM +0200, Bartosz Golaszewski wrote:
> > śr., 2 paź 2019 o 12:33 Daniel Thompson <daniel.thompson@linaro.org> napisał(a):
> > >
> > > On Tue, Oct 01, 2019 at 02:58:37PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > The probe function in the gpio-backlight driver is quite short. If we
> > > > pull gpio_backlight_initial_power_state() into probe we can drop two
> > > > more fields from struct gpio_backlight and shrink the driver code.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > ---
> > > >  drivers/video/backlight/gpio_backlight.c | 36 ++++++++----------------
> > > >  1 file changed, 12 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> > > > index 6247687b6330..37ec184f0c5c 100644
> > > > --- a/drivers/video/backlight/gpio_backlight.c
> > > > +++ b/drivers/video/backlight/gpio_backlight.c
> > > > @@ -17,11 +17,8 @@
> > > >  #include <linux/slab.h>
> > > >
> > > >  struct gpio_backlight {
> > > > -     struct device *dev;
> > > >       struct device *fbdev;
> > > > -
> > > >       struct gpio_desc *gpiod;
> > > > -     int def_value;
> > > >  };
> > > >
> > > >  static int gpio_backlight_update_status(struct backlight_device *bl)
> > > > @@ -53,41 +50,24 @@ static const struct backlight_ops gpio_backlight_ops = {
> > > >       .check_fb       = gpio_backlight_check_fb,
> > > >  };
> > > >
> > > > -static int gpio_backlight_initial_power_state(struct gpio_backlight *gbl)
> > >
> > > I'm inclined to view deleting this function as removing a comment (e.g.
> > > the function name helps us to read the code)!
> > >
> >
> > Right, but why not just add a comment then?
>
> I guess you could add a comment but keeping it pulled out in a function
> makes it easier to compare against equivalent code in other drivers
> (such as pwm_bl).
>

The pwm driver seems to be the only one that has this function and
it's also much more complicated. Unless it's a hard NACK, I think that
pulling all initialization into probe looks better and shrinks the
driver visually.

Bart

>
> Daniel.
>
>
> > The probe function is 50
> > lines long, there's really no need to split it. This will get inlined
> > anyway too.
> >
> > Bart
> >
> > > Removing the variables from the context structure is good but why not
> > > just pass them to the function and let the compiler decided whether or
> > > not to inline.
> > >
> > >
> > > Daniel.
> > >
> > >
> > > > -{
> > > > -     struct device_node *node = gbl->dev->of_node;
> > > > -
> > > > -     /* Not booted with device tree or no phandle link to the node */
> > > > -     if (!node || !node->phandle)
> > > > -             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)
> > > > -             return FB_BLANK_POWERDOWN;
> > > > -
> > > > -     return FB_BLANK_UNBLANK;
> > > > -}
> > > > -
> > > > -
> > > >  static int gpio_backlight_probe(struct platform_device *pdev)
> > > >  {
> > > >       struct device *dev = &pdev->dev;
> > > >       struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev);
> > > > +     struct device_node *of_node = dev->of_node;
> > > >       struct backlight_properties props;
> > > >       struct backlight_device *bl;
> > > >       struct gpio_backlight *gbl;
> > > > -     int ret;
> > > > +     int ret, def_value;
> > > >
> > > >       gbl = devm_kzalloc(dev, sizeof(*gbl), GFP_KERNEL);
> > > >       if (gbl == NULL)
> > > >               return -ENOMEM;
> > > >
> > > > -     gbl->dev = dev;
> > > > -
> > > >       if (pdata)
> > > >               gbl->fbdev = pdata->fbdev;
> > > >
> > > > -     gbl->def_value = device_property_read_bool(dev, "default-on");
> > > > +     def_value = device_property_read_bool(dev, "default-on");
> > > >
> > > >       gbl->gpiod = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> > > >       if (IS_ERR(gbl->gpiod)) {
> > > > @@ -109,7 +89,15 @@ static int gpio_backlight_probe(struct platform_device *pdev)
> > > >               return PTR_ERR(bl);
> > > >       }
> > > >
> > > > -     bl->props.power = gpio_backlight_initial_power_state(gbl);
> > > > +     /* Not booted with device tree or no phandle link to the node */
> > > > +     if (!of_node || !of_node->phandle)
> > > > +             bl->props.power = def_value ? FB_BLANK_UNBLANK
> > > > +                                         : FB_BLANK_POWERDOWN;
> > > > +     else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
> > > > +             bl->props.power = FB_BLANK_POWERDOWN;
> > > > +     else
> > > > +             bl->props.power = FB_BLANK_UNBLANK;
> > > > +
> > > >       bl->props.brightness = 1;
> > > >
> > > >       backlight_update_status(bl);
> > > > --
> > > > 2.23.0
> > > >

Patch
diff mbox series

diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index 6247687b6330..37ec184f0c5c 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -17,11 +17,8 @@ 
 #include <linux/slab.h>
 
 struct gpio_backlight {
-	struct device *dev;
 	struct device *fbdev;
-
 	struct gpio_desc *gpiod;
-	int def_value;
 };
 
 static int gpio_backlight_update_status(struct backlight_device *bl)
@@ -53,41 +50,24 @@  static const struct backlight_ops gpio_backlight_ops = {
 	.check_fb	= gpio_backlight_check_fb,
 };
 
-static int gpio_backlight_initial_power_state(struct gpio_backlight *gbl)
-{
-	struct device_node *node = gbl->dev->of_node;
-
-	/* Not booted with device tree or no phandle link to the node */
-	if (!node || !node->phandle)
-		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)
-		return FB_BLANK_POWERDOWN;
-
-	return FB_BLANK_UNBLANK;
-}
-
-
 static int gpio_backlight_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct gpio_backlight_platform_data *pdata = dev_get_platdata(dev);
+	struct device_node *of_node = dev->of_node;
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct gpio_backlight *gbl;
-	int ret;
+	int ret, def_value;
 
 	gbl = devm_kzalloc(dev, sizeof(*gbl), GFP_KERNEL);
 	if (gbl == NULL)
 		return -ENOMEM;
 
-	gbl->dev = dev;
-
 	if (pdata)
 		gbl->fbdev = pdata->fbdev;
 
-	gbl->def_value = device_property_read_bool(dev, "default-on");
+	def_value = device_property_read_bool(dev, "default-on");
 
 	gbl->gpiod = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
 	if (IS_ERR(gbl->gpiod)) {
@@ -109,7 +89,15 @@  static int gpio_backlight_probe(struct platform_device *pdev)
 		return PTR_ERR(bl);
 	}
 
-	bl->props.power = gpio_backlight_initial_power_state(gbl);
+	/* Not booted with device tree or no phandle link to the node */
+	if (!of_node || !of_node->phandle)
+		bl->props.power = def_value ? FB_BLANK_UNBLANK
+					    : FB_BLANK_POWERDOWN;
+	else if (gpiod_get_value_cansleep(gbl->gpiod) == 0)
+		bl->props.power = FB_BLANK_POWERDOWN;
+	else
+		bl->props.power = FB_BLANK_UNBLANK;
+
 	bl->props.brightness = 1;
 
 	backlight_update_status(bl);