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

[PATCH v4 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;
    - add missing reg constraints;
    - 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].

Changes v4 since v3 at [4]
    - properly handle max_source = 0 situations
    - fix Rob's dt_binding_check alerts

Changes v3 since v2 at [3]:
    - handle all cases during hwen gpio get routine execution
    - rename 'hwen-gpios' to standard 'enable-gpios'
    - properly handle aw200xx_probe_get_display_rows() ret values
    - fix timestamp format in the comments and commit messages
    - expand LEDS_AW200XX config and dt-bindings description
    - describe reg constraints for all compatible variants
    - add Conor's Acked-by tag

Changes v2 since v1 at [2]:
    - rebase on the latest aw200xx changes from lee/leds git repo
    - some commit messages rewording
    - replace legacy gpio_* API with gpiod_* and devm_gpiod_* API
    - rename dt property awinic,hwen-gpio to hwen-gpios according to
      gpiod API
    - use fsleep() instead of usleep_range() per Andy's suggestion
    - add max_brightness parameter to led cdev to restrict
      set_brightness() overflow
    - provide reg constraints as Rob suggested
    - move hwen-gpios to proper dt node in the bindings example

Links:
    [1] https://doc.awinic.com/doc/20230609wm/8a9a9ac8-1d8f-4e75-bf7a-67a04465c153.pdf
    [2] https://lore.kernel.org/all/20231006160437.15627-1-ddrokosov@salutedevices.com/
    [3] https://lore.kernel.org/all/20231018182943.18700-1-ddrokosov@salutedevices.com/
    [4] https://lore.kernel.org/all/20231101142445.8753-1-ddrokosov@salutedevices.com/

Dmitry Rokosov (3):
  leds: aw200xx: support HWEN hardware control
  dt-bindings: leds: aw200xx: introduce optional enable-gpios property
  dt-bindings: leds: aw200xx: fix led pattern and add reg constraints

George Stark (7):
  leds: aw200xx: calculate dts property display_rows in the 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: awinic,aw200xx: add AW20108 device

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

 .../bindings/leds/awinic,aw200xx.yaml         |  95 ++++++++++++-----
 drivers/leds/Kconfig                          |  14 ++-
 drivers/leds/leds-aw200xx.c                   | 100 +++++++++++++++---
 3 files changed, 163 insertions(+), 46 deletions(-)

-- 
2.36.0

[PATCH v4 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 ef4eda6a09ee..842a22087b16 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 v4 02/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 page 1 to page 5 can be
configured via i2c interface.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/leds/leds-aw200xx.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 842a22087b16..7762b3a132ac 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -10,6 +10,7 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/container_of.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
 #include <linux/mod_devicetable.h>
@@ -116,6 +117,7 @@ struct aw200xx {
 	struct mutex mutex;
 	u32 num_leds;
 	u32 display_rows;
+	struct gpio_desc *hwen;
 	struct aw200xx_led leds[] __counted_by(num_leds);
 };
 
@@ -358,6 +360,25 @@ static int aw200xx_chip_check(const struct aw200xx *const chip)
 	return 0;
 }
 
+static void aw200xx_enable(const struct aw200xx *const chip)
+{
+	gpiod_set_value_cansleep(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.
+	 */
+	fsleep(400);
+}
+
+static void aw200xx_disable(const struct aw200xx *const chip)
+{
+	return gpiod_set_value_cansleep(chip->hwen, 0);
+}
+
 static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 {
 	struct fwnode_handle *child;
@@ -517,6 +538,14 @@ static int aw200xx_probe(struct i2c_client *client)
 	if (IS_ERR(chip->regmap))
 		return PTR_ERR(chip->regmap);
 
+	chip->hwen = devm_gpiod_get_optional(&client->dev, "enable",
+					     GPIOD_OUT_HIGH);
+	if (IS_ERR(chip->hwen))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->hwen),
+				     "Cannot get enable gpio");
+
+	aw200xx_enable(chip);
+
 	ret = aw200xx_chip_check(chip);
 	if (ret)
 		return ret;
@@ -537,6 +566,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 +578,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

Re: [PATCH v4 02/11] leds: aw200xx: support HWEN hardware control

[Lee Jones]

