[PATCH v2] iio: core: register chardev only if needed

[PATCH v2] iio: core: register chardev only if needed

[Alexandru Ardelean]

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>
---

Changelog v1 -> v2:
* https://lore.kernel.org/linux-iio/20201117162340.43924-2-alexandru.ardelean@analog.com/
* split away from series; I don't know when I will have time to re-visit
  the entire original series, so might as well move forward a few patches

 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 c2e4c267c36b..d4de32d878aa 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1725,6 +1725,15 @@ static const struct file_operations iio_buffer_fileops = {
 	.release = iio_chrdev_release,
 };
 
+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;
@@ -1752,6 +1761,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)
@@ -1769,9 +1779,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);
@@ -1800,9 +1807,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)
-- 
2.27.0

Re: [PATCH v2] iio: core: register chardev only if needed

[Andy Shevchenko]

On Thu, Dec 3, 2020 at 11:55 AM 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()

un-initialized -> uninitialized

> behaves like device_add().

Are you sure there is no user space application that doesn't rely on
character device to be always present?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: core: register chardev only if needed

[Alexandru Ardelean]

On Wed, Dec 9, 2020 at 5:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Dec 3, 2020 at 11:55 AM 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()
>
> un-initialized -> uninitialized
>
> > behaves like device_add().
>
> Are you sure there is no user space application that doesn't rely on
> character device to be always present?

Nope.
I'm not sure.
I'm also not completely sure how Jonathan feels about this patch being
added now [so late].

Though, technically if the chardev was already there, without all the
control in place [to enable IIO buffers and other stuff through the
chardev] then it's technically just a "marker" file.
Which arguably is a lot to have (i.e. chardev that should be unusable).

If it is usable with no control in place for buffers or other stuff
(i.e. I missed something), then I also don't know.

So, this patch on it's own can still be interpreted as an RFC.
See:
https://lore.kernel.org/linux-iio/20201121180246.772ad299@archlinux/

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] iio: core: register chardev only if needed

[Andy Shevchenko]

On Wed, Dec 9, 2020 at 5:45 PM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
> On Wed, Dec 9, 2020 at 5:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Dec 3, 2020 at 11:55 AM Alexandru Ardelean
> > <alexandru.ardelean@analog.com> wrote:

...

> > Are you sure there is no user space application that doesn't rely on
> > character device to be always present?
>
> Nope.
> I'm not sure.
> I'm also not completely sure how Jonathan feels about this patch being
> added now [so late].
>
> Though, technically if the chardev was already there, without all the
> control in place [to enable IIO buffers and other stuff through the
> chardev] then it's technically just a "marker" file.
> Which arguably is a lot to have (i.e. chardev that should be unusable).
>
> If it is usable with no control in place for buffers or other stuff
> (i.e. I missed something), then I also don't know.
>
> So, this patch on it's own can still be interpreted as an RFC.
> See:
> https://lore.kernel.org/linux-iio/20201121180246.772ad299@archlinux/

Don't take me wrong, I'm not against a good change (I doesn't like
dangling files), but it might break some use cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: core: register chardev only if needed

[Alexandru Ardelean]

On Wed, Dec 9, 2020 at 5:53 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Dec 9, 2020 at 5:45 PM Alexandru Ardelean
> <ardeleanalex@gmail.com> wrote:
> > On Wed, Dec 9, 2020 at 5:37 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Dec 3, 2020 at 11:55 AM Alexandru Ardelean
> > > <alexandru.ardelean@analog.com> wrote:
>
> ...
>
> > > Are you sure there is no user space application that doesn't rely on
> > > character device to be always present?
> >
> > Nope.
> > I'm not sure.
> > I'm also not completely sure how Jonathan feels about this patch being
> > added now [so late].
> >
> > Though, technically if the chardev was already there, without all the
> > control in place [to enable IIO buffers and other stuff through the
> > chardev] then it's technically just a "marker" file.
> > Which arguably is a lot to have (i.e. chardev that should be unusable).
> >
> > If it is usable with no control in place for buffers or other stuff
> > (i.e. I missed something), then I also don't know.
> >
> > So, this patch on it's own can still be interpreted as an RFC.
> > See:
> > https://lore.kernel.org/linux-iio/20201121180246.772ad299@archlinux/
>
> Don't take me wrong, I'm not against a good change (I doesn't like
> dangling files), but it might break some use cases.

Yeah I know.
But how else do you know if a dangling file might break some use cases?

The worst that would happen is that we get a report and create a Fixes
tag and we know.
But if we don't try it, we're stuck with it, and will never know.

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] iio: core: register chardev only if needed

[Jonathan Cameron]

