Re: [PATCH v3 2/2] drivers: hwmon: ltc2991: add driver support

From: Nuno Sá
Date: Tue Oct 10 2023 - 04:42:05 EST


Hi Antoniu,

A few more comments from me. But essentially, I think you should revisit V1 and
Guenter's comments. I'm fairly sure you are still missing his main points...

On Tue, 2023-10-03 at 11:00 +0300, Antoniu Miclaus wrote:
> Add support for LTC2991 Octal I2C Voltage, Current, and Temperature
> Monitor.
>
> The LTC2991 is used to monitor system temperatures, voltages and
> currents. Through the I2C serial interface, the eight monitors can
> individually measure supply voltages and can be paired for
> differential measurements of current sense resistors or temperature
> sensing transistors. Additional measurements include internal
> temperature and internal VCC.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> ---
>  Documentation/hwmon/index.rst   |   1 +
>  Documentation/hwmon/ltc2991.rst |  43 +++
>  MAINTAINERS                     |   8 +
>  drivers/hwmon/Kconfig           |  11 +
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/ltc2991.c         | 479 ++++++++++++++++++++++++++++++++
>  6 files changed, 543 insertions(+)
>  create mode 100644 Documentation/hwmon/ltc2991.rst
>  create mode 100644 drivers/hwmon/ltc2991.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 88dadea85cfc..0ec96abe3f7d 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -121,6 +121,7 @@ Hardware Monitoring Kernel Drivers
>     ltc2947
>     ltc2978
>     ltc2990
> +   ltc2991
>     ltc3815
>     ltc4151
>     ltc4215
> diff --git a/Documentation/hwmon/ltc2991.rst b/Documentation/hwmon/ltc2991.rst
> new file mode 100644
> index 000000000000..9ab29dd85012
> --- /dev/null
> +++ b/Documentation/hwmon/ltc2991.rst
> @@ -0,0 +1,43 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver ltc2991
> +=====================
> +
> +Supported chips:
> +
> +  * Analog Devices LTC2991
> +
> +    Prefix: 'ltc2991'
> +
> +    Addresses scanned: I2C 0x48 - 0x4f
> +
> +    Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/2991ff.pdf
> +
> +Authors:
> +
> +  - Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> +
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for Analog Devices LTC2991 Octal I2C
> +Voltage, Current and Temperature Monitor.
> +
> +The LTC2991 is used to monitor system temperatures, voltages and currents.
> +Through the I2C serial interface, the eight monitors can individually measure
> +supply voltages and can be paired for differential measurements of current
> sense
> +resistors or temperature sensing transistors. Additional measurements include
> +internal temperatureand internal VCC.
> +
> +
> +sysfs-Interface
> +-------------
> +
> +The following attributes are supported. Limits are read-only.
> +
> +=============== =================
> +inX_input:      voltage input
> +currX_input:    current input
> +tempX_input:    temperature input
> +=============== =================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..98dd8a8e1f84 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12451,6 +12451,14 @@ F:     drivers/hwmon/ltc2947-i2c.c
>  F:     drivers/hwmon/ltc2947-spi.c
>  F:     drivers/hwmon/ltc2947.h
>  
> +LTC2991 HARDWARE MONITOR DRIVER
> +M:     Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> +L:     linux-hwmon@xxxxxxxxxxxxxxx
> +S:     Supported
> +W:     https://ez.analog.com/linux-software-drivers
> +F:     Documentation/devicetree/bindings/hwmon/adi,ltc2991.yaml
> +F:     drivers/hwmon/ltc2991.c
> +
>  LTC2983 IIO TEMPERATURE DRIVER
>  M:     Nuno Sá <nuno.sa@xxxxxxxxxx>
>  L:     linux-iio@xxxxxxxxxxxxxxx
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index ec38c8892158..818a67328fcd 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -932,6 +932,17 @@ config SENSORS_LTC2990
>           This driver can also be built as a module. If so, the module will
>           be called ltc2990.
>  
> +config SENSORS_LTC2991
> +       tristate "Analog Devices LTC2991"
> +       depends on I2C
> +       help
> +         If you say yes here you get support for Analog Devices LTC2991
> +         Octal I2C Voltage, Current, and Temperature Monitor. The LTC2991
> +         supports a combination of voltage, current and temperature
> monitoring.
> +
> +         This driver can also be built as a module. If so, the module will
> +         be called ltc2991.
> +
>  config SENSORS_LTC2992
>         tristate "Linear Technology LTC2992"
>         depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 4ac9452b5430..f324d057535a 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -127,6 +127,7 @@ obj-$(CONFIG_SENSORS_LTC2947)       += ltc2947-core.o
>  obj-$(CONFIG_SENSORS_LTC2947_I2C) += ltc2947-i2c.o
>  obj-$(CONFIG_SENSORS_LTC2947_SPI) += ltc2947-spi.o
>  obj-$(CONFIG_SENSORS_LTC2990)  += ltc2990.o
> +obj-$(CONFIG_SENSORS_LTC2991)  += ltc2991.o
>  obj-$(CONFIG_SENSORS_LTC2992)  += ltc2992.o
>  obj-$(CONFIG_SENSORS_LTC4151)  += ltc4151.o
>  obj-$(CONFIG_SENSORS_LTC4215)  += ltc4215.o
> diff --git a/drivers/hwmon/ltc2991.c b/drivers/hwmon/ltc2991.c
> new file mode 100644
> index 000000000000..b5333c25cb31
> --- /dev/null
> +++ b/drivers/hwmon/ltc2991.c
> @@ -0,0 +1,479 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Analog Devices, Inc.
> + * Author: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define LTC2991_STATUS_LOW             0x00
> +#define LTC2991_CH_EN_TRIGGER          0x01
> +#define LTC2991_V1_V4_CTRL             0x06
> +#define LTC2991_V5_V8_CTRL             0x07
> +#define LTC2991_PWM_TH_LSB_T_INT       0x08
> +#define LTC2991_PWM_TH_MSB             0x09
> +#define LTC2991_CHANNEL_V_MSB(x)       (0x0A + ((x) * 2))
> +#define LTC2991_CHANNEL_T_MSB(x)       (0x0A + ((x) * 4))
> +#define LTC2991_CHANNEL_C_MSB(x)       (0x0C + ((x) * 4))
> +#define LTC2991_T_INT_MSB              0x1A
> +#define LTC2991_VCC_MSB                        0x1C
> +
> +#define LTC2991_V7_V8_EN               BIT(7)
> +#define LTC2991_V5_V6_EN               BIT(6)
> +#define LTC2991_V3_V4_EN               BIT(5)
> +#define LTC2991_V1_V2_EN               BIT(4)
> +#define LTC2991_T_INT_VCC_EN           BIT(3)
> +
> +#define LTC2991_V3_V4_FILT_EN          BIT(7)
> +#define LTC2991_V3_V4_TEMP_EN          BIT(5)
> +#define LTC2991_V3_V4_DIFF_EN          BIT(4)
> +#define LTC2991_V1_V2_FILT_EN          BIT(3)
> +#define LTC2991_V1_V2_TEMP_EN          BIT(1)
> +#define LTC2991_V1_V2_DIFF_EN          BIT(0)
> +
> +#define LTC2991_V7_V8_FILT_EN          BIT(7)
> +#define LTC2991_V7_V8_TEMP_EN          BIT(5)
> +#define LTC2991_V7_V8_DIFF_EN          BIT(4)
> +#define LTC2991_V5_V6_FILT_EN          BIT(7)
> +#define LTC2991_V5_V6_TEMP_EN          BIT(5)
> +#define LTC2991_V5_V6_DIFF_EN          BIT(4)
> +
> +#define LTC2991_REPEAT_ACQ_EN          BIT(4)
> +#define LTC2991_T_INT_FILT_EN          BIT(3)
> +
> +#define LTC2991_MAX_CHANNEL            4
> +#define LTC2991_T_INT_CH_NR            4
> +#define LTC2991_VCC_CH_NR              0
> +
> +static const char *const label_voltages[] = {
> +       "vcc",
> +       "voltage1",
> +       "voltage2",
> +       "voltage3",
> +       "voltage4",
> +       "voltage5",
> +       "voltage6",
> +       "voltage7",
> +       "voltage8"
> +};
> +
> +static const char *const label_temp[] = {
> +       "t1",
> +       "t2",
> +       "t3",
> +       "t4",
> +       "t_int"
> +};
> +
> +static const char *const label_curr[] = {
> +       "v1-v2",
> +       "v3-v4",
> +       "v5-v6",
> +       "v7-v8"
> +};
> +
> +struct ltc2991_state {
> +       struct i2c_client       *client;
> +       struct regmap           *regmap;
> +       u32                     r_sense_uohm[LTC2991_MAX_CHANNEL];
> +       bool                    temp_en[LTC2991_MAX_CHANNEL];
> +};
> +
> +static int ltc2991_read_reg(struct ltc2991_state *st, u8 addr, u8 reg_len,
> +                           int *val)
> +{
> +       u8 regvals[2];
> +       int ret;
> +
> +       if (reg_len > 2 || !reg_len)
> +               return -EINVAL;
> +

IMO, this is too much checking... It's an internal API, just make sure to
properly call it ;).

