[PATCH 0/4] Add Ampere's Altra SMPro hwmon driver

[PATCH 0/4] Add Ampere's Altra SMPro hwmon driver

[Quan Nguyen]

This patch series adds support for Ampere SMpro hwmon driver. This driver
supports accessing various CPU sensors provided by the SMpro co-processor
including temperature, power, voltages, and current found on Ampere
Altra processor family.

Quan Nguyen (4):
  dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers
  mfd: simple-mfd-i2c: Adds Ampere's Altra SMpro support
  hwmon: smpro: Add Ampere's Altra smpro-hwmon driver
  docs: hwmon: (smpro-hwmon) Add documentation

 .../bindings/hwmon/ampere,ac01-hwmon.yaml     |  27 +
 .../bindings/mfd/ampere,ac01-smpro.yaml       |  82 +++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/smpro-hwmon.rst           | 100 +++
 drivers/hwmon/Kconfig                         |   8 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/smpro-hwmon.c                   | 620 ++++++++++++++++++
 drivers/mfd/Kconfig                           |  10 +
 drivers/mfd/simple-mfd-i2c.c                  |  15 +-
 9 files changed, 862 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
 create mode 100644 Documentation/hwmon/smpro-hwmon.rst
 create mode 100644 drivers/hwmon/smpro-hwmon.c

-- 
2.28.0

[PATCH 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Quan Nguyen]

Adds device tree bindings for SMPro drivers found on the Mt.Jade hardware
reference platform with Ampere's Altra Processor family.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 .../bindings/hwmon/ampere,ac01-hwmon.yaml     | 27 ++++++
 .../bindings/mfd/ampere,ac01-smpro.yaml       | 82 +++++++++++++++++++
 2 files changed, 109 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
new file mode 100644
index 000000000000..d13862ba646b
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hardware monitoring driver for the Ampere Altra SMPro
+
+maintainers:
+  - Quan Nguyen <quan@os.amperecomputing.com>
+
+description: |
+  This module is part of the Ampere Altra SMPro multi-function device. For more
+  details see ../mfd/ampere,ac01-smpro.yaml.
+
+properties:
+  compatible:
+    enum:
+      - ampere,ac01-hwmon
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
new file mode 100644
index 000000000000..06b0239413ae
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ampere,ac01-smpro.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ampere Altra SMPro firmware driver
+
+maintainers:
+  - Quan Nguyen <quan@os.amperecomputing.com>
+
+description: |
+  Ampere Altra SMPro firmware may contain different blocks like hardware
+  monitoring, error monitoring and other miscellaneous features.
+
+properties:
+  compatible:
+    const: ampere,ac01-smpro
+
+  reg:
+    description:
+      I2C device address.
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^hwmon(@[0-9a-f]+)?$":
+    $ref: ../hwmon/ampere,ac01-hwmon.yaml
+
+  "^misc(@[0-9a-f]+)?$":
+    type: object
+    description: Ampere Altra SMPro Misc driver
+    properties:
+      compatible:
+        const: "ampere,ac01-misc"
+
+  "^errmon(@[0-9a-f]+)?$":
+    type: object
+    description: Ampere Altra SMPro Error Monitor driver
+    properties:
+      compatible:
+        const: "ampere,ac01-errmon"
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        smpro@4f {
+            compatible = "ampere,ac01-smpro";
+            reg = <0x4f>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            hwmon {
+                compatible = "ampere,ac01-hwmon";
+            };
+
+            misc {
+                compatible = "ampere,ac01-misc";
+            };
+
+            errmon {
+                compatible = "ampere,ac01-errmon";
+            };
+
+        };
+    };
-- 
2.28.0

[PATCH 2/4] mfd: simple-mfd-i2c: Adds Ampere's Altra SMpro support

[Quan Nguyen]

Adds an MFD driver for SMpro found on the Mt.Jade hardware reference
platform with Ampere's Altra processor family.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 drivers/mfd/Kconfig          | 10 ++++++++++
 drivers/mfd/simple-mfd-i2c.c | 15 +++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b74efa469e90..5414371bdea1 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -77,6 +77,16 @@ config MFD_AS3711
 	help
 	  Support for the AS3711 PMIC from AMS
 
+config MFD_SMPRO
+	tristate "Ampere Computing MFD SMpro core driver"
+	select MFD_SIMPLE_MFD_I2C
+	help
+	  Say yes here to enable SMpro driver support for Ampere's Altra
+	  processor family.
+
+	  Ampere's Altra SMpro exposes an I2C regmap interface that can
+	  be accessed by child devices.
+
 config MFD_AS3722
 	tristate "ams AS3722 Power Management IC"
 	select MFD_CORE
diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index 87f684cff9a1..0459a9fbd3f5 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -21,14 +21,24 @@ static const struct regmap_config simple_regmap_config = {
 	.val_bits = 8,
 };
 
