[PATCH v5 0/5] leds: add devicetree functionality to lltc,lt3593 driver

[PATCH v5 0/5] leds: add devicetree functionality to lltc,lt3593 driver

[Daniel Mack]

This is v5 of the series that brings devicetree support for the
lltc,lt3593 LED driver.

Before this series, the driver supported controlling multiple LEDs
through an array in the platform data. IOW, a single instance of the
driver was able to control multiple hardware chips.

This series changes that, and requires a distinct platform device to
be set up for each of them, if a board has multiple of these hardware
chips. The reason is that in DT, nodes should represent hardware, and
it's much cleaner this way.

As stated earlier in the thread for v1, the driver currently only has
one user in mainline (the Raumfeld platform) which is soon to be
replaced by a devicetree file. This user only uses one LED via pdata, so
the change mentioned above does not cause a regression.

Once the platform is fully ported to DT, I'll send another patch that
removes pdata handling from this driver completely, but it's kept around
as legacy bridge for now.


Changelog:

v4 → v5:
* Moved GPIO property from LED subnode to device node, as requested
  by Rob Herring.
* Squashed the patch titled "sync module license information with
  header" in 5/5.

v3 → v4:

* Fixed a mishap that slipped in during rebase which caused a compile
  error
* Removed empty line at end of the DT documentation file
* Switched to SPDX license header in the driver

v2 → v3:

* Fixed a typo in the commit log of 1/5
* Rebased onto 4.18-rc1

v1 → v2:

* Moved LED-specific properties into a sub-node in DT, as requested by
  Jacek Anaszewski.


Daniel Mack (5):
  dt-bindings: leds: Add bindings for lltc,lt3593
  leds: lt3593: merge functions and clean up code
  leds: lt3593: switch to gpiod interface
  leds: lt3593: Add device tree probing glue
  leds: lt3593: update email address and switch to SPDX license header

 .../devicetree/bindings/leds/leds-lt3593.txt  |  32 +++
 drivers/leds/leds-lt3593.c                    | 190 ++++++++++--------
 2 files changed, 137 insertions(+), 85 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt

-- 
2.17.1

[PATCH v5 1/5] dt-bindings: leds: Add bindings for lltc,lt3593

[Daniel Mack]

This patch add the bindings document for LT3593 LED drivers.
The binding is kept consistent with other LED driver bindings in that it
stores all the LED-specific properties in its own subnode. As the hardware
only supports one channel, there can consequently only be one sub-node.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 .../devicetree/bindings/leds/leds-lt3593.txt  | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lt3593.txt b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
new file mode 100644
index 000000000000..6b2cabc36c0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
@@ -0,0 +1,32 @@
+Bindings for Linear Technologies LT3593 LED controller
+
+Required properties:
+- compatible:		Should be "lltc,lt3593".
+- lltc,ctrl-gpios:	A handle to the GPIO that is connected to the 'CTRL'
+			pin of the chip.
+
+The hardware supports only one LED. The properties of this LED are
+configured in a sub-node in the device node.
+
+Optional sub-node properties:
+- label:	A label for the LED. If none is given, the LED will be
+		named "lt3595::".
+- linux,default-trigger: The default trigger for the LED.
+			See Documentation/devicetree/bindings/leds/common.txt
+- default-state:	The initial state of the LED.
+			See Documentation/devicetree/bindings/leds/common.txt
+
+If multiple chips of this type are found in a design, each one needs to
+be handled by its own device node.
+
+Example:
+
+led-controller {
+	compatible = "lltc,lt3593";
+	lltc,ctrl-gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
+
+	led {
+		label = "white:backlight";
+		default-state = "on";
+	};
+};
-- 
2.17.1

Re: [PATCH v5 1/5] dt-bindings: leds: Add bindings for lltc,lt3593

[Rob Herring]

On Wed, Jun 27, 2018 at 08:52:33PM +0200, Daniel Mack wrote:
> This patch add the bindings document for LT3593 LED drivers.
> The binding is kept consistent with other LED driver bindings in that it
> stores all the LED-specific properties in its own subnode. As the hardware
> only supports one channel, there can consequently only be one sub-node.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  .../devicetree/bindings/leds/leds-lt3593.txt  | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v5 2/5] leds: lt3593: merge functions and clean up code

