Re: [PATCH V4] iio: magnetometer: mag3110: add optional vdd/vddio regulator operation support

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 11 Dec 2018 05:06:20 +0000
Anson Huang <anson.huang@nxp.com> wrote:

> The magnetometer's power supply could be controlled by regulator
> on some platforms, such as i.MX6Q-SABRESD board, the mag3110's
> power supply is controlled by a GPIO fixed regulator, need to make
> sure the regulator is enabled before any communication with mag3110,
> this patch adds optional vdd/vddio regulator operation support.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

This device currently has documented bindings in trivial-devices.txt
Adding these non trivial elements means that we need to take it out
of there and add a specific binding doc for it.

So please provide a binding doc and drop the entry in trivial-devices.
Remember to cc devicetree list and maintainers for that.

Code looks good, but as we have missed the coming merge window
anyway, we have plenty of time to tidy up those docs.

Thanks,

Jonathan

> ---
> ChangeLog since V3:
> 	- add disabling vdd in vddio error path;
> 	- improve the regulator enable/disable sequence.
> ---
>  drivers/iio/magnetometer/mag3110.c | 96 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 88 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/mag3110.c b/drivers/iio/magnetometer/mag3110.c
> index f063355..328c0c4 100644
> --- a/drivers/iio/magnetometer/mag3110.c
> +++ b/drivers/iio/magnetometer/mag3110.c
> @@ -20,6 +20,7 @@
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
>  
>  #define MAG3110_STATUS 0x00
>  #define MAG3110_OUT_X 0x01 /* MSB first */
> @@ -56,6 +57,8 @@ struct mag3110_data {
>  	struct mutex lock;
>  	u8 ctrl_reg1;
>  	int sleep_val;
> +	struct regulator *vdd_reg;
> +	struct regulator *vddio_reg;
>  };
>  
>  static int mag3110_request(struct mag3110_data *data)
> @@ -469,17 +472,46 @@ static int mag3110_probe(struct i2c_client *client,
>  	struct iio_dev *indio_dev;
>  	int ret;
>  
> -	ret = i2c_smbus_read_byte_data(client, MAG3110_WHO_AM_I);
> -	if (ret < 0)
> -		return ret;
> -	if (ret != MAG3110_DEVICE_ID)
> -		return -ENODEV;
> -
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	data = iio_priv(indio_dev);
> +
> +	data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
> +	if (!IS_ERR(data->vdd_reg)) {
> +		ret = regulator_enable(data->vdd_reg);
> +		if (ret) {
> +			dev_err(&client->dev, "failed to enable VDD regulator\n");
> +			return ret;
> +		}
> +	} else {
> +		ret = PTR_ERR(data->vdd_reg);
> +		if (ret != -ENODEV)
> +			return ret;
> +	}
> +
> +	data->vddio_reg = devm_regulator_get_optional(&client->dev, "vddio");
> +	if (!IS_ERR(data->vddio_reg)) {
> +		ret = regulator_enable(data->vddio_reg);
> +		if (ret) {
> +			dev_err(&client->dev, "failed to enable VDDIO regulator\n");
> +			goto disable_regulator_vdd;
> +		}
> +	} else {
> +		ret = PTR_ERR(data->vddio_reg);
> +		if (ret != -ENODEV)
> +			goto disable_regulator_vdd;
> +	}
> +
> +	ret = i2c_smbus_read_byte_data(client, MAG3110_WHO_AM_I);
> +	if (ret < 0)
> +		goto disable_regulators;
> +	if (ret != MAG3110_DEVICE_ID) {
> +		ret = -ENODEV;
> +		goto disable_regulators;
> +	}
> +
>  	data->client = client;
>  	mutex_init(&data->lock);
>  
> @@ -499,7 +531,7 @@ static int mag3110_probe(struct i2c_client *client,
>  
>  	ret = mag3110_change_config(data, MAG3110_CTRL_REG1, data->ctrl_reg1);
>  	if (ret < 0)
> -		return ret;
> +		goto disable_regulators;
>  
>  	ret = i2c_smbus_write_byte_data(client, MAG3110_CTRL_REG2,
>  		MAG3110_CTRL_AUTO_MRST_EN);
> @@ -520,6 +552,13 @@ static int mag3110_probe(struct i2c_client *client,
>  	iio_triggered_buffer_cleanup(indio_dev);
>  standby_on_error:
>  	mag3110_standby(iio_priv(indio_dev));
> +disable_regulators:
> +	if (!IS_ERR(data->vddio_reg))
> +		regulator_disable(data->vddio_reg);
> +disable_regulator_vdd:
> +	if (!IS_ERR(data->vdd_reg))
> +		regulator_disable(data->vdd_reg);
> +
>  	return ret;
>  }
>  
> @@ -537,14 +576,55 @@ static int mag3110_remove(struct i2c_client *client)
>  #ifdef CONFIG_PM_SLEEP
>  static int mag3110_suspend(struct device *dev)
>  {
> -	return mag3110_standby(iio_priv(i2c_get_clientdata(
> +	struct mag3110_data *data = iio_priv(i2c_get_clientdata(
> +		to_i2c_client(dev)));
> +	int ret;
> +
> +	ret = mag3110_standby(iio_priv(i2c_get_clientdata(
>  		to_i2c_client(dev))));
> +	if (ret)
> +		return ret;
> +
> +	if (!IS_ERR(data->vddio_reg)) {
> +		ret = regulator_disable(data->vddio_reg);
> +		if (ret) {
> +			dev_err(dev, "failed to disable VDDIO regulator\n");
> +			return ret;
> +		}
> +	}
> +	if (!IS_ERR(data->vdd_reg)) {
> +		ret = regulator_disable(data->vdd_reg);
> +		if (ret) {
> +			dev_err(dev, "failed to disable VDD regulator\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static int mag3110_resume(struct device *dev)
>  {
>  	struct mag3110_data *data = iio_priv(i2c_get_clientdata(
>  		to_i2c_client(dev)));
> +	int ret;
> +
> +	if (!IS_ERR(data->vdd_reg)) {
> +		ret = regulator_enable(data->vdd_reg);
> +		if (ret) {
> +			dev_err(dev, "failed to enable VDD regulator\n");
> +			return ret;
> +		}
> +	}
> +	if (!IS_ERR(data->vddio_reg)) {
> +		ret = regulator_enable(data->vddio_reg);
> +		if (ret) {
> +			dev_err(dev, "failed to enable VDDIO regulator\n");
> +			if (!IS_ERR(data->vdd_reg))
> +				regulator_disable(data->vdd_reg);
> +			return ret;
> +		}
> +	}
>  
>  	return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
>  		data->ctrl_reg1);