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

[Jacek Anaszewski]

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