Re: [PATCH net-next 2/2] net: pse-pd: Add Si3474 PSE controller driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 13/05/2025 00:06, Piotr Kubik wrote:
> +/* Parse pse-pis subnode into chan array of si3474_priv */
> +static int si3474_get_of_channels(struct si3474_priv *priv)
> +{
> +	struct device_node *pse_node, *node;
> +	struct pse_pi *pi;
> +	u32 pi_no, chan_id;
> +	s8 pairset_cnt;
> +	s32 ret = 0;
> +
> +	pse_node = of_get_child_by_name(priv->np, "pse-pis");
> +	if (!pse_node) {
> +		dev_warn(&priv->client[0]->dev,
> +			 "Unable to parse DT PSE power interface matrix, no pse-pis node\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_child_of_node(pse_node, node) {

Use scoped variant. One cleanup less.


> +		if (!of_node_name_eq(node, "pse-pi"))
> +			continue;

...

> +
> +	ret = i2c_smbus_read_byte_data(client, FIRMWARE_REVISION_REG);
> +	if (ret < 0)
> +		return ret;
> +	fw_version = ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, CHIP_REVISION_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(dev, "Chip revision: 0x%x, firmware version: 0x%x\n",

dev_dbg or just drop. Drivers should be silent on success.

> +		 ret, fw_version);
> +
> +	priv->client[0] = client;
> +	i2c_set_clientdata(client, priv);
> +
> +	priv->client[1] = i2c_new_ancillary_device(priv->client[0], "slave",
> +						   priv->client[0]->addr + 1);
> +	if (IS_ERR(priv->client[1]))
> +		return PTR_ERR(priv->client[1]);
> +
> +	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
> +	if (ret < 0) {
> +		dev_err(&priv->client[1]->dev, "Cannot access slave PSE controller\n");
> +		goto out_err_slave;
> +	}
> +
> +	if (ret != SI3474_DEVICE_ID) {
> +		dev_err(&priv->client[1]->dev,
> +			"Wrong device ID for slave PSE controller: 0x%x\n", ret);
> +		ret = -ENXIO;
> +		goto out_err_slave;
> +	}
> +
> +	priv->np = dev->of_node;
> +	priv->pcdev.owner = THIS_MODULE;
> +	priv->pcdev.ops = &si3474_ops;
> +	priv->pcdev.dev = dev;
> +	priv->pcdev.types = ETHTOOL_PSE_C33;
> +	priv->pcdev.nr_lines = SI3474_MAX_CHANS;
> +
> +	ret = devm_pse_controller_register(dev, &priv->pcdev);
> +	if (ret) {

No need for {}

> +		return dev_err_probe(dev, ret,
> +				     "Failed to register PSE controller\n");

No cleanup here? That's odd.

> +	}
> +
> +	return ret;

return 0

> +
> +out_err_slave:
> +	i2c_unregister_device(priv->client[1]);
> +	return ret;
> +}
> +
> +static void si3474_i2c_remove(struct i2c_client *client)
> +{
> +	struct si3474_priv *priv = i2c_get_clientdata(client);
> +
> +	i2c_unregister_device(priv->client[1]);

So you first unregister i2c device and then unregister pse controller.
Feels like possible issues, difficult to debug.... Use devm reset
wrapper for that.


Best regards,
Krzysztof