[PATCH 0/4] dev_printk: add dev_errp_probe() helper

[PATCH 0/4] dev_printk: add dev_errp_probe() helper

[Nuno Sa]

This series adds a dev_errp_probe() helper. This is similar to
dev_err_probe() but for cases where an ERR_PTR() is to be returned
simplifying patterns like:

	dev_err_probe(dev, ret, ...);
	return ERR_PTR(ret)

The other three patches are adding users for it. The main motivator for
this were the changes in the commit ("iio: temperature: ltc2983: convert
to dev_err_probe()"). Initially I just had a local helper [1] but then
it was suggested to try a new, common helper. As a result, I looked for
a couple more users.

I then move into dev_errp_probe() [2] but it was then suggested to separare
the patch series so we have onde dedicated for the printk helper.

[1]: https://lore.kernel.org/all/20240301-ltc2983-misc-improv-v3-1-c09516ac0efc@analog.com/
[2]: https://lore.kernel.org/all/20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com/

---
Nuno Sa (4):
      dev_printk: add new dev_errp_probe() helper
      iio: temperature: ltc2983: convert to dev_err_probe()
      iio: backend: make use of dev_errp_probe()
      iio: common: scmi_iio: convert to dev_err_probe()

 drivers/iio/common/scmi_sensors/scmi_iio.c |  45 +++--
 drivers/iio/industrialio-backend.c         |   8 +-
 drivers/iio/temperature/ltc2983.c          | 255 +++++++++++++----------------
 include/linux/dev_printk.h                 |   5 +
 4 files changed, 142 insertions(+), 171 deletions(-)
---
base-commit: 2b3d5988ae2cb5cd945ddbc653f0a71706231fdd
change-id: 20240404-dev-add_dev_errp_probe-69e7524c2803
--

Thanks!
- Nuno Sá

[PATCH 1/4] dev_printk: add new dev_errp_probe() helper

[Nuno Sa]

This is similar to dev_err_probe() but for cases where an ERR_PTR() is
to be returned simplifying patterns like:

	dev_err_probe(dev, ret, ...);
	return ERR_PTR(ret)

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 include/linux/dev_printk.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index ae80a303c216..790144f6f99c 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -277,4 +277,9 @@ do {									\
 
 __printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
 
+/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
+#define dev_errp_probe(dev, ___err, fmt, ...)	({		\
+	ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__));	\
+})
+
 #endif /* _DEVICE_PRINTK_H_ */

-- 
2.44.0

[PATCH 2/4] iio: temperature: ltc2983: convert to dev_err_probe()

[Nuno Sa]

Use dev_err_probe() in the probe() path. While at it, made some simple
improvements:
 * Declare a struct device *dev helper. This also makes the style more
   consistent (some places the helper was used and not in other places);
 * Explicitly included the err.h and errno.h headers;
 * Removed an useless else if();
 * Removed some unnecessary line breaks.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/temperature/ltc2983.c | 255 +++++++++++++++++---------------------
 1 file changed, 115 insertions(+), 140 deletions(-)

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 3c4524d57b8e..b4a8ca36458a 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -8,6 +8,8 @@
 #include <linux/bitfield.h>
 #include <linux/completion.h>
 #include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/iio/iio.h>
 #include <linux/interrupt.h>
@@ -657,10 +659,11 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
 			 const struct ltc2983_sensor *sensor)
 {
 	struct ltc2983_thermocouple *thermo;
+	struct device *dev = &st->spi->dev;
 	u32 oc_current;
 	int ret;
 
-	thermo = devm_kzalloc(&st->spi->dev, sizeof(*thermo), GFP_KERNEL);
+	thermo = devm_kzalloc(dev, sizeof(*thermo), GFP_KERNEL);
 	if (!thermo)
 		return ERR_PTR(-ENOMEM);
 
@@ -687,21 +690,19 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
 					LTC2983_THERMOCOUPLE_OC_CURR(3);
 			break;
 		default:
-			dev_err(&st->spi->dev,
-				"Invalid open circuit current:%u", oc_current);
-			return ERR_PTR(-EINVAL);
+			return dev_errp_probe(dev, -EINVAL,
+					      "Invalid open circuit current:%u",
+					      oc_current);
 		}
 
 		thermo->sensor_config |= LTC2983_THERMOCOUPLE_OC_CHECK(1);
 	}
 	/* validate channel index */
 	if (!(thermo->sensor_config & LTC2983_THERMOCOUPLE_DIFF_MASK) &&
-	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-		dev_err(&st->spi->dev,
-			"Invalid chann:%d for differential thermocouple",
-			sensor->chan);
-		return ERR_PTR(-EINVAL);
-	}
+	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+		return dev_errp_probe(dev, -EINVAL,
+				      "Invalid chann:%d for differential thermocouple",
+				      sensor->chan);
 
 	struct fwnode_handle *ref __free(fwnode_handle) =
 		fwnode_find_reference(child, "adi,cold-junction-handle", 0);
@@ -709,14 +710,13 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
 		ref = NULL;
 	} else {
 		ret = fwnode_property_read_u32(ref, "reg", &thermo->cold_junction_chan);
-		if (ret) {
+		if (ret)
 			/*
 			 * This would be catched later but we can just return
 			 * the error right away.
 			 */
-			dev_err(&st->spi->dev, "Property reg must be given\n");
-			return ERR_PTR(ret);
-		}
+			return dev_errp_probe(dev, ret,
+					      "Property reg must be given\n");
 	}
 
 	/* check custom sensor */
@@ -752,16 +752,14 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 
 	struct fwnode_handle *ref __free(fwnode_handle) =
 		fwnode_find_reference(child, "adi,rsense-handle", 0);
-	if (IS_ERR(ref)) {
-		dev_err(dev, "Property adi,rsense-handle missing or invalid");
-		return ERR_CAST(ref);
-	}
+	if (IS_ERR(ref))
+		return dev_errp_probe(dev, PTR_ERR(ref),
+				      "Property adi,rsense-handle missing or invalid");
 
 	ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
