| Message ID | 20240220-hdc3020-pm-v1-2-d8e60dbe79e9@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: humidity: hdc3020: add power and reset management | expand |
On Tue, 20 Feb 2024 22:14:56 +0100 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > The HDC3020 sensor carries out periodic measurements during normal > operation, but as long as the power supply is enabled, it will carry on > in low-power modes. In order to avoid that and reduce power consumption, > the device can be switched to Trigger-on Demand mode, and if possible, > turn off its regulator. > > According to the datasheet, the maximum "Power Up Ready" is 5 ms. > > Add resume/suspend pm operations to manage measurement mode and > regulator state. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Hi Javier, Comments inline. Mainly that you should not have side effects if the power up fails and you should fail probe. Thanks, Jonathan > --- > drivers/iio/humidity/hdc3020.c | 81 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 65 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c > index 11ede97a31d7..0da5c5c41cd2 100644 > --- a/drivers/iio/humidity/hdc3020.c > +++ b/drivers/iio/humidity/hdc3020.c > @@ -20,6 +20,8 @@ > #include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/pm.h> > +#include <linux/regulator/consumer.h> > #include <linux/units.h> > > #include <asm/unaligned.h> > @@ -68,6 +70,7 @@ > > struct hdc3020_data { > struct i2c_client *client; > + struct regulator *vdd_supply; > /* > * Ensure that the sensor configuration (currently only heater is > * supported) will not be changed during the process of reading > @@ -551,9 +554,39 @@ static const struct iio_info hdc3020_info = { > .write_event_value = hdc3020_write_thresh, > }; > > -static void hdc3020_stop(void *data) > +static int hdc3020_power_on(struct hdc3020_data *data) > { > - hdc3020_exec_cmd((struct hdc3020_data *)data, HDC3020_EXIT_AUTO); > + int ret; > + > + ret = regulator_enable(data->vdd_supply); > + if (ret) > + return ret; > + > + fsleep(5000); > + > + if (data->client->irq) { > + /* > + * The alert output is activated by default upon power up, > + * hardware reset, and soft reset. Clear the status register. > + */ > + ret = hdc3020_exec_cmd(data, HDC3020_S_STATUS); > + if (ret) > + return ret; > + } > + > + return hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0); Expectation of a power on fail, in probe at least is it should cleanup after itself. It's messier in resume because there isn't anything sensible to do about it, but we should keep to the convention of no side effects on failure. As such if either of the later parts of this fail, you should power down the regulator before returning. > +} > + > +static int hdc3020_power_off(struct hdc3020_data *data) > +{ > + hdc3020_exec_cmd(data, HDC3020_EXIT_AUTO); > + > + return regulator_disable(data->vdd_supply); > +} > + > +static void hdc3020_exit(void *data) > +{ > + hdc3020_power_off((struct hdc3020_data *)data); Trivial but no need to cast a void * to anything as the C standard says this is fine as implicit. > } > > static int hdc3020_probe(struct i2c_client *client) > @@ -569,6 +602,8 @@ static int hdc3020_probe(struct i2c_client *client) > if (!indio_dev) > return -ENOMEM; > > + dev_set_drvdata(&client->dev, (void *)indio_dev); > + > data = iio_priv(indio_dev); > data->client = client; > mutex_init(&data->lock); > @@ -580,6 +615,14 @@ static int hdc3020_probe(struct i2c_client *client) > indio_dev->info = &hdc3020_info; > indio_dev->channels = hdc3020_channels; > indio_dev->num_channels = ARRAY_SIZE(hdc3020_channels); > + > + data->vdd_supply = devm_regulator_get(&client->dev, "vdd"); > + if (IS_ERR(data->vdd_supply)) > + return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply), > + "Unable to get VDD regulator\n"); > + > + hdc3020_power_on(data) Check return value. We want to fail probe if the power up didn't work! > + > if (client->irq) { > ret = devm_request_threaded_irq(&client->dev, client->irq, > NULL, hdc3020_interrupt_handler, > @@ -588,22 +631,9 @@ static int hdc3020_probe(struct i2c_client *client) > if (ret) > return dev_err_probe(&client->dev, ret, > "Failed to request IRQ\n"); > - > - /* > - * The alert output is activated by default upon power up, > - * hardware reset, and soft reset. Clear the status register. > - */ > - ret = hdc3020_exec_cmd(data, HDC3020_S_STATUS); > - if (ret) > - return ret; > } > > - ret = hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0); > - if (ret) > - return dev_err_probe(&client->dev, ret, > - "Unable to set up measurement\n"); > - > - ret = devm_add_action_or_reset(&data->client->dev, hdc3020_stop, data); > + ret = devm_add_action_or_reset(&data->client->dev, hdc3020_exit, data); > if (ret) > return ret; > > @@ -614,6 +644,24 @@ static int hdc3020_probe(struct i2c_client *client) > return 0; > } >
diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c index 11ede97a31d7..0da5c5c41cd2 100644 --- a/drivers/iio/humidity/hdc3020.c +++ b/drivers/iio/humidity/hdc3020.c @@ -20,6 +20,8 @@ #include <linux/interrupt.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/pm.h> +#include <linux/regulator/consumer.h> #include <linux/units.h> #include <asm/unaligned.h> @@ -68,6 +70,7 @@ struct hdc3020_data { struct i2c_client *client; + struct regulator *vdd_supply; /* * Ensure that the sensor configuration (currently only heater is * supported) will not be changed during the process of reading @@ -551,9 +554,39 @@ static const struct iio_info hdc3020_info = { .write_event_value = hdc3020_write_thresh, }; -static void hdc3020_stop(void *data) +static int hdc3020_power_on(struct hdc3020_data *data) { - hdc3020_exec_cmd((struct hdc3020_data *)data, HDC3020_EXIT_AUTO); + int ret; + + ret = regulator_enable(data->vdd_supply); + if (ret) + return ret; + + fsleep(5000); + + if (data->client->irq) { + /* + * The alert output is activated by default upon power up, + * hardware reset, and soft reset. Clear the status register. + */ + ret = hdc3020_exec_cmd(data, HDC3020_S_STATUS); + if (ret) + return ret; + } + + return hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0); +} + +static int hdc3020_power_off(struct hdc3020_data *data) +{ + hdc3020_exec_cmd(data, HDC3020_EXIT_AUTO); + + return regulator_disable(data->vdd_supply); +} + +static void hdc3020_exit(void *data) +{ + hdc3020_power_off((struct hdc3020_data *)data); } static int hdc3020_probe(struct i2c_client *client) @@ -569,6 +602,8 @@ static int hdc3020_probe(struct i2c_client *client) if (!indio_dev) return -ENOMEM; + dev_set_drvdata(&client->dev, (void *)indio_dev); + data = iio_priv(indio_dev); data->client = client; mutex_init(&data->lock); @@ -580,6 +615,14 @@ static int hdc3020_probe(struct i2c_client *client) indio_dev->info = &hdc3020_info; indio_dev->channels = hdc3020_channels; indio_dev->num_channels = ARRAY_SIZE(hdc3020_channels); + + data->vdd_supply = devm_regulator_get(&client->dev, "vdd"); + if (IS_ERR(data->vdd_supply)) + return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply), + "Unable to get VDD regulator\n"); + + hdc3020_power_on(data); + if (client->irq) { ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, hdc3020_interrupt_handler, @@ -588,22 +631,9 @@ static int hdc3020_probe(struct i2c_client *client) if (ret) return dev_err_probe(&client->dev, ret, "Failed to request IRQ\n"); - - /* - * The alert output is activated by default upon power up, - * hardware reset, and soft reset. Clear the status register. - */ - ret = hdc3020_exec_cmd(data, HDC3020_S_STATUS); - if (ret) - return ret; } - ret = hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0); - if (ret) - return dev_err_probe(&client->dev, ret, - "Unable to set up measurement\n"); - - ret = devm_add_action_or_reset(&data->client->dev, hdc3020_stop, data); + ret = devm_add_action_or_reset(&data->client->dev, hdc3020_exit, data); if (ret) return ret; @@ -614,6 +644,24 @@ static int hdc3020_probe(struct i2c_client *client) return 0; } +static int hdc3020_suspend(struct device *dev) +{ + struct iio_dev *iio_dev = dev_get_drvdata(dev); + struct hdc3020_data *data = iio_priv(iio_dev); + + return hdc3020_power_off(data); +} + +static int hdc3020_resume(struct device *dev) +{ + struct iio_dev *iio_dev = dev_get_drvdata(dev); + struct hdc3020_data *data = iio_priv(iio_dev); + + return hdc3020_power_on(data); +} + +static DEFINE_SIMPLE_DEV_PM_OPS(hdc3020_pm_ops, hdc3020_suspend, hdc3020_resume); + static const struct i2c_device_id hdc3020_id[] = { { "hdc3020" }, { "hdc3021" }, @@ -633,6 +681,7 @@ MODULE_DEVICE_TABLE(of, hdc3020_dt_ids); static struct i2c_driver hdc3020_driver = { .driver = { .name = "hdc3020", + .pm = pm_sleep_ptr(&hdc3020_pm_ops), .of_match_table = hdc3020_dt_ids, }, .probe = hdc3020_probe,
The HDC3020 sensor carries out periodic measurements during normal operation, but as long as the power supply is enabled, it will carry on in low-power modes. In order to avoid that and reduce power consumption, the device can be switched to Trigger-on Demand mode, and if possible, turn off its regulator. According to the datasheet, the maximum "Power Up Ready" is 5 ms. Add resume/suspend pm operations to manage measurement mode and regulator state. Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/iio/humidity/hdc3020.c | 81 +++++++++++++++++++++++++++++++++--------- 1 file changed, 65 insertions(+), 16 deletions(-)