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

[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?
Or does it mimic existing hardware?
Please add that into to the comment at the head of the file.

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

> +static const USBDescIface desc_iface0 = {
> +    .bInterfaceNumber              = 1,
> +    .bNumEndpoints                 = 0,
> +    .bInterfaceClass               = 0xff,
> +    .bInterfaceSubClass            = 0xff,
> +    .bInterfaceProtocol            = 0xff,
> +    .eps = (USBDescEndpoint[]) {
> +    }
> +};

Hmm?  No endpoints?

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

cheers,
  Gerd