-	if (ret) {
-		dev_err(dev, "Property reg must be given\n");
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		return dev_errp_probe(dev, ret,
+				      "Property reg must be given\n");
 
 	ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
 	if (!ret) {
@@ -780,19 +778,19 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 			rtd->sensor_config = LTC2983_RTD_N_WIRES(3);
 			break;
 		default:
-			dev_err(dev, "Invalid number of wires:%u\n", n_wires);
-			return ERR_PTR(-EINVAL);
+			return dev_errp_probe(dev, -EINVAL,
+					      "Invalid number of wires:%u\n",
+					      n_wires);
 		}
 	}
 
 	if (fwnode_property_read_bool(child, "adi,rsense-share")) {
 		/* Current rotation is only available with rsense sharing */
 		if (fwnode_property_read_bool(child, "adi,current-rotate")) {
-			if (n_wires == 2 || n_wires == 3) {
-				dev_err(dev,
-					"Rotation not allowed for 2/3 Wire RTDs");
-				return ERR_PTR(-EINVAL);
-			}
+			if (n_wires == 2 || n_wires == 3)
+				return dev_errp_probe(dev, -EINVAL,
+						      "Rotation not allowed for 2/3 Wire RTDs");
+
 			rtd->sensor_config |= LTC2983_RTD_C_ROTATE(1);
 		} else {
 			rtd->sensor_config |= LTC2983_RTD_R_SHARE(1);
@@ -815,29 +813,22 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 
 		if (((rtd->sensor_config & LTC2983_RTD_KELVIN_R_SENSE_MASK)
 		     == LTC2983_RTD_KELVIN_R_SENSE_MASK) &&
-		    (rtd->r_sense_chan <=  min)) {
+		    (rtd->r_sense_chan <=  min))
 			/* kelvin rsense*/
-			dev_err(dev,
-				"Invalid rsense chann:%d to use in kelvin rsense",
-				rtd->r_sense_chan);
+			return dev_errp_probe(dev, -EINVAL,
+					      "Invalid rsense chann:%d to use in kelvin rsense",
+					      rtd->r_sense_chan);
 
-			return ERR_PTR(-EINVAL);
-		}
-
-		if (sensor->chan < min || sensor->chan > max) {
-			dev_err(dev, "Invalid chann:%d for the rtd config",
-				sensor->chan);
-
-			return ERR_PTR(-EINVAL);
-		}
+		if (sensor->chan < min || sensor->chan > max)
+			return dev_errp_probe(dev, -EINVAL,
+					      "Invalid chann:%d for the rtd config",
+					      sensor->chan);
 	} else {
 		/* same as differential case */
-		if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-			dev_err(&st->spi->dev,
-				"Invalid chann:%d for RTD", sensor->chan);
-
-			return ERR_PTR(-EINVAL);
-		}
+		if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+			return dev_errp_probe(dev, -EINVAL,
+					      "Invalid chann:%d for RTD",
+					      sensor->chan);
 	}
 
 	/* check custom sensor */
@@ -885,10 +876,9 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 			rtd->excitation_current = 0x08;
 			break;
 		default:
-			dev_err(&st->spi->dev,
-				"Invalid value for excitation current(%u)",
-				excitation_current);
-			return ERR_PTR(-EINVAL);
+			return dev_errp_probe(dev, -EINVAL,
+					      "Invalid value for excitation current(%u)",
+					      excitation_current);
 		}
 	}
 
@@ -912,16 +902,14 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 
 	struct fwnode_handle *ref __free(fwnode_handle) =
 		fwnode_find_reference(child, "adi,rsense-handle", 0);
-	if (IS_ERR(ref)) {
-		dev_err(dev, "Property adi,rsense-handle missing or invalid");
-		return ERR_CAST(ref);
-	}
+	if (IS_ERR(ref))
+		return dev_errp_probe(dev, PTR_ERR(ref),
+				      "Property adi,rsense-handle missing or invalid");
 
 	ret = fwnode_property_read_u32(ref, "reg", &thermistor->r_sense_chan);
-	if (ret) {
-		dev_err(dev, "rsense channel must be configured...\n");
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		return dev_errp_probe(dev, ret,
+				      "rsense channel must be configured...\n");
 
 	if (fwnode_property_read_bool(child, "adi,single-ended")) {
 		thermistor->sensor_config = LTC2983_THERMISTOR_SGL(1);
@@ -936,12 +924,10 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 	}
 	/* validate channel index */
 	if (!(thermistor->sensor_config & LTC2983_THERMISTOR_DIFF_MASK) &&
-	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-		dev_err(&st->spi->dev,
-			"Invalid chann:%d for differential thermistor",
-			sensor->chan);
-		return ERR_PTR(-EINVAL);
-	}
+	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+		return dev_errp_probe(dev, -EINVAL,
+				      "Invalid chann:%d for differential thermistor",
+				      sensor->chan);
 
 	/* check custom sensor */
 	if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART) {
@@ -980,12 +966,10 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 		switch (excitation_current) {
 		case 0:
 			/* auto range */
-			if (sensor->type >=
-			    LTC2983_SENSOR_THERMISTOR_STEINHART) {
-				dev_err(&st->spi->dev,
-					"Auto Range not allowed for custom sensors\n");
-				return ERR_PTR(-EINVAL);
-			}
+			if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART)
+				return dev_errp_probe(dev, -EINVAL,
+						      "Auto Range not allowed for custom sensors\n");
+
 			thermistor->excitation_current = 0x0c;
 			break;
 		case 250:
@@ -1022,10 +1006,9 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 			thermistor->excitation_current = 0x0b;
 			break;
 		default:
-			dev_err(&st->spi->dev,
-				"Invalid value for excitation current(%u)",
-				excitation_current);
-			return ERR_PTR(-EINVAL);
+			return dev_errp_probe(dev, -EINVAL,
+					      "Invalid value for excitation current(%u)",
+					      excitation_current);
 		}
 	}
 
@@ -1036,11 +1019,12 @@ static struct ltc2983_sensor *
 ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *st,
 		  const struct ltc2983_sensor *sensor)
 {
+	struct device *dev = &st->spi->dev;
 	struct ltc2983_diode *diode;
 	u32 temp = 0, excitation_current = 0;
 	int ret;
 
-	diode = devm_kzalloc(&st->spi->dev, sizeof(*diode), GFP_KERNEL);
+	diode = devm_kzalloc(dev, sizeof(*diode), GFP_KERNEL);
 	if (!diode)
 		return ERR_PTR(-ENOMEM);
 
@@ -1055,12 +1039,11 @@ ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *
 
 	/* validate channel index */
 	if (!(diode->sensor_config & LTC2983_DIODE_DIFF_MASK) &&
-	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-		dev_err(&st->spi->dev,
-			"Invalid chann:%d for differential thermistor",
-			sensor->chan);
-		return ERR_PTR(-EINVAL);
-	}
+	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+		return dev_errp_probe(dev, -EINVAL,
+				      "Invalid chann:%d for differential thermistor",
+				      sensor->chan);
+
 	/* set common parameters */
 	diode->sensor.fault_handler = ltc2983_common_fault_handler;
 	diode->sensor.assign_chan = ltc2983_diode_assign_chan;
@@ -1082,10 +1065,9 @@ ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *
 			diode->excitation_current = 0x03;
 			break;
 		default:
-			dev_err(&st->spi->dev,
-				"Invalid value for excitation current(%u)",
-				excitation_current);
-			return ERR_PTR(-EINVAL);
+			return dev_errp_probe(dev, -EINVAL,
+					      "Invalid value for excitation current(%u)",
+					      excitation_current);
 		}
 	}
 
