RE: [PATCH v3 2/2] drivers: hwmon: ltc2991: add driver support
From: Miclaus, Antoniu
Date: Thu Oct 12 2023 - 06:02:55 EST
>
> On Tue, Oct 10, 2023 at 10:44:44AM +0200, Nuno Sá wrote:
> > 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...
> >
> Yes. And there is no change log either.
>
> Guenter
>
For some reason I completely missed your comments on V1 (saw only Nuno's).
And I apologize for that.
Regarding the channel exposure, according to the typical applications of the part
(datasheet page 24/25 for example) a voltage pair (V1-V2) can be used as follows:
V1-V2 differential reading to obtain current based on rsense (Imotor) and in the
same setup V1 is used for singled ended voltage reading (Vmotor). While for the
Temperature readings there are no other readings done on the same pins.
Also looking at all usecases of the part, the differential input can be used either to
obtain current, either to measure temperature.
I will update the channel exposure accordingly in V4 and do the other changes
requested.
Thank you,
Antoniu
> > 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(®vals[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, ®_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, ®_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, ®_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 = <c2991_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://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.6-
> rc5/source/drivers/iio/temperature/ltc2983.c*L1371__;Iw!!A3Ni8CS0y2Y!7K
> woudmzAsHvuDAbhjSNiwM0gCzmfIMYzwFrV2pm7XJTQe7WmW2uioGLsZKt
> PXKk5IKWSMN0yDWCca5NCvWZ$
> >
> > - Nuno Sá
> >