| Message ID | 20200410141729.82834-2-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: core,buffer: re-organize chardev creation | expand |
On Fri, 10 Apr 2020 17:17:25 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > The intent is to localize all buffer ops into the industrialio-buffer.c > file, to be able to add support for multiple buffers per IIO device. > > We still need to allocate a chardev in __iio_device_register() to be able > to pass event ioctl commands. So, if the IIO device has no buffer, we > create the legacy chardev for the event ioctl() command. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Whilst we are here, can we avoid allocating the chardev at all if we have neither buffer support, nor events? So don't add the chrdev to the device. That covers quite a wide range of slow devices and is a nice incidental improvement (to be honest I'd forgotten we actually created a chardev in those circumstance :( Jonathan > --- > drivers/iio/industrialio-core.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 157d95a24faa..c8c074602709 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1707,6 +1707,15 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev) > > static const struct iio_buffer_setup_ops noop_ring_setup_ops; > > +static const struct file_operations iio_event_fileops = { > + .release = iio_chrdev_release, > + .open = iio_chrdev_open, > + .owner = THIS_MODULE, > + .llseek = noop_llseek, > + .unlocked_ioctl = iio_ioctl, > + .compat_ioctl = compat_ptr_ioctl, > +}; > + > int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > { > int ret; > @@ -1757,7 +1766,10 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > indio_dev->setup_ops == NULL) > indio_dev->setup_ops = &noop_ring_setup_ops; > > - cdev_init(&indio_dev->chrdev, &iio_buffer_fileops); > + if (indio_dev->buffer) > + cdev_init(&indio_dev->chrdev, &iio_buffer_fileops); > + else > + cdev_init(&indio_dev->chrdev, &iio_event_fileops); > > indio_dev->chrdev.owner = this_mod; >
On Mon, 2020-04-13 at 16:47 +0100, Jonathan Cameron wrote: > [External] > > On Fri, 10 Apr 2020 17:17:25 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > The intent is to localize all buffer ops into the industrialio-buffer.c > > file, to be able to add support for multiple buffers per IIO device. > > > > We still need to allocate a chardev in __iio_device_register() to be able > > to pass event ioctl commands. So, if the IIO device has no buffer, we > > create the legacy chardev for the event ioctl() command. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Whilst we are here, can we avoid allocating the chardev at all if > we have neither buffer support, nor events? So don't add the chrdev to the > device. That should happen after patch 5/5. If there aren't any buffers, and 'indio_dev->event_interface' is NULL, no chardev should exist. Maybe I can reduce this, given the fact that this goes away into files later. I did things in very-small incremental steps that I later squashed. This patch kind of highlights an intermediate step towards the final rework [moving chardevs into files] > > That covers quite a wide range of slow devices and is a nice incidental > improvement (to be honest I'd forgotten we actually created a chardev > in those circumstance :( > > Jonathan > > > --- > > drivers/iio/industrialio-core.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio- > > core.c > > index 157d95a24faa..c8c074602709 100644 > > --- a/drivers/iio/industrialio-core.c > > +++ b/drivers/iio/industrialio-core.c > > @@ -1707,6 +1707,15 @@ static int iio_check_unique_scan_index(struct iio_dev > > *indio_dev) > > > > static const struct iio_buffer_setup_ops noop_ring_setup_ops; > > > > +static const struct file_operations iio_event_fileops = { > > + .release = iio_chrdev_release, > > + .open = iio_chrdev_open, > > + .owner = THIS_MODULE, > > + .llseek = noop_llseek, > > + .unlocked_ioctl = iio_ioctl, > > + .compat_ioctl = compat_ptr_ioctl, > > +}; > > + > > int __iio_device_register(struct iio_dev *indio_dev, struct module > > *this_mod) > > { > > int ret; > > @@ -1757,7 +1766,10 @@ int __iio_device_register(struct iio_dev *indio_dev, > > struct module *this_mod) > > indio_dev->setup_ops == NULL) > > indio_dev->setup_ops = &noop_ring_setup_ops; > > > > - cdev_init(&indio_dev->chrdev, &iio_buffer_fileops); > > + if (indio_dev->buffer) > > + cdev_init(&indio_dev->chrdev, &iio_buffer_fileops); > > + else > > + cdev_init(&indio_dev->chrdev, &iio_event_fileops); > > > > indio_dev->chrdev.owner = this_mod; > >
On Tue, 2020-04-14 at 05:20 +0000, Ardelean, Alexandru wrote: > [External] > > On Mon, 2020-04-13 at 16:47 +0100, Jonathan Cameron wrote: > > [External] > > > > On Fri, 10 Apr 2020 17:17:25 +0300 > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > The intent is to localize all buffer ops into the industrialio-buffer.c > > > file, to be able to add support for multiple buffers per IIO device. > > > > > > We still need to allocate a chardev in __iio_device_register() to be able > > > to pass event ioctl commands. So, if the IIO device has no buffer, we > > > create the legacy chardev for the event ioctl() command. > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > Whilst we are here, can we avoid allocating the chardev at all if > > we have neither buffer support, nor events? So don't add the chrdev to the > > device. > > That should happen after patch 5/5. > If there aren't any buffers, and 'indio_dev->event_interface' is NULL, no > chardev should exist. > > Maybe I can reduce this, given the fact that this goes away into files later. > I did things in very-small incremental steps that I later squashed. > > This patch kind of highlights an intermediate step towards the final rework > [moving chardevs into files] oh... silly me we can check if 'indio_dev->event_interface' is NULL here as well > > > That covers quite a wide range of slow devices and is a nice incidental > > improvement (to be honest I'd forgotten we actually created a chardev > > in those circumstance :( > > > > Jonathan > > > > > --- > > > drivers/iio/industrialio-core.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio- > > > core.c > > > index 157d95a24faa..c8c074602709 100644 > > > --- a/drivers/iio/industrialio-core.c > > > +++ b/drivers/iio/industrialio-core.c > > > @@ -1707,6 +1707,15 @@ static int iio_check_unique_scan_index(struct > > > iio_dev > > > *indio_dev) > > > > > > static const struct iio_buffer_setup_ops noop_ring_setup_ops; > > > > > > +static const struct file_operations iio_event_fileops = { > > > + .release = iio_chrdev_release, > > > + .open = iio_chrdev_open, > > > + .owner = THIS_MODULE, > > > + .llseek = noop_llseek, > > > + .unlocked_ioctl = iio_ioctl, > > > + .compat_ioctl = compat_ptr_ioctl, > > > +}; > > > + > > > int __iio_device_register(struct iio_dev *indio_dev, struct module > > > *this_mod) > > > { > > > int ret; > > > @@ -1757,7 +1766,10 @@ int __iio_device_register(struct iio_dev > > > *indio_dev, > > > struct module *this_mod) > > > indio_dev->setup_ops == NULL) > > > indio_dev->setup_ops = &noop_ring_setup_ops; > > > > > > - cdev_init(&indio_dev->chrdev, &iio_buffer_fileops); > > > + if (indio_dev->buffer) > > > + cdev_init(&indio_dev->chrdev, &iio_buffer_fileops); > > > + else > > > + cdev_init(&indio_dev->chrdev, &iio_event_fileops); > > > > > > indio_dev->chrdev.owner = this_mod; > > >
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 157d95a24faa..c8c074602709 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1707,6 +1707,15 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev) static const struct iio_buffer_setup_ops noop_ring_setup_ops; +static const struct file_operations iio_event_fileops = { + .release = iio_chrdev_release, + .open = iio_chrdev_open, + .owner = THIS_MODULE, + .llseek = noop_llseek, + .unlocked_ioctl = iio_ioctl, + .compat_ioctl = compat_ptr_ioctl, +}; + int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) { int ret; @@ -1757,7 +1766,10 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) indio_dev->setup_ops == NULL) indio_dev->setup_ops = &noop_ring_setup_ops; - cdev_init(&indio_dev->chrdev, &iio_buffer_fileops); + if (indio_dev->buffer) + cdev_init(&indio_dev->chrdev, &iio_buffer_fileops); + else + cdev_init(&indio_dev->chrdev, &iio_event_fileops); indio_dev->chrdev.owner = this_mod;
The intent is to localize all buffer ops into the industrialio-buffer.c file, to be able to add support for multiple buffers per IIO device. We still need to allocate a chardev in __iio_device_register() to be able to pass event ioctl commands. So, if the IIO device has no buffer, we create the legacy chardev for the event ioctl() command. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/industrialio-core.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)