2020-05-03 12:43:15

by Dan Murphy

[permalink] [raw]
Subject: [PATCH v24 02/16] leds: multicolor: Introduce a multicolor class definition

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 <[email protected]>
Signed-off-by: Dan Murphy <[email protected]>
---
.../ABI/testing/sysfs-class-led-multicolor | 34 +++
Documentation/leds/index.rst | 1 +
Documentation/leds/leds-class-multicolor.rst | 86 +++++++
MAINTAINERS | 8 +
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/led-class-multicolor.c | 210 ++++++++++++++++++
include/linux/led-class-multicolor.h | 121 ++++++++++
8 files changed, 472 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

diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
new file mode 100644
index 000000000000..7d33a82a4b07
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
@@ -0,0 +1,34 @@
+What: /sys/class/leds/<led>/brightness
+Date: March 2020
+KernelVersion: 5.8
+Contact: Dan Murphy <[email protected]>
+Description: read/write
+ Writing to this file will update all LEDs within the group to a
+ calculated percentage of what each color LED intensity is set
+ to. The percentage is calculated for each grouped LED via the
+ equation below:
+
+ led_brightness = brightness * multi_intensity/max_brightness
+
+ For additional details please refer to
+ Documentation/leds/leds-class-multicolor.rst.
+
+ The value of the color is from 0 to
+ /sys/class/leds/<led>/max_brightness.
+
+What: /sys/class/leds/<led>/multi_index
+Date: March 2020
+KernelVersion: 5.8
+Contact: Dan Murphy <[email protected]>
+Description: read
+ The multi_index array, when read, will output the LED colors
+ by name as they are indexed in the multi_intensity file.
+
+What: /sys/class/leds/<led>/multi_intensity
+Date: March 2020
+KernelVersion: 5.8
+Contact: Dan Murphy <[email protected]>
+Description: read/write
+ Intensity level for the LED color within the array.
+ The intensities for each color must be entered based on the
+ multi_index array.
diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
index 060f4e485897..bc70c6aa7138 100644
--- a/Documentation/leds/index.rst
+++ b/Documentation/leds/index.rst
@@ -9,6 +9,7 @@ LEDs

leds-class
leds-class-flash
+ leds-class-multicolor
ledtrig-oneshot
ledtrig-transient
ledtrig-usbport
diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst
new file mode 100644
index 000000000000..8cfbe2e44021
--- /dev/null
+++ b/Documentation/leds/leds-class-multicolor.rst
@@ -0,0 +1,86 @@
+====================================
+MultiColor LED handling under Linux
+====================================
+
+Description
+===========
+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 multi_intensity array file and the latter is controlled
+via brightness file.
+
+Multicolor Class Control
+========================
+The multicolor class presents files that groups the colors as indexes in an
+array. These files are children under the LED parent node created by the
+led_class framework. The led_class framework is documented in led-class.rst
+within this documentation directory.
+
+Each colored LED will be indexed under the multi_* files. The order of the
+colors will be arbitrary. The multi_index file can be read to determine the
+color name to indexed value.
+
+The multi_index file is an array that contains the string list of the colors as
+they are defined in each multi_* array file.
+
+The multi_intensity is an array that can be read or written to for the
+individual color intensities. All elements within this array must be written in
+order for the color LED intensities to be updated.
+
+Directory Layout Example
+========================
+root:/sys/class/leds/multicolor:status# ls -lR
+-rw-r--r-- 1 root root 4096 Oct 19 16:16 brightness
+-r--r--r-- 1 root root 4096 Oct 19 16:16 max_brightness
+-r--r--r-- 1 root root 4096 Oct 19 16:16 multi_index
+-rw-r--r-- 1 root root 4096 Oct 19 16:16 multi_intensity
+
+Multicolor Class Brightness Control
+===================================
+The multicolor class framework will calculate each monochrome LEDs intensity.
+
+The brightness level for each LED is calculated based on the color LED
+intensity setting divided by the global max_brightness setting multiplied by
+the requested brightness.
+
+led_brightness = brightness * multi_intensity/max_brightness
+
+Example:
+A user first writes the multi_intensity file with the brightness levels
+for each LED that are necessary to achieve a certain color output from a
+multicolor LED group.
+
+cat /sys/class/leds/multicolor:status/multi_index
+green blue red
+
+echo 43 226 138 > /sys/class/leds/multicolor:status/multi_intensity
+
+red -
+ intensity = 138
+ max_brightness = 255
+green -
+ intensity = 43
+ max_brightness = 255
+blue -
+ intensity = 226
+ max_brightness = 255
+
+The user can control the brightness of that multicolor LED group by writing the
+global 'brightness' control. Assuming a max_brightness of 255 the user
+may want to dim the LED color group to half. The user would write a value of
+128 to the global brightness file then the values written to each LED will be
+adjusted base on this value.
+
+cat /sys/class/leds/multicolor:status/max_brightness
+255
+echo 128 > /sys/class/leds/multicolor:status/brightness
+
+adjusted_red_value = 128 * 138/255 = 69
+adjusted_green_value = 128 * 43/255 = 21
+adjusted_blue_value = 128 * 226/255 = 113
+
+Reading the global brightness file will return the current brightness value of
+the color LED group.
+
+cat /sys/class/leds/multicolor:status/brightness
+128
diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db31497..e5f0c3c17bea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9533,6 +9533,14 @@ F: Documentation/devicetree/bindings/leds/
F: drivers/leds/
F: include/linux/leds.h

