[PATCH 0/6] iio: temperature: ltc2983: small improvements

[PATCH 0/6] iio: temperature: ltc2983: small improvements

[Nuno Sa]

This series adds small improvements to the driver. There's no functional
change intended for it.

Jonathan, this will conflict with your series on the cleanup stuff for
fwnodes. If you want, I can rebase on top of that series.

---
Nuno Sa (6):
      iio: temperature: ltc2983: make use of spi_get_device_match_data()
      iio: temperature: ltc2983: rename ltc2983_parse_dt()
      iio: temperature: ltc2983: convert to dev_err_probe()
      iio: temperature: ltc2983: explicitly set the name in chip_info
      dt-bindings: iio: temperature: ltc2983: document power supply
      iio: temperature: ltc2983: support vdd regulator

 .../bindings/iio/temperature/adi,ltc2983.yaml      |   2 +
 drivers/iio/temperature/ltc2983.c                  | 223 +++++++++++----------
 2 files changed, 123 insertions(+), 102 deletions(-)
---
base-commit: 3cc5ebd3a2d6247aeba81873d6b040d5d87f7db1
change-id: 20240222-ltc2983-misc-improv-1c7a78ece93f
--

Thanks!
- Nuno Sá

[PATCH 1/6] iio: temperature: ltc2983: make use of spi_get_device_match_data()

[Nuno Sa]

Use spi_get_device_match_data() as it simplifies the code. No functional
change intended...

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

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index fcb96c44d954..acc631857e27 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -1614,9 +1614,7 @@ static int ltc2983_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
-	st->info = device_get_match_data(&spi->dev);
-	if (!st->info)
-		st->info = (void *)spi_get_device_id(spi)->driver_data;
+	st->info = spi_get_device_match_data(spi);
 	if (!st->info)
 		return -ENODEV;
 

-- 
2.43.2

Re: [PATCH 1/6] iio: temperature: ltc2983: make use of spi_get_device_match_data()

[Jonathan Cameron]

On Thu, 22 Feb 2024 13:55:52 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> Use spi_get_device_match_data() as it simplifies the code. No functional
> change intended...
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
I'm a sucker for cleanup patches.

Applied 
> ---
>  drivers/iio/temperature/ltc2983.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> index fcb96c44d954..acc631857e27 100644
> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
> @@ -1614,9 +1614,7 @@ static int ltc2983_probe(struct spi_device *spi)
>  
>  	st = iio_priv(indio_dev);
>  
> -	st->info = device_get_match_data(&spi->dev);
> -	if (!st->info)
> -		st->info = (void *)spi_get_device_id(spi)->driver_data;
> +	st->info = spi_get_device_match_data(spi);
>  	if (!st->info)
>  		return -ENODEV;
>  
> 

[PATCH 2/6] iio: temperature: ltc2983: rename ltc2983_parse_dt()

[Nuno Sa]

Rename ltc2983_parse_dt() to ltc2983_parse_fw() as there's no explicit
dependency on devicetree. No functional change intended...

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

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index acc631857e27..23f2d43fc040 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -1346,7 +1346,7 @@ static irqreturn_t ltc2983_irq_handler(int irq, void *data)
 	__chan; \
 })
 
