[PATCH v1 00/11] leds: aw200xx: several driver updates

[PATCH v1 00/11] leds: aw200xx: several driver updates

[Dmitry Rokosov]

The following patch series includes several updates for the AW200XX LED
driver:
    - some small fixes and optimizations to the driver implementation:
      delays, autodimming calculation, disable_locking regmap flag,
      display_rows calculation in runtime;
    - fix LED device tree node pattern to accept LED names counting not
      only from 0 to f;
    - support HWEN hardware control, which allows enabling or disabling
      AW200XX RTL logic from the main SoC using a GPIO pin;
    - introduce the new AW20108 LED controller, the datasheet for this
      controller can be found at [1].

Links:
    [1] https://doc.awinic.com/doc/20230609wm/8a9a9ac8-1d8f-4e75-bf7a-67a04465c153.pdf

Dmitry Rokosov (3):
  dt-bindings: leds: aw200xx: fix led dt node pattern
  leds: aw200xx: support HWEN hardware control
  dt-bindings: leds: aw200xx: introduce optional hwen-gpio property

George Stark (7):
  leds: aw200xx: calculate dts property display_rows in driver
  dt-bindings: leds: aw200xx: remove property "awinic,display-rows"
  leds: aw200xx: add delay after software reset
  leds: aw200xx: enable disable_locking flag in regmap config
  leds: aw200xx: improve autodim calculation method
  leds: aw200xx: add support for aw20108 device
  dt-bindings: leds: Add binding for AW20108 led driver

Martin Kurbanov (1):
  leds: aw200xx: fix write to DIM parameter

 .../bindings/leds/awinic,aw200xx.yaml         |  42 +++----
 drivers/leds/Kconfig                          |   8 +-
 drivers/leds/leds-aw200xx.c                   | 112 +++++++++++++++---
 3 files changed, 114 insertions(+), 48 deletions(-)

-- 
2.36.0

[PATCH v1 01/11] leds: aw200xx: fix write to DIM parameter

[Dmitry Rokosov]

From: Martin Kurbanov <mmkurbanov@salutedevices.com>

If write only DIM value to the page 4, LED brightness will not be
updated, as both DIM and FADE need to be written to the page 4.
Therefore, write DIM to the page 1.

Fixes: 36a87f371b7a ("leds: Add AW20xx driver")
Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 96979b8e09b7..4c1e7caf8941 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -74,6 +74,10 @@
 #define AW200XX_LED2REG(x, columns) \
 	((x) + (((x) / (columns)) * (AW200XX_DSIZE_COLUMNS_MAX - (columns))))
 
+/* DIM current configuration register on page 1 */
+#define AW200XX_REG_DIM_PAGE1(x, columns) \
+	AW200XX_REG(AW200XX_PAGE1, AW200XX_LED2REG(x, columns))
+
 /*
  * DIM current configuration register (page 4).
  * The even address for current DIM configuration.
@@ -153,7 +157,8 @@ static ssize_t dim_store(struct device *dev, struct device_attribute *devattr,
 
 	if (dim >= 0) {
 		ret = regmap_write(chip->regmap,
-				   AW200XX_REG_DIM(led->num, columns), dim);
+				   AW200XX_REG_DIM_PAGE1(led->num, columns),
+				   dim);
 		if (ret)
 			goto out_unlock;
 	}
-- 
2.36.0

[PATCH v1 02/11] dt-bindings: leds: aw200xx: fix led dt node pattern

[Dmitry Rokosov]

AW200XX controllers have the capability to declare more than 0xf LEDs,
therefore, it is necessary to accept LED names using an appropriate
regex pattern.

Fixes: e338a05e76ca ("dt-bindings: leds: Add binding for AW200xx")
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
index feb5febaf361..73b81f7a7258 100644
--- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -42,7 +42,7 @@ properties:
       Leds matrix size
 
 patternProperties:
-  "^led@[0-9a-f]$":
+  "^led@[0-9a-f]+$":
     type: object
     $ref: common.yaml#
     unevaluatedProperties: false
-- 
2.36.0

[PATCH v1 03/11] leds: aw200xx: support HWEN hardware control

[Dmitry Rokosov]

HWEN is hardware control, which is used for enable/disable aw200xx chip.
It's high active, internally pulled down to GND.

After HWEN pin set high the chip begins to load the OTP information,
which takes 200us to complete. About 200us wait time is needed for
internal oscillator startup and display SRAM initialization. After
display SRAM initialization, the registers in page1 to page5 can be
configured via i2c interface.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 4c1e7caf8941..d92c082d4ab3 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -15,6 +15,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_gpio.h>
 #include <linux/regmap.h>
 #include <linux/time.h>
 #include <linux/units.h>
@@ -116,6 +117,7 @@ struct aw200xx {
 	struct mutex mutex;
 	u32 num_leds;
 	u32 display_rows;
+	int hwen;
 	struct aw200xx_led leds[];
 };
 
@@ -358,6 +360,29 @@ static int aw200xx_chip_check(const struct aw200xx *const chip)
 	return 0;
 }
 
+static void aw200xx_enable(const struct aw200xx *const chip)
+{
+	if (!gpio_is_valid(chip->hwen))
+		return;
+
+	gpio_set_value(chip->hwen, 1);
+
+	/*
+	 * After HWEN pin set high the chip begins to load the OTP information,
+	 * which takes 200us to complete. About 200us wait time is needed for
+	 * internal oscillator startup and display SRAM initialization. After
+	 * display SRAM initialization, the registers in page1 to page5 can be
+	 * configured via i2c interface.
+	 */
+	usleep_range(400, 500);
+}
+
+static void aw200xx_disable(const struct aw200xx *const chip)
+{
+	if (gpio_is_valid(chip->hwen))
+		gpio_set_value(chip->hwen, 0);
+}
+
 static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 {
 	struct fwnode_handle *child;
@@ -445,6 +470,18 @@ static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 	return aw200xx_set_imax(chip, min_uA);
 }
 
