linux-kernel - Re: [PATCH v2 1/2] dt-bindings: leds: Add LED1202 LED Controller

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2b01874f-26e9-41a1-84c0-9a2ed15cb630@kernel.org>
Date: Mon, 24 Jun 2024 15:08:56 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Vicentiu Galanopulo <vicentiu.galanopulo@...ote-tech.co.uk>,
 pavel@....cz, lee@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] dt-bindings: leds: Add LED1202 LED Controller

On 24/06/2024 15:06, Vicentiu Galanopulo wrote:
> On Mon, Jun 24, 2024 at 07:02:12AM +0200, Krzysztof Kozlowski wrote:
>> On 23/06/2024 23:02, Vicentiu Galanopulo wrote:
>>> The LED1202 is a 12-channel low quiescent current LED driver with:
>>>   * Supply range from 2.6 V to 5 V
>>>   * 20 mA current capability per channel
>>>   * 1.8 V compatible I2C control interface
>>>   * 8-bit analog dimming individual control
>>>   * 12-bit local PWM resolution
>>>   * 8 programmable patterns
>>>
>>> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@...ote-tech.co.uk>
>>> ---
>>>
>>> Changes in v2:
>>>   - renamed label to remove color from it
>>>   - add color property for each node
>>>   - add function and function-enumerator property for each node
>>
>> Fix your email setup, because your broken or non-existing threading
>> messes with review process. See:
>>
>> b4 diff '<ZniNdGgKyUMV-hjq@...ins-Air>'
>> Grabbing thread from
>> lore.kernel.org/all/ZniNdGgKyUMV-hjq@...ins-Air/t.mbox.gz
>> Checking for older revisions
>> Grabbing search results from lore.kernel.org
>>   Added from v1: 1 patches
>> ---
>> Analyzing 3 messages in the thread
>> Looking for additional code-review trailers on lore.kernel.org
>> Preparing fake-am for v1: dt-bindings: leds: Add LED1202 LED Controller
>> ERROR: v1 series incomplete; unable to create a fake-am range
>> ---
>> Could not create fake-am range for lower series v1
>>
>>
>>>
>>>  .../devicetree/bindings/leds/st,led1202.yml   | 162 ++++++++++++++++++
>>>  1 file changed, 162 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/leds/st,led1202.yml
>>
>> yaml, not yml
> ok, will change
>>
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/st,led1202.yml b/Documentation/devicetree/bindings/leds/st,led1202.yml
>>> new file mode 100644
>>> index 000000000000..1484b09c8eeb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/st,led1202.yml
>>> @@ -0,0 +1,162 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/leds/st,led1202.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: ST LED1202 LED controllers
>>> +
>>> +maintainers:
>>> +  - Vicentiu Galanopulo <vicentiu.galanopulo@...ote-tech.co.uk>
>>> +
>>> +description:
>>> +  The LED1202 is a 12-channel low quiescent current LED controller
>>> +  programmable via I2C; The output current can be adjusted separately
>>> +  for each channel by 8-bit analog and 12-bit digital dimming control.
>>> +
>>> +  Datasheet available at
>>> +  https://www.st.com/en/power-management/led1202.html
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - st,led1202
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 0
>>> +
>>> +patternProperties:
>>> +  "^led@[0-9a-f]+$":
>>> +    type: object
>>> +    $ref: common.yaml#
>>> +    unevaluatedProperties: false
>>> +
>>> +    properties:
>>> +      reg:
>>> +        minimum: 0
>>> +        maximum: 11
>>> +
>>> +    required:
>>> +      - reg
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/leds/common.h>
>>> +
>>> +    i2c {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        led-controller@58 {
>>> +            compatible = "st,led1202";
>>> +            reg = <0x58>;
>>> +            address-cells = <1>;
>>> +            size-cells = <0>;
>>> +
>>> +            led@0 {
>>> +                reg = <0>;
>>> +                label = "led1";
>>> +                function = LED_FUNCTION_STATUS;
>>> +                color = <LED_COLOR_ID_RED>;
>>> +                function-enumerator = <1>;
>>> +                active = <1>;
>>
>> This did not improve. First, which binding defines this field?
>>
> it's a new field I added, but if you would like for me to use another
> please advise.

Look at the LED bindings. Anyway, you cannot sprinkle new properties to
some nodes without defining them in the bindings.

> Depending on this value, the enabled/disabled bit is set in the
> appropriate register, and the led appears with the label name in sysfs.
> Hope this extra info helps in helping me pick the appropiate binding. 
> 
>> Second this was never tested.
>>
> are you referring to the automated test done by the kernel test robot?

No, your testing. See writing-schema doc.

> 
>  
>> Third, where did you give me any chance to reply to your comment before
>> posting new version?
>>
> I think I have a wrong understanding of the process or mutt client is missconfigured
> or missued on my side.

Sending new version of patchset without allowing me to respond is not
"mutt misconfiguration".

Best regards,
Krzysztof