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:

quoted

+	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;
}

quoted

+	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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel