linux-kernel - Re: Re: [PATCH v7 2/2] ethernet: eswin: Add eic7700 ethernet driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <30080c70.16e1.199748921d3.Coremail.weishangjuan@eswincomputing.com>
Date: Tue, 23 Sep 2025 11:06:08 +0800 (GMT+08:00)
From: 韦尚娟 <weishangjuan@...incomputing.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: devicetree@...r.kernel.org, andrew+netdev@...n.ch, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, netdev@...r.kernel.org,
	pabeni@...hat.com, mcoquelin.stm32@...il.com,
	alexandre.torgue@...s.st.com, vladimir.oltean@....com,
	yong.liang.choong@...ux.intel.com, anthony.l.nguyen@...el.com,
	prabhakar.mahadev-lad.rj@...renesas.com, jan.petrous@....nxp.com,
	jszhang@...nel.org, inochiama@...il.com, 0x1207@...il.com,
	boon.khai.ng@...era.com, linux-kernel@...r.kernel.org,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, ningyu@...incomputing.com,
	linmin@...incomputing.com, lizhi2@...incomputing.com,
	pinkesh.vaghela@...fochips.com
Subject: Re: Re: [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@...linux.org.uk>
> 发送时间:2025-09-19 01:16:38 (星期五)
> 收件人: weishangjuan@...incomputing.com
> 抄送: devicetree@...r.kernel.org, andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, netdev@...r.kernel.org, pabeni@...hat.com, mcoquelin.stm32@...il.com, alexandre.torgue@...s.st.com, vladimir.oltean@....com, yong.liang.choong@...ux.intel.com, anthony.l.nguyen@...el.com, prabhakar.mahadev-lad.rj@...renesas.com, jan.petrous@....nxp.com, jszhang@...nel.org, inochiama@...il.com, 0x1207@...il.com, boon.khai.ng@...era.com, linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, linux-arm-kernel@...ts.infradead.org, ningyu@...incomputing.com, linmin@...incomputing.com, lizhi2@...incomputing.com, pinkesh.vaghela@...fochips.com
> 主题: Re: [PATCH v7 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> On Thu, Sep 18, 2025 at 05:00:26PM +0800, weishangjuan@...incomputing.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!