linux-kernel - Re: [PATCH v1 3/3] iio: adc: ad7192: Add sync gpio

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: <6435f696-40fe-4ff9-ae76-1f121fe7604f@baylibre.com>
Date: Mon, 2 Dec 2024 16:21:43 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>,
 Alisa-Dariana Roman <alisadariana@...il.com>
Cc: Alisa-Dariana Roman <alisa.roman@...log.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Michael Hennerich <michael.hennerich@...log.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH v1 3/3] iio: adc: ad7192: Add sync gpio

On 11/30/24 12:38 PM, Jonathan Cameron wrote:
> On Thu, 28 Nov 2024 14:55:03 +0200
> Alisa-Dariana Roman <alisadariana@...il.com> wrote:
> 
>> Add support for the SYNC pin of AD719x devices. This pin is controlled
>> through a GPIO. The pin allows synchronization of digital filters and
>> analog modulators when using multiple devices.
>>
>> Signed-off-by: Alisa-Dariana Roman <alisa.roman@...log.com>
> Hi.
> 
> Like all userspace ABI, this needs documentation.
> 
> It's an unusual feature, so some usecases would help.
> 
> It is also cross multiple devices which makes this odd as only one device
> can presumably acquire the gpio?
> 
> An alternative would be to look at how to do this with a 'wrapper' sort of device
> so that we have one instance to which this applies.
> 
> I'm not sure that helps that much though as we'd still need some for of
> 'I'm setup for all channels, now you can go' ABI.
> 
> Jonathan
> 

Giving userspace direct control over the /SYNC pin without coordinating
with the rest of the driver does seem like it could be asking for trouble.

It seems like the only time you would want to actually toggle the /SYNC
pin is when starting a buffered read.

1. Deassert /SYNC so that no conversions can be triggered.
2. Enable buffered reads for all chips connected to the same GPIO.
3. Assert /SYNC to start all conversions at the same time.

So it could make sense to integrate this into the buffer pre/post enable
callbacks somehow instead of adding a new sysfs attribute.

For the "wrapper" device, maybe we could do something with configfs to
enable dynamically connecting multiple device instances? We might not
need to actually create a separate device in sysfs, but just do something
so that enabling a buffered read on the first chip will enable buffered
reads on all of the chips in the group.

It seems like we have some other chips that are currently being worked on
that also have the possibility of some sort of multi-chip synchronization
like this so it would be nice to come up with a general solution.

Another use case for a general synchronized buffered read/write between
multiple chips would be the AD3552R DAC. Recently, while adding support
for an IIO backend for this chip, we saw that the AXI DAC backend has a
synchronization feature like this where you set an "arm" bit on all AXI
DAC instances. Then when you enable streaming to the first chip, it also
triggers all of the other AXI DAC blocks to start streaming at the same
time. We ended up not implementing that feature since the IIO subsystem
doesn't really support this yet, but could be a good one to look at as a
similar feature with a different implementation to help us find a general
solution.