leds: add devicetree functionality to lltc,lt3593 driver
Subsystems (2)
v4 cover leds: add devicetree functionality to lltc,lt3593 driver
This is v4 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:
v3 → v4:
* Fixed a mishap that slipped in during rebase which caused a compile error
* Removed empty line at end of the DT documentaion 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 | 34 ++++ drivers/leds/leds-lt3593.c | 192 ++++++++++-------- 2 files changed, 142 insertions(+), 84 deletions(-) create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt
Signature · 1 line
2.17.1
PATCH v4 1/5 dt-bindings: leds: Add bindings for lltc,lt3593
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.
--- .../devicetree/bindings/leds/leds-lt3593.txt | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txt
Documentation/devicetree/bindings/leds/leds-lt3593.txt new file +34 -0
@@ -0,0 +1,34 @@ +Bindings for Linear Technologies LT3593 LED controller + +Required properties: +- compatible: Should be "lltc,lt3593". + +The hardware supports only one LED. The properties of this LED are +configured in a sub-node in the device node. + +Required sub-node properties: +- gpios: A handle to the GPIO that is connected to the 'CTRL' + pin of the chip. + +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"; + + led { + label = "white:backlight"; + gpios = <&gpio 0 GPIO_ACTIVE_HIGH>; + default-state = "on"; + }; +};
Signature · 1 line
2.17.1
Rob Herring
maintainerRe: [PATCH v4 1/5] dt-bindings: leds: Add bindings for lltc,lt3593
Quoted 18 lines · 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 <[email protected]>--- .../devicetree/bindings/leds/leds-lt3593.txt | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txtDocumentation/devicetree/bindings/leds/leds-lt3593.txt new file +1
-0@@ -0,0 +1,34 @@ +Bindings for Linear Technologies LT3593 LED controller
Documentation/devicetree/bindings/leds/leds-lt3593.txt line 11
Quoted 10 lines
Documentation/devicetree/bindings/leds/leds-lt3593.txt new file +10
-0+ +Required properties: +- compatible: Should be "lltc,lt3593". + +The hardware supports only one LED. The properties of this LED are +configured in a sub-node in the device node. + +Required sub-node properties: +- gpios: A handle to the GPIO that is connected to the 'CTRL' + pin of the chip.
lltc,ctrl-gpios
Or "shutdown-gpios" as that is a standard name and seems to be its function.
This also belongs in the parent node because it is a GPIO connection to the controller, not the LEDs.
Re: [PATCH v4 1/5] dt-bindings: leds: Add bindings for lltc,lt3593
Hi Rob,
Thanks for the review!
Quoted 32 lines · Rob Herring
Quoted 18 lines · 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 <[email protected]>--- .../devicetree/bindings/leds/leds-lt3593.txt | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lt3593.txtDocumentation/devicetree/bindings/leds/leds-lt3593.txt new file +1
-0@@ -0,0 +1,34 @@ +Bindings for Linear Technologies LT3593 LED controllerDocumentation/devicetree/bindings/leds/leds-lt3593.txt line 11
Quoted 10 lines
Documentation/devicetree/bindings/leds/leds-lt3593.txt new file +10
-0+ +Required properties: +- compatible: Should be "lltc,lt3593". + +The hardware supports only one LED. The properties of this LED are +configured in a sub-node in the device node. + +Required sub-node properties: +- gpios: A handle to the GPIO that is connected to the 'CTRL' + pin of the chip.lltc,ctrl-gpios
Quoted 2 lines
Or "shutdown-gpios" as that is a standard name and seems to be its function.
No, it's a control pin that receives multiple pulses to set the LED current (and by that, its brightness).
Quoted 2 lines
This also belongs in the parent node because it is a GPIO connection to the controller, not the LEDs.
Okay, not a problem. I'll change that and send a patch.
Thanks!
Daniel
PATCH v4 2/5 leds: lt3593: merge functions and clean up code
In reply to Daniel MackIn 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.
--- drivers/leds/leds-lt3593.c | 103 +++++++++++++------------------------ 1 file changed, 35 insertions(+), 68 deletions(-)
drivers/leds/leds-lt3593.c +35 -68
@@ -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" , },
Signature · 1 line
2.17.1
PATCH v4 3/5 leds: lt3593: switch to gpiod interface
In reply to Daniel MackClean up the code a bit and transition over to the gpiod based interface.
--- drivers/leds/leds-lt3593.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)
drivers/leds/leds-lt3593.c +12 -14
@@ -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);
Signature · 1 line
2.17.1
PATCH v4 4/5 leds: lt3593: Add device tree probing glue
In reply to Daniel MackThe binding details are described in an earlier commit that adds the documentation.
--- drivers/leds/leds-lt3593.c | 74 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)
drivers/leds/leds-lt3593.c +74 -0
@@ -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,93 @@ 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 ; + } + + 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->gpiod =devm_fwnode_get_gpiod_from_child (dev,NULL , child, + flags, + led_data->name); +if (IS_ERR (led_data->gpiod)) { +fwnode_handle_put (child); +return PTR_ERR (led_data->gpiod); + } + + 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), }, };
Signature · 1 line
2.17.1
PATCH v4 5/5 leds: lt3593: update email address and switch to SPDX license header
In reply to Daniel MackUpdate 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.
--- drivers/leds/leds-lt3593.c | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-)
drivers/leds/leds-lt3593.c +3 -18
@@ -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 <[email protected]> -* -* Based on leds-gpio.c, -* -* Copyright (C) 2007 8D Technologies inc. -* Raphael Assenat <[email protected]> -* 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 <[email protected]> #include <linux/kernel.h> #include <linux/platform_device.h>
@@ -215,7 +200,7 @@ static struct platform_driver lt3593_led_driver = {module_platform_driver (lt3593_led_driver); -MODULE_AUTHOR ("Daniel Mack <[email protected]>" ); +MODULE_AUTHOR ("Daniel Mack <[email protected]>" );MODULE_DESCRIPTION ("LED driver for LT3593 controllers" );MODULE_LICENSE ("GPL" );MODULE_ALIAS ("platform:leds-lt3593" );
Signature · 1 line
2.17.1
Re: [PATCH v4 5/5] leds: lt3593: update email address and switch to SPDX license header
Hi Daniel,
Thanks for sorting the things out.
Patch set applied to the for-next branch of linux-leds.git
I have one more remark while we are at it, please refer below.
Quoted 37 lines · 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.
Signed-off-by Daniel Mack <[email protected]>--- drivers/leds/leds-lt3593.c | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-)drivers/leds/leds-lt3593.c +2
-17@@ -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 <[email protected]> -* -* Based on leds-gpio.c, -* -* Copyright (C) 2007 8D Technologies inc. -* Raphael Assenat <[email protected]> -* 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 <[email protected]> #include <linux/kernel.h> #include <linux/platform_device.h>
drivers/leds/leds-lt3593.c static struct platform_driver lt3593_led_driver = { · line 205
Quoted 7 lines
drivers/leds/leds-lt3593.c +1
-1@@ -215,7 +200,7 @@ static struct platform_driver lt3593_led_driver = {module_platform_driver (lt3593_led_driver); -MODULE_AUTHOR ("Daniel Mack <[email protected]>" ); +MODULE_AUTHOR ("Daniel Mack <[email protected]>" );MODULE_DESCRIPTION ("LED driver for LT3593 controllers" );MODULE_LICENSE ("GPL" );
I've just noticed the pre-existing discrepancy between the license message (GPL v2) and MODULE_LICENSE("GPL"). Is there a reason to keep it that way?
Quoted 2 lines
drivers/leds/leds-lt3593.c +0
-0MODULE_ALIAS ("platform:leds-lt3593" );
Signature · 2 lines
Best regards, Jacek Anaszewski
Re: [PATCH v4 5/5] leds: lt3593: update email address and switch to SPDX license header
Quoted 8 lines · Jacek Anaszewski
drivers/leds/leds-lt3593.c static struct platform_driver lt3593_led_driver = { · line 205
Quoted 4 lines
drivers/leds/leds-lt3593.c +1
-1-MODULE_AUTHOR ("Daniel Mack <[email protected]>" ); +MODULE_AUTHOR ("Daniel Mack <[email protected]>" );MODULE_DESCRIPTION ("LED driver for LT3593 controllers" );MODULE_LICENSE ("GPL" );I've just noticed the pre-existing discrepancy between the license message (GPL v2) and
MODULE_LICENSE("GPL"). Is there a reason to keep it that way?
Ah, no, that's just a mistake. I'll send a patch to update the string.
Thanks for your reviews and all!
Daniel