[PATCH v3 0/5] iio: core,buffer: re-organize chardev creation

[PATCH v3 0/5] iio: core,buffer: re-organize chardev creation

[Alexandru Ardelean]

The main intent is to be able to add more chardevs per IIO device, one
for each buffer. To get there, some rework is needed, and one important
facet is to move the creation of the current chardev into
'industrialio-buffer.c', so that control/logic of these chardevs is
localized in that file.

This changeset does that [incrementally] by moving the common chardev
creation from 'industrialio-core.c' to 'industrialio-buffer.c' &
'industrialio-event.c'.
The common chardev is required for both IIO buffers & IIO events.
In order to make this work, the 'iio_device_event_ioctl()' needs to be
passed from 'industrialio-event.c' to 'industrialio-buffer.c' flying past
'industrialio-core.c'. This sounds a bit wrong [at first] but it has the
effect of reducing inter-dependencies between 'industrialio-core.c' to
'industrialio-buffer.c' quite a bit.
The IIO buffer also has a CONFIG_IIO_BUFFER symbol which can turn it
off. No idea how widely this is used [as disabled], but this changeset
also takes that into consideration.

So, now the logic [for __iio_device_register() with regard to chardev
init] is:
1. iio_device_buffers_init() will init buffer and the chardev, if that
   works, the 'iio_device_event_ioctl()' will be attached to the chardev
2. if CONFIG_IIO_BUFFER is not defined or 'indio_dev->buffer == NULL'
   (no buffter attached), -ENOTSUPP should be returned from
   iio_device_buffers_init(), in which case the chardev should be
   initialized in  'industrialio-event.c' via
   'iio_device_register_event_chrdev()'

One neat side effect of this logic, is that we can also move the buffer
sysfs alloc/cleanup into 'industrialio-buffer.c' under the new
'iio_device_buffers_{un}init()' functions.

Changelog v2 -> v3:
* removed double init in
  'iio: event: move event-only chardev in industrialio-event.c'

Changelog v1 -> v2:
* re-reviewed some exit-paths and cleanup some potential leaks on those
  exit paths:
  - for 'iio: buffer: move iio buffer chrdev in industrialio-buffer.c'
    add iio_device_buffers_put() helper and calling iio_buffers_uninit()
    on device un-regsiter
  - for 'move sysfs alloc/free in industrialio-buffer.c'
    call 'iio_buffer_free_sysfs_and_mask()' on exit path if
    cdev_device_add() fails
  - for 'move event-only chardev in industrialio-event.c'
    check if event_interface is NULL in
    iio_device_unregister_event_chrdev()

Alexandru Ardelean (5):
  iio: core: register buffer fileops only if buffer present
  iio: buffer: add back-ref from iio_buffer to iio_dev
  iio: buffer: move iio buffer chrdev in industrialio-buffer.c
  iio: buffer: move sysfs alloc/free in industrialio-buffer.c
  iio: event: move event-only chardev in industrialio-event.c

 drivers/iio/iio_core.h            |  30 +++----
 drivers/iio/industrialio-buffer.c | 144 ++++++++++++++++++++++++++----
 drivers/iio/industrialio-core.c   | 107 +++-------------------
 drivers/iio/industrialio-event.c  | 122 ++++++++++++++++++++++++-
 include/linux/iio/buffer_impl.h   |  10 +++
 include/linux/iio/iio.h           |   4 -
 6 files changed, 285 insertions(+), 132 deletions(-)

-- 
2.17.1

[PATCH v3 1/5] iio: core: register buffer fileops only if buffer present

[Alexandru Ardelean]

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

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

Re: [PATCH v3 1/5] iio: core: register buffer fileops only if buffer present

[Jonathan Cameron]

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

Re: [PATCH v3 1/5] iio: core: register buffer fileops only if buffer present

[Ardelean, Alexandru]

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

Re: [PATCH v3 1/5] iio: core: register buffer fileops only if buffer present

[Ardelean, Alexandru]

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

[PATCH v3 2/5] iio: buffer: add back-ref from iio_buffer to iio_dev

[Alexandru Ardelean]

An IIO device will have multiple buffers, but it shouldn't be allowed that
an IIO buffer should belong to more than 1 IIO device.

Once things get moved more from IIO device to the IIO buffer, and an IIO
device will be able to have more than 1 buffer attached, there will be a
need for a back-ref to the IIO device [from the IIO buffer].

This change adds that.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 2 ++
 include/linux/iio/buffer_impl.h   | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index e6fa1a4e135d..f9ffc7762f6c 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1478,5 +1478,7 @@ void iio_device_attach_buffer(struct iio_dev *indio_dev,
 			      struct iio_buffer *buffer)
 {
 	indio_dev->buffer = iio_buffer_get(buffer);
+
+	indio_dev->buffer->indio_dev = indio_dev;
 }
 EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 1e7edf6bed96..8fb92250a190 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -69,6 +69,9 @@ struct iio_buffer_access_funcs {
  * those writing new buffer implementations.
  */
 struct iio_buffer {
+	/** @indio_dev: IIO device to which this buffer belongs to. */
+	struct iio_dev *indio_dev;
+
 	/** @length: Number of datums in buffer. */
 	unsigned int length;
 
-- 
2.17.1

Re: [PATCH v3 2/5] iio: buffer: add back-ref from iio_buffer to iio_dev

[Jonathan Cameron]

On Fri, 10 Apr 2020 17:17:26 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> An IIO device will have multiple buffers, but it shouldn't be allowed that
> an IIO buffer should belong to more than 1 IIO device.
> 
> Once things get moved more from IIO device to the IIO buffer, and an IIO
> device will be able to have more than 1 buffer attached, there will be a
> need for a back-ref to the IIO device [from the IIO buffer].
> 
> This change adds that.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Much as I hate circular references, I can't see a way around this one...

> ---
>  drivers/iio/industrialio-buffer.c | 2 ++
>  include/linux/iio/buffer_impl.h   | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index e6fa1a4e135d..f9ffc7762f6c 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1478,5 +1478,7 @@ void iio_device_attach_buffer(struct iio_dev *indio_dev,
>  			      struct iio_buffer *buffer)
>  {
>  	indio_dev->buffer = iio_buffer_get(buffer);
> +
> +	indio_dev->buffer->indio_dev = indio_dev;
>  }
>  EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 1e7edf6bed96..8fb92250a190 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -69,6 +69,9 @@ struct iio_buffer_access_funcs {
>   * those writing new buffer implementations.
>   */
>  struct iio_buffer {
> +	/** @indio_dev: IIO device to which this buffer belongs to. */
> +	struct iio_dev *indio_dev;
> +
>  	/** @length: Number of datums in buffer. */
>  	unsigned int length;
>  

[PATCH v3 3/5] iio: buffer: move iio buffer chrdev in industrialio-buffer.c

[Alexandru Ardelean]

This change makes the first jump to move the buffer chardev from indio_dev
to the buffer attached to indio_dev (i.e. indio_dev->buffer).

This requires that some functions that are shared between
'industrialio-core.c' & industrialio-buffer.c be re-shuffled.
The 'iio_buffer_fileops' is now moved to 'industrialio-buffer.c'. It's also
more appropriate to have it here, but the old 'legacy' iio_ioctl() must be
passed to it.

What happens now is:
* if IS_ENABLED(CONFIG_IIO_BUFFER) and 'indio_dev->buffer != NULL' the
  indio_dev->buffer->chrdev will be initialized (as the old legacy style
  chardev)
* if -ENOTSUPP is returned for either of the conditions above (not being
  met), the chardev will be created directly via 'indio_dev->chrdev', same
  as before; a new field is required now 'indio_dev->chrdev_initialized' to
  mark it true, so that 'indio_dev->chrdev' gets deleted if initialized;

'indio_dev->chrdev_initialized' is of type 'int', because recently
checkpatch complains that 'bool' types on structs can cause alignment
issues.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/iio_core.h            |  25 ++++---
 drivers/iio/industrialio-buffer.c | 119 ++++++++++++++++++++++++++++--
 drivers/iio/industrialio-core.c   |  59 ++++++++-------
 include/linux/iio/buffer_impl.h   |   7 ++
 include/linux/iio/iio.h           |   2 +
 5 files changed, 170 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index fd9a5f1d5e51..4bdadeac2710 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -40,24 +40,31 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
 #ifdef CONFIG_IIO_BUFFER
 struct poll_table_struct;
 
-__poll_t iio_buffer_poll(struct file *filp,
-			     struct poll_table_struct *wait);
-ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
-			      size_t n, loff_t *f_ps);
+long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
+			    unsigned int cmd, unsigned long arg);
+
+int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod);
+void iio_device_buffers_uninit(struct iio_dev *indio_dev);
+
+void iio_device_buffers_put(struct iio_dev *indio_dev);
 
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
 void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
 
-#define iio_buffer_poll_addr (&iio_buffer_poll)
-#define iio_buffer_read_outer_addr (&iio_buffer_read_outer)
-
 void iio_disable_all_buffers(struct iio_dev *indio_dev);
 void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
 
 #else
 
-#define iio_buffer_poll_addr NULL
-#define iio_buffer_read_outer_addr NULL
+static inline int iio_device_buffers_init(struct iio_dev *indio_dev,
+					  struct module *this_mod)
+{
+	return -ENOTSUPP;
+}
+
+static inline void iio_device_buffers_uninit(struct iio_dev *indio_dev) {}
+
+static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
 
 static inline 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 f9ffc7762f6c..4b5c3baadaab 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -99,11 +99,11 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
  * Return: negative values corresponding to error codes or ret != 0
  *	   for ending the reading activity
  **/
-ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
-			      size_t n, loff_t *f_ps)
+static ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
+				     size_t n, loff_t *f_ps)
 {
-	struct iio_dev *indio_dev = filp->private_data;
-	struct iio_buffer *rb = indio_dev->buffer;
+	struct iio_buffer *rb = filp->private_data;
+	struct iio_dev *indio_dev = rb->indio_dev;
 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	size_t datum_size;
 	size_t to_wait;
@@ -165,11 +165,11 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
  * Return: (EPOLLIN | EPOLLRDNORM) if data is available for reading
  *	   or 0 for other cases
  */
-__poll_t iio_buffer_poll(struct file *filp,
+static __poll_t iio_buffer_poll(struct file *filp,
 			     struct poll_table_struct *wait)
 {
-	struct iio_dev *indio_dev = filp->private_data;
-	struct iio_buffer *rb = indio_dev->buffer;
+	struct iio_buffer *rb = filp->private_data;
+	struct iio_dev *indio_dev = rb->indio_dev;
 
 	if (!indio_dev->info || rb == NULL)
 		return 0;
@@ -180,6 +180,48 @@ __poll_t iio_buffer_poll(struct file *filp,
 	return 0;
 }
 
+/**
+ * iio_buffer_chrdev_open() - chrdev file open for buffer access
+ * @inode:	Inode structure for identifying the device in the file system
+ * @filp:	File structure for iio device used to keep and later access
+ *		private data
+ *
+ * Return: 0 on success or -EBUSY if the device is already opened
+ **/
+static int iio_buffer_chrdev_open(struct inode *inode, struct file *filp)
+{
+	struct iio_buffer *buffer = container_of(inode->i_cdev,
+						 struct iio_buffer, chrdev);
+
+	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags))
+		return -EBUSY;
+
+	iio_buffer_get(buffer);
+
+	filp->private_data = buffer;
+
+	return 0;
+}
+
+/**
+ * iio_buffer_chrdev_release() - chrdev file close for buffer access
+ * @inode:	Inode structure pointer for the char device
+ * @filp:	File structure pointer for the char device
+ *
+ * Return: 0 for successful release
+ */
+static int iio_buffer_chrdev_release(struct inode *inode, struct file *filp)
+{
+	struct iio_buffer *buffer = container_of(inode->i_cdev,
+						 struct iio_buffer, chrdev);
+
+	clear_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags);
+
+	iio_buffer_put(buffer);
+
+	return 0;
+}
+
 /**
  * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
  * @indio_dev: The IIO device
@@ -1121,6 +1163,14 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
 	iio_buffer_deactivate_all(indio_dev);
 }
 
+long iio_buffer_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+	struct iio_buffer *buffer = filep->private_data;
+	struct iio_dev *indio_dev = buffer->indio_dev;
+
+	return iio_device_event_ioctl(indio_dev, filep, cmd, arg);
+}
+
 static ssize_t iio_buffer_store_enable(struct device *dev,
 				       struct device_attribute *attr,
 				       const char *buf,
@@ -1356,6 +1406,61 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
 }
 
+static const struct file_operations iio_buffer_fileops = {
+	.read = iio_buffer_read_outer,
+	.release = iio_buffer_chrdev_release,
+	.open = iio_buffer_chrdev_open,
+	.poll = iio_buffer_poll,
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.unlocked_ioctl = iio_buffer_ioctl,
+	.compat_ioctl = compat_ptr_ioctl,
+};
+
+int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod)
+{
+	struct iio_buffer *buffer = indio_dev->buffer;
+	int ret;
+
+	if (!buffer)
+		return -ENOTSUPP;
+
+	cdev_init(&buffer->chrdev, &iio_buffer_fileops);
+
+	buffer->chrdev.owner = this_mod;
+
+	ret = cdev_device_add(&buffer->chrdev, &indio_dev->dev);
+	if (ret)
+		return ret;
+
+	iio_device_get(indio_dev);
+	iio_buffer_get(buffer);
+
+	return 0;
+}
+
+void iio_device_buffers_put(struct iio_dev *indio_dev)
+{
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	if (!buffer)
+		return;
+
+	iio_buffer_put(buffer);
+}
+
+void iio_device_buffers_uninit(struct iio_dev *indio_dev)
+{
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	if (!buffer)
+		return;
+
+	cdev_device_del(&buffer->chrdev, &indio_dev->dev);
+	iio_buffer_put(buffer);
+	iio_device_put(indio_dev);
+}
+
 /**
  * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
  * @indio_dev: the iio device
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c8c074602709..2158aeab0bd2 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1475,7 +1475,7 @@ static void iio_dev_release(struct device *device)
 	iio_device_unregister_eventset(indio_dev);
 	iio_device_unregister_sysfs(indio_dev);
 
-	iio_buffer_put(indio_dev->buffer);
+	iio_device_buffers_put(indio_dev);
 
 	ida_simple_remove(&iio_ida, indio_dev->id);
 	kfree(indio_dev);
@@ -1610,7 +1610,7 @@ void devm_iio_device_free(struct device *dev, struct iio_dev *iio_dev)
 EXPORT_SYMBOL_GPL(devm_iio_device_free);
 
 /**
- * iio_chrdev_open() - chrdev file open for buffer access and ioctls
+ * iio_chrdev_open() - chrdev file open for event ioctls
  * @inode:	Inode structure for identifying the device in the file system
  * @filp:	File structure for iio device used to keep and later access
  *		private data
@@ -1633,7 +1633,7 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
 }
 
 /**
- * iio_chrdev_release() - chrdev file close buffer access and ioctls
+ * iio_chrdev_release() - chrdev file close for event ioctls
  * @inode:	Inode structure pointer for the char device
  * @filp:	File structure pointer for the char device
  *
@@ -1649,11 +1649,9 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
 	return 0;
 }
 
-/* Somewhat of a cross file organization violation - ioctls here are actually
- * event related */
-static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
+			    unsigned int cmd, unsigned long arg)
 {
-	struct iio_dev *indio_dev = filp->private_data;
 	int __user *ip = (int __user *)arg;
 	int fd;
 
@@ -1671,16 +1669,15 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	return -EINVAL;
 }
 
