[PATCH v4 0/5] iio: light: Modernize al3010 and al3320a codebase

[PATCH v4 0/5] iio: light: Modernize al3010 and al3320a codebase

[David Heidelberg via B4 Relay]

This series aims to improve code readability and modernize it to align
with the recently upstreamed AL3000a.

Apart from slightly improved error reporting, and error handling
there should be no functional changes.

Module  before after
al3010  72 kB  58 kB
al3320a 72 kB  58 kB

Signed-off-by: David Heidelberg <david@ixit.cz>
---
Changes in v4:
- Fixed mixed-up rebase changes between commits and added
  regmap_get_device into _init functions to get the device.
- Link to v3: https://lore.kernel.org/r/20250402-al3010-iio-regmap-v3-0-cc3da273b5b2@ixit.cz

Changes in v3:
- Stripped patches merged from second version of patchset.
- Dropped iio: light: al3010: Move devm_add_action_or_reset back to _probe
  in favor of opposite approach moving devm_add.. to _init for al3xx0a:
  - iio: light: al3000a: Fix an error handling path in al3000a_probe()
  - iio: light: al3320a: Fix an error handling path in al3320a_probe()
- Link to v2: https://lore.kernel.org/r/20250319-al3010-iio-regmap-v2-0-1310729d0543@ixit.cz

Changes in v2:
- Dropped Daniel's email update.
- Dropped DRV_NAME introduction for al3000a
- Added DRV_NAME define removal for al3010 and al3320a.
- Splitted unsigned int conversion into separate patches.
- Replaced generic value with specific raw and gain variable.
- Use dev_err_probe() for error handling.
- Separated devm_add_action_or_reset move from _init back to _probe.
- Dropped copyright update.
- Link to v1: https://lore.kernel.org/r/20250308-al3010-iio-regmap-v1-0-b672535e8213@ixit.cz

---
David Heidelberg (5):
      iio: light: al3010: Improve al3010_init error handling with dev_err_probe()
      iio: light: al3000a: Fix an error handling path in al3000a_probe()
      iio: light: al3320a: Fix an error handling path in al3320a_probe()
      iio: light: al3010: Implement regmap support
      iio: light: al3320a: Implement regmap support

 drivers/iio/light/al3000a.c |  9 +++--
 drivers/iio/light/al3010.c  | 85 +++++++++++++++++++++++--------------------
 drivers/iio/light/al3320a.c | 89 +++++++++++++++++++++++++--------------------
 3 files changed, 100 insertions(+), 83 deletions(-)
---
base-commit: f8ffc92ae9052e6615896052f0c5b808bfc17520
change-id: 20250308-al3010-iio-regmap-038cea39f85d

Best regards,
-- 
David Heidelberg <david@ixit.cz>

[PATCH v4 1/5] iio: light: al3010: Improve al3010_init error handling with dev_err_probe()

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

Minor code simplifications and improved error reporting.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3010.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
index 8c004a9239aef246a8c6f6c3f4acd6b760ee8249..34a29a51c5f9e8aa143d3ba195b79a19793e4f88 100644
--- a/drivers/iio/light/al3010.c
+++ b/drivers/iio/light/al3010.c
@@ -92,8 +92,8 @@ static int al3010_init(struct al3010_data *data)
 	ret = devm_add_action_or_reset(&data->client->dev,
 				       al3010_set_pwr_off,
 				       data);
