| Message ID | 20200716182007.18389-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | Multicolor Framework v31 | expand |
Hi! > Introduce a multicolor class that groups colored LEDs > within a LED node. > > The multicolor class groups monochrome LEDs and allows controlling two > aspects of the final combined color: hue and lightness. The former is > controlled via the intensity file and the latter is controlled > via brightness file. > > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Signed-off-by: Dan Murphy <dmurphy@ti.com> Thanks, applied and pushed out. > +==================================== > +MultiColor LED handling under Linux > +==================================== ... > +Multicolor Class Control > +======================== AFAICT The first one should be "Multicolor" for consistency. > +config LEDS_CLASS_MULTICOLOR > + tristate "LED MultiColor Class Support" Here too. Can you send a followup patch to fix it up? Best regards, Pavel
Pavel On 7/20/20 4:54 AM, Pavel Machek wrote: > Hi! > >> Introduce a multicolor class that groups colored LEDs >> within a LED node. >> >> The multicolor class groups monochrome LEDs and allows controlling two >> aspects of the final combined color: hue and lightness. The former is >> controlled via the intensity file and the latter is controlled >> via brightness file. >> >> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > Thanks, applied and pushed out. Thank you. What about the patches for the users? >> +==================================== >> +MultiColor LED handling under Linux >> +==================================== > ... >> +Multicolor Class Control >> +======================== > AFAICT The first one should be "Multicolor" for consistency. > >> +config LEDS_CLASS_MULTICOLOR >> + tristate "LED MultiColor Class Support" > Here too. > > Can you send a followup patch to fix it up? Will send a patch to fix it up but not sure if I should send as part of this series or separately? Because I am not sure if you are going to apply the remaining patches up to the DTs Dan > Best regards, > Pavel
Hi! > The device has the ability to group LED output into control banks > so that multiple LED banks can be controlled with the same mixing and > brightness. Inversely the LEDs can also be controlled independently. Inversely? > --- /dev/null > +++ b/drivers/leds/leds-lp50xx.c > @@ -0,0 +1,784 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// TI LP50XX LED chip family driver > +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + Can we get https here and in the binding document? Please run this through checkpatch -- I believe it will have some comments. > + > + device_for_each_child_node(priv->dev, child) { > + led = &priv->leds[i]; > + ret = fwnode_property_count_u32(child, "reg"); > + if (ret < 0) { > + dev_err(&priv->client->dev, > + "reg property is invalid\n"); > + return -EINVAL; is handle_put(child) needed here? > + } > + if (ret > 1) { > + priv->num_of_banked_leds = ret; > + if (priv->num_of_banked_leds > > + priv->chip_info->max_modules) { > + dev_err(&priv->client->dev, > + "reg property is invalid\n"); > + ret = -EINVAL; > + fwnode_handle_put(child); > + goto child_out; > + } > + > + ret = fwnode_property_read_u32_array(child, > + "reg", > + led_banks, > + ret); Move this to subfunction to reduce the indentation? (Or, just refactor it somehow). > + if (ret) { > + dev_err(&priv->client->dev, > + "reg property is missing\n"); > + fwnode_handle_put(child); > + goto child_out; > + } Create label that does the handle_put so you don't need to repeat it quite so often? > + fwnode_for_each_child_node(child, led_node) { > + ret = fwnode_property_read_u32(led_node, "color", > + &color_id); > + if (ret) > + dev_err(priv->dev, "Cannot read color\n"); > + > + mc_led_info[num_colors].color_index = color_id; This uses undefined value. > + ret = lp50xx_reset(led); Does the GPIO need to be disabled before enabling it for reset? Best regards, Pavel
On Thu 2020-07-16 13:19:58, Dan Murphy wrote: > Introduce the LP5036/30/24/18/12/9 RGB LED driver. > The difference in these parts are the number of > LED outputs where the: > > LP5036 can control 36 LEDs > LP5030 can control 30 LEDs > LP5024 can control 24 LEDs > LP5018 can control 18 LEDs > LP5012 can control 12 LEDs > LP5009 can control 9 LEDs > > The device has the ability to group LED output into control banks > so that multiple LED banks can be controlled with the same mixing and > brightness. Inversely the LEDs can also be controlled independently. > > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Signed-off-by: Dan Murphy <dmurphy@ti.com> > +/* > + * struct lp50xx_chip_info - > + * @num_leds: number of LED outputs available on the device > + * @led_brightness0_reg: first brightness register of the device > + * @mix_out0_reg: first color mix register of the device > + * @bank_brt_reg: bank brightness register > + * @bank_mix_reg: color mix register > + * @reset_reg: device reset register > + */ Should have /** if this is kerneldoc. > + init_data.fwnode = child; > + num_colors = 0; > + > + /* There are only 3 LEDs per module otherwise they should be > + * banked which also is presented as 3 LEDs > + */ This is not usual comment style for kernel. (And add . at end of sentence). Best regards, Pavel
Hi! > Add multicolor framework support for the lp55xx family. > > Acked-by: Pavel Machek <pavel@ucw.cz> > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Signed-off-by: Dan Murphy <dmurphy@ti.com> Applied 4,5,6 and 8,9. > config LEDS_LP55XX_COMMON > tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" > - depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 > + depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR > + depends on OF > select FW_LOADER > select FW_LOADER_USER_HELPER But I have to ask: what does this do to userland interface once LEDS_CLASS_MULTICOLOR is enabled? Will users see some changes? Will they see some changes after dts parts are applied? Best regards, Pavel
Pavel On 7/21/20 4:05 PM, Pavel Machek wrote: > Hi! > >> The device has the ability to group LED output into control banks >> so that multiple LED banks can be controlled with the same mixing and >> brightness. Inversely the LEDs can also be controlled independently. > Inversely? I will revise it. > >> --- /dev/null >> +++ b/drivers/leds/leds-lp50xx.c >> @@ -0,0 +1,784 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// TI LP50XX LED chip family driver >> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ >> + > Can we get https here and in the binding document? > > Please run this through checkpatch -- I believe it will have some > comments. OK. > >> + >> + device_for_each_child_node(priv->dev, child) { >> + led = &priv->leds[i]; >> + ret = fwnode_property_count_u32(child, "reg"); >> + if (ret < 0) { >> + dev_err(&priv->client->dev, >> + "reg property is invalid\n"); >> + return -EINVAL; > is handle_put(child) needed here? It will be after I refactor the label >> + } >> + if (ret > 1) { >> + priv->num_of_banked_leds = ret; >> + if (priv->num_of_banked_leds > >> + priv->chip_info->max_modules) { >> + dev_err(&priv->client->dev, >> + "reg property is invalid\n"); >> + ret = -EINVAL; >> + fwnode_handle_put(child); >> + goto child_out; >> + } >> + >> + ret = fwnode_property_read_u32_array(child, >> + "reg", >> + led_banks, >> + ret); > Move this to subfunction to reduce the indentation? (Or, just refactor > it somehow). Actually I can just put it all on the same line since the 80 character requirement is relaxed. >> + if (ret) { >> + dev_err(&priv->client->dev, >> + "reg property is missing\n"); >> + fwnode_handle_put(child); >> + goto child_out; >> + } > Create label that does the handle_put so you don't need to repeat it > quite so often? I will rework it for all > >> + fwnode_for_each_child_node(child, led_node) { >> + ret = fwnode_property_read_u32(led_node, "color", >> + &color_id); >> + if (ret) >> + dev_err(priv->dev, "Cannot read color\n"); >> + >> + mc_led_info[num_colors].color_index = color_id; > This uses undefined value. OK needs to goto to out. > >> + ret = lp50xx_reset(led); > Does the GPIO need to be disabled before enabling it for reset? You mean toggle the GPIO? Yes it should be toggled I will update it. Dan > Best regards, > Pavel >
Pavel On 7/21/20 4:07 PM, Pavel Machek wrote: > On Thu 2020-07-16 13:19:58, Dan Murphy wrote: >> Introduce the LP5036/30/24/18/12/9 RGB LED driver. >> The difference in these parts are the number of >> LED outputs where the: >> >> LP5036 can control 36 LEDs >> LP5030 can control 30 LEDs >> LP5024 can control 24 LEDs >> LP5018 can control 18 LEDs >> LP5012 can control 12 LEDs >> LP5009 can control 9 LEDs >> >> The device has the ability to group LED output into control banks >> so that multiple LED banks can be controlled with the same mixing and >> brightness. Inversely the LEDs can also be controlled independently. >> >> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> +/* >> + * struct lp50xx_chip_info - >> + * @num_leds: number of LED outputs available on the device >> + * @led_brightness0_reg: first brightness register of the device >> + * @mix_out0_reg: first color mix register of the device >> + * @bank_brt_reg: bank brightness register >> + * @bank_mix_reg: color mix register >> + * @reset_reg: device reset register >> + */ > Should have /** if this is kerneldoc. > >> + init_data.fwnode = child; >> + num_colors = 0; >> + >> + /* There are only 3 LEDs per module otherwise they should be >> + * banked which also is presented as 3 LEDs >> + */ > This is not usual comment style for kernel. (And add . at end of > sentence). I will fix both. Dan > Best regards, > Pavel
Hi! > >>+ ret = fwnode_property_read_u32_array(child, > >>+ "reg", > >>+ led_banks, > >>+ ret); > >Move this to subfunction to reduce the indentation? (Or, just refactor > >it somehow). > > Actually I can just put it all on the same line since the 80 character > requirement is relaxed. No. You have too long and too complex function, with too many blocks inside each other. Please fix it. Pavel
Pavel On 7/22/20 2:10 AM, Pavel Machek wrote: > Hi! > >>>> + ret = fwnode_property_read_u32_array(child, >>>> + "reg", >>>> + led_banks, >>>> + ret); >>> Move this to subfunction to reduce the indentation? (Or, just refactor >>> it somehow). >> Actually I can just put it all on the same line since the 80 character >> requirement is relaxed. > No. > > You have too long and too complex function, with too many blocks > inside each other. Please fix it. I will refactor Dan
Hi! > > > > > + ret = fwnode_property_read_u32_array(child, > > > > > + "reg", > > > > > + led_banks, > > > > > + ret); > > > > Move this to subfunction to reduce the indentation? (Or, just refactor > > > > it somehow). > > > Actually I can just put it all on the same line since the 80 character > > > requirement is relaxed. > > No. > > > > You have too long and too complex function, with too many blocks > > inside each other. Please fix it. > > I will refactor Thank you. Pavel
Pavel On 7/21/20 4:11 PM, Pavel Machek wrote: > Hi! > >> Add multicolor framework support for the lp55xx family. >> >> Acked-by: Pavel Machek <pavel@ucw.cz> >> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > Applied 4,5,6 and 8,9. > >> config LEDS_LP55XX_COMMON >> tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" >> - depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 >> + depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR >> + depends on OF >> select FW_LOADER >> select FW_LOADER_USER_HELPER > But I have to ask: what does this do to userland interface once > LEDS_CLASS_MULTICOLOR is enabled? If the DT is instrumented with MC FW properties there will be a change to the user land interface. If the properties follow the LED properties then there should be no change to the userland interface. See the DT and user interface examples below. So the n900 should see no delta in the lighting since it has the LED properties and not the MC FW properties. I hope this answers your question. > Will users see some changes? Will they see some changes after dts > parts are applied? In my testing I did not see any delta in the user interface. I tested with both MC FW and non-MC FW properties on the LP5523 evm. So the legacy DT's should not be affected and should work as is. The DT patches were to make the DT's compliant with the updated bindings. Only the u8500 defconfig patch needs to be added or the u8500_defconfig will break in compilation so we need that patch as well. DTS changes not applied With multicolor framework properties in the DT ls beaglebone:green:heartbeat beaglebone:green:usr2 lp5523:channel1 beaglebone:green:mmc0 beaglebone:green:usr3 lp5523:channel5 chan5 { color = <LED_COLOR_ID_WHITE>; chan-name = "lp5523:channel5"; reg = <0x5>; led-cur = /bits/ 8 <50>; max-cur = /bits/ 8 <100>; }; multi-led@0 { #address-cells = <1>; #size-cells = <0>; reg = <0>; color = <LED_COLOR_ID_MULTI>; function = LED_FUNCTION_STANDBY; linux,default-trigger = "heartbeat"; led@0 { led-cur = /bits/ 8 <50>; max-cur = /bits/ 8 <100>; reg = <0x0>; color = <LED_COLOR_ID_GREEN>; }; led@1 { led-cur = /bits/ 8 <50>; max-cur = /bits/ 8 <100>; reg = <0x1>; color = <LED_COLOR_ID_BLUE>; }; led@6 { led-cur = /bits/ 8 <50>; max-cur = /bits/ 8 <100>; reg = <0x6>; color = <LED_COLOR_ID_RED>; }; }; lp5523:channel5# ls brightness device led_current max_brightness max_current power subsystem trigger uevent ls lp5523\:channel1 brightness device max_brightness multi_index multi_intensity power subsystem trigger uevent Without MC FW DT properties And as individual LEDs as the DTs are populated today. ls beaglebone:green:heartbeat beaglebone:green:usr2 lp5523:channel0 lp5523:channel5 beaglebone:green:mmc0 beaglebone:green:usr3 lp5523:channel2 lp5523:channel6 chan0 { color = <LED_COLOR_ID_GREEN>; chan-name = "lp5523:channel0"; reg = <0x0>; led-cur = /bits/ 8 <50>; max-cur = /bits/ 8 <100>; }; chan7 { color = <LED_COLOR_ID_RED>; chan-name = "lp5523:channel6"; reg = <0x6>; led-cur = /bits/ 8 <50>; max-cur = /bits/ 8 <100>; }; chan2 { color = <LED_COLOR_ID_BLUE>; chan-name = "lp5523:channel2"; reg = <0x1>; led-cur = /bits/ 8 <50>; max-cur = /bits/ 8 <100>; }; chan5 { color = <LED_COLOR_ID_WHITE>; chan-name = "lp5523:channel5"; reg = <0x5>; led-cur = /bits/ 8 <50>; max-cur = /bits/ 8 <100>; }; Dan
Hello This is the multi color LED framework. This framework presents clustered colored LEDs into an array and allows the user space to adjust the brightness of the cluster using a single file write. The individual colored LEDs intensities are controlled via a single file that is an array of LEDs Globally changed multi color->multicolor. Simplified adding a new line. Updated testing doc to reflect 5.9 kernel. Rebased on LEDs for-next branch. Dan Dan Murphy (12): leds: multicolor: Introduce a multicolor class definition dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers leds: lp50xx: Add the LP50XX family of the RGB LED driver dt-bindings: leds: Convert leds-lp55xx to yaml leds: lp55xx: Convert LED class registration to devm_* leds: lp55xx: Add multicolor framework support to lp55xx ARM: defconfig: u8500: Add LP55XX_COMMON config flag leds: lp5523: Update the lp5523 code to add multicolor brightness function leds: lp5521: Add multicolor framework multicolor brightness support ARM: dts: n900: Add reg property to the LP5523 channel node ARM: dts: imx6dl-yapp4: Add reg property to the lp5562 channel node ARM: dts: ste-href: Add reg property to the LP5521 channel nodes .../ABI/testing/sysfs-class-led-multicolor | 35 + .../devicetree/bindings/leds/leds-lp50xx.yaml | 130 +++ .../devicetree/bindings/leds/leds-lp55xx.txt | 228 ----- .../devicetree/bindings/leds/leds-lp55xx.yaml | 220 +++++ Documentation/leds/index.rst | 1 + Documentation/leds/leds-class-multicolor.rst | 86 ++ arch/arm/boot/dts/imx6dl-yapp4-common.dtsi | 14 +- arch/arm/boot/dts/omap3-n900.dts | 29 +- arch/arm/boot/dts/ste-href.dtsi | 22 +- arch/arm/configs/u8500_defconfig | 1 + drivers/leds/Kconfig | 32 +- drivers/leds/Makefile | 2 + drivers/leds/led-class-multicolor.c | 203 +++++ drivers/leds/leds-lp50xx.c | 784 ++++++++++++++++++ drivers/leds/leds-lp5521.c | 43 +- drivers/leds/leds-lp5523.c | 43 +- drivers/leds/leds-lp5562.c | 22 +- drivers/leds/leds-lp55xx-common.c | 190 ++++- drivers/leds/leds-lp55xx-common.h | 16 +- drivers/leds/leds-lp8501.c | 23 +- include/linux/led-class-multicolor.h | 121 +++ include/linux/platform_data/leds-lp55xx.h | 7 + 22 files changed, 1914 insertions(+), 338 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.yaml delete mode 100644 Documentation/devicetree/bindings/leds/leds-lp55xx.txt create mode 100644 Documentation/devicetree/bindings/leds/leds-lp55xx.yaml create mode 100644 Documentation/leds/leds-class-multicolor.rst create mode 100644 drivers/leds/led-class-multicolor.c create mode 100644 drivers/leds/leds-lp50xx.c create mode 100644 include/linux/led-class-multicolor.h