[PATCH 1/4] dt-bindings: iio: magnetometer: bmc150: Document regulator supplies

[PATCH 1/4] dt-bindings: iio: magnetometer: bmc150: Document regulator supplies

[Stephan Gerhold]

BMC150 needs VDD and VDDIO regulators that might need to be explicitly
enabled. Document support for vdd/vddio-supply to implement this.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../bindings/iio/magnetometer/bosch,bmc150_magn.yaml           | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/bosch,bmc150_magn.yaml b/Documentation/devicetree/bindings/iio/magnetometer/bosch,bmc150_magn.yaml
index cdef7aeba708..2867ab6bf9b0 100644
--- a/Documentation/devicetree/bindings/iio/magnetometer/bosch,bmc150_magn.yaml
+++ b/Documentation/devicetree/bindings/iio/magnetometer/bosch,bmc150_magn.yaml
@@ -30,6 +30,9 @@ properties:
   reg:
     maxItems: 1
 
+  vdd-supply: true
+  vddio-supply: true
+
   interrupts:
     maxItems: 1
 
-- 
2.29.2

[PATCH 2/4] iio: magnetometer: bmc150: Add rudimentary regulator support

[Stephan Gerhold]

BMC150 needs VDD and VDDIO regulators that might need to be explicitly
enabled. Add some rudimentary support to obtain and enable these
regulators during probe() and disable them during remove()
or on the error path.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
This is mostly copy-paste of
079c1c3f2082 ("iio: accel: bmc150-accel: Add rudimentary regulator support")
from Linus Walleij but for the BMC150 magnetometer driver.
---
 drivers/iio/magnetometer/bmc150_magn.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index fa09fcab620a..b2f3129e1b4f 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -25,6 +25,7 @@
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 
 #include "bmc150_magn.h"
 
@@ -135,6 +136,7 @@ struct bmc150_magn_data {
 	 */
 	struct mutex mutex;
 	struct regmap *regmap;
+	struct regulator_bulk_data regulators[2];
 	struct iio_mount_matrix orientation;
 	/* 4 x 32 bits for x, y z, 4 bytes align, 64 bits timestamp */
 	s32 buffer[6];
@@ -692,12 +694,24 @@ static int bmc150_magn_init(struct bmc150_magn_data *data)
 	int ret, chip_id;
 	struct bmc150_magn_preset preset;
 
+	ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
+				    data->regulators);
+	if (ret < 0) {
+		dev_err(data->dev, "Failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+	/*
+	 * 3ms power-on time according to datasheet, let's better
+	 * be safe than sorry and set this delay to 5ms.
+	 */
+	msleep(5);
+
 	ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND,
 					 false);
 	if (ret < 0) {
 		dev_err(data->dev,
 			"Failed to bring up device from suspend mode\n");
-		return ret;
+		goto err_regulator_disable;
 	}
 
 	ret = regmap_read(data->regmap, BMC150_MAGN_REG_CHIP_ID, &chip_id);
@@ -752,6 +766,8 @@ static int bmc150_magn_init(struct bmc150_magn_data *data)
 
 err_poweroff:
 	bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
+err_regulator_disable:
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
 	return ret;
 }
 
@@ -867,6 +883,13 @@ int bmc150_magn_probe(struct device *dev, struct regmap *regmap,
 	data->irq = irq;
 	data->dev = dev;
 
+	data->regulators[0].supply = "vdd";
+	data->regulators[1].supply = "vddio";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
+				      data->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get regulators\n");
+
 	ret = iio_read_mount_matrix(dev, "mount-matrix",
 				&data->orientation);
 	if (ret)
@@ -984,6 +1007,7 @@ int bmc150_magn_remove(struct device *dev)
 	bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
 	mutex_unlock(&data->mutex);
 
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
 	return 0;
 }
 EXPORT_SYMBOL(bmc150_magn_remove);
-- 
2.29.2

Re: [PATCH 2/4] iio: magnetometer: bmc150: Add rudimentary regulator support

[Linus Walleij]