+static const struct regmap_config simple_word_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+};
+
 static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 {
 	const struct regmap_config *config;
 	struct regmap *regmap;
 
 	config = device_get_match_data(&i2c->dev);
-	if (!config)
-		config = &simple_regmap_config;
+	if (!config) {
+		if (of_device_is_compatible(i2c->dev.of_node,
+						"ampere,ac01-smpro"))
+			config = &simple_word_regmap_config;
+		else
+			config = &simple_regmap_config;
+	}
 
 	regmap = devm_regmap_init_i2c(i2c, config);
 	if (IS_ERR(regmap))
@@ -39,6 +49,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 
 static const struct of_device_id simple_mfd_i2c_of_match[] = {
 	{ .compatible = "kontron,sl28cpld" },
+	{ .compatible = "ampere,ac01-smpro" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);
-- 
2.28.0

[PATCH 3/4] hwmon: smpro: Add Ampere's Altra smpro-hwmon driver

[Quan Nguyen]

This commit adds support for Ampere SMpro hwmon driver. This driver
supports accessing various CPU sensors provided by the SMpro co-processor
including temperature, power, voltages, and current.

Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 drivers/hwmon/Kconfig       |   8 +
 drivers/hwmon/Makefile      |   1 +
 drivers/hwmon/smpro-hwmon.c | 620 ++++++++++++++++++++++++++++++++++++
 3 files changed, 629 insertions(+)
 create mode 100644 drivers/hwmon/smpro-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 54f04e61fb83..7b0458bf903c 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -67,6 +67,14 @@ config SENSORS_ABITUGURU3
 	  This driver can also be built as a module. If so, the module
 	  will be called abituguru3.
 
+config SENSORS_SMPRO
+	tristate "Ampere's Altra SMpro hardware monitoring driver"
+	depends on MFD_SMPRO
+	help
+	  If you say yes here you get support for the thermal, voltage,
+	  current and power sensors of Ampere's Altra processor family SoC
+	  with SMpro co-processor.
+
 config SENSORS_AD7314
 	tristate "Analog Devices AD7314 and compatibles"
 	depends on SPI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index fe38e8a5c979..ac0892540abb 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -173,6 +173,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_SMPRO)	+= smpro-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/smpro-hwmon.c b/drivers/hwmon/smpro-hwmon.c
new file mode 100644
index 000000000000..d70764ffc78f
--- /dev/null
+++ b/drivers/hwmon/smpro-hwmon.c
@@ -0,0 +1,620 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Ampere Computing SoC's SMPro Hardware Monitoring Driver
+ *
+ * Copyright (c) 2021, 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/bitfield.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+/* Identification Registers */
+#define MANUFACTURER_ID_REG		0x02
+#define AMPERE_MANUFACTURER_ID		0xCD3A
+
+/* Capability Registers  */
+#define SOC_TDP_REG			0x0E
+
+/* 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 RCA_VRD_TEMP_REG		0x1C
+
+#define CORE_VRD_PWR_REG		0x20
+#define SOC_PWR_REG			0x21
+#define DIMM_VRD1_PWR_REG		0x22
+#define DIMM_VRD2_PWR_REG		0x23
+#define CORE_VRD_PWR_MW_REG		0x26
+#define SOC_PWR_MW_REG			0x27
+#define DIMM_VRD1_PWR_MW_REG		0x28
+#define DIMM_VRD2_PWR_MW_REG		0x29
+#define RCA_VRD_PWR_REG			0x2A
+#define RCA_VRD_PWR_MW_REG		0x2B
+
+#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 RCA_VRD_VOLT_REG		0x38
+
+#define CORE_VRD_CURR_REG		0x39
+#define SOC_VRD_CURR_REG		0x3A
+#define DIMM_VRD1_CURR_REG		0x3B
+#define DIMM_VRD2_CURR_REG		0x3C
+#define RCA_VRD_CURR_REG		0x3D
+
+/* Add for DIMM group */
+#define DIMM_GROUP_DUMMY_REG		0xFF
+
+struct smpro_hwmon {
+	struct regmap *regmap;
+};
+
+static const u8 temp_regs[] = {
+	SOC_TEMP_REG,
+	SOC_VRD_TEMP_REG,
+	DIMM_VRD_TEMP_REG,
+	CORE_VRD_TEMP_REG,
+	/* reserved for DIMM G0 */
+	DIMM_GROUP_DUMMY_REG,
+	CH0_DIMM_TEMP_REG,
+	CH1_DIMM_TEMP_REG,
+	CH2_DIMM_TEMP_REG,
+	CH3_DIMM_TEMP_REG,
+	/* reserved for DIMM G1 */
+	DIMM_GROUP_DUMMY_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,
+	RCA_VRD_TEMP_REG,
+};
+
+static const u8 volt_regs[] = {
+	CORE_VRD_VOLT_REG,
+	SOC_VRD_VOLT_REG,
+	DIMM_VRD1_VOLT_REG,
+	DIMM_VRD2_VOLT_REG,
+	/* vrd1 has higher priority than vrd2 using vrd1 as output for ddr */
+	DIMM_VRD1_VOLT_REG,
+	RCA_VRD_VOLT_REG,
+};
+
+static const u8 curr_regs[] = {
+	CORE_VRD_CURR_REG,
+	SOC_VRD_CURR_REG,
+	DIMM_VRD1_CURR_REG,
+	DIMM_VRD2_CURR_REG,
+	RCA_VRD_CURR_REG,
+};
+
+enum pwr_regs {
+	CORE_VRD_PWR,
+	SOC_PWR,
+	DIMM_VRD1_PWR,
+	DIMM_VRD2_PWR,
+	CPU_VRD_PWR,
+	DIMM_VRD_PWR,
+	RCA_VRD_PWR,
+	SOC_TDP_PWR,
+};
+static const char * const label[] = {
+	"SoC",
+	"SoC VRD",
+	"DIMM VRD",
+	"DIMM VRD1",
+	"DIMM VRD2",
+	"CORE VRD",
+	"CH0 DIMM",
+	"CH1 DIMM",
+	"CH2 DIMM",
+	"CH3 DIMM",
+	"CH4 DIMM",
+	"CH5 DIMM",
+	"CH6 DIMM",
+	"CH7 DIMM",
+	"MEM HOT",
+	"SoC VR HOT",
+	"CPU VRD",
+	"RCA VRD",
+	"SOC TDP",
+	"DIMM G0",
+	"DIMM G1",
+};
+
+static int smpro_read_temp(struct device *dev, u32 attr, int channel,
+				long *val)
+{
+	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int t_max = 0xffffffff;
+	unsigned int value;
+	s32 i;
+	int ret;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		if (temp_regs[channel] == DIMM_GROUP_DUMMY_REG) {
+			for (i = 1; i <= 4; i++) {
+				ret = regmap_read(hwmon->regmap,
+						temp_regs[channel + i], &value);
+				if (ret)
+					return ret;
+				/* continue if invalid */
+				if (value == 0xffff)
+					continue;
+
+				value &= 0x1ff; /* 9-bit value */
+				if (t_max != 0xffffffff)
+					t_max = (value > t_max) ? value : t_max;
+				else
+					t_max = value;
+			}
+
+			if (t_max == 0xffffffff)
+				return -1;
+
+			*val = t_max * 1000;
+		} else {
+			ret = regmap_read(hwmon->regmap,
+					temp_regs[channel], &value);
+			if (ret)
+				return ret;
+			*val = (value & 0x1ff) * 1000;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int smpro_read_in(struct device *dev, u32 attr, int channel,
+				long *val)
+{
+	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	switch (attr) {
+	case hwmon_in_input:
+		ret = regmap_read(hwmon->regmap, volt_regs[channel], &value);
+		if (ret < 0)
+			return ret;
+		*val = value & 0x7fff; /* 15-bit value */
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpro_read_curr(struct device *dev, u32 attr, int channel,
+				long *val)
+{
+	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	switch (attr) {
+	case hwmon_curr_input:
+		ret = regmap_read(hwmon->regmap, curr_regs[channel], &value);
+		if (ret < 0)
+			return ret;
+		*val = value & 0x7fff; /* 15-bit value */
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpro_read_power(struct device *dev, u32 attr, int channel,
+				long *val_pwr)
+{
+	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int val2, val2_mw;
+	unsigned int val, val_mw;
+	int ret;
+
+	switch (attr) {
+	case hwmon_power_input:
+		switch (channel) {
+		case CORE_VRD_PWR:
+			ret = regmap_read(hwmon->regmap,
+					CORE_VRD_PWR_REG, &val);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						CORE_VRD_PWR_MW_REG, &val_mw);
+			if (ret)
+				return ret;
+			break;
+		case SOC_PWR:
+			ret = regmap_read(hwmon->regmap,
+					SOC_PWR_REG, &val);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						SOC_PWR_MW_REG, &val_mw);
+			if (ret)
+				return ret;
+			break;
+		case DIMM_VRD1_PWR:
+			ret = regmap_read(hwmon->regmap,
+					DIMM_VRD1_PWR_REG, &val);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						DIMM_VRD1_PWR_MW_REG, &val_mw);
+			if (ret)
+				return ret;
+			break;
+		case DIMM_VRD2_PWR:
+			ret = regmap_read(hwmon->regmap,
+					DIMM_VRD2_PWR_REG, &val);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						DIMM_VRD2_PWR_MW_REG, &val_mw);
+			if (ret)
+				return ret;
+			break;
+		case RCA_VRD_PWR:
+			ret = regmap_read(hwmon->regmap,
+					RCA_VRD_PWR_REG, &val);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						RCA_VRD_PWR_MW_REG, &val_mw);
+			if (ret)
+				return ret;
+			break;
+		case SOC_TDP_PWR:
+			ret = regmap_read(hwmon->regmap,
+					SOC_TDP_REG, &val);
+			if (ret)
+				return ret;
+			break;
+		case CPU_VRD_PWR:
+			ret = regmap_read(hwmon->regmap,
+					CORE_VRD_PWR_REG, &val);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						CORE_VRD_PWR_MW_REG, &val_mw);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						SOC_PWR_REG, &val2);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						SOC_PWR_MW_REG, &val2_mw);
+			if (ret)
+				return ret;
+			break;
+		case DIMM_VRD_PWR:
+			ret = regmap_read(hwmon->regmap,
+					DIMM_VRD1_PWR_REG, &val);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						DIMM_VRD1_PWR_MW_REG, &val_mw);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						DIMM_VRD2_PWR_REG, &val2);
+			if (!ret)
+				ret = regmap_read(hwmon->regmap,
+						DIMM_VRD2_PWR_MW_REG, &val2_mw);
+			if (ret)
+				return ret;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		/* Set to 0 if invalid */
+		if (val_mw == 0xffff)
+			val_mw = 0;
+		if (val2_mw == 0xffff)
+			val2_mw = 0;
+
+		*val_pwr = (val + val2) * 1000000 + (val_mw + val2_mw) * 1000;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpro_read(struct device *dev, enum hwmon_sensor_types type,
+				u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return smpro_read_temp(dev, attr, channel, val);
+	case hwmon_in:
+		return smpro_read_in(dev, attr, channel, val);
+	case hwmon_power:
+		return smpro_read_power(dev, attr, channel, val);
+	case hwmon_curr:
+		return smpro_read_curr(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int smpro_write(struct device *dev, enum hwmon_sensor_types type,
+				u32 attr, int channel, long val)
+{
+	return -EOPNOTSUPP;
+}
+
+static umode_t smpro_is_visible(const void *data,
+				enum hwmon_sensor_types type,
+				u32 attr, int channel)
+{
+	return 0444;
+}
+
+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 const u32 smpro_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,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	HWMON_T_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info smpro_temp = {
+	.type = hwmon_temp,
+	.config = smpro_temp_config,
+};
+
+static const u32 smpro_in_config[] = {
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	HWMON_I_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info smpro_in = {
+	.type = hwmon_in,
+	.config = smpro_in_config,
+};
+
+static const u32 smpro_curr_config[] = {
+	HWMON_C_INPUT,
+	HWMON_C_INPUT,
+	HWMON_C_INPUT,
+	HWMON_C_INPUT,
+	HWMON_C_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info smpro_curr = {
+	.type = hwmon_curr,
+	.config = smpro_curr_config,
+};
+
+static const u32 smpro_power_config[] = {
+	HWMON_P_INPUT,
+	HWMON_P_INPUT,
+	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 smpro_power = {
+	.type = hwmon_power,
+	.config = smpro_power_config,
+};
+
+static const struct hwmon_channel_info *smpro_info[] = {
+	&smpro_temp,
+	&smpro_in,
+	&smpro_power,
+	&smpro_curr,
+	NULL
+};
+
+static const struct hwmon_ops smpro_hwmon_ops = {
+	.is_visible = smpro_is_visible,
+	.read = smpro_read,
+	.write = smpro_write,
+};
+
+static const struct hwmon_chip_info smpro_chip_info = {
+	.ops = &smpro_hwmon_ops,
+	.info = smpro_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, 19);
+static SENSOR_DEVICE_ATTR(temp6_label, 0444, show_label, NULL, 6);
+static SENSOR_DEVICE_ATTR(temp7_label, 0444, show_label, NULL, 7);
+static SENSOR_DEVICE_ATTR(temp8_label, 0444, show_label, NULL, 8);
+static SENSOR_DEVICE_ATTR(temp9_label, 0444, show_label, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp10_label, 0444, show_label, NULL, 20);
+static SENSOR_DEVICE_ATTR(temp11_label, 0444, show_label, NULL, 10);
+static SENSOR_DEVICE_ATTR(temp12_label, 0444, show_label, NULL, 11);
+static SENSOR_DEVICE_ATTR(temp13_label, 0444, show_label, NULL, 12);
+static SENSOR_DEVICE_ATTR(temp14_label, 0444, show_label, NULL, 13);
+static SENSOR_DEVICE_ATTR(temp15_label, 0444, show_label, NULL, 14);
+static SENSOR_DEVICE_ATTR(temp16_label, 0444, show_label, NULL, 15);
+static SENSOR_DEVICE_ATTR(temp17_label, 0444, show_label, NULL, 17);
+
+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(in4_label, 0444, show_label, NULL, 2);
+static SENSOR_DEVICE_ATTR(in5_label, 0444, show_label, NULL, 17);
+
+static SENSOR_DEVICE_ATTR(power1_label, 0444, show_label, NULL, 5);
+static SENSOR_DEVICE_ATTR(power2_label, 0444, show_label, NULL, 0);
+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(power7_label, 0444, show_label, NULL, 17);
+static SENSOR_DEVICE_ATTR(power8_label, 0444, show_label, NULL, 18);
+
+static SENSOR_DEVICE_ATTR(curr1_label, 0444, show_label, NULL, 5);
+static SENSOR_DEVICE_ATTR(curr2_label, 0444, show_label, NULL, 1);
+static SENSOR_DEVICE_ATTR(curr3_label, 0444, show_label, NULL, 3);
+static SENSOR_DEVICE_ATTR(curr4_label, 0444, show_label, NULL, 4);
+static SENSOR_DEVICE_ATTR(curr5_label, 0444, show_label, NULL, 17);
+
+static struct attribute *smpro_attrs[] = {
+	&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_temp15_label.dev_attr.attr,
+	&sensor_dev_attr_temp16_label.dev_attr.attr,
+	&sensor_dev_attr_temp17_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_in4_label.dev_attr.attr,
+	&sensor_dev_attr_in5_label.dev_attr.attr,
+
+	&sensor_dev_attr_curr1_label.dev_attr.attr,
+	&sensor_dev_attr_curr2_label.dev_attr.attr,
+	&sensor_dev_attr_curr3_label.dev_attr.attr,
+	&sensor_dev_attr_curr4_label.dev_attr.attr,
+	&sensor_dev_attr_curr5_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_power7_label.dev_attr.attr,
+	&sensor_dev_attr_power8_label.dev_attr.attr,
+
+	NULL
+};
+ATTRIBUTE_GROUPS(smpro);
+
+static int is_valid_id(struct regmap *regmap)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(regmap, MANUFACTURER_ID_REG, &val);
+
+	return  (ret || (val != AMPERE_MANUFACTURER_ID)) ? 0 : 1;
+}
+
+static int smpro_hwmon_probe(struct platform_device *pdev)
+{
+	struct smpro_hwmon *hwmon;
+	struct device *hwmon_dev;
+
+	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct smpro_hwmon),
+			GFP_KERNEL);
+	if (!hwmon)
+		return -ENOMEM;
+
+	hwmon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!hwmon->regmap)
+		return -ENODEV;
+
+	/* Check for valid ID */
+	if (!is_valid_id(hwmon->regmap))
+		dev_warn(&pdev->dev, "Hmmh, SMPro not ready yet\n");
+
+	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
+			"smpro_hwmon", hwmon,
+			&smpro_chip_info, smpro_groups);
+	if (IS_ERR(hwmon_dev))
+		dev_err(&pdev->dev, "failed to register as hwmon device");
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct of_device_id smpro_hwmon_of_match[] = {
+	{ .compatible = "ampere,ac01-hwmon" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, smpro_hwmon_of_match);
+
+static struct platform_driver smpro_hwmon_driver = {
+	.probe		= smpro_hwmon_probe,
+	.driver = {
+		.name	= "smpro-hwmon",
+		.of_match_table = smpro_hwmon_of_match,
+	},
+};
+
+module_platform_driver(smpro_hwmon_driver);
+
+MODULE_AUTHOR("Thu Nguyen <thu@os.amperecomputing.com>");
+MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
+MODULE_DESCRIPTION("Ampere Altra SMPro hwmon driver");
+MODULE_LICENSE("GPL");
-- 
2.28.0

[PATCH 4/4] docs: hwmon: (smpro-hwmon) Add documentation

[Quan Nguyen]

Add documentation for the Ampere(R)'s Altra(R) SMpro hwmon driver.

Signed-off-by: Thu Nguyen <thu@os.amperecomputing.com>
Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
---
 Documentation/hwmon/index.rst       |   1 +
 Documentation/hwmon/smpro-hwmon.rst | 100 ++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100644 Documentation/hwmon/smpro-hwmon.rst

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 8d5a2df1ecb6..b48a980ed08b 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -164,6 +164,7 @@ Hardware Monitoring Kernel Drivers
    sis5595
    sl28cpld
    smm665
+   smpro-hwmon
    smsc47b397
    smsc47m192
    smsc47m1
diff --git a/Documentation/hwmon/smpro-hwmon.rst b/Documentation/hwmon/smpro-hwmon.rst
new file mode 100644
index 000000000000..d546b90982e5
--- /dev/null
+++ b/Documentation/hwmon/smpro-hwmon.rst
@@ -0,0 +1,100 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver Ampere(R)'s Altra(R) SMpro hwmon
+==============================================
+
+Supported chips:
+
+  * Ampere(R) Altra(R)
+
+    Prefix: 'smpro'
+
+    Reference: Altra SoC BMC Interface Specification
+
+Author: Thu Nguyen <thu@os.amperecomputing.com>
+
+Description
+-----------
+This driver supports hardware monitoring for Ampere(R) Altra(R) SoC's based on the
+SMpro co-processor (SMpro).
+The following sensor types are supported by the driver:
+
+  * temperature
+  * voltage
+  * current
+  * power
+
+The SMpro interface provides the registers to query the various sensors and
+their values which are then exported to userspace by this driver.
+
+Usage Notes
+-----------
+
+SMpro hwmon driver creates two sysfs files for each sensor.
+
+* File ``<sensor_type><idx>_label`` reports the sensor label.
+* File ``<sensor_type><idx>_input`` returns the sensor value.
+
+The sysfs files are allocated in the SMpro root fs folder.
+There is one root folder for each SMpro instance.
+
+When the SoC is turned off, the driver is failed to read the registers.
+It returns TIMEDOUT Error(-110) for the read sensors.
+
+Sysfs entries
+-------------
+
+The following sysfs files are supported:
+
+* Ampere(R) Altra(R):
+
+===============    =============   ======= ===============================================
+Name        Unit        Perm    Description
+temp1_input     mili Celsius     RO    SoC temperature
+temp2_input     mili Celsius     RO    Highest temperature reported by the SoC VRDs
+temp3_input     mili Celsius     RO    Highest temperature reported by the DIMM VRDs
+temp4_input     mili Celsius     RO    Highest temperature reported by the Core VRDs
+temp5_input     mili Celsius     RO    Highest temperature of DIMM Channel 0 to 3
+temp6_input     mili Celsius     RO    Temperature of DIMM0 on CH0
+temp7_input     mili Celsius     RO    Temperature of DIMM0 on CH1
+temp8_input     mili Celsius     RO    Temperature of DIMM0 on CH2
+temp9_input     mili Celsius     RO    Temperature of DIMM0 on CH3
+temp10_input     mili Celsius     RO    Highest temperature of DIMM Channel 4 to 7
+temp11_input     mili Celsius     RO    Temperature of DIMM0 on CH4
+temp12_input     mili Celsius     RO    Temperature of DIMM0 on CH5
+temp13_input     mili Celsius     RO    Temperature of DIMM0 on CH6
+temp14_input     mili Celsius     RO    Temperature of DIMM0 on CH7
+temp15_input     mili Celsius     RO    MEM HOT Threshold
+temp16_input     mili Celsius     RO    SoC VRD HOT Threshold
+temp17_input     mili Celsius     RO    Highest temperature reported by the RCA VRD
+in0_input     mili Volt     RO    Core voltage
+in1_input     mili Volt     RO    SoC voltage
+in2_input     mili Volt     RO    DIMM VRD1 voltage
+in3_input     mili Volt     RO    DIMM VRD2 voltage
+in4_input     mili Volt     RO    Maximum voltage of DIMM VRD1 and VRD2
+in5_input     mili Volt     RO    RCA VRD voltage
+cur1_input     mili Ampere     RO    Core VRD current
+cur2_input     mili Ampere     RO    SoC VRD current
+cur3_input     mili Ampere     RO    DIMM VRD1 current
+cur4_input     mili Ampere     RO    DIMM VRD2 current
+cur5_input     mili Ampere     RO    RCA VRD current
+power1_input     nano Wat     RO    Core VRD power
+power2_input     nano Wat     RO    SoC VRD power
+power3_input     nano Wat     RO    DIMM VRD1 power
+power4_input     nano Wat     RO    DIMM VRD2 power
+power5_input     nano Wat     RO    CPU VRD power, total of SoC and Core VRD power
+power6_input     nano Wat     RO    Total of DIMM VRD1 and VRD2 power
+power7_input     nano Wat     RO    RCA VRD power
+power8_input     nano Wat     RO    Socket TDP
+===============    =============   ======= ===============================================
+
+Example::
+
+    # cat in0_input
+    830
+    # cat temp1_input
+    37000
+    # cat curr1_input
+    9000
+    # cat power5_input
+    19500000
-- 
2.28.0

Re: [PATCH 3/4] hwmon: smpro: Add Ampere's Altra smpro-hwmon driver

[Guenter Roeck]

On 2/25/21 2:18 AM, Quan Nguyen wrote:
> This commit adds support for Ampere SMpro hwmon driver. This driver
> supports accessing various CPU sensors provided by the SMpro co-processor
> including temperature, power, voltages, and current.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  drivers/hwmon/Kconfig       |   8 +
>  drivers/hwmon/Makefile      |   1 +
>  drivers/hwmon/smpro-hwmon.c | 620 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 629 insertions(+)
>  create mode 100644 drivers/hwmon/smpro-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 54f04e61fb83..7b0458bf903c 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -67,6 +67,14 @@ config SENSORS_ABITUGURU3
>  	  This driver can also be built as a module. If so, the module
>  	  will be called abituguru3.
>  
> +config SENSORS_SMPRO
> +	tristate "Ampere's Altra SMpro hardware monitoring driver"
> +	depends on MFD_SMPRO
> +	help
> +	  If you say yes here you get support for the thermal, voltage,
> +	  current and power sensors of Ampere's Altra processor family SoC
> +	  with SMpro co-processor.
> +
>  config SENSORS_AD7314
>  	tristate "Analog Devices AD7314 and compatibles"
>  	depends on SPI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index fe38e8a5c979..ac0892540abb 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -173,6 +173,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_SMPRO)	+= smpro-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/smpro-hwmon.c b/drivers/hwmon/smpro-hwmon.c
> new file mode 100644
> index 000000000000..d70764ffc78f
> --- /dev/null
> +++ b/drivers/hwmon/smpro-hwmon.c
> @@ -0,0 +1,620 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Ampere Computing SoC's SMPro Hardware Monitoring Driver
> + *
> + * Copyright (c) 2021, 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/>.

Repeating the text of GPL is not necessary. This is what
SPDX-License-Identifier is for.

> + */
> +#include <linux/bitfield.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Identification Registers */
> +#define MANUFACTURER_ID_REG		0x02
> +#define AMPERE_MANUFACTURER_ID		0xCD3A
> +
> +/* Capability Registers  */
> +#define SOC_TDP_REG			0x0E

This is used to report power. What does it actually report ?
If it is the design power, it should not be reported as power,
but possibly as maximum SOC power.

If so, and if other limit registers are reported as actual values,
please change to report as limits.

> +
> +/* 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 RCA_VRD_TEMP_REG		0x1C
> +
> +#define CORE_VRD_PWR_REG		0x20
> +#define SOC_PWR_REG			0x21
> +#define DIMM_VRD1_PWR_REG		0x22
> +#define DIMM_VRD2_PWR_REG		0x23
> +#define CORE_VRD_PWR_MW_REG		0x26
> +#define SOC_PWR_MW_REG			0x27
> +#define DIMM_VRD1_PWR_MW_REG		0x28
> +#define DIMM_VRD2_PWR_MW_REG		0x29
> +#define RCA_VRD_PWR_REG			0x2A
> +#define RCA_VRD_PWR_MW_REG		0x2B
> +
> +#define MEM_HOT_THRESHOLD_REG		0x32
> +#define SOC_VR_HOT_THRESHOLD_REG	0x33

Apparently there _are_ more limit registers.
Please implement those as limit attributes.

> +#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 RCA_VRD_VOLT_REG		0x38
> +
> +#define CORE_VRD_CURR_REG		0x39
> +#define SOC_VRD_CURR_REG		0x3A
> +#define DIMM_VRD1_CURR_REG		0x3B
> +#define DIMM_VRD2_CURR_REG		0x3C
> +#define RCA_VRD_CURR_REG		0x3D
> +
> +/* Add for DIMM group */
> +#define DIMM_GROUP_DUMMY_REG		0xFF
> +
> +struct smpro_hwmon {
> +	struct regmap *regmap;
> +};
> +
> +static const u8 temp_regs[] = {
> +	SOC_TEMP_REG,
> +	SOC_VRD_TEMP_REG,
> +	DIMM_VRD_TEMP_REG,
> +	CORE_VRD_TEMP_REG,
> +	/* reserved for DIMM G0 */
> +	DIMM_GROUP_DUMMY_REG,
> +	CH0_DIMM_TEMP_REG,
> +	CH1_DIMM_TEMP_REG,
> +	CH2_DIMM_TEMP_REG,
> +	CH3_DIMM_TEMP_REG,
> +	/* reserved for DIMM G1 */
> +	DIMM_GROUP_DUMMY_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,
> +	RCA_VRD_TEMP_REG,
> +};
> +
> +static const u8 volt_regs[] = {
> +	CORE_VRD_VOLT_REG,
> +	SOC_VRD_VOLT_REG,
> +	DIMM_VRD1_VOLT_REG,
> +	DIMM_VRD2_VOLT_REG,
> +	/* vrd1 has higher priority than vrd2 using vrd1 as output for ddr */

What is the point of this comment ?

> +	DIMM_VRD1_VOLT_REG,

That doesn't make sense. It appears that DIMM VRD1
is reported as DIMM VRD1 _and_ as DIMM VRD. I don't know
what this is supposed to accomplish, but no. Please remove
all instances of "virtual" attributes.

> +	RCA_VRD_VOLT_REG,
> +};
> +
> +static const u8 curr_regs[] = {
> +	CORE_VRD_CURR_REG,
> +	SOC_VRD_CURR_REG,
> +	DIMM_VRD1_CURR_REG,
> +	DIMM_VRD2_CURR_REG,
> +	RCA_VRD_CURR_REG,
> +};
> +
> +enum pwr_regs {
> +	CORE_VRD_PWR,
> +	SOC_PWR,
> +	DIMM_VRD1_PWR,
> +	DIMM_VRD2_PWR,
> +	CPU_VRD_PWR,
> +	DIMM_VRD_PWR,
> +	RCA_VRD_PWR,
> +	SOC_TDP_PWR,
> +};

Add empty line. And run checkpatch - it would have told you.

> +static const char * const label[] = {
> +	"SoC",
> +	"SoC VRD",
> +	"DIMM VRD",
> +	"DIMM VRD1",
> +	"DIMM VRD2",
> +	"CORE VRD",
> +	"CH0 DIMM",
> +	"CH1 DIMM",
> +	"CH2 DIMM",
> +	"CH3 DIMM",
> +	"CH4 DIMM",
> +	"CH5 DIMM",
> +	"CH6 DIMM",
> +	"CH7 DIMM",
> +	"MEM HOT",
> +	"SoC VR HOT",
> +	"CPU VRD",
> +	"RCA VRD",
> +	"SOC TDP",
> +	"DIMM G0",
> +	"DIMM G1",
> +};
> +
> +static int smpro_read_temp(struct device *dev, u32 attr, int channel,
> +				long *val)

This looks suspicious, and indeed, checkpatch states:

total: 0 errors, 2 warnings, 31 checks, 641 lines checked

Besides, the line length limit is now 100 columns.

> +{
> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int t_max = 0xffffffff;
> +	unsigned int value;
> +	s32 i;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		if (temp_regs[channel] == DIMM_GROUP_DUMMY_REG) {
> +			for (i = 1; i <= 4; i++) {
> +				ret = regmap_read(hwmon->regmap,
> +						temp_regs[channel + i], &value);
> +				if (ret)
> +					return ret;
> +				/* continue if invalid */
> +				if (value == 0xffff)
> +					continue;
> +
> +				value &= 0x1ff; /* 9-bit value */
> +				if (t_max != 0xffffffff)
> +					t_max = (value > t_max) ? value : t_max;
> +				else
> +					t_max = value;
> +			}

This needs explanation. Why not return four different sensor values ?
Otherwise, why four different registers to read data for a single sensor ?

But ... no, wait, it looks like this is used to report the maximum
of the subsequent four DIMMs. No, this is unacceptable. If this
"channel" does not exist, but the index/channel is for some reason
needed, report it as not visible using the is_visible callback.

> +
> +			if (t_max == 0xffffffff)
> +				return -1;

Valid error codes, please. -1 translates to -EPERM. However,
it looks like this may suggest that the DIMMs are not populated.
If so, mask affected channels using the is_visible callback.

> +
> +			*val = t_max * 1000;
> +		} else {
> +			ret = regmap_read(hwmon->regmap,
> +					temp_regs[channel], &value);
> +			if (ret)
> +				return ret;
> +			*val = (value & 0x1ff) * 1000;

Above code suggests that a value of 0xffff might be reported,
which micht suggest an unpopulated sensor (eg an unpouplated DIMM).
The driver should not blindly report 255 Dgrees C in that situation,
but instead mark the sensor as invisible.

The same applies to all other sensors.

> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int smpro_read_in(struct device *dev, u32 attr, int channel,
> +				long *val)
> +{
> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		ret = regmap_read(hwmon->regmap, volt_regs[channel], &value);
> +		if (ret < 0)
> +			return ret;
> +		*val = value & 0x7fff; /* 15-bit value */

Scale reported by the hardware is 1mV ? Justw wondering;
a brief comment might make sense.

> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int smpro_read_curr(struct device *dev, u32 attr, int channel,
> +				long *val)
> +{
> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_curr_input:
> +		ret = regmap_read(hwmon->regmap, curr_regs[channel], &value);
> +		if (ret < 0)
> +			return ret;
> +		*val = value & 0x7fff; /* 15-bit value */
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int smpro_read_power(struct device *dev, u32 attr, int channel,
> +				long *val_pwr)
> +{
> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int val2, val2_mw;
> +	unsigned int val, val_mw;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		switch (channel) {
> +		case CORE_VRD_PWR:
> +			ret = regmap_read(hwmon->regmap,
> +					CORE_VRD_PWR_REG, &val);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						CORE_VRD_PWR_MW_REG, &val_mw);
> +			if (ret)
> +				return ret;

Please handle errors directly.

			ret = regmap_read();
			if (ret)
				return ret;
			ret = regmap_read()
			if (ret)
				return ret;

Same everywhere below.

> +			break;
> +		case SOC_PWR:
> +			ret = regmap_read(hwmon->regmap,
> +					SOC_PWR_REG, &val);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						SOC_PWR_MW_REG, &val_mw);
> +			if (ret)
> +				return ret;
> +			break;
> +		case DIMM_VRD1_PWR:
> +			ret = regmap_read(hwmon->regmap,
> +					DIMM_VRD1_PWR_REG, &val);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						DIMM_VRD1_PWR_MW_REG, &val_mw);
> +			if (ret)
> +				return ret;
> +			break;
> +		case DIMM_VRD2_PWR:
> +			ret = regmap_read(hwmon->regmap,
> +					DIMM_VRD2_PWR_REG, &val);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						DIMM_VRD2_PWR_MW_REG, &val_mw);
> +			if (ret)
> +				return ret;
> +			break;
> +		case RCA_VRD_PWR:
> +			ret = regmap_read(hwmon->regmap,
> +					RCA_VRD_PWR_REG, &val);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						RCA_VRD_PWR_MW_REG, &val_mw);
> +			if (ret)
> +				return ret;
> +			break;
> +		case SOC_TDP_PWR:
> +			ret = regmap_read(hwmon->regmap,
> +					SOC_TDP_REG, &val);
> +			if (ret)
> +				return ret;
> +			break;
> +		case CPU_VRD_PWR:
> +			ret = regmap_read(hwmon->regmap,
> +					CORE_VRD_PWR_REG, &val);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						CORE_VRD_PWR_MW_REG, &val_mw);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						SOC_PWR_REG, &val2);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						SOC_PWR_MW_REG, &val2_mw);
> +			if (ret)
> +				return ret;
> +			break;
> +		case DIMM_VRD_PWR:
> +			ret = regmap_read(hwmon->regmap,
> +					DIMM_VRD1_PWR_REG, &val);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						DIMM_VRD1_PWR_MW_REG, &val_mw);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						DIMM_VRD2_PWR_REG, &val2);
> +			if (!ret)
> +				ret = regmap_read(hwmon->regmap,
> +						DIMM_VRD2_PWR_MW_REG, &val2_mw);
> +			if (ret)
> +				return ret;
> +			break;

