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

[Jonathan Cameron]

On Fri, 24 Apr 2020 07:51:05 +0000
"Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote:

> > 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.
Given that we handle the demux only at the level of scan elements that won't work in general
(even if it wasn't a horrible layering issue).

> 
> 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. 

Excellent points. We need to address support for:

1) available_scan_mask - if we have complicated rules on mixtures of channels inside
   a given buffer element.

2) channel enabling though I'm sort of inclined to say that if you are using this approach
   you only get information on channels that make up a scan mask element.  Tough luck you
   may end up enabling more than you'd like.

It might be possible to make switch to using a channel mask but given the channel index is
implicit that is going to be at least a little bit nasty.

How much does it hurt to not have the ability to separately control channels within
a given buffer element?   Userspace can enable / disable them but reality is you'll
get data for all the channels in a buffer element if any of them are enabled.

Given the demux will copy the whole element anyway (don't want to waste time doing
masking inside an element), userspace might get these extra channels anyway if another
consumer has enabled them.

Jonathan


> 
> - Nuno SÃ