From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:33929)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1aGMIJ-0004Sm-8o
	for qemu-devel@nongnu.org; Tue, 05 Jan 2016 02:44:40 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1aGMIF-0001Dw-6h
	for qemu-devel@nongnu.org; Tue, 05 Jan 2016 02:44:39 -0500
Received: from mx1.redhat.com ([209.132.183.28]:60191)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1aGMIF-0001Cz-11
	for qemu-devel@nongnu.org; Tue, 05 Jan 2016 02:44:35 -0500
Message-ID: <1451979870.31764.19.camel@redhat.com>
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 05 Jan 2016 08:44:30 +0100
In-Reply-To: <1690320.QWN6MV0ZO0@dabox>
References: <1690320.QWN6MV0ZO0@dabox>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH] i2c-tiny-usb: add new usb to i2c bridge
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Tim Sander <tim@krieglstein.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>, Alex =?ISO-8859-1?Q?Benn=E9e?= <alex.bennee@linaro.org>, qemu-devel@nongnu.org, Markus Armbruster <armbru@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 =3D 0; /* write failure */
> +            break;
> +        }
> +        for (i =3D 0; i < length; i++) {
> +            trace_usb_i2c_tiny_write(request, index, i, data[i]);
> +            i2c_send(s->i2cbus, data[i]);
> +        }
> +        p->actual_length =3D 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 he=
re?=20
> */

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 =3D 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],=
=20
> data[3]);
> +        if (i2c_start_transfer(s->i2cbus, /*address*/ index, 1)) {
> +            trace_usb_i2c_tiny_i2c_start_transfer_failed();
> +            p->actual_length =3D 0;
> +            break;
> +        }

Doesn't look like this request is unknown ...

> +        for (i =3D 0; i < length; i++) {
> +            data[i] =3D i2c_recv(s->i2cbus);

Can this fail?

cheers,
  Gerd