linux-kernel - Re: [PATCH v8 2/2] iio: adc: Add ti-ads1018 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DEYY46ZUJQ35.YBNYWLGZMRYA@gmail.com>
Date: Mon, 15 Dec 2025 11:54:13 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "David Lechner" <dlechner@...libre.com>, "Kurt Borja"
 <kuurtb@...il.com>, "Jonathan Cameron" <jic23@...nel.org>, "Rob Herring"
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, "Tobias Sperling"
 <tobias.sperling@...ting.com>
Cc: Nuno Sá <nuno.sa@...log.com>, "Andy Shevchenko"
 <andy@...nel.org>, <linux-iio@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Jonathan
 Cameron" <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v8 2/2] iio: adc: Add ti-ads1018 driver

On Mon Dec 15, 2025 at 10:55 AM -05, David Lechner wrote:
> On 12/11/25 10:25 PM, Kurt Borja wrote:
>> Add ti-ads1018 driver for Texas Instruments ADS1018 and ADS1118 SPI
>> analog-to-digital converters.
>> 
>
> ...
>
>> +/**
>> + * ads1018_spi_read_exclusive - Reads a conversion value from the device
>> + * @ads1018: Device data
>> + * @cnv: ADC Conversion value (optional)
>> + * @hold_cs: Keep CS line asserted after the SPI transfer
>> + *
>> + * Reads the most recent ADC conversion value, without updating the
>> + * device's configuration.
>> + *
>> + * Context: Expects iio_device_claim_buffer_mode() is held and SPI bus
>> + *	    *exclusive* use.
>
> I guess "exclusive" is supposed to mean that the SPI bus lock is held?
> Would have been more clear to just say "SPI bus lock is held".

I went with "exclusive" because we are not holding the lock on the same
thread, so that could be misleading.

...

>> +static int ads1018_buffer_postdisable(struct iio_dev *indio_dev)
>> +{
>> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
>> +	u16 cfg;
>> +
>> +	cfg = ADS1018_CFG_VALID;
>> +	cfg |= FIELD_PREP(ADS1018_CFG_MODE_MASK, ADS1018_MODE_ONESHOT);
>> +
>> +	ads1018->tx_buf[0] = cpu_to_be16(cfg);
>> +	ads1018->tx_buf[1] = 0;
>> +
>> +	return spi_write(ads1018->spi, ads1018->tx_buf, sizeof(ads1018->tx_buf));
>> +}
>> +
>> +static const struct iio_buffer_setup_ops ads1018_buffer_ops = {
>> +	.preenable = ads1018_buffer_preenable,
>> +	.postdisable = ads1018_buffer_postdisable,
>> +	.validate_scan_mask = iio_validate_scan_mask_onehot,
>
> Why do we only allow one channel to be selected at a time?
> I guess we can remove that restriction later. I just didn't
> notice this in previous reviews.

I chose this restriction because of the timestamp discussion a few
versions ago.

I will remove it later and implement multichannel buffers as Jonathan
suggested.

...

>> +static irqreturn_t ads1018_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct ads1018 *ads1018 = iio_priv(indio_dev);
>> +	struct {
>> +		__be16 conv;
>> +		aligned_s64 ts;
>> +	} scan = {};
>> +	int ret;
>> +
>
>
>> +	if (iio_device_claim_buffer_mode(indio_dev))
>> +		goto out_notify_done;
>
> This should not be needed. It should not be possible to
> exit buffer mode without triggers being stopped first.
> (No other driver is doing this.)

Previously I had my own lock here because ads1018_spi_read_exclusive()
needs locking. I dropped it per your suggestion in v1 I believe, but
given the discussion in the cleanup.h series I was thinking in bringing
it back.

But yes, the scope can be adjusted too.

-- 
Thanks,
 ~ Kurt