[PATCH v1 0/2] Add Nuvoton NCT7718W IIO driver

[PATCH v1 0/2] Add Nuvoton NCT7718W IIO driver

[Ming Yu]

NCT7718W is an I2C based thermal sensor chip from Nuvoton.

Ming Yu (2):
  dt-bindings: iio: temperature: Add support for NCT7718W
  iio: temperature: Add Nuvoton NCT7718W support

 .../iio/temperature/nuvoton,nct7718.yaml      |  44 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/temperature/Kconfig               |  10 +
 drivers/iio/temperature/Makefile              |   1 +
 drivers/iio/temperature/nct7718.c             | 505 ++++++++++++++++++
 5 files changed, 567 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
 create mode 100644 drivers/iio/temperature/nct7718.c

-- 
2.34.1

[PATCH v1 1/2] dt-bindings: iio: temperature: Add support for NCT7718W

[Ming Yu]

Add devicetree binding document for Nuvoton NCT7718W thermal sensor.

Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
---
 .../iio/temperature/nuvoton,nct7718.yaml      | 44 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml

diff --git a/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml b/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
new file mode 100644
index 000000000000..a3573e3d454d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/temperature/nuvoton,nct7718.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton NCT7718W Thermal Sensor IC
+
+maintainers:
+  - Ming Yu <tmyu0@nuvoton.com>
+
+description:
+  https://www.nuvoton.com/resource-files/Nuvoton_NCT7718W_Datasheet_V11.pdf
+
+properties:
+  compatible:
+    const: nuvoton,nct7718
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        temp-sensor@4c {
+            compatible = "nuvoton,nct7718";
+            reg = <0x4c>;
+            interrupt-parent = <&gpio0>;
+            interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 67d2159406c2..6d147ce6b060 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16541,6 +16541,12 @@ F:	drivers/nubus/
 F:	include/linux/nubus.h
 F:	include/uapi/linux/nubus.h
 
+NUVOTON NCT7718W TEMPERATURE SENSOR DRIVER
+M:	Ming Yu <tmyu0@nuvoton.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
+
 NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
 M:	Antonino Daplas <adaplas@gmail.com>
 L:	linux-fbdev@vger.kernel.org
-- 
2.34.1

Re: [PATCH v1 1/2] dt-bindings: iio: temperature: Add support for NCT7718W

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

On Tue, Nov 26, 2024 at 03:40:04PM +0800, Ming Yu wrote:
> Add devicetree binding document for Nuvoton NCT7718W thermal sensor.
> 
> Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
> ---
>  .../iio/temperature/nuvoton,nct7718.yaml      | 44 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml b/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
> new file mode 100644
> index 000000000000..a3573e3d454d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/nuvoton,nct7718.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton NCT7718W Thermal Sensor IC
> +
> +maintainers:
> +  - Ming Yu <tmyu0@nuvoton.com>
> +
> +description:
> +  https://www.nuvoton.com/resource-files/Nuvoton_NCT7718W_Datasheet_V11.pdf
> +
> +properties:
> +  compatible:
> +    const: nuvoton,nct7718
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg

Please add the vdd supply as a required property.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        temp-sensor@4c {

The generic node name is actually temperature-sensor.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 1/2] dt-bindings: iio: temperature: Add support for NCT7718W

[Ming Yu]

Dear Conor,

Thank you for your comments,
I will make the modifications in the next patch.

Best regards,
Ming.

Conor Dooley <conor@kernel.org> 於 2024年11月27日 週三 上午1:58寫道：
>
> On Tue, Nov 26, 2024 at 03:40:04PM +0800, Ming Yu wrote:
> > Add devicetree binding document for Nuvoton NCT7718W thermal sensor.
> >
> > Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
> > ---
> >  .../iio/temperature/nuvoton,nct7718.yaml      | 44 +++++++++++++++++++
> >  MAINTAINERS                                   |  6 +++
> >  2 files changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml b/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
> > new file mode 100644
> > index 000000000000..a3573e3d454d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
> > @@ -0,0 +1,44 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/temperature/nuvoton,nct7718.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Nuvoton NCT7718W Thermal Sensor IC
> > +
> > +maintainers:
> > +  - Ming Yu <tmyu0@nuvoton.com>
> > +
> > +description:
> > +  https://www.nuvoton.com/resource-files/Nuvoton_NCT7718W_Datasheet_V11.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: nuvoton,nct7718
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
>
> Please add the vdd supply as a required property.
>
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        temp-sensor@4c {
>
> The generic node name is actually temperature-sensor.
>
> Thanks,
> Conor.

[PATCH v1 2/2] iio: temperature: Add Nuvoton NCT7718W support

[Ming Yu]

This patch adds support for the Nuvoton NCT7718W temperature sensor.

Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
---
 MAINTAINERS                       |   1 +
 drivers/iio/temperature/Kconfig   |  10 +
 drivers/iio/temperature/Makefile  |   1 +
 drivers/iio/temperature/nct7718.c | 505 ++++++++++++++++++++++++++++++
 4 files changed, 517 insertions(+)
 create mode 100644 drivers/iio/temperature/nct7718.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6d147ce6b060..5600b4f7c4bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16546,6 +16546,7 @@ M:	Ming Yu <tmyu0@nuvoton.com>
 L:	linux-iio@vger.kernel.org
 S:	Supported
 F:	Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
+F:	drivers/iio/temperature/nct7718.c
 
 NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
 M:	Antonino Daplas <adaplas@gmail.com>
diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index ed0e4963362f..e57d643fa664 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -180,4 +180,14 @@ config MCP9600
 	  This driver can also be built as a module. If so, the module
 	  will be called mcp9600.
 
+config NCT7718
+	tristate "Nuvoton NCT7718W thermal sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Nuvoton NCT7718W
+	  thermal sensor.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nct7718.
+
 endmenu
diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
index 07d6e65709f7..fa1543ebd105 100644
--- a/drivers/iio/temperature/Makefile
+++ b/drivers/iio/temperature/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_MCP9600) += mcp9600.o
 obj-$(CONFIG_MLX90614) += mlx90614.o
 obj-$(CONFIG_MLX90632) += mlx90632.o
 obj-$(CONFIG_MLX90632) += mlx90635.o
+obj-$(CONFIG_NCT7718) += nct7718.o
 obj-$(CONFIG_TMP006) += tmp006.o
 obj-$(CONFIG_TMP007) += tmp007.o
 obj-$(CONFIG_TMP117) += tmp117.o
diff --git a/drivers/iio/temperature/nct7718.c b/drivers/iio/temperature/nct7718.c
new file mode 100644
index 000000000000..60624b3de629
--- /dev/null
+++ b/drivers/iio/temperature/nct7718.c
@@ -0,0 +1,505 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * nct7718.c - Support for Nuvoton NCT7718W Thermal Sensor IC.
+ *
+ * Copyright (C) 2024 Nuvoton Technology Corp.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/bits.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/events.h>
+
+#define NCT7718_LDT_REG			0x00
+#define NCT7718_RT1_TEMP_MSB		0x01
+#define NCT7718_RT1_TEMP_LSB		0x10
+
+#define NCT7718_ALERTSTATUS_REG		0x02
+#define NCT7718_CONFIG_REG		0x03
+#define NCT7718_ALERTMASK_REG		0x16
+#define NCT7718_ALERTMODE_REG		0xBF
+
+#define NCT7718_LT_HALERT_REG		0x05
+#define NCT7718_RT1_HALERT_MSB_REG	0x0D
+#define NCT7718_RT1_LALERT_MSB_REG	0x0E
+#define NCT7718_RT1_HALERT_LSB_REG	0x13
+#define NCT7718_RT1_LALERT_LSB_REG	0x14
+
+#define NCT7718_CID_REG			0xFD
+#define NCT7718_VID_REG			0xFE
+#define NCT7718_DID_REG			0xFF
+
+#define NCT7718_CHIP_ID			0x50
+#define NCT7718_VENDOR_ID		0x50
+#define NCT7718_DEVICE_ID		0x91
+
+#define NCT7718_LSB_REG_MASK		GENMASK(7, 5)
+#define NCT7718_STS_RT1LA		BIT(3)
+#define NCT7718_STS_RT1HA		BIT(4)
+#define NCT7718_STS_LTHA		BIT(6)
+#define NCT7718_MSK_RT1L		BIT(3)
+#define NCT7718_MSK_RT1H		BIT(4)
+#define NCT7718_MSK_LTH			BIT(7)
+#define NCT7718_MOD_COMP		BIT(0)
+
+#define NCT7718_LOCAL_TEMP_MAX		125
+#define NCT7718_LOCAL_TEMP_MIN		-40
+#define NCT7718_REMOTE_TEMP_MAX_MICRO	127000000
+#define NCT7718_REMOTE_TEMP_MIN_MICRO	-40000000
+
+struct nct7718_data {
+	struct i2c_client *client;
+	struct mutex lock;
+	u16 status_mask;
+};
+
+static const struct iio_event_spec nct7718_local_temp_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_event_spec nct7718_remote_temp_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_chan_spec nct7718_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.modified = 1,
+		.channel2 = IIO_MOD_TEMP_AMBIENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.event_spec = nct7718_local_temp_events,
+		.num_event_specs = ARRAY_SIZE(nct7718_local_temp_events),
+	},
+	{
+		.type = IIO_TEMP,
+		.modified = 1,
+		.channel2 = IIO_MOD_TEMP_OBJECT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.event_spec = nct7718_remote_temp_events,
+		.num_event_specs = ARRAY_SIZE(nct7718_remote_temp_events),
+	}
+};
+
+static int nct7718_read_temp_raw(struct iio_dev *indio_dev,
+				 u8 msb_reg, u8 lsb_reg)
+{
+	struct nct7718_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	u16 reg_val;
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, msb_reg);
+	if (ret < 0)
+		return ret;
+
+	reg_val = (u16)(ret << 3);
+
+	ret = i2c_smbus_read_byte_data(client, lsb_reg);
+	if (ret < 0)
+		return ret;
+
+	reg_val |= FIELD_GET(NCT7718_LSB_REG_MASK, ret);
+
+	return reg_val;
+}
+
+static int nct7718_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct nct7718_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->channel2) {
+		case IIO_MOD_TEMP_AMBIENT:
+			ret = i2c_smbus_read_byte_data(client,
+						       NCT7718_LDT_REG);
+			if (ret < 0)
+				return ret;
+			*val = sign_extend32(ret, 7) << 3;
+			break;
+		case IIO_MOD_TEMP_OBJECT:
+			ret = nct7718_read_temp_raw(indio_dev,
+						    NCT7718_RT1_TEMP_MSB,
+						    NCT7718_RT1_TEMP_LSB);
+			if (ret < 0)
+				return ret;
+			*val = sign_extend32(ret, 10);
+			break;
+		default:
+			return -EINVAL;
+		}
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 125;
+		*val2 = 0;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int nct7718_read_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct nct7718_data *data = iio_priv(indio_dev);
+	unsigned int mask;
+
+	switch (chan->channel2) {
+	case IIO_MOD_TEMP_AMBIENT:
+		if (dir == IIO_EV_DIR_RISING)
+			mask = NCT7718_MSK_LTH;
+		break;
+	case IIO_MOD_TEMP_OBJECT:
+		if (dir == IIO_EV_DIR_RISING)
+			mask = NCT7718_MSK_RT1H;
+		else
+			mask = NCT7718_MSK_RT1L;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return !(data->status_mask & mask);
+}
+
+static int nct7718_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      int state)
+{
+	struct nct7718_data *data = iio_priv(indio_dev);
+	unsigned int status_mask;
+	int ret;
+
+	switch (chan->channel2) {
+	case IIO_MOD_TEMP_AMBIENT:
+		if (dir == IIO_EV_DIR_RISING)
+			status_mask = NCT7718_MSK_LTH;
+		break;
+	case IIO_MOD_TEMP_OBJECT:
+		if (dir == IIO_EV_DIR_RISING)
+			status_mask = NCT7718_MSK_RT1H;
+		else
+			status_mask = NCT7718_MSK_RT1L;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_read_byte_data(data->client, NCT7718_ALERTMASK_REG);
+	mutex_unlock(&data->lock);
+	if (ret < 0)
+		return ret;
+
+	if (state)
+		ret &= ~status_mask;
+	else
+		ret |=  status_mask;
+
+	return i2c_smbus_write_byte_data(data->client, NCT7718_ALERTMASK_REG,
+					 data->status_mask = ret);
+}
+
+static int nct7718_read_thresh(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info,
+			       int *val, int *val2)
+{
+	struct nct7718_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	u8 msb_reg, lsb_reg;
+	int ret;
+
+	switch (chan->channel2) {
+	case IIO_MOD_TEMP_AMBIENT:
+		if (dir == IIO_EV_DIR_RISING) {
+			ret = i2c_smbus_read_byte_data(client,
+						       NCT7718_LT_HALERT_REG);
+			if (ret < 0)
+				return ret;
+			*val = sign_extend32(ret, 7);
+			return IIO_VAL_INT;
+		}
+		return -EINVAL;
+	case IIO_MOD_TEMP_OBJECT:
+		if (dir == IIO_EV_DIR_RISING) {
+			msb_reg = NCT7718_RT1_HALERT_MSB_REG;
+			lsb_reg = NCT7718_RT1_HALERT_LSB_REG;
+		} else {
+			msb_reg = NCT7718_RT1_LALERT_MSB_REG;
+			lsb_reg = NCT7718_RT1_LALERT_LSB_REG;
+		}
+		ret = nct7718_read_temp_raw(indio_dev, msb_reg, lsb_reg);
+		if (ret < 0)
+			return ret;
+
+		*val = sign_extend32(ret, 10);
+		*val2 = 8;
+		return IIO_VAL_FRACTIONAL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int nct7718_write_thresh(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int val, int val2)
+{
+	struct nct7718_data *data = iio_priv(indio_dev);
+	struct i2c_client *client = data->client;
+	u8 msb_reg, lsb_reg;
+	s16 thresh;
+	int ret, s_val;
+
+	switch (chan->channel2) {
+	case IIO_MOD_TEMP_AMBIENT:
+		val = clamp_val(val, NCT7718_LOCAL_TEMP_MIN,
+				NCT7718_LOCAL_TEMP_MAX);
+
+		if (dir == IIO_EV_DIR_RISING) {
+			return i2c_smbus_write_byte_data(client,
+							 NCT7718_LT_HALERT_REG,
+							 val);
+		}
+		break;
+	case IIO_MOD_TEMP_OBJECT:
+		s_val = (val < 0) ? ((val * 1000000) - val2) :
+				    ((val * 1000000) + val2);
+
+		s_val = clamp_val(s_val, NCT7718_REMOTE_TEMP_MIN_MICRO,
+				  NCT7718_REMOTE_TEMP_MAX_MICRO);
+
+		if (dir == IIO_EV_DIR_RISING) {
+			msb_reg = NCT7718_RT1_HALERT_MSB_REG;
+			lsb_reg = NCT7718_RT1_HALERT_LSB_REG;
+		} else {
+			msb_reg = NCT7718_RT1_LALERT_MSB_REG;
+			lsb_reg = NCT7718_RT1_LALERT_LSB_REG;
+		}
+
+		thresh = (s16)(s_val / (1000000 >> 3));
+		ret = i2c_smbus_write_byte_data(client,
+						msb_reg, thresh >> 3);
+		if (ret < 0)
+			return ret;
+		return i2c_smbus_write_byte_data(client,
+						 lsb_reg, thresh << 5);
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info nct7718_info = {
+	.read_raw = nct7718_read_raw,
+	.read_event_config = nct7718_read_event_config,
+	.write_event_config = nct7718_write_event_config,
+	.read_event_value = nct7718_read_thresh,
+	.write_event_value = nct7718_write_thresh,
+};
+
+static irqreturn_t nct7718_alert_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct nct7718_data *data = iio_priv(indio_dev);
+	int ret;
+
+	guard(mutex)(&data->lock);
+	ret = i2c_smbus_read_byte_data(data->client,
+				       NCT7718_ALERTSTATUS_REG);
+	if (ret < 0)
+		return IRQ_NONE;
+
+	if ((ret & NCT7718_STS_LTHA) &&
+	    !(data->status_mask & NCT7718_MSK_LTH)) {
+		iio_push_event(indio_dev,
+			       IIO_MOD_EVENT_CODE(IIO_TEMP, 0,
+						  IIO_MOD_TEMP_AMBIENT,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_RISING),
+			       iio_get_time_ns(indio_dev));
+		data->status_mask |= NCT7718_MSK_LTH;
+	}
+	if ((ret & NCT7718_STS_RT1HA) &&
+	    !(data->status_mask & NCT7718_MSK_RT1H)) {
+		iio_push_event(indio_dev,
+			       IIO_MOD_EVENT_CODE(IIO_TEMP, 0,
+						  IIO_MOD_TEMP_OBJECT,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_RISING),
+			       iio_get_time_ns(indio_dev));
+		data->status_mask |= NCT7718_MSK_RT1H;
+	}
+	if ((ret & NCT7718_STS_RT1LA) &&
+	    !(data->status_mask & NCT7718_MSK_RT1L)) {
+		iio_push_event(indio_dev,
+			       IIO_MOD_EVENT_CODE(IIO_TEMP, 0,
+						  IIO_MOD_TEMP_OBJECT,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_FALLING),
+			       iio_get_time_ns(indio_dev));
+		data->status_mask |= NCT7718_MSK_RT1L;
+	}
+
+	i2c_smbus_write_byte_data(data->client,
+				  NCT7718_ALERTMASK_REG,
+				  data->status_mask);
+
+	return IRQ_HANDLED;
+}
+
+static bool nct7718_check_id(struct i2c_client *client)
+{
+	int chip_id, vendor_id, device_id;
+
+	chip_id = i2c_smbus_read_byte_data(client, NCT7718_VID_REG);
+	if (chip_id < 0)
+		return false;
+
+	vendor_id = i2c_smbus_read_byte_data(client, NCT7718_VID_REG);
+	if (vendor_id < 0)
+		return false;
+
+	device_id = i2c_smbus_read_byte_data(client, NCT7718_DID_REG);
+	if (device_id < 0)
+		return false;
+
+	return (chip_id == NCT7718_CHIP_ID &&
+		vendor_id == NCT7718_VENDOR_ID &&
+		device_id == NCT7718_DEVICE_ID);
+}
+
+static int nct7718_chip_config(struct nct7718_data *data)
+{
+	int ret;
+
+	/* Enable MSK_LTH, MSK_RT1H, and MSK_RT1L to monitor alarm */
+	ret = i2c_smbus_read_byte_data(data->client,
+				       NCT7718_ALERTMASK_REG);
+	if (ret < 0)
+		return ret;
+	data->status_mask = ret;
+	data->status_mask &= ~(NCT7718_MSK_LTH	|
+			       NCT7718_MSK_RT1H	|
+			       NCT7718_MSK_RT1L);
+
+	ret = i2c_smbus_write_byte_data(data->client,
+					NCT7718_ALERTMASK_REG,
+					data->status_mask);
+	if (ret < 0)
+		return ret;
+
+	/* Config ALERT Mode Setting to comparator mode */
+	return i2c_smbus_write_byte_data(data->client,
+					 NCT7718_ALERTMODE_REG,
+					 NCT7718_MOD_COMP);
+}
+
+static int nct7718_probe(struct i2c_client *client)
+{
+	struct nct7718_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	if (!nct7718_check_id(client)) {
+		dev_err(&client->dev, "No NCT7718 device\n");
+		return -ENODEV;
+	}
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	mutex_init(&data->lock);
+
+	indio_dev->name = client->name;
+	indio_dev->channels = nct7718_channels;
+	indio_dev->num_channels = ARRAY_SIZE(nct7718_channels);
+	indio_dev->info = &nct7718_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = nct7718_chip_config(data);
+	if (ret)
+		return ret;
+
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev,
+						client->irq,
+						NULL,
+						nct7718_alert_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"nct7718", indio_dev);
+		if (ret) {
+			dev_err(&client->dev, "Failed to request irq!\n");
+			return ret;
+		}
+	}
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct of_device_id nct7718_of_match[] = {
+	{ .compatible = "nuvoton,nct7718" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, nct7718_of_match);
+
+static const struct i2c_device_id nct7718_id[] = {
+	{ "nct7718" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, nct7718_id);
+
+static struct i2c_driver nct7718_driver = {
+	.driver = {
+		.name	= "nct7718",
+		.of_match_table = nct7718_of_match,
+	},
+	.probe		= nct7718_probe,
+	.id_table	= nct7718_id,
+};
+module_i2c_driver(nct7718_driver);
+
+MODULE_DESCRIPTION("Thermal sensor driver for NCT7718W");
+MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v1 2/2] iio: temperature: Add Nuvoton NCT7718W support

[Jonathan Cameron]

On Tue, 26 Nov 2024 15:40:05 +0800
Ming Yu <a0282524688@gmail.com> wrote:

> This patch adds support for the Nuvoton NCT7718W temperature sensor.
> 
Hi Ming, I'll give this a quick look only as I suspect you will end
up moving over to hwmon.

Thanks,

Jonathan

> Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
...

> diff --git a/drivers/iio/temperature/nct7718.c b/drivers/iio/temperature/nct7718.c
> new file mode 100644
> index 000000000000..60624b3de629
> --- /dev/null
> +++ b/drivers/iio/temperature/nct7718.c
> @@ -0,0 +1,505 @@

> +struct nct7718_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
Locks need a comment to say what data they are protecting.

> +	u16 status_mask;
> +};

> +static int nct7718_write_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir,
> +				      int state)
> +{
> +	struct nct7718_data *data = iio_priv(indio_dev);
> +	unsigned int status_mask;
> +	int ret;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_TEMP_AMBIENT:
> +		if (dir == IIO_EV_DIR_RISING)
> +			status_mask = NCT7718_MSK_LTH;
> +		break;
> +	case IIO_MOD_TEMP_OBJECT:
> +		if (dir == IIO_EV_DIR_RISING)
> +			status_mask = NCT7718_MSK_RT1H;
> +		else
> +			status_mask = NCT7718_MSK_RT1L;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_read_byte_data(data->client, NCT7718_ALERTMASK_REG);
> +	mutex_unlock(&data->lock);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (state)
> +		ret &= ~status_mask;
> +	else
> +		ret |=  status_mask;
I would not bother with this sort of alignment. It tends to be fragile longer
term as code gets modified and doesn't make much difference to readablility.

> +
> +	return i2c_smbus_write_byte_data(data->client, NCT7718_ALERTMASK_REG,
> +					 data->status_mask = ret);
> +}

> +
> +static int nct7718_write_thresh(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				enum iio_event_info info,
> +				int val, int val2)
> +{
> +	struct nct7718_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	u8 msb_reg, lsb_reg;
> +	s16 thresh;
> +	int ret, s_val;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_TEMP_AMBIENT:
> +		val = clamp_val(val, NCT7718_LOCAL_TEMP_MIN,
> +				NCT7718_LOCAL_TEMP_MAX);
> +
> +		if (dir == IIO_EV_DIR_RISING) {
> +			return i2c_smbus_write_byte_data(client,
> +							 NCT7718_LT_HALERT_REG,
> +							 val);
> +		}
> +		break;
> +	case IIO_MOD_TEMP_OBJECT:
> +		s_val = (val < 0) ? ((val * 1000000) - val2) :
> +				    ((val * 1000000) + val2);
> +
> +		s_val = clamp_val(s_val, NCT7718_REMOTE_TEMP_MIN_MICRO,
> +				  NCT7718_REMOTE_TEMP_MAX_MICRO);
> +
> +		if (dir == IIO_EV_DIR_RISING) {
> +			msb_reg = NCT7718_RT1_HALERT_MSB_REG;
> +			lsb_reg = NCT7718_RT1_HALERT_LSB_REG;
> +		} else {
> +			msb_reg = NCT7718_RT1_LALERT_MSB_REG;
> +			lsb_reg = NCT7718_RT1_LALERT_LSB_REG;
> +		}
> +
> +		thresh = (s16)(s_val / (1000000 >> 3));
> +		ret = i2c_smbus_write_byte_data(client,
> +						msb_reg, thresh >> 3);
> +		if (ret < 0)
> +			return ret;
> +		return i2c_smbus_write_byte_data(client,
> +						 lsb_reg, thresh << 5);
> +	default:
> +		break;
return -EINVAL; and drop return below.

> +	}
> +
> +	return -EINVAL;
> +}
>
> +
> +static bool nct7718_check_id(struct i2c_client *client)
> +{
> +	int chip_id, vendor_id, device_id;
> +
> +	chip_id = i2c_smbus_read_byte_data(client, NCT7718_VID_REG);
> +	if (chip_id < 0)
> +		return false;
> +
> +	vendor_id = i2c_smbus_read_byte_data(client, NCT7718_VID_REG);
> +	if (vendor_id < 0)
> +		return false;
> +
> +	device_id = i2c_smbus_read_byte_data(client, NCT7718_DID_REG);
> +	if (device_id < 0)
> +		return false;
> +
> +	return (chip_id == NCT7718_CHIP_ID &&
> +		vendor_id == NCT7718_VENDOR_ID &&
> +		device_id == NCT7718_DEVICE_ID);
As below. Don't treat this failing as an error.   It is an error if
you can't read anything however.

> +}
> +
> +static int nct7718_chip_config(struct nct7718_data *data)
> +{
> +	int ret;
> +
> +	/* Enable MSK_LTH, MSK_RT1H, and MSK_RT1L to monitor alarm */
Code makes this fairly obvoius.

> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       NCT7718_ALERTMASK_REG);
> +	if (ret < 0)
> +		return ret;
> +	data->status_mask = ret;
> +	data->status_mask &= ~(NCT7718_MSK_LTH	|
> +			       NCT7718_MSK_RT1H	|
> +			       NCT7718_MSK_RT1L);
> +
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					NCT7718_ALERTMASK_REG,
> +					data->status_mask);

