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 > 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://elixir.bootlin.com/linux/v6.6-rc5/source/drivers/iio/temperature/ltc2983.c#L1371 > > - Nuno Sá >