On Wed, Dec 2, 2020 at 10:33 AM Stephan Gerhold <stephan@gerhold.net> wrote:

> BMC150 needs VDD and VDDIO regulators that might need to be explicitly
> enabled. Add some rudimentary support to obtain and enable these
> regulators during probe() and disable them during remove()
> or on the error path.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

LGTM:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/4] iio: magnetometer: bmc150: Add rudimentary regulator support

[Stephan Gerhold]

Hi Jonathan,

On Wed, Dec 02, 2020 at 10:33:20AM +0100, Stephan Gerhold wrote:
> BMC150 needs VDD and VDDIO regulators that might need to be explicitly
> enabled. Add some rudimentary support to obtain and enable these
> regulators during probe() and disable them during remove()
> or on the error path.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

I think we forgot about the bmc150_magn part of this patch series. :)

This is likely my mistake - I only sent a v2 for the bmg160 part of this
patch series, but not for the bmc150_magn part (since it's not affected
by the problem you mentioned for the bmg160 patch). Sorry for the confusion!

Should I re-send the bmc150_magn part (with or without some changes)?

Thanks!
Stephan

> ---
> This is mostly copy-paste of
> 079c1c3f2082 ("iio: accel: bmc150-accel: Add rudimentary regulator support")
> from Linus Walleij but for the BMC150 magnetometer driver.
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index fa09fcab620a..b2f3129e1b4f 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -25,6 +25,7 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include "bmc150_magn.h"
>  
> @@ -135,6 +136,7 @@ struct bmc150_magn_data {
>  	 */
>  	struct mutex mutex;
>  	struct regmap *regmap;
> +	struct regulator_bulk_data regulators[2];
>  	struct iio_mount_matrix orientation;
>  	/* 4 x 32 bits for x, y z, 4 bytes align, 64 bits timestamp */
>  	s32 buffer[6];
> @@ -692,12 +694,24 @@ static int bmc150_magn_init(struct bmc150_magn_data *data)
>  	int ret, chip_id;
>  	struct bmc150_magn_preset preset;
>  
> +	ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				    data->regulators);
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +	/*
> +	 * 3ms power-on time according to datasheet, let's better
> +	 * be safe than sorry and set this delay to 5ms.
> +	 */
> +	msleep(5);
> +
>  	ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND,
>  					 false);
>  	if (ret < 0) {
>  		dev_err(data->dev,
>  			"Failed to bring up device from suspend mode\n");
> -		return ret;
> +		goto err_regulator_disable;
>  	}
>  
>  	ret = regmap_read(data->regmap, BMC150_MAGN_REG_CHIP_ID, &chip_id);
> @@ -752,6 +766,8 @@ static int bmc150_magn_init(struct bmc150_magn_data *data)
>  
>  err_poweroff:
>  	bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
> +err_regulator_disable:
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>  	return ret;
>  }
>  
> @@ -867,6 +883,13 @@ int bmc150_magn_probe(struct device *dev, struct regmap *regmap,
>  	data->irq = irq;
>  	data->dev = dev;
>  
> +	data->regulators[0].supply = "vdd";
> +	data->regulators[1].supply = "vddio";
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
>  	ret = iio_read_mount_matrix(dev, "mount-matrix",
>  				&data->orientation);
>  	if (ret)
> @@ -984,6 +1007,7 @@ int bmc150_magn_remove(struct device *dev)
>  	bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
>  	mutex_unlock(&data->mutex);
>  
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>  	return 0;
>  }
>  EXPORT_SYMBOL(bmc150_magn_remove);
> -- 
> 2.29.2
> 

Re: [PATCH 2/4] iio: magnetometer: bmc150: Add rudimentary regulator support

[Jonathan Cameron]

On Wed, 6 Jan 2021 11:33:31 +0100
Stephan Gerhold <stephan@gerhold.net> wrote:

> Hi Jonathan,
> 
> On Wed, Dec 02, 2020 at 10:33:20AM +0100, Stephan Gerhold wrote:
> > BMC150 needs VDD and VDDIO regulators that might need to be explicitly
> > enabled. Add some rudimentary support to obtain and enable these
> > regulators during probe() and disable them during remove()
> > or on the error path.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>  
> 
> I think we forgot about the bmc150_magn part of this patch series. :)
> 
> This is likely my mistake - I only sent a v2 for the bmg160 part of this
> patch series, but not for the bmc150_magn part (since it's not affected
> by the problem you mentioned for the bmg160 patch). Sorry for the confusion!
> 
> Should I re-send the bmc150_magn part (with or without some changes)?
Please resend. Don't think any changes are needed.

I tend to use the b4 tool to apply patches. I can pick individual
patches out of a series, but it's much easier to just pick up a full
series once it's ready.

Thanks,

Jonathan

> 
> Thanks!
> Stephan
> 
> > ---
> > This is mostly copy-paste of
> > 079c1c3f2082 ("iio: accel: bmc150-accel: Add rudimentary regulator support")
> > from Linus Walleij but for the BMC150 magnetometer driver.
> > ---
> >  drivers/iio/magnetometer/bmc150_magn.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> > index fa09fcab620a..b2f3129e1b4f 100644
> > --- a/drivers/iio/magnetometer/bmc150_magn.c
> > +++ b/drivers/iio/magnetometer/bmc150_magn.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> >  #include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> >  
> >  #include "bmc150_magn.h"
> >  
> > @@ -135,6 +136,7 @@ struct bmc150_magn_data {
> >  	 */
> >  	struct mutex mutex;
> >  	struct regmap *regmap;
> > +	struct regulator_bulk_data regulators[2];
> >  	struct iio_mount_matrix orientation;
> >  	/* 4 x 32 bits for x, y z, 4 bytes align, 64 bits timestamp */
> >  	s32 buffer[6];
> > @@ -692,12 +694,24 @@ static int bmc150_magn_init(struct bmc150_magn_data *data)
> >  	int ret, chip_id;
> >  	struct bmc150_magn_preset preset;
> >  
> > +	ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> > +				    data->regulators);
> > +	if (ret < 0) {
> > +		dev_err(data->dev, "Failed to enable regulators: %d\n", ret);
> > +		return ret;
> > +	}
> > +	/*
> > +	 * 3ms power-on time according to datasheet, let's better
> > +	 * be safe than sorry and set this delay to 5ms.
> > +	 */
> > +	msleep(5);
> > +
> >  	ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND,
> >  					 false);
> >  	if (ret < 0) {
> >  		dev_err(data->dev,
> >  			"Failed to bring up device from suspend mode\n");
> > -		return ret;
> > +		goto err_regulator_disable;
> >  	}
> >  
> >  	ret = regmap_read(data->regmap, BMC150_MAGN_REG_CHIP_ID, &chip_id);
> > @@ -752,6 +766,8 @@ static int bmc150_magn_init(struct bmc150_magn_data *data)
> >  
> >  err_poweroff:
> >  	bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
> > +err_regulator_disable:
> > +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> >  	return ret;
> >  }
> >  
> > @@ -867,6 +883,13 @@ int bmc150_magn_probe(struct device *dev, struct regmap *regmap,
> >  	data->irq = irq;
> >  	data->dev = dev;
> >  
> > +	data->regulators[0].supply = "vdd";
> > +	data->regulators[1].supply = "vddio";
> > +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
> > +				      data->regulators);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> > +
> >  	ret = iio_read_mount_matrix(dev, "mount-matrix",
> >  				&data->orientation);
> >  	if (ret)
> > @@ -984,6 +1007,7 @@ int bmc150_magn_remove(struct device *dev)
> >  	bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
> >  	mutex_unlock(&data->mutex);
> >  
> > +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(bmc150_magn_remove);
> > -- 
> > 2.29.2
> >   

[PATCH 3/4] dt-bindings: iio: gyroscope: bmg160: Document regulator supplies

[Stephan Gerhold]

