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

[Jonathan Cameron]

On Wed, 20 May 2026 21:19:37 +0300
Liviu Stan <liviu.stan@xxxxxxxxxx> wrote:

> On Mon, 18 May 2026 14:58:02 +0100 Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> ...
> > > > > +		    !(st->info->supported_sensors & BIT_ULL(sensor.type)))
> > > > > +			return dev_err_probe(dev, -EINVAL,
> > > > > +					     "sensor type %d not supported on %s\n",
> > > > > +					     sensor.type, st->info->name);
> > > > > +
> > > > > +		dev_dbg(dev, "Create new sensor, type %u, channel %u",
> > > > >  			sensor.type, sensor.chan);
> > > > >      
> > > >     
> > > > > @@ -1445,8 +1782,9 @@ static int ltc2983_eeprom_cmd(struct ltc2983_data *st, unsigned int cmd,
> > > > >  
> > > > >  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;
> > > > >  	struct device *dev = &st->spi->dev;
> > > > > +	u32 iio_chan_t = 0, iio_chan_v = 0, iio_chan_r = 0, iio_chan_c = 0;
> > > > > +	u32 chan, iio_idx = 0, status;
> > > > >  	int ret;
> > > > >  
> > > > >  	/* make sure the device is up: start bit (7) is 0 and done bit (6) is 1 */
> > > > > @@ -1493,8 +1831,26 @@ static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
> > > > >  		    !assign_iio)
> > > > >  			continue;
> > > > >  
> > > > > +		/*
> > > > > +		 * Copper trace and leak detector sensors without a custom table
> > > > > +		 * produce only a resistance result; the chip does not populate
> > > > > +		 * the temperature result register. Emit only an IIO_RESISTANCE
> > > > > +		 * channel in this case.    
> > > > 
> > > > Do we care?  That is are they useful without the table? We could just make it
> > > > required in the binding.
> > > >     
> > > 
> > > The datasheet specifies the table is optional. But more practically, in order to
> > > be able to add accurate values to the custom table, the users first need to measure
> > > the sensor's resistance at multiple known conditions, so I think the resistance-only
> > > output is useful during that characterization phase, before the table exists. Making
> > > it required would force users to provide placeholder values just to get the driver
> > > to probe.  
> > Who cares of datasheet is crazy :)  
> 
> Fair enough :)

Nice if I could type "if" obviously!

> 
> > 
> > The initial case could I think be handled by an 'identity' table. 
> > If it's useful in more general cases maybe we should always put out the resistance
> > channels? This would be a bit like we often do for ambient light sensors, where
> > we have a computed illuminance channel (IIO_LIGHT) + the data it comes from
> > (IIO_INTENSITY)  
> 
> I checked internally and we could make the table required for leak detectors. For
> copper traces, sub-ohms variants cannot have one, but we could make it required
> for > 1ohm ones.
Great. Thanks for chasing that down.

> 
> This means we could remove the LTC2983_SENSOR_LEAK_DETECTOR from the if condition,
> and have something like this in ltc2983_setup:
> 
> if (st->sensors[chan]->type == LTC2983_SENSOR_COPPER_TRACE) {
>     if (st->sensors[chan]->n_iio_chan == 1) {
>         st->iio_chan[iio_idx++] =
>             LTC2983_CHAN(IIO_RESISTANCE, iio_chan_r++, chan);
>         continue;
>     }
> }
> + the n_iio_chan == 2 check at the end
> 
> or drop the n_iio_chan == 2 check and do something like:
> 
> if (st->sensors[chan]->type == LTC2983_SENSOR_COPPER_TRACE) {
>     if (st->sensors[chan]->n_iio_chan == 1) {
>         st->iio_chan[iio_idx++] =
>             LTC2983_CHAN(IIO_RESISTANCE, iio_chan_r++, chan);
>     } else {
>         st->iio_chan[iio_idx++] =
>             LTC2983_CHAN(IIO_TEMP, iio_chan_t++, chan);
>         st->iio_chan[iio_idx++] =
>             LTC2983_CHAN(IIO_RESISTANCE, iio_chan_r++, chan);
>     }
>     continue;
> }
> 
> if (st->sensors[chan]->type == LTC2983_SENSOR_LEAK_DETECTOR) {
>     st->iio_chan[iio_idx++] =
>         LTC2983_CHAN(IIO_COVERAGE_PERCENT, iio_chan_c++, chan);
>     st->iio_chan[iio_idx++] =
>         LTC2983_CHAN(IIO_RESISTANCE, iio_chan_r++, chan);
>     continue;
> }
> 
> or use a switch case:
> 
> switch (st->sensors[chan]->type) {
> case LTC2983_SENSOR_COPPER_TRACE:
>     if (st->sensors[chan]->n_iio_chan == 1) {
>         st->iio_chan[iio_idx++] =
>             LTC2983_CHAN(IIO_RESISTANCE, iio_chan_r++, chan);
>     } else {
>         st->iio_chan[iio_idx++] =
>             LTC2983_CHAN(IIO_TEMP, iio_chan_t++, chan);
>         st->iio_chan[iio_idx++] =
>             LTC2983_CHAN(IIO_RESISTANCE, iio_chan_r++, chan);
>     }
>     continue;
> case LTC2983_SENSOR_LEAK_DETECTOR:
>     st->iio_chan[iio_idx++] =
>         LTC2983_CHAN(IIO_COVERAGE_PERCENT, iio_chan_c++, chan);
>     st->iio_chan[iio_idx++] =
>         LTC2983_CHAN(IIO_RESISTANCE, iio_chan_r++, chan);
>     continue;
> case LTC2983_SENSOR_DIRECT_ADC:
>     chan_type = IIO_VOLTAGE;
>     iio_chan = &iio_chan_v;
>     break;
> default:
>     chan_type = IIO_TEMP;
>     iio_chan = &iio_chan_t;
>     break;
> }
> st->iio_chan[iio_idx++] = LTC2983_CHAN(chan_type, (*iio_chan)++, chan);
> 
> What do you think?
> 
I don't immediately have a strong opinion. So choose which ever
looks most readable in situ.

Jonathan


> Thanks,
> Liviu