* [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference @ 2017-02-20 8:28 Cheah Kok Cheong 2017-02-20 10:03 ` Ian Abbott 0 siblings, 1 reply; 10+ messages in thread From: Cheah Kok Cheong @ 2017-02-20 8:28 UTC (permalink / raw) To: abbotti, hsweeten, gregkh, devel; +Cc: linux-kernel, Cheah Kok Cheong Fix checkpatch warning "Avoid multiple line dereference" using a local variable to avoid line wrap. Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com> --- drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c index 2a063f0..fde83e0 100644 --- a/drivers/staging/comedi/drivers/comedi_test.c +++ b/drivers/staging/comedi/drivers/comedi_test.c @@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) /* output the last scan */ for (i = 0; i < cmd->scan_end_arg; i++) { unsigned int chan = CR_CHAN(cmd->chanlist[i]); + unsigned short d = devpriv->ao_loopbacks[chan]; - if (comedi_buf_read_samples(s, - &devpriv-> - ao_loopbacks[chan], - 1) == 0) { + if (!comedi_buf_read_samples(s, &d, 1)) { /* unexpected underrun! (cancelled?) */ async->events |= COMEDI_CB_OVERFLOW; goto underrun; -- 2.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-20 8:28 [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference Cheah Kok Cheong @ 2017-02-20 10:03 ` Ian Abbott 2017-02-20 16:02 ` Cheah Kok Cheong 0 siblings, 1 reply; 10+ messages in thread From: Ian Abbott @ 2017-02-20 10:03 UTC (permalink / raw) To: Cheah Kok Cheong, hsweeten, gregkh, devel; +Cc: linux-kernel On 20/02/17 08:28, Cheah Kok Cheong wrote: > Fix checkpatch warning "Avoid multiple line dereference" > using a local variable to avoid line wrap. > > Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com> > --- > drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c > index 2a063f0..fde83e0 100644 > --- a/drivers/staging/comedi/drivers/comedi_test.c > +++ b/drivers/staging/comedi/drivers/comedi_test.c > @@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) > /* output the last scan */ > for (i = 0; i < cmd->scan_end_arg; i++) { > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > + unsigned short d = devpriv->ao_loopbacks[chan]; > > - if (comedi_buf_read_samples(s, > - &devpriv-> > - ao_loopbacks[chan], > - 1) == 0) { > + if (!comedi_buf_read_samples(s, &d, 1)) { > /* unexpected underrun! (cancelled?) */ > async->events |= COMEDI_CB_OVERFLOW; > goto underrun; > NAK. This leaves devpriv->ao_loopbacks[chan] unchanged. -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Web: http://www.mev.co.uk/ )=- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-20 10:03 ` Ian Abbott @ 2017-02-20 16:02 ` Cheah Kok Cheong 2017-02-20 17:36 ` Ian Abbott 0 siblings, 1 reply; 10+ messages in thread From: Cheah Kok Cheong @ 2017-02-20 16:02 UTC (permalink / raw) To: Ian Abbott; +Cc: hsweeten, gregkh, devel, linux-kernel On Mon, Feb 20, 2017 at 10:03:39AM +0000, Ian Abbott wrote: > On 20/02/17 08:28, Cheah Kok Cheong wrote: > >Fix checkpatch warning "Avoid multiple line dereference" > >using a local variable to avoid line wrap. > > > >Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com> > >--- > > drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > >diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c > >index 2a063f0..fde83e0 100644 > >--- a/drivers/staging/comedi/drivers/comedi_test.c > >+++ b/drivers/staging/comedi/drivers/comedi_test.c > >@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) > > /* output the last scan */ > > for (i = 0; i < cmd->scan_end_arg; i++) { > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > >+ unsigned short d = devpriv->ao_loopbacks[chan]; > > > >- if (comedi_buf_read_samples(s, > >- &devpriv-> > >- ao_loopbacks[chan], > >- 1) == 0) { > >+ if (!comedi_buf_read_samples(s, &d, 1)) { > > /* unexpected underrun! (cancelled?) */ > > async->events |= COMEDI_CB_OVERFLOW; > > goto underrun; > > > > NAK. This leaves devpriv->ao_loopbacks[chan] unchanged. > Thanks for pointing this out. In that case will assigning the variable to devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet. Otherwise I'll just drop the variable and adjust the lines to avoid checkpatch warning. Sorry for the inconvenience caused. [ Snip ] /* output the last scan */ for (i = 0; i < cmd->scan_end_arg; i++) { unsigned int chan = CR_CHAN(cmd->chanlist[i]); unsigned short data; if (!comedi_buf_read_samples(s, &data, 1)) { /* unexpected underrun! (cancelled?) */ async->events |= COMEDI_CB_OVERFLOW; goto underrun; } devpriv->ao_loopbacks[chan] = data; } /* advance time of last scan */ [ Snip ] Thks. Brgds, CheahKC ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-20 16:02 ` Cheah Kok Cheong @ 2017-02-20 17:36 ` Ian Abbott 2017-02-21 9:33 ` Cheah Kok Cheong 0 siblings, 1 reply; 10+ messages in thread From: Ian Abbott @ 2017-02-20 17:36 UTC (permalink / raw) To: Cheah Kok Cheong; +Cc: hsweeten, gregkh, devel, linux-kernel On 20/02/17 16:02, Cheah Kok Cheong wrote: > On Mon, Feb 20, 2017 at 10:03:39AM +0000, Ian Abbott wrote: >> On 20/02/17 08:28, Cheah Kok Cheong wrote: >>> Fix checkpatch warning "Avoid multiple line dereference" >>> using a local variable to avoid line wrap. >>> >>> Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com> >>> --- >>> drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c >>> index 2a063f0..fde83e0 100644 >>> --- a/drivers/staging/comedi/drivers/comedi_test.c >>> +++ b/drivers/staging/comedi/drivers/comedi_test.c >>> @@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) >>> /* output the last scan */ >>> for (i = 0; i < cmd->scan_end_arg; i++) { >>> unsigned int chan = CR_CHAN(cmd->chanlist[i]); >>> + unsigned short d = devpriv->ao_loopbacks[chan]; >>> >>> - if (comedi_buf_read_samples(s, >>> - &devpriv-> >>> - ao_loopbacks[chan], >>> - 1) == 0) { >>> + if (!comedi_buf_read_samples(s, &d, 1)) { >>> /* unexpected underrun! (cancelled?) */ >>> async->events |= COMEDI_CB_OVERFLOW; >>> goto underrun; >>> >> >> NAK. This leaves devpriv->ao_loopbacks[chan] unchanged. >> > > Thanks for pointing this out. In that case will assigning the variable to > devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet. > > Otherwise I'll just drop the variable and adjust the lines to avoid > checkpatch warning. > > Sorry for the inconvenience caused. > > [ Snip ] > > /* output the last scan */ > for (i = 0; i < cmd->scan_end_arg; i++) { > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > unsigned short data; > > if (!comedi_buf_read_samples(s, &data, 1)) { > /* unexpected underrun! (cancelled?) */ > async->events |= COMEDI_CB_OVERFLOW; > goto underrun; > } > > devpriv->ao_loopbacks[chan] = data; > } > /* advance time of last scan */ > > [ Snip ] It will work, but you could just use a pointer variable set to &devpriv->ao_loopbacks[chan] and pass that to comedi_buf_read_samples(). -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Web: http://www.mev.co.uk/ )=- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-20 17:36 ` Ian Abbott @ 2017-02-21 9:33 ` Cheah Kok Cheong 2017-02-21 10:12 ` Ian Abbott 0 siblings, 1 reply; 10+ messages in thread From: Cheah Kok Cheong @ 2017-02-21 9:33 UTC (permalink / raw) To: Ian Abbott; +Cc: hsweeten, gregkh, devel, linux-kernel On Mon, Feb 20, 2017 at 05:36:52PM +0000, Ian Abbott wrote: > On 20/02/17 16:02, Cheah Kok Cheong wrote: > >On Mon, Feb 20, 2017 at 10:03:39AM +0000, Ian Abbott wrote: > >>On 20/02/17 08:28, Cheah Kok Cheong wrote: > >>>Fix checkpatch warning "Avoid multiple line dereference" > >>>using a local variable to avoid line wrap. > >>> > >>>Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com> > >>>--- > >>>drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- > >>>1 file changed, 2 insertions(+), 4 deletions(-) > >>> > >>>diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c > >>>index 2a063f0..fde83e0 100644 > >>>--- a/drivers/staging/comedi/drivers/comedi_test.c > >>>+++ b/drivers/staging/comedi/drivers/comedi_test.c > >>>@@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) > >>> /* output the last scan */ > >>> for (i = 0; i < cmd->scan_end_arg; i++) { > >>> unsigned int chan = CR_CHAN(cmd->chanlist[i]); > >>>+ unsigned short d = devpriv->ao_loopbacks[chan]; > >>> > >>>- if (comedi_buf_read_samples(s, > >>>- &devpriv-> > >>>- ao_loopbacks[chan], > >>>- 1) == 0) { > >>>+ if (!comedi_buf_read_samples(s, &d, 1)) { > >>> /* unexpected underrun! (cancelled?) */ > >>> async->events |= COMEDI_CB_OVERFLOW; > >>> goto underrun; > >>> > >> > >>NAK. This leaves devpriv->ao_loopbacks[chan] unchanged. > >> > > > >Thanks for pointing this out. In that case will assigning the variable to > >devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet. > > > >Otherwise I'll just drop the variable and adjust the lines to avoid > >checkpatch warning. > > > >Sorry for the inconvenience caused. > > > >[ Snip ] > > > > /* output the last scan */ > > for (i = 0; i < cmd->scan_end_arg; i++) { > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > unsigned short data; > > > > if (!comedi_buf_read_samples(s, &data, 1)) { > > /* unexpected underrun! (cancelled?) */ > > async->events |= COMEDI_CB_OVERFLOW; > > goto underrun; > > } > > > > devpriv->ao_loopbacks[chan] = data; > > } > > /* advance time of last scan */ > > > >[ Snip ] > > It will work, but you could just use a pointer variable set to > &devpriv->ao_loopbacks[chan] and pass that to comedi_buf_read_samples(). > Thanks for the suggestion. I tried below snippet 1 with the shortest pointer name but 80 characters is exceeded. The declaration and initialisation will have to be splitted. Will this be acceptable or am I doing it wrong again? Sorry for the trouble. Snippet 1: [ Snip ] /* output the last scan */ for (i = 0; i < cmd->scan_end_arg; i++) { unsigned int chan = CR_CHAN(cmd->chanlist[i]); unsigned short *p = &devpriv->ao_loopbacks[chan]; if (!comedi_buf_read_samples(s, p, 1)) { /* unexpected underrun! (cancelled?) */ async->events |= COMEDI_CB_OVERFLOW; goto underrun; } } /* advance time of last scan */ [ Snip ] Snippet 2: [ Snip ] /* output the last scan */ for (i = 0; i < cmd->scan_end_arg; i++) { unsigned int chan = CR_CHAN(cmd->chanlist[i]); unsigned short *pd; pd = &devpriv->ao_loopbacks[chan]; if (!comedi_buf_read_samples(s, pd, 1)) { /* unexpected underrun! (cancelled?) */ async->events |= COMEDI_CB_OVERFLOW; goto underrun; } } [ Snip ] Thks. Brgds, CheahKC ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-21 9:33 ` Cheah Kok Cheong @ 2017-02-21 10:12 ` Ian Abbott 2017-02-21 10:20 ` Valentin Rothberg 0 siblings, 1 reply; 10+ messages in thread From: Ian Abbott @ 2017-02-21 10:12 UTC (permalink / raw) To: Cheah Kok Cheong; +Cc: hsweeten, gregkh, devel, linux-kernel On 21/02/2017 09:33, Cheah Kok Cheong wrote: > On Mon, Feb 20, 2017 at 05:36:52PM +0000, Ian Abbott wrote: >> On 20/02/17 16:02, Cheah Kok Cheong wrote: >>> On Mon, Feb 20, 2017 at 10:03:39AM +0000, Ian Abbott wrote: >>>> On 20/02/17 08:28, Cheah Kok Cheong wrote: >>>>> Fix checkpatch warning "Avoid multiple line dereference" >>>>> using a local variable to avoid line wrap. >>>>> >>>>> Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com> >>>>> --- >>>>> drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- >>>>> 1 file changed, 2 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c >>>>> index 2a063f0..fde83e0 100644 >>>>> --- a/drivers/staging/comedi/drivers/comedi_test.c >>>>> +++ b/drivers/staging/comedi/drivers/comedi_test.c >>>>> @@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) >>>>> /* output the last scan */ >>>>> for (i = 0; i < cmd->scan_end_arg; i++) { >>>>> unsigned int chan = CR_CHAN(cmd->chanlist[i]); >>>>> + unsigned short d = devpriv->ao_loopbacks[chan]; >>>>> >>>>> - if (comedi_buf_read_samples(s, >>>>> - &devpriv-> >>>>> - ao_loopbacks[chan], >>>>> - 1) == 0) { >>>>> + if (!comedi_buf_read_samples(s, &d, 1)) { >>>>> /* unexpected underrun! (cancelled?) */ >>>>> async->events |= COMEDI_CB_OVERFLOW; >>>>> goto underrun; >>>>> >>>> >>>> NAK. This leaves devpriv->ao_loopbacks[chan] unchanged. >>>> >>> >>> Thanks for pointing this out. In that case will assigning the variable to >>> devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet. >>> >>> Otherwise I'll just drop the variable and adjust the lines to avoid >>> checkpatch warning. >>> >>> Sorry for the inconvenience caused. >>> >>> [ Snip ] >>> >>> /* output the last scan */ >>> for (i = 0; i < cmd->scan_end_arg; i++) { >>> unsigned int chan = CR_CHAN(cmd->chanlist[i]); >>> unsigned short data; >>> >>> if (!comedi_buf_read_samples(s, &data, 1)) { >>> /* unexpected underrun! (cancelled?) */ >>> async->events |= COMEDI_CB_OVERFLOW; >>> goto underrun; >>> } >>> >>> devpriv->ao_loopbacks[chan] = data; >>> } >>> /* advance time of last scan */ >>> >>> [ Snip ] >> >> It will work, but you could just use a pointer variable set to >> &devpriv->ao_loopbacks[chan] and pass that to comedi_buf_read_samples(). >> > > Thanks for the suggestion. I tried below snippet 1 with the shortest pointer > name but 80 characters is exceeded. The declaration and initialisation > will have to be splitted. Will this be acceptable or am I doing it wrong > again? > > Sorry for the trouble. > > Snippet 1: > [ Snip ] > > /* output the last scan */ > for (i = 0; i < cmd->scan_end_arg; i++) { > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > unsigned short *p = &devpriv->ao_loopbacks[chan]; > > if (!comedi_buf_read_samples(s, p, 1)) { > /* unexpected underrun! (cancelled?) */ > async->events |= COMEDI_CB_OVERFLOW; > goto underrun; > } > } > /* advance time of last scan */ > > [ Snip ] > > Snippet 2: > [ Snip ] > > /* output the last scan */ > for (i = 0; i < cmd->scan_end_arg; i++) { > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > unsigned short *pd; > > pd = &devpriv->ao_loopbacks[chan]; > > if (!comedi_buf_read_samples(s, pd, 1)) { > /* unexpected underrun! (cancelled?) */ > async->events |= COMEDI_CB_OVERFLOW; > goto underrun; > } > } > > [ Snip ] Snippet 2 looks fine. Alternatives are to modify Snippet 1 to split the initialization of the pointer variable after the '=', or to shorten the the name of the 'chan' variable. -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Web: http://www.mev.co.uk/ )=- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-21 10:12 ` Ian Abbott @ 2017-02-21 10:20 ` Valentin Rothberg 2017-02-21 16:31 ` Cheah Kok Cheong 0 siblings, 1 reply; 10+ messages in thread From: Valentin Rothberg @ 2017-02-21 10:20 UTC (permalink / raw) To: Ian Abbott; +Cc: Cheah Kok Cheong, hsweeten, gregkh, devel, linux-kernel On Feb 21 '17 10:12, Ian Abbott wrote: > On 21/02/2017 09:33, Cheah Kok Cheong wrote: > > On Mon, Feb 20, 2017 at 05:36:52PM +0000, Ian Abbott wrote: > > > On 20/02/17 16:02, Cheah Kok Cheong wrote: > > > > On Mon, Feb 20, 2017 at 10:03:39AM +0000, Ian Abbott wrote: > > > > > On 20/02/17 08:28, Cheah Kok Cheong wrote: > > > > > > Fix checkpatch warning "Avoid multiple line dereference" > > > > > > using a local variable to avoid line wrap. > > > > > > > > > > > > Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com> > > > > > > --- > > > > > > drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- > > > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c > > > > > > index 2a063f0..fde83e0 100644 > > > > > > --- a/drivers/staging/comedi/drivers/comedi_test.c > > > > > > +++ b/drivers/staging/comedi/drivers/comedi_test.c > > > > > > @@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) > > > > > > /* output the last scan */ > > > > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > > > > + unsigned short d = devpriv->ao_loopbacks[chan]; > > > > > > > > > > > > - if (comedi_buf_read_samples(s, > > > > > > - &devpriv-> > > > > > > - ao_loopbacks[chan], > > > > > > - 1) == 0) { > > > > > > + if (!comedi_buf_read_samples(s, &d, 1)) { > > > > > > /* unexpected underrun! (cancelled?) */ > > > > > > async->events |= COMEDI_CB_OVERFLOW; > > > > > > goto underrun; > > > > > > > > > > > > > > > > NAK. This leaves devpriv->ao_loopbacks[chan] unchanged. > > > > > > > > > > > > > Thanks for pointing this out. In that case will assigning the variable to > > > > devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet. > > > > > > > > Otherwise I'll just drop the variable and adjust the lines to avoid > > > > checkpatch warning. > > > > > > > > Sorry for the inconvenience caused. > > > > > > > > [ Snip ] > > > > > > > > /* output the last scan */ > > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > > unsigned short data; > > > > > > > > if (!comedi_buf_read_samples(s, &data, 1)) { > > > > /* unexpected underrun! (cancelled?) */ > > > > async->events |= COMEDI_CB_OVERFLOW; > > > > goto underrun; > > > > } > > > > > > > > devpriv->ao_loopbacks[chan] = data; > > > > } > > > > /* advance time of last scan */ > > > > > > > > [ Snip ] > > > > > > It will work, but you could just use a pointer variable set to > > > &devpriv->ao_loopbacks[chan] and pass that to comedi_buf_read_samples(). > > > > > > > Thanks for the suggestion. I tried below snippet 1 with the shortest pointer > > name but 80 characters is exceeded. The declaration and initialisation > > will have to be splitted. Will this be acceptable or am I doing it wrong > > again? > > > > Sorry for the trouble. > > > > Snippet 1: > > [ Snip ] > > > > /* output the last scan */ > > for (i = 0; i < cmd->scan_end_arg; i++) { > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > unsigned short *p = &devpriv->ao_loopbacks[chan]; > > > > if (!comedi_buf_read_samples(s, p, 1)) { > > /* unexpected underrun! (cancelled?) */ > > async->events |= COMEDI_CB_OVERFLOW; > > goto underrun; > > } > > } > > /* advance time of last scan */ > > > > [ Snip ] > > > > Snippet 2: > > [ Snip ] > > > > /* output the last scan */ > > for (i = 0; i < cmd->scan_end_arg; i++) { > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > unsigned short *pd; > > > > pd = &devpriv->ao_loopbacks[chan]; > > > > if (!comedi_buf_read_samples(s, pd, 1)) { > > /* unexpected underrun! (cancelled?) */ > > async->events |= COMEDI_CB_OVERFLOW; > > goto underrun; > > } > > } > > > > [ Snip ] > > Snippet 2 looks fine. Alternatives are to modify Snippet 1 to split the > initialization of the pointer variable after the '=', or to shorten the the > name of the 'chan' variable. Another option could be using the typedefs from include/linux/types.h, e.g. ushort. However, this might require changing other declarations as well to keep consistency. > -- > -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- > -=( Web: http://www.mev.co.uk/ )=- ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-21 10:20 ` Valentin Rothberg @ 2017-02-21 16:31 ` Cheah Kok Cheong 2017-02-21 17:22 ` Joe Perches 0 siblings, 1 reply; 10+ messages in thread From: Cheah Kok Cheong @ 2017-02-21 16:31 UTC (permalink / raw) To: Valentin Rothberg; +Cc: Ian Abbott, hsweeten, gregkh, devel, linux-kernel On Tue, Feb 21, 2017 at 11:20:08AM +0100, Valentin Rothberg wrote: > On Feb 21 '17 10:12, Ian Abbott wrote: > > On 21/02/2017 09:33, Cheah Kok Cheong wrote: > > > On Mon, Feb 20, 2017 at 05:36:52PM +0000, Ian Abbott wrote: > > > > On 20/02/17 16:02, Cheah Kok Cheong wrote: > > > > > On Mon, Feb 20, 2017 at 10:03:39AM +0000, Ian Abbott wrote: > > > > > > On 20/02/17 08:28, Cheah Kok Cheong wrote: > > > > > > > Fix checkpatch warning "Avoid multiple line dereference" > > > > > > > using a local variable to avoid line wrap. > > > > > > > > > > > > > > Signed-off-by: Cheah Kok Cheong <thrust73@gmail.com> > > > > > > > --- > > > > > > > drivers/staging/comedi/drivers/comedi_test.c | 6 ++---- > > > > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/staging/comedi/drivers/comedi_test.c b/drivers/staging/comedi/drivers/comedi_test.c > > > > > > > index 2a063f0..fde83e0 100644 > > > > > > > --- a/drivers/staging/comedi/drivers/comedi_test.c > > > > > > > +++ b/drivers/staging/comedi/drivers/comedi_test.c > > > > > > > @@ -480,11 +480,9 @@ static void waveform_ao_timer(unsigned long arg) > > > > > > > /* output the last scan */ > > > > > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > > > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > > > > > + unsigned short d = devpriv->ao_loopbacks[chan]; > > > > > > > > > > > > > > - if (comedi_buf_read_samples(s, > > > > > > > - &devpriv-> > > > > > > > - ao_loopbacks[chan], > > > > > > > - 1) == 0) { > > > > > > > + if (!comedi_buf_read_samples(s, &d, 1)) { > > > > > > > /* unexpected underrun! (cancelled?) */ > > > > > > > async->events |= COMEDI_CB_OVERFLOW; > > > > > > > goto underrun; > > > > > > > > > > > > > > > > > > > NAK. This leaves devpriv->ao_loopbacks[chan] unchanged. > > > > > > > > > > > > > > > > Thanks for pointing this out. In that case will assigning the variable to > > > > > devpriv->ao_loopbacks[chan] be acceptable? Please review below snippet. > > > > > > > > > > Otherwise I'll just drop the variable and adjust the lines to avoid > > > > > checkpatch warning. > > > > > > > > > > Sorry for the inconvenience caused. > > > > > > > > > > [ Snip ] > > > > > > > > > > /* output the last scan */ > > > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > > > unsigned short data; > > > > > > > > > > if (!comedi_buf_read_samples(s, &data, 1)) { > > > > > /* unexpected underrun! (cancelled?) */ > > > > > async->events |= COMEDI_CB_OVERFLOW; > > > > > goto underrun; > > > > > } > > > > > > > > > > devpriv->ao_loopbacks[chan] = data; > > > > > } > > > > > /* advance time of last scan */ > > > > > > > > > > [ Snip ] > > > > > > > > It will work, but you could just use a pointer variable set to > > > > &devpriv->ao_loopbacks[chan] and pass that to comedi_buf_read_samples(). > > > > > > > > > > Thanks for the suggestion. I tried below snippet 1 with the shortest pointer > > > name but 80 characters is exceeded. The declaration and initialisation > > > will have to be splitted. Will this be acceptable or am I doing it wrong > > > again? > > > > > > Sorry for the trouble. > > > > > > Snippet 1: > > > [ Snip ] > > > > > > /* output the last scan */ > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > unsigned short *p = &devpriv->ao_loopbacks[chan]; > > > > > > if (!comedi_buf_read_samples(s, p, 1)) { > > > /* unexpected underrun! (cancelled?) */ > > > async->events |= COMEDI_CB_OVERFLOW; > > > goto underrun; > > > } > > > } > > > /* advance time of last scan */ > > > > > > [ Snip ] > > > > > > Snippet 2: > > > [ Snip ] > > > > > > /* output the last scan */ > > > for (i = 0; i < cmd->scan_end_arg; i++) { > > > unsigned int chan = CR_CHAN(cmd->chanlist[i]); > > > unsigned short *pd; > > > > > > pd = &devpriv->ao_loopbacks[chan]; > > > > > > if (!comedi_buf_read_samples(s, pd, 1)) { > > > /* unexpected underrun! (cancelled?) */ > > > async->events |= COMEDI_CB_OVERFLOW; > > > goto underrun; > > > } > > > } > > > > > > [ Snip ] > > > > Snippet 2 looks fine. Alternatives are to modify Snippet 1 to split the > > initialization of the pointer variable after the '=', or to shorten the the > > name of the 'chan' variable. I'm tempted to shorten the 'chan' variable but this will break consistency since it's also use in static int waveform_ai_insn_read() and static int waveform_ao_insn_write(). I'll send Snippet 2 as V2. > > Another option could be using the typedefs from include/linux/types.h, > e.g. ushort. However, this might require changing other declarations as > well to keep consistency. Thanks for the idea. I counted seven instances of 'unsigned short' in this file so it's not viable for our situation. Thks. Brgds, CheahKC ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-21 16:31 ` Cheah Kok Cheong @ 2017-02-21 17:22 ` Joe Perches 2017-02-22 8:30 ` Valentin Rothberg 0 siblings, 1 reply; 10+ messages in thread From: Joe Perches @ 2017-02-21 17:22 UTC (permalink / raw) To: Cheah Kok Cheong, Valentin Rothberg Cc: Ian Abbott, hsweeten, gregkh, devel, linux-kernel On Wed, 2017-02-22 at 00:31 +0800, Cheah Kok Cheong wrote: > > Another option could be using the typedefs from include/linux/types.h, > > e.g. ushort. However, this might require changing other declarations as > > well to keep consistency. > > Thanks for the idea. I counted seven instances of 'unsigned short' > in this file so it's not viable for our situation. I believe using ushort is relatively undesirable as "unsigned short" is preferred ~10:1 in the kernel $ git grep -w ushort | wc -l 1381 $ git grep -E "\bunsigned\s+short\b" | wc -l 11288 $ git grep --name-only -w "ushort" | wc -l 129 $ git grep --name-only -E "\bunsigned\s+short\b" | wc -l 2497 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference 2017-02-21 17:22 ` Joe Perches @ 2017-02-22 8:30 ` Valentin Rothberg 0 siblings, 0 replies; 10+ messages in thread From: Valentin Rothberg @ 2017-02-22 8:30 UTC (permalink / raw) To: Joe Perches Cc: Cheah Kok Cheong, Ian Abbott, hsweeten, gregkh, devel, linux-kernel On Feb 21 '17 09:22, Joe Perches wrote: > On Wed, 2017-02-22 at 00:31 +0800, Cheah Kok Cheong wrote: > > > Another option could be using the typedefs from include/linux/types.h, > > > e.g. ushort. However, this might require changing other declarations as > > > well to keep consistency. > > > > Thanks for the idea. I counted seven instances of 'unsigned short' > > in this file so it's not viable for our situation. > > I believe using ushort is relatively undesirable as > "unsigned short" is preferred ~10:1 in the kernel Wow, that is suprising to me considering the 80 character limit. Thanks for pointing this out! Regards, Valentin > $ git grep -w ushort | wc -l > 1381 > $ git grep -E "\bunsigned\s+short\b" | wc -l > 11288 > > $ git grep --name-only -w "ushort" | wc -l > 129 > $ git grep --name-only -E "\bunsigned\s+short\b" | wc -l > 2497 > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-02-22 8:30 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-20 8:28 [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference Cheah Kok Cheong 2017-02-20 10:03 ` Ian Abbott 2017-02-20 16:02 ` Cheah Kok Cheong 2017-02-20 17:36 ` Ian Abbott 2017-02-21 9:33 ` Cheah Kok Cheong 2017-02-21 10:12 ` Ian Abbott 2017-02-21 10:20 ` Valentin Rothberg 2017-02-21 16:31 ` Cheah Kok Cheong 2017-02-21 17:22 ` Joe Perches 2017-02-22 8:30 ` Valentin Rothberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox