Message-ID: <c44a56bd-955a-4e4a-af3c-7ba754659f69@kernel.org>
Date: Tue, 14 Oct 2025 10:19:05 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jonathan Brophy <professor_jonny@...mail.com>,
 Jonathan Brophy <professorjonny98@...il.com>, lee Jones <lee@...nel.org>,
 Pavel Machek <pavel@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Radoslav Tsvetkov <rtsvetkov@...dotech.eu>
Cc: "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>
Subject: Re: [PATCH v2 1/4] dt-bindings: leds: Add YAML bindings for Virtual
 Color LED Group driver

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

Powered by blists - more mailing lists