-static int ltc2983_parse_dt(struct ltc2983_data *st)
+static int ltc2983_parse_fw(struct ltc2983_data *st)
 {
 	struct device *dev = &st->spi->dev;
 	struct fwnode_handle *child;
@@ -1630,7 +1630,7 @@ static int ltc2983_probe(struct spi_device *spi)
 	st->eeprom_key = cpu_to_be32(LTC2983_EEPROM_KEY);
 	spi_set_drvdata(spi, st);
 
-	ret = ltc2983_parse_dt(st);
+	ret = ltc2983_parse_fw(st);
 	if (ret)
 		return ret;
 

-- 
2.43.2

Re: [PATCH 2/6] iio: temperature: ltc2983: rename ltc2983_parse_dt()

[Jonathan Cameron]

On Thu, 22 Feb 2024 13:55:53 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> Rename ltc2983_parse_dt() to ltc2983_parse_fw() as there's no explicit
> dependency on devicetree. No functional change intended...
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
It thought the same whilst doing the cleanup.h series but punted it for another
day.  Seems that day has come. Applied.

> ---
>  drivers/iio/temperature/ltc2983.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> index acc631857e27..23f2d43fc040 100644
> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
> @@ -1346,7 +1346,7 @@ static irqreturn_t ltc2983_irq_handler(int irq, void *data)
>  	__chan; \
>  })
>  
> -static int ltc2983_parse_dt(struct ltc2983_data *st)
> +static int ltc2983_parse_fw(struct ltc2983_data *st)
>  {
>  	struct device *dev = &st->spi->dev;
>  	struct fwnode_handle *child;
> @@ -1630,7 +1630,7 @@ static int ltc2983_probe(struct spi_device *spi)
>  	st->eeprom_key = cpu_to_be32(LTC2983_EEPROM_KEY);
>  	spi_set_drvdata(spi, st);
>  
> -	ret = ltc2983_parse_dt(st);
> +	ret = ltc2983_parse_fw(st);
>  	if (ret)
>  		return ret;
>  
> 

[PATCH 3/6] 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().

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

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 23f2d43fc040..4b096aa3fbd8 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>
@@ -656,11 +658,12 @@ 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;
 	struct fwnode_handle *ref;
 	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,8 +690,9 @@ 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);
+			dev_err_probe(dev, -EINVAL,
+				      "Invalid open circuit current:%u",
+				      oc_current);
 			return ERR_PTR(-EINVAL);
 		}
 
@@ -697,9 +701,9 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
 	/* 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);
+		dev_err_probe(dev, -EINVAL,
+			      "Invalid chann:%d for differential thermocouple",
+			      sensor->chan);
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -713,7 +717,7 @@ ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data
 			 * 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");
+			dev_err_probe(dev, ret, "Property reg must be given\n");
 			goto fail;
 		}
 	}
@@ -759,13 +763,14 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 
 	ref = fwnode_find_reference(child, "adi,rsense-handle", 0);
 	if (IS_ERR(ref)) {
-		dev_err(dev, "Property adi,rsense-handle missing or invalid");
+		dev_err_probe(dev, PTR_ERR(ref),
+			      "Property adi,rsense-handle missing or invalid");
 		return ERR_CAST(ref);
 	}
 
 	ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
 	if (ret) {
-		dev_err(dev, "Property reg must be given\n");
+		dev_err_probe(dev, ret, "Property reg must be given\n");
 		goto fail;
 	}
 
@@ -786,8 +791,9 @@ 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);
-			ret = -EINVAL;
+			ret = dev_err_probe(dev, -EINVAL,
+					    "Invalid number of wires:%u\n",
+					    n_wires);
 			goto fail;
 		}
 	}
@@ -796,9 +802,8 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 		/* 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");
-				ret = -EINVAL;
+				ret = dev_err_probe(dev, -EINVAL,
+						    "Rotation not allowed for 2/3 Wire RTDs");
 				goto fail;
 			}
 			rtd->sensor_config |= LTC2983_RTD_C_ROTATE(1);
@@ -825,28 +830,24 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 		     == LTC2983_RTD_KELVIN_R_SENSE_MASK) &&
 		    (rtd->r_sense_chan <=  min)) {
 			/* kelvin rsense*/
-			dev_err(dev,
-				"Invalid rsense chann:%d to use in kelvin rsense",
-				rtd->r_sense_chan);
-
-			ret = -EINVAL;
+			ret = dev_err_probe(dev, -EINVAL,
+					    "Invalid rsense chann:%d to use in kelvin rsense",
+					    rtd->r_sense_chan);
 			goto fail;
 		}
 
 		if (sensor->chan < min || sensor->chan > max) {
-			dev_err(dev, "Invalid chann:%d for the rtd config",
-				sensor->chan);
-
-			ret = -EINVAL;
+			ret = dev_err_probe(dev, -EINVAL,
+					    "Invalid chann:%d for the rtd config",
+					    sensor->chan);
 			goto fail;
 		}
 	} else {
 		/* same as differential case */
 		if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-			dev_err(&st->spi->dev,
-				"Invalid chann:%d for RTD", sensor->chan);
-
-			ret = -EINVAL;
+			ret = dev_err_probe(dev, -EINVAL,
+					    "Invalid chann:%d for RTD",
+					    sensor->chan);
 			goto fail;
 		}
 	}
