diff mbox series

[v1,05/11] leds: aw200xx: calculate dts property display_rows in driver

Message ID 20231006160437.15627-6-ddrokosov@salutedevices.com
State New
Headers
Series leds: aw200xx: several driver updates |

Commit Message

Dmitry Rokosov Oct. 6, 2023, 4:04 p.m. UTC 
  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 | 40 ++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 12 deletions(-)

Comments

Andy Shevchenko Oct. 6, 2023, 5:59 p.m. UTC | #1 
On Fri, Oct 6, 2023 at 7:05 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.

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)?
Dmitry Rokosov Oct. 9, 2023, 1:20 p.m. UTC | #2 
On Fri, Oct 06, 2023 at 08:59:46PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 6, 2023 at 7:05 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.
> 
> 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.
George Stark Oct. 16, 2023, 11:30 p.m. UTC | #3 
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
>> <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.
>>
>> 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.
>
diff mbox series

Patch

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);