From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54214) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGQeT-0007d8-8U for qemu-devel@nongnu.org; Tue, 05 Jan 2016 07:23:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGQeO-00031Y-8n for qemu-devel@nongnu.org; Tue, 05 Jan 2016 07:23:49 -0500 Received: from lvps176-28-13-145.dedicated.hosteurope.de ([176.28.13.145]:60772) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGQeO-00031T-2G for qemu-devel@nongnu.org; Tue, 05 Jan 2016 07:23:44 -0500 From: Tim Sander Date: Tue, 05 Jan 2016 13:23:12 +0100 Message-ID: <3104807.PJOGURGZId@dabox> In-Reply-To: <1451979870.31764.19.camel@redhat.com> References: <1690320.QWN6MV0ZO0@dabox> <1451979870.31764.19.camel@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [Qemu-devel] [PATCH] i2c-tiny-usb: add new usb to i2c bridge List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: Paolo Bonzini , Alex =?ISO-8859-1?Q?Benn=E9e?= , qemu-devel@nongnu.org, Markus Armbruster Hi Gerd Thanks for your review. Am Dienstag, 5. Januar 2016, 08:44:30 schrieb Gerd Hoffmann: > > + case 0x4107: > > + /* this seems to be a byte type access */ > > + if (i2c_start_transfer(s->i2cbus, /*address*/index, 0)) { > > + trace_usb_i2c_tiny_i2c_start_transfer_failed(); > > + p->actual_length = 0; /* write failure */ > > + break; > > + } > > + for (i = 0; i < length; i++) { > > + trace_usb_i2c_tiny_write(request, index, i, data[i]); > > + i2c_send(s->i2cbus, data[i]); > > + } > > + p->actual_length = length; > > + i2c_end_transfer(s->i2cbus); > > + break; > > I think most of the tracepoints should be moved into i2c code (or just > dropped in case we already have tracepoints there). I don't think that there are any tracepoints in there. > One (high-level) tracepoint per transfer request makes sense in the usb > code, i.e. trace_usb_i2c_transfer_{read,write}, so one can see in the > trace log which usb request triggered which i2c transaction. > > > + case 0xc101: > > + { > > + /* thats what the real thing reports, FIXME: can we do better > > here? */ > > Hmm, didn't we agree on adding a note about what the "real thing" we > mimic here is, to the comment at the start of the file? Ok, that was a missunderstanding. I thought you wanted a description on top of the patch i was sending but having a description in the file makes sense and i will add it. > > + uint32_t func = htole32(I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL); > > Can we move 'func' to the start of the function too, like we did with > 'i'? will do. > > + case 0xc106: > > + trace_usb_i2c_tiny_unknown_request(index, request, value, > > length); > > + trace_usb_i2c_tiny_unknown_request(data[0], data[1], data[2], > > data[3]); > > + if (i2c_start_transfer(s->i2cbus, /*address*/ index, 1)) { > > + trace_usb_i2c_tiny_i2c_start_transfer_failed(); > > + p->actual_length = 0; > > + break; > > + } > > Doesn't look like this request is unknown ... > > > + for (i = 0; i < length; i++) { > > + data[i] = i2c_recv(s->i2cbus); > > Can this fail? I think failure is just returning 255 as a value? AFAIK thats what real i2c hardware returns. Best regards Tim