public inbox for linux-hwmon@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hwmon: (spd5118) Explicitly enable temperature sensor in probe function
@ 2026-01-31 15:20 Guenter Roeck
  2026-01-31 19:20 ` TINSAE TADESSE
  2026-02-03 19:23 ` Kurt Borja
  0 siblings, 2 replies; 5+ messages in thread
From: Guenter Roeck @ 2026-01-31 15:20 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Guenter Roeck, Armin Wolf, Kurt Borja, Tinsae Tadesse

Instantiating the driver does not make sense if the temperature sensor
is disabled, so enable it unconditionally in the probe function.

If that fails, write operations to the chip are likely disabled
by the I2C controller. Bail out with an error message if that happens.

Cc: Armin Wolf <W_Armin@gmx.de>
Cc: Kurt Borja <kuurtb@gmail.com>
Cc: Tinsae Tadesse <tinsaetadesse2015@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/spd5118.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 5da44571b6a0..d8834d4d980b 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -552,6 +552,20 @@ static int spd5118_common_probe(struct device *dev, struct regmap *regmap,
 	if (!spd5118_vendor_valid(bank, vendor))
 		return -ENODEV;
 
+	/*
+	 * Some I2C controllers write protect the address range used by SPD5118
+	 * compliant chips. This makes the chips effectively unaccessible since
+	 * the driver needs to be able to set the page in the legacy mode
+	 * register, and it needs to be able to disable the temperature sensor
+	 * during suspend. Check if writes to the chip are possible by
+	 * explicitly enabling the temperature sensor. Bail out if that fails.
+	 */
+	err = regmap_write_bits(regmap, SPD5118_REG_TEMP_CONFIG,
+				SPD5118_TS_DISABLE, 0);
+	if (err)
+		return dev_err_probe(dev, err,
+				     "Failed to enable temperature sensor (write protected ?)\n");
+
 	data->regmap = regmap;
 	mutex_init(&data->nvmem_lock);
 	dev_set_drvdata(dev, data);
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] hwmon: (spd5118) Explicitly enable temperature sensor in probe function
  2026-01-31 15:20 [PATCH] hwmon: (spd5118) Explicitly enable temperature sensor in probe function Guenter Roeck
@ 2026-01-31 19:20 ` TINSAE TADESSE
  2026-02-03 19:23 ` Kurt Borja
  1 sibling, 0 replies; 5+ messages in thread
From: TINSAE TADESSE @ 2026-01-31 19:20 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Hardware Monitoring, Armin Wolf, Kurt Borja

