From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751676AbdBUKNG (ORCPT ); Tue, 21 Feb 2017 05:13:06 -0500 Received: from smtp73.iad3a.emailsrvr.com ([173.203.187.73]:39752 "EHLO smtp73.iad3a.emailsrvr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751107AbdBUKM7 (ORCPT ); Tue, 21 Feb 2017 05:12:59 -0500 X-Auth-ID: abbotti@mev.co.uk X-Sender-Id: abbotti@mev.co.uk Subject: Re: [PATCH] Staging: comedi: drivers: comedi_test: Avoid multiple line dereference To: Cheah Kok Cheong References: <1487579315-14775-1-git-send-email-thrust73@gmail.com> <20170220160205.GA4017@linux-Precision-WorkStation-T5500> <675e00f6-2e6c-e486-898d-b942c76e26a0@mev.co.uk> <20170221093333.GA2775@linux-Precision-WorkStation-T5500> Cc: hsweeten@visionengravers.com, gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org From: Ian Abbott Message-ID: <4b35e656-8fc2-13a5-663b-c8db45bed832@mev.co.uk> Date: Tue, 21 Feb 2017 10:12:57 +0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20170221093333.GA2775@linux-Precision-WorkStation-T5500> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>> --- >>>>> 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: )=- -=( Web: http://www.mev.co.uk/ )=-