diff mbox series

[v1,1/2] dt-bindings: leds: add binding for aw200xx

Message ID 20221124204807.1593241-2-mmkurbanov@sberdevices.ru
State New
Headers show
Series leds: add aw20xx driver | expand

Commit Message

Martin Kurbanov Nov. 24, 2022, 8:48 p.m. UTC 
Add YAML devicetree binding for AWINIC AW20036/AW20052/AW20074
led driver.

Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
---
 .../bindings/leds/leds-aw200xx.yaml           | 110 ++++++++++++++++++
 include/dt-bindings/leds/leds-aw200xx.h       |  48 ++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-aw200xx.yaml
 create mode 100644 include/dt-bindings/leds/leds-aw200xx.h

Comments

Krzysztof Kozlowski Dec. 2, 2022, 4:41 p.m. UTC | #1 
On 28/11/2022 18:43, Martin Kurbanov wrote:
> Hi. Thank you for quick reply. 
> 
> On 25.11.2022 11:29, Krzysztof Kozlowski wrote:
>>> +
>>> +  imax:
>>> +    maxItems: 1
>>> +    description:
>>> +      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h
>>
>> No. Use existing properties from common.yaml. This looks like
>> led-max-microamp and it is per LED, not per entire device.
> 
> The AW200XX LED chip does not support imax setup per led.
> Imax is the global parameter over the all leds. I suppose, it's better
> to add vendor prefix or take minimum from all subnodes?
> How do you think?

Have in mind that led-max-microamp is a required property in some cases,
so skipping it and using per-device properties does not solve the
problem of adjusting proper currents. What if each LED you set for
something which in total gives more than your imax?

> 
> 
>>> +/* Global max current (IMAX) */
>>> +#define AW200XX_IMAX_3_3MA  8
>>> +#define AW200XX_IMAX_6_7MA  9
>>
>> No. Bindings are not for storing register constants. Feel free to store
>> here IDs (ID start from 0 or 1 and is incremented by 1)... but how the
>> IMAX even matches any need for "ID"?
> 
> IMAX can be chosen from the predefined values in the
> datasheet (10mA, 20mA, etc). Do you mean the IMAX should be round down
> to nearest supported value in the driver?

What Linux driver support does not matter here. Bindings should reflect
hardware and the same time not store register constants but logical
values (for current this is in uA).

Best regards,
Krzysztof
Dmitry Rokosov Dec. 2, 2022, 6:53 p.m. UTC | #2 
Hello Krzysztof,

On Fri, Dec 02, 2022 at 05:41:37PM +0100, Krzysztof Kozlowski wrote:
> On 28/11/2022 18:43, Martin Kurbanov wrote:
> > Hi. Thank you for quick reply. 
> > 
> > On 25.11.2022 11:29, Krzysztof Kozlowski wrote:
> >>> +
> >>> +  imax:
> >>> +    maxItems: 1
> >>> +    description:
> >>> +      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h
> >>
> >> No. Use existing properties from common.yaml. This looks like
> >> led-max-microamp and it is per LED, not per entire device.
> > 
> > The AW200XX LED chip does not support imax setup per led.
> > Imax is the global parameter over the all leds. I suppose, it's better
> > to add vendor prefix or take minimum from all subnodes?
> > How do you think?
> 
> Have in mind that led-max-microamp is a required property in some cases,
> so skipping it and using per-device properties does not solve the
> problem of adjusting proper currents. What if each LED you set for
> something which in total gives more than your imax?
> 

You are right. From my point of view too, we must build our solutions from
HW capabilities. In the current situation, AW200XX chips support global
Imax value, so it's acceptable decision to use vendor prefix for global
imax parameter, why not?

...
Krzysztof Kozlowski Dec. 3, 2022, 10:44 a.m. UTC | #3 
On 02/12/2022 19:53, Dmitry Rokosov wrote:
> Hello Krzysztof,
> 
> On Fri, Dec 02, 2022 at 05:41:37PM +0100, Krzysztof Kozlowski wrote:
>> On 28/11/2022 18:43, Martin Kurbanov wrote:
>>> Hi. Thank you for quick reply. 
>>>
>>> On 25.11.2022 11:29, Krzysztof Kozlowski wrote:
>>>>> +
>>>>> +  imax:
>>>>> +    maxItems: 1
>>>>> +    description:
>>>>> +      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h
>>>>
>>>> No. Use existing properties from common.yaml. This looks like
>>>> led-max-microamp and it is per LED, not per entire device.
>>>
>>> The AW200XX LED chip does not support imax setup per led.
>>> Imax is the global parameter over the all leds. I suppose, it's better
>>> to add vendor prefix or take minimum from all subnodes?
>>> How do you think?
>>
>> Have in mind that led-max-microamp is a required property in some cases,
>> so skipping it and using per-device properties does not solve the
>> problem of adjusting proper currents. What if each LED you set for
>> something which in total gives more than your imax?
>>
> 
> You are right. From my point of view too, we must build our solutions from
> HW capabilities. 

And there was no proposal to go around HW capabilities. We talk only
about representation.

> In the current situation, AW200XX chips support global
> Imax value, so it's acceptable decision to use vendor prefix for global
> imax parameter, why not?

Jacek made his statement some time ago quite clear:

