[PATCH v2] i2c: i801: Fix missing Kconfig dependency

[PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Heiner Kallweit]

The original change adds usage of i2c_root_adapter(), which is
implemented in i2c-mux.c. Therefore we can't use the multiplexing
if I2C_I801=y and I2C_MUX=m.
Handling the dependencies in the code would become unnecessarily
complex, therefore create a new config symbol.

Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
- Completely rework the fix and create a new config symbol.
---
 drivers/i2c/busses/Kconfig    | 8 ++++++++
 drivers/i2c/busses/i2c-i801.c | 8 ++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 97989c914..3eb17f43a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -163,6 +163,14 @@ config I2C_I801
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-i801.
 
+config I2C_I801_MUX
+	def_bool I2C_I801
+	depends on DMI && I2C_MUX_GPIO
+	depends on !(I2C_I801=y && I2C_MUX=m)
+	help
+	  Optional support for multiplexed SMBUS on certain systems with
+	  more than 8 memory slots.
+
 config I2C_ISCH
 	tristate "Intel SCH SMBus 1.0"
 	depends on PCI
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 4294c0c63..92b53b480 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -120,7 +120,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/mutex.h>
 
-#if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
+#ifdef CONFIG_I2C_I801_MUX
 #include <linux/gpio/machine.h>
 #include <linux/platform_data/i2c-mux-gpio.h>
 #endif
@@ -289,7 +289,7 @@ struct i801_priv {
 	int len;
 	u8 *data;
 
-#if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
+#ifdef CONFIG_I2C_I801_MUX
 	struct platform_device *mux_pdev;
 	struct gpiod_lookup_table *lookup;
 	struct notifier_block mux_notifier_block;
@@ -1300,7 +1300,7 @@ static void i801_probe_optional_slaves(struct i801_priv *priv)
 		register_dell_lis3lv02d_i2c_device(priv);
 
 	/* Instantiate SPD EEPROMs unless the SMBus is multiplexed */
-#if IS_ENABLED(CONFIG_I2C_MUX_GPIO)
+#ifdef CONFIG_I2C_I801_MUX
 	if (!priv->mux_pdev)
 #endif
 		i2c_register_spd(&priv->adapter);
@@ -1310,7 +1310,7 @@ static void __init input_apanel_init(void) {}
 static void i801_probe_optional_slaves(struct i801_priv *priv) {}
 #endif	/* CONFIG_X86 && CONFIG_DMI */
 
-#if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
+#ifdef CONFIG_I2C_I801_MUX
 static struct i801_mux_config i801_mux_config_asus_z8_d12 = {
 	.gpio_chip = "gpio_ich",
 	.values = { 0x02, 0x03 },
-- 
2.44.0

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Andi Shyti]

Hi Heiner,

first of all, thanks for the fast reaction!

On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote:
> The original change adds usage of i2c_root_adapter(), which is
> implemented in i2c-mux.c. Therefore we can't use the multiplexing
> if I2C_I801=y and I2C_MUX=m.

What is wrong with select I2C_MUX?

And is this covering all the cases?

Last thing, how have you tested and reproduced the issue?

Thanks,
Andi

> Handling the dependencies in the code would become unnecessarily
> complex, therefore create a new config symbol.
> 
> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Heiner Kallweit]

On 05.04.2024 02:54, Andi Shyti wrote:
> Hi Heiner,
> 
> first of all, thanks for the fast reaction!
> 
> On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote:
>> The original change adds usage of i2c_root_adapter(), which is
>> implemented in i2c-mux.c. Therefore we can't use the multiplexing
>> if I2C_I801=y and I2C_MUX=m.
> 
> What is wrong with select I2C_MUX?
> 
It would solve the issue, but:
We would uselessly enable I2C_MUX also if I2C_MUX_GPIO or DMI are disabled.
W/o them the mux part in i801 is a no-op. The call to i2c_root_adapter()
is in a conditionally compiled code part, controlled by:

#if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI

> And is this covering all the cases?
> 
yes

> Last thing, how have you tested and reproduced the issue?
> 
The CI bot report included a link to the kernel config. So it was easy
to understand the root cause of the issue. I could reproduce it by setting:
I2C_I801=y
I2C_MUX=m
I2C_MUX_GPIO=m
This config was also used to test the fix.

Underlying reason for the issue is that i801 has a code dependency on i2c_mux,
but not on i2c_mux_gpio.

> Thanks,
> Andi
> 
Heiner

>> Handling the dependencies in the code would become unnecessarily
>> complex, therefore create a new config symbol.
>>
>> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Andi Shyti]

Hi Heiner,

