linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
To: "jic23@kernel.org" <jic23@kernel.org>
Cc: "lars@metafoo.de" <lars@metafoo.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Subject: Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
Date: Sun, 17 May 2020 06:26:17 +0000	[thread overview]
Message-ID: <0727f57d1faccb08dff24b6f9343754f3598e76e.camel@analog.com> (raw)
In-Reply-To: <20200516172445.7f0d96cb@archlinux>

On Sat, 2020-05-16 at 17:24 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Sat, 16 May 2020 13:08:46 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote:
> > > [External]
> > > 
> > > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote:  
> > > > [External]
> > > > 
> > > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:  
> > > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:  
> > > > > > [External]
> > > > > > 
> > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:  
> > > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:  
> > > > > > > > [External]
> > > > > > > > 
> > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:  
> > > > > > > > > [External]
> > > > > > > > > 
> > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:  
> > > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:  
> > > > > > > > > > > [External]
> > > > > > > > > > > 
> > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > > > > > >   
> > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:  
> > > > > > > > > > > > > [...]
> > > > > > > > > > > > > What I don't like, is that iio:device3 has
> > > > > > > > > > > > > iio:buffer3:0
> > > > > > > > > > > > > (to
> > > > > > > > > > > > > 3).
> > > > > > > > > > > > > This is because the 'buffer->dev.parent =
> > > > > > > > > > > > > &indio_dev-  
> > > > > > > > > > > > > > dev'.  
> > > > > > > > > > > > > But I do feel this is correct.
> > > > > > > > > > > > > So, now I don't know whether to leave it like that or
> > > > > > > > > > > > > symlink to
> > > > > > > > > > > > > shorter
> > > > > > > > > > > > > versions like 'iio:buffer3:Y' ->
> > > > > > > > > > > > > 'iio:device3/bufferY'.
> > > > > > > > > > > > > The reason for naming the IIO buffer devices to
> > > > > > > > > > > > > 'iio:bufferX:Y'
> > > > > > > > > > > > > is
> > > > > > > > > > > > > mostly to make the names unique. It would have looked
> > > > > > > > > > > > > weird
> > > > > > > > > > > > > to
> > > > > > > > > > > > > do
> > > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer
> > > > > > > > > > > > > devices
> > > > > > > > > > > > > 'bufferX'.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > So, now I'm thinking of whether all this is
> > > > > > > > > > > > > acceptable.
> > > > > > > > > > > > > Or what is acceptable?
> > > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > > > > > > > > 'iio:device3/buffer0'?
> > > > > > > > > > > > > What else should I consider moving forward?
> > > > > > > > > > > > > What means forward?
> > > > > > > > > > > > > Where did I leave my beer?  
> > > > > > > > > > > > Looking at how the /dev/ devices are named I think we
> > > > > > > > > > > > can
> > > > > > > > > > > > provide
> > > > > > > > > > > > a
> > > > > > > > > > > > name
> > > > > > > > > > > > that is different from the dev_name() of the device.
> > > > > > > > > > > > Have a
> > > > > > > > > > > > look
> > > > > > > > > > > > at
> > > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should
> > > > > > > > > > > > be
> > > > > > > > > > > > able
> > > > > > > > > > > > to
> > > > > > > > > > > > provide the name for the chardev through the devnode()
> > > > > > > > > > > > callback.
> > > > > > > > > > > > 
> > > > > > > > > > > > While we are at this, do we want to move the new devices
> > > > > > > > > > > > into
> > > > > > > > > > > > an
> > > > > > > > > > > > iio
> > > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?  
> > > > > > > > > > > Possibly on the folder.  I can't for the life of me
> > > > > > > > > > > remember
> > > > > > > > > > > why
> > > > > > > > > > > I
> > > > > > > > > > > decided
> > > > > > > > > > > not to do that the first time around - I'll leave it at
> > > > > > > > > > > the
> > > > > > > > > > > mysterious "it may turn out to be harder than you'd
> > > > > > > > > > > think..."
> > > > > > > > > > > Hopefully not ;)  
> > > > > > > > > > I was also thinking about the /dev/iio subfolder while doing
> > > > > > > > > > this.
> > > > > > > > > > I can copy that from /dev/input
> > > > > > > > > > They seem to do it already.
> > > > > > > > > > I don't know how difficult it would be. But it looks like a
> > > > > > > > > > good
> > > > > > > > > > precedent.  
> > > > > > > > > All you have to do is return "iio/..." from the devnode()
> > > > > > > > > callback.  
> > > > > > > > I admit I did not look closely into drivers/input/input.c before
> > > > > > > > mentioning
> > > > > > > > this
> > > > > > > > as as good precedent.
> > > > > > > > 
> > > > > > > > But, I looks like /dev/inpput is a class.
> > > > > > > > While IIO devices are a bus_type devices.
> > > > > > > > Should we start implementing an IIO class? or?  
> > > > > > > What I should have highlighted [before] with this, is that there
> > > > > > > is no
> > > > > > > devnode()
> > > > > > > callback for the bus_type [type].  
> > > > > > But there is one in device_type :)  
> > > > > Many thanks :)
> > > > > That worked nicely.
> > > > > 
> > > > > I now have:
> > > > > 
> > > > > root@analog:~# ls /dev/iio/*
> > > > > /dev/iio/iio:device0  /dev/iio/iio:device1
> > > > > 
> > > > > /dev/iio/device3:
> > > > > buffer0  buffer1  buffer2  buffer3
> > > > > 
> > > > > /dev/iio/device4:
> > > > > buffer0
> > > > > 
> > > > > 
> > > > > It looks like I can shift these around as needed.
> > > > > This is just an experiment.
> > > > > I managed to move the iio devices under /dev/iio, though probably the
> > > > > IIO
> > > > > devices will still be around as /dev/iio:deviceX for legacy reasons.
> > > > > 
> > > > > Two things remain unresolved.
> > > > > 1. The name of the IIO buffer device.
> > > > > 
> > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/
> > > > > buffer          in_voltage0_test_mode           name
> > > > > events          in_voltage1_test_mode           of_node
> > > > > iio:buffer:3:0  in_voltage_sampling_frequency   power
> > > > > iio:buffer:3:1  in_voltage_scale                scan_elements
> > > > > iio:buffer:3:2  in_voltage_scale_available      subsystem
> > > > > iio:buffer:3:3  in_voltage_test_mode_available  uevent
> > > > > 
> > > > > 
> > > > > Right now, each buffer device is named 'iio:buffer:X:Y'.
> > > > > One suggesttion was  'iio:deviceX:bufferY'
> > > > > I'm suspecting the latter is preferred as when you sort the folders,
> > > > > buffers
> > > > > come right after the iio:deviceX folders in /sys/bus/iio/devices.
> > > > > 
> > > > > I don't feel it matters much the device name of the IIO buffer if we
> > > > > symlink
> > > > > it
> > > > > to a shorter form.
> > > > >   
> > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders
> > > > > in
> > > > > each
> > > > > 'iio:deviceX'?  
> > > > 
> > > > I think that would be a bit excessive. Only for the legacy buffer we 
> > > > need to have a symlink.
> > > >   
> > > > > [...]
> > > > > 2. I know this is [still] stupid now; but any suggestions one how to
> > > > > symlink
> > > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ?
> > > > >   
> > > > Does not seem to be possible. Userspace will have to take care of it. 
> > > > This means we need to keep legacy devices in /dev/ and only new buffers 
> > > > in /dev/iio/.  
> > > 
> > > One thought about this, was that we keep the chardev for the IIO device
> > > for
> > > this.
> > > i.e.  /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer.
> > > This means that for a device with 4 buffers, you get 5 chardevs.
> > > This also seems a bit much/excessive. Maybe also in terms of source-code.
> > > It would at least mean not moving the event-only chardev to 'industrialio-
> > > event.c', OR move it, and have the same chardev in 3 places
> > > ['industrialio-
> > > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c'
> > > 
> > > Maybe this sort-of makes sense to have for a few years/kernel-revisions
> > > until
> > > things clean-up.
> > > 
> > > I guess at this point, the maintainer should have the final say about
> > > this.  
> > 
> > Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY'
> > thing
> > a feature for new devices, and leave '/dev/iio:deviceX' devices [for
> > buffers] a
> > thing for current devices.
> > It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name
> > [for
> > this yet].
> 
> Definitely a no to that.  If we make this transition it needs to be
> automatic and subsystem wide.  At some point we could have a kconfig option
> to disable the legacy interface subsystem wise as a precursor to eventually
> dropping it.  
> 
> > Over time, people can convert existing drivers to the new IIO-buffer format,
> > if
> > they want to. That also gives them a bit better control over symlinking
> > '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse
> > if
> > they want to].
> > 
> > That may create confusion I guess during a transition period.
> > And it would [ideally] have a mechanism [preferably at build/compile time]
> > to
> > notify users to use the new IIO buffer mechanism [vs the old one] when
> > adding
> > new drivers.
> > Otherwise, there's the risk of people copying the old IIO buffer mechanism.
> > This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying.
> 
> If we can't do this in a transparent fashion we need to rethink.
> The existing interface 'has' to remain and do something sensible.
> Realistically
> we need to keep it in place for 3-5 years at least.
> 
> I'm not yet convinced the complexity is worthwhile.  We 'could' fallback to
> the same trick used for events and use an ioctl to access all buffers
> other than the first one...  Then we retain one chardev per iio device
> and still get the flexibility we need to have multiple buffers.
> In some ways it is tidier, even if a bit less intuitive...
> If we can't build the symlinks we were all kind of assuming we could
> we may need to rethink the overall path.
> 
> Anyhow, you are doing great work exploring the options!

