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@xxxxxxxxxxxxxxxxxxx>
>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
