[PATCH v2 0/4] iio: temperature: Add support for P3T1085

[PATCH v2 0/4] iio: temperature: Add support for P3T1085

[Frank Li]

Add basic function support for P3T1085 temperature sensor.
- reuse tmp108 driver
- Update imx93-9x9-qsb.dts

To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Jonathan Cameron <jic23@kernel.org>
To: Lars-Peter Clausen <lars@metafoo.de>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: Jean Delvare <jdelvare@suse.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-iio@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Frank Li <Frank.Li@nxp.com>

Changes in v2:
- Amost rewrite and reuse existed TMP108 driver
- Link to v1: https://lore.kernel.org/r/20241107-p3t1085-v1-0-9a76cb85673f@nxp.com

---
Frank Li (4):
      dt-bindings: hwmon: ti,tmp108: Add nxp,p3t1085 compatible string
      hwmon: tmp108: Add help function tmp108_common_probe()
      hwmon: tmp108: Add support for I3C device
      arm64: dts: imx93-9x9-qsb: add temp-sensor nxp,p3t1085

 .../devicetree/bindings/hwmon/ti,tmp108.yaml       |  8 ++-
 arch/arm64/boot/dts/freescale/imx93-9x9-qsb.dts    |  5 ++
 drivers/hwmon/tmp108.c                             | 71 ++++++++++++++++------
 3 files changed, 64 insertions(+), 20 deletions(-)
---
base-commit: 74741a050b79d31d8d2eeee12c77736596d0a6b2
change-id: 20241107-p3t1085-fbd8726cbc0e

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v2 1/4] dt-bindings: hwmon: ti,tmp108: Add nxp,p3t1085 compatible string

[Frank Li]

The register layout of P3T1085 is the same as ti,tmp108. Add compatible
string nxp,p3t1085 for it. The difference of P3T1085 is support I3C.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml b/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
index 0ad10d43fac0c..a6f9319e068d4 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml
@@ -4,22 +4,26 @@
 $id: http://devicetree.org/schemas/hwmon/ti,tmp108.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: TMP108 temperature sensor
+title: TMP108/P3T1085(NXP) temperature sensor
 
 maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
 
 description: |
-  The TMP108 is a digital-output temperature sensor with a
+  The TMP108/P3T1085(NXP) is a digital-output temperature sensor with a
   dynamically-programmable limit window, and under- and overtemperature
   alert functions.
 
+  P3T1085(NXP) support I3C.
+
   Datasheets:
     https://www.ti.com/product/TMP108
+    https://www.nxp.com/docs/en/data-sheet/P3T1085UK.pdf
 
 properties:
   compatible:
     enum:
+      - nxp,p3t1085
       - ti,tmp108
 
   interrupts:

-- 
2.34.1

[PATCH v2 2/4] hwmon: tmp108: Add help function tmp108_common_probe()

[Frank Li]

Add help function tmp108_common_probe() to pave road to support i3c for
P3T1085(NXP) chip.

Using dev_err_probe() simple code.

Add compatible string "nxp,p3t1085".

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
dev_err_probe() have not involve addition diff change. The difference
always list these code block change regardless use dev_err_probe().
---
 drivers/hwmon/tmp108.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
index a82bbc959eb15..bfbea6349a95f 100644
--- a/drivers/hwmon/tmp108.c
+++ b/drivers/hwmon/tmp108.c
@@ -323,33 +323,19 @@ static const struct regmap_config tmp108_regmap_config = {
 	.use_single_write = true,
 };
 
-static int tmp108_probe(struct i2c_client *client)
+static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name)
 {
-	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct tmp108 *tmp108;
-	int err;
 	u32 config;
-
-	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_WORD_DATA)) {
-		dev_err(dev,
-			"adapter doesn't support SMBus word transactions\n");
-		return -ENODEV;
-	}
+	int err;
 
 	tmp108 = devm_kzalloc(dev, sizeof(*tmp108), GFP_KERNEL);
 	if (!tmp108)
 		return -ENOMEM;
 
 	dev_set_drvdata(dev, tmp108);
