| Message ID | IA1PR20MB4953032E1B5D58D2F6C7393DBBB32@IA1PR20MB4953.namprd20.prod.outlook.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: sophgo: Add SG2042 external hardware monitor support | expand |
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 <inochiama@outlook.com> > Tested-by: Chen Wang <unicorn_wang@outlook.com> > Reviewed-by: Chen Wang <unicorn_wang@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 | 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 <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..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 <inochiama@outlook.com> > + * > + * 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 <inochiama@outlook.com>"); > +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform"); > +MODULE_LICENSE("GPL");
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 <inochiama@outlook.com> > > Tested-by: Chen Wang <unicorn_wang@outlook.com> > > Reviewed-by: Chen Wang <unicorn_wang@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 | 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 <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..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 <inochiama@outlook.com> > > + * > > + * 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 <inochiama@outlook.com>"); > > +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform"); > > +MODULE_LICENSE("GPL"); >
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 <inochiama@outlook.com> >>> Tested-by: Chen Wang <unicorn_wang@outlook.com> >>> Reviewed-by: Chen Wang <unicorn_wang@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 | 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 <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..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 <inochiama@outlook.com> >>> + * >>> + * 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 <inochiama@outlook.com>"); >>> +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform"); >>> +MODULE_LICENSE("GPL"); >>
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 <inochiama@outlook.com> > > > > Tested-by: Chen Wang <unicorn_wang@outlook.com> > > > > Reviewed-by: Chen Wang <unicorn_wang@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 | 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 <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..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 <inochiama@outlook.com> > > > > + * > > > > + * 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 <inochiama@outlook.com>"); > > > > +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform"); > > > > +MODULE_LICENSE("GPL"); > > > >
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 <inochiama@outlook.com> > Tested-by: Chen Wang <unicorn_wang@outlook.com> > Reviewed-by: Chen Wang <unicorn_wang@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 | 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 <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..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 <inochiama@outlook.com> > + * > + * 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 <inochiama@outlook.com>"); > +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform"); > +MODULE_LICENSE("GPL");
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 <inochiama@outlook.com> > > Tested-by: Chen Wang <unicorn_wang@outlook.com> > > Reviewed-by: Chen Wang <unicorn_wang@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 | 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 <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..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 <inochiama@outlook.com> > > + * > > + * 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
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..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 <inochiama@outlook.com> + * + * 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 <inochiama@outlook.com>"); +MODULE_DESCRIPTION("MCU I2C driver for SG2042 soc platform"); +MODULE_LICENSE("GPL");