[PATCH 0/3] Support more parts in LTC2983

[PATCH 0/3] Support more parts in LTC2983

[Cosmin Tanislav]

Add support for the following parts:
 * LTC2984
 * LTC2986
 * LTM2985

The LTC2984 is a variant of the LTC2983 with EEPROM.
The LTC2986 is a variant of the LTC2983 with only 10 channels,
EEPROM and support for active analog temperature sensors.
The LTM2985 is software-compatible with the LTC2986.

Also, remove excessive allocations on resume.

Cosmin Tanislav (3):
  iio: temperature: ltc2983: allocate iio channels once
  dt-bindings: iio: temperature: ltc2983: support more parts
  iio: temperature: ltc2983: support more parts

 .../bindings/iio/temperature/adi,ltc2983.yaml |  63 +++++-
 drivers/iio/temperature/ltc2983.c             | 195 ++++++++++++++++--
 2 files changed, 240 insertions(+), 18 deletions(-)

-- 
2.37.3

[PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

Currently, every time the device wakes up from sleep, the
iio_chan array is reallocated, leaking the previous one
until the device is removed (basically never).

Move the allocation to the probe function to avoid this.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/iio/temperature/ltc2983.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index b652d2b39bcf..a60ccf183687 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -1385,13 +1385,6 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
 		return ret;
 	}
 
-	st->iio_chan = devm_kzalloc(&st->spi->dev,
-				    st->iio_channels * sizeof(*st->iio_chan),
-				    GFP_KERNEL);
-
-	if (!st->iio_chan)
-		return -ENOMEM;
-
 	ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG,
 				 LTC2983_NOTCH_FREQ_MASK,
 				 LTC2983_NOTCH_FREQ(st->filter_notch_freq));
@@ -1514,6 +1507,12 @@ static int ltc2983_probe(struct spi_device *spi)
 		gpiod_set_value_cansleep(gpio, 0);
 	}
 
+	st->iio_chan = devm_kzalloc(&spi->dev,
+				    st->iio_channels * sizeof(*st->iio_chan),
+				    GFP_KERNEL);
+	if (!st->iio_chan)
+		return -ENOMEM;
+
 	ret = ltc2983_setup(st, true);
 	if (ret)
 		return ret;
-- 
2.37.3

[PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

Add support for the following parts:
 * LTC2984
 * LTC2986
 * LTM2985

The LTC2984 is a variant of the LTC2983 with EEPROM.
The LTC2986 is a variant of the LTC2983 with only 10 channels,
EEPROM and support for active analog temperature sensors.
The LTM2985 is software-compatible with the LTC2986.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
 1 file changed, 59 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
index 722781aa4697..c33ab524fb64 100644
--- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
@@ -4,19 +4,27 @@
 $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Analog Devices LTC2983 Multi-sensor Temperature system
+title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
 
 maintainers:
   - Nuno Sá <nuno.sa@analog.com>
 
 description: |
-  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
+  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
+  Temperature Measurement Systems
+
   https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
 
 properties:
   compatible:
     enum:
       - adi,ltc2983
+      - adi,ltc2984
+      - adi,ltc2986
+      - adi,ltm2985
 
   reg:
     maxItems: 1
@@ -26,7 +34,7 @@ properties:
 
   adi,mux-delay-config-us:
     description:
-      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
+      The device performs 2 or 3 internal conversion cycles per temperature
       result. Each conversion cycle is performed with different excitation and
       input multiplexer configurations. Prior to each conversion, these
       excitation circuits and input switch configurations are changed and an
@@ -145,7 +153,7 @@ patternProperties:
       adi,three-conversion-cycles:
         description:
           Boolean property which set's three conversion cycles removing
-          parasitic resistance effects between the LTC2983 and the diode.
+          parasitic resistance effects between the device and the diode.
         type: boolean
 
       adi,average-on:
@@ -353,6 +361,41 @@ patternProperties:
         description: Boolean property which set's the adc as single-ended.
         type: boolean
 
+  "^temp@":
+    type: object
+    description:
+      Represents a channel which is being used as an active analog temperature
+      sensor.
+
+    properties:
+      adi,sensor-type:
+        description:
+          Identifies the sensor as an active analog temperature sensor.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        const: 31
+
+      adi,single-ended:
+        description: Boolean property which sets the sensor as single-ended.
+        type: boolean
+
+      adi,custom-temp:
+        description:
+          This is a table, where each entry should be a pair of
+          voltage(mv)-temperature(K). The entries must be given in nv and uK
+          so that, the original values must be multiplied by 1000000. For
+          more details look at table 71 and 72.
+          Note should be signed, but dtc doesn't currently maintain the
+          sign.
+        $ref: /schemas/types.yaml#/definitions/uint64-matrix
+        minItems: 3
+        maxItems: 64
+        items:
+          minItems: 2
+          maxItems: 2
+
+    required:
+      - adi,custom-temp
+
   "^rsense@":
     type: object
     description:
@@ -382,6 +425,18 @@ required:
   - reg
   - interrupts
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ltc2983
+              - adi,ltc2984
+    then:
+      patternProperties:
+        "^temp@": false
+
 additionalProperties: false
 
 examples:
-- 
2.37.3

[PATCH 3/3] iio: temperature: ltc2983: support more parts

[Cosmin Tanislav]

From: Cosmin Tanislav <cosmin.tanislav@analog.com>

Add support for the following parts:
 * LTC2984
 * LTC2986
 * LTM2985

The LTC2984 is a variant of the LTC2983 with EEPROM.
The LTC2986 is a variant of the LTC2983 with only 10 channels,
EEPROM and support for active analog temperature sensors.
The LTM2985 is software-compatible with the LTC2986.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 drivers/iio/temperature/ltc2983.c | 182 ++++++++++++++++++++++++++++--
 1 file changed, 175 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
index a60ccf183687..22977bcdd2a3 100644
--- a/drivers/iio/temperature/ltc2983.c
+++ b/drivers/iio/temperature/ltc2983.c
@@ -25,9 +25,12 @@
 #define LTC2983_STATUS_REG			0x0000
 #define LTC2983_TEMP_RES_START_REG		0x0010
 #define LTC2983_TEMP_RES_END_REG		0x005F
+#define LTC2983_EEPROM_KEY_REG			0x00B0
+#define LTC2983_EEPROM_READ_STATUS_REG		0x00D0
 #define LTC2983_GLOBAL_CONFIG_REG		0x00F0
 #define LTC2983_MULT_CHANNEL_START_REG		0x00F4
 #define LTC2983_MULT_CHANNEL_END_REG		0x00F7
+#define LTC2986_EEPROM_STATUS_REG		0x00F9
 #define LTC2983_MUX_CONFIG_REG			0x00FF
 #define LTC2983_CHAN_ASSIGN_START_REG		0x0200
 #define LTC2983_CHAN_ASSIGN_END_REG		0x024F
@@ -35,13 +38,21 @@
 #define LTC2983_CUST_SENS_TBL_END_REG		0x03CF
 
 #define LTC2983_DIFFERENTIAL_CHAN_MIN		2
-#define LTC2983_MAX_CHANNELS_NR			20
 #define LTC2983_MIN_CHANNELS_NR			1
 #define LTC2983_SLEEP				0x97
 #define LTC2983_CUSTOM_STEINHART_SIZE		24
 #define LTC2983_CUSTOM_SENSOR_ENTRY_SZ		6
 #define LTC2983_CUSTOM_STEINHART_ENTRY_SZ	4
 
+#define LTC2983_EEPROM_KEY			0xA53C0F5A
+#define LTC2983_EEPROM_WRITE_CMD		0x15
+#define LTC2983_EEPROM_READ_CMD			0x16
+#define LTC2983_EEPROM_STATUS_FAILURE_MASK	GENMASK(3, 1)
+#define LTC2983_EEPROM_READ_FAILURE_MASK	GENMASK(7, 0)
+
+#define LTC2983_EEPROM_WRITE_TIME_MS		2600
+#define LTC2983_EEPROM_READ_TIME_MS		20
+
 #define LTC2983_CHAN_START_ADDR(chan) \
 			(((chan - 1) * 4) + LTC2983_CHAN_ASSIGN_START_REG)
 #define LTC2983_CHAN_RES_ADDR(chan) \
@@ -171,6 +182,7 @@ enum {
 	LTC2983_SENSOR_DIODE = 28,
 	LTC2983_SENSOR_SENSE_RESISTOR = 29,
 	LTC2983_SENSOR_DIRECT_ADC = 30,
+	LTC2983_SENSOR_ACTIVE_TEMP = 31,
 };
 
 #define to_thermocouple(_sensor) \
@@ -191,7 +203,17 @@ enum {
 #define to_adc(_sensor) \
 		container_of(_sensor, struct ltc2983_adc, sensor)
 
+#define to_temp(_sensor) \
+		container_of(_sensor, struct ltc2983_temp, sensor)
+
+struct ltc2983_chip_info {
+	unsigned int max_channels_nr;
+	bool has_temp;
+	bool has_eeprom;
+};
+
 struct ltc2983_data {
+	const struct ltc2983_chip_info *info;
 	struct regmap *regmap;
 	struct spi_device *spi;
 	struct mutex lock;
@@ -271,6 +293,12 @@ struct ltc2983_adc {
 	bool single_ended;
 };
 
+struct ltc2983_temp {
+	struct ltc2983_sensor sensor;
+	struct ltc2983_custom_sensor *custom;
+	bool single_ended;
+};
+
 /*
  * Convert to Q format numbers. These number's are integers where
  * the number of integer and fractional bits are specified. The resolution
@@ -606,6 +634,22 @@ static int ltc2983_adc_assign_chan(struct ltc2983_data *st,
 	return __ltc2983_chan_assign_common(st, sensor, chan_val);
 }
 
+static int ltc2983_temp_assign_chan(struct ltc2983_data *st,
+				    const struct ltc2983_sensor *sensor)
+{
+	struct ltc2983_temp *temp = to_temp(sensor);
+	u32 chan_val;
+	int ret;
+
+	chan_val = LTC2983_ADC_SINGLE_ENDED(temp->single_ended);
+
+	ret = __ltc2983_chan_custom_sensor_assign(st, temp->custom, &chan_val);
+	if (ret)
+		return ret;
+
+	return __ltc2983_chan_assign_common(st, sensor, chan_val);
+}
+
 static struct ltc2983_sensor *
 ltc2983_thermocouple_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 			 const struct ltc2983_sensor *sensor)
@@ -771,10 +815,10 @@ ltc2983_rtd_new(const struct fwnode_handle *child, struct ltc2983_data *st,
 	if (rtd->sensor_config & LTC2983_RTD_4_WIRE_MASK) {
 		/* 4-wire */
 		u8 min = LTC2983_DIFFERENTIAL_CHAN_MIN,
-			max = LTC2983_MAX_CHANNELS_NR;
+			max = st->info->max_channels_nr;
 
 		if (rtd->sensor_config & LTC2983_RTD_ROTATION_MASK)
-			max = LTC2983_MAX_CHANNELS_NR - 1;
+			max = st->info->max_channels_nr - 1;
 
 		if (((rtd->sensor_config & LTC2983_RTD_KELVIN_R_SENSE_MASK)
 		     == LTC2983_RTD_KELVIN_R_SENSE_MASK) &&
@@ -1143,6 +1187,38 @@ static struct ltc2983_sensor *ltc2983_adc_new(struct fwnode_handle *child,
 	return &adc->sensor;
 }
 
+static struct ltc2983_sensor *ltc2983_temp_new(struct fwnode_handle *child,
+					       struct ltc2983_data *st,
+					       const struct ltc2983_sensor *sensor)
+{
+	struct ltc2983_temp *temp;
+
+	temp = devm_kzalloc(&st->spi->dev, sizeof(*temp), GFP_KERNEL);
+	if (!temp)
+		return ERR_PTR(-ENOMEM);
+
+	if (fwnode_property_read_bool(child, "adi,single-ended"))
+		temp->single_ended = true;
+
+	if (!temp->single_ended &&
+	    sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN) {
+		dev_err(&st->spi->dev, "Invalid chan:%d for differential temp\n",
+			sensor->chan);
+		return ERR_PTR(-EINVAL);
+	}
+
+	temp->custom = __ltc2983_custom_sensor_new(st, child, "adi,custom-temp",
+						   false, 4096, true);
+	if (IS_ERR(temp->custom))
+		return ERR_CAST(temp->custom);
+
+	/* set common parameters */
+	temp->sensor.assign_chan = ltc2983_temp_assign_chan;
+	temp->sensor.fault_handler = ltc2983_common_fault_handler;
+
+	return &temp->sensor;
+}
+
 static int ltc2983_chan_read(struct ltc2983_data *st,
 			const struct ltc2983_sensor *sensor, int *val)
 {
@@ -1302,10 +1378,10 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
 
 		/* check if we have a valid channel */
 		if (sensor.chan < LTC2983_MIN_CHANNELS_NR ||
-		    sensor.chan > LTC2983_MAX_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, LTC2983_MAX_CHANNELS_NR);
+				LTC2983_MIN_CHANNELS_NR, st->info->max_channels_nr);
 			goto put_child;
 		} else if (channel_avail_mask & BIT(sensor.chan)) {
 			ret = -EINVAL;
@@ -1345,6 +1421,9 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
 			st->iio_channels--;
 		} else if (sensor.type == LTC2983_SENSOR_DIRECT_ADC) {
 			st->sensors[chan] = ltc2983_adc_new(child, st, &sensor);
+		} else if (st->info->has_temp &&
+			   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;
@@ -1371,6 +1450,46 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
 	return ret;
 }
 
+static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
+			      unsigned int wait_time, unsigned int status_reg,
+			      unsigned long status_fail_mask)
+{
+	__be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY);
+	unsigned long time;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval,
+				sizeof(bval));
+	if (ret)
+		return ret;
+
+	reinit_completion(&st->completion);
+
+	ret = regmap_write(st->regmap, LTC2983_STATUS_REG,
+			   LTC2983_STATUS_START(true) | cmd);
+	if (ret)
+		return ret;
+
+	time = wait_for_completion_timeout(&st->completion,
+					   msecs_to_jiffies(wait_time));
+	if (!time) {
+		dev_err(&st->spi->dev, "EEPROM command timed out\n");
+		return -ETIMEDOUT;
+	}
+
+	ret = regmap_read(st->regmap, status_reg, &val);
+	if (ret)
+		return ret;
+
+	if (val & status_fail_mask) {
+		dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
 {
 	u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0, status;
@@ -1396,6 +1515,15 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
 	if (ret)
 		return ret;
 
+	if (st->info->has_eeprom && !assign_iio) {
+		ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_READ_CMD,
+					 LTC2983_EEPROM_READ_TIME_MS,
+					 LTC2983_EEPROM_READ_STATUS_REG,
+					 LTC2983_EEPROM_READ_FAILURE_MASK);
+		if (!ret)
+			return 0;
+	}
+
 	for (chan = 0; chan < st->num_channels; chan++) {
 		u32 chan_type = 0, *iio_chan;
 
@@ -1435,9 +1563,13 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
 static const struct regmap_range ltc2983_reg_ranges[] = {
 	regmap_reg_range(LTC2983_STATUS_REG, LTC2983_STATUS_REG),
 	regmap_reg_range(LTC2983_TEMP_RES_START_REG, LTC2983_TEMP_RES_END_REG),
+	regmap_reg_range(LTC2983_EEPROM_KEY_REG, LTC2983_EEPROM_KEY_REG),
+	regmap_reg_range(LTC2983_EEPROM_READ_STATUS_REG,
+			 LTC2983_EEPROM_READ_STATUS_REG),
 	regmap_reg_range(LTC2983_GLOBAL_CONFIG_REG, LTC2983_GLOBAL_CONFIG_REG),
 	regmap_reg_range(LTC2983_MULT_CHANNEL_START_REG,
 			 LTC2983_MULT_CHANNEL_END_REG),
+	regmap_reg_range(LTC2986_EEPROM_STATUS_REG, LTC2986_EEPROM_STATUS_REG),
 	regmap_reg_range(LTC2983_MUX_CONFIG_REG, LTC2983_MUX_CONFIG_REG),
 	regmap_reg_range(LTC2983_CHAN_ASSIGN_START_REG,
 			 LTC2983_CHAN_ASSIGN_END_REG),
@@ -1482,6 +1614,12 @@ 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;
+	if (!st->info)
+		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");
@@ -1524,6 +1662,15 @@ static int ltc2983_probe(struct spi_device *spi)
 		return ret;
 	}
 
+	if (st->info->has_eeprom) {
+		ret = ltc2983_eeprom_cmd(st, LTC2983_EEPROM_WRITE_CMD,
+					 LTC2983_EEPROM_WRITE_TIME_MS,
+					 LTC2986_EEPROM_STATUS_REG,
+					 LTC2983_EEPROM_STATUS_FAILURE_MASK);
+		if (ret)
+			return ret;
+	}
+
 	indio_dev->name = name;
 	indio_dev->num_channels = st->iio_channels;
 	indio_dev->channels = st->iio_chan;
@@ -1554,14 +1701,35 @@ static int ltc2983_suspend(struct device *dev)
 static DEFINE_SIMPLE_DEV_PM_OPS(ltc2983_pm_ops, ltc2983_suspend,
 				ltc2983_resume);
 
+static const struct ltc2983_chip_info ltc2983_chip_info_data = {
+	.max_channels_nr = 20,
+};
+
+static const struct ltc2983_chip_info ltc2984_chip_info_data = {
+	.max_channels_nr = 20,
+	.has_eeprom = true,
+};
+
+static const struct ltc2983_chip_info ltc2986_chip_info_data = {
+	.max_channels_nr = 10,
+	.has_temp = true,
+	.has_eeprom = true,
+};
+
 static const struct spi_device_id ltc2983_id_table[] = {
-	{ "ltc2983" },
+	{ "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 },
 	{},
 };
 MODULE_DEVICE_TABLE(spi, ltc2983_id_table);
 
 static const struct of_device_id ltc2983_of_match[] = {
-	{ .compatible = "adi,ltc2983" },
+	{ .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 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ltc2983_of_match);
-- 
2.37.3

Re: [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once

[Jonathan Cameron]

On Fri, 14 Oct 2022 15:37:22 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> Currently, every time the device wakes up from sleep, the
> iio_chan array is reallocated, leaking the previous one
> until the device is removed (basically never).
> 
> Move the allocation to the probe function to avoid this.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
Hi Cosmin,

Please give a fixes tag for this one as we'll definitely want to
backport it.

Reply to this patch is fine as b4 will pick it up like any other tag.

> ---
>  drivers/iio/temperature/ltc2983.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> index b652d2b39bcf..a60ccf183687 100644
> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
> @@ -1385,13 +1385,6 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
>  		return ret;
>  	}
>  
> -	st->iio_chan = devm_kzalloc(&st->spi->dev,
> -				    st->iio_channels * sizeof(*st->iio_chan),
> -				    GFP_KERNEL);
> -
> -	if (!st->iio_chan)
> -		return -ENOMEM;
> -
>  	ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG,
>  				 LTC2983_NOTCH_FREQ_MASK,
>  				 LTC2983_NOTCH_FREQ(st->filter_notch_freq));
> @@ -1514,6 +1507,12 @@ static int ltc2983_probe(struct spi_device *spi)
>  		gpiod_set_value_cansleep(gpio, 0);
>  	}
>  
> +	st->iio_chan = devm_kzalloc(&spi->dev,
> +				    st->iio_channels * sizeof(*st->iio_chan),
> +				    GFP_KERNEL);
> +	if (!st->iio_chan)
> +		return -ENOMEM;
> +
>  	ret = ltc2983_setup(st, true);
>  	if (ret)
>  		return ret;

Re: [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once

[Jonathan Cameron]

On Fri, 14 Oct 2022 15:11:47 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Fri, 14 Oct 2022 15:37:22 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
> > From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > 
> > Currently, every time the device wakes up from sleep, the
> > iio_chan array is reallocated, leaking the previous one
> > until the device is removed (basically never).
> > 
> > Move the allocation to the probe function to avoid this.
> > 
> > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>  
> Hi Cosmin,
> 
> Please give a fixes tag for this one as we'll definitely want to
> backport it.
> 
> Reply to this patch is fine as b4 will pick it up like any other tag.
Fixes: f110f3188e563 ("iio: temperature: Add support for LTC2983")

(from direct mail)

> 
> > ---
> >  drivers/iio/temperature/ltc2983.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> > index b652d2b39bcf..a60ccf183687 100644
> > --- a/drivers/iio/temperature/ltc2983.c
> > +++ b/drivers/iio/temperature/ltc2983.c
> > @@ -1385,13 +1385,6 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
> >  		return ret;
> >  	}
> >  
> > -	st->iio_chan = devm_kzalloc(&st->spi->dev,
> > -				    st->iio_channels * sizeof(*st->iio_chan),
> > -				    GFP_KERNEL);
> > -
> > -	if (!st->iio_chan)
> > -		return -ENOMEM;
> > -
> >  	ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG,
> >  				 LTC2983_NOTCH_FREQ_MASK,
> >  				 LTC2983_NOTCH_FREQ(st->filter_notch_freq));
> > @@ -1514,6 +1507,12 @@ static int ltc2983_probe(struct spi_device *spi)
> >  		gpiod_set_value_cansleep(gpio, 0);
> >  	}
> >  
> > +	st->iio_chan = devm_kzalloc(&spi->dev,
> > +				    st->iio_channels * sizeof(*st->iio_chan),
> > +				    GFP_KERNEL);
> > +	if (!st->iio_chan)
> > +		return -ENOMEM;
> > +
> >  	ret = ltc2983_setup(st, true);
> >  	if (ret)
> >  		return ret;  
> 
> 

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Jonathan Cameron]

