On 2014-04-15 18:37, H Hartley Sweeten wrote:
Step 5 of the (*do_cmdtest) validates that the chanlist, chan/range/aref/etc, is compatible with the actual hardware. At this point in the (*do_cmdtest) the chanlist is valid, due to checks in the core, so the sanity check is not needed.For aesthetics, factor the step 5 code into a helper function. Tidy up the factored out code. Signed-off-by: H Hartley Sweeten <hswee...@visionengravers.com> Cc: Ian Abbott <abbo...@mev.co.uk> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> --- drivers/staging/comedi/drivers/amplc_pci224.c | 73 ++++++++++++--------------- 1 file changed, 32 insertions(+), 41 deletions(-) diff --git a/drivers/staging/comedi/drivers/amplc_pci224.c b/drivers/staging/comedi/drivers/amplc_pci224.c index 9f0c20f..eea27e6 100644 --- a/drivers/staging/comedi/drivers/amplc_pci224.c +++ b/drivers/staging/comedi/drivers/amplc_pci224.c @@ -687,6 +687,36 @@ pci224_ao_inttrig_start(struct comedi_device *dev, struct comedi_subdevice *s, return 1; } +static int pci224_ao_check_chanlist(struct comedi_device *dev, + struct comedi_subdevice *s, + struct comedi_cmd *cmd) +{ + unsigned int range0 = CR_RANGE(cmd->chanlist[0]); + unsigned int chan_mask = 0; + int i; + + for (i = 0; i < cmd->chanlist_len; i++) { + unsigned int chan = CR_CHAN(cmd->chanlist[i]); + unsigned int range = CR_RANGE(cmd->chanlist[i]); + + if (chan_mask & (1 << chan)) { + dev_dbg(dev->class_dev, + "%s: chanlist must contain no duplicate channels\n", + __func__); + return -EINVAL; + } + chan_mask |= (1 << chan); + + if (range != range0) { + dev_dbg(dev->class_dev, + "%s: chanlist must all have the same range index\n", + __func__); + return -EINVAL; + } + } + return 0; +} + #define MAX_SCAN_PERIOD 0xFFFFFFFFU #define MIN_SCAN_PERIOD 2500 #define CONVERT_PERIOD 625 @@ -871,48 +901,9 @@ pci224_ao_cmdtest(struct comedi_device *dev, struct comedi_subdevice *s, if (err) return 4; - /* Step 5: check channel list. */ + /* Step 5: check channel list */ - if (cmd->chanlist && (cmd->chanlist_len > 0)) { - unsigned int range; - enum { range_err = 1, dupchan_err = 2, }; - unsigned errors; - unsigned int n; - unsigned int ch; - - /* - * Check all channels have the same range index. Don't care - * about analogue reference, as we can't configure it. - * - * Check the list has no duplicate channels. - */ - range = CR_RANGE(cmd->chanlist[0]); - errors = 0; - tmp = 0; - for (n = 0; n < cmd->chanlist_len; n++) { - ch = CR_CHAN(cmd->chanlist[n]); - if (tmp & (1U << ch)) - errors |= dupchan_err; - - tmp |= (1U << ch); - if (CR_RANGE(cmd->chanlist[n]) != range) - errors |= range_err; - - } - if (errors) { - if (errors & dupchan_err) { - dev_dbg(dev->class_dev, - "%s: entries in chanlist must contain no duplicate channels\n", - __func__); - } - if (errors & range_err) { - dev_dbg(dev->class_dev, - "%s: entries in chanlist must all have the same range index\n", - __func__); - } - err++; - } - } + err |= pci224_ao_check_chanlist(dev, s, cmd); if (err) return 5;
As for the remarks in patch 02, it's possible for chanlist to be NULL or for chanlist_len to be 0, so pci224_ao_check_chanlist() should only be called if chanlist is non-NULL and chanlist_len is non-0.
But if chanlist is forced to be NULL by __comedi_get_user_chanlist() when chanlist_len is 0, it would be sufficient to check that chanlist is non-NULL before calling pci224_ao_check_chanlist().
-- -=( Ian Abbott @ MEV Ltd. E-mail: <abbo...@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