@@ -898,10 +899,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);
-			ret = -EINVAL;
+			ret = dev_err_probe(dev, -EINVAL,
+					    "Invalid value for excitation current(%u)",
+					    excitation_current);
 			goto fail;
 		}
 	}
@@ -931,13 +931,15 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 
 	ref = fwnode_find_reference(child, "adi,rsense-handle", 0);
 	if (IS_ERR(ref)) {
-		dev_err(dev, "Property adi,rsense-handle missing or invalid");
+		dev_err_probe(dev, PTR_ERR(ref),
+			      "Property adi,rsense-handle missing or invalid");
 		return ERR_CAST(ref);
 	}
 
 	ret = fwnode_property_read_u32(ref, "reg", &thermistor->r_sense_chan);
 	if (ret) {
-		dev_err(dev, "rsense channel must be configured...\n");
+		dev_err_probe(dev, ret,
+			      "rsense channel must be configured...\n");
 		goto fail;
 	}
 
@@ -955,10 +957,9 @@ 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);
-		ret = -EINVAL;
+		ret = dev_err_probe(dev, -EINVAL,
+				    "Invalid chann:%d for differential thermistor",
+				    sensor->chan);
 		goto fail;
 	}
 
@@ -1003,9 +1004,8 @@ ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *s
 			/* auto range */
 			if (sensor->type >=
 			    LTC2983_SENSOR_THERMISTOR_STEINHART) {
-				dev_err(&st->spi->dev,
-					"Auto Range not allowed for custom sensors\n");
-				ret = -EINVAL;
+				ret = dev_err_probe(dev, -EINVAL,
+						    "Auto Range not allowed for custom sensors\n");
 				goto fail;
 			}
 			thermistor->excitation_current = 0x0c;
@@ -1044,10 +1044,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);
-			ret = -EINVAL;
+			ret = dev_err_probe(dev, -EINVAL,
+					    "Invalid value for excitation current(%u)",
+					    excitation_current);
 			goto fail;
 		}
 	}
@@ -1063,11 +1062,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);
 
@@ -1083,9 +1083,9 @@ 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);
+		dev_err_probe(dev, -EINVAL,
+			      "Invalid chann:%d for differential thermistor",
+			      sensor->chan);
 		return ERR_PTR(-EINVAL);
 	}
 	/* set common parameters */
@@ -1109,9 +1109,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);
+			dev_err_probe(dev, -EINVAL,
+				      "Invalid value for excitation current(%u)",
+				      excitation_current);
 			return ERR_PTR(-EINVAL);
 		}
 	}
@@ -1128,24 +1128,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);
+		dev_err_probe(dev, -EINVAL, "Invalid chann:%d for r_sense",
+			      sensor->chan);
 		return ERR_PTR(-EINVAL);
 	}
 
 	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");
+		dev_err_probe(dev, -EINVAL,
+			      "Property adi,rsense-val-milli-ohms missing\n");
 		return ERR_PTR(-EINVAL);
 	}
 	/*
@@ -1166,9 +1168,10 @@ 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);
 
@@ -1177,8 +1180,9 @@ static struct ltc2983_sensor *ltc2983_adc_new(struct fwnode_handle *child,
 
 	if (!adc->single_ended &&
 	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-		dev_err(&st->spi->dev, "Invalid chan:%d for differential adc\n",
-			sensor->chan);
+		dev_err_probe(dev, -EINVAL,
+			      "Invalid chan:%d for differential adc\n",
+			      sensor->chan);
 		return ERR_PTR(-EINVAL);
 	}
 	/* set common parameters */
@@ -1192,9 +1196,10 @@ 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);
 
@@ -1203,8 +1208,8 @@ static struct ltc2983_sensor *ltc2983_temp_new(struct fwnode_handle *child,
 
 	if (!temp->single_ended &&
 	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
-		dev_err(&st->spi->dev, "Invalid chan:%d for differential temp\n",
-			sensor->chan);
+		dev_err_probe(dev, -EINVAL, "Invalid chan:%d for differential temp\n",
+			      sensor->chan);
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -1357,10 +1362,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);
@@ -1373,27 +1377,31 @@ static int ltc2983_parse_fw(struct ltc2983_data *st)
 
 		ret = fwnode_property_read_u32(child, "reg", &sensor.chan);
 		if (ret) {
-			dev_err(dev, "reg property must given for child nodes\n");
+			dev_err_probe(dev, ret,
+				      "reg property must given for child nodes\n");
 			goto put_child;
 		}
 
 		/* check if we have a valid channel */
 		if (sensor.chan < LTC2983_MIN_CHANNELS_NR ||
 		    sensor.chan > st->info->max_channels_nr) {
-			ret = -EINVAL;
-			dev_err(dev, "chan:%d must be from %u to %u\n", sensor.chan,
-				LTC2983_MIN_CHANNELS_NR, st->info->max_channels_nr);
+			ret = dev_err_probe(dev, -EINVAL,
+					    "chan:%d must be from %u to %u\n",
+					    sensor.chan, LTC2983_MIN_CHANNELS_NR,
+					    st->info->max_channels_nr);
 			goto put_child;
-		} else if (channel_avail_mask & BIT(sensor.chan)) {
-			ret = -EINVAL;
-			dev_err(dev, "chan:%d already in use\n", sensor.chan);
+		}
+
+		if (channel_avail_mask & BIT(sensor.chan)) {
+			ret = dev_err_probe(dev, -EINVAL, "chan:%d already in use\n",
+					    sensor.chan);
 			goto put_child;
 		}
 
 		ret = fwnode_property_read_u32(child, "adi,sensor-type", &sensor.type);
 		if (ret) {
-			dev_err(dev,
-				"adi,sensor-type property must given for child nodes\n");
+			dev_err_probe(dev, ret,
+				      "adi,sensor-type property must given for child nodes\n");
 			goto put_child;
 		}
 
@@ -1426,15 +1434,14 @@ static int ltc2983_parse_fw(struct ltc2983_data *st)
 			   sensor.type == LTC2983_SENSOR_ACTIVE_TEMP) {
 			st->sensors[chan] = ltc2983_temp_new(child, st, &sensor);
 		} else {
-			dev_err(dev, "Unknown sensor type %d\n", sensor.type);
-			ret = -EINVAL;
+			ret = dev_err_probe(dev, -EINVAL, "Unknown sensor type %d\n",
+					    sensor.type);
 			goto put_child;
 		}
 
 		if (IS_ERR(st->sensors[chan])) {
-			dev_err(dev, "Failed to create sensor %ld",
-				PTR_ERR(st->sensors[chan]));
-			ret = PTR_ERR(st->sensors[chan]);
+			ret = dev_err_probe(dev, PTR_ERR(st->sensors[chan]),
+					    "Failed to create sensor\n");
 			goto put_child;
 		}
 		/* set generic sensor parameters */
@@ -1602,13 +1609,14 @@ 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;
 	const char *name = spi_get_device_id(spi)->name;
 	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;
 
@@ -1619,10 +1627,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);
@@ -1634,7 +1641,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);
 
