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

[Stan, Liviu]

Thank you for the comments, and I apologize for the late response.

On Mon, Apr 27, 2026 Andy Shevchenko wrote:
...
> >  #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.

Understood. Will change in v2.

> > +	bool sub_ohm;
> 
> What does this mean? Perhaps rename to is_in_milliohms or something like
> that?

The datasheet describes two cases for the copper trace sensor type: < 1ohm and > 1 
ohm copper traces. The difference between the two is that < 1 ohm copper traces
have bits 17:0 zeroed (excitation current and custom sensor data pointer configuration
bits). For > 1 ohm copper traces an excitation current needs to be specified and the 
custom table bits are optional. "Sub_ohm" reflects the selection of the sub-ohm variant,
not the result units. For me "sub_ohm" or "is_sub_ohm" feels more in relation to the 
datasheet, but if something like "is_in_milliohms" feels more understandable to you I can
change it in v2.

> > +		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

Noted, I can add a patch for this in v2.

> > +			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.

That was already present in the original code, but I can have a patch where I 
clean up all occurrences in v2.

> > +							 "Invalid chann:%d for
> RTD\n",
> 
> chann? Perhaps just "chan"?

This, also, was present in the original code, the error messages I introduced 
for leak detector and copper trace follow the same pattern though. Should I
modify it everywhere?

> > +							 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.

I understand, I will restructure in v2.

Thanks,
Liviu