On Thursday, July 25, 2013 6:39 AM, Ian Abbott wrote:
> On 2013-07-24 19:48, H Hartley Sweeten wrote:
>> When updating the output channels make sure the channels configured as
>> inputs stay inputs.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/staging/comedi/drivers/pcmuio.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/pcmuio.c b/drivers/staging/comedi/drivers/pcmuio.c
>> index 076a08a..5b0ade2 100644
>> --- a/drivers/staging/comedi/drivers/pcmuio.c
>> +++ b/drivers/staging/comedi/drivers/pcmuio.c
>> @@ -215,8 +215,14 @@ static int pcmuio_dio_insn_bits(struct comedi_device *dev,
>> s->state &= ~mask;
>> s->state |= (mask & bits);
>>
>> - /* invert the state and update the channels */
>> + /*
>> + * Invert the state and update the channels.
>> + *
>> + * The s->io_bits mask makes sure inputs are '0' so
>> + * that the output pins stay in a high-z state.
>> + */
>> val = s->state ^ ((0x1 << s->n_chan) - 1);
>> + val &= s->io_bits;
>> pcmuio_write(dev, val, asic, 0, port);
>> }
>>
>>
>
> That's fine although the original function has other problems, for
> example the value returned in data[1] should reflect the modified data,
> not the original data and probably shouldn't be modifying s->state based
> on the data read from the hardware. I think the function should be
> something like this:
>
> static int pcmuio_dio_insn_bits(struct comedi_device *dev,
> struct comedi_subdevice *s,
> struct comedi_insn *insn,
> unsigned int *data)
> {
> unsigned int chanmask = ((1 << s->n_chan) - 1;
> unsigned int mask = data[0] & chanmask;
> unsigned int bits = data[1];
> int asic = s->index / 2;
> int port = (s->index % 2) ? 3 : 0;
> unsigned int val;
>
> if (mask) {
> s->state &= ~mask;
> s->state |= (mask & bits);
> /*
> * Invert the state and update the channels.
> *
> * The s->io_bits mask makes sure inputs are '0' so
> * that the output pins stay in a high-z state.
> */
> val = ~s->state & s->io_bits & chanmask;
> pcmuio_write(dev, val, asic, 0, port);
> }
>
> /* get inverted state of the channels from the port */
> val = pcmuio_read(dev, asic, 0, port);
>
> /* get the true state of the channels */
> data[1] = ~val & chanmask;
>
> return insn->n;
> }
This routine made my head hurt...
End result I "think" both solutions end up doing the same thing. But, I like
your proposal better.
I'll try to get this series rebased today. Please let me know if you want me
to add the 'irq' member to the private data for the 2nd interrupt.
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
