[PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Aleksandrs Vinarskis]

Currently supports passing 'led-names' used to map LED devices to their
respective functions.

Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
---
 .../devicetree/bindings/leds/leds-consumer.yaml    | 69 ++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common leds consumer
+
+maintainers:
+  - Aleksandrs Vinarskis <alex@vinarskis.com>
+
+description:
+  Some LED defined in DT are required by other DT consumers, for example
+  v4l2 subnode may require privacy or flash LED.
+
+  Document LED properties that its consumers may define.
+
+properties:
+  leds:
+    description:
+      Phandle to LED device(s) required by particular consumer.
+    $ref: /schemas/types.yaml#/definitions/phandle
+  led-names:
+    description:
+      List of device name(s). Used to map LED devices to their respective
+      functions, when consumer requires more than one LED.
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      camera@36 {
+        compatible = "ovti,ov02c10";
+        reg = <0x36>;
+
+        reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&cam_rgb_default>;
+
+        led-names = "privacy-led";
+        leds = <&privacy_led>;
+
+        clocks = <&ov02e10_clk>;
+
+        assigned-clocks = <&ov02e10_clk>;
+        assigned-clock-rates = <19200000>;
+
+        avdd-supply = <&vreg_l7b_2p8>;
+        dvdd-supply = <&vreg_l7b_2p8>;
+        dovdd-supply = <&vreg_cam_1p8>;
+
+        port {
+          ov02e10_ep: endpoint {
+            data-lanes = <1 2>;
+            link-frequencies = /bits/ 64 <400000000>;
+            remote-endpoint = <&csiphy4_ep>;
+          };
+        };
+      };
+    };
+
+...

-- 
2.48.1

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Rob Herring (Arm)]

On Tue, 02 Sep 2025 11:10:51 +0000, Aleksandrs Vinarskis wrote:
> Currently supports passing 'led-names' used to map LED devices to their
> respective functions.
> 
> Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> ---
>  .../devicetree/bindings/leds/leds-consumer.yaml    | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/leds/leds-consumer.example.dtb: camera@36 (ovti,ov02c10): Unevaluated properties are not allowed ('led-names', 'leds' were unexpected)
	from schema $id: http://devicetree.org/schemas/media/i2c/ovti,ov02e10.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/010201990a1f5ad8-fc97fc84-9ef9-4a03-bf1c-2d54423c6497-000000@eu-west-1.amazonses.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Rob Herring]

On Tue, Sep 02, 2025 at 11:10:51AM +0000, Aleksandrs Vinarskis wrote:
> Currently supports passing 'led-names' used to map LED devices to their
> respective functions.
> 
> Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> ---
>  .../devicetree/bindings/leds/leds-consumer.yaml    | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common leds consumer
> +
> +maintainers:
> +  - Aleksandrs Vinarskis <alex@vinarskis.com>
> +
> +description:
> +  Some LED defined in DT are required by other DT consumers, for example
> +  v4l2 subnode may require privacy or flash LED.
> +
> +  Document LED properties that its consumers may define.

We already have the trigger-source binding for "attaching" LEDs to 
devices. Why does that not work here?

Rob

> +
> +properties:
> +  leds:
> +    description:
> +      Phandle to LED device(s) required by particular consumer.
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +  led-names:
> +    description:
> +      List of device name(s). Used to map LED devices to their respective
> +      functions, when consumer requires more than one LED.
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      camera@36 {
> +        compatible = "ovti,ov02c10";
> +        reg = <0x36>;
> +
> +        reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&cam_rgb_default>;
> +
> +        led-names = "privacy-led";
> +        leds = <&privacy_led>;
> +
> +        clocks = <&ov02e10_clk>;
> +
> +        assigned-clocks = <&ov02e10_clk>;
> +        assigned-clock-rates = <19200000>;
> +
> +        avdd-supply = <&vreg_l7b_2p8>;
> +        dvdd-supply = <&vreg_l7b_2p8>;
> +        dovdd-supply = <&vreg_cam_1p8>;
> +
> +        port {
> +          ov02e10_ep: endpoint {
> +            data-lanes = <1 2>;
> +            link-frequencies = /bits/ 64 <400000000>;
> +            remote-endpoint = <&csiphy4_ep>;
> +          };
> +        };
> +      };
> +    };
> +
> +...
> 
> -- 
> 2.48.1
> 

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Aleksandrs Vinarskis]

> On Tue, Sep 02, 2025 at 11:10:51AM +0000, Aleksandrs Vinarskis wrote:
> > Currently supports passing 'led-names' used to map LED devices to their
> > respective functions.
> > 
> > Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> > ---
> >  .../devicetree/bindings/leds/leds-consumer.yaml    | 69 ++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Common leds consumer
> > +
> > +maintainers:
> > +  - Aleksandrs Vinarskis <alex@vinarskis.com>
> > +
> > +description:
> > +  Some LED defined in DT are required by other DT consumers, for example
> > +  v4l2 subnode may require privacy or flash LED.
> > +
> > +  Document LED properties that its consumers may define.
> 
> We already have the trigger-source binding for "attaching" LEDs to 
> devices. Why does that not work here?

I have not actually considered this, as the existing privacy-led solution
from the original series is not trigger based. At least one of the reasons
for that is that trigger source can be rather easily altered from user
space, which would've been bad for this use case. If v4l2 acquires control
over the LED it actually removes triggers and disables sysfs on that LED.

Regarding DT check that is failing because 'ovti,ov02e10.yaml' does not
allow for additional properties - the same issue would apply to basically
any camera, I missed it. So would need to either include this new binding
in 'video-interface-devices.yaml', or drop new binding and directly include
these new generic LED related properties in the video one. However, in this
case it gets a bit ugly, as the latter already contains 'flash-leds' for
flash specifically, and we would be adding a more generic way only used for
privacy LED, at least for now... not too sure whats the best way here,
leaning towards 1st option.

Let me know what you think,

Alex


