[PATCH] backlight: bd6107: Convert to use GPIO descriptor

[PATCH] backlight: bd6107: Convert to use GPIO descriptor

[Linus Walleij]

The Rohm BD6107 driver can pass a fixed GPIO line using the old
GPIO API using platform data. As there are no in-tree users of this
platform data since 2013, we can convert this to use a GPIO descriptor
and require any out-of-tree consumers to pass the GPIO using
a machine descriptor table instead.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/video/backlight/bd6107.c     | 24 ++++++++++++++++--------
 include/linux/platform_data/bd6107.h |  1 -
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/video/backlight/bd6107.c b/drivers/video/backlight/bd6107.c
index d344fb03cb86..d5d5fb457e78 100644
--- a/drivers/video/backlight/bd6107.c
+++ b/drivers/video/backlight/bd6107.c
@@ -11,7 +11,7 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/fb.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/platform_data/bd6107.h>
@@ -71,6 +71,7 @@ struct bd6107 {
 	struct i2c_client *client;
 	struct backlight_device *backlight;
 	struct bd6107_platform_data *pdata;
+	struct gpio_desc *reset;
 };
 
 static int bd6107_write(struct bd6107 *bd, u8 reg, u8 data)
@@ -94,9 +95,10 @@ static int bd6107_backlight_update_status(struct backlight_device *backlight)
 		bd6107_write(bd, BD6107_MAINCNT1, brightness);
 		bd6107_write(bd, BD6107_LEDCNT1, BD6107_LEDCNT1_LEDONOFF1);
 	} else {
-		gpio_set_value(bd->pdata->reset, 0);
+		/* Assert the reset line (gpiolib will handle active low) */
+		gpiod_set_value(bd->reset, 1);
 		msleep(24);
-		gpio_set_value(bd->pdata->reset, 1);
+		gpiod_set_value(bd->reset, 0);
 	}
 
 	return 0;
