All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X
@ 2023-12-04  5:56 baneric926
  2023-12-04  5:56 ` [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings baneric926
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: baneric926 @ 2023-12-04  5:56 UTC (permalink / raw)
  To: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, kwliu,
	kcfeng0, DELPHINE_CHIU, Bonnie_Lo, Ban Feng

From: Ban Feng <baneric926@gmail.com>

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings
  2023-12-04  5:56 [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X baneric926
@ 2023-12-04  5:56 ` baneric926
  2023-12-04  8:04   ` Krzysztof Kozlowski
  2023-12-04  5:56 ` [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X baneric926
  2023-12-04  7:04   ` Guenter Roeck
  2 siblings, 1 reply; 23+ messages in thread
From: baneric926 @ 2023-12-04  5:56 UTC (permalink / raw)
  To: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, kwliu,
	kcfeng0, DELPHINE_CHIU, Bonnie_Lo

From: Ban Feng <kcfeng0@nuvoton.com>

This change documents the device tree bindings for the Nuvoton
NCT7362Y, NCT7363Y driver.

Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
---
 .../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 <kcfeng0@nuvoton.com>
+
+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 <kcfeng0@nuvoton.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
+
 NETDEVSIM
 M:	Jakub Kicinski <kuba@kernel.org>
 S:	Maintained
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  5:56 [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X baneric926
  2023-12-04  5:56 ` [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings baneric926
@ 2023-12-04  5:56 ` baneric926
  2023-12-04  7:06     ` Guenter Roeck
                     ` (4 more replies)
  2023-12-04  7:04   ` Guenter Roeck
  2 siblings, 5 replies; 23+ messages in thread
From: baneric926 @ 2023-12-04  5:56 UTC (permalink / raw)
  To: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, kwliu,
	kcfeng0, DELPHINE_CHIU, Bonnie_Lo

From: Ban Feng <kcfeng0@nuvoton.com>

NCT736X is an I2C based hardware monitoring chip from Nuvoton.

Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
---
 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 <kcfeng0@nuvoton.com>
+
+
+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 <kcfeng0@nuvoton.com>
 L:	linux-hwmon@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/hwmon/nuvoton,nct736x.yaml
+F:	Documentation/hwmon/nct736x.rst
+F:	drivers/hwmon/nct736x.c
 
 NETDEVSIM
 M:	Jakub Kicinski <kuba@kernel.org>
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 <cwho@nuvoton.com>");
+MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
+MODULE_DESCRIPTION("NCT736X Hardware Monitoring Driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  5:56 [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X baneric926
@ 2023-12-04  7:04   ` Guenter Roeck
  2023-12-04  5:56 ` [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X baneric926
  2023-12-04  7:04   ` Guenter Roeck
  2 siblings, 0 replies; 23+ messages in thread
From: Guenter Roeck @ 2023-12-04  7:04 UTC (permalink / raw)
  To: baneric926, jdelvare, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, kwliu,
	kcfeng0, DELPHINE_CHIU, Bonnie_Lo

On 12/3/23 21:56, baneric926@gmail.com wrote:
> From: Ban Feng <baneric926@gmail.com>
> 
> 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X
@ 2023-12-04  7:04   ` Guenter Roeck
  0 siblings, 0 replies; 23+ messages in thread
From: Guenter Roeck @ 2023-12-04  7:04 UTC (permalink / raw)
  To: baneric926, jdelvare, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, kcfeng0, kwliu, openbmc, linux-doc,
	linux-kernel, DELPHINE_CHIU

On 12/3/23 21:56, baneric926@gmail.com wrote:
> From: Ban Feng <baneric926@gmail.com>
> 
> 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  5:56 ` [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X baneric926
@ 2023-12-04  7:06     ` Guenter Roeck
  2023-12-04  8:06   ` Krzysztof Kozlowski
                       ` (3 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Guenter Roeck @ 2023-12-04  7:06 UTC (permalink / raw)
  To: baneric926, jdelvare, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, kwliu,
	kcfeng0, DELPHINE_CHIU, Bonnie_Lo

On 12/3/23 21:56, baneric926@gmail.com wrote:
> From: Ban Feng <kcfeng0@nuvoton.com>
> 
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
> 
> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> ---
[ ... ]

> +	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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
@ 2023-12-04  7:06     ` Guenter Roeck
  0 siblings, 0 replies; 23+ messages in thread
From: Guenter Roeck @ 2023-12-04  7:06 UTC (permalink / raw)
  To: baneric926, jdelvare, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, kcfeng0, kwliu, openbmc, linux-doc,
	linux-kernel, DELPHINE_CHIU

On 12/3/23 21:56, baneric926@gmail.com wrote:
> From: Ban Feng <kcfeng0@nuvoton.com>
> 
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
> 
> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> ---
[ ... ]

> +	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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings
  2023-12-04  5:56 ` [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings baneric926
@ 2023-12-04  8:04   ` Krzysztof Kozlowski
  2023-12-05  9:31     ` Ban Feng
  0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2023-12-04  8:04 UTC (permalink / raw)
  To: baneric926, jdelvare, linux, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, kwliu,
	kcfeng0, DELPHINE_CHIU, Bonnie_Lo

On 04/12/2023 06:56, baneric926@gmail.com wrote:
> From: Ban Feng <kcfeng0@nuvoton.com>
> 
> This change documents the device tree bindings for the Nuvoton
> NCT7362Y, NCT7363Y driver.
> 
> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> ---
>  .../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 <kcfeng0@nuvoton.com>
> +
> +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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  5:56 ` [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X baneric926
  2023-12-04  7:06     ` Guenter Roeck
@ 2023-12-04  8:06   ` Krzysztof Kozlowski
  2023-12-06  3:35     ` Ban Feng
  2023-12-05 11:04     ` kernel test robot
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2023-12-04  8:06 UTC (permalink / raw)
  To: baneric926, jdelvare, linux, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, kwliu,
	kcfeng0, DELPHINE_CHIU, Bonnie_Lo

On 04/12/2023 06:56, baneric926@gmail.com wrote:
> From: Ban Feng <kcfeng0@nuvoton.com>
> 
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
> 
> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> ---


> +
> +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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* RE: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  7:06     ` Guenter Roeck
  (?)
@ 2023-12-05  5:02     ` KCFENG0
  2023-12-05  8:04       ` Krzysztof Kozlowski
  -1 siblings, 1 reply; 23+ messages in thread
From: KCFENG0 @ 2023-12-05  5:02 UTC (permalink / raw)
  To: Guenter Roeck, baneric926, jdelvare, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, KWLIU,
	DELPHINE_CHIU, Bonnie_Lo

Hi Guenter

-----Original Message-----
From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
Sent: Monday, December 4, 2023 3:07 PM
To: baneric926@gmail.com; jdelvare@suse.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; corbet@lwn.net
Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org; openbmc@lists.ozlabs.org; CS20 KWLiu <KWLIU@nuvoton.com>; CS20 KCFeng0 <KCFENG0@nuvoton.com>; DELPHINE_CHIU@wiwynn.com; Bonnie_Lo@wiwynn.com
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, baneric926@gmail.com wrote:
> From: Ban Feng <kcfeng0@nuvoton.com>
>
> NCT736X is an I2C based hardware monitoring chip from Nuvoton.
>
> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> ---
[ ... ]

> +     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.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  7:04   ` Guenter Roeck
  (?)
@ 2023-12-05  7:00   ` Ban Feng
  -1 siblings, 0 replies; 23+ messages in thread
From: Ban Feng @ 2023-12-05  7:00 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: jdelvare, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet,
	linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, kwliu,
	kcfeng0, DELPHINE_CHIU, Bonnie_Lo

On Mon, Dec 4, 2023 at 3:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 12/3/23 21:56, baneric926@gmail.com wrote:
> > From: Ban Feng <baneric926@gmail.com>
> >
> > 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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-05  5:02     ` KCFENG0
@ 2023-12-05  8:04       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 23+ messages in thread
From: Krzysztof Kozlowski @ 2023-12-05  8:04 UTC (permalink / raw)
  To: KCFENG0, Guenter Roeck, baneric926, jdelvare, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, openbmc, KWLIU,
	DELPHINE_CHIU, Bonnie_Lo

On 05/12/2023 06:02, KCFENG0@nuvoton.com 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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings
  2023-12-04  8:04   ` Krzysztof Kozlowski
@ 2023-12-05  9:31     ` Ban Feng
  2023-12-05 15:49       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 23+ messages in thread
From: Ban Feng @ 2023-12-05  9:31 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	corbet, linux-hwmon, devicetree, linux-kernel, linux-doc,
	openbmc, kwliu, kcfeng0, DELPHINE_CHIU, Bonnie_Lo

Hi Krzysztof,

On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 04/12/2023 06:56, baneric926@gmail.com wrote:
> > From: Ban Feng <kcfeng0@nuvoton.com>
> >
> > This change documents the device tree bindings for the Nuvoton
> > NCT7362Y, NCT7363Y driver.
> >
> > Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> > ---
> >  .../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 <kcfeng0@nuvoton.com>
> > +
> > +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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  5:56 ` [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X baneric926
@ 2023-12-05 11:04     ` kernel test robot
  2023-12-04  8:06   ` Krzysztof Kozlowski
                       ` (3 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2023-12-05 11:04 UTC (permalink / raw)
  To: baneric926, jdelvare, linux, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, corbet
  Cc: llvm, oe-kbuild-all, linux-hwmon, devicetree, linux-kernel,
	linux-doc, openbmc, kwliu, kcfeng0, DELPHINE_CHIU, Bonnie_Lo

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/202312051854.qBIoJW1N-lkp@intel.com/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/202312051854.qBIoJW1N-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051854.qBIoJW1N-lkp@intel.com/

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
@ 2023-12-05 11:04     ` kernel test robot
  0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2023-12-05 11:04 UTC (permalink / raw)
  To: baneric926, jdelvare, linux, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, corbet
  Cc: linux-hwmon, devicetree, kcfeng0, linux-doc, openbmc, llvm,
	linux-kernel, DELPHINE_CHIU, oe-kbuild-all, kwliu

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/202312051854.qBIoJW1N-lkp@intel.com/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/202312051854.qBIoJW1N-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312051854.qBIoJW1N-lkp@intel.com/

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  5:56 ` [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X baneric926
@ 2023-12-05 14:14     ` kernel test robot
  2023-12-04  8:06   ` Krzysztof Kozlowski
                       ` (3 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2023-12-05 14:14 UTC (permalink / raw)
  To: baneric926, jdelvare, linux, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, corbet
  Cc: oe-kbuild-all, linux-hwmon, devicetree, linux-kernel, linux-doc,
	openbmc, kwliu, kcfeng0, DELPHINE_CHIU, Bonnie_Lo

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/202312052243.AbRqbNyT-lkp@intel.com/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/202312052243.AbRqbNyT-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312052243.AbRqbNyT-lkp@intel.com/

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
@ 2023-12-05 14:14     ` kernel test robot
  0 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2023-12-05 14:14 UTC (permalink / raw)
  To: baneric926, jdelvare, linux, robh+dt, krzysztof.kozlowski+dt,
	conor+dt, corbet
  Cc: linux-hwmon, devicetree, kcfeng0, linux-doc, openbmc,
	linux-kernel, DELPHINE_CHIU, oe-kbuild-all, kwliu

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/202312052243.AbRqbNyT-lkp@intel.com/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/202312052243.AbRqbNyT-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312052243.AbRqbNyT-lkp@intel.com/

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings
  2023-12-05  9:31     ` Ban Feng
@ 2023-12-05 15:49       ` Krzysztof Kozlowski
  2023-12-08  3:05           ` Ban Feng
  0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2023-12-05 15:49 UTC (permalink / raw)
  To: Ban Feng
  Cc: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	corbet, linux-hwmon, devicetree, linux-kernel, linux-doc,
	openbmc, kwliu, kcfeng0, DELPHINE_CHIU, Bonnie_Lo

On 05/12/2023 10:31, Ban Feng wrote:
> Hi Krzysztof,
> 
> On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 04/12/2023 06:56, baneric926@gmail.com wrote:
>>> From: Ban Feng <kcfeng0@nuvoton.com>
>>>
>>> This change documents the device tree bindings for the Nuvoton
>>> NCT7362Y, NCT7363Y driver.
>>>
>>> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
>>> ---
>>>  .../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 <kcfeng0@nuvoton.com>
>>> +
>>> +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/20230607101827.8544-5-zev@bewilderbeest.net/

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  8:06   ` Krzysztof Kozlowski
@ 2023-12-06  3:35     ` Ban Feng
  0 siblings, 0 replies; 23+ messages in thread
From: Ban Feng @ 2023-12-06  3:35 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	corbet, linux-hwmon, devicetree, linux-kernel, linux-doc,
	openbmc, kwliu, kcfeng0, DELPHINE_CHIU, Bonnie_Lo

Hi Krzysztof,

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2023年12月4日 週一 下午4:06寫道:
>
> On 04/12/2023 06:56, baneric926@gmail.com wrote:
> > From: Ban Feng <kcfeng0@nuvoton.com>
> >
> > NCT736X is an I2C based hardware monitoring chip from Nuvoton.
> >
> > Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> > ---
>
>
> > +
> > +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
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
  2023-12-04  5:56 ` [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X baneric926
@ 2023-12-07  5:41     ` Dan Carpenter
  2023-12-04  8:06   ` Krzysztof Kozlowski
                       ` (3 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Dan Carpenter @ 2023-12-07  5:41 UTC (permalink / raw)
  To: oe-kbuild, baneric926, jdelvare, linux, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: lkp, oe-kbuild-all, linux-hwmon, devicetree, linux-kernel,
	linux-doc, openbmc, kwliu, kcfeng0, DELPHINE_CHIU, Bonnie_Lo

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/202312071152.Kfcw1KlD-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231207/202312071152.Kfcw1KlD-lkp@intel.com/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 <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202312071152.Kfcw1KlD-lkp@intel.com/

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X
@ 2023-12-07  5:41     ` Dan Carpenter
  0 siblings, 0 replies; 23+ messages in thread
From: Dan Carpenter @ 2023-12-07  5:41 UTC (permalink / raw)
  To: oe-kbuild, baneric926, jdelvare, linux, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, corbet
  Cc: linux-hwmon, devicetree, kcfeng0, lkp, linux-doc, openbmc,
	linux-kernel, DELPHINE_CHIU, oe-kbuild-all, kwliu

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/202312071152.Kfcw1KlD-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231207/202312071152.Kfcw1KlD-lkp@intel.com/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 <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202312071152.Kfcw1KlD-lkp@intel.com/

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings
  2023-12-05 15:49       ` Krzysztof Kozlowski
@ 2023-12-08  3:05           ` Ban Feng
  0 siblings, 0 replies; 23+ messages in thread
From: Ban Feng @ 2023-12-08  3:05 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	corbet, linux-hwmon, devicetree, linux-kernel, linux-doc,
	openbmc, kwliu, kcfeng0, DELPHINE_CHIU, Bonnie_Lo

Hi Krzysztof,

On Tuesday, December 5, 2023, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/12/2023 10:31, Ban Feng wrote:
> > Hi Krzysztof,
> >
> > On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 04/12/2023 06:56, baneric926@gmail.com wrote:
> >>> From: Ban Feng <kcfeng0@nuvoton.com>
> >>>
> >>> This change documents the device tree bindings for the Nuvoton
> >>> NCT7362Y, NCT7363Y driver.
> >>>
> >>> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> >>> ---
> >>>  .../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 <kcfeng0@nuvoton.com>
> >>> +
> >>> +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/20230607101827.8544-5-zev@bewilderbeest.net/
>
> 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
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings
@ 2023-12-08  3:05           ` Ban Feng
  0 siblings, 0 replies; 23+ messages in thread
From: Ban Feng @ 2023-12-08  3:05 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: linux-hwmon, devicetree, conor+dt, jdelvare, corbet, openbmc,
	linux-doc, linux-kernel, DELPHINE_CHIU, kcfeng0, robh+dt,
	krzysztof.kozlowski+dt, kwliu, linux

Hi Krzysztof,

On Tuesday, December 5, 2023, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/12/2023 10:31, Ban Feng wrote:
> > Hi Krzysztof,
> >
> > On Mon, Dec 4, 2023 at 4:04 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 04/12/2023 06:56, baneric926@gmail.com wrote:
> >>> From: Ban Feng <kcfeng0@nuvoton.com>
> >>>
> >>> This change documents the device tree bindings for the Nuvoton
> >>> NCT7362Y, NCT7363Y driver.
> >>>
> >>> Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> >>> ---
> >>>  .../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 <kcfeng0@nuvoton.com>
> >>> +
> >>> +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/20230607101827.8544-5-zev@bewilderbeest.net/
>
> 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
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2023-12-11 23:17 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-04  5:56 [PATCH v1 0/2] hwmon: Driver for Nuvoton NCT736X baneric926
2023-12-04  5:56 ` [PATCH v1 1/2] dt-bindings: hwmon: Add nct736x bindings baneric926
2023-12-04  8:04   ` Krzysztof Kozlowski
2023-12-05  9:31     ` Ban Feng
2023-12-05 15:49       ` Krzysztof Kozlowski
2023-12-08  3:05         ` Ban Feng
2023-12-08  3:05           ` Ban Feng
2023-12-04  5:56 ` [PATCH v1 2/2] hwmon: Driver for Nuvoton NCT736X baneric926
2023-12-04  7:06   ` Guenter Roeck
2023-12-04  7:06     ` Guenter Roeck
2023-12-05  5:02     ` KCFENG0
2023-12-05  8:04       ` Krzysztof Kozlowski
2023-12-04  8:06   ` Krzysztof Kozlowski
2023-12-06  3:35     ` Ban Feng
2023-12-05 11:04   ` kernel test robot
2023-12-05 11:04     ` kernel test robot
2023-12-05 14:14   ` kernel test robot
2023-12-05 14:14     ` kernel test robot
2023-12-07  5:41   ` Dan Carpenter
2023-12-07  5:41     ` Dan Carpenter
2023-12-04  7:04 ` [PATCH v1 0/2] " Guenter Roeck
2023-12-04  7:04   ` Guenter Roeck
2023-12-05  7:00   ` Ban Feng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.