Ah, there are more "virtual" atributes. Again, please remove all those.
If userspace w ants to group attributes in whatever way it wants to, fine,
but that is not a kernel concern.

Besides, what if a DIMM is unpopulated ?

> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		/* Set to 0 if invalid */
> +		if (val_mw == 0xffff)
> +			val_mw = 0;
> +		if (val2_mw == 0xffff)
> +			val2_mw = 0;
> +

Does this indicate a temporary or a permanent problem ?
If it means that the sensor is not populated, it should be
handled in is_visible. Also, what about val / val2 ?

> +		*val_pwr = (val + val2) * 1000000 + (val_mw + val2_mw) * 1000;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int smpro_read(struct device *dev, enum hwmon_sensor_types type,
> +				u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return smpro_read_temp(dev, attr, channel, val);
> +	case hwmon_in:
> +		return smpro_read_in(dev, attr, channel, val);
> +	case hwmon_power:
> +		return smpro_read_power(dev, attr, channel, val);
> +	case hwmon_curr:
> +		return smpro_read_curr(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int smpro_write(struct device *dev, enum hwmon_sensor_types type,
> +				u32 attr, int channel, long val)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t smpro_is_visible(const void *data,
> +				enum hwmon_sensor_types type,
> +				u32 attr, int channel)
> +{
> +	return 0444;
> +}
> +
> +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 const u32 smpro_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,
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	HWMON_T_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info smpro_temp = {
> +	.type = hwmon_temp,
> +	.config = smpro_temp_config,
> +};
> +
> +static const u32 smpro_in_config[] = {
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	HWMON_I_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info smpro_in = {
> +	.type = hwmon_in,
> +	.config = smpro_in_config,
> +};
> +
> +static const u32 smpro_curr_config[] = {
> +	HWMON_C_INPUT,
> +	HWMON_C_INPUT,
> +	HWMON_C_INPUT,
> +	HWMON_C_INPUT,
> +	HWMON_C_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info smpro_curr = {
> +	.type = hwmon_curr,
> +	.config = smpro_curr_config,
> +};
> +
> +static const u32 smpro_power_config[] = {
> +	HWMON_P_INPUT,
> +	HWMON_P_INPUT,
> +	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 smpro_power = {
> +	.type = hwmon_power,
> +	.config = smpro_power_config,
> +};
> +
> +static const struct hwmon_channel_info *smpro_info[] = {
> +	&smpro_temp,
> +	&smpro_in,
> +	&smpro_power,
> +	&smpro_curr,
> +	NULL
> +};
> +

The above can all be polulated with HWMON_CHANNEL_INFO().

> +static const struct hwmon_ops smpro_hwmon_ops = {
> +	.is_visible = smpro_is_visible,
> +	.read = smpro_read,
> +	.write = smpro_write,
> +};
> +
> +static const struct hwmon_chip_info smpro_chip_info = {
> +	.ops = &smpro_hwmon_ops,
> +	.info = smpro_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, 19);
> +static SENSOR_DEVICE_ATTR(temp6_label, 0444, show_label, NULL, 6);
> +static SENSOR_DEVICE_ATTR(temp7_label, 0444, show_label, NULL, 7);
> +static SENSOR_DEVICE_ATTR(temp8_label, 0444, show_label, NULL, 8);
> +static SENSOR_DEVICE_ATTR(temp9_label, 0444, show_label, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp10_label, 0444, show_label, NULL, 20);
> +static SENSOR_DEVICE_ATTR(temp11_label, 0444, show_label, NULL, 10);
> +static SENSOR_DEVICE_ATTR(temp12_label, 0444, show_label, NULL, 11);
> +static SENSOR_DEVICE_ATTR(temp13_label, 0444, show_label, NULL, 12);
> +static SENSOR_DEVICE_ATTR(temp14_label, 0444, show_label, NULL, 13);
> +static SENSOR_DEVICE_ATTR(temp15_label, 0444, show_label, NULL, 14);
> +static SENSOR_DEVICE_ATTR(temp16_label, 0444, show_label, NULL, 15);
> +static SENSOR_DEVICE_ATTR(temp17_label, 0444, show_label, NULL, 17);
> +
> +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(in4_label, 0444, show_label, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in5_label, 0444, show_label, NULL, 17);
> +
> +static SENSOR_DEVICE_ATTR(power1_label, 0444, show_label, NULL, 5);
> +static SENSOR_DEVICE_ATTR(power2_label, 0444, show_label, NULL, 0);
> +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(power7_label, 0444, show_label, NULL, 17);
> +static SENSOR_DEVICE_ATTR(power8_label, 0444, show_label, NULL, 18);
> +
> +static SENSOR_DEVICE_ATTR(curr1_label, 0444, show_label, NULL, 5);
> +static SENSOR_DEVICE_ATTR(curr2_label, 0444, show_label, NULL, 1);
> +static SENSOR_DEVICE_ATTR(curr3_label, 0444, show_label, NULL, 3);
> +static SENSOR_DEVICE_ATTR(curr4_label, 0444, show_label, NULL, 4);
> +static SENSOR_DEVICE_ATTR(curr5_label, 0444, show_label, NULL, 17);
> +
> +static struct attribute *smpro_attrs[] = {
> +	&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_temp15_label.dev_attr.attr,
> +	&sensor_dev_attr_temp16_label.dev_attr.attr,
> +	&sensor_dev_attr_temp17_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_in4_label.dev_attr.attr,
> +	&sensor_dev_attr_in5_label.dev_attr.attr,
> +
> +	&sensor_dev_attr_curr1_label.dev_attr.attr,
> +	&sensor_dev_attr_curr2_label.dev_attr.attr,
> +	&sensor_dev_attr_curr3_label.dev_attr.attr,
> +	&sensor_dev_attr_curr4_label.dev_attr.attr,
> +	&sensor_dev_attr_curr5_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_power7_label.dev_attr.attr,
> +	&sensor_dev_attr_power8_label.dev_attr.attr,
> +
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(smpro);
> +

This can be done with LABEL attributes (HWMON_T_LABEL etc).

> +static int is_valid_id(struct regmap *regmap)

bool

> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(regmap, MANUFACTURER_ID_REG, &val);
> +
> +	return  (ret || (val != AMPERE_MANUFACTURER_ID)) ? 0 : 1;
> +}
> +
> +static int smpro_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct smpro_hwmon *hwmon;
> +	struct device *hwmon_dev;
> +
> +	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct smpro_hwmon),
> +			GFP_KERNEL);
> +	if (!hwmon)
> +		return -ENOMEM;
> +
> +	hwmon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!hwmon->regmap)
> +		return -ENODEV;
> +
> +	/* Check for valid ID */
> +	if (!is_valid_id(hwmon->regmap))
> +		dev_warn(&pdev->dev, "Hmmh, SMPro not ready yet\n");

This does not appear appropriate. It should be either -EPROBE_DEFER
or -ENODEV.

> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> +			"smpro_hwmon", hwmon,
> +			&smpro_chip_info, smpro_groups);
> +	if (IS_ERR(hwmon_dev))
> +		dev_err(&pdev->dev, "failed to register as hwmon device");

Unnecessary error message. It is either -EINVAL, which would
always be the case, or -ENOMEM, which is already reported.

> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id smpro_hwmon_of_match[] = {
> +	{ .compatible = "ampere,ac01-hwmon" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, smpro_hwmon_of_match);
> +
> +static struct platform_driver smpro_hwmon_driver = {
> +	.probe		= smpro_hwmon_probe,
> +	.driver = {
> +		.name	= "smpro-hwmon",
> +		.of_match_table = smpro_hwmon_of_match,
> +	},
> +};
> +
> +module_platform_driver(smpro_hwmon_driver);
> +
> +MODULE_AUTHOR("Thu Nguyen <thu@os.amperecomputing.com>");
> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
> +MODULE_DESCRIPTION("Ampere Altra SMPro hwmon driver");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 4/4] docs: hwmon: (smpro-hwmon) Add documentation

[Guenter Roeck]

On 2/25/21 2:18 AM, Quan Nguyen wrote:
> Add documentation for the Ampere(R)'s Altra(R) SMpro hwmon driver.
> 
> Signed-off-by: Thu Nguyen <thu@os.amperecomputing.com>
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  Documentation/hwmon/index.rst       |   1 +
>  Documentation/hwmon/smpro-hwmon.rst | 100 ++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
>  create mode 100644 Documentation/hwmon/smpro-hwmon.rst
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 8d5a2df1ecb6..b48a980ed08b 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -164,6 +164,7 @@ Hardware Monitoring Kernel Drivers
>     sis5595
>     sl28cpld
>     smm665
> +   smpro-hwmon

"hwmon" seems a bit redundant here.

>     smsc47b397
>     smsc47m192
>     smsc47m1
> diff --git a/Documentation/hwmon/smpro-hwmon.rst b/Documentation/hwmon/smpro-hwmon.rst
> new file mode 100644
> index 000000000000..d546b90982e5
> --- /dev/null
> +++ b/Documentation/hwmon/smpro-hwmon.rst
> @@ -0,0 +1,100 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver Ampere(R)'s Altra(R) SMpro hwmon
> +==============================================
> +
> +Supported chips:
> +
> +  * Ampere(R) Altra(R)
> +
> +    Prefix: 'smpro'
> +
> +    Reference: Altra SoC BMC Interface Specification
> +
> +Author: Thu Nguyen <thu@os.amperecomputing.com>
> +
> +Description
> +-----------
> +This driver supports hardware monitoring for Ampere(R) Altra(R) SoC's based on the
> +SMpro co-processor (SMpro).
> +The following sensor types are supported by the driver:
> +
> +  * temperature
> +  * voltage
> +  * current
> +  * power
> +
> +The SMpro interface provides the registers to query the various sensors and
> +their values which are then exported to userspace by this driver.
> +
> +Usage Notes
> +-----------
> +
> +SMpro hwmon driver creates two sysfs files for each sensor.
> +
> +* File ``<sensor_type><idx>_label`` reports the sensor label.
> +* File ``<sensor_type><idx>_input`` returns the sensor value.
> +
> +The sysfs files are allocated in the SMpro root fs folder.
> +There is one root folder for each SMpro instance.
> +
> +When the SoC is turned off, the driver is failed to read the registers.
> +It returns TIMEDOUT Error(-110) for the read sensors.
> +

Maybe better something like

When the SoC is turned off, the driver will fail to read registers
and return -ETIMEDOUT.

Can that indeed happen ? That seems to be highly undesirable.

> +Sysfs entries
> +-------------
> +
> +The following sysfs files are supported:
> +
> +* Ampere(R) Altra(R):
> +
> +===============    =============   ======= ===============================================
> +Name        Unit        Perm    Description
> +temp1_input     mili Celsius     RO    SoC temperature

s/mili/milli/ throughout

> +temp2_input     mili Celsius     RO    Highest temperature reported by the SoC VRDs
> +temp3_input     mili Celsius     RO    Highest temperature reported by the DIMM VRDs
> +temp4_input     mili Celsius     RO    Highest temperature reported by the Core VRDs

What does "highest" stand for here ? Is it the _current_ highest
temperature, added up by the hardware/firmware, or is it the historic
highest temperature ? Historic data should be reported as tempX_highest.

> +temp5_input     mili Celsius     RO    Highest temperature of DIMM Channel 0 to 3

drop; reported individually.

> +temp6_input     mili Celsius     RO    Temperature of DIMM0 on CH0
> +temp7_input     mili Celsius     RO    Temperature of DIMM0 on CH1
> +temp8_input     mili Celsius     RO    Temperature of DIMM0 on CH2
> +temp9_input     mili Celsius     RO    Temperature of DIMM0 on CH3
> +temp10_input     mili Celsius     RO    Highest temperature of DIMM Channel 4 to 7

drop; reported individually.

> +temp11_input     mili Celsius     RO    Temperature of DIMM0 on CH4
> +temp12_input     mili Celsius     RO    Temperature of DIMM0 on CH5
> +temp13_input     mili Celsius     RO    Temperature of DIMM0 on CH6
> +temp14_input     mili Celsius     RO    Temperature of DIMM0 on CH7
> +temp15_input     mili Celsius     RO    MEM HOT Threshold
> +temp16_input     mili Celsius     RO    SoC VRD HOT Threshold

Report as tempX_max or tempX_crit, as appropriate (eg temp2_max or
temp2_crit for SoC VRD HOT Threshold). If there is a single threshold
temperature for all DIMMs, report the same limit value for all DIMM
temperature sensors.

> +temp17_input     mili Celsius     RO    Highest temperature reported by the RCA VRD

Same question about "highest" as above. Either "highest" is
inappropriate, or there are multiple RCA VRDs and only the
highest temperature of those is reported (which should be
explicitly stated).

> +in0_input     mili Volt     RO    Core voltage
> +in1_input     mili Volt     RO    SoC voltage
> +in2_input     mili Volt     RO    DIMM VRD1 voltage
> +in3_input     mili Volt     RO    DIMM VRD2 voltage
> +in4_input     mili Volt     RO    Maximum voltage of DIMM VRD1 and VRD2

drop; reported individually.

> +in5_input     mili Volt     RO    RCA VRD voltage
> +cur1_input     mili Ampere     RO    Core VRD current
> +cur2_input     mili Ampere     RO    SoC VRD current
> +cur3_input     mili Ampere     RO    DIMM VRD1 current
> +cur4_input     mili Ampere     RO    DIMM VRD2 current
> +cur5_input     mili Ampere     RO    RCA VRD current
> +power1_input     nano Wat     RO    Core VRD power

Expected scale is micro-Watt.

> +power2_input     nano Wat     RO    SoC VRD power
> +power3_input     nano Wat     RO    DIMM VRD1 power
> +power4_input     nano Wat     RO    DIMM VRD2 power
> +power5_input     nano Wat     RO    CPU VRD power, total of SoC and Core VRD power

drop

> +power6_input     nano Wat     RO    Total of DIMM VRD1 and VRD2 power

drop

> +power7_input     nano Wat     RO    RCA VRD power
> +power8_input     nano Wat     RO    Socket TDP

Report as max attribute

> +===============    =============   ======= ===============================================
> +
> +Example::
> +
> +    # cat in0_input
> +    830
> +    # cat temp1_input
> +    37000
> +    # cat curr1_input
> +    9000
> +    # cat power5_input
> +    19500000
> 

Re: [PATCH 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Guenter Roeck]

On 2/25/21 2:18 AM, Quan Nguyen wrote:
> Adds device tree bindings for SMPro drivers found on the Mt.Jade hardware
> reference platform with Ampere's Altra Processor family.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  .../bindings/hwmon/ampere,ac01-hwmon.yaml     | 27 ++++++
>  .../bindings/mfd/ampere,ac01-smpro.yaml       | 82 +++++++++++++++++++
>  2 files changed, 109 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>  create mode 100644 Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
> new file mode 100644
> index 000000000000..d13862ba646b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
> @@ -0,0 +1,27 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hardware monitoring driver for the Ampere Altra SMPro
> +
> +maintainers:
> +  - Quan Nguyen <quan@os.amperecomputing.com>
> +
> +description: |
> +  This module is part of the Ampere Altra SMPro multi-function device. For more
> +  details see ../mfd/ampere,ac01-smpro.yaml.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ampere,ac01-hwmon
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
> new file mode 100644
> index 000000000000..06b0239413ae
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ampere,ac01-smpro.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ampere Altra SMPro firmware driver
> +
> +maintainers:
> +  - Quan Nguyen <quan@os.amperecomputing.com>
> +
> +description: |
> +  Ampere Altra SMPro firmware may contain different blocks like hardware
> +  monitoring, error monitoring and other miscellaneous features.
> +
> +properties:
> +  compatible:
> +    const: ampere,ac01-smpro

Is that the same as the "ampere,smpro" in
arch/arm/boot/dts/nuvoton-npcm730-kudo.dts ?

Guenter

> +
> +  reg:
> +    description:
> +      I2C device address.
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^hwmon(@[0-9a-f]+)?$":
> +    $ref: ../hwmon/ampere,ac01-hwmon.yaml
> +
> +  "^misc(@[0-9a-f]+)?$":
> +    type: object
> +    description: Ampere Altra SMPro Misc driver
> +    properties:
> +      compatible:
> +        const: "ampere,ac01-misc"
> +
> +  "^errmon(@[0-9a-f]+)?$":
> +    type: object
> +    description: Ampere Altra SMPro Error Monitor driver
> +    properties:
> +      compatible:
> +        const: "ampere,ac01-errmon"
> +
> +required:
> +  - "#address-cells"
> +  - "#size-cells"
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        smpro@4f {
> +            compatible = "ampere,ac01-smpro";
> +            reg = <0x4f>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            hwmon {
> +                compatible = "ampere,ac01-hwmon";
> +            };
> +
> +            misc {
> +                compatible = "ampere,ac01-misc";
> +            };
> +
> +            errmon {
> +                compatible = "ampere,ac01-errmon";
> +            };
> +
> +        };
> +    };
> 

