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: <ZnlvOuvMQmJFrfSX@admins-Air>
Date: Mon, 24 Jun 2024 15:06:02 +0200
From: Vicentiu Galanopulo <vicentiu.galanopulo@...ote-tech.co.uk>
To: 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 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.
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?

 
> 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.
I've been replying to your emails in the mutt client, but sending the patches with
mutt -H.
But the changes you mentioned related to function on color, I don't know what should have happend there.. 
I sent a v2 with the changes you indicated.

Thanks,
Vicentiu

> 
> Best regards,
> Krzysztof
>