@@ -1644,7 +1651,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)
@@ -1654,12 +1661,11 @@ 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, 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, %d",
+				     ret);
 
 	if (st->info->has_eeprom) {
 		ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_WRITE_CMD,
@@ -1676,7 +1682,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.43.2

Re: [PATCH 3/6] iio: temperature: ltc2983: convert to dev_err_probe()

[Jonathan Cameron]

On Thu, 22 Feb 2024 13:55:54 +0100
Nuno Sa <nuno.sa@analog.com> 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().
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

Hmm. Up to you whether you rebase on top of the device_for_each_child_node_scoped()
patch - mostly depends if you give the new version a reviewed by or not!

If they land in the other order I can fix it up whilst applying.
After that series is in place though the number of places this will do

	dev_err_probe(dev, ret, "message\n");
	return ERR_PTR(ret);

Makes me wonder whether
	return ERR_PTR(dev_err_probe(dev, ret, "message\n")); is
too ugly or not?

Maybe we need a dev_err_probe_ret_ptr() but that's also ugly.

One comment inline which is why I didn't just pick this up today and send
a new version of this patch in my series.

Jonathan

> ---
>  drivers/iio/temperature/ltc2983.c | 190 ++++++++++++++++++++------------------
>  1 file changed, 98 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> index 23f2d43fc040..4b096aa3fbd8 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>
> @@ -656,11 +658,12 @@ 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;
>  	struct fwnode_handle *ref;
>  	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,8 +690,9 @@ 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);
> +			dev_err_probe(dev, -EINVAL,
> +				      "Invalid open circuit current:%u",
> +				      oc_current);
Hmm. I'm in two minds on these. 
We don't get the advantage of return dev_error_probe() and I'm not seeing these
hitting EPROBE_DEFER so getting the debug advantages.


>  			return ERR_PTR(-EINVAL);
>  		}

Re: [PATCH 3/6] iio: temperature: ltc2983: convert to dev_err_probe()

[Nuno Sá]

On Sat, 2024-02-24 at 18:44 +0000, Jonathan Cameron wrote:
> On Thu, 22 Feb 2024 13:55:54 +0100
> Nuno Sa <nuno.sa@analog.com> 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().
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> Hmm. Up to you whether you rebase on top of the device_for_each_child_node_scoped()
> patch - mostly depends if you give the new version a reviewed by or not!
> 

Already gave my tag. I see you're already doing some dev_err_probe() in there so I
can just add the missing ones after rebasing. 

> If they land in the other order I can fix it up whilst applying.
> After that series is in place though the number of places this will do
> 
> 	dev_err_probe(dev, ret, "message\n");
> 	return ERR_PTR(ret);
> 
> Makes me wonder whether
> 	return ERR_PTR(dev_err_probe(dev, ret, "message\n")); is
> too ugly or not?
> 

Yeah, IMO, that's a bit ugly :). We may making it slightly better for the driver by
adding a macro for the above.

> Maybe we need a dev_err_probe_ret_ptr() but that's also ugly.
> 

I almost send a patch for that as well. I think this driver is indeed a good
candidate for introducing an helper like this. My thought on naming was something
like dev_errp_probe() though.

> One comment inline which is why I didn't just pick this up today and send
> a new version of this patch in my series.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/temperature/ltc2983.c | 190 ++++++++++++++++++++------------------
> >  1 file changed, 98 insertions(+), 92 deletions(-)
> > 
> > diff --git a/drivers/iio/temperature/ltc2983.c
> > b/drivers/iio/temperature/ltc2983.c
> > index 23f2d43fc040..4b096aa3fbd8 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>
> > @@ -656,11 +658,12 @@ 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;
> >  	struct fwnode_handle *ref;
> >  	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,8 +690,9 @@ 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);
> > +			dev_err_probe(dev, -EINVAL,
> > +				      "Invalid open circuit current:%u",
> > +				      oc_current);
> Hmm. I'm in two minds on these. 
> We don't get the advantage of return dev_error_probe() and I'm not seeing these
> hitting EPROBE_DEFER so getting the debug advantages.

I would argue on logging (output) consistency (I think Andy pointed that in some
other series - I think my IIO backend stuff).

- Nuno Sá

[PATCH 4/6] iio: temperature: ltc2983: explicitly set the name in chip_info

[Nuno Sa]

Getting the part name with 'spi_get_device_id(spi)->name' is not a very
good pattern. Hence, explicitly add the name in the struct chip_info and
use that instead.

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

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 4b096aa3fbd8..9bd53e102ab3 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -209,6 +209,7 @@ enum {
 		container_of(_sensor, struct ltc2983_temp, sensor)
 
 struct ltc2983_chip_info {
+	const char *name;
 	unsigned int max_channels_nr;
 	bool has_temp;
 	bool has_eeprom;
@@ -1613,7 +1614,6 @@ static int ltc2983_probe(struct spi_device *spi)
 	struct ltc2983_data *st;
 	struct iio_dev *indio_dev;
 	struct gpio_desc *gpio;
-	const char *name = spi_get_device_id(spi)->name;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
@@ -1662,7 +1662,7 @@ static int ltc2983_probe(struct spi_device *spi)
 		return ret;
 
 	ret = devm_request_irq(dev, spi->irq, ltc2983_irq_handler,
-			       IRQF_TRIGGER_RISING, name, st);
+			       IRQF_TRIGGER_RISING, st->info->name, st);
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to request an irq, %d",
 				     ret);
