Re: [PATCH 5/8] hw/sensor: switch adm1266 to millivolts vout

On Thu, 30 Jul 2026 at 00:13, Titus Rwantare <[email protected]> wrote:
>
> Enables storing fractional voltages for the ADM1266 over QMP
>
> Signed-off-by: Titus Rwantare <[email protected]>
> ---
>  hw/sensor/adm1266.c        |  27 ++++---
>  tests/qtest/adm1266-test.c | 150 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 167 insertions(+), 10 deletions(-)
>
> diff --git a/hw/sensor/adm1266.c b/hw/sensor/adm1266.c
> index 2979557309..80960dc1c4 100644
> --- a/hw/sensor/adm1266.c
> +++ b/hw/sensor/adm1266.c
> @@ -263,32 +263,39 @@ static int adm1266_write_data(PMBusDevice *pmdev, const uint8_t *buf,
>  static void adm1266_get(Object *obj, Visitor *v, const char *name, void *opaque,
>                          Error **errp)
>  {
> -    uint16_t value;
> +    uint32_t value, index;
>      PMBusDevice *pmdev = PMBUS_DEVICE(obj);
>      PMBusVoutMode *mode = (PMBusVoutMode *)&pmdev->pages[0].vout_mode;
>
> -    if (strcmp(name, "vout") == 0) {
> -        value = pmbus_linear_mode2data(*(uint16_t *)opaque, mode->exp);
> +    if (strncmp(name, "vout[", 5) == 0) {
> +        sscanf(name, "vout[%u]", &index);

What is this doing? Why do we need to do it now when we didn't
need to call sscanf before? If we do need to use sscanf() we
should check the return result for errors.

> +        mode = (PMBusVoutMode *)&pmdev->pages[index].vout_mode;
> +        value = pmbus_linear_mode2milliunits(*(uint16_t *)opaque, mode->exp);
>      } else {
>          value = *(uint16_t *)opaque;

You've changed this to a uint32_t property, so leaving this as
a uint16_t* cast looks wrong now.

>      }
>
> -    visit_type_uint16(v, name, &value, errp);
> +    visit_type_uint32(v, name, &value, errp);
>  }
>
>  static void adm1266_set(Object *obj, Visitor *v, const char *name, void *opaque,
>                          Error **errp)
>  {
>      uint16_t *internal = opaque;
> -    uint16_t value;
> +    uint32_t value, index;
>      PMBusDevice *pmdev = PMBUS_DEVICE(obj);
> -    PMBusVoutMode *mode = (PMBusVoutMode *)&pmdev->pages[0].vout_mode;
> +    PMBusVoutMode *mode;
>
> -    if (!visit_type_uint16(v, name, &value, errp)) {
> +    if (!visit_type_uint32(v, name, &value, errp)) {
>          return;
>      }
> -
> -    *internal = pmbus_data2linear_mode(value, mode->exp);
> +    if (strncmp(name, "vout[", 5) == 0) {
> +        sscanf(name, "vout[%u]", &index);
> +        mode = (PMBusVoutMode *)&pmdev->pages[index].vout_mode;
> +        *internal = pmbus_milliunits2linear_mode(value, mode->exp);
> +    } else {
> +        *internal = value;
> +    }
>      pmbus_check_limits(pmdev);

This looks like it's changing the semantics of this property,
but there's no documentation either of what the old set of
accepted strings were or what the new set are.

We shouldn't change the semantics of existing properties
in a non-backwards compatible way, generally speaking.
If we do really need to do that then we should flag that
up clearly in the commit message with the rationale and
the description of the change and its effects.

> +static void test_vout_milliunits(void *obj, void *data, QGuestAllocator *alloc)
> +{
> +    uint16_t i2c_value, value;
> +    uint64_t i2c_milliunits;
> +    QI2CDevice *i2cdev = (QI2CDevice *)obj;
> +    char *path;
> +    ADM1266VoutMode m;
> +
> +    /* set a different value in millivolts for each page */
> +    for (int i = 0; i < ADM1266_NUM_PAGES; i++) {
> +        path = g_strdup_printf("vout[%d]", i);
> +        qmp_adm1266_set(TEST_ID, path, (1000 * (i + 1)));
> +    }

These test functions leak the strings allocated by
g_strdup_printf() (the leaks show up if you run 'make check'
under the clang leak sanitizer). I recommend adjusting the
scope of the variables so you can do
  g_autofree char *path = g_strdup_printf(....);
and have it automatically freed as it goes out of scope.

thanks
-- PMM