Re: [PATCH v3] hwmon: (spd5118) Select page 0 unconditionally during probe
From: Guenter Roeck
Date: Tue Sep 01 2026 - 13:50:43 EST
On 9/1/26 09:52, Armin Wolf wrote:
Am 01.09.26 um 18:35 schrieb Guenter Roeck:
On 9/1/26 09:09, Armin Wolf wrote:
Am 01.09.26 um 00:22 schrieb Guenter Roeck:Good point.
On 8/31/26 12:34, Armin Wolf wrote:This would prevent the driver from being forward-compatible to newer standard revisions.
Some Intel i2c controllers can be configured by the BIOS to reject
writes to the SPD device. This often causes problems when the register
page needs to be changed, usually during resume.
Avoid probing on affected devices by unconditionally selecting page 0
by writing the SPD5118_REG_I2C_LEGACY_MODE register during probe.
This will fail on affected controllers and thus prevent the driver
from probing.
Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
---
Changes since v2:
- restore original register content if i2c init fails
Changes since v1:
- Avoid zeroing reserved bits inside SPD5118_REG_I2C_LEGACY_MODE
---
drivers/hwmon/spd5118.c | 55 ++++++++++++++++-------------------------
1 file changed, 21 insertions(+), 34 deletions(-)
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 9724cf70b61d..038d9e6e6a73 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -637,44 +637,31 @@ static int spd5118_i2c_init(struct i2c_client *client)
I2C_FUNC_SMBUS_WORD_DATA))
return -ENODEV;
- regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
- if (regval < 0 || (regval && regval != 0x5118))
- return -ENODEV;
-
Thinking about it, Sashiko has a point here. I think we should keep
the above code to have an early exit point.
/*
- * If the device type registers return 0, it is possible that the chip
- * has a non-zero page selected and takes the specification literally,
+ * We must first select page 0 to ensure that we can reliably read
+ * the volatile registers on chips that take the specification literally,
* i.e. disables access to volatile registers besides the page register
* if the page is not 0. The Renesas/ITD SPD5118 Hub Controller is known
- * to show this behavior. Try to identify such chips.
+ * to show this behavior.
+ *
+ * We must also perform an unconditional register write to detect if
+ * the i2c controller blocks write accesses to the SPD device. Some Intel
+ * controllers might be configured by the BIOS to do this.
*/
- if (!regval) {
- /* Vendor ID registers must also be 0 */
- regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
- if (regval)
- return -ENODEV;
-
- /* The selected page in MR11 must not be 0 */
- mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
- if (mode < 0 || (mode & ~SPD5118_LEGACY_MODE_MASK) ||
- !(mode & SPD5118_LEGACY_PAGE_MASK))
- return -ENODEV;
-
- err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE,
- mode & SPD5118_LEGACY_MODE_ADDR);
- if (err)
- return -ENODEV;
-
- /*
- * If the device type registers are still bad after selecting
- * page 0, this is not a SPD5118 device. Restore original
- * legacy mode register value and abort.
- */
- regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
- if (regval != 0x5118) {
- i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
- return -ENODEV;
- }
+ mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
+ if (mode < 0)
+ return mode;
+
I also wonder if we should strengthen this and bail out if any of bit 3..7 is
set. 2-byte addressing is known not to work in the first place, and bit 4..7
are reserved in the supported version of the standard and should always be 0.
Also, if we keep above check, we should only only get here if the page is != 0.
That means the register value has to be 0x01..0x07. So we could bail out with
if (!mode || (mode & ~SPD5118_LEGACY_PAGE_MASK))
return -ENODEV;
followed by
err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, 0);
Thoughts ?
Honestly i think that users should rely on the detection feature if they are not sure
if a given device is really a spd5118 device. Manually instantiating a spd5118 device
on a given bus address tells the kernel that there _is_ a valid spd5118 device at this
address.
Otherwise we would basically duplicate parts of the detect callback inside spd5118_i2c_init().
This _is_ the detect callback inside spd5118_i2c_init(). Problem is that the device
currently instantiates if bit 3 of MR11 is set, even though it is known not to work.
Prior to commit 7bf5a11dde2c ("hwmon: spd5118: Remove 16-bit addressing") it would
at least bail out if the controller does not support I2C_FUNC_I2C. That means we'll
at least need to check for bit 3 (SPD5118_LEGACY_MODE_ADDR) and bail out if the bit
is set.
Thanks,
Guenter
Good point, we can indeed check for that. But the early exit point suggested by Sashiko
is a bit overblown IMHO.
I disagree. This is a critical component, and overwriting SPD data would be fatal.
The additional read may be a nuisance, but does not hurt. One thing we could do though
is to only read SPD5118_REG_TYPE again if the first read failed.
Thanks,
Guenter
Thanks,
Armin Wolf