-static const struct file_operations iio_buffer_fileops = {
-	.read = iio_buffer_read_outer_addr,
-	.release = iio_chrdev_release,
-	.open = iio_chrdev_open,
-	.poll = iio_buffer_poll_addr,
-	.owner = THIS_MODULE,
-	.llseek = noop_llseek,
-	.unlocked_ioctl = iio_ioctl,
-	.compat_ioctl = compat_ptr_ioctl,
-};
+/* Somewhat of a cross file organization violation - ioctls here are actually
+ * event related */
+static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
+				    unsigned long arg)
+{
+	struct iio_dev *indio_dev = filp->private_data;
+
+	return iio_device_event_ioctl(indio_dev, filp, cmd, arg);
+}
 
 static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
 {
@@ -1712,7 +1709,7 @@ static const struct file_operations iio_event_fileops = {
 	.open = iio_chrdev_open,
 	.owner = THIS_MODULE,
 	.llseek = noop_llseek,
-	.unlocked_ioctl = iio_ioctl,
+	.unlocked_ioctl = iio_event_ioctl_wrapper,
 	.compat_ioctl = compat_ptr_ioctl,
 };
 
@@ -1766,16 +1763,21 @@ 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;
 
-	if (indio_dev->buffer)
-		cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
-	else
+	ret = iio_device_buffers_init(indio_dev, this_mod);
+	if (ret) {
+		if (ret != -ENOTSUPP)
+			goto error_unreg_eventset;
+
 		cdev_init(&indio_dev->chrdev, &iio_event_fileops);
 
-	indio_dev->chrdev.owner = this_mod;
+		indio_dev->chrdev.owner = this_mod;
 
-	ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
-	if (ret < 0)
-		goto error_unreg_eventset;
+		ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
+		if (ret < 0)
+			goto error_unreg_eventset;
+
+		indio_dev->chrdev_initialized = true;
+	}
 
 	return 0;
 
@@ -1797,7 +1799,12 @@ 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->chrdev_initialized)
+		cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
+
+	indio_dev->chrdev_initialized = false;
+
+	iio_device_buffers_uninit(indio_dev);
 
 	mutex_lock(&indio_dev->info_exist_lock);
 
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 8fb92250a190..b4a55c3f556b 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #ifndef _IIO_BUFFER_GENERIC_IMPL_H_
 #define _IIO_BUFFER_GENERIC_IMPL_H_
+#include <linux/cdev.h>
 #include <linux/sysfs.h>
 #include <linux/kref.h>
 