@@ -1101,26 +1083,26 @@ static struct ltc2983_sensor *ltc2983_r_sense_new(struct fwnode_handle *child,
 					struct ltc2983_data *st,
 					const struct ltc2983_sensor *sensor)
 {
+	struct device *dev = &st->spi->dev;
 	struct ltc2983_rsense *rsense;
 	int ret;
 	u32 temp;
 
-	rsense = devm_kzalloc(&st->spi->dev, sizeof(*rsense), GFP_KERNEL);
+	rsense = devm_kzalloc(dev, sizeof(*rsense), GFP_KERNEL);
 	if (!rsense)
 		return ERR_PTR(-ENOMEM);
 
 	/* validate channel index */
-	if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-		dev_err(&st->spi->dev, "Invalid chann:%d for r_sense",
-			sensor->chan);
-		return ERR_PTR(-EINVAL);
-	}
+	if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+		return dev_errp_probe(dev, -EINVAL,
+				      "Invalid chann:%d for r_sense",
+				      sensor->chan);
 
 	ret = fwnode_property_read_u32(child, "adi,rsense-val-milli-ohms", &temp);
-	if (ret) {
-		dev_err(&st->spi->dev, "Property adi,rsense-val-milli-ohms missing\n");
-		return ERR_PTR(-EINVAL);
-	}
+	if (ret)
+		return dev_errp_probe(dev, -EINVAL,
+				      "Property adi,rsense-val-milli-ohms missing\n");
+
 	/*
 	 * Times 1000 because we have milli-ohms and __convert_to_raw
 	 * expects scales of 1000000 which are used for all other
@@ -1139,21 +1121,21 @@ static struct ltc2983_sensor *ltc2983_adc_new(struct fwnode_handle *child,
 					 struct ltc2983_data *st,
 					 const struct ltc2983_sensor *sensor)
 {
+	struct device *dev = &st->spi->dev;
 	struct ltc2983_adc *adc;
 
-	adc = devm_kzalloc(&st->spi->dev, sizeof(*adc), GFP_KERNEL);
+	adc = devm_kzalloc(dev, sizeof(*adc), GFP_KERNEL);
 	if (!adc)
 		return ERR_PTR(-ENOMEM);
 
 	if (fwnode_property_read_bool(child, "adi,single-ended"))
 		adc->single_ended = true;
 
-	if (!adc->single_ended &&
-	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-		dev_err(&st->spi->dev, "Invalid chan:%d for differential adc\n",
-			sensor->chan);
-		return ERR_PTR(-EINVAL);
-	}
+	if (!adc->single_ended && sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+		return dev_errp_probe(dev, -EINVAL,
+				      "Invalid chan:%d for differential adc\n",
+				      sensor->chan);
+
 	/* set common parameters */
 	adc->sensor.assign_chan = ltc2983_adc_assign_chan;
 	adc->sensor.fault_handler = ltc2983_common_fault_handler;
