All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030
@ 2026-01-15 11:14 Mayank Mahajan
  2026-01-15 11:14 ` [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030 Mayank Mahajan
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Mayank Mahajan @ 2026-01-15 11:14 UTC (permalink / raw)
  To: linux, corbet, robh, krzk+dt, conor+dt, linux-hwmon, devicetree,
	linux-doc, linux-kernel
  Cc: priyanka.jain, vikash.bansal, Mayank Mahajan

Document the NXP P3T1035 and P3T2030 compatibles in TMP108.

Signed-off-by: Mayank Mahajan <mayankmahajan.x@nxp.com>
---
V1 -> V2:
- No changes in v2.
V2 -> V3:
- Add P3T1035 fallback for P3T2030 as they are functionally identical.
- Add comment in the description explaining the use of P3T2030.

 .../devicetree/bindings/hwmon/ti,tmp108.yaml  | 24 ++++++++++++-------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
index a6f9319e068d..1f540c623de6 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
@@ -4,27 +4,35 @@
 $id: http://devicetree.org/schemas/hwmon/ti,tmp108.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#

-title: TMP108/P3T1085(NXP) temperature sensor
+title: TMP108/P3T1035/P3T1085/P3T2030 temperature sensor

 maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>

 description: |
-  The TMP108/P3T1085(NXP) is a digital-output temperature sensor with a
-  dynamically-programmable limit window, and under- and overtemperature
-  alert functions.
+  The TMP108 or NXP P3T Family (P3T1035, P3T1085 and P3T2030) is a digital-
+  output temperature sensor with a dynamically-programmable limit window,
+  and under- and over-temperature alert functions.

-  P3T1085(NXP) support I3C.
+  NXP P3T Family (P3T1035, P3T1085 and P3T2030) supports I3C.

   Datasheets:
     https://www.ti.com/product/TMP108
     https://www.nxp.com/docs/en/data-sheet/P3T1085UK.pdf
+    https://www.nxp.com/docs/en/data-sheet/P3T1035XUK_P3T2030XUK.pdf
+
+  P3T2030 is functionally identical to P3T1035. Hence, device tree nodes that
+  use the P3T2030 must provide a fallback compatible string of "nxp,p3t1035"

 properties:
   compatible:
-    enum:
-      - nxp,p3t1085
-      - ti,tmp108
+    oneOf:
+      - items:
+          - const: nxp,p3t2030
+          - const: nxp,p3t1035
+      - const: nxp,p3t1035
+      - const: nxp,p3t1085
+      - const: ti,tmp108

   interrupts:
     items:
--
2.34.1

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

* [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030
  2026-01-15 11:14 [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030 Mayank Mahajan
@ 2026-01-15 11:14 ` Mayank Mahajan
  2026-01-16  4:31   ` kernel test robot
  2026-01-16  6:11   ` Guenter Roeck
  2026-01-15 11:14 ` [PATCH v3 3/3] hwmon: (tmp108) Add P3T1035 and P3T2030 support Mayank Mahajan
  2026-01-16  7:22 ` [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030 Krzysztof Kozlowski
  2 siblings, 2 replies; 6+ messages in thread
From: Mayank Mahajan @ 2026-01-15 11:14 UTC (permalink / raw)
  To: linux, corbet, robh, krzk+dt, conor+dt, linux-hwmon, devicetree,
	linux-doc, linux-kernel
  Cc: priyanka.jain, vikash.bansal, Mayank Mahajan

Add support for the P3T1035 & P3T2030 temperature sensor. While mostly
compatible with the TMP108, P3T1035 uses an 8-bit configuration register
instead of the 16-bit layout used by TMP108. Updated driver to handle
this difference during configuration read/write.

Signed-off-by: Mayank Mahajan <mayankmahajan.x@nxp.com>
---
V1 -> V2:
- Disabled hysteresis in is_visible function for P3T1035.
- Added tables for conversion rate similar to the LM75 driver.
- Implemented different bus access depending on the chip being used.
  - Removed regmap for 8 bits; now we are using one regmap as before.
  - Added read and write functions for i2c and i3c for use with regmap.
  - Mapped the 8-bit configuration register to a 16 bit value for P3T1035.
V2 -> V3:
- Remove changes not relevant to adding a new device in the driver.
- Address warnings due to incorrect usage of casting operations.
- Remove the usage of P3T2030 as it's functionally identical to P3T1035.

 drivers/hwmon/Kconfig  |   2 +-
 drivers/hwmon/tmp108.c | 227 +++++++++++++++++++++++++++++++++--------
 2 files changed, 186 insertions(+), 43 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 157678b821fc..31969bddc812 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2398,7 +2398,7 @@ config SENSORS_TMP108
 	select REGMAP_I3C if I3C
 	help
 	  If you say yes here you get support for Texas Instruments TMP108
-	  sensor chips and NXP P3T1085.
+	  sensor chips, NXP temperature sensors P3T1035, P3T1085 and P3T2030.

 	  This driver can also be built as a module. If so, the module
 	  will be called tmp108.
diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
index 60a237cbedbc..38a2203c3bd9 100644
--- a/drivers/hwmon/tmp108.c
+++ b/drivers/hwmon/tmp108.c
@@ -17,9 +17,16 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/util_macros.h>

 #define	DRIVER_NAME "tmp108"

+enum tmp108_hw_id {
+	P3T1035_ID,		/* For sensors p3t1035 and p3t2030 */
+	P3T1085_ID,
+	TMP108_ID,
+};
+
 #define	TMP108_REG_TEMP		0x00
 #define	TMP108_REG_CONF		0x01
 #define	TMP108_REG_TLOW		0x02
@@ -61,6 +68,7 @@
 #define TMP108_CONVRATE_1HZ		TMP108_CONF_CR0		/* Default */
 #define TMP108_CONVRATE_4HZ		TMP108_CONF_CR1
 #define TMP108_CONVRATE_16HZ		(TMP108_CONF_CR0|TMP108_CONF_CR1)
+#define TMP108_CONVRATE_SHIFT		13

 #define TMP108_CONF_HYSTERESIS_MASK	(TMP108_CONF_HYS0|TMP108_CONF_HYS1)
 #define TMP108_HYSTERESIS_0C		0x0000
@@ -71,11 +79,21 @@
 #define TMP108_CONVERSION_TIME_MS	30	/* in milli-seconds */

 struct tmp108 {
-	struct regmap *regmap;
-	u16 orig_config;
-	unsigned long ready_time;
+	struct regmap		*regmap;
+	u16			orig_config;
+	unsigned long		ready_time;
+	enum tmp108_hw_id	hw_id;
+	bool			config_reg_16bits;
+	u8			reg_buf[1];
+	u8			val_buf[3];
+	unsigned int		sample_times[4];
 };

+static const u16 tmp108_sample_set_masks[] = { 3 << TMP108_CONVRATE_SHIFT,
+					       2 << TMP108_CONVRATE_SHIFT,
+					       1 << TMP108_CONVRATE_SHIFT,
+					       0 << TMP108_CONVRATE_SHIFT };
+
 /* convert 12-bit TMP108 register value to milliCelsius */
 static inline int tmp108_temp_reg_to_mC(s16 val)
 {
@@ -94,6 +112,8 @@ static int tmp108_read(struct device *dev, enum hwmon_sensor_types type,
 	struct tmp108 *tmp108 = dev_get_drvdata(dev);
 	unsigned int regval;
 	int err, hyst;
+	u16 conv_rate;
+	u8 index;

 	if (type == hwmon_chip) {
 		if (attr == hwmon_chip_update_interval) {
@@ -101,21 +121,10 @@ static int tmp108_read(struct device *dev, enum hwmon_sensor_types type,
 					  &regval);
 			if (err < 0)
 				return err;
-			switch (regval & TMP108_CONF_CONVRATE_MASK) {
-			case TMP108_CONVRATE_0P25HZ:
-			default:
-				*temp = 4000;
-				break;
-			case TMP108_CONVRATE_1HZ:
-				*temp = 1000;
-				break;
-			case TMP108_CONVRATE_4HZ:
-				*temp = 250;
-				break;
-			case TMP108_CONVRATE_16HZ:
-				*temp = 63;
-				break;
-			}
+			conv_rate = regval & TMP108_CONF_CONVRATE_MASK;
+			index = find_closest_descending(conv_rate, tmp108_sample_set_masks,
+							(int)ARRAY_SIZE(tmp108_sample_set_masks));
+			*temp = tmp108->sample_times[index];
 			return 0;
 		}
 		return -EOPNOTSUPP;
@@ -192,22 +201,17 @@ static int tmp108_write(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct tmp108 *tmp108 = dev_get_drvdata(dev);
 	u32 regval, mask;
+	u8 index;
 	int err;

 	if (type == hwmon_chip) {
 		if (attr == hwmon_chip_update_interval) {
-			if (temp < 156)
-				mask = TMP108_CONVRATE_16HZ;
-			else if (temp < 625)
-				mask = TMP108_CONVRATE_4HZ;
-			else if (temp < 2500)
-				mask = TMP108_CONVRATE_1HZ;
-			else
-				mask = TMP108_CONVRATE_0P25HZ;
+			index = find_closest(temp, tmp108->sample_times,
+					     (int)ARRAY_SIZE(tmp108->sample_times));
 			return regmap_update_bits(tmp108->regmap,
 						  TMP108_REG_CONF,
 						  TMP108_CONF_CONVRATE_MASK,
-						  mask);
+						  tmp108_sample_set_masks[index]);
 		}
 		return -EOPNOTSUPP;
 	}
@@ -251,6 +255,8 @@ static int tmp108_write(struct device *dev, enum hwmon_sensor_types type,
 static umode_t tmp108_is_visible(const void *data, enum hwmon_sensor_types type,
 				 u32 attr, int channel)
 {
+	const struct tmp108 *tmp108 = data;
+
 	if (type == hwmon_chip && attr == hwmon_chip_update_interval)
 		return 0644;

@@ -264,8 +270,11 @@ static umode_t tmp108_is_visible(const void *data, enum hwmon_sensor_types type,
 		return 0444;
 	case hwmon_temp_min:
 	case hwmon_temp_max:
+		return 0644;
 	case hwmon_temp_min_hyst:
 	case hwmon_temp_max_hyst:
+		if (tmp108->hw_id == P3T1035_ID)
+			return 0;
 		return 0644;
 	default:
 		return 0;
@@ -311,6 +320,105 @@ static bool tmp108_is_volatile_reg(struct device *dev, unsigned int reg)
 	return reg == TMP108_REG_TEMP || reg == TMP108_REG_CONF;
 }

+static int tmp108_i2c_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct i2c_client *client = context;
+	struct tmp108 *tmp108 = i2c_get_clientdata(client);
+	int ret;
+
+	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
+		ret = i2c_smbus_read_byte_data(client, TMP108_REG_CONF);
+	else
+		ret = i2c_smbus_read_word_swapped(client, reg);
+
+	if (ret < 0)
+		return ret;
+
+	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
+		*val = ret << 8;
+	else
+		*val = ret;
+
+	return 0;
+}
+
+static int tmp108_i2c_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct i2c_client *client = context;
+	struct tmp108 *tmp108 = i2c_get_clientdata(client);
+
+	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
+		return i2c_smbus_write_byte_data(client, reg, val >> 8);
+	return i2c_smbus_write_word_swapped(client, reg, val);
+}
+
+static const struct regmap_bus tmp108_i2c_regmap_bus = {
+	.reg_read = tmp108_i2c_reg_read,
+	.reg_write = tmp108_i2c_reg_write,
+};
+
+static int tmp108_i3c_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct i3c_device *i3cdev = context;
+	struct tmp108 *tmp108 = i3cdev_get_drvdata(i3cdev);
+	struct i3c_xfer xfers[] = {
+		{
+			.rnw = false,
+			.len = 1,
+			.data.out = tmp108->reg_buf,
+		},
+		{
+			.rnw = true,
+			.len = 2,
+			.data.in = tmp108->val_buf,
+		},
+	};
+	int ret;
+
+	tmp108->reg_buf[0] = reg;
+
+	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
+		xfers[1].len--;
+
+	ret = i3c_device_do_xfers(i3cdev, xfers, 2, I3C_SDR);
+	if (ret < 0)
+		return ret;
+
+	*val = tmp108->val_buf[0] << 8;
+	if (!(reg == TMP108_REG_CONF && !tmp108->config_reg_16bits))
+		*val |= tmp108->val_buf[1];
+
+	return 0;
+}
+
+static int tmp108_i3c_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct i3c_device *i3cdev = context;
+	struct tmp108 *tmp108 = i3cdev_get_drvdata(i3cdev);
+	struct i3c_xfer xfers[] = {
+		{
+			.rnw = false,
+			.len = 3,
+			.data.out = tmp108->val_buf,
+		},
+	};
+
+	tmp108->val_buf[0] = reg;
+	tmp108->val_buf[1] = (val >> 8) & 0xff;
+
+	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
+		xfers[0].len--;
+	else
+		tmp108->val_buf[2] = val & 0xff;
+
+	return i3c_device_do_xfers(i3cdev, xfers, 1, I3C_SDR);
+}
+
+static const struct regmap_bus tmp108_i3c_regmap_bus = {
+	.reg_read = tmp108_i3c_reg_read,
+	.reg_write = tmp108_i3c_reg_write,
+};
+
 static const struct regmap_config tmp108_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
@@ -323,7 +431,8 @@ static const struct regmap_config tmp108_regmap_config = {
 	.use_single_write = true,
 };

-static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name)
+static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name,
+			       enum tmp108_hw_id hw_id)
 {
 	struct device *hwmon_dev;
 	struct tmp108 *tmp108;
@@ -340,6 +449,14 @@ static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *

 	dev_set_drvdata(dev, tmp108);
 	tmp108->regmap = regmap;
+	tmp108->hw_id = hw_id;
+	tmp108->config_reg_16bits = (hw_id == P3T1035_ID) ? false : true;
+	if (hw_id == P3T1035_ID)
+		memcpy(tmp108->sample_times, (unsigned int[]){ 125, 250, 1000, 4000 },
+		       sizeof(tmp108->sample_times));
+	else
+		memcpy(tmp108->sample_times, (unsigned int[]){ 63, 250, 1000, 4000 },
+		       sizeof(tmp108->sample_times));

 	err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &config);
 	if (err < 0) {
@@ -351,7 +468,6 @@ static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *
 	/* Only continuous mode is supported. */
 	config &= ~TMP108_CONF_MODE_MASK;
 	config |= TMP108_MODE_CONTINUOUS;
-
 	/* Only comparator mode is supported. */
 	config &= ~TMP108_CONF_TM;

@@ -384,17 +500,33 @@ static int tmp108_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct regmap *regmap;
+	enum tmp108_hw_id hw_id;
+	const void *of_data;

 	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_WORD_DATA))
