linux-kernel - Re: [PATCH v3 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: <DENT40NRKH0V.3UD0JBGRUSCDS@gmail.com>
Date: Tue, 02 Dec 2025 09:39:34 -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 v3 2/2] iio: adc: Add ti-ads1018 driver

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