https://lore.kernel.org/all/5785F17D.3010108@samsung.com/

"If you question the idea of having different maximum brightness per
sub-LEDs controlled by the same device, then it means that you have
objections to the entire idea of LED subsystem max_brightness property,
whereas it has been broadly accepted and successfully used for years."

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-aw200xx.yaml b/Documentation/devicetree/bindings/leds/leds-aw200xx.yaml
new file mode 100644
index 000000000000..3bdadcbc2ee2
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-aw200xx.yaml
@@ -0,0 +1,110 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-aw200xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AWINIC AW200XX LED Driver
+
+maintainers:
+  - Martin Kurbanov <mmkurbanov@sberdevices.ru>
+
+description: |
+  This controller is present on AW20036/AW20054/AW20072.
+  It is a 3x12/6x9/6x12 matrix LED driver programmed via
+  an I2C interface, up to 36/54/72 LEDs or 12/18/24 RGBs,
+  3 pattern controllers for auto breathing or group dimming control.
+
+  For more product information please see the link below:
+  aw20036 - https://www.awinic.com/Public/Uploads/uploadfile/files/20200509/20200509151532_5eb65894d205a.pdf
+  aw20054 - https://www.awinic.com/Public/Uploads/uploadfile/files/20200509/20200509151602_5eb658b2b77cb.pdf
+  aw20072 - https://www.awinic.com/Public/Uploads/uploadfile/files/20200509/20200509151754_5eb659227a145.pdf
+
+properties:
+  compatible:
+    enum:
+      - awinic,aw20036
+      - awinic,aw20054
+      - awinic,aw20072
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  interrupts:
+    maxItems: 1
+
+  display-size:
+    maxItems: 1
+    description:
+      Leds matrix size, see dt-bindings/leds/leds-aw200xx.h
+
+  imax:
+    maxItems: 1
+    description:
+      Maximum supply current, see dt-bindings/leds/leds-aw200xx.h
+
+patternProperties:
+  "^led@[0-9a-f]$":
+    type: object
+    $ref: common.yaml#
+
+    properties:
+      reg:
+        description:
+          LED number
+        maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - display-size
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/leds/common.h>
+    #include <dt-bindings/leds/leds-aw200xx.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@3a {
+            compatible = "awinic,aw20036";
+            reg = <0x3a>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            interrupt-parent = <&gpio_intc>;
+            interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
+
+            display-size = <AW20036_DSIZE_3X12>;
+            imax = <AW200XX_IMAX_60MA>;
+
+            led@0 {
+                reg = <0x0>;
+                color = <LED_COLOR_ID_RED>;
+            };
+
+            led@1 {
+                reg = <0x1>;
+                color = <LED_COLOR_ID_GREEN>;
+            };
+
+            led@2 {
+                reg = <0x2>;
+                color = <LED_COLOR_ID_BLUE>;
+            };
+        };
+    };
+
+...
diff --git a/include/dt-bindings/leds/leds-aw200xx.h b/include/dt-bindings/leds/leds-aw200xx.h
new file mode 100644
index 000000000000..6b2ba4c3c6b1
--- /dev/null
+++ b/include/dt-bindings/leds/leds-aw200xx.h
@@ -0,0 +1,48 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/**
+ * This header provides constants for aw200xx LED bindings.
+ *
+ * Copyright (c) 2022, SberDevices. All Rights Reserved.
+ *
+ * Author: Martin Kurbanov <mmkurbanov@sberdevices.ru>
+ */
+#ifndef _DT_BINDINGS_LEDS_AW200XX_H
+#define _DT_BINDINGS_LEDS_AW200XX_H
+
+/* Global max current (IMAX) */
+#define AW200XX_IMAX_3_3MA  8
+#define AW200XX_IMAX_6_7MA  9
+#define AW200XX_IMAX_10MA   0
+#define AW200XX_IMAX_13_3MA 11
+#define AW200XX_IMAX_20MA   1
+#define AW200XX_IMAX_26_7MA 13
+#define AW200XX_IMAX_30MA   2
+#define AW200XX_IMAX_40MA   3
+#define AW200XX_IMAX_53_3MA 15
+#define AW200XX_IMAX_60MA   4
+#define AW200XX_IMAX_80MA   5
+#define AW200XX_IMAX_120MA  6
+#define AW200XX_IMAX_160MA  7
+
+/* Display size for aw20036 */
+#define AW20036_DSIZE_1X12 0
+#define AW20036_DSIZE_2X12 1
+#define AW20036_DSIZE_3X12 2
+
+/* Display size for aw20054 */
+#define AW20054_DSIZE_1X9 0
+#define AW20054_DSIZE_2X9 1
+#define AW20054_DSIZE_3X9 2
+#define AW20054_DSIZE_4X9 3
+#define AW20054_DSIZE_5X9 4
+#define AW20054_DSIZE_6X9 5
+
+/* Display size for aw20072 */
+#define AW20072_DSIZE_1X12 0
+#define AW20072_DSIZE_2X12 1
+#define AW20072_DSIZE_3X12 2
+#define AW20072_DSIZE_4X12 3
+#define AW20072_DSIZE_5X12 4
+#define AW20072_DSIZE_6X12 5
+
+#endif /* !_DT_BINDINGS_LEDS_AW200XX_H */