+static void aw200xx_probe_hwen(struct device *dev, struct aw200xx *chip)
+{
+	chip->hwen = of_get_named_gpio(dev->of_node, "awinic,hwen-gpio", 0);
+	if (gpio_is_valid(chip->hwen))
+		if (devm_gpio_request_one(dev, chip->hwen, GPIOF_OUT_INIT_HIGH,
+					  "AW200XX HWEN")) {
+			dev_warn(dev, "Can't request gpio %d, tag it invalid\n",
+				 chip->hwen);
+			chip->hwen = -EINVAL;
+		}
+}
+
 static const struct regmap_range_cfg aw200xx_ranges[] = {
 	{
 		.name = "aw200xx",
@@ -517,6 +554,10 @@ static int aw200xx_probe(struct i2c_client *client)
 	if (IS_ERR(chip->regmap))
 		return PTR_ERR(chip->regmap);
 
+	aw200xx_probe_hwen(&client->dev, chip);
+
+	aw200xx_enable(chip);
+
 	ret = aw200xx_chip_check(chip);
 	if (ret)
 		return ret;
@@ -537,6 +578,9 @@ static int aw200xx_probe(struct i2c_client *client)
 	ret = aw200xx_chip_init(chip);
 
 out_unlock:
+	if (ret)
+		aw200xx_disable(chip);
+
 	mutex_unlock(&chip->mutex);
 	return ret;
 }
@@ -546,6 +590,7 @@ static void aw200xx_remove(struct i2c_client *client)
 	struct aw200xx *chip = i2c_get_clientdata(client);
 
 	aw200xx_chip_reset(chip);
+	aw200xx_disable(chip);
 	mutex_destroy(&chip->mutex);
 }
 
-- 
2.36.0

[PATCH v1 04/11] dt-bindings: leds: aw200xx: introduce optional hwen-gpio property

[Dmitry Rokosov]

Property 'awinic,hwen-gpio' is optional, it can be used by the board
developer to connect AW200XX LED controller with appropriate poweron
GPIO pad.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
index 73b81f7a7258..e3ad11fc7a84 100644
--- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -41,6 +41,9 @@ properties:
     description:
       Leds matrix size
 
+  awinic,hwen-gpio:
+    maxItems: 1
+
 patternProperties:
   "^led@[0-9a-f]+$":
     type: object
@@ -90,12 +93,15 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     #include <dt-bindings/leds/common.h>
 
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;
 
+        awinic,hwen-gpio = <&gpio 3 GPIO_ACTIVE_HIGH>;
+
         led-controller@3a {
             compatible = "awinic,aw20036";
             reg = <0x3a>;
-- 
2.36.0

[PATCH v1 05/11] leds: aw200xx: calculate dts property display_rows in driver

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

Get rid of device tree property "awinic,display-rows" and calculate it
in driver using led definition nodes. display-row actually means number
of current switches and depends on how leds are connected to the device.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 40 ++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index d92c082d4ab3..5b6907eb6299 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -383,6 +383,32 @@ static void aw200xx_disable(const struct aw200xx *const chip)
 		gpio_set_value(chip->hwen, 0);
 }
 
