[PATCH v3 0/5] hwmon: tmp108: Add support for P3T1085

[PATCH v3 0/5] hwmon: tmp108: 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 v3:
- Change cover letter subject
- Split compatible string change to a new patch
- Remove i3c/master.h
- Update document about p3t1085
- Link to v2: https://lore.kernel.org/r/20241108-p3t1085-v2-0-6a8990a59efd@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 (5):
      dt-bindings: hwmon: ti,tmp108: Add nxp,p3t1085 compatible string
      hwmon: tmp108: Add NXP p3t1085 support
      hwmon: tmp108: Add helper function tmp108_common_probe() to prepare I3C support
      hwmon: tmp108: Add support for I3C device
      arm64: dts: imx93-9x9-qsb: add temp-sensor nxp,p3t1085

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

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

[PATCH v3 1/5] 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.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v2 to v3
- add Krzy's ack tag
---
 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

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

[Guenter Roeck]

On Mon, Nov 11, 2024 at 12:31:58PM -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.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Applied.

Thanks,
Guenter

[PATCH v3 2/5] hwmon: tmp108: Add NXP p3t1085 support

[Frank Li]

Add compatible string 'nxp,p3t1085' since p3t1085's register layout is the
same as tmp108.

The p3t1085 supports I3C interface.

Update document tmp108.rst and Kconfig's help context.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v2 to v3
- new patch
- update documment
---
 Documentation/hwmon/tmp108.rst | 8 ++++++++
 drivers/hwmon/Kconfig          | 2 +-
 drivers/hwmon/tmp108.c         | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/tmp108.rst b/Documentation/hwmon/tmp108.rst
index 6df7cf1b42f49..bc4941d982681 100644
--- a/Documentation/hwmon/tmp108.rst
+++ b/Documentation/hwmon/tmp108.rst
@@ -3,6 +3,14 @@ Kernel driver tmp108
 
 Supported chips:
 
+  * NXP P3T1085
+
+    Prefix: 'p3t1085'
+
+    Addresses scanned: none
+
+    Datasheet: https://www.nxp.com/docs/en/data-sheet/P3T1085UK.pdf
+
   * Texas Instruments TMP108
 
     Prefix: 'tmp108'
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index cfb4e9314c62a..d43ca7aa4a548 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2300,7 +2300,7 @@ config SENSORS_TMP108
 	select REGMAP_I2C
 	help
 	  If you say yes here you get support for Texas Instruments TMP108
-	  sensor chips.
+	  sensor chips and NXP P3T1085.
 
 	  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 a82bbc959eb15..b561b452d8d39 100644
--- a/drivers/hwmon/tmp108.c
+++ b/drivers/hwmon/tmp108.c
@@ -420,6 +420,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

Re: [PATCH v3 2/5] hwmon: tmp108: Add NXP p3t1085 support

[Guenter Roeck]

On Mon, Nov 11, 2024 at 12:31:59PM -0500, Frank Li wrote:
> Add compatible string 'nxp,p3t1085' since p3t1085's register layout is the
> same as tmp108.
> 
> The p3t1085 supports I3C interface.
> 
> Update document tmp108.rst and Kconfig's help context.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Applied.

Thanks,
Guenter

[PATCH v3 3/5] hwmon: tmp108: Add helper function tmp108_common_probe() to prepare I3C support

[Frank Li]

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

Use dev_err_probe() to simple code.

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().
---
change from v2 to v3
- update subject by add  prepare I3C support"
---
 drivers/hwmon/tmp108.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
index b561b452d8d39..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);

-- 
2.34.1

Re: [PATCH v3 3/5] hwmon: tmp108: Add helper function tmp108_common_probe() to prepare I3C support

[Guenter Roeck]

On 11/11/24 09:32, Frank Li wrote:
> Add help function tmp108_common_probe() to pave road to support i3c for
> P3T1085(NXP) chip.
> 
> Use dev_err_probe() to simple code.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

I applied the first two patches of the series, but I am holding back this
and the subsequent patches since I don't know how the kernel handles having
two drivers in a single module, and one of them fails to instantiate. I did
not find a matching example elsewhere in the kernel. We may have to use
the method used by the st_lsm6dsx driver - essentially splitting the driver
into three parts (i2c, i3c, and common). That would be overkill; the alternative
might be something like the approach used by module_i3c_i2c_driver(), but with
a set of #ifdefs around (some of) the i3c code.

Guenter

Re: [PATCH v3 3/5] hwmon: tmp108: Add helper function tmp108_common_probe() to prepare I3C support

[Frank Li]

On Mon, Nov 11, 2024 at 11:44:08AM -0800, Guenter Roeck wrote:
> On 11/11/24 09:32, Frank Li wrote:
> > Add help function tmp108_common_probe() to pave road to support i3c for
> > P3T1085(NXP) chip.
> >
> > Use dev_err_probe() to simple code.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
>
> I applied the first two patches of the series, but I am holding back this
> and the subsequent patches since I don't know how the kernel handles having
> two drivers in a single module, and one of them fails to instantiate. I did
> not find a matching example elsewhere in the kernel. We may have to use
> the method used by the st_lsm6dsx driver - essentially splitting the driver
> into three parts (i2c, i3c, and common). That would be overkill; the alternative
> might be something like the approach used by module_i3c_i2c_driver(), but with
> a set of #ifdefs around (some of) the i3c code.

Thanks, let me think more about this.

>
> Guenter
>

[PATCH v3 4/5] 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>
---
change from v2 to v3
- change kconfig to select REGMAP_I3C if enable i3c
- remove i3c/master.h
- remove , after {}
- use #ifdef CONFIG_I3C about i3c register code

I2C    I3C
Y       Y       support both
Y       N       i3c part code will not be compiled
N       Y       whole TPM108 will not be compiled
N       N       whole TPM108 will not be compiled
---
 drivers/hwmon/Kconfig  |  1 +
 drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d43ca7aa4a548..9579db7849e1f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2298,6 +2298,7 @@ config SENSORS_TMP108
 	tristate "Texas Instruments TMP108"
 	depends on I2C
 	select REGMAP_I2C
+	select REGMAP_I3C if I3C
 	help
 	  If you say yes here you get support for Texas Instruments TMP108
 	  sensor chips and NXP P3T1085.
diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
index bfbea6349a95f..deb1505321335 100644
--- a/drivers/hwmon/tmp108.c
+++ b/drivers/hwmon/tmp108.c
@@ -13,6 +13,7 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/i2c.h>
+#include <linux/i3c/device.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/regmap.h>
@@ -442,6 +443,36 @@ static struct i2c_driver tmp108_driver = {
 
 module_i2c_driver(tmp108_driver);
 
+#ifdef CONFIG_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 device *dev = i3cdev_to_dev(i3cdev);
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i3c(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");
+}
+
+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

Re: [PATCH v3 4/5] hwmon: tmp108: Add support for I3C device

[Guenter Roeck]

On 11/11/24 09:32, 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>
> ---
> change from v2 to v3
> - change kconfig to select REGMAP_I3C if enable i3c
> - remove i3c/master.h
> - remove , after {}
> - use #ifdef CONFIG_I3C about i3c register code
> 
> I2C    I3C
> Y       Y       support both
> Y       N       i3c part code will not be compiled
> N       Y       whole TPM108 will not be compiled
> N       N       whole TPM108 will not be compiled
> ---
>   drivers/hwmon/Kconfig  |  1 +
>   drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d43ca7aa4a548..9579db7849e1f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2298,6 +2298,7 @@ config SENSORS_TMP108
>   	tristate "Texas Instruments TMP108"
>   	depends on I2C
>   	select REGMAP_I2C
> +	select REGMAP_I3C if I3C
>   	help
>   	  If you say yes here you get support for Texas Instruments TMP108
>   	  sensor chips and NXP P3T1085.
> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> index bfbea6349a95f..deb1505321335 100644
> --- a/drivers/hwmon/tmp108.c
> +++ b/drivers/hwmon/tmp108.c
> @@ -13,6 +13,7 @@
>   #include <linux/mutex.h>
>   #include <linux/of.h>
>   #include <linux/i2c.h>
> +#include <linux/i3c/device.h>
>   #include <linux/init.h>
>   #include <linux/jiffies.h>
>   #include <linux/regmap.h>
> @@ -442,6 +443,36 @@ static struct i2c_driver tmp108_driver = {
>   
>   module_i2c_driver(tmp108_driver);
>   
> +#ifdef CONFIG_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 device *dev = i3cdev_to_dev(i3cdev);
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i3c(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");
> +}
> +
> +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

While looking at i3c code, I found module_i3c_i2c_driver(). Can we use
that function to register both i2c and i3c in one call ?

Thanks,
Guenter

Re: [PATCH v3 4/5] hwmon: tmp108: Add support for I3C device

[Guenter Roeck]

On 11/11/24 10:04, Guenter Roeck wrote:
[ ... ]
>> +static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
>> +{
>> +    struct device *dev = i3cdev_to_dev(i3cdev);
>> +    struct regmap *regmap;
>> +
>> +    regmap = devm_regmap_init_i3c(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");
>> +}
>> +
>> +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
> 
> While looking at i3c code, I found module_i3c_i2c_driver(). Can we use
> that function to register both i2c and i3c in one call ?
> 
Answering my own question: No, because devm_regmap_init_i3c()
does not provide a dummy function if i3C is not enabled.

Guenter

Re: [PATCH v3 4/5] hwmon: tmp108: Add support for I3C device

[Guenter Roeck]

On 11/11/24 10:10, Guenter Roeck wrote:
> On 11/11/24 10:04, Guenter Roeck wrote:
> [ ... ]
>>> +static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
>>> +{
>>> +    struct device *dev = i3cdev_to_dev(i3cdev);
>>> +    struct regmap *regmap;
>>> +
>>> +    regmap = devm_regmap_init_i3c(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");
>>> +}
>>> +
>>> +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
>>
>> While looking at i3c code, I found module_i3c_i2c_driver(). Can we use
>> that function to register both i2c and i3c in one call ?
>>
> Answering my own question: No, because devm_regmap_init_i3c()
> does not provide a dummy function if i3C is not enabled.
> 

I do have another concern, though: What happens if the i2c part of the driver
registers and the i3c part fails to register ? module_i3c_i2c_driver() handles
that situation by unregistering the i2c driver, but I don't really know
what happens if a single module registers two drivers and one of them fails.

Thanks,
Guenter

Re: [PATCH v3 4/5] hwmon: tmp108: Add support for I3C device

[Frank Li]

On Mon, Nov 11, 2024 at 10:37:04AM -0800, Guenter Roeck wrote:
> On 11/11/24 10:10, Guenter Roeck wrote:
> > On 11/11/24 10:04, Guenter Roeck wrote:
> > [ ... ]
> > > > +static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
> > > > +{
> > > > +    struct device *dev = i3cdev_to_dev(i3cdev);
> > > > +    struct regmap *regmap;
> > > > +
> > > > +    regmap = devm_regmap_init_i3c(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");
> > > > +}
> > > > +
> > > > +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
> > >
> > > While looking at i3c code, I found module_i3c_i2c_driver(). Can we use
> > > that function to register both i2c and i3c in one call ?
> > >
> > Answering my own question: No, because devm_regmap_init_i3c()
> > does not provide a dummy function if i3C is not enabled.
> >
>
> I do have another concern, though: What happens if the i2c part of the driver
> registers and the i3c part fails to register ? module_i3c_i2c_driver() handles
> that situation by unregistering the i2c driver, but I don't really know
> what happens if a single module registers two drivers and one of them fails.

After use module_i3c_i2c_driver(), and remove #ifdef I3C, and disable I3C
in config, build passed.

It possible cause by

static inline int i3c_i2c_driver_register(struct i3c_driver *i3cdrv,
					  struct i2c_driver *i2cdrv)
{
	int ret;

	ret = i2c_add_driver(i2cdrv);
	if (ret || !IS_ENABLED(CONFIG_I3C))
		return ret;

^^^ !IS_ENABLED(CONFIG_I3C) is true, so linker skip below part. So no
ref to i3cdrv, so linker remove all related codes.

I use aarch64-linux-gnu-gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0.

Did you met error if use module_i3c_i2c_driver()?


	ret = i3c_driver_register(i3cdrv);
	if (ret)
		i2c_del_driver(i2cdrv);

	return ret;
}

>
> Thanks,
> Guenter
>

Re: [PATCH v3 4/5] hwmon: tmp108: Add support for I3C device

[Guenter Roeck]

On 11/11/24 15:20, Frank Li wrote:
> On Mon, Nov 11, 2024 at 10:37:04AM -0800, Guenter Roeck wrote:
>> On 11/11/24 10:10, Guenter Roeck wrote:
>>> On 11/11/24 10:04, Guenter Roeck wrote:
>>> [ ... ]
>>>>> +static int p3t1085_i3c_probe(struct i3c_device *i3cdev)
>>>>> +{
>>>>> +    struct device *dev = i3cdev_to_dev(i3cdev);
>>>>> +    struct regmap *regmap;
>>>>> +
>>>>> +    regmap = devm_regmap_init_i3c(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");
>>>>> +}
>>>>> +
>>>>> +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
>>>>
>>>> While looking at i3c code, I found module_i3c_i2c_driver(). Can we use
>>>> that function to register both i2c and i3c in one call ?
>>>>
>>> Answering my own question: No, because devm_regmap_init_i3c()
>>> does not provide a dummy function if i3C is not enabled.
>>>
>>
>> I do have another concern, though: What happens if the i2c part of the driver
>> registers and the i3c part fails to register ? module_i3c_i2c_driver() handles
>> that situation by unregistering the i2c driver, but I don't really know
>> what happens if a single module registers two drivers and one of them fails.
> 
> After use module_i3c_i2c_driver(), and remove #ifdef I3C, and disable I3C
> in config, build passed.
> 
> It possible cause by
> 
> static inline int i3c_i2c_driver_register(struct i3c_driver *i3cdrv,
> 					  struct i2c_driver *i2cdrv)
> {
> 	int ret;
> 
> 	ret = i2c_add_driver(i2cdrv);
> 	if (ret || !IS_ENABLED(CONFIG_I3C))
> 		return ret;
> 
> ^^^ !IS_ENABLED(CONFIG_I3C) is true, so linker skip below part. So no
> ref to i3cdrv, so linker remove all related codes.
> 

Yes, but I don't think we can rely on the compiler removing the call to
devm_regmap_init_i3c().

Thanks,
Guenter

Re: [PATCH v3 4/5] hwmon: tmp108: Add support for I3C device

[Guenter Roeck]

On 11/11/24 09:32, 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>
> ---
> change from v2 to v3
> - change kconfig to select REGMAP_I3C if enable i3c
> - remove i3c/master.h
> - remove , after {}
> - use #ifdef CONFIG_I3C about i3c register code
> 
> I2C    I3C
> Y       Y       support both
> Y       N       i3c part code will not be compiled
> N       Y       whole TPM108 will not be compiled
> N       N       whole TPM108 will not be compiled
> ---
>   drivers/hwmon/Kconfig  |  1 +
>   drivers/hwmon/tmp108.c | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d43ca7aa4a548..9579db7849e1f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2298,6 +2298,7 @@ config SENSORS_TMP108
>   	tristate "Texas Instruments TMP108"
>   	depends on I2C
>   	select REGMAP_I2C
> +	select REGMAP_I3C if I3C
>   	help
>   	  If you say yes here you get support for Texas Instruments TMP108
>   	  sensor chips and NXP P3T1085.
> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> index bfbea6349a95f..deb1505321335 100644
> --- a/drivers/hwmon/tmp108.c
> +++ b/drivers/hwmon/tmp108.c
> @@ -13,6 +13,7 @@
>   #include <linux/mutex.h>
>   #include <linux/of.h>
>   #include <linux/i2c.h>
> +#include <linux/i3c/device.h>
>   #include <linux/init.h>
>   #include <linux/jiffies.h>
>   #include <linux/regmap.h>
> @@ -442,6 +443,36 @@ static struct i2c_driver tmp108_driver = {
>   
>   module_i2c_driver(tmp108_driver);
>   
> +#ifdef CONFIG_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 device *dev = i3cdev_to_dev(i3cdev);
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);

It is a bit kludgy, but maybe

#ifdef REGMAP_I3C
	regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
#else
	regmap = ERR_PTR(-ENODEV);
#endif

and then using module_i3c_i2c_driver() would work.

Guenter

> +	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");
> +}
> +
> +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 v3 4/5] hwmon: tmp108: Add support for I3C device

[kernel test robot]

Hi Frank,

kernel test robot noticed the following build errors:

[auto build test ERROR on 74741a050b79d31d8d2eeee12c77736596d0a6b2]

url:    https://github.com/intel-lab-lkp/linux/commits/Frank-Li/dt-bindings-hwmon-ti-tmp108-Add-nxp-p3t1085-compatible-string/20241112-013721
base:   74741a050b79d31d8d2eeee12c77736596d0a6b2
patch link:    https://lore.kernel.org/r/20241111-p3t1085-v3-4-bff511550aad%40nxp.com
patch subject: [PATCH v3 4/5] hwmon: tmp108: Add support for I3C device
config: arc-randconfig-001-20241114 (https://download.01.org/0day-ci/archive/20241114/202411141530.qTxjCzf7-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241114/202411141530.qTxjCzf7-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/202411141530.qTxjCzf7-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/device/driver.h:21,
                    from include/linux/device.h:32,
                    from drivers/hwmon/tmp108.c:8:
>> include/linux/module.h:131:49: error: redefinition of '__inittest'
     131 |         static inline initcall_t __maybe_unused __inittest(void)                \
         |                                                 ^~~~~~~~~~
   include/linux/device/driver.h:262:1: note: in expansion of macro 'module_init'
     262 | module_init(__driver##_init); \
         | ^~~~~~~~~~~
   include/linux/i3c/device.h:233:9: note: in expansion of macro 'module_driver'
     233 |         module_driver(__drv, i3c_driver_register, i3c_driver_unregister)
         |         ^~~~~~~~~~~~~
   drivers/hwmon/tmp108.c:473:1: note: in expansion of macro 'module_i3c_driver'
     473 | module_i3c_driver(p3t1085_driver);
         | ^~~~~~~~~~~~~~~~~
   include/linux/module.h:131:49: note: previous definition of '__inittest' with type 'int (*(void))(void)'
     131 |         static inline initcall_t __maybe_unused __inittest(void)                \
         |                                                 ^~~~~~~~~~
   include/linux/device/driver.h:262:1: note: in expansion of macro 'module_init'
     262 | module_init(__driver##_init); \
         | ^~~~~~~~~~~
   include/linux/i2c.h:965:9: note: in expansion of macro 'module_driver'
     965 |         module_driver(__i2c_driver, i2c_add_driver, \
         |         ^~~~~~~~~~~~~
   drivers/hwmon/tmp108.c:444:1: note: in expansion of macro 'module_i2c_driver'
     444 | module_i2c_driver(tmp108_driver);
         | ^~~~~~~~~~~~~~~~~
>> include/linux/module.h:133:13: error: redefinition of 'init_module'
     133 |         int init_module(void) __copy(initfn)                    \
         |             ^~~~~~~~~~~
   include/linux/device/driver.h:262:1: note: in expansion of macro 'module_init'
     262 | module_init(__driver##_init); \
         | ^~~~~~~~~~~
   include/linux/i3c/device.h:233:9: note: in expansion of macro 'module_driver'
     233 |         module_driver(__drv, i3c_driver_register, i3c_driver_unregister)
         |         ^~~~~~~~~~~~~
   drivers/hwmon/tmp108.c:473:1: note: in expansion of macro 'module_i3c_driver'
     473 | module_i3c_driver(p3t1085_driver);
         | ^~~~~~~~~~~~~~~~~
   include/linux/module.h:133:13: note: previous definition of 'init_module' with type 'int(void)'
     133 |         int init_module(void) __copy(initfn)                    \
         |             ^~~~~~~~~~~
   include/linux/device/driver.h:262:1: note: in expansion of macro 'module_init'
     262 | module_init(__driver##_init); \
         | ^~~~~~~~~~~
   include/linux/i2c.h:965:9: note: in expansion of macro 'module_driver'
     965 |         module_driver(__i2c_driver, i2c_add_driver, \
         |         ^~~~~~~~~~~~~
   drivers/hwmon/tmp108.c:444:1: note: in expansion of macro 'module_i2c_driver'
     444 | module_i2c_driver(tmp108_driver);
         | ^~~~~~~~~~~~~~~~~
>> include/linux/module.h:139:49: error: redefinition of '__exittest'
     139 |         static inline exitcall_t __maybe_unused __exittest(void)                \
         |                                                 ^~~~~~~~~~
   include/linux/device/driver.h:267:1: note: in expansion of macro 'module_exit'
     267 | module_exit(__driver##_exit);
         | ^~~~~~~~~~~
   include/linux/i3c/device.h:233:9: note: in expansion of macro 'module_driver'
     233 |         module_driver(__drv, i3c_driver_register, i3c_driver_unregister)
         |         ^~~~~~~~~~~~~
   drivers/hwmon/tmp108.c:473:1: note: in expansion of macro 'module_i3c_driver'
     473 | module_i3c_driver(p3t1085_driver);
         | ^~~~~~~~~~~~~~~~~
   include/linux/module.h:139:49: note: previous definition of '__exittest' with type 'void (*(void))(void)'
     139 |         static inline exitcall_t __maybe_unused __exittest(void)                \
         |                                                 ^~~~~~~~~~
   include/linux/device/driver.h:267:1: note: in expansion of macro 'module_exit'
     267 | module_exit(__driver##_exit);
         | ^~~~~~~~~~~
   include/linux/i2c.h:965:9: note: in expansion of macro 'module_driver'
     965 |         module_driver(__i2c_driver, i2c_add_driver, \
         |         ^~~~~~~~~~~~~
   drivers/hwmon/tmp108.c:444:1: note: in expansion of macro 'module_i2c_driver'
     444 | module_i2c_driver(tmp108_driver);
         | ^~~~~~~~~~~~~~~~~
>> include/linux/module.h:141:14: error: redefinition of 'cleanup_module'
     141 |         void cleanup_module(void) __copy(exitfn)                \
         |              ^~~~~~~~~~~~~~
   include/linux/device/driver.h:267:1: note: in expansion of macro 'module_exit'
     267 | module_exit(__driver##_exit);
         | ^~~~~~~~~~~
   include/linux/i3c/device.h:233:9: note: in expansion of macro 'module_driver'
     233 |         module_driver(__drv, i3c_driver_register, i3c_driver_unregister)
         |         ^~~~~~~~~~~~~
   drivers/hwmon/tmp108.c:473:1: note: in expansion of macro 'module_i3c_driver'
     473 | module_i3c_driver(p3t1085_driver);
         | ^~~~~~~~~~~~~~~~~
   include/linux/module.h:141:14: note: previous definition of 'cleanup_module' with type 'void(void)'
     141 |         void cleanup_module(void) __copy(exitfn)                \
         |              ^~~~~~~~~~~~~~
   include/linux/device/driver.h:267:1: note: in expansion of macro 'module_exit'
     267 | module_exit(__driver##_exit);
         | ^~~~~~~~~~~
   include/linux/i2c.h:965:9: note: in expansion of macro 'module_driver'
     965 |         module_driver(__i2c_driver, i2c_add_driver, \
         |         ^~~~~~~~~~~~~
   drivers/hwmon/tmp108.c:444:1: note: in expansion of macro 'module_i2c_driver'
     444 | module_i2c_driver(tmp108_driver);
         | ^~~~~~~~~~~~~~~~~


vim +/__inittest +131 include/linux/module.h

0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  128  
0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  129  /* Each module must use one module_init(). */
0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  130  #define module_init(initfn)					\
1f318a8bafcfba9 Arnd Bergmann  2017-02-01 @131  	static inline initcall_t __maybe_unused __inittest(void)		\
0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  132  	{ return initfn; }					\
cf68fffb66d60d9 Sami Tolvanen  2021-04-08 @133  	int init_module(void) __copy(initfn)			\
cf68fffb66d60d9 Sami Tolvanen  2021-04-08  134  		__attribute__((alias(#initfn)));		\
92efda8eb15295a Sami Tolvanen  2022-09-08  135  	___ADDRESSABLE(init_module, __initdata);
0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  136  
0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  137  /* This is only required if you want to be unloadable. */
0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  138  #define module_exit(exitfn)					\
1f318a8bafcfba9 Arnd Bergmann  2017-02-01 @139  	static inline exitcall_t __maybe_unused __exittest(void)		\
0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  140  	{ return exitfn; }					\
cf68fffb66d60d9 Sami Tolvanen  2021-04-08 @141  	void cleanup_module(void) __copy(exitfn)		\
cf68fffb66d60d9 Sami Tolvanen  2021-04-08  142  		__attribute__((alias(#exitfn)));		\
92efda8eb15295a Sami Tolvanen  2022-09-08  143  	___ADDRESSABLE(cleanup_module, __exitdata);
0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  144  

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

[PATCH v3 5/5] 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>
---
change from v1 to v3
- none
---
 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 v3 5/5] arm64: dts: imx93-9x9-qsb: add temp-sensor nxp,p3t1085

[Marco Felsch]

Hi,

On 24-11-11, Frank Li wrote:
> Add temp-sensor nxp,p3t1085 for imx93-9x9-qsb boards.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change from v1 to v3
> - none
> ---
>  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>;
> +	};

Out of curiosity, did you tested that the I3C is working on a real
device, since you added the node here as I2C device? If not, please skip
adding the I3C part and keep it as TODO for later.

Regards,
  Marco

>  	ptn5110: tcpc@50 {
>  		compatible = "nxp,ptn5110", "tcpci";
>  		reg = <0x50>;
> 
> -- 
> 2.34.1
> 
> 
> 

Re: [PATCH v3 5/5] arm64: dts: imx93-9x9-qsb: add temp-sensor nxp,p3t1085

[Frank Li]

On Mon, Nov 11, 2024 at 08:13:11PM +0100, Marco Felsch wrote:
> Hi,
>
> On 24-11-11, Frank Li wrote:
> > Add temp-sensor nxp,p3t1085 for imx93-9x9-qsb boards.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change from v1 to v3
> > - none
> > ---
> >  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>;
> > +	};
>
> Out of curiosity, did you tested that the I3C is working on a real
> device, since you added the node here as I2C device? If not, please skip
> adding the I3C part and keep it as TODO for later.

Yes, it work as i3c. I3C device can auto probe according to PID
information, needn't add dt node for i3c device if it needn't power-supply
or some special settings.

Frank

>
> Regards,
>   Marco
>
> >  	ptn5110: tcpc@50 {
> >  		compatible = "nxp,ptn5110", "tcpci";
> >  		reg = <0x50>;
> >
> > --
> > 2.34.1
> >
> >
> >