> +       ret = regmap_bulk_read(st->regmap, addr, regvals, reg_len);
> +       if (ret)
> +               return ret;
> +
> +       if (reg_len == 2)
> +               *val = get_unaligned_be16(&regvals[0]);
> +       else
> +               *val = regvals[0];

not what I had in mind. Having unaligned access is overkill when you can go with
typical __be16 + __be16_to_cpu(). If I'm not missing anything, you just have to
shift out the LSB in case of 1byte registers. Alternatively, before the bulk()
call you can:

if (reg_len < 2)
return regmap_read();

and then just assume 2 byte reads...

> +       return 0;
> +}
> +
> +static int ltc2991_get_voltage(struct ltc2991_state *st, u32 reg, long *val)
> +{
> +       int reg_val, ret, offset = 0;
> +
> +       ret = ltc2991_read_reg(st, reg, 2, &reg_val);
> +       if (ret)
> +               return ret;
> +
> +       if (reg == LTC2991_VCC_MSB)
> +               /* Vcc 2.5V offset */
> +               offset = 2500;
> +
> +       /* Vx, 305.18uV/LSB */
> +       *val = DIV_ROUND_CLOSEST(sign_extend32(reg_val, 14) * 30518,
> +                                1000 * 100) + offset;
> +
> +       return 0;
> +}
> +
> +static int ltc2991_read_in(struct device *dev, u32 attr, int channel, long
> *val)
> +{
> +       struct ltc2991_state *st = dev_get_drvdata(dev);
> +       u32 reg;
> +
> +       switch (attr) {
> +       case hwmon_in_input:
> +               if (channel == LTC2991_VCC_CH_NR)
> +                       reg = LTC2991_VCC_MSB;
> +               else
> +                       reg = LTC2991_CHANNEL_V_MSB(channel - 1);
> +               break;
> +       default:
> +               return -EOPNOTSUPP;
> +       }
> +
> +       return ltc2991_get_voltage(st, reg, val);
> +}
> +
> +static int ltc2991_get_curr(struct ltc2991_state *st, u32 reg, int channel,
> +                           long *val)
> +{
> +       int reg_val, ret;
> +
> +       ret = ltc2991_read_reg(st, reg, 2, &reg_val);
> +       if (ret)
> +               return ret;
> +
> +       /* Vx-Vy, 19.075uV/LSB */
> +       *val = DIV_ROUND_CLOSEST(sign_extend32(reg_val, 14) * 19075, 1000)
> +                                / (st->r_sense_uohm[channel] / 1000);
> +
> +       return 0;
> +}
> +
> +static int ltc2991_read_curr(struct device *dev, u32 attr, int channel,
> +                            long *val)
> +{
> +       struct ltc2991_state *st = dev_get_drvdata(dev);
> +       u32 reg;
> +
> +       switch (attr) {
> +       case hwmon_curr_input:
> +               reg = LTC2991_CHANNEL_C_MSB(channel);
> +               return ltc2991_get_curr(st, reg, channel, val);
> +       default:
> +               return -EOPNOTSUPP;
> +       }
> +}
> +
> +static int ltc2991_get_temp(struct ltc2991_state *st, u32 reg, int channel,
> +                           long *val)
> +{
> +       int reg_val, ret;
> +
> +       ret = ltc2991_read_reg(st, reg, 2, &reg_val);
> +       if (ret)
> +               return ret;
> +
> +       /* Temp LSB = 0.0625 Degrees */
> +       *val = DIV_ROUND_CLOSEST(sign_extend32(reg_val, 12) * 1000, 16);
> +
> +       return 0;
> +}
> +
> +static int ltc2991_read_temp(struct device *dev, u32 attr, int channel,
> +                            long *val)
> +{
> +       struct ltc2991_state *st = dev_get_drvdata(dev);
> +       u32 reg;
> +
> +       switch (attr) {
> +       case hwmon_temp_input:
> +               if (channel == LTC2991_T_INT_CH_NR)
> +                       reg = LTC2991_T_INT_MSB;
> +               else
> +                       reg = LTC2991_CHANNEL_T_MSB(channel);
> +
> +               return ltc2991_get_temp(st, reg, channel, val);
> +       default:
> +               return -EOPNOTSUPP;
> +       }
> +}
> +
> +static int ltc2991_read(struct device *dev, enum hwmon_sensor_types type,
> +                       u32 attr, int channel, long *val)
> +{
> +       switch (type) {
> +       case hwmon_in:
> +               return ltc2991_read_in(dev, attr, channel, val);
> +       case hwmon_curr:
> +               return ltc2991_read_curr(dev, attr, channel, val);
> +       case hwmon_temp:
> +               return ltc2991_read_temp(dev, attr, channel, val);
> +       default:
> +               return -EOPNOTSUPP;
> +       }
> +}
> +
> +static umode_t ltc2991_is_visible(const void *data,
> +                                 enum hwmon_sensor_types type, u32 attr,
> +                                 int channel)
> +{
> +       const struct ltc2991_state *st = data;
> +
> +       switch (type) {
> +       case hwmon_in:
> +               switch (attr) {
> +               case hwmon_in_input:
> +               case hwmon_in_label:
> +                       return 0444;
> +               }
> +               break;
> +       case hwmon_curr:
> +               switch (attr) {
> +               case hwmon_curr_input:
> +               case hwmon_curr_label:
> +                       if (st->r_sense_uohm[channel])
> +                               return 0444;
> +                       break;
> +               }

This is what I was speaking about... You should go read again v1 an try to
understand the points made or continue the discussion.

I didn't looked at the datasheet but what I understood from Guenter is that each
channel can only be a specific type of sensor and you should be explicit about
that (not using rsense as the deciding factor). Thus, your bindings should
likely be refactored so you have a property that explicitly "defines" a channel
(not forgetting to handle things like differential channels and overlaps etc...)
and then you need to reflect the ABI according to what sensors OF/ACPI have
defined.

This looks a bit like the ltc2983 [1] I upstreamed where most of the fun was in
defining the channels (and maybe that one was actually an hwmon driver but what
did I know back then :))

Also important is that in absence of OF/ACPI, you should read whatever
configuration is the device holding so, once again, you export the correct ABI.
But in this case, I'm not really sure what would be a sane (if there's one) for
rsense in case of a current sensor.