On Fri, 14 Oct 2022 15:37:23 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> Add support for the following parts:
>  * LTC2984
>  * LTC2986
>  * LTM2985
> 
> The LTC2984 is a variant of the LTC2983 with EEPROM.
> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> EEPROM and support for active analog temperature sensors.

If they 'work' but with fewer features, perhaps a fallback
compatible?

> The LTM2985 is software-compatible with the LTC2986.

Fallback compatible?

> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> index 722781aa4697..c33ab524fb64 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> @@ -4,19 +4,27 @@
>  $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Analog Devices LTC2983 Multi-sensor Temperature system
> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>  
>  maintainers:
>    - Nuno Sá <nuno.sa@analog.com>
>  
>  description: |
> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
> +  Temperature Measurement Systems
> +
>    https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>  
>  properties:
>    compatible:
>      enum:
>        - adi,ltc2983
> +      - adi,ltc2984
> +      - adi,ltc2986
> +      - adi,ltm2985
>  
>    reg:
>      maxItems: 1
> @@ -26,7 +34,7 @@ properties:
>  
>    adi,mux-delay-config-us:
>      description:
> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
> +      The device performs 2 or 3 internal conversion cycles per temperature

Definitely a lesson here in avoiding device names in the descriptions!

>        result. Each conversion cycle is performed with different excitation and
>        input multiplexer configurations. Prior to each conversion, these
>        excitation circuits and input switch configurations are changed and an
> @@ -145,7 +153,7 @@ patternProperties:
>        adi,three-conversion-cycles:
>          description:
>            Boolean property which set's three conversion cycles removing
> -          parasitic resistance effects between the LTC2983 and the diode.
> +          parasitic resistance effects between the device and the diode.
>          type: boolean
>  
>        adi,average-on:
> @@ -353,6 +361,41 @@ patternProperties:
>          description: Boolean property which set's the adc as single-ended.
>          type: boolean
>  
> +  "^temp@":
> +    type: object
> +    description:
> +      Represents a channel which is being used as an active analog temperature
> +      sensor.
> +
> +    properties:
> +      adi,sensor-type:
> +        description:
> +          Identifies the sensor as an active analog temperature sensor.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        const: 31

Ah. This is a bit odd as it's fixed for the channel type.  However there
is precedence in this binding so fair enough.

> +
> +      adi,single-ended:
> +        description: Boolean property which sets the sensor as single-ended.
> +        type: boolean
> +
> +      adi,custom-temp:
> +        description:
> +          This is a table, where each entry should be a pair of
> +          voltage(mv)-temperature(K). The entries must be given in nv and uK
> +          so that, the original values must be multiplied by 1000000. For
> +          more details look at table 71 and 72.
> +          Note should be signed, but dtc doesn't currently maintain the
> +          sign.
> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> +        minItems: 3
> +        maxItems: 64
> +        items:
> +          minItems: 2
> +          maxItems: 2
> +
> +    required:
> +      - adi,custom-temp
> +
>    "^rsense@":
>      type: object
>      description:
> @@ -382,6 +425,18 @@ required:
>    - reg
>    - interrupts
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ltc2983
> +              - adi,ltc2984
> +    then:
> +      patternProperties:
> +        "^temp@": false
> +
>  additionalProperties: false
>  
>  examples:

Re: [PATCH 3/3] iio: temperature: ltc2983: support more parts

[Jonathan Cameron]

On Fri, 14 Oct 2022 15:37:24 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> Add support for the following parts:
>  * LTC2984
>  * LTC2986
>  * LTM2985
> 
> The LTC2984 is a variant of the LTC2983 with EEPROM.
> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> EEPROM and support for active analog temperature sensors.
> The LTM2985 is software-compatible with the LTC2986.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>

...

Hi Cosmin,

Looks good except, I think we are still in the position that
regmap for spi doesn't guarantee to bounce buffer the bulk accesses
(last time I checked it actually did do so, but before that it didn't
and there are obvious optimizations to take it back to not doing so -
IRC Mark Brown's answer was we shouldn't rely on it..)

Anyhow, the existing driver has instances of this so its no worse
but we should really clean those up.

Jonathan


>  
> +static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
> +			      unsigned int wait_time, unsigned int status_reg,
> +			      unsigned long status_fail_mask)
> +{
> +	__be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY);
> +	unsigned long time;
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval,
> +				sizeof(bval));

SPI device and I was clearly dozing on existing driver but normally
we avoid assuming that regmap will always use a bounce buffer for bulk
accessors. Hence this should be a DMA safe buffer.


> +	if (ret)
> +		return ret;
> +
> +	reinit_completion(&st->completion);
> +
> +	ret = regmap_write(st->regmap, LTC2983_STATUS_REG,
> +			   LTC2983_STATUS_START(true) | cmd);
> +	if (ret)
> +		return ret;
> +
> +	time = wait_for_completion_timeout(&st->completion,
> +					   msecs_to_jiffies(wait_time));
> +	if (!time) {
> +		dev_err(&st->spi->dev, "EEPROM command timed out\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = regmap_read(st->regmap, status_reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & status_fail_mask) {
> +		dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

Re: [PATCH 1/3] iio: temperature: ltc2983: allocate iio channels once

[Jonathan Cameron]

On Fri, 14 Oct 2022 16:18:24 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Fri, 14 Oct 2022 15:11:47 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Fri, 14 Oct 2022 15:37:22 +0300
> > Cosmin Tanislav <demonsingur@gmail.com> wrote:
> >   
> > > From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > > 
> > > Currently, every time the device wakes up from sleep, the
> > > iio_chan array is reallocated, leaking the previous one
> > > until the device is removed (basically never).
> > > 
> > > Move the allocation to the probe function to avoid this.
> > > 
> > > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>    
> > Hi Cosmin,
> > 
> > Please give a fixes tag for this one as we'll definitely want to
> > backport it.
> > 
> > Reply to this patch is fine as b4 will pick it up like any other tag.  
> Fixes: f110f3188e563 ("iio: temperature: Add support for LTC2983")
> 
> (from direct mail)
> 
Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> >   
> > > ---
> > >  drivers/iio/temperature/ltc2983.c | 13 ++++++-------
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> > > index b652d2b39bcf..a60ccf183687 100644
> > > --- a/drivers/iio/temperature/ltc2983.c
> > > +++ b/drivers/iio/temperature/ltc2983.c
> > > @@ -1385,13 +1385,6 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
> > >  		return ret;
> > >  	}
> > >  
> > > -	st->iio_chan = devm_kzalloc(&st->spi->dev,
> > > -				    st->iio_channels * sizeof(*st->iio_chan),
> > > -				    GFP_KERNEL);
> > > -
> > > -	if (!st->iio_chan)
> > > -		return -ENOMEM;
> > > -
> > >  	ret = regmap_update_bits(st->regmap, LTC2983_GLOBAL_CONFIG_REG,
> > >  				 LTC2983_NOTCH_FREQ_MASK,
> > >  				 LTC2983_NOTCH_FREQ(st->filter_notch_freq));
> > > @@ -1514,6 +1507,12 @@ static int ltc2983_probe(struct spi_device *spi)
> > >  		gpiod_set_value_cansleep(gpio, 0);
> > >  	}
> > >  
> > > +	st->iio_chan = devm_kzalloc(&spi->dev,
> > > +				    st->iio_channels * sizeof(*st->iio_chan),
> > > +				    GFP_KERNEL);
> > > +	if (!st->iio_chan)
> > > +		return -ENOMEM;
> > > +
> > >  	ret = ltc2983_setup(st, true);
> > >  	if (ret)
> > >  		return ret;    
> > 
> >   
> 

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Krzysztof Kozlowski]

On 14/10/2022 08:37, Cosmin Tanislav wrote:
> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> Add support for the following parts:
>  * LTC2984
>  * LTC2986
>  * LTM2985
> 
> The LTC2984 is a variant of the LTC2983 with EEPROM.
> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> EEPROM and support for active analog temperature sensors.
> The LTM2985 is software-compatible with the LTC2986.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> index 722781aa4697..c33ab524fb64 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> @@ -4,19 +4,27 @@
>  $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Analog Devices LTC2983 Multi-sensor Temperature system
> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>  
>  maintainers:
>    - Nuno Sá <nuno.sa@analog.com>
>  
>  description: |
> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
> +  Temperature Measurement Systems
> +
>    https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>  
>  properties:
>    compatible:
>      enum:
>        - adi,ltc2983
> +      - adi,ltc2984
> +      - adi,ltc2986
> +      - adi,ltm2985
>  
>    reg:
>      maxItems: 1
> @@ -26,7 +34,7 @@ properties:
>  
>    adi,mux-delay-config-us:
>      description:
> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
> +      The device performs 2 or 3 internal conversion cycles per temperature
>        result. Each conversion cycle is performed with different excitation and
>        input multiplexer configurations. Prior to each conversion, these
>        excitation circuits and input switch configurations are changed and an
> @@ -145,7 +153,7 @@ patternProperties:
>        adi,three-conversion-cycles:
>          description:
>            Boolean property which set's three conversion cycles removing
> -          parasitic resistance effects between the LTC2983 and the diode.
> +          parasitic resistance effects between the device and the diode.
>          type: boolean
>  
>        adi,average-on:
> @@ -353,6 +361,41 @@ patternProperties:
>          description: Boolean property which set's the adc as single-ended.
>          type: boolean
>  
> +  "^temp@":

There is already a property for thermocouple. Isn't a thermocouple a
temperature sensor? IOW, why new property is needed?

> +    type: object
> +    description:
> +      Represents a channel which is being used as an active analog temperature
> +      sensor.
> +
> +    properties:
> +      adi,sensor-type:
> +        description:
> +          Identifies the sensor as an active analog temperature sensor.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        const: 31
> +
> +      adi,single-ended:
> +        description: Boolean property which sets the sensor as single-ended.