Re: [PATCH 2/4] mfd: simple-mfd-i2c: Adds Ampere's Altra SMpro support

[Lee Jones]

On Thu, 25 Feb 2021, Quan Nguyen wrote:

> Adds an MFD driver for SMpro found on the Mt.Jade hardware reference
> platform with Ampere's Altra processor family.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  drivers/mfd/Kconfig          | 10 ++++++++++
>  drivers/mfd/simple-mfd-i2c.c | 15 +++++++++++++--
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b74efa469e90..5414371bdea1 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -77,6 +77,16 @@ config MFD_AS3711
>  	help
>  	  Support for the AS3711 PMIC from AMS
>  
> +config MFD_SMPRO
> +	tristate "Ampere Computing MFD SMpro core driver"
> +	select MFD_SIMPLE_MFD_I2C

Nice to see another user here.

> +	help
> +	  Say yes here to enable SMpro driver support for Ampere's Altra
> +	  processor family.
> +
> +	  Ampere's Altra SMpro exposes an I2C regmap interface that can
> +	  be accessed by child devices.
> +
>  config MFD_AS3722
>  	tristate "ams AS3722 Power Management IC"
>  	select MFD_CORE
> diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
> index 87f684cff9a1..0459a9fbd3f5 100644
> --- a/drivers/mfd/simple-mfd-i2c.c
> +++ b/drivers/mfd/simple-mfd-i2c.c
> @@ -21,14 +21,24 @@ static const struct regmap_config simple_regmap_config = {
>  	.val_bits = 8,
>  };
>  
> +static const struct regmap_config simple_word_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +};
> +
>  static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>  {
>  	const struct regmap_config *config;
>  	struct regmap *regmap;
>  
>  	config = device_get_match_data(&i2c->dev);
> -	if (!config)
> -		config = &simple_regmap_config;
> +	if (!config) {
> +		if (of_device_is_compatible(i2c->dev.of_node,
> +						"ampere,ac01-smpro"))

Could you use 'struct of_device_id's .data attribute instead please?

> +			config = &simple_word_regmap_config;
> +		else
> +			config = &simple_regmap_config;
> +	}
>  
>  	regmap = devm_regmap_init_i2c(i2c, config);
>  	if (IS_ERR(regmap))
> @@ -39,6 +49,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>  
>  static const struct of_device_id simple_mfd_i2c_of_match[] = {
>  	{ .compatible = "kontron,sl28cpld" },
> +	{ .compatible = "ampere,ac01-smpro" },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/4] hwmon: smpro: Add Ampere's Altra smpro-hwmon driver

[Quan Nguyen]

On 25/02/2021 23:08, Guenter Roeck wrote:
> On 2/25/21 2:18 AM, Quan Nguyen wrote:
>> This commit adds support for Ampere SMpro hwmon driver. This driver
>> supports accessing various CPU sensors provided by the SMpro co-processor
>> including temperature, power, voltages, and current.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>>   drivers/hwmon/Kconfig       |   8 +
>>   drivers/hwmon/Makefile      |   1 +
>>   drivers/hwmon/smpro-hwmon.c | 620 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 629 insertions(+)
>>   create mode 100644 drivers/hwmon/smpro-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 54f04e61fb83..7b0458bf903c 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -67,6 +67,14 @@ config SENSORS_ABITUGURU3
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called abituguru3.
>>   
>> +config SENSORS_SMPRO
>> +	tristate "Ampere's Altra SMpro hardware monitoring driver"
>> +	depends on MFD_SMPRO
>> +	help
>> +	  If you say yes here you get support for the thermal, voltage,
>> +	  current and power sensors of Ampere's Altra processor family SoC
>> +	  with SMpro co-processor.
>> +
>>   config SENSORS_AD7314
>>   	tristate "Analog Devices AD7314 and compatibles"
>>   	depends on SPI
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index fe38e8a5c979..ac0892540abb 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -173,6 +173,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_SMPRO)	+= smpro-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/smpro-hwmon.c b/drivers/hwmon/smpro-hwmon.c
>> new file mode 100644
>> index 000000000000..d70764ffc78f
>> --- /dev/null
>> +++ b/drivers/hwmon/smpro-hwmon.c
>> @@ -0,0 +1,620 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Ampere Computing SoC's SMPro Hardware Monitoring Driver
>> + *
>> + * Copyright (c) 2021, 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/>.
> 
> Repeating the text of GPL is not necessary. This is what
> SPDX-License-Identifier is for.
> 
Thanks Guenter,
Wil fix this in next version.