@@ -72,6 +73,12 @@ struct iio_buffer {
 	/** @indio_dev: IIO device to which this buffer belongs to. */
 	struct iio_dev *indio_dev;
 
+	/** @chrdev: associated character device. */
+	struct cdev chrdev;
+
+	/** @file_ops_flags: file ops related flags including busy flag. */
+	unsigned long file_ops_flags;
+
 	/** @length: Number of datums in buffer. */
 	unsigned int length;
 
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index e975020abaa6..e93497f483f7 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -517,6 +517,7 @@ struct iio_buffer_setup_ops {
  * @setup_ops:		[DRIVER] callbacks to call before and after buffer
  *			enable/disable
  * @chrdev:		[INTERN] associated character device
+ * @chrdev_initialized:	[INTERN] true if @chrdev device has been initialized
  * @groups:		[INTERN] attribute groups
  * @groupcounter:	[INTERN] index of next attribute group
  * @flags:		[INTERN] file ops related flags including busy flag.
@@ -560,6 +561,7 @@ struct iio_dev {
 	struct mutex			info_exist_lock;
 	const struct iio_buffer_setup_ops	*setup_ops;
 	struct cdev			chrdev;
+	int				chrdev_initialized;
 #define IIO_MAX_GROUPS 6
 	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
 	int				groupcounter;
-- 
2.17.1

Re: [PATCH v3 3/5] iio: buffer: move iio buffer chrdev in industrialio-buffer.c

[Jonathan Cameron]

On Fri, 10 Apr 2020 17:17:27 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change makes the first jump to move the buffer chardev from indio_dev
> to the buffer attached to indio_dev (i.e. indio_dev->buffer).
> 
> This requires that some functions that are shared between
> 'industrialio-core.c' & industrialio-buffer.c be re-shuffled.
> The 'iio_buffer_fileops' is now moved to 'industrialio-buffer.c'. It's also
> more appropriate to have it here, but the old 'legacy' iio_ioctl() must be
> passed to it.
> 
> What happens now is:
> * if IS_ENABLED(CONFIG_IIO_BUFFER) and 'indio_dev->buffer != NULL' the
>   indio_dev->buffer->chrdev will be initialized (as the old legacy style
>   chardev)
> * if -ENOTSUPP is returned for either of the conditions above (not being
>   met), the chardev will be created directly via 'indio_dev->chrdev', same
>   as before; a new field is required now 'indio_dev->chrdev_initialized' to
>   mark it true, so that 'indio_dev->chrdev' gets deleted if initialized;
> 
> 'indio_dev->chrdev_initialized' is of type 'int', because recently
> checkpatch complains that 'bool' types on structs can cause alignment
> issues.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

I wonder if it would be possible to make the ioctl mess a bit cleaner.
Have the buffer ioctl function as here, but have that call a generic
iio_device_ioctl function in industrialio-core.c which can in theory
handle 'device wide' ioctls.  Then have the events code register into
that as a consumer of a particular device wide ioctl.

That way the layering is a violated a little less.

What do you think?

Jonathan

> ---
>  drivers/iio/iio_core.h            |  25 ++++---
>  drivers/iio/industrialio-buffer.c | 119 ++++++++++++++++++++++++++++--
>  drivers/iio/industrialio-core.c   |  59 ++++++++-------
>  include/linux/iio/buffer_impl.h   |   7 ++
>  include/linux/iio/iio.h           |   2 +
>  5 files changed, 170 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index fd9a5f1d5e51..4bdadeac2710 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -40,24 +40,31 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
>  #ifdef CONFIG_IIO_BUFFER
>  struct poll_table_struct;
>  
> -__poll_t iio_buffer_poll(struct file *filp,
> -			     struct poll_table_struct *wait);
> -ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> -			      size_t n, loff_t *f_ps);
> +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> +			    unsigned int cmd, unsigned long arg);
> +
> +int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod);
> +void iio_device_buffers_uninit(struct iio_dev *indio_dev);
> +
> +void iio_device_buffers_put(struct iio_dev *indio_dev);
>  
>  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
>  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
>  
> -#define iio_buffer_poll_addr (&iio_buffer_poll)
> -#define iio_buffer_read_outer_addr (&iio_buffer_read_outer)
> -
>  void iio_disable_all_buffers(struct iio_dev *indio_dev);
>  void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
>  
>  #else
>  
> -#define iio_buffer_poll_addr NULL
> -#define iio_buffer_read_outer_addr NULL
> +static inline int iio_device_buffers_init(struct iio_dev *indio_dev,
> +					  struct module *this_mod)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline void iio_device_buffers_uninit(struct iio_dev *indio_dev) {}
> +
> +static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
>  
>  static inline 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 f9ffc7762f6c..4b5c3baadaab 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -99,11 +99,11 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
>   * Return: negative values corresponding to error codes or ret != 0
>   *	   for ending the reading activity
>   **/
> -ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> -			      size_t n, loff_t *f_ps)
> +static ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> +				     size_t n, loff_t *f_ps)
>  {
> -	struct iio_dev *indio_dev = filp->private_data;
> -	struct iio_buffer *rb = indio_dev->buffer;
> +	struct iio_buffer *rb = filp->private_data;
> +	struct iio_dev *indio_dev = rb->indio_dev;
>  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>  	size_t datum_size;
>  	size_t to_wait;
> @@ -165,11 +165,11 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
>   * Return: (EPOLLIN | EPOLLRDNORM) if data is available for reading
>   *	   or 0 for other cases
>   */
> -__poll_t iio_buffer_poll(struct file *filp,
> +static __poll_t iio_buffer_poll(struct file *filp,
>  			     struct poll_table_struct *wait)
>  {
> -	struct iio_dev *indio_dev = filp->private_data;
> -	struct iio_buffer *rb = indio_dev->buffer;
> +	struct iio_buffer *rb = filp->private_data;
> +	struct iio_dev *indio_dev = rb->indio_dev;
>  
>  	if (!indio_dev->info || rb == NULL)
>  		return 0;
> @@ -180,6 +180,48 @@ __poll_t iio_buffer_poll(struct file *filp,
>  	return 0;
>  }
>  
> +/**
> + * iio_buffer_chrdev_open() - chrdev file open for buffer access
> + * @inode:	Inode structure for identifying the device in the file system
> + * @filp:	File structure for iio device used to keep and later access
> + *		private data
> + *
> + * Return: 0 on success or -EBUSY if the device is already opened
> + **/
> +static int iio_buffer_chrdev_open(struct inode *inode, struct file *filp)
> +{
> +	struct iio_buffer *buffer = container_of(inode->i_cdev,
> +						 struct iio_buffer, chrdev);
> +
> +	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags))
> +		return -EBUSY;
> +
> +	iio_buffer_get(buffer);
> +
> +	filp->private_data = buffer;
> +
> +	return 0;
> +}
> +
> +/**
> + * iio_buffer_chrdev_release() - chrdev file close for buffer access
> + * @inode:	Inode structure pointer for the char device
> + * @filp:	File structure pointer for the char device
> + *
> + * Return: 0 for successful release
> + */
> +static int iio_buffer_chrdev_release(struct inode *inode, struct file *filp)
> +{
> +	struct iio_buffer *buffer = container_of(inode->i_cdev,
> +						 struct iio_buffer, chrdev);
> +
> +	clear_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags);
> +
> +	iio_buffer_put(buffer);
> +
> +	return 0;
> +}
> +
>  /**
>   * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
>   * @indio_dev: The IIO device
> @@ -1121,6 +1163,14 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
>  	iio_buffer_deactivate_all(indio_dev);
>  }
>  
> +long iio_buffer_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> +{
> +	struct iio_buffer *buffer = filep->private_data;
> +	struct iio_dev *indio_dev = buffer->indio_dev;
> +
> +	return iio_device_event_ioctl(indio_dev, filep, cmd, arg);
> +}
> +
>  static ssize_t iio_buffer_store_enable(struct device *dev,
>  				       struct device_attribute *attr,
>  				       const char *buf,
> @@ -1356,6 +1406,61 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
>  }
>  
> +static const struct file_operations iio_buffer_fileops = {
> +	.read = iio_buffer_read_outer,
> +	.release = iio_buffer_chrdev_release,
> +	.open = iio_buffer_chrdev_open,
> +	.poll = iio_buffer_poll,
> +	.owner = THIS_MODULE,
> +	.llseek = noop_llseek,
> +	.unlocked_ioctl = iio_buffer_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
> +};
> +
> +int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod)
> +{
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	int ret;
> +
> +	if (!buffer)
> +		return -ENOTSUPP;
> +
> +	cdev_init(&buffer->chrdev, &iio_buffer_fileops);
> +
> +	buffer->chrdev.owner = this_mod;
> +
> +	ret = cdev_device_add(&buffer->chrdev, &indio_dev->dev);
> +	if (ret)
> +		return ret;
> +
> +	iio_device_get(indio_dev);
> +	iio_buffer_get(buffer);
> +
> +	return 0;
> +}
> +
> +void iio_device_buffers_put(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +
> +	if (!buffer)
> +		return;
> +
> +	iio_buffer_put(buffer);
> +}
> +
> +void iio_device_buffers_uninit(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +
> +	if (!buffer)
> +		return;
> +
> +	cdev_device_del(&buffer->chrdev, &indio_dev->dev);
> +	iio_buffer_put(buffer);
> +	iio_device_put(indio_dev);
> +}
> +
>  /**
>   * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
>   * @indio_dev: the iio device
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c8c074602709..2158aeab0bd2 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1475,7 +1475,7 @@ static void iio_dev_release(struct device *device)
>  	iio_device_unregister_eventset(indio_dev);
>  	iio_device_unregister_sysfs(indio_dev);
>  
> -	iio_buffer_put(indio_dev->buffer);
> +	iio_device_buffers_put(indio_dev);
>  
>  	ida_simple_remove(&iio_ida, indio_dev->id);
>  	kfree(indio_dev);
> @@ -1610,7 +1610,7 @@ void devm_iio_device_free(struct device *dev, struct iio_dev *iio_dev)
>  EXPORT_SYMBOL_GPL(devm_iio_device_free);
>  
>  /**
> - * iio_chrdev_open() - chrdev file open for buffer access and ioctls
> + * iio_chrdev_open() - chrdev file open for event ioctls
>   * @inode:	Inode structure for identifying the device in the file system
>   * @filp:	File structure for iio device used to keep and later access
>   *		private data
> @@ -1633,7 +1633,7 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
>  }
>  
>  /**
> - * iio_chrdev_release() - chrdev file close buffer access and ioctls
> + * iio_chrdev_release() - chrdev file close for event ioctls
>   * @inode:	Inode structure pointer for the char device
>   * @filp:	File structure pointer for the char device
>   *
> @@ -1649,11 +1649,9 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
>  	return 0;
>  }
>  
> -/* Somewhat of a cross file organization violation - ioctls here are actually
> - * event related */
> -static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> +			    unsigned int cmd, unsigned long arg)
>  {
> -	struct iio_dev *indio_dev = filp->private_data;
>  	int __user *ip = (int __user *)arg;
>  	int fd;
>  
> @@ -1671,16 +1669,15 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	return -EINVAL;
>  }
>  
> -static const struct file_operations iio_buffer_fileops = {
> -	.read = iio_buffer_read_outer_addr,
> -	.release = iio_chrdev_release,
> -	.open = iio_chrdev_open,
> -	.poll = iio_buffer_poll_addr,
> -	.owner = THIS_MODULE,
> -	.llseek = noop_llseek,
> -	.unlocked_ioctl = iio_ioctl,
> -	.compat_ioctl = compat_ptr_ioctl,
> -};
> +/* Somewhat of a cross file organization violation - ioctls here are actually
> + * event related */
> +static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> +				    unsigned long arg)
> +{
> +	struct iio_dev *indio_dev = filp->private_data;
> +

Hmm. I wonder if we keep a level of indirection in here.  So actually call
something like: 'iio_device_ioctl' in industrialio-core.c and have the
event registration register a callback with the core.  Thus no events and
nothing is registered so that core function simply returns an error
- right now I suspect it returns a chardev that is completely useless
(if no events supported by the device).

> +	return iio_device_event_ioctl(indio_dev, filp, cmd, arg);
> +}
>  
>  static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
>  {
> @@ -1712,7 +1709,7 @@ static const struct file_operations iio_event_fileops = {
>  	.open = iio_chrdev_open,
>  	.owner = THIS_MODULE,
>  	.llseek = noop_llseek,
> -	.unlocked_ioctl = iio_ioctl,
> +	.unlocked_ioctl = iio_event_ioctl_wrapper,
>  	.compat_ioctl = compat_ptr_ioctl,
>  };
>  
> @@ -1766,16 +1763,21 @@ 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;
>  
> -	if (indio_dev->buffer)
> -		cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> -	else
> +	ret = iio_device_buffers_init(indio_dev, this_mod);
> +	if (ret) {
> +		if (ret != -ENOTSUPP)
> +			goto error_unreg_eventset;
> +
>  		cdev_init(&indio_dev->chrdev, &iio_event_fileops);
>  
> -	indio_dev->chrdev.owner = this_mod;
> +		indio_dev->chrdev.owner = this_mod;
>  
> -	ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> -	if (ret < 0)
> -		goto error_unreg_eventset;
> +		ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> +		if (ret < 0)
> +			goto error_unreg_eventset;
> +
> +		indio_dev->chrdev_initialized = true;
> +	}
>  
>  	return 0;
>  
> @@ -1797,7 +1799,12 @@ 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->chrdev_initialized)
> +		cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> +
> +	indio_dev->chrdev_initialized = false;
> +
> +	iio_device_buffers_uninit(indio_dev);
>  
>  	mutex_lock(&indio_dev->info_exist_lock);
>  
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 8fb92250a190..b4a55c3f556b 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  #ifndef _IIO_BUFFER_GENERIC_IMPL_H_
>  #define _IIO_BUFFER_GENERIC_IMPL_H_
> +#include <linux/cdev.h>
>  #include <linux/sysfs.h>
>  #include <linux/kref.h>
>  
> @@ -72,6 +73,12 @@ struct iio_buffer {
>  	/** @indio_dev: IIO device to which this buffer belongs to. */
>  	struct iio_dev *indio_dev;
>  
> +	/** @chrdev: associated character device. */
> +	struct cdev chrdev;
> +
> +	/** @file_ops_flags: file ops related flags including busy flag. */
> +	unsigned long file_ops_flags;
> +
>  	/** @length: Number of datums in buffer. */
>  	unsigned int length;
>  
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index e975020abaa6..e93497f483f7 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -517,6 +517,7 @@ struct iio_buffer_setup_ops {
>   * @setup_ops:		[DRIVER] callbacks to call before and after buffer
>   *			enable/disable
>   * @chrdev:		[INTERN] associated character device
> + * @chrdev_initialized:	[INTERN] true if @chrdev device has been initialized
>   * @groups:		[INTERN] attribute groups
>   * @groupcounter:	[INTERN] index of next attribute group
>   * @flags:		[INTERN] file ops related flags including busy flag.
> @@ -560,6 +561,7 @@ struct iio_dev {
>  	struct mutex			info_exist_lock;
>  	const struct iio_buffer_setup_ops	*setup_ops;
>  	struct cdev			chrdev;
> +	int				chrdev_initialized;
>  #define IIO_MAX_GROUPS 6
>  	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
>  	int				groupcounter;

Re: [PATCH v3 3/5] iio: buffer: move iio buffer chrdev in industrialio-buffer.c

[Ardelean, Alexandru]

On Mon, 2020-04-13 at 16:58 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Fri, 10 Apr 2020 17:17:27 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This change makes the first jump to move the buffer chardev from indio_dev
> > to the buffer attached to indio_dev (i.e. indio_dev->buffer).
> > 
> > This requires that some functions that are shared between
> > 'industrialio-core.c' & industrialio-buffer.c be re-shuffled.
> > The 'iio_buffer_fileops' is now moved to 'industrialio-buffer.c'. It's also
> > more appropriate to have it here, but the old 'legacy' iio_ioctl() must be
> > passed to it.
> > 
> > What happens now is:
> > * if IS_ENABLED(CONFIG_IIO_BUFFER) and 'indio_dev->buffer != NULL' the
> >   indio_dev->buffer->chrdev will be initialized (as the old legacy style
> >   chardev)
> > * if -ENOTSUPP is returned for either of the conditions above (not being
> >   met), the chardev will be created directly via 'indio_dev->chrdev', same
> >   as before; a new field is required now 'indio_dev->chrdev_initialized' to
> >   mark it true, so that 'indio_dev->chrdev' gets deleted if initialized;
> > 
> > 'indio_dev->chrdev_initialized' is of type 'int', because recently
> > checkpatch complains that 'bool' types on structs can cause alignment
> > issues.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> I wonder if it would be possible to make the ioctl mess a bit cleaner.
> Have the buffer ioctl function as here, but have that call a generic
> iio_device_ioctl function in industrialio-core.c which can in theory
> handle 'device wide' ioctls.  Then have the events code register into
> that as a consumer of a particular device wide ioctl.
> 
> That way the layering is a violated a little less.
> 
> What do you think?

I like it.
Will create a patch for that.

I'll see if I can make it neat into this series somehow.
Otherwise, would it be ok to do it on top of this series?
I currently don't have a clear idea of how clean the patchset would look.


> 
> Jonathan
> 
> > ---
> >  drivers/iio/iio_core.h            |  25 ++++---
> >  drivers/iio/industrialio-buffer.c | 119 ++++++++++++++++++++++++++++--
> >  drivers/iio/industrialio-core.c   |  59 ++++++++-------
> >  include/linux/iio/buffer_impl.h   |   7 ++
> >  include/linux/iio/iio.h           |   2 +
> >  5 files changed, 170 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index fd9a5f1d5e51..4bdadeac2710 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -40,24 +40,31 @@ ssize_t iio_format_value(char *buf, unsigned int type,
> > int size, int *vals);
> >  #ifdef CONFIG_IIO_BUFFER
> >  struct poll_table_struct;
> >  
> > -__poll_t iio_buffer_poll(struct file *filp,
> > -			     struct poll_table_struct *wait);
> > -ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> > -			      size_t n, loff_t *f_ps);
> > +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > +			    unsigned int cmd, unsigned long arg);
> > +
> > +int iio_device_buffers_init(struct iio_dev *indio_dev, struct module
> > *this_mod);
> > +void iio_device_buffers_uninit(struct iio_dev *indio_dev);
> > +
> > +void iio_device_buffers_put(struct iio_dev *indio_dev);
> >  
> >  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> >  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
> >  
> > -#define iio_buffer_poll_addr (&iio_buffer_poll)
> > -#define iio_buffer_read_outer_addr (&iio_buffer_read_outer)
> > -
> >  void iio_disable_all_buffers(struct iio_dev *indio_dev);
> >  void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> >  
> >  #else
> >  
> > -#define iio_buffer_poll_addr NULL
> > -#define iio_buffer_read_outer_addr NULL
> > +static inline int iio_device_buffers_init(struct iio_dev *indio_dev,
> > +					  struct module *this_mod)
> > +{
> > +	return -ENOTSUPP;
> > +}
> > +
> > +static inline void iio_device_buffers_uninit(struct iio_dev *indio_dev) {}
> > +
> > +static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
> >  
> >  static inline 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 f9ffc7762f6c..4b5c3baadaab 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -99,11 +99,11 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev,
> > struct iio_buffer *buf,
> >   * Return: negative values corresponding to error codes or ret != 0
> >   *	   for ending the reading activity
> >   **/
> > -ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> > -			      size_t n, loff_t *f_ps)
> > +static ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> > +				     size_t n, loff_t *f_ps)
> >  {
> > -	struct iio_dev *indio_dev = filp->private_data;
> > -	struct iio_buffer *rb = indio_dev->buffer;
> > +	struct iio_buffer *rb = filp->private_data;
> > +	struct iio_dev *indio_dev = rb->indio_dev;
> >  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> >  	size_t datum_size;
> >  	size_t to_wait;
> > @@ -165,11 +165,11 @@ ssize_t iio_buffer_read_outer(struct file *filp, char
> > __user *buf,
> >   * Return: (EPOLLIN | EPOLLRDNORM) if data is available for reading
> >   *	   or 0 for other cases
> >   */
> > -__poll_t iio_buffer_poll(struct file *filp,
> > +static __poll_t iio_buffer_poll(struct file *filp,
> >  			     struct poll_table_struct *wait)
> >  {
> > -	struct iio_dev *indio_dev = filp->private_data;
> > -	struct iio_buffer *rb = indio_dev->buffer;
> > +	struct iio_buffer *rb = filp->private_data;
> > +	struct iio_dev *indio_dev = rb->indio_dev;
> >  
> >  	if (!indio_dev->info || rb == NULL)
> >  		return 0;
> > @@ -180,6 +180,48 @@ __poll_t iio_buffer_poll(struct file *filp,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * iio_buffer_chrdev_open() - chrdev file open for buffer access
> > + * @inode:	Inode structure for identifying the device in the file system
> > + * @filp:	File structure for iio device used to keep and later access
> > + *		private data
> > + *
> > + * Return: 0 on success or -EBUSY if the device is already opened
> > + **/
> > +static int iio_buffer_chrdev_open(struct inode *inode, struct file *filp)
> > +{
> > +	struct iio_buffer *buffer = container_of(inode->i_cdev,
> > +						 struct iio_buffer, chrdev);
> > +
> > +	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags))
> > +		return -EBUSY;
> > +
> > +	iio_buffer_get(buffer);
> > +
> > +	filp->private_data = buffer;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * iio_buffer_chrdev_release() - chrdev file close for buffer access
> > + * @inode:	Inode structure pointer for the char device
> > + * @filp:	File structure pointer for the char device
> > + *
> > + * Return: 0 for successful release
> > + */
> > +static int iio_buffer_chrdev_release(struct inode *inode, struct file
> > *filp)
> > +{
> > +	struct iio_buffer *buffer = container_of(inode->i_cdev,
> > +						 struct iio_buffer, chrdev);
> > +
> > +	clear_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags);
> > +
> > +	iio_buffer_put(buffer);
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
> >   * @indio_dev: The IIO device
> > @@ -1121,6 +1163,14 @@ void iio_disable_all_buffers(struct iio_dev
> > *indio_dev)
> >  	iio_buffer_deactivate_all(indio_dev);
> >  }
> >  
> > +long iio_buffer_ioctl(struct file *filep, unsigned int cmd, unsigned long
> > arg)
> > +{
> > +	struct iio_buffer *buffer = filep->private_data;
> > +	struct iio_dev *indio_dev = buffer->indio_dev;
> > +
> > +	return iio_device_event_ioctl(indio_dev, filep, cmd, arg);
> > +}
> > +
> >  static ssize_t iio_buffer_store_enable(struct device *dev,
> >  				       struct device_attribute *attr,
> >  				       const char *buf,
> > @@ -1356,6 +1406,61 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> >  	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
> >  }
> >  
> > +static const struct file_operations iio_buffer_fileops = {
> > +	.read = iio_buffer_read_outer,
> > +	.release = iio_buffer_chrdev_release,
> > +	.open = iio_buffer_chrdev_open,
> > +	.poll = iio_buffer_poll,
> > +	.owner = THIS_MODULE,
> > +	.llseek = noop_llseek,
> > +	.unlocked_ioctl = iio_buffer_ioctl,
> > +	.compat_ioctl = compat_ptr_ioctl,
> > +};
> > +
> > +int iio_device_buffers_init(struct iio_dev *indio_dev, struct module
> > *this_mod)
> > +{
> > +	struct iio_buffer *buffer = indio_dev->buffer;
> > +	int ret;
> > +
> > +	if (!buffer)
> > +		return -ENOTSUPP;
> > +
> > +	cdev_init(&buffer->chrdev, &iio_buffer_fileops);
> > +
> > +	buffer->chrdev.owner = this_mod;
> > +
> > +	ret = cdev_device_add(&buffer->chrdev, &indio_dev->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	iio_device_get(indio_dev);
> > +	iio_buffer_get(buffer);
> > +
> > +	return 0;
> > +}
> > +
> > +void iio_device_buffers_put(struct iio_dev *indio_dev)
> > +{
> > +	struct iio_buffer *buffer = indio_dev->buffer;
> > +
> > +	if (!buffer)
> > +		return;
> > +
> > +	iio_buffer_put(buffer);
> > +}
> > +
> > +void iio_device_buffers_uninit(struct iio_dev *indio_dev)
> > +{
> > +	struct iio_buffer *buffer = indio_dev->buffer;
> > +
> > +	if (!buffer)
> > +		return;
> > +
> > +	cdev_device_del(&buffer->chrdev, &indio_dev->dev);
> > +	iio_buffer_put(buffer);
> > +	iio_device_put(indio_dev);
> > +}
> > +
> >  /**
> >   * iio_validate_scan_mask_onehot() - Validates that exactly one channel is
> > selected
> >   * @indio_dev: the iio device
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index c8c074602709..2158aeab0bd2 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1475,7 +1475,7 @@ static void iio_dev_release(struct device *device)
> >  	iio_device_unregister_eventset(indio_dev);
> >  	iio_device_unregister_sysfs(indio_dev);
> >  
> > -	iio_buffer_put(indio_dev->buffer);
> > +	iio_device_buffers_put(indio_dev);
> >  
> >  	ida_simple_remove(&iio_ida, indio_dev->id);
> >  	kfree(indio_dev);
> > @@ -1610,7 +1610,7 @@ void devm_iio_device_free(struct device *dev, struct
> > iio_dev *iio_dev)
> >  EXPORT_SYMBOL_GPL(devm_iio_device_free);
> >  
> >  /**
> > - * iio_chrdev_open() - chrdev file open for buffer access and ioctls
> > + * iio_chrdev_open() - chrdev file open for event ioctls
> >   * @inode:	Inode structure for identifying the device in the file system
> >   * @filp:	File structure for iio device used to keep and later access
> >   *		private data
> > @@ -1633,7 +1633,7 @@ static int iio_chrdev_open(struct inode *inode, struct
> > file *filp)
> >  }
> >  
> >  /**
> > - * iio_chrdev_release() - chrdev file close buffer access and ioctls
> > + * iio_chrdev_release() - chrdev file close for event ioctls
> >   * @inode:	Inode structure pointer for the char device
> >   * @filp:	File structure pointer for the char device
> >   *
> > @@ -1649,11 +1649,9 @@ static int iio_chrdev_release(struct inode *inode,
> > struct file *filp)
> >  	return 0;
> >  }
> >  
> > -/* Somewhat of a cross file organization violation - ioctls here are
> > actually
> > - * event related */
> > -static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long
> > arg)
> > +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > +			    unsigned int cmd, unsigned long arg)
> >  {
> > -	struct iio_dev *indio_dev = filp->private_data;
> >  	int __user *ip = (int __user *)arg;
> >  	int fd;
> >  
> > @@ -1671,16 +1669,15 @@ static long iio_ioctl(struct file *filp, unsigned
> > int cmd, unsigned long arg)
> >  	return -EINVAL;
> >  }
> >  
> > -static const struct file_operations iio_buffer_fileops = {
> > -	.read = iio_buffer_read_outer_addr,
> > -	.release = iio_chrdev_release,
> > -	.open = iio_chrdev_open,
> > -	.poll = iio_buffer_poll_addr,
> > -	.owner = THIS_MODULE,
> > -	.llseek = noop_llseek,
> > -	.unlocked_ioctl = iio_ioctl,
> > -	.compat_ioctl = compat_ptr_ioctl,
> > -};
> > +/* Somewhat of a cross file organization violation - ioctls here are
> > actually
> > + * event related */
> > +static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> > +				    unsigned long arg)
> > +{
> > +	struct iio_dev *indio_dev = filp->private_data;
> > +
> 
> Hmm. I wonder if we keep a level of indirection in here.  So actually call
> something like: 'iio_device_ioctl' in industrialio-core.c and have the
> event registration register a callback with the core.  Thus no events and
> nothing is registered so that core function simply returns an error
> - right now I suspect it returns a chardev that is completely useless
> (if no events supported by the device).
> 
> > +	return iio_device_event_ioctl(indio_dev, filp, cmd, arg);
> > +}
> >  
> >  static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
> >  {
> > @@ -1712,7 +1709,7 @@ static const struct file_operations iio_event_fileops
> > = {
> >  	.open = iio_chrdev_open,
> >  	.owner = THIS_MODULE,
> >  	.llseek = noop_llseek,
> > -	.unlocked_ioctl = iio_ioctl,
> > +	.unlocked_ioctl = iio_event_ioctl_wrapper,
> >  	.compat_ioctl = compat_ptr_ioctl,
> >  };
> >  
> > @@ -1766,16 +1763,21 @@ 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;
> >  
> > -	if (indio_dev->buffer)
> > -		cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> > -	else
> > +	ret = iio_device_buffers_init(indio_dev, this_mod);
> > +	if (ret) {
> > +		if (ret != -ENOTSUPP)
> > +			goto error_unreg_eventset;
> > +
> >  		cdev_init(&indio_dev->chrdev, &iio_event_fileops);
> >  
> > -	indio_dev->chrdev.owner = this_mod;
> > +		indio_dev->chrdev.owner = this_mod;
> >  
> > -	ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> > -	if (ret < 0)
> > -		goto error_unreg_eventset;
> > +		ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> > +		if (ret < 0)
> > +			goto error_unreg_eventset;
> > +
> > +		indio_dev->chrdev_initialized = true;
> > +	}
> >  
> >  	return 0;
> >  
> > @@ -1797,7 +1799,12 @@ 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->chrdev_initialized)
> > +		cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> > +
> > +	indio_dev->chrdev_initialized = false;
> > +
> > +	iio_device_buffers_uninit(indio_dev);
> >  
> >  	mutex_lock(&indio_dev->info_exist_lock);
> >  
> > diff --git a/include/linux/iio/buffer_impl.h
> > b/include/linux/iio/buffer_impl.h
> > index 8fb92250a190..b4a55c3f556b 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -1,6 +1,7 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> >  #ifndef _IIO_BUFFER_GENERIC_IMPL_H_
> >  #define _IIO_BUFFER_GENERIC_IMPL_H_
> > +#include <linux/cdev.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/kref.h>
> >  
> > @@ -72,6 +73,12 @@ struct iio_buffer {
> >  	/** @indio_dev: IIO device to which this buffer belongs to. */
> >  	struct iio_dev *indio_dev;
> >  
> > +	/** @chrdev: associated character device. */
> > +	struct cdev chrdev;
> > +
> > +	/** @file_ops_flags: file ops related flags including busy flag. */
> > +	unsigned long file_ops_flags;
> > +
> >  	/** @length: Number of datums in buffer. */
> >  	unsigned int length;
> >  
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index e975020abaa6..e93497f483f7 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -517,6 +517,7 @@ struct iio_buffer_setup_ops {
> >   * @setup_ops:		[DRIVER] callbacks to call before and after
> > buffer
> >   *			enable/disable
> >   * @chrdev:		[INTERN] associated character device
> > + * @chrdev_initialized:	[INTERN] true if @chrdev device has been
> > initialized
> >   * @groups:		[INTERN] attribute groups
> >   * @groupcounter:	[INTERN] index of next attribute group
> >   * @flags:		[INTERN] file ops related flags including busy flag.
> > @@ -560,6 +561,7 @@ struct iio_dev {
> >  	struct mutex			info_exist_lock;
> >  	const struct iio_buffer_setup_ops	*setup_ops;
> >  	struct cdev			chrdev;
> > +	int				chrdev_initialized;
> >  #define IIO_MAX_GROUPS 6
> >  	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
> >  	int				groupcounter;

Re: [PATCH v3 3/5] iio: buffer: move iio buffer chrdev in industrialio-buffer.c

[Jonathan Cameron]

On Tue, 14 Apr 2020 05:43:40 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Mon, 2020-04-13 at 16:58 +0100, Jonathan Cameron wrote:
> > [External]
> > 
> > On Fri, 10 Apr 2020 17:17:27 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >   
> > > This change makes the first jump to move the buffer chardev from indio_dev
> > > to the buffer attached to indio_dev (i.e. indio_dev->buffer).
> > > 
> > > This requires that some functions that are shared between
> > > 'industrialio-core.c' & industrialio-buffer.c be re-shuffled.
> > > The 'iio_buffer_fileops' is now moved to 'industrialio-buffer.c'. It's also
> > > more appropriate to have it here, but the old 'legacy' iio_ioctl() must be
> > > passed to it.
> > > 
> > > What happens now is:
> > > * if IS_ENABLED(CONFIG_IIO_BUFFER) and 'indio_dev->buffer != NULL' the
> > >   indio_dev->buffer->chrdev will be initialized (as the old legacy style
> > >   chardev)
> > > * if -ENOTSUPP is returned for either of the conditions above (not being
> > >   met), the chardev will be created directly via 'indio_dev->chrdev', same
> > >   as before; a new field is required now 'indio_dev->chrdev_initialized' to
> > >   mark it true, so that 'indio_dev->chrdev' gets deleted if initialized;
> > > 
> > > 'indio_dev->chrdev_initialized' is of type 'int', because recently
> > > checkpatch complains that 'bool' types on structs can cause alignment
> > > issues.
> > > 
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > 
> > I wonder if it would be possible to make the ioctl mess a bit cleaner.
> > Have the buffer ioctl function as here, but have that call a generic
> > iio_device_ioctl function in industrialio-core.c which can in theory
> > handle 'device wide' ioctls.  Then have the events code register into
> > that as a consumer of a particular device wide ioctl.
> > 
> > That way the layering is a violated a little less.
> > 
> > What do you think?  
> 
> I like it.
> Will create a patch for that.
> 
> I'll see if I can make it neat into this series somehow.
> Otherwise, would it be ok to do it on top of this series?
> I currently don't have a clear idea of how clean the patchset would look.

Ideally as part of this series.  I don't 'think' it should be too bad
but if it proves horrible I'll cope with it as patches on top of this series.

Jonathan

> 
> 
> > 
> > Jonathan
> >   
> > > ---
> > >  drivers/iio/iio_core.h            |  25 ++++---
> > >  drivers/iio/industrialio-buffer.c | 119 ++++++++++++++++++++++++++++--
> > >  drivers/iio/industrialio-core.c   |  59 ++++++++-------
> > >  include/linux/iio/buffer_impl.h   |   7 ++
> > >  include/linux/iio/iio.h           |   2 +
> > >  5 files changed, 170 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > index fd9a5f1d5e51..4bdadeac2710 100644
> > > --- a/drivers/iio/iio_core.h
> > > +++ b/drivers/iio/iio_core.h
> > > @@ -40,24 +40,31 @@ ssize_t iio_format_value(char *buf, unsigned int type,
> > > int size, int *vals);
> > >  #ifdef CONFIG_IIO_BUFFER
> > >  struct poll_table_struct;
> > >  
> > > -__poll_t iio_buffer_poll(struct file *filp,
> > > -			     struct poll_table_struct *wait);
> > > -ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> > > -			      size_t n, loff_t *f_ps);
> > > +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > > +			    unsigned int cmd, unsigned long arg);
> > > +
> > > +int iio_device_buffers_init(struct iio_dev *indio_dev, struct module
> > > *this_mod);
> > > +void iio_device_buffers_uninit(struct iio_dev *indio_dev);
> > > +
> > > +void iio_device_buffers_put(struct iio_dev *indio_dev);
> > >  
> > >  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > >  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
> > >  
> > > -#define iio_buffer_poll_addr (&iio_buffer_poll)
> > > -#define iio_buffer_read_outer_addr (&iio_buffer_read_outer)
> > > -
> > >  void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > >  void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> > >  
> > >  #else
> > >  
> > > -#define iio_buffer_poll_addr NULL
> > > -#define iio_buffer_read_outer_addr NULL
> > > +static inline int iio_device_buffers_init(struct iio_dev *indio_dev,
> > > +					  struct module *this_mod)
> > > +{
> > > +	return -ENOTSUPP;
> > > +}
> > > +
> > > +static inline void iio_device_buffers_uninit(struct iio_dev *indio_dev) {}
> > > +
> > > +static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
> > >  
> > >  static inline 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 f9ffc7762f6c..4b5c3baadaab 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -99,11 +99,11 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev,
> > > struct iio_buffer *buf,
> > >   * Return: negative values corresponding to error codes or ret != 0
> > >   *	   for ending the reading activity
> > >   **/
> > > -ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> > > -			      size_t n, loff_t *f_ps)
> > > +static ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
> > > +				     size_t n, loff_t *f_ps)
> > >  {
> > > -	struct iio_dev *indio_dev = filp->private_data;
> > > -	struct iio_buffer *rb = indio_dev->buffer;
> > > +	struct iio_buffer *rb = filp->private_data;
> > > +	struct iio_dev *indio_dev = rb->indio_dev;
> > >  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > >  	size_t datum_size;
> > >  	size_t to_wait;
> > > @@ -165,11 +165,11 @@ ssize_t iio_buffer_read_outer(struct file *filp, char
> > > __user *buf,
> > >   * Return: (EPOLLIN | EPOLLRDNORM) if data is available for reading
> > >   *	   or 0 for other cases
> > >   */
> > > -__poll_t iio_buffer_poll(struct file *filp,
> > > +static __poll_t iio_buffer_poll(struct file *filp,
> > >  			     struct poll_table_struct *wait)
> > >  {
> > > -	struct iio_dev *indio_dev = filp->private_data;
> > > -	struct iio_buffer *rb = indio_dev->buffer;
> > > +	struct iio_buffer *rb = filp->private_data;
> > > +	struct iio_dev *indio_dev = rb->indio_dev;
> > >  
> > >  	if (!indio_dev->info || rb == NULL)
> > >  		return 0;
> > > @@ -180,6 +180,48 @@ __poll_t iio_buffer_poll(struct file *filp,
> > >  	return 0;
> > >  }
> > >  
> > > +/**
> > > + * iio_buffer_chrdev_open() - chrdev file open for buffer access
> > > + * @inode:	Inode structure for identifying the device in the file system
> > > + * @filp:	File structure for iio device used to keep and later access
> > > + *		private data
> > > + *
> > > + * Return: 0 on success or -EBUSY if the device is already opened
> > > + **/
> > > +static int iio_buffer_chrdev_open(struct inode *inode, struct file *filp)
> > > +{
> > > +	struct iio_buffer *buffer = container_of(inode->i_cdev,
> > > +						 struct iio_buffer, chrdev);
> > > +
> > > +	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags))
> > > +		return -EBUSY;
> > > +
> > > +	iio_buffer_get(buffer);
> > > +
> > > +	filp->private_data = buffer;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * iio_buffer_chrdev_release() - chrdev file close for buffer access
> > > + * @inode:	Inode structure pointer for the char device
> > > + * @filp:	File structure pointer for the char device
> > > + *
> > > + * Return: 0 for successful release
> > > + */
> > > +static int iio_buffer_chrdev_release(struct inode *inode, struct file
> > > *filp)
> > > +{
> > > +	struct iio_buffer *buffer = container_of(inode->i_cdev,
> > > +						 struct iio_buffer, chrdev);
> > > +
> > > +	clear_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags);
> > > +
> > > +	iio_buffer_put(buffer);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
> > >   * @indio_dev: The IIO device
> > > @@ -1121,6 +1163,14 @@ void iio_disable_all_buffers(struct iio_dev
> > > *indio_dev)
> > >  	iio_buffer_deactivate_all(indio_dev);
> > >  }
> > >  
> > > +long iio_buffer_ioctl(struct file *filep, unsigned int cmd, unsigned long
> > > arg)
> > > +{
> > > +	struct iio_buffer *buffer = filep->private_data;
> > > +	struct iio_dev *indio_dev = buffer->indio_dev;
> > > +
> > > +	return iio_device_event_ioctl(indio_dev, filep, cmd, arg);
> > > +}
> > > +
> > >  static ssize_t iio_buffer_store_enable(struct device *dev,
> > >  				       struct device_attribute *attr,
> > >  				       const char *buf,
> > > @@ -1356,6 +1406,61 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > >  	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
> > >  }
> > >  
> > > +static const struct file_operations iio_buffer_fileops = {
> > > +	.read = iio_buffer_read_outer,
> > > +	.release = iio_buffer_chrdev_release,
> > > +	.open = iio_buffer_chrdev_open,
> > > +	.poll = iio_buffer_poll,
> > > +	.owner = THIS_MODULE,
> > > +	.llseek = noop_llseek,
> > > +	.unlocked_ioctl = iio_buffer_ioctl,
> > > +	.compat_ioctl = compat_ptr_ioctl,
> > > +};
> > > +
> > > +int iio_device_buffers_init(struct iio_dev *indio_dev, struct module
> > > *this_mod)
> > > +{
> > > +	struct iio_buffer *buffer = indio_dev->buffer;
> > > +	int ret;
> > > +
> > > +	if (!buffer)
> > > +		return -ENOTSUPP;
> > > +
> > > +	cdev_init(&buffer->chrdev, &iio_buffer_fileops);
> > > +
> > > +	buffer->chrdev.owner = this_mod;
> > > +
> > > +	ret = cdev_device_add(&buffer->chrdev, &indio_dev->dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	iio_device_get(indio_dev);
> > > +	iio_buffer_get(buffer);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void iio_device_buffers_put(struct iio_dev *indio_dev)
> > > +{
> > > +	struct iio_buffer *buffer = indio_dev->buffer;
> > > +
> > > +	if (!buffer)
> > > +		return;
> > > +
> > > +	iio_buffer_put(buffer);
> > > +}
> > > +
> > > +void iio_device_buffers_uninit(struct iio_dev *indio_dev)
> > > +{
> > > +	struct iio_buffer *buffer = indio_dev->buffer;
> > > +
> > > +	if (!buffer)
> > > +		return;
> > > +
> > > +	cdev_device_del(&buffer->chrdev, &indio_dev->dev);
> > > +	iio_buffer_put(buffer);
> > > +	iio_device_put(indio_dev);
> > > +}
> > > +
> > >  /**
> > >   * iio_validate_scan_mask_onehot() - Validates that exactly one channel is
> > > selected
> > >   * @indio_dev: the iio device
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > core.c
> > > index c8c074602709..2158aeab0bd2 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -1475,7 +1475,7 @@ static void iio_dev_release(struct device *device)
> > >  	iio_device_unregister_eventset(indio_dev);
> > >  	iio_device_unregister_sysfs(indio_dev);
> > >  
> > > -	iio_buffer_put(indio_dev->buffer);
> > > +	iio_device_buffers_put(indio_dev);
> > >  
> > >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > >  	kfree(indio_dev);
> > > @@ -1610,7 +1610,7 @@ void devm_iio_device_free(struct device *dev, struct
> > > iio_dev *iio_dev)
> > >  EXPORT_SYMBOL_GPL(devm_iio_device_free);
> > >  
> > >  /**
> > > - * iio_chrdev_open() - chrdev file open for buffer access and ioctls
> > > + * iio_chrdev_open() - chrdev file open for event ioctls
> > >   * @inode:	Inode structure for identifying the device in the file system
> > >   * @filp:	File structure for iio device used to keep and later access
> > >   *		private data
> > > @@ -1633,7 +1633,7 @@ static int iio_chrdev_open(struct inode *inode, struct
> > > file *filp)
> > >  }
> > >  
> > >  /**
> > > - * iio_chrdev_release() - chrdev file close buffer access and ioctls
> > > + * iio_chrdev_release() - chrdev file close for event ioctls
> > >   * @inode:	Inode structure pointer for the char device
> > >   * @filp:	File structure pointer for the char device
> > >   *
> > > @@ -1649,11 +1649,9 @@ static int iio_chrdev_release(struct inode *inode,
> > > struct file *filp)
> > >  	return 0;
> > >  }
> > >  
> > > -/* Somewhat of a cross file organization violation - ioctls here are
> > > actually
> > > - * event related */
> > > -static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long
> > > arg)
> > > +long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > > +			    unsigned int cmd, unsigned long arg)
> > >  {
> > > -	struct iio_dev *indio_dev = filp->private_data;
> > >  	int __user *ip = (int __user *)arg;
> > >  	int fd;
> > >  
> > > @@ -1671,16 +1669,15 @@ static long iio_ioctl(struct file *filp, unsigned
> > > int cmd, unsigned long arg)
> > >  	return -EINVAL;
> > >  }
> > >  
> > > -static const struct file_operations iio_buffer_fileops = {
> > > -	.read = iio_buffer_read_outer_addr,
> > > -	.release = iio_chrdev_release,
> > > -	.open = iio_chrdev_open,
> > > -	.poll = iio_buffer_poll_addr,
> > > -	.owner = THIS_MODULE,
> > > -	.llseek = noop_llseek,
> > > -	.unlocked_ioctl = iio_ioctl,
> > > -	.compat_ioctl = compat_ptr_ioctl,
> > > -};
> > > +/* Somewhat of a cross file organization violation - ioctls here are
> > > actually
> > > + * event related */
> > > +static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> > > +				    unsigned long arg)
> > > +{
> > > +	struct iio_dev *indio_dev = filp->private_data;
> > > +  
> > 
> > Hmm. I wonder if we keep a level of indirection in here.  So actually call
> > something like: 'iio_device_ioctl' in industrialio-core.c and have the
> > event registration register a callback with the core.  Thus no events and
> > nothing is registered so that core function simply returns an error
> > - right now I suspect it returns a chardev that is completely useless
> > (if no events supported by the device).
> >   
> > > +	return iio_device_event_ioctl(indio_dev, filp, cmd, arg);
> > > +}
> > >  
> > >  static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
> > >  {
> > > @@ -1712,7 +1709,7 @@ static const struct file_operations iio_event_fileops
> > > = {
> > >  	.open = iio_chrdev_open,
> > >  	.owner = THIS_MODULE,
> > >  	.llseek = noop_llseek,
> > > -	.unlocked_ioctl = iio_ioctl,
> > > +	.unlocked_ioctl = iio_event_ioctl_wrapper,
> > >  	.compat_ioctl = compat_ptr_ioctl,
> > >  };
> > >  
> > > @@ -1766,16 +1763,21 @@ 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;
> > >  
> > > -	if (indio_dev->buffer)
> > > -		cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> > > -	else
> > > +	ret = iio_device_buffers_init(indio_dev, this_mod);
> > > +	if (ret) {
> > > +		if (ret != -ENOTSUPP)
> > > +			goto error_unreg_eventset;
> > > +
> > >  		cdev_init(&indio_dev->chrdev, &iio_event_fileops);
> > >  
> > > -	indio_dev->chrdev.owner = this_mod;
> > > +		indio_dev->chrdev.owner = this_mod;
> > >  
> > > -	ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> > > -	if (ret < 0)
> > > -		goto error_unreg_eventset;
> > > +		ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> > > +		if (ret < 0)
> > > +			goto error_unreg_eventset;
> > > +
> > > +		indio_dev->chrdev_initialized = true;
> > > +	}
> > >  
> > >  	return 0;
> > >  
> > > @@ -1797,7 +1799,12 @@ 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->chrdev_initialized)
> > > +		cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> > > +
> > > +	indio_dev->chrdev_initialized = false;
> > > +
> > > +	iio_device_buffers_uninit(indio_dev);
> > >  
> > >  	mutex_lock(&indio_dev->info_exist_lock);
> > >  
> > > diff --git a/include/linux/iio/buffer_impl.h
> > > b/include/linux/iio/buffer_impl.h
> > > index 8fb92250a190..b4a55c3f556b 100644
> > > --- a/include/linux/iio/buffer_impl.h
> > > +++ b/include/linux/iio/buffer_impl.h
> > > @@ -1,6 +1,7 @@
> > >  /* SPDX-License-Identifier: GPL-2.0 */
> > >  #ifndef _IIO_BUFFER_GENERIC_IMPL_H_
> > >  #define _IIO_BUFFER_GENERIC_IMPL_H_
> > > +#include <linux/cdev.h>
> > >  #include <linux/sysfs.h>
> > >  #include <linux/kref.h>
> > >  
> > > @@ -72,6 +73,12 @@ struct iio_buffer {
> > >  	/** @indio_dev: IIO device to which this buffer belongs to. */
> > >  	struct iio_dev *indio_dev;
> > >  
> > > +	/** @chrdev: associated character device. */
> > > +	struct cdev chrdev;
> > > +
> > > +	/** @file_ops_flags: file ops related flags including busy flag. */
> > > +	unsigned long file_ops_flags;
> > > +
> > >  	/** @length: Number of datums in buffer. */
> > >  	unsigned int length;
> > >  
> > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > index e975020abaa6..e93497f483f7 100644
> > > --- a/include/linux/iio/iio.h
> > > +++ b/include/linux/iio/iio.h
> > > @@ -517,6 +517,7 @@ struct iio_buffer_setup_ops {
> > >   * @setup_ops:		[DRIVER] callbacks to call before and after
> > > buffer
> > >   *			enable/disable
> > >   * @chrdev:		[INTERN] associated character device
> > > + * @chrdev_initialized:	[INTERN] true if @chrdev device has been
> > > initialized
> > >   * @groups:		[INTERN] attribute groups
> > >   * @groupcounter:	[INTERN] index of next attribute group
> > >   * @flags:		[INTERN] file ops related flags including busy flag.
> > > @@ -560,6 +561,7 @@ struct iio_dev {
> > >  	struct mutex			info_exist_lock;
> > >  	const struct iio_buffer_setup_ops	*setup_ops;
> > >  	struct cdev			chrdev;
> > > +	int				chrdev_initialized;
> > >  #define IIO_MAX_GROUPS 6
> > >  	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
> > >  	int				groupcounter;  

[PATCH v3 4/5] iio: buffer: move sysfs alloc/free in industrialio-buffer.c

[Alexandru Ardelean]

Since we now have the 'iio_device_buffers_{un}init()' entry-points into the
industrialio-buffer.c we can now move the sysfs alloc & free in there as
part of that init/uninit.

This changes the order of iio_device_register()/iio_device_unregister() a
bit, but overall this shouldn't matter.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/iio_core.h            | 10 ----------
 drivers/iio/industrialio-buffer.c | 29 +++++++++++++++--------------
 drivers/iio/industrialio-core.c   | 14 +-------------
 3 files changed, 16 insertions(+), 37 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 4bdadeac2710..1d69071e1426 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -48,9 +48,6 @@ void iio_device_buffers_uninit(struct iio_dev *indio_dev);
 
 void iio_device_buffers_put(struct iio_dev *indio_dev);
 
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
-
 void iio_disable_all_buffers(struct iio_dev *indio_dev);
 void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
 
@@ -66,13 +63,6 @@ static inline void iio_device_buffers_uninit(struct iio_dev *indio_dev) {}
 
 static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
 
-static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
-{
-	return 0;
-}
-
-static inline void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) {}
-
 static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
 static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
 
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 4b5c3baadaab..8ab089a9c3bc 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1284,11 +1284,11 @@ 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;
 
@@ -1301,9 +1301,6 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 		indio_dev->masklength = ml;
 	}
 
-	if (!buffer)
-		return 0;
-
 	attrcount = 0;
 	if (buffer->attrs) {
 		while (buffer->attrs[attrcount] != NULL)
@@ -1395,15 +1392,12 @@ 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)
+static void iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
 {
-	if (!indio_dev->buffer)
-		return;
-
-	bitmap_free(indio_dev->buffer->scan_mask);
-	kfree(indio_dev->buffer->buffer_group.attrs);
-	kfree(indio_dev->buffer->scan_el_group.attrs);
-	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
+	bitmap_free(buffer->scan_mask);
+	kfree(buffer->buffer_group.attrs);
+	kfree(buffer->scan_el_group.attrs);
+	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
 }
 
 static const struct file_operations iio_buffer_fileops = {
@@ -1425,13 +1419,19 @@ int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod)
 	if (!buffer)
 		return -ENOTSUPP;
 
+	ret = iio_buffer_alloc_sysfs_and_mask(buffer);
+	if (ret)
+		return ret;
+
 	cdev_init(&buffer->chrdev, &iio_buffer_fileops);
 
 	buffer->chrdev.owner = this_mod;
 
 	ret = cdev_device_add(&buffer->chrdev, &indio_dev->dev);
-	if (ret)
+	if (ret) {
+		iio_buffer_free_sysfs_and_mask(buffer);
 		return ret;
+	}
 
 	iio_device_get(indio_dev);
 	iio_buffer_get(buffer);
@@ -1457,6 +1457,7 @@ void iio_device_buffers_uninit(struct iio_dev *indio_dev)
 		return;
 
 	cdev_device_del(&buffer->chrdev, &indio_dev->dev);
+	iio_buffer_free_sysfs_and_mask(buffer);
 	iio_buffer_put(buffer);
 	iio_device_put(indio_dev);
 }
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2158aeab0bd2..794aaa4be832 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1737,18 +1737,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 
 	iio_device_register_debugfs(indio_dev);
 
-	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
-	if (ret) {
-		dev_err(indio_dev->dev.parent,
-			"Failed to create buffer sysfs interfaces\n");
-		goto error_unreg_debugfs;
-	}
-
 	ret = iio_device_register_sysfs(indio_dev);
 	if (ret) {
 		dev_err(indio_dev->dev.parent,
 			"Failed to register sysfs interfaces\n");
-		goto error_buffer_free_sysfs;
+		goto error_free_sysfs;
 	}
 	ret = iio_device_register_eventset(indio_dev);
 	if (ret) {
@@ -1785,9 +1778,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 	iio_device_unregister_eventset(indio_dev);
 error_free_sysfs:
 	iio_device_unregister_sysfs(indio_dev);
-error_buffer_free_sysfs:
-	iio_buffer_free_sysfs_and_mask(indio_dev);
-error_unreg_debugfs:
 	iio_device_unregister_debugfs(indio_dev);
 	return ret;
 }
@@ -1818,8 +1808,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 	iio_buffer_wakeup_poll(indio_dev);
 
 	mutex_unlock(&indio_dev->info_exist_lock);
-
-	iio_buffer_free_sysfs_and_mask(indio_dev);
 }
 EXPORT_SYMBOL(iio_device_unregister);
 
