Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Sun, Apr 28, 2024 at 05:53:37PM GMT, Jonathan Cameron wrote: > On Tue, 23 Apr 2024 18:33:05 -0400 > Aren Moynihan wrote: > > > From: Ondrej Jirman > > > > VDD power input can be used to completely power off the chip during > > system suspend. Do so if available. > > > > Signed-off-by: Ondrej Jirman > > Signed-off-by: Aren Moynihan > > Suggestions inline. Key thing is take the whole thing devm_ managed > and your life gets much easier. It is mixing the two approaches that > causes problems and often the best plan is to do everything in probe/remove > with devm_ calls to do the cleanup for you. Thank you for writing this up. I've been a bit distracted lately, but I'm hoping to find some time to implement this in a new revision soon. - Aren
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Tue, 23 Apr 2024 18:33:05 -0400
Aren Moynihan wrote:
> From: Ondrej Jirman
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
>
> Signed-off-by: Ondrej Jirman
> Signed-off-by: Aren Moynihan
Suggestions inline. Key thing is take the whole thing devm_ managed
and your life gets much easier. It is mixing the two approaches that
causes problems and often the best plan is to do everything in probe/remove
with devm_ calls to do the cleanup for you.
> ---
>
> Notes:
> 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 | 49 ++---
> 1 file changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 7b71ad71d78d..a0547eeca3e3 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -13,6 +13,8 @@
> #include
> #include
> #include
> +#include
> +
> #include
> #include
> #include
> @@ -117,6 +119,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 +610,10 @@ static int stk3310_probe(struct i2c_client *client)
>
> mutex_init(>lock);
>
> + data->vdd_reg = devm_regulator_get(>dev, "vdd");
> + if (IS_ERR(data->vdd_reg))
> + return dev_err_probe(>dev, ret, "get regulator vdd
> failed\n");
> +
> ret = stk3310_regmap_init(data);
> if (ret < 0)
> return ret;
> @@ -617,9 +624,17 @@ static int stk3310_probe(struct i2c_client *client)
> indio_dev->channels = stk3310_channels;
> indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
>
> + ret = regulator_enable(data->vdd_reg);
> + if (ret)
> + return dev_err_probe(>dev, ret,
> + "regulator vdd enable failed\n");
> +
> + /* we need a short delay to allow the chip time to power on */
> + fsleep(1000);
> +
> ret = stk3310_init(indio_dev);
> if (ret < 0)
> - return ret;
> + goto err_vdd_disable;
>
> if (client->irq > 0) {
> ret = devm_request_threaded_irq(>dev, client->irq,
> @@ -645,32 +660,60 @@ static int stk3310_probe(struct i2c_client *client)
>
> err_standby:
> stk3310_set_state(data, STK3310_STATE_STANDBY);
Move this handling to a devm_add_action_or_reset() callback in a precursor
patch.
That will fix the current ordering issue wrt to the irq registration.
Then use devm_iio_device_register() in that precursor patch.
> +err_vdd_disable:
> + regulator_disable(data->vdd_reg);
Add a devm_add_action_or_reset() callback to disable this regulator in this
patch.
Register that just after the enable.
That way the ordering will be maintained for all calls.
> 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);
> + regulator_disable(data->vdd_reg);
With above suggested changes, you can drop the remove function entirely.
> }
>
> 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);
> + 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)));
> +
> + ret = regulator_enable(data->vdd_reg);
> + if (ret) {
> + dev_err(dev, "Failed to re-enable regulator vdd\n");
> + return ret;
> + }
> +
> + fsleep(1000);
> +
> + 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)
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, 24 Apr 2024 18:20:41 +0300 Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > > wrote: > > ... > > > > > ret = stk3310_init(indio_dev); > > > > if (ret < 0) > > > > - return ret; > > > > + goto err_vdd_disable; > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > IRQ. Note, that the original code likely has a bug which sets states > > > before disabling IRQ and removing a handler. > > > > How so? stk3310_init is called before enabling the interrupt. > > Exactly, IRQ is registered with devm and hence the error path and > remove stages will got it in a wrong order. > > > Original code has a bug that IRQ is enabled before registering the > > IIO device, > > Indeed, but this is another bug. It shouldn't be. A device that produces interrupts before we have told it to is a) buggy, b) almost certainly already had it's interrupt masked due to spurious interrupt detection. Definitely don't want to do it in the opposite order where userspace could turn the device on and have it start generating interrupts before the irq is registered. I'd rather assume non buggy hardware (and that if there are bugs, the normal protections kick in) than introduce a race into the software. > > > so if IRQ is triggered before registration, iio_push_event > > from IRQ handler may be called on a not yet registered IIO device. > > > > Never saw it happen, though. :) > > Because nobody cares enough to enable DEBUG_SHIRQ In most devices there is a status register and we should be doing nothing unless that is set. Interestingly this device either doesn't have one or the driver doesn't read it - it reads a flag only and so will always push an event. Such a register read doesn't require the IIO device registration to be complete. There are corner cases where that isn't true that need to manually mask at the host but they are rare. There is also a basic level of defense in iio_push_event() against that being called when the event interface is not registered. Jonathan >
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, 24 Apr 2024 02:16:06 +0300
Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan wrote:
> >
> > From: Ondrej Jirman
> >
> > VDD power input can be used to completely power off the chip during
> > system suspend. Do so if available.
>
> ...
>
> > ret = stk3310_init(indio_dev);
> > if (ret < 0)
> > - return ret;
> > + goto err_vdd_disable;
>
> This is wrong. You will have the regulator being disabled _before_
> IRQ. Note, that the original code likely has a bug which sets states
> before disabling IRQ and removing a handler.
>
> Side note, you may make the driver neater with help of
>
> struct device *dev = >dev;
>
> defined in this patch.
>
> ...
>
> > static int stk3310_suspend(struct device *dev)
> > {
> > struct stk3310_data *data;
>
> > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>
> Side note: This may be updated (in a separate change) to use
> dev_get_drvdata() directly.
>
> Jonathan, do we have something like iio_priv_from_drvdata(struct
> device *dev)? Seems many drivers may utilise it.
Not yet, but I'm not sure it's a good idea as there is no inherent
reason to assume the drvdata is a struct iio_dev. It often is but
adding a function that assumes that is a path to subtle bugs.
Jonathan
>
> > }
>
> ...
>
> > static int stk3310_resume(struct device *dev)
>
> Ditto.
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
Hi Aren,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Aren-Moynihan/dt-bindings-iio-light-stk33xx-add-vdd-and-leda-regulators/20240424-064250
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:
https://lore.kernel.org/r/20240423223309.1468198-4-aren%40peacevolution.org
patch subject: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and
power it off during suspend
config: i386-randconfig-141-20240424
(https://download.01.org/0day-ci/archive/20240425/202404251021.4oper3os-...@intel.com/config)
compiler: gcc-10 (Ubuntu 10.5.0-1ubuntu1) 10.5.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202404251021.4oper3os-...@intel.com/
smatch warnings:
drivers/iio/light/stk3310.c:615 stk3310_probe() error: uninitialized symbol
'ret'.
vim +/ret +615 drivers/iio/light/stk3310.c
9046d80dce04c6 Uwe Kleine-König 2022-11-18 592 static int
stk3310_probe(struct i2c_client *client)
be9e6229d67696 Tiberiu Breana 2015-04-27 593 {
be9e6229d67696 Tiberiu Breana 2015-04-27 594 int ret;
be9e6229d67696 Tiberiu Breana 2015-04-27 595 struct iio_dev
*indio_dev;
be9e6229d67696 Tiberiu Breana 2015-04-27 596 struct stk3310_data
*data;
be9e6229d67696 Tiberiu Breana 2015-04-27 597
be9e6229d67696 Tiberiu Breana 2015-04-27 598 indio_dev =
devm_iio_device_alloc(>dev, sizeof(*data));
be9e6229d67696 Tiberiu Breana 2015-04-27 599 if (!indio_dev) {
be9e6229d67696 Tiberiu Breana 2015-04-27 600
dev_err(>dev, "iio allocation failed!\n");
be9e6229d67696 Tiberiu Breana 2015-04-27 601 return -ENOMEM;
be9e6229d67696 Tiberiu Breana 2015-04-27 602 }
be9e6229d67696 Tiberiu Breana 2015-04-27 603
be9e6229d67696 Tiberiu Breana 2015-04-27 604 data =
iio_priv(indio_dev);
be9e6229d67696 Tiberiu Breana 2015-04-27 605 data->client = client;
be9e6229d67696 Tiberiu Breana 2015-04-27 606
i2c_set_clientdata(client, indio_dev);
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 607
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 608
device_property_read_u32(>dev, "proximity-near-level",
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 609
>ps_near_level);
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 610
be9e6229d67696 Tiberiu Breana 2015-04-27 611 mutex_init(>lock);
be9e6229d67696 Tiberiu Breana 2015-04-27 612
dd231c1d219f6b Ondrej Jirman2024-04-23 613 data->vdd_reg =
devm_regulator_get(>dev, "vdd");
dd231c1d219f6b Ondrej Jirman2024-04-23 614 if
(IS_ERR(data->vdd_reg))
dd231c1d219f6b Ondrej Jirman2024-04-23 @615 return
dev_err_probe(>dev, ret, "get regulator vdd failed\n");
s/ret/PTR_ERR(data->vdd_reg)/
dd231c1d219f6b Ondrej Jirman2024-04-23 616
be9e6229d67696 Tiberiu Breana 2015-04-27 617 ret =
stk3310_regmap_init(data);
be9e6229d67696 Tiberiu Breana 2015-04-27 618 if (ret < 0)
be9e6229d67696 Tiberiu Breana 2015-04-27 619 return ret;
be9e6229d67696 Tiberiu Breana 2015-04-27 620
be9e6229d67696 Tiberiu Breana 2015-04-27 621 indio_dev->info =
_info;
be9e6229d67696 Tiberiu Breana 2015-04-27 622 indio_dev->name =
STK3310_DRIVER_NAME;
be9e6229d67696 Tiberiu Breana 2015-04-27 623 indio_dev->modes =
INDIO_DIRECT_MODE;
be9e6229d67696 Tiberiu Breana 2015-04-27 624 indio_dev->channels =
stk3310_channels;
be9e6229d67696 Tiberiu Breana 2015-04-27 625 indio_dev->num_channels
= ARRAY_SIZE(stk3310_channels);
be9e6229d67696 Tiberiu Breana 2015-04-27 626
dd231c1d219f6b Ondrej Jirman2024-04-23 627 ret =
regulator_enable(data->vdd_reg);
dd231c1d219f6b Ondrej Jirman2024-04-23 628 if (ret)
dd231c1d219f6b Ondrej Jirman2024-04-23 629 return
dev_err_probe(>dev, ret,
dd231c1d219f6b Ondrej Jirman2024-04-23 630
"regulator vdd enable failed\n");
dd231c1d219f6b Ondrej Jirman2024-04-23 631
dd231c1d219f6b Ondrej Jirman2024-04-23 632 /* we need a short
delay to allow the chip time to power on */
dd231c1d219f6b Ondrej Jirman2024-04-23 633 fsleep(1000);
dd231c1d219f6b Ondrej Jirman2024-04-23 634
be9e6229d67696 Tiberiu Breana 2015-04-27 635 ret =
stk3310_init(indio_dev);
be9e6229d67696 Tiberiu Breana 2015-04-27 636 if (ret < 0)
dd231c1d219f6b Ondrej Jirman2024-04-23 637 goto
err_
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 02:16:06AM +0300, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan wrote:
> >
> > From: Ondrej Jirman
> >
> > VDD power input can be used to completely power off the chip during
> > system suspend. Do so if available.
>
> ...
>
> > ret = stk3310_init(indio_dev);
> > if (ret < 0)
> > - return ret;
> > + goto err_vdd_disable;
>
> This is wrong. You will have the regulator being disabled _before_
> IRQ. Note, that the original code likely has a bug which sets states
> before disabling IRQ and removing a handler.
Oh! now I see the issue you were talking about last time around. I
expect that means the irq shouldn't be managed with devres, so it can be
the first thing freed in the remove function (I haven't checked the docs
to see if there's an easier way yet).
I'll add a patch to fix the order of the handling of the irq (both this and
the issue Ondřej brought up).
> Side note, you may make the driver neater with help of
>
> struct device *dev = >dev;
>
> defined in this patch.
Good point, it's minor, but it should be a net improvement.
> ...
>
> > static int stk3310_suspend(struct device *dev)
> > {
> > struct stk3310_data *data;
>
> > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>
> Side note: This may be updated (in a separate change) to use
> dev_get_drvdata() directly.
>
> Jonathan, do we have something like iio_priv_from_drvdata(struct
> device *dev)? Seems many drivers may utilise it.
>
At this rate I'm going to need to split off a separate style / code
cleanup series so I don't keep introducing dumb bugs while rebasing this
one.
Thank you for your time
- Aren
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 08:31:27PM GMT, Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman wrote: > > On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > > > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > > > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > > > > wrote: > > ... > > > > > > > ret = stk3310_init(indio_dev); > > > > > > if (ret < 0) > > > > > > - return ret; > > > > > > + goto err_vdd_disable; > > > > > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > > > IRQ. Note, that the original code likely has a bug which sets states > > > > > before disabling IRQ and removing a handler. > > > > > > > > How so? stk3310_init is called before enabling the interrupt. > > > > > > Exactly, IRQ is registered with devm and hence the error path and > > > remove stages will got it in a wrong order. > > > > Makes no sense. > > Huh?! > > > IRQ is not enabled here, yet. So in error path, the code will > > just disable the regulator and devm will unref it later on. IRQ doesn't > > enter > > the picture here at all in the error path. > > Error path _after_ IRQ handler has been _successfully_ installed. > And complete ->remove() stage. Allright. So fixing the other issue I mentioned will fix this one too, because there will be no error path after IRQ enable, then. kind regards, o. > > > > Original code has a bug that IRQ is enabled before registering the > > > > IIO device, > > > > > > Indeed, but this is another bug. > > > > > > > so if IRQ is triggered before registration, iio_push_event > > > > from IRQ handler may be called on a not yet registered IIO device. > > > > > > > > Never saw it happen, though. :) > > > > > > Because nobody cares enough to enable DEBUG_SHIRQ. > > > > Nice debug tool. I bet it makes quite a mess when enabled. :) > > FWIW, I have had it enabled for ages, but I have only a few devices, > so I fixed a few cases in the past WRT shared IRQ issues. > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman wrote: > On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > > > wrote: ... > > > > > ret = stk3310_init(indio_dev); > > > > > if (ret < 0) > > > > > - return ret; > > > > > + goto err_vdd_disable; > > > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > > IRQ. Note, that the original code likely has a bug which sets states > > > > before disabling IRQ and removing a handler. > > > > > > How so? stk3310_init is called before enabling the interrupt. > > > > Exactly, IRQ is registered with devm and hence the error path and > > remove stages will got it in a wrong order. > > Makes no sense. Huh?! > IRQ is not enabled here, yet. So in error path, the code will > just disable the regulator and devm will unref it later on. IRQ doesn't enter > the picture here at all in the error path. Error path _after_ IRQ handler has been _successfully_ installed. And complete ->remove() stage. > > > Original code has a bug that IRQ is enabled before registering the > > > IIO device, > > > > Indeed, but this is another bug. > > > > > so if IRQ is triggered before registration, iio_push_event > > > from IRQ handler may be called on a not yet registered IIO device. > > > > > > Never saw it happen, though. :) > > > > Because nobody cares enough to enable DEBUG_SHIRQ. > > Nice debug tool. I bet it makes quite a mess when enabled. :) FWIW, I have had it enabled for ages, but I have only a few devices, so I fixed a few cases in the past WRT shared IRQ issues. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > > wrote: > > ... > > > > > ret = stk3310_init(indio_dev); > > > > if (ret < 0) > > > > - return ret; > > > > + goto err_vdd_disable; > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > IRQ. Note, that the original code likely has a bug which sets states > > > before disabling IRQ and removing a handler. > > > > How so? stk3310_init is called before enabling the interrupt. > > Exactly, IRQ is registered with devm and hence the error path and > remove stages will got it in a wrong order. Makes no sense. IRQ is not enabled here, yet. So in error path, the code will just disable the regulator and devm will unref it later on. IRQ doesn't enter the picture here at all in the error path. > > Original code has a bug that IRQ is enabled before registering the > > IIO device, > > Indeed, but this is another bug. > > > so if IRQ is triggered before registration, iio_push_event > > from IRQ handler may be called on a not yet registered IIO device. > > > > Never saw it happen, though. :) > > Because nobody cares enough to enable DEBUG_SHIRQ. Nice debug tool. I bet it makes quite a mess when enabled. :) Kind regards, o. > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman wrote: > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan > > wrote: ... > > > ret = stk3310_init(indio_dev); > > > if (ret < 0) > > > - return ret; > > > + goto err_vdd_disable; > > > > This is wrong. You will have the regulator being disabled _before_ > > IRQ. Note, that the original code likely has a bug which sets states > > before disabling IRQ and removing a handler. > > How so? stk3310_init is called before enabling the interrupt. Exactly, IRQ is registered with devm and hence the error path and remove stages will got it in a wrong order. > Original code has a bug that IRQ is enabled before registering the > IIO device, Indeed, but this is another bug. > so if IRQ is triggered before registration, iio_push_event > from IRQ handler may be called on a not yet registered IIO device. > > Never saw it happen, though. :) Because nobody cares enough to enable DEBUG_SHIRQ. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan wrote:
> >
> > From: Ondrej Jirman
> >
> > VDD power input can be used to completely power off the chip during
> > system suspend. Do so if available.
>
> ...
>
> > ret = stk3310_init(indio_dev);
> > if (ret < 0)
> > - return ret;
> > + goto err_vdd_disable;
>
> This is wrong. You will have the regulator being disabled _before_
> IRQ. Note, that the original code likely has a bug which sets states
> before disabling IRQ and removing a handler.
How so? stk3310_init is called before enabling the interrupt.
Original code has a bug that IRQ is enabled before registering the
IIO device, so if IRQ is triggered before registration, iio_push_event
from IRQ handler may be called on a not yet registered IIO device.
Never saw it happen, though. :)
kind regards,
o.
> Side note, you may make the driver neater with help of
>
> struct device *dev = >dev;
>
> defined in this patch.
>
> ...
>
> > static int stk3310_suspend(struct device *dev)
> > {
> > struct stk3310_data *data;
>
> > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>
> Side note: This may be updated (in a separate change) to use
> dev_get_drvdata() directly.
>
> Jonathan, do we have something like iio_priv_from_drvdata(struct
> device *dev)? Seems many drivers may utilise it.
>
> > }
>
> ...
>
> > static int stk3310_resume(struct device *dev)
>
> Ditto.
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
Hi Aren,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on sunxi/sunxi/for-next robh/for-next linus/master
v6.9-rc5 next-20240423]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Aren-Moynihan/dt-bindings-iio-light-stk33xx-add-vdd-and-leda-regulators/20240424-064250
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:
https://lore.kernel.org/r/20240423223309.1468198-4-aren%40peacevolution.org
patch subject: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and
power it off during suspend
config: arm64-randconfig-001-20240424
(https://download.01.org/0day-ci/archive/20240424/202404242057.pudy5rb1-...@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project
5ef5eb66fb428aaf61fb51b709f065c069c11242)
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20240424/202404242057.pudy5rb1-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202404242057.pudy5rb1-...@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/iio/light/stk3310.c:10:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2208:
include/linux/vmstat.h:522:36: warning: arithmetic between different
enumeration types ('enum node_stat_item' and 'enum lru_list')
[-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~ ^ ~~~
>> drivers/iio/light/stk3310.c:615:38: warning: variable 'ret' is uninitialized
>> when used here [-Wuninitialized]
615 | return dev_err_probe(>dev, ret, "get
regulator vdd failed\n");
|^~~
drivers/iio/light/stk3310.c:594:9: note: initialize the variable 'ret' to
silence this warning
594 | int ret;
|^
| = 0
2 warnings generated.
vim +/ret +615 drivers/iio/light/stk3310.c
591
592 static int stk3310_probe(struct i2c_client *client)
593 {
594 int ret;
595 struct iio_dev *indio_dev;
596 struct stk3310_data *data;
597
598 indio_dev = devm_iio_device_alloc(>dev, sizeof(*data));
599 if (!indio_dev) {
600 dev_err(>dev, "iio allocation failed!\n");
601 return -ENOMEM;
602 }
603
604 data = iio_priv(indio_dev);
605 data->client = client;
606 i2c_set_clientdata(client, indio_dev);
607
608 device_property_read_u32(>dev, "proximity-near-level",
609 >ps_near_level);
610
611 mutex_init(>lock);
612
613 data->vdd_reg = devm_regulator_get(>dev, "vdd");
614 if (IS_ERR(data->vdd_reg))
> 615 return dev_err_probe(>dev, ret, "get regulator
> vdd failed\n");
616
617 ret = stk3310_regmap_init(data);
618 if (ret < 0)
619 return ret;
620
621 indio_dev->info = _info;
622 indio_dev->name = STK3310_DRIVER_NAME;
623 indio_dev->modes = INDIO_DIRECT_MODE;
624 indio_dev->channels = stk3310_channels;
625 indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
626
627 ret = regulator_enable(data->vdd_reg);
628 if (ret)
629 return dev_err_probe(>dev, ret,
630 "regulator vdd enable failed\n");
631
632 /* we need a short delay to allow the chip time to power on */
633 fsleep(1000);
634
635 ret = stk3310_init(indio_dev);
636 if (ret < 0)
637 goto err_vdd_disable;
638
639 if (client->irq > 0) {
640 ret = devm_request_threaded_irq(>dev,
client->irq,
641 stk3310_irq_handler,
Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan wrote:
>
> From: Ondrej Jirman
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
...
> ret = stk3310_init(indio_dev);
> if (ret < 0)
> - return ret;
> + goto err_vdd_disable;
This is wrong. You will have the regulator being disabled _before_
IRQ. Note, that the original code likely has a bug which sets states
before disabling IRQ and removing a handler.
Side note, you may make the driver neater with help of
struct device *dev = >dev;
defined in this patch.
...
> static int stk3310_suspend(struct device *dev)
> {
> struct stk3310_data *data;
> data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
Side note: This may be updated (in a separate change) to use
dev_get_drvdata() directly.
Jonathan, do we have something like iio_priv_from_drvdata(struct
device *dev)? Seems many drivers may utilise it.
> }
...
> static int stk3310_resume(struct device *dev)
Ditto.
--
With Best Regards,
Andy Shevchenko
[PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
From: Ondrej Jirman
VDD power input can be used to completely power off the chip during
system suspend. Do so if available.
Signed-off-by: Ondrej Jirman
Signed-off-by: Aren Moynihan
---
Notes:
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 | 49 ++---
1 file changed, 46 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 7b71ad71d78d..a0547eeca3e3 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -13,6 +13,8 @@
#include
#include
#include
+#include
+
#include
#include
#include
@@ -117,6 +119,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 +610,10 @@ static int stk3310_probe(struct i2c_client *client)
mutex_init(>lock);
+ data->vdd_reg = devm_regulator_get(>dev, "vdd");
+ if (IS_ERR(data->vdd_reg))
+ return dev_err_probe(>dev, ret, "get regulator vdd
failed\n");
+
ret = stk3310_regmap_init(data);
if (ret < 0)
return ret;
@@ -617,9 +624,17 @@ static int stk3310_probe(struct i2c_client *client)
indio_dev->channels = stk3310_channels;
indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
+ ret = regulator_enable(data->vdd_reg);
+ if (ret)
+ return dev_err_probe(>dev, ret,
+"regulator vdd enable failed\n");
+
+ /* we need a short delay to allow the chip time to power on */
+ fsleep(1000);
+
ret = stk3310_init(indio_dev);
if (ret < 0)
- return ret;
+ goto err_vdd_disable;
if (client->irq > 0) {
ret = devm_request_threaded_irq(>dev, client->irq,
@@ -645,32 +660,60 @@ static int stk3310_probe(struct i2c_client *client)
err_standby:
stk3310_set_state(data, STK3310_STATE_STANDBY);
+err_vdd_disable:
+ 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);
+ 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;
+
+ 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)));
+
+ ret = regulator_enable(data->vdd_reg);
+ if (ret) {
+ dev_err(dev, "Failed to re-enable regulator vdd\n");
+ return ret;
+ }
+
+ fsleep(1000);
+
+ 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)
--
2.44.0
