linux-kernel - Re: [PATCH v1 1/3] iio: adc: ad_sigma

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: <Z49qgvy0ZVKJ0h0W@spiri>
Date: Tue, 21 Jan 2025 11:36:02 +0200
From: Alisa-Dariana Roman <alisadariana@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Alisa-Dariana Roman <alisa.roman@...log.com>,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	David Lechner <dlechner@...libre.com>,
	Uwe Kleine-König <ukleinek@...nel.org>,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH v1 1/3] iio: adc: ad_sigma_delta: Add CS assert function

On Sun, Dec 22, 2024 at 06:07:13PM +0000, Jonathan Cameron wrote:
> On Sat, 21 Dec 2024 17:56:00 +0200
> Alisa-Dariana Roman <alisadariana@...il.com> wrote:
> 
> > Some sigma-delta ADCs, such as AD7191 and AD7780, have no registers and
> > start conversion when CS is asserted. Add helper function to support
> > this use case by allowing devices to assert CS without performing
> > register operations.
> Hi Alisa-Dariana,
> 
> I had a look at the ad7191 datasheet. Given this description,
> I was expecting to see it do a pre pulse of the chip select to trigger
> the acquisition.  However, what I see is a power down line (which is more
> or less a chip select) but it just has a specified t1 delay before the
> DOUT will change to the state for the first bit and the host
> can start driving the clock.
> 
> That can be done by setting spi_device->cs_setup to whatever delay is
> needed.  The text is spi_device docs are a little vague,
> but I'd take it as t1 + t2 (maybe t3 to be safe).
> 
> That is going to be more reliable than trying to hold the cs across
> messages / spi_sync() calls, particularly if the bus might not be
> locked (which the code below suggests).
> 
> Jonathan
> 
> 

Hello Jonathan! I am grateful for your and everyone's feedback, as
always!

I got a bit stuck on this part. The motivation for adding this function
is as following:

int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
	const struct iio_chan_spec *chan, int *val)
{

...
	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE);

	ad_sd_enable_irq(sigma_delta);
	ret = wait_for_completion_interruptible_timeout(
			&sigma_delta->completion, HZ);
...
}

I noticed that adc drivers need to call the ad_sd_write_reg function in
their callback set_mode function, in order to keep the cs line pulled
down before waiting for the interrupt (if I understand correctly). But
since this component and AD7780 have no register I just copied the
functionality of ad_sd_write_reg without actually writing anything.

Should I change the description/name to more accurately present the
functionality? Or would it be a better idea to not use the single
conversion function and write something from scratch leveraging the
cs_setup?

Thank you!

Kind regards,
Alisa-Dariana Roman.