[PATCH v2 0/2] leds: add aw20xx driver

[PATCH v2 0/2] leds: add aw20xx driver

[Martin Kurbanov]

This patch series adds support for AWINIC AW20036/AW20054/AW20072 LED
driver programmed via an I2C interface.

This driver supports following AW200XX features:
  - Individual 64-level DIM currents

Datasheet:
  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

Add YAML dt-binding schema for AW200XX.

Changelog:
 v1 -> v2:
  - Remove the hardware pattern support (I will send a separate patch)
  - Support the 'led-max-microamp' property

Martin Kurbanov (2):
  dt-bindings: leds: add binding for aw200xx
  leds: add aw20xx driver

 .../testing/sysfs-class-led-driver-aw200xx    |   4 +
 .../bindings/leds/awinic,aw200xx.yaml         | 126 ++++
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-aw200xx.c                   | 649 ++++++++++++++++++
 5 files changed, 790 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-aw200xx
 create mode 100644 Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
 create mode 100644 drivers/leds/leds-aw200xx.c

--
2.38.1

[PATCH v2 1/2] dt-bindings: leds: add binding for aw200xx

[Martin Kurbanov]

Add YAML devicetree binding for AWINIC AW20036/AW20052/AW20074
led driver.

Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
---
 .../bindings/leds/awinic,aw200xx.yaml         | 126 ++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
new file mode 100644
index 000000000000..08181703e223
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -0,0 +1,126 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/awinic,aw200xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AWINIC AW200XX LED
+
+maintainers:
+  - Martin Kurbanov <mmkurbanov@sberdevices.ru>
+
+description: |
+  This controller is present on AW20036/AW20054/AW20072.
+  It is a 3x12/6x9/6x12 matrix LED 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
+
+  awinic,display-rows:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Leds matrix size
+
+patternProperties:
+  "^led@[0-9a-f]$":
+    type: object
+    $ref: common.yaml#
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        description:
+          LED number
+        maxItems: 1
+
+      led-max-microamp:
+        default: 9780
+        description: |
+          Note that a driver will take the minimum of all LED limits
+          since the chip has a single global setting.
+          The maximum output current of each LED is calculated by the
+          following formula:
+            IMAXled = 160000 * (592 / 600.5) * (1 / display-rows)
+          And the minimum output current formula:
+            IMINled = 3300 * (592 / 600.5) * (1 / display-rows)
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - awinic,display-rows
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: awinic,aw20036
+    then:
+      properties:
+        awinic,display-rows:
+          enum: [1, 2, 3]
+    else:
+      properties:
+        awinic,display-rows:
+          enum: [1, 2, 3, 4, 5, 6, 7]
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@3a {
+            compatible = "awinic,aw20036";
+            reg = <0x3a>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            awinic,display-rows = <3>;
+
+            led@0 {
+                reg = <0x0>;
+                color = <LED_COLOR_ID_RED>;
+                led-max-microamp = <9780>;
+            };
+
+            led@1 {
+                reg = <0x1>;
+                color = <LED_COLOR_ID_GREEN>;
+                led-max-microamp = <9780>;
+            };
+
+            led@2 {
+                reg = <0x2>;
+                color = <LED_COLOR_ID_BLUE>;
+                led-max-microamp = <9780>;
+            };
+        };
+    };
+
+...
-- 
2.38.1

Re: [PATCH v2 1/2] dt-bindings: leds: add binding for aw200xx

[Krzysztof Kozlowski]

On 28/02/2023 22:10, Martin Kurbanov wrote:
> Add YAML devicetree binding for AWINIC AW20036/AW20052/AW20074
> led driver.
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> ---
>  .../bindings/leds/awinic,aw200xx.yaml         | 126 ++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> new file mode 100644
> index 000000000000..08181703e223
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> @@ -0,0 +1,126 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/awinic,aw200xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AWINIC AW200XX LED
> +
> +maintainers:
> +  - Martin Kurbanov <mmkurbanov@sberdevices.ru>
> +
> +description: |
> +  This controller is present on AW20036/AW20054/AW20072.
> +  It is a 3x12/6x9/6x12 matrix LED 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

Links do not work. Error 401

> +
> +properties:
> +  compatible:
> +    enum:
> +      - awinic,aw20036
> +      - awinic,aw20054
> +      - awinic,aw20072
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  awinic,display-rows:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Leds matrix size

Why do you need this property? Number of LEDs are known from the number
of children. Matrix size is fixed in compatible, isn't it?

> +
> +patternProperties:
> +  "^led@[0-9a-f]$":
> +    type: object
> +    $ref: common.yaml#
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        description:
> +          LED number
> +        maxItems: 1
> +
> +      led-max-microamp:
> +        default: 9780
> +        description: |
> +          Note that a driver will take the minimum of all LED limits
> +          since the chip has a single global setting.
> +          The maximum output current of each LED is calculated by the
> +          following formula:
> +            IMAXled = 160000 * (592 / 600.5) * (1 / display-rows)
> +          And the minimum output current formula:
> +            IMINled = 3300 * (592 / 600.5) * (1 / display-rows)
> +


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: leds: add binding for aw200xx

[Martin Kurbanov]

On 2023-03-02 10:48, Krzysztof Kozlowski wrote:

>> +  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
> 
> Links do not work. Error 401

They are changed the links. I will correct in the next version.

>> +  awinic,display-rows:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Leds matrix size
> 
> Why do you need this property? Number of LEDs are known from the number
> of children. Matrix size is fixed in compatible, isn't it?

Number of LEDs are known, but matrix size are programmable.
Example for the aw20036, the matrix size can be 1x12, 2x12, 3x12.

-- 
Best Regards,
Kurbanov Martin

Re: [PATCH v2 1/2] dt-bindings: leds: add binding for aw200xx

[Krzysztof Kozlowski]

On 02/03/2023 17:02, Martin Kurbanov wrote:
> On 2023-03-02 10:48, Krzysztof Kozlowski wrote:
> 
>>> +  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
>>
>> Links do not work. Error 401
> 
> They are changed the links. I will correct in the next version.
> 
>>> +  awinic,display-rows:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Leds matrix size
>>
>> Why do you need this property? Number of LEDs are known from the number
>> of children. Matrix size is fixed in compatible, isn't it?
> 
> Number of LEDs are known, but matrix size are programmable.
> Example for the aw20036, the matrix size can be 1x12, 2x12, 3x12.

Ah ok, makes sense.

Best regards,
Krzysztof

[PATCH v2 2/2] leds: add aw20xx driver

[Martin Kurbanov]

This commit adds support for AWINIC AW20036/AW20054/AW20072 LED driver.
This driver supports following AW200XX features:
  - Individual 64-level DIM currents

Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
---
 .../testing/sysfs-class-led-driver-aw200xx    |   4 +
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-aw200xx.c                   | 649 ++++++++++++++++++
 4 files changed, 664 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-aw200xx
 create mode 100644 drivers/leds/leds-aw200xx.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-aw200xx b/Documentation/ABI/testing/sysfs-class-led-driver-aw200xx
new file mode 100644
index 000000000000..09a8247c0bf7
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-aw200xx
@@ -0,0 +1,4 @@
+What:		/sys/class/leds/<led>/dim
+Date:		February 2023
+Description: 	64-level DIM current. If write negative value or "auto",
+		the dim will be calculated according to the brightness.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 499d0f215a8b..66e136f43870 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -94,6 +94,16 @@ config LEDS_ARIEL
 
 	  Say Y to if your machine is a Dell Wyse 3020 thin client.
 
+config LEDS_AW200XX
+	tristate "LED support for Awinic AW20036/AW20054/AW20072"
+	depends on LEDS_CLASS
+	depends on I2C
+	help
+	  This option enables support for the AW20036/AW20054/AW20072 LED driver.
+	  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.
+
 config LEDS_AW2013
 	tristate "LED support for Awinic AW2013"
 	depends on LEDS_CLASS && I2C && OF
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4fd2f92cd198..f611e48cd3f5 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_AN30259A)		+= leds-an30259a.o
 obj-$(CONFIG_LEDS_APU)			+= leds-apu.o
 obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
 obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
+obj-$(CONFIG_LEDS_AW200XX)		+= leds-aw200xx.o
 obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
