linux-kernel - 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: <acc244a7-54e8-49d0-848e-6eafb850c93b@kernel.org>
Date: Fri, 16 May 2025 15:30:48 +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:58, Piotr Kubik wrote:
> On 5/15/25 17:40, 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.
>>> This approach is similar to other drivers, all current PSE drivers log it this way.
>>>
>> And now I noticed that you already sent it, you got review:
>> https://lore.kernel.org/all/6ee047d4-f3de-4c25-aaae-721221dc3003@kernel.org/
>>
>> and you ignored it completely sending the same again.
>>
>> Sending the same over and over and asking us to do the same review over
>> and over is really waste of our time.
>>
>> Go back to v1, implement entire review. Then start versioning your patches.
>>
>> Best regards,
>> Krzysztof
> 
> 
> I didn't ignore, I replied to your comment, since there was no answer I assumed you agree.
> https://lore.kernel.org/all/38b02e2d-7935-4a23-b351-d23941e781b0@adtran.com/
> 
> Thanks for a reference and explanation, I'll change it.
Coding style has it pretty explicit as well.

Best regards,
Krzysztof