> 
> Rob
> 
> > +
> > +properties:
> > +  leds:
> > +    description:
> > +      Phandle to LED device(s) required by particular consumer.
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +  led-names:
> > +    description:
> > +      List of device name(s). Used to map LED devices to their respective
> > +      functions, when consumer requires more than one LED.
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      camera@36 {
> > +        compatible = "ovti,ov02c10";
> > +        reg = <0x36>;
> > +
> > +        reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&cam_rgb_default>;
> > +
> > +        led-names = "privacy-led";
> > +        leds = <&privacy_led>;
> > +
> > +        clocks = <&ov02e10_clk>;
> > +
> > +        assigned-clocks = <&ov02e10_clk>;
> > +        assigned-clock-rates = <19200000>;
> > +
> > +        avdd-supply = <&vreg_l7b_2p8>;
> > +        dvdd-supply = <&vreg_l7b_2p8>;
> > +        dovdd-supply = <&vreg_cam_1p8>;
> > +
> > +        port {
> > +          ov02e10_ep: endpoint {
> > +            data-lanes = <1 2>;
> > +            link-frequencies = /bits/ 64 <400000000>;
> > +            remote-endpoint = <&csiphy4_ep>;
> > +          };
> > +        };
> > +      };
> > +    };
> > +
> > +...
> > 
> > -- 
> > 2.48.1
> > 

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Krzysztof Kozlowski]

On Thu, Sep 04, 2025 at 01:56:15AM +0200, Aleksandrs Vinarskis wrote:
> > On Tue, Sep 02, 2025 at 11:10:51AM +0000, Aleksandrs Vinarskis wrote:
> > > Currently supports passing 'led-names' used to map LED devices to their
> > > respective functions.
> > > 
> > > Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> > > ---
> > >  .../devicetree/bindings/leds/leds-consumer.yaml    | 69 ++++++++++++++++++++++
> > >  1 file changed, 69 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> > > @@ -0,0 +1,69 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Common leds consumer
> > > +
> > > +maintainers:
> > > +  - Aleksandrs Vinarskis <alex@vinarskis.com>
> > > +
> > > +description:
> > > +  Some LED defined in DT are required by other DT consumers, for example
> > > +  v4l2 subnode may require privacy or flash LED.
> > > +
> > > +  Document LED properties that its consumers may define.
> > 
> > We already have the trigger-source binding for "attaching" LEDs to 
> > devices. Why does that not work here?
> 
> I have not actually considered this, as the existing privacy-led solution
> from the original series is not trigger based. At least one of the reasons
> for that is that trigger source can be rather easily altered from user
> space, which would've been bad for this use case. If v4l2 acquires control
> over the LED it actually removes triggers and disables sysfs on that LED.

So does that mean that v4l2 solves the problem of "trigger source can be
rather easily altered from user space"?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Hans de Goede]

Hi, 
On 4-Sep-25 8:41 AM, Krzysztof Kozlowski wrote:
> On Thu, Sep 04, 2025 at 01:56:15AM +0200, Aleksandrs Vinarskis wrote:
>>> On Tue, Sep 02, 2025 at 11:10:51AM +0000, Aleksandrs Vinarskis wrote:
>>>> Currently supports passing 'led-names' used to map LED devices to their
>>>> respective functions.
>>>>
>>>> Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
>>>> ---
>>>>  .../devicetree/bindings/leds/leds-consumer.yaml    | 69 ++++++++++++++++++++++
>>>>  1 file changed, 69 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
>>>> new file mode 100644
>>>> index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
>>>> @@ -0,0 +1,69 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Common leds consumer
>>>> +
>>>> +maintainers:
>>>> +  - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>> +
>>>> +description:
>>>> +  Some LED defined in DT are required by other DT consumers, for example
>>>> +  v4l2 subnode may require privacy or flash LED.
>>>> +
>>>> +  Document LED properties that its consumers may define.
>>>
>>> We already have the trigger-source binding for "attaching" LEDs to 
>>> devices. Why does that not work here?
>>
>> I have not actually considered this, as the existing privacy-led solution
>> from the original series is not trigger based. At least one of the reasons
>> for that is that trigger source can be rather easily altered from user
>> space, which would've been bad for this use case. If v4l2 acquires control
>> over the LED it actually removes triggers and disables sysfs on that LED.
> 
> So does that mean that v4l2 solves the problem of "trigger source can be
> rather easily altered from user space"?

Yes, currently the v4l2-core already does:

	sd->privacy_led = led_get(sd->dev, "privacy-led")
	led_sysfs_disable(sd->privacy_led);

Which disallows changing the LED state or trigger from
userspace. This is similar to how flash-LEDs are handled
which also involves directly controlling the LED rather
then using triggers and which also calls led_sysfs_disable().

led_get() already works for this on x86 and is already used
for this there which is why this code is already there.

I guess the difference with triggers is that triggers are
more of a soft binding between LED and controller of
the LED and where here we need more of a hard binding.

Regards,

Hans

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Krzysztof Kozlowski]

On 04/09/2025 09:26, Hans de Goede wrote:
>>>>> +maintainers:
>>>>> +  - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>> +
>>>>> +description:
>>>>> +  Some LED defined in DT are required by other DT consumers, for example
>>>>> +  v4l2 subnode may require privacy or flash LED.
>>>>> +
>>>>> +  Document LED properties that its consumers may define.
>>>>
>>>> We already have the trigger-source binding for "attaching" LEDs to 
>>>> devices. Why does that not work here?
>>>
>>> I have not actually considered this, as the existing privacy-led solution
>>> from the original series is not trigger based. At least one of the reasons
>>> for that is that trigger source can be rather easily altered from user
>>> space, which would've been bad for this use case. If v4l2 acquires control
>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>
>> So does that mean that v4l2 solves the problem of "trigger source can be
>> rather easily altered from user space"?
> 
> Yes, currently the v4l2-core already does:

Thanks, I understand that it solves the problem described in the patch,
so the patch can be dropped.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Hans de Goede]

Hi Krzysztof,

On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
> On 04/09/2025 09:26, Hans de Goede wrote:
>>>>>> +maintainers:
>>>>>> +  - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>>> +
>>>>>> +description:
>>>>>> +  Some LED defined in DT are required by other DT consumers, for example
>>>>>> +  v4l2 subnode may require privacy or flash LED.
>>>>>> +
>>>>>> +  Document LED properties that its consumers may define.
>>>>>
>>>>> We already have the trigger-source binding for "attaching" LEDs to 
>>>>> devices. Why does that not work here?
>>>>
>>>> I have not actually considered this, as the existing privacy-led solution
>>>> from the original series is not trigger based. At least one of the reasons
>>>> for that is that trigger source can be rather easily altered from user
>>>> space, which would've been bad for this use case. If v4l2 acquires control
>>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>>
>>> So does that mean that v4l2 solves the problem of "trigger source can be
>>> rather easily altered from user space"?
>>
>> Yes, currently the v4l2-core already does:
> 
> Thanks, I understand that it solves the problem described in the patch,
> so the patch can be dropped.

