regmap I3C support

regmap I3C support

[Guenter Roeck]

Hi all,

in general, regmap support for a specific interface type requires
a matching configuration option such as REGMAP_I2C or REGMAP_I3C.

For most interface types that works just fine. However, I3C is
kind of an exception since the same hardware and driver typically
supports both I2C and I3C.

The problem can be addressed by implementing three drivers - an I2C driver,
an I3C driver, and a common driver. This is done in the st_lsm6dsx IIO driver.

For simpler I2C / I3C devices, there is module_i3c_i2c_driver() which is
supposed to register both the I2C and the I3C drivers and handle the situation
where I3C is not enabled.

That works fine unless the driver uses regmap, but I3C and with it REGMAP_I3C
is not enabled. There is no dummy function for devm_regmap_init_i3c(), so
a probe function for an I2C/I3C driver utilizing regmap can not be built
unless REGMAP_I3C and with it I3C is enabled. Ultimately that means that
I2C/I3C drivers using regmap can not use module_i3c_i2c_driver() to register
the driver.

Am I missing something ? Otherwise, would it be possible to consider
a dummy devm_regmap_init_i3c() function if REGMAP_I3C=n ?

Thanks,
Guenter

Re: regmap I3C support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

On Mon, Nov 11, 2024 at 11:07:40AM -0800, Guenter Roeck wrote:

> For simpler I2C / I3C devices, there is module_i3c_i2c_driver() which is
> supposed to register both the I2C and the I3C drivers and handle the situation
> where I3C is not enabled.

> That works fine unless the driver uses regmap, but I3C and with it REGMAP_I3C
> is not enabled. There is no dummy function for devm_regmap_init_i3c(), so
> a probe function for an I2C/I3C driver utilizing regmap can not be built
> unless REGMAP_I3C and with it I3C is enabled. Ultimately that means that
> I2C/I3C drivers using regmap can not use module_i3c_i2c_driver() to register
> the driver.

> Am I missing something ? Otherwise, would it be possible to consider
> a dummy devm_regmap_init_i3c() function if REGMAP_I3C=n ?

Don't these drivers end up with the same miserable problems with
dependencies on variations of things being configured built in and
modules anyway that mean we build separate SPI and I2C bus wrappers for
the same case with devices that do both I2C and SPI?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: regmap I3C support

[Guenter Roeck]

On 11/13/24 06:01, Mark Brown wrote:
> On Mon, Nov 11, 2024 at 11:07:40AM -0800, Guenter Roeck wrote:
> 
>> For simpler I2C / I3C devices, there is module_i3c_i2c_driver() which is
>> supposed to register both the I2C and the I3C drivers and handle the situation
>> where I3C is not enabled.
> 
>> That works fine unless the driver uses regmap, but I3C and with it REGMAP_I3C
>> is not enabled. There is no dummy function for devm_regmap_init_i3c(), so
>> a probe function for an I2C/I3C driver utilizing regmap can not be built
>> unless REGMAP_I3C and with it I3C is enabled. Ultimately that means that
>> I2C/I3C drivers using regmap can not use module_i3c_i2c_driver() to register
>> the driver.
> 
>> Am I missing something ? Otherwise, would it be possible to consider
>> a dummy devm_regmap_init_i3c() function if REGMAP_I3C=n ?
> 
> Don't these drivers end up with the same miserable problems with
> dependencies on variations of things being configured built in and
> modules anyway that mean we build separate SPI and I2C bus wrappers for
> the same case with devices that do both I2C and SPI?

Not really. There is no equivalent to module_i3c_i2c_driver() to handle both
I2C and SPI variants of a chip. Also, SPI and I2C/I3C are not interdependent,
while I3C automatically selects I2C. That means it does make sense to handle
I2C and I3C in the same driver, but not I2C and SPI.

Sure, separate wrappers can be used, but that makes module_i3c_i2c_driver()
pointless.

