Am 02.05.2012 20:10, schrieb Tomas Melin:
> 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
>
ok i understand.
>>
>> 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
