diff mbox series

[2/2] backlight: gpio: delete pdata inversion

Message ID 20170430081745.14364-1-linus.walleij@linaro.org
State Superseded
Headers show
Series None | expand

Commit Message

Linus Walleij April 30, 2017, 8:17 a.m. UTC 
The option to invert the output of the GPIO (active low) is
not used by the only platform still using platform data to
set up a GPIO backlight (one SH board). Delete the option
as we do not expect to expand the use of board files for
this driver, and GPIO descriptors intrinsically keep track
of any signal inversion.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/video/backlight/gpio_backlight.c     | 15 ++-------------
 include/linux/platform_data/gpio_backlight.h |  1 -
 2 files changed, 2 insertions(+), 14 deletions(-)

-- 
2.9.3

Comments

Linus Walleij May 22, 2017, 8:45 a.m. UTC | #1 
On Sun, Apr 30, 2017 at 10:17 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:

> The option to invert the output of the GPIO (active low) is

> not used by the only platform still using platform data to

> set up a GPIO backlight (one SH board). Delete the option

> as we do not expect to expand the use of board files for

> this driver, and GPIO descriptors intrinsically keep track

> of any signal inversion.

>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


No reaction on this patch since 21 days so sending an
unsolicited ping.

Yours,
Linus Walleij
Daniel Thompson May 23, 2017, 2:15 p.m. UTC | #2 
On 30/04/17 09:17, Linus Walleij wrote:
> The option to invert the output of the GPIO (active low) is

> not used by the only platform still using platform data to

> set up a GPIO backlight (one SH board). Delete the option

> as we do not expect to expand the use of board files for

> this driver, and GPIO descriptors intrinsically keep track

> of any signal inversion.

> 

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


I note that only ~40% of Linux architectures currently have DT support 
at this point... There are still a few "embedded" ones that make do with 
platform bus (although they are growing increasingly niche).

Having said that I can't argue with "it's not used anywhere" (I've only 
really posted the above paragraph as a hint for future archeology... 
since it might be me doing the digging):
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>



Daniel.


> ---

>   drivers/video/backlight/gpio_backlight.c     | 15 ++-------------

>   include/linux/platform_data/gpio_backlight.h |  1 -

>   2 files changed, 2 insertions(+), 14 deletions(-)

> 

> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c

> index 17dd8071ad4d..8c19dc6c9236 100644

> --- a/drivers/video/backlight/gpio_backlight.c

> +++ b/drivers/video/backlight/gpio_backlight.c

> @@ -25,7 +25,6 @@ struct gpio_backlight {

>   	struct device *fbdev;

>   

>   	struct gpio_desc *gpiod;

> -	int active;

>   	int def_value;

>   };

>   

> @@ -39,8 +38,7 @@ static int gpio_backlight_update_status(struct backlight_device *bl)

>   	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))

>   		brightness = 0;

>   

> -	gpiod_set_value_cansleep(gbl->gpiod,

> -				 brightness ? gbl->active : !gbl->active);

> +	gpiod_set_value_cansleep(gbl->gpiod, brightness);

>   

>   	return 0;

>   }