Perhaps consider regmap for it's richer set of functionality.

> +	if (ret < 0)
> +		return ret;
> +
> +	/* Config ALERT Mode Setting to comparator mode */

Ideally (like here) the code should be self explanatory so you don't
need comments.  When that is the case the comment is both unnecessary
and a source of possible future confusion if the code changes and we
fail to keep the comment in sync.

> +	return i2c_smbus_write_byte_data(data->client,
> +					 NCT7718_ALERTMODE_REG,
> +					 NCT7718_MOD_COMP);
> +}
> +
> +static int nct7718_probe(struct i2c_client *client)
> +{
> +	struct nct7718_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	if (!nct7718_check_id(client)) {
> +		dev_err(&client->dev, "No NCT7718 device\n");

If you get an ID missmatch, it is fine to print a message, but carry on anyway.
This is necessary because DT has fallback compatibles to allow for newer devices
working with older drivers.  That only works if we believe the firmware and
ignore a mismatched ID.

> +		return -ENODEV;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	mutex_init(&data->lock);

For new code prefer
	ret = devm_mutex_init()
	if (ret)
		return ret;

> +
> +	indio_dev->name = client->name;

client->name doesn't always end up as the part number which is what
we need here.  Just put "nct7718" in here directly.

Some drivers do this wrong and we can't fix them without breaking 
userspace, but we don't want to introduce more.

> +	indio_dev->channels = nct7718_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(nct7718_channels);
> +	indio_dev->info = &nct7718_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = nct7718_chip_config(data);
> +	if (ret)
> +		return ret;
> +
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev,
> +						client->irq,
> +						NULL,
> +						nct7718_alert_handler,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"nct7718", indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev, "Failed to request irq!\n");
> +			return ret;
> +		}
> +	}
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}

