diff mbox series

leds: gpio: Try to lookup gpiod from device

Message ID 20180906220940.12053-1-linus.walleij@linaro.org
State New
Headers show
Series leds: gpio: Try to lookup gpiod from device | expand

Commit Message

Linus Walleij Sept. 6, 2018, 10:09 p.m. UTC 
This augments the GPIO lookup code in the GPIO LEDs to
attempt to look up a GPIO descriptor from the device with
index.

This makes it possible to use GPIO machine look-up tables
and stop passing global GPIO numbers through platform data.

Using this we can stepwise convert existing board files
to use machine descriptor tables and then eventually drop
the legacy GPIO support and only include
<linux/gpio/consumer.h> and use descriptors exclusively.

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

---
 drivers/leds/leds-gpio.c | 93 +++++++++++++++++++++++++++-------------
 1 file changed, 63 insertions(+), 30 deletions(-)

-- 
2.17.1

Comments

Jacek Anaszewski Sept. 8, 2018, 8:38 p.m. UTC | #1 
Hi Linus,

Thank you for the patch.

One trivial nit below.

On 09/07/2018 12:09 AM, Linus Walleij wrote:
> This augments the GPIO lookup code in the GPIO LEDs to

> attempt to look up a GPIO descriptor from the device with

> index.

> 

> This makes it possible to use GPIO machine look-up tables

> and stop passing global GPIO numbers through platform data.

> 

> Using this we can stepwise convert existing board files

> to use machine descriptor tables and then eventually drop

> the legacy GPIO support and only include

> <linux/gpio/consumer.h> and use descriptors exclusively.

> 

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

> ---

>  drivers/leds/leds-gpio.c | 93 +++++++++++++++++++++++++++-------------

>  1 file changed, 63 insertions(+), 30 deletions(-)

> 

> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c

> index 764c31301f90..fbc623148595 100644

> --- a/drivers/leds/leds-gpio.c

> +++ b/drivers/leds/leds-gpio.c

