Re: [PATCH v2 09/17] PCI: dwc: dra7xx: Add ifdefs for host/ep specific code

[Niklas Cassel]

On 10/31/2017 09:29 AM, Kishon Vijay Abraham I wrote:

(snip)

>>  
>> +#ifdef CONFIG_PCI_DRA7XX_HOST
> 
> This block can also be moved around the file so that there is a single +#ifdef
> CONFIG_PCI_DRA7XX_HOST in this file.

Hello Kishon,

Sure, will fix in V3.

>>  static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
>>  				       struct platform_device *pdev)
>>  {
>> @@ -470,6 +475,7 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
>>  
>>  	return 0;
>>  }
>> +#endif
>>  
>>  static const struct dw_pcie_ops dw_pcie_ops = {
>>  	.cpu_addr_fixup = dra7xx_pcie_cpu_addr_fixup,
>> @@ -517,23 +523,31 @@ static int dra7xx_pcie_enable_phy(struct dra7xx_pcie *dra7xx)
>>  	return ret;
>>  }
> 
> IMO all the #ifdef's after this point can be removed.

I will remove the ifdefs in the match table and the ifdefs surrounding
the of match table data.

>>  
>> +#ifdef CONFIG_PCI_DRA7XX_HOST
>>  static const struct dra7xx_pcie_of_data dra7xx_pcie_rc_of_data = {
>>  	.mode = DW_PCIE_RC_TYPE,
>>  };
>> +#endif
>>  
>> +#ifdef CONFIG_PCI_DRA7XX_EP
>>  static const struct dra7xx_pcie_of_data dra7xx_pcie_ep_of_data = {
>>  	.mode = DW_PCIE_EP_TYPE,
>>  };
>> +#endif
>>  
>>  static const struct of_device_id of_dra7xx_pcie_match[] = {
>> +#ifdef CONFIG_PCI_DRA7XX_HOST
>>  	{
>>  		.compatible = "ti,dra7-pcie",
>>  		.data = &dra7xx_pcie_rc_of_data,
>>  	},
>> +#endif
>> +#ifdef CONFIG_PCI_DRA7XX_EP
>>  	{
>>  		.compatible = "ti,dra7-pcie-ep",
>>  		.data = &dra7xx_pcie_ep_of_data,
>>  	},
>> +#endif
>>  	{},
>>  };
>>  
>> @@ -548,6 +562,7 @@ static const struct of_device_id of_dra7xx_pcie_match[] = {
>>   *
>>   * To avoid this issue set PCIE_SS1_AXI2OCP_LEGACY_MODE_ENABLE to 1.
>>   */
>> +#ifdef CONFIG_PCI_DRA7XX_EP
>>  static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev)
>>  {
>>  	int ret;
>> @@ -578,6 +593,7 @@ static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev)
>>  
>>  	return ret;
>>  }
>> +#endif

I will move this to the CONFIG_PCI_DRA7XX_EP block, so that there will only
be a single CONFIG_PCI_DRA7XX_EP ifdef surrounding EP specific functions.

>>  
>>  static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>>  {
>> @@ -681,6 +697,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>>  		dra7xx->link_gen = 2;
>>  
>>  	switch (mode) {
>> +#ifdef CONFIG_PCI_DRA7XX_HOST
>>  	case DW_PCIE_RC_TYPE:
>>  		dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE,
>>  				   DEVICE_TYPE_RC);
>> @@ -688,6 +705,8 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>>  		if (ret < 0)
>>  			goto err_gpio;
>>  		break;
>> +#endif
>> +#ifdef CONFIG_PCI_DRA7XX_EP
>>  	case DW_PCIE_EP_TYPE:
>>  		dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE,
>>  				   DEVICE_TYPE_EP);
>> @@ -700,6 +719,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>>  		if (ret < 0)
>>  			goto err_gpio;
>>  		break;
>> +#endif

Actually, these ifdefs has to stay, otherwise we get build warnings, since we
are calling functions that aren't defined (dra7xx_pcie_ep_unaligned_memaccess,
dra7xx_add_pcie_ep, dra7xx_add_pcie_port).
We could add dummy implementations for these inside an #else block following
the ifdef blocks. However, I think that adding dummy implementations in the
#else block is uglier and more verbose than keeping the ifdefs around the
two cases.


Regards,
Niklas