| Message ID | 20231006160437.15627-6-ddrokosov@salutedevices.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: aw200xx: several driver updates | expand |
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)?
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.
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 --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);