Re: [PATCH v4 2/5] PCI: iproc: Add PAXC interface support

[Ray Jui <rjui@broadcom.com>]

Hi Arnd,

On 11/27/2015 12:22 PM, Arnd Bergmann wrote:
> On Friday 27 November 2015 09:37:45 Ray Jui wrote:
>>
>> +static const struct of_device_id iproc_pcie_of_match_table[] = {
>> +       {
>> +               .compatible = "brcm,iproc-pcie",
>> +               .data = (int *)IPROC_PCIE_PAXB,
>> +       }, {
>> +               .compatible = "brcm,iproc-pcie-paxc",
>> +               .data = (int *)IPROC_PCIE_PAXC,
>> +       },
>> +       { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, iproc_pcie_of_match_table);
>
> You seem to only need the identifiers in order to set a single
> pointer, so just point to that array directly.

Not just a single pointer. This sets the iProc PCIe interface type, 
which is an enum. Based on the interface type, both the iProc PCIe core 
driver and the iProc PCIe MSI driver can 1) load the correct set of 
register offsets; 2) skip/invoke some of the link detection, controller 
reset related behaviors; 3) setting up the correct number of MSI event 
queue regions and MSI address regions.

> Alternatively,
> do the more common thing and point to a structure of function
> pointers and have different implementations of the low-level
> access functions there.

I thought about replacing the reset and link check functions in 
pcie-iproc.c with callbacks but felt it has not reached the point where 
we need callbacks to completely differentiate the operations. I'm hoping 
to wait until I see the next revision of PAXC (PAXC v2) implementation 
in the next-gen iProc SoC to make the call....

Note even with the callback approach, it would not change much of the 
pcie-iproc-platform.c here, which still needs to pass in just the 
interface information but no more than that. Main reason being the core 
driver and MSI drivers are used by both the platform driver and BCMA 
driver, and I do not want to duplicate the callback implementation at 
the bus driver level.

>
> 	Arnd
>

Thanks,

Ray