linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers)
@ 2020-09-16 23:16 Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 01/10] leds: parse linux,default-trigger DT property in LED core Marek Behún
                   ` (9 more replies)
  0 siblings, 10 replies; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún,
	Álvaro Fernández Rojas, Bartosz Golaszewski,
	Bjorn Andersson, David Rivshin, H . Nikolaus Schaller,
	Jaedon Shin, John Crispin, Kevin Cernekee, Linus Walleij,
	Ryder Lee, Sean Wang, Simon Guinot, Simon Guinot,
	Thomas Petazzoni, Vincent Donnefort

Hi,

this series is also available at
  https://git.kernel.org/pub/scm/linux/kernel/git/kabel/linux.git/log/?h=leds-cleanup-for-pavel

this is a cleanup of some LED subsystem drivers. The main reason behind
this is that I wanted to avoid code repetition by moving the parsing
of `linux,default-trigger` DT property from specific drivers to LED
core. Before this series 32 drivers parse this property (31 in
drivers/leds and one in drivers/input/keyboard/cap11xx.c).
After applying this series only 10 drivers are parsing this property.

The reason is that in discussion [1] Rob Herring says that
`linux,default-trigger` DT property is deprecated in favor of the
`function` DT property. This makes sense in a way since DT should not
be Linux specific.

After all drivers are converted we can maybe start work on slow
deprecation of this property. I do realize that we can't take it away,
but we can at least convert device trees in Linux repository to stop
using it in favor of `function` (and for default-on trigger in favor
of the `default-state` DT property), and print a deprecation warning
to the user when this `linux,default-trigger` property is present.

I wanted to prepare the way for slow deprecation of the DT property,
but it turns out that it is more difficult.

The first thing I wanted to do was to move the parsing of the
`linux,default-trigger` property to LED core. Currently many drivers
do this themselves. But it can't be moved that simply.

The first patch in this series adds the parsing of this DT property
into led_classdev_register_ext. If fwnode is given in init_data, the
property is read. This patch also removes the parsing of this property
from drivers where led_classdev_register_ext is already called. These
are:
  an30259a, aw2013, cr0014114, el15203000, gpio, lm3532, lm3692x,
  lp8860, lt3593, tlc591xx and turris-omnia.

Patches 2 to 6 do a simple conversion of some drivers to use
led_classdev_register_ext. These drivers are:
  bcm6328, bcm6358, lm3697, max77650, mt6323 and pm8058.

In patches 7 to 10 I did a bigger refactor: either they first parsed
all LED nodes and only after that started registering them, or they
used too deep nesting or were weird in some other ways:
  is31fl32xx, is31fl319x, lm36274 and ns2.

There is still a long way to go: some drivers still use the old
platform_data framework (which has a different structure for every
driver) instead of device properties via fwnode_* functions or OF).

Some of these can be changed to use device tree only, since they
already support it and the platform_data isn't used by anything in
the kernel (for example tca6507 can work with platform_data but
there is no board definition using it, all usage is via DT).

Some will be harder, because the platform_data code is still used
(pca9532 is used in arch/arm/mach-iop32x/n2100.c). Even this can
be done by converting the drivers to use fwnode_* API and converting
the mach code to use swnodes. I shall look into this later.

This series is compile tested on top of Pavel's tree. Since I
obviously don't have the various hardware that this code touches,
I am unable to test it. I therefore add maintainers and authors of
these drivers to Cc.

Marek

[1] https://lore.kernel.org/linux-leds/20200909235819.0b0fe7ce@nic.cz/T/#m3b6c154f49d0467a707c0f9a552ec87bcbd89df2

Cc: Álvaro Fernández Rojas <noltari@gmail.com>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Dan Murphy <dmurphy@ti.com>
Cc: David Rivshin <drivshin@allworx.com>
Cc: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Jaedon Shin <jaedon.shin@gmail.com>
Cc: John Crispin <john@phrozen.org>
Cc: Kevin Cernekee <cernekee@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Ryder Lee <ryder.lee@mediatek.com>
Cc: Sean Wang <sean.wang@mediatek.com>
Cc: Simon Guinot <sguinot@lacie.com>
Cc: Simon Guinot <simon.guinot@sequanux.org>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Vincent Donnefort <vdonnefort@gmail.com>

Marek Behún (10):
  leds: parse linux,default-trigger DT property in LED core
  leds: bcm6328, bcm6358: use struct led_init_data when registering
  leds: lm3697: use struct led_init_data when registering
  leds: max77650: use struct led_init_data when registering
  leds: mt6323: use struct led_init_data when registering
  leds: pm8058: use struct led_init_data when registering
  leds: is31fl32xx: use struct led_init_data when registering
  leds: is31fl319x: use struct led_init_data when registering
  leds: lm36274: use struct led_init_data when registering
  leds: ns2: refactor and use struct led_init_data

 drivers/leds/Kconfig             |   2 +-
 drivers/leds/led-class.c         |   5 +
 drivers/leds/leds-an30259a.c     |   3 -
 drivers/leds/leds-aw2013.c       |   3 -
 drivers/leds/leds-bcm6328.c      |  10 +-
 drivers/leds/leds-bcm6358.c      |  10 +-
 drivers/leds/leds-cr0014114.c    |   3 -
 drivers/leds/leds-el15203000.c   |   3 -
 drivers/leds/leds-gpio.c         |   3 -
 drivers/leds/leds-is31fl319x.c   | 204 ++++++++---------
 drivers/leds/leds-is31fl32xx.c   |  95 +++-----
 drivers/leds/leds-lm3532.c       |   3 -
 drivers/leds/leds-lm36274.c      | 100 +++++----
 drivers/leds/leds-lm3692x.c      |   3 -
 drivers/leds/leds-lm3697.c       |  18 +-
 drivers/leds/leds-lp8860.c       |   4 -
 drivers/leds/leds-lt3593.c       |   3 -
 drivers/leds/leds-max77650.c     |  24 +-
 drivers/leds/leds-mt6323.c       |  13 +-
 drivers/leds/leds-ns2.c          | 361 ++++++++++---------------------
 drivers/leds/leds-pm8058.c       |  38 ++--
 drivers/leds/leds-tlc591xx.c     |   2 -
 drivers/leds/leds-turris-omnia.c |   2 -
 23 files changed, 337 insertions(+), 575 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH leds v1 01/10] leds: parse linux,default-trigger DT property in LED core
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 02/10] leds: bcm6328, bcm6358: use struct led_init_data when registering Marek Behún
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún

Do parsing of `linux,default-trigger` DT property to LED core. Currently
it is done in many different drivers and the code is repeated.

This commit removes the parsing only from drivers where the removal is
simple, i.e. those which pass struct led_init_data with fwnode handle to
the LED classdev registering function.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/leds/led-class.c         | 5 +++++
 drivers/leds/leds-an30259a.c     | 3 ---
 drivers/leds/leds-aw2013.c       | 3 ---
 drivers/leds/leds-cr0014114.c    | 3 ---
 drivers/leds/leds-el15203000.c   | 3 ---
 drivers/leds/leds-gpio.c         | 3 ---
 drivers/leds/leds-lm3532.c       | 3 ---
 drivers/leds/leds-lm3692x.c      | 3 ---
 drivers/leds/leds-lp8860.c       | 4 ----
 drivers/leds/leds-lt3593.c       | 3 ---
 drivers/leds/leds-tlc591xx.c     | 2 --
 drivers/leds/leds-turris-omnia.c | 2 --
 12 files changed, 5 insertions(+), 32 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index cc3929f858b68..131ca83f5fb38 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -354,6 +354,11 @@ int led_classdev_register_ext(struct device *parent,
 		ret = led_compose_name(parent, init_data, composed_name);
 		if (ret < 0)
 			return ret;
+
+		if (init_data->fwnode)
+			fwnode_property_read_string(init_data->fwnode,
+				"linux,default-trigger",
+				&led_cdev->default_trigger);
 	} else {
 		proposed_name = led_cdev->name;
 	}
diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
index 82350a28a5644..d8aea64563414 100644
--- a/drivers/leds/leds-an30259a.c
+++ b/drivers/leds/leds-an30259a.c
@@ -238,9 +238,6 @@ static int an30259a_dt_init(struct i2c_client *client,
 				led->default_state = STATE_OFF;
 		}
 
-		of_property_read_string(child, "linux,default-trigger",
-					&led->cdev.default_trigger);
-
 		i++;
 	}
 
diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index d709cc1f949e3..8be8a185bfa30 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -297,9 +297,6 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 				 "DT property led-max-microamp is missing\n");
 		}
 
-		of_property_read_string(child, "linux,default-trigger",
-					&led->cdev.default_trigger);
-
 		led->cdev.brightness_set_blocking = aw2013_brightness_set;
 		led->cdev.blink_set = aw2013_blink_set;
 
diff --git a/drivers/leds/leds-cr0014114.c b/drivers/leds/leds-cr0014114.c
index 2da448ae718e9..d03cfd3c0bfbe 100644
--- a/drivers/leds/leds-cr0014114.c
+++ b/drivers/leds/leds-cr0014114.c
@@ -188,9 +188,6 @@ static int cr0014114_probe_dt(struct cr0014114 *priv)
 	device_for_each_child_node(priv->dev, child) {
 		led = &priv->leds[i];
 
-		fwnode_property_read_string(child, "linux,default-trigger",
-					    &led->ldev.default_trigger);
-
 		led->priv			  = priv;
 		led->ldev.max_brightness	  = CR_MAX_BRIGHTNESS;
 		led->ldev.brightness_set_blocking = cr0014114_set_sync;
diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
index 298b13e4807a8..6ca47f2a20041 100644
--- a/drivers/leds/leds-el15203000.c
+++ b/drivers/leds/leds-el15203000.c
@@ -263,9 +263,6 @@ static int el15203000_probe_dt(struct el15203000 *priv)
 			return -EINVAL;
 		}
 
-		fwnode_property_read_string(child, "linux,default-trigger",
-					    &led->ldev.default_trigger);
-
 		led->priv			  = priv;
 		led->ldev.max_brightness	  = LED_ON;
 		led->ldev.brightness_set_blocking = el15203000_set_blocking;
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index cf84096d88cec..93f5b1b60fdec 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -160,9 +160,6 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 
 		led_dat->gpiod = led.gpiod;
 
-		fwnode_property_read_string(child, "linux,default-trigger",
-					    &led.default_trigger);
-
 		if (!fwnode_property_read_string(child, "default-state",
 						 &state)) {
 			if (!strcmp(state, "keep"))
diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index 946ad67eaecb7..dac037141d445 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -643,9 +643,6 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 			goto child_out;
 		}
 
-		fwnode_property_read_string(child, "linux,default-trigger",
-					    &led->led_dev.default_trigger);
-
 		ret = fwnode_property_read_string(child, "label", &name);
 		if (ret)
 			snprintf(led->label, sizeof(led->label),
diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 1d7ea1b76a125..e945de45388ca 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -433,9 +433,6 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
 		return -ENODEV;
 	}
 
-	fwnode_property_read_string(child, "linux,default-trigger",
-				    &led->led_dev.default_trigger);
-
 	ret = fwnode_property_read_u32(child, "reg", &led->led_enable);
 	if (ret) {
 		dev_err(&led->client->dev, "reg DT property missing\n");
diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index ac2f5d6272dc0..20b463f290431 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -392,10 +392,6 @@ static int lp8860_probe(struct i2c_client *client,
 	if (!child_node)
 		return -EINVAL;
 
-	led->led_dev.default_trigger = of_get_property(child_node,
-					    "linux,default-trigger",
-					    NULL);
-
 	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
 						   "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(led->enable_gpio)) {
diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c
index 9079850e6ea44..da96340271dfe 100644
--- a/drivers/leds/leds-lt3593.c
+++ b/drivers/leds/leds-lt3593.c
@@ -86,9 +86,6 @@ static int lt3593_led_probe(struct platform_device *pdev)
 
 	child = device_get_next_child_node(dev, NULL);
 
-	fwnode_property_read_string(child, "linux,default-trigger",
-				    &led_data->cdev.default_trigger);
-
 	if (!fwnode_property_read_string(child, "default-state", &tmp)) {
 		if (!strcmp(tmp, "on"))
 			state = LEDS_GPIO_DEFSTATE_ON;
diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
index 1dc14051639c9..de22c28c42a56 100644
--- a/drivers/leds/leds-tlc591xx.c
+++ b/drivers/leds/leds-tlc591xx.c
@@ -204,8 +204,6 @@ tlc591xx_probe(struct i2c_client *client,
 		led = &priv->leds[reg];
 
 		led->active = true;
-		led->ldev.default_trigger =
-			of_get_property(child, "linux,default-trigger", NULL);
 
 		led->priv = priv;
 		led->led_no = reg;
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index bb23d8e166144..7ef5ee2c0773e 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -121,8 +121,6 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	cdev->max_brightness = 255;
 	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
 
-	of_property_read_string(np, "linux,default-trigger", &cdev->default_trigger);
-
 	/* put the LED into software mode */
 	ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE,
 					CMD_LED_MODE_LED(led->reg) |
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 02/10] leds: bcm6328, bcm6358: use struct led_init_data when registering
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 01/10] leds: parse linux,default-trigger DT property in LED core Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 03/10] leds: lm3697: " Marek Behún
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún,
	Álvaro Fernández Rojas, Kevin Cernekee, Jaedon Shin

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Álvaro Fernández Rojas <noltari@gmail.com>
Cc: Kevin Cernekee <cernekee@gmail.com>
Cc: Jaedon Shin <jaedon.shin@gmail.com>
---
 drivers/leds/leds-bcm6328.c | 10 ++++------
 drivers/leds/leds-bcm6358.c | 10 ++++------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c
index bad7efb751120..1aa47f3086060 100644
--- a/drivers/leds/leds-bcm6328.c
+++ b/drivers/leds/leds-bcm6328.c
@@ -328,6 +328,9 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 		       void __iomem *mem, spinlock_t *lock,
 		       unsigned long *blink_leds, unsigned long *blink_delay)
 {
+	struct led_init_data init_data = {
+		.fwnode = of_fwnode_handle(nc),
+	};
 	struct bcm6328_led *led;
 	const char *state;
 	int rc;
@@ -345,11 +348,6 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 	if (of_property_read_bool(nc, "active-low"))
 		led->active_low = true;
 
-	led->cdev.name = of_get_property(nc, "label", NULL) ? : nc->name;
-	led->cdev.default_trigger = of_get_property(nc,
-						    "linux,default-trigger",
-						    NULL);
-
 	if (!of_property_read_string(nc, "default-state", &state)) {
 		if (!strcmp(state, "on")) {
 			led->cdev.brightness = LED_FULL;
@@ -383,7 +381,7 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 	led->cdev.brightness_set = bcm6328_led_set;
 	led->cdev.blink_set = bcm6328_blink_set;
 
-	rc = led_classdev_register(dev, &led->cdev);
+	rc = led_classdev_register_ext(dev, &led->cdev, &init_data);
 	if (rc < 0)
 		return rc;
 
diff --git a/drivers/leds/leds-bcm6358.c b/drivers/leds/leds-bcm6358.c
index 94fefd456ba07..2be38211f5383 100644
--- a/drivers/leds/leds-bcm6358.c
+++ b/drivers/leds/leds-bcm6358.c
@@ -94,6 +94,9 @@ static void bcm6358_led_set(struct led_classdev *led_cdev,
 static int bcm6358_led(struct device *dev, struct device_node *nc, u32 reg,
 		       void __iomem *mem, spinlock_t *lock)
 {
+	struct led_init_data init_data = {
+		.fwnode = of_fwnode_handle(nc),
+	};
 	struct bcm6358_led *led;
 	const char *state;
 	int rc;
@@ -109,11 +112,6 @@ static int bcm6358_led(struct device *dev, struct device_node *nc, u32 reg,
 	if (of_property_read_bool(nc, "active-low"))
 		led->active_low = true;
 
-	led->cdev.name = of_get_property(nc, "label", NULL) ? : nc->name;
-	led->cdev.default_trigger = of_get_property(nc,
-						    "linux,default-trigger",
-						    NULL);
-
 	if (!of_property_read_string(nc, "default-state", &state)) {
 		if (!strcmp(state, "on")) {
 			led->cdev.brightness = LED_FULL;
@@ -137,7 +135,7 @@ static int bcm6358_led(struct device *dev, struct device_node *nc, u32 reg,
 
 	led->cdev.brightness_set = bcm6358_led_set;
 
-	rc = led_classdev_register(dev, &led->cdev);
+	rc = led_classdev_register_ext(dev, &led->cdev, &init_data);
 	if (rc < 0)
 		return rc;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 03/10] leds: lm3697: use struct led_init_data when registering
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 01/10] leds: parse linux,default-trigger DT property in LED core Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 02/10] leds: bcm6328, bcm6358: use struct led_init_data when registering Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-17 11:39   ` Dan Murphy
  2020-09-16 23:16 ` [PATCH leds v1 04/10] leds: max77650: " Marek Behún
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

Previously if the `label` DT property was not present, the code composed
name for the LED in the form
  "parent_name::"
For backwards compatibility we therefore set
  init_data->default_label = ":";
so that the LED will not get a different name if `label` property is not
present.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lm3697.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
index 024983088d599..fffeac37b8afa 100644
--- a/drivers/leds/leds-lm3697.c
+++ b/drivers/leds/leds-lm3697.c
@@ -194,7 +194,6 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 {
 	struct fwnode_handle *child = NULL;
 	struct lm3697_led *led;
-	const char *name;
 	int control_bank;
 	size_t i = 0;
 	int ret = -EINVAL;
@@ -214,6 +213,8 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 		priv->regulator = NULL;
 
 	device_for_each_child_node(priv->dev, child) {
+		struct led_init_data init_data = {};
+
 		ret = fwnode_property_read_u32(child, "reg", &control_bank);
 		if (ret) {
 			dev_err(&priv->client->dev, "reg property missing\n");
@@ -268,19 +269,12 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 		if (ret)
 			dev_warn(&priv->client->dev, "runtime-ramp properties missing\n");
 
-		fwnode_property_read_string(child, "linux,default-trigger",
-					    &led->led_dev.default_trigger);
-
-		ret = fwnode_property_read_string(child, "label", &name);
-		if (ret)
-			snprintf(led->label, sizeof(led->label),
-				"%s::", priv->client->name);
-		else
-			snprintf(led->label, sizeof(led->label),
-				 "%s:%s", priv->client->name, name);
+		init_data.fwnode = child;
+		init_data.devicename = priv->client->name;
+		/* for backwards compatibility if `label` is not present */
+		init_data.default_label = ":";
 
 		led->priv = priv;
-		led->led_dev.name = led->label;
 		led->led_dev.max_brightness = led->lmu_data.max_brightness;
 		led->led_dev.brightness_set_blocking = lm3697_brightness_set;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 04/10] leds: max77650: use struct led_init_data when registering
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
                   ` (2 preceding siblings ...)
  2020-09-16 23:16 ` [PATCH leds v1 03/10] leds: lm3697: " Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-17 10:09   ` Bartosz Golaszewski
  2020-09-16 23:16 ` [PATCH leds v1 05/10] leds: mt6323: " Marek Behún
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún, Bartosz Golaszewski

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

Previously if the `label` DT property was not present, the code composed
name for the LED in the form
  "max77650::"
For backwards compatibility we therefore set
  init_data->default_label = ":";
so that the LED will not get a different name if `label` property is not
present.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/leds/leds-max77650.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/leds-max77650.c b/drivers/leds/leds-max77650.c
index a0d4b725c9178..1eeac56b00146 100644
--- a/drivers/leds/leds-max77650.c
+++ b/drivers/leds/leds-max77650.c
@@ -66,7 +66,6 @@ static int max77650_led_probe(struct platform_device *pdev)
 	struct max77650_led *leds, *led;
 	struct device *dev;
 	struct regmap *map;
-	const char *label;
 	int rv, num_leds;
 	u32 reg;
 
@@ -86,6 +85,8 @@ static int max77650_led_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	device_for_each_child_node(dev, child) {
+		struct led_init_data init_data = {};
+
 		rv = fwnode_property_read_u32(child, "reg", &reg);
 		if (rv || reg >= MAX77650_LED_NUM_LEDS) {
 			rv = -EINVAL;
@@ -99,22 +100,13 @@ static int max77650_led_probe(struct platform_device *pdev)
 		led->cdev.brightness_set_blocking = max77650_led_brightness_set;
 		led->cdev.max_brightness = MAX77650_LED_MAX_BRIGHTNESS;
 
-		rv = fwnode_property_read_string(child, "label", &label);
-		if (rv) {
-			led->cdev.name = "max77650::";
-		} else {
-			led->cdev.name = devm_kasprintf(dev, GFP_KERNEL,
-							"max77650:%s", label);
-			if (!led->cdev.name) {
-				rv = -ENOMEM;
-				goto err_node_put;
-			}
-		}
-
-		fwnode_property_read_string(child, "linux,default-trigger",
-					    &led->cdev.default_trigger);
+		init_data.fwnode = child;
+		init_data.devicename = "max77650";
+		/* for backwards compatibility if `label` is not present */
+		init_data.default_label = ":";
 
-		rv = devm_led_classdev_register(dev, &led->cdev);
+		rv = devm_led_classdev_register_ext(dev, &led->cdev,
+						    &init_data);
 		if (rv)
 			goto err_node_put;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 05/10] leds: mt6323: use struct led_init_data when registering
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
                   ` (3 preceding siblings ...)
  2020-09-16 23:16 ` [PATCH leds v1 04/10] leds: max77650: " Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 06/10] leds: pm8058: " Marek Behún
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún, Sean Wang,
	John Crispin, Ryder Lee

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Sean Wang <sean.wang@mediatek.com>
Cc: John Crispin <john@phrozen.org>
Cc: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/leds/leds-mt6323.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
index 7b240771e45bb..c2dfafb694cfe 100644
--- a/drivers/leds/leds-mt6323.c
+++ b/drivers/leds/leds-mt6323.c
@@ -342,11 +342,6 @@ static int mt6323_led_set_dt_default(struct led_classdev *cdev,
 	const char *state;
 	int ret = 0;
 
-	led->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
-	led->cdev.default_trigger = of_get_property(np,
-						    "linux,default-trigger",
-						    NULL);
-
 	state = of_get_property(np, "default-state", NULL);
 	if (state) {
 		if (!strcmp(state, "keep")) {
@@ -402,6 +397,8 @@ static int mt6323_led_probe(struct platform_device *pdev)
 	}
 
 	for_each_available_child_of_node(np, child) {
+		struct led_init_data init_data = {};
+
 		ret = of_property_read_u32(child, "reg", &reg);
 		if (ret) {
 			dev_err(dev, "Failed to read led 'reg' property\n");
@@ -437,13 +434,15 @@ static int mt6323_led_probe(struct platform_device *pdev)
 			goto put_child_node;
 		}
 
-		ret = devm_led_classdev_register(dev, &leds->led[reg]->cdev);
+		init_data.fwnode = of_fwnode_handle(child);
+
+		ret = devm_led_classdev_register_ext(dev, &leds->led[reg]->cdev,
+						     &init_data);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to register LED: %d\n",
 				ret);
 			goto put_child_node;
 		}
-		leds->led[reg]->cdev.dev->of_node = child;
 	}
 
 	return 0;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 06/10] leds: pm8058: use struct led_init_data when registering
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
                   ` (4 preceding siblings ...)
  2020-09-16 23:16 ` [PATCH leds v1 05/10] leds: mt6323: " Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-17  0:46   ` Bjorn Andersson
  2020-09-16 23:16 ` [PATCH leds v1 07/10] leds: is31fl32xx: " Marek Behún
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún, Linus Walleij,
	Bjorn Andersson

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/leds/leds-pm8058.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c
index 7869ccdf70ce6..f6190e4af60fe 100644
--- a/drivers/leds/leds-pm8058.c
+++ b/drivers/leds/leds-pm8058.c
@@ -87,36 +87,37 @@ static enum led_brightness pm8058_led_get(struct led_classdev *cled)
 
 static int pm8058_led_probe(struct platform_device *pdev)
 {
+	struct led_init_data init_data = {};
+	struct device *dev = &pdev->dev;
+	enum led_brightness maxbright;
+	struct device_node *np;
 	struct pm8058_led *led;
-	struct device_node *np = pdev->dev.of_node;
-	int ret;
 	struct regmap *map;
 	const char *state;
-	enum led_brightness maxbright;
+	int ret;
 
-	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
 	if (!led)
 		return -ENOMEM;
 
-	led->ledtype = (u32)(unsigned long)of_device_get_match_data(&pdev->dev);
+	led->ledtype = (u32)(unsigned long)device_get_match_data(dev);
 
-	map = dev_get_regmap(pdev->dev.parent, NULL);
+	map = dev_get_regmap(dev->parent, NULL);
 	if (!map) {
-		dev_err(&pdev->dev, "Parent regmap unavailable.\n");
+		dev_err(dev, "Parent regmap unavailable.\n");
 		return -ENXIO;
 	}
 	led->map = map;
 
+	np = dev_of_node(dev);
+
 	ret = of_property_read_u32(np, "reg", &led->reg);
 	if (ret) {
-		dev_err(&pdev->dev, "no register offset specified\n");
+		dev_err(dev, "no register offset specified\n");
 		return -EINVAL;
 	}
 
 	/* Use label else node name */
-	led->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
-	led->cdev.default_trigger =
-		of_get_property(np, "linux,default-trigger", NULL);
 	led->cdev.brightness_set = pm8058_led_set;
 	led->cdev.brightness_get = pm8058_led_get;
 	if (led->ledtype == PM8058_LED_TYPE_COMMON)
@@ -142,14 +143,13 @@ static int pm8058_led_probe(struct platform_device *pdev)
 	    led->ledtype == PM8058_LED_TYPE_FLASH)
 		led->cdev.flags	= LED_CORE_SUSPENDRESUME;
 
-	ret = devm_led_classdev_register(&pdev->dev, &led->cdev);
-	if (ret) {
-		dev_err(&pdev->dev, "unable to register led \"%s\"\n",
-			led->cdev.name);
-		return ret;
-	}
+	init_data.fwnode = of_fwnode_handle(np);
+
+	ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+	if (ret)
+		dev_err(dev, "Failed to register LED for node %pOF\n", np);
 
-	return 0;
+	return ret;
 }
 
 static const struct of_device_id pm8058_leds_id_table[] = {
@@ -173,7 +173,7 @@ static struct platform_driver pm8058_led_driver = {
 	.probe		= pm8058_led_probe,
 	.driver		= {
 		.name	= "pm8058-leds",
-		.of_match_table = pm8058_leds_id_table,
+		.of_match_table = of_match_ptr(pm8058_leds_id_table),
 	},
 };
 module_platform_driver(pm8058_led_driver);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 07/10] leds: is31fl32xx: use struct led_init_data when registering
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
                   ` (5 preceding siblings ...)
  2020-09-16 23:16 ` [PATCH leds v1 06/10] leds: pm8058: " Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-17 15:23   ` Marek Behun
  2020-09-16 23:16 ` [PATCH leds v1 08/10] leds: is31fl319x: " Marek Behún
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún, H . Nikolaus Schaller,
	David Rivshin

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

This driver needed small refactoring for this to work nicely.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: H. Nikolaus Schaller <hns@goldelico.com>
Cc: David Rivshin <drivshin@allworx.com>
---
 drivers/leds/leds-is31fl32xx.c | 95 +++++++++++++---------------------
 1 file changed, 35 insertions(+), 60 deletions(-)

diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index cd768f991da10..769cce5fd2814 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -324,84 +324,63 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
 	return 0;
 }
 
-static int is31fl32xx_parse_child_dt(const struct device *dev,
-				     const struct device_node *child,
-				     struct is31fl32xx_led_data *led_data)
+static bool is31fl32xx_has_led(struct is31fl32xx_priv *priv, u8 channel)
 {
-	struct led_classdev *cdev = &led_data->cdev;
-	int ret = 0;
-	u32 reg;
+	int i;
 
-	if (of_property_read_string(child, "label", &cdev->name))
-		cdev->name = child->name;
+	for (i = 0; i < priv->num_leds; ++i)
+		if (priv->leds[i].channel == channel)
+			return true;
+
+	return false;
+}
+
+static int is31fl32xx_led_register(struct device *dev, struct device_node *np,
+				   struct is31fl32xx_led_data *led_data)
+{
+	struct led_init_data init_data = {};
+	u32 reg;
+	int ret;
 
-	ret = of_property_read_u32(child, "reg", &reg);
+	ret = of_property_read_u32(np, "reg", &reg);
 	if (ret || reg < 1 || reg > led_data->priv->cdef->channels) {
-		dev_err(dev,
-			"Child node %pOF does not have a valid reg property\n",
-			child);
+		dev_err(dev, "Node %pOF has no valid reg property\n", np);
 		return -EINVAL;
 	}
-	led_data->channel = reg;
-
-	of_property_read_string(child, "linux,default-trigger",
-				&cdev->default_trigger);
 
-	cdev->brightness_set_blocking = is31fl32xx_brightness_set;
-
-	return 0;
-}
+	if (is31fl32xx_has_led(led_data->priv, reg)) {
+		dev_err(dev, "Node %pOF reg property already used\n", np);
+		return -EEXIST;
+	}
 
-static struct is31fl32xx_led_data *is31fl32xx_find_led_data(
-					struct is31fl32xx_priv *priv,
-					u8 channel)
-{
-	size_t i;
+	led_data->channel = reg;
+	led_data->cdev.brightness_set_blocking = is31fl32xx_brightness_set;
+	init_data.fwnode = of_fwnode_handle(np);
 
-	for (i = 0; i < priv->num_leds; i++) {
-		if (priv->leds[i].channel == channel)
-			return &priv->leds[i];
-	}
+	ret = devm_led_classdev_register_ext(dev, &led_data->cdev, &init_data);
+	if (ret)
+		dev_err(dev, "Failed to register LED for %pOF: %d\n", np,
+			ret);
 
-	return NULL;
+	return ret;
 }
 
-static int is31fl32xx_parse_dt(struct device *dev,
-			       struct is31fl32xx_priv *priv)
+static int is31fl32xx_leds_register(struct device *dev,
+				    struct is31fl32xx_priv *priv)
 {
 	struct device_node *child;
-	int ret = 0;
+	int ret;
 
-	for_each_child_of_node(dev->of_node, child) {
+	for_each_child_of_node(dev_of_node(dev), child) {
 		struct is31fl32xx_led_data *led_data =
 			&priv->leds[priv->num_leds];
-		const struct is31fl32xx_led_data *other_led_data;
 
 		led_data->priv = priv;
 
-		ret = is31fl32xx_parse_child_dt(dev, child, led_data);
+		ret = is31fl32xx_led_register(dev, child, led_data);
 		if (ret)
 			goto err;
 
-		/* Detect if channel is already in use by another child */
-		other_led_data = is31fl32xx_find_led_data(priv,
-							  led_data->channel);
-		if (other_led_data) {
-			dev_err(dev,
-				"%s and %s both attempting to use channel %d\n",
-				led_data->cdev.name,
-				other_led_data->cdev.name,
-				led_data->channel);
-			goto err;
-		}
-
-		ret = devm_led_classdev_register(dev, &led_data->cdev);
-		if (ret) {
-			dev_err(dev, "failed to register PWM led for %s: %d\n",
-				led_data->cdev.name, ret);
-			goto err;
-		}
-
 		priv->num_leds++;
 	}
 
@@ -457,11 +436,7 @@ static int is31fl32xx_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	ret = is31fl32xx_parse_dt(dev, priv);
-	if (ret)
-		return ret;
-
-	return 0;
+	return is31fl32xx_leds_register(dev, priv);
 }
 
 static int is31fl32xx_remove(struct i2c_client *client)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 08/10] leds: is31fl319x: use struct led_init_data when registering
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
                   ` (6 preceding siblings ...)
  2020-09-16 23:16 ` [PATCH leds v1 07/10] leds: is31fl32xx: " Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 09/10] leds: lm36274: " Marek Behún
  2020-09-16 23:16 ` [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data Marek Behún
  9 siblings, 0 replies; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún, H . Nikolaus Schaller

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

This driver needed bigger refactor because it first parsed DT for all
LEDs and only after that started registering them.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/leds/leds-is31fl319x.c | 204 ++++++++++++++-------------------
 1 file changed, 89 insertions(+), 115 deletions(-)

diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
index 54ac50740d43d..3995f41687e16 100644
--- a/drivers/leds/leds-is31fl319x.c
+++ b/drivers/leds/leds-is31fl319x.c
@@ -63,7 +63,6 @@
 struct is31fl319x_chip {
 	const struct is31fl319x_chipdef *cdef;
 	struct i2c_client               *client;
-	struct gpio_desc		*shutdown_gpio;
 	struct regmap                   *regmap;
 	struct mutex                    lock;
 	u32                             audio_gain_db;
@@ -71,7 +70,6 @@ struct is31fl319x_chip {
 	struct is31fl319x_led {
 		struct is31fl319x_chip  *chip;
 		struct led_classdev     cdev;
-		u32                     max_microamp;
 		bool                    configured;
 	} leds[IS31FL319X_MAX_LEDS];
 };
@@ -171,117 +169,100 @@ static int is31fl319x_brightness_set(struct led_classdev *cdev,
 	return ret;
 }
 
-static int is31fl319x_parse_child_dt(const struct device *dev,
-				     const struct device_node *child,
-				     struct is31fl319x_led *led)
+static int is31fl319x_parse_max_current(struct device *dev, u32 *aggregated)
 {
-	struct led_classdev *cdev = &led->cdev;
+	struct device_node *np;
+	u32 max_microamp;
 	int ret;
 
-	if (of_property_read_string(child, "label", &cdev->name))
-		cdev->name = child->name;
-
-	ret = of_property_read_string(child, "linux,default-trigger",
-				      &cdev->default_trigger);
-	if (ret < 0 && ret != -EINVAL) /* is optional */
-		return ret;
+	*aggregated = IS31FL319X_CURRENT_MAX;
+
+	for_each_child_of_node(dev_of_node(dev), np) {
+		max_microamp = IS31FL319X_CURRENT_DEFAULT;
+		ret = of_property_read_u32(np, "led-max-microamp",
+					   &max_microamp);
+		if (!ret)
+			max_microamp = min(max_microamp,
+					   IS31FL319X_CURRENT_MAX);
+
+		if (max_microamp < IS31FL319X_CURRENT_MIN) {
+			dev_err(dev,
+				"Node %pOF led-max-microamp below supported value\n",
+				np);
+			of_node_put(np);
+			return -EINVAL;
+		}
 
-	led->max_microamp = IS31FL319X_CURRENT_DEFAULT;
-	ret = of_property_read_u32(child, "led-max-microamp",
-				   &led->max_microamp);
-	if (!ret) {
-		if (led->max_microamp < IS31FL319X_CURRENT_MIN)
-			return -EINVAL;	/* not supported */
-		led->max_microamp = min(led->max_microamp,
-					  IS31FL319X_CURRENT_MAX);
+		if (max_microamp < *aggregated)
+			*aggregated = max_microamp;
 	}
 
 	return 0;
 }
 
-static int is31fl319x_parse_dt(struct device *dev,
-			       struct is31fl319x_chip *is31)
+static int is31fl319x_led_register(struct device *dev,
+				   struct is31fl319x_chip *is31,
+				   struct device_node *np)
 {
-	struct device_node *np = dev->of_node, *child;
-	const struct of_device_id *of_dev_id;
-	int count;
+	struct led_init_data init_data = {};
+	struct is31fl319x_led *led;
+	u32 reg;
 	int ret;
 
-	if (!np)
-		return -ENODEV;
-
-	is31->shutdown_gpio = devm_gpiod_get_optional(dev,
-						"shutdown",
-						GPIOD_OUT_HIGH);
-	if (IS_ERR(is31->shutdown_gpio)) {
-		ret = PTR_ERR(is31->shutdown_gpio);
-		dev_err(dev, "Failed to get shutdown gpio: %d\n", ret);
+	ret = of_property_read_u32(np, "reg", &reg);
+	if (ret) {
+		dev_err(dev, "Cannot read reg property of %pOF\n", np);
 		return ret;
 	}
 
-	of_dev_id = of_match_device(of_is31fl319x_match, dev);
-	if (!of_dev_id) {
-		dev_err(dev, "Failed to match device with supported chips\n");
+	if (reg < 1 || reg > is31->cdef->num_leds) {
+		dev_err(dev, "Node %pOF has invalid reg property\n", np);
 		return -EINVAL;
 	}
 
-	is31->cdef = of_dev_id->data;
-
-	count = of_get_child_count(np);
+	led = &is31->leds[reg - 1];
 
-	dev_dbg(dev, "probe %s with %d leds defined in DT\n",
-		of_dev_id->compatible, count);
-
-	if (!count || count > is31->cdef->num_leds) {
-		dev_err(dev, "Number of leds defined must be between 1 and %u\n",
-			is31->cdef->num_leds);
-		return -ENODEV;
+	if (led->configured) {
+		dev_err(dev, "Node %pOF reg property already used\n", np);
+		return -EEXIST;
 	}
 
-	for_each_child_of_node(np, child) {
-		struct is31fl319x_led *led;
-		u32 reg;
+	led->configured = true;
+	led->chip = is31;
+	led->cdev.brightness_set_blocking = is31fl319x_brightness_set;
+	init_data.fwnode = of_fwnode_handle(np);
 
-		ret = of_property_read_u32(child, "reg", &reg);
-		if (ret) {
-			dev_err(dev, "Failed to read led 'reg' property\n");
-			goto put_child_node;
-		}
-
-		if (reg < 1 || reg > is31->cdef->num_leds) {
-			dev_err(dev, "invalid led reg %u\n", reg);
-			ret = -EINVAL;
-			goto put_child_node;
-		}
+	ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+	if (ret)
+		dev_err(dev, "Failed to register LED for node %pOF\n", np);
 
-		led = &is31->leds[reg - 1];
-
-		if (led->configured) {
-			dev_err(dev, "led %u is already configured\n", reg);
-			ret = -EINVAL;
-			goto put_child_node;
-		}
+	return ret;
+}
 
-		ret = is31fl319x_parse_child_dt(dev, child, led);
-		if (ret) {
-			dev_err(dev, "led %u DT parsing failed\n", reg);
-			goto put_child_node;
-		}
+static int is31fl319x_parse_and_register(struct device *dev,
+					 struct is31fl319x_chip *is31)
+{
+	struct device_node *np = dev_of_node(dev), *child;
+	int ret, count;
 
-		led->configured = true;
+	count = of_get_child_count(np);
+	if (count < 1 || count > is31->cdef->num_leds) {
+		dev_err(dev, "Between 1 and %u LEDs have to be defined\n",
+			is31->cdef->num_leds);
+		return -ENODEV;
 	}
 
-	is31->audio_gain_db = 0;
-	ret = of_property_read_u32(np, "audio-gain-db", &is31->audio_gain_db);
-	if (!ret)
+	if (!of_property_read_u32(np, "audio-gain-db", &is31->audio_gain_db))
 		is31->audio_gain_db = min(is31->audio_gain_db,
 					  IS31FL319X_AUDIO_GAIN_DB_MAX);
 
-	return 0;
+	for_each_child_of_node(np, child) {
+		ret = is31fl319x_led_register(dev, is31, child);
+		if (ret)
+			return ret;
+	}
 
-put_child_node:
-	of_node_put(child);
-	return ret;
+	return 0;
 }
 
 static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
@@ -345,30 +326,43 @@ static int is31fl319x_probe(struct i2c_client *client,
 {
 	struct is31fl319x_chip *is31;
 	struct device *dev = &client->dev;
+	struct gpio_desc *shutdown_gpio;
+	u32 aggregated_led_microamp;
 	int err;
-	int i = 0;
-	u32 aggregated_led_microamp = IS31FL319X_CURRENT_MAX;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -EIO;
 
+	/*
+	 * Kernel conventions require per-LED led-max-microamp property.
+	 * But the chip does not allow to limit individual LEDs.
+	 * So we take minimum from all subnodes for safety of hardware.
+	 */
+	err = is31fl319x_parse_max_current(dev, &aggregated_led_microamp);
+	if (err)
+		return err;
+
 	is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
 	if (!is31)
 		return -ENOMEM;
 
-	mutex_init(&is31->lock);
-
-	err = is31fl319x_parse_dt(&client->dev, is31);
-	if (err)
-		goto free_mutex;
+	shutdown_gpio = gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
+	if (IS_ERR(shutdown_gpio)) {
+		dev_err(dev, "Failed to get shutdown gpio\n");
+		return PTR_ERR(shutdown_gpio);
+	}
 
-	if (is31->shutdown_gpio) {
-		gpiod_direction_output(is31->shutdown_gpio, 0);
+	if (shutdown_gpio) {
+		gpiod_direction_output(shutdown_gpio, 0);
 		mdelay(5);
-		gpiod_direction_output(is31->shutdown_gpio, 1);
+		gpiod_direction_output(shutdown_gpio, 1);
+		gpiod_put(shutdown_gpio);
 	}
 
+	mutex_init(&is31->lock);
+
 	is31->client = client;
+	is31->cdef = device_get_match_data(dev);
 	is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
 	if (IS_ERR(is31->regmap)) {
 		dev_err(&client->dev, "failed to allocate register map\n");
@@ -387,33 +381,13 @@ static int is31fl319x_probe(struct i2c_client *client,
 		goto free_mutex;
 	}
 
-	/*
-	 * Kernel conventions require per-LED led-max-microamp property.
-	 * But the chip does not allow to limit individual LEDs.
-	 * So we take minimum from all subnodes for safety of hardware.
-	 */
-	for (i = 0; i < is31->cdef->num_leds; i++)
-		if (is31->leds[i].configured &&
-		    is31->leds[i].max_microamp < aggregated_led_microamp)
-			aggregated_led_microamp = is31->leds[i].max_microamp;
-
 	regmap_write(is31->regmap, IS31FL319X_CONFIG2,
 		     is31fl319x_microamp_to_cs(dev, aggregated_led_microamp) |
 		     is31fl319x_db_to_gain(is31->audio_gain_db));
 
-	for (i = 0; i < is31->cdef->num_leds; i++) {
-		struct is31fl319x_led *led = &is31->leds[i];
-
-		if (!led->configured)
-			continue;
-
-		led->chip = is31;
-		led->cdev.brightness_set_blocking = is31fl319x_brightness_set;
-
-		err = devm_led_classdev_register(&client->dev, &led->cdev);
-		if (err < 0)
-			goto free_mutex;
-	}
+	err = is31fl319x_parse_and_register(dev, is31);
+	if (err)
+		goto free_mutex;
 
 	return 0;
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 09/10] leds: lm36274: use struct led_init_data when registering
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
                   ` (7 preceding siblings ...)
  2020-09-16 23:16 ` [PATCH leds v1 08/10] leds: is31fl319x: " Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-17 15:28   ` Dan Murphy
  2020-09-16 23:16 ` [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data Marek Behún
  9 siblings, 1 reply; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

A small refactor was also done:
- with using devm_led_classdev_register_ext the driver remove method is
  not needed
- since only one child node is allowed for this driver, use
  device_get_next_child_node instead of device_for_each_child_node

Previously if the `label` DT property was not present, the code composed
name for the LED in the form
  "parent_name::"
For backwards compatibility we therefore set
  init_data->default_label = ":";
so that the LED will not get a different name if `label` property is not
present.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lm36274.c | 100 ++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 51 deletions(-)

diff --git a/drivers/leds/leds-lm36274.c b/drivers/leds/leds-lm36274.c
index bfeee03a0053c..7ec53b4669eac 100644
--- a/drivers/leds/leds-lm36274.c
+++ b/drivers/leds/leds-lm36274.c
@@ -67,61 +67,53 @@ static int lm36274_init(struct lm36274 *lm36274_data)
 			    enable_val);
 }
 
-static int lm36274_parse_dt(struct lm36274 *lm36274_data)
+static int lm36274_parse_dt(struct lm36274 *lm36274_data,
+			    struct led_init_data *init_data)
 {
-	struct fwnode_handle *child = NULL;
-	char label[LED_MAX_NAME_SIZE];
 	struct device *dev = &lm36274_data->pdev->dev;
-	const char *name;
-	int child_cnt;
-	int ret = -EINVAL;
+	struct fwnode_handle *fwnode;
+	int ret;
 
 	/* There should only be 1 node */
-	child_cnt = device_get_child_node_count(dev);
-	if (child_cnt != 1)
+	if (device_get_child_node_count(dev) != 1)
 		return -EINVAL;
 
-	device_for_each_child_node(dev, child) {
-		ret = fwnode_property_read_string(child, "label", &name);
-		if (ret)
-			snprintf(label, sizeof(label),
-				"%s::", lm36274_data->pdev->name);
-		else
-			snprintf(label, sizeof(label),
-				 "%s:%s", lm36274_data->pdev->name, name);
-
-		lm36274_data->num_leds = fwnode_property_count_u32(child, "led-sources");
-		if (lm36274_data->num_leds <= 0)
-			return -ENODEV;
-
-		ret = fwnode_property_read_u32_array(child, "led-sources",
-						     lm36274_data->led_sources,
-						     lm36274_data->num_leds);
-		if (ret) {
-			dev_err(dev, "led-sources property missing\n");
-			return ret;
-		}
-
-		fwnode_property_read_string(child, "linux,default-trigger",
-					&lm36274_data->led_dev.default_trigger);
+	fwnode = device_get_next_child_node(dev, NULL);
+	if (!fwnode)
+		return -ENODEV;
 
-	}
+	init_data->fwnode = fwnode;
+	init_data->devicename = lm36274_data->pdev->name;
 
-	lm36274_data->lmu_data.regmap = lm36274_data->regmap;
-	lm36274_data->lmu_data.max_brightness = MAX_BRIGHTNESS_11BIT;
-	lm36274_data->lmu_data.msb_brightness_reg = LM36274_REG_BRT_MSB;
-	lm36274_data->lmu_data.lsb_brightness_reg = LM36274_REG_BRT_LSB;
+	/* for backwards compatibility when `label` property is not present */
+	init_data->default_label = ":";
 
-	lm36274_data->led_dev.name = label;
-	lm36274_data->led_dev.max_brightness = MAX_BRIGHTNESS_11BIT;
-	lm36274_data->led_dev.brightness_set_blocking = lm36274_brightness_set;
+	lm36274_data->num_leds = fwnode_property_count_u32(fwnode,
+							   "led-sources");
+	if (lm36274_data->num_leds <= 0) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	ret = fwnode_property_read_u32_array(fwnode, "led-sources",
+					     lm36274_data->led_sources,
+					     lm36274_data->num_leds);
+	if (ret) {
+		dev_err(dev, "led-sources property missing\n");
+		goto err;
+	}
 
 	return 0;
+err:
+	fwnode_handle_put(fwnode);
+	return ret;
 }
 
 static int lm36274_probe(struct platform_device *pdev)
 {
 	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
+	struct led_init_data init_data = {};
+	struct device *dev = lmu->dev;
 	struct lm36274 *lm36274_data;
 	int ret;
 
@@ -131,32 +123,39 @@ static int lm36274_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	lm36274_data->pdev = pdev;
-	lm36274_data->dev = lmu->dev;
+	lm36274_data->dev = dev;
 	lm36274_data->regmap = lmu->regmap;
 	platform_set_drvdata(pdev, lm36274_data);
 
-	ret = lm36274_parse_dt(lm36274_data);
+	ret = lm36274_parse_dt(lm36274_data, &init_data);
 	if (ret) {
-		dev_err(lm36274_data->dev, "Failed to parse DT node\n");
+		dev_err(dev, "Failed to parse DT node\n");
 		return ret;
 	}
 
+	lm36274_data->lmu_data.regmap = lm36274_data->regmap;
+	lm36274_data->lmu_data.max_brightness = MAX_BRIGHTNESS_11BIT;
+	lm36274_data->lmu_data.msb_brightness_reg = LM36274_REG_BRT_MSB;
+	lm36274_data->lmu_data.lsb_brightness_reg = LM36274_REG_BRT_LSB;
+
 	ret = lm36274_init(lm36274_data);
 	if (ret) {
-		dev_err(lm36274_data->dev, "Failed to init the device\n");
+		dev_err(dev, "Failed to init the device\n");
 		return ret;
 	}
 
-	return led_classdev_register(lm36274_data->dev, &lm36274_data->led_dev);
-}
+	lm36274_data->led_dev.max_brightness = MAX_BRIGHTNESS_11BIT;
+	lm36274_data->led_dev.brightness_set_blocking = lm36274_brightness_set;
 
-static int lm36274_remove(struct platform_device *pdev)
-{
-	struct lm36274 *lm36274_data = platform_get_drvdata(pdev);
+	ret = devm_led_classdev_register_ext(dev, &lm36274_data->led_dev,
+					     &init_data);
+	if (ret)
+		dev_err(dev, "Failed to register LED for node %pfw\n",
+			init_data.fwnode);
 
-	led_classdev_unregister(&lm36274_data->led_dev);
+	fwnode_handle_put(init_data.fwnode);
 
-	return 0;
+	return ret;
 }
 
 static const struct of_device_id of_lm36274_leds_match[] = {
@@ -167,7 +166,6 @@ MODULE_DEVICE_TABLE(of, of_lm36274_leds_match);
 
 static struct platform_driver lm36274_driver = {
 	.probe  = lm36274_probe,
-	.remove = lm36274_remove,
 	.driver = {
 		.name = "lm36274-leds",
 	},
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
  2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
                   ` (8 preceding siblings ...)
  2020-09-16 23:16 ` [PATCH leds v1 09/10] leds: lm36274: " Marek Behún
@ 2020-09-16 23:16 ` Marek Behún
  2020-09-18 13:02   ` Simon Guinot
  9 siblings, 1 reply; 25+ messages in thread
From: Marek Behún @ 2020-09-16 23:16 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, Marek Behún, Simon Guinot,
	Simon Guinot, Vincent Donnefort, Thomas Petazzoni, Linus Walleij

By using struct led_init_data when registering we do not need to parse
`label` DT property nor `linux,default-trigger` property.

Also, move forward from platform data to device tree only:
since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
platform data structure is absorbed into the driver, because nothing
else in the source tree used it. Since nobody complained and all usage
of this driver is via device tree, refactor the code to work with
device tree only. As Linus Walleij wrote, the device tree should be the
way forward anyway.

Also build this driver if COMPILE_TEST is enabled.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Simon Guinot <simon.guinot@sequanux.org>
Cc: Simon Guinot <sguinot@lacie.com>
Cc: Vincent Donnefort <vdonnefort@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/leds/Kconfig    |   2 +-
 drivers/leds/leds-ns2.c | 361 ++++++++++++----------------------------
 2 files changed, 112 insertions(+), 251 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 4f6464a169d57..58c33636afdbf 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -644,7 +644,7 @@ config LEDS_MC13783
 config LEDS_NS2
 	tristate "LED support for Network Space v2 GPIO LEDs"
 	depends on LEDS_CLASS
-	depends on MACH_KIRKWOOD || MACH_ARMADA_370
+	depends on MACH_KIRKWOOD || MACH_ARMADA_370 || COMPILE_TEST
 	default y
 	help
 	  This option enables support for the dual-GPIO LEDs found on the
diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
index bd806e7c8017b..6a5d326c5bddc 100644
--- a/drivers/leds/leds-ns2.c
+++ b/drivers/leds/leds-ns2.c
@@ -30,20 +30,6 @@ struct ns2_led_modval {
 	int			slow_level;
 };
 
-struct ns2_led {
-	const char	*name;
-	const char	*default_trigger;
-	struct gpio_desc *cmd;
-	struct gpio_desc *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
@@ -51,7 +37,7 @@ struct ns2_led_platform_data {
  * for the command/slow GPIOs corresponds to a LED mode.
  */
 
-struct ns2_led_data {
+struct ns2_led {
 	struct led_classdev	cdev;
 	struct gpio_desc	*cmd;
 	struct gpio_desc	*slow;
@@ -62,95 +48,81 @@ struct ns2_led_data {
 	struct ns2_led_modval	*modval;
 };
 
-static int ns2_led_get_mode(struct ns2_led_data *led_dat,
-			    enum ns2_led_modes *mode)
+static int ns2_led_get_mode(struct ns2_led *led, enum ns2_led_modes *mode)
 {
-	int i;
-	int ret = -EINVAL;
-	int cmd_level;
-	int slow_level;
-
-	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 &&
-		    slow_level == led_dat->modval[i].slow_level) {
-			*mode = led_dat->modval[i].mode;
-			ret = 0;
-			break;
+	int i, cmd_level, slow_level;
+
+	cmd_level = gpiod_get_value_cansleep(led->cmd);
+	slow_level = gpiod_get_value_cansleep(led->slow);
+
+	for (i = 0; i < led->num_modes; i++) {
+		if (cmd_level == led->modval[i].cmd_level &&
+		    slow_level == led->modval[i].slow_level) {
+			*mode = led->modval[i].mode;
+			return 0;
 		}
 	}
 
-	return ret;
+	return -EINVAL;
 }
 
-static void ns2_led_set_mode(struct ns2_led_data *led_dat,
-			     enum ns2_led_modes mode)
+static void ns2_led_set_mode(struct ns2_led *led, enum ns2_led_modes mode)
 {
 	int i;
-	bool found = false;
 	unsigned long flags;
 
-	for (i = 0; i < led_dat->num_modes; i++)
-		if (mode == led_dat->modval[i].mode) {
-			found = true;
+	for (i = 0; i < led->num_modes; i++)
+		if (mode == led->modval[i].mode)
 			break;
-		}
 
-	if (!found)
+	if (i == led->num_modes)
 		return;
 
-	write_lock_irqsave(&led_dat->rw_lock, flags);
+	write_lock_irqsave(&led->rw_lock, flags);
 
-	if (!led_dat->can_sleep) {
-		gpiod_set_value(led_dat->cmd,
-				led_dat->modval[i].cmd_level);
-		gpiod_set_value(led_dat->slow,
-				led_dat->modval[i].slow_level);
+	if (!led->can_sleep) {
+		gpiod_set_value(led->cmd, led->modval[i].cmd_level);
+		gpiod_set_value(led->slow, led->modval[i].slow_level);
 		goto exit_unlock;
 	}
 
-	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);
+	gpiod_set_value_cansleep(led->cmd, led->modval[i].cmd_level);
+	gpiod_set_value_cansleep(led->slow, led->modval[i].slow_level);
 
 exit_unlock:
-	write_unlock_irqrestore(&led_dat->rw_lock, flags);
+	write_unlock_irqrestore(&led->rw_lock, flags);
 }
 
 static void ns2_led_set(struct led_classdev *led_cdev,
 			enum led_brightness value)
 {
-	struct ns2_led_data *led_dat =
-		container_of(led_cdev, struct ns2_led_data, cdev);
+	struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
 	enum ns2_led_modes mode;
 
 	if (value == LED_OFF)
 		mode = NS_V2_LED_OFF;
-	else if (led_dat->sata)
+	else if (led->sata)
 		mode = NS_V2_LED_SATA;
 	else
 		mode = NS_V2_LED_ON;
 
-	ns2_led_set_mode(led_dat, mode);
+	ns2_led_set_mode(led, mode);
 }
 
 static int ns2_led_set_blocking(struct led_classdev *led_cdev,
-			enum led_brightness value)
+				enum led_brightness value)
 {
 	ns2_led_set(led_cdev, value);
 	return 0;
 }
 
-static ssize_t ns2_led_sata_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buff, size_t count)
+static ssize_t sata_store(struct device *dev, struct device_attribute *attr,
+			  const char *buff, size_t count)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
-	struct ns2_led_data *led_dat =
-		container_of(led_cdev, struct ns2_led_data, cdev);
-	int ret;
+	struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
 	unsigned long enable;
+	int ret;
 
 	ret = kstrtoul(buff, 10, &enable);
 	if (ret < 0)
@@ -158,34 +130,33 @@ static ssize_t ns2_led_sata_store(struct device *dev,
 
 	enable = !!enable;
 
-	if (led_dat->sata == enable)
+	if (led->sata == enable)
 		goto exit;
 
-	led_dat->sata = enable;
+	led->sata = enable;
 
 	if (!led_get_brightness(led_cdev))
 		goto exit;
 
 	if (enable)
-		ns2_led_set_mode(led_dat, NS_V2_LED_SATA);
+		ns2_led_set_mode(led, NS_V2_LED_SATA);
 	else
-		ns2_led_set_mode(led_dat, NS_V2_LED_ON);
+		ns2_led_set_mode(led, NS_V2_LED_ON);
 
 exit:
 	return count;
 }
 
-static ssize_t ns2_led_sata_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
+static ssize_t sata_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
-	struct ns2_led_data *led_dat =
-		container_of(led_cdev, struct ns2_led_data, cdev);
+	struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
 
-	return sprintf(buf, "%d\n", led_dat->sata);
+	return sprintf(buf, "%d\n", led->sata);
 }
 
-static DEVICE_ATTR(sata, 0644, ns2_led_sata_show, ns2_led_sata_store);
+static DEVICE_ATTR_RW(sata);
 
 static struct attribute *ns2_led_attrs[] = {
 	&dev_attr_sata.attr,
@@ -193,147 +164,101 @@ static struct attribute *ns2_led_attrs[] = {
 };
 ATTRIBUTE_GROUPS(ns2_led);
 
-static int
-create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
-	       const struct ns2_led *template)
+static int ns2_led_register(struct device *dev, struct ns2_led *led,
+			    struct device_node *np)
 {
-	int ret;
+	struct led_init_data init_data = {};
+	struct ns2_led_modval *modval;
 	enum ns2_led_modes mode;
+	int ret, nmodes, i;
+
+	led->cmd = devm_gpiod_get_from_of_node(dev, np, "cmd-gpio", 0,
+					       GPIOD_ASIS, np->name);
+	if (IS_ERR(led->cmd))
+		return PTR_ERR(led->cmd);
+
+	led->slow = devm_gpiod_get_from_of_node(dev, np, "slow-gpio", 0,
+						GPIOD_ASIS, np->name);
+	if (IS_ERR(led->slow))
+		return PTR_ERR(led->slow);
+
+	ret = of_property_count_u32_elems(np, "modes-map");
+	if (ret <= 0 || ret % 3) {
+		dev_err(dev, "Missing or malformed modes-map in node %pOF\n",
+			np);
+		return ret < 0 ? ret : -EINVAL;
+	}
+
+	nmodes = ret / 3;
+	modval = devm_kcalloc(dev, nmodes, sizeof(*modval), GFP_KERNEL);
+	if (!modval)
+		return -ENOMEM;
+
+	for (i = 0; i < nmodes; ++i) {
+		u32 val;
+
+		of_property_read_u32_index(np, "modes-map", 3 * i, &val);
+		modval[i].mode = val;
+		of_property_read_u32_index(np, "modes-map", 3 * i + 1, &val);
+		modval[i].cmd_level = val;
+		of_property_read_u32_index(np, "modes-map", 3 * i + 2, &val);
+		modval[i].slow_level = val;
+	}
+
+	led->num_modes = nmodes;
+	led->modval = modval;
 
-	rwlock_init(&led_dat->rw_lock);
-
-	led_dat->cdev.name = template->name;
-	led_dat->cdev.default_trigger = template->default_trigger;
-	led_dat->cdev.blink_set = NULL;
-	led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
-	led_dat->cdev.groups = ns2_led_groups;
-	led_dat->cmd = template->cmd;
-	led_dat->slow = template->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;
+	rwlock_init(&led->rw_lock);
+
+	led->cdev.flags |= LED_CORE_SUSPENDRESUME;
+	led->cdev.groups = ns2_led_groups;
+	led->can_sleep = gpiod_cansleep(led->cmd) || gpiod_cansleep(led->slow);
+	if (led->can_sleep)
+		led->cdev.brightness_set_blocking = ns2_led_set_blocking;
 	else
-		led_dat->cdev.brightness_set = ns2_led_set;
-	led_dat->modval = template->modval;
-	led_dat->num_modes = template->num_modes;
+		led->cdev.brightness_set = ns2_led_set;
 
-	ret = ns2_led_get_mode(led_dat, &mode);
+	ret = ns2_led_get_mode(led, &mode);
 	if (ret < 0)
 		return ret;
 
 	/* Set LED initial state. */
-	led_dat->sata = (mode == NS_V2_LED_SATA) ? 1 : 0;
-	led_dat->cdev.brightness =
-		(mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
+	led->sata = (mode == NS_V2_LED_SATA) ? 1 : 0;
+	led->cdev.brightness = (mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
 
-	ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
-	if (ret < 0)
-		return ret;
+	init_data.fwnode = of_fwnode_handle(np);
 
-	return 0;
-}
+	ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+	if (ret < 0)
+		dev_err(dev, "Failed to register LED for node %pOF\n", np);
 
-static void delete_ns2_led(struct ns2_led_data *led_dat)
-{
-	led_classdev_unregister(&led_dat->cdev);
+	return ret;
 }
 
-#ifdef CONFIG_OF_GPIO
-/*
- * Translate OpenFirmware node properties into platform_data.
- */
-static int
-ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata)
+static int ns2_led_probe(struct platform_device *pdev)
 {
-	struct device_node *np = dev->of_node;
+	struct device *dev = &pdev->dev;
 	struct device_node *child;
-	struct ns2_led *led, *leds;
-	int ret, num_leds = 0;
+	struct ns2_led *leds;
+	int count, ret;
 
-	num_leds = of_get_child_count(np);
-	if (!num_leds)
+	count = device_get_child_node_count(dev);
+	if (count < 1) {
+		dev_err(dev, "Device has no child nodes\n");
 		return -ENODEV;
+	}
 
-	leds = devm_kcalloc(dev, num_leds, sizeof(struct ns2_led),
-			    GFP_KERNEL);
+	leds = devm_kzalloc(dev, array_size(count, sizeof(*leds)), GFP_KERNEL);
 	if (!leds)
 		return -ENOMEM;
 
-	led = leds;
-	for_each_child_of_node(np, child) {
-		const char *string;
-		int i, num_modes;
-		struct ns2_led_modval *modval;
-		struct gpio_desc *gd;
-
-		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)
-			led->default_trigger = string;
-
-		ret = of_property_count_u32_elems(child, "modes-map");
-		if (ret < 0 || ret % 3) {
-			dev_err(dev,
-				"Missing or malformed modes-map property\n");
-			ret = -EINVAL;
-			goto err_node_put;
-		}
-
-		num_modes = ret / 3;
-		modval = devm_kcalloc(dev,
-				      num_modes,
-				      sizeof(struct ns2_led_modval),
-				      GFP_KERNEL);
-		if (!modval) {
-			ret = -ENOMEM;
-			goto err_node_put;
-		}
-
-		for (i = 0; i < num_modes; i++) {
-			of_property_read_u32_index(child,
-						"modes-map", 3 * i,
-						(u32 *) &modval[i].mode);
-			of_property_read_u32_index(child,
-						"modes-map", 3 * i + 1,
-						(u32 *) &modval[i].cmd_level);
-			of_property_read_u32_index(child,
-						"modes-map", 3 * i + 2,
-						(u32 *) &modval[i].slow_level);
-		}
-
-		led->num_modes = num_modes;
-		led->modval = modval;
-
-		led++;
+	for_each_available_child_of_node(dev_of_node(dev), child) {
+		ret = ns2_led_register(dev, leds++, child);
+		if (ret)
+			return ret;
 	}
 
-	pdata->leds = leds;
-	pdata->num_leds = num_leds;
-
 	return 0;
-
-err_node_put:
-	of_node_put(child);
-	return ret;
 }
 
 static const struct of_device_id of_ns2_leds_match[] = {
@@ -341,73 +266,9 @@ static const struct of_device_id of_ns2_leds_match[] = {
 	{},
 };
 MODULE_DEVICE_TABLE(of, of_ns2_leds_match);
-#endif /* CONFIG_OF_GPIO */
-
-struct ns2_led_priv {
-	int num_leds;
-	struct ns2_led_data leds_data[];
-};
-
-static int ns2_led_probe(struct platform_device *pdev)
-{
-	struct ns2_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct ns2_led_priv *priv;
-	int i;
-	int ret;
-
-#ifdef CONFIG_OF_GPIO
-	if (!pdata) {
-		pdata = devm_kzalloc(&pdev->dev,
-				     sizeof(struct ns2_led_platform_data),
-				     GFP_KERNEL);
-		if (!pdata)
-			return -ENOMEM;
-
-		ret = ns2_leds_get_of_pdata(&pdev->dev, pdata);
-		if (ret)
-			return ret;
-	}
-#else
-	if (!pdata)
-		return -EINVAL;
-#endif /* CONFIG_OF_GPIO */
-
-	priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds_data, pdata->num_leds), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-	priv->num_leds = pdata->num_leds;
-
-	for (i = 0; i < priv->num_leds; i++) {
-		ret = create_ns2_led(pdev, &priv->leds_data[i],
-				     &pdata->leds[i]);
-		if (ret < 0) {
-			for (i = i - 1; i >= 0; i--)
-				delete_ns2_led(&priv->leds_data[i]);
-			return ret;
-		}
-	}
-
-	platform_set_drvdata(pdev, priv);
-
-	return 0;
-}
-
-static int ns2_led_remove(struct platform_device *pdev)
-{
-	int i;
-	struct ns2_led_priv *priv;
-
-	priv = platform_get_drvdata(pdev);
-
-	for (i = 0; i < priv->num_leds; i++)
-		delete_ns2_led(&priv->leds_data[i]);
-
-	return 0;
-}
 
 static struct platform_driver ns2_led_driver = {
 	.probe		= ns2_led_probe,
-	.remove		= ns2_led_remove,
 	.driver		= {
 		.name		= "leds-ns2",
 		.of_match_table	= of_match_ptr(of_ns2_leds_match),
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 06/10] leds: pm8058: use struct led_init_data when registering
  2020-09-16 23:16 ` [PATCH leds v1 06/10] leds: pm8058: " Marek Behún
@ 2020-09-17  0:46   ` Bjorn Andersson
  2020-09-17 15:24     ` Marek Behun
  0 siblings, 1 reply; 25+ messages in thread
From: Bjorn Andersson @ 2020-09-17  0:46 UTC (permalink / raw)
  To: Marek Beh?n
  Cc: linux-leds, Pavel Machek, Dan Murphy, Ond??ej Jirman,
	linux-kernel, Rob Herring, devicetree, Linus Walleij

On Wed 16 Sep 18:16 CDT 2020, Marek Beh?n wrote:

> By using struct led_init_data when registering we do not need to parse
> `label` DT property nor `linux,default-trigger` property.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/leds/leds-pm8058.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c
> index 7869ccdf70ce6..f6190e4af60fe 100644
> --- a/drivers/leds/leds-pm8058.c
> +++ b/drivers/leds/leds-pm8058.c
> @@ -87,36 +87,37 @@ static enum led_brightness pm8058_led_get(struct led_classdev *cled)
>  
>  static int pm8058_led_probe(struct platform_device *pdev)
>  {
> +	struct led_init_data init_data = {};
> +	struct device *dev = &pdev->dev;
> +	enum led_brightness maxbright;
> +	struct device_node *np;
>  	struct pm8058_led *led;
> -	struct device_node *np = pdev->dev.of_node;
> -	int ret;
>  	struct regmap *map;
>  	const char *state;
> -	enum led_brightness maxbright;
> +	int ret;
>  
> -	led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);

The pdev->dev -> dev and of_node changes are reasonable, but shouldn't
be part of this patch. It simply makes it hard to reason about he actual
change.

Please respin this with only the introduction of led_init_data.

Thanks,
Bjorn

>  	if (!led)
>  		return -ENOMEM;
>  
> -	led->ledtype = (u32)(unsigned long)of_device_get_match_data(&pdev->dev);
> +	led->ledtype = (u32)(unsigned long)device_get_match_data(dev);
>  
> -	map = dev_get_regmap(pdev->dev.parent, NULL);
> +	map = dev_get_regmap(dev->parent, NULL);
>  	if (!map) {
> -		dev_err(&pdev->dev, "Parent regmap unavailable.\n");
> +		dev_err(dev, "Parent regmap unavailable.\n");
>  		return -ENXIO;
>  	}
>  	led->map = map;
>  
> +	np = dev_of_node(dev);
> +
>  	ret = of_property_read_u32(np, "reg", &led->reg);
>  	if (ret) {
> -		dev_err(&pdev->dev, "no register offset specified\n");
> +		dev_err(dev, "no register offset specified\n");
>  		return -EINVAL;
>  	}
>  
>  	/* Use label else node name */
> -	led->cdev.name = of_get_property(np, "label", NULL) ? : np->name;
> -	led->cdev.default_trigger =
> -		of_get_property(np, "linux,default-trigger", NULL);
>  	led->cdev.brightness_set = pm8058_led_set;
>  	led->cdev.brightness_get = pm8058_led_get;
>  	if (led->ledtype == PM8058_LED_TYPE_COMMON)
> @@ -142,14 +143,13 @@ static int pm8058_led_probe(struct platform_device *pdev)
>  	    led->ledtype == PM8058_LED_TYPE_FLASH)
>  		led->cdev.flags	= LED_CORE_SUSPENDRESUME;
>  
> -	ret = devm_led_classdev_register(&pdev->dev, &led->cdev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "unable to register led \"%s\"\n",
> -			led->cdev.name);
> -		return ret;
> -	}
> +	init_data.fwnode = of_fwnode_handle(np);
> +
> +	ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
> +	if (ret)
> +		dev_err(dev, "Failed to register LED for node %pOF\n", np);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static const struct of_device_id pm8058_leds_id_table[] = {
> @@ -173,7 +173,7 @@ static struct platform_driver pm8058_led_driver = {
>  	.probe		= pm8058_led_probe,
>  	.driver		= {
>  		.name	= "pm8058-leds",
> -		.of_match_table = pm8058_leds_id_table,
> +		.of_match_table = of_match_ptr(pm8058_leds_id_table),
>  	},
>  };
>  module_platform_driver(pm8058_led_driver);
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 04/10] leds: max77650: use struct led_init_data when registering
  2020-09-16 23:16 ` [PATCH leds v1 04/10] leds: max77650: " Marek Behún
@ 2020-09-17 10:09   ` Bartosz Golaszewski
  0 siblings, 0 replies; 25+ messages in thread
From: Bartosz Golaszewski @ 2020-09-17 10:09 UTC (permalink / raw)
  To: Marek Behún
  Cc: Linux LED Subsystem, Pavel Machek, Dan Murphy,
	Ondřej Jirman, LKML, Rob Herring, linux-devicetree

On Thu, Sep 17, 2020 at 1:16 AM Marek Behún <marek.behun@nic.cz> wrote:
>
> By using struct led_init_data when registering we do not need to parse
> `label` DT property nor `linux,default-trigger` property.
>
> Previously if the `label` DT property was not present, the code composed
> name for the LED in the form
>   "max77650::"
> For backwards compatibility we therefore set
>   init_data->default_label = ":";
> so that the LED will not get a different name if `label` property is not
> present.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/leds/leds-max77650.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/leds/leds-max77650.c b/drivers/leds/leds-max77650.c
> index a0d4b725c9178..1eeac56b00146 100644
> --- a/drivers/leds/leds-max77650.c
> +++ b/drivers/leds/leds-max77650.c
> @@ -66,7 +66,6 @@ static int max77650_led_probe(struct platform_device *pdev)
>         struct max77650_led *leds, *led;
>         struct device *dev;
>         struct regmap *map;
> -       const char *label;
>         int rv, num_leds;
>         u32 reg;
>
> @@ -86,6 +85,8 @@ static int max77650_led_probe(struct platform_device *pdev)
>                 return -ENODEV;
>
>         device_for_each_child_node(dev, child) {
> +               struct led_init_data init_data = {};
> +
>                 rv = fwnode_property_read_u32(child, "reg", &reg);
>                 if (rv || reg >= MAX77650_LED_NUM_LEDS) {
>                         rv = -EINVAL;
> @@ -99,22 +100,13 @@ static int max77650_led_probe(struct platform_device *pdev)
>                 led->cdev.brightness_set_blocking = max77650_led_brightness_set;
>                 led->cdev.max_brightness = MAX77650_LED_MAX_BRIGHTNESS;
>
> -               rv = fwnode_property_read_string(child, "label", &label);
> -               if (rv) {
> -                       led->cdev.name = "max77650::";
> -               } else {
> -                       led->cdev.name = devm_kasprintf(dev, GFP_KERNEL,
> -                                                       "max77650:%s", label);
> -                       if (!led->cdev.name) {
> -                               rv = -ENOMEM;
> -                               goto err_node_put;
> -                       }
> -               }
> -
> -               fwnode_property_read_string(child, "linux,default-trigger",
> -                                           &led->cdev.default_trigger);
> +               init_data.fwnode = child;
> +               init_data.devicename = "max77650";
> +               /* for backwards compatibility if `label` is not present */
> +               init_data.default_label = ":";
>
> -               rv = devm_led_classdev_register(dev, &led->cdev);
> +               rv = devm_led_classdev_register_ext(dev, &led->cdev,
> +                                                   &init_data);
>                 if (rv)
>                         goto err_node_put;
>
> --
> 2.26.2
>

I don't know this new API very well but looks good to me.

Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 03/10] leds: lm3697: use struct led_init_data when registering
  2020-09-16 23:16 ` [PATCH leds v1 03/10] leds: lm3697: " Marek Behún
@ 2020-09-17 11:39   ` Dan Murphy
  2020-09-17 15:24     ` Marek Behun
  0 siblings, 1 reply; 25+ messages in thread
From: Dan Murphy @ 2020-09-17 11:39 UTC (permalink / raw)
  To: Marek Behún, linux-leds
  Cc: Pavel Machek, Ondřej Jirman, linux-kernel, Rob Herring, devicetree

Marek

On 9/16/20 6:16 PM, Marek Behún wrote:
> By using struct led_init_data when registering we do not need to parse
> `label` DT property nor `linux,default-trigger` property.

I would prefer 2 separate patches. One implementing the init_data and 
the other removing the default trigger

Dan



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 07/10] leds: is31fl32xx: use struct led_init_data when registering
  2020-09-16 23:16 ` [PATCH leds v1 07/10] leds: is31fl32xx: " Marek Behún
@ 2020-09-17 15:23   ` Marek Behun
  0 siblings, 0 replies; 25+ messages in thread
From: Marek Behun @ 2020-09-17 15:23 UTC (permalink / raw)
  To: linux-leds
  Cc: Pavel Machek, Dan Murphy, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree, H . Nikolaus Schaller, David Rivshin

This can be done without refactoring, please ignore this patch in this
spin.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 03/10] leds: lm3697: use struct led_init_data when registering
  2020-09-17 11:39   ` Dan Murphy
@ 2020-09-17 15:24     ` Marek Behun
  0 siblings, 0 replies; 25+ messages in thread
From: Marek Behun @ 2020-09-17 15:24 UTC (permalink / raw)
  To: Dan Murphy
  Cc: linux-leds, Pavel Machek, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree

On Thu, 17 Sep 2020 06:39:56 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> Marek
> 
> On 9/16/20 6:16 PM, Marek Behún wrote:
> > By using struct led_init_data when registering we do not need to parse
> > `label` DT property nor `linux,default-trigger` property.  
> 
> I would prefer 2 separate patches. One implementing the init_data and 
> the other removing the default trigger
> 
> Dan
> 
> 

Am working on it :)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 06/10] leds: pm8058: use struct led_init_data when registering
  2020-09-17  0:46   ` Bjorn Andersson
@ 2020-09-17 15:24     ` Marek Behun
  0 siblings, 0 replies; 25+ messages in thread
From: Marek Behun @ 2020-09-17 15:24 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: linux-leds, Pavel Machek, Dan Murphy, Ond??ej Jirman,
	linux-kernel, Rob Herring, devicetree, Linus Walleij

On Wed, 16 Sep 2020 19:46:25 -0500
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:

> The pdev->dev -> dev and of_node changes are reasonable, but shouldn't
> be part of this patch. It simply makes it hard to reason about he actual
> change.
> 
> Please respin this with only the introduction of led_init_data.
> 
> Thanks,
> Bjorn
> 
Am working on this :)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 09/10] leds: lm36274: use struct led_init_data when registering
  2020-09-16 23:16 ` [PATCH leds v1 09/10] leds: lm36274: " Marek Behún
@ 2020-09-17 15:28   ` Dan Murphy
  2020-09-17 15:54     ` Marek Behun
  0 siblings, 1 reply; 25+ messages in thread
From: Dan Murphy @ 2020-09-17 15:28 UTC (permalink / raw)
  To: Marek Behún, linux-leds
  Cc: Pavel Machek, Ondřej Jirman, linux-kernel, Rob Herring, devicetree

Marek

On 9/16/20 6:16 PM, Marek Behún wrote:
> By using struct led_init_data when registering we do not need to parse
> `label` DT property nor `linux,default-trigger` property.
>
> A small refactor was also done:
> - with using devm_led_classdev_register_ext the driver remove method is
>    not needed
> - since only one child node is allowed for this driver, use
>    device_get_next_child_node instead of device_for_each_child_node
>
> Previously if the `label` DT property was not present, the code composed
> name for the LED in the form
>    "parent_name::"
> For backwards compatibility we therefore set
>    init_data->default_label = ":";
> so that the LED will not get a different name if `label` property is not
> present.

You are going to re-factor this as well a lot of changes in a single 
patch is hard to review

Dan


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 09/10] leds: lm36274: use struct led_init_data when registering
  2020-09-17 15:28   ` Dan Murphy
@ 2020-09-17 15:54     ` Marek Behun
  0 siblings, 0 replies; 25+ messages in thread
From: Marek Behun @ 2020-09-17 15:54 UTC (permalink / raw)
  To: Dan Murphy
  Cc: linux-leds, Pavel Machek, Ondřej Jirman, linux-kernel,
	Rob Herring, devicetree

On Thu, 17 Sep 2020 10:28:12 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> Marek
> 
> On 9/16/20 6:16 PM, Marek Behún wrote:
> > By using struct led_init_data when registering we do not need to parse
> > `label` DT property nor `linux,default-trigger` property.
> >
> > A small refactor was also done:
> > - with using devm_led_classdev_register_ext the driver remove method is
> >    not needed
> > - since only one child node is allowed for this driver, use
> >    device_get_next_child_node instead of device_for_each_child_node
> >
> > Previously if the `label` DT property was not present, the code composed
> > name for the LED in the form
> >    "parent_name::"
> > For backwards compatibility we therefore set
> >    init_data->default_label = ":";
> > so that the LED will not get a different name if `label` property is not
> > present.  
> 
> You are going to re-factor this as well a lot of changes in a single 
> patch is hard to review
> 
> Dan
> 
I am trying to do this now.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
  2020-09-16 23:16 ` [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data Marek Behún
@ 2020-09-18 13:02   ` Simon Guinot
  2020-09-18 17:14     ` Marek Behun
  0 siblings, 1 reply; 25+ messages in thread
From: Simon Guinot @ 2020-09-18 13:02 UTC (permalink / raw)
  To: Marek Behún
  Cc: linux-leds, Pavel Machek, Dan Murphy, Ondřej Jirman,
	linux-kernel, Rob Herring, devicetree, Simon Guinot,
	Vincent Donnefort, Thomas Petazzoni, Linus Walleij

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

On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:

Hi Marek,

> By using struct led_init_data when registering we do not need to parse
> `label` DT property nor `linux,default-trigger` property.
> 
> Also, move forward from platform data to device tree only:
> since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> platform data structure is absorbed into the driver, because nothing
> else in the source tree used it. Since nobody complained and all usage

Well, I probably should have...

I am using this driver on the Seagate Superbee NAS devices. This devices
are based on a x86 SoC. Since I have been unable to get from the ODM the
LED information written in the ACPI tables, then platform data are used
to pass the LED description to the driver.

The support of this boards is not available mainline yet but it is still
on my todo list. So that's why I am complaining right now :) If it is
not too much trouble I'd like to keep platform data support in this
driver.

Thanks in advance.

Simon

> of this driver is via device tree, refactor the code to work with
> device tree only. As Linus Walleij wrote, the device tree should be the
> way forward anyway.
> 
> Also build this driver if COMPILE_TEST is enabled.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> Cc: Simon Guinot <sguinot@lacie.com>
> Cc: Vincent Donnefort <vdonnefort@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/leds/Kconfig    |   2 +-
>  drivers/leds/leds-ns2.c | 361 ++++++++++++----------------------------
>  2 files changed, 112 insertions(+), 251 deletions(-)
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 4f6464a169d57..58c33636afdbf 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -644,7 +644,7 @@ config LEDS_MC13783
>  config LEDS_NS2
>  	tristate "LED support for Network Space v2 GPIO LEDs"
>  	depends on LEDS_CLASS
> -	depends on MACH_KIRKWOOD || MACH_ARMADA_370
> +	depends on MACH_KIRKWOOD || MACH_ARMADA_370 || COMPILE_TEST
>  	default y
>  	help
>  	  This option enables support for the dual-GPIO LEDs found on the
> diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
> index bd806e7c8017b..6a5d326c5bddc 100644
> --- a/drivers/leds/leds-ns2.c
> +++ b/drivers/leds/leds-ns2.c
> @@ -30,20 +30,6 @@ struct ns2_led_modval {
>  	int			slow_level;
>  };
>  
> -struct ns2_led {
> -	const char	*name;
> -	const char	*default_trigger;
> -	struct gpio_desc *cmd;
> -	struct gpio_desc *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
> @@ -51,7 +37,7 @@ struct ns2_led_platform_data {
>   * for the command/slow GPIOs corresponds to a LED mode.
>   */
>  
> -struct ns2_led_data {
> +struct ns2_led {
>  	struct led_classdev	cdev;
>  	struct gpio_desc	*cmd;
>  	struct gpio_desc	*slow;
> @@ -62,95 +48,81 @@ struct ns2_led_data {
>  	struct ns2_led_modval	*modval;
>  };
>  
> -static int ns2_led_get_mode(struct ns2_led_data *led_dat,
> -			    enum ns2_led_modes *mode)
> +static int ns2_led_get_mode(struct ns2_led *led, enum ns2_led_modes *mode)
>  {
> -	int i;
> -	int ret = -EINVAL;
> -	int cmd_level;
> -	int slow_level;
> -
> -	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 &&
> -		    slow_level == led_dat->modval[i].slow_level) {
> -			*mode = led_dat->modval[i].mode;
> -			ret = 0;
> -			break;
> +	int i, cmd_level, slow_level;
> +
> +	cmd_level = gpiod_get_value_cansleep(led->cmd);
> +	slow_level = gpiod_get_value_cansleep(led->slow);
> +
> +	for (i = 0; i < led->num_modes; i++) {
> +		if (cmd_level == led->modval[i].cmd_level &&
> +		    slow_level == led->modval[i].slow_level) {
> +			*mode = led->modval[i].mode;
> +			return 0;
>  		}
>  	}
>  
> -	return ret;
> +	return -EINVAL;
>  }
>  
> -static void ns2_led_set_mode(struct ns2_led_data *led_dat,
> -			     enum ns2_led_modes mode)
> +static void ns2_led_set_mode(struct ns2_led *led, enum ns2_led_modes mode)
>  {
>  	int i;
> -	bool found = false;
>  	unsigned long flags;
>  
> -	for (i = 0; i < led_dat->num_modes; i++)
> -		if (mode == led_dat->modval[i].mode) {
> -			found = true;
> +	for (i = 0; i < led->num_modes; i++)
> +		if (mode == led->modval[i].mode)
>  			break;
> -		}
>  
> -	if (!found)
> +	if (i == led->num_modes)
>  		return;
>  
> -	write_lock_irqsave(&led_dat->rw_lock, flags);
> +	write_lock_irqsave(&led->rw_lock, flags);
>  
> -	if (!led_dat->can_sleep) {
> -		gpiod_set_value(led_dat->cmd,
> -				led_dat->modval[i].cmd_level);
> -		gpiod_set_value(led_dat->slow,
> -				led_dat->modval[i].slow_level);
> +	if (!led->can_sleep) {
> +		gpiod_set_value(led->cmd, led->modval[i].cmd_level);
> +		gpiod_set_value(led->slow, led->modval[i].slow_level);
>  		goto exit_unlock;
>  	}
>  
> -	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);
> +	gpiod_set_value_cansleep(led->cmd, led->modval[i].cmd_level);
> +	gpiod_set_value_cansleep(led->slow, led->modval[i].slow_level);
>  
>  exit_unlock:
> -	write_unlock_irqrestore(&led_dat->rw_lock, flags);
> +	write_unlock_irqrestore(&led->rw_lock, flags);
>  }
>  
>  static void ns2_led_set(struct led_classdev *led_cdev,
>  			enum led_brightness value)
>  {
> -	struct ns2_led_data *led_dat =
> -		container_of(led_cdev, struct ns2_led_data, cdev);
> +	struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
>  	enum ns2_led_modes mode;
>  
>  	if (value == LED_OFF)
>  		mode = NS_V2_LED_OFF;
> -	else if (led_dat->sata)
> +	else if (led->sata)
>  		mode = NS_V2_LED_SATA;
>  	else
>  		mode = NS_V2_LED_ON;
>  
> -	ns2_led_set_mode(led_dat, mode);
> +	ns2_led_set_mode(led, mode);
>  }
>  
>  static int ns2_led_set_blocking(struct led_classdev *led_cdev,
> -			enum led_brightness value)
> +				enum led_brightness value)
>  {
>  	ns2_led_set(led_cdev, value);
>  	return 0;
>  }
>  
> -static ssize_t ns2_led_sata_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buff, size_t count)
> +static ssize_t sata_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buff, size_t count)
>  {
>  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> -	struct ns2_led_data *led_dat =
> -		container_of(led_cdev, struct ns2_led_data, cdev);
> -	int ret;
> +	struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
>  	unsigned long enable;
> +	int ret;
>  
>  	ret = kstrtoul(buff, 10, &enable);
>  	if (ret < 0)
> @@ -158,34 +130,33 @@ static ssize_t ns2_led_sata_store(struct device *dev,
>  
>  	enable = !!enable;
>  
> -	if (led_dat->sata == enable)
> +	if (led->sata == enable)
>  		goto exit;
>  
> -	led_dat->sata = enable;
> +	led->sata = enable;
>  
>  	if (!led_get_brightness(led_cdev))
>  		goto exit;
>  
>  	if (enable)
> -		ns2_led_set_mode(led_dat, NS_V2_LED_SATA);
> +		ns2_led_set_mode(led, NS_V2_LED_SATA);
>  	else
> -		ns2_led_set_mode(led_dat, NS_V2_LED_ON);
> +		ns2_led_set_mode(led, NS_V2_LED_ON);
>  
>  exit:
>  	return count;
>  }
>  
> -static ssize_t ns2_led_sata_show(struct device *dev,
> -				 struct device_attribute *attr, char *buf)
> +static ssize_t sata_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
>  {
>  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> -	struct ns2_led_data *led_dat =
> -		container_of(led_cdev, struct ns2_led_data, cdev);
> +	struct ns2_led *led = container_of(led_cdev, struct ns2_led, cdev);
>  
> -	return sprintf(buf, "%d\n", led_dat->sata);
> +	return sprintf(buf, "%d\n", led->sata);
>  }
>  
> -static DEVICE_ATTR(sata, 0644, ns2_led_sata_show, ns2_led_sata_store);
> +static DEVICE_ATTR_RW(sata);
>  
>  static struct attribute *ns2_led_attrs[] = {
>  	&dev_attr_sata.attr,
> @@ -193,147 +164,101 @@ static struct attribute *ns2_led_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(ns2_led);
>  
> -static int
> -create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
> -	       const struct ns2_led *template)
> +static int ns2_led_register(struct device *dev, struct ns2_led *led,
> +			    struct device_node *np)
>  {
> -	int ret;
> +	struct led_init_data init_data = {};
> +	struct ns2_led_modval *modval;
>  	enum ns2_led_modes mode;
> +	int ret, nmodes, i;
> +
> +	led->cmd = devm_gpiod_get_from_of_node(dev, np, "cmd-gpio", 0,
> +					       GPIOD_ASIS, np->name);
> +	if (IS_ERR(led->cmd))
> +		return PTR_ERR(led->cmd);
> +
> +	led->slow = devm_gpiod_get_from_of_node(dev, np, "slow-gpio", 0,
> +						GPIOD_ASIS, np->name);
> +	if (IS_ERR(led->slow))
> +		return PTR_ERR(led->slow);
> +
> +	ret = of_property_count_u32_elems(np, "modes-map");
> +	if (ret <= 0 || ret % 3) {
> +		dev_err(dev, "Missing or malformed modes-map in node %pOF\n",
> +			np);
> +		return ret < 0 ? ret : -EINVAL;
> +	}
> +
> +	nmodes = ret / 3;
> +	modval = devm_kcalloc(dev, nmodes, sizeof(*modval), GFP_KERNEL);
> +	if (!modval)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nmodes; ++i) {
> +		u32 val;
> +
> +		of_property_read_u32_index(np, "modes-map", 3 * i, &val);
> +		modval[i].mode = val;
> +		of_property_read_u32_index(np, "modes-map", 3 * i + 1, &val);
> +		modval[i].cmd_level = val;
> +		of_property_read_u32_index(np, "modes-map", 3 * i + 2, &val);
> +		modval[i].slow_level = val;
> +	}
> +
> +	led->num_modes = nmodes;
> +	led->modval = modval;
>  
> -	rwlock_init(&led_dat->rw_lock);
> -
> -	led_dat->cdev.name = template->name;
> -	led_dat->cdev.default_trigger = template->default_trigger;
> -	led_dat->cdev.blink_set = NULL;
> -	led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
> -	led_dat->cdev.groups = ns2_led_groups;
> -	led_dat->cmd = template->cmd;
> -	led_dat->slow = template->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;
> +	rwlock_init(&led->rw_lock);
> +
> +	led->cdev.flags |= LED_CORE_SUSPENDRESUME;
> +	led->cdev.groups = ns2_led_groups;
> +	led->can_sleep = gpiod_cansleep(led->cmd) || gpiod_cansleep(led->slow);
> +	if (led->can_sleep)
> +		led->cdev.brightness_set_blocking = ns2_led_set_blocking;
>  	else
> -		led_dat->cdev.brightness_set = ns2_led_set;
> -	led_dat->modval = template->modval;
> -	led_dat->num_modes = template->num_modes;
> +		led->cdev.brightness_set = ns2_led_set;
>  
> -	ret = ns2_led_get_mode(led_dat, &mode);
> +	ret = ns2_led_get_mode(led, &mode);
>  	if (ret < 0)
>  		return ret;
>  
>  	/* Set LED initial state. */
> -	led_dat->sata = (mode == NS_V2_LED_SATA) ? 1 : 0;
> -	led_dat->cdev.brightness =
> -		(mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
> +	led->sata = (mode == NS_V2_LED_SATA) ? 1 : 0;
> +	led->cdev.brightness = (mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
>  
> -	ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
> -	if (ret < 0)
> -		return ret;
> +	init_data.fwnode = of_fwnode_handle(np);
>  
> -	return 0;
> -}
> +	ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
> +	if (ret < 0)
> +		dev_err(dev, "Failed to register LED for node %pOF\n", np);
>  
> -static void delete_ns2_led(struct ns2_led_data *led_dat)
> -{
> -	led_classdev_unregister(&led_dat->cdev);
> +	return ret;
>  }
>  
> -#ifdef CONFIG_OF_GPIO
> -/*
> - * Translate OpenFirmware node properties into platform_data.
> - */
> -static int
> -ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata)
> +static int ns2_led_probe(struct platform_device *pdev)
>  {
> -	struct device_node *np = dev->of_node;
> +	struct device *dev = &pdev->dev;
>  	struct device_node *child;
> -	struct ns2_led *led, *leds;
> -	int ret, num_leds = 0;
> +	struct ns2_led *leds;
> +	int count, ret;
>  
> -	num_leds = of_get_child_count(np);
> -	if (!num_leds)
> +	count = device_get_child_node_count(dev);
> +	if (count < 1) {
> +		dev_err(dev, "Device has no child nodes\n");
>  		return -ENODEV;
> +	}
>  
> -	leds = devm_kcalloc(dev, num_leds, sizeof(struct ns2_led),
> -			    GFP_KERNEL);
> +	leds = devm_kzalloc(dev, array_size(count, sizeof(*leds)), GFP_KERNEL);
>  	if (!leds)
>  		return -ENOMEM;
>  
> -	led = leds;
> -	for_each_child_of_node(np, child) {
> -		const char *string;
> -		int i, num_modes;
> -		struct ns2_led_modval *modval;
> -		struct gpio_desc *gd;
> -
> -		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)
> -			led->default_trigger = string;
> -
> -		ret = of_property_count_u32_elems(child, "modes-map");
> -		if (ret < 0 || ret % 3) {
> -			dev_err(dev,
> -				"Missing or malformed modes-map property\n");
> -			ret = -EINVAL;
> -			goto err_node_put;
> -		}
> -
> -		num_modes = ret / 3;
> -		modval = devm_kcalloc(dev,
> -				      num_modes,
> -				      sizeof(struct ns2_led_modval),
> -				      GFP_KERNEL);
> -		if (!modval) {
> -			ret = -ENOMEM;
> -			goto err_node_put;
> -		}
> -
> -		for (i = 0; i < num_modes; i++) {
> -			of_property_read_u32_index(child,
> -						"modes-map", 3 * i,
> -						(u32 *) &modval[i].mode);
> -			of_property_read_u32_index(child,
> -						"modes-map", 3 * i + 1,
> -						(u32 *) &modval[i].cmd_level);
> -			of_property_read_u32_index(child,
> -						"modes-map", 3 * i + 2,
> -						(u32 *) &modval[i].slow_level);
> -		}
> -
> -		led->num_modes = num_modes;
> -		led->modval = modval;
> -
> -		led++;
> +	for_each_available_child_of_node(dev_of_node(dev), child) {
> +		ret = ns2_led_register(dev, leds++, child);
> +		if (ret)
> +			return ret;
>  	}
>  
> -	pdata->leds = leds;
> -	pdata->num_leds = num_leds;
> -
>  	return 0;
> -
> -err_node_put:
> -	of_node_put(child);
> -	return ret;
>  }
>  
>  static const struct of_device_id of_ns2_leds_match[] = {
> @@ -341,73 +266,9 @@ static const struct of_device_id of_ns2_leds_match[] = {
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, of_ns2_leds_match);
> -#endif /* CONFIG_OF_GPIO */
> -
> -struct ns2_led_priv {
> -	int num_leds;
> -	struct ns2_led_data leds_data[];
> -};
> -
> -static int ns2_led_probe(struct platform_device *pdev)
> -{
> -	struct ns2_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> -	struct ns2_led_priv *priv;
> -	int i;
> -	int ret;
> -
> -#ifdef CONFIG_OF_GPIO
> -	if (!pdata) {
> -		pdata = devm_kzalloc(&pdev->dev,
> -				     sizeof(struct ns2_led_platform_data),
> -				     GFP_KERNEL);
> -		if (!pdata)
> -			return -ENOMEM;
> -
> -		ret = ns2_leds_get_of_pdata(&pdev->dev, pdata);
> -		if (ret)
> -			return ret;
> -	}
> -#else
> -	if (!pdata)
> -		return -EINVAL;
> -#endif /* CONFIG_OF_GPIO */
> -
> -	priv = devm_kzalloc(&pdev->dev, struct_size(priv, leds_data, pdata->num_leds), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> -	priv->num_leds = pdata->num_leds;
> -
> -	for (i = 0; i < priv->num_leds; i++) {
> -		ret = create_ns2_led(pdev, &priv->leds_data[i],
> -				     &pdata->leds[i]);
> -		if (ret < 0) {
> -			for (i = i - 1; i >= 0; i--)
> -				delete_ns2_led(&priv->leds_data[i]);
> -			return ret;
> -		}
> -	}
> -
> -	platform_set_drvdata(pdev, priv);
> -
> -	return 0;
> -}
> -
> -static int ns2_led_remove(struct platform_device *pdev)
> -{
> -	int i;
> -	struct ns2_led_priv *priv;
> -
> -	priv = platform_get_drvdata(pdev);
> -
> -	for (i = 0; i < priv->num_leds; i++)
> -		delete_ns2_led(&priv->leds_data[i]);
> -
> -	return 0;
> -}
>  
>  static struct platform_driver ns2_led_driver = {
>  	.probe		= ns2_led_probe,
> -	.remove		= ns2_led_remove,
>  	.driver		= {
>  		.name		= "leds-ns2",
>  		.of_match_table	= of_match_ptr(of_ns2_leds_match),
> -- 
> 2.26.2

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
  2020-09-18 13:02   ` Simon Guinot
@ 2020-09-18 17:14     ` Marek Behun
  2020-09-21 12:53       ` Simon Guinot
  0 siblings, 1 reply; 25+ messages in thread
From: Marek Behun @ 2020-09-18 17:14 UTC (permalink / raw)
  To: Simon Guinot
  Cc: linux-leds, Pavel Machek, Dan Murphy, Ondřej Jirman,
	linux-kernel, Rob Herring, devicetree, Simon Guinot,
	Vincent Donnefort, Thomas Petazzoni, Linus Walleij

On Fri, 18 Sep 2020 15:02:06 +0200
Simon Guinot <simon.guinot@sequanux.org> wrote:

> On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> 
> Hi Marek,
> 
> > By using struct led_init_data when registering we do not need to parse
> > `label` DT property nor `linux,default-trigger` property.
> > 
> > Also, move forward from platform data to device tree only:
> > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > platform data structure is absorbed into the driver, because nothing
> > else in the source tree used it. Since nobody complained and all usage  
> 
> Well, I probably should have...
> 
> I am using this driver on the Seagate Superbee NAS devices. This devices
> are based on a x86 SoC. Since I have been unable to get from the ODM the
> LED information written in the ACPI tables, then platform data are used
> to pass the LED description to the driver.
> 
> The support of this boards is not available mainline yet but it is still
> on my todo list. So that's why I am complaining right now :) If it is
> not too much trouble I'd like to keep platform data support in this
> driver.
> 
> Thanks in advance.
> 
> Simon
> 

Simon, what if we refactored the driver to use fwnode API instead of OF
API? Then if it is impossible for you to write DTS for that device,
instead of platform data you could implement your device via swnode
fwnodes. :)

static const struct property_entry entries[] = {
	PROPERTY_ENTRY_STRING("compatible", "lacie,ns2-leds"),
	...
};

Look at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/chromeos_laptop.c?h=v5.9-rc5
search for PROPERTY_ENTRY.

I am willing to work on this with you, but I would really like to rid
the LED drivers of platform data.

Marek

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
  2020-09-18 17:14     ` Marek Behun
@ 2020-09-21 12:53       ` Simon Guinot
  2020-09-21 13:02         ` Marek Behun
  0 siblings, 1 reply; 25+ messages in thread
From: Simon Guinot @ 2020-09-21 12:53 UTC (permalink / raw)
  To: Marek Behun
  Cc: linux-leds, Pavel Machek, Dan Murphy, Ondřej Jirman,
	linux-kernel, Rob Herring, devicetree, Simon Guinot,
	Vincent Donnefort, Thomas Petazzoni, Linus Walleij

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

On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> On Fri, 18 Sep 2020 15:02:06 +0200
> Simon Guinot <simon.guinot@sequanux.org> wrote:
> 
> > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > 
> > Hi Marek,
> > 
> > > By using struct led_init_data when registering we do not need to parse
> > > `label` DT property nor `linux,default-trigger` property.
> > > 
> > > Also, move forward from platform data to device tree only:
> > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > platform data structure is absorbed into the driver, because nothing
> > > else in the source tree used it. Since nobody complained and all usage  
> > 
> > Well, I probably should have...
> > 
> > I am using this driver on the Seagate Superbee NAS devices. This devices
> > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > LED information written in the ACPI tables, then platform data are used
> > to pass the LED description to the driver.
> > 
> > The support of this boards is not available mainline yet but it is still
> > on my todo list. So that's why I am complaining right now :) If it is
> > not too much trouble I'd like to keep platform data support in this
> > driver.
> > 
> > Thanks in advance.
> > 
> > Simon
> > 
> 
> Simon, what if we refactored the driver to use fwnode API instead of OF
> API? Then if it is impossible for you to write DTS for that device,
> instead of platform data you could implement your device via swnode
> fwnodes. :)

Yes. That would be perfect.

Simon

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
  2020-09-21 12:53       ` Simon Guinot
@ 2020-09-21 13:02         ` Marek Behun
  2020-09-21 14:03           ` Simon Guinot
  0 siblings, 1 reply; 25+ messages in thread
From: Marek Behun @ 2020-09-21 13:02 UTC (permalink / raw)
  To: Simon Guinot
  Cc: linux-leds, Pavel Machek, Dan Murphy, Ondřej Jirman,
	linux-kernel, Rob Herring, devicetree, Simon Guinot,
	Vincent Donnefort, Thomas Petazzoni, Linus Walleij

On Mon, 21 Sep 2020 14:53:43 +0200
Simon Guinot <simon.guinot@sequanux.org> wrote:

> On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> > On Fri, 18 Sep 2020 15:02:06 +0200
> > Simon Guinot <simon.guinot@sequanux.org> wrote:
> >   
> > > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > > 
> > > Hi Marek,
> > >   
> > > > By using struct led_init_data when registering we do not need to parse
> > > > `label` DT property nor `linux,default-trigger` property.
> > > > 
> > > > Also, move forward from platform data to device tree only:
> > > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > > platform data structure is absorbed into the driver, because nothing
> > > > else in the source tree used it. Since nobody complained and all usage    
> > > 
> > > Well, I probably should have...
> > > 
> > > I am using this driver on the Seagate Superbee NAS devices. This devices
> > > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > > LED information written in the ACPI tables, then platform data are used
> > > to pass the LED description to the driver.
> > > 
> > > The support of this boards is not available mainline yet but it is still
> > > on my todo list. So that's why I am complaining right now :) If it is
> > > not too much trouble I'd like to keep platform data support in this
> > > driver.
> > > 
> > > Thanks in advance.
> > > 
> > > Simon
> > >   
> > 
> > Simon, what if we refactored the driver to use fwnode API instead of OF
> > API? Then if it is impossible for you to write DTS for that device,
> > instead of platform data you could implement your device via swnode
> > fwnodes. :)  
> 
> Yes. That would be perfect.
> 
> Simon

BTW if you have access to device schematics I could try to write DTS,
with schematics and the current board source file it should not be that
hard. But I can't test it, since I don't have the board.

Marek

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
  2020-09-21 13:02         ` Marek Behun
@ 2020-09-21 14:03           ` Simon Guinot
  2020-09-21 14:31             ` Marek Behun
  0 siblings, 1 reply; 25+ messages in thread
From: Simon Guinot @ 2020-09-21 14:03 UTC (permalink / raw)
  To: Marek Behun
  Cc: linux-leds, Pavel Machek, Dan Murphy, Ondřej Jirman,
	linux-kernel, Rob Herring, devicetree, Simon Guinot,
	Vincent Donnefort, Thomas Petazzoni, Linus Walleij

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

On Mon, Sep 21, 2020 at 03:02:08PM +0200, Marek Behun wrote:
> On Mon, 21 Sep 2020 14:53:43 +0200
> Simon Guinot <simon.guinot@sequanux.org> wrote:
> 
> > On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:
> > > On Fri, 18 Sep 2020 15:02:06 +0200
> > > Simon Guinot <simon.guinot@sequanux.org> wrote:
> > >   
> > > > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > > > 
> > > > Hi Marek,
> > > >   
> > > > > By using struct led_init_data when registering we do not need to parse
> > > > > `label` DT property nor `linux,default-trigger` property.
> > > > > 
> > > > > Also, move forward from platform data to device tree only:
> > > > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > > > platform data structure is absorbed into the driver, because nothing
> > > > > else in the source tree used it. Since nobody complained and all usage    
> > > > 
> > > > Well, I probably should have...
> > > > 
> > > > I am using this driver on the Seagate Superbee NAS devices. This devices
> > > > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > > > LED information written in the ACPI tables, then platform data are used
> > > > to pass the LED description to the driver.
> > > > 
> > > > The support of this boards is not available mainline yet but it is still
> > > > on my todo list. So that's why I am complaining right now :) If it is
> > > > not too much trouble I'd like to keep platform data support in this
> > > > driver.
> > > > 
> > > > Thanks in advance.
> > > > 
> > > > Simon
> > > >   
> > > 
> > > Simon, what if we refactored the driver to use fwnode API instead of OF
> > > API? Then if it is impossible for you to write DTS for that device,
> > > instead of platform data you could implement your device via swnode
> > > fwnodes. :)  
> > 
> > Yes. That would be perfect.
> > 
> > Simon
> 
> BTW if you have access to device schematics I could try to write DTS,
> with schematics and the current board source file it should not be that
> hard. But I can't test it, since I don't have the board.

Don't worry, I'll do the writing and the testing of the fwnode in the
x86 board files. This boards are not mainlined yet. So it is my problem.

And actually if you don't have the time I can do the writing of the
fwnode support in the driver as well. And you can just let the driver
with the OF support. That's fine.

But if you are willing to add fwnode support to the driver yourself,
then you are more than welcome to do it. On my side, I can help with
the testing. I can check that the ARM boards ant their DTB are still
supported by the driver. And I can also check the support of the x86
boards with the addition of the fwnode properties.

Simon

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data
  2020-09-21 14:03           ` Simon Guinot
@ 2020-09-21 14:31             ` Marek Behun
  0 siblings, 0 replies; 25+ messages in thread
From: Marek Behun @ 2020-09-21 14:31 UTC (permalink / raw)
  To: Simon Guinot
  Cc: linux-leds, Pavel Machek, Dan Murphy, Ondřej Jirman,
	linux-kernel, Rob Herring, devicetree, Simon Guinot,
	Vincent Donnefort, Thomas Petazzoni, Linus Walleij

On Mon, 21 Sep 2020 16:03:22 +0200
Simon Guinot <simon.guinot@sequanux.org> wrote:

> On Mon, Sep 21, 2020 at 03:02:08PM +0200, Marek Behun wrote:
> > On Mon, 21 Sep 2020 14:53:43 +0200
> > Simon Guinot <simon.guinot@sequanux.org> wrote:
> >   
> > > On Fri, Sep 18, 2020 at 07:14:05PM +0200, Marek Behun wrote:  
> > > > On Fri, 18 Sep 2020 15:02:06 +0200
> > > > Simon Guinot <simon.guinot@sequanux.org> wrote:
> > > >     
> > > > > On Thu, Sep 17, 2020 at 01:16:50AM +0200, Marek Behún wrote:
> > > > > 
> > > > > Hi Marek,
> > > > >     
> > > > > > By using struct led_init_data when registering we do not need to parse
> > > > > > `label` DT property nor `linux,default-trigger` property.
> > > > > > 
> > > > > > Also, move forward from platform data to device tree only:
> > > > > > since commit c7896490dd1a ("leds: ns2: Absorb platform data") the
> > > > > > platform data structure is absorbed into the driver, because nothing
> > > > > > else in the source tree used it. Since nobody complained and all usage      
> > > > > 
> > > > > Well, I probably should have...
> > > > > 
> > > > > I am using this driver on the Seagate Superbee NAS devices. This devices
> > > > > are based on a x86 SoC. Since I have been unable to get from the ODM the
> > > > > LED information written in the ACPI tables, then platform data are used
> > > > > to pass the LED description to the driver.
> > > > > 
> > > > > The support of this boards is not available mainline yet but it is still
> > > > > on my todo list. So that's why I am complaining right now :) If it is
> > > > > not too much trouble I'd like to keep platform data support in this
> > > > > driver.
> > > > > 
> > > > > Thanks in advance.
> > > > > 
> > > > > Simon
> > > > >     
> > > > 
> > > > Simon, what if we refactored the driver to use fwnode API instead of OF
> > > > API? Then if it is impossible for you to write DTS for that device,
> > > > instead of platform data you could implement your device via swnode
> > > > fwnodes. :)    
> > > 
> > > Yes. That would be perfect.
> > > 
> > > Simon  
> > 
> > BTW if you have access to device schematics I could try to write DTS,
> > with schematics and the current board source file it should not be that
> > hard. But I can't test it, since I don't have the board.  
> 
> Don't worry, I'll do the writing and the testing of the fwnode in the
> x86 board files. This boards are not mainlined yet. So it is my problem.
> 
> And actually if you don't have the time I can do the writing of the
> fwnode support in the driver as well. And you can just let the driver
> with the OF support. That's fine.
> 
> But if you are willing to add fwnode support to the driver yourself,
> then you are more than welcome to do it. On my side, I can help with
> the testing. I can check that the ARM boards ant their DTB are still
> supported by the driver. And I can also check the support of the x86
> boards with the addition of the fwnode properties.
> 
> Simon

Very well, I shall convert the driver to fwnode API and send the patch.
Marek

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2020-09-21 14:31 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 23:16 [PATCH leds v1 00/10] Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) Marek Behún
2020-09-16 23:16 ` [PATCH leds v1 01/10] leds: parse linux,default-trigger DT property in LED core Marek Behún
2020-09-16 23:16 ` [PATCH leds v1 02/10] leds: bcm6328, bcm6358: use struct led_init_data when registering Marek Behún
2020-09-16 23:16 ` [PATCH leds v1 03/10] leds: lm3697: " Marek Behún
2020-09-17 11:39   ` Dan Murphy
2020-09-17 15:24     ` Marek Behun
2020-09-16 23:16 ` [PATCH leds v1 04/10] leds: max77650: " Marek Behún
2020-09-17 10:09   ` Bartosz Golaszewski
2020-09-16 23:16 ` [PATCH leds v1 05/10] leds: mt6323: " Marek Behún
2020-09-16 23:16 ` [PATCH leds v1 06/10] leds: pm8058: " Marek Behún
2020-09-17  0:46   ` Bjorn Andersson
2020-09-17 15:24     ` Marek Behun
2020-09-16 23:16 ` [PATCH leds v1 07/10] leds: is31fl32xx: " Marek Behún
2020-09-17 15:23   ` Marek Behun
2020-09-16 23:16 ` [PATCH leds v1 08/10] leds: is31fl319x: " Marek Behún
2020-09-16 23:16 ` [PATCH leds v1 09/10] leds: lm36274: " Marek Behún
2020-09-17 15:28   ` Dan Murphy
2020-09-17 15:54     ` Marek Behun
2020-09-16 23:16 ` [PATCH leds v1 10/10] leds: ns2: refactor and use struct led_init_data Marek Behún
2020-09-18 13:02   ` Simon Guinot
2020-09-18 17:14     ` Marek Behun
2020-09-21 12:53       ` Simon Guinot
2020-09-21 13:02         ` Marek Behun
2020-09-21 14:03           ` Simon Guinot
2020-09-21 14:31             ` Marek Behun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).