+LED MULTICOLOR FRAMEWORK
+M: Dan Murphy <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/leds/leds-class-multicolor.rst
+F: drivers/leds/led-class-multicolor.c
+F: include/linux/led-class-multicolor.h
+
LEGACY EEPROM DRIVER
M: Jean Delvare <[email protected]>
S: Maintained
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9cdc4cfc5d11..5b43f4957b8a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -30,6 +30,17 @@ config LEDS_CLASS_FLASH
for the flash related features of a LED device. It can be built
as a module.

+config LEDS_CLASS_MULTI_COLOR
+ tristate "LED MultiColor LED Class Support"
+ depends on LEDS_CLASS
+ depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
+ 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.
+
config LEDS_BRIGHTNESS_HW_CHANGED
bool "LED Class brightness_hw_changed attribute support"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d0dff504f108..53bf92f30258 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -4,6 +4,7 @@
obj-$(CONFIG_NEW_LEDS) += led-core.o
obj-$(CONFIG_LEDS_CLASS) += led-class.o
obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o
+obj-$(CONFIG_LEDS_CLASS_MULTI_COLOR) += led-class-multicolor.o
obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o

# LED Platform Drivers (keep this sorted, M-| sort)
diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
new file mode 100644
index 000000000000..7badefb66f62
--- /dev/null
+++ b/drivers/leds/led-class-multicolor.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0
+// LED Multi Color class interface
+// Copyright (C) 2019-20 Texas Instruments Incorporated - http://www.ti.com/
+// Author: Dan Murphy <[email protected]>
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include "leds.h"
+
+int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
+ enum led_brightness brightness)
+{
+ struct led_classdev *led_cdev = &mcled_cdev->led_cdev;
+ int i;
+
+ for (i = 0; i < mcled_cdev->num_colors; i++)
+ mcled_cdev->subled_info[i].brightness = brightness *
+ mcled_cdev->subled_info[i].intensity /
+ led_cdev->max_brightness;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(led_mc_calc_color_components);
+
+static ssize_t multi_intensity_store(struct device *dev,
+ struct device_attribute *intensity_attr,
+ const char *buf, size_t size)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
+ int nrchars, offset = 0;
+ int intensity_value[LED_COLOR_ID_MAX];
+ int i;
+ ssize_t ret;
+
+ mutex_lock(&led_cdev->led_access);
+
+ for (i = 0; i < mcled_cdev->num_colors; i++) {
+ ret = sscanf(buf + offset, "%i%n",
+ &intensity_value[i], &nrchars);
+ if (ret != 1) {
+ dev_dbg(led_cdev->dev,
+ "Incorrect number of LEDs expected %i values intensity was not applied\n",
+ mcled_cdev->num_colors);
+ ret = -EINVAL;
+ goto err_out;
+ }
+ offset += nrchars;
+ }
+
+ /* account for the space at the end of the buffer */
+ offset++;
+ if (offset < size) {
+ dev_dbg(led_cdev->dev,
+ "Incorrect number of LEDs expected %i values intensity was not applied\n",
+ mcled_cdev->num_colors);
+ ret =-EINVAL;
+ goto err_out;
+ }
+
+ for (i = 0; i < mcled_cdev->num_colors; i++)
+ mcled_cdev->subled_info[i].intensity = intensity_value[i];
+
+ led_set_brightness(led_cdev, led_cdev->brightness);
+ ret = size;
+err_out:
+ mutex_unlock(&led_cdev->led_access);
+ return ret;
+}
+
+static ssize_t multi_intensity_show(struct device *dev,
+ struct device_attribute *intensity_attr,
+ char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
+ int len = 0;
+ int i;
+
+ for (i = 0; i < mcled_cdev->num_colors; i++) {
+ len += sprintf(buf + len, "%d",
+ mcled_cdev->subled_info[i].intensity);
+ if (i < mcled_cdev->num_colors)
+ len += sprintf(buf + len, " ");
+ }
+
+ len += sprintf(buf + len, "\n");
+ return len;
+}
+static DEVICE_ATTR_RW(multi_intensity);
+
+static ssize_t multi_index_show(struct device *dev,
+ struct device_attribute *multi_index_attr,
+ char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
+ int len = 0;
+ int index;
+ int i;
+
+ for (i = 0; i < mcled_cdev->num_colors; i++) {
+ index = mcled_cdev->subled_info[i].color_index;
+ len += sprintf(buf + len, "%s", led_colors[index]);
+ if (i < mcled_cdev->num_colors)
+ len += sprintf(buf + len, " ");
+ }
+
+ len += sprintf(buf + len, "\n");
+ return len;
+}
+static DEVICE_ATTR_RO(multi_index);
+
+static struct attribute *led_multicolor_attrs[] = {
+ &dev_attr_multi_intensity.attr,
+ &dev_attr_multi_index.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(led_multicolor);
+
+int led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data)
+{
+ struct led_classdev *led_cdev;
+
+ if (!mcled_cdev)
+ return -EINVAL;
+
+ if (!mcled_cdev->num_colors)
+ return -EINVAL;
+
+ if (mcled_cdev->num_colors > LED_COLOR_ID_MAX)
+ return -EINVAL;
+
+ led_cdev = &mcled_cdev->led_cdev;
+ mcled_cdev->led_cdev.groups = led_multicolor_groups;
+
+ return led_classdev_register_ext(parent, led_cdev, init_data);
+}
+EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
+
+void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev)
+{
+ if (!mcled_cdev)
+ return;
+
+ led_classdev_unregister(&mcled_cdev->led_cdev);
+}
+EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister);
+
+static void devm_led_classdev_multicolor_release(struct device *dev, void *res)
+{
+ led_classdev_multicolor_unregister(*(struct led_classdev_mc **)res);
+}
+
+int devm_led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data)
+{
+ struct led_classdev_mc **dr;
+ int ret;
+
+ dr = devres_alloc(devm_led_classdev_multicolor_release,
+ sizeof(*dr), GFP_KERNEL);
+ if (!dr)
+ return -ENOMEM;
+
+ ret = led_classdev_multicolor_register_ext(parent, mcled_cdev,
+ init_data);
+ if (ret) {
+ devres_free(dr);
+ return ret;
+ }
+
+ *dr = mcled_cdev;
+ devres_add(parent, dr);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_register_ext);
+
+static int devm_led_classdev_multicolor_match(struct device *dev,
+ void *res, void *data)
+{
+ struct led_classdev_mc **p = res;
+
+ if (WARN_ON(!p || !*p))
+ return 0;
+
+ return *p == data;
+}
+
+void devm_led_classdev_multicolor_unregister(struct device *dev,
+ struct led_classdev_mc *mcled_cdev)
+{
+ WARN_ON(devres_release(dev,
+ devm_led_classdev_multicolor_release,
+ devm_led_classdev_multicolor_match, mcled_cdev));
+}
+EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_unregister);
+
+MODULE_AUTHOR("Dan Murphy <[email protected]>");
+MODULE_DESCRIPTION("Multi Color LED class interface");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h
new file mode 100644
index 000000000000..50bd986497f0
--- /dev/null
+++ b/include/linux/led-class-multicolor.h
@@ -0,0 +1,121 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* LED Multicolor class interface
+ * Copyright (C) 2019-20 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#ifndef _LINUX_MULTICOLOR_LEDS_H_INCLUDED
+#define _LINUX_MULTICOLOR_LEDS_H_INCLUDED
+
+#include <linux/leds.h>
+#include <dt-bindings/leds/common.h>
+
+struct mc_subled {
+ int color_index;
+ int brightness;
+ int intensity;
+ int channel;
+};
+
+struct led_classdev_mc {
+ /* led class device */
+ struct led_classdev led_cdev;
+ int num_colors;
+
+ struct mc_subled *subled_info;
+};
+
+static inline struct led_classdev_mc *lcdev_to_mccdev(
+ struct led_classdev *led_cdev)
+{
+ return container_of(led_cdev, struct led_classdev_mc, led_cdev);
+}
+
+#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
+/**
+ * led_classdev_multicolor_register_ext - register a new object of led_classdev
+ * class with support for multicolor LEDs
+ * @parent: the multicolor LED to register
+ * @mcled_cdev: the led_classdev_mc structure for this device
+ * @init_data: the LED class Multi color device initialization data
+ *
+ * Returns: 0 on success or negative error value on failure
+ */
+int led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data);
+
+static inline int led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL);
+}
+
+/**
+ * led_classdev_multicolor_unregister - unregisters an object of led_classdev
+ * class with support for multicolor LEDs
+ * @mcled_cdev: the multicolor LED to unregister
+ *
+ * Unregister a previously registered via led_classdev_multicolor_register
+ * object
+ */
+void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev);
+
+/* Calculate brightness for the monochrome LED cluster */
+int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
+ enum led_brightness brightness);
+
+int devm_led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data);
+
+static inline int devm_led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev,
+ NULL);
+}
+
+void devm_led_classdev_multicolor_unregister(struct device *parent,
+ struct led_classdev_mc *mcled_cdev);
+#else
+
+static inline int led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data)
+{
+ return -EINVAL;
+}
+
+static inline int led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL);
+}
+
+static inline void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) {};
+static inline int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
+ enum led_brightness brightness)
+{
+ return -EINVAL;
+}
+
+static inline int devm_led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data)
+{
+ return -EINVAL;
+}
+
+static inline int devm_led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev,
+ NULL);
+}
+
+static inline void devm_led_classdev_multicolor_unregister(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR) */
+#endif /* _LINUX_MULTICOLOR_LEDS_H_INCLUDED */
--
2.25.1


