On Tue, 7 Mar 2023, Khadija wrote: > Hey Julia! Thank you for the feedback. I will make the following changes and > resend the patch: > 1. Correct the patch description that is right under the subject line (make > it precise and imperative) and make sure that it does not have more than 70 > characters per line. > 2. Adjust all the arguments of wait_event_interruptible_timeout so that they > are lined up. All of them should begin right under ( . The problem here is that the ( is really far to the right. My opinion is that the position of the second argument (ie the first one that is on a line of its own) is ok in this case. So you can leave that one where it is and line up the other one. julia > Kindly let me know if I have understood it > right.[EIu7f6EdNusD0UITKM3h?rid=EIu7f6EdNusD0UITKM3h] > > On Tue, Mar 7, 2023 at 2:08 AM Julia Lawall <julia.lawall@xxxxxxxx> wrote: > > > 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@xxxxxxxxx> > > --- > > 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 > > > > > > > > >
