[v4,3/3] hwmon: Driver for Nuvoton NCT7363Y

  From: Ban Feng <kcfeng0@nuvoton.com>

NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.

Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
---
 Documentation/hwmon/index.rst   |   1 +
 Documentation/hwmon/nct7363.rst |  33 +++
 MAINTAINERS                     |   2 +
 drivers/hwmon/Kconfig           |  11 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/nct7363.c         | 412 ++++++++++++++++++++++++++++++++
 6 files changed, 460 insertions(+)
 create mode 100644 Documentation/hwmon/nct7363.rst
 create mode 100644 drivers/hwmon/nct7363.c
  

Dear Ban,


Thank you for your patch. Some minor things from my side.


Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
> From: Ban Feng <kcfeng0@nuvoton.com>
> 
> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.

Please reference the datasheet.

Could you please give a high level description of the driver design?

I’d also add a verb (in imperative mood) to the commit message summary, 
so it’s a statement:

Add driver for I2C chip Nuvoton NCT7363Y

> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> ---
>   Documentation/hwmon/index.rst   |   1 +
>   Documentation/hwmon/nct7363.rst |  33 +++
>   MAINTAINERS                     |   2 +
>   drivers/hwmon/Kconfig           |  11 +
>   drivers/hwmon/Makefile          |   1 +
>   drivers/hwmon/nct7363.c         | 412 ++++++++++++++++++++++++++++++++
>   6 files changed, 460 insertions(+)
>   create mode 100644 Documentation/hwmon/nct7363.rst
>   create mode 100644 drivers/hwmon/nct7363.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index c7ed1f73ac06..302f954b45be 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
>      mp5990
>      nct6683
>      nct6775
> +   nct7363
>      nct7802
>      nct7904
>      npcm750-pwm-fan
> diff --git a/Documentation/hwmon/nct7363.rst b/Documentation/hwmon/nct7363.rst
> new file mode 100644
> index 000000000000..89699c95aa4b
> --- /dev/null
> +++ b/Documentation/hwmon/nct7363.rst
> @@ -0,0 +1,33 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver nct7363
> +=====================
> +
> +Supported chip:
> +
> +  * Nuvoton NCT7363Y
> +
> +    Prefix: nct7363
> +
> +    Addresses: I2C 0x20, 0x21, 0x22, 0x23
> +
> +Author: Ban Feng <kcfeng0@nuvoton.com>
> +
> +
> +Description
> +-----------
> +
> +The NCT7363Y is a Fan controller which provides up to 16 independent

fan controller

> +FAN input monitors, and up to 16 independent PWM output with SMBus interface.

output*s*

> +
> +
> +Sysfs entries
> +-------------
> +
> +Currently, the driver supports the following features:
> +
> +======================= =======================================================
> +fanX_input		provide current fan rotation value in RPM
> +
> +pwmX			get or set PWM fan control value.
> +======================= =======================================================

In the diff view this does not align with the “holes”.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b1efefed7c4..7ca66b713e30 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15089,6 +15089,8 @@ M:	Ban Feng <kcfeng0@nuvoton.com>
>   L:	linux-hwmon@vger.kernel.org
>   S:	Maintained
>   F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> +F:	Documentation/hwmon/nct7363.rst
> +F:	drivers/hwmon/nct7363.c
>   
>   NETDEVSIM
>   M:	Jakub Kicinski <kuba@kernel.org>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index a608264da87d..a297b5409b04 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1616,6 +1616,17 @@ config SENSORS_NCT6775_I2C
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nct6775-i2c.
>   
> +config SENSORS_NCT7363
> +	tristate "Nuvoton NCT7363Y"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for the Nuvoton NCT7363Y,

The comma is not needed.

