Re: [PATCH] i2c: i801: Use I2C_CLASS_HWMON for i2c mux children

[Heiner Kallweit <hkallweit1@gmail.com>]

On 06.11.2023 15:50, Jean Delvare wrote:
> Hi Heiner,
> 
> On Tue, 31 Oct 2023 21:30:22 +0100, Heiner Kallweit wrote:
>> In case there ever should be a jc42-driven device on a muxed child bus,
>> we may end up with multiple logical devices, see linked discussion.
>>
>> jc42 is the only remaining i2c client device driver supporting
>> I2C_CLASS_SPD, however it supports I2C_CLASS_HWMON too. Therefore
>> we can safely switch to I2C_CLASS_HWMON on the muxed child busses.
>> IMO it's more logical anyway that the muxed child busses support
>> the same class that the parent supports if it's not muxed.
>>
>> This change is one further step towards removing I2C_CLASS_SPD.
>>
>> Link: https://www.spinics.net/lists/linux-i2c/msg65458.html
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 070999139..8c9bb2e09 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1298,7 +1298,7 @@ static struct i801_mux_config i801_mux_config_asus_z8_d12 = {
>>  	.gpio_chip = "gpio_ich",
>>  	.values = { 0x02, 0x03 },
>>  	.n_values = 2,
>> -	.classes = { I2C_CLASS_SPD, I2C_CLASS_SPD },
>> +	.classes = { I2C_CLASS_HWMON, I2C_CLASS_HWMON },
>>  	.gpios = { 52, 53 },
>>  	.n_gpios = 2,
>>  };
>> @@ -1307,7 +1307,7 @@ static struct i801_mux_config i801_mux_config_asus_z8_d18 = {
>>  	.gpio_chip = "gpio_ich",
>>  	.values = { 0x02, 0x03, 0x01 },
>>  	.n_values = 3,
>> -	.classes = { I2C_CLASS_SPD, I2C_CLASS_SPD, I2C_CLASS_SPD },
>> +	.classes = { I2C_CLASS_HWMON, I2C_CLASS_HWMON, I2C_CLASS_HWMON },
>>  	.gpios = { 52, 53 },
>>  	.n_gpios = 2,
>>  };
>> @@ -1395,6 +1395,9 @@ static void i801_add_mux(struct i801_priv *priv)
>>  
>>  	mux_config = id->driver_data;
>>  
>> +	/* Parent and mux children class support must not overlap */
>> +	priv->adapter.class = 0;
> 
> There may also be a hardware monitoring device on the parent segment on
> these boards. With this change, that device will no longer be detected.
> That would be a regression, so I have to nack this proposed change,
> sorry.
> 
Hmm, right. To me it seems we have a fundamental problem:
The parent has no way to find out whether a detected device sits on
the parent segment or whether it belongs to a muxed child segment.
In the latter case the device may show up twice, as you mentioned
before.
And the core check "supported classes of parent and child must not
overlap" doesn't consider that we may have e.g. hwmon devices
on parent segment and on muxed child segment too.

> The only way forward I can think of (if we want to get rid of
> I2C_CLASS_SPD) would be to remove device auto-detection on children
> segments completely (.class = 0 for them) and instead actively probe
> for SPD EEPROMs and JC42 thermal sensors on these segments. For SPD we
> should be able to just reuse i2c_register_spd(). For JC42 we would need
> similar code, which doesn't exist yet.
> 

Regarding the first part, removing auto-detection on muxed children:
We have only two i2c mux drivers supporting auto-detection, i2c-mux-gpio
and i2c-mux-reg. Both do not support auto-detection for DT-configured
systems.
Only user for i2c-mux-gpio is i801. Only user for i2c-mux-reg is
drivers/platform/x86/mlx-platform, and it doesn't make use of
auto-detection.
So I think as first step we should remove auto-detection support from
i2c-mux-reg.
Then we could do the same for i2c-mux-gpio, and afterwards remove the
class parameter from i2c_mux_add_adapter().
So we need an "i2c_register_jc42()" only for use by i801.
What's good, because we can make this function private to i801 and
won't attract any potential additional users.

+ i2c-mux-gpio and i2c-mux-reg maintainers