-- 
2.17.1

Re: [PATCH v3 4/5] iio: buffer: move sysfs alloc/free in industrialio-buffer.c

[Jonathan Cameron]

On Fri, 10 Apr 2020 17:17:28 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Since we now have the 'iio_device_buffers_{un}init()' entry-points into the
> industrialio-buffer.c we can now move the sysfs alloc & free in there as
> part of that init/uninit.
> 
> This changes the order of iio_device_register()/iio_device_unregister() a
> bit, but overall this shouldn't matter.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Seems fine at first glance.

Thanks,

Jonathan

> ---
>  drivers/iio/iio_core.h            | 10 ----------
>  drivers/iio/industrialio-buffer.c | 29 +++++++++++++++--------------
>  drivers/iio/industrialio-core.c   | 14 +-------------
>  3 files changed, 16 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 4bdadeac2710..1d69071e1426 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -48,9 +48,6 @@ void iio_device_buffers_uninit(struct iio_dev *indio_dev);
>  
>  void iio_device_buffers_put(struct iio_dev *indio_dev);
>  
> -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
> -
>  void iio_disable_all_buffers(struct iio_dev *indio_dev);
>  void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
>  
> @@ -66,13 +63,6 @@ static inline void iio_device_buffers_uninit(struct iio_dev *indio_dev) {}
>  
>  static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
>  
> -static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> -{
> -	return 0;
> -}
> -
> -static inline void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) {}
> -
>  static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
>  static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
>  
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 4b5c3baadaab..8ab089a9c3bc 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1284,11 +1284,11 @@ 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;
>  
> @@ -1301,9 +1301,6 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  		indio_dev->masklength = ml;
>  	}
>  
> -	if (!buffer)
> -		return 0;
> -
>  	attrcount = 0;
>  	if (buffer->attrs) {
>  		while (buffer->attrs[attrcount] != NULL)
> @@ -1395,15 +1392,12 @@ 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)
> +static void iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
>  {
> -	if (!indio_dev->buffer)
> -		return;
> -
> -	bitmap_free(indio_dev->buffer->scan_mask);
> -	kfree(indio_dev->buffer->buffer_group.attrs);
> -	kfree(indio_dev->buffer->scan_el_group.attrs);
> -	iio_free_chan_devattr_list(&indio_dev->buffer->scan_el_dev_attr_list);
> +	bitmap_free(buffer->scan_mask);
> +	kfree(buffer->buffer_group.attrs);
> +	kfree(buffer->scan_el_group.attrs);
> +	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
>  }
>  
>  static const struct file_operations iio_buffer_fileops = {
> @@ -1425,13 +1419,19 @@ int iio_device_buffers_init(struct iio_dev *indio_dev, struct module *this_mod)
>  	if (!buffer)
>  		return -ENOTSUPP;
>  
> +	ret = iio_buffer_alloc_sysfs_and_mask(buffer);
> +	if (ret)
> +		return ret;
> +
>  	cdev_init(&buffer->chrdev, &iio_buffer_fileops);
>  
>  	buffer->chrdev.owner = this_mod;
>  
>  	ret = cdev_device_add(&buffer->chrdev, &indio_dev->dev);
> -	if (ret)
> +	if (ret) {
> +		iio_buffer_free_sysfs_and_mask(buffer);
>  		return ret;
> +	}
>  
>  	iio_device_get(indio_dev);
>  	iio_buffer_get(buffer);
> @@ -1457,6 +1457,7 @@ void iio_device_buffers_uninit(struct iio_dev *indio_dev)
>  		return;
>  
>  	cdev_device_del(&buffer->chrdev, &indio_dev->dev);
> +	iio_buffer_free_sysfs_and_mask(buffer);
>  	iio_buffer_put(buffer);
>  	iio_device_put(indio_dev);
>  }
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 2158aeab0bd2..794aaa4be832 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1737,18 +1737,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  
>  	iio_device_register_debugfs(indio_dev);
>  
> -	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> -	if (ret) {
> -		dev_err(indio_dev->dev.parent,
> -			"Failed to create buffer sysfs interfaces\n");
> -		goto error_unreg_debugfs;
> -	}
> -
>  	ret = iio_device_register_sysfs(indio_dev);
>  	if (ret) {
>  		dev_err(indio_dev->dev.parent,
>  			"Failed to register sysfs interfaces\n");
> -		goto error_buffer_free_sysfs;
> +		goto error_free_sysfs;
>  	}
>  	ret = iio_device_register_eventset(indio_dev);
>  	if (ret) {
> @@ -1785,9 +1778,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  	iio_device_unregister_eventset(indio_dev);
>  error_free_sysfs:
>  	iio_device_unregister_sysfs(indio_dev);
> -error_buffer_free_sysfs:
> -	iio_buffer_free_sysfs_and_mask(indio_dev);
> -error_unreg_debugfs:
>  	iio_device_unregister_debugfs(indio_dev);
>  	return ret;
>  }
> @@ -1818,8 +1808,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  	iio_buffer_wakeup_poll(indio_dev);
>  
>  	mutex_unlock(&indio_dev->info_exist_lock);
> -
> -	iio_buffer_free_sysfs_and_mask(indio_dev);
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
>  