+static int aw200xx_probe_get_display_rows(struct device *dev, struct aw200xx *chip)
+{
+	struct fwnode_handle *child;
+	u32 max_source = 0;
+
+	device_for_each_child_node(dev, child) {
+		u32 source;
+		int ret;
+
+		ret = fwnode_property_read_u32(child, "reg", &source);
+		if (ret || source >= chip->cdef->channels)
+			continue;
+
+		if (max_source < source)
+			max_source = source;
+	}
+
+	chip->display_rows = max_source / chip->cdef->display_size_columns + 1;
+	if (!chip->display_rows) {
+		dev_err(dev, "No valid led definitions found\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 {
 	struct fwnode_handle *child;
@@ -390,18 +416,8 @@ static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 	int ret;
 	int i;
 
-	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);
-	}
+	if (aw200xx_probe_get_display_rows(dev, chip))
+		return -EINVAL;
 
 	current_max = aw200xx_imax_from_global(chip, AW200XX_IMAX_MAX_uA);
 	current_min = aw200xx_imax_from_global(chip, AW200XX_IMAX_MIN_uA);
-- 
2.36.0

[PATCH v1 06/11] dt-bindings: leds: aw200xx: remove property "awinic,display-rows"

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

Get rid of the property "awinic,display-rows" and calculate it
in the driver using led definition nodes.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 .../bindings/leds/awinic,aw200xx.yaml         | 28 +++----------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
index e3ad11fc7a84..5cd167ab0f04 100644
--- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -36,11 +36,6 @@ properties:
   "#size-cells":
     const: 0
 
-  awinic,display-rows:
-    $ref: /schemas/types.yaml#/definitions/uint32
-    description:
-      Leds matrix size
-
   awinic,hwen-gpio:
     maxItems: 1
 
@@ -63,31 +58,17 @@ patternProperties:
           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)
+            IMAXled = 160000 * (592 / 600.5) * (1 / max-current-switch-number)
           And the minimum output current formula:
-            IMINled = 3300 * (592 / 600.5) * (1 / display-rows)
+            IMINled = 3300 * (592 / 600.5) * (1 / max-current-switch-number)
+          where max-current-switch-number is determinated by led configuration
+          and depends on how leds are physically connected to the led driver.
 
 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
 
@@ -107,7 +88,6 @@ examples:
             reg = <0x3a>;
             #address-cells = <1>;
             #size-cells = <0>;
-            awinic,display-rows = <3>;
 
             led@0 {
                 reg = <0x0>;
-- 
2.36.0

[PATCH v1 07/11] leds: aw200xx: add delay after software reset

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

According to datasheets of aw200xx devices software reset takes at
least 1 ms so add delay after reset before issuing commands to device.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 5b6907eb6299..a1ef0b0a62fc 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -321,6 +321,9 @@ static int aw200xx_chip_reset(const struct aw200xx *const chip)
 	if (ret)
 		return ret;
 
+	/* according to datasheet software reset takes at least 1ms */
+	usleep_range(1000, 2000);
+
 	regcache_mark_dirty(chip->regmap);
 	return regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
 }
-- 
2.36.0

[PATCH v1 08/11] leds: aw200xx: enable disable_locking flag in regmap config

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

In the driver regmap is always used under mutex so regmap's inner lock
can be disabled.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index a1ef0b0a62fc..5a1a93ffe36c 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -541,6 +541,7 @@ static const struct regmap_config aw200xx_regmap_config = {
 	.rd_table = &aw200xx_readable_table,
 	.wr_table = &aw200xx_writeable_table,
 	.cache_type = REGCACHE_RBTREE,
+	.disable_locking = true,
 };
 
 static int aw200xx_probe(struct i2c_client *client)
-- 
2.36.0

[PATCH v1 09/11] leds: aw200xx: improve autodim calculation method

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

use DIV_ROUND_UP instead of coarse div

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 5a1a93ffe36c..09b8bc6724c7 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -87,6 +87,8 @@
 #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_FADE2DIM(fade) \
+	DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX)
 
 /*
  * Duty ratio of display scan (see p.15 of datasheet for formula):
@@ -195,9 +197,7 @@ static int aw200xx_brightness_set(struct led_classdev *cdev,
 
 	dim = led->dim;
 	if (dim < 0)
-		dim = max_t(int,
-			    brightness / (AW200XX_FADE_MAX / AW200XX_DIM_MAX),
-			    1);
+		dim = AW200XX_REG_FADE2DIM(brightness);
 
 	ret = regmap_write(chip->regmap, reg, dim);
 	if (ret)
-- 
2.36.0

[PATCH v1 10/11] leds: aw200xx: add support for aw20108 device

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

Add support for Awinic aw20108 device from the same LED drivers famliy.
New device supports 108 leds using matrix of 12x9 outputs.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/Kconfig        |  8 ++++----
 drivers/leds/leds-aw200xx.c | 10 +++++++++-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6046dfeca16f..40b3f4191cff 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -95,13 +95,13 @@ 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"
+	tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
 	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,
+	  This option enables support for the AW20036/AW20054/AW20072/AW20108
+	  LED driver. It is a 3x12/6x9/6x12/9x12 matrix LED driver programmed via
+	  an I2C interface, up to 36/54/72/108 LEDs or 12/18/24/36 RGBs,
 	  3 pattern controllers for auto breathing or group dimming control.
 
 	  To compile this driver as a module, choose M here: the module
diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 09b8bc6724c7..717c6e2e7bb1 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Awinic AW20036/AW20054/AW20072 LED driver
+ * Awinic AW20036/AW20054/AW20072/AW20108 LED driver
  *
  * Copyright (c) 2023, SberDevices. All Rights Reserved.
  *
@@ -632,10 +632,17 @@ static const struct aw200xx_chipdef aw20072_cdef = {
 	.display_size_columns = 12,
 };
 
+static const struct aw200xx_chipdef aw20108_cdef = {
+	.channels = 108,
+	.display_size_rows_max = 9,
+	.display_size_columns = 12,
+};
+
 static const struct i2c_device_id aw200xx_id[] = {
 	{ "aw20036" },
 	{ "aw20054" },
 	{ "aw20072" },
+	{ "aw20108" },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, aw200xx_id);
@@ -644,6 +651,7 @@ static const struct of_device_id aw200xx_match_table[] = {
 	{ .compatible = "awinic,aw20036", .data = &aw20036_cdef, },
 	{ .compatible = "awinic,aw20054", .data = &aw20054_cdef, },
 	{ .compatible = "awinic,aw20072", .data = &aw20072_cdef, },
+	{ .compatible = "awinic,aw20108", .data = &aw20108_cdef, },
 	{}
 };
 MODULE_DEVICE_TABLE(of, aw200xx_match_table);
-- 
2.36.0

[PATCH v1 11/11] dt-bindings: leds: Add binding for AW20108 led driver

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

Add aw20108 compatible in devicetree binding for aw200xx led driver.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 .../devicetree/bindings/leds/awinic,aw200xx.yaml          | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
index 5cd167ab0f04..c3abb0f7ded3 100644
--- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -10,15 +10,16 @@ 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,
+  This controller is present on AW20036/AW20054/AW20072/AW20108.
+  It is a 3x12/6x9/6x12/9x12 matrix LED programmed via
+  an I2C interface, up to 36/54/72/108 LEDs or 12/18/24/36 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/en/productDetail/AW20036QNR#tech-docs
   aw20054 - https://www.awinic.com/en/productDetail/AW20054QNR#tech-docs
   aw20072 - https://www.awinic.com/en/productDetail/AW20072QNR#tech-docs
+  aw20108 - https://www.awinic.com/en/productDetail/AW20108QNR#tech-docs
 
 properties:
   compatible:
@@ -26,6 +27,7 @@ properties:
       - awinic,aw20036
       - awinic,aw20054
       - awinic,aw20072
+      - awinic,aw20108
 
   reg:
     maxItems: 1
-- 
2.36.0

Re: [PATCH v1 03/11] leds: aw200xx: support HWEN hardware control

[Andy Shevchenko]

On Fri, Oct 6, 2023 at 7:04 PM Dmitry Rokosov
<ddrokosov@salutedevices.com> wrote:
>
> HWEN is hardware control, which is used for enable/disable aw200xx chip.
> It's high active, internally pulled down to GND.
>
> After HWEN pin set high the chip begins to load the OTP information,
> which takes 200us to complete. About 200us wait time is needed for
> internal oscillator startup and display SRAM initialization. After
> display SRAM initialization, the registers in page1 to page5 can be

pages 1 to 5


> configured via i2c interface.

...

> +#include <linux/of_gpio.h>

Definitely not.

Use agnostic APIs.

...

> @@ -116,6 +117,7 @@ struct aw200xx {
>         struct mutex mutex;

>         u32 num_leds;
>         u32 display_rows;
> +       int hwen;
>         struct aw200xx_led leds[];

Side note: add a patch to use __counted_by() here.

>  };

...

> +       if (!gpio_is_valid(chip->hwen))

Absolutely not. You may not use legacy GPIO APIs.

> +               return;
> +
> +       gpio_set_value(chip->hwen, 1);

Ditto.

...

> +       usleep_range(400, 500);

fsleep() ?

...

> +static void aw200xx_disable(const struct aw200xx *const chip)
> +{
> +       if (gpio_is_valid(chip->hwen))
> +               gpio_set_value(chip->hwen, 0);
> +}

As per above.

...

> +static void aw200xx_probe_hwen(struct device *dev, struct aw200xx *chip)
> +{
> +       chip->hwen = of_get_named_gpio(dev->of_node, "awinic,hwen-gpio", 0);
> +       if (gpio_is_valid(chip->hwen))
> +               if (devm_gpio_request_one(dev, chip->hwen, GPIOF_OUT_INIT_HIGH,
> +                                         "AW200XX HWEN")) {
> +                       dev_warn(dev, "Can't request gpio %d, tag it invalid\n",
> +                                chip->hwen);
> +                       chip->hwen = -EINVAL;
> +               }
> +}

Please, rewrite this completely using supported APIs and not
deprecated or obsolete ones.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 05/11] leds: aw200xx: calculate dts property display_rows in driver

[Andy Shevchenko]

On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
<ddrokosov@salutedevices.com> wrote:
>
> From: George Stark <gnstark@salutedevices.com>
>
> Get rid of device tree property "awinic,display-rows" and calculate it
> in driver using led definition nodes. display-row actually means number
> of current switches and depends on how leds are connected to the device.

So, how do we know that there will be no regressions on the systems
where this property is used in production?

...

> +               if (max_source < source)
> +                       max_source = source;

max()  (will need minmax.h)?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 07/11] leds: aw200xx: add delay after software reset

[Andy Shevchenko]

On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
<ddrokosov@salutedevices.com> wrote:
>
> From: George Stark <gnstark@salutedevices.com>
>
> According to datasheets of aw200xx devices software reset takes at
> least 1 ms so add delay after reset before issuing commands to device.

> +       /* according to datasheet software reset takes at least 1ms */