Drop "Boolean property which sets" - it's obvious from the type.



> +        type: boolean
> +
> +      adi,custom-temp:
> +        description:
> +          This is a table, where each entry should be a pair of

"This is a table" - obvious from the type.

> +          voltage(mv)-temperature(K). The entries must be given in nv and uK

mv-K or nv-uK? Confusing...

> +          so that, the original values must be multiplied by 1000000. For
> +          more details look at table 71 and 72.

There is no table 71 in the bindings... It seems you pasted it from
somewhere.

> +          Note should be signed, but dtc doesn't currently maintain the
> +          sign.

What do you mean? "Maintain" as allow or keep when building FDT?  What's
the problem of using negative numbers here and why it should be part of
bindings?

> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> +        minItems: 3
> +        maxItems: 64
> +        items:
> +          minItems: 2
> +          maxItems: 2

Instead describe the items with "description" (and maybe constraints)
like here:

https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278

> +
> +    required:
> +      - adi,custom-temp
> +
>    "^rsense@":
>      type: object
>      description:
> @@ -382,6 +425,18 @@ required:
>    - reg
>    - interrupts
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ltc2983
> +              - adi,ltc2984
> +    then:
> +      patternProperties:
> +        "^temp@": false
> +
>  additionalProperties: false
>  
>  examples:

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Cosmin Tanislav]

On 10/17/22 04:59, Krzysztof Kozlowski wrote:
> On 14/10/2022 08:37, Cosmin Tanislav wrote:
>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>
>> Add support for the following parts:
>>   * LTC2984
>>   * LTC2986
>>   * LTM2985
>>
>> The LTC2984 is a variant of the LTC2983 with EEPROM.
>> The LTC2986 is a variant of the LTC2983 with only 10 channels,
>> EEPROM and support for active analog temperature sensors.
>> The LTM2985 is software-compatible with the LTC2986.
>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
>> ---
>>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>>   1 file changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> index 722781aa4697..c33ab524fb64 100644
>> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> @@ -4,19 +4,27 @@
>>   $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
>>   $schema: http://devicetree.org/meta-schemas/core.yaml#
>>   
>> -title: Analog Devices LTC2983 Multi-sensor Temperature system
>> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>>   
>>   maintainers:
>>     - Nuno Sá <nuno.sa@analog.com>
>>   
>>   description: |
>> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
>> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
>> +  Temperature Measurement Systems
>> +
>>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>>   
>>   properties:
>>     compatible:
>>       enum:
>>         - adi,ltc2983
>> +      - adi,ltc2984
>> +      - adi,ltc2986
>> +      - adi,ltm2985
>>   
>>     reg:
>>       maxItems: 1
>> @@ -26,7 +34,7 @@ properties:
>>   
>>     adi,mux-delay-config-us:
>>       description:
>> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
>> +      The device performs 2 or 3 internal conversion cycles per temperature
>>         result. Each conversion cycle is performed with different excitation and
>>         input multiplexer configurations. Prior to each conversion, these
>>         excitation circuits and input switch configurations are changed and an
>> @@ -145,7 +153,7 @@ patternProperties:
>>         adi,three-conversion-cycles:
>>           description:
>>             Boolean property which set's three conversion cycles removing
>> -          parasitic resistance effects between the LTC2983 and the diode.
>> +          parasitic resistance effects between the device and the diode.
>>           type: boolean
>>   
>>         adi,average-on:
>> @@ -353,6 +361,41 @@ patternProperties:
>>           description: Boolean property which set's the adc as single-ended.
>>           type: boolean
>>   
>> +  "^temp@":
> 
> There is already a property for thermocouple. Isn't a thermocouple a
> temperature sensor? IOW, why new property is needed?
> 
This node is needed for active analog temperature sensors.
It has fewer options than the thermocouple, as it only supports
a table to map from voltage to temperature and specifying whether
the measurement is differential or single-ended.

If you did as much as glimpsed at the datasheet you would have
understood.

>> +    type: object
>> +    description:
>> +      Represents a channel which is being used as an active analog temperature
>> +      sensor.
>> +
>> +    properties:
>> +      adi,sensor-type:
>> +        description:
>> +          Identifies the sensor as an active analog temperature sensor.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        const: 31
>> +
>> +      adi,single-ended:
>> +        description: Boolean property which sets the sensor as single-ended.
> 
> Drop "Boolean property which sets" - it's obvious from the type.
> 

That's how the rest of the file is written.

> 
> 
>> +        type: boolean
>> +
>> +      adi,custom-temp:
>> +        description:
>> +          This is a table, where each entry should be a pair of
> 
> "This is a table" - obvious from the type.
> 

That's how the rest of the file is written.

>> +          voltage(mv)-temperature(K). The entries must be given in nv and uK
> 
> mv-K or nv-uK? Confusing...

That's how the rest of the file is written.

The chip uses mv-K, but the binding specifies nv-uK, the driver
translates it into the appropriate unit.

> 
>> +          so that, the original values must be multiplied by 1000000. For
>> +          more details look at table 71 and 72.
> 
> There is no table 71 in the bindings... It seems you pasted it from
> somewhere.
> 

It's pretty obvious that "Table" in a binding refers to the datasheet.
But if you meant datasheet, not binding, you're looking at the wrong
datasheet then.
Also, that's how the rest of the file is written.

>> +          Note should be signed, but dtc doesn't currently maintain the
>> +          sign.
> 
> What do you mean? "Maintain" as allow or keep when building FDT?  What's
> the problem of using negative numbers here and why it should be part of
> bindings?
> 

You're really clueless, I'll let you figure it out on your own.
Also, that's how the rest of the file is written.

>> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
>> +        minItems: 3
>> +        maxItems: 64
>> +        items:
>> +          minItems: 2
>> +          maxItems: 2
> 
> Instead describe the items with "description" (and maybe constraints)
> like here:
> 
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> 

That's how the rest of the file is written.
If you really want to use something different, you can submit a
patch later and fix the whole binding however you want.

>> +
>> +    required:
>> +      - adi,custom-temp
>> +
>>     "^rsense@":
>>       type: object
>>       description:
>> @@ -382,6 +425,18 @@ required:
>>     - reg
>>     - interrupts
>>   
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - adi,ltc2983
>> +              - adi,ltc2984
>> +    then:
>> +      patternProperties:
>> +        "^temp@": false
>> +
>>   additionalProperties: false
>>   
>>   examples:
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 3/3] iio: temperature: ltc2983: support more parts

[Cosmin Tanislav]

On 10/14/22 18:44, Jonathan Cameron wrote:
> On Fri, 14 Oct 2022 15:37:24 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>
>> Add support for the following parts:
>>   * LTC2984
>>   * LTC2986
>>   * LTM2985
>>
>> The LTC2984 is a variant of the LTC2983 with EEPROM.
>> The LTC2986 is a variant of the LTC2983 with only 10 channels,
>> EEPROM and support for active analog temperature sensors.
>> The LTM2985 is software-compatible with the LTC2986.
>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> ...
> 
> Hi Cosmin,
> 
> Looks good except, I think we are still in the position that
> regmap for spi doesn't guarantee to bounce buffer the bulk accesses
> (last time I checked it actually did do so, but before that it didn't
> and there are obvious optimizations to take it back to not doing so -
> IRC Mark Brown's answer was we shouldn't rely on it..)
> 
> Anyhow, the existing driver has instances of this so its no worse
> but we should really clean those up.
> 
> Jonathan
> 

I can submit another patch for it. Although I'm pretty sure that
SPI regmap implementation doesn't need DMA safe access for it,
as I checked when I wrote the code.

