Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 12/21/18 2:05 PM, Dan Murphy wrote:
> On 12/21/2018 01:32 AM, Jacek Anaszewski wrote:
>> On 12/20/18 9:31 PM, Jacek Anaszewski wrote:
>>> On 12/19/18 10:50 PM, Dan Murphy wrote:
>>>> On 12/19/2018 03:36 PM, Jacek Anaszewski wrote:
>>>>> Hi Dan and Pavel,
>>>>>
>>>>> On 12/19/18 9:41 PM, Dan Murphy wrote:
>>>>>> Pavel
>>>>>>
>>>>>> On 12/19/2018 02:10 PM, Pavel Machek wrote:
>>>>>>> On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
>>>>>>>> Pavel
>>>>>>>>
>>>>>>>> Thanks for the review.
>>>>>>>>
>>>>>>>> On 12/19/2018 01:34 PM, Pavel Machek wrote:
>>>>>>>>> Hi!
>>>>>>>>>
>>>>>>>>>> +static DEVICE_ATTR_WO(ctrl_bank_a_mix);
>>>>>>>>>> +static DEVICE_ATTR_WO(ctrl_bank_b_mix);
>>>>>>>>>> +static DEVICE_ATTR_WO(ctrl_bank_c_mix);
>>>>>>>>>> +
>>>>>>>>>> +static struct attribute *lp5024_ctrl_bank_attrs[] = {
>>>>>>>>>> +    &dev_attr_ctrl_bank_a_mix.attr,
>>>>>>>>>> +    &dev_attr_ctrl_bank_b_mix.attr,
>>>>>>>>>> +    &dev_attr_ctrl_bank_c_mix.attr,
>>>>>>>>>> +    NULL
>>>>>>>>>> +};
>>>>>>>>>> +ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>>> +
>>>>>>>>>> +static DEVICE_ATTR_WO(led1_mix);
>>>>>>>>>> +static DEVICE_ATTR_WO(led2_mix);
>>>>>>>>>> +static DEVICE_ATTR_WO(led3_mix);
>>>>>>>>>> +
>>>>>>>>>> +static struct attribute *lp5024_led_independent_attrs[] = {
>>>>>>>>>> +    &dev_attr_led1_mix.attr,
>>>>>>>>>> +    &dev_attr_led2_mix.attr,
>>>>>>>>>> +    &dev_attr_led3_mix.attr,
>>>>>>>>>> +    NULL
>>>>>>>>>> +};
>>>>>>>>>> +ATTRIBUTE_GROUPS(lp5024_led_independent);
>>>>>>>>>
>>>>>>>>> Ok, so you are adding new sysfs files. Are they _really_ neccessary?
>>>>>>>>> We'd like to have uniform interfaces for the leds.
>>>>>>>>
>>>>>>>> Yes I am adding these for this driver.
>>>>>>>> They adjust the individual brightness of each LED connected (what the HW guys called mixing).
>>>>>>>>
>>>>>>>> The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all 3 LEDs brightness are
>>>>>>>> uniform.
>>>>>>>
>>>>>>> 1 LED, 1 brightness file... that's what we do. Why should this one be different?
>>>>>>>
>>>>>>
>>>>>> Yes I understand this and 1 DT child node per LED out but...
>>>>>>
>>>>>> This device has a single register to control 3 LEDs brightness as a group and individual brightness
>>>>>> registers to control the LEDs independently to mix the LEDs to a specific color.
>>>>>>
>>>>>> There needs to be a way to control both so that developers can mix and adjust the individual RGB and
>>>>>> then control the brightness of the group during run time without touching the "mixing" value.
>>>>>>
>>>>>> I don't think a user needs nor would want to have 24 different LED nodes with 24 different brightness files.
>>>>>> Or with the LP5036 that would have 36 LED nodes.
>>>>>>
>>>>>> Table 1 in the data sheet shows how the outputs map to the control banks to the LED registers.
>>>>>
>>>>> Some time ago we had discussion with Vesa Jääskeläinen about possible
>>>>> approaches to RGB LEDs [0]. What seemed to be the most suitable
>>>>> variation of the discussed out-of-tree approach was the "color" property
>>>>> and array of color triplets defined in Device Tree per each color.
>>>>>
>>>>
>>>> Why does Device tree define the color?
>>>>
>>>> Rob indicated that Device tree is supposed to define the hardware.
>>>> This thread seems to be defining the operation.
>>>
>>> Perceived colors produced by LEDs from different manufacturers may
>>> differ and this alone should be deemed a sufficient argument for having
>>> board specific color definitions.
>>>
>>>> Shouldn't the color be done via user space and not dt?
>>>
>>> I think that we should keep the userspace interface as simple
>>> as possible and backwards compatible with monochrome LEDs.
>>>
>>> I also propose to avoid the introduction of a color sysfs
>>> property in favor of creating separate LED class devices
>>> for different "color ranges". The devices would drive the same
>>> LED but using different preset color levels.
>>
>> On the other hand, scattering the control over the hardware
>> among multiple LED class devices would complicate extension
>> of pattern trigger with the support for RGB LEDs.
>>
>> I looks like we will need the "color" sysfs file  anyway.
>>
>>> We don't have to expose all device knobs to the userspace,
>>> but instead provide some predefined configurations. It would
>>> improve user experience by keeping LED class devices simple
>>> in use. It would be Device Tree designer's responsibility to
>>> provide color definitions that make sense for given RGB LED
>>> controller and RGB LED element configuration.
>>>
>>> Registering color palette with devm_rgb_register() you proposed
>>> is also an option, but with one LED class device per color palette
>>> it would mean allowing for creation/destruction of LED class
>>> devices by any user having access to given LED's sysfs interface,
>>> which is really bad solution.
>>
>> With the "color" sysfs file it will make more sense to allow for user
>> defined color palettes.
>>
> 
> I think defining these values in the device tree or acpi severely limits the devices
> capabilities.  Especially in development phases.  If the knobs were exposed then the user space
> can create new experiences.  The color definition should be an absolute color defined in the dt and
> either the framework or user space needs to mix these appropriately.  IMO user space should set the policy
> of the user experience and the dt/acpi needs to set the capabilities.
> 
> I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group.
> 
> Maybe the framework could take these groups and combine/group them into a single node with the groups colors.

There is still HSV approach [0] in store. One problem with proposed
implementation is fixed algorithm of RGB <-> HSV color space conversion.
Maybe allowing for some board specific adjustments in DT would add
more flexibility.

[0] https://lkml.org/lkml/2017/8/31/255

-- 
Best regards,
Jacek Anaszewski