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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 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
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 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
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 <[email protected]>
---
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
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 | 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
From: George Stark <[email protected]>
Get rid of the property "awinic,display-rows" and calculate it
in the driver using led definition nodes.
Signed-off-by: George Stark <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
.../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
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 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
From: George Stark <[email protected]>
Add aw20108 compatible in devicetree binding for aw200xx led driver.
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 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 <[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]>
use DIV_ROUND_UP instead of coarse div
Signed-off-by: George Stark <[email protected]>
Signed-off-by: Dmitry Rokosov <[email protected]>
---
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
On Fri, Oct 6, 2023 at 7:04 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 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
On Fri, Oct 6, 2023 at 7:05 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.
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
On Fri, Oct 6, 2023 at 7:05 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.
> + /* 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
On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
<[email protected]> wrote:
>
> From: George Stark <[email protected]>
>
> 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
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
> <[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 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
On Fri, Oct 06, 2023 at 09:01:39PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 6, 2023 at 7:05 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.
>
> > + /* 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
On Fri, Oct 06, 2023 at 09:03:47PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov
> <[email protected]> wrote:
> >
> > From: George Stark <[email protected]>
> >
> > 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
On Fri, Oct 06, 2023 at 08:59:46PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 6, 2023 at 7:05 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.
>
> 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
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
> > <[email protected]> wrote:
> > >
> > > From: George Stark <[email protected]>
> > >
> > > 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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
On Fri, 06 Oct 2023 19:04:32 +0300, Dmitry Rokosov wrote:
> From: George Stark <[email protected]>
>
> Get rid of the property "awinic,display-rows" and calculate it
> in the driver using led definition nodes.
>
> Signed-off-by: George Stark <[email protected]>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> .../bindings/leds/awinic,aw200xx.yaml | 28 +++----------------
> 1 file changed, 4 insertions(+), 24 deletions(-)
>
Acked-by: Rob Herring <[email protected]>
On Fri, Oct 06, 2023 at 07:04:37PM +0300, Dmitry Rokosov wrote:
> From: George Stark <[email protected]>
>
> 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 <[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 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 <[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
>
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 <[email protected]>
> > ---
> > 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
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
>>> <[email protected]> wrote:
>>>>
>>>> From: George Stark <[email protected]>
>>>>
>>>> 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
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
>> <[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.
>>
>> 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