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

[Vicentiu Galanopulo <vicentiu.galanopulo@remote-tech.co.uk>]

On Mon, Jun 24, 2024 at 03:08:56PM +0200, Krzysztof Kozlowski wrote:
> 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@remote-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@admins-Air>'
> >> Grabbing thread from
> >> lore.kernel.org/all/ZniNdGgKyUMV-hjq@admins-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@remote-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.
>
Ok, will do
 
> > 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.
> 
Thanks for pointing this out, will look.
> > 
> >  
> >> 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".
> 
I'm insisting with this just to be sure that I'm not using mutt any other way than
I should. I've configured threading, but I don't know how it shows on your end.
So:
I've sent v1 -> you said change color and function and asked about active ->
I've send v2 with color and function *changed*, and contiuned the conversation in the
 email thread about active.

Expectation was that the active stuff was to be decided and then a v2 should
have comed from me?

If yes, this is the "process" part I was previously referring to, that I don't know.
From my point of view, the outcome is the same, function and color needed to be changed anyway. 

> Best regards,
> Krzysztof
> 

Kind regards,
Vicentiu