RE: [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support
Hi, Jonathan
Best Regards!
Anson Huang
> -Original Message-
> From: Jonathan Cameron [mailto:ji...@kernel.org]
> Sent: 2018年12月23日 1:20
> To: Anson Huang
> Cc: knaac...@gmx.de; l...@metafoo.de; pme...@pmeerw.net;
> robh...@kernel.org; mark.rutl...@arm.com; mart...@posteo.de;
> gre...@linuxfoundation.org; gust...@embeddedor.com; Leonard Crestez
> ; rtres...@electromag.com.au;
> linux-...@vger.kernel.org; devicet...@vger.kernel.org;
> linux-kernel@vger.kernel.org; feste...@gmail.com;
> pr...@electromag.com.au; dl-linux-imx
> Subject: Re: [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator
> operation support
>
> On Mon, 17 Dec 2018 05:22:55 +
> Anson Huang 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
> > Acked-by: Martin Kepplinger
>
> 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
> > #include
> > #include
> > +#include
> >
> > #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(>lock);
> > data->chip_info = match->data;
> >
> > + data->vdd_reg = devm_regulator_get(>dev, "vdd");
> > + if (IS_ERR(data->vdd_reg)) {
> > + ret = PTR_ERR(data->vdd_reg);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(>dev, "failed to get VDD regulator!\n");
> > + return ret;
> > + }
> > +
> > + ret = regulator_enable(data->vdd_reg);
> > + if (ret) {
> > + dev_err(>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(>dev, "vddio");
> > + if (IS_ERR(data->vddio_reg)) {
> > + ret = PTR_ERR(data->vddio_reg);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(>dev, "failed to get VDDIO
> > regulator!\n");
> > + goto disable_regulator_vdd;
> > + }
> > +
> > + ret = regulator_enable(data->vddio_reg);
> > + if (ret) {
> > + dev_err(>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;
>
Re: [PATCH V5 2/2] iio: accell: mma8452: add vdd/vddio regulator operation support
On Mon, 17 Dec 2018 05:22:55 +
Anson Huang 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
> Acked-by: Martin Kepplinger
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
> #include
> #include
> +#include
>
> #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(>lock);
> data->chip_info = match->data;
>
> + data->vdd_reg = devm_regulator_get(>dev, "vdd");
> + if (IS_ERR(data->vdd_reg)) {
> + ret = PTR_ERR(data->vdd_reg);
> + if (ret != -EPROBE_DEFER)
> + dev_err(>dev, "failed to get VDD regulator!\n");
> + return ret;
> + }
> +
> + ret = regulator_enable(data->vdd_reg);
> + if (ret) {
> + dev_err(>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(>dev, "vddio");
> + if (IS_ERR(data->vddio_reg)) {
> + ret = PTR_ERR(data->vddio_reg);
> + if (ret != -EPROBE_DEFER)
> + dev_err(>dev, "failed to get VDDIO
> regulator!\n");
> + goto disable_regulator_vdd;
> + }
> +
> + ret = regulator_enable(data->vddio_reg);
> + if (ret) {
> + dev_err(>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(>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(>dev, "using interrupt line INT1\n");
> }
> @@ -1604,11 +1636,11 @@
