diff mbox series

[v5,4/9] net: stmmac: dwmac-loongson: Refactor code for loongson_dwmac_probe()

Message ID 5f659ea8ab3a90ab27b99dfa24b05c20f3698545.1699533745.git.siyanteng@loongson.cn (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series stmmac: Add Loongson platform support | expand

Checks

Context Check Description
netdev/series_format warning Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1134 this patch: 1134
netdev/cc_maintainers warning 6 maintainers not CCed: linux-arm-kernel@lists.infradead.org edumazet@google.com pabeni@redhat.com kuba@kernel.org linux-stm32@st-md-mailman.stormreply.com mcoquelin.stm32@gmail.com
netdev/build_clang success Errors and warnings before: 1161 this patch: 1161
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1161 this patch: 1161
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 157 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Yanteng Si Nov. 10, 2023, 9:25 a.m. UTC 
Add a setup() function to initialize data, and simplify code for
loongson_dwmac_probe().

Signed-off-by: Yanteng Si <siyanteng@loongson.cn>
Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
---
 .../ethernet/stmicro/stmmac/dwmac-loongson.c  | 87 +++++++++++--------
 1 file changed, 53 insertions(+), 34 deletions(-)

Comments

Andrew Lunn Nov. 11, 2023, 8:19 p.m. UTC | #1 
On Fri, Nov 10, 2023 at 05:25:43PM +0800, Yanteng Si wrote:
> Add a setup() function to initialize data, and simplify code for
> loongson_dwmac_probe().

This does not look like a refactoring patch. Such patches just move
code around, but otherwise leave the code alone. There are real
changes in here.

> -	if (!of_device_is_compatible(np, "loongson, pci-gmac")) {
> -		pr_info("dwmac_loongson_pci: Incompatible OF node\n");
> -		return -ENODEV;
> -	}
> -

This just disappears. Why is it no longer needed?


>  	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
>  	if (!plat)
>  		return -ENOMEM;
>  
> +	plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> +					   sizeof(*plat->mdio_bus_data),
> +					   GFP_KERNEL);
> +	if (!plat->mdio_bus_data)
> +		return -ENOMEM;
> +
> +	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg),
> +				     GFP_KERNEL);
> +	if (!plat->dma_cfg)
> +		return -ENOMEM;
> +
>  	plat->mdio_node = of_get_child_by_name(np, "mdio");
>  	if (plat->mdio_node) {
>  		dev_info(&pdev->dev, "Found MDIO subnode\n");
> -
> -		plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> -						   sizeof(*plat->mdio_bus_data),
> -						   GFP_KERNEL);
> -		if (!plat->mdio_bus_data) {
> -			ret = -ENOMEM;
> -			goto err_put_node;
> -		}

MDIO was conditional, but now is mandatory. Why? 

>  	if (ret) {
> -		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n", __func__);
> +		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
> +			__func__);

Changes like this are a distraction. The reviewer is trying to
understand what has changed and why. If you want to make white space
changes, please do it in a patch of its own.

Please break this patch up into lots of smaller parts, each with a
good commit messaged explaining what is going on, and importantly,
why.

	Andrew
Yanteng Si Dec. 10, 2023, 1:46 a.m. UTC | #2 
在 2023/11/12 04:19, Andrew Lunn 写道:
> On Fri, Nov 10, 2023 at 05:25:43PM +0800, Yanteng Si wrote:
>> Add a setup() function to initialize data, and simplify code for
>> loongson_dwmac_probe().
> This does not look like a refactoring patch. Such patches just move
> code around, but otherwise leave the code alone. There are real
> changes in here.
>
>> -	if (!of_device_is_compatible(np, "loongson, pci-gmac")) {
>> -		pr_info("dwmac_loongson_pci: Incompatible OF node\n");
>> -		return -ENODEV;
>> -	}
>> -
> This just disappears. Why is it no longer needed?
>
>
>>   	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
>>   	if (!plat)
>>   		return -ENOMEM;
>>   
>> +	plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
>> +					   sizeof(*plat->mdio_bus_data),
>> +					   GFP_KERNEL);
>> +	if (!plat->mdio_bus_data)
>> +		return -ENOMEM;
>> +
>> +	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg),
>> +				     GFP_KERNEL);
>> +	if (!plat->dma_cfg)
>> +		return -ENOMEM;
>> +
>>   	plat->mdio_node = of_get_child_by_name(np, "mdio");
>>   	if (plat->mdio_node) {
>>   		dev_info(&pdev->dev, "Found MDIO subnode\n");
>> -
>> -		plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
>> -						   sizeof(*plat->mdio_bus_data),
>> -						   GFP_KERNEL);
>> -		if (!plat->mdio_bus_data) {
>> -			ret = -ENOMEM;
>> -			goto err_put_node;
>> -		}
> MDIO was conditional, but now is mandatory. Why?
>
>>   	if (ret) {
>> -		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n", __func__);
>> +		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
>> +			__func__);
> Changes like this are a distraction. The reviewer is trying to
> understand what has changed and why. If you want to make white space
> changes, please do it in a patch of its own.
>
> Please break this patch up into lots of smaller parts, each with a
> good commit messaged explaining what is going on, and importantly,
> why.


OK,Iwillgiveitatry.


