From: Jonathan Cameron <jic23@kernel.org>
To: Alexandru Ardelean <alexandru.ardelean@analog.com>
Cc: <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<lars@metafoo.de>
Subject: Re: [RFC PATCH 06/12] iio: buffer: re-route scan_elements via it's kobj_type
Date: Sat, 21 Nov 2020 18:33:57 +0000 [thread overview]
Message-ID: <20201121183357.254682d5@archlinux> (raw)
In-Reply-To: <20201117162340.43924-7-alexandru.ardelean@analog.com>
On Tue, 17 Nov 2020 18:23:34 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> The scan_elements attributes are solely located inside
> 'industrialio-buffer-sysfs.c'. In order to support more than one buffer per
> IIO device, we need to expand scan_elements attributes directly to IIO
> buffer object, and not the IIO device.
>
> This also requires that a new 'iio_buffer_attr' type be added which is
> mostly a copy of 'iio_dev_attr', but this expands to an IIO buffer object.
>
> The 'iio_dev_attr' type could have been re-used here, but managing 'device'
> objects is a bit more tricky (than it looks at first). A 'device' object
> needs to be initialized & managed and we only need to the 'kobj' to expand
> from the 'bufferX' directory back to an IIO buffer.
> kobjects are simpler to manage.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
One trivial thing inline but otherwise looks fine to me.
Jonathan
> ---
> drivers/iio/iio_core.h | 5 +
> drivers/iio/industrialio-buffer.c | 162 +++++++++++++++++++++++-------
> drivers/iio/industrialio-core.c | 1 -
> 3 files changed, 129 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index fced02cadcc3..43d44ec92781 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -31,6 +31,11 @@ void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
> struct iio_ioctl_handler *h);
> void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
>
> +int iio_attr_init(struct attribute *attr,
> + const char *postfix,
> + struct iio_chan_spec const *chan,
> + enum iio_shared_by shared_by);
> +
> int __iio_add_chan_devattr(const char *postfix,
> struct iio_chan_spec const *chan,
> ssize_t (*func)(struct device *dev,
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 62c8bd6b67cd..445709ef245c 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -26,6 +26,26 @@
> #include <linux/iio/buffer.h>
> #include <linux/iio/buffer_impl.h>
>
> +/**
> + * struct iio_buf_attr - iio buffer specific attribute
> + * @attr: underlying attribute
> + * @address: associated register address
> + * @l: list head for maintaining list of dynamically created attrs
> + * @c: specification for the underlying channel
> + * @show: sysfs show hook for this attribute
> + * @store: sysfs store hook for this attribute
> + */
> +struct iio_buf_attr {
> + struct attribute attr;
> + u64 address;
> + struct list_head l;
> + struct iio_chan_spec const *c;
> + ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
> + char *buf);
> + ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
> + const char *buf, size_t count);
> +};
> +
> static const char * const iio_endian_prefix[] = {
> [IIO_BE] = "be",
> [IIO_LE] = "le",
> @@ -210,18 +230,17 @@ void iio_buffer_init(struct iio_buffer *buffer)
> }
> EXPORT_SYMBOL(iio_buffer_init);
>
> -static ssize_t iio_show_scan_index(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t iio_show_scan_index(struct iio_buffer *buffer,
> + struct iio_buf_attr *attr,
> char *buf)
> {
> - return sprintf(buf, "%u\n", to_iio_dev_attr(attr)->c->scan_index);
> + return sprintf(buf, "%u\n", attr->c->scan_index);
> }
>
> -static ssize_t iio_show_fixed_type(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t iio_show_fixed_type(struct iio_buffer *buffer,
> + struct iio_buf_attr *this_attr,
> char *buf)
> {
> - struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> u8 type = this_attr->c->scan_type.endianness;
>
> if (type == IIO_CPU) {
> @@ -248,17 +267,14 @@ static ssize_t iio_show_fixed_type(struct device *dev,
> this_attr->c->scan_type.shift);
> }
>
> -static ssize_t iio_scan_el_show(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t iio_scan_el_show(struct iio_buffer *buffer,
> + struct iio_buf_attr *attr,
> char *buf)
> {
> int ret;
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
>
> /* Ensure ret is 0 or 1. */
> - ret = !!test_bit(to_iio_dev_attr(attr)->address,
> - buffer->scan_mask);
> + ret = !!test_bit(attr->address, buffer->scan_mask);
>
> return sprintf(buf, "%d\n", ret);
> }
> @@ -359,16 +375,14 @@ static int iio_scan_mask_query(struct iio_dev *indio_dev,
> return !!test_bit(bit, buffer->scan_mask);
> };
>
> -static ssize_t iio_scan_el_store(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t iio_scan_el_store(struct iio_buffer *buffer,
> + struct iio_buf_attr *this_attr,
> const char *buf,
> size_t len)
> {
> + struct iio_dev *indio_dev = buffer->indio_dev;
> int ret;
> bool state;
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> - struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>
> ret = strtobool(buf, &state);
> if (ret < 0)
> @@ -398,24 +412,20 @@ static ssize_t iio_scan_el_store(struct device *dev,
>
> }
>
> -static ssize_t iio_scan_el_ts_show(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t iio_scan_el_ts_show(struct iio_buffer *buffer,
> + struct iio_buf_attr *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> -
> return sprintf(buf, "%d\n", buffer->scan_timestamp);
> }
>
> -static ssize_t iio_scan_el_ts_store(struct device *dev,
> - struct device_attribute *attr,
> +static ssize_t iio_scan_el_ts_store(struct iio_buffer *buffer,
> + struct iio_buf_attr *attr,
> const char *buf,
> size_t len)
> {
> + struct iio_dev *indio_dev = buffer->indio_dev;
> int ret;
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> bool state;
>
> ret = strtobool(buf, &state);
> @@ -434,13 +444,88 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
> return ret ? ret : len;
> }
>
> +static int __iio_add_chan_bufattr(const char *postfix,
> + struct iio_chan_spec const *chan,
> + ssize_t (*readfunc)(struct iio_buffer *buffer,
> + struct iio_buf_attr *attr,
> + char *buf),
> + ssize_t (*writefunc)(struct iio_buffer *buffer,
> + struct iio_buf_attr *attr,
> + const char *buf,
> + size_t len),
> + u64 mask,
> + enum iio_shared_by shared_by,
> + struct device *dev,
> + struct list_head *attr_list)
> +{
> + struct iio_buf_attr *iio_attr, *t;
> + int ret;
> +
> + iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
> + if (iio_attr == NULL)
> + return -ENOMEM;
> +
> + ret = iio_attr_init(&iio_attr->attr, postfix, chan, shared_by);
> + if (ret)
> + goto error_iio_buf_attr_free;
> +
> + iio_attr->c = chan;
> + iio_attr->address = mask;
> + list_for_each_entry(t, attr_list, l) {
> + if (strcmp(t->attr.name, iio_attr->attr.name) == 0) {
> + if (shared_by == IIO_SEPARATE)
> + dev_err(dev, "tried to double register : %s\n",
> + t->attr.name);
> + ret = -EBUSY;
> + goto error_iio_buf_attr_deinit;
> + }
> + }
> + list_add(&iio_attr->l, attr_list);
> +
> + if (readfunc) {
> + iio_attr->attr.mode |= S_IRUGO;
> + iio_attr->show = readfunc;
> + }
> +
> + if (writefunc) {
> + iio_attr->attr.mode |= S_IWUSR;
> + iio_attr->store = writefunc;
> + }
> +
> + return 0;
> +
> +error_iio_buf_attr_deinit:
> + kfree(iio_attr->attr.name);
> +error_iio_buf_attr_free:
> + kfree(iio_attr);
> + return ret;
> +}
> +
> +/**
> + * iio_free_chan_bufattr_list() - Free a list of IIO buffer attributes
> + * @attr_list: List of IIO buffer attributes
> + *
> + * This function frees the memory allocated for each of the IIO buffer
> + * attributes in the list.
> + */
> +static void iio_free_chan_bufattr_list(struct list_head *attr_list)
> +{
> + struct iio_buf_attr *p, *n;
> +
> + list_for_each_entry_safe(p, n, attr_list, l) {
> + kfree(p->attr.name);
Trivial but ordering in here seems a bit odd. Take it off
list before doing the name free makes more sense to me.
> + list_del(&p->l);
> + kfree(p);
> + }
> +}
> +
> static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> struct iio_buffer *buffer,
> const struct iio_chan_spec *chan)
> {
> int ret, attrcount = 0;
>
> - ret = __iio_add_chan_devattr("index",
> + ret = __iio_add_chan_bufattr("index",
> chan,
> &iio_show_scan_index,
> NULL,
> @@ -451,7 +536,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> if (ret)
> return ret;
> attrcount++;
> - ret = __iio_add_chan_devattr("type",
> + ret = __iio_add_chan_bufattr("type",
> chan,
> &iio_show_fixed_type,
> NULL,
> @@ -463,7 +548,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> return ret;
> attrcount++;
> if (chan->type != IIO_TIMESTAMP)
> - ret = __iio_add_chan_devattr("en",
> + ret = __iio_add_chan_bufattr("en",
> chan,
> &iio_scan_el_show,
> &iio_scan_el_store,
> @@ -472,7 +557,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> &indio_dev->dev,
> &buffer->scan_el_dev_attr_list);
> else
> - ret = __iio_add_chan_devattr("en",
> + ret = __iio_add_chan_bufattr("en",
> chan,
> &iio_scan_el_ts_show,
> &iio_scan_el_ts_store,
> @@ -1251,6 +1336,7 @@ static struct attribute *iio_buffer_attrs[] = {
> };
>
> #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> +#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
>
> static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> struct attribute *attr,
> @@ -1291,9 +1377,9 @@ static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
> char *buf)
> {
> struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> - struct device_attribute *dattr = to_dev_attr(attr);
> + struct iio_buf_attr *battr = to_iio_buf_attr(attr);
>
> - return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> + return battr->show(buffer, battr, buf);
> }
>
> static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> @@ -1302,9 +1388,9 @@ static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> size_t len)
> {
> struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> - struct device_attribute *dattr = to_dev_attr(attr);
> + struct iio_buf_attr *battr = to_iio_buf_attr(attr);
>
> - return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> + return battr->store(buffer, battr, buf, len);
> }
>
> static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
> @@ -1349,7 +1435,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> struct iio_dev *indio_dev,
> unsigned int idx)
> {
> - struct iio_dev_attr *p;
> + struct iio_buf_attr *p;
> struct attribute **attr;
> int ret, i, attrn, attrcount;
> const struct iio_chan_spec *channels;
> @@ -1430,7 +1516,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>
> attrn = 0;
> list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
> - buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
> + buffer->scan_el_attrs[attrn++] = &p->attr;
>
> ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> if (ret)
> @@ -1446,7 +1532,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> bitmap_free(buffer->scan_mask);
> error_cleanup_dynamic:
> iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> + iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
> error_buffer_kobject_put:
> kobject_put(&buffer->buffer_dir);
> error_buffer_free_attrs:
> @@ -1507,7 +1593,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> kfree(buffer->scan_el_attrs);
> bitmap_free(buffer->scan_mask);
> iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> - iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> + iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
> kobject_put(&buffer->buffer_dir);
> kfree(buffer->buffer_attrs);
> }
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index e9aa84f5b05a..28830e87e8cb 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -972,7 +972,6 @@ static ssize_t iio_write_channel_info(struct device *dev,
> return len;
> }
>
> -static
> int iio_attr_init(struct attribute *attr,
> const char *postfix,
> struct iio_chan_spec const *chan,
next prev parent reply other threads:[~2020-11-21 18:34 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-17 16:23 [RFC PATCH 00/12] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 01/12] iio: core: register chardev only if needed Alexandru Ardelean
2020-11-21 18:02 ` Jonathan Cameron
2020-11-23 12:10 ` Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 02/12] iio: buffer: add back-ref from iio_buffer to iio_dev Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 03/12] iio: buffer: rework buffer & scan_elements dir creation Alexandru Ardelean
2020-11-21 18:24 ` Jonathan Cameron
2020-11-23 12:21 ` Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 04/12] iio: buffer: add index to the first IIO buffer dir and symlink it back Alexandru Ardelean
2020-11-21 18:27 ` Jonathan Cameron
2020-11-17 16:23 ` [RFC PATCH 05/12] iio: core: split __iio_device_attr_init() to init only the attr object Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 06/12] iio: buffer: re-route scan_elements via it's kobj_type Alexandru Ardelean
2020-11-21 18:33 ` Jonathan Cameron [this message]
2020-11-23 12:22 ` Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 07/12] iio: buffer: re-route core buffer attributes via it's new kobj_type Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 08/12] iio: buffer: add helper to get the IIO device to which a buffer belongs Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 09/12] iio: re-route all buffer attributes through new buffer kobj_type Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 10/12] iio: core: wrap iio device & buffer into struct for character devices Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 11/12] iio: buffer: introduce support for attaching more IIO buffers Alexandru Ardelean
2020-11-21 18:44 ` Jonathan Cameron
2020-11-23 12:27 ` Alexandru Ardelean
2020-11-17 16:23 ` [RFC PATCH 12/12] iio: buffer: add ioctl() to support opening extra buffers for IIO device Alexandru Ardelean
2020-11-21 18:50 ` Jonathan Cameron
2020-11-23 12:54 ` Alexandru Ardelean
2020-11-21 18:52 ` [RFC PATCH 00/12] iio: core,buffer: add support for multiple IIO buffers per " Jonathan Cameron
2020-11-23 13:03 ` Alexandru Ardelean
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201121183357.254682d5@archlinux \
--to=jic23@kernel.org \
--cc=alexandru.ardelean@analog.com \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).