Linux-IIO Archive on lore.kernel.org
 help / Atom feed
* [PATCH V5 1/2] dt-bindings: iio: accel: mma8452: add power supplies property
@ 2018-12-17  5:22 Anson Huang
  2018-12-17  5:22 ` [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support Anson Huang
  2018-12-17 21:42 ` [PATCH V5 1/2] dt-bindings: iio: accel: mma8452: add power supplies property Rob Herring
  0 siblings, 2 replies; 5+ messages in thread
From: Anson Huang @ 2018-12-17  5:22 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, martink,
	gregkh, gustavo, Leonard Crestez, rtresidd, linux-iio,
	devicetree, linux-kernel, festevam, preid
  Cc: dl-linux-imx

The accelerometer's power supplies could be controllable on some
platforms, add property "vdd/vddio" power supply to let device tree
to pass phandles to the regulators to driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 Documentation/devicetree/bindings/iio/accel/mma8452.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/mma8452.txt b/Documentation/devicetree/bindings/iio/accel/mma8452.txt
index 2100e9a..e132394 100644
--- a/Documentation/devicetree/bindings/iio/accel/mma8452.txt
+++ b/Documentation/devicetree/bindings/iio/accel/mma8452.txt
@@ -20,6 +20,10 @@ Optional properties:
   - interrupt-names: should contain "INT1" and/or "INT2", the accelerometer's
 		     interrupt line in use.
 
