| Message ID | 20240414175716.958831-2-aren@peacevolution.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: stk3310: support powering off during suspend | expand |
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote: > > From: Ondrej Jirman <megi@xff.cz> > > VDD power input can be used to completely power off the chip during > system suspend. Do so if available. ... > #include <linux/iio/events.h> > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > +#include <linux/regulator/consumer.h> Move it to be ordered and add a blank line to separate iio/*.h group. ... > + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); > + if (IS_ERR(data->vdd_reg)) { > + ret = PTR_ERR(data->vdd_reg); > + if (ret == -ENODEV) > + data->vdd_reg = NULL; > + else Redundant 'else' when you follow the pattern "check for error condition first". > + return dev_err_probe(&client->dev, ret, > + "get regulator vdd failed\n"); > + } ... > + if (data->vdd_reg) { > + ret = regulator_enable(data->vdd_reg); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "regulator vdd enable failed\n"); > + > + usleep_range(1000, 2000); fsleep() > + } ... > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > + if (data->vdd_reg) > + regulator_disable(data->vdd_reg); I forgot to check the order of freeing resources, be sure you have no devm_*() releases happening before this call. ... > + usleep_range(1000, 2000); fsleep()
On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote: > > > > From: Ondrej Jirman <megi@xff.cz> > > > > VDD power input can be used to completely power off the chip during > > system suspend. Do so if available. > > ... > > > #include <linux/iio/events.h> > > #include <linux/iio/iio.h> > > #include <linux/iio/sysfs.h> > > > +#include <linux/regulator/consumer.h> > > Move it to be ordered and add a blank line to separate iio/*.h group. > > ... > > > + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); > > + if (IS_ERR(data->vdd_reg)) { > > + ret = PTR_ERR(data->vdd_reg); > > + if (ret == -ENODEV) > > + data->vdd_reg = NULL; > > > + else > > Redundant 'else' when you follow the pattern "check for error condition first". > > > + return dev_err_probe(&client->dev, ret, > > + "get regulator vdd failed\n"); > > + } > > ... > > > + if (data->vdd_reg) { > > + ret = regulator_enable(data->vdd_reg); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "regulator vdd enable failed\n"); > > + > > + usleep_range(1000, 2000); > > fsleep() > > > + } > > ... > > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > > + if (data->vdd_reg) > > + regulator_disable(data->vdd_reg); > > I forgot to check the order of freeing resources, be sure you have no > devm_*() releases happening before this call. If I understand what you're saying, this should be fine. The driver just uses devm to clean up acquired resources after remove is called. Or am I missing something and resources could be freed before calling stk3310_remove? > ... > > > + usleep_range(1000, 2000); > > fsleep() Everything else makes sense, I'll include those in v2 along with a patch to switch stk3310_init to dev_err_probe. Thanks for taking the time to review - Aren
On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote: > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote: ... > > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > > > + if (data->vdd_reg) > > > + regulator_disable(data->vdd_reg); > > > > I forgot to check the order of freeing resources, be sure you have no > > devm_*() releases happening before this call. > > If I understand what you're saying, this should be fine. The driver just > uses devm to clean up acquired resources after remove is called. Or am I > missing something and resources could be freed before calling > stk3310_remove? I'm not objecting to that. The point here is that the resources should be freed in the reversed order. devm-allocated resources are deferred to be freed after the explicit driver ->remove() callback. At the end it should not interleave with each other, i.o.w. it should be probe: devm followed by non-devm remove: non-devm only.
On Thu, Apr 18, 2024 at 06:56:09PM +0300, Andy Shevchenko wrote: > On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote: > > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote: > > ... > > > > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > > > > + if (data->vdd_reg) > > > > + regulator_disable(data->vdd_reg); > > > > > > I forgot to check the order of freeing resources, be sure you have no > > > devm_*() releases happening before this call. > > > > If I understand what you're saying, this should be fine. The driver just > > uses devm to clean up acquired resources after remove is called. Or am I > > missing something and resources could be freed before calling > > stk3310_remove? > > I'm not objecting to that. The point here is that the resources should > be freed in the reversed order. devm-allocated resources are deferred > to be freed after the explicit driver ->remove() callback. At the end > it should not interleave with each other, i.o.w. it should be > probe: devm followed by non-devm > remove: non-devm only. I think what you're describing is already the case, with the exception of parts of the probe function not changed in this patch mixing acquiring resources through devm with configuring the device. I hope I'm not being dense, thanks for the clarification - Aren
On Thu, Apr 18, 2024 at 8:50 PM Aren <aren@peacevolution.org> wrote: > On Thu, Apr 18, 2024 at 06:56:09PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote: > > > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote: > > > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote: ... > > > > I forgot to check the order of freeing resources, be sure you have no > > > > devm_*() releases happening before this call. > > > > > > If I understand what you're saying, this should be fine. The driver just > > > uses devm to clean up acquired resources after remove is called. Or am I > > > missing something and resources could be freed before calling > > > stk3310_remove? > > > > I'm not objecting to that. The point here is that the resources should > > be freed in the reversed order. devm-allocated resources are deferred > > to be freed after the explicit driver ->remove() callback. At the end > > it should not interleave with each other, i.o.w. it should be > > probe: devm followed by non-devm > > remove: non-devm only. > > I think what you're describing is already the case, with the exception > of parts of the probe function not changed in this patch mixing > acquiring resources through devm with configuring the device. Okay, then we are fine! > I hope I'm not being dense, thanks for the clarification
On Sun, 14 Apr 2024 13:57:14 -0400 Aren Moynihan <aren@peacevolution.org> wrote: > From: Ondrej Jirman <megi@xff.cz> > > VDD power input can be used to completely power off the chip during > system suspend. Do so if available. I'd make this non optional (relying on regulator framework providing us a stub for old DT etc) and pay the minor cost of potentially restoring registers when it was never powered down. Simpler code and likely anyone who is doing suspend / resume will have power control anyway. Jonathan > > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Aren Moynihan <aren@peacevolution.org> > --- > drivers/iio/light/stk3310.c | 56 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 53 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c > index 7b71ad71d78d..bfa090538df7 100644 > --- a/drivers/iio/light/stk3310.c > +++ b/drivers/iio/light/stk3310.c > @@ -16,6 +16,7 @@ > #include <linux/iio/events.h> > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > +#include <linux/regulator/consumer.h> > > #define STK3310_REG_STATE 0x00 > #define STK3310_REG_PSCTRL 0x01 > @@ -117,6 +118,7 @@ struct stk3310_data { > struct regmap_field *reg_int_ps; > struct regmap_field *reg_flag_psint; > struct regmap_field *reg_flag_nf; > + struct regulator *vdd_reg; > }; > > static const struct iio_event_spec stk3310_events[] = { > @@ -607,6 +609,16 @@ static int stk3310_probe(struct i2c_client *client) > > mutex_init(&data->lock); > > + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); This needs a comment on why it is optional. Generally power supply regulators are not, but I think the point here is to avoid restoring the registers if the chip wasn't powered down? This feels like an interesting gap in the regulator framework. For most cases we can rely on stub / fake regulator being created for always on supplies, but that doesn't let us elide the register writes. My gut feeling is do them unconditionally. Suspend / resume isn't that common that it will matter much. That would allow you to have this as devm_regulator_get() and drop handling of it not being provided. > + if (IS_ERR(data->vdd_reg)) { > + ret = PTR_ERR(data->vdd_reg); > + if (ret == -ENODEV) > + data->vdd_reg = NULL; > + else > + return dev_err_probe(&client->dev, ret, > + "get regulator vdd failed\n"); > + } > + > ret = stk3310_regmap_init(data); > if (ret < 0) > return ret; > @@ -617,9 +629,18 @@ static int stk3310_probe(struct i2c_client *client) > indio_dev->channels = stk3310_channels; > indio_dev->num_channels = ARRAY_SIZE(stk3310_channels); > > + if (data->vdd_reg) { > + ret = regulator_enable(data->vdd_reg); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "regulator vdd enable failed\n"); > + > + usleep_range(1000, 2000); > + } > + > ret = stk3310_init(indio_dev); > if (ret < 0) > - return ret; > + goto err_vdd_disable; > > if (client->irq > 0) { > ret = devm_request_threaded_irq(&client->dev, client->irq, > @@ -645,32 +666,61 @@ static int stk3310_probe(struct i2c_client *client) > > err_standby: > stk3310_set_state(data, STK3310_STATE_STANDBY); > +err_vdd_disable: > + if (data->vdd_reg) > + regulator_disable(data->vdd_reg); > return ret; > } > > static void stk3310_remove(struct i2c_client *client) > { > struct iio_dev *indio_dev = i2c_get_clientdata(client); > + struct stk3310_data *data = iio_priv(indio_dev); > > iio_device_unregister(indio_dev); > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > + if (data->vdd_reg) > + regulator_disable(data->vdd_reg); > } > > static int stk3310_suspend(struct device *dev) > { > struct stk3310_data *data; > + int ret; > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > > - return stk3310_set_state(data, STK3310_STATE_STANDBY); > + ret = stk3310_set_state(data, STK3310_STATE_STANDBY); > + if (ret) > + return ret; > + > + if (data->vdd_reg) { As above, I don't think we care enough about overhead on boards where there isn't a vdd regulator. Just do this unconditionally. > + regcache_mark_dirty(data->regmap); > + regulator_disable(data->vdd_reg); > + } > + > + return 0; > } > > static int stk3310_resume(struct device *dev) > { > - u8 state = 0; > struct stk3310_data *data; > + u8 state = 0; > + int ret; > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > + > + if (data->vdd_reg) { > + ret = regulator_enable(data->vdd_reg); > + if (ret) { > + dev_err(dev, "Failed to re-enable regulator vdd\n"); > + return ret; > + } > + > + usleep_range(1000, 2000); > + regcache_sync(data->regmap); > + } > + > if (data->ps_enabled) > state |= STK3310_STATE_EN_PS; > if (data->als_enabled)
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c index 7b71ad71d78d..bfa090538df7 100644 --- a/drivers/iio/light/stk3310.c +++ b/drivers/iio/light/stk3310.c @@ -16,6 +16,7 @@ #include <linux/iio/events.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> +#include <linux/regulator/consumer.h> #define STK3310_REG_STATE 0x00 #define STK3310_REG_PSCTRL 0x01 @@ -117,6 +118,7 @@ struct stk3310_data { struct regmap_field *reg_int_ps; struct regmap_field *reg_flag_psint; struct regmap_field *reg_flag_nf; + struct regulator *vdd_reg; }; static const struct iio_event_spec stk3310_events[] = { @@ -607,6 +609,16 @@ static int stk3310_probe(struct i2c_client *client) mutex_init(&data->lock); + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd"); + if (IS_ERR(data->vdd_reg)) { + ret = PTR_ERR(data->vdd_reg); + if (ret == -ENODEV) + data->vdd_reg = NULL; + else + return dev_err_probe(&client->dev, ret, + "get regulator vdd failed\n"); + } + ret = stk3310_regmap_init(data); if (ret < 0) return ret; @@ -617,9 +629,18 @@ static int stk3310_probe(struct i2c_client *client) indio_dev->channels = stk3310_channels; indio_dev->num_channels = ARRAY_SIZE(stk3310_channels); + if (data->vdd_reg) { + ret = regulator_enable(data->vdd_reg); + if (ret) + return dev_err_probe(&client->dev, ret, + "regulator vdd enable failed\n"); + + usleep_range(1000, 2000); + } + ret = stk3310_init(indio_dev); if (ret < 0) - return ret; + goto err_vdd_disable; if (client->irq > 0) { ret = devm_request_threaded_irq(&client->dev, client->irq, @@ -645,32 +666,61 @@ static int stk3310_probe(struct i2c_client *client) err_standby: stk3310_set_state(data, STK3310_STATE_STANDBY); +err_vdd_disable: + if (data->vdd_reg) + regulator_disable(data->vdd_reg); return ret; } static void stk3310_remove(struct i2c_client *client) { struct iio_dev *indio_dev = i2c_get_clientdata(client); + struct stk3310_data *data = iio_priv(indio_dev); iio_device_unregister(indio_dev); stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); + if (data->vdd_reg) + regulator_disable(data->vdd_reg); } static int stk3310_suspend(struct device *dev) { struct stk3310_data *data; + int ret; data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); - return stk3310_set_state(data, STK3310_STATE_STANDBY); + ret = stk3310_set_state(data, STK3310_STATE_STANDBY); + if (ret) + return ret; + + if (data->vdd_reg) { + regcache_mark_dirty(data->regmap); + regulator_disable(data->vdd_reg); + } + + return 0; } static int stk3310_resume(struct device *dev) { - u8 state = 0; struct stk3310_data *data; + u8 state = 0; + int ret; data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); + + if (data->vdd_reg) { + ret = regulator_enable(data->vdd_reg); + if (ret) { + dev_err(dev, "Failed to re-enable regulator vdd\n"); + return ret; + } + + usleep_range(1000, 2000); + regcache_sync(data->regmap); + } + if (data->ps_enabled) state |= STK3310_STATE_EN_PS; if (data->als_enabled)