I wonder if you got to read the idea about adding more chardevs.

The one where /dev/iio:deviceX & /dev/iio/deviceX/buffer0 open the same buffer
object. I'm not sure about any race issues here.
The bad-part [I feel] is that we get more duplication on chardev file_operations
(open, release, ioctl, etc).
We need to re-wrap code-paths so that they open the same buffer.
And the number of chardevs per IIO device increases by 1 (for a device with 4
buffers == 4 chardevs + 1 legacy)

I kinda like the idea of the /dev/iio/ folder. But I'm not strongly opinionated
towards it either.
This also allows some /dev/iio/deviceX/eventY chardevs.
And some other types of chardevs /dev/iio/deviceX/somethingNewY

But, if I think about it, the only benefit of this [over anon inodes for
chardevs] is that it allows us to do direct access via cat/echo to the actual
chardev of the buffer.
Then, there's also the fact that adding more chardevs increases complexity to
userspace, so it won't matter much. People would probably prefer some userspace
IIO library to do the data read/write.

I'm getting the feeling now that the final debathe is 'anon inodes or not'

Thoughts here?


> 
> Thanks,
> 
> Jonathan
> 
> 
> > 
> > >   
> > > >   

  reply	other threads:[~2020-05-17  6:26 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 01/14] iio: Move scan mask management to the core Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 02/14] iio: hw_consumer: use new scanmask functions Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 03/14] iio: buffer: add back-ref from iio_buffer to iio_dev Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 04/14] iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put() Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 05/14] iio: core: register chardev only if needed Alexandru Ardelean