[Daniel Mack]

In preparation to DT probe functionality, merge create_lt3593_led() into
its only call-site. The DT based setup code will be quite different, so this
internal helper function is of no help.

This also changes the way the driver works by only handling one entry inside
'struct gpio_led_platform_data'. If multiple devices of the same type are
found in a design, there should be a platform device for each of them. The
only mainline user of this driver is not affected by this change.

Last, use devm_led_classdev_register() instead of
led_classdev_register(), so the driver's remove callback can go away.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/leds/leds-lt3593.c | 103 +++++++++++++------------------------
 1 file changed, 35 insertions(+), 68 deletions(-)

diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c
index 5ec730a31b65..1a3dc347db21 100644
--- a/drivers/leds/leds-lt3593.c
+++ b/drivers/leds/leds-lt3593.c
@@ -71,104 +71,71 @@ static int lt3593_led_set(struct led_classdev *led_cdev,
 	return 0;
 }
 
-static int create_lt3593_led(const struct gpio_led *template,
-	struct lt3593_led_data *led_dat, struct device *parent)
+static struct lt3593_led_data *lt3593_led_probe_pdata(struct device *dev)
 {
+	struct gpio_led_platform_data *pdata = dev_get_platdata(dev);
+	const struct gpio_led *template = &pdata->leds[0];
+	struct lt3593_led_data *led_data;
 	int ret, state;
 
-	/* skip leds on GPIOs that aren't available */
+	if (pdata->num_leds != 1)
+		return ERR_PTR(-EINVAL);
+
+	led_data = devm_kzalloc(dev, sizeof(*led_data), GFP_KERNEL);
+	if (!led_data)
+		return ERR_PTR(-ENOMEM);
+
 	if (!gpio_is_valid(template->gpio)) {
-		dev_info(parent, "%s: skipping unavailable LT3593 LED at gpio %d (%s)\n",
-				KBUILD_MODNAME, template->gpio, template->name);
-		return 0;
+		dev_info(dev, "skipping unavailable LT3593 LED at gpio "
+			 "%d (%s)\n", template->gpio, template->name);
+		return ERR_PTR(-EINVAL);
 	}
 
-	led_dat->cdev.name = template->name;
-	led_dat->cdev.default_trigger = template->default_trigger;
-	led_dat->gpio = template->gpio;
-
-	led_dat->cdev.brightness_set_blocking = lt3593_led_set;
+	led_data->cdev.name = template->name;
+	led_data->cdev.default_trigger = template->default_trigger;
+	led_data->gpio = template->gpio;
+	led_data->cdev.brightness_set_blocking = lt3593_led_set;
 
 	state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
-	led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
+	led_data->cdev.brightness = state ? LED_FULL : LED_OFF;
 
 	if (!template->retain_state_suspended)
-		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
+		led_data->cdev.flags |= LED_CORE_SUSPENDRESUME;
 
-	ret = devm_gpio_request_one(parent, template->gpio, state ?
+	ret = devm_gpio_request_one(dev, template->gpio, state ?
 				    GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
 				    template->name);
 	if (ret < 0)
-		return ret;
+		return ERR_PTR(ret);
 
-	ret = led_classdev_register(parent, &led_dat->cdev);
+	ret = devm_led_classdev_register(dev, &led_data->cdev);
 	if (ret < 0)
-		return ret;
-
-	dev_info(parent, "%s: registered LT3593 LED '%s' at GPIO %d\n",
-		KBUILD_MODNAME, template->name, template->gpio);
+		return ERR_PTR(ret);
 
-	return 0;
-}
+	dev_info(dev, "registered LT3593 LED '%s' at GPIO %d\n",
+		 template->name, template->gpio);
 
-static void delete_lt3593_led(struct lt3593_led_data *led)
-{
-	if (!gpio_is_valid(led->gpio))
-		return;
-
-	led_classdev_unregister(&led->cdev);
+	return led_data;
 }
 
 static int lt3593_led_probe(struct platform_device *pdev)
 {
-	struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct lt3593_led_data *leds_data;
-	int i, ret = 0;
-
-	if (!pdata)
-		return -EBUSY;
-
-	leds_data = devm_kcalloc(&pdev->dev,
-			pdata->num_leds, sizeof(struct lt3593_led_data),
-			GFP_KERNEL);
-	if (!leds_data)
-		return -ENOMEM;
-
-	for (i = 0; i < pdata->num_leds; i++) {
-		ret = create_lt3593_led(&pdata->leds[i], &leds_data[i],
-				      &pdev->dev);
-		if (ret < 0)
-			goto err;
-	}
+	struct device *dev = &pdev->dev;
+	struct lt3593_led_data *led_data;
 
-	platform_set_drvdata(pdev, leds_data);
-
-	return 0;
-
-err:
-	for (i = i - 1; i >= 0; i--)
-		delete_lt3593_led(&leds_data[i]);
-
-	return ret;
-}
-
-static int lt3593_led_remove(struct platform_device *pdev)
-{
-	int i;
-	struct gpio_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct lt3593_led_data *leds_data;
-
-	leds_data = platform_get_drvdata(pdev);
+	if (dev_get_platdata(dev)) {
+		led_data = lt3593_led_probe_pdata(dev);
+		if (IS_ERR(led_data))
+			return PTR_ERR(led_data);
+	}
 
-	for (i = 0; i < pdata->num_leds; i++)
-		delete_lt3593_led(&leds_data[i]);
+	platform_set_drvdata(pdev, led_data);
 
 	return 0;
 }
 
 static struct platform_driver lt3593_led_driver = {
 	.probe		= lt3593_led_probe,
-	.remove		= lt3593_led_remove,
 	.driver		= {
 		.name	= "leds-lt3593",
 	},
-- 
2.17.1

[PATCH v5 3/5] leds: lt3593: switch to gpiod interface

[Daniel Mack]

Clean up the code a bit and transition over to the gpiod based interface.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/leds/leds-lt3593.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c
index 1a3dc347db21..a096ee64cbbb 100644
--- a/drivers/leds/leds-lt3593.c
+++ b/drivers/leds/leds-lt3593.c
@@ -21,12 +21,13 @@
 #include <linux/leds.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 
 struct lt3593_led_data {
 	struct led_classdev cdev;
-	unsigned gpio;
+	struct gpio_desc *gpiod;
 };
 
 static int lt3593_led_set(struct led_classdev *led_cdev,
@@ -46,25 +47,25 @@ static int lt3593_led_set(struct led_classdev *led_cdev,
 	 */
 
 	if (value == 0) {
-		gpio_set_value_cansleep(led_dat->gpio, 0);
+		gpiod_set_value_cansleep(led_dat->gpiod, 0);
 		return 0;
 	}
 
 	pulses = 32 - (value * 32) / 255;
 
 	if (pulses == 0) {
-		gpio_set_value_cansleep(led_dat->gpio, 0);
+		gpiod_set_value_cansleep(led_dat->gpiod, 0);
 		mdelay(1);
-		gpio_set_value_cansleep(led_dat->gpio, 1);
+		gpiod_set_value_cansleep(led_dat->gpiod, 1);
 		return 0;
 	}
 
-	gpio_set_value_cansleep(led_dat->gpio, 1);
+	gpiod_set_value_cansleep(led_dat->gpiod, 1);
 
 	while (pulses--) {
-		gpio_set_value_cansleep(led_dat->gpio, 0);
+		gpiod_set_value_cansleep(led_dat->gpiod, 0);
 		udelay(1);
-		gpio_set_value_cansleep(led_dat->gpio, 1);
+		gpiod_set_value_cansleep(led_dat->gpiod, 1);
 		udelay(1);
 	}
 
@@ -85,15 +86,8 @@ static struct lt3593_led_data *lt3593_led_probe_pdata(struct device *dev)
 	if (!led_data)
 		return ERR_PTR(-ENOMEM);
 
-	if (!gpio_is_valid(template->gpio)) {
-		dev_info(dev, "skipping unavailable LT3593 LED at gpio "
-			 "%d (%s)\n", template->gpio, template->name);
-		return ERR_PTR(-EINVAL);
-	}
-
 	led_data->cdev.name = template->name;
 	led_data->cdev.default_trigger = template->default_trigger;
-	led_data->gpio = template->gpio;
 	led_data->cdev.brightness_set_blocking = lt3593_led_set;
 
 	state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
@@ -108,6 +102,10 @@ static struct lt3593_led_data *lt3593_led_probe_pdata(struct device *dev)
 	if (ret < 0)
 		return ERR_PTR(ret);
 
+	led_data->gpiod = gpio_to_desc(template->gpio);
+	if (!led_data->gpiod)
+		return ERR_PTR(-EPROBE_DEFER);
+
 	ret = devm_led_classdev_register(dev, &led_data->cdev);
 	if (ret < 0)
 		return ERR_PTR(ret);
-- 
2.17.1

[PATCH v5 4/5] leds: lt3593: Add device tree probing glue

[Daniel Mack]

The binding details are described in an earlier commit that adds the
documentation.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/leds/leds-lt3593.c | 70 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c
index a096ee64cbbb..2e35a4a4c571 100644
--- a/drivers/leds/leds-lt3593.c
+++ b/drivers/leds/leds-lt3593.c
@@ -24,8 +24,11 @@
 #include <linux/gpio/consumer.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <uapi/linux/uleds.h>
 
 struct lt3593_led_data {
+	char name[LED_MAX_NAME_SIZE];
 	struct led_classdev cdev;
 	struct gpio_desc *gpiod;
 };
@@ -120,22 +123,89 @@ static int lt3593_led_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct lt3593_led_data *led_data;
+	struct fwnode_handle *child;
+	int ret, state = LEDS_GPIO_DEFSTATE_OFF;
+	enum gpiod_flags flags = GPIOD_OUT_LOW;
+	const char *tmp;
 
 	if (dev_get_platdata(dev)) {
 		led_data = lt3593_led_probe_pdata(dev);
 		if (IS_ERR(led_data))
 			return PTR_ERR(led_data);
+
+		goto out;
 	}
 
+	if (!dev->of_node)
+		return -ENODEV;
+
+	led_data = devm_kzalloc(dev, sizeof(*led_data), GFP_KERNEL);
+	if (!led_data)
+		return -ENOMEM;
+
+	if (device_get_child_node_count(dev) != 1) {
+		dev_err(dev, "Device must have exactly one LED sub-node.");
+		return -EINVAL;
+	}
+
+	led_data->gpiod = devm_gpiod_get(dev, "lltc,ctrl", 0);
+	if (IS_ERR(led_data->gpiod))
+		return PTR_ERR(led_data->gpiod);
+
+	child = device_get_next_child_node(dev, NULL);
+
+	ret = fwnode_property_read_string(child, "label", &tmp);
+	if (ret < 0)
+		snprintf(led_data->name, sizeof(led_data->name),
+			 "lt3593::");
+	else
+		snprintf(led_data->name, sizeof(led_data->name),
+			 "lt3593:%s", tmp);
+
+	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, "keep")) {
+			state = LEDS_GPIO_DEFSTATE_KEEP;
+			flags = GPIOD_ASIS;
+		} else if (!strcmp(tmp, "on")) {
+			state = LEDS_GPIO_DEFSTATE_ON;
+			flags = GPIOD_OUT_HIGH;
+		}
+	}
+
+	led_data->cdev.name = led_data->name;
+	led_data->cdev.brightness_set_blocking = lt3593_led_set;
+	led_data->cdev.brightness = state ? LED_FULL : LED_OFF;
+
+	ret = devm_led_classdev_register(dev, &led_data->cdev);
+	if (ret < 0) {
+		fwnode_handle_put(child);
+		return ret;
+	}
+
+	led_data->cdev.dev->of_node = dev->of_node;
+
+out:
 	platform_set_drvdata(pdev, led_data);
 
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id of_lt3593_leds_match[] = {
+	{ .compatible = "lltc,lt3593", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_lt3593_leds_match);
+#endif
+
 static struct platform_driver lt3593_led_driver = {
 	.probe		= lt3593_led_probe,
 	.driver		= {
 		.name	= "leds-lt3593",
+		.of_match_table = of_match_ptr(of_lt3593_leds_match),
 	},
 };
 
-- 
2.17.1

[PATCH v5 5/5] leds: lt3593: update email address and switch to SPDX license header

[Daniel Mack]

Update the email address in the module information and in the comments
and in the module information, and drop the existing boilerplate in
favor of a SPDX header.

Also sync the module license information with the header.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/leds/leds-lt3593.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c
index 2e35a4a4c571..de3623e0d094 100644
--- a/drivers/leds/leds-lt3593.c
+++ b/drivers/leds/leds-lt3593.c
@@ -1,20 +1,5 @@
-/*
- * LEDs driver for LT3593 controllers
- *
- * See the datasheet at http://cds.linear.com/docs/Datasheet/3593f.pdf
- *
- * Copyright (c) 2009 Daniel Mack <daniel@caiaq.de>
- *
- * Based on leds-gpio.c,
- *
- *   Copyright (C) 2007 8D Technologies inc.
- *   Raphael Assenat <raph@8d.com>
- *   Copyright (C) 2008 Freescale Semiconductor, Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2009,2018 Daniel Mack <daniel@zonque.org>
 
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
@@ -211,7 +196,7 @@ static struct platform_driver lt3593_led_driver = {
 
 module_platform_driver(lt3593_led_driver);
 
-MODULE_AUTHOR("Daniel Mack <daniel@caiaq.de>");
+MODULE_AUTHOR("Daniel Mack <daniel@zonque.org>");
 MODULE_DESCRIPTION("LED driver for LT3593 controllers");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("platform:leds-lt3593");
-- 
2.17.1

Re: [PATCH v5 0/5] leds: add devicetree functionality to lltc,lt3593 driver

[Jacek Anaszewski]

Hi Daniel,

Thank you for the updated set.

Updated for-next branch of linux-leds.git accordingly.

Best regards,
Jacek Anaszewski

On 06/27/2018 08:52 PM, Daniel Mack wrote:
> This is v5 of the series that brings devicetree support for the
> lltc,lt3593 LED driver.
> 
> Before this series, the driver supported controlling multiple LEDs
> through an array in the platform data. IOW, a single instance of the
> driver was able to control multiple hardware chips.
> 
> This series changes that, and requires a distinct platform device to
> be set up for each of them, if a board has multiple of these hardware
> chips. The reason is that in DT, nodes should represent hardware, and
> it's much cleaner this way.
> 
> As stated earlier in the thread for v1, the driver currently only has
> one user in mainline (the Raumfeld platform) which is soon to be
> replaced by a devicetree file. This user only uses one LED via pdata, so
> the change mentioned above does not cause a regression.
> 
> Once the platform is fully ported to DT, I'll send another patch that
> removes pdata handling from this driver completely, but it's kept around
> as legacy bridge for now.
> 
> 
> Changelog:
> 
> v4 → v5:
> * Moved GPIO property from LED subnode to device node, as requested
>    by Rob Herring.
> * Squashed the patch titled "sync module license information with
>    header" in 5/5.
> 
> v3 → v4:
> 
> * Fixed a mishap that slipped in during rebase which caused a compile
>    error
> * Removed empty line at end of the DT documentation file
> * Switched to SPDX license header in the driver
> 
> v2 → v3:
> 
> * Fixed a typo in the commit log of 1/5
> * Rebased onto 4.18-rc1
> 
> v1 → v2:
> 
> * Moved LED-specific properties into a sub-node in DT, as requested by
>    Jacek Anaszewski.
> 
> 
> Daniel Mack (5):
>    dt-bindings: leds: Add bindings for lltc,lt3593
>    leds: lt3593: merge functions and clean up code
>    leds: lt3593: switch to gpiod interface
>    leds: lt3593: Add device tree probing glue
>    leds: lt3593: update email address and switch to SPDX license header
> 
>   .../devicetree/bindings/leds/leds-lt3593.txt  |  32 +++
>   drivers/leds/leds-lt3593.c                    | 190 ++++++++++--------
>   2 files changed, 137 insertions(+), 85 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt
>