@@ -125,8 +127,8 @@ static int bd6107_probe(struct i2c_client *client,
 	struct bd6107 *bd;
 	int ret;
 
-	if (pdata == NULL || !pdata->reset) {
-		dev_err(&client->dev, "No reset GPIO in platform data\n");
+	if (pdata == NULL) {
+		dev_err(&client->dev, "No platform data\n");
 		return -EINVAL;
 	}
 
@@ -144,10 +146,16 @@ static int bd6107_probe(struct i2c_client *client,
 	bd->client = client;
 	bd->pdata = pdata;
 
-	ret = devm_gpio_request_one(&client->dev, pdata->reset,
-				    GPIOF_DIR_OUT | GPIOF_INIT_LOW, "reset");
-	if (ret < 0) {
+	/*
+	 * Request the reset GPIO line with GPIOD_OUT_HIGH meaning asserted,
+	 * so in the machine descriptor table (or other hardware description),
+	 * the line should be flagged as active low so this will assert
+	 * the reset.
+	 */
+	bd->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(bd->reset)) {
 		dev_err(&client->dev, "unable to request reset GPIO\n");
+		ret = PTR_ERR(bd->reset);
 		return ret;
 	}
 
diff --git a/include/linux/platform_data/bd6107.h b/include/linux/platform_data/bd6107.h
index 3bd019037eb3..54a06a4d2618 100644
--- a/include/linux/platform_data/bd6107.h
+++ b/include/linux/platform_data/bd6107.h
@@ -9,7 +9,6 @@ struct device;
 
 struct bd6107_platform_data {
 	struct device *fbdev;
-	int reset;			/* Reset GPIO */
 	unsigned int def_value;
 };
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] backlight: bd6107: Convert to use GPIO descriptor

[Laurent Pinchart]

Hi Linus,

Thank you for the patch.

On Mon, Dec 02, 2019 at 11:30:28AM +0100, Linus Walleij wrote:
> The Rohm BD6107 driver can pass a fixed GPIO line using the old
> GPIO API using platform data. As there are no in-tree users of this
> platform data since 2013, we can convert this to use a GPIO descriptor
> and require any out-of-tree consumers to pass the GPIO using
> a machine descriptor table instead.

Or better, converting to DT :-)

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

How about going one step further and removing platform data support in a
second patch ?

> ---
>  drivers/video/backlight/bd6107.c     | 24 ++++++++++++++++--------
>  include/linux/platform_data/bd6107.h |  1 -
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/video/backlight/bd6107.c b/drivers/video/backlight/bd6107.c
> index d344fb03cb86..d5d5fb457e78 100644
> --- a/drivers/video/backlight/bd6107.c
> +++ b/drivers/video/backlight/bd6107.c
> @@ -11,7 +11,7 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/fb.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/platform_data/bd6107.h>
> @@ -71,6 +71,7 @@ struct bd6107 {
>  	struct i2c_client *client;
>  	struct backlight_device *backlight;
>  	struct bd6107_platform_data *pdata;
> +	struct gpio_desc *reset;
>  };
>  
>  static int bd6107_write(struct bd6107 *bd, u8 reg, u8 data)
> @@ -94,9 +95,10 @@ static int bd6107_backlight_update_status(struct backlight_device *backlight)
>  		bd6107_write(bd, BD6107_MAINCNT1, brightness);
>  		bd6107_write(bd, BD6107_LEDCNT1, BD6107_LEDCNT1_LEDONOFF1);
>  	} else {
> -		gpio_set_value(bd->pdata->reset, 0);
> +		/* Assert the reset line (gpiolib will handle active low) */
> +		gpiod_set_value(bd->reset, 1);
>  		msleep(24);
> -		gpio_set_value(bd->pdata->reset, 1);
> +		gpiod_set_value(bd->reset, 0);
>  	}
>  
>  	return 0;
> @@ -125,8 +127,8 @@ static int bd6107_probe(struct i2c_client *client,
>  	struct bd6107 *bd;
>  	int ret;
>  
> -	if (pdata == NULL || !pdata->reset) {
> -		dev_err(&client->dev, "No reset GPIO in platform data\n");
> +	if (pdata == NULL) {
> +		dev_err(&client->dev, "No platform data\n");
>  		return -EINVAL;
>  	}
>  
> @@ -144,10 +146,16 @@ static int bd6107_probe(struct i2c_client *client,
>  	bd->client = client;
>  	bd->pdata = pdata;
>  
> -	ret = devm_gpio_request_one(&client->dev, pdata->reset,
> -				    GPIOF_DIR_OUT | GPIOF_INIT_LOW, "reset");
> -	if (ret < 0) {
> +	/*
> +	 * Request the reset GPIO line with GPIOD_OUT_HIGH meaning asserted,
> +	 * so in the machine descriptor table (or other hardware description),
> +	 * the line should be flagged as active low so this will assert
> +	 * the reset.
> +	 */
> +	bd->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(bd->reset)) {
>  		dev_err(&client->dev, "unable to request reset GPIO\n");
> +		ret = PTR_ERR(bd->reset);
>  		return ret;
>  	}
>  
> diff --git a/include/linux/platform_data/bd6107.h b/include/linux/platform_data/bd6107.h
> index 3bd019037eb3..54a06a4d2618 100644
> --- a/include/linux/platform_data/bd6107.h
> +++ b/include/linux/platform_data/bd6107.h
> @@ -9,7 +9,6 @@ struct device;
>  
>  struct bd6107_platform_data {
>  	struct device *fbdev;
> -	int reset;			/* Reset GPIO */
>  	unsigned int def_value;
>  };
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] backlight: bd6107: Convert to use GPIO descriptor

[Linus Walleij]

On Mon, Dec 2, 2019 at 11:42 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Dec 02, 2019 at 11:30:28AM +0100, Linus Walleij wrote:
> > The Rohm BD6107 driver can pass a fixed GPIO line using the old
> > GPIO API using platform data. As there are no in-tree users of this
> > platform data since 2013, we can convert this to use a GPIO descriptor
> > and require any out-of-tree consumers to pass the GPIO using
> > a machine descriptor table instead.
>
> Or better, converting to DT :-)

Yeah right now my objective is just to get the old GPIO API
out of the kernel, so I'm focusing on that.

> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> How about going one step further and removing platform data support in a
> second patch ?

That seems to mandate also adding a DT binding and probe path
for DT otherwise there is no way to probe the driver at all :P, and
I would really need to have the hardware to test with
for that.

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] backlight: bd6107: Convert to use GPIO descriptor

[Daniel Thompson]