> @@ -81,35 +81,6 @@ static int create_gpio_led(const struct gpio_led *template,

>  {

>  	int ret, state;

>  

> -	led_dat->gpiod = template->gpiod;

> -	if (!led_dat->gpiod) {

> -		/*

> -		 * This is the legacy code path for platform code that

> -		 * still uses GPIO numbers. Ultimately we would like to get

> -		 * rid of this block completely.

> -		 */

> -		unsigned long flags = GPIOF_OUT_INIT_LOW;

> -

> -		/* skip leds that aren't available */

> -		if (!gpio_is_valid(template->gpio)) {

> -			dev_info(parent, "Skipping unavailable LED gpio %d (%s)\n",

> -					template->gpio, template->name);

> -			return 0;

> -		}

> -

> -		if (template->active_low)

> -			flags |= GPIOF_ACTIVE_LOW;

> -

> -		ret = devm_gpio_request_one(parent, template->gpio, flags,

> -					    template->name);

> -		if (ret < 0)

> -			return ret;

> -

> -		led_dat->gpiod = gpio_to_desc(template->gpio);

> -		if (!led_dat->gpiod)

> -			return -EINVAL;

> -	}

> -

>  	led_dat->cdev.name = template->name;

>  	led_dat->cdev.default_trigger = template->default_trigger;

>  	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);

> @@ -231,6 +202,53 @@ static const struct of_device_id of_gpio_leds_match[] = {

>  

>  MODULE_DEVICE_TABLE(of, of_gpio_leds_match);

>  

> +static struct gpio_desc *gpio_led_get_gpiod(struct device *dev, int idx,

> +					    const struct gpio_led *template)

> +{

> +	struct gpio_desc *gpiod;

> +	unsigned long flags = GPIOF_OUT_INIT_LOW;

> +	int ret;

> +

> +	/*

> +	 * This means the LED does not come from the device tree

> +	 * or ACPI, so let's try just getting it by index from the

> +	 * device, this will hit the board file, if any and get

> +	 * the GPIO from there.

> +	 */

> +	gpiod = devm_gpiod_get_index(dev, NULL, idx, flags);

> +	if (!IS_ERR(gpiod)) {

> +		gpiod_set_consumer_name(gpiod, template->name);

> +		return gpiod;

> +	}

> +	if (PTR_ERR(gpiod) != -ENOENT)

> +		return gpiod;

> +

> +	/*

> +	 * This is the legacy code path for platform code that

> +	 * still uses GPIO numbers. Ultimately we would like to get

> +	 * rid of this block completely.

> +	 */

> +

> +	/* skip leds that aren't available */

> +	if (!gpio_is_valid(template->gpio)) {

> +		return ERR_PTR(-ENOENT);

> +	}


Curly braces are not needed for single statement block.

I can fix it up by myself if you agree.

> +	if (template->active_low)

> +		flags |= GPIOF_ACTIVE_LOW;

> +

> +	ret = devm_gpio_request_one(dev, template->gpio, flags,

> +				    template->name);

> +	if (ret < 0)

> +		return ERR_PTR(ret);

> +

> +	gpiod = gpio_to_desc(template->gpio);

> +	if (!gpiod)

> +		return ERR_PTR(-EINVAL);

> +

> +	return gpiod;

> +}

> +

>  static int gpio_led_probe(struct platform_device *pdev)

>  {

>  	struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);

> @@ -246,7 +264,22 @@ static int gpio_led_probe(struct platform_device *pdev)

>  

>  		priv->num_leds = pdata->num_leds;

>  		for (i = 0; i < priv->num_leds; i++) {

> -			ret = create_gpio_led(&pdata->leds[i], &priv->leds[i],

> +			const struct gpio_led *template = &pdata->leds[i];

> +			struct gpio_led_data *led_dat = &priv->leds[i];

> +

> +			if (template->gpiod)

> +				led_dat->gpiod = template->gpiod;

> +			else

> +				led_dat->gpiod =

> +					gpio_led_get_gpiod(&pdev->dev,

> +							   i, template);

> +			if (IS_ERR(led_dat->gpiod)) {

> +				dev_info(&pdev->dev, "Skipping unavailable LED gpio %d (%s)\n",

> +					 template->gpio, template->name);

> +				continue;

> +			}

> +

> +			ret = create_gpio_led(template, led_dat,

>  					      &pdev->dev, NULL,

>  					      pdata->gpio_blink_set);

>  			if (ret < 0)

> 


-- 
Best regards,
Jacek Anaszewski
Linus Walleij Sept. 9, 2018, 12:18 a.m. UTC | #2 
On Sat, Sep 8, 2018 at 10:38 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:

> > +     /* skip leds that aren't available */

> > +     if (!gpio_is_valid(template->gpio)) {

> > +             return ERR_PTR(-ENOENT);

> > +     }

>

> Curly braces are not needed for single statement block.

>

> I can fix it up by myself if you agree.


Sure thing, fix it as you like it and apply, thanks! :)

Yours,
Linus Walleij
Jacek Anaszewski Sept. 10, 2018, 7:32 p.m. UTC | #3 
On 09/09/2018 02:18 AM, Linus Walleij wrote:
> On Sat, Sep 8, 2018 at 10:38 PM Jacek Anaszewski

> <jacek.anaszewski@gmail.com> wrote:

> 

>>> +     /* skip leds that aren't available */

>>> +     if (!gpio_is_valid(template->gpio)) {

>>> +             return ERR_PTR(-ENOENT);

>>> +     }

>>

>> Curly braces are not needed for single statement block.

>>

>> I can fix it up by myself if you agree.

> 

> Sure thing, fix it as you like it and apply, thanks! :)


Fixed up and applied, thanks.

-- 
Best regards,
Jacek Anaszewski
diff mbox series

Patch

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 764c31301f90..fbc623148595 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -81,35 +81,6 @@  static int create_gpio_led(const struct gpio_led *template,
 {
 	int ret, state;
 
-	led_dat->gpiod = template->gpiod;
-	if (!led_dat->gpiod) {
-		/*
-		 * This is the legacy code path for platform code that
-		 * still uses GPIO numbers. Ultimately we would like to get
-		 * rid of this block completely.
-		 */
-		unsigned long flags = GPIOF_OUT_INIT_LOW;
-
-		/* skip leds that aren't available */
-		if (!gpio_is_valid(template->gpio)) {
-			dev_info(parent, "Skipping unavailable LED gpio %d (%s)\n",
-					template->gpio, template->name);
-			return 0;
-		}
-
-		if (template->active_low)
-			flags |= GPIOF_ACTIVE_LOW;
-
-		ret = devm_gpio_request_one(parent, template->gpio, flags,
-					    template->name);
-		if (ret < 0)
-			return ret;
-
-		led_dat->gpiod = gpio_to_desc(template->gpio);
-		if (!led_dat->gpiod)
-			return -EINVAL;
-	}
-
 	led_dat->cdev.name = template->name;
 	led_dat->cdev.default_trigger = template->default_trigger;
 	led_dat->can_sleep = gpiod_cansleep(led_dat->gpiod);
@@ -231,6 +202,53 @@  static const struct of_device_id of_gpio_leds_match[] = {
 
 MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
 
+static struct gpio_desc *gpio_led_get_gpiod(struct device *dev, int idx,
+					    const struct gpio_led *template)
+{
+	struct gpio_desc *gpiod;
+	unsigned long flags = GPIOF_OUT_INIT_LOW;
+	int ret;
+
+	/*
+	 * This means the LED does not come from the device tree
+	 * or ACPI, so let's try just getting it by index from the
+	 * device, this will hit the board file, if any and get
+	 * the GPIO from there.
+	 */
+	gpiod = devm_gpiod_get_index(dev, NULL, idx, flags);
+	if (!IS_ERR(gpiod)) {
+		gpiod_set_consumer_name(gpiod, template->name);
+		return gpiod;
+	}
+	if (PTR_ERR(gpiod) != -ENOENT)
+		return gpiod;
+
+	/*
+	 * This is the legacy code path for platform code that
+	 * still uses GPIO numbers. Ultimately we would like to get
+	 * rid of this block completely.
+	 */
+
+	/* skip leds that aren't available */
+	if (!gpio_is_valid(template->gpio)) {
+		return ERR_PTR(-ENOENT);
+	}
+
+	if (template->active_low)
+		flags |= GPIOF_ACTIVE_LOW;
+
+	ret = devm_gpio_request_one(dev, template->gpio, flags,
+				    template->name);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	gpiod = gpio_to_desc(template->gpio);
+	if (!gpiod)
+		return ERR_PTR(-EINVAL);
+
+	return gpiod;
+}
+
 static int gpio_led_probe(struct platform_device *pdev)
 {
 	struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -246,7 +264,22 @@  static int gpio_led_probe(struct platform_device *pdev)
 
 		priv->num_leds = pdata->num_leds;
 		for (i = 0; i < priv->num_leds; i++) {
-			ret = create_gpio_led(&pdata->leds[i], &priv->leds[i],
+			const struct gpio_led *template = &pdata->leds[i];
+			struct gpio_led_data *led_dat = &priv->leds[i];
+
+			if (template->gpiod)
+				led_dat->gpiod = template->gpiod;
+			else
+				led_dat->gpiod =
+					gpio_led_get_gpiod(&pdev->dev,
+							   i, template);
+			if (IS_ERR(led_dat->gpiod)) {
+				dev_info(&pdev->dev, "Skipping unavailable LED gpio %d (%s)\n",
+					 template->gpio, template->name);
+				continue;
+			}
+
+			ret = create_gpio_led(template, led_dat,
 					      &pdev->dev, NULL,
 					      pdata->gpio_blink_set);
 			if (ret < 0)