Guenter

Re: regmap I3C support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On Wed, Nov 13, 2024 at 06:15:38AM -0800, Guenter Roeck wrote:
> On 11/13/24 06:01, Mark Brown wrote:

> > Don't these drivers end up with the same miserable problems with
> > dependencies on variations of things being configured built in and
> > modules anyway that mean we build separate SPI and I2C bus wrappers for
> > the same case with devices that do both I2C and SPI?

> Not really. There is no equivalent to module_i3c_i2c_driver() to handle both
> I2C and SPI variants of a chip. Also, SPI and I2C/I3C are not interdependent,

Sure, but lots of drivers were open coding an equivalent of that
(possibly some still do).

> while I3C automatically selects I2C. That means it does make sense to handle
> I2C and I3C in the same driver, but not I2C and SPI.

In terms of the devices they're very much the same and interdependent -
it's generally one IP block and one set of pins that's doing both I2C
and SPI, with nothing software visible different.  If I3C selects I2C
then that does eliminate some of the problem space, I can't remember the
speciifcs of how people (I think mainly randconfig people?) were
breaking things.  You at least have I2C=y I3C=m which means that
dependencies have to force the users modular.

> Sure, separate wrappers can be used, but that makes module_i3c_i2c_driver()
> pointless.

That's kind of my question.  If we are going to have this sort of stuff
we should also have it for I2C and SPI since it's such a common pattern.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: regmap I3C support

[Guenter Roeck]

On 11/13/24 06:41, Mark Brown wrote:
> On Wed, Nov 13, 2024 at 06:15:38AM -0800, Guenter Roeck wrote:
>> On 11/13/24 06:01, Mark Brown wrote:
> 
>>> Don't these drivers end up with the same miserable problems with
>>> dependencies on variations of things being configured built in and
>>> modules anyway that mean we build separate SPI and I2C bus wrappers for
>>> the same case with devices that do both I2C and SPI?
> 
>> Not really. There is no equivalent to module_i3c_i2c_driver() to handle both
>> I2C and SPI variants of a chip. Also, SPI and I2C/I3C are not interdependent,
> 
> Sure, but lots of drivers were open coding an equivalent of that
> (possibly some still do).
> 
>> while I3C automatically selects I2C. That means it does make sense to handle
>> I2C and I3C in the same driver, but not I2C and SPI.
> 
> In terms of the devices they're very much the same and interdependent -
> it's generally one IP block and one set of pins that's doing both I2C
> and SPI, with nothing software visible different.  If I3C selects I2C
> then that does eliminate some of the problem space, I can't remember the
> speciifcs of how people (I think mainly randconfig people?) were
> breaking things.  You at least have I2C=y I3C=m which means that
> dependencies have to force the users modular.
> 
>> Sure, separate wrappers can be used, but that makes module_i3c_i2c_driver()
>> pointless.
> 
> That's kind of my question.  If we are going to have this sort of stuff
> we should also have it for I2C and SPI since it's such a common pattern.

I'll take that as "we are not going to provide a dummy regmap i3c registration
function". Thanks, that is all I needed to know.

Guenter

Re: regmap I3C support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Wed, Nov 13, 2024 at 12:16:37PM -0800, Guenter Roeck wrote:
> On 11/13/24 06:41, Mark Brown wrote:

> > That's kind of my question.  If we are going to have this sort of stuff
> > we should also have it for I2C and SPI since it's such a common pattern.

> I'll take that as "we are not going to provide a dummy regmap i3c registration
> function". Thanks, that is all I needed to know.

It's not a definite no, it's a "is this just going to run into the same
problems we ran into with I2C and SPI and never get used?" or
alternatively "have people figured out a better solution to the problems
we had with I2C and SPI which we can adopt there?" - it's something we
wanted to do before but ran into trouble with.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: regmap I3C support

[Guenter Roeck]

