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.
This tries to re-organize the init of the chardev.
Changelog v4 -> v5:
- dropped patch 'iio: Use an early return in iio_device_alloc to simplify code.'
is applied upstream
Changelog v3 -> v4:
- added patch [1] 'iio: Use an early return in iio_device_alloc to simplify code.'
it's main purpose is so that this patch applies:
[2]'iio: core: add simple centralized mechanism for ioctl() handlers'
depending on the final version of patch [1], patch [2] needs some
minor fixup
- added patch 'iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put()'
- patch 'iio: core: register buffer fileops only if buffer present'
is now: 'iio: core: register chardev only if needed'
- dropped 'iio: buffer: move sysfs alloc/free in industrialio-buffer.c'
it's likely we won't be doing this patch anymore
- patches:
'iio: buffer: move iio buffer chrdev in industrialio-buffer.c'
'iio: event: move event-only chardev in industrialio-event.c'
have been merged into 'iio: buffer,event: duplicate chardev creation for buffers & events'
since now, the logic is a bit different, and 'indio_dev->chrdev' is
now a reference to either the buffer's chrdev & or the events-only
chrdev
- added simple mechanism to register ioctl() handlers for IIO device
which is currently used only by events mechanism
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 (6):
iio: buffer: add back-ref from iio_buffer to iio_dev
iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put()
iio: core: register chardev only if needed
iio: buffer,event: duplicate chardev creation for buffers & events
iio: core: add simple centralized mechanism for ioctl() handlers
iio: core: use new common ioctl() mechanism
drivers/iio/iio_core.h | 29 ++++++---
drivers/iio/industrialio-buffer.c | 102 +++++++++++++++++++++++++++--
drivers/iio/industrialio-core.c | 105 +++++++++++-------------------
drivers/iio/industrialio-event.c | 100 +++++++++++++++++++++++++++-
include/linux/iio/buffer_impl.h | 10 +++
include/linux/iio/iio.h | 8 +--
6 files changed, 267 insertions(+), 87 deletions(-)
--
2.17.1
The final intent is to localize all buffer ops into the
industrialio-buffer.c file, to be able to add support for multiple buffers
per IIO device.
We only need a chardev if we need to support buffers and/or events.
With this change, a chardev will be created only if an IIO buffer is
attached OR an event_interface is configured.
Otherwise, no chardev will be created, and the IIO device will get
registered with the 'device_add()' call.
Quite a lot of IIO devices don't really need a chardev, so this is a minor
improvement to the IIO core, as the IIO device will take up (slightly)
fewer resources.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/industrialio-core.c | 25 ++++++++++++++++++++++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 32c489139cd2..a65022396329 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1682,6 +1682,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;
@@ -1732,11 +1741,18 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
indio_dev->setup_ops == NULL)
indio_dev->setup_ops = &noop_ring_setup_ops;
- cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
+ if (indio_dev->buffer)
+ cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
+ else if (indio_dev->event_interface)
+ cdev_init(&indio_dev->chrdev, &iio_event_fileops);
indio_dev->chrdev.owner = this_mod;
- ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
+ if (indio_dev->buffer || indio_dev->event_interface)
+ ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
+ else
+ ret = device_add(&indio_dev->dev);
+
if (ret < 0)
goto error_unreg_eventset;
@@ -1760,7 +1776,10 @@ EXPORT_SYMBOL(__iio_device_register);
**/
void iio_device_unregister(struct iio_dev *indio_dev)
{
- cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
+ if (indio_dev->buffer || indio_dev->event_interface)
+ cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
+ else
+ device_del(&indio_dev->dev);
mutex_lock(&indio_dev->info_exist_lock);
--
2.17.1
This patch moves the chardev creation into industrialio-buffer.c &
industrialio-event.c. The move has to be in a single step (which makes the
patch a bit big), in order to pass the reference of the chardev to
'indio_dev->chrdev'.
For structure purposes, industrialio-core.c should be the place where
cdev_device_add()/device_add() gets called, and the deletion function as
well.
What happens after this patch is:
- 'indio_dev->chrdev' is converted to a pointer
- if there is an IIO buffer, iio_device_buffer_attach_chrdev() will attach
it's chardev to 'indio_chrdev'
- if it doesn't, the event interface will attach a reference to it's
chardev (if it is instantiated) via iio_device_event_attach_chrdev()
That way, the control of the 'legacy' chardev is still visible in
'industrialio-core.c'. So, each logic file (for buffers & events) shouldn't
hide things too much away from the core file.
Signed-off-by: Alexandru Ardelean <[email protected]>
---
drivers/iio/iio_core.h | 16 ++---
drivers/iio/industrialio-buffer.c | 90 +++++++++++++++++++++++--
drivers/iio/industrialio-core.c | 105 ++++--------------------------
drivers/iio/industrialio-event.c | 98 +++++++++++++++++++++++++++-
include/linux/iio/buffer_impl.h | 7 ++
include/linux/iio/iio.h | 6 +-
6 files changed, 208 insertions(+), 114 deletions(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 39ec0344fb68..a527a66be9e5 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -40,17 +40,14 @@ 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);
+
+void iio_device_buffer_attach_chrdev(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_device_buffers_put(struct iio_dev *indio_dev);
void iio_disable_all_buffers(struct iio_dev *indio_dev);
@@ -58,8 +55,7 @@ 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 void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev) {}
static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
{
@@ -77,8 +73,8 @@ static inline void iio_buffer_wakeup_poll(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_event_attach_chrdev(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-buffer.c b/drivers/iio/industrialio-buffer.c
index a66d3fbc2905..f5a975079bf4 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
@@ -1129,6 +1171,17 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
iio_buffer_deactivate_all(indio_dev);
}
+static long iio_buffer_ioctl(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ struct iio_buffer *buffer = filep->private_data;
+
+ if (!buffer || !buffer->access)
+ return -ENODEV;
+
+ return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
+}
+
static ssize_t iio_buffer_store_enable(struct device *dev,
struct device_attribute *attr,
const char *buf,
@@ -1355,6 +1408,29 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
iio_free_chan_devattr_list(&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,
+};
+
+void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev)
+{
+ struct iio_buffer *buffer = indio_dev->buffer;
+
+ if (!buffer)
+ return;
+
+ cdev_init(&buffer->chrdev, &iio_buffer_fileops);
+
+ indio_dev->chrdev = &buffer->chrdev;
+}
+
void iio_device_buffers_put(struct iio_dev *indio_dev)
{
struct iio_buffer *buffer = indio_dev->buffer;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index a65022396329..aec585cc8453 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1584,79 +1584,6 @@ struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
}
EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
-/**
- * iio_chrdev_open() - chrdev file open for buffer access and 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 buffer access and 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;
-}
-
-/* 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)
-{
- struct iio_dev *indio_dev = filp->private_data;
- 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 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,
-};
-
static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
{
int i, j;
@@ -1682,15 +1609,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_ioctl,
- .compat_ioctl = compat_ptr_ioctl,
-};
-
int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
{
int ret;
@@ -1741,16 +1659,17 @@ 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 if (indio_dev->event_interface)
- cdev_init(&indio_dev->chrdev, &iio_event_fileops);
+ iio_device_buffer_attach_chrdev(indio_dev);
- indio_dev->chrdev.owner = this_mod;
+ /* no chrdev attached from buffer, we go with event-only chrdev */
+ if (!indio_dev->chrdev)
+ iio_device_event_attach_chrdev(indio_dev);
- if (indio_dev->buffer || indio_dev->event_interface)
- ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
- else
+ if (indio_dev->chrdev) {
+ indio_dev->chrdev->owner = this_mod;
+
+ ret = cdev_device_add(indio_dev->chrdev, &indio_dev->dev);
+ } else
ret = device_add(&indio_dev->dev);
if (ret < 0)
@@ -1776,13 +1695,15 @@ EXPORT_SYMBOL(__iio_device_register);
**/
void iio_device_unregister(struct iio_dev *indio_dev)
{
- if (indio_dev->buffer || indio_dev->event_interface)
- cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
+ if (indio_dev->chrdev)
+ cdev_device_del(indio_dev->chrdev, &indio_dev->dev);
else
device_del(&indio_dev->dev);
mutex_lock(&indio_dev->info_exist_lock);
+ indio_dev->chrdev = NULL;
+
iio_device_unregister_debugfs(indio_dev);
iio_disable_all_buffers(indio_dev);
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 5b17c92d3b50..0674b2117c98 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -30,6 +30,8 @@
* @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
+ * @indio_dev: IIO device to which this event interface belongs to
*/
struct iio_event_interface {
wait_queue_head_t wait;
@@ -39,6 +41,9 @@ struct iio_event_interface {
unsigned long flags;
struct attribute_group group;
struct mutex read_lock;
+
+ struct cdev chrdev;
+ struct iio_dev *indio_dev;
};
bool iio_event_enabled(const struct iio_event_interface *ev_int)
@@ -182,7 +187,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 +220,97 @@ 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_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,
+};
+
+void iio_device_event_attach_chrdev(struct iio_dev *indio_dev)
+{
+ struct iio_event_interface *ev = indio_dev->event_interface;
+
+ if (!ev)
+ return;
+
+ cdev_init(&ev->chrdev, &iio_event_fileops);
+
+ ev->indio_dev = indio_dev;
+ indio_dev->chrdev = &ev->chrdev;
+}
+
static const char * const iio_ev_type_text[] = {
[IIO_EV_TYPE_THRESH] = "thresh",
[IIO_EV_TYPE_MAG] = "mag",
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 67d73d465e02..46fc977deae3 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 5f9f439a4f01..52992be44e9e 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -516,10 +516,9 @@ 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: [INTERN] reference to associated character device
* @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.
*/
@@ -559,12 +558,11 @@ struct iio_dev {
clockid_t clock_id;
struct mutex info_exist_lock;
const struct iio_buffer_setup_ops *setup_ops;
- struct cdev chrdev;
+ struct cdev *chrdev;
#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
On Sun, 26 Apr 2020 10:38:15 +0300
Alexandru Ardelean <[email protected]> wrote:
> This patch moves the chardev creation into industrialio-buffer.c &
> industrialio-event.c. The move has to be in a single step (which makes the
> patch a bit big), in order to pass the reference of the chardev to
> 'indio_dev->chrdev'.
>
> For structure purposes, industrialio-core.c should be the place where
> cdev_device_add()/device_add() gets called, and the deletion function as
> well.
>
> What happens after this patch is:
> - 'indio_dev->chrdev' is converted to a pointer
> - if there is an IIO buffer, iio_device_buffer_attach_chrdev() will attach
> it's chardev to 'indio_chrdev'
> - if it doesn't, the event interface will attach a reference to it's
> chardev (if it is instantiated) via iio_device_event_attach_chrdev()
>
> That way, the control of the 'legacy' chardev is still visible in
> 'industrialio-core.c'. So, each logic file (for buffers & events) shouldn't
> hide things too much away from the core file.
>
> Signed-off-by: Alexandru Ardelean <[email protected]>
A few really minor suggestions inline. At least half of them are tidying up
things whilst we are here that were ugly in the code we are duplicating.
Looks good to me. However, I'd like more eyes on this set before I apply it
so will give it at least a week or two on the list!
Jonathan
> ---
> drivers/iio/iio_core.h | 16 ++---
> drivers/iio/industrialio-buffer.c | 90 +++++++++++++++++++++++--
> drivers/iio/industrialio-core.c | 105 ++++--------------------------
> drivers/iio/industrialio-event.c | 98 +++++++++++++++++++++++++++-
> include/linux/iio/buffer_impl.h | 7 ++
> include/linux/iio/iio.h | 6 +-
> 6 files changed, 208 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 39ec0344fb68..a527a66be9e5 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -40,17 +40,14 @@ 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);
> +
> +void iio_device_buffer_attach_chrdev(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_device_buffers_put(struct iio_dev *indio_dev);
>
> void iio_disable_all_buffers(struct iio_dev *indio_dev);
> @@ -58,8 +55,7 @@ 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 void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev) {}
>
> static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> {
> @@ -77,8 +73,8 @@ static inline void iio_buffer_wakeup_poll(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_event_attach_chrdev(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-buffer.c b/drivers/iio/industrialio-buffer.c
> index a66d3fbc2905..f5a975079bf4 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
> @@ -1129,6 +1171,17 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
> iio_buffer_deactivate_all(indio_dev);
> }
>
> +static long iio_buffer_ioctl(struct file *filep, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct iio_buffer *buffer = filep->private_data;
> +
> + if (!buffer || !buffer->access)
> + return -ENODEV;
> +
> + return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
> +}
> +
> static ssize_t iio_buffer_store_enable(struct device *dev,
> struct device_attribute *attr,
> const char *buf,
> @@ -1355,6 +1408,29 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> iio_free_chan_devattr_list(&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,
> +};
> +
> +void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev)
> +{
> + struct iio_buffer *buffer = indio_dev->buffer;
> +
> + if (!buffer)
> + return;
> +
> + cdev_init(&buffer->chrdev, &iio_buffer_fileops);
> +
> + indio_dev->chrdev = &buffer->chrdev;
> +}
> +
> void iio_device_buffers_put(struct iio_dev *indio_dev)
> {
> struct iio_buffer *buffer = indio_dev->buffer;
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index a65022396329..aec585cc8453 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1584,79 +1584,6 @@ struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
> }
> EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
>
> -/**
> - * iio_chrdev_open() - chrdev file open for buffer access and 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 buffer access and 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;
> -}
> -
> -/* 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)
> -{
> - struct iio_dev *indio_dev = filp->private_data;
> - 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 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,
> -};
> -
> static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
> {
> int i, j;
> @@ -1682,15 +1609,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_ioctl,
> - .compat_ioctl = compat_ptr_ioctl,
> -};
> -
> int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> {
> int ret;
> @@ -1741,16 +1659,17 @@ 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 if (indio_dev->event_interface)
> - cdev_init(&indio_dev->chrdev, &iio_event_fileops);
> + iio_device_buffer_attach_chrdev(indio_dev);
>
> - indio_dev->chrdev.owner = this_mod;
> + /* no chrdev attached from buffer, we go with event-only chrdev */
> + if (!indio_dev->chrdev)
> + iio_device_event_attach_chrdev(indio_dev);
>
> - if (indio_dev->buffer || indio_dev->event_interface)
> - ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> - else
> + if (indio_dev->chrdev) {
> + indio_dev->chrdev->owner = this_mod;
> +
> + ret = cdev_device_add(indio_dev->chrdev, &indio_dev->dev);
> + } else
> ret = device_add(&indio_dev->dev);
>
> if (ret < 0)
> @@ -1776,13 +1695,15 @@ EXPORT_SYMBOL(__iio_device_register);
> **/
> void iio_device_unregister(struct iio_dev *indio_dev)
> {
> - if (indio_dev->buffer || indio_dev->event_interface)
> - cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> + if (indio_dev->chrdev)
> + cdev_device_del(indio_dev->chrdev, &indio_dev->dev);
> else
> device_del(&indio_dev->dev);
>
> mutex_lock(&indio_dev->info_exist_lock);
>
> + indio_dev->chrdev = NULL;
> +
Why? I can't immediately see the reason and I'm too lazy to figure it out
on a Sunday morning ;)
> iio_device_unregister_debugfs(indio_dev);
>
> iio_disable_all_buffers(indio_dev);
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 5b17c92d3b50..0674b2117c98 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -30,6 +30,8 @@
> * @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
> + * @indio_dev: IIO device to which this event interface belongs to
> */
> struct iio_event_interface {
> wait_queue_head_t wait;
> @@ -39,6 +41,9 @@ struct iio_event_interface {
> unsigned long flags;
> struct attribute_group group;
> struct mutex read_lock;
> +
> + struct cdev chrdev;
> + struct iio_dev *indio_dev;
> };
>
> bool iio_event_enabled(const struct iio_event_interface *ev_int)
> @@ -182,7 +187,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 +220,97 @@ 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
> + **/
Don't we use the */ termination or kernel-doc through out IIO. Nice to be
consistent. Ah. I see we don't. Tidy that up whilst you are here :)
> +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;
> +
Given we've has some confusion on this recently perhaps a note that
this is protection on driver 'going away'. We could even
add a little wrapper around it to make it explicit in the code.
What do you think? Maybe a comment is enough.
> + 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_event_fileops = {
> + .release = iio_chrdev_release,
> + .open = iio_chrdev_open,
> + .owner = THIS_MODULE,
As we are here. Might be nice to order this to match the file_operations structure
ordering. Certainly seems odd to have .owner in the middle..
lalala for where you might have copied this from ;)
> + .llseek = noop_llseek,
> + .unlocked_ioctl = iio_event_ioctl_wrapper,
> + .compat_ioctl = compat_ptr_ioctl,
> +};
> +
> +void iio_device_event_attach_chrdev(struct iio_dev *indio_dev)
> +{
> + struct iio_event_interface *ev = indio_dev->event_interface;
> +
> + if (!ev)
> + return;
> +
> + cdev_init(&ev->chrdev, &iio_event_fileops);
> +
> + ev->indio_dev = indio_dev;
> + indio_dev->chrdev = &ev->chrdev;
> +}
> +
> static const char * const iio_ev_type_text[] = {
> [IIO_EV_TYPE_THRESH] = "thresh",
> [IIO_EV_TYPE_MAG] = "mag",
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 67d73d465e02..46fc977deae3 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 5f9f439a4f01..52992be44e9e 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -516,10 +516,9 @@ 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: [INTERN] reference to associated character device
> * @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.
> */
> @@ -559,12 +558,11 @@ struct iio_dev {
> clockid_t clock_id;
> struct mutex info_exist_lock;
> const struct iio_buffer_setup_ops *setup_ops;
> - struct cdev chrdev;
> + struct cdev *chrdev;
> #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;
On Sun, 2020-04-26 at 11:00 +0100, Jonathan Cameron wrote:
> [External]
>
> On Sun, 26 Apr 2020 10:38:15 +0300
> Alexandru Ardelean <[email protected]> wrote:
>
> > This patch moves the chardev creation into industrialio-buffer.c &
> > industrialio-event.c. The move has to be in a single step (which makes the
> > patch a bit big), in order to pass the reference of the chardev to
> > 'indio_dev->chrdev'.
> >
> > For structure purposes, industrialio-core.c should be the place where
> > cdev_device_add()/device_add() gets called, and the deletion function as
> > well.
> >
> > What happens after this patch is:
> > - 'indio_dev->chrdev' is converted to a pointer
> > - if there is an IIO buffer, iio_device_buffer_attach_chrdev() will attach
> > it's chardev to 'indio_chrdev'
> > - if it doesn't, the event interface will attach a reference to it's
> > chardev (if it is instantiated) via iio_device_event_attach_chrdev()
> >
> > That way, the control of the 'legacy' chardev is still visible in
> > 'industrialio-core.c'. So, each logic file (for buffers & events) shouldn't
> > hide things too much away from the core file.
> >
> > Signed-off-by: Alexandru Ardelean <[email protected]>
>
> A few really minor suggestions inline. At least half of them are tidying up
> things whilst we are here that were ugly in the code we are duplicating.
>
> Looks good to me. However, I'd like more eyes on this set before I apply it
> so will give it at least a week or two on the list!
>
> Jonathan
>
> > ---
> > drivers/iio/iio_core.h | 16 ++---
> > drivers/iio/industrialio-buffer.c | 90 +++++++++++++++++++++++--
> > drivers/iio/industrialio-core.c | 105 ++++--------------------------
> > drivers/iio/industrialio-event.c | 98 +++++++++++++++++++++++++++-
> > include/linux/iio/buffer_impl.h | 7 ++
> > include/linux/iio/iio.h | 6 +-
> > 6 files changed, 208 insertions(+), 114 deletions(-)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index 39ec0344fb68..a527a66be9e5 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -40,17 +40,14 @@ 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);
> > +
> > +void iio_device_buffer_attach_chrdev(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_device_buffers_put(struct iio_dev *indio_dev);
> >
> > void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > @@ -58,8 +55,7 @@ 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 void iio_device_buffer_attach_chrdev(struct iio_dev
> > *indio_dev) {}
> >
> > static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> > {
> > @@ -77,8 +73,8 @@ static inline void iio_buffer_wakeup_poll(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_event_attach_chrdev(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-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index a66d3fbc2905..f5a975079bf4 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
> > @@ -1129,6 +1171,17 @@ void iio_disable_all_buffers(struct iio_dev
> > *indio_dev)
> > iio_buffer_deactivate_all(indio_dev);
> > }
> >
> > +static long iio_buffer_ioctl(struct file *filep, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + struct iio_buffer *buffer = filep->private_data;
> > +
> > + if (!buffer || !buffer->access)
> > + return -ENODEV;
> > +
> > + return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
> > +}
> > +
> > static ssize_t iio_buffer_store_enable(struct device *dev,
> > struct device_attribute *attr,
> > const char *buf,
> > @@ -1355,6 +1408,29 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> > iio_free_chan_devattr_list(&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,
> > +};
> > +
> > +void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev)
> > +{
> > + struct iio_buffer *buffer = indio_dev->buffer;
> > +
> > + if (!buffer)
> > + return;
> > +
> > + cdev_init(&buffer->chrdev, &iio_buffer_fileops);
> > +
> > + indio_dev->chrdev = &buffer->chrdev;
> > +}
> > +
> > void iio_device_buffers_put(struct iio_dev *indio_dev)
> > {
> > struct iio_buffer *buffer = indio_dev->buffer;
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index a65022396329..aec585cc8453 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1584,79 +1584,6 @@ struct iio_dev *devm_iio_device_alloc(struct device
> > *dev, int sizeof_priv)
> > }
> > EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
> >
> > -/**
> > - * iio_chrdev_open() - chrdev file open for buffer access and 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 buffer access and 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;
> > -}
> > -
> > -/* 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)
> > -{
> > - struct iio_dev *indio_dev = filp->private_data;
> > - 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 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,
> > -};
> > -
> > static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
> > {
> > int i, j;
> > @@ -1682,15 +1609,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_ioctl,
> > - .compat_ioctl = compat_ptr_ioctl,
> > -};
> > -
> > int __iio_device_register(struct iio_dev *indio_dev, struct module
> > *this_mod)
> > {
> > int ret;
> > @@ -1741,16 +1659,17 @@ 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 if (indio_dev->event_interface)
> > - cdev_init(&indio_dev->chrdev, &iio_event_fileops);
> > + iio_device_buffer_attach_chrdev(indio_dev);
> >
> > - indio_dev->chrdev.owner = this_mod;
> > + /* no chrdev attached from buffer, we go with event-only chrdev */
> > + if (!indio_dev->chrdev)
> > + iio_device_event_attach_chrdev(indio_dev);
> >
> > - if (indio_dev->buffer || indio_dev->event_interface)
> > - ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> > - else
> > + if (indio_dev->chrdev) {
> > + indio_dev->chrdev->owner = this_mod;
> > +
> > + ret = cdev_device_add(indio_dev->chrdev, &indio_dev->dev);
> > + } else
> > ret = device_add(&indio_dev->dev);
> >
> > if (ret < 0)
> > @@ -1776,13 +1695,15 @@ EXPORT_SYMBOL(__iio_device_register);
> > **/
> > void iio_device_unregister(struct iio_dev *indio_dev)
> > {
> > - if (indio_dev->buffer || indio_dev->event_interface)
> > - cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> > + if (indio_dev->chrdev)
> > + cdev_device_del(indio_dev->chrdev, &indio_dev->dev);
> > else
> > device_del(&indio_dev->dev);
> >
> > mutex_lock(&indio_dev->info_exist_lock);
> >
> > + indio_dev->chrdev = NULL;
> > +
>
> Why? I can't immediately see the reason and I'm too lazy to figure it out
> on a Sunday morning ;)
Paranoid code.
Initially I just thought I'd add it as a similar mechanism for 'indio_dev->info'
being made NULL later.
But I can't think of a good reason.
I guess I added it in the middle of coding, without much thought.
Especially, since there isn't any code to check this being NULL to do anything.
Will remove it.
'indio_dev->info = NULL' should be enough.
>
> > iio_device_unregister_debugfs(indio_dev);
> >
> > iio_disable_all_buffers(indio_dev);
> > diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-
> > event.c
> > index 5b17c92d3b50..0674b2117c98 100644
> > --- a/drivers/iio/industrialio-event.c
> > +++ b/drivers/iio/industrialio-event.c
> > @@ -30,6 +30,8 @@
> > * @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
> > + * @indio_dev: IIO device to which this event interface belongs
> > to
> > */
> > struct iio_event_interface {
> > wait_queue_head_t wait;
> > @@ -39,6 +41,9 @@ struct iio_event_interface {
> > unsigned long flags;
> > struct attribute_group group;
> > struct mutex read_lock;
> > +
> > + struct cdev chrdev;
> > + struct iio_dev *indio_dev;
> > };
> >
> > bool iio_event_enabled(const struct iio_event_interface *ev_int)
> > @@ -182,7 +187,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 +220,97 @@ 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
> > + **/
>
> Don't we use the */ termination or kernel-doc through out IIO. Nice to be
> consistent. Ah. I see we don't. Tidy that up whilst you are here :)
ack
>
>
> > +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;
> > +
>
> Given we've has some confusion on this recently perhaps a note that
> this is protection on driver 'going away'. We could even
> add a little wrapper around it to make it explicit in the code.
> What do you think? Maybe a comment is enough.
Comment sounds good enough.
This code should get called via 2 potential code-paths: 1 via events chardev & 1
via buffer chardev.
>
> > + 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_event_fileops = {
> > + .release = iio_chrdev_release,
> > + .open = iio_chrdev_open,
> > + .owner = THIS_MODULE,
>
> As we are here. Might be nice to order this to match the file_operations
> structure
> ordering. Certainly seems odd to have .owner in the middle..
> lalala for where you might have copied this from ;)
ack
>
> > + .llseek = noop_llseek,
> > + .unlocked_ioctl = iio_event_ioctl_wrapper,
> > + .compat_ioctl = compat_ptr_ioctl,
> > +};
> > +
> > +void iio_device_event_attach_chrdev(struct iio_dev *indio_dev)
> > +{
> > + struct iio_event_interface *ev = indio_dev->event_interface;
> > +
> > + if (!ev)
> > + return;
> > +
> > + cdev_init(&ev->chrdev, &iio_event_fileops);
> > +
> > + ev->indio_dev = indio_dev;
> > + indio_dev->chrdev = &ev->chrdev;
> > +}
> > +
> > static const char * const iio_ev_type_text[] = {
> > [IIO_EV_TYPE_THRESH] = "thresh",
> > [IIO_EV_TYPE_MAG] = "mag",
> > diff --git a/include/linux/iio/buffer_impl.h
> > b/include/linux/iio/buffer_impl.h
> > index 67d73d465e02..46fc977deae3 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 5f9f439a4f01..52992be44e9e 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -516,10 +516,9 @@ 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: [INTERN] reference to associated character
> > device
> > * @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.
> > */
> > @@ -559,12 +558,11 @@ struct iio_dev {
> > clockid_t clock_id;
> > struct mutex info_exist_lock;
> > const struct iio_buffer_setup_ops *setup_ops;
> > - struct cdev chrdev;
> > + struct cdev *chrdev;
> > #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;