> +	  hardware monitoring chip.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called nct7363.
> +
>   config SENSORS_NCT7802
>   	tristate "Nuvoton NCT7802Y"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 47be39af5c03..d5e7531204df 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o
>   nct6775-objs			:= nct6775-platform.o
>   obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
>   obj-$(CONFIG_SENSORS_NCT6775_I2C) += nct6775-i2c.o
> +obj-$(CONFIG_SENSORS_NCT7363)	+= nct7363.o
>   obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
>   obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
>   obj-$(CONFIG_SENSORS_NPCM7XX)	+= npcm750-pwm-fan.o
> diff --git a/drivers/hwmon/nct7363.c b/drivers/hwmon/nct7363.c
> new file mode 100644
> index 000000000000..c79d3ca4f111
> --- /dev/null
> +++ b/drivers/hwmon/nct7363.c
> @@ -0,0 +1,412 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Nuvoton Technology corporation.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define NCT7363_REG_GPIO_0_3		0x20
> +#define NCT7363_REG_GPIO_4_7		0x21
> +#define NCT7363_REG_GPIO_10_13		0x22
> +#define NCT7363_REG_GPIO_14_17		0x23
> +#define NCT7363_REG_PWMEN_0_7		0x38
> +#define NCT7363_REG_PWMEN_8_15		0x39
> +#define NCT7363_REG_FANINEN_0_7		0x41
> +#define NCT7363_REG_FANINEN_8_15	0x42
> +#define NCT7363_REG_FANINX_HVAL(x)	(0x48 + ((x) * 2))
> +#define NCT7363_REG_FANINX_LVAL(x)	(0x49 + ((x) * 2))
> +#define NCT7363_REG_FSCPXDUTY(x)	(0x90 + ((x) * 2))
> +
> +#define PWM_SEL(x)			(BIT(0) << (((x) % 4) * 2))
> +#define FANIN_SEL(x)			(BIT(1) << (((x) % 4) * 2))
> +
> +#define NCT7363_FANINX_LVAL_MASK	GENMASK(4, 0)
> +#define NCT7363_FANIN_MASK		GENMASK(12, 0)
> +
> +#define NCT7363_PWM_COUNT		16
> +
> +static inline unsigned long FAN_FROM_REG(u16 val)
> +{
> +	/* In case fan is stopped or divide by 0 */
> +	if (val == NCT7363_FANIN_MASK || val == 0)

The comment seems redundant. Maybe also apply the mask to `val` and 
check the result.

> +		return	0;
> +
> +	return (1350000UL / val);

Why does the mask not to be applied to `val`?

> +}
> +
> +static const struct of_device_id nct7363_of_match[] = {
> +	{ .compatible = "nuvoton,nct7363" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, nct7363_of_match);
> +
> +struct nct7363_data {
> +	struct regmap		*regmap;
> +	struct mutex		lock; /* protect register access */
> +
> +	u16			fanin_mask;
> +	u16			pwm_mask;
> +};
> +
> +static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
> +			    long *val)
> +{
> +	struct nct7363_data *data = dev_get_drvdata(dev);
> +	unsigned int hi, lo;
> +	u16 cnt, rpm;
> +	int ret = 0;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		/*
> +		 * High-byte register should be read first to latch
> +		 * synchronous low-byte value
> +		 */
> +		ret = regmap_read(data->regmap,
> +				  NCT7363_REG_FANINX_HVAL(channel), &hi);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(data->regmap,
> +			NCT7363_REG_FANINX_LVAL(channel), &lo);
> +		if (ret)
> +			return ret;
> +
> +		cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
> +		rpm = FAN_FROM_REG(cnt);
> +		*val = (long)rpm;

Could `FAN_FROM_REG()` return long, so the cast here is not needed?

> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t nct7363_fan_is_visible(const void *_data, u32 attr, int channel)
> +{
> +	const struct nct7363_data *data = _data;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		if (data->fanin_mask & BIT(channel))
> +			return 0444;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int nct7363_read_pwm(struct device *dev, u32 attr, int channel,
> +			    long *val)
> +{
> +	struct nct7363_data *data = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	u16 ret;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		ret = regmap_read(data->regmap,
> +				  NCT7363_REG_FSCPXDUTY(channel), &regval);

     int regmap_read(struct regmap *map, unsigned int reg, unsigned int 
val);

So `ret` should be `int`?


Kind regards,

Paul


> +		if (ret)
> +			return ret;
> +
> +		*val = (long)regval;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int nct7363_write_pwm(struct device *dev, u32 attr, int channel,
> +			     long val)
> +{
> +	struct nct7363_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		if (val < 0 || val > 255)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->lock);
> +		ret = regmap_write(data->regmap,
> +				   NCT7363_REG_FSCPXDUTY(channel), val);
> +		mutex_unlock(&data->lock);
> +
> +		return ret;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t nct7363_pwm_is_visible(const void *_data, u32 attr, int channel)
> +{
> +	const struct nct7363_data *data = _data;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		if (data->pwm_mask & BIT(channel))
> +			return 0644;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int nct7363_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		return nct7363_read_fan(dev, attr, channel, val);
> +	case hwmon_pwm:
> +		return nct7363_read_pwm(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int nct7363_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_pwm:
> +		return nct7363_write_pwm(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t nct7363_is_visible(const void *data,
> +				  enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		return nct7363_fan_is_visible(data, attr, channel);
> +	case hwmon_pwm:
> +		return nct7363_pwm_is_visible(data, attr, channel);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static const struct hwmon_channel_info *nct7363_info[] = {
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops nct7363_hwmon_ops = {
> +	.is_visible = nct7363_is_visible,
> +	.read = nct7363_read,
> +	.write = nct7363_write,
> +};
> +
> +static const struct hwmon_chip_info nct7363_chip_info = {
> +	.ops = &nct7363_hwmon_ops,
> +	.info = nct7363_info,
> +};
> +
> +static int nct7363_init_chip(struct nct7363_data *data)
> +{
> +	u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
> +	int ret;
> +
> +	for (i = 0; i < NCT7363_PWM_COUNT; i++) {
> +		if (i < 4) {
> +			if (data->pwm_mask & BIT(i))
> +				gpio0_3 |= PWM_SEL(i);
> +			if (data->fanin_mask & BIT(i))
> +				gpio10_13 |= FANIN_SEL(i);
> +		} else if (i < 8) {
> +			if (data->pwm_mask & BIT(i))
> +				gpio4_7 |= PWM_SEL(i);
> +			if (data->fanin_mask & BIT(i))
> +				gpio14_17 |= FANIN_SEL(i);
> +		} else if (i < 12) {
> +			if (data->pwm_mask & BIT(i))
> +				gpio10_13 |= PWM_SEL(i);
> +			if (data->fanin_mask & BIT(i))
> +				gpio0_3 |= FANIN_SEL(i);
> +		} else {
> +			if (data->pwm_mask & BIT(i))
> +				gpio14_17 |= PWM_SEL(i);
> +			if (data->fanin_mask & BIT(i))
> +				gpio4_7 |= FANIN_SEL(i);
> +		}
> +	}
> +
> +	/* Pin Function Configuration */
> +	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* PWM and FANIN Monitoring Enable */
> +	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
> +			   data->pwm_mask & 0xff);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
> +			   (data->pwm_mask >> 8) & 0xff);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
> +			   data->fanin_mask & 0xff);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
> +			   (data->fanin_mask >> 8) & 0xff);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int nct7363_present_pwm_fanin(struct device *dev,
> +				     struct device_node *child,
> +				     struct nct7363_data *data)
> +{
> +	struct of_phandle_args args;
> +	int ret, fanin_cnt;
> +	u8 *fanin_ch;
> +	u8 ch, index;
> +
> +	ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
> +					 0, &args);
> +	if (ret)
> +		return ret;
> +
> +	data->pwm_mask |= BIT(args.args[0]);
> +
> +	fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
> +	if (fanin_cnt < 1)
> +		return -EINVAL;
> +
> +	fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);
> +	if (!fanin_ch)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u8_array(child, "tach-ch", fanin_ch, fanin_cnt);
> +	if (ret)
> +		return ret;
> +
> +	for (ch = 0; ch < fanin_cnt; ch++) {
> +		index = fanin_ch[ch];
> +		data->fanin_mask |= BIT(index);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config nct7363_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int nct7363_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device_node *child;
> +	struct nct7363_data *data;
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	mutex_init(&data->lock);
> +
> +	for_each_child_of_node(dev->of_node, child) {
> +		ret = nct7363_present_pwm_fanin(dev, child, data);
> +		if (ret) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +	}
> +
> +	/* Initialize the chip */
> +	ret = nct7363_init_chip(data);
> +	if (ret)
> +		return ret;
> +
> +	hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, client->name, data,
> +						     &nct7363_chip_info, NULL);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct i2c_driver nct7363_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name = "nct7363",
> +		.of_match_table = nct7363_of_match,
> +	},
> +	.probe = nct7363_probe,
> +};
> +
> +module_i2c_driver(nct7363_driver);
> +
> +MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
> +MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
> +MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
> +MODULE_LICENSE("GPL");
  

On 2/27/24 23:57, Paul Menzel wrote:
> Dear Ban,
> 
> 
> Thank you for your patch. Some minor things from my side.
> 
> 
> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
>> From: Ban Feng <kcfeng0@nuvoton.com>
>>
>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
> 
> Please reference the datasheet.
> 

Note that something like

Datasheet: Available from Nuvoton upon request

is quite common for hardware monitoring chips and acceptable.

> Could you please give a high level description of the driver design?
> 

Can you be more specific ? I didn't have time yet to look into details,
but at first glance this looks like a standard hardware monitoring driver.
One could argue that the high level design of such drivers is described
in Documentation/hwmon/hwmon-kernel-api.rst.
I don't usually ask for a additional design information for hwmon drivers
unless some chip interaction is unusual and needs to be explained,
and then I prefer to have it explained in the code. Given that, I am
quite curious and would like to understand what you are looking for.

Thanks,
Guenter
  

Dear Guenter,


Am 28.02.24 um 10:03 schrieb Guenter Roeck:
> On 2/27/24 23:57, Paul Menzel wrote:

>> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
>>> From: Ban Feng <kcfeng0@nuvoton.com>
>>>
>>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>>
>> Please reference the datasheet.
> 
> Note that something like
> 
> Datasheet: Available from Nuvoton upon request
> 
> is quite common for hardware monitoring chips and acceptable.

Yes, it would be nice to document it though. (And finally for vendors to 
just make them available for download.)

>> Could you please give a high level description of the driver design?
> 
> Can you be more specific ? I didn't have time yet to look into details,
> but at first glance this looks like a standard hardware monitoring driver.
> One could argue that the high level design of such drivers is described
> in Documentation/hwmon/hwmon-kernel-api.rst.
> 
> I don't usually ask for a additional design information for hwmon drivers
> unless some chip interaction is unusual and needs to be explained,
> and then I prefer to have it explained in the code. Given that, I am
> quite curious and would like to understand what you are looking for.
For a 10+ lines commit, in my opinion the commit message should say 
something about the implementation. Even it is just, as you wrote, a 
note, that it follows the standard design.


Kind regards,

Paul
  

On 2/28/24 03:03, Paul Menzel wrote:
> Dear Guenter,
> 
> 
> Am 28.02.24 um 10:03 schrieb Guenter Roeck:
>> On 2/27/24 23:57, Paul Menzel wrote:
> 
>>> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
>>>> From: Ban Feng <kcfeng0@nuvoton.com>
>>>>
>>>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>>>
>>> Please reference the datasheet.
>>
>> Note that something like
>>
>> Datasheet: Available from Nuvoton upon request
>>
>> is quite common for hardware monitoring chips and acceptable.
> 
> Yes, it would be nice to document it though. (And finally for vendors to just make them available for download.)
> 

Nuvoton is nice enough and commonly makes datasheets available on request.
The only exception I have seen so far is where they were forced into an NDA
by a large chip and board vendor, which prevented them from publishing a
specific datasheet.

Others are much worse. Many PMIC vendors don't publish their datasheets at
all, and sometimes chips don't even officially exist (notorious for chips
intended for the automotive market). Just look at the whole discussion
around MAX31335.

Anyway, there are lots of examples in Documentation/hwmon/. I don't see
the need to add further documentation, and I specifically don't want to
make it official that "Datasheet not public" is acceptable as well.
We really don't have a choice unless we want to exclude a whole class
of chips from the kernel, but that doesn't make it better.

>>> Could you please give a high level description of the driver design?
>>
>> Can you be more specific ? I didn't have time yet to look into details,
>> but at first glance this looks like a standard hardware monitoring driver.
>> One could argue that the high level design of such drivers is described
>> in Documentation/hwmon/hwmon-kernel-api.rst.
>>
>> I don't usually ask for a additional design information for hwmon drivers
>> unless some chip interaction is unusual and needs to be explained,
>> and then I prefer to have it explained in the code. Given that, I am
>> quite curious and would like to understand what you are looking for.
> For a 10+ lines commit, in my opinion the commit message should say something about the implementation. Even it is just, as you wrote, a note, that it follows the standard design.
> 

Again, I have not looked into the submission, but usually we ask for that
to be documented in Documentation/hwmon/. I find that much better than
a soon-to-be-forgotten commit message. I don't mind something like
"The NCT7363Y is a fan controller with up to 16 independent fan input
  monitors and up to 16 independent PWM outputs. It also supports up
  to 16 GPIO pins"
or in other words a description of the chip, not the implementation.
That a driver hwmon driver uses the hardware monitoring API seems to be
obvious to me, so I don't see the value of adding it to the commit
description. I would not mind having something there, but I don't
see it as mandatory.

On the  other side, granted, that is just _my_ personal opinion.
Do we have a common guideline for what exactly should be in commit
descriptions for driver submissions ? I guess I should look that up.

Thanks,
Guenter
  

Dear Guenter,


Thank you for your reply.

Am 28.02.24 um 17:03 schrieb Guenter Roeck:
> On 2/28/24 03:03, Paul Menzel wrote:

>> Am 28.02.24 um 10:03 schrieb Guenter Roeck:
>>> On 2/27/24 23:57, Paul Menzel wrote:
>>
>>>> Am 27.02.24 um 01:56 schrieb baneric926@gmail.com:
>>>>> From: Ban Feng <kcfeng0@nuvoton.com>
>>>>>
>>>>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>>>>
>>>> Please reference the datasheet.
>>>
>>> Note that something like
>>>
>>> Datasheet: Available from Nuvoton upon request
>>>
>>> is quite common for hardware monitoring chips and acceptable.
>>
>> Yes, it would be nice to document it though. (And finally for vendors 
>> to just make them available for download.)
> 
> Nuvoton is nice enough and commonly makes datasheets available on request.
> The only exception I have seen so far is where they were forced into an NDA
> by a large chip and board vendor, which prevented them from publishing a
> specific datasheet.

Nice, that they are better in this regard than others.

> Others are much worse. Many PMIC vendors don't publish their datasheets at
> all, and sometimes chips don't even officially exist (notorious for chips
> intended for the automotive market). Just look at the whole discussion
> around MAX31335.
> 
> Anyway, there are lots of examples in Documentation/hwmon/. I don't see
> the need to add further documentation, and I specifically don't want to
> make it official that "Datasheet not public" is acceptable as well.
> We really don't have a choice unless we want to exclude a whole class
> of chips from the kernel, but that doesn't make it better.

