[PATCH v6 0/2] riscv: sophgo: Add SG2042 external hardware monitor support

[PATCH v6 0/2] riscv: sophgo: Add SG2042 external hardware monitor support

[Inochi Amaoto]

Add support for the onboard hardware monitor for SG2042.

Related SBI patch:
https://lists.infradead.org/pipermail/opensbi/2024-April/006849.html

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 (2):
  dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor
    support
  drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

 .../hwmon/sophgo,sg2042-hwmon-mcu.yaml        |  43 ++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/sgmcu.rst                 |  44 ++
 drivers/hwmon/Kconfig                         |  11 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/sgmcu.c                         | 585 ++++++++++++++++++
 6 files changed, 685 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/sophgo,sg2042-hwmon-mcu.yaml
 create mode 100644 Documentation/hwmon/sgmcu.rst
 create mode 100644 drivers/hwmon/sgmcu.c

--
2.45.2

[PATCH v6 1/2] 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 v6 2/2] 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/sgmcu.rst |  44 +++
 drivers/hwmon/Kconfig         |  11 +
 drivers/hwmon/Makefile        |   1 +
 drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
 5 files changed, 642 insertions(+)
 create mode 100644 Documentation/hwmon/sgmcu.rst
 create mode 100644 drivers/hwmon/sgmcu.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 03d313af469a..189626b3a055 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
    sch5636
    scpi-hwmon
    sfctemp
+   sgmcu
    sht15
    sht21
    sht3x
diff --git a/Documentation/hwmon/sgmcu.rst b/Documentation/hwmon/sgmcu.rst
new file mode 100644
index 000000000000..5669dcfb2a33
--- /dev/null
+++ b/Documentation/hwmon/sgmcu.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver sgmcu
+=====================
+
+Supported chips:
+
+  * Onboard MCU for sg2042
+
+    Addresses scanned: -
+
+    Prefix: 'sgmcu'
+
+Authors:
+
+  - Inochi Amaoto <inochiama@outlook.com>
+
+Description
+-----------
+
+This driver supprts hardware monitoring for onboard MCU with
+PMBus 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.
+
+Platform data support
+---------------------
+
+The driver supports standard PMBus driver platform data.
+
+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 e14ae18a973b..1100dd11f7f5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2079,6 +2079,17 @@ config SENSORS_SFCTEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called sfctemp.

+config SENSORS_SGMCU
+	tristate "Sophgo onboard MCU support"
+	depends on I2C
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  Support for onboard MCU of Sophgo 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 sgmcu.
+
 config SENSORS_SURFACE_FAN
 	tristate "Surface Fan Driver"
 	depends on SURFACE_AGGREGATOR
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e3f25475d1f0..e9b78ff8338e 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -195,6 +195,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_SGMCU)	+= sgmcu.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/sgmcu.c b/drivers/hwmon/sgmcu.c
new file mode 100644
index 000000000000..d941d6fe741f
--- /dev/null
+++ b/drivers/hwmon/sgmcu.c
@@ -0,0 +1,585 @@
+// 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/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/hwmon.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			0x1
+#define CRITICAL_ACTION_POWEROFF		0x2
+
+#define DEFAULT_REPOWER_TEMP			60
+#define MAX_REPOWER_TEMP			100
+
+#define sg2042_mcu_read_byte(client, reg)			\
+	i2c_smbus_read_byte_data(client, reg)
+#define sg2042_mcu_write_byte(client, reg, value)		\
+	i2c_smbus_write_byte_data(client, reg, value)
+#define sg2042_mcu_read_block(client, reg, array)		\
+	i2c_smbus_read_i2c_block_data(client, reg, sizeof(array), array)
+
+#define DEFINE_MCU_ATTR_READ_FUNC(_name, _type, _format)		\
+	static ssize_t _name##_show(struct device *dev,			\
+				    struct device_attribute *attr,	\
+				    char *buf)				\
+	{								\
+		struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);	\
+		_type ret;						\
+		ret = sg2042_mcu_get_##_name(mcu->client);		\
+		if (ret < 0)						\
+			return ret;					\
+		return sprintf(buf, _format "\n", ret);			\
+	}
+
+#define DEFINE_MCU_DEBUG_ATTR_READ_FUNC(_name, _type, _format)		\
+	static int _name##_show(struct seq_file *seqf,			\
+				    void *unused)			\
+	{								\
+		struct sg2042_mcu_data *mcu = seqf->private;		\
+		_type ret;						\
+		ret = sg2042_mcu_get_##_name(mcu->client);		\
+		if (ret < 0)						\
+			return ret;					\
+		seq_printf(seqf, _format "\n", ret);			\
+		return 0;						\
+	}
+
+#define _CREATE_DEBUG_ENTRY(name, perm, d, data)			\
+	debugfs_create_file(#name, perm, d, data, &name##_fops)
+
+struct sg2042_mcu_board_data {
+	u8		id;
+	const char	*name;
+};
+
+struct sg2042_mcu_data {
+	struct i2c_client			*client;
+	const struct sg2042_mcu_board_data	*board_info;
+	struct dentry				*debugfs;
+};
+
+static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
+	{
+		.id = 0x80,
+		.name = "SG2042 evb x8",
+	},
+	{
+		.id = 0x81,
+		.name = "SG2042R evb",
+	},
+	{
+		.id = 0x83,
+		.name = "SG2042 evb x4",
+	},
+	{
+		.id = 0x90,
+		.name = "Milk-V Pioneer",
+	},
+};
+
+static const char *sg2042_mcu_reset_reason[8] = {
+	"Power supply overheat",
+	"Power supply failure",
+	"12V power supply failure",
+	"Reset commant",
+	"Unknown",
+	"Unknown",
+	"Unknown",
+	"SoC overheat",
+};
+
+static struct dentry *sgmcu_debugfs;
+
+static int sg2042_mcu_get_board_type(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_BOARD_TYPE);
+}
+
+static int sg2042_mcu_get_firmware_version(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_MCU_FIRMWARE_VERSION);
+}
+
+static int sg2042_mcu_get_pcb_version(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_PCB_VERSION);
+}
+
+static int sg2042_mcu_get_soc_temp(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_SOC_TEMP);
+}
+
+static int sg2042_mcu_get_board_temp(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_BOARD_TEMP);
+}
+
+static int sg2042_mcu_get_reset_count(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_RST_COUNT);
+}
+
+static s32 sg2042_mcu_get_uptime(struct i2c_client *client)
+{
+	int ret;
+	u8 time_val[2];
+
+	ret = sg2042_mcu_read_block(client, REG_UPTIME, time_val);
+	if (ret < 0)
+		return ret;
+
+	return (s32)(time_val[0]) + ((s32)(time_val[1]) << 8);
+}
+
+static int sg2042_mcu_get_reset_reason(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_RESET_REASON);
+}
+
+static int sg2042_mcu_get_mcu_type(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_MCU_TYPE);
+}
+
+static int sg2042_mcu_get_soc_crit_action(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_CRITICAL_ACTIONS);
+}
+
+static int sg2042_mcu_get_soc_crit_temp(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_CRITICAL_TEMP);
+}
+
+static int sg2042_mcu_get_soc_hyst_temp(struct i2c_client *client)
+{
+	return sg2042_mcu_read_byte(client, REG_REPOWER_TEMP);
+}
+
+static int sg2042_mcu_set_soc_crit_action(struct i2c_client *client,
+					  u8 value)
+{
+	return sg2042_mcu_write_byte(client, REG_CRITICAL_ACTIONS, value);
+}
+
+static int sg2042_mcu_set_soc_crit_temp(struct i2c_client *client,
+					u8 value)
+{
+	return sg2042_mcu_write_byte(client, REG_CRITICAL_TEMP, value);
+}
+
+static int sg2042_mcu_set_soc_hyst_temp(struct i2c_client *client,
+					u8 value)
+{
+	return sg2042_mcu_write_byte(client, REG_REPOWER_TEMP, value);
+}
+
+DEFINE_MCU_ATTR_READ_FUNC(reset_count, int, "%d");
+DEFINE_MCU_ATTR_READ_FUNC(uptime, s32, "%d");
+
+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, val, i;
+
+	val = sg2042_mcu_get_reset_reason(mcu->client);
+	if (val < 0)
+		return val;
+
+	ret = sprintf(buf, "Reason: 0x%02x\n", val);
+
+	for (i = 0; i < ARRAY_SIZE(sg2042_mcu_reset_reason); i++) {
+		if (val & BIT(i))
+			ret += sprintf(buf + ret, "bit %d: %s\n", i,
+						  sg2042_mcu_reset_reason[i]);
+	}
+
+	return 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 = sg2042_mcu_get_soc_crit_action(mcu->client);
+	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);
+	int value;
+
+	if (sysfs_streq("reboot", buf))
+		value = CRITICAL_ACTION_REBOOT;
+	else if (sysfs_streq("poweroff", buf))
+		value = CRITICAL_ACTION_POWEROFF;
+	else
+		return -EINVAL;
+
+	return sg2042_mcu_set_soc_crit_action(mcu->client, value);
+}
+
+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_READ_FUNC(firmware_version, int, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR_READ_FUNC(pcb_version, int, "0x%02x");
+
+static int board_type_show(struct seq_file *seqf, void *unused)
+{
+	struct sg2042_mcu_data *mcu = seqf->private;
+
+	seq_printf(seqf, "%s\n", mcu->board_info->name ?: "Unknown");
+
+	return 0;
+}
+
+static int mcu_type_show(struct seq_file *seqf, void *unused)
+{
+	struct sg2042_mcu_data *mcu = seqf->private;
+	int ret;
+
+	ret = sg2042_mcu_get_mcu_type(mcu->client);
+	if (ret < 0)
+		return ret;
+
+	seq_puts(seqf, ret ? "GD32\n" : "STM32\n");
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(firmware_version);
+DEFINE_SHOW_ATTRIBUTE(pcb_version);
+DEFINE_SHOW_ATTRIBUTE(mcu_type);
+DEFINE_SHOW_ATTRIBUTE(board_type);
+
+// TODO: to debugfs
+
+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 hwmon_channel_info * const sg2042_mcu_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
+	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);
+	long tmp;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		switch (channel) {
+		case 0:
+			tmp = sg2042_mcu_get_soc_temp(mcu->client);
+			if (tmp < 0)
+				return tmp;
+			*val = tmp * 1000;
+			break;
+		case 1:
+			tmp = sg2042_mcu_get_board_temp(mcu->client);
+			if (tmp < 0)
+				return tmp;
+			*val = tmp * 1000;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	case hwmon_temp_crit:
+		if (channel)
+			return -EOPNOTSUPP;
+
+		tmp = sg2042_mcu_get_soc_crit_temp(mcu->client);
+		if (tmp < 0)
+			return tmp;
+		*val = tmp * 1000;
+		break;
+	case hwmon_temp_crit_hyst:
+		if (channel)
+			return -EOPNOTSUPP;
+
+		tmp = sg2042_mcu_get_soc_hyst_temp(mcu->client);
+		if (tmp < 0)
+			return tmp;
+		*val = tmp * 1000;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int sg2042_mcu_read(struct device *dev,
+			   enum hwmon_sensor_types type,
+			   u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_chip:
+		if (attr != hwmon_chip_update_interval)
+			return -EOPNOTSUPP;
+		*val = 1000;
+		break;
+	case hwmon_temp:
+		return sg2042_mcu_read_temp(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+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);
+	u8 down_temp, repower_temp;
+	int ret;
+
+	if (type != hwmon_temp || attr != hwmon_temp_crit || !channel)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_temp_crit:
+		ret = sg2042_mcu_get_soc_hyst_temp(mcu->client);
+		if (ret < 0)
+			repower_temp = DEFAULT_REPOWER_TEMP;
+		else
+			repower_temp = ret;
+
+		down_temp = val / 1000;
+		if (down_temp < repower_temp)
+			return -EINVAL;
+
+		return sg2042_mcu_set_soc_crit_temp(mcu->client,
+						    (u8)(val / 1000));
+	case hwmon_temp_crit_hyst:
+		ret = sg2042_mcu_get_soc_crit_temp(mcu->client);
+		if (ret < 0)
+			return -ENODEV;
+
+		down_temp = ret;
+		repower_temp = val / 1000;
+		if (down_temp < repower_temp)
+			return -EINVAL;
+
+		return sg2042_mcu_set_soc_hyst_temp(mcu->client,
+						    (u8)(val / 1000));
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t sg2042_mcu_is_visible(const void *_data,
+				     enum hwmon_sensor_types type,
+				     u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_chip:
+		if (attr == hwmon_chip_update_interval)
+			return 0444;
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			if (channel < 2)
+				return 0444;
+			break;
+		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) {
+		_CREATE_DEBUG_ENTRY(firmware_version, 0444, mcu->debugfs, mcu);
+		_CREATE_DEBUG_ENTRY(pcb_version, 0444, mcu->debugfs, mcu);
+		_CREATE_DEBUG_ENTRY(mcu_type, 0444, mcu->debugfs, mcu);
+		_CREATE_DEBUG_ENTRY(board_type, 0444, mcu->debugfs, mcu);
+	}
+}
+
+static int sg2042_mcu_check_board(u8 id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(sg2042_boards_data); i++) {
+		if (sg2042_boards_data[i].id == id)
+			return i;
+	}
+
+	return -ENODEV;
+}
+
+static int sg2042_mcu_i2c_probe(struct i2c_client *client)
+{
+	int ret;
+	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;
+
+	ret = sg2042_mcu_get_board_type(client);
+	if (ret < 0)
+		return ret;
+
+	ret = sg2042_mcu_check_board(ret);
+	if (ret < 0)
+		return ret;
+
+	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
+	if (!mcu)
+		return -ENOMEM;
+
+	mcu->client = client;
+	mcu->board_info = &sg2042_boards_data[ret];
+
+	ret = sysfs_create_group(&dev->kobj, &sg2042_mcu_attr_group);
+	if (ret < 0)
+		return ret;
+
+	i2c_set_clientdata(client, mcu);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 mcu,
+							 &sg2042_mcu_chip_info,
+							 NULL);
+
+	sg2042_mcu_debugfs_init(mcu, dev);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static void sg2042_mcu_i2c_remove(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+
+	sysfs_remove_group(&dev->kobj, &sg2042_mcu_attr_group);
+}
+
+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,
+	},
+	.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("sgmcu", 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

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

[Guenter Roeck]

On 7/2/24 19:30, Inochi Amaoto wrote:
> 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/sgmcu.rst |  44 +++
>   drivers/hwmon/Kconfig         |  11 +
>   drivers/hwmon/Makefile        |   1 +
>   drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
>   5 files changed, 642 insertions(+)
>   create mode 100644 Documentation/hwmon/sgmcu.rst
>   create mode 100644 drivers/hwmon/sgmcu.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 03d313af469a..189626b3a055 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
>      sch5636
>      scpi-hwmon
>      sfctemp
> +   sgmcu
>      sht15
>      sht21
>      sht3x
> diff --git a/Documentation/hwmon/sgmcu.rst b/Documentation/hwmon/sgmcu.rst
> new file mode 100644
> index 000000000000..5669dcfb2a33
> --- /dev/null
> +++ b/Documentation/hwmon/sgmcu.rst
> @@ -0,0 +1,44 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver sgmcu
> +=====================
> +
> +Supported chips:
> +
> +  * Onboard MCU for sg2042
> +
> +    Addresses scanned: -
> +
> +    Prefix: 'sgmcu'
> +
> +Authors:
> +
> +  - Inochi Amaoto <inochiama@outlook.com>
> +
> +Description
> +-----------
> +
> +This driver supprts hardware monitoring for onboard MCU with
> +PMBus 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.
> +
> +Platform data support
> +---------------------
> +
> +The driver supports standard PMBus driver platform data.
> +

It doesn't. Where does PMBus come into play ? Is this a cut-and-paste problem ?

> +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 e14ae18a973b..1100dd11f7f5 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2079,6 +2079,17 @@ config SENSORS_SFCTEMP
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called sfctemp.
> 
> +config SENSORS_SGMCU
> +	tristate "Sophgo onboard MCU support"
> +	depends on I2C
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	help
> +	  Support for onboard MCU of Sophgo 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 sgmcu.
> +
>   config SENSORS_SURFACE_FAN
>   	tristate "Surface Fan Driver"
>   	depends on SURFACE_AGGREGATOR
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e3f25475d1f0..e9b78ff8338e 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -195,6 +195,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_SGMCU)	+= sgmcu.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/sgmcu.c b/drivers/hwmon/sgmcu.c
> new file mode 100644
> index 000000000000..d941d6fe741f
> --- /dev/null
> +++ b/drivers/hwmon/sgmcu.c
> @@ -0,0 +1,585 @@
> +// 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +
Alphabetic include file order, please.

