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

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

[Daniel Mack]

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 .../devicetree/bindings/leds/leds-lt3593.txt  | 25 +++++++++++++++++++
 1 file changed, 25 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..0a7035acda8f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
@@ -0,0 +1,25 @@
+Bindings for Linear Technologies LT3593 LED controller
+
+Required properties:
+- compatible : should be "lltc,lt3593".
+- label: A label for the LED.
+
+Optional properties:
+- linux,default-trigger :  (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- default-state:  (optional) The initial state of the LED.
+  see Documentation/devicetree/bindings/leds/common.txt
+
+The controller only suports one LED, hence the properties are all set
+in the root node of the device. If multiple chips of this kind are
+found in a design, each one needs to be handled by its own device node.
+
+Example:
+
+led-controller {
+	compatible = "lltc,lt3593";
+	label = "led-stripe";
+	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
+	linux,default-trigger = "heartbeat";
+};
+
-- 
2.17.1

[PATCH 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 a7ff510cbdd0..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_kzalloc(&pdev->dev,
-			sizeof(struct lt3593_led_data) * pdata->num_leds,
-			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 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 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 | 56 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c
index a096ee64cbbb..44b57e7059dd 100644
--- a/drivers/leds/leds-lt3593.c
+++ b/drivers/leds/leds-lt3593.c
@@ -24,6 +24,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 struct lt3593_led_data {
 	struct led_classdev cdev;
@@ -120,22 +121,77 @@ static int lt3593_led_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct lt3593_led_data *led_data;
+	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;
+
+	ret = of_property_read_string(dev->of_node, "label",
+				      &led_data->cdev.name);
+	if (ret < 0) {
+		dev_err(dev, "Unable to read label from child node\n");
+		return -EINVAL;
 	}
 
+	of_property_read_string(dev->of_node, "linux,default-trigger",
+				&led_data->cdev.default_trigger);
+
+	if (!of_property_read_string(dev->of_node, "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->gpiod = devm_gpiod_get(dev, NULL, flags);
+	if (IS_ERR(led_data->gpiod))
+		return PTR_ERR(led_data->gpiod);
+
+	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)
+		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 5/5] leds: lt3593: update email address

[Daniel Mack]

Update the email address in the module information and in the header.

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

diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c
index 44b57e7059dd..684880360a13 100644
--- a/drivers/leds/leds-lt3593.c
+++ b/drivers/leds/leds-lt3593.c
@@ -3,7 +3,7 @@
  *
  * See the datasheet at http://cds.linear.com/docs/Datasheet/3593f.pdf
  *
- * Copyright (c) 2009 Daniel Mack <daniel@caiaq.de>
+ * Copyright (c) 2009,2018 Daniel Mack <daniel@zonque.org>
  *
  * Based on leds-gpio.c,
  *
@@ -197,7 +197,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_ALIAS("platform:leds-lt3593");
-- 
2.17.1

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

[Jacek Anaszewski]

Hi Daniel,

Thank you for the patch.

On 06/17/2018 01:49 PM, Daniel Mack wrote:
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>   .../devicetree/bindings/leds/leds-lt3593.txt  | 25 +++++++++++++++++++
>   1 file changed, 25 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..0a7035acda8f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
> @@ -0,0 +1,25 @@
> +Bindings for Linear Technologies LT3593 LED controller
> +
> +Required properties:
> +- compatible : should be "lltc,lt3593".
> +- label: A label for the LED.
> +
> +Optional properties:
> +- linux,default-trigger :  (optional)
> +  see Documentation/devicetree/bindings/leds/common.txt
> +- default-state:  (optional) The initial state of the LED.
> +  see Documentation/devicetree/bindings/leds/common.txt
> +
> +The controller only suports one LED, hence the properties are all set
> +in the root node of the device. If multiple chips of this kind are
> +found in a design, each one needs to be handled by its own device node.

Please do not make an excuse for a single LED. Let's keep the bindings
consistent across all LED class drivers, i.e. each LED should have
a dedicated child node. Even there is a single output available.

Please compare how recently added LED bindings look like.
This also influences the way of LED class device name construction.
Please also refer to the recently added drivers for that, e.g.:
drivers/leds/leds-cr0014114.c.

> +Example:
> +
> +led-controller {
> +	compatible = "lltc,lt3593";
> +	label = "led-stripe";

Label should stick to the "color:function" pattern.

> +	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> +	linux,default-trigger = "heartbeat";
> +};
> +
> 

-- 
Best regards,
Jacek Anaszewski

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

[Daniel Mack]

Hi Jacek,

Thanks for the prompt reply!

On Sunday, June 17, 2018 09:39 PM, Jacek Anaszewski wrote:
> On 06/17/2018 01:49 PM, Daniel Mack wrote:
>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>> ---
>>    .../devicetree/bindings/leds/leds-lt3593.txt  | 25 +++++++++++++++++++
>>    1 file changed, 25 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..0a7035acda8f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-lt3593.txt
>> @@ -0,0 +1,25 @@
>> +Bindings for Linear Technologies LT3593 LED controller
>> +
>> +Required properties:
>> +- compatible : should be "lltc,lt3593".
>> +- label: A label for the LED.
>> +
>> +Optional properties:
>> +- linux,default-trigger :  (optional)
>> +  see Documentation/devicetree/bindings/leds/common.txt
>> +- default-state:  (optional) The initial state of the LED.
>> +  see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +The controller only suports one LED, hence the properties are all set
>> +in the root node of the device. If multiple chips of this kind are
>> +found in a design, each one needs to be handled by its own device node.
> 
> Please do not make an excuse for a single LED. Let's keep the bindings
> consistent across all LED class drivers, i.e. each LED should have
> a dedicated child node. Even there is a single output available.

Okay, I can do that. But you do agree with the decision to only support 
one single LED in the driver and the binding? The hardware chip can only 
drive one single LED, but of course, the driver could support multiple 
chips with a single device node and different sub-nodes. In fact the 
driver did support this via pdata before this series.

I believe, however, that this is against the principles of how 
device-trees are supposed to describe the hardware, so I changed it. And 
as I wrote in the commit log, the only mainline user only uses one LED 
anyway, so the change won't cause any regression.


Thanks,
Daniel

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

[Daniel Mack]

On Sunday, June 17, 2018 09:52 PM, Daniel Mack wrote:
> On Sunday, June 17, 2018 09:39 PM, Jacek Anaszewski wrote:

>> Please do not make an excuse for a single LED. Let's keep the bindings
>> consistent across all LED class drivers, i.e. each LED should have
>> a dedicated child node. Even there is a single output available.
> 
> Okay, I can do that. But you do agree with the decision to only support
> one single LED in the driver and the binding? The hardware chip can only
> drive one single LED, but of course, the driver could support multiple
> chips with a single device node and different sub-nodes. In fact the
> driver did support this via pdata before this series.
> 
> I believe, however, that this is against the principles of how
> device-trees are supposed to describe the hardware, so I changed it. And
> as I wrote in the commit log, the only mainline user only uses one LED
> anyway, so the change won't cause any regression.

And btw, that one user of the driver is soon to be replaced by a 
device-tree based version, and once that's finished, I'll send another 
patch to kill pdata handling from this driver entirely.

So the question is only how the DT bindings should look like, the pdata 
bits are just kept around for legacy compat and don't matter.


Thanks,
Daniel