I'm a bit confused now, do you mean that this dt-bindings patch can
be dropped ?

The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
on DT there is no official bindings-docs for directly getting a LED with
led_get() AFAICT and I believe that having a binding is mandatory before
we just start adding leds and led-names properties to DT nodes for sensors ?

Maybe for v2 of this patch-set Aleksanders should also add a patch
actually using the new binding in a dts file to make clear that
the intent is to also start using privacy-LEDs in the same way
on DT systems ?

Regards,

Hans

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Krzysztof Kozlowski]

On 04/09/2025 12:29, Hans de Goede wrote:
> Hi Krzysztof,
> 
> On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
>> On 04/09/2025 09:26, Hans de Goede wrote:
>>>>>>> +maintainers:
>>>>>>> +  - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  Some LED defined in DT are required by other DT consumers, for example
>>>>>>> +  v4l2 subnode may require privacy or flash LED.
>>>>>>> +
>>>>>>> +  Document LED properties that its consumers may define.
>>>>>>
>>>>>> We already have the trigger-source binding for "attaching" LEDs to 
>>>>>> devices. Why does that not work here?
>>>>>
>>>>> I have not actually considered this, as the existing privacy-led solution
>>>>> from the original series is not trigger based. At least one of the reasons
>>>>> for that is that trigger source can be rather easily altered from user
>>>>> space, which would've been bad for this use case. If v4l2 acquires control
>>>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>>>
>>>> So does that mean that v4l2 solves the problem of "trigger source can be
>>>> rather easily altered from user space"?
>>>
>>> Yes, currently the v4l2-core already does:
>>
>> Thanks, I understand that it solves the problem described in the patch,
>> so the patch can be dropped.
> 
> I'm a bit confused now, do you mean that this dt-bindings patch can
> be dropped ?

Yes.

Alex's explanation to Rob felt confusing, so I asked for clarification.
You clarfiied that that v4l2 solves the problem, therefore there is no
problem to be solved.

If there is no problem to be solved, this patch is not needed.

If this patch is needed, just describe the problem accurately.

> 
> The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
> on DT there is no official bindings-docs for directly getting a LED with

There are and Rob pointed to them. If Rob's answer is not enough, make
it explicit.

Really, there are here some long explanations which do not really
explain this in simple terms. Simple term is: "existing property foo
does not work because <here goes the reason>".

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Hans de Goede]

Hi Krzysztof,

On 4-Sep-25 12:47 PM, Krzysztof Kozlowski wrote:
> On 04/09/2025 12:29, Hans de Goede wrote:
>> Hi Krzysztof,
>>
>> On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
>>> On 04/09/2025 09:26, Hans de Goede wrote:
>>>>>>>> +maintainers:
>>>>>>>> +  - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>>>>> +
>>>>>>>> +description:
>>>>>>>> +  Some LED defined in DT are required by other DT consumers, for example
>>>>>>>> +  v4l2 subnode may require privacy or flash LED.
>>>>>>>> +
>>>>>>>> +  Document LED properties that its consumers may define.
>>>>>>>
>>>>>>> We already have the trigger-source binding for "attaching" LEDs to 
>>>>>>> devices. Why does that not work here?
>>>>>>
>>>>>> I have not actually considered this, as the existing privacy-led solution
>>>>>> from the original series is not trigger based. At least one of the reasons
>>>>>> for that is that trigger source can be rather easily altered from user
>>>>>> space, which would've been bad for this use case. If v4l2 acquires control
>>>>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>>>>
>>>>> So does that mean that v4l2 solves the problem of "trigger source can be
>>>>> rather easily altered from user space"?
>>>>
>>>> Yes, currently the v4l2-core already does:
>>>
>>> Thanks, I understand that it solves the problem described in the patch,
>>> so the patch can be dropped.
>>
>> I'm a bit confused now, do you mean that this dt-bindings patch can
>> be dropped ?
> 
> Yes.
> 
> Alex's explanation to Rob felt confusing, so I asked for clarification.
> You clarfiied that that v4l2 solves the problem, therefore there is no
> problem to be solved.
> 
> If there is no problem to be solved, this patch is not needed.
> 
> If this patch is needed, just describe the problem accurately.
> 
>>
>> The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
>> on DT there is no official bindings-docs for directly getting a LED with
> 
> There are and Rob pointed to them. If Rob's answer is not enough, make
> it explicit.
> 
> Really, there are here some long explanations which do not really
> explain this in simple terms. Simple term is: "existing property foo
> does not work because <here goes the reason>".

The existing trigger-source binding for "attaching" LEDs to 
devices does not work because:

1. It depends on the Linux specific LED trigger mechanism where as
   DT should describe hw in an OS agnostic manner

2. It puts the world upside down by giving possible event-sources 
   for the (again) Linux specific trigger rather then allowing
   specifying e.g. specific privacy and flash LEDs as part
   of a camera dts node. IOW it makes the LED DT note point to
   the camera, while the LED is a part of the camera-module.
   not the other way around. So it does not properly allow
   describing the composition of the camera.

   Note that Rob actually put "" around attaching because this
   property really is not proper attaching / composition as
   we would normally do in dt.

IMHO 1. alone (this being Linux specific) warrants a new better
binding for this.

Regards,

Hans

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Hans de Goede]

Hi Krzysztof,

