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 Jonathan Cameron
> Sent: Sonntag, 26. April 2020 12:51
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: Ardelean, Alexandru <alexandru.Ardelean@xxxxxxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; lars@xxxxxxxxxx
> Subject: Re: [RFC PATCH 4/4] iio: Track enabled channels on a per channel
> basis
> 
> 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).

Yes, and the driver just adds 16 channels and points all of them to scan_index 0. It then
sets real_bits and the shift so that userspace can get the right channel bit. So, in the end
we have just one buffer/scan element with 16bits. My problem here is more architectural...
We should not directly include "buffer_impl.h" in drivers...

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

Maybe one solution to expose channel mask is to check if channel_mask != scan_mask
before calling the ` validate_scan_mask()`. If it is, we pass channel_mask to the callback.
Driver's should then know what to do with it...

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

Not sure if I'm fully understanding this point. I believe with this approach channel
enablement works as before since the core is kind of mapping channel_mask to
scan_mask. So if we have 16 channels using only 1 scan_element we can still
enable/disable all 16 channels.

In the end, if we have a traditional driver with one channel per scan_index, channel_mask
should be equal to scan_mask. As we start to have more than one channel pointing to the
same scan_index, these masks will be different.

> 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

As long as we are "ok" with the extra amount of allocated memory, I think it would work.
Though drivers will have to replicate the same data trough all the enabled scan elements...

- Nuno SÃ

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