new file mode 100644
index 000000000000..dd26993192bc
--- /dev/null
+++ b/drivers/leds/leds-aw200xx.c
@@ -0,0 +1,649 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * leds-aw200xx.c - Awinic AW20036/AW20054/AW20072 LED driver
+ *
+ * Copyright (c) 2023, SberDevices. All Rights Reserved.
+ *
+ * Author: Martin Kurbanov <mmkurbanov@sberdevices.ru>
+ */
+
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/bitfield.h>
+
+#define AW200XX_LEDS_MAX                 72
+#define AW200XX_PATTERN_MAX              3
+#define AW200XX_DIM_MAX                  0x3F
+#define AW200XX_FADE_MAX                 0xFF
+#define AW200XX_IMAX_DEFAULT_MICROAMP    60000
+#define AW200XX_IMAX_MAX_MICROAMP        160000
+#define AW200XX_IMAX_MIN_MICROAMP        3300
+
+/* Page 0 */
+#define AW200XX_REG_PAGE0_BASE 0xc000
+
+/* Select page register */
+#define AW200XX_REG_PAGE       0xF0
+#define AW200XX_PAGE_MASK      (GENMASK(7, 6) | GENMASK(2, 0))
+#define AW200XX_PAGE_SHIFT     0
+#define AW200XX_NUM_PAGES      6
+#define AW200XX_PAGE_SIZE      256
+#define AW200XX_REG(page, reg) \
+	(AW200XX_REG_PAGE0_BASE + ((page) * AW200XX_PAGE_SIZE) + (reg))
+#define AW200XX_REG_MAX \
+	AW200XX_REG(AW200XX_NUM_PAGES - 1, AW200XX_PAGE_SIZE - 1)
+#define AW200XX_PAGE0 0
+#define AW200XX_PAGE1 1
+#define AW200XX_PAGE2 2
+#define AW200XX_PAGE3 3
+#define AW200XX_PAGE4 4
+#define AW200XX_PAGE5 5
+
+/* Chip ID register */
+#define AW200XX_REG_IDR       AW200XX_REG(AW200XX_PAGE0, 0x00)
+#define AW200XX_IDR_CHIPID    0x18
+
+/* Sleep mode register */
+#define AW200XX_REG_SLPCR     AW200XX_REG(AW200XX_PAGE0, 0x01)
+#define AW200XX_SLPCR_ACTIVE  0x00
+
+/* Reset register */
+#define AW200XX_REG_RSTR      AW200XX_REG(AW200XX_PAGE0, 0x02)
+#define AW200XX_RSTR_RESET    0x01
+
+/* Global current configuration register */
+#define AW200XX_REG_GCCR        AW200XX_REG(AW200XX_PAGE0, 0x03)
+#define AW200XX_GCCR_IMAX_MASK  GENMASK(7, 4)
+#define AW200XX_GCCR_IMAX(x)    ((x) << 4)
+#define AW200XX_GCCR_ALLON      BIT(3)
+
+/* Fast clear display control register */
+#define AW200XX_REG_FCD       AW200XX_REG(AW200XX_PAGE0, 0x04)
+#define AW200XX_FCD_CLEAR     0x01
+
+/* Interrupt status register */
+#define AW200XX_REG_ISRFLT          AW200XX_REG(AW200XX_PAGE0, 0x0B)
+#define AW200XX_ISRFLT_PATIS_MASK   GENMASK(6, 4)
+
+/* Pattern enable control register */
+#define AW200XX_REG_PATCR           AW200XX_REG(AW200XX_PAGE0, 0x43)
+#define AW200XX_PATCR_PAT_IE_MASK   GENMASK(6, 4)
+#define AW200XX_PATCR_PAT_IE_ALL    AW200XX_PATCR_PAT_IE_MASK
+#define AW200XX_PATCR_PAT_ENABLE(x) BIT(x)
+
+/*
+ * Maximum breathing level registers
+ * For patterns 0 - 0x44, 1 - 0x45, 2 - 0x46 (step 1)
+ */
+#define AW200XX_REG_PAT0_MAX_BREATH AW200XX_REG(AW200XX_PAGE0, 0x44)
+
+/*
+ * Minimum breathing level registers
+ * For patterns 0 - 0x47, 1 - 0x48, 2 - 0x49 (step 1)
+ */
+#define AW200XX_REG_PAT0_MIN_BREATH AW200XX_REG(AW200XX_PAGE0, 0x47)
+
+/*
+ * Template 1 (rise-time) & template 2 (on-time) configuration register
+ * For patterns 0 - 0x4A, 1 - 0x4E, 2 - 0x52 (step 4)
+ */
+#define AW200XX_REG_PAT0_T0 AW200XX_REG(AW200XX_PAGE0, 0x4A)
+
+/*
+ * Template 3 (fall-time) & template 4 (off-time) configuration register
+ * For patterns 0 - 0x4B, 1 - 0x4F, 2 - 0x53 (step 4)
+ */
+#define AW200XX_REG_PAT0_T1 AW200XX_REG(AW200XX_PAGE0, 0x4B)
+
+/*
+ * Loop configuration registers:
+ *   loop end point setting (LE)
+ *   loop beginning point setting (LB)
+ *   MSB of loop times (LT)
+ * For patterns 0 - 0x4C, 1 - 0x50, 2 - 0x54 (step 4)
+ */
+#define AW200XX_REG_PAT0_T2     AW200XX_REG(AW200XX_PAGE0, 0x4C)
+#define AW200XX_REG_PATX_T2(x) (AW200XX_REG_PAT0_T2 + (x))
+
+/*
+ * Loop configuration registers:
+ *    LSB of loop times (LT)
+ * For patterns 0 - 0x4D, 1 - 0x51, 2 - 0x55 (step 4)
+ */
+#define AW200XX_REG_PAT0_T3    AW200XX_REG(AW200XX_PAGE0, 0x4D)
+#define AW200XX_REG_PATX_T3(x) (AW200XX_REG_PAT0_T3 + (x))
+
+#define AW200XX_PAT_T2_LE_MASK      GENMASK(7, 6)
+#define AW200XX_PAT_T2_LB_MASK      GENMASK(5, 4)
+#define AW200XX_PAT_T2_LT_MASK      GENMASK(3, 0)
+#define AW200XX_PAT_T3_LT_MASK      0xFF
+#define AW200XX_PAT0_T2_LT_MSB(x)   ((x) >> 8)
+#define AW200XX_PAT0_T3_LT_LSB(x)   ((x) & 0xFF)
+#define AW200XX_PAT0_T_LT(msb, lsb) ((msb) << 8 | (lsb))
+#define AW200XX_PAT0_T_LT_MAX       0xFFF
+
+#define AW200XX_PAT_T_STEP          4
+
+#define AW200XX_PAT_T1_T3_MASK      0xF0
+#define AW200XX_PAT_T2_T4_MASK      0x0F
+#define AW200XX_TEMPLATE_TIME_MAX   0x0F
+
+/*
+ * Pattern mode configuration register
+ * For patterns 0 - 0x56, 1 - 0x57, 2 - 0x58 (step 1)
+ */
+#define AW200XX_REG_PAT0_CFG        AW200XX_REG(AW200XX_PAGE0, 0x56)
+#define AW200XX_PAT_CFG_MODE_MASK   BIT(0)
+#define AW200XX_PAT_CFG_RAMP_MASK   BIT(1)
+#define AW200XX_PAT_CFG_SWITCH_MASK BIT(2)
+
+/* Start pattern register */
+#define AW200XX_REG_PATGO           AW200XX_REG(AW200XX_PAGE0, 0x59)
+#define AW200XX_PATGO(x)            BIT(x)
+#define AW200XX_PATGO_RUN(x, run)   ((run) << (x))
+#define AW200XX_PATGO_STATE(x)      BIT((x) + 4)
+
+/* Display size configuration */
+#define AW200XX_REG_DSIZE          AW200XX_REG(AW200XX_PAGE0, 0x80)
+#define AW200XX_DSIZE_COLUMNS_MAX  12
+
+#define AW200XX_LED2REG(x, columns) \
+	((x) + (((x) / (columns)) * (AW200XX_DSIZE_COLUMNS_MAX - (columns))))
+
+/* Patern selection register*/
+#define AW200XX_REG_PAT_SELECT(x, columns) \
+	AW200XX_REG(AW200XX_PAGE3, AW200XX_LED2REG(x, columns))
+#define AW200XX_PATX_SELECT(x) ((x) + 1)
+
+/*
+ * DIM current configuration register (page 4).
+ * The even address for current DIM configuration.
+ * The odd address for current FADE configuration
+ */
+#define AW200XX_REG_DIM(x, columns) \
+	AW200XX_REG(AW200XX_PAGE4, AW200XX_LED2REG(x, columns) * 2)
+#define AW200XX_REG_DIM2FADE(x) ((x) + 1)
+#define AW200XX_REG_FADE(x, columns) (AW200XX_REG_DIM(x, columns) + 1)
+
+/* Duty ratio of display scan (see p.15 of datasheet for formula) */
+#define AW200XX_DUTY_RATIO(rows) \
+	(((592UL * 1000000UL) / 600500UL) * (1000UL / (rows)) / 1000UL)
+
+struct aw200xx_led {
+	struct aw200xx *chip;
+	struct led_classdev cdev;
+	int dim;
+	u32 num;
+};
+
+struct aw200xx {
+	const struct aw200xx_chipdef *cdef;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct mutex mutex;
+	u32 num_leds;
+	u32 display_rows;
+	struct aw200xx_led leds[];
+};
+
+struct aw200xx_chipdef {
+	u32 channels;
+	u32 display_size_rows_max;
+	u32 display_size_columns;
+};
+
+static ssize_t aw200xx_dim_show(struct device *dev,
+				struct device_attribute *devattr,
+				char *buf)
+{
+	struct led_classdev *cdev = dev_get_drvdata(dev);
+	struct aw200xx_led *led = container_of(cdev, struct aw200xx_led, cdev);
+	int dim = led->dim;
+	ssize_t ret;
+
+	if (dim < 0)
+		ret = sysfs_emit(buf, "auto\n");
+	else
+		ret = sysfs_emit(buf, "%d\n", dim);
+
+	return ret;
+}
+
+static ssize_t aw200xx_dim_store(struct device *dev,
+				 struct device_attribute *devattr,
+				 const char *buf, size_t count)
+{
+	struct led_classdev *cdev = dev_get_drvdata(dev);
+	struct aw200xx_led *led = container_of(cdev, struct aw200xx_led, cdev);
+	struct aw200xx *chip = led->chip;
+	u32 columns = chip->cdef->display_size_columns;
+	int dim;
+	ssize_t ret;
+
+	if (sysfs_streq(buf, "auto")) {
+		dim = -1;
+	} else {
+		ret = kstrtoint(buf, 0, &dim);
+		if (ret < 0 || dim > AW200XX_DIM_MAX)
+			return -EINVAL;
+	}
+
+	mutex_lock(&chip->mutex);
+
+	if (dim >= 0) {
+		ret = regmap_write(chip->regmap,
+				   AW200XX_REG_DIM(led->num, columns), dim);
+		if (ret)
+			goto exit;
+	}
+
+	led->dim = dim;
+	ret = count;
+
+exit:
+	mutex_unlock(&chip->mutex);
+	return ret;
+}
+static DEVICE_ATTR(dim, 0644, aw200xx_dim_show, aw200xx_dim_store);
+
+static struct attribute *dim_attrs[] = {
+	&dev_attr_dim.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(dim);
+
+static int aw200xx_brightness_set(struct led_classdev *cdev,
+				  enum led_brightness brightness)
+{
+	struct aw200xx_led *led = container_of(cdev, struct aw200xx_led, cdev);
+	struct aw200xx *chip = led->chip;
+	int dim;
+	u32 reg;
+	int ret;
+
+	mutex_lock(&chip->mutex);
+
+	reg = AW200XX_REG_DIM(led->num, chip->cdef->display_size_columns);
+	dim = led->dim;
+
+	if (dim < 0) {
+		dim = brightness / (AW200XX_FADE_MAX / AW200XX_DIM_MAX);
+		dim = max(dim, 1);
+	}
+
+	ret = regmap_write(chip->regmap, reg, dim);
+	if (ret)
+		goto error;
+
+	ret = regmap_write(chip->regmap,
+			   AW200XX_REG_DIM2FADE(reg), brightness);
+
+error:
+	mutex_unlock(&chip->mutex);
+
+	return ret;
+}
+
+static u32 aw200xx_imax_from_global(const struct aw200xx *const chip,
+				    u32 global_imax_microamp)
+{
+	unsigned long long duty = AW200XX_DUTY_RATIO(chip->display_rows);
+
+	/* The output current of each LED (see p.14 of datasheet for formula) */
+	return (duty * global_imax_microamp) / 1000U;
+}
+
+static u32 aw200xx_imax_to_global(const struct aw200xx *const chip,
+				  u32 led_imax_microamp)
+{
+	u32 duty = AW200XX_DUTY_RATIO(chip->display_rows);
+
+	/* The output current of each LED (see p.14 of datasheet for formula) */
+	return (led_imax_microamp * 1000U) / duty;
+}
+
+static int aw200xx_set_imax(const struct aw200xx *const chip,
+			    u32 led_imax_microamp)
+{
+	struct imax_global {
+		u32 regval;
+		u32 microamp;
+	} imaxs[] = {
+		{ 8,  3300 },
+		{ 9,  6700 },
+		{ 0,  10000 },
+		{ 11, 13300 },
+		{ 1,  20000 },
+		{ 13, 26700 },
+		{ 2,  30000 },
+		{ 3,  40000 },
+		{ 15, 53300 },
+		{ 4,  60000 },
+		{ 5,  80000 },
+		{ 6,  120000 },
+		{ 7,  160000 },
+	};
+	u32 g_imax_microamp = aw200xx_imax_to_global(chip, led_imax_microamp);
+	int i;
+
+	for (i = ARRAY_SIZE(imaxs) - 1; i >= 0; i--) {
+		if (g_imax_microamp >= imaxs[i].microamp)
+			break;
+	}
+
+	if (i < 0)
+		return -EINVAL;
+
+	return regmap_update_bits(chip->regmap, AW200XX_REG_GCCR,
+				  AW200XX_GCCR_IMAX_MASK,
+				  AW200XX_GCCR_IMAX(imaxs[i].regval));
+}
+
+static int aw200xx_chip_reset(const struct aw200xx *const chip)
+{
+	int ret;
+
+	ret = regmap_write(chip->regmap, AW200XX_REG_RSTR, AW200XX_RSTR_RESET);
+	if (ret)
+		return ret;
+
+	regcache_mark_dirty(chip->regmap);
+	ret = regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
+
+	return ret;
+}
+
+static int aw200xx_chip_init(const struct aw200xx *const chip)
+{
+	int ret;
+
+	ret = regmap_write(chip->regmap, AW200XX_REG_DSIZE,
+			   chip->display_rows - 1);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(chip->regmap, AW200XX_REG_SLPCR,
+			   AW200XX_SLPCR_ACTIVE);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(chip->regmap, AW200XX_REG_GCCR,
+				 AW200XX_GCCR_ALLON, AW200XX_GCCR_ALLON);
+
+	return ret;
+}
+
+static int aw200xx_chip_check(const struct aw200xx *const chip)
+{
+	struct device *dev = &chip->client->dev;
+	u32 chipid;
+	int ret;
+
+	ret = regmap_read(chip->regmap, AW200XX_REG_IDR, &chipid);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read chip ID\n");
+
+	if (chipid != AW200XX_IDR_CHIPID)
+		return dev_err_probe(dev, -ENODEV,
+				     "Chip reported wrong ID: %x\n", chipid);
+
+	return 0;
+}
+
+static int aw200xx_probe_dt(struct device *dev, struct aw200xx *chip)
+{
+	struct fwnode_handle *child;
+	u32 current_min, current_max, min_microamp;
+	int ret;
+	int i = 0;
+
+	ret = device_property_read_u32(dev, "awinic,display-rows",
+				       &chip->display_rows);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to read 'display-rows' property\n");
+
+	if (!chip->display_rows ||
+	    chip->display_rows > chip->cdef->display_size_rows_max) {
+		return dev_err_probe(dev, ret,
+				     "Invalid leds display size %u\n",
+				     chip->display_rows);
+	}
+
+	current_max = aw200xx_imax_from_global(chip, AW200XX_IMAX_MAX_MICROAMP);
+	current_min = aw200xx_imax_from_global(chip, AW200XX_IMAX_MIN_MICROAMP);
+	min_microamp = UINT_MAX;
+
+	device_for_each_child_node(dev, child) {
+		struct led_init_data init_data = {};
+		struct aw200xx_led *led;
+		u32 source, imax;
+
+		ret = fwnode_property_read_u32(child, "reg", &source);
+		if (ret) {
+			dev_err(dev, "Missing reg property\n");
+			chip->num_leds--;
+			continue;
+		}
+
+		if (source >= chip->cdef->channels) {
+			dev_err(dev, "LED reg %u out of range (max %u)\n",
+				source, chip->cdef->channels);
+			chip->num_leds--;
+			continue;
+		}
+
+		ret = fwnode_property_read_u32(child, "led-max-microamp",
+					       &imax);
+		if (ret) {
+			dev_info(&chip->client->dev,
+				 "DT property led-max-microamp is missing\n");
+		} else if (imax < current_min || imax > current_max) {
+			dev_err(dev, "Invalid value %u for led-max-microamp\n",
+				imax);
+			chip->num_leds--;
+			continue;
+		} else {
+			min_microamp = min(min_microamp, imax);
+		}
+
+		led = &chip->leds[i];
+		led->dim = -1;
+		led->num = source;
+		led->chip = chip;
+		led->cdev.brightness_set_blocking = aw200xx_brightness_set;
+		led->cdev.groups = dim_groups;
+		init_data.fwnode = child;
+
+		ret = devm_led_classdev_register_ext(dev, &led->cdev,
+						     &init_data);
+		if (ret) {
+			fwnode_handle_put(child);
+			break;
+		}
+
+		i++;
+	}
+
+	if (!chip->num_leds)
+		return -EINVAL;
+
+	if (min_microamp == UINT_MAX) {
+		min_microamp =
+			aw200xx_imax_from_global(chip,
+						 AW200XX_IMAX_DEFAULT_MICROAMP);
+	}
+
+	ret = aw200xx_set_imax(chip, min_microamp);
+
+	return ret;
+}
+
+static const struct regmap_range_cfg aw200xx_ranges[] = {
+	{
+		.name = "aw200xx",
+		.range_min = 0,
+		.range_max = AW200XX_REG_MAX,
+		.selector_reg = AW200XX_REG_PAGE,
+		.selector_mask = AW200XX_PAGE_MASK,
+		.selector_shift = AW200XX_PAGE_SHIFT,
+		.window_start = 0,
+		.window_len = AW200XX_PAGE_SIZE,
+	},
+};
+
+static const struct regmap_range aw200xx_writeonly_ranges[] = {
+	regmap_reg_range(AW200XX_REG(AW200XX_PAGE1, 0x00), AW200XX_REG_MAX),
+};
+
+static const struct regmap_access_table aw200xx_readable_table = {
+	.no_ranges = aw200xx_writeonly_ranges,
+	.n_no_ranges = ARRAY_SIZE(aw200xx_writeonly_ranges),
+};
+
+static const struct regmap_range aw200xx_readonly_ranges[] = {
+	regmap_reg_range(AW200XX_REG_IDR, AW200XX_REG_IDR),
+	regmap_reg_range(AW200XX_REG_ISRFLT, AW200XX_REG_ISRFLT),
+};
+
+static const struct regmap_access_table aw200xx_writeable_table = {
+	.no_ranges = aw200xx_readonly_ranges,
+	.n_no_ranges = ARRAY_SIZE(aw200xx_readonly_ranges),
+};
+
+static const struct regmap_range aw200xx_volatile_registers[] = {
+	regmap_reg_range(AW200XX_REG_ISRFLT, AW200XX_REG_ISRFLT),
+};
+
+static const struct regmap_access_table aw200xx_volatile_table = {
+	.yes_ranges = aw200xx_volatile_registers,
+	.n_yes_ranges = ARRAY_SIZE(aw200xx_volatile_registers),
+};
+
+static const struct regmap_config aw200xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AW200XX_REG_MAX,
+	.ranges = aw200xx_ranges,
+	.num_ranges = ARRAY_SIZE(aw200xx_ranges),
+	.rd_table = &aw200xx_readable_table,
+	.wr_table = &aw200xx_writeable_table,
+	.volatile_table = &aw200xx_volatile_table,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static int aw200xx_probe(struct i2c_client *client)
+{
+	const struct aw200xx_chipdef *cdef;
+	struct aw200xx *chip;
+	int count;
+	int ret;
+
+	cdef = device_get_match_data(&client->dev);
+
+	count = device_get_child_node_count(&client->dev);
+	if (!count || count > cdef->channels)
+		return dev_err_probe(&client->dev, -EINVAL,
+				     "Incorrect number of leds (%d)", count);
+
+	chip = devm_kzalloc(&client->dev,
+			    struct_size(chip, leds, count),
+			    GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->cdef = cdef;
+	chip->num_leds = count;
+	chip->client = client;
+	i2c_set_clientdata(client, chip);
+
+	chip->regmap = devm_regmap_init_i2c(client, &aw200xx_regmap_config);
+	if (IS_ERR(chip->regmap))
+		return PTR_ERR(chip->regmap);
+
+	ret = aw200xx_chip_check(chip);
+	if (ret)
+		return ret;
+
+	mutex_init(&chip->mutex);
+
+	/* Need a lock now since after call aw200xx_probe_dt, sysfs nodes created */
+	mutex_lock(&chip->mutex);
+
+	ret = aw200xx_probe_dt(&client->dev, chip);
+	if (ret < 0)
+		goto exit;
+
+	ret = aw200xx_chip_reset(chip);
+	if (ret)
+		goto exit;
+
+	ret = aw200xx_chip_init(chip);
+
+exit:
+	mutex_unlock(&chip->mutex);
+	return ret;
+}
+
+static void aw200xx_remove(struct i2c_client *client)
+{
+	struct aw200xx *chip = i2c_get_clientdata(client);
+
+	aw200xx_chip_reset(chip);
+	mutex_destroy(&chip->mutex);
+}
+
+static const struct aw200xx_chipdef aw20036_cdef = {
+	.channels = 36,
+	.display_size_rows_max = 3,
+	.display_size_columns = 12,
+};
+
+static const struct aw200xx_chipdef aw20054_cdef = {
+	.channels = 54,
+	.display_size_rows_max = 6,
+	.display_size_columns = 9,
+};
+
+static const struct aw200xx_chipdef aw20072_cdef = {
+	.channels = 72,
+	.display_size_rows_max = 6,
+	.display_size_columns = 12,
+};
+
+static const struct i2c_device_id aw200xx_id[] = {
+	{ "aw20036" },
+	{ "aw20054" },
+	{ "aw20072" },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, aw200xx_id);
+
+static const struct of_device_id __maybe_unused aw200xx_match_table[] = {
+	{ .compatible = "awinic,aw20036", .data = &aw20036_cdef, },
+	{ .compatible = "awinic,aw20054", .data = &aw20054_cdef, },
+	{ .compatible = "awinic,aw20072", .data = &aw20072_cdef, },
+	{},
+};
+MODULE_DEVICE_TABLE(of, aw200xx_match_table);
+
+static struct i2c_driver aw200xx_driver = {
+	.driver = {
+		.name = "aw200xx",
+		.of_match_table = of_match_ptr(aw200xx_match_table),
+	},
+	.probe_new = aw200xx_probe,
+	.remove = aw200xx_remove,
+	.id_table = aw200xx_id,
+};
+
+module_i2c_driver(aw200xx_driver);
+
+MODULE_AUTHOR("Martin Kurbanov <mmkurbanov@sberdevices.ru>");
+MODULE_DESCRIPTION("AW200XX LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-aw200xx");
-- 
2.38.1

Re: [PATCH v2 2/2] leds: add aw20xx driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

Hi!

> +config LEDS_AW200XX
> +	tristate "LED support for Awinic AW20036/AW20054/AW20072"
> +	depends on LEDS_CLASS
> +	depends on I2C
> +	help
> +	  This option enables support for the AW20036/AW20054/AW20072 LED driver.
> +	  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.

I'm afraid this should be handled as a display, not as an array of
individual LEDs.

Plus RGB LEDs should certainly be handled using multicolor class.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

AUXdisplay for LED arrays, keyboards with per-key LEDs -- was Re: [PATCH v2 2/2] leds: add aw20xx driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

Hi!

> > +config LEDS_AW200XX
> > +	tristate "LED support for Awinic AW20036/AW20054/AW20072"
> > +	depends on LEDS_CLASS
> > +	depends on I2C
> > +	help
> > +	  This option enables support for the AW20036/AW20054/AW20072 LED driver.
> > +	  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.
> 
> I'm afraid this should be handled as a display, not as an array of
> individual LEDs.

You probably want to see

AUXILIARY DISPLAY DRIVERS
M:      Miguel Ojeda <ojeda@kernel.org>
S:      Maintained
F:      Documentation/devicetree/bindings/auxdisplay/
F:      drivers/auxdisplay/
F:      include/linux/cfag12864b.h

And this brings another question...

...sooner or later we'll see LED displays with around 100 pixels in
almost rectangular grid. Minority of the pixels will have funny
shapes. How will we handle those? Pretend it is regular display with
some pixels missing? How do we handle cellphone displays with rounded
corners and holes for front camera?

And yes, such crazy displays are being manufactured -- it is called
keyboard with per-key backlight... 

https://www.reddit.com/r/MechanicalKeyboards/comments/8dtvgo/keyboard_with_individually_programmable_leds/

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: AUXdisplay for LED arrays, keyboards with per-key LEDs -- was Re: [PATCH v2 2/2] leds: add aw20xx driver

[Jani Nikula]

On Tue, 28 Feb 2023, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > +config LEDS_AW200XX
>> > +	tristate "LED support for Awinic AW20036/AW20054/AW20072"
>> > +	depends on LEDS_CLASS
>> > +	depends on I2C
>> > +	help
>> > +	  This option enables support for the AW20036/AW20054/AW20072 LED driver.
>> > +	  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.
>> 
>> I'm afraid this should be handled as a display, not as an array of
>> individual LEDs.
>
> You probably want to see
>
> AUXILIARY DISPLAY DRIVERS
> M:      Miguel Ojeda <ojeda@kernel.org>
> S:      Maintained
> F:      Documentation/devicetree/bindings/auxdisplay/
> F:      drivers/auxdisplay/
> F:      include/linux/cfag12864b.h
>
> And this brings another question...
>
> ...sooner or later we'll see LED displays with around 100 pixels in
> almost rectangular grid. Minority of the pixels will have funny
> shapes. How will we handle those? Pretend it is regular display with
> some pixels missing? How do we handle cellphone displays with rounded
> corners and holes for front camera?
>
> And yes, such crazy displays are being manufactured -- it is called
> keyboard with per-key backlight... 
>
> https://www.reddit.com/r/MechanicalKeyboards/comments/8dtvgo/keyboard_with_individually_programmable_leds/

But... is that a display or a HID?

Only half-joking, really. This somewhat reminds me of using input system
force feedback stuff for touch screen vibrations.

Cc: Dmitry & linux-input.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v2 2/2] leds: add aw20xx driver

[Lee Jones]

On Tue, 28 Feb 2023, Pavel Machek wrote:

> Hi!
> 
> > +config LEDS_AW200XX
> > +	tristate "LED support for Awinic AW20036/AW20054/AW20072"
> > +	depends on LEDS_CLASS
> > +	depends on I2C
> > +	help
> > +	  This option enables support for the AW20036/AW20054/AW20072 LED driver.
> > +	  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.
> 
> I'm afraid this should be handled as a display, not as an array of
> individual LEDs.

Just for my own information, where do we draw the line on this?

Is 4x4 okay?  How about 6x6?

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/2] leds: add aw20xx driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