> @@ -72,8 +70,6 @@ static int gpio_backlight_probe_dt(struct platform_device *pdev,

>   		flags = GPIOD_OUT_HIGH;

>   	else

>   		flags = GPIOD_OUT_LOW;

> -	/* GPIO descriptors keep track of inversion */

> -	gbl->active = 1;

>   

>   	gbl->gpiod = devm_gpiod_get(dev, NULL, flags);

>   	if (IS_ERR(gbl->gpiod)) {

> @@ -124,15 +120,8 @@ static int gpio_backlight_probe(struct platform_device *pdev)

>   		unsigned long flags = GPIOF_DIR_OUT;

>   

>   		gbl->fbdev = pdata->fbdev;

> -		gbl->active = pdata->active_low ? 0 : 1;

>   		gbl->def_value = pdata->def_value;

> -

> -		if (gbl->active)

> -			flags |= gbl->def_value ?

> -				GPIOF_INIT_HIGH : GPIOF_INIT_LOW;

> -		else

> -			flags |= gbl->def_value ?

> -				GPIOF_INIT_LOW : GPIOF_INIT_HIGH;

> +		flags |= gbl->def_value ? GPIOF_INIT_HIGH : GPIOF_INIT_LOW;

>   

>   		ret = devm_gpio_request_one(gbl->dev, pdata->gpio, flags,

>   					    pdata ? pdata->name : "backlight");

> diff --git a/include/linux/platform_data/gpio_backlight.h b/include/linux/platform_data/gpio_backlight.h

> index 5ae0d9c80d4d..683d90453c41 100644

> --- a/include/linux/platform_data/gpio_backlight.h

> +++ b/include/linux/platform_data/gpio_backlight.h

> @@ -14,7 +14,6 @@ struct gpio_backlight_platform_data {

>   	struct device *fbdev;

>   	int gpio;

>   	int def_value;

> -	bool active_low;

>   	const char *name;

>   };

>   

>
Sebastian Reichel May 23, 2017, 3:14 p.m. UTC | #3 
On Tue, May 23, 2017 at 03:15:01PM +0100, Daniel Thompson wrote:
> On 30/04/17 09:17, Linus Walleij wrote:

> > The option to invert the output of the GPIO (active low) is

> > not used by the only platform still using platform data to

> > set up a GPIO backlight (one SH board). Delete the option

> > as we do not expect to expand the use of board files for

> > this driver, and GPIO descriptors intrinsically keep track

> > of any signal inversion.

> > 

> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> 

> I note that only ~40% of Linux architectures currently have DT support at

> this point... There are still a few "embedded" ones that make do with

> platform bus (although they are growing increasingly niche).

> 

> Having said that I can't argue with "it's not used anywhere" (I've only

> really posted the above paragraph as a hint for future archeology... since

> it might be me doing the digging):

> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>


Just for the record, if somebody needs to use board files, the
gpio subsystem provides gpiod_lookup_table instead, as described
in: Documentation/gpio/board.txt.

-- Sebastian

> > ---

> >   drivers/video/backlight/gpio_backlight.c     | 15 ++-------------

> >   include/linux/platform_data/gpio_backlight.h |  1 -

> >   2 files changed, 2 insertions(+), 14 deletions(-)

> > 

> > diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c

> > index 17dd8071ad4d..8c19dc6c9236 100644

> > --- a/drivers/video/backlight/gpio_backlight.c

> > +++ b/drivers/video/backlight/gpio_backlight.c

> > @@ -25,7 +25,6 @@ struct gpio_backlight {

> >   	struct device *fbdev;

> >   	struct gpio_desc *gpiod;

> > -	int active;

> >   	int def_value;

> >   };

> > @@ -39,8 +38,7 @@ static int gpio_backlight_update_status(struct backlight_device *bl)

> >   	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))

> >   		brightness = 0;

> > -	gpiod_set_value_cansleep(gbl->gpiod,

> > -				 brightness ? gbl->active : !gbl->active);

> > +	gpiod_set_value_cansleep(gbl->gpiod, brightness);

> >   	return 0;

> >   }

