RE: [PATCH V6] iio: magnetometer: mag3110: add vdd/vddio regulator operation support

[Anson Huang <anson.huang@nxp.com>]

Hi, Jonathan

Best Regards!
Anson Huang

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: 2019年1月6日 1:47
> To: Anson Huang <anson.huang@nxp.com>
> Cc: knaack.h@gmx.de; lars@metafoo.de; pmeerw@pmeerw.net;
> rtresidd@electromag.com.au; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V6] iio: magnetometer: mag3110: add vdd/vddio
> regulator operation support
> 
> On Sun, 23 Dec 2018 06:12:30 +0000
> Anson Huang <anson.huang@nxp.com> wrote:
> 
> > The magnetometer's power supplies could be controllable on some
> > platforms, such as i.MX6Q-SABRESD board, the mag3110's power supplies
> > are controlled by a GPIO fixed regulator, need to make sure the
> > regulators are enabled before any communication with mag3110, this
> > patch adds vdd/vddio regulator operation support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Same comment on not combining the error handling.  It's simply harder to
> read to save a few lines of code.

I separated the error handling in V7 patch, please help review, thanks,

Anson.

> 
> Otherwise, looks fine, though I would ideally like an ack from Peter on this as
> it's his driver. If he's busy though I think this should be safe enough without.
> 
> thanks,
> 
> Jonathan
> 
> > ---
> > ChangeLog since V5:
> >     Make sure both vdd and vddio regulators are aquired before enabling
> any one of them.
> > ---
> >  drivers/iio/magnetometer/mag3110.c | 88
> > ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 80 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/iio/magnetometer/mag3110.c
> > b/drivers/iio/magnetometer/mag3110.c
> > index f063355..f56a99a 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,44 @@ 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(&client->dev, "vdd");
> > +	data->vddio_reg = devm_regulator_get(&client->dev, "vddio");
> > +	if (IS_ERR(data->vdd_reg) || IS_ERR(data->vddio_reg)) {
> > +		if (PTR_ERR(data->vdd_reg) == -EPROBE_DEFER ||
> > +			PTR_ERR(data->vddio_reg) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		dev_err(&client->dev, "failed to get VDD/VDDIO regulator!\n");
> > +		return IS_ERR(data->vdd_reg) ?
> > +		       PTR_ERR(data->vdd_reg) : PTR_ERR(data->vddio_reg);
> > +	}
> > +
> > +	ret = regulator_enable(data->vdd_reg);
> > +	if (ret) {
> > +		dev_err(&client->dev, "failed to enable VDD regulator!\n");
> > +		return ret;
> > +	}
> > +
> > +	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, 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 +529,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,16 +550,24 @@ 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:
> > +	regulator_disable(data->vddio_reg);
> > +disable_regulator_vdd:
> > +	regulator_disable(data->vdd_reg);
> > +
> >  	return ret;
> >  }
> >
> >  static int mag3110_remove(struct i2c_client *client)  {
> >  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct mag3110_data *data = iio_priv(indio_dev);
> >
> >  	iio_device_unregister(indio_dev);
> >  	iio_triggered_buffer_cleanup(indio_dev);
> >  	mag3110_standby(iio_priv(indio_dev));
> > +	regulator_disable(data->vddio_reg);
> > +	regulator_disable(data->vdd_reg);
> >
> >  	return 0;
> >  }
> > @@ -537,14 +575,48 @@ 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;
> > +
> > +	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;
> >  }
> >
> >  static int mag3110_resume(struct device *dev)  {
> >  	struct mag3110_data *data = iio_priv(i2c_get_clientdata(
> >  		to_i2c_client(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;
> > +	}
> >
> >  	return i2c_smbus_write_byte_data(data->client, MAG3110_CTRL_REG1,
> >  		data->ctrl_reg1);