On Fri, Apr 05, 2024 at 07:57:24AM +0200, Heiner Kallweit wrote:
> On 05.04.2024 02:54, Andi Shyti wrote:
> > Hi Heiner,
> > 
> > first of all, thanks for the fast reaction!
> > 
> > On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote:
> >> The original change adds usage of i2c_root_adapter(), which is
> >> implemented in i2c-mux.c. Therefore we can't use the multiplexing
> >> if I2C_I801=y and I2C_MUX=m.
> > 
> > What is wrong with select I2C_MUX?
> > 
> It would solve the issue, but:
> We would uselessly enable I2C_MUX also if I2C_MUX_GPIO or DMI are disabled.
> W/o them the mux part in i801 is a no-op. The call to i2c_root_adapter()
> is in a conditionally compiled code part, controlled by:
> 
> #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
> 
> > And is this covering all the cases?
> > 
> yes
> 
> > Last thing, how have you tested and reproduced the issue?
> > 
> The CI bot report included a link to the kernel config. So it was easy
> to understand the root cause of the issue. I could reproduce it by setting:
> I2C_I801=y
> I2C_MUX=m
> I2C_MUX_GPIO=m
> This config was also used to test the fix.
> 
> Underlying reason for the issue is that i801 has a code dependency on i2c_mux,
> but not on i2c_mux_gpio.

Even though it might look trivial, I need to reproduce the issue
first and check the various cases.

Because I'm not at home and, anyway, the report has come late in
the week, I can queue this up in the next week's pull request.

I'm not extremely happy with the new _CONFIG but I understand the
reason behind it.

Thanks,
Andi

> > Thanks,
> > Andi
> > 
> Heiner
> 
> >> Handling the dependencies in the code would become unnecessarily
> >> complex, therefore create a new config symbol.
> >>
> >> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Heiner Kallweit]

On 06.04.2024 02:29, Andi Shyti wrote:
> Hi Heiner,
> 
> On Fri, Apr 05, 2024 at 07:57:24AM +0200, Heiner Kallweit wrote:
>> On 05.04.2024 02:54, Andi Shyti wrote:
>>> Hi Heiner,
>>>
>>> first of all, thanks for the fast reaction!
>>>
>>> On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote:
>>>> The original change adds usage of i2c_root_adapter(), which is
>>>> implemented in i2c-mux.c. Therefore we can't use the multiplexing
>>>> if I2C_I801=y and I2C_MUX=m.
>>>
>>> What is wrong with select I2C_MUX?
>>>
>> It would solve the issue, but:
>> We would uselessly enable I2C_MUX also if I2C_MUX_GPIO or DMI are disabled.
>> W/o them the mux part in i801 is a no-op. The call to i2c_root_adapter()
>> is in a conditionally compiled code part, controlled by:
>>
>> #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
>>
>>> And is this covering all the cases?
>>>
>> yes
>>
>>> Last thing, how have you tested and reproduced the issue?
>>>
>> The CI bot report included a link to the kernel config. So it was easy
>> to understand the root cause of the issue. I could reproduce it by setting:
>> I2C_I801=y
>> I2C_MUX=m
>> I2C_MUX_GPIO=m
>> This config was also used to test the fix.
>>
>> Underlying reason for the issue is that i801 has a code dependency on i2c_mux,
>> but not on i2c_mux_gpio.
> 
> Even though it might look trivial, I need to reproduce the issue
> first and check the various cases.
> 
> Because I'm not at home and, anyway, the report has come late in
> the week, I can queue this up in the next week's pull request.
> 
> I'm not extremely happy with the new _CONFIG but I understand the
> reason behind it.
> 
Whether an additional config symbol or an equivalent expression for conditional
compiling is better may be a matter of personal taste. I had a similar issue
in r8169 and followed the suggestion to put the logic to a new config symbol.
a2634a5ffcaf ("r8169: fix building with CONFIG_LEDS_CLASS=m")
Therefore I went the same way here.
Sometimes I encounter concerns when using macros like IS_REACHABLE().

> Thanks,
> Andi
> 
Heiner

>>> Thanks,
>>> Andi
>>>
>> Heiner
>>
>>>> Handling the dependencies in the code would become unnecessarily
>>>> complex, therefore create a new config symbol.
>>>>
>>>> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Heiner Kallweit]

