diff mbox series

[v2,5/5] iio: sx9310: Enable vdd and svdd regulators at probe

Message ID 20200724213329.899216-6-swboyd@chromium.org (mailing list archive)
State New, archived
Headers show
Series Some sx9310 iio driver updates | expand

Commit Message

Stephen Boyd July 24, 2020, 9:33 p.m. UTC
Enable the main power supply (vdd) and digital IO power supply (svdd)
during probe so that the i2c communication and device works properly on
boards that aggressively power gate these supplies.

Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Daniel Campello <campello@chromium.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/iio/proximity/sx9310.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Doug Anderson July 24, 2020, 10:02 p.m. UTC | #1
Hi,

On Fri, Jul 24, 2020 at 2:33 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Enable the main power supply (vdd) and digital IO power supply (svdd)
> during probe so that the i2c communication and device works properly on
> boards that aggressively power gate these supplies.
>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Daniel Campello <campello@chromium.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/iio/proximity/sx9310.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index 1e1f6bba50f6..ad6ed100c7a6 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -19,6 +19,7 @@
>  #include <linux/of.h>
>  #include <linux/pm.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>
>  #include <linux/iio/buffer.h>
> @@ -899,12 +900,21 @@ static int sx9310_set_indio_dev_name(struct device *dev,
>         return 0;
>  }
>
> +static void sx9310_regulator_disable(void *supplies)
> +{
> +       regulator_bulk_disable(2, supplies);
> +}
> +
>  static int sx9310_probe(struct i2c_client *client,
>                         const struct i2c_device_id *id)
>  {
>         int ret;
>         struct iio_dev *indio_dev;
>         struct sx9310_data *data;
> +       struct regulator_bulk_data supplies[] = {
> +               { .supply = "vdd" },
> +               { .supply = "svdd" },
> +       };
>
>         indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>         if (indio_dev == NULL)
> @@ -919,6 +929,23 @@ static int sx9310_probe(struct i2c_client *client,
>         if (IS_ERR(data->regmap))
>                 return PTR_ERR(data->regmap);
>
> +       ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(supplies), supplies);
> +       if (ret)
> +               return ret;
> +
> +       ret = regulator_bulk_enable(ARRAY_SIZE(supplies), supplies);
> +       if (ret)
> +               return ret;
> +       /* Must wait for Tpor time after initial power up */
> +       usleep_range(1000, 1100);
> +
> +       /* Update sx9310_regulator_disable() size if this bug is hit */
> +       BUILD_BUG_ON(ARRAY_SIZE(supplies) != 2);
> +       ret = devm_add_action_or_reset(&client->dev, sx9310_regulator_disable,
> +                                      supplies);

...but, but...  Aren't you storing a pointer to stack memory?  How
does that work?  I think you either need to store the "struct
regulator_bulk_data" in your private data or just make two normal
regulator calls.

-Doug
Stephen Boyd July 25, 2020, 12:38 a.m. UTC | #2
Quoting Doug Anderson (2020-07-24 15:02:23)
> On Fri, Jul 24, 2020 at 2:33 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > index 1e1f6bba50f6..ad6ed100c7a6 100644
> > --- a/drivers/iio/proximity/sx9310.c
> > +++ b/drivers/iio/proximity/sx9310.c
> > @@ -919,6 +929,23 @@ static int sx9310_probe(struct i2c_client *client,
> >         if (IS_ERR(data->regmap))
> >                 return PTR_ERR(data->regmap);
> >
> > +       ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(supplies), supplies);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = regulator_bulk_enable(ARRAY_SIZE(supplies), supplies);
> > +       if (ret)
> > +               return ret;
> > +       /* Must wait for Tpor time after initial power up */
> > +       usleep_range(1000, 1100);
> > +
> > +       /* Update sx9310_regulator_disable() size if this bug is hit */
> > +       BUILD_BUG_ON(ARRAY_SIZE(supplies) != 2);
> > +       ret = devm_add_action_or_reset(&client->dev, sx9310_regulator_disable,
> > +                                      supplies);
> 
> ...but, but...  Aren't you storing a pointer to stack memory?  How
> does that work?  I think you either need to store the "struct
> regulator_bulk_data" in your private data or just make two normal
> regulator calls.
> 

Doh, no coffee today.
diff mbox series

Patch

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 1e1f6bba50f6..ad6ed100c7a6 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -19,6 +19,7 @@ 
 #include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 #include <linux/iio/buffer.h>
@@ -899,12 +900,21 @@  static int sx9310_set_indio_dev_name(struct device *dev,
 	return 0;
 }
 
+static void sx9310_regulator_disable(void *supplies)
+{
+	regulator_bulk_disable(2, supplies);
+}
+
 static int sx9310_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	int ret;
 	struct iio_dev *indio_dev;
 	struct sx9310_data *data;
+	struct regulator_bulk_data supplies[] = {
+		{ .supply = "vdd" },
+		{ .supply = "svdd" },
+	};
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (indio_dev == NULL)
@@ -919,6 +929,23 @@  static int sx9310_probe(struct i2c_client *client,
 	if (IS_ERR(data->regmap))
 		return PTR_ERR(data->regmap);
 
+	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(supplies), supplies);
+	if (ret)
+		return ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(supplies), supplies);
+	if (ret)
+		return ret;
+	/* Must wait for Tpor time after initial power up */
+	usleep_range(1000, 1100);
+
+	/* Update sx9310_regulator_disable() size if this bug is hit */
+	BUILD_BUG_ON(ARRAY_SIZE(supplies) != 2);
+	ret = devm_add_action_or_reset(&client->dev, sx9310_regulator_disable,
+				       supplies);
+	if (ret)
+		return ret;
+
 	ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami);
 	if (ret < 0) {
 		dev_err(&client->dev, "error in reading WHOAMI register: %d\n",