[PATCH v8 0/4] riscv: sophgo: Add SG2042 external hardware monitor support

[PATCH v8 0/4] riscv: sophgo: Add SG2042 external hardware monitor support

[Inochi Amaoto]

Add support for the onboard hardware monitor for SG2042.
Can be tested with OpenSBI v1.5.

The patch require the following i2c patch:
https://lore.kernel.org/all/IA1PR20MB4953DB82FB7D75BF8409FFF4BBB72@IA1PR20MB4953.namprd20.prod.outlook.com/

Changed from v7:
1. add mutex protected and fix the return value when writing
"critical_action"

Changed from v6:
1. restore the driver name to sg2042-mcu
2. remove unnecessary wrap function and check in the driver.
3. add dts and config entry.

Changed from v5:
1. rename driver name to sgmcu as it will support more sophgo chip.
2. move some attr to debugfs.
3. add standard crit_hyst support
4. add documentation

Changed from v4:
1. use fix patch for binding ref.
2. use unevaluatedProperties instead of additionalProperties for binding

Changed from v3:
1. add thermal-sensor check.
2. change node type from syscon to hwmon

Changed from v2:
1. fix bindings id path.

Changed from v1:
1. Move patch from soc to hwmon.
2. Fix typo.

Inochi Amaoto (4):
  dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor
    support
  drivers: hwmon: sophgo: Add SG2042 external hardware monitor support
  riscv: dts: sophgo: Add mcu device for Milk-V Pioneer
  riscv: defconfig: Enable MCU support for SG2042

 .../hwmon/sophgo,sg2042-hwmon-mcu.yaml        |  43 ++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/sg2042-mcu.rst            |  39 ++
 .../boot/dts/sophgo/sg2042-milkv-pioneer.dts  |  60 +++
 arch/riscv/configs/defconfig                  |   1 +
 drivers/hwmon/Kconfig                         |  11 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/sg2042-mcu.c                    | 406 ++++++++++++++++++
 8 files changed, 562 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
 create mode 100644 Documentation/hwmon/sg2042-mcu.rst
 create mode 100644 drivers/hwmon/sg2042-mcu.c

--
2.45.2

[PATCH v8 1/4] dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor support

[Inochi Amaoto]

Due to the design, Sophgo SG2042 use an external MCU to provide
hardware information, thermal information and reset control.

Add bindings for this monitor device.

Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../hwmon/sophgo,sg2042-hwmon-mcu.yaml        | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
new file mode 100644
index 000000000000..f0667ac41d75
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/sophgo,sg2042-hwmon-mcu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 onboard MCU support
+
+maintainers:
+  - Inochi Amaoto <inochiama@outlook.com>
+
+properties:
+  compatible:
+    const: sophgo,sg2042-hwmon-mcu
+
+  reg:
+    maxItems: 1
+
+  "#thermal-sensor-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#thermal-sensor-cells"
+
+allOf:
+  - $ref: /schemas/thermal/thermal-sensor.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        hwmon@17 {
+            compatible = "sophgo,sg2042-hwmon-mcu";
+            reg = <0x17>;
+            #thermal-sensor-cells = <1>;
+        };
+    };
--
2.45.2

[PATCH v8 2/4] drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

[Inochi Amaoto]

SG2042 use an external MCU to provide basic hardware information
and thermal sensors.

Add driver support for the onboard MCU of SG2042.

Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
---
 Documentation/hwmon/index.rst      |   1 +
 Documentation/hwmon/sg2042-mcu.rst |  39 +++
 drivers/hwmon/Kconfig              |  11 +
 drivers/hwmon/Makefile             |   1 +
 drivers/hwmon/sg2042-mcu.c         | 406 +++++++++++++++++++++++++++++
 5 files changed, 458 insertions(+)
 create mode 100644 Documentation/hwmon/sg2042-mcu.rst
 create mode 100644 drivers/hwmon/sg2042-mcu.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 913c11390a45..ea3b5be8fe4f 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -206,6 +206,7 @@ Hardware Monitoring Kernel Drivers
    sch5636
    scpi-hwmon
    sfctemp
+   sg2042-mcu
    sht15
    sht21
    sht3x
diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
new file mode 100644
index 000000000000..250016b47dd1
--- /dev/null
+++ b/Documentation/hwmon/sg2042-mcu.rst
@@ -0,0 +1,39 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver sg2042-mcu
+=====================
+
+Supported chips:
+
+  * Onboard MCU for sg2042
+
+    Addresses scanned: -
+
+    Prefix: 'sg2042-mcu'
+
+Authors:
+
+  - Inochi Amaoto <inochiama@outlook.com>
+
+Description
+-----------
+
+This driver supprts hardware monitoring for onboard MCU with
+i2c interface.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate
+the devices explicitly.
+Please see Documentation/i2c/instantiating-devices.rst for details.
+
+Sysfs Attributes
+----------------
+
+================= =============================================
+temp1_input       Measured temperature of SoC
+temp1_crit        Critical high temperature
+temp1_crit_hyst   hysteresis temperature restore from Critical
+temp2_input       Measured temperature of the base board
+================= =============================================
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b60fe2e58ad6..7aa6c3f322e5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2066,6 +2066,17 @@ config SENSORS_SFCTEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called sfctemp.

+config SENSORS_SG2042_MCU
+	tristate "Sophgo onboard MCU support"
+	depends on I2C
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
+	  power control and some basic information.
+
+	  This driver can be built as a module. If so, the module
+	  will be called sg2042-mcu.
+
 config SENSORS_SURFACE_FAN
 	tristate "Surface Fan Driver"
 	depends on SURFACE_AGGREGATOR
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b1c7056c37db..0bbe812a67ae 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
 obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
 obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
 obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
+obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
 obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
new file mode 100644
index 000000000000..37fac3e5f233
--- /dev/null
+++ b/drivers/hwmon/sg2042-mcu.c
@@ -0,0 +1,406 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Inochi Amaoto <inochiama@outlook.com>
+ *
+ * Sophgo power control mcu for SG2042
+ */
+
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+
+/* fixed MCU registers */
+#define REG_BOARD_TYPE				0x00
+#define REG_MCU_FIRMWARE_VERSION		0x01
+#define REG_PCB_VERSION				0x02
+#define REG_PWR_CTRL				0x03
+#define REG_SOC_TEMP				0x04
+#define REG_BOARD_TEMP				0x05
+#define REG_RST_COUNT				0x0a
+#define REG_UPTIME				0x0b
+#define REG_RESET_REASON			0x0d
+#define REG_MCU_TYPE				0x18
+#define REG_CRITICAL_ACTIONS			0x65
+#define REG_CRITICAL_TEMP			0x66
+#define REG_REPOWER_TEMP			0x67
+
+#define CRITICAL_ACTION_REBOOT			1
+#define CRITICAL_ACTION_POWEROFF		2
+
+#define MCU_POWER_MAX				0xff
+
+#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format)			\
+	static int _name##_show(struct seq_file *seqf,			\
+				    void *unused)			\
+	{								\
+		struct sg2042_mcu_data *mcu = seqf->private;		\
+		int ret;						\
+		ret = i2c_smbus_read_byte_data(mcu->client, (_reg));	\
+		if (ret < 0)						\
+			return ret;					\
+		seq_printf(seqf, _format "\n", ret);			\
+		return 0;						\
+	}								\
+	DEFINE_SHOW_ATTRIBUTE(_name)					\
+
+struct sg2042_mcu_data {
+	struct i2c_client	*client;
+	struct dentry		*debugfs;
+	struct mutex		mutex;
+};
+
+static struct dentry *sgmcu_debugfs;
+
+static ssize_t reset_count_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", ret);
+}
+
+static ssize_t uptime_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	u8 time_val[2];
+	int ret;
+
+	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
+					    sizeof(time_val), time_val);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n",
+		       (int)(time_val[0]) + (int)(time_val[1] << 8));
+}
+
+static ssize_t reset_reason_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "0x%02x\n", ret);
+}
+
+static ssize_t critical_action_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+	const char *action;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_CRITICAL_ACTIONS);
+	if (ret < 0)
+		return ret;
+
+	if (ret == CRITICAL_ACTION_REBOOT)
+		action = "reboot";
+	else if (ret == CRITICAL_ACTION_POWEROFF)
+		action = "poweroff";
+	else
+		action = "unknown";
+
+	return sprintf(buf, "%s\n", action);
+}
+
+static ssize_t critical_action_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	u8 value;
+	int ret;
+
+	if (sysfs_streq("reboot", buf))
+		value = CRITICAL_ACTION_REBOOT;
+	else if (sysfs_streq("poweroff", buf))
+		value = CRITICAL_ACTION_POWEROFF;
+	else
+		return -EINVAL;
+
+	mutex_lock(&mcu->mutex);
+	ret = i2c_smbus_write_byte_data(mcu->client,
+					REG_CRITICAL_ACTIONS, value);
+	mutex_unlock(&mcu->mutex);
+
+	if (ret < 0)
+		return ret;
+	return count;
+}
+
+static DEVICE_ATTR_RO(reset_count);
+static DEVICE_ATTR_RO(uptime);
+static DEVICE_ATTR_RO(reset_reason);
+static DEVICE_ATTR_RW(critical_action);
+
+DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
+
+static struct attribute *sg2042_mcu_attrs[] = {
+	&dev_attr_reset_count.attr,
+	&dev_attr_uptime.attr,
+	&dev_attr_reset_reason.attr,
+	&dev_attr_critical_action.attr,
+	NULL
+};
+
+static const struct attribute_group sg2042_mcu_attr_group = {
+	.attrs	= sg2042_mcu_attrs,
+};
+
+static const struct attribute_group *sg2042_mcu_groups[] = {
+	&sg2042_mcu_attr_group,
+	NULL
+};
+
+static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
+					HWMON_T_CRIT_HYST,
+				 HWMON_T_INPUT),
+	NULL
+};
+
+static int sg2042_mcu_read_temp(struct device *dev,
+				u32 attr, int channel,
+				long *val)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int tmp;
+	u8 reg;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
+		break;
+	case hwmon_temp_crit:
+		reg = REG_CRITICAL_TEMP;
+		break;
+	case hwmon_temp_crit_hyst:
+		reg = REG_REPOWER_TEMP;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
+	if (tmp < 0)
+		return tmp;
+	*val = tmp * 1000;
+
+	return 0;
+}
+
+static int sg2042_mcu_read(struct device *dev,
+			   enum hwmon_sensor_types type,
+			   u32 attr, int channel, long *val)
+{
+	return sg2042_mcu_read_temp(dev, attr, channel, val);
+}
+
+static int sg2042_mcu_write(struct device *dev,
+			    enum hwmon_sensor_types type,
+			    u32 attr, int channel, long val)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int temp = val / 1000;
+	int hyst_temp, crit_temp;
+	int ret;
+	u8 reg;
+
+	if (temp > MCU_POWER_MAX)
+		temp = MCU_POWER_MAX;
+
+	mutex_lock(&mcu->mutex);
+
+	switch (attr) {
+	case hwmon_temp_crit:
+		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
+						     REG_REPOWER_TEMP);
+		if (hyst_temp < 0) {
+			ret = -ENODEV;
+			goto failed;
+		}
+
+		crit_temp = temp;
+		reg = REG_CRITICAL_TEMP;
+		break;
+	case hwmon_temp_crit_hyst:
+		crit_temp = i2c_smbus_read_byte_data(mcu->client,
+						     REG_CRITICAL_TEMP);
+		if (crit_temp < 0) {
+			ret = -ENODEV;
+			goto failed;
+		}
+
+		hyst_temp = temp;
+		reg = REG_REPOWER_TEMP;
+		break;
+	default:
+		mutex_unlock(&mcu->mutex);
+		return -EOPNOTSUPP;
+	}
+
+	if (crit_temp < hyst_temp) {
+		ret = -EINVAL;
+		goto failed;
+	}
+
+	ret = i2c_smbus_write_byte_data(mcu->client, reg, temp);
+
+failed:
+	mutex_unlock(&mcu->mutex);
+	return ret;
+}
+
+static umode_t sg2042_mcu_is_visible(const void *_data,
+				     enum hwmon_sensor_types type,
+				     u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return 0444;
+		case hwmon_temp_crit:
+		case hwmon_temp_crit_hyst:
+			if (channel == 0)
+				return 0664;
+			break;
+		default:
+			return 0;
+		}
+		break;
+	default:
+		return 0;
+	}
+	return 0;
+}
+
+static const struct hwmon_ops sg2042_mcu_ops = {
+	.is_visible = sg2042_mcu_is_visible,
+	.read = sg2042_mcu_read,
+	.write = sg2042_mcu_write,
+};
+
+static const struct hwmon_chip_info sg2042_mcu_chip_info = {
+	.ops = &sg2042_mcu_ops,
+	.info = sg2042_mcu_info,
+};
+
+static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
+				    struct device *dev)
+{
+	mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
+	if (mcu->debugfs) {
+		debugfs_create_file("firmware_version", 0444, mcu->debugfs,
+				    mcu, &firmware_version_fops);
+		debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
+				    &pcb_version_fops);
+		debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
+				    &mcu_type_fops);
+		debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
+				    &board_type_fops);
+	}
+}
+
+static int sg2042_mcu_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct sg2042_mcu_data *mcu;
+	struct device *hwmon_dev;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+						I2C_FUNC_SMBUS_BLOCK_DATA))
+		return -EIO;
+
+	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
+	if (!mcu)
+		return -ENOMEM;
+
+	mutex_init(&mcu->mutex);
+	mcu->client = client;
+
+	i2c_set_clientdata(client, mcu);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
+							 mcu,
+							 &sg2042_mcu_chip_info,
+							 NULL);
+	if (IS_ERR_OR_NULL(hwmon_dev))
+		return -EFAULT;
+
+	sg2042_mcu_debugfs_init(mcu, dev);
+
+	return 0;
+}
+
+static void sg2042_mcu_i2c_remove(struct i2c_client *client)
+{
+	struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
+
+	debugfs_remove_recursive(mcu->debugfs);
+}
+
+static const struct i2c_device_id sg2042_mcu_id[] = {
+	{ "sg2042-hwmon-mcu", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
+
+static const struct of_device_id sg2042_mcu_of_id[] = {
+	{ .compatible = "sophgo,sg2042-hwmon-mcu" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
+
+static struct i2c_driver sg2042_mcu_driver = {
+	.driver = {
+		.name = "sg2042-mcu",
+		.of_match_table = sg2042_mcu_of_id,
+		.dev_groups = sg2042_mcu_groups,
+	},
+	.probe = sg2042_mcu_i2c_probe,
+	.remove = sg2042_mcu_i2c_remove,
+	.id_table = sg2042_mcu_id,
+};
+
+static int __init sg2042_mcu_init(void)
+{
+	sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
+	return i2c_add_driver(&sg2042_mcu_driver);
+}
+
+static void __exit sg2042_mcu_exit(void)
+{
+	debugfs_remove_recursive(sgmcu_debugfs);
+	i2c_del_driver(&sg2042_mcu_driver);
+}
+
+module_init(sg2042_mcu_init);
+module_exit(sg2042_mcu_exit);
+
+MODULE_AUTHOR("Inochi Amaoto <inochiama@outlook.com>");
+MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
+MODULE_LICENSE("GPL");
--
2.45.2

[PATCH v8 3/4] riscv: dts: sophgo: Add mcu device for Milk-V Pioneer

[Inochi Amaoto]

Add mcu device and thermal zones node for Milk-V Pioneer.

Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
---
 .../boot/dts/sophgo/sg2042-milkv-pioneer.dts  | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
index 80cb017974d8..8b8fdf6243d4 100644
--- a/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
+++ b/arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts
@@ -26,6 +26,66 @@ &cgi_dpll1 {
 	clock-frequency = <25000000>;
 };

+&i2c1 {
+	status = "okay";
+
+	mcu: syscon@17 {
+		compatible = "sophgo,sg2042-hwmon-mcu";
+		reg = <0x17>;
+		#thermal-sensor-cells = <1>;
+	};
+};
+
 &uart0 {
 	status = "okay";
 };
+
+/ {
+	thermal-zones {
+		soc-thermal {
+			polling-delay-passive = <1000>;
+			polling-delay = <1000>;
+			thermal-sensors = <&mcu 0>;
+
+			trips {
+				soc_active1: soc-active1 {
+					temperature = <30000>;
+					hysteresis = <8000>;
+					type = "active";
+				};
+
+				soc_active2: soc-active2 {
+					temperature = <58000>;
+					hysteresis = <12000>;
+					type = "active";
+				};
+
+				soc_active3: soc-active3 {
+					temperature = <70000>;
+					hysteresis = <10000>;
+					type = "active";
+				};
+
+				soc_hot: soc-hot {
+					temperature = <85000>;
+					hysteresis = <5000>;
+					type = "hot";
+				};
+			};
+		};
+
+		board-thermal {
+			polling-delay-passive = <1000>;
+			polling-delay = <1000>;
+			thermal-sensors = <&mcu 1>;
+
+			trips {
+				board_active: board-active {
+					temperature = <75000>;
+					hysteresis = <8000>;
+					type = "active";
+				};
+			};
+		};
+	};
+};
--
2.45.2

[PATCH v8 4/4] riscv: defconfig: Enable MCU support for SG2042

[Inochi Amaoto]

Enable MCU driver for SG2042 to provide thermal and reboot support.

Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
---
 arch/riscv/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index 0d678325444f..a0f346301df6 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -170,6 +170,7 @@ CONFIG_SPI_SUN6I=y
 CONFIG_GPIO_SIFIVE=y
 CONFIG_POWER_RESET_GPIO_RESTART=y
 CONFIG_SENSORS_SFCTEMP=m
+CONFIG_SENSORS_SG2042_MCU=y
 CONFIG_CPU_THERMAL=y
 CONFIG_DEVFREQ_THERMAL=y
 CONFIG_RZG2L_THERMAL=y
--
2.45.2

Re: [PATCH v8 3/4] riscv: dts: sophgo: Add mcu device for Milk-V Pioneer

[kernel test robot]

Hi Inochi,

kernel test robot noticed the following build errors:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on robh/for-next linus/master v6.11-rc1 next-20240730]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Inochi-Amaoto/dt-bindings-hwmon-Add-Sophgo-SG2042-external-hardware-monitor-support/20240730-160416
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/IA1PR20MB4953826DECDCC141A7CDE634BBB02%40IA1PR20MB4953.namprd20.prod.outlook.com
patch subject: [PATCH v8 3/4] riscv: dts: sophgo: Add mcu device for Milk-V Pioneer
config: riscv-randconfig-051-20240730 (https://download.01.org/0day-ci/archive/20240730/202407301749.K0pFCNLU-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 14.1.0
dtschema version: 2024.6.dev4+g23441a4
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240730/202407301749.K0pFCNLU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407301749.K0pFCNLU-lkp@intel.com/

All errors (new ones prefixed by >>):

>> Error: arch/riscv/boot/dts/sophgo/sg2042-milkv-pioneer.dts:29.1-6 Label or path i2c1 not found
   FATAL ERROR: Syntax error parsing input tree

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v8 2/4] drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

[Chen Wang]

On 2024/7/30 15:50, Inochi Amaoto wrote:
[......]
> +#define REG_CRITICAL_ACTIONS			0x65
The name "REG_CRITICAL_ACTIONS" is ambiguous. I have confirmed with 
sophgo engineers that the complete process is: when the measured 
temperature exceeds the temperature set by REG_CRITICAL_TEMP, the 
processor is powered off and shut down, and then after the temperature 
returns to the temperature set by REG_REPOWER_TEMP, it is decided 
whether to power on again or remain in the shutdown state based on the 
action set by REG_CRITICAL_ACTIONS, whether it is reboot or poweroff.

So based on the above description, I think it would be better to 
call "REG_CRITICAL_ACTIONS" as "REG_REPOWER_ACTIONS". 
"REG_CRITICAL_ACTIONS" gives people the first impression that it is used 
to set actions related to REG_CRITICAL_TEMP.

It is also recommended to add the above description of temperature 
control and action settings in the code. Currently, sophgo does not have 
a clear document description for this part, and adding it will help us 
understand its functions.

Adding sophgo engineers Chunzhi and Haijiao, FYI.

> +#define REG_CRITICAL_TEMP			0x66
> +#define REG_REPOWER_TEMP			0x67
> +
> +#define CRITICAL_ACTION_REBOOT			1
> +#define CRITICAL_ACTION_POWEROFF		2

As I said upon, actions are not related to critical, but is for 
restoring from critical, suggest to give a better name.

[......]

> +static ssize_t critical_action_show(struct device *dev,
[......]
> +static ssize_t critical_action_store(struct device *dev,

[......]

The same reason as upon, "critical_action_xxx" is misleading.

[......]

> +static int sg2042_mcu_read_temp(struct device *dev,
> +				u32 attr, int channel,
> +				long *val)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int tmp;
> +	u8 reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> +		break;
> +	case hwmon_temp_crit:
> +		reg = REG_CRITICAL_TEMP;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		reg = REG_REPOWER_TEMP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> +	if (tmp < 0)
> +		return tmp;
> +	*val = tmp * 1000;
> +
> +	return 0;
> +}
> +
> +static int sg2042_mcu_read(struct device *dev,
> +			   enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	return sg2042_mcu_read_temp(dev, attr, channel, val);
> +}
Can we merge sg2042_mcu_read and sg2042_mcu_read_temp？
> +
> +static int sg2042_mcu_write(struct device *dev,
> +			    enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long val)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int temp = val / 1000;
> +	int hyst_temp, crit_temp;
> +	int ret;
> +	u8 reg;
> +
> +	if (temp > MCU_POWER_MAX)
> +		temp = MCU_POWER_MAX;
> +
> +	mutex_lock(&mcu->mutex);
> +
> +	switch (attr) {
> +	case hwmon_temp_crit:
> +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> +						     REG_REPOWER_TEMP);
> +		if (hyst_temp < 0) {
> +			ret = -ENODEV;
> +			goto failed;
> +		}
> +
> +		crit_temp = temp;
> +		reg = REG_CRITICAL_TEMP;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> +						     REG_CRITICAL_TEMP);
> +		if (crit_temp < 0) {
> +			ret = -ENODEV;
> +			goto failed;
> +		}
> +
> +		hyst_temp = temp;
> +		reg = REG_REPOWER_TEMP;
> +		break;
> +	default:
> +		mutex_unlock(&mcu->mutex);
> +		return -EOPNOTSUPP;
> +	}
> +
It is recommended to add some comments to explain why we need to ensure 
that crit_temp is greater than or equal to hyst_temp. This is entirely 
because the current MCU does not limit the input, which may cause user 
to set incorrect crit_temp and hyst_temp.
> +	if (crit_temp < hyst_temp) {
> +		ret = -EINVAL;
> +		goto failed;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(mcu->client, reg, temp);
> +
> +failed:
> +	mutex_unlock(&mcu->mutex);
> +	return ret;
> +}
> +
[......]

Re: [PATCH v8 0/4] riscv: sophgo: Add SG2042 external hardware monitor support

[Chen Wang]

On 2024/7/30 15:49, Inochi Amaoto wrote:
> Add support for the onboard hardware monitor for SG2042.
> Can be tested with OpenSBI v1.5.
>
> The patch require the following i2c patch:
> https://lore.kernel.org/all/IA1PR20MB4953DB82FB7D75BF8409FFF4BBB72@IA1PR20MB4953.namprd20.prod.outlook.com/

Please check my comments on patch 2 of this serials.

Others are LGTM.

Tested-by: Chen Wang <unicorn_wang@outlook.com>

Reviewed-by: Chen Wang <unicorn_wang@outlook.com>

[......]

Re: [PATCH v8 2/4] drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

[Inochi Amaoto]

On Wed, Jul 31, 2024 at 02:13:20PM GMT, Chen Wang wrote:
> 
> On 2024/7/30 15:50, Inochi Amaoto wrote:
> [......]
> > +#define REG_CRITICAL_ACTIONS			0x65
> The name "REG_CRITICAL_ACTIONS" is ambiguous. I have confirmed with sophgo
> engineers that the complete process is: when the measured temperature
> exceeds the temperature set by REG_CRITICAL_TEMP, the processor is powered
> off and shut down, and then after the temperature returns to the temperature
> set by REG_REPOWER_TEMP, it is decided whether to power on again or remain
> in the shutdown state based on the action set by REG_CRITICAL_ACTIONS,
> whether it is reboot or poweroff.
> 
> So based on the above description, I think it would be better to
> call "REG_CRITICAL_ACTIONS" as "REG_REPOWER_ACTIONS". "REG_CRITICAL_ACTIONS"
> gives people the first impression that it is used to set actions related to
> REG_CRITICAL_TEMP.
> 
> It is also recommended to add the above description of temperature control
> and action settings in the code. Currently, sophgo does not have a clear
> document description for this part, and adding it will help us understand
> its functions.
> 
> Adding sophgo engineers Chunzhi and Haijiao, FYI.
> 
> > +#define REG_CRITICAL_TEMP			0x66
> > +#define REG_REPOWER_TEMP			0x67
> > +
> > +#define CRITICAL_ACTION_REBOOT			1
> > +#define CRITICAL_ACTION_POWEROFF		2
> 
> As I said upon, actions are not related to critical, but is for restoring
> from critical, suggest to give a better name.
> 
> [......]
> 
> > +static ssize_t critical_action_show(struct device *dev,
> [......]
> > +static ssize_t critical_action_store(struct device *dev,
> 
> [......]
> 
> The same reason as upon, "critical_action_xxx" is misleading.
> 
> [......]
> 

Thanks for explanation, I just get the name from the driver of SG2042.
This is out of my knowledge.

> > +static int sg2042_mcu_read_temp(struct device *dev,
> > +				u32 attr, int channel,
> > +				long *val)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	int tmp;
> > +	u8 reg;
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> > +		break;
> > +	case hwmon_temp_crit:
> > +		reg = REG_CRITICAL_TEMP;
> > +		break;
> > +	case hwmon_temp_crit_hyst:
> > +		reg = REG_REPOWER_TEMP;
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> > +	if (tmp < 0)
> > +		return tmp;
> > +	*val = tmp * 1000;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sg2042_mcu_read(struct device *dev,
> > +			   enum hwmon_sensor_types type,
> > +			   u32 attr, int channel, long *val)
> > +{
> > +	return sg2042_mcu_read_temp(dev, attr, channel, val);
> > +}
> Can we merge sg2042_mcu_read and sg2042_mcu_read_temp？

Yes, it can be merged. but I think using this nested function 
is more clear. And gcc can auto inline this function so we
got no performance penalty.

> > +
> > +static int sg2042_mcu_write(struct device *dev,
> > +			    enum hwmon_sensor_types type,
> > +			    u32 attr, int channel, long val)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	int temp = val / 1000;
> > +	int hyst_temp, crit_temp;
> > +	int ret;
> > +	u8 reg;
> > +
> > +	if (temp > MCU_POWER_MAX)
> > +		temp = MCU_POWER_MAX;
> > +
> > +	mutex_lock(&mcu->mutex);
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_crit:
> > +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> > +						     REG_REPOWER_TEMP);
> > +		if (hyst_temp < 0) {
> > +			ret = -ENODEV;
> > +			goto failed;
> > +		}
> > +
> > +		crit_temp = temp;
> > +		reg = REG_CRITICAL_TEMP;
> > +		break;
> > +	case hwmon_temp_crit_hyst:
> > +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> > +						     REG_CRITICAL_TEMP);
> > +		if (crit_temp < 0) {
> > +			ret = -ENODEV;
> > +			goto failed;
> > +		}
> > +
> > +		hyst_temp = temp;
> > +		reg = REG_REPOWER_TEMP;
> > +		break;
> > +	default:
> > +		mutex_unlock(&mcu->mutex);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> It is recommended to add some comments to explain why we need to ensure that
> crit_temp is greater than or equal to hyst_temp. This is entirely because
> the current MCU does not limit the input, which may cause user to set
> incorrect crit_temp and hyst_temp.

Yeah, this is good idea.

> > +	if (crit_temp < hyst_temp) {
> > +		ret = -EINVAL;
> > +		goto failed;
> > +	}
> > +
> > +	ret = i2c_smbus_write_byte_data(mcu->client, reg, temp);
> > +
> > +failed:
> > +	mutex_unlock(&mcu->mutex);
> > +	return ret;
> > +}
> > +
> [......]

Re: [PATCH v8 2/4] drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

[Guenter Roeck]

On Wed, Jul 31, 2024 at 03:17:57PM +0800, Inochi Amaoto wrote:
> On Wed, Jul 31, 2024 at 02:13:20PM GMT, Chen Wang wrote:
> > 
> > On 2024/7/30 15:50, Inochi Amaoto wrote:
> > [......]
> > > +#define REG_CRITICAL_ACTIONS			0x65
> > The name "REG_CRITICAL_ACTIONS" is ambiguous. I have confirmed with sophgo
> > engineers that the complete process is: when the measured temperature
> > exceeds the temperature set by REG_CRITICAL_TEMP, the processor is powered
> > off and shut down, and then after the temperature returns to the temperature
> > set by REG_REPOWER_TEMP, it is decided whether to power on again or remain
> > in the shutdown state based on the action set by REG_CRITICAL_ACTIONS,
> > whether it is reboot or poweroff.
> > 
> > So based on the above description, I think it would be better to
> > call "REG_CRITICAL_ACTIONS" as "REG_REPOWER_ACTIONS". "REG_CRITICAL_ACTIONS"
> > gives people the first impression that it is used to set actions related to
> > REG_CRITICAL_TEMP.
> > 
> > It is also recommended to add the above description of temperature control
> > and action settings in the code. Currently, sophgo does not have a clear
> > document description for this part, and adding it will help us understand
> > its functions.
> > 
> > Adding sophgo engineers Chunzhi and Haijiao, FYI.
> > 
> > > +#define REG_CRITICAL_TEMP			0x66
> > > +#define REG_REPOWER_TEMP			0x67
> > > +
> > > +#define CRITICAL_ACTION_REBOOT			1
> > > +#define CRITICAL_ACTION_POWEROFF		2
> > 
> > As I said upon, actions are not related to critical, but is for restoring
> > from critical, suggest to give a better name.
> > 
> > [......]
> > 
> > > +static ssize_t critical_action_show(struct device *dev,
> > [......]
> > > +static ssize_t critical_action_store(struct device *dev,
> > 
> > [......]
> > 
> > The same reason as upon, "critical_action_xxx" is misleading.
> > 
> > [......]
> > 
> 
> Thanks for explanation, I just get the name from the driver of SG2042.
> This is out of my knowledge.
> 
> > > +static int sg2042_mcu_read_temp(struct device *dev,
> > > +				u32 attr, int channel,
> > > +				long *val)
> > > +{
> > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > +	int tmp;
> > > +	u8 reg;
> > > +
> > > +	switch (attr) {
> > > +	case hwmon_temp_input:
> > > +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> > > +		break;
> > > +	case hwmon_temp_crit:
> > > +		reg = REG_CRITICAL_TEMP;
> > > +		break;
> > > +	case hwmon_temp_crit_hyst:
> > > +		reg = REG_REPOWER_TEMP;
> > > +		break;
> > > +	default:
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +
> > > +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> > > +	if (tmp < 0)
> > > +		return tmp;
> > > +	*val = tmp * 1000;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int sg2042_mcu_read(struct device *dev,
> > > +			   enum hwmon_sensor_types type,
> > > +			   u32 attr, int channel, long *val)
> > > +{
> > > +	return sg2042_mcu_read_temp(dev, attr, channel, val);
> > > +}
> > Can we merge sg2042_mcu_read and sg2042_mcu_read_temp？
> 
> Yes, it can be merged. but I think using this nested function 
> is more clear. And gcc can auto inline this function so we
> got no performance penalty.
> 

FWIW, I think that is pointless. Te only difference is unused
parameters.

> > > +
> > > +static int sg2042_mcu_write(struct device *dev,
> > > +			    enum hwmon_sensor_types type,
> > > +			    u32 attr, int channel, long val)
> > > +{
> > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > +	int temp = val / 1000;
> > > +	int hyst_temp, crit_temp;
> > > +	int ret;
> > > +	u8 reg;
> > > +
> > > +	if (temp > MCU_POWER_MAX)
> > > +		temp = MCU_POWER_MAX;

No lower limit ? -1000000 is ok ?

> > > +
> > > +	mutex_lock(&mcu->mutex);
> > > +
> > > +	switch (attr) {
> > > +	case hwmon_temp_crit:
> > > +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> > > +						     REG_REPOWER_TEMP);
> > > +		if (hyst_temp < 0) {
> > > +			ret = -ENODEV;
> > > +			goto failed;

Do not overwrite error codes.

> > > +		}
> > > +
> > > +		crit_temp = temp;
> > > +		reg = REG_CRITICAL_TEMP;
> > > +		break;
> > > +	case hwmon_temp_crit_hyst:
> > > +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> > > +						     REG_CRITICAL_TEMP);
> > > +		if (crit_temp < 0) {
> > > +			ret = -ENODEV;
> > > +			goto failed;

Do not overwrite error codes.

> > > +		}
> > > +
> > > +		hyst_temp = temp;
> > > +		reg = REG_REPOWER_TEMP;
> > > +		break;
> > > +	default:
> > > +		mutex_unlock(&mcu->mutex);
> > > +		return -EOPNOTSUPP;

This is inconsistent.

> > > +	}
> > > +
> > It is recommended to add some comments to explain why we need to ensure that
> > crit_temp is greater than or equal to hyst_temp. This is entirely because
> > the current MCU does not limit the input, which may cause user to set
> > incorrect crit_temp and hyst_temp.
> 
> Yeah, this is good idea.
> 
> > > +	if (crit_temp < hyst_temp) {
> > > +		ret = -EINVAL;
> > > +		goto failed;
> > > +	}
> > > +
> > > +	ret = i2c_smbus_write_byte_data(mcu->client, reg, temp);
> > > +
> > > +failed:
> > > +	mutex_unlock(&mcu->mutex);
> > > +	return ret;
> > > +}
> > > +
> > [......]

Re: [PATCH v8 2/4] drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

[Inochi Amaoto]

On Wed, Jul 31, 2024 at 08:02:36AM GMT, Guenter Roeck wrote:
> On Wed, Jul 31, 2024 at 03:17:57PM +0800, Inochi Amaoto wrote:
> > On Wed, Jul 31, 2024 at 02:13:20PM GMT, Chen Wang wrote:
> > > 
> > > On 2024/7/30 15:50, Inochi Amaoto wrote:
> > > [......]
> > > > +#define REG_CRITICAL_ACTIONS			0x65
> > > The name "REG_CRITICAL_ACTIONS" is ambiguous. I have confirmed with sophgo
> > > engineers that the complete process is: when the measured temperature
> > > exceeds the temperature set by REG_CRITICAL_TEMP, the processor is powered
> > > off and shut down, and then after the temperature returns to the temperature
> > > set by REG_REPOWER_TEMP, it is decided whether to power on again or remain
> > > in the shutdown state based on the action set by REG_CRITICAL_ACTIONS,
> > > whether it is reboot or poweroff.
> > > 
> > > So based on the above description, I think it would be better to
> > > call "REG_CRITICAL_ACTIONS" as "REG_REPOWER_ACTIONS". "REG_CRITICAL_ACTIONS"
> > > gives people the first impression that it is used to set actions related to
> > > REG_CRITICAL_TEMP.
> > > 
> > > It is also recommended to add the above description of temperature control
> > > and action settings in the code. Currently, sophgo does not have a clear
> > > document description for this part, and adding it will help us understand
> > > its functions.
> > > 
> > > Adding sophgo engineers Chunzhi and Haijiao, FYI.
> > > 
> > > > +#define REG_CRITICAL_TEMP			0x66
> > > > +#define REG_REPOWER_TEMP			0x67
> > > > +
> > > > +#define CRITICAL_ACTION_REBOOT			1
> > > > +#define CRITICAL_ACTION_POWEROFF		2
> > > 
> > > As I said upon, actions are not related to critical, but is for restoring
> > > from critical, suggest to give a better name.
> > > 
> > > [......]
> > > 
> > > > +static ssize_t critical_action_show(struct device *dev,
> > > [......]
> > > > +static ssize_t critical_action_store(struct device *dev,
> > > 
> > > [......]
> > > 
> > > The same reason as upon, "critical_action_xxx" is misleading.
> > > 
> > > [......]
> > > 
> > 
> > Thanks for explanation, I just get the name from the driver of SG2042.
> > This is out of my knowledge.
> > 
> > > > +static int sg2042_mcu_read_temp(struct device *dev,
> > > > +				u32 attr, int channel,
> > > > +				long *val)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	int tmp;
> > > > +	u8 reg;
> > > > +
> > > > +	switch (attr) {
> > > > +	case hwmon_temp_input:
> > > > +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> > > > +		break;
> > > > +	case hwmon_temp_crit:
> > > > +		reg = REG_CRITICAL_TEMP;
> > > > +		break;
> > > > +	case hwmon_temp_crit_hyst:
> > > > +		reg = REG_REPOWER_TEMP;
> > > > +		break;
> > > > +	default:
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > > +
> > > > +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> > > > +	if (tmp < 0)
> > > > +		return tmp;
> > > > +	*val = tmp * 1000;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int sg2042_mcu_read(struct device *dev,
> > > > +			   enum hwmon_sensor_types type,
> > > > +			   u32 attr, int channel, long *val)
> > > > +{
> > > > +	return sg2042_mcu_read_temp(dev, attr, channel, val);
> > > > +}
> > > Can we merge sg2042_mcu_read and sg2042_mcu_read_temp？
> > 
> > Yes, it can be merged. but I think using this nested function 
> > is more clear. And gcc can auto inline this function so we
> > got no performance penalty.
> > 
> 
> FWIW, I think that is pointless. Te only difference is unused
> parameters.
> 

OK, I will merge these function.

> > > > +
> > > > +static int sg2042_mcu_write(struct device *dev,
> > > > +			    enum hwmon_sensor_types type,
> > > > +			    u32 attr, int channel, long val)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	int temp = val / 1000;
> > > > +	int hyst_temp, crit_temp;
> > > > +	int ret;
> > > > +	u8 reg;
> > > > +
> > > > +	if (temp > MCU_POWER_MAX)
> > > > +		temp = MCU_POWER_MAX;
> 
> No lower limit ? -1000000 is ok ?
> 

My fault, it should always > 0.

> > > > +
> > > > +	mutex_lock(&mcu->mutex);
> > > > +
> > > > +	switch (attr) {
> > > > +	case hwmon_temp_crit:
> > > > +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> > > > +						     REG_REPOWER_TEMP);
> > > > +		if (hyst_temp < 0) {
> > > > +			ret = -ENODEV;
> > > > +			goto failed;
> 
> Do not overwrite error codes.
> 
> > > > +		}
> > > > +
> > > > +		crit_temp = temp;
> > > > +		reg = REG_CRITICAL_TEMP;
> > > > +		break;
> > > > +	case hwmon_temp_crit_hyst:
> > > > +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> > > > +						     REG_CRITICAL_TEMP);
> > > > +		if (crit_temp < 0) {
> > > > +			ret = -ENODEV;
> > > > +			goto failed;
> 
> Do not overwrite error codes.
> 
> > > > +		}
> > > > +
> > > > +		hyst_temp = temp;
> > > > +		reg = REG_REPOWER_TEMP;
> > > > +		break;
> > > > +	default:
> > > > +		mutex_unlock(&mcu->mutex);
> > > > +		return -EOPNOTSUPP;
> 
> This is inconsistent.
> 

Thanks, I will handle the return properly.

> > > > +	}
> > > > +
> > > It is recommended to add some comments to explain why we need to ensure that
> > > crit_temp is greater than or equal to hyst_temp. This is entirely because
> > > the current MCU does not limit the input, which may cause user to set
> > > incorrect crit_temp and hyst_temp.
> > 
> > Yeah, this is good idea.
> > 
> > > > +	if (crit_temp < hyst_temp) {
> > > > +		ret = -EINVAL;
> > > > +		goto failed;
> > > > +	}
> > > > +
> > > > +	ret = i2c_smbus_write_byte_data(mcu->client, reg, temp);
> > > > +
> > > > +failed:
> > > > +	mutex_unlock(&mcu->mutex);
> > > > +	return ret;
> > > > +}
> > > > +
> > > [......]