>> + */
>> +#include <linux/bitfield.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +
>> +/* Identification Registers */
>> +#define MANUFACTURER_ID_REG		0x02
>> +#define AMPERE_MANUFACTURER_ID		0xCD3A
>> +
>> +/* Capability Registers  */
>> +#define SOC_TDP_REG			0x0E
> 
> This is used to report power. What does it actually report ?
> If it is the design power, it should not be reported as power,
> but possibly as maximum SOC power.
> 
> If so, and if other limit registers are reported as actual values,
> please change to report as limits.
> 
>> +
>> +/* 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 RCA_VRD_TEMP_REG		0x1C
>> +
>> +#define CORE_VRD_PWR_REG		0x20
>> +#define SOC_PWR_REG			0x21
>> +#define DIMM_VRD1_PWR_REG		0x22
>> +#define DIMM_VRD2_PWR_REG		0x23
>> +#define CORE_VRD_PWR_MW_REG		0x26
>> +#define SOC_PWR_MW_REG			0x27
>> +#define DIMM_VRD1_PWR_MW_REG		0x28
>> +#define DIMM_VRD2_PWR_MW_REG		0x29
>> +#define RCA_VRD_PWR_REG			0x2A
>> +#define RCA_VRD_PWR_MW_REG		0x2B
>> +
>> +#define MEM_HOT_THRESHOLD_REG		0x32
>> +#define SOC_VR_HOT_THRESHOLD_REG	0x33
> 
> Apparently there _are_ more limit registers.
> Please implement those as limit attributes.
>
Will change in next version

>> +#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 RCA_VRD_VOLT_REG		0x38
>> +
>> +#define CORE_VRD_CURR_REG		0x39
>> +#define SOC_VRD_CURR_REG		0x3A
>> +#define DIMM_VRD1_CURR_REG		0x3B
>> +#define DIMM_VRD2_CURR_REG		0x3C
>> +#define RCA_VRD_CURR_REG		0x3D
>> +
>> +/* Add for DIMM group */
>> +#define DIMM_GROUP_DUMMY_REG		0xFF
>> +
>> +struct smpro_hwmon {
>> +	struct regmap *regmap;
>> +};
>> +
>> +static const u8 temp_regs[] = {
>> +	SOC_TEMP_REG,
>> +	SOC_VRD_TEMP_REG,
>> +	DIMM_VRD_TEMP_REG,
>> +	CORE_VRD_TEMP_REG,
>> +	/* reserved for DIMM G0 */
>> +	DIMM_GROUP_DUMMY_REG,
>> +	CH0_DIMM_TEMP_REG,
>> +	CH1_DIMM_TEMP_REG,
>> +	CH2_DIMM_TEMP_REG,
>> +	CH3_DIMM_TEMP_REG,
>> +	/* reserved for DIMM G1 */
>> +	DIMM_GROUP_DUMMY_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,
>> +	RCA_VRD_TEMP_REG,
>> +};
>> +
>> +static const u8 volt_regs[] = {
>> +	CORE_VRD_VOLT_REG,
>> +	SOC_VRD_VOLT_REG,
>> +	DIMM_VRD1_VOLT_REG,
>> +	DIMM_VRD2_VOLT_REG,
>> +	/* vrd1 has higher priority than vrd2 using vrd1 as output for ddr */
> 
> What is the point of this comment ?
> 
>> +	DIMM_VRD1_VOLT_REG,
> 
> That doesn't make sense. It appears that DIMM VRD1
> is reported as DIMM VRD1 _and_ as DIMM VRD. I don't know
> what this is supposed to accomplish, but no. Please remove
> all instances of "virtual" attributes.
> 
Yes, will try to remove "virtual" attr in next version.

>> +	RCA_VRD_VOLT_REG,
>> +};
>> +
>> +static const u8 curr_regs[] = {
>> +	CORE_VRD_CURR_REG,
>> +	SOC_VRD_CURR_REG,
>> +	DIMM_VRD1_CURR_REG,
>> +	DIMM_VRD2_CURR_REG,
>> +	RCA_VRD_CURR_REG,
>> +};
>> +
>> +enum pwr_regs {
>> +	CORE_VRD_PWR,
>> +	SOC_PWR,
>> +	DIMM_VRD1_PWR,
>> +	DIMM_VRD2_PWR,
>> +	CPU_VRD_PWR,
>> +	DIMM_VRD_PWR,
>> +	RCA_VRD_PWR,
>> +	SOC_TDP_PWR,
>> +};
> 
> Add empty line. And run checkpatch - it would have told you.
> 
Thanks, will fix this.