Thanks,

Yanteng

>
> 	Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index 2cd6fce5c993..56d1fd8c61e1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -9,7 +9,12 @@ 
 #include <linux/of_irq.h>
 #include "stmmac.h"
 
-static int loongson_default_data(struct plat_stmmacenet_data *plat)
+struct stmmac_pci_info {
+	int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat);
+};
+
+static void loongson_default_data(struct pci_dev *pdev,
+				  struct plat_stmmacenet_data *plat)
 {
 	plat->clk_csr = 2;	/* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
 	plat->has_gmac = 1;
@@ -34,23 +39,38 @@  static int loongson_default_data(struct plat_stmmacenet_data *plat)
 
 	/* Disable RX queues routing by default */
 	plat->rx_queues_cfg[0].pkt_route = 0x0;
+}
+
+static int loongson_gmac_data(struct pci_dev *pdev,
+			      struct plat_stmmacenet_data *plat)
+{
+	loongson_default_data(pdev, plat);
+
+	plat->multicast_filter_bins = 256;
+
+	plat->mdio_bus_data->phy_mask = 0;
 
-	/* Default to phy auto-detection */
 	plat->phy_addr = -1;
+	plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID;
 
 	plat->dma_cfg->pbl = 32;
 	plat->dma_cfg->pblx8 = true;
 
-	plat->multicast_filter_bins = 256;
 	return 0;
 }
 
-static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+static struct stmmac_pci_info loongson_gmac_pci_info = {
+	.setup = loongson_gmac_data,
+};
+
+static int loongson_dwmac_probe(struct pci_dev *pdev,
+				const struct pci_device_id *id)
 {
+	int ret, i, bus_id, phy_mode;
 	struct plat_stmmacenet_data *plat;
+	struct stmmac_pci_info *info;
 	struct stmmac_resources res;
 	struct device_node *np;
-	int ret, i, phy_mode;
 
 	np = dev_of_node(&pdev->dev);
 
@@ -59,39 +79,32 @@  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 		return -ENODEV;
 	}
 
-	if (!of_device_is_compatible(np, "loongson, pci-gmac")) {
-		pr_info("dwmac_loongson_pci: Incompatible OF node\n");
-		return -ENODEV;
-	}
-
 	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
 	if (!plat)
 		return -ENOMEM;
 
+	plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
+					   sizeof(*plat->mdio_bus_data),
+					   GFP_KERNEL);
+	if (!plat->mdio_bus_data)
+		return -ENOMEM;
+
+	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg),
+				     GFP_KERNEL);
+	if (!plat->dma_cfg)
+		return -ENOMEM;
+
 	plat->mdio_node = of_get_child_by_name(np, "mdio");
 	if (plat->mdio_node) {
 		dev_info(&pdev->dev, "Found MDIO subnode\n");
-
-		plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
-						   sizeof(*plat->mdio_bus_data),
-						   GFP_KERNEL);
-		if (!plat->mdio_bus_data) {
-			ret = -ENOMEM;
-			goto err_put_node;
-		}
 		plat->mdio_bus_data->needs_reset = true;
 	}
 
-	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
-	if (!plat->dma_cfg) {
-		ret = -ENOMEM;
-		goto err_put_node;
-	}
-
 	/* Enable pci device */
 	ret = pci_enable_device(pdev);
 	if (ret) {
-		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n", __func__);
+		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
+			__func__);
 		goto err_put_node;
 	}
 
@@ -105,9 +118,16 @@  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 		break;
 	}
 
-	plat->bus_id = of_alias_get_id(np, "ethernet");
-	if (plat->bus_id < 0)
-		plat->bus_id = pci_dev_id(pdev);
+	pci_set_master(pdev);
+
+	info = (struct stmmac_pci_info *)id->driver_data;
+	ret = info->setup(pdev, plat);
+	if (ret)
+		goto err_disable_device;
+
+	bus_id = of_alias_get_id(np, "ethernet");
+	if (bus_id >= 0)
+		plat->bus_id = bus_id;
 
 	phy_mode = device_get_phy_mode(&pdev->dev);
 	if (phy_mode < 0) {
@@ -117,11 +137,7 @@  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 	}
 
 	plat->phy_interface = phy_mode;
-	plat->mac_interface = PHY_INTERFACE_MODE_GMII;
 
-	pci_set_master(pdev);
-
-	loongson_default_data(plat);
 	pci_enable_msi(pdev);
 	memset(&res, 0, sizeof(res));
 	res.addr = pcim_iomap_table(pdev)[0];
@@ -135,7 +151,8 @@  static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
 
 	res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
 	if (res.wol_irq < 0) {
-		dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
+		dev_info(&pdev->dev,
+			 "IRQ eth_wake_irq not found, using macirq\n");
 		res.wol_irq = res.irq;
 	}
 
@@ -219,8 +236,10 @@  static int __maybe_unused loongson_dwmac_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend,
 			 loongson_dwmac_resume);
 
+#define PCI_DEVICE_ID_LOONGSON_GMAC	0x7a03
+
 static const struct pci_device_id loongson_dwmac_id_table[] = {
-	{ PCI_VDEVICE(LOONGSON, 0x7a03) },
+	{ PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) },
 	{}
 };
 MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);