[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: |
Gerd Hoffmann |
Subject: |
Re: [Qemu-devel] [PATCH] i2c-tiny-usb: add new usb to i2c bridge |
Date: |
Tue, 05 Jan 2016 08:44:30 +0100 |
> + 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).
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?
> + 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'?
> + 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?
cheers,
Gerd