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

[Guenter Roeck]

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