Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Dan Murphy <dmurphy@ti.com>, pavel@ucw.cz
Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
	Greg KH <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v18 4/4] leds: multicolor: Introduce a multicolor class definition
Date: Sun, 29 Mar 2020 20:43:42 +0200
Message-ID: <ea7915a1-91ea-43d5-2e96-99d1ecc49875@gmail.com> (raw)
In-Reply-To: <0e782f2d-2968-50c0-bf34-7ee781014ef8@ti.com>

Dan,

On 3/29/20 5:14 PM, Dan Murphy wrote:
> Jacek
> 
> On 3/29/20 7:47 AM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 3/28/20 10:31 PM, Dan Murphy wrote:
>>> Jacek
>>>
>>> Thanks for the review
>>>
>>> On 3/28/20 9:03 AM, Jacek Anaszewski wrote:
>>>> Hi Dan,
>>>>
>>>> Thanks for the update. The picture would be more complete if
>>>> the patch set contained a client though.
>>> I was going to send the ones I did but they are pretty dirty from a code
>>> stand point.
>>>
>>> Besides I would expect the framework to change which then changes the
>>> driver code.
>>>
>>>> Anyway, please find my review remarks below.
>>>>
>>>> On 3/24/20 7:14 PM, Dan Murphy wrote:
>>>>> Introduce a multicolor class that groups colored LEDs
>>>>> within a LED node.
>>>>>
>> [...]
>>>>> +What:        /sys/class/leds/<led>/color_max_intensity
>>>>> +Date:        March 2020
>>>>> +KernelVersion:    5.8
>>>>> +Contact:    Dan Murphy <dmurphy@ti.com>
>>>>> +Description:    read
>>>>> +        Maximum intensity level for the LED color within the array.
>>>>> +        The max intensities for each color must be entered based
>>>>> on the
>>>>> +        color_index array.
>>>> I wonder if we should mention here that each LED within a cluster
>>>> should
>>>> have the same maximum intensity for linear color lightness calculation
>>>> via brightness file.
>>> Does it really have to?
>> Say we have two LEDs:
>>
>> red, intensity = 255, max_intensity = 255
>> green, intensity = 15, max_intensity = 15
>>
>> If setting global brightness to 127 we have:
>>
>> led[red].brightness = 127 * 255 / 255 = 127
>> led[green].brightness = 127 * 15 / 15 = 15 (cut down to max_intensity)
>>
>> Clearly for green LED you're not getting a value being a half of
>> the intensity range.
>>
>> In addition to my previous statement, global max_brightness
>> should also have the same value as all max color intensities.
> OK I will update the code and docs to indicate max_intensity should
> equal max_brightness.
>>
>> [...]
>>>>> +Directory Layout Example
>>>>> +========================
>>>>> +root:/sys/class/leds/multicolor:grouped_leds# 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
>>>>> color_max_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 color LED max intensity setting
>>>>> multiplied by
>>>>> +the requested brightness.
>>>>> +
>>>>> +led_brightness = brightness * color_intensity/color_max_intensity
>>>> Maybe some pseudo code would allow for better understanding here:
>>>>
>>>> for color in color_intensity
>>>>       led[color].brightness = brightness *
>>>>      led[color].intensity / led[color].max_intensity
>>> I think this would be fine at least there is a documented equation. I
>>> don't think we need to document the code.
>> You mean what would be fine - my or your solution ? :-)
> My solution is probably good enough for documentation

OK, this description actually mentions "The brightness level for each
LED" so it should be fine as is.

>> [...]
>>>>> +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 *priv = 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 < priv->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",
>>>>> +                priv->num_colors);
>>>>> +            goto err_out;
>>>>> +        }
>>>>> +        offset += nrchars;
>>>>> +    }
>>>> I've just realized that moving to single color_intensity file
>>>> doesn't allow setting all colors together with new brightness
>>>> atomically. In effect, we will need to pass brightness to this file
>>>> too,
>>>> if we want to avoid "interesting" latching via brightenss file.
>>>>
>>>> Then we would need to call led_set_brightness() from here as well.
>>> Why?  This just caches the intensity of each colored LED.  Then the
>>> actual brightness is calculated only when the brightness file is
>>> updated.
>> And this is wrong. We should be able to set the color with a single
>> write.
> 
> Well that is a change in philosophy and direction from the prior patches.
> 
> You only wanted the LEDs to update when the brightness file was written.
> Why the change in stance?

It was devised for an interface with separate sysfs file per color.
Now we can leverage the fact that we will have single file for setting
all color intensities.