>> +static const char * const label[] = {
>> +	"SoC",
>> +	"SoC VRD",
>> +	"DIMM VRD",
>> +	"DIMM VRD1",
>> +	"DIMM VRD2",
>> +	"CORE VRD",
>> +	"CH0 DIMM",
>> +	"CH1 DIMM",
>> +	"CH2 DIMM",
>> +	"CH3 DIMM",
>> +	"CH4 DIMM",
>> +	"CH5 DIMM",
>> +	"CH6 DIMM",
>> +	"CH7 DIMM",
>> +	"MEM HOT",
>> +	"SoC VR HOT",
>> +	"CPU VRD",
>> +	"RCA VRD",
>> +	"SOC TDP",
>> +	"DIMM G0",
>> +	"DIMM G1",
>> +};
>> +
>> +static int smpro_read_temp(struct device *dev, u32 attr, int channel,
>> +				long *val)
> 
> This looks suspicious, and indeed, checkpatch states:
> 
> total: 0 errors, 2 warnings, 31 checks, 641 lines checked
> 
> Besides, the line length limit is now 100 columns.
> 
Thanks, let me check this.
>> +{
>> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
>> +	unsigned int t_max = 0xffffffff;
>> +	unsigned int value;
>> +	s32 i;
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_temp_input:
>> +		if (temp_regs[channel] == DIMM_GROUP_DUMMY_REG) {
>> +			for (i = 1; i <= 4; i++) {
>> +				ret = regmap_read(hwmon->regmap,
>> +						temp_regs[channel + i], &value);
>> +				if (ret)
>> +					return ret;
>> +				/* continue if invalid */
>> +				if (value == 0xffff)
>> +					continue;
>> +
>> +				value &= 0x1ff; /* 9-bit value */
>> +				if (t_max != 0xffffffff)
>> +					t_max = (value > t_max) ? value : t_max;
>> +				else
>> +					t_max = value;
>> +			}
> 
> This needs explanation. Why not return four different sensor values ?
> Otherwise, why four different registers to read data for a single sensor ?
> 
> But ... no, wait, it looks like this is used to report the maximum
> of the subsequent four DIMMs. No, this is unacceptable. If this
> "channel" does not exist, but the index/channel is for some reason
> needed, report it as not visible using the is_visible callback.
> 
>> +
>> +			if (t_max == 0xffffffff)
>> +				return -1;
> 
> Valid error codes, please. -1 translates to -EPERM. However,
> it looks like this may suggest that the DIMMs are not populated.
> If so, mask affected channels using the is_visible callback.
> 
Yes, you are right, this is to report the max temp among DIMMs.
We will drop it as it is "virtual" attr.
>> +
>> +			*val = t_max * 1000;
>> +		} else {
>> +			ret = regmap_read(hwmon->regmap,
>> +					temp_regs[channel], &value);
>> +			if (ret)
>> +				return ret;
>> +			*val = (value & 0x1ff) * 1000;
> 
> Above code suggests that a value of 0xffff might be reported,
> which micht suggest an unpopulated sensor (eg an unpouplated DIMM).
> The driver should not blindly report 255 Dgrees C in that situation,
> but instead mark the sensor as invisible.
> 
> The same applies to all other sensors.
> 
Thanks,
The value of 0xffff indeed indicate that the sensor is not available and 
should be marked as invisible attr.

>> +		}
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int smpro_read_in(struct device *dev, u32 attr, int channel,
>> +				long *val)
>> +{
>> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
>> +	unsigned int value;
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_in_input:
>> +		ret = regmap_read(hwmon->regmap, volt_regs[channel], &value);
>> +		if (ret < 0)
>> +			return ret;
>> +		*val = value & 0x7fff; /* 15-bit value */
> 
> Scale reported by the hardware is 1mV ? Justw wondering;
> a brief comment might make sense.
> 
Will add comment to clarify this.

>> +		return 0;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int smpro_read_curr(struct device *dev, u32 attr, int channel,
>> +				long *val)
>> +{
>> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
>> +	unsigned int value;
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_curr_input:
>> +		ret = regmap_read(hwmon->regmap, curr_regs[channel], &value);
>> +		if (ret < 0)
>> +			return ret;
>> +		*val = value & 0x7fff; /* 15-bit value */
>> +		return 0;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int smpro_read_power(struct device *dev, u32 attr, int channel,
>> +				long *val_pwr)
>> +{
>> +	struct smpro_hwmon *hwmon = dev_get_drvdata(dev);
>> +	unsigned int val2, val2_mw;
>> +	unsigned int val, val_mw;
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_power_input:
>> +		switch (channel) {
>> +		case CORE_VRD_PWR:
>> +			ret = regmap_read(hwmon->regmap,
>> +					CORE_VRD_PWR_REG, &val);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						CORE_VRD_PWR_MW_REG, &val_mw);
>> +			if (ret)
>> +				return ret;
> 
> Please handle errors directly.
> 
> 			ret = regmap_read();
> 			if (ret)
> 				return ret;
> 			ret = regmap_read()
> 			if (ret)
> 				return ret;
> 
> Same everywhere below.
> 
Will change them

>> +			break;
>> +		case SOC_PWR:
>> +			ret = regmap_read(hwmon->regmap,
>> +					SOC_PWR_REG, &val);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						SOC_PWR_MW_REG, &val_mw);
>> +			if (ret)
>> +				return ret;
>> +			break;
>> +		case DIMM_VRD1_PWR:
>> +			ret = regmap_read(hwmon->regmap,
>> +					DIMM_VRD1_PWR_REG, &val);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						DIMM_VRD1_PWR_MW_REG, &val_mw);
>> +			if (ret)
>> +				return ret;
>> +			break;
>> +		case DIMM_VRD2_PWR:
>> +			ret = regmap_read(hwmon->regmap,
>> +					DIMM_VRD2_PWR_REG, &val);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						DIMM_VRD2_PWR_MW_REG, &val_mw);
>> +			if (ret)
>> +				return ret;
>> +			break;
>> +		case RCA_VRD_PWR:
>> +			ret = regmap_read(hwmon->regmap,
>> +					RCA_VRD_PWR_REG, &val);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						RCA_VRD_PWR_MW_REG, &val_mw);
>> +			if (ret)
>> +				return ret;
>> +			break;
>> +		case SOC_TDP_PWR:
>> +			ret = regmap_read(hwmon->regmap,
>> +					SOC_TDP_REG, &val);
>> +			if (ret)
>> +				return ret;
>> +			break;
>> +		case CPU_VRD_PWR:
>> +			ret = regmap_read(hwmon->regmap,
>> +					CORE_VRD_PWR_REG, &val);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						CORE_VRD_PWR_MW_REG, &val_mw);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						SOC_PWR_REG, &val2);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						SOC_PWR_MW_REG, &val2_mw);
>> +			if (ret)
>> +				return ret;
>> +			break;
>> +		case DIMM_VRD_PWR:
>> +			ret = regmap_read(hwmon->regmap,
>> +					DIMM_VRD1_PWR_REG, &val);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						DIMM_VRD1_PWR_MW_REG, &val_mw);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						DIMM_VRD2_PWR_REG, &val2);
>> +			if (!ret)
>> +				ret = regmap_read(hwmon->regmap,
>> +						DIMM_VRD2_PWR_MW_REG, &val2_mw);
>> +			if (ret)
>> +				return ret;
>> +			break;
> 
> Ah, there are more "virtual" atributes. Again, please remove all those.
> If userspace w ants to group attributes in whatever way it wants to, fine,
> but that is not a kernel concern.
> 
> Besides, what if a DIMM is unpopulated ?
> 
>> +		default:
>> +			return -EOPNOTSUPP;
>> +		}
>> +		/* Set to 0 if invalid */
>> +		if (val_mw == 0xffff)
>> +			val_mw = 0;
>> +		if (val2_mw == 0xffff)
>> +			val2_mw = 0;
>> +
> 
> Does this indicate a temporary or a permanent problem ?
> If it means that the sensor is not populated, it should be
> handled in is_visible. Also, what about val / val2 ?
> 
This means that the sensor is not populated and will be handled in 
is_visible() in next version.

