[v9,2/4] drivers: hwmon: sophgo: Add SG2042 external hardware monitor support

Commit Message

Inochi Amaoto Aug. 2, 2024, 12:38 p.m. UTC

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 <[e-mail]>
Tested-by: Chen Wang <[e-mail]>
Reviewed-by: Chen Wang <[e-mail]>
---
 Documentation/hwmon/index.rst      |   1 +
 Documentation/hwmon/sg2042-mcu.rst |  39 +++
 drivers/hwmon/Kconfig              |  11 +
 drivers/hwmon/Makefile             |   1 +
 drivers/hwmon/sg2042-mcu.c         | 394 +++++++++++++++++++++++++++++
 5 files changed, 446 insertions(+)
 create mode 100644 Documentation/hwmon/sg2042-mcu.rst
 create mode 100644 drivers/hwmon/sg2042-mcu.c

Comments

Guenter Roeck Aug. 2, 2024, 1:59 p.m. UTC | #1

On 8/2/24 05:38, 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 <[e-mail]>
> Tested-by: Chen Wang <[e-mail]>
> Reviewed-by: Chen Wang <[e-mail]>
> ---
>   Documentation/hwmon/index.rst      |   1 +
>   Documentation/hwmon/sg2042-mcu.rst |  39 +++
>   drivers/hwmon/Kconfig              |  11 +
>   drivers/hwmon/Makefile             |   1 +
>   drivers/hwmon/sg2042-mcu.c         | 394 +++++++++++++++++++++++++++++
>   5 files changed, 446 insertions(+)
>   create mode 100644 Documentation/hwmon/sg2042-mcu.rst
>   create mode 100644 drivers/hwmon/sg2042-mcu.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 913c11390a45..ea3b5be8fe4f 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -206,6 +206,7 @@ Hardware Monitoring Kernel Drivers
>      sch5636
>      scpi-hwmon
>      sfctemp
> +   sg2042-mcu
>      sht15
>      sht21
>      sht3x
> diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
> new file mode 100644
> index 000000000000..250016b47dd1
> --- /dev/null
> +++ b/Documentation/hwmon/sg2042-mcu.rst
> @@ -0,0 +1,39 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver sg2042-mcu
> +=====================
> +
> +Supported chips:
> +
> +  * Onboard MCU for sg2042
> +
> +    Addresses scanned: -
> +
> +    Prefix: 'sg2042-mcu'
> +
> +Authors:
> +
> +  - Inochi Amaoto <[e-mail]>
> +
> +Description
> +-----------
> +
> +This driver supprts hardware monitoring for onboard MCU with
> +i2c interface.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate
> +the devices explicitly.
> +Please see Documentation/i2c/instantiating-devices.rst for details.
> +
> +Sysfs Attributes
> +----------------
> +
> +================= =============================================
> +temp1_input       Measured temperature of SoC
> +temp1_crit        Critical high temperature
> +temp1_crit_hyst   hysteresis temperature restore from Critical
> +temp2_input       Measured temperature of the base board
> +================= =============================================
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index b60fe2e58ad6..7aa6c3f322e5 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2066,6 +2066,17 @@ config SENSORS_SFCTEMP
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called sfctemp.
>   
> +config SENSORS_SG2042_MCU
> +	tristate "Sophgo onboard MCU support"
> +	depends on I2C
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	help
> +	  Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
> +	  power control and some basic information.
> +
> +	  This driver can be built as a module. If so, the module
> +	  will be called sg2042-mcu.
> +
>   config SENSORS_SURFACE_FAN
>   	tristate "Surface Fan Driver"
>   	depends on SURFACE_AGGREGATOR
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b1c7056c37db..0bbe812a67ae 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>   obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
>   obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
>   obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
> +obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
>   obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
>   obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>   obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
> new file mode 100644
> index 000000000000..6d8d677f86f3
> --- /dev/null
> +++ b/drivers/hwmon/sg2042-mcu.c
> @@ -0,0 +1,394 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Inochi Amaoto <[e-mail]>
> + *
> + * Sophgo power control mcu for SG2042
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/debugfs.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +
> +/* fixed MCU registers */
> +#define REG_BOARD_TYPE				0x00
> +#define REG_MCU_FIRMWARE_VERSION		0x01
> +#define REG_PCB_VERSION				0x02
> +#define REG_PWR_CTRL				0x03
> +#define REG_SOC_TEMP				0x04
> +#define REG_BOARD_TEMP				0x05
> +#define REG_RST_COUNT				0x0a
> +#define REG_UPTIME				0x0b
> +#define REG_RESET_REASON			0x0d
> +#define REG_MCU_TYPE				0x18
> +#define REG_REPOWER_ACTION			0x65
> +#define REG_CRITICAL_TEMP			0x66
> +#define REG_REPOWER_TEMP			0x67
> +
> +#define REPOWER_ACTION_REBOOT			1
> +#define REPOWER_ACTION_POWEROFF			2
> +
> +#define MCU_POWER_MAX				0xff
> +
> +#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format)			\
> +	static int _name##_show(struct seq_file *seqf,			\
> +				    void *unused)			\
> +	{								\
> +		struct sg2042_mcu_data *mcu = seqf->private;		\
> +		int ret;						\
> +		ret = i2c_smbus_read_byte_data(mcu->client, (_reg));	\
> +		if (ret < 0)						\
> +			return ret;					\
> +		seq_printf(seqf, _format "\n", ret);			\
> +		return 0;						\
> +	}								\
> +	DEFINE_SHOW_ATTRIBUTE(_name)					\
> +
> +struct sg2042_mcu_data {
> +	struct i2c_client	*client;
> +	struct dentry		*debugfs;
> +	struct mutex		mutex;
> +};
> +
> +static struct dentry *sgmcu_debugfs;
> +
> +static ssize_t reset_count_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t uptime_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	u8 time_val[2];
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
> +					    sizeof(time_val), time_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n",
> +		       (int)(time_val[0]) + (int)(time_val[1] << 8));

Pointless type casts

> +}
> +
> +static ssize_t reset_reason_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "0x%02x\n", ret);
> +}
> +
> +static ssize_t repower_action_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int ret;
> +	const char *action;
> +
> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_REPOWER_ACTION);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret == REPOWER_ACTION_REBOOT)
> +		action = "reboot";
> +	else if (ret == REPOWER_ACTION_POWEROFF)
> +		action = "poweroff";
> +	else
> +		action = "unknown";
> +
> +	return sprintf(buf, "%s\n", action);
> +}
> +
> +static ssize_t repower_action_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	u8 value;
> +	int ret;
> +
> +	if (sysfs_streq("reboot", buf))
> +		value = REPOWER_ACTION_REBOOT;
> +	else if (sysfs_streq("poweroff", buf))
> +		value = REPOWER_ACTION_POWEROFF;
> +	else
> +		return -EINVAL;
> +
> +	guard(mutex)(&mcu->mutex);
> +
Pointless mutex protection.

> +	ret = i2c_smbus_write_byte_data(mcu->client,
> +					REG_REPOWER_ACTION, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RO(reset_count);
> +static DEVICE_ATTR_RO(uptime);
> +static DEVICE_ATTR_RO(reset_reason);
> +static DEVICE_ATTR_RW(repower_action);
> +
> +DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
> +DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
> +DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
> +DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
> +
> +static struct attribute *sg2042_mcu_attrs[] = {
> +	&dev_attr_reset_count.attr,
> +	&dev_attr_uptime.attr,
> +	&dev_attr_reset_reason.attr,
> +	&dev_attr_repower_action.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group sg2042_mcu_attr_group = {
> +	.attrs	= sg2042_mcu_attrs,
> +};
> +
> +static const struct attribute_group *sg2042_mcu_groups[] = {
> +	&sg2042_mcu_attr_group,
> +	NULL
> +};
> +
> +static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
> +					HWMON_T_CRIT_HYST,
> +				 HWMON_T_INPUT),
> +	NULL
> +};
> +
> +static int sg2042_mcu_read(struct device *dev,
> +			   enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int tmp;
> +	u8 reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> +		break;
> +	case hwmon_temp_crit:
> +		reg = REG_CRITICAL_TEMP;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		reg = REG_REPOWER_TEMP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> +	if (tmp < 0)
> +		return tmp;
> +	*val = tmp * 1000;
> +
> +	return 0;
> +}
> +
> +static int sg2042_mcu_write(struct device *dev,
> +			    enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long val)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int temp = val / 1000;
> +	int hyst_temp, crit_temp;
> +	u8 reg;
> +
> +	if (temp > MCU_POWER_MAX)
> +		temp = MCU_POWER_MAX;
> +	else if (temp < 0)
> +		temp = 0;
> +
	temp = clamp_val(temp, 0, MCU_POWER_MAX);

> +	guard(mutex)(&mcu->mutex);
> +
> +	switch (attr) {
> +	case hwmon_temp_crit:
> +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> +						     REG_REPOWER_TEMP);
> +		if (hyst_temp < 0)
> +			return hyst_temp;
> +
> +		crit_temp = temp;
> +		reg = REG_CRITICAL_TEMP;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> +						     REG_CRITICAL_TEMP);
> +		if (crit_temp < 0)
> +			return crit_temp;
> +
> +		hyst_temp = temp;
> +		reg = REG_REPOWER_TEMP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/*
> +	 * ensure hyst_temp is smaller to avoid MCU from
> +	 * keeping triggering repower event.
> +	 */
> +	if (crit_temp < hyst_temp)
> +		return -EINVAL;
> +
> +	return i2c_smbus_write_byte_data(mcu->client, reg, temp);
> +}
> +
> +static umode_t sg2042_mcu_is_visible(const void *_data,
> +				     enum hwmon_sensor_types type,
> +				     u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			return 0444;
> +		case hwmon_temp_crit:
> +		case hwmon_temp_crit_hyst:
> +			if (channel == 0)
> +				return 0664;
> +			break;

In practice this is the only break; in this nested switch sequence,
making the return 0; at the end necessary.

Please either use break; always when returning 0, or return 0 here
as well and drop the final return 0;

> +		default:
> +			return 0;
> +		}
> +		break;
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_ops sg2042_mcu_ops = {
> +	.is_visible = sg2042_mcu_is_visible,
> +	.read = sg2042_mcu_read,
> +	.write = sg2042_mcu_write,
> +};
> +
> +static const struct hwmon_chip_info sg2042_mcu_chip_info = {
> +	.ops = &sg2042_mcu_ops,
> +	.info = sg2042_mcu_info,
> +};
> +
> +static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
> +				    struct device *dev)
> +{
> +	mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
> +	if (mcu->debugfs) {

debugfs_create_dir() returns an ERR_PTR(), which is checked in debugfs_create_file().
This if() is pointless.

> +		debugfs_create_file("firmware_version", 0444, mcu->debugfs,
> +				    mcu, &firmware_version_fops);
> +		debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
> +				    &pcb_version_fops);
> +		debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
> +				    &mcu_type_fops);
> +		debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
> +				    &board_type_fops);
> +	}
> +}
> +
> +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct sg2042_mcu_data *mcu;
> +	struct device *hwmon_dev;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +						I2C_FUNC_SMBUS_BLOCK_DATA))
> +		return -EIO;
> +
		return -ENODEV;

> +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
> +	if (!mcu)
> +		return -ENOMEM;
> +
> +	mutex_init(&mcu->mutex);
> +	mcu->client = client;
> +
> +	i2c_set_clientdata(client, mcu);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
> +							 mcu,
> +							 &sg2042_mcu_chip_info,
> +							 NULL);
> +	if (IS_ERR_OR_NULL(hwmon_dev))
> +		return -EFAULT;

devm_hwmon_device_register_with_info() returns an ERR_PTR(), and
returning -EFAULT instead of it is more than wrong (this does not indicate
a "bad address"). This needs to be

	if (IS_ERR(hwmon_dev))
		return PTR_ERR(hwmon_dev);

I am sure I asked before not to overwrite error codes.

> +
> +	sg2042_mcu_debugfs_init(mcu, dev);
> +
> +	return 0;
> +}
> +
> +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
> +{
> +	struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
> +
> +	debugfs_remove_recursive(mcu->debugfs);
> +}
> +
> +static const struct i2c_device_id sg2042_mcu_id[] = {
> +	{ "sg2042-hwmon-mcu", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
> +
> +static const struct of_device_id sg2042_mcu_of_id[] = {
> +	{ .compatible = "sophgo,sg2042-hwmon-mcu" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
> +
> +static struct i2c_driver sg2042_mcu_driver = {
> +	.driver = {
> +		.name = "sg2042-mcu",
> +		.of_match_table = sg2042_mcu_of_id,
> +		.dev_groups = sg2042_mcu_groups,
> +	},
> +	.probe = sg2042_mcu_i2c_probe,
> +	.remove = sg2042_mcu_i2c_remove,
> +	.id_table = sg2042_mcu_id,
> +};
> +
> +static int __init sg2042_mcu_init(void)
> +{
> +	sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
> +	return i2c_add_driver(&sg2042_mcu_driver);
> +}
> +
> +static void __exit sg2042_mcu_exit(void)
> +{
> +	debugfs_remove_recursive(sgmcu_debugfs);
> +	i2c_del_driver(&sg2042_mcu_driver);
> +}
> +
> +module_init(sg2042_mcu_init);
> +module_exit(sg2042_mcu_exit);
> +
> +MODULE_AUTHOR("Inochi Amaoto <[e-mail]>");
> +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
> +MODULE_LICENSE("GPL");

Inochi Amaoto Aug. 2, 2024, 11:28 p.m. UTC | #2

On Fri, Aug 02, 2024 at 06:59:22AM GMT, Guenter Roeck wrote:
> On 8/2/24 05:38, 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 <[e-mail]>
> > Tested-by: Chen Wang <[e-mail]>
> > Reviewed-by: Chen Wang <[e-mail]>
> > ---
> >   Documentation/hwmon/index.rst      |   1 +
> >   Documentation/hwmon/sg2042-mcu.rst |  39 +++
> >   drivers/hwmon/Kconfig              |  11 +
> >   drivers/hwmon/Makefile             |   1 +
> >   drivers/hwmon/sg2042-mcu.c         | 394 +++++++++++++++++++++++++++++
> >   5 files changed, 446 insertions(+)
> >   create mode 100644 Documentation/hwmon/sg2042-mcu.rst
> >   create mode 100644 drivers/hwmon/sg2042-mcu.c
> > 
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index 913c11390a45..ea3b5be8fe4f 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -206,6 +206,7 @@ Hardware Monitoring Kernel Drivers
> >      sch5636
> >      scpi-hwmon
> >      sfctemp
> > +   sg2042-mcu
> >      sht15
> >      sht21
> >      sht3x
> > diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
> > new file mode 100644
> > index 000000000000..250016b47dd1
> > --- /dev/null
> > +++ b/Documentation/hwmon/sg2042-mcu.rst
> > @@ -0,0 +1,39 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver sg2042-mcu
> > +=====================
> > +
> > +Supported chips:
> > +
> > +  * Onboard MCU for sg2042
> > +
> > +    Addresses scanned: -
> > +
> > +    Prefix: 'sg2042-mcu'
> > +
> > +Authors:
> > +
> > +  - Inochi Amaoto <[e-mail]>
> > +
> > +Description
> > +-----------
> > +
> > +This driver supprts hardware monitoring for onboard MCU with
> > +i2c interface.
> > +
> > +Usage Notes
> > +-----------
> > +
> > +This driver does not auto-detect devices. You will have to instantiate
> > +the devices explicitly.
> > +Please see Documentation/i2c/instantiating-devices.rst for details.
> > +
> > +Sysfs Attributes
> > +----------------
> > +
> > +================= =============================================
> > +temp1_input       Measured temperature of SoC
> > +temp1_crit        Critical high temperature
> > +temp1_crit_hyst   hysteresis temperature restore from Critical
> > +temp2_input       Measured temperature of the base board
> > +================= =============================================
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index b60fe2e58ad6..7aa6c3f322e5 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -2066,6 +2066,17 @@ config SENSORS_SFCTEMP
> >   	  This driver can also be built as a module.  If so, the module
> >   	  will be called sfctemp.
> > +config SENSORS_SG2042_MCU
> > +	tristate "Sophgo onboard MCU support"
> > +	depends on I2C
> > +	depends on ARCH_SOPHGO || COMPILE_TEST
> > +	help
> > +	  Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
> > +	  power control and some basic information.
> > +
> > +	  This driver can be built as a module. If so, the module
> > +	  will be called sg2042-mcu.
> > +
> >   config SENSORS_SURFACE_FAN
> >   	tristate "Surface Fan Driver"
> >   	depends on SURFACE_AGGREGATOR
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index b1c7056c37db..0bbe812a67ae 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> >   obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
> >   obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
> >   obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
> > +obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
> >   obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
> >   obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
> >   obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> > diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
> > new file mode 100644
> > index 000000000000..6d8d677f86f3
> > --- /dev/null
> > +++ b/drivers/hwmon/sg2042-mcu.c
> > @@ -0,0 +1,394 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2024 Inochi Amaoto <[e-mail]>
> > + *
> > + * Sophgo power control mcu for SG2042
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +
> > +/* fixed MCU registers */
> > +#define REG_BOARD_TYPE				0x00
> > +#define REG_MCU_FIRMWARE_VERSION		0x01
> > +#define REG_PCB_VERSION				0x02
> > +#define REG_PWR_CTRL				0x03
> > +#define REG_SOC_TEMP				0x04
> > +#define REG_BOARD_TEMP				0x05
> > +#define REG_RST_COUNT				0x0a
> > +#define REG_UPTIME				0x0b
> > +#define REG_RESET_REASON			0x0d
> > +#define REG_MCU_TYPE				0x18
> > +#define REG_REPOWER_ACTION			0x65
> > +#define REG_CRITICAL_TEMP			0x66
> > +#define REG_REPOWER_TEMP			0x67
> > +
> > +#define REPOWER_ACTION_REBOOT			1
> > +#define REPOWER_ACTION_POWEROFF			2
> > +
> > +#define MCU_POWER_MAX				0xff
> > +
> > +#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format)			\
> > +	static int _name##_show(struct seq_file *seqf,			\
> > +				    void *unused)			\
> > +	{								\
> > +		struct sg2042_mcu_data *mcu = seqf->private;		\
> > +		int ret;						\
> > +		ret = i2c_smbus_read_byte_data(mcu->client, (_reg));	\
> > +		if (ret < 0)						\
> > +			return ret;					\
> > +		seq_printf(seqf, _format "\n", ret);			\
> > +		return 0;						\
> > +	}								\
> > +	DEFINE_SHOW_ATTRIBUTE(_name)					\
> > +
> > +struct sg2042_mcu_data {
> > +	struct i2c_client	*client;
> > +	struct dentry		*debugfs;
> > +	struct mutex		mutex;
> > +};
> > +
> > +static struct dentry *sgmcu_debugfs;
> > +
> > +static ssize_t reset_count_show(struct device *dev,
> > +				struct device_attribute *attr,
> > +				char *buf)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return sprintf(buf, "%d\n", ret);
> > +}
> > +
> > +static ssize_t uptime_show(struct device *dev,
> > +			   struct device_attribute *attr,
> > +			   char *buf)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	u8 time_val[2];
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
> > +					    sizeof(time_val), time_val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return sprintf(buf, "%d\n",
> > +		       (int)(time_val[0]) + (int)(time_val[1] << 8));
> 
> Pointless type casts
> 
> > +}
> > +
> > +static ssize_t reset_reason_show(struct device *dev,
> > +				 struct device_attribute *attr,
> > +				 char *buf)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return sprintf(buf, "0x%02x\n", ret);
> > +}
> > +
> > +static ssize_t repower_action_show(struct device *dev,
> > +				   struct device_attribute *attr,
> > +				   char *buf)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	int ret;
> > +	const char *action;
> > +
> > +	ret = i2c_smbus_read_byte_data(mcu->client, REG_REPOWER_ACTION);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (ret == REPOWER_ACTION_REBOOT)
> > +		action = "reboot";
> > +	else if (ret == REPOWER_ACTION_POWEROFF)
> > +		action = "poweroff";
> > +	else
> > +		action = "unknown";
> > +
> > +	return sprintf(buf, "%s\n", action);
> > +}
> > +
> > +static ssize_t repower_action_store(struct device *dev,
> > +				    struct device_attribute *attr,
> > +				    const char *buf, size_t count)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	u8 value;
> > +	int ret;
> > +
> > +	if (sysfs_streq("reboot", buf))
> > +		value = REPOWER_ACTION_REBOOT;
> > +	else if (sysfs_streq("poweroff", buf))
> > +		value = REPOWER_ACTION_POWEROFF;
> > +	else
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&mcu->mutex);
> > +
> Pointless mutex protection.
> 

OK, I have seen the protection in the i2c bus. I will remove this.

> > +	ret = i2c_smbus_write_byte_data(mcu->client,
> > +					REG_REPOWER_ACTION, value);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return count;
> > +}
> > +
> > +static DEVICE_ATTR_RO(reset_count);
> > +static DEVICE_ATTR_RO(uptime);
> > +static DEVICE_ATTR_RO(reset_reason);
> > +static DEVICE_ATTR_RW(repower_action);
> > +
> > +DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
> > +DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
> > +DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
> > +DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
> > +
> > +static struct attribute *sg2042_mcu_attrs[] = {
> > +	&dev_attr_reset_count.attr,
> > +	&dev_attr_uptime.attr,
> > +	&dev_attr_reset_reason.attr,
> > +	&dev_attr_repower_action.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group sg2042_mcu_attr_group = {
> > +	.attrs	= sg2042_mcu_attrs,
> > +};
> > +
> > +static const struct attribute_group *sg2042_mcu_groups[] = {
> > +	&sg2042_mcu_attr_group,
> > +	NULL
> > +};
> > +
> > +static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
> > +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
> > +					HWMON_T_CRIT_HYST,
> > +				 HWMON_T_INPUT),
> > +	NULL
> > +};
> > +
> > +static int sg2042_mcu_read(struct device *dev,
> > +			   enum hwmon_sensor_types type,
> > +			   u32 attr, int channel, long *val)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	int tmp;
> > +	u8 reg;
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> > +		break;
> > +	case hwmon_temp_crit:
> > +		reg = REG_CRITICAL_TEMP;
> > +		break;
> > +	case hwmon_temp_crit_hyst:
> > +		reg = REG_REPOWER_TEMP;
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> > +	if (tmp < 0)
> > +		return tmp;
> > +	*val = tmp * 1000;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sg2042_mcu_write(struct device *dev,
> > +			    enum hwmon_sensor_types type,
> > +			    u32 attr, int channel, long val)
> > +{
> > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > +	int temp = val / 1000;
> > +	int hyst_temp, crit_temp;
> > +	u8 reg;
> > +
> > +	if (temp > MCU_POWER_MAX)
> > +		temp = MCU_POWER_MAX;
> > +	else if (temp < 0)
> > +		temp = 0;
> > +
> 	temp = clamp_val(temp, 0, MCU_POWER_MAX);
> 
> > +	guard(mutex)(&mcu->mutex);
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_crit:
> > +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> > +						     REG_REPOWER_TEMP);
> > +		if (hyst_temp < 0)
> > +			return hyst_temp;
> > +
> > +		crit_temp = temp;
> > +		reg = REG_CRITICAL_TEMP;
> > +		break;
> > +	case hwmon_temp_crit_hyst:
> > +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> > +						     REG_CRITICAL_TEMP);
> > +		if (crit_temp < 0)
> > +			return crit_temp;
> > +
> > +		hyst_temp = temp;
> > +		reg = REG_REPOWER_TEMP;
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	/*
> > +	 * ensure hyst_temp is smaller to avoid MCU from
> > +	 * keeping triggering repower event.
> > +	 */
> > +	if (crit_temp < hyst_temp)
> > +		return -EINVAL;
> > +
> > +	return i2c_smbus_write_byte_data(mcu->client, reg, temp);
> > +}
> > +
> > +static umode_t sg2042_mcu_is_visible(const void *_data,
> > +				     enum hwmon_sensor_types type,
> > +				     u32 attr, int channel)
> > +{
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_input:
> > +			return 0444;
> > +		case hwmon_temp_crit:
> > +		case hwmon_temp_crit_hyst:
> > +			if (channel == 0)
> > +				return 0664;
> > +			break;
> 
> In practice this is the only break; in this nested switch sequence,
> making the return 0; at the end necessary.
> 
> Please either use break; always when returning 0, or return 0 here
> as well and drop the final return 0;
> 

Thanks, but I have a question: as this break can be fallthrough to the
default, is it better to use break;/return 0;, or just remove this
break; and let it fallthrough?

> > +		default:
> > +			return 0;
> > +		}
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static const struct hwmon_ops sg2042_mcu_ops = {
> > +	.is_visible = sg2042_mcu_is_visible,
> > +	.read = sg2042_mcu_read,
> > +	.write = sg2042_mcu_write,
> > +};
> > +
> > +static const struct hwmon_chip_info sg2042_mcu_chip_info = {
> > +	.ops = &sg2042_mcu_ops,
> > +	.info = sg2042_mcu_info,
> > +};
> > +
> > +static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
> > +				    struct device *dev)
> > +{
> > +	mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
> > +	if (mcu->debugfs) {
> 
> debugfs_create_dir() returns an ERR_PTR(), which is checked in debugfs_create_file().
> This if() is pointless.
> 

Thanks.

> > +		debugfs_create_file("firmware_version", 0444, mcu->debugfs,
> > +				    mcu, &firmware_version_fops);
> > +		debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
> > +				    &pcb_version_fops);
> > +		debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
> > +				    &mcu_type_fops);
> > +		debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
> > +				    &board_type_fops);
> > +	}
> > +}
> > +
> > +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct sg2042_mcu_data *mcu;
> > +	struct device *hwmon_dev;
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> > +						I2C_FUNC_SMBUS_BLOCK_DATA))
> > +		return -EIO;
> > +
> 		return -ENODEV;
> 
> > +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
> > +	if (!mcu)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&mcu->mutex);
> > +	mcu->client = client;
> > +
> > +	i2c_set_clientdata(client, mcu);
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
> > +							 mcu,
> > +							 &sg2042_mcu_chip_info,
> > +							 NULL);
> > +	if (IS_ERR_OR_NULL(hwmon_dev))
> > +		return -EFAULT;
> 
> devm_hwmon_device_register_with_info() returns an ERR_PTR(), and
> returning -EFAULT instead of it is more than wrong (this does not indicate
> a "bad address"). This needs to be
> 
> 	if (IS_ERR(hwmon_dev))
> 		return PTR_ERR(hwmon_dev);
> 
> I am sure I asked before not to overwrite error codes.

Yes, you have asked, it seems that I have missed this code when I
check the driver. I apology for this mistake and thanks for your
reminder.