Re: [PATCH v1 2/2] iio: temperature: Add Nuvoton NCT7718W support

[Ming Yu]

Dear Jonathan,

Thank you for your comments,
I will move the driver to HWMON.

Best regards,
Ming

Jonathan Cameron <jic23@kernel.org> 於 2024年12月1日 週日 上午4:39寫道：
>
> On Tue, 26 Nov 2024 15:40:05 +0800
> Ming Yu <a0282524688@gmail.com> wrote:
>
> > This patch adds support for the Nuvoton NCT7718W temperature sensor.
> >
> Hi Ming, I'll give this a quick look only as I suspect you will end
> up moving over to hwmon.
>
> Thanks,
>
> Jonathan
>
> > Signed-off-by: Ming Yu <tmyu0@nuvoton.com>
> ...
>
> > diff --git a/drivers/iio/temperature/nct7718.c b/drivers/iio/temperature/nct7718.c
> > new file mode 100644
> > index 000000000000..60624b3de629
> > --- /dev/null
> > +++ b/drivers/iio/temperature/nct7718.c
> > @@ -0,0 +1,505 @@
>
> > +struct nct7718_data {
> > +     struct i2c_client *client;
> > +     struct mutex lock;
> Locks need a comment to say what data they are protecting.
>
> > +     u16 status_mask;
> > +};
>
> > +static int nct7718_write_event_config(struct iio_dev *indio_dev,
> > +                                   const struct iio_chan_spec *chan,
> > +                                   enum iio_event_type type,
> > +                                   enum iio_event_direction dir,
> > +                                   int state)
> > +{
> > +     struct nct7718_data *data = iio_priv(indio_dev);
> > +     unsigned int status_mask;
> > +     int ret;
> > +
> > +     switch (chan->channel2) {
> > +     case IIO_MOD_TEMP_AMBIENT:
> > +             if (dir == IIO_EV_DIR_RISING)
> > +                     status_mask = NCT7718_MSK_LTH;
> > +             break;
> > +     case IIO_MOD_TEMP_OBJECT:
> > +             if (dir == IIO_EV_DIR_RISING)
> > +                     status_mask = NCT7718_MSK_RT1H;
> > +             else
> > +                     status_mask = NCT7718_MSK_RT1L;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     mutex_lock(&data->lock);
> > +     ret = i2c_smbus_read_byte_data(data->client, NCT7718_ALERTMASK_REG);
> > +     mutex_unlock(&data->lock);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (state)
> > +             ret &= ~status_mask;
> > +     else
> > +             ret |=  status_mask;
> I would not bother with this sort of alignment. It tends to be fragile longer
> term as code gets modified and doesn't make much difference to readablility.
>
> > +
> > +     return i2c_smbus_write_byte_data(data->client, NCT7718_ALERTMASK_REG,
> > +                                      data->status_mask = ret);
> > +}
>
> > +
> > +static int nct7718_write_thresh(struct iio_dev *indio_dev,
> > +                             const struct iio_chan_spec *chan,
> > +                             enum iio_event_type type,
> > +                             enum iio_event_direction dir,
> > +                             enum iio_event_info info,
> > +                             int val, int val2)
> > +{
> > +     struct nct7718_data *data = iio_priv(indio_dev);
> > +     struct i2c_client *client = data->client;
> > +     u8 msb_reg, lsb_reg;
> > +     s16 thresh;
> > +     int ret, s_val;
> > +
> > +     switch (chan->channel2) {
> > +     case IIO_MOD_TEMP_AMBIENT:
> > +             val = clamp_val(val, NCT7718_LOCAL_TEMP_MIN,
> > +                             NCT7718_LOCAL_TEMP_MAX);
> > +
> > +             if (dir == IIO_EV_DIR_RISING) {
> > +                     return i2c_smbus_write_byte_data(client,
> > +                                                      NCT7718_LT_HALERT_REG,
> > +                                                      val);
> > +             }
> > +             break;
> > +     case IIO_MOD_TEMP_OBJECT:
> > +             s_val = (val < 0) ? ((val * 1000000) - val2) :
> > +                                 ((val * 1000000) + val2);
> > +
> > +             s_val = clamp_val(s_val, NCT7718_REMOTE_TEMP_MIN_MICRO,
> > +                               NCT7718_REMOTE_TEMP_MAX_MICRO);
> > +
> > +             if (dir == IIO_EV_DIR_RISING) {
> > +                     msb_reg = NCT7718_RT1_HALERT_MSB_REG;
> > +                     lsb_reg = NCT7718_RT1_HALERT_LSB_REG;
> > +             } else {
> > +                     msb_reg = NCT7718_RT1_LALERT_MSB_REG;
> > +                     lsb_reg = NCT7718_RT1_LALERT_LSB_REG;
> > +             }
> > +
> > +             thresh = (s16)(s_val / (1000000 >> 3));
> > +             ret = i2c_smbus_write_byte_data(client,
> > +                                             msb_reg, thresh >> 3);
> > +             if (ret < 0)
> > +                     return ret;
> > +             return i2c_smbus_write_byte_data(client,
> > +                                              lsb_reg, thresh << 5);
> > +     default:
> > +             break;
> return -EINVAL; and drop return below.
>
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> >
> > +
> > +static bool nct7718_check_id(struct i2c_client *client)
> > +{
> > +     int chip_id, vendor_id, device_id;
> > +
> > +     chip_id = i2c_smbus_read_byte_data(client, NCT7718_VID_REG);
> > +     if (chip_id < 0)
> > +             return false;
> > +
> > +     vendor_id = i2c_smbus_read_byte_data(client, NCT7718_VID_REG);
> > +     if (vendor_id < 0)
> > +             return false;
> > +
> > +     device_id = i2c_smbus_read_byte_data(client, NCT7718_DID_REG);
> > +     if (device_id < 0)
> > +             return false;
> > +
> > +     return (chip_id == NCT7718_CHIP_ID &&
> > +             vendor_id == NCT7718_VENDOR_ID &&
> > +             device_id == NCT7718_DEVICE_ID);
> As below. Don't treat this failing as an error.   It is an error if
> you can't read anything however.
>
> > +}
> > +
> > +static int nct7718_chip_config(struct nct7718_data *data)
> > +{
> > +     int ret;
> > +
> > +     /* Enable MSK_LTH, MSK_RT1H, and MSK_RT1L to monitor alarm */
> Code makes this fairly obvoius.
>
> > +     ret = i2c_smbus_read_byte_data(data->client,
> > +                                    NCT7718_ALERTMASK_REG);
> > +     if (ret < 0)
> > +             return ret;
> > +     data->status_mask = ret;
> > +     data->status_mask &= ~(NCT7718_MSK_LTH  |
> > +                            NCT7718_MSK_RT1H |
> > +                            NCT7718_MSK_RT1L);
> > +
> > +     ret = i2c_smbus_write_byte_data(data->client,
> > +                                     NCT7718_ALERTMASK_REG,
> > +                                     data->status_mask);
>
> Perhaps consider regmap for it's richer set of functionality.
>
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Config ALERT Mode Setting to comparator mode */
>
> Ideally (like here) the code should be self explanatory so you don't
> need comments.  When that is the case the comment is both unnecessary
> and a source of possible future confusion if the code changes and we
> fail to keep the comment in sync.
>
> > +     return i2c_smbus_write_byte_data(data->client,
> > +                                      NCT7718_ALERTMODE_REG,
> > +                                      NCT7718_MOD_COMP);
> > +}
> > +
> > +static int nct7718_probe(struct i2c_client *client)
> > +{
> > +     struct nct7718_data *data;
> > +     struct iio_dev *indio_dev;
> > +     int ret;
> > +
> > +     if (!nct7718_check_id(client)) {
> > +             dev_err(&client->dev, "No NCT7718 device\n");
>
> If you get an ID missmatch, it is fine to print a message, but carry on anyway.
> This is necessary because DT has fallback compatibles to allow for newer devices
> working with older drivers.  That only works if we believe the firmware and
> ignore a mismatched ID.
>
> > +             return -ENODEV;
> > +     }
> > +
> > +     indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     data = iio_priv(indio_dev);
> > +     data->client = client;
> > +     mutex_init(&data->lock);
>
> For new code prefer
>         ret = devm_mutex_init()
>         if (ret)
>                 return ret;
>
> > +
> > +     indio_dev->name = client->name;
>
> client->name doesn't always end up as the part number which is what
> we need here.  Just put "nct7718" in here directly.
>
> Some drivers do this wrong and we can't fix them without breaking
> userspace, but we don't want to introduce more.
>
> > +     indio_dev->channels = nct7718_channels;
> > +     indio_dev->num_channels = ARRAY_SIZE(nct7718_channels);
> > +     indio_dev->info = &nct7718_info;
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +     ret = nct7718_chip_config(data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (client->irq) {
> > +             ret = devm_request_threaded_irq(&client->dev,
> > +                                             client->irq,
> > +                                             NULL,
> > +                                             nct7718_alert_handler,
> > +                                             IRQF_TRIGGER_FALLING |
> > +                                             IRQF_ONESHOT,
> > +                                             "nct7718", indio_dev);
> > +             if (ret) {
> > +                     dev_err(&client->dev, "Failed to request irq!\n");
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     return devm_iio_device_register(&client->dev, indio_dev);
> > +}

Re: [PATCH v1 0/2] Add Nuvoton NCT7718W IIO driver

[Jonathan Cameron]

On Tue, 26 Nov 2024 15:40:03 +0800
Ming Yu <a0282524688@gmail.com> wrote:

> NCT7718W is an I2C based thermal sensor chip from Nuvoton.
Hi Ming Yu,

+CC Jean and Guenter,

Why an IIO driver rather than a HWMON one?  Superficially this looks like a hwmon
chip.  We do have the means to put a generic driver in IIO and bridge to hwmon, but
when a device is very much intended for monitoring of hardware temperatures etc
the IIO driver rarely has any purpose and a simpler hwmon only solution makes sense.

For temperature sensors IIO normally makes sense if:
1) They are part of a series of devices some of which have more functionality than temp
2) Fast devices where hwmon sysfs interfaces become a bottleneck - note you have to have
a usecase for reading them fast, not simply a device that is capable of it.
3) 'Unusual' temperature sensors such as infrared thermometers or very high precision
   thermocouple interfaces.

Any of those apply here?

Note that hwmon has better threshold and critical temperature handling than we can do
in IIO and it seems your part has those as well.

Thanks,

Jonathan


> 
> Ming Yu (2):
>   dt-bindings: iio: temperature: Add support for NCT7718W
>   iio: temperature: Add Nuvoton NCT7718W support
> 
>  .../iio/temperature/nuvoton,nct7718.yaml      |  44 ++
>  MAINTAINERS                                   |   7 +
>  drivers/iio/temperature/Kconfig               |  10 +
>  drivers/iio/temperature/Makefile              |   1 +
>  drivers/iio/temperature/nct7718.c             | 505 ++++++++++++++++++
>  5 files changed, 567 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
>  create mode 100644 drivers/iio/temperature/nct7718.c
> 

Re: [PATCH v1 0/2] Add Nuvoton NCT7718W IIO driver

[Guenter Roeck]

On 11/30/24 12:28, Jonathan Cameron wrote:
> On Tue, 26 Nov 2024 15:40:03 +0800
> Ming Yu <a0282524688@gmail.com> wrote:
> 
>> NCT7718W is an I2C based thermal sensor chip from Nuvoton.
> Hi Ming Yu,
> 
> +CC Jean and Guenter,
> 
> Why an IIO driver rather than a HWMON one?  Superficially this looks like a hwmon
> chip.  We do have the means to put a generic driver in IIO and bridge to hwmon, but
> when a device is very much intended for monitoring of hardware temperatures etc
> the IIO driver rarely has any purpose and a simpler hwmon only solution makes sense.
> 
> For temperature sensors IIO normally makes sense if:
> 1) They are part of a series of devices some of which have more functionality than temp
> 2) Fast devices where hwmon sysfs interfaces become a bottleneck - note you have to have
> a usecase for reading them fast, not simply a device that is capable of it.
> 3) 'Unusual' temperature sensors such as infrared thermometers or very high precision
>     thermocouple interfaces.
> 
> Any of those apply here?
> 