-	if (ret < 0)
-		return ret;
+	if (ret)
+		return dev_err_probe(&data->client->dev, ret, "failed to add action\n");
 
 	ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
 					FIELD_PREP(AL3010_GAIN_MASK,
@@ -190,10 +190,8 @@ static int al3010_probe(struct i2c_client *client)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = al3010_init(data);
-	if (ret < 0) {
-		dev_err(dev, "al3010 chip init failed\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init ALS\n");
 
 	return devm_iio_device_register(dev, indio_dev);
 }

-- 
2.49.0

[PATCH v4 2/5] iio: light: al3000a: Fix an error handling path in al3000a_probe()

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

If regmap_write() fails in al3000a_init(), al3000a_set_pwr_off is
not called.

In order to avoid such a situation, move the devm_add_action_or_reset()
which calls al3000a_set_pwr_off right after a successful
al3000a_set_pwr_on.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3000a.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
index e2fbb1270040f43d9f0a97838861818a8eaef813..6d5115b2a06c5acce18d831b9c41d3d5121fba12 100644
--- a/drivers/iio/light/al3000a.c
+++ b/drivers/iio/light/al3000a.c
@@ -85,12 +85,17 @@ static void al3000a_set_pwr_off(void *_data)
 
 static int al3000a_init(struct al3000a_data *data)
 {
+	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
 	ret = al3000a_set_pwr_on(data);
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add action\n");
+
 	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
 	if (ret)
 		return ret;
@@ -157,10 +162,6 @@ static int al3000a_probe(struct i2c_client *client)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to init ALS\n");
 
-	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to add action\n");
-
 	return devm_iio_device_register(dev, indio_dev);
 }
 

-- 
2.49.0

[PATCH v4 3/5] iio: light: al3320a: Fix an error handling path in al3320a_probe()

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

If regmap_write() fails in al3320a_init(), al3320a_set_pwr_off is
not called.

In order to avoid such a situation, move the devm_add_action_or_reset()
which calls al3320a_set_pwr_off right after a successful
al3320a_set_pwr_on.

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3320a.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index 1b2b0359ed5dad5e00d2fe584f8f3495c13c997e..1fa693d54ae2a6e5ead3a9c7ac7018afeba9b760 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -101,6 +101,12 @@ static int al3320a_init(struct al3320a_data *data)
 	if (ret < 0)
 		return ret;
 
+	ret = devm_add_action_or_reset(&data->client->dev,
+				       al3320a_set_pwr_off,
+				       data);
+	if (ret)
+		return dev_err_probe(&data->client->dev, ret, "failed to add action\n");
+
 	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
 					FIELD_PREP(AL3320A_GAIN_MASK,
 						   AL3320A_RANGE_3));
@@ -211,10 +217,6 @@ static int al3320a_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	ret = devm_add_action_or_reset(dev, al3320a_set_pwr_off, data);
-	if (ret < 0)
-		return ret;
-
 	return devm_iio_device_register(dev, indio_dev);
 }
 

-- 
2.49.0

[PATCH v4 4/5] iio: light: al3010: Implement regmap support

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

Modernize and clean up the driver using the regmap framework.

With the regmap implementation, the compiler produces
a significantly smaller module.

Size before: 72 kB
Size after:  58 kB

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3010.c | 77 +++++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
index 34a29a51c5f9e8aa143d3ba195b79a19793e4f88..d4c9a6e5c415269332dda67622b17b46fb157218 100644
--- a/drivers/iio/light/al3010.c
+++ b/drivers/iio/light/al3010.c
@@ -17,6 +17,7 @@
 #include <linux/bitfield.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/mod_devicetable.h>
 
 #include <linux/iio/iio.h>
@@ -44,8 +45,14 @@ static const int al3010_scales[][2] = {
 	{0, 1187200}, {0, 296800}, {0, 74200}, {0, 18600}
 };
 
+static const struct regmap_config al3010_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AL3010_REG_CONFIG,
+};
+
 struct al3010_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 };
 
 static const struct iio_chan_spec al3010_channels[] = {
@@ -67,41 +74,37 @@ static const struct attribute_group al3010_attribute_group = {
 	.attrs = al3010_attributes,
 };
 
-static int al3010_set_pwr_on(struct i2c_client *client)
+static int al3010_set_pwr_on(struct al3010_data *data)
 {
-	return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM,
-					 AL3010_CONFIG_ENABLE);
+	return regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_ENABLE);
 }
 
 static void al3010_set_pwr_off(void *_data)
 {
 	struct al3010_data *data = _data;
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
 
-	i2c_smbus_write_byte_data(data->client, AL3010_REG_SYSTEM,
-				  AL3010_CONFIG_DISABLE);
+	ret = regmap_write(data->regmap, AL3010_REG_SYSTEM, AL3010_CONFIG_DISABLE);
+	if (ret)
+		dev_err(dev, "failed to write system register\n");
 }
 
 static int al3010_init(struct al3010_data *data)
 {
+	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
-	ret = al3010_set_pwr_on(data->client);
-	if (ret < 0)
+	ret = al3010_set_pwr_on(data);
+	if (ret)
 		return ret;
 
-	ret = devm_add_action_or_reset(&data->client->dev,
-				       al3010_set_pwr_off,
-				       data);
+	ret = devm_add_action_or_reset(dev, al3010_set_pwr_off, data);
 	if (ret)
-		return dev_err_probe(&data->client->dev, ret, "failed to add action\n");
-
-	ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
-					FIELD_PREP(AL3010_GAIN_MASK,
-						   AL3XXX_RANGE_3));
-	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "failed to add action\n");
 
