lkml.org 
[lkml]   [2020]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v19 04/18] leds: multicolor: Introduce a multicolor class definition
From
Date
Jacek

On 4/6/20 2:25 PM, Jacek Anaszewski wrote:
> Hi Dan,
>
> Thanks for the update. I have a few comments below.
>
> On 4/2/20 10:42 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 <color>_intensity files and the latter is controlled
>> via brightness file.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>> .../ABI/testing/sysfs-class-led-multicolor | 42 ++++
>> Documentation/leds/index.rst | 1 +
>> Documentation/leds/leds-class-multicolor.rst | 95 ++++++++
>> drivers/leds/Kconfig | 10 +
>> drivers/leds/Makefile | 1 +
>> drivers/leds/led-class-multicolor.c | 206 ++++++++++++++++++
>> include/linux/led-class-multicolor.h | 121 ++++++++++
>> 7 files changed, 476 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..1f50324d24fd
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
>> @@ -0,0 +1,42 @@
>> +What: /sys/class/leds/<led>/brightness
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <dmurphy@ti.com>
>> +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 * color_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>/color_index
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <dmurphy@ti.com>
>> +Description: read
>> + The color_index array, when read, will output the LED colors
>> + by name as they are indexed in the color_intensity array.
>> +
>> +What: /sys/class/leds/<led>/num_colors
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <dmurphy@ti.com>
>> +Description: read
>> + The num_colors indicates the number of LEDs defined in the
>> + color_intensity and color_index arrays.
>> +
>> +What: /sys/class/leds/<led>/color_intensity
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <dmurphy@ti.com>
>> +Description: read/write
>> + Intensity level for the LED color within the array.
>> + The intensities for each color must be entered based on the
>> + color_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..5bb004999248
>> --- /dev/null
>> +++ b/Documentation/leds/leds-class-multicolor.rst
>> @@ -0,0 +1,95 @@
>> +====================================
>> +Multi Color LED handling under Linux
>> +====================================
>> +
>> +Description
>> +===========
>> +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 color_intensity array file and the latter is controlled
>> +via brightness file.
>> +
>> +For more details on hue and lightness notions please refer to
>> +https://en.wikipedia.org/wiki/CIECAM02.
>> +
>> +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 color_* files. The order of the
>> +colors are arbitrary the color_index file can be read to determine the color
> Something wrong with punctuation here, or it is even a stylistic
> problem.
Ack and fixed
>
>> +to index value.
>> +
>> +The color_index file is an array that contains the string list of the colors as
>> +they are defined in each color_* array file.
>> +
>> +The color_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.
>> +
>> +The color_max_intensity is an array that can be read to indicate each color LED
>> +maximum intensity value.
>> +
>> +The num_color_leds file returns the total number of color LEDs that are
>> +presented in each color_* array.
>> +
>> +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 color_index
>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16 color_intensity
>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 num_color_leds
>> +
>> +Multicolor Class Brightness Control
>> +===================================
>> +The multiclor 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 parent max_brightness setting multiplied by
>> +the requested brightness.
>> +
>> +led_brightness = brightness * color_intensity/max_brightness
>> +
>> +Example:
>> +A user first writes the color_intensity file with the brightness levels
>> +that for each LED that is necessary to achieve a blueish violet output from a
>> +RGB LED group.
>> +
>> +cat /sys/class/leds/multicolor:status/color_index
>> +green blue red
>> +
>> +echo 43 226 138 > /sys/class/leds/multicolor:status/color_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 RGB group by writing the parent
> s/RGB/multicolor/
Ack and fixed throughout the doc
>
>> +'brightness' control. Assuming a parent 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
>> +parent 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 parent brightness file will return the current brightness value of
>> +the color LED group.
>> +
>> +cat /sys/class/leds/multicolor:status/brightness
>> +128
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index d82f1dea3711..2e4611b25054 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -30,6 +30,16 @@ 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 Multi Color LED Class Support"
>> + 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.
>> +
>> 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 d7e1107753fb..310b5518783a 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
>> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
>> new file mode 100644
>> index 000000000000..838b1d679647
>> --- /dev/null
>> +++ b/drivers/leds/led-class-multicolor.c
>> @@ -0,0 +1,206 @@
>> +// 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 <dmurphy@ti.com>
>> +
>> +#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->multicolor_info[i].color_brightness = (brightness *
>> + mcled_cdev->multicolor_info[i].color_led_intensity /
>> + led_cdev->max_brightness);
> Why the need for brackets?

