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

From: Jacek Anaszewski
Date: Sat Dec 29 2018 - 13:28:15 EST

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:

On 12/19/2018 02:10 PM, Pavel Machek wrote:
On Wed 2018-12-19 13:41:18, Dan Murphy wrote:

Thanks for the review.

On 12/19/2018 01:34 PM, Pavel Machek wrote:

+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

+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

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

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