linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support
@ 2025-08-27 10:31 Lakshay Piplani
  2025-08-27 10:31 ` [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor Lakshay Piplani
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Lakshay Piplani @ 2025-08-27 10:31 UTC (permalink / raw)
  To: linux-kernel, linux-iio, jic23, dlechner, nuno.sa, andy,
	marcelo.schmitt1, gregkh, viro, peterz, jstephan, robh, krzk+dt,
	conor+dt, devicetree, ilpo.jarvinen, jonathan.cameron, akpm, chao,
	jaegeuk
  Cc: vikash.bansal, priyanka.jain, shashank.rebbapragada, Frank.Li,
	Lakshay Piplani

Add bindings for the NXP P3T175x (P3T1750/P3T1755) temperature
sensor, supporting both I2C & I3C interfaces.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
Changes in v2 (addressed review comments from Krzysztof Kozlowski):
 - Dropped nxp,alert-active-high: unnecessary as polarity handling is implicit in driver.
 - Retained nxp,interrupt-mode: required to program TM(thermostat mode) bit; enables interrupt
   (latched) mode. If not present in DT entry comparator mode is set as default.
 - Retained nxp,fault-queue: This needs to be configured during device initialization.
   This property configures the hardware fault queue length. Defines how many consecutive faults
   are required before ALERT/IBI is asserted, preventing false triggers in noisy environments.
 - The `reg` property remains required to satisfy `dt_binding_check`.
 - Fixed YAML formatting, line wrapping, and examples.
 - Changed compatibles from nxp,p3t1755 to nxp,p3t1755-iio and nxp,p3t1750 to nxp,p3t1750-iio
   as reported by kernel test robot.

 .../bindings/iio/temperature/nxp,p3t1755.yaml | 97 +++++++++++++++++++
 1 file changed, 97 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml

diff --git a/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml b/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
new file mode 100644
index 000000000000..4eb6fc5cb247
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
@@ -0,0 +1,97 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/temperature/nxp,p3t1755.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP P3T175x Temperature Sensor
+
+maintainers:
+  - Lakshay Piplani <lakshay.piplani@nxp.com>
+
+description: |
+  Datasheet: https://www.nxp.com/docs/en/data-sheet/P3T1755.pdf
+
+  P3T175x (P3T1750/P3T1755) is a digital temperature sensor with a range of -40°C to
+  +125°C and a 12-bit resolution. It supports communication over
+  both I2C and I3C interfaces.
+
+  The I2C interface supports up to 32 static addresses and provides
+  an ALERT output to signal when temperature thresholds are crossed.
+
+  The I3C interface supports In-Band interrupts (IBI) in interrupt mode,
+  allowing the device to notify the controller of threshold events without
+  dedicated alert pin.
+
+  The device supports configurable thermostat modes (interrupt or comparator),
+  fault queue length etc.
+
+properties:
+  compatible:
+    enum:
+      - nxp,p3t1750-iio
+      - nxp,p3t1755-iio
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+    description: |
+      In I2C mode, the device supports up to 32 static addresses.
+      In I3C mode, the 'reg' property encodes a triplet of
+      <static-address BCR PID> used for device matching.
+      Static address is optional if matching is done via PID.
+
+  nxp,interrupt-mode:
+    type: boolean
+    description: |
+      Enables interrupt mode (TM = 1), where alerts are latched until
+      cleared by a register read.
+      Required for IBI support over I3C. On I2C, both interrupt and
+      comparator mode support events.
+
+  nxp,fault-queue:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 6]
+    description: |
+      Number of consecutive temperature limit
+      violations required before an alert is triggered.
+      valid values:- 1, 2, 4 or 6.
+      If unspecified, hardware default (2) is used.
+
+  assigned-address:
+    true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        temp-sensor@48 {
+            compatible = "nxp,p3t1755-iio";
+            reg = <0x48>;
+            nxp,interrupt-mode;
+            nxp,fault-queue = <6>;
+            interrupt-parent = <&gpio2>;
+            interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
+        };
+    };
+
+  - |
+    i3c {
+      #address-cells = <3>;
+      #size-cells = <0>;
+      temp-sensor@48,236152a00 {
+        reg = <0x48 0x236 0x152a00>;
+        assigned-address = <0x50>;
+      };
+    };
-- 
2.25.1


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

* [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor
  2025-08-27 10:31 [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support Lakshay Piplani
@ 2025-08-27 10:31 ` Lakshay Piplani
  2025-08-27 14:36   ` David Lechner
                     ` (2 more replies)
  2025-08-27 13:59 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support David Lechner
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 10+ messages in thread
From: Lakshay Piplani @ 2025-08-27 10:31 UTC (permalink / raw)
  To: linux-kernel, linux-iio, jic23, dlechner, nuno.sa, andy,
	marcelo.schmitt1, gregkh, viro, peterz, jstephan, robh, krzk+dt,
	conor+dt, devicetree, ilpo.jarvinen, jonathan.cameron, akpm, chao,
	jaegeuk
  Cc: vikash.bansal, priyanka.jain, shashank.rebbapragada, Frank.Li,
	Lakshay Piplani

Add support for the NXP P3T175x (P3T1750/P3T1755) family of temperature
sensor devices. These devices communicates via both I2C or I3C interfaces.

This driver belongs under IIO because:
  The P3T1750/P3T1755 sensors require interrupt or IBI support to handle
  threshold events, which the hwmon subsystem does not provide. In contrast,
  the IIO subsystem offers robust event handling that matches the hardware
  capabilities of these sensors. Therefore, this driver is better suited
  under IIO.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
Changes in v2 (addressed review comments from Jonathan Cameron, Krzysztof Kozlowski, Andy Shevchenko):
 - Added endian-safe handling for register read (__be16 conversion).
 - Replaced manual bit masking with FIELD_GET bit extraction.
 - General cleanups: dropped unused headers, reduced logging to dev_dbg.
 - Dropped sysfs attributes for fault queue length and thermostat mode (comparator or interrupt).
 - Added ABI doc: Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755 describing
   trigger_one_shot attribute.
 - Updated Kconfig to allow building both I2C and I3C drivers simultaneously.
 - I3C: switched to device_property_* from of_property_*.
 - Added devm_add_action_or_reset() for IBI disable/free.
 - Cleaned i3c_ibi_setup init.
 - Channel info structures are now part-specific (p3t1755_channels_info, p3t1750_channels_info), no wildcards.

 .../testing/sysfs-bus-iio-temperature-p3t1755 |  11 +
 drivers/iio/temperature/Kconfig               |   1 +
 drivers/iio/temperature/p3t/Kconfig           |  28 ++
 drivers/iio/temperature/p3t/Makefile          |   5 +
 drivers/iio/temperature/p3t/p3t1755.h         |  56 +++
 drivers/iio/temperature/p3t/p3t1755_core.c    | 456 ++++++++++++++++++
 drivers/iio/temperature/p3t/p3t1755_i2c.c     |  91 ++++
 drivers/iio/temperature/p3t/p3t1755_i3c.c     | 133 +++++
 8 files changed, 781 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755
 create mode 100644 drivers/iio/temperature/p3t/Kconfig
 create mode 100644 drivers/iio/temperature/p3t/Makefile
 create mode 100644 drivers/iio/temperature/p3t/p3t1755.h
 create mode 100644 drivers/iio/temperature/p3t/p3t1755_core.c
 create mode 100644 drivers/iio/temperature/p3t/p3t1755_i2c.c
 create mode 100644 drivers/iio/temperature/p3t/p3t1755_i3c.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755
new file mode 100644
index 000000000000..4ab79e814e6a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755
@@ -0,0 +1,11 @@
+What:		/sys/bus/iio/devices/iio:deviceX/trigger_one_shot
+Date:		August 2025
+KernelVersion:	6.17
+Contact:	Lakshay Piplani <lakshay.piplani@nxp.com>
+Description:
+		(WO) Write-only attribute to trigger a one-shot temperature
+		measurement on the P3T1750/P3T1755 sensor. Writing '1' initiates
+		the measurement if the device is in shutdown mode. Writing '0' or
+		any other value is invalid.
+
+		Returns -EBUSY if the device is not in shutdown mode.
diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 1244d8e17d50..7bfa204ba428 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -181,5 +181,6 @@ config MCP9600
 
 	  This driver can also be built as a module. If so, the module
 	  will be called mcp9600.
+source "drivers/iio/temperature/p3t/Kconfig"
 
 endmenu
