Re: [PATCH v3 6/7] hwmon: Add Nuvoton NCT6694 HWMON support

[Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>]

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