-
-	tmp108->regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config);
-	if (IS_ERR(tmp108->regmap)) {
-		err = PTR_ERR(tmp108->regmap);
-		dev_err(dev, "regmap init failed: %d", err);
-		return err;
-	}
+	tmp108->regmap = regmap;
 
 	err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &config);
 	if (err < 0) {
@@ -383,13 +369,30 @@ static int tmp108_probe(struct i2c_client *client)
 		return err;
 	}
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, name,
 							 tmp108,
 							 &tmp108_chip_info,
 							 NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
+static int tmp108_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+
+	if (!i2c_check_functionality(client->adapter,
+				     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);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed");
+
+	return tmp108_common_probe(dev, regmap, client->name);
+}
+
 static int tmp108_suspend(struct device *dev)
 {
 	struct tmp108 *tmp108 = dev_get_drvdata(dev);
@@ -420,6 +423,7 @@ MODULE_DEVICE_TABLE(i2c, tmp108_i2c_ids);
 
 #ifdef CONFIG_OF
 static const struct of_device_id tmp108_of_ids[] = {
+	{ .compatible = "nxp,p3t1085", },
 	{ .compatible = "ti,tmp108", },
 	{}
 };

-- 
2.34.1

[PATCH v2 3/4] hwmon: tmp108: Add support for I3C device

[Frank Li]

Add support for I3C device in the tmp108 driver to handle the P3T1085
sensor. Register the I3C device driver to enable I3C functionality for the
sensor.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
index bfbea6349a95f..83d6847cb542c 100644
--- a/drivers/hwmon/tmp108.c
+++ b/drivers/hwmon/tmp108.c
@@ -13,6 +13,8 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/i2c.h>
+#include <linux/i3c/device.h>
+#include <linux/i3c/master.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/regmap.h>
@@ -442,6 +444,35 @@ static struct i2c_driver tmp108_driver = {
 
 module_i2c_driver(tmp108_driver);
 
+#ifdef CONFIG_REGMAP_I3C
+static const struct i3c_device_id p3t1085_i3c_ids[] = {
+	I3C_DEVICE(0x011b, 0x1529, NULL),
+	{},
+};
+MODULE_DEVICE_TABLE(i3c, p3t1085_i3c_ids);
+
+static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
+{
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(&i3cdev->dev, PTR_ERR(regmap),
+				     "Failed to register i3c regmap\n");
+
+	return tmp108_common_probe(&i3cdev->dev, regmap, "p3t1085_i3c");
+}
+
+static struct i3c_driver p3t1085_driver = {
+	.driver = {
+		.name = "p3t1085_i3c",
+	},
+	.probe = p3t1085_i3c_probe,
+	.id_table = p3t1085_i3c_ids,
+};
+module_i3c_driver(p3t1085_driver);
+#endif
+
 MODULE_AUTHOR("John Muir <john@jmuir.com>");
 MODULE_DESCRIPTION("Texas Instruments TMP108 temperature sensor driver");
 MODULE_LICENSE("GPL");

-- 
2.34.1

[PATCH v2 4/4] arm64: dts: imx93-9x9-qsb: add temp-sensor nxp,p3t1085

[Frank Li]

Add temp-sensor nxp,p3t1085 for imx93-9x9-qsb boards.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx93-9x9-qsb.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx93-9x9-qsb.dts b/arch/arm64/boot/dts/freescale/imx93-9x9-qsb.dts
index 20ec5b3c21f42..36f2995acbe29 100644
--- a/arch/arm64/boot/dts/freescale/imx93-9x9-qsb.dts
+++ b/arch/arm64/boot/dts/freescale/imx93-9x9-qsb.dts
@@ -221,6 +221,11 @@ wm8962: audio-codec@1a {
 		>;
 	};
 
+	p3t1085: temperature-sensor@48 {
+		compatible = "nxp,p3t1085";
+		reg = <0x48>;
+	};
+
 	ptn5110: tcpc@50 {
 		compatible = "nxp,ptn5110", "tcpci";
 		reg = <0x50>;

-- 
2.34.1

Re: [PATCH v2 2/4] hwmon: tmp108: Add help function tmp108_common_probe()

[Guenter Roeck]

On 11/8/24 14:26, Frank Li wrote:
> Add help function tmp108_common_probe() to pave road to support i3c for

help -> helper

> P3T1085(NXP) chip.
> 
> Using dev_err_probe() simple code.

Use dev_err_probe() to simplify the code.

> 
> Add compatible string "nxp,p3t1085".
> 

This is borderline and problematic. First, it is the one functional change,
and second, that functional change is not mentioned in the subject. At the very
least it needs to be mentioned in the subject. I would, however, prefer two
separate patches, even if that is just a one-liner.

Also, the key change is preparation for i3c support, not that a helper function
is added. The subject should be something like "Prepare for adding I3C support",
and the description should then mention the added helper function.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> dev_err_probe() have not involve addition diff change. The difference
> always list these code block change regardless use dev_err_probe().
> ---
>   drivers/hwmon/tmp108.c | 40 ++++++++++++++++++++++------------------
>   1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> index a82bbc959eb15..bfbea6349a95f 100644
> --- a/drivers/hwmon/tmp108.c
> +++ b/drivers/hwmon/tmp108.c
> @@ -323,33 +323,19 @@ static const struct regmap_config tmp108_regmap_config = {
>   	.use_single_write = true,
>   };
>   
> -static int tmp108_probe(struct i2c_client *client)
> +static int tmp108_common_probe(struct device *dev, struct regmap *regmap, char *name)
>   {
> -	struct device *dev = &client->dev;
>   	struct device *hwmon_dev;
>   	struct tmp108 *tmp108;
> -	int err;
>   	u32 config;
> -
> -	if (!i2c_check_functionality(client->adapter,
> -				     I2C_FUNC_SMBUS_WORD_DATA)) {
> -		dev_err(dev,
> -			"adapter doesn't support SMBus word transactions\n");
> -		return -ENODEV;
> -	}
> +	int err;
>   
>   	tmp108 = devm_kzalloc(dev, sizeof(*tmp108), GFP_KERNEL);
>   	if (!tmp108)
>   		return -ENOMEM;
>   
>   	dev_set_drvdata(dev, tmp108);
> -
> -	tmp108->regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config);
> -	if (IS_ERR(tmp108->regmap)) {
> -		err = PTR_ERR(tmp108->regmap);
> -		dev_err(dev, "regmap init failed: %d", err);
> -		return err;
> -	}
> +	tmp108->regmap = regmap;
>   
>   	err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &config);
>   	if (err < 0) {
> @@ -383,13 +369,30 @@ static int tmp108_probe(struct i2c_client *client)
>   		return err;
>   	}
>   
> -	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, name,
>   							 tmp108,
>   							 &tmp108_chip_info,
>   							 NULL);
>   	return PTR_ERR_OR_ZERO(hwmon_dev);
>   }
>   
> +static int tmp108_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     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);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed");
> +
> +	return tmp108_common_probe(dev, regmap, client->name);
> +}
> +
>   static int tmp108_suspend(struct device *dev)
>   {
>   	struct tmp108 *tmp108 = dev_get_drvdata(dev);
> @@ -420,6 +423,7 @@ MODULE_DEVICE_TABLE(i2c, tmp108_i2c_ids);
>   
>   #ifdef CONFIG_OF

It might also make sense to get rid of this conditional and of the
of_match_ptr() below to enable instantiation through ACPI.
That should be a separate patch, though.

Thanks,
Guenter

>   static const struct of_device_id tmp108_of_ids[] = {
> +	{ .compatible = "nxp,p3t1085", },
>   	{ .compatible = "ti,tmp108", },
>   	{}
>   };
> 

Re: [PATCH v2 0/4] iio: temperature: Add support for P3T1085

[Guenter Roeck]

On 11/8/24 14:26, Frank Li wrote:
> Add basic function support for P3T1085 temperature sensor.
> - reuse tmp108 driver
> - Update imx93-9x9-qsb.dts
> 

Subject 'iio' is now misleading.

Thanks,
Guenter

> To: Rob Herring <robh@kernel.org>
> To: Krzysztof Kozlowski <krzk+dt@kernel.org>
> To: Conor Dooley <conor+dt@kernel.org>
> To: Jonathan Cameron <jic23@kernel.org>
> To: Lars-Peter Clausen <lars@metafoo.de>
> To: Shawn Guo <shawnguo@kernel.org>
> To: Sascha Hauer <s.hauer@pengutronix.de>
> To: Pengutronix Kernel Team <kernel@pengutronix.de>
> To: Fabio Estevam <festevam@gmail.com>
> To: Jean Delvare <jdelvare@suse.com>
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-iio@vger.kernel.org
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> 
> Changes in v2:
> - Amost rewrite and reuse existed TMP108 driver
> - Link to v1: https://lore.kernel.org/r/20241107-p3t1085-v1-0-9a76cb85673f@nxp.com
> 
> ---
> Frank Li (4):
>        dt-bindings: hwmon: ti,tmp108: Add nxp,p3t1085 compatible string
>        hwmon: tmp108: Add help function tmp108_common_probe()
>        hwmon: tmp108: Add support for I3C device
>        arm64: dts: imx93-9x9-qsb: add temp-sensor nxp,p3t1085
> 
>   .../devicetree/bindings/hwmon/ti,tmp108.yaml       |  8 ++-
>   arch/arm64/boot/dts/freescale/imx93-9x9-qsb.dts    |  5 ++
>   drivers/hwmon/tmp108.c                             | 71 ++++++++++++++++------
>   3 files changed, 64 insertions(+), 20 deletions(-)
> ---
> base-commit: 74741a050b79d31d8d2eeee12c77736596d0a6b2
> change-id: 20241107-p3t1085-fbd8726cbc0e
> 
> Best regards,
> ---
> Frank Li <Frank.Li@nxp.com>
> 

Re: [PATCH v2 3/4] hwmon: tmp108: Add support for I3C device

[Guenter Roeck]

On 11/8/24 14:26, Frank Li wrote:
> Add support for I3C device in the tmp108 driver to handle the P3T1085
> sensor. Register the I3C device driver to enable I3C functionality for the
> sensor.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>   drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> index bfbea6349a95f..83d6847cb542c 100644
> --- a/drivers/hwmon/tmp108.c
> +++ b/drivers/hwmon/tmp108.c
> @@ -13,6 +13,8 @@
>   #include <linux/mutex.h>
>   #include <linux/of.h>
>   #include <linux/i2c.h>
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/master.h>
>   #include <linux/init.h>
>   #include <linux/jiffies.h>
>   #include <linux/regmap.h>
> @@ -442,6 +444,35 @@ static struct i2c_driver tmp108_driver = {
>   
>   module_i2c_driver(tmp108_driver);
>   
> +#ifdef CONFIG_REGMAP_I3C

REGMAP_I3C needs to be selected in Kconfig. Maybe with "select REGMAP_I3C if I3C" or
similar, but we'll also need to cover situations where I2C=y and I3C=n. That means
that an additional "depends on I3C || I3C=n" may be necessary. In summary,

	config SENSORS_TMP108
         tristate "Texas Instruments TMP108 and P3T1085"   <--
         depends on I2C
	select REGMAP_I2C
	depends on depends on I3C || I3C=n                <--
	select REGMAP_I3C if I3C                          <--
         help
           If you say yes here you get support for Texas Instruments TMP108
           and compatible sensor chips.                    <--

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

Thanks,
Guenter

> +static const struct i3c_device_id p3t1085_i3c_ids[] = {
> +	I3C_DEVICE(0x011b, 0x1529, NULL),
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i3c, p3t1085_i3c_ids);
> +
> +static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&i3cdev->dev, PTR_ERR(regmap),
> +				     "Failed to register i3c regmap\n");
> +
> +	return tmp108_common_probe(&i3cdev->dev, regmap, "p3t1085_i3c");
> +}
> +
> +static struct i3c_driver p3t1085_driver = {
> +	.driver = {
> +		.name = "p3t1085_i3c",
> +	},
> +	.probe = p3t1085_i3c_probe,
> +	.id_table = p3t1085_i3c_ids,
> +};
> +module_i3c_driver(p3t1085_driver);
> +#endif
> +
>   MODULE_AUTHOR("John Muir <john@jmuir.com>");
>   MODULE_DESCRIPTION("Texas Instruments TMP108 temperature sensor driver");
>   MODULE_LICENSE("GPL");
> 

