> + 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