| Message ID | 20200414083656.7696-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: core: register chardev only if needed | expand |
On Tue, 14 Apr 2020 11:36:56 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > The final 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 only need a chardev if we need to support buffers and/or events. > > With this change, a chardev will be created: > 1. if there is an IIO buffer attached OR > 2. if there is an event_interface configured > > Otherwise, no chardev will be created. > 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 fewer > resources. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > --- > > Changelog v1 -> v2: > * split away from series 'iio: core,buffer: re-organize chardev creation'; > i'm getting the feeling that this has some value on it's own; > no idea if it needs 'Fixes' tag; it is a bit fuzzy to point to a patch > which this would be fixed by this; i'm guessing it would be fine > without one I'd argue it's an 'optimization' rather than a fix :) Still looks good to me but I'd like it to sit for a little while to see if anyone points out something we are both missing! Thanks for tidying this up. Jonathan > > drivers/iio/industrialio-core.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index f4daf19f2a3b..32e72d9fd1e9 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1676,6 +1676,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; > @@ -1726,7 +1735,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 if (indio_dev->event_interface) > + cdev_init(&indio_dev->chrdev, &iio_event_fileops); > > indio_dev->chrdev.owner = this_mod; > > @@ -1754,7 +1766,8 @@ EXPORT_SYMBOL(__iio_device_register); > **/ > void iio_device_unregister(struct iio_dev *indio_dev) > { > - cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); > + if (indio_dev->buffer || indio_dev->event_interface) > + cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); > > mutex_lock(&indio_dev->info_exist_lock); >
On Tue, 2020-04-14 at 19:06 +0100, Jonathan Cameron wrote: > [External] > > On Tue, 14 Apr 2020 11:36:56 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > The final 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 only need a chardev if we need to support buffers and/or events. > > > > With this change, a chardev will be created: > > 1. if there is an IIO buffer attached OR > > 2. if there is an event_interface configured > > > > Otherwise, no chardev will be created. > > 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 fewer > > resources. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > --- > > > > Changelog v1 -> v2: > > * split away from series 'iio: core,buffer: re-organize chardev creation'; > > i'm getting the feeling that this has some value on it's own; > > no idea if it needs 'Fixes' tag; it is a bit fuzzy to point to a patch > > which this would be fixed by this; i'm guessing it would be fine > > without one > > I'd argue it's an 'optimization' rather than a fix :) > > Still looks good to me but I'd like it to sit for a little while to > see if anyone points out something we are both missing! > This is not good. It seems that I did not properly test all cases. I had to break a device to not have an event_interface to notice that the sysfs doesn't get instantiated either because device_add is missing. Will do another try. > Thanks for tidying this up. > > Jonathan > > > drivers/iio/industrialio-core.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio- > > core.c > > index f4daf19f2a3b..32e72d9fd1e9 100644 > > --- a/drivers/iio/industrialio-core.c > > +++ b/drivers/iio/industrialio-core.c > > @@ -1676,6 +1676,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; > > @@ -1726,7 +1735,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 if (indio_dev->event_interface) > > + cdev_init(&indio_dev->chrdev, &iio_event_fileops); > > > > indio_dev->chrdev.owner = this_mod; > > > > @@ -1754,7 +1766,8 @@ EXPORT_SYMBOL(__iio_device_register); > > **/ > > void iio_device_unregister(struct iio_dev *indio_dev) > > { > > - cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); > > + if (indio_dev->buffer || indio_dev->event_interface) > > + cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); > > > > mutex_lock(&indio_dev->info_exist_lock); > >
On 4/15/20 3:56 PM, Ardelean, Alexandru wrote: > On Tue, 2020-04-14 at 19:06 +0100, Jonathan Cameron wrote: >> [External] >> >> On Tue, 14 Apr 2020 11:36:56 +0300 >> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: >> >>> The final 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 only need a chardev if we need to support buffers and/or events. >>> >>> With this change, a chardev will be created: >>> 1. if there is an IIO buffer attached OR >>> 2. if there is an event_interface configured >>> >>> Otherwise, no chardev will be created. >>> 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 fewer >>> resources. >>> >>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> >>> --- >>> >>> Changelog v1 -> v2: >>> * split away from series 'iio: core,buffer: re-organize chardev creation'; >>> i'm getting the feeling that this has some value on it's own; >>> no idea if it needs 'Fixes' tag; it is a bit fuzzy to point to a patch >>> which this would be fixed by this; i'm guessing it would be fine >>> without one >> I'd argue it's an 'optimization' rather than a fix :) >> >> Still looks good to me but I'd like it to sit for a little while to >> see if anyone points out something we are both missing! >> > This is not good. > It seems that I did not properly test all cases. > I had to break a device to not have an event_interface to notice that the sysfs > doesn't get instantiated either because device_add is missing. > > Will do another try. I think you also have to make the `indio_dev->dev.devt = ...` conditional. Or conditionally use device_add() instead of device_add_cdev(). If you go for the former you need to call cdev_device_del() unconditionally, for the latter call device_del() or cdev_device_del() depending on whether the cdev was registered. - Lars
On Wed, 2020-04-15 at 16:55 +0200, Lars-Peter Clausen wrote: > [External] > > On 4/15/20 3:56 PM, Ardelean, Alexandru wrote: > > On Tue, 2020-04-14 at 19:06 +0100, Jonathan Cameron wrote: > > > [External] > > > > > > On Tue, 14 Apr 2020 11:36:56 +0300 > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > The final 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 only need a chardev if we need to support buffers and/or events. > > > > > > > > With this change, a chardev will be created: > > > > 1. if there is an IIO buffer attached OR > > > > 2. if there is an event_interface configured > > > > > > > > Otherwise, no chardev will be created. > > > > 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 fewer > > > > resources. > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > --- > > > > > > > > Changelog v1 -> v2: > > > > * split away from series 'iio: core,buffer: re-organize chardev > > > > creation'; > > > > i'm getting the feeling that this has some value on it's own; > > > > no idea if it needs 'Fixes' tag; it is a bit fuzzy to point to a > > > > patch > > > > which this would be fixed by this; i'm guessing it would be fine > > > > without one > > > I'd argue it's an 'optimization' rather than a fix :) > > > > > > Still looks good to me but I'd like it to sit for a little while to > > > see if anyone points out something we are both missing! > > > > > This is not good. > > It seems that I did not properly test all cases. > > I had to break a device to not have an event_interface to notice that the > > sysfs > > doesn't get instantiated either because device_add is missing. > > > > Will do another try. > > I think you also have to make the `indio_dev->dev.devt = ...` > conditional. Or conditionally use device_add() instead of device_add_cdev(). > > If you go for the former you need to call cdev_device_del() > unconditionally, for the latter call device_del() or cdev_device_del() > depending on whether the cdev was registered. I was thinking of conditionally using cdev_device_add/del() somehow. But, this complicates the rest of the series a bit. And I am thinking of how to simplify it. Anyway, this will go back to the series ¯\_(ツ)_/¯ > > - Lars > >
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index f4daf19f2a3b..32e72d9fd1e9 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1676,6 +1676,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; @@ -1726,7 +1735,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 if (indio_dev->event_interface) + cdev_init(&indio_dev->chrdev, &iio_event_fileops); indio_dev->chrdev.owner = this_mod; @@ -1754,7 +1766,8 @@ EXPORT_SYMBOL(__iio_device_register); **/ void iio_device_unregister(struct iio_dev *indio_dev) { - cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); + if (indio_dev->buffer || indio_dev->event_interface) + cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); mutex_lock(&indio_dev->info_exist_lock);
The final 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 only need a chardev if we need to support buffers and/or events. With this change, a chardev will be created: 1. if there is an IIO buffer attached OR 2. if there is an event_interface configured Otherwise, no chardev will be created. 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 fewer resources. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- Changelog v1 -> v2: * split away from series 'iio: core,buffer: re-organize chardev creation'; i'm getting the feeling that this has some value on it's own; no idea if it needs 'Fixes' tag; it is a bit fuzzy to point to a patch which this would be fixed by this; i'm guessing it would be fine without one drivers/iio/industrialio-core.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)