2020-05-04 20:33:28

by Jacek Anaszewski

[permalink] [raw]
Subject: Re: [PATCH v24 02/16] leds: multicolor: Introduce a multicolor class definition

Dan,

On 5/3/20 2:32 PM, Dan Murphy 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 <[email protected]>
> Signed-off-by: Dan Murphy <[email protected]>
> ---
[...]
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -30,6 +30,17 @@ config LEDS_CLASS_FLASH
> for the flash related features of a LED device. It can be built
> as a module.
>
> +config LEDS_CLASS_MULTI_COLOR
> + tristate "LED MultiColor LED Class Support"
> + depends on LEDS_CLASS
> + depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR

I was saying about adding this dependency to the drivers based on
LED mc class. This way it does not make any sense. Moreover it is
erroneous:

$ make menuconfig
drivers/leds/Kconfig:33:error: recursive dependency detected!

Instead you should add it to the Kconfig entries of all drivers
that depend on LED mc class, i.e.:

- config LEDS_LP50XX
- config LEDS_LP5521
- config LEDS_LP5523

Moreover there are still some checkpatch.pl problems:

---------------------------------------------------------------
0003-leds-multicolor-Introduce-a-multicolor-class-definit.patch
---------------------------------------------------------------
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#89: FILE: Documentation/leds/leds-class-multicolor.rst:1:
+====================================

