On Thursday, August 29, 2013 9:37 AM, Ian Abbott wrote:
> 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@xxxxxxxxxxxxxxxxxxx>
>>>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
[snip]
>>>> +/**
>>>> + * 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.)
I could also add a sanity check of (insn->n) but it adds that overhead to every
call.
Actually, the only place in the core that calls the (*insn_bits) is in comedi_fops.c
parse_insn(). In that function the sanity check of insn->n is already done.
To reduce the number of parameters I'll modify the patch to the reduced
form above.
>> 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.
I'll also return the filtered mask and leave all the conditional updates in
the drivers.
Hopefully I will be able to get the updated series posted later today.
Thanks,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
