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

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

On Sat Dec 6, 2025 at 3:07 PM -05, Jonathan Cameron wrote:
> On Thu, 04 Dec 2025 13:01:28 -0500
> Kurt Borja <kuurtb@gmail.com> wrote:
>
>> Add ti-ads1018 driver for Texas Instruments ADS1018 and ADS1118 SPI
>> analog-to-digital converters.
>> 
>> These chips' MOSI pin is shared with a data-ready interrupt. Defining
>> this interrupt in devicetree is optional, therefore we only create an
>> IIO trigger if one is found.
>> 
>> Handling this interrupt requires some considerations. When enabling the
>> trigger the CS line is tied low (active), thus we need to hold
>> spi_bus_lock() too, to avoid state corruption. This is done inside the
>> set_trigger_state() callback, to let users use other triggers without
>> wasting a bus lock.
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>

...

>> +#define ADS1018_VOLT_CHAN(_index, _chan, _realbits) {				\
>> +	.type = IIO_VOLTAGE,							\
>> +	.channel = _chan,							\
>> +	.scan_index = _index,							\
>> +	.scan_type = {								\
>> +		.sign = 's',							\
>> +		.realbits = _realbits,						\
>> +		.storagebits = 16,						\
>> +		.shift = 16 - _realbits,					\
>> +		.endianness = IIO_BE,						\
>> +	},									\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
>> +			      BIT(IIO_CHAN_INFO_SCALE) |			\
>> +			      BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
>
> What motivates per channel sampling frequency?
>
> Given you have to write it each time you configure I guess it doesn't matter much
> either way.

I guess making it shared by all is simpler too, so I'll go with that.

...

>> +/**
>> + * ads1018_calc_delay - Calculates a suitable delay for a single-shot reading
>> + * @ads1018: Device data
>> + *
>> + * Calculates an appropriate delay for a single shot reading, assuming the
>> + * device's maximum data-rate is used.
>> + *
>> + * Context: Expects iio_device_claim_direct() is held.
>
> What in here changes if we are in buffered mode?
> We have no reason to call it but why does that matter?

Yep, I just pasted this mindlessly. I'll remove it.

...

>> +/**
>> + * ads1018_single_shot - Performs a one-shot reading sequence
>> + * @ads1018: Device data
>> + * @cfg: New configuration for the device
>> + * @cnv: Conversion value
>> + *
>> + * Writes a new configuration, waits an appropriate delay (assuming the new
>> + * configuration uses the maximum data-rate) and then reads the most recent
>
> I'm lost on this.  Normally the longest delay is governed by the minimum data rate.
> I.e. Samples take longer when running few per second, so we wait longer.

We are using the minimum data rate on the maximum data-rate mode. I
should have added "mode" there.

>
> I think this is meant to mean the delay needed for a sample at the minimum expected
> rate for this configuration.

Yes, I think this was too confusing.

I'll add a `hz` argument to ads1018_calc_delay() and pass the frequency
of the maximum data-rate mode when preparing the config.

I think this will make the intent more explicit.

...

>> +static int
>> +ads1018_write_raw_unlocked(struct iio_dev *indio_dev,
>
> Similar to the naming discussion on the ACQUIRE RFC I'm not sure
> using locked here is really descriptive of more than an internal
> detail of how we prevent mode switching. I'd prefer something like
> ads1018_write_raw_direct_claimed() or ads1018_write_raw_direct_mode()
> (the absence of any other write_raw_*** would indicate this is the only
> valid one perhaps).
>
> Also this isn't the unlocked version, it's the one that doesn't take
> the lock.

I'll go with ads1018_{read,write}_raw_direct_mode().

>

Ack to everything else, including bindings stuff. Thanks, Jonathan!


-- 
 ~ Kurt