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

[Hartley Sweeten]

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.

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

Regards,
Hartley