Please, be consistent in all patches in all commit messages and code
comments on how you supply physical units (w/ space or w/o, take also
into consideration traditional scientific practices).

> +       usleep_range(1000, 2000);

fsleep() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 09/11] leds: aw200xx: improve autodim calculation method

[Andy Shevchenko]

On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
<ddrokosov@salutedevices.com> wrote:
>
> From: George Stark <gnstark@salutedevices.com>
>
> use DIV_ROUND_UP instead of coarse div

Please, respect English grammar and punctuation.
Refer to macros and functions as func() (note the parentheses).

...

>  #define AW200XX_REG_DIM2FADE(x) ((x) + 1)
> +#define AW200XX_REG_FADE2DIM(fade) \
> +       DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX)

Have you checked if the overflow is _now_ possible (compiling on
32-bit platforms as well)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 03/11] leds: aw200xx: support HWEN hardware control

[Dmitry Rokosov]

Hello Andy,

Thanks a lot for such a quick review!

Please find my comments below.

On Fri, Oct 06, 2023 at 08:57:23PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 6, 2023 at 7:04 PM Dmitry Rokosov
> <ddrokosov@salutedevices.com> wrote:
> >
> > HWEN is hardware control, which is used for enable/disable aw200xx chip.
> > It's high active, internally pulled down to GND.
> >
> > After HWEN pin set high the chip begins to load the OTP information,
> > which takes 200us to complete. About 200us wait time is needed for
> > internal oscillator startup and display SRAM initialization. After
> > display SRAM initialization, the registers in page1 to page5 can be
> 
> pages 1 to 5
> 
> 

Sure, you are totally right.

> > configured via i2c interface.
> 
> ...
> 
> > +#include <linux/of_gpio.h>
> 
> Definitely not.
> 
> Use agnostic APIs.

Sure