BMG160 needs VDD and VDDIO regulators that might need to be explicitly
enabled. Document support for vdd/vddio-supply to implement this.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../devicetree/bindings/iio/gyroscope/bosch,bmg160.yaml        | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/gyroscope/bosch,bmg160.yaml b/Documentation/devicetree/bindings/iio/gyroscope/bosch,bmg160.yaml
index 0466483be6bb..b6bbc312a7cf 100644
--- a/Documentation/devicetree/bindings/iio/gyroscope/bosch,bmg160.yaml
+++ b/Documentation/devicetree/bindings/iio/gyroscope/bosch,bmg160.yaml
@@ -19,6 +19,9 @@ properties:
   reg:
     maxItems: 1
 
+  vdd-supply: true
+  vddio-supply: true
+
   interrupts:
     minItems: 1
     description:
-- 
2.29.2

Re: [PATCH 3/4] dt-bindings: iio: gyroscope: bmg160: Document regulator supplies

[Linus Walleij]

On Wed, Dec 2, 2020 at 10:33 AM Stephan Gerhold <stephan@gerhold.net> wrote:

> BMG160 needs VDD and VDDIO regulators that might need to be explicitly
> enabled. Document support for vdd/vddio-supply to implement this.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/4] dt-bindings: iio: gyroscope: bmg160: Document regulator supplies

[Rob Herring]

On Wed, 02 Dec 2020 10:33:21 +0100, Stephan Gerhold wrote:
> BMG160 needs VDD and VDDIO regulators that might need to be explicitly
> enabled. Document support for vdd/vddio-supply to implement this.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  .../devicetree/bindings/iio/gyroscope/bosch,bmg160.yaml        | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 4/4] iio: gyro: bmg160: Add rudimentary regulator support

[Stephan Gerhold]

BMG160 needs VDD and VDDIO regulators that might need to be explicitly
enabled. Add some rudimentary support to obtain and enable these
regulators during probe() and disable them during remove()
or on the error path.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/iio/gyro/bmg160_core.c | 38 +++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index 2d5015801a75..4baa4169c5a2 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -19,6 +19,7 @@
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include "bmg160.h"
 
 #define BMG160_IRQ_NAME		"bmg160_event"
@@ -92,6 +93,7 @@
 
 struct bmg160_data {
 	struct regmap *regmap;
+	struct regulator_bulk_data regulators[2];
 	struct iio_trigger *dready_trig;
 	struct iio_trigger *motion_trig;
 	struct iio_mount_matrix orientation;
@@ -1077,14 +1079,28 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 	data->irq = irq;
 	data->regmap = regmap;
 
+	data->regulators[0].supply = "vdd";
+	data->regulators[1].supply = "vddio";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
+				      data->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
 	ret = iio_read_mount_matrix(dev, "mount-matrix",
 				&data->orientation);
 	if (ret)
 		return ret;
 
+	ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
+				    data->regulators);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
 	ret = bmg160_chip_init(data);
 	if (ret < 0)
-		return ret;
+		goto err_regulator_disable;
 
 	mutex_init(&data->mutex);
 
@@ -1107,28 +1123,32 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 						BMG160_IRQ_NAME,
 						indio_dev);
 		if (ret)
-			return ret;
+			goto err_regulator_disable;
 
 		data->dready_trig = devm_iio_trigger_alloc(dev,
 							   "%s-dev%d",
 							   indio_dev->name,
 							   indio_dev->id);
-		if (!data->dready_trig)
-			return -ENOMEM;
+		if (!data->dready_trig) {
+			ret = -ENOMEM;
+			goto err_regulator_disable;
+		}
 
 		data->motion_trig = devm_iio_trigger_alloc(dev,
 							  "%s-any-motion-dev%d",
 							  indio_dev->name,
 							  indio_dev->id);
-		if (!data->motion_trig)
-			return -ENOMEM;
+		if (!data->motion_trig) {
+			ret = -ENOMEM;
+			goto err_regulator_disable;
+		}
 
 		data->dready_trig->dev.parent = dev;
 		data->dready_trig->ops = &bmg160_trigger_ops;
 		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
 		ret = iio_trigger_register(data->dready_trig);
 		if (ret)