On 4-Sep-25 1:47 PM, Hans de Goede wrote:
> Hi Krzysztof,
> 
> On 4-Sep-25 12:47 PM, Krzysztof Kozlowski wrote:
>> On 04/09/2025 12:29, Hans de Goede wrote:
>>> Hi Krzysztof,
>>>
>>> On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
>>>> On 04/09/2025 09:26, Hans de Goede wrote:
>>>>>>>>> +maintainers:
>>>>>>>>> +  - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>>>>>> +
>>>>>>>>> +description:
>>>>>>>>> +  Some LED defined in DT are required by other DT consumers, for example
>>>>>>>>> +  v4l2 subnode may require privacy or flash LED.
>>>>>>>>> +
>>>>>>>>> +  Document LED properties that its consumers may define.
>>>>>>>>
>>>>>>>> We already have the trigger-source binding for "attaching" LEDs to 
>>>>>>>> devices. Why does that not work here?
>>>>>>>
>>>>>>> I have not actually considered this, as the existing privacy-led solution
>>>>>>> from the original series is not trigger based. At least one of the reasons
>>>>>>> for that is that trigger source can be rather easily altered from user
>>>>>>> space, which would've been bad for this use case. If v4l2 acquires control
>>>>>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>>>>>
>>>>>> So does that mean that v4l2 solves the problem of "trigger source can be
>>>>>> rather easily altered from user space"?
>>>>>
>>>>> Yes, currently the v4l2-core already does:
>>>>
>>>> Thanks, I understand that it solves the problem described in the patch,
>>>> so the patch can be dropped.
>>>
>>> I'm a bit confused now, do you mean that this dt-bindings patch can
>>> be dropped ?
>>
>> Yes.
>>
>> Alex's explanation to Rob felt confusing, so I asked for clarification.
>> You clarfiied that that v4l2 solves the problem, therefore there is no
>> problem to be solved.
>>
>> If there is no problem to be solved, this patch is not needed.
>>
>> If this patch is needed, just describe the problem accurately.
>>
>>>
>>> The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
>>> on DT there is no official bindings-docs for directly getting a LED with
>>
>> There are and Rob pointed to them. If Rob's answer is not enough, make
>> it explicit.
>>
>> Really, there are here some long explanations which do not really
>> explain this in simple terms. Simple term is: "existing property foo
>> does not work because <here goes the reason>".
> 
> The existing trigger-source binding for "attaching" LEDs to 
> devices does not work because:
> 
> 1. It depends on the Linux specific LED trigger mechanism where as
>    DT should describe hw in an OS agnostic manner
> 
> 2. It puts the world upside down by giving possible event-sources 
>    for the (again) Linux specific trigger rather then allowing
>    specifying e.g. specific privacy and flash LEDs as part
>    of a camera dts node. IOW it makes the LED DT note point to
>    the camera, while the LED is a part of the camera-module.
>    not the other way around. So it does not properly allow
>    describing the composition of the camera.
> 
>    Note that Rob actually put "" around attaching because this
>    property really is not proper attaching / composition as
>    we would normally do in dt.
> 
> IMHO 1. alone (this being Linux specific) warrants a new better
> binding for this.

And:

3. There already are bindings using a leds = phandle-array property in:
Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml

So we already have this as another and IMHO much clenaer way to tie
a LED to a device.

The suggest generic leds = phandle-array property description added
in this new binding just adds a leds-names to give names to the
various indexes in the array which is a very common design-pattern
in dt-bindings.

Regards,

Hans

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Rob Herring]

On Thu, Sep 04, 2025 at 02:05:08PM +0200, Hans de Goede wrote:
> Hi Krzysztof,
> 
> On 4-Sep-25 1:47 PM, Hans de Goede wrote:
> > Hi Krzysztof,
> > 
> > On 4-Sep-25 12:47 PM, Krzysztof Kozlowski wrote:
> >> On 04/09/2025 12:29, Hans de Goede wrote:
> >>> Hi Krzysztof,
> >>>
> >>> On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
> >>>> On 04/09/2025 09:26, Hans de Goede wrote:
> >>>>>>>>> +maintainers:
> >>>>>>>>> +  - Aleksandrs Vinarskis <alex@vinarskis.com>
> >>>>>>>>> +
> >>>>>>>>> +description:
> >>>>>>>>> +  Some LED defined in DT are required by other DT consumers, for example
> >>>>>>>>> +  v4l2 subnode may require privacy or flash LED.
> >>>>>>>>> +
> >>>>>>>>> +  Document LED properties that its consumers may define.
> >>>>>>>>
> >>>>>>>> We already have the trigger-source binding for "attaching" LEDs to 
> >>>>>>>> devices. Why does that not work here?
> >>>>>>>
> >>>>>>> I have not actually considered this, as the existing privacy-led solution
> >>>>>>> from the original series is not trigger based. At least one of the reasons
> >>>>>>> for that is that trigger source can be rather easily altered from user
> >>>>>>> space, which would've been bad for this use case. If v4l2 acquires control
> >>>>>>> over the LED it actually removes triggers and disables sysfs on that LED.
> >>>>>>
> >>>>>> So does that mean that v4l2 solves the problem of "trigger source can be
> >>>>>> rather easily altered from user space"?
> >>>>>
> >>>>> Yes, currently the v4l2-core already does:
> >>>>
> >>>> Thanks, I understand that it solves the problem described in the patch,
> >>>> so the patch can be dropped.
> >>>
> >>> I'm a bit confused now, do you mean that this dt-bindings patch can
> >>> be dropped ?
> >>
> >> Yes.
> >>
> >> Alex's explanation to Rob felt confusing, so I asked for clarification.
> >> You clarfiied that that v4l2 solves the problem, therefore there is no
> >> problem to be solved.
> >>
> >> If there is no problem to be solved, this patch is not needed.
> >>
> >> If this patch is needed, just describe the problem accurately.
> >>
> >>>
> >>> The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
> >>> on DT there is no official bindings-docs for directly getting a LED with
> >>
> >> There are and Rob pointed to them. If Rob's answer is not enough, make
> >> it explicit.
> >>
> >> Really, there are here some long explanations which do not really
> >> explain this in simple terms. Simple term is: "existing property foo
> >> does not work because <here goes the reason>".
> > 
> > The existing trigger-source binding for "attaching" LEDs to 
> > devices does not work because:
> > 
> > 1. It depends on the Linux specific LED trigger mechanism where as
> >    DT should describe hw in an OS agnostic manner
> > 

Using a binding does not require using the linux subsystem normally 
associated with it. Certainly the naming was inspired by the Linux 
subsystem, but it's really nothing more than a link.