Re: [PATCH v2 2/4] hwmon: tmp108: Add help function tmp108_common_probe()

[Guenter Roeck]

On 11/8/24 15:55, Guenter Roeck wrote:
> On 11/8/24 14:26, Frank Li wrote:
>> Add help function tmp108_common_probe() to pave road to support i3c for
> 
> help -> helper
> 
>> P3T1085(NXP) chip.
>>
>> Using dev_err_probe() simple code.
> 
> Use dev_err_probe() to simplify the code.
> 
>>
>> Add compatible string "nxp,p3t1085".
>>
> 
> This is borderline and problematic. First, it is the one functional change,
> and second, that functional change is not mentioned in the subject. At the very
> least it needs to be mentioned in the subject. I would, however, prefer two
> separate patches, even if that is just a one-liner.
> 
I forgot: The additional chip support should also be mentioned in Kconfig and in
Documentation/hwmon/tmp108.rst.

Thanks,
Guenter

Re: [PATCH v2 1/4] dt-bindings: hwmon: ti,tmp108: Add nxp,p3t1085 compatible string

[Krzysztof Kozlowski]

On Fri, Nov 08, 2024 at 05:26:55PM -0500, Frank Li wrote:
> The register layout of P3T1085 is the same as ti,tmp108. Add compatible
> string nxp,p3t1085 for it. The difference of P3T1085 is support I3C.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  Documentation/devicetree/bindings/hwmon/ti,tmp108.yaml | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 3/4] hwmon: tmp108: Add support for I3C device

[Jonathan Cameron]

On Fri, 08 Nov 2024 17:26:57 -0500
Frank Li <Frank.Li@nxp.com> wrote:

> Add support for I3C device in the tmp108 driver to handle the P3T1085
> sensor. Register the I3C device driver to enable I3C functionality for the
> sensor.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> index bfbea6349a95f..83d6847cb542c 100644
> --- a/drivers/hwmon/tmp108.c
> +++ b/drivers/hwmon/tmp108.c
> @@ -13,6 +13,8 @@
>  #include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/i2c.h>
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/master.h>

Seems odd you need master.h in a device driver.
I'll guess that's because you should be using i3cdev_to_device()
and not looking in the i3c_device structure at all.


>  #include <linux/init.h>
>  #include <linux/jiffies.h>
>  #include <linux/regmap.h>
> @@ -442,6 +444,35 @@ static struct i2c_driver tmp108_driver = {
>  
>  module_i2c_driver(tmp108_driver);
>  
> +#ifdef CONFIG_REGMAP_I3C
> +static const struct i3c_device_id p3t1085_i3c_ids[] = {
> +	I3C_DEVICE(0x011b, 0x1529, NULL),
> +	{},

Trivial, but no trailing comma needed here as nothing can come after
this terminator entry.  That is also consistent with existing similar
tables in this driver.

> +};
> +MODULE_DEVICE_TABLE(i3c, p3t1085_i3c_ids);
> +
> +static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&i3cdev->dev, PTR_ERR(regmap),
> +				     "Failed to register i3c regmap\n");
> +
> +	return tmp108_common_probe(&i3cdev->dev, regmap, "p3t1085_i3c");
> +}
> +
> +static struct i3c_driver p3t1085_driver = {
> +	.driver = {
> +		.name = "p3t1085_i3c",
> +	},
> +	.probe = p3t1085_i3c_probe,
> +	.id_table = p3t1085_i3c_ids,
> +};
> +module_i3c_driver(p3t1085_driver);
> +#endif
> +
>  MODULE_AUTHOR("John Muir <john@jmuir.com>");
>  MODULE_DESCRIPTION("Texas Instruments TMP108 temperature sensor driver");
>  MODULE_LICENSE("GPL");
> 

Re: [PATCH v2 3/4] hwmon: tmp108: Add support for I3C device

[Guenter Roeck]

On 11/9/24 05:16, Jonathan Cameron wrote:
> On Fri, 08 Nov 2024 17:26:57 -0500
> Frank Li <Frank.Li@nxp.com> wrote:
> 
>> Add support for I3C device in the tmp108 driver to handle the P3T1085
>> sensor. Register the I3C device driver to enable I3C functionality for the
>> sensor.
>>
>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>> ---
>>   drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
>> index bfbea6349a95f..83d6847cb542c 100644
>> --- a/drivers/hwmon/tmp108.c
>> +++ b/drivers/hwmon/tmp108.c
>> @@ -13,6 +13,8 @@
>>   #include <linux/mutex.h>
>>   #include <linux/of.h>
>>   #include <linux/i2c.h>
>> +#include <linux/i3c/device.h>
>> +#include <linux/i3c/master.h>
> 
> Seems odd you need master.h in a device driver.
> I'll guess that's because you should be using i3cdev_to_device()

I assume you mean i3cdev_to_dev() ?

Good point, but there are not many examples to draw from. The one
existing iio driver (st_lsm6dsx) doesn't use it either. I'll send
a patch shortly to fix that to prevent others from making the same
mistake.

