[PATCH v7 2/2] ethernet: eswin: Add eic7700 ethernet driver

[韦尚娟]

Hi King,
I hope this message finds you well.
Thank you for your professional and valuable suggestions.
Our questions are embedded below your comments in the original email below.

Best regards,
Shangjuan Wei


> -----原始邮件-----
> 发件人: "Russell King (Oracle)" <linux at armlinux.org.uk>
> 发送时间:2025-09-19 01:16:38 (星期五)
> 收件人: weishangjuan at eswincomputing.com
> 抄送: devicetree at vger.kernel.org, andrew+netdev at lunn.ch, davem at davemloft.net, edumazet at google.com, kuba at kernel.org, robh at kernel.org, krzk+dt at kernel.org, conor+dt at kernel.org, netdev at vger.kernel.org, pabeni at redhat.com, mcoquelin.stm32 at gmail.com, alexandre.torgue at foss.st.com, vladimir.oltean at nxp.com, yong.liang.choong at linux.intel.com, anthony.l.nguyen at intel.com, prabhakar.mahadev-lad.rj at bp.renesas.com, jan.petrous at oss.nxp.com, jszhang at kernel.org, inochiama at gmail.com, 0x1207 at gmail.com, boon.khai.ng at altera.com, linux-kernel at vger.kernel.org, linux-stm32 at st-md-mailman.stormreply.com, linux-arm-kernel at lists.infradead.org, ningyu at eswincomputing.com, linmin at eswincomputing.com, lizhi2 at eswincomputing.com, pinkesh.vaghela at einfochips.com
> 主题: Re: [PATCH v7 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> On Thu, Sep 18, 2025 at 05:00:26PM +0800, weishangjuan at eswincomputing.com wrote:
> > +	plat_dat->clk_tx_i = stmmac_pltfr_find_clk(plat_dat, "tx");
> > +	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
> > +	plat_dat->bsp_priv = dwc_priv;
> > +	plat_dat->clks_config = eic7700_clks_config;
> > +	dwc_priv->plat_dat = plat_dat;
> > +
> > +	ret = eic7700_clks_config(dwc_priv, true);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"error enable clock\n");
> > +
> > +	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > +	if (ret) {
> > +		eic7700_clks_config(dwc_priv, false);
> > +		return dev_err_probe(&pdev->dev,
> > +				ret,
> > +				"Failed to driver probe\n");
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void eic7700_dwmac_remove(struct platform_device *pdev)
> > +{
> > +	struct eic7700_qos_priv *dwc_priv = get_stmmac_bsp_priv(&pdev->dev);
> > +
> > +	stmmac_pltfr_remove(pdev);
> > +	eic7700_clks_config(dwc_priv, false);
> 
> It would be nice to see the above code cleaned up like I did for all
> the other stmmac glue drivers recently.
> 
> However, this is not to say this shouldn't be merged - but please
> consider this if you do another rework of these patches, if not as
> a follow-up patch.
> 
> Essentially, you can use devm_stmmac_pltfm_probe(), populate the
> plat_dat->init() and plat_dat->exit() methods to call the
> clks_config function, but as you don't want these methods to be
> called during suspend/resume (because plat_dat->clks_config() is
> already called there), provide empty plat_dat->suspend() and
> plat_dat->resume() methods.
> 
> Bonus points if you include a patch which provides this functionality
> as library functions in stmmac_platform.c which can be used to
> initialise ->init() and ->exit() for this behaviour, and check other
> stmmac platform glue drivers to see if they would benefit from using
> these.
> 

In the current eic7700_dwmac glue driver, the regmap_read()/write()
operations(for phy_ctrl1, axi_lp_ctrl1, and the RX/TX delay registers))are 
performed directly in the probe() function. Would it be cleaner to move these
register configurations into the init() callback instead, so that they are
also reapplied during resume()?

> Of course, it would be nice not to have to go to the extent of
> adding empty functions for ->suspend() and ->resume(), but stmmac has
> a lot of weirdo history, and there was no easy way to maintain
> compatibility without doing that when I added these two new methods.
> 
> Lastly, please consider using "net: stmmac: <shortened-glue-name>: blah"
> as the subject so there's a consistent style for stmmac patches.
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!