Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller support

[Paul Gortmaker]

[Re: [PATCH v7 2/2] PCI: Rockchip: Add Rockchip PCIe controller support] On 27/07/2016 (Wed 13:04) Brian Norris wrote:

> + Guenter
> 
> Hi,
> 
> On Wed, Jul 27, 2016 at 03:56:36PM -0400, Paul Gortmaker wrote:
> > On Wed, Jul 27, 2016 at 2:22 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > On Mon, Jul 18, 2016 at 08:42:13AM +0800, Shawn Lin wrote:
> > >> This patch adds Rockchip PCIe controller support found
> > >> on RK3399 Soc platform.
> > >>
> > >> Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> > >>
> > >> ---
> > >>
> > >> Changes in v7:
> > >> - make it as a build-in driver
> > 
> > [...]
> > 
> 
> ...
> 
> > [...]
> > 
> > >> +static struct platform_driver rockchip_pcie_driver = {
> > >> +     .driver = {
> > >> +             .name = "rockchip-pcie",
> > >> +             .of_match_table = rockchip_pcie_of_match,
> > >> +     },
> > >> +     .probe = rockchip_pcie_probe,
> > >> +
> > >> +};
> > >> +module_platform_driver(rockchip_pcie_driver);
> > >> +
> > >> +MODULE_AUTHOR("Rockchip Inc");
> > >> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
> > >> +MODULE_LICENSE("GPL v2");
> > >
> > > Per your Kconfig, this driver cannot be a module, so remove these MODULE_*
> > > annotations.  This is to follow Paul Gortmaker's recent "Make explicitly
> > > non-modular" work.  Also change the include of <linux/module.h> to
> > > <linux/init.h>.
> > 
> > Thanks!  You beat me to it.    I didn't see any EXPORT_SYMBOL so it doesn't
> > look like we need export.h  added here.   Also, at the risk of stating
> > the obvious,
> > the module_platform_driver becomes builtin_platform_driver.
> 
> Just for reference (not to necessarily disagree here): we've kinda
> ping-ponged on this one. I suggested it could be built as a module, so
> Shawn added it, but apparently we're missing an export:
> 
> https://lkml.org/lkml/2016/7/8/704
> 
> so he reverted back. It's probably best to just leave this built-in and
> resolve the comments Paul and Bjorn made, to avoid too much more
> flip-flopping. If we still want to patch up the __weak
> pci_remap_iospace(), then we can make this a module at a later time.

Agreed, and this was the general consensus we came to for other PCI code
as well.   It might be different if we were deleting hundreds of lines
of code, but we are just making a few simple changes to make it clear
that things aren't modular, and that ensures others don't continue to
copy the same inconsistency into their new driver(s).  It doesn't in any
way prevent a future conversion to tristate if that use case makes sense.

THanks,
Paul.
--

> 
> Regards,
> Brian