Also, it looks like this chip is compatible to LM90. It isn't entirely clear to me
why this would require a new driver.

Guenter

> Note that hwmon has better threshold and critical temperature handling than we can do
> in IIO and it seems your part has those as well.
> 
> Thanks,
> 
> Jonathan
> 
> 
>>
>> Ming Yu (2):
>>    dt-bindings: iio: temperature: Add support for NCT7718W
>>    iio: temperature: Add Nuvoton NCT7718W support
>>
>>   .../iio/temperature/nuvoton,nct7718.yaml      |  44 ++
>>   MAINTAINERS                                   |   7 +
>>   drivers/iio/temperature/Kconfig               |  10 +
>>   drivers/iio/temperature/Makefile              |   1 +
>>   drivers/iio/temperature/nct7718.c             | 505 ++++++++++++++++++
>>   5 files changed, 567 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
>>   create mode 100644 drivers/iio/temperature/nct7718.c
>>
> 

Re: [PATCH v1 0/2] Add Nuvoton NCT7718W IIO driver

[Ming Yu]

Dear Guenter,

Thank you for your reply. I will add a patch to the LM90 driver to
support NCT7718W.

Best regards,
Ming

Guenter Roeck <linux@roeck-us.net> 於 2024年12月1日 週日 上午4:50寫道：
>
> On 11/30/24 12:28, Jonathan Cameron wrote:
> > On Tue, 26 Nov 2024 15:40:03 +0800
> > Ming Yu <a0282524688@gmail.com> wrote:
> >
> >> NCT7718W is an I2C based thermal sensor chip from Nuvoton.
> > Hi Ming Yu,
> >
> > +CC Jean and Guenter,
> >
> > Why an IIO driver rather than a HWMON one?  Superficially this looks like a hwmon
> > chip.  We do have the means to put a generic driver in IIO and bridge to hwmon, but
> > when a device is very much intended for monitoring of hardware temperatures etc
> > the IIO driver rarely has any purpose and a simpler hwmon only solution makes sense.
> >
> > For temperature sensors IIO normally makes sense if:
> > 1) They are part of a series of devices some of which have more functionality than temp
> > 2) Fast devices where hwmon sysfs interfaces become a bottleneck - note you have to have
> > a usecase for reading them fast, not simply a device that is capable of it.
> > 3) 'Unusual' temperature sensors such as infrared thermometers or very high precision
> >     thermocouple interfaces.
> >
> > Any of those apply here?
> >
>
> Also, it looks like this chip is compatible to LM90. It isn't entirely clear to me
> why this would require a new driver.
>
> Guenter
>
> > Note that hwmon has better threshold and critical temperature handling than we can do
> > in IIO and it seems your part has those as well.
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> >>
> >> Ming Yu (2):
> >>    dt-bindings: iio: temperature: Add support for NCT7718W
> >>    iio: temperature: Add Nuvoton NCT7718W support
> >>
> >>   .../iio/temperature/nuvoton,nct7718.yaml      |  44 ++
> >>   MAINTAINERS                                   |   7 +
> >>   drivers/iio/temperature/Kconfig               |  10 +
> >>   drivers/iio/temperature/Makefile              |   1 +
> >>   drivers/iio/temperature/nct7718.c             | 505 ++++++++++++++++++
> >>   5 files changed, 567 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
> >>   create mode 100644 drivers/iio/temperature/nct7718.c
> >>
> >
>