On Tue, 21 Nov 2023, Dmitry Rokosov 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 page 1 to page 5 can be
> configured via i2c interface.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/leds/leds-aw200xx.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> index 842a22087b16..7762b3a132ac 100644
> --- a/drivers/leds/leds-aw200xx.c
> +++ b/drivers/leds/leds-aw200xx.c
> @@ -10,6 +10,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
>  #include <linux/container_of.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/leds.h>
>  #include <linux/mod_devicetable.h>
> @@ -116,6 +117,7 @@ struct aw200xx {
>  	struct mutex mutex;
>  	u32 num_leds;
>  	u32 display_rows;
> +	struct gpio_desc *hwen;
>  	struct aw200xx_led leds[] __counted_by(num_leds);
>  };
>  
> @@ -358,6 +360,25 @@ static int aw200xx_chip_check(const struct aw200xx *const chip)
>  	return 0;
>  }
>  
> +static void aw200xx_enable(const struct aw200xx *const chip)
> +{
> +	gpiod_set_value_cansleep(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.
> +	 */
> +	fsleep(400);
> +}
> +
> +static void aw200xx_disable(const struct aw200xx *const chip)
> +{
> +	return gpiod_set_value_cansleep(chip->hwen, 0);
> +}
> +
>  static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
>  {
>  	struct fwnode_handle *child;
> @@ -517,6 +538,14 @@ static int aw200xx_probe(struct i2c_client *client)
>  	if (IS_ERR(chip->regmap))
>  		return PTR_ERR(chip->regmap);
>  
> +	chip->hwen = devm_gpiod_get_optional(&client->dev, "enable",
> +					     GPIOD_OUT_HIGH);
> +	if (IS_ERR(chip->hwen))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->hwen),
> +				     "Cannot get enable gpio");

Nit: "GPIO"

> +
> +	aw200xx_enable(chip);
> +
>  	ret = aw200xx_chip_check(chip);
>  	if (ret)
>  		return ret;
> @@ -537,6 +566,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 +578,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
> 

-- 
Lee Jones [李琼斯]

[PATCH v4 03/11] dt-bindings: leds: aw200xx: introduce optional enable-gpios property

[Dmitry Rokosov]

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

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
index feb5febaf361..3da3633a242c 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
 
+  enable-gpios:
+    maxItems: 1
+
 patternProperties:
   "^led@[0-9a-f]$":
     type: object
@@ -90,6 +93,7 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     #include <dt-bindings/leds/common.h>
 
     i2c {
@@ -102,6 +106,7 @@ examples:
             #address-cells = <1>;
             #size-cells = <0>;
             awinic,display-rows = <3>;
+            enable-gpios = <&gpio 3 GPIO_ACTIVE_HIGH>;
 
             led@0 {
                 reg = <0x0>;
-- 
2.36.0

[PATCH v4 04/11] leds: aw200xx: calculate dts property display_rows in the driver

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

Get rid of device tree property "awinic,display-rows". The property
value actually means number of current switches and depends on how leds
are connected to the device. It should be calculated manually by max
used led number. In the same way it is computed automatically now.
Max used led is taken from led definition subnodes.

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

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 7762b3a132ac..4bce5e7381c0 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
 	return gpiod_set_value_cansleep(chip->hwen, 0);
 }
 
+static bool 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;
+
+		max_source = max(max_source, source);
+	}
+
+	if (!max_source)
+		return false;
+
+	chip->display_rows = max_source / chip->cdef->display_size_columns + 1;
+
+	return true;
+}
+
 static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 {
 	struct fwnode_handle *child;
@@ -386,18 +410,9 @@ 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 dev_err_probe(dev, -EINVAL,
+				     "No valid led definitions found\n");
 
 	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

Re: [PATCH v4 04/11] leds: aw200xx: calculate dts property display_rows in the driver

[Lee Jones]

On Tue, 21 Nov 2023, Dmitry Rokosov wrote:

> From: George Stark <gnstark@salutedevices.com>
> 
> Get rid of device tree property "awinic,display-rows". The property
> value actually means number of current switches and depends on how leds

Nit: LEDs

> are connected to the device. It should be calculated manually by max
> used led number. In the same way it is computed automatically now.

As above - I won't mention this again.

> Max used led is taken from led definition subnodes.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> index 7762b3a132ac..4bce5e7381c0 100644
> --- a/drivers/leds/leds-aw200xx.c
> +++ b/drivers/leds/leds-aw200xx.c
> @@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
>  	return gpiod_set_value_cansleep(chip->hwen, 0);
>  }
>  
> +static bool 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)

Shouldn't the second clause fail instantly?

