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

[Gerd Hoffmann <kraxel@redhat.com>]

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