Openbmc archive at lore.kernel.org
 help / color / Atom feed
* [PATCH linux dev-5.8] hwmon: Ampere Computing ALTRA SMPMPRO sensor driver
@ 2020-09-30 22:26 Lancelot
  2020-10-01 12:32 ` Patrick Williams
  0 siblings, 1 reply; 4+ messages in thread
From: Lancelot @ 2020-09-30 22:26 UTC (permalink / raw)
  To: Joel Stanley, openbmc; +Cc: Xiaopeng XP Chen, Lancelot Kao

From: Lancelot Kao <lancelot.kao@fii-usa.com>

Add SMPMPro-hwmon driver to monitor Ampere CPU/Memory/VR via an
i2c interface of the CPU's smpmpro management device.

Signed-off-by: Xiaopeng XP Chen <xiao-peng.chen@fii-na.com>
Signed-off-by: Lancelot Kao <lancelot.kao@fii-usa.com>
---
 MAINTAINERS                   |   8 +
 drivers/hwmon/Kconfig         |  10 +
 drivers/hwmon/Makefile        |   1 +
 drivers/hwmon/smpmpro-hwmon.c | 903 ++++++++++++++++++++++++++++++++++
 4 files changed, 922 insertions(+)
 create mode 100644 drivers/hwmon/smpmpro-hwmon.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5fecb388d073..c169fd9a4d7c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15759,6 +15759,14 @@ S:	Maintained
 F:	Documentation/hwmon/smm665.rst
 F:	drivers/hwmon/smm665.c
 
+SMPMPRO HARDWARE MONITOR DRIVER
+M:	Lancelot Kao <lancelot.kao@fii-usa.com>
+M:	Xiaopeng XP Chen <xiao-peng.chen@fii-na.com>
+M:	Mohaimen Alsamarai <Mohaimen.Alsamarai@fii-na.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	drivers/hwmon/smpmpro-hwmon.c
+
 SMSC EMC2103 HARDWARE MONITOR DRIVER
 M:	Steve Glendinning <steve.glendinning@shawell.net>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 9aa89d7d4193..50881ebcb022 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1777,6 +1777,16 @@ config SENSORS_INA3221
 	  This driver can also be built as a module. If so, the module
 	  will be called ina3221.
 
+config SENSORS_SMPMPRO
+	tristate "Ampere Computing Altra SMPMPRO sensor chip"
+	depends on I2C
+	help
+	  If you say yes here you get support for Ampere Computing Altra SMPMPRO
+	  sensor chip. This will enable monitoring of Ampere CPU Sensors; via hwmon.
+
+	  This driver can also be built as a module. If so, the module 
+	  will be called smpmpro-hwmon.
+
 config SENSORS_TC74
 	tristate "Microchip TC74"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index ae41ee71a71b..49a1a8e0c73f 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -166,6 +166,7 @@ obj-$(CONFIG_SENSORS_SHT3x)	+= sht3x.o
 obj-$(CONFIG_SENSORS_SHTC1)	+= shtc1.o
 obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
+obj-$(CONFIG_SENSORS_SMPMPRO)	+= smpmpro-hwmon.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
diff --git a/drivers/hwmon/smpmpro-hwmon.c b/drivers/hwmon/smpmpro-hwmon.c
new file mode 100644
index 000000000000..37b48c42a372
--- /dev/null
+++ b/drivers/hwmon/smpmpro-hwmon.c
@@ -0,0 +1,903 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Ampere Computing SoC Hardware Monitoring Driver
+ *
+ * Copyright (c) 2019-2020, Ampere Computing LLC
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+/* Identification Registers */
+#define REG_SPEC_VERSION_REG	0x00
+#define SCP_VERSION_REG	        0x01
+#define MANUFACTURER_ID_REG	    0x02
+#define DEVICE_ID_REG	        0x03
+#define SCP_BUILD_ID_LO_REG	    0x09
+#define SCP_BUILD_ID_HI_REG	    0x0A
+
+/* Capability Registers  */
+#define TEMP_SENSOR_SUPPORT_REG	0x05
+#define PWR_SENSOR_SUPPORT_REG	0x06
+#define VOLT_SENSOR_SUPPORT_REG	0x07
+#define OTHER_CAP_REG		    0x08
+#define CORE_CLUSTER_CNT_REG	0x0B
+#define SYS_CACHE_PCIE_CNT_REG	0x0C
+#define SOCKET_INFO_REG	        0x0D
+
+/* Logical Power Sensor Registers */
+#define SOC_TEMP_REG		0x10
+#define SOC_VRD_TEMP_REG	0x11
+#define DIMM_VRD_TEMP_REG	0x12
+#define CORE_VRD_TEMP_REG	0x13
+#define CH0_DIMM_TEMP_REG	0x14
+#define CH1_DIMM_TEMP_REG	0x15
+#define CH2_DIMM_TEMP_REG	0x16
+#define CH3_DIMM_TEMP_REG	0x17
+#define CH4_DIMM_TEMP_REG	0x18
+#define CH5_DIMM_TEMP_REG	0x19
+#define CH6_DIMM_TEMP_REG	0x1A
+#define CH7_DIMM_TEMP_REG	0x1B
+#define CORE_VRD_PWR_REG		0x20
+#define SOC_VRD_PWR_REG		0x21
+#define DIMM_VRD1_PWR_REG	0x22
+#define DIMM_VRD2_PWR_REG	0x23
+#define CORE_VRD_PWR_MW_REG	0x26
+#define SOC_VRD_PWR_MW_REG	0x27
+#define DIMM_VRD1_PWR_MW_REG	0x28
+#define DIMM_VRD2_PWR_MW_REG	0x29
+#define MEM_HOT_THRESHOLD_REG	0x32
+#define SOC_VR_HOT_THRESHOLD_REG	0x33
+#define CORE_VRD_VOLT_REG	0x34
+#define SOC_VRD_VOLT_REG	0x35
+#define DIMM_VRD1_VOLT_REG	0x36
+#define DIMM_VRD2_VOLT_REG	0x37
+#define DIMM_CE_THRESHOLD_REG	0x38
+
+/* GPI Control set  Registers */
+#define GPI_CTRL0_REG		0x50
+#define GPI_CTRL1_REG		0x51
+#define GPI_CTRL2_REG		0x52
+#define GPI_CTRL3_REG		0x53
+#define GPI_CE_UE_MASK_REG		0x54
+
+/* GPI data set Registers */
+#define GPI_DATA_SET_REG	0x60
+#define GPI_DATA_SET0_REG	0x61
+#define GPI_DATA_SET1_REG	0x62
+#define GPI_DATA_SET2_REG	0x63
+#define GPI_DATA_SET3_REG	0x64
+
+/* GPI Status Registers */
+#define GPI_CLUSTER_ERR_SET0_REG	0x70
+#define GPI_CLUSTER_ERR_SEL_REG	 0x71
+#define GPI_MCU_ERR_REG		0x72
+#define GPI_PCIE_ERR_REG	0x73
+#define GPI_SYS_CACHE_ERR_SEL_REG	0x74
+#define GPI_SYS_CACHE_ERR_REG	0x75
+#define GPI_PCIE_ERR_SEL_REG	0x76
+#define GPI_VRD_FAULT_ERR_REG	0x78
+#define GPI_VRD_HOT_ERR_REG	0x79
+#define GPI_DIMM_HOT_ERR_REG	0x7A
+#define GPI_BOOT_ERR1_REG	0x7B
+#define GPI_BOOT_ERR2_REG	0x7C
+
+/* GPI RAS Error Registers */
+#define GPI_WDT_STS_REG		0x7D
+#define GPI_RAS_ERR_REG		0x7E
+
+/* Core and L2C Error Registers */
+#define GPI_CORE_CLUSTER_SEL_REG	0x80
+#define GPI_CORE_L1_ERR_REG	0x81
+#define GPI_CORE_L2_ERR_REG	0x82
+#define GPI_SYS_CACHE_INST_SEL_REG	0x86
+#define GPI_SYS_CACHE_ERR_REG	0x87
+
+/* Memory Error Registers */
+#define GPI_MCU_DIMM_SELECT_REG	0x90
+#define GPI_MCU_DIMM_ERR_REG	0x91
+
+/* RAS Error/Warning Registers */
+#define GPI_RAS_ERR_SMPRO_TYPE_REG	0xA0
+#define GPI_RAS_ERR_PMPRO_TYPE_REG	0xA1
+#define GPI_RAS_ERR_SMPRO_INFO_LO_REG	0xA2
+#define GPI_RAS_ERR_SMPRO_INFO_HI_REG	0xA3
+#define GPI_RAS_ERR_SMPRO_DATA_LO_REG	0xA4
+#define GPI_RAS_ERR_SMPRO_DATA_HI_REG	0xA5
+#define GPI_RAS_WARN_SMPRO_INFO_LO_REG	0xAA
+#define GPI_RAS_WARN_SMPRO_INFO_HI_REG	0xAB
+#define GPI_RAS_ERR_PMPRO_INFO_LO_REG	0xA6
+#define GPI_RAS_ERR_PMPRO_INFO_HI_REG	0xA7
+#define GPI_RAS_ERR_PMPRO_DATA_LO_REG	0xA8
+#define GPI_RAS_ERR_PMPRO_DATA_HI_REG	0xA9
+#define GPI_RAS_WARN_PMPRO_INFO_LO_REG	0xAC
+#define GPI_RAS_WARN_PMPRO_INFO_HI_REG	0xAD
+
+/* Boot Stage/Progress Registers */
+#define GPI_BOOT_STAGE_SELECT_REG	0xB0
+#define GPI_BOOT_STAGE_STATUS_LO_REG	0xB1
+#define GPI_BOOT_STAGE_CUR_STAGE_REG	0xB2
+
+/* PCIE Error Registers */
+#define GPI_PCIE_ERR_SEL_REG		0xC0
+#define GPI_PCIE_ERR_TYPE_REG		0xC1
+#define GPI_PCIE_ERR_DEVICE_REG	0xC2
+
+/* Other Error Registers */
+#define OTHER_CE_ERR_CNT_REG	0xD0
+#define OTHER_CE_ERR_LEN_REG	0xD1
+#define OTHER_CE_ERR_DATA_REG	0xD2
+#define OTHER_UE_ERR_CNT_REG	0xD0
+#define OTHER_UE_ERR_LEN_REG	0xD1
+#define OTHER_UE_ERR_DATA_REG	0xD2
+
+/* ACPI State Registers */
+#define ACPI_SYSTEM_STATE_REG		0xE0
+#define ACPI_CPPC_CLUSTER_SEL_REG	0xE3
+#define ACPI_CPPC_CLUSTER_DATA_REG		0xE4
+#define ACPI_POWER_LIMIT_REG		0xE5
+
+
+struct smpmpro_data {
+	struct i2c_client *client;
+
+	u16 temp_support_regs;
+	u16 pwr_support_regs;
+	u16 volt_support_regs;
+	u16 other_caps;
+	u16 core_cluster_cnt_reg;
+	u16 sys_cache_pcie_cnt_reg;
+	u16 socket_info_reg;
+};
+
+static const u8 temp_regs[] = {
+	SOC_TEMP_REG,
+	SOC_VRD_TEMP_REG,
+	DIMM_VRD_TEMP_REG,
+	CORE_VRD_TEMP_REG,
+	CH0_DIMM_TEMP_REG,
+	CH1_DIMM_TEMP_REG,
+	CH2_DIMM_TEMP_REG,
+	CH3_DIMM_TEMP_REG,
+	CH4_DIMM_TEMP_REG,
+	CH5_DIMM_TEMP_REG,
+	CH6_DIMM_TEMP_REG,
+	CH7_DIMM_TEMP_REG,
+	MEM_HOT_THRESHOLD_REG,
+	SOC_VR_HOT_THRESHOLD_REG
+};
+
+static const u8 volt_regs[] = {
+	CORE_VRD_VOLT_REG,
+	SOC_VRD_VOLT_REG,
+	DIMM_VRD1_VOLT_REG,
+	DIMM_VRD2_VOLT_REG
+};
+
+enum pwr_regs {
+	PMD_VRD_PWR,
+	SOC_VRD_PWR,
+	DIMM_VRD1_PWR,
+	DIMM_VRD2_PWR,
+	CPU_VRD_PWR,
+	DIMM_VRD_PWR,
+};
+
+static const char * const label[] = {
+	"SoC",
+	"SoC VRD",
+	"DIMM VRD",
+	"DIMM VRD1",
+	"DIMM VRD2",
+	"PMD VRD",
+	"CH0 DIMM",
+	"CH1 DIMM",
+	"CH2 DIMM",
+	"CH3 DIMM",
+	"CH4 DIMM",
+	"CH5 DIMM",
+	"CH6 DIMM",
+	"CH7 DIMM",
+	"MEM HOT",
+	"SoC VR HOT",
+	"CPU VRD",
+};
+
+static const char * const gpi_label[] = {
+	"GPI CTRL 0",
+	"GPI CTRL 1",
+	"GPI CTRL 2",
+	"GPI CTRL 3",
+	"GPI CE UE MASK",
+	"GPI DATA SET",
+	"GPI DATA SET 0",
+	"GPI DATA SET 1",
+	"GPI DATA SET 2",
+	"GPI DATA SET 3",
+	"CLUSTER ERROR SET 0",
+	"CLUSTER ERROR SEL",
+	"MCU ERROR",
+	"PCIE ERROR",
+	"SYS CACHE ERR SEL",
+	"SYS CACHE ERR",
+	"PCIE ERR SEL",
+	"VRD FAULT ERR",
+	"VRD HOT ERROR",
+	"DIMM HOT ERROR",
+	"BOOT 1 ERROR",
+	"BOOT 2 ERROR",
+	"WATCHDOG STATUS",
+	"RAS INTERNAL ERROR",
+};
+
+static void smpmpro_init_device(struct i2c_client *client,
+				struct smpmpro_data *data)
+{
+	u16 ret;
+
+	ret = i2c_smbus_read_word_swapped(client, TEMP_SENSOR_SUPPORT_REG);
+	if (ret < 0)
+		return;
+	data->temp_support_regs = ret;
+
+	ret = i2c_smbus_read_word_swapped(client, PWR_SENSOR_SUPPORT_REG);
+	if (ret < 0)
+		return;
+	data->pwr_support_regs = ret;
+
+	ret = i2c_smbus_read_word_swapped(client, VOLT_SENSOR_SUPPORT_REG);
+	if (ret < 0)
+		return;
+	data->volt_support_regs = ret;
+
+	ret = i2c_smbus_read_word_swapped(client, OTHER_CAP_REG);
+	if (ret < 0)
+		return;
+	data->other_caps = ret;
+
+	ret = i2c_smbus_read_word_swapped(client, CORE_CLUSTER_CNT_REG);
+	if (ret < 0)
+		return;
+	data->core_cluster_cnt_reg = ret;
+
+	ret = i2c_smbus_read_word_swapped(client, SYS_CACHE_PCIE_CNT_REG);
+	if (ret < 0)
+		return;
+	data->sys_cache_pcie_cnt_reg = ret;
+
+	ret = i2c_smbus_read_word_swapped(client, SOCKET_INFO_REG);
+	if (ret < 0)
+		return;
+	data->socket_info_reg = ret;
+}
+
+static int smpmpro_read_temp(struct device *dev, u32 attr, int channel,
+			     long *val)
+{
+	struct smpmpro_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		ret = i2c_smbus_read_word_swapped(client, temp_regs[channel]);
+		if (ret < 0)
+			return ERR_PTR(ret);
+		*val = (ret & 0x1ff) * 1000;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t smpmpro_temp_is_visible(const void *_data, u32 attr, int channel)
+{
+	return 0444;
+}
+
+static int smpmpro_read_in(struct device *dev, u32 attr, int channel,
+			     long *val)
+{
+	struct smpmpro_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	switch (attr) {
+	case hwmon_in_input:
+		ret = i2c_smbus_read_word_swapped(client, volt_regs[channel]);
+		if (ret < 0)
+			return ERR_PTR(ret);
+		*val = ret & 0x7fff;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t smpmpro_in_is_visible(const void *_data, u32 attr, int channel)
+{
+	return 0444;
+}
+
+static int smpmpro_read_power(struct device *dev, u32 attr, int channel,
+			     long *val)
+{
+	struct smpmpro_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret, ret_mw;
+	int ret2 = 0, ret2_mw = 0;
+
+	switch (attr) {
+	case hwmon_power_input:
+		switch (channel) {
+		case PMD_VRD_PWR:
+			ret = i2c_smbus_read_word_swapped(client, CORE_VRD_PWR_REG);
+			ret_mw = i2c_smbus_read_word_swapped(client, CORE_VRD_PWR_MW_REG);
+			break;
+		case SOC_VRD_PWR:
+			ret = i2c_smbus_read_word_swapped(client, SOC_VRD_PWR_REG);
+			ret_mw = i2c_smbus_read_word_swapped(client, SOC_VRD_PWR_MW_REG);
+			break;
+		case DIMM_VRD1_PWR:
+			ret = i2c_smbus_read_word_swapped(client, DIMM_VRD1_PWR_REG);
+			ret_mw = i2c_smbus_read_word_swapped(client, DIMM_VRD1_PWR_MW_REG);
+			break;
+		case DIMM_VRD2_PWR:
+			ret = i2c_smbus_read_word_swapped(client, DIMM_VRD2_PWR_REG);
+			ret_mw = i2c_smbus_read_word_swapped(client, DIMM_VRD2_PWR_MW_REG);
+			break;
+		case CPU_VRD_PWR:
+			ret = i2c_smbus_read_word_swapped(client, CORE_VRD_PWR_REG);
+			ret_mw = i2c_smbus_read_word_swapped(client, CORE_VRD_PWR_MW_REG);
+			ret2 = i2c_smbus_read_word_swapped(client, SOC_VRD_PWR_REG);
+			ret2_mw = i2c_smbus_read_word_swapped(client, SOC_VRD_PWR_MW_REG);
+			break;
+		case DIMM_VRD_PWR:
+			ret = i2c_smbus_read_word_swapped(client, DIMM_VRD1_PWR_REG);
+			ret_mw = i2c_smbus_read_word_swapped(client, DIMM_VRD1_PWR_MW_REG);
+			ret2 = i2c_smbus_read_word_swapped(client, DIMM_VRD2_PWR_REG);
+			ret2_mw = i2c_smbus_read_word_swapped(client, DIMM_VRD2_PWR_MW_REG);
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		if (ret < 0 || ret_mw < 0 || ret2 < 0 || ret2_mw < 0)
+			return ERR_PTR(ret < 0 ? ret : ret_mw < 0 ? ret_mw
+					: ret2 < 0 ? ret2 : ret2_mw);
+		*val = (ret + ret2)*1000000 + (ret_mw + ret2_mw)*1000;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t smpmpro_power_is_visible(const void *_data, u32 attr, int channel)
+{
+	return 0444;
+}
+
+
+static int smpmpro_read_gpi(struct device *dev,
+				 struct device_attribute *da, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct smpmpro_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	ret = i2c_smbus_read_word_swapped(client, attr->index);
+
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
+}
+
+static int smpmpro_read(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return smpmpro_read_temp(dev, attr, channel, val);
+	case hwmon_in:
+		return smpmpro_read_in(dev, attr, channel, val);
+	case hwmon_power:
+		return smpmpro_read_power(dev, attr, channel, val);
+//	case hwmon_dimm_ce_thres:
+//		return smpmpro_read_dimm_ce_thres(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpmpro_write_gpi(struct device *dev,
+				   struct device_attribute *da,
+				   const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct smpmpro_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 16, &val);
+
+	ret = i2c_smbus_write_word_swapped(client, attr->index, val);
+	if (ret < 0)
+		return -EPROTO;
+
+	return count;
+}
+
+static int smpmpro_write(struct device *dev, enum hwmon_sensor_types type,
+			  u32 attr, int channel, long val)
+{
+	return -EOPNOTSUPP;
+}
+
+static umode_t smpmpro_is_visible(const void *data,
+				   enum hwmon_sensor_types type,
+				   u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		return smpmpro_temp_is_visible(data, attr, channel);
+	case hwmon_in:
+		return smpmpro_in_is_visible(data, attr, channel);
+	case hwmon_power:
+		return smpmpro_power_is_visible(data, attr, channel);
+//	case hwmon_dimm_ce_thres:
+//		return smpmpro_dimm_ce_thres_is_visible(data, attr, channel);
+	default:
+		return 0;
+	}
+}
+
+static ssize_t show_label(struct device *dev, struct device_attribute *devattr,
+				 char *buf)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+
+	return sprintf(buf, "%s\n", label[index]);
+}
+
+static ssize_t show_gpi_label(struct device *dev,
+		struct device_attribute *devattr, char *buf)
+{
+	int index = to_sensor_dev_attr(devattr)->index;
+
+	return sprintf(buf, "%s\n", gpi_label[index]);
+}
+
+static const u32 smpmpro_temp_config[] = {
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info smpmpro_temp = {
+	.type = hwmon_temp,
+	.config = smpmpro_temp_config,
+};
+
+static const u32 smpmpro_in_config[] = {
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info smpmpro_in = {
+	.type = hwmon_in,
+	.config = smpmpro_in_config,
+};
+
+static const u32 smpmpro_power_config[] = {
+	HWMON_P_INPUT,
+	HWMON_P_INPUT,
+	HWMON_P_INPUT,
+	HWMON_P_INPUT,
+	HWMON_P_INPUT,
+	HWMON_P_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info smpmpro_power = {
+	.type = hwmon_power,
+	.config = smpmpro_power_config,
+};
+
+static const struct hwmon_channel_info *smpmpro_info[] = {
+	&smpmpro_temp,
+	&smpmpro_in,
+	&smpmpro_power,
+	NULL
+};
+
+static const struct hwmon_ops smpmpro_hwmon_ops = {
+	.is_visible = smpmpro_is_visible,
+	.read = smpmpro_read,
+	.write = smpmpro_write,
+};
+
+static const struct hwmon_chip_info smpmpro_chip_info = {
+	.ops = &smpmpro_hwmon_ops,
+	.info = smpmpro_info,
+};
+
+static SENSOR_DEVICE_ATTR(temp1_label, 0444, show_label, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp2_label, 0444, show_label, NULL, 1);
+static SENSOR_DEVICE_ATTR(temp3_label, 0444, show_label, NULL, 2);
+static SENSOR_DEVICE_ATTR(temp4_label, 0444, show_label, NULL, 5);
+static SENSOR_DEVICE_ATTR(temp5_label, 0444, show_label, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp6_label, 0444, show_label, NULL, 7);
+static SENSOR_DEVICE_ATTR(temp7_label, 0444, show_label, NULL, 8);
+static SENSOR_DEVICE_ATTR(temp8_label, 0444, show_label, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp9_label, 0444, show_label, NULL, 10);
+static SENSOR_DEVICE_ATTR(temp10_label, 0444, show_label, NULL, 11);
+static SENSOR_DEVICE_ATTR(temp11_label, 0444, show_label, NULL, 12);
+static SENSOR_DEVICE_ATTR(temp12_label, 0444, show_label, NULL, 13);
+static SENSOR_DEVICE_ATTR(temp13_label, 0444, show_label, NULL, 14);
+static SENSOR_DEVICE_ATTR(temp14_label, 0444, show_label, NULL, 15);
+
+static SENSOR_DEVICE_ATTR(in0_label, 0444, show_label, NULL, 5);
+static SENSOR_DEVICE_ATTR(in1_label, 0444, show_label, NULL, 1);
+static SENSOR_DEVICE_ATTR(in2_label, 0444, show_label, NULL, 3);
+static SENSOR_DEVICE_ATTR(in3_label, 0444, show_label, NULL, 4);
+
+static SENSOR_DEVICE_ATTR(power1_label, 0444, show_label, NULL, 5);
+static SENSOR_DEVICE_ATTR(power2_label, 0444, show_label, NULL, 1);
+static SENSOR_DEVICE_ATTR(power3_label, 0444, show_label, NULL, 3);
+static SENSOR_DEVICE_ATTR(power4_label, 0444, show_label, NULL, 4);
+static SENSOR_DEVICE_ATTR(power5_label, 0444, show_label, NULL, 16);
+static SENSOR_DEVICE_ATTR(power6_label, 0444, show_label, NULL, 2);
+
+static SENSOR_DEVICE_ATTR(gpi0_label, 0444, show_gpi_label, NULL, 0);
+static SENSOR_DEVICE_ATTR(gpi0_input, 0644, smpmpro_read_gpi, smpmpro_write_gpi, GPI_CTRL0_REG);
+static SENSOR_DEVICE_ATTR(gpi1_label, 0444, show_gpi_label, NULL, 1);
+static SENSOR_DEVICE_ATTR(gpi1_input, 0644, smpmpro_read_gpi, smpmpro_write_gpi, GPI_CTRL1_REG);
+static SENSOR_DEVICE_ATTR(gpi2_label, 0444, show_gpi_label, NULL, 2);
+static SENSOR_DEVICE_ATTR(gpi2_input, 0644, smpmpro_read_gpi, smpmpro_write_gpi, GPI_CTRL2_REG);
+static SENSOR_DEVICE_ATTR(gpi3_label, 0444, show_gpi_label, NULL, 3);
+static SENSOR_DEVICE_ATTR(gpi3_input, 0644, smpmpro_read_gpi, smpmpro_write_gpi, GPI_CTRL3_REG);
+static SENSOR_DEVICE_ATTR(gpi4_label, 0444, show_gpi_label, NULL, 4);
+static SENSOR_DEVICE_ATTR(gpi4_input, 0644, smpmpro_read_gpi,
+	       smpmpro_write_gpi, GPI_CE_UE_MASK_REG);
+static SENSOR_DEVICE_ATTR(gpi5_label, 0444, show_gpi_label, NULL, 5);
+static SENSOR_DEVICE_ATTR(gpi5_input, 0444, smpmpro_read_gpi, NULL, GPI_DATA_SET_REG);
+static SENSOR_DEVICE_ATTR(gpi6_label, 0444, show_gpi_label, NULL, 6);
+static SENSOR_DEVICE_ATTR(gpi6_input, 0444, smpmpro_read_gpi, NULL, GPI_DATA_SET0_REG);
+static SENSOR_DEVICE_ATTR(gpi7_label, 0444, show_gpi_label, NULL, 7);
+static SENSOR_DEVICE_ATTR(gpi7_input, 0444, smpmpro_read_gpi, NULL, GPI_DATA_SET1_REG);
+static SENSOR_DEVICE_ATTR(gpi8_label, 0444, show_gpi_label, NULL, 8);
+static SENSOR_DEVICE_ATTR(gpi8_input, 0444, smpmpro_read_gpi, NULL, GPI_DATA_SET2_REG);
+static SENSOR_DEVICE_ATTR(gpi9_label, 0444, show_gpi_label, NULL, 9);
+static SENSOR_DEVICE_ATTR(gpi9_input, 0444, smpmpro_read_gpi, NULL, GPI_DATA_SET3_REG);
+static SENSOR_DEVICE_ATTR(gpi10_label, 0444, show_gpi_label, NULL, 10);
+static SENSOR_DEVICE_ATTR(gpi10_input, 0444, smpmpro_read_gpi, NULL, GPI_CLUSTER_ERR_SET0_REG);
+static SENSOR_DEVICE_ATTR(gpi11_label, 0444, show_gpi_label, NULL, 11);
+static SENSOR_DEVICE_ATTR(gpi11_input, 0444, smpmpro_read_gpi, NULL, GPI_CLUSTER_ERR_SEL_REG);
+static SENSOR_DEVICE_ATTR(gpi12_label, 0444, show_gpi_label, NULL, 12);
+static SENSOR_DEVICE_ATTR(gpi12_input, 0444, smpmpro_read_gpi, NULL, GPI_MCU_ERR_REG);
+static SENSOR_DEVICE_ATTR(gpi13_label, 0444, show_gpi_label, NULL, 13);
+static SENSOR_DEVICE_ATTR(gpi13_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_PCIE_ERR_REG);
+static SENSOR_DEVICE_ATTR(gpi14_label, 0444, show_gpi_label, NULL, 14);
+static SENSOR_DEVICE_ATTR(gpi14_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_SYS_CACHE_ERR_SEL_REG);
+static SENSOR_DEVICE_ATTR(gpi15_label, 0444, show_gpi_label, NULL, 15);
+static SENSOR_DEVICE_ATTR(gpi15_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_SYS_CACHE_ERR_REG);
+static SENSOR_DEVICE_ATTR(gpi16_label, 0444, show_gpi_label, NULL, 16);
+static SENSOR_DEVICE_ATTR(gpi16_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_PCIE_ERR_SEL_REG);
+static SENSOR_DEVICE_ATTR(gpi17_label, 0444, show_gpi_label, NULL, 17);
+static SENSOR_DEVICE_ATTR(gpi17_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_VRD_FAULT_ERR_REG);
+static SENSOR_DEVICE_ATTR(gpi18_label, 0444, show_gpi_label, NULL, 18);
+static SENSOR_DEVICE_ATTR(gpi18_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_VRD_HOT_ERR_REG);
+static SENSOR_DEVICE_ATTR(gpi19_label, 0444, show_gpi_label, NULL, 19);
+static SENSOR_DEVICE_ATTR(gpi19_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_DIMM_HOT_ERR_REG);
+static SENSOR_DEVICE_ATTR(gpi20_label, 0444, show_gpi_label, NULL, 20);
+static SENSOR_DEVICE_ATTR(gpi20_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_BOOT_ERR1_REG);
+static SENSOR_DEVICE_ATTR(gpi21_label, 0444, show_gpi_label, NULL, 21);
+static SENSOR_DEVICE_ATTR(gpi21_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_BOOT_ERR2_REG);
+static SENSOR_DEVICE_ATTR(gpi22_label, 0444, show_gpi_label, NULL, 22);
+static SENSOR_DEVICE_ATTR(gpi22_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_WDT_STS_REG);
+static SENSOR_DEVICE_ATTR(gpi23_label, 0444, show_gpi_label, NULL, 23);
+static SENSOR_DEVICE_ATTR(gpi23_input, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_REG);
+
+static SENSOR_DEVICE_ATTR(gpi_core_cluster_sel, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, GPI_CORE_CLUSTER_SEL_REG);
+static SENSOR_DEVICE_ATTR(gpi_core_l1_err, 0444,
+	smpmpro_read_gpi, NULL, GPI_CORE_L1_ERR_REG);
+static SENSOR_DEVICE_ATTR(gpi_core_l2_err, 0444,
+	smpmpro_read_gpi, NULL, GPI_CORE_L2_ERR_REG);
+static SENSOR_DEVICE_ATTR(gpi_sys_cache_inst_sel, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, GPI_SYS_CACHE_INST_SEL_REG);
+static SENSOR_DEVICE_ATTR(gpi_sys_cache_err, 0444,
+	smpmpro_read_gpi, NULL, GPI_SYS_CACHE_ERR_REG);
+
+static SENSOR_DEVICE_ATTR(gpi_mcu_dimm_select, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, GPI_MCU_DIMM_SELECT_REG);
+static SENSOR_DEVICE_ATTR(gpi_mcu_dimm_err, 0444,
+	smpmpro_read_gpi, NULL, GPI_MCU_DIMM_ERR_REG);
+
+static SENSOR_DEVICE_ATTR(gpi_ras_err_smpro_type, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_SMPRO_TYPE_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_pmpro_type, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_PMPRO_TYPE_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_smpro_info_lo, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_SMPRO_INFO_LO_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_smpro_info_hi, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_SMPRO_INFO_HI_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_smpro_data_lo, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_SMPRO_DATA_LO_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_smpro_data_hi, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_SMPRO_DATA_HI_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_warn_smpro_info_lo, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_WARN_SMPRO_INFO_LO_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_warn_smpro_info_hi, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_WARN_SMPRO_INFO_HI_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_pmpro_info_lo, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_PMPRO_INFO_LO_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_pmpro_info_hi, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_PMPRO_INFO_HI_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_pmpro_data_lo, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_PMPRO_DATA_LO_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_err_pmpro_data_hi, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_ERR_PMPRO_DATA_HI_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_warn_pmpro_info_lo, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_WARN_PMPRO_INFO_LO_REG);
+static SENSOR_DEVICE_ATTR(gpi_ras_warn_pmpro_info_hi, 0444,
+	smpmpro_read_gpi, NULL, GPI_RAS_WARN_PMPRO_INFO_HI_REG);
+
+static SENSOR_DEVICE_ATTR(gpi_boot_stage_select, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, GPI_BOOT_STAGE_SELECT_REG);
+static SENSOR_DEVICE_ATTR(gpi_boot_stage_status_lo, 0444,
+	smpmpro_read_gpi, NULL, GPI_BOOT_STAGE_STATUS_LO_REG);
+static SENSOR_DEVICE_ATTR(gpi_boot_stage_cur_stage, 0444,
+	smpmpro_read_gpi, NULL, GPI_BOOT_STAGE_CUR_STAGE_REG);
+
+static SENSOR_DEVICE_ATTR(gpi_pcie_err_sel, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, GPI_PCIE_ERR_SEL_REG);
+static SENSOR_DEVICE_ATTR(gpi_pcie_err_type, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, GPI_PCIE_ERR_TYPE_REG);
+static SENSOR_DEVICE_ATTR(gpi_pcie_err_device, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, GPI_PCIE_ERR_DEVICE_REG);
+
+static SENSOR_DEVICE_ATTR(other_ce_err_cnt, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, OTHER_CE_ERR_CNT_REG);
+static SENSOR_DEVICE_ATTR(other_ce_err_len, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, OTHER_CE_ERR_LEN_REG);
+static SENSOR_DEVICE_ATTR(other_ce_err_data, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, OTHER_CE_ERR_DATA_REG);
+static SENSOR_DEVICE_ATTR(other_ue_err_cnt, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, OTHER_UE_ERR_CNT_REG);
+static SENSOR_DEVICE_ATTR(other_ue_err_len, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, OTHER_UE_ERR_LEN_REG);
+static SENSOR_DEVICE_ATTR(other_ue_err_data, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, OTHER_UE_ERR_DATA_REG);
+
+static SENSOR_DEVICE_ATTR(acpi_system_state, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, ACPI_SYSTEM_STATE_REG);
+static SENSOR_DEVICE_ATTR(acpi_cppc_cluster_sel, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, ACPI_CPPC_CLUSTER_SEL_REG);
+static SENSOR_DEVICE_ATTR(acpi_cppc_cluster_data, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, ACPI_CPPC_CLUSTER_DATA_REG);
+static SENSOR_DEVICE_ATTR(acpi_power_limit, 0644,
+	smpmpro_read_gpi, smpmpro_write_gpi, ACPI_POWER_LIMIT_REG);
+
+static struct attribute *smpmpro_attrs[] = {
+	&sensor_dev_attr_gpi0_label.dev_attr.attr,
+	&sensor_dev_attr_gpi0_input.dev_attr.attr,
+	&sensor_dev_attr_gpi1_label.dev_attr.attr,
+	&sensor_dev_attr_gpi1_input.dev_attr.attr,
+	&sensor_dev_attr_gpi2_label.dev_attr.attr,
+	&sensor_dev_attr_gpi2_input.dev_attr.attr,
+	&sensor_dev_attr_gpi3_label.dev_attr.attr,
+	&sensor_dev_attr_gpi3_input.dev_attr.attr,
+	&sensor_dev_attr_gpi4_label.dev_attr.attr,
+	&sensor_dev_attr_gpi4_input.dev_attr.attr,
+	&sensor_dev_attr_gpi5_label.dev_attr.attr,
+	&sensor_dev_attr_gpi5_input.dev_attr.attr,
+	&sensor_dev_attr_gpi6_label.dev_attr.attr,
+	&sensor_dev_attr_gpi6_input.dev_attr.attr,
+	&sensor_dev_attr_gpi7_label.dev_attr.attr,
+	&sensor_dev_attr_gpi7_input.dev_attr.attr,
+	&sensor_dev_attr_gpi8_label.dev_attr.attr,
+	&sensor_dev_attr_gpi8_input.dev_attr.attr,
+	&sensor_dev_attr_gpi9_label.dev_attr.attr,
+	&sensor_dev_attr_gpi9_input.dev_attr.attr,
+	&sensor_dev_attr_gpi10_label.dev_attr.attr,
+	&sensor_dev_attr_gpi10_input.dev_attr.attr,
+	&sensor_dev_attr_gpi11_label.dev_attr.attr,
+	&sensor_dev_attr_gpi11_input.dev_attr.attr,
+	&sensor_dev_attr_gpi12_label.dev_attr.attr,
+	&sensor_dev_attr_gpi12_input.dev_attr.attr,
+	&sensor_dev_attr_gpi13_label.dev_attr.attr,
+	&sensor_dev_attr_gpi13_input.dev_attr.attr,
+	&sensor_dev_attr_gpi14_label.dev_attr.attr,
+	&sensor_dev_attr_gpi14_input.dev_attr.attr,
+	&sensor_dev_attr_gpi15_label.dev_attr.attr,
+	&sensor_dev_attr_gpi15_input.dev_attr.attr,
+	&sensor_dev_attr_gpi16_label.dev_attr.attr,
+	&sensor_dev_attr_gpi16_input.dev_attr.attr,
+	&sensor_dev_attr_gpi17_label.dev_attr.attr,
+	&sensor_dev_attr_gpi17_input.dev_attr.attr,
+	&sensor_dev_attr_gpi18_label.dev_attr.attr,
+	&sensor_dev_attr_gpi18_input.dev_attr.attr,
+	&sensor_dev_attr_gpi19_label.dev_attr.attr,
+	&sensor_dev_attr_gpi19_input.dev_attr.attr,
+	&sensor_dev_attr_gpi20_label.dev_attr.attr,
+	&sensor_dev_attr_gpi20_input.dev_attr.attr,
+	&sensor_dev_attr_gpi21_label.dev_attr.attr,
+	&sensor_dev_attr_gpi21_input.dev_attr.attr,
+	&sensor_dev_attr_gpi22_label.dev_attr.attr,
+	&sensor_dev_attr_gpi22_input.dev_attr.attr,
+	&sensor_dev_attr_gpi23_label.dev_attr.attr,
+	&sensor_dev_attr_gpi23_input.dev_attr.attr,
+
+	&sensor_dev_attr_temp1_label.dev_attr.attr,
+	&sensor_dev_attr_temp2_label.dev_attr.attr,
+	&sensor_dev_attr_temp3_label.dev_attr.attr,
+	&sensor_dev_attr_temp4_label.dev_attr.attr,
+	&sensor_dev_attr_temp5_label.dev_attr.attr,
+	&sensor_dev_attr_temp6_label.dev_attr.attr,
+	&sensor_dev_attr_temp7_label.dev_attr.attr,
+	&sensor_dev_attr_temp8_label.dev_attr.attr,
+	&sensor_dev_attr_temp9_label.dev_attr.attr,
+	&sensor_dev_attr_temp10_label.dev_attr.attr,
+	&sensor_dev_attr_temp11_label.dev_attr.attr,
+	&sensor_dev_attr_temp12_label.dev_attr.attr,
+	&sensor_dev_attr_temp13_label.dev_attr.attr,
+	&sensor_dev_attr_temp14_label.dev_attr.attr,
+
+	&sensor_dev_attr_in0_label.dev_attr.attr,
+	&sensor_dev_attr_in1_label.dev_attr.attr,
+	&sensor_dev_attr_in2_label.dev_attr.attr,
+	&sensor_dev_attr_in3_label.dev_attr.attr,
+
+	&sensor_dev_attr_power1_label.dev_attr.attr,
+	&sensor_dev_attr_power2_label.dev_attr.attr,
+	&sensor_dev_attr_power3_label.dev_attr.attr,
+	&sensor_dev_attr_power4_label.dev_attr.attr,
+	&sensor_dev_attr_power5_label.dev_attr.attr,
+	&sensor_dev_attr_power6_label.dev_attr.attr,
+
+	&sensor_dev_attr_gpi_core_cluster_sel.dev_attr.attr,
+	&sensor_dev_attr_gpi_core_l1_err.dev_attr.attr,
+	&sensor_dev_attr_gpi_core_l2_err.dev_attr.attr,
+	&sensor_dev_attr_gpi_sys_cache_inst_sel.dev_attr.attr,
+	&sensor_dev_attr_gpi_sys_cache_err.dev_attr.attr,
+
+	&sensor_dev_attr_gpi_mcu_dimm_select.dev_attr.attr,
+	&sensor_dev_attr_gpi_mcu_dimm_err.dev_attr.attr,
+
+	&sensor_dev_attr_gpi_ras_err_smpro_type.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_pmpro_type.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_smpro_info_lo.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_smpro_info_hi.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_smpro_data_lo.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_smpro_data_hi.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_warn_smpro_info_lo.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_warn_smpro_info_hi.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_pmpro_info_lo.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_pmpro_info_hi.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_pmpro_data_lo.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_err_pmpro_data_hi.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_warn_pmpro_info_lo.dev_attr.attr,
+	&sensor_dev_attr_gpi_ras_warn_pmpro_info_hi.dev_attr.attr,
+
+	&sensor_dev_attr_gpi_boot_stage_select.dev_attr.attr,
+	&sensor_dev_attr_gpi_boot_stage_status_lo.dev_attr.attr,
+	&sensor_dev_attr_gpi_boot_stage_cur_stage.dev_attr.attr,
+
+	&sensor_dev_attr_gpi_pcie_err_sel.dev_attr.attr,
+	&sensor_dev_attr_gpi_pcie_err_type.dev_attr.attr,
+	&sensor_dev_attr_gpi_pcie_err_device.dev_attr.attr,
+
+	&sensor_dev_attr_other_ce_err_cnt.dev_attr.attr,
+	&sensor_dev_attr_other_ce_err_len.dev_attr.attr,
+	&sensor_dev_attr_other_ce_err_data.dev_attr.attr,
+	&sensor_dev_attr_other_ue_err_cnt.dev_attr.attr,
+	&sensor_dev_attr_other_ue_err_len.dev_attr.attr,
+	&sensor_dev_attr_other_ue_err_data.dev_attr.attr,
+
+	&sensor_dev_attr_acpi_system_state.dev_attr.attr,
+	&sensor_dev_attr_acpi_cppc_cluster_sel.dev_attr.attr,
+	&sensor_dev_attr_acpi_cppc_cluster_data.dev_attr.attr,
+	&sensor_dev_attr_acpi_power_limit.dev_attr.attr,
+
+	NULL
+};
+ATTRIBUTE_GROUPS(smpmpro);
+
+static int smpmpro_i2c_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	struct device *dev = &client->dev;
+	struct smpmpro_data *data;
+	struct device *hwmon_dev;
+
+	if (!i2c_check_functionality(adapter,
+			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	data = devm_kzalloc(dev, sizeof(struct smpmpro_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+
+	/* Initialize the Altra SMPMPro chip */
+	smpmpro_init_device(client, data);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data,
+							 &smpmpro_chip_info,
+							 smpmpro_groups);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id smpmpro_i2c_id[] = {
+	{ "smpmpro", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, smpmpro_i2c_id);
+
+static struct i2c_driver smpmpro_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.probe		= smpmpro_i2c_probe,
+	.driver = {
+		.name	= "smpmpro",
+	},
+	.id_table	= smpmpro_i2c_id,
+};
+
+module_i2c_driver(smpmpro_driver);
+
+MODULE_AUTHOR("Thinh Pham <thinh.pham@amperecomputing.com>");
+MODULE_AUTHOR("Dien Nguyen <dinguyen@amperecomputing.com>");
+MODULE_AUTHOR("Hoang Nguyen <hnguyen@amperecomputing.com>");
+MODULE_DESCRIPTION("Altra SMPMPro driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH linux dev-5.8] hwmon: Ampere Computing ALTRA SMPMPRO sensor driver
  2020-09-30 22:26 [PATCH linux dev-5.8] hwmon: Ampere Computing ALTRA SMPMPRO sensor driver Lancelot
@ 2020-10-01 12:32 ` Patrick Williams
  2020-10-05 23:05   ` Joel Stanley
  0 siblings, 1 reply; 4+ messages in thread
From: Patrick Williams @ 2020-10-01 12:32 UTC (permalink / raw)
  To: Lancelot; +Cc: openbmc, Xiaopeng XP Chen


[-- Attachment #1: Type: text/plain, Size: 3441 bytes --]

On Wed, Sep 30, 2020 at 05:26:57PM -0500, Lancelot wrote:
> From: Lancelot Kao <lancelot.kao@fii-usa.com>
> 
> Add SMPMPro-hwmon driver to monitor Ampere CPU/Memory/VR via an
> i2c interface of the CPU's smpmpro management device.
> 
> Signed-off-by: Xiaopeng XP Chen <xiao-peng.chen@fii-na.com>
> Signed-off-by: Lancelot Kao <lancelot.kao@fii-usa.com>

Nice work at adding this driver.

It does look like you've missed CC'ing upstream though.  Was this
intentional?  (linux-hwmon@vger.kernel.org)

> +/* Capability Registers  */
> +#define TEMP_SENSOR_SUPPORT_REG	0x05
> +#define PWR_SENSOR_SUPPORT_REG	0x06
> +#define VOLT_SENSOR_SUPPORT_REG	0x07
> +#define OTHER_CAP_REG		    0x08
> +#define CORE_CLUSTER_CNT_REG	0x0B
> +#define SYS_CACHE_PCIE_CNT_REG	0x0C
> +#define SOCKET_INFO_REG	        0x0D

There seems to be some sporatic indentation throughout all the #defines
in this file, where it appears you attempted to align the values.  Make
sure you have tabs set to 8-step spacing for kernel code.

> +static void smpmpro_init_device(struct i2c_client *client,
> +				struct smpmpro_data *data)
> +{
> +	u16 ret;
> +
> +	ret = i2c_smbus_read_word_swapped(client, TEMP_SENSOR_SUPPORT_REG);
> +	if (ret < 0)
> +		return;
> +	data->temp_support_regs = ret;

i2c_smbus_read_word_swapped returns a s32 even though you're looking for
a u16.  By setting `ret` to u16 you've caused two problems:

    * You are immediately truncating -ERRNO values into a u16 so that
      you are unable to differentiate values like 0xFFFFFFFF as a
      register value and -1 as an errno.

    * The if condition here can never be true, so you're never catching
      error conditions.  (An u16 can never be negative, so ret < 0 can
      never be true.)

This issue occurs throughout the driver.

> +static int smpmpro_read_temp(struct device *dev, u32 attr, int channel,
> +			     long *val)
> +{
> +	struct smpmpro_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret;

You might want a sized int on this one?  Repeated in most other
functions.

> +static int smpmpro_read_power(struct device *dev, u32 attr, int channel,
> +			     long *val)
> +{
> +	struct smpmpro_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret, ret_mw;
> +	int ret2 = 0, ret2_mw = 0;

Any reason to not initialize ret/ret_mw?  By it being different from
ret2/ret2_mw it makes me question "is this ok?", which spends more time
in review.

> +static int smpmpro_i2c_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
...
> +	/* Initialize the Altra SMPMPro chip */
> +	smpmpro_init_device(client, data);

I didn't see anything in the smpmpro_init_device function, but is there
anything you can or should do to ensure this device really is an
SMPMPro rather than exclusively relying on the device tree compatible?

> +static struct i2c_driver smpmpro_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.probe		= smpmpro_i2c_probe,
> +	.driver = {
> +		.name	= "smpmpro",
> +	},
> +	.id_table	= smpmpro_i2c_id,
> +};
> +
> +module_i2c_driver(smpmpro_driver);

Are you missing the .of_match_table inside .driver?  Is that necessary
or useful for your use?  I'm not sure if you can have device tree
entries that automatically instantiate the hwmon driver otherwise.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH linux dev-5.8] hwmon: Ampere Computing ALTRA SMPMPRO sensor driver
  2020-10-01 12:32 ` Patrick Williams
