Re: [PATCH v2 1/4] dt-bindings: leds: Add YAML bindings for Virtual Color LED Group driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 14/10/2025 05:08, Jonathan Brophy wrote:
>> Few minor things follow up, but considering missing reasoning I did not perform full review.
>>
>> A nit, subject: drop second/last, redundant "YAML bindings for". The "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>>
>> ... and driver. Again - explain the hardware. Bindings are not for driver.
> 
> I'm kind a little bit confused what you mean by this statement.
> 
> I'm guessing I should omit hardware info in the class yaml and move it to a group yaml like the multicolor ones as below?


I speak about the subject.

> If so that is just a mistake on my part not knowing the file structure well.
> 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml
> https://elixir.bootlin.com/linux/v6.17.1/source/Documentation/devicetree/bindings/leds/leds-group-multicolor.yaml
> 
>>>
>>> +description: |
>>> +  Bindings to show how to achieve logically grouped virtual LEDs.
>>> +  The nodes and properties defined in this document are unique to the
>>> +  virtualcolor LED class.
>>
>> That's completely redundant statement.
> 
> Ok fair enough, but I basically cloned this comment from the leds-group-multicolor as they have something simular.
> 
>>> +  Common LED nodes and properties are inherited from the common.yaml  
>>> + within this documentation directory
>>
>> As well drop. Your description is pretty obvious and does not help at all.
> 
> Ok thanks
> 
>>> +    properties:
>>> +      reg:
>>> +        maxItems: 1
>>> +        description: Virtual LED number
>>> +
>>> +      leds:
>>> +        $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +        description: List of phandles to the monochromatic LEDs to 
>>> + group
>>> +
>>> +      function:
>>> +        description: |
>>> +          For virtualcolor LEDs this property should be defined as
>>> +          LED_FUNCTION_VIRTUAL_STATUS as outlined in:
>>> +          include/dt-bindings/leds/common.h.
>>> +
>>> +      priority:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        description: Priority level for LED activation
>>> +          (higher value means higher priority)
>>> +
>>> +      blink-delay-on:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        description: Time in milliseconds the LED is on during blink
>>> +
>>> +      blink-delay-off:
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        description: Time in milliseconds the LED is off during blink
>>> +        note: Setting just one of the blink delays to a valid value while
>>> +          setting the other to null will cause the LED to operate with a one-shot
>>> +          on or off delay instead of a repeat cycle.
>>
>>
>> And drop all above, except reg and leds. If these are new properties, then you need to use proper unit suffixes.
>>
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> Thanks for pointing this out I guessed there was a definition's somewhere,


You reference common LED bindings, so no need to duplicate properties
from there.


Best regards,
Krzysztof