From: George Stark <gnstark@salutedevices.com>

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

Signed-off-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 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

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

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

Hello Andy

On 10/19/23 12:01, Andy Shevchenko wrote:
> On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov
> <ddrokosov@salutedevices.com> wrote:
>>
>> From: George Stark <gnstark@salutedevices.com>
>>
>> Get rid of device tree property "awinic,display-rows" and calculate it
>> in driver using led definition nodes. display-row actually means number
>> of current switches and depends on how leds are connected to the device.
> 
> 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