On Sat, Jan 31, 2026 at 6:20 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Instantiating the driver does not make sense if the temperature sensor
> is disabled, so enable it unconditionally in the probe function.
>
> If that fails, write operations to the chip are likely disabled
> by the I2C controller. Bail out with an error message if that happens.
>
> Cc: Armin Wolf <W_Armin@gmx.de>
> Cc: Kurt Borja <kuurtb@gmail.com>
> Cc: Tinsae Tadesse <tinsaetadesse2015@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/spd5118.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 5da44571b6a0..d8834d4d980b 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -552,6 +552,20 @@ static int spd5118_common_probe(struct device *dev, struct regmap *regmap,
>         if (!spd5118_vendor_valid(bank, vendor))
>                 return -ENODEV;
>
> +       /*
> +        * Some I2C controllers write protect the address range used by SPD5118
> +        * compliant chips. This makes the chips effectively unaccessible since
> +        * the driver needs to be able to set the page in the legacy mode
> +        * register, and it needs to be able to disable the temperature sensor
> +        * during suspend. Check if writes to the chip are possible by
> +        * explicitly enabling the temperature sensor. Bail out if that fails.
> +        */
> +       err = regmap_write_bits(regmap, SPD5118_REG_TEMP_CONFIG,
> +                               SPD5118_TS_DISABLE, 0);
> +       if (err)
> +               return dev_err_probe(dev, err,
> +                                    "Failed to enable temperature sensor (write protected ?)\n");
> +
>         data->regmap = regmap;
>         mutex_init(&data->nvmem_lock);
>         dev_set_drvdata(dev, data);
> --
> 2.45.2
>

Hi Guenter,

Tested this patch on my system, and it actually fixes the issue.

[   11.523971] i2c-core: driver [spd5118] registered
[   28.040686] i801_smbus 0000:00:1f.4: SPD Write Disable is set
[   28.041003] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt
[   28.078412] i801_smbus 0000:00:1f.4: No response
[   28.078643] i801_smbus 0000:00:1f.4: No response
[   28.082927] i2c i2c-14: Creating spd5118 at 0x52
[   28.087685] spd5118 14-0052: probe
[   28.092707] i801_smbus 0000:00:1f.4: No response
[   28.092736] spd5118 14-0052: error -ENXIO: Failed to enable
temperature sensor (write protected ?)
[   28.094709] i2c i2c-14: client [spd5118] registered with bus id 14-0052
[   28.094936] i801_smbus 0000:00:1f.4: No response
[   28.095143] i801_smbus 0000:00:1f.4: No response
[   28.095351] i801_smbus 0000:00:1f.4: No response
[   28.095558] i801_smbus 0000:00:1f.4: No response
[   28.095757] i801_smbus 0000:00:1f.4: No response
[   28.095956] i801_smbus 0000:00:1f.4: No response
[   28.096153] i801_smbus 0000:00:1f.4: No response
[   28.097139] i801_smbus 0000:00:1f.4: No response
[   28.097361] i801_smbus 0000:00:1f.4: No response
[   28.097567] i801_smbus 0000:00:1f.4: No response
[   78.689689] PM: suspend entry (s2idle)
[   80.616943] ACPI: EC: interrupt blocked
[   86.510434] ACPI: EC: interrupt unblocked
[   88.175249] PM: suspend exit

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hwmon: (spd5118) Explicitly enable temperature sensor in probe function
  2026-01-31 15:20 [PATCH] hwmon: (spd5118) Explicitly enable temperature sensor in probe function Guenter Roeck
  2026-01-31 19:20 ` TINSAE TADESSE
@ 2026-02-03 19:23 ` Kurt Borja
  2026-02-03 19:49   ` Guenter Roeck
  1 sibling, 1 reply; 5+ messages in thread
From: Kurt Borja @ 2026-02-03 19:23 UTC (permalink / raw)
  To: Guenter Roeck, Hardware Monitoring; +Cc: Armin Wolf, Kurt Borja, Tinsae Tadesse

On Sat Jan 31, 2026 at 10:20 AM -05, Guenter Roeck wrote:
> Instantiating the driver does not make sense if the temperature sensor
> is disabled, so enable it unconditionally in the probe function.
>
> If that fails, write operations to the chip are likely disabled
> by the I2C controller. Bail out with an error message if that happens.

Hi Guenter,

As I mentiond in the other thread, after applying this patch the probe
fails

	spd5118 17-0051: error -ENXIO: Failed to enable temperature sensor (write protected ?)
	spd5118 17-0053: error -ENXIO: Failed to enable temperature sensor (write protected ?)

This would be a regression in my platform because, even though the
register is write protected, I can still get temperature readings just
fine (even after the resume error).

-- 
Thanks,
 ~ Kurt

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hwmon: (spd5118) Explicitly enable temperature sensor in probe function
  2026-02-03 19:23 ` Kurt Borja
@ 2026-02-03 19:49   ` Guenter Roeck
  2026-02-04  6:22     ` Kurt Borja
  0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2026-02-03 19:49 UTC (permalink / raw)
  To: Kurt Borja, Hardware Monitoring; +Cc: Armin Wolf, Tinsae Tadesse

On 2/3/26 11:23, Kurt Borja wrote:
> On Sat Jan 31, 2026 at 10:20 AM -05, Guenter Roeck wrote:
>> Instantiating the driver does not make sense if the temperature sensor
>> is disabled, so enable it unconditionally in the probe function.
>>
>> If that fails, write operations to the chip are likely disabled
>> by the I2C controller. Bail out with an error message if that happens.
> 
> Hi Guenter,
> 
> As I mentiond in the other thread, after applying this patch the probe
> fails
> 
> 	spd5118 17-0051: error -ENXIO: Failed to enable temperature sensor (write protected ?)
> 	spd5118 17-0053: error -ENXIO: Failed to enable temperature sensor (write protected ?)
> 
> This would be a regression in my platform because, even though the
> register is write protected, I can still get temperature readings just
> fine (even after the resume error).
> 

Yes, but after the next BIOS update it might not work anymore, and
it is impossible to suspend the system. On top of that, it instantiates
the spd eeprom which can not really be accessed because that code _does_
need to write into the chip.

There is no safe way for a spd5118 compliant chip to be accessed reliably
with write protection active. There is nothing we can do about that.

Guenter


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hwmon: (spd5118) Explicitly enable temperature sensor in probe function
  2026-02-03 19:49   ` Guenter Roeck
@ 2026-02-04  6:22     ` Kurt Borja
  0 siblings, 0 replies; 5+ messages in thread
From: Kurt Borja @ 2026-02-04  6:22 UTC (permalink / raw)
  To: Guenter Roeck, Kurt Borja, Hardware Monitoring; +Cc: Armin Wolf, Tinsae Tadesse

On Tue Feb 3, 2026 at 2:49 PM -05, Guenter Roeck wrote:
> On 2/3/26 11:23, Kurt Borja wrote:
>> On Sat Jan 31, 2026 at 10:20 AM -05, Guenter Roeck wrote:
>>> Instantiating the driver does not make sense if the temperature sensor
>>> is disabled, so enable it unconditionally in the probe function.
>>>
>>> If that fails, write operations to the chip are likely disabled
>>> by the I2C controller. Bail out with an error message if that happens.
>> 
>> Hi Guenter,
>> 
>> As I mentiond in the other thread, after applying this patch the probe
>> fails
>> 
>> 	spd5118 17-0051: error -ENXIO: Failed to enable temperature sensor (write protected ?)
>> 	spd5118 17-0053: error -ENXIO: Failed to enable temperature sensor (write protected ?)
>> 
>> This would be a regression in my platform because, even though the
>> register is write protected, I can still get temperature readings just
>> fine (even after the resume error).
>> 
>
> Yes, but after the next BIOS update it might not work anymore, and
> it is impossible to suspend the system. On top of that, it instantiates
> the spd eeprom which can not really be accessed because that code _does_
> need to write into the chip.
>
> There is no safe way for a spd5118 compliant chip to be accessed reliably
> with write protection active. There is nothing we can do about that.
>
> Guenter

I see... I had the module blacklisted anyway :P but I can imagine future
regression reports.

Thanks for the explaination!

-- 
Thanks,
 ~ Kurt

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-02-04  6:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-31 15:20 [PATCH] hwmon: (spd5118) Explicitly enable temperature sensor in probe function Guenter Roeck
2026-01-31 19:20 ` TINSAE TADESSE
2026-02-03 19:23 ` Kurt Borja
2026-02-03 19:49   ` Guenter Roeck
2026-02-04  6:22     ` Kurt Borja

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox