From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42505) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cIzuy-0002m0-2G for qemu-devel@nongnu.org; Mon, 19 Dec 2016 10:32:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cIzuw-0001Vz-QH for qemu-devel@nongnu.org; Mon, 19 Dec 2016 10:32:00 -0500 Received: from mail-ua0-x22d.google.com ([2607:f8b0:400c:c08::22d]:36168) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cIzuw-0001Uo-CV for qemu-devel@nongnu.org; Mon, 19 Dec 2016 10:31:58 -0500 Received: by mail-ua0-x22d.google.com with SMTP id 3so89529745uaz.3 for ; Mon, 19 Dec 2016 07:31:56 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <20161123194605.94717-1-flus@cesar.org.br> <20161129193052.11736-1-flus@cesar.org.br> <1482112026.17769.25.camel@au1.ibm.com> From: Peter Maydell Date: Mon, 19 Dec 2016 15:31:35 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v1 0/2] Add Atmel I2C TPM AT97SC3204T emulated device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Minyard Cc: Alastair D'Silva , Fabio Urquiza , QEMU Developers On 19 December 2016 at 13:55, Corey Minyard 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