| Message ID | 20241111-p3t1085-v3-4-bff511550aad@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hwmon: tmp108: Add support for P3T1085 | expand |
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
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
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
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 >
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
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"); >
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");
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(+)