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

[Peter Crosthwaite]

On Tue, Jan 5, 2016 at 4:23 AM, Tim Sander <address@hidden> wrote:
> 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?

Yes it can. No device would return -1. Positive values indicate
non-errors I think.

Regards,
Peter

> I think failure is just returning 255 as a value? AFAIK thats what real i2c
> hardware returns.
>
> Best regards
> Tim
>