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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 15/05/2025 17:20, Piotr Kubik wrote:
> Thanks Krzysztof for your review,
> 
>> 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.
> 
> good point
> 
>>
>>
>>> +		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.
> 
> Is there any rule for this I'm not aware of? 
> I'd like to know that device is present and what versions it runs just by looking into dmesg.

sysfs is the interface for this.

> This approach is similar to other drivers, all current PSE drivers log it this way.

And this approach was dropped for many, many more drivers. One poor
choice should not be reason to do the same.


> 
>>
>>> +		 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.
>>
> 
> Indeed, will fix
> 
>>> +	}
>>> +
>>> +	return ret;
>>
>> return 0
>>
> 
> This is actually not needed. return above will return

Huh? I know it will, but this should be explicit 0. Don't make code more
complicated than it should be.



Best regards,
Krzysztof