> +			continue;
> +
> +		max_source = max(max_source, source);
> +	}
> +
> +	if (!max_source)

Since max_source is an integer, please use an '== 0' comparison.

> +		return false;
> +
> +	chip->display_rows = max_source / chip->cdef->display_size_columns + 1;
> +
> +	return true;
> +}
> +
>  static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
>  {
>  	struct fwnode_handle *child;
> @@ -386,18 +410,9 @@ 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))

Function calls in side if() statements in general is rough.

Please break it out and use 'ret' as we usually do.

> +		return dev_err_probe(dev, -EINVAL,

Make this the return value from aw200xx_probe_get_display_rows() and use
'ret' instead.

> +				     "No valid led definitions found\n");
>  
>  	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
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v4 04/11] leds: aw200xx: calculate dts property display_rows in the driver

[Dmitry Rokosov]

On Thu, Nov 23, 2023 at 04:32:52PM +0000, Lee Jones wrote:
> On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> 
> > From: George Stark <gnstark@salutedevices.com>
> > 
> > Get rid of device tree property "awinic,display-rows". The property
> > value actually means number of current switches and depends on how leds
> 
> Nit: LEDs
> 
> > are connected to the device. It should be calculated manually by max
> > used led number. In the same way it is computed automatically now.
> 
> As above - I won't mention this again.
> 
> > Max used led is taken from led definition subnodes.
> > 
> > Signed-off-by: George Stark <gnstark@salutedevices.com>
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> > index 7762b3a132ac..4bce5e7381c0 100644
> > --- a/drivers/leds/leds-aw200xx.c
> > +++ b/drivers/leds/leds-aw200xx.c
> > @@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
> >  	return gpiod_set_value_cansleep(chip->hwen, 0);
> >  }
> >  
> > +static bool 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)
> 
> Shouldn't the second clause fail instantly?
> 

We already have such logic in the aw200xx_probe_fw() function, which
skips the LED node with the wrong reg value too. Furthermore, we have
strict reg constraints in the dt-bindings parts (in the current patch
series), so we assume that the DT developer will not create an LED with
the wrong reg value.

> > +			continue;
> > +
> > +		max_source = max(max_source, source);
> > +	}
> > +
> > +	if (!max_source)
> 
> Since max_source is an integer, please use an '== 0' comparison.
> 

Okay

> > +		return false;
> > +
> > +	chip->display_rows = max_source / chip->cdef->display_size_columns + 1;
> > +
> > +	return true;
> > +}
> > +
> >  static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
> >  {
> >  	struct fwnode_handle *child;
> > @@ -386,18 +410,9 @@ 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))
> 
> Function calls in side if() statements in general is rough.
> 
> Please break it out and use 'ret' as we usually do.
> 
> > +		return dev_err_probe(dev, -EINVAL,
> 
> Make this the return value from aw200xx_probe_get_display_rows() and use
> 'ret' instead.
> 

No problem, I'll prepare a new version.

> > +				     "No valid led definitions found\n");
> >  
> >  	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
> > 
> 
> -- 
> Lee Jones [李琼斯]

-- 
Thank you,
Dmitry

Re: [PATCH v4 04/11] leds: aw200xx: calculate dts property display_rows in the driver

[Lee Jones]

On Fri, 24 Nov 2023, Dmitry Rokosov wrote:

> On Thu, Nov 23, 2023 at 04:32:52PM +0000, Lee Jones wrote:
> > On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> > 
> > > From: George Stark <gnstark@salutedevices.com>
> > > 
> > > Get rid of device tree property "awinic,display-rows". The property
> > > value actually means number of current switches and depends on how leds
> > 
> > Nit: LEDs
> > 
> > > are connected to the device. It should be calculated manually by max
> > > used led number. In the same way it is computed automatically now.
> > 
> > As above - I won't mention this again.
> > 
> > > Max used led is taken from led definition subnodes.
> > > 
> > > Signed-off-by: George Stark <gnstark@salutedevices.com>
> > > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > > ---
> > >  drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
> > >  1 file changed, 27 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> > > index 7762b3a132ac..4bce5e7381c0 100644
> > > --- a/drivers/leds/leds-aw200xx.c
> > > +++ b/drivers/leds/leds-aw200xx.c
> > > @@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
> > >  	return gpiod_set_value_cansleep(chip->hwen, 0);
> > >  }
> > >  
> > > +static bool 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)
> > 
> > Shouldn't the second clause fail instantly?
> > 
> 
> We already have such logic in the aw200xx_probe_fw() function, which
> skips the LED node with the wrong reg value too. Furthermore, we have
> strict reg constraints in the dt-bindings parts (in the current patch
> series), so we assume that the DT developer will not create an LED with
> the wrong reg value.

