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 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/[email protected]/
Dmitry Rokosov (3):
leds: aw200xx: support HWEN hardware control
dt-bindings: leds: aw200xx: introduce optional hwen-gpios property
dt-bindings: leds: aw200xx: fix led pattern and add reg constraints
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: awinic,aw200xx: add AW20108 device
Martin Kurbanov (1):
leds: aw200xx: fix write to DIM parameter
.../bindings/leds/awinic,aw200xx.yaml | 49 ++++------
drivers/leds/Kconfig | 8 +-
drivers/leds/leds-aw200xx.c | 96 +++++++++++++++----
3 files changed, 102 insertions(+), 51 deletions(-)
--
2.36.0
From: George Stark <[email protected]>
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 <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
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 911c3154585f..a2a31b8e623e 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -379,6 +379,31 @@ static void aw200xx_disable(const struct aw200xx *const chip)
return gpiod_set_value_cansleep(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;
+
+ max_source = max(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;
@@ -386,18 +411,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
Property 'hwen-gpios' 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 <[email protected]>
---
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..255eb0563737 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
+ hwen-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>;
+ hwen-gpios = <&gpio 3 GPIO_ACTIVE_HIGH>;
led@0 {
reg = <0x0>;
--
2.36.0
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 <[email protected]>
---
drivers/leds/leds-aw200xx.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 842a22087b16..911c3154585f 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,10 @@ 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, "hwen", GPIOD_OUT_HIGH);
+
+ aw200xx_enable(chip);
+
ret = aw200xx_chip_check(chip);
if (ret)
return ret;
@@ -537,6 +562,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 +574,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
From: George Stark <[email protected]>
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 <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
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 a2a31b8e623e..77760406abbf 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 1 ms */
+ fsleep(1000);
+
regcache_mark_dirty(chip->regmap);
return regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
}
--
2.36.0
From: George Stark <[email protected]>
In the driver regmap is always used under mutex so regmap's inner lock
can be disabled.
Signed-off-by: George Stark <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
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 77760406abbf..ad4bfb9f0938 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
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 <[email protected]>
---
.../devicetree/bindings/leds/awinic,aw200xx.yaml | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
index efb18ddce383..677c73aa6232 100644
--- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -42,16 +42,18 @@ properties:
maxItems: 1
patternProperties:
- "^led@[0-9a-f]$":
+ "^led@[0-9a-f]+$":
type: object
$ref: common.yaml#
unevaluatedProperties: false
properties:
reg:
- description:
- LED number
- maxItems: 1
+ items:
+ description:
+ LED number
+ minimum: 0
+ maximum: 108
led-max-microamp:
default: 9780
--
2.36.0
From: George Stark <[email protected]>
Add aw20108 compatible for Awinic AW20108 led controller.
Signed-off-by: George Stark <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
.../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 ee849ef3236a..efb18ddce383 100644
--- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
+++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
@@ -10,15 +10,16 @@ maintainers:
- Martin Kurbanov <[email protected]>
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
From: George Stark <[email protected]>
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 <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
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 ad4bfb9f0938..7b8802bd9497 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
From: George Stark <[email protected]>
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 <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
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 7b8802bd9497..529a4ab9c876 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.
*
@@ -616,10 +616,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);
@@ -628,6 +635,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
On Wed, Oct 18, 2023 at 9:29 PM Dmitry Rokosov
<[email protected]> 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.
Is there any Documentation update for this new binding?
...
> + chip->hwen = devm_gpiod_get_optional(&client->dev, "hwen", GPIOD_OUT_HIGH);
With _optional APIs we distinguish 3 cases: found, not found, error.
And error can be (among others) the deferred probe, meaning that GPIO
_is coming_. Hence the rule of thumb for the _optional() APIs is to
check for error and bail out on that condition (note, it's applicable
to any _optional() APIs, not limited by GPIO library).
...
> aw200xx_chip_reset(chip);
> + aw200xx_disable(chip);
It seems it can be modeled as a (GPIO) regulator. At least many
drivers do so, but I leave this to the maintainers to decide.
--
With Best Regards,
Andy Shevchenko
On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov
<[email protected]> wrote:
>
> From: George Stark <[email protected]>
>
> 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.
Still the commit message does not answer the question why it's safe
for the users that have this property enabled in their DTBs (note B
letter).
...
> + 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)
Perhaps a warning?
dev_warn(dev, "Unable to read from %pfw or apply a source channel
number\n", child);
> + continue;
> +
> + max_source = max(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;
So, this part is in ->probe() flow only, correct? If so,
return dev_err_probe(...);
> + }
...
> + if (aw200xx_probe_get_display_rows(dev, chip))
> + return -EINVAL;
Why is the error code shadowed?
--
With Best Regards,
Andy Shevchenko
On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov
<[email protected]> wrote:
>
> From: George Stark <[email protected]>
>
> According to datasheets of aw200xx devices software reset takes at
> least 1 ms so add delay after reset before issuing commands to device.
For the "us" unit you chose "XXXus" style, why is "ms" different?
...
> + /* according to datasheet software reset takes at least 1 ms */
Ditto.
--
With Best Regards,
Andy Shevchenko
On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov
<[email protected]> wrote:
>
> From: George Stark <[email protected]>
>
> Add support for Awinic aw20108 device from the same LED drivers famliy.
family
> New device supports 108 leds using matrix of 12x9 outputs.
LEDs
a matrix
...
> - 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.
For better maintenance I always suggest in the cases like this to
convert help to provide a list of the supported devices, like:
This option enables support for the following Awinic LED drivers:
- AW20036 (3x12)
- ...
...
And if any new comes to this, it will be just a one liner change.
--
With Best Regards,
Andy Shevchenko
On Thu, Oct 19, 2023 at 12:10 PM Andy Shevchenko
<[email protected]> wrote:
> On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov
> <[email protected]> wrote:
...
> > - 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.
>
> For better maintenance I always suggest in the cases like this to
> convert help to provide a list of the supported devices, like:
>
> This option enables support for the following Awinic LED drivers:
> - AW20036 (3x12)
(and other specifics can be listed in parentheses, or in free way, but
short enough to occupy only a single line)
> - ...
> ...
>
> And if any new comes to this, it will be just a one liner change.
And you may do that conversion as a precursor to this one and you will
see what I mean.
--
With Best Regards,
Andy Shevchenko
On Wed, Oct 18, 2023 at 09:29:42PM +0300, Dmitry Rokosov wrote:
> From: George Stark <[email protected]>
>
> Add aw20108 compatible for Awinic AW20108 led controller.
>
> Signed-off-by: George Stark <[email protected]>
> Signed-off-by: Dmitry Rokosov <[email protected]>
Acked-by: Conor Dooley <[email protected]>
Cheers,
Conor.
> ---
> .../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 ee849ef3236a..efb18ddce383 100644
> --- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> @@ -10,15 +10,16 @@ maintainers:
> - Martin Kurbanov <[email protected]>
>
> 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
>
>
On Wed, Oct 18, 2023 at 09:29:43PM +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.
>
> 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.
Do all of these controllers have identical max numbers of LEDs?
Cheers,
Conor.
>
> Fixes: e338a05e76ca ("dt-bindings: leds: Add binding for AW200xx")
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> .../devicetree/bindings/leds/awinic,aw200xx.yaml | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> index efb18ddce383..677c73aa6232 100644
> --- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> +++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> @@ -42,16 +42,18 @@ properties:
> maxItems: 1
>
> patternProperties:
> - "^led@[0-9a-f]$":
> + "^led@[0-9a-f]+$":
> type: object
> $ref: common.yaml#
> unevaluatedProperties: false
>
> properties:
> reg:
> - description:
> - LED number
> - maxItems: 1
> + items:
> + description:
> + LED number
> + minimum: 0
> + maximum: 108
>
> led-max-microamp:
> default: 9780
> --
> 2.36.0
>
On Wed, Oct 18, 2023 at 09:29:35PM +0300, Dmitry Rokosov wrote:
> Property 'hwen-gpios' is optional, it can be used by the board
> developer to connect AW200XX LED controller with appropriate poweron
> GPIO pad.
If the pad is called "poweron", why is the property called "hwen"?
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> 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..255eb0563737 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
>
> + hwen-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>;
> + hwen-gpios = <&gpio 3 GPIO_ACTIVE_HIGH>;
>
> led@0 {
> reg = <0x0>;
> --
> 2.36.0
>
Hello Andy
On 10/19/23 12:01, Andy Shevchenko wrote:
> On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov
> <[email protected]> wrote:
>>
>> From: George Stark <[email protected]>
>>
>> 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.
>
> Still the commit message does not answer the question why it's safe
> for the users that have this property enabled in their DTBs (note B
> letter).
>
> ...
>
>> + 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)
>
> Perhaps a warning?
>
> dev_warn(dev, "Unable to read from %pfw or apply a source channel
> number\n", child);
I skipped the warning intentionally because we have just the same loop
several steps below and with the same check. There we have all proper
warnings and aw200xx_probe_get_display_rows was intended to be short and
lightweight. I'll redesign it to be even more simple.
>
>> + continue;
>> +
>> + max_source = max(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;
>
> So, this part is in ->probe() flow only, correct? If so,
> return dev_err_probe(...);
>
>> + }
>
> ...
>
>> + if (aw200xx_probe_get_display_rows(dev, chip))
>> + return -EINVAL;
>
> Why is the error code shadowed?
>
--
Best regards
George
On Wed, Oct 18, 2023 at 09:29:35PM +0300, Dmitry Rokosov wrote:
> Property 'hwen-gpios' 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 <[email protected]>
> ---
> 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..255eb0563737 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
>
> + hwen-gpios:
> + maxItems: 1
The standard enable-gpios or powerdown-gpios don't work for you?
On Tue, Oct 24, 2023 at 01:30:14PM -0500, Rob Herring wrote:
> On Wed, Oct 18, 2023 at 09:29:35PM +0300, Dmitry Rokosov wrote:
> > Property 'hwen-gpios' 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 <[email protected]>
> > ---
> > 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..255eb0563737 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
> >
> > + hwen-gpios:
> > + maxItems: 1
>
> The standard enable-gpios or powerdown-gpios don't work for you?
HWEN is the name from the official datasheet. I thought it's always
better to use a naming convention that is similar to the notations used
in the datasheet.
--
Thank you,
Dmitry
On Thu, Oct 19, 2023 at 03:11:06PM +0100, Conor Dooley wrote:
> On Wed, Oct 18, 2023 at 09:29:35PM +0300, Dmitry Rokosov wrote:
> > Property 'hwen-gpios' is optional, it can be used by the board
> > developer to connect AW200XX LED controller with appropriate poweron
> > GPIO pad.
>
> If the pad is called "poweron", why is the property called "hwen"?
>
I have just referred to GPIO as 'poweron gpio', which is my own figure
of speech. In actuality, this pin is officially referred to as 'hwen' in
the datasheet.
> >
> > Signed-off-by: Dmitry Rokosov <[email protected]>
> > ---
> > 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..255eb0563737 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
> >
> > + hwen-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>;
> > + hwen-gpios = <&gpio 3 GPIO_ACTIVE_HIGH>;
> >
> > led@0 {
> > reg = <0x0>;
> > --
> > 2.36.0
> >
--
Thank you,
Dmitry
On Thu, Oct 19, 2023 at 03:08:38PM +0100, Conor Dooley wrote:
> On Wed, Oct 18, 2023 at 09:29:43PM +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.
> >
> > 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.
>
> Do all of these controllers have identical max numbers of LEDs?
Nope... I believe you are hinting at some conditional logic based on the
value of 'compatible'. I will figure it out and send the appropriate
implementation in the next version.
> >
> > Fixes: e338a05e76ca ("dt-bindings: leds: Add binding for AW200xx")
> > Signed-off-by: Dmitry Rokosov <[email protected]>
> > ---
> > .../devicetree/bindings/leds/awinic,aw200xx.yaml | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> > index efb18ddce383..677c73aa6232 100644
> > --- a/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> > +++ b/Documentation/devicetree/bindings/leds/awinic,aw200xx.yaml
> > @@ -42,16 +42,18 @@ properties:
> > maxItems: 1
> >
> > patternProperties:
> > - "^led@[0-9a-f]$":
> > + "^led@[0-9a-f]+$":
> > type: object
> > $ref: common.yaml#
> > unevaluatedProperties: false
> >
> > properties:
> > reg:
> > - description:
> > - LED number
> > - maxItems: 1
> > + items:
> > + description:
> > + LED number
> > + minimum: 0
> > + maximum: 108
> >
> > led-max-microamp:
> > default: 9780
> > --
> > 2.36.0
> >
--
Thank you,
Dmitry
On 24/10/2023 20:52, Dmitry Rokosov wrote:
> On Tue, Oct 24, 2023 at 01:30:14PM -0500, Rob Herring wrote:
>> On Wed, Oct 18, 2023 at 09:29:35PM +0300, Dmitry Rokosov wrote:
>>> Property 'hwen-gpios' 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 <[email protected]>
>>> ---
>>> 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..255eb0563737 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
>>>
>>> + hwen-gpios:
>>> + maxItems: 1
>>
>> The standard enable-gpios or powerdown-gpios don't work for you?
>
> HWEN is the name from the official datasheet. I thought it's always
> better to use a naming convention that is similar to the notations used
> in the datasheet.
I think we have such rule only for supplies, otherwise you will have
multiple variants of the same reset/enable/powerdown-gpios.
Best regards,
Krzysztof