I know folks figure it out eventually, but I found it helpful to have 
the datesheet name in the commit message to know what to search for, ask 
for, or in case of difference between datasheet revision what to compare 
against.

>>>> Could you please give a high level description of the driver design?
>>>
>>> Can you be more specific ? I didn't have time yet to look into details,
>>> but at first glance this looks like a standard hardware monitoring 
>>> driver.
>>> One could argue that the high level design of such drivers is described
>>> in Documentation/hwmon/hwmon-kernel-api.rst.
>>>
>>> I don't usually ask for a additional design information for hwmon drivers
>>> unless some chip interaction is unusual and needs to be explained,
>>> and then I prefer to have it explained in the code. Given that, I am
>>> quite curious and would like to understand what you are looking for.
>> For a 10+ lines commit, in my opinion the commit message should say 
>> something about the implementation. Even it is just, as you wrote, a 
>> note, that it follows the standard design.
> 
> Again, I have not looked into the submission, but usually we ask for that
> to be documented in Documentation/hwmon/. I find that much better than
> a soon-to-be-forgotten commit message. I don't mind something like
> "The NCT7363Y is a fan controller with up to 16 independent fan input
>   monitors and up to 16 independent PWM outputs. It also supports up
>   to 16 GPIO pins"
> or in other words a description of the chip, not the implementation.
> That a driver hwmon driver uses the hardware monitoring API seems to be
> obvious to me, so I don't see the value of adding it to the commit
> description. I would not mind having something there, but I don't
> see it as mandatory.
> 
> On the  other side, granted, that is just _my_ personal opinion.
> Do we have a common guideline for what exactly should be in commit
> descriptions for driver submissions ? I guess I should look that up.