@@ -1676,7 +1676,7 @@ static int ltc2983_probe(struct spi_device *spi)
 			return ret;
 	}
 
-	indio_dev->name = name;
+	indio_dev->name = st->info->name;
 	indio_dev->num_channels = st->iio_channels;
 	indio_dev->channels = st->iio_chan;
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -1707,15 +1707,25 @@ static DEFINE_SIMPLE_DEV_PM_OPS(ltc2983_pm_ops, ltc2983_suspend,
 				ltc2983_resume);
 
 static const struct ltc2983_chip_info ltc2983_chip_info_data = {
+	.name = "ltc2983",
 	.max_channels_nr = 20,
 };
 
 static const struct ltc2983_chip_info ltc2984_chip_info_data = {
+	.name = "ltc2984",
 	.max_channels_nr = 20,
 	.has_eeprom = true,
 };
 
 static const struct ltc2983_chip_info ltc2986_chip_info_data = {
+	.name = "ltc2986",
+	.max_channels_nr = 10,
+	.has_temp = true,
+	.has_eeprom = true,
+};
+
+static const struct ltc2983_chip_info ltm2985_chip_info_data = {
+	.name = "ltm2985",
 	.max_channels_nr = 10,
 	.has_temp = true,
 	.has_eeprom = true,
@@ -1725,7 +1735,7 @@ static const struct spi_device_id ltc2983_id_table[] = {
 	{ "ltc2983", (kernel_ulong_t)&ltc2983_chip_info_data },
 	{ "ltc2984", (kernel_ulong_t)&ltc2984_chip_info_data },
 	{ "ltc2986", (kernel_ulong_t)&ltc2986_chip_info_data },
-	{ "ltm2985", (kernel_ulong_t)&ltc2986_chip_info_data },
+	{ "ltm2985", (kernel_ulong_t)&ltm2985_chip_info_data },
 	{},
 };
 MODULE_DEVICE_TABLE(spi, ltc2983_id_table);
@@ -1734,7 +1744,7 @@ static const struct of_device_id ltc2983_of_match[] = {
 	{ .compatible = "adi,ltc2983", .data = &ltc2983_chip_info_data },
 	{ .compatible = "adi,ltc2984", .data = &ltc2984_chip_info_data },
 	{ .compatible = "adi,ltc2986", .data = &ltc2986_chip_info_data },
-	{ .compatible = "adi,ltm2985", .data = &ltc2986_chip_info_data },
+	{ .compatible = "adi,ltm2985", .data = &ltm2985_chip_info_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ltc2983_of_match);

-- 
2.43.2

Re: [PATCH 4/6] iio: temperature: ltc2983: explicitly set the name in chip_info

[Jonathan Cameron]

On Thu, 22 Feb 2024 13:55:55 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> Getting the part name with 'spi_get_device_id(spi)->name' is not a very
> good pattern. Hence, explicitly add the name in the struct chip_info and
> use that instead.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

Good change and it nearly went in clean without the previous so I
dealt with the one context issue and applied it.

Too many patches floating around at the moment so I'm keen to
reduce the number where I can by applying them :)

[PATCH 5/6] dt-bindings: iio: temperature: ltc2983: document power supply

[Nuno Sa]

Add a property for the VDD power supply regulator.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
index dbb85135fd66..8aae867a770a 100644
--- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
@@ -57,6 +57,8 @@ properties:
   interrupts:
     maxItems: 1
 
+  vdd-supply: true
+
   adi,mux-delay-config-us:
     description: |
       Extra delay prior to each conversion, in addition to the internal 1ms

-- 
2.43.2

Re: [PATCH 5/6] dt-bindings: iio: temperature: ltc2983: document power supply

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote:
> Add a property for the VDD power supply regulator.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> index dbb85135fd66..8aae867a770a 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> @@ -57,6 +57,8 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  vdd-supply: true

Although technically an ABI break, should we make this supply required?
It is, at the end of the day, required by the hardware for operation.

> +
>    adi,mux-delay-config-us:
>      description: |
>        Extra delay prior to each conversion, in addition to the internal 1ms
> 
> -- 
> 2.43.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 5/6] dt-bindings: iio: temperature: ltc2983: document power supply

