by Jonathan Cameron

[permalink] [raw]

Subject: Re: [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg

On Mon, 25 May 2020 07:28:18 +0000
"Ardelean, Alexandru" <[email protected]> wrote:

> On Sun, 2020-05-24 at 17:49 +0100, Jonathan Cameron wrote:
> > [External]
> >
> > On Fri, 8 May 2020 16:53:43 +0300
> > Alexandru Ardelean <[email protected]> wrote:
> >
> > > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
> > > primary argument. This change converts to take an IIO buffer as a primary
> > > argument.
> > >
> > > That will allow the functions to get called for multiple buffers.
> > >
> > > Signed-off-by: Alexandru Ardelean <[email protected]>
> >
> > Looks good to me. We'll need this whatever the interface ends up being as
> > need the separate control infrastructure.
>
> I was wanting to split this into it's own patch at some point and send it now.
> I'll probably do it.

Great.
>
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
> > > 1 file changed, 30 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > > buffer.c
> > > index e7a847e7b103..6b1b5d5387bd 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1312,26 +1312,14 @@ static struct attribute *iio_buffer_attrs[] = {
> > > &dev_attr_data_available.attr,
> > > };
> > >
> > > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
> > > {
> > > + struct iio_dev *indio_dev = buffer->indio_dev;
> > > struct iio_dev_attr *p;
> > > struct attribute **attr;
> > > - struct iio_buffer *buffer = indio_dev->buffer;
> > > int ret, i, attrn, attrcount, attrcount_orig = 0;
> > > const struct iio_chan_spec *channels;
> > >
> > > - channels = indio_dev->channels;
> > > - if (channels) {
> > > - int ml = indio_dev->masklength;
> > > -
> > > - for (i = 0; i < indio_dev->num_channels; i++)
> > > - ml = max(ml, channels[i].scan_index + 1);
> > > - indio_dev->masklength = ml;
> > > - }
> > > -
> > > - if (!buffer)
> > > - return 0;
> > > -
> > > attrcount = 0;
> > > if (buffer->attrs) {
> > > while (buffer->attrs[attrcount] != NULL)
> > > @@ -1411,19 +1399,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > > return ret;
> > > }
> > >
> > > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > {
> > > struct iio_buffer *buffer = indio_dev->buffer;
> > > + const struct iio_chan_spec *channels;
> > > + int i;
> > > +
> > > + channels = indio_dev->channels;
> > > + if (channels) {
> > > + int ml = indio_dev->masklength;
> > > +
> > > + for (i = 0; i < indio_dev->num_channels; i++)
> > > + ml = max(ml, channels[i].scan_index + 1);
> > > + indio_dev->masklength = ml;
> > > + }
> > >
> > > if (!buffer)
> > > - return;
> > > + return 0;
> > > +
> > > + return __iio_buffer_alloc_sysfs_and_mask(buffer);
> > > +}
> > >
> > > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> > > +{
> > > iio_buffer_free_scanmask(buffer);
> > > kfree(buffer->buffer_group.attrs);
> > > kfree(buffer->scan_el_group.attrs);
> > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > > }
> > >
> > > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +{
> > > + struct iio_buffer *buffer = indio_dev->buffer;
> > > +
> > > + if (!buffer)
> > > + return;
> > > +
> > > + __iio_buffer_free_sysfs_and_mask(buffer);
> > > +}
> > > +
> > > static const struct file_operations iio_buffer_fileops = {
> > > .read = iio_buffer_read_outer,
> > > .release = iio_buffer_chrdev_release,

2020-05-31 15:59:19

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism

On Mon, 25 May 2020 07:27:43 +0000
"Ardelean, Alexandru" <[email protected]> wrote:

> On Sun, 2020-05-24 at 17:47 +0100, Jonathan Cameron wrote:
> > [External]
> >
> > On Fri, 8 May 2020 16:53:42 +0300
> > Alexandru Ardelean <[email protected]> wrote:
> >
> > > This change makes use of the new centralized ioctl() mechanism. The event
> > > interface registers it's ioctl() handler to IIO device.
> > > Both the buffer & event interface call 'iio_device_ioctl()', which should
> > > take care of all of indio_dev's ioctl() calls.
> > >
> > > Later, we may add per-buffer ioctl() calls, and since each buffer will get
> > > it's own chardev, the buffer ioctl() handler will need a bit of tweaking
> > > for the first/legacy buffer (i.e. indio_dev->buffer).
> >
> > Do we have an ioctls that aren't safe if we just use them on 'any'
> > buffer? I don't think we do yet, though I guess we may have in the future.
> >
>
> This was done in the idea that we may want to keep the /dev/iio:deviceX for
> backwards compatibility.
> But, it's undetermined yet how this will look.
> I am currently working on more rework stuff [as you've seen].
> I'd try to do some of the rework now, before adding more stuff [like
> iio_dev_opaque].
> To avoid adding this work, then having to rework it.

Agreed. You are being so productive at the moment you may end up blocking
yourself. Best perhaps to slow down and clear some of the backlog.

Jonathan

>
> >
> > > Also, those per-buffer ioctl() calls will not be registered with this
> > > mechanism.
> > >
> > > Signed-off-by: Alexandru Ardelean <[email protected]>
> > > ---
> > > drivers/iio/iio_core.h | 3 ---
> > > drivers/iio/industrialio-buffer.c | 2 +-
> > > drivers/iio/industrialio-event.c | 14 ++++++++------
> > > 3 files changed, 9 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > index 34c3e19229d8..f68de4af2738 100644
> > > --- a/drivers/iio/iio_core.h
> > > +++ b/drivers/iio/iio_core.h
> > > @@ -54,9 +54,6 @@ ssize_t iio_format_value(char *buf, unsigned int type, int
> > > size, int *vals);
> > > #ifdef CONFIG_IIO_BUFFER
> > > struct poll_table_struct;
> > >
> > > -long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > > - unsigned int cmd, unsigned long arg);
> > > -
> > > void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev);
> > >
> > > int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > > buffer.c
> > > index 1400688f5e42..e7a847e7b103 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1199,7 +1199,7 @@ static long iio_buffer_ioctl(struct file *filep,
> > > unsigned int cmd,
> > > if (!buffer || !buffer->access)
> > > return -ENODEV;
> > >
> > > - return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
> > > + return iio_device_ioctl(buffer->indio_dev, filep, cmd, arg);
> > > }
> > >
> > > static ssize_t iio_buffer_store_enable(struct device *dev,
> > > diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-
> > > event.c
> > > index 0674b2117c98..1961c1d19370 100644
> > > --- a/drivers/iio/industrialio-event.c
> > > +++ b/drivers/iio/industrialio-event.c
> > > @@ -32,6 +32,7 @@
> > > * @read_lock: lock to protect kfifo read operations
> > > * @chrdev: associated chardev for this event
> > > * @indio_dev: IIO device to which this event interface belongs
> > > to
> > > + * @ioctl_handler: handler for event ioctl() calls
> > > */
> > > struct iio_event_interface {
> > > wait_queue_head_t wait;
> > > @@ -44,6 +45,7 @@ struct iio_event_interface {
> > >
> > > struct cdev chrdev;
> > > struct iio_dev *indio_dev;
> > > + struct iio_ioctl_handler ioctl_handler;
> > > };
> > >
> > > bool iio_event_enabled(const struct iio_event_interface *ev_int)
> > > @@ -261,15 +263,12 @@ static int iio_chrdev_release(struct inode *inode,
> > > struct file *filp)
> > > return 0;
> > > }
> > >
> > > -long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > > +static long iio_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > > unsigned int cmd, unsigned long arg)
> > > {
> > > int __user *ip = (int __user *)arg;
> > > int fd;
> > >
> > > - if (!indio_dev->info)
> > > - return -ENODEV;
> > > -
> > > if (cmd == IIO_GET_EVENT_FD_IOCTL) {
> > > fd = iio_event_getfd(indio_dev);
> > > if (fd < 0)
> > > @@ -278,7 +277,7 @@ long iio_device_event_ioctl(struct iio_dev *indio_dev,
> > > struct file *filp,
> > > return -EFAULT;
> > > return 0;
> > > }
> > > - return -EINVAL;
> > > + return IIO_IOCTL_UNHANDLED;
> > > }
> > >
> > > static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> > > @@ -286,7 +285,7 @@ static long iio_event_ioctl_wrapper(struct file *filp,
> > > unsigned int cmd,
> > > {
> > > struct iio_event_interface *ev = filp->private_data;
> > >
> > > - return iio_device_event_ioctl(ev->indio_dev, filp, cmd, arg);
> > > + return iio_device_ioctl(ev->indio_dev, filp, cmd, arg);
> > > }
> > >
> > > static const struct file_operations iio_event_fileops = {
> > > @@ -308,7 +307,10 @@ void iio_device_event_attach_chrdev(struct iio_dev
> > > *indio_dev)
> > > cdev_init(&ev->chrdev, &iio_event_fileops);
> > >
> > > ev->indio_dev = indio_dev;
> > > + ev->ioctl_handler.ioctl = iio_event_ioctl;
> > > indio_dev->chrdev = &ev->chrdev;
> > > +
> > > + iio_device_ioctl_handler_register(indio_dev, &ev->ioctl_handler);
> > > }
> > >
> > > static const char * const iio_ev_type_text[] = {