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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Sat, 11 Mar 2023 10:06:34 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Bogdan Ionescu <bogdan.ionescu.work@...il.com>
Cc:     Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-leds@...r.kernel.or, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: leds: Document rohm,bd65b60 bindings

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