| Message ID | 20231018182943.18700-1-ddrokosov@salutedevices.com |
|---|---|
| Headers | show |
| Series | leds: aw200xx: several driver updates | expand |
On Wed, Oct 18, 2023 at 9:29 PM Dmitry Rokosov <ddrokosov@salutedevices.com> 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 page 1 to page 5 can be > configured via i2c interface. Is there any Documentation update for this new binding? ... > + chip->hwen = devm_gpiod_get_optional(&client->dev, "hwen", GPIOD_OUT_HIGH); With _optional APIs we distinguish 3 cases: found, not found, error. And error can be (among others) the deferred probe, meaning that GPIO _is coming_. Hence the rule of thumb for the _optional() APIs is to check for error and bail out on that condition (note, it's applicable to any _optional() APIs, not limited by GPIO library). ... > aw200xx_chip_reset(chip); > + aw200xx_disable(chip); It seems it can be modeled as a (GPIO) regulator. At least many drivers do so, but I leave this to the maintainers to decide.
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?
On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov <ddrokosov@salutedevices.com> wrote: > > From: George Stark <gnstark@salutedevices.com> > > According to datasheets of aw200xx devices software reset takes at > least 1 ms so add delay after reset before issuing commands to device. For the "us" unit you chose "XXXus" style, why is "ms" different? ... > + /* according to datasheet software reset takes at least 1 ms */ Ditto.
On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov <ddrokosov@salutedevices.com> wrote: > > From: George Stark <gnstark@salutedevices.com> > > Add support for Awinic aw20108 device from the same LED drivers famliy. family > New device supports 108 leds using matrix of 12x9 outputs. LEDs a matrix ... > - 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. For better maintenance I always suggest in the cases like this to convert help to provide a list of the supported devices, like: This option enables support for the following Awinic LED drivers: - AW20036 (3x12) - ... ... And if any new comes to this, it will be just a one liner change. -- With Best Regards, Andy Shevchenko
On Thu, Oct 19, 2023 at 12:10 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Oct 18, 2023 at 9:30 PM Dmitry Rokosov > <ddrokosov@salutedevices.com> wrote: ... > > - 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. > > For better maintenance I always suggest in the cases like this to > convert help to provide a list of the supported devices, like: > > This option enables support for the following Awinic LED drivers: > - AW20036 (3x12) (and other specifics can be listed in parentheses, or in free way, but short enough to occupy only a single line) > - ... > ... > > And if any new comes to this, it will be just a one liner change. And you may do that conversion as a precursor to this one and you will see what I mean.
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? >
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 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/20231006160437.15627-1-ddrokosov@salutedevices.com/ Dmitry Rokosov (3): leds: aw200xx: support HWEN hardware control dt-bindings: leds: aw200xx: introduce optional hwen-gpios property dt-bindings: leds: aw200xx: fix led pattern and add reg constraints 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: awinic,aw200xx: add AW20108 device Martin Kurbanov (1): leds: aw200xx: fix write to DIM parameter .../bindings/leds/awinic,aw200xx.yaml | 49 ++++------ drivers/leds/Kconfig | 8 +- drivers/leds/leds-aw200xx.c | 96 +++++++++++++++---- 3 files changed, 102 insertions(+), 51 deletions(-)