On Friday, August 30, 2013 5:26 AM, Ian Abbott wrote:
> On 2013-08-30 02:00, H Hartley Sweeten wrote:
>> Make sure that only the state of the channels configured as outputs are
>> updated.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/staging/comedi/drivers.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
>> index 75d877b..777be6d 100644
>> --- a/drivers/staging/comedi/drivers.c
>> +++ b/drivers/staging/comedi/drivers.c
>> @@ -198,7 +198,7 @@ EXPORT_SYMBOL_GPL(comedi_dio_insn_config);
>> unsigned int comedi_dio_update_state(struct comedi_subdevice *s,
>> unsigned int *data)
>> {
>> - unsigned int mask = data[0];
>> + unsigned int mask = data[0] & s->io_bits; /* outputs only */
>> unsigned int bits = data[1];
>>
>> if (mask) {
>>
>
> I have had second thoughts about that as some application code could be
> trying to set the state on an input channel in advance of reconfiguring
> it as an output. That wouldn't work on all hardware but would work on
> some hardware (e.g. hardware that has a "data output" register plus a
> "data direction" register).
>
> But filtering the mask in this way seems like a wholesale change for the
> sake of a handful of drivers, and for some of those handful of drivers
> it probably doesn't matter anyway.
>
> Callers of comedi_dio_update_state() that want to limit register writes
> to outputs only can AND the return value with s->io_bits. The s->state
> bits would still be updated for the input channels, but they wouldn't
> get written to the hardware (unless they fall in the same register as
> the output channels).
>
> For the drivers in patch 17:
[snip]
> It's probably a good idea to limit the mask to channels that actually
> exist though, something like:
>
> unsigned int chanmask = s->n_chans < 32 ?
> (1U << s->n_chans) - 1 : 0xffffffff;
> unsigned int mask = data[0] & chanmask;
> unsigned int bits = data[1];
Hmm.. Maybe the 'chanmask' should be added to struct comedi_subdevice.
I need to look through the drivers but it seems like it would be generically
useful. It could be initialized during the postconfig.
For now I'll just modify comedi_dio_update_state() as you suggested.
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
