Re: [PATCH 2/2] iio: temperature: ltc2983: Add support for ADT7604

[Andy Shevchenko]

On Mon, Apr 27, 2026 at 04:25:08PM +0300, Liviu Stan wrote:
> The ADT7604 shares the same die as the LTC2984. It repurposes the
> custom RTD sensor type (18) as a copper trace resistance sensor
> and the custom thermistor type (27) as a leak detector, and
> removes thermocouple, diode and direct ADC sensor types.
> 
> Custom RTD (type 18) becomes the copper trace sensor. Sensor
> configuration bits 21:18 are hardcoded to 0b1001 per the
> datasheet. Two variants are supported via the new
> adi,copper-trace-sub-ohm DT property: sub-ohm traces (< 1 ohm)
> have bits 17:0 cleared with no excitation current or custom
> table; standard traces (> 1 ohm) accept an optional
> resistance-to-temperature table.
> 
> Custom thermistor (type 27) becomes the leak detector. Sensor
> configuration bits are hardcoded to 0b001. The custom table
> uses a resolution of 16 (20+4 bit resistance field) instead of
> 64, and is specified via the new adi,custom-leak-detector DT
> property.
> 
> Both sensor types expose an IIO_RESISTANCE channel reading from
> the resistance result register bank (0x060-0x00AF), added to
> the regmap readable ranges. Scales are 1/1,024,000 for copper
> trace (result in mOhm) and 1/1024 for leak detector (result
> in Ohm).
> 
> A has_copper_trace capability flag is introduced in
> ltc2983_chip_info to identify the ADT7604, following the
> existing has_temp and has_eeprom pattern.
> 
> Tested on EVAL-ADT7604-AZ connected to Raspberry Pi 5 via SPI.

...

>  #define LTC2983_CHAN_START_ADDR(chan) \
>  			(((chan - 1) * 4) + LTC2983_CHAN_ASSIGN_START_REG)
> -#define LTC2983_CHAN_RES_ADDR(chan) \
> -			(((chan - 1) * 4) + LTC2983_TEMP_RES_START_REG)
> +#define LTC2983_CHAN_RES_ADDR(chan, base) \
> +			((((chan) - 1) * 4) + (base))

For the sake of consistency I would see (base) also to be in the _START_ADDR()
macro.

...

> +	bool sub_ohm;

What does this mean? Perhaps rename to is_in_milliohms or something like that?

...

> +		ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
> +		if (!ret) {

Yeah, this is in the original code. Consider at some point to make it rather
returning meaningful error codes, id est

		if (fwnode_property_present(child, "adi,number-of-wires")) {
			ret = fwnode_property_read_u32(child, "adi,number-of-wires", &n_wires);
			if (ret)
				return ret; // or with message that we can't get property value

> +			switch (n_wires) {
> +			case 2:
> +				rtd->sensor_config = LTC2983_RTD_N_WIRES(0);
> +				break;
> +			case 3:
> +				rtd->sensor_config = LTC2983_RTD_N_WIRES(1);
> +				break;
> +			case 4:
> +				rtd->sensor_config = LTC2983_RTD_N_WIRES(2);
> +				break;
> +			case 5:
> +				/* 4 wires, Kelvin Rsense */
> +				rtd->sensor_config = LTC2983_RTD_N_WIRES(3);
> +				break;
> +			default:
> +				return dev_err_ptr_probe(dev, -EINVAL,
> +							 "Invalid number of wires:%u\n",
> +							 n_wires);
> +			}
>  		}

...

> +			if (sensor->chan < LTC2983_DIFFERENTIAL_CHAN_MIN)
> +				return dev_err_ptr_probe(&st->spi->dev, -EINVAL,

Don't you have 'dev' variable to use? If not, maybe makes sense to introduce.

> +							 "Invalid chann:%d for RTD\n",

chann? Perhaps just "chan"?

> +							 sensor->chan);
>  		}

...

> +		if (st->info->has_copper_trace) {
> +			if (fwnode_property_present(child, "adi,custom-rtd")) {
> +				rtd->custom = __ltc2983_custom_sensor_new(st, child,
> +									  "adi,custom-rtd",
> +									  false, 2048,
> +									  false);
> +				if (IS_ERR(rtd->custom))
> +					return ERR_CAST(rtd->custom);
> +			}
> +		} else {
> +			rtd->custom = __ltc2983_custom_sensor_new(st, child,
> +								  "adi,custom-rtd",
> +								  false, 2048, false);
> +			if (IS_ERR(rtd->custom))
> +				return ERR_CAST(rtd->custom);
> +		}

Seeing so many indentation noise, I think this patch starves for some
preparatory ones that make helper(s) out of the existing rather long functions
and then in a new code it will much easier to follow what gets changed and how.

...

Due to above I stopped here, because patch seems unreviewable to me. If others
are motivated more than me ans see this change nice in terms of readability,
I won't object. Personally I think it must be refactored (a lot!) before actually
adding a support of a new HW.

-- 
With Best Regards,
Andy Shevchenko