Why is it being checked again then?

-- 
Lee Jones [李琼斯]

Re: [PATCH v4 04/11] leds: aw200xx: calculate dts property display_rows in the driver

[Dmitry Rokosov]

Lee,

Thank you for the quick reply!

On Mon, Nov 27, 2023 at 08:57:55AM +0000, Lee Jones wrote:
> On Fri, 24 Nov 2023, Dmitry Rokosov wrote:
> 
> > On Thu, Nov 23, 2023 at 04:32:52PM +0000, Lee Jones wrote:
> > > On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> > > 
> > > > From: George Stark <gnstark@salutedevices.com>
> > > > 
> > > > Get rid of device tree property "awinic,display-rows". The property
> > > > value actually means number of current switches and depends on how leds
> > > 
> > > Nit: LEDs
> > > 
> > > > are connected to the device. It should be calculated manually by max
> > > > used led number. In the same way it is computed automatically now.
> > > 
> > > As above - I won't mention this again.
> > > 
> > > > Max used led is taken from led definition subnodes.
> > > > 
> > > > Signed-off-by: George Stark <gnstark@salutedevices.com>
> > > > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > > > ---
> > > >  drivers/leds/leds-aw200xx.c | 39 +++++++++++++++++++++++++------------
> > > >  1 file changed, 27 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> > > > index 7762b3a132ac..4bce5e7381c0 100644
> > > > --- a/drivers/leds/leds-aw200xx.c
> > > > +++ b/drivers/leds/leds-aw200xx.c
> > > > @@ -379,6 +379,30 @@ static void aw200xx_disable(const struct aw200xx *const chip)
> > > >  	return gpiod_set_value_cansleep(chip->hwen, 0);
> > > >  }
> > > >  
> > > > +static bool 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)
> > > 
> > > Shouldn't the second clause fail instantly?
> > > 
> > 
> > We already have such logic in the aw200xx_probe_fw() function, which
> > skips the LED node with the wrong reg value too. Furthermore, we have
> > strict reg constraints in the dt-bindings parts (in the current patch
> > series), so we assume that the DT developer will not create an LED with
> > the wrong reg value.
> 
> Why is it being checked again then?

Hmmm, aw200xx_probe_get_display_rows() executes before the old
implementation... So we need to check it again. Do you think it should
be reworked? I've already sent a new patchset. Could you please take a
look at the other fixes?

-- 
Thank you,
Dmitry

[PATCH v4 05/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>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../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 3da3633a242c..a6dced59599d 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
-
   enable-gpios:
     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
 
@@ -105,7 +86,6 @@ examples:
             reg = <0x3a>;
             #address-cells = <1>;
             #size-cells = <0>;
-            awinic,display-rows = <3>;
             enable-gpios = <&gpio 3 GPIO_ACTIVE_HIGH>;
 
             led@0 {
-- 
2.36.0

[PATCH v4 06/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 1ms 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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 4bce5e7381c0..bb17e48b3e2a 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 */
+	fsleep(1000);
+
 	regcache_mark_dirty(chip->regmap);
 	return regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
 }
-- 
2.36.0

Re: [PATCH v4 06/11] leds: aw200xx: add delay after software reset

[Lee Jones]

On Tue, 21 Nov 2023, Dmitry Rokosov wrote:

> From: George Stark <gnstark@salutedevices.com>
> 
> According to datasheets of aw200xx devices software reset takes at
> least 1ms so add delay after reset before issuing commands to device.

Are you able to use an auto-correct tool to sharpen the grammar a little?

> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 4bce5e7381c0..bb17e48b3e2a 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 */

Please start sentences with an uppercase char.

"According to the datasheet, software resets take at least 1ms"
              ^                            ^     ^

> +	fsleep(1000);
> +
>  	regcache_mark_dirty(chip->regmap);
>  	return regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
>  }
> -- 
> 2.36.0
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v4 06/11] leds: aw200xx: add delay after software reset

[Dmitry Rokosov]

Hello Lee,

Thank you for the detailed review!

Please find my answer below.