>>> This would be an automatic update of the LED and that is not the intent
>>> of the intensity file per the documentation.
>> Documentation needs to be changed then.
>>
>>> The user should be able to set the colors x number of times before the
>>> LED group is actually updated with the brightness.
>> What benefit would stem from that? In fact we should be able to
>> atomically set color in two ways, either via brightness or via
>> color_intensity file.
>>
>> But in previous message I unnecessarily proposed the addition
>> of brightness to the color_intensity interface. It is not needed
>> since updating color intensities should be considered as setting
>> entirely new color and that should reset global brightness to
>> max_brightness.
>>
>> Therefore here we should call at the end:
>>
>> led_set_brightness(led_cdev, led_cdev->max_brightness);
>>
>> That will update each color LED with new brightness values which
>> will correspond exactly to the color intensities just written.
>>
>> [...]
>>>>> diff --git a/include/linux/led-class-multicolor.h
>>>>> b/include/linux/led-class-multicolor.h
>>>>> new file mode 100644
>>>>> index 000000000000..bfbde2e98340
>>>>> --- /dev/null
>>>>> +++ b/include/linux/led-class-multicolor.h
>>>>> @@ -0,0 +1,124 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +/* LED Multicolor class interface
>>>>> + * Copyright (C) 2019 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_classdev_mc {
>>>>> +    /* led class device */
>>>>> +    struct led_classdev led_cdev;
>>>>> +
>>>>> +    struct device_attribute color_max_intensity_attr;
>>>>> +    struct device_attribute color_intensity_attr;
>>>>> +    struct device_attribute color_index_attr;
>>>> These are no longer needed as you define attrs statically.
>>> Ack
>>>>> +
>>>>> +    int color_brightness[LED_COLOR_ID_MAX];
>>>>> +
>>>>> +    int color_led_intensity[LED_COLOR_ID_MAX];
>>>>> +    int color_led_max_intensity[LED_COLOR_ID_MAX];
>>>>> +    const char *color_index[LED_COLOR_ID_MAX];
>>>> I think that we should get back to the available_colors bitmask
>>>> and allow the framework to allocate arrays by itself.
>>>> And yes, all the above should be pointers.
>>>>
>>>> Driver would only need to set led_mcdev->available_colors bits.
>>> Nack to the available_colors.  I did this originally and the issue is
>>> that the driver sets the bits in available_colors and no matter what the
>>> order is in the DT file the indexing is always red green and blue per
>>> the LED_COLORS array.  The framework has no legitimate way to know the
>>> order in which the colors were added.
>>>
>>> This posed an issue with the LP55xx code as the RGB was defined with
>>> different colors assigned to different channels.  Green was 0 blue was 2
>>> and red was 6.  So the driver would have to map the channels to the
>>> colors.  In forcing the device driver to set the color index it can then
>>> map the output channels itself.  The framework should not care what
>>> channel is for what color.  In either case the device driver will need
>>> to store the color index mapped to the channel output but having the
>>> index to color being a 1-1 mapping made the code much simpler for the
>>> device driver.
>>>
>>> Basically it turned out to be a simple for loop that just stored both
>>> channel and color as opposed to having to re-map the colors to indexes.
>>>
>>> So for the LP55xx I can get an index of green, blue red and that maps to
>>> the channels per the DT.  I don't think the framework should enforce a
>>> standard color index array ordering.
>> OK, if that indeed helps simplifying the code on the driver side.
>> But maybe it would be possible to come up with some generic helpers
>> for color sub-LEDs initialization?
> 
> Well the only helpers I can think of is a DT helper to parse the
> Multicolor child nodes.  But that maybe more complicated for the LP55xx
> as the children have additional
> 
> properties to parse through.  But the multicolor framework can parse the
> color and channel which means we would have to mandate the use of a
> specific common property node like
> 
> led_sources even if it is just a single channel.

Let's see how the driver will look like after adding array allocations.
Maybe something will come to one's mind after seeing the code.

>>> If we use the available_colors we don't even need the color_index and we
>>> can just pass the available_colors to the user space as a u32 and let
>>> the user space figure what colors are available. Which means the user
>>> space would assume the order per the LED_COLORS array.
>> Sysfs should be rather human readable so this would not necessarily
>> need to be the case.
>>
> That is what I was thinking as well.
> 
> Dan
> 

-- 
Best regards,
Jacek Anaszewski

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 18:14 [PATCH v18 0/4] Multi Color LED Framework Patches Dan Murphy
2020-03-24 18:14 ` [PATCH v18 1/4] dt: bindings: Add multicolor class dt bindings documention Dan Murphy
2020-03-24 18:14 ` [PATCH v18 2/4] dt-bindings: leds: Add multicolor ID to the color ID list Dan Murphy
2020-03-24 18:14 ` [PATCH v18 3/4] " Dan Murphy
2020-03-24 18:14 ` [PATCH v18 4/4] leds: multicolor: Introduce a multicolor class definition Dan Murphy
2020-03-24 18:41   ` Randy Dunlap
2020-03-24 18:52     ` Dan Murphy
2020-03-28 14:03   ` Jacek Anaszewski
2020-03-28 21:31     ` Dan Murphy
2020-03-29 12:47       ` Jacek Anaszewski
2020-03-29 15:14         ` Dan Murphy
2020-03-29 18:43           ` Jacek Anaszewski [this message]
2020-03-29 21:09             ` Dan Murphy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ea7915a1-91ea-43d5-2e96-99d1ecc49875@gmail.com \
    --to=jacek.anaszewski@gmail.com \
    --cc=dmurphy@ti.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=pavel@ucw.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git