staging: comedi: 8255: refactor subdev_8255_insn function
PATCH 12/15 staging: comedi: 8255: refactor subdev_8255_insn function
Refactor the subdev_8255_insn function to follow what the user space library is doing.
1) This function requires 'data' to contain 2 parameters. Add a sanity check for this (insn->n != 2).
2) The 'data' parameters are actually a 'mask' of the valid bits and the actual 'bits' to modify. Create local variables of these names to help with maintainability.
3) The value returned in data[1] reflects the actual state of the 8255 io pins after the update of the masked bits. Fetch this data in a local variable and then set data[1].
4) The user space library only checks for a < 0 return to indicate an error. It appears that the 'return 2' in this function is supposed to indicate the number of data parameters used to perform the command. Return the insn->n value for this, the open coded '2' looks wrong.
--- drivers/staging/comedi/drivers/8255.c | 42 +++++++++++++++++++++----------- 1 files changed, 27 insertions(+), 15 deletions(-)
drivers/staging/comedi/drivers/8255.c +27 -15
@@ -135,26 +135,38 @@ static int subdev_8255_insn(struct comedi_device *dev, {struct subdev_8255_private *spriv = s->private;unsigned long iobase = spriv->iobase; +unsigned int mask; +unsigned int bits; +unsigned int v; + +if (insn->n !=2 ) +return -EINVAL ; -if (data[0 ]) { - s->state &= ~data[0 ]; - s->state |= (data[0 ] & data[1 ]); + mask = data[0 ]; + bits = data[1 ]; -if (data[0 ] &0xff ) - spriv->io (1 , _8255_DATA, s->state &0xff , iobase); -if (data[0 ] &0xff00 ) - spriv->io (1 , _8255_DATA +1 , - (s->state >>8 ) &0xff , iobase); -if (data[0 ] &0xff0000 ) - spriv->io (1 , _8255_DATA +2 , - (s->state >>16 ) &0xff , iobase); +if (mask) { + v = s->state; + v &= ~mask; + v |= (bits & mask); + +if (mask &0xff ) + spriv->io (1 , _8255_DATA, v &0xff , iobase); +if (mask &0xff00 ) + spriv->io (1 , _8255_DATA +1 , (v >>8 ) &0xff , iobase); +if (mask &0xff0000 ) + spriv->io (1 , _8255_DATA +2 , (v >>16 ) &0xff , iobase); + + s->state = v; } - data[1 ] = spriv->io (0 , _8255_DATA,0 , iobase); - data[1 ] |= (spriv->io (0 , _8255_DATA +1 ,0 , iobase) <<8 ); - data[1 ] |= (spriv->io (0 , _8255_DATA +2 ,0 , iobase) <<16 ); + v = spriv->io (0 , _8255_DATA,0 , iobase); + v |= (spriv->io (0 , _8255_DATA +1 ,0 , iobase) <<8 ); + v |= (spriv->io (0 , _8255_DATA +2 ,0 , iobase) <<16 ); + + data[1 ] = v; -return 2 ; +return insn->n; }static void do_config (struct comedi_device *dev,struct comedi_subdevice *s)
Signature · 1 line
1.7.7
Quoted 5 lines · H Hartley Sweeten
Refactor the
subdev_8255_insnfunction to follow what the user space library is doing.1) This function requires 'data' to contain 2 parameters. Add a sanity check for this (
insn->n!= 2).
Actually, the comedi core will have already checked that.
Quoted 4 lines
4) The user space library only checks for a < 0 return to indicate an error. It appears that the 'return 2' in this function is supposed to indicate the number of data parameters used to perform the command. Return the
insn->nvalue for this, the open coded '2' looks wrong.
As long as it still returns 2, I don't care how you do it!
Signature · 2 lines
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-