Re: [PATCH v5 2/2] hwmon: Add support for TI INA4230 power monitor

[Guenter Roeck]

On 3/30/26 08:14, Alexey Charkov wrote:
> Add a driver for the TI INA4230, a 4-channel power monitor with I2C
> interface.
>
> The driver supports voltage, current, power and energy measurements, but
> skips the alert functionality in this initial implementation.
>
> Signed-off-by: Alexey Charkov <alchark@xxxxxxxxxxx>

Sashiko report is at

https://sashiko.dev/#/patchset/20260330-ina4230-v5-0-eeb322d95b3a%40flipper.net

Valid concerns, as far as I can see, are:

- There are various overflow issues. Please address, either by making sure that
  the operations can not overflow, or that all parameters such as the shunt
  resistor value or the interval are bound such that an overflow can not occur.
  This includes implicit conversions. For example, the interval passed to
  ina4230_interval_ms_to_conv_time() is int, but the parameter is actually long.
  There is not even a signed check, meaning the resulting interval can be pretty
  much anything.

- power is reported by the chip as unsigned value. Yet, it is converted via type
  cast to int16_t.

- Please add a comment to the energy reading to confirm that regmap_noinc_read()
  performs as expected.

- The definition of ina4230_curr_reg[] is wasteful. There is only an entry for
  hwmon_curr_input. Why specify an unnecessary two-dimensional array ?

- The dummy channel 0 for voltage is not acceptable. Make it 0-based as expected
  by the ABI. Yes, I know, but that is how the ABI was defined.


I can not comment on Saskiko's pm related feedback. PM is and has always been
a mystery to me, so I just assume that it is WAI and that there are no problems.
I do assume that you have tested the code thoroughly across suspend/resume cycles
to make sure that it works as intended.

Thanks,
Guenter