>> +		*val_pwr = (val + val2) * 1000000 + (val_mw + val2_mw) * 1000;
>> +		return 0;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int smpro_read(struct device *dev, enum hwmon_sensor_types type,
>> +				u32 attr, int channel, long *val)
>> +{
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		return smpro_read_temp(dev, attr, channel, val);
>> +	case hwmon_in:
>> +		return smpro_read_in(dev, attr, channel, val);
>> +	case hwmon_power:
>> +		return smpro_read_power(dev, attr, channel, val);
>> +	case hwmon_curr:
>> +		return smpro_read_curr(dev, attr, channel, val);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int smpro_write(struct device *dev, enum hwmon_sensor_types type,
>> +				u32 attr, int channel, long val)
>> +{
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static umode_t smpro_is_visible(const void *data,
>> +				enum hwmon_sensor_types type,
>> +				u32 attr, int channel)
>> +{
>> +	return 0444;
>> +}
>> +
>> +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 const u32 smpro_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,
>> +	HWMON_T_INPUT,
>> +	HWMON_T_INPUT,
>> +	HWMON_T_INPUT,
>> +	0
>> +};
>> +
>> +static const struct hwmon_channel_info smpro_temp = {
>> +	.type = hwmon_temp,
>> +	.config = smpro_temp_config,
>> +};
>> +
>> +static const u32 smpro_in_config[] = {
>> +	HWMON_I_INPUT,
>> +	HWMON_I_INPUT,
>> +	HWMON_I_INPUT,
>> +	HWMON_I_INPUT,
>> +	HWMON_I_INPUT,
>> +	HWMON_I_INPUT,
>> +	0
>> +};
>> +
>> +static const struct hwmon_channel_info smpro_in = {
>> +	.type = hwmon_in,
>> +	.config = smpro_in_config,
>> +};
>> +
>> +static const u32 smpro_curr_config[] = {
>> +	HWMON_C_INPUT,
>> +	HWMON_C_INPUT,
>> +	HWMON_C_INPUT,
>> +	HWMON_C_INPUT,
>> +	HWMON_C_INPUT,
>> +	0
>> +};
>> +
>> +static const struct hwmon_channel_info smpro_curr = {
>> +	.type = hwmon_curr,
>> +	.config = smpro_curr_config,
>> +};
>> +
>> +static const u32 smpro_power_config[] = {
>> +	HWMON_P_INPUT,
>> +	HWMON_P_INPUT,
>> +	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 smpro_power = {
>> +	.type = hwmon_power,
>> +	.config = smpro_power_config,
>> +};
>> +
>> +static const struct hwmon_channel_info *smpro_info[] = {
>> +	&smpro_temp,
>> +	&smpro_in,
>> +	&smpro_power,
>> +	&smpro_curr,
>> +	NULL
>> +};
>> +
> 
> The above can all be polulated with HWMON_CHANNEL_INFO().
> 
Will try this in next version.

>> +static const struct hwmon_ops smpro_hwmon_ops = {
>> +	.is_visible = smpro_is_visible,
>> +	.read = smpro_read,
>> +	.write = smpro_write,
>> +};
>> +
>> +static const struct hwmon_chip_info smpro_chip_info = {
>> +	.ops = &smpro_hwmon_ops,
>> +	.info = smpro_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, 19);
>> +static SENSOR_DEVICE_ATTR(temp6_label, 0444, show_label, NULL, 6);
>> +static SENSOR_DEVICE_ATTR(temp7_label, 0444, show_label, NULL, 7);
>> +static SENSOR_DEVICE_ATTR(temp8_label, 0444, show_label, NULL, 8);
>> +static SENSOR_DEVICE_ATTR(temp9_label, 0444, show_label, NULL, 9);
>> +static SENSOR_DEVICE_ATTR(temp10_label, 0444, show_label, NULL, 20);
>> +static SENSOR_DEVICE_ATTR(temp11_label, 0444, show_label, NULL, 10);
>> +static SENSOR_DEVICE_ATTR(temp12_label, 0444, show_label, NULL, 11);
>> +static SENSOR_DEVICE_ATTR(temp13_label, 0444, show_label, NULL, 12);
>> +static SENSOR_DEVICE_ATTR(temp14_label, 0444, show_label, NULL, 13);
>> +static SENSOR_DEVICE_ATTR(temp15_label, 0444, show_label, NULL, 14);
>> +static SENSOR_DEVICE_ATTR(temp16_label, 0444, show_label, NULL, 15);
>> +static SENSOR_DEVICE_ATTR(temp17_label, 0444, show_label, NULL, 17);
>> +
>> +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(in4_label, 0444, show_label, NULL, 2);
>> +static SENSOR_DEVICE_ATTR(in5_label, 0444, show_label, NULL, 17);
>> +
>> +static SENSOR_DEVICE_ATTR(power1_label, 0444, show_label, NULL, 5);
>> +static SENSOR_DEVICE_ATTR(power2_label, 0444, show_label, NULL, 0);
>> +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(power7_label, 0444, show_label, NULL, 17);
>> +static SENSOR_DEVICE_ATTR(power8_label, 0444, show_label, NULL, 18);
>> +
>> +static SENSOR_DEVICE_ATTR(curr1_label, 0444, show_label, NULL, 5);
>> +static SENSOR_DEVICE_ATTR(curr2_label, 0444, show_label, NULL, 1);
>> +static SENSOR_DEVICE_ATTR(curr3_label, 0444, show_label, NULL, 3);
>> +static SENSOR_DEVICE_ATTR(curr4_label, 0444, show_label, NULL, 4);
>> +static SENSOR_DEVICE_ATTR(curr5_label, 0444, show_label, NULL, 17);
>> +
>> +static struct attribute *smpro_attrs[] = {
>> +	&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_temp15_label.dev_attr.attr,
>> +	&sensor_dev_attr_temp16_label.dev_attr.attr,
>> +	&sensor_dev_attr_temp17_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_in4_label.dev_attr.attr,
>> +	&sensor_dev_attr_in5_label.dev_attr.attr,
>> +
>> +	&sensor_dev_attr_curr1_label.dev_attr.attr,
>> +	&sensor_dev_attr_curr2_label.dev_attr.attr,
>> +	&sensor_dev_attr_curr3_label.dev_attr.attr,
>> +	&sensor_dev_attr_curr4_label.dev_attr.attr,
>> +	&sensor_dev_attr_curr5_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_power7_label.dev_attr.attr,
>> +	&sensor_dev_attr_power8_label.dev_attr.attr,
>> +
>> +	NULL
>> +};
>> +ATTRIBUTE_GROUPS(smpro);
>> +
> 
> This can be done with LABEL attributes (HWMON_T_LABEL etc).
> 
Thanks, will try this next time.

>> +static int is_valid_id(struct regmap *regmap)
> 
> bool
> 
>> +{
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = regmap_read(regmap, MANUFACTURER_ID_REG, &val);
>> +
>> +	return  (ret || (val != AMPERE_MANUFACTURER_ID)) ? 0 : 1;
>> +}
>> +
>> +static int smpro_hwmon_probe(struct platform_device *pdev)
>> +{
>> +	struct smpro_hwmon *hwmon;
>> +	struct device *hwmon_dev;
>> +
>> +	hwmon = devm_kzalloc(&pdev->dev, sizeof(struct smpro_hwmon),
>> +			GFP_KERNEL);
>> +	if (!hwmon)
>> +		return -ENOMEM;
>> +
>> +	hwmon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	if (!hwmon->regmap)
>> +		return -ENODEV;
>> +
>> +	/* Check for valid ID */
>> +	if (!is_valid_id(hwmon->regmap))
>> +		dev_warn(&pdev->dev, "Hmmh, SMPro not ready yet\n");
> 
> This does not appear appropriate. It should be either -EPROBE_DEFER
> or -ENODEV.
> 

Will re-check

>> +
>> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
>> +			"smpro_hwmon", hwmon,
>> +			&smpro_chip_info, smpro_groups);
>> +	if (IS_ERR(hwmon_dev))
>> +		dev_err(&pdev->dev, "failed to register as hwmon device");
> 
> Unnecessary error message. It is either -EINVAL, which would
> always be the case, or -ENOMEM, which is already reported.
> 
Will drop this message in next version.

>> +
>> +	return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct of_device_id smpro_hwmon_of_match[] = {
>> +	{ .compatible = "ampere,ac01-hwmon" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, smpro_hwmon_of_match);
>> +
>> +static struct platform_driver smpro_hwmon_driver = {
>> +	.probe		= smpro_hwmon_probe,
>> +	.driver = {
>> +		.name	= "smpro-hwmon",
>> +		.of_match_table = smpro_hwmon_of_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(smpro_hwmon_driver);
>> +
>> +MODULE_AUTHOR("Thu Nguyen <thu@os.amperecomputing.com>");
>> +MODULE_AUTHOR("Quan Nguyen <quan@os.amperecomputing.com>");
>> +MODULE_DESCRIPTION("Ampere Altra SMPro hwmon driver");
>> +MODULE_LICENSE("GPL");
>>
> 

Re: [PATCH 4/4] docs: hwmon: (smpro-hwmon) Add documentation

[Quan Nguyen]

On 25/02/2021 23:42, Guenter Roeck wrote:
> On 2/25/21 2:18 AM, Quan Nguyen wrote:
>> Add documentation for the Ampere(R)'s Altra(R) SMpro hwmon driver.
>>
>> Signed-off-by: Thu Nguyen <thu@os.amperecomputing.com>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>>   Documentation/hwmon/index.rst       |   1 +
>>   Documentation/hwmon/smpro-hwmon.rst | 100 ++++++++++++++++++++++++++++
>>   2 files changed, 101 insertions(+)
>>   create mode 100644 Documentation/hwmon/smpro-hwmon.rst
>>
>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>> index 8d5a2df1ecb6..b48a980ed08b 100644
>> --- a/Documentation/hwmon/index.rst
>> +++ b/Documentation/hwmon/index.rst
>> @@ -164,6 +164,7 @@ Hardware Monitoring Kernel Drivers
>>      sis5595
>>      sl28cpld
>>      smm665
>> +   smpro-hwmon
> 
> "hwmon" seems a bit redundant here.
> 

Dear Guenter,

That is necessary for HWMON because SMpro supports monitoring and 
reporting various data not only hwmon-related data but also including 
RAS errors, and other miscellaneous information


>>      smsc47b397
>>      smsc47m192
>>      smsc47m1
>> diff --git a/Documentation/hwmon/smpro-hwmon.rst b/Documentation/hwmon/smpro-hwmon.rst
>> new file mode 100644
>> index 000000000000..d546b90982e5
>> --- /dev/null
>> +++ b/Documentation/hwmon/smpro-hwmon.rst
>> @@ -0,0 +1,100 @@
>> +.. SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +Kernel driver Ampere(R)'s Altra(R) SMpro hwmon
>> +==============================================
>> +
>> +Supported chips:
>> +
>> +  * Ampere(R) Altra(R)
>> +
>> +    Prefix: 'smpro'
>> +
>> +    Reference: Altra SoC BMC Interface Specification
>> +
>> +Author: Thu Nguyen <thu@os.amperecomputing.com>
>> +
>> +Description
>> +-----------
>> +This driver supports hardware monitoring for Ampere(R) Altra(R) SoC's based on the
>> +SMpro co-processor (SMpro).
>> +The following sensor types are supported by the driver:
>> +
>> +  * temperature
>> +  * voltage
>> +  * current
>> +  * power
>> +
>> +The SMpro interface provides the registers to query the various sensors and
>> +their values which are then exported to userspace by this driver.
>> +
>> +Usage Notes
>> +-----------
>> +
>> +SMpro hwmon driver creates two sysfs files for each sensor.
>> +
>> +* File ``<sensor_type><idx>_label`` reports the sensor label.
>> +* File ``<sensor_type><idx>_input`` returns the sensor value.
>> +
>> +The sysfs files are allocated in the SMpro root fs folder.
>> +There is one root folder for each SMpro instance.
>> +
>> +When the SoC is turned off, the driver is failed to read the registers.
>> +It returns TIMEDOUT Error(-110) for the read sensors.
>> +
> 
> Maybe better something like
> 
> When the SoC is turned off, the driver will fail to read registers
> and return -ETIMEDOUT.
> 
> Can that indeed happen ? That seems to be highly undesirable.
> 
Thanks Guenter,
Will apply this in next version.

>> +Sysfs entries
>> +-------------
>> +
>> +The following sysfs files are supported:
>> +
>> +* Ampere(R) Altra(R):
>> +
>> +===============    =============   ======= ===============================================
>> +Name        Unit        Perm    Description
>> +temp1_input     mili Celsius     RO    SoC temperature
> 
> s/mili/milli/ throughout
> 

Yes, thanks for catching this.

>> +temp2_input     mili Celsius     RO    Highest temperature reported by the SoC VRDs
>> +temp3_input     mili Celsius     RO    Highest temperature reported by the DIMM VRDs
>> +temp4_input     mili Celsius     RO    Highest temperature reported by the Core VRDs
> 
> What does "highest" stand for here ? Is it the _current_ highest
> temperature, added up by the hardware/firmware, or is it the historic
> highest temperature ? Historic data should be reported as tempX_highest.
> 
>> +temp5_input     mili Celsius     RO    Highest temperature of DIMM Channel 0 to 3
> 
> drop; reported individually.
> 
Will drop in next version

>> +temp6_input     mili Celsius     RO    Temperature of DIMM0 on CH0
>> +temp7_input     mili Celsius     RO    Temperature of DIMM0 on CH1
>> +temp8_input     mili Celsius     RO    Temperature of DIMM0 on CH2
>> +temp9_input     mili Celsius     RO    Temperature of DIMM0 on CH3
>> +temp10_input     mili Celsius     RO    Highest temperature of DIMM Channel 4 to 7
> 
> drop; reported individually.
>
Yes, this one as well.

>> +temp11_input     mili Celsius     RO    Temperature of DIMM0 on CH4
>> +temp12_input     mili Celsius     RO    Temperature of DIMM0 on CH5
>> +temp13_input     mili Celsius     RO    Temperature of DIMM0 on CH6
>> +temp14_input     mili Celsius     RO    Temperature of DIMM0 on CH7
>> +temp15_input     mili Celsius     RO    MEM HOT Threshold
>> +temp16_input     mili Celsius     RO    SoC VRD HOT Threshold
> 
> Report as tempX_max or tempX_crit, as appropriate (eg temp2_max or
> temp2_crit for SoC VRD HOT Threshold). If there is a single threshold
> temperature for all DIMMs, report the same limit value for all DIMM
> temperature sensors.
> 
>> +temp17_input     mili Celsius     RO    Highest temperature reported by the RCA VRD
> 
> Same question about "highest" as above. Either "highest" is
> inappropriate, or there are multiple RCA VRDs and only the
> highest temperature of those is reported (which should be
> explicitly stated).
> 
This definitely need to be re-considered. Will try your suggestion in 
next version.

>> +in0_input     mili Volt     RO    Core voltage
>> +in1_input     mili Volt     RO    SoC voltage
>> +in2_input     mili Volt     RO    DIMM VRD1 voltage
>> +in3_input     mili Volt     RO    DIMM VRD2 voltage
>> +in4_input     mili Volt     RO    Maximum voltage of DIMM VRD1 and VRD2
> 
> drop; reported individually.
> 
will drop as it is "virtual" attr.

>> +in5_input     mili Volt     RO    RCA VRD voltage
>> +cur1_input     mili Ampere     RO    Core VRD current
>> +cur2_input     mili Ampere     RO    SoC VRD current
>> +cur3_input     mili Ampere     RO    DIMM VRD1 current
>> +cur4_input     mili Ampere     RO    DIMM VRD2 current
>> +cur5_input     mili Ampere     RO    RCA VRD current
>> +power1_input     nano Wat     RO    Core VRD power
> 
> Expected scale is micro-Watt.
>
Will change in next version

>> +power2_input     nano Wat     RO    SoC VRD power
>> +power3_input     nano Wat     RO    DIMM VRD1 power
>> +power4_input     nano Wat     RO    DIMM VRD2 power
>> +power5_input     nano Wat     RO    CPU VRD power, total of SoC and Core VRD power
> 
> drop
>
OK

>> +power6_input     nano Wat     RO    Total of DIMM VRD1 and VRD2 power
> 
> drop
> 
OK

>> +power7_input     nano Wat     RO    RCA VRD power
>> +power8_input     nano Wat     RO    Socket TDP
> 
> Report as max attribute
>
Will try this in next version

>> +===============    =============   ======= ===============================================
>> +
>> +Example::
>> +
>> +    # cat in0_input
>> +    830
>> +    # cat temp1_input
>> +    37000
>> +    # cat curr1_input
>> +    9000
>> +    # cat power5_input
>> +    19500000
>>
> 

Re: [PATCH 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Quan Nguyen]

On 25/02/2021 23:47, Guenter Roeck wrote:
> On 2/25/21 2:18 AM, Quan Nguyen wrote:
>> Adds device tree bindings for SMPro drivers found on the Mt.Jade hardware
>> reference platform with Ampere's Altra Processor family.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>>   .../bindings/hwmon/ampere,ac01-hwmon.yaml     | 27 ++++++
>>   .../bindings/mfd/ampere,ac01-smpro.yaml       | 82 +++++++++++++++++++
>>   2 files changed, 109 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>>   create mode 100644 Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>> new file mode 100644
>> index 000000000000..d13862ba646b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>> @@ -0,0 +1,27 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Hardware monitoring driver for the Ampere Altra SMPro
>> +
>> +maintainers:
>> +  - Quan Nguyen <quan@os.amperecomputing.com>
>> +
>> +description: |
>> +  This module is part of the Ampere Altra SMPro multi-function device. For more
>> +  details see ../mfd/ampere,ac01-smpro.yaml.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ampere,ac01-hwmon
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> diff --git a/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
>> new file mode 100644
>> index 000000000000..06b0239413ae
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
>> @@ -0,0 +1,82 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/ampere,ac01-smpro.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Ampere Altra SMPro firmware driver
>> +
>> +maintainers:
>> +  - Quan Nguyen <quan@os.amperecomputing.com>
>> +
>> +description: |
>> +  Ampere Altra SMPro firmware may contain different blocks like hardware
>> +  monitoring, error monitoring and other miscellaneous features.
>> +
>> +properties:
>> +  compatible:
>> +    const: ampere,ac01-smpro
> 
> Is that the same as the "ampere,smpro" in
> arch/arm/boot/dts/nuvoton-npcm730-kudo.dts ?
> 
> Guenter
> 
Yes, Guenter, looks like this need to change in next version.

Re: [PATCH 2/4] mfd: simple-mfd-i2c: Adds Ampere's Altra SMpro support

[Quan Nguyen]

On 26/02/2021 15:31, Lee Jones wrote:
> On Thu, 25 Feb 2021, Quan Nguyen wrote:
> 
>> Adds an MFD driver for SMpro found on the Mt.Jade hardware reference
>> platform with Ampere's Altra processor family.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>>   drivers/mfd/Kconfig          | 10 ++++++++++
>>   drivers/mfd/simple-mfd-i2c.c | 15 +++++++++++++--
>>   2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index b74efa469e90..5414371bdea1 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -77,6 +77,16 @@ config MFD_AS3711
>>   	help
>>   	  Support for the AS3711 PMIC from AMS
>>   
>> +config MFD_SMPRO
>> +	tristate "Ampere Computing MFD SMpro core driver"
>> +	select MFD_SIMPLE_MFD_I2C
> 
> Nice to see another user here.
> 
>> +	help
>> +	  Say yes here to enable SMpro driver support for Ampere's Altra
>> +	  processor family.
>> +
>> +	  Ampere's Altra SMpro exposes an I2C regmap interface that can
>> +	  be accessed by child devices.
>> +
>>   config MFD_AS3722
>>   	tristate "ams AS3722 Power Management IC"
>>   	select MFD_CORE
>> diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
>> index 87f684cff9a1..0459a9fbd3f5 100644
>> --- a/drivers/mfd/simple-mfd-i2c.c
>> +++ b/drivers/mfd/simple-mfd-i2c.c
>> @@ -21,14 +21,24 @@ static const struct regmap_config simple_regmap_config = {
>>   	.val_bits = 8,
>>   };
>>   
>> +static const struct regmap_config simple_word_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 16,
>> +};
>> +
>>   static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>>   {
>>   	const struct regmap_config *config;
>>   	struct regmap *regmap;
>>   
>>   	config = device_get_match_data(&i2c->dev);
>> -	if (!config)
>> -		config = &simple_regmap_config;
>> +	if (!config) {
>> +		if (of_device_is_compatible(i2c->dev.of_node,
>> +						"ampere,ac01-smpro"))
> 
> Could you use 'struct of_device_id's .data attribute instead please?
> 
Thank you, but I'm not sure if I got it right.

Do you mean we could use .data attribute to get the expected 
reg_bits/val_bits values and translate them to simple_word_regmap_config 
without the need to match the compatible string ?

>> +			config = &simple_word_regmap_config;
>> +		else
>> +			config = &simple_regmap_config;
>> +	}
>>   
>>   	regmap = devm_regmap_init_i2c(i2c, config);
>>   	if (IS_ERR(regmap))
>> @@ -39,6 +49,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>>   
>>   static const struct of_device_id simple_mfd_i2c_of_match[] = {
>>   	{ .compatible = "kontron,sl28cpld" },
>> +	{ .compatible = "ampere,ac01-smpro" },
>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);
> 

Re: [PATCH 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Rob Herring]

On Thu, Feb 25, 2021 at 05:18:51PM +0700, Quan Nguyen wrote:
> Adds device tree bindings for SMPro drivers found on the Mt.Jade hardware
> reference platform with Ampere's Altra Processor family.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
>  .../bindings/hwmon/ampere,ac01-hwmon.yaml     | 27 ++++++
>  .../bindings/mfd/ampere,ac01-smpro.yaml       | 82 +++++++++++++++++++
>  2 files changed, 109 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>  create mode 100644 Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
> new file mode 100644
> index 000000000000..d13862ba646b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
> @@ -0,0 +1,27 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Hardware monitoring driver for the Ampere Altra SMPro
> +
> +maintainers:
> +  - Quan Nguyen <quan@os.amperecomputing.com>
> +
> +description: |
> +  This module is part of the Ampere Altra SMPro multi-function device. For more
> +  details see ../mfd/ampere,ac01-smpro.yaml.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ampere,ac01-hwmon
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
> new file mode 100644
> index 000000000000..06b0239413ae
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ampere,ac01-smpro.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ampere Altra SMPro firmware driver
> +
> +maintainers:
> +  - Quan Nguyen <quan@os.amperecomputing.com>
> +
> +description: |
> +  Ampere Altra SMPro firmware may contain different blocks like hardware
> +  monitoring, error monitoring and other miscellaneous features.
> +
> +properties:
> +  compatible:
> +    const: ampere,ac01-smpro
> +
> +  reg:
> +    description:
> +      I2C device address.
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^hwmon(@[0-9a-f]+)?$":
> +    $ref: ../hwmon/ampere,ac01-hwmon.yaml
> +
> +  "^misc(@[0-9a-f]+)?$":
> +    type: object
> +    description: Ampere Altra SMPro Misc driver
> +    properties:
> +      compatible:
> +        const: "ampere,ac01-misc"
> +
> +  "^errmon(@[0-9a-f]+)?$":
> +    type: object
> +    description: Ampere Altra SMPro Error Monitor driver
> +    properties:
> +      compatible:
> +        const: "ampere,ac01-errmon"
> +
> +required:
> +  - "#address-cells"
> +  - "#size-cells"
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        smpro@4f {
> +            compatible = "ampere,ac01-smpro";
> +            reg = <0x4f>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            hwmon {
> +                compatible = "ampere,ac01-hwmon";
> +            };
> +
> +            misc {
> +                compatible = "ampere,ac01-misc";
> +            };
> +
> +            errmon {
> +                compatible = "ampere,ac01-errmon";
> +            };

No of these have any properties or resources, why do you need them? DT 
is not the only way to instantiate drivers...

Rob

Re: [PATCH 1/4] dt-bindings: mfd: Add bindings for Ampere Altra SMPro drivers

[Quan Nguyen]

On 07/03/2021 03:58, Rob Herring wrote:
> On Thu, Feb 25, 2021 at 05:18:51PM +0700, Quan Nguyen wrote:
>> Adds device tree bindings for SMPro drivers found on the Mt.Jade hardware
>> reference platform with Ampere's Altra Processor family.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>>   .../bindings/hwmon/ampere,ac01-hwmon.yaml     | 27 ++++++
>>   .../bindings/mfd/ampere,ac01-smpro.yaml       | 82 +++++++++++++++++++
>>   2 files changed, 109 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>>   create mode 100644 Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>> new file mode 100644
>> index 000000000000..d13862ba646b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/ampere,ac01-hwmon.yaml
>> @@ -0,0 +1,27 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/ampere,ac01-hwmon.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Hardware monitoring driver for the Ampere Altra SMPro
>> +
>> +maintainers:
>> +  - Quan Nguyen <quan@os.amperecomputing.com>
>> +
>> +description: |
>> +  This module is part of the Ampere Altra SMPro multi-function device. For more
>> +  details see ../mfd/ampere,ac01-smpro.yaml.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ampere,ac01-hwmon
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> diff --git a/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
>> new file mode 100644
>> index 000000000000..06b0239413ae
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/ampere,ac01-smpro.yaml
>> @@ -0,0 +1,82 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/ampere,ac01-smpro.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Ampere Altra SMPro firmware driver
>> +
>> +maintainers:
>> +  - Quan Nguyen <quan@os.amperecomputing.com>
>> +
>> +description: |
>> +  Ampere Altra SMPro firmware may contain different blocks like hardware
>> +  monitoring, error monitoring and other miscellaneous features.
>> +
>> +properties:
>> +  compatible:
>> +    const: ampere,ac01-smpro
>> +
>> +  reg:
>> +    description:
>> +      I2C device address.
>> +    maxItems: 1
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 0
>> +
>> +patternProperties:
>> +  "^hwmon(@[0-9a-f]+)?$":
>> +    $ref: ../hwmon/ampere,ac01-hwmon.yaml
>> +
>> +  "^misc(@[0-9a-f]+)?$":
>> +    type: object
>> +    description: Ampere Altra SMPro Misc driver
>> +    properties:
>> +      compatible:
>> +        const: "ampere,ac01-misc"
>> +
>> +  "^errmon(@[0-9a-f]+)?$":
>> +    type: object
>> +    description: Ampere Altra SMPro Error Monitor driver
>> +    properties:
>> +      compatible:
>> +        const: "ampere,ac01-errmon"
>> +
>> +required:
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        smpro@4f {
>> +            compatible = "ampere,ac01-smpro";
>> +            reg = <0x4f>;
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            hwmon {
>> +                compatible = "ampere,ac01-hwmon";
>> +            };
>> +
>> +            misc {
>> +                compatible = "ampere,ac01-misc";
>> +            };
>> +
>> +            errmon {
>> +                compatible = "ampere,ac01-errmon";
>> +            };
> 
> No of these have any properties or resources, why do you need them? DT
> is not the only way to instantiate drivers...
> 
Thanks Rob,

SMpro (MFD driver) reports various data included hwmon-related data, RAS 
error monitor and other miscellaneous information, and we need three 
difference drivers for these purposes. And that is why hwmon, misc and 
errmon nodes are added to instantiate these drivers.

I'm wonder if this is the right way or if there's anything that can be 
improved ?

- Quan