Re: [PATCH 2/2] drivers: hwmon: ltc2991: add driver support
From: Nuno Sá
Date: Thu Sep 28 2023 - 03:37:29 EST
Hi Antoniu,
some comments from me...
On Tue, 2023-09-26 at 17:05 +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 I 2C serial interface, theeight 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 | 490 ++++++++++++++++++++++++++++++++
> 6 files changed, 554 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..51a60ca8c24e
> --- /dev/null
> +++ b/drivers/hwmon/ltc2991.c
> @@ -0,0 +1,490 @@
> +// 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>
> +
> +#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_mohm[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;
> + int i;
> +
> + ret = regmap_bulk_read(st->regmap, addr, regvals, reg_len);
> + if (ret)
> + return ret;
> +
> + *val = 0;
> + for (i = 0; i < reg_len; i++)
> + *val |= regvals[reg_len - i - 1] << (i * 8);
Hmm this does not look good :)... You should use proper __be annotations and API...
> +
> + 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_mohm[channel];
> +
> + 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);
you could just return...
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ltc2991_get_curr(st, reg, channel, val);
> +}
> +
> +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);
nit: you could just return in each of the statements...
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return ltc2991_get_temp(st, reg, channel, val);
> +}
> +
> +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_mohm[channel])
> + return 0444;
Is this is a real usecase? I mean, not having a rsense? If not, I would just make the
property mandatory...
> + break;
> + }
> + 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 *fwnode;
> + 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");
> +
> + fwnode = dev_fwnode(&st->client->dev);
> +
> + fwnode_for_each_available_child_node(fwnode, child) {
device_for_each_child_node()
> + 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-mili-ohms",
> &val);
> + if (!ret) {
> + st->r_sense_mohm[addr] = val;
> + switch (addr) {
> + case 0:
> + v1_v4_reg_data |= LTC2991_V1_V2_DIFF_EN;
> + break;
> + case 1:
> + v1_v4_reg_data |= LTC2991_V3_V4_DIFF_EN;
> + break;
> + case 2:
> + v5_v8_reg_data |= LTC2991_V5_V6_DIFF_EN;
> + break;
> + case 3:
> + v5_v8_reg_data |= LTC2991_V7_V8_DIFF_EN;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + ret = fwnode_property_read_bool(child, "temperature-enable");
> + if (ret) {
> + st->temp_en[addr] = ret;
> + switch (addr) {
> + case 0:
> + v1_v4_reg_data |= LTC2991_V1_V2_TEMP_EN;
> + break;
> + case 1:
> + v1_v4_reg_data |= LTC2991_V3_V4_TEMP_EN;
> + break;
> + case 2:
> + v5_v8_reg_data |= LTC2991_V5_V6_TEMP_EN;
> + break;
> + case 3:
> + v5_v8_reg_data |= LTC2991_V7_V8_TEMP_EN;
> + break;
> + default:
> + break;
> + }
> + }
> + }
> +
> + /* Setup V5-V8 Control register */
> + ret = regmap_write(st->regmap, LTC2991_V5_V8_CTRL, v5_v8_reg_data);
> + if (ret)
> + return dev_err_probe(&st->client->dev, ret,
> + "Error: Failed to set V5-V8 CTRL reg.\n");
> +
> + /* Setup V1-V4 Control register */
> + ret = regmap_write(st->regmap, LTC2991_V1_V4_CTRL, v1_v4_reg_data);
> + if (ret)
> + return dev_err_probe(&st->client->dev, ret,
> + "Error: Failed to set V1-V4 CTRL reg.\n");
> +
> + /* Setup continuous mode */
> + ret = regmap_write(st->regmap, LTC2991_PWM_TH_LSB_T_INT,
> + LTC2991_REPEAT_ACQ_EN);
> + if (ret)
> + return dev_err_probe(&st->client->dev, ret,
> + "Error: Failed to set contiuous mode.\n");
> +
> + /* Enable all channels and trigger conversions */
> + ret = regmap_write(st->regmap, LTC2991_CH_EN_TRIGGER,
> + LTC2991_V7_V8_EN | LTC2991_V5_V6_EN |
> + LTC2991_V3_V4_EN | LTC2991_V1_V2_EN |
> + LTC2991_T_INT_VCC_EN);
Not sure on the added value of all the comments before doing a configuration...
> + if (ret)
> + return dev_err_probe(&st->client->dev, ret,
> + "Error: Failed to enable conversions.\n");
> +
I would just return regmap_write()...
> + return 0;
> +}
> +
> +static int ltc2991_i2c_probe(struct i2c_client *client)
> +{
> + int ret;
> + struct device *hwmon_dev;
> + struct ltc2991_state *st;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> + return -EOPNOTSUPP;
> +
Is the above really needed? Isn't the device also compatible with plain i2c?
> + st = devm_kzalloc(&client->dev, sizeof(*st), GFP_KERNEL);
> + if (!st)
> + return -ENOMEM;
> +
> + st->client = client;
> + st->regmap = devm_regmap_init_i2c(client, <c2991_regmap_config);
> + if (IS_ERR(st->regmap))
> + return PTR_ERR(st->regmap);
> +
> + ret = ltc2991_init(st);
> + if (ret)
> + return ret;
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(&client->dev,
> + client->name, st,
> + <c2991_chip_info,
> + NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id ltc2991_of_match[] = {
> + { .compatible = "adi,ltc2991" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ltc2991_of_match);
> +
> +static const struct i2c_device_id ltc2991_i2c_id[] = {
> + { "ltc2991", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2991_i2c_id);
> +
> +static struct i2c_driver ltc2991_i2c_driver = {
> + .class = I2C_CLASS_HWMON,
curious: why or when is the above needed?
- Nuno Sá