Re: [PATCH v2 1/2] hw/sensor: Add SB-TSI Temperature Sensor Interface

[Philippe Mathieu-Daudé]

Hi Patrick,

On 13/1/22 00:26, Patrick Venture wrote:
> From: Hao Wu <wuhaotsh@google.com>
>
> SB Temperature Sensor Interface (SB-TSI) is an SMBus compatible
> interface that reports AMD SoC's Ttcl (normalized temperature),
> and resembles a typical 8-pin remote temperature sensor's I2C interface
> to BMC.
>
> This patch implements a basic AMD SB-TSI sensor that is
> compatible with the open-source data sheet from AMD and Linux
> kernel driver.
>
> Reference:
> Linux kernel driver:
> https://lkml.org/lkml/2020/12/11/968
> Register Map:
> https://developer.amd.com/wp-content/resources/56255_3_03.PDF
> (Chapter 6)
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> Reviewed-by: Doug Evans <dje@google.com>
> ---
>   hw/sensor/Kconfig         |   4 +
>   hw/sensor/meson.build     |   1 +
>   hw/sensor/tmp_sbtsi.c     | 365 ++++++++++++++++++++++++++++++++++++++
>   hw/sensor/trace-events    |   5 +
>   hw/sensor/trace.h         |   1 +
>   include/hw/sensor/sbtsi.h |  50 ++++++
>   meson.build               |   1 +
>   7 files changed, 427 insertions(+)
>   create mode 100644 hw/sensor/tmp_sbtsi.c
>   create mode 100644 hw/sensor/trace-events
>   create mode 100644 hw/sensor/trace.h
>   create mode 100644 include/hw/sensor/sbtsi.h

Since you are posting various patches, consider setting
the scripts/git.orderfile script up to ease on-list reviews.

> diff --git a/include/hw/sensor/sbtsi.h b/include/hw/sensor/sbtsi.h
> new file mode 100644
> index 0000000000..841891e89e
> --- /dev/null
> +++ b/include/hw/sensor/sbtsi.h
> @@ -0,0 +1,50 @@
> +/*
> + * AMD SBI Temperature Sensor Interface (SB-TSI)
> + *
> + * Copyright 2021 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +#ifndef QEMU_TMP_SBTSI_H
> +#define QEMU_TMP_SBTSI_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_SBTSI "sbtsi"
> +#define SBTSI(obj) OBJECT_CHECK(SBTSIState, (obj), TYPE_SBTSI)

SBTSIState is not declared outside of tmp_sbtsi.c, so I'd keep
SBTSI() there. Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>