Re: [Qemu-devel] [PATCH] i2c-tiny-usb: add new usb to i2c bridge

[Tim Sander <tim@krieglstein.org>]

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