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

[Jonathan Brophy <professor_jonny@hotmail.com>]

on  14/10/ 12:42, Krzysztof Kozlowski wrote:

>> From: Jonathan Brophy <professor_jonny@hotmail.com>
>> 
>> Document Virtual Color device tree bindings.
>
>I don't see how you answered my comment about missing justification.
>
>Rob's questions also were not answered.

Ok I kind of justified the inclusion of the driver in the cover letter commit message, as for the multi-led binding I have done
the same thing I will make changes.

Sorry It’s a big learning curve for me and I may have put my justification in the wrong place.


>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?
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,
At the moment the blink settings are unique to this driver I went this way as we were trying to get specific behaviour as the
native timing functions did not work as intended, but I'm looking at replacing it with standard functions if I can get it to
work.


Best regards,
Jonathan