> > @@ -72,8 +70,6 @@ static int gpio_backlight_probe_dt(struct platform_device *pdev,

> >   		flags = GPIOD_OUT_HIGH;

> >   	else

> >   		flags = GPIOD_OUT_LOW;

> > -	/* GPIO descriptors keep track of inversion */

> > -	gbl->active = 1;

> >   	gbl->gpiod = devm_gpiod_get(dev, NULL, flags);

> >   	if (IS_ERR(gbl->gpiod)) {

> > @@ -124,15 +120,8 @@ static int gpio_backlight_probe(struct platform_device *pdev)

> >   		unsigned long flags = GPIOF_DIR_OUT;

> >   		gbl->fbdev = pdata->fbdev;

> > -		gbl->active = pdata->active_low ? 0 : 1;

> >   		gbl->def_value = pdata->def_value;

> > -

> > -		if (gbl->active)

> > -			flags |= gbl->def_value ?

> > -				GPIOF_INIT_HIGH : GPIOF_INIT_LOW;

> > -		else

> > -			flags |= gbl->def_value ?

> > -				GPIOF_INIT_LOW : GPIOF_INIT_HIGH;

> > +		flags |= gbl->def_value ? GPIOF_INIT_HIGH : GPIOF_INIT_LOW;

> >   		ret = devm_gpio_request_one(gbl->dev, pdata->gpio, flags,

> >   					    pdata ? pdata->name : "backlight");

> > diff --git a/include/linux/platform_data/gpio_backlight.h b/include/linux/platform_data/gpio_backlight.h

> > index 5ae0d9c80d4d..683d90453c41 100644

> > --- a/include/linux/platform_data/gpio_backlight.h

> > +++ b/include/linux/platform_data/gpio_backlight.h

> > @@ -14,7 +14,6 @@ struct gpio_backlight_platform_data {

> >   	struct device *fbdev;

> >   	int gpio;

> >   	int def_value;

> > -	bool active_low;

> >   	const char *name;

> >   };

> > 

>
Lee Jones May 30, 2017, 8:15 a.m. UTC | #4 
On Sun, 30 Apr 2017, Linus Walleij wrote:

> The option to invert the output of the GPIO (active low) is

> not used by the only platform still using platform data to

> set up a GPIO backlight (one SH board). Delete the option

> as we do not expect to expand the use of board files for

> this driver, and GPIO descriptors intrinsically keep track

> of any signal inversion.

> 

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  drivers/video/backlight/gpio_backlight.c     | 15 ++-------------

>  include/linux/platform_data/gpio_backlight.h |  1 -

>  2 files changed, 2 insertions(+), 14 deletions(-)


Applied, thanks.

> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c

> index 17dd8071ad4d..8c19dc6c9236 100644

> --- a/drivers/video/backlight/gpio_backlight.c

> +++ b/drivers/video/backlight/gpio_backlight.c

> @@ -25,7 +25,6 @@ struct gpio_backlight {

>  	struct device *fbdev;

>  

>  	struct gpio_desc *gpiod;

> -	int active;

>  	int def_value;

>  };

>  

> @@ -39,8 +38,7 @@ static int gpio_backlight_update_status(struct backlight_device *bl)

>  	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))

>  		brightness = 0;

>  

> -	gpiod_set_value_cansleep(gbl->gpiod,

> -				 brightness ? gbl->active : !gbl->active);

> +	gpiod_set_value_cansleep(gbl->gpiod, brightness);

>  

>  	return 0;

>  }

