Re: [PATCH v2 11/17] xen/arm: PCI host bridge discovery within XEN on ARM

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 23 Sep 2021, Rahul Singh wrote:
> >> +            goto err_exit;
> >> +    }
> > 
> > This is unnecessary at the moment, right? Can we get rid of ops->init ?
> 
> No this is required for N1SDP board. Please check below patch.
> https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5

OK


> >> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> >> +{
> >> +    struct pci_host_bridge *bridge;
> >> +    struct pci_config_window *cfg;
> >> +    struct pci_ecam_ops *ops;
> >> +    const struct dt_device_match *of_id;
> >> +    int err;
> >> +
> >> +    if ( dt_device_for_passthrough(dev) )
> >> +        return 0;
> >> +
> >> +    of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
> >> +    ops = (struct pci_ecam_ops *) of_id->data;
> > 
> > Do we really need dt_match_node and dev->dev.of_match_table to get
> > dt_device_match.data?
> > 
> 
> > data is passed as a parameter to pci_host_common_probe, isn't it enough
> > to do:
> > 
> > ops = (struct pci_ecam_ops *) data;
> 
> As of now not required but in future we might need it if we implement other 
> ecam supported bridge
> 
> static const struct dt_device_match gen_pci_dt_match[] = {                    
>   
>     { .compatible = "pci-host-ecam-generic",                                  
>   
>       .data =       &pci_generic_ecam_ops },
> 
>     { .compatible = "pci-host-cam-generic",
>       .data = &gen_pci_cfg_cam_bus_ops },                                 
>                                                                               
>   
>     { },                                                                      
>   
> };

Even if we add another ECAM-supported bridge, the following:

ops = (struct pci_ecam_ops *) data;

could still work, right? The probe function will directly receive as
parameter the .data pointer. You shouldn't need the indirection via
dt_match_node?

If you are worried about gen_pci_cfg_cam_bus_ops not being a struct
pci_ecam_ops: that problem can also be solved by making
gen_pci_cfg_cam_bus_ops a struct containinig a struct pci_ecam_ops.