Re: [PATCH v3 7/9] hw/sensor: add Intersil ISL69260 device model

[Philippe Mathieu-Daudé]

On 2/3/22 02:50, Titus Rwantare wrote:
> Signed-off-by: Titus Rwantare <titusr@google.com>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> ---
>   MAINTAINERS                      |   3 +
>   hw/arm/Kconfig                   |   1 +
>   hw/sensor/Kconfig                |   5 +
>   hw/sensor/isl_pmbus_vr.c         | 211 +++++++++++++++++
>   hw/sensor/meson.build            |   1 +
>   include/hw/sensor/isl_pmbus_vr.h |  50 ++++
>   tests/qtest/isl_pmbus_vr-test.c  | 394 +++++++++++++++++++++++++++++++
>   tests/qtest/meson.build          |   1 +
>   8 files changed, 666 insertions(+)
>   create mode 100644 hw/sensor/isl_pmbus_vr.c
>   create mode 100644 include/hw/sensor/isl_pmbus_vr.h
>   create mode 100644 tests/qtest/isl_pmbus_vr-test.c

> diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig
> index 215944decc..a834d2f814 100644
> --- a/hw/sensor/Kconfig
> +++ b/hw/sensor/Kconfig
> @@ -30,3 +30,8 @@ config LSM303DLHC_MAG
>       bool
>       depends on I2C
>       default y if I2C_DEVICES
> +
> +config ISL_PMBUS_VR
> +    bool
> +    depends on I2C

There is a PMBUS kconfig selector (which selects SMBUS -> I2C).

> +
> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> new file mode 100644
> index 0000000000..b3d24e40ab
> --- /dev/null
> +++ b/hw/sensor/isl_pmbus_vr.c
> @@ -0,0 +1,211 @@
> +/*
> + * PMBus device for Renesas Digital Multiphase Voltage Regulators
> + *
> + * Copyright 2021 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sensor/isl_pmbus_vr.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/visitor.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +
> +static uint8_t isl_pmbus_vr_read_byte(PMBusDevice *pmdev)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR,
> +                  "%s: reading from unsupported register: 0x%02x\n",
> +                  __func__, pmdev->code);
> +    return 0xFF;

Eventually PMBUS_ERROR_BYTE.

> +}
> +
> +static int isl_pmbus_vr_write_data(PMBusDevice *pmdev, const uint8_t *buf,
> +                              uint8_t len)

Mis-aligned.

> +{
> +    qemu_log_mask(LOG_GUEST_ERROR,
> +                  "%s: write to unsupported register: 0x%02x\n",
> +                  __func__, pmdev->code);
> +    return 0xFF;
> +}

> +static void raa22xx_init(Object *obj)
> +{
> +    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
> +    uint64_t flags[2];
> +
> +    flags[0] = PB_HAS_VIN | PB_HAS_VOUT | PB_HAS_VOUT_MODE |
> +               PB_HAS_VOUT_RATING | PB_HAS_VOUT_MARGIN | PB_HAS_IIN |
> +               PB_HAS_IOUT | PB_HAS_PIN | PB_HAS_POUT | PB_HAS_TEMPERATURE |
> +               PB_HAS_TEMP2 | PB_HAS_TEMP3 | PB_HAS_STATUS_MFR_SPECIFIC;
> +    flags[1] = PB_HAS_IIN | PB_HAS_PIN | PB_HAS_TEMPERATURE | PB_HAS_TEMP3 |
> +               PB_HAS_VOUT | PB_HAS_VOUT_MODE | PB_HAS_VOUT_MARGIN |
> +               PB_HAS_VOUT_RATING | PB_HAS_IOUT | PB_HAS_POUT |
> +               PB_HAS_STATUS_MFR_SPECIFIC;
> +
> +    pmbus_page_config(pmdev, 0, flags[0]);
> +    pmbus_page_config(pmdev, 1, flags[1]);
> +    isl_pmbus_vr_add_props(obj, flags, 2);

Eventually 2 -> ARRAY_SIZE(flags).

> +++ b/include/hw/sensor/isl_pmbus_vr.h
> @@ -0,0 +1,50 @@
> +/*
> + * PMBus device for Renesas Digital Multiphase Voltage Regulators
> + *
> + * Copyright 2022 Google LLC
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_MISC_ISL_PMBUS_VR_H
> +#define HW_MISC_ISL_PMBUS_VR_H
> +
> +#include "hw/i2c/pmbus_device.h"
> +#include "qom/object.h"
> +
> +#define TYPE_ISL69260   "isl69260"
> +
> +struct ISLState {
> +    PMBusDevice parent;
> +};
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ISLState, ISL69260)
> +
> +#define ISL_CAPABILITY_DEFAULT                 0x40
> +#define ISL_OPERATION_DEFAULT                  0x80
> +#define ISL_ON_OFF_CONFIG_DEFAULT              0x16
> +#define ISL_VOUT_MODE_DEFAULT                  0x40
> +#define ISL_VOUT_COMMAND_DEFAULT               0x0384
> +#define ISL_VOUT_MAX_DEFAULT                   0x08FC
> +#define ISL_VOUT_MARGIN_HIGH_DEFAULT           0x0640
> +#define ISL_VOUT_MARGIN_LOW_DEFAULT            0xFA
> +#define ISL_VOUT_TRANSITION_RATE_DEFAULT       0x64
> +#define ISL_VOUT_OV_FAULT_LIMIT_DEFAULT        0x076C
> +#define ISL_OT_FAULT_LIMIT_DEFAULT             0x7D
> +#define ISL_OT_WARN_LIMIT_DEFAULT              0x07D0
> +#define ISL_VIN_OV_WARN_LIMIT_DEFAULT          0x36B0
> +#define ISL_VIN_UV_WARN_LIMIT_DEFAULT          0x1F40
> +#define ISL_IIN_OC_FAULT_LIMIT_DEFAULT         0x32
> +#define ISL_TON_DELAY_DEFAULT                  0x14
> +#define ISL_TON_RISE_DEFAULT                   0x01F4
> +#define ISL_TOFF_FALL_DEFAULT                  0x01F4
> +#define ISL_REVISION_DEFAULT                   0x33
> +#define ISL_READ_VOUT_DEFAULT                  1000
> +#define ISL_READ_IOUT_DEFAULT                  40
> +#define ISL_READ_POUT_DEFAULT                  4
> +#define ISL_READ_TEMP_DEFAULT                  25
> +#define ISL_READ_VIN_DEFAULT                   1100
> +#define ISL_READ_IIN_DEFAULT                   40
> +#define ISL_READ_PIN_DEFAULT                   4

Hmmm we need to expose the DEFAULT definitions for the tests. OK.

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