Message-Id: <DEIX2829UYMB.8D3TGUFHXS10@gmail.com>
Date: Wed, 26 Nov 2025 15:41:58 -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 2/2] iio: adc: Add ti-ads1x18 driver

On Sat Nov 22, 2025 at 10:56 AM -05, David Lechner wrote:
> 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.

Oh - I meant haveing spi_bus_lock() and enable_irq() in buffer enable is
the approach ad_sigma_delta takes.

I moved them to set_trigger_state() to be able to use other triggers.
Furthermore if interrupts or drdy-gpios are not defined, we don't really
have to take spi_bus_lock() and doing it this way ensures that.

For this reason I would like to keep this for v2 and we can discuss it
further if you disagree.

>
>>>> +
>>>> +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.

Thanks for the heads up!

This was a misunderstading on my part, I thought the SPI core would
de-assert CS if another device requested a transfer.

>
>
>>>> +
>>>> +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.

Agreed.

>
>
>>>> +	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.

Then I'll drop it.

Thanks! I will submit v2 soon :)


-- 
 ~ Kurt

Powered by blists - more mailing lists