Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Raviteja Garimella]

Hi Florian,

On Fri, Jan 20, 2017 at 12:58 AM, Florian Fainelli  wrote:
> On 01/19/2017 02:44 AM, Raviteja Garimella wrote:
>> Hi,
>>
>> On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli  
>> 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 
>>>
>>> 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

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Raviteja Garimella]

Hi Florian,

On Fri, Jan 20, 2017 at 12:58 AM, Florian Fainelli  wrote:
> On 01/19/2017 02:44 AM, Raviteja Garimella wrote:
>> Hi,
>>
>> On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli  
>> 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 
>>>
>>> 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

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Florian Fainelli]

On 01/19/2017 02:44 AM, Raviteja Garimella wrote:
> Hi,
> 
> On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli  
> 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 
>>
>> 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

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

Thanks
-- 
Florian

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Florian Fainelli]

On 01/19/2017 02:44 AM, Raviteja Garimella wrote:
> Hi,
> 
> On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli  
> 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 
>>
>> 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

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

Thanks
-- 
Florian

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Raviteja Garimella]

Hi,

On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli  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 
>
> 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.

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.

Rest all is same as in old amd5536udc.c common file. It's just a copy from the
old file.

And, the file amd5536udc.c will now only do the pci device probe and
remove functions.

I hope this helps. Please let me know of any clarifications needed.
Since both the files are required to be reviewed, I think renaming is
inevitable.

Thanks,
Ravi

>
> Also, keep in mind that anytime a driver file is renamed, this poses a
> backport/maintenance issue where backporting fixes from latest upstream
> to a kernel version that has a different file/directory structure is a
> major pain.
> --
> Florian

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Raviteja Garimella]

Hi,

On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli  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 
>
> 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.

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.

Rest all is same as in old amd5536udc.c common file. It's just a copy from the
old file.

And, the file amd5536udc.c will now only do the pci device probe and
remove functions.

I hope this helps. Please let me know of any clarifications needed.
Since both the files are required to be reviewed, I think renaming is
inevitable.

Thanks,
Ravi

>
> Also, keep in mind that anytime a driver file is renamed, this poses a
> backport/maintenance issue where backporting fixes from latest upstream
> to a kernel version that has a different file/directory structure is a
> major pain.
> --
> Florian

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Greg Kroah-Hartman]

On Wed, Jan 18, 2017 at 10:45:39AM -0800, Florian Fainelli 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 
> 
> 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?
> 
> Also, keep in mind that anytime a driver file is renamed, this poses a
> backport/maintenance issue where backporting fixes from latest upstream
> to a kernel version that has a different file/directory structure is a
> major pain.

Don't ever let stable tree work prevent you from doing the right thing.
If this needs to be split, wonderful, stable kernel work will just have
to deal with it.  Hint, we handle it all the time just fine :)

thanks,

greg k-h

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Greg Kroah-Hartman]

On Wed, Jan 18, 2017 at 10:45:39AM -0800, Florian Fainelli 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 
> 
> 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?
> 
> Also, keep in mind that anytime a driver file is renamed, this poses a
> backport/maintenance issue where backporting fixes from latest upstream
> to a kernel version that has a different file/directory structure is a
> major pain.

Don't ever let stable tree work prevent you from doing the right thing.
If this needs to be split, wonderful, stable kernel work will just have
to deal with it.  Hint, we handle it all the time just fine :)

thanks,

greg k-h

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Florian Fainelli]

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 

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?

Also, keep in mind that anytime a driver file is renamed, this poses a
backport/maintenance issue where backporting fixes from latest upstream
to a kernel version that has a different file/directory structure is a
major pain.
-- 
Florian

Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

[Florian Fainelli]

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 

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?

Also, keep in mind that anytime a driver file is renamed, this poses a
backport/maintenance issue where backporting fixes from latest upstream
to a kernel version that has a different file/directory structure is a
major pain.
-- 
Florian