diff --git a/drivers/iio/temperature/p3t/Kconfig b/drivers/iio/temperature/p3t/Kconfig
new file mode 100644
index 000000000000..bec2fb09eceb
--- /dev/null
+++ b/drivers/iio/temperature/p3t/Kconfig
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config IIO_P3T1755
+	tristate
+	depends on (I2C || I3C)
+
+config IIO_P3T1755_I2C
+	tristate "NXP P3T1755 temprature sensor I2C driver"
+	select IIO_P3T1755
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for NXP P3T1755 I2C temperature
+	  sensor.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called p3t1755_i2c
+
+config IIO_P3T1755_I3C
+	tristate "NXP P3T1755 temprature sensor I3C driver"
+	select IIO_P3T1755
+	select REGMAP_I3C
+	depends on I3C
+	help
+	  Say yes here to build support for NXP P3T1755 I3C temperature
+	  sensor.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called p3t1755_i3c
diff --git a/drivers/iio/temperature/p3t/Makefile b/drivers/iio/temperature/p3t/Makefile
new file mode 100644
index 000000000000..7d33b507f1f1
--- /dev/null
+++ b/drivers/iio/temperature/p3t/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_IIO_P3T1755) += p3t1755_core.o
+obj-$(CONFIG_IIO_P3T1755_I2C) += p3t1755_i2c.o
+obj-$(CONFIG_IIO_P3T1755_I3C) += p3t1755_i3c.o
diff --git a/drivers/iio/temperature/p3t/p3t1755.h b/drivers/iio/temperature/p3t/p3t1755.h
new file mode 100644
index 000000000000..8e56dd64b813
--- /dev/null
+++ b/drivers/iio/temperature/p3t/p3t1755.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * NXP P3T175x Temperature Sensor Driver
+ *
+ * Copyright 2025 NXP
+ */
+#ifndef P3T1755_H
+#define P3T1755_H
+
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+
+#define P3T1755_REG_TEMP		0x0
+#define P3T1755_REG_CFGR		0x1
+#define P3T1755_REG_LOW_LIM		0x2
+#define P3T1755_REG_HIGH_LIM		0x3
+
+#define P3T1755_SHUTDOWN_BIT		BIT(0)
+#define P3T1755_TM_BIT			BIT(1)
+#define P3T1755_POL_BIT			BIT(2)
+#define P3T1755_ONE_SHOT_BIT		BIT(7)
+
+#define P3T1755_FAULT_QUEUE_SHIFT	3
+#define P3T1755_FAULT_QUEUE_MASK	GENMASK(4, 3)
+
+#define P3T1755_CONVERSION_TIME_BITS	GENMASK(6, 5)
+
+extern const struct p3t1755_info p3t1755_channels_info;
+extern const struct p3t1755_info p3t1750_channels_info;
+
+enum p3t1755_hw_id {
+	P3T1755_ID = 0,
+	P3T1750_ID,
+};
+
+struct p3t1755_info {
+	const char *name;
+	const struct iio_chan_spec *channels;
+	int num_channels;
+};
+
+struct p3t1755_data {
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex lock; /* Protects access to sensor registers */
+	bool tm_mode;
+};
+
+int p3t1755_fault_queue_to_bits(int val);
+int p3t1755_probe(struct device *dev, const struct p3t1755_info *chip,
+		  struct regmap *regmap, bool tm_mode, int fq_bits, int irq);
+int p3t1755_get_temp_and_limits(struct p3t1755_data *data,
+				int *temp_raw, int *thigh_raw, int *tlow_raw);
+void p3t1755_push_thresh_event(struct iio_dev *indio_dev);
+
+#endif /* P3T1755_H */
diff --git a/drivers/iio/temperature/p3t/p3t1755_core.c b/drivers/iio/temperature/p3t/p3t1755_core.c
new file mode 100644
index 000000000000..264bf4abb088
--- /dev/null
+++ b/drivers/iio/temperature/p3t/p3t1755_core.c
@@ -0,0 +1,456 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * NXP P3T175x Temperature Sensor Driver
+ *
+ * Copyright 2025 NXP
+ */
+#include <linux/err.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/module.h>
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/interrupt.h>
+#include <linux/types.h>
+#include <linux/regmap.h>
+#include <linux/device.h>
+#include <linux/iio/events.h>
+
+#include "p3t1755.h"
+
+enum p3t1755_attr_index {
+	P3T1755_ATTR_THERMOSTAT_MODE,
+	P3T1755_ATTR_TRIGGER_ONE_SHOT,
+	P3T1755_ATTR_FAULT_QUEUE_LENGTH,
+};
+
+static const struct {
+	u8 bits;
+	unsigned int freq_hz;
+} p3t1755_samp_freqs[] = {
+	{ 0x00, 36 },
+	{ 0x01, 18 },
+	{ 0x02, 9 },
+	{ 0x03, 4 },
+};
+
+static const int p3t1755_fault_queue_values[] = { 1, 2, 4, 6 };
+
+int p3t1755_fault_queue_to_bits(int val)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(p3t1755_fault_queue_values); i++)
+		if (p3t1755_fault_queue_values[i] == val)
+			return i;
+	return -EINVAL;
+}
+
+int p3t1755_get_temp_and_limits(struct p3t1755_data *data,
+				int *temp_raw, int *thigh_raw, int *tlow_raw)
+{
+	__be16 be;
+	int ret;
+	int raw12;
+
+	ret = regmap_bulk_read(data->regmap, P3T1755_REG_TEMP, &be, sizeof(be));
+	if (ret) {
+		dev_dbg(data->dev, "TEMP read failed: %d\n", ret);
+		return ret;
+	}
+
+	raw12 = sign_extend32(be16_to_cpu(be) >> 4, 11);
+	*temp_raw = raw12;
+
+	ret = regmap_bulk_read(data->regmap, P3T1755_REG_HIGH_LIM, &be, sizeof(be));
+	if (ret) {
+		dev_dbg(data->dev, "HIGH_LIM read failed: %d\n", ret);
+		return ret;
+	}
+
+	raw12 = sign_extend32(be16_to_cpu(be) >> 4, 11);
+	*thigh_raw = raw12;
+
+	ret = regmap_bulk_read(data->regmap, P3T1755_REG_LOW_LIM, &be, sizeof(be));
+	if (ret) {
+		dev_dbg(data->dev, "LOW_LIM read failed: %d\n", ret);
+		return ret;
+	}
+
+	raw12 = sign_extend32(be16_to_cpu(be) >> 4, 11);
+	*tlow_raw = raw12;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(p3t1755_get_temp_and_limits, IIO_P3T1755);
+
+void p3t1755_push_thresh_event(struct iio_dev *indio_dev)
+{
+	struct p3t1755_data *data = iio_priv(indio_dev);
+	int ret, temp, thigh, tlow;
+	unsigned int cfgr;
+
+	/* Read CFGR register to check device mode and implicitly clear the ALERT latch.
+	 * As per Datasheet: "Any register read will clear the interrupt"
+	 */
+	ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
+	if (ret) {
+		dev_err(data->dev, "Failed to read CFGR register: %d\n", ret);
+		return;
+	}
+
+	if (FIELD_GET(P3T1755_SHUTDOWN_BIT, cfgr)) {
+		dev_dbg(data->dev, "Device is in shutdown mode, skipping event push\n");
+		return;
+	}
+
+	ret = p3t1755_get_temp_and_limits(data, &temp, &thigh, &tlow);
+	if (ret) {
+		dev_err(data->dev, "Failed to get temperature and limits: %d\n", ret);
+		return;
+	}
+
+	if (temp >= thigh || temp <= tlow) {
+		dev_dbg(data->dev, "Threshold event: DIR_EITHER (T=%d, TH=%d, TL=%d)\n",
+			temp, thigh, tlow);
+
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_TEMP, 0, IIO_NO_MOD,
+							     IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
+			       iio_get_time_ns(indio_dev));
+	} else {
+		dev_dbg(data->dev, "Temperature within limits: no event triggered (T=%d, TH=%d, TL=%d)\n",
+			temp, thigh, tlow);
+	}
+}
+EXPORT_SYMBOL_NS_GPL(p3t1755_push_thresh_event, IIO_P3T1755);
+
+static int p3t1755_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *channel, int *val,
+			    int *val2, long mask)
+{
+	struct p3t1755_data *data = iio_priv(indio_dev);
+	unsigned int cfgr;
+	__be16 be;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_bulk_read(data->regmap, P3T1755_REG_TEMP, &be, sizeof(be));
+		if (ret < 0) {
+			dev_err(data->dev, "Failed to read temperature register\n");
+			return ret;
+		}
+		*val = sign_extend32(be16_to_cpu(be) >> 4, 11);
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = 625;
+		*val2 = 10000;
+
+		return IIO_VAL_FRACTIONAL;
+
+	case IIO_CHAN_INFO_ENABLE:
+		ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
+		if (ret < 0) {
+			dev_err(data->dev, "Failed to read configuration register\n");
+			return ret;
+		}
+		*val = !FIELD_GET(P3T1755_SHUTDOWN_BIT, cfgr);
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		u8 sel;
+
+		ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
+		if (ret < 0) {
+			dev_err(data->dev, "Failed to read configuration register\n");
+			return ret;
+		}
+
+		sel = FIELD_GET(P3T1755_CONVERSION_TIME_BITS, cfgr);
+		if (sel >= ARRAY_SIZE(p3t1755_samp_freqs))
+			return -EINVAL;
+
+		*val = p3t1755_samp_freqs[sel].freq_hz;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int p3t1755_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct p3t1755_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_ENABLE:
+		ret = regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
+					 P3T1755_SHUTDOWN_BIT,
+					 val == 0 ? P3T1755_SHUTDOWN_BIT : 0);
+		if (ret < 0) {
+			dev_err(data->dev, "Failed to update SHUTDOWN bit\n");
+			return ret;
+		}
+		return 0;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		u32 regbits;
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(p3t1755_samp_freqs); i++) {
+			if (p3t1755_samp_freqs[i].freq_hz == val)
+				break;
+		}
+
+		if (i == ARRAY_SIZE(p3t1755_samp_freqs))
+			return -EINVAL;
+
+		regbits = FIELD_PREP(P3T1755_CONVERSION_TIME_BITS, (u32)i);
+
+		return regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
+					  P3T1755_CONVERSION_TIME_BITS,
+					  regbits);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int p3t1755_read_event_value(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 p3t1755_data *data = iio_priv(indio_dev);
+	unsigned int reg;
+	__be16 be;
+	int ret;
+
+	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
+					   P3T1755_REG_LOW_LIM;
+
+	ret = regmap_bulk_read(data->regmap, reg, &be, sizeof(be));
+	if (ret < 0) {
+		dev_err(data->dev, "Failed to read Thigh or Tlow register\n");
+		return ret;
+	}
+
+	*val = sign_extend32(be16_to_cpu(be) >> 4, 11);
+
+	return IIO_VAL_INT;
+}
+
+static int p3t1755_write_event_value(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 p3t1755_data *data = iio_priv(indio_dev);
+	unsigned int reg;
+	__be16 be;
+
+	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
+					   P3T1755_REG_LOW_LIM;
+
+	if (val < -2048 || val > 2047)
+		return -ERANGE;
+
+	be = cpu_to_be16((u16)((val & 0xfff) << 4));
+
+	return regmap_bulk_write(data->regmap, reg, &be, sizeof(be));
+}
+
+static int p3t1755_trigger_one_shot(struct p3t1755_data *data)
+{
+	unsigned int config;
+	int ret;
+
+	mutex_lock(&data->lock);
+
+	ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &config);
+	if (ret)
+		goto out;
+
+	if (!(config & P3T1755_SHUTDOWN_BIT)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	config |= P3T1755_ONE_SHOT_BIT;
+	ret = regmap_write(data->regmap, P3T1755_REG_CFGR, config);
+
+out:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static ssize_t p3t1755_attr_store(struct device *dev, struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct p3t1755_data *data = iio_priv(indio_dev);
+	int ret;
+	bool enable;
+
+	switch (iattr->address) {
+	case P3T1755_ATTR_TRIGGER_ONE_SHOT:
+		ret = kstrtobool(buf, &enable);
+		if (ret || !enable)
+			return ret ? ret : -EINVAL;
+		ret = p3t1755_trigger_one_shot(data);
+		return ret ?: count;
+
+	default:
+		return -EINVAL;
+		}
+	}
+
+static IIO_DEVICE_ATTR(trigger_one_shot, 0200, NULL, p3t1755_attr_store,
+		       P3T1755_ATTR_TRIGGER_ONE_SHOT);
+
+static const struct iio_event_spec p3t1755_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE)
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE)
+	},
+};
+
+static const struct iio_chan_spec p3t1755_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_ENABLE) |
+				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.event_spec = p3t1755_events,
+		.num_event_specs = ARRAY_SIZE(p3t1755_events),
+	},
+};
+
+const struct p3t1755_info p3t1755_channels_info = {
+	.name = "p3t1755",
+	.channels = p3t1755_channels,
+	.num_channels = ARRAY_SIZE(p3t1755_channels)
+};
+EXPORT_SYMBOL_NS(p3t1755_channels_info, IIO_P3T1755);
+
+const struct p3t1755_info p3t1750_channels_info = {
+	.name = "p3t1750",
+	.channels = p3t1755_channels,
+	.num_channels = ARRAY_SIZE(p3t1755_channels)
+};
+EXPORT_SYMBOL_NS(p3t1750_channels_info, IIO_P3T1755);
+
+static struct attribute *p3t1755_attributes[] = {
+	&iio_dev_attr_trigger_one_shot.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group p3t1755_attr_group = {
+	.attrs = p3t1755_attributes,
+};
+
+static const struct iio_info p3t1755_info = {
+	.read_raw = p3t1755_read_raw,
+	.write_raw = p3t1755_write_raw,
+	.read_event_value = p3t1755_read_event_value,
+	.write_event_value = p3t1755_write_event_value,
+	.attrs = &p3t1755_attr_group,
+};
+
+static irqreturn_t p3t1755_irq_handler(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+
+	dev_dbg(&indio_dev->dev, "IRQ triggered, processing threshold event\n");
+
+	p3t1755_push_thresh_event(indio_dev);
+
+	return IRQ_HANDLED;
+}
+
+int p3t1755_probe(struct device *dev, const struct p3t1755_info *chip,
+		  struct regmap *regmap, bool tm_mode, int fq_bits, int irq)
+{
+	struct p3t1755_data *data;
+	struct iio_dev *iio_dev;
+	unsigned long irq_flags;
+	int ret;
+
+	iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!iio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(iio_dev);
+	data->dev = dev;
+	data->regmap = regmap;
+	data->tm_mode = tm_mode;
+
+	mutex_init(&data->lock);
+
+	iio_dev->name = chip->name;
+	iio_dev->modes = INDIO_DIRECT_MODE;
+	iio_dev->info = &p3t1755_info;
+	iio_dev->channels = chip->channels;
+	iio_dev->num_channels = chip->num_channels;
+
+	dev_set_drvdata(dev, iio_dev);
+
+	ret = regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
+				 P3T1755_TM_BIT,
+				(tm_mode ? P3T1755_TM_BIT : 0));
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Failed to update TM bit\n");
+
+	if (fq_bits >= 0)
+		regmap_update_bits(data->regmap, P3T1755_REG_CFGR, P3T1755_FAULT_QUEUE_MASK,
+				   fq_bits << P3T1755_FAULT_QUEUE_SHIFT);
+
+	ret = devm_iio_device_register(dev, iio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Temperature sensor failed to register\n");
+
+	if (irq > 0) {
+		iio_dev = dev_get_drvdata(dev);
+		data = iio_priv(iio_dev);
+
+		if (tm_mode)
+			irq_flags = IRQF_TRIGGER_FALLING;
+		else
+			irq_flags = (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING);
+
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						p3t1755_irq_handler, irq_flags | IRQF_ONESHOT,
+						"p3t175x", iio_dev);
+		if (ret)
+			dev_err_probe(dev, ret, "Failed to request IRQ: %d\n", ret);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(p3t1755_probe, IIO_P3T1755);
+
+MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
+MODULE_DESCRIPTION("NXP P3T175x Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/temperature/p3t/p3t1755_i2c.c b/drivers/iio/temperature/p3t/p3t1755_i2c.c
new file mode 100644
index 000000000000..7c12f326a859
--- /dev/null
+++ b/drivers/iio/temperature/p3t/p3t1755_i2c.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * NXP P3T175x Temperature Sensor Driver
+ *
+ * Copyright 2025 NXP
+ */
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/events.h>
+
+#include "p3t1755.h"
+
+static const struct regmap_config p3t1755_i2c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct of_device_id p3t1755_i2c_of_match[] = {
+	{ .compatible = "nxp,p3t1755-iio", .data = &p3t1755_channels_info },
+	{ .compatible = "nxp,p3t1750-iio", .data = &p3t1750_channels_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, p3t1755_i2c_of_match);
+
+static const struct i2c_device_id p3t1755_i2c_id_table[] = {
+	{ "p3t1755", (kernel_ulong_t)&p3t1755_channels_info },
+	{ "p3t1750", (kernel_ulong_t)&p3t1750_channels_info},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, p3t1755_i2c_id_table);
+
+static int p3t1755_i2c_probe(struct i2c_client *client)
+{
+	const struct p3t1755_info *chip;
+	struct regmap *regmap;
+	bool tm_mode = false;
+	int fq_bits = -1;
+	int ret;
+	u32 fq;
+
+	regmap = devm_regmap_init_i2c(client, &p3t1755_i2c_regmap_config);
+	if (IS_ERR(regmap)) {
+		return dev_err_probe(&client->dev, PTR_ERR(regmap),
+				     "regmap init failed\n");
+	}
+
+	tm_mode = device_property_read_bool(&client->dev, "nxp,interrupt-mode");
+
+	if (!device_property_read_u32(&client->dev, "nxp,fault-queue", &fq)) {
+		fq_bits = p3t1755_fault_queue_to_bits(fq);
+		if (fq_bits < 0) {
+			return dev_err_probe(&client->dev, fq_bits,
+						     "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
+			}
+	}
+
+	dev_dbg(&client->dev, "Using TM mode: %s\n",
+		tm_mode ? "Interrupt" : "Comparator");
+
+	chip = i2c_get_match_data(client);
+
+	dev_dbg(&client->dev, "Registering p3t175x temperature sensor");
+
+	ret = p3t1755_probe(&client->dev, chip, regmap,
+			    tm_mode, fq_bits, client->irq);
+
+	if (ret) {
+		dev_err_probe(&client->dev, ret, "p3t175x probe failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct i2c_driver p3t1755_driver = {
+	.driver = {
+		.name = "p3t175x_i2c",
+		.of_match_table = p3t1755_i2c_of_match,
+	},
+	.probe = p3t1755_i2c_probe,
+	.id_table = p3t1755_i2c_id_table,
+};
+module_i2c_driver(p3t1755_driver);
+
+MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
+MODULE_DESCRIPTION("NXP P3T175x I2C Driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_P3T1755);
diff --git a/drivers/iio/temperature/p3t/p3t1755_i3c.c b/drivers/iio/temperature/p3t/p3t1755_i3c.c
new file mode 100644
index 000000000000..9f61544b2eb6
--- /dev/null
+++ b/drivers/iio/temperature/p3t/p3t1755_i3c.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * NXP P3T175x Temperature Sensor Driver
+ *
+ * Copyright 2025 NXP
+ */
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
+#include <linux/slab.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/events.h>
+
+#include "p3t1755.h"
+
+static void p3t1755_ibi_handler(struct i3c_device *dev,
+				const struct i3c_ibi_payload *payload)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(&dev->dev);
+
+	p3t1755_push_thresh_event(indio_dev);
+}
+
+/*
+ * Both P3T1755 and P3T1750 share the same I3C PID (0x011B:0x152A),
+ * making runtime differentiation impossible, so using "p3t1755" as
+ * name in sysfs and IIO for I3C based instances.
+ */
+static const struct i3c_device_id p3t1755_i3c_ids[] = {
+	I3C_DEVICE(0x011B, 0x152A, &p3t1755_channels_info),
+	{ },
+};
+
+MODULE_DEVICE_TABLE(i3c, p3t1755_i3c_ids);
+
+static void p3t1755_disable_ibi(void *data)
+{
+	i3c_device_disable_ibi((struct i3c_device *)data);
+}
+
+static void p3t1755_free_ibi(void *data)
+{
+	i3c_device_free_ibi((struct i3c_device *)data);
+}
+
+static int p3t1755_i3c_probe(struct i3c_device *i3cdev)
+{
+	const struct regmap_config p3t1755_i3c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	};
+
+	const struct i3c_device_id *id = i3c_device_match_id(i3cdev, p3t1755_i3c_ids);
+	const struct p3t1755_info *chip;
+	struct device *dev = &i3cdev->dev;
+	struct i3c_ibi_setup ibi_setup;
+	struct regmap *regmap;
+	bool tm_mode = false;
+	int fq_bits = -1;
+	int ret;
+	u32 fq;
+
+	chip = id ? id->data : NULL;
+
+	regmap = devm_regmap_init_i3c(i3cdev, &p3t1755_i3c_regmap_config);
+	if (IS_ERR(regmap)) {
+		return dev_err_probe(&i3cdev->dev, PTR_ERR(regmap),
+				     "Failed to register I3C regmap %ld\n", PTR_ERR(regmap));
+	}
+
+	tm_mode = device_property_read_bool(dev, "nxp,interrupt-mode");
+
+	if (!device_property_read_u32(dev, "nxp,fault-queue", &fq)) {
+		fq_bits = p3t1755_fault_queue_to_bits(fq);
+		if (fq_bits < 0) {
+			return dev_err_probe(&i3cdev->dev, fq_bits,
+					     "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
+		}
+	}
+
+	dev_dbg(&i3cdev->dev, "Using TM mode: %s\n", tm_mode ? "Interrupt" : "Comparator");
+
+	ret = p3t1755_probe(dev, chip, regmap, tm_mode, fq_bits, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "p3t175x probe failed: %d\n", ret);
+
+	if (!tm_mode) {
+		dev_warn(&i3cdev->dev, "IBI not supported in comparator mode, skipping IBI registration\n");
+		return 0;
+	}
+
+	ibi_setup = (struct i3c_ibi_setup) {
+		.handler = p3t1755_ibi_handler,
+		.num_slots = 4,
+		.max_payload_len = 0,
+	};
+
+	ret = i3c_device_request_ibi(i3cdev, &ibi_setup);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request IBI\n");
+
+	ret = i3c_device_enable_ibi(i3cdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable IBI\n");
+
+	ret = devm_add_action_or_reset(dev, p3t1755_disable_ibi, i3cdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register IBI disable action\n");
+
+	ret = devm_add_action_or_reset(dev, p3t1755_free_ibi, i3cdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register IBI free action\n");
+
+	dev_dbg(&i3cdev->dev, "IBI successfully registered\n");
+
+	return 0;
+}
+
+static struct i3c_driver p3t1755_driver = {
+	.driver = {
+		.name = "p3t175x_i3c",
+	},
+	.probe = p3t1755_i3c_probe,
+	.id_table = p3t1755_i3c_ids,
+};
+module_i3c_driver(p3t1755_driver);
+
+MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
+MODULE_DESCRIPTION("NXP P3T175x I3C Driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_P3T1755);
-- 
2.25.1


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

* Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support
  2025-08-27 10:31 [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support Lakshay Piplani
  2025-08-27 10:31 ` [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor Lakshay Piplani
@ 2025-08-27 13:59 ` David Lechner
  2025-08-31 16:01 ` Jonathan Cameron
  2025-08-31 16:47 ` Krzysztof Kozlowski
  3 siblings, 0 replies; 10+ messages in thread
From: David Lechner @ 2025-08-27 13:59 UTC (permalink / raw)
  To: Lakshay Piplani, linux-kernel, linux-iio, jic23, nuno.sa, andy,
	marcelo.schmitt1, gregkh, viro, peterz, jstephan, robh, krzk+dt,
	conor+dt, devicetree, ilpo.jarvinen, jonathan.cameron, akpm, chao,
	jaegeuk
  Cc: vikash.bansal, priyanka.jain, shashank.rebbapragada, Frank.Li

On 8/27/25 5:31 AM, Lakshay Piplani wrote:
> Add bindings for the NXP P3T175x (P3T1750/P3T1755) temperature
> sensor, supporting both I2C & I3C interfaces.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
> ---
> Changes in v2 (addressed review comments from Krzysztof Kozlowski):
>  - Dropped nxp,alert-active-high: unnecessary as polarity handling is implicit in driver.

>  - Retained nxp,interrupt-mode: required to program TM(thermostat mode) bit; enables interrupt
>    (latched) mode. If not present in DT entry comparator mode is set as default.
>  - Retained nxp,fault-queue: This needs to be configured during device initialization.
>    This property configures the hardware fault queue length. Defines how many consecutive faults
>    are required before ALERT/IBI is asserted, preventing false triggers in noisy environments.

These are not very convincing reasons that these to properties should
be in the devicetree. The devicetree describes how things are wired
up, not how they are used in the driver. For the first one, we already
have the parent node to tell us if we are using I2C or I3C, so the
property is redundant. For the second one, the whole system could be
moved from a less noisy to a more noisy environment and we should not
have to change the devicetree to handle that.

>  - The `reg` property remains required to satisfy `dt_binding_check`.
>  - Fixed YAML formatting, line wrapping, and examples.
>  - Changed compatibles from nxp,p3t1755 to nxp,p3t1755-iio and nxp,p3t1750 to nxp,p3t1750-iio
>    as reported by kernel test robot.
> 
>  .../bindings/iio/temperature/nxp,p3t1755.yaml | 97 +++++++++++++++++++
>  1 file changed, 97 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml b/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> new file mode 100644
> index 000000000000..4eb6fc5cb247
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/nxp,p3t1755.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP P3T175x Temperature Sensor
> +
> +maintainers:
> +  - Lakshay Piplani <lakshay.piplani@nxp.com>
> +
> +description: |
> +  Datasheet: https://www.nxp.com/docs/en/data-sheet/P3T1755.pdf
> +
> +  P3T175x (P3T1750/P3T1755) is a digital temperature sensor with a range of -40°C to
> +  +125°C and a 12-bit resolution. It supports communication over
> +  both I2C and I3C interfaces.
> +
> +  The I2C interface supports up to 32 static addresses and provides
> +  an ALERT output to signal when temperature thresholds are crossed.
> +
> +  The I3C interface supports In-Band interrupts (IBI) in interrupt mode,
> +  allowing the device to notify the controller of threshold events without
> +  dedicated alert pin.
> +
> +  The device supports configurable thermostat modes (interrupt or comparator),
> +  fault queue length etc.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,p3t1750-iio
> +      - nxp,p3t1755-iio
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +    description: |
> +      In I2C mode, the device supports up to 32 static addresses.
> +      In I3C mode, the 'reg' property encodes a triplet of
> +      <static-address BCR PID> used for device matching.
> +      Static address is optional if matching is done via PID.
> +
> +  nxp,interrupt-mode:
> +    type: boolean
> +    description: |
> +      Enables interrupt mode (TM = 1), where alerts are latched until
> +      cleared by a register read.

As mentioned above, the driver should know best which mode makes the
most sense without having a property to restrict it.

> +      Required for IBI support over I3C. On I2C, both interrupt and
> +      comparator mode support events.
> +
> +  nxp,fault-queue:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 6]
> +    description: |
> +      Number of consecutive temperature limit
> +      violations required before an alert is triggered.
> +      valid values:- 1, 2, 4 or 6.
> +      If unspecified, hardware default (2) is used.

If we kept this, it should have `default: 2`. But as mentioned above,
this doesn't seem like something that would be known when writing
the device tree since it could depend on variable environmental
conditions.

We already have IIO_EV_INFO_RUNNING_COUNT that sounds similar to this
type of control that would allow it to be set at runtime instead.

> +
> +  assigned-address:
> +    true
> +

Missing `vcc-supply: true`, which should also be required.

> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        temp-sensor@48 {
> +            compatible = "nxp,p3t1755-iio";
> +            reg = <0x48>;
> +            nxp,interrupt-mode;
> +            nxp,fault-queue = <6>;
> +            interrupt-parent = <&gpio2>;
> +            interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> +        };
> +    };
> +
> +  - |
> +    i3c {
> +      #address-cells = <3>;
> +      #size-cells = <0>;
> +      temp-sensor@48,236152a00 {
> +        reg = <0x48 0x236 0x152a00>;
> +        assigned-address = <0x50>;
> +      };
> +    };


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

* Re: [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor
  2025-08-27 10:31 ` [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor Lakshay Piplani
@ 2025-08-27 14:36   ` David Lechner
  2025-08-29 15:43   ` Andy Shevchenko
  2025-08-31 16:46   ` Krzysztof Kozlowski
  2 siblings, 0 replies; 10+ messages in thread
From: David Lechner @ 2025-08-27 14:36 UTC (permalink / raw)
  To: Lakshay Piplani, linux-kernel, linux-iio, jic23, nuno.sa, andy,
	marcelo.schmitt1, gregkh, viro, peterz, jstephan, robh, krzk+dt,
	conor+dt, devicetree, ilpo.jarvinen, jonathan.cameron, akpm, chao,
	jaegeuk
  Cc: vikash.bansal, priyanka.jain, shashank.rebbapragada, Frank.Li

On 8/27/25 5:31 AM, Lakshay Piplani wrote:
> Add support for the NXP P3T175x (P3T1750/P3T1755) family of temperature
> sensor devices. These devices communicates via both I2C or I3C interfaces.
> 
> This driver belongs under IIO because:
>   The P3T1750/P3T1755 sensors require interrupt or IBI support to handle
>   threshold events, which the hwmon subsystem does not provide. In contrast,
>   the IIO subsystem offers robust event handling that matches the hardware
>   capabilities of these sensors. Therefore, this driver is better suited
>   under IIO.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
> ---
> Changes in v2 (addressed review comments from Jonathan Cameron, Krzysztof Kozlowski, Andy Shevchenko):
>  - Added endian-safe handling for register read (__be16 conversion).
>  - Replaced manual bit masking with FIELD_GET bit extraction.
>  - General cleanups: dropped unused headers, reduced logging to dev_dbg.
>  - Dropped sysfs attributes for fault queue length and thermostat mode (comparator or interrupt).
>  - Added ABI doc: Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755 describing
>    trigger_one_shot attribute.
>  - Updated Kconfig to allow building both I2C and I3C drivers simultaneously.
>  - I3C: switched to device_property_* from of_property_*.
>  - Added devm_add_action_or_reset() for IBI disable/free.
>  - Cleaned i3c_ibi_setup init.
>  - Channel info structures are now part-specific (p3t1755_channels_info, p3t1750_channels_info), no wildcards.
> 
>  .../testing/sysfs-bus-iio-temperature-p3t1755 |  11 +
>  drivers/iio/temperature/Kconfig               |   1 +
>  drivers/iio/temperature/p3t/Kconfig           |  28 ++
>  drivers/iio/temperature/p3t/Makefile          |   5 +
>  drivers/iio/temperature/p3t/p3t1755.h         |  56 +++
>  drivers/iio/temperature/p3t/p3t1755_core.c    | 456 ++++++++++++++++++
>  drivers/iio/temperature/p3t/p3t1755_i2c.c     |  91 ++++
>  drivers/iio/temperature/p3t/p3t1755_i3c.c     | 133 +++++
>  8 files changed, 781 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755
>  create mode 100644 drivers/iio/temperature/p3t/Kconfig
>  create mode 100644 drivers/iio/temperature/p3t/Makefile
>  create mode 100644 drivers/iio/temperature/p3t/p3t1755.h
>  create mode 100644 drivers/iio/temperature/p3t/p3t1755_core.c
>  create mode 100644 drivers/iio/temperature/p3t/p3t1755_i2c.c
>  create mode 100644 drivers/iio/temperature/p3t/p3t1755_i3c.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755
> new file mode 100644
> index 000000000000..4ab79e814e6a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755
> @@ -0,0 +1,11 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/trigger_one_shot

What is the use case for this attribute? Normally, a one-shot feature on a
chip like this would just be transparently used when doing a direct read
(reading single sample via in_temp0_raw). We shouldn't need a custom attribute
for it.

If there is a convincing reason we need it, it should be in a separate patch
and the commit message should explain the motivation.

> +Date:		August 2025
> +KernelVersion:	6.17
> +Contact:	Lakshay Piplani <lakshay.piplani@nxp.com>
> +Description:
> +		(WO) Write-only attribute to trigger a one-shot temperature
> +		measurement on the P3T1750/P3T1755 sensor. Writing '1' initiates
> +		the measurement if the device is in shutdown mode. Writing '0' or
> +		any other value is invalid.
> +
> +		Returns -EBUSY if the device is not in shutdown mode.
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 1244d8e17d50..7bfa204ba428 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -181,5 +181,6 @@ config MCP9600
>  
>  	  This driver can also be built as a module. If so, the module
>  	  will be called mcp9600.
> +source "drivers/iio/temperature/p3t/Kconfig"
>  
>  endmenu
> diff --git a/drivers/iio/temperature/p3t/Kconfig b/drivers/iio/temperature/p3t/Kconfig
> new file mode 100644
> index 000000000000..bec2fb09eceb
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/Kconfig
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config IIO_P3T1755
> +	tristate
> +	depends on (I2C || I3C)
> +
> +config IIO_P3T1755_I2C
> +	tristate "NXP P3T1755 temprature sensor I2C driver"

s/temprature/temperature/


> +	select IIO_P3T1755
> +	select REGMAP_I2C

	depends on I2C

> +	help
> +	  Say yes here to build support for NXP P3T1755 I2C temperature
> +	  sensor.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called p3t1755_i2c
> +
> +config IIO_P3T1755_I3C
> +	tristate "NXP P3T1755 temprature sensor I3C driver"

s/temprature/temperature/

> +	select IIO_P3T1755
> +	select REGMAP_I3C
> +	depends on I3C
> +	help
> +	  Say yes here to build support for NXP P3T1755 I3C temperature
> +	  sensor.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called p3t1755_i3c
> diff --git a/drivers/iio/temperature/p3t/Makefile b/drivers/iio/temperature/p3t/Makefile
> new file mode 100644
> index 000000000000..7d33b507f1f1
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_IIO_P3T1755) += p3t1755_core.o
> +obj-$(CONFIG_IIO_P3T1755_I2C) += p3t1755_i2c.o
> +obj-$(CONFIG_IIO_P3T1755_I3C) += p3t1755_i3c.o
> diff --git a/drivers/iio/temperature/p3t/p3t1755.h b/drivers/iio/temperature/p3t/p3t1755.h
> new file mode 100644
> index 000000000000..8e56dd64b813
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * NXP P3T175x Temperature Sensor Driver
> + *
> + * Copyright 2025 NXP
> + */
> +#ifndef P3T1755_H
> +#define P3T1755_H
> +
> +#include <linux/device.h>

Normally, we would just write `struct device;` instead of including
this header.

> +#include <linux/iio/iio.h>

We can do the same for `struct iio_chan_spec;`.

But we should be including linux/types.h for bool and linux/mutex.h
for struct mutex since that one is not just used as a pointer.

> +
> +#define P3T1755_REG_TEMP		0x0
> +#define P3T1755_REG_CFGR		0x1
> +#define P3T1755_REG_LOW_LIM		0x2
> +#define P3T1755_REG_HIGH_LIM		0x3
> +
> +#define P3T1755_SHUTDOWN_BIT		BIT(0)
> +#define P3T1755_TM_BIT			BIT(1)
> +#define P3T1755_POL_BIT			BIT(2)
> +#define P3T1755_ONE_SHOT_BIT		BIT(7)
> +
> +#define P3T1755_FAULT_QUEUE_SHIFT	3

Use FIELD_PREP() with P3T1755_FAULT_QUEUE_MASK instead of
P3T1755_FAULT_QUEUE_SHIFT and remove P3T1755_FAULT_QUEUE_SHIFT.

> +#define P3T1755_FAULT_QUEUE_MASK	GENMASK(4, 3)
> +
> +#define P3T1755_CONVERSION_TIME_BITS	GENMASK(6, 5)
> +
> +extern const struct p3t1755_info p3t1755_channels_info;
> +extern const struct p3t1755_info p3t1750_channels_info;
> +
> +enum p3t1755_hw_id {
> +	P3T1755_ID = 0,
> +	P3T1750_ID,
> +};
> +
> +struct p3t1755_info {
> +	const char *name;
> +	const struct iio_chan_spec *channels;
> +	int num_channels;
> +};
> +
> +struct p3t1755_data {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct mutex lock; /* Protects access to sensor registers */
> +	bool tm_mode;
> +};
> +
> +int p3t1755_fault_queue_to_bits(int val);
> +int p3t1755_probe(struct device *dev, const struct p3t1755_info *chip,
> +		  struct regmap *regmap, bool tm_mode, int fq_bits, int irq);
> +int p3t1755_get_temp_and_limits(struct p3t1755_data *data,
> +				int *temp_raw, int *thigh_raw, int *tlow_raw);
> +void p3t1755_push_thresh_event(struct iio_dev *indio_dev);
> +
> +#endif /* P3T1755_H */
> diff --git a/drivers/iio/temperature/p3t/p3t1755_core.c b/drivers/iio/temperature/p3t/p3t1755_core.c
> new file mode 100644
> index 000000000000..264bf4abb088
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755_core.c
> @@ -0,0 +1,456 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NXP P3T175x Temperature Sensor Driver
> + *
> + * Copyright 2025 NXP
> + */
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/interrupt.h>
> +#include <linux/types.h>
> +#include <linux/regmap.h>
> +#include <linux/device.h>
> +#include <linux/iio/events.h>
> +
> +#include "p3t1755.h"
> +
> +enum p3t1755_attr_index {
> +	P3T1755_ATTR_THERMOSTAT_MODE,
> +	P3T1755_ATTR_TRIGGER_ONE_SHOT,
> +	P3T1755_ATTR_FAULT_QUEUE_LENGTH,

Two of these aren't used in the code.

> +};
> +
> +static const struct {
> +	u8 bits;
> +	unsigned int freq_hz;
> +} p3t1755_samp_freqs[] = {
> +	{ 0x00, 36 },
> +	{ 0x01, 18 },
> +	{ 0x02, 9 },
> +	{ 0x03, 4 },
> +};
> +
> +static const int p3t1755_fault_queue_values[] = { 1, 2, 4, 6 };
> +
> +int p3t1755_fault_queue_to_bits(int val)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(p3t1755_fault_queue_values); i++)
> +		if (p3t1755_fault_queue_values[i] == val)
> +			return i;
> +	return -EINVAL;
> +}
> +
> +int p3t1755_get_temp_and_limits(struct p3t1755_data *data,
> +				int *temp_raw, int *thigh_raw, int *tlow_raw)
> +{
> +	__be16 be;
> +	int ret;
> +	int raw12;
> +
> +	ret = regmap_bulk_read(data->regmap, P3T1755_REG_TEMP, &be, sizeof(be));
> +	if (ret) {
> +		dev_dbg(data->dev, "TEMP read failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	raw12 = sign_extend32(be16_to_cpu(be) >> 4, 11);
> +	*temp_raw = raw12;
> +
> +	ret = regmap_bulk_read(data->regmap, P3T1755_REG_HIGH_LIM, &be, sizeof(be));
> +	if (ret) {
> +		dev_dbg(data->dev, "HIGH_LIM read failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	raw12 = sign_extend32(be16_to_cpu(be) >> 4, 11);> +	*thigh_raw = raw12;
> +
> +	ret = regmap_bulk_read(data->regmap, P3T1755_REG_LOW_LIM, &be, sizeof(be));
> +	if (ret) {
> +		dev_dbg(data->dev, "LOW_LIM read failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	raw12 = sign_extend32(be16_to_cpu(be) >> 4, 11);
> +	*tlow_raw = raw12;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(p3t1755_get_temp_and_limits, IIO_P3T1755);
> +
> +void p3t1755_push_thresh_event(struct iio_dev *indio_dev)
> +{
> +	struct p3t1755_data *data = iio_priv(indio_dev);
> +	int ret, temp, thigh, tlow;
> +	unsigned int cfgr;
> +
> +	/* Read CFGR register to check device mode and implicitly clear the ALERT latch.

IIO subsystem style is:

	/*
	 * Read ...

> +	 * As per Datasheet: "Any register read will clear the interrupt"
> +	 */
> +	ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
> +	if (ret) {
> +		dev_err(data->dev, "Failed to read CFGR register: %d\n", ret);
> +		return;
> +	}
> +
> +	if (FIELD_GET(P3T1755_SHUTDOWN_BIT, cfgr)) {
> +		dev_dbg(data->dev, "Device is in shutdown mode, skipping event push\n");
> +		return;
> +	}
> +
> +	ret = p3t1755_get_temp_and_limits(data, &temp, &thigh, &tlow);
> +	if (ret) {
> +		dev_err(data->dev, "Failed to get temperature and limits: %d\n", ret);
> +		return;
> +	}
> +
> +	if (temp >= thigh || temp <= tlow) {
> +		dev_dbg(data->dev, "Threshold event: DIR_EITHER (T=%d, TH=%d, TL=%d)\n",
> +			temp, thigh, tlow);
> +
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_TEMP, 0, IIO_NO_MOD,
> +							     IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
> +			       iio_get_time_ns(indio_dev));
> +	} else {
> +		dev_dbg(data->dev, "Temperature within limits: no event triggered (T=%d, TH=%d, TL=%d)\n",
> +			temp, thigh, tlow);
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(p3t1755_push_thresh_event, IIO_P3T1755);
> +
> +static int p3t1755_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *channel, int *val,
> +			    int *val2, long mask)
> +{
> +	struct p3t1755_data *data = iio_priv(indio_dev);
> +	unsigned int cfgr;
> +	__be16 be;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(data->regmap, P3T1755_REG_TEMP, &be, sizeof(be));
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to read temperature register\n");

The error goes to userspace, so dev_err() is redundant and should be removed.
Same advice applies elsewhere.

> +			return ret;
> +		}
> +		*val = sign_extend32(be16_to_cpu(be) >> 4, 11);
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 625;
> +		*val2 = 10000;
> +
> +		return IIO_VAL_FRACTIONAL;
> +
> +	case IIO_CHAN_INFO_ENABLE:
> +		ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to read configuration register\n");
> +			return ret;
> +		}
> +		*val = !FIELD_GET(P3T1755_SHUTDOWN_BIT, cfgr);
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		u8 sel;
> +
> +		ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to read configuration register\n");
> +			return ret;
> +		}
> +
> +		sel = FIELD_GET(P3T1755_CONVERSION_TIME_BITS, cfgr);
> +		if (sel >= ARRAY_SIZE(p3t1755_samp_freqs))
> +			return -EINVAL;
> +
> +		*val = p3t1755_samp_freqs[sel].freq_hz;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int p3t1755_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct p3t1755_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_ENABLE:
> +		ret = regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
> +					 P3T1755_SHUTDOWN_BIT,
> +					 val == 0 ? P3T1755_SHUTDOWN_BIT : 0);

Generally, a shutdown mode like this would be controled by power management
rather than through an enable bit. Perhaps something to save for a later
patch.

> +		if (ret < 0) {
> +			dev_err(data->dev, "Failed to update SHUTDOWN bit\n");
> +			return ret;
> +		}
> +		return 0;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		u32 regbits;
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(p3t1755_samp_freqs); i++) {
> +			if (p3t1755_samp_freqs[i].freq_hz == val)
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(p3t1755_samp_freqs))
> +			return -EINVAL;
> +
> +		regbits = FIELD_PREP(P3T1755_CONVERSION_TIME_BITS, (u32)i);
> +
> +		return regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
> +					  P3T1755_CONVERSION_TIME_BITS,
> +					  regbits);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int p3t1755_read_event_value(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 p3t1755_data *data = iio_priv(indio_dev);
> +	unsigned int reg;
> +	__be16 be;
> +	int ret;
> +
> +	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
> +					   P3T1755_REG_LOW_LIM;
> +
> +	ret = regmap_bulk_read(data->regmap, reg, &be, sizeof(be));
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed to read Thigh or Tlow register\n");
> +		return ret;
> +	}
> +
> +	*val = sign_extend32(be16_to_cpu(be) >> 4, 11);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int p3t1755_write_event_value(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 p3t1755_data *data = iio_priv(indio_dev);
> +	unsigned int reg;
> +	__be16 be;
> +
> +	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
> +					   P3T1755_REG_LOW_LIM;
> +
> +	if (val < -2048 || val > 2047)
> +		return -ERANGE;
> +
> +	be = cpu_to_be16((u16)((val & 0xfff) << 4));
> +
> +	return regmap_bulk_write(data->regmap, reg, &be, sizeof(be));
> +}
> +
> +static int p3t1755_trigger_one_shot(struct p3t1755_data *data)
> +{
> +	unsigned int config;
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &config);
> +	if (ret)
> +		goto out;
> +
> +	if (!(config & P3T1755_SHUTDOWN_BIT)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	config |= P3T1755_ONE_SHOT_BIT;
> +	ret = regmap_write(data->regmap, P3T1755_REG_CFGR, config);
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static ssize_t p3t1755_attr_store(struct device *dev, struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct p3t1755_data *data = iio_priv(indio_dev);
> +	int ret;
> +	bool enable;
> +
> +	switch (iattr->address) {
> +	case P3T1755_ATTR_TRIGGER_ONE_SHOT:
> +		ret = kstrtobool(buf, &enable);
> +		if (ret || !enable)
> +			return ret ? ret : -EINVAL;
> +		ret = p3t1755_trigger_one_shot(data);
> +		return ret ?: count;
> +
> +	default:
> +		return -EINVAL;
> +		}
> +	}
> +
> +static IIO_DEVICE_ATTR(trigger_one_shot, 0200, NULL, p3t1755_attr_store,
> +		       P3T1755_ATTR_TRIGGER_ONE_SHOT);
> +
> +static const struct iio_event_spec p3t1755_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE)
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE)
> +	},
> +};
> +
> +static const struct iio_chan_spec p3t1755_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_ENABLE) |
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.event_spec = p3t1755_events,
> +		.num_event_specs = ARRAY_SIZE(p3t1755_events),
> +	},
> +};
> +
> +const struct p3t1755_info p3t1755_channels_info = {
> +	.name = "p3t1755",
> +	.channels = p3t1755_channels,
> +	.num_channels = ARRAY_SIZE(p3t1755_channels)
> +};
> +EXPORT_SYMBOL_NS(p3t1755_channels_info, IIO_P3T1755);
> +
> +const struct p3t1755_info p3t1750_channels_info = {
> +	.name = "p3t1750",
> +	.channels = p3t1755_channels,
> +	.num_channels = ARRAY_SIZE(p3t1755_channels)
> +};
> +EXPORT_SYMBOL_NS(p3t1750_channels_info, IIO_P3T1755);
> +
> +static struct attribute *p3t1755_attributes[] = {
> +	&iio_dev_attr_trigger_one_shot.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group p3t1755_attr_group = {
> +	.attrs = p3t1755_attributes,
> +};
> +
> +static const struct iio_info p3t1755_info = {
> +	.read_raw = p3t1755_read_raw,
> +	.write_raw = p3t1755_write_raw,
> +	.read_event_value = p3t1755_read_event_value,
> +	.write_event_value = p3t1755_write_event_value,
> +	.attrs = &p3t1755_attr_group,
> +};
> +
> +static irqreturn_t p3t1755_irq_handler(int irq, void *dev_id)
> +{
> +	struct iio_dev *indio_dev = dev_id;
> +
> +	dev_dbg(&indio_dev->dev, "IRQ triggered, processing threshold event\n");
> +
> +	p3t1755_push_thresh_event(indio_dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +int p3t1755_probe(struct device *dev, const struct p3t1755_info *chip,
> +		  struct regmap *regmap, bool tm_mode, int fq_bits, int irq)
> +{
> +	struct p3t1755_data *data;
> +	struct iio_dev *iio_dev;
> +	unsigned long irq_flags;
> +	int ret;
> +
> +	iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(iio_dev);
> +	data->dev = dev;
> +	data->regmap = regmap;
> +	data->tm_mode = tm_mode;
> +
> +	mutex_init(&data->lock);
> +
> +	iio_dev->name = chip->name;
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +	iio_dev->info = &p3t1755_info;
> +	iio_dev->channels = chip->channels;
> +	iio_dev->num_channels = chip->num_channels;
> +
> +	dev_set_drvdata(dev, iio_dev);
> +
> +	ret = regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
> +				 P3T1755_TM_BIT,
> +				(tm_mode ? P3T1755_TM_BIT : 0));
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Failed to update TM bit\n");
> +
> +	if (fq_bits >= 0)
> +		regmap_update_bits(data->regmap, P3T1755_REG_CFGR, P3T1755_FAULT_QUEUE_MASK,
> +				   fq_bits << P3T1755_FAULT_QUEUE_SHIFT);
> +
> +	ret = devm_iio_device_register(dev, iio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Temperature sensor failed to register\n");
> +
> +	if (irq > 0) {
> +		iio_dev = dev_get_drvdata(dev);

iio_dev is already in scope, so this is not needed.

> +		data = iio_priv(iio_dev);

ditto.

> +
> +		if (tm_mode)
> +			irq_flags = IRQF_TRIGGER_FALLING;
> +		else
> +			irq_flags = (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING);

Usually, these flags are coming from the interrupts property in the
devicetree.

> +
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						p3t1755_irq_handler, irq_flags | IRQF_ONESHOT,
> +						"p3t175x", iio_dev);
> +		if (ret)
> +			dev_err_probe(dev, ret, "Failed to request IRQ: %d\n", ret);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(p3t1755_probe, IIO_P3T1755);
> +
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP P3T175x Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/temperature/p3t/p3t1755_i2c.c b/drivers/iio/temperature/p3t/p3t1755_i2c.c
> new file mode 100644
> index 000000000000..7c12f326a859
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755_i2c.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NXP P3T175x Temperature Sensor Driver
> + *
> + * Copyright 2025 NXP
> + */
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
> +
> +#include "p3t1755.h"
> +
> +static const struct regmap_config p3t1755_i2c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct of_device_id p3t1755_i2c_of_match[] = {
> +	{ .compatible = "nxp,p3t1755-iio", .data = &p3t1755_channels_info },
> +	{ .compatible = "nxp,p3t1750-iio", .data = &p3t1750_channels_info },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, p3t1755_i2c_of_match);
> +
> +static const struct i2c_device_id p3t1755_i2c_id_table[] = {
> +	{ "p3t1755", (kernel_ulong_t)&p3t1755_channels_info },
> +	{ "p3t1750", (kernel_ulong_t)&p3t1750_channels_info},

Missing space before }.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, p3t1755_i2c_id_table);
> +
> +static int p3t1755_i2c_probe(struct i2c_client *client)
> +{
> +	const struct p3t1755_info *chip;
> +	struct regmap *regmap;
> +	bool tm_mode = false;
> +	int fq_bits = -1;
> +	int ret;
> +	u32 fq;
> +
> +	regmap = devm_regmap_init_i2c(client, &p3t1755_i2c_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		return dev_err_probe(&client->dev, PTR_ERR(regmap),
> +				     "regmap init failed\n");
> +	}
> +
> +	tm_mode = device_property_read_bool(&client->dev, "nxp,interrupt-mode");
> +
> +	if (!device_property_read_u32(&client->dev, "nxp,fault-queue", &fq)) {
> +		fq_bits = p3t1755_fault_queue_to_bits(fq);
> +		if (fq_bits < 0) {
> +			return dev_err_probe(&client->dev, fq_bits,
> +						     "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
> +			}
> +	}
> +
> +	dev_dbg(&client->dev, "Using TM mode: %s\n",
> +		tm_mode ? "Interrupt" : "Comparator");
> +
> +	chip = i2c_get_match_data(client);
> +
> +	dev_dbg(&client->dev, "Registering p3t175x temperature sensor");
> +
> +	ret = p3t1755_probe(&client->dev, chip, regmap,
> +			    tm_mode, fq_bits, client->irq);
> +
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret, "p3t175x probe failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver p3t1755_driver = {
> +	.driver = {
> +		.name = "p3t175x_i2c",
> +		.of_match_table = p3t1755_i2c_of_match,
> +	},
> +	.probe = p3t1755_i2c_probe,
> +	.id_table = p3t1755_i2c_id_table,
> +};
> +module_i2c_driver(p3t1755_driver);
> +
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP P3T175x I2C Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_P3T1755);
> diff --git a/drivers/iio/temperature/p3t/p3t1755_i3c.c b/drivers/iio/temperature/p3t/p3t1755_i3c.c
> new file mode 100644
> index 000000000000..9f61544b2eb6
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755_i3c.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * NXP P3T175x Temperature Sensor Driver
> + *
> + * Copyright 2025 NXP
> + */
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/master.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
> +
> +#include "p3t1755.h"
> +
> +static void p3t1755_ibi_handler(struct i3c_device *dev,
> +				const struct i3c_ibi_payload *payload)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&dev->dev);
> +
> +	p3t1755_push_thresh_event(indio_dev);
> +}
> +
> +/*
> + * Both P3T1755 and P3T1750 share the same I3C PID (0x011B:0x152A),
> + * making runtime differentiation impossible, so using "p3t1755" as
> + * name in sysfs and IIO for I3C based instances.
> + */
> +static const struct i3c_device_id p3t1755_i3c_ids[] = {
> +	I3C_DEVICE(0x011B, 0x152A, &p3t1755_channels_info),
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i3c, p3t1755_i3c_ids);
> +
> +static void p3t1755_disable_ibi(void *data)
> +{
> +	i3c_device_disable_ibi((struct i3c_device *)data);
> +}
> +
> +static void p3t1755_free_ibi(void *data)
> +{
> +	i3c_device_free_ibi((struct i3c_device *)data);
> +}
> +
> +static int p3t1755_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	const struct regmap_config p3t1755_i3c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,

Needs more indent.

> +	};
> +
> +	const struct i3c_device_id *id = i3c_device_match_id(i3cdev, p3t1755_i3c_ids);
> +	const struct p3t1755_info *chip;
> +	struct device *dev = &i3cdev->dev;
> +	struct i3c_ibi_setup ibi_setup;
> +	struct regmap *regmap;
> +	bool tm_mode = false;
> +	int fq_bits = -1;
> +	int ret;
> +	u32 fq;
> +
> +	chip = id ? id->data : NULL;
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &p3t1755_i3c_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		return dev_err_probe(&i3cdev->dev, PTR_ERR(regmap),
> +				     "Failed to register I3C regmap %ld\n", PTR_ERR(regmap));
> +	}
> +
> +	tm_mode = device_property_read_bool(dev, "nxp,interrupt-mode");
> +
> +	if (!device_property_read_u32(dev, "nxp,fault-queue", &fq)) {
> +		fq_bits = p3t1755_fault_queue_to_bits(fq);
> +		if (fq_bits < 0) {
> +			return dev_err_probe(&i3cdev->dev, fq_bits,
> +					     "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
> +		}
> +	}
> +
> +	dev_dbg(&i3cdev->dev, "Using TM mode: %s\n", tm_mode ? "Interrupt" : "Comparator");
> +
> +	ret = p3t1755_probe(dev, chip, regmap, tm_mode, fq_bits, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "p3t175x probe failed: %d\n", ret);
> +
> +	if (!tm_mode) {
> +		dev_warn(&i3cdev->dev, "IBI not supported in comparator mode, skipping IBI registration\n");
> +		return 0;
> +	}
> +
> +	ibi_setup = (struct i3c_ibi_setup) {
> +		.handler = p3t1755_ibi_handler,
> +		.num_slots = 4,
> +		.max_payload_len = 0,
> +	};
> +
> +	ret = i3c_device_request_ibi(i3cdev, &ibi_setup);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request IBI\n");
> +
> +	ret = i3c_device_enable_ibi(i3cdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable IBI\n");
> +
> +	ret = devm_add_action_or_reset(dev, p3t1755_disable_ibi, i3cdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register IBI disable action\n");
> +
> +	ret = devm_add_action_or_reset(dev, p3t1755_free_ibi, i3cdev);

This should be immediatly after the action that it undoes. I assume
i3c_device_request_ibi(). Otherwise, if enable fails, it won't be
freed.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register IBI free action\n");
> +
> +	dev_dbg(&i3cdev->dev, "IBI successfully registered\n");
> +
> +	return 0;
> +}
> +
> +static struct i3c_driver p3t1755_driver = {
> +	.driver = {
> +		.name = "p3t175x_i3c",
> +	},
> +	.probe = p3t1755_i3c_probe,
> +	.id_table = p3t1755_i3c_ids,
> +};
> +module_i3c_driver(p3t1755_driver);
> +
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP P3T175x I3C Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_P3T1755);


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

* Re: [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor
  2025-08-27 10:31 ` [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor Lakshay Piplani
  2025-08-27 14:36   ` David Lechner
@ 2025-08-29 15:43   ` Andy Shevchenko
  2025-08-31 16:46   ` Krzysztof Kozlowski
  2 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2025-08-29 15:43 UTC (permalink / raw)
  To: Lakshay Piplani
  Cc: linux-kernel, linux-iio, jic23, dlechner, nuno.sa, andy,
	marcelo.schmitt1, gregkh, viro, peterz, jstephan, robh, krzk+dt,
	conor+dt, devicetree, ilpo.jarvinen, jonathan.cameron, akpm, chao,
	jaegeuk, vikash.bansal, priyanka.jain, shashank.rebbapragada,
	Frank.Li

On Wed, Aug 27, 2025 at 1:31 PM Lakshay Piplani <lakshay.piplani@nxp.com> wrote:
>
> Add support for the NXP P3T175x (P3T1750/P3T1755) family of temperature
> sensor devices. These devices communicates via both I2C or I3C interfaces.
>
> This driver belongs under IIO because:
>   The P3T1750/P3T1755 sensors require interrupt or IBI support to handle
>   threshold events, which the hwmon subsystem does not provide. In contrast,
>   the IIO subsystem offers robust event handling that matches the hardware
>   capabilities of these sensors. Therefore, this driver is better suited
>   under IIO.

...

> +Date:          August 2025
> +KernelVersion: 6.17
> +Contact:       Lakshay Piplani <lakshay.piplani@nxp.com>



...

>           This driver can also be built as a module. If so, the module
>           will be called mcp9600.

Missed blank line here.

> +source "drivers/iio/temperature/p3t/Kconfig"
>
>  endmenu

...

> +config IIO_P3T1755_I2C
> +       tristate "NXP P3T1755 temprature sensor I2C driver"

temperature

> +       select IIO_P3T1755
> +       select REGMAP_I2C
> +       help
> +         Say yes here to build support for NXP P3T1755 I2C temperature
> +         sensor.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called p3t1755_i2c
> +
> +config IIO_P3T1755_I3C
> +       tristate "NXP P3T1755 temprature sensor I3C driver"

Ditto.

> +       select IIO_P3T1755
> +       select REGMAP_I3C
> +       depends on I3C
> +       help
> +         Say yes here to build support for NXP P3T1755 I3C temperature
> +         sensor.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called p3t1755_i3c

...

> +#ifndef P3T1755_H
> +#define P3T1755_H
> +
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>

This is definitely an incorrect list of the inclusions. Follow the
IWYU principle (include what you use).

...

> +#define P3T1755_SHUTDOWN_BIT           BIT(0)
> +#define P3T1755_TM_BIT                 BIT(1)
> +#define P3T1755_POL_BIT                        BIT(2)
> +#define P3T1755_ONE_SHOT_BIT           BIT(7)

+bits.h

...

> +extern const struct p3t1755_info p3t1755_channels_info;
> +extern const struct p3t1755_info p3t1750_channels_info;

Please, move this after the actual structure type definition.

...

> +enum p3t1755_hw_id {
> +       P3T1755_ID = 0,
> +       P3T1750_ID,
> +};

Is this related to HW (like values that are read from HW? If so, make
them all to be explicitly assigned. Otherwise, drop the assignment and
sort the list.

...

> +struct p3t1755_data {
> +       struct device *dev;
> +       struct regmap *regmap;
> +       struct mutex lock; /* Protects access to sensor registers */

+ mutex.h

> +       bool tm_mode;

+ types.h

> +};

> +#endif /* P3T1755_H */

The rest can be declared with forward declarations.

...

> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/interrupt.h>
> +#include <linux/types.h>
> +#include <linux/regmap.h>
> +#include <linux/device.h>
> +#include <linux/iio/events.h>

Please, keep it ordered, also move iio/* to a separate group

linux/*
...blank line...
linux/iio/*

...

> +static const struct {
> +       u8 bits;
> +       unsigned int freq_hz;
> +} p3t1755_samp_freqs[] = {
> +       { 0x00, 36 },
> +       { 0x01, 18 },
> +       { 0x02, 9 },
> +       { 0x03, 4 },

Is it 4 for real? To me it sounds like it should be 4.5. Also, the
bits field is redundant. Index is the same.

> +};

...

> +int p3t1755_fault_queue_to_bits(int val)
> +{
> +       int i;

Here and elsewhere why is 'i' signed?

> +       for (i = 0; i < ARRAY_SIZE(p3t1755_fault_queue_values); i++)
> +               if (p3t1755_fault_queue_values[i] == val)
> +                       return i;
> +       return -EINVAL;
> +}

...

> +static int p3t1755_read_raw(struct iio_dev *indio_dev,
> +                           struct iio_chan_spec const *channel, int *val,
> +                           int *val2, long mask)
> +{
> +       struct p3t1755_data *data = iio_priv(indio_dev);
> +       unsigned int cfgr;
> +       __be16 be;
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               ret = regmap_bulk_read(data->regmap, P3T1755_REG_TEMP, &be, sizeof(be));

> +               if (ret < 0) {

Here and elsewhere out of a sudden the ' < 0' part. Please, remove it
where it's not needed.

> +                       dev_err(data->dev, "Failed to read temperature register\n");
> +                       return ret;
> +               }
> +               *val = sign_extend32(be16_to_cpu(be) >> 4, 11);
> +
> +               return IIO_VAL_INT;
> +
> +       case IIO_CHAN_INFO_SCALE:
> +               *val = 625;
> +               *val2 = 10000;
> +
> +               return IIO_VAL_FRACTIONAL;
> +
> +       case IIO_CHAN_INFO_ENABLE:
> +               ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
> +               if (ret < 0) {
> +                       dev_err(data->dev, "Failed to read configuration register\n");
> +                       return ret;
> +               }
> +               *val = !FIELD_GET(P3T1755_SHUTDOWN_BIT, cfgr);
> +
> +               return IIO_VAL_INT;
> +
> +       case IIO_CHAN_INFO_SAMP_FREQ:

> +               u8 sel;

Here and elsewhere, we usually don't allow mix definitions with the
code, only in exceptional cases (RAII, loop iterators). To fix this,
the whole case block should go in curly braces {}.

> +               ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
> +               if (ret < 0) {
> +                       dev_err(data->dev, "Failed to read configuration register\n");
> +                       return ret;
> +               }
> +
> +               sel = FIELD_GET(P3T1755_CONVERSION_TIME_BITS, cfgr);
> +               if (sel >= ARRAY_SIZE(p3t1755_samp_freqs))
> +                       return -EINVAL;
> +
> +               *val = p3t1755_samp_freqs[sel].freq_hz;
> +
> +               return IIO_VAL_INT;
> +       default:
> +               return -EINVAL;
> +       }
> +}

...

> +static int p3t1755_write_event_value(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 p3t1755_data *data = iio_priv(indio_dev);
> +       unsigned int reg;
> +       __be16 be;
> +
> +       if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
> +               return -EINVAL;
> +
> +       reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
> +                                          P3T1755_REG_LOW_LIM;

> +       if (val < -2048 || val > 2047)
> +               return -ERANGE;

Logically in_range() here is better and since it's s11 type, I would
comment on this like "compare that the value is in a range of the
11-bit signed type".

> +       be = cpu_to_be16((u16)((val & 0xfff) << 4));

Why casting? Why not GENMASK()?

> +       return regmap_bulk_write(data->regmap, reg, &be, sizeof(be));
> +}
> +
> +static int p3t1755_trigger_one_shot(struct p3t1755_data *data)
> +{
> +       unsigned int config;
> +       int ret;
> +
> +       mutex_lock(&data->lock);

Use guard()() from cleanup.h

> +       ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &config);
> +       if (ret)
> +               goto out;
> +
> +       if (!(config & P3T1755_SHUTDOWN_BIT)) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       config |= P3T1755_ONE_SHOT_BIT;
> +       ret = regmap_write(data->regmap, P3T1755_REG_CFGR, config);
> +
> +out:
> +       mutex_unlock(&data->lock);
> +       return ret;
> +}

...

> +       switch (iattr->address) {
> +       case P3T1755_ATTR_TRIGGER_ONE_SHOT:
> +               ret = kstrtobool(buf, &enable);
> +               if (ret || !enable)
> +                       return ret ? ret : -EINVAL;

Split to two conditionals of the same level

 if (ret)
  return ret;
if (...)
  return -E...

> +               ret = p3t1755_trigger_one_shot(data);
> +               return ret ?: count;

Ditto.

> +       default:
> +               return -EINVAL;
> +               }
> +       }

...

> +static IIO_DEVICE_ATTR(trigger_one_shot, 0200, NULL, p3t1755_attr_store,
> +                      P3T1755_ATTR_TRIGGER_ONE_SHOT);

IIO_DEVICE_ATTR_WO()

...

> +static const struct iio_event_spec p3t1755_events[] = {
> +       {
> +               .type = IIO_EV_TYPE_THRESH,
> +               .dir = IIO_EV_DIR_RISING,
> +               .mask_separate = BIT(IIO_EV_INFO_VALUE)

Leave trailing comma.

> +       },
> +       {
> +               .type = IIO_EV_TYPE_THRESH,
> +               .dir = IIO_EV_DIR_FALLING,
> +               .mask_separate = BIT(IIO_EV_INFO_VALUE)

Ditto.

> +       },
> +};

...

> +const struct p3t1755_info p3t1755_channels_info = {
> +       .name = "p3t1755",
> +       .channels = p3t1755_channels,
> +       .num_channels = ARRAY_SIZE(p3t1755_channels)

Ditto.

> +};

...

> +static struct attribute *p3t1755_attributes[] = {
> +       &iio_dev_attr_trigger_one_shot.dev_attr.attr,

> +       NULL,

Remove trailing comma for the terminator entry.

> +};

...

> +int p3t1755_probe(struct device *dev, const struct p3t1755_info *chip,
> +                 struct regmap *regmap, bool tm_mode, int fq_bits, int irq)
> +{
> +       struct p3t1755_data *data;
> +       struct iio_dev *iio_dev;
> +       unsigned long irq_flags;
> +       int ret;
> +
> +       iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +       if (!iio_dev)
> +               return -ENOMEM;
> +
> +       data = iio_priv(iio_dev);
> +       data->dev = dev;
> +       data->regmap = regmap;
> +       data->tm_mode = tm_mode;

> +       mutex_init(&data->lock);

devm_mutex_init()

> +       iio_dev->name = chip->name;
> +       iio_dev->modes = INDIO_DIRECT_MODE;
> +       iio_dev->info = &p3t1755_info;
> +       iio_dev->channels = chip->channels;
> +       iio_dev->num_channels = chip->num_channels;
> +
> +       dev_set_drvdata(dev, iio_dev);
> +
> +       ret = regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
> +                                P3T1755_TM_BIT,
> +                               (tm_mode ? P3T1755_TM_BIT : 0));

Too many parentheses.

> +       if (ret)
> +               return dev_err_probe(data->dev, ret, "Failed to update TM bit\n");
> +
> +       if (fq_bits >= 0)
> +               regmap_update_bits(data->regmap, P3T1755_REG_CFGR, P3T1755_FAULT_QUEUE_MASK,
> +                                  fq_bits << P3T1755_FAULT_QUEUE_SHIFT);
> +
> +       ret = devm_iio_device_register(dev, iio_dev);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Temperature sensor failed to register\n");
> +
> +       if (irq > 0) {
> +               iio_dev = dev_get_drvdata(dev);
> +               data = iio_priv(iio_dev);

> +               if (tm_mode)
> +                       irq_flags = IRQF_TRIGGER_FALLING;
> +               else
> +                       irq_flags = (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING);

As David said, we use firmware description for these.

> +               ret = devm_request_threaded_irq(dev, irq, NULL,
> +                                               p3t1755_irq_handler, irq_flags | IRQF_ONESHOT,
> +                                               "p3t175x", iio_dev);
> +               if (ret)
> +                       dev_err_probe(dev, ret, "Failed to request IRQ: %d\n", ret);
> +       }
> +
> +       return 0;
> +}

...

> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>

Follow IWYU, please.

...

> +static int p3t1755_i2c_probe(struct i2c_client *client)
> +{
> +       const struct p3t1755_info *chip;
> +       struct regmap *regmap;
> +       bool tm_mode = false;
> +       int fq_bits = -1;
> +       int ret;
> +       u32 fq;
> +
> +       regmap = devm_regmap_init_i2c(client, &p3t1755_i2c_regmap_config);
> +       if (IS_ERR(regmap)) {
> +               return dev_err_probe(&client->dev, PTR_ERR(regmap),

With

  struct device *dev = &client->dev;

this and other code statements become neater.

> +                                    "regmap init failed\n");
> +       }
> +
> +       tm_mode = device_property_read_bool(&client->dev, "nxp,interrupt-mode");
> +
> +       if (!device_property_read_u32(&client->dev, "nxp,fault-queue", &fq)) {
> +               fq_bits = p3t1755_fault_queue_to_bits(fq);

> +               if (fq_bits < 0) {
> +                       return dev_err_probe(&client->dev, fq_bits,
> +                                                    "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
> +                       }

Why {} ?

> +       }
> +
> +       dev_dbg(&client->dev, "Using TM mode: %s\n",
> +               tm_mode ? "Interrupt" : "Comparator");
> +
> +       chip = i2c_get_match_data(client);
> +
> +       dev_dbg(&client->dev, "Registering p3t175x temperature sensor");
> +
> +       ret = p3t1755_probe(&client->dev, chip, regmap,
> +                           tm_mode, fq_bits, client->irq);
> +
> +       if (ret) {
> +               dev_err_probe(&client->dev, ret, "p3t175x probe failed: %d\n", ret);
> +               return ret;

Again, it looks like drivers are written by two or more people. Use
consistent style everywhere.

  return dev_err_probe(...);

> +       }
> +
> +       return 0;
> +}

...

> +++ b/drivers/iio/temperature/p3t/p3t1755_i3c.c

> +/*
> + * Both P3T1755 and P3T1750 share the same I3C PID (0x011B:0x152A),
> + * making runtime differentiation impossible, so using "p3t1755" as
> + * name in sysfs and IIO for I3C based instances.
> + */
> +static const struct i3c_device_id p3t1755_i3c_ids[] = {
> +       I3C_DEVICE(0x011B, 0x152A, &p3t1755_channels_info),
> +       { },

No comma for the terminator line.

> +};

> +

Redundant blank line.

> +MODULE_DEVICE_TABLE(i3c, p3t1755_i3c_ids);

> +static int p3t1755_i3c_probe(struct i3c_device *i3cdev)
> +{
> +       const struct regmap_config p3t1755_i3c_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       };

Why not in the same way as in i²c driver?

> +       const struct i3c_device_id *id = i3c_device_match_id(i3cdev, p3t1755_i3c_ids);
> +       const struct p3t1755_info *chip;
> +       struct device *dev = &i3cdev->dev;
> +       struct i3c_ibi_setup ibi_setup;
> +       struct regmap *regmap;
> +       bool tm_mode = false;
> +       int fq_bits = -1;
> +       int ret;
> +       u32 fq;

> +       chip = id ? id->data : NULL;

Can i3c code gain or may already have the analogue of
device_get_match_data() as i²c has?


> +       regmap = devm_regmap_init_i3c(i3cdev, &p3t1755_i3c_regmap_config);
> +       if (IS_ERR(regmap)) {
> +               return dev_err_probe(&i3cdev->dev, PTR_ERR(regmap),
> +                                    "Failed to register I3C regmap %ld\n", PTR_ERR(regmap));
> +       }
> +
> +       tm_mode = device_property_read_bool(dev, "nxp,interrupt-mode");

> +       if (!device_property_read_u32(dev, "nxp,fault-queue", &fq)) {
> +               fq_bits = p3t1755_fault_queue_to_bits(fq);
> +               if (fq_bits < 0) {
> +                       return dev_err_probe(&i3cdev->dev, fq_bits,
> +                                            "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
> +               }
> +       }

Isn't it the same as in i²c? Make it part of the core probe instead.

> +       dev_dbg(&i3cdev->dev, "Using TM mode: %s\n", tm_mode ? "Interrupt" : "Comparator");

Ditto. Also why is this message detached from the above
device_property_read_bool()?

> +       ret = p3t1755_probe(dev, chip, regmap, tm_mode, fq_bits, 0);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "p3t175x probe failed: %d\n", ret);
> +
> +       if (!tm_mode) {
> +               dev_warn(&i3cdev->dev, "IBI not supported in comparator mode, skipping IBI registration\n");
> +               return 0;
> +       }
> +
> +       ibi_setup = (struct i3c_ibi_setup) {
> +               .handler = p3t1755_ibi_handler,
> +               .num_slots = 4,
> +               .max_payload_len = 0,
> +       };
> +
> +       ret = i3c_device_request_ibi(i3cdev, &ibi_setup);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to request IBI\n");
> +
> +       ret = i3c_device_enable_ibi(i3cdev);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to enable IBI\n");
> +
> +       ret = devm_add_action_or_reset(dev, p3t1755_disable_ibi, i3cdev);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to register IBI disable action\n");
> +
> +       ret = devm_add_action_or_reset(dev, p3t1755_free_ibi, i3cdev);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to register IBI free action\n");

> +       dev_dbg(&i3cdev->dev, "IBI successfully registered\n");

Noise, remove.

> +       return 0;
> +}


--
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support
  2025-08-27 10:31 [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support Lakshay Piplani
  2025-08-27 10:31 ` [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor Lakshay Piplani
  2025-08-27 13:59 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support David Lechner
@ 2025-08-31 16:01 ` Jonathan Cameron
  2025-08-31 16:47 ` Krzysztof Kozlowski
  3 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2025-08-31 16:01 UTC (permalink / raw)
  To: Lakshay Piplani
  Cc: linux-kernel, linux-iio, dlechner, nuno.sa, andy,
	marcelo.schmitt1, gregkh, viro, peterz, jstephan, robh, krzk+dt,
	conor+dt, devicetree, ilpo.jarvinen, jonathan.cameron, akpm, chao,
	jaegeuk, vikash.bansal, priyanka.jain, shashank.rebbapragada,
	Frank.Li

On Wed, 27 Aug 2025 16:01:04 +0530
Lakshay Piplani <lakshay.piplani@nxp.com> wrote:

> Add bindings for the NXP P3T175x (P3T1750/P3T1755) temperature
> sensor, supporting both I2C & I3C interfaces.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
> ---
> Changes in v2 (addressed review comments from Krzysztof Kozlowski):
>  - Dropped nxp,alert-active-high: unnecessary as polarity handling is implicit in driver.
>  - Retained nxp,interrupt-mode: required to program TM(thermostat mode) bit; enables interrupt
>    (latched) mode. If not present in DT entry comparator mode is set as default.
>  - Retained nxp,fault-queue: This needs to be configured during device initialization.
>    This property configures the hardware fault queue length. Defines how many consecutive faults
>    are required before ALERT/IBI is asserted, preventing false triggers in noisy environments.
>  - The `reg` property remains required to satisfy `dt_binding_check`.
>  - Fixed YAML formatting, line wrapping, and examples.
>  - Changed compatibles from nxp,p3t1755 to nxp,p3t1755-iio and nxp,p3t1750 to nxp,p3t1750-iio
>    as reported by kernel test robot.
> 
>  .../bindings/iio/temperature/nxp,p3t1755.yaml | 97 +++++++++++++++++++
>  1 file changed, 97 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml b/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> new file mode 100644
> index 000000000000..4eb6fc5cb247
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/nxp,p3t1755.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP P3T175x Temperature Sensor
> +
> +maintainers:
> +  - Lakshay Piplani <lakshay.piplani@nxp.com>
> +
> +description: |
> +  Datasheet: https://www.nxp.com/docs/en/data-sheet/P3T1755.pdf
> +
> +  P3T175x (P3T1750/P3T1755) is a digital temperature sensor with a range of -40°C to
> +  +125°C and a 12-bit resolution. It supports communication over

Wrap consistently to 80 chars

> +  both I2C and I3C interfaces.
> +
> +  The I2C interface supports up to 32 static addresses and provides
> +  an ALERT output to signal when temperature thresholds are crossed.
> +
> +  The I3C interface supports In-Band interrupts (IBI) in interrupt mode,
> +  allowing the device to notify the controller of threshold events without
> +  dedicated alert pin.
> +
> +  The device supports configurable thermostat modes (interrupt or comparator),
> +  fault queue length etc.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,p3t1750-iio
> +      - nxp,p3t1755-iio

Wouldn't expect an 'iio' bit in a compatible. It's not about what driver
is binding, it's about what the device is.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +    description: |
> +      In I2C mode, the device supports up to 32 static addresses.
> +      In I3C mode, the 'reg' property encodes a triplet of
> +      <static-address BCR PID> used for device matching.
> +      Static address is optional if matching is done via PID.
> +
> +  nxp,interrupt-mode:
> +    type: boolean
> +    description: |
> +      Enables interrupt mode (TM = 1), where alerts are latched until
> +      cleared by a register read.
> +      Required for IBI support over I3C. On I2C, both interrupt and
> +      comparator mode support events.

What David said wrt to this. If it is discoverable from the bus type
doesn't need a property.

> +
> +  nxp,fault-queue:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 6]
> +    description: |
> +      Number of consecutive temperature limit
> +      violations required before an alert is triggered.
> +      valid values:- 1, 2, 4 or 6.
> +      If unspecified, hardware default (2) is used.

This is the userspace period control. Don't have it in DT
as nothing much to do with wiring.

> +
> +  assigned-address:
> +    true

Is there not a top level i3c file that we can $ref like tend to do for spi-peripheral-props.yaml?
Seems not unfortunately but we do need some sort of reference for this.




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

* Re: [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor
  2025-08-27 10:31 ` [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor Lakshay Piplani
  2025-08-27 14:36   ` David Lechner
  2025-08-29 15:43   ` Andy Shevchenko
@ 2025-08-31 16:46   ` Krzysztof Kozlowski
  2025-09-01 15:47     ` Jonathan Cameron
  2 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-31 16:46 UTC (permalink / raw)
  To: Lakshay Piplani, linux-kernel, linux-iio, jic23, dlechner,
	nuno.sa, andy, marcelo.schmitt1, gregkh, viro, peterz, jstephan,
	robh, krzk+dt, conor+dt, devicetree, ilpo.jarvinen,
	jonathan.cameron, akpm, chao, jaegeuk
  Cc: vikash.bansal, priyanka.jain, shashank.rebbapragada, Frank.Li

On 27/08/2025 12:31, Lakshay Piplani wrote:
> Add support for the NXP P3T175x (P3T1750/P3T1755) family of temperature
> sensor devices. These devices communicates via both I2C or I3C interfaces.
> 
> This driver belongs under IIO because:
>   The P3T1750/P3T1755 sensors require interrupt or IBI support to handle
>   threshold events, which the hwmon subsystem does not provide. In contrast,
>   the IIO subsystem offers robust event handling that matches the hardware
>   capabilities of these sensors. Therefore, this driver is better suited
>   under IIO.
> 


...

> +static int p3t1755_write_event_value(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 p3t1755_data *data = iio_priv(indio_dev);
> +	unsigned int reg;
> +	__be16 be;
> +
> +	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
> +					   P3T1755_REG_LOW_LIM;
> +
> +	if (val < -2048 || val > 2047)
> +		return -ERANGE;
> +
> +	be = cpu_to_be16((u16)((val & 0xfff) << 4));
> +
> +	return regmap_bulk_write(data->regmap, reg, &be, sizeof(be));

Now I wonder why regmap does not handle your data format?

> +}
> +
> +static int p3t1755_trigger_one_shot(struct p3t1755_data *data)
> +{
> +	unsigned int config;
> +	int ret;
> +
> +	mutex_lock(&data->lock);

Just use guard.

> +
> +	ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &config);
> +	if (ret)
> +		goto out;
> +
> +	if (!(config & P3T1755_SHUTDOWN_BIT)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	config |= P3T1755_ONE_SHOT_BIT;
> +	ret = regmap_write(data->regmap, P3T1755_REG_CFGR, config);
> +
> +out:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}


...


> +static int p3t1755_i2c_probe(struct i2c_client *client)
> +{
> +	const struct p3t1755_info *chip;
> +	struct regmap *regmap;
> +	bool tm_mode = false;
> +	int fq_bits = -1;
> +	int ret;
> +	u32 fq;
> +
> +	regmap = devm_regmap_init_i2c(client, &p3t1755_i2c_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		return dev_err_probe(&client->dev, PTR_ERR(regmap),
> +				     "regmap init failed\n");
> +	}
> +
> +	tm_mode = device_property_read_bool(&client->dev, "nxp,interrupt-mode");
> +
> +	if (!device_property_read_u32(&client->dev, "nxp,fault-queue", &fq)) {
> +		fq_bits = p3t1755_fault_queue_to_bits(fq);
> +		if (fq_bits < 0) {
> +			return dev_err_probe(&client->dev, fq_bits,
> +						     "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
> +			}
> +	}
> +
> +	dev_dbg(&client->dev, "Using TM mode: %s\n",
> +		tm_mode ? "Interrupt" : "Comparator");

Pretty useless, static coming from DT :/

> +
> +	chip = i2c_get_match_data(client);
> +
> +	dev_dbg(&client->dev, "Registering p3t175x temperature sensor");

Drop

> +
> +	ret = p3t1755_probe(&client->dev, chip, regmap,
> +			    tm_mode, fq_bits, client->irq);
> +
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret, "p3t175x probe failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver p3t1755_driver = {
> +	.driver = {
> +		.name = "p3t175x_i2c",
> +		.of_match_table = p3t1755_i2c_of_match,
> +	},
> +	.probe = p3t1755_i2c_probe,
> +	.id_table = p3t1755_i2c_id_table,
> +};
> +module_i2c_driver(p3t1755_driver);
> +
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP P3T175x I2C Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_P3T1755);
> diff --git a/drivers/iio/temperature/p3t/p3t1755_i3c.c b/drivers/iio/temperature/p3t/p3t1755_i3c.c
> new file mode 100644
> index 000000000000..9f61544b2eb6
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755_i3c.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * NXP P3T175x Temperature Sensor Driver
> + *
> + * Copyright 2025 NXP
> + */
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/master.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
> +
> +#include "p3t1755.h"
> +
> +static void p3t1755_ibi_handler(struct i3c_device *dev,
> +				const struct i3c_ibi_payload *payload)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&dev->dev);
> +
> +	p3t1755_push_thresh_event(indio_dev);
> +}
> +
> +/*
> + * Both P3T1755 and P3T1750 share the same I3C PID (0x011B:0x152A),
> + * making runtime differentiation impossible, so using "p3t1755" as
> + * name in sysfs and IIO for I3C based instances.
> + */
> +static const struct i3c_device_id p3t1755_i3c_ids[] = {
> +	I3C_DEVICE(0x011B, 0x152A, &p3t1755_channels_info),
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i3c, p3t1755_i3c_ids);
> +
> +static void p3t1755_disable_ibi(void *data)
> +{
> +	i3c_device_disable_ibi((struct i3c_device *)data);
> +}
> +
> +static void p3t1755_free_ibi(void *data)
> +{
> +	i3c_device_free_ibi((struct i3c_device *)data);
> +}
> +
> +static int p3t1755_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	const struct regmap_config p3t1755_i3c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	};
> +
> +	const struct i3c_device_id *id = i3c_device_match_id(i3cdev, p3t1755_i3c_ids);
> +	const struct p3t1755_info *chip;
> +	struct device *dev = &i3cdev->dev;
> +	struct i3c_ibi_setup ibi_setup;
> +	struct regmap *regmap;
> +	bool tm_mode = false;
> +	int fq_bits = -1;
> +	int ret;
> +	u32 fq;
> +
> +	chip = id ? id->data : NULL;
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &p3t1755_i3c_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		return dev_err_probe(&i3cdev->dev, PTR_ERR(regmap),
> +				     "Failed to register I3C regmap %ld\n", PTR_ERR(regmap));
> +	}

No need for {}

> +
> +	tm_mode = device_property_read_bool(dev, "nxp,interrupt-mode");
> +
> +	if (!device_property_read_u32(dev, "nxp,fault-queue", &fq)) {
> +		fq_bits = p3t1755_fault_queue_to_bits(fq);
> +		if (fq_bits < 0) {
> +			return dev_err_probe(&i3cdev->dev, fq_bits,
> +					     "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
> +		}
> +	}
> +
> +	dev_dbg(&i3cdev->dev, "Using TM mode: %s\n", tm_mode ? "Interrupt" : "Comparator");
> +
> +	ret = p3t1755_probe(dev, chip, regmap, tm_mode, fq_bits, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "p3t175x probe failed: %d\n", ret);
> +
> +	if (!tm_mode) {
> +		dev_warn(&i3cdev->dev, "IBI not supported in comparator mode, skipping IBI registration\n");
> +		return 0;
> +	}
> +
> +	ibi_setup = (struct i3c_ibi_setup) {
> +		.handler = p3t1755_ibi_handler,
> +		.num_slots = 4,
> +		.max_payload_len = 0,
> +	};
> +
> +	ret = i3c_device_request_ibi(i3cdev, &ibi_setup);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request IBI\n");
> +
> +	ret = i3c_device_enable_ibi(i3cdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable IBI\n");
> +
> +	ret = devm_add_action_or_reset(dev, p3t1755_disable_ibi, i3cdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register IBI disable action\n");
> +
> +	ret = devm_add_action_or_reset(dev, p3t1755_free_ibi, i3cdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register IBI free action\n");
> +
> +	dev_dbg(&i3cdev->dev, "IBI successfully registered\n");

You really should not need this. You already have one probe debug.


Best regards,
Krzysztof

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

* Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support
  2025-08-27 10:31 [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support Lakshay Piplani
                   ` (2 preceding siblings ...)
  2025-08-31 16:01 ` Jonathan Cameron
@ 2025-08-31 16:47 ` Krzysztof Kozlowski
  3 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-31 16:47 UTC (permalink / raw)
  To: Lakshay Piplani, linux-kernel, linux-iio, jic23, dlechner,
	nuno.sa, andy, marcelo.schmitt1, gregkh, viro, peterz, jstephan,
	robh, krzk+dt, conor+dt, devicetree, ilpo.jarvinen,
	jonathan.cameron, akpm, chao, jaegeuk
  Cc: vikash.bansal, priyanka.jain, shashank.rebbapragada, Frank.Li

On 27/08/2025 12:31, Lakshay Piplani wrote:
> Add bindings for the NXP P3T175x (P3T1750/P3T1755) temperature
> sensor, supporting both I2C & I3C interfaces.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
> ---
> Changes in v2 (addressed review comments from Krzysztof Kozlowski):
>  - Dropped nxp,alert-active-high: unnecessary as polarity handling is implicit in driver.
>  - Retained nxp,interrupt-mode: required to program TM(thermostat mode) bit; enables interrupt
>    (latched) mode. If not present in DT entry comparator mode is set as default.
>  - Retained nxp,fault-queue: This needs to be configured during device initialization.
>    This property configures the hardware fault queue length. Defines how many consecutive faults
>    are required before ALERT/IBI is asserted, preventing false triggers in noisy environments.
>  - The `reg` property remains required to satisfy `dt_binding_check`.

Where was v1? Why aren't you using b4 for all this?

>  - Fixed YAML formatting, line wrapping, and examples.
>  - Changed compatibles from nxp,p3t1755 to nxp,p3t1755-iio and nxp,p3t1750 to nxp,p3t1750-iio
>    as reported by kernel test robot.

Huh? Why?

> 
>  .../bindings/iio/temperature/nxp,p3t1755.yaml | 97 +++++++++++++++++++
>  1 file changed, 97 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml b/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> new file mode 100644
> index 000000000000..4eb6fc5cb247
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/nxp,p3t1755.yaml
> @@ -0,0 +1,97 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/nxp,p3t1755.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP P3T175x Temperature Sensor
> +
> +maintainers:
> +  - Lakshay Piplani <lakshay.piplani@nxp.com>
> +
> +description: |
> +  Datasheet: https://www.nxp.com/docs/en/data-sheet/P3T1755.pdf
> +
> +  P3T175x (P3T1750/P3T1755) is a digital temperature sensor with a range of -40°C to
> +  +125°C and a 12-bit resolution. It supports communication over
> +  both I2C and I3C interfaces.
> +
> +  The I2C interface supports up to 32 static addresses and provides
> +  an ALERT output to signal when temperature thresholds are crossed.
> +
> +  The I3C interface supports In-Band interrupts (IBI) in interrupt mode,
> +  allowing the device to notify the controller of threshold events without
> +  dedicated alert pin.
> +
> +  The device supports configurable thermostat modes (interrupt or comparator),
> +  fault queue length etc.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,p3t1750-iio
> +      - nxp,p3t1755-iio

Drop iio

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +    description: |
> +      In I2C mode, the device supports up to 32 static addresses.
> +      In I3C mode, the 'reg' property encodes a triplet of
> +      <static-address BCR PID> used for device matching.
> +      Static address is optional if matching is done via PID.
> +
> +  nxp,interrupt-mode:
> +    type: boolean
> +    description: |
> +      Enables interrupt mode (TM = 1), where alerts are latched until
> +      cleared by a register read.
> +      Required for IBI support over I3C. On I2C, both interrupt and
> +      comparator mode support events.
> +
> +  nxp,fault-queue:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 6]
> +    description: |
> +      Number of consecutive temperature limit
> +      violations required before an alert is triggered.
> +      valid values:- 1, 2, 4 or 6.
> +      If unspecified, hardware default (2) is used.
> +
> +  assigned-address:
> +    true

Drop and instead we need to define i3c-peripheral-properties, just like
spi has. Then you reference here that file.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        temp-sensor@48 {
> +            compatible = "nxp,p3t1755-iio";
> +            reg = <0x48>;
> +            nxp,interrupt-mode;
> +            nxp,fault-queue = <6>;
> +            interrupt-parent = <&gpio2>;
> +            interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> +        };
> +    };
> +
> +  - |
> +    i3c {
> +      #address-cells = <3>;
> +      #size-cells = <0>;
> +      temp-sensor@48,236152a00 {
> +        reg = <0x48 0x236 0x152a00>;
> +        assigned-address = <0x50>;

This example is pretty incomplete.

> +      };
> +    };


Best regards,
Krzysztof

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

* Re: [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor
  2025-08-31 16:46   ` Krzysztof Kozlowski
@ 2025-09-01 15:47     ` Jonathan Cameron
  2025-09-02 11:06       ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2025-09-01 15:47 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Lakshay Piplani, linux-kernel, linux-iio, dlechner, nuno.sa, andy,
	marcelo.schmitt1, gregkh, viro, peterz, jstephan, robh, krzk+dt,
	conor+dt, devicetree, ilpo.jarvinen, jonathan.cameron, akpm, chao,
	jaegeuk, vikash.bansal, priyanka.jain, shashank.rebbapragada,
	Frank.Li

On Sun, 31 Aug 2025 18:46:32 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 27/08/2025 12:31, Lakshay Piplani wrote:
> > Add support for the NXP P3T175x (P3T1750/P3T1755) family of temperature
> > sensor devices. These devices communicates via both I2C or I3C interfaces.
> > 
> > This driver belongs under IIO because:
> >   The P3T1750/P3T1755 sensors require interrupt or IBI support to handle
> >   threshold events, which the hwmon subsystem does not provide. In contrast,
> >   the IIO subsystem offers robust event handling that matches the hardware
> >   capabilities of these sensors. Therefore, this driver is better suited
> >   under IIO.
> >   
> 
> 
Picking out one thing that made me curious.

> ...
> 
> > +static int p3t1755_write_event_value(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 p3t1755_data *data = iio_priv(indio_dev);
> > +	unsigned int reg;
> > +	__be16 be;
> > +
> > +	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
> > +		return -EINVAL;
> > +
> > +	reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
> > +					   P3T1755_REG_LOW_LIM;
> > +
> > +	if (val < -2048 || val > 2047)
> > +		return -ERANGE;
> > +
> > +	be = cpu_to_be16((u16)((val & 0xfff) << 4));
> > +
> > +	return regmap_bulk_write(data->regmap, reg, &be, sizeof(be));  
> 
> Now I wonder why regmap does not handle your data format?

This device does have a novel definition of register. There
are 4 of them, 3 of which are 12 bits zero padded to 16 and
the other 8 bits.

So, I think only way to wrap that up fully in regmap would be
a pair of regmaps one of which has only a single register in it.

Agreed though that using bulk accesses is not a good plan.
I'd been assuming that was actually a pair of registers, not
a single larger one.


Jonathan


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

* Re: [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor
  2025-09-01 15:47     ` Jonathan Cameron
@ 2025-09-02 11:06       ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2025-09-02 11:06 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Krzysztof Kozlowski, Lakshay Piplani, linux-kernel, linux-iio,
	dlechner, nuno.sa, andy, marcelo.schmitt1, gregkh, viro, peterz,
	jstephan, robh, krzk+dt, conor+dt, devicetree, ilpo.jarvinen,
	jonathan.cameron, akpm, chao, jaegeuk, vikash.bansal,
	priyanka.jain, shashank.rebbapragada, Frank.Li

On Mon, Sep 01, 2025 at 04:47:17PM +0100, Jonathan Cameron wrote:
> On Sun, 31 Aug 2025 18:46:32 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:

...

> This device does have a novel definition of register. There
> are 4 of them, 3 of which are 12 bits zero padded to 16 and
> the other 8 bits.
> 
> So, I think only way to wrap that up fully in regmap would be
> a pair of regmaps one of which has only a single register in it.
> 
> Agreed though that using bulk accesses is not a good plan.
> I'd been assuming that was actually a pair of registers, not
> a single larger one.

Why is it a problem? We have PMICs which have 16- and 8-bit registers and we do
use the bulk transfers. Yes, we have also an exception when it's required to
use byte transfers to make device work, but otherwise it works fine.

-- 
With Best Regards,
Andy Shevchenko



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

end of thread, other threads:[~2025-09-02 11:06 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-27 10:31 [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support Lakshay Piplani
2025-08-27 10:31 ` [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor Lakshay Piplani
2025-08-27 14:36   ` David Lechner
2025-08-29 15:43   ` Andy Shevchenko
2025-08-31 16:46   ` Krzysztof Kozlowski
2025-09-01 15:47     ` Jonathan Cameron
2025-09-02 11:06       ` Andy Shevchenko
2025-08-27 13:59 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add NXP P3T175x support David Lechner
2025-08-31 16:01 ` Jonathan Cameron
2025-08-31 16:47 ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).