Re: [PATCH v2 01/15] usb: dwc2: Fix/update enter/exit partial power down.

Re: [PATCH v2 01/15] usb: dwc2: Fix/update enter/exit partial power down.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]


Hi Arthur,

before I review your series, there are few things I'd like to point out:

1. A single patch should do one thing and one thing only

2. Every single patch should compile and work on its own

3. When sending a series, remember to include a cover letter

4. When sending a series, you can rely on git to produce a thread with a
   cover letter

	git format-patch -o series --cover-letter HEAD~15..

5. Remember to run checkpatch on every patch

6. Please, read https://www.kernel.org/doc/html/latest/process/submit-checklist.html

Artur Petrosyan <Arthur.Petrosyan@synopsys.com> writes:
> - Updated entering and exiting partial power down function
>   flow. Before there was a lot of confusions with core
>   entering to partial power down in device or host mode.
>
> - Added "rem_wakeup" for host exiting from Partial Power
>   Down mode from host remote wakeup flow. According to
>   programming guide in host mode, port power must be
>   turned on when wakeup is detected.
>
> - Added "in_ppd" flag to indicate the core state after
>   entering into Partial Power Down mode.
>
> - Moved setting of lx_state into partial power down
>   specific functions.
>
> - Added dev_dbg() messages when entering and exiting from
>   partial power down.
>
> - During Partial Power Down exit rely on backuped value of
>   "GOTGCTL_CURMODE_HOST" to determine the mode of core
>   before entering to PPD.
>
> - Set missing "DCTL_PWRONPRGDONE" bit when exiting device
>   partial power down according to programming guide.
>
> - Added missing restore of DCFG register in device mode
>   according to programming guide.

From a quick read, it seems like each of these topics deserve a patch of
its own. Could you break this down into smaller patches? Also, you have
colleagues who have been dealing with the community for a long time,
perhaps ask them to do an internal round of review before submitting?

That may help you get your patches merged in a timely manner.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v2 01/15] usb: dwc2: Fix/update enter/exit partial power down.

[Artur Petrosyan]

Hi Felipe,

On 10/27/2020 12:21, Felipe Balbi wrote:
> 
> Hi Arthur,
> 
> before I review your series, there are few things I'd like to point out:
> 
> 1. A single patch should do one thing and one thing only
> 
> 2. Every single patch should compile and work on its own
> 
> 3. When sending a series, remember to include a cover letter
> 
> 4. When sending a series, you can rely on git to produce a thread with a
>     cover letter
> 
> 	git format-patch -o series --cover-letter HEAD~15..
> 
> 5. Remember to run checkpatch on every patch
> 
> 6. Please, read https://www.kernel.org/doc/html/latest/process/submit-checklist.html
The above statements are of course done before submitting to LKML.
Moreover each patch is first of all tested using Jenkins, and passed a 
review process on gerrit.
Did you see any build error? or checkpatch error?

> 
> Artur Petrosyan <Arthur.Petrosyan@synopsys.com> writes:
>> - Updated entering and exiting partial power down function
>>    flow. Before there was a lot of confusions with core
>>    entering to partial power down in device or host mode.
>>
>> - Added "rem_wakeup" for host exiting from Partial Power
>>    Down mode from host remote wakeup flow. According to
>>    programming guide in host mode, port power must be
>>    turned on when wakeup is detected.
>>
>> - Added "in_ppd" flag to indicate the core state after
>>    entering into Partial Power Down mode.
>>
>> - Moved setting of lx_state into partial power down
>>    specific functions.
>>
>> - Added dev_dbg() messages when entering and exiting from
>>    partial power down.
>>
>> - During Partial Power Down exit rely on backuped value of
>>    "GOTGCTL_CURMODE_HOST" to determine the mode of core
>>    before entering to PPD.
>>
>> - Set missing "DCTL_PWRONPRGDONE" bit when exiting device
>>    partial power down according to programming guide.
>>
>> - Added missing restore of DCFG register in device mode
>>    according to programming guide.
> 
>  From a quick read, it seems like each of these topics deserve a patch of
> its own. Could you break this down into smaller patches? Also, you have
> colleagues who have been dealing with the community for a long time,
> perhaps ask them to do an internal round of review before submitting?
> 
> That may help you get your patches merged in a timely manner.
> 
I will work on breaking this patch down into smaller patches I could do 
this before of course the reason I didn't break them down was that I 
didn't want to make the patch series so big.
But as it is necessary I will do it.

Thank you very much for the advice. I will also invite the colleagues to 
test or give a review.

Regards,
Artur

Re: [PATCH v2 01/15] usb: dwc2: Fix/update enter/exit partial power down.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3395 bytes --]


Hi,

Artur Petrosyan <Arthur.Petrosyan@synopsys.com> writes:
> Hi Felipe,
>
> On 10/27/2020 12:21, Felipe Balbi wrote:
>> 
>> Hi Arthur,
>> 
>> before I review your series, there are few things I'd like to point out:
>> 
>> 1. A single patch should do one thing and one thing only
>> 
>> 2. Every single patch should compile and work on its own
>> 
>> 3. When sending a series, remember to include a cover letter
>> 
>> 4. When sending a series, you can rely on git to produce a thread with a
>>     cover letter
>> 
>> 	git format-patch -o series --cover-letter HEAD~15..
>> 
>> 5. Remember to run checkpatch on every patch
>> 
>> 6. Please, read https://www.kernel.org/doc/html/latest/process/submit-checklist.html
> The above statements are of course done before submitting to LKML.
> Moreover each patch is first of all tested using Jenkins, and passed a 
> review process on gerrit.

The fact that you're doing multiple things in a single commit should
have been caught during your internal review process. John, Minas, did
any of you review these patches before submission? Please make sure
details such as this are caught before hand ;-)

> Did you see any build error? or checkpatch error?

just a general comment, seen that patches were not send as a reply to
patch 0, in a separate thread.

>> Artur Petrosyan <Arthur.Petrosyan@synopsys.com> writes:
>>> - Updated entering and exiting partial power down function
>>>    flow. Before there was a lot of confusions with core
>>>    entering to partial power down in device or host mode.
>>>
>>> - Added "rem_wakeup" for host exiting from Partial Power
>>>    Down mode from host remote wakeup flow. According to
>>>    programming guide in host mode, port power must be
>>>    turned on when wakeup is detected.
>>>
>>> - Added "in_ppd" flag to indicate the core state after
>>>    entering into Partial Power Down mode.
>>>
>>> - Moved setting of lx_state into partial power down
>>>    specific functions.
>>>
>>> - Added dev_dbg() messages when entering and exiting from
>>>    partial power down.
>>>
>>> - During Partial Power Down exit rely on backuped value of
>>>    "GOTGCTL_CURMODE_HOST" to determine the mode of core
>>>    before entering to PPD.
>>>
>>> - Set missing "DCTL_PWRONPRGDONE" bit when exiting device
>>>    partial power down according to programming guide.
>>>
>>> - Added missing restore of DCFG register in device mode
>>>    according to programming guide.
>> 
>>  From a quick read, it seems like each of these topics deserve a patch of
>> its own. Could you break this down into smaller patches? Also, you have
>> colleagues who have been dealing with the community for a long time,
>> perhaps ask them to do an internal round of review before submitting?
>> 
>> That may help you get your patches merged in a timely manner.
>> 
> I will work on breaking this patch down into smaller patches I could do 
> this before of course the reason I didn't break them down was that I 
> didn't want to make the patch series so big.

too big series are not a problem. Too big patches doing multiple things
generally are. Keep in mind that you want maintainers to receive
patches that obviously correct.

> Thank you very much for the advice. I will also invite the colleagues to 
> test or give a review.

thanks

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]