Re: [PATCH 04/16] staging:iio: scrap scan_count and ensure all drivers use active_scan_mask

[Jonathan Cameron <jic23@kernel.org>]

On 11/28/2011 09:19 PM, Lars-Peter Clausen wrote:
> On 11/28/2011 10:02 PM, Jonathan Cameron wrote:
>> On 11/28/2011 04:15 PM, Lars-Peter Clausen wrote:
>>> On 11/28/2011 10:45 AM, Lars-Peter Clausen wrote:
>>>> On 11/27/2011 02:33 PM, Jonathan Cameron wrote:
>>>>> From: Jonathan Cameron <jic23@cam.ac.uk>
>>>>>
>>>>> Obviously drivers should only use this for pushing to buffers.
>>>>> They need buffer->scan_mask for pulling from them post demux.
>>>>>
>>>>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
>>>>> ---
>>>>>  drivers/staging/iio/accel/adis16201_ring.c      |   10 +++++-----
>>>>>  drivers/staging/iio/accel/adis16203_ring.c      |   10 +++++-----
>>>>>  drivers/staging/iio/accel/adis16204_ring.c      |   10 +++++-----
>>>>>  drivers/staging/iio/accel/adis16209_ring.c      |    5 +++--
>>>>>  drivers/staging/iio/accel/adis16240_ring.c      |    5 +++--
>>>>>  drivers/staging/iio/accel/lis3l02dq_ring.c      |   23 +++++++++++++----------
>>>>>  drivers/staging/iio/adc/ad7192.c                |   10 ++++++----
>>>>>  drivers/staging/iio/adc/ad7298_ring.c           |   12 +++++++-----
>>>>>  drivers/staging/iio/adc/ad7476_ring.c           |    3 ++-
>>>>>  drivers/staging/iio/adc/ad7793.c                |   11 ++++++-----
>>>>>  drivers/staging/iio/adc/ad7887_ring.c           |    8 +++++---
>>>>>  drivers/staging/iio/adc/ad799x_ring.c           |   13 ++++++++-----
>>>>>  drivers/staging/iio/buffer.h                    |    2 --
>>>>>  drivers/staging/iio/gyro/adis16260_ring.c       |    5 +++--
>>>>>  drivers/staging/iio/iio_simple_dummy_buffer.c   |    7 +++++--
>>>>>  drivers/staging/iio/impedance-analyzer/ad5933.c |   14 ++++++++------
>>>>>  drivers/staging/iio/imu/adis16400_ring.c        |   19 +++++++++++--------
>>>>>  drivers/staging/iio/industrialio-buffer.c       |    2 --
>>>>>  drivers/staging/iio/meter/ade7758_ring.c        |    7 ++++---
>>>>>  19 files changed, 99 insertions(+), 77 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/iio/accel/adis16201_ring.c b/drivers/staging/iio/accel/adis16201_ring.c
>>>>> index 936e8cb..68d4b38 100644
>>>>> --- a/drivers/staging/iio/accel/adis16201_ring.c
>>>>> +++ b/drivers/staging/iio/accel/adis16201_ring.c
>>>>> @@ -74,11 +74,11 @@ static irqreturn_t adis16201_trigger_handler(int irq, void *p)
>>>>>  		return -ENOMEM;
>>>>>  	}
>>>>>  
>>>>> -	if (ring->scan_count)
>>>>> -		if (adis16201_read_ring_data(indio_dev, st->rx) >= 0)
>>>>> -			for (; i < ring->scan_count; i++)
>>>>> -				data[i] = be16_to_cpup(
>>>>> -					(__be16 *)&(st->rx[i*2]));
>>>>> +	if (!bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)
>>>>> +	    && adis16201_read_ring_data(indio_dev, st->rx) >= 0)
>>>>> +		for (; i < bitmap_weight(indio_dev->active_scan_mask,
>>>>> +					 indio_dev->masklength); i++)
>>>>> +			data[i] = be16_to_cpup((__be16 *)&(st->rx[i*2]));
>>>>>  
>>>>
>>>> Does it really make sense to recompute bitmap_weight for each transfer?
>>>> Can't we update scan_count once, when we update the scan_mask?
>> I partly got rid of scan mask to make the more 'interesting' route with
>> the active scan mask stuff easier to adapt (information is now in only
>> one place.).
>> I'd not be against caching this somewhere if it is convenient for the
>> driver (such as in the update_scan_mode callback). I'm not however
>> convinced it should be in the core code.  Mostly the transfers that rely
>> on this should probably be setup in the preenable functions etc
>> rather than buffers being allocated in this fast path. This particular
>> approach is based on some of my early drivers. I much prefer Michaels
>> drivers where the whole transfer sequence is setup and only changed when
>> the channels being captured change.  The only other common use case is a
>> fudge to get the timestamp in the right place.
>> It kind of feels there ought to be a better way of handing software
>> timestamps.  Perhaps your timestamp source stuff that mentioned in
>> the ZIO thread.
>>
>> This particular example is a bad one as it shouldn't be there at all
>> given we no longer do endian conversion but rather describe the buffer.
>>
>> Don't suppose you fancy writing the patch that gets rid of these endian
>> conversions?  If not I'll get to it, but may not be that soon.
> 
> I'll try to have a look at it tomorrow.
> 
>>> Also: for chips where we can only read all values at once, should we set
>>> available_scan_masks to ~0 and just let the demuxer handle everything else?
>> No. For consistency (and ability to use some of the standard functions)
>> the available scan masks should list the actual combination that is
>> available.  Assuming I've understood you correctly and you don't just
>> mean for the channels that exist?
>>>
> 
> What I meant was, if for example a device has 4 channels and we can only read
> all 4 channels together and not selectively only a subset, available_scan_masks
> should contain one item set to 0xf. This would allow us to just pass the buffer
> we read from the device up to the next level and let the demuxer take care of
> splitting it up instead of manually do the demuxing, like it is done now.
Agreed.  I just only implemented the demux usage for the max1363 so far.
Definitely the intention that things like this will be covered.
> 
> E.g. the adis16201_trigger_handler would basically boil down to:
> ring->access->store_to(ring, st->rx, pf->timestamp);
> 
> On the other hand the adis16201 seems to support reading a subset of registers,
> but it's just not implemented.
IIRC it's to do with the fact that burst mode reading of everything is
faster for more than a couple of registers...  Hence no one bothered.