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/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
[4] https://lore.kernel.org/all/[email protected]/
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
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]>
Reviewed-by: Andy Shevchenko <[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 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
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]>
---
.../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 <[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,
- 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
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]>
Acked-by: Rob Herring <[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 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
From: George Stark <[email protected]>
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 <[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 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
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]>
Reviewed-by: Andy Shevchenko <[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 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
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]>
Reviewed-by: Conor Dooley <[email protected]>
---
.../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
On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
> From: George Stark <[email protected]>
>
> 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 <[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 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 [李琼斯]
On Thu, Nov 23, 2023 at 04:32:52PM +0000, Lee Jones wrote:
> On Tue, 21 Nov 2023, Dmitry Rokosov wrote:
>
> > From: George Stark <[email protected]>
> >
> > 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 <[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 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
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 <[email protected]>
> > >
> > > 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 <[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 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 [李琼斯]
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 <[email protected]>
> > > >
> > > > 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 <[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 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