Received-SPF: pass (google.com: domain of linux-kernel+bounces-27202-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 16 Jan 2024 10:49:49 +0100
From: =?UTF-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>, Eric Dumazet
 <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
 <pabeni@redhat.com>, Jonathan Corbet <corbet@lwn.net>, Luis Chamberlain
 <mcgrof@kernel.org>, Russ Weight <russ.weight@linux.dev>, Greg
 Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki"
 <rafael@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
 Oleksij Rempel <o.rempel@pengutronix.de>, Thomas Petazzoni
 <thomas.petazzoni@bootlin.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
 devicetree@vger.kernel.org, Dent Project <dentproject@linuxfoundation.org>
Subject: Re: [PATCH net-next v2 8/8] net: pse-pd: Add PD692x0 PSE controller
 driver
Message-ID: <20240116104949.12708cd5@kmaincent-XPS-13-7390>
In-Reply-To: <639c5222-043f-4e27-9efa-ce2a1d73eaba@lunn.ch>
References: <20231201-feature_poe-v2-0-56d8cac607fa@bootlin.com>
	<20231201-feature_poe-v2-8-56d8cac607fa@bootlin.com>
	<639c5222-043f-4e27-9efa-ce2a1d73eaba@lunn.ch>
Organization: bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hell Andrew,

Thanks for your reviews and sorry for replying so late, I was working on the
core to fit the new bindings and requirements lifted by Oleksij.

On Sun, 3 Dec 2023 20:34:54 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +static int pd692x0_try_recv_msg(const struct i2c_client *client,
> > +				struct pd692x0_msg *msg,
> > +				struct pd692x0_msg *buf)
> > +{
> > +	msleep(30);
> > +
> > +	memset(buf, 0, sizeof(*buf));
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		return 1;
> > +
> > +	msleep(100);
> > +
> > +	memset(buf, 0, sizeof(*buf));
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		return 1;
> > +
> > +	return 0; =20
>=20
> Maybe make this function return a bool? Or 0 on success, -EIO on
> error?

Indeed, I will move on to bool.

> > +static int pd692x0_update_matrix(struct pd692x0_priv *priv)
> > +{
> > +	struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS];
> > +	struct device *dev =3D &priv->client->dev;
> > +	int ret;
> > +
> > +	ret =3D pd692x0_get_of_matrix(dev, port_matrix);
> > +	if (ret < 0) {
> > +		dev_warn(dev,
> > +			 "Unable to parse port-matrix, saved matrix will
> > be used\n");
> > +		return 0;
> > +	}
> > +
> > +	ret =3D pd692x0_set_ports_matrix(priv, port_matrix);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +#define PD692X0_FW_LINE_MAX_SZ 0xff =20
>=20
> That probably works. Most linkers producing SREC output do limit
> themselves to lines of 80 charactors max. But the SREC format actually
> allows longer lines.

I set it to SREC limit but indeed the firmware lines does not exceed 80
characters except the comments. 0xff line size limit won't break anything
though.

> > +static int pd692x0_fw_get_next_line(const u8 *data,
> > +				    char *line, size_t size)
> > +{
> > +	size_t line_size;
> > +	int i;
> > +
> > +	line_size =3D min_t(size_t, size, (size_t)PD692X0_FW_LINE_MAX_SZ);
> > +
> > +	memset(line, 0, PD692X0_FW_LINE_MAX_SZ);
> > +	for (i =3D 0; i < line_size - 1; i++) {
> > +		if (*data =3D=3D '\r' && *(data + 1) =3D=3D '\n') {
> > +			line[i] =3D '\r';
> > +			line[i + 1] =3D '\n';
> > +			return i + 2;
> > +		} =20
>=20
> Does the Vendor Documentation indicate Windoze line endings will
> always be used? Motorola SREC allow both Windows or rest of the world
> line endings to be used.=20

All the firmware lines end with "\r\n" but indeed it is not specifically
written that the firmware content would follow this. IMHO it is implicit th=
at
it would be the case as all i2c messages use this line termination.
Do you prefer that I add support to the world line endings possibility?=20

> > +static enum fw_upload_err pd692x0_fw_poll_complete(struct fw_upload *f=
wl)
> > +{
> > +	struct pd692x0_priv *priv =3D fwl->dd_handle;
> > +	const struct i2c_client *client =3D priv->client;
> > +	struct pd692x0_msg_ver ver;
> > +	int ret;
> > +
> > +	priv->fw_state =3D PD692X0_FW_COMPLETE;
> > +
> > +	ret =3D pd692x0_fw_reset(client);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ver =3D pd692x0_get_sw_version(priv);
> > +	if (ver.maj_sw_ver !=3D PD692X0_FW_MAJ_VER) { =20
>=20
> That is probably too strong a condition. You need to allow firmware
> upgrades, etc. Does it need to be an exact match, or would < be
> enough?

The major version is not compatible with the last one, the i2c messages
content changed. I supposed a change in major version would imply a change =
in
the i2c messages content and would need a driver update that's why I used t=
his
strong condition.

--=20
K=C3=B6ry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com