2020-04-29 15:21:21

by Alexandru Ardelean

Subject: [PATCH v2 1/2] iio: Move scan mask management to the core

From: Lars-Peter Clausen <[email protected]>

Let the core handle the buffer scan mask management including allocation
and channel selection. Having this handled in a central place rather than
open-coding it all over the place will make it easier to change the
implementation (if needed).
At the very least, this change abstracts scan-mask management away from
buffer implementations.

Signed-off-by: Lars-Peter Clausen <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---

Changelog v1 -> v2:
- split away from initial series; the `buffer->channel_mask` attribute
requires a bit more dicussion; or may even be dropped; just these 2
patches helps with diff-ing 2 trees, as applying patches between my
work tree & IIO has fewer conflicts
- return -EINVAL if masklength is 0 in iio_buffer_alloc_scanmask()
- convert 2nd parameter to `unsigned int masklength` in
iio_buffer_alloc_scanmask()

drivers/iio/buffer/industrialio-buffer-cb.c | 17 +++++-----
drivers/iio/industrialio-buffer.c | 36 +++++++++++++++------
drivers/iio/inkern.c | 15 +++++++++
include/linux/iio/consumer.h | 10 ++++++
4 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
index 47c96f7f4976..dc5bb2ab533a 100644
--- a/drivers/iio/buffer/industrialio-buffer-cb.c
+++ b/drivers/iio/buffer/industrialio-buffer-cb.c
@@ -34,7 +34,7 @@ static void iio_buffer_cb_release(struct iio_buffer *buffer)
{
struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);

- bitmap_free(cb_buff->buffer.scan_mask);
+ iio_buffer_free_scanmask(&cb_buff->buffer);
kfree(cb_buff);
}

@@ -72,27 +72,26 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
}

cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
- cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev->masklength,
- GFP_KERNEL);
- if (cb_buff->buffer.scan_mask == NULL) {
- ret = -ENOMEM;
+
+ ret = iio_buffer_alloc_scanmask(&cb_buff->buffer,
+ cb_buff->indio_dev->masklength);
+ if (ret)
goto error_release_channels;
- }
+
chan = &cb_buff->channels[0];
while (chan->indio_dev) {
if (chan->indio_dev != cb_buff->indio_dev) {
ret = -EINVAL;
goto error_free_scan_mask;
}
- set_bit(chan->channel->scan_index,
- cb_buff->buffer.scan_mask);
+ iio_buffer_channel_enable(&cb_buff->buffer, chan);
chan++;
}

return cb_buff;

error_free_scan_mask:
- bitmap_free(cb_buff->buffer.scan_mask);
+ iio_buffer_free_scanmask(&cb_buff->buffer);
error_release_channels:
iio_channel_release_all(cb_buff->channels);
error_free_cb_buff:
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index eae39eaf49af..c6b63f4474ff 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -208,6 +208,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
}
EXPORT_SYMBOL(iio_buffer_init);

