Re: [PATCH v2 08/13] hw/i2c/pmbus: Reset out buf after switching pages

[Peter Delevoryas]

> On Jun 29, 2022, at 11:05 AM, Titus Rwantare <titusr@google.com> wrote:
> 
> On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> <peterdelevoryas@gmail.com> wrote:
>> 
>> When a pmbus device switches pages, it should clears its output buffer so
>> that the next transaction doesn't emit data from the previous page.
>> 
>> Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”)
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> hw/i2c/pmbus_device.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
>> index 62885fa6a1..efddc36fd9 100644
>> --- a/hw/i2c/pmbus_device.c
>> +++ b/hw/i2c/pmbus_device.c
>> @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t 
>> *buf, uint8_t len)
>> 
>>     if (pmdev->code == PMBUS_PAGE) {
>>         pmdev->page = pmbus_receive8(pmdev);
>> +        pmdev->out_buf_len = 0;
>>         return 0;
>>     }
>> 
> 
> I suspect you were running into this because ic_device_id was putting
> too much data in the output buffer. Still, I wouldn't want the buffer
> cleared if the page hasn't changed. Some drivers write the same page
> before every read.

Yes you’re correct: this is the code that was querying the ic_device_id [1]:

    memset(&msg, 0, sizeof(msg));
    msg.bus = sensor_config[index].port;
    msg.target_addr = sensor_config[index].target_addr;
    msg.tx_len = 1;
    msg.rx_len = 7;
    msg.data[0] = PMBUS_IC_DEVICE_ID;
    if (i2c_master_read(&msg, retry)) {
        printf("Failed to read VR IC_DEVICE_ID: register(0x%x)\n", 
PMBUS_IC_DEVICE_ID);
        return;
    }

By sending a buffer that was way larger than the rx buffer of 7, it was
leaving stuff lying around for the next query.

I’ll test it out and see what happens if I fix the IC_DEVICE_ID length
transmitted by the ISL69259 to 4, maybe we don’t need this patch. But,
at the very least, I’ll make sure to gate this on the page value changing,
not just being set.

Thanks for the review, this was really useful. I’m not very familiar
with pmbus devices.

[1] 
https://github.com/facebook/OpenBIC/blob/cda4c00b032b1d9c9b94c45faa2c0eca4886a0a3/meta-facebook/yv35-cl/src/platform/plat_sensor_table.c#L332-L355

> 
> Titus