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