+int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
+ unsigned int masklength)
+{
+ if (!masklength)
+ return -EINVAL;
+
+ buffer->scan_mask = bitmap_zalloc(masklength, GFP_KERNEL);
+ if (buffer->scan_mask == NULL)
+ return -ENOMEM;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
+
+void iio_buffer_free_scanmask(struct iio_buffer *buffer)
+{
+ bitmap_free(buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
+
/**
* iio_buffer_set_attrs - Set buffer specific attributes
* @buffer: The buffer for which we are setting attributes
@@ -1306,14 +1326,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
indio_dev->scan_index_timestamp =
channels[i].scan_index;
}
- if (indio_dev->masklength && buffer->scan_mask == NULL) {
- buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
- GFP_KERNEL);
- if (buffer->scan_mask == NULL) {
- ret = -ENOMEM;
- goto error_cleanup_dynamic;
- }
- }
+
+ ret = iio_buffer_alloc_scanmask(buffer, indio_dev->masklength);
+ if (ret)
+ goto error_cleanup_dynamic;
}

buffer->scan_el_group.name = iio_scan_elements_group_name;
@@ -1334,7 +1350,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
return 0;

error_free_scan_mask:
- bitmap_free(buffer->scan_mask);
+ iio_buffer_free_scanmask(buffer);
error_cleanup_dynamic:
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
kfree(buffer->buffer_group.attrs);
@@ -1349,7 +1365,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
if (!buffer)
return;

- bitmap_free(buffer->scan_mask);
+ iio_buffer_free_scanmask(buffer);
kfree(buffer->buffer_group.attrs);
kfree(buffer->scan_el_group.attrs);
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index ede99e0d5371..f35cb9985edc 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -11,6 +11,7 @@

#include <linux/iio/iio.h>
#include "iio_core.h"
+#include <linux/iio/buffer_impl.h>
#include <linux/iio/machine.h>
#include <linux/iio/driver.h>
#include <linux/iio/consumer.h>
@@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
}
EXPORT_SYMBOL_GPL(iio_write_channel_raw);

+void iio_buffer_channel_enable(struct iio_buffer *buffer,
+ const struct iio_channel *chan)
+{
+ set_bit(chan->channel->scan_index, buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
+
+void iio_buffer_channel_disable(struct iio_buffer *buffer,
+ const struct iio_channel *chan)
+{
+ clear_bit(chan->channel->scan_index, buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
+
unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
{
const struct iio_chan_spec_ext_info *ext_info;
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index c4118dcb8e05..9fcd320c2fb4 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -12,6 +12,7 @@

struct iio_dev;
struct iio_chan_spec;
+struct iio_buffer;
struct device;

/**
@@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
int *processed, unsigned int scale);

+void iio_buffer_channel_enable(struct iio_buffer *buffer,
+ const struct iio_channel *chan);
+void iio_buffer_channel_disable(struct iio_buffer *buffer,
+ const struct iio_channel *chan);
+
+int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
+ unsigned int masklength);
+void iio_buffer_free_scanmask(struct iio_buffer *buffer);
+
/**
* iio_get_channel_ext_info_count() - get number of ext_info attributes
* connected to the channel.
--
2.17.1

2020-05-03 12:53:31

by Jonathan Cameron

Subject: Re: [PATCH v2 1/2] iio: Move scan mask management to the core

On Wed, 29 Apr 2020 18:17:39 +0300
Alexandru Ardelean <[email protected]> wrote:

> From: Lars-Peter Clausen <[email protected]>
>
> Let the core handle the buffer scan mask management including allocation
> and channel selection. Having this handled in a central place rather than
> open-coding it all over the place will make it easier to change the
> implementation (if needed).
> At the very least, this change abstracts scan-mask management away from
> buffer implementations.
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> Signed-off-by: Alexandru Ardelean <[email protected]>

I'm not 100% happy with including the buffer_impl.h header in the inkern
code, but I can't see a simple way around it.

However, there are some missing statics in here and I'm feeling lazy
so not going to fix them up for you.

Thanks,

Jonathan

> ---
>
> Changelog v1 -> v2:
> - split away from initial series; the `buffer->channel_mask` attribute
> requires a bit more dicussion; or may even be dropped; just these 2
> patches helps with diff-ing 2 trees, as applying patches between my
> work tree & IIO has fewer conflicts
> - return -EINVAL if masklength is 0 in iio_buffer_alloc_scanmask()
> - convert 2nd parameter to `unsigned int masklength` in
> iio_buffer_alloc_scanmask()
>
> drivers/iio/buffer/industrialio-buffer-cb.c | 17 +++++-----
> drivers/iio/industrialio-buffer.c | 36 +++++++++++++++------
> drivers/iio/inkern.c | 15 +++++++++
> include/linux/iio/consumer.h | 10 ++++++
> 4 files changed, 59 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
> index 47c96f7f4976..dc5bb2ab533a 100644
> --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> @@ -34,7 +34,7 @@ static void iio_buffer_cb_release(struct iio_buffer *buffer)
> {
> struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
>
> - bitmap_free(cb_buff->buffer.scan_mask);
> + iio_buffer_free_scanmask(&cb_buff->buffer);
> kfree(cb_buff);
> }
>
> @@ -72,27 +72,26 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
> }
>
> cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
> - cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev->masklength,
> - GFP_KERNEL);
> - if (cb_buff->buffer.scan_mask == NULL) {
> - ret = -ENOMEM;
> +
> + ret = iio_buffer_alloc_scanmask(&cb_buff->buffer,
> + cb_buff->indio_dev->masklength);
> + if (ret)
> goto error_release_channels;
> - }
> +
> chan = &cb_buff->channels[0];
> while (chan->indio_dev) {
> if (chan->indio_dev != cb_buff->indio_dev) {
> ret = -EINVAL;
> goto error_free_scan_mask;
> }
> - set_bit(chan->channel->scan_index,
> - cb_buff->buffer.scan_mask);
> + iio_buffer_channel_enable(&cb_buff->buffer, chan);
> chan++;
> }
>
> return cb_buff;
>
> error_free_scan_mask:
> - bitmap_free(cb_buff->buffer.scan_mask);
> + iio_buffer_free_scanmask(&cb_buff->buffer);
> error_release_channels:
> iio_channel_release_all(cb_buff->channels);
> error_free_cb_buff:
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index eae39eaf49af..c6b63f4474ff 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -208,6 +208,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
> }
> EXPORT_SYMBOL(iio_buffer_init);
>
> +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> + unsigned int masklength)
> +{
> + if (!masklength)
> + return -EINVAL;
> +
> + buffer->scan_mask = bitmap_zalloc(masklength, GFP_KERNEL);
> + if (buffer->scan_mask == NULL)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> +
> +void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> +{
> + bitmap_free(buffer->scan_mask);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> +
> /**
> * iio_buffer_set_attrs - Set buffer specific attributes
> * @buffer: The buffer for which we are setting attributes
> @@ -1306,14 +1326,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> indio_dev->scan_index_timestamp =
> channels[i].scan_index;
> }
> - if (indio_dev->masklength && buffer->scan_mask == NULL) {
> - buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
> - GFP_KERNEL);
> - if (buffer->scan_mask == NULL) {
> - ret = -ENOMEM;
> - goto error_cleanup_dynamic;
> - }
> - }
> +
> + ret = iio_buffer_alloc_scanmask(buffer, indio_dev->masklength);
> + if (ret)
> + goto error_cleanup_dynamic;
> }
>
> buffer->scan_el_group.name = iio_scan_elements_group_name;
> @@ -1334,7 +1350,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> return 0;
>
> error_free_scan_mask:
> - bitmap_free(buffer->scan_mask);
> + iio_buffer_free_scanmask(buffer);
> error_cleanup_dynamic:
> iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> kfree(buffer->buffer_group.attrs);
> @@ -1349,7 +1365,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> if (!buffer)
> return;
>
> - bitmap_free(buffer->scan_mask);
> + iio_buffer_free_scanmask(buffer);
> kfree(buffer->buffer_group.attrs);
> kfree(buffer->scan_el_group.attrs);
> iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index ede99e0d5371..f35cb9985edc 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -11,6 +11,7 @@
>
> #include <linux/iio/iio.h>
> #include "iio_core.h"
> +#include <linux/iio/buffer_impl.h>
> #include <linux/iio/machine.h>
> #include <linux/iio/driver.h>
> #include <linux/iio/consumer.h>
> @@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
> }
> EXPORT_SYMBOL_GPL(iio_write_channel_raw);
>
> +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> + const struct iio_channel *chan)
> +{
> + set_bit(chan->channel->scan_index, buffer->scan_mask);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
> +
> +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> + const struct iio_channel *chan)
> +{
> + clear_bit(chan->channel->scan_index, buffer->scan_mask);
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
> +
> unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
> {
> const struct iio_chan_spec_ext_info *ext_info;
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index c4118dcb8e05..9fcd320c2fb4 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -12,6 +12,7 @@
>
> struct iio_dev;
> struct iio_chan_spec;
> +struct iio_buffer;
> struct device;
>
> /**
> @@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
> int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
> int *processed, unsigned int scale);
>
> +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> + const struct iio_channel *chan);
> +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> + const struct iio_channel *chan);
> +
> +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> + unsigned int masklength);
> +void iio_buffer_free_scanmask(struct iio_buffer *buffer);
> +
> /**
> * iio_get_channel_ext_info_count() - get number of ext_info attributes
> * connected to the channel.

2020-05-04 06:49:52

by Alexandru Ardelean

Subject: Re: [PATCH v2 1/2] iio: Move scan mask management to the core

On Sun, 2020-05-03 at 13:51 +0100, Jonathan Cameron wrote:
> On Wed, 29 Apr 2020 18:17:39 +0300
> Alexandru Ardelean <[email protected]> wrote:
>
> > From: Lars-Peter Clausen <[email protected]>
> >
> > Let the core handle the buffer scan mask management including allocation
> > and channel selection. Having this handled in a central place rather than
> > open-coding it all over the place will make it easier to change the
> > implementation (if needed).
> > At the very least, this change abstracts scan-mask management away from
> > buffer implementations.
> >
> > Signed-off-by: Lars-Peter Clausen <[email protected]>
> > Signed-off-by: Alexandru Ardelean <[email protected]>
>
> I'm not 100% happy with including the buffer_impl.h header in the inkern
> code, but I can't see a simple way around it.

I actually also tried to not do it [I suspect Lars may have tried as well].
But you end up either having to expose 'struct iio_channel' outside of
'inkern.c' or having to include 'iio/consumer.h' in industrialio-buffer.c

Since there will be a V3, I will probably try to look again at a potential
different solution. Maybe there is a better solution I haven't considered.

>
> However, there are some missing statics in here and I'm feeling lazy
> so not going to fix them up for you.
>

No problem from my side.
I'll fix them up.
I prefer it when people don't cleanup after me. Makes me feel less guilty.
Though I will admit, it's a double-edged feeling: I am greatful when someone
cleans up after me, but I also feel slightly guilty [about it].


> Thanks,
>
> Jonathan
>
> > ---
> >
> > Changelog v1 -> v2:
> > - split away from initial series; the `buffer->channel_mask` attribute
> > requires a bit more dicussion; or may even be dropped; just these 2
> > patches helps with diff-ing 2 trees, as applying patches between my
> > work tree & IIO has fewer conflicts
> > - return -EINVAL if masklength is 0 in iio_buffer_alloc_scanmask()
> > - convert 2nd parameter to `unsigned int masklength` in
> > iio_buffer_alloc_scanmask()
> >
> > drivers/iio/buffer/industrialio-buffer-cb.c | 17 +++++-----
> > drivers/iio/industrialio-buffer.c | 36 +++++++++++++++------
> > drivers/iio/inkern.c | 15 +++++++++
> > include/linux/iio/consumer.h | 10 ++++++
> > 4 files changed, 59 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c
> > b/drivers/iio/buffer/industrialio-buffer-cb.c
> > index 47c96f7f4976..dc5bb2ab533a 100644
> > --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> > +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> > @@ -34,7 +34,7 @@ static void iio_buffer_cb_release(struct iio_buffer
> > *buffer)
> > {
> > struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> >
> > - bitmap_free(cb_buff->buffer.scan_mask);
> > + iio_buffer_free_scanmask(&cb_buff->buffer);
> > kfree(cb_buff);
> > }
> >
> > @@ -72,27 +72,26 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct
> > device *dev,
> > }
> >
> > cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
> > - cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev-
> > >masklength,
> > - GFP_KERNEL);
> > - if (cb_buff->buffer.scan_mask == NULL) {
> > - ret = -ENOMEM;
> > +
> > + ret = iio_buffer_alloc_scanmask(&cb_buff->buffer,
> > + cb_buff->indio_dev->masklength);
> > + if (ret)
> > goto error_release_channels;
> > - }
> > +
> > chan = &cb_buff->channels[0];
> > while (chan->indio_dev) {
> > if (chan->indio_dev != cb_buff->indio_dev) {
> > ret = -EINVAL;
> > goto error_free_scan_mask;
> > }
> > - set_bit(chan->channel->scan_index,
> > - cb_buff->buffer.scan_mask);
> > + iio_buffer_channel_enable(&cb_buff->buffer, chan);
> > chan++;
> > }
> >
> > return cb_buff;
> >
> > error_free_scan_mask:
> > - bitmap_free(cb_buff->buffer.scan_mask);
> > + iio_buffer_free_scanmask(&cb_buff->buffer);
> > error_release_channels:
> > iio_channel_release_all(cb_buff->channels);
> > error_free_cb_buff:
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index eae39eaf49af..c6b63f4474ff 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -208,6 +208,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
> > }
> > EXPORT_SYMBOL(iio_buffer_init);
> >
> > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > + unsigned int masklength)
> > +{
> > + if (!masklength)
> > + return -EINVAL;
> > +
> > + buffer->scan_mask = bitmap_zalloc(masklength, GFP_KERNEL);
> > + if (buffer->scan_mask == NULL)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > +
> > +void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > +{
> > + bitmap_free(buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > +
> > /**
> > * iio_buffer_set_attrs - Set buffer specific attributes
> > * @buffer: The buffer for which we are setting attributes
> > @@ -1306,14 +1326,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> > indio_dev->scan_index_timestamp =
> > channels[i].scan_index;
> > }
> > - if (indio_dev->masklength && buffer->scan_mask == NULL) {
> > - buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
> > - GFP_KERNEL);
> > - if (buffer->scan_mask == NULL) {
> > - ret = -ENOMEM;
> > - goto error_cleanup_dynamic;
> > - }
> > - }
> > +
> > + ret = iio_buffer_alloc_scanmask(buffer, indio_dev->masklength);
> > + if (ret)
> > + goto error_cleanup_dynamic;
> > }
> >
> > buffer->scan_el_group.name = iio_scan_elements_group_name;
> > @@ -1334,7 +1350,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> > return 0;
> >
> > error_free_scan_mask:
> > - bitmap_free(buffer->scan_mask);
> > + iio_buffer_free_scanmask(buffer);
> > error_cleanup_dynamic:
> > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > kfree(buffer->buffer_group.attrs);
> > @@ -1349,7 +1365,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> > if (!buffer)
> > return;
> >
> > - bitmap_free(buffer->scan_mask);
> > + iio_buffer_free_scanmask(buffer);
> > kfree(buffer->buffer_group.attrs);
> > kfree(buffer->scan_el_group.attrs);
> > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index ede99e0d5371..f35cb9985edc 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -11,6 +11,7 @@
> >
> > #include <linux/iio/iio.h>
> > #include "iio_core.h"
> > +#include <linux/iio/buffer_impl.h>
> > #include <linux/iio/machine.h>
> > #include <linux/iio/driver.h>
> > #include <linux/iio/consumer.h>
> > @@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int
> > val)
> > }
> > EXPORT_SYMBOL_GPL(iio_write_channel_raw);
> >
> > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > + const struct iio_channel *chan)
> > +{
> > + set_bit(chan->channel->scan_index, buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
> > +
> > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > + const struct iio_channel *chan)
> > +{
> > + clear_bit(chan->channel->scan_index, buffer->scan_mask);
> > +}
> > +EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
> > +
> > unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
> > {
> > const struct iio_chan_spec_ext_info *ext_info;
> > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> > index c4118dcb8e05..9fcd320c2fb4 100644
> > --- a/include/linux/iio/consumer.h
> > +++ b/include/linux/iio/consumer.h
> > @@ -12,6 +12,7 @@
> >
> > struct iio_dev;
> > struct iio_chan_spec;
> > +struct iio_buffer;
> > struct device;
> >
> > /**
> > @@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan,
> > int *val,
> > int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
> > int *processed, unsigned int scale);
> >
> > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > + const struct iio_channel *chan);
> > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > + const struct iio_channel *chan);
> > +
> > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > + unsigned int masklength);
> > +void iio_buffer_free_scanmask(struct iio_buffer *buffer);
> > +
> > /**
> > * iio_get_channel_ext_info_count() - get number of ext_info attributes
> > * connected to the channel.

2020-05-04 09:48:34

by Jonathan Cameron

Subject: Re: [PATCH v2 1/2] iio: Move scan mask management to the core

On Mon, 4 May 2020 06:45:27 +0000
"Ardelean, Alexandru" <[email protected]> wrote:

> On Sun, 2020-05-03 at 13:51 +0100, Jonathan Cameron wrote:
> > On Wed, 29 Apr 2020 18:17:39 +0300
> > Alexandru Ardelean <[email protected]> wrote:
> >
> > > From: Lars-Peter Clausen <[email protected]>
> > >
> > > Let the core handle the buffer scan mask management including allocation
> > > and channel selection. Having this handled in a central place rather than
> > > open-coding it all over the place will make it easier to change the
> > > implementation (if needed).
> > > At the very least, this change abstracts scan-mask management away from
> > > buffer implementations.
> > >
> > > Signed-off-by: Lars-Peter Clausen <[email protected]>
> > > Signed-off-by: Alexandru Ardelean <[email protected]>
> >
> > I'm not 100% happy with including the buffer_impl.h header in the inkern
> > code, but I can't see a simple way around it.
>
> I actually also tried to not do it [I suspect Lars may have tried as well].
> But you end up either having to expose 'struct iio_channel' outside of
> 'inkern.c' or having to include 'iio/consumer.h' in industrialio-buffer.c
>
> Since there will be a V3, I will probably try to look again at a potential
> different solution. Maybe there is a better solution I haven't considered.
>
> >
> > However, there are some missing statics in here and I'm feeling lazy
> > so not going to fix them up for you.
> >
>
> No problem from my side.
> I'll fix them up.
> I prefer it when people don't cleanup after me. Makes me feel less guilty.
> Though I will admit, it's a double-edged feeling: I am greatful when someone
> cleans up after me, but I also feel slightly guilty [about it].
>

*laughs*. It's a trade off for the patch applier between

1) Making people feel guilty so they are more careful in future.
2) Effort to actually make trivial fixes.
3) Having to get their brain back in gear when they get around to looking
at the next version.

Mostly 3 wins out though I like to go with 1 every now and then when
I'm feeling grump. Warning for all, lock down makes me grumpy so
you may all suffer the effects :)

J

>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > >
> > > Changelog v1 -> v2:
> > > - split away from initial series; the `buffer->channel_mask` attribute
> > > requires a bit more dicussion; or may even be dropped; just these 2
> > > patches helps with diff-ing 2 trees, as applying patches between my
> > > work tree & IIO has fewer conflicts
> > > - return -EINVAL if masklength is 0 in iio_buffer_alloc_scanmask()
> > > - convert 2nd parameter to `unsigned int masklength` in
> > > iio_buffer_alloc_scanmask()
> > >
> > > drivers/iio/buffer/industrialio-buffer-cb.c | 17 +++++-----
> > > drivers/iio/industrialio-buffer.c | 36 +++++++++++++++------
> > > drivers/iio/inkern.c | 15 +++++++++
> > > include/linux/iio/consumer.h | 10 ++++++
> > > 4 files changed, 59 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c
> > > b/drivers/iio/buffer/industrialio-buffer-cb.c
> > > index 47c96f7f4976..dc5bb2ab533a 100644
> > > --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> > > +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> > > @@ -34,7 +34,7 @@ static void iio_buffer_cb_release(struct iio_buffer
> > > *buffer)
> > > {
> > > struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
> > >
> > > - bitmap_free(cb_buff->buffer.scan_mask);
> > > + iio_buffer_free_scanmask(&cb_buff->buffer);
> > > kfree(cb_buff);
> > > }
> > >
> > > @@ -72,27 +72,26 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct
> > > device *dev,
> > > }
> > >
> > > cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
> > > - cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev-
> > > >masklength,
> > > - GFP_KERNEL);
> > > - if (cb_buff->buffer.scan_mask == NULL) {
> > > - ret = -ENOMEM;
> > > +
> > > + ret = iio_buffer_alloc_scanmask(&cb_buff->buffer,
> > > + cb_buff->indio_dev->masklength);
> > > + if (ret)
> > > goto error_release_channels;
> > > - }
> > > +
> > > chan = &cb_buff->channels[0];
> > > while (chan->indio_dev) {
> > > if (chan->indio_dev != cb_buff->indio_dev) {
> > > ret = -EINVAL;
> > > goto error_free_scan_mask;
> > > }
> > > - set_bit(chan->channel->scan_index,
> > > - cb_buff->buffer.scan_mask);
> > > + iio_buffer_channel_enable(&cb_buff->buffer, chan);
> > > chan++;
> > > }
> > >
> > > return cb_buff;
> > >
> > > error_free_scan_mask:
> > > - bitmap_free(cb_buff->buffer.scan_mask);
> > > + iio_buffer_free_scanmask(&cb_buff->buffer);
> > > error_release_channels:
> > > iio_channel_release_all(cb_buff->channels);
> > > error_free_cb_buff:
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > > buffer.c
> > > index eae39eaf49af..c6b63f4474ff 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -208,6 +208,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
> > > }
> > > EXPORT_SYMBOL(iio_buffer_init);
> > >
> > > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > > + unsigned int masklength)
> > > +{
> > > + if (!masklength)
> > > + return -EINVAL;
> > > +
> > > + buffer->scan_mask = bitmap_zalloc(masklength, GFP_KERNEL);
> > > + if (buffer->scan_mask == NULL)
> > > + return -ENOMEM;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> > > +
> > > +void iio_buffer_free_scanmask(struct iio_buffer *buffer)
> > > +{
> > > + bitmap_free(buffer->scan_mask);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> > > +
> > > /**
> > > * iio_buffer_set_attrs - Set buffer specific attributes
> > > * @buffer: The buffer for which we are setting attributes
> > > @@ -1306,14 +1326,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > > indio_dev->scan_index_timestamp =
> > > channels[i].scan_index;
> > > }
> > > - if (indio_dev->masklength && buffer->scan_mask == NULL) {
> > > - buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
> > > - GFP_KERNEL);
> > > - if (buffer->scan_mask == NULL) {
> > > - ret = -ENOMEM;
> > > - goto error_cleanup_dynamic;
> > > - }
> > > - }
> > > +
> > > + ret = iio_buffer_alloc_scanmask(buffer, indio_dev->masklength);
> > > + if (ret)
> > > + goto error_cleanup_dynamic;
> > > }
> > >
> > > buffer->scan_el_group.name = iio_scan_elements_group_name;
> > > @@ -1334,7 +1350,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > > return 0;
> > >
> > > error_free_scan_mask:
> > > - bitmap_free(buffer->scan_mask);
> > > + iio_buffer_free_scanmask(buffer);
> > > error_cleanup_dynamic:
> > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > > kfree(buffer->buffer_group.attrs);
> > > @@ -1349,7 +1365,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > > if (!buffer)
> > > return;
> > >
> > > - bitmap_free(buffer->scan_mask);
> > > + iio_buffer_free_scanmask(buffer);
> > > kfree(buffer->buffer_group.attrs);
> > > kfree(buffer->scan_el_group.attrs);
> > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index ede99e0d5371..f35cb9985edc 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -11,6 +11,7 @@
> > >
> > > #include <linux/iio/iio.h>
> > > #include "iio_core.h"
> > > +#include <linux/iio/buffer_impl.h>
> > > #include <linux/iio/machine.h>
> > > #include <linux/iio/driver.h>
> > > #include <linux/iio/consumer.h>
> > > @@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int
> > > val)
> > > }
> > > EXPORT_SYMBOL_GPL(iio_write_channel_raw);
> > >
> > > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > > + const struct iio_channel *chan)
> > > +{
> > > + set_bit(chan->channel->scan_index, buffer->scan_mask);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
> > > +
> > > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > > + const struct iio_channel *chan)
> > > +{
> > > + clear_bit(chan->channel->scan_index, buffer->scan_mask);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
> > > +
> > > unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
> > > {
> > > const struct iio_chan_spec_ext_info *ext_info;
> > > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> > > index c4118dcb8e05..9fcd320c2fb4 100644
> > > --- a/include/linux/iio/consumer.h
> > > +++ b/include/linux/iio/consumer.h
> > > @@ -12,6 +12,7 @@
> > >
> > > struct iio_dev;
> > > struct iio_chan_spec;
> > > +struct iio_buffer;
> > > struct device;
> > >
> > > /**
> > > @@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan,
> > > int *val,
> > > int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
> > > int *processed, unsigned int scale);
> > >
> > > +void iio_buffer_channel_enable(struct iio_buffer *buffer,
> > > + const struct iio_channel *chan);
> > > +void iio_buffer_channel_disable(struct iio_buffer *buffer,
> > > + const struct iio_channel *chan);
> > > +
> > > +int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
> > > + unsigned int masklength);
> > > +void iio_buffer_free_scanmask(struct iio_buffer *buffer);
> > > +
> > > /**
> > > * iio_get_channel_ext_info_count() - get number of ext_info attributes
> > > * connected to the channel.