ERROR: spaces required around that '=' (ctx:WxO)
#294: FILE: drivers/leds/led-class-multicolor.c:62:
+ ret =-EINVAL;
^

ERROR: space required before that '-' (ctx:OxV)
#294: FILE: drivers/leds/led-class-multicolor.c:62:
+ ret =-EINVAL;

WARNING: DT binding documents should be licensed (GPL-2.0-only OR
BSD-2-Clause)
#31: FILE: Documentation/devicetree/bindings/leds/leds-lp50xx.yaml:1:
+# SPDX-License-Identifier: GPL-2.0

WARNING: Block comments use * on subsequent lines
#705: FILE: drivers/leds/leds-lp50xx.c:636:
+ /* There are only 3 LEDs per module otherwise they should be
+ banked which also is presented as 3 LEDs*/

WARNING: Block comments use a trailing */ on a separate line
#705: FILE: drivers/leds/leds-lp50xx.c:636:
+ banked which also is presented as 3 LEDs*/


---------------------------------------------------------------
0008-ARM-dts-n900-Add-reg-property-to-the-LP5523-channel-.patch
---------------------------------------------------------------
WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?

---------------------------------------------------------------
0009-ARM-dts-imx6dl-yapp4-Add-reg-property-to-the-lp5562-.patch
---------------------------------------------------------------
WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?

