[PATCH v5 0/8] iio: light: stk3310: support powering off during suspend

[PATCH v5 0/8] 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.

Changes in v5:
 - inline STK3310_REGFIELD macro
 - extract a separate dev variable instead of using &client->dev or
   &data->client->dev
 - make log / error message format consistent (no period and a newline
   at the end)
 - add error handling for devm_mutex_init

Major changes in v4:
 - replace mutex_init with devm_mutex_init
 - code style cleanup

Major changes in v3:
 - Use bulk regulators instead of two individual ones
 - Replace stk3310_remove with devm callbacks

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 (7):
  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: simplify and inline STK3310_REGFIELD macro
  iio: light: stk3310: refactor to always make dev a variable
  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                   | 254 ++++++++++++------
 3 files changed, 178 insertions(+), 82 deletions(-)

-- 
2.48.1

[PATCH v5 1/8] 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 e4341fdced98c..96ee8ec16463d 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.48.1

[PATCH v5 2/8] 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.

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

Notes:
    Changes in v4:
     - also replace mutex_init with devm_mutex_init
    
    Added in v3

 drivers/iio/light/stk3310.c | 40 +++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index b81cc44db43c3..73ea36fa3acec 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -481,6 +481,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;
@@ -488,6 +499,7 @@ static int stk3310_init(struct iio_dev *indio_dev)
 	u8 state;
 	struct stk3310_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
 
 	ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
 	if (ret < 0)
@@ -504,6 +516,10 @@ static int stk3310_init(struct iio_dev *indio_dev)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, stk3310_set_state_disable, data);
+	if (ret)
+		return dev_err_probe(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)
@@ -607,6 +623,7 @@ static int stk3310_probe(struct i2c_client *client)
 	int ret;
 	struct iio_dev *indio_dev;
 	struct stk3310_data *data;
+	struct device *dev = &client->dev;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev) {
@@ -621,7 +638,9 @@ static int stk3310_probe(struct i2c_client *client)
 	device_property_read_u32(&client->dev, "proximity-near-level",
 				 &data->ps_near_level);
 
-	mutex_init(&data->lock);
+	ret = devm_mutex_init(dev, &data->lock);
+	if (ret)
+		return ret;
 
 	ret = stk3310_regmap_init(data);
 	if (ret < 0)
@@ -647,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(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)
@@ -733,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.48.1

[PATCH v5 3/8] 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.

This is originally based on a patch by Ondrej Jirman, but has been
rewritten since.

Link: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---

Notes:
    Changes in v5:
     - explicitely include array_size.h
    
    Changes in v4:
     - fix variable declaration order in stk3310_resume to match the rest of
       the driver
    
    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 73ea36fa3acec..9c38ff01fdf0f 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -7,12 +7,15 @@
  * IIO driver for STK3310/STK3311. 7-bit I2C address: 0x48.
  */
 
+#include <linux/array_size.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #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 +133,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[] = {
@@ -618,6 +622,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 +671,13 @@ static int stk3310_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	data->supplies[0].supply = "vdd";
+	data->supplies[1].supply = "leda";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
+				      data->supplies);
+	if (ret)
+		return dev_err_probe(dev, ret, "get regulators failed\n");
+
 	ret = stk3310_regmap_init(data);
 	if (ret < 0)
 		return ret;
@@ -652,6 +688,15 @@ 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(dev, ret, "regulator enable failed\n");
+
+	ret = devm_add_action_or_reset(dev, stk3310_regulators_disable, data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to register regulator cleanup\n");
+
 	ret = stk3310_init(indio_dev);
 	if (ret < 0)
 		return ret;
@@ -681,19 +726,46 @@ static int stk3310_probe(struct i2c_client *client)
 
 static int stk3310_suspend(struct device *dev)
 {
+	int ret;
 	struct stk3310_data *data;
 
 	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;
+	int ret;
 	struct stk3310_data *data;
+	u8 state = 0;
 
 	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\n", 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.48.1

[PATCH v5 4/8] iio: light: stk3310: simplify and inline STK3310_REGFIELD macro

[Aren Moynihan]

This macro has a conditional return statement, which obfuscates control
flow. Inlining makes the control flow more clear.

This also switches to using dev_err_probe for error reporting, which is
a shorter way of expressing the same logic as before.

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

Notes:
    Added in v5

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

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 9c38ff01fdf0f..2e883e24dc4b2 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -59,17 +59,6 @@
 	"0.023680 0.047360 0.094720 0.189440 0.378880 0.757760 1.515520 " \
 	"3.031040 6.062080"
 
-#define STK3310_REGFIELD(name)						    \
-	do {								    \
-		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);		    \
-		}							    \
-	} while (0)
-
 static const struct reg_field stk3310_reg_field_state =
 				REG_FIELD(STK3310_REG_STATE, 0, 2);
 static const struct reg_field stk3310_reg_field_als_gain =
