Message-ID: <uhup3bm6ez6kg7efvimy6rcthmqfcdkg2vmcwafqz33vouplfl@i25wn6q6c4h6>
Date: Tue, 11 Feb 2025 08:47:02 +0800
From: Inochi Amaoto <inochiama@...il.com>
To: Romain Gantois <romain.gantois@...tlin.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Chen Wang <unicorn_wang@...look.com>, Inochi Amaoto <inochiama@...look.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Richard Cochran <richardcochran@...il.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Emil Renner Berthing <emil.renner.berthing@...onical.com>, Jisheng Zhang <jszhang@...nel.org>, 
	"Jan Petrous (OSS)" <jan.petrous@....nxp.com>, Clément Léger <clement.leger@...tlin.com>, 
	Simon Horman <horms@...nel.org>, Furong Xu <0x1207@...il.com>, 
	Serge Semin <fancer.lancer@...il.com>, Lothar Rubusch <l.rubusch@...il.com>, 
	Suraj Jaiswal <quic_jsuraj@...cinc.com>, Joe Hattori <joe@...is.s.u-tokyo.ac.jp>, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Giuseppe Cavallaro <peppe.cavallaro@...com>, 
	Jose Abreu <joabreu@...opsys.com>, Inochi Amaoto <inochiama@...il.com>
Cc: netdev@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org, linux-riscv@...ts.infradead.org, Yixun Lan <dlan@...too.org>, 
	Longbin Li <looong.bin@...il.com>
Subject: Re: [PATCH net-next v4 3/3] net: stmmac: Add glue layer for Sophgo
 SG2044 SoC

On Mon, Feb 10, 2025 at 12:01:56PM +0100, Romain Gantois wrote:
> Hello Inochi,
> 
> On dimanche 9 février 2025 02:30:52 heure normale d’Europe centrale Inochi 
> Amaoto wrote:
> > Adds Sophgo dwmac driver support on the Sophgo SG2044 SoC.
> ...
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
> > @@ -0,0 +1,105 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Sophgo DWMAC platform driver
> > + *
> > + * Copyright (C) 2024 Inochi Amaoto <inochiama@...il.com>
> > + */
> > +
> > +#include <linux/bits.h>
> 
> It doesn't look like this include is used, could you please remove it?
> 

Thanks, I will. And I will add the miss header 
linux/clk.h and linux/module.h.

> > +#include <linux/mod_devicetable.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "stmmac_platform.h"
> > +
> > +struct sophgo_dwmac {
> > +	struct device *dev;
> > +	struct clk *clk_tx;
> > +};
> > +
> > +static void sophgo_dwmac_fix_mac_speed(void *priv, unsigned int speed,
> > unsigned int mode) +{
> > +	struct sophgo_dwmac *dwmac = priv;
> > +	long rate;
> > +	int ret;
> > +
> > +	rate = rgmii_clock(speed);
> > +	if (rate < 0) {
> > +		dev_err(dwmac->dev, "invalid speed %u\n", speed);
> > +		return;
> > +	}
> > +
> > +	ret = clk_set_rate(dwmac->clk_tx, rate);
> > +	if (ret)
> > +		dev_err(dwmac->dev, "failed to set tx rate %lu: %pe\n",
> 
> nit: shouldn't this be "%ld"?
> 

Yeah, it is my mistake, I will fix it.

> > +			rate, ERR_PTR(ret));
> > +}
> > +
> > +static int sophgo_sg2044_dwmac_init(struct platform_device *pdev,
> > +				    struct plat_stmmacenet_data *plat_dat,
> > +				    struct stmmac_resources *stmmac_res)
> > +{
> > +	struct sophgo_dwmac *dwmac;
> > +
> > +	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
> > +	if (!dwmac)
> > +		return -ENOMEM;
> > +
> > +	dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
> > +	if (IS_ERR(dwmac->clk_tx))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
> > +				     "failed to get tx clock\n");
> > +
> > +	dwmac->dev = &pdev->dev;
> > +	plat_dat->bsp_priv = dwmac;
> > +	plat_dat->flags |= STMMAC_FLAG_SPH_DISABLE;
> > +	plat_dat->fix_mac_speed = sophgo_dwmac_fix_mac_speed;
> > +	plat_dat->multicast_filter_bins = 0;
> > +	plat_dat->unicast_filter_entries = 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sophgo_dwmac_probe(struct platform_device *pdev)
> > +{
> > +	struct plat_stmmacenet_data *plat_dat;
> > +	struct stmmac_resources stmmac_res;
> 
> nit: I think adding "struct device *dev = &pdev->dev;" here would
> be better than repeating "&pdev->dev" later on.
> 

Thanks, I will change that.

> > +	int ret;
> > +
> > +	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to get resources\n");
> 
> This error message is a bit too vague, maybe replace it with "failed to get 
> platform resources"?
> 

OK.

> > +
> > +	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> > +	if (IS_ERR(plat_dat))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
> > +				     "dt configuration failed\n");
> 
> This error message is a bit misleading IMO, I would replace it with
> something like "failed to parse device-tree parameters".
> 

OK.

> > +
> > +	ret = sophgo_sg2044_dwmac_init(pdev, plat_dat, &stmmac_res);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > +}
> > +
> > +static const struct of_device_id sophgo_dwmac_match[] = {
> > +	{ .compatible = "sophgo,sg2044-dwmac" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sophgo_dwmac_match);
> > +
> > +static struct platform_driver sophgo_dwmac_driver = {
> > +	.probe  = sophgo_dwmac_probe,
> > +	.remove = stmmac_pltfr_remove,
> > +	.driver = {
> > +		.name = "sophgo-dwmac",
> > +		.pm = &stmmac_pltfr_pm_ops,
> > +		.of_match_table = sophgo_dwmac_match,
> > +	},
> > +};
> > +module_platform_driver(sophgo_dwmac_driver);
> > +
> > +MODULE_AUTHOR("Inochi Amaoto <inochiama@...il.com>");
> > +MODULE_DESCRIPTION("Sophgo DWMAC platform driver");
> > +MODULE_LICENSE("GPL");
> 
> Thanks,
> 
> -- 
> Romain Gantois, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Powered by blists - more mailing lists