> @@ -72,8 +70,6 @@ static int gpio_backlight_probe_dt(struct platform_device *pdev,

>  		flags = GPIOD_OUT_HIGH;

>  	else

>  		flags = GPIOD_OUT_LOW;

> -	/* GPIO descriptors keep track of inversion */

> -	gbl->active = 1;

>  

>  	gbl->gpiod = devm_gpiod_get(dev, NULL, flags);

>  	if (IS_ERR(gbl->gpiod)) {

> @@ -124,15 +120,8 @@ static int gpio_backlight_probe(struct platform_device *pdev)

>  		unsigned long flags = GPIOF_DIR_OUT;

>  

>  		gbl->fbdev = pdata->fbdev;

> -		gbl->active = pdata->active_low ? 0 : 1;

>  		gbl->def_value = pdata->def_value;

> -

> -		if (gbl->active)

> -			flags |= gbl->def_value ?

> -				GPIOF_INIT_HIGH : GPIOF_INIT_LOW;

> -		else

> -			flags |= gbl->def_value ?

> -				GPIOF_INIT_LOW : GPIOF_INIT_HIGH;

> +		flags |= gbl->def_value ? GPIOF_INIT_HIGH : GPIOF_INIT_LOW;

>  

>  		ret = devm_gpio_request_one(gbl->dev, pdata->gpio, flags,

>  					    pdata ? pdata->name : "backlight");

> diff --git a/include/linux/platform_data/gpio_backlight.h b/include/linux/platform_data/gpio_backlight.h

> index 5ae0d9c80d4d..683d90453c41 100644

> --- a/include/linux/platform_data/gpio_backlight.h

> +++ b/include/linux/platform_data/gpio_backlight.h

> @@ -14,7 +14,6 @@ struct gpio_backlight_platform_data {

>  	struct device *fbdev;

>  	int gpio;

>  	int def_value;

> -	bool active_low;

>  	const char *name;

>  };

>  


-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones May 30, 2017, 8:29 a.m. UTC | #5 
On Tue, 30 May 2017, Lee Jones wrote:

> On Sun, 30 Apr 2017, Linus Walleij wrote:

> 

> > The option to invert the output of the GPIO (active low) is

> > not used by the only platform still using platform data to

> > set up a GPIO backlight (one SH board). Delete the option

> > as we do not expect to expand the use of board files for

> > this driver, and GPIO descriptors intrinsically keep track

> > of any signal inversion.

> > 

> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> > ---

> >  drivers/video/backlight/gpio_backlight.c     | 15 ++-------------

> >  include/linux/platform_data/gpio_backlight.h |  1 -

> >  2 files changed, 2 insertions(+), 14 deletions(-)

> 

> Applied, thanks.


Change of plan.

This patch depends on:

 "backlight: gpio: Convert to use GPIO descriptor"

... which is expecting an update.

> > diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c

> > index 17dd8071ad4d..8c19dc6c9236 100644

> > --- a/drivers/video/backlight/gpio_backlight.c

> > +++ b/drivers/video/backlight/gpio_backlight.c

> > @@ -25,7 +25,6 @@ struct gpio_backlight {

> >  	struct device *fbdev;

> >  

> >  	struct gpio_desc *gpiod;

> > -	int active;

> >  	int def_value;

> >  };

> >  

> > @@ -39,8 +38,7 @@ static int gpio_backlight_update_status(struct backlight_device *bl)

> >  	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))

> >  		brightness = 0;

> >  

> > -	gpiod_set_value_cansleep(gbl->gpiod,

> > -				 brightness ? gbl->active : !gbl->active);

> > +	gpiod_set_value_cansleep(gbl->gpiod, brightness);

> >  

> >  	return 0;

> >  }

