| Message ID | 20201117162340.43924-2-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,01/12] iio: core: register chardev only if needed | expand |
On Tue, 17 Nov 2020 18:23:29 +0200 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > We only need a chardev if we need to support buffers and/or events. > > With this change, a chardev will be created only if an IIO buffer is > attached OR an event_interface is configured. > > Otherwise, no chardev will be created, and the IIO device will get > registered with the 'device_add()' call. > > Quite a lot of IIO devices don't really need a chardev, so this is a minor > improvement to the IIO core, as the IIO device will take up (slightly) > fewer resources. > > In order to not create a chardev, we mostly just need to not initialize the > indio_dev->dev.devt field. If that is un-initialized, cdev_device_add() > behaves like device_add(). > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> I'll be honest. I have no idea why I didn't do this in first place! I 'think' we are safe dropping this but I suppose it's possible some odd code checks for the chrdev presence? Hopefully not though. Jonathan > --- > drivers/iio/industrialio-core.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 419d6f8acc13..ca8b11541477 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1763,6 +1763,15 @@ static const struct file_operations iio_buffer_fileops = { > .compat_ioctl = compat_ptr_ioctl, > }; > > +static const struct file_operations iio_event_fileops = { > + .owner = THIS_MODULE, > + .llseek = noop_llseek, > + .unlocked_ioctl = iio_ioctl, > + .compat_ioctl = compat_ptr_ioctl, > + .open = iio_chrdev_open, > + .release = iio_chrdev_release, > +}; > + > static int iio_check_unique_scan_index(struct iio_dev *indio_dev) > { > int i, j; > @@ -1790,6 +1799,7 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops; > > int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > { > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > int ret; > > if (!indio_dev->info) > @@ -1807,9 +1817,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > if (ret < 0) > return ret; > > - /* configure elements for the chrdev */ > - indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id); > - > iio_device_register_debugfs(indio_dev); > > ret = iio_buffer_alloc_sysfs_and_mask(indio_dev); > @@ -1838,9 +1845,15 @@ 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 if (iio_dev_opaque->event_interface) > + cdev_init(&indio_dev->chrdev, &iio_event_fileops); > > - indio_dev->chrdev.owner = this_mod; > + if (indio_dev->buffer || iio_dev_opaque->event_interface) { > + indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id); > + indio_dev->chrdev.owner = this_mod; > + } > > ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev); > if (ret < 0)
On Sat, Nov 21, 2020 at 8:05 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Tue, 17 Nov 2020 18:23:29 +0200 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > We only need a chardev if we need to support buffers and/or events. > > > > With this change, a chardev will be created only if an IIO buffer is > > attached OR an event_interface is configured. > > > > Otherwise, no chardev will be created, and the IIO device will get > > registered with the 'device_add()' call. > > > > Quite a lot of IIO devices don't really need a chardev, so this is a minor > > improvement to the IIO core, as the IIO device will take up (slightly) > > fewer resources. > > > > In order to not create a chardev, we mostly just need to not initialize the > > indio_dev->dev.devt field. If that is un-initialized, cdev_device_add() > > behaves like device_add(). > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > I'll be honest. I have no idea why I didn't do this in first place! > > I 'think' we are safe dropping this but I suppose it's possible some > odd code checks for the chrdev presence? So, libiio at least doesn't rely on this being there for any odd things. But yeah, who knows what else is out there that might. I was also thinking of sending this separately to have this earlier out there in case it bothers other people. I guess I got a little mixed by other patches and re-ordered things a few times and this remained in this series. > > Hopefully not though. > > Jonathan > > > --- > > drivers/iio/industrialio-core.c | 23 ++++++++++++++++++----- > > 1 file changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > > index 419d6f8acc13..ca8b11541477 100644 > > --- a/drivers/iio/industrialio-core.c > > +++ b/drivers/iio/industrialio-core.c > > @@ -1763,6 +1763,15 @@ static const struct file_operations iio_buffer_fileops = { > > .compat_ioctl = compat_ptr_ioctl, > > }; > > > > +static const struct file_operations iio_event_fileops = { > > + .owner = THIS_MODULE, > > + .llseek = noop_llseek, > > + .unlocked_ioctl = iio_ioctl, > > + .compat_ioctl = compat_ptr_ioctl, > > + .open = iio_chrdev_open, > > + .release = iio_chrdev_release, > > +}; > > + > > static int iio_check_unique_scan_index(struct iio_dev *indio_dev) > > { > > int i, j; > > @@ -1790,6 +1799,7 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops; > > > > int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > > { > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > > int ret; > > > > if (!indio_dev->info) > > @@ -1807,9 +1817,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > > if (ret < 0) > > return ret; > > > > - /* configure elements for the chrdev */ > > - indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id); > > - > > iio_device_register_debugfs(indio_dev); > > > > ret = iio_buffer_alloc_sysfs_and_mask(indio_dev); > > @@ -1838,9 +1845,15 @@ 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 if (iio_dev_opaque->event_interface) > > + cdev_init(&indio_dev->chrdev, &iio_event_fileops); > > > > - indio_dev->chrdev.owner = this_mod; > > + if (indio_dev->buffer || iio_dev_opaque->event_interface) { > > + indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id); > > + indio_dev->chrdev.owner = this_mod; > > + } > > > > ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev); > > if (ret < 0) >
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 419d6f8acc13..ca8b11541477 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1763,6 +1763,15 @@ static const struct file_operations iio_buffer_fileops = { .compat_ioctl = compat_ptr_ioctl, }; +static const struct file_operations iio_event_fileops = { + .owner = THIS_MODULE, + .llseek = noop_llseek, + .unlocked_ioctl = iio_ioctl, + .compat_ioctl = compat_ptr_ioctl, + .open = iio_chrdev_open, + .release = iio_chrdev_release, +}; + static int iio_check_unique_scan_index(struct iio_dev *indio_dev) { int i, j; @@ -1790,6 +1799,7 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops; int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) { + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); int ret; if (!indio_dev->info) @@ -1807,9 +1817,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) if (ret < 0) return ret; - /* configure elements for the chrdev */ - indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id); - iio_device_register_debugfs(indio_dev); ret = iio_buffer_alloc_sysfs_and_mask(indio_dev); @@ -1838,9 +1845,15 @@ 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 if (iio_dev_opaque->event_interface) + cdev_init(&indio_dev->chrdev, &iio_event_fileops); - indio_dev->chrdev.owner = this_mod; + if (indio_dev->buffer || iio_dev_opaque->event_interface) { + indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id); + indio_dev->chrdev.owner = this_mod; + } ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev); if (ret < 0)
We only need a chardev if we need to support buffers and/or events. With this change, a chardev will be created only if an IIO buffer is attached OR an event_interface is configured. Otherwise, no chardev will be created, and the IIO device will get registered with the 'device_add()' call. Quite a lot of IIO devices don't really need a chardev, so this is a minor improvement to the IIO core, as the IIO device will take up (slightly) fewer resources. In order to not create a chardev, we mostly just need to not initialize the indio_dev->dev.devt field. If that is un-initialized, cdev_device_add() behaves like device_add(). Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/industrialio-core.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)