On Thu, Nov 23, 2023 at 04:38:16PM +0000, Lee Jones wrote:
> On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> 
> > From: George Stark <gnstark@salutedevices.com>
> > 
> > According to datasheets of aw200xx devices software reset takes at
> > least 1ms so add delay after reset before issuing commands to device.
> 
> Are you able to use an auto-correct tool to sharpen the grammar a little?
> 
> > Signed-off-by: George Stark <gnstark@salutedevices.com>
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 4bce5e7381c0..bb17e48b3e2a 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 */
> 
> Please start sentences with an uppercase char.
> 
> "According to the datasheet, software resets take at least 1ms"
>               ^                            ^     ^
> 

Here it's only one 'software reset' mentioned.

> > +	fsleep(1000);
> > +
> >  	regcache_mark_dirty(chip->regmap);
> >  	return regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
> >  }
> > -- 
> > 2.36.0
> > 
> 
> -- 
> Lee Jones [李琼斯]

-- 
Thank you,
Dmitry

Re: [PATCH v4 06/11] leds: aw200xx: add delay after software reset

[Lee Jones]

On Fri, 24 Nov 2023, Dmitry Rokosov wrote:

> Hello Lee,
> 
> Thank you for the detailed review!
> 
> Please find my answer below.
> 
> On Thu, Nov 23, 2023 at 04:38:16PM +0000, Lee Jones wrote:
> > On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> > 
> > > From: George Stark <gnstark@salutedevices.com>
> > > 
> > > According to datasheets of aw200xx devices software reset takes at
> > > least 1ms so add delay after reset before issuing commands to device.
> > 
> > Are you able to use an auto-correct tool to sharpen the grammar a little?
> > 
> > > Signed-off-by: George Stark <gnstark@salutedevices.com>
> > > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 4bce5e7381c0..bb17e48b3e2a 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 */
> > 
> > Please start sentences with an uppercase char.
> > 
> > "According to the datasheet, software resets take at least 1ms"
> >               ^                            ^     ^
> > 
> 
> Here it's only one 'software reset' mentioned.

That's okay.  The English is still 100% valid, since this describes them
happening more than once; say per week, per year, per lifetime of the
H/W or some such.  If you *really* want to describe one reset happening
once, ever, then you can say "a software reset takes".

> > > +	fsleep(1000);
> > > +
> > >  	regcache_mark_dirty(chip->regmap);
> > >  	return regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
> > >  }

-- 
Lee Jones [李琼斯]

[PATCH v4 07/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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.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 bb17e48b3e2a..4c83d4979cf5 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -524,6 +524,7 @@ static const struct regmap_config aw200xx_regmap_config = {
 	.rd_table = &aw200xx_readable_table,
 	.wr_table = &aw200xx_writeable_table,
 	.cache_type = REGCACHE_MAPLE,
+	.disable_locking = true,
 };
 
 static int aw200xx_probe(struct i2c_client *client)
-- 
2.36.0

[PATCH v4 08/11] leds: aw200xx: improve autodim calculation method

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

It is highly recommended to leverage the DIV_ROUND_UP() function as a
more refined and mathematically precise alternative to employing a
coarse division method.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/leds/leds-aw200xx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 4c83d4979cf5..c48aa11fd411 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)
@@ -460,6 +460,7 @@ static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
 		led->num = source;
 		led->chip = chip;
 		led->cdev.brightness_set_blocking = aw200xx_brightness_set;
+		led->cdev.max_brightness = AW200XX_FADE_MAX;
 		led->cdev.groups = dim_groups;
 		init_data.fwnode = child;
 
-- 
2.36.0

[PATCH v4 09/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 family.
New device supports 108 LEDs using a matrix of 12x9 outputs.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/leds/Kconfig        | 14 +++++++++-----
 drivers/leds/leds-aw200xx.c | 10 +++++++++-
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6046dfeca16f..a879628e985c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -95,14 +95,18 @@ 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,
-	  3 pattern controllers for auto breathing or group dimming control.
+	  This option enables support for Awinic AW200XX LED controller.
+	  It is a matrix LED driver programmed via an I2C interface. Devices have
+	  a set of individually controlled leds and support 3 pattern controllers
+	  for auto breathing or group dimming control. Supported devices:
+	    - AW20036 (3x12) 36 LEDs
+	    - AW20054 (6x9)  54 LEDs
+	    - AW20072 (6x12) 72 LEDs
+	    - AW20108 (9x12) 108 LEDs
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-aw200xx.
diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index c48aa11fd411..4b5036360887 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.
  *
@@ -620,10 +620,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);
@@ -632,6 +639,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