Hopefully I got it right...

> +               break;
> +       case hwmon_temp:
> +               switch (attr) {
> +               case hwmon_temp_input:
> +               case hwmon_temp_label:
> +                       if (st->temp_en[channel] ||
> +                           channel == LTC2991_T_INT_CH_NR)
> +                               return 0444;
> +                       break;
> +               }
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ltc2991_read_string(struct device *dev, enum hwmon_sensor_types
> type,
> +                              u32 attr, int channel, const char **str)
> +{
> +       switch (type) {
> +       case hwmon_temp:
> +               *str = label_temp[channel];
> +               break;
> +       case hwmon_curr:
> +               *str = label_curr[channel];
> +               break;
> +       case hwmon_in:
> +               *str = label_voltages[channel];
> +               break;
> +       default:
> +               return -EOPNOTSUPP;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct hwmon_ops ltc2991_hwmon_ops = {
> +       .is_visible = ltc2991_is_visible,
> +       .read = ltc2991_read,
> +       .read_string = ltc2991_read_string,
> +};
> +
> +static const struct hwmon_channel_info *ltc2991_info[] = {
> +       HWMON_CHANNEL_INFO(temp,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL
> +                          ),
> +       HWMON_CHANNEL_INFO(curr,
> +                          HWMON_C_INPUT | HWMON_C_LABEL,
> +                          HWMON_C_INPUT | HWMON_C_LABEL,
> +                          HWMON_C_INPUT | HWMON_C_LABEL,
> +                          HWMON_C_INPUT | HWMON_C_LABEL
> +                          ),
> +       HWMON_CHANNEL_INFO(in,
> +                          HWMON_I_INPUT | HWMON_I_LABEL,
> +                          HWMON_I_INPUT | HWMON_I_LABEL,
> +                          HWMON_I_INPUT | HWMON_I_LABEL,
> +                          HWMON_I_INPUT | HWMON_I_LABEL,
> +                          HWMON_I_INPUT | HWMON_I_LABEL,
> +                          HWMON_I_INPUT | HWMON_I_LABEL,
> +                          HWMON_I_INPUT | HWMON_I_LABEL,
> +                          HWMON_I_INPUT | HWMON_I_LABEL,
> +                          HWMON_I_INPUT | HWMON_I_LABEL
> +                          ),
> +       NULL
> +};
> +
> +static const struct hwmon_chip_info ltc2991_chip_info = {
> +       .ops = &ltc2991_hwmon_ops,
> +       .info = ltc2991_info,
> +};
> +
> +static const struct regmap_config ltc2991_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .max_register = 0x1D,
> +};
> +
> +static int ltc2991_init(struct ltc2991_state *st)
> +{
> +       struct fwnode_handle *child;
> +       int ret;
> +       u32 val, addr;
> +       u8 v5_v8_reg_data = 0, v1_v4_reg_data = 0;
> +
> +       ret = devm_regulator_get_enable(&st->client->dev, "vcc");
> +       if (ret)
> +               return dev_err_probe(&st->client->dev, ret,
> +                                    "failed to enable regulator\n");
> +
> +       device_for_each_child_node(&st->client->dev, child) {
> +               ret = fwnode_property_read_u32(child, "reg", &addr);
> +               if (ret < 0) {
> +                       fwnode_handle_put(child);
> +                       return ret;
> +               }
> +
> +               if (addr > 3) {
> +                       fwnode_handle_put(child);
> +                       return -EINVAL;
> +               }
> +
> +               ret = fwnode_property_read_u32(child, "shunt-resistor-micro-
> ohms", &val);
> +               if (!ret) {
> +                       st->r_sense_uohm[addr] = val;

This still allows 0 as a valid value which would lead to a divide by 0
exception...

[1]: https://elixir.bootlin.com/linux/v6.6-rc5/source/drivers/iio/temperature/ltc2983.c#L1371

- Nuno Sá