On Thu, 11 Feb 2021 14:24:41 +0200
Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
> With this change, we create a new directory for the IIO device called
> buffer0, under which both the old buffer/ and scan_elements/ are stored.
>
> This is done to simplify the addition of multiple IIO buffers per IIO
> device. Otherwise we would need to add a bufferX/ and scan_elementsX/
> directory for each IIO buffer.
> With the current way of storing attribute groups, we can't have directories
> stored under each other (i.e. scan_elements/ under buffer/), so the best
> approach moving forward is to merge their attributes.
>
> The old/legacy buffer/ & scan_elements/ groups are not stored on the opaque
> IIO device object. This way the IIO buffer can have just a single
> attribute_group object, saving a bit of memory when adding multiple IIO
> buffers.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
Two trivial things inline.
Jonathan
> ---
> drivers/iio/industrialio-buffer.c | 115 +++++++++++++++++++++++-------
> include/linux/iio/buffer_impl.h | 9 +--
> include/linux/iio/iio-opaque.h | 4 ++
> 3 files changed, 95 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 23f22be62cc7..fcbbffb904bf 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> return (ret < 0) ? ret : len;
> }
>
> -static const char * const iio_scan_elements_group_name = "scan_elements";
> -
> static ssize_t iio_buffer_show_watermark(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -1252,8 +1250,68 @@ static struct attribute *iio_buffer_attrs[] = {
> &dev_attr_data_available.attr,
> };
>
> +static int iio_buffer_register_legacy_sysfs_groups(struct iio_dev *indio_dev,
> + struct attribute **buffer_attrs,
> + int buffer_attrcount,
> + int scan_el_attrcount)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + struct attribute_group *group;
> + int ret;
> +
> + group = &iio_dev_opaque->legacy_buffer_group;
> +
> + group->attrs = kcalloc(buffer_attrcount + 1,
> + sizeof(struct attribute *), GFP_KERNEL);
> + if (!group->attrs)
> + return -ENOMEM;
> +
> + memcpy(group->attrs, buffer_attrs,
> + buffer_attrcount * sizeof(struct attribute *));
> + group->name = "buffer";
> +
> + ret = iio_device_register_sysfs_group(indio_dev, group);
> + if (ret)
> + goto error_free_buffer_attrs;
> +
> + group = &iio_dev_opaque->legacy_scan_el_group;
> +
> + group->attrs = kcalloc(scan_el_attrcount + 1,
> + sizeof(struct attribute *), GFP_KERNEL);
> + if (!group->attrs) {
> + ret = -ENOMEM;
> + goto error_free_buffer_attrs;
> + }
> +
> + memcpy(group->attrs, &buffer_attrs[buffer_attrcount],
> + scan_el_attrcount * sizeof(struct attribute *));
> + group->name = "scan_elements";
> +
> + ret = iio_device_register_sysfs_group(indio_dev, group);
> + if (ret)
> + goto error_free_scan_el_attrs;
> +
> + return 0;
> +
> +error_free_buffer_attrs:
> + kfree(iio_dev_opaque->legacy_buffer_group.attrs);
> +error_free_scan_el_attrs:
> + kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> +
> + return ret;
> +}
> +
> +static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> + kfree(iio_dev_opaque->legacy_buffer_group.attrs);
> + kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> +}
> +
> static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> - struct iio_dev *indio_dev)
> + struct iio_dev *indio_dev,
> + int index)
> {
> struct iio_dev_attr *p;
> struct attribute **attr;
> @@ -1294,8 +1352,8 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> }
> }
>
> - attr = kcalloc(buffer_attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
> - sizeof(struct attribute *), GFP_KERNEL);
> + attrn = buffer_attrcount + scan_el_attrcount + ARRAY_SIZE(iio_buffer_attrs);
> + attr = kcalloc(attrn + 1, sizeof(struct attribute *), GFP_KERNEL);
> if (!attr) {
> ret = -ENOMEM;
> goto error_free_scan_mask;
> @@ -1313,37 +1371,38 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> sizeof(struct attribute *) * buffer_attrcount);
>
> buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs);
> - attr[buffer_attrcount] = NULL;
> -
> - buffer->buffer_group.name = "buffer";
> - buffer->buffer_group.attrs = attr;
>
> - ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
> - if (ret)
> - goto error_free_buffer_attrs;
> + attrn = buffer_attrcount;
>
> - buffer->scan_el_group.name = iio_scan_elements_group_name;
> + list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
> + attr[attrn++] = &p->dev_attr.attr;
>
> - buffer->scan_el_group.attrs = kcalloc(scan_el_attrcount + 1,
> - sizeof(buffer->scan_el_group.attrs[0]),
> - GFP_KERNEL);
> - if (buffer->scan_el_group.attrs == NULL) {
> + buffer->buffer_group.name = kasprintf(GFP_KERNEL, "buffer%d", index);
> + if (!buffer->buffer_group.name) {
> ret = -ENOMEM;
> - goto error_free_scan_mask;
> + goto error_free_buffer_attrs;
> }
> - attrn = 0;
>
> - list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
> - buffer->scan_el_group.attrs[attrn++] = &p->dev_attr.attr;
> + buffer->buffer_group.attrs = attr;
>
> - ret = iio_device_register_sysfs_group(indio_dev, &buffer->scan_el_group);
> + ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
> if (ret)
> - goto error_free_scan_el_attrs;
> + goto error_free_buffer_attr_group_name;
> +
> + /* we only need to link the legacy buffer groups for the first buffer */
Not linking as such any more so comment could do with an update.
> + if (index > 0)
> + return 0;
> +
> + ret = iio_buffer_register_legacy_sysfs_groups(indio_dev, attr,
> + buffer_attrcount,
> + scan_el_attrcount);
> + if (ret)
> + goto error_free_buffer_attr_group_name;
>
> return 0;
>
> -error_free_scan_el_attrs:
> - kfree(buffer->scan_el_group.attrs);
> +error_free_buffer_attr_group_name:
> + kfree(buffer->buffer_group.name);
> error_free_buffer_attrs:
> kfree(buffer->buffer_group.attrs);
> error_free_scan_mask:
> @@ -1372,14 +1431,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> if (!buffer)
> return 0;
>
> - return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
> + return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
> }
>
> static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> {
> bitmap_free(buffer->scan_mask);
> + kfree(buffer->buffer_group.name);
> kfree(buffer->buffer_group.attrs);
> - kfree(buffer->scan_el_group.attrs);
> iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> }
>
> @@ -1390,6 +1449,8 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> if (!buffer)
> return;
>
> + iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
> +
> __iio_buffer_free_sysfs_and_mask(buffer);
> }
>
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index a63dc07b7350..3e555e58475b 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -100,14 +100,11 @@ struct iio_buffer {
> /* @scan_el_dev_attr_list: List of scan element related attributes. */
> struct list_head scan_el_dev_attr_list;
>
> - /* @buffer_group: Attributes of the buffer group. */
> - struct attribute_group buffer_group;
> -
> /*
> - * @scan_el_group: Attribute group for those attributes not
> - * created from the iio_chan_info array.
> + * @buffer_group: Attributes of the new buffer group.
> + * Includes scan elements attributes.
> */
> - struct attribute_group scan_el_group;
> + struct attribute_group buffer_group;
>
> /* @attrs: Standard attributes of the buffer. */
> const struct attribute **attrs;
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> index 8ba13a5c7af6..3e4c3cd248fd 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -14,6 +14,8 @@
> * @ioctl_handlers: ioctl handlers registered with the core handler
> * @groups: attribute groups
> * @groupcounter: index of next attribute group
> + * @legacy_scan_el_group: attribute group for legacy scan elements attribute group
> + * @legacy_buffer_el_group: attribute group for legacy buffer attributes group
Wrong name
> * @debugfs_dentry: device specific debugfs dentry
> * @cached_reg_addr: cached register address for debugfs reads
> * @read_buf: read buffer to be used for the initial reg read
> @@ -28,6 +30,8 @@ struct iio_dev_opaque {
> struct list_head ioctl_handlers;
> const struct attribute_group **groups;
> int groupcounter;
> + struct attribute_group legacy_scan_el_group;
> + struct attribute_group legacy_buffer_group;
> #if defined(CONFIG_DEBUG_FS)
> struct dentry *debugfs_dentry;
> unsigned cached_reg_addr;