> 
> > +
> > +	sg2042_mcu_debugfs_init(mcu, dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
> > +{
> > +	struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
> > +
> > +	debugfs_remove_recursive(mcu->debugfs);
> > +}
> > +
> > +static const struct i2c_device_id sg2042_mcu_id[] = {
> > +	{ "sg2042-hwmon-mcu", 0 },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
> > +
> > +static const struct of_device_id sg2042_mcu_of_id[] = {
> > +	{ .compatible = "sophgo,sg2042-hwmon-mcu" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
> > +
> > +static struct i2c_driver sg2042_mcu_driver = {
> > +	.driver = {
> > +		.name = "sg2042-mcu",
> > +		.of_match_table = sg2042_mcu_of_id,
> > +		.dev_groups = sg2042_mcu_groups,
> > +	},
> > +	.probe = sg2042_mcu_i2c_probe,
> > +	.remove = sg2042_mcu_i2c_remove,
> > +	.id_table = sg2042_mcu_id,
> > +};
> > +
> > +static int __init sg2042_mcu_init(void)
> > +{
> > +	sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
> > +	return i2c_add_driver(&sg2042_mcu_driver);
> > +}
> > +
> > +static void __exit sg2042_mcu_exit(void)
> > +{
> > +	debugfs_remove_recursive(sgmcu_debugfs);
> > +	i2c_del_driver(&sg2042_mcu_driver);
> > +}
> > +
> > +module_init(sg2042_mcu_init);
> > +module_exit(sg2042_mcu_exit);
> > +
> > +MODULE_AUTHOR("Inochi Amaoto <[e-mail]>");
> > +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
> > +MODULE_LICENSE("GPL");
>

Guenter Roeck Aug. 3, 2024, 3:47 a.m. UTC | #3

On 8/2/24 16:28, Inochi Amaoto wrote:
> On Fri, Aug 02, 2024 at 06:59:22AM GMT, Guenter Roeck wrote:
>> On 8/2/24 05:38, 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 <[e-mail]>
>>> Tested-by: Chen Wang <[e-mail]>
>>> Reviewed-by: Chen Wang <[e-mail]>
>>> ---
>>>    Documentation/hwmon/index.rst      |   1 +
>>>    Documentation/hwmon/sg2042-mcu.rst |  39 +++
>>>    drivers/hwmon/Kconfig              |  11 +
>>>    drivers/hwmon/Makefile             |   1 +
>>>    drivers/hwmon/sg2042-mcu.c         | 394 +++++++++++++++++++++++++++++
>>>    5 files changed, 446 insertions(+)
>>>    create mode 100644 Documentation/hwmon/sg2042-mcu.rst
>>>    create mode 100644 drivers/hwmon/sg2042-mcu.c
>>>
>>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>>> index 913c11390a45..ea3b5be8fe4f 100644
>>> --- a/Documentation/hwmon/index.rst
>>> +++ b/Documentation/hwmon/index.rst
>>> @@ -206,6 +206,7 @@ Hardware Monitoring Kernel Drivers
>>>       sch5636
>>>       scpi-hwmon
>>>       sfctemp
>>> +   sg2042-mcu
>>>       sht15
>>>       sht21
>>>       sht3x
>>> diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
>>> new file mode 100644
>>> index 000000000000..250016b47dd1
>>> --- /dev/null
>>> +++ b/Documentation/hwmon/sg2042-mcu.rst
>>> @@ -0,0 +1,39 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
>>> +Kernel driver sg2042-mcu
>>> +=====================
>>> +
>>> +Supported chips:
>>> +
>>> +  * Onboard MCU for sg2042
>>> +
>>> +    Addresses scanned: -
>>> +
>>> +    Prefix: 'sg2042-mcu'
>>> +
>>> +Authors:
>>> +
>>> +  - Inochi Amaoto <[e-mail]>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +This driver supprts hardware monitoring for onboard MCU with
>>> +i2c interface.
>>> +
>>> +Usage Notes
>>> +-----------
>>> +
>>> +This driver does not auto-detect devices. You will have to instantiate
>>> +the devices explicitly.
>>> +Please see Documentation/i2c/instantiating-devices.rst for details.
>>> +
>>> +Sysfs Attributes
>>> +----------------
>>> +
>>> +================= =============================================
>>> +temp1_input       Measured temperature of SoC
>>> +temp1_crit        Critical high temperature
>>> +temp1_crit_hyst   hysteresis temperature restore from Critical
>>> +temp2_input       Measured temperature of the base board
>>> +================= =============================================
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index b60fe2e58ad6..7aa6c3f322e5 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -2066,6 +2066,17 @@ config SENSORS_SFCTEMP
>>>    	  This driver can also be built as a module.  If so, the module
>>>    	  will be called sfctemp.
>>> +config SENSORS_SG2042_MCU
>>> +	tristate "Sophgo onboard MCU support"
>>> +	depends on I2C
>>> +	depends on ARCH_SOPHGO || COMPILE_TEST
>>> +	help
>>> +	  Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
>>> +	  power control and some basic information.
>>> +
>>> +	  This driver can be built as a module. If so, the module
>>> +	  will be called sg2042-mcu.
>>> +
>>>    config SENSORS_SURFACE_FAN
>>>    	tristate "Surface Fan Driver"
>>>    	depends on SURFACE_AGGREGATOR
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index b1c7056c37db..0bbe812a67ae 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>>>    obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
>>>    obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
>>>    obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
>>> +obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
>>>    obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
>>>    obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>>>    obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
>>> diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
>>> new file mode 100644
>>> index 000000000000..6d8d677f86f3
>>> --- /dev/null
>>> +++ b/drivers/hwmon/sg2042-mcu.c
>>> @@ -0,0 +1,394 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2024 Inochi Amaoto <[e-mail]>
>>> + *
>>> + * Sophgo power control mcu for SG2042
>>> + */
>>> +
>>> +#include <linux/cleanup.h>
>>> +#include <linux/debugfs.h>
>>> +#include <linux/err.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +
>>> +/* fixed MCU registers */
>>> +#define REG_BOARD_TYPE				0x00
>>> +#define REG_MCU_FIRMWARE_VERSION		0x01
>>> +#define REG_PCB_VERSION				0x02
>>> +#define REG_PWR_CTRL				0x03
>>> +#define REG_SOC_TEMP				0x04
>>> +#define REG_BOARD_TEMP				0x05
>>> +#define REG_RST_COUNT				0x0a
>>> +#define REG_UPTIME				0x0b
>>> +#define REG_RESET_REASON			0x0d
>>> +#define REG_MCU_TYPE				0x18
>>> +#define REG_REPOWER_ACTION			0x65
>>> +#define REG_CRITICAL_TEMP			0x66
>>> +#define REG_REPOWER_TEMP			0x67
>>> +
>>> +#define REPOWER_ACTION_REBOOT			1
>>> +#define REPOWER_ACTION_POWEROFF			2
>>> +
>>> +#define MCU_POWER_MAX				0xff
>>> +
>>> +#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format)			\
>>> +	static int _name##_show(struct seq_file *seqf,			\
>>> +				    void *unused)			\
>>> +	{								\
>>> +		struct sg2042_mcu_data *mcu = seqf->private;		\
>>> +		int ret;						\
>>> +		ret = i2c_smbus_read_byte_data(mcu->client, (_reg));	\
>>> +		if (ret < 0)						\
>>> +			return ret;					\
>>> +		seq_printf(seqf, _format "\n", ret);			\
>>> +		return 0;						\
>>> +	}								\
>>> +	DEFINE_SHOW_ATTRIBUTE(_name)					\
>>> +
>>> +struct sg2042_mcu_data {
>>> +	struct i2c_client	*client;
>>> +	struct dentry		*debugfs;
>>> +	struct mutex		mutex;
>>> +};
>>> +
>>> +static struct dentry *sgmcu_debugfs;
>>> +
>>> +static ssize_t reset_count_show(struct device *dev,
>>> +				struct device_attribute *attr,
>>> +				char *buf)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return sprintf(buf, "%d\n", ret);
>>> +}
>>> +
>>> +static ssize_t uptime_show(struct device *dev,
>>> +			   struct device_attribute *attr,
>>> +			   char *buf)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> +	u8 time_val[2];
>>> +	int ret;
>>> +
>>> +	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
>>> +					    sizeof(time_val), time_val);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return sprintf(buf, "%d\n",
>>> +		       (int)(time_val[0]) + (int)(time_val[1] << 8));
>>
>> Pointless type casts
>>
>>> +}
>>> +
>>> +static ssize_t reset_reason_show(struct device *dev,
>>> +				 struct device_attribute *attr,
>>> +				 char *buf)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return sprintf(buf, "0x%02x\n", ret);
>>> +}
>>> +
>>> +static ssize_t repower_action_show(struct device *dev,
>>> +				   struct device_attribute *attr,
>>> +				   char *buf)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> +	int ret;
>>> +	const char *action;
>>> +
>>> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_REPOWER_ACTION);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	if (ret == REPOWER_ACTION_REBOOT)
>>> +		action = "reboot";
>>> +	else if (ret == REPOWER_ACTION_POWEROFF)
>>> +		action = "poweroff";
>>> +	else
>>> +		action = "unknown";
>>> +
>>> +	return sprintf(buf, "%s\n", action);
>>> +}
>>> +
>>> +static ssize_t repower_action_store(struct device *dev,
>>> +				    struct device_attribute *attr,
>>> +				    const char *buf, size_t count)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> +	u8 value;
>>> +	int ret;
>>> +
>>> +	if (sysfs_streq("reboot", buf))
>>> +		value = REPOWER_ACTION_REBOOT;
>>> +	else if (sysfs_streq("poweroff", buf))
>>> +		value = REPOWER_ACTION_POWEROFF;
>>> +	else
>>> +		return -EINVAL;
>>> +
>>> +	guard(mutex)(&mcu->mutex);
>>> +
>> Pointless mutex protection.
>>
> 
> OK, I have seen the protection in the i2c bus. I will remove this.
> 
>>> +	ret = i2c_smbus_write_byte_data(mcu->client,
>>> +					REG_REPOWER_ACTION, value);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR_RO(reset_count);
>>> +static DEVICE_ATTR_RO(uptime);
>>> +static DEVICE_ATTR_RO(reset_reason);
>>> +static DEVICE_ATTR_RW(repower_action);
>>> +
>>> +DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
>>> +DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
>>> +DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
>>> +DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
>>> +
>>> +static struct attribute *sg2042_mcu_attrs[] = {
>>> +	&dev_attr_reset_count.attr,
>>> +	&dev_attr_uptime.attr,
>>> +	&dev_attr_reset_reason.attr,
>>> +	&dev_attr_repower_action.attr,
>>> +	NULL
>>> +};
>>> +
>>> +static const struct attribute_group sg2042_mcu_attr_group = {
>>> +	.attrs	= sg2042_mcu_attrs,
>>> +};
>>> +
>>> +static const struct attribute_group *sg2042_mcu_groups[] = {
>>> +	&sg2042_mcu_attr_group,
>>> +	NULL
>>> +};
>>> +
>>> +static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
>>> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
>>> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
>>> +					HWMON_T_CRIT_HYST,
>>> +				 HWMON_T_INPUT),
>>> +	NULL
>>> +};
>>> +
>>> +static int sg2042_mcu_read(struct device *dev,
>>> +			   enum hwmon_sensor_types type,
>>> +			   u32 attr, int channel, long *val)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> +	int tmp;
>>> +	u8 reg;
>>> +
>>> +	switch (attr) {
>>> +	case hwmon_temp_input:
>>> +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
>>> +		break;
>>> +	case hwmon_temp_crit:
>>> +		reg = REG_CRITICAL_TEMP;
>>> +		break;
>>> +	case hwmon_temp_crit_hyst:
>>> +		reg = REG_REPOWER_TEMP;
>>> +		break;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +
>>> +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
>>> +	if (tmp < 0)
>>> +		return tmp;
>>> +	*val = tmp * 1000;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int sg2042_mcu_write(struct device *dev,
>>> +			    enum hwmon_sensor_types type,
>>> +			    u32 attr, int channel, long val)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
>>> +	int temp = val / 1000;
>>> +	int hyst_temp, crit_temp;
>>> +	u8 reg;
>>> +
>>> +	if (temp > MCU_POWER_MAX)
>>> +		temp = MCU_POWER_MAX;
>>> +	else if (temp < 0)
>>> +		temp = 0;
>>> +
>> 	temp = clamp_val(temp, 0, MCU_POWER_MAX);
>>
>>> +	guard(mutex)(&mcu->mutex);
>>> +
>>> +	switch (attr) {
>>> +	case hwmon_temp_crit:
>>> +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
>>> +						     REG_REPOWER_TEMP);
>>> +		if (hyst_temp < 0)
>>> +			return hyst_temp;
>>> +
>>> +		crit_temp = temp;
>>> +		reg = REG_CRITICAL_TEMP;
>>> +		break;
>>> +	case hwmon_temp_crit_hyst:
>>> +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
>>> +						     REG_CRITICAL_TEMP);
>>> +		if (crit_temp < 0)
>>> +			return crit_temp;
>>> +
>>> +		hyst_temp = temp;
>>> +		reg = REG_REPOWER_TEMP;
>>> +		break;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +
>>> +	/*
>>> +	 * ensure hyst_temp is smaller to avoid MCU from
>>> +	 * keeping triggering repower event.
>>> +	 */
>>> +	if (crit_temp < hyst_temp)
>>> +		return -EINVAL;
>>> +
>>> +	return i2c_smbus_write_byte_data(mcu->client, reg, temp);
>>> +}
>>> +
>>> +static umode_t sg2042_mcu_is_visible(const void *_data,
>>> +				     enum hwmon_sensor_types type,
>>> +				     u32 attr, int channel)
>>> +{
>>> +	switch (type) {
>>> +	case hwmon_temp:
>>> +		switch (attr) {
>>> +		case hwmon_temp_input:
>>> +			return 0444;
>>> +		case hwmon_temp_crit:
>>> +		case hwmon_temp_crit_hyst:
>>> +			if (channel == 0)
>>> +				return 0664;
>>> +			break;
>>
>> In practice this is the only break; in this nested switch sequence,
>> making the return 0; at the end necessary.
>>
>> Please either use break; always when returning 0, or return 0 here
>> as well and drop the final return 0;
>>
> 
> Thanks, but I have a question: as this break can be fallthrough to the
> default, is it better to use break;/return 0;, or just remove this
> break; and let it fallthrough?
> 

Either always use break; when returning 0 or always use "return 0;".
Please don't rely on fallthrough; it doesn't improve anything since you'd
have to tag it as fallthrough and you might then as well use break;
or return 0; directly.

Guenter

>>> +		default:
>>> +			return 0;
>>> +		}
>>> +		break;
>>> +	default:
>>> +		return 0;
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct hwmon_ops sg2042_mcu_ops = {
>>> +	.is_visible = sg2042_mcu_is_visible,
>>> +	.read = sg2042_mcu_read,
>>> +	.write = sg2042_mcu_write,
>>> +};
>>> +
>>> +static const struct hwmon_chip_info sg2042_mcu_chip_info = {
>>> +	.ops = &sg2042_mcu_ops,
>>> +	.info = sg2042_mcu_info,
>>> +};
>>> +
>>> +static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
>>> +				    struct device *dev)
>>> +{
>>> +	mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
>>> +	if (mcu->debugfs) {
>>
>> debugfs_create_dir() returns an ERR_PTR(), which is checked in debugfs_create_file().
>> This if() is pointless.
>>
> 
> Thanks.
> 
>>> +		debugfs_create_file("firmware_version", 0444, mcu->debugfs,
>>> +				    mcu, &firmware_version_fops);
>>> +		debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
>>> +				    &pcb_version_fops);
>>> +		debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
>>> +				    &mcu_type_fops);
>>> +		debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
>>> +				    &board_type_fops);
>>> +	}
>>> +}
>>> +
>>> +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
>>> +{
>>> +	struct device *dev = &client->dev;
>>> +	struct sg2042_mcu_data *mcu;
>>> +	struct device *hwmon_dev;
>>> +
>>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>>> +						I2C_FUNC_SMBUS_BLOCK_DATA))
>>> +		return -EIO;
>>> +
>> 		return -ENODEV;
>>
>>> +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
>>> +	if (!mcu)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&mcu->mutex);
>>> +	mcu->client = client;
>>> +
>>> +	i2c_set_clientdata(client, mcu);
>>> +
>>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
>>> +							 mcu,
>>> +							 &sg2042_mcu_chip_info,
>>> +							 NULL);
>>> +	if (IS_ERR_OR_NULL(hwmon_dev))
>>> +		return -EFAULT;
>>
>> devm_hwmon_device_register_with_info() returns an ERR_PTR(), and
>> returning -EFAULT instead of it is more than wrong (this does not indicate
>> a "bad address"). This needs to be
>>
>> 	if (IS_ERR(hwmon_dev))
>> 		return PTR_ERR(hwmon_dev);
>>
>> I am sure I asked before not to overwrite error codes.
> 
> Yes, you have asked, it seems that I have missed this code when I
> check the driver. I apology for this mistake and thanks for your
> reminder.
> 
>>
>>> +
>>> +	sg2042_mcu_debugfs_init(mcu, dev);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
>>> +{
>>> +	struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
>>> +
>>> +	debugfs_remove_recursive(mcu->debugfs);
>>> +}
>>> +
>>> +static const struct i2c_device_id sg2042_mcu_id[] = {
>>> +	{ "sg2042-hwmon-mcu", 0 },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
>>> +
>>> +static const struct of_device_id sg2042_mcu_of_id[] = {
>>> +	{ .compatible = "sophgo,sg2042-hwmon-mcu" },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
>>> +
>>> +static struct i2c_driver sg2042_mcu_driver = {
>>> +	.driver = {
>>> +		.name = "sg2042-mcu",
>>> +		.of_match_table = sg2042_mcu_of_id,
>>> +		.dev_groups = sg2042_mcu_groups,
>>> +	},
>>> +	.probe = sg2042_mcu_i2c_probe,
>>> +	.remove = sg2042_mcu_i2c_remove,
>>> +	.id_table = sg2042_mcu_id,
>>> +};
>>> +
>>> +static int __init sg2042_mcu_init(void)
>>> +{
>>> +	sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
>>> +	return i2c_add_driver(&sg2042_mcu_driver);
>>> +}
>>> +
>>> +static void __exit sg2042_mcu_exit(void)
>>> +{
>>> +	debugfs_remove_recursive(sgmcu_debugfs);
>>> +	i2c_del_driver(&sg2042_mcu_driver);
>>> +}
>>> +
>>> +module_init(sg2042_mcu_init);
>>> +module_exit(sg2042_mcu_exit);
>>> +
>>> +MODULE_AUTHOR("Inochi Amaoto <[e-mail]>");
>>> +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
>>> +MODULE_LICENSE("GPL");
>>

Inochi Amaoto Aug. 3, 2024, 3:56 a.m. UTC | #4

On Fri, Aug 02, 2024 at 08:47:10PM GMT, Guenter Roeck wrote:
> On 8/2/24 16:28, Inochi Amaoto wrote:
> > On Fri, Aug 02, 2024 at 06:59:22AM GMT, Guenter Roeck wrote:
> > > On 8/2/24 05:38, 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 <[e-mail]>
> > > > Tested-by: Chen Wang <[e-mail]>
> > > > Reviewed-by: Chen Wang <[e-mail]>
> > > > ---
> > > >    Documentation/hwmon/index.rst      |   1 +
> > > >    Documentation/hwmon/sg2042-mcu.rst |  39 +++
> > > >    drivers/hwmon/Kconfig              |  11 +
> > > >    drivers/hwmon/Makefile             |   1 +
> > > >    drivers/hwmon/sg2042-mcu.c         | 394 +++++++++++++++++++++++++++++
> > > >    5 files changed, 446 insertions(+)
> > > >    create mode 100644 Documentation/hwmon/sg2042-mcu.rst
> > > >    create mode 100644 drivers/hwmon/sg2042-mcu.c
> > > > 
> > > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > > > index 913c11390a45..ea3b5be8fe4f 100644
> > > > --- a/Documentation/hwmon/index.rst
> > > > +++ b/Documentation/hwmon/index.rst
> > > > @@ -206,6 +206,7 @@ Hardware Monitoring Kernel Drivers
> > > >       sch5636
> > > >       scpi-hwmon
> > > >       sfctemp
> > > > +   sg2042-mcu
> > > >       sht15
> > > >       sht21
> > > >       sht3x
> > > > diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
> > > > new file mode 100644
> > > > index 000000000000..250016b47dd1
> > > > --- /dev/null
> > > > +++ b/Documentation/hwmon/sg2042-mcu.rst
> > > > @@ -0,0 +1,39 @@
> > > > +.. SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +Kernel driver sg2042-mcu
> > > > +=====================
> > > > +
> > > > +Supported chips:
> > > > +
> > > > +  * Onboard MCU for sg2042
> > > > +
> > > > +    Addresses scanned: -
> > > > +
> > > > +    Prefix: 'sg2042-mcu'
> > > > +
> > > > +Authors:
> > > > +
> > > > +  - Inochi Amaoto <[e-mail]>
> > > > +
> > > > +Description
> > > > +-----------
> > > > +
> > > > +This driver supprts hardware monitoring for onboard MCU with
> > > > +i2c interface.
> > > > +
> > > > +Usage Notes
> > > > +-----------
> > > > +
> > > > +This driver does not auto-detect devices. You will have to instantiate
> > > > +the devices explicitly.
> > > > +Please see Documentation/i2c/instantiating-devices.rst for details.
> > > > +
> > > > +Sysfs Attributes
> > > > +----------------
> > > > +
> > > > +================= =============================================
> > > > +temp1_input       Measured temperature of SoC
> > > > +temp1_crit        Critical high temperature
> > > > +temp1_crit_hyst   hysteresis temperature restore from Critical
> > > > +temp2_input       Measured temperature of the base board
> > > > +================= =============================================
> > > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > > index b60fe2e58ad6..7aa6c3f322e5 100644
> > > > --- a/drivers/hwmon/Kconfig
> > > > +++ b/drivers/hwmon/Kconfig
> > > > @@ -2066,6 +2066,17 @@ config SENSORS_SFCTEMP
> > > >    	  This driver can also be built as a module.  If so, the module
> > > >    	  will be called sfctemp.
> > > > +config SENSORS_SG2042_MCU
> > > > +	tristate "Sophgo onboard MCU support"
> > > > +	depends on I2C
> > > > +	depends on ARCH_SOPHGO || COMPILE_TEST
> > > > +	help
> > > > +	  Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
> > > > +	  power control and some basic information.
> > > > +
> > > > +	  This driver can be built as a module. If so, the module
> > > > +	  will be called sg2042-mcu.
> > > > +
> > > >    config SENSORS_SURFACE_FAN
> > > >    	tristate "Surface Fan Driver"
> > > >    	depends on SURFACE_AGGREGATOR
> > > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > > index b1c7056c37db..0bbe812a67ae 100644
> > > > --- a/drivers/hwmon/Makefile
> > > > +++ b/drivers/hwmon/Makefile
> > > > @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > > >    obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
> > > >    obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
> > > >    obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
> > > > +obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
> > > >    obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
> > > >    obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
> > > >    obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> > > > diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
> > > > new file mode 100644
> > > > index 000000000000..6d8d677f86f3
> > > > --- /dev/null
> > > > +++ b/drivers/hwmon/sg2042-mcu.c
> > > > @@ -0,0 +1,394 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Copyright (c) 2024 Inochi Amaoto <[e-mail]>
> > > > + *
> > > > + * Sophgo power control mcu for SG2042
> > > > + */
> > > > +
> > > > +#include <linux/cleanup.h>
> > > > +#include <linux/debugfs.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/hwmon.h>
> > > > +#include <linux/i2c.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/mutex.h>
> > > > +
> > > > +/* fixed MCU registers */
> > > > +#define REG_BOARD_TYPE				0x00
> > > > +#define REG_MCU_FIRMWARE_VERSION		0x01
> > > > +#define REG_PCB_VERSION				0x02
> > > > +#define REG_PWR_CTRL				0x03
> > > > +#define REG_SOC_TEMP				0x04
> > > > +#define REG_BOARD_TEMP				0x05
> > > > +#define REG_RST_COUNT				0x0a
> > > > +#define REG_UPTIME				0x0b
> > > > +#define REG_RESET_REASON			0x0d
> > > > +#define REG_MCU_TYPE				0x18
> > > > +#define REG_REPOWER_ACTION			0x65
> > > > +#define REG_CRITICAL_TEMP			0x66
> > > > +#define REG_REPOWER_TEMP			0x67
> > > > +
> > > > +#define REPOWER_ACTION_REBOOT			1
> > > > +#define REPOWER_ACTION_POWEROFF			2
> > > > +
> > > > +#define MCU_POWER_MAX				0xff
> > > > +
> > > > +#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format)			\
> > > > +	static int _name##_show(struct seq_file *seqf,			\
> > > > +				    void *unused)			\
> > > > +	{								\
> > > > +		struct sg2042_mcu_data *mcu = seqf->private;		\
> > > > +		int ret;						\
> > > > +		ret = i2c_smbus_read_byte_data(mcu->client, (_reg));	\
> > > > +		if (ret < 0)						\
> > > > +			return ret;					\
> > > > +		seq_printf(seqf, _format "\n", ret);			\
> > > > +		return 0;						\
> > > > +	}								\
> > > > +	DEFINE_SHOW_ATTRIBUTE(_name)					\
> > > > +
> > > > +struct sg2042_mcu_data {
> > > > +	struct i2c_client	*client;
> > > > +	struct dentry		*debugfs;
> > > > +	struct mutex		mutex;
> > > > +};
> > > > +
> > > > +static struct dentry *sgmcu_debugfs;
> > > > +
> > > > +static ssize_t reset_count_show(struct device *dev,
> > > > +				struct device_attribute *attr,
> > > > +				char *buf)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	int ret;
> > > > +
> > > > +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return sprintf(buf, "%d\n", ret);
> > > > +}
> > > > +
> > > > +static ssize_t uptime_show(struct device *dev,
> > > > +			   struct device_attribute *attr,
> > > > +			   char *buf)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	u8 time_val[2];
> > > > +	int ret;
> > > > +
> > > > +	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
> > > > +					    sizeof(time_val), time_val);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return sprintf(buf, "%d\n",
> > > > +		       (int)(time_val[0]) + (int)(time_val[1] << 8));
> > > 
> > > Pointless type casts
> > > 
> > > > +}
> > > > +
> > > > +static ssize_t reset_reason_show(struct device *dev,
> > > > +				 struct device_attribute *attr,
> > > > +				 char *buf)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	int ret;
> > > > +
> > > > +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return sprintf(buf, "0x%02x\n", ret);
> > > > +}
> > > > +
> > > > +static ssize_t repower_action_show(struct device *dev,
> > > > +				   struct device_attribute *attr,
> > > > +				   char *buf)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	int ret;
> > > > +	const char *action;
> > > > +
> > > > +	ret = i2c_smbus_read_byte_data(mcu->client, REG_REPOWER_ACTION);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	if (ret == REPOWER_ACTION_REBOOT)
> > > > +		action = "reboot";
> > > > +	else if (ret == REPOWER_ACTION_POWEROFF)
> > > > +		action = "poweroff";
> > > > +	else
> > > > +		action = "unknown";
> > > > +
> > > > +	return sprintf(buf, "%s\n", action);
> > > > +}
> > > > +
> > > > +static ssize_t repower_action_store(struct device *dev,
> > > > +				    struct device_attribute *attr,
> > > > +				    const char *buf, size_t count)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	u8 value;
> > > > +	int ret;
> > > > +
> > > > +	if (sysfs_streq("reboot", buf))
> > > > +		value = REPOWER_ACTION_REBOOT;
> > > > +	else if (sysfs_streq("poweroff", buf))
> > > > +		value = REPOWER_ACTION_POWEROFF;
> > > > +	else
> > > > +		return -EINVAL;
> > > > +
> > > > +	guard(mutex)(&mcu->mutex);
> > > > +
> > > Pointless mutex protection.
> > > 
> > 
> > OK, I have seen the protection in the i2c bus. I will remove this.
> > 
> > > > +	ret = i2c_smbus_write_byte_data(mcu->client,
> > > > +					REG_REPOWER_ACTION, value);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return count;
> > > > +}
> > > > +
> > > > +static DEVICE_ATTR_RO(reset_count);
> > > > +static DEVICE_ATTR_RO(uptime);
> > > > +static DEVICE_ATTR_RO(reset_reason);
> > > > +static DEVICE_ATTR_RW(repower_action);
> > > > +
> > > > +DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
> > > > +DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
> > > > +DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
> > > > +DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
> > > > +
> > > > +static struct attribute *sg2042_mcu_attrs[] = {
> > > > +	&dev_attr_reset_count.attr,
> > > > +	&dev_attr_uptime.attr,
> > > > +	&dev_attr_reset_reason.attr,
> > > > +	&dev_attr_repower_action.attr,
> > > > +	NULL
> > > > +};
> > > > +
> > > > +static const struct attribute_group sg2042_mcu_attr_group = {
> > > > +	.attrs	= sg2042_mcu_attrs,
> > > > +};
> > > > +
> > > > +static const struct attribute_group *sg2042_mcu_groups[] = {
> > > > +	&sg2042_mcu_attr_group,
> > > > +	NULL
> > > > +};
> > > > +
> > > > +static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
> > > > +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> > > > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
> > > > +					HWMON_T_CRIT_HYST,
> > > > +				 HWMON_T_INPUT),
> > > > +	NULL
> > > > +};
> > > > +
> > > > +static int sg2042_mcu_read(struct device *dev,
> > > > +			   enum hwmon_sensor_types type,
> > > > +			   u32 attr, int channel, long *val)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	int tmp;
> > > > +	u8 reg;
> > > > +
> > > > +	switch (attr) {
> > > > +	case hwmon_temp_input:
> > > > +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> > > > +		break;
> > > > +	case hwmon_temp_crit:
> > > > +		reg = REG_CRITICAL_TEMP;
> > > > +		break;
> > > > +	case hwmon_temp_crit_hyst:
> > > > +		reg = REG_REPOWER_TEMP;
> > > > +		break;
> > > > +	default:
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > > +
> > > > +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> > > > +	if (tmp < 0)
> > > > +		return tmp;
> > > > +	*val = tmp * 1000;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int sg2042_mcu_write(struct device *dev,
> > > > +			    enum hwmon_sensor_types type,
> > > > +			    u32 attr, int channel, long val)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> > > > +	int temp = val / 1000;
> > > > +	int hyst_temp, crit_temp;
> > > > +	u8 reg;
> > > > +
> > > > +	if (temp > MCU_POWER_MAX)
> > > > +		temp = MCU_POWER_MAX;
> > > > +	else if (temp < 0)
> > > > +		temp = 0;
> > > > +
> > > 	temp = clamp_val(temp, 0, MCU_POWER_MAX);
> > > 
> > > > +	guard(mutex)(&mcu->mutex);
> > > > +
> > > > +	switch (attr) {
> > > > +	case hwmon_temp_crit:
> > > > +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> > > > +						     REG_REPOWER_TEMP);
> > > > +		if (hyst_temp < 0)
> > > > +			return hyst_temp;
> > > > +
> > > > +		crit_temp = temp;
> > > > +		reg = REG_CRITICAL_TEMP;
> > > > +		break;
> > > > +	case hwmon_temp_crit_hyst:
> > > > +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> > > > +						     REG_CRITICAL_TEMP);
> > > > +		if (crit_temp < 0)
> > > > +			return crit_temp;
> > > > +
> > > > +		hyst_temp = temp;
> > > > +		reg = REG_REPOWER_TEMP;
> > > > +		break;
> > > > +	default:
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * ensure hyst_temp is smaller to avoid MCU from
> > > > +	 * keeping triggering repower event.
> > > > +	 */
> > > > +	if (crit_temp < hyst_temp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	return i2c_smbus_write_byte_data(mcu->client, reg, temp);
> > > > +}
> > > > +
> > > > +static umode_t sg2042_mcu_is_visible(const void *_data,
> > > > +				     enum hwmon_sensor_types type,
> > > > +				     u32 attr, int channel)
> > > > +{
> > > > +	switch (type) {
> > > > +	case hwmon_temp:
> > > > +		switch (attr) {
> > > > +		case hwmon_temp_input:
> > > > +			return 0444;
> > > > +		case hwmon_temp_crit:
> > > > +		case hwmon_temp_crit_hyst:
> > > > +			if (channel == 0)
> > > > +				return 0664;
> > > > +			break;
> > > 
> > > In practice this is the only break; in this nested switch sequence,
> > > making the return 0; at the end necessary.
> > > 
> > > Please either use break; always when returning 0, or return 0 here
> > > as well and drop the final return 0;
> > > 
> > 
> > Thanks, but I have a question: as this break can be fallthrough to the
> > default, is it better to use break;/return 0;, or just remove this
> > break; and let it fallthrough?
> > 
> 
> Either always use break; when returning 0 or always use "return 0;".
> Please don't rely on fallthrough; it doesn't improve anything since you'd
> have to tag it as fallthrough and you might then as well use break;
> or return 0; directly.
> 
> Guenter
> 

Thanks.

Regards,
Inochi.

> > > > +		default:
> > > > +			return 0;
> > > > +		}
> > > > +		break;
> > > > +	default:
> > > > +		return 0;
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct hwmon_ops sg2042_mcu_ops = {
> > > > +	.is_visible = sg2042_mcu_is_visible,
> > > > +	.read = sg2042_mcu_read,
> > > > +	.write = sg2042_mcu_write,
> > > > +};
> > > > +
> > > > +static const struct hwmon_chip_info sg2042_mcu_chip_info = {
> > > > +	.ops = &sg2042_mcu_ops,
> > > > +	.info = sg2042_mcu_info,
> > > > +};
> > > > +
> > > > +static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
> > > > +				    struct device *dev)
> > > > +{
> > > > +	mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
> > > > +	if (mcu->debugfs) {
> > > 
> > > debugfs_create_dir() returns an ERR_PTR(), which is checked in debugfs_create_file().
> > > This if() is pointless.
> > > 
> > 
> > Thanks.
> > 
> > > > +		debugfs_create_file("firmware_version", 0444, mcu->debugfs,
> > > > +				    mcu, &firmware_version_fops);
> > > > +		debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
> > > > +				    &pcb_version_fops);
> > > > +		debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
> > > > +				    &mcu_type_fops);
> > > > +		debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
> > > > +				    &board_type_fops);
> > > > +	}
> > > > +}
> > > > +
> > > > +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
> > > > +{
> > > > +	struct device *dev = &client->dev;
> > > > +	struct sg2042_mcu_data *mcu;
> > > > +	struct device *hwmon_dev;
> > > > +
> > > > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> > > > +						I2C_FUNC_SMBUS_BLOCK_DATA))
> > > > +		return -EIO;
> > > > +
> > > 		return -ENODEV;
> > > 
> > > > +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
> > > > +	if (!mcu)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	mutex_init(&mcu->mutex);
> > > > +	mcu->client = client;
> > > > +
> > > > +	i2c_set_clientdata(client, mcu);
> > > > +
> > > > +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
> > > > +							 mcu,
> > > > +							 &sg2042_mcu_chip_info,
> > > > +							 NULL);
> > > > +	if (IS_ERR_OR_NULL(hwmon_dev))
> > > > +		return -EFAULT;
> > > 
> > > devm_hwmon_device_register_with_info() returns an ERR_PTR(), and
> > > returning -EFAULT instead of it is more than wrong (this does not indicate
> > > a "bad address"). This needs to be
> > > 
> > > 	if (IS_ERR(hwmon_dev))
> > > 		return PTR_ERR(hwmon_dev);
> > > 
> > > I am sure I asked before not to overwrite error codes.
> > 
> > Yes, you have asked, it seems that I have missed this code when I
> > check the driver. I apology for this mistake and thanks for your
> > reminder.
> > 
> > > 
> > > > +
> > > > +	sg2042_mcu_debugfs_init(mcu, dev);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
> > > > +{
> > > > +	struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
> > > > +
> > > > +	debugfs_remove_recursive(mcu->debugfs);
> > > > +}
> > > > +
> > > > +static const struct i2c_device_id sg2042_mcu_id[] = {
> > > > +	{ "sg2042-hwmon-mcu", 0 },
> > > > +	{},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
> > > > +
> > > > +static const struct of_device_id sg2042_mcu_of_id[] = {
> > > > +	{ .compatible = "sophgo,sg2042-hwmon-mcu" },
> > > > +	{},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
> > > > +
> > > > +static struct i2c_driver sg2042_mcu_driver = {
> > > > +	.driver = {
> > > > +		.name = "sg2042-mcu",
> > > > +		.of_match_table = sg2042_mcu_of_id,
> > > > +		.dev_groups = sg2042_mcu_groups,
> > > > +	},
> > > > +	.probe = sg2042_mcu_i2c_probe,
> > > > +	.remove = sg2042_mcu_i2c_remove,
> > > > +	.id_table = sg2042_mcu_id,
> > > > +};
> > > > +
> > > > +static int __init sg2042_mcu_init(void)
> > > > +{
> > > > +	sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
> > > > +	return i2c_add_driver(&sg2042_mcu_driver);
> > > > +}
> > > > +
> > > > +static void __exit sg2042_mcu_exit(void)
> > > > +{
> > > > +	debugfs_remove_recursive(sgmcu_debugfs);
> > > > +	i2c_del_driver(&sg2042_mcu_driver);
> > > > +}
> > > > +
> > > > +module_init(sg2042_mcu_init);
> > > > +module_exit(sg2042_mcu_exit);
> > > > +
> > > > +MODULE_AUTHOR("Inochi Amaoto <[e-mail]>");
> > > > +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
> > > > +MODULE_LICENSE("GPL");
> > > 
>

Chen Wang Aug. 5, 2024, 1:38 a.m. UTC | #5

On 2024/8/2 20:38, 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 <[e-mail]>
> Tested-by: Chen Wang <[e-mail]>
> Reviewed-by: Chen Wang <[e-mail]>
> ---
>   Documentation/hwmon/index.rst      |   1 +
>   Documentation/hwmon/sg2042-mcu.rst |  39 +++
>   drivers/hwmon/Kconfig              |  11 +
>   drivers/hwmon/Makefile             |   1 +
>   drivers/hwmon/sg2042-mcu.c         | 394 +++++++++++++++++++++++++++++
>   5 files changed, 446 insertions(+)
>   create mode 100644 Documentation/hwmon/sg2042-mcu.rst
>   create mode 100644 drivers/hwmon/sg2042-mcu.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 913c11390a45..ea3b5be8fe4f 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -206,6 +206,7 @@ Hardware Monitoring Kernel Drivers
>      sch5636
>      scpi-hwmon
>      sfctemp
> +   sg2042-mcu
>      sht15
>      sht21
>      sht3x
> diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
> new file mode 100644
> index 000000000000..250016b47dd1
> --- /dev/null
> +++ b/Documentation/hwmon/sg2042-mcu.rst
> @@ -0,0 +1,39 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver sg2042-mcu
> +=====================
> +
> +Supported chips:
> +
> +  * Onboard MCU for sg2042
> +
> +    Addresses scanned: -
> +
> +    Prefix: 'sg2042-mcu'
> +
> +Authors:
> +
> +  - Inochi Amaoto <[e-mail]>
> +
> +Description
> +-----------
> +
> +This driver supprts hardware monitoring for onboard MCU with
> +i2c interface.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate
> +the devices explicitly.
> +Please see Documentation/i2c/instantiating-devices.rst for details.
> +
> +Sysfs Attributes
> +----------------
> +
> +================= =============================================
> +temp1_input       Measured temperature of SoC
> +temp1_crit        Critical high temperature
> +temp1_crit_hyst   hysteresis temperature restore from Critical
> +temp2_input       Measured temperature of the base board
> +================= =============================================
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index b60fe2e58ad6..7aa6c3f322e5 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2066,6 +2066,17 @@ config SENSORS_SFCTEMP
>   	  This driver can also be built as a module.  If so, the module
>   	  will be called sfctemp.
>   
> +config SENSORS_SG2042_MCU
> +	tristate "Sophgo onboard MCU support"
> +	depends on I2C
> +	depends on ARCH_SOPHGO || COMPILE_TEST
> +	help
> +	  Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
> +	  power control and some basic information.
> +
> +	  This driver can be built as a module. If so, the module
> +	  will be called sg2042-mcu.
> +
>   config SENSORS_SURFACE_FAN
>   	tristate "Surface Fan Driver"
>   	depends on SURFACE_AGGREGATOR
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b1c7056c37db..0bbe812a67ae 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>   obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
>   obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
>   obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
> +obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
>   obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
>   obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>   obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
> new file mode 100644
> index 000000000000..6d8d677f86f3
> --- /dev/null
> +++ b/drivers/hwmon/sg2042-mcu.c
> @@ -0,0 +1,394 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Inochi Amaoto <[e-mail]>
> + *
> + * Sophgo power control mcu for SG2042
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/debugfs.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +
> +/* fixed MCU registers */
> +#define REG_BOARD_TYPE				0x00
> +#define REG_MCU_FIRMWARE_VERSION		0x01
> +#define REG_PCB_VERSION				0x02
> +#define REG_PWR_CTRL				0x03
> +#define REG_SOC_TEMP				0x04
> +#define REG_BOARD_TEMP				0x05
> +#define REG_RST_COUNT				0x0a
> +#define REG_UPTIME				0x0b
> +#define REG_RESET_REASON			0x0d
> +#define REG_MCU_TYPE				0x18
> +#define REG_REPOWER_ACTION			0x65
> +#define REG_CRITICAL_TEMP			0x66
> +#define REG_REPOWER_TEMP			0x67
As I mentioend in last review, suggest to add some clarification for 
CRITICAL and REPOWER. Due to no document from sophgo, I hope some 
comments in code for memo.
> +
> +#define REPOWER_ACTION_REBOOT			1
> +#define REPOWER_ACTION_POWEROFF			2
> +
> +#define MCU_POWER_MAX				0xff
> +
> +#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format)			\
> +	static int _name##_show(struct seq_file *seqf,			\
> +				    void *unused)			\
> +	{								\
> +		struct sg2042_mcu_data *mcu = seqf->private;		\
> +		int ret;						\
> +		ret = i2c_smbus_read_byte_data(mcu->client, (_reg));	\
> +		if (ret < 0)						\
> +			return ret;					\
> +		seq_printf(seqf, _format "\n", ret);			\
> +		return 0;						\
> +	}								\
> +	DEFINE_SHOW_ATTRIBUTE(_name)					\
> +
> +struct sg2042_mcu_data {
> +	struct i2c_client	*client;
> +	struct dentry		*debugfs;
> +	struct mutex		mutex;
> +};
> +
> +static struct dentry *sgmcu_debugfs;
> +
> +static ssize_t reset_count_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t uptime_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	u8 time_val[2];
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
> +					    sizeof(time_val), time_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n",
> +		       (int)(time_val[0]) + (int)(time_val[1] << 8));
> +}
> +
> +static ssize_t reset_reason_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "0x%02x\n", ret);
> +}
> +
> +static ssize_t repower_action_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int ret;
> +	const char *action;
> +
> +	ret = i2c_smbus_read_byte_data(mcu->client, REG_REPOWER_ACTION);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret == REPOWER_ACTION_REBOOT)
> +		action = "reboot";
> +	else if (ret == REPOWER_ACTION_POWEROFF)
> +		action = "poweroff";
> +	else
> +		action = "unknown";
> +
> +	return sprintf(buf, "%s\n", action);
> +}
> +
> +static ssize_t repower_action_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	u8 value;
> +	int ret;
> +
> +	if (sysfs_streq("reboot", buf))
> +		value = REPOWER_ACTION_REBOOT;
> +	else if (sysfs_streq("poweroff", buf))
> +		value = REPOWER_ACTION_POWEROFF;
> +	else
> +		return -EINVAL;
> +
> +	guard(mutex)(&mcu->mutex);
> +
> +	ret = i2c_smbus_write_byte_data(mcu->client,
> +					REG_REPOWER_ACTION, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RO(reset_count);
> +static DEVICE_ATTR_RO(uptime);
> +static DEVICE_ATTR_RO(reset_reason);
> +static DEVICE_ATTR_RW(repower_action);
> +
> +DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
> +DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
> +DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
> +DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
> +
> +static struct attribute *sg2042_mcu_attrs[] = {
> +	&dev_attr_reset_count.attr,
> +	&dev_attr_uptime.attr,
> +	&dev_attr_reset_reason.attr,
> +	&dev_attr_repower_action.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group sg2042_mcu_attr_group = {
> +	.attrs	= sg2042_mcu_attrs,
> +};
> +
> +static const struct attribute_group *sg2042_mcu_groups[] = {
> +	&sg2042_mcu_attr_group,
> +	NULL
> +};
> +
> +static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
> +					HWMON_T_CRIT_HYST,
> +				 HWMON_T_INPUT),
> +	NULL
> +};
> +
> +static int sg2042_mcu_read(struct device *dev,
> +			   enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int tmp;
> +	u8 reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
> +		break;
> +	case hwmon_temp_crit:
> +		reg = REG_CRITICAL_TEMP;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		reg = REG_REPOWER_TEMP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
> +	if (tmp < 0)
> +		return tmp;
> +	*val = tmp * 1000;
> +
> +	return 0;
> +}
> +
> +static int sg2042_mcu_write(struct device *dev,
> +			    enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long val)
> +{
> +	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
> +	int temp = val / 1000;
> +	int hyst_temp, crit_temp;
> +	u8 reg;
> +
> +	if (temp > MCU_POWER_MAX)
> +		temp = MCU_POWER_MAX;
> +	else if (temp < 0)
> +		temp = 0;
> +
> +	guard(mutex)(&mcu->mutex);
> +
> +	switch (attr) {
> +	case hwmon_temp_crit:
> +		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
> +						     REG_REPOWER_TEMP);
> +		if (hyst_temp < 0)
> +			return hyst_temp;
> +
> +		crit_temp = temp;
> +		reg = REG_CRITICAL_TEMP;
> +		break;
> +	case hwmon_temp_crit_hyst:
> +		crit_temp = i2c_smbus_read_byte_data(mcu->client,
> +						     REG_CRITICAL_TEMP);
> +		if (crit_temp < 0)
> +			return crit_temp;
> +
> +		hyst_temp = temp;
> +		reg = REG_REPOWER_TEMP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/*
> +	 * ensure hyst_temp is smaller to avoid MCU from
> +	 * keeping triggering repower event.
> +	 */
> +	if (crit_temp < hyst_temp)
> +		return -EINVAL;
> +
> +	return i2c_smbus_write_byte_data(mcu->client, reg, temp);
> +}
> +
> +static umode_t sg2042_mcu_is_visible(const void *_data,
> +				     enum hwmon_sensor_types type,
> +				     u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			return 0444;
> +		case hwmon_temp_crit:
> +		case hwmon_temp_crit_hyst:
> +			if (channel == 0)
> +				return 0664;
> +			break;
> +		default:
> +			return 0;
> +		}
> +		break;
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_ops sg2042_mcu_ops = {
> +	.is_visible = sg2042_mcu_is_visible,
> +	.read = sg2042_mcu_read,
> +	.write = sg2042_mcu_write,
> +};
> +
> +static const struct hwmon_chip_info sg2042_mcu_chip_info = {
> +	.ops = &sg2042_mcu_ops,
> +	.info = sg2042_mcu_info,
> +};
> +
> +static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
> +				    struct device *dev)
> +{
> +	mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
> +	if (mcu->debugfs) {
> +		debugfs_create_file("firmware_version", 0444, mcu->debugfs,
> +				    mcu, &firmware_version_fops);
> +		debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
> +				    &pcb_version_fops);
> +		debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
> +				    &mcu_type_fops);
> +		debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
> +				    &board_type_fops);
> +	}
> +}
> +
> +static int sg2042_mcu_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct sg2042_mcu_data *mcu;
> +	struct device *hwmon_dev;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +						I2C_FUNC_SMBUS_BLOCK_DATA))
> +		return -EIO;
> +
> +	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
> +	if (!mcu)
> +		return -ENOMEM;
> +
> +	mutex_init(&mcu->mutex);
> +	mcu->client = client;
> +
> +	i2c_set_clientdata(client, mcu);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
> +							 mcu,
> +							 &sg2042_mcu_chip_info,
> +							 NULL);
> +	if (IS_ERR_OR_NULL(hwmon_dev))
> +		return -EFAULT;
> +
> +	sg2042_mcu_debugfs_init(mcu, dev);
> +
> +	return 0;
> +}
> +
> +static void sg2042_mcu_i2c_remove(struct i2c_client *client)
> +{
> +	struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
> +
> +	debugfs_remove_recursive(mcu->debugfs);
> +}
> +
> +static const struct i2c_device_id sg2042_mcu_id[] = {
> +	{ "sg2042-hwmon-mcu", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
> +
> +static const struct of_device_id sg2042_mcu_of_id[] = {
> +	{ .compatible = "sophgo,sg2042-hwmon-mcu" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
> +
> +static struct i2c_driver sg2042_mcu_driver = {
> +	.driver = {
> +		.name = "sg2042-mcu",
> +		.of_match_table = sg2042_mcu_of_id,
> +		.dev_groups = sg2042_mcu_groups,
> +	},
> +	.probe = sg2042_mcu_i2c_probe,
> +	.remove = sg2042_mcu_i2c_remove,
> +	.id_table = sg2042_mcu_id,
> +};
> +
> +static int __init sg2042_mcu_init(void)
> +{
> +	sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
> +	return i2c_add_driver(&sg2042_mcu_driver);
> +}
> +
> +static void __exit sg2042_mcu_exit(void)
> +{
> +	debugfs_remove_recursive(sgmcu_debugfs);
> +	i2c_del_driver(&sg2042_mcu_driver);
> +}
> +
> +module_init(sg2042_mcu_init);
> +module_exit(sg2042_mcu_exit);
> +
> +MODULE_AUTHOR("Inochi Amaoto <[e-mail]>");
> +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
> +MODULE_LICENSE("GPL");

Inochi Amaoto Aug. 10, 2024, 7:22 a.m. UTC | #6

On Mon, Aug 05, 2024 at 09:38:03AM GMT, Chen Wang wrote:
> 
> On 2024/8/2 20:38, 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 <[e-mail]>
> > Tested-by: Chen Wang <[e-mail]>
> > Reviewed-by: Chen Wang <[e-mail]>
> > ---
> >   Documentation/hwmon/index.rst      |   1 +
> >   Documentation/hwmon/sg2042-mcu.rst |  39 +++
> >   drivers/hwmon/Kconfig              |  11 +
> >   drivers/hwmon/Makefile             |   1 +
> >   drivers/hwmon/sg2042-mcu.c         | 394 +++++++++++++++++++++++++++++
> >   5 files changed, 446 insertions(+)
> >   create mode 100644 Documentation/hwmon/sg2042-mcu.rst
> >   create mode 100644 drivers/hwmon/sg2042-mcu.c
> > 
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index 913c11390a45..ea3b5be8fe4f 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -206,6 +206,7 @@ Hardware Monitoring Kernel Drivers
> >      sch5636
> >      scpi-hwmon
> >      sfctemp
> > +   sg2042-mcu
> >      sht15
> >      sht21
> >      sht3x
> > diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
> > new file mode 100644
> > index 000000000000..250016b47dd1
> > --- /dev/null
> > +++ b/Documentation/hwmon/sg2042-mcu.rst
> > @@ -0,0 +1,39 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver sg2042-mcu
> > +=====================
> > +
> > +Supported chips:
> > +
> > +  * Onboard MCU for sg2042
> > +
> > +    Addresses scanned: -
> > +
> > +    Prefix: 'sg2042-mcu'
> > +
> > +Authors:
> > +
> > +  - Inochi Amaoto <[e-mail]>
> > +
> > +Description
> > +-----------
> > +
> > +This driver supprts hardware monitoring for onboard MCU with
> > +i2c interface.
> > +
> > +Usage Notes
> > +-----------
> > +
> > +This driver does not auto-detect devices. You will have to instantiate
> > +the devices explicitly.
> > +Please see Documentation/i2c/instantiating-devices.rst for details.
> > +
> > +Sysfs Attributes
> > +----------------
> > +
> > +================= =============================================
> > +temp1_input       Measured temperature of SoC
> > +temp1_crit        Critical high temperature
> > +temp1_crit_hyst   hysteresis temperature restore from Critical
> > +temp2_input       Measured temperature of the base board
> > +================= =============================================
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index b60fe2e58ad6..7aa6c3f322e5 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -2066,6 +2066,17 @@ config SENSORS_SFCTEMP
> >   	  This driver can also be built as a module.  If so, the module
> >   	  will be called sfctemp.
> > +config SENSORS_SG2042_MCU
> > +	tristate "Sophgo onboard MCU support"
> > +	depends on I2C
> > +	depends on ARCH_SOPHGO || COMPILE_TEST
> > +	help
> > +	  Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
> > +	  power control and some basic information.
> > +
> > +	  This driver can be built as a module. If so, the module
> > +	  will be called sg2042-mcu.
> > +
> >   config SENSORS_SURFACE_FAN
> >   	tristate "Surface Fan Driver"
> >   	depends on SURFACE_AGGREGATOR
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index b1c7056c37db..0bbe812a67ae 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -194,6 +194,7 @@ obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> >   obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
> >   obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
> >   obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
> > +obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
> >   obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
> >   obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
> >   obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
> > diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
> > new file mode 100644
> > index 000000000000..6d8d677f86f3
> > --- /dev/null
> > +++ b/drivers/hwmon/sg2042-mcu.c
> > @@ -0,0 +1,394 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2024 Inochi Amaoto <[e-mail]>
> > + *
> > + * Sophgo power control mcu for SG2042
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +
> > +/* fixed MCU registers */
> > +#define REG_BOARD_TYPE				0x00
> > +#define REG_MCU_FIRMWARE_VERSION		0x01
> > +#define REG_PCB_VERSION				0x02
> > +#define REG_PWR_CTRL				0x03
> > +#define REG_SOC_TEMP				0x04
> > +#define REG_BOARD_TEMP				0x05
> > +#define REG_RST_COUNT				0x0a
> > +#define REG_UPTIME				0x0b
> > +#define REG_RESET_REASON			0x0d
> > +#define REG_MCU_TYPE				0x18
> > +#define REG_REPOWER_ACTION			0x65
> > +#define REG_CRITICAL_TEMP			0x66
> > +#define REG_REPOWER_TEMP			0x67
> As I mentioend in last review, suggest to add some clarification for
> CRITICAL and REPOWER. Due to no document from sophgo, I hope some comments
> in code for memo.

After some struggle, I find it is hard to add some comments here. 
It is useless and the code should be self-explained. Instead, I 
add the comment in the document and rename this reg to "repower 
policy" to make it clear. See it in the next version.

Regards,
Inochi

Patch

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 913c11390a45..ea3b5be8fe4f 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -206,6 +206,7 @@  Hardware Monitoring Kernel Drivers
    sch5636
    scpi-hwmon
    sfctemp
+   sg2042-mcu
    sht15
    sht21
    sht3x
diff --git a/Documentation/hwmon/sg2042-mcu.rst b/Documentation/hwmon/sg2042-mcu.rst
new file mode 100644
index 000000000000..250016b47dd1
--- /dev/null
+++ b/Documentation/hwmon/sg2042-mcu.rst
@@ -0,0 +1,39 @@ 
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver sg2042-mcu
+=====================
+
+Supported chips:
+
+  * Onboard MCU for sg2042
+
+    Addresses scanned: -
+
+    Prefix: 'sg2042-mcu'
+
+Authors:
+
+  - Inochi Amaoto <[e-mail]>
+
+Description
+-----------
+
+This driver supprts hardware monitoring for onboard MCU with
+i2c interface.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate
+the devices explicitly.
+Please see Documentation/i2c/instantiating-devices.rst for details.
+
+Sysfs Attributes
+----------------
+
+================= =============================================
+temp1_input       Measured temperature of SoC
+temp1_crit        Critical high temperature
+temp1_crit_hyst   hysteresis temperature restore from Critical
+temp2_input       Measured temperature of the base board
+================= =============================================
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b60fe2e58ad6..7aa6c3f322e5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2066,6 +2066,17 @@  config SENSORS_SFCTEMP
 	  This driver can also be built as a module.  If so, the module
 	  will be called sfctemp.
 
+config SENSORS_SG2042_MCU
+	tristate "Sophgo onboard MCU support"
+	depends on I2C
+	depends on ARCH_SOPHGO || COMPILE_TEST
+	help
+	  Support for onboard MCU of Sophgo SG2042 SoCs. This mcu provides
+	  power control and some basic information.
+
+	  This driver can be built as a module. If so, the module
+	  will be called sg2042-mcu.
+
 config SENSORS_SURFACE_FAN
 	tristate "Surface Fan Driver"
 	depends on SURFACE_AGGREGATOR
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b1c7056c37db..0bbe812a67ae 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -194,6 +194,7 @@  obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
 obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
 obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
 obj-$(CONFIG_SENSORS_SFCTEMP)	+= sfctemp.o
+obj-$(CONFIG_SENSORS_SG2042_MCU) += sg2042-mcu.o
 obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
diff --git a/drivers/hwmon/sg2042-mcu.c b/drivers/hwmon/sg2042-mcu.c
new file mode 100644
index 000000000000..6d8d677f86f3
--- /dev/null
+++ b/drivers/hwmon/sg2042-mcu.c
@@ -0,0 +1,394 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Inochi Amaoto <[e-mail]>
+ *
+ * Sophgo power control mcu for SG2042
+ */
+
+#include <linux/cleanup.h>
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+
+/* fixed MCU registers */
+#define REG_BOARD_TYPE				0x00
+#define REG_MCU_FIRMWARE_VERSION		0x01
+#define REG_PCB_VERSION				0x02
+#define REG_PWR_CTRL				0x03
+#define REG_SOC_TEMP				0x04
+#define REG_BOARD_TEMP				0x05
+#define REG_RST_COUNT				0x0a
+#define REG_UPTIME				0x0b
+#define REG_RESET_REASON			0x0d
+#define REG_MCU_TYPE				0x18
+#define REG_REPOWER_ACTION			0x65
+#define REG_CRITICAL_TEMP			0x66
+#define REG_REPOWER_TEMP			0x67
+
+#define REPOWER_ACTION_REBOOT			1
+#define REPOWER_ACTION_POWEROFF			2
+
+#define MCU_POWER_MAX				0xff
+
+#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format)			\
+	static int _name##_show(struct seq_file *seqf,			\
+				    void *unused)			\
+	{								\
+		struct sg2042_mcu_data *mcu = seqf->private;		\
+		int ret;						\
+		ret = i2c_smbus_read_byte_data(mcu->client, (_reg));	\
+		if (ret < 0)						\
+			return ret;					\
+		seq_printf(seqf, _format "\n", ret);			\
+		return 0;						\
+	}								\
+	DEFINE_SHOW_ATTRIBUTE(_name)					\
+
+struct sg2042_mcu_data {
+	struct i2c_client	*client;
+	struct dentry		*debugfs;
+	struct mutex		mutex;
+};
+
+static struct dentry *sgmcu_debugfs;
+
+static ssize_t reset_count_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", ret);
+}
+
+static ssize_t uptime_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	u8 time_val[2];
+	int ret;
+
+	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
+					    sizeof(time_val), time_val);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n",
+		       (int)(time_val[0]) + (int)(time_val[1] << 8));
+}
+
+static ssize_t reset_reason_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "0x%02x\n", ret);
+}
+
+static ssize_t repower_action_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+	const char *action;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_REPOWER_ACTION);
+	if (ret < 0)
+		return ret;
+
+	if (ret == REPOWER_ACTION_REBOOT)
+		action = "reboot";
+	else if (ret == REPOWER_ACTION_POWEROFF)
+		action = "poweroff";
+	else
+		action = "unknown";
+
+	return sprintf(buf, "%s\n", action);
+}
+
+static ssize_t repower_action_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	u8 value;
+	int ret;
+
+	if (sysfs_streq("reboot", buf))
+		value = REPOWER_ACTION_REBOOT;
+	else if (sysfs_streq("poweroff", buf))
+		value = REPOWER_ACTION_POWEROFF;
+	else
+		return -EINVAL;
+
+	guard(mutex)(&mcu->mutex);
+
+	ret = i2c_smbus_write_byte_data(mcu->client,
+					REG_REPOWER_ACTION, value);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static DEVICE_ATTR_RO(reset_count);
+static DEVICE_ATTR_RO(uptime);
+static DEVICE_ATTR_RO(reset_reason);
+static DEVICE_ATTR_RW(repower_action);
+
+DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
+
+static struct attribute *sg2042_mcu_attrs[] = {
+	&dev_attr_reset_count.attr,
+	&dev_attr_uptime.attr,
+	&dev_attr_reset_reason.attr,
+	&dev_attr_repower_action.attr,
+	NULL
+};
+
+static const struct attribute_group sg2042_mcu_attr_group = {
+	.attrs	= sg2042_mcu_attrs,
+};
+
+static const struct attribute_group *sg2042_mcu_groups[] = {
+	&sg2042_mcu_attr_group,
+	NULL
+};
+
+static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
+					HWMON_T_CRIT_HYST,
+				 HWMON_T_INPUT),
+	NULL
+};
+
+static int sg2042_mcu_read(struct device *dev,
+			   enum hwmon_sensor_types type,
+			   u32 attr, int channel, long *val)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int tmp;
+	u8 reg;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
+		break;
+	case hwmon_temp_crit:
+		reg = REG_CRITICAL_TEMP;
+		break;
+	case hwmon_temp_crit_hyst:
+		reg = REG_REPOWER_TEMP;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
+	if (tmp < 0)
+		return tmp;
+	*val = tmp * 1000;
+
+	return 0;
+}
+
+static int sg2042_mcu_write(struct device *dev,
+			    enum hwmon_sensor_types type,
+			    u32 attr, int channel, long val)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int temp = val / 1000;
+	int hyst_temp, crit_temp;
+	u8 reg;
+
+	if (temp > MCU_POWER_MAX)
+		temp = MCU_POWER_MAX;
+	else if (temp < 0)
+		temp = 0;
+
+	guard(mutex)(&mcu->mutex);
+
+	switch (attr) {
+	case hwmon_temp_crit:
+		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
+						     REG_REPOWER_TEMP);
+		if (hyst_temp < 0)
+			return hyst_temp;
+
+		crit_temp = temp;
+		reg = REG_CRITICAL_TEMP;
+		break;
+	case hwmon_temp_crit_hyst:
+		crit_temp = i2c_smbus_read_byte_data(mcu->client,
+						     REG_CRITICAL_TEMP);
+		if (crit_temp < 0)
+			return crit_temp;
+
+		hyst_temp = temp;
+		reg = REG_REPOWER_TEMP;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	/*
+	 * ensure hyst_temp is smaller to avoid MCU from
+	 * keeping triggering repower event.
+	 */
+	if (crit_temp < hyst_temp)
+		return -EINVAL;
+
+	return i2c_smbus_write_byte_data(mcu->client, reg, temp);
+}
+
+static umode_t sg2042_mcu_is_visible(const void *_data,
+				     enum hwmon_sensor_types type,
+				     u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return 0444;
+		case hwmon_temp_crit:
+		case hwmon_temp_crit_hyst:
+			if (channel == 0)
+				return 0664;
+			break;
+		default:
+			return 0;
+		}
+		break;
+	default:
+		return 0;
+	}
+	return 0;
+}
+
+static const struct hwmon_ops sg2042_mcu_ops = {
+	.is_visible = sg2042_mcu_is_visible,
+	.read = sg2042_mcu_read,
+	.write = sg2042_mcu_write,
+};
+
+static const struct hwmon_chip_info sg2042_mcu_chip_info = {
+	.ops = &sg2042_mcu_ops,
+	.info = sg2042_mcu_info,
+};
+
+static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
+				    struct device *dev)
+{
+	mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
+	if (mcu->debugfs) {
+		debugfs_create_file("firmware_version", 0444, mcu->debugfs,
+				    mcu, &firmware_version_fops);
+		debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
+				    &pcb_version_fops);
+		debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
+				    &mcu_type_fops);
+		debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
+				    &board_type_fops);
+	}
+}
+
+static int sg2042_mcu_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct sg2042_mcu_data *mcu;
+	struct device *hwmon_dev;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+						I2C_FUNC_SMBUS_BLOCK_DATA))
+		return -EIO;
+
+	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
+	if (!mcu)
+		return -ENOMEM;
+
+	mutex_init(&mcu->mutex);
+	mcu->client = client;
+
+	i2c_set_clientdata(client, mcu);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
+							 mcu,
+							 &sg2042_mcu_chip_info,
+							 NULL);
+	if (IS_ERR_OR_NULL(hwmon_dev))
+		return -EFAULT;
+
+	sg2042_mcu_debugfs_init(mcu, dev);
+
+	return 0;
+}
+
+static void sg2042_mcu_i2c_remove(struct i2c_client *client)
+{
+	struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
+
+	debugfs_remove_recursive(mcu->debugfs);
+}
+
+static const struct i2c_device_id sg2042_mcu_id[] = {
+	{ "sg2042-hwmon-mcu", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
+
+static const struct of_device_id sg2042_mcu_of_id[] = {
+	{ .compatible = "sophgo,sg2042-hwmon-mcu" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
+
+static struct i2c_driver sg2042_mcu_driver = {
+	.driver = {
+		.name = "sg2042-mcu",
+		.of_match_table = sg2042_mcu_of_id,
+		.dev_groups = sg2042_mcu_groups,
+	},
+	.probe = sg2042_mcu_i2c_probe,
+	.remove = sg2042_mcu_i2c_remove,
+	.id_table = sg2042_mcu_id,
+};
+
+static int __init sg2042_mcu_init(void)
+{
+	sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
+	return i2c_add_driver(&sg2042_mcu_driver);
+}
+
+static void __exit sg2042_mcu_exit(void)
+{
+	debugfs_remove_recursive(sgmcu_debugfs);
+	i2c_del_driver(&sg2042_mcu_driver);
+}
+
+module_init(sg2042_mcu_init);
+module_exit(sg2042_mcu_exit);
+
+MODULE_AUTHOR("Inochi Amaoto <[e-mail]>");
+MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
+MODULE_LICENSE("GPL");