Re: [PATCH v4 0/2] Add support for Texas Instruments INA4230 power monitor

[Guenter Roeck]

On 3/26/26 12:58, Alexey Charkov wrote:
> TI INA4230 is a 4-channel power monitor with I2C interface, similar in
> operation to INA3221 (3-channel) and INA219 (single-channel) but with
> a different register layout, different alerting mechanism and slightly
> different support for directly reading calculated current/power/energy
> values (pre-multiplied by the device itself and needing only to be scaled
> by the driver depending on its selected LSB unit values).
>
> In this initial implementation, the driver supports reading voltage,
> current, power and energy values, but does not yet support alerts, which
> can be added separately if needed. Also the overflows during hardware
> calculations are not yet handled, nor is the support for the device's
> internal 32-bit energy counter reset.
>
> An example device tree using this binding and driver is available at [1]
> (not currently upstreamed, as the device in question is in engineering
> phase and not yet publicly available)
>
> [1] https://github.com/flipperdevices/flipper-linux-kernel/blob/flipper-devel/arch/arm64/boot/dts/rockchip/rk3576-flipper-one-rev-f0b0c1.dts
>
> Signed-off-by: Alexey Charkov <alchark@xxxxxxxxxxx>

More feedback from Sashiko:

https://sashiko.dev/#/patchset/20260326-ina4230-v4-0-c1e312c09de7%40flipper.net

I can not comment on the devicetree feedback, but the driver
concerns seem real. Please take a look.

Thanks,
Guenter

> ---
> Changes in v4:
> - Aligned the maximum value of ti,maximum-expected-current-microamp property
>    in the binding with the one expected by the driver (Guenter Roeck)
>    "2147A ought to be enough for anybody (c)"
> - Actually requested the optional vs-supply regulator in the driver (Guenter Roeck)
> - Program the ALERT_POL bit according to the value of ti,alert-polarity-active-high
>    even though the alerts themselves are not yet implemented (Guenter Roeck)
> - Added a check for manually disabled channels in the is_enabled() function to
>    avoid reading invalid data from them (Guenter Roeck)
> - Dropped support for the single-shot mode as its operation is not clearly
>    documented in the datasheet and there is no pressing need to support it (Guenter Roeck)
> - NB: AI feedback regarding regmap_noinc_read() producing incorrect byte order on LE
>    hosts is incorrect, as its implementation does a byte-wise read and doesn't care
>    about the regmap value width or endianness flags, so it produces a 4-byte output
>    buffer in the same byte order as the device returns, which is BE in this case
> - NB: AI feedback regarding fail-path pm_runtime_put_noidle() potentially being
>    unbalanced if the probe loop failed early is technically correct but practically
>    irrelevant, as the driver will simply fail to load, and the usage count won't
>    decrease beyond zero anyway. The alternatives are cumbersome for no real benefit
> - Link to v3: https://lore.kernel.org/r/20260310-ina4230-v3-0-06ab3a77c570@xxxxxxxxxxx
>
> Changes in v3:
> - Updated the description of the ti,maximum-expected-current-microamp property
>    in the binding to clarify how it is used, and drop the irrelevant mention of
>    the PMbus (Guenter Roeck)
> - Use div64_u64() instead of do_div() for the final division in the calibration value
>    calculation to avoid overflows in the denominator (Guenter Roeck)
> - Avoid overflow while scaling the voltage values on 32-bit platforms (Guenter Roeck)
> - Use regmap_noinc_read() instead of regmap_raw_read() for reading the energy values
>    to ensure that the regmap / bus driver don't wander off to adjacent registers
>    during the read operation (on INA4230 the whole 32 bits should be read from
>    the same register offset) (Guenter Roeck)
> - Remove redundant call to ina4230_set_calibration() in the current read path,
>    as the calibration value is already set when enabling the channel and restored
>    across PM changes via regcache_sync() (Guenter Roeck)
> - Add missing write_enable() function to make hwmon_in_enable writes work as
>    advertised in is_visible() (Guenter Roeck)
> - Add a check for disabled channels before calling pm_runtime_put_noidle() on them
>    to avoid refcount underflow due to imbalanced get_sync/put_noidle calls (Guenter Roeck)
> - Dropped unused include of linux/debugfs.h
> - Add missing return checks on regmap_write() calls
> - uO -> uOhm in the error message to avoid confusion
> - Move probe-time calibration after enabling runtime PM to avoid it being reverted
>    by the PM sync
> - Link to v2: https://lore.kernel.org/r/20260302-ina4230-v2-0-55b49d19d2ab@xxxxxxxxxxx
>
> Changes in v2:
> - Replace u64/u64 division with do_div() (kernel test robot)
> - Add an example with ti,maximum-expected-current-microamp property in
>    bindings (Krzysztof Kozlowski)
> - Include the newly added binding in MAINTAINERS file (Krzysztof Kozlowski)
> - Use dev_err_probe() where appropriate in the driver (Krzysztof Kozlowski)
> - Switch to devm_regmap_field_bulk_alloc() instead of an open-coded loop
> - Add a bounds check for the calculated calibration value,
>    and a corresponding error message
> - Link to v1: https://lore.kernel.org/r/20260225-ina4230-v1-0-92b1de981d46@xxxxxxxxxxx
>
> ---
> Alexey Charkov (2):
>        dt-bindings: hwmon: Add TI INA4230 4-channel I2C power monitor
>        hwmon: Add support for TI INA4230 power monitor
>
>   .../devicetree/bindings/hwmon/ti,ina4230.yaml      |  134 +++
>   MAINTAINERS                                        |    7 +
>   drivers/hwmon/Kconfig                              |   11 +
>   drivers/hwmon/Makefile                             |    1 +
>   drivers/hwmon/ina4230.c                            | 1032 ++++++++++++++++++++
>   5 files changed, 1185 insertions(+)
> ---
> base-commit: 66ba480978ce390e631e870b740a3406e3eb6b01
> change-id: 20260219-ina4230-74a02409153d
>
> Best regards,