Re: [PATCH net-next v2 8/8] net: pse-pd: Add PD692x0 PSE controller driver

[Köry Maincent]

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@xxxxxxx> 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;  
> 
> 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 = &priv->client->dev;
> > +	int ret;
> > +
> > +	ret = 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 = pd692x0_set_ports_matrix(priv, port_matrix);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +#define PD692X0_FW_LINE_MAX_SZ 0xff  
> 
> 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 = min_t(size_t, size, (size_t)PD692X0_FW_LINE_MAX_SZ);
> > +
> > +	memset(line, 0, PD692X0_FW_LINE_MAX_SZ);
> > +	for (i = 0; i < line_size - 1; i++) {
> > +		if (*data == '\r' && *(data + 1) == '\n') {
> > +			line[i] = '\r';
> > +			line[i + 1] = '\n';
> > +			return i + 2;
> > +		}  
> 
> 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. 

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 that
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? 

> > +static enum fw_upload_err pd692x0_fw_poll_complete(struct fw_upload *fwl)
> > +{
> > +	struct pd692x0_priv *priv = fwl->dd_handle;
> > +	const struct i2c_client *client = priv->client;
> > +	struct pd692x0_msg_ver ver;
> > +	int ret;
> > +
> > +	priv->fw_state = PD692X0_FW_COMPLETE;
> > +
> > +	ret = pd692x0_fw_reset(client);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ver = pd692x0_get_sw_version(priv);
> > +	if (ver.maj_sw_ver != PD692X0_FW_MAJ_VER) {  
> 
> 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 this
strong condition.

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com