[PATCH v3 5/5] iio: event: move event-only chardev in industrialio-event.c

[Alexandru Ardelean]

At this point, we should be able to move the IIO device's chardev from the
iio_dev struct to the event_interface struct.

The IIO buffer chardev will also take-care of event ioctl() (if it is
initialized. The chardev for the event_interface (which has been moved to
industrialio-event.c) will be initialized only if iio_device_buffers_init()
returns -ENOTSUPP.

This does require an extra pair of iio_device_register_event_chrdev() &
iio_device_unregister_event_chrdev() functions. The
iio_device_register_event_chrdev() should (and will) be  only called if
iio_device_buffers_init() returns -ENOTSUPP.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/iio_core.h           |   5 +-
 drivers/iio/industrialio-core.c  |  94 +-----------------------
 drivers/iio/industrialio-event.c | 122 ++++++++++++++++++++++++++++++-
 include/linux/iio/iio.h          |   6 --
 4 files changed, 128 insertions(+), 99 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 1d69071e1426..2249e5b28023 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -68,10 +68,13 @@ static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
 
 #endif
 
+int iio_device_register_event_chrdev(struct iio_dev *indio_dev,
+				     struct module *this_mod);
+void iio_device_unregister_event_chrdev(struct iio_dev *indio_dev);
+
 int iio_device_register_eventset(struct iio_dev *indio_dev);
 void iio_device_unregister_eventset(struct iio_dev *indio_dev);
 void iio_device_wakeup_eventset(struct iio_dev *indio_dev);
-int iio_event_getfd(struct iio_dev *indio_dev);
 
 struct iio_event_interface;
 bool iio_event_enabled(const struct iio_event_interface *ev_int);
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 794aaa4be832..7d3258f4528b 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1609,76 +1609,6 @@ void devm_iio_device_free(struct device *dev, struct iio_dev *iio_dev)
 }
 EXPORT_SYMBOL_GPL(devm_iio_device_free);
 
