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