[lkml]   [2018]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
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.


Best regards,
Jacek Anaszewski

 \ /
  Last update: 2018-12-29 19:28    [W:0.121 / U:2.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site