linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages
@ 2022-02-10 13:55 Andy Shevchenko
  2022-02-10 13:55 ` [PATCH v3 2/3] iio: temperature: ltc2983: Use single error path to put OF node Andy Shevchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Andy Shevchenko @ 2022-02-10 13:55 UTC (permalink / raw)
  To: Andy Shevchenko, linux-iio, linux-kernel
  Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen

In a couple of messages the constants, which have their definitions,
are hard coded into the message text. Unhardcode them.

While at it, add a trailing \n where it's currently missing.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
---
v3: added \n, used %u (Joe)
 drivers/iio/temperature/ltc2983.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 301c3f13fb26..94d6dd4db47a 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -409,8 +409,8 @@ static struct ltc2983_custom_sensor *__ltc2983_custom_sensor_new(
 	new_custom->size = n_entries * n_size;
 	/* check Steinhart size */
 	if (is_steinhart && new_custom->size != LTC2983_CUSTOM_STEINHART_SIZE) {
-		dev_err(dev, "Steinhart sensors size(%zu) must be 24",
-							new_custom->size);
+		dev_err(dev, "Steinhart sensors size(%zu) must be %u\n", new_custom->size,
+			LTC2983_CUSTOM_STEINHART_SIZE);
 		return ERR_PTR(-EINVAL);
 	}
 	/* Check space on the table. */
@@ -1299,8 +1299,8 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
 		if (sensor.chan < LTC2983_MIN_CHANNELS_NR ||
 		    sensor.chan > LTC2983_MAX_CHANNELS_NR) {
 			ret = -EINVAL;
-			dev_err(dev,
-				"chan:%d must be from 1 to 20\n", sensor.chan);
+			dev_err(dev, "chan:%d must be from %u to %u\n", sensor.chan,
+				LTC2983_MIN_CHANNELS_NR, LTC2983_MAX_CHANNELS_NR);
 			goto put_child;
 		} else if (channel_avail_mask & BIT(sensor.chan)) {
 			ret = -EINVAL;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/3] iio: temperature: ltc2983: Use single error path to put OF node
  2022-02-10 13:55 [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Andy Shevchenko
@ 2022-02-10 13:55 ` Andy Shevchenko
  2022-02-10 13:55 ` [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties Andy Shevchenko
  2022-02-13 17:55 ` [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Jonathan Cameron
  2 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2022-02-10 13:55 UTC (permalink / raw)
  To: Andy Shevchenko, linux-iio, linux-kernel
  Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen

There are several, possibly copied'n'pasted, places in the functions,
where OF node is put and error returned directly, while the common
exit point exists. Unify all these cases to use a single error path.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: no changes
 drivers/iio/temperature/ltc2983.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 94d6dd4db47a..636bbc9011b9 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -653,8 +653,6 @@ static struct ltc2983_sensor *ltc2983_thermocouple_new(
 
 	phandle = of_parse_phandle(child, "adi,cold-junction-handle", 0);
 	if (phandle) {
-		int ret;
-
 		ret = of_property_read_u32(phandle, "reg",
 					   &thermo->cold_junction_chan);
 		if (ret) {
@@ -663,8 +661,7 @@ static struct ltc2983_sensor *ltc2983_thermocouple_new(
 			 * the error right away.
 			 */
 			dev_err(&st->spi->dev, "Property reg must be given\n");
-			of_node_put(phandle);
-			return ERR_PTR(-EINVAL);
+			goto fail;
 		}
 	}
 
@@ -676,8 +673,8 @@ static struct ltc2983_sensor *ltc2983_thermocouple_new(
 							     propname, false,
 							     16384, true);
 		if (IS_ERR(thermo->custom)) {
-			of_node_put(phandle);
-			return ERR_CAST(thermo->custom);
+			ret = PTR_ERR(thermo->custom);
+			goto fail;
 		}
 	}
 
@@ -687,6 +684,10 @@ static struct ltc2983_sensor *ltc2983_thermocouple_new(
 
 	of_node_put(phandle);
 	return &thermo->sensor;
+
+fail:
+	of_node_put(phandle);
+	return ERR_PTR(ret);
 }
 
 static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
@@ -803,8 +804,8 @@ static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
 							  "adi,custom-rtd",
 							  false, 2048, false);
 		if (IS_ERR(rtd->custom)) {
-			of_node_put(phandle);
-			return ERR_CAST(rtd->custom);
+			ret = PTR_ERR(rtd->custom);
+			goto fail;
 		}
 	}
 