> > 2. It puts the world upside down by giving possible event-sources 
> >    for the (again) Linux specific trigger rather then allowing
> >    specifying e.g. specific privacy and flash LEDs as part
> >    of a camera dts node. IOW it makes the LED DT note point to
> >    the camera, while the LED is a part of the camera-module.
> >    not the other way around. So it does not properly allow
> >    describing the composition of the camera.

Direction of the connection doesn't really matter. You can get the 
association either way. But certainly one way is easier than the other.

> > 
> >    Note that Rob actually put "" around attaching because this
> >    property really is not proper attaching / composition as
> >    we would normally do in dt.
> > 
> > IMHO 1. alone (this being Linux specific) warrants a new better
> > binding for this.
> 
> And:
> 
> 3. There already are bindings using a leds = phandle-array property in:
> Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml

This is most convincing for me. So please move this to a 
led-consumer.yaml schema first so we have exactly 1 definition of the 
property. And summarize the discussion here for why we need this.

Rob

Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation

[Aleksandrs Vinarskis]

On Thursday, September 4th, 2025 at 16:10, Rob Herring <robh@kernel.org> wrote:

> 
> 
> On Thu, Sep 04, 2025 at 02:05:08PM +0200, Hans de Goede wrote:
> 
> > Hi Krzysztof,
> > 
> > On 4-Sep-25 1:47 PM, Hans de Goede wrote:
> > 
> > > Hi Krzysztof,
> > > 
> > > On 4-Sep-25 12:47 PM, Krzysztof Kozlowski wrote:
> > > 
> > > > On 04/09/2025 12:29, Hans de Goede wrote:
> > > > 
> > > > > Hi Krzysztof,
> > > > > 
> > > > > On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
> > > > > 
> > > > > > On 04/09/2025 09:26, Hans de Goede wrote:
> > > > > > 
> > > > > > > > > > > +maintainers:
> > > > > > > > > > > + - Aleksandrs Vinarskis alex@vinarskis.com
> > > > > > > > > > > +
> > > > > > > > > > > +description:
> > > > > > > > > > > + Some LED defined in DT are required by other DT consumers, for example
> > > > > > > > > > > + v4l2 subnode may require privacy or flash LED.
> > > > > > > > > > > +
> > > > > > > > > > > + Document LED properties that its consumers may define.
> > > > > > > > > > 
> > > > > > > > > > We already have the trigger-source binding for "attaching" LEDs to
> > > > > > > > > > devices. Why does that not work here?
> > > > > > > > > 
> > > > > > > > > I have not actually considered this, as the existing privacy-led solution
> > > > > > > > > from the original series is not trigger based. At least one of the reasons
> > > > > > > > > for that is that trigger source can be rather easily altered from user
> > > > > > > > > space, which would've been bad for this use case. If v4l2 acquires control
> > > > > > > > > over the LED it actually removes triggers and disables sysfs on that LED.
> > > > > > > > 
> > > > > > > > So does that mean that v4l2 solves the problem of "trigger source can be
> > > > > > > > rather easily altered from user space"?
> > > > > > > 
> > > > > > > Yes, currently the v4l2-core already does:
> > > > > > 
> > > > > > Thanks, I understand that it solves the problem described in the patch,
> > > > > > so the patch can be dropped.
> > > > > 
> > > > > I'm a bit confused now, do you mean that this dt-bindings patch can
> > > > > be dropped ?
> > > > 
> > > > Yes.
> > > > 
> > > > Alex's explanation to Rob felt confusing, so I asked for clarification.
> > > > You clarfiied that that v4l2 solves the problem, therefore there is no
> > > > problem to be solved.
> > > > 
> > > > If there is no problem to be solved, this patch is not needed.
> > > > 
> > > > If this patch is needed, just describe the problem accurately.
> > > > 
> > > > > The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
> > > > > on DT there is no official bindings-docs for directly getting a LED with
> > > > 
> > > > There are and Rob pointed to them. If Rob's answer is not enough, make
> > > > it explicit.
> > > > 
> > > > Really, there are here some long explanations which do not really
> > > > explain this in simple terms. Simple term is: "existing property foo
> > > > does not work because <here goes the reason>".

Ill extend the commit description to better explain why the existing trigger-source
could not be re-used.

> > > 
> > > The existing trigger-source binding for "attaching" LEDs to
> > > devices does not work because:
> > > 
> > > 1. It depends on the Linux specific LED trigger mechanism where as
> > > DT should describe hw in an OS agnostic manner
> 
> 
> Using a binding does not require using the linux subsystem normally
> associated with it. Certainly the naming was inspired by the Linux
> subsystem, but it's really nothing more than a link.
> 
> > > 2. It puts the world upside down by giving possible event-sources
> > > for the (again) Linux specific trigger rather then allowing
> > > specifying e.g. specific privacy and flash LEDs as part
> > > of a camera dts node. IOW it makes the LED DT note point to
> > > the camera, while the LED is a part of the camera-module.
> > > not the other way around. So it does not properly allow
> > > describing the composition of the camera.
> 
> 
> Direction of the connection doesn't really matter. You can get the
> association either way. But certainly one way is easier than the other.
> 
> > > Note that Rob actually put "" around attaching because this
> > > property really is not proper attaching / composition as
> > > we would normally do in dt.
> > > 
> > > IMHO 1. alone (this being Linux specific) warrants a new better
> > > binding for this.
> > 
> > And:
> > 
> > 3. There already are bindings using a leds = phandle-array property in:
> > Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
> 

Another example using leds = phandle-array in:
Documentation/devicetree/bindings/leds/backlight/led-backlight.yaml

> 
> This is most convincing for me. So please move this to a
> led-consumer.yaml schema first so we have exactly 1 definition of the
> property. And summarize the discussion here for why we need this.

Just to confirm that I understood this discussion correctly:
- Keep 'led-consumer.yaml' (with improved description)
- Drop leds property from existing schemas that use it as phandle-array
- Instead reference 'led-consumer.yaml' via 'allOf'. This should also be
  added to 'video-interface-devices.yaml' as various camera schemas based
  on it typically do not allow unevaluated properties.

Thanks for the reviews,

Alex

> 
> Rob

[PATCH 2/2] leds: led-class: Add devicetree support to led_get()

[Aleksandrs Vinarskis]

From: Hans de Goede <hansg@kernel.org>