> 
>>   
>> +static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
>> +			      unsigned int wait_time, unsigned int status_reg,
>> +			      unsigned long status_fail_mask)
>> +{
>> +	__be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY);
>> +	unsigned long time;
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval,
>> +				sizeof(bval));
> 
> SPI device and I was clearly dozing on existing driver but normally
> we avoid assuming that regmap will always use a bounce buffer for bulk
> accessors. Hence this should be a DMA safe buffer.
> 
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	reinit_completion(&st->completion);
>> +
>> +	ret = regmap_write(st->regmap, LTC2983_STATUS_REG,
>> +			   LTC2983_STATUS_START(true) | cmd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	time = wait_for_completion_timeout(&st->completion,
>> +					   msecs_to_jiffies(wait_time));
>> +	if (!time) {
>> +		dev_err(&st->spi->dev, "EEPROM command timed out\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	ret = regmap_read(st->regmap, status_reg, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (val & status_fail_mask) {
>> +		dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Cosmin Tanislav]

On 10/14/22 18:37, Jonathan Cameron wrote:
> On Fri, 14 Oct 2022 15:37:23 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>
>> Add support for the following parts:
>>   * LTC2984
>>   * LTC2986
>>   * LTM2985
>>
>> The LTC2984 is a variant of the LTC2983 with EEPROM.
>> The LTC2986 is a variant of the LTC2983 with only 10 channels,
>> EEPROM and support for active analog temperature sensors.
> 
> If they 'work' but with fewer features, perhaps a fallback
> compatible?
> 

10 channels vs 20 channels. Using ltc2983 compatible as fallback
would allow you to have 10 non-functional channels specified in the
dts.

>> The LTM2985 is software-compatible with the LTC2986.
> 
> Fallback compatible?
> 
>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
>> ---
>>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>>   1 file changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> index 722781aa4697..c33ab524fb64 100644
>> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>> @@ -4,19 +4,27 @@
>>   $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
>>   $schema: http://devicetree.org/meta-schemas/core.yaml#
>>   
>> -title: Analog Devices LTC2983 Multi-sensor Temperature system
>> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>>   
>>   maintainers:
>>     - Nuno Sá <nuno.sa@analog.com>
>>   
>>   description: |
>> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
>> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
>> +  Temperature Measurement Systems
>> +
>>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>>   
>>   properties:
>>     compatible:
>>       enum:
>>         - adi,ltc2983
>> +      - adi,ltc2984
>> +      - adi,ltc2986
>> +      - adi,ltm2985
>>   
>>     reg:
>>       maxItems: 1
>> @@ -26,7 +34,7 @@ properties:
>>   
>>     adi,mux-delay-config-us:
>>       description:
>> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
>> +      The device performs 2 or 3 internal conversion cycles per temperature
> 
> Definitely a lesson here in avoiding device names in the descriptions!
> 
>>         result. Each conversion cycle is performed with different excitation and
>>         input multiplexer configurations. Prior to each conversion, these
>>         excitation circuits and input switch configurations are changed and an
>> @@ -145,7 +153,7 @@ patternProperties:
>>         adi,three-conversion-cycles:
>>           description:
>>             Boolean property which set's three conversion cycles removing
>> -          parasitic resistance effects between the LTC2983 and the diode.
>> +          parasitic resistance effects between the device and the diode.
>>           type: boolean
>>   
>>         adi,average-on:
>> @@ -353,6 +361,41 @@ patternProperties:
>>           description: Boolean property which set's the adc as single-ended.
>>           type: boolean
>>   
>> +  "^temp@":
>> +    type: object
>> +    description:
>> +      Represents a channel which is being used as an active analog temperature
>> +      sensor.
>> +
>> +    properties:
>> +      adi,sensor-type:
>> +        description:
>> +          Identifies the sensor as an active analog temperature sensor.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        const: 31
> 
> Ah. This is a bit odd as it's fixed for the channel type.  However there
> is precedence in this binding so fair enough.
> 

I think it

>> +
>> +      adi,single-ended:
>> +        description: Boolean property which sets the sensor as single-ended.
>> +        type: boolean
>> +
>> +      adi,custom-temp:
>> +        description:
>> +          This is a table, where each entry should be a pair of
>> +          voltage(mv)-temperature(K). The entries must be given in nv and uK
>> +          so that, the original values must be multiplied by 1000000. For
>> +          more details look at table 71 and 72.
>> +          Note should be signed, but dtc doesn't currently maintain the
>> +          sign.
>> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
>> +        minItems: 3
>> +        maxItems: 64
>> +        items:
>> +          minItems: 2
>> +          maxItems: 2
>> +
>> +    required:
>> +      - adi,custom-temp
>> +
>>     "^rsense@":
>>       type: object
>>       description:
>> @@ -382,6 +425,18 @@ required:
>>     - reg
>>     - interrupts
>>   
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - adi,ltc2983
>> +              - adi,ltc2984
>> +    then:
>> +      patternProperties:
>> +        "^temp@": false
>> +
>>   additionalProperties: false
>>   
>>   examples:
> 

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Nuno Sá]

Hi Krzysztof,

As I wrote the original bindings, let me reply to some of your
points...

On Sun, 2022-10-16 at 21:59 -0400, Krzysztof Kozlowski wrote:
> On 14/10/2022 08:37, Cosmin Tanislav wrote:
> > From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > 
> > Add support for the following parts:
> >  * LTC2984
> >  * LTC2986
> >  * LTM2985
> > 
> > The LTC2984 is a variant of the LTC2983 with EEPROM.
> > The LTC2986 is a variant of the LTC2983 with only 10 channels,
> > EEPROM and support for active analog temperature sensors.
> > The LTM2985 is software-compatible with the LTC2986.
> > 
> > Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > ---
> >  .../bindings/iio/temperature/adi,ltc2983.yaml | 63
> > +++++++++++++++++--
> >  1 file changed, 59 insertions(+), 4 deletions(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam
> > l
> > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam
> > l
> > index 722781aa4697..c33ab524fb64 100644
> > ---
> > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam
> > l
> > +++
> > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yam
> > l
> > @@ -4,19 +4,27 @@
> >  $id:
> > http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
> >  $schema: http://devicetree.org/meta-schemas/core.yaml#
> >  
> > -title: Analog Devices LTC2983 Multi-sensor Temperature system
> > +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor
> > Temperature system
> >  
> >  maintainers:
> >    - Nuno Sá <nuno.sa@analog.com>
> >  
> >  description: |
> > -  Analog Devices LTC2983 Multi-Sensor Digital Temperature
> > Measurement System
> > +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor
> > Digital
> > +  Temperature Measurement Systems
> > +
> >   
> > https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
> >  
> >  properties:
> >    compatible:
> >      enum:
> >        - adi,ltc2983
> > +      - adi,ltc2984
> > +      - adi,ltc2986
> > +      - adi,ltm2985
> >  
> >    reg:
> >      maxItems: 1
> > @@ -26,7 +34,7 @@ properties:
> >  
> >    adi,mux-delay-config-us:
> >      description:
> > -      The LTC2983 performs 2 or 3 internal conversion cycles per
> > temperature
> > +      The device performs 2 or 3 internal conversion cycles per
> > temperature
> >        result. Each conversion cycle is performed with different
> > excitation and
> >        input multiplexer configurations. Prior to each conversion,
> > these
> >        excitation circuits and input switch configurations are
> > changed and an
> > @@ -145,7 +153,7 @@ patternProperties:
> >        adi,three-conversion-cycles:
> >          description:
> >            Boolean property which set's three conversion cycles
> > removing
> > -          parasitic resistance effects between the LTC2983 and the
> > diode.
> > +          parasitic resistance effects between the device and the
> > diode.
> >          type: boolean
> >  
> >        adi,average-on:
> > @@ -353,6 +361,41 @@ patternProperties:
> >          description: Boolean property which set's the adc as
> > single-ended.
> >          type: boolean
> >  
> > +  "^temp@":
> 
> There is already a property for thermocouple. Isn't a thermocouple a
> temperature sensor? IOW, why new property is needed?
> 

Well, most of the patternProps in this bindings are temperature
sensors... It's just that the device(s) support different types of
them. 'adi,sensor-type' is used to identify each sensor (as this
translates in different configurations being written in the device
channels).

> > +    type: object
> > +    description:
> > +      Represents a channel which is being used as an active analog
> > temperature
> > +      sensor.
> > +
> > +    properties:
> > +      adi,sensor-type:
> > +        description:
> > +          Identifies the sensor as an active analog temperature
> > sensor.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        const: 31
> > +
> > +      adi,single-ended:
> > +        description: Boolean property which sets the sensor as
> > single-ended.
> 
> Drop "Boolean property which sets" - it's obvious from the type.
> 
> 
> 
> > +        type: boolean
> > +
> > +      adi,custom-temp:
> > +        description:
> > +          This is a table, where each entry should be a pair of
> 
> "This is a table" - obvious from the type.
> 
> > +          voltage(mv)-temperature(K). The entries must be given in
> > nv and uK
> 
> mv-K or nv-uK? Confusing...

Yeah, a bit. In Cosmin defense, I think he's just keeping the same
"style" as the rest of the properties...

> 
> > +          so that, the original values must be multiplied by
> > 1000000. For
> > +          more details look at table 71 and 72.
> 
> There is no table 71 in the bindings... It seems you pasted it from
> somewhere.

I'm fairly sure this refers to the datasheet. I see now that this can
be confusing (again this kind of references are being (ab)used in the
rest of the file).

> 
> > +          Note should be signed, but dtc doesn't currently
> > maintain the
> > +          sign.
> 
> What do you mean? "Maintain" as allow or keep when building FDT? 
> What's
> the problem of using negative numbers here and why it should be part
> of
> bindings?
> 
> > +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> > +        minItems: 3
> > +        maxItems: 64
> > +        items:
> > +          minItems: 2
> > +          maxItems: 2
> 
> Instead describe the items with "description" (and maybe constraints)
> like here:
> 
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> 

Neat... My only comment (which probably applies to my previous ones) is
that the rest of the properties are already in this "style". So maybe,
follow up patches with small clean-ups would be more appropriate?
> 


- Nuno Sá

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Nuno Sá]

On Mon, 2022-10-17 at 11:38 +0200, Nuno Sá wrote:
> Hi Krzysztof,
> 
> As I wrote the original bindings, let me reply to some of your
> points...
> 
> 

Just realized now there were already some replies (forgot to 'lei up'
before replying...)

- Nuno Sá

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Jonathan Cameron]

On Mon, 17 Oct 2022 10:01:03 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> On 10/14/22 18:37, Jonathan Cameron wrote:
> > On Fri, 14 Oct 2022 15:37:23 +0300
> > Cosmin Tanislav <demonsingur@gmail.com> wrote:
> >   
> >> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> >>
> >> Add support for the following parts:
> >>   * LTC2984
> >>   * LTC2986
> >>   * LTM2985
> >>
> >> The LTC2984 is a variant of the LTC2983 with EEPROM.
> >> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> >> EEPROM and support for active analog temperature sensors.  
> > 
> > If they 'work' but with fewer features, perhaps a fallback
> > compatible?
> >   
> 
> 10 channels vs 20 channels. Using ltc2983 compatible as fallback
> would allow you to have 10 non-functional channels specified in the
> dts.

Ah. That one probably doesn't make sense then.  The 2984?

> 
> >> The LTM2985 is software-compatible with the LTC2986.  
> > 
> > Fallback compatible?
> >   
> >>
> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> >> ---
> >>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
> >>   1 file changed, 59 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> >> index 722781aa4697..c33ab524fb64 100644
> >> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> >> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> >> @@ -4,19 +4,27 @@
> >>   $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
> >>   $schema: http://devicetree.org/meta-schemas/core.yaml#
> >>   
> >> -title: Analog Devices LTC2983 Multi-sensor Temperature system
> >> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
> >>   
> >>   maintainers:
> >>     - Nuno Sá <nuno.sa@analog.com>
> >>   
> >>   description: |
> >> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
> >> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
> >> +  Temperature Measurement Systems
> >> +
> >>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
> >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
> >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
> >>   
> >>   properties:
> >>     compatible:
> >>       enum:
> >>         - adi,ltc2983
> >> +      - adi,ltc2984
> >> +      - adi,ltc2986
> >> +      - adi,ltm2985
> >>   
> >>     reg:
> >>       maxItems: 1
> >> @@ -26,7 +34,7 @@ properties:
> >>   
> >>     adi,mux-delay-config-us:
> >>       description:
> >> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
> >> +      The device performs 2 or 3 internal conversion cycles per temperature  
> > 
> > Definitely a lesson here in avoiding device names in the descriptions!
> >   
> >>         result. Each conversion cycle is performed with different excitation and
> >>         input multiplexer configurations. Prior to each conversion, these
> >>         excitation circuits and input switch configurations are changed and an
> >> @@ -145,7 +153,7 @@ patternProperties:
> >>         adi,three-conversion-cycles:
> >>           description:
> >>             Boolean property which set's three conversion cycles removing
> >> -          parasitic resistance effects between the LTC2983 and the diode.
> >> +          parasitic resistance effects between the device and the diode.
> >>           type: boolean
> >>   
> >>         adi,average-on:
> >> @@ -353,6 +361,41 @@ patternProperties:
> >>           description: Boolean property which set's the adc as single-ended.
> >>           type: boolean
> >>   
> >> +  "^temp@":
> >> +    type: object
> >> +    description:
> >> +      Represents a channel which is being used as an active analog temperature
> >> +      sensor.
> >> +
> >> +    properties:
> >> +      adi,sensor-type:
> >> +        description:
> >> +          Identifies the sensor as an active analog temperature sensor.
> >> +        $ref: /schemas/types.yaml#/definitions/uint32
> >> +        const: 31  
> > 
> > Ah. This is a bit odd as it's fixed for the channel type.  However there
> > is precedence in this binding so fair enough.
> >   
> 
> I think it
> 
> >> +
> >> +      adi,single-ended:
> >> +        description: Boolean property which sets the sensor as single-ended.
> >> +        type: boolean
> >> +
> >> +      adi,custom-temp:
> >> +        description:
> >> +          This is a table, where each entry should be a pair of
> >> +          voltage(mv)-temperature(K). The entries must be given in nv and uK
> >> +          so that, the original values must be multiplied by 1000000. For
> >> +          more details look at table 71 and 72.
> >> +          Note should be signed, but dtc doesn't currently maintain the
> >> +          sign.
> >> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> >> +        minItems: 3
> >> +        maxItems: 64
> >> +        items:
> >> +          minItems: 2
> >> +          maxItems: 2
> >> +
> >> +    required:
> >> +      - adi,custom-temp
> >> +
> >>     "^rsense@":
> >>       type: object
> >>       description:
> >> @@ -382,6 +425,18 @@ required:
> >>     - reg
> >>     - interrupts
> >>   
> >> +allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            enum:
> >> +              - adi,ltc2983
> >> +              - adi,ltc2984
> >> +    then:
> >> +      patternProperties:
> >> +        "^temp@": false
> >> +
> >>   additionalProperties: false
> >>   
> >>   examples:  
> >   

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Jonathan Cameron]