-	return 0;
+	return regmap_write(data->regmap, AL3010_REG_CONFIG,
+			    FIELD_PREP(AL3010_GAIN_MASK, AL3XXX_RANGE_3));
 }
 
 static int al3010_read_raw(struct iio_dev *indio_dev,
@@ -109,7 +112,7 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
 			   int *val2, long mask)
 {
 	struct al3010_data *data = iio_priv(indio_dev);
-	int ret;
+	int ret, gain, raw;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -118,21 +121,21 @@ static int al3010_read_raw(struct iio_dev *indio_dev,
 		 * - low byte of output is stored at AL3010_REG_DATA_LOW
 		 * - high byte of output is stored at AL3010_REG_DATA_LOW + 1
 		 */
-		ret = i2c_smbus_read_word_data(data->client,
-					       AL3010_REG_DATA_LOW);
-		if (ret < 0)
+		ret = regmap_read(data->regmap, AL3010_REG_DATA_LOW, &raw);
+		if (ret)
 			return ret;
-		*val = ret;
+
+		*val = raw;
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		ret = i2c_smbus_read_byte_data(data->client,
-					       AL3010_REG_CONFIG);
-		if (ret < 0)
+		ret = regmap_read(data->regmap, AL3010_REG_CONFIG, &gain);
+		if (ret)
 			return ret;
 
-		ret = FIELD_GET(AL3010_GAIN_MASK, ret);
-		*val = al3010_scales[ret][0];
-		*val2 = al3010_scales[ret][1];
+		gain = FIELD_GET(AL3010_GAIN_MASK, gain);
+		*val = al3010_scales[gain][0];
+		*val2 = al3010_scales[gain][1];
 
 		return IIO_VAL_INT_PLUS_MICRO;
 	}
@@ -153,9 +156,8 @@ static int al3010_write_raw(struct iio_dev *indio_dev,
 			    val2 != al3010_scales[i][1])
 				continue;
 
-			return i2c_smbus_write_byte_data(data->client,
-					AL3010_REG_CONFIG,
-					FIELD_PREP(AL3010_GAIN_MASK, i));
+			return regmap_write(data->regmap, AL3010_REG_CONFIG,
+					    FIELD_PREP(AL3010_GAIN_MASK, i));
 		}
 		break;
 	}
@@ -181,7 +183,10 @@ static int al3010_probe(struct i2c_client *client)
 
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
-	data->client = client;
+	data->regmap = devm_regmap_init_i2c(client, &al3010_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "cannot allocate regmap\n");
 
 	indio_dev->info = &al3010_info;
 	indio_dev->name = "al3010";
@@ -206,7 +211,9 @@ static int al3010_suspend(struct device *dev)
 
 static int al3010_resume(struct device *dev)
 {
-	return al3010_set_pwr_on(to_i2c_client(dev));
+	struct al3010_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return al3010_set_pwr_on(data);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(al3010_pm_ops, al3010_suspend, al3010_resume);

-- 
2.49.0

[PATCH v4 5/5] iio: light: al3320a: Implement regmap support

[David Heidelberg via B4 Relay]

From: David Heidelberg <david@ixit.cz>

Modernize and clean up the driver using the regmap framework.

With the regmap implementation, the compiler produces
a significantly smaller module.

Size before: 72 kB
Size after:  58 kB

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3320a.c | 89 +++++++++++++++++++++++++--------------------
 1 file changed, 49 insertions(+), 40 deletions(-)

diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index 1fa693d54ae2a6e5ead3a9c7ac7018afeba9b760..988a3dd3a8449f10c657eb75fd744b2a9db2965e 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -15,6 +15,7 @@
 #include <linux/bitfield.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/mod_devicetable.h>
 
 #include <linux/iio/iio.h>
@@ -57,8 +58,14 @@ static const int al3320a_scales[][2] = {
 	{0, 512000}, {0, 128000}, {0, 32000}, {0, 10000}
 };
 
+static const struct regmap_config al3320a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AL3320A_REG_HIGH_THRESH_HIGH,
+};
+
 struct al3320a_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 };
 
 static const struct iio_chan_spec al3320a_channels[] = {
@@ -80,50 +87,47 @@ static const struct attribute_group al3320a_attribute_group = {
 	.attrs = al3320a_attributes,
 };
 
-static int al3320a_set_pwr_on(struct i2c_client *client)
+static int al3320a_set_pwr_on(struct al3320a_data *data)
 {
-	return i2c_smbus_write_byte_data(client, AL3320A_REG_CONFIG, AL3320A_CONFIG_ENABLE);
+	return regmap_write(data->regmap, AL3320A_REG_CONFIG, AL3320A_CONFIG_ENABLE);
 }
 
 static void al3320a_set_pwr_off(void *_data)
 {
 	struct al3320a_data *data = _data;
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
 
-	i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG, AL3320A_CONFIG_DISABLE);
+	ret = regmap_write(data->regmap, AL3320A_REG_CONFIG, AL3320A_CONFIG_DISABLE);
+	if (ret)
+		dev_err(dev, "failed to write system register\n");
 }
 
 static int al3320a_init(struct al3320a_data *data)
 {
+	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
-	ret = al3320a_set_pwr_on(data->client);
-
-	if (ret < 0)
+	ret = al3320a_set_pwr_on(data);
+	if (ret)
 		return ret;
 
-	ret = devm_add_action_or_reset(&data->client->dev,
-				       al3320a_set_pwr_off,
-				       data);
+	ret = devm_add_action_or_reset(dev, al3320a_set_pwr_off, data);
 	if (ret)
-		return dev_err_probe(&data->client->dev, ret, "failed to add action\n");
-
-	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
-					FIELD_PREP(AL3320A_GAIN_MASK,
-						   AL3320A_RANGE_3));
-	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "failed to add action\n");
 
-	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_MEAN_TIME,
-					AL3320A_DEFAULT_MEAN_TIME);
-	if (ret < 0)
+	ret = regmap_write(data->regmap, AL3320A_REG_CONFIG_RANGE,
+			   FIELD_PREP(AL3320A_GAIN_MASK, AL3320A_RANGE_3));
+	if (ret)
 		return ret;
 
-	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_WAIT,
-					AL3320A_DEFAULT_WAIT_TIME);
-	if (ret < 0)
+	ret = regmap_write(data->regmap, AL3320A_REG_MEAN_TIME,
+			   AL3320A_DEFAULT_MEAN_TIME);
+	if (ret)
 		return ret;
 
-	return 0;
+	return regmap_write(data->regmap, AL3320A_REG_WAIT,
+			    AL3320A_DEFAULT_WAIT_TIME);
 }
 
 static int al3320a_read_raw(struct iio_dev *indio_dev,
@@ -131,7 +135,7 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
 			    int *val2, long mask)
 {
 	struct al3320a_data *data = iio_priv(indio_dev);
-	int ret;
+	int ret, gain, raw;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -140,21 +144,21 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
 		 * - low byte of output is stored at AL3320A_REG_DATA_LOW
 		 * - high byte of output is stored at AL3320A_REG_DATA_LOW + 1
 		 */
-		ret = i2c_smbus_read_word_data(data->client,
-					       AL3320A_REG_DATA_LOW);
-		if (ret < 0)
+		ret = regmap_read(data->regmap, AL3320A_REG_DATA_LOW, &raw);
+		if (ret)
 			return ret;
-		*val = ret;
+
+		*val = raw;
+
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		ret = i2c_smbus_read_byte_data(data->client,
-					       AL3320A_REG_CONFIG_RANGE);
-		if (ret < 0)
+		ret = regmap_read(data->regmap, AL3320A_REG_CONFIG_RANGE, &gain);
+		if (ret)
 			return ret;
 
-		ret = FIELD_GET(AL3320A_GAIN_MASK, ret);
-		*val = al3320a_scales[ret][0];
-		*val2 = al3320a_scales[ret][1];
+		gain = FIELD_GET(AL3320A_GAIN_MASK, gain);
+		*val = al3320a_scales[gain][0];
+		*val2 = al3320a_scales[gain][1];
 
 		return IIO_VAL_INT_PLUS_MICRO;
 	}
