From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1764180AbdEWNrt (ORCPT ); Tue, 23 May 2017 09:47:49 -0400 Received: from mail-wm0-f54.google.com ([74.125.82.54]:38056 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1764166AbdEWNrq (ORCPT ); Tue, 23 May 2017 09:47:46 -0400 Subject: Re: [PATCH 1/2] backlight: gpio: Convert to use GPIO descriptor To: Linus Walleij , Lee Jones , Jingoo Han , linux-kernel@vger.kernel.org Cc: Laurent Pinchart References: <20170430081731.14315-1-linus.walleij@linaro.org> From: Daniel Thompson Message-ID: Date: Tue, 23 May 2017 14:47:40 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <20170430081731.14315-1-linus.walleij@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Only a tiny, tiny nit from me and, with that change: Acked-by: Daniel Thompson > --- > 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 > #include > #include > -#include > +#include /* Only for legacy support */ > +#include > #include > #include > #include > @@ -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)); >