2020-05-24 16:40   ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 06/14] iio: buffer,event: duplicate chardev creation for buffers & events Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 07/14] iio: core: add simple centralized mechanism for ioctl() handlers Alexandru Ardelean
2020-05-24 16:45   ` Jonathan Cameron
2020-05-25  7:24     ` Ardelean, Alexandru
2020-05-08 13:53 ` [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism Alexandru Ardelean
2020-05-24 16:47   ` Jonathan Cameron
2020-05-25  7:27     ` Ardelean, Alexandru
2020-05-31 15:20       ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg Alexandru Ardelean
2020-05-24 16:49   ` Jonathan Cameron
2020-05-25  7:28     ` Ardelean, Alexandru
2020-05-31 15:21       ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 10/14] iio: buffer: remove attrcount_orig var from sysfs creation Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 11/14] iio: buffer: add underlying device object and convert buffers to devices Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 12/14] iio: buffer: symlink the scan_elements dir back into IIO device's dir Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 13/14] iio: unpack all iio buffer attributes correctly Alexandru Ardelean
2020-05-24 17:28   ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 14/14] iio: buffer: convert single buffer to list of buffers Alexandru Ardelean
2020-05-09  8:52 ` [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Lars-Peter Clausen
2020-05-10 10:09   ` Jonathan Cameron
2020-05-11 10:33     ` Ardelean, Alexandru
2020-05-11 10:37       ` Lars-Peter Clausen
2020-05-11 13:03         ` Ardelean, Alexandru
2020-05-11 13:24           ` Ardelean, Alexandru
2020-05-11 13:58             ` Lars-Peter Clausen
2020-05-11 14:56               ` Ardelean, Alexandru
2020-05-11 19:56                 ` Lars-Peter Clausen
2020-05-12  6:26                   ` Ardelean, Alexandru
2020-05-16 13:08                     ` Ardelean, Alexandru
2020-05-16 16:24                       ` Jonathan Cameron
2020-05-17  6:26                         ` Ardelean, Alexandru [this message]
2020-05-17 13:40                           ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0727f57d1faccb08dff24b6f9343754f3598e76e.camel@analog.com \
    --to=alexandru.ardelean@analog.com \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).