On Fri, Aug 21, 2020 at 11:57:47AM -0600, Rob Herring wrote: > On Thu, Aug 20, 2020 at 12:10 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > On Wed, Aug 19, 2020 at 04:33:10PM +0000, Daire.McNamara@xxxxxxxxxxxxx wrote: > > > > > +static struct mc_port *port; > > > > This file scope item is not ideal. It might work in your situation if > > you can never have more than one device, but it's not a pattern we > > want other people to copy. > > Indeed. > > > I think I sort of see how it works: > > > > mc_pci_host_probe > > pci_host_common_probe > > ops = of_device_get_match_data() # mc_ecam_ops > > gen_pci_init(..., ops) > > pci_ecam_create(..., ops) > > ops->init # mc_ecam_ops.init > > mc_platform_init(pci_config_window *) > > port = devm_kzalloc(...) # initialized > > mc_setup_windows > > bridge_base_addr = port->axi_base_addr + ... # used > > > > And you're using the file scope "port" because mc_platform_init() > > doesn't have a pointer to the platform_device. > > This is a simple fix. Move platform_set_drvdata to just after > devm_pci_alloc_host_bridge() in pci_host_common_probe(). (Don't fall > into the 'platform problem'[1] and work-around the core code.) > > Then pci_host_common_probe can be called directly and mc_setup_windows > can be moved to mc_platform_init(). > > > But I think this > > abuses the pci_ecam_ops design to do host bridge initialization that > > it is not intended for. > > What init should be done then? IMO, given this driver is using ECAM > support already and it's all one time init that fits into init(), it > seems like a fit to me. Oh, OK. If you can solve this as you outlined above, that's fine with me. It didn't look like a common pattern yet, but maybe it will be. Thanks for chiming in. I didn't have a good idea for how to fix the file-scope variable problem. Bjorn
