Re: [PATCH net-next 6/6] net: dsa: sja1105: parse {rx,tx}-internal-delay-ps properties for RGMII delays

[Vladimir Oltean <vladimir.oltean@xxxxxxx>]

On Wed, Oct 13, 2021 at 05:24:48PM -0700, Jakub Kicinski wrote:
> Some take it or leave it comments, checkpatch pointed out some extra
> brackets so I had a look at the patch.
> 
> On Thu, 14 Oct 2021 01:23:13 +0300 Vladimir Oltean wrote:
> > +	int rx_delay = -1, tx_delay = -1;
> >  
> > +	if (!phy_interface_mode_is_rgmii(phy_mode))
> > +		return 0;
> >  
> > +	of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
> > +	of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);
> 
> If I'm reading this right you're depending on delays being left as -1
> in case the property reads fail. Is this commonly accepted practice?

It works.

> Why not code it up as:
> 
> 	u32 rx_delay;
> 
> 	if (of_property_read_u32(...))
> 		rx_delay = 0;
> 	else if (rx_delay != clamp(rx_delay, ...MIN, ...MAX)
> 		goto err;
> 
> or some such?

"or some such" is not functionally equivalent.

This is what would be functionally equivalent, and following your
suggestion to check the return code of of_property_read_u32 instead of
assigning default values, and to use clamp() instead of open-coding the
bounds checks.

static int sja1105_parse_rgmii_delays(struct sja1105_private *priv, int port,
				      struct device_node *port_dn)
{
	phy_interface_t phy_mode = priv->phy_mode[port];
	struct device *dev = &priv->spidev->dev;
	int rx_delay, tx_delay;
	int err_rx, err_tx;

	if (!phy_interface_mode_is_rgmii(phy_mode))
		return 0;

	err_rx = of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
	if (err_rx)
		rx_delay = 0;

	err_tx = of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);
	if (err_tx)
		tx_delay = 0;

	if (err_rx && err_tx) {
		if (priv->fixed_link[port]) {
			dev_warn(dev,
				 "Port %d interpreting RGMII delay settings based on \"phy-mode\" property, "
				 "please update device tree to specify \"rx-internal-delay-ps\" and "
				 "\"tx-internal-delay-ps\"",
				 port);

			if (phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
			    phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
				rx_delay = 2000;

			if (phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
			    phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
				tx_delay = 2000;
		}
	} else {
		if ((rx_delay && rx_delay != clamp(rx_delay, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS)) ||
		    (tx_delay && tx_delay != clamp(tx_delay, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS))) {
			dev_err(dev,
				"port %d RGMII delay values out of range, must be between %d and %d ps\n",
				port, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS);
			return -ERANGE;
		}
	}

	priv->rgmii_rx_delay_ps[port] = rx_delay;
	priv->rgmii_tx_delay_ps[port] = tx_delay;

	return 0;
}

> 
> > +	if ((rx_delay && rx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
> > +	    (tx_delay && tx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
> > +	    (rx_delay > SJA1105_RGMII_DELAY_MAX_PS) ||
> > +	    (tx_delay > SJA1105_RGMII_DELAY_MAX_PS)) {
> 
> nit: checkpatch says the brackets around the latter two are unnecessary,
>      just in case it's not for symmetry / on purpose

It is on purpose.