Re: [PATCH v2 4/4] net: mtip: The L2 switch driver for imx287

Krzysztof Kozlowski
2025-03-28T14:19:12Z

On 28/03/2025 14:35, Lukasz Majewski wrote:
> +
> +static void mtip_mii_unregister(struct switch_enet_private *fep)
> +{
> + mdiobus_unregister(fep->mii_bus);
> + mdiobus_free(fep->mii_bus);
> +}
> +
> +static const struct fec_devinfo fec_imx28_l2switch_info = {
> + .quirks = FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_SINGLE_MDIO,
> +};
> +
> +static struct platform_device_id pdev_id = {

That's const.

> + .name = "imx28-l2switch",
> + .driver_data = (kernel_ulong_t)&fec_imx28_l2switch_info,
> +};
> +
> +static int __init mtip_sw_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct switch_enet_private *fep;
> + struct fec_devinfo *dev_info;
> + struct switch_t *fecp;
> + int ret;
> +
> + fep = devm_kzalloc(&pdev->dev, sizeof(*fep), GFP_KERNEL);
> + if (!fep)
> + return -ENOMEM;
> +
> + pdev->id_entry = &pdev_id;

Hm? This is some odd pattern. You are supposed to use OF table and get
matched by it, not populate some custom/odd handling of platform tables.

> +
> + dev_info = (struct fec_devinfo *)pdev->id_entry->driver_data;

I did not notice it before, but that's a no - you cannot drop the cast.
Driver data is always const.

> + if (dev_info)
> + fep->quirks = dev_info->quirks;
> +
> + fep->pdev = pdev;
> + platform_set_drvdata(pdev, fep);
> +
> + fep->enet_addr = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(fep->enet_addr))
> + return PTR_ERR(fep->enet_addr);
> +
> + fep->irq = platform_get_irq(pdev, 0);
> + if (fep->irq < 0)
> + return fep->irq;
> +
> + ret = mtip_parse_of(fep, np);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "%s: OF parse error (%d)!\n", __func__,
> + ret);
> + return ret;
> + }
> +
> + /* Create an Ethernet device instance.
> + * The switch lookup address memory starts at 0x800FC000
> + */
> + fep->hwp_enet = fep->enet_addr;
> + fecp = (struct switch_t *)(fep->enet_addr + ENET_SWI_PHYS_ADDR_OFFSET);
> +
> + fep->hwp = fecp;
> + fep->hwentry = (struct mtip_addr_table_t *)
> + ((unsigned long)fecp + MCF_ESW_LOOKUP_MEM_OFFSET);
> +
> + ret = devm_regulator_get_enable_optional(&pdev->dev, "phy");
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "Unable to get and enable 'phy'\n");
> +
> + fep->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
> + if (IS_ERR(fep->clk_ipg))
> + return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_ipg),
> + "Unable to acquire 'ipg' clock\n");
> +
> + fep->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
> + if (IS_ERR(fep->clk_ahb))
> + return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_ahb),
> + "Unable to acquire 'ahb' clock\n");
> +
> + fep->clk_enet_out = devm_clk_get_optional_enabled(&pdev->dev,
> + "enet_out");
> + if (IS_ERR(fep->clk_enet_out))
> + return dev_err_probe(&pdev->dev, PTR_ERR(fep->clk_enet_out),
> + "Unable to acquire 'enet_out' clock\n");
> +
> + spin_lock_init(&fep->learn_lock);
> + spin_lock_init(&fep->hw_lock);
> + spin_lock_init(&fep->mii_lock);
> +
> + ret = devm_request_irq(&pdev->dev, fep->irq, mtip_interrupt, 0,
> + "mtip_l2sw", fep);
> + if (ret)
> + return dev_err_probe(&pdev->dev, fep->irq,
> + "Could not alloc IRQ\n");
> +
> + ret = mtip_register_notifiers(fep);
> + if (ret)
> + return ret;
> +
> + ret = mtip_ndev_init(fep);
> + if (ret) {
> + dev_err(&pdev->dev, "%s: Failed to create virtual ndev (%d)\n",
> + __func__, ret);
> + goto ndev_init_err;
> + }
> +
> + ret = mtip_switch_dma_init(fep);
> + if (ret) {
> + dev_err(&pdev->dev, "%s: ethernet switch init fail (%d)!\n",
> + __func__, ret);
> + goto dma_init_err;
> + }
> +
> + ret = mtip_mii_init(fep, pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "%s: Cannot init phy bus (%d)!\n", __func__,
> + ret);
> + goto mii_init_err;
> + }
> + /* setup timer for learning aging function */
> + timer_setup(&fep->timer_aging, mtip_aging_timer, 0);
> + mod_timer(&fep->timer_aging,
> + jiffies + msecs_to_jiffies(LEARNING_AGING_INTERVAL));
> +
> + fep->task = kthread_run(mtip_sw_learning, fep, "mtip_l2sw_learning");
> + if (IS_ERR(fep->task)) {
> + ret = PTR_ERR(fep->task);
> + dev_err(&pdev->dev, "%s: learning kthread_run error (%d)!\n",
> + __func__, ret);
> + goto task_learning_err;
> + }
> +
> + /* setup MII interface for external switch ports*/
> + mtip_enet_init(fep, 1);
> + mtip_enet_init(fep, 2);
> +
> + return 0;
> +
> + task_learning_err:
> + del_timer(&fep->timer_aging);
> + mtip_mii_unregister(fep);
> + mii_init_err:
> + dma_init_err:
> + mtip_ndev_cleanup(fep);
> + ndev_init_err:
> + mtip_unregister_notifiers(fep);
> +
> + return ret;
> +}
> +
> +static void mtip_sw_remove(struct platform_device *pdev)
> +{
> + struct switch_enet_private *fep = platform_get_drvdata(pdev);
> +
> + mtip_unregister_notifiers(fep);
> + mtip_ndev_cleanup(fep);
> +
> + mtip_mii_remove(fep);
> +
> + kthread_stop(fep->task);
> + del_timer(&fep->timer_aging);
> + platform_set_drvdata(pdev, NULL);
> +
> + kfree(fep);
> +}
> +
> +static const struct of_device_id mtipl2_of_match[] = {
> + { .compatible = "nxp,imx287-mtip-switch", },
> + { /* sentinel */ }
> +};

Missing module device table.

> +
> +static struct platform_driver mtipl2plat_driver = {
> + .driver = {
> + .name = "mtipl2sw",
> + .of_match_table = mtipl2_of_match,
> + .suppress_bind_attrs = true,
> + },
> + .probe = mtip_sw_probe,
> + .remove_new = mtip_sw_remove,
> +};
> +
> +module_platform_driver(mtipl2plat_driver);
> +MODULE_AUTHOR("Lukasz Majewski <lukma@denx.de>");
> +MODULE_DESCRIPTION("Driver for MTIP L2 on SOC switch");
> +MODULE_VERSION(VERSION);

What is the point of paralell versioning with the kernel? Are you going
to keep this updated or - just like in other cases - it will stay always
theh same. Look for example at net/bridge/br.c or some other files -
they are always the same even if driver changed significantly.

BTW, this would be 1.0, not 1.4. Your out of tree versioning does not
matter.

> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:mtipl2sw");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.

Best regards,
Krzysztof