Turn of_led_get() into a more generic __of_led_get() helper function,
which can lookup LEDs in devicetree by either name or index.

And use this new helper to add devicetree support to the generic
(non devicetree specific) [devm_]led_get() function.

This uses the standard devicetree pattern of adding a -names string array
to map names to the indexes for an array of resources.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Lee Jones <lee@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
---
 drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -248,19 +248,18 @@ static const struct class leds_class = {
 	.pm = &leds_class_dev_pm_ops,
 };
 
-/**
- * of_led_get() - request a LED device via the LED framework
- * @np: device node to get the LED device from
- * @index: the index of the LED
- *
- * Returns the LED device parsed from the phandle specified in the "leds"
- * property of a device tree node or a negative error-code on failure.
- */
-static struct led_classdev *of_led_get(struct device_node *np, int index)
+static struct led_classdev *__of_led_get(struct device_node *np, int index,
+					 const char *name)
 {
 	struct device *led_dev;
 	struct device_node *led_node;
 
+	/*
+	 * For named LEDs, first look up the name in the "led-names" property.
+	 * If it cannot be found, then of_parse_phandle() will propagate the error.
+	 */
+	if (name)
+		index = of_property_match_string(np, "led-names", name);
 	led_node = of_parse_phandle(np, "leds", index);
 	if (!led_node)
 		return ERR_PTR(-ENOENT);
@@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
 	return led_module_get(led_dev);
 }
 
+/**
+ * of_led_get() - request a LED device via the LED framework
+ * @np: device node to get the LED device from
+ * @index: the index of the LED
+ *
+ * Returns the LED device parsed from the phandle specified in the "leds"
+ * property of a device tree node or a negative error-code on failure.
+ */
+struct led_classdev *of_led_get(struct device_node *np, int index)
+{
+	return __of_led_get(np, index, NULL);
+}
+EXPORT_SYMBOL_GPL(of_led_get);
+
 /**
  * led_put() - release a LED device
  * @led_cdev: LED device
@@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
 struct led_classdev *led_get(struct device *dev, char *con_id)
 {
 	struct led_lookup_data *lookup;
+	struct led_classdev *led_cdev;
 	const char *provider = NULL;
 	struct device *led_dev;
 
+	if (dev->of_node) {
+		led_cdev = __of_led_get(dev->of_node, -1, con_id);
+		if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
+			return led_cdev;
+	}
+
 	mutex_lock(&leds_lookup_lock);
 	list_for_each_entry(lookup, &leds_lookup_list, list) {
 		if (!strcmp(lookup->dev_id, dev_name(dev)) &&

-- 
2.48.1

Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()

[Hans de Goede]

Hi Aleksandrs,

Thank you for working on this.

On 2-Sep-25 1:10 PM, Aleksandrs Vinarskis wrote:
> From: Hans de Goede <hansg@kernel.org>
> 
> Turn of_led_get() into a more generic __of_led_get() helper function,
> which can lookup LEDs in devicetree by either name or index.
> 
> And use this new helper to add devicetree support to the generic
> (non devicetree specific) [devm_]led_get() function.
> 
> This uses the standard devicetree pattern of adding a -names string array
> to map names to the indexes for an array of resources.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Lee Jones <lee@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Please update this to:

Reviewed-by: Hans de Goede <hansg@kernel.org>

to match the update of the author which you already did.

Also note that checkpatch should complain about the mismatch,
please ensure to run checkpatch before posting v2.

> Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> ---
>  drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -248,19 +248,18 @@ static const struct class leds_class = {
>  	.pm = &leds_class_dev_pm_ops,
>  };
>  
> -/**
> - * of_led_get() - request a LED device via the LED framework
> - * @np: device node to get the LED device from
> - * @index: the index of the LED
> - *
> - * Returns the LED device parsed from the phandle specified in the "leds"
> - * property of a device tree node or a negative error-code on failure.
> - */
> -static struct led_classdev *of_led_get(struct device_node *np, int index)
> +static struct led_classdev *__of_led_get(struct device_node *np, int index,
> +					 const char *name)
>  {
>  	struct device *led_dev;
>  	struct device_node *led_node;
>  
> +	/*
> +	 * For named LEDs, first look up the name in the "led-names" property.
> +	 * If it cannot be found, then of_parse_phandle() will propagate the error.
> +	 */
> +	if (name)
> +		index = of_property_match_string(np, "led-names", name);
>  	led_node = of_parse_phandle(np, "leds", index);
>  	if (!led_node)
>  		return ERR_PTR(-ENOENT);
> @@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
>  	return led_module_get(led_dev);
>  }
>  
> +/**
> + * of_led_get() - request a LED device via the LED framework
> + * @np: device node to get the LED device from
> + * @index: the index of the LED
> + *
> + * Returns the LED device parsed from the phandle specified in the "leds"
> + * property of a device tree node or a negative error-code on failure.
> + */
> +struct led_classdev *of_led_get(struct device_node *np, int index)
> +{
> +	return __of_led_get(np, index, NULL);
> +}
> +EXPORT_SYMBOL_GPL(of_led_get);

I probably did this myself, but since of_led_get() is private now
(I guess it was not private before?) and since we are moving away from
"of" specific functions to using generic dev_xxxx functions in the kernel
in general, I think this just should be a static helper.

Or probably even better: just add the name argument to of_led_get()
before without renaming it, update the existing callers to pass
an extra NULL arg and completely drop this wrapper.

Also notice that adding the EXPORT_SYMBOL_GPL() while there was
none before should go hand in hand with adding a prototype to
a relevant .h file. However please just keep this private and
drop the wrapper.

Regards,

Hans




