Date:   Mon, 6 Mar 2023 22:08:21 +0100 (CET)
From:   Julia Lawall <julia.lawall@inria.fr>
To:     Khadija Kamran <kamrankhadijadj@gmail.com>
cc:     outreachy@lists.linux.dev, linux-staging@lists.linux.dev,
        gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: axis-fifo: alignment should match opening
 parenthesis in axis-fifo.c
In-Reply-To: <ZAZSmPpB6fcozGa4@khadija-virtual-machine>
Message-ID: <alpine.DEB.2.22.394.2303062202500.3050@hadrien>
References: <ZAZSmPpB6fcozGa4@khadija-virtual-machine>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk


On Tue, 7 Mar 2023, Khadija Kamran wrote:

> In file drivers/staging/axis-fifo/axis-fifo.c the alignment did not match the opening parenthesis. So, a few tabs were added to match the alignment to exactly where the parenthesis started.

Hello Khadija,

Thanks for plunging in and being the first participant!

However, there are a number of issues with the proposed patch.

1.  The log message should be at most around 70 characters wide.  You have
one long line.

2.  The log message should be written in the imperative.  Instead of "a
few tabs were added", ay "add a few tabs".

3.  I'm not sure that it is worth creating a very long line to respect the
rule about (.  On the other hand, the way the code is written at the
moment seems to be very misleading, because the third argument to
wait_event_interruptible_timeout is written as though it is the second
argument to ioread32.  So you can adjust the argument list of
wait_event_interruptible_timeout so that at least all of the arguments
that are not on the same line as the function call are lined up.

julia

>
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> ---
>  drivers/staging/axis-fifo/axis-fifo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index dfd2b357f484..6e959224add0 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -383,7 +383,7 @@ static ssize_t axis_fifo_read(struct file *f, char __user *buf,
>  		 */
>  		mutex_lock(&fifo->read_lock);
>  		ret = wait_event_interruptible_timeout(fifo->read_queue,
> -			ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> +						       ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
>  				 (read_timeout >= 0) ?
>  				  msecs_to_jiffies(read_timeout) :
>  				  MAX_SCHEDULE_TIMEOUT);
> --
> 2.34.1
>
>
>