@@ -568,14 +557,45 @@ static int stk3310_regmap_init(struct stk3310_data *data)
 	}
 	data->regmap = regmap;
 
-	STK3310_REGFIELD(state);
-	STK3310_REGFIELD(als_gain);
-	STK3310_REGFIELD(ps_gain);
-	STK3310_REGFIELD(als_it);
-	STK3310_REGFIELD(ps_it);
-	STK3310_REGFIELD(int_ps);
-	STK3310_REGFIELD(flag_psint);
-	STK3310_REGFIELD(flag_nf);
+	data->reg_state = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_state);
+	if (IS_ERR(data->reg_state))
+		return dev_err_probe(dev, PTR_ERR(data->reg_state),
+				     "reg_state alloc failed\n");
+
+	data->reg_als_gain = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_als_gain);
+	if (IS_ERR(data->reg_als_gain))
+		return dev_err_probe(dev, PTR_ERR(data->reg_als_gain),
+				     "reg_als_gain alloc failed\n");
+
+	data->reg_ps_gain = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_ps_gain);
+	if (IS_ERR(data->reg_ps_gain))
+		return dev_err_probe(dev, PTR_ERR(data->reg_ps_gain),
+				     "reg_ps_gain alloc failed\n");
+
+	data->reg_als_it = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_als_it);
+	if (IS_ERR(data->reg_als_it))
+		return dev_err_probe(dev, PTR_ERR(data->reg_als_it),
+				     "reg_als_it alloc failed\n");
+
+	data->reg_ps_it = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_ps_it);
+	if (IS_ERR(data->reg_ps_it))
+		return dev_err_probe(dev, PTR_ERR(data->reg_ps_it),
+				     "reg_ps_it alloc failed\n");
+
+	data->reg_int_ps = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_int_ps);
+	if (IS_ERR(data->reg_int_ps))
+		return dev_err_probe(dev, PTR_ERR(data->reg_int_ps),
+				     "reg_int_ps alloc failed\n");
+
+	data->reg_flag_psint = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_flag_psint);
+	if (IS_ERR(data->reg_flag_psint))
+		return dev_err_probe(dev, PTR_ERR(data->reg_flag_psint),
+				     "reg_flag_psint alloc failed\n");
+
+	data->reg_flag_nf = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_flag_nf);
+	if (IS_ERR(data->reg_flag_nf))
+		return dev_err_probe(dev, PTR_ERR(data->reg_flag_nf),
+				     "reg_flag_nf alloc failed\n");
 
 	return 0;
 }
-- 
2.48.1

[PATCH v5 5/8] iio: light: stk3310: refactor to always make dev a variable

[Aren Moynihan]

This reduces syntactic noise where the dev variable is used, which
should help improve readability.

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

Notes:
    Added in v5

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

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 2e883e24dc4b2..2233eab63b7aa 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -237,6 +237,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
 	__be16 buf;
 	int ret;
 	struct stk3310_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
 
 	if (info != IIO_EV_INFO_VALUE)
 		return -EINVAL;
@@ -253,7 +254,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
 	ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
 	mutex_unlock(&data->lock);
 	if (ret < 0) {
-		dev_err(&data->client->dev, "register read failed\n");
+		dev_err(dev, "register read failed\n");
 		return ret;
 	}
 	*val = be16_to_cpu(buf);
@@ -273,7 +274,7 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
 	int ret;
 	unsigned int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
-	struct i2c_client *client = data->client;
+	struct device *dev = &data->client->dev;
 
 	ret = regmap_field_read(data->reg_ps_gain, &index);
 	if (ret < 0)
@@ -292,7 +293,7 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
 	buf = cpu_to_be16(val);
 	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
 	if (ret < 0)
-		dev_err(&client->dev, "failed to set PS threshold!\n");
+		dev_err(dev, "failed to set PS threshold!\n");
 
 	return ret;
 }
