RE: [PATCH v8 07/15] PCI: cadence: Move PCIe controller common functions as a separate file

[Manikandan Karunakaran Pillai <mpillai@cadence.com>]

>functions as a separate file
>
>EXTERNAL MAIL
>
>
>On Tue, Aug 19, 2025 at 07:52:31PM GMT, hans.zhang@cixtech.com wrote:
>> From: Manikandan K Pillai <mpillai@cadence.com>
>>
>> Move the functions for platform common tasks to a separate file. The
>> common library functions and functions specific to platform are
>> now in different files.
>>
>
>Why do we have too many library files? What is the need to split
>pcie-cadence-common.c which itself is a library?
>
Hi Mani,
I plan to drop this patch and that address all your comments.

- Mani

>> Signed-off-by: Manikandan K Pillai <mpillai@cadence.com>
>> Co-developed-by: Hans Zhang <hans.zhang@cixtech.com>
>> Signed-off-by: Hans Zhang <hans.zhang@cixtech.com>
>> ---
>>  drivers/pci/controller/cadence/Makefile       |   2 +-
>>  .../controller/cadence/pcie-cadence-common.c  | 141 ++++++++++++++++++
>>  drivers/pci/controller/cadence/pcie-cadence.c | 129 ----------------
>>  3 files changed, 142 insertions(+), 130 deletions(-)
>>  create mode 100644 drivers/pci/controller/cadence/pcie-cadence-common.c
>>
>> diff --git a/drivers/pci/controller/cadence/Makefile
>b/drivers/pci/controller/cadence/Makefile
>> index e45f72388bbb..b104562fb86a 100644
>> --- a/drivers/pci/controller/cadence/Makefile
>> +++ b/drivers/pci/controller/cadence/Makefile
>> @@ -1,5 +1,5 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> -obj-$(CONFIG_PCIE_CADENCE) += pcie-cadence.o
>> +obj-$(CONFIG_PCIE_CADENCE) += pcie-cadence-common.o pcie-cadence.o
>>  obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host-common.o pcie-
>cadence-host.o
>>  obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep-common.o pcie-
>cadence-ep.o
>>  obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-common.c
>b/drivers/pci/controller/cadence/pcie-cadence-common.c
>> new file mode 100644
>> index 000000000000..e14d53d64bf1
>> --- /dev/null
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-common.c
>> @@ -0,0 +1,141 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2017 Cadence
>> +// Cadence PCIe controller driver.
>> +// Author: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
>
>We use below style for multi line comments:
>
>	/*
>	 *
>	 */
>
>So even though this style existed, you should change it in all new files.
>
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/of.h>
>> +
>> +#include "pcie-cadence.h"
>> +
>> +void cdns_pcie_disable_phy(struct cdns_pcie *pcie)
>> +{
>> +	int i = pcie->phy_count;
>> +
>> +	while (i--) {
>> +		phy_power_off(pcie->phy[i]);
>> +		phy_exit(pcie->phy[i]);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(cdns_pcie_disable_phy);
>> +
>> +int cdns_pcie_enable_phy(struct cdns_pcie *pcie)
>> +{
>> +	int ret;
>> +	int i;
>> +
>> +	for (i = 0; i < pcie->phy_count; i++) {
>> +		ret = phy_init(pcie->phy[i]);
>> +		if (ret < 0)
>> +			goto err_phy;
>> +
>> +		ret = phy_power_on(pcie->phy[i]);
>> +		if (ret < 0) {
>> +			phy_exit(pcie->phy[i]);
>> +			goto err_phy;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +err_phy:
>> +	while (--i >= 0) {
>> +		phy_power_off(pcie->phy[i]);
>> +		phy_exit(pcie->phy[i]);
>> +	}
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(cdns_pcie_enable_phy);
>> +
>> +int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	int phy_count;
>> +	struct phy **phy;
>> +	struct device_link **link;
>> +	int i;
>> +	int ret;
>> +	const char *name;
>> +
>> +	phy_count = of_property_count_strings(np, "phy-names");
>> +	if (phy_count < 1) {
>> +		dev_info(dev, "no \"phy-names\" property found; PHY will not
>be initialized\n");
>> +		pcie->phy_count = 0;
>> +		return 0;
>> +	}
>> +
>> +	phy = devm_kcalloc(dev, phy_count, sizeof(*phy), GFP_KERNEL);
>> +	if (!phy)
>> +		return -ENOMEM;
>> +
>> +	link = devm_kcalloc(dev, phy_count, sizeof(*link), GFP_KERNEL);
>> +	if (!link)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < phy_count; i++) {
>> +		of_property_read_string_index(np, "phy-names", i, &name);
>> +		phy[i] = devm_phy_get(dev, name);
>> +		if (IS_ERR(phy[i])) {
>> +			ret = PTR_ERR(phy[i]);
>> +			goto err_phy;
>> +		}
>> +		link[i] = device_link_add(dev, &phy[i]->dev,
>DL_FLAG_STATELESS);
>> +		if (!link[i]) {
>> +			devm_phy_put(dev, phy[i]);
>> +			ret = -EINVAL;
>> +			goto err_phy;
>> +		}
>> +	}
>> +
>> +	pcie->phy_count = phy_count;
>> +	pcie->phy = phy;
>> +	pcie->link = link;
>> +
>> +	ret =  cdns_pcie_enable_phy(pcie);
>
>Double space after =
>
>- Mani
>
>--
>மணிவண்ணன் சதாசிவம்