Thanks,
Guenter

Re: [PATCH v2 3/4] hwmon: tmp108: Add support for I3C device

[Jonathan Cameron]

On Sat, 9 Nov 2024 06:53:28 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 11/9/24 05:16, Jonathan Cameron wrote:
> > On Fri, 08 Nov 2024 17:26:57 -0500
> > Frank Li <Frank.Li@nxp.com> wrote:
> >   
> >> Add support for I3C device in the tmp108 driver to handle the P3T1085
> >> sensor. Register the I3C device driver to enable I3C functionality for the
> >> sensor.
> >>
> >> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> >> ---
> >>   drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
> >>   1 file changed, 31 insertions(+)
> >>
> >> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> >> index bfbea6349a95f..83d6847cb542c 100644
> >> --- a/drivers/hwmon/tmp108.c
> >> +++ b/drivers/hwmon/tmp108.c
> >> @@ -13,6 +13,8 @@
> >>   #include <linux/mutex.h>
> >>   #include <linux/of.h>
> >>   #include <linux/i2c.h>
> >> +#include <linux/i3c/device.h>
> >> +#include <linux/i3c/master.h>  
> > 
> > Seems odd you need master.h in a device driver.
> > I'll guess that's because you should be using i3cdev_to_device()  
> 
> I assume you mean i3cdev_to_dev() ?
> 
Indeed! :(

> Good point, but there are not many examples to draw from. The one
> existing iio driver (st_lsm6dsx) doesn't use it either. I'll send
> a patch shortly to fix that to prevent others from making the same
> mistake.
Excellent.
> 
> Thanks,
> Guenter
> 

Re: [PATCH v2 3/4] hwmon: tmp108: Add support for I3C device

[Guenter Roeck]

On 11/9/24 07:15, Jonathan Cameron wrote:
> On Sat, 9 Nov 2024 06:53:28 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 11/9/24 05:16, Jonathan Cameron wrote:
>>> On Fri, 08 Nov 2024 17:26:57 -0500
>>> Frank Li <Frank.Li@nxp.com> wrote:
>>>    
>>>> Add support for I3C device in the tmp108 driver to handle the P3T1085
>>>> sensor. Register the I3C device driver to enable I3C functionality for the
>>>> sensor.
>>>>
>>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>>>> ---
>>>>    drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
>>>> index bfbea6349a95f..83d6847cb542c 100644
>>>> --- a/drivers/hwmon/tmp108.c
>>>> +++ b/drivers/hwmon/tmp108.c
>>>> @@ -13,6 +13,8 @@
>>>>    #include <linux/mutex.h>
>>>>    #include <linux/of.h>
>>>>    #include <linux/i2c.h>
>>>> +#include <linux/i3c/device.h>
>>>> +#include <linux/i3c/master.h>
>>>
>>> Seems odd you need master.h in a device driver.
>>> I'll guess that's because you should be using i3cdev_to_device()
>>
>> I assume you mean i3cdev_to_dev() ?
>>
> Indeed! :(
> 
>> Good point, but there are not many examples to draw from. The one
>> existing iio driver (st_lsm6dsx) doesn't use it either. I'll send
>> a patch shortly to fix that to prevent others from making the same
>> mistake.
> Excellent.

In this context, are you by any chance aware of an USB<->I3C adapter
wit decent price point ? With more I3C devices becoming available, I'd
like to be able to test at least some of the code with real hardware.
For I2C I use the Devantech USB-ISS adapter, but I have not yet found
anything comparable for I3C, at least nothing that is affordable.

Thanks,
Guenter

Re: [PATCH v2 3/4] hwmon: tmp108: Add support for I3C device

[Frank Li]

On Sat, Nov 09, 2024 at 08:18:13AM -0800, Guenter Roeck wrote:
> On 11/9/24 07:15, Jonathan Cameron wrote:
> > On Sat, 9 Nov 2024 06:53:28 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > > On 11/9/24 05:16, Jonathan Cameron wrote:
> > > > On Fri, 08 Nov 2024 17:26:57 -0500
> > > > Frank Li <Frank.Li@nxp.com> wrote:
> > > > > Add support for I3C device in the tmp108 driver to handle the P3T1085
> > > > > sensor. Register the I3C device driver to enable I3C functionality for the
> > > > > sensor.
> > > > >
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > >    drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
> > > > >    1 file changed, 31 insertions(+)
> > > > >
> > > > > diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> > > > > index bfbea6349a95f..83d6847cb542c 100644
> > > > > --- a/drivers/hwmon/tmp108.c
> > > > > +++ b/drivers/hwmon/tmp108.c
> > > > > @@ -13,6 +13,8 @@
> > > > >    #include <linux/mutex.h>
> > > > >    #include <linux/of.h>
> > > > >    #include <linux/i2c.h>
> > > > > +#include <linux/i3c/device.h>
> > > > > +#include <linux/i3c/master.h>
> > > >
> > > > Seems odd you need master.h in a device driver.
> > > > I'll guess that's because you should be using i3cdev_to_device()
> > >
> > > I assume you mean i3cdev_to_dev() ?
> > >
> > Indeed! :(
> >
> > > Good point, but there are not many examples to draw from. The one
> > > existing iio driver (st_lsm6dsx) doesn't use it either. I'll send
> > > a patch shortly to fix that to prevent others from making the same
> > > mistake.
> > Excellent.
>
> In this context, are you by any chance aware of an USB<->I3C adapter
> wit decent price point ? With more I3C devices becoming available, I'd
> like to be able to test at least some of the code with real hardware.
> For I2C I use the Devantech USB-ISS adapter, but I have not yet found
> anything comparable for I3C, at least nothing that is affordable.

I have not find usb->i3c adapter yet. you may try our qsb board
https://www.nxp.com/design/design-center/software/development-software/i-mx-93-quick-start-evaluation-kit:IMX93QSB

There are DIP header for it, SCL, SDA, GND and POWER.

About $311

Frank

>
> Thanks,
> Guenter
>