> > @@ -116,6 +117,7 @@ struct aw200xx {
> >         struct mutex mutex;
> 
> >         u32 num_leds;
> >         u32 display_rows;
> > +       int hwen;
> >         struct aw200xx_led leds[];
> 
> Side note: add a patch to use __counted_by() here.

Okay, now I see the patch with __counted_by() some days ago. I will
rebase on it.

> > +       if (!gpio_is_valid(chip->hwen))
> 
> Absolutely not. You may not use legacy GPIO APIs.

Exactly

> > +               return;
> > +
> > +       gpio_set_value(chip->hwen, 1);
> 
> Ditto.

Ok

> > +       usleep_range(400, 500);
> 
> fsleep() ?

Agreed. In this situation fsleep() automatic behaviour is acceptable.

> 
> ...
> 
> > +static void aw200xx_disable(const struct aw200xx *const chip)
> > +{
> > +       if (gpio_is_valid(chip->hwen))
> > +               gpio_set_value(chip->hwen, 0);
> > +}
> 
> As per above.

Ok

> > +static void aw200xx_probe_hwen(struct device *dev, struct aw200xx *chip)
> > +{
> > +       chip->hwen = of_get_named_gpio(dev->of_node, "awinic,hwen-gpio", 0);
> > +       if (gpio_is_valid(chip->hwen))
> > +               if (devm_gpio_request_one(dev, chip->hwen, GPIOF_OUT_INIT_HIGH,
> > +                                         "AW200XX HWEN")) {
> > +                       dev_warn(dev, "Can't request gpio %d, tag it invalid\n",
> > +                                chip->hwen);
> > +                       chip->hwen = -EINVAL;
> > +               }
> > +}
> 
> Please, rewrite this completely using supported APIs and not
> deprecated or obsolete ones.

Yep, I see. I will use devm_gpiod_* API.

-- 
Thank you,
Dmitry

Re: [PATCH v1 07/11] leds: aw200xx: add delay after software reset

[Dmitry Rokosov]

On Fri, Oct 06, 2023 at 09:01:39PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
> <ddrokosov@salutedevices.com> wrote:
> >
> > From: George Stark <gnstark@salutedevices.com>
> >
> > According to datasheets of aw200xx devices software reset takes at
> > least 1 ms so add delay after reset before issuing commands to device.
> 
> > +       /* according to datasheet software reset takes at least 1ms */
> 
> Please, be consistent in all patches in all commit messages and code
> comments on how you supply physical units (w/ space or w/o, take also
> into consideration traditional scientific practices).
> 
> > +       usleep_range(1000, 2000);
> 
> fsleep() ?

Good catch! Thank you for pointing!

-- 
Thank you,
Dmitry

Re: [PATCH v1 09/11] leds: aw200xx: improve autodim calculation method

[Dmitry Rokosov]

On Fri, Oct 06, 2023 at 09:03:47PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
> <ddrokosov@salutedevices.com> wrote:
> >
> > From: George Stark <gnstark@salutedevices.com>
> >
> > use DIV_ROUND_UP instead of coarse div
> 
> Please, respect English grammar and punctuation.
> Refer to macros and functions as func() (note the parentheses).
> 
> ...
> 
> >  #define AW200XX_REG_DIM2FADE(x) ((x) + 1)
> > +#define AW200XX_REG_FADE2DIM(fade) \
> > +       DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX)
> 
> Have you checked if the overflow is _now_ possible (compiling on
> 32-bit platforms as well)?

I suppose we shouldn't carry on about overflow here because the value of
fade cannot exceed 255, and DIM_MAX is set at 63

You can find maximum values of fade and dim in the aw200xx driver
header:

#define AW200XX_DIM_MAX                  (BIT(6) - 1)
#define AW200XX_FADE_MAX                 (BIT(8) - 1)

-- 
Thank you,
Dmitry

Re: [PATCH v1 05/11] leds: aw200xx: calculate dts property display_rows in driver

[Dmitry Rokosov]

On Fri, Oct 06, 2023 at 08:59:46PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
> <ddrokosov@salutedevices.com> wrote:
> >
> > From: George Stark <gnstark@salutedevices.com>
> >
> > Get rid of device tree property "awinic,display-rows" and calculate it
> > in driver using led definition nodes. display-row actually means number
> > of current switches and depends on how leds are connected to the device.
> 
> So, how do we know that there will be no regressions on the systems
> where this property is used in production?

In the production boards, developers should set up the display-rows
correctly; otherwise, the AW200XX LED controller will not function
properly. In the new implementation, we calculate display-rows
automatically, and I assume that the value will remain unchanged.

> > +               if (max_source < source)
> > +                       max_source = source;
> 
> max()  (will need minmax.h)?

Correct, I will fix it in the v2.

-- 
Thank you,
Dmitry

Re: [PATCH v1 09/11] leds: aw200xx: improve autodim calculation method

[Dmitry Rokosov]

On Mon, Oct 09, 2023 at 04:18:08PM +0300, Dmitry Rokosov wrote:
> On Fri, Oct 06, 2023 at 09:03:47PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
> > <ddrokosov@salutedevices.com> wrote:
> > >
> > > From: George Stark <gnstark@salutedevices.com>
> > >
> > > use DIV_ROUND_UP instead of coarse div
> > 
> > Please, respect English grammar and punctuation.
> > Refer to macros and functions as func() (note the parentheses).
> > 
> > ...
> > 
> > >  #define AW200XX_REG_DIM2FADE(x) ((x) + 1)
> > > +#define AW200XX_REG_FADE2DIM(fade) \
> > > +       DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX)
> > 
> > Have you checked if the overflow is _now_ possible (compiling on
> > 32-bit platforms as well)?
> 
> I suppose we shouldn't carry on about overflow here because the value of
> fade cannot exceed 255, and DIM_MAX is set at 63
> 
> You can find maximum values of fade and dim in the aw200xx driver
> header:
> 
> #define AW200XX_DIM_MAX                  (BIT(6) - 1)
> #define AW200XX_FADE_MAX                 (BIT(8) - 1)

I agree that checking if the fade is not greater than FADE_MAX will not
be excessive. The LED subsystem is capable of sending brightness levels
higher than 255

-- 
Thank you,
Dmitry

Re: [PATCH v1 02/11] dt-bindings: leds: aw200xx: fix led dt node pattern

[Rob Herring]

