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

[李志]

Dear Russell King,
Thank you for your valuable and professional suggestions.

You're right — tx_delay_ps, rx_delay_ps and hsp_regmap are
only used in probe, so we’ll switch them to local variables.
We’ll also inline the delay handling into probe as suggested.
This will be updated in the next version.

Best regards,

Li Zhi
Eswin Computing
> -----原始邮件-----
> 发件人: "Russell King (Oracle)" <linux at armlinux.org.uk>
> 发送时间:2025-09-04 18:26:24 (星期四)
> 收件人: 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, pabeni at redhat.com, robh at kernel.org, krzk+dt at kernel.org, conor+dt at kernel.org, linux-arm-kernel at lists.infradead.org, mcoquelin.stm32 at gmail.com, alexandre.torgue at foss.st.com, yong.liang.choong at linux.intel.com, vladimir.oltean at nxp.com, faizal.abdul.rahim at linux.intel.com, prabhakar.mahadev-lad.rj at bp.renesas.com, inochiama at gmail.com, jan.petrous at oss.nxp.com, jszhang at kernel.org, p.zabel at pengutronix.de, boon.khai.ng at altera.com, 0x1207 at gmail.com, netdev at vger.kernel.org, linux-kernel at vger.kernel.org, linux-stm32 at st-md-mailman.stormreply.com, emil.renner.berthing at canonical.com, ningyu at eswincomputing.com, linmin at eswincomputing.com, lizhi2 at eswincomputing.com, pinkesh.vaghela at einfochips.com
> 主题: Re: [PATCH v5 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> On Thu, Sep 04, 2025 at 05:01:25PM +0800, weishangjuan at eswincomputing.com wrote:
> > +struct eic7700_qos_priv {
> > +	struct plat_stmmacenet_data *plat_dat;
> > +	struct device *dev;
> > +	struct regmap *hsp_regmap;
> > +	u32 tx_delay_ps;
> > +	u32 rx_delay_ps;
> > +};
> > +
> > +/**
> > + * eic7700_apply_delay - Apply TX or RX delay to a register value.
> > + * @delay_ps: Delay in picoseconds, converted to 0.1ns units.
> > + * @reg:      Pointer to register value to update in-place.
> > + * @is_rx:    True for RX delay (bits 30:24), false for TX delay (bits 14:8).
> > + *
> > + * Converts delay from ps to 0.1ns units, capped by EIC7700_MAX_DELAY_UNIT.
> > + * Updates only the RX or TX delay field (using FIELD_PREP), leaving all
> > + * other bits in *@reg unchanged.
> > + */
> > +static void eic7700_apply_delay(u32 delay_ps, u32 *reg, bool is_rx)
> > +{
> > +	u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
> > +
> > +	if (is_rx) {
> > +		*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
> > +		*reg |= FIELD_PREP(EIC7700_ETH_RX_ADJ_DELAY, val);
> > +	} else {
> > +		*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
> > +		*reg |= FIELD_PREP(EIC7700_ETH_TX_ADJ_DELAY, val);
> > +	}
> > +}
> 
> ...
> 
> > +	/* Read rx-internal-delay-ps and update rx_clk delay */
> > +	if (!of_property_read_u32(pdev->dev.of_node,
> > +				  "rx-internal-delay-ps",
> > +				  &dwc_priv->rx_delay_ps)) {
> > +		eic7700_apply_delay(dwc_priv->rx_delay_ps,
> > +				    &eth_dly_param, true);
> 
> I've been trying to figure out the reasoning behind the following:
> 
> 1. the presence of dwc_priv->rx_delay_ps and dwc_priv->tx_delay_ps
>    rather than just using a local variable ("delay" ?)
> 2. the presence of eic7700_apply_delay() when we have to do something
>    different to get the delay value anyway
> 
> It seems to me that this should either be:
> 
> static void eic7700_parse_delay(u32 *reg, struct device *dev,
> 				const char *name, bool is_rx)
> {
> 	u32 delay;
> 
> 	if (of_property_read_u32(dev->of_node, name, &delay)) {
> 		dev_warn(dev, "can't get %s\n", name);
> 		return
> 	}
> 
> 	if (is_rx) {
> 		*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
> 		*reg |= FIELD_PREP(EIC7700_ETH_RX_ADJ_DELAY, delay);
> 	} else {
> 		*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
> 		*reg |= FIELD_PREP(EIC7700_ETH_TX_ADJ_DELAY, delay);
> 	}
> }
> 
> or just not bother with the function at all and just write it out
> fully in the probe function.
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!