On 11/14/24 03:31, Mark Brown wrote:
> On Wed, Nov 13, 2024 at 12:16:37PM -0800, Guenter Roeck wrote:
>> On 11/13/24 06:41, Mark Brown wrote:
> 
>>> That's kind of my question.  If we are going to have this sort of stuff
>>> we should also have it for I2C and SPI since it's such a common pattern.
> 
>> I'll take that as "we are not going to provide a dummy regmap i3c registration
>> function". Thanks, that is all I needed to know.
> 
> It's not a definite no, it's a "is this just going to run into the same
> problems we ran into with I2C and SPI and never get used?" or
> alternatively "have people figured out a better solution to the problems
> we had with I2C and SPI which we can adopt there?" - it's something we
> wanted to do before but ran into trouble with.

We now use

config SENSORS_TMP108
         tristate "Texas Instruments TMP108"
         depends on I2C
         depends on I3C || !I3C
         select REGMAP_I2C
         select REGMAP_I3C if I3C

and in the i3c_probe function

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

Clumsy, and not my preferred solution, but it works.

Guenter

Re: regmap I3C support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Thu, Nov 14, 2024 at 06:45:52AM -0800, Guenter Roeck wrote:

> We now use

> config SENSORS_TMP108
>         tristate "Texas Instruments TMP108"
>         depends on I2C
>         depends on I3C || !I3C
>         select REGMAP_I2C
>         select REGMAP_I3C if I3C

> and in the i3c_probe function

> #ifdef CONFIG_REGMAP_I3C
>         regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
> #else
>         regmap = ERR_PTR(-ENODEV);
> #endif
>         if (IS_ERR(regmap))

> Clumsy, and not my preferred solution, but it works.

Right, so the fact that I3C depends on I2C deals with a lot of the
problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
should be OK and there's not much doing for I2C/SPI.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: regmap I3C support

[Guenter Roeck]

On Thu, Nov 14, 2024 at 05:26:19PM +0000, Mark Brown wrote:
> On Thu, Nov 14, 2024 at 06:45:52AM -0800, Guenter Roeck wrote:
> 
> > We now use
> 
> > config SENSORS_TMP108
> >         tristate "Texas Instruments TMP108"
> >         depends on I2C
> >         depends on I3C || !I3C
> >         select REGMAP_I2C
> >         select REGMAP_I3C if I3C
> 
> > and in the i3c_probe function
> 
> > #ifdef CONFIG_REGMAP_I3C
> >         regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
> > #else
> >         regmap = ERR_PTR(-ENODEV);
> > #endif
> >         if (IS_ERR(regmap))
> 
> > Clumsy, and not my preferred solution, but it works.
> 
> Right, so the fact that I3C depends on I2C deals with a lot of the
> problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
> should be OK and there's not much doing for I2C/SPI.

Is it really that difficult for I2C and SPI ? The patch below seems to work
for the LTC2947 driver. It doesn't even need dummies (the compiler drops
the unused code), though I am not sure if that can be relied on. I thought
that dummy functions are needed, but maybe I am wrong.

The Kconfig for the combined ltc2947 driver is

config SENSORS_LTC2947
        tristate "Analog Devices LTC2947 High Precision Power and Energy Monitor"
        depends on I2C || SPI
        depends on I2C || I2C=n
        select REGMAP_I2C if I2C
        select REGMAP_SPI if SPI
        help
	...

Guenter

---
From 8b72bcea4f399b3ffbea07256c5e48af63dfd230 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Thu, 14 Nov 2024 17:30:01 -0800
Subject: [PATCH] Add infrastructure for supporting both I2C and SPI in single
 driver

Add support for register and unregister functions for drivers supporting
both I2C and SPI. Support situations where only one of the protocols is
enabled.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 include/linux/i2c_spi.h | 81 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 include/linux/i2c_spi.h

diff --git a/include/linux/i2c_spi.h b/include/linux/i2c_spi.h
new file mode 100644
index 000000000000..3a8d32355338
--- /dev/null
+++ b/include/linux/i2c_spi.h
@@ -0,0 +1,81 @@
+/*---------------------------------------------------------------------------
+ *
+ * i2c_spi.h
+ *     Copyright (c) 2024 Guenter Roeck <linux@roeck-us.net>
+ *
+ * API functions to support both I2C and SPI in a single driver.
+ */
+
+#ifndef I2C_SPI_H
+#define I2C_SPI_H
+
+#include <linux/i2c.h>
+#include <linux/spi/spi.h>
+
+/**
+ * i2c_spi_driver_register() - Register an I2C and a SPI driver
+ * @i2cdrv: the I2C driver to register
+ * @spidrv: the SPI driver to register
+ *
+ * This function registers both @i2cdev and @spidev, and fails if one of these
+ * registrations fails. This is mainly useful for devices that support both I2C
+ * and SPI modes.
+ * Note that the function only registers drivers for the enabled protocol(s).
+ * If neither I2C nor SPI are enabled, it does nothing.
+ *
+ * Return: 0 if enabled registrations succeeded, a negative error code otherwise.
+ */
+static inline int i2c_spi_driver_register(struct i2c_driver *i2cdrv,
+					  struct spi_driver *spidrv)
+{
+	int ret = 0;
+
+	if (IS_ENABLED(CONFIG_I2C))
+		ret = i2c_add_driver(i2cdrv);
+	if (ret || !IS_ENABLED(CONFIG_SPI))
+		return ret;
+
+	ret = spi_register_driver(spidrv);
+	if (ret && IS_ENABLED(CONFIG_I2C))
+		i2c_del_driver(i2cdrv);
+
+	return ret;
+}
+
+/**
+ * i2c_spi_driver_unregister() - Unregister an I2C and a SPI driver
+ * @i2cdrv: the I2C driver to register
+ * @spidrv: the SPI driver to register
+ *
+ * This function unregisters both @i2cdrv and @i3cdrv.
+ * Note that the function only unregisters drivers for the enabled protocol(s).
+ */
+static inline void i2c_spi_driver_unregister(struct i2c_driver *i2cdrv,
+					     struct spi_driver *spidrv)
+{
+	if (IS_ENABLED(CONFIG_SPI))
+		spi_unregister_driver(spidrv);
+
+	if (IS_ENABLED(CONFIG_I2C))
+		i2c_del_driver(i2cdrv);
+}
+
+/**
+ * module_i2c_spi_driver() - Register a module providing an I2C and a SPI
+ *			     driver
+ * @__i2cdrv: the I2C driver to register
+ * @__spidrv: the SPI driver to register
+ *
+ * Provide generic init/exit functions that simply register/unregister an I2C
+ * and a SPI driver.
+ * This macro can be used even if CONFIG_I2C and/or CONFIG_SPI are disabled,
+ * in this case, only the enabled driver(s) driver will be registered.
+ * Should be used by any driver that does not require extra init/cleanup steps.
+ */
+#define module_i2c_spi_driver(__i2cdrv, __spidrv)	\
+	module_driver(__i2cdrv,				\
+		      i2c_spi_driver_register,		\
+		      i2c_spi_driver_unregister,	\
+		      __spidrv)
+
+#endif /* I2C_SPI_H */
-- 
2.45.2

Re: regmap I3C support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

On Thu, Nov 14, 2024 at 10:58:05PM -0800, Guenter Roeck wrote:
> On Thu, Nov 14, 2024 at 05:26:19PM +0000, Mark Brown wrote:

> > Right, so the fact that I3C depends on I2C deals with a lot of the
> > problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
> > should be OK and there's not much doing for I2C/SPI.

