Re: [PATCH v3 7/7] PCI: microchip: Re-partition code between probe() and init()

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jul 28, 2023 at 02:14:01PM +0100, daire.mcnamara@microchip.com wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
> 
> Continuing to use pci_host_common_probe() for the PCIe Root Complex on
> PolarFire SoC was leading to an extremely large _init() function and
> some unnatural code flow. Re-partition so some tasks are done in
> a _probe() routine, which calls pci_host_common_probe() and then use a
> much smaller _init() function, mainly to enable interrupts after address
> translation tables are set up.
> ...

> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -384,6 +384,8 @@ static struct {
>  
>  static char poss_clks[][5] = { "fic0", "fic1", "fic2", "fic3" };
>  
> +static struct mc_pcie *port;
> ...

> +static int mc_host_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
>  	void __iomem *bridge_base_addr;
>  	int ret;
>  	u32 val;
> @@ -1112,13 +1141,8 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);

Saving this per-device pointer in a static singleton is kind of
problematic.  We may know there can only be a single instance right
now, but even if that remains true forever for this driver, we don't
want this pattern to be be copied elsewhere.

I didn't look hard enough to figure out exactly what problem this
singleton solves, but is there any other way to address it?

I suspect it's related to using pci_host_common_probe(), and it's
great to share that code, but it seems like that basically forces some
non-ECAM initialization into the struct pci_ecam_ops.init() method
where it doesn't really fit very well.

Bjorn