@@ -321,13 +322,13 @@ static int stk3310_write_event_config(struct iio_dev *indio_dev,
 {
 	int ret;
 	struct stk3310_data *data = iio_priv(indio_dev);
-	struct i2c_client *client = data->client;
+	struct device *dev = &data->client->dev;
 
 	/* Set INT_PS value */
 	mutex_lock(&data->lock);
 	ret = regmap_field_write(data->reg_int_ps, state);
 	if (ret < 0)
-		dev_err(&client->dev, "failed to set interrupt mode\n");
+		dev_err(dev, "failed to set interrupt mode\n");
 	mutex_unlock(&data->lock);
 
 	return ret;
@@ -342,7 +343,7 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 	int ret;
 	unsigned int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
-	struct i2c_client *client = data->client;
+	struct device *dev = &data->client->dev;
 
 	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
 		return -EINVAL;
@@ -357,7 +358,7 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 		mutex_lock(&data->lock);
 		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
 		if (ret < 0) {
-			dev_err(&client->dev, "register read failed\n");
+			dev_err(dev, "register read failed\n");
 			mutex_unlock(&data->lock);
 			return ret;
 		}
@@ -398,6 +399,7 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 	int ret;
 	int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
 
 	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
 		return -EINVAL;
@@ -415,8 +417,7 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 		else
 			ret = regmap_field_write(data->reg_ps_it, index);
 		if (ret < 0)
-			dev_err(&data->client->dev,
-				"sensor configuration failed\n");
+			dev_err(dev, "sensor configuration failed\n");
 		mutex_unlock(&data->lock);
 		return ret;
 
@@ -432,8 +433,7 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 		else
 			ret = regmap_field_write(data->reg_ps_gain, index);
 		if (ret < 0)
-			dev_err(&data->client->dev,
-				"sensor configuration failed\n");
+			dev_err(dev, "sensor configuration failed\n");
 		mutex_unlock(&data->lock);
 		return ret;
 	}
@@ -454,7 +454,7 @@ static const struct iio_info stk3310_info = {
 static int stk3310_set_state(struct stk3310_data *data, u8 state)
 {
 	int ret;
-	struct i2c_client *client = data->client;
+	struct device *dev = &data->client->dev;
 
 	/* 3-bit state; 0b100 is not supported. */
 	if (state > 7 || state == 4)
@@ -463,7 +463,7 @@ static int stk3310_set_state(struct stk3310_data *data, u8 state)
 	mutex_lock(&data->lock);
 	ret = regmap_field_write(data->reg_state, state);
 	if (ret < 0) {
-		dev_err(&client->dev, "failed to change sensor state\n");
+		dev_err(dev, "failed to change sensor state\n");
 	} else if (state != STK3310_STATE_STANDBY) {
 		/* Don't reset the 'enabled' flags if we're going in standby */
 		data->ps_enabled  = !!(state & STK3310_STATE_EN_PS);
@@ -500,7 +500,7 @@ static int stk3310_init(struct iio_dev *indio_dev)
 
 	ret = stk3310_check_chip_id(chipid);
 	if (ret < 0)
-		dev_info(&client->dev, "new unknown chip id: 0x%x\n", chipid);
+		dev_info(dev, "new unknown chip id: 0x%x\n", chipid);
 
 	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
 	ret = stk3310_set_state(data, state);
@@ -618,12 +618,13 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
 
 	struct iio_dev *indio_dev = private;
 	struct stk3310_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
 
 	/* Read FLAG_NF to figure out what threshold has been met. */
 	mutex_lock(&data->lock);
 	ret = regmap_field_read(data->reg_flag_nf, &dir);
 	if (ret < 0) {
-		dev_err(&data->client->dev, "register read failed: %d\n", ret);
+		dev_err(dev, "register read failed: %d\n", ret);
 		goto out;
 	}
 	event = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 1,
@@ -635,7 +636,7 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
 	/* Reset the interrupt flag */
 	ret = regmap_field_write(data->reg_flag_psint, 0);
 	if (ret < 0)
-		dev_err(&data->client->dev, "failed to reset interrupts\n");
+		dev_err(dev, "failed to reset interrupts\n");
 out:
 	mutex_unlock(&data->lock);
 
@@ -674,7 +675,7 @@ static int stk3310_probe(struct i2c_client *client)
 	struct stk3310_data *data;
 	struct device *dev = &client->dev;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev) {
 		dev_err(&client->dev, "iio allocation failed!\n");
 		return -ENOMEM;
@@ -684,8 +685,7 @@ static int stk3310_probe(struct i2c_client *client)
 	data->client = client;
 	i2c_set_clientdata(client, indio_dev);
 
-	device_property_read_u32(&client->dev, "proximity-near-level",
-				 &data->ps_near_level);
+	device_property_read_u32(dev, "proximity-near-level", &data->ps_near_level);
 
 	ret = devm_mutex_init(dev, &data->lock);
 	if (ret)
@@ -722,7 +722,7 @@ static int stk3310_probe(struct i2c_client *client)
 		return ret;
 
 	if (client->irq > 0) {
-		ret = devm_request_threaded_irq(&client->dev, client->irq,
+		ret = devm_request_threaded_irq(dev, client->irq,
 						stk3310_irq_handler,
 						stk3310_irq_event_handler,
 						IRQF_TRIGGER_FALLING |
-- 
2.48.1

[PATCH v5 6/8] 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:
    Changes in v4:
     - Get a struct device ahead of time so it can be passed as "dev"
       instead of "&client->dev"
    
    No changes in v3
    
    Added in v2

 drivers/iio/light/stk3310.c | 42 +++++++++++++++----------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 2233eab63b7aa..9d517d51f6bae 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -504,10 +504,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(dev, ret, "failed to enable sensor\n");
 
 	ret = devm_add_action_or_reset(dev, stk3310_set_state_disable, data);
 	if (ret)
@@ -516,9 +514,9 @@ 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(dev, ret, "failed to enable interrupts!\n");
 
-	return ret;
+	return 0;
 }
 
 static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
@@ -547,14 +545,14 @@ static const struct regmap_config stk3310_regmap_config = {
 static int stk3310_regmap_init(struct stk3310_data *data)
 {
 	struct regmap *regmap;
-	struct i2c_client *client;
+	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
 
-	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(dev, PTR_ERR(regmap),
+				     "regmap initialization failed\n");
+
 	data->regmap = regmap;
 
 	data->reg_state = devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_state);
@@ -676,10 +674,8 @@ static int stk3310_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
-	if (!indio_dev) {
-		dev_err(&client->dev, "iio allocation failed!\n");
-		return -ENOMEM;
-	}
+	if (!indio_dev)
+		return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n");
 
 	data = iio_priv(indio_dev);
 	data->client = client;
@@ -728,18 +724,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(dev, ret, "request irq %d failed\n",
+					     client->irq);
 	}
 
 	ret = devm_iio_device_register(dev, indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "device_register failed\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "device_register failed\n");
 
 	return 0;
 }
-- 
2.48.1

Re: [PATCH v5 6/8] iio: light: stk3310: use dev_err_probe where possible

[Andy Shevchenko]

On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote:
> Using dev_err_probe instead of dev_err and return makes the errors

Use dev_err_probe()
dev_err()

> easier to understand by including the error name, and saves a little
> code.

I believe this patch will make more sense before switching to local 'dev'
variable. Then the previous one will have an additional justification as
the "struct device *dev = ...;" lines in some cases will be added already
by this patch.

...

>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> -	if (!indio_dev) {
> -		dev_err(&client->dev, "iio allocation failed!\n");
> -		return -ENOMEM;
> -	}
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n");

We don't issue the messages for -ENOMEM.

If it's in the current code, add a new patch to drop this message and return an
error code directly.

...

> +	if (ret < 0)

Perhaps, while at it, drop these ' < 0' parts where they are not hinting about
anything.

> +		return dev_err_probe(dev, ret, "device_register failed\n");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 6/8] iio: light: stk3310: use dev_err_probe where possible

[Jonathan Cameron]

On Sun, 9 Feb 2025 16:47:44 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote:
> > Using dev_err_probe instead of dev_err and return makes the errors  
> 
> Use dev_err_probe()
> dev_err()
> 
> > easier to understand by including the error name, and saves a little
> > code.  
> 
> I believe this patch will make more sense before switching to local 'dev'
> variable. Then the previous one will have an additional justification as
> the "struct device *dev = ...;" lines in some cases will be added already
> by this patch.

I'm not sure I follow this one comment.
The only line that has struct device *dev =  added in this patch is
replacing an existing client->dev lookup that could have been pushed
to previous patch if this patch ordering was maintained.

For dev_err() to dev_err_probe() the number of references to dev
is the same after all. The only additional justification this patch
makes is some longer lines that using a local dev pointer shortens
again.

> 
> ...
> 
> >  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > -	if (!indio_dev) {
> > -		dev_err(&client->dev, "iio allocation failed!\n");
> > -		return -ENOMEM;
> > -	}
> > +	if (!indio_dev)
> > +		return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n");  
> 
> We don't issue the messages for -ENOMEM.
> 
> If it's in the current code, add a new patch to drop this message and return an
> error code directly.
I'd be fine with that dev_err() dropped in this patch as long as the
description mentions it.
> 
> ...
> 
> > +	if (ret < 0)  
> 
> Perhaps, while at it, drop these ' < 0' parts where they are not hinting about
> anything.

That would be a separate patch and indeed makes sense to me as well.

Jonathan

> 
> > +		return dev_err_probe(dev, ret, "device_register failed\n");  
> 

Re: [PATCH v5 6/8] iio: light: stk3310: use dev_err_probe where possible

[Andy Shevchenko]

On Tue, Feb 11, 2025 at 07:43:11PM +0000, Jonathan Cameron wrote:
> On Sun, 9 Feb 2025 16:47:44 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote:
> > > Using dev_err_probe instead of dev_err and return makes the errors  
> > 
> > Use dev_err_probe()
> > dev_err()
> > 
> > > easier to understand by including the error name, and saves a little
> > > code.  
> > 
> > I believe this patch will make more sense before switching to local 'dev'
> > variable. Then the previous one will have an additional justification as
> > the "struct device *dev = ...;" lines in some cases will be added already
> > by this patch.
> 
> I'm not sure I follow this one comment.
> The only line that has struct device *dev =  added in this patch is
> replacing an existing client->dev lookup that could have been pushed
> to previous patch if this patch ordering was maintained.
> 
> For dev_err() to dev_err_probe() the number of references to dev
> is the same after all. The only additional justification this patch
> makes is some longer lines that using a local dev pointer shortens
> again.

When converting to dev_err_probe() in some cases it makes sense to add a
temporary variable at the same time.

	if (ret) {
		dev_err(&pdev->dev, ...);
		return ...;
	}

===>

	struct device *dev = &pdev->dev;
	...
	if (ret)
		return dev_err_probe(dev, ...);

which reduces automatically the churn in the patch that wants to (re)use that
temporary variable and also adds to the justification as "we already have that
variable, just want to use it".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 6/8] iio: light: stk3310: use dev_err_probe where possible

[Aren]

On Sun, Feb 09, 2025 at 04:47:44PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 08, 2025 at 04:13:24PM -0500, Aren Moynihan wrote:
> > Using dev_err_probe instead of dev_err and return makes the errors
> 
> Use dev_err_probe()
> dev_err()
> 
> > easier to understand by including the error name, and saves a little
> > code.
> 
> I believe this patch will make more sense before switching to local 'dev'
> variable. Then the previous one will have an additional justification as
> the "struct device *dev = ...;" lines in some cases will be added already
> by this patch.

That will only be added in one spot, and I skipped updating the dev_err
calls in the previous patch that this patch rewrites, so churn shouldn't
be an issue. That also makes it trivial to reorder them, so I guess it
can't hurt.

> >  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > -	if (!indio_dev) {
> > -		dev_err(&client->dev, "iio allocation failed!\n");
> > -		return -ENOMEM;
> > -	}
> > +	if (!indio_dev)
> > +		return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n");
> 
> We don't issue the messages for -ENOMEM.
> 
> If it's in the current code, add a new patch to drop this message and return an
> error code directly.
> 
> ...
> 
> > +	if (ret < 0)
> 
> Perhaps, while at it, drop these ' < 0' parts where they are not hinting about
> anything.

Sure, I can add patches for these, although continuing to rebase this
series is getting a bit cumbersome (perhaps just because I haven't found
a good workflow for it). Would I be better off reordering this so the
refactoring patches come first and can be partially merged?

Regards
 - Aren

[PATCH v5 7/8] 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. Potential
errors include ENXIO (when the chip isn't powered) and ETIMEDOUT (when
the i2c bus isn't powered).

This function is only called from stk3310_probe, and this condition
should return an error, which fits what dev_err_probe is designed for.

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

Notes:
    Changes in v4:
     - get a struct device ahead of time so it can be passed as "dev"
       instead of "&client->dev"
    
    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 9d517d51f6bae..3a06852cb0ab4 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -496,7 +496,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(dev, ret, "failed to read chip id\n");
 
 	ret = stk3310_check_chip_id(chipid);
 	if (ret < 0)
-- 
2.48.1

[PATCH v5 8/8] 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 a2ceb08a91950..d70fbc06b8d23 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -278,6 +278,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.48.1