[PATCH v2 0/6] iio: light: stk3310: support powering off during suspend

[PATCH v2 0/6] iio: light: stk3310: support powering off during suspend

[Aren Moynihan]

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

[PATCH v2 1/6] dt-bindings: iio: light: stk33xx: add vdd and leda regulators

[Aren Moynihan]

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 <aren@peacevolution.org>
---

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

[PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

[Aren Moynihan]

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.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---

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

[PATCH v2 3/6] iio: light: stk3310: Manage LED power supply

[Aren Moynihan]

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 <aren@peacevolution.org>
---
 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

[PATCH v2 4/6] iio: light: stk3310: use dev_err_probe where possible

[Aren Moynihan]

Using dev_err_probe instead of dev_err and return makes the errors
easier to understand by including the error name, and saves a little
code.

Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---

Notes:
    Added in v2

 drivers/iio/light/stk3310.c | 44 +++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index ee1ac95dbc0e..c56c6298d292 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -60,10 +60,10 @@
 		data->reg_##name =					    \
 			devm_regmap_field_alloc(&client->dev, regmap,	    \
 				stk3310_reg_field_##name);		    \
-		if (IS_ERR(data->reg_##name)) {				    \
-			dev_err(&client->dev, "reg field alloc failed.\n"); \
-			return PTR_ERR(data->reg_##name);		    \
-		}							    \
+		if (IS_ERR(data->reg_##name))				    \
+			return dev_err_probe(&client->dev,		    \
+				PTR_ERR(data->reg_##name),		    \
+				"reg field alloc failed.\n");		    \
 	} while (0)
 
 static const struct reg_field stk3310_reg_field_state =
@@ -480,22 +480,20 @@ static int stk3310_init(struct iio_dev *indio_dev)
 	if (chipid != STK3310_CHIP_ID_VAL &&
 	    chipid != STK3311_CHIP_ID_VAL &&
 	    chipid != STK3311X_CHIP_ID_VAL &&
-	    chipid != STK3335_CHIP_ID_VAL) {
-		dev_err(&client->dev, "invalid chip id: 0x%x\n", chipid);
-		return -ENODEV;
-	}
+	    chipid != STK3335_CHIP_ID_VAL)
+		return dev_err_probe(&client->dev, -ENODEV,
+				     "invalid chip id: 0x%x\n", chipid);
 
 	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
 	ret = stk3310_set_state(data, state);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed to enable sensor");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret, "failed to enable sensor\n");
 
 	/* Enable PS interrupts */
 	ret = regmap_field_write(data->reg_int_ps, STK3310_PSINT_EN);
 	if (ret < 0)
-		dev_err(&client->dev, "failed to enable interrupts!\n");
+		return dev_err_probe(&client->dev, ret,
+				     "failed to enable interrupts!\n");
 
 	return ret;
 }
@@ -530,10 +528,10 @@ static int stk3310_regmap_init(struct stk3310_data *data)
 
 	client = data->client;
 	regmap = devm_regmap_init_i2c(client, &stk3310_regmap_config);
-	if (IS_ERR(regmap)) {
-		dev_err(&client->dev, "regmap initialization failed.\n");
-		return PTR_ERR(regmap);
-	}
+	if (IS_ERR(regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(regmap),
+				     "regmap initialization failed.\n");
+
 	data->regmap = regmap;
 
 	STK3310_REGFIELD(state);
@@ -597,10 +595,8 @@ static int stk3310_probe(struct i2c_client *client)
 	struct stk3310_data *data;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
-	if (!indio_dev) {
-		dev_err(&client->dev, "iio allocation failed!\n");
-		return -ENOMEM;
-	}
+	if (!indio_dev)
+		return dev_err_probe(&client->dev, -ENOMEM, "iio allocation failed!\n");
 
 	data = iio_priv(indio_dev);
 	data->client = client;
@@ -655,15 +651,15 @@ static int stk3310_probe(struct i2c_client *client)
 						IRQF_ONESHOT,
 						STK3310_EVENT, indio_dev);
 		if (ret < 0) {
-			dev_err(&client->dev, "request irq %d failed\n",
-				client->irq);
+			dev_err_probe(&client->dev, ret, "request irq %d failed\n",
+				      client->irq);
 			goto err_standby;
 		}
 	}
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
-		dev_err(&client->dev, "device_register failed\n");
+		dev_err_probe(&client->dev, ret, "device_register failed\n");
 		goto err_standby;
 	}
 
-- 
2.44.0

[PATCH v2 5/6] iio: light: stk3310: log error if reading the chip id fails

[Aren Moynihan]

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 <aren@peacevolution.org>
---

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

[PATCH v2 6/6] arm64: dts: allwinner: pinephone: Add power supplies to stk3311

[Aren Moynihan]

From: Ondrej Jirman <megi@xff.cz>

This makes the driver disable the supply during sleep.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---

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 = <&reg_ldo_io0>;
+		leda-supply = <&reg_dldo1>;
 	};
 
 	/* Accelerometer/gyroscope */
-- 
2.44.0

Re: [PATCH v2 3/6] iio: light: stk3310: Manage LED power supply

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> 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

Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 1:41 AM 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.

...

>         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

Re: [PATCH v2 1/6] dt-bindings: iio: light: stk33xx: add vdd and leda regulators

[Krzysztof Kozlowski]

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 <aren@peacevolution.org>
> ---
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

[kernel test robot]

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-lkp@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-lkp@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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404242057.PUDY5RB1-lkp@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(&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

Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

[Ondřej Jirman]

On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 1:41 AM 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.
> 
> ...
> 
> >         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

Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <megi@xff.cz> 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 <aren@peacevolution.org> 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

[Ondřej Jirman]

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 <megi@xff.cz> 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 <aren@peacevolution.org> 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

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman <megi@xff.cz> 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 <megi@xff.cz> 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 <aren@peacevolution.org> 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

[Ondřej Jirman]

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 <megi@xff.cz> 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 <megi@xff.cz> 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 <aren@peacevolution.org> 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

[Aren]

On Wed, Apr 24, 2024 at 02:16:06AM +0300, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 1:41 AM 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.
> 
> ...
> 
> >         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

Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

[Dan Carpenter]

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-lkp@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 <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202404251021.4OPER3OS-lkp@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(&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

Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

[Jonathan Cameron]

On Wed, 24 Apr 2024 02:16:06 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Apr 24, 2024 at 1:41 AM 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.  
> 
> ...
> 
> >         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

Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend

[Jonathan Cameron]

On Wed, 24 Apr 2024 18:20:41 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <megi@xff.cz> 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 <aren@peacevolution.org> 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

[Jonathan Cameron]

On Tue, 23 Apr 2024 18:33:05 -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.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>

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)