Re: [PATCH v4 2/2] leds: initial support for Turris Omnia LEDs

[Dan Murphy <dmurphy@ti.com>]

Marek

On 7/15/20 2:03 PM, Marek Behún wrote:
> On Wed, 15 Jul 2020 08:27:51 -0500
> Dan Murphy <dmurphy@ti.com> wrote:
>
>> Can this be built as a module?
> Yes. But only if MC framework is compiled in. If MC framework is
> compiled as module as well, this results in
>    FATAL: modpost: GPL-incompatible module led-class-multicolor.ko uses
>    GPL-only symbol 'led_set_brightness'

Hmm.  That is interesting

<snip>

>>> +	led->subled_info[0].color_index = LED_COLOR_ID_RED;
>>> +	led->subled_info[0].channel = 0;
>>> +	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
>>> +	led->subled_info[1].channel = 1;
>>> +	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
>>> +	led->subled_info[2].channel = 2;
>> OK this is why you may want to have sub-nodes.  Where reg is the
>> channel and color is the color.  Then you can do a for_each_child.
> Rob says that it doesn't need to be in DT if the controller only
> supports RGB LEDs. This controller is only in Turris Omnia which was
> never made with another kind of LEDs. So I think it is pointless and
> would only grow the DT and complicate the driver.
OK I know there was a discussion on this
>>> +	cdev->max_brightness = 255;
>> This is not needed.  It is defaulted to LED_FULL in led_class
> This was discussed last year and resulted in LED_FULL being
> declared obsolete in the header file.

No I am referring to setting the max_brightness to 255 the LED class 
sets this to 255 if the value is not set.


>>> +/*
>>> + * On the front panel of the Turris Omnia router there is also a
>>> button which can be used to control
>>> + * the intensity of all the LEDs at once, so that if they are too
>>> bright, user can dim them.
>>> + * The microcontroller cycles between 8 levels of this global
>>> brightness (from 100% to 0%), but this
>>> + * setting can have any integer value between 0 and 100.
>>> + * It is usable to be able to change this value from software, so
>>> that it does not start at 100%
>> This does not make sense.
> It does. The user changes the brightness of all 12 LEDs with the button
> to his liking and wants to have the same setting after powering
> the router on again.

No the english does not make sense

" It is usable to be able to change this value from software, so
that it does not start at 100%"

"It is usable" is not really clear.

>>> + * after every power on and annoy the user.
>>> + * We expose this setting via a sysfs attribute file called
>>> "brightness". This file lives in the
>>> + * device directory of the LED controller, not an individual LED,
>>> so it should not confuse users.
>>> + */
>> Sorry if this has been discussed already
>>
>> This seems a bit wonky.  You are overriding the brightness set by the
>> LED class.
> I am not. Pressing the button does not change the brightness read from
> the /sys/class/leds/<LED>/brightness file. This is different brightness,
> it is above the classic brightnes in the PWM hierarchy in the
> microcontroller. I discussed this with Pavel and he said we can call
> this file brightness as well (since it is brightness of the whole
> panel), and the file does not reside in /sys/class/leds/<LED> directory.

OK then there needs to be some ABI documentation no?

Dan