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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 22/08/2025 11:08, Jack Ping CHNG wrote:
> Add build infrastructure for MxL network driver.
> Ethernet driver to initialize and create network devices.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

What is a "build support for a driver/soc"? Confusing.


...

> + */
> +#include <linux/clk.h>
> +#include <linux/etherdevice.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +#define ETH_TX_TIMEOUT		(10 * HZ)
> +#define MXL_NUM_TX_RING		8
> +#define MXL_NUM_RX_RING		8
> +#define MXL_NUM_PORT		2
> +
> +static const char * const clk_names = "ethif";

Drop, pretty useless.

...

> +
> +static void mxl_eth_cleanup(struct mxl_eth_drvdata *pdata)
> +{
> +	int i;
> +
> +	for (i = 0; i < MXL_NUM_PORT && pdata->ndevs[i]; i++) {
> +		unregister_netdev(pdata->ndevs[i]);
> +		pdata->ndevs[i] = NULL;
> +	}
> +
> +	if (!IS_ERR(pdata->clks))

Hm? Why?

> +		clk_disable_unprepare(pdata->clks);
> +}
> +
> +static int mxl_eth_probe(struct platform_device *pdev)
> +{
> +	struct mxl_eth_drvdata *pdata;
> +	struct reset_control *rst;
> +	struct net_device *ndev;
> +	struct device_node *np;
> +	int ret, i;
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->clks = devm_clk_get(&pdev->dev, clk_names);
> +	if (IS_ERR(pdata->clks)) {
> +		dev_err(&pdev->dev, "failed to get %s\n", clk_names);

You are sending us some 10 year old coding style. This is supposed to be
dev_err_probe and devm_get_clk_enabled.

I think my second talk for OSSE 25 about static analyzers is also
suitable...

> +		return PTR_ERR(pdata->clks);
> +	}
> +
> +	ret = clk_prepare_enable(pdata->clks);

> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable %s\n", clk_names);
> +		return ret;
> +	}
> +
> +	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));
> +		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, pdata);
> +
> +	i = 0;
> +	for_each_available_child_of_node(pdev->dev.of_node, np) {
> +		if (!of_device_is_compatible(np, "mxl,eth-mac"))
> +			continue;
> +
> +		if (!of_device_is_available(np))

Redundant.


Best regards,
Krzysztof