Re: [PATCH 2/2] tmp105: Correct handling of temperature limit checks

["Cédric Le Goater" <clg@kaod.org>]

On 11/10/20 4:00 PM, Peter Maydell wrote:
> The TMP105 datasheet says that in Interrupt Mode (when TM==1) the device
> signals an alert when the temperature equals or exceeds the T_high value and
> then remains high until a device register is read or the device responds to
> the SMBUS Alert Response address, or the device is put into Shutdown Mode.
> Thereafter the Alert pin will only be re-signalled when temperature falls
> below T_low; alert can then be cleared in the same set of ways, and the
> device returns to its initial "alert when temperature goes above T_high"
> mode. (If this textual description is confusing, see figure 3 in the
> TI datasheet at https://www.ti.com/lit/gpn/tmp105 .)
> 
> We were misimplementing this as a simple "always alert if temperature is
> above T_high or below T_low" condition, which gives a spurious alert on
> startup if using the "T_high = 80 degrees C, T_low = 75 degrees C" reset
> limit values.
> 
> Implement the correct (hysteresis) behaviour by tracking whether we
> are currently looking for the temperature to rise over T_high or
> for it to fall below T_low. Our implementation of the comparator
> mode (TM==0) wasn't wrong, but rephrase it to match the way that
> interrupt mode is now handled for clarity.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

LGTM,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>  hw/misc/tmp105.c | 70 +++++++++++++++++++++++++++++++++++++++++-------
>  hw/misc/tmp105.h |  7 +++++
>  2 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/misc/tmp105.c b/hw/misc/tmp105.c
> index 0a4aad4854..d299d9b21b 100644
> --- a/hw/misc/tmp105.c
> +++ b/hw/misc/tmp105.c
> @@ -41,16 +41,40 @@ static void tmp105_alarm_update(TMP105State *s)
>              return;
>      }
>  
> -    if ((s->config >> 1) & 1) {					/* TM */
> -        if (s->temperature >= s->limit[1])
> -            s->alarm = 1;
> -        else if (s->temperature < s->limit[0])
> -            s->alarm = 1;
> +    if (s->config >> 1 & 1) {
> +        /*
> +         * TM == 1 : Interrupt mode. We signal Alert when the
> +         * temperature rises above T_high, and expect the guest to clear
> +         * it (eg by reading a device register).
> +         */
> +        if (s->detect_falling) {
> +            if (s->temperature < s->limit[0]) {
> +                s->alarm = 1;
> +                s->detect_falling = false;
> +            }
> +        } else {
> +            if (s->temperature >= s->limit[1]) {
> +                s->alarm = 1;
> +                s->detect_falling = true;
> +            }
> +        }
>      } else {
> -        if (s->temperature >= s->limit[1])
> -            s->alarm = 1;
> -        else if (s->temperature < s->limit[0])
> -            s->alarm = 0;
> +        /*
> +         * TM == 0 : Comparator mode. We signal Alert when the temperature
> +         * rises above T_high, and stop signalling it when the temperature
> +         * falls below T_low.
> +         */
> +        if (s->detect_falling) {
> +            if (s->temperature < s->limit[0]) {
> +                s->alarm = 0;
> +                s->detect_falling = false;
> +            }
> +        } else {
> +            if (s->temperature >= s->limit[1]) {
> +                s->alarm = 1;
> +                s->detect_falling = true;
> +            }
> +        }
>      }
>  
>      tmp105_interrupt_update(s);
> @@ -197,6 +221,29 @@ static int tmp105_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool detect_falling_needed(void *opaque)
> +{
> +    TMP105State *s = opaque;
> +
> +    /*
> +     * We only need to migrate the detect_falling bool if it's set;
> +     * for migration from older machines we assume that it is false
> +     * (ie temperature is not out of range).
> +     */
> +    return s->detect_falling;
> +}
> +
> +static const VMStateDescription vmstate_tmp105_detect_falling = {
> +    .name = "TMP105/detect-falling",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = detect_falling_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(detect_falling, TMP105State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_tmp105 = {
>      .name = "TMP105",
>      .version_id = 0,
> @@ -212,6 +259,10 @@ static const VMStateDescription vmstate_tmp105 = {
>          VMSTATE_UINT8(alarm, TMP105State),
>          VMSTATE_I2C_SLAVE(i2c, TMP105State),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_tmp105_detect_falling,
> +        NULL
>      }
>  };
>  
> @@ -224,6 +275,7 @@ static void tmp105_reset(I2CSlave *i2c)
>      s->config = 0;
>      s->faults = tmp105_faultq[(s->config >> 3) & 3];
>      s->alarm = 0;
> +    s->detect_falling = false;
>  
>      s->limit[0] = 0x4b00; /* T_LOW, 75 degrees C */
>      s->limit[1] = 0x5000; /* T_HIGH, 80 degrees C */
> diff --git a/hw/misc/tmp105.h b/hw/misc/tmp105.h
> index e5198fce80..7c97071ad7 100644
> --- a/hw/misc/tmp105.h
> +++ b/hw/misc/tmp105.h
> @@ -43,6 +43,13 @@ struct TMP105State {
>      int16_t limit[2];
>      int faults;
>      uint8_t alarm;
> +    /*
> +     * The TMP105 initially looks for a temperature rising above T_high;
> +     * once this is detected, the condition it looks for next is the
> +     * temperature falling below T_low. This flag is false when initially
> +     * looking for T_high, true when looking for T_low.
> +     */
> +    bool detect_falling;
>  };
>  
>  #endif
>