Re: [PATCH net-next v8 02/11] net: stmmac: dwmac-loongson: Refactor code for loongson_dwmac_probe()
From: Serge Semin <hidden>
Date: 2024-02-05 14:43:47
On Tue, Jan 30, 2024 at 04:43:22PM +0800, Yanteng Si wrote:
The driver function is not changed, but the code location is adjusted to prepare for adding more loongson drivers.
Having the word "refactoring" in the subject is always suspicious because submitters very often try to hind behind it many small changes they didn't want to/didn't know how to unpin from a more bulky change. Moreover if there is no detailed explanation what is done and why, it raises too many review questions and makes the reviewers life much harder. So it would have been much better for us if you split up this change into the smaller patches (see my last comment for a presumable subset of the patches) to simplify the review process and improve the driver bisectability especially seeing there actually are functional changes introduced here despite of what is said in the commit log.
quoted hunk ↗ jump to hunk
Signed-off-by: Yanteng Si <redacted> Signed-off-by: Feiyang Chen <redacted> Signed-off-by: Yinggang Gu <redacted> --- .../ethernet/stmicro/stmmac/dwmac-loongson.c | 61 +++++++++++++------ 1 file changed, 42 insertions(+), 19 deletions(-)diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c index 9e40c28d453a..e2dcb339b8b0 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,37 @@ 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;
Why do you need to move this here from the function tail?
+
+ plat->mdio_bus_data->phy_mask = 0;
This is already zero. Why do you need this?
- /* Default to phy auto-detection */
What is wrong with this comment?
plat->phy_addr = -1;
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;
Reverse xmas tree order please.
quoted hunk ↗ jump to hunk
np = dev_of_node(&pdev->dev);@@ -69,18 +88,17 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id 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; +
Why do you need this moved above the mdio_node getting procedure? They seem independent.
quoted hunk ↗ jump to hunk
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->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) {@@ -98,9 +116,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);
This is a functional change because further bus_id is no longer initialized by the pci_dev_id() method as a fallback case. If you are sure this is required please unpin to a separate patch and explain.
quoted hunk ↗ jump to hunk
+ 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) {@@ -110,11 +135,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;
This is just dropped. Are you sure that the driver will work correctly after this change is applied? Russell already asked you about this change here: https://lore.kernel.org/netdev/ZZPnaziDZEcv5GGw@shell.armlinux.org.uk/ (local) Anyway please unpin it to a separate patch and explain.
quoted hunk ↗ jump to hunk
- pci_set_master(pdev); - - loongson_default_data(plat); pci_enable_msi(pdev); memset(&res, 0, sizeof(res)); res.addr = pcim_iomap_table(pdev)[0];@@ -212,8 +233,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) },
If I were you and needed to preserve all the changes I would have split the patch up into the next patches: 1. Use PCI_DEVICE_DATA() macro for device identification 2. Drop mac-interface initialization 3. Don't initialize MDIO bus ID with PCIe device ID 4. Introduce device-specific setup callback -Serge(y)
{}
};
MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);
--
2.31.4