-			return ret;
+			goto err_regulator_disable;
 
 		data->motion_trig->dev.parent = dev;
 		data->motion_trig->ops = &bmg160_trigger_ops;
@@ -1174,6 +1194,8 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
 		iio_trigger_unregister(data->dready_trig);
 	if (data->motion_trig)
 		iio_trigger_unregister(data->motion_trig);
+err_regulator_disable:
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
 
 	return ret;
 }
@@ -1200,6 +1222,8 @@ void bmg160_core_remove(struct device *dev)
 	mutex_lock(&data->mutex);
 	bmg160_set_mode(data, BMG160_MODE_DEEP_SUSPEND);
 	mutex_unlock(&data->mutex);
+
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
 }
 EXPORT_SYMBOL_GPL(bmg160_core_remove);
 
-- 
2.29.2

Re: [PATCH 4/4] iio: gyro: bmg160: Add rudimentary regulator support

[Linus Walleij]

On Wed, Dec 2, 2020 at 10:33 AM Stephan Gerhold <stephan@gerhold.net> wrote:

> BMG160 needs VDD and VDDIO regulators that might need to be explicitly
> enabled. Add some rudimentary support to obtain and enable these
> regulators during probe() and disable them during remove()
> or on the error path.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Linus Walleij <linus.walleij@linar.org>

Yours,
Linus Walleij

Re: [PATCH 4/4] iio: gyro: bmg160: Add rudimentary regulator support

[Jonathan Cameron]

On Wed,  2 Dec 2020 10:33:22 +0100
Stephan Gerhold <stephan@gerhold.net> wrote:

> BMG160 needs VDD and VDDIO regulators that might need to be explicitly
> enabled. Add some rudimentary support to obtain and enable these
> regulators during probe() and disable them during remove()
> or on the error path.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Hi Stephan,

This one is a bit tricky due to the extensive use of devm_ managed
cleanup.  Normally I'd be very fussy about ensuring remove order
is precise reverse of probe, but in this driver it isn't quite
already, due to that chip_init being before the interrupt allocation.

Having said that I'd rather not make it worse.  Would you mind
using automated clean up of the regulator_enable as well via
devm_add_action_or_reset() call?

As a side note, should we not have more cleanup of chip_init()
in error paths, specifically putting the device into it's suspended
mode?  Obviously nothing to do with your patch...

Thanks,

Jonathan

> ---
>  drivers/iio/gyro/bmg160_core.c | 38 +++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index 2d5015801a75..4baa4169c5a2 100644
> --- a/drivers/iio/gyro/bmg160_core.c
> +++ b/drivers/iio/gyro/bmg160_core.c
> @@ -19,6 +19,7 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include "bmg160.h"
>  
>  #define BMG160_IRQ_NAME		"bmg160_event"
> @@ -92,6 +93,7 @@
>  
>  struct bmg160_data {
>  	struct regmap *regmap;
> +	struct regulator_bulk_data regulators[2];
>  	struct iio_trigger *dready_trig;
>  	struct iio_trigger *motion_trig;
>  	struct iio_mount_matrix orientation;
> @@ -1077,14 +1079,28 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  	data->irq = irq;
>  	data->regmap = regmap;
>  
> +	data->regulators[0].supply = "vdd";
> +	data->regulators[1].supply = "vddio";
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
> +				      data->regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
> +
>  	ret = iio_read_mount_matrix(dev, "mount-matrix",
>  				&data->orientation);
>  	if (ret)
>  		return ret;

Why not put regulator get and enable together?  

>  
> +	ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				    data->regulators);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +

If you were to use devm_add_action_or_reset() and a trivial wrapper
the disable would be automated, simplifying the error handling etc.