> +
>  /**
>   * led_put() - release a LED device
>   * @led_cdev: LED device
> @@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
>  struct led_classdev *led_get(struct device *dev, char *con_id)
>  {
>  	struct led_lookup_data *lookup;
> +	struct led_classdev *led_cdev;
>  	const char *provider = NULL;
>  	struct device *led_dev;
>  
> +	if (dev->of_node) {
> +		led_cdev = __of_led_get(dev->of_node, -1, con_id);
> +		if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
> +			return led_cdev;
> +	}
> +
>  	mutex_lock(&leds_lookup_lock);
>  	list_for_each_entry(lookup, &leds_lookup_list, list) {
>  		if (!strcmp(lookup->dev_id, dev_name(dev)) &&
> 

Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()

[Aleksandrs Vinarskis]

> Hi Aleksandrs,
> 
> Thank you for working on this.
> 
> On 2-Sep-25 1:10 PM, Aleksandrs Vinarskis wrote:
> > From: Hans de Goede <hansg@kernel.org>
> > 
> > Turn of_led_get() into a more generic __of_led_get() helper function,
> > which can lookup LEDs in devicetree by either name or index.
> > 
> > And use this new helper to add devicetree support to the generic
> > (non devicetree specific) [devm_]led_get() function.
> > 
> > This uses the standard devicetree pattern of adding a -names string array
> > to map names to the indexes for an array of resources.
> > 
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Reviewed-by: Lee Jones <lee@kernel.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Please update this to:
> 
> Reviewed-by: Hans de Goede <hansg@kernel.org>
> 
> to match the update of the author which you already did.
> 
> Also note that checkpatch should complain about the mismatch,
> please ensure to run checkpatch before posting v2.

Hi,

ahh, I actually did not even see that email got changed, apologies. Seems
'b4' auto-corrected it when sending, which would explain why checkpatch
did not catch it, as I run it before importing and sending via 'b4'. Sure,
will fix - did you mean to change your signoff to R-by, or is it a mistake?

> 
> > Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> > ---
> >  drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
> >  1 file changed, 29 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -248,19 +248,18 @@ static const struct class leds_class = {
> >  	.pm = &leds_class_dev_pm_ops,
> >  };
> >  
> > -/**
> > - * of_led_get() - request a LED device via the LED framework
> > - * @np: device node to get the LED device from
> > - * @index: the index of the LED
> > - *
> > - * Returns the LED device parsed from the phandle specified in the "leds"
> > - * property of a device tree node or a negative error-code on failure.
> > - */
> > -static struct led_classdev *of_led_get(struct device_node *np, int index)
> > +static struct led_classdev *__of_led_get(struct device_node *np, int index,
> > +					 const char *name)
> >  {
> >  	struct device *led_dev;
> >  	struct device_node *led_node;
> >  
> > +	/*
> > +	 * For named LEDs, first look up the name in the "led-names" property.
> > +	 * If it cannot be found, then of_parse_phandle() will propagate the error.
> > +	 */
> > +	if (name)
> > +		index = of_property_match_string(np, "led-names", name);
> >  	led_node = of_parse_phandle(np, "leds", index);
> >  	if (!led_node)
> >  		return ERR_PTR(-ENOENT);
> > @@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
> >  	return led_module_get(led_dev);
> >  }
> >  
> > +/**
> > + * of_led_get() - request a LED device via the LED framework
> > + * @np: device node to get the LED device from
> > + * @index: the index of the LED
> > + *
> > + * Returns the LED device parsed from the phandle specified in the "leds"
> > + * property of a device tree node or a negative error-code on failure.
> > + */
> > +struct led_classdev *of_led_get(struct device_node *np, int index)
> > +{
> > +	return __of_led_get(np, index, NULL);
> > +}
> > +EXPORT_SYMBOL_GPL(of_led_get);
> 
> I probably did this myself, but since of_led_get() is private now
> (I guess it was not private before?) and since we are moving away from
> "of" specific functions to using generic dev_xxxx functions in the kernel
> in general, I think this just should be a static helper.
> 
> Or probably even better: just add the name argument to of_led_get()
> before without renaming it, update the existing callers to pass
> an extra NULL arg and completely drop this wrapper.
> 

That indeed sounds like a better and cleaner option, will change it.
This way also incorporates the rest of the feedback on this series.

> Also notice that adding the EXPORT_SYMBOL_GPL() while there was
> none before should go hand in hand with adding a prototype to
> a relevant .h file. However please just keep this private and
> drop the wrapper.

Thanks for the clarification, missed that, and the review.

