| Message ID | 20241024085922.133071-7-tmyu0@nuvoton.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add Nuvoton NCT6694 MFD devices | expand |
On Thu, Oct 24, 2024 at 2:33 PM Ming Yu <a0282524688@gmail.com> wrote: > > This driver supports Hardware monitor functionality for NCT6694 MFD > device based on USB interface. > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > --- > MAINTAINERS | 1 + > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/nct6694-hwmon.c | 407 ++++++++++++++++++++++++++++++++++ > 4 files changed, 419 insertions(+) > create mode 100644 drivers/hwmon/nct6694-hwmon.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 63387c0d4ab6..2aa87ad84156 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16439,6 +16439,7 @@ M: Ming Yu <tmyu0@nuvoton.com> > L: linux-kernel@vger.kernel.org > S: Supported > F: drivers/gpio/gpio-nct6694.c > +F: drivers/hwmon/nct6694-hwmon.c > F: drivers/i2c/busses/i2c-nct6694.c > F: drivers/mfd/nct6694.c > F: drivers/net/can/nct6694_canfd.c > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 08a3c863f80a..740e4afe6582 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1625,6 +1625,16 @@ config SENSORS_NCT6683 > This driver can also be built as a module. If so, the module > will be called nct6683. > > +config SENSORS_NCT6694 > + tristate "Nuvoton NCT6694 Hardware Monitor support" > + depends on MFD_NCT6694 > + help > + Say Y here to support Nuvoton NCT6694 hardware monitoring > + functionality. > + > + This driver can also be built as a module. If so, the module > + will be called nct6694-hwmon. > + > config SENSORS_NCT6775_CORE > tristate > select REGMAP > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 9554d2fdcf7b..729961176d00 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o > obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o > obj-$(CONFIG_SENSORS_MR75203) += mr75203.o > obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o > +obj-$(CONFIG_SENSORS_NCT6694) += nct6694-hwmon.o > obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o > nct6775-objs := nct6775-platform.o > obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o > diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c > new file mode 100644 > index 000000000000..7d7d22a650b0 > --- /dev/null > +++ b/drivers/hwmon/nct6694-hwmon.c > @@ -0,0 +1,407 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Nuvoton NCT6694 HWMON driver based on USB interface. > + * > + * Copyright (C) 2024 Nuvoton Technology Corp. > + */ > + > +#include <linux/slab.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/hwmon.h> > +#include <linux/platform_device.h> > +#include <linux/mfd/nct6694.h> > + > +#define DRVNAME "nct6694-hwmon" > + > +/* Host interface */ > +#define REQUEST_RPT_MOD 0xFF > +#define REQUEST_HWMON_MOD 0x00 > + > +/* Report Channel */ > +#define HWMON_FIN_IDX(x) (0x50 + ((x) * 2)) > +#define HWMON_FIN_STS(x) (0x6E + (x)) > +#define HWMON_PWM_IDX(x) (0x70 + (x)) > + > +/* Message Channel*/ > +/* Command 00h */ > +#define REQUEST_HWMON_CMD0_LEN 0x40 > +#define REQUEST_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ > +#define HWMON_FIN_EN(x) (0x04 + (x)) > +#define HWMON_PWM_FREQ_IDX(x) (0x30 + (x)) > +/* Command 02h */ > +#define REQUEST_HWMON_CMD2_LEN 0x90 > +#define REQUEST_HWMON_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ > +#define HWMON_SMI_CTRL_IDX 0x00 > +#define HWMON_FIN_LIMIT_IDX(x) (0x70 + ((x) * 2)) > +#define HWMON_CMD2_HYST_MASK 0x1F > +/* Command 03h */ > +#define REQUEST_HWMON_CMD3_LEN 0x08 > +#define REQUEST_HWMON_CMD3_OFFSET 0x0003 /* OFFSET = SEL|CMD */ > + > +struct nct6694_hwmon_data { > + struct nct6694 *nct6694; > + > + /* Make sure read & write commands are consecutive */ > + struct mutex hwmon_lock; > +}; > + > +#define NCT6694_HWMON_FAN_CONFIG (HWMON_F_ENABLE | HWMON_F_INPUT | \ > + HWMON_F_MIN | HWMON_F_MIN_ALARM) > +#define NCT6694_HWMON_PWM_CONFIG (HWMON_PWM_INPUT | HWMON_PWM_FREQ) > + > +static const struct hwmon_channel_info *nct6694_info[] = { > + HWMON_CHANNEL_INFO(fan, > + NCT6694_HWMON_FAN_CONFIG, /* FIN0 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN1 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN2 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN3 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN4 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN5 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN6 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN7 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN8 */ > + NCT6694_HWMON_FAN_CONFIG), /* FIN9 */ > + > + HWMON_CHANNEL_INFO(pwm, > + NCT6694_HWMON_PWM_CONFIG, /* PWM0 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM1 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM2 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM3 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM4 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM5 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM6 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM7 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM8 */ > + NCT6694_HWMON_PWM_CONFIG), /* PWM9 */ > + NULL > +}; > + > +static int nct6694_fan_read(struct device *dev, u32 attr, int channel, > + long *val) > +{ > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > + unsigned char buf[2]; > + int ret; > + > + switch (attr) { > + case hwmon_fan_enable: > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD0_OFFSET, > + REQUEST_HWMON_CMD0_LEN, > + HWMON_FIN_EN(channel / 8), > + 1, buf); > + if (ret) > + return -EINVAL; > + > + *val = buf[0] & BIT(channel % 8) ? 1 : 0; > + > + break; > + > + case hwmon_fan_input: > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > + HWMON_FIN_IDX(channel), 2, 0, > + 2, buf); > + if (ret) > + return -EINVAL; > + > + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > + > + break; > + > + case hwmon_fan_min: > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, > + HWMON_FIN_LIMIT_IDX(channel), > + 2, buf); > + if (ret) > + return -EINVAL; > + > + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > + > + break; > + > + case hwmon_fan_min_alarm: > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > + HWMON_FIN_STS(channel / 8), > + 1, 0, 1, buf); > + if (ret) > + return -EINVAL; > + > + *val = buf[0] & BIT(channel % 8); > + > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int nct6694_pwm_read(struct device *dev, u32 attr, int channel, > + long *val) > +{ > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > + unsigned char buf; > + int ret; > + > + switch (attr) { > + case hwmon_pwm_input: > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > + HWMON_PWM_IDX(channel), > + 1, 0, 1, &buf); > + if (ret) > + return -EINVAL; > + > + *val = buf; > + > + break; > + case hwmon_pwm_freq: > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD0_OFFSET, > + REQUEST_HWMON_CMD0_LEN, > + HWMON_PWM_FREQ_IDX(channel), > + 1, &buf); > + if (ret) > + return -EINVAL; > + > + *val = buf * 25000 / 255; > + > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > + long val) > +{ > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; [Kalesh] Please try to maintain RCT order for variable declaration > + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > + u16 fan_val = (u16)val; > + int ret; > + > + switch (attr) { > + case hwmon_fan_enable: > + mutex_lock(&data->hwmon_lock); > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD0_OFFSET, > + REQUEST_HWMON_CMD0_LEN, 0, > + REQUEST_HWMON_CMD0_LEN, > + enable_buf); > + if (ret) > + goto err; > + > + if (val) > + enable_buf[HWMON_FIN_EN(channel / 8)] |= BIT(channel % 8); > + else > + enable_buf[HWMON_FIN_EN(channel / 8)] &= ~BIT(channel % 8); > + > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD0_OFFSET, > + REQUEST_HWMON_CMD0_LEN, enable_buf); > + if (ret) > + goto err; > + > + break; > + > + case hwmon_fan_min: > + mutex_lock(&data->hwmon_lock); > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, 0, > + REQUEST_HWMON_CMD2_LEN, buf); > + if (ret) > + goto err; > + > + buf[HWMON_FIN_LIMIT_IDX(channel)] = (u8)((fan_val >> 8) & 0xFF); > + buf[HWMON_FIN_LIMIT_IDX(channel) + 1] = (u8)(fan_val & 0xFF); > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, buf); > + if (ret) > + goto err; > + > + break; > + > + default: > + ret = -EOPNOTSUPP; [Kalesh] If you initialize "ret = -EOPNOTSUPP;" during declararion, you can just break from here. > + goto err; > + } > + > +err: > + mutex_unlock(&data->hwmon_lock); > + return ret; > +} > + > +static int nct6694_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + switch (type) { > + case hwmon_fan: /* in RPM */ > + return nct6694_fan_read(dev, attr, channel, val); > + > + case hwmon_pwm: /* in value 0~255 */ > + return nct6694_pwm_read(dev, attr, channel, val); > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int nct6694_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + switch (type) { > + case hwmon_fan: > + return nct6694_fan_write(dev, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } [Kalesh] You can use simple if condition here than a switch like: if (type != hwmon_fan) return -EOPNOTSUPP; return nct6694_fan_write(dev, attr, channel, val); > + > + return 0; > +} > + > +static umode_t nct6694_is_visible(const void *data, enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_fan: > + switch (attr) { > + case hwmon_fan_enable: > + case hwmon_fan_min: > + return 0644; [Kalesh] I think there is no need to leave a new line in between cases > + > + case hwmon_fan_input: > + case hwmon_fan_min_alarm: > + return 0444; > + > + default: > + return 0; > + } > + > + case hwmon_pwm: > + switch (attr) { > + case hwmon_pwm_input: > + case hwmon_pwm_freq: > + return 0444; > + default: > + return 0; > + } > + > + default: > + return 0; > + } > + > + return 0; [Kalesh] This return statement looks redundant as the execution never reaches here. Same comment applies to other functions above as well. > +} > + > +static const struct hwmon_ops nct6694_hwmon_ops = { > + .is_visible = nct6694_is_visible, > + .read = nct6694_read, > + .write = nct6694_write, > +}; > + > +static const struct hwmon_chip_info nct6694_chip_info = { > + .ops = &nct6694_hwmon_ops, > + .info = nct6694_info, > +}; > + > +static int nct6694_hwmon_init(struct nct6694_hwmon_data *data) > +{ > + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > + int ret; > + > + /* Set Fan input Real Time alarm mode */ > + mutex_lock(&data->hwmon_lock); > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, 0, > + REQUEST_HWMON_CMD2_LEN, buf); > + if (ret) > + goto err; [Kalesh] It would be better to rename the label as "unlock". Same comment on other functions as well. > + > + buf[HWMON_SMI_CTRL_IDX] = 0x02; > + > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, buf); > + if (ret) > + goto err; > + > +err: > + mutex_unlock(&data->hwmon_lock); > + return ret; > +} > + > +static int nct6694_hwmon_probe(struct platform_device *pdev) > +{ > + struct nct6694_hwmon_data *data; > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); > + struct device *hwmon_dev; > + int ret; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->nct6694 = nct6694; > + mutex_init(&data->hwmon_lock); > + platform_set_drvdata(pdev, data); > + > + ret = nct6694_hwmon_init(data); > + if (ret) > + return -EIO; > + > + /* Register hwmon device to HWMON framework */ > + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > + "nct6694", data, > + &nct6694_chip_info, > + NULL); > + if (IS_ERR(hwmon_dev)) { > + dev_err(&pdev->dev, "%s: Failed to register hwmon device!\n", > + __func__); > + return PTR_ERR(hwmon_dev); > + } > + > + return 0; > +} > + > +static struct platform_driver nct6694_hwmon_driver = { > + .driver = { > + .name = DRVNAME, > + }, > + .probe = nct6694_hwmon_probe, > +}; > + > +static int __init nct6694_init(void) > +{ > + int err; > + > + err = platform_driver_register(&nct6694_hwmon_driver); > + if (!err) { > + if (err) [Kalesh] This whole check looks strange. You can simplify this function as: return platform_driver_register(&nct6694_hwmon_driver); > + platform_driver_unregister(&nct6694_hwmon_driver); > + } > + > + return err; > +} > +subsys_initcall(nct6694_init); > + > +static void __exit nct6694_exit(void) > +{ > + platform_driver_unregister(&nct6694_hwmon_driver); > +} > +module_exit(nct6694_exit); > + > +MODULE_DESCRIPTION("USB-HWMON driver for NCT6694"); > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); > +MODULE_LICENSE("GPL"); > -- > 2.34.1 > >
On 10/24/24 02:20, Kalesh Anakkur Purayil wrote: > On Thu, Oct 24, 2024 at 2:33 PM Ming Yu <a0282524688@gmail.com> wrote: >> >> This driver supports Hardware monitor functionality for NCT6694 MFD >> device based on USB interface. >> >> Signed-off-by: Ming Yu <tmyu0@nuvoton.com> >> --- >> MAINTAINERS | 1 + >> drivers/hwmon/Kconfig | 10 + >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/nct6694-hwmon.c | 407 ++++++++++++++++++++++++++++++++++ >> 4 files changed, 419 insertions(+) >> create mode 100644 drivers/hwmon/nct6694-hwmon.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 63387c0d4ab6..2aa87ad84156 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -16439,6 +16439,7 @@ M: Ming Yu <tmyu0@nuvoton.com> >> L: linux-kernel@vger.kernel.org >> S: Supported >> F: drivers/gpio/gpio-nct6694.c >> +F: drivers/hwmon/nct6694-hwmon.c >> F: drivers/i2c/busses/i2c-nct6694.c >> F: drivers/mfd/nct6694.c >> F: drivers/net/can/nct6694_canfd.c >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index 08a3c863f80a..740e4afe6582 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -1625,6 +1625,16 @@ config SENSORS_NCT6683 >> This driver can also be built as a module. If so, the module >> will be called nct6683. >> >> +config SENSORS_NCT6694 >> + tristate "Nuvoton NCT6694 Hardware Monitor support" >> + depends on MFD_NCT6694 >> + help >> + Say Y here to support Nuvoton NCT6694 hardware monitoring >> + functionality. >> + >> + This driver can also be built as a module. If so, the module >> + will be called nct6694-hwmon. >> + >> config SENSORS_NCT6775_CORE >> tristate >> select REGMAP >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index 9554d2fdcf7b..729961176d00 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o >> obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o >> obj-$(CONFIG_SENSORS_MR75203) += mr75203.o >> obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o >> +obj-$(CONFIG_SENSORS_NCT6694) += nct6694-hwmon.o >> obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o >> nct6775-objs := nct6775-platform.o >> obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o >> diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c >> new file mode 100644 >> index 000000000000..7d7d22a650b0 >> --- /dev/null >> +++ b/drivers/hwmon/nct6694-hwmon.c >> @@ -0,0 +1,407 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Nuvoton NCT6694 HWMON driver based on USB interface. >> + * >> + * Copyright (C) 2024 Nuvoton Technology Corp. >> + */ >> + >> +#include <linux/slab.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/hwmon.h> >> +#include <linux/platform_device.h> >> +#include <linux/mfd/nct6694.h> >> + >> +#define DRVNAME "nct6694-hwmon" >> + >> +/* Host interface */ >> +#define REQUEST_RPT_MOD 0xFF >> +#define REQUEST_HWMON_MOD 0x00 >> + >> +/* Report Channel */ >> +#define HWMON_FIN_IDX(x) (0x50 + ((x) * 2)) >> +#define HWMON_FIN_STS(x) (0x6E + (x)) >> +#define HWMON_PWM_IDX(x) (0x70 + (x)) >> + >> +/* Message Channel*/ >> +/* Command 00h */ >> +#define REQUEST_HWMON_CMD0_LEN 0x40 >> +#define REQUEST_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ >> +#define HWMON_FIN_EN(x) (0x04 + (x)) >> +#define HWMON_PWM_FREQ_IDX(x) (0x30 + (x)) >> +/* Command 02h */ >> +#define REQUEST_HWMON_CMD2_LEN 0x90 >> +#define REQUEST_HWMON_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ >> +#define HWMON_SMI_CTRL_IDX 0x00 >> +#define HWMON_FIN_LIMIT_IDX(x) (0x70 + ((x) * 2)) >> +#define HWMON_CMD2_HYST_MASK 0x1F >> +/* Command 03h */ >> +#define REQUEST_HWMON_CMD3_LEN 0x08 >> +#define REQUEST_HWMON_CMD3_OFFSET 0x0003 /* OFFSET = SEL|CMD */ >> + >> +struct nct6694_hwmon_data { >> + struct nct6694 *nct6694; >> + >> + /* Make sure read & write commands are consecutive */ >> + struct mutex hwmon_lock; >> +}; >> + >> +#define NCT6694_HWMON_FAN_CONFIG (HWMON_F_ENABLE | HWMON_F_INPUT | \ >> + HWMON_F_MIN | HWMON_F_MIN_ALARM) >> +#define NCT6694_HWMON_PWM_CONFIG (HWMON_PWM_INPUT | HWMON_PWM_FREQ) >> + >> +static const struct hwmon_channel_info *nct6694_info[] = { >> + HWMON_CHANNEL_INFO(fan, >> + NCT6694_HWMON_FAN_CONFIG, /* FIN0 */ >> + NCT6694_HWMON_FAN_CONFIG, /* FIN1 */ >> + NCT6694_HWMON_FAN_CONFIG, /* FIN2 */ >> + NCT6694_HWMON_FAN_CONFIG, /* FIN3 */ >> + NCT6694_HWMON_FAN_CONFIG, /* FIN4 */ >> + NCT6694_HWMON_FAN_CONFIG, /* FIN5 */ >> + NCT6694_HWMON_FAN_CONFIG, /* FIN6 */ >> + NCT6694_HWMON_FAN_CONFIG, /* FIN7 */ >> + NCT6694_HWMON_FAN_CONFIG, /* FIN8 */ >> + NCT6694_HWMON_FAN_CONFIG), /* FIN9 */ >> + >> + HWMON_CHANNEL_INFO(pwm, >> + NCT6694_HWMON_PWM_CONFIG, /* PWM0 */ >> + NCT6694_HWMON_PWM_CONFIG, /* PWM1 */ >> + NCT6694_HWMON_PWM_CONFIG, /* PWM2 */ >> + NCT6694_HWMON_PWM_CONFIG, /* PWM3 */ >> + NCT6694_HWMON_PWM_CONFIG, /* PWM4 */ >> + NCT6694_HWMON_PWM_CONFIG, /* PWM5 */ >> + NCT6694_HWMON_PWM_CONFIG, /* PWM6 */ >> + NCT6694_HWMON_PWM_CONFIG, /* PWM7 */ >> + NCT6694_HWMON_PWM_CONFIG, /* PWM8 */ >> + NCT6694_HWMON_PWM_CONFIG), /* PWM9 */ >> + NULL >> +}; >> + >> +static int nct6694_fan_read(struct device *dev, u32 attr, int channel, >> + long *val) >> +{ >> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); >> + unsigned char buf[2]; >> + int ret; >> + >> + switch (attr) { >> + case hwmon_fan_enable: >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD0_OFFSET, >> + REQUEST_HWMON_CMD0_LEN, >> + HWMON_FIN_EN(channel / 8), >> + 1, buf); >> + if (ret) >> + return -EINVAL; >> + >> + *val = buf[0] & BIT(channel % 8) ? 1 : 0; >> + >> + break; >> + >> + case hwmon_fan_input: >> + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, >> + HWMON_FIN_IDX(channel), 2, 0, >> + 2, buf); >> + if (ret) >> + return -EINVAL; >> + >> + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; >> + >> + break; >> + >> + case hwmon_fan_min: >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD2_OFFSET, >> + REQUEST_HWMON_CMD2_LEN, >> + HWMON_FIN_LIMIT_IDX(channel), >> + 2, buf); >> + if (ret) >> + return -EINVAL; >> + >> + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; >> + >> + break; >> + >> + case hwmon_fan_min_alarm: >> + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, >> + HWMON_FIN_STS(channel / 8), >> + 1, 0, 1, buf); >> + if (ret) >> + return -EINVAL; >> + >> + *val = buf[0] & BIT(channel % 8); >> + >> + break; >> + >> + default: >> + return -EOPNOTSUPP; >> + } >> + >> + return 0; >> +} >> + >> +static int nct6694_pwm_read(struct device *dev, u32 attr, int channel, >> + long *val) >> +{ >> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); >> + unsigned char buf; >> + int ret; >> + >> + switch (attr) { >> + case hwmon_pwm_input: >> + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, >> + HWMON_PWM_IDX(channel), >> + 1, 0, 1, &buf); >> + if (ret) >> + return -EINVAL; >> + >> + *val = buf; >> + >> + break; >> + case hwmon_pwm_freq: >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD0_OFFSET, >> + REQUEST_HWMON_CMD0_LEN, >> + HWMON_PWM_FREQ_IDX(channel), >> + 1, &buf); >> + if (ret) >> + return -EINVAL; >> + >> + *val = buf * 25000 / 255; >> + >> + break; >> + >> + default: >> + return -EOPNOTSUPP; >> + } >> + >> + return 0; >> +} >> + >> +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, >> + long val) >> +{ >> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); >> + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > [Kalesh] Please try to maintain RCT order for variable declaration Ok, but that is already the case here ? >> + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; >> + u16 fan_val = (u16)val; >> + int ret; >> + >> + switch (attr) { >> + case hwmon_fan_enable: >> + mutex_lock(&data->hwmon_lock); >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD0_OFFSET, >> + REQUEST_HWMON_CMD0_LEN, 0, >> + REQUEST_HWMON_CMD0_LEN, >> + enable_buf); >> + if (ret) >> + goto err; >> + >> + if (val) >> + enable_buf[HWMON_FIN_EN(channel / 8)] |= BIT(channel % 8); >> + else >> + enable_buf[HWMON_FIN_EN(channel / 8)] &= ~BIT(channel % 8); >> + >> + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD0_OFFSET, >> + REQUEST_HWMON_CMD0_LEN, enable_buf); >> + if (ret) >> + goto err; >> + >> + break; >> + >> + case hwmon_fan_min: >> + mutex_lock(&data->hwmon_lock); >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD2_OFFSET, >> + REQUEST_HWMON_CMD2_LEN, 0, >> + REQUEST_HWMON_CMD2_LEN, buf); >> + if (ret) >> + goto err; >> + >> + buf[HWMON_FIN_LIMIT_IDX(channel)] = (u8)((fan_val >> 8) & 0xFF); >> + buf[HWMON_FIN_LIMIT_IDX(channel) + 1] = (u8)(fan_val & 0xFF); >> + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD2_OFFSET, >> + REQUEST_HWMON_CMD2_LEN, buf); >> + if (ret) >> + goto err; >> + >> + break; >> + >> + default: >> + ret = -EOPNOTSUPP; > [Kalesh] If you initialize "ret = -EOPNOTSUPP;" during declararion, > you can just break from here. You are missing the point. The lock wasn't acquired here in the first place. It is conceptually wrong to acquire a lock in the switch statement and release it outside. This patch is a case in point. >> + goto err; >> + } >> + >> +err: >> + mutex_unlock(&data->hwmon_lock); >> + return ret; >> +} >> + >> +static int nct6694_read(struct device *dev, enum hwmon_sensor_types type, >> + u32 attr, int channel, long *val) >> +{ >> + switch (type) { >> + case hwmon_fan: /* in RPM */ >> + return nct6694_fan_read(dev, attr, channel, val); >> + >> + case hwmon_pwm: /* in value 0~255 */ >> + return nct6694_pwm_read(dev, attr, channel, val); >> + >> + default: >> + return -EOPNOTSUPP; >> + } >> + >> + return 0; >> +} >> + >> +static int nct6694_write(struct device *dev, enum hwmon_sensor_types type, >> + u32 attr, int channel, long val) >> +{ >> + switch (type) { >> + case hwmon_fan: >> + return nct6694_fan_write(dev, attr, channel, val); >> + default: >> + return -EOPNOTSUPP; >> + } > [Kalesh] You can use simple if condition here than a switch like: > if (type != hwmon_fan) > return -EOPNOTSUPP; > return nct6694_fan_write(dev, attr, channel, val); That is a bit POV. I'd leave that to the developer. More important is that the return statements after the switch are unnecessary and never reached if each case returns immediately. >> + >> + return 0; >> +} >> + >> +static umode_t nct6694_is_visible(const void *data, enum hwmon_sensor_types type, >> + u32 attr, int channel) >> +{ >> + switch (type) { >> + case hwmon_fan: >> + switch (attr) { >> + case hwmon_fan_enable: >> + case hwmon_fan_min: >> + return 0644; > [Kalesh] I think there is no need to leave a new line in between cases >> + >> + case hwmon_fan_input: >> + case hwmon_fan_min_alarm: >> + return 0444; >> + >> + default: >> + return 0; >> + } >> + >> + case hwmon_pwm: >> + switch (attr) { >> + case hwmon_pwm_input: >> + case hwmon_pwm_freq: >> + return 0444; >> + default: >> + return 0; >> + } >> + >> + default: >> + return 0; >> + } >> + >> + return 0; > [Kalesh] This return statement looks redundant as the execution never > reaches here. Same comment applies to other functions above as well. >> +} >> + >> +static const struct hwmon_ops nct6694_hwmon_ops = { >> + .is_visible = nct6694_is_visible, >> + .read = nct6694_read, >> + .write = nct6694_write, >> +}; >> + >> +static const struct hwmon_chip_info nct6694_chip_info = { >> + .ops = &nct6694_hwmon_ops, >> + .info = nct6694_info, >> +}; >> + >> +static int nct6694_hwmon_init(struct nct6694_hwmon_data *data) >> +{ >> + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; >> + int ret; >> + >> + /* Set Fan input Real Time alarm mode */ >> + mutex_lock(&data->hwmon_lock); >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD2_OFFSET, >> + REQUEST_HWMON_CMD2_LEN, 0, >> + REQUEST_HWMON_CMD2_LEN, buf); >> + if (ret) >> + goto err; > [Kalesh] It would be better to rename the label as "unlock". Same > comment on other functions as well. The lock is not needed here in the first place. The function is called exactly once during initialization. >> + >> + buf[HWMON_SMI_CTRL_IDX] = 0x02; >> + >> + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, >> + REQUEST_HWMON_CMD2_OFFSET, >> + REQUEST_HWMON_CMD2_LEN, buf); >> + if (ret) >> + goto err; >> + >> +err: >> + mutex_unlock(&data->hwmon_lock); >> + return ret; >> +} >> + >> +static int nct6694_hwmon_probe(struct platform_device *pdev) >> +{ >> + struct nct6694_hwmon_data *data; >> + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); >> + struct device *hwmon_dev; >> + int ret; >> + >> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + data->nct6694 = nct6694; >> + mutex_init(&data->hwmon_lock); >> + platform_set_drvdata(pdev, data); >> + >> + ret = nct6694_hwmon_init(data); >> + if (ret) >> + return -EIO; >> + >> + /* Register hwmon device to HWMON framework */ >> + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, >> + "nct6694", data, >> + &nct6694_chip_info, >> + NULL); >> + if (IS_ERR(hwmon_dev)) { >> + dev_err(&pdev->dev, "%s: Failed to register hwmon device!\n", >> + __func__); >> + return PTR_ERR(hwmon_dev); >> + } >> + >> + return 0; >> +} >> + >> +static struct platform_driver nct6694_hwmon_driver = { >> + .driver = { >> + .name = DRVNAME, >> + }, >> + .probe = nct6694_hwmon_probe, >> +}; >> + >> +static int __init nct6694_init(void) >> +{ >> + int err; >> + >> + err = platform_driver_register(&nct6694_hwmon_driver); >> + if (!err) { >> + if (err) > [Kalesh] This whole check looks strange. You can simplify this function as: > return platform_driver_register(&nct6694_hwmon_driver); >> + platform_driver_unregister(&nct6694_hwmon_driver); >> + } >> + >> + return err; >> +} >> +subsys_initcall(nct6694_init); >> + >> +static void __exit nct6694_exit(void) >> +{ >> + platform_driver_unregister(&nct6694_hwmon_driver); >> +} >> +module_exit(nct6694_exit); >> + >> +MODULE_DESCRIPTION("USB-HWMON driver for NCT6694"); >> +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.34.1 >> >> > >
On 10/24/24 01:59, Ming Yu wrote: > This driver supports Hardware monitor functionality for NCT6694 MFD > device based on USB interface. > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > --- > MAINTAINERS | 1 + > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/nct6694-hwmon.c | 407 ++++++++++++++++++++++++++++++++++ > 4 files changed, 419 insertions(+) > create mode 100644 drivers/hwmon/nct6694-hwmon.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 63387c0d4ab6..2aa87ad84156 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16439,6 +16439,7 @@ M: Ming Yu <tmyu0@nuvoton.com> > L: linux-kernel@vger.kernel.org > S: Supported > F: drivers/gpio/gpio-nct6694.c > +F: drivers/hwmon/nct6694-hwmon.c > F: drivers/i2c/busses/i2c-nct6694.c > F: drivers/mfd/nct6694.c > F: drivers/net/can/nct6694_canfd.c > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 08a3c863f80a..740e4afe6582 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1625,6 +1625,16 @@ config SENSORS_NCT6683 > This driver can also be built as a module. If so, the module > will be called nct6683. > > +config SENSORS_NCT6694 > + tristate "Nuvoton NCT6694 Hardware Monitor support" > + depends on MFD_NCT6694 > + help > + Say Y here to support Nuvoton NCT6694 hardware monitoring > + functionality. > + > + This driver can also be built as a module. If so, the module > + will be called nct6694-hwmon. > + > config SENSORS_NCT6775_CORE > tristate > select REGMAP > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 9554d2fdcf7b..729961176d00 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o > obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o > obj-$(CONFIG_SENSORS_MR75203) += mr75203.o > obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o > +obj-$(CONFIG_SENSORS_NCT6694) += nct6694-hwmon.o > obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o > nct6775-objs := nct6775-platform.o > obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o > diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c > new file mode 100644 > index 000000000000..7d7d22a650b0 > --- /dev/null > +++ b/drivers/hwmon/nct6694-hwmon.c > @@ -0,0 +1,407 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Nuvoton NCT6694 HWMON driver based on USB interface. > + * > + * Copyright (C) 2024 Nuvoton Technology Corp. > + */ > + > +#include <linux/slab.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/hwmon.h> > +#include <linux/platform_device.h> > +#include <linux/mfd/nct6694.h> > + > +#define DRVNAME "nct6694-hwmon" > + > +/* Host interface */ > +#define REQUEST_RPT_MOD 0xFF > +#define REQUEST_HWMON_MOD 0x00 > + > +/* Report Channel */ > +#define HWMON_FIN_IDX(x) (0x50 + ((x) * 2)) > +#define HWMON_FIN_STS(x) (0x6E + (x)) > +#define HWMON_PWM_IDX(x) (0x70 + (x)) > + > +/* Message Channel*/ > +/* Command 00h */ > +#define REQUEST_HWMON_CMD0_LEN 0x40 > +#define REQUEST_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ > +#define HWMON_FIN_EN(x) (0x04 + (x)) > +#define HWMON_PWM_FREQ_IDX(x) (0x30 + (x)) > +/* Command 02h */ > +#define REQUEST_HWMON_CMD2_LEN 0x90 > +#define REQUEST_HWMON_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ > +#define HWMON_SMI_CTRL_IDX 0x00 > +#define HWMON_FIN_LIMIT_IDX(x) (0x70 + ((x) * 2)) > +#define HWMON_CMD2_HYST_MASK 0x1F > +/* Command 03h */ > +#define REQUEST_HWMON_CMD3_LEN 0x08 > +#define REQUEST_HWMON_CMD3_OFFSET 0x0003 /* OFFSET = SEL|CMD */ > + > +struct nct6694_hwmon_data { > + struct nct6694 *nct6694; > + > + /* Make sure read & write commands are consecutive */ > + struct mutex hwmon_lock; > +}; > + > +#define NCT6694_HWMON_FAN_CONFIG (HWMON_F_ENABLE | HWMON_F_INPUT | \ > + HWMON_F_MIN | HWMON_F_MIN_ALARM) > +#define NCT6694_HWMON_PWM_CONFIG (HWMON_PWM_INPUT | HWMON_PWM_FREQ) > + > +static const struct hwmon_channel_info *nct6694_info[] = { > + HWMON_CHANNEL_INFO(fan, > + NCT6694_HWMON_FAN_CONFIG, /* FIN0 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN1 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN2 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN3 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN4 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN5 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN6 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN7 */ > + NCT6694_HWMON_FAN_CONFIG, /* FIN8 */ > + NCT6694_HWMON_FAN_CONFIG), /* FIN9 */ > + > + HWMON_CHANNEL_INFO(pwm, > + NCT6694_HWMON_PWM_CONFIG, /* PWM0 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM1 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM2 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM3 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM4 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM5 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM6 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM7 */ > + NCT6694_HWMON_PWM_CONFIG, /* PWM8 */ > + NCT6694_HWMON_PWM_CONFIG), /* PWM9 */ > + NULL > +}; > + > +static int nct6694_fan_read(struct device *dev, u32 attr, int channel, > + long *val) > +{ > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > + unsigned char buf[2]; > + int ret; > + > + switch (attr) { > + case hwmon_fan_enable: > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD0_OFFSET, > + REQUEST_HWMON_CMD0_LEN, > + HWMON_FIN_EN(channel / 8), > + 1, buf); > + if (ret) > + return -EINVAL; Do not overwrite error return values. > + > + *val = buf[0] & BIT(channel % 8) ? 1 : 0; > + > + break; > + > + case hwmon_fan_input: > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > + HWMON_FIN_IDX(channel), 2, 0, > + 2, buf); > + if (ret) > + return -EINVAL; > + > + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > + > + break; > + > + case hwmon_fan_min: > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, > + HWMON_FIN_LIMIT_IDX(channel), > + 2, buf); > + if (ret) > + return -EINVAL; > + > + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > + > + break; > + > + case hwmon_fan_min_alarm: > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > + HWMON_FIN_STS(channel / 8), > + 1, 0, 1, buf); > + if (ret) > + return -EINVAL; > + > + *val = buf[0] & BIT(channel % 8); > + > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int nct6694_pwm_read(struct device *dev, u32 attr, int channel, > + long *val) > +{ > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > + unsigned char buf; > + int ret; > + > + switch (attr) { > + case hwmon_pwm_input: > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > + HWMON_PWM_IDX(channel), > + 1, 0, 1, &buf); > + if (ret) > + return -EINVAL; > + > + *val = buf; > + > + break; > + case hwmon_pwm_freq: > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD0_OFFSET, > + REQUEST_HWMON_CMD0_LEN, > + HWMON_PWM_FREQ_IDX(channel), > + 1, &buf); > + if (ret) > + return -EINVAL; > + > + *val = buf * 25000 / 255; > + > + break; > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > + long val) > +{ > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > + u16 fan_val = (u16)val; This is wrong. It will result in overflows/underflows if out of range values are provided, and the driver should not write 0 if the user writes 65536. You need to use clamp_val() instead. For values with well defined range (specifically hwmon_fan_enable) you need to validate the parameter, not just take it as boolean. > + int ret; > + > + switch (attr) { > + case hwmon_fan_enable: > + mutex_lock(&data->hwmon_lock); > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD0_OFFSET, > + REQUEST_HWMON_CMD0_LEN, 0, > + REQUEST_HWMON_CMD0_LEN, > + enable_buf); > + if (ret) > + goto err; > + > + if (val) > + enable_buf[HWMON_FIN_EN(channel / 8)] |= BIT(channel % 8); > + else > + enable_buf[HWMON_FIN_EN(channel / 8)] &= ~BIT(channel % 8); > + > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD0_OFFSET, > + REQUEST_HWMON_CMD0_LEN, enable_buf); > + if (ret) > + goto err; > + > + break; > + > + case hwmon_fan_min: > + mutex_lock(&data->hwmon_lock); > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, 0, > + REQUEST_HWMON_CMD2_LEN, buf); > + if (ret) > + goto err; > + > + buf[HWMON_FIN_LIMIT_IDX(channel)] = (u8)((fan_val >> 8) & 0xFF); > + buf[HWMON_FIN_LIMIT_IDX(channel) + 1] = (u8)(fan_val & 0xFF); > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, buf); > + if (ret) > + goto err; > + > + break; The error handler goto and the break accomplish exactly the same, thus the conditional goto is pointless. > + > + default: > + ret = -EOPNOTSUPP; > + goto err; As mentioned in my other reply, the lock is not acquired here, causing an unbalanced unlock. > + } > + > +err: > + mutex_unlock(&data->hwmon_lock); > + return ret; > +} > + > +static int nct6694_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + switch (type) { > + case hwmon_fan: /* in RPM */ > + return nct6694_fan_read(dev, attr, channel, val); > + > + case hwmon_pwm: /* in value 0~255 */ > + return nct6694_pwm_read(dev, attr, channel, val); > + > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; Unnecessary return statement. Also seen elsewhere. > +} > + > +static int nct6694_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + switch (type) { > + case hwmon_fan: > + return nct6694_fan_write(dev, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static umode_t nct6694_is_visible(const void *data, enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_fan: > + switch (attr) { > + case hwmon_fan_enable: > + case hwmon_fan_min: > + return 0644; > + > + case hwmon_fan_input: > + case hwmon_fan_min_alarm: > + return 0444; > + > + default: > + return 0; > + } > + > + case hwmon_pwm: > + switch (attr) { > + case hwmon_pwm_input: > + case hwmon_pwm_freq: > + return 0444; > + default: > + return 0; > + } > + > + default: > + return 0; > + } > + > + return 0; > +} > + > +static const struct hwmon_ops nct6694_hwmon_ops = { > + .is_visible = nct6694_is_visible, > + .read = nct6694_read, > + .write = nct6694_write, > +}; > + > +static const struct hwmon_chip_info nct6694_chip_info = { > + .ops = &nct6694_hwmon_ops, > + .info = nct6694_info, > +}; > + > +static int nct6694_hwmon_init(struct nct6694_hwmon_data *data) > +{ > + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > + int ret; > + > + /* Set Fan input Real Time alarm mode */ > + mutex_lock(&data->hwmon_lock); Pointless lock (this is init code) > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, 0, > + REQUEST_HWMON_CMD2_LEN, buf); > + if (ret) > + goto err; > + > + buf[HWMON_SMI_CTRL_IDX] = 0x02; > + > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > + REQUEST_HWMON_CMD2_OFFSET, > + REQUEST_HWMON_CMD2_LEN, buf); > + if (ret) > + goto err; > + > +err: > + mutex_unlock(&data->hwmon_lock); > + return ret; > +} > + > +static int nct6694_hwmon_probe(struct platform_device *pdev) > +{ > + struct nct6694_hwmon_data *data; > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); > + struct device *hwmon_dev; > + int ret; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->nct6694 = nct6694; > + mutex_init(&data->hwmon_lock); > + platform_set_drvdata(pdev, data); > + > + ret = nct6694_hwmon_init(data); > + if (ret) > + return -EIO; Again, do not overwrite error codes. > + > + /* Register hwmon device to HWMON framework */ > + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > + "nct6694", data, > + &nct6694_chip_info, > + NULL); > + if (IS_ERR(hwmon_dev)) { > + dev_err(&pdev->dev, "%s: Failed to register hwmon device!\n", > + __func__); Use dev_err_probe(), and the function name is pointless. > + return PTR_ERR(hwmon_dev); > + } > + > + return 0; > +} > + > +static struct platform_driver nct6694_hwmon_driver = { > + .driver = { > + .name = DRVNAME, > + }, > + .probe = nct6694_hwmon_probe, > +}; > + > +static int __init nct6694_init(void) > +{ > + int err; > + > + err = platform_driver_register(&nct6694_hwmon_driver); > + if (!err) { > + if (err) Seriously ? Read this code again. It is never reached (and pointless). > + platform_driver_unregister(&nct6694_hwmon_driver); > + } > + > + return err; > +} > +subsys_initcall(nct6694_init); > + > +static void __exit nct6694_exit(void) > +{ > + platform_driver_unregister(&nct6694_hwmon_driver); > +} > +module_exit(nct6694_exit); > + > +MODULE_DESCRIPTION("USB-HWMON driver for NCT6694"); > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); > +MODULE_LICENSE("GPL");
Dear Kalesh, Thank you for your comments. Ming Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com> 於 2024年10月24日 週四 下午5:20寫道： > > On Thu, Oct 24, 2024 at 2:33 PM Ming Yu <a0282524688@gmail.com> wrote: > > > > This driver supports Hardware monitor functionality for NCT6694 MFD > > device based on USB interface. > > > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > > --- > > MAINTAINERS | 1 + > > drivers/hwmon/Kconfig | 10 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/nct6694-hwmon.c | 407 ++++++++++++++++++++++++++++++++++ > > 4 files changed, 419 insertions(+) > > create mode 100644 drivers/hwmon/nct6694-hwmon.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 63387c0d4ab6..2aa87ad84156 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16439,6 +16439,7 @@ M: Ming Yu <tmyu0@nuvoton.com> > > L: linux-kernel@vger.kernel.org > > S: Supported > > F: drivers/gpio/gpio-nct6694.c > > +F: drivers/hwmon/nct6694-hwmon.c > > F: drivers/i2c/busses/i2c-nct6694.c > > F: drivers/mfd/nct6694.c > > F: drivers/net/can/nct6694_canfd.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index 08a3c863f80a..740e4afe6582 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -1625,6 +1625,16 @@ config SENSORS_NCT6683 > > This driver can also be built as a module. If so, the module > > will be called nct6683. > > > > +config SENSORS_NCT6694 > > + tristate "Nuvoton NCT6694 Hardware Monitor support" > > + depends on MFD_NCT6694 > > + help > > + Say Y here to support Nuvoton NCT6694 hardware monitoring > > + functionality. > > + > > + This driver can also be built as a module. If so, the module > > + will be called nct6694-hwmon. > > + > > config SENSORS_NCT6775_CORE > > tristate > > select REGMAP > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index 9554d2fdcf7b..729961176d00 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o > > obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o > > obj-$(CONFIG_SENSORS_MR75203) += mr75203.o > > obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o > > +obj-$(CONFIG_SENSORS_NCT6694) += nct6694-hwmon.o > > obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o > > nct6775-objs := nct6775-platform.o > > obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o > > diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c > > new file mode 100644 > > index 000000000000..7d7d22a650b0 > > --- /dev/null > > +++ b/drivers/hwmon/nct6694-hwmon.c > > @@ -0,0 +1,407 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Nuvoton NCT6694 HWMON driver based on USB interface. > > + * > > + * Copyright (C) 2024 Nuvoton Technology Corp. > > + */ > > + > > +#include <linux/slab.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/hwmon.h> > > +#include <linux/platform_device.h> > > +#include <linux/mfd/nct6694.h> > > + > > +#define DRVNAME "nct6694-hwmon" > > + > > +/* Host interface */ > > +#define REQUEST_RPT_MOD 0xFF > > +#define REQUEST_HWMON_MOD 0x00 > > + > > +/* Report Channel */ > > +#define HWMON_FIN_IDX(x) (0x50 + ((x) * 2)) > > +#define HWMON_FIN_STS(x) (0x6E + (x)) > > +#define HWMON_PWM_IDX(x) (0x70 + (x)) > > + > > +/* Message Channel*/ > > +/* Command 00h */ > > +#define REQUEST_HWMON_CMD0_LEN 0x40 > > +#define REQUEST_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ > > +#define HWMON_FIN_EN(x) (0x04 + (x)) > > +#define HWMON_PWM_FREQ_IDX(x) (0x30 + (x)) > > +/* Command 02h */ > > +#define REQUEST_HWMON_CMD2_LEN 0x90 > > +#define REQUEST_HWMON_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ > > +#define HWMON_SMI_CTRL_IDX 0x00 > > +#define HWMON_FIN_LIMIT_IDX(x) (0x70 + ((x) * 2)) > > +#define HWMON_CMD2_HYST_MASK 0x1F > > +/* Command 03h */ > > +#define REQUEST_HWMON_CMD3_LEN 0x08 > > +#define REQUEST_HWMON_CMD3_OFFSET 0x0003 /* OFFSET = SEL|CMD */ > > + > > +struct nct6694_hwmon_data { > > + struct nct6694 *nct6694; > > + > > + /* Make sure read & write commands are consecutive */ > > + struct mutex hwmon_lock; > > +}; > > + > > +#define NCT6694_HWMON_FAN_CONFIG (HWMON_F_ENABLE | HWMON_F_INPUT | \ > > + HWMON_F_MIN | HWMON_F_MIN_ALARM) > > +#define NCT6694_HWMON_PWM_CONFIG (HWMON_PWM_INPUT | HWMON_PWM_FREQ) > > + > > +static const struct hwmon_channel_info *nct6694_info[] = { > > + HWMON_CHANNEL_INFO(fan, > > + NCT6694_HWMON_FAN_CONFIG, /* FIN0 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN1 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN2 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN3 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN4 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN5 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN6 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN7 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN8 */ > > + NCT6694_HWMON_FAN_CONFIG), /* FIN9 */ > > + > > + HWMON_CHANNEL_INFO(pwm, > > + NCT6694_HWMON_PWM_CONFIG, /* PWM0 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM1 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM2 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM3 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM4 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM5 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM6 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM7 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM8 */ > > + NCT6694_HWMON_PWM_CONFIG), /* PWM9 */ > > + NULL > > +}; > > + > > +static int nct6694_fan_read(struct device *dev, u32 attr, int channel, > > + long *val) > > +{ > > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > > + unsigned char buf[2]; > > + int ret; > > + > > + switch (attr) { > > + case hwmon_fan_enable: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD0_OFFSET, > > + REQUEST_HWMON_CMD0_LEN, > > + HWMON_FIN_EN(channel / 8), > > + 1, buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = buf[0] & BIT(channel % 8) ? 1 : 0; > > + > > + break; > > + > > + case hwmon_fan_input: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > > + HWMON_FIN_IDX(channel), 2, 0, > > + 2, buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > > + > > + break; > > + > > + case hwmon_fan_min: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, > > + HWMON_FIN_LIMIT_IDX(channel), > > + 2, buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > > + > > + break; > > + > > + case hwmon_fan_min_alarm: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > > + HWMON_FIN_STS(channel / 8), > > + 1, 0, 1, buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = buf[0] & BIT(channel % 8); > > + > > + break; > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int nct6694_pwm_read(struct device *dev, u32 attr, int channel, > > + long *val) > > +{ > > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > > + unsigned char buf; > > + int ret; > > + > > + switch (attr) { > > + case hwmon_pwm_input: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > > + HWMON_PWM_IDX(channel), > > + 1, 0, 1, &buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = buf; > > + > > + break; > > + case hwmon_pwm_freq: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD0_OFFSET, > > + REQUEST_HWMON_CMD0_LEN, > > + HWMON_PWM_FREQ_IDX(channel), > > + 1, &buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = buf * 25000 / 255; > > + > > + break; > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > > + long val) > > +{ > > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > > + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > [Kalesh] Please try to maintain RCT order for variable declaration > > + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > > + u16 fan_val = (u16)val; > > + int ret; > > + > > + switch (attr) { > > + case hwmon_fan_enable: > > + mutex_lock(&data->hwmon_lock); > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD0_OFFSET, > > + REQUEST_HWMON_CMD0_LEN, 0, > > + REQUEST_HWMON_CMD0_LEN, > > + enable_buf); > > + if (ret) > > + goto err; > > + > > + if (val) > > + enable_buf[HWMON_FIN_EN(channel / 8)] |= BIT(channel % 8); > > + else > > + enable_buf[HWMON_FIN_EN(channel / 8)] &= ~BIT(channel % 8); > > + > > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD0_OFFSET, > > + REQUEST_HWMON_CMD0_LEN, enable_buf); > > + if (ret) > > + goto err; > > + > > + break; > > + > > + case hwmon_fan_min: > > + mutex_lock(&data->hwmon_lock); > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, 0, > > + REQUEST_HWMON_CMD2_LEN, buf); > > + if (ret) > > + goto err; > > + > > + buf[HWMON_FIN_LIMIT_IDX(channel)] = (u8)((fan_val >> 8) & 0xFF); > > + buf[HWMON_FIN_LIMIT_IDX(channel) + 1] = (u8)(fan_val & 0xFF); > > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, buf); > > + if (ret) > > + goto err; > > + > > + break; > > + > > + default: > > + ret = -EOPNOTSUPP; > [Kalesh] If you initialize "ret = -EOPNOTSUPP;" during declararion, > you can just break from here. > > + goto err; > > + } > > + > > +err: > > + mutex_unlock(&data->hwmon_lock); > > + return ret; > > +} > > + > > +static int nct6694_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + switch (type) { > > + case hwmon_fan: /* in RPM */ > > + return nct6694_fan_read(dev, attr, channel, val); > > + > > + case hwmon_pwm: /* in value 0~255 */ > > + return nct6694_pwm_read(dev, attr, channel, val); > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int nct6694_write(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + return nct6694_fan_write(dev, attr, channel, val); > > + default: > > + return -EOPNOTSUPP; > > + } > [Kalesh] You can use simple if condition here than a switch like: > if (type != hwmon_fan) > return -EOPNOTSUPP; > return nct6694_fan_write(dev, attr, channel, val); > > + > > + return 0; > > +} > > + > > +static umode_t nct6694_is_visible(const void *data, enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + switch (attr) { > > + case hwmon_fan_enable: > > + case hwmon_fan_min: > > + return 0644; > [Kalesh] I think there is no need to leave a new line in between cases > > + > > + case hwmon_fan_input: > > + case hwmon_fan_min_alarm: > > + return 0444; > > + > > + default: > > + return 0; > > + } > > + > > + case hwmon_pwm: > > + switch (attr) { > > + case hwmon_pwm_input: > > + case hwmon_pwm_freq: > > + return 0444; > > + default: > > + return 0; > > + } > > + > > + default: > > + return 0; > > + } > > + > > + return 0; > [Kalesh] This return statement looks redundant as the execution never > reaches here. Same comment applies to other functions above as well. > > +} > > + > > +static const struct hwmon_ops nct6694_hwmon_ops = { > > + .is_visible = nct6694_is_visible, > > + .read = nct6694_read, > > + .write = nct6694_write, > > +}; > > + > > +static const struct hwmon_chip_info nct6694_chip_info = { > > + .ops = &nct6694_hwmon_ops, > > + .info = nct6694_info, > > +}; > > + > > +static int nct6694_hwmon_init(struct nct6694_hwmon_data *data) > > +{ > > + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > > + int ret; > > + > > + /* Set Fan input Real Time alarm mode */ > > + mutex_lock(&data->hwmon_lock); > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, 0, > > + REQUEST_HWMON_CMD2_LEN, buf); > > + if (ret) > > + goto err; > [Kalesh] It would be better to rename the label as "unlock". Same > comment on other functions as well. > > + > > + buf[HWMON_SMI_CTRL_IDX] = 0x02; > > + > > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, buf); > > + if (ret) > > + goto err; > > + > > +err: > > + mutex_unlock(&data->hwmon_lock); > > + return ret; > > +} > > + > > +static int nct6694_hwmon_probe(struct platform_device *pdev) > > +{ > > + struct nct6694_hwmon_data *data; > > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); > > + struct device *hwmon_dev; > > + int ret; > > + > > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + data->nct6694 = nct6694; > > + mutex_init(&data->hwmon_lock); > > + platform_set_drvdata(pdev, data); > > + > > + ret = nct6694_hwmon_init(data); > > + if (ret) > > + return -EIO; > > + > > + /* Register hwmon device to HWMON framework */ > > + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > > + "nct6694", data, > > + &nct6694_chip_info, > > + NULL); > > + if (IS_ERR(hwmon_dev)) { > > + dev_err(&pdev->dev, "%s: Failed to register hwmon device!\n", > > + __func__); > > + return PTR_ERR(hwmon_dev); > > + } > > + > > + return 0; > > +} > > + > > +static struct platform_driver nct6694_hwmon_driver = { > > + .driver = { > > + .name = DRVNAME, > > + }, > > + .probe = nct6694_hwmon_probe, > > +}; > > + > > +static int __init nct6694_init(void) > > +{ > > + int err; > > + > > + err = platform_driver_register(&nct6694_hwmon_driver); > > + if (!err) { > > + if (err) > [Kalesh] This whole check looks strange. You can simplify this function as: > return platform_driver_register(&nct6694_hwmon_driver); > > + platform_driver_unregister(&nct6694_hwmon_driver); > > + } > > + > > + return err; > > +} > > +subsys_initcall(nct6694_init); > > + > > +static void __exit nct6694_exit(void) > > +{ > > + platform_driver_unregister(&nct6694_hwmon_driver); > > +} > > +module_exit(nct6694_exit); > > + > > +MODULE_DESCRIPTION("USB-HWMON driver for NCT6694"); > > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.34.1 > > > > > > > -- > Regards, > Kalesh A P
Dear Guenter, Thank you for your comments. Guenter Roeck <linux@roeck-us.net> 於 2024年10月24日 週四 下午10:53寫道： > > On 10/24/24 02:20, Kalesh Anakkur Purayil wrote: > > On Thu, Oct 24, 2024 at 2:33 PM Ming Yu <a0282524688@gmail.com> wrote: > >> > >> This driver supports Hardware monitor functionality for NCT6694 MFD > >> device based on USB interface. > >> > >> Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > >> --- > >> MAINTAINERS | 1 + > >> drivers/hwmon/Kconfig | 10 + > >> drivers/hwmon/Makefile | 1 + > >> drivers/hwmon/nct6694-hwmon.c | 407 ++++++++++++++++++++++++++++++++++ > >> 4 files changed, 419 insertions(+) > >> create mode 100644 drivers/hwmon/nct6694-hwmon.c > >> > >> diff --git a/MAINTAINERS b/MAINTAINERS > >> index 63387c0d4ab6..2aa87ad84156 100644 > >> --- a/MAINTAINERS > >> +++ b/MAINTAINERS > >> @@ -16439,6 +16439,7 @@ M: Ming Yu <tmyu0@nuvoton.com> > >> L: linux-kernel@vger.kernel.org > >> S: Supported > >> F: drivers/gpio/gpio-nct6694.c > >> +F: drivers/hwmon/nct6694-hwmon.c > >> F: drivers/i2c/busses/i2c-nct6694.c > >> F: drivers/mfd/nct6694.c > >> F: drivers/net/can/nct6694_canfd.c > >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > >> index 08a3c863f80a..740e4afe6582 100644 > >> --- a/drivers/hwmon/Kconfig > >> +++ b/drivers/hwmon/Kconfig > >> @@ -1625,6 +1625,16 @@ config SENSORS_NCT6683 > >> This driver can also be built as a module. If so, the module > >> will be called nct6683. > >> > >> +config SENSORS_NCT6694 > >> + tristate "Nuvoton NCT6694 Hardware Monitor support" > >> + depends on MFD_NCT6694 > >> + help > >> + Say Y here to support Nuvoton NCT6694 hardware monitoring > >> + functionality. > >> + > >> + This driver can also be built as a module. If so, the module > >> + will be called nct6694-hwmon. > >> + > >> config SENSORS_NCT6775_CORE > >> tristate > >> select REGMAP > >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > >> index 9554d2fdcf7b..729961176d00 100644 > >> --- a/drivers/hwmon/Makefile > >> +++ b/drivers/hwmon/Makefile > >> @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o > >> obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o > >> obj-$(CONFIG_SENSORS_MR75203) += mr75203.o > >> obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o > >> +obj-$(CONFIG_SENSORS_NCT6694) += nct6694-hwmon.o > >> obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o > >> nct6775-objs := nct6775-platform.o > >> obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o > >> diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c > >> new file mode 100644 > >> index 000000000000..7d7d22a650b0 > >> --- /dev/null > >> +++ b/drivers/hwmon/nct6694-hwmon.c > >> @@ -0,0 +1,407 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Nuvoton NCT6694 HWMON driver based on USB interface. > >> + * > >> + * Copyright (C) 2024 Nuvoton Technology Corp. > >> + */ > >> + > >> +#include <linux/slab.h> > >> +#include <linux/kernel.h> > >> +#include <linux/module.h> > >> +#include <linux/hwmon.h> > >> +#include <linux/platform_device.h> > >> +#include <linux/mfd/nct6694.h> > >> + > >> +#define DRVNAME "nct6694-hwmon" > >> + > >> +/* Host interface */ > >> +#define REQUEST_RPT_MOD 0xFF > >> +#define REQUEST_HWMON_MOD 0x00 > >> + > >> +/* Report Channel */ > >> +#define HWMON_FIN_IDX(x) (0x50 + ((x) * 2)) > >> +#define HWMON_FIN_STS(x) (0x6E + (x)) > >> +#define HWMON_PWM_IDX(x) (0x70 + (x)) > >> + > >> +/* Message Channel*/ > >> +/* Command 00h */ > >> +#define REQUEST_HWMON_CMD0_LEN 0x40 > >> +#define REQUEST_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ > >> +#define HWMON_FIN_EN(x) (0x04 + (x)) > >> +#define HWMON_PWM_FREQ_IDX(x) (0x30 + (x)) > >> +/* Command 02h */ > >> +#define REQUEST_HWMON_CMD2_LEN 0x90 > >> +#define REQUEST_HWMON_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ > >> +#define HWMON_SMI_CTRL_IDX 0x00 > >> +#define HWMON_FIN_LIMIT_IDX(x) (0x70 + ((x) * 2)) > >> +#define HWMON_CMD2_HYST_MASK 0x1F > >> +/* Command 03h */ > >> +#define REQUEST_HWMON_CMD3_LEN 0x08 > >> +#define REQUEST_HWMON_CMD3_OFFSET 0x0003 /* OFFSET = SEL|CMD */ > >> + > >> +struct nct6694_hwmon_data { > >> + struct nct6694 *nct6694; > >> + > >> + /* Make sure read & write commands are consecutive */ > >> + struct mutex hwmon_lock; > >> +}; > >> + > >> +#define NCT6694_HWMON_FAN_CONFIG (HWMON_F_ENABLE | HWMON_F_INPUT | \ > >> + HWMON_F_MIN | HWMON_F_MIN_ALARM) > >> +#define NCT6694_HWMON_PWM_CONFIG (HWMON_PWM_INPUT | HWMON_PWM_FREQ) > >> + > >> +static const struct hwmon_channel_info *nct6694_info[] = { > >> + HWMON_CHANNEL_INFO(fan, > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN0 */ > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN1 */ > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN2 */ > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN3 */ > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN4 */ > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN5 */ > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN6 */ > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN7 */ > >> + NCT6694_HWMON_FAN_CONFIG, /* FIN8 */ > >> + NCT6694_HWMON_FAN_CONFIG), /* FIN9 */ > >> + > >> + HWMON_CHANNEL_INFO(pwm, > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM0 */ > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM1 */ > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM2 */ > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM3 */ > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM4 */ > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM5 */ > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM6 */ > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM7 */ > >> + NCT6694_HWMON_PWM_CONFIG, /* PWM8 */ > >> + NCT6694_HWMON_PWM_CONFIG), /* PWM9 */ > >> + NULL > >> +}; > >> + > >> +static int nct6694_fan_read(struct device *dev, u32 attr, int channel, > >> + long *val) > >> +{ > >> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > >> + unsigned char buf[2]; > >> + int ret; > >> + > >> + switch (attr) { > >> + case hwmon_fan_enable: > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD0_OFFSET, > >> + REQUEST_HWMON_CMD0_LEN, > >> + HWMON_FIN_EN(channel / 8), > >> + 1, buf); > >> + if (ret) > >> + return -EINVAL; > >> + > >> + *val = buf[0] & BIT(channel % 8) ? 1 : 0; > >> + > >> + break; > >> + > >> + case hwmon_fan_input: > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > >> + HWMON_FIN_IDX(channel), 2, 0, > >> + 2, buf); > >> + if (ret) > >> + return -EINVAL; > >> + > >> + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > >> + > >> + break; > >> + > >> + case hwmon_fan_min: > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD2_OFFSET, > >> + REQUEST_HWMON_CMD2_LEN, > >> + HWMON_FIN_LIMIT_IDX(channel), > >> + 2, buf); > >> + if (ret) > >> + return -EINVAL; > >> + > >> + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > >> + > >> + break; > >> + > >> + case hwmon_fan_min_alarm: > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > >> + HWMON_FIN_STS(channel / 8), > >> + 1, 0, 1, buf); > >> + if (ret) > >> + return -EINVAL; > >> + > >> + *val = buf[0] & BIT(channel % 8); > >> + > >> + break; > >> + > >> + default: > >> + return -EOPNOTSUPP; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int nct6694_pwm_read(struct device *dev, u32 attr, int channel, > >> + long *val) > >> +{ > >> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > >> + unsigned char buf; > >> + int ret; > >> + > >> + switch (attr) { > >> + case hwmon_pwm_input: > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > >> + HWMON_PWM_IDX(channel), > >> + 1, 0, 1, &buf); > >> + if (ret) > >> + return -EINVAL; > >> + > >> + *val = buf; > >> + > >> + break; > >> + case hwmon_pwm_freq: > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD0_OFFSET, > >> + REQUEST_HWMON_CMD0_LEN, > >> + HWMON_PWM_FREQ_IDX(channel), > >> + 1, &buf); > >> + if (ret) > >> + return -EINVAL; > >> + > >> + *val = buf * 25000 / 255; > >> + > >> + break; > >> + > >> + default: > >> + return -EOPNOTSUPP; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > >> + long val) > >> +{ > >> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > >> + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > > [Kalesh] Please try to maintain RCT order for variable declaration > > Ok, but that is already the case here ? [Ming] Is there anything that needs to be changed? > > >> + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > >> + u16 fan_val = (u16)val; > >> + int ret; > >> + > >> + switch (attr) { > >> + case hwmon_fan_enable: > >> + mutex_lock(&data->hwmon_lock); > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD0_OFFSET, > >> + REQUEST_HWMON_CMD0_LEN, 0, > >> + REQUEST_HWMON_CMD0_LEN, > >> + enable_buf); > >> + if (ret) > >> + goto err; > >> + > >> + if (val) > >> + enable_buf[HWMON_FIN_EN(channel / 8)] |= BIT(channel % 8); > >> + else > >> + enable_buf[HWMON_FIN_EN(channel / 8)] &= ~BIT(channel % 8); > >> + > >> + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD0_OFFSET, > >> + REQUEST_HWMON_CMD0_LEN, enable_buf); > >> + if (ret) > >> + goto err; > >> + > >> + break; > >> + > >> + case hwmon_fan_min: > >> + mutex_lock(&data->hwmon_lock); > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD2_OFFSET, > >> + REQUEST_HWMON_CMD2_LEN, 0, > >> + REQUEST_HWMON_CMD2_LEN, buf); > >> + if (ret) > >> + goto err; > >> + > >> + buf[HWMON_FIN_LIMIT_IDX(channel)] = (u8)((fan_val >> 8) & 0xFF); > >> + buf[HWMON_FIN_LIMIT_IDX(channel) + 1] = (u8)(fan_val & 0xFF); > >> + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD2_OFFSET, > >> + REQUEST_HWMON_CMD2_LEN, buf); > >> + if (ret) > >> + goto err; > >> + > >> + break; > >> + > >> + default: > >> + ret = -EOPNOTSUPP; > > [Kalesh] If you initialize "ret = -EOPNOTSUPP;" during declararion, > > you can just break from here. > > You are missing the point. The lock wasn't acquired here in the first place. > It is conceptually wrong to acquire a lock in the switch statement and release > it outside. This patch is a case in point. [Ming] Okay! I'll acquire the lock before switch() in the next patch. > > >> + goto err; > >> + } > >> + > >> +err: > >> + mutex_unlock(&data->hwmon_lock); > >> + return ret; > >> +} > >> + > >> +static int nct6694_read(struct device *dev, enum hwmon_sensor_types type, > >> + u32 attr, int channel, long *val) > >> +{ > >> + switch (type) { > >> + case hwmon_fan: /* in RPM */ > >> + return nct6694_fan_read(dev, attr, channel, val); > >> + > >> + case hwmon_pwm: /* in value 0~255 */ > >> + return nct6694_pwm_read(dev, attr, channel, val); > >> + > >> + default: > >> + return -EOPNOTSUPP; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int nct6694_write(struct device *dev, enum hwmon_sensor_types type, > >> + u32 attr, int channel, long val) > >> +{ > >> + switch (type) { > >> + case hwmon_fan: > >> + return nct6694_fan_write(dev, attr, channel, val); > >> + default: > >> + return -EOPNOTSUPP; > >> + } > > [Kalesh] You can use simple if condition here than a switch like: > > if (type != hwmon_fan) > > return -EOPNOTSUPP; > > return nct6694_fan_write(dev, attr, channel, val); > > That is a bit POV. I'd leave that to the developer. > More important is that the return statements after the switch are unnecessary > and never reached if each case returns immediately. [Ming] Okay! I'll drop it in the next patch. > > >> + > >> + return 0; > >> +} > >> + > >> +static umode_t nct6694_is_visible(const void *data, enum hwmon_sensor_types type, > >> + u32 attr, int channel) > >> +{ > >> + switch (type) { > >> + case hwmon_fan: > >> + switch (attr) { > >> + case hwmon_fan_enable: > >> + case hwmon_fan_min: > >> + return 0644; > > [Kalesh] I think there is no need to leave a new line in between cases > >> + > >> + case hwmon_fan_input: > >> + case hwmon_fan_min_alarm: > >> + return 0444; > >> + > >> + default: > >> + return 0; > >> + } > >> + > >> + case hwmon_pwm: > >> + switch (attr) { > >> + case hwmon_pwm_input: > >> + case hwmon_pwm_freq: > >> + return 0444; > >> + default: > >> + return 0; > >> + } > >> + > >> + default: > >> + return 0; > >> + } > >> + > >> + return 0; > > [Kalesh] This return statement looks redundant as the execution never > > reaches here. Same comment applies to other functions above as well. > >> +} > >> + > >> +static const struct hwmon_ops nct6694_hwmon_ops = { > >> + .is_visible = nct6694_is_visible, > >> + .read = nct6694_read, > >> + .write = nct6694_write, > >> +}; > >> + > >> +static const struct hwmon_chip_info nct6694_chip_info = { > >> + .ops = &nct6694_hwmon_ops, > >> + .info = nct6694_info, > >> +}; > >> + > >> +static int nct6694_hwmon_init(struct nct6694_hwmon_data *data) > >> +{ > >> + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > >> + int ret; > >> + > >> + /* Set Fan input Real Time alarm mode */ > >> + mutex_lock(&data->hwmon_lock); > >> + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD2_OFFSET, > >> + REQUEST_HWMON_CMD2_LEN, 0, > >> + REQUEST_HWMON_CMD2_LEN, buf); > >> + if (ret) > >> + goto err; > > [Kalesh] It would be better to rename the label as "unlock". Same > > comment on other functions as well. > > The lock is not needed here in the first place. The function is called > exactly once during initialization. [Ming] I'll remove the lock in the next patch! > > >> + > >> + buf[HWMON_SMI_CTRL_IDX] = 0x02; > >> + > >> + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > >> + REQUEST_HWMON_CMD2_OFFSET, > >> + REQUEST_HWMON_CMD2_LEN, buf); > >> + if (ret) > >> + goto err; > >> + > >> +err: > >> + mutex_unlock(&data->hwmon_lock); > >> + return ret; > >> +} > >> + > >> +static int nct6694_hwmon_probe(struct platform_device *pdev) > >> +{ > >> + struct nct6694_hwmon_data *data; > >> + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); > >> + struct device *hwmon_dev; > >> + int ret; > >> + > >> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > >> + if (!data) > >> + return -ENOMEM; > >> + > >> + data->nct6694 = nct6694; > >> + mutex_init(&data->hwmon_lock); > >> + platform_set_drvdata(pdev, data); > >> + > >> + ret = nct6694_hwmon_init(data); > >> + if (ret) > >> + return -EIO; > >> + > >> + /* Register hwmon device to HWMON framework */ > >> + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > >> + "nct6694", data, > >> + &nct6694_chip_info, > >> + NULL); > >> + if (IS_ERR(hwmon_dev)) { > >> + dev_err(&pdev->dev, "%s: Failed to register hwmon device!\n", > >> + __func__); > >> + return PTR_ERR(hwmon_dev); > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static struct platform_driver nct6694_hwmon_driver = { > >> + .driver = { > >> + .name = DRVNAME, > >> + }, > >> + .probe = nct6694_hwmon_probe, > >> +}; > >> + > >> +static int __init nct6694_init(void) > >> +{ > >> + int err; > >> + > >> + err = platform_driver_register(&nct6694_hwmon_driver); > >> + if (!err) { > >> + if (err) > > [Kalesh] This whole check looks strange. You can simplify this function as: > > return platform_driver_register(&nct6694_hwmon_driver); > >> + platform_driver_unregister(&nct6694_hwmon_driver); > >> + } > >> + > >> + return err; > >> +} > >> +subsys_initcall(nct6694_init); > >> + > >> +static void __exit nct6694_exit(void) > >> +{ > >> + platform_driver_unregister(&nct6694_hwmon_driver); > >> +} > >> +module_exit(nct6694_exit); > >> + > >> +MODULE_DESCRIPTION("USB-HWMON driver for NCT6694"); > >> +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); > >> +MODULE_LICENSE("GPL"); > >> -- > >> 2.34.1 > >> > >> > > > > > [Ming] For platform driver registration, I'll change it to module_platform_driver() in the next patch. Thank, Ming
Hi Guenter, I will remove the unnecessary return statement and lock, and update the part of the code you mentioned. Thanks, Ming Guenter Roeck <linux@roeck-us.net> 於 2024年10月24日 週四 下午11:03寫道： > > On 10/24/24 01:59, Ming Yu wrote: > > This driver supports Hardware monitor functionality for NCT6694 MFD > > device based on USB interface. > > > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > > --- > > MAINTAINERS | 1 + > > drivers/hwmon/Kconfig | 10 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/nct6694-hwmon.c | 407 ++++++++++++++++++++++++++++++++++ > > 4 files changed, 419 insertions(+) > > create mode 100644 drivers/hwmon/nct6694-hwmon.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 63387c0d4ab6..2aa87ad84156 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16439,6 +16439,7 @@ M: Ming Yu <tmyu0@nuvoton.com> > > L: linux-kernel@vger.kernel.org > > S: Supported > > F: drivers/gpio/gpio-nct6694.c > > +F: drivers/hwmon/nct6694-hwmon.c > > F: drivers/i2c/busses/i2c-nct6694.c > > F: drivers/mfd/nct6694.c > > F: drivers/net/can/nct6694_canfd.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index 08a3c863f80a..740e4afe6582 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -1625,6 +1625,16 @@ config SENSORS_NCT6683 > > This driver can also be built as a module. If so, the module > > will be called nct6683. > > > > +config SENSORS_NCT6694 > > + tristate "Nuvoton NCT6694 Hardware Monitor support" > > + depends on MFD_NCT6694 > > + help > > + Say Y here to support Nuvoton NCT6694 hardware monitoring > > + functionality. > > + > > + This driver can also be built as a module. If so, the module > > + will be called nct6694-hwmon. > > + > > config SENSORS_NCT6775_CORE > > tristate > > select REGMAP > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index 9554d2fdcf7b..729961176d00 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o > > obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o > > obj-$(CONFIG_SENSORS_MR75203) += mr75203.o > > obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o > > +obj-$(CONFIG_SENSORS_NCT6694) += nct6694-hwmon.o > > obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o > > nct6775-objs := nct6775-platform.o > > obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o > > diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c > > new file mode 100644 > > index 000000000000..7d7d22a650b0 > > --- /dev/null > > +++ b/drivers/hwmon/nct6694-hwmon.c > > @@ -0,0 +1,407 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Nuvoton NCT6694 HWMON driver based on USB interface. > > + * > > + * Copyright (C) 2024 Nuvoton Technology Corp. > > + */ > > + > > +#include <linux/slab.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/hwmon.h> > > +#include <linux/platform_device.h> > > +#include <linux/mfd/nct6694.h> > > + > > +#define DRVNAME "nct6694-hwmon" > > + > > +/* Host interface */ > > +#define REQUEST_RPT_MOD 0xFF > > +#define REQUEST_HWMON_MOD 0x00 > > + > > +/* Report Channel */ > > +#define HWMON_FIN_IDX(x) (0x50 + ((x) * 2)) > > +#define HWMON_FIN_STS(x) (0x6E + (x)) > > +#define HWMON_PWM_IDX(x) (0x70 + (x)) > > + > > +/* Message Channel*/ > > +/* Command 00h */ > > +#define REQUEST_HWMON_CMD0_LEN 0x40 > > +#define REQUEST_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ > > +#define HWMON_FIN_EN(x) (0x04 + (x)) > > +#define HWMON_PWM_FREQ_IDX(x) (0x30 + (x)) > > +/* Command 02h */ > > +#define REQUEST_HWMON_CMD2_LEN 0x90 > > +#define REQUEST_HWMON_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ > > +#define HWMON_SMI_CTRL_IDX 0x00 > > +#define HWMON_FIN_LIMIT_IDX(x) (0x70 + ((x) * 2)) > > +#define HWMON_CMD2_HYST_MASK 0x1F > > +/* Command 03h */ > > +#define REQUEST_HWMON_CMD3_LEN 0x08 > > +#define REQUEST_HWMON_CMD3_OFFSET 0x0003 /* OFFSET = SEL|CMD */ > > + > > +struct nct6694_hwmon_data { > > + struct nct6694 *nct6694; > > + > > + /* Make sure read & write commands are consecutive */ > > + struct mutex hwmon_lock; > > +}; > > + > > +#define NCT6694_HWMON_FAN_CONFIG (HWMON_F_ENABLE | HWMON_F_INPUT | \ > > + HWMON_F_MIN | HWMON_F_MIN_ALARM) > > +#define NCT6694_HWMON_PWM_CONFIG (HWMON_PWM_INPUT | HWMON_PWM_FREQ) > > + > > +static const struct hwmon_channel_info *nct6694_info[] = { > > + HWMON_CHANNEL_INFO(fan, > > + NCT6694_HWMON_FAN_CONFIG, /* FIN0 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN1 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN2 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN3 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN4 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN5 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN6 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN7 */ > > + NCT6694_HWMON_FAN_CONFIG, /* FIN8 */ > > + NCT6694_HWMON_FAN_CONFIG), /* FIN9 */ > > + > > + HWMON_CHANNEL_INFO(pwm, > > + NCT6694_HWMON_PWM_CONFIG, /* PWM0 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM1 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM2 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM3 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM4 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM5 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM6 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM7 */ > > + NCT6694_HWMON_PWM_CONFIG, /* PWM8 */ > > + NCT6694_HWMON_PWM_CONFIG), /* PWM9 */ > > + NULL > > +}; > > + > > +static int nct6694_fan_read(struct device *dev, u32 attr, int channel, > > + long *val) > > +{ > > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > > + unsigned char buf[2]; > > + int ret; > > + > > + switch (attr) { > > + case hwmon_fan_enable: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD0_OFFSET, > > + REQUEST_HWMON_CMD0_LEN, > > + HWMON_FIN_EN(channel / 8), > > + 1, buf); > > + if (ret) > > + return -EINVAL; > > Do not overwrite error return values. > > > + > > + *val = buf[0] & BIT(channel % 8) ? 1 : 0; > > + > > + break; > > + > > + case hwmon_fan_input: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > > + HWMON_FIN_IDX(channel), 2, 0, > > + 2, buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > > + > > + break; > > + > > + case hwmon_fan_min: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, > > + HWMON_FIN_LIMIT_IDX(channel), > > + 2, buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; > > + > > + break; > > + > > + case hwmon_fan_min_alarm: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > > + HWMON_FIN_STS(channel / 8), > > + 1, 0, 1, buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = buf[0] & BIT(channel % 8); > > + > > + break; > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int nct6694_pwm_read(struct device *dev, u32 attr, int channel, > > + long *val) > > +{ > > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > > + unsigned char buf; > > + int ret; > > + > > + switch (attr) { > > + case hwmon_pwm_input: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, > > + HWMON_PWM_IDX(channel), > > + 1, 0, 1, &buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = buf; > > + > > + break; > > + case hwmon_pwm_freq: > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD0_OFFSET, > > + REQUEST_HWMON_CMD0_LEN, > > + HWMON_PWM_FREQ_IDX(channel), > > + 1, &buf); > > + if (ret) > > + return -EINVAL; > > + > > + *val = buf * 25000 / 255; > > + > > + break; > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > > + long val) > > +{ > > + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > > + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > > + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > > + u16 fan_val = (u16)val; > > This is wrong. It will result in overflows/underflows if out of range > values are provided, and the driver should not write 0 if the user > writes 65536. You need to use clamp_val() instead. For values with > well defined range (specifically hwmon_fan_enable) you need to validate > the parameter, not just take it as boolean. [Ming] Okay! I'll update the code in the next patch. > > > + int ret; > > + > > + switch (attr) { > > + case hwmon_fan_enable: > > + mutex_lock(&data->hwmon_lock); > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD0_OFFSET, > > + REQUEST_HWMON_CMD0_LEN, 0, > > + REQUEST_HWMON_CMD0_LEN, > > + enable_buf); > > + if (ret) > > + goto err; > > + > > + if (val) > > + enable_buf[HWMON_FIN_EN(channel / 8)] |= BIT(channel % 8); > > + else > > + enable_buf[HWMON_FIN_EN(channel / 8)] &= ~BIT(channel % 8); > > + > > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD0_OFFSET, > > + REQUEST_HWMON_CMD0_LEN, enable_buf); > > + if (ret) > > + goto err; > > + > > + break; > > + > > + case hwmon_fan_min: > > + mutex_lock(&data->hwmon_lock); > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, 0, > > + REQUEST_HWMON_CMD2_LEN, buf); > > + if (ret) > > + goto err; > > + > > + buf[HWMON_FIN_LIMIT_IDX(channel)] = (u8)((fan_val >> 8) & 0xFF); > > + buf[HWMON_FIN_LIMIT_IDX(channel) + 1] = (u8)(fan_val & 0xFF); > > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, buf); > > + if (ret) > > + goto err; > > + > > + break; > > The error handler goto and the break accomplish exactly the same, > thus the conditional goto is pointless. > > > + > > + default: > > + ret = -EOPNOTSUPP; > > + goto err; > > As mentioned in my other reply, the lock is not acquired here, > causing an unbalanced unlock. > > > + } > > + > > +err: > > + mutex_unlock(&data->hwmon_lock); > > + return ret; > > +} > > + > > +static int nct6694_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + switch (type) { > > + case hwmon_fan: /* in RPM */ > > + return nct6694_fan_read(dev, attr, channel, val); > > + > > + case hwmon_pwm: /* in value 0~255 */ > > + return nct6694_pwm_read(dev, attr, channel, val); > > + > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > Unnecessary return statement. Also seen elsewhere. > > > +} > > + > > +static int nct6694_write(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + return nct6694_fan_write(dev, attr, channel, val); > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > +static umode_t nct6694_is_visible(const void *data, enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + switch (attr) { > > + case hwmon_fan_enable: > > + case hwmon_fan_min: > > + return 0644; > > + > > + case hwmon_fan_input: > > + case hwmon_fan_min_alarm: > > + return 0444; > > + > > + default: > > + return 0; > > + } > > + > > + case hwmon_pwm: > > + switch (attr) { > > + case hwmon_pwm_input: > > + case hwmon_pwm_freq: > > + return 0444; > > + default: > > + return 0; > > + } > > + > > + default: > > + return 0; > > + } > > + > > + return 0; > > +} > > + > > +static const struct hwmon_ops nct6694_hwmon_ops = { > > + .is_visible = nct6694_is_visible, > > + .read = nct6694_read, > > + .write = nct6694_write, > > +}; > > + > > +static const struct hwmon_chip_info nct6694_chip_info = { > > + .ops = &nct6694_hwmon_ops, > > + .info = nct6694_info, > > +}; > > + > > +static int nct6694_hwmon_init(struct nct6694_hwmon_data *data) > > +{ > > + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; > > + int ret; > > + > > + /* Set Fan input Real Time alarm mode */ > > + mutex_lock(&data->hwmon_lock); > > Pointless lock (this is init code) > > > + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, 0, > > + REQUEST_HWMON_CMD2_LEN, buf); > > + if (ret) > > + goto err; > > + > > + buf[HWMON_SMI_CTRL_IDX] = 0x02; > > + > > + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, > > + REQUEST_HWMON_CMD2_OFFSET, > > + REQUEST_HWMON_CMD2_LEN, buf); > > + if (ret) > > + goto err; > > + > > +err: > > + mutex_unlock(&data->hwmon_lock); > > + return ret; > > +} > > + > > +static int nct6694_hwmon_probe(struct platform_device *pdev) > > +{ > > + struct nct6694_hwmon_data *data; > > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); > > + struct device *hwmon_dev; > > + int ret; > > + > > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + data->nct6694 = nct6694; > > + mutex_init(&data->hwmon_lock); > > + platform_set_drvdata(pdev, data); > > + > > + ret = nct6694_hwmon_init(data); > > + if (ret) > > + return -EIO; > > Again, do not overwrite error codes. > > > + > > + /* Register hwmon device to HWMON framework */ > > + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > > + "nct6694", data, > > + &nct6694_chip_info, > > + NULL); > > + if (IS_ERR(hwmon_dev)) { > > + dev_err(&pdev->dev, "%s: Failed to register hwmon device!\n", > > + __func__); > > Use dev_err_probe(), and the function name is pointless. [Ming] Okay! I'll change it in the next patch. > > > + return PTR_ERR(hwmon_dev); > > + } > > + > > + return 0; > > +} > > + > > +static struct platform_driver nct6694_hwmon_driver = { > > + .driver = { > > + .name = DRVNAME, > > + }, > > + .probe = nct6694_hwmon_probe, > > +}; > > + > > +static int __init nct6694_init(void) > > +{ > > + int err; > > + > > + err = platform_driver_register(&nct6694_hwmon_driver); > > + if (!err) { > > + if (err) > > Seriously ? Read this code again. It is never reached (and pointless). [Ming] For platform driver registration, I'll change it to module_platform_driver() in the next patch. > > > + platform_driver_unregister(&nct6694_hwmon_driver); > > + } > > + > > + return err; > > +} > > +subsys_initcall(nct6694_init); > > + > > +static void __exit nct6694_exit(void) > > +{ > > + platform_driver_unregister(&nct6694_hwmon_driver); > > +} > > +module_exit(nct6694_exit); > > + > > +MODULE_DESCRIPTION("USB-HWMON driver for NCT6694"); > > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); > > +MODULE_LICENSE("GPL"); >
On 10/25/24 08:22, Ming Yu wrote: [ ... ] >>>> +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, >>>> + long val) >>>> +{ >>>> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); >>>> + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; >>> [Kalesh] Please try to maintain RCT order for variable declaration >> >> Ok, but that is already the case here ? > > [Ming] Is there anything that needs to be changed? > I don't think so, If two lines have the same length, the order is up to the developer to decide. Question though is if the buffer needs to be initialized. You should drop the initialization if it is not necessary. In that case the second line would be shorter anyway, and the order question would not arise. Thanks, Guenter
On 10/25/24 08:44, Guenter Roeck wrote: > On 10/25/24 08:22, Ming Yu wrote: > [ ... ] > >>>>> +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, >>>>> + long val) >>>>> +{ >>>>> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); >>>>> + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; >>>> [Kalesh] Please try to maintain RCT order for variable declaration >>> >>> Ok, but that is already the case here ? >> >> [Ming] Is there anything that needs to be changed? >> > > I don't think so, If two lines have the same length, the order is up > to the developer to decide. > > Question though is if the buffer needs to be initialized. You should drop > the initialization if it is not necessary. In that case the second line > would be shorter anyway, and the order question would not arise. > Actually, I just noticed that you also submitted an IIO driver which reports the same data again. If a chip has an IIO driver, there should be no HWMON driver since the IIO -> HWMON bridge can then be used if necessary. So please drop this driver. Thanks, Guenter
Understood, Thank you. Best regards, Ming Guenter Roeck <linux@roeck-us.net> 於 2024年10月25日 週五 下午11:44寫道： > > On 10/25/24 08:22, Ming Yu wrote: > [ ... ] > > >>>> +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > >>>> + long val) > >>>> +{ > >>>> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > >>>> + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > >>> [Kalesh] Please try to maintain RCT order for variable declaration > >> > >> Ok, but that is already the case here ? > > > > [Ming] Is there anything that needs to be changed? > > > > I don't think so, If two lines have the same length, the order is up > to the developer to decide. > > Question though is if the buffer needs to be initialized. You should drop > the initialization if it is not necessary. In that case the second line > would be shorter anyway, and the order question would not arise. > > Thanks, > Guenter >
Dear Guenter, The original plan was to use the IIO driver to access the temperature and voltage sensors, and the HWMON driver to access the tachometers. However, since the device is a hot-plug USB device, as far as I know, IIO-HWMON is not applicable. I will merge the IIO driver part into the HWMON driver in the next patch. In other words, the driver will be used to access TIN, VIN and FIN. Best regards Ming Guenter Roeck <linux@roeck-us.net> 於 2024年10月26日 週六 下午10:50寫道： > > On 10/25/24 08:44, Guenter Roeck wrote: > > On 10/25/24 08:22, Ming Yu wrote: > > [ ... ] > > > >>>>> +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > >>>>> + long val) > >>>>> +{ > >>>>> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > >>>>> + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > >>>> [Kalesh] Please try to maintain RCT order for variable declaration > >>> > >>> Ok, but that is already the case here ? > >> > >> [Ming] Is there anything that needs to be changed? > >> > > > > I don't think so, If two lines have the same length, the order is up > > to the developer to decide. > > > > Question though is if the buffer needs to be initialized. You should drop > > the initialization if it is not necessary. In that case the second line > > would be shorter anyway, and the order question would not arise. > > > > Actually, I just noticed that you also submitted an IIO driver which > reports the same data again. If a chip has an IIO driver, there should > be no HWMON driver since the IIO -> HWMON bridge can then be used if > necessary. So please drop this driver. > > Thanks, > Guenter > >
On Mon, 28 Oct 2024 15:58:00 +0800 Ming Yu <a0282524688@gmail.com> wrote: > Dear Guenter, > > The original plan was to use the IIO driver to access the temperature > and voltage sensors, and the HWMON driver to access the tachometers. > However, since the device is a hot-plug USB device, as far as I know, > IIO-HWMON is not applicable. I will merge the IIO driver part into the > HWMON driver in the next patch. > In other words, the driver will be used to access TIN, VIN and FIN. See drivers/mfd/sun4i-gpadc.c for an example of an mfd using the iio-hwmon bridge. Jonathan > > Best regards > Ming > > Guenter Roeck <linux@roeck-us.net> 於 2024年10月26日 週六 下午10:50寫道： > > > > On 10/25/24 08:44, Guenter Roeck wrote: > > > On 10/25/24 08:22, Ming Yu wrote: > > > [ ... ] > > > > > >>>>> +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > > >>>>> + long val) > > >>>>> +{ > > >>>>> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > > >>>>> + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > > >>>> [Kalesh] Please try to maintain RCT order for variable declaration > > >>> > > >>> Ok, but that is already the case here ? > > >> > > >> [Ming] Is there anything that needs to be changed? > > >> > > > > > > I don't think so, If two lines have the same length, the order is up > > > to the developer to decide. > > > > > > Question though is if the buffer needs to be initialized. You should drop > > > the initialization if it is not necessary. In that case the second line > > > would be shorter anyway, and the order question would not arise. > > > > > > > Actually, I just noticed that you also submitted an IIO driver which > > reports the same data again. If a chip has an IIO driver, there should > > be no HWMON driver since the IIO -> HWMON bridge can then be used if > > necessary. So please drop this driver. > > > > Thanks, > > Guenter > > > > >
Dear Jonathan, Thanks you for your comments, I tested your suggestion in both the MFD driver and the IIO driver, and the iio-hwmon bridge worked well. On the other hand, my requirements involve accessing thermal sensors, voltage sensors and tachometers, so I should implement it in this HWMON drive, right? Best regards Ming Jonathan Cameron <jic23@kernel.org> 於 2024年10月29日 週二 上午2:54寫道： > > On Mon, 28 Oct 2024 15:58:00 +0800 > Ming Yu <a0282524688@gmail.com> wrote: > > > Dear Guenter, > > > > The original plan was to use the IIO driver to access the temperature > > and voltage sensors, and the HWMON driver to access the tachometers. > > However, since the device is a hot-plug USB device, as far as I know, > > IIO-HWMON is not applicable. I will merge the IIO driver part into the > > HWMON driver in the next patch. > > In other words, the driver will be used to access TIN, VIN and FIN. > See drivers/mfd/sun4i-gpadc.c > for an example of an mfd using the iio-hwmon bridge. > > Jonathan > > > > > Best regards > > Ming > > > > Guenter Roeck <linux@roeck-us.net> 於 2024年10月26日 週六 下午10:50寫道： > > > > > > On 10/25/24 08:44, Guenter Roeck wrote: > > > > On 10/25/24 08:22, Ming Yu wrote: > > > > [ ... ] > > > > > > > >>>>> +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, > > > >>>>> + long val) > > > >>>>> +{ > > > >>>>> + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); > > > >>>>> + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; > > > >>>> [Kalesh] Please try to maintain RCT order for variable declaration > > > >>> > > > >>> Ok, but that is already the case here ? > > > >> > > > >> [Ming] Is there anything that needs to be changed? > > > >> > > > > > > > > I don't think so, If two lines have the same length, the order is up > > > > to the developer to decide. > > > > > > > > Question though is if the buffer needs to be initialized. You should drop > > > > the initialization if it is not necessary. In that case the second line > > > > would be shorter anyway, and the order question would not arise. > > > > > > > > > > Actually, I just noticed that you also submitted an IIO driver which > > > reports the same data again. If a chip has an IIO driver, there should > > > be no HWMON driver since the IIO -> HWMON bridge can then be used if > > > necessary. So please drop this driver. > > > > > > Thanks, > > > Guenter > > > > > > > > >
On 10/29/24 20:29, Ming Yu wrote: > Dear Jonathan, > > Thanks you for your comments, > I tested your suggestion in both the MFD driver and the IIO driver, and > the iio-hwmon bridge worked well. > On the other hand, my requirements involve accessing thermal sensors, > voltage sensors and tachometers, so I should implement it in this HWMON > drive, right? > Duplicate drivers for the same hardware is not acceptable. I see that so far only pwm and fan control is implemented in the hwmon driver. There is no public documentation for NCT6694, so it is difficult to evaluate the chip's capabilities. The summary doesn't even mention fan speed readings, meaning pretty much everything is guesswork. Either case, I do see that you also implemented a pwm driver which _does_ duplicate hwmon functionality. Sorry, that is a no-go. Again, we can not have multiple drivers controlling the same hardware. A pwm controller implemented in a hwmon device is supposed to be limited to fan control. It looks like the pwm controller implemented in the NCT6694 is a generic pwm controller. It is not appropriate to have a hwmon driver for such a pwm controller. Guenter
> Duplicate drivers for the same hardware is not acceptable. > > I see that so far only pwm and fan control is implemented in the hwmon driver. > There is no public documentation for NCT6694, so it is difficult to evaluate the > chip's capabilities. The summary doesn't even mention fan speed readings, meaning > pretty much everything is guesswork. > > Either case, I do see that you also implemented a pwm driver which _does_ > duplicate hwmon functionality. Sorry, that is a no-go. Again, we can not have > multiple drivers controlling the same hardware. A pwm controller implemented > in a hwmon device is supposed to be limited to fan control. It looks like > the pwm controller implemented in the NCT6694 is a generic pwm controller. > It is not appropriate to have a hwmon driver for such a pwm controller. > > Guenter > I understand, I'll drop the pwm controller driver in the next patch, and implement only the fan control functionality in HWMON driver. These functionalities are actually implemented by NCT6694 firmware, which supports up to 16 voltage sensors, 26 temperature sensors, 10 PWM controllers and 10 tachometers. Do you think implementing these functionalities in the HWMON driver complies with regulations? If the answer is yes, I will drop the IIO and PWM driver and implement everything in the HWMON driver in the next patch. Thanks, Ming
diff --git a/MAINTAINERS b/MAINTAINERS index 63387c0d4ab6..2aa87ad84156 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16439,6 +16439,7 @@ M: Ming Yu <tmyu0@nuvoton.com> L: linux-kernel@vger.kernel.org S: Supported F: drivers/gpio/gpio-nct6694.c +F: drivers/hwmon/nct6694-hwmon.c F: drivers/i2c/busses/i2c-nct6694.c F: drivers/mfd/nct6694.c F: drivers/net/can/nct6694_canfd.c diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 08a3c863f80a..740e4afe6582 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1625,6 +1625,16 @@ config SENSORS_NCT6683 This driver can also be built as a module. If so, the module will be called nct6683. +config SENSORS_NCT6694 + tristate "Nuvoton NCT6694 Hardware Monitor support" + depends on MFD_NCT6694 + help + Say Y here to support Nuvoton NCT6694 hardware monitoring + functionality. + + This driver can also be built as a module. If so, the module + will be called nct6694-hwmon. + config SENSORS_NCT6775_CORE tristate select REGMAP diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 9554d2fdcf7b..729961176d00 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o obj-$(CONFIG_SENSORS_MR75203) += mr75203.o obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o +obj-$(CONFIG_SENSORS_NCT6694) += nct6694-hwmon.o obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o nct6775-objs := nct6775-platform.o obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c new file mode 100644 index 000000000000..7d7d22a650b0 --- /dev/null +++ b/drivers/hwmon/nct6694-hwmon.c @@ -0,0 +1,407 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Nuvoton NCT6694 HWMON driver based on USB interface. + * + * Copyright (C) 2024 Nuvoton Technology Corp. + */ + +#include <linux/slab.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/hwmon.h> +#include <linux/platform_device.h> +#include <linux/mfd/nct6694.h> + +#define DRVNAME "nct6694-hwmon" + +/* Host interface */ +#define REQUEST_RPT_MOD 0xFF +#define REQUEST_HWMON_MOD 0x00 + +/* Report Channel */ +#define HWMON_FIN_IDX(x) (0x50 + ((x) * 2)) +#define HWMON_FIN_STS(x) (0x6E + (x)) +#define HWMON_PWM_IDX(x) (0x70 + (x)) + +/* Message Channel*/ +/* Command 00h */ +#define REQUEST_HWMON_CMD0_LEN 0x40 +#define REQUEST_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ +#define HWMON_FIN_EN(x) (0x04 + (x)) +#define HWMON_PWM_FREQ_IDX(x) (0x30 + (x)) +/* Command 02h */ +#define REQUEST_HWMON_CMD2_LEN 0x90 +#define REQUEST_HWMON_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ +#define HWMON_SMI_CTRL_IDX 0x00 +#define HWMON_FIN_LIMIT_IDX(x) (0x70 + ((x) * 2)) +#define HWMON_CMD2_HYST_MASK 0x1F +/* Command 03h */ +#define REQUEST_HWMON_CMD3_LEN 0x08 +#define REQUEST_HWMON_CMD3_OFFSET 0x0003 /* OFFSET = SEL|CMD */ + +struct nct6694_hwmon_data { + struct nct6694 *nct6694; + + /* Make sure read & write commands are consecutive */ + struct mutex hwmon_lock; +}; + +#define NCT6694_HWMON_FAN_CONFIG (HWMON_F_ENABLE | HWMON_F_INPUT | \ + HWMON_F_MIN | HWMON_F_MIN_ALARM) +#define NCT6694_HWMON_PWM_CONFIG (HWMON_PWM_INPUT | HWMON_PWM_FREQ) + +static const struct hwmon_channel_info *nct6694_info[] = { + HWMON_CHANNEL_INFO(fan, + NCT6694_HWMON_FAN_CONFIG, /* FIN0 */ + NCT6694_HWMON_FAN_CONFIG, /* FIN1 */ + NCT6694_HWMON_FAN_CONFIG, /* FIN2 */ + NCT6694_HWMON_FAN_CONFIG, /* FIN3 */ + NCT6694_HWMON_FAN_CONFIG, /* FIN4 */ + NCT6694_HWMON_FAN_CONFIG, /* FIN5 */ + NCT6694_HWMON_FAN_CONFIG, /* FIN6 */ + NCT6694_HWMON_FAN_CONFIG, /* FIN7 */ + NCT6694_HWMON_FAN_CONFIG, /* FIN8 */ + NCT6694_HWMON_FAN_CONFIG), /* FIN9 */ + + HWMON_CHANNEL_INFO(pwm, + NCT6694_HWMON_PWM_CONFIG, /* PWM0 */ + NCT6694_HWMON_PWM_CONFIG, /* PWM1 */ + NCT6694_HWMON_PWM_CONFIG, /* PWM2 */ + NCT6694_HWMON_PWM_CONFIG, /* PWM3 */ + NCT6694_HWMON_PWM_CONFIG, /* PWM4 */ + NCT6694_HWMON_PWM_CONFIG, /* PWM5 */ + NCT6694_HWMON_PWM_CONFIG, /* PWM6 */ + NCT6694_HWMON_PWM_CONFIG, /* PWM7 */ + NCT6694_HWMON_PWM_CONFIG, /* PWM8 */ + NCT6694_HWMON_PWM_CONFIG), /* PWM9 */ + NULL +}; + +static int nct6694_fan_read(struct device *dev, u32 attr, int channel, + long *val) +{ + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); + unsigned char buf[2]; + int ret; + + switch (attr) { + case hwmon_fan_enable: + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD0_OFFSET, + REQUEST_HWMON_CMD0_LEN, + HWMON_FIN_EN(channel / 8), + 1, buf); + if (ret) + return -EINVAL; + + *val = buf[0] & BIT(channel % 8) ? 1 : 0; + + break; + + case hwmon_fan_input: + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, + HWMON_FIN_IDX(channel), 2, 0, + 2, buf); + if (ret) + return -EINVAL; + + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; + + break; + + case hwmon_fan_min: + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD2_OFFSET, + REQUEST_HWMON_CMD2_LEN, + HWMON_FIN_LIMIT_IDX(channel), + 2, buf); + if (ret) + return -EINVAL; + + *val = (buf[1] | (buf[0] << 8)) & 0xFFFF; + + break; + + case hwmon_fan_min_alarm: + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, + HWMON_FIN_STS(channel / 8), + 1, 0, 1, buf); + if (ret) + return -EINVAL; + + *val = buf[0] & BIT(channel % 8); + + break; + + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static int nct6694_pwm_read(struct device *dev, u32 attr, int channel, + long *val) +{ + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); + unsigned char buf; + int ret; + + switch (attr) { + case hwmon_pwm_input: + ret = nct6694_read_msg(data->nct6694, REQUEST_RPT_MOD, + HWMON_PWM_IDX(channel), + 1, 0, 1, &buf); + if (ret) + return -EINVAL; + + *val = buf; + + break; + case hwmon_pwm_freq: + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD0_OFFSET, + REQUEST_HWMON_CMD0_LEN, + HWMON_PWM_FREQ_IDX(channel), + 1, &buf); + if (ret) + return -EINVAL; + + *val = buf * 25000 / 255; + + break; + + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static int nct6694_fan_write(struct device *dev, u32 attr, int channel, + long val) +{ + struct nct6694_hwmon_data *data = dev_get_drvdata(dev); + unsigned char enable_buf[REQUEST_HWMON_CMD0_LEN] = {0}; + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; + u16 fan_val = (u16)val; + int ret; + + switch (attr) { + case hwmon_fan_enable: + mutex_lock(&data->hwmon_lock); + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD0_OFFSET, + REQUEST_HWMON_CMD0_LEN, 0, + REQUEST_HWMON_CMD0_LEN, + enable_buf); + if (ret) + goto err; + + if (val) + enable_buf[HWMON_FIN_EN(channel / 8)] |= BIT(channel % 8); + else + enable_buf[HWMON_FIN_EN(channel / 8)] &= ~BIT(channel % 8); + + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD0_OFFSET, + REQUEST_HWMON_CMD0_LEN, enable_buf); + if (ret) + goto err; + + break; + + case hwmon_fan_min: + mutex_lock(&data->hwmon_lock); + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD2_OFFSET, + REQUEST_HWMON_CMD2_LEN, 0, + REQUEST_HWMON_CMD2_LEN, buf); + if (ret) + goto err; + + buf[HWMON_FIN_LIMIT_IDX(channel)] = (u8)((fan_val >> 8) & 0xFF); + buf[HWMON_FIN_LIMIT_IDX(channel) + 1] = (u8)(fan_val & 0xFF); + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD2_OFFSET, + REQUEST_HWMON_CMD2_LEN, buf); + if (ret) + goto err; + + break; + + default: + ret = -EOPNOTSUPP; + goto err; + } + +err: + mutex_unlock(&data->hwmon_lock); + return ret; +} + +static int nct6694_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + switch (type) { + case hwmon_fan: /* in RPM */ + return nct6694_fan_read(dev, attr, channel, val); + + case hwmon_pwm: /* in value 0~255 */ + return nct6694_pwm_read(dev, attr, channel, val); + + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static int nct6694_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long val) +{ + switch (type) { + case hwmon_fan: + return nct6694_fan_write(dev, attr, channel, val); + default: + return -EOPNOTSUPP; + } + + return 0; +} + +static umode_t nct6694_is_visible(const void *data, enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_fan: + switch (attr) { + case hwmon_fan_enable: + case hwmon_fan_min: + return 0644; + + case hwmon_fan_input: + case hwmon_fan_min_alarm: + return 0444; + + default: + return 0; + } + + case hwmon_pwm: + switch (attr) { + case hwmon_pwm_input: + case hwmon_pwm_freq: + return 0444; + default: + return 0; + } + + default: + return 0; + } + + return 0; +} + +static const struct hwmon_ops nct6694_hwmon_ops = { + .is_visible = nct6694_is_visible, + .read = nct6694_read, + .write = nct6694_write, +}; + +static const struct hwmon_chip_info nct6694_chip_info = { + .ops = &nct6694_hwmon_ops, + .info = nct6694_info, +}; + +static int nct6694_hwmon_init(struct nct6694_hwmon_data *data) +{ + unsigned char buf[REQUEST_HWMON_CMD2_LEN] = {0}; + int ret; + + /* Set Fan input Real Time alarm mode */ + mutex_lock(&data->hwmon_lock); + ret = nct6694_read_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD2_OFFSET, + REQUEST_HWMON_CMD2_LEN, 0, + REQUEST_HWMON_CMD2_LEN, buf); + if (ret) + goto err; + + buf[HWMON_SMI_CTRL_IDX] = 0x02; + + ret = nct6694_write_msg(data->nct6694, REQUEST_HWMON_MOD, + REQUEST_HWMON_CMD2_OFFSET, + REQUEST_HWMON_CMD2_LEN, buf); + if (ret) + goto err; + +err: + mutex_unlock(&data->hwmon_lock); + return ret; +} + +static int nct6694_hwmon_probe(struct platform_device *pdev) +{ + struct nct6694_hwmon_data *data; + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); + struct device *hwmon_dev; + int ret; + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->nct6694 = nct6694; + mutex_init(&data->hwmon_lock); + platform_set_drvdata(pdev, data); + + ret = nct6694_hwmon_init(data); + if (ret) + return -EIO; + + /* Register hwmon device to HWMON framework */ + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, + "nct6694", data, + &nct6694_chip_info, + NULL); + if (IS_ERR(hwmon_dev)) { + dev_err(&pdev->dev, "%s: Failed to register hwmon device!\n", + __func__); + return PTR_ERR(hwmon_dev); + } + + return 0; +} + +static struct platform_driver nct6694_hwmon_driver = { + .driver = { + .name = DRVNAME, + }, + .probe = nct6694_hwmon_probe, +}; + +static int __init nct6694_init(void) +{ + int err; + + err = platform_driver_register(&nct6694_hwmon_driver); + if (!err) { + if (err) + platform_driver_unregister(&nct6694_hwmon_driver); + } + + return err; +} +subsys_initcall(nct6694_init); + +static void __exit nct6694_exit(void) +{ + platform_driver_unregister(&nct6694_hwmon_driver); +} +module_exit(nct6694_exit); + +MODULE_DESCRIPTION("USB-HWMON driver for NCT6694"); +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); +MODULE_LICENSE("GPL");
This driver supports Hardware monitor functionality for NCT6694 MFD device based on USB interface. Signed-off-by: Ming Yu <tmyu0@nuvoton.com> --- MAINTAINERS | 1 + drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/nct6694-hwmon.c | 407 ++++++++++++++++++++++++++++++++++ 4 files changed, 419 insertions(+) create mode 100644 drivers/hwmon/nct6694-hwmon.c