+  - vdd-supply: phandle to the regulator that provides vdd power to the accelerometer.
+
+  - vddio-supply: phandle to the regulator that provides vddio power to the accelerometer.
+
 Example:
 
 	mma8453fc@1d {
-- 
2.7.4


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

* [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support
  2018-12-17  5:22 [PATCH V5 1/2] dt-bindings: iio: accel: mma8452: add power supplies property Anson Huang
@ 2018-12-17  5:22 ` Anson Huang
  2018-12-22 17:20   ` Jonathan Cameron
  2018-12-17 21:42 ` [PATCH V5 1/2] dt-bindings: iio: accel: mma8452: add power supplies property Rob Herring
  1 sibling, 1 reply; 5+ messages in thread
From: Anson Huang @ 2018-12-17  5:22 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, martink,
	gregkh, gustavo, Leonard Crestez, rtresidd, linux-iio,
	devicetree, linux-kernel, festevam, preid
  Cc: dl-linux-imx

The accelerometer's power supply could be controllable on some
platforms, such as i.MX6Q-SABRESD board, the mma8451's power supplies
are controlled by a GPIO fixed regulator, need to make sure the
regulators are enabled before any communication with mma8451, this
patch adds vdd/vddio regulator operation support.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Acked-by: Martin Kepplinger <martink@posteo.de>
---
ChangeLog since V4:
    - using devm_regulator_get() instead of devm_regulator_get_optional() since the regulator is
      there always, if dtb does NOT specify one, regulator framework will assign dummy regulator for it.
---
 drivers/iio/accel/mma8452.c | 183 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 172 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 421a0a8..9a52426 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -31,6 +31,7 @@
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 
 #define MMA8452_STATUS				0x00
 #define  MMA8452_STATUS_DRDY			(BIT(2) | BIT(1) | BIT(0))
@@ -107,6 +108,8 @@ struct mma8452_data {
 	u8 data_cfg;
 	const struct mma_chip_info *chip_info;
 	int sleep_val;
+	struct regulator *vdd_reg;
+	struct regulator *vddio_reg;
 };
 
  /**
@@ -1534,9 +1537,37 @@ static int mma8452_probe(struct i2c_client *client,
 	mutex_init(&data->lock);
 	data->chip_info = match->data;
 
+	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(data->vdd_reg)) {
+		ret = PTR_ERR(data->vdd_reg);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&client->dev, "failed to get VDD regulator!\n");
+		return ret;
+	}
+
+	ret = regulator_enable(data->vdd_reg);
+	if (ret) {
+		dev_err(&client->dev, "failed to enable VDD regulator!\n");
+		return ret;
+	}
+
+	data->vddio_reg = devm_regulator_get(&client->dev, "vddio");
+	if (IS_ERR(data->vddio_reg)) {
+		ret = PTR_ERR(data->vddio_reg);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&client->dev, "failed to get VDDIO regulator!\n");
+		goto disable_regulator_vdd;
+	}
+
+	ret = regulator_enable(data->vddio_reg);
+	if (ret) {
+		dev_err(&client->dev, "failed to enable VDDIO regulator!\n");
+		goto disable_regulator_vdd;
+	}
+
 	ret = i2c_smbus_read_byte_data(client, MMA8452_WHO_AM_I);
 	if (ret < 0)
-		return ret;
+		goto disable_regulators;
 
 	switch (ret) {
 	case MMA8451_DEVICE_ID:
@@ -1549,7 +1580,8 @@ static int mma8452_probe(struct i2c_client *client,
 			break;
 		/* else: fall through */
 	default:
-		return -ENODEV;
+		ret = -ENODEV;
+		goto disable_regulators;
 	}
 
 	dev_info(&client->dev, "registering %s accelerometer; ID 0x%x\n",
@@ -1566,13 +1598,13 @@ static int mma8452_probe(struct i2c_client *client,
 
 	ret = mma8452_reset(client);
 	if (ret < 0)
-		return ret;
+		goto disable_regulators;
 
 	data->data_cfg = MMA8452_DATA_CFG_FS_2G;
 	ret = i2c_smbus_write_byte_data(client, MMA8452_DATA_CFG,
 					data->data_cfg);
 	if (ret < 0)
-		return ret;
+		goto disable_regulators;
 
 	/*
 	 * By default set transient threshold to max to avoid events if
@@ -1581,7 +1613,7 @@ static int mma8452_probe(struct i2c_client *client,
 	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS,
 					MMA8452_TRANSIENT_THS_MASK);
 	if (ret < 0)
-		return ret;
+		goto disable_regulators;
 
 	if (client->irq) {
 		int irq2;
@@ -1595,7 +1627,7 @@ static int mma8452_probe(struct i2c_client *client,
 						MMA8452_CTRL_REG5,
 						data->chip_info->all_events);
 			if (ret < 0)
-				return ret;
+				goto disable_regulators;
 
 			dev_dbg(&client->dev, "using interrupt line INT1\n");
 		}
@@ -1604,11 +1636,11 @@ static int mma8452_probe(struct i2c_client *client,
 					MMA8452_CTRL_REG4,
 					data->chip_info->enabled_events);
 		if (ret < 0)
-			return ret;
+			goto disable_regulators;
 
 		ret = mma8452_trigger_setup(indio_dev);
 		if (ret < 0)
-			return ret;
+			goto disable_regulators;
 	}
 
 	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
@@ -1661,12 +1693,20 @@ static int mma8452_probe(struct i2c_client *client,
 trigger_cleanup:
 	mma8452_trigger_cleanup(indio_dev);
 
+disable_regulators:
+	regulator_disable(data->vddio_reg);
+
+disable_regulator_vdd:
+	regulator_disable(data->vdd_reg);
+
 	return ret;
 }
 
 static int mma8452_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int ret;
 
 	iio_device_unregister(indio_dev);
 
@@ -1678,6 +1718,18 @@ static int mma8452_remove(struct i2c_client *client)
 	mma8452_trigger_cleanup(indio_dev);
 	mma8452_standby(iio_priv(indio_dev));
 
+	ret = regulator_disable(data->vddio_reg);
+	if (ret) {
+		dev_err(&client->dev, "failed to disable VDDIO regulator\n");
+		return ret;
+	}
+
+	ret = regulator_disable(data->vdd_reg);
+	if (ret) {
+		dev_err(&client->dev, "failed to disable VDD regulator\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -1696,6 +1748,18 @@ static int mma8452_runtime_suspend(struct device *dev)
 		return -EAGAIN;
 	}
 
+	ret = regulator_disable(data->vddio_reg);
+	if (ret) {
+		dev_err(dev, "failed to disable VDDIO regulator\n");
+		return ret;
+	}
+
+	ret = regulator_disable(data->vdd_reg);
+	if (ret) {
+		dev_err(dev, "failed to disable VDD regulator\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -1705,9 +1769,22 @@ static int mma8452_runtime_resume(struct device *dev)
 	struct mma8452_data *data = iio_priv(indio_dev);
 	int ret, sleep_val;
 
+	ret = regulator_enable(data->vdd_reg);
+	if (ret) {
+		dev_err(dev, "failed to enable VDD regulator\n");
+		return ret;
+	}
+
+	ret = regulator_enable(data->vddio_reg);
+	if (ret) {
+		dev_err(dev, "failed to enable VDDIO regulator\n");
+		regulator_disable(data->vdd_reg);
+		return ret;
+	}
+
 	ret = mma8452_active(data);
 	if (ret < 0)
-		return ret;
+		goto runtime_resume_failed;
 
 	ret = mma8452_get_odr_index(data);
 	sleep_val = 1000 / mma8452_samp_freq[ret][0];
@@ -1717,20 +1794,104 @@ static int mma8452_runtime_resume(struct device *dev)
 		msleep_interruptible(sleep_val);
 
 	return 0;
+
+runtime_resume_failed:
+	regulator_disable(data->vddio_reg);
+	regulator_disable(data->vdd_reg);
+
+	return ret;
 }
 #endif
 
 #ifdef CONFIG_PM_SLEEP
 static int mma8452_suspend(struct device *dev)
 {
-	return mma8452_standby(iio_priv(i2c_get_clientdata(
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regulator_enable(data->vdd_reg);
+	if (ret) {
+		dev_err(dev, "failed to enable VDD regulator\n");
+		return ret;
+	}
+
+	ret = regulator_enable(data->vddio_reg);
+	if (ret) {
+		dev_err(dev, "failed to enable VDDIO regulator\n");
+		regulator_disable(data->vdd_reg);
+		return ret;
+	}
+
+	ret = mma8452_standby(iio_priv(i2c_get_clientdata(
 		to_i2c_client(dev))));
+	if (ret)
+		goto suspend_failed;
+
+	ret = regulator_disable(data->vddio_reg);
+	if (ret) {
+		dev_err(dev, "failed to disable VDDIO regulator\n");
+		return ret;
+	}
+
+	ret = regulator_disable(data->vdd_reg);
+	if (ret) {
+		dev_err(dev, "failed to disable VDD regulator\n");
+		return ret;
+	}
+
+	return 0;
+
+suspend_failed:
+	regulator_disable(data->vddio_reg);
+	regulator_disable(data->vdd_reg);
+
+	return ret;
 }
 
 static int mma8452_resume(struct device *dev)
 {
-	return mma8452_active(iio_priv(i2c_get_clientdata(
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regulator_enable(data->vdd_reg);
+	if (ret) {
+		dev_err(dev, "failed to enable VDD regulator\n");
+		return ret;
+	}
+
+	ret = regulator_enable(data->vddio_reg);
+	if (ret) {
+		dev_err(dev, "failed to enable VDDIO regulator\n");
+		regulator_disable(data->vdd_reg);
+		return ret;
+	}
+
+	ret = mma8452_active(iio_priv(i2c_get_clientdata(
 		to_i2c_client(dev))));
+	if (ret)
+		goto resume_failed;
+
+	ret = regulator_disable(data->vddio_reg);
+	if (ret) {
+		dev_err(dev, "failed to disable VDDIO regulator\n");
+		return ret;
+	}
+
+	ret = regulator_disable(data->vdd_reg);
+	if (ret) {
+		dev_err(dev, "failed to disable VDD regulator\n");
+		return ret;
+	}
+
+	return 0;
+
+resume_failed:
+	regulator_disable(data->vddio_reg);
+	regulator_disable(data->vdd_reg);
+
+	return ret;
 }
 #endif
 
-- 
2.7.4


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

* Re: [PATCH V5 1/2] dt-bindings: iio: accel: mma8452: add power supplies  property
  2018-12-17  5:22 [PATCH V5 1/2] dt-bindings: iio: accel: mma8452: add power supplies property Anson Huang
  2018-12-17  5:22 ` [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support Anson Huang
@ 2018-12-17 21:42 ` Rob Herring
  1 sibling, 0 replies; 5+ messages in thread
From: Rob Herring @ 2018-12-17 21:42 UTC (permalink / raw)
  To: Anson Huang
  Cc: jic23, knaack.h, lars, pmeerw, robh+dt, mark.rutland, martink,
	gregkh, gustavo, Leonard Crestez, rtresidd, linux-iio,
	devicetree, linux-kernel, festevam, preid, dl-linux-imx

On Mon, 17 Dec 2018 05:22:49 +0000, Anson Huang wrote:
> The accelerometer's power supplies could be controllable on some
> platforms, add property "vdd/vddio" power supply to let device tree
> to pass phandles to the regulators to driver.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  Documentation/devicetree/bindings/iio/accel/mma8452.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 

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

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

* Re: [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support
  2018-12-17  5:22 ` [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support Anson Huang
@ 2018-12-22 17:20   ` Jonathan Cameron
  2018-12-23  9:04     ` Anson Huang
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2018-12-22 17:20 UTC (permalink / raw)
  To: Anson Huang
  Cc: knaack.h, lars, pmeerw, robh+dt, mark.rutland, martink, gregkh,
	gustavo, Leonard Crestez, rtresidd, linux-iio, devicetree,
	linux-kernel, festevam, preid, dl-linux-imx

On Mon, 17 Dec 2018 05:22:55 +0000
Anson Huang <anson.huang@nxp.com> wrote:

> The accelerometer's power supply could be controllable on some
> platforms, such as i.MX6Q-SABRESD board, the mma8451's power supplies
> are controlled by a GPIO fixed regulator, need to make sure the
> regulators are enabled before any communication with mma8451, this
> patch adds vdd/vddio regulator operation support.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Acked-by: Martin Kepplinger <martink@posteo.de>

There is a lot of manual handling in the suspend and resume I would not expect
to see.  That should be using the runtime pm calls to take control of pm rather
than making assumptions about what runtime pm state we are in...

Jonathan

> ---
> ChangeLog since V4:
>     - using devm_regulator_get() instead of devm_regulator_get_optional() since the regulator is
>       there always, if dtb does NOT specify one, regulator framework will assign dummy regulator for it.
> ---
>  drivers/iio/accel/mma8452.c | 183 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 172 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 421a0a8..9a52426 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -31,6 +31,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  
>  #define MMA8452_STATUS				0x00
>  #define  MMA8452_STATUS_DRDY			(BIT(2) | BIT(1) | BIT(0))
> @@ -107,6 +108,8 @@ struct mma8452_data {
>  	u8 data_cfg;
>  	const struct mma_chip_info *chip_info;
>  	int sleep_val;
> +	struct regulator *vdd_reg;
> +	struct regulator *vddio_reg;
>  };
>  
>   /**
> @@ -1534,9 +1537,37 @@ static int mma8452_probe(struct i2c_client *client,
>  	mutex_init(&data->lock);
>  	data->chip_info = match->data;
>  
> +	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(data->vdd_reg)) {
> +		ret = PTR_ERR(data->vdd_reg);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&client->dev, "failed to get VDD regulator!\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable VDD regulator!\n");
> +		return ret;
> +	}

Please reorder so the unwind order in remove and devm cleanup is the
opposite of setup order.  I suspect that is just a case of
doing the two gets before enabling either regulator, but I haven't checked.

> +
> +	data->vddio_reg = devm_regulator_get(&client->dev, "vddio");
> +	if (IS_ERR(data->vddio_reg)) {
> +		ret = PTR_ERR(data->vddio_reg);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&client->dev, "failed to get VDDIO regulator!\n");
> +		goto disable_regulator_vdd;
> +	}
> +
> +	ret = regulator_enable(data->vddio_reg);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable VDDIO regulator!\n");
> +		goto disable_regulator_vdd;
> +	}
> +
>  	ret = i2c_smbus_read_byte_data(client, MMA8452_WHO_AM_I);
>  	if (ret < 0)
> -		return ret;
> +		goto disable_regulators;
>  
>  	switch (ret) {
>  	case MMA8451_DEVICE_ID:
> @@ -1549,7 +1580,8 @@ static int mma8452_probe(struct i2c_client *client,
>  			break;
>  		/* else: fall through */
>  	default:
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto disable_regulators;
>  	}
>  
>  	dev_info(&client->dev, "registering %s accelerometer; ID 0x%x\n",
> @@ -1566,13 +1598,13 @@ static int mma8452_probe(struct i2c_client *client,
>  
>  	ret = mma8452_reset(client);
>  	if (ret < 0)
> -		return ret;
> +		goto disable_regulators;
>  
>  	data->data_cfg = MMA8452_DATA_CFG_FS_2G;
>  	ret = i2c_smbus_write_byte_data(client, MMA8452_DATA_CFG,
>  					data->data_cfg);
>  	if (ret < 0)
> -		return ret;
> +		goto disable_regulators;
>  
>  	/*
>  	 * By default set transient threshold to max to avoid events if
> @@ -1581,7 +1613,7 @@ static int mma8452_probe(struct i2c_client *client,
>  	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS,
>  					MMA8452_TRANSIENT_THS_MASK);
>  	if (ret < 0)
> -		return ret;
> +		goto disable_regulators;
>  
>  	if (client->irq) {
>  		int irq2;
> @@ -1595,7 +1627,7 @@ static int mma8452_probe(struct i2c_client *client,
>  						MMA8452_CTRL_REG5,
>  						data->chip_info->all_events);
>  			if (ret < 0)
> -				return ret;
> +				goto disable_regulators;
>  
>  			dev_dbg(&client->dev, "using interrupt line INT1\n");
>  		}
> @@ -1604,11 +1636,11 @@ static int mma8452_probe(struct i2c_client *client,
>  					MMA8452_CTRL_REG4,
>  					data->chip_info->enabled_events);
>  		if (ret < 0)
> -			return ret;
> +			goto disable_regulators;
>  
>  		ret = mma8452_trigger_setup(indio_dev);
>  		if (ret < 0)
> -			return ret;
> +			goto disable_regulators;
>  	}
>  
>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
> @@ -1661,12 +1693,20 @@ static int mma8452_probe(struct i2c_client *client,
>  trigger_cleanup:
>  	mma8452_trigger_cleanup(indio_dev);
>  
> +disable_regulators:
> +	regulator_disable(data->vddio_reg);
> +
> +disable_regulator_vdd:
> +	regulator_disable(data->vdd_reg);
> +
>  	return ret;
>  }
>  
>  static int mma8452_remove(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
>  
>  	iio_device_unregister(indio_dev);
>  
> @@ -1678,6 +1718,18 @@ static int mma8452_remove(struct i2c_client *client)
>  	mma8452_trigger_cleanup(indio_dev);
>  	mma8452_standby(iio_priv(indio_dev));
>  
> +	ret = regulator_disable(data->vddio_reg);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to disable VDDIO regulator\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_disable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to disable VDD regulator\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1696,6 +1748,18 @@ static int mma8452_runtime_suspend(struct device *dev)
>  		return -EAGAIN;
>  	}
>  
> +	ret = regulator_disable(data->vddio_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to disable VDDIO regulator\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_disable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to disable VDD regulator\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1705,9 +1769,22 @@ static int mma8452_runtime_resume(struct device *dev)
>  	struct mma8452_data *data = iio_priv(indio_dev);
>  	int ret, sleep_val;
>  
> +	ret = regulator_enable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VDD regulator\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(data->vddio_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VDDIO regulator\n");
> +		regulator_disable(data->vdd_reg);
> +		return ret;
> +	}
> +
>  	ret = mma8452_active(data);
>  	if (ret < 0)
> -		return ret;
> +		goto runtime_resume_failed;
>  
>  	ret = mma8452_get_odr_index(data);
>  	sleep_val = 1000 / mma8452_samp_freq[ret][0];
> @@ -1717,20 +1794,104 @@ static int mma8452_runtime_resume(struct device *dev)
>  		msleep_interruptible(sleep_val);
>  
>  	return 0;
> +
> +runtime_resume_failed:
> +	regulator_disable(data->vddio_reg);
> +	regulator_disable(data->vdd_reg);
> +
> +	return ret;
>  }
>  #endif
>  
>  #ifdef CONFIG_PM_SLEEP
>  static int mma8452_suspend(struct device *dev)
>  {
> -	return mma8452_standby(iio_priv(i2c_get_clientdata(
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +

I'd rather see the runtime pm calls done than doing this directly.
So you should let the runtime pm know it needs to power the device on,
then take over control (disable runtime pm) and power down.

> +	ret = regulator_enable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VDD regulator\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(data->vddio_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VDDIO regulator\n");
> +		regulator_disable(data->vdd_reg);
> +		return ret;
> +	}
> +
> +	ret = mma8452_standby(iio_priv(i2c_get_clientdata(
>  		to_i2c_client(dev))));
> +	if (ret)
> +		goto suspend_failed;
> +
> +	ret = regulator_disable(data->vddio_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to disable VDDIO regulator\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_disable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to disable VDD regulator\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +suspend_failed:
> +	regulator_disable(data->vddio_reg);
> +	regulator_disable(data->vdd_reg);
> +
> +	return ret;
>  }
>  
>  static int mma8452_resume(struct device *dev)
>  {
> -	return mma8452_active(iio_priv(i2c_get_clientdata(
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regulator_enable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VDD regulator\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(data->vddio_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to enable VDDIO regulator\n");
> +		regulator_disable(data->vdd_reg);
> +		return ret;
> +	}
> +
> +	ret = mma8452_active(iio_priv(i2c_get_clientdata(
>  		to_i2c_client(dev))));
> +	if (ret)
> +		goto resume_failed;
> +
> +	ret = regulator_disable(data->vddio_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to disable VDDIO regulator\n");
> +		return ret;

This is odd in a resume path to turn it off.  Normally you just hand
off to the runtime pm again to autosuspend as necessary.

> +	}
> +
> +	ret = regulator_disable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(dev, "failed to disable VDD regulator\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +resume_failed:
> +	regulator_disable(data->vddio_reg);
> +	regulator_disable(data->vdd_reg);
> +
> +	return ret;
>  }
>  #endif
>  


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

* RE: [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support
  2018-12-22 17:20   ` Jonathan Cameron
@ 2018-12-23  9:04     ` Anson Huang
  0 siblings, 0 replies; 5+ messages in thread
From: Anson Huang @ 2018-12-23  9:04 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: knaack.h, lars, pmeerw, robh+dt, mark.rutland, martink, gregkh,
	gustavo, Leonard Crestez, rtresidd, linux-iio, devicetree,
	linux-kernel, festevam, preid, dl-linux-imx

Hi, Jonathan

Best Regards!
Anson Huang

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 2018Äê12ÔÂ23ÈÕ 1:20
> To: Anson Huang <anson.huang@nxp.com>
> Cc: knaack.h@gmx.de; lars@metafoo.de; pmeerw@pmeerw.net;
> robh+dt@kernel.org; mark.rutland@arm.com; martink@posteo.de;
> gregkh@linuxfoundation.org; gustavo@embeddedor.com; Leonard Crestez
> <leonard.crestez@nxp.com>; rtresidd@electromag.com.au;
> linux-iio@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; festevam@gmail.com;
> preid@electromag.com.au; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator
> operation support
> 
> On Mon, 17 Dec 2018 05:22:55 +0000
> Anson Huang <anson.huang@nxp.com> wrote:
> 
> > The accelerometer's power supply could be controllable on some
> > platforms, such as i.MX6Q-SABRESD board, the mma8451's power supplies
> > are controlled by a GPIO fixed regulator, need to make sure the
> > regulators are enabled before any communication with mma8451, this
> > patch adds vdd/vddio regulator operation support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > Acked-by: Martin Kepplinger <martink@posteo.de>
> 
> There is a lot of manual handling in the suspend and resume I would not
> expect to see.  That should be using the runtime pm calls to take control of
> pm rather than making assumptions about what runtime pm state we are in...

Yes, but I found we can just use pm_runtime_force_suspend/resume for suspend/resume
operations, since the operations are same, it will simply the code and I tested it, it works.

Anson.

> 
> Jonathan
> 
> > ---
> > ChangeLog since V4:
> >     - using devm_regulator_get() instead of devm_regulator_get_optional()
> since the regulator is
> >       there always, if dtb does NOT specify one, regulator framework will
> assign dummy regulator for it.
> > ---
> >  drivers/iio/accel/mma8452.c | 183
> > +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 172 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> > index 421a0a8..9a52426 100644
> > --- a/drivers/iio/accel/mma8452.c
> > +++ b/drivers/iio/accel/mma8452.c
> > @@ -31,6 +31,7 @@
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >
> >  #define MMA8452_STATUS				0x00
> >  #define  MMA8452_STATUS_DRDY			(BIT(2) | BIT(1) | BIT(0))
> > @@ -107,6 +108,8 @@ struct mma8452_data {
> >  	u8 data_cfg;
> >  	const struct mma_chip_info *chip_info;
> >  	int sleep_val;
> > +	struct regulator *vdd_reg;
> > +	struct regulator *vddio_reg;
> >  };
> >
> >   /**
> > @@ -1534,9 +1537,37 @@ static int mma8452_probe(struct i2c_client
> *client,
> >  	mutex_init(&data->lock);
> >  	data->chip_info = match->data;
> >
> > +	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> > +	if (IS_ERR(data->vdd_reg)) {
> > +		ret = PTR_ERR(data->vdd_reg);
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&client->dev, "failed to get VDD regulator!\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to enable VDD regulator!\n");
> > +		return ret;
> > +	}
> 
> Please reorder so the unwind order in remove and devm cleanup is the
> opposite of setup order.  I suspect that is just a case of doing the two gets
> before enabling either regulator, but I haven't checked.
> 
> > +
> > +	data->vddio_reg = devm_regulator_get(&client->dev, "vddio");
> > +	if (IS_ERR(data->vddio_reg)) {
> > +		ret = PTR_ERR(data->vddio_reg);
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&client->dev, "failed to get VDDIO regulator!\n");
> > +		goto disable_regulator_vdd;
> > +	}
> > +
> > +	ret = regulator_enable(data->vddio_reg);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to enable VDDIO regulator!\n");
> > +		goto disable_regulator_vdd;
> > +	}
> > +
> >  	ret = i2c_smbus_read_byte_data(client, MMA8452_WHO_AM_I);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto disable_regulators;
> >
> >  	switch (ret) {
> >  	case MMA8451_DEVICE_ID:
> > @@ -1549,7 +1580,8 @@ static int mma8452_probe(struct i2c_client
> *client,
> >  			break;
> >  		/* else: fall through */
> >  	default:
> > -		return -ENODEV;
> > +		ret = -ENODEV;
> > +		goto disable_regulators;
> >  	}
> >
> >  	dev_info(&client->dev, "registering %s accelerometer; ID 0x%x\n", @@
> > -1566,13 +1598,13 @@ static int mma8452_probe(struct i2c_client
> > *client,
> >
> >  	ret = mma8452_reset(client);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto disable_regulators;
> >
> >  	data->data_cfg = MMA8452_DATA_CFG_FS_2G;
> >  	ret = i2c_smbus_write_byte_data(client, MMA8452_DATA_CFG,
> >  					data->data_cfg);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto disable_regulators;
> >
> >  	/*
> >  	 * By default set transient threshold to max to avoid events if @@
> > -1581,7 +1613,7 @@ static int mma8452_probe(struct i2c_client *client,
> >  	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS,
> >  					MMA8452_TRANSIENT_THS_MASK);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto disable_regulators;
> >
> >  	if (client->irq) {
> >  		int irq2;
> > @@ -1595,7 +1627,7 @@ static int mma8452_probe(struct i2c_client
> *client,
> >  						MMA8452_CTRL_REG5,
> >  						data->chip_info->all_events);
> >  			if (ret < 0)
> > -				return ret;
> > +				goto disable_regulators;
> >
> >  			dev_dbg(&client->dev, "using interrupt line INT1\n");
> >  		}
> > @@ -1604,11 +1636,11 @@ static int mma8452_probe(struct i2c_client
> *client,
> >  					MMA8452_CTRL_REG4,
> >  					data->chip_info->enabled_events);
> >  		if (ret < 0)
> > -			return ret;
> > +			goto disable_regulators;
> >
> >  		ret = mma8452_trigger_setup(indio_dev);
> >  		if (ret < 0)
> > -			return ret;
> > +			goto disable_regulators;
> >  	}
> >
> >  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE | @@ -1661,12 +1693,20
> @@
> > static int mma8452_probe(struct i2c_client *client,
> >  trigger_cleanup:
> >  	mma8452_trigger_cleanup(indio_dev);
> >
> > +disable_regulators:
> > +	regulator_disable(data->vddio_reg);
> > +
> > +disable_regulator_vdd:
> > +	regulator_disable(data->vdd_reg);
> > +
> >  	return ret;
> >  }
> >
> >  static int mma8452_remove(struct i2c_client *client)  {
> >  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct mma8452_data *data = iio_priv(indio_dev);
> > +	int ret;
> >
> >  	iio_device_unregister(indio_dev);
> >
> > @@ -1678,6 +1718,18 @@ static int mma8452_remove(struct i2c_client
> *client)
> >  	mma8452_trigger_cleanup(indio_dev);
> >  	mma8452_standby(iio_priv(indio_dev));
> >
> > +	ret = regulator_disable(data->vddio_reg);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to disable VDDIO regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_disable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to disable VDD regulator\n");
> > +		return ret;
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > @@ -1696,6 +1748,18 @@ static int mma8452_runtime_suspend(struct
> device *dev)
> >  		return -EAGAIN;
> >  	}
> >
> > +	ret = regulator_disable(data->vddio_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to disable VDDIO regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_disable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to disable VDD regulator\n");
> > +		return ret;
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > @@ -1705,9 +1769,22 @@ static int mma8452_runtime_resume(struct
> device *dev)
> >  	struct mma8452_data *data = iio_priv(indio_dev);
> >  	int ret, sleep_val;
> >
> > +	ret = regulator_enable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable VDD regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(data->vddio_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable VDDIO regulator\n");
> > +		regulator_disable(data->vdd_reg);
> > +		return ret;
> > +	}
> > +
> >  	ret = mma8452_active(data);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto runtime_resume_failed;
> >
> >  	ret = mma8452_get_odr_index(data);
> >  	sleep_val = 1000 / mma8452_samp_freq[ret][0]; @@ -1717,20
> +1794,104
> > @@ static int mma8452_runtime_resume(struct device *dev)
> >  		msleep_interruptible(sleep_val);
> >
> >  	return 0;
> > +
> > +runtime_resume_failed:
> > +	regulator_disable(data->vddio_reg);
> > +	regulator_disable(data->vdd_reg);
> > +
> > +	return ret;
> >  }
> >  #endif
> >
> >  #ifdef CONFIG_PM_SLEEP
> >  static int mma8452_suspend(struct device *dev)  {
> > -	return mma8452_standby(iio_priv(i2c_get_clientdata(
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct mma8452_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> 
> I'd rather see the runtime pm calls done than doing this directly.
> So you should let the runtime pm know it needs to power the device on, then
> take over control (disable runtime pm) and power down.
> 
> > +	ret = regulator_enable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable VDD regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(data->vddio_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable VDDIO regulator\n");
> > +		regulator_disable(data->vdd_reg);
> > +		return ret;
> > +	}
> > +
> > +	ret = mma8452_standby(iio_priv(i2c_get_clientdata(
> >  		to_i2c_client(dev))));
> > +	if (ret)
> > +		goto suspend_failed;
> > +
> > +	ret = regulator_disable(data->vddio_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to disable VDDIO regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_disable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to disable VDD regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +
> > +suspend_failed:
> > +	regulator_disable(data->vddio_reg);
> > +	regulator_disable(data->vdd_reg);
> > +
> > +	return ret;
> >  }
> >
> >  static int mma8452_resume(struct device *dev)  {
> > -	return mma8452_active(iio_priv(i2c_get_clientdata(
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct mma8452_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = regulator_enable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable VDD regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_enable(data->vddio_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable VDDIO regulator\n");
> > +		regulator_disable(data->vdd_reg);
> > +		return ret;
> > +	}
> > +
> > +	ret = mma8452_active(iio_priv(i2c_get_clientdata(
> >  		to_i2c_client(dev))));
> > +	if (ret)
> > +		goto resume_failed;
> > +
> > +	ret = regulator_disable(data->vddio_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to disable VDDIO regulator\n");
> > +		return ret;
> 
> This is odd in a resume path to turn it off.  Normally you just hand off to the
> runtime pm again to autosuspend as necessary.
> 
> > +	}
> > +
> > +	ret = regulator_disable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(dev, "failed to disable VDD regulator\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +
> > +resume_failed:
> > +	regulator_disable(data->vddio_reg);
> > +	regulator_disable(data->vdd_reg);
> > +
> > +	return ret;
> >  }
> >  #endif
> >


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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-17  5:22 [PATCH V5 1/2] dt-bindings: iio: accel: mma8452: add power supplies property Anson Huang
2018-12-17  5:22 ` [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support Anson Huang
2018-12-22 17:20   ` Jonathan Cameron
2018-12-23  9:04     ` Anson Huang
2018-12-17 21:42 ` [PATCH V5 1/2] dt-bindings: iio: accel: mma8452: add power supplies property Rob Herring

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox