linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply
@ 2019-11-06 18:35 Stephan Gerhold
  2019-11-06 18:35 ` [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator Stephan Gerhold
  2019-11-07 15:06 ` [PATCH 1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply Linus Walleij
  0 siblings, 2 replies; 7+ messages in thread
From: Stephan Gerhold @ 2019-11-06 18:35 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, Linus Walleij, Brian Masney,
	Jonathan Marek, Jean-Baptiste Maneyrol, linux-iio, devicetree,
	linux-kernel, Stephan Gerhold

inv_mpu6050 now supports an additional vdd-supply; document it.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
index 268bf7568e19..c5ee8a20af9f 100644
--- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
+++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
@@ -21,6 +21,7 @@ Required properties:
   bindings.
 
 Optional properties:
+ - vdd-supply: regulator phandle for VDD supply
  - vddio-supply: regulator phandle for VDDIO supply
  - mount-matrix: an optional 3x3 mounting rotation matrix
  - i2c-gate node.  These devices also support an auxiliary i2c bus.  This is
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator
  2019-11-06 18:35 [PATCH 1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply Stephan Gerhold
@ 2019-11-06 18:35 ` Stephan Gerhold
  2019-11-06 19:55   ` Jean-Baptiste Maneyrol
  2019-11-07 15:06 ` [PATCH 1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply Linus Walleij
  1 sibling, 1 reply; 7+ messages in thread
From: Stephan Gerhold @ 2019-11-06 18:35 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, Linus Walleij, Brian Masney,
	Jonathan Marek, Jean-Baptiste Maneyrol, linux-iio, devicetree,
	linux-kernel, Stephan Gerhold

MPU6050 has two power supply pins: VDD and VLOGIC, but the mpu6050
only supports enabling one of them at the moment. In some cases,
they may need to be enabled separately.

Add an additional "vdd-supply", but keep the code simple by making
the driver use the regulator bulk API. This actually allows further
simplifying the code since regulator_bulk_* already logs the errors
for us.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 30 +++++++---------------
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  5 ++--
 2 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 354030e9bed5..8e50dbcd730b 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -1144,11 +1144,8 @@ static int inv_mpu_core_enable_regulator(struct inv_mpu6050_state *st)
 {
 	int result;
 
-	result = regulator_enable(st->vddio_supply);
-	if (result) {
-		dev_err(regmap_get_device(st->map),
-			"Failed to enable regulator: %d\n", result);
-	} else {
+	result = regulator_bulk_enable(ARRAY_SIZE(st->supplies), st->supplies);
+	if (result == 0) {
 		/* Give the device a little bit of time to start up. */
 		usleep_range(35000, 70000);
 	}
@@ -1158,14 +1155,7 @@ static int inv_mpu_core_enable_regulator(struct inv_mpu6050_state *st)
 
 static int inv_mpu_core_disable_regulator(struct inv_mpu6050_state *st)
 {
-	int result;
-
-	result = regulator_disable(st->vddio_supply);
-	if (result)
-		dev_err(regmap_get_device(st->map),
-			"Failed to disable regulator: %d\n", result);
-
-	return result;
+	return regulator_bulk_disable(ARRAY_SIZE(st->supplies), st->supplies);
 }
 
 static void inv_mpu_core_disable_regulator_action(void *_data)
@@ -1239,14 +1229,12 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
 		return -EINVAL;
 	}
 
-	st->vddio_supply = devm_regulator_get(dev, "vddio");
-	if (IS_ERR(st->vddio_supply)) {
-		if (PTR_ERR(st->vddio_supply) != -EPROBE_DEFER)
-			dev_err(dev, "Failed to get vddio regulator %d\n",
-				(int)PTR_ERR(st->vddio_supply));
-
-		return PTR_ERR(st->vddio_supply);
-	}
+	st->supplies[0].supply = "vdd";
+	st->supplies[1].supply = "vddio";
+	result = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->supplies),
+					 st->supplies);
+	if (result)
+		return result;
 
 	result = inv_mpu_core_enable_regulator(st);
 	if (result)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 52fcf45050a5..96cbd7f2b4b3 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -12,6 +12,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/kfifo_buf.h>
 #include <linux/iio/trigger.h>
@@ -130,7 +131,7 @@ struct inv_mpu6050_hw {
  *  @chip_period:	chip internal period estimation (~1kHz).
  *  @it_timestamp:	timestamp from previous interrupt.
  *  @data_timestamp:	timestamp for next data sample.
- *  @vddio_supply	voltage regulator for the chip.
+ *  @supplies:		voltage regulators for the chip.
  *  @magn_disabled:     magnetometer disabled for backward compatibility reason.
  *  @magn_raw_to_gauss:	coefficient to convert mag raw value to Gauss.
  *  @magn_orient:       magnetometer sensor chip orientation if available.
@@ -154,7 +155,7 @@ struct inv_mpu6050_state {
 	s64 chip_period;
 	s64 it_timestamp;
 	s64 data_timestamp;
-	struct regulator *vddio_supply;
+	struct regulator_bulk_data supplies[2];
 	bool magn_disabled;
 	s32 magn_raw_to_gauss[3];
 	struct iio_mount_matrix magn_orient;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator
  2019-11-06 18:35 ` [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator Stephan Gerhold
@ 2019-11-06 19:55   ` Jean-Baptiste Maneyrol
  2019-11-06 21:36     ` Stephan Gerhold
  0 siblings, 1 reply; 7+ messages in thread
From: Jean-Baptiste Maneyrol @ 2019-11-06 19:55 UTC (permalink / raw)
  To: Stephan Gerhold, Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, Linus Walleij, Brian Masney,
	Jonathan Marek, linux-iio, devicetree, linux-kernel

Hello Stephan,

nice patch but I have an important concern.

We are calling the core_enable/disable_regulator functions when going into suspend and resume. With your changes, we are going to power down the chip when going into suspend and then power it up again. This way we will loose all already set configuration, like FSR, sampling rate, init values, ... The chip will not be able to work correctly anymore after a suspend-resume cycle.

You need to change the resume/suspend handlers to only disable/enable the vddio regulator, not the vdd one.

Thanks.

Best regards,
JB

From: Stephan Gerhold <stephan@gerhold.net>

Sent: Wednesday, November 6, 2019 19:35

To: Jonathan Cameron <jic23@kernel.org>

Cc: Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>; Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Linus Walleij <linus.walleij@linaro.org>; Brian Masney <masneyb@onstation.org>;
 Jonathan Marek <jonathan@marek.ca>; Jean-Baptiste Maneyrol <JManeyrol@invensense.com>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>;
 Stephan Gerhold <stephan@gerhold.net>

Subject: [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



MPU6050 has two power supply pins: VDD and VLOGIC, but the mpu6050

only supports enabling one of them at the moment. In some cases,

they may need to be enabled separately.



Add an additional "vdd-supply", but keep the code simple by making

the driver use the regulator bulk API. This actually allows further

simplifying the code since regulator_bulk_* already logs the errors

for us.



Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

---

 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 30 +++++++---------------

 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  5 ++--

 2 files changed, 12 insertions(+), 23 deletions(-)



diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

index 354030e9bed5..8e50dbcd730b 100644

--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c

@@ -1144,11 +1144,8 @@ static int inv_mpu_core_enable_regulator(struct inv_mpu6050_state *st)

 {

         int result;

 

-       result = regulator_enable(st->vddio_supply);

-       if (result) {

-               dev_err(regmap_get_device(st->map),

-                       "Failed to enable regulator: %d\n", result);

-       } else {

+       result = regulator_bulk_enable(ARRAY_SIZE(st->supplies), st->supplies);

+       if (result == 0) {

                 /* Give the device a little bit of time to start up. */

                 usleep_range(35000, 70000);

         }

@@ -1158,14 +1155,7 @@ static int inv_mpu_core_enable_regulator(struct inv_mpu6050_state *st)

 

 static int inv_mpu_core_disable_regulator(struct inv_mpu6050_state *st)

 {

-       int result;

-

-       result = regulator_disable(st->vddio_supply);

-       if (result)

-               dev_err(regmap_get_device(st->map),

-                       "Failed to disable regulator: %d\n", result);

-

-       return result;

+       return regulator_bulk_disable(ARRAY_SIZE(st->supplies), st->supplies);

 }

 

 static void inv_mpu_core_disable_regulator_action(void *_data)

@@ -1239,14 +1229,12 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,

                 return -EINVAL;

         }

 

-       st->vddio_supply = devm_regulator_get(dev, "vddio");

-       if (IS_ERR(st->vddio_supply)) {

-               if (PTR_ERR(st->vddio_supply) != -EPROBE_DEFER)

-                       dev_err(dev, "Failed to get vddio regulator %d\n",

-                               (int)PTR_ERR(st->vddio_supply));

-

-               return PTR_ERR(st->vddio_supply);

-       }

+       st->supplies[0].supply = "vdd";

+       st->supplies[1].supply = "vddio";

+       result = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->supplies),

+                                        st->supplies);

+       if (result)

+               return result;

 

         result = inv_mpu_core_enable_regulator(st);

         if (result)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h

index 52fcf45050a5..96cbd7f2b4b3 100644

--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h

+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h

@@ -12,6 +12,7 @@

 #include <linux/iio/iio.h>

 #include <linux/iio/buffer.h>

 #include <linux/regmap.h>

+#include <linux/regulator/consumer.h>

 #include <linux/iio/sysfs.h>

 #include <linux/iio/kfifo_buf.h>

 #include <linux/iio/trigger.h>

@@ -130,7 +131,7 @@ struct inv_mpu6050_hw {

  *  @chip_period:      chip internal period estimation (~1kHz).

  *  @it_timestamp:     timestamp from previous interrupt.

  *  @data_timestamp:   timestamp for next data sample.

- *  @vddio_supply      voltage regulator for the chip.

+ *  @supplies:         voltage regulators for the chip.

  *  @magn_disabled:     magnetometer disabled for backward compatibility reason.

  *  @magn_raw_to_gauss:        coefficient to convert mag raw value to Gauss.

  *  @magn_orient:       magnetometer sensor chip orientation if available.

@@ -154,7 +155,7 @@ struct inv_mpu6050_state {

         s64 chip_period;

         s64 it_timestamp;

         s64 data_timestamp;

-       struct regulator *vddio_supply;

+       struct regulator_bulk_data supplies[2];

         bool magn_disabled;

         s32 magn_raw_to_gauss[3];

         struct iio_mount_matrix magn_orient;

-- 

2.23.0




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator
  2019-11-06 19:55   ` Jean-Baptiste Maneyrol
@ 2019-11-06 21:36     ` Stephan Gerhold
  2019-11-07 13:29       ` Jean-Baptiste Maneyrol
  0 siblings, 1 reply; 7+ messages in thread
From: Stephan Gerhold @ 2019-11-06 21:36 UTC (permalink / raw)
  To: Jean-Baptiste Maneyrol
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland, Linus Walleij,
	Brian Masney, Jonathan Marek, linux-iio, devicetree,
	linux-kernel

Hi JB,

On Wed, Nov 06, 2019 at 07:55:20PM +0000, Jean-Baptiste Maneyrol wrote:
> Hello Stephan,
> 
> nice patch but I have an important concern.
> 
> We are calling the core_enable/disable_regulator functions when going into suspend and resume.
> With your changes, we are going to power down the chip when going into suspend and then power it up again.
> This way we will loose all already set configuration, like FSR, sampling rate, init values, ...
> The chip will not be able to work correctly anymore after a suspend-resume cycle.
> 
> You need to change the resume/suspend handlers to only disable/enable the vddio regulator, not the vdd one.

That is a good point, thanks!
I guess we are not able to use the regulator bulk API in this case...

I will send a v2 soon.

Stephan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator
  2019-11-06 21:36     ` Stephan Gerhold
@ 2019-11-07 13:29       ` Jean-Baptiste Maneyrol
  2019-11-07 18:12         ` Stephan Gerhold
  0 siblings, 1 reply; 7+ messages in thread
From: Jean-Baptiste Maneyrol @ 2019-11-07 13:29 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland, Linus Walleij,
	Brian Masney, Jonathan Marek, linux-iio, devicetree,
	linux-kernel

 Hi Stephan,

I think the regulator_bulk usage is good, and the core_enable/disable_regulator functions implemented the way you did is perfect for the init/shutdown phase.

We just need to change the suspend/resume implementation to use something different.

Thanks,
JB


From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> on behalf of Stephan Gerhold <stephan@gerhold.net>

Sent: Wednesday, November 6, 2019 22:36

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: Jonathan Cameron <jic23@kernel.org>; Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>; Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Linus Walleij <linus.walleij@linaro.org>;
 Brian Masney <masneyb@onstation.org>; Jonathan Marek <jonathan@marek.ca>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>

Subject: Re: [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



Hi JB,



On Wed, Nov 06, 2019 at 07:55:20PM +0000, Jean-Baptiste Maneyrol wrote:

> Hello Stephan,

> 

> nice patch but I have an important concern.

> 

> We are calling the core_enable/disable_regulator functions when going into suspend and resume.

> With your changes, we are going to power down the chip when going into suspend and then power it up again.

> This way we will loose all already set configuration, like FSR, sampling rate, init values, ...

> The chip will not be able to work correctly anymore after a suspend-resume cycle.

> 

> You need to change the resume/suspend handlers to only disable/enable the vddio regulator, not the vdd one.



That is a good point, thanks!

I guess we are not able to use the regulator bulk API in this case...



I will send a v2 soon.



Stephan


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply
  2019-11-06 18:35 [PATCH 1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply Stephan Gerhold
  2019-11-06 18:35 ` [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator Stephan Gerhold
@ 2019-11-07 15:06 ` Linus Walleij
  1 sibling, 0 replies; 7+ messages in thread
From: Linus Walleij @ 2019-11-07 15:06 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland, Brian Masney,
	Jonathan Marek, Jean-Baptiste Maneyrol, linux-iio,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel

On Wed, Nov 6, 2019 at 7:37 PM Stephan Gerhold <stephan@gerhold.net> wrote:

> inv_mpu6050 now supports an additional vdd-supply; document it.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

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

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator
  2019-11-07 13:29       ` Jean-Baptiste Maneyrol
@ 2019-11-07 18:12         ` Stephan Gerhold
  0 siblings, 0 replies; 7+ messages in thread
From: Stephan Gerhold @ 2019-11-07 18:12 UTC (permalink / raw)
  To: Jean-Baptiste Maneyrol
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland, Linus Walleij,
	Brian Masney, Jonathan Marek, linux-iio, devicetree,
	linux-kernel

On Thu, Nov 07, 2019 at 01:29:33PM +0000, Jean-Baptiste Maneyrol wrote:
> Hi Stephan,
> 
> I think the regulator_bulk usage is good, and the core_enable/disable_regulator functions implemented the way you did is perfect for the init/shutdown phase.
> 
> We just need to change the suspend/resume implementation to use something different.

As far as I can tell, the regulator bulk API is designed to be used when
you want to enable/disable multiple regulators at the same time.
It does not really allow controlling one of its regulators separately
(in a clean way).

E.g. if you would use regulator_bulk_disable() but already disabled one
of the regulators at some point earlier, you would run into a warning
because the regulator is disabled twice.

My updated patch is still clean enough (in my opinion), so I would say
it is better to avoid the regulator bulk API for this situation.
I will send v2 shortly.

Thanks,
Stephan

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-11-07 18:12 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06 18:35 [PATCH 1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply Stephan Gerhold
2019-11-06 18:35 ` [PATCH 2/2] iio: imu: mpu6050: Add support for vdd-supply regulator Stephan Gerhold
2019-11-06 19:55   ` Jean-Baptiste Maneyrol
2019-11-06 21:36     ` Stephan Gerhold
2019-11-07 13:29       ` Jean-Baptiste Maneyrol
2019-11-07 18:12         ` Stephan Gerhold
2019-11-07 15:06 ` [PATCH 1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).