+				     I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
 		return dev_err_probe(dev, -ENODEV,
 				     "adapter doesn't support SMBus word transactions\n");

-	regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config);
+	regmap = devm_regmap_init(dev, &tmp108_i2c_regmap_bus, client, &tmp108_regmap_config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed");

-	return tmp108_common_probe(dev, regmap, client->name);
+	/* Prefer OF match data (DT-first systems) */
+	of_data = device_get_match_data(&client->dev);
+	if (of_data) {
+		hw_id = (unsigned long)of_data;
+	} else {
+		/* Fall back to legacy I2C ID table */
+		const struct i2c_device_id *id = i2c_client_get_device_id(client);
+
+		if (!id) {
+			return dev_err_probe(dev, -ENODEV, "No matching device ID for i2c device\n");
+		}
+		hw_id = (unsigned long)id->driver_data;
+	}
+
+	return tmp108_common_probe(dev, regmap, client->name, hw_id);
 }

 static int tmp108_suspend(struct device *dev)
@@ -420,16 +552,18 @@ static int tmp108_resume(struct device *dev)
 static DEFINE_SIMPLE_DEV_PM_OPS(tmp108_dev_pm_ops, tmp108_suspend, tmp108_resume);

 static const struct i2c_device_id tmp108_i2c_ids[] = {
-	{ "p3t1085" },
-	{ "tmp108" },
-	{ }
+	{ "p3t1035", P3T1035_ID },
+	{ "p3t1085", P3T1085_ID },
+	{ "tmp108", TMP108_ID },
+	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(i2c, tmp108_i2c_ids);

 static const struct of_device_id tmp108_of_ids[] = {
-	{ .compatible = "nxp,p3t1085", },
-	{ .compatible = "ti,tmp108", },
-	{}
+	{ .compatible = "nxp,p3t1035", .data = (void *)(uintptr_t)P3T1035_ID },
+	{ .compatible = "nxp,p3t1085", .data = (void *)(uintptr_t)P3T1085_ID },
+	{ .compatible = "ti,tmp108", .data = (void *)(uintptr_t)TMP108_ID },
+	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, tmp108_of_ids);

@@ -444,8 +578,9 @@ static struct i2c_driver tmp108_driver = {
 };

 static const struct i3c_device_id p3t1085_i3c_ids[] = {
-	I3C_DEVICE(0x011b, 0x1529, NULL),
-	{}
+	I3C_DEVICE(0x011B, 0x1529, (void *)P3T1085_ID),
+	I3C_DEVICE(0x011B, 0x152B, (void *)P3T1035_ID),
+	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(i3c, p3t1085_i3c_ids);

@@ -453,13 +588,21 @@ static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
 {
 	struct device *dev = i3cdev_to_dev(i3cdev);
 	struct regmap *regmap;
+	const struct i3c_device_id *id;
+	enum tmp108_hw_id hw_id;

-	regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
+	regmap = devm_regmap_init(dev, &tmp108_i3c_regmap_bus, i3cdev, &tmp108_regmap_config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap),
 				     "Failed to register i3c regmap\n");

-	return tmp108_common_probe(dev, regmap, "p3t1085_i3c");
+	id = i3c_device_match_id(i3cdev, p3t1085_i3c_ids);
+	if (!id) {
+		return dev_err_probe(dev, -ENODEV, "No matching device ID for i3c device\n");
+	}
+	hw_id = (enum tmp108_hw_id)(uintptr_t)id->data;
+
+	return tmp108_common_probe(dev, regmap, "p3t1085_i3c", hw_id);
 }

 static struct i3c_driver p3t1085_driver = {
--
2.34.1

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

* [PATCH v3 3/3] hwmon: (tmp108) Add P3T1035 and P3T2030 support
  2026-01-15 11:14 [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030 Mayank Mahajan
  2026-01-15 11:14 ` [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030 Mayank Mahajan
@ 2026-01-15 11:14 ` Mayank Mahajan
  2026-01-16  7:22 ` [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030 Krzysztof Kozlowski
  2 siblings, 0 replies; 6+ messages in thread
From: Mayank Mahajan @ 2026-01-15 11:14 UTC (permalink / raw)
  To: linux, corbet, robh, krzk+dt, conor+dt, linux-hwmon, devicetree,
	linux-doc, linux-kernel
  Cc: priyanka.jain, vikash.bansal, Mayank Mahajan

Update the hwmon driver documentation for sensors: P3T1035 and P3T2030.

Signed-off-by: Mayank Mahajan <mayankmahajan.x@nxp.com>
---
V1 -> V2:
- No changes in v2.
V2 -> V3:
- No changes in v3.

 Documentation/hwmon/tmp108.rst | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/hwmon/tmp108.rst b/Documentation/hwmon/tmp108.rst
index bc4941d98268..c218ea333dd6 100644
--- a/Documentation/hwmon/tmp108.rst
+++ b/Documentation/hwmon/tmp108.rst
@@ -3,6 +3,15 @@ Kernel driver tmp108

 Supported chips:

+  * NXP P3T1035
+
+    Prefix: 'p3t1035'
+
+    Addresses scanned: none
+
+    Datasheet: https://www.nxp.com/docs/en/data-sheet/P3T1035XUK_P3T2030XUK.pdf
+
+
   * NXP P3T1085

     Prefix: 'p3t1085'
@@ -11,6 +20,14 @@ Supported chips:

     Datasheet: https://www.nxp.com/docs/en/data-sheet/P3T1085UK.pdf

+  * NXP P3T2030
+
+    Prefix: 'p3t2030'
+
+    Addresses scanned: none
+
+    Datasheet: https://www.nxp.com/docs/en/data-sheet/P3T1035XUK_P3T2030XUK.pdf
+
   * Texas Instruments TMP108

     Prefix: 'tmp108'
--
2.34.1

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

* Re: [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030
  2026-01-15 11:14 ` [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030 Mayank Mahajan
@ 2026-01-16  4:31   ` kernel test robot
  2026-01-16  6:11   ` Guenter Roeck
  1 sibling, 0 replies; 6+ messages in thread
From: kernel test robot @ 2026-01-16  4:31 UTC (permalink / raw)
  To: Mayank Mahajan, linux, corbet, robh, krzk+dt, conor+dt,
	linux-hwmon, devicetree, linux-doc, linux-kernel
  Cc: oe-kbuild-all, priyanka.jain, vikash.bansal, Mayank Mahajan

Hi Mayank,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.19-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mayank-Mahajan/hwmon-tmp108-Add-support-for-P3T1035-and-P3T2030/20260115-191549
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20260115111418.1851-2-mayankmahajan.x%40nxp.com
patch subject: [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030
config: xtensa-randconfig-r122-20260116 (https://download.01.org/0day-ci/archive/20260116/202601161234.jWOgBbs8-lkp@intel.com/config)
compiler: xtensa-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260116/202601161234.jWOgBbs8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601161234.jWOgBbs8-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   drivers/hwmon/tmp108.c: note: in included file (through arch/xtensa/include/asm/bitops.h, include/linux/bitops.h, include/linux/log2.h, ...):
   arch/xtensa/include/asm/processor.h:105:2: sparse: sparse: Unsupported xtensa ABI
   arch/xtensa/include/asm/processor.h:135:2: sparse: sparse: Unsupported Xtensa ABI
>> drivers/hwmon/tmp108.c:455:17: sparse: sparse: macro "memcpy" passed 6 arguments, but takes just 3
   drivers/hwmon/tmp108.c:458:17: sparse: sparse: macro "memcpy" passed 6 arguments, but takes just 3
   drivers/hwmon/tmp108.c:618:1: sparse: sparse: bad integer constant expression
   drivers/hwmon/tmp108.c:618:1: sparse: sparse: static assertion failed: "MODULE_INFO(author, ...) contains embedded NUL byte"
   drivers/hwmon/tmp108.c:619:1: sparse: sparse: bad integer constant expression
   drivers/hwmon/tmp108.c:619:1: sparse: sparse: static assertion failed: "MODULE_INFO(description, ...) contains embedded NUL byte"
   drivers/hwmon/tmp108.c:620:1: sparse: sparse: bad integer constant expression
   drivers/hwmon/tmp108.c:620:1: sparse: sparse: static assertion failed: "MODULE_INFO(license, ...) contains embedded NUL byte"

vim +/memcpy +455 drivers/hwmon/tmp108.c

   433	
   434	static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name,
   435				       enum tmp108_hw_id hw_id)
   436	{
   437		struct device *hwmon_dev;
   438		struct tmp108 *tmp108;
   439		u32 config;
   440		int err;
   441	
   442		err = devm_regulator_get_enable(dev, "vcc");
   443		if (err)
   444			return dev_err_probe(dev, err, "Failed to enable regulator\n");
   445	
   446		tmp108 = devm_kzalloc(dev, sizeof(*tmp108), GFP_KERNEL);
   447		if (!tmp108)
   448			return -ENOMEM;
   449	
   450		dev_set_drvdata(dev, tmp108);
   451		tmp108->regmap = regmap;
   452		tmp108->hw_id = hw_id;
   453		tmp108->config_reg_16bits = (hw_id == P3T1035_ID) ? false : true;
   454		if (hw_id == P3T1035_ID)
 > 455			memcpy(tmp108->sample_times, (unsigned int[]){ 125, 250, 1000, 4000 },
   456			       sizeof(tmp108->sample_times));
   457		else
   458			memcpy(tmp108->sample_times, (unsigned int[]){ 63, 250, 1000, 4000 },
   459			       sizeof(tmp108->sample_times));
   460	
   461		err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &config);
   462		if (err < 0) {
   463			dev_err(dev, "error reading config register: %d", err);
   464			return err;
   465		}
   466		tmp108->orig_config = config;
   467	
   468		/* Only continuous mode is supported. */
   469		config &= ~TMP108_CONF_MODE_MASK;
   470		config |= TMP108_MODE_CONTINUOUS;
   471		/* Only comparator mode is supported. */
   472		config &= ~TMP108_CONF_TM;
   473	
   474		err = regmap_write(tmp108->regmap, TMP108_REG_CONF, config);
   475		if (err < 0) {
   476			dev_err(dev, "error writing config register: %d", err);
   477			return err;
   478		}
   479	
   480		tmp108->ready_time = jiffies;
   481		if ((tmp108->orig_config & TMP108_CONF_MODE_MASK) ==
   482		    TMP108_MODE_SHUTDOWN)
   483			tmp108->ready_time +=
   484				msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
   485	
   486		err = devm_add_action_or_reset(dev, tmp108_restore_config, tmp108);
   487		if (err) {
   488			dev_err(dev, "add action or reset failed: %d", err);
   489			return err;
   490		}
   491	
   492		hwmon_dev = devm_hwmon_device_register_with_info(dev, name,
   493								 tmp108,
   494								 &tmp108_chip_info,
   495								 NULL);
   496		return PTR_ERR_OR_ZERO(hwmon_dev);
   497	}
   498	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

* Re: [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030
  2026-01-15 11:14 ` [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030 Mayank Mahajan
  2026-01-16  4:31   ` kernel test robot
@ 2026-01-16  6:11   ` Guenter Roeck
  1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2026-01-16  6:11 UTC (permalink / raw)
  To: Mayank Mahajan, corbet, robh, krzk+dt, conor+dt, linux-hwmon,
	devicetree, linux-doc, linux-kernel
  Cc: priyanka.jain, vikash.bansal

On 1/15/26 03:14, Mayank Mahajan wrote:
> Add support for the P3T1035 & P3T2030 temperature sensor. While mostly
> compatible with the TMP108, P3T1035 uses an 8-bit configuration register
> instead of the 16-bit layout used by TMP108. Updated driver to handle
> this difference during configuration read/write.
> 
> Signed-off-by: Mayank Mahajan <mayankmahajan.x@nxp.com>
> ---
> V1 -> V2:
> - Disabled hysteresis in is_visible function for P3T1035.
> - Added tables for conversion rate similar to the LM75 driver.
> - Implemented different bus access depending on the chip being used.
>    - Removed regmap for 8 bits; now we are using one regmap as before.
>    - Added read and write functions for i2c and i3c for use with regmap.
>    - Mapped the 8-bit configuration register to a 16 bit value for P3T1035.
> V2 -> V3:
> - Remove changes not relevant to adding a new device in the driver.
> - Address warnings due to incorrect usage of casting operations.
> - Remove the usage of P3T2030 as it's functionally identical to P3T1035.
> 
>   drivers/hwmon/Kconfig  |   2 +-
>   drivers/hwmon/tmp108.c | 227 +++++++++++++++++++++++++++++++++--------
>   2 files changed, 186 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 157678b821fc..31969bddc812 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2398,7 +2398,7 @@ config SENSORS_TMP108
>   	select REGMAP_I3C if I3C
>   	help
>   	  If you say yes here you get support for Texas Instruments TMP108
> -	  sensor chips and NXP P3T1085.
> +	  sensor chips, NXP temperature sensors P3T1035, P3T1085 and P3T2030.
> 
>   	  This driver can also be built as a module. If so, the module
>   	  will be called tmp108.
> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> index 60a237cbedbc..38a2203c3bd9 100644
> --- a/drivers/hwmon/tmp108.c
> +++ b/drivers/hwmon/tmp108.c
> @@ -17,9 +17,16 @@
>   #include <linux/regmap.h>
>   #include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
> +#include <linux/util_macros.h>
> 
>   #define	DRIVER_NAME "tmp108"
> 
> +enum tmp108_hw_id {
> +	P3T1035_ID,		/* For sensors p3t1035 and p3t2030 */
> +	P3T1085_ID,
> +	TMP108_ID,
> +};
> +
>   #define	TMP108_REG_TEMP		0x00
>   #define	TMP108_REG_CONF		0x01
>   #define	TMP108_REG_TLOW		0x02
> @@ -61,6 +68,7 @@
>   #define TMP108_CONVRATE_1HZ		TMP108_CONF_CR0		/* Default */
>   #define TMP108_CONVRATE_4HZ		TMP108_CONF_CR1
>   #define TMP108_CONVRATE_16HZ		(TMP108_CONF_CR0|TMP108_CONF_CR1)
> +#define TMP108_CONVRATE_SHIFT		13
> 
>   #define TMP108_CONF_HYSTERESIS_MASK	(TMP108_CONF_HYS0|TMP108_CONF_HYS1)
>   #define TMP108_HYSTERESIS_0C		0x0000
> @@ -71,11 +79,21 @@
>   #define TMP108_CONVERSION_TIME_MS	30	/* in milli-seconds */
> 
>   struct tmp108 {
> -	struct regmap *regmap;
> -	u16 orig_config;
> -	unsigned long ready_time;
> +	struct regmap		*regmap;
> +	u16			orig_config;
> +	unsigned long		ready_time;
> +	enum tmp108_hw_id	hw_id;
> +	bool			config_reg_16bits;
> +	u8			reg_buf[1];
> +	u8			val_buf[3];
> +	unsigned int		sample_times[4];
>   };
> 
> +static const u16 tmp108_sample_set_masks[] = { 3 << TMP108_CONVRATE_SHIFT,
> +					       2 << TMP108_CONVRATE_SHIFT,
> +					       1 << TMP108_CONVRATE_SHIFT,
> +					       0 << TMP108_CONVRATE_SHIFT };
> +

Unnecessary. See below.

>   /* convert 12-bit TMP108 register value to milliCelsius */
>   static inline int tmp108_temp_reg_to_mC(s16 val)
>   {
> @@ -94,6 +112,8 @@ static int tmp108_read(struct device *dev, enum hwmon_sensor_types type,
>   	struct tmp108 *tmp108 = dev_get_drvdata(dev);
>   	unsigned int regval;
>   	int err, hyst;
> +	u16 conv_rate;
> +	u8 index;
> 
>   	if (type == hwmon_chip) {
>   		if (attr == hwmon_chip_update_interval) {
> @@ -101,21 +121,10 @@ static int tmp108_read(struct device *dev, enum hwmon_sensor_types type,
>   					  &regval);
>   			if (err < 0)
>   				return err;
> -			switch (regval & TMP108_CONF_CONVRATE_MASK) {
> -			case TMP108_CONVRATE_0P25HZ:
> -			default:
> -				*temp = 4000;
> -				break;
> -			case TMP108_CONVRATE_1HZ:
> -				*temp = 1000;
> -				break;
> -			case TMP108_CONVRATE_4HZ:
> -				*temp = 250;
> -				break;
> -			case TMP108_CONVRATE_16HZ:
> -				*temp = 63;
> -				break;
> -			}
> +			conv_rate = regval & TMP108_CONF_CONVRATE_MASK;
> +			index = find_closest_descending(conv_rate, tmp108_sample_set_masks,
> +							(int)ARRAY_SIZE(tmp108_sample_set_masks));
> +			*temp = tmp108->sample_times[index];

(regval & TMP108_CONF_CONVRATE_MASK) >> TMP108_CONVRATE_SHIFT, or alternatively
FIELD_GET(TMP108_CONF_CONVRATE_MASK, regval), yields 0..3. With a sample_times
array of { 4000, 1000, 250, 63 } or { 4000, 1000, 250, 125 }, the code above
could simply be
			*temp = tmp108->sample_times[FIELD_GET(TMP108_CONF_CONVRATE_MASK, regval)];
which would both be easier to understand and much simpler.

>   			return 0;
>   		}
>   		return -EOPNOTSUPP;
> @@ -192,22 +201,17 @@ static int tmp108_write(struct device *dev, enum hwmon_sensor_types type,
>   {
>   	struct tmp108 *tmp108 = dev_get_drvdata(dev);
>   	u32 regval, mask;
> +	u8 index;
>   	int err;
> 
>   	if (type == hwmon_chip) {
>   		if (attr == hwmon_chip_update_interval) {
> -			if (temp < 156)
> -				mask = TMP108_CONVRATE_16HZ;
> -			else if (temp < 625)
> -				mask = TMP108_CONVRATE_4HZ;
> -			else if (temp < 2500)
> -				mask = TMP108_CONVRATE_1HZ;
> -			else
> -				mask = TMP108_CONVRATE_0P25HZ;
> +			index = find_closest(temp, tmp108->sample_times,
> +					     (int)ARRAY_SIZE(tmp108->sample_times));

I don't see why the type cast would be needed. Other users of find_closest()
don't need it either.

>   			return regmap_update_bits(tmp108->regmap,
>   						  TMP108_REG_CONF,
>   						  TMP108_CONF_CONVRATE_MASK,
> -						  mask);
> +						  tmp108_sample_set_masks[index]);

Use GENMASK().

>   		}
>   		return -EOPNOTSUPP;
>   	}
> @@ -251,6 +255,8 @@ static int tmp108_write(struct device *dev, enum hwmon_sensor_types type,
>   static umode_t tmp108_is_visible(const void *data, enum hwmon_sensor_types type,
>   				 u32 attr, int channel)
>   {
> +	const struct tmp108 *tmp108 = data;
> +
>   	if (type == hwmon_chip && attr == hwmon_chip_update_interval)
>   		return 0644;
> 
> @@ -264,8 +270,11 @@ static umode_t tmp108_is_visible(const void *data, enum hwmon_sensor_types type,
>   		return 0444;
>   	case hwmon_temp_min:
>   	case hwmon_temp_max:
> +		return 0644;
>   	case hwmon_temp_min_hyst:
>   	case hwmon_temp_max_hyst:
> +		if (tmp108->hw_id == P3T1035_ID)
> +			return 0;
>   		return 0644;
>   	default:
>   		return 0;
> @@ -311,6 +320,105 @@ static bool tmp108_is_volatile_reg(struct device *dev, unsigned int reg)
>   	return reg == TMP108_REG_TEMP || reg == TMP108_REG_CONF;
>   }
> 
> +static int tmp108_i2c_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct i2c_client *client = context;
> +	struct tmp108 *tmp108 = i2c_get_clientdata(client);
> +	int ret;
> +
> +	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
> +		ret = i2c_smbus_read_byte_data(client, TMP108_REG_CONF);
> +	else
> +		ret = i2c_smbus_read_word_swapped(client, reg);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
> +		*val = ret << 8;
> +	else
> +		*val = ret;

This evaluates reg and tmp108->config_reg_16bits twice. Try

	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits) {
		ret = i2c_smbus_read_byte_data(client, TMP108_REG_CONF);
		if (ret < 0)
			return ret;
		*val = ret << 8;
		return 0;
	}
	ret = i2c_smbus_read_word_swapped(client, reg);
	if (ret < 0)
		return ret;
	*val = ret;
	return 0;

instead to reduce runtime overhead.

> +
> +	return 0;
> +}
> +
> +static int tmp108_i2c_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct i2c_client *client = context;
> +	struct tmp108 *tmp108 = i2c_get_clientdata(client);
> +
> +	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
> +		return i2c_smbus_write_byte_data(client, reg, val >> 8);
> +	return i2c_smbus_write_word_swapped(client, reg, val);
> +}
> +
> +static const struct regmap_bus tmp108_i2c_regmap_bus = {
> +	.reg_read = tmp108_i2c_reg_read,
> +	.reg_write = tmp108_i2c_reg_write,
> +};
> +
> +static int tmp108_i3c_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct i3c_device *i3cdev = context;
> +	struct tmp108 *tmp108 = i3cdev_get_drvdata(i3cdev);
> +	struct i3c_xfer xfers[] = {
> +		{
> +			.rnw = false,
> +			.len = 1,
> +			.data.out = tmp108->reg_buf,
> +		},
> +		{
> +			.rnw = true,
> +			.len = 2,
> +			.data.in = tmp108->val_buf,

What is the point of having reg_buf and val_buf allocated instead
of just using local variables/arrays ?

> +		},
> +	};
> +	int ret;
> +
> +	tmp108->reg_buf[0] = reg;
> +
> +	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
> +		xfers[1].len--;
> +
> +	ret = i3c_device_do_xfers(i3cdev, xfers, 2, I3C_SDR);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = tmp108->val_buf[0] << 8;
> +	if (!(reg == TMP108_REG_CONF && !tmp108->config_reg_16bits))

Please refrain from using double negations.
	if (reg != TMP108_REG_CONF || tmp108->config_reg_16bits)
is much easier to understand.

> +		*val |= tmp108->val_buf[1];
> +
> +	return 0;
> +}
> +
> +static int tmp108_i3c_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct i3c_device *i3cdev = context;
> +	struct tmp108 *tmp108 = i3cdev_get_drvdata(i3cdev);
> +	struct i3c_xfer xfers[] = {
> +		{
> +			.rnw = false,
> +			.len = 3,
> +			.data.out = tmp108->val_buf,
> +		},
> +	};
> +
> +	tmp108->val_buf[0] = reg;
> +	tmp108->val_buf[1] = (val >> 8) & 0xff;
> +
> +	if (reg == TMP108_REG_CONF && !tmp108->config_reg_16bits)
> +		xfers[0].len--;
> +	else
> +		tmp108->val_buf[2] = val & 0xff;
> +
> +	return i3c_device_do_xfers(i3cdev, xfers, 1, I3C_SDR);
> +}
> +
> +static const struct regmap_bus tmp108_i3c_regmap_bus = {
> +	.reg_read = tmp108_i3c_reg_read,
> +	.reg_write = tmp108_i3c_reg_write,
> +};
> +
>   static const struct regmap_config tmp108_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 16,
> @@ -323,7 +431,8 @@ static const struct regmap_config tmp108_regmap_config = {
>   	.use_single_write = true,
>   };
> 
> -static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name)
> +static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name,
> +			       enum tmp108_hw_id hw_id)
>   {
>   	struct device *hwmon_dev;
>   	struct tmp108 *tmp108;
> @@ -340,6 +449,14 @@ static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *
> 
>   	dev_set_drvdata(dev, tmp108);
>   	tmp108->regmap = regmap;
> +	tmp108->hw_id = hw_id;
> +	tmp108->config_reg_16bits = (hw_id == P3T1035_ID) ? false : true;
> +	if (hw_id == P3T1035_ID)
> +		memcpy(tmp108->sample_times, (unsigned int[]){ 125, 250, 1000, 4000 },
> +		       sizeof(tmp108->sample_times));
> +	else
> +		memcpy(tmp108->sample_times, (unsigned int[]){ 63, 250, 1000, 4000 },
> +		       sizeof(tmp108->sample_times));

You'd think that the repeated 0-day complaints have an effect.
Just make tmp108->sample_times a pointer and create two ushort arrays where the values
match the index values.

	struct tmp108 {
		ushort *sample_times;
	};

	ushort p3t_1035_sample_times[] = {4000, 1000, 250, 125};
	ushort tmp108_sample_times[] = {4000, 1000, 250, 125};

	if (hw_id == P3T1035_ID)
		tmp108->sample_times = p3t_1035_sample_times;
	else
		tmp108->sample_times = tmp108_sample_times;

Something like
	tmp108->sample_times = (ushort []) {4000, 1000, 250, 125};
might work as well, but I did not test it.
		
The memcpy is really unnecessary here.

> 
>   	err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &config);
>   	if (err < 0) {
> @@ -351,7 +468,6 @@ static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *
>   	/* Only continuous mode is supported. */
>   	config &= ~TMP108_CONF_MODE_MASK;
>   	config |= TMP108_MODE_CONTINUOUS;
> -
>   	/* Only comparator mode is supported. */
>   	config &= ~TMP108_CONF_TM;
> 
> @@ -384,17 +500,33 @@ static int tmp108_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
>   	struct regmap *regmap;
> +	enum tmp108_hw_id hw_id;
> +	const void *of_data;
> 
>   	if (!i2c_check_functionality(client->adapter,
> -				     I2C_FUNC_SMBUS_WORD_DATA))
> +				     I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
>   		return dev_err_probe(dev, -ENODEV,
>   				     "adapter doesn't support SMBus word transactions\n");
> 
> -	regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config);
> +	regmap = devm_regmap_init(dev, &tmp108_i2c_regmap_bus, client, &tmp108_regmap_config);
>   	if (IS_ERR(regmap))
>   		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed");
> 
> -	return tmp108_common_probe(dev, regmap, client->name);
> +	/* Prefer OF match data (DT-first systems) */
> +	of_data = device_get_match_data(&client->dev);
> +	if (of_data) {
> +		hw_id = (unsigned long)of_data;
> +	} else {
> +		/* Fall back to legacy I2C ID table */
> +		const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +
> +		if (!id) {
> +			return dev_err_probe(dev, -ENODEV, "No matching device ID for i2c device\n");
> +		}
> +		hw_id = (unsigned long)id->driver_data;
> +	}

That complexity is unnecessary. Just use i2c_get_match_data().

> +
> +	return tmp108_common_probe(dev, regmap, client->name, hw_id);
>   }
> 
>   static int tmp108_suspend(struct device *dev)
> @@ -420,16 +552,18 @@ static int tmp108_resume(struct device *dev)
>   static DEFINE_SIMPLE_DEV_PM_OPS(tmp108_dev_pm_ops, tmp108_suspend, tmp108_resume);
> 
>   static const struct i2c_device_id tmp108_i2c_ids[] = {
> -	{ "p3t1085" },
> -	{ "tmp108" },
> -	{ }
> +	{ "p3t1035", P3T1035_ID },
> +	{ "p3t1085", P3T1085_ID },
> +	{ "tmp108", TMP108_ID },
> +	{ /* sentinel */ },
>   };
>   MODULE_DEVICE_TABLE(i2c, tmp108_i2c_ids);
> 
>   static const struct of_device_id tmp108_of_ids[] = {
> -	{ .compatible = "nxp,p3t1085", },
> -	{ .compatible = "ti,tmp108", },
> -	{}
> +	{ .compatible = "nxp,p3t1035", .data = (void *)(uintptr_t)P3T1035_ID },
> +	{ .compatible = "nxp,p3t1085", .data = (void *)(uintptr_t)P3T1085_ID },
> +	{ .compatible = "ti,tmp108", .data = (void *)(uintptr_t)TMP108_ID },
> +	{ /* sentinel */ },
>   };
>   MODULE_DEVICE_TABLE(of, tmp108_of_ids);
> 
> @@ -444,8 +578,9 @@ static struct i2c_driver tmp108_driver = {
>   };
> 
>   static const struct i3c_device_id p3t1085_i3c_ids[] = {
> -	I3C_DEVICE(0x011b, 0x1529, NULL),
> -	{}
> +	I3C_DEVICE(0x011B, 0x1529, (void *)P3T1085_ID),
> +	I3C_DEVICE(0x011B, 0x152B, (void *)P3T1035_ID),
> +	{ /* sentinel */ },

I know that some people like that comment, and I accept it for new drivers.
I do _not_ accept it being changed in existing drivers.

>   };
>   MODULE_DEVICE_TABLE(i3c, p3t1085_i3c_ids);
> 
> @@ -453,13 +588,21 @@ static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
>   {
>   	struct device *dev = i3cdev_to_dev(i3cdev);
>   	struct regmap *regmap;
> +	const struct i3c_device_id *id;
> +	enum tmp108_hw_id hw_id;
> 
> -	regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
> +	regmap = devm_regmap_init(dev, &tmp108_i3c_regmap_bus, i3cdev, &tmp108_regmap_config);
>   	if (IS_ERR(regmap))
>   		return dev_err_probe(dev, PTR_ERR(regmap),
>   				     "Failed to register i3c regmap\n");
> 
> -	return tmp108_common_probe(dev, regmap, "p3t1085_i3c");
> +	id = i3c_device_match_id(i3cdev, p3t1085_i3c_ids);
> +	if (!id) {
> +		return dev_err_probe(dev, -ENODEV, "No matching device ID for i3c device\n");
> +	}

Unnecessary error check since the id already matches or the function would not
have been called.

> +	hw_id = (enum tmp108_hw_id)(uintptr_t)id->data;
> +
> +	return tmp108_common_probe(dev, regmap, "p3t1085_i3c", hw_id);
>   }
> 
>   static struct i3c_driver p3t1085_driver = {
> --
> 2.34.1
> 


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

* Re: [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030
  2026-01-15 11:14 [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030 Mayank Mahajan
  2026-01-15 11:14 ` [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030 Mayank Mahajan
  2026-01-15 11:14 ` [PATCH v3 3/3] hwmon: (tmp108) Add P3T1035 and P3T2030 support Mayank Mahajan
@ 2026-01-16  7:22 ` Krzysztof Kozlowski
  2 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2026-01-16  7:22 UTC (permalink / raw)
  To: Mayank Mahajan, linux, corbet, robh, krzk+dt, conor+dt,
	linux-hwmon, devicetree, linux-doc, linux-kernel
  Cc: priyanka.jain, vikash.bansal

On 15/01/2026 12:14, Mayank Mahajan wrote:
> Document the NXP P3T1035 and P3T2030 compatibles in TMP108.
> 
> Signed-off-by: Mayank Mahajan <mayankmahajan.x@nxp.com>
> ---
> V1 -> V2:
> - No changes in v2.
> V2 -> V3:
> - Add P3T1035 fallback for P3T2030 as they are functionally identical.
> - Add comment in the description explaining the use of P3T2030.
> 
>  .../devicetree/bindings/hwmon/ti,tmp108.yaml  | 24 ++++++++++++-------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
> index a6f9319e068d..1f540c623de6 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
> @@ -4,27 +4,35 @@
>  $id: http://devicetree.org/schemas/hwmon/ti,tmp108.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
> 
> -title: TMP108/P3T1085(NXP) temperature sensor
> +title: TMP108/P3T1035/P3T1085/P3T2030 temperature sensor
> 
>  maintainers:
>    - Krzysztof Kozlowski <krzk@kernel.org>
> 
>  description: |
> -  The TMP108/P3T1085(NXP) is a digital-output temperature sensor with a
> -  dynamically-programmable limit window, and under- and overtemperature
> -  alert functions.
> +  The TMP108 or NXP P3T Family (P3T1035, P3T1085 and P3T2030) is a digital-
> +  output temperature sensor with a dynamically-programmable limit window,
> +  and under- and over-temperature alert functions.
> 
> -  P3T1085(NXP) support I3C.
> +  NXP P3T Family (P3T1035, P3T1085 and P3T2030) supports I3C.
> 
>    Datasheets:
>      https://www.ti.com/product/TMP108
>      https://www.nxp.com/docs/en/data-sheet/P3T1085UK.pdf
> +    https://www.nxp.com/docs/en/data-sheet/P3T1035XUK_P3T2030XUK.pdf
> +
> +  P3T2030 is functionally identical to P3T1035. Hence, device tree nodes that
> +  use the P3T2030 must provide a fallback compatible string of "nxp,p3t1035"

Drop the sentence. Schema already tells that. Never repeat the schema in
free text. It's like adding comments to code:

ptr = kzalloc()
/* Check for null pointer after allocation */
if (!ptr)
	return -ENOMEM

This is worse coding. Write concise, clearly readable code.

With this fixed:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

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

end of thread, other threads:[~2026-01-16  7:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-15 11:14 [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030 Mayank Mahajan
2026-01-15 11:14 ` [PATCH v3 2/3] hwmon: (tmp108) Add support for P3T1035 and P3T2030 Mayank Mahajan
2026-01-16  4:31   ` kernel test robot
2026-01-16  6:11   ` Guenter Roeck
2026-01-15 11:14 ` [PATCH v3 3/3] hwmon: (tmp108) Add P3T1035 and P3T2030 support Mayank Mahajan
2026-01-16  7:22 ` [PATCH v3 1/3] dt-bindings: hwmon: ti,tmp108: Add P3T1035,P3T2030 Krzysztof Kozlowski

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