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

Commit Message

Inochi Amaoto July 30, 2024, 7:50 a.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]>
---
 Documentation/hwmon/index.rst      |   1 +
 Documentation/hwmon/sg2042-mcu.rst |  39 +++
 drivers/hwmon/Kconfig              |  11 +
 drivers/hwmon/Makefile             |   1 +
 drivers/hwmon/sg2042-mcu.c         | 406 +++++++++++++++++++++++++++++
 5 files changed, 458 insertions(+)
 create mode 100644 Documentation/hwmon/sg2042-mcu.rst
 create mode 100644 drivers/hwmon/sg2042-mcu.c

--
2.45.2

Comments

Chen Wang July 31, 2024, 6:13 a.m. UTC | #1

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

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

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

Adding sophgo engineers Chunzhi and Haijiao, FYI.

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

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

[......]

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

[......]

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

[......]

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

Inochi Amaoto July 31, 2024, 7:17 a.m. UTC | #2

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

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

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

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

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

Yeah, this is good idea.

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

Guenter Roeck July 31, 2024, 3:02 p.m. UTC | #3

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

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

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

No lower limit ? -1000000 is ok ?

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

Do not overwrite error codes.

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

Do not overwrite error codes.

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

This is inconsistent.

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

Inochi Amaoto July 31, 2024, 10:48 p.m. UTC | #4

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

OK, I will merge these function.

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

My fault, it should always > 0.

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