It was cleaner to read that was all.


>
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(led_mc_calc_color_components);
>> +
>> +static ssize_t color_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_err(led_cdev->dev,
>> + "Incorrect number of LEDs expected %i values intensity was not applied\n",
>> + mcled_cdev->num_colors);
>> + goto err_out;
>> + }
>> + offset += nrchars;
>> + }
>> +
>> + for (i = 0; i < mcled_cdev->num_colors; i++)
>> + mcled_cdev->multicolor_info[i].color_led_intensity =
>> + intensity_value[i];
>> +
>> + led_set_brightness(led_cdev, led_cdev->brightness);
>> +err_out:
>> + ret = size;
>> + mutex_unlock(&led_cdev->led_access);
>> + return ret;
>> +}
>> +
>> +static ssize_t color_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->multicolor_info[i].color_led_intensity);
>> +
>> + len += sprintf(buf + len, "%s", "\n");
>> +
>> + return len;
>> +}
>> +static DEVICE_ATTR_RW(color_intensity);
>> +
>> +static ssize_t color_index_show(struct device *dev,
>> + struct device_attribute *color_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->multicolor_info[i].color_index;
>> + len += sprintf(buf + len, "%s ", led_colors[index]);
>> + }
>> +
>> + len += sprintf(buf + len, "%s", "\n");
>> +
>> + return len;
>> +}
>> +static DEVICE_ATTR_RO(color_index);
>> +
>> +static ssize_t num_colors_show(struct device *dev,
>> + struct device_attribute *max_intensity_attr,
>> + char *buf)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
>> +
>> + return sprintf(buf, "%d\n", mcled_cdev->num_colors);
>> +}
>> +static DEVICE_ATTR_RO(num_colors);
>> +
>> +static struct attribute *led_multicolor_attrs[] = {
>> + &dev_attr_color_intensity.attr,
>> + &dev_attr_color_index.attr,
>> + &dev_attr_num_colors.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;
>> +
>> + 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 <dmurphy@ti.com>");
>> +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..a6a3715c0d48
>> --- /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 led_multicolor_info {
>> + int color_index;
>> + int color_brightness;
>> + int color_led_intensity;
>> + int color_channel;
>> +};
>> +
>> +struct led_classdev_mc {
>> + /* led class device */
>> + struct led_classdev led_cdev;
>> + int num_colors;
>> +
>> + struct led_multicolor_info multicolor_info[LED_COLOR_ID_MAX];
> I still have a problem with this fixed array that can grow for each
> driver as new color identifiers are added to the LED core. Not
> mentioning waste of memory due to unused colors. This is bad design.

I did try to do the dynamic allocation in both the device driver and
framework

When I used this

        led->mc_cdev.multitest_info = devm_kzalloc(&priv->client->dev,
        struct_size(&led->mc_cdev, multitest_info, num_colors),
               GFP_KERNEL);

The compiler complained.

drivers/leds/leds-lp50xx.c: In function ‘lp50xx_probe_dt’:
drivers/leds/leds-lp50xx.c:669:31: error: invalid use of flexible array
member
  669 |   led->mc_cdev.multitest_info = devm_kzalloc(&priv->client->dev,
      |                               ^

The pointer was defined as

    struct led_multicolor_info *multitest_info[];

The led_classdev_mc structure is allocated in the device driver
structure.  This struct contains the multicolor_info structure.

Unless I am doing this incorrectly.

Dan


Dan

\
 
 \ /
  Last update: 2020-04-07 16:59    [W:0.266 / U:8.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site