Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230328161541.599463-1-jjhiblot@traphandler.com> <20230328161541.599463-7-jjhiblot@traphandler.com>
In-Reply-To: <20230328161541.599463-7-jjhiblot@traphandler.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 28 Mar 2023 20:20:27 +0300
Message-ID: <CAHp75Vca0SKPkyPM_14Zny+Vn0a=hKWRfYW9qzTpN8AVZ_R_YQ@mail.gmail.com>
Subject: Re: [PATCH v8 6/6] leds: Add a multicolor LED driver to group
 monochromatic LEDs
To:     Jean-Jacques Hiblot <jjhiblot@traphandler.com>
Cc:     lee.jones@linaro.org, pavel@ucw.cz, robh+dt@kernel.org,
        sven.schwermer@disruptive-technologies.com,
        krzysztof.kozlowski+dt@linaro.org, johan+linaro@kernel.org,
        marijn.suijten@somainline.org, jacek.anaszewski@gmail.com,
        linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 28, 2023 at 7:15=E2=80=AFPM Jean-Jacques Hiblot
<jjhiblot@traphandler.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?

> +       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 =3D 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), "Una=
ble to get LED #%d",
> +                                            count);

At least we need to have a FIXME or so here.

--=20
With Best Regards,
Andy Shevchenko