> > @@ -72,8 +70,6 @@ static int gpio_backlight_probe_dt(struct platform_device *pdev,

> >  		flags = GPIOD_OUT_HIGH;

> >  	else

> >  		flags = GPIOD_OUT_LOW;

> > -	/* GPIO descriptors keep track of inversion */

> > -	gbl->active = 1;

> >  

> >  	gbl->gpiod = devm_gpiod_get(dev, NULL, flags);

> >  	if (IS_ERR(gbl->gpiod)) {

> > @@ -124,15 +120,8 @@ static int gpio_backlight_probe(struct platform_device *pdev)

> >  		unsigned long flags = GPIOF_DIR_OUT;

> >  

> >  		gbl->fbdev = pdata->fbdev;

> > -		gbl->active = pdata->active_low ? 0 : 1;

> >  		gbl->def_value = pdata->def_value;

> > -

> > -		if (gbl->active)

> > -			flags |= gbl->def_value ?

> > -				GPIOF_INIT_HIGH : GPIOF_INIT_LOW;

> > -		else

> > -			flags |= gbl->def_value ?

> > -				GPIOF_INIT_LOW : GPIOF_INIT_HIGH;

> > +		flags |= gbl->def_value ? GPIOF_INIT_HIGH : GPIOF_INIT_LOW;

> >  

> >  		ret = devm_gpio_request_one(gbl->dev, pdata->gpio, flags,

> >  					    pdata ? pdata->name : "backlight");

> > diff --git a/include/linux/platform_data/gpio_backlight.h b/include/linux/platform_data/gpio_backlight.h

> > index 5ae0d9c80d4d..683d90453c41 100644

> > --- a/include/linux/platform_data/gpio_backlight.h

> > +++ b/include/linux/platform_data/gpio_backlight.h

> > @@ -14,7 +14,6 @@ struct gpio_backlight_platform_data {

> >  	struct device *fbdev;

> >  	int gpio;

> >  	int def_value;

> > -	bool active_low;

> >  	const char *name;

> >  };

> >  

> 


-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linus Walleij May 30, 2017, 11:45 a.m. UTC | #6 
On Tue, May 30, 2017 at 10:29 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 30 May 2017, Lee Jones wrote:

>

>> On Sun, 30 Apr 2017, Linus Walleij wrote:

>>

>> > The option to invert the output of the GPIO (active low) is

>> > not used by the only platform still using platform data to

>> > set up a GPIO backlight (one SH board). Delete the option

>> > as we do not expect to expand the use of board files for

>> > this driver, and GPIO descriptors intrinsically keep track

>> > of any signal inversion.

>> >

>> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>> > ---

>> >  drivers/video/backlight/gpio_backlight.c     | 15 ++-------------

>> >  include/linux/platform_data/gpio_backlight.h |  1 -

>> >  2 files changed, 2 insertions(+), 14 deletions(-)

>>

>> Applied, thanks.

>

> Change of plan.

>

> This patch depends on:

>

>  "backlight: gpio: Convert to use GPIO descriptor"


Oh, sorry for sluggishness.

I've updated it, resending a v2 now.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index 17dd8071ad4d..8c19dc6c9236 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -25,7 +25,6 @@  struct gpio_backlight {
 	struct device *fbdev;
 
 	struct gpio_desc *gpiod;
-	int active;
 	int def_value;
 };
 
@@ -39,8 +38,7 @@  static int gpio_backlight_update_status(struct backlight_device *bl)
 	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
 		brightness = 0;
 
-	gpiod_set_value_cansleep(gbl->gpiod,
-				 brightness ? gbl->active : !gbl->active);
+	gpiod_set_value_cansleep(gbl->gpiod, brightness);
 
 	return 0;
 }
@@ -72,8 +70,6 @@  static int gpio_backlight_probe_dt(struct platform_device *pdev,
 		flags = GPIOD_OUT_HIGH;
 	else
 		flags = GPIOD_OUT_LOW;
-	/* GPIO descriptors keep track of inversion */
-	gbl->active = 1;
 
 	gbl->gpiod = devm_gpiod_get(dev, NULL, flags);
 	if (IS_ERR(gbl->gpiod)) {
@@ -124,15 +120,8 @@  static int gpio_backlight_probe(struct platform_device *pdev)
 		unsigned long flags = GPIOF_DIR_OUT;
 
 		gbl->fbdev = pdata->fbdev;
-		gbl->active = pdata->active_low ? 0 : 1;
 		gbl->def_value = pdata->def_value;
-
-		if (gbl->active)
-			flags |= gbl->def_value ?
-				GPIOF_INIT_HIGH : GPIOF_INIT_LOW;
-		else
-			flags |= gbl->def_value ?
-				GPIOF_INIT_LOW : GPIOF_INIT_HIGH;
+		flags |= gbl->def_value ? GPIOF_INIT_HIGH : GPIOF_INIT_LOW;
 
 		ret = devm_gpio_request_one(gbl->dev, pdata->gpio, flags,
 					    pdata ? pdata->name : "backlight");
diff --git a/include/linux/platform_data/gpio_backlight.h b/include/linux/platform_data/gpio_backlight.h
index 5ae0d9c80d4d..683d90453c41 100644
--- a/include/linux/platform_data/gpio_backlight.h
+++ b/include/linux/platform_data/gpio_backlight.h
@@ -14,7 +14,6 @@  struct gpio_backlight_platform_data {
 	struct device *fbdev;
 	int gpio;
 	int def_value;
-	bool active_low;
 	const char *name;
 };