Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs
Guenter Roeck <[email protected]>
| Newsgroups | org.kernel.vger.linux-i2c,org.kernel.vger.linux-hwmon,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
On Wed, Aug 19, 2026 at 11:37:45PM +1000, Stephen Horvath wrote: > Add support for sensors present on PMIC5000 (JEDEC JESD301) compliant > power management ICs. These chips are commonly found on DDR5 memory > modules. > > Tested against PMIC5100 chips, but only features present in the > PMIC5000 (JESD301-1) specification have been implemented. > > Signed-off-by: Stephen Horvath <[email protected]> > --- > Hi maintainers, I wrote this over the past couple days and I'm looking > for feedback. > > Here's a few concerns I have: > - I'm not too experienced with kernel development. > - I don't think there's a good way to auto-detect PMIC5000 chips > standalone, would it be reasonable to instantiate it from > spd5118? > - I'm unsure what to do with temperatures below 85°C, I'd rather not > return 85 as that would make it seem hotter than it is, but > returning -EOPNOTSUPP seems wrong. > - Toggling between current and power seems like it's a bit of a hack, > is that okay, or should I just pick one and drop the other? > - Does this belong in HWMON, or should it be part of the regulator > subsystem? It depends on its functionality. Is this just for monitoring, or are the voltages controllable ? > --- > .../devicetree/bindings/trivial-devices.yaml | 2 + > Documentation/hwmon/index.rst | 1 + > Documentation/hwmon/pmic5000.rst | 103 +++ > drivers/hwmon/Kconfig | 12 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/pmic5000.c | 896 +++++++++++++++++++++ > 6 files changed, 1015 insertions(+) > > > --- > base-commit: 75f2c0b3690702c90863c2e138cb5520670845ea > change-id: 20260819-pmic5000-3840f8a9ade1 > > Best regards, > -- > Stephen Horvath <[email protected]> > > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml > index 2de8eb09cb7d..86e76be7d41b 100644 > --- a/Documentation/devicetree/bindings/trivial-devices.yaml > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml > @@ -194,6 +194,8 @@ properties: > - isil,isl29030 > # Intersil ISL76682 Ambient Light Sensor > - isil,isl76682 > + # JEDEC JESD301 (PMIC5000) Power Management IC > + - jedec,pmic5000 > # JEDEC JESD300 (SPD5118) Hub and Serial Presence Detect > - jedec,spd5118 > # Kandou KB9002 PCIe 5.0 retimer > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > index d979e6d6e9f2..4bb518d1927c 100644 > --- a/Documentation/hwmon/index.rst > +++ b/Documentation/hwmon/index.rst > @@ -226,6 +226,7 @@ Hardware Monitoring Kernel Drivers > peci-cputemp > peci-dimmtemp > pmbus > + pmic5000 > powerz > powr1220 > prom21-xhci > diff --git a/Documentation/hwmon/pmic5000.rst b/Documentation/hwmon/pmic5000.rst > new file mode 100644 > index 000000000000..e7d864088ba5 > --- /dev/null > +++ b/Documentation/hwmon/pmic5000.rst > @@ -0,0 +1,103 @@ > +.. SPDX-License-Identifier: GPL-2.0-or-later > + > +Kernel driver pmic5000 > +===================== > + > +Supported chips: > + > + * PMIC5000 (JEDEC JESD301) compliant power management chips > + > + JEDEC standard download: > + https://www.jedec.org/standards-documents/docs/jesd301-1a03 > + (account required) > + > + > + Prefix: 'pmic5000' > + > +Author: > + Stephen Horvath <[email protected]> > + > + > +Description > +----------- > + > +This driver implements support for PMIC5000 (JEDEC JESD301) compliant power > +management chips, which are used on many DDR5 memory modules. > + > +The driver does not auto-detect PMIC5000 compliant chips, and must be > +instantiated manually or via device tree. The address of these chips are > +usually between (inclusive) 0x48 and 0x4F. > + > + > +Hardware monitoring sysfs entries > +--------------------------------- > + > +======================= ================================== > +in0_enable Whether SWA is enabled (RO) Does that enable the voltage or its monitoriong ? The enable attribute is only to enable monitoring, not to enable the voltage. If this is a controllable voltage, the driver should be a regulator driver, as Sashiko suggested. > +in0_input SWA Voltage (RO) > +in0_min Minimum SWA Voltage (RO) > +in0_max Maximum SWA Voltage (RO) > +in0_min_alarm SWA Voltage low alarm > +in0_max_alarm SWA Voltage high alarm > + > +in1_enable Whether SWB is enabled (RO) > +in1_input SWB Voltage (RO) > +in1_min Minimum SWB Voltage (RO) > +in1_max Maximum SWB Voltage (RO) > +in1_min_alarm SWB Voltage low alarm > +in1_max_alarm SWB Voltage high alarm > + > +in2_enable Whether SWC is enabled (RO) > +in2_input SWC Voltage (RO) > +in2_min Minimum SWC Voltage (RO) > +in2_max Maximum SWC Voltage (RO) > +in2_min_alarm SWC Voltage low alarm > +in2_max_alarm SWC Voltage high alarm > + > +in3_enable Whether SWD is enabled (RO) > +in3_input SWD Voltage (RO) > +in3_min Minimum SWD Voltage (RO) > +in3_max Maximum SWD Voltage (RO) > +in3_min_alarm SWD Voltage low alarm > +in3_max_alarm SWD Voltage high alarm > + > +in5_input VIN_Bulk Voltage (RO) > +in5_max Maximum VIN_Bulk Voltage (RO) > +in5_max_alarm VIN_Bulk Voltage high alarm > + > +in6_input VIN_Mgmt Voltage (RO) > +in6_max Maximum VIN_Mgmt Voltage (RO) > +in6_max_alarm VIN_Mgmt Voltage high alarm > + > +in7_input VBias Voltage (RO) > +in8_input VOUT_1.8V Voltage (RO) > +in9_input VOUT_1.0V Voltage (RO) > + > +temp1_input Temperature (RO) > +temp1_max Maximum temperature (RO) > +temp1_max_alarm Temperature high alarm > + > +power1_input SWA Wattage (RO) > +power2_input SWB Wattage (RO) > +power3_input SWC Wattage (RO) > +power4_input SWD Wattage (RO) > + > +curr1_input SWA Current (RO) > +curr1_max Maximum SWA Current (RO) > +curr1_max_alarm SWA Current high alarm > + > +curr2_input SWB Current (RO) > +curr2_max Maximum SWB Current (RO) > +curr2_max_alarm SWB Current high alarm > + > +curr3_input SWC Current (RO) > +curr3_max Maximum SWC Current (RO) > +curr3_max_alarm SWC Current high alarm > + > +curr4_input SWD Current (RO) > +curr4_max Maximum SWD Current (RO) > +curr4_max_alarm SWD Current high alarm > + > +update_interval Update interval in milliseconds (RW), Allowed values: 1, 2, 4, 8 > + > +======================= ================================== > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 81a9a1d40eec..5ed5f9499c43 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -950,6 +950,18 @@ config SENSORS_JC42 > This driver can also be built as a module. If so, the module > will be called jc42. > > +config SENSORS_PMIC5000 > + tristate "PMIC5000 Compliant Sensors" > + depends on I2C > + select REGMAP_I2C > + help > + If you say yes here you get support for PMIC5000 (JEDEC JESD301) > + compliant sensors. Such sensors are found on DDR5 memory > + modules. > + > + This driver can also be built as a module. If so, the module > + will be called pmic5000. > + > config SENSORS_POLARFIRE_SOC_TVS > tristate "PolarFire SoC (MPFS) temperature and voltage sensor" > depends on POLARFIRE_SOC_SYSCONS || COMPILE_TEST > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 0cad7e21634c..ac519123796f 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -199,6 +199,7 @@ obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o > obj-$(CONFIG_SENSORS_PC87360) += pc87360.o > obj-$(CONFIG_SENSORS_PC87427) += pc87427.o > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o > +obj-$(CONFIG_SENSORS_PMIC5000) += pmic5000.o > obj-$(CONFIG_SENSORS_POLARFIRE_SOC_TVS) += tvs-mpfs.o > obj-$(CONFIG_SENSORS_POWERZ) += powerz.o > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o > diff --git a/drivers/hwmon/pmic5000.c b/drivers/hwmon/pmic5000.c > new file mode 100644 > index 000000000000..d3f3126dd9c8 > --- /dev/null > +++ b/drivers/hwmon/pmic5000.c > @@ -0,0 +1,896 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Driver for Jedec PMIC5000 compliant sensors > + * > + * Copyright (c) 2026 Stephen Horvath > + * > + * Inspired by spd5118.c. > + * > + * PMIC5000 compliant sensors are typically used on DDR5 memory modules. > + */ > + > +#include <linux/bitops.h> > +#include <linux/bits.h> > +#include <linux/err.h> > +#include <linux/hwmon.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/pm.h> > +#include <linux/regmap.h> > +#include <linux/units.h> > + > +/* PMIC5000 registers. */ > +// clang-format off No way. Are you serious ? > +#define PMIC5000_REG_OVER_VOLT_IN 0x08 > +#define PMIC5000_REG_OVER_CURRENT 0x09 > + #define PMIC5000_HIGH_TEMP BIT(7) > +#define PMIC5000_REG_OVER_VOLTAGE 0x0A > +#define PMIC5000_REG_UNDER_VOLTAGE 0x0B > +#define PMIC5000_REG_SWA_POWER 0x0C > +#define PMIC5000_REG_SWB_POWER 0x0D > +#define PMIC5000_REG_SWC_POWER 0x0E > +#define PMIC5000_REG_SWD_POWER 0x0F > +#define PMIC5000_REG_OUTPUT_SELECT 0x1A > + #define PMIC5000_OUTPUT_SELECT BIT(1) > +#define PMIC5000_REG_THRES_AND_SEL 0x1B > + #define PMIC5000_VIN_MGMT_THRESH BIT(5) > + #define PMIC5000_CURR_OR_PWR BIT(6) > + #define PMIC5000_VIN_BULK_THRESH BIT(7) > +#define PMIC5000_REG_SWA_CURR_WARN 0x1C > +#define PMIC5000_REG_SWB_CURR_WARN 0x1D > +#define PMIC5000_REG_SWC_CURR_WARN 0x1E > +#define PMIC5000_REG_SWD_CURR_WARN 0x1F > +#define PMIC5000_REG_SWA_VOLT_SET 0x21 > +#define PMIC5000_REG_SWA_THRESH 0x22 > +#define PMIC5000_REG_SWB_VOLT_SET 0x23 > +#define PMIC5000_REG_SWB_THRESH 0x24 > +#define PMIC5000_REG_SWC_VOLT_SET 0x25 > +#define PMIC5000_REG_SWC_THRESH 0x26 > +#define PMIC5000_REG_SWD_VOLT_SET 0x27 > +#define PMIC5000_REG_SWD_THRESH 0x28 > +#define PMIC5000_REG_SW_VOLT_RANGE 0x2B > + #define PMIC5000_SWD_RANGE BIT(0) > + #define PMIC5000_SWC_RANGE BIT(3) > + #define PMIC5000_SWB_RANGE BIT(4) > + #define PMIC5000_SWA_RANGE BIT(5) > +#define PMIC5000_REG_REGULATOR_CONTROL 0x2F > + #define PMIC5000_SWD_CONTROL BIT(3) > + #define PMIC5000_SWC_CONTROL BIT(4) > + #define PMIC5000_SWB_CONTROL BIT(5) > + #define PMIC5000_SWA_CONTROL BIT(6) > +#define PMIC5000_REG_ADC_CONFIG 0x30 > + #define PMIC5000_ADC_SELECT_MASK GENMASK(6, 3) > + #define PMIC5000_ADC_ENABLE BIT(7) > +#define PMIC5000_REG_ADC_VOLTAGE 0x31 > +#define PMIC5000_REG_TEMPERATURE 0x33 > +#define PMIC5000_REG_REVISION 0x3B > +#define PMIC5000_REG_VENDOR 0x3C > + > + Please run checkpatch --strict on your patches and fix what it reports. > +/* 125 mA multiplier */ > +#define PMIC5000_CURR_UNIT 125 > +/* 125 mW multiplier */ > +#define PMIC5000_POWER_UNIT (125 * MILLIWATT_PER_WATT) > +/* mV multipliers */ > +#define PMIC5000_VOLT_UNIT 15 > +#define PMIC5000_VINBULK_UNIT 70 > +#define PMIC5000_VBIAS_UNIT 25 > +// clang-format on > + > +struct pmic5000_data { > + struct regmap *regmap; > + struct mutex mode_lock; > + struct mutex adc_lock; Explain why those are needed on top of the hwmon subsystem lock. > +}; > + > +static const char *const pmic5000_power_labels[] = { "SWA", "SWB", "SWC", > + "SWD" }; > + > +static const char *const pmic5000_voltage_labels[] = { > + "SWA", "SWB", "SWC", "SWD", NULL, > + "VIN_Bulk", "VIN_Mgmt", "VBias", "VOUT_1.8V", "VOUT_1.0V" > +}; > + > +/* hwmon */ > + > +static int pmic5000_check_regulator_enabled(struct regmap *regmap, int channel) Pretty clear indication that this should be a regulator driver. > +{ > + u32 regval; > + int err; > + > + err = regmap_read(regmap, PMIC5000_REG_REGULATOR_CONTROL, ®val); > + if (err) > + return err; > + switch (channel) { > + case 0: > + return !!(regval & PMIC5000_SWA_CONTROL); > + case 1: > + return !!(regval & PMIC5000_SWB_CONTROL); > + case 2: > + return !!(regval & PMIC5000_SWC_CONTROL); > + case 3: > + return !!(regval & PMIC5000_SWD_CONTROL); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int pmic5000_read_temp(struct regmap *regmap, u32 attr, int channel, > + long *val) > +{ > + int err; > + u32 regval; > + > + if (channel != 0) > + return -EOPNOTSUPP; Unnecessary check. > + > + switch (attr) { > + case hwmon_temp_input: { > + err = regmap_read(regmap, PMIC5000_REG_TEMPERATURE, ®val); > + if (err) > + return err; > + regval >>= 5; > + /* Below 85°C */ > + if (regval == 0) > + return -EOPNOTSUPP; > + /* 0b001 = 85°C, 0b010 = 95°C, etc. */ > + *val = (75 + regval * 10) * MILLIDEGREE_PER_DEGREE; > + return 0; > + } > + case hwmon_temp_max: { > + err = regmap_read(regmap, PMIC5000_REG_THRES_AND_SEL, ®val); > + if (err) > + return err; > + regval &= 0x07; > + /* Reserved */ > + if (regval == 0 || regval == 0x7) > + return -EOPNOTSUPP; > + *val = (75 + regval * 10) * MILLIDEGREE_PER_DEGREE; > + return 0; > + } > + case hwmon_temp_max_alarm: { > + err = regmap_read(regmap, PMIC5000_REG_OVER_CURRENT, ®val); > + if (err) > + return err; > + *val = !!(regval & PMIC5000_HIGH_TEMP); > + return 0; > + } > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int pmic5000_read_curr(struct pmic5000_data *data, u32 attr, int channel, > + long *val) > +{ > + struct regmap *regmap = data->regmap; > + int reg, err; > + int shift = 0; > + u32 regval; > + > + if (attr == hwmon_curr_input) { > + mutex_lock(&data->mode_lock); I don't see why this is needed on top of the hwmon subsystem lock. Same for other usage of this lock. > + /* Select power measurements */ > + err = regmap_update_bits(regmap, PMIC5000_REG_THRES_AND_SEL, > + PMIC5000_CURR_OR_PWR, 0); > + if (err) > + goto error; > + > + switch (channel) { > + case 0: > + reg = PMIC5000_REG_SWA_POWER; > + break; > + case 1: > + reg = PMIC5000_REG_SWB_POWER; > + break; > + case 2: > + reg = PMIC5000_REG_SWC_POWER; > + break; > + case 3: > + reg = PMIC5000_REG_SWD_POWER; > + break; > + default: > + err = -EOPNOTSUPP; > + goto error; > + } > + } else if (attr == hwmon_curr_max) { > + shift = 2; > + switch (channel) { > + case 0: > + reg = PMIC5000_REG_SWA_CURR_WARN; > + break; > + case 1: > + reg = PMIC5000_REG_SWB_CURR_WARN; > + break; > + case 2: > + reg = PMIC5000_REG_SWC_CURR_WARN; > + break; > + case 3: > + reg = PMIC5000_REG_SWD_CURR_WARN; > + break; > + default: > + return -EOPNOTSUPP; > + } > + } else if (attr == hwmon_curr_max_alarm && channel >= 0 && > + channel <= 3) { Unnecessary channel checks (at least if the is_visible function does its job and the info data is correct). > + err = regmap_read(regmap, PMIC5000_REG_OVER_CURRENT, ®val); > + if (err) > + return err; > + *val = regval >> (3 - channel) & 0x01; > + return 0; > + } else { > + return -EOPNOTSUPP; > + } > + > + err = regmap_read(regmap, reg, ®val); > + if (err) > + goto error; > + > + if (attr == hwmon_curr_input) > + mutex_unlock(&data->mode_lock); > + > + *val = regval * PMIC5000_CURR_UNIT >> shift; > + return 0; > + > +error: > + if (attr == hwmon_curr_input) > + mutex_unlock(&data->mode_lock); > + return err; > +} > + > +static int pmic5000_read_power(struct pmic5000_data *data, u32 attr, > + int channel, long *val) > +{ > + struct regmap *regmap = data->regmap; > + int reg, err; > + u32 regval; > + > + if (attr != hwmon_power_input) > + return -EOPNOTSUPP; > + > + mutex_lock(&data->mode_lock); > + /* Select power measurements */ > + err = regmap_update_bits(regmap, PMIC5000_REG_THRES_AND_SEL, > + PMIC5000_CURR_OR_PWR, PMIC5000_CURR_OR_PWR); > + if (err) > + goto error; > + > + switch (channel) { > + case 0: > + reg = PMIC5000_REG_SWA_POWER; > + break; > + case 1: > + reg = PMIC5000_REG_SWB_POWER; > + break; > + case 2: > + reg = PMIC5000_REG_SWC_POWER; > + break; > + case 3: > + reg = PMIC5000_REG_SWD_POWER; > + break; > + default: > + err = -EOPNOTSUPP; > + goto error; > + } > + > + err = regmap_read(regmap, reg, ®val); > + if (err) > + goto error; > + > + mutex_unlock(&data->mode_lock); > + > + *val = regval * PMIC5000_POWER_UNIT; > + return 0; > + > +error: > + mutex_unlock(&data->mode_lock); > + return err; > +} > + > +static int pmic5000_read_volt_thresholds(struct regmap *regmap, u32 attr, > + int channel, long *val) > +{ > + int err; > + u32 set_reg, thresh_reg, range_bit; > + u32 set_regval, thresh_regval, range_regval; > + u32 volt_set; > + int base_volts[2]; > + > + switch (channel) { > + case 0: > + set_reg = PMIC5000_REG_SWA_VOLT_SET; > + thresh_reg = PMIC5000_REG_SWA_THRESH; > + range_bit = PMIC5000_SWA_RANGE; > + base_volts[0] = 800; > + base_volts[1] = 600; > + break; > + case 1: > + set_reg = PMIC5000_REG_SWB_VOLT_SET; > + thresh_reg = PMIC5000_REG_SWB_THRESH; > + range_bit = PMIC5000_SWB_RANGE; > + base_volts[0] = 800; > + base_volts[1] = 600; > + break; > + case 2: > + set_reg = PMIC5000_REG_SWC_VOLT_SET; > + thresh_reg = PMIC5000_REG_SWC_THRESH; > + range_bit = PMIC5000_SWC_RANGE; > + base_volts[0] = 800; > + base_volts[1] = 600; > + break; > + case 3: > + set_reg = PMIC5000_REG_SWD_VOLT_SET; > + thresh_reg = PMIC5000_REG_SWD_THRESH; > + range_bit = PMIC5000_SWD_RANGE; > + base_volts[0] = 1500; > + base_volts[1] = 2200; > + break; > + case 5: > + case 6: { Why "{" ? > + err = regmap_read(regmap, PMIC5000_REG_THRES_AND_SEL, > + &thresh_regval); > + if (err) > + return err; > + if (channel == 5) { > + if (thresh_regval & PMIC5000_VIN_BULK_THRESH) > + *val = 14500; > + else > + *val = 16000; > + return 0; > + } else { > + if (thresh_regval & PMIC5000_VIN_MGMT_THRESH) > + *val = 3800; > + else > + *val = 3700; > + return 0; > + } > + } > + > + default: > + return -EOPNOTSUPP; > + } > + > + err = regmap_read(regmap, set_reg, &set_regval); > + if (err) > + return err; > + err = regmap_read(regmap, thresh_reg, &thresh_regval); > + if (err) > + return err; > + err = regmap_read(regmap, PMIC5000_REG_SW_VOLT_RANGE, &range_regval); > + if (err) > + return err; > + > + volt_set = range_regval & range_bit ? base_volts[1] : base_volts[0]; > + volt_set += (set_regval >> 1) * 5; > + > + switch (attr) { > + case hwmon_in_min: > + /* 10%, 12.5%, Reserved, Reserved */ > + const int min_permilles[4] = { 100, 125, PERMILLE, PERMILLE }; > + *val = volt_set - (min_permilles[(thresh_regval >> 2) & 0x03] * > + volt_set / PERMILLE); > + return 0; > + case hwmon_in_max: > + /* 7.5%, 10%, 12.5%, Reserved */ > + const int max_permilles[4] = { 75, 100, 125, PERMILLE }; > + *val = volt_set + (max_permilles[(thresh_regval >> 4) & 0x03] * > + volt_set / PERMILLE); > + return 0; > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int pmic5000_read_adc_alarms(struct regmap *regmap, u32 attr, > + int channel, long *val) > +{ > + int err; > + u32 regval; > + > + if (channel >= 0 && channel <= 3) { channel is always >= 0. > + switch (attr) { > + case hwmon_in_min_alarm: { > + err = regmap_read(regmap, PMIC5000_REG_UNDER_VOLTAGE, > + ®val); > + if (err) > + return err; > + *val = regval >> (3 - channel) & 0x01; > + return 0; > + } > + case hwmon_in_max_alarm: { > + err = regmap_read(regmap, PMIC5000_REG_OVER_VOLTAGE, > + ®val); > + if (err) > + return err; > + *val = regval >> (7 - channel) & 0x01; > + return 0; > + } > + } Alignment. > + } else if (channel == 5 || channel == 6) { > + switch (attr) { > + case hwmon_in_max_alarm: { > + err = regmap_read(regmap, PMIC5000_REG_OVER_VOLT_IN, > + ®val); > + if (err) > + return err; > + *val = regval >> (channel - 4) & 0x01; > + return 0; > + } > + } > + } > + > + return -EOPNOTSUPP; > +} > + > +static int pmic5000_read_adc(struct pmic5000_data *data, u32 attr, int channel, > + long *val) > +{ > + struct regmap *regmap = data->regmap; > + int err, mult; > + u32 regval; > + > + switch (attr) { > + case hwmon_in_enable: > + err = pmic5000_check_regulator_enabled(regmap, channel); > + if (err < 0) > + return err; > + *val = err; > + return 0; The enable attribute is supposed to enable or disable monitoring. It is not supposed to report regulator status. > + case hwmon_in_input: > + break; > + case hwmon_in_min: > + case hwmon_in_max: > + return pmic5000_read_volt_thresholds(regmap, attr, channel, > + val); > + case hwmon_in_min_alarm: > + case hwmon_in_max_alarm: > + return pmic5000_read_adc_alarms(regmap, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > + > + /* Channel 4 is reserved */ > + if (channel < 0 || channel > 9 || channel == 4) > + return -EOPNOTSUPP; Then it should not be enabled by the is_visible function. Also, channel is never < 0 or > 9. > + > + switch (channel) { + case 5: > + mult = PMIC5000_VINBULK_UNIT; > + break; > + case 7: > + mult = PMIC5000_VBIAS_UNIT; > + break; > + default: > + mult = PMIC5000_VOLT_UNIT; > + break; > + } > + > + mutex_lock(&data->adc_lock); > + > + err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG, > + PMIC5000_ADC_SELECT_MASK, channel << 3); > + if (err) > + goto error; > + > + /* > + * The host shall wait minimum of 9 ms delay after the input selection > + * for ADC readout and the actual readout > + * > + * msleep may sleep for up to 20ms, which is fine. No, it isn't fine. Also, why wait if the channel was not changed ? > + */ > + msleep(9); > + > + err = regmap_read(regmap, PMIC5000_REG_ADC_VOLTAGE, ®val); > + if (err) > + goto error; > + > + mutex_unlock(&data->adc_lock); > + > + *val = regval * mult; > + return 0; > + > +error: > + mutex_unlock(&data->adc_lock); > + return err; > +} > + > +static int pmic5000_read_interval(struct regmap *regmap, u32 attr, long *val) > +{ > + unsigned int regval; > + int err; > + > + if (attr != hwmon_chip_update_interval) > + return -EOPNOTSUPP; > + > + err = regmap_read(regmap, PMIC5000_REG_ADC_CONFIG, ®val); > + if (err < 0) > + return err; > + *val = 1 << (regval & 0x03); BIT() > + return 0; > +} > + > +static int pmic5000_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct pmic5000_data *data = dev_get_drvdata(dev); > + struct regmap *regmap = data->regmap; > + > + switch (type) { > + case hwmon_chip: > + return pmic5000_read_interval(regmap, attr, val); > + case hwmon_temp: > + return pmic5000_read_temp(regmap, attr, channel, val); > + case hwmon_in: > + return pmic5000_read_adc(data, attr, channel, val); > + case hwmon_curr: > + return pmic5000_read_curr(data, attr, channel, val); > + case hwmon_power: > + return pmic5000_read_power(data, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int pmic5000_read_string(struct device *dev, > + enum hwmon_sensor_types type, u32 attr, > + int channel, const char **str) > +{ > + if (type == hwmon_curr && attr == hwmon_curr_label) { > + if (channel < 0 || channel > 3) > + return -EOPNOTSUPP; Again, channel is never < 0, and unless is_visible and/or the configuration data is wrong it should never be > 3. > + *str = pmic5000_power_labels[channel]; > + } else if (type == hwmon_power && attr == hwmon_power_label) { > + if (channel < 0 || channel > 3) > + return -EOPNOTSUPP; > + *str = pmic5000_power_labels[channel]; > + } else if (type == hwmon_in && attr == hwmon_in_label) { > + if (channel < 0 || channel > 9 || channel == 4) Many more unencessary channel checks. > + return -EOPNOTSUPP; > + *str = pmic5000_voltage_labels[channel]; > + } else { > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int pmic5000_write_interval(struct pmic5000_data *data, long val) > +{ > + struct regmap *regmap = data->regmap; > + u32 regval; > + int err; > + > + switch (val) { > + case 1: > + regval = 0; > + break; > + case 2: > + regval = 1; > + break; > + case 4: > + regval = 2; > + break; > + case 8: > + regval = 3; > + break; > + default: > + return -EINVAL; > + } find_closest() would be more appropriate here. We don't usually expect users to know valid update intervals. > + > + mutex_lock(&data->adc_lock); > + err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG, 0x03, regval); > + mutex_unlock(&data->adc_lock); Another unnecessary lock. > + return err; > +} > + > +static int pmic5000_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + struct pmic5000_data *data = dev_get_drvdata(dev); > + > + switch (type) { > + case hwmon_chip: > + return pmic5000_write_interval(data, val); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static umode_t pmic5000_is_visible(const void *data, > + enum hwmon_sensor_types type, u32 attr, > + int channel) > +{ > + int ret; > + struct pmic5000_data *pmic_data = (struct pmic5000_data *)data; > + struct regmap *regmap = pmic_data->regmap; > + > + switch (type) { > + case hwmon_chip: > + if (attr == hwmon_chip_update_interval) > + return 0644; > + break; > + case hwmon_temp: > + return 0444; > + case hwmon_in: > + if (channel == 4) > + return 0; > + if (channel >= 0 && channel <= 3 && (attr != hwmon_in_enable)) { > + ret = pmic5000_check_regulator_enabled(regmap, channel); > + if (!ret || ret < 0) > + return 0; > + } > + return 0444; > + case hwmon_power: > + if (channel >= 0 && channel <= 3) { > + ret = pmic5000_check_regulator_enabled(regmap, channel); > + if (!ret || ret < 0) > + return 0; > + } > + return 0444; > + case hwmon_curr: > + if (channel >= 0 && channel <= 3) { > + ret = pmic5000_check_regulator_enabled(regmap, channel); > + if (!ret || ret < 0) > + return 0; > + } > + return 0444; > + default: > + break; > + } > + return 0444; > +} > + > +/* > + * Bank and vendor id are 8-bit fields with seven data bits and odd parity. > + * Vendor IDs 0 and 0x7f are invalid. > + * See Jedec standard JEP106BJ for details and a list of assigned vendor IDs. > + */ > +static bool pmic5000_vendor_valid(u8 bank, u8 id) > +{ > + if (parity8(bank) == 0 || parity8(id) == 0) > + return false; > + > + id &= 0x7f; > + return id && id != 0x7f; > +} > + > +static const struct hwmon_channel_info *pmic5000_info[] = { > + HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL), > + HWMON_CHANNEL_INFO(temp, > + HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_ALARM), > + HWMON_CHANNEL_INFO( > + in, > + HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | > + HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL, > + HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | > + HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL, > + HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | > + HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL, > + HWMON_I_ENABLE | HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | > + HWMON_I_MIN_ALARM | HWMON_I_MAX_ALARM | HWMON_I_LABEL, > + HWMON_I_INPUT, > + HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_MAX_ALARM | HWMON_I_LABEL, > + HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_MAX_ALARM | HWMON_I_LABEL, > + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL, > + HWMON_I_INPUT | HWMON_I_LABEL), > + HWMON_CHANNEL_INFO( > + curr, > + HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL, > + HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL, > + HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | HWMON_C_LABEL, > + HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_MAX_ALARM | > + HWMON_C_LABEL), > + HWMON_CHANNEL_INFO(power, HWMON_P_INPUT | HWMON_P_LABEL, > + HWMON_P_INPUT | HWMON_P_LABEL, > + HWMON_P_INPUT | HWMON_P_LABEL, > + HWMON_P_INPUT | HWMON_P_LABEL), > + NULL > +}; > + > +static const struct hwmon_ops pmic5000_hwmon_ops = { > + .is_visible = pmic5000_is_visible, > + .read = pmic5000_read, > + .read_string = pmic5000_read_string, > + .write = pmic5000_write, > +}; > + > +static const struct hwmon_chip_info pmic5000_chip_info = { > + .ops = &pmic5000_hwmon_ops, > + .info = pmic5000_info, > +}; > + > +/* regmap */ > + > +static bool pmic5000_writeable_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case PMIC5000_REG_OUTPUT_SELECT: > + case PMIC5000_REG_THRES_AND_SEL: > + case PMIC5000_REG_ADC_CONFIG: > + return true; > + default: > + return false; > + } > +} > + > +static bool pmic5000_volatile_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case PMIC5000_REG_OVER_VOLT_IN: > + case PMIC5000_REG_OVER_CURRENT: > + case PMIC5000_REG_OVER_VOLTAGE: > + case PMIC5000_REG_UNDER_VOLTAGE: > + case PMIC5000_REG_SWA_POWER: > + case PMIC5000_REG_SWB_POWER: > + case PMIC5000_REG_SWC_POWER: > + case PMIC5000_REG_SWD_POWER: > + case PMIC5000_REG_ADC_VOLTAGE: > + case PMIC5000_REG_TEMPERATURE: > + return true; > + default: > + return false; > + } > +} > + > +static const struct regmap_config pmic5000_regmap8_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0x3f, > + .writeable_reg = pmic5000_writeable_reg, > + .volatile_reg = pmic5000_volatile_reg, > + .cache_type = REGCACHE_MAPLE, > +}; > + > +static int pmic5000_suspend(struct device *dev) > +{ > + struct pmic5000_data *data = dev_get_drvdata(dev); > + struct regmap *regmap = data->regmap; > + u32 regval; > + int err; > + > + /* > + * Make sure the configuration register in the regmap cache is current > + * before bypassing it. > + */ > + err = regmap_read(regmap, PMIC5000_REG_ADC_CONFIG, ®val); > + if (err < 0) > + return err; > + > + regcache_cache_bypass(regmap, true); > + regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG, PMIC5000_ADC_ENABLE, > + 0); > + regcache_cache_bypass(regmap, false); > + > + regcache_cache_only(regmap, true); > + regcache_mark_dirty(regmap); > + > + return 0; > +} > + > +static int pmic5000_resume(struct device *dev) > +{ > + struct pmic5000_data *data = dev_get_drvdata(dev); > + struct regmap *regmap = data->regmap; > + > + regcache_cache_only(regmap, false); > + return regcache_sync(regmap); > +} > + > +static DEFINE_SIMPLE_DEV_PM_OPS(pmic5000_pm_ops, pmic5000_suspend, > + pmic5000_resume); > + > +static int pmic5000_common_probe(struct device *dev, struct regmap *regmap) Why pmic5000_common_probe() ? There is only one caller. > +{ > + unsigned int revision, vendor, bank; > + struct pmic5000_data *data; > + struct device *hwmon_dev; > + int err; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + err = regmap_read(regmap, PMIC5000_REG_REVISION, &revision); > + if (err) > + return err; > + > + err = regmap_read(regmap, PMIC5000_REG_VENDOR, &bank); > + if (err) > + return err; > + err = regmap_read(regmap, PMIC5000_REG_VENDOR + 1, &vendor); > + if (err) > + return err; > + if (!pmic5000_vendor_valid(bank, vendor)) > + return -ENODEV; > + > + data->regmap = regmap; > + mutex_init(&data->mode_lock); > + mutex_init(&data->adc_lock); > + dev_set_drvdata(dev, data); > + > + hwmon_dev = devm_hwmon_device_register_with_info( > + dev, "pmic5000", data, &pmic5000_chip_info, NULL); > + if (IS_ERR(hwmon_dev)) > + return PTR_ERR(hwmon_dev); > + > + dev_info(dev, "DDR5 PMIC sensor: vendor 0x%02x:0x%02x revision %d.%d\n", > + bank & 0x7f, vendor, ((revision >> 4) & 0x03) + 1, > + ((revision >> 1) & 0x07) + 1); > + > + /* Enable individual measurements and enable ADC */ > + err = regmap_update_bits(regmap, PMIC5000_REG_OUTPUT_SELECT, > + PMIC5000_OUTPUT_SELECT, > + PMIC5000_OUTPUT_SELECT); > + if (err) > + return err; > + err = regmap_update_bits(regmap, PMIC5000_REG_ADC_CONFIG, > + PMIC5000_ADC_ENABLE, PMIC5000_ADC_ENABLE); > + if (err) > + return err; > + > + return 0; > +} > + > +/* I2C */ > + > +static int pmic5000_i2c_init(struct i2c_client *client) > +{ > + struct i2c_adapter *adapter = client->adapter; > + > + /* > + * Register accesses are 8-bit, so require byte-data transactions only. > + * Requiring WORD_DATA here rejects otherwise valid adapters. > + */ > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) > + return -ENODEV; > + > + return 0; > +} > + > +static int pmic5000_i2c_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct regmap *regmap; > + int err; > + > + err = pmic5000_i2c_init(client); > + if (err) > + return dev_err_probe(dev, err, "I2C capability check failed\n"); > + > + regmap = devm_regmap_init_i2c(client, &pmic5000_regmap8_config); > + if (IS_ERR(regmap)) > + return dev_err_probe(dev, PTR_ERR(regmap), > + "regmap init failed\n"); > + > + return pmic5000_common_probe(dev, regmap); > +} > + > +static const struct i2c_device_id pmic5000_i2c_id[] = { { .name = "pmic5000" }, > + {} }; > +MODULE_DEVICE_TABLE(i2c, pmic5000_i2c_id); > + > +static const struct of_device_id pmic5000_of_ids[] = { > + { > + .compatible = "jedec,pmic5000", jedec,pmic5000 is not documented. > + }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, pmic5000_of_ids); > + > +static struct i2c_driver pmic5000_i2c_driver = { > + .class = I2C_CLASS_HWMON, > + .driver = { > + .name = "pmic5000", > + .of_match_table = pmic5000_of_ids, > + .pm = pm_sleep_ptr(&pmic5000_pm_ops), > + }, > + .probe = pmic5000_i2c_probe, > + .id_table = pmic5000_i2c_id, > +}; > + > +module_i2c_driver(pmic5000_i2c_driver); > + > +MODULE_AUTHOR("Stephen Horvath <[email protected]>"); > +MODULE_DESCRIPTION("PMIC5000 driver"); > +MODULE_LICENSE("GPL");