| Message ID | 20231006160437.15627-1-ddrokosov@salutedevices.com |
|---|---|
| Headers | show |
| Series | leds: aw200xx: several driver updates | expand |
On Fri, Oct 6, 2023 at 7:04 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 page1 to page5 can be pages 1 to 5 > configured via i2c interface. ... > +#include <linux/of_gpio.h> Definitely not. Use agnostic APIs. ... > @@ -116,6 +117,7 @@ struct aw200xx { > struct mutex mutex; > u32 num_leds; > u32 display_rows; > + int hwen; > struct aw200xx_led leds[]; Side note: add a patch to use __counted_by() here. > }; ... > + if (!gpio_is_valid(chip->hwen)) Absolutely not. You may not use legacy GPIO APIs. > + return; > + > + gpio_set_value(chip->hwen, 1); Ditto. ... > + usleep_range(400, 500); fsleep() ? ... > +static void aw200xx_disable(const struct aw200xx *const chip) > +{ > + if (gpio_is_valid(chip->hwen)) > + gpio_set_value(chip->hwen, 0); > +} As per above. ... > +static void aw200xx_probe_hwen(struct device *dev, struct aw200xx *chip) > +{ > + chip->hwen = of_get_named_gpio(dev->of_node, "awinic,hwen-gpio", 0); > + if (gpio_is_valid(chip->hwen)) > + if (devm_gpio_request_one(dev, chip->hwen, GPIOF_OUT_INIT_HIGH, > + "AW200XX HWEN")) { > + dev_warn(dev, "Can't request gpio %d, tag it invalid\n", > + chip->hwen); > + chip->hwen = -EINVAL; > + } > +} Please, rewrite this completely using supported APIs and not deprecated or obsolete ones.
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 6, 2023 at 7:05 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. > + /* according to datasheet software reset takes at least 1ms */ Please, be consistent in all patches in all commit messages and code comments on how you supply physical units (w/ space or w/o, take also into consideration traditional scientific practices). > + usleep_range(1000, 2000); fsleep() ?
On Fri, Oct 6, 2023 at 7:05 PM Dmitry Rokosov <ddrokosov@salutedevices.com> wrote: > > From: George Stark <gnstark@salutedevices.com> > > use DIV_ROUND_UP instead of coarse div Please, respect English grammar and punctuation. Refer to macros and functions as func() (note the parentheses). ... > #define AW200XX_REG_DIM2FADE(x) ((x) + 1) > +#define AW200XX_REG_FADE2DIM(fade) \ > + DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX) Have you checked if the overflow is _now_ possible (compiling on 32-bit platforms as well)?
Hello Andy, Thanks a lot for such a quick review! Please find my comments below. On Fri, Oct 06, 2023 at 08:57:23PM +0300, Andy Shevchenko wrote: > On Fri, Oct 6, 2023 at 7:04 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 page1 to page5 can be > > pages 1 to 5 > > Sure, you are totally right. > > configured via i2c interface. > > ... > > > +#include <linux/of_gpio.h> > > Definitely not. > > Use agnostic APIs. Sure > > @@ -116,6 +117,7 @@ struct aw200xx { > > struct mutex mutex; > > > u32 num_leds; > > u32 display_rows; > > + int hwen; > > struct aw200xx_led leds[]; > > Side note: add a patch to use __counted_by() here. Okay, now I see the patch with __counted_by() some days ago. I will rebase on it. > > + if (!gpio_is_valid(chip->hwen)) > > Absolutely not. You may not use legacy GPIO APIs. Exactly > > + return; > > + > > + gpio_set_value(chip->hwen, 1); > > Ditto. Ok > > + usleep_range(400, 500); > > fsleep() ? Agreed. In this situation fsleep() automatic behaviour is acceptable. > > ... > > > +static void aw200xx_disable(const struct aw200xx *const chip) > > +{ > > + if (gpio_is_valid(chip->hwen)) > > + gpio_set_value(chip->hwen, 0); > > +} > > As per above. Ok > > +static void aw200xx_probe_hwen(struct device *dev, struct aw200xx *chip) > > +{ > > + chip->hwen = of_get_named_gpio(dev->of_node, "awinic,hwen-gpio", 0); > > + if (gpio_is_valid(chip->hwen)) > > + if (devm_gpio_request_one(dev, chip->hwen, GPIOF_OUT_INIT_HIGH, > > + "AW200XX HWEN")) { > > + dev_warn(dev, "Can't request gpio %d, tag it invalid\n", > > + chip->hwen); > > + chip->hwen = -EINVAL; > > + } > > +} > > Please, rewrite this completely using supported APIs and not > deprecated or obsolete ones. Yep, I see. I will use devm_gpiod_* API.
On Fri, Oct 06, 2023 at 09:01:39PM +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> > > > > According to datasheets of aw200xx devices software reset takes at > > least 1 ms so add delay after reset before issuing commands to device. > > > + /* according to datasheet software reset takes at least 1ms */ > > Please, be consistent in all patches in all commit messages and code > comments on how you supply physical units (w/ space or w/o, take also > into consideration traditional scientific practices). > > > + usleep_range(1000, 2000); > > fsleep() ? Good catch! Thank you for pointing!
On Fri, Oct 06, 2023 at 09:03:47PM +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> > > > > use DIV_ROUND_UP instead of coarse div > > Please, respect English grammar and punctuation. > Refer to macros and functions as func() (note the parentheses). > > ... > > > #define AW200XX_REG_DIM2FADE(x) ((x) + 1) > > +#define AW200XX_REG_FADE2DIM(fade) \ > > + DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX) > > Have you checked if the overflow is _now_ possible (compiling on > 32-bit platforms as well)? I suppose we shouldn't carry on about overflow here because the value of fade cannot exceed 255, and DIM_MAX is set at 63 You can find maximum values of fade and dim in the aw200xx driver header: #define AW200XX_DIM_MAX (BIT(6) - 1) #define AW200XX_FADE_MAX (BIT(8) - 1)
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.
On Mon, Oct 09, 2023 at 04:18:08PM +0300, Dmitry Rokosov wrote: > On Fri, Oct 06, 2023 at 09:03:47PM +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> > > > > > > use DIV_ROUND_UP instead of coarse div > > > > Please, respect English grammar and punctuation. > > Refer to macros and functions as func() (note the parentheses). > > > > ... > > > > > #define AW200XX_REG_DIM2FADE(x) ((x) + 1) > > > +#define AW200XX_REG_FADE2DIM(fade) \ > > > + DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX) > > > > Have you checked if the overflow is _now_ possible (compiling on > > 32-bit platforms as well)? > > I suppose we shouldn't carry on about overflow here because the value of > fade cannot exceed 255, and DIM_MAX is set at 63 > > You can find maximum values of fade and dim in the aw200xx driver > header: > > #define AW200XX_DIM_MAX (BIT(6) - 1) > #define AW200XX_FADE_MAX (BIT(8) - 1) I agree that checking if the fade is not greater than FADE_MAX will not be excessive. The LED subsystem is capable of sending brightness levels higher than 255
On 10/9/23 17:02, Dmitry Rokosov wrote: > On Mon, Oct 09, 2023 at 04:18:08PM +0300, Dmitry Rokosov wrote: >> On Fri, Oct 06, 2023 at 09:03:47PM +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> >>>> >>>> use DIV_ROUND_UP instead of coarse div >>> >>> Please, respect English grammar and punctuation. >>> Refer to macros and functions as func() (note the parentheses). >>> >>> ... >>> >>>> #define AW200XX_REG_DIM2FADE(x) ((x) + 1) >>>> +#define AW200XX_REG_FADE2DIM(fade) \ >>>> + DIV_ROUND_UP((fade) * AW200XX_DIM_MAX, AW200XX_FADE_MAX) >>> >>> Have you checked if the overflow is _now_ possible (compiling on >>> 32-bit platforms as well)? >> >> I suppose we shouldn't carry on about overflow here because the value of >> fade cannot exceed 255, and DIM_MAX is set at 63 >> >> You can find maximum values of fade and dim in the aw200xx driver >> header: >> >> #define AW200XX_DIM_MAX (BIT(6) - 1) >> #define AW200XX_FADE_MAX (BIT(8) - 1) > > I agree that checking if the fade is not greater than FADE_MAX will not > be excessive. The LED subsystem is capable of sending brightness levels > higher than 255 > Probably we should set max_brightness to AW200XX_FADE_MAX in led_classdev when adding leds.
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. >
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; - 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]. Links: [1] https://doc.awinic.com/doc/20230609wm/8a9a9ac8-1d8f-4e75-bf7a-67a04465c153.pdf Dmitry Rokosov (3): dt-bindings: leds: aw200xx: fix led dt node pattern leds: aw200xx: support HWEN hardware control dt-bindings: leds: aw200xx: introduce optional hwen-gpio property 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: Add binding for AW20108 led driver Martin Kurbanov (1): leds: aw200xx: fix write to DIM parameter .../bindings/leds/awinic,aw200xx.yaml | 42 +++---- drivers/leds/Kconfig | 8 +- drivers/leds/leds-aw200xx.c | 112 +++++++++++++++--- 3 files changed, 114 insertions(+), 48 deletions(-)