Re: [PATCH v3 2/2] iio: adc: Add ti-ads1018 driver

["Kurt Borja" <kuurtb@gmail.com>]

On Mon Dec 1, 2025 at 6:09 PM -05, David Lechner wrote:

...

>> +#define ADS1018_CFG_DEFAULT		0x058b
>
> Would be nice to use the macros below to define this value so that we
> know what it is actually doing.

I will drop this on the next version. More on it bellow.

...

>> +static int ads1018_read_unlocked(struct ads1018 *ads1018, __be16 *cnv, bool hold_cs)
>
> It is a bit odd to me to call this "unlocked" and then call
> "spi_synced_locked()". It sounds like we are using opposite words
> to mean the same thing.

Yes, that's true. I will revert this back to "locked", to match the SPI
naming.

...

> Do we actually need to transmit two words to trigger a conversion?
>
> It looks like there is a "16-Bit Data Transmission Cycle" for when
> we don't need to read the config register back.

Yes, we don't need the config readback anymore. I will convert
everything to 16-bit transmission.

...

>> +static int
>> +ads1018_read_raw_unlocked(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
>
> Saying "ulocked" here is a bit confusing since the previous "unlocked" had
> to do with SPI bus lock rather than iio_device_claim_direct().

I agree, this naming is a bit confusing.

Andy, are you okay if I revert this back to __ads1018_read_raw()? I can
add a comment on context.

...

>> +static int ads1018_buffer_preenable(struct iio_dev *indio_dev)
>> +{
>> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
>> +	const struct ads1018_chip_info *chip_info = ads1018->chip_info;
>> +	unsigned int pga, drate, addr;
>> +	u16 cfg;
>> +
>> +	addr = find_first_bit(indio_dev->active_scan_mask, iio_get_masklength(indio_dev));
>> +	pga = ads1018_get_pga_mode(ads1018, addr);
>> +	drate = ads1018_get_data_rate_mode(ads1018, addr);
>> +
>> +	cfg = ADS1018_CFG_VALID;
>> +	cfg |= FIELD_PREP(ADS1018_CFG_MUX_MASK, addr);
>> +	cfg |= FIELD_PREP(ADS1018_CFG_PGA_MASK, pga);
>> +	cfg |= FIELD_PREP(ADS1018_CFG_MODE_MASK, ADS1018_MODE_CONTINUOUS);
>> +	cfg |= FIELD_PREP(ADS1018_CFG_DRATE_MASK, drate);
>> +
>> +	if (chip_info->channels[addr].type == IIO_TEMP)
>> +		cfg |= ADS1018_CFG_TS_MODE_EN;
>> +
>> +	ads1018->tx_buf[0] = cpu_to_be16(cfg);
>> +	ads1018->tx_buf[1] = 0;
>
> Seems like we could use 16-bit cycles here too?
>
>> +
>> +	return spi_write(ads1018->spi, ads1018->tx_buf, sizeof(ads1018->tx_buf));
>
> Hmmm... In the case where the trigger is not the DRDY signal (i.e. hritmer or
> sysfs), it seems like we would want to defer this until we actually receive
> a trigger. Otherwise, if the trigger is not already enabled and it is a while
> before the trigger is enabled, then the first data value will be quite stale
> compared to the others since the conversion was done when the buffer was enabled
> rather than when the trigger fired.

Because this is configured as ADS1018_MODE_CONTINUOUS, every time a
conversion is completed, it automatically refreshes the last conversion
on the register, so it is never stale (Section 8.4.2.2).

>
>> +}
>> +
>> +static int ads1018_buffer_postdisable(struct iio_dev *indio_dev)
>> +{
>> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
>> +
>> +	ads1018->tx_buf[0] = cpu_to_be16(ADS1018_CFG_DEFAULT);
>
> Changing DEFAULT to a more descritive name (e.g. SINGLE_SHOT_MODE) would make
> this more clear that the purpose of doing this is to take it out of conversion
> mode. Otherwise, a comment would be helpful here.

I think it's better to just construct a minimal config with
ADS1018_MODE_ONESHOT here.

Ack to everything else. Thanks, David.


-- 
 ~ Kurt