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

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 01/12/2023 à 18:10, Kory Maincent a écrit :
> Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller.
> This driver only support i2c communication for now.
> 
> Sponsored-by: Dent Project <dentproject@linuxfoundation.org>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> This driver is based on the patch merged in an immutable branch from Jakub
> repo. It is Tagged at:
> git://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git firmware_loader-add-upload-error
> 
> Change in v2:
> - Drop of_match_ptr
> - Follow the "c33" PoE prefix naming change.
> - Remove unused delay_recv variable. Then, remove struct pd692x0_msg_content
>    which is similar to struct pd692x0_msg.
> - Fix a weird sleep loop.
> - Improve pd692x0_recv_msg for better readability.
> - Fix a warning reported by Simon on a pd692x0_fw_write_line call.
> ---

...

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

Nit: useless size_t cast
> +
> +	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;
> +		}
> +		line[i] = *data;
> +		data++;
> +	}
> +
> +	return 0;
> +}

...

> +static int pd692x0_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct pd692x0_msg buf = {0};
> +	struct pd692x0_msg_ver ver;
> +	struct pd692x0_priv *priv;
> +	struct fw_upload *fwl;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(dev, "i2c check functionality failed\n");
> +		return -ENXIO;
> +	}
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +	i2c_set_clientdata(client, priv);
> +
> +	priv->pcdev.owner = THIS_MODULE;
> +	priv->pcdev.ops = &pd692x0_ops;
> +	priv->pcdev.dev = dev;
> +	priv->pcdev.types = PSE_C33;
> +	priv->pcdev.of_pse_n_cells = 1;
> +	priv->pcdev.nr_lines = PD692X0_MAX_LOGICAL_PORTS;
> +	ret = devm_pse_controller_register(dev, &priv->pcdev);
> +	if (ret) {
> +		return dev_err_probe(dev, ret,
> +				     "failed to register PSE controller\n");
> +	}

Nit: un-needed {}

> +
> +	fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
> +				       &pd692x0_fw_ops, priv);
> +	if (IS_ERR(fwl)) {
> +		dev_err(dev, "Failed to register to the Firmware Upload API\n");
> +		ret = PTR_ERR(fwl);
> +		return ret;

Nit: return dev_err_probe()?

> +	}
> +	priv->fwl = fwl;
> +
> +	ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
> +	if (ret != sizeof(buf)) {
> +		dev_err(dev, "Failed to get device status\n");
> +		ret = -EIO;
> +		goto err_fw_unregister;
> +	}
> +
> +	if (buf.key != 0x03 || buf.echo != 0xff || buf.sub[0] & 0x01) {
> +		dev_err(dev, "PSE controller error\n");
> +		ret = -EIO;
> +		goto err_fw_unregister;
> +	}
> +
> +	if (buf.sub[0] & 0x02) {
> +		dev_err(dev, "PSE firmware error. Please update it.\n");
> +		priv->fw_state = PD692X0_FW_BROKEN;
> +		return 0;
> +	}
> +
> +	ver = pd692x0_get_sw_version(priv);
> +	dev_info(&client->dev, "Software version %d.%02d.%d.%d\n", ver.prod,
> +		 ver.maj_sw_ver, ver.min_sw_ver, ver.pa_sw_ver);
> +
> +	if (ver.maj_sw_ver != PD692X0_FW_MAJ_VER) {
> +		dev_err(dev, "Too old firmware version. Please update it\n");
> +		priv->fw_state = PD692X0_FW_NEED_UPDATE;
> +		return 0;
> +	}
> +
> +	ret = pd692x0_update_matrix(priv);
> +	if (ret < 0) {
> +		dev_err(dev, "Error configuring ports matrix (%pe)\n",
> +			ERR_PTR(ret));
> +		goto err_fw_unregister;
> +	}
> +
> +	priv->fw_state = PD692X0_FW_OK;
> +	return 0;
> +
> +err_fw_unregister:
> +	firmware_upload_unregister(priv->fwl);
> +	return ret;
> +}

...