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

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

On Sat Nov 29, 2025 at 9:21 AM -05, Andy Shevchenko wrote:

...

>> + * @ad1018: 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.
>> + *
>> + * Return: Delay in microseconds.
>
> Does 0 have any special meaning?

This function is never 0.

...

>> +	/* We subtract 10% data-rate error */
>> +	hz -= DIV_ROUND_UP(hz, 10);
>
> Hmm... For delays I expect to see adding 10% to have a good margin.

hz goes in the denomitor bellow, so less hz is more delay. Makes sense
because worst case sample rate is less sample rate.

...

>> + * Context: Expects spi_bus_lock() is held.
>
> Do we have a lockdep assert for this?

Lockdep checks lock is held on the same thread right?

Thinking about it, this context is wrong because we don't hold the bus
lock on the same thread. Rather we hold it to avoid other devices from
transfering while we hold our CS line.

I should probably remove this context and mention this peculiarity in
the long description.

...

>> +static int ads1018_read_unlocked(struct ads1018 *ads1018, __be16 *cnv, bool hold_cs)
>
> Hmm... Don't we want to return value in CPU order? I don't know the answer
> here, and IIRC IIO triggers might be actually good with endianess conversion
> done, if required, in user space.

I specified IIO_BE endianness in each channel's .scan_type, so this
works. However, I don't have issue especifying IIO_CPU and just
returning CPU order values.

...

>> + * Context: Expects iio_device_claim_direct() is held.
>
> Jonathan et al., do we have lockdep assert available for this?
> I really prefer to see the code for it, while comment is good,
> it is not good enough.

This would be nice.

...

>> +	if (iio_device_claim_buffer_mode(indio_dev))
>> +		goto out_notify_done;
>> +
>> +	if (iio_trigger_using_own(indio_dev)) {
>> +		disable_irq(ads1018->drdy_irq);
>> +		ret = ads1018_read_unlocked(ads1018, &scan.conv, true);
>> +		enable_irq(ads1018->drdy_irq);
>> +	} else {
>> +		ret = spi_read(ads1018->spi, ads1018->rx_buf, sizeof(ads1018->rx_buf));
>> +		scan.conv = ads1018->rx_buf[0];
>> +	}
>> +
>> +	iio_device_release_buffer_mode(indio_dev);
>> +
>> +	if (ret)
>> +		goto out_notify_done;
>> +
>> +	iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
>> +
>> +out_notify_done:
>> +	iio_trigger_notify_done(ads1018->indio_trig);
>
> Jonathan et al., maybe we need an ACQUIRE() class for this? It will solve
> the conditional scoped guard case, no?

...

If no one prefers to do it, I can submit a patch implementing this. Same
for the lockdep issue above.

>> +/**
>> + * ADS1018_FSR_TO_SCALE - Converts FSR into scale
>> + * @_fsr: Full-scale range in millivolts
>> + * @_res: ADC resolution
>
> Add here something like this:
>
> *
> * The macro is crafted to avoid potential overflows on 32-bit machines.
> * This imposes restrictions to the possible values for @_fsr (less
> * than 274878), and @_res (great or equal to 6 bits).
> *

Sure.

I'll address the rest of the comments in v4. Thanks, Andy!


-- 
 ~ Kurt