---------------------------------------------------------------
0010-ARM-dts-ste-href-Add-reg-property-to-the-LP5521-chan.patch
---------------------------------------------------------------
WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?


> + 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.
> +


--
Best regards,
Jacek Anaszewski

2020-05-04 20:42:30

by Dan Murphy

[permalink] [raw]
Subject: Re: [PATCH v24 02/16] leds: multicolor: Introduce a multicolor class definition

Jacek

On 5/4/20 3:31 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 5/3/20 2:32 PM, Dan Murphy 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 <[email protected]>
>> Signed-off-by: Dan Murphy <[email protected]>
>> ---
> [...]
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -30,6 +30,17 @@ config LEDS_CLASS_FLASH
>>         for the flash related features of a LED device. It can be built
>>         as a module.
>>   +config LEDS_CLASS_MULTI_COLOR
>> +    tristate "LED MultiColor LED Class Support"
>> +    depends on LEDS_CLASS
>> +    depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
>
> I was saying about adding this dependency to the drivers based on
> LED mc class. This way it does not make any sense. Moreover it is
> erroneous:
>
> $ make menuconfig
> drivers/leds/Kconfig:33:error: recursive dependency detected!
>
> Instead you should add it to the Kconfig entries of all drivers
> that depend on LED mc class, i.e.:
>
> - config LEDS_LP50XX
> - config LEDS_LP5521
> - config LEDS_LP5523
>
> Moreover there are still some checkpatch.pl problems:
>
> ---------------------------------------------------------------
> 0003-leds-multicolor-Introduce-a-multicolor-class-definit.patch
> ---------------------------------------------------------------
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> #89: FILE: Documentation/leds/leds-class-multicolor.rst:1:
> +====================================

