| Message ID | 20191202103028.102770-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | c8fdcc86725cc93a021139e8de68914857ea7489 |
| Headers | show |
| Series | backlight: bd6107: Convert to use GPIO descriptor | expand |
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; > }; >
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
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 >
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.
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; };
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(-)