Re: [PATCH v4 09/11] leds: aw200xx: add support for aw20108 device

[Lee Jones]

On Tue, 21 Nov 2023, Dmitry Rokosov wrote:

> From: George Stark <gnstark@salutedevices.com>
> 
> Add support for Awinic aw20108 device from the same LED drivers family.
> New device supports 108 LEDs using a matrix of 12x9 outputs.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/leds/Kconfig        | 14 +++++++++-----
>  drivers/leds/leds-aw200xx.c | 10 +++++++++-
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6046dfeca16f..a879628e985c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -95,14 +95,18 @@ 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,
> -	  3 pattern controllers for auto breathing or group dimming control.
> +	  This option enables support for Awinic AW200XX LED controller.

"for ..." THE or AN.

Or put an 's' at the end of "controller".

> +	  It is a matrix LED driver programmed via an I2C interface. Devices have
> +	  a set of individually controlled leds and support 3 pattern controllers

LEDs

> +	  for auto breathing or group dimming control. Supported devices:
> +	    - AW20036 (3x12) 36 LEDs
> +	    - AW20054 (6x9)  54 LEDs
> +	    - AW20072 (6x12) 72 LEDs
> +	    - AW20108 (9x12) 108 LEDs
>  
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-aw200xx.
> diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> index c48aa11fd411..4b5036360887 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.
>   *
> @@ -620,10 +620,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);
> @@ -632,6 +639,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
> 

-- 
Lee Jones [李琼斯]

[PATCH v4 10/11] dt-bindings: leds: awinic,aw200xx: add AW20108 device

[Dmitry Rokosov]

From: George Stark <gnstark@salutedevices.com>

Add aw20108 compatible for Awinic AW20108 led controller.

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../devicetree/bindings/leds/awinic,aw200xx.yaml    | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
index a6dced59599d..67c1d960db1d 100644
--- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -10,15 +10,19 @@ 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.
+  It is a matrix LED driver programmed via an I2C interface. Devices have
+  a set of individually controlled leds and support 3 pattern controllers
+  for auto breathing or group dimming control. Supported devices:
+    - AW20036 (3x12) 36 LEDs
+    - AW20054 (6x9)  54 LEDs
+    - AW20072 (6x12) 72 LEDs
+    - AW20108 (9x12) 108 LEDs
 
   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 +30,7 @@ properties:
       - awinic,aw20036
       - awinic,aw20054
       - awinic,aw20072
+      - awinic,aw20108
 
   reg:
     maxItems: 1
-- 
2.36.0

[PATCH v4 11/11] dt-bindings: leds: aw200xx: fix led pattern and add reg constraints

[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.

The register offsets can be adjusted within the specified range, with
the maximum value corresponding to the highest number of LEDs that can
be connected to the controller.

Fixes: e338a05e76ca ("dt-bindings: leds: Add binding for AW200xx")
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/leds/awinic,aw200xx.yaml         | 59 ++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
index 67c1d960db1d..54d6d1f08e24 100644
--- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -45,7 +45,7 @@ properties:
     maxItems: 1
 
 patternProperties:
-  "^led@[0-9a-f]$":
+  "^led@[0-9a-f]+$":
     type: object
     $ref: common.yaml#
     unevaluatedProperties: false
@@ -69,6 +69,63 @@ patternProperties:
           where max-current-switch-number is determinated by led configuration
           and depends on how leds are physically connected to the led driver.
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: awinic,aw20036
+    then:
+      patternProperties:
+        "^led@[0-9a-f]+$":
+          properties:
+            reg:
+              items:
+                minimum: 0
+                maximum: 36
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: awinic,aw20054
+    then:
+      patternProperties:
+        "^led@[0-9a-f]+$":
+          properties:
+            reg:
+              items:
+                minimum: 0
+                maximum: 54
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: awinic,aw20072
+    then:
+      patternProperties:
+        "^led@[0-9a-f]+$":
+          properties:
+            reg:
+              items:
+                minimum: 0
+                maximum: 72
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: awinic,aw20108
+    then:
+      patternProperties:
+        "^led@[0-9a-f]+$":
+          properties:
+            reg:
+              items:
+                minimum: 0
+                maximum: 108
+
 required:
   - compatible
   - reg
-- 
2.36.0