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: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