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