On Mon, 17 Oct 2022 09:53:40 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> On 10/17/22 04:59, Krzysztof Kozlowski wrote:
> > On 14/10/2022 08:37, Cosmin Tanislav wrote:  
> >> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> >>
> >> Add support for the following parts:
> >>   * LTC2984
> >>   * LTC2986
> >>   * LTM2985
> >>
> >> The LTC2984 is a variant of the LTC2983 with EEPROM.
> >> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> >> EEPROM and support for active analog temperature sensors.
> >> The LTM2985 is software-compatible with the LTC2986.
> >>
> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>

Cosmin,

Please take care to keep things polite. Krzysztof is reviewing an
enormous number of bindings and is very helpful indeed. We should
respect his feedback and respond appropriately. Sometimes what is
'obvious' to one person may need a clarifying comment or explanation
for another.

Jonathan


> >> ---
> >>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
> >>   1 file changed, 59 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> >> index 722781aa4697..c33ab524fb64 100644
> >> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> >> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> >> @@ -4,19 +4,27 @@
> >>   $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
> >>   $schema: http://devicetree.org/meta-schemas/core.yaml#
> >>   
> >> -title: Analog Devices LTC2983 Multi-sensor Temperature system
> >> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
> >>   
> >>   maintainers:
> >>     - Nuno Sá <nuno.sa@analog.com>
> >>   
> >>   description: |
> >> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
> >> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
> >> +  Temperature Measurement Systems
> >> +
> >>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
> >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
> >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
> >>   
> >>   properties:
> >>     compatible:
> >>       enum:
> >>         - adi,ltc2983
> >> +      - adi,ltc2984
> >> +      - adi,ltc2986
> >> +      - adi,ltm2985
> >>   
> >>     reg:
> >>       maxItems: 1
> >> @@ -26,7 +34,7 @@ properties:
> >>   
> >>     adi,mux-delay-config-us:
> >>       description:
> >> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
> >> +      The device performs 2 or 3 internal conversion cycles per temperature
> >>         result. Each conversion cycle is performed with different excitation and
> >>         input multiplexer configurations. Prior to each conversion, these
> >>         excitation circuits and input switch configurations are changed and an
> >> @@ -145,7 +153,7 @@ patternProperties:
> >>         adi,three-conversion-cycles:
> >>           description:
> >>             Boolean property which set's three conversion cycles removing
> >> -          parasitic resistance effects between the LTC2983 and the diode.
> >> +          parasitic resistance effects between the device and the diode.
> >>           type: boolean
> >>   
> >>         adi,average-on:
> >> @@ -353,6 +361,41 @@ patternProperties:
> >>           description: Boolean property which set's the adc as single-ended.
> >>           type: boolean
> >>   
> >> +  "^temp@":  
> > 
> > There is already a property for thermocouple. Isn't a thermocouple a
> > temperature sensor? IOW, why new property is needed?
> >   
> This node is needed for active analog temperature sensors.
> It has fewer options than the thermocouple, as it only supports
> a table to map from voltage to temperature and specifying whether
> the measurement is differential or single-ended.
> 
> If you did as much as glimpsed at the datasheet you would have
> understood.
> 
> >> +    type: object
> >> +    description:
> >> +      Represents a channel which is being used as an active analog temperature
> >> +      sensor.
> >> +
> >> +    properties:
> >> +      adi,sensor-type:
> >> +        description:
> >> +          Identifies the sensor as an active analog temperature sensor.
> >> +        $ref: /schemas/types.yaml#/definitions/uint32
> >> +        const: 31
> >> +
> >> +      adi,single-ended:
> >> +        description: Boolean property which sets the sensor as single-ended.  
> > 
> > Drop "Boolean property which sets" - it's obvious from the type.
> >   
> 
> That's how the rest of the file is written.
> 
> > 
> >   
> >> +        type: boolean
> >> +
> >> +      adi,custom-temp:
> >> +        description:
> >> +          This is a table, where each entry should be a pair of  
> > 
> > "This is a table" - obvious from the type.
> >   
> 
> That's how the rest of the file is written.
> 
> >> +          voltage(mv)-temperature(K). The entries must be given in nv and uK  
> > 
> > mv-K or nv-uK? Confusing...  
> 
> That's how the rest of the file is written.
> 
> The chip uses mv-K, but the binding specifies nv-uK, the driver
> translates it into the appropriate unit.
> 
> >   
> >> +          so that, the original values must be multiplied by 1000000. For
> >> +          more details look at table 71 and 72.  
> > 
> > There is no table 71 in the bindings... It seems you pasted it from
> > somewhere.
> >   
> 
> It's pretty obvious that "Table" in a binding refers to the datasheet.
> But if you meant datasheet, not binding, you're looking at the wrong
> datasheet then.
> Also, that's how the rest of the file is written.
> 
> >> +          Note should be signed, but dtc doesn't currently maintain the
> >> +          sign.  
> > 
> > What do you mean? "Maintain" as allow or keep when building FDT?  What's
> > the problem of using negative numbers here and why it should be part of
> > bindings?
> >   
> 
> You're really clueless, I'll let you figure it out on your own.
> Also, that's how the rest of the file is written.

> 
> >> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> >> +        minItems: 3
> >> +        maxItems: 64
> >> +        items:
> >> +          minItems: 2
> >> +          maxItems: 2  
> > 
> > Instead describe the items with "description" (and maybe constraints)
> > like here:
> > 
> > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> >   
> 
> That's how the rest of the file is written.
> If you really want to use something different, you can submit a
> patch later and fix the whole binding however you want.
> 
> >> +
> >> +    required:
> >> +      - adi,custom-temp
> >> +
> >>     "^rsense@":
> >>       type: object
> >>       description:
> >> @@ -382,6 +425,18 @@ required:
> >>     - reg
> >>     - interrupts
> >>   
> >> +allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            enum:
> >> +              - adi,ltc2983
> >> +              - adi,ltc2984
> >> +    then:
> >> +      patternProperties:
> >> +        "^temp@": false
> >> +
> >>   additionalProperties: false
> >>   
> >>   examples:  
> > 
> > Best regards,
> > Krzysztof
> >   

Re: [PATCH 3/3] iio: temperature: ltc2983: support more parts

[Jonathan Cameron]

On Mon, 17 Oct 2022 09:59:27 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> On 10/14/22 18:44, Jonathan Cameron wrote:
> > On Fri, 14 Oct 2022 15:37:24 +0300
> > Cosmin Tanislav <demonsingur@gmail.com> wrote:
> >   
> >> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> >>
> >> Add support for the following parts:
> >>   * LTC2984
> >>   * LTC2986
> >>   * LTM2985
> >>
> >> The LTC2984 is a variant of the LTC2983 with EEPROM.
> >> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> >> EEPROM and support for active analog temperature sensors.
> >> The LTM2985 is software-compatible with the LTC2986.
> >>
> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>  
> > 
> > ...
> > 
> > Hi Cosmin,
> > 
> > Looks good except, I think we are still in the position that
> > regmap for spi doesn't guarantee to bounce buffer the bulk accesses
> > (last time I checked it actually did do so, but before that it didn't
> > and there are obvious optimizations to take it back to not doing so -
> > IRC Mark Brown's answer was we shouldn't rely on it..)
> > 
> > Anyhow, the existing driver has instances of this so its no worse
> > but we should really clean those up.
> > 
> > Jonathan
> >   
> 
> I can submit another patch for it. Although I'm pretty sure that
> SPI regmap implementation doesn't need DMA safe access for it,
> as I checked when I wrote the code.

It doesn't today (last time I checked it didn't anyway and that matches
with what you found), but it did in the past and there are no guarantees
it won't require DMA safe buffers in the future.  I doubt I'll find the
reference but we had a discussion with Mark Brown about this for a similar
case a year or two back and he confirmed we should continue to assume
that DMA safe buffers should be used. 

Jonathan