Hi!

> > > +config LEDS_AW200XX
> > > +	tristate "LED support for Awinic AW20036/AW20054/AW20072"
> > > +	depends on LEDS_CLASS
> > > +	depends on I2C
> > > +	help
> > > +	  This option enables support for the AW20036/AW20054/AW20072 LED driver.
> > > +	  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.
> > 
> > I'm afraid this should be handled as a display, not as an array of
> > individual LEDs.
> 
> Just for my own information, where do we draw the line on this?

Not sure.

> Is 4x4 okay?  How about 6x6?

I'd say "As soon as it is 2-dimensional". Even 3x2 array is a display.

If the LEDs are independend, and it makes sense to display disk
activity on one and CPU activity on second... that's for LED
subsystem.

When userspace needs to know where are the LEDs spatially located, and
when you start putting synchronized animations over the LEDs... well,
then maybe that's a display.

6x9 is definitely a display. We won't put 1920x1080 phone display into
drivers/leds just because it is OLED...

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2 2/2] leds: add aw20xx driver

[Lee Jones]

On Wed, 01 Mar 2023, Pavel Machek wrote:

> Hi!
> 
> > > > +config LEDS_AW200XX
> > > > +	tristate "LED support for Awinic AW20036/AW20054/AW20072"
> > > > +	depends on LEDS_CLASS
> > > > +	depends on I2C
> > > > +	help
> > > > +	  This option enables support for the AW20036/AW20054/AW20072 LED driver.
> > > > +	  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.
> > > 
> > > I'm afraid this should be handled as a display, not as an array of
> > > individual LEDs.
> > 
> > Just for my own information, where do we draw the line on this?
> 
> Not sure.
> 
> > Is 4x4 okay?  How about 6x6?
> 
> I'd say "As soon as it is 2-dimensional". Even 3x2 array is a display.
> 
> If the LEDs are independend, and it makes sense to display disk
> activity on one and CPU activity on second... that's for LED
> subsystem.
> 
> When userspace needs to know where are the LEDs spatially located, and
> when you start putting synchronized animations over the LEDs... well,
> then maybe that's a display.
> 
> 6x9 is definitely a display. We won't put 1920x1080 phone display into
> drivers/leds just because it is OLED...

