From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:55823)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <tim@krieglstein.org>) id 1a2GlK-000118-4Y
	for qemu-devel@nongnu.org; Fri, 27 Nov 2015 06:00:23 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <tim@krieglstein.org>) id 1a2GlF-00049z-Ts
	for qemu-devel@nongnu.org; Fri, 27 Nov 2015 06:00:22 -0500
Received: from lvps176-28-13-145.dedicated.hosteurope.de
	([176.28.13.145]:42211) by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <tim@krieglstein.org>) id 1a2GlF-00049r-Nx
	for qemu-devel@nongnu.org; Fri, 27 Nov 2015 06:00:17 -0500
From: Tim Sander <tim@krieglstein.org>
Date: Fri, 27 Nov 2015 11:59:46 +0100
Message-ID: <1748245.5TmL5QNuAr@dabox>
In-Reply-To: <1448606901.8613.135.camel@redhat.com>
References: <1664220.kcr3K9QWbf@dabox> <1448606901.8613.135.camel@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Subject: Re: [Qemu-devel] [PATCH RFC] i2c-tiny-usb
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: Gerd Hoffmann <kraxel@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org

Am Freitag, 27. November 2015, 07:48:21 schrieb Gerd Hoffmann:
> On Do, 2015-11-26 at 17:35 +0100, Tim Sander wrote:
> > Hi
> > 
> > Below is a patch implementing the i2c-tiny-usb device.
> 
> Is there a specification for this kind of device?
http://www.harbaum.org/till/i2c_tiny_usb/index.shtml
> Or does it mimic existing hardware?
Yes, the reason i choose this device where:
*simple
*linux driver available
which makes a cmdline configurable i2c bus easy.
> Please add that into to the comment at the head of the file.
Will do.
 
> > +#ifdef DEBUG_USBI2C
> > +#define DPRINTF(fmt, ...) \
> > +do { printf("usb-i2c-tiny: " fmt , ## __VA_ARGS__); } while (0)
> > +#else
> > +#define DPRINTF(fmt, ...) do {} while (0)
> > +#endif
> 
> Please consider turning them into trace points.
Ok.

> > +static const USBDescIface desc_iface0 = {
> > +    .bInterfaceNumber              = 1,
> > +    .bNumEndpoints                 = 0,
> > +    .bInterfaceClass               = 0xff,
> > +    .bInterfaceSubClass            = 0xff,
> > +    .bInterfaceProtocol            = 0xff,
> > +    .eps = (USBDescEndpoint[]) {
> > +    }
> > +};
> 
> Hmm?  No endpoints?
Yes this device has indeed no endpoints just control as you found out below.

> > +static void usb_i2c_handle_control(USBDevice *dev, USBPacket *p,
> > +               int request, int value, int index, int length, uint8_t
> > *data) +{
> > 
> > +    case 0xc101:
> > +        {
> > +        /* thats what the real thing reports, FIXME: can we do better
> > here? */ +        uint32_t func = htole32(I2C_FUNC_I2C |
> > I2C_FUNC_SMBUS_EMUL); +        DPRINTF("got functionality read %x, value
> > %x\n", request, value); +        memcpy(data, &func, sizeof(func));
> > +        p->actual_length = sizeof(func);
> > +        }
> > +        break;
> 
> Ah, it all goes over the control pipe.
> 
> > +static USBDevice *usb_i2c_init(USBBus *bus, const char *filename)
> 
> Please drop this ...
> 
> > +    usb_legacy_register("usb-i2c-tiny", "i2c-bus-tiny", usb_i2c_init);
> 
> ... and this.
> 
> It's for backward compatibility with old qemu versions (-usbdevice ...),
> and we don't need that for new devices.
Nice this makes the code even smaller :-).

Best regards
Tim