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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Artur Petrosyan <Arthur.Petrosyan@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux