On 12.01.19 20:11, Jonathan Cameron wrote:
> On Tue, 8 Jan 2019 14:48:48 +0000
> Anson Huang <anson.huang@xxxxxxx> wrote:
>
>> Hi, Martin
>>
>> From Anson's iPhone 6
>>
>>
>>> 在 2019年1月8日，19:41，Martin Kepplinger <martink@xxxxxxxxx> 写道：
>>>
>>>> On 08.01.19 10:14, 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 <Anson.Huang@xxxxxxx>
>>>> Acked-by: Martin Kepplinger <martink@xxxxxxxxx>
>>>> ---
>>>> ChangeLog Since V6:
>>>> - separate the error handling of regulators get to make code easy to read.
>>>> ---
>>>> drivers/iio/accel/mma8452.c | 105 ++++++++++++++++++++++++++++++++++----------
>>>> 1 file changed, 83 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>>>> index 421a0a8..3027811 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,39 @@ 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)) {
>>>> + if (PTR_ERR(data->vdd_reg) == -EPROBE_DEFER)
>>>> + return -EPROBE_DEFER;
>>>> +
>>>> + dev_err(&client->dev, "failed to get VDD regulator!\n");
>>>> + return PTR_ERR(data->vdd_reg);
>>>> + }
>>>> +
>>>> + data->vddio_reg = devm_regulator_get(&client->dev, "vddio");
>>>> + if (IS_ERR(data->vddio_reg)) {
>>>> + if (PTR_ERR(data->vddio_reg) == -EPROBE_DEFER)
>>>> + return -EPROBE_DEFER;
>>>> +
>>>> + dev_err(&client->dev, "failed to get VDDIO regulator!\n");
>>>> + return 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, MMA8452_WHO_AM_I);
>>>> if (ret < 0)
>>>> - return ret;
>>>> + goto disable_regulators;
>>>>
>>>> switch (ret) {
>>>> case MMA8451_DEVICE_ID:
>>>> @@ -1549,7 +1582,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 +1600,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 +1615,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 +1629,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 +1638,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 +1695,19 @@ 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);
>>>>
>>>> iio_device_unregister(indio_dev);
>>>>
>>>> @@ -1678,6 +1719,9 @@ static int mma8452_remove(struct i2c_client *client)
>>>> mma8452_trigger_cleanup(indio_dev);
>>>> mma8452_standby(iio_priv(indio_dev));
>>>>
>>>> + regulator_disable(data->vddio_reg);
>>>> + regulator_disable(data->vdd_reg);
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -1696,6 +1740,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 +1761,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,25 +1786,17 @@ static int mma8452_runtime_resume(struct device *dev)
>>>> msleep_interruptible(sleep_val);
>>>>
>>>> return 0;
>>>> -}
>>>> -#endif
>>>>
>>>> -#ifdef CONFIG_PM_SLEEP
>>>> -static int mma8452_suspend(struct device *dev)
>>>> -{
>>>> - return mma8452_standby(iio_priv(i2c_get_clientdata(
>>>> - to_i2c_client(dev))));
>>>> -}
>>>> +runtime_resume_failed:
>>>> + regulator_disable(data->vddio_reg);
>>>> + regulator_disable(data->vdd_reg);
>>>>
>>>> -static int mma8452_resume(struct device *dev)
>>>> -{
>>>> - return mma8452_active(iio_priv(i2c_get_clientdata(
>>>> - to_i2c_client(dev))));
>>>> + return ret;
>>>> }
>>>> #endif
>>>>
>>>> static const struct dev_pm_ops mma8452_pm_ops = {
>>>> - SET_SYSTEM_SLEEP_PM_OPS(mma8452_suspend, mma8452_resume)
>>>> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
>>>
>>> Just a quesion: Isn't it semantically a different change to replace
>>> mma8452_suspend() and mma8452_resume()? If not, why is it needed here?
>>>
>>> other than that, lgtm.
>>
>> It is because with regulator operation added, before system suspend, driver is in runtime suspend already, the suspend/resume callback need to deal with regulator again including error handling etc., and also need to disable runtime pm and re-enable it etc., this introduces too many complicated code/logic, also, the things suspend/resume callback does are actually same as runtime suspend/ resume, so just using pm_runtime_force_suspend/resume can save these duplicated operations and make code easy/clean.
>>
> Anson, please wrap below 80 chars. Some of us have old old old
> laptops, (though I admit I still have more than 80 chars ;)
>
> I 'think' Anson is correct that these end up running the same
> code, but with considerably less complexity an this is
> the reason the force functions exist. Often the runtime case
> is very similar to the full pm case so you can avoid the whole
> runtime resume then suspend dance by doing this.
>
> Anyhow I've applied it to the togreg branch of iio.git and pushed
> it out as testing. Hopefully we haven't missed any corner
> cases.
>
> Runtime PM always gives me a headache when I dig into exactly
> what the various dances are that go on.
>
Alright. If not too late:
Acked-by: Martin Kepplinger <martink@xxxxxxxxx>
thanks,
martin
