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

[李志 <lizhi2@eswincomputing.com>]

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@armlinux.org.uk>
> 发送时间:2025-09-04 18:26:24 (星期四)
> 收件人: weishangjuan@eswincomputing.com
> 抄送: devicetree@vger.kernel.org, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-arm-kernel@lists.infradead.org, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, faizal.abdul.rahim@linux.intel.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, jan.petrous@oss.nxp.com, jszhang@kernel.org, p.zabel@pengutronix.de, boon.khai.ng@altera.com, 0x1207@gmail.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, emil.renner.berthing@canonical.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, lizhi2@eswincomputing.com, pinkesh.vaghela@einfochips.com
> 主题: Re: [PATCH v5 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> On Thu, Sep 04, 2025 at 05:01:25PM +0800, weishangjuan@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!