Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device

[Peter Maydell]

On 19 December 2016 at 13:55, Corey Minyard <address@hidden> wrote:
> On 12/18/2016 07:47 PM, Alastair D'Silva wrote:
>>
>> On Fri, 2016-12-16 at 17:35 +0000, Peter Maydell wrote:
>>> Our current API seems to envisage that the slave can return a
>>> negative value from I2CSlaveClass::recv instead of a data byte,
>>> but I'm not sure what this means in the i2c protocol.
>>
>> Negative values are propagated upwards, where they are treated as
>> errors, eg, in hw/i2c/aspeed_i2c.c:aspeed_i2c_bus_handle_cmd():
>>
>> int ret = i2c_recv(bus->bus);
>> if (ret < 0) {
>>      qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>      ret = 0xff;
>> }
>>
>> The call to i2c_recv is too late to issue the NAK, I believe they occur
>> during the start_transfer() call.

OK, so if returning negative values from i2c_recv() isn't
the device saying "I am NAKing this", what *do* they mean?

>>> If I understand your patch correctly, this is adding support
>>> for the slave refusing to ACK when the master sends out the
>>> slave address and r/w bit. I think that makes sense, but rather
>>> than having a state flag in the I2CSlave struct, we should
>>> change the prototype of the I2CSlaveClass event method so that
>>> it can return a value indicating ack or nak.
>>>
>> Hmm, this could end up being quite an invasive change, but ultimately
>> more elegant. I'm not sure which way the community prefers.
>
>
> I have a patch that adds a check_event() handler along side the event()
> handler.
> If a device wants to send a NAK, it can implement check_event() instead of
> event() and return non-zero to NAK.
>
> I toyed with just changing all the event() calls, but there are a bunch.
> This seemed
> like the better approach.  I can send if you like.

It looks like there are only a dozen or so. I think it would
be better for the long term just to change the event calls.
We should also document in the comments in the I2CSlaveClass
struct definition exactly what the semantics of the various
functions are.

thanks
-- PMM