-/**
- * iio_chrdev_open() - chrdev file open for event ioctls
- * @inode:	Inode structure for identifying the device in the file system
- * @filp:	File structure for iio device used to keep and later access
- *		private data
- *
- * Return: 0 on success or -EBUSY if the device is already opened
- **/
-static int iio_chrdev_open(struct inode *inode, struct file *filp)
-{
-	struct iio_dev *indio_dev = container_of(inode->i_cdev,
-						struct iio_dev, chrdev);
-
-	if (test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->flags))
-		return -EBUSY;
-
-	iio_device_get(indio_dev);
-
-	filp->private_data = indio_dev;
-
-	return 0;
-}
-
-/**
- * iio_chrdev_release() - chrdev file close for event ioctls
- * @inode:	Inode structure pointer for the char device
- * @filp:	File structure pointer for the char device
- *
- * Return: 0 for successful release
- */
-static int iio_chrdev_release(struct inode *inode, struct file *filp)
-{
-	struct iio_dev *indio_dev = container_of(inode->i_cdev,
-						struct iio_dev, chrdev);
-	clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
-	iio_device_put(indio_dev);
-
-	return 0;
-}
-
-long iio_device_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)
-			return fd;
-		if (copy_to_user(ip, &fd, sizeof(fd)))
-			return -EFAULT;
-		return 0;
-	}
-	return -EINVAL;
-}
-
-/* Somewhat of a cross file organization violation - ioctls here are actually
- * event related */
-static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
-				    unsigned long arg)
-{
-	struct iio_dev *indio_dev = filp->private_data;
-
-	return iio_device_event_ioctl(indio_dev, filp, cmd, arg);
-}
-
 static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
 {
 	int i, j;
@@ -1704,15 +1634,6 @@ 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_event_ioctl_wrapper,
-	.compat_ioctl = compat_ptr_ioctl,
-};
-
 int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 {
 	int ret;
@@ -1761,15 +1682,9 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 		if (ret != -ENOTSUPP)
 			goto error_unreg_eventset;
 
-		cdev_init(&indio_dev->chrdev, &iio_event_fileops);
-
-		indio_dev->chrdev.owner = this_mod;
-
-		ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
-		if (ret < 0)
+		ret = iio_device_register_event_chrdev(indio_dev, this_mod);
+		if (ret)
 			goto error_unreg_eventset;
-
-		indio_dev->chrdev_initialized = true;
 	}
 
 	return 0;