[Nuno Sá]

On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote:
> On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote:
> > Add a property for the VDD power supply regulator.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > index dbb85135fd66..8aae867a770a 100644
> > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > @@ -57,6 +57,8 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >  
> > +  vdd-supply: true
> 
> Although technically an ABI break, should we make this supply required?
> It is, at the end of the day, required by the hardware for operation.
> 

I thought about it but then realized it could break some existing users which is
never a nice thing.

I recently (in another series - the IIO backend) went through some trouble to
actually not break ABI. Meaning, I had to do some not so neat hacking in the
driver because Rob was more comfortable with not breaking ABI in DT. So, I
assumed he would not like for me to break it in here.

- Nuno Sá
> 

Re: [PATCH 5/6] dt-bindings: iio: temperature: ltc2983: document power supply

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

On Thu, Feb 22, 2024 at 05:41:03PM +0100, Nuno Sá wrote:
> On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote:
> > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote:
> > > Add a property for the VDD power supply regulator.
> > > 
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > index dbb85135fd66..8aae867a770a 100644
> > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > @@ -57,6 +57,8 @@ properties:
> > >    interrupts:
> > >      maxItems: 1
> > >  
> > > +  vdd-supply: true
> > 
> > Although technically an ABI break, should we make this supply required?
> > It is, at the end of the day, required by the hardware for operation.
> > 
> 
> I thought about it but then realized it could break some existing users which is
> never a nice thing.

Could you explain what scenario it actually breaks a system (not
produces warnings with dtbs_check)?

If anything actually broke something, it would be the driver change that
actually assumed that the regulator was present and refused to probe
otherwise, right? In Linux at least, the regulator core will provide a
dummy regulator if one doesn't exist - otherwise patch 6/6 would
actually contain a DT ABI breakage, since it does:

	ret = devm_regulator_get_enable(&spi->dev, "vdd");
	if (ret)
		return ret;

IMO making a new property required is only a meaningful break of the ABI
when drivers reject probe when it is missing, but I must admit I don't
know if other operating systems handle missing regulators as nicely as
Linux does.

> I recently (in another series - the IIO backend) went through some trouble to
> actually not break ABI. Meaning, I had to do some not so neat hacking in the
> driver because Rob was more comfortable with not breaking ABI in DT. So, I
> assumed he would not like for me to break it in here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 5/6] dt-bindings: iio: temperature: ltc2983: document power supply

[Jonathan Cameron]

On Thu, 22 Feb 2024 17:54:28 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Thu, Feb 22, 2024 at 05:41:03PM +0100, Nuno Sá wrote:
> > On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote:  
> > > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote:  
> > > > Add a property for the VDD power supply regulator.
> > > > 
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > index dbb85135fd66..8aae867a770a 100644
> > > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > @@ -57,6 +57,8 @@ properties:
> > > >    interrupts:
> > > >      maxItems: 1
> > > >  
> > > > +  vdd-supply: true  
> > > 
> > > Although technically an ABI break, should we make this supply required?
> > > It is, at the end of the day, required by the hardware for operation.
> > >   
> > 
> > I thought about it but then realized it could break some existing users which is
> > never a nice thing.  
> 
> Could you explain what scenario it actually breaks a system (not
> produces warnings with dtbs_check)?
> 
> If anything actually broke something, it would be the driver change that
> actually assumed that the regulator was present and refused to probe
> otherwise, right? In Linux at least, the regulator core will provide a
> dummy regulator if one doesn't exist - otherwise patch 6/6 would
> actually contain a DT ABI breakage, since it does:
> 
> 	ret = devm_regulator_get_enable(&spi->dev, "vdd");
> 	if (ret)
> 		return ret;
> 
> IMO making a new property required is only a meaningful break of the ABI
> when drivers reject probe when it is missing, but I must admit I don't
> know if other operating systems handle missing regulators as nicely as
> Linux does.

