Re: [PATCH net-next v2 2/2] net: maxlinear: Add support for MxL LGM SoC

[Krzysztof Kozlowski <krzk@kernel.org>]

On 26/08/2025 05:10, Jack Ping CHNG wrote:
> Introduce the build system integration and initial implementation for the

Nothing improved - do not describe how kernel works, builds. We all know
Kconfig.

Describe the driver and hardware.

> MaxLinear LGM SoC Ethernet driver. This patch adds Kconfig and Makefile

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95


...

> +
> +static int mxl_eth_probe(struct platform_device *pdev)
> +{
> +	struct mxl_eth_drvdata *drvdata;
> +	struct reset_control *rst;
> +	struct net_device *ndev;
> +	struct device_node *np;
> +	int ret, i;
> +
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->clks = devm_clk_get_enabled(&pdev->dev, "ethif");
> +	if (IS_ERR(drvdata->clks))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->clks),

That's correct...

> +				     "failed to get/enable clock\n");
> +
> +	rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> +	if (IS_ERR(rst)) {
> +		dev_err(&pdev->dev,
> +			"failed to get optional reset control: %ld\n",
> +			PTR_ERR(rst));


But here nothing improved.

Please look how ALL DRIVERS do it? What syntax they use?

Do your homework and really work on this driver. If I pointed issue on
clocks, YOU MUST fix it everywhere, not just clocks.


> +		ret = PTR_ERR(rst);
> +		goto err_cleanup;
> +	}
> +
> +	if (rst) {
> +		ret = reset_control_assert(rst);
> +		if (ret)
> +			goto err_cleanup;
> +
> +		udelay(1);
> +
> +		ret = reset_control_deassert(rst);
> +		if (ret)
> +			goto err_cleanup;
> +	}
> +
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	i = 0;
> +	for_each_available_child_of_node(pdev->dev.of_node, np) {
> +		if (!of_device_is_compatible(np, "mxl,eth-mac"))
> +			continue;
> +
> +		ret = mxl_eth_create_ndev(pdev, np, &ndev);
> +		if (ret)
> +			goto err_cleanup;

You leak of node. Use scoped loop.

> +
> +		drvdata->ndevs[i++] = ndev;
> +		if (i >= MXL_NUM_PORT)
> +			break;
> +	}
> +
> +	return 0;
> +
> +err_cleanup:
> +	mxl_eth_cleanup(drvdata);
> +	return ret;
> +}
> +
> +static void mxl_eth_remove(struct platform_device *pdev)
> +{
> +	struct mxl_eth_drvdata *drvdata = platform_get_drvdata(pdev);
> +
> +	mxl_eth_cleanup(drvdata);
> +}
> +
> +/* Device Tree match table */
> +static const struct of_device_id mxl_eth_of_match[] = {
> +	{ .compatible = "mxl,lgm-eth" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxl_eth_of_match);
> +
> +/* Platform driver struct */
> +static struct platform_driver mxl_eth_drv = {
> +	.probe    = mxl_eth_probe,
> +	.remove   = mxl_eth_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = mxl_eth_of_match,
> +	},
> +};
> +
> +module_platform_driver(mxl_eth_drv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Ethernet driver for MxL SoC");


Best regards,
Krzysztof