Re: [PATCH RFC] i2c: i801: Add i801_register_jc42, similar to i2c_register_spd

Jean Delvare
2023-11-14T14:00:11Z

Hi Heiner,

On Wed, 08 Nov 2023 11:28:45 +0100, Heiner Kallweit wrote:
> On 08.11.2023 08:27, Heiner Kallweit wrote:
> > As discussed, this is a RFC version of changing jc42 auto-detection
> > with the goal to get rid of I2C_CLASS_SPD completely mid-term.
> >
> > Code of i801_jc42_probe() was copied from jc42 driver, just w/o
> > the device id check. I think it's safe enough w/o this check.
> >
> > I don't have hw to test this, therefore it's compile-tested only.
> >
> > Link:
> > https://lore.kernel.org/linux-i2c/a22978a4-88e4-46f4-b71c-032b22321599@gmail.com/
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> ---
> > drivers/i2c/busses/i2c-i801.c | 48
> > ++++++++++++++++++++++++++++++++--- 1 file changed, 44
> > insertions(+), 4 deletions(-)
>
> That's quite some code for more or less nothing. I2C_CLASS_SPD is
> relevant only for users:
> - having one of the specific old ASUS machines with i2c-muxing
> - having RAM with a jc42-compatible temperature sensor
> - manually loading module jc42 to expose the temp sensor

People running such systems would typically run sensors-detect to setup
their hardware monitoring, so the jc42 driver would be loaded at boot
by the lm-sensors service. This is "manual" from the kernel's
perspective, but still this is integrated and has been working for
years. If you break that, this is a functional regression.

There is nothing fundamentally specific to i2c-i801 or these Asus
boards here. The only reason why we are discussing it in this context
is because SMBus multiplexing adds some implementation constraints, and
it turns out that right now only the i2c-i801 driver has support for
PC-style boards with multiplexed SMBus.

The solution however needs to work on all PC-style systems, Intel or
AMD (or anything else that exists), with SMBus multiplexed or not.

Originally, I2C_CLASS_SPD was there, the eeprom and jc42 drivers were
using it, and just loading these drivers would instantiate all the
devices. This is the level of user-friendliness we must aim at.

Now, the eeprom driver is gone, so class-based SPD device support no
longer exists. This was replaced by i2c_register_spd(), but is
currently only working on non-multiplexed Intel-based systems. Ideally
this should be extended to non-Intel systems (I'm surprised nobody
reported about that regression yet) and Intel systems with multiplexed
SMBus (that would be achieved by calling i2c_register_spd explicitly on
these segments, possibly with a few changes, as discussed earlier).

The jc42 driver still works the way it used to. If you remove
I2C_CLASS_SPD, this will still work on most non-SMBus-multiplexed
systems (thanks to I2C_CLASS_HWMON), but will stop working on the
multiplexed Asus boards (because the bus segments which host the memory
modules don't have I2C_CLASS_HWMON, and can't have it), or any other
board using SMbus multiplexing which we would like to support in the
future. I believe there are still many such systems out there, as
server systems with more than 8 memory slots are legions and this is
the hard limit of how many memory slots can be connected to a single
SMBus segment. We could receive a request to support such recent server
boards at any time, so we better be ready for it.

This is the reason why I asked for jc42 devices to be instantiated
automatically on multiplexed SMBus segments. The function doing that
should however not live in the i2c-i801 driver, it must be usable by
any SMBus controller driver. Also, while we only need this for
multiplexed SMBus segments, we could still use it everywhere
i2c_register_spd() is used, so that jc42 devices get instantiated at
boot-time without the need for user-space support.

> From a maintenance point of view the easiest solution would be:
> - set flag I2C_CLASS_DEPRECATED in addition to I2C_CLASS_SPD
> to encourage potential users to switch to explicit instantiating

Bad idea. That's just going to spam a warning message on millions of
systems while there's just nothing most users can do about it. That's
not helpful, we are already aware of the problem, and we are the guys
looking into it.

> - wait few kernel versions and remove class-based instantiation

Assuming you only refer to I2C_CLASS_SPD and not I2C_CLASS_HWMON, then
yes. I2C_CLASS_HWMON must stay as there's no suitable replacement for
it yet (and sadly I can't foresee any).

I think the steps to follow are:
* Extend i2c_register_spd() to support up to 8 memory modules (I'm
already working on that, patch is coming).
* Call i2c_register_spd() on the mux'd SMBus segments on the Asus
boards.
* Extend i2c_register_spd() to also instantiate jc42 devices in
addition to at24 (or ee1004) devices. I think this is better than
writing a separate function as I initially suggested. The reason why
I think so is because the SPD EEPROM does contain the information
about thermal sensor presence. So the code which instantiates the at24
or ee1004 device could also read from it to figure out whether a jc42
device must be instantiated. This removes the need for probing.
* Get rid of I2C_CLASS_SPD.

--
Jean Delvare
SUSE L3 Support