Got it, thank you Pavel.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/2] leds: add aw20xx driver

[Martin Kurbanov]

On 2023-03-01 00:24, Pavel Machek wrote:
> Hi!
> 
>> +config LEDS_AW200XX
>> +	tristate "LED support for Awinic AW20036/AW20054/AW20072"
>> +	depends on LEDS_CLASS
>> +	depends on I2C
>> +	help
>> +	  This option enables support for the AW20036/AW20054/AW20072 LED driver.
>> +	  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.
> 
> I'm afraid this should be handled as a display, not as an array of
> individual LEDs.

Hello Pavel,

Thank you for the quick feedback and your detailed thoughts, appreciate
it!
I'm totally agree with you, that matrix LED controllers should be
interpreted as display and it must be controlled as display from
userspace. But actually, AW20036 controller usage status is strongly
dependent from board PCB. In the our internal projects AW20036 controls
LEDs line. Each LED brightness/pattern/etc are managed from userspace
independently. From the current registers specification I can't imagine
that it's possible to develop display driver for that. All controller
features involve LED independent managing, like hardware patterns and
brightness setup. Therefore I suppose for AW20036 controller LED
subsystem is more suitable.

-- 
Best Regards,
Kurbanov Martin

Re: [PATCH v2 2/2] leds: add aw20xx driver

