| Message ID | 20231006160437.15627-4-ddrokosov@salutedevices.com |
|---|---|
| State | New |
| 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.
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.
diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c index 4c1e7caf8941..d92c082d4ab3 100644 --- a/drivers/leds/leds-aw200xx.c +++ b/drivers/leds/leds-aw200xx.c @@ -15,6 +15,7 @@ #include <linux/mod_devicetable.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_gpio.h> #include <linux/regmap.h> #include <linux/time.h> #include <linux/units.h> @@ -116,6 +117,7 @@ struct aw200xx { struct mutex mutex; u32 num_leds; u32 display_rows; + int hwen; struct aw200xx_led leds[]; }; @@ -358,6 +360,29 @@ static int aw200xx_chip_check(const struct aw200xx *const chip) return 0; } +static void aw200xx_enable(const struct aw200xx *const chip) +{ + if (!gpio_is_valid(chip->hwen)) + return; + + gpio_set_value(chip->hwen, 1); + + /* + * 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 + * configured via i2c interface. + */ + usleep_range(400, 500); +} + +static void aw200xx_disable(const struct aw200xx *const chip) +{ + if (gpio_is_valid(chip->hwen)) + gpio_set_value(chip->hwen, 0); +} + static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip) { struct fwnode_handle *child; @@ -445,6 +470,18 @@ static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip) return aw200xx_set_imax(chip, min_uA); } +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; + } +} + static const struct regmap_range_cfg aw200xx_ranges[] = { { .name = "aw200xx", @@ -517,6 +554,10 @@ static int aw200xx_probe(struct i2c_client *client) if (IS_ERR(chip->regmap)) return PTR_ERR(chip->regmap); + aw200xx_probe_hwen(&client->dev, chip); + + aw200xx_enable(chip); + ret = aw200xx_chip_check(chip); if (ret) return ret; @@ -537,6 +578,9 @@ static int aw200xx_probe(struct i2c_client *client) ret = aw200xx_chip_init(chip); out_unlock: + if (ret) + aw200xx_disable(chip); + mutex_unlock(&chip->mutex); return ret; } @@ -546,6 +590,7 @@ static void aw200xx_remove(struct i2c_client *client) struct aw200xx *chip = i2c_get_clientdata(client); aw200xx_chip_reset(chip); + aw200xx_disable(chip); mutex_destroy(&chip->mutex); }
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 configured via i2c interface. Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com> --- drivers/leds/leds-aw200xx.c | 45 +++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)