@@ -1789,10 +1704,7 @@ EXPORT_SYMBOL(__iio_device_register);
  **/
 void iio_device_unregister(struct iio_dev *indio_dev)
 {
-	if (indio_dev->chrdev_initialized)
-		cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
-
-	indio_dev->chrdev_initialized = false;
+	iio_device_unregister_event_chrdev(indio_dev);
 
 	iio_device_buffers_uninit(indio_dev);
 
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 5b17c92d3b50..da01c8c38f21 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -30,6 +30,9 @@
  * @flags:		file operations related flags including busy flag.
  * @group:		event interface sysfs attribute group
  * @read_lock:		lock to protect kfifo read operations
+ * @chrdev:		associated chardev for this event
+ * @chrdev_initialized:	true if this @chrdev was initialized
+ * @indio_dev:		IIO device to which this event interface belongs to
  */
 struct iio_event_interface {
 	wait_queue_head_t	wait;
@@ -39,6 +42,10 @@ struct iio_event_interface {
 	unsigned long		flags;
 	struct attribute_group	group;
 	struct mutex		read_lock;
+
+	struct cdev		chrdev;
+	bool			chrdev_initialized;
+	struct iio_dev		*indio_dev;
 };
 
 bool iio_event_enabled(const struct iio_event_interface *ev_int)
@@ -182,7 +189,7 @@ static const struct file_operations iio_event_chrdev_fileops = {
 	.llseek = noop_llseek,
 };
 
-int iio_event_getfd(struct iio_dev *indio_dev)
+static int iio_event_getfd(struct iio_dev *indio_dev)
 {
 	struct iio_event_interface *ev_int = indio_dev->event_interface;
 	int fd;
@@ -215,6 +222,119 @@ int iio_event_getfd(struct iio_dev *indio_dev)
 	return fd;
 }
 
+/**
+ * iio_chrdev_open() - chrdev file open for event ioctls
+ * @inode:	Inode structure for identifying the device in the file system
+ * @filp:	File structure for iio device used to keep and later access
+ *		private data
+ *
+ * Return: 0 on success or -EBUSY if the device is already opened
+ **/
+static int iio_chrdev_open(struct inode *inode, struct file *filp)
+{
+	struct iio_event_interface *ev =
+		container_of(inode->i_cdev, struct iio_event_interface, chrdev);
+
+	if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev->flags))
+		return -EBUSY;
+
+	iio_device_get(ev->indio_dev);
+
+	filp->private_data = ev;
+
+	return 0;
+}
+
+/**
+ * iio_chrdev_release() - chrdev file close for event ioctls
+ * @inode:	Inode structure pointer for the char device
+ * @filp:	File structure pointer for the char device
+ *
+ * Return: 0 for successful release
+ */
+static int iio_chrdev_release(struct inode *inode, struct file *filp)
+{
+	struct iio_event_interface *ev =
+		container_of(inode->i_cdev, struct iio_event_interface, chrdev);
+
+	clear_bit(IIO_BUSY_BIT_POS, &ev->flags);
+	iio_device_put(ev->indio_dev);
+
+	return 0;
+}
+
+long iio_device_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)
+			return fd;
+		if (copy_to_user(ip, &fd, sizeof(fd)))
+			return -EFAULT;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
+				    unsigned long arg)
+{
+	struct iio_event_interface *ev = filp->private_data;
+
+	return iio_device_event_ioctl(ev->indio_dev, filp, cmd, arg);
+}
+
+static const struct file_operations iio_device_event_fileops = {
+	.release = iio_chrdev_release,
+	.open = iio_chrdev_open,
+	.owner = THIS_MODULE,
+	.llseek = noop_llseek,
+	.unlocked_ioctl = iio_event_ioctl_wrapper,
+	.compat_ioctl = compat_ptr_ioctl,
+};
+
+int iio_device_register_event_chrdev(struct iio_dev *indio_dev,
+				     struct module *this_mod)
+{
+	struct iio_event_interface *ev = indio_dev->event_interface;
+	int ret;
+
+	if (!ev)
+		return 0;
+
+	cdev_init(&ev->chrdev, &iio_device_event_fileops);
+
+	ev->chrdev.owner = this_mod;
+
+	ret = cdev_device_add(&ev->chrdev, &indio_dev->dev);
+	if (ret < 0)
+		return ret;
+
+	ev->chrdev_initialized = true;
+
+	return 0;
+}
+
+void iio_device_unregister_event_chrdev(struct iio_dev *indio_dev)
+{
+	struct iio_event_interface *ev = indio_dev->event_interface;
+
+	if (!ev)
+		return;
+
+	if (ev->chrdev_initialized)
+		cdev_device_del(&ev->chrdev, &indio_dev->dev);
+
+	ev->chrdev_initialized = false;
+}
+
 static const char * const iio_ev_type_text[] = {
 	[IIO_EV_TYPE_THRESH] = "thresh",
 	[IIO_EV_TYPE_MAG] = "mag",
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index e93497f483f7..8fa22e4e246f 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -516,11 +516,8 @@ struct iio_buffer_setup_ops {
  * @info_exist_lock:	[INTERN] lock to prevent use during removal
  * @setup_ops:		[DRIVER] callbacks to call before and after buffer
  *			enable/disable
- * @chrdev:		[INTERN] associated character device
- * @chrdev_initialized:	[INTERN] true if @chrdev device has been initialized
  * @groups:		[INTERN] attribute groups
  * @groupcounter:	[INTERN] index of next attribute group
- * @flags:		[INTERN] file ops related flags including busy flag.
  * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
  * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
  */
@@ -560,13 +557,10 @@ struct iio_dev {
 	clockid_t			clock_id;
 	struct mutex			info_exist_lock;
 	const struct iio_buffer_setup_ops	*setup_ops;
-	struct cdev			chrdev;
-	int				chrdev_initialized;
 #define IIO_MAX_GROUPS 6
 	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
 	int				groupcounter;
 
-	unsigned long			flags;
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry			*debugfs_dentry;
 	unsigned			cached_reg_addr;
-- 
2.17.1

Re: [PATCH v3 0/5] iio: core,buffer: re-organize chardev creation

[Ardelean, Alexandru]

On Fri, 2020-04-10 at 17:17 +0300, Alexandru Ardelean wrote:
> The main intent is to be able to add more chardevs per IIO device, one
> for each buffer. To get there, some rework is needed, and one important
> facet is to move the creation of the current chardev into
> 'industrialio-buffer.c', so that control/logic of these chardevs is
> localized in that file.
> 

This needs a new V4 now.
The drop of the devm_ APIs is causing some conflicts for this series.

Will send one tomorrow.

Still, any feedback until then [about this] is appreciated.

Thanks
Alex

> This changeset does that [incrementally] by moving the common chardev
> creation from 'industrialio-core.c' to 'industrialio-buffer.c' &
> 'industrialio-event.c'.
> The common chardev is required for both IIO buffers & IIO events.
> In order to make this work, the 'iio_device_event_ioctl()' needs to be
> passed from 'industrialio-event.c' to 'industrialio-buffer.c' flying past
> 'industrialio-core.c'. This sounds a bit wrong [at first] but it has the
> effect of reducing inter-dependencies between 'industrialio-core.c' to
> 'industrialio-buffer.c' quite a bit.
> The IIO buffer also has a CONFIG_IIO_BUFFER symbol which can turn it
> off. No idea how widely this is used [as disabled], but this changeset
> also takes that into consideration.
> 
> So, now the logic [for __iio_device_register() with regard to chardev
> init] is:
> 1. iio_device_buffers_init() will init buffer and the chardev, if that
>    works, the 'iio_device_event_ioctl()' will be attached to the chardev
> 2. if CONFIG_IIO_BUFFER is not defined or 'indio_dev->buffer == NULL'
>    (no buffter attached), -ENOTSUPP should be returned from
>    iio_device_buffers_init(), in which case the chardev should be
>    initialized in  'industrialio-event.c' via
>    'iio_device_register_event_chrdev()'
> 
> One neat side effect of this logic, is that we can also move the buffer
> sysfs alloc/cleanup into 'industrialio-buffer.c' under the new
> 'iio_device_buffers_{un}init()' functions.
> 
> Changelog v2 -> v3:
> * removed double init in
>   'iio: event: move event-only chardev in industrialio-event.c'
> 
> Changelog v1 -> v2:
> * re-reviewed some exit-paths and cleanup some potential leaks on those
>   exit paths:
>   - for 'iio: buffer: move iio buffer chrdev in industrialio-buffer.c'
>     add iio_device_buffers_put() helper and calling iio_buffers_uninit()
>     on device un-regsiter
>   - for 'move sysfs alloc/free in industrialio-buffer.c'
>     call 'iio_buffer_free_sysfs_and_mask()' on exit path if
>     cdev_device_add() fails
>   - for 'move event-only chardev in industrialio-event.c'
>     check if event_interface is NULL in
>     iio_device_unregister_event_chrdev()
> 
> Alexandru Ardelean (5):
>   iio: core: register buffer fileops only if buffer present
>   iio: buffer: add back-ref from iio_buffer to iio_dev
>   iio: buffer: move iio buffer chrdev in industrialio-buffer.c
>   iio: buffer: move sysfs alloc/free in industrialio-buffer.c
>   iio: event: move event-only chardev in industrialio-event.c
> 
>  drivers/iio/iio_core.h            |  30 +++----
>  drivers/iio/industrialio-buffer.c | 144 ++++++++++++++++++++++++++----
>  drivers/iio/industrialio-core.c   | 107 +++-------------------
>  drivers/iio/industrialio-event.c  | 122 ++++++++++++++++++++++++-
>  include/linux/iio/buffer_impl.h   |  10 +++
>  include/linux/iio/iio.h           |   4 -
>  6 files changed, 285 insertions(+), 132 deletions(-)
>