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

[Alexey Charkov]

On Fri, Feb 27, 2026 at 1:49 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On Wed, Feb 25, 2026 at 01:29:12PM +0400, 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>
> > ---
> >  MAINTAINERS             |   6 +
> >  drivers/hwmon/Kconfig   |  11 +
> >  drivers/hwmon/Makefile  |   1 +
> >  drivers/hwmon/ina4230.c | 997 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 1015 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4d879f6a7b51..77f7a416e682 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12511,6 +12511,12 @@ S:   Maintained
> >  F:   Documentation/hwmon/ina233.rst
> >  F:   drivers/hwmon/pmbus/ina233.c
> >
> > +INA4230 HWMON DRIVER
> > +M:   Alexey Charkov <alchark@xxxxxxxxxxx>
> > +L:   linux-hwmon@xxxxxxxxxxxxxxx
> > +S:   Maintained
> > +F:   drivers/hwmon/ina4230.c
>
> List here binding as well, please.

Will do. It does confuse me a bit that we duplicate the maintainers
info both inside the binding files and in here.

> > +
> > +static int ina4230_probe_child_from_dt(struct device *dev,
> > +                                    struct device_node *child,
> > +                                    struct ina4230_data *ina)
> > +{
> > +     struct ina4230_input *input;
> > +     u32 val;
> > +     int ret;
> > +
> > +     ret = of_property_read_u32(child, "reg", &val);
> > +     if (ret) {
> > +             dev_err(dev, "missing reg property of %pOFn\n", child);
> > +             return ret;
> > +     } else if (val > INA4230_CHANNEL4) {
> > +             dev_err(dev, "invalid reg %d of %pOFn\n", val, child);
>
> All these are probe, so return dev_err_probe

Good point, thanks. Will adjust in v2.

> > +             return -EINVAL;
> > +     }
> > +
>
> ...
>
> > +     ina->regmap = devm_regmap_init_i2c(client, &ina4230_regmap_config);
> > +     if (IS_ERR(ina->regmap)) {
> > +             dev_err(dev, "Unable to allocate register map\n");
> > +             return PTR_ERR(ina->regmap);
> > +     }
> > +
> > +     for (i = 0; i < F_MAX_FIELDS; i++) {
> > +             ina->fields[i] = devm_regmap_field_alloc(dev,
> > +                                                      ina->regmap,
> > +                                                      ina4230_reg_fields[i]);
> > +             if (IS_ERR(ina->fields[i])) {
> > +                     dev_err(dev, "Unable to allocate regmap fields\n");
> > +                     return PTR_ERR(ina->fields[i]);
>
> Syntax is return dev_err_probe, but allocations should not have printks.
> It is not possible to get there any other error code.

Alright, will drop the error message and return the errno directly,
thanks. -ENOMEM should be pretty self-explanatory.

It probably also makes sense to replace the open-coded loop with a
call to devm_regmap_field_bulk_alloc while we're here.

> > +             }
> > +     }
> > +
> > +     for (i = 0; i < INA4230_NUM_CHANNELS; i++) {
> > +             ina->inputs[i].shunt_resistor = INA4230_RSHUNT_DEFAULT;
> > +             /* Default for 1mA LSB current measurements */
> > +             ina->inputs[i].max_expected_current = 32768000;
> > +     }
> > +
> > +     ret = ina4230_probe_from_dt(dev, ina);
> > +     if (ret) {
> > +             dev_err(dev, "Unable to probe from device tree\n");
> > +             return ret;
>
> return dev_err_probe

Ack

> > +     }
> > +
> > +     /* The driver will be reset, so use reset value */
> > +     ina->reg_config1 = INA4230_CONFIG_DEFAULT;
> > +     ina->reg_config2 = 0;
> > +
> > +     if (ina->single_shot)
> > +             FIELD_MODIFY(INA4230_CONFIG1_MODE_MASK, &ina->reg_config1,
> > +                          INA4230_MODE_BUS_SHUNT_SINGLE);
> > +
> > +     /* Disable channels if their inputs are disconnected */
> > +     for (i = 0; i < INA4230_NUM_CHANNELS; i++) {
> > +             if (ina->inputs[i].disconnected)
> > +                     ina->reg_config1 &= ~INA4230_CONFIG_CHx_EN(i);
> > +     }
> > +
> > +     /* Set calibration values */
> > +     for (i = 0; i < INA4230_NUM_CHANNELS; i++) {
> > +             if (!ina->inputs[i].disconnected)
> > +                     ina4230_set_calibration(ina, i);
> > +     }
> > +
> > +     ina->pm_dev = dev;
> > +     dev_set_drvdata(dev, ina);
> > +
> > +     /* Enable PM runtime -- status is suspended by default */
> > +     pm_runtime_enable(ina->pm_dev);
> > +
> > +     /* Initialize (resume) the device */
> > +     for (i = 0; i < INA4230_NUM_CHANNELS; i++) {
> > +             if (ina->inputs[i].disconnected)
> > +                     continue;
> > +             /* Match the refcount with number of enabled channels */
> > +             ret = pm_runtime_get_sync(ina->pm_dev);
> > +             if (ret < 0)
> > +                     goto fail;
> > +     }
> > +
> > +     hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
> > +                                                      &ina4230_chip_info,
> > +                                                      ina4230_groups);
> > +     if (IS_ERR(hwmon_dev)) {
> > +             dev_err(dev, "Unable to register hwmon device\n");
> > +             ret = PTR_ERR(hwmon_dev);
>
> just ret = dev_err_probe

Ack

Thanks for your review Krzysztof!

Best regards,
Alexey