`Documentation/hwmon/submitting-patches.rst` refers to 
`Documentation/process/submitting-patches.rst`, and there *Describe your 
changes* seems to have been written for documenting bug fixes or 
enhancements and not new additions. It for example contains:

> Once the problem is established, describe what you are actually doing
> about it in technical detail.  It's important to describe the change
> in plain English for the reviewer to verify that the code is behaving
> as you intend it to.

I agree with your description, but I am also convinced if you write 500 
lines of code, that you can write ten lines of commit messages giving a 
broad overview. In this case, saying that it follows the standard driver 
model would be good enough for me.

Also, at least for me, often having to bisect stuff and using `git 
blame` to look at old commits, commit messages are very valuable to me, 
and not “forgotten”. ;-)


Kind regards,

Paul
  

On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
>From: Ban Feng <kcfeng0@nuvoton.com>
>
>NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
>
>Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>---
> Documentation/hwmon/index.rst   |   1 +
> Documentation/hwmon/nct7363.rst |  33 +++
> MAINTAINERS                     |   2 +
> drivers/hwmon/Kconfig           |  11 +
> drivers/hwmon/Makefile          |   1 +
> drivers/hwmon/nct7363.c         | 412 ++++++++++++++++++++++++++++++++
> 6 files changed, 460 insertions(+)
> create mode 100644 Documentation/hwmon/nct7363.rst
> create mode 100644 drivers/hwmon/nct7363.c
>
>diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>index c7ed1f73ac06..302f954b45be 100644
>--- a/Documentation/hwmon/index.rst
>+++ b/Documentation/hwmon/index.rst
>@@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
>    mp5990
>    nct6683
>    nct6775
>+   nct7363
>    nct7802
>    nct7904
>    npcm750-pwm-fan
>diff --git a/Documentation/hwmon/nct7363.rst b/Documentation/hwmon/nct7363.rst
>new file mode 100644
>index 000000000000..89699c95aa4b
>--- /dev/null
>+++ b/Documentation/hwmon/nct7363.rst
>@@ -0,0 +1,33 @@
>+.. SPDX-License-Identifier: GPL-2.0
>+
>+Kernel driver nct7363
>+=====================
>+
>+Supported chip:
>+
>+  * Nuvoton NCT7363Y
>+
>+    Prefix: nct7363
>+
>+    Addresses: I2C 0x20, 0x21, 0x22, 0x23
>+
>+Author: Ban Feng <kcfeng0@nuvoton.com>
>+
>+
>+Description
>+-----------
>+
>+The NCT7363Y is a Fan controller which provides up to 16 independent
>+FAN input monitors, and up to 16 independent PWM output with SMBus interface.
>+
>+
>+Sysfs entries
>+-------------
>+
>+Currently, the driver supports the following features:
>+
>+======================= =======================================================
>+fanX_input		provide current fan rotation value in RPM
>+
>+pwmX			get or set PWM fan control value.
>+======================= =======================================================
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 7b1efefed7c4..7ca66b713e30 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -15089,6 +15089,8 @@ M:	Ban Feng <kcfeng0@nuvoton.com>
> L:	linux-hwmon@vger.kernel.org
> S:	Maintained
> F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
>+F:	Documentation/hwmon/nct7363.rst
>+F:	drivers/hwmon/nct7363.c
>
> NETDEVSIM
> M:	Jakub Kicinski <kuba@kernel.org>
>diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>index a608264da87d..a297b5409b04 100644
>--- a/drivers/hwmon/Kconfig
>+++ b/drivers/hwmon/Kconfig
>@@ -1616,6 +1616,17 @@ config SENSORS_NCT6775_I2C
> 	  This driver can also be built as a module. If so, the module
> 	  will be called nct6775-i2c.
>
>+config SENSORS_NCT7363
>+	tristate "Nuvoton NCT7363Y"
>+	depends on I2C
>+	select REGMAP_I2C
>+	help
>+	  If you say yes here you get support for the Nuvoton NCT7363Y,
>+	  hardware monitoring chip.
>+
>+	  This driver can also be built as a module. If so, the module
>+	  will be called nct7363.
>+
> config SENSORS_NCT7802
> 	tristate "Nuvoton NCT7802Y"
> 	depends on I2C
>diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>index 47be39af5c03..d5e7531204df 100644
>--- a/drivers/hwmon/Makefile
>+++ b/drivers/hwmon/Makefile
>@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o
> nct6775-objs			:= nct6775-platform.o
> obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
> obj-$(CONFIG_SENSORS_NCT6775_I2C) += nct6775-i2c.o
>+obj-$(CONFIG_SENSORS_NCT7363)	+= nct7363.o
> obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
> obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
> obj-$(CONFIG_SENSORS_NPCM7XX)	+= npcm750-pwm-fan.o
>diff --git a/drivers/hwmon/nct7363.c b/drivers/hwmon/nct7363.c
>new file mode 100644
>index 000000000000..c79d3ca4f111
>--- /dev/null
>+++ b/drivers/hwmon/nct7363.c
>@@ -0,0 +1,412 @@
>+// SPDX-License-Identifier: GPL-2.0-or-later
>+/*
>+ * Copyright (c) 2023 Nuvoton Technology corporation.
>+ */
>+
>+#include <linux/bitfield.h>
>+#include <linux/bits.h>
>+#include <linux/err.h>
>+#include <linux/hwmon.h>
>+#include <linux/hwmon-sysfs.h>
>+#include <linux/i2c.h>
>+#include <linux/module.h>
>+#include <linux/mutex.h>
>+#include <linux/regmap.h>
>+#include <linux/slab.h>
>+
>+#define NCT7363_REG_GPIO_0_3		0x20
>+#define NCT7363_REG_GPIO_4_7		0x21
>+#define NCT7363_REG_GPIO_10_13		0x22
>+#define NCT7363_REG_GPIO_14_17		0x23
>+#define NCT7363_REG_PWMEN_0_7		0x38
>+#define NCT7363_REG_PWMEN_8_15		0x39
>+#define NCT7363_REG_FANINEN_0_7		0x41
>+#define NCT7363_REG_FANINEN_8_15	0x42
>+#define NCT7363_REG_FANINX_HVAL(x)	(0x48 + ((x) * 2))
>+#define NCT7363_REG_FANINX_LVAL(x)	(0x49 + ((x) * 2))
>+#define NCT7363_REG_FSCPXDUTY(x)	(0x90 + ((x) * 2))
>+
>+#define PWM_SEL(x)			(BIT(0) << (((x) % 4) * 2))
>+#define FANIN_SEL(x)			(BIT(1) << (((x) % 4) * 2))
>+
>+#define NCT7363_FANINX_LVAL_MASK	GENMASK(4, 0)
>+#define NCT7363_FANIN_MASK		GENMASK(12, 0)
>+
>+#define NCT7363_PWM_COUNT		16
>+
>+static inline unsigned long FAN_FROM_REG(u16 val)
>+{
>+	/* In case fan is stopped or divide by 0 */
>+	if (val == NCT7363_FANIN_MASK || val == 0)
>+		return	0;
>+
>+	return (1350000UL / val);
>+}
>+
>+static const struct of_device_id nct7363_of_match[] = {
>+	{ .compatible = "nuvoton,nct7363" },

As far as I can see from the code in this driver, it looks like this 
driver should also be compatible with the nct7362; shall we add the ID 
table entry for it now?  (Though I only have a datasheet for the 
nct7362, not the nct7363, so I don't know exactly how they differ.)

>+	{ },
>+};
>+MODULE_DEVICE_TABLE(of, nct7363_of_match);
>+
>+struct nct7363_data {
>+	struct regmap		*regmap;
>+	struct mutex		lock; /* protect register access */
>+
>+	u16			fanin_mask;
>+	u16			pwm_mask;
>+};
>+
>+static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
>+			    long *val)
>+{
>+	struct nct7363_data *data = dev_get_drvdata(dev);
>+	unsigned int hi, lo;
>+	u16 cnt, rpm;
>+	int ret = 0;
>+
>+	switch (attr) {
>+	case hwmon_fan_input:
>+		/*
>+		 * High-byte register should be read first to latch
>+		 * synchronous low-byte value
>+		 */
>+		ret = regmap_read(data->regmap,
>+				  NCT7363_REG_FANINX_HVAL(channel), &hi);
>+		if (ret)
>+			return ret;
>+
>+		ret = regmap_read(data->regmap,
>+			NCT7363_REG_FANINX_LVAL(channel), &lo);
>+		if (ret)
>+			return ret;

I think this pair of reads should be done under data->lock to ensure 
that the LVAL read gets the right latched value in a scenario where 
multiple threads executing this function end up with their register 
reads interleaved.

>+
>+		cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
>+		rpm = FAN_FROM_REG(cnt);
>+		*val = (long)rpm;

Given that rpm is assigned from an unsigned long (FAN_FROM_REG()) and 
then to a long (*val), is there any reason we want it as a u16 in 
between the two?  Or for that matter, why not just:

   *val = FAN_FROM_REG(cnt);

?

>+		return 0;
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static umode_t nct7363_fan_is_visible(const void *_data, u32 attr, int channel)
>+{
>+	const struct nct7363_data *data = _data;
>+
>+	switch (attr) {
>+	case hwmon_fan_input:
>+		if (data->fanin_mask & BIT(channel))
>+			return 0444;
>+		break;
>+	default:
>+		break;
>+	}
>+
>+	return 0;
>+}
>+
>+static int nct7363_read_pwm(struct device *dev, u32 attr, int channel,
>+			    long *val)
>+{
>+	struct nct7363_data *data = dev_get_drvdata(dev);
>+	unsigned int regval;
>+	u16 ret;
>+
>+	switch (attr) {
>+	case hwmon_pwm_input:
>+		ret = regmap_read(data->regmap,
>+				  NCT7363_REG_FSCPXDUTY(channel), &regval);
>+		if (ret)
>+			return ret;
>+
>+		*val = (long)regval;
>+		return 0;
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static int nct7363_write_pwm(struct device *dev, u32 attr, int channel,
>+			     long val)
>+{
>+	struct nct7363_data *data = dev_get_drvdata(dev);
>+	int ret;
>+
>+	switch (attr) {
>+	case hwmon_pwm_input:
>+		if (val < 0 || val > 255)
>+			return -EINVAL;
>+
>+		mutex_lock(&data->lock);
>+		ret = regmap_write(data->regmap,
>+				   NCT7363_REG_FSCPXDUTY(channel), val);
>+		mutex_unlock(&data->lock);

..though here, I'm not sure the locking is needed -- is there something 
that necessitates it for a single register write?

>+
>+		return ret;
>+
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static umode_t nct7363_pwm_is_visible(const void *_data, u32 attr, int channel)
>+{
>+	const struct nct7363_data *data = _data;
>+
>+	switch (attr) {
>+	case hwmon_pwm_input:
>+		if (data->pwm_mask & BIT(channel))
>+			return 0644;
>+		break;
>+	default:
>+		break;
>+	}
>+
>+	return 0;
>+}
>+
>+static int nct7363_read(struct device *dev, enum hwmon_sensor_types type,
>+			u32 attr, int channel, long *val)
>+{
>+	switch (type) {
>+	case hwmon_fan:
>+		return nct7363_read_fan(dev, attr, channel, val);
>+	case hwmon_pwm:
>+		return nct7363_read_pwm(dev, attr, channel, val);
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static int nct7363_write(struct device *dev, enum hwmon_sensor_types type,
>+			 u32 attr, int channel, long val)
>+{
>+	switch (type) {
>+	case hwmon_pwm:
>+		return nct7363_write_pwm(dev, attr, channel, val);
>+	default:
>+		return -EOPNOTSUPP;
>+	}
>+}
>+
>+static umode_t nct7363_is_visible(const void *data,
>+				  enum hwmon_sensor_types type,
>+				  u32 attr, int channel)
>+{
>+	switch (type) {
>+	case hwmon_fan:
>+		return nct7363_fan_is_visible(data, attr, channel);
>+	case hwmon_pwm:
>+		return nct7363_pwm_is_visible(data, attr, channel);
>+	default:
>+		return 0;
>+	}
>+}
>+
>+static const struct hwmon_channel_info *nct7363_info[] = {
>+	HWMON_CHANNEL_INFO(fan,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT,
>+			   HWMON_F_INPUT),
>+	HWMON_CHANNEL_INFO(pwm,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT,
>+			   HWMON_PWM_INPUT),
>+	NULL
>+};
>+
>+static const struct hwmon_ops nct7363_hwmon_ops = {
>+	.is_visible = nct7363_is_visible,
>+	.read = nct7363_read,
>+	.write = nct7363_write,
>+};
>+
>+static const struct hwmon_chip_info nct7363_chip_info = {
>+	.ops = &nct7363_hwmon_ops,
>+	.info = nct7363_info,
>+};
>+
>+static int nct7363_init_chip(struct nct7363_data *data)
>+{
>+	u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
>+	int ret;
>+
>+	for (i = 0; i < NCT7363_PWM_COUNT; i++) {
>+		if (i < 4) {
>+			if (data->pwm_mask & BIT(i))
>+				gpio0_3 |= PWM_SEL(i);
>+			if (data->fanin_mask & BIT(i))
>+				gpio10_13 |= FANIN_SEL(i);
>+		} else if (i < 8) {
>+			if (data->pwm_mask & BIT(i))
>+				gpio4_7 |= PWM_SEL(i);
>+			if (data->fanin_mask & BIT(i))
>+				gpio14_17 |= FANIN_SEL(i);
>+		} else if (i < 12) {
>+			if (data->pwm_mask & BIT(i))
>+				gpio10_13 |= PWM_SEL(i);
>+			if (data->fanin_mask & BIT(i))
>+				gpio0_3 |= FANIN_SEL(i);
>+		} else {
>+			if (data->pwm_mask & BIT(i))
>+				gpio14_17 |= PWM_SEL(i);
>+			if (data->fanin_mask & BIT(i))
>+				gpio4_7 |= FANIN_SEL(i);
>+		}
>+	}

With the caveat that I haven't actually sketched it out myself to be 
sure, might it be a bit cleaner to do this with a 4-element array 
indexed by 'i / 4' instead of a big if/else chain and a bunch of 
near-duplicated blocks?

>+
>+	/* Pin Function Configuration */
>+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
>+	if (ret < 0)
>+		return ret;
>+
>+	/* PWM and FANIN Monitoring Enable */
>+	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
>+			   data->pwm_mask & 0xff);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
>+			   (data->pwm_mask >> 8) & 0xff);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
>+			   data->fanin_mask & 0xff);
>+	if (ret < 0)
>+		return ret;
>+	ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
>+			   (data->fanin_mask >> 8) & 0xff);
>+	if (ret < 0)
>+		return ret;
>+
>+	return 0;
>+}
>+
>+static int nct7363_present_pwm_fanin(struct device *dev,
>+				     struct device_node *child,
>+				     struct nct7363_data *data)
>+{
>+	struct of_phandle_args args;
>+	int ret, fanin_cnt;
>+	u8 *fanin_ch;
>+	u8 ch, index;
>+
>+	ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
>+					 0, &args);
>+	if (ret)
>+		return ret;
>+
>+	data->pwm_mask |= BIT(args.args[0]);

Perhaps we should have

   if (args.args[0] >= NCT7363_PWM_COUNT)
     return -EINVAL;

here?

>+
>+	fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
>+	if (fanin_cnt < 1)

fanin_cnt < 1 || fanin_cnt >= NCT7363_PWM_COUNT

>+		return -EINVAL;
>+
>+	fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);

Keeping this allocation around persistently for the whole lifetime of 
the device seems unnecessary -- it's not used beyond this function, 
right?  In fact, dynamically allocating it at all seems like overkill, 
considering that in addition to being temporary, it's also got a pretty 
small upper bound on its size.  I'd think a simple

   u8 fanin_ch[NCT7363_PWM_COUNT];

would suffice?  (Along with the check above to ensure fanin_cnt is 
within range of course.)

>+	if (!fanin_ch)
>+		return -ENOMEM;
>+
>+	ret = of_property_read_u8_array(child, "tach-ch", fanin_ch, fanin_cnt);
>+	if (ret)
>+		return ret;
>+
>+	for (ch = 0; ch < fanin_cnt; ch++) {
>+		index = fanin_ch[ch];

..and likewise a range check here too?

>+		data->fanin_mask |= BIT(index);
>+	}
>+

Should we also grab the pulses-per-revolution property here and factor 
that in in FAN_FROM_REG()?

>+	return 0;
>+}
>+
>+static const struct regmap_config nct7363_regmap_config = {
>+	.reg_bits = 8,
>+	.val_bits = 8,
>+};
>+
>+static int nct7363_probe(struct i2c_client *client)
>+{
>+	struct device *dev = &client->dev;
>+	struct device_node *child;
>+	struct nct7363_data *data;
>+	struct device *hwmon_dev;
>+	int ret;
>+
>+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>+	if (!data)
>+		return -ENOMEM;
>+
>+	data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
>+	if (IS_ERR(data->regmap))
>+		return PTR_ERR(data->regmap);
>+
>+	mutex_init(&data->lock);
>+
>+	for_each_child_of_node(dev->of_node, child) {
>+		ret = nct7363_present_pwm_fanin(dev, child, data);
>+		if (ret) {
>+			of_node_put(child);
>+			return ret;
>+		}
>+	}
>+
>+	/* Initialize the chip */
>+	ret = nct7363_init_chip(data);
>+	if (ret)
>+		return ret;
>+
>+	hwmon_dev =
>+		devm_hwmon_device_register_with_info(dev, client->name, data,
>+						     &nct7363_chip_info, NULL);
>+	return PTR_ERR_OR_ZERO(hwmon_dev);
>+}
>+
>+static struct i2c_driver nct7363_driver = {
>+	.class = I2C_CLASS_HWMON,

Maybe add an i2c_device_id table to accompany the of_match table?

>+	.driver = {
>+		.name = "nct7363",
>+		.of_match_table = nct7363_of_match,
>+	},
>+	.probe = nct7363_probe,
>+};
>+
>+module_i2c_driver(nct7363_driver);
>+
>+MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
>+MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
>+MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
>+MODULE_LICENSE("GPL");
>-- 
>2.34.1
>
>