[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a8a22af2-3d11-bda8-5453-768e2174dc08@traphandler.com>
Date: Thu, 6 Apr 2023 08:54:38 +0200
From: Jean-Jacques Hiblot <jjhiblot@...phandler.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: <lee.jones@...aro.org>, <pavel@....cz>, <robh+dt@...nel.org>,
<sven.schwermer@...ruptive-technologies.com>,
<krzysztof.kozlowski+dt@...aro.org>, <johan+linaro@...nel.org>,
<marijn.suijten@...ainline.org>, <jacek.anaszewski@...il.com>,
<linux-leds@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 6/6] leds: Add a multicolor LED driver to group
monochromatic LEDs
On 28/03/2023 19:20, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 7:15 PM Jean-Jacques Hiblot
> <jjhiblot@...phandler.com> wrote:
>>
>> Grouping multiple monochrome LEDs into a multicolor LED device has a few
>> benefits over handling the group in user-space:
>> - The state of the LEDs relative to each other is consistent. In other
>> words, if 2 threads competes to set the LED to green and red, the
>> end-result cannot be black or yellow.
>> - The multicolor LED as a whole can be driven through the sysfs LED
>> interface.
>
> ...
>
>> +config LEDS_GROUP_MULTICOLOR
>> + tristate "LEDs group multi-color support"
>> + depends on OF || COMPILE_TEST
>
> Why is OF a dependency?
> Can't we make it a firmware provider agnostic solution from day 1?
That would be possible, unfortunately I have no way of testing the ACPI
stuff. That's why I prefer sticking to OF.
>
>> + help
>> + This option enables support for monochrome LEDs that are
>> + grouped into multicolor LEDs.
>> + This useful in the case where LEDs of different colors are
>
> This is
>
>> + physically grouped in a single multi-color LED and driven
>> + by a controller that doesn't have multi-color support.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called leds-group-multicolor.
>
> ...
>
>> + led_cdev = devm_of_led_get_optional(dev, count);
>
> I don't see how this is OF specific to this driver. Maybe it needs to
> be patched first, so we will have something non-OF specific?
>
>> + if (IS_ERR(led_cdev))
>> + return dev_err_probe(dev, PTR_ERR(led_cdev), "Unable to get LED #%d",
>> + count);
>
> At least we need to have a FIXME or so here.
>
Powered by blists - more mailing lists