Re: [PATCH v1 0/2] Add Nuvoton NCT7718W IIO driver

[Ming Yu]

Dear Jonathan,

Thank you for your reply, I'll move the driver to HWMON.
Additionally, for conventional ADC, Thermal sensor, and tachometer
like chips, would it be more appropriate to implement them in HWMON?

Best regards
Ming


Jonathan Cameron <jic23@kernel.org> 於 2024年12月1日 週日 上午4:28寫道：
>
> On Tue, 26 Nov 2024 15:40:03 +0800
> Ming Yu <a0282524688@gmail.com> wrote:
>
> > NCT7718W is an I2C based thermal sensor chip from Nuvoton.
> Hi Ming Yu,
>
> +CC Jean and Guenter,
>
> Why an IIO driver rather than a HWMON one?  Superficially this looks like a hwmon
> chip.  We do have the means to put a generic driver in IIO and bridge to hwmon, but
> when a device is very much intended for monitoring of hardware temperatures etc
> the IIO driver rarely has any purpose and a simpler hwmon only solution makes sense.
>
> For temperature sensors IIO normally makes sense if:
> 1) They are part of a series of devices some of which have more functionality than temp
> 2) Fast devices where hwmon sysfs interfaces become a bottleneck - note you have to have
> a usecase for reading them fast, not simply a device that is capable of it.
> 3) 'Unusual' temperature sensors such as infrared thermometers or very high precision
>    thermocouple interfaces.
>
> Any of those apply here?
>
> Note that hwmon has better threshold and critical temperature handling than we can do
> in IIO and it seems your part has those as well.
>
> Thanks,
>
> Jonathan
>
>
> >
> > Ming Yu (2):
> >   dt-bindings: iio: temperature: Add support for NCT7718W
> >   iio: temperature: Add Nuvoton NCT7718W support
> >
> >  .../iio/temperature/nuvoton,nct7718.yaml      |  44 ++
> >  MAINTAINERS                                   |   7 +
> >  drivers/iio/temperature/Kconfig               |  10 +
> >  drivers/iio/temperature/Makefile              |   1 +
> >  drivers/iio/temperature/nct7718.c             | 505 ++++++++++++++++++
> >  5 files changed, 567 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/temperature/nuvoton,nct7718.yaml
> >  create mode 100644 drivers/iio/temperature/nct7718.c
> >
>

Re: [PATCH v1 0/2] Add Nuvoton NCT7718W IIO driver

[Guenter Roeck]

On Mon, Dec 02, 2024 at 05:22:52PM +0800, Ming Yu wrote:
> Dear Jonathan,
> 
> Thank you for your reply, I'll move the driver to HWMON.

AFAICS the chip is compatible to lm90, so please add support for
it to the lm90 driver. Note that this also applies to NCT7717, which
looks like an internal-sensor-only version of the same chip.

> Additionally, for conventional ADC, Thermal sensor, and tachometer
> like chips, would it be more appropriate to implement them in HWMON?
> 

Thermal sensor and tachometer, yes. For ADC it depends on the use case.
If the ADC is used for hardware monitoring (which is typically the case
if it has limit and alert support), yes. If it is a pure ADC, IIO
is a better place.

Thanks,
Guenter