Re: [PATCH 1/3] hwmon: spd5118: Do not fail resume on temporary I2C errors

[Armin Wolf <W_Armin@gmx.de>]

Am 31.01.26 um 03:06 schrieb Guenter Roeck:

> On 1/30/26 17:54, Kurt Borja wrote:
>> On Fri Jan 30, 2026 at 8:21 PM -05, Guenter Roeck wrote:
>>> Hi Kurt,
>>>
>>> On 1/30/26 16:55, Kurt Borja wrote:
>>>> On Tue Jan 27, 2026 at 6:41 PM -05, Guenter Roeck wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Jan 27, 2026 at 10:23:24PM +0300, TINSAE TADESSE wrote:
>>>>>>
>>>>>> Disabling CONFIG_SENSORS_SPD5118_DETECT completely avoids the 
>>>>>> issue on
>>>>>> affected platforms,
>>>>>> even without any code changes. This confirms that the failures are
>>>>>> triggered specifically by automatic
>>>>>> SPD5118 instantiation on systems where the i801 controller enforces
>>>>>> SPD Write Disable.
>>>>>
>>>>> Thanks for confirming. Can you try if the patch below fixes the 
>>>>> problem ?
>>>>> It is a wild shot, but it might be worth a try.
>>>>>
>>>>> Thanks,
>>>>> Guenter
>>>>>
>>>>> ---
>>>>>   From b44c31c2c779a67827e3144b818cf21f5efacea1 Mon Sep 17 
>>>>> 00:00:00 2001
>>>>> From: Guenter Roeck <linux@roeck-us.net>
>>>>> Date: Tue, 27 Jan 2026 15:32:32 -0800
>>>>> Subject: [PATCH] hwmon: (spd5118) Explicitly enable temperature 
>>>>> sensor in
>>>>>    probe function
>>>>>
>>>>> 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 eror message if that happens.
>>>>>
>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>>> ---
>>>>>    drivers/hwmon/spd5118.c | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
>>>>> index 5da44571b6a0..3e0e780014f9 100644
>>>>> --- a/drivers/hwmon/spd5118.c
>>>>> +++ b/drivers/hwmon/spd5118.c
>>>>> @@ -552,6 +552,11 @@ static int spd5118_common_probe(struct device 
>>>>> *dev, struct regmap *regmap,
>>>>>        if (!spd5118_vendor_valid(bank, vendor))
>>>>>            return -ENODEV;
>>>>>    +    if (regmap_update_bits(regmap, SPD5118_REG_TEMP_CONFIG,
>>>>> +                   SPD5118_TS_DISABLE, 0) < 0)
>>>>> +        return dev_err_probe(dev, -ENODEV,
>>>>> +                     "Failed to enable temperature sensor\n");
>>>>> +
>>>>>        data->regmap = regmap;
>>>>>        mutex_init(&data->nvmem_lock);
>>>>>        dev_set_drvdata(dev, data);
>>>>
>>>> Hi Guenter,
>>>>
>>>> I'm experiencing the same issue reported in this thread. This patch 
>>>> does
>>>> not fix it for me :(.
>>>>
>>> Thanks for a note. Well, it was a wild shot, so it is not entirely 
>>> surprising
>>> that it didn't work. I suspect regmap doesn't actually write the 
>>> register
>>> if the value is unchanged. Another option might be to try writing a 
>>> value
>>
>> I tried forcing the write with
>>
>>     bool change;
>>     err = regmap_update_bits_base(regmap, SPD5118_REG_TEMP_CONFIG,
>>                       SPD5118_TS_DISABLE, 0, &change, false, true);
>>     if (err)
>>         return dev_err_probe(dev, err,
>>                      "Failed to enable temperature sensor\n");
>>
>> and it fails to probe
>>
>>     spd5118 17-0051: error -ENXIO: Failed to enable temperature sensor
>>     spd5118 17-0053: error -ENXIO: Failed to enable temperature sensor
>>
>
> Nice, I didn't know about that API function. And, even better, 
> 'change' can be NULL.
>
>>
>>> (e.g., 0x01) into register 0x13 or 0x14. Those are "clear status" 
>>> registers.
>>> If that fails, we would know that the chip is write protected.
>>
>> I'll try this later too.
>>
>
> Don't bother. I'll write up a patch using regmap_update_bits_base().
>
> Thanks!
> Guenter
>
Good idea, maybe regmap_write_bits() is exactly what we need for this.

Thanks,
Armin Wolf

>>>
>>> Thanks,
>>> Guenter
>>
>
>