[Andy Shevchenko]

On Tue, Feb 28, 2023 at 11:11 PM Martin Kurbanov
<mmkurbanov@sberdevices.ru> wrote:
>
> This commit adds support for AWINIC AW20036/AW20054/AW20072 LED driver.
> This driver supports following AW200XX features:
>   - Individual 64-level DIM currents

I'm wondering if I already commented on the v1 of this. A lot of
issues with the code and your email rings a bell...
Okay, I have dug into archives and it was something else.

...

> +Date:          February 2023

Blast from the past? The best you can get is March 2023.

...

> +Description:   64-level DIM current. If write negative value or "auto",

If you write a

> +               the dim will be calculated according to the brightness.

...

> +config LEDS_AW200XX
> +       tristate "LED support for Awinic AW20036/AW20054/AW20072"
> +       depends on LEDS_CLASS
> +       depends on I2C
> +       help
> +         This option enables support for the AW20036/AW20054/AW20072 LED driver.
> +         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.

What would be the name of the module if M?

...

> +/**

Is it a kernel doc?

> + * leds-aw200xx.c - Awinic AW20036/AW20054/AW20072 LED driver

No name of the file in the file. It's impractical (in case it will be
moved/renamed).

> + *
> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> + *
> + * Author: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> + */

...

> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/bitfield.h>

Can you keep this sorted?

...

> +#define AW200XX_DIM_MAX                  0x3F
> +#define AW200XX_FADE_MAX                 0xFF

If it is limited by the amount of bits in the bitfields, better to use
the form of (BIT(x) - 1), where x is the amount of bits.

...

> +#define AW200XX_IMAX_DEFAULT_MICROAMP    60000
> +#define AW200XX_IMAX_MAX_MICROAMP        160000
> +#define AW200XX_IMAX_MIN_MICROAMP        3300

A is the unit, and for microamperes we already use (in another
driver(s)) the _uA suffix.

...

> +/* Page 0 */
> +#define AW200XX_REG_PAGE0_BASE 0xc000

Indeed, like Pavel mentioned, why not consider the DRM approach for
this? If it's not really a display similar to LCD, then there is
drivers/auxdisplay folder for the non-standard / alphanumeric / etc
cases.

...

> +       (AW200XX_REG_PAGE0_BASE + ((page) * AW200XX_PAGE_SIZE) + (reg))

Multiplication doesn't require parentheses.

...

> +#define AW200XX_PAT_T3_LT_MASK      0xFF

> +#define AW200XX_PAT0_T3_LT_LSB(x)   ((x) & 0xFF)

GENMASK()

...

> +#define AW200XX_PAT0_T_LT_MAX       0xFFF

(BIT(12) - 1) ?

...

> +#define AW200XX_PAT_T1_T3_MASK      0xF0
> +#define AW200XX_PAT_T2_T4_MASK      0x0F

GENMASK()

...

> +#define AW200XX_TEMPLATE_TIME_MAX   0x0F

(BIT(4) - 1)

...

> +/* Duty ratio of display scan (see p.15 of datasheet for formula) */
> +#define AW200XX_DUTY_RATIO(rows) \
> +       (((592UL * 1000000UL) / 600500UL) * (1000UL / (rows)) / 1000UL)

Something to use from units.h?

...

> +struct aw200xx_led {
> +       struct aw200xx *chip;

> +       struct led_classdev cdev;

Moving embedded structure to be the first member might make some code
to be no-op at compile time.

> +       int dim;
> +       u32 num;
> +};

...

> +       ssize_t ret;

Useless, just use return directly.

> +       if (dim < 0)
> +               ret = sysfs_emit(buf, "auto\n");
> +       else
> +               ret = sysfs_emit(buf, "%d\n", dim);
> +
> +       return ret;

if (dim >= 0)
  return sysfs_emit(...);

return sysfs_emit(...);

...

> +               ret = kstrtoint(buf, 0, &dim);
> +               if (ret < 0 || dim > AW200XX_DIM_MAX)
> +                       return -EINVAL;

Why shadowing ret?
Hint: it may not be EINVAL in some cases.

...

> +       if (dim >= 0) {

Hmm... Why not simply use an unsigned type?

> +       }

...

> +       /* The output current of each LED (see p.14 of datasheet for formula) */
> +       return (duty * global_imax_microamp) / 1000U;

units.h ?

...

> +       /* The output current of each LED (see p.14 of datasheet for formula) */
> +       return (led_imax_microamp * 1000U) / duty;

Ditto.

...

> +static int aw200xx_set_imax(const struct aw200xx *const chip,
> +                           u32 led_imax_microamp)
> +{
> +       struct imax_global {
> +               u32 regval;
> +               u32 microamp;
> +       } imaxs[] = {
> +               { 8,  3300 },
> +               { 9,  6700 },
> +               { 0,  10000 },
> +               { 11, 13300 },
> +               { 1,  20000 },
> +               { 13, 26700 },
> +               { 2,  30000 },
> +               { 3,  40000 },
> +               { 15, 53300 },
> +               { 4,  60000 },
> +               { 5,  80000 },
> +               { 6,  120000 },
> +               { 7,  160000 },

This looks a bit random. Is there any pattern on how value is
connected to the register value?

> +       };
> +       u32 g_imax_microamp = aw200xx_imax_to_global(chip, led_imax_microamp);
> +       int i;

int i = ARRAY_SIZE(...);

> +       for (i = ARRAY_SIZE(imaxs) - 1; i >= 0; i--) {

while (i--) {

> +               if (g_imax_microamp >= imaxs[i].microamp)
> +                       break;
> +       }

> +

Redundant blank line.

> +       if (i < 0)
> +               return -EINVAL;
> +
> +       return regmap_update_bits(chip->regmap, AW200XX_REG_GCCR,
> +                                 AW200XX_GCCR_IMAX_MASK,
> +                                 AW200XX_GCCR_IMAX(imaxs[i].regval));
> +}

...

> +       ret = regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
> +
> +       return ret;

return regmap_write(...);

...

> +       ret = regmap_update_bits(chip->regmap, AW200XX_REG_GCCR,
> +                                AW200XX_GCCR_ALLON, AW200XX_GCCR_ALLON);
> +
> +       return ret;

Ditto.

...

> +       ret = aw200xx_set_imax(chip, min_microamp);
> +
> +       return ret;

Ditto.

...

> +       chip = devm_kzalloc(&client->dev,
> +                           struct_size(chip, leds, count),
> +                           GFP_KERNEL);

There is a lot of room on the previous lines.

> +       if (!chip)
> +               return -ENOMEM;

...

> +static const struct of_device_id __maybe_unused aw200xx_match_table[] = {
> +       { .compatible = "awinic,aw20036", .data = &aw20036_cdef, },
> +       { .compatible = "awinic,aw20054", .data = &aw20054_cdef, },
> +       { .compatible = "awinic,aw20072", .data = &aw20072_cdef, },
> +       {},

Comma is not needed for the terminator entry.

> +};

...

> +static struct i2c_driver aw200xx_driver = {
> +       .driver = {
> +               .name = "aw200xx",
> +               .of_match_table = of_match_ptr(aw200xx_match_table),

Why of_match_ptr()? It's a very rare case when you really need this.

> +       },
> +       .probe_new = aw200xx_probe,
> +       .remove = aw200xx_remove,
> +       .id_table = aw200xx_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(aw200xx_driver);

...

> +MODULE_ALIAS("platform:leds-aw200xx");

What is this?! Or i.o.w. why is this violation of the subsystems?


--
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] leds: add aw20xx driver

[kernel test robot]

Hi Martin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pavel-leds/for-next]
[also build test WARNING on robh/for-next krzk-dt/for-next v6.2]
[cannot apply to linus/master next-20230301]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Martin-Kurbanov/dt-bindings-leds-add-binding-for-aw200xx/20230301-051348
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link:    https://lore.kernel.org/r/20230228211046.109693-3-mmkurbanov%40sberdevices.ru
patch subject: [PATCH v2 2/2] leds: add aw20xx driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230301/202303011345.29QIi1UF-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/67ff1bda566825f286aa03653a54a5ca9bc98012
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Martin-Kurbanov/dt-bindings-leds-add-binding-for-aw200xx/20230301-051348
        git checkout 67ff1bda566825f286aa03653a54a5ca9bc98012
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303011345.29QIi1UF-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/leds/leds-aw200xx.c:18: warning: expecting prototype for leds(). Prototype was for AW200XX_LEDS_MAX() instead


vim +18 drivers/leds/leds-aw200xx.c

    17	
  > 18	#define AW200XX_LEDS_MAX                 72
    19	#define AW200XX_PATTERN_MAX              3
    20	#define AW200XX_DIM_MAX                  0x3F
    21	#define AW200XX_FADE_MAX                 0xFF
    22	#define AW200XX_IMAX_DEFAULT_MICROAMP    60000
    23	#define AW200XX_IMAX_MAX_MICROAMP        160000
    24	#define AW200XX_IMAX_MIN_MICROAMP        3300
    25	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v2 2/2] leds: add aw20xx driver

[kernel test robot]

Hi Martin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next krzk-dt/for-next v6.2]
[cannot apply to linus/master next-20230301]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Martin-Kurbanov/dt-bindings-leds-add-binding-for-aw200xx/20230301-051348
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link:    https://lore.kernel.org/r/20230228211046.109693-3-mmkurbanov%40sberdevices.ru
patch subject: [PATCH v2 2/2] leds: add aw20xx driver
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20230301/202303012020.TjjSJTcQ-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/67ff1bda566825f286aa03653a54a5ca9bc98012
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Martin-Kurbanov/dt-bindings-leds-add-binding-for-aw200xx/20230301-051348
        git checkout 67ff1bda566825f286aa03653a54a5ca9bc98012
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303012020.TjjSJTcQ-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__udivdi3" [drivers/leds/leds-aw200xx.ko] undefined!
>> ERROR: modpost: "__divdi3" [drivers/leds/leds-aw200xx.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH v2 2/2] leds: add aw20xx driver

[kernel test robot]

Hi Martin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next krzk-dt/for-next v6.2]
[cannot apply to linus/master next-20230301]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Martin-Kurbanov/dt-bindings-leds-add-binding-for-aw200xx/20230301-051348
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link:    https://lore.kernel.org/r/20230228211046.109693-3-mmkurbanov%40sberdevices.ru
patch subject: [PATCH v2 2/2] leds: add aw20xx driver
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230301/202303012132.lXHhYZRy-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/67ff1bda566825f286aa03653a54a5ca9bc98012
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Martin-Kurbanov/dt-bindings-leds-add-binding-for-aw200xx/20230301-051348
        git checkout 67ff1bda566825f286aa03653a54a5ca9bc98012
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303012132.lXHhYZRy-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__aeabi_uldivmod" [drivers/leds/leds-aw200xx.ko] undefined!
>> ERROR: modpost: "__aeabi_ldivmod" [drivers/leds/leds-aw200xx.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests