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

[Frank Li <Frank.li@nxp.com>]

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
>