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

[PATCH v3 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 v3:
 - Use bulk regulators instead of two individual ones
 - Replace stk3310_remove with devm callbacks
 - Hopefully I haven't missed anything, it's been a while since I worked on this
   patch, and I didn't take good enough notes

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 (5):
  dt-bindings: iio: light: stk33xx: add vdd and leda regulators
  iio: light: stk3310: handle all remove logic with devm callbacks
  iio: light: stk3310: Implement vdd and leda supplies
  iio: light: stk3310: use dev_err_probe where possible
  iio: light: stk3310: log error if reading the chip id fails

Ondrej Jirman (1):
  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                   | 156 +++++++++++++-----
 3 files changed, 118 insertions(+), 44 deletions(-)

-- 
2.47.0

[PATCH v3 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.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 e4341fdced98..96ee8ec16463 100644
--- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
+++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
@@ -34,6 +34,8 @@ properties:
   interrupts:
     maxItems: 1
 
+  vdd-supply: true
+  leda-supply: true
   proximity-near-level: true
 
 required:
@@ -57,6 +59,8 @@ examples:
                 proximity-near-level = <25>;
                 interrupt-parent = <&gpio1>;
                 interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
+                vdd-supply = <&vdd_regulator>;
+                leda-supply = <&led_regulator>;
         };
     };
 ...
-- 
2.47.0

[PATCH v3 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Aren Moynihan]

Using devm callbacks helps to make the ordering of probe / remove
operations easier to reason about and removes some duplicate code
between the probe error path and driver remove.
---

Notes:
    Added in v3

 drivers/iio/light/stk3310.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index ed20b6714546..2e1aa551bdbc 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -484,6 +484,17 @@ static int stk3310_set_state(struct stk3310_data *data, u8 state)
 	return ret;
 }
 
+static void stk3310_set_state_disable(void *private)
+{
+	int ret;
+	struct stk3310_data *data = private;
+	struct device *dev = &data->client->dev;
+
+	ret = stk3310_set_state(data, STK3310_STATE_STANDBY);
+	if (ret)
+		dev_err(dev, "failed to set state to standby: %d\n", ret);
+}
+
 static int stk3310_init(struct iio_dev *indio_dev)
 {
 	int ret;
@@ -507,6 +518,11 @@ static int stk3310_init(struct iio_dev *indio_dev)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to register cleanup function\n");
+
 	/* Enable PS interrupts */
 	ret = regmap_field_write(data->reg_int_ps, STK3310_PSINT_EN);
 	if (ret < 0)
@@ -650,29 +666,17 @@ static int stk3310_probe(struct i2c_client *client)
 		if (ret < 0) {
 			dev_err(&client->dev, "request irq %d failed\n",
 				client->irq);
-			goto err_standby;
+			return ret;
 		}
 	}
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(&client->dev, indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev, "device_register failed\n");
-		goto err_standby;
+		return ret;
 	}
 
 	return 0;
-
-err_standby:
-	stk3310_set_state(data, STK3310_STATE_STANDBY);
-	return ret;
-}
-
-static void stk3310_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
-	iio_device_unregister(indio_dev);
-	stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
 }
 
 static int stk3310_suspend(struct device *dev)
@@ -736,7 +740,6 @@ static struct i2c_driver stk3310_driver = {
 		.acpi_match_table = stk3310_acpi_id,
 	},
 	.probe =        stk3310_probe,
-	.remove =           stk3310_remove,
 	.id_table =         stk3310_i2c_id,
 };
 
-- 
2.47.0

[PATCH v3 3/6] iio: light: stk3310: Implement vdd and leda supplies

[Aren Moynihan]

The vdd and leda supplies must be powered on for the chip to function
and can be powered off during system suspend.

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