On Fri, Oct 06, 2023 at 07:04:28PM +0300, Dmitry Rokosov wrote:
> AW200XX controllers have the capability to declare more than 0xf LEDs,
> therefore, it is necessary to accept LED names using an appropriate
> regex pattern.
> 
> Fixes: e338a05e76ca ("dt-bindings: leds: Add binding for AW200xx")
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> index feb5febaf361..73b81f7a7258 100644
> --- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> @@ -42,7 +42,7 @@ properties:
>        Leds matrix size
>  
>  patternProperties:
> -  "^led@[0-9a-f]$":
> +  "^led@[0-9a-f]+$":

There should be some updated constraints on 'reg' too. Actually, looks 
like they were missing altogether. Please add.

>      type: object
>      $ref: common.yaml#
>      unevaluatedProperties: false
> -- 
> 2.36.0
> 

Re: [PATCH v1 04/11] dt-bindings: leds: aw200xx: introduce optional hwen-gpio property

[Rob Herring]

On Fri, Oct 06, 2023 at 07:04:30PM +0300, Dmitry Rokosov wrote:
> Property 'awinic,hwen-gpio' is optional, it can be used by the board
> developer to connect AW200XX LED controller with appropriate poweron
> GPIO pad.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> index 73b81f7a7258..e3ad11fc7a84 100644
> --- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> @@ -41,6 +41,9 @@ properties:
>      description:
>        Leds matrix size
>  
> +  awinic,hwen-gpio:
> +    maxItems: 1

We have standard 'enable-gpios' or 'powerdown-gpios'. Those don't work 
here?

Note that *-gpio is deprecated in favor of *-gpios.


