Hi,
> the fix is nice but the org code has if (!n_char > 1) return 1 but now you have
> if (nchar == 1 ) is there any reason not to use ( nchar <= 1 ) ?
> (i do not see the whole code and it is not clear from your changelog)
You are missing out the last "else", basically the original code was
if n_chan <1
return 0
if n_chan >1
//do some counting
return count
else
return 1
For n_chan==1 the function doesn't actually do anything clever, that's
why I made the return right at the beginning.
Tomas
>
> re,
> wh
On Wed, May 2, 2012 at 8:41 PM, walter harms <wharms@xxxxxx> wrote:
>
>
> Am 02.05.2012 18:13, schrieb Tomas Melin:
>> -Simplified function logic by assuming that n_chan >1 if not <=1. Removes
>> one level of indentation.
>> -> readability improved and code fits into 80 chars
>> - Code indentation fixes, corrected comments
>> - Added braces to if() for readability
>>
>> Signed-off-by: Tomas Melin <tomas.melin@xxxxxx>
>> ---
>> drivers/staging/comedi/drivers/adv_pci1710.c | 84 +++++++++++++-------------
>> 1 files changed, 43 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c b/drivers/staging/comedi/drivers/adv_pci1710.c
>> index 8318c82..fa4a6fb 100644
>> --- a/drivers/staging/comedi/drivers/adv_pci1710.c
>> +++ b/drivers/staging/comedi/drivers/adv_pci1710.c
>> @@ -1142,9 +1142,9 @@ static int pci171x_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>>
>> /*
>> ==============================================================================
>> - Check if channel list from user is builded correctly
>> - If it's ok, then program scan/gain logic.
>> - This works for all cards.
>> + * Check if channel list from user is build correctly
>> + * If it's ok, then program scan/gain logic.
>> + * This works for all cards.
>> */
>> static int check_channel_list(struct comedi_device *dev,
>> struct comedi_subdevice *s,
>> @@ -1160,48 +1160,50 @@ static int check_channel_list(struct comedi_device *dev,
>> return 0;
>> }
>>
>> - if (n_chan > 1) {
>> - chansegment[0] = chanlist[0]; /* first channel is every time ok */
>> - for (i = 1, seglen = 1; i < n_chan; i++, seglen++) { /* build part of chanlist */
>> - /* printk("%d. %d %d\n",i,CR_CHAN(chanlist[i]),CR_RANGE(chanlist[i])); */
>> - if (chanlist[0] == chanlist[i])
>> - break; /* we detect loop, this must by finish */
>> - if (CR_CHAN(chanlist[i]) & 1) /* odd channel cann't by differencial */
>> - if (CR_AREF(chanlist[i]) == AREF_DIFF) {
>> - comedi_error(dev,
>> - "Odd channel can't be differential input!\n");
>> - return 0;
>> - }
>> - nowmustbechan =
>> - (CR_CHAN(chansegment[i - 1]) + 1) % s->n_chan;
>> - if (CR_AREF(chansegment[i - 1]) == AREF_DIFF)
>> - nowmustbechan = (nowmustbechan + 1) % s->n_chan;
>> - if (nowmustbechan != CR_CHAN(chanlist[i])) { /* channel list isn't continuous :-( */
>> - printk
>> - ("channel list must be continuous! chanlist[%i]=%d but must be %d or %d!\n",
>> - i, CR_CHAN(chanlist[i]), nowmustbechan,
>> - CR_CHAN(chanlist[0]));
>> + if (n_chan == 1)
>> + return 1; /* seglen=1 */
>> +
>> + /* n_chan must be >1 */
>> + /* first channel is every time ok */
>> + chansegment[0] = chanlist[0];
>> + /* build part of chanlist */
>> + for (i = 1, seglen = 1; i < n_chan; i++, seglen++) {
>> + if (chanlist[0] == chanlist[i])
>> + break; /* detected loop, done */
>> + if (CR_CHAN(chanlist[i]) & 1) {
>> + if (CR_AREF(chanlist[i]) == AREF_DIFF) {
>> + comedi_error(dev,
>> + "Odd channel can't be differential input!\n");
>> return 0;
>> }
>> - chansegment[i] = chanlist[i]; /* well, this is next correct channel in list */
>> }
>> -
>> - for (i = 0, segpos = 0; i < n_chan; i++) { /* check whole chanlist */
>> - /* printk("%d %d=%d %d\n",CR_CHAN(chansegment[i%seglen]),CR_RANGE(chansegment[i%seglen]),CR_CHAN(chanlist[i]),CR_RANGE(chanlist[i])); */
>> - if (chanlist[i] != chansegment[i % seglen]) {
>> - printk
>> - ("bad channel, reference or range number! chanlist[%i]=%d,%d,%d and not %d,%d,%d!\n",
>> - i, CR_CHAN(chansegment[i]),
>> - CR_RANGE(chansegment[i]),
>> - CR_AREF(chansegment[i]),
>> - CR_CHAN(chanlist[i % seglen]),
>> - CR_RANGE(chanlist[i % seglen]),
>> - CR_AREF(chansegment[i % seglen]));
>> - return 0; /* chan/gain list is strange */
>> - }
>> + nowmustbechan = (CR_CHAN(chansegment[i - 1]) + 1) % s->n_chan;
>> + if (CR_AREF(chansegment[i - 1]) == AREF_DIFF)
>> + nowmustbechan = (nowmustbechan + 1) % s->n_chan;
>> + /* channel list isn't continuous :-( */
>> + if (nowmustbechan != CR_CHAN(chanlist[i])) {
>> + printk(
>> + "channel list must be continuous! chanlist[%i]=%d but must be %d or %d!\n",
>> + i, CR_CHAN(chanlist[i]), nowmustbechan,
>> + CR_CHAN(chanlist[0]));
>> + return 0;
>> + }
>> + /* well, this is next correct channel in list */
>> + chansegment[i] = chanlist[i];
>> + }
>> + /* check whole chanlist */
>> + for (i = 0, segpos = 0; i < n_chan; i++) {
>> + if (chanlist[i] != chansegment[i % seglen]) {
>> + printk(
>> + "bad channel, reference or range number! chanlist[%i]=%d,%d,%d and not %d,%d,%d!\n",
>> + i, CR_CHAN(chansegment[i]),
>> + CR_RANGE(chansegment[i]),
>> + CR_AREF(chansegment[i]),
>> + CR_CHAN(chanlist[i % seglen]),
>> + CR_RANGE(chanlist[i % seglen]),
>> + CR_AREF(chansegment[i % seglen]));
>> + return 0; /* chan/gain list is strange */
>> }
>> - } else {
>> - seglen = 1;
>> }
>> return seglen;
>> }
>
> hi Tomas,
> the fix is nice but the org code has if (!n_char > 1) return 1 but now you have
> if (nchar == 1 ) is there any reason not to use ( nchar <= 1 ) ?
> (i do not see the whole code and it is not clear from your changelog)
>
> re,
> wh
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
