| Message ID | ZniNdGgKyUMV-hjq@admins-Air |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] dt-bindings: leds: Add LED1202 LED Controller | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success |
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 > > 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? Second this was never tested. Third, where did you give me any chance to reply to your comment before posting new version? Best regards, Krzysztof
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. 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 >
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. > 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
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
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>; + }; + + led@1 { + reg = <1>; + label = "led2"; + function = LED_FUNCTION_STATUS; + color = <LED_COLOR_ID_GREEN>; + function-enumerator = <2>; + active = <1>; + }; + + led@2 { + reg = <2>; + label = "led3"; + function = LED_FUNCTION_STATUS; + color = <LED_COLOR_ID_BLUE>; + function-enumerator = <3>; + active = <1>; + }; + + led@3 { + reg = <3>; + label = "led4"; + function = LED_FUNCTION_STATUS; + color = <LED_COLOR_ID_RED>; + function-enumerator = <4>; + active = <1>; + }; + + led@4 { + reg = <4>; + label = "led5"; + function = LED_FUNCTION_STATUS; + color = <LED_COLOR_ID_GREEN>; + function-enumerator = <5>; + active = <1>; + }; + + led@5 { + reg = <5>; + label = "led6"; + function = LED_FUNCTION_STATUS; + color = <LED_COLOR_ID_BLUE>; + function-enumerator = <6>; + active = <1>; + }; + + led@6 { + reg = <6>; + label = "led7"; + function = LED_FUNCTION_STATUS; + color = <LED_COLOR_ID_RED>; + function-enumerator = <7>; + active = <1>; + }; + + led@7 { + reg = <7>; + label = "led8"; + function = LED_FUNCTION_STATUS; + color = <LED_COLOR_ID_GREEN>; + function-enumerator = <8>; + active = <1>; + }; + + led@8 { + reg = <8>; + label = "led9"; + function = LED_FUNCTION_STATUS; + color = <LED_COLOR_ID_BLUE>; + function-enumerator = <9>; + active = <1>; + }; + + led@9 { + reg = <9>; + active = <0>; + }; + + led@a { + reg = <10>; + active = <0>; + }; + + led@b { + reg = <11>; + active = <0>; + }; + }; + }; + +...
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 .../devicetree/bindings/leds/st,led1202.yml | 162 ++++++++++++++++++ 1 file changed, 162 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/st,led1202.yml