Re: [Outreachy kernel] [RESEND PATCH] staging: emxx_udc: Ending line with argument

[Beatriz Martins de Carvalho]

Em 06/04/21 20:56, Matthew Wilcox escreveu:

On Tue, Apr 06, 2021 at 08:34:09PM +0100, Beatriz Martins de Carvalho wrote:

Cleans up check of "Lines should not end with a '('"
with argument present in next line in file emxx_udc.c

I appreciate that you've removed the checkpatch warning, but this is
still harder to read than the original used to be.

Thank you for your review.

-                               _nbu2ss_writel(
-                                       &preg->EP_REGS[ep->epnum - 1].EP_WRITE,
-                                       p_buf_32->dw);
+                               _nbu2ss_writel(&preg->EP_REGS[ep->epnum - 
1].EP_WRITE,
+                                              p_buf_32->dw);
-               length = _nbu2ss_readl(
-                       &ep->udc->p_regs->EP_REGS[ep->epnum - 1].EP_LEN_DCNT);
+               length = _nbu2ss_readl(&ep->udc->p_regs->EP_REGS[ep->epnum - 
1].EP_LEN_DCNT);
-                       regdata = _nbu2ss_readl(
-                               &preg->EP_REGS[ep->epnum - 1].EP_STATUS);
+                       regdata = _nbu2ss_readl(&preg->EP_REGS[ep->epnum - 
1].EP_STATUS);

The real problem with this driver is that their abstraction layer is
wrong.  For example:

         /* Interrupt Status */
         status = _nbu2ss_readl(&udc->p_regs->EP_REGS[num].EP_STATUS);

         /* Interrupt Clear */
         _nbu2ss_writel(&udc->p_regs->EP_REGS[num].EP_STATUS, ~status);

If instead this were:

        status = nbu2ss_read_ep_status(udc, num);
        nbu2ss_write_ep_status(udc, num, ~status);

that would be a lot shorter and clearer.  Cleanups along these lines
would be a lot more useful, and would fix the 80 column warning.

I can see what you mean and makes sense. Thank you.

Beatriz Martins de Carvalho