Thanks, I will handle the return properly.

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

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..37fac3e5f233
--- /dev/null
+++ b/drivers/hwmon/sg2042-mcu.c
@@ -0,0 +1,406 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Inochi Amaoto <[e-mail]>
+ *
+ * Sophgo power control mcu for SG2042
+ */
+
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+
+/* fixed MCU registers */
+#define REG_BOARD_TYPE				0x00
+#define REG_MCU_FIRMWARE_VERSION		0x01
+#define REG_PCB_VERSION				0x02
+#define REG_PWR_CTRL				0x03
+#define REG_SOC_TEMP				0x04
+#define REG_BOARD_TEMP				0x05
+#define REG_RST_COUNT				0x0a
+#define REG_UPTIME				0x0b
+#define REG_RESET_REASON			0x0d
+#define REG_MCU_TYPE				0x18
+#define REG_CRITICAL_ACTIONS			0x65
+#define REG_CRITICAL_TEMP			0x66
+#define REG_REPOWER_TEMP			0x67
+
+#define CRITICAL_ACTION_REBOOT			1
+#define CRITICAL_ACTION_POWEROFF		2
+
+#define MCU_POWER_MAX				0xff
+
+#define DEFINE_MCU_DEBUG_ATTR(_name, _reg, _format)			\
+	static int _name##_show(struct seq_file *seqf,			\
+				    void *unused)			\
+	{								\
+		struct sg2042_mcu_data *mcu = seqf->private;		\
+		int ret;						\
+		ret = i2c_smbus_read_byte_data(mcu->client, (_reg));	\
+		if (ret < 0)						\
+			return ret;					\
+		seq_printf(seqf, _format "\n", ret);			\
+		return 0;						\
+	}								\
+	DEFINE_SHOW_ATTRIBUTE(_name)					\
+
+struct sg2042_mcu_data {
+	struct i2c_client	*client;
+	struct dentry		*debugfs;
+	struct mutex		mutex;
+};
+
+static struct dentry *sgmcu_debugfs;
+
+static ssize_t reset_count_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_RST_COUNT);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", ret);
+}
+
+static ssize_t uptime_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	u8 time_val[2];
+	int ret;
+
+	ret = i2c_smbus_read_i2c_block_data(mcu->client, REG_UPTIME,
+					    sizeof(time_val), time_val);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n",
+		       (int)(time_val[0]) + (int)(time_val[1] << 8));
+}
+
+static ssize_t reset_reason_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_RESET_REASON);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "0x%02x\n", ret);
+}
+
+static ssize_t critical_action_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int ret;
+	const char *action;
+
+	ret = i2c_smbus_read_byte_data(mcu->client, REG_CRITICAL_ACTIONS);
+	if (ret < 0)
+		return ret;
+
+	if (ret == CRITICAL_ACTION_REBOOT)
+		action = "reboot";
+	else if (ret == CRITICAL_ACTION_POWEROFF)
+		action = "poweroff";
+	else
+		action = "unknown";
+
+	return sprintf(buf, "%s\n", action);
+}
+
+static ssize_t critical_action_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	u8 value;
+	int ret;
+
+	if (sysfs_streq("reboot", buf))
+		value = CRITICAL_ACTION_REBOOT;
+	else if (sysfs_streq("poweroff", buf))
+		value = CRITICAL_ACTION_POWEROFF;
+	else
+		return -EINVAL;
+
+	mutex_lock(&mcu->mutex);
+	ret = i2c_smbus_write_byte_data(mcu->client,
+					REG_CRITICAL_ACTIONS, value);
+	mutex_unlock(&mcu->mutex);
+
+	if (ret < 0)
+		return ret;
+	return count;
+}
+
+static DEVICE_ATTR_RO(reset_count);
+static DEVICE_ATTR_RO(uptime);
+static DEVICE_ATTR_RO(reset_reason);
+static DEVICE_ATTR_RW(critical_action);
+
+DEFINE_MCU_DEBUG_ATTR(firmware_version, REG_MCU_FIRMWARE_VERSION, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(pcb_version, REG_PCB_VERSION, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(board_type, REG_BOARD_TYPE, "0x%02x");
+DEFINE_MCU_DEBUG_ATTR(mcu_type, REG_MCU_TYPE, "%d");
+
+static struct attribute *sg2042_mcu_attrs[] = {
+	&dev_attr_reset_count.attr,
+	&dev_attr_uptime.attr,
+	&dev_attr_reset_reason.attr,
+	&dev_attr_critical_action.attr,
+	NULL
+};
+
+static const struct attribute_group sg2042_mcu_attr_group = {
+	.attrs	= sg2042_mcu_attrs,
+};
+
+static const struct attribute_group *sg2042_mcu_groups[] = {
+	&sg2042_mcu_attr_group,
+	NULL
+};
+
+static const struct hwmon_channel_info * const sg2042_mcu_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_CRIT |
+					HWMON_T_CRIT_HYST,
+				 HWMON_T_INPUT),
+	NULL
+};
+
+static int sg2042_mcu_read_temp(struct device *dev,
+				u32 attr, int channel,
+				long *val)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int tmp;
+	u8 reg;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = channel ? REG_BOARD_TEMP : REG_SOC_TEMP;
+		break;
+	case hwmon_temp_crit:
+		reg = REG_CRITICAL_TEMP;
+		break;
+	case hwmon_temp_crit_hyst:
+		reg = REG_REPOWER_TEMP;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	tmp = i2c_smbus_read_byte_data(mcu->client, reg);
+	if (tmp < 0)
+		return tmp;
+	*val = tmp * 1000;
+
+	return 0;
+}
+
+static int sg2042_mcu_read(struct device *dev,
+			   enum hwmon_sensor_types type,
+			   u32 attr, int channel, long *val)
+{
+	return sg2042_mcu_read_temp(dev, attr, channel, val);
+}
+
+static int sg2042_mcu_write(struct device *dev,
+			    enum hwmon_sensor_types type,
+			    u32 attr, int channel, long val)
+{
+	struct sg2042_mcu_data *mcu = dev_get_drvdata(dev);
+	int temp = val / 1000;
+	int hyst_temp, crit_temp;
+	int ret;
+	u8 reg;
+
+	if (temp > MCU_POWER_MAX)
+		temp = MCU_POWER_MAX;
+
+	mutex_lock(&mcu->mutex);
+
+	switch (attr) {
+	case hwmon_temp_crit:
+		hyst_temp = i2c_smbus_read_byte_data(mcu->client,
+						     REG_REPOWER_TEMP);
+		if (hyst_temp < 0) {
+			ret = -ENODEV;
+			goto failed;
+		}
+
+		crit_temp = temp;
+		reg = REG_CRITICAL_TEMP;
+		break;
+	case hwmon_temp_crit_hyst:
+		crit_temp = i2c_smbus_read_byte_data(mcu->client,
+						     REG_CRITICAL_TEMP);
+		if (crit_temp < 0) {
+			ret = -ENODEV;
+			goto failed;
+		}
+
+		hyst_temp = temp;
+		reg = REG_REPOWER_TEMP;
+		break;
+	default:
+		mutex_unlock(&mcu->mutex);
+		return -EOPNOTSUPP;
+	}
+
+	if (crit_temp < hyst_temp) {
+		ret = -EINVAL;
+		goto failed;
+	}
+
+	ret = i2c_smbus_write_byte_data(mcu->client, reg, temp);
+
+failed:
+	mutex_unlock(&mcu->mutex);
+	return ret;
+}
+
+static umode_t sg2042_mcu_is_visible(const void *_data,
+				     enum hwmon_sensor_types type,
+				     u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return 0444;
+		case hwmon_temp_crit:
+		case hwmon_temp_crit_hyst:
+			if (channel == 0)
+				return 0664;
+			break;
+		default:
+			return 0;
+		}
+		break;
+	default:
+		return 0;
+	}
+	return 0;
+}
+
+static const struct hwmon_ops sg2042_mcu_ops = {
+	.is_visible = sg2042_mcu_is_visible,
+	.read = sg2042_mcu_read,
+	.write = sg2042_mcu_write,
+};
+
+static const struct hwmon_chip_info sg2042_mcu_chip_info = {
+	.ops = &sg2042_mcu_ops,
+	.info = sg2042_mcu_info,
+};
+
+static void sg2042_mcu_debugfs_init(struct sg2042_mcu_data *mcu,
+				    struct device *dev)
+{
+	mcu->debugfs = debugfs_create_dir(dev_name(dev), sgmcu_debugfs);
+	if (mcu->debugfs) {
+		debugfs_create_file("firmware_version", 0444, mcu->debugfs,
+				    mcu, &firmware_version_fops);
+		debugfs_create_file("pcb_version", 0444, mcu->debugfs, mcu,
+				    &pcb_version_fops);
+		debugfs_create_file("mcu_type", 0444, mcu->debugfs, mcu,
+				    &mcu_type_fops);
+		debugfs_create_file("board_type", 0444, mcu->debugfs, mcu,
+				    &board_type_fops);
+	}
+}
+
+static int sg2042_mcu_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct sg2042_mcu_data *mcu;
+	struct device *hwmon_dev;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+						I2C_FUNC_SMBUS_BLOCK_DATA))
+		return -EIO;
+
+	mcu = devm_kmalloc(dev, sizeof(*mcu), GFP_KERNEL);
+	if (!mcu)
+		return -ENOMEM;
+
+	mutex_init(&mcu->mutex);
+	mcu->client = client;
+
+	i2c_set_clientdata(client, mcu);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "sg2042_mcu",
+							 mcu,
+							 &sg2042_mcu_chip_info,
+							 NULL);
+	if (IS_ERR_OR_NULL(hwmon_dev))
+		return -EFAULT;
+
+	sg2042_mcu_debugfs_init(mcu, dev);
+
+	return 0;
+}
+
+static void sg2042_mcu_i2c_remove(struct i2c_client *client)
+{
+	struct sg2042_mcu_data *mcu = i2c_get_clientdata(client);
+
+	debugfs_remove_recursive(mcu->debugfs);
+}
+
+static const struct i2c_device_id sg2042_mcu_id[] = {
+	{ "sg2042-hwmon-mcu", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, sg2042_mcu_id);
+
+static const struct of_device_id sg2042_mcu_of_id[] = {
+	{ .compatible = "sophgo,sg2042-hwmon-mcu" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sg2042_mcu_of_id);
+
+static struct i2c_driver sg2042_mcu_driver = {
+	.driver = {
+		.name = "sg2042-mcu",
+		.of_match_table = sg2042_mcu_of_id,
+		.dev_groups = sg2042_mcu_groups,
+	},
+	.probe = sg2042_mcu_i2c_probe,
+	.remove = sg2042_mcu_i2c_remove,
+	.id_table = sg2042_mcu_id,
+};
+
+static int __init sg2042_mcu_init(void)
+{
+	sgmcu_debugfs = debugfs_create_dir("sg2042-mcu", NULL);
+	return i2c_add_driver(&sg2042_mcu_driver);
+}
+
+static void __exit sg2042_mcu_exit(void)
+{
+	debugfs_remove_recursive(sgmcu_debugfs);
+	i2c_del_driver(&sg2042_mcu_driver);
+}
+
+module_init(sg2042_mcu_init);
+module_exit(sg2042_mcu_exit);
+
+MODULE_AUTHOR("Inochi Amaoto <[e-mail]>");
+MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform");
+MODULE_LICENSE("GPL");