[1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply
diff mbox series

Message ID 20191106183536.123070-1-stephan@gerhold.net
State Superseded
Headers show
Series
  • [1/2] dt-bindings: iio: imo: mpu6050: add vdd-supply
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Stephan Gerhold Nov. 6, 2019, 6:35 p.m. UTC
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(+)

Comments

Stephan Gerhold Nov. 6, 2019, 9:36 p.m. UTC | #1
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
Jean-Baptiste Maneyrol Nov. 7, 2019, 1:29 p.m. UTC | #2
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
Linus Walleij Nov. 7, 2019, 3:06 p.m. UTC | #3
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
Stephan Gerhold Nov. 7, 2019, 6:12 p.m. UTC | #4
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

Patch
diff mbox series

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