Re: [Qemu-devel] [PATCH RFC] i2c-tiny-usb

[Tim Sander <tim@krieglstein.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