> +
>  patternProperties:
>    "^led@[0-9a-f]+$":
>      type: object
> @@ -90,12 +93,15 @@ additionalProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
>      #include <dt-bindings/leds/common.h>
>  
>      i2c {
>          #address-cells = <1>;
>          #size-cells = <0>;
>  
> +        awinic,hwen-gpio = <&gpio 3 GPIO_ACTIVE_HIGH>;
> +
>          led-controller@3a {
>              compatible = "awinic,aw20036";
>              reg = <0x3a>;
> -- 
> 2.36.0
> 

Re: [PATCH v1 06/11] dt-bindings: leds: aw200xx: remove property "awinic,display-rows"

[Rob Herring]

On Fri, 06 Oct 2023 19:04:32 +0300, Dmitry Rokosov wrote:
> From: George Stark <gnstark@salutedevices.com>
> 
> Get rid of the property "awinic,display-rows" and calculate it
> in the driver using led definition nodes.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  .../bindings/leds/awinic,aw200xx.yaml         | 28 +++----------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v1 11/11] dt-bindings: leds: Add binding for AW20108 led driver

[Rob Herring]

On Fri, Oct 06, 2023 at 07:04:37PM +0300, Dmitry Rokosov wrote:
> From: George Stark <gnstark@salutedevices.com>
> 
> Add aw20108 compatible in devicetree binding for aw200xx led driver.

Binding is for the hardware, not a driver.

For the subject: dt-bindings: leds: awinic,aw200xx: Add AW20108 device

> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  .../devicetree/bindings/leds/awinic,aw200xx.yaml          | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> index 5cd167ab0f04..c3abb0f7ded3 100644
> --- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> @@ -10,15 +10,16 @@ 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,
> +  This controller is present on AW20036/AW20054/AW20072/AW20108.
> +  It is a 3x12/6x9/6x12/9x12 matrix LED programmed via
> +  an I2C interface, up to 36/54/72/108 LEDs or 12/18/24/36 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/en/productDetail/AW20036QNR#tech-docs
>    aw20054 - https://www.awinic.com/en/productDetail/AW20054QNR#tech-docs
>    aw20072 - https://www.awinic.com/en/productDetail/AW20072QNR#tech-docs
> +  aw20108 - https://www.awinic.com/en/productDetail/AW20108QNR#tech-docs
>  
>  properties:
>    compatible:
> @@ -26,6 +27,7 @@ properties:
>        - awinic,aw20036
>        - awinic,aw20054
>        - awinic,aw20072
> +      - awinic,aw20108
>  
>    reg:
>      maxItems: 1
> -- 
> 2.36.0
> 

Re: [PATCH v1 04/11] dt-bindings: leds: aw200xx: introduce optional hwen-gpio property

[Dmitry Rokosov]

Hello Rob,

Thank you for the review! Please find my comments below.

On Tue, Oct 10, 2023 at 09:13:32AM -0500, Rob Herring wrote:
> On Fri, Oct 06, 2023 at 07:04:30PM +0300, Dmitry Rokosov wrote:
> > Property 'awinic,hwen-gpio' is optional, it can be used by the board
> > developer to connect AW200XX LED controller with appropriate poweron
> > GPIO pad.
> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> > index 73b81f7a7258..e3ad11fc7a84 100644
> > --- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> > +++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> > @@ -41,6 +41,9 @@ properties:
> >      description:
> >        Leds matrix size
> >  
> > +  awinic,hwen-gpio:
> > +    maxItems: 1
> 
> We have standard 'enable-gpios' or 'powerdown-gpios'. Those don't work 
> here?
> 
> Note that *-gpio is deprecated in favor of *-gpios.

Yes, you are absolutely correct. Andy has already addressed this issue
in the driver patchset. I will revise the driver to utilize the current
GPIO API.

> > +
> >  patternProperties:
> >    "^led@[0-9a-f]+$":
> >      type: object
> > @@ -90,12 +93,15 @@ additionalProperties: false
> >  
> >  examples:
> >    - |
> > +    #include <dt-bindings/gpio/gpio.h>
> >      #include <dt-bindings/leds/common.h>
> >  
> >      i2c {
> >          #address-cells = <1>;
> >          #size-cells = <0>;
> >  
> > +        awinic,hwen-gpio = <&gpio 3 GPIO_ACTIVE_HIGH>;
> > +
> >          led-controller@3a {
> >              compatible = "awinic,aw20036";
> >              reg = <0x3a>;
> > -- 
> > 2.36.0
> > 

-- 
Thank you,
Dmitry

Re: [PATCH v1 09/11] leds: aw200xx: improve autodim calculation method

[George Stark]

On 10/9/23 17:02, Dmitry Rokosov wrote:
> On Mon, Oct 09, 2023 at 04:18:08PM +0300, Dmitry Rokosov wrote:
>> On Fri, Oct 06, 2023 at 09:03:47PM +0300, Andy Shevchenko wrote:
>>> On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
>>> <ddrokosov@salutedevices.com> wrote:
>>>>
>>>> From: George Stark <gnstark@salutedevices.com>
>>>>
>>>> use DIV_ROUND_UP instead of coarse div
>>>
>>> Please, respect English grammar and punctuation.
>>> Refer to macros and functions as func() (note the parentheses).
>>>
>>> ...
>>>
>>>>   #define AW200XX_REG_DIM2FADE(x) ((x) + 1)
>>>> +#define AW200XX_REG_FADE2DIM(fade) \
>>>> +       DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX)
>>>
>>> Have you checked if the overflow is _now_ possible (compiling on
>>> 32-bit platforms as well)?
>>
>> I suppose we shouldn't carry on about overflow here because the value of
>> fade cannot exceed 255, and DIM_MAX is set at 63
>>
>> You can find maximum values of fade and dim in the aw200xx driver
>> header:
>>
>> #define AW200XX_DIM_MAX                  (BIT(6) - 1)
>> #define AW200XX_FADE_MAX                 (BIT(8) - 1)
> 
> I agree that checking if the fade is not greater than FADE_MAX will not
> be excessive. The LED subsystem is capable of sending brightness levels
> higher than 255
> 

Probably we should set max_brightness to AW200XX_FADE_MAX in 
led_classdev when adding leds.

-- 
Best regards
George

Re: [PATCH v1 05/11] leds: aw200xx: calculate dts property display_rows in driver

[George Stark]

Hello Andy

On 10/9/23 16:20, Dmitry Rokosov wrote:
> On Fri, Oct 06, 2023 at 08:59:46PM +0300, Andy Shevchenko wrote:
>> On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
>> <ddrokosov@salutedevices.com> wrote:
>>>
>>> From: George Stark <gnstark@salutedevices.com>
>>>
>>> Get rid of device tree property "awinic,display-rows" and calculate it
>>> in driver using led definition nodes. display-row actually means number
>>> of current switches and depends on how leds are connected to the device.
>>
>> So, how do we know that there will be no regressions on the systems
>> where this property is used in production?

There're two possible cases here if "awinic,display-rows" value is not 
equal to autocalculated value which is incorrect way of using the driver:

1) "awinic,display-rows" value was less then autocalculated value - it 
means that some leds never couldn't be turned on even if they are 
defined in dts. Now all defined leds can be controlled.

2)"awinic,display-rows" value was higher then autocalculated value -
it means that leds refresh cycle time was greater then it really needed 
due to controller spent time powering unconnected pins. It will affect 
leds' current but I consider it a kind of hack - the driver provides 
means to control current.

> 
> In the production boards, developers should set up the display-rows
> correctly; otherwise, the AW200XX LED controller will not function
> properly. In the new implementation, we calculate display-rows
> automatically, and I assume that the value will remain unchanged.
> 
>>> +               if (max_source < source)
>>> +                       max_source = source;
>>
>> max()  (will need minmax.h)?
> 
> Correct, I will fix it in the v2.
> 

-- 
Best regards
George