Notes:
    I'm not sure what the proper way to handle attribution for this patch
    is. It was origionally based on a patch by Ondrej Jirman[1], but I have
    rewritten a large portion if it. I have included a Co-developed-by tag
    to indicate this, but haven't sent him this patch, so I'm not sure what
    to do about a Signed-off-by.
    
    1: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82
    
    Changes in v3:
     - use bulk regulators instead of two individual ones
     - handle cleanup using devm callbacks instead of the remove function
    
    Changes in v2:
     - always enable / disable regulators and rely on a dummy regulator if
       one isn't specified
     - replace usleep_range with fsleep
     - reorder includes so iio headers are last
     - add missing error handling to resume

 drivers/iio/light/stk3310.c | 76 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 2e1aa551bdbc..f02b4d20c282 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -13,6 +13,8 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
 #include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -130,6 +132,7 @@ struct stk3310_data {
 	struct regmap_field *reg_int_ps;
 	struct regmap_field *reg_flag_psint;
 	struct regmap_field *reg_flag_nf;
+	struct regulator_bulk_data supplies[2];
 };
 
 static const struct iio_event_spec stk3310_events[] = {
@@ -621,6 +624,31 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static int stk3310_regulators_enable(struct stk3310_data *data)
+{
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+	if (ret)
+		return ret;
+
+	/* we need a short delay to allow the chip time to power on */
+	fsleep(1000);
+
+	return 0;
+}
+
+static void stk3310_regulators_disable(void *private)
+{
+	int ret;
+	struct stk3310_data *data = private;
+	struct device *dev = &data->client->dev;
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+	if (ret)
+		dev_err(dev, "failed to disable regulators: %d\n", ret);
+}
+
 static int stk3310_probe(struct i2c_client *client)
 {
 	int ret;
@@ -642,6 +670,13 @@ static int stk3310_probe(struct i2c_client *client)
 
 	mutex_init(&data->lock);
 
+	data->supplies[0].supply = "vdd";
+	data->supplies[1].supply = "leda";
+	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies),
+				      data->supplies);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "get regulators failed\n");
+
 	ret = stk3310_regmap_init(data);
 	if (ret < 0)
 		return ret;
@@ -652,6 +687,16 @@ static int stk3310_probe(struct i2c_client *client)
 	indio_dev->channels = stk3310_channels;
 	indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
 
+	ret = stk3310_regulators_enable(data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "regulator enable failed\n");
+
+	ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to register regulator cleanup\n");
+
 	ret = stk3310_init(indio_dev);
 	if (ret < 0)
 		return ret;
@@ -682,18 +727,45 @@ static int stk3310_probe(struct i2c_client *client)
 static int stk3310_suspend(struct device *dev)
 {
 	struct stk3310_data *data;
+	int ret;
 
 	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
 
-	return stk3310_set_state(data, STK3310_STATE_STANDBY);
+	ret = stk3310_set_state(data, STK3310_STATE_STANDBY);
+	if (ret)
+		return ret;
+
+	regcache_mark_dirty(data->regmap);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+	if (ret) {
+		dev_err(dev, "failed to disable regulators: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
 }
 
 static int stk3310_resume(struct device *dev)
 {
-	u8 state = 0;
 	struct stk3310_data *data;
+	u8 state = 0;
+	int ret;
 
 	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
+
+	ret = stk3310_regulators_enable(data);
+	if (ret) {
+		dev_err(dev, "Failed to re-enable regulators: %d", ret);
+		return ret;
+	}
+
+	ret = regcache_sync(data->regmap);
+	if (ret) {
+		dev_err(dev, "Failed to restore registers: %d\n", ret);
+		return ret;
+	}
+
 	if (data->ps_enabled)
 		state |= STK3310_STATE_EN_PS;
 	if (data->als_enabled)
-- 
2.47.0

[PATCH v3 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 | 47 ++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index f02b4d20c282..968a2cc59d09 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -63,10 +63,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 =
@@ -516,10 +516,8 @@ static int stk3310_init(struct iio_dev *indio_dev)
 
 	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");
 
 	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);
 	if (ret)
@@ -529,9 +527,10 @@ static int stk3310_init(struct iio_dev *indio_dev)
 	/* 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;
+	return 0;
 }
 
 static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
@@ -564,10 +563,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);
@@ -656,10 +655,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;
@@ -708,18 +705,14 @@ static int stk3310_probe(struct i2c_client *client)
 						IRQF_TRIGGER_FALLING |
 						IRQF_ONESHOT,
 						STK3310_EVENT, indio_dev);
-		if (ret < 0) {
-			dev_err(&client->dev, "request irq %d failed\n",
-				client->irq);
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(&client->dev, ret, "request irq %d failed\n",
+					     client->irq);
 	}
 
 	ret = devm_iio_device_register(&client->dev, indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "device_register failed\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret, "device_register failed\n");
 
 	return 0;
 }
-- 
2.47.0

[PATCH v3 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 968a2cc59d09..d314659e7dc2 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -508,7 +508,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");
 
 	ret = stk3310_check_chip_id(chipid);
 	if (ret < 0)
-- 
2.47.0

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

[Aren Moynihan]

From: Ondrej Jirman <megi@xff.cz>

This allows the driver to properly handle powering this device, and
disable power during suspend.

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 6d152066a59a..618341b63db2 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -260,6 +260,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.47.0

Re: [PATCH v3 3/6] iio: light: stk3310: Implement vdd and leda supplies

[Andy Shevchenko]

On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote:
> The vdd and leda supplies must be powered on for the chip to function
> and can be powered off during system suspend.
> 
> Co-developed-by: Ondrej Jirman <megi@xff.cz>

Missing SoB. Please, read Submitting Patches documentation for understanding
what has to be done here.

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

...

> Notes:
>     I'm not sure what the proper way to handle attribution for this patch
>     is. It was origionally based on a patch by Ondrej Jirman[1], but I have
>     rewritten a large portion if it. I have included a Co-developed-by tag
>     to indicate this, but haven't sent him this patch, so I'm not sure what
>     to do about a Signed-off-by.

Ah, seems you already aware of this issue. So, either drop Co-developed-by
(and if you wish you may give a credit in a free form inside commit message)
or make sure you get his SoB tag.

...

>  	mutex_init(&data->lock);

Somewhere (in the previous patch?) you want to switch to devm_mutex_init().

...

> +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies),
> +				      data->supplies);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "get regulators failed\n");

> +		return dev_err_probe(&client->dev, ret,
> +				     "regulator enable failed\n");

> +	ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to register regulator cleanup\n");

With

	struct devuce *dev = &client->dev;

at the top of the function makes these and more lines neater.

...

>  static int stk3310_resume(struct device *dev)
>  {
> -	u8 state = 0;
>  	struct stk3310_data *data;
> +	u8 state = 0;
> +	int ret;

While changing to RCT order here, it seems you have inconsistent approach
elsewhere (in your own patches!). Please, be consistent with chosen style.

>  	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Oct 28, 2024 at 10:19:58AM -0400, Aren Moynihan wrote:
> 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.

...

>  		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)

Same remark, if client is not used here, supply struct device pointer directly
and make these all lines better to read.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Oct 28, 2024 at 10:19:59AM -0400, Aren Moynihan wrote:
> 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).

The commit message does not explain why dev_err_probe() has been chosen
and not simple dev_err().

-- 
With Best Regards,
Andy Shevchenko

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

[Aren]

On Mon, Oct 28, 2024 at 04:45:35PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 28, 2024 at 10:19:59AM -0400, Aren Moynihan wrote:
> > 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).
> 
> The commit message does not explain why dev_err_probe() has been chosen
> and not simple dev_err().

This function is only called from stk3310_probe, and this condition
should propagate it's error, so it fits what dev_err_probe is designed
for. dev_err would be pretty much equivalent just longer, like this:

if (ret < 0) {
	dev_err(&client->dev, "failed to read chip it: %d\n", ret);
	return ret;
}

Regards
 - Aren

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

[Andy Shevchenko]

On Mon, Oct 28, 2024 at 11:29:35AM -0400, Aren wrote:
> On Mon, Oct 28, 2024 at 04:45:35PM +0200, Andy Shevchenko wrote:
> > On Mon, Oct 28, 2024 at 10:19:59AM -0400, Aren Moynihan wrote:
> > > 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).
> > 
> > The commit message does not explain why dev_err_probe() has been chosen
> > and not simple dev_err().
> 
> This function is only called from stk3310_probe, and this condition
> should propagate it's error, so it fits what dev_err_probe is designed
> for. dev_err would be pretty much equivalent just longer, like this:
> 
> if (ret < 0) {
> 	dev_err(&client->dev, "failed to read chip it: %d\n", ret);
> 	return ret;
> }

It's fine, the problem is the commit message in the patch. Please,
update the commit message accordingly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/6] iio: light: stk3310: Implement vdd and leda supplies

[Aren Moynihan]

On Mon, Oct 28, 2024 at 04:38:37PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote:
> > The vdd and leda supplies must be powered on for the chip to function
> > and can be powered off during system suspend.
> > 
> > Co-developed-by: Ondrej Jirman <megi@xff.cz>
> 
> Missing SoB. Please, read Submitting Patches documentation for understanding
> what has to be done here.
> 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> 
> ...
> 
> > Notes:
> >     I'm not sure what the proper way to handle attribution for this patch
> >     is. It was origionally based on a patch by Ondrej Jirman[1], but I have
> >     rewritten a large portion if it. I have included a Co-developed-by tag
> >     to indicate this, but haven't sent him this patch, so I'm not sure what
> >     to do about a Signed-off-by.
> 
> Ah, seems you already aware of this issue. So, either drop Co-developed-by
> (and if you wish you may give a credit in a free form inside commit message)
> or make sure you get his SoB tag.

Alright, thanks for clarifying that.

> >  	mutex_init(&data->lock);
> 
> Somewhere (in the previous patch?) you want to switch to devm_mutex_init().

Good catch, it looks like that was being leaked before this refactor.
Yeah that sounds like the right place, I'll include it in v4.

> > +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies),
> > +				      data->supplies);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "get regulators failed\n");
> 
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "regulator enable failed\n");
> 
> > +	ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "failed to register regulator cleanup\n");
> 
> With
> 
> 	struct devuce *dev = &client->dev;
> 
> at the top of the function makes these and more lines neater.
> 
[snip]
> 
> While changing to RCT order here, it seems you have inconsistent approach
> elsewhere (in your own patches!). Please, be consistent with chosen style.

Sounds easy enough to fix, I'll include these in v4.

Thanks taking the time to review
 - Aren

Re: [PATCH v3 3/6] iio: light: stk3310: Implement vdd and leda supplies

[Jonathan Cameron]

On Mon, 28 Oct 2024 12:37:14 -0400
Aren Moynihan <aren@peacevolution.org> wrote:

> On Mon, Oct 28, 2024 at 04:38:37PM +0200, Andy Shevchenko wrote:
> > On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote:  
> > > The vdd and leda supplies must be powered on for the chip to function
> > > and can be powered off during system suspend.
> > > 
> > > Co-developed-by: Ondrej Jirman <megi@xff.cz>  
> > 
> > Missing SoB. Please, read Submitting Patches documentation for understanding
> > what has to be done here.
> >   
> > > Signed-off-by: Aren Moynihan <aren@peacevolution.org>  
> > 
> > ...
> >   
> > > Notes:
> > >     I'm not sure what the proper way to handle attribution for this patch
> > >     is. It was origionally based on a patch by Ondrej Jirman[1], but I have
> > >     rewritten a large portion if it. I have included a Co-developed-by tag
> > >     to indicate this, but haven't sent him this patch, so I'm not sure what
> > >     to do about a Signed-off-by.  
> > 
> > Ah, seems you already aware of this issue. So, either drop Co-developed-by
> > (and if you wish you may give a credit in a free form inside commit message)
> > or make sure you get his SoB tag.  
> 
> Alright, thanks for clarifying that.
> 
> > >  	mutex_init(&data->lock);  
> > 
> > Somewhere (in the previous patch?) you want to switch to devm_mutex_init().  
> 
> Good catch, it looks like that was being leaked before this refactor.
> Yeah that sounds like the right place, I'll include it in v4.
Not really on the leaking.  Take a look at the cleanup for devm_mutex_init().
It's debug only and not all that useful in most cases.

However, it is good to not assume that now we have a devm_mutex_init()
available that is easy to use.

> 
> > > +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies),
> > > +				      data->supplies);
> > > +	if (ret)
> > > +		return dev_err_probe(&client->dev, ret, "get regulators failed\n");  
> >   
> > > +		return dev_err_probe(&client->dev, ret,
> > > +				     "regulator enable failed\n");  
> >   
> > > +	ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data);
> > > +	if (ret)
> > > +		return dev_err_probe(&client->dev, ret,
> > > +				     "failed to register regulator cleanup\n");  
> > 
> > With
> > 
> > 	struct devuce *dev = &client->dev;
> > 
> > at the top of the function makes these and more lines neater.
> >   
> [snip]
> > 
> > While changing to RCT order here, it seems you have inconsistent approach
> > elsewhere (in your own patches!). Please, be consistent with chosen style.  
> 
> Sounds easy enough to fix, I'll include these in v4.
> 
> Thanks taking the time to review
>  - Aren

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

[Jonathan Cameron]

On Mon, 28 Oct 2024 10:19:54 -0400
Aren Moynihan <aren@peacevolution.org> wrote:

> 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.
> 
I took a quick look and nothing to add to Andy's excellent review.

J
> Major changes in v3:
>  - Use bulk regulators instead of two individual ones
>  - Replace stk3310_remove with devm callbacks
>  - Hopefully I haven't missed anything, it's been a while since I worked on this
>    patch, and I didn't take good enough notes
> 
> 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 (5):
>   dt-bindings: iio: light: stk33xx: add vdd and leda regulators
>   iio: light: stk3310: handle all remove logic with devm callbacks
>   iio: light: stk3310: Implement vdd and leda supplies
>   iio: light: stk3310: use dev_err_probe where possible
>   iio: light: stk3310: log error if reading the chip id fails
> 
> Ondrej Jirman (1):
>   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                   | 156 +++++++++++++-----
>  3 files changed, 118 insertions(+), 44 deletions(-)
> 

Re: [PATCH v3 3/6] iio: light: stk3310: Implement vdd and leda supplies

[Dragan Simic]

Hello Aren,

On 2024-10-28 15:38, Andy Shevchenko wrote:
> On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote:
>> The vdd and leda supplies must be powered on for the chip to function
>> and can be powered off during system suspend.
>> 
>> Co-developed-by: Ondrej Jirman <megi@xff.cz>
> 
> Missing SoB. Please, read Submitting Patches documentation for 
> understanding
> what has to be done here.
> 
>> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> 
> ...
> 
>> Notes:
>>     I'm not sure what the proper way to handle attribution for this 
>> patch
>>     is. It was origionally based on a patch by Ondrej Jirman[1], but I 
>> have
>>     rewritten a large portion if it. I have included a Co-developed-by 
>> tag
>>     to indicate this, but haven't sent him this patch, so I'm not sure 
>> what
>>     to do about a Signed-off-by.
> 
> Ah, seems you already aware of this issue. So, either drop 
> Co-developed-by
> (and if you wish you may give a credit in a free form inside commit 
> message)
> or make sure you get his SoB tag.

Perhaps the best and also easiest solution would be to provide an
Originally-by tag for Ondrej, because that's what it is.  The patch
was written originally by Ondrej, but you've changed many parts of
the patch while upstreaming it.