>  	ret = bmg160_chip_init(data);
>  	if (ret < 0)
> -		return ret;
> +		goto err_regulator_disable;
>  
>  	mutex_init(&data->mutex);
>  
> @@ -1107,28 +1123,32 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  						BMG160_IRQ_NAME,
>  						indio_dev);
>  		if (ret)
> -			return ret;
> +			goto err_regulator_disable;
>  
>  		data->dready_trig = devm_iio_trigger_alloc(dev,
>  							   "%s-dev%d",
>  							   indio_dev->name,
>  							   indio_dev->id);
> -		if (!data->dready_trig)
> -			return -ENOMEM;
> +		if (!data->dready_trig) {
> +			ret = -ENOMEM;
> +			goto err_regulator_disable;
> +		}
>  
>  		data->motion_trig = devm_iio_trigger_alloc(dev,
>  							  "%s-any-motion-dev%d",
>  							  indio_dev->name,
>  							  indio_dev->id);
> -		if (!data->motion_trig)
> -			return -ENOMEM;
> +		if (!data->motion_trig) {
> +			ret = -ENOMEM;
> +			goto err_regulator_disable;
> +		}
>  
>  		data->dready_trig->dev.parent = dev;
>  		data->dready_trig->ops = &bmg160_trigger_ops;
>  		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
>  		ret = iio_trigger_register(data->dready_trig);
>  		if (ret)
> -			return ret;
> +			goto err_regulator_disable;
>  
>  		data->motion_trig->dev.parent = dev;
>  		data->motion_trig->ops = &bmg160_trigger_ops;
> @@ -1174,6 +1194,8 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
>  		iio_trigger_unregister(data->dready_trig);
>  	if (data->motion_trig)
>  		iio_trigger_unregister(data->motion_trig);
> +err_regulator_disable:
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>  
>  	return ret;
>  }
> @@ -1200,6 +1222,8 @@ void bmg160_core_remove(struct device *dev)
>  	mutex_lock(&data->mutex);
>  	bmg160_set_mode(data, BMG160_MODE_DEEP_SUSPEND);
>  	mutex_unlock(&data->mutex);
> +
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>  }
>  EXPORT_SYMBOL_GPL(bmg160_core_remove);
>  

Re: [PATCH 4/4] iio: gyro: bmg160: Add rudimentary regulator support

[Stephan Gerhold]

Hi Jonathan,

On Sat, Dec 05, 2020 at 03:38:48PM +0000, Jonathan Cameron wrote:
> On Wed,  2 Dec 2020 10:33:22 +0100
> Stephan Gerhold <stephan@gerhold.net> wrote:
> 
> > BMG160 needs VDD and VDDIO regulators that might need to be explicitly
> > enabled. Add some rudimentary support to obtain and enable these
> > regulators during probe() and disable them during remove()
> > or on the error path.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> 
> This one is a bit tricky due to the extensive use of devm_ managed
> cleanup.  Normally I'd be very fussy about ensuring remove order
> is precise reverse of probe, but in this driver it isn't quite
> already, due to that chip_init being before the interrupt allocation.
> 
> Having said that I'd rather not make it worse.  Would you mind
> using automated clean up of the regulator_enable as well via
> devm_add_action_or_reset() call?
> 

Good point, devm_add_action_or_reset() definitely looks better for this
driver. I will send a v2 with just the bmg160 part shortly.

> As a side note, should we not have more cleanup of chip_init()
> in error paths, specifically putting the device into it's suspended
> mode?  Obviously nothing to do with your patch...
> 

I'm not sure. I guess when bmg160_chip_init() fails there is some kind
of communication problem or a problem with the chip. Chances are that
putting it back into suspend mode would also fail.

But I don't really know enough about the hardware to say more. :)

I have also fixed your other comments below in v2.

Thanks!
Stephan