On 06.04.2024 02:29, Andi Shyti wrote:
> Hi Heiner,
> 
> On Fri, Apr 05, 2024 at 07:57:24AM +0200, Heiner Kallweit wrote:
>> On 05.04.2024 02:54, Andi Shyti wrote:
>>> Hi Heiner,
>>>
>>> first of all, thanks for the fast reaction!
>>>
>>> On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote:
>>>> The original change adds usage of i2c_root_adapter(), which is
>>>> implemented in i2c-mux.c. Therefore we can't use the multiplexing
>>>> if I2C_I801=y and I2C_MUX=m.
>>>
>>> What is wrong with select I2C_MUX?
>>>
>> It would solve the issue, but:
>> We would uselessly enable I2C_MUX also if I2C_MUX_GPIO or DMI are disabled.
>> W/o them the mux part in i801 is a no-op. The call to i2c_root_adapter()
>> is in a conditionally compiled code part, controlled by:
>>
>> #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI
>>
>>> And is this covering all the cases?
>>>
>> yes
>>
>>> Last thing, how have you tested and reproduced the issue?
>>>
>> The CI bot report included a link to the kernel config. So it was easy
>> to understand the root cause of the issue. I could reproduce it by setting:
>> I2C_I801=y
>> I2C_MUX=m
>> I2C_MUX_GPIO=m
>> This config was also used to test the fix.
>>
>> Underlying reason for the issue is that i801 has a code dependency on i2c_mux,
>> but not on i2c_mux_gpio.
> 
> Even though it might look trivial, I need to reproduce the issue
> first and check the various cases.
> 
> Because I'm not at home and, anyway, the report has come late in
> the week, I can queue this up in the next week's pull request.
> 
> I'm not extremely happy with the new _CONFIG but I understand the
> reason behind it.
> 
Another simple solution would be to move the implementation of
i2c_root_adapter() from i2c mux to i2c core. It just uses
i2c_parent_is_i2c_adapter() which is an inline function of
i2c core. What do you think?

> Thanks,
> Andi
> 
Heiner

>>> Thanks,
>>> Andi
>>>
>> Heiner
>>
>>>> Handling the dependencies in the code would become unnecessarily
>>>> complex, therefore create a new config symbol.
>>>>
>>>> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Wolfram Sang]

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


> Another simple solution would be to move the implementation of
> i2c_root_adapter() from i2c mux to i2c core. It just uses
> i2c_parent_is_i2c_adapter() which is an inline function of
> i2c core. What do you think?

I have no objections putting i2c_root_adapter() into the core. I think,
however, that this patch makes the code a tad more readable. What is the
downside of the symbol (despite we have way too many of those in
general)?


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

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Heiner Kallweit]

On 08.04.2024 09:42, Wolfram Sang wrote:
> 
>> Another simple solution would be to move the implementation of
>> i2c_root_adapter() from i2c mux to i2c core. It just uses
>> i2c_parent_is_i2c_adapter() which is an inline function of
>> i2c core. What do you think?
> 
> I have no objections putting i2c_root_adapter() into the core. I think,
> however, that this patch makes the code a tad more readable. What is the
> downside of the symbol (despite we have way too many of those in
> general)?
> 
I have no strong preference here, Andi mention that a new config symbol
wouldn't be his preferred approach. Therefore I brought up moving the
function to i2c core as an alternative.
Maybe he can elaborate on the reasoning.

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Andi Shyti]

Hi Wolfram,

On Mon, Apr 08, 2024 at 09:42:11AM +0200, Wolfram Sang wrote:
> > Another simple solution would be to move the implementation of
> > i2c_root_adapter() from i2c mux to i2c core. It just uses
> > i2c_parent_is_i2c_adapter() which is an inline function of
> > i2c core. What do you think?
> 
> I have no objections putting i2c_root_adapter() into the core. I think,
> however, that this patch makes the code a tad more readable. What is the
> downside of the symbol (despite we have way too many of those in
> general)?

That's the only reason: "having too many of those in general".

But i2c_root_adapter(), I believe should belong to the mux, even
though it makes life easier moving it to the core.

Heiner, I tested your patch and it works as expected. If no
objections or suggestions, I will push it tomorrow.

Andi

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Wolfram Sang]

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

On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote:
> The original change adds usage of i2c_root_adapter(), which is
> implemented in i2c-mux.c. Therefore we can't use the multiplexing
> if I2C_I801=y and I2C_MUX=m.
> Handling the dependencies in the code would become unnecessarily
> complex, therefore create a new config symbol.
> 
> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v2] i2c: i801: Fix missing Kconfig dependency

[Andi Shyti]

Hi Heiner,

On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote:
> The original change adds usage of i2c_root_adapter(), which is
> implemented in i2c-mux.c. Therefore we can't use the multiplexing
> if I2C_I801=y and I2C_MUX=m.
> Handling the dependencies in the code would become unnecessarily
> complex, therefore create a new config symbol.
> 
> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

I forgot that the original patch was queued for the next merge
window, so that this patch goes through i2c-host.

Applied to i2c/i2c-host on

git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Thank you,
Andi

Patches applied
===============
[1/1] i2c: i801: Fix missing Kconfig dependency
      commit: ab9713bb88443f8791e2917833ff1ed802046b2f