From: Ban Feng <[email protected]>
NCT736X is an I2C based hardware monitoring chip from Nuvoton.
Ban Feng (2):
dt-bindings: hwmon: Add nct736x bindings
hwmon: Driver for Nuvoton NCT736X
.../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/nct736x.rst | 35 ++
MAINTAINERS | 8 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/nct736x.c | 479 ++++++++++++++++++
7 files changed, 614 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
create mode 100644 Documentation/hwmon/nct736x.rst
create mode 100644 drivers/hwmon/nct736x.c
--
2.34.1
From: Ban Feng <[email protected]>
This change documents the device tree bindings for the Nuvoton
NCT7362Y, NCT7363Y driver.
Signed-off-by: Ban Feng <[email protected]>
---
.../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++
MAINTAINERS | 6 ++
2 files changed, 86 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
new file mode 100644
index 000000000000..f98fd260a20f
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NCT736X Hardware Monitoring IC
+
+maintainers:
+ - Ban Feng <[email protected]>
+
+description: |
+ The NCT736X is a Fan controller which provides up to 16 independent
+ FAN input monitors, and up to 16 independent PWM output with SMBus interface.
+ Besides, NCT7363Y has a built-in watchdog timer which is used for
+ conditionally generating a system reset output (INT#).
+
+additionalProperties: false
+
+properties:
+ compatible:
+ enum:
+ - nuvoton,nct7362
+ - nuvoton,nct7363
+
+ reg:
+ maxItems: 1
+
+ nuvoton,pwm-mask:
+ description: |
+ each bit means PWMx enable/disable setting, where x = 0~15.
+ 0: disabled, 1: enabled
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0x0
+ maximum: 0xFFFF
+ default: 0x0
+
+ nuvoton,fanin-mask:
+ description: |
+ each bit means FANINx monitoring enable/disable setting,
+ where x = 0~15.
+ 0: disabled, 1: enabled
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0x0
+ maximum: 0xFFFF
+ default: 0x0
+
+ nuvoton,wdt-timeout:
+ description: |
+ Watchdog Timer time configuration for NCT7363Y, as below
+ 0: 15 sec (default)
+ 1: 7.5 sec
+ 2: 3.75 sec
+ 3: 30 sec
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2, 3]
+ default: 0
+
+required:
+ - compatible
+ - reg
+ - nuvoton,pwm-mask
+ - nuvoton,fanin-mask
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ nct7363@22 {
+ compatible = "nuvoton,nct7363";
+ reg = <0x22>;
+
+ nuvoton,pwm-mask = <0x003F>;
+ nuvoton,fanin-mask = <0x003F>;
+ nuvoton,wdt-timeout = <0x3>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 012df8ccf34e..eef44c13278c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14900,6 +14900,12 @@ S: Maintained
F: Documentation/devicetree/bindings/hwmon/nuvoton,nct6775.yaml
F: drivers/hwmon/nct6775-i2c.c
+NCT736X HARDWARE MONITOR DRIVER
+M: Ban Feng <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
+
NETDEVSIM
M: Jakub Kicinski <[email protected]>
S: Maintained
--
2.34.1
From: Ban Feng <[email protected]>
NCT736X is an I2C based hardware monitoring chip from Nuvoton.
Signed-off-by: Ban Feng <[email protected]>
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/nct736x.rst | 35 +++
MAINTAINERS | 2 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/nct736x.c | 479 ++++++++++++++++++++++++++++++++
6 files changed, 528 insertions(+)
create mode 100644 Documentation/hwmon/nct736x.rst
create mode 100644 drivers/hwmon/nct736x.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 72f4e6065bae..98eb0efeb121 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -161,6 +161,7 @@ Hardware Monitoring Kernel Drivers
mp5023
nct6683
nct6775
+ nct736x
nct7802
nct7904
npcm750-pwm-fan
diff --git a/Documentation/hwmon/nct736x.rst b/Documentation/hwmon/nct736x.rst
new file mode 100644
index 000000000000..b7d64087263a
--- /dev/null
+++ b/Documentation/hwmon/nct736x.rst
@@ -0,0 +1,35 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver nct736x
+=====================
+
+Supported chip:
+
+ * Nuvoton NCT7362Y NCT7363Y
+
+ Prefix: nct736x
+
+ Addresses: I2C 0x20, 0x21, 0x22, 0x23
+
+Author: Ban Feng <[email protected]>
+
+
+Description
+-----------
+
+The NCT736X is a Fan controller which provides up to 16 independent
+FAN input monitors, and up to 16 independent PWM output with SMBus interface.
+Besides, NCT7363Y has a built-in watchdog timer which is used for
+conditionally generating a system reset output (INT#).
+
+
+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 eef44c13278c..fed10a0bc49c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14905,6 +14905,8 @@ M: Ban Feng <[email protected]>
L: [email protected]
S: Maintained
F: Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
+F: Documentation/hwmon/nct736x.rst
+F: drivers/hwmon/nct736x.c
NETDEVSIM
M: Jakub Kicinski <[email protected]>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index cf27523eed5a..b2ca07e6c03a 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1605,6 +1605,16 @@ config SENSORS_NCT6775_I2C
This driver can also be built as a module. If so, the module
will be called nct6775-i2c.
+config SENSORS_NCT736X
+ tristate "Nuvoton NCT736X"
+ depends on I2C
+ help
+ If you say yes here you get support for the Nuvoton NCT7362Y,
+ NCT7363Y hardware monitoring chip.
+
+ This driver can also be built as a module. If so, the module
+ will be called nct736x.
+
config SENSORS_NCT7802
tristate "Nuvoton NCT7802Y"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e84bd9685b5c..13f378452a4b 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -166,6 +166,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_NCT736X) += nct736x.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/nct736x.c b/drivers/hwmon/nct736x.c
new file mode 100644
index 000000000000..4c59e823062d
--- /dev/null
+++ b/drivers/hwmon/nct736x.c
@@ -0,0 +1,479 @@
+// 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/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+
+#define NCT736X_REG_GPIO_0_3 0x20
+#define NCT736X_REG_GPIO_4_7 0x21
+#define NCT736X_REG_GPIO_10_13 0x22
+#define NCT736X_REG_GPIO_14_17 0x23
+#define NCT7363_REG_WDT 0x2A
+#define WDT_CFG_MASK GENMASK(3, 2)
+#define WDT_CFG(x) FIELD_PREP(WDT_CFG_MASK, (x))
+#define EN_WDT BIT(7)
+#define NCT736X_REG_PWMEN_0_7 0x38
+#define NCT736X_REG_PWMEN_8_15 0x39
+#define NCT736X_REG_FANINEN_0_7 0x41
+#define NCT736X_REG_FANINEN_8_15 0x42
+#define NCT736X_REG_FANINx_HVAL(x) (0x48 + ((x) * 2))
+#define NCT736X_REG_FANINx_LVAL(x) (0x49 + ((x) * 2))
+#define NCT736X_REG_FSCPxDUTY(x) (0x90 + ((x) * 2))
+#define NCT736X_REG_VENDOR_ID 0xFD
+#define NCT736X_REG_CHIP_ID 0xFE
+#define NCT736X_REG_DEVICE_ID 0xFF
+
+#define NUVOTON_ID 0x49
+#define CHIP_ID 0x19
+#define DEVICE_ID 0x88
+
+#define PWM_SEL(x) (BIT(0) << ((x % 4) * 2))
+#define FANIN_SEL(x) (BIT(1) << ((x % 4) * 2))
+#define BIT_CHECK(x) (BIT(0) << x)
+
+#define NCT736X_FANINx_LVAL_MASK GENMASK(4, 0)
+#define NCT736X_FANIN_MASK GENMASK(12, 0)
+
+#define NCT736X_PWM_COUNT 16
+#define NCT736X_FANIN_COUNT 16
+
+#define REFRESH_INTERVAL (2 * HZ)
+
+static inline unsigned long FAN_FROM_REG(u16 val)
+{
+ if ((val >= NCT736X_FANIN_MASK) || (val == 0))
+ return 0;
+
+ return (1350000UL / val);
+}
+
+static const unsigned short normal_i2c[] = {
+ 0x20, 0x21, 0x22, 0x23, I2C_CLIENT_END
+};
+
+enum chips { nct7362, nct7363 };
+
+struct nct736x_data {
+ struct i2c_client *client;
+ const struct attribute_group *groups[3];
+ struct mutex update_lock;
+ bool valid;
+ unsigned long last_updated; /* In jiffies */
+
+ u16 fan_mask;
+ u16 fan[NCT736X_FANIN_COUNT];
+ u16 pwm_mask;
+ u8 pwm[NCT736X_PWM_COUNT];
+};
+
+/* Read 1-byte register. Returns unsigned reg or -ERRNO on error. */
+static int nct736x_read_reg(struct i2c_client *client, unsigned int reg)
+{
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(client, reg);
+ return ret;
+}
+
+/* Write 1-byte register. Returns 0 or -ERRNO on error. */
+static int nct736x_write_reg(struct i2c_client *client,
+ unsigned int reg, u8 value)
+{
+ int ret;
+
+ ret = i2c_smbus_write_byte_data(client, reg, value);
+ return ret;
+}
+
+static struct nct736x_data *nct736x_update_device(struct device *dev)
+{
+ struct nct736x_data *data = dev_get_drvdata(dev);
+ struct i2c_client *client = data->client;
+ int i;
+
+ mutex_lock(&data->update_lock);
+
+ if (!(time_after(jiffies, data->last_updated + REFRESH_INTERVAL)
+ || !data->valid))
+ goto no_sensor_update;
+
+ for (i = 0; i < ARRAY_SIZE(data->fan); i++) {
+ if (!(data->fan_mask & BIT_CHECK(i)))
+ continue;
+
+ data->fan[i] = ((u16)nct736x_read_reg(client,
+ NCT736X_REG_FANINx_HVAL(i))) << 5;
+ data->fan[i] |= nct736x_read_reg(client,
+ NCT736X_REG_FANINx_LVAL(i)) & NCT736X_FANINx_LVAL_MASK;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(data->pwm); i++) {
+ if (!(data->pwm_mask & BIT_CHECK(i)))
+ continue;
+
+ data->pwm[i] = nct736x_read_reg(client,
+ NCT736X_REG_FSCPxDUTY(i));
+ }
+
+ data->last_updated = jiffies;
+ data->valid = true;
+
+no_sensor_update:
+ mutex_unlock(&data->update_lock);
+ return data;
+}
+
+static ssize_t
+fan_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+ struct nct736x_data *data = nct736x_update_device(dev);
+ u16 val;
+
+ val = data->fan[sattr->index] & NCT736X_FANIN_MASK;
+
+ return sprintf(buf, "%lu\n", FAN_FROM_REG(val));
+}
+
+static ssize_t
+pwm_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+ struct nct736x_data *data = nct736x_update_device(dev);
+ u16 val;
+
+ val = data->pwm[sattr->index];
+
+ return sprintf(buf, "%u\n", (val));
+}
+
+static ssize_t
+pwm_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+ struct nct736x_data *data = nct736x_update_device(dev);
+ struct i2c_client *client = data->client;
+ unsigned long tmpVal;
+ int err;
+
+ err = kstrtoul(buf, 10, &tmpVal);
+ if (err)
+ return err;
+
+ tmpVal = clamp_val(tmpVal, 0, 255);
+
+ mutex_lock(&data->update_lock);
+ err = nct736x_write_reg(client, NCT736X_REG_FSCPxDUTY(sattr->index),
+ tmpVal);
+ if (err)
+ goto abort;
+
+ data->pwm[sattr->index] = tmpVal;
+
+abort:
+ mutex_unlock(&data->update_lock);
+ return count;
+}
+
+static umode_t nct736x_fan_is_visible(struct kobject *kobj,
+ struct attribute *attr, int index)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct nct736x_data *data = dev_get_drvdata(dev);
+
+ if (!(data->fan_mask & BIT_CHECK(index)))
+ return 0;
+
+ return attr->mode;
+}
+
+static umode_t nct736x_pwm_is_visible(struct kobject *kobj,
+ struct attribute *attr, int index)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct nct736x_data *data = dev_get_drvdata(dev);
+
+ if (!(data->pwm_mask & BIT_CHECK(index)))
+ return 0;
+
+ return attr->mode;
+}
+
+#define FAN_INPUT 0
+
+static SENSOR_DEVICE_ATTR_2_RO(fan1_input, fan, FAN_INPUT, 0);
+static SENSOR_DEVICE_ATTR_2_RO(fan2_input, fan, FAN_INPUT, 1);
+static SENSOR_DEVICE_ATTR_2_RO(fan3_input, fan, FAN_INPUT, 2);
+static SENSOR_DEVICE_ATTR_2_RO(fan4_input, fan, FAN_INPUT, 3);
+static SENSOR_DEVICE_ATTR_2_RO(fan5_input, fan, FAN_INPUT, 4);
+static SENSOR_DEVICE_ATTR_2_RO(fan6_input, fan, FAN_INPUT, 5);
+static SENSOR_DEVICE_ATTR_2_RO(fan7_input, fan, FAN_INPUT, 6);
+static SENSOR_DEVICE_ATTR_2_RO(fan8_input, fan, FAN_INPUT, 7);
+static SENSOR_DEVICE_ATTR_2_RO(fan9_input, fan, FAN_INPUT, 8);
+static SENSOR_DEVICE_ATTR_2_RO(fan10_input, fan, FAN_INPUT, 9);
+static SENSOR_DEVICE_ATTR_2_RO(fan11_input, fan, FAN_INPUT, 10);
+static SENSOR_DEVICE_ATTR_2_RO(fan12_input, fan, FAN_INPUT, 11);
+static SENSOR_DEVICE_ATTR_2_RO(fan13_input, fan, FAN_INPUT, 12);
+static SENSOR_DEVICE_ATTR_2_RO(fan14_input, fan, FAN_INPUT, 13);
+static SENSOR_DEVICE_ATTR_2_RO(fan15_input, fan, FAN_INPUT, 14);
+static SENSOR_DEVICE_ATTR_2_RO(fan16_input, fan, FAN_INPUT, 15);
+
+static struct attribute *nct736x_attributes_fan[] = {
+ &sensor_dev_attr_fan1_input.dev_attr.attr,
+ &sensor_dev_attr_fan2_input.dev_attr.attr,
+ &sensor_dev_attr_fan3_input.dev_attr.attr,
+ &sensor_dev_attr_fan4_input.dev_attr.attr,
+ &sensor_dev_attr_fan5_input.dev_attr.attr,
+ &sensor_dev_attr_fan6_input.dev_attr.attr,
+ &sensor_dev_attr_fan7_input.dev_attr.attr,
+ &sensor_dev_attr_fan8_input.dev_attr.attr,
+ &sensor_dev_attr_fan9_input.dev_attr.attr,
+ &sensor_dev_attr_fan10_input.dev_attr.attr,
+ &sensor_dev_attr_fan11_input.dev_attr.attr,
+ &sensor_dev_attr_fan12_input.dev_attr.attr,
+ &sensor_dev_attr_fan13_input.dev_attr.attr,
+ &sensor_dev_attr_fan14_input.dev_attr.attr,
+ &sensor_dev_attr_fan15_input.dev_attr.attr,
+ &sensor_dev_attr_fan16_input.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group nct736x_group_fan = {
+ .attrs = nct736x_attributes_fan,
+ .is_visible = nct736x_fan_is_visible,
+};
+
+#define PWM_OUTPUT 0
+
+static SENSOR_DEVICE_ATTR_2_RW(pwm1, pwm, PWM_OUTPUT, 0);
+static SENSOR_DEVICE_ATTR_2_RW(pwm2, pwm, PWM_OUTPUT, 1);
+static SENSOR_DEVICE_ATTR_2_RW(pwm3, pwm, PWM_OUTPUT, 2);
+static SENSOR_DEVICE_ATTR_2_RW(pwm4, pwm, PWM_OUTPUT, 3);
+static SENSOR_DEVICE_ATTR_2_RW(pwm5, pwm, PWM_OUTPUT, 4);
+static SENSOR_DEVICE_ATTR_2_RW(pwm6, pwm, PWM_OUTPUT, 5);
+static SENSOR_DEVICE_ATTR_2_RW(pwm7, pwm, PWM_OUTPUT, 6);
+static SENSOR_DEVICE_ATTR_2_RW(pwm8, pwm, PWM_OUTPUT, 7);
+static SENSOR_DEVICE_ATTR_2_RW(pwm9, pwm, PWM_OUTPUT, 8);
+static SENSOR_DEVICE_ATTR_2_RW(pwm10, pwm, PWM_OUTPUT, 9);
+static SENSOR_DEVICE_ATTR_2_RW(pwm11, pwm, PWM_OUTPUT, 10);
+static SENSOR_DEVICE_ATTR_2_RW(pwm12, pwm, PWM_OUTPUT, 11);
+static SENSOR_DEVICE_ATTR_2_RW(pwm13, pwm, PWM_OUTPUT, 12);
+static SENSOR_DEVICE_ATTR_2_RW(pwm14, pwm, PWM_OUTPUT, 13);
+static SENSOR_DEVICE_ATTR_2_RW(pwm15, pwm, PWM_OUTPUT, 14);
+static SENSOR_DEVICE_ATTR_2_RW(pwm16, pwm, PWM_OUTPUT, 15);
+
+static struct attribute *nct736x_attributes_pwm[] = {
+ &sensor_dev_attr_pwm1.dev_attr.attr,
+ &sensor_dev_attr_pwm2.dev_attr.attr,
+ &sensor_dev_attr_pwm3.dev_attr.attr,
+ &sensor_dev_attr_pwm4.dev_attr.attr,
+ &sensor_dev_attr_pwm5.dev_attr.attr,
+ &sensor_dev_attr_pwm6.dev_attr.attr,
+ &sensor_dev_attr_pwm7.dev_attr.attr,
+ &sensor_dev_attr_pwm8.dev_attr.attr,
+ &sensor_dev_attr_pwm9.dev_attr.attr,
+ &sensor_dev_attr_pwm10.dev_attr.attr,
+ &sensor_dev_attr_pwm11.dev_attr.attr,
+ &sensor_dev_attr_pwm12.dev_attr.attr,
+ &sensor_dev_attr_pwm13.dev_attr.attr,
+ &sensor_dev_attr_pwm14.dev_attr.attr,
+ &sensor_dev_attr_pwm15.dev_attr.attr,
+ &sensor_dev_attr_pwm16.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group nct736x_group_pwm = {
+ .attrs = nct736x_attributes_pwm,
+ .is_visible = nct736x_pwm_is_visible,
+};
+
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static int nct736x_detect(struct i2c_client *client,
+ struct i2c_board_info *info)
+{
+ struct i2c_adapter *adapter = client->adapter;
+ int vendor, chip, device;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+ return -ENODEV;
+
+ vendor = i2c_smbus_read_byte_data(client, NCT736X_REG_VENDOR_ID);
+ if (vendor != NUVOTON_ID)
+ return -ENODEV;
+
+ chip = i2c_smbus_read_byte_data(client, NCT736X_REG_CHIP_ID);
+ if (chip != CHIP_ID)
+ return -ENODEV;
+
+ device = i2c_smbus_read_byte_data(client, NCT736X_REG_DEVICE_ID);
+ if (device != DEVICE_ID)
+ return -ENODEV;
+
+ strscpy(info->type, "nct736x", I2C_NAME_SIZE);
+
+ return 0;
+}
+
+static const struct i2c_device_id nct736x_id[] = {
+ {"nct7362", nct7362},
+ {"nct7363", nct7363},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, nct736x_id);
+
+static int nct736x_init_chip(struct i2c_client *client,
+ u32 pwm_mask, u32 fanin_mask, u32 wdt_cfg)
+{
+ const struct i2c_device_id *id = i2c_match_id(nct736x_id, client);
+ u8 i, gpio0_3, gpio4_7, gpio10_13, gpio14_17;
+ int ret;
+
+ for (i = 0; i < NCT736X_PWM_COUNT; i++) {
+ if (i < 4) {
+ if (pwm_mask & BIT_CHECK(i))
+ gpio0_3 |= PWM_SEL(i);
+ if (fanin_mask & BIT_CHECK(i))
+ gpio10_13 |= FANIN_SEL(i);
+ } else if (i < 8) {
+ if (pwm_mask & BIT_CHECK(i))
+ gpio4_7 |= PWM_SEL(i);
+ if (fanin_mask & BIT_CHECK(i))
+ gpio14_17 |= FANIN_SEL(i);
+ } else if (i < 12) {
+ if (pwm_mask & BIT_CHECK(i))
+ gpio10_13 |= PWM_SEL(i);
+ if (fanin_mask & BIT_CHECK(i))
+ gpio0_3 |= FANIN_SEL(i);
+ } else {
+ if (pwm_mask & BIT_CHECK(i))
+ gpio14_17 |= PWM_SEL(i);
+ if (fanin_mask & BIT_CHECK(i))
+ gpio4_7 |= FANIN_SEL(i);
+ }
+ }
+
+ /* Pin Function Configuration */
+ ret = nct736x_write_reg(client, NCT736X_REG_GPIO_0_3, gpio0_3);
+ if (ret < 0)
+ return ret;
+ ret = nct736x_write_reg(client, NCT736X_REG_GPIO_4_7, gpio4_7);
+ if (ret < 0)
+ return ret;
+ ret = nct736x_write_reg(client, NCT736X_REG_GPIO_10_13, gpio10_13);
+ if (ret < 0)
+ return ret;
+ ret = nct736x_write_reg(client, NCT736X_REG_GPIO_14_17, gpio14_17);
+ if (ret < 0)
+ return ret;
+
+ /* PWM and FANIN Monitoring Enable */
+ ret = nct736x_write_reg(client, NCT736X_REG_PWMEN_0_7,
+ pwm_mask & 0xff);
+ if (ret < 0)
+ return ret;
+ ret = nct736x_write_reg(client,
+ NCT736X_REG_PWMEN_8_15, (pwm_mask >> 8) & 0xff);
+ if (ret < 0)
+ return ret;
+ ret = nct736x_write_reg(client, NCT736X_REG_FANINEN_0_7,
+ fanin_mask & 0xff);
+ if (ret < 0)
+ return ret;
+ ret = nct736x_write_reg(client, NCT736X_REG_FANINEN_8_15,
+ (fanin_mask >> 8) & 0xff);
+ if (ret < 0)
+ return ret;
+
+ /* Watchdog Timer Configuration */
+ if (wdt_cfg != 0xff && id->driver_data == nct7363) {
+ ret = nct736x_write_reg(client, NCT7363_REG_WDT, wdt_cfg);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int nct736x_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct nct736x_data *data;
+ struct device *hwmon_dev;
+ u32 pwm_mask, fanin_mask, val, wdt_cfg;
+ int ret;
+
+ data = devm_kzalloc(dev, sizeof(struct nct736x_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, data);
+ mutex_init(&data->update_lock);
+
+ data->client = client;
+
+ if (of_property_read_u32(dev->of_node, "nuvoton,pwm-mask", &pwm_mask))
+ pwm_mask = 0;
+ if (of_property_read_u32(dev->of_node,
+ "nuvoton,fanin-mask", &fanin_mask))
+ fanin_mask = 0;
+ if (of_property_read_u32(dev->of_node, "nuvoton,wdt-timeout", &val))
+ wdt_cfg = 0xff;
+ else
+ wdt_cfg = WDT_CFG(val) | EN_WDT;
+
+ /* Initialize the chip */
+ ret = nct736x_init_chip(client, pwm_mask, fanin_mask, wdt_cfg);
+ if (ret)
+ return ret;
+
+ data->fan_mask = (u16)fanin_mask;
+ data->pwm_mask = (u16)pwm_mask;
+
+ data = nct736x_update_device(dev);
+
+ data->groups[0] = &nct736x_group_fan;
+ data->groups[1] = &nct736x_group_pwm;
+ data->groups[2] = NULL;
+
+ hwmon_dev = devm_hwmon_device_register_with_groups(dev,
+ client->name,
+ data, data->groups);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct of_device_id nct736x_of_match[] = {
+ { .compatible = "nuvoton,nct7362" },
+ { .compatible = "nuvoton,nct7363" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, nct736x_of_match);
+
+static struct i2c_driver nct736x_driver = {
+ .class = I2C_CLASS_HWMON,
+ .driver = {
+ .name = "nct736x",
+ .of_match_table = nct736x_of_match,
+ },
+ .probe = nct736x_probe,
+ .id_table = nct736x_id,
+ .detect = nct736x_detect,
+ .address_list = normal_i2c,
+};
+
+module_i2c_driver(nct736x_driver);
+
+MODULE_AUTHOR("CWHo <[email protected]>");
+MODULE_AUTHOR("Ban Feng <[email protected]>");
+MODULE_DESCRIPTION("NCT736X Hardware Monitoring Driver");
+MODULE_LICENSE("GPL");
--
2.34.1
On 12/3/23 21:56, [email protected] wrote:
> From: Ban Feng <[email protected]>
>
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
>
No, it isn't. Such a chip does not exist. The chips are apparently
NCT7362Y and NCT7363Y. No wildcards in filenames, variables, etc.,
please. Pick one name (nct7362y) instead and reference both chips
where appropriate.
Guenter
On 12/3/23 21:56, [email protected] wrote:
> From: Ban Feng <[email protected]>
>
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
>
> Signed-off-by: Ban Feng <[email protected]>
> ---
[ ... ]
> + hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> + client->name,
> + data, data->groups);
Please resubmit using devm_hwmon_device_register_with_info().
Drivers using deprecated APIs will not be accepted.
Guenter
On 04/12/2023 06:56, [email protected] wrote:
> From: Ban Feng <[email protected]>
>
> This change documents the device tree bindings for the Nuvoton
> NCT7362Y, NCT7363Y driver.
>
> Signed-off-by: Ban Feng <[email protected]>
> ---
> .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++
> MAINTAINERS | 6 ++
> 2 files changed, 86 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> new file mode 100644
> index 000000000000..f98fd260a20f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton NCT736X Hardware Monitoring IC
> +
> +maintainers:
> + - Ban Feng <[email protected]>
> +
> +description: |
> + The NCT736X is a Fan controller which provides up to 16 independent
> + FAN input monitors, and up to 16 independent PWM output with SMBus interface.
> + Besides, NCT7363Y has a built-in watchdog timer which is used for
> + conditionally generating a system reset output (INT#).
> +
> +additionalProperties: false
Please place it just like other bindings are placing it. Not in some
random order. See example-schema.
You should use common fan properties. If it was not merged yet, you must
rebase on patchset on LKML and mention the dependency in the change log
(---).
> +
> +properties:
> + compatible:
> + enum:
> + - nuvoton,nct7362
> + - nuvoton,nct7363
> +
> + reg:
> + maxItems: 1
> +
> + nuvoton,pwm-mask:
> + description: |
> + each bit means PWMx enable/disable setting, where x = 0~15.
> + 0: disabled, 1: enabled
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0x0
> + maximum: 0xFFFF
> + default: 0x0
Use pwms, not own property for this.
> +
> + nuvoton,fanin-mask:
> + description: |
> + each bit means FANINx monitoring enable/disable setting,
> + where x = 0~15.
> + 0: disabled, 1: enabled
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0x0
> + maximum: 0xFFFF
> + default: 0x0
Use properties from common fan bindings.
> +
> + nuvoton,wdt-timeout:
> + description: |
> + Watchdog Timer time configuration for NCT7363Y, as below
> + 0: 15 sec (default)
> + 1: 7.5 sec
> + 2: 3.75 sec
> + 3: 30 sec
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2, 3]
> + default: 0
Nope, reference watchdog.yaml and use its properties. See other watchdog
bindings for examples.
> +
> +required:
> + - compatible
> + - reg
> + - nuvoton,pwm-mask
> + - nuvoton,fanin-mask
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + nct7363@22 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Best regards,
Krzysztof
On 04/12/2023 06:56, [email protected] wrote:
> From: Ban Feng <[email protected]>
>
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
>
> Signed-off-by: Ban Feng <[email protected]>
> ---
> +
> +static const struct i2c_device_id nct736x_id[] = {
> + {"nct7362", nct7362},
> + {"nct7363", nct7363},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, nct736x_id);
> +
All ID tables are next to each other. Move it down. Why does it not
match of_device_id?
...
> +
> +static int nct736x_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct nct736x_data *data;
> + struct device *hwmon_dev;
> + u32 pwm_mask, fanin_mask, val, wdt_cfg;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(struct nct736x_data), GFP_KERNEL);
sizeof(*)
> + if (!data)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, data);
> + mutex_init(&data->update_lock);
> +
> + data->client = client;
> +
> + if (of_property_read_u32(dev->of_node, "nuvoton,pwm-mask", &pwm_mask))
> + pwm_mask = 0;
> + if (of_property_read_u32(dev->of_node,
> + "nuvoton,fanin-mask", &fanin_mask))
> + fanin_mask = 0;
> + if (of_property_read_u32(dev->of_node, "nuvoton,wdt-timeout", &val))
> + wdt_cfg = 0xff;
> + else
> + wdt_cfg = WDT_CFG(val) | EN_WDT;
> +
> + /* Initialize the chip */
> + ret = nct736x_init_chip(client, pwm_mask, fanin_mask, wdt_cfg);
> + if (ret)
> + return ret;
> +
> + data->fan_mask = (u16)fanin_mask;
> + data->pwm_mask = (u16)pwm_mask;
> +
> + data = nct736x_update_device(dev);
> +
> + data->groups[0] = &nct736x_group_fan;
> + data->groups[1] = &nct736x_group_pwm;
> + data->groups[2] = NULL;
> +
> + hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> + client->name,
> + data, data->groups);
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id nct736x_of_match[] = {
> + { .compatible = "nuvoton,nct7362" },
> + { .compatible = "nuvoton,nct7363" },
This means your devices are compatible. Express compatibility in your
bindings (specific compatible followed by fallback). But then your
i2c_device_id is not matching this one here... confusing and clearly wrong.
Best regards,
Krzysztof
Hi Guenter
-----Original Message-----
From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
Sent: Monday, December 4, 2023 3:07 PM
To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; CS20 KWLiu <[email protected]>; CS20 KCFeng0 <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
CAUTION - External Email: Do not click links or open attachments unless you acknowledge the sender and content.
On 12/3/23 21:56, [email protected] wrote:
> From: Ban Feng <[email protected]>
>
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
>
> Signed-off-by: Ban Feng <[email protected]>
> ---
[ ... ]
> + hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> + client->name,
> + data, data->groups);
Please resubmit using devm_hwmon_device_register_with_info().
Drivers using deprecated APIs will not be accepted.
I'll convert to devm_hwmon_device_register_with_info in PATCH v2.
Guenter
Thanks,
Ban
________________________________
________________________________
The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.
On Mon, Dec 4, 2023 at 3:04 PM Guenter Roeck <[email protected]> wrote:
>
> On 12/3/23 21:56, [email protected] wrote:
> > From: Ban Feng <[email protected]>
> >
> > NCT736X is an I2C based hardware monitoring chip from Nuvoton.
> >
>
> No, it isn't. Such a chip does not exist. The chips are apparently
> NCT7362Y and NCT7363Y. No wildcards in filenames, variables, etc.,
> please. Pick one name (nct7362y) instead and reference both chips
> where appropriate.
>
This driver is based on nct7363y, so I'll rename all to NCT7363Y in v2.
Thanks,
Ban
On 05/12/2023 06:02, [email protected] wrote:
> Hi Guenter
...
> The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately.
As requested: I am going to delete all the copies of your emails.
Best regards,
Krzysztof
Hi Krzysztof,
On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 04/12/2023 06:56, [email protected] wrote:
> > From: Ban Feng <[email protected]>
> >
> > This change documents the device tree bindings for the Nuvoton
> > NCT7362Y, NCT7363Y driver.
> >
> > Signed-off-by: Ban Feng <[email protected]>
> > ---
> > .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++
> > MAINTAINERS | 6 ++
> > 2 files changed, 86 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> > new file mode 100644
> > index 000000000000..f98fd260a20f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Nuvoton NCT736X Hardware Monitoring IC
> > +
> > +maintainers:
> > + - Ban Feng <[email protected]>
> > +
> > +description: |
> > + The NCT736X is a Fan controller which provides up to 16 independent
> > + FAN input monitors, and up to 16 independent PWM output with SMBus interface.
> > + Besides, NCT7363Y has a built-in watchdog timer which is used for
> > + conditionally generating a system reset output (INT#).
> > +
> > +additionalProperties: false
>
> Please place it just like other bindings are placing it. Not in some
> random order. See example-schema.
ok, I'll move additionalProperties to the correct place.
>
> You should use common fan properties. If it was not merged yet, you must
> rebase on patchset on LKML and mention the dependency in the change log
> (---).
please kindly see below
>
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - nuvoton,nct7362
> > + - nuvoton,nct7363
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + nuvoton,pwm-mask:
> > + description: |
> > + each bit means PWMx enable/disable setting, where x = 0~15.
> > + 0: disabled, 1: enabled
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x0
> > + maximum: 0xFFFF
> > + default: 0x0
>
> Use pwms, not own property for this.
NCT736X has 16 funtion pins, they can be
GPIO/PWM/FANIN/Reserved_or_ALERT#, and default is GPIO.
We would like to add such a property that can configure the function
pin for pin0~7 and pin10~17.
My original design is only for PWM/FANIN, however,
I noticed that we can change the design to "nuvoton,pin[0-7]$" and
"nuvoton,pin[10-17]$", like example in adt7475.yaml.
I'm not sure if this can be accepted or not, please kindly review this.
>
> > +
> > + nuvoton,fanin-mask:
> > + description: |
> > + each bit means FANINx monitoring enable/disable setting,
> > + where x = 0~15.
> > + 0: disabled, 1: enabled
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 0x0
> > + maximum: 0xFFFF
> > + default: 0x0
>
> Use properties from common fan bindings.
Ditto
>
> > +
> > + nuvoton,wdt-timeout:
> > + description: |
> > + Watchdog Timer time configuration for NCT7363Y, as below
> > + 0: 15 sec (default)
> > + 1: 7.5 sec
> > + 2: 3.75 sec
> > + 3: 30 sec
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 1, 2, 3]
> > + default: 0
>
> Nope, reference watchdog.yaml and use its properties. See other watchdog
> bindings for examples.
NCT7363 has a built-in watchdog timer which is used for conditionally
generating a system reset
output (INT#) if the microcontroller or microprocessor stops to
periodically send a pulse signal or
interface communication access signal like SCL signal from SMBus interface.
We only consider "Watchdog Timer Configuration Register" enable bit
and timeout setting.
Should we still need to add struct watchdog_device to struct nct736x_data?
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - nuvoton,pwm-mask
> > + - nuvoton,fanin-mask
> > +
> > +examples:
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + nct7363@22 {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
ok, I'll change nct7363@22 to hwmon@22.
Thanks,
Ban
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.7-rc4 next-20231205]
[cannot apply to groeck-staging/hwmon-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/baneric926-gmail-com/dt-bindings-hwmon-Add-nct736x-bindings/20231204-135942
base: linus/master
patch link: https://lore.kernel.org/r/20231204055650.788388-3-kcfeng0%40nuvoton.com
patch subject: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20231205/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/hwmon/nct736x.c:352:5: warning: variable 'gpio14_17' is uninitialized when used here [-Wuninitialized]
gpio14_17 |= FANIN_SEL(i);
^~~~~~~~~
drivers/hwmon/nct736x.c:339:46: note: initialize the variable 'gpio14_17' to silence this warning
u8 i, gpio0_3, gpio4_7, gpio10_13, gpio14_17;
^
= '\0'
>> drivers/hwmon/nct736x.c:347:5: warning: variable 'gpio10_13' is uninitialized when used here [-Wuninitialized]
gpio10_13 |= FANIN_SEL(i);
^~~~~~~~~
drivers/hwmon/nct736x.c:339:35: note: initialize the variable 'gpio10_13' to silence this warning
u8 i, gpio0_3, gpio4_7, gpio10_13, gpio14_17;
^
= '\0'
>> drivers/hwmon/nct736x.c:350:5: warning: variable 'gpio4_7' is uninitialized when used here [-Wuninitialized]
gpio4_7 |= PWM_SEL(i);
^~~~~~~
drivers/hwmon/nct736x.c:339:24: note: initialize the variable 'gpio4_7' to silence this warning
u8 i, gpio0_3, gpio4_7, gpio10_13, gpio14_17;
^
= '\0'
>> drivers/hwmon/nct736x.c:345:5: warning: variable 'gpio0_3' is uninitialized when used here [-Wuninitialized]
gpio0_3 |= PWM_SEL(i);
^~~~~~~
drivers/hwmon/nct736x.c:339:15: note: initialize the variable 'gpio0_3' to silence this warning
u8 i, gpio0_3, gpio4_7, gpio10_13, gpio14_17;
^
= '\0'
4 warnings generated.
vim +/gpio14_17 +352 drivers/hwmon/nct736x.c
334
335 static int nct736x_init_chip(struct i2c_client *client,
336 u32 pwm_mask, u32 fanin_mask, u32 wdt_cfg)
337 {
338 const struct i2c_device_id *id = i2c_match_id(nct736x_id, client);
339 u8 i, gpio0_3, gpio4_7, gpio10_13, gpio14_17;
340 int ret;
341
342 for (i = 0; i < NCT736X_PWM_COUNT; i++) {
343 if (i < 4) {
344 if (pwm_mask & BIT_CHECK(i))
> 345 gpio0_3 |= PWM_SEL(i);
346 if (fanin_mask & BIT_CHECK(i))
> 347 gpio10_13 |= FANIN_SEL(i);
348 } else if (i < 8) {
349 if (pwm_mask & BIT_CHECK(i))
> 350 gpio4_7 |= PWM_SEL(i);
351 if (fanin_mask & BIT_CHECK(i))
> 352 gpio14_17 |= FANIN_SEL(i);
353 } else if (i < 12) {
354 if (pwm_mask & BIT_CHECK(i))
355 gpio10_13 |= PWM_SEL(i);
356 if (fanin_mask & BIT_CHECK(i))
357 gpio0_3 |= FANIN_SEL(i);
358 } else {
359 if (pwm_mask & BIT_CHECK(i))
360 gpio14_17 |= PWM_SEL(i);
361 if (fanin_mask & BIT_CHECK(i))
362 gpio4_7 |= FANIN_SEL(i);
363 }
364 }
365
366 /* Pin Function Configuration */
367 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_0_3, gpio0_3);
368 if (ret < 0)
369 return ret;
370 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_4_7, gpio4_7);
371 if (ret < 0)
372 return ret;
373 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_10_13, gpio10_13);
374 if (ret < 0)
375 return ret;
376 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_14_17, gpio14_17);
377 if (ret < 0)
378 return ret;
379
380 /* PWM and FANIN Monitoring Enable */
381 ret = nct736x_write_reg(client, NCT736X_REG_PWMEN_0_7,
382 pwm_mask & 0xff);
383 if (ret < 0)
384 return ret;
385 ret = nct736x_write_reg(client,
386 NCT736X_REG_PWMEN_8_15, (pwm_mask >> 8) & 0xff);
387 if (ret < 0)
388 return ret;
389 ret = nct736x_write_reg(client, NCT736X_REG_FANINEN_0_7,
390 fanin_mask & 0xff);
391 if (ret < 0)
392 return ret;
393 ret = nct736x_write_reg(client, NCT736X_REG_FANINEN_8_15,
394 (fanin_mask >> 8) & 0xff);
395 if (ret < 0)
396 return ret;
397
398 /* Watchdog Timer Configuration */
399 if (wdt_cfg != 0xff && id->driver_data == nct7363) {
400 ret = nct736x_write_reg(client, NCT7363_REG_WDT, wdt_cfg);
401 if (ret < 0)
402 return ret;
403 }
404
405 return 0;
406 }
407
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.7-rc4 next-20231205]
[cannot apply to groeck-staging/hwmon-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/baneric926-gmail-com/dt-bindings-hwmon-Add-nct736x-bindings/20231204-135942
base: linus/master
patch link: https://lore.kernel.org/r/20231204055650.788388-3-kcfeng0%40nuvoton.com
patch subject: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
config: um-randconfig-001-20231205 (https://download.01.org/0day-ci/archive/20231205/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/hwmon/nct736x.c: In function 'nct736x_init_chip.constprop':
>> drivers/hwmon/nct736x.c:367:15: warning: 'gpio0_3' is used uninitialized [-Wuninitialized]
367 | ret = nct736x_write_reg(client, NCT736X_REG_GPIO_0_3, gpio0_3);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/gpio0_3 +367 drivers/hwmon/nct736x.c
334
335 static int nct736x_init_chip(struct i2c_client *client,
336 u32 pwm_mask, u32 fanin_mask, u32 wdt_cfg)
337 {
338 const struct i2c_device_id *id = i2c_match_id(nct736x_id, client);
339 u8 i, gpio0_3, gpio4_7, gpio10_13, gpio14_17;
340 int ret;
341
342 for (i = 0; i < NCT736X_PWM_COUNT; i++) {
343 if (i < 4) {
344 if (pwm_mask & BIT_CHECK(i))
345 gpio0_3 |= PWM_SEL(i);
346 if (fanin_mask & BIT_CHECK(i))
347 gpio10_13 |= FANIN_SEL(i);
348 } else if (i < 8) {
349 if (pwm_mask & BIT_CHECK(i))
350 gpio4_7 |= PWM_SEL(i);
351 if (fanin_mask & BIT_CHECK(i))
352 gpio14_17 |= FANIN_SEL(i);
353 } else if (i < 12) {
354 if (pwm_mask & BIT_CHECK(i))
355 gpio10_13 |= PWM_SEL(i);
356 if (fanin_mask & BIT_CHECK(i))
357 gpio0_3 |= FANIN_SEL(i);
358 } else {
359 if (pwm_mask & BIT_CHECK(i))
360 gpio14_17 |= PWM_SEL(i);
361 if (fanin_mask & BIT_CHECK(i))
362 gpio4_7 |= FANIN_SEL(i);
363 }
364 }
365
366 /* Pin Function Configuration */
> 367 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_0_3, gpio0_3);
368 if (ret < 0)
369 return ret;
370 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_4_7, gpio4_7);
371 if (ret < 0)
372 return ret;
373 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_10_13, gpio10_13);
374 if (ret < 0)
375 return ret;
376 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_14_17, gpio14_17);
377 if (ret < 0)
378 return ret;
379
380 /* PWM and FANIN Monitoring Enable */
381 ret = nct736x_write_reg(client, NCT736X_REG_PWMEN_0_7,
382 pwm_mask & 0xff);
383 if (ret < 0)
384 return ret;
385 ret = nct736x_write_reg(client,
386 NCT736X_REG_PWMEN_8_15, (pwm_mask >> 8) & 0xff);
387 if (ret < 0)
388 return ret;
389 ret = nct736x_write_reg(client, NCT736X_REG_FANINEN_0_7,
390 fanin_mask & 0xff);
391 if (ret < 0)
392 return ret;
393 ret = nct736x_write_reg(client, NCT736X_REG_FANINEN_8_15,
394 (fanin_mask >> 8) & 0xff);
395 if (ret < 0)
396 return ret;
397
398 /* Watchdog Timer Configuration */
399 if (wdt_cfg != 0xff && id->driver_data == nct7363) {
400 ret = nct736x_write_reg(client, NCT7363_REG_WDT, wdt_cfg);
401 if (ret < 0)
402 return ret;
403 }
404
405 return 0;
406 }
407
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 05/12/2023 10:31, Ban Feng wrote:
> Hi Krzysztof,
>
> On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 04/12/2023 06:56, [email protected] wrote:
>>> From: Ban Feng <[email protected]>
>>>
>>> This change documents the device tree bindings for the Nuvoton
>>> NCT7362Y, NCT7363Y driver.
>>>
>>> Signed-off-by: Ban Feng <[email protected]>
>>> ---
>>> .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++
>>> MAINTAINERS | 6 ++
>>> 2 files changed, 86 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
>>> new file mode 100644
>>> index 000000000000..f98fd260a20f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
>>> @@ -0,0 +1,80 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +
>>> +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Nuvoton NCT736X Hardware Monitoring IC
>>> +
>>> +maintainers:
>>> + - Ban Feng <[email protected]>
>>> +
>>> +description: |
>>> + The NCT736X is a Fan controller which provides up to 16 independent
>>> + FAN input monitors, and up to 16 independent PWM output with SMBus interface.
>>> + Besides, NCT7363Y has a built-in watchdog timer which is used for
>>> + conditionally generating a system reset output (INT#).
>>> +
>>> +additionalProperties: false
>>
>> Please place it just like other bindings are placing it. Not in some
>> random order. See example-schema.
>
> ok, I'll move additionalProperties to the correct place.
>
>>
>> You should use common fan properties. If it was not merged yet, you must
>> rebase on patchset on LKML and mention the dependency in the change log
>> (---).
>
> please kindly see below
>
>>
>>> +
>>> +properties:
>>> + compatible:
>>> + enum:
>>> + - nuvoton,nct7362
>>> + - nuvoton,nct7363
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + nuvoton,pwm-mask:
>>> + description: |
>>> + each bit means PWMx enable/disable setting, where x = 0~15.
>>> + 0: disabled, 1: enabled
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + minimum: 0x0
>>> + maximum: 0xFFFF
>>> + default: 0x0
>>
>> Use pwms, not own property for this.
>
> NCT736X has 16 funtion pins, they can be
> GPIO/PWM/FANIN/Reserved_or_ALERT#, and default is GPIO.
> We would like to add such a property that can configure the function
> pin for pin0~7 and pin10~17.
It looks you are writing custom pinctrl instead of using standard
bindings and frameworks.
>
> My original design is only for PWM/FANIN, however,
> I noticed that we can change the design to "nuvoton,pin[0-7]$" and
> "nuvoton,pin[10-17]$", like example in adt7475.yaml.
> I'm not sure if this can be accepted or not, please kindly review this.
It looks like the same problem as with other fan/Nuvoton bindings we
discussed some time ago on the lists.
Please do not duplicate threads, work and reviews:
https://lore.kernel.org/all/[email protected]/
I already gave same comments there.
>>> + nuvoton,wdt-timeout:
>>> + description: |
>>> + Watchdog Timer time configuration for NCT7363Y, as below
>>> + 0: 15 sec (default)
>>> + 1: 7.5 sec
>>> + 2: 3.75 sec
>>> + 3: 30 sec
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + enum: [0, 1, 2, 3]
>>> + default: 0
>>
>> Nope, reference watchdog.yaml and use its properties. See other watchdog
>> bindings for examples.
>
> NCT7363 has a built-in watchdog timer which is used for conditionally
> generating a system reset
> output (INT#) if the microcontroller or microprocessor stops to
> periodically send a pulse signal or
> interface communication access signal like SCL signal from SMBus interface.
>
> We only consider "Watchdog Timer Configuration Register" enable bit
> and timeout setting.
> Should we still need to add struct watchdog_device to struct nct736x_data?
I do not see correlation between watchdog.yaml and some struct. I did
not write anything about driver, so I don't understand your comments.
Anyway, I don't like that we are duplicating entire effort and our
review. Please join existing discussions, threads and build on top of it.
Best regards,
Krzysztof
Hi Krzysztof,
Krzysztof Kozlowski <[email protected]> 於 2023年12月4日 週一 下午4:06寫道：
>
> On 04/12/2023 06:56, [email protected] wrote:
> > From: Ban Feng <[email protected]>
> >
> > NCT736X is an I2C based hardware monitoring chip from Nuvoton.
> >
> > Signed-off-by: Ban Feng <[email protected]>
> > ---
>
>
> > +
> > +static const struct i2c_device_id nct736x_id[] = {
> > + {"nct7362", nct7362},
> > + {"nct7363", nct7363},
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, nct736x_id);
> > +
>
> All ID tables are next to each other. Move it down. Why does it not
> match of_device_id?
ok, I'll put all ID tables together,
and add .data to of_device_id so that matching i2c_device_id.
>
> ...
>
> > +
> > +static int nct736x_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct nct736x_data *data;
> > + struct device *hwmon_dev;
> > + u32 pwm_mask, fanin_mask, val, wdt_cfg;
> > + int ret;
> > +
> > + data = devm_kzalloc(dev, sizeof(struct nct736x_data), GFP_KERNEL);
>
> sizeof(*)
ok, I'll modify it in v2.
>
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(client, data);
> > + mutex_init(&data->update_lock);
> > +
> > + data->client = client;
> > +
> > + if (of_property_read_u32(dev->of_node, "nuvoton,pwm-mask", &pwm_mask))
> > + pwm_mask = 0;
> > + if (of_property_read_u32(dev->of_node,
> > + "nuvoton,fanin-mask", &fanin_mask))
> > + fanin_mask = 0;
> > + if (of_property_read_u32(dev->of_node, "nuvoton,wdt-timeout", &val))
> > + wdt_cfg = 0xff;
> > + else
> > + wdt_cfg = WDT_CFG(val) | EN_WDT;
> > +
> > + /* Initialize the chip */
> > + ret = nct736x_init_chip(client, pwm_mask, fanin_mask, wdt_cfg);
> > + if (ret)
> > + return ret;
> > +
> > + data->fan_mask = (u16)fanin_mask;
> > + data->pwm_mask = (u16)pwm_mask;
> > +
> > + data = nct736x_update_device(dev);
> > +
> > + data->groups[0] = &nct736x_group_fan;
> > + data->groups[1] = &nct736x_group_pwm;
> > + data->groups[2] = NULL;
> > +
> > + hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> > + client->name,
> > + data, data->groups);
> > + return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static const struct of_device_id nct736x_of_match[] = {
> > + { .compatible = "nuvoton,nct7362" },
> > + { .compatible = "nuvoton,nct7363" },
>
> This means your devices are compatible. Express compatibility in your
> bindings (specific compatible followed by fallback). But then your
> i2c_device_id is not matching this one here... confusing and clearly wrong.
>
Same as above.
> Best regards,
> Krzysztof
>
Hi,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/baneric926-gmail-com/dt-bindings-hwmon-Add-nct736x-bindings/20231204-135942
base: linus/master
patch link: https://lore.kernel.org/r/20231204055650.788388-3-kcfeng0%40nuvoton.com
patch subject: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
config: m68k-randconfig-r071-20231207 (https://download.01.org/0day-ci/archive/20231207/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231207/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/
smatch warnings:
drivers/hwmon/nct736x.c:367 nct736x_init_chip() error: uninitialized symbol 'gpio0_3'.
drivers/hwmon/nct736x.c:370 nct736x_init_chip() error: uninitialized symbol 'gpio4_7'.
drivers/hwmon/nct736x.c:373 nct736x_init_chip() error: uninitialized symbol 'gpio10_13'.
drivers/hwmon/nct736x.c:376 nct736x_init_chip() error: uninitialized symbol 'gpio14_17'.
vim +/gpio0_3 +367 drivers/hwmon/nct736x.c
16e62bcf3c9b93 Ban Feng 2023-12-04 335 static int nct736x_init_chip(struct i2c_client *client,
16e62bcf3c9b93 Ban Feng 2023-12-04 336 u32 pwm_mask, u32 fanin_mask, u32 wdt_cfg)
16e62bcf3c9b93 Ban Feng 2023-12-04 337 {
16e62bcf3c9b93 Ban Feng 2023-12-04 338 const struct i2c_device_id *id = i2c_match_id(nct736x_id, client);
16e62bcf3c9b93 Ban Feng 2023-12-04 339 u8 i, gpio0_3, gpio4_7, gpio10_13, gpio14_17;
16e62bcf3c9b93 Ban Feng 2023-12-04 340 int ret;
16e62bcf3c9b93 Ban Feng 2023-12-04 341
16e62bcf3c9b93 Ban Feng 2023-12-04 342 for (i = 0; i < NCT736X_PWM_COUNT; i++) {
16e62bcf3c9b93 Ban Feng 2023-12-04 343 if (i < 4) {
16e62bcf3c9b93 Ban Feng 2023-12-04 344 if (pwm_mask & BIT_CHECK(i))
16e62bcf3c9b93 Ban Feng 2023-12-04 345 gpio0_3 |= PWM_SEL(i);
This doesn't work. gpio0_3 needs to be initialized to zero before we
can turn on individual bits.
16e62bcf3c9b93 Ban Feng 2023-12-04 346 if (fanin_mask & BIT_CHECK(i))
16e62bcf3c9b93 Ban Feng 2023-12-04 347 gpio10_13 |= FANIN_SEL(i);
Etc...
16e62bcf3c9b93 Ban Feng 2023-12-04 348 } else if (i < 8) {
16e62bcf3c9b93 Ban Feng 2023-12-04 349 if (pwm_mask & BIT_CHECK(i))
16e62bcf3c9b93 Ban Feng 2023-12-04 350 gpio4_7 |= PWM_SEL(i);
16e62bcf3c9b93 Ban Feng 2023-12-04 351 if (fanin_mask & BIT_CHECK(i))
16e62bcf3c9b93 Ban Feng 2023-12-04 352 gpio14_17 |= FANIN_SEL(i);
16e62bcf3c9b93 Ban Feng 2023-12-04 353 } else if (i < 12) {
16e62bcf3c9b93 Ban Feng 2023-12-04 354 if (pwm_mask & BIT_CHECK(i))
16e62bcf3c9b93 Ban Feng 2023-12-04 355 gpio10_13 |= PWM_SEL(i);
16e62bcf3c9b93 Ban Feng 2023-12-04 356 if (fanin_mask & BIT_CHECK(i))
16e62bcf3c9b93 Ban Feng 2023-12-04 357 gpio0_3 |= FANIN_SEL(i);
16e62bcf3c9b93 Ban Feng 2023-12-04 358 } else {
16e62bcf3c9b93 Ban Feng 2023-12-04 359 if (pwm_mask & BIT_CHECK(i))
16e62bcf3c9b93 Ban Feng 2023-12-04 360 gpio14_17 |= PWM_SEL(i);
16e62bcf3c9b93 Ban Feng 2023-12-04 361 if (fanin_mask & BIT_CHECK(i))
16e62bcf3c9b93 Ban Feng 2023-12-04 362 gpio4_7 |= FANIN_SEL(i);
16e62bcf3c9b93 Ban Feng 2023-12-04 363 }
16e62bcf3c9b93 Ban Feng 2023-12-04 364 }
16e62bcf3c9b93 Ban Feng 2023-12-04 365
16e62bcf3c9b93 Ban Feng 2023-12-04 366 /* Pin Function Configuration */
16e62bcf3c9b93 Ban Feng 2023-12-04 @367 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_0_3, gpio0_3);
^^^^^^^
16e62bcf3c9b93 Ban Feng 2023-12-04 368 if (ret < 0)
16e62bcf3c9b93 Ban Feng 2023-12-04 369 return ret;
16e62bcf3c9b93 Ban Feng 2023-12-04 @370 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_4_7, gpio4_7);
16e62bcf3c9b93 Ban Feng 2023-12-04 371 if (ret < 0)
16e62bcf3c9b93 Ban Feng 2023-12-04 372 return ret;
16e62bcf3c9b93 Ban Feng 2023-12-04 @373 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_10_13, gpio10_13);
16e62bcf3c9b93 Ban Feng 2023-12-04 374 if (ret < 0)
16e62bcf3c9b93 Ban Feng 2023-12-04 375 return ret;
16e62bcf3c9b93 Ban Feng 2023-12-04 @376 ret = nct736x_write_reg(client, NCT736X_REG_GPIO_14_17, gpio14_17);
16e62bcf3c9b93 Ban Feng 2023-12-04 377 if (ret < 0)
16e62bcf3c9b93 Ban Feng 2023-12-04 378 return ret;
16e62bcf3c9b93 Ban Feng 2023-12-04 379
16e62bcf3c9b93 Ban Feng 2023-12-04 380 /* PWM and FANIN Monitoring Enable */
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Krzysztof,
On Tuesday, December 5, 2023, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 05/12/2023 10:31, Ban Feng wrote:
> > Hi Krzysztof,
> >
> > On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 04/12/2023 06:56, [email protected] wrote:
> >>> From: Ban Feng <[email protected]>
> >>>
> >>> This change documents the device tree bindings for the Nuvoton
> >>> NCT7362Y, NCT7363Y driver.
> >>>
> >>> Signed-off-by: Ban Feng <[email protected]>
> >>> ---
> >>> .../bindings/hwmon/nuvoton,nct736x.yaml | 80 +++++++++++++++++++
> >>> MAINTAINERS | 6 ++
> >>> 2 files changed, 86 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> >>> new file mode 100644
> >>> index 000000000000..f98fd260a20f
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
> >>> @@ -0,0 +1,80 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +
> >>> +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct736x.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Nuvoton NCT736X Hardware Monitoring IC
> >>> +
> >>> +maintainers:
> >>> + - Ban Feng <[email protected]>
> >>> +
> >>> +description: |
> >>> + The NCT736X is a Fan controller which provides up to 16 independent
> >>> + FAN input monitors, and up to 16 independent PWM output with SMBus interface.
> >>> + Besides, NCT7363Y has a built-in watchdog timer which is used for
> >>> + conditionally generating a system reset output (INT#).
> >>> +
> >>> +additionalProperties: false
> >>
> >> Please place it just like other bindings are placing it. Not in some
> >> random order. See example-schema.
> >
> > ok, I'll move additionalProperties to the correct place.
> >
> >>
> >> You should use common fan properties. If it was not merged yet, you must
> >> rebase on patchset on LKML and mention the dependency in the change log
> >> (---).
> >
> > please kindly see below
> >
> >>
> >>> +
> >>> +properties:
> >>> + compatible:
> >>> + enum:
> >>> + - nuvoton,nct7362
> >>> + - nuvoton,nct7363
> >>> +
> >>> + reg:
> >>> + maxItems: 1
> >>> +
> >>> + nuvoton,pwm-mask:
> >>> + description: |
> >>> + each bit means PWMx enable/disable setting, where x = 0~15.
> >>> + 0: disabled, 1: enabled
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 0x0
> >>> + maximum: 0xFFFF
> >>> + default: 0x0
> >>
> >> Use pwms, not own property for this.
> >
> > NCT736X has 16 funtion pins, they can be
> > GPIO/PWM/FANIN/Reserved_or_ALERT#, and default is GPIO.
> > We would like to add such a property that can configure the function
> > pin for pin0~7 and pin10~17.
>
> It looks you are writing custom pinctrl instead of using standard
> bindings and frameworks.
Please kindly see below
>
>
> >
> > My original design is only for PWM/FANIN, however,
> > I noticed that we can change the design to "nuvoton,pin[0-7]$" and
> > "nuvoton,pin[10-17]$", like example in adt7475.yaml.
> > I'm not sure if this can be accepted or not, please kindly review this.
>
> It looks like the same problem as with other fan/Nuvoton bindings we
> discussed some time ago on the lists.
>
> Please do not duplicate threads, work and reviews:
> https://lore.kernel.org/all/[email protected]/
>
> I already gave same comments there.
Thanks for your kindly sharing. I noticed that [1] defines some useful
properties, pwms and tach-ch, like you mentioned.
Therefore, I'll modify our design to follow the common fan properties in v2.
[1] https://lists.openwall.net/linux-kernel/2023/11/07/406
>
>
> >>> + nuvoton,wdt-timeout:
> >>> + description: |
> >>> + Watchdog Timer time configuration for NCT7363Y, as below
> >>> + 0: 15 sec (default)
> >>> + 1: 7.5 sec
> >>> + 2: 3.75 sec
> >>> + 3: 30 sec
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + enum: [0, 1, 2, 3]
> >>> + default: 0
> >>
> >> Nope, reference watchdog.yaml and use its properties. See other watchdog
> >> bindings for examples.
> >
> > NCT7363 has a built-in watchdog timer which is used for conditionally
> > generating a system reset
> > output (INT#) if the microcontroller or microprocessor stops to
> > periodically send a pulse signal or
> > interface communication access signal like SCL signal from SMBus interface.
> >
> > We only consider "Watchdog Timer Configuration Register" enable bit
> > and timeout setting.
> > Should we still need to add struct watchdog_device to struct nct736x_data?
>
> I do not see correlation between watchdog.yaml and some struct. I did
> not write anything about driver, so I don't understand your comments.
>
> Anyway, I don't like that we are duplicating entire effort and our
> review. Please join existing discussions, threads and build on top of it.
>
Thanks, I'll remove unused function in hwmon subsystem,
and consider a watchdog subsystem design if necessary.
>
> Best regards,
> Krzysztof
>