> Is it really that difficult for I2C and SPI ? The patch below seems to work
> for the LTC2947 driver. It doesn't even need dummies (the compiler drops
> the unused code), though I am not sure if that can be relied on. I thought
> that dummy functions are needed, but maybe I am wrong.
> 
> The Kconfig for the combined ltc2947 driver is
> 
> config SENSORS_LTC2947
>         tristate "Analog Devices LTC2947 High Precision Power and Energy Monitor"
>         depends on I2C || SPI
>         depends on I2C || I2C=n
>         select REGMAP_I2C if I2C
>         select REGMAP_SPI if SPI

This prevents building the driver in if I2C=m which isn't always
desirable, and IIRC the randconfig people kept turning issues up.  You
can make things work well enough for normal configurations, it's all
edge cases that cause issues.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: regmap I3C support

[Guenter Roeck]

On 11/15/24 07:06, Mark Brown wrote:
> On Thu, Nov 14, 2024 at 10:58:05PM -0800, Guenter Roeck wrote:
>> On Thu, Nov 14, 2024 at 05:26:19PM +0000, Mark Brown wrote:
> 
>>> Right, so the fact that I3C depends on I2C deals with a lot of the
>>> problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
>>> should be OK and there's not much doing for I2C/SPI.
> 
>> Is it really that difficult for I2C and SPI ? The patch below seems to work
>> for the LTC2947 driver. It doesn't even need dummies (the compiler drops
>> the unused code), though I am not sure if that can be relied on. I thought
>> that dummy functions are needed, but maybe I am wrong.
>>
>> The Kconfig for the combined ltc2947 driver is
>>
>> config SENSORS_LTC2947
>>          tristate "Analog Devices LTC2947 High Precision Power and Energy Monitor"
>>          depends on I2C || SPI
>>          depends on I2C || I2C=n
>>          select REGMAP_I2C if I2C
>>          select REGMAP_SPI if SPI
> 
> This prevents building the driver in if I2C=m which isn't always
> desirable, and IIRC the randconfig people kept turning issues up.  You
> can make things work well enough for normal configurations, it's all
> edge cases that cause issues.

You mean if SPI=y and I2C=m ? Good point. That isn't normally a problem
for sensor drivers, but I understand that it may be undesirable for others.

Thanks,
Guenter

Re: regmap I3C support

[Guenter Roeck]

On 11/14/24 09:26, Mark Brown wrote:
> On Thu, Nov 14, 2024 at 06:45:52AM -0800, Guenter Roeck wrote:
> 
>> We now use
> 
>> config SENSORS_TMP108
>>          tristate "Texas Instruments TMP108"
>>          depends on I2C
>>          depends on I3C || !I3C
>>          select REGMAP_I2C
>>          select REGMAP_I3C if I3C
> 
>> and in the i3c_probe function
> 
>> #ifdef CONFIG_REGMAP_I3C
>>          regmap = devm_regmap_init_i3c(i3cdev, &tmp108_regmap_config);
>> #else
>>          regmap = ERR_PTR(-ENODEV);
>> #endif
>>          if (IS_ERR(regmap))
> 
>> Clumsy, and not my preferred solution, but it works.
> 
> Right, so the fact that I3C depends on I2C deals with a lot of the
> problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
> should be OK and there's not much doing for I2C/SPI.

It looks like we can use

        if (IS_ENABLED(CONFIG_REGMAP_I3C)) {
                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");
		...
	}

even if a stub function is not available as long as there is an external
declaration.

I don't really like it, but it turns out that this kind of code is already used
elsewhere in the kernel. It looks like dead code elimination can now assumed
to be available when building kernel code. We live and learn.

Guenter

Re: regmap I3C support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On Fri, Nov 15, 2024 at 08:35:04PM -0800, Guenter Roeck wrote:
> On 11/14/24 09:26, Mark Brown wrote:

> > Right, so the fact that I3C depends on I2C deals with a lot of the
> > problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
> > should be OK and there's not much doing for I2C/SPI.

> It looks like we can use

