Hi Florian, On Fri, Jan 20, 2017 at 12:58 AM, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: > On 01/19/2017 02:44 AM, Raviteja Garimella wrote: >> Hi, >> >> On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: >>> On 01/17/2017 12:05 AM, Raviteja Garimella wrote: >>>> This patch splits the amd5536udc driver into two -- one that does >>>> pci device registration and the other file that does the rest of >>>> the driver tasks like the gadget/ep ops etc for Synopsys UDC. >>>> >>>> This way of splitting helps in exporting core driver symbols which >>>> can be used by any other platform/pci driver that is written for >>>> the same Synopsys USB device controller. >>>> >>>> The current patch also includes a change in the Kconfig and Makefile. >>>> A new config option USB_SNP_CORE will be selected automatically when >>>> any one of the platform or pci driver for the same UDC is selected. >>>> >>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@xxxxxxxxxxxx> >>> >>> Although the changes you have done make sense and it is most certainly a >>> good idea to split udc core from bus specific glue logic, it is really >>> hard to review the changes per-se because of the file rename, could that >>> happen at a later time? >> >> If you start looking at this specific patch from the header file (amd5536udc.h), >> the additions in there comprise of: >> - 9 function declarations >> - some module parameter variable declarations that's moved out from the older >> common file amd5536udc.c >> - 2 #includes that are needed by all files. > > Well, I don't really question the changes themselves, rather how this is > presented as a patch series to reviewers. > > What I would do, to help introduce both the rename, and the splitting of > core vs. bus-glue specific changes is: > > - have an initial patch which extracts the core functionality of the > driver and the PCI bus glue logic into adm5536udc_pci.c and left > adm5536udc.c intact (that would be a small delta to review) > > - have a second patch that performs the file rename from adm5536udc.c > into snps_udc_core.c and updates adm5536udc_pci.c eventually as a result > of that, then again, a reviewer can ignore the rename part (don't format > to generate your patches with git format-patch -M in that case) and just > focus on the conversion part for adm5536udc_pci.c > Just waited for any more comments coming in. I will submit the next version as PATCH like the way you suggested. >> >> So, basically what's done for this split is that: >> 1. the static keyword is removed from those 9 functions in the new file >> snps_udc_core.c and are exported. >> 2. The module parameters declarations (since they are used in both core >> and pci file) are moved to the header file now. > > These should really be part of the commit messages for each commit doing > the changes, this is meant to help a reviewer understand what you are > doing, and to some degree, will help him/her make an educated decision > as to what part of the code the focus should be put on. > Will do. Thanks, Ravi > Thanks > -- > Florian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
