linux-kernel - Re: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

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: <0a1df032-dfb6-4a26-a27d-14cc301cf12c@roeck-us.net>
Date: Sun, 25 Aug 2024 15:50:29 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Wenliang <wenliang202407@....com>
Cc: jdelvare@...e.com, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205

On 8/22/24 00:44, Wenliang wrote:
> Thank you for bringing up your questions and suggestions.The SQ52205 is a
> part number specific to the Asian region, which is why you might not be
> able to find it through a search. I'll provide you the website
> (https://us1.silergy.com/zh/productsview/SQ52205FBP).

That page does not point to the chip datasheet. The almost identical
page at https://us1.silergy.com/productsview/SQ52205FBP does.
The datasheet is _not_ "Publicly available" as claimed in this submission.
The version I was able to obtain is tagged with "Silergy Confidential For
<my employer>", so I am not even sure if I can use it to review this driver
submission.

> Some registers of this chip are similar to those of the INA226, but it has
> additional registers such as integrators, which is the main reason why I'm
> offering a new driver.And I plan add drivers of the same series based on

That is not a reason to add a separate driver. Look at, for example, lm90.c,
which supports a variety of chips in a single driver. The ina2xx driver already
does support several chips, and adding another one would be straightforward,
even if it is from a different manufacturer. On top of that, only the EIN and
ACCUM_CONFIG registers are additional, and the rest appear to be exactly the
same as INA226.

> this. I commit the new patch and look forward to your reply.
> 

Additonal comments:
- Please review and follow
   Documentation/process/submitting-patches.rst
   Documentation/hwmon/submitting-patches.rst
- As mentioned before, a reworked version of the ina2xx.c driver is
   available. I am not inclined to accept a new driver for this chip.
   Even if I were, I would not accept a driver based on deprecated
   hwmon APIs. I would strongly advise to have a look into the reworked
   driver. As mentioned before, it is available in the hwmon-staging branch
   of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git.
- "calculate_avg_power" is (unnecessarily) not a standard attribute and
   therefore unacceptable. Its value (on top of the already averaged power
   attribute) seems questionable. I would understand an attempt to report
   the energy, but I fail to understand the value in reporting yet another
   power average - even more so one that is not well defined in terms of
   number of samples used to determine the average.

Guenter