On Wed, 9 Dec 2020 17:55:22 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Wed, Dec 9, 2020 at 5:53 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Wed, Dec 9, 2020 at 5:45 PM Alexandru Ardelean
> > <ardeleanalex@gmail.com> wrote:  
> > > On Wed, Dec 9, 2020 at 5:37 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:  
> > > > On Thu, Dec 3, 2020 at 11:55 AM Alexandru Ardelean
> > > > <alexandru.ardelean@analog.com> wrote:  
> >
> > ...
> >  
> > > > Are you sure there is no user space application that doesn't rely on
> > > > character device to be always present?  
> > >
> > > Nope.
> > > I'm not sure.
> > > I'm also not completely sure how Jonathan feels about this patch being
> > > added now [so late].
> > >
> > > Though, technically if the chardev was already there, without all the
> > > control in place [to enable IIO buffers and other stuff through the
> > > chardev] then it's technically just a "marker" file.
> > > Which arguably is a lot to have (i.e. chardev that should be unusable).
> > >
> > > If it is usable with no control in place for buffers or other stuff
> > > (i.e. I missed something), then I also don't know.
> > >
> > > So, this patch on it's own can still be interpreted as an RFC.
> > > See:
> > > https://lore.kernel.org/linux-iio/20201121180246.772ad299@archlinux/  
> >
> > Don't take me wrong, I'm not against a good change (I doesn't like
> > dangling files), but it might break some use cases.  
> 
> Yeah I know.
> But how else do you know if a dangling file might break some use cases?
> 
> The worst that would happen is that we get a report and create a Fixes
> tag and we know.
> But if we don't try it, we're stuck with it, and will never know.
> 
It's definitely a high risk change.  I'd 'hope' it's not a problem
but we should do a bit more due diligence. 

I hope we can assume the ADI software is all fine with dropping this.
Bastien can you see any issues with dropping chrdev for IIO devices
that don't actually support using it for anything (sysfs interface only).

What other stacks are people aware of that we should enquire about?

Thanks,

Jonathan
> >
> > --
> > With Best Regards,
> > Andy Shevchenko  

Re: [PATCH v2] iio: core: register chardev only if needed

[Alexandru Ardelean]

On Sun, Dec 13, 2020 at 4:31 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 9 Dec 2020 17:55:22 +0200
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
>
> > On Wed, Dec 9, 2020 at 5:53 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Wed, Dec 9, 2020 at 5:45 PM Alexandru Ardelean
> > > <ardeleanalex@gmail.com> wrote:
> > > > On Wed, Dec 9, 2020 at 5:37 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Thu, Dec 3, 2020 at 11:55 AM Alexandru Ardelean
> > > > > <alexandru.ardelean@analog.com> wrote:
> > >
> > > ...
> > >
> > > > > Are you sure there is no user space application that doesn't rely on
> > > > > character device to be always present?
> > > >
> > > > Nope.
> > > > I'm not sure.
> > > > I'm also not completely sure how Jonathan feels about this patch being
> > > > added now [so late].
> > > >
> > > > Though, technically if the chardev was already there, without all the
> > > > control in place [to enable IIO buffers and other stuff through the
> > > > chardev] then it's technically just a "marker" file.
> > > > Which arguably is a lot to have (i.e. chardev that should be unusable).
> > > >
> > > > If it is usable with no control in place for buffers or other stuff
> > > > (i.e. I missed something), then I also don't know.
> > > >
> > > > So, this patch on it's own can still be interpreted as an RFC.
> > > > See:
> > > > https://lore.kernel.org/linux-iio/20201121180246.772ad299@archlinux/
> > >
> > > Don't take me wrong, I'm not against a good change (I doesn't like
> > > dangling files), but it might break some use cases.
> >
> > Yeah I know.
> > But how else do you know if a dangling file might break some use cases?
> >
> > The worst that would happen is that we get a report and create a Fixes
> > tag and we know.
> > But if we don't try it, we're stuck with it, and will never know.
> >
> It's definitely a high risk change.  I'd 'hope' it's not a problem
> but we should do a bit more due diligence.
>
> I hope we can assume the ADI software is all fine with dropping this.
> Bastien can you see any issues with dropping chrdev for IIO devices
> that don't actually support using it for anything (sysfs interface only).
>
> What other stacks are people aware of that we should enquire about?

Hey,

Any more thoughts on this?

Thanks
Alex

>
> Thanks,
>
> Jonathan
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
>

[PATCH v2] iio: core: register chardev only if needed

[Alexandru Ardelean]

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(-)

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);
 
-- 
2.17.1

Re: [PATCH v2] iio: core: register chardev only if needed

[Jonathan Cameron]

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);
>  

Re: [PATCH v2] iio: core: register chardev only if needed

[Ardelean, Alexandru]

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);
> >  

Re: [PATCH v2] iio: core: register chardev only if needed

[Lars-Peter Clausen]

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

Re: [PATCH v2] iio: core: register chardev only if needed

[Ardelean, Alexandru]

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
> 
>