@@ -926,8 +927,8 @@ static struct ltc2983_sensor *ltc2983_thermistor_new(
 								 steinhart,
 								 64, false);
 		if (IS_ERR(thermistor->custom)) {
-			of_node_put(phandle);
-			return ERR_CAST(thermistor->custom);
+			ret = PTR_ERR(thermistor->custom);
+			goto fail;
 		}
 	}
 	/* set common parameters */
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties
  2022-02-10 13:55 [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Andy Shevchenko
  2022-02-10 13:55 ` [PATCH v3 2/3] iio: temperature: ltc2983: Use single error path to put OF node Andy Shevchenko
@ 2022-02-10 13:55 ` Andy Shevchenko
  2022-03-03 13:31   ` Sa, Nuno
  2022-02-13 17:55 ` [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Jonathan Cameron
  2 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2022-02-10 13:55 UTC (permalink / raw)
  To: Andy Shevchenko, linux-iio, linux-kernel
  Cc: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen

Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.

The conversion slightly changes the logic behind property reading for
the configuration values. Original code allocates just as much memory
as needed. Then for each separate 32- or 64-bit value it reads it from
the property and converts to a raw one which will be fed to the sensor.
In the new code we allocate the amount of memory needed to retrieve all
values at once from the property and then convert them as required.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: no changes
 drivers/iio/temperature/ltc2983.c | 203 +++++++++++++++---------------
 1 file changed, 102 insertions(+), 101 deletions(-)

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index 636bbc9011b9..d20f957ca0d9 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -12,11 +12,15 @@
 #include <linux/iio/iio.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/of_gpio.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/spi/spi.h>
 
+#include <asm/byteorder.h>
+#include <asm/unaligned.h>
+
 /* register map */
 #define LTC2983_STATUS_REG			0x0000
 #define LTC2983_TEMP_RES_START_REG		0x0010
@@ -219,7 +223,7 @@ struct ltc2983_sensor {
 
 struct ltc2983_custom_sensor {
 	/* raw table sensor data */
-	u8 *table;
+	void *table;
 	size_t size;
 	/* address offset */
 	s8 offset;
@@ -377,25 +381,25 @@ static int __ltc2983_chan_custom_sensor_assign(struct ltc2983_data *st,
 	return regmap_bulk_write(st->regmap, reg, custom->table, custom->size);
 }
 
-static struct ltc2983_custom_sensor *__ltc2983_custom_sensor_new(
-						struct ltc2983_data *st,
-						const struct device_node *np,
-						const char *propname,
-						const bool is_steinhart,
-						const u32 resolution,
-						const bool has_signed)
+static struct ltc2983_custom_sensor *
+__ltc2983_custom_sensor_new(struct ltc2983_data *st, const struct fwnode_handle *fn,
+			    const char *propname, const bool is_steinhart,
+			    const u32 resolution, const bool has_signed)
 {
 	struct ltc2983_custom_sensor *new_custom;
-	u8 index, n_entries, tbl = 0;
 	struct device *dev = &st->spi->dev;
 	/*
 	 * For custom steinhart, the full u32 is taken. For all the others
 	 * the MSB is discarded.
 	 */
 	const u8 n_size = is_steinhart ? 4 : 3;
-	const u8 e_size = is_steinhart ? sizeof(u32) : sizeof(u64);
+	u8 index, n_entries;
+	int ret;
 
-	n_entries = of_property_count_elems_of_size(np, propname, e_size);
+	if (is_steinhart)
+		n_entries = fwnode_property_count_u32(fn, propname);
+	else
+		n_entries = fwnode_property_count_u64(fn, propname);
 	/* n_entries must be an even number */
 	if (!n_entries || (n_entries % 2) != 0) {
 		dev_err(dev, "Number of entries either 0 or not even\n");
@@ -423,21 +427,33 @@ static struct ltc2983_custom_sensor *__ltc2983_custom_sensor_new(
 	}
 
 	/* allocate the table */
-	new_custom->table = devm_kzalloc(dev, new_custom->size, GFP_KERNEL);
+	if (is_steinhart)
+		new_custom->table = devm_kcalloc(dev, n_entries, sizeof(u32), GFP_KERNEL);
+	else
+		new_custom->table = devm_kcalloc(dev, n_entries, sizeof(u64), GFP_KERNEL);
 	if (!new_custom->table)
 		return ERR_PTR(-ENOMEM);
 
-	for (index = 0; index < n_entries; index++) {
-		u64 temp = 0, j;
-		/*
-		 * Steinhart sensors are configured with raw values in the
-		 * devicetree. For the other sensors we must convert the
-		 * value to raw. The odd index's correspond to temperarures
-		 * and always have 1/1024 of resolution. Temperatures also
-		 * come in kelvin, so signed values is not possible
-		 */
-		if (!is_steinhart) {
-			of_property_read_u64_index(np, propname, index, &temp);
+	/*
+	 * Steinhart sensors are configured with raw values in the firmware
+	 * node. For the other sensors we must convert the value to raw.
+	 * The odd index's correspond to temperatures and always have 1/1024
+	 * of resolution. Temperatures also come in Kelvin, so signed values
+	 * are not possible.
+	 */
+	if (is_steinhart) {
+		ret = fwnode_property_read_u32_array(fn, propname, new_custom->table, n_entries);
+		if (ret < 0)
+			return ERR_PTR(ret);
+
+		cpu_to_be32_array(new_custom->table, new_custom->table, n_entries);
+	} else {
+		ret = fwnode_property_read_u64_array(fn, propname, new_custom->table, n_entries);
+		if (ret < 0)
+			return ERR_PTR(ret);
+
+		for (index = 0; index < n_entries; index++) {
+			u64 temp = ((u64 *)new_custom->table)[index];
 
 			if ((index % 2) != 0)
 				temp = __convert_to_raw(temp, 1024);
@@ -445,16 +461,9 @@ static struct ltc2983_custom_sensor *__ltc2983_custom_sensor_new(
 				temp = __convert_to_raw_sign(temp, resolution);
 			else
 				temp = __convert_to_raw(temp, resolution);
-		} else {
-			u32 t32;
 
-			of_property_read_u32_index(np, propname, index, &t32);
-			temp = t32;
+			put_unaligned_be24(temp, new_custom->table + index * 3);
 		}
-
-		for (j = 0; j < n_size; j++)
-			new_custom->table[tbl++] =
-				temp >> (8 * (n_size - j - 1));
 	}
 
 	new_custom->is_steinhart = is_steinhart;
@@ -597,13 +606,12 @@ static int ltc2983_adc_assign_chan(struct ltc2983_data *st,
 	return __ltc2983_chan_assign_common(st, sensor, chan_val);
 }
 
-static struct ltc2983_sensor *ltc2983_thermocouple_new(
-					const struct device_node *child,
-					struct ltc2983_data *st,
-					const struct ltc2983_sensor *sensor)
+static struct ltc2983_sensor *
+ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data *st,
+			 const struct ltc2983_sensor *sensor)
 {
 	struct ltc2983_thermocouple *thermo;
-	struct device_node *phandle;
+	struct fwnode_handle *ref;
 	u32 oc_current;
 	int ret;
 
@@ -611,11 +619,10 @@ static struct ltc2983_sensor *ltc2983_thermocouple_new(
 	if (!thermo)
 		return ERR_PTR(-ENOMEM);
 
-	if (of_property_read_bool(child, "adi,single-ended"))
+	if (fwnode_property_read_bool(child, "adi,single-ended"))
 		thermo->sensor_config = LTC2983_THERMOCOUPLE_SGL(1);
 
-	ret = of_property_read_u32(child, "adi,sensor-oc-current-microamp",
-				   &oc_current);
+	ret = fwnode_property_read_u32(child, "adi,sensor-oc-current-microamp", &oc_current);
 	if (!ret) {
 		switch (oc_current) {
 		case 10:
@@ -651,10 +658,9 @@ static struct ltc2983_sensor *ltc2983_thermocouple_new(
 		return ERR_PTR(-EINVAL);
 	}
 
-	phandle = of_parse_phandle(child, "adi,cold-junction-handle", 0);
-	if (phandle) {
-		ret = of_property_read_u32(phandle, "reg",
-					   &thermo->cold_junction_chan);
+	ref = fwnode_find_reference(child, "adi,cold-junction-handle", 0);
+	if (ref) {
+		ret = fwnode_property_read_u32(ref, "reg", &thermo->cold_junction_chan);
 		if (ret) {
 			/*
 			 * This would be catched later but we can just return
@@ -682,41 +688,41 @@ static struct ltc2983_sensor *ltc2983_thermocouple_new(
 	thermo->sensor.fault_handler = ltc2983_thermocouple_fault_handler;
 	thermo->sensor.assign_chan = ltc2983_thermocouple_assign_chan;
 
-	of_node_put(phandle);
+	fwnode_handle_put(ref);
 	return &thermo->sensor;
 
 fail:
-	of_node_put(phandle);
+	fwnode_handle_put(ref);
 	return ERR_PTR(ret);
 }
 
-static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
-					  struct ltc2983_data *st,
-					  const struct ltc2983_sensor *sensor)
+static struct ltc2983_sensor *
+ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
+		const struct ltc2983_sensor *sensor)
 {
 	struct ltc2983_rtd *rtd;
 	int ret = 0;
 	struct device *dev = &st->spi->dev;
-	struct device_node *phandle;
+	struct fwnode_handle *ref;
 	u32 excitation_current = 0, n_wires = 0;
 
 	rtd = devm_kzalloc(dev, sizeof(*rtd), GFP_KERNEL);
 	if (!rtd)
 		return ERR_PTR(-ENOMEM);
 
-	phandle = of_parse_phandle(child, "adi,rsense-handle", 0);
-	if (!phandle) {
+	ref = fwnode_find_reference(child, "adi,rsense-handle", 0);
+	if (!ref) {
 		dev_err(dev, "Property adi,rsense-handle missing or invalid");
 		return ERR_PTR(-EINVAL);
 	}
 
-	ret = of_property_read_u32(phandle, "reg", &rtd->r_sense_chan);
+	ret = fwnode_property_read_u32(ref, "reg", &rtd->r_sense_chan);
 	if (ret) {
 		dev_err(dev, "Property reg must be given\n");
 		goto fail;
 	}
 
-	ret = of_property_read_u32(child, "adi,number-of-wires", &n_wires);
+	ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
 	if (!ret) {
 		switch (n_wires) {
 		case 2:
@@ -739,9 +745,9 @@ static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
 		}
 	}
 
-	if (of_property_read_bool(child, "adi,rsense-share")) {
+	if (fwnode_property_read_bool(child, "adi,rsense-share")) {
 		/* Current rotation is only available with rsense sharing */
-		if (of_property_read_bool(child, "adi,current-rotate")) {
+		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");
@@ -813,8 +819,8 @@ static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
 	rtd->sensor.fault_handler = ltc2983_common_fault_handler;
 	rtd->sensor.assign_chan = ltc2983_rtd_assign_chan;
 
-	ret = of_property_read_u32(child, "adi,excitation-current-microamp",
-				   &excitation_current);
+	ret = fwnode_property_read_u32(child, "adi,excitation-current-microamp",
+				       &excitation_current);
 	if (ret) {
 		/* default to 5uA */
 		rtd->excitation_current = 1;
@@ -853,23 +859,22 @@ static struct ltc2983_sensor *ltc2983_rtd_new(const struct device_node *child,
 		}
 	}
 
-	of_property_read_u32(child, "adi,rtd-curve", &rtd->rtd_curve);
+	fwnode_property_read_u32(child, "adi,rtd-curve", &rtd->rtd_curve);
 
-	of_node_put(phandle);
+	fwnode_handle_put(ref);
 	return &rtd->sensor;
 fail:
-	of_node_put(phandle);
+	fwnode_handle_put(ref);
 	return ERR_PTR(ret);
 }
 
-static struct ltc2983_sensor *ltc2983_thermistor_new(
-					const struct device_node *child,
-					struct ltc2983_data *st,
-					const struct ltc2983_sensor *sensor)
+static struct ltc2983_sensor *
+ltc2983_thermistor_new(const struct fwnode_handle *child, struct ltc2983_data *st,
+		       const struct ltc2983_sensor *sensor)
 {
 	struct ltc2983_thermistor *thermistor;
 	struct device *dev = &st->spi->dev;
-	struct device_node *phandle;
+	struct fwnode_handle *ref;
 	u32 excitation_current = 0;
 	int ret = 0;
 
@@ -877,23 +882,23 @@ static struct ltc2983_sensor *ltc2983_thermistor_new(
 	if (!thermistor)
 		return ERR_PTR(-ENOMEM);
 
-	phandle = of_parse_phandle(child, "adi,rsense-handle", 0);
-	if (!phandle) {
+	ref = fwnode_find_reference(child, "adi,rsense-handle", 0);
+	if (!ref) {
 		dev_err(dev, "Property adi,rsense-handle missing or invalid");
 		return ERR_PTR(-EINVAL);
 	}
 
-	ret = of_property_read_u32(phandle, "reg", &thermistor->r_sense_chan);
+	ret = fwnode_property_read_u32(ref, "reg", &thermistor->r_sense_chan);
 	if (ret) {
 		dev_err(dev, "rsense channel must be configured...\n");
 		goto fail;
 	}
 
-	if (of_property_read_bool(child, "adi,single-ended")) {
+	if (fwnode_property_read_bool(child, "adi,single-ended")) {
 		thermistor->sensor_config = LTC2983_THERMISTOR_SGL(1);
-	} else if (of_property_read_bool(child, "adi,rsense-share")) {
+	} else if (fwnode_property_read_bool(child, "adi,rsense-share")) {
 		/* rotation is only possible if sharing rsense */
-		if (of_property_read_bool(child, "adi,current-rotate"))
+		if (fwnode_property_read_bool(child, "adi,current-rotate"))
 			thermistor->sensor_config =
 						LTC2983_THERMISTOR_C_ROTATE(1);
 		else
@@ -935,8 +940,8 @@ static struct ltc2983_sensor *ltc2983_thermistor_new(
 	thermistor->sensor.fault_handler = ltc2983_common_fault_handler;
 	thermistor->sensor.assign_chan = ltc2983_thermistor_assign_chan;
 
-	ret = of_property_read_u32(child, "adi,excitation-current-nanoamp",
-				   &excitation_current);
+	ret = fwnode_property_read_u32(child, "adi,excitation-current-nanoamp",
+				       &excitation_current);
 	if (ret) {
 		/* Auto range is not allowed for custom sensors */
 		if (sensor->type >= LTC2983_SENSOR_THERMISTOR_STEINHART)
@@ -1000,17 +1005,16 @@ static struct ltc2983_sensor *ltc2983_thermistor_new(
 		}
 	}
 
-	of_node_put(phandle);
+	fwnode_handle_put(ref);
 	return &thermistor->sensor;
 fail:
-	of_node_put(phandle);
+	fwnode_handle_put(ref);
 	return ERR_PTR(ret);
 }
 
-static struct ltc2983_sensor *ltc2983_diode_new(
-					const struct device_node *child,
-					const struct ltc2983_data *st,
-					const struct ltc2983_sensor *sensor)
+static struct ltc2983_sensor *
+ltc2983_diode_new(const struct fwnode_handle *child, const struct ltc2983_data *st,
+		  const struct ltc2983_sensor *sensor)
 {
 	struct ltc2983_diode *diode;
 	u32 temp = 0, excitation_current = 0;
@@ -1020,13 +1024,13 @@ static struct ltc2983_sensor *ltc2983_diode_new(
 	if (!diode)
 		return ERR_PTR(-ENOMEM);
 
-	if (of_property_read_bool(child, "adi,single-ended"))
+	if (fwnode_property_read_bool(child, "adi,single-ended"))
 		diode->sensor_config = LTC2983_DIODE_SGL(1);
 
-	if (of_property_read_bool(child, "adi,three-conversion-cycles"))
+	if (fwnode_property_read_bool(child, "adi,three-conversion-cycles"))
 		diode->sensor_config |= LTC2983_DIODE_3_CONV_CYCLE(1);
 
-	if (of_property_read_bool(child, "adi,average-on"))
+	if (fwnode_property_read_bool(child, "adi,average-on"))
 		diode->sensor_config |= LTC2983_DIODE_AVERAGE_ON(1);
 
 	/* validate channel index */
@@ -1041,8 +1045,8 @@ static struct ltc2983_sensor *ltc2983_diode_new(
 	diode->sensor.fault_handler = ltc2983_common_fault_handler;
 	diode->sensor.assign_chan = ltc2983_diode_assign_chan;
 
-	ret = of_property_read_u32(child, "adi,excitation-current-microamp",
-				   &excitation_current);
+	ret = fwnode_property_read_u32(child, "adi,excitation-current-microamp",
+				       &excitation_current);
 	if (!ret) {
 		switch (excitation_current) {
 		case 10:
@@ -1065,7 +1069,7 @@ static struct ltc2983_sensor *ltc2983_diode_new(
 		}
 	}
 
-	of_property_read_u32(child, "adi,ideal-factor-value", &temp);
+	fwnode_property_read_u32(child, "adi,ideal-factor-value", &temp);
 
 	/* 2^20 resolution */
 	diode->ideal_factor_value = __convert_to_raw(temp, 1048576);
@@ -1073,7 +1077,7 @@ static struct ltc2983_sensor *ltc2983_diode_new(
 	return &diode->sensor;
 }
 
-static struct ltc2983_sensor *ltc2983_r_sense_new(struct device_node *child,
+static struct ltc2983_sensor *ltc2983_r_sense_new(struct fwnode_handle *child,
 					struct ltc2983_data *st,
 					const struct ltc2983_sensor *sensor)
 {
@@ -1092,7 +1096,7 @@ static struct ltc2983_sensor *ltc2983_r_sense_new(struct device_node *child,
 		return ERR_PTR(-EINVAL);
 	}
 
-	ret = of_property_read_u32(child, "adi,rsense-val-milli-ohms", &temp);
+	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);
@@ -1111,7 +1115,7 @@ static struct ltc2983_sensor *ltc2983_r_sense_new(struct device_node *child,
 	return &rsense->sensor;
 }
 
-static struct ltc2983_sensor *ltc2983_adc_new(struct device_node *child,
+static struct ltc2983_sensor *ltc2983_adc_new(struct fwnode_handle *child,
 					 struct ltc2983_data *st,
 					 const struct ltc2983_sensor *sensor)
 {
@@ -1121,7 +1125,7 @@ static struct ltc2983_sensor *ltc2983_adc_new(struct device_node *child,
 	if (!adc)
 		return ERR_PTR(-ENOMEM);
 
-	if (of_property_read_bool(child, "adi,single-ended"))
+	if (fwnode_property_read_bool(child, "adi,single-ended"))
 		adc->single_ended = true;
 
 	if (!adc->single_ended &&
@@ -1265,17 +1269,15 @@ static irqreturn_t ltc2983_irq_handler(int irq, void *data)
 
 static int ltc2983_parse_dt(struct ltc2983_data *st)
 {
-	struct device_node *child;
 	struct device *dev = &st->spi->dev;
+	struct fwnode_handle *child;
 	int ret = 0, chan = 0, channel_avail_mask = 0;
 
-	of_property_read_u32(dev->of_node, "adi,mux-delay-config-us",
-			     &st->mux_delay_config);
+	device_property_read_u32(dev, "adi,mux-delay-config-us", &st->mux_delay_config);
 
-	of_property_read_u32(dev->of_node, "adi,filter-notch-freq",
-			     &st->filter_notch_freq);
+	device_property_read_u32(dev, "adi,filter-notch-freq", &st->filter_notch_freq);
 
-	st->num_channels = of_get_available_child_count(dev->of_node);
+	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;
@@ -1287,10 +1289,10 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
 		return -ENOMEM;
 
 	st->iio_channels = st->num_channels;
-	for_each_available_child_of_node(dev->of_node, child) {
+	device_for_each_child_node(dev, child) {
 		struct ltc2983_sensor sensor;
 
-		ret = of_property_read_u32(child, "reg", &sensor.chan);
+		ret = fwnode_property_read_u32(child, "reg", &sensor.chan);
 		if (ret) {
 			dev_err(dev, "reg property must given for child nodes\n");
 			goto put_child;
@@ -1309,8 +1311,7 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
 			goto put_child;
 		}
 
-		ret = of_property_read_u32(child, "adi,sensor-type",
-					       &sensor.type);
+		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");
@@ -1364,7 +1365,7 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
 
 	return 0;
 put_child:
-	of_node_put(child);
+	fwnode_handle_put(child);
 	return ret;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages
  2022-02-10 13:55 [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Andy Shevchenko
  2022-02-10 13:55 ` [PATCH v3 2/3] iio: temperature: ltc2983: Use single error path to put OF node Andy Shevchenko
  2022-02-10 13:55 ` [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties Andy Shevchenko
@ 2022-02-13 17:55 ` Jonathan Cameron
  2022-02-14  9:23   ` Andy Shevchenko
  2022-03-02 15:50   ` Andy Shevchenko
  2 siblings, 2 replies; 13+ messages in thread
From: Jonathan Cameron @ 2022-02-13 17:55 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-iio, linux-kernel, Nuno Sá, Lars-Peter Clausen

On Thu, 10 Feb 2022 15:55:20 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> In a couple of messages the constants, which have their definitions,
> are hard coded into the message text. Unhardcode them.
> 
> While at it, add a trailing \n where it's currently missing.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>

Mostly so I can remember what is going on with this patch,
Nuno is OoO and planning to test this series when he returns.

Given that I'll wait on Nuno's testing.

Thanks,

Jonathan

> ---
> v3: added \n, used %u (Joe)
>  drivers/iio/temperature/ltc2983.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> index 301c3f13fb26..94d6dd4db47a 100644
> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
> @@ -409,8 +409,8 @@ static struct ltc2983_custom_sensor *__ltc2983_custom_sensor_new(
>  	new_custom->size = n_entries * n_size;
>  	/* check Steinhart size */
>  	if (is_steinhart && new_custom->size != LTC2983_CUSTOM_STEINHART_SIZE) {
> -		dev_err(dev, "Steinhart sensors size(%zu) must be 24",
> -							new_custom->size);
> +		dev_err(dev, "Steinhart sensors size(%zu) must be %u\n", new_custom->size,
> +			LTC2983_CUSTOM_STEINHART_SIZE);
>  		return ERR_PTR(-EINVAL);
>  	}
>  	/* Check space on the table. */
> @@ -1299,8 +1299,8 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
>  		if (sensor.chan < LTC2983_MIN_CHANNELS_NR ||
>  		    sensor.chan > LTC2983_MAX_CHANNELS_NR) {
>  			ret = -EINVAL;
> -			dev_err(dev,
> -				"chan:%d must be from 1 to 20\n", sensor.chan);
> +			dev_err(dev, "chan:%d must be from %u to %u\n", sensor.chan,
> +				LTC2983_MIN_CHANNELS_NR, LTC2983_MAX_CHANNELS_NR);
>  			goto put_child;
>  		} else if (channel_avail_mask & BIT(sensor.chan)) {
>  			ret = -EINVAL;


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages
  2022-02-13 17:55 ` [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Jonathan Cameron
@ 2022-02-14  9:23   ` Andy Shevchenko
  2022-03-02 15:50   ` Andy Shevchenko
  1 sibling, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2022-02-14  9:23 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, linux-kernel, Nuno Sá, Lars-Peter Clausen

On Sun, Feb 13, 2022 at 05:55:59PM +0000, Jonathan Cameron wrote:
> On Thu, 10 Feb 2022 15:55:20 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > In a couple of messages the constants, which have their definitions,
> > are hard coded into the message text. Unhardcode them.
> > 
> > While at it, add a trailing \n where it's currently missing.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> Mostly so I can remember what is going on with this patch,
> Nuno is OoO and planning to test this series when he returns.
> 
> Given that I'll wait on Nuno's testing.

Me too :-)

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages
  2022-02-13 17:55 ` [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Jonathan Cameron
  2022-02-14  9:23   ` Andy Shevchenko
@ 2022-03-02 15:50   ` Andy Shevchenko
  2022-03-02 19:56     ` Nuno Sá
  1 sibling, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2022-03-02 15:50 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, linux-kernel, Nuno Sá, Lars-Peter Clausen

On Sun, Feb 13, 2022 at 05:55:59PM +0000, Jonathan Cameron wrote:
> On Thu, 10 Feb 2022 15:55:20 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > In a couple of messages the constants, which have their definitions,
> > are hard coded into the message text. Unhardcode them.
> > 
> > While at it, add a trailing \n where it's currently missing.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> Mostly so I can remember what is going on with this patch,
> Nuno is OoO and planning to test this series when he returns.
> 
> Given that I'll wait on Nuno's testing.

Any news?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages
  2022-03-02 15:50   ` Andy Shevchenko
@ 2022-03-02 19:56     ` Nuno Sá
  0 siblings, 0 replies; 13+ messages in thread
From: Nuno Sá @ 2022-03-02 19:56 UTC (permalink / raw)
  To: Andy Shevchenko, Jonathan Cameron
  Cc: linux-iio, linux-kernel, Nuno Sá, Lars-Peter Clausen

On Wed, 2022-03-02 at 17:50 +0200, Andy Shevchenko wrote:
> On Sun, Feb 13, 2022 at 05:55:59PM +0000, Jonathan Cameron wrote:
> > On Thu, 10 Feb 2022 15:55:20 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > In a couple of messages the constants, which have their
> > > definitions,
> > > are hard coded into the message text. Unhardcode them.
> > > 
> > > While at it, add a trailing \n where it's currently missing.
> > > 
> > > Signed-off-by: Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com>
> > > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> > 
> > Mostly so I can remember what is going on with this patch,
> > Nuno is OoO and planning to test this series when he returns.
> > 
> > Given that I'll wait on Nuno's testing.
> 
> Any news?
> 
Started to prepare things by the end of the day. Should be tested by
tomorrow.

- Nuno Sá


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties
  2022-02-10 13:55 ` [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties Andy Shevchenko
@ 2022-03-03 13:31   ` Sa, Nuno
  2022-03-03 13:57     ` Andy Shevchenko
  2022-03-03 14:23     ` Andy Shevchenko
  0 siblings, 2 replies; 13+ messages in thread
From: Sa, Nuno @ 2022-03-03 13:31 UTC (permalink / raw)
  To: Andy Shevchenko, linux-iio, linux-kernel
  Cc: Jonathan Cameron, Lars-Peter Clausen

Hi Andy,

Good that we waited to test this patch. The fundamental logic change
for fetching and writing the custom tables are fine. That said, there
some issues that I had to fix to test the patch. See below...

> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Thursday, February 10, 2022 2:55 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; Jonathan Cameron
> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
> Subject: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device
> properties
> 
> [External]
> 
> Convert the module to be property provider agnostic and allow
> it to be used on non-OF platforms.
> 
> The conversion slightly changes the logic behind property reading for
> the configuration values. Original code allocates just as much memory
> as needed. Then for each separate 32- or 64-bit value it reads it from
> the property and converts to a raw one which will be fed to the sensor.
> In the new code we allocate the amount of memory needed to
> retrieve all
> values at once from the property and then convert them as required.
> 
> Signed-off-by: Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>
> ---
> v3: no changes
>  drivers/iio/temperature/ltc2983.c | 203 +++++++++++++++-------------
> --
>  1 file changed, 102 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/iio/temperature/ltc2983.c
> b/drivers/iio/temperature/ltc2983.c
> index 636bbc9011b9..d20f957ca0d9 100644
> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
> @@ -12,11 +12,15 @@
>  #include <linux/iio/iio.h>
>  #include <linux/interrupt.h>
>  #include <linux/list.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> -#include <linux/of_gpio.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/spi/spi.h>
> 
> +#include <asm/byteorder.h>
> +#include <asm/unaligned.h>
> +
>  /* register map */
>  #define LTC2983_STATUS_REG			0x0000
>  #define LTC2983_TEMP_RES_START_REG		0x0010
> @@ -219,7 +223,7 @@ struct ltc2983_sensor {
> 
>  struct ltc2983_custom_sensor {
>  	/* raw table sensor data */
> -	u8 *table;
> +	void *table;
>  	size_t size;
>  	/* address offset */
>  	s8 offset;
> @@ -377,25 +381,25 @@ static int
> __ltc2983_chan_custom_sensor_assign(struct ltc2983_data *st,
>  	return regmap_bulk_write(st->regmap, reg, custom->table,
> custom->size);
>  }
> 
> -static struct ltc2983_custom_sensor
> *__ltc2983_custom_sensor_new(
> -						struct ltc2983_data *st,
> -						const struct
> device_node *np,
> -						const char *propname,
> -						const bool is_steinhart,
> -						const u32 resolution,
> -						const bool has_signed)
> +static struct ltc2983_custom_sensor *
> +__ltc2983_custom_sensor_new(struct ltc2983_data *st, const struct
> fwnode_handle *fn,
> +			    const char *propname, const bool
> is_steinhart,
> +			    const u32 resolution, const bool has_signed)
>  {
>  	struct ltc2983_custom_sensor *new_custom;
> -	u8 index, n_entries, tbl = 0;
>  	struct device *dev = &st->spi->dev;
>  	/*
>  	 * For custom steinhart, the full u32 is taken. For all the others
>  	 * the MSB is discarded.
>  	 */
>  	const u8 n_size = is_steinhart ? 4 : 3;
> -	const u8 e_size = is_steinhart ? sizeof(u32) : sizeof(u64);
> +	u8 index, n_entries;
> +	int ret;
> 
> -	n_entries = of_property_count_elems_of_size(np, propname,
> e_size);
> +	if (is_steinhart)
> +		n_entries = fwnode_property_count_u32(fn,
> propname);
> +	else
> +		n_entries = fwnode_property_count_u64(fn,
> propname);
>  	/* n_entries must be an even number */
>  	if (!n_entries || (n_entries % 2) != 0) {
>  		dev_err(dev, "Number of entries either 0 or not
> even\n");
> @@ -423,21 +427,33 @@ static struct ltc2983_custom_sensor
> *__ltc2983_custom_sensor_new(
>  	}
> 
>  	/* allocate the table */
> -	new_custom->table = devm_kzalloc(dev, new_custom->size,
> GFP_KERNEL);
> +	if (is_steinhart)
> +		new_custom->table = devm_kcalloc(dev, n_entries,
> sizeof(u32), GFP_KERNEL);
> +	else
> +		new_custom->table = devm_kcalloc(dev, n_entries,
> sizeof(u64), GFP_KERNEL);
>  	if (!new_custom->table)
>  		return ERR_PTR(-ENOMEM);
> 
> -	for (index = 0; index < n_entries; index++) {
> -		u64 temp = 0, j;
> -		/*
> -		 * Steinhart sensors are configured with raw values in
> the
> -		 * devicetree. For the other sensors we must convert
> the
> -		 * value to raw. The odd index's correspond to
> temperarures
> -		 * and always have 1/1024 of resolution. Temperatures
> also
> -		 * come in kelvin, so signed values is not possible
> -		 */
> -		if (!is_steinhart) {
> -			of_property_read_u64_index(np, propname,
> index, &temp);
> +	/*
> +	 * Steinhart sensors are configured with raw values in the
> firmware
> +	 * node. For the other sensors we must convert the value to
> raw.
> +	 * The odd index's correspond to temperatures and always
> have 1/1024
> +	 * of resolution. Temperatures also come in Kelvin, so signed
> values
> +	 * are not possible.
> +	 */
> +	if (is_steinhart) {
> +		ret = fwnode_property_read_u32_array(fn,
> propname, new_custom->table, n_entries);
> +		if (ret < 0)
> +			return ERR_PTR(ret);
> +
> +		cpu_to_be32_array(new_custom->table,
> new_custom->table, n_entries);
> +	} else {
> +		ret = fwnode_property_read_u64_array(fn,
> propname, new_custom->table, n_entries);
> +		if (ret < 0)
> +			return ERR_PTR(ret);
> +
> +		for (index = 0; index < n_entries; index++) {
> +			u64 temp = ((u64 *)new_custom-
> >table)[index];
> 
>  			if ((index % 2) != 0)
>  				temp = __convert_to_raw(temp, 1024);
> @@ -445,16 +461,9 @@ static struct ltc2983_custom_sensor
> *__ltc2983_custom_sensor_new(
>  				temp = __convert_to_raw_sign(temp,
> resolution);
>  			else
>  				temp = __convert_to_raw(temp,
> resolution);
> -		} else {
> -			u32 t32;
> 
> -			of_property_read_u32_index(np, propname,
> index, &t32);
> -			temp = t32;
> +			put_unaligned_be24(temp, new_custom-
> >table + index * 3);
>  		}
> -
> -		for (j = 0; j < n_size; j++)
> -			new_custom->table[tbl++] =
> -				temp >> (8 * (n_size - j - 1));
>  	}
> 
>  	new_custom->is_steinhart = is_steinhart;
> @@ -597,13 +606,12 @@ static int ltc2983_adc_assign_chan(struct
> ltc2983_data *st,
>  	return __ltc2983_chan_assign_common(st, sensor, chan_val);
>  }
> 
> -static struct ltc2983_sensor *ltc2983_thermocouple_new(
> -					const struct device_node *child,
> -					struct ltc2983_data *st,
> -					const struct ltc2983_sensor
> *sensor)
> +static struct ltc2983_sensor *
> +ltc2983_thermocouple_new(const struct fwnode_handle *child,
> struct ltc2983_data *st,
> +			 const struct ltc2983_sensor *sensor)
>  {
>  	struct ltc2983_thermocouple *thermo;
> -	struct device_node *phandle;
> +	struct fwnode_handle *ref;
>  	u32 oc_current;
>  	int ret;
> 
> @@ -611,11 +619,10 @@ static struct ltc2983_sensor
> *ltc2983_thermocouple_new(
>  	if (!thermo)
>  		return ERR_PTR(-ENOMEM);
> 
> -	if (of_property_read_bool(child, "adi,single-ended"))
> +	if (fwnode_property_read_bool(child, "adi,single-ended"))
>  		thermo->sensor_config =
> LTC2983_THERMOCOUPLE_SGL(1);
> 
> -	ret = of_property_read_u32(child, "adi,sensor-oc-current-
> microamp",
> -				   &oc_current);
> +	ret = fwnode_property_read_u32(child, "adi,sensor-oc-
> current-microamp", &oc_current);
>  	if (!ret) {
>  		switch (oc_current) {
>  		case 10:
> @@ -651,10 +658,9 @@ static struct ltc2983_sensor
> *ltc2983_thermocouple_new(
>  		return ERR_PTR(-EINVAL);
>  	}
> 
> -	phandle = of_parse_phandle(child, "adi,cold-junction-handle",
> 0);
> -	if (phandle) {
> -		ret = of_property_read_u32(phandle, "reg",
> -					   &thermo-
> >cold_junction_chan);
> +	ref = fwnode_find_reference(child, "adi,cold-junction-
> handle", 0);
> +	if (ref) {

This is nok. It needs to be 'if (IS_ERR(ref))'. We then should return
ERR_CAST() in case of errors inside the if block. As this reference
is also optional, we need to nullify ref in case we don't find the
it. Otherwise fwnode_handle_put() breaks.

We also need to use ptr error logic in the other places where
fwnode_find_reference() is used. Although, in the other cases
the ref is mandatory, so there's no need to care with breaking
fwnode_handle_put().

After these changes (I think the changes are straight enough;
but I can re-test if you or Jonathan ask for it):

Tested-by: Nuno Sá <nuno.sa@analog.com>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties
  2022-03-03 13:31   ` Sa, Nuno
@ 2022-03-03 13:57     ` Andy Shevchenko
  2022-03-03 14:53       ` Sa, Nuno
  2022-03-03 14:23     ` Andy Shevchenko
  1 sibling, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2022-03-03 13:57 UTC (permalink / raw)
  To: Sa, Nuno; +Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen

On Thu, Mar 03, 2022 at 01:31:56PM +0000, Sa, Nuno wrote:
> Hi Andy,
> 
> Good that we waited to test this patch. The fundamental logic change
> for fetching and writing the custom tables are fine. That said, there
> some issues that I had to fix to test the patch. See below...

Thanks and indeed it's a good news that we caught a bug beforehand.

> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Thursday, February 10, 2022 2:55 PM

...

> > -	phandle = of_parse_phandle(child, "adi,cold-junction-handle",
> > 0);
> > -	if (phandle) {
> > -		ret = of_property_read_u32(phandle, "reg",
> > -					   &thermo-
> > >cold_junction_chan);
> > +	ref = fwnode_find_reference(child, "adi,cold-junction-
> > handle", 0);
> > +	if (ref) {
> 
> This is nok. It needs to be 'if (IS_ERR(ref))'. We then should return
> ERR_CAST() in case of errors inside the if block. As this reference
> is also optional, we need to nullify ref in case we don't find the
> it. Otherwise fwnode_handle_put() breaks.

Got it (I hope).
Lemme go through it again and issue a v4.

> We also need to use ptr error logic in the other places where
> fwnode_find_reference() is used. Although, in the other cases
> the ref is mandatory, so there's no need to care with breaking
> fwnode_handle_put().
> 
> After these changes (I think the changes are straight enough;
> but I can re-test if you or Jonathan ask for it):
> 
> Tested-by: Nuno Sá <nuno.sa@analog.com>

I think of v4 where I may add this, but still would be nice to re-review and
check if I got correctly your testing report.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties
  2022-03-03 13:31   ` Sa, Nuno
  2022-03-03 13:57     ` Andy Shevchenko
@ 2022-03-03 14:23     ` Andy Shevchenko
  2022-03-03 14:27       ` Andy Shevchenko
  1 sibling, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2022-03-03 14:23 UTC (permalink / raw)
  To: Sa, Nuno; +Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen

On Thu, Mar 03, 2022 at 01:31:56PM +0000, Sa, Nuno wrote:

...

> > +	ref = fwnode_find_reference(child, "adi,cold-junction-
> > handle", 0);
> > +	if (ref) {
> 
> This is nok. It needs to be 'if (IS_ERR(ref))'. We then should return
> ERR_CAST() in case of errors inside the if block.

This is a good catch!

> As this reference
> is also optional, we need to nullify ref in case we don't find the
> it. Otherwise fwnode_handle_put() breaks.

No, this is not correct. fwnode_handle_put() is ERR_PTR aware.

> We also need to use ptr error logic in the other places where
> fwnode_find_reference() is used. Although, in the other cases
> the ref is mandatory, so there's no need to care with breaking
> fwnode_handle_put().
> 
> After these changes (I think the changes are straight enough;
> but I can re-test if you or Jonathan ask for it):
> 
> Tested-by: Nuno Sá <nuno.sa@analog.com>
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties
  2022-03-03 14:23     ` Andy Shevchenko
@ 2022-03-03 14:27       ` Andy Shevchenko
  2022-03-03 14:52         ` Sa, Nuno
  0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2022-03-03 14:27 UTC (permalink / raw)
  To: Sa, Nuno; +Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen

On Thu, Mar 03, 2022 at 04:23:34PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 03, 2022 at 01:31:56PM +0000, Sa, Nuno wrote:
> 
> ...
> 
> > > +	ref = fwnode_find_reference(child, "adi,cold-junction-
> > > handle", 0);
> > > +	if (ref) {
> > 
> > This is nok. It needs to be 'if (IS_ERR(ref))'. We then should return
> > ERR_CAST() in case of errors inside the if block.
> 
> This is a good catch!
> 
> > As this reference
> > is also optional, we need to nullify ref in case we don't find the
> > it. Otherwise fwnode_handle_put() breaks.
> 
> No, this is not correct. fwnode_handle_put() is ERR_PTR aware.

Oh, the ->put() handles that, but the fwnode_call_void_op() doesn't!

This has to be fixed on fwnode level.

> > We also need to use ptr error logic in the other places where
> > fwnode_find_reference() is used. Although, in the other cases
> > the ref is mandatory, so there's no need to care with breaking
> > fwnode_handle_put().
> > 
> > After these changes (I think the changes are straight enough;
> > but I can re-test if you or Jonathan ask for it):
> > 
> > Tested-by: Nuno Sá <nuno.sa@analog.com>

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties
  2022-03-03 14:27       ` Andy Shevchenko
@ 2022-03-03 14:52         ` Sa, Nuno
  0 siblings, 0 replies; 13+ messages in thread
From: Sa, Nuno @ 2022-03-03 14:52 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen



> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Thursday, March 3, 2022 3:27 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Jonathan
> Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
> Subject: Re: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of
> device properties
> 
> [External]
> 
> On Thu, Mar 03, 2022 at 04:23:34PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 03, 2022 at 01:31:56PM +0000, Sa, Nuno wrote:
> >
> > ...
> >
> > > > +	ref = fwnode_find_reference(child, "adi,cold-junction-
> > > > handle", 0);
> > > > +	if (ref) {
> > >
> > > This is nok. It needs to be 'if (IS_ERR(ref))'. We then should return
> > > ERR_CAST() in case of errors inside the if block.
> >
> > This is a good catch!
> >
> > > As this reference
> > > is also optional, we need to nullify ref in case we don't find the
> > > it. Otherwise fwnode_handle_put() breaks.
> >
> > No, this is not correct. fwnode_handle_put() is ERR_PTR aware.
> 
> Oh, the ->put() handles that, but the fwnode_call_void_op() doesn't!
> 
> This has to be fixed on fwnode level.

Yeah, it was crashing. Even better if you fix there...

- Nuno Sá
 
> --
> With Best Regards,
> Andy Shevchenko
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties
  2022-03-03 13:57     ` Andy Shevchenko
@ 2022-03-03 14:53       ` Sa, Nuno
  0 siblings, 0 replies; 13+ messages in thread
From: Sa, Nuno @ 2022-03-03 14:53 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen



> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Thursday, March 3, 2022 2:58 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Jonathan
> Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
> Subject: Re: [PATCH v3 3/3] iio: temperature: ltc2983: Make use of
> device properties
> 
> [External]
> 
> On Thu, Mar 03, 2022 at 01:31:56PM +0000, Sa, Nuno wrote:
> > Hi Andy,
> >
> > Good that we waited to test this patch. The fundamental logic
> change
> > for fetching and writing the custom tables are fine. That said, there
> > some issues that I had to fix to test the patch. See below...
> 
> Thanks and indeed it's a good news that we caught a bug beforehand.
> 
> > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Sent: Thursday, February 10, 2022 2:55 PM
> 
> ...
> 
> > > -	phandle = of_parse_phandle(child, "adi,cold-junction-handle",
> > > 0);
> > > -	if (phandle) {
> > > -		ret = of_property_read_u32(phandle, "reg",
> > > -					   &thermo-
> > > >cold_junction_chan);
> > > +	ref = fwnode_find_reference(child, "adi,cold-junction-
> > > handle", 0);
> > > +	if (ref) {
> >
> > This is nok. It needs to be 'if (IS_ERR(ref))'. We then should return
> > ERR_CAST() in case of errors inside the if block. As this reference
> > is also optional, we need to nullify ref in case we don't find the
> > it. Otherwise fwnode_handle_put() breaks.
> 
> Got it (I hope).
> Lemme go through it again and issue a v4.
> 
> > We also need to use ptr error logic in the other places where
> > fwnode_find_reference() is used. Although, in the other cases
> > the ref is mandatory, so there's no need to care with breaking
> > fwnode_handle_put().
> >
> > After these changes (I think the changes are straight enough;
> > but I can re-test if you or Jonathan ask for it):
> >
> > Tested-by: Nuno Sá <nuno.sa@analog.com>
> 
> I think of v4 where I may add this, but still would be nice to re-review
> and
> check if I got correctly your testing report.

No problem... I will leave the setup up and re-test v4...

- Nuno Sá


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-03-03 14:54 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-10 13:55 [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Andy Shevchenko
2022-02-10 13:55 ` [PATCH v3 2/3] iio: temperature: ltc2983: Use single error path to put OF node Andy Shevchenko
2022-02-10 13:55 ` [PATCH v3 3/3] iio: temperature: ltc2983: Make use of device properties Andy Shevchenko
2022-03-03 13:31   ` Sa, Nuno
2022-03-03 13:57     ` Andy Shevchenko
2022-03-03 14:53       ` Sa, Nuno
2022-03-03 14:23     ` Andy Shevchenko
2022-03-03 14:27       ` Andy Shevchenko
2022-03-03 14:52         ` Sa, Nuno
2022-02-13 17:55 ` [PATCH v3 1/3] iio: temperature: ltc2983: Don't hard code defined constants in messages Jonathan Cameron
2022-02-14  9:23   ` Andy Shevchenko
2022-03-02 15:50   ` Andy Shevchenko
2022-03-02 19:56     ` Nuno Sá

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).