diff mbox series

[v2,03/14] mfd: pm8008: deassert reset on probe

Message ID 20240529162958.18081-4-johan+linaro@kernel.org
State New
Headers show
Series arm64: dts: qcom: sc8280xp-x13s: enable pm8008 camera pmic | expand

Commit Message

Johan Hovold May 29, 2024, 4:29 p.m. UTC
Request and deassert any (optional) reset gpio during probe in case it
has been left asserted by the boot firmware.

Note the reset line is not asserted to avoid reverting to the default
I2C address in case the firmware has configured an alternate address.

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/mfd/qcom-pm8008.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Andy Shevchenko May 29, 2024, 7:45 p.m. UTC | #1
On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Request and deassert any (optional) reset gpio during probe in case it
> has been left asserted by the boot firmware.
>
> Note the reset line is not asserted to avoid reverting to the default
> I2C address in case the firmware has configured an alternate address.

...

> +       /*
> +        * The PMIC does not appear to require a post-reset delay, but wait
> +        * for a millisecond for now anyway.
> +        */

> +       usleep_range(1000, 2000);

fsleep() ?
Johan Hovold May 30, 2024, 8:07 a.m. UTC | #2
On Wed, May 29, 2024 at 10:45:40PM +0300, Andy Shevchenko wrote:
> On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > Request and deassert any (optional) reset gpio during probe in case it
> > has been left asserted by the boot firmware.
> >
> > Note the reset line is not asserted to avoid reverting to the default
> > I2C address in case the firmware has configured an alternate address.
> 
> ...
> 
> > +       /*
> > +        * The PMIC does not appear to require a post-reset delay, but wait
> > +        * for a millisecond for now anyway.
> > +        */
> 
> > +       usleep_range(1000, 2000);
> 
> fsleep() ?

No, I'd only use fsleep() when the argument is variable.

Johan
Andy Shevchenko May 30, 2024, 8:34 a.m. UTC | #3
On Thu, May 30, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
> On Wed, May 29, 2024 at 10:45:40PM +0300, Andy Shevchenko wrote:
> > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> > >
> > > Request and deassert any (optional) reset gpio during probe in case it
> > > has been left asserted by the boot firmware.
> > >
> > > Note the reset line is not asserted to avoid reverting to the default
> > > I2C address in case the firmware has configured an alternate address.

...

> > > +       /*
> > > +        * The PMIC does not appear to require a post-reset delay, but wait
> > > +        * for a millisecond for now anyway.
> > > +        */
> >
> > > +       usleep_range(1000, 2000);
> >
> > fsleep() ?
>
> No, I'd only use fsleep() when the argument is variable.

Okay, this is basically the same issue as with use of dev_err_probe()
with known errors. fsleep() hides the choice between let's say
msleep() / usleep_range() / udelay() from the caller. This, in
particular, might allow shifting constraints if the timer core is
changed or becomes more granular. It's independent to the variable or
constant parameter(s). Whatever, I'm not going to insist.
Johan Hovold May 30, 2024, 8:51 a.m. UTC | #4
On Thu, May 30, 2024 at 11:34:55AM +0300, Andy Shevchenko wrote:
> On Thu, May 30, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
> > On Wed, May 29, 2024 at 10:45:40PM +0300, Andy Shevchenko wrote:
> > > On Wed, May 29, 2024 at 7:30 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> > > >
> > > > Request and deassert any (optional) reset gpio during probe in case it
> > > > has been left asserted by the boot firmware.
> > > >
> > > > Note the reset line is not asserted to avoid reverting to the default
> > > > I2C address in case the firmware has configured an alternate address.
> 
> ...
> 
> > > > +       /*
> > > > +        * The PMIC does not appear to require a post-reset delay, but wait
> > > > +        * for a millisecond for now anyway.
> > > > +        */
> > >
> > > > +       usleep_range(1000, 2000);
> > >
> > > fsleep() ?
> >
> > No, I'd only use fsleep() when the argument is variable.
> 
> Okay, this is basically the same issue as with use of dev_err_probe()
> with known errors. fsleep() hides the choice between let's say
> msleep() / usleep_range() / udelay() from the caller. This, in
> particular, might allow shifting constraints if the timer core is
> changed or becomes more granular. It's independent to the variable or
> constant parameter(s). Whatever, I'm not going to insist.

I prefer that developers are aware of what they are doing and understand
the difference between, say, usleep_range() and udelay(), instead of
hiding things away in obscure helper functions.

Johan
diff mbox series

Patch

diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c
index f71c490f25c8..5a77155a63d7 100644
--- a/drivers/mfd/qcom-pm8008.c
+++ b/drivers/mfd/qcom-pm8008.c
@@ -4,6 +4,7 @@ 
  */
 
 #include <linux/bitops.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -156,6 +157,7 @@  static struct regmap_config qcom_mfd_regmap_cfg = {
 static int pm8008_probe(struct i2c_client *client)
 {
 	struct regmap_irq_chip_data *irq_data;
+	struct gpio_desc *reset;
 	int rc;
 	struct device *dev;
 	struct regmap *regmap;
@@ -167,6 +169,16 @@  static int pm8008_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, regmap);
 
+	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
+
+	/*
+	 * The PMIC does not appear to require a post-reset delay, but wait
+	 * for a millisecond for now anyway.
+	 */
+	usleep_range(1000, 2000);
+
 	if (of_property_read_bool(dev->of_node, "interrupt-controller")) {
 		rc = devm_regmap_add_irq_chip(dev, regmap, client->irq,
 				IRQF_SHARED, 0, &pm8008_irq_chip, &irq_data);