@@ -1165,21 +1147,20 @@ static struct ltc2983_sensor *ltc2983_temp_new(struct fwnode_handle *child,
 					       struct ltc2983_data *st,
 					       const struct ltc2983_sensor *sensor)
 {
+	struct device *dev = &st->spi->dev;
 	struct ltc2983_temp *temp;
 
-	temp = devm_kzalloc(&st->spi->dev, sizeof(*temp), GFP_KERNEL);
+	temp = devm_kzalloc(dev, sizeof(*temp), GFP_KERNEL);
 	if (!temp)
 		return ERR_PTR(-ENOMEM);
 
 	if (fwnode_property_read_bool(child, "adi,single-ended"))
 		temp->single_ended = true;
 
-	if (!temp->single_ended &&
-	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-		dev_err(&st->spi->dev, "Invalid chan:%d for differential temp\n",
-			sensor->chan);
-		return ERR_PTR(-EINVAL);
-	}
+	if (!temp->single_ended && sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
+		return dev_errp_probe(dev, -EINVAL,
+				      "Invalid chan:%d for differential temp\n",
+				      sensor->chan);
 
 	temp->custom = __ltc2983_custom_sensor_new(st, child, "adi,custom-temp",
 						   false, 4096, true);
@@ -1329,10 +1310,9 @@ static int ltc2983_parse_fw(struct ltc2983_data *st)
 	device_property_read_u32(dev, "adi,filter-notch-freq", &st->filter_notch_freq);
 
 	st->num_channels = device_get_child_node_count(dev);
-	if (!st->num_channels) {
-		dev_err(&st->spi->dev, "At least one channel must be given!");
-		return -EINVAL;
-	}
+	if (!st->num_channels)
+		return dev_err_probe(dev, -EINVAL,
+				     "At least one channel must be given!");
 
 	st->sensors = devm_kcalloc(dev, st->num_channels, sizeof(*st->sensors),
 				   GFP_KERNEL);
@@ -1419,6 +1399,7 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
 			      unsigned int wait_time, unsigned int status_reg,
 			      unsigned long status_fail_mask)
 {
+	struct device *dev = &st->spi->dev;
 	unsigned long time;
 	unsigned int val;
 	int ret;
@@ -1437,19 +1418,16 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
 
 	time = wait_for_completion_timeout(&st->completion,
 					   msecs_to_jiffies(wait_time));
-	if (!time) {
-		dev_err(&st->spi->dev, "EEPROM command timed out\n");
-		return -ETIMEDOUT;
-	}
+	if (!time)
+		return dev_err_probe(dev, -ETIMEDOUT, "EEPROM command timed out\n");
 
 	ret = regmap_read(st->regmap, status_reg, &val);
 	if (ret)
 		return ret;
 
-	if (val & status_fail_mask) {
-		dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
-		return -EINVAL;
-	}
+	if (val & status_fail_mask)
+		return dev_err_probe(dev, -EINVAL,
+				     "EEPROM command failed: 0x%02X\n", val);
 
 	return 0;
 }
@@ -1457,16 +1435,15 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
 static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
 {
 	u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0, status;
+	struct device *dev = &st->spi->dev;
 	int ret;
 
 	/* make sure the device is up: start bit (7) is 0 and done bit (6) is 1 */
 	ret = regmap_read_poll_timeout(st->regmap, LTC2983_STATUS_REG, status,
 				       LTC2983_STATUS_UP(status) == 1, 25000,
 				       25000 * 10);
-	if (ret) {
-		dev_err(&st->spi->dev, "Device startup timed out\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Device startup timed out\n");
 
 	ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG,
 				 LTC2983_NOTCH_FREQ_MASK,
@@ -1566,12 +1543,13 @@ static const struct  iio_info ltc2983_iio_info = {
 
 static int ltc2983_probe(struct spi_device *spi)
 {
+	struct device *dev = &spi->dev;
 	struct ltc2983_data *st;
 	struct iio_dev *indio_dev;
 	struct gpio_desc *gpio;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -1582,10 +1560,9 @@ static int ltc2983_probe(struct spi_device *spi)
 		return -ENODEV;
 
 	st->regmap = devm_regmap_init_spi(spi, &ltc2983_regmap_config);
-	if (IS_ERR(st->regmap)) {
-		dev_err(&spi->dev, "Failed to initialize regmap\n");
-		return PTR_ERR(st->regmap);
-	}
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Failed to initialize regmap\n");
 
 	mutex_init(&st->lock);
 	init_completion(&st->completion);
@@ -1597,7 +1574,7 @@ static int ltc2983_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH);
+	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
 
@@ -1607,7 +1584,7 @@ static int ltc2983_probe(struct spi_device *spi)
 		gpiod_set_value_cansleep(gpio, 0);
 	}
 
-	st->iio_chan = devm_kzalloc(&spi->dev,
+	st->iio_chan = devm_kzalloc(dev,
 				    st->iio_channels * sizeof(*st->iio_chan),
 				    GFP_KERNEL);
 	if (!st->iio_chan)
@@ -1617,12 +1594,10 @@ static int ltc2983_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = devm_request_irq(&spi->dev, spi->irq, ltc2983_irq_handler,
+	ret = devm_request_irq(dev, spi->irq, ltc2983_irq_handler,
 			       IRQF_TRIGGER_RISING, st->info->name, st);
-	if (ret) {
-		dev_err(&spi->dev, "failed to request an irq, %d", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to request an irq\n");
 
 	if (st->info->has_eeprom) {
 		ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_WRITE_CMD,
@@ -1639,7 +1614,7 @@ static int ltc2983_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &ltc2983_iio_info;
 
-	return devm_iio_device_register(&spi->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static int ltc2983_resume(struct device *dev)

-- 
2.44.0

[PATCH 3/4] iio: backend: make use of dev_errp_probe()

[Nuno Sa]

Using dev_errp_probe() to simplify the code.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-backend.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 2fea2bbbe47f..e0b08283d667 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -296,11 +296,9 @@ struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
 	}
 
 	fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
-	if (IS_ERR(fwnode)) {
-		dev_err_probe(dev, PTR_ERR(fwnode),
-			      "Cannot get Firmware reference\n");
-		return ERR_CAST(fwnode);
-	}
+	if (IS_ERR(fwnode))
+		return dev_errp_probe(dev, PTR_ERR(fwnode),
+				      "Cannot get Firmware reference\n");
 
 	guard(mutex)(&iio_back_lock);
 	list_for_each_entry(back, &iio_back_list, entry) {

-- 
2.44.0

[PATCH 4/4] iio: common: scmi_iio: convert to dev_err_probe()

[Nuno Sa]

Make use of dev_err_probe() and dev_errp_probe() to simplify error paths
during probe.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/common/scmi_sensors/scmi_iio.c | 45 +++++++++++++-----------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 0c2caf3570db..30d58af02b4c 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -626,12 +626,10 @@ scmi_alloc_iiodev(struct scmi_device *sdev,
 				SCMI_PROTOCOL_SENSOR, SCMI_EVENT_SENSOR_UPDATE,
 				&sensor->sensor_info->id,
 				&sensor->sensor_update_nb);
-	if (ret) {
-		dev_err(&iiodev->dev,
-			"Error in registering sensor update notifier for sensor %s err %d",
-			sensor->sensor_info->name, ret);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		return dev_errp_probe(&iiodev->dev, ret,
+				      "Error in registering sensor update notifier for sensor %s err %d",
+				      sensor->sensor_info->name, ret);
 
 	scmi_iio_set_timestamp_channel(&iio_channels[i], i);
 	iiodev->channels = iio_channels;
@@ -653,10 +651,9 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
 		return -ENODEV;
 
 	sensor_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_SENSOR, &ph);
-	if (IS_ERR(sensor_ops)) {
-		dev_err(dev, "SCMI device has no sensor interface\n");
-		return PTR_ERR(sensor_ops);
-	}
+	if (IS_ERR(sensor_ops))
+		return dev_err_probe(dev, PTR_ERR(sensor_ops),
+				     "SCMI device has no sensor interface\n");
 
 	nr_sensors = sensor_ops->count_get(ph);
 	if (!nr_sensors) {
@@ -667,8 +664,8 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
 	for (i = 0; i < nr_sensors; i++) {
 		sensor_info = sensor_ops->info_get(ph, i);
 		if (!sensor_info) {
-			dev_err(dev, "SCMI sensor %d has missing info\n", i);
-			return -EINVAL;
+			return dev_err_probe(dev, -EINVAL,
+					     "SCMI sensor %d has missing info\n", i);
 		}
 
 		/* This driver only supports 3-axis accel and gyro, skipping other sensors */
@@ -683,29 +680,25 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
 		scmi_iio_dev = scmi_alloc_iiodev(sdev, sensor_ops, ph,
 						 sensor_info);
 		if (IS_ERR(scmi_iio_dev)) {
-			dev_err(dev,
-				"failed to allocate IIO device for sensor %s: %ld\n",
-				sensor_info->name, PTR_ERR(scmi_iio_dev));
-			return PTR_ERR(scmi_iio_dev);
+			return dev_err_probe(dev, PTR_ERR(scmi_iio_dev),
+					     "failed to allocate IIO device for sensor %s: %ld\n",
+					     sensor_info->name, PTR_ERR(scmi_iio_dev));
 		}
 
 		err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
 						  scmi_iio_dev,
 						  &scmi_iio_buffer_ops);
 		if (err < 0) {
-			dev_err(dev,
-				"IIO buffer setup error at sensor %s: %d\n",
-				sensor_info->name, err);
-			return err;
+			return dev_err_probe(dev, err,
+					     "IIO buffer setup error at sensor %s: %d\n",
+					     sensor_info->name, err);
 		}
 
 		err = devm_iio_device_register(dev, scmi_iio_dev);
-		if (err) {
-			dev_err(dev,
-				"IIO device registration failed at sensor %s: %d\n",
-				sensor_info->name, err);
-			return err;
-		}
+		if (err)
+			return dev_err_probe(dev, err,
+					     "IIO device registration failed at sensor %s: %d\n",
+					     sensor_info->name, err);
 	}
 	return err;
 }

-- 
2.44.0

Re: [PATCH 0/4] dev_printk: add dev_errp_probe() helper

[Andy Shevchenko]

+Cc: Andi

On Thu, Apr 04, 2024 at 01:06:22PM +0200, Nuno Sa wrote:
> This series adds a dev_errp_probe() helper. This is similar to
> dev_err_probe() but for cases where an ERR_PTR() is to be returned
> simplifying patterns like:
> 
> 	dev_err_probe(dev, ret, ...);
> 	return ERR_PTR(ret)

What about ERR_CAST() cases?

> The other three patches are adding users for it. The main motivator for
> this were the changes in the commit ("iio: temperature: ltc2983: convert
> to dev_err_probe()"). Initially I just had a local helper [1] but then
> it was suggested to try a new, common helper. As a result, I looked for
> a couple more users.
> 
> I then move into dev_errp_probe() [2] but it was then suggested to separare
> the patch series so we have onde dedicated for the printk helper.
> 
> [1]: https://lore.kernel.org/all/20240301-ltc2983-misc-improv-v3-1-c09516ac0efc@analog.com/
> [2]: https://lore.kernel.org/all/20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com/

Have you seen mine?

20220214143248.502-1-andriy.shevchenko@linux.intel.com

(Note, I'm pretty much fine and thankful that you take care of this)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] iio: temperature: ltc2983: convert to dev_err_probe()

[Andy Shevchenko]

On Thu, Apr 04, 2024 at 01:06:24PM +0200, Nuno Sa wrote:
> Use dev_err_probe() in the probe() path. While at it, made some simple
> improvements:
>  * Declare a struct device *dev helper. This also makes the style more
>    consistent (some places the helper was used and not in other places);
>  * Explicitly included the err.h and errno.h headers;
>  * Removed an useless else if();
>  * Removed some unnecessary line breaks.

...

>  	if (!(thermo->sensor_config & LTC2983_THERMOCOUPLE_DIFF_MASK) &&
> -	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {

It's better if you leave {} when the body goes after a single line.
This applies to your entire series.

> -		dev_err(&st->spi->dev,
> -			"Invalid chann:%d for differential thermocouple",
> -			sensor->chan);
> -		return ERR_PTR(-EINVAL);
> -	}
> +	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
> +		return dev_errp_probe(dev, -EINVAL,
> +				      "Invalid chann:%d for differential thermocouple",
> +				      sensor->chan);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] dev_printk: add dev_errp_probe() helper

[Andy Shevchenko]

On Thu, Apr 04, 2024 at 03:15:35PM +0300, Andy Shevchenko wrote:
> +Cc: Andi
> 
> On Thu, Apr 04, 2024 at 01:06:22PM +0200, Nuno Sa wrote:
> > This series adds a dev_errp_probe() helper. This is similar to
> > dev_err_probe() but for cases where an ERR_PTR() is to be returned
> > simplifying patterns like:
> > 
> > 	dev_err_probe(dev, ret, ...);
> > 	return ERR_PTR(ret)
> 
> What about ERR_CAST() cases?
> 
> > The other three patches are adding users for it. The main motivator for
> > this were the changes in the commit ("iio: temperature: ltc2983: convert
> > to dev_err_probe()"). Initially I just had a local helper [1] but then
> > it was suggested to try a new, common helper. As a result, I looked for
> > a couple more users.
> > 
> > I then move into dev_errp_probe() [2] but it was then suggested to separare
> > the patch series so we have onde dedicated for the printk helper.
> > 
> > [1]: https://lore.kernel.org/all/20240301-ltc2983-misc-improv-v3-1-c09516ac0efc@analog.com/
> > [2]: https://lore.kernel.org/all/20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com/
> 
> Have you seen mine?
> 
> 20220214143248.502-1-andriy.shevchenko@linux.intel.com
> 
> (Note, I'm pretty much fine and thankful that you take care of this)

Also you might be interested to have this
20231201151446.1593472-1-andriy.shevchenko@linux.intel.com

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: backend: make use of dev_errp_probe()

[Andy Shevchenko]

On Thu, Apr 04, 2024 at 01:06:25PM +0200, Nuno Sa wrote:
> Using dev_errp_probe() to simplify the code.

...

> +	if (IS_ERR(fwnode))
> +		return dev_errp_probe(dev, PTR_ERR(fwnode),
> +				      "Cannot get Firmware reference\n");

ERR_CAST() seems quite good candidate to have here.

		return dev_errp_probe(dev, fwnode, "Cannot get Firmware reference\n");

(Assuming dev_errp_probe() magically understands that, note you may have it as
 a macro and distinguish parameter type with _Generic() or so and behave
 differently: ERR_PTR() vs. ERR_CAST(), see acpi_dev_hid_uid_match()
 implementation, but also keep in mind that it doesn't distinguish NULL/0, there
 is a patch available in the mailing list to fix that, though.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: backend: make use of dev_errp_probe()

[Nuno Sá]

On Thu, 2024-04-04 at 15:23 +0300, Andy Shevchenko wrote:
> On Thu, Apr 04, 2024 at 01:06:25PM +0200, Nuno Sa wrote:
> > Using dev_errp_probe() to simplify the code.
> 
> ...
> 
> > +	if (IS_ERR(fwnode))
> > +		return dev_errp_probe(dev, PTR_ERR(fwnode),
> > +				      "Cannot get Firmware reference\n");
> 
> ERR_CAST() seems quite good candidate to have here.
> 
> 		return dev_errp_probe(dev, fwnode, "Cannot get Firmware
> reference\n");
> 
> (Assuming dev_errp_probe() magically understands that, note you may have it as
>  a macro and distinguish parameter type with _Generic() or so and behave
>  differently: ERR_PTR() vs. ERR_CAST(), see acpi_dev_hid_uid_match()
>  implementation, but also keep in mind that it doesn't distinguish NULL/0,
> there
>  is a patch available in the mailing list to fix that, though.)
> 

Do we care that much for going with that trouble? I understand like this we go
PTR_ERR() to then comeback to ERR_PTR() but this for probe() which is not a
fastpath. So perhaps we could just keep it simple?

- Nuno Sá 

Re: [PATCH 0/4] dev_printk: add dev_errp_probe() helper

[Nuno Sá]

On Thu, 2024-04-04 at 15:15 +0300, Andy Shevchenko wrote:
> +Cc: Andi
> 
> On Thu, Apr 04, 2024 at 01:06:22PM +0200, Nuno Sa wrote:
> > This series adds a dev_errp_probe() helper. This is similar to
> > dev_err_probe() but for cases where an ERR_PTR() is to be returned
> > simplifying patterns like:
> > 
> > 	dev_err_probe(dev, ret, ...);
> > 	return ERR_PTR(ret)
> 
> What about ERR_CAST() cases?

In theory not really needed (I think) but see my reply to the other patch.
> 
> > The other three patches are adding users for it. The main motivator for
> > this were the changes in the commit ("iio: temperature: ltc2983: convert
> > to dev_err_probe()"). Initially I just had a local helper [1] but then
> > it was suggested to try a new, common helper. As a result, I looked for
> > a couple more users.
> > 
> > I then move into dev_errp_probe() [2] but it was then suggested to separare
> > the patch series so we have onde dedicated for the printk helper.
> > 
> > [1]:
> > https://lore.kernel.org/all/20240301-ltc2983-misc-improv-v3-1-c09516ac0efc@analog.com/
> > [2]:
> > https://lore.kernel.org/all/20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com/
> 
> Have you seen mine?
> 
> 20220214143248.502-1-andriy.shevchenko@linux.intel.com
> 
> (Note, I'm pretty much fine and thankful that you take care of this)
> 

Ups nope... I very prefer your naming :). I can take care of this only if you
don't intend to continue with your series. You sent it first so... :)

- Nuno Sá

Re: [PATCH 3/4] iio: backend: make use of dev_errp_probe()

[Andy Shevchenko]

On Thu, Apr 04, 2024 at 04:58:27PM +0200, Nuno Sá wrote:
> On Thu, 2024-04-04 at 15:23 +0300, Andy Shevchenko wrote:
> > On Thu, Apr 04, 2024 at 01:06:25PM +0200, Nuno Sa wrote:
> > > Using dev_errp_probe() to simplify the code.

...

> > > +	if (IS_ERR(fwnode))
> > > +		return dev_errp_probe(dev, PTR_ERR(fwnode),
> > > +				      "Cannot get Firmware reference\n");
> > 
> > ERR_CAST() seems quite good candidate to have here.
> > 
> > 		return dev_errp_probe(dev, fwnode, "Cannot get Firmware
> > reference\n");
> > 
> > (Assuming dev_errp_probe() magically understands that, note you may have it as
> >  a macro and distinguish parameter type with _Generic() or so and behave
> >  differently: ERR_PTR() vs. ERR_CAST(), see acpi_dev_hid_uid_match()
> >  implementation, but also keep in mind that it doesn't distinguish NULL/0,
> > there
> >  is a patch available in the mailing list to fix that, though.)
> 
> Do we care that much for going with that trouble?

I don't think we do. We are not supposed to be called with ret == 0/NULL.
That's why I pointed out to the current version.

> I understand like this we go
> PTR_ERR() to then comeback to ERR_PTR() but this for probe() which is not a
> fastpath. So perhaps we could just keep it simple?

It's not about performance, it's about readability. See the difference between
yours and mine.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] dev_printk: add dev_errp_probe() helper

[Andy Shevchenko]

On Thu, Apr 04, 2024 at 05:03:53PM +0200, Nuno Sá wrote:
> On Thu, 2024-04-04 at 15:15 +0300, Andy Shevchenko wrote:
> > On Thu, Apr 04, 2024 at 01:06:22PM +0200, Nuno Sa wrote:
> > > This series adds a dev_errp_probe() helper. This is similar to
> > > dev_err_probe() but for cases where an ERR_PTR() is to be returned
> > > simplifying patterns like:
> > > 
> > > 	dev_err_probe(dev, ret, ...);
> > > 	return ERR_PTR(ret)
> > 
> > What about ERR_CAST() cases?
> 
> In theory not really needed (I think) but see my reply to the other patch.
> > 
> > > The other three patches are adding users for it. The main motivator for
> > > this were the changes in the commit ("iio: temperature: ltc2983: convert
> > > to dev_err_probe()"). Initially I just had a local helper [1] but then
> > > it was suggested to try a new, common helper. As a result, I looked for
> > > a couple more users.
> > > 
> > > I then move into dev_errp_probe() [2] but it was then suggested to separare
> > > the patch series so we have onde dedicated for the printk helper.
> > > 
> > > [1]:
> > > https://lore.kernel.org/all/20240301-ltc2983-misc-improv-v3-1-c09516ac0efc@analog.com/
> > > [2]:
> > > https://lore.kernel.org/all/20240328-ltc2983-misc-improv-v4-0-0cc428c07cd5@analog.com/
> > 
> > Have you seen mine?
> > 
> > 20220214143248.502-1-andriy.shevchenko@linux.intel.com
> > 
> > (Note, I'm pretty much fine and thankful that you take care of this)
> > 
> 
> Ups nope... I very prefer your naming :). I can take care of this only if you
> don't intend to continue with your series. You sent it first so... :)

No objections, I have no time and that already was rotting for 2+ years.
And pls keep Andi in the circle, he wanted this change to happen.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: backend: make use of dev_errp_probe()

[Jonathan Cameron]

On Thu, 4 Apr 2024 18:12:25 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Apr 04, 2024 at 04:58:27PM +0200, Nuno Sá wrote:
> > On Thu, 2024-04-04 at 15:23 +0300, Andy Shevchenko wrote:  
> > > On Thu, Apr 04, 2024 at 01:06:25PM +0200, Nuno Sa wrote:  
> > > > Using dev_errp_probe() to simplify the code.  
> 
> ...
> 
> > > > +	if (IS_ERR(fwnode))
> > > > +		return dev_errp_probe(dev, PTR_ERR(fwnode),
> > > > +				      "Cannot get Firmware reference\n");  
> > > 
> > > ERR_CAST() seems quite good candidate to have here.
> > > 
> > > 		return dev_errp_probe(dev, fwnode, "Cannot get Firmware
> > > reference\n");
> > > 
> > > (Assuming dev_errp_probe() magically understands that, note you may have it as
> > >  a macro and distinguish parameter type with _Generic() or so and behave
> > >  differently: ERR_PTR() vs. ERR_CAST(), see acpi_dev_hid_uid_match()
> > >  implementation, but also keep in mind that it doesn't distinguish NULL/0,
> > > there
> > >  is a patch available in the mailing list to fix that, though.)  
> > 
> > Do we care that much for going with that trouble?  
> 
> I don't think we do. We are not supposed to be called with ret == 0/NULL.
> That's why I pointed out to the current version.
> 
> > I understand like this we go
> > PTR_ERR() to then comeback to ERR_PTR() but this for probe() which is not a
> > fastpath. So perhaps we could just keep it simple?  
> 
> It's not about performance, it's about readability. See the difference between
> yours and mine.
> 

You are suggesting making it transparently take an error ptr or an integer?
Whilst clever, I'm not seeing that as a good idea for readability / reviewability.
I expect something that looks like a function to take the same parameters (other vargs)
always.  _Generic messes with that.

Maybe I just don't like to learn new things!  If consensus comes down in favour
of _Generic trickery then I'll get used to it eventually.

Jonathan

Re: [PATCH 1/4] dev_printk: add new dev_errp_probe() helper

[Andi Shyti]

Hi Nuno,

...

> +/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
> +#define dev_errp_probe(dev, ___err, fmt, ...)	({		\
> +	ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__));	\
> +})

I have a whole series adding a set of error oriente printk's. But
for the time being this looks OK.

I just don't like the name, the 'p' is an important detail, but
a bit hidden... how about dev_err_ptr_probe(...)?

Andi

Re: [PATCH 2/4] iio: temperature: ltc2983: convert to dev_err_probe()

[Andi Shyti]

Hi Andy,

On Thu, Apr 04, 2024 at 03:18:05PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 04, 2024 at 01:06:24PM +0200, Nuno Sa wrote:
> > Use dev_err_probe() in the probe() path. While at it, made some simple
> > improvements:
> >  * Declare a struct device *dev helper. This also makes the style more
> >    consistent (some places the helper was used and not in other places);
> >  * Explicitly included the err.h and errno.h headers;
> >  * Removed an useless else if();
> >  * Removed some unnecessary line breaks.
> 
> ...
> 
> >  	if (!(thermo->sensor_config & LTC2983_THERMOCOUPLE_DIFF_MASK) &&
> > -	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
> 
> It's better if you leave {} when the body goes after a single line.
> This applies to your entire series.

I think checkpatch complains if you leave the {...} and,
honestly, I'm not a big fan of the {...}. Unless there is a last
minute rule I missed.

If checkpatch doesn't complain, I'm OK with both ways, though.

Andi

Re: [PATCH 3/4] iio: backend: make use of dev_errp_probe()

[Andi Shyti]

Hi,

On Sat, Apr 06, 2024 at 05:07:17PM +0100, Jonathan Cameron wrote:
> On Thu, 4 Apr 2024 18:12:25 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Thu, Apr 04, 2024 at 04:58:27PM +0200, Nuno Sá wrote:
> > > On Thu, 2024-04-04 at 15:23 +0300, Andy Shevchenko wrote:  
> > > > On Thu, Apr 04, 2024 at 01:06:25PM +0200, Nuno Sa wrote:  
> > > > > Using dev_errp_probe() to simplify the code.  
> > 
> > ...
> > 
> > > > > +	if (IS_ERR(fwnode))
> > > > > +		return dev_errp_probe(dev, PTR_ERR(fwnode),
> > > > > +				      "Cannot get Firmware reference\n");  
> > > > 
> > > > ERR_CAST() seems quite good candidate to have here.
> > > > 
> > > > 		return dev_errp_probe(dev, fwnode, "Cannot get Firmware
> > > > reference\n");
> > > > 
> > > > (Assuming dev_errp_probe() magically understands that, note you may have it as
> > > >  a macro and distinguish parameter type with _Generic() or so and behave
> > > >  differently: ERR_PTR() vs. ERR_CAST(), see acpi_dev_hid_uid_match()
> > > >  implementation, but also keep in mind that it doesn't distinguish NULL/0,
> > > > there
> > > >  is a patch available in the mailing list to fix that, though.)  
> > > 
> > > Do we care that much for going with that trouble?  
> > 
> > I don't think we do. We are not supposed to be called with ret == 0/NULL.
> > That's why I pointed out to the current version.
> > 
> > > I understand like this we go
> > > PTR_ERR() to then comeback to ERR_PTR() but this for probe() which is not a
> > > fastpath. So perhaps we could just keep it simple?  
> > 
> > It's not about performance, it's about readability. See the difference between
> > yours and mine.
> > 
> 
> You are suggesting making it transparently take an error ptr or an integer?
> Whilst clever, I'm not seeing that as a good idea for readability / reviewability.
> I expect something that looks like a function to take the same parameters (other vargs)
> always.  _Generic messes with that.
> 
> Maybe I just don't like to learn new things!  If consensus comes down in favour
> of _Generic trickery then I'll get used to it eventually.

the whole point of the dev_err_...() functions is to add trickery
in order to reduce code and brackets.

The way I see this is to have a combination of functions:

 - takes integer, returns integer -> dev_err_probe()
 - takes integer, returns pointer -> dev_errp_probe() (or dev_err_ptr_probe())
 - takes pointer, return integer -> ? dev_ptr_err_probe()
 - takes pointer, returns pointer -> ? dev_ptr_probe()

Thoughts?

Andi

Re: [PATCH 1/4] dev_printk: add new dev_errp_probe() helper

[Nuno Sá]

On Sat, 2024-04-06 at 20:35 +0200, Andi Shyti wrote:
> Hi Nuno,
> 
> ...
> 
> > +/* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
> > +#define dev_errp_probe(dev, ___err, fmt, ...)	({		\
> > +	ERR_PTR(dev_err_probe(dev, ___err, fmt, ##__VA_ARGS__));	\
> > +})
> 
> I have a whole series adding a set of error oriente printk's. But
> for the time being this looks OK.
> 
> I just don't like the name, the 'p' is an important detail, but
> a bit hidden... how about dev_err_ptr_probe(...)?
> 

Agreed, not a very good name indeed.

- Nuno Sá

Re: [PATCH 3/4] iio: backend: make use of dev_errp_probe()

[Nuno Sá]

On Sat, 2024-04-06 at 17:07 +0100, Jonathan Cameron wrote:
> On Thu, 4 Apr 2024 18:12:25 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Thu, Apr 04, 2024 at 04:58:27PM +0200, Nuno Sá wrote:
> > > On Thu, 2024-04-04 at 15:23 +0300, Andy Shevchenko wrote:  
> > > > On Thu, Apr 04, 2024 at 01:06:25PM +0200, Nuno Sa wrote:  
> > > > > Using dev_errp_probe() to simplify the code.  
> > 
> > ...
> > 
> > > > > +	if (IS_ERR(fwnode))
> > > > > +		return dev_errp_probe(dev, PTR_ERR(fwnode),
> > > > > +				      "Cannot get Firmware
> > > > > reference\n");  
> > > > 
> > > > ERR_CAST() seems quite good candidate to have here.
> > > > 
> > > > 		return dev_errp_probe(dev, fwnode, "Cannot get Firmware
> > > > reference\n");
> > > > 
> > > > (Assuming dev_errp_probe() magically understands that, note you may have
> > > > it as
> > > >  a macro and distinguish parameter type with _Generic() or so and behave
> > > >  differently: ERR_PTR() vs. ERR_CAST(), see acpi_dev_hid_uid_match()
> > > >  implementation, but also keep in mind that it doesn't distinguish
> > > > NULL/0,
> > > > there
> > > >  is a patch available in the mailing list to fix that, though.)  
> > > 
> > > Do we care that much for going with that trouble?  
> > 
> > I don't think we do. We are not supposed to be called with ret == 0/NULL.
> > That's why I pointed out to the current version.
> > 
> > > I understand like this we go
> > > PTR_ERR() to then comeback to ERR_PTR() but this for probe() which is not
> > > a
> > > fastpath. So perhaps we could just keep it simple?  
> > 
> > It's not about performance, it's about readability. See the difference
> > between
> > yours and mine.
> > 
> 
> You are suggesting making it transparently take an error ptr or an integer?
> Whilst clever, I'm not seeing that as a good idea for readability /
> reviewability.
> I expect something that looks like a function to take the same parameters
> (other vargs)
> always.  _Generic messes with that.

> Maybe I just don't like to learn new things!  If consensus comes down in
> favour
> of _Generic trickery then I'll get used to it eventually.
> 

Yeah, I agree with the above. Not fully convinced but for the ERR_CAST() case I
would very much prefer to have another explicit helper rather than hiding stuff
in the same macro.

- Nuno Sá

Re: [PATCH 3/4] iio: backend: make use of dev_errp_probe()

[Nuno Sá]

On Sat, 2024-04-06 at 20:54 +0200, Andi Shyti wrote:
> Hi,
> 
> On Sat, Apr 06, 2024 at 05:07:17PM +0100, Jonathan Cameron wrote:
> > On Thu, 4 Apr 2024 18:12:25 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > On Thu, Apr 04, 2024 at 04:58:27PM +0200, Nuno Sá wrote:
> > > > On Thu, 2024-04-04 at 15:23 +0300, Andy Shevchenko wrote:  
> > > > > On Thu, Apr 04, 2024 at 01:06:25PM +0200, Nuno Sa wrote:  
> > > > > > Using dev_errp_probe() to simplify the code.  
> > > 
> > > ...
> > > 
> > > > > > +	if (IS_ERR(fwnode))
> > > > > > +		return dev_errp_probe(dev, PTR_ERR(fwnode),
> > > > > > +				      "Cannot get Firmware
> > > > > > reference\n");  
> > > > > 
> > > > > ERR_CAST() seems quite good candidate to have here.
> > > > > 
> > > > > 		return dev_errp_probe(dev, fwnode, "Cannot get
> > > > > Firmware
> > > > > reference\n");
> > > > > 
> > > > > (Assuming dev_errp_probe() magically understands that, note you may
> > > > > have it as
> > > > >  a macro and distinguish parameter type with _Generic() or so and
> > > > > behave
> > > > >  differently: ERR_PTR() vs. ERR_CAST(), see acpi_dev_hid_uid_match()
> > > > >  implementation, but also keep in mind that it doesn't distinguish
> > > > > NULL/0,
> > > > > there
> > > > >  is a patch available in the mailing list to fix that, though.)  
> > > > 
> > > > Do we care that much for going with that trouble?  
> > > 
> > > I don't think we do. We are not supposed to be called with ret == 0/NULL.
> > > That's why I pointed out to the current version.
> > > 
> > > > I understand like this we go
> > > > PTR_ERR() to then comeback to ERR_PTR() but this for probe() which is
> > > > not a
> > > > fastpath. So perhaps we could just keep it simple?  
> > > 
> > > It's not about performance, it's about readability. See the difference
> > > between
> > > yours and mine.
> > > 
> > 
> > You are suggesting making it transparently take an error ptr or an integer?
> > Whilst clever, I'm not seeing that as a good idea for readability /
> > reviewability.
> > I expect something that looks like a function to take the same parameters
> > (other vargs)
> > always.  _Generic messes with that.
> > 
> > Maybe I just don't like to learn new things!  If consensus comes down in
> > favour
> > of _Generic trickery then I'll get used to it eventually.
> 
> the whole point of the dev_err_...() functions is to add trickery
> in order to reduce code and brackets.
> 

I'm not sure I'm completely convinced on having more helpers but also no strong
opinion tbh. But see below...

> The way I see this is to have a combination of functions:
> 
>  - takes integer, returns integer -> dev_err_probe()
>  - takes integer, returns pointer -> dev_errp_probe() (or dev_err_ptr_probe())
>  - takes pointer, return integer -> ? dev_ptr_err_probe()

This is pretty much all the dev_err_probe(dev, PTR_ERR(), ...) we already have
out there. Do we really want to have this variant?

>  - takes pointer, returns pointer -> ? dev_ptr_probe()

dev_ptr_probe() misses to be clear about being an error and think this is pretty
much the ERR_CAST() case right? Maybe dev_err_cast_ptr_probe()? Or
dev_err_cast_probe()?

- Nuno Sá