Great another requirement and issue


>
> ERROR: spaces required around that '=' (ctx:WxO)
> #294: FILE: drivers/leds/led-class-multicolor.c:62:
> +        ret =-EINVAL;
>              ^
>
> ERROR: space required before that '-' (ctx:OxV)
> #294: FILE: drivers/leds/led-class-multicolor.c:62:
> +        ret =-EINVAL;
>
Ack


> WARNING: DT binding documents should be licensed (GPL-2.0-only OR
> BSD-2-Clause)
> #31: FILE: Documentation/devicetree/bindings/leds/leds-lp50xx.yaml:1:
> +# SPDX-License-Identifier: GPL-2.0
>
ack
> WARNING: Block comments use * on subsequent lines
> #705: FILE: drivers/leds/leds-lp50xx.c:636:
> +        /* There are only 3 LEDs per module otherwise they should be
> +           banked which also is presented as 3 LEDs*/
>
> WARNING: Block comments use a trailing */ on a separate line
> #705: FILE: drivers/leds/leds-lp50xx.c:636:
> +           banked which also is presented as 3 LEDs*/
>
Ack
>
> ---------------------------------------------------------------
> 0008-ARM-dts-n900-Add-reg-property-to-the-LP5523-channel-.patch
> ---------------------------------------------------------------
> WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?
>
> ---------------------------------------------------------------
> 0009-ARM-dts-imx6dl-yapp4-Add-reg-property-to-the-lp5562-.patch
> ---------------------------------------------------------------
> WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?
>
> ---------------------------------------------------------------
> 0010-ARM-dts-ste-href-Add-reg-property-to-the-LP5521-chan.patch
> ---------------------------------------------------------------
> WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?
>
ACK * 3

Dan

2020-05-04 21:22:24

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v24 02/16] leds: multicolor: Introduce a multicolor class definition

Hi Dan,

I love your patch! Yet something to improve:

[auto build test ERROR on pavel-linux-leds/for-next]
[also build test ERROR on robh/for-next linus/master j.anaszewski-leds/for-next v5.7-rc4 next-20200504]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v24/20200505-031241
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: parisc-allnoconfig
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=parisc allnoconfig
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=parisc

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>

All errors (new ones prefixed by >>):

>> drivers/leds/Kconfig:33:error: recursive dependency detected!
>> drivers/leds/Kconfig:33: symbol LEDS_CLASS_MULTI_COLOR depends on LEDS_CLASS_MULTI_COLOR
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"

vim +33 drivers/leds/Kconfig

16
17 config LEDS_CLASS
18 tristate "LED Class Support"
19 help
20 This option enables the LED sysfs class in /sys/class/leds. You'll
21 need this to do anything useful with LEDs. If unsure, say N.
22
23 config LEDS_CLASS_FLASH
24 tristate "LED Flash Class Support"
25 depends on LEDS_CLASS
26 help
27 This option enables the flash LED sysfs class in /sys/class/leds.
28 It wraps LED Class and adds flash LEDs specific sysfs attributes
29 and kernel internal API to it. You'll need this to provide support
30 for the flash related features of a LED device. It can be built
31 as a module.
32
> 33 config LEDS_CLASS_MULTI_COLOR
34 tristate "LED MultiColor LED Class Support"
35 depends on LEDS_CLASS
36 depends on LEDS_CLASS_MULTI_COLOR || !LEDS_CLASS_MULTI_COLOR
37 help
38 This option enables the multicolor LED sysfs class in /sys/class/leds.
39 It wraps LED class and adds multicolor LED specific sysfs attributes
40 and kernel internal API to it. You'll need this to provide support
41 for multicolor LEDs that are grouped together. This class is not
42 intended for single color LEDs. It can be built as a module.
43

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]