diff mbox series

iio: buffer: split buffer sysfs creation to take buffer as primary arg

Message ID 20200917125951.861-1-alexandru.ardelean@analog.com
State New
Headers show
Series iio: buffer: split buffer sysfs creation to take buffer as primary arg | expand

Commit Message

Alexandru Ardelean Sept. 17, 2020, 12:59 p.m. UTC
Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
primary argument. This change splits the main logic into a private function
that takes an IIO buffer as primary argument.

That way, the functions can be extended to configure the sysfs for multiple
buffers.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 16 deletions(-)

Comments

Jonathan Cameron Sept. 17, 2020, 5:16 p.m. UTC | #1
On Thu, 17 Sep 2020 15:59:51 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
> primary argument. This change splits the main logic into a private function
> that takes an IIO buffer as primary argument.
> 
> That way, the functions can be extended to configure the sysfs for multiple
> buffers.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

One comment inline.  Whilst I think it is safe as you have it, I'd
rather avoid the minor change in logic if we don't need to make it.

Thanks,

Jonathan


> ---
>  drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a7d7e5143ed2..a4f6bb96d4f4 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_data_available.attr,
>  };
>  
> -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> +					     struct iio_dev *indio_dev)
>  {
>  	struct iio_dev_attr *p;
>  	struct attribute **attr;
> -	struct iio_buffer *buffer = indio_dev->buffer;
>  	int ret, i, attrn, attrcount;
>  	const struct iio_chan_spec *channels;
>  
> -	channels = indio_dev->channels;
> -	if (channels) {
> -		int ml = indio_dev->masklength;
> -
> -		for (i = 0; i < indio_dev->num_channels; i++)
> -			ml = max(ml, channels[i].scan_index + 1);
> -		indio_dev->masklength = ml;
> -	}
> -
> -	if (!buffer)
> -		return 0;
> -
>  	attrcount = 0;
>  	if (buffer->attrs) {
>  		while (buffer->attrs[attrcount] != NULL)
> @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	return ret;
>  }
>  
> -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  {
>  	struct iio_buffer *buffer = indio_dev->buffer;
> +	const struct iio_chan_spec *channels;
> +	int i;
> +
> +	channels = indio_dev->channels;
> +	if (channels) {
> +		int ml = indio_dev->masklength;
> +
> +		for (i = 0; i < indio_dev->num_channels; i++)
> +			ml = max(ml, channels[i].scan_index + 1);
> +		indio_dev->masklength = ml;
> +	}

I've not really figured out if it matters, but this is a logic change.
Previously we didn't compute masklength if there was no buffer provided.
Now we do.  It's probably better to move the if (!buffer) check above
this block or at least mention this change in the patch description.


>  
>  	if (!buffer)
> -		return;
> +		return 0;
> +
> +	return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
> +}
>  
> +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> +{
>  	bitmap_free(buffer->scan_mask);
>  	kfree(buffer->buffer_group.attrs);
>  	kfree(buffer->scan_el_group.attrs);
>  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
>  }
>  
> +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +
> +	if (!buffer)
> +		return;
> +
> +	__iio_buffer_free_sysfs_and_mask(buffer);
> +}
> +
>  /**
>   * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
>   * @indio_dev: the iio device
Alexandru Ardelean Sept. 17, 2020, 5:41 p.m. UTC | #2
On Thu, Sep 17, 2020 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 17 Sep 2020 15:59:51 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
> > primary argument. This change splits the main logic into a private function
> > that takes an IIO buffer as primary argument.
> >
> > That way, the functions can be extended to configure the sysfs for multiple
> > buffers.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> One comment inline.  Whilst I think it is safe as you have it, I'd
> rather avoid the minor change in logic if we don't need to make it.
>
> Thanks,
>
> Jonathan
>
>
> > ---
> >  drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
> >  1 file changed, 30 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index a7d7e5143ed2..a4f6bb96d4f4 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = {
> >       &dev_attr_data_available.attr,
> >  };
> >
> > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> > +                                          struct iio_dev *indio_dev)
> >  {
> >       struct iio_dev_attr *p;
> >       struct attribute **attr;
> > -     struct iio_buffer *buffer = indio_dev->buffer;
> >       int ret, i, attrn, attrcount;
> >       const struct iio_chan_spec *channels;
> >
> > -     channels = indio_dev->channels;
> > -     if (channels) {
> > -             int ml = indio_dev->masklength;
> > -
> > -             for (i = 0; i < indio_dev->num_channels; i++)
> > -                     ml = max(ml, channels[i].scan_index + 1);
> > -             indio_dev->masklength = ml;
> > -     }
> > -
> > -     if (!buffer)
> > -             return 0;
> > -
> >       attrcount = 0;
> >       if (buffer->attrs) {
> >               while (buffer->attrs[attrcount] != NULL)
> > @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >       return ret;
> >  }
> >
> > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >  {
> >       struct iio_buffer *buffer = indio_dev->buffer;
> > +     const struct iio_chan_spec *channels;
> > +     int i;
> > +
> > +     channels = indio_dev->channels;
> > +     if (channels) {
> > +             int ml = indio_dev->masklength;
> > +
> > +             for (i = 0; i < indio_dev->num_channels; i++)
> > +                     ml = max(ml, channels[i].scan_index + 1);
> > +             indio_dev->masklength = ml;
> > +     }
>
> I've not really figured out if it matters, but this is a logic change.
> Previously we didn't compute masklength if there was no buffer provided.
> Now we do.  It's probably better to move the if (!buffer) check above
> this block or at least mention this change in the patch description.
>

Umm, are you referring that this patch is a logic change or you are
suggesting a logic change?
The "if (!buffer)" check was positioned after the masklength
computation even in the old code.

>
> >
> >       if (!buffer)
> > -             return;
> > +             return 0;
> > +
> > +     return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
> > +}
> >
> > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> > +{
> >       bitmap_free(buffer->scan_mask);
> >       kfree(buffer->buffer_group.attrs);
> >       kfree(buffer->scan_el_group.attrs);
> >       iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> >  }
> >
> > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > +{
> > +     struct iio_buffer *buffer = indio_dev->buffer;
> > +
> > +     if (!buffer)
> > +             return;
> > +
> > +     __iio_buffer_free_sysfs_and_mask(buffer);
> > +}
> > +
> >  /**
> >   * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
> >   * @indio_dev: the iio device
>
Jonathan Cameron Sept. 17, 2020, 5:55 p.m. UTC | #3
On Thu, 17 Sep 2020 20:41:08 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Thu, Sep 17, 2020 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 17 Sep 2020 15:59:51 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >  
> > > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
> > > primary argument. This change splits the main logic into a private function
> > > that takes an IIO buffer as primary argument.
> > >
> > > That way, the functions can be extended to configure the sysfs for multiple
> > > buffers.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> >
> > One comment inline.  Whilst I think it is safe as you have it, I'd
> > rather avoid the minor change in logic if we don't need to make it.
> >
> > Thanks,
> >
> > Jonathan
Applied to the togreg branch of iio.git.

See below for my pathetic Diff confused me excuse :)

Jonathan

> >
> >  
> > > ---
> > >  drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
> > >  1 file changed, 30 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > index a7d7e5143ed2..a4f6bb96d4f4 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = {
> > >       &dev_attr_data_available.attr,
> > >  };
> > >
> > > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> > > +                                          struct iio_dev *indio_dev)
> > >  {
> > >       struct iio_dev_attr *p;
> > >       struct attribute **attr;
> > > -     struct iio_buffer *buffer = indio_dev->buffer;
> > >       int ret, i, attrn, attrcount;
> > >       const struct iio_chan_spec *channels;
> > >
> > > -     channels = indio_dev->channels;
> > > -     if (channels) {
> > > -             int ml = indio_dev->masklength;
> > > -
> > > -             for (i = 0; i < indio_dev->num_channels; i++)
> > > -                     ml = max(ml, channels[i].scan_index + 1);
> > > -             indio_dev->masklength = ml;
> > > -     }
> > > -
> > > -     if (!buffer)
> > > -             return 0;
> > > -
> > >       attrcount = 0;
> > >       if (buffer->attrs) {
> > >               while (buffer->attrs[attrcount] != NULL)
> > > @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > >       return ret;
> > >  }
> > >
> > > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > >  {
> > >       struct iio_buffer *buffer = indio_dev->buffer;
> > > +     const struct iio_chan_spec *channels;
> > > +     int i;
> > > +
> > > +     channels = indio_dev->channels;
> > > +     if (channels) {
> > > +             int ml = indio_dev->masklength;
> > > +
> > > +             for (i = 0; i < indio_dev->num_channels; i++)
> > > +                     ml = max(ml, channels[i].scan_index + 1);
> > > +             indio_dev->masklength = ml;
> > > +     }  
> >
> > I've not really figured out if it matters, but this is a logic change.
> > Previously we didn't compute masklength if there was no buffer provided.
> > Now we do.  It's probably better to move the if (!buffer) check above
> > this block or at least mention this change in the patch description.
> >  
> 
> Umm, are you referring that this patch is a logic change or you are
> suggesting a logic change?
> The "if (!buffer)" check was positioned after the masklength
> computation even in the old code.
> 
Got you.  Diff confused me :)


> >  
> > >
> > >       if (!buffer)
> > > -             return;
> > > +             return 0;
> > > +
> > > +     return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
> > > +}
> > >
> > > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> > > +{
> > >       bitmap_free(buffer->scan_mask);
> > >       kfree(buffer->buffer_group.attrs);
> > >       kfree(buffer->scan_el_group.attrs);
> > >       iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > >  }
> > >
> > > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +{
> > > +     struct iio_buffer *buffer = indio_dev->buffer;
> > > +
> > > +     if (!buffer)
> > > +             return;
> > > +
> > > +     __iio_buffer_free_sysfs_and_mask(buffer);
> > > +}
> > > +
> > >  /**
> > >   * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
> > >   * @indio_dev: the iio device  
> >
Alexandru Ardelean Sept. 18, 2020, 9:25 a.m. UTC | #4
On Thu, Sep 17, 2020 at 8:56 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 17 Sep 2020 20:41:08 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
>
> > On Thu, Sep 17, 2020 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Thu, 17 Sep 2020 15:59:51 +0300
> > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > >
> > > > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
> > > > primary argument. This change splits the main logic into a private function
> > > > that takes an IIO buffer as primary argument.
> > > >
> > > > That way, the functions can be extended to configure the sysfs for multiple
> > > > buffers.
> > > >
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > >
> > > One comment inline.  Whilst I think it is safe as you have it, I'd
> > > rather avoid the minor change in logic if we don't need to make it.
> > >
> > > Thanks,
> > >
> > > Jonathan
> Applied to the togreg branch of iio.git.
>
> See below for my pathetic Diff confused me excuse :)
>
> Jonathan
>
> > >
> > >
> > > > ---
> > > >  drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
> > > >  1 file changed, 30 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > > index a7d7e5143ed2..a4f6bb96d4f4 100644
> > > > --- a/drivers/iio/industrialio-buffer.c
> > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = {
> > > >       &dev_attr_data_available.attr,
> > > >  };
> > > >
> > > > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> > > > +                                          struct iio_dev *indio_dev)
> > > >  {
> > > >       struct iio_dev_attr *p;
> > > >       struct attribute **attr;
> > > > -     struct iio_buffer *buffer = indio_dev->buffer;
> > > >       int ret, i, attrn, attrcount;
> > > >       const struct iio_chan_spec *channels;
> > > >
> > > > -     channels = indio_dev->channels;
> > > > -     if (channels) {
> > > > -             int ml = indio_dev->masklength;
> > > > -
> > > > -             for (i = 0; i < indio_dev->num_channels; i++)
> > > > -                     ml = max(ml, channels[i].scan_index + 1);
> > > > -             indio_dev->masklength = ml;
> > > > -     }
> > > > -
> > > > -     if (!buffer)
> > > > -             return 0;
> > > > -
> > > >       attrcount = 0;
> > > >       if (buffer->attrs) {
> > > >               while (buffer->attrs[attrcount] != NULL)
> > > > @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > >       return ret;
> > > >  }
> > > >
> > > > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > >  {
> > > >       struct iio_buffer *buffer = indio_dev->buffer;
> > > > +     const struct iio_chan_spec *channels;
> > > > +     int i;
> > > > +
> > > > +     channels = indio_dev->channels;
> > > > +     if (channels) {
> > > > +             int ml = indio_dev->masklength;
> > > > +
> > > > +             for (i = 0; i < indio_dev->num_channels; i++)
> > > > +                     ml = max(ml, channels[i].scan_index + 1);
> > > > +             indio_dev->masklength = ml;
> > > > +     }
> > >
> > > I've not really figured out if it matters, but this is a logic change.
> > > Previously we didn't compute masklength if there was no buffer provided.
> > > Now we do.  It's probably better to move the if (!buffer) check above
> > > this block or at least mention this change in the patch description.
> > >
> >
> > Umm, are you referring that this patch is a logic change or you are
> > suggesting a logic change?
> > The "if (!buffer)" check was positioned after the masklength
> > computation even in the old code.
> >
> Got you.  Diff confused me :)

Yeah.
Happens to me to with some git diffs.

>
>
> > >
> > > >
> > > >       if (!buffer)
> > > > -             return;
> > > > +             return 0;
> > > > +
> > > > +     return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
> > > > +}
> > > >
> > > > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> > > > +{
> > > >       bitmap_free(buffer->scan_mask);
> > > >       kfree(buffer->buffer_group.attrs);
> > > >       kfree(buffer->scan_el_group.attrs);
> > > >       iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > > >  }
> > > >
> > > > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > > +{
> > > > +     struct iio_buffer *buffer = indio_dev->buffer;
> > > > +
> > > > +     if (!buffer)
> > > > +             return;
> > > > +
> > > > +     __iio_buffer_free_sysfs_and_mask(buffer);
> > > > +}
> > > > +
> > > >  /**
> > > >   * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
> > > >   * @indio_dev: the iio device
> > >
>
diff mbox series

Patch

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a7d7e5143ed2..a4f6bb96d4f4 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1264,26 +1264,14 @@  static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_data_available.attr,
 };
 
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
+static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
+					     struct iio_dev *indio_dev)
 {
 	struct iio_dev_attr *p;
 	struct attribute **attr;
-	struct iio_buffer *buffer = indio_dev->buffer;
 	int ret, i, attrn, attrcount;
 	const struct iio_chan_spec *channels;
 
-	channels = indio_dev->channels;
-	if (channels) {
-		int ml = indio_dev->masklength;
-
-		for (i = 0; i < indio_dev->num_channels; i++)
-			ml = max(ml, channels[i].scan_index + 1);
-		indio_dev->masklength = ml;
-	}
-
-	if (!buffer)
-		return 0;
-
 	attrcount = 0;
 	if (buffer->attrs) {
 		while (buffer->attrs[attrcount] != NULL)
@@ -1367,19 +1355,45 @@  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	return ret;
 }
 
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
+int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 {
 	struct iio_buffer *buffer = indio_dev->buffer;
+	const struct iio_chan_spec *channels;
+	int i;
+
+	channels = indio_dev->channels;
+	if (channels) {
+		int ml = indio_dev->masklength;
+
+		for (i = 0; i < indio_dev->num_channels; i++)
+			ml = max(ml, channels[i].scan_index + 1);
+		indio_dev->masklength = ml;
+	}
 
 	if (!buffer)
-		return;
+		return 0;
+
+	return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
+}
 
+static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
+{
 	bitmap_free(buffer->scan_mask);
 	kfree(buffer->buffer_group.attrs);
 	kfree(buffer->scan_el_group.attrs);
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
 }
 
+void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
+{
+	struct iio_buffer *buffer = indio_dev->buffer;
+
+	if (!buffer)
+		return;
+
+	__iio_buffer_free_sysfs_and_mask(buffer);
+}
+
 /**
  * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
  * @indio_dev: the iio device