[RESEND PATCH] staging: emxx_udc: Ending line with argument

[RESEND PATCH] staging: emxx_udc: Ending line with argument

[Beatriz Martins de Carvalho]

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

Signed-off-by: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>
---
 drivers/staging/emxx_udc/emxx_udc.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
index 741147a4f0fe..20f53cf6e20f 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -1073,9 +1073,8 @@ static int _nbu2ss_epn_in_pio(struct nbu2ss_udc *udc, struct nbu2ss_ep *ep,
 		i_word_length = length / sizeof(u32);
 		if (i_word_length > 0) {
 			for (i = 0; i < i_word_length; i++) {
-				_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);
 
 				p_buf_32++;
 			}
@@ -1225,8 +1224,7 @@ static void _nbu2ss_restert_transfer(struct nbu2ss_ep *ep)
 		return;
 
 	if (ep->epnum > 0) {
-		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);
 
 		length &= EPN_LDATA;
 		if (length < ep->ep.maxpacket)
@@ -1462,8 +1460,7 @@ static void _nbu2ss_epn_set_stall(struct nbu2ss_udc *udc,
 		for (limit_cnt = 0
 			; limit_cnt < IN_DATA_EMPTY_COUNT
 			; limit_cnt++) {
-			regdata = _nbu2ss_readl(
-				&preg->EP_REGS[ep->epnum - 1].EP_STATUS);
+			regdata = _nbu2ss_readl(&preg->EP_REGS[ep->epnum - 1].EP_STATUS);
 
 			if ((regdata & EPN_IN_DATA) == 0)
 				break;
-- 
2.25.1

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

[Greg KH]

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
> 
> Signed-off-by: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>
> ---
>  drivers/staging/emxx_udc/emxx_udc.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Why is this a [RESEND] ?

What happened to the first version?

Also, your subject is odd, please look at the documentation for how to
write good subject lines for patches.

thanks,

greg k-h

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

[Beatriz Martins de Carvalho]

Em 06/04/21 20:36, Greg KH 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
>>
>> Signed-off-by: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>
>> ---
>>   drivers/staging/emxx_udc/emxx_udc.c | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
> Why is this a [RESEND] ?
>
> What happened to the first version?
Sorry, I didn't receive your review, and in kernelnewbies tutorial, they 
say if not receive a response, may have missed the patch, so I resent it.
>
> Also, your subject is odd, please look at the documentation for how to
> write good subject lines for patches.

Yes, I know. It was my second patch, and I was learning, and when I 
resent it, I didn't know if I can change the subject.

>
> thanks,
Thanks for you review
>
> greg k-h
Beatriz Carvalho

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

[Greg KH]

On Tue, Apr 06, 2021 at 09:00:07PM +0100, Beatriz Martins de Carvalho wrote:
> 
> Em 06/04/21 20:36, Greg KH 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
> > > 
> > > Signed-off-by: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>
> > > ---
> > >   drivers/staging/emxx_udc/emxx_udc.c | 11 ++++-------
> > >   1 file changed, 4 insertions(+), 7 deletions(-)
> > Why is this a [RESEND] ?
> > 
> > What happened to the first version?
> Sorry, I didn't receive your review, and in kernelnewbies tutorial, they say
> if not receive a response, may have missed the patch, so I resent it.

Do you have a pointer to your previous patch in the lore.kernel.org
archives anywhere?  I can't seem to find it.

> > Also, your subject is odd, please look at the documentation for how to
> > write good subject lines for patches.
> 
> Yes, I know. It was my second patch, and I was learning, and when I resent
> it, I didn't know if I can change the subject.

Of course you can always fix up things you know you did incorrectly,
don't make others review stuff that you know is wrong, that just wastes
everyone's time.

thanks,

greg k-h

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

[Greg KH]

On Wed, Apr 07, 2021 at 09:16:44AM +0100, Beatriz Martins de Carvalho wrote:
> 
> Em 07/04/21 06:37, Greg KH escreveu:
> > On Tue, Apr 06, 2021 at 09:00:07PM +0100, Beatriz Martins de Carvalho wrote:
> > > Em 06/04/21 20:36, Greg KH 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
> > > > > 
> > > > > Signed-off-by: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>
> > > > > ---
> > > > >    drivers/staging/emxx_udc/emxx_udc.c | 11 ++++-------
> > > > >    1 file changed, 4 insertions(+), 7 deletions(-)
> > > > Why is this a [RESEND] ?
> > > > 
> > > > What happened to the first version?
> > > Sorry, I didn't receive your review, and in kernelnewbies tutorial, they say
> > > if not receive a response, may have missed the patch, so I resent it.
> > Do you have a pointer to your previous patch in the lore.kernel.org
> > archives anywhere?  I can't seem to find it.
> 
> 
> I found this, it's what is you need?
> 
> https://lore.kernel.org/linux-staging/20210401195457.24512-1-martinsdecarvalhobeatriz@gmail.com/

Ah, yes, I saw Julia's review and assumed you would fix up your patch
based on her comments.  Please do not ignore review comments, it makes
everyone's lives harder.

thanks,

greg k-h

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

[Beatriz Martins de Carvalho]

Em 07/04/21 09:34, Greg KH escreveu:
> On Wed, Apr 07, 2021 at 09:16:44AM +0100, Beatriz Martins de Carvalho wrote:
>> Em 07/04/21 06:37, Greg KH escreveu:
>>> On Tue, Apr 06, 2021 at 09:00:07PM +0100, Beatriz Martins de Carvalho wrote:
>>>> Em 06/04/21 20:36, Greg KH 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
>>>>>>
>>>>>> Signed-off-by: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>
>>>>>> ---
>>>>>>     drivers/staging/emxx_udc/emxx_udc.c | 11 ++++-------
>>>>>>     1 file changed, 4 insertions(+), 7 deletions(-)
>>>>> Why is this a [RESEND] ?
>>>>>
>>>>> What happened to the first version?
>>>> Sorry, I didn't receive your review, and in kernelnewbies tutorial, they say
>>>> if not receive a response, may have missed the patch, so I resent it.
>>> Do you have a pointer to your previous patch in the lore.kernel.org
>>> archives anywhere?  I can't seem to find it.
>>
>> I found this, it's what is you need?
>>
>> https://lore.kernel.org/linux-staging/20210401195457.24512-1-martinsdecarvalhobeatriz@gmail.com/
> Ah, yes, I saw Julia's review and assumed you would fix up your patch
> based on her comments.  Please do not ignore review comments, it makes
> everyone's lives harder.
Sorry, I didn't fix up my patch based on her comments, because to do 
this was
to revert all my patch or will break a code line if I remaining within 80
characters. How the code line still stays between 85 or 90 I assumed 
that was
ok, so I was waiting for your review. Now I saw that I should send this 
answer
to her. What should I do with the patch?

Thank you for all reviews, I'm learning a lot about how to do a good
contribution and the importance of answer all reviews.
> thanks,
>
> greg k-h
Beatriz Carvalho

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

[Greg KH]

On Wed, Apr 07, 2021 at 10:46:23AM +0100, Beatriz Martins de Carvalho wrote:
> 
> Em 07/04/21 09:34, Greg KH escreveu:
> > On Wed, Apr 07, 2021 at 09:16:44AM +0100, Beatriz Martins de Carvalho wrote:
> > > Em 07/04/21 06:37, Greg KH escreveu:
> > > > On Tue, Apr 06, 2021 at 09:00:07PM +0100, Beatriz Martins de Carvalho wrote:
> > > > > Em 06/04/21 20:36, Greg KH 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
> > > > > > > 
> > > > > > > Signed-off-by: Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@gmail.com>
> > > > > > > ---
> > > > > > >     drivers/staging/emxx_udc/emxx_udc.c | 11 ++++-------
> > > > > > >     1 file changed, 4 insertions(+), 7 deletions(-)
> > > > > > Why is this a [RESEND] ?
> > > > > > 
> > > > > > What happened to the first version?
> > > > > Sorry, I didn't receive your review, and in kernelnewbies tutorial, they say
> > > > > if not receive a response, may have missed the patch, so I resent it.
> > > > Do you have a pointer to your previous patch in the lore.kernel.org
> > > > archives anywhere?  I can't seem to find it.
> > > 
> > > I found this, it's what is you need?
> > > 
> > > https://lore.kernel.org/linux-staging/20210401195457.24512-1-martinsdecarvalhobeatriz@gmail.com/
> > Ah, yes, I saw Julia's review and assumed you would fix up your patch
> > based on her comments.  Please do not ignore review comments, it makes
> > everyone's lives harder.
> Sorry, I didn't fix up my patch based on her comments, because to do this
> was
> to revert all my patch or will break a code line if I remaining within 80
> characters. How the code line still stays between 85 or 90 I assumed that
> was
> ok, so I was waiting for your review. Now I saw that I should send this
> answer
> to her. What should I do with the patch?

If you do not know, ask the reviewer what they mean by their review
comments.

thanks,

greg k-h

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

[Matthew Wilcox]

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.

> -				_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.

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