[PATCH 01/11] staging: comedi: core: introduce comedi_dio_insn_bits()

Ian Abbott abbotti at mev.co.uk
Thu Aug 29 16:36:40 UTC 2013


On 2013-08-29 17:20, Hartley Sweeten wrote:
> On Thursday, August 29, 2013 4:14 AM, Ian Abbott wrote:
>> On 2013-08-28 21:26, H Hartley Sweeten wrote:
>>> The (*insn_bits) functions for DIO and DO subdevices typically use
>>> the subdevice 's->state' to hold the current state of the output
>>> channels. The 'insn' passed to these functions, INSN_BITS, specifies
>>> two parameters passed in the 'data'.
>>>
>>>     data[0] = 'mask', the channels to update
>>>     data[1] = 'bits', the new state for the channels
>>>
>>> Introduce a helper function to handle this boilerplate.
>>>
>>> The function returns:
>>>
>>>     0 if there are no changes in the state
>>>
>>>     1 if the driver needs to update the hardware to a new state
>>>
>>> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
>>> Cc: Ian Abbott <abbotti at mev.co.uk>
>>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>>> ---
>>>    drivers/staging/comedi/comedidev.h |  2 ++
>>>    drivers/staging/comedi/drivers.c   | 25 +++++++++++++++++++++++++
>>>    2 files changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
>>> index 2e19f65..de36499 100644
>>> --- a/drivers/staging/comedi/comedidev.h
>>> +++ b/drivers/staging/comedi/comedidev.h
>>> @@ -345,6 +345,8 @@ void comedi_buf_memcpy_from(struct comedi_async *async, unsigned int offset,
>>>    int comedi_dio_insn_config(struct comedi_device *, struct comedi_subdevice *,
>>>    			   struct comedi_insn *, unsigned int *data,
>>>    			   unsigned int mask);
>>> +int comedi_dio_insn_bits(struct comedi_device *, struct comedi_subdevice *,
>>> +			 struct comedi_insn *, unsigned int *data);
>>>
>>>    void *comedi_alloc_devpriv(struct comedi_device *, size_t);
>>>    int comedi_alloc_subdevices(struct comedi_device *, int);
>>> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
>>> index 317a821..2bbd9d0 100644
>>> --- a/drivers/staging/comedi/drivers.c
>>> +++ b/drivers/staging/comedi/drivers.c
>>> @@ -190,6 +190,31 @@ int comedi_dio_insn_config(struct comedi_device *dev,
>>>    }
>>>    EXPORT_SYMBOL_GPL(comedi_dio_insn_config);
>>>
>>> +/**
>>> + * comedi_dio_insn_bits() - boilerplate (*insn_bits) for DIO and DO subdevices.
>>> + * @dev: comedi_device struct
>>> + * @s: comedi_subdevice struct
>>> + * @insn: comedi_insn struct
>>> + * @data: parameters for the @insn
>>> + */
>>> +int comedi_dio_insn_bits(struct comedi_device *dev,
>>> +			 struct comedi_subdevice *s,
>>> +			 struct comedi_insn *insn,
>>> +			 unsigned int *data)
>>> +{
>>> +	unsigned int mask = data[0];
>>> +	unsigned int bits = data[1];
>>> +
>>> +	if (mask) {
>>> +		s->state &= ~mask;
>>> +		s->state |= (bits & mask);
>>> +
>>> +		return 1;
>>> +	}
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(comedi_dio_insn_bits);
>>> +
>>
>> I'm not convinced this achieves much.  It doesn't use the insn parameter
>> at all.
>
> Well it does remove 302 lines of boilerplate code.
>
> The parameters for the function could be reduced to just.
>
> int comedi_dio_insn_bits(struct comedi_subdevice *s,
> 			 unsigned int mask,
> 			unsigned int bits)
>
> Then the callers could do:
>
> 	If (comedi_dio_insn_bits(s, data[0], data[1]) {
> 		/* Update the hardware */
> 	}
>
> But, I think just passing all the (*insn_bits) parameters is a bit cleaner.

It just seemed a waste passing a parameter that is never used.  You 
could still pass the data[] array as a pointer with the assumption that 
it's 2 elements long.  (Or you could declare it as `unsigned int 
data[2]` in the prototype.  I'm a little wary of using array syntax in 
function prototypes but I suppose it allows some bounds checking during 
static analysis.)

>
> To address your thought in PATCH 04/11, the return could be either the
> raw 'mask' (data[0]) or the filtered mask (data[0] & s->io_bits).

I suppose the filtered mask makes more sense, as that is what has been 
used to mask the s->state changes.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-


More information about the devel mailing list