>        if (IS_ENABLED(CONFIG_REGMAP_I3C)) {
>                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");
> 		...
> 	}

> even if a stub function is not available as long as there is an external
> declaration.

> I don't really like it, but it turns out that this kind of code is already used
> elsewhere in the kernel. It looks like dead code elimination can now assumed
> to be available when building kernel code. We live and learn.

Ah, that solves that problem then I guess?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: regmap I3C support

[Guenter Roeck]

On 11/19/24 09:46, Mark Brown wrote:
> On Fri, Nov 15, 2024 at 08:35:04PM -0800, Guenter Roeck wrote:
>> On 11/14/24 09:26, Mark Brown wrote:
> 
>>> Right, so the fact that I3C depends on I2C deals with a lot of the
>>> problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
>>> should be OK and there's not much doing for I2C/SPI.
> 
>> It looks like we can use
> 
>>         if (IS_ENABLED(CONFIG_REGMAP_I3C)) {
>>                 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");
>> 		...
>> 	}
> 
>> even if a stub function is not available as long as there is an external
>> declaration.
> 
>> I don't really like it, but it turns out that this kind of code is already used
>> elsewhere in the kernel. It looks like dead code elimination can now assumed
>> to be available when building kernel code. We live and learn.
> 
> Ah, that solves that problem then I guess?


Yes. It actually goes a step further - the IS_ENABLED(CONFIG_REGMAP_I3C)) in the
probe function isn't needed either because the entire i3c probe function is
optimized away if CONFIG_I3C=n.

I'll send a patch dropping the #ifdef in the tmp108 driver after the commit
window closes.

Thanks,
Guenter

Re: regmap I3C support

[Frank Li]

On Tue, Nov 19, 2024 at 10:41:05AM -0800, Guenter Roeck wrote:
> On 11/19/24 09:46, Mark Brown wrote:
> > On Fri, Nov 15, 2024 at 08:35:04PM -0800, Guenter Roeck wrote:
> > > On 11/14/24 09:26, Mark Brown wrote:
> >
> > > > Right, so the fact that I3C depends on I2C deals with a lot of the
> > > > problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
> > > > should be OK and there's not much doing for I2C/SPI.
> >
> > > It looks like we can use
> >
> > >         if (IS_ENABLED(CONFIG_REGMAP_I3C)) {
> > >                 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");
> > > 		...
> > > 	}
> >
> > > even if a stub function is not available as long as there is an external
> > > declaration.
> >
> > > I don't really like it, but it turns out that this kind of code is already used
> > > elsewhere in the kernel. It looks like dead code elimination can now assumed
> > > to be available when building kernel code. We live and learn.
> >
> > Ah, that solves that problem then I guess?
>
>
> Yes. It actually goes a step further - the IS_ENABLED(CONFIG_REGMAP_I3C)) in the
> probe function isn't needed either because the entire i3c probe function is
> optimized away if CONFIG_I3C=n.
>
> I'll send a patch dropping the #ifdef in the tmp108 driver after the commit
> window closes.

Already tried this at v3
https://lore.kernel.org/imx/7bdd2db8-41c8-43d8-ae73-84a221d2d004@roeck-us.net/

but I am not sure if it is good on rely on the compiler. Maybe some option
like some debug option or -O0 cause problem.

Frank

>
> Thanks,
> Guenter
>

Re: regmap I3C support

[Guenter Roeck]