> 
> > ---
> >  drivers/iio/gyro/bmg160_core.c | 38 +++++++++++++++++++++++++++-------
> >  1 file changed, 31 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> > index 2d5015801a75..4baa4169c5a2 100644
> > --- a/drivers/iio/gyro/bmg160_core.c
> > +++ b/drivers/iio/gyro/bmg160_core.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> >  #include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> >  #include "bmg160.h"
> >  
> >  #define BMG160_IRQ_NAME		"bmg160_event"
> > @@ -92,6 +93,7 @@
> >  
> >  struct bmg160_data {
> >  	struct regmap *regmap;
> > +	struct regulator_bulk_data regulators[2];
> >  	struct iio_trigger *dready_trig;
> >  	struct iio_trigger *motion_trig;
> >  	struct iio_mount_matrix orientation;
> > @@ -1077,14 +1079,28 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> >  	data->irq = irq;
> >  	data->regmap = regmap;
> >  
> > +	data->regulators[0].supply = "vdd";
> > +	data->regulators[1].supply = "vddio";
> > +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators),
> > +				      data->regulators);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
> > +
> >  	ret = iio_read_mount_matrix(dev, "mount-matrix",
> >  				&data->orientation);
> >  	if (ret)
> >  		return ret;
> 
> Why not put regulator get and enable together?  
> 
> >  
> > +	ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> > +				    data->regulators);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to enable regulators: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> 
> If you were to use devm_add_action_or_reset() and a trivial wrapper
> the disable would be automated, simplifying the error handling etc.
> 
> >  	ret = bmg160_chip_init(data);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto err_regulator_disable;
> >  
> >  	mutex_init(&data->mutex);
> >  
> > @@ -1107,28 +1123,32 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> >  						BMG160_IRQ_NAME,
> >  						indio_dev);
> >  		if (ret)
> > -			return ret;
> > +			goto err_regulator_disable;
> >  
> >  		data->dready_trig = devm_iio_trigger_alloc(dev,
> >  							   "%s-dev%d",
> >  							   indio_dev->name,
> >  							   indio_dev->id);
> > -		if (!data->dready_trig)
> > -			return -ENOMEM;
> > +		if (!data->dready_trig) {
> > +			ret = -ENOMEM;
> > +			goto err_regulator_disable;
> > +		}
> >  
> >  		data->motion_trig = devm_iio_trigger_alloc(dev,
> >  							  "%s-any-motion-dev%d",
> >  							  indio_dev->name,
> >  							  indio_dev->id);
> > -		if (!data->motion_trig)
> > -			return -ENOMEM;
> > +		if (!data->motion_trig) {
> > +			ret = -ENOMEM;
> > +			goto err_regulator_disable;
> > +		}
> >  
> >  		data->dready_trig->dev.parent = dev;
> >  		data->dready_trig->ops = &bmg160_trigger_ops;
> >  		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
> >  		ret = iio_trigger_register(data->dready_trig);
> >  		if (ret)
> > -			return ret;
> > +			goto err_regulator_disable;
> >  
> >  		data->motion_trig->dev.parent = dev;
> >  		data->motion_trig->ops = &bmg160_trigger_ops;
> > @@ -1174,6 +1194,8 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq,
> >  		iio_trigger_unregister(data->dready_trig);
> >  	if (data->motion_trig)
> >  		iio_trigger_unregister(data->motion_trig);
> > +err_regulator_disable:
> > +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> >  
> >  	return ret;
> >  }
> > @@ -1200,6 +1222,8 @@ void bmg160_core_remove(struct device *dev)
> >  	mutex_lock(&data->mutex);
> >  	bmg160_set_mode(data, BMG160_MODE_DEEP_SUSPEND);
> >  	mutex_unlock(&data->mutex);
> > +
> > +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> >  }
> >  EXPORT_SYMBOL_GPL(bmg160_core_remove);
> >  
> 

Re: [PATCH 1/4] dt-bindings: iio: magnetometer: bmc150: Document regulator supplies

[Linus Walleij]

On Wed, Dec 2, 2020 at 10:33 AM Stephan Gerhold <stephan@gerhold.net> wrote:

> BMC150 needs VDD and VDDIO regulators that might need to be explicitly
> enabled. Document support for vdd/vddio-supply to implement this.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 1/4] dt-bindings: iio: magnetometer: bmc150: Document regulator supplies

[Rob Herring]

On Wed, 02 Dec 2020 10:33:19 +0100, Stephan Gerhold wrote:
> BMC150 needs VDD and VDDIO regulators that might need to be explicitly
> enabled. Document support for vdd/vddio-supply to implement this.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  .../bindings/iio/magnetometer/bosch,bmc150_magn.yaml           | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>