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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3fb13368-0bef-421c-9568-754cdeef607b@kernel.org>
Date: Thu, 15 May 2025 17:35:51 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Piotr Kubik <piotr.kubik@...ran.com>,
 Oleksij Rempel <o.rempel@...gutronix.de>,
 Kory Maincent <kory.maincent@...tlin.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [EXTERNAL]Re: [PATCH net-next 2/2] net: pse-pd: Add Si3474 PSE
 controller driver

On 15/05/2025 17:32, Krzysztof Kozlowski wrote:
> 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.
... and I missed reference:

> In almost all cases the
> debug statements shouldn't be upstreamed, as a working driver is
> supposed to be

dev_info should not be upstreamed even more. Really, really there is no
need to tell the user that every time device was probed. That's obvious
thing. That's I2C, not really pluggable interface, unless you claim this
is on some mikrobus or other connector?

Best regards,
Krzysztof