| Message ID | 20200713154544.1683-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | Multicolor Framework v30 | expand |
On Mon, 13 Jul 2020 10:45:29 -0500 Dan Murphy <dmurphy@ti.com> wrote: > Fix the checkpatch warnings for the use of the file permission macros. > In converting the file permissions to the DEVICE_ATTR_XX macros the > call back function names needed to be updated within the code. > > This means that the lp55xx_ needed to be dropped in the name to keep > in harmony with the ABI documentation. > > Acked-by: Pavel Machek <pavel@ucw.cz> > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/leds-lp55xx-common.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > Reviewed-by: Marek Behún <marek.behun@nic.cz>
On Mon, 13 Jul 2020 10:45:30 -0500 Dan Murphy <dmurphy@ti.com> wrote: > Fix checkpatch errors and warnings for the LP5523.c device > driver. > > Acked-by: Pavel Machek <pavel@ucw.cz> > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/leds-lp5523.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) Reviewed-by: Marek Behún <marek.behun@nic.cz>
On Mon, 13 Jul 2020 10:45:32 -0500 Dan Murphy <dmurphy@ti.com> wrote: > Add a new color ID that is declared as MULTICOLOR as with the > multicolor framework declaring a definitive color is not accurate > as the node can contain multiple colors. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/led-core.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Marek Behún <marek.behun@nic.cz>
On Mon, 13 Jul 2020 10:45:33 -0500 Dan Murphy <dmurphy@ti.com> wrote: > Introduce a multicolor class that groups colored LEDs > within a LED node. > > The multi color 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> > --- > .../ABI/testing/sysfs-class-led-multicolor | 35 +++ > Documentation/leds/index.rst | 1 + > Documentation/leds/leds-class-multicolor.rst | 86 ++++++++ > drivers/leds/Kconfig | 10 + > drivers/leds/Makefile | 1 + > drivers/leds/led-class-multicolor.c | 204 > ++++++++++++++++++ include/linux/led-class-multicolor.h | > 121 +++++++++++ 7 files changed, 458 insertions(+) > create mode 100644 > Documentation/ABI/testing/sysfs-class-led-multicolor create mode > 100644 Documentation/leds/leds-class-multicolor.rst create mode > 100644 drivers/leds/led-class-multicolor.c create mode 100644 > include/linux/led-class-multicolor.h Reviewed-by: Marek Behún <marek.behun@nic.cz>
On Wed 2020-07-15 15:20:56, Marek Behún wrote: > On Mon, 13 Jul 2020 10:45:32 -0500 > Dan Murphy <dmurphy@ti.com> wrote: > > > Add a new color ID that is declared as MULTICOLOR as with the > > multicolor framework declaring a definitive color is not accurate > > as the node can contain multiple colors. > > > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > > --- > > drivers/leds/led-core.c | 1 + > > 1 file changed, 1 insertion(+) > > Reviewed-by: Marek Behún <marek.behun@nic.cz> Thanks for patches, thanks for reviews, 1-4 applied. Pavel
On Wed, 15 Jul 2020 19:36:10 +0200 Pavel Machek <pavel@ucw.cz> wrote: > Thanks for patches, thanks for reviews, 1-4 applied. > Pavel The most important one is 5th, though :D
Hi! First, let's substitute multi.color -> multicolor globally, LEDS_CLASS_MULTI_COLOR is most visible example of this. Please also decide whether it is MultiColor or multicolor, and make it consistent. > Introduce a multicolor class that groups colored LEDs > within a LED node. > > The multi color class groups monochrome LEDs and allows controlling two For example here. Plus, the LEDs are not neccessarily monochrome, we support white LEDs, too. Let's use "simple LEDs"? > 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. > + depends on LEDS_CLASS > + help > + This option enables the multicolor LED sysfs class in /sys/class/leds. > + It wraps LED class and adds multicolor LED specific sysfs attributes > + and kernel internal API to it. You'll need this to provide support > + for multicolor LEDs that are grouped together. This class is not > + intended for single color LEDs. It can be built as a module. "single color" -> "simple"? > + /* account for the new line at the end of the buffer */ > + offset++; > + if (offset < size) { > + ret = -EINVAL; > + goto err_out; > + } "new line" -> "newline", and actually check that character you are skipping is newline. Someone could put '%' in there... > + if (i < mcled_cdev->num_colors - 1) > + len += sprintf(buf + len, " "); > + len += sprintf(buf + len, "\n"); Using sprintf for single character has... quite a lot of overhead. Something like buf[len++] = '\n' would be simpler/shorter/better. Please fix all relevant places. Note I already applied patches 1-4. Best regards, Pavel
Pavel On 7/16/20 3:31 AM, Pavel Machek wrote: > Hi! > > First, let's substitute multi.color -> multicolor globally, > LEDS_CLASS_MULTI_COLOR is most visible example of this. Please also > decide whether it is MultiColor or multicolor, and make it consistent. Dictionary definition is multicolor no space. Capitalization is dependent on how it is use. Basically no capital in the middle of a sentence >> Introduce a multicolor class that groups colored LEDs >> within a LED node. >> >> The multi color class groups monochrome LEDs and allows controlling two > For example here. Plus, the LEDs are not neccessarily monochrome, we > support white LEDs, too. Let's use "simple LEDs"? OK > >> 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. >> + depends on LEDS_CLASS >> + help >> + This option enables the multicolor LED sysfs class in /sys/class/leds. >> + It wraps LED class and adds multicolor LED specific sysfs attributes >> + and kernel internal API to it. You'll need this to provide support >> + for multicolor LEDs that are grouped together. This class is not >> + intended for single color LEDs. It can be built as a module. > "single color" -> "simple"? ok > >> + /* account for the new line at the end of the buffer */ >> + offset++; >> + if (offset < size) { >> + ret = -EINVAL; >> + goto err_out; >> + } > "new line" -> "newline", and actually check that character you are > skipping is newline. Someone could put '%' in there... Actually we don't need to check for the character. Even if someone put the '%' there there will still be a '\n' at the end of the buffer. The for..loop above only processes the total number of available colors so effectively the '%' will be ignored just like the '\n'. If the buffer contains more entries then the number of colors an error will be returned via the check below since size will be greater then offset if (offset < size) { ret = -EINVAL; goto err_out; } Maybe I should remove the comment as it is a bit confusing. >> + if (i < mcled_cdev->num_colors - 1) >> + len += sprintf(buf + len, " "); >> + len += sprintf(buf + len, "\n"); > Using sprintf for single character has... quite a lot of > overhead. Something like buf[len++] = '\n' would be > simpler/shorter/better. Please fix all relevant places. OK > Note I already applied patches 1-4. I will rebase on top > Best regards, > Pavel
Pavel On 7/16/20 10:03 AM, Dan Murphy wrote: > Pavel > > On 7/16/20 3:31 AM, Pavel Machek wrote: >> Hi! >> >> First, let's substitute multi.color -> multicolor globally, >> LEDS_CLASS_MULTI_COLOR is most visible example of this. Please also >> decide whether it is MultiColor or multicolor, and make it consistent. > > Dictionary definition is multicolor no space. Capitalization is > dependent on how it is use. > > Basically no capital in the middle of a sentence > >>> Introduce a multicolor class that groups colored LEDs >>> within a LED node. >>> >>> The multi color class groups monochrome LEDs and allows controlling two >> For example here. Plus, the LEDs are not neccessarily monochrome, we >> support white LEDs, too. Let's use "simple LEDs"? > OK >> >>> 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. >>> + depends on LEDS_CLASS >>> + help >>> + This option enables the multicolor LED sysfs class in >>> /sys/class/leds. >>> + It wraps LED class and adds multicolor LED specific sysfs >>> attributes >>> + and kernel internal API to it. You'll need this to provide >>> support >>> + for multicolor LEDs that are grouped together. This class is not >>> + intended for single color LEDs. It can be built as a module. >> "single color" -> "simple"? > ok >> >>> + /* account for the new line at the end of the buffer */ >>> + offset++; >>> + if (offset < size) { >>> + ret = -EINVAL; >>> + goto err_out; >>> + } >> "new line" -> "newline", and actually check that character you are >> skipping is newline. Someone could put '%' in there... > > Actually we don't need to check for the character. Even if someone > put the '%' there there will still be a '\n' at the end of the buffer. > > The for..loop above only processes the total number of available > colors so effectively the '%' will be ignored just like the '\n'. > > If the buffer contains more entries then the number of colors an error > will be returned via the check below since size will be greater then > offset > > if (offset < size) { > ret = -EINVAL; > goto err_out; > } > > Maybe I should remove the comment as it is a bit confusing. > >>> + if (i < mcled_cdev->num_colors - 1) >>> + len += sprintf(buf + len, " "); >>> + len += sprintf(buf + len, "\n"); >> Using sprintf for single character has... quite a lot of >> overhead. Something like buf[len++] = '\n' would be >> simpler/shorter/better. Please fix all relevant places. > > OK > > >> Note I already applied patches 1-4. > > I will rebase on top > > One last change my legal team came back and said no to GPL v2+ so I reverted that change Dan >> Best regards, >> Pavel
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 Fixed documentation, removed adding a space when reading intensity and index, update LP50xx to store trigger directly into led_cdev added RB's by Rob H. Dan Dan Murphy (16): leds: lp55xx: Fix file permissions to use DEVICE_ATTR macros leds: lp5523: Fix various formatting issues in the code dt: bindings: Add multicolor class dt bindings documention leds: Add multicolor ID to the color ID list 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 + .../bindings/leds/leds-class-multicolor.yaml | 37 + .../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 | 204 +++++ drivers/leds/led-core.c | 1 + drivers/leds/leds-lp50xx.c | 784 ++++++++++++++++++ drivers/leds/leds-lp5521.c | 43 +- drivers/leds/leds-lp5523.c | 62 +- drivers/leds/leds-lp5562.c | 22 +- drivers/leds/leds-lp55xx-common.c | 212 +++-- drivers/leds/leds-lp55xx-common.h | 16 +- drivers/leds/leds-lp8501.c | 23 +- include/dt-bindings/leds/common.h | 3 +- include/linux/led-class-multicolor.h | 121 +++ include/linux/platform_data/leds-lp55xx.h | 7 + 25 files changed, 1975 insertions(+), 360 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml 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