Message-ID: <9fba8701-a9c2-45b4-9bc1-5c49813e72cc@baylibre.com>
Date: Sat, 22 Nov 2025 09:56:38 -0600
From: David Lechner <dlechner@...libre.com>
To: 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 2/2] iio: adc: Add ti-ads1x18 driver

On 11/21/25 6:24 PM, Kurt Borja wrote:
> On Fri Nov 21, 2025 at 5:33 PM -05, David Lechner wrote:
>> On 11/21/25 11:16 AM, Kurt Borja wrote:
>>

...

>> #define ADS1018_CFG_REG			0x0000
> 
> I didn't define these because ads1118 dumps all registers (2) in each
> transfer.

Oh, right, there is basically only one register so we don't have to
address it. :-)


>>
>> It is a bit confusing to have this here rather than in the buffer
>> enable callback since that is also setting the config that triggers
>> the first conversion.
>>
>> Having the spi_bus_lock() and enable_irq() in the buffer enable
>> would make more sense to me too.
> 
> This is the approach ad_sigma_delta takes.
> 

I did some work on that with that module recently. I would not say that
it is an ideal reference. IIRC, it still has some race condition with
enabling/disabling interrupts in some cases. So hopefully we can do better
here.

>>> +
>>> +static int ads1x18_message_init(struct ads1x18 *ads1x18)
>>> +{
>>> +	struct spi_device *spi = ads1x18->spi;
>>> +
>>> +	/*
>>> +	 * We need to keep CS asserted to catch "data-ready" interrupts.
>>> +	 * Otherwise the DOUT/DRDY line enters a Hi-Z state and it can't be
>>> +	 * driven by the ADC.
>>> +	 */
>>> +	ads1x18->xfer.cs_change = 1;
>>
>> I think this is going to be problematic for reading/writing the configuration
>> register and for direct reads of a single sample. My suggestion to make a
> 
> Can you elaborate on why it would be problematic?

This transfer is used for all SPI messages. So it means that CS will still
be high after every transfer, not just the ones during a buffered read where
it is actually needed.

This would be a problem if there were any other devices on the SPI bus.
When the controller communicates with the other device, the ADC will
still be listening and responding because CS is still high.


>>> +
>>> +static int ads1x18_channels_init(struct ads1x18 *ads1x18,
>>> +				 const struct ads1x18_chip_info *info,
>>> +				 struct iio_chan_spec **cs)
>>> +{
>>> +	struct device *dev = &ads1x18->spi->dev;
>>> +	struct iio_chan_spec *channels;
>>> +	int ret, nchans, index = 0;
>>> +
>>> +	nchans = device_get_named_child_node_count(dev, "channel");
>>> +	if (!nchans)
>>> +		return dev_err_probe(dev, -ENODEV,
>>> +				     "No ADC channels described.\n");
>>> +
>>> +	channels = devm_kcalloc(dev, nchans + 2, sizeof(*channels), GFP_KERNEL);
>>> +	if (!channels)
>>> +		return -ENOMEM;
>>> +
>>> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
>>> +		ret = ads1x18_fill_properties(ads1x18, child, &channels[index]);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		channels[index].scan_index = index;
>>> +		ads1x18->bufidx_to_addr[index] = channels[index].address;
>>> +		index++;
>>> +	}
>>
>> There is a small enough number of channels that we shouldn't need any of this.
>> We can just make an array big enough for all channels in struct ads1x18.
> 
> Ack.
> 
> Do you think we should just let every channel be visible in sysfs or
> should we still control visibility with the channel@[0-7] node?

Yes. It is normal to show all channels. The few exceptions, like multiplexed
chips where there can be 100s or 1000s of possible combinations of differential
channels possible. And sometimes for ADCs built into a SoC, we omit the channels
that aren't wired up to something.

It makes it much easier to write userspace software though if every instance
of the ADC has exactly the same attributes, so I try to advocate for that.


>>> +	ads1x18->chip_info = info;
>>> +	mutex_init(&ads1x18->msg_lock);
>>> +	init_completion(&ads1x18->data_ready);
>>> +	spi_set_drvdata(spi, ads1x18);
>>
>> There is no spi_get_drvdata(), so we don't need this.
> 
> I do however use dev_get_drvdata() directly in PM ops.
> 

OK, so dev_set_drvdata() would make it symmetric.


>>
>> I think we could simplify this and avoid needing to use pm runtime (and use
>> even less power!). During probe, put the chip in power down mode. When doing
>> direct reads of a single value, put the chip in single-shot mode. When doing
>> starting a buffered read, put it in continuous mode and when the buffered read
>> is stopped, put it back in shutdown mode.
> 
> These chips only have two modes single-shot (low-power) and continuous.
> Are you suggesting we shut it down using the vdd regulator?
> 
> Either way, can't the system go to sleep while in buffer mode? If that's
> the case we should still need these handlers.
> 

I hope not. I would suspect that most IIO drivers are broken in this
regard. I've never attempted to try to implement suspend/resume in an
IIO driver yet because I didn't have an application that required it
and it would be very difficult to get right without very extensive
testing.

Powered by blists - more mailing lists