Re: [PATCH v3 1/2] dt-bindings: hwmon: Add TI INA4230 4-channel I2C power monitor

[Guenter Roeck]

On Tue, Mar 10, 2026 at 03:43:46PM +0400, Alexey Charkov wrote:
> Add TI INA4230, which is a 48V 4-channel 16-bit I2C-based
> current/voltage/power/energy monitor with alert function.
> 
> Link: https://www.ti.com/product/INA4230
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxxxxx>
> Signed-off-by: Alexey Charkov <alchark@xxxxxxxxxxx>

AI review feedback inline. As far as I can see all are valid points
which will need to be addressed either in the bindings or in the driver.

Guenter

> ---
>  .../devicetree/bindings/hwmon/ti,ina4230.yaml      | 134 +++++++++++++++++++++
>  MAINTAINERS                                        |   6 +
>  2 files changed, 140 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina4230.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina4230.yaml
> new file mode 100644
> index 000000000000..f33e52a12657
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina4230.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ti,ina4230.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments INA4230 quad-channel power monitors
> +
> +maintainers:
> +  - Alexey Charkov <alchark@xxxxxxxxxxx>
> +
> +description: |
> +  The INA4230 is a 48V quad-channel 16-bit current, voltage, power and energy
> +  monitor with an I2C interface.
> +
> +  Datasheet:
> +    https://www.ti.com/product/INA4230
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ina4230
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    description: Required only if a child node is present.
> +    const: 1
> +
> +  "#size-cells":
> +    description: Required only if a child node is present.
> +    const: 0
> +
> +  vs-supply:
> +    description: phandle to the regulator that provides the VS supply typically
> +      in range from 1.7 V to 5.5 V.
> +
> +  ti,alert-polarity-active-high:
> +    description: Alert pin is asserted based on the value of Alert polarity Bit
> +      of the CONFIG2 register. Default value is 0, for which the alert pin
> +      toggles from high to low during faults. When this property is set, the
> +      corresponding register bit is set to 1, and the alert pin toggles from
> +      low to high during faults.
> +    $ref: /schemas/types.yaml#/definitions/flag

The property is defined here and read by the driver in a subsequent
patch, but the driver never uses the value to set the ALERT_POL bit
in the CONFIG2 register. This renders the property non-functional.

> +
> +patternProperties:
> +  "^input@[0-3]$":
> +    description: The node contains optional child nodes for four channels.
> +      Each child node describes the information of input source. Input channels
> +      default to enabled in the chip. Unless channels are explicitly disabled
> +      in device-tree, input channels will be enabled.
> +    type: object
> +    additionalProperties: false
> +    properties:
> +      reg:
> +        description: Must be 0, 1, 2 or 3, corresponding to the IN1, IN2, IN3
> +          or IN4 ports of the INA4230, respectively.
> +        enum: [ 0, 1, 2, 3 ]
> +
> +      label:
> +        description: name of the input source
> +
> +      shunt-resistor-micro-ohms:
> +        description: shunt resistor value in micro-Ohm
> +
> +      ti,maximum-expected-current-microamp:
> +        description: |
> +          This value indicates the maximum current in microamps that you can
> +          expect to measure with ina4230 in your circuit.
> +
> +          This value will be used to calculate the Current_LSB to maximize the
> +          available precision while ensuring your expected maximum current fits
> +          within the chip's ADC range. It will also enable built-in shunt gain
> +          to increase ADC granularity by a factor of 4 if the provided maximum
> +          current / shunt resistance combination does not produce more than
> +          20.48 mV drop at the shunt.
> +        minimum: 32768
> +        maximum: 4294967295
> +        default: 32768000

The binding allows values up to UINT32_MAX, but the driver rejects
values above INT_MAX. While 2147 Amperes is likely sufficient, the
mismatch should be resolved.

The driver also attempts to read a "ti,single-shot" property which is
missing from this binding file. Given it was present in the INA3221
bindings this driver is based on, its omission appears to be an
oversight.

Furthermore, the "vs-supply" property is defined in the bindings but
completely ignored by the driver (it does not acquire or enable the
regulator).