> +/* 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			0x1
> +#define CRITICAL_ACTION_POWEROFF		0x2
> +

Please use BIT() for bit masks.

> +#define DEFAULT_REPOWER_TEMP			60
> +#define MAX_REPOWER_TEMP			100
> +
> +#define sg2042_mcu_read_byte(client, reg)			\
> +	i2c_smbus_read_byte_data(client, reg)
> +#define sg2042_mcu_write_byte(client, reg, value)		\
> +	i2c_smbus_write_byte_data(client, reg, value)
> +#define sg2042_mcu_read_block(client, reg, array)		\
> +	i2c_smbus_read_i2c_block_data(client, reg, sizeof(array), array)
> +

Pointless defines. See below.

> +#define DEFINE_MCU_ATTR_READ_FUNC(_name, _type, _format)		\
> +	static ssize_t _name##_show(struct device *dev,			\
> +				    struct device_attribute *attr,	\
> +				    char *buf)				\
> +	{								\
> +		struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);	\
> +		_type ret;						\
> +		ret = sg2042_mcu_get_##_name(mcu->client);		\
> +		if (ret < 0)						\
> +			return ret;					\
> +		return sprintf(buf, _format "\n", ret);			\
> +	}
> +
> +#define DEFINE_MCU_DEBUG_ATTR_READ_FUNC(_name, _type, _format)		\
> +	static int _name##_show(struct seq_file *seqf,			\
> +				    void *unused)			\
> +	{								\
> +		struct sg2042_mcu_data *mcu = seqf->private;		\
> +		_type ret;						\
> +		ret = sg2042_mcu_get_##_name(mcu->client);		\
> +		if (ret < 0)						\
> +			return ret;					\
> +		seq_printf(seqf, _format "\n", ret);			\
> +		return 0;						\
> +	}
> +
> +#define _CREATE_DEBUG_ENTRY(name, perm, d, data)			\
> +	debugfs_create_file(#name, perm, d, data, &name##_fops)
> +

Pointless define. Please do not replace API function names with your own;
that just makes it harder to find callers.

> +struct sg2042_mcu_board_data {
> +	u8		id;
> +	const char	*name;
> +};
> +
> +struct sg2042_mcu_data {
> +	struct i2c_client			*client;
> +	const struct sg2042_mcu_board_data	*board_info;
> +	struct dentry				*debugfs;
> +};
> +
> +static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
> +	{
> +		.id = 0x80,
> +		.name = "SG2042 evb x8",
> +	},
> +	{
> +		.id = 0x81,
> +		.name = "SG2042R evb",
> +	},
> +	{
> +		.id = 0x83,
> +		.name = "SG2042 evb x4",
> +	},
> +	{
> +		.id = 0x90,
> +		.name = "Milk-V Pioneer",
> +	},
> +};
> +
> +static const char *sg2042_mcu_reset_reason[8] = {
> +	"Power supply overheat",
> +	"Power supply failure",
> +	"12V power supply failure",
> +	"Reset commant",

command

> +	"Unknown",
> +	"Unknown",
> +	"Unknown",
> +	"SoC overheat",
> +};
> +
> +static struct dentry *sgmcu_debugfs;
> +
> +static int sg2042_mcu_get_board_type(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_BOARD_TYPE);
> +}
> +
> +static int sg2042_mcu_get_firmware_version(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_MCU_FIRMWARE_VERSION);
> +}
> +
> +static int sg2042_mcu_get_pcb_version(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_PCB_VERSION);
> +}
> +
> +static int sg2042_mcu_get_soc_temp(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_SOC_TEMP);
> +}
> +
> +static int sg2042_mcu_get_board_temp(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_BOARD_TEMP);
> +}
> +
> +static int sg2042_mcu_get_reset_count(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_RST_COUNT);
> +}
> +

Those one-line functions are just as pointless.
	i2c_smbus_read_byte_data(client, REG_RST_COUNT);
does exactly the same without extra function and define.

> +static s32 sg2042_mcu_get_uptime(struct i2c_client *client)
> +{
> +	int ret;
> +	u8 time_val[2];
> +
> +	ret = sg2042_mcu_read_block(client, REG_UPTIME, time_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return (s32)(time_val[0]) + ((s32)(time_val[1]) << 8);
> +}
> +

This is the only one of those functions which adds at least some value.

> +static int sg2042_mcu_get_reset_reason(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_RESET_REASON);
> +}
> +
> +static int sg2042_mcu_get_mcu_type(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_MCU_TYPE);
> +}
> +
> +static int sg2042_mcu_get_soc_crit_action(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_CRITICAL_ACTIONS);
> +}
> +
> +static int sg2042_mcu_get_soc_crit_temp(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_CRITICAL_TEMP);
> +}
> +
> +static int sg2042_mcu_get_soc_hyst_temp(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_REPOWER_TEMP);
> +}
> +
> +static int sg2042_mcu_set_soc_crit_action(struct i2c_client *client,
> +					  u8 value)
> +{
> +	return sg2042_mcu_write_byte(client, REG_CRITICAL_ACTIONS, value);
> +}
> +
> +static int sg2042_mcu_set_soc_crit_temp(struct i2c_client *client,
> +					u8 value)
> +{
> +	return sg2042_mcu_write_byte(client, REG_CRITICAL_TEMP, value);
> +}
> +
> +static int sg2042_mcu_set_soc_hyst_temp(struct i2c_client *client,
> +					u8 value)
> +{
> +	return sg2042_mcu_write_byte(client, REG_REPOWER_TEMP, value);
> +}
> +
> +DEFINE_MCU_ATTR_READ_FUNC(reset_count, int, "%d");
> +DEFINE_MCU_ATTR_READ_FUNC(uptime, s32, "%d");
> +
> +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, val, i;
> +
> +	val = sg2042_mcu_get_reset_reason(mcu->client);
> +	if (val < 0)
> +		return val;
> +
> +	ret = sprintf(buf, "Reason: 0x%02x\n", val);
> +
> +	for (i = 0; i < ARRAY_SIZE(sg2042_mcu_reset_reason); i++) {
> +		if (val & BIT(i))
> +			ret += sprintf(buf + ret, "bit %d: %s\n", i,
> +						  sg2042_mcu_reset_reason[i]);
> +	}
> +
> +	return ret;
> +}

This violates sysfs standards (one word only). If you want that level of detail,
add it to debugfs.

> +
> +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 = sg2042_mcu_get_soc_crit_action(mcu->client);
> +	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);
> +	int value;
> +
> +	if (sysfs_streq("reboot", buf))
> +		value = CRITICAL_ACTION_REBOOT;
> +	else if (sysfs_streq("poweroff", buf))
> +		value = CRITICAL_ACTION_POWEROFF;
> +	else
> +		return -EINVAL;
> +
> +	return sg2042_mcu_set_soc_crit_action(mcu->client, value);
> +}
> +
> +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_READ_FUNC(firmware_version, int, "0x%02x");
> +DEFINE_MCU_DEBUG_ATTR_READ_FUNC(pcb_version, int, "0x%02x");
> +
> +static int board_type_show(struct seq_file *seqf, void *unused)
> +{
> +	struct sg2042_mcu_data *mcu = seqf->private;
> +
> +	seq_printf(seqf, "%s\n", mcu->board_info->name ?: "Unknown");
> +
> +	return 0;
> +}
> +
> +static int mcu_type_show(struct seq_file *seqf, void *unused)
> +{
> +	struct sg2042_mcu_data *mcu = seqf->private;
> +	int ret;
> +
> +	ret = sg2042_mcu_get_mcu_type(mcu->client);
> +	if (ret < 0)
> +		return ret;
> +
> +	seq_puts(seqf, ret ? "GD32\n" : "STM32\n");
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(firmware_version);
> +DEFINE_SHOW_ATTRIBUTE(pcb_version);
> +DEFINE_SHOW_ATTRIBUTE(mcu_type);
> +DEFINE_SHOW_ATTRIBUTE(board_type);
> +
> +// TODO: to debugfs
> +

If there is a TODO left, move the driver to drivers/staging/
and keep it there until it is complete.

> +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 hwmon_channel_info * const sg2042_mcu_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
> +	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);
> +	long tmp;

Why long ?

> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		switch (channel) {
> +		case 0:
> +			tmp = sg2042_mcu_get_soc_temp(mcu->client);
> +			if (tmp < 0)
> +				return tmp;
> +			*val = tmp * 1000;
> +			break;
> +		case 1:
> +			tmp = sg2042_mcu_get_board_temp(mcu->client);
> +			if (tmp < 0)
> +				return tmp;
> +			*val = tmp * 1000;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}

This would be much simpler written with something like
		tmp = i2c_smbus_read_byte_data(client, channel ? REG_SOC_TEMP : REG_BOARD_TEMP);
or even
		reg = channel ? REG_SOC_TEMP : REG_BOARD_TEMP;
followed by unified read handling below.

> +		break;
> +	case hwmon_temp_crit:
> +		if (channel)
> +			return -EOPNOTSUPP;
> +

Those checks are unnecessary if the is_visible() function
does its job. If it doesn't, it is broken and needs to be fixed.
Returning -EOPNOTSUPP is ok for default: cases, because _something_
needs to be done, but not as result of extra and unnecessary checks
like this one.

> +		tmp = sg2042_mcu_get_soc_crit_temp(mcu->client);
> +		if (tmp < 0)
> +			return tmp;
> +		*val = tmp * 1000;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		if (channel)
> +			return -EOPNOTSUPP;
> +
> +		tmp = sg2042_mcu_get_soc_hyst_temp(mcu->client);
> +		if (tmp < 0)
> +			return tmp;
> +		*val = tmp * 1000;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}

There is a lot of repetition here. Setting the register in the switch statement, followed
by
	tmp = i2c_smbus_read_byte_data(client, reg);
	if (tmp < 0)
		return tmp;
	*val = tmp * 1000;
would be much simpler.


> +	return 0;
> +}
> +
> +static int sg2042_mcu_read(struct device *dev,
> +			   enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		if (attr != hwmon_chip_update_interval)
> +			return -EOPNOTSUPP;
> +		*val = 1000;
> +		break;

Pointless attribute. Attributes are only valuable if 1) used and
2) written into the chip. This one isn't used for anything.

> +	case hwmon_temp:
> +		return sg2042_mcu_read_temp(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +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);
> +	u8 down_temp, repower_temp;
> +	int ret;
> +
> +	if (type != hwmon_temp || attr != hwmon_temp_crit || !channel)
> +		return -EOPNOTSUPP;
> +
Pointless check if the is_visible() function does its job.

Also, this is inconsistent: It only accepts writes if channel > 0,
but the is_visible code only makes the attributes available for
channel 0. On top of that, how does this expession ever allow writing
hwmon_temp_crit_hyst ?

Either I am missing something, or you did not test this code.

> +	switch (attr) {
> +	case hwmon_temp_crit:
> +		ret = sg2042_mcu_get_soc_hyst_temp(mcu->client);
> +		if (ret < 0)
> +			repower_temp = DEFAULT_REPOWER_TEMP;

Why is this not an error ? If some of the supported boards
don't support it, the value should not be read in the first place,
and the hyst attribute should not be there.

> +		else
> +			repower_temp = ret;
> +
> +		down_temp = val / 1000;

val needs to be range checked.

> +		if (down_temp < repower_temp)
> +			return -EINVAL;
> +
> +		return sg2042_mcu_set_soc_crit_temp(mcu->client,
> +						    (u8)(val / 1000));

A read followed by a write needs to be mutex protected because the other value
could be changed at the same time from another process. I am not sure if those
checks are valuable or even make sense, but if you want that compexity you'll
have to add mutex protection.

Also, what is the point of recalculating down_temp ?

> +	case hwmon_temp_crit_hyst:
> +		ret = sg2042_mcu_get_soc_crit_temp(mcu->client);
> +		if (ret < 0)
> +			return -ENODEV;
> +

Do not overwrite error codes. If the attribute does not exist for some of the
boards, it should not be created in the first place, and no attempt should be
made to read it.

> +		down_temp = ret;
> +		repower_temp = val / 1000;

val needs to be range checked.

> +		if (down_temp < repower_temp)
> +			return -EINVAL;
> +
> +		return sg2042_mcu_set_soc_hyst_temp(mcu->client,
> +						    (u8)(val / 1000));

What is the point of recalculating repower_temp ?

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t sg2042_mcu_is_visible(const void *_data,
> +				     enum hwmon_sensor_types type,
> +				     u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		if (attr == hwmon_chip_update_interval)
> +			return 0444;
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			if (channel < 2)
> +				return 0444;

channel is always < 2.

> +			break;
> +		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) {
> +		_CREATE_DEBUG_ENTRY(firmware_version, 0444, mcu->debugfs, mcu);
> +		_CREATE_DEBUG_ENTRY(pcb_version, 0444, mcu->debugfs, mcu);
> +		_CREATE_DEBUG_ENTRY(mcu_type, 0444, mcu->debugfs, mcu);
> +		_CREATE_DEBUG_ENTRY(board_type, 0444, mcu->debugfs, mcu);
> +	}
> +}
> +
> +static int sg2042_mcu_check_board(u8 id)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(sg2042_boards_data); i++) {
> +		if (sg2042_boards_data[i].id == id)
> +			return i;
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	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;
> +
> +	ret = sg2042_mcu_get_board_type(client);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sg2042_mcu_check_board(ret);
> +	if (ret < 0)
> +		return ret;
> +
> +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
> +	if (!mcu)
> +		return -ENOMEM;
> +
> +	mcu->client = client;
> +	mcu->board_info = &sg2042_boards_data[ret];
> +
> +	ret = sysfs_create_group(&dev->kobj, &sg2042_mcu_attr_group);
> +	if (ret < 0)
> +		return ret;
> +

Why not use .dev_groups provided by struct device_driver ?

> +	i2c_set_clientdata(client, mcu);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +							 mcu,
> +							 &sg2042_mcu_chip_info,
> +							 NULL);
> +
> +	sg2042_mcu_debugfs_init(mcu, dev);
> +

This leaves the debugfs files orphan if the device (not the driver) is removed.
Also, it is still created even if devm_hwmon_device_register_with_info() returned
an error.

> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +
> +	sysfs_remove_group(&dev->kobj, &sg2042_mcu_attr_group);
> +}
> +
> +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" },

Based on sg2042_boards_data, this seems wrong. Devicetree data should distinguish the supported
boards. That is its whole point. The same is true for sg2042_mcu_id; there should be separate
entries for each of the supported boards.

> +	{},
> +};
> +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,
> +	},
> +	.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("sgmcu", 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
> 
> 

Re: [PATCH v6 1/2] dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor support

[Guenter Roeck]

On Wed, Jul 03, 2024 at 10:30:43AM +0800, Inochi Amaoto wrote:
> 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
> 
> --
> 2.45.2
> 
> 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

According to the other patch, this actually covers four
distinct models/devices.

static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
> +	{
> +		.id = 0x80,
> +		.name = "SG2042 evb x8",
> +	},
> +	{
> +		.id = 0x81,
> +		.name = "SG2042R evb",
> +	},
> +	{
> +		.id = 0x83,
> +		.name = "SG2042 evb x4",
> +	},
> +	{
> +		.id = 0x90,
> +		.name = "Milk-V Pioneer",
> +	},
> +};
> +

Is it really appropriate to use a single compatible property for all of those ?

Guenter

Re: [PATCH v6 1/2] dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor support

[Inochi Amaoto]

On Sat, Jul 06, 2024 at 09:48:58AM GMT, Guenter Roeck wrote:
> On Wed, Jul 03, 2024 at 10:30:43AM +0800, Inochi Amaoto wrote:
> > 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
> > 
> > --
> > 2.45.2
> > 
> > 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
> 
> According to the other patch, this actually covers four
> distinct models/devices.
> 
> static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
> > +	{
> > +		.id = 0x80,
> > +		.name = "SG2042 evb x8",
> > +	},
> > +	{
> > +		.id = 0x81,
> > +		.name = "SG2042R evb",
> > +	},
> > +	{
> > +		.id = 0x83,
> > +		.name = "SG2042 evb x4",
> > +	},
> > +	{
> > +		.id = 0x90,
> > +		.name = "Milk-V Pioneer",
> > +	},
> > +};
> > +
> 
> Is it really appropriate to use a single compatible property for all of those ?
> 
> Guenter

These board can only be detected at running time (even this should on
a specific board). On real world, it can only sees a MCU onboard.
I don't think it is a good idea to add some bindings to cover these
model. It seems better to remove this array and let userspace to parse
these ids.

Regards,
Inochi

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

[Inochi Amaoto]

On Sat, Jul 06, 2024 at 07:52:47AM GMT, Guenter Roeck wrote:
> On 7/2/24 19:30, Inochi Amaoto wrote:
> > 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/sgmcu.rst |  44 +++
> >   drivers/hwmon/Kconfig         |  11 +
> >   drivers/hwmon/Makefile        |   1 +
> >   drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
> >   5 files changed, 642 insertions(+)
> >   create mode 100644 Documentation/hwmon/sgmcu.rst
> >   create mode 100644 drivers/hwmon/sgmcu.c
> > 
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index 03d313af469a..189626b3a055 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
> >      sch5636
> >      scpi-hwmon
> >      sfctemp
> > +   sgmcu
> >      sht15
> >      sht21
> >      sht3x
> > diff --git a/Documentation/hwmon/sgmcu.rst b/Documentation/hwmon/sgmcu.rst
> > new file mode 100644
> > index 000000000000..5669dcfb2a33
> > --- /dev/null
> > +++ b/Documentation/hwmon/sgmcu.rst
> > @@ -0,0 +1,44 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver sgmcu
> > +=====================
> > +
> > +Supported chips:
> > +
> > +  * Onboard MCU for sg2042
> > +
> > +    Addresses scanned: -
> > +
> > +    Prefix: 'sgmcu'
> > +
> > +Authors:
> > +
> > +  - Inochi Amaoto <inochiama@outlook.com>
> > +
> > +Description
> > +-----------
> > +
> > +This driver supprts hardware monitoring for onboard MCU with
> > +PMBus 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.
> > +
> > +Platform data support
> > +---------------------
> > +
> > +The driver supports standard PMBus driver platform data.
> > +
> 
> It doesn't. Where does PMBus come into play ? Is this a cut-and-paste problem ?
> 

Yes, It should be smbus/i2c, I forgot to modify.

> > +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 e14ae18a973b..1100dd11f7f5 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -2079,6 +2079,17 @@ config SENSORS_SFCTEMP
> >   	  This driver can also be built as a module.  If so, the module
> >   	  will be called sfctemp.
> > 
> > +config SENSORS_SGMCU
> > +	tristate "Sophgo onboard MCU support"
> > +	depends on I2C
> > +	depends on ARCH_SOPHGO || COMPILE_TEST
> > +	help
> > +	  Support for onboard MCU of Sophgo 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 sgmcu.
> > +
> >   config SENSORS_SURFACE_FAN
> >   	tristate "Surface Fan Driver"
> >   	depends on SURFACE_AGGREGATOR
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e3f25475d1f0..e9b78ff8338e 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -195,6 +195,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_SGMCU)	+= sgmcu.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/sgmcu.c b/drivers/hwmon/sgmcu.c
> > new file mode 100644
> > index 000000000000..d941d6fe741f
> > --- /dev/null
> > +++ b/drivers/hwmon/sgmcu.c
> > @@ -0,0 +1,585 @@
> > +// 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/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +
> Alphabetic include file order, please.
> 
> > +/* 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			0x1
> > +#define CRITICAL_ACTION_POWEROFF		0x2
> > +
> 
> Please use BIT() for bit masks.
> 
> > +#define DEFAULT_REPOWER_TEMP			60
> > +#define MAX_REPOWER_TEMP			100
> > +
> > +#define sg2042_mcu_read_byte(client, reg)			\
> > +	i2c_smbus_read_byte_data(client, reg)
> > +#define sg2042_mcu_write_byte(client, reg, value)		\
> > +	i2c_smbus_write_byte_data(client, reg, value)
> > +#define sg2042_mcu_read_block(client, reg, array)		\
> > +	i2c_smbus_read_i2c_block_data(client, reg, sizeof(array), array)
> > +
> 
> Pointless defines. See below.
> 
> > +#define DEFINE_MCU_ATTR_READ_FUNC(_name, _type, _format)		\
> > +	static ssize_t _name##_show(struct device *dev,			\
> > +				    struct device_attribute *attr,	\
> > +				    char *buf)				\
> > +	{								\
> > +		struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);	\
> > +		_type ret;						\
> > +		ret = sg2042_mcu_get_##_name(mcu->client);		\
> > +		if (ret < 0)						\
> > +			return ret;					\
> > +		return sprintf(buf, _format "\n", ret);			\
> > +	}
> > +
> > +#define DEFINE_MCU_DEBUG_ATTR_READ_FUNC(_name, _type, _format)		\
> > +	static int _name##_show(struct seq_file *seqf,			\
> > +				    void *unused)			\
> > +	{								\
> > +		struct sg2042_mcu_data *mcu = seqf->private;		\
> > +		_type ret;						\
> > +		ret = sg2042_mcu_get_##_name(mcu->client);		\
> > +		if (ret < 0)						\
> > +			return ret;					\
> > +		seq_printf(seqf, _format "\n", ret);			\
> > +		return 0;						\
> > +	}
> > +
> > +#define _CREATE_DEBUG_ENTRY(name, perm, d, data)			\
> > +	debugfs_create_file(#name, perm, d, data, &name##_fops)
> > +
> 
> Pointless define. Please do not replace API function names with your own;
> that just makes it harder to find callers.
> 
> > +struct sg2042_mcu_board_data {
> > +	u8		id;
> > +	const char	*name;
> > +};
> > +
> > +struct sg2042_mcu_data {
> > +	struct i2c_client			*client;
> > +	const struct sg2042_mcu_board_data	*board_info;
> > +	struct dentry				*debugfs;
> > +};
> > +
> > +static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
> > +	{
> > +		.id = 0x80,
> > +		.name = "SG2042 evb x8",
> > +	},
> > +	{
> > +		.id = 0x81,
> > +		.name = "SG2042R evb",
> > +	},
> > +	{
> > +		.id = 0x83,
> > +		.name = "SG2042 evb x4",
> > +	},
> > +	{
> > +		.id = 0x90,
> > +		.name = "Milk-V Pioneer",
> > +	},
> > +};
> > +
> > +static const char *sg2042_mcu_reset_reason[8] = {
> > +	"Power supply overheat",
> > +	"Power supply failure",
> > +	"12V power supply failure",
> > +	"Reset commant",
> 
> command
> 
> > +	"Unknown",
> > +	"Unknown",
> > +	"Unknown",
> > +	"SoC overheat",
> > +};
> > +
> > +static struct dentry *sgmcu_debugfs;
> > +
> > +static int sg2042_mcu_get_board_type(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_BOARD_TYPE);
> > +}
> > +
> > +static int sg2042_mcu_get_firmware_version(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_MCU_FIRMWARE_VERSION);
> > +}
> > +
> > +static int sg2042_mcu_get_pcb_version(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_PCB_VERSION);
> > +}
> > +
> > +static int sg2042_mcu_get_soc_temp(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_SOC_TEMP);
> > +}
> > +
> > +static int sg2042_mcu_get_board_temp(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_BOARD_TEMP);
> > +}
> > +
> > +static int sg2042_mcu_get_reset_count(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_RST_COUNT);
> > +}
> > +
> 
> Those one-line functions are just as pointless.
> 	i2c_smbus_read_byte_data(client, REG_RST_COUNT);
> does exactly the same without extra function and define.
> 
> > +static s32 sg2042_mcu_get_uptime(struct i2c_client *client)
> > +{
> > +	int ret;
> > +	u8 time_val[2];
> > +
> > +	ret = sg2042_mcu_read_block(client, REG_UPTIME, time_val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return (s32)(time_val[0]) + ((s32)(time_val[1]) << 8);
> > +}
> > +
> 
> This is the only one of those functions which adds at least some value.
> 
> > +static int sg2042_mcu_get_reset_reason(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_RESET_REASON);
> > +}
> > +
> > +static int sg2042_mcu_get_mcu_type(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_MCU_TYPE);
> > +}
> > +
> > +static int sg2042_mcu_get_soc_crit_action(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_CRITICAL_ACTIONS);
> > +}
> > +
> > +static int sg2042_mcu_get_soc_crit_temp(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_CRITICAL_TEMP);
> > +}
> > +
> > +static int sg2042_mcu_get_soc_hyst_temp(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_REPOWER_TEMP);
> > +}
> > +
> > +static int sg2042_mcu_set_soc_crit_action(struct i2c_client *client,
> > +					  u8 value)
> > +{
> > +	return sg2042_mcu_write_byte(client, REG_CRITICAL_ACTIONS, value);
> > +}
> > +
> > +static int sg2042_mcu_set_soc_crit_temp(struct i2c_client *client,
> > +					u8 value)
> > +{
> > +	return sg2042_mcu_write_byte(client, REG_CRITICAL_TEMP, value);
> > +}
> > +
> > +static int sg2042_mcu_set_soc_hyst_temp(struct i2c_client *client,
> > +					u8 value)
> > +{
> > +	return sg2042_mcu_write_byte(client, REG_REPOWER_TEMP, value);
> > +}
> > +
> > +DEFINE_MCU_ATTR_READ_FUNC(reset_count, int, "%d");
> > +DEFINE_MCU_ATTR_READ_FUNC(uptime, s32, "%d");
> > +
> > +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, val, i;
> > +
> > +	val = sg2042_mcu_get_reset_reason(mcu->client);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	ret = sprintf(buf, "Reason: 0x%02x\n", val);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(sg2042_mcu_reset_reason); i++) {
> > +		if (val & BIT(i))
> > +			ret += sprintf(buf + ret, "bit %d: %s\n", i,
> > +						  sg2042_mcu_reset_reason[i]);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> This violates sysfs standards (one word only). If you want that level of detail,
> add it to debugfs.
> 

I think it would be better to change name and leave it in the sysfs.
If a suitable world is not found, I will move it to debugfs.

> > +
> > +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 = sg2042_mcu_get_soc_crit_action(mcu->client);
> > +	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);
> > +	int value;
> > +
> > +	if (sysfs_streq("reboot", buf))
> > +		value = CRITICAL_ACTION_REBOOT;
> > +	else if (sysfs_streq("poweroff", buf))
> > +		value = CRITICAL_ACTION_POWEROFF;
> > +	else
> > +		return -EINVAL;
> > +
> > +	return sg2042_mcu_set_soc_crit_action(mcu->client, value);
> > +}
> > +
> > +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_READ_FUNC(firmware_version, int, "0x%02x");
> > +DEFINE_MCU_DEBUG_ATTR_READ_FUNC(pcb_version, int, "0x%02x");
> > +
> > +static int board_type_show(struct seq_file *seqf, void *unused)
> > +{
> > +	struct sg2042_mcu_data *mcu = seqf->private;
> > +
> > +	seq_printf(seqf, "%s\n", mcu->board_info->name ?: "Unknown");
> > +
> > +	return 0;
> > +}
> > +
> > +static int mcu_type_show(struct seq_file *seqf, void *unused)
> > +{
> > +	struct sg2042_mcu_data *mcu = seqf->private;
> > +	int ret;
> > +
> > +	ret = sg2042_mcu_get_mcu_type(mcu->client);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	seq_puts(seqf, ret ? "GD32\n" : "STM32\n");
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(firmware_version);
> > +DEFINE_SHOW_ATTRIBUTE(pcb_version);
> > +DEFINE_SHOW_ATTRIBUTE(mcu_type);
> > +DEFINE_SHOW_ATTRIBUTE(board_type);
> > +
> > +// TODO: to debugfs
> > +
> 
> If there is a TODO left, move the driver to drivers/staging/
> and keep it there until it is complete.
> 

My fault, it is already finished and can be removed.

> > +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 hwmon_channel_info * const sg2042_mcu_info[] = {
> > +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
> > +	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);
> > +	long tmp;
> 
> Why long ?
> 

just use to calculate the value.

> > +
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		switch (channel) {
> > +		case 0:
> > +			tmp = sg2042_mcu_get_soc_temp(mcu->client);
> > +			if (tmp < 0)
> > +				return tmp;
> > +			*val = tmp * 1000;
> > +			break;
> > +		case 1:
> > +			tmp = sg2042_mcu_get_board_temp(mcu->client);
> > +			if (tmp < 0)
> > +				return tmp;
> > +			*val = tmp * 1000;
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> 
> This would be much simpler written with something like
> 		tmp = i2c_smbus_read_byte_data(client, channel ? REG_SOC_TEMP : REG_BOARD_TEMP);
> or even
> 		reg = channel ? REG_SOC_TEMP : REG_BOARD_TEMP;
> followed by unified read handling below.
> 

You are right, thanks.

> > +		break;
> > +	case hwmon_temp_crit:
> > +		if (channel)
> > +			return -EOPNOTSUPP;
> > +
> 
> Those checks are unnecessary if the is_visible() function
> does its job. If it doesn't, it is broken and needs to be fixed.
> Returning -EOPNOTSUPP is ok for default: cases, because _something_
> needs to be done, but not as result of extra and unnecessary checks
> like this one.
> 

Thanks for this explanation. I will change it.

> > +		tmp = sg2042_mcu_get_soc_crit_temp(mcu->client);
> > +		if (tmp < 0)
> > +			return tmp;
> > +		*val = tmp * 1000;
> > +		break;
> > +	case hwmon_temp_crit_hyst:
> > +		if (channel)
> > +			return -EOPNOTSUPP;
> > +
> > +		tmp = sg2042_mcu_get_soc_hyst_temp(mcu->client);
> > +		if (tmp < 0)
> > +			return tmp;
> > +		*val = tmp * 1000;
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> 
> There is a lot of repetition here. Setting the register in the switch statement, followed
> by
> 	tmp = i2c_smbus_read_byte_data(client, reg);
> 	if (tmp < 0)
> 		return tmp;
> 	*val = tmp * 1000;
> would be much simpler.
> 
> 
> > +	return 0;
> > +}
> > +
> > +static int sg2042_mcu_read(struct device *dev,
> > +			   enum hwmon_sensor_types type,
> > +			   u32 attr, int channel, long *val)
> > +{
> > +	switch (type) {
> > +	case hwmon_chip:
> > +		if (attr != hwmon_chip_update_interval)
> > +			return -EOPNOTSUPP;
> > +		*val = 1000;
> > +		break;
> 
> Pointless attribute. Attributes are only valuable if 1) used and
> 2) written into the chip. This one isn't used for anything.
> 

OK. I will remove this.

> > +	case hwmon_temp:
> > +		return sg2042_mcu_read_temp(dev, attr, channel, val);
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +	return 0;
> > +}
> > +
> > +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);
> > +	u8 down_temp, repower_temp;
> > +	int ret;
> > +
> > +	if (type != hwmon_temp || attr != hwmon_temp_crit || !channel)
> > +		return -EOPNOTSUPP;
> > +
> Pointless check if the is_visible() function does its job.
> 
> Also, this is inconsistent: It only accepts writes if channel > 0,
> but the is_visible code only makes the attributes available for
> channel 0. On top of that, how does this expession ever allow writing
> hwmon_temp_crit_hyst ?
> 
> Either I am missing something, or you did not test this code.
> 

This is my fault. Only channel 0 can be writed.

> > +	switch (attr) {
> > +	case hwmon_temp_crit:
> > +		ret = sg2042_mcu_get_soc_hyst_temp(mcu->client);
> > +		if (ret < 0)
> > +			repower_temp = DEFAULT_REPOWER_TEMP;
> 
> Why is this not an error ? If some of the supported boards
> don't support it, the value should not be read in the first place,
> and the hyst attribute should not be there.
> 

At least for now, all the chip this driver support can get
hyst_temp. So with your advice, I will issue an error.

> > +		else
> > +			repower_temp = ret;
> > +
> > +		down_temp = val / 1000;
> 
> val needs to be range checked.
> 
> > +		if (down_temp < repower_temp)
> > +			return -EINVAL;
> > +
> > +		return sg2042_mcu_set_soc_crit_temp(mcu->client,
> > +						    (u8)(val / 1000));
> 
> A read followed by a write needs to be mutex protected because the other value
> could be changed at the same time from another process. I am not sure if those
> checks are valuable or even make sense, but if you want that compexity you'll
> have to add mutex protection.
> 

Thanks, this is what I forgot.

> Also, what is the point of recalculating down_temp ?
> 

After some real test, I found the sg2042 can not handle the case
when "down_temp < repower_temp", it will keep closing. To avoid
this, I added this check. This is the same for the repower_temp.

> > +	case hwmon_temp_crit_hyst:
> > +		ret = sg2042_mcu_get_soc_crit_temp(mcu->client);
> > +		if (ret < 0)
> > +			return -ENODEV;
> > +
> 
> Do not overwrite error codes. If the attribute does not exist for some of the
> boards, it should not be created in the first place, and no attempt should be
> made to read it.
> 
> > +		down_temp = ret;
> > +		repower_temp = val / 1000;
> 
> val needs to be range checked.
> 
> > +		if (down_temp < repower_temp)
> > +			return -EINVAL;
> > +
> > +		return sg2042_mcu_set_soc_hyst_temp(mcu->client,
> > +						    (u8)(val / 1000));
> 
> What is the point of recalculating repower_temp ?
> 
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> > +
> > +static umode_t sg2042_mcu_is_visible(const void *_data,
> > +				     enum hwmon_sensor_types type,
> > +				     u32 attr, int channel)
> > +{
> > +	switch (type) {
> > +	case hwmon_chip:
> > +		if (attr == hwmon_chip_update_interval)
> > +			return 0444;
> > +		break;
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_input:
> > +			if (channel < 2)
> > +				return 0444;
> 
> channel is always < 2.
> 
> > +			break;
> > +		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) {
> > +		_CREATE_DEBUG_ENTRY(firmware_version, 0444, mcu->debugfs, mcu);
> > +		_CREATE_DEBUG_ENTRY(pcb_version, 0444, mcu->debugfs, mcu);
> > +		_CREATE_DEBUG_ENTRY(mcu_type, 0444, mcu->debugfs, mcu);
> > +		_CREATE_DEBUG_ENTRY(board_type, 0444, mcu->debugfs, mcu);
> > +	}
> > +}
> > +
> > +static int sg2042_mcu_check_board(u8 id)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(sg2042_boards_data); i++) {
> > +		if (sg2042_boards_data[i].id == id)
> > +			return i;
> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +
> > +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
> > +{
> > +	int ret;
> > +	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;
> > +
> > +	ret = sg2042_mcu_get_board_type(client);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sg2042_mcu_check_board(ret);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
> > +	if (!mcu)
> > +		return -ENOMEM;
> > +
> > +	mcu->client = client;
> > +	mcu->board_info = &sg2042_boards_data[ret];
> > +
> > +	ret = sysfs_create_group(&dev->kobj, &sg2042_mcu_attr_group);
> > +	if (ret < 0)
> > +		return ret;
> > +
> 
> Why not use .dev_groups provided by struct device_driver ?
> 
> > +	i2c_set_clientdata(client, mcu);
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> > +							 mcu,
> > +							 &sg2042_mcu_chip_info,
> > +							 NULL);
> > +
> > +	sg2042_mcu_debugfs_init(mcu, dev);
> > +
> 
> This leaves the debugfs files orphan if the device (not the driver) is removed.
> Also, it is still created even if devm_hwmon_device_register_with_info() returned
> an error.
> 

OK, I forgot this condition, I will add necessary check, thanks.

> > +	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +
> > +	sysfs_remove_group(&dev->kobj, &sg2042_mcu_attr_group);
> > +}
> > +
> > +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" },
> 
> Based on sg2042_boards_data, this seems wrong. Devicetree data should distinguish the supported
> boards. That is its whole point. The same is true for sg2042_mcu_id; there should be separate
> entries for each of the supported boards.
> 
> > +	{},
> > +};
> > +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,
> > +	},
> > +	.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("sgmcu", 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
> > 
> > 
> 

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

[Guenter Roeck]

On 7/6/24 17:31, Inochi Amaoto wrote:
> On Sat, Jul 06, 2024 at 07:52:47AM GMT, Guenter Roeck wrote:
>> On 7/2/24 19:30, Inochi Amaoto wrote:
>>> 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/sgmcu.rst |  44 +++
>>>    drivers/hwmon/Kconfig         |  11 +
>>>    drivers/hwmon/Makefile        |   1 +
>>>    drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
>>>    5 files changed, 642 insertions(+)
>>>    create mode 100644 Documentation/hwmon/sgmcu.rst
>>>    create mode 100644 drivers/hwmon/sgmcu.c
>>>
...
>>> +
>>> +static int sg2042_mcu_read_temp(struct device *dev,
>>> +				u32 attr, int channel,
>>> +				long *val)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> +	long tmp;
>>
>> Why long ?
>>
> 
> just use to calculate the value.
> 

That is not a valid reason. There won't be any overflows, so int would
be good enough.

Guenter

Re: [PATCH v6 1/2] dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor support

[Guenter Roeck]

On 7/6/24 16:34, Inochi Amaoto wrote:
> On Sat, Jul 06, 2024 at 09:48:58AM GMT, Guenter Roeck wrote:
>> On Wed, Jul 03, 2024 at 10:30:43AM +0800, Inochi Amaoto wrote:
>>> 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
>>>
>>> --
>>> 2.45.2
>>>
>>> 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
>>
>> According to the other patch, this actually covers four
>> distinct models/devices.
>>
>> static const struct sg2042_mcu_board_data[] = {
>>> +	{
>>> +		.id = 0x80,
>>> +		.name = "SG2042 evb x8",
>>> +	},
>>> +	{
>>> +		.id = 0x81,
>>> +		.name = "SG2042R evb",
>>> +	},
>>> +	{
>>> +		.id = 0x83,
>>> +		.name = "SG2042 evb x4",
>>> +	},
>>> +	{
>>> +		.id = 0x90,
>>> +		.name = "Milk-V Pioneer",
>>> +	},
>>> +};
>>> +
>>
>> Is it really appropriate to use a single compatible property for all of those ?
>>
>> Guenter
> 
> These board can only be detected at running time (even this should on
> a specific board). On real world, it can only sees a MCU onboard.
> I don't think it is a good idea to add some bindings to cover these
> model. It seems better to remove this array and let userspace to parse
> these ids.
> 

Isn't that what devicetree is for ? It should either not be necessary
to distinguish the models because they all behave the same and all of them
are happy with the same compatible property, or there is some difference
which should be reflected in devicetree. In other words, either they
are all compatible with sophgo,sg2042-hwmon-mcu, and sg2042_mcu_board_data
as well as sg2042_mcu_check_board() are unnecessary, or they are not
compatible and there should be separate compatible property names.
struct of_device_data does have a .data pointer for a reason, after all.

In yet other words, it seems odd to have a devicetree file for Milk-V Pioneer
and then to check in the hwmon driver if this is _really_ a Milk-V Pioneer
and bail out if it isn't. And I _really_ don't want to deal with continuous
driver patches whenever one of the systems using one of those MCUs starts
shipping a new firmware version or is deployed on a new board variant.

Thanks,
Guenter

Re: [PATCH v6 1/2] dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor support

[Inochi Amaoto]

On Sat, Jul 06, 2024 at 06:35:35PM GMT, Guenter Roeck wrote:
> On 7/6/24 16:34, Inochi Amaoto wrote:
> > On Sat, Jul 06, 2024 at 09:48:58AM GMT, Guenter Roeck wrote:
> > > On Wed, Jul 03, 2024 at 10:30:43AM +0800, Inochi Amaoto wrote:
> > > > 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
> > > > 
> > > > --
> > > > 2.45.2
> > > > 
> > > > 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
> > > 
> > > According to the other patch, this actually covers four
> > > distinct models/devices.
> > > 
> > > static const struct sg2042_mcu_board_data[] = {
> > > > +	{
> > > > +		.id = 0x80,
> > > > +		.name = "SG2042 evb x8",
> > > > +	},
> > > > +	{
> > > > +		.id = 0x81,
> > > > +		.name = "SG2042R evb",
> > > > +	},
> > > > +	{
> > > > +		.id = 0x83,
> > > > +		.name = "SG2042 evb x4",
> > > > +	},
> > > > +	{
> > > > +		.id = 0x90,
> > > > +		.name = "Milk-V Pioneer",
> > > > +	},
> > > > +};
> > > > +
> > > 
> > > Is it really appropriate to use a single compatible property for all of those ?
> > > 
> > > Guenter
> > 
> > These board can only be detected at running time (even this should on
> > a specific board). On real world, it can only sees a MCU onboard.
> > I don't think it is a good idea to add some bindings to cover these
> > model. It seems better to remove this array and let userspace to parse
> > these ids.
> > 
> 
> Isn't that what devicetree is for ? It should either not be necessary
> to distinguish the models because they all behave the same and all of them
> are happy with the same compatible property, or there is some difference
> which should be reflected in devicetree. In other words, either they
> are all compatible with sophgo,sg2042-hwmon-mcu, and sg2042_mcu_board_data
> as well as sg2042_mcu_check_board() are unnecessary, or they are not
> compatible and there should be separate compatible property names.
> struct of_device_data does have a .data pointer for a reason, after all.
> 
> In yet other words, it seems odd to have a devicetree file for Milk-V Pioneer
> and then to check in the hwmon driver if this is _really_ a Milk-V Pioneer
> and bail out if it isn't. And I _really_ don't want to deal with continuous
> driver patches whenever one of the systems using one of those MCUs starts
> shipping a new firmware version or is deployed on a new board variant.
> 
> Thanks,
> Guenter
> 

When I firstly added this compatible, I want to match all boards with this
SoC. This is why I add compatiable like "sg2042-xxx". In driver, I tested
the id of the MCU to ensure it is sg2042 (although the id can used to detect
board). And this is why I said that I was not sure it is better to add board
specific compatible.

I know you concern about continuous patches about new board or new firmware
version. I was told the MCU use almost the same register layout to provide
necessary information about its SoC. So is it better to remove the id check
and let the dts write to decide?

Regards,
Inochi

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

[Chen Wang]

On 2024/7/3 10:30, Inochi Amaoto wrote:
> 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/sgmcu.rst |  44 +++
>   drivers/hwmon/Kconfig         |  11 +
>   drivers/hwmon/Makefile        |   1 +
>   drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
>   5 files changed, 642 insertions(+)
>   create mode 100644 Documentation/hwmon/sgmcu.rst
>   create mode 100644 drivers/hwmon/sgmcu.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 03d313af469a..189626b3a055 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
>      sch5636
>      scpi-hwmon
>      sfctemp
> +   sgmcu
This driver is for sg2042 only, right? "sgmcu" looks be general for all 
sophgo products.
>      sht15
>      sht21
>      sht3x
> diff --git a/Documentation/hwmon/sgmcu.rst b/Documentation/hwmon/sgmcu.rst
> new file mode 100644
> index 000000000000..5669dcfb2a33
> --- /dev/null
> +++ b/Documentation/hwmon/sgmcu.rst
Same question as upon.
> @@ -0,0 +1,44 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver sgmcu
> +=====================
> +
> +Supported chips:
> +
> +  * Onboard MCU for sg2042
> +
> +    Addresses scanned: -
> +
> +    Prefix: 'sgmcu'
Same question as upon.
> +
> +Authors:
> +
> +  - Inochi Amaoto <inochiama@outlook.com>
> +
> +Description
> +-----------
> +
> +This driver supprts hardware monitoring for onboard MCU with
> +PMBus 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.
> +
> +Platform data support
> +---------------------
> +
> +The driver supports standard PMBus driver platform data.
> +
> +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 e14ae18a973b..1100dd11f7f5 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2079,6 +2079,17 @@ config SENSORS_SFCTEMP
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called sfctemp.
>
> +config SENSORS_SGMCU
Same question as upon.
> +	tristate "Sophgo onboard MCU support"
Same question as upon.
> +	depends on I2C
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	help
> +	  Support for onboard MCU of Sophgo 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 sgmcu.
> +
>   config SENSORS_SURFACE_FAN
>   	tristate "Surface Fan Driver"
>   	depends on SURFACE_AGGREGATOR
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e3f25475d1f0..e9b78ff8338e 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -195,6 +195,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_SGMCU)	+= sgmcu.o
Same question as upon.
>   obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
>   obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>   obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> diff --git a/drivers/hwmon/sgmcu.c b/drivers/hwmon/sgmcu.c
> new file mode 100644
> index 000000000000..d941d6fe741f
> --- /dev/null
> +++ b/drivers/hwmon/sgmcu.c
Same question as upon.
> @@ -0,0 +1,585 @@
> +// 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.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			0x1
> +#define CRITICAL_ACTION_POWEROFF		0x2
> +
> +#define DEFAULT_REPOWER_TEMP			60
> +#define MAX_REPOWER_TEMP			100
> +
> +#define sg2042_mcu_read_byte(client, reg)			\
> +	i2c_smbus_read_byte_data(client, reg)
> +#define sg2042_mcu_write_byte(client, reg, value)		\
> +	i2c_smbus_write_byte_data(client, reg, value)
> +#define sg2042_mcu_read_block(client, reg, array)		\
> +	i2c_smbus_read_i2c_block_data(client, reg, sizeof(array), array)
> +
> +#define DEFINE_MCU_ATTR_READ_FUNC(_name, _type, _format)		\
> +	static ssize_t _name##_show(struct device *dev,			\
> +				    struct device_attribute *attr,	\
> +				    char *buf)				\
> +	{								\
> +		struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);	\
> +		_type ret;						\
> +		ret = sg2042_mcu_get_##_name(mcu->client);		\
> +		if (ret < 0)						\
> +			return ret;					\
> +		return sprintf(buf, _format "\n", ret);			\
> +	}
> +
> +#define DEFINE_MCU_DEBUG_ATTR_READ_FUNC(_name, _type, _format)		\
> +	static int _name##_show(struct seq_file *seqf,			\
> +				    void *unused)			\
> +	{								\
> +		struct sg2042_mcu_data *mcu = seqf->private;		\
> +		_type ret;						\
> +		ret = sg2042_mcu_get_##_name(mcu->client);		\
> +		if (ret < 0)						\
> +			return ret;					\
> +		seq_printf(seqf, _format "\n", ret);			\
> +		return 0;						\
> +	}
> +
> +#define _CREATE_DEBUG_ENTRY(name, perm, d, data)			\
> +	debugfs_create_file(#name, perm, d, data, &name##_fops)
> +
> +struct sg2042_mcu_board_data {
> +	u8		id;
> +	const char	*name;
> +};
> +
> +struct sg2042_mcu_data {
> +	struct i2c_client			*client;
> +	const struct sg2042_mcu_board_data	*board_info;
> +	struct dentry				*debugfs;
> +};
> +
> +static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
> +	{
> +		.id = 0x80,
> +		.name = "SG2042 evb x8",
> +	},
> +	{
> +		.id = 0x81,
> +		.name = "SG2042R evb",
> +	},
> +	{
> +		.id = 0x83,
> +		.name = "SG2042 evb x4",
> +	},
> +	{
> +		.id = 0x90,
> +		.name = "Milk-V Pioneer",
> +	},
> +};

Upstream kernel DTS only supports Milk-V pioneer, evb boards are not 
supported in plan. Can we figure a method to let user extend this 
outside kernel or just ask vendor to patch this when necessary?

> +
> +static const char *sg2042_mcu_reset_reason[8] = {
> +	"Power supply overheat",
> +	"Power supply failure",
> +	"12V power supply failure",
> +	"Reset commant",
> +	"Unknown",
> +	"Unknown",
> +	"Unknown",
> +	"SoC overheat",
> +};
> +
> +static struct dentry *sgmcu_debugfs;
> +
> +static int sg2042_mcu_get_board_type(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_BOARD_TYPE);
> +}
> +
> +static int sg2042_mcu_get_firmware_version(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_MCU_FIRMWARE_VERSION);
> +}
> +
> +static int sg2042_mcu_get_pcb_version(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_PCB_VERSION);
> +}
> +
> +static int sg2042_mcu_get_soc_temp(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_SOC_TEMP);
> +}
> +
> +static int sg2042_mcu_get_board_temp(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_BOARD_TEMP);
> +}
> +
> +static int sg2042_mcu_get_reset_count(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_RST_COUNT);
> +}
> +
> +static s32 sg2042_mcu_get_uptime(struct i2c_client *client)
> +{
> +	int ret;
> +	u8 time_val[2];
> +
> +	ret = sg2042_mcu_read_block(client, REG_UPTIME, time_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return (s32)(time_val[0]) + ((s32)(time_val[1]) << 8);
> +}
> +
> +static int sg2042_mcu_get_reset_reason(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_RESET_REASON);
> +}
> +
> +static int sg2042_mcu_get_mcu_type(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_MCU_TYPE);
> +}
> +
> +static int sg2042_mcu_get_soc_crit_action(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_CRITICAL_ACTIONS);
> +}
> +
> +static int sg2042_mcu_get_soc_crit_temp(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_CRITICAL_TEMP);
> +}
> +
> +static int sg2042_mcu_get_soc_hyst_temp(struct i2c_client *client)
> +{
> +	return sg2042_mcu_read_byte(client, REG_REPOWER_TEMP);
> +}
> +
> +static int sg2042_mcu_set_soc_crit_action(struct i2c_client *client,
> +					  u8 value)
> +{
> +	return sg2042_mcu_write_byte(client, REG_CRITICAL_ACTIONS, value);
> +}
> +
> +static int sg2042_mcu_set_soc_crit_temp(struct i2c_client *client,
> +					u8 value)
> +{
> +	return sg2042_mcu_write_byte(client, REG_CRITICAL_TEMP, value);
> +}
> +
> +static int sg2042_mcu_set_soc_hyst_temp(struct i2c_client *client,
> +					u8 value)
> +{
> +	return sg2042_mcu_write_byte(client, REG_REPOWER_TEMP, value);
> +}
> +
> +DEFINE_MCU_ATTR_READ_FUNC(reset_count, int, "%d");
> +DEFINE_MCU_ATTR_READ_FUNC(uptime, s32, "%d");
> +
> +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, val, i;
> +
> +	val = sg2042_mcu_get_reset_reason(mcu->client);
> +	if (val < 0)
> +		return val;
> +
> +	ret = sprintf(buf, "Reason: 0x%02x\n", val);
> +
> +	for (i = 0; i < ARRAY_SIZE(sg2042_mcu_reset_reason); i++) {
> +		if (val & BIT(i))
> +			ret += sprintf(buf + ret, "bit %d: %s\n", i,
> +						  sg2042_mcu_reset_reason[i]);
> +	}
> +
> +	return 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 = sg2042_mcu_get_soc_crit_action(mcu->client);
> +	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);
> +	int value;
> +
> +	if (sysfs_streq("reboot", buf))
> +		value = CRITICAL_ACTION_REBOOT;
> +	else if (sysfs_streq("poweroff", buf))
> +		value = CRITICAL_ACTION_POWEROFF;
> +	else
> +		return -EINVAL;
> +
> +	return sg2042_mcu_set_soc_crit_action(mcu->client, value);
> +}
> +
> +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_READ_FUNC(firmware_version, int, "0x%02x");
> +DEFINE_MCU_DEBUG_ATTR_READ_FUNC(pcb_version, int, "0x%02x");
> +
> +static int board_type_show(struct seq_file *seqf, void *unused)
> +{
> +	struct sg2042_mcu_data *mcu = seqf->private;
> +
> +	seq_printf(seqf, "%s\n", mcu->board_info->name ?: "Unknown");
> +
> +	return 0;
> +}
> +
> +static int mcu_type_show(struct seq_file *seqf, void *unused)
> +{
> +	struct sg2042_mcu_data *mcu = seqf->private;
> +	int ret;
> +
> +	ret = sg2042_mcu_get_mcu_type(mcu->client);
> +	if (ret < 0)
> +		return ret;
> +
> +	seq_puts(seqf, ret ? "GD32\n" : "STM32\n");
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(firmware_version);
> +DEFINE_SHOW_ATTRIBUTE(pcb_version);
> +DEFINE_SHOW_ATTRIBUTE(mcu_type);
> +DEFINE_SHOW_ATTRIBUTE(board_type);
> +
> +// TODO: to debugfs
> +
> +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 hwmon_channel_info * const sg2042_mcu_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
> +	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);
> +	long tmp;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		switch (channel) {
> +		case 0:
> +			tmp = sg2042_mcu_get_soc_temp(mcu->client);
> +			if (tmp < 0)
> +				return tmp;
> +			*val = tmp * 1000;
> +			break;
> +		case 1:
> +			tmp = sg2042_mcu_get_board_temp(mcu->client);
> +			if (tmp < 0)
> +				return tmp;
> +			*val = tmp * 1000;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	case hwmon_temp_crit:
> +		if (channel)
> +			return -EOPNOTSUPP;
> +
> +		tmp = sg2042_mcu_get_soc_crit_temp(mcu->client);
> +		if (tmp < 0)
> +			return tmp;
> +		*val = tmp * 1000;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		if (channel)
> +			return -EOPNOTSUPP;
> +
> +		tmp = sg2042_mcu_get_soc_hyst_temp(mcu->client);
> +		if (tmp < 0)
> +			return tmp;
> +		*val = tmp * 1000;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int sg2042_mcu_read(struct device *dev,
> +			   enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		if (attr != hwmon_chip_update_interval)
> +			return -EOPNOTSUPP;
> +		*val = 1000;
> +		break;
> +	case hwmon_temp:
> +		return sg2042_mcu_read_temp(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +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);
> +	u8 down_temp, repower_temp;
> +	int ret;
> +
> +	if (type != hwmon_temp || attr != hwmon_temp_crit || !channel)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_temp_crit:
> +		ret = sg2042_mcu_get_soc_hyst_temp(mcu->client);
> +		if (ret < 0)
> +			repower_temp = DEFAULT_REPOWER_TEMP;
> +		else
> +			repower_temp = ret;
> +
> +		down_temp = val / 1000;
> +		if (down_temp < repower_temp)
> +			return -EINVAL;
> +
> +		return sg2042_mcu_set_soc_crit_temp(mcu->client,
> +						    (u8)(val / 1000));
> +	case hwmon_temp_crit_hyst:
> +		ret = sg2042_mcu_get_soc_crit_temp(mcu->client);
> +		if (ret < 0)
> +			return -ENODEV;
> +
> +		down_temp = ret;
> +		repower_temp = val / 1000;
> +		if (down_temp < repower_temp)
> +			return -EINVAL;
> +
> +		return sg2042_mcu_set_soc_hyst_temp(mcu->client,
> +						    (u8)(val / 1000));
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t sg2042_mcu_is_visible(const void *_data,
> +				     enum hwmon_sensor_types type,
> +				     u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		if (attr == hwmon_chip_update_interval)
> +			return 0444;
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			if (channel < 2)
> +				return 0444;
> +			break;
> +		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) {
> +		_CREATE_DEBUG_ENTRY(firmware_version, 0444, mcu->debugfs, mcu);
> +		_CREATE_DEBUG_ENTRY(pcb_version, 0444, mcu->debugfs, mcu);
> +		_CREATE_DEBUG_ENTRY(mcu_type, 0444, mcu->debugfs, mcu);
> +		_CREATE_DEBUG_ENTRY(board_type, 0444, mcu->debugfs, mcu);
> +	}
> +}
> +
> +static int sg2042_mcu_check_board(u8 id)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(sg2042_boards_data); i++) {
> +		if (sg2042_boards_data[i].id == id)
> +			return i;
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	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;
> +
> +	ret = sg2042_mcu_get_board_type(client);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sg2042_mcu_check_board(ret);
> +	if (ret < 0)
> +		return ret;
> +
> +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
> +	if (!mcu)
> +		return -ENOMEM;
> +
> +	mcu->client = client;
> +	mcu->board_info = &sg2042_boards_data[ret];
> +
> +	ret = sysfs_create_group(&dev->kobj, &sg2042_mcu_attr_group);
> +	if (ret < 0)
> +		return ret;
> +
> +	i2c_set_clientdata(client, mcu);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +							 mcu,
> +							 &sg2042_mcu_chip_info,
> +							 NULL);
> +
> +	sg2042_mcu_debugfs_init(mcu, dev);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +
> +	sysfs_remove_group(&dev->kobj, &sg2042_mcu_attr_group);
> +}
> +
> +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,
> +	},
> +	.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("sgmcu", 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
>

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

[Inochi Amaoto]

On Mon, Jul 08, 2024 at 08:25:55AM GMT, Chen Wang wrote:
> 
> On 2024/7/3 10:30, Inochi Amaoto wrote:
> > 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/sgmcu.rst |  44 +++
> >   drivers/hwmon/Kconfig         |  11 +
> >   drivers/hwmon/Makefile        |   1 +
> >   drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
> >   5 files changed, 642 insertions(+)
> >   create mode 100644 Documentation/hwmon/sgmcu.rst
> >   create mode 100644 drivers/hwmon/sgmcu.c
> > 
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index 03d313af469a..189626b3a055 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
> >      sch5636
> >      scpi-hwmon
> >      sfctemp
> > +   sgmcu
> This driver is for sg2042 only, right? "sgmcu" looks be general for all
> sophgo products.

Yes, according to sophgo, it use this mechanism for multiple products,
so I switch to a general name.

> >      sht15
> >      sht21
> >      sht3x
> > diff --git a/Documentation/hwmon/sgmcu.rst b/Documentation/hwmon/sgmcu.rst
> > new file mode 100644
> > index 000000000000..5669dcfb2a33
> > --- /dev/null
> > +++ b/Documentation/hwmon/sgmcu.rst
> Same question as upon.
> > @@ -0,0 +1,44 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver sgmcu
> > +=====================
> > +
> > +Supported chips:
> > +
> > +  * Onboard MCU for sg2042
> > +
> > +    Addresses scanned: -
> > +
> > +    Prefix: 'sgmcu'
> Same question as upon.
> > +
> > +Authors:
> > +
> > +  - Inochi Amaoto <inochiama@outlook.com>
> > +
> > +Description
> > +-----------
> > +
> > +This driver supprts hardware monitoring for onboard MCU with
> > +PMBus 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.
> > +
> > +Platform data support
> > +---------------------
> > +
> > +The driver supports standard PMBus driver platform data.
> > +
> > +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 e14ae18a973b..1100dd11f7f5 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -2079,6 +2079,17 @@ config SENSORS_SFCTEMP
> >   	  This driver can also be built as a module.  If so, the module
> >   	  will be called sfctemp.
> > 
> > +config SENSORS_SGMCU
> Same question as upon.
> > +	tristate "Sophgo onboard MCU support"
> Same question as upon.
> > +	depends on I2C
> > +	depends on ARCH_SOPHGO || COMPILE_TEST
> > +	help
> > +	  Support for onboard MCU of Sophgo 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 sgmcu.
> > +
> >   config SENSORS_SURFACE_FAN
> >   	tristate "Surface Fan Driver"
> >   	depends on SURFACE_AGGREGATOR
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e3f25475d1f0..e9b78ff8338e 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -195,6 +195,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_SGMCU)	+= sgmcu.o
> Same question as upon.
> >   obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
> >   obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
> >   obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> > diff --git a/drivers/hwmon/sgmcu.c b/drivers/hwmon/sgmcu.c
> > new file mode 100644
> > index 000000000000..d941d6fe741f
> > --- /dev/null
> > +++ b/drivers/hwmon/sgmcu.c
> Same question as upon.
> > @@ -0,0 +1,585 @@
> > +// 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/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.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			0x1
> > +#define CRITICAL_ACTION_POWEROFF		0x2
> > +
> > +#define DEFAULT_REPOWER_TEMP			60
> > +#define MAX_REPOWER_TEMP			100
> > +
> > +#define sg2042_mcu_read_byte(client, reg)			\
> > +	i2c_smbus_read_byte_data(client, reg)
> > +#define sg2042_mcu_write_byte(client, reg, value)		\
> > +	i2c_smbus_write_byte_data(client, reg, value)
> > +#define sg2042_mcu_read_block(client, reg, array)		\
> > +	i2c_smbus_read_i2c_block_data(client, reg, sizeof(array), array)
> > +
> > +#define DEFINE_MCU_ATTR_READ_FUNC(_name, _type, _format)		\
> > +	static ssize_t _name##_show(struct device *dev,			\
> > +				    struct device_attribute *attr,	\
> > +				    char *buf)				\
> > +	{								\
> > +		struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);	\
> > +		_type ret;						\
> > +		ret = sg2042_mcu_get_##_name(mcu->client);		\
> > +		if (ret < 0)						\
> > +			return ret;					\
> > +		return sprintf(buf, _format "\n", ret);			\
> > +	}
> > +
> > +#define DEFINE_MCU_DEBUG_ATTR_READ_FUNC(_name, _type, _format)		\
> > +	static int _name##_show(struct seq_file *seqf,			\
> > +				    void *unused)			\
> > +	{								\
> > +		struct sg2042_mcu_data *mcu = seqf->private;		\
> > +		_type ret;						\
> > +		ret = sg2042_mcu_get_##_name(mcu->client);		\
> > +		if (ret < 0)						\
> > +			return ret;					\
> > +		seq_printf(seqf, _format "\n", ret);			\
> > +		return 0;						\
> > +	}
> > +
> > +#define _CREATE_DEBUG_ENTRY(name, perm, d, data)			\
> > +	debugfs_create_file(#name, perm, d, data, &name##_fops)
> > +
> > +struct sg2042_mcu_board_data {
> > +	u8		id;
> > +	const char	*name;
> > +};
> > +
> > +struct sg2042_mcu_data {
> > +	struct i2c_client			*client;
> > +	const struct sg2042_mcu_board_data	*board_info;
> > +	struct dentry				*debugfs;
> > +};
> > +
> > +static const struct sg2042_mcu_board_data sg2042_boards_data[] = {
> > +	{
> > +		.id = 0x80,
> > +		.name = "SG2042 evb x8",
> > +	},
> > +	{
> > +		.id = 0x81,
> > +		.name = "SG2042R evb",
> > +	},
> > +	{
> > +		.id = 0x83,
> > +		.name = "SG2042 evb x4",
> > +	},
> > +	{
> > +		.id = 0x90,
> > +		.name = "Milk-V Pioneer",
> > +	},
> > +};
> 
> Upstream kernel DTS only supports Milk-V pioneer, evb boards are not
> supported in plan. Can we figure a method to let user extend this outside
> kernel or just ask vendor to patch this when necessary?
> 
> > +
> > +static const char *sg2042_mcu_reset_reason[8] = {
> > +	"Power supply overheat",
> > +	"Power supply failure",
> > +	"12V power supply failure",
> > +	"Reset commant",
> > +	"Unknown",
> > +	"Unknown",
> > +	"Unknown",
> > +	"SoC overheat",
> > +};
> > +
> > +static struct dentry *sgmcu_debugfs;
> > +
> > +static int sg2042_mcu_get_board_type(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_BOARD_TYPE);
> > +}
> > +
> > +static int sg2042_mcu_get_firmware_version(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_MCU_FIRMWARE_VERSION);
> > +}
> > +
> > +static int sg2042_mcu_get_pcb_version(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_PCB_VERSION);
> > +}
> > +
> > +static int sg2042_mcu_get_soc_temp(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_SOC_TEMP);
> > +}
> > +
> > +static int sg2042_mcu_get_board_temp(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_BOARD_TEMP);
> > +}
> > +
> > +static int sg2042_mcu_get_reset_count(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_RST_COUNT);
> > +}
> > +
> > +static s32 sg2042_mcu_get_uptime(struct i2c_client *client)
> > +{
> > +	int ret;
> > +	u8 time_val[2];
> > +
> > +	ret = sg2042_mcu_read_block(client, REG_UPTIME, time_val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return (s32)(time_val[0]) + ((s32)(time_val[1]) << 8);
> > +}
> > +
> > +static int sg2042_mcu_get_reset_reason(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_RESET_REASON);
> > +}
> > +
> > +static int sg2042_mcu_get_mcu_type(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_MCU_TYPE);
> > +}
> > +
> > +static int sg2042_mcu_get_soc_crit_action(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_CRITICAL_ACTIONS);
> > +}
> > +
> > +static int sg2042_mcu_get_soc_crit_temp(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_CRITICAL_TEMP);
> > +}
> > +
> > +static int sg2042_mcu_get_soc_hyst_temp(struct i2c_client *client)
> > +{
> > +	return sg2042_mcu_read_byte(client, REG_REPOWER_TEMP);
> > +}
> > +
> > +static int sg2042_mcu_set_soc_crit_action(struct i2c_client *client,
> > +					  u8 value)
> > +{
> > +	return sg2042_mcu_write_byte(client, REG_CRITICAL_ACTIONS, value);
> > +}
> > +
> > +static int sg2042_mcu_set_soc_crit_temp(struct i2c_client *client,
> > +					u8 value)
> > +{
> > +	return sg2042_mcu_write_byte(client, REG_CRITICAL_TEMP, value);
> > +}
> > +
> > +static int sg2042_mcu_set_soc_hyst_temp(struct i2c_client *client,
> > +					u8 value)
> > +{
> > +	return sg2042_mcu_write_byte(client, REG_REPOWER_TEMP, value);
> > +}
> > +
> > +DEFINE_MCU_ATTR_READ_FUNC(reset_count, int, "%d");
> > +DEFINE_MCU_ATTR_READ_FUNC(uptime, s32, "%d");
> > +
> > +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, val, i;
> > +
> > +	val = sg2042_mcu_get_reset_reason(mcu->client);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	ret = sprintf(buf, "Reason: 0x%02x\n", val);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(sg2042_mcu_reset_reason); i++) {
> > +		if (val & BIT(i))
> > +			ret += sprintf(buf + ret, "bit %d: %s\n", i,
> > +						  sg2042_mcu_reset_reason[i]);
> > +	}
> > +
> > +	return 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 = sg2042_mcu_get_soc_crit_action(mcu->client);
> > +	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);
> > +	int value;
> > +
> > +	if (sysfs_streq("reboot", buf))
> > +		value = CRITICAL_ACTION_REBOOT;
> > +	else if (sysfs_streq("poweroff", buf))
> > +		value = CRITICAL_ACTION_POWEROFF;
> > +	else
> > +		return -EINVAL;
> > +
> > +	return sg2042_mcu_set_soc_crit_action(mcu->client, value);
> > +}
> > +
> > +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_READ_FUNC(firmware_version, int, "0x%02x");
> > +DEFINE_MCU_DEBUG_ATTR_READ_FUNC(pcb_version, int, "0x%02x");
> > +
> > +static int board_type_show(struct seq_file *seqf, void *unused)
> > +{
> > +	struct sg2042_mcu_data *mcu = seqf->private;
> > +
> > +	seq_printf(seqf, "%s\n", mcu->board_info->name ?: "Unknown");
> > +
> > +	return 0;
> > +}
> > +
> > +static int mcu_type_show(struct seq_file *seqf, void *unused)
> > +{
> > +	struct sg2042_mcu_data *mcu = seqf->private;
> > +	int ret;
> > +
> > +	ret = sg2042_mcu_get_mcu_type(mcu->client);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	seq_puts(seqf, ret ? "GD32\n" : "STM32\n");
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(firmware_version);
> > +DEFINE_SHOW_ATTRIBUTE(pcb_version);
> > +DEFINE_SHOW_ATTRIBUTE(mcu_type);
> > +DEFINE_SHOW_ATTRIBUTE(board_type);
> > +
> > +// TODO: to debugfs
> > +
> > +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 hwmon_channel_info * const sg2042_mcu_info[] = {
> > +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
> > +	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);
> > +	long tmp;
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		switch (channel) {
> > +		case 0:
> > +			tmp = sg2042_mcu_get_soc_temp(mcu->client);
> > +			if (tmp < 0)
> > +				return tmp;
> > +			*val = tmp * 1000;
> > +			break;
> > +		case 1:
> > +			tmp = sg2042_mcu_get_board_temp(mcu->client);
> > +			if (tmp < 0)
> > +				return tmp;
> > +			*val = tmp * 1000;
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	case hwmon_temp_crit:
> > +		if (channel)
> > +			return -EOPNOTSUPP;
> > +
> > +		tmp = sg2042_mcu_get_soc_crit_temp(mcu->client);
> > +		if (tmp < 0)
> > +			return tmp;
> > +		*val = tmp * 1000;
> > +		break;
> > +	case hwmon_temp_crit_hyst:
> > +		if (channel)
> > +			return -EOPNOTSUPP;
> > +
> > +		tmp = sg2042_mcu_get_soc_hyst_temp(mcu->client);
> > +		if (tmp < 0)
> > +			return tmp;
> > +		*val = tmp * 1000;
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int sg2042_mcu_read(struct device *dev,
> > +			   enum hwmon_sensor_types type,
> > +			   u32 attr, int channel, long *val)
> > +{
> > +	switch (type) {
> > +	case hwmon_chip:
> > +		if (attr != hwmon_chip_update_interval)
> > +			return -EOPNOTSUPP;
> > +		*val = 1000;
> > +		break;
> > +	case hwmon_temp:
> > +		return sg2042_mcu_read_temp(dev, attr, channel, val);
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +	return 0;
> > +}
> > +
> > +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);
> > +	u8 down_temp, repower_temp;
> > +	int ret;
> > +
> > +	if (type != hwmon_temp || attr != hwmon_temp_crit || !channel)
> > +		return -EOPNOTSUPP;
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_crit:
> > +		ret = sg2042_mcu_get_soc_hyst_temp(mcu->client);
> > +		if (ret < 0)
> > +			repower_temp = DEFAULT_REPOWER_TEMP;
> > +		else
> > +			repower_temp = ret;
> > +
> > +		down_temp = val / 1000;
> > +		if (down_temp < repower_temp)
> > +			return -EINVAL;
> > +
> > +		return sg2042_mcu_set_soc_crit_temp(mcu->client,
> > +						    (u8)(val / 1000));
> > +	case hwmon_temp_crit_hyst:
> > +		ret = sg2042_mcu_get_soc_crit_temp(mcu->client);
> > +		if (ret < 0)
> > +			return -ENODEV;
> > +
> > +		down_temp = ret;
> > +		repower_temp = val / 1000;
> > +		if (down_temp < repower_temp)
> > +			return -EINVAL;
> > +
> > +		return sg2042_mcu_set_soc_hyst_temp(mcu->client,
> > +						    (u8)(val / 1000));
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> > +
> > +static umode_t sg2042_mcu_is_visible(const void *_data,
> > +				     enum hwmon_sensor_types type,
> > +				     u32 attr, int channel)
> > +{
> > +	switch (type) {
> > +	case hwmon_chip:
> > +		if (attr == hwmon_chip_update_interval)
> > +			return 0444;
> > +		break;
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_input:
> > +			if (channel < 2)
> > +				return 0444;
> > +			break;
> > +		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) {
> > +		_CREATE_DEBUG_ENTRY(firmware_version, 0444, mcu->debugfs, mcu);
> > +		_CREATE_DEBUG_ENTRY(pcb_version, 0444, mcu->debugfs, mcu);
> > +		_CREATE_DEBUG_ENTRY(mcu_type, 0444, mcu->debugfs, mcu);
> > +		_CREATE_DEBUG_ENTRY(board_type, 0444, mcu->debugfs, mcu);
> > +	}
> > +}
> > +
> > +static int sg2042_mcu_check_board(u8 id)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(sg2042_boards_data); i++) {
> > +		if (sg2042_boards_data[i].id == id)
> > +			return i;
> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +
> > +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
> > +{
> > +	int ret;
> > +	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;
> > +
> > +	ret = sg2042_mcu_get_board_type(client);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = sg2042_mcu_check_board(ret);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
> > +	if (!mcu)
> > +		return -ENOMEM;
> > +
> > +	mcu->client = client;
> > +	mcu->board_info = &sg2042_boards_data[ret];
> > +
> > +	ret = sysfs_create_group(&dev->kobj, &sg2042_mcu_attr_group);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	i2c_set_clientdata(client, mcu);
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> > +							 mcu,
> > +							 &sg2042_mcu_chip_info,
> > +							 NULL);
> > +
> > +	sg2042_mcu_debugfs_init(mcu, dev);
> > +
> > +	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +
> > +	sysfs_remove_group(&dev->kobj, &sg2042_mcu_attr_group);
> > +}
> > +
> > +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,
> > +	},
> > +	.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("sgmcu", 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
> > 

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

[Chen Wang]

On 2024/7/8 8:53, Inochi Amaoto wrote:
> On Mon, Jul 08, 2024 at 08:25:55AM GMT, Chen Wang wrote:
>> On 2024/7/3 10:30, Inochi Amaoto wrote:
>>> 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/sgmcu.rst |  44 +++
>>>    drivers/hwmon/Kconfig         |  11 +
>>>    drivers/hwmon/Makefile        |   1 +
>>>    drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
>>>    5 files changed, 642 insertions(+)
>>>    create mode 100644 Documentation/hwmon/sgmcu.rst
>>>    create mode 100644 drivers/hwmon/sgmcu.c
>>>
>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>>> index 03d313af469a..189626b3a055 100644
>>> --- a/Documentation/hwmon/index.rst
>>> +++ b/Documentation/hwmon/index.rst
>>> @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
>>>       sch5636
>>>       scpi-hwmon
>>>       sfctemp
>>> +   sgmcu
>> This driver is for sg2042 only, right? "sgmcu" looks be general for all
>> sophgo products.
> Yes, according to sophgo, it use this mechanism for multiple products,
> so I switch to a general name.

But multiple != ALL.

[......]

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

[Inochi Amaoto]

On Mon, Jul 08, 2024 at 03:11:37PM GMT, Chen Wang wrote:
> 
> On 2024/7/8 8:53, Inochi Amaoto wrote:
> > On Mon, Jul 08, 2024 at 08:25:55AM GMT, Chen Wang wrote:
> > > On 2024/7/3 10:30, Inochi Amaoto wrote:
> > > > 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/sgmcu.rst |  44 +++
> > > >    drivers/hwmon/Kconfig         |  11 +
> > > >    drivers/hwmon/Makefile        |   1 +
> > > >    drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
> > > >    5 files changed, 642 insertions(+)
> > > >    create mode 100644 Documentation/hwmon/sgmcu.rst
> > > >    create mode 100644 drivers/hwmon/sgmcu.c
> > > > 
> > > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > > > index 03d313af469a..189626b3a055 100644
> > > > --- a/Documentation/hwmon/index.rst
> > > > +++ b/Documentation/hwmon/index.rst
> > > > @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
> > > >       sch5636
> > > >       scpi-hwmon
> > > >       sfctemp
> > > > +   sgmcu
> > > This driver is for sg2042 only, right? "sgmcu" looks be general for all
> > > sophgo products.
> > Yes, according to sophgo, it use this mechanism for multiple products,
> > so I switch to a general name.
> 
> But multiple != ALL.
> 
> [......]
> 
> 

We can add new driver when there is new mechanism.

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

[Guenter Roeck]

On 7/8/24 15:15, Inochi Amaoto wrote:
> On Mon, Jul 08, 2024 at 03:11:37PM GMT, Chen Wang wrote:
>>
>> On 2024/7/8 8:53, Inochi Amaoto wrote:
>>> On Mon, Jul 08, 2024 at 08:25:55AM GMT, Chen Wang wrote:
>>>> On 2024/7/3 10:30, Inochi Amaoto wrote:
>>>>> 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/sgmcu.rst |  44 +++
>>>>>     drivers/hwmon/Kconfig         |  11 +
>>>>>     drivers/hwmon/Makefile        |   1 +
>>>>>     drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
>>>>>     5 files changed, 642 insertions(+)
>>>>>     create mode 100644 Documentation/hwmon/sgmcu.rst
>>>>>     create mode 100644 drivers/hwmon/sgmcu.c
>>>>>
>>>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>>>>> index 03d313af469a..189626b3a055 100644
>>>>> --- a/Documentation/hwmon/index.rst
>>>>> +++ b/Documentation/hwmon/index.rst
>>>>> @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
>>>>>        sch5636
>>>>>        scpi-hwmon
>>>>>        sfctemp
>>>>> +   sgmcu
>>>> This driver is for sg2042 only, right? "sgmcu" looks be general for all
>>>> sophgo products.
>>> Yes, according to sophgo, it use this mechanism for multiple products,
>>> so I switch to a general name.
>>
>> But multiple != ALL.
>>
>> [......]
>>
>>
> 
> We can add new driver when there is new mechanism.

Now you are contradicting yourself. Either sgmcu is the catch-all
driver, or it isn't. How are you going to call that new driver ? sgmcuv2 ?
Are we going to have sgmcuv[2-N] over time ?

All we know so far is that the driver and the mcu support sg2042. That is how the
driver should be named. It is easier to add support a new device with a different
name to the existing driver than to add a new driver if the name of an existing driver
is too generic.

Ultimately this is similar to wildcards in a file name, which are strongly discouraged.
One of the worst examples is drivers/hwmon/ina2xx.c, which does _not_ support all chips
from ina200 to ina299. Please don't let us go there.

An opposite example is the lm90 driver, which has not problem supporting more than 40
different chips with different names because they are all similar. The driver can be named
sg2042 and support as many similar variants if that mcu as feasible. It should not be named
sgmcu because we can not make the assumption that it will support all mcu variants from
sophgo.

Guenter

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

[Inochi Amaoto]

On Mon, Jul 08, 2024 at 06:42:14PM GMT, Guenter Roeck wrote:
> On 7/8/24 15:15, Inochi Amaoto wrote:
> > On Mon, Jul 08, 2024 at 03:11:37PM GMT, Chen Wang wrote:
> > > 
> > > On 2024/7/8 8:53, Inochi Amaoto wrote:
> > > > On Mon, Jul 08, 2024 at 08:25:55AM GMT, Chen Wang wrote:
> > > > > On 2024/7/3 10:30, Inochi Amaoto wrote:
> > > > > > 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/sgmcu.rst |  44 +++
> > > > > >     drivers/hwmon/Kconfig         |  11 +
> > > > > >     drivers/hwmon/Makefile        |   1 +
> > > > > >     drivers/hwmon/sgmcu.c         | 585 ++++++++++++++++++++++++++++++++++
> > > > > >     5 files changed, 642 insertions(+)
> > > > > >     create mode 100644 Documentation/hwmon/sgmcu.rst
> > > > > >     create mode 100644 drivers/hwmon/sgmcu.c
> > > > > > 
> > > > > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > > > > > index 03d313af469a..189626b3a055 100644
> > > > > > --- a/Documentation/hwmon/index.rst
> > > > > > +++ b/Documentation/hwmon/index.rst
> > > > > > @@ -203,6 +203,7 @@ Hardware Monitoring Kernel Drivers
> > > > > >        sch5636
> > > > > >        scpi-hwmon
> > > > > >        sfctemp
> > > > > > +   sgmcu
> > > > > This driver is for sg2042 only, right? "sgmcu" looks be general for all
> > > > > sophgo products.
> > > > Yes, according to sophgo, it use this mechanism for multiple products,
> > > > so I switch to a general name.
> > > 
> > > But multiple != ALL.
> > > 
> > > [......]
> > > 
> > > 
> > 
> > We can add new driver when there is new mechanism.
> 
> Now you are contradicting yourself. Either sgmcu is the catch-all
> driver, or it isn't. How are you going to call that new driver ? sgmcuv2 ?
> Are we going to have sgmcuv[2-N] over time ?
> 

No, I mean new mechanism that does not use MCU. For example, the chip 
cv1800b and the incoming sg2380, these chip have different mechanism. 
For all chip with the MCU, I want to keep only one driver.

> All we know so far is that the driver and the mcu support sg2042. That is how the
> driver should be named. It is easier to add support a new device with a different
> name to the existing driver than to add a new driver if the name of an existing driver
> is too generic.
> 
> Ultimately this is similar to wildcards in a file name, which are strongly discouraged.
> One of the worst examples is drivers/hwmon/ina2xx.c, which does _not_ support all chips
> from ina200 to ina299. Please don't let us go there.
> 

> An opposite example is the lm90 driver, which has not problem supporting more than 40
> different chips with different names because they are all similar. The driver can be named
> sg2042 and support as many similar variants if that mcu as feasible. It should not be named
> sgmcu because we can not make the assumption that it will support all mcu variants from
> sophgo.

Thanks, this is hole I don't consider. I have made an assumption that 
this driver only serves for sophgo products (especially products with
this MCU mechanism), which is too limited. Now let me restrict it as
sg2042 specific and evolve it in the future.

Regards,
Inochi

> 
> Guenter
>