Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Titus Rwantare <titusr@google.com>]

On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
<peterdelevoryas@gmail.com> wrote:
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---

> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>          }
>          break;
>
> +    case PMBUS_IC_DEVICE_ID:
> +        pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
> +                   sizeof(pmdev->pages[index].ic_device_id));
> +        break;
> +

I don't think it's a good idea to add this here because this sends 16
bytes for all PMBus devices. I have at least one device that formats
IC_DEVICE_ID differently that I've not got permission to upstream.
The spec leaves the size and format up to the manufacturer, so this is
best done in isl_pmbus_vr.c in isl_pmbus_vr_read_byte().
Look at the adm1272_read_byte() which is more interesting than
isl_pmbus_vr one as an example.


>      case PMBUS_CLEAR_FAULTS:              /* Send Byte */
>      case PMBUS_PAGE_PLUS_WRITE:           /* Block Write-only */
>      case PMBUS_STORE_DEFAULT_ALL:         /* Send Byte */
> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> index e11e028884..b12c46ab6d 100644
> --- a/hw/sensor/isl_pmbus_vr.c
> +++ b/hw/sensor/isl_pmbus_vr.c
> @@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass *klass, void *data,
>      k->device_num_pages = pages;
>  }
>
> +static void isl69259_init(Object *obj)
> +{
> +    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49};
> +    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
> +    int i;
> +
> +    raa22xx_init(obj);
> +    for (i = 0; i < pmdev->num_pages; i++) {
> +        memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
> +               sizeof(ic_device_id));
> +    }
> +}
> +

We tend to set default register values in exit_reset() calls. You can
do something like in raa228000_exit_reset()


> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index 0f4d6b3fad..aed7809841 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -407,6 +407,7 @@ typedef struct PMBusPage {
>      uint16_t mfr_max_temp_1;           /* R/W word */
>      uint16_t mfr_max_temp_2;           /* R/W word */
>      uint16_t mfr_max_temp_3;           /* R/W word */
> +    uint8_t ic_device_id[16];          /* Read-Only block-read */

You wouldn't be able to do this here either, since the size could be
anything for other devices.

Thanks for the new device. It helps me see where to expand on PMBus.


Titus