@@ -175,9 +179,8 @@ static int al3320a_write_raw(struct iio_dev *indio_dev,
 			    val2 != al3320a_scales[i][1])
 				continue;
 
-			return i2c_smbus_write_byte_data(data->client,
-					AL3320A_REG_CONFIG_RANGE,
-					FIELD_PREP(AL3320A_GAIN_MASK, i));
+			return regmap_write(data->regmap, AL3320A_REG_CONFIG_RANGE,
+					    FIELD_PREP(AL3320A_GAIN_MASK, i));
 		}
 		break;
 	}
@@ -203,7 +206,11 @@ static int al3320a_probe(struct i2c_client *client)
 
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
-	data->client = client;
+
+	data->regmap = devm_regmap_init_i2c(client, &al3320a_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "cannot allocate regmap\n");
 
 	indio_dev->info = &al3320a_info;
 	indio_dev->name = "al3320a";
@@ -230,7 +237,9 @@ static int al3320a_suspend(struct device *dev)
 
 static int al3320a_resume(struct device *dev)
 {
-	return al3320a_set_pwr_on(to_i2c_client(dev));
+	struct al3320a_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return al3320a_set_pwr_on(data);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(al3320a_pm_ops, al3320a_suspend,

-- 
2.49.0

Re: [PATCH v4 1/5] iio: light: al3010: Improve al3010_init error handling with dev_err_probe()

[Christophe JAILLET]

Le 02/04/2025 à 21:33, David Heidelberg via B4 Relay a écrit :
> From: David Heidelberg <david-W22tF5X+A20@public.gmane.org>
> 
> Minor code simplifications and improved error reporting.
> 
> Signed-off-by: David Heidelberg <david-W22tF5X+A20@public.gmane.org>
> ---
>   drivers/iio/light/al3010.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
> index 8c004a9239aef246a8c6f6c3f4acd6b760ee8249..34a29a51c5f9e8aa143d3ba195b79a19793e4f88 100644
> --- a/drivers/iio/light/al3010.c
> +++ b/drivers/iio/light/al3010.c
> @@ -92,8 +92,8 @@ static int al3010_init(struct al3010_data *data)
>   	ret = devm_add_action_or_reset(&data->client->dev,
>   				       al3010_set_pwr_off,
>   				       data);
> -	if (ret < 0)
> -		return ret;
> +	if (ret)
> +		return dev_err_probe(&data->client->dev, ret, "failed to add action\n");

Not sure this new message is needed.

The error is unlikely, and kmalloc(), which is used in is 
devm_add_action_or_reset(), is already verbose.

CJ

>   
>   	ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
>   					FIELD_PREP(AL3010_GAIN_MASK,
> @@ -190,10 +190,8 @@ static int al3010_probe(struct i2c_client *client)
>   	indio_dev->modes = INDIO_DIRECT_MODE;
>   
>   	ret = al3010_init(data);
> -	if (ret < 0) {
> -		dev_err(dev, "al3010 chip init failed\n");
> -		return ret;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to init ALS\n");
>   
>   	return devm_iio_device_register(dev, indio_dev);
>   }
> 

Re: [PATCH v4 1/5] iio: light: al3010: Improve al3010_init error handling with dev_err_probe()

[Jonathan Cameron]

On Thu, 3 Apr 2025 20:01:46 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> Le 02/04/2025 à 21:33, David Heidelberg via B4 Relay a écrit :
> > From: David Heidelberg <david-W22tF5X+A20@public.gmane.org>
> > 
> > Minor code simplifications and improved error reporting.
> > 
> > Signed-off-by: David Heidelberg <david-W22tF5X+A20@public.gmane.org>
> > ---
> >   drivers/iio/light/al3010.c | 10 ++++------
> >   1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
> > index 8c004a9239aef246a8c6f6c3f4acd6b760ee8249..34a29a51c5f9e8aa143d3ba195b79a19793e4f88 100644
> > --- a/drivers/iio/light/al3010.c
> > +++ b/drivers/iio/light/al3010.c
> > @@ -92,8 +92,8 @@ static int al3010_init(struct al3010_data *data)
> >   	ret = devm_add_action_or_reset(&data->client->dev,
> >   				       al3010_set_pwr_off,
> >   				       data);
> > -	if (ret < 0)
> > -		return ret;
> > +	if (ret)
> > +		return dev_err_probe(&data->client->dev, ret, "failed to add action\n");  
> 
> Not sure this new message is needed.
> 
> The error is unlikely, and kmalloc(), which is used in is 
> devm_add_action_or_reset(), is already verbose.
Good point. I agree this should stay as a simple return.  

If nothing else comes up I'll tidy that up whilst applying,

Jonathan

> 
> CJ
> 
> >   
> >   	ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
> >   					FIELD_PREP(AL3010_GAIN_MASK,
> > @@ -190,10 +190,8 @@ static int al3010_probe(struct i2c_client *client)
> >   	indio_dev->modes = INDIO_DIRECT_MODE;
> >   
> >   	ret = al3010_init(data);
> > -	if (ret < 0) {
> > -		dev_err(dev, "al3010 chip init failed\n");
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to init ALS\n");
> >   
> >   	return devm_iio_device_register(dev, indio_dev);
> >   }
> >   
> 

Re: [PATCH v4 2/5] iio: light: al3000a: Fix an error handling path in al3000a_probe()

[Jonathan Cameron]

On Wed, 02 Apr 2025 21:33:25 +0200
David Heidelberg via B4 Relay <devnull+david.ixit.cz@kernel.org> wrote:

> From: David Heidelberg <david@ixit.cz>
> 
> If regmap_write() fails in al3000a_init(), al3000a_set_pwr_off is
> not called.
> 
> In order to avoid such a situation, move the devm_add_action_or_reset()
> which calls al3000a_set_pwr_off right after a successful
> al3000a_set_pwr_on.
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  drivers/iio/light/al3000a.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/al3000a.c b/drivers/iio/light/al3000a.c
> index e2fbb1270040f43d9f0a97838861818a8eaef813..6d5115b2a06c5acce18d831b9c41d3d5121fba12 100644
> --- a/drivers/iio/light/al3000a.c
> +++ b/drivers/iio/light/al3000a.c
> @@ -85,12 +85,17 @@ static void al3000a_set_pwr_off(void *_data)
>  
>  static int al3000a_init(struct al3000a_data *data)
>  {
> +	struct device *dev = regmap_get_device(data->regmap);
>  	int ret;
>  
>  	ret = al3000a_set_pwr_on(data);
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add action\n");
Whilst this is the same thing Christophe pointed out in patch 1, as it is simple code
movement I think it is fine to leave it here 'for now'. 

It probably makes sense to scrub IIO in general for cases of this if we decide
the general rule is no error messages for devm_add_action* failures (given they
are only possible if memory allocation fails).

Jonathan

> +
>  	ret = regmap_write(data->regmap, AL3000A_REG_SYSTEM, AL3000A_CONFIG_RESET);
>  	if (ret)
>  		return ret;
> @@ -157,10 +162,6 @@ static int al3000a_probe(struct i2c_client *client)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to init ALS\n");
>  
> -	ret = devm_add_action_or_reset(dev, al3000a_set_pwr_off, data);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to add action\n");
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> 

Re: [PATCH v4 3/5] iio: light: al3320a: Fix an error handling path in al3320a_probe()

[Jonathan Cameron]

On Wed, 02 Apr 2025 21:33:26 +0200
David Heidelberg via B4 Relay <devnull+david.ixit.cz@kernel.org> wrote:

> From: David Heidelberg <david@ixit.cz>
> 
> If regmap_write() fails in al3320a_init(), al3320a_set_pwr_off is
> not called.
> 
> In order to avoid such a situation, move the devm_add_action_or_reset()
> which calls al3320a_set_pwr_off right after a successful
> al3320a_set_pwr_on.
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  drivers/iio/light/al3320a.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
> index 1b2b0359ed5dad5e00d2fe584f8f3495c13c997e..1fa693d54ae2a6e5ead3a9c7ac7018afeba9b760 100644
> --- a/drivers/iio/light/al3320a.c
> +++ b/drivers/iio/light/al3320a.c
> @@ -101,6 +101,12 @@ static int al3320a_init(struct al3320a_data *data)
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = devm_add_action_or_reset(&data->client->dev,
> +				       al3320a_set_pwr_off,
> +				       data);
> +	if (ret)
> +		return dev_err_probe(&data->client->dev, ret, "failed to add action\n");
This one I'll drop whilst applying given we are adding a message with little
purpose (or chance of occurring) rather than moving one as we were in patch 2.

I've applied up to this patch with those tweaks. I might well pick up the others but
not looked closely at them yet!
> +
>  	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
>  					FIELD_PREP(AL3320A_GAIN_MASK,
>  						   AL3320A_RANGE_3));
> @@ -211,10 +217,6 @@ static int al3320a_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> -	ret = devm_add_action_or_reset(dev, al3320a_set_pwr_off, data);
> -	if (ret < 0)
> -		return ret;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  
> 

Re: [PATCH v4 0/5] iio: light: Modernize al3010 and al3320a codebase

[Jonathan Cameron]

On Wed, 02 Apr 2025 21:33:23 +0200
David Heidelberg via B4 Relay <devnull+david.ixit.cz@kernel.org> wrote:

> This series aims to improve code readability and modernize it to align
> with the recently upstreamed AL3000a.
> 
> Apart from slightly improved error reporting, and error handling
> there should be no functional changes.
> 
> Module  before after
> al3010  72 kB  58 kB
> al3320a 72 kB  58 kB
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
Applied, but with tweaks to not add print messages as described in patches 1 and 3.
That meant a bunch of hand application was needed for 4-5.  Please check the result
in the testing branch of iio.git.

Thanks,

Jonathan

> Changes in v4:
> - Fixed mixed-up rebase changes between commits and added
>   regmap_get_device into _init functions to get the device.
> - Link to v3: https://lore.kernel.org/r/20250402-al3010-iio-regmap-v3-0-cc3da273b5b2@ixit.cz
> 
> Changes in v3:
> - Stripped patches merged from second version of patchset.
> - Dropped iio: light: al3010: Move devm_add_action_or_reset back to _probe
>   in favor of opposite approach moving devm_add.. to _init for al3xx0a:
>   - iio: light: al3000a: Fix an error handling path in al3000a_probe()
>   - iio: light: al3320a: Fix an error handling path in al3320a_probe()
> - Link to v2: https://lore.kernel.org/r/20250319-al3010-iio-regmap-v2-0-1310729d0543@ixit.cz
> 
> Changes in v2:
> - Dropped Daniel's email update.
> - Dropped DRV_NAME introduction for al3000a
> - Added DRV_NAME define removal for al3010 and al3320a.
> - Splitted unsigned int conversion into separate patches.
> - Replaced generic value with specific raw and gain variable.
> - Use dev_err_probe() for error handling.
> - Separated devm_add_action_or_reset move from _init back to _probe.
> - Dropped copyright update.
> - Link to v1: https://lore.kernel.org/r/20250308-al3010-iio-regmap-v1-0-b672535e8213@ixit.cz
> 
> ---
> David Heidelberg (5):
>       iio: light: al3010: Improve al3010_init error handling with dev_err_probe()
>       iio: light: al3000a: Fix an error handling path in al3000a_probe()
>       iio: light: al3320a: Fix an error handling path in al3320a_probe()
>       iio: light: al3010: Implement regmap support
>       iio: light: al3320a: Implement regmap support
> 
>  drivers/iio/light/al3000a.c |  9 +++--
>  drivers/iio/light/al3010.c  | 85 +++++++++++++++++++++++--------------------
>  drivers/iio/light/al3320a.c | 89 +++++++++++++++++++++++++--------------------
>  3 files changed, 100 insertions(+), 83 deletions(-)
> ---
> base-commit: f8ffc92ae9052e6615896052f0c5b808bfc17520
> change-id: 20250308-al3010-iio-regmap-038cea39f85d
> 
> Best regards,

Re: [PATCH v4 0/5] iio: light: Modernize al3010 and al3320a codebase

[David Heidelberg]

On 05/04/2025 19:12, Jonathan Cameron wrote:
> On Wed, 02 Apr 2025 21:33:23 +0200
> David Heidelberg via B4 Relay <devnull+david.ixit.cz@kernel.org> wrote:
> 
>> This series aims to improve code readability and modernize it to align
>> with the recently upstreamed AL3000a.
>>
>> Apart from slightly improved error reporting, and error handling
>> there should be no functional changes.
>>
>> Module  before after
>> al3010  72 kB  58 kB
>> al3320a 72 kB  58 kB
>>
>> Signed-off-by: David Heidelberg <david@ixit.cz>
>> ---
> Applied, but with tweaks to not add print messages as described in patches 1 and 3.
> That meant a bunch of hand application was needed for 4-5.  Please check the result
> in the testing branch of iio.git.

It does look good.

Thank you!
David

> 
> Thanks,
> 
> Jonathan
> 
>> Changes in v4:
>> - Fixed mixed-up rebase changes between commits and added
>>    regmap_get_device into _init functions to get the device.
>> - Link to v3: https://lore.kernel.org/r/20250402-al3010-iio-regmap-v3-0-cc3da273b5b2@ixit.cz
>>
>> Changes in v3:
>> - Stripped patches merged from second version of patchset.
>> - Dropped iio: light: al3010: Move devm_add_action_or_reset back to _probe
>>    in favor of opposite approach moving devm_add.. to _init for al3xx0a:
>>    - iio: light: al3000a: Fix an error handling path in al3000a_probe()
>>    - iio: light: al3320a: Fix an error handling path in al3320a_probe()
>> - Link to v2: https://lore.kernel.org/r/20250319-al3010-iio-regmap-v2-0-1310729d0543@ixit.cz
>>
>> Changes in v2:
>> - Dropped Daniel's email update.
>> - Dropped DRV_NAME introduction for al3000a
>> - Added DRV_NAME define removal for al3010 and al3320a.
>> - Splitted unsigned int conversion into separate patches.
>> - Replaced generic value with specific raw and gain variable.
>> - Use dev_err_probe() for error handling.
>> - Separated devm_add_action_or_reset move from _init back to _probe.
>> - Dropped copyright update.
>> - Link to v1: https://lore.kernel.org/r/20250308-al3010-iio-regmap-v1-0-b672535e8213@ixit.cz
>>
>> ---
>> David Heidelberg (5):
>>        iio: light: al3010: Improve al3010_init error handling with dev_err_probe()
>>        iio: light: al3000a: Fix an error handling path in al3000a_probe()
>>        iio: light: al3320a: Fix an error handling path in al3320a_probe()
>>        iio: light: al3010: Implement regmap support
>>        iio: light: al3320a: Implement regmap support
>>
>>   drivers/iio/light/al3000a.c |  9 +++--
>>   drivers/iio/light/al3010.c  | 85 +++++++++++++++++++++++--------------------
>>   drivers/iio/light/al3320a.c | 89 +++++++++++++++++++++++++--------------------
>>   3 files changed, 100 insertions(+), 83 deletions(-)
>> ---
>> base-commit: f8ffc92ae9052e6615896052f0c5b808bfc17520
>> change-id: 20250308-al3010-iio-regmap-038cea39f85d
>>
>> Best regards,
> 

-- 
David Heidelberg