Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Jakub Kicinski <kuba@kernel.org>
CC:     "David S . Miller" <davem@davemloft.net>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        Rob Herring <robh+dt@kernel.org>,
        Shawn Guo <shawnguo@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Prasanna Vengateshan <prasanna.vengateshan@microchip.com>,
        Ansuel Smith <ansuelsmth@gmail.com>,
        =?Windows-1252?Q?Alvin_=8Aipraga?= <alsi@bang-olufsen.dk>
Subject: Re: [PATCH net-next 6/6] net: dsa: sja1105: parse
 {rx,tx}-internal-delay-ps properties for RGMII delays
Thread-Topic: [PATCH net-next 6/6] net: dsa: sja1105: parse
 {rx,tx}-internal-delay-ps properties for RGMII delays
Thread-Index: AQHXwIEVq1/AgHYHDEKe3jQ+7QpyaqvRosgAgADCSAA=
Date:   Thu, 14 Oct 2021 12:00:11 +0000
Message-ID: <20211014120010.im3bvg6ga7i2423n@skbuf>
References: <20211013222313.3767605-1-vladimir.oltean@nxp.com>
 <20211013222313.3767605-7-vladimir.oltean@nxp.com>
 <20211013172448.2db3b99b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <20211013172448.2db3b99b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?Windows-1252?Q?XAPB+yYg+ulqDb9ux2Zwky/Mio2FbFvwqSEzLzvKdquLy1VI2yiSQlWa?=
 =?Windows-1252?Q?n0dURy9lCAkX4YL+EcnKUrNcBp58MK7QpYRKNMFLNJvxL3ds9wGAVutN?=
 =?Windows-1252?Q?kGYCt7u+Hn3sFmRrSylBbaCjVwR94IJ2GV72GgpgfPOjpWS7YGy2ll/0?=
 =?Windows-1252?Q?SF6180vCP1UIA4PHDgez8TrO83J5wGd8P1Fn/rGwxwPHrtvFIOUZLtJu?=
 =?Windows-1252?Q?1PzU9xQ6LSTWYSjXdB783rC8viKDssKDSrkQ7QQu2q9unuSEy1aMCNA5?=
 =?Windows-1252?Q?EoYDQ/h0K+EkECgvXT5Kf+gPHzav3Bk2JQQatgvNAxXeJZHOYIoY+ZD+?=
 =?Windows-1252?Q?Ho3Gc92fpd4MWh5/iL3smbR8aaSPIwfFQeky5EZWfbi8ja93mzIjS06j?=
 =?Windows-1252?Q?vGs7ydoToFj4Rpt0QiXEo13patsj2KAqCSrFlA2EdtG26P0j7mLannbu?=
 =?Windows-1252?Q?2j9Dbvku8NHS2Yni1XS1X6g5EZw+xxwIfD12DWQLMG4+OnW7O0LVeU0U?=
 =?Windows-1252?Q?etRJJRi7dxaibdKqklWZqlNM775xiICgPDViIku1HtDKhASXxpFe/ivt?=
 =?Windows-1252?Q?5Gr6sUaJ/vkBu7Tyueg35Xhjg63nBXQN8Qok/sAHQalPORBvZLZmqKaL?=
 =?Windows-1252?Q?ENMmAzEJvq7yJDy5EXtfV+DXOHKPLBAMCEe5N8K96RBp7RRSQrZkmj5T?=
 =?Windows-1252?Q?YdU2t6vwxf7IzzJrtLw1R+/jmx6oftSDsKFlyhesicpl65ZFYV4gEqo6?=
 =?Windows-1252?Q?GZSrIfPd+OwrErRwI6DeZBtJuWw+loolt/eLdbXX2TwujSy3FhOSrTvD?=
 =?Windows-1252?Q?Zxl/2ujYf+TewwmeKWj3TNH9KRAkN/Tkj55M7VAKSy1Mx7RFwVlLiKAV?=
 =?Windows-1252?Q?1b7DrIFM8J6g4PJsyR8Bwc5NjGmCtL9ajMD7E9QYrhWkZfEX7ecZtYSX?=
 =?Windows-1252?Q?DxRIpOCUpcIhkZrewkQ/TXrHgodsvOGs5e3IKRYBUE6D2ZunfZruYi1z?=
 =?Windows-1252?Q?q9xtTKXB+w2uTCv5BmOsWMUDbgqWSucPFZdUw23w6IHJc3G5DyMTsSwd?=
 =?Windows-1252?Q?UueNLoqzGZyQAqARulFqAYYXrGQgdH3Giv9suNPgnHH0WlSr86IjIZ8q?=
 =?Windows-1252?Q?GqB96fkTy8Y6apjZhjt6MvKkUID1HTsbpxVltObhgFOW3pk6EqIGSSI6?=
 =?Windows-1252?Q?LMGOf+A/QcY2hTZ1yKYc7FFKFmQvP/idgsigEYdmAsE+TGCASiLdKFo0?=
 =?Windows-1252?Q?PaXTit2Suw+JUCvmtJoEhEd9uVBhGdElDwAlZ1xJK0a38vplvMRe5UEZ?=
 =?Windows-1252?Q?ni9srZrWWeWgcgPeTuXlIFOHFWjn5hUSDdqZvdYCW140R9LdHh2G+H8t?=
 =?Windows-1252?Q?sxTk5AEr0AbeYys6ojAu2DvTdZR0S740ts7tJF8zzgRaZuiIxMi0KvBZ?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="Windows-1252"
Content-ID: <28CAFDAF059A324692B703DDBA1DB6F6@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e9f4fc6a-d34c-44e4-a7be-08d98f0a2cc0
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Oct 2021 12:00:11.1305
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: a2RRF74/ve1k8B0H0VHgirv3v2KCIww8TbPPqheviAFUOvv+5ap063hO/hfS9mdA6i8UyhpmYGFjdu1tMgCHqA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB4814
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.
>=20
> On Thu, 14 Oct 2021 01:23:13 +0300 Vladimir Oltean wrote:
> > +	int rx_delay =3D -1, tx_delay =3D -1;
> > =20
> > +	if (!phy_interface_mode_is_rgmii(phy_mode))
> > +		return 0;
> > =20
> > +	of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
> > +	of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);
>=20
> 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:
>=20
> 	u32 rx_delay;
>=20
> 	if (of_property_read_u32(...))
> 		rx_delay =3D 0;
> 	else if (rx_delay !=3D clamp(rx_delay, ...MIN, ...MAX)
> 		goto err;
>=20
> 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 por=
t,
				      struct device_node *port_dn)
{
	phy_interface_t phy_mode =3D priv->phy_mode[port];
	struct device *dev =3D &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 =3D of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay=
);
	if (err_rx)
		rx_delay =3D 0;

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

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

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

			if (phy_mode =3D=3D PHY_INTERFACE_MODE_RGMII_TXID ||
			    phy_mode =3D=3D PHY_INTERFACE_MODE_RGMII_ID)
				tx_delay =3D 2000;
		}
	} else {
		if ((rx_delay && rx_delay !=3D clamp(rx_delay, SJA1105_RGMII_DELAY_MIN_PS=
, SJA1105_RGMII_DELAY_MAX_PS)) ||
		    (tx_delay && tx_delay !=3D 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] =3D rx_delay;
	priv->rgmii_tx_delay_ps[port] =3D tx_delay;

	return 0;
}

>=20
> > +	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)) {
>=20
> 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.=