> 
> >   
> >>   
> >> +static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
> >> +			      unsigned int wait_time, unsigned int status_reg,
> >> +			      unsigned long status_fail_mask)
> >> +{
> >> +	__be32 bval = cpu_to_be32(LTC2983_EEPROM_KEY);
> >> +	unsigned long time;
> >> +	unsigned int val;
> >> +	int ret;
> >> +
> >> +	ret = regmap_bulk_write(st->regmap, LTC2983_EEPROM_KEY_REG, &bval,
> >> +				sizeof(bval));  
> > 
> > SPI device and I was clearly dozing on existing driver but normally
> > we avoid assuming that regmap will always use a bounce buffer for bulk
> > accessors. Hence this should be a DMA safe buffer.
> > 
> >   
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	reinit_completion(&st->completion);
> >> +
> >> +	ret = regmap_write(st->regmap, LTC2983_STATUS_REG,
> >> +			   LTC2983_STATUS_START(true) | cmd);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	time = wait_for_completion_timeout(&st->completion,
> >> +					   msecs_to_jiffies(wait_time));
> >> +	if (!time) {
> >> +		dev_err(&st->spi->dev, "EEPROM command timed out\n");
> >> +		return -ETIMEDOUT;
> >> +	}
> >> +
> >> +	ret = regmap_read(st->regmap, status_reg, &val);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (val & status_fail_mask) {
> >> +		dev_err(&st->spi->dev, "EEPROM command failed: 0x%02X\n", val);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +  
> >   

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Jonathan Cameron]

On Mon, 17 Oct 2022 11:26:56 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Mon, 17 Oct 2022 09:53:40 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
> > On 10/17/22 04:59, Krzysztof Kozlowski wrote:  
> > > On 14/10/2022 08:37, Cosmin Tanislav wrote:    
> > >> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
> > >>
> > >> Add support for the following parts:
> > >>   * LTC2984
> > >>   * LTC2986
> > >>   * LTM2985
> > >>
> > >> The LTC2984 is a variant of the LTC2983 with EEPROM.
> > >> The LTC2986 is a variant of the LTC2983 with only 10 channels,
> > >> EEPROM and support for active analog temperature sensors.
> > >> The LTM2985 is software-compatible with the LTC2986.
> > >>
> > >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>  
> 
> Cosmin,
> 
> Please take care to keep things polite. Krzysztof is reviewing an
> enormous number of bindings and is very helpful indeed. We should
> respect his feedback and respond appropriately. Sometimes what is
> 'obvious' to one person may need a clarifying comment or explanation
> for another.

Note I should have said we should respect ALL reviewers for taking
the time to look at our code.  Not just the particularly active ones!

Jonathan

> 
> Jonathan
> 
> 
> > >> ---
> > >>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
> > >>   1 file changed, 59 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > >> index 722781aa4697..c33ab524fb64 100644
> > >> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > >> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > >> @@ -4,19 +4,27 @@
> > >>   $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
> > >>   $schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>   
> > >> -title: Analog Devices LTC2983 Multi-sensor Temperature system
> > >> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
> > >>   
> > >>   maintainers:
> > >>     - Nuno Sá <nuno.sa@analog.com>
> > >>   
> > >>   description: |
> > >> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
> > >> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
> > >> +  Temperature Measurement Systems
> > >> +
> > >>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
> > >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
> > >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
> > >> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
> > >>   
> > >>   properties:
> > >>     compatible:
> > >>       enum:
> > >>         - adi,ltc2983
> > >> +      - adi,ltc2984
> > >> +      - adi,ltc2986
> > >> +      - adi,ltm2985
> > >>   
> > >>     reg:
> > >>       maxItems: 1
> > >> @@ -26,7 +34,7 @@ properties:
> > >>   
> > >>     adi,mux-delay-config-us:
> > >>       description:
> > >> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
> > >> +      The device performs 2 or 3 internal conversion cycles per temperature
> > >>         result. Each conversion cycle is performed with different excitation and
> > >>         input multiplexer configurations. Prior to each conversion, these
> > >>         excitation circuits and input switch configurations are changed and an
> > >> @@ -145,7 +153,7 @@ patternProperties:
> > >>         adi,three-conversion-cycles:
> > >>           description:
> > >>             Boolean property which set's three conversion cycles removing
> > >> -          parasitic resistance effects between the LTC2983 and the diode.
> > >> +          parasitic resistance effects between the device and the diode.
> > >>           type: boolean
> > >>   
> > >>         adi,average-on:
> > >> @@ -353,6 +361,41 @@ patternProperties:
> > >>           description: Boolean property which set's the adc as single-ended.
> > >>           type: boolean
> > >>   
> > >> +  "^temp@":    
> > > 
> > > There is already a property for thermocouple. Isn't a thermocouple a
> > > temperature sensor? IOW, why new property is needed?
> > >     
> > This node is needed for active analog temperature sensors.
> > It has fewer options than the thermocouple, as it only supports
> > a table to map from voltage to temperature and specifying whether
> > the measurement is differential or single-ended.
> > 
> > If you did as much as glimpsed at the datasheet you would have
> > understood.
> >   
> > >> +    type: object
> > >> +    description:
> > >> +      Represents a channel which is being used as an active analog temperature
> > >> +      sensor.
> > >> +
> > >> +    properties:
> > >> +      adi,sensor-type:
> > >> +        description:
> > >> +          Identifies the sensor as an active analog temperature sensor.
> > >> +        $ref: /schemas/types.yaml#/definitions/uint32
> > >> +        const: 31
> > >> +
> > >> +      adi,single-ended:
> > >> +        description: Boolean property which sets the sensor as single-ended.    
> > > 
> > > Drop "Boolean property which sets" - it's obvious from the type.
> > >     
> > 
> > That's how the rest of the file is written.
> >   
> > > 
> > >     
> > >> +        type: boolean
> > >> +
> > >> +      adi,custom-temp:
> > >> +        description:
> > >> +          This is a table, where each entry should be a pair of    
> > > 
> > > "This is a table" - obvious from the type.
> > >     
> > 
> > That's how the rest of the file is written.
> >   
> > >> +          voltage(mv)-temperature(K). The entries must be given in nv and uK    
> > > 
> > > mv-K or nv-uK? Confusing...    
> > 
> > That's how the rest of the file is written.
> > 
> > The chip uses mv-K, but the binding specifies nv-uK, the driver
> > translates it into the appropriate unit.
> >   
> > >     
> > >> +          so that, the original values must be multiplied by 1000000. For
> > >> +          more details look at table 71 and 72.    
> > > 
> > > There is no table 71 in the bindings... It seems you pasted it from
> > > somewhere.
> > >     
> > 
> > It's pretty obvious that "Table" in a binding refers to the datasheet.
> > But if you meant datasheet, not binding, you're looking at the wrong
> > datasheet then.
> > Also, that's how the rest of the file is written.
> >   
> > >> +          Note should be signed, but dtc doesn't currently maintain the
> > >> +          sign.    
> > > 
> > > What do you mean? "Maintain" as allow or keep when building FDT?  What's
> > > the problem of using negative numbers here and why it should be part of
> > > bindings?
> > >     
> > 
> > You're really clueless, I'll let you figure it out on your own.
> > Also, that's how the rest of the file is written.  
> 
> >   
> > >> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> > >> +        minItems: 3
> > >> +        maxItems: 64
> > >> +        items:
> > >> +          minItems: 2
> > >> +          maxItems: 2    
> > > 
> > > Instead describe the items with "description" (and maybe constraints)
> > > like here:
> > > 
> > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> > >     
> > 
> > That's how the rest of the file is written.
> > If you really want to use something different, you can submit a
> > patch later and fix the whole binding however you want.
> >   
> > >> +
> > >> +    required:
> > >> +      - adi,custom-temp
> > >> +
> > >>     "^rsense@":
> > >>       type: object
> > >>       description:
> > >> @@ -382,6 +425,18 @@ required:
> > >>     - reg
> > >>     - interrupts
> > >>   
> > >> +allOf:
> > >> +  - if:
> > >> +      properties:
> > >> +        compatible:
> > >> +          contains:
> > >> +            enum:
> > >> +              - adi,ltc2983
> > >> +              - adi,ltc2984
> > >> +    then:
> > >> +      patternProperties:
> > >> +        "^temp@": false
> > >> +
> > >>   additionalProperties: false
> > >>   
> > >>   examples:    
> > > 
> > > Best regards,
> > > Krzysztof
> > >     
> 
> 

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Krzysztof Kozlowski]

On 17/10/2022 02:53, Cosmin Tanislav wrote:
> 
> 
> On 10/17/22 04:59, Krzysztof Kozlowski wrote:
>> On 14/10/2022 08:37, Cosmin Tanislav wrote:
>>> From: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>>
>>> Add support for the following parts:
>>>   * LTC2984
>>>   * LTC2986
>>>   * LTM2985
>>>
>>> The LTC2984 is a variant of the LTC2983 with EEPROM.
>>> The LTC2986 is a variant of the LTC2983 with only 10 channels,
>>> EEPROM and support for active analog temperature sensors.
>>> The LTM2985 is software-compatible with the LTC2986.
>>>
>>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
>>> ---
>>>   .../bindings/iio/temperature/adi,ltc2983.yaml | 63 +++++++++++++++++--
>>>   1 file changed, 59 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>>> index 722781aa4697..c33ab524fb64 100644
>>> --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
>>> @@ -4,19 +4,27 @@
>>>   $id: http://devicetree.org/schemas/iio/temperature/adi,ltc2983.yaml#
>>>   $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>   
>>> -title: Analog Devices LTC2983 Multi-sensor Temperature system
>>> +title: Analog Devices LTC2983, LTC2986, LTM2985 Multi-sensor Temperature system
>>>   
>>>   maintainers:
>>>     - Nuno Sá <nuno.sa@analog.com>
>>>   
>>>   description: |
>>> -  Analog Devices LTC2983 Multi-Sensor Digital Temperature Measurement System
>>> +  Analog Devices LTC2983, LTC2984, LTC2986, LTM2985 Multi-Sensor Digital
>>> +  Temperature Measurement Systems
>>> +
>>>     https://www.analog.com/media/en/technical-documentation/data-sheets/2983fc.pdf
>>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/2984fb.pdf
>>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/29861fa.pdf
>>> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ltm2985.pdf
>>>   
>>>   properties:
>>>     compatible:
>>>       enum:
>>>         - adi,ltc2983
>>> +      - adi,ltc2984
>>> +      - adi,ltc2986
>>> +      - adi,ltm2985
>>>   
>>>     reg:
>>>       maxItems: 1
>>> @@ -26,7 +34,7 @@ properties:
>>>   
>>>     adi,mux-delay-config-us:
>>>       description:
>>> -      The LTC2983 performs 2 or 3 internal conversion cycles per temperature
>>> +      The device performs 2 or 3 internal conversion cycles per temperature
>>>         result. Each conversion cycle is performed with different excitation and
>>>         input multiplexer configurations. Prior to each conversion, these
>>>         excitation circuits and input switch configurations are changed and an
>>> @@ -145,7 +153,7 @@ patternProperties:
>>>         adi,three-conversion-cycles:
>>>           description:
>>>             Boolean property which set's three conversion cycles removing
>>> -          parasitic resistance effects between the LTC2983 and the diode.
>>> +          parasitic resistance effects between the device and the diode.
>>>           type: boolean
>>>   
>>>         adi,average-on:
>>> @@ -353,6 +361,41 @@ patternProperties:
>>>           description: Boolean property which set's the adc as single-ended.
>>>           type: boolean
>>>   
>>> +  "^temp@":
>>
>> There is already a property for thermocouple. Isn't a thermocouple a
>> temperature sensor? IOW, why new property is needed?
>>
> This node is needed for active analog temperature sensors.
> It has fewer options than the thermocouple, as it only supports
> a table to map from voltage to temperature and specifying whether
> the measurement is differential or single-ended.
> 
> If you did as much as glimpsed at the datasheet you would have
> understood.

We receive a lot of bindings to review. If I glimpse through every
datasheet, when would I work?

Instead of expecting reviewer to dive into datasheets for this one
particular sensor, make it explicit in commit msg.

> 
>>> +    type: object
>>> +    description:
>>> +      Represents a channel which is being used as an active analog temperature
>>> +      sensor.
>>> +
>>> +    properties:
>>> +      adi,sensor-type:
>>> +        description:
>>> +          Identifies the sensor as an active analog temperature sensor.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        const: 31
>>> +
>>> +      adi,single-ended:
>>> +        description: Boolean property which sets the sensor as single-ended.
>>
>> Drop "Boolean property which sets" - it's obvious from the type.
>>
> 
> That's how the rest of the file is written.

Not really an argument... You can correct the other pieces in separate
patch.

> 
>>
>>
>>> +        type: boolean
>>> +
>>> +      adi,custom-temp:
>>> +        description:
>>> +          This is a table, where each entry should be a pair of
>>
>> "This is a table" - obvious from the type.
>>
> 
> That's how the rest of the file is written.
> 
>>> +          voltage(mv)-temperature(K). The entries must be given in nv and uK
>>
>> mv-K or nv-uK? Confusing...
> 
> That's how the rest of the file is written.

The same.

> 
> The chip uses mv-K, but the binding specifies nv-uK, the driver
> translates it into the appropriate unit.

It does not matter here, what the driver is doing. Use only one unit
here, matching the DTS.

> 
>>
>>> +          so that, the original values must be multiplied by 1000000. For
>>> +          more details look at table 71 and 72.
>>
>> There is no table 71 in the bindings... It seems you pasted it from
>> somewhere.
>>
> 
> It's pretty obvious that "Table" in a binding refers to the datasheet.

There are multiple datasheets and how would I know to which one this refers?

> But if you meant datasheet, not binding, you're looking at the wrong
> datasheet then.
> Also, that's how the rest of the file is written.

Not really an argument... Poor examples like to spread, it's an effort
to drop them.

> 
>>> +          Note should be signed, but dtc doesn't currently maintain the
>>> +          sign.
>>
>> What do you mean? "Maintain" as allow or keep when building FDT?  What's
>> the problem of using negative numbers here and why it should be part of
>> bindings?
>>
> 
> You're really clueless, I'll let you figure it out on your own.

Yes, I am clueless and that's not the way how the review conversation
should look like.

NAK.

> Also, that's how the rest of the file is written.
> 
>>> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
>>> +        minItems: 3
>>> +        maxItems: 64
>>> +        items:
>>> +          minItems: 2
>>> +          maxItems: 2
>>
>> Instead describe the items with "description" (and maybe constraints)
>> like here:
>>
>> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
>>
> 
> That's how the rest of the file is written.
> If you really want to use something different, you can submit a
> patch later and fix the whole binding however you want.

Nope. I expect the new pieces to be correct, not incorrect because
"there is already poor pattern, so I will do the same".

If inconsistency bothers you, anyone can fix it in following up patch.
Also you.

Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Krzysztof Kozlowski]

On 17/10/2022 05:38, Nuno Sá wrote:
> Hi Krzysztof,
> 

(...)

>>> @@ -353,6 +361,41 @@ patternProperties:
>>>          description: Boolean property which set's the adc as
>>> single-ended.
>>>          type: boolean
>>>  
>>> +  "^temp@":
>>
>> There is already a property for thermocouple. Isn't a thermocouple a
>> temperature sensor? IOW, why new property is needed?
>>
> 
> Well, most of the patternProps in this bindings are temperature
> sensors... It's just that the device(s) support different types of
> them. 'adi,sensor-type' is used to identify each sensor (as this
> translates in different configurations being written in the device
> channels).

Sure.

> 
>>> +    type: object
>>> +    description:
>>> +      Represents a channel which is being used as an active analog
>>> temperature
>>> +      sensor.
>>> +
>>> +    properties:
>>> +      adi,sensor-type:
>>> +        description:
>>> +          Identifies the sensor as an active analog temperature
>>> sensor.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +        const: 31
>>> +
>>> +      adi,single-ended:
>>> +        description: Boolean property which sets the sensor as
>>> single-ended.
>>
>> Drop "Boolean property which sets" - it's obvious from the type.
>>
>>
>>
>>> +        type: boolean
>>> +
>>> +      adi,custom-temp:
>>> +        description:
>>> +          This is a table, where each entry should be a pair of
>>
>> "This is a table" - obvious from the type.
>>
>>> +          voltage(mv)-temperature(K). The entries must be given in
>>> nv and uK
>>
>> mv-K or nv-uK? Confusing...
> 
> Yeah, a bit. In Cosmin defense, I think he's just keeping the same
> "style" as the rest of the properties...

That's not the best approach for two reasons:
1. The unit used by hardware matters less here, because bindings are
used to write DTS. In many, many other cases there will be some
translation (just take random voltage regulator bindings).

2. What the driver is doing matters even less.

So just describe here what is expected in DTS.

> 
>>
>>> +          so that, the original values must be multiplied by
>>> 1000000. For
>>> +          more details look at table 71 and 72.
>>
>> There is no table 71 in the bindings... It seems you pasted it from
>> somewhere.
> 
> I'm fairly sure this refers to the datasheet. I see now that this can
> be confusing (again this kind of references are being (ab)used in the
> rest of the file).

Yep, but there are now multiple datasheets, aren't there?

> 
>>
>>> +          Note should be signed, but dtc doesn't currently
>>> maintain the
>>> +          sign.
>>
>> What do you mean? "Maintain" as allow or keep when building FDT? 
>> What's
>> the problem of using negative numbers here and why it should be part
>> of
>> bindings?
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
>>> +        minItems: 3
>>> +        maxItems: 64
>>> +        items:
>>> +          minItems: 2
>>> +          maxItems: 2
>>
>> Instead describe the items with "description" (and maybe constraints)
>> like here:
>>
>> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
>>
> 
> Neat... My only comment (which probably applies to my previous ones) is
> that the rest of the properties are already in this "style". So maybe,
> follow up patches with small clean-ups would be more appropriate?

Of course. It would be great if the file was improved before or after
this one.


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: iio: temperature: ltc2983: support more parts

[Nuno Sá]

On Mon, 2022-10-17 at 19:32 -0400, Krzysztof Kozlowski wrote:
> On 17/10/2022 05:38, Nuno Sá wrote:
> > Hi Krzysztof,
> > 
> 
> (...)
> 
> > > > @@ -353,6 +361,41 @@ patternProperties:
> > > >          description: Boolean property which set's the adc as
> > > > single-ended.
> > > >          type: boolean
> > > >  
> > > > +  "^temp@":
> > > 
> > > There is already a property for thermocouple. Isn't a
> > > thermocouple a
> > > temperature sensor? IOW, why new property is needed?
> > > 
> > 
> > Well, most of the patternProps in this bindings are temperature
> > sensors... It's just that the device(s) support different types of
> > them. 'adi,sensor-type' is used to identify each sensor (as this
> > translates in different configurations being written in the device
> > channels).
> 
> Sure.
> 
> > 
> > > > +    type: object
> > > > +    description:
> > > > +      Represents a channel which is being used as an active
> > > > analog
> > > > temperature
> > > > +      sensor.
> > > > +
> > > > +    properties:
> > > > +      adi,sensor-type:
> > > > +        description:
> > > > +          Identifies the sensor as an active analog
> > > > temperature
> > > > sensor.
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        const: 31
> > > > +
> > > > +      adi,single-ended:
> > > > +        description: Boolean property which sets the sensor as
> > > > single-ended.
> > > 
> > > Drop "Boolean property which sets" - it's obvious from the type.
> > > 
> > > 
> > > 
> > > > +        type: boolean
> > > > +
> > > > +      adi,custom-temp:
> > > > +        description:
> > > > +          This is a table, where each entry should be a pair
> > > > of
> > > 
> > > "This is a table" - obvious from the type.
> > > 
> > > > +          voltage(mv)-temperature(K). The entries must be
> > > > given in
> > > > nv and uK
> > > 
> > > mv-K or nv-uK? Confusing...
> > 
> > Yeah, a bit. In Cosmin defense, I think he's just keeping the same
> > "style" as the rest of the properties...
> 
> That's not the best approach for two reasons:
> 1. The unit used by hardware matters less here, because bindings are
> used to write DTS. In many, many other cases there will be some
> translation (just take random voltage regulator bindings).
> 
> 2. What the driver is doing matters even less.
> 
> So just describe here what is expected in DTS.
> 

Alright, I see. So we just refer to nv-uK as that is what I wanted for
dts to expect (reason being to have more resolution).

> > 
> > > 
> > > > +          so that, the original values must be multiplied by
> > > > 1000000. For
> > > > +          more details look at table 71 and 72.
> > > 
> > > There is no table 71 in the bindings... It seems you pasted it
> > > from
> > > somewhere.
> > 
> > I'm fairly sure this refers to the datasheet. I see now that this
> > can
> > be confusing (again this kind of references are being (ab)used in
> > the
> > rest of the file).
> 
> Yep, but there are now multiple datasheets, aren't there?
> 

Hmm yeah that's true. By the time I wrote this binding I was not even
thinking on the possibility of new parts being added to it... I guess
the lesson in here is to avoid this kind os specific descriptions.

> > 
> > > 
> > > > +          Note should be signed, but dtc doesn't currently
> > > > maintain the
> > > > +          sign.
> > > 
> > > What do you mean? "Maintain" as allow or keep when building FDT? 
> > > What's
> > > the problem of using negative numbers here and why it should be
> > > part
> > > of
> > > bindings?
> > > 
> > > > +        $ref: /schemas/types.yaml#/definitions/uint64-matrix
> > > > +        minItems: 3
> > > > +        maxItems: 64
> > > > +        items:
> > > > +          minItems: 2
> > > > +          maxItems: 2
> > > 
> > > Instead describe the items with "description" (and maybe
> > > constraints)
> > > like here:
> > > 
> > > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.yaml#L278
> > > 
> > 
> > Neat... My only comment (which probably applies to my previous
> > ones) is
> > that the rest of the properties are already in this "style". So
> > maybe,
> > follow up patches with small clean-ups would be more appropriate?
> 
> Of course. It would be great if the file was improved before or after
> this one.
> 

Ok, IMO it would make sense to have it in this series but if Cosmin
does not feel like fixing my mess :), I'll send a separate patch with
your inputs...

- Nuno Sá