In the Pine64 PinePhone, the stk3310 chip is powered by a regulator that is
disabled at system boot and can be shut off during suspend. To ensure that
the chip properly initializes, both after boot and suspend, we need to
manage this regulator.
Additionally if the chip is shut off in suspend, we need to make sure that
it gets reinitialized with the same parameters after resume.
Major changes in v2:
- Add handling of the IR LED. I was hesitant to include this as it is the
same as pull-up regulator for the i2c bus on the hardware I have, so I
can't test it well. I think leaving it out is more likely to cause
issues than including it.
- Convert stk3310 to use dev_err_probe for errors.
- Always enable / disable regulators and rely on dummy devices if they're
not specified.
- more listed in individual patches
Aren Moynihan (4):
dt-bindings: iio: light: stk33xx: add vdd and leda regulators
iio: light: stk3310: Manage LED power supply
iio: light: stk3310: use dev_err_probe where possible
iio: light: stk3310: log error if reading the chip id fails
Ondrej Jirman (2):
iio: light: stk3310: Implement vdd supply and power it off during
suspend
arm64: dts: allwinner: pinephone: Add power supplies to stk3311
.../bindings/iio/light/stk33xx.yaml | 4 +
.../dts/allwinner/sun50i-a64-pinephone.dtsi | 2 +
drivers/iio/light/stk3310.c | 116 +++++++++++++-----
3 files changed, 94 insertions(+), 28 deletions(-)
--
2.44.0
stk3310 and stk3311 are typically connected to power supplies for the
chip (vdd) and the infrared LED (leda). Add properties so we can power
these up / down appropriately.
Signed-off-by: Aren Moynihan <[email protected]>
---
Notes:
Changes in v2:
- add leda-supply
- add supplies to examples
Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
index f6e22dc9814a..43ead524cecb 100644
--- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
+++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
@@ -29,6 +29,8 @@ properties:
interrupts:
maxItems: 1
+ vdd-supply: true
+ leda-supply: true
proximity-near-level: true
required:
@@ -52,6 +54,8 @@ examples:
proximity-near-level = <25>;
interrupt-parent = <&gpio1>;
interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
+ vdd-supply = <&vdd_regulator>;
+ leda-supply = <&led_regulator>;
};
};
...
--
2.44.0
If the chip isn't powered, this call is likely to return an error.
Without a log here the driver will silently fail to probe. Common errors
are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
isn't powered).
Signed-off-by: Aren Moynihan <[email protected]>
---
Notes:
Changes in v2:
- use dev_err_probe
drivers/iio/light/stk3310.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index c56c6298d292..6ee6f145a6d5 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -475,7 +475,7 @@ static int stk3310_init(struct iio_dev *indio_dev)
ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
if (ret < 0)
- return ret;
+ return dev_err_probe(&client->dev, ret, "failed to read chip id\n");
if (chipid != STK3310_CHIP_ID_VAL &&
chipid != STK3311_CHIP_ID_VAL &&
--
2.44.0
The stk3310 and stk3310 chips have an input for power to the infrared
LED. Add support for managing it's state.
Signed-off-by: Aren Moynihan <[email protected]>
---
drivers/iio/light/stk3310.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index a0547eeca3e3..ee1ac95dbc0e 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -120,6 +120,7 @@ struct stk3310_data {
struct regmap_field *reg_flag_psint;
struct regmap_field *reg_flag_nf;
struct regulator *vdd_reg;
+ struct regulator *led_reg;
};
static const struct iio_event_spec stk3310_events[] = {
@@ -614,6 +615,10 @@ static int stk3310_probe(struct i2c_client *client)
if (IS_ERR(data->vdd_reg))
return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n");
+ data->led_reg = devm_regulator_get(&client->dev, "leda");
+ if (IS_ERR(data->led_reg))
+ return dev_err_probe(&client->dev, ret, "get regulator led failed\n");
+
ret = stk3310_regmap_init(data);
if (ret < 0)
return ret;
@@ -629,12 +634,18 @@ static int stk3310_probe(struct i2c_client *client)
return dev_err_probe(&client->dev, ret,
"regulator vdd enable failed\n");
+ ret = regulator_enable(data->led_reg);
+ if (ret) {
+ dev_err_probe(&client->dev, ret, "regulator led enable failed\n");
+ goto err_vdd_disable;
+ }
+
/* we need a short delay to allow the chip time to power on */
fsleep(1000);
ret = stk3310_init(indio_dev);
if (ret < 0)
- goto err_vdd_disable;
+ goto err_led_disable;
if (client->irq > 0) {
ret = devm_request_threaded_irq(&client->dev, client->irq,
@@ -660,6 +671,8 @@ static int stk3310_probe(struct i2c_client *client)
err_standby:
stk3310_set_state(data, STK3310_STATE_STANDBY);
+err_led_disable:
+ regulator_disable(data->led_reg);
err_vdd_disable:
regulator_disable(data->vdd_reg);
return ret;
@@ -672,6 +685,7 @@ static void stk3310_remove(struct i2c_client *client)
iio_device_unregister(indio_dev);
stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
+ regulator_disable(data->led_reg);
regulator_disable(data->vdd_reg);
}
@@ -687,6 +701,7 @@ static int stk3310_suspend(struct device *dev)
return ret;
regcache_mark_dirty(data->regmap);
+ regulator_disable(data->led_reg);
regulator_disable(data->vdd_reg);
return 0;
@@ -706,6 +721,12 @@ static int stk3310_resume(struct device *dev)
return ret;
}
+ ret = regulator_enable(data->led_reg);
+ if (ret) {
+ dev_err(dev, "Failed to re-enable regulator led\n");
+ return ret;
+ }
+
fsleep(1000);
ret = regcache_sync(data->regmap);
--
2.44.0
From: Ondrej Jirman <[email protected]>
VDD power input can be used to completely power off the chip during
system suspend. Do so if available.
Signed-off-by: Ondrej Jirman <[email protected]>
Signed-off-by: Aren Moynihan <[email protected]>
---
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 <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>
@@ -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(&data->lock);
+ data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
+ if (IS_ERR(data->vdd_reg))
+ return dev_err_probe(&client->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(&client->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(&client->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
From: Ondrej Jirman <[email protected]>
This makes the driver disable the supply during sleep.
Signed-off-by: Ondrej Jirman <[email protected]>
Signed-off-by: Aren Moynihan <[email protected]>
---
Notes:
Changes in v2:
- add leda-supply
arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
index b5a232209f2b..51ab1db95f81 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -250,6 +250,8 @@ light-sensor@48 {
reg = <0x48>;
interrupt-parent = <&pio>;
interrupts = <1 0 IRQ_TYPE_EDGE_FALLING>; /* PB0 */
+ vdd-supply = <®_ldo_io0>;
+ leda-supply = <®_dldo1>;
};
/* Accelerometer/gyroscope */
--
2.44.0
On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <[email protected]> wrote:
>
> From: Ondrej Jirman <[email protected]>
>
> 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 = &client->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
On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <[email protected]> wrote:
>
> The stk3310 and stk3310 chips have an input for power to the infrared
> LED. Add support for managing it's state.
its
..
> if (IS_ERR(data->vdd_reg))
> return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n");
>
> + data->led_reg = devm_regulator_get(&client->dev, "leda");
> + if (IS_ERR(data->led_reg))
> + return dev_err_probe(&client->dev, ret, "get regulator led failed\n");
Can't you use a bulk regulator API instead?
--
With Best Regards,
Andy Shevchenko
On 24/04/2024 00:33, Aren Moynihan wrote:
> stk3310 and stk3311 are typically connected to power supplies for the
> chip (vdd) and the infrared LED (leda). Add properties so we can power
> these up / down appropriately.
>
> Signed-off-by: Aren Moynihan <[email protected]>
> ---
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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(&client->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(&client->dev, sizeof(*data));
599 if (!indio_dev) {
600 dev_err(&client->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(&client->dev, "proximity-near-level",
609 &data->ps_near_level);
610
611 mutex_init(&data->lock);
612
613 data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
614 if (IS_ERR(data->vdd_reg))
> 615 return dev_err_probe(&client->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 = &stk3310_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(&client->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(&client->dev, client->irq,
641 stk3310_irq_handler,
642 stk3310_irq_event_handler,
643 IRQF_TRIGGER_FALLING |
644 IRQF_ONESHOT,
645 STK3310_EVENT, indio_dev);
646 if (ret < 0) {
647 dev_err(&client->dev, "request irq %d failed\n",
648 client->irq);
649 goto err_standby;
650 }
651 }
652
653 ret = iio_device_register(indio_dev);
654 if (ret < 0) {
655 dev_err(&client->dev, "device_register failed\n");
656 goto err_standby;
657 }
658
659 return 0;
660
661 err_standby:
662 stk3310_set_state(data, STK3310_STATE_STANDBY);
663 err_vdd_disable:
664 regulator_disable(data->vdd_reg);
665 return ret;
666 }
667
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <[email protected]> wrote:
> >
> > From: Ondrej Jirman <[email protected]>
> >
> > 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 = &client->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
On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <[email protected]> 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 <[email protected]> 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
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 <[email protected]> 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 <[email protected]> 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
On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
On Wed, Apr 24, 2024 at 02:16:06AM +0300, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <[email protected]> wrote:
> >
> > From: Ondrej Jirman <[email protected]>
> >
> > 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 = &client->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
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/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/
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(&client->dev, sizeof(*data));
be9e6229d67696 Tiberiu Breana 2015-04-27 599 if (!indio_dev) {
be9e6229d67696 Tiberiu Breana 2015-04-27 600 dev_err(&client->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(&client->dev, "proximity-near-level",
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 609 &data->ps_near_level);
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 610
be9e6229d67696 Tiberiu Breana 2015-04-27 611 mutex_init(&data->lock);
be9e6229d67696 Tiberiu Breana 2015-04-27 612
dd231c1d219f6b Ondrej Jirman 2024-04-23 613 data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
dd231c1d219f6b Ondrej Jirman 2024-04-23 614 if (IS_ERR(data->vdd_reg))
dd231c1d219f6b Ondrej Jirman 2024-04-23 @615 return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n");
s/ret/PTR_ERR(data->vdd_reg)/
dd231c1d219f6b Ondrej Jirman 2024-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 = &stk3310_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 Jirman 2024-04-23 627 ret = regulator_enable(data->vdd_reg);
dd231c1d219f6b Ondrej Jirman 2024-04-23 628 if (ret)
dd231c1d219f6b Ondrej Jirman 2024-04-23 629 return dev_err_probe(&client->dev, ret,
dd231c1d219f6b Ondrej Jirman 2024-04-23 630 "regulator vdd enable failed\n");
dd231c1d219f6b Ondrej Jirman 2024-04-23 631
dd231c1d219f6b Ondrej Jirman 2024-04-23 632 /* we need a short delay to allow the chip time to power on */
dd231c1d219f6b Ondrej Jirman 2024-04-23 633 fsleep(1000);
dd231c1d219f6b Ondrej Jirman 2024-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 Jirman 2024-04-23 637 goto err_vdd_disable;
be9e6229d67696 Tiberiu Breana 2015-04-27 638
6839c1b0700a79 Octavian Purdila 2015-09-23 639 if (client->irq > 0) {
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 640 ret = devm_request_threaded_irq(&client->dev, client->irq,
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 641 stk3310_irq_handler,
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 642 stk3310_irq_event_handler,
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 643 IRQF_TRIGGER_FALLING |
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 644 IRQF_ONESHOT,
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 645 STK3310_EVENT, indio_dev);
7c7a9eeaa335df Hartmut Knaack 2015-07-09 646 if (ret < 0) {
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 647 dev_err(&client->dev, "request irq %d failed\n",
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 648 client->irq);
7c7a9eeaa335df Hartmut Knaack 2015-07-09 649 goto err_standby;
7c7a9eeaa335df Hartmut Knaack 2015-07-09 650 }
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 651 }
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 652
037e966f2d6389 Hartmut Knaack 2015-07-09 653 ret = iio_device_register(indio_dev);
037e966f2d6389 Hartmut Knaack 2015-07-09 654 if (ret < 0) {
037e966f2d6389 Hartmut Knaack 2015-07-09 655 dev_err(&client->dev, "device_register failed\n");
7c7a9eeaa335df Hartmut Knaack 2015-07-09 656 goto err_standby;
037e966f2d6389 Hartmut Knaack 2015-07-09 657 }
037e966f2d6389 Hartmut Knaack 2015-07-09 658
7c7a9eeaa335df Hartmut Knaack 2015-07-09 659 return 0;
7c7a9eeaa335df Hartmut Knaack 2015-07-09 660
7c7a9eeaa335df Hartmut Knaack 2015-07-09 661 err_standby:
7c7a9eeaa335df Hartmut Knaack 2015-07-09 662 stk3310_set_state(data, STK3310_STATE_STANDBY);
dd231c1d219f6b Ondrej Jirman 2024-04-23 663 err_vdd_disable:
dd231c1d219f6b Ondrej Jirman 2024-04-23 664 regulator_disable(data->vdd_reg);
be9e6229d67696 Tiberiu Breana 2015-04-27 665 return ret;
be9e6229d67696 Tiberiu Breana 2015-04-27 666 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, 24 Apr 2024 02:16:06 +0300
Andy Shevchenko <[email protected]> wrote:
> On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolutionorg> wrote:
> >
> > From: Ondrej Jirman <[email protected]>
> >
> > 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 = &client->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
On Wed, 24 Apr 2024 18:20:41 +0300
Andy Shevchenko <[email protected]> wrote:
> On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <[email protected]> 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 <[email protected]> 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
>
On Wed, 24 Apr 2024 22:00:46 +0200
Ondřej Jirman <[email protected]> wrote:
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.
Don't do reorder stuff past iio_device_register.
It generates a bunch of it's own issues wrt to functionality.
The iio_device_register() call is the last one because the device must be in a
state to correctly deal with all userspace actions by the time they are made
available.
Harden the driver to not call IIO core functions for false events if that
is easy to do, but there shouldn't be an issue if you do (if there is we should
address that in the IIO core).
Jonathan
>
> 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
On Tue, 23 Apr 2024 18:33:05 -0400
Aren Moynihan <[email protected]> wrote:
> From: Ondrej Jirman <[email protected]>
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
>
> Signed-off-by: Ondrej Jirman <[email protected]>
> Signed-off-by: Aren Moynihan <[email protected]>
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 <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>
> @@ -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(&data->lock);
>
> + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> + if (IS_ERR(data->vdd_reg))
> + return dev_err_probe(&client->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(&client->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(&client->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)