[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] i2c-tiny-usb: add new usb to i2c bridge
From: |
Tim Sander |
Subject: |
Re: [Qemu-devel] [PATCH] i2c-tiny-usb: add new usb to i2c bridge |
Date: |
Tue, 05 Jan 2016 13:23:12 +0100 |
User-agent: |
KMail/4.14.3 (Linux/4.4.0-rc8-00001-g1ec18c8; KDE/4.14.13; x86_64; ; ) |
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