RE: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis

[Sa, Nuno]

> From: linux-iio-owner@xxxxxxxxxxxxxxx <linux-iio-owner@xxxxxxxxxxxxxxx>
> On Behalf Of Alexandru Ardelean
> Sent: Freitag, 24. April 2020 07:18
> To: linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: jic23@xxxxxxxxxx; lars@xxxxxxxxxx; Ardelean, Alexandru
> <alexandru.Ardelean@xxxxxxxxxx>
> Subject: [RFC PATCH 4/4] iio: Track enabled channels on a per channel basis
> 
> From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> 
> Now that we support multiple channels with the same scan index we can no
> longer use the scan mask to track which channels have been enabled.
> Otherwise it is not possible to enable channels with the same scan index
> independently.
> 
> Introduce a new channel mask which is used instead of the scan mask to
> track which channels are enabled. Whenever the channel mask is changed a
> new scan mask is computed based on it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> ---
>  drivers/iio/industrialio-buffer.c | 62 +++++++++++++++++++++----------
>  drivers/iio/inkern.c              | 19 +++++++++-
>  include/linux/iio/buffer_impl.h   |  3 ++
>  include/linux/iio/consumer.h      |  2 +
>  4 files changed, 64 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index c06691281287..1821a3e32fb3 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -216,12 +216,20 @@ int iio_buffer_alloc_scanmask(struct iio_buffer
> *buffer,
>  	if (buffer->scan_mask == NULL)
>  		return -ENOMEM;
> 
> +	buffer->channel_mask = bitmap_zalloc(indio_dev->num_channels,
> +					     GFP_KERNEL);
> +	if (buffer->channel_mask == NULL) {
> +		bitmap_free(buffer->scan_mask);
> +		return -ENOMEM;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
> 
>  void iio_buffer_free_scanmask(struct iio_buffer *buffer)
>  {
> +	bitmap_free(buffer->channel_mask);
>  	bitmap_free(buffer->scan_mask);
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
> @@ -285,7 +293,7 @@ static ssize_t iio_scan_el_show(struct device *dev,
> 
>  	/* Ensure ret is 0 or 1. */
>  	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> -		       indio_dev->buffer->scan_mask);
> +		       indio_dev->buffer->channel_mask);
> 
>  	return sprintf(buf, "%d\n", ret);
>  }
> @@ -330,11 +338,12 @@ static bool iio_validate_scan_mask(struct iio_dev
> *indio_dev,
>   * buffers might request, hence this code only verifies that the
>   * individual buffers request is plausible.
>   */
> -static int iio_scan_mask_set(struct iio_dev *indio_dev,
> -		      struct iio_buffer *buffer, int bit)
> +static int iio_channel_mask_set(struct iio_dev *indio_dev,
> +				struct iio_buffer *buffer, int bit)
>  {
>  	const unsigned long *mask;
>  	unsigned long *trialmask;
> +	unsigned int ch;
> 
>  	trialmask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
>  	if (trialmask == NULL)
> @@ -343,8 +352,11 @@ static int iio_scan_mask_set(struct iio_dev
> *indio_dev,
>  		WARN(1, "Trying to set scanmask prior to registering
> buffer\n");
>  		goto err_invalid_mask;
>  	}
> -	bitmap_copy(trialmask, buffer->scan_mask, indio_dev-
> >masklength);
> -	set_bit(bit, trialmask);
> +
> +	set_bit(bit, buffer->channel_mask);
> +
> +	for_each_set_bit(ch, buffer->channel_mask, indio_dev-
> >num_channels)
> +		set_bit(indio_dev->channels[ch].scan_index, trialmask);

So, here if the channels all have the same scan_index, we will end up with a scan_mask which is
different that channel_mask, right? I saw that in our internal driver's we then just access the
channel_mask field directly to know what pieces/channels do we need to enable prior to
buffering, which implies including buffer_impl.h.

So, for me it would make sense to compute scan_mask so that it will be the same as channel_mask
(hmm but that would be a problem when computing the buffer size...) and drivers can correctly use
` validate_scan_mask ()` cb. Alternatively, we need to expose channel_mask either on a new cb or
change the ` validate_scan_mask ()` footprint. 

- Nuno Sá