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>
---
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.
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.)
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.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
