| Message ID | 20241028142000.1058149-4-aren@peacevolution.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: stk3310: support powering off during suspend | expand |
On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote: > The vdd and leda supplies must be powered on for the chip to function > and can be powered off during system suspend. > > Co-developed-by: Ondrej Jirman <megi@xff.cz> Missing SoB. Please, read Submitting Patches documentation for understanding what has to be done here. > Signed-off-by: Aren Moynihan <aren@peacevolution.org> ... > Notes: > I'm not sure what the proper way to handle attribution for this patch > is. It was origionally based on a patch by Ondrej Jirman[1], but I have > rewritten a large portion if it. I have included a Co-developed-by tag > to indicate this, but haven't sent him this patch, so I'm not sure what > to do about a Signed-off-by. Ah, seems you already aware of this issue. So, either drop Co-developed-by (and if you wish you may give a credit in a free form inside commit message) or make sure you get his SoB tag. ... > mutex_init(&data->lock); Somewhere (in the previous patch?) you want to switch to devm_mutex_init(). ... > + ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies), > + data->supplies); > + if (ret) > + return dev_err_probe(&client->dev, ret, "get regulators failed\n"); > + return dev_err_probe(&client->dev, ret, > + "regulator enable failed\n"); > + ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "failed to register regulator cleanup\n"); With struct devuce *dev = &client->dev; at the top of the function makes these and more lines neater. ... > static int stk3310_resume(struct device *dev) > { > - u8 state = 0; > struct stk3310_data *data; > + u8 state = 0; > + int ret; While changing to RCT order here, it seems you have inconsistent approach elsewhere (in your own patches!). Please, be consistent with chosen style. > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
On Mon, Oct 28, 2024 at 04:38:37PM +0200, Andy Shevchenko wrote: > On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote: > > The vdd and leda supplies must be powered on for the chip to function > > and can be powered off during system suspend. > > > > Co-developed-by: Ondrej Jirman <megi@xff.cz> > > Missing SoB. Please, read Submitting Patches documentation for understanding > what has to be done here. > > > Signed-off-by: Aren Moynihan <aren@peacevolution.org> > > ... > > > Notes: > > I'm not sure what the proper way to handle attribution for this patch > > is. It was origionally based on a patch by Ondrej Jirman[1], but I have > > rewritten a large portion if it. I have included a Co-developed-by tag > > to indicate this, but haven't sent him this patch, so I'm not sure what > > to do about a Signed-off-by. > > Ah, seems you already aware of this issue. So, either drop Co-developed-by > (and if you wish you may give a credit in a free form inside commit message) > or make sure you get his SoB tag. Alright, thanks for clarifying that. > > mutex_init(&data->lock); > > Somewhere (in the previous patch?) you want to switch to devm_mutex_init(). Good catch, it looks like that was being leaked before this refactor. Yeah that sounds like the right place, I'll include it in v4. > > + ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies), > > + data->supplies); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, "get regulators failed\n"); > > > + return dev_err_probe(&client->dev, ret, > > + "regulator enable failed\n"); > > > + ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "failed to register regulator cleanup\n"); > > With > > struct devuce *dev = &client->dev; > > at the top of the function makes these and more lines neater. > [snip] > > While changing to RCT order here, it seems you have inconsistent approach > elsewhere (in your own patches!). Please, be consistent with chosen style. Sounds easy enough to fix, I'll include these in v4. Thanks taking the time to review - Aren
On Mon, 28 Oct 2024 12:37:14 -0400 Aren Moynihan <aren@peacevolution.org> wrote: > On Mon, Oct 28, 2024 at 04:38:37PM +0200, Andy Shevchenko wrote: > > On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote: > > > The vdd and leda supplies must be powered on for the chip to function > > > and can be powered off during system suspend. > > > > > > Co-developed-by: Ondrej Jirman <megi@xff.cz> > > > > Missing SoB. Please, read Submitting Patches documentation for understanding > > what has to be done here. > > > > > Signed-off-by: Aren Moynihan <aren@peacevolution.org> > > > > ... > > > > > Notes: > > > I'm not sure what the proper way to handle attribution for this patch > > > is. It was origionally based on a patch by Ondrej Jirman[1], but I have > > > rewritten a large portion if it. I have included a Co-developed-by tag > > > to indicate this, but haven't sent him this patch, so I'm not sure what > > > to do about a Signed-off-by. > > > > Ah, seems you already aware of this issue. So, either drop Co-developed-by > > (and if you wish you may give a credit in a free form inside commit message) > > or make sure you get his SoB tag. > > Alright, thanks for clarifying that. > > > > mutex_init(&data->lock); > > > > Somewhere (in the previous patch?) you want to switch to devm_mutex_init(). > > Good catch, it looks like that was being leaked before this refactor. > Yeah that sounds like the right place, I'll include it in v4. Not really on the leaking. Take a look at the cleanup for devm_mutex_init(). It's debug only and not all that useful in most cases. However, it is good to not assume that now we have a devm_mutex_init() available that is easy to use. > > > > + ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies), > > > + data->supplies); > > > + if (ret) > > > + return dev_err_probe(&client->dev, ret, "get regulators failed\n"); > > > > > + return dev_err_probe(&client->dev, ret, > > > + "regulator enable failed\n"); > > > > > + ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data); > > > + if (ret) > > > + return dev_err_probe(&client->dev, ret, > > > + "failed to register regulator cleanup\n"); > > > > With > > > > struct devuce *dev = &client->dev; > > > > at the top of the function makes these and more lines neater. > > > [snip] > > > > While changing to RCT order here, it seems you have inconsistent approach > > elsewhere (in your own patches!). Please, be consistent with chosen style. > > Sounds easy enough to fix, I'll include these in v4. > > Thanks taking the time to review > - Aren
Hello Aren, On 2024-10-28 15:38, Andy Shevchenko wrote: > On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote: >> The vdd and leda supplies must be powered on for the chip to function >> and can be powered off during system suspend. >> >> Co-developed-by: Ondrej Jirman <megi@xff.cz> > > Missing SoB. Please, read Submitting Patches documentation for > understanding > what has to be done here. > >> Signed-off-by: Aren Moynihan <aren@peacevolution.org> > > ... > >> Notes: >> I'm not sure what the proper way to handle attribution for this >> patch >> is. It was origionally based on a patch by Ondrej Jirman[1], but I >> have >> rewritten a large portion if it. I have included a Co-developed-by >> tag >> to indicate this, but haven't sent him this patch, so I'm not sure >> what >> to do about a Signed-off-by. > > Ah, seems you already aware of this issue. So, either drop > Co-developed-by > (and if you wish you may give a credit in a free form inside commit > message) > or make sure you get his SoB tag. Perhaps the best and also easiest solution would be to provide an Originally-by tag for Ondrej, because that's what it is. The patch was written originally by Ondrej, but you've changed many parts of the patch while upstreaming it.
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c index 2e1aa551bdbc..f02b4d20c282 100644 --- a/drivers/iio/light/stk3310.c +++ b/drivers/iio/light/stk3310.c @@ -13,6 +13,8 @@ #include <linux/module.h> #include <linux/mod_devicetable.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> + #include <linux/iio/events.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> @@ -130,6 +132,7 @@ struct stk3310_data { struct regmap_field *reg_int_ps; struct regmap_field *reg_flag_psint; struct regmap_field *reg_flag_nf; + struct regulator_bulk_data supplies[2]; }; static const struct iio_event_spec stk3310_events[] = { @@ -621,6 +624,31 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private) return IRQ_HANDLED; } +static int stk3310_regulators_enable(struct stk3310_data *data) +{ + int ret; + + ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies); + if (ret) + return ret; + + /* we need a short delay to allow the chip time to power on */ + fsleep(1000); + + return 0; +} + +static void stk3310_regulators_disable(void *private) +{ + int ret; + struct stk3310_data *data = private; + struct device *dev = &data->client->dev; + + ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies); + if (ret) + dev_err(dev, "failed to disable regulators: %d\n", ret); +} + static int stk3310_probe(struct i2c_client *client) { int ret; @@ -642,6 +670,13 @@ static int stk3310_probe(struct i2c_client *client) mutex_init(&data->lock); + data->supplies[0].supply = "vdd"; + data->supplies[1].supply = "leda"; + ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies), + data->supplies); + if (ret) + return dev_err_probe(&client->dev, ret, "get regulators failed\n"); + ret = stk3310_regmap_init(data); if (ret < 0) return ret; @@ -652,6 +687,16 @@ static int stk3310_probe(struct i2c_client *client) indio_dev->channels = stk3310_channels; indio_dev->num_channels = ARRAY_SIZE(stk3310_channels); + ret = stk3310_regulators_enable(data); + if (ret) + return dev_err_probe(&client->dev, ret, + "regulator enable failed\n"); + + ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data); + if (ret) + return dev_err_probe(&client->dev, ret, + "failed to register regulator cleanup\n"); + ret = stk3310_init(indio_dev); if (ret < 0) return ret; @@ -682,18 +727,45 @@ static int stk3310_probe(struct i2c_client *client) 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; + + regcache_mark_dirty(data->regmap); + + ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies); + if (ret) { + dev_err(dev, "failed to disable regulators: %d\n", ret); + return ret; + } + + 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))); + + ret = stk3310_regulators_enable(data); + if (ret) { + dev_err(dev, "Failed to re-enable regulators: %d", ret); + return ret; + } + + ret = regcache_sync(data->regmap); + if (ret) { + dev_err(dev, "Failed to restore registers: %d\n", ret); + return ret; + } + if (data->ps_enabled) state |= STK3310_STATE_EN_PS; if (data->als_enabled)
The vdd and leda supplies must be powered on for the chip to function and can be powered off during system suspend. Co-developed-by: Ondrej Jirman <megi@xff.cz> Signed-off-by: Aren Moynihan <aren@peacevolution.org> --- Notes: I'm not sure what the proper way to handle attribution for this patch is. It was origionally based on a patch by Ondrej Jirman[1], but I have rewritten a large portion if it. I have included a Co-developed-by tag to indicate this, but haven't sent him this patch, so I'm not sure what to do about a Signed-off-by. 1: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82 Changes in v3: - use bulk regulators instead of two individual ones - handle cleanup using devm callbacks instead of the remove function Changes in v2: - always enable / disable regulators and rely on a dummy regulator if one isn't specified - replace usleep_range with fsleep - reorder includes so iio headers are last - add missing error handling to resume drivers/iio/light/stk3310.c | 76 ++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 2 deletions(-)