On Mon, Jan 25, 2021 at 9:32 PM Greg Kroah-Hartman
[off-list ref] wrote:

On Sun, Jan 24, 2021 at 06:11:26PM +0000, Jonathan Cameron wrote:

quoted

On Fri, 22 Jan 2021 18:25:20 +0200
Alexandru Ardelean [off-list ref] wrote:

quoted

When adding more than one IIO buffer per IIO device, we will need to create
a buffer & scan_elements directory for each buffer.
We also want to move the 'scan_elements' to be a sub-directory of the
'buffer' folder.

The format we want to reach is, for a iio:device0 folder, for 2 buffers
[for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
it's own 'scan_elements' subfolder.

So, for example:
   iio:device0/buffer0
      scan_elements/

   iio:device0/buffer1
      scan_elements/

The other attributes under 'bufferX' would remain unchanged.

However, we would also need to symlink back to the old 'buffer' &
'scan_elements' folders, to keep backwards compatibility.

Doing all these, require that we maintain the kobjects for each 'bufferX'
and 'scan_elements' so that we can symlink them back. We also need to
implement the sysfs_ops for these folders.

Signed-off-by: Alexandru Ardelean <redacted>

+CC GregKH and Rafael W for feedback on various things inline.

It might be that this is the neatest solution that we can come up with but
more eyes would be good!

In short, please do NOT do this.

At all.

no.

{sigh}

quoted

Whilst I think this looks fine, I'm less confident than I'd like to be.

Jonathan

quoted

---
 drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++---
 drivers/iio/industrialio-core.c   |  24 ++--
 include/linux/iio/buffer_impl.h   |  14 ++-
 include/linux/iio/iio.h           |   2 +-
 4 files changed, 200 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 0412c4fda4c1..0f470d902790 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c

@@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
    return (ret < 0) ? ret : len;
 }

-static const char * const iio_scan_elements_group_name = "scan_elements";
-
 static ssize_t iio_buffer_show_watermark(struct device *dev,
                                     struct device_attribute *attr,
                                     char *buf)

@@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = {
    &dev_attr_data_available.attr,
 };

+#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
+
+static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
+                                   struct attribute *attr,
+                                   char *buf)
+{
+   struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
+   struct device_attribute *dattr;
+
+   dattr = to_dev_attr(attr);
+
+   return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+}

First off, you are dealing with "raw" kobjects here, below a 'struct
device' in the device tree, which means that suddenly userspace does not
know what in the world is going on, and you lost events and lots of
other stuff.

Never do this.  It should not be needed, and you are just trying to
paper over one odd decision of an api with another one you will be stuck
with for forever.

Remember the driver core can create subdirectories for your attributes
automatically if you want them to be in a subdir, but that's it, no
further than that.  Just name the attribute group.

But yes, you can not create a symlink to there, because (surprise), you
don't want to!

So please, just rethink your naming, create a totally new naming scheme
for multiple entities, and just drop the old one (or keep a single
value if you really want to.)  Don't make it harder than it has to be
please, you can never remove the "compatible symlinks", just make a new
api and move on.

So, coming back to Jonathan.
Any thoughts on how to proceed?

We could merge the files 'buffer & scan_elements' [from in the
/sys/bus/iio/devices/iio:deviceX/{buffer,scan_elements}

So, essentially:
# ls /sys/bus/iio/devices/iio:deviceX/bufferY
data_available       length              watermark
enable                   length_align_bytes
in_voltage0_en      in_voltage0_type   in_voltage1_index
in_voltage0_index  in_voltage1_en     in_voltage1_type

Where:
# ls  /sys/bus/iio/devices/iio:deviceX/scan_elements
in_voltage0_en     in_voltage0_type   in_voltage1_index
in_voltage0_index  in_voltage1_en     in_voltage1_typ

# ls  /sys/bus/iio/devices/iio:deviceX/buffer
data_available      length              watermark
enable              length_align_bytes

I don't think we need to add any prefixes for the scan_elements/buffer
files, or?

Do we still do this new ioctl() for buffer0, 1, 2, N being accessed
via anon inodes?
Or do we go [back] via the route of each buffer with it's own chardev?
i.e.  introduce a "/dev/iio/deviceX/bufferY" structure

I'm fine either way.

Thanks
Alex

thanks,

greg k-h