linux-kernel - Re: [PATCH] drivers/hwmon/pmbus: Add support for raa229141 in isl68137

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: <f35193de-a106-42ec-b318-1501793fcfb9@roeck-us.net>
Date: Fri, 26 Sep 2025 01:27:41 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Jeff Lin <jefflin994697@...il.com>, jdelvare@...e.com
Cc: cedricjustine.encarnacion@...log.com, ninad@...ux.ibm.com,
 andriy.shevchenko@...ux.intel.com, johnerasmusmari.geronimo@...log.com,
 Mariel.Tinaco@...log.com, jbrunet@...libre.com, kimseer.paller@...log.com,
 leo.yang.sy0@...il.com, nuno.sa@...log.com, chiang.brian@...entec.com,
 gregkh@...uxfoundation.org, grantpeltier93@...il.com, peterz@...radead.org,
 william@...nnington.com, krzysztof.kozlowski@...aro.org, tzungbi@...nel.org,
 thorsten.blum@...ux.dev, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers/hwmon/pmbus: Add support for raa229141 in
 isl68137

On 9/25/25 18:45, Jeff Lin wrote:
> In chip RAA229141 there exist ISYS pin which can report the current data
> for the device connected to this chip through this pin by routed by Direct
> Memory Access(DMA) command. To read the data in ISYS pin, we have to set
> the DMA address to 0xC5 and then read the DMA data from 0xC7. And then use
> the Direct read format with 10mA per LSB to transfer the data in 0xC7.
> And for ISYS input pin, the DMA address is 0xE0D3 and for ISYS output pin,
> the DMA address is 0xEE42.
> 
> Signed-off-by: Jeff Lin <jefflin994697@...il.com>

As submitted this is a no-go, for several reasons.

The description should describe what is done and why (i.e., here, describe
the added chip), not implementation details. Those are useful as comments
in the code, not as patch description.

Introducing a new sensor class is unacceptable. This is a current (I think),
treat it as such.

Changes in the core together with other changes are unacceptable.

A new virtual command (or commands) would have to be discussed and be generic.

A new Kconfig symbol when adding support for a new chip to an existing driver
is unacceptable. Besides, the new Kconfig symbol would have no effect if
the driver supporting the chip is not enabled, so this is not only unacceptable
but wrong.

Regarding the code itself: Stick with existing coding style. Do not use C++ comments,
declare variables at the beginning of code blocks.

I would suggest to add support for RAA229141 in one patch, then we can discuss
what ISYS_{IN,OUT} actually measures, how it differs from IIN/IOUT, if it indeed
requires new virtual commands and how those command might look like, or if it
can be handled by mapping a existing commands.

The datasheet for RAA229141 is not public, so be prepared to provide a detailed
description.

Guenter