On 11/19/24 11:30, Frank Li wrote:
> On Tue, Nov 19, 2024 at 10:41:05AM -0800, Guenter Roeck wrote:
>> On 11/19/24 09:46, Mark Brown wrote:
>>> On Fri, Nov 15, 2024 at 08:35:04PM -0800, Guenter Roeck wrote:
>>>> On 11/14/24 09:26, Mark Brown wrote:
>>>
>>>>> Right, so the fact that I3C depends on I2C deals with a lot of the
>>>>> problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
>>>>> should be OK and there's not much doing for I2C/SPI.
>>>
>>>> It looks like we can use
>>>
>>>>          if (IS_ENABLED(CONFIG_REGMAP_I3C)) {
>>>>                  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");
>>>> 		...
>>>> 	}
>>>
>>>> even if a stub function is not available as long as there is an external
>>>> declaration.
>>>
>>>> I don't really like it, but it turns out that this kind of code is already used
>>>> elsewhere in the kernel. It looks like dead code elimination can now assumed
>>>> to be available when building kernel code. We live and learn.
>>>
>>> Ah, that solves that problem then I guess?
>>
>>
>> Yes. It actually goes a step further - the IS_ENABLED(CONFIG_REGMAP_I3C)) in the
>> probe function isn't needed either because the entire i3c probe function is
>> optimized away if CONFIG_I3C=n.
>>
>> I'll send a patch dropping the #ifdef in the tmp108 driver after the commit
>> window closes.
> 
> Already tried this at v3
> https://lore.kernel.org/imx/7bdd2db8-41c8-43d8-ae73-84a221d2d004@roeck-us.net/
> 

Yes, I know. Sorry for that. We live and learn. I didn't think this works,
but it does.

> but I am not sure if it is good on rely on the compiler. Maybe some option
> like some debug option or -O0 cause problem.
> 

Yes, I thought so too, but it turns out that the kernel doesn't build anymore
with -O0 anyway, and other code already _does_ depend on dead code elimination.

Guenter

Re: regmap I3C support

[Frank Li]

On Tue, Nov 19, 2024 at 01:57:38PM -0800, Guenter Roeck wrote:
> On 11/19/24 11:30, Frank Li wrote:
> > On Tue, Nov 19, 2024 at 10:41:05AM -0800, Guenter Roeck wrote:
> > > On 11/19/24 09:46, Mark Brown wrote:
> > > > On Fri, Nov 15, 2024 at 08:35:04PM -0800, Guenter Roeck wrote:
> > > > > On 11/14/24 09:26, Mark Brown wrote:
> > > >
> > > > > > Right, so the fact that I3C depends on I2C deals with a lot of the
> > > > > > problems that plague the I2C/SPI combination.  Ugh.  I guess the helper
> > > > > > should be OK and there's not much doing for I2C/SPI.
> > > >
> > > > > It looks like we can use
> > > >
> > > > >          if (IS_ENABLED(CONFIG_REGMAP_I3C)) {
> > > > >                  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");
> > > > > 		...
> > > > > 	}
> > > >
> > > > > even if a stub function is not available as long as there is an external
> > > > > declaration.
> > > >
> > > > > I don't really like it, but it turns out that this kind of code is already used
> > > > > elsewhere in the kernel. It looks like dead code elimination can now assumed
> > > > > to be available when building kernel code. We live and learn.
> > > >
> > > > Ah, that solves that problem then I guess?
> > >
> > >
> > > Yes. It actually goes a step further - the IS_ENABLED(CONFIG_REGMAP_I3C)) in the
> > > probe function isn't needed either because the entire i3c probe function is
> > > optimized away if CONFIG_I3C=n.
> > >
> > > I'll send a patch dropping the #ifdef in the tmp108 driver after the commit
> > > window closes.
> >
> > Already tried this at v3
> > https://lore.kernel.org/imx/7bdd2db8-41c8-43d8-ae73-84a221d2d004@roeck-us.net/
> >
>
> Yes, I know. Sorry for that. We live and learn. I didn't think this works,
> but it does.
>
> > but I am not sure if it is good on rely on the compiler. Maybe some option
> > like some debug option or -O0 cause problem.
> >
>
> Yes, I thought so too, but it turns out that the kernel doesn't build anymore
> with -O0 anyway, and other code already _does_ depend on dead code elimination.

Good to know this, which keep life simple.

Frank

>
> Guenter
>