On Mon, Dec 02, 2019 at 11:30:28AM +0100, Linus Walleij wrote:
> The Rohm BD6107 driver can pass a fixed GPIO line using the old
> GPIO API using platform data. As there are no in-tree users of this
> platform data since 2013, we can convert this to use a GPIO descriptor
> and require any out-of-tree consumers to pass the GPIO using
> a machine descriptor table instead.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

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

> ---
>  drivers/video/backlight/bd6107.c     | 24 ++++++++++++++++--------
>  include/linux/platform_data/bd6107.h |  1 -
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/video/backlight/bd6107.c b/drivers/video/backlight/bd6107.c
> index d344fb03cb86..d5d5fb457e78 100644
> --- a/drivers/video/backlight/bd6107.c
> +++ b/drivers/video/backlight/bd6107.c
> @@ -11,7 +11,7 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/fb.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/platform_data/bd6107.h>
> @@ -71,6 +71,7 @@ struct bd6107 {
>  	struct i2c_client *client;
>  	struct backlight_device *backlight;
>  	struct bd6107_platform_data *pdata;
> +	struct gpio_desc *reset;
>  };
>  
>  static int bd6107_write(struct bd6107 *bd, u8 reg, u8 data)
> @@ -94,9 +95,10 @@ static int bd6107_backlight_update_status(struct backlight_device *backlight)
>  		bd6107_write(bd, BD6107_MAINCNT1, brightness);
>  		bd6107_write(bd, BD6107_LEDCNT1, BD6107_LEDCNT1_LEDONOFF1);
>  	} else {
> -		gpio_set_value(bd->pdata->reset, 0);
> +		/* Assert the reset line (gpiolib will handle active low) */
> +		gpiod_set_value(bd->reset, 1);
>  		msleep(24);
> -		gpio_set_value(bd->pdata->reset, 1);
> +		gpiod_set_value(bd->reset, 0);
>  	}
>  
>  	return 0;
> @@ -125,8 +127,8 @@ static int bd6107_probe(struct i2c_client *client,
>  	struct bd6107 *bd;
>  	int ret;
>  
> -	if (pdata == NULL || !pdata->reset) {
> -		dev_err(&client->dev, "No reset GPIO in platform data\n");
> +	if (pdata == NULL) {
> +		dev_err(&client->dev, "No platform data\n");
>  		return -EINVAL;
>  	}
>  
> @@ -144,10 +146,16 @@ static int bd6107_probe(struct i2c_client *client,
>  	bd->client = client;
>  	bd->pdata = pdata;
>  
> -	ret = devm_gpio_request_one(&client->dev, pdata->reset,
> -				    GPIOF_DIR_OUT | GPIOF_INIT_LOW, "reset");
> -	if (ret < 0) {
> +	/*
> +	 * Request the reset GPIO line with GPIOD_OUT_HIGH meaning asserted,
> +	 * so in the machine descriptor table (or other hardware description),
> +	 * the line should be flagged as active low so this will assert
> +	 * the reset.
> +	 */
> +	bd->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(bd->reset)) {
>  		dev_err(&client->dev, "unable to request reset GPIO\n");
> +		ret = PTR_ERR(bd->reset);
>  		return ret;
>  	}
>  
> diff --git a/include/linux/platform_data/bd6107.h b/include/linux/platform_data/bd6107.h
> index 3bd019037eb3..54a06a4d2618 100644
> --- a/include/linux/platform_data/bd6107.h
> +++ b/include/linux/platform_data/bd6107.h
> @@ -9,7 +9,6 @@ struct device;
>  
>  struct bd6107_platform_data {
>  	struct device *fbdev;
> -	int reset;			/* Reset GPIO */
>  	unsigned int def_value;
>  };
>  
> -- 
> 2.23.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] backlight: bd6107: Convert to use GPIO descriptor

[Lee Jones]

On Mon, 02 Dec 2019, Linus Walleij wrote:

> The Rohm BD6107 driver can pass a fixed GPIO line using the old
> GPIO API using platform data. As there are no in-tree users of this
> platform data since 2013, we can convert this to use a GPIO descriptor
> and require any out-of-tree consumers to pass the GPIO using
> a machine descriptor table instead.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/video/backlight/bd6107.c     | 24 ++++++++++++++++--------
>  include/linux/platform_data/bd6107.h |  1 -
>  2 files changed, 16 insertions(+), 9 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel