[PATCH 1/2] leds: ns2: Absorb platform data

[PATCH 1/2] leds: ns2: Absorb platform data

[Linus Walleij]

Nothing in the kernel includes the external header
<linux/platform_data/leds-kirkwood-ns2.h> so just push the
contents into the ns2 leds driver. If someone wants to use
platform data or board files to describe this device they
should be able to do so using GPIO machine descriptors but
in any case device tree should be the way forward for these
systems in all cases I can think of, and the driver already
supports that.

Cc: Simon Guinot <simon.guinot@sequanux.org>
Cc: Vincent Donnefort <vdonnefort@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/leds/leds-ns2.c                       | 30 +++++++++++++--
 .../linux/platform_data/leds-kirkwood-ns2.h   | 38 -------------------
 2 files changed, 27 insertions(+), 41 deletions(-)
 delete mode 100644 include/linux/platform_data/leds-kirkwood-ns2.h

diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
index 7c500dfdcfa3..6d37dda12c39 100644
--- a/drivers/leds/leds-ns2.c
+++ b/drivers/leds/leds-ns2.c
@@ -12,14 +12,38 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/leds.h>
 #include <linux/module.h>
-#include <linux/platform_data/leds-kirkwood-ns2.h>
 #include <linux/of.h>
-#include <linux/of_gpio.h>
 #include "leds.h"
 
+enum ns2_led_modes {
+	NS_V2_LED_OFF,
+	NS_V2_LED_ON,
+	NS_V2_LED_SATA,
+};
+
+struct ns2_led_modval {
+	enum ns2_led_modes	mode;
+	int			cmd_level;
+	int			slow_level;
+};
+
+struct ns2_led {
+	const char	*name;
+	const char	*default_trigger;
+	unsigned	cmd;
+	unsigned	slow;
+	int		num_modes;
+	struct ns2_led_modval *modval;
+};
+
+struct ns2_led_platform_data {
+	int		num_leds;
+	struct ns2_led	*leds;
+};
+
 /*
  * The Network Space v2 dual-GPIO LED is wired to a CPLD. Three different LED
  * modes are available: off, on and SATA activity blinking. The LED modes are
diff --git a/include/linux/platform_data/leds-kirkwood-ns2.h b/include/linux/platform_data/leds-kirkwood-ns2.h
deleted file mode 100644
index eb8a6860e816..000000000000
--- a/include/linux/platform_data/leds-kirkwood-ns2.h
+++ /dev/null
@@ -1,38 +0,0 @@
-/*
- * Platform data structure for Network Space v2 LED driver
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2.  This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- */
-
-#ifndef __LEDS_KIRKWOOD_NS2_H
-#define __LEDS_KIRKWOOD_NS2_H
-
-enum ns2_led_modes {
-	NS_V2_LED_OFF,
-	NS_V2_LED_ON,
-	NS_V2_LED_SATA,
-};
-
-struct ns2_led_modval {
-	enum ns2_led_modes	mode;
-	int			cmd_level;
-	int			slow_level;
-};
-
-struct ns2_led {
-	const char	*name;
-	const char	*default_trigger;
-	unsigned	cmd;
-	unsigned	slow;
-	int		num_modes;
-	struct ns2_led_modval *modval;
-};
-
-struct ns2_led_platform_data {
-	int		num_leds;
-	struct ns2_led	*leds;
-};
-
-#endif /* __LEDS_KIRKWOOD_NS2_H */
-- 
2.23.0

[PATCH 2/2] leds: ns2: Convert to GPIO descriptors

[Linus Walleij]

This converts the NS2 LED driver to use GPIO descriptors.
We take care to request the GPIOs "as is" which is what
the current driver goes to lengths to achieve, then we use
GPIOs throughout.

As the nodes for each LED does not have any corresponding
device, we need to use the DT-specific accessors to get these
GPIO descriptors from the device tree.

Cc: Simon Guinot <simon.guinot@sequanux.org>
Cc: Vincent Donnefort <vdonnefort@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/leds/leds-ns2.c | 73 +++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 42 deletions(-)

diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
index 6d37dda12c39..538ca5755602 100644
--- a/drivers/leds/leds-ns2.c
+++ b/drivers/leds/leds-ns2.c
@@ -33,8 +33,8 @@ struct ns2_led_modval {
 struct ns2_led {
 	const char	*name;
 	const char	*default_trigger;
-	unsigned	cmd;
-	unsigned	slow;
+	struct gpio_desc *cmd;
+	struct gpio_desc *slow;
 	int		num_modes;
 	struct ns2_led_modval *modval;
 };
@@ -53,8 +53,8 @@ struct ns2_led_platform_data {
 
 struct ns2_led_data {
 	struct led_classdev	cdev;
-	unsigned int		cmd;
-	unsigned int		slow;
+	struct gpio_desc	*cmd;
+	struct gpio_desc	*slow;
 	bool			can_sleep;
 	unsigned char		sata; /* True when SATA mode active. */
 	rwlock_t		rw_lock; /* Lock GPIOs. */
@@ -70,8 +70,8 @@ static int ns2_led_get_mode(struct ns2_led_data *led_dat,
 	int cmd_level;
 	int slow_level;
 
-	cmd_level = gpio_get_value_cansleep(led_dat->cmd);
-	slow_level = gpio_get_value_cansleep(led_dat->slow);
+	cmd_level = gpiod_get_value_cansleep(led_dat->cmd);
+	slow_level = gpiod_get_value_cansleep(led_dat->slow);
 
 	for (i = 0; i < led_dat->num_modes; i++) {
 		if (cmd_level == led_dat->modval[i].cmd_level &&
@@ -104,15 +104,15 @@ static void ns2_led_set_mode(struct ns2_led_data *led_dat,
 	write_lock_irqsave(&led_dat->rw_lock, flags);
 
 	if (!led_dat->can_sleep) {
-		gpio_set_value(led_dat->cmd,
-			       led_dat->modval[i].cmd_level);
-		gpio_set_value(led_dat->slow,
-			       led_dat->modval[i].slow_level);
+		gpiod_set_value(led_dat->cmd,
+				led_dat->modval[i].cmd_level);
+		gpiod_set_value(led_dat->slow,
+				led_dat->modval[i].slow_level);
 		goto exit_unlock;
 	}
 
-	gpio_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
-	gpio_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);
+	gpiod_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
+	gpiod_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);
 
 exit_unlock:
 	write_unlock_irqrestore(&led_dat->rw_lock, flags);
@@ -200,26 +200,6 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	int ret;
 	enum ns2_led_modes mode;
 
-	ret = devm_gpio_request_one(&pdev->dev, template->cmd,
-			gpio_get_value_cansleep(template->cmd) ?
-			GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
-			template->name);
-	if (ret) {
-		dev_err(&pdev->dev, "%s: failed to setup command GPIO\n",
-			template->name);
-		return ret;
-	}
-
-	ret = devm_gpio_request_one(&pdev->dev, template->slow,
-			gpio_get_value_cansleep(template->slow) ?
-			GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
-			template->name);
-	if (ret) {
-		dev_err(&pdev->dev, "%s: failed to setup slow GPIO\n",
-			template->name);
-		return ret;
-	}
-
 	rwlock_init(&led_dat->rw_lock);
 
 	led_dat->cdev.name = template->name;
@@ -229,8 +209,8 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	led_dat->cdev.groups = ns2_led_groups;
 	led_dat->cmd = template->cmd;
 	led_dat->slow = template->slow;
-	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) |
-				gpio_cansleep(led_dat->slow);
+	led_dat->can_sleep = gpiod_cansleep(led_dat->cmd) |
+				gpiod_cansleep(led_dat->slow);
 	if (led_dat->can_sleep)
 		led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;
 	else
@@ -285,17 +265,26 @@ ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata)
 		const char *string;
 		int i, num_modes;
 		struct ns2_led_modval *modval;
+		struct gpio_desc *gd;
 
-		ret = of_get_named_gpio(child, "cmd-gpio", 0);
-		if (ret < 0)
-			goto err_node_put;
-		led->cmd = ret;
-		ret = of_get_named_gpio(child, "slow-gpio", 0);
-		if (ret < 0)
-			goto err_node_put;
-		led->slow = ret;
 		ret = of_property_read_string(child, "label", &string);
 		led->name = (ret == 0) ? string : child->name;
+
+		gd = gpiod_get_from_of_node(child, "cmd-gpio", 0,
+					    GPIOD_ASIS, led->name);
+		if (IS_ERR(gd)) {
+			ret = PTR_ERR(gd);
+			goto err_node_put;
+		}
+		led->cmd = gd;
+		gd = gpiod_get_from_of_node(child, "slow-gpio", 0,
+					    GPIOD_ASIS, led->name);
+		if (IS_ERR(gd)) {
+			ret = PTR_ERR(gd);
+			goto err_node_put;
+		}
+		led->slow = gd;
+
 		ret = of_property_read_string(child, "linux,default-trigger",
 					      &string);
 		if (ret == 0)
-- 
2.23.0

Re: [PATCH 2/2] leds: ns2: Convert to GPIO descriptors

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

Hi!

> This converts the NS2 LED driver to use GPIO descriptors.
> We take care to request the GPIOs "as is" which is what
> the current driver goes to lengths to achieve, then we use
> GPIOs throughout.
> 
> As the nodes for each LED does not have any corresponding
> device, we need to use the DT-specific accessors to get these
> GPIO descriptors from the device tree.

Ok. But what is motivation for doing this? Was this tested?

Does anyone still use leds-ns2?
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/2] leds: ns2: Convert to GPIO descriptors

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Tue, Jan 07, 2020 at 03:17:42PM +0100, Pavel Machek wrote:
> Hi!
> 
> > This converts the NS2 LED driver to use GPIO descriptors.
> > We take care to request the GPIOs "as is" which is what
> > the current driver goes to lengths to achieve, then we use
> > GPIOs throughout.
> > 
> > As the nodes for each LED does not have any corresponding
> > device, we need to use the DT-specific accessors to get these
> > GPIO descriptors from the device tree.
> 
> Ok. But what is motivation for doing this? Was this tested?
> 
> Does anyone still use leds-ns2?

Hi,

Yes the leds-ns2 driver is used by The LaCie Network Space v2 family of
NAS and by the Seagate NAS 2 and 4-Bay. The latter is a bit old but not
obsolete. AFAIK all this NASes are supported by Debian.

I'll do the testing this week-end.

Simon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] leds: ns2: Convert to GPIO descriptors

[Linus Walleij]

On Tue, Jan 7, 2020 at 3:17 PM Pavel Machek <pavel@ucw.cz> wrote:

> Ok. But what is motivation for doing this?

Zip from drivers/gpio/TODO:

Starting with commit 79a9becda894 the GPIO subsystem embarked on a journey
to move away from the global GPIO numberspace and toward a decriptor-based
approach. This means that GPIO consumers, drivers and machine descriptions
ideally have no use or idea of the global GPIO numberspace that has/was
used in the inception of the GPIO subsystem.

The motivation for that in turn is that the GPIO number space has become
unmanageable and is also unpredictable due to factors such as probe ordering
and the introduction of -EPROBE_DEFER. The number space issue is the
same as to why irq is moving away from irq numbers to IRQ descriptors.

> Was this tested?

No, I change a lot of code I can't test, I rely on volunteers to test
it, it seems
Simon volunteered.

Yours,
Linus Walleij

Re: [PATCH 2/2] leds: ns2: Convert to GPIO descriptors

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 5583 bytes --]

On Tue, Jan 07, 2020 at 03:10:29PM +0100, Linus Walleij wrote:
> This converts the NS2 LED driver to use GPIO descriptors.
> We take care to request the GPIOs "as is" which is what
> the current driver goes to lengths to achieve, then we use
> GPIOs throughout.
> 
> As the nodes for each LED does not have any corresponding
> device, we need to use the DT-specific accessors to get these
> GPIO descriptors from the device tree.
> 
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> Cc: Vincent Donnefort <vdonnefort@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Hi Linus and Pavel,

I have tested this patches on a LaCie d2 Network v2 board and the LEDs
are still working as expected.

Tested-by: Simon Guinot <simon.guinot@sequanux.org>

> ---
>  drivers/leds/leds-ns2.c | 73 +++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
> index 6d37dda12c39..538ca5755602 100644
> --- a/drivers/leds/leds-ns2.c
> +++ b/drivers/leds/leds-ns2.c
> @@ -33,8 +33,8 @@ struct ns2_led_modval {
>  struct ns2_led {
>  	const char	*name;
>  	const char	*default_trigger;
> -	unsigned	cmd;
> -	unsigned	slow;
> +	struct gpio_desc *cmd;
> +	struct gpio_desc *slow;
>  	int		num_modes;
>  	struct ns2_led_modval *modval;
>  };
> @@ -53,8 +53,8 @@ struct ns2_led_platform_data {
>  
>  struct ns2_led_data {
>  	struct led_classdev	cdev;
> -	unsigned int		cmd;
> -	unsigned int		slow;
> +	struct gpio_desc	*cmd;
> +	struct gpio_desc	*slow;
>  	bool			can_sleep;
>  	unsigned char		sata; /* True when SATA mode active. */
>  	rwlock_t		rw_lock; /* Lock GPIOs. */
> @@ -70,8 +70,8 @@ static int ns2_led_get_mode(struct ns2_led_data *led_dat,
>  	int cmd_level;
>  	int slow_level;
>  
> -	cmd_level = gpio_get_value_cansleep(led_dat->cmd);
> -	slow_level = gpio_get_value_cansleep(led_dat->slow);
> +	cmd_level = gpiod_get_value_cansleep(led_dat->cmd);
> +	slow_level = gpiod_get_value_cansleep(led_dat->slow);
>  
>  	for (i = 0; i < led_dat->num_modes; i++) {
>  		if (cmd_level == led_dat->modval[i].cmd_level &&
> @@ -104,15 +104,15 @@ static void ns2_led_set_mode(struct ns2_led_data *led_dat,
>  	write_lock_irqsave(&led_dat->rw_lock, flags);
>  
>  	if (!led_dat->can_sleep) {
> -		gpio_set_value(led_dat->cmd,
> -			       led_dat->modval[i].cmd_level);
> -		gpio_set_value(led_dat->slow,
> -			       led_dat->modval[i].slow_level);
> +		gpiod_set_value(led_dat->cmd,
> +				led_dat->modval[i].cmd_level);
> +		gpiod_set_value(led_dat->slow,
> +				led_dat->modval[i].slow_level);
>  		goto exit_unlock;
>  	}
>  
> -	gpio_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
> -	gpio_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);
> +	gpiod_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
> +	gpiod_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);
>  
>  exit_unlock:
>  	write_unlock_irqrestore(&led_dat->rw_lock, flags);
> @@ -200,26 +200,6 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
>  	int ret;
>  	enum ns2_led_modes mode;
>  
> -	ret = devm_gpio_request_one(&pdev->dev, template->cmd,
> -			gpio_get_value_cansleep(template->cmd) ?
> -			GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> -			template->name);
> -	if (ret) {
> -		dev_err(&pdev->dev, "%s: failed to setup command GPIO\n",
> -			template->name);
> -		return ret;
> -	}
> -
> -	ret = devm_gpio_request_one(&pdev->dev, template->slow,
> -			gpio_get_value_cansleep(template->slow) ?
> -			GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> -			template->name);
> -	if (ret) {
> -		dev_err(&pdev->dev, "%s: failed to setup slow GPIO\n",
> -			template->name);
> -		return ret;
> -	}
> -
>  	rwlock_init(&led_dat->rw_lock);
>  
>  	led_dat->cdev.name = template->name;
> @@ -229,8 +209,8 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
>  	led_dat->cdev.groups = ns2_led_groups;
>  	led_dat->cmd = template->cmd;
>  	led_dat->slow = template->slow;
> -	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) |
> -				gpio_cansleep(led_dat->slow);
> +	led_dat->can_sleep = gpiod_cansleep(led_dat->cmd) |
> +				gpiod_cansleep(led_dat->slow);
>  	if (led_dat->can_sleep)
>  		led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;
>  	else
> @@ -285,17 +265,26 @@ ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata)
>  		const char *string;
>  		int i, num_modes;
>  		struct ns2_led_modval *modval;
> +		struct gpio_desc *gd;
>  
> -		ret = of_get_named_gpio(child, "cmd-gpio", 0);
> -		if (ret < 0)
> -			goto err_node_put;
> -		led->cmd = ret;
> -		ret = of_get_named_gpio(child, "slow-gpio", 0);
> -		if (ret < 0)
> -			goto err_node_put;
> -		led->slow = ret;
>  		ret = of_property_read_string(child, "label", &string);
>  		led->name = (ret == 0) ? string : child->name;
> +
> +		gd = gpiod_get_from_of_node(child, "cmd-gpio", 0,
> +					    GPIOD_ASIS, led->name);
> +		if (IS_ERR(gd)) {
> +			ret = PTR_ERR(gd);
> +			goto err_node_put;
> +		}
> +		led->cmd = gd;
> +		gd = gpiod_get_from_of_node(child, "slow-gpio", 0,
> +					    GPIOD_ASIS, led->name);
> +		if (IS_ERR(gd)) {
> +			ret = PTR_ERR(gd);
> +			goto err_node_put;
> +		}
> +		led->slow = gd;
> +
>  		ret = of_property_read_string(child, "linux,default-trigger",
>  					      &string);
>  		if (ret == 0)
> -- 
> 2.23.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] leds: ns2: Convert to GPIO descriptors

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Sun 2020-01-12 14:56:30, Simon Guinot wrote:
> On Tue, Jan 07, 2020 at 03:10:29PM +0100, Linus Walleij wrote:
> > This converts the NS2 LED driver to use GPIO descriptors.
> > We take care to request the GPIOs "as is" which is what
> > the current driver goes to lengths to achieve, then we use
> > GPIOs throughout.
> > 
> > As the nodes for each LED does not have any corresponding
> > device, we need to use the DT-specific accessors to get these
> > GPIO descriptors from the device tree.
> > 
> > Cc: Simon Guinot <simon.guinot@sequanux.org>
> > Cc: Vincent Donnefort <vdonnefort@gmail.com>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Hi Linus and Pavel,
> 
> I have tested this patches on a LaCie d2 Network v2 board and the LEDs
> are still working as expected.
> 
> Tested-by: Simon Guinot <simon.guinot@sequanux.org>

Thanks, applied to my for-next branch.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]