Agreed - adding a requirement on a supply to the dt-binding shouldn't
be a problem because of the dummy regulators.

Jonathan

> 
> > I recently (in another series - the IIO backend) went through some trouble to
> > actually not break ABI. Meaning, I had to do some not so neat hacking in the
> > driver because Rob was more comfortable with not breaking ABI in DT. So, I
> > assumed he would not like for me to break it in here.  
> 

Re: [PATCH 5/6] dt-bindings: iio: temperature: ltc2983: document power supply

[Nuno Sá]

On Thu, 2024-02-22 at 17:54 +0000, Conor Dooley wrote:
> On Thu, Feb 22, 2024 at 05:41:03PM +0100, Nuno Sá wrote:
> > On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote:
> > > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote:
> > > > Add a property for the VDD power supply regulator.
> > > > 
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2
> > > > ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > index dbb85135fd66..8aae867a770a 100644
> > > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > @@ -57,6 +57,8 @@ properties:
> > > >    interrupts:
> > > >      maxItems: 1
> > > >  
> > > > +  vdd-supply: true
> > > 
> > > Although technically an ABI break, should we make this supply required?
> > > It is, at the end of the day, required by the hardware for operation.
> > > 
> > 
> > I thought about it but then realized it could break some existing users
> > which is
> > never a nice thing.
> 
> Could you explain what scenario it actually breaks a system (not
> produces warnings with dtbs_check)?

Oh, I guess I could not explain myself :). I did not meant breaking the system
(I'm aware of the dummy regulator) but I meant exactly what you mention above
about dtbs_check. Like, if someone already validated a devicetree against the
current bindings, that same devicetree will fail to validate now right? And I
had the idea that we should not allow that... If not the case, I'm perfectly
fine in making the supply required.

- Nuno Sá

> 

Re: [PATCH 5/6] dt-bindings: iio: temperature: ltc2983: document power supply

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

On Fri, Feb 23, 2024 at 09:17:16AM +0100, Nuno Sá wrote:
> On Thu, 2024-02-22 at 17:54 +0000, Conor Dooley wrote:
> > On Thu, Feb 22, 2024 at 05:41:03PM +0100, Nuno Sá wrote:
> > > On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote:
> > > > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote:
> > > > > Add a property for the VDD power supply regulator.
> > > > > 
> > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2
> > > > > ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > > index dbb85135fd66..8aae867a770a 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > > > > @@ -57,6 +57,8 @@ properties:
> > > > >    interrupts:
> > > > >      maxItems: 1
> > > > >  
> > > > > +  vdd-supply: true
> > > > 
> > > > Although technically an ABI break, should we make this supply required?
> > > > It is, at the end of the day, required by the hardware for operation.
> > > > 
> > > 
> > > I thought about it but then realized it could break some existing users
> > > which is
> > > never a nice thing.
> > 
> > Could you explain what scenario it actually breaks a system (not
> > produces warnings with dtbs_check)?
> 
> Oh, I guess I could not explain myself :). I did not meant breaking the system
> (I'm aware of the dummy regulator) but I meant exactly what you mention above
> about dtbs_check. Like, if someone already validated a devicetree against the
> current bindings, that same devicetree will fail to validate now right? And I
> had the idea that we should not allow that... If not the case, I'm perfectly
> fine in making the supply required.

I think that's fine, the system will still work which is the important
part of the ABI.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 6/6] iio: temperature: ltc2983: support vdd regulator

[Nuno Sa]

Add support for the power supply regulator.

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

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 9bd53e102ab3..bc0518ee1b89 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
 #include <asm/byteorder.h>
@@ -1641,6 +1642,10 @@ static int ltc2983_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	ret = devm_regulator_get_enable(&spi->dev, "vdd");
+	if (ret)
+		return ret;
+
 	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);

-- 
2.43.2