Alex

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > +
> >  /**
> >   * led_put() - release a LED device
> >   * @led_cdev: LED device
> > @@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
> >  struct led_classdev *led_get(struct device *dev, char *con_id)
> >  {
> >  	struct led_lookup_data *lookup;
> > +	struct led_classdev *led_cdev;
> >  	const char *provider = NULL;
> >  	struct device *led_dev;
> >  
> > +	if (dev->of_node) {
> > +		led_cdev = __of_led_get(dev->of_node, -1, con_id);
> > +		if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
> > +			return led_cdev;
> > +	}
> > +
> >  	mutex_lock(&leds_lookup_lock);
> >  	list_for_each_entry(lookup, &leds_lookup_list, list) {
> >  		if (!strcmp(lookup->dev_id, dev_name(dev)) &&
> > 

Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()

[Hans de Goede]

Hi,

On 4-Sep-25 1:01 AM, Aleksandrs Vinarskis wrote:
>> Hi Aleksandrs,
>>
>> Thank you for working on this.
>>
>> On 2-Sep-25 1:10 PM, Aleksandrs Vinarskis wrote:
>>> From: Hans de Goede <hansg@kernel.org>
>>>
>>> Turn of_led_get() into a more generic __of_led_get() helper function,
>>> which can lookup LEDs in devicetree by either name or index.
>>>
>>> And use this new helper to add devicetree support to the generic
>>> (non devicetree specific) [devm_]led_get() function.
>>>
>>> This uses the standard devicetree pattern of adding a -names string array
>>> to map names to the indexes for an array of resources.
>>>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Reviewed-by: Lee Jones <lee@kernel.org>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Please update this to:
>>
>> Reviewed-by: Hans de Goede <hansg@kernel.org>
>>
>> to match the update of the author which you already did.
>>
>> Also note that checkpatch should complain about the mismatch,
>> please ensure to run checkpatch before posting v2.
> 
> Hi,
> 
> ahh, I actually did not even see that email got changed, apologies. Seems
> 'b4' auto-corrected it when sending,

I already wondered if it might be something like that. b4 probably did
this because of the .mailmap entry mapping my Red Hat address (which
I've stopped using since I'm leaving Red Hat) to hansg@kernel.org .

> which would explain why checkpatch
> did not catch it, as I run it before importing and sending via 'b4'. Sure,
> will fix - did you mean to change your signoff to R-by, or is it a mistake?

It is a mistake please keep it as S-o-b.

> 
>>
>>> Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
>>> ---
>>>  drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
>>>  1 file changed, 29 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -248,19 +248,18 @@ static const struct class leds_class = {
>>>  	.pm = &leds_class_dev_pm_ops,
>>>  };
>>>  
>>> -/**
>>> - * of_led_get() - request a LED device via the LED framework
>>> - * @np: device node to get the LED device from
>>> - * @index: the index of the LED
>>> - *
>>> - * Returns the LED device parsed from the phandle specified in the "leds"
>>> - * property of a device tree node or a negative error-code on failure.
>>> - */
>>> -static struct led_classdev *of_led_get(struct device_node *np, int index)
>>> +static struct led_classdev *__of_led_get(struct device_node *np, int index,
>>> +					 const char *name)
>>>  {
>>>  	struct device *led_dev;
>>>  	struct device_node *led_node;
>>>  
>>> +	/*
>>> +	 * For named LEDs, first look up the name in the "led-names" property.
>>> +	 * If it cannot be found, then of_parse_phandle() will propagate the error.
>>> +	 */
>>> +	if (name)
>>> +		index = of_property_match_string(np, "led-names", name);
>>>  	led_node = of_parse_phandle(np, "leds", index);
>>>  	if (!led_node)
>>>  		return ERR_PTR(-ENOENT);
>>> @@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
>>>  	return led_module_get(led_dev);
>>>  }
>>>  
>>> +/**
>>> + * of_led_get() - request a LED device via the LED framework
>>> + * @np: device node to get the LED device from
>>> + * @index: the index of the LED
>>> + *
>>> + * Returns the LED device parsed from the phandle specified in the "leds"
>>> + * property of a device tree node or a negative error-code on failure.
>>> + */
>>> +struct led_classdev *of_led_get(struct device_node *np, int index)
>>> +{
>>> +	return __of_led_get(np, index, NULL);
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_led_get);
>>
>> I probably did this myself, but since of_led_get() is private now
>> (I guess it was not private before?) and since we are moving away from
>> "of" specific functions to using generic dev_xxxx functions in the kernel
>> in general, I think this just should be a static helper.
>>
>> Or probably even better: just add the name argument to of_led_get()
>> before without renaming it, update the existing callers to pass
>> an extra NULL arg and completely drop this wrapper.
>>
> 
> That indeed sounds like a better and cleaner option, will change it.
> This way also incorporates the rest of the feedback on this series.

Sounds good.

Regards,

Hans



>>> +
>>>  /**
>>>   * led_put() - release a LED device
>>>   * @led_cdev: LED device
>>> @@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
>>>  struct led_classdev *led_get(struct device *dev, char *con_id)
>>>  {
>>>  	struct led_lookup_data *lookup;
>>> +	struct led_classdev *led_cdev;
>>>  	const char *provider = NULL;
>>>  	struct device *led_dev;
>>>  
>>> +	if (dev->of_node) {
>>> +		led_cdev = __of_led_get(dev->of_node, -1, con_id);
>>> +		if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
>>> +			return led_cdev;
>>> +	}
>>> +
>>>  	mutex_lock(&leds_lookup_lock);
>>>  	list_for_each_entry(lookup, &leds_lookup_list, list) {
>>>  		if (!strcmp(lookup->dev_id, dev_name(dev)) &&
>>>

Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()

[Linus Walleij]

On Tue, Sep 2, 2025 at 1:10 PM Aleksandrs Vinarskis <alex@vinarskis.com> wrote:

> From: Hans de Goede <hansg@kernel.org>
>
> Turn of_led_get() into a more generic __of_led_get() helper function,
> which can lookup LEDs in devicetree by either name or index.

I don't really like __inner_function() as naming since it
is easily confused with __COMPILER_FLAGS__ and also
with __non_atomic_bitset().

I would name it of_led_get_inner() simply.

Admitted it's nitpicky, feel free to ignore this remark, my
reviewed-by holds.

Yours,
Linus Walleij

Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()

[Lee Jones]

On Wed, 03 Sep 2025, Linus Walleij wrote:

> On Tue, Sep 2, 2025 at 1:10 PM Aleksandrs Vinarskis <alex@vinarskis.com> wrote:
> 
> > From: Hans de Goede <hansg@kernel.org>
> >
> > Turn of_led_get() into a more generic __of_led_get() helper function,
> > which can lookup LEDs in devicetree by either name or index.
> 
> I don't really like __inner_function() as naming since it
> is easily confused with __COMPILER_FLAGS__ and also
> with __non_atomic_bitset().
> 
> I would name it of_led_get_inner() simply.

I'm personally okay with '__' to mean private.  There are many examples
of this already.  I'm generally less happy with '__' functions being
exported or otherwise used outside of the subsystem / core that they
reside in.

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()

[kernel test robot]

Hi Aleksandrs,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lee-leds/for-leds-next]
[also build test WARNING on linus/master v6.17-rc4 next-20250902]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Aleksandrs-Vinarskis/leds-led-class-Add-devicetree-support-to-led_get/20250902-191342
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next
patch link:    https://lore.kernel.org/r/010201990a1f6559-9e836a40-f534-4535-bd59-5e967d80559a-000000%40eu-west-1.amazonses.com
patch subject: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250903/202509031142.WBnVOXZW-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250903/202509031142.WBnVOXZW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509031142.WBnVOXZW-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/leds/led-class.c:281:22: warning: no previous prototype for 'of_led_get' [-Wmissing-prototypes]
     281 | struct led_classdev *of_led_get(struct device_node *np, int index)
         |                      ^~~~~~~~~~


vim +/of_led_get +281 drivers/leds/led-class.c

   272	
   273	/**
   274	 * of_led_get() - request a LED device via the LED framework
   275	 * @np: device node to get the LED device from
   276	 * @index: the index of the LED
   277	 *
   278	 * Returns the LED device parsed from the phandle specified in the "leds"
   279	 * property of a device tree node or a negative error-code on failure.
   280	 */
 > 281	struct led_classdev *of_led_get(struct device_node *np, int index)
   282	{
   283		return __of_led_get(np, index, NULL);
   284	}
   285	EXPORT_SYMBOL_GPL(of_led_get);
   286	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki