| Message ID | 20170430081731.14315-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] backlight: gpio: Convert to use GPIO descriptor | expand |
On Sun, Apr 30, 2017 at 10:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > This driver is predominantly used by device tree systems, all > of which can deal with modern GPIO descriptors. The legacy > GPIO API is only used by one SH board so make the GPIO > descriptor the default way to deal with it. > > As an intended side effect we do not need to look around in > the device tree for the inversion flag since the GPIO > descriptors will intrinsically deal with this. > > 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
On 22/05/17 09:45, Linus Walleij wrote: > On Sun, Apr 30, 2017 at 10:17 AM, Linus Walleij > <linus.walleij@linaro.org> wrote: > >> This driver is predominantly used by device tree systems, all >> of which can deal with modern GPIO descriptors. The legacy >> GPIO API is only used by one SH board so make the GPIO >> descriptor the default way to deal with it. >> >> As an intended side effect we do not need to look around in >> the device tree for the inversion flag since the GPIO >> descriptors will intrinsically deal with this. >> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > No reaction on this patch since 21 days so sending an > unsolicited ping. Maintainer is alive! Sorry for sluggish service. It is TODOed but has been eclipsed for many days. Will get to it soon. Daniel.
On 30/04/17 09:17, Linus Walleij wrote: > This driver is predominantly used by device tree systems, all > of which can deal with modern GPIO descriptors. The legacy > GPIO API is only used by one SH board so make the GPIO > descriptor the default way to deal with it. Every time I've reviewed previously I kind of got tripped up here and decided to defer the review "until I had more time". I've since realized I should probably just trust you on this bit and read the backlight code instead ;-). > As an intended side effect we do not need to look around in > the device tree for the inversion flag since the GPIO > descriptors will intrinsically deal with this. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Only a tiny, tiny nit from me and, with that change: Acked-by: Daniel Thompson <daniel.thompson@linaro.org> > --- > drivers/video/backlight/gpio_backlight.c | 74 ++++++++++++++++++++------------ > 1 file changed, 46 insertions(+), 28 deletions(-) > > diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c > index 18134416b154..17dd8071ad4d 100644 > --- a/drivers/video/backlight/gpio_backlight.c > +++ b/drivers/video/backlight/gpio_backlight.c > @@ -9,7 +9,8 @@ > #include <linux/backlight.h> > #include <linux/err.h> > #include <linux/fb.h> > -#include <linux/gpio.h> > +#include <linux/gpio.h> /* Only for legacy support */ > +#include <linux/gpio/consumer.h> > #include <linux/init.h> > #include <linux/kernel.h> > #include <linux/module.h> > @@ -23,7 +24,7 @@ struct gpio_backlight { > struct device *dev; > struct device *fbdev; > > - int gpio; > + struct gpio_desc *gpiod; > int active; > int def_value; > }; > @@ -38,8 +39,8 @@ static int gpio_backlight_update_status(struct backlight_device *bl) > bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK)) > brightness = 0; > > - gpio_set_value_cansleep(gbl->gpio, > - brightness ? gbl->active : !gbl->active); > + gpiod_set_value_cansleep(gbl->gpiod, > + brightness ? gbl->active : !gbl->active); > > return 0; > } > @@ -61,23 +62,30 @@ static const struct backlight_ops gpio_backlight_ops = { > static int gpio_backlight_probe_dt(struct platform_device *pdev, > struct gpio_backlight *gbl) > { > - struct device_node *np = pdev->dev.of_node; > - enum of_gpio_flags gpio_flags; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + enum gpiod_flags flags; > + int ret; > + > + gbl->def_value = of_property_read_bool(np, "default-on"); > + if (gbl->def_value) > + flags = GPIOD_OUT_HIGH; > + else > + flags = GPIOD_OUT_LOW; Would prefer ternary here (like other flags assignments)... > + /* GPIO descriptors keep track of inversion */ > + gbl->active = 1; > > - gbl->gpio = of_get_gpio_flags(np, 0, &gpio_flags); > + gbl->gpiod = devm_gpiod_get(dev, NULL, flags); > + if (IS_ERR(gbl->gpiod)) { > + ret = PTR_ERR(gbl->gpiod); > > - if (!gpio_is_valid(gbl->gpio)) { > - if (gbl->gpio != -EPROBE_DEFER) { > - dev_err(&pdev->dev, > + if (ret != -EPROBE_DEFER) { > + dev_err(dev, > "Error: The gpios parameter is missing or invalid.\n"); > } > - return gbl->gpio; > + return ret; > } > > - gbl->active = (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1; > - > - gbl->def_value = of_property_read_bool(np, "default-on"); > - > return 0; > } > > @@ -89,7 +97,6 @@ static int gpio_backlight_probe(struct platform_device *pdev) > struct backlight_device *bl; > struct gpio_backlight *gbl; > struct device_node *np = pdev->dev.of_node; > - unsigned long flags = GPIOF_DIR_OUT; > int ret; > > if (!pdata && !np) { > @@ -109,22 +116,33 @@ static int gpio_backlight_probe(struct platform_device *pdev) > if (ret) > return ret; > } else { > + /* > + * Legacy platform data GPIO retrieveal. Do not expand > + * the use of this code path, currently only used by one > + * SH board. > + */ > + unsigned long flags = GPIOF_DIR_OUT; > + > gbl->fbdev = pdata->fbdev; > - gbl->gpio = pdata->gpio; > 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; > > - ret = devm_gpio_request_one(gbl->dev, gbl->gpio, flags, > - pdata ? pdata->name : "backlight"); > - if (ret < 0) { > - dev_err(&pdev->dev, "unable to request GPIO\n"); > - return ret; > + if (gbl->active) > + flags |= gbl->def_value ? > + GPIOF_INIT_HIGH : GPIOF_INIT_LOW; > + else > + flags |= gbl->def_value ? > + GPIOF_INIT_LOW : GPIOF_INIT_HIGH; > + > + ret = devm_gpio_request_one(gbl->dev, pdata->gpio, flags, > + pdata ? pdata->name : "backlight"); > + if (ret < 0) { > + dev_err(&pdev->dev, "unable to request GPIO\n"); > + return ret; > + } > + gbl->gpiod = gpio_to_desc(pdata->gpio); > + if (!gbl->gpiod) > + return -EINVAL; > } > > memset(&props, 0, sizeof(props)); >
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c index 18134416b154..17dd8071ad4d 100644 --- a/drivers/video/backlight/gpio_backlight.c +++ b/drivers/video/backlight/gpio_backlight.c @@ -9,7 +9,8 @@ #include <linux/backlight.h> #include <linux/err.h> #include <linux/fb.h> -#include <linux/gpio.h> +#include <linux/gpio.h> /* Only for legacy support */ +#include <linux/gpio/consumer.h> #include <linux/init.h> #include <linux/kernel.h> #include <linux/module.h> @@ -23,7 +24,7 @@ struct gpio_backlight { struct device *dev; struct device *fbdev; - int gpio; + struct gpio_desc *gpiod; int active; int def_value; }; @@ -38,8 +39,8 @@ static int gpio_backlight_update_status(struct backlight_device *bl) bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK)) brightness = 0; - gpio_set_value_cansleep(gbl->gpio, - brightness ? gbl->active : !gbl->active); + gpiod_set_value_cansleep(gbl->gpiod, + brightness ? gbl->active : !gbl->active); return 0; } @@ -61,23 +62,30 @@ static const struct backlight_ops gpio_backlight_ops = { static int gpio_backlight_probe_dt(struct platform_device *pdev, struct gpio_backlight *gbl) { - struct device_node *np = pdev->dev.of_node; - enum of_gpio_flags gpio_flags; + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + enum gpiod_flags flags; + int ret; + + gbl->def_value = of_property_read_bool(np, "default-on"); + if (gbl->def_value) + flags = GPIOD_OUT_HIGH; + else + flags = GPIOD_OUT_LOW; + /* GPIO descriptors keep track of inversion */ + gbl->active = 1; - gbl->gpio = of_get_gpio_flags(np, 0, &gpio_flags); + gbl->gpiod = devm_gpiod_get(dev, NULL, flags); + if (IS_ERR(gbl->gpiod)) { + ret = PTR_ERR(gbl->gpiod); - if (!gpio_is_valid(gbl->gpio)) { - if (gbl->gpio != -EPROBE_DEFER) { - dev_err(&pdev->dev, + if (ret != -EPROBE_DEFER) { + dev_err(dev, "Error: The gpios parameter is missing or invalid.\n"); } - return gbl->gpio; + return ret; } - gbl->active = (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1; - - gbl->def_value = of_property_read_bool(np, "default-on"); - return 0; } @@ -89,7 +97,6 @@ static int gpio_backlight_probe(struct platform_device *pdev) struct backlight_device *bl; struct gpio_backlight *gbl; struct device_node *np = pdev->dev.of_node; - unsigned long flags = GPIOF_DIR_OUT; int ret; if (!pdata && !np) { @@ -109,22 +116,33 @@ static int gpio_backlight_probe(struct platform_device *pdev) if (ret) return ret; } else { + /* + * Legacy platform data GPIO retrieveal. Do not expand + * the use of this code path, currently only used by one + * SH board. + */ + unsigned long flags = GPIOF_DIR_OUT; + gbl->fbdev = pdata->fbdev; - gbl->gpio = pdata->gpio; 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; - ret = devm_gpio_request_one(gbl->dev, gbl->gpio, flags, - pdata ? pdata->name : "backlight"); - if (ret < 0) { - dev_err(&pdev->dev, "unable to request GPIO\n"); - return ret; + if (gbl->active) + flags |= gbl->def_value ? + GPIOF_INIT_HIGH : GPIOF_INIT_LOW; + else + flags |= gbl->def_value ? + GPIOF_INIT_LOW : GPIOF_INIT_HIGH; + + ret = devm_gpio_request_one(gbl->dev, pdata->gpio, flags, + pdata ? pdata->name : "backlight"); + if (ret < 0) { + dev_err(&pdev->dev, "unable to request GPIO\n"); + return ret; + } + gbl->gpiod = gpio_to_desc(pdata->gpio); + if (!gbl->gpiod) + return -EINVAL; } memset(&props, 0, sizeof(props));
This driver is predominantly used by device tree systems, all of which can deal with modern GPIO descriptors. The legacy GPIO API is only used by one SH board so make the GPIO descriptor the default way to deal with it. As an intended side effect we do not need to look around in the device tree for the inversion flag since the GPIO descriptors will intrinsically deal with this. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/video/backlight/gpio_backlight.c | 74 ++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 28 deletions(-) -- 2.9.3