Re: [PATCH] iio: buffer: split buffer sysfs creation to take buffer as primary arg

[Jonathan Cameron]

On Thu, 17 Sep 2020 15:59:51 +0300
Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> 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@xxxxxxxxxx>

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