Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <0d1b4fed-7ded-88fc-3c37-4f859fc505c1@roeck-us.net>
Date:   Wed, 19 Oct 2022 14:51:05 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH] hwmon: (jc42) Restore the min/max/critical temperatures
 on resume
Content-Language: en-US
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        linux-hwmon@vger.kernel.org
Cc:     jdelvare@suse.com, linux-kernel@vger.kernel.org
References: <20221019214108.220319-1-martin.blumenstingl@googlemail.com>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20221019214108.220319-1-martin.blumenstingl@googlemail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 10/19/22 14:41, Martin Blumenstingl wrote:
> The JC42 compatible thermal sensor on Kingston KSM32ES8/16ME DIMMs
> (using Micron E-Die) is an ST Microelectronics STTS2004 (manufacturer
> 0x104a, device 0x2201). It does not keep the previously programmed
> minimum, maximum and critical temperatures after system suspend and
> resume (which is a shutdown / startup cycle for the JC42 temperature
> sensor). This results in an alarm on system resume because the hardware
> default for these values is 0°C (so any environment temperature greater
> than 0°C will trigger the alarm).
> 
> Example before system suspend:
>    jc42-i2c-0-1a
>    Adapter: SMBus PIIX4 adapter port 0 at 0b00
>    temp1:        +34.8°C  (low  =  +0.0°C)
>                           (high = +85.0°C, hyst = +85.0°C)
>                           (crit = +95.0°C, hyst = +95.0°C)
> 
> Example after system resume (without this change):
>    jc42-i2c-0-1a
>    Adapter: SMBus PIIX4 adapter port 0 at 0b00
>    temp1:        +34.8°C  (low  =  +0.0°C)             ALARM (HIGH, CRIT)
>                           (high =  +0.0°C, hyst =  +0.0°C)
>                           (crit =  +0.0°C, hyst =  +0.0°C)
> 
> Apply the previously read or previously programmed temperature limits on
> system resume. This fixes the alarm due to the hardware defaults of 0°C
> because the previously applied limits (from a userspace setting) are
> re-applied on system resume.
> 
> Fixes: 175c490c9e7f ("hwmon: (jc42) Add support for STTS2004 and AT30TSE004")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> This is my first change to jc42. I tried to be defensive with applying
> the previous values by only configuring them if they are known valid. I
> only have this one set of JC42 compatible sensors but I can adapt the
> code here based on your suggestions.
> 
> 
>   drivers/hwmon/jc42.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> index 30888feaf589..f98b28ff10ad 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -558,6 +558,19 @@ static int jc42_resume(struct device *dev)
>   	data->config &= ~JC42_CFG_SHUTDOWN;
>   	i2c_smbus_write_word_swapped(data->client, JC42_REG_CONFIG,
>   				     data->config);
> +
> +	if (data->valid || data->temp[t_min])

This contradicts "with applying the previous values by only configuring
them if they are known valid". It explicitly applies the values if they
are marked as not valid, and it also applies the values if they are 0
(I don't really see the value of doing that).

Sorry, I don't understand the logic. Did you mean to use "&&" instead
of "||" ?

Thanks,
Guenter

> +		i2c_smbus_write_word_swapped(data->client, temp_regs[t_min],
> +					     data->temp[t_min]);
> +
> +	if (data->valid || data->temp[t_max])
> +		i2c_smbus_write_word_swapped(data->client, temp_regs[t_max],
> +					     data->temp[t_max]);
> +
> +	if (data->valid || data->temp[t_crit])
> +		i2c_smbus_write_word_swapped(data->client, temp_regs[t_crit],
> +					     data->temp[t_crit]);
> +
>   	return 0;
>   }
>