* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.