Re: [PATCH 1/2] dt-bindings: leds: Document rohm,bd65b60 bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 10/03/2023 11:41, Bogdan Ionescu wrote:
> Thanks for the feedback, I am working on addressing the issues you have
> raised. If I haven't mentioned something, please assume I am implementing
> your suggestion.
> 
>>> +    properties:
>>> +      rohm,enable-outputs:
>>
>> I don't understand why do you need this property. You should use
>> generic/existing properties, if possible.
> 
>> Drop led object. There is no real need for it, is it?
> 
> Would this structure be more appropriate:
> 
> + led-controler@64 {
> +     compatible = "rohm,bd65b60";
> +     reg = <0x64>;
> +     default-state = "keep";
> +     linux,default-trigger = "backlight";
> +
> +     led@1 {
> +         reg = <1>;
> +         function = LED_FUNCTION_BACKLIGHT;
> +         color = <LED_COLOR_ID_WHITE>;
> +     };
> +
> +     led@2 {
> +         reg = <2>;
> +         function = LED_FUNCTION_BACKLIGHT;
> +         color = <LED_COLOR_ID_WHITE>;
> +     };
> + };
> 
> Can I have 2 led nodes if they share the same brightness?

I think yes, but what about LED on/off? Is it also shared or separate?
Because if it is shared, then basically it is just one LED...

> 
>>> +        enum: [ 0, 8, 16 ]
>>> +        default: 16
>>
>> What are the units? percent? Volts? Then use unit suffix in property
>> name. Your other file suggests volts so make it microvolts.
> 
> The chip has 3 possible values 25V, 30V and 35V. How should the driver
>  handle a value that is not exactly that?
> Should it round down for safety or return an error?

Then use -microvolt and real units
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml


> 
> This was the reason for creating the bindings header file.
> Is it possible to reference defines from the bindings header file?
> 
> + enum:
> +   - BD65B60_OVP_25V
> +   - BD65B60_OVP_30V
> +   - BD65B60_OVP_35V
> + default: BD65B60_OVP_35V
> 
> I would prefer it if I didn't have to expose the weird bitmasks that ROHM
> have decided to use and instead use the actual values.
> 
> Apologies for the long email, but I wanted to make sure I was clear.




Best regards,
Krzysztof