@ 2020-10-05 23:05   ` Joel Stanley
  2020-10-06  3:13     ` Guenter Roeck
  0 siblings, 1 reply; 4+ messages in thread
From: Joel Stanley @ 2020-10-05 23:05 UTC (permalink / raw)
  To: Patrick Williams, linux-hwmon
  Cc: OpenBMC Maillist, Lancelot, Xiaopeng XP Chen

On Thu, 1 Oct 2020 at 12:32, Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Wed, Sep 30, 2020 at 05:26:57PM -0500, Lancelot wrote:
> > From: Lancelot Kao <lancelot.kao@fii-usa.com>
> >
> > Add SMPMPro-hwmon driver to monitor Ampere CPU/Memory/VR via an
> > i2c interface of the CPU's smpmpro management device.
> >
> > Signed-off-by: Xiaopeng XP Chen <xiao-peng.chen@fii-na.com>
> > Signed-off-by: Lancelot Kao <lancelot.kao@fii-usa.com>
>
> Nice work at adding this driver.
>
> It does look like you've missed CC'ing upstream though.  Was this
> intentional?  (linux-hwmon@vger.kernel.org)

As Patrick mentioned, let's review this on the upstream list.

Cheers,

Joel

>
> > +/* Capability Registers  */
> > +#define TEMP_SENSOR_SUPPORT_REG      0x05
> > +#define PWR_SENSOR_SUPPORT_REG       0x06
> > +#define VOLT_SENSOR_SUPPORT_REG      0x07
> > +#define OTHER_CAP_REG                    0x08
> > +#define CORE_CLUSTER_CNT_REG 0x0B
> > +#define SYS_CACHE_PCIE_CNT_REG       0x0C
> > +#define SOCKET_INFO_REG              0x0D
>
> There seems to be some sporatic indentation throughout all the #defines
> in this file, where it appears you attempted to align the values.  Make
> sure you have tabs set to 8-step spacing for kernel code.
>
> > +static void smpmpro_init_device(struct i2c_client *client,
> > +                             struct smpmpro_data *data)
> > +{
> > +     u16 ret;
> > +
> > +     ret = i2c_smbus_read_word_swapped(client, TEMP_SENSOR_SUPPORT_REG);
> > +     if (ret < 0)
> > +             return;
> > +     data->temp_support_regs = ret;
>
> i2c_smbus_read_word_swapped returns a s32 even though you're looking for
> a u16.  By setting `ret` to u16 you've caused two problems:
>
>     * You are immediately truncating -ERRNO values into a u16 so that
>       you are unable to differentiate values like 0xFFFFFFFF as a
>       register value and -1 as an errno.
>
>     * The if condition here can never be true, so you're never catching
>       error conditions.  (An u16 can never be negative, so ret < 0 can
>       never be true.)
>
> This issue occurs throughout the driver.
>
> > +static int smpmpro_read_temp(struct device *dev, u32 attr, int channel,
> > +                          long *val)
> > +{
> > +     struct smpmpro_data *data = dev_get_drvdata(dev);
> > +     struct i2c_client *client = data->client;
> > +     int ret;
>
> You might want a sized int on this one?  Repeated in most other
> functions.
>
> > +static int smpmpro_read_power(struct device *dev, u32 attr, int channel,
> > +                          long *val)
> > +{
> > +     struct smpmpro_data *data = dev_get_drvdata(dev);
> > +     struct i2c_client *client = data->client;
> > +     int ret, ret_mw;
> > +     int ret2 = 0, ret2_mw = 0;
>
> Any reason to not initialize ret/ret_mw?  By it being different from
> ret2/ret2_mw it makes me question "is this ok?", which spends more time
> in review.
>
> > +static int smpmpro_i2c_probe(struct i2c_client *client,
> > +                       const struct i2c_device_id *id)
> ...
> > +     /* Initialize the Altra SMPMPro chip */
> > +     smpmpro_init_device(client, data);
>
> I didn't see anything in the smpmpro_init_device function, but is there
> anything you can or should do to ensure this device really is an
> SMPMPro rather than exclusively relying on the device tree compatible?
>
> > +static struct i2c_driver smpmpro_driver = {
> > +     .class          = I2C_CLASS_HWMON,
> > +     .probe          = smpmpro_i2c_probe,
> > +     .driver = {
> > +             .name   = "smpmpro",
> > +     },
> > +     .id_table       = smpmpro_i2c_id,
> > +};
> > +
> > +module_i2c_driver(smpmpro_driver);
>
> Are you missing the .of_match_table inside .driver?  Is that necessary
> or useful for your use?  I'm not sure if you can have device tree
> entries that automatically instantiate the hwmon driver otherwise.
>
> --
> Patrick Williams

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH linux dev-5.8] hwmon: Ampere Computing ALTRA SMPMPRO sensor driver
  2020-10-05 23:05   ` Joel Stanley
@ 2020-10-06  3:13     ` Guenter Roeck
  0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2020-10-06  3:13 UTC (permalink / raw)
  To: Joel Stanley, Patrick Williams, linux-hwmon
  Cc: OpenBMC Maillist, Lancelot, Xiaopeng XP Chen

On 10/5/20 4:05 PM, Joel Stanley wrote:
> On Thu, 1 Oct 2020 at 12:32, Patrick Williams <patrick@stwcx.xyz> wrote:
>>
>> On Wed, Sep 30, 2020 at 05:26:57PM -0500, Lancelot wrote:
>>> From: Lancelot Kao <lancelot.kao@fii-usa.com>
>>>
>>> Add SMPMPro-hwmon driver to monitor Ampere CPU/Memory/VR via an
>>> i2c interface of the CPU's smpmpro management device.
>>>
>>> Signed-off-by: Xiaopeng XP Chen <xiao-peng.chen@fii-na.com>
>>> Signed-off-by: Lancelot Kao <lancelot.kao@fii-usa.com>
>>
>> Nice work at adding this driver.
>>
>> It does look like you've missed CC'ing upstream though.  Was this
>> intentional?  (linux-hwmon@vger.kernel.org)
> 
> As Patrick mentioned, let's review this on the upstream list.
> 

I can not really comment, not having seen the entire patch.
However, looking it up on the OpenBMC patchwork, couple of
high level comments:

- Label attributes are handled by the hwmon core. Label attributes
  outside the core are unacceptable.
- There is no discussion about the non-standard attributes, nor a description
  of those, nor an explanation for why they are needed (as hwmon sysfs attributes)
  or what they report. This is unacceptable.
  Besides, many of those attributes - say, gpi22_input, which seems to report
  the content of GPI_WDT_STS_REG, suggesting association with a watchdog -
  seem inappropriate for a hwmon driver to start with. It seems like the
  hwmon driver us used as catch-all driver for this chip. Sorry,
  that is an absolute no-go.

If this patch is supposed to be submitted as upstream patch, I would suggest
to read and follow the guidance in Documentation/hwmon/submitting-patches.rst.

Guenter

> Cheers,
> 
> Joel
> 
>>
>>> +/* Capability Registers  */
>>> +#define TEMP_SENSOR_SUPPORT_REG      0x05
>>> +#define PWR_SENSOR_SUPPORT_REG       0x06
>>> +#define VOLT_SENSOR_SUPPORT_REG      0x07
>>> +#define OTHER_CAP_REG                    0x08
>>> +#define CORE_CLUSTER_CNT_REG 0x0B
>>> +#define SYS_CACHE_PCIE_CNT_REG       0x0C
>>> +#define SOCKET_INFO_REG              0x0D
>>
>> There seems to be some sporatic indentation throughout all the #defines
>> in this file, where it appears you attempted to align the values.  Make
>> sure you have tabs set to 8-step spacing for kernel code.
>>
>>> +static void smpmpro_init_device(struct i2c_client *client,
>>> +                             struct smpmpro_data *data)
>>> +{
>>> +     u16 ret;
>>> +
>>> +     ret = i2c_smbus_read_word_swapped(client, TEMP_SENSOR_SUPPORT_REG);
>>> +     if (ret < 0)
>>> +             return;
>>> +     data->temp_support_regs = ret;
>>
>> i2c_smbus_read_word_swapped returns a s32 even though you're looking for
>> a u16.  By setting `ret` to u16 you've caused two problems:
>>
>>     * You are immediately truncating -ERRNO values into a u16 so that
>>       you are unable to differentiate values like 0xFFFFFFFF as a
>>       register value and -1 as an errno.
>>
>>     * The if condition here can never be true, so you're never catching
>>       error conditions.  (An u16 can never be negative, so ret < 0 can
>>       never be true.)
>>
>> This issue occurs throughout the driver.
>>
>>> +static int smpmpro_read_temp(struct device *dev, u32 attr, int channel,
>>> +                          long *val)
>>> +{
>>> +     struct smpmpro_data *data = dev_get_drvdata(dev);
>>> +     struct i2c_client *client = data->client;
>>> +     int ret;
>>
>> You might want a sized int on this one?  Repeated in most other
>> functions.
>>
>>> +static int smpmpro_read_power(struct device *dev, u32 attr, int channel,
>>> +                          long *val)
>>> +{
>>> +     struct smpmpro_data *data = dev_get_drvdata(dev);
>>> +     struct i2c_client *client = data->client;
>>> +     int ret, ret_mw;
>>> +     int ret2 = 0, ret2_mw = 0;
>>
>> Any reason to not initialize ret/ret_mw?  By it being different from
>> ret2/ret2_mw it makes me question "is this ok?", which spends more time
>> in review.
>>
>>> +static int smpmpro_i2c_probe(struct i2c_client *client,
>>> +                       const struct i2c_device_id *id)
>> ...
>>> +     /* Initialize the Altra SMPMPro chip */
>>> +     smpmpro_init_device(client, data);
>>
>> I didn't see anything in the smpmpro_init_device function, but is there
>> anything you can or should do to ensure this device really is an
>> SMPMPro rather than exclusively relying on the device tree compatible?
>>
>>> +static struct i2c_driver smpmpro_driver = {
>>> +     .class          = I2C_CLASS_HWMON,
>>> +     .probe          = smpmpro_i2c_probe,
>>> +     .driver = {
>>> +             .name   = "smpmpro",
>>> +     },
>>> +     .id_table       = smpmpro_i2c_id,
>>> +};
>>> +
>>> +module_i2c_driver(smpmpro_driver);
>>
>> Are you missing the .of_match_table inside .driver?  Is that necessary
>> or useful for your use?  I'm not sure if you can have device tree
>> entries that automatically instantiate the hwmon driver otherwise.
>>
>> --
>> Patrick Williams


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 22:26 [PATCH linux dev-5.8] hwmon: Ampere Computing ALTRA SMPMPRO sensor driver Lancelot
2020-10-01 12:32 ` Patrick Williams
2020-10-05 23:05   ` Joel Stanley
2020-10-06  3:13     ` Guenter Roeck

Openbmc archive at lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/openbmc/0 openbmc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 openbmc openbmc/ https://lore.kernel.org/openbmc \
		openbmc@lists.ozlabs.org openbmc@ozlabs.org
	public-inbox-index openbmc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.openbmc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git