Re: [PATCH v3 6/7] hwmon: Add Nuvoton NCT6694 HWMON support
From: Vincent Mailhol
Date: Thu Dec 12 2024 - 11:44:54 EST
On 10/12/2024 at 19:45, Ming Yu wrote:
> This driver supports Hardware monitor functionality for NCT6694 MFD
> device based on USB interface.
>
> Signed-off-by: Ming Yu <tmyu0@xxxxxxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/nct6694-hwmon.c | 768 ++++++++++++++++++++++++++++++++++
> 4 files changed, 780 insertions(+)
> create mode 100644 drivers/hwmon/nct6694-hwmon.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 496fe7d5a23f..d6414eea0463 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16546,6 +16546,7 @@ M: Ming Yu <tmyu0@xxxxxxxxxxx>
> L: linux-kernel@xxxxxxxxxxxxxxx
> 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 dd376602f3f1..df40986424bd 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1636,6 +1636,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 b827b92f2a78..27a43e67cdb7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -168,6 +168,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..b2320d64090b
> --- /dev/null
> +++ b/drivers/hwmon/nct6694-hwmon.c
> @@ -0,0 +1,768 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nuvoton NCT6694 HWMON driver based on USB interface.
> + *
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/nct6694.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* Host interface */
> +#define NCT6694_RPT_MOD 0xFF
> +#define NCT6694_HWMON_MOD 0x00
> +#define NCT6694_PWM_MOD 0x01
> +
> +/* Report Channel */
> +#define NCT6694_VIN_IDX(x) (0x00 + (x))
> +#define NCT6694_TIN_IDX(x) \
> + ({ typeof(x) (_x) = (x); \
> + ((_x) < 10) ? (0x10 + ((_x) * 2)) : \
> + (0x30 + (((_x) - 10) * 2)); })
> +#define NCT6694_FIN_IDX(x) (0x50 + ((x) * 2))
> +#define NCT6694_PWM_IDX(x) (0x70 + (x))
> +#define NCT6694_VIN_STS(x) (0x68 + (x))
> +#define NCT6694_TIN_STS(x) (0x6A + (x))
> +#define NCT6694_FIN_STS(x) (0x6E + (x))
> +
> +/* Message Channel*/
> +/* HWMON Command */
> +/* Command 00h */
> +#define NCT6694_HWMON_CMD0_LEN 0x40
> +#define NCT6694_HWMON_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */
> +#define NCT6694_VIN_EN(x) (0x00 + (x))
> +#define NCT6694_TIN_EN(x) (0x02 + (x))
> +#define NCT6694_FIN_EN(x) (0x04 + (x))
> +#define NCT6694_PWM_EN(x) (0x06 + (x))
> +#define NCT6694_PWM_FREQ_IDX(x) (0x30 + (x))
> +/* Command 02h */
> +#define NCT6694_HWMON_CMD2_LEN 0x90
> +#define NCT6694_HWMON_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */
> +#define NCT6694_SMI_CTRL_IDX 0x00
> +#define NCT6694_VIN_HL(x) (0x10 + ((x) * 2))
> +#define NCT6694_VIN_LL(x) (0x11 + ((x) * 2))
> +#define NCT6694_TIN_HYST(x) (0x30 + ((x) * 2))
> +#define NCT6694_TIN_HL(x) (0x31 + ((x) * 2))
> +#define NCT6694_FIN_HL(x) (0x70 + ((x) * 2))
> +#define NCT6694_FIN_LL(x) (0x71 + ((x) * 2))
> +/* PWM Command */
> +#define NCT6694_PWM_CMD1_LEN 0x18
> +#define NCT6694_PWM_CMD1_OFFSET 0x0001
> +#define NCT6694_MAL_VAL(x) (0x02 + (x))
> +
> +#define NCT6694_FREQ_FROM_REG(reg) ((reg) * 25000 / 255)
> +#define NCT6694_FREQ_TO_REG(val) \
> + (DIV_ROUND_CLOSEST(clamp_val((val), 100, 25000) * 255, 25000))
> +
> +#define NCT6694_LSB_REG_MASK GENMASK(7, 5)
> +#define NCT6694_TIN_HYST_MASK GENMASK(7, 5)
> +
> +static inline long in_from_reg(u8 reg)
> +{
> + return reg * 16;
> +}
> +
> +static inline u8 in_to_reg(long val)
> +{
> + if (val <= 0)
> + return 0;
> + return val / 16;
> +}
> +
> +static inline long temp_from_reg(u8 reg)
> +{
> + return reg * 1000;
> +}
> +
> +static inline u8 temp_to_reg(long val)
> +{
> + return val / 1000;
> +}
> +
> +struct nct6694_hwmon_data {
> + struct nct6694 *nct6694;
> + struct mutex lock;
> + unsigned char *xmit_buf;
> + unsigned char hwmon_en[NCT6694_HWMON_CMD0_LEN];
> +};
A global comment on this series: do not declare your buffers as some
opaque unsigned char arrays. Instead, make it a structure (or an array
of structures if needed) using the little endian types for the
different fields.
You already applied this change to the CAN driver after I made a
comment, please do the same throughout the series.
The same applies with any other comments made by anyone else: do not
only apply to the patch where the comment is made, but apply it
broadly to the series.
Thank you.
Yours sincerely,
Vincent Mailhol