qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH] i2c-tiny-usb: add new usb to i2c bridge
Date: Thu, 7 Jan 2016 02:14:23 -0800

Patch subject prefix should contain the version number. Use the
--subject-prefix or -v options to git format-patch.

On Wed, Jan 6, 2016 at 6:58 AM, Tim Sander <address@hidden> wrote:
> Version 4 with improvements suggested by Gerd Hoffmann:
>

Changelog information should go below the line ...

> Signed-off-by: Tim Sander <address@hidden>
>

Signed-off-by usually at end of the commit message.

> i2c-tiny-usb is a small usb to i2c bridge:
>  http://www.harbaum.org/till/i2c_tiny_usb/index.shtml
>
> It is pretty simple and has no usb endpoints just a control.
> Reasons for adding this device:
> * Linux device driver available
> * adding an additional i2c bus via command line e.g.
>   -device usb-i2c-tiny,id=i2c-0 -device tmp105,bus=i2c,address=0x50
> ---

... here.

>  default-configs/usb.mak |   1 +
>  hw/usb/Makefile.objs    |   1 +
>  hw/usb/dev-i2c-tiny.c   | 320 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  trace-events            |  11 ++
>  4 files changed, 333 insertions(+)
>  create mode 100644 hw/usb/dev-i2c-tiny.c
>
> diff --git a/default-configs/usb.mak b/default-configs/usb.mak
> index f4b8568..01d2c9f 100644
> --- a/default-configs/usb.mak
> +++ b/default-configs/usb.mak
> @@ -8,3 +8,4 @@ CONFIG_USB_AUDIO=y
>  CONFIG_USB_SERIAL=y
>  CONFIG_USB_NETWORK=y
>  CONFIG_USB_BLUETOOTH=y
> +CONFIG_USB_I2C_TINY=y
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index 8f00fbd..3a4c337 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -20,6 +20,7 @@ common-obj-$(CONFIG_USB_AUDIO)        += dev-audio.o
>  common-obj-$(CONFIG_USB_SERIAL)       += dev-serial.o
>  common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
>  common-obj-$(CONFIG_USB_BLUETOOTH)    += dev-bluetooth.o
> +common-obj-$(CONFIG_USB_I2C_TINY)     += dev-i2c-tiny.o
>
>  ifeq ($(CONFIG_USB_SMARTCARD),y)
>  common-obj-y                          += dev-smartcard-reader.o
> diff --git a/hw/usb/dev-i2c-tiny.c b/hw/usb/dev-i2c-tiny.c
> new file mode 100644
> index 0000000..c28d7a5
> --- /dev/null
> +++ b/hw/usb/dev-i2c-tiny.c
> @@ -0,0 +1,320 @@
> +/*
> + * I2C tiny usb device emulation
> + *
> + * i2c-tiny-usb is a small usb to i2c bridge:
> + *
> + * http://www.harbaum.org/till/i2c_tiny_usb/index.shtml
> + *
> + * The simulated device is pretty simple and has no usb endpoints.
> + * There is a Linux device driver available named i2c-tiny-usb.
> + *
> + * Below is an example how to use this device from command line:
> + *  -device usb-i2c-tiny,id=i2c-0 -device tmp105,bus=i2c,address=0x50
> + *
> + * Copyright (c) 2015 Tim Sander <address@hidden>
> + *
> + * Loosly based on usb dev-serial.c:
> + * Copyright (c) 2006 CodeSourcery.
> + * Copyright (c) 2008 Samuel Thibault <address@hidden>
> + * Written by Paul Brook, reused for FTDI by Samuel Thibault
> + *
> + * This code is licensed under the LGPL.
> + *
> + */
> +
> +#include "trace.h"
> +#include "qemu-common.h"
> +#include "qemu/error-report.h"
> +#include "hw/usb.h"
> +#include "hw/usb/desc.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/smbus.h"
> +#include "sysemu/char.h"
> +#include "endian.h"
> +
> +/* commands from USB, must e.g. match command ids in kernel driver */
> +#define CMD_ECHO       0
> +#define CMD_GET_FUNC   1
> +#define CMD_SET_DELAY  2
> +#define CMD_GET_STATUS 3
> +
> +/* To determine what functionality is present */
> +#define I2C_FUNC_I2C                            0x00000001
> +#define I2C_FUNC_10BIT_ADDR                     0x00000002
> +#define I2C_FUNC_PROTOCOL_MANGLING              0x00000004
> +#define I2C_FUNC_SMBUS_HWPEC_CALC               0x00000008 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_READ_WORD_DATA_PEC       0x00000800 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_WRITE_WORD_DATA_PEC      0x00001000 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_PROC_CALL_PEC            0x00002000 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_BLOCK_PROC_CALL_PEC      0x00004000 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_BLOCK_PROC_CALL          0x00008000 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_QUICK                    0x00010000
> +#define I2C_FUNC_SMBUS_READ_BYTE                0x00020000
> +#define I2C_FUNC_SMBUS_WRITE_BYTE               0x00040000
> +#define I2C_FUNC_SMBUS_READ_BYTE_DATA           0x00080000
> +#define I2C_FUNC_SMBUS_WRITE_BYTE_DATA          0x00100000
> +#define I2C_FUNC_SMBUS_READ_WORD_DATA           0x00200000
> +#define I2C_FUNC_SMBUS_WRITE_WORD_DATA          0x00400000
> +#define I2C_FUNC_SMBUS_PROC_CALL                0x00800000
> +#define I2C_FUNC_SMBUS_READ_BLOCK_DATA          0x01000000
> +#define I2C_FUNC_SMBUS_WRITE_BLOCK_DATA         0x02000000
> +#define I2C_FUNC_SMBUS_READ_I2C_BLOCK           0x04000000 /*I2C-like 
> blk-xfr */
> +#define I2C_FUNC_SMBUS_WRITE_I2C_BLOCK          0x08000000 /*1-byte reg. 
> addr.*/
> +#define I2C_FUNC_SMBUS_READ_I2C_BLOCK_2         0x10000000 /*I2C-like 
> blk-xfer*/
> +#define I2C_FUNC_SMBUS_WRITE_I2C_BLOCK_2        0x20000000 /* w/ 2-byte 
> regadr*/
> +#define I2C_FUNC_SMBUS_READ_BLOCK_DATA_PEC      0x40000000 /* SMBus 2.0 */
> +#define I2C_FUNC_SMBUS_WRITE_BLOCK_DATA_PEC     0x80000000 /* SMBus 2.0 */
> +
> +#define I2C_FUNC_SMBUS_BYTE (I2C_FUNC_SMBUS_READ_BYTE | \
> +    I2C_FUNC_SMBUS_WRITE_BYTE)
> +#define I2C_FUNC_SMBUS_BYTE_DATA (I2C_FUNC_SMBUS_READ_BYTE_DATA | \
> +    I2C_FUNC_SMBUS_WRITE_BYTE_DATA)
> +#define I2C_FUNC_SMBUS_WORD_DATA (I2C_FUNC_SMBUS_READ_WORD_DATA | \
> +    I2C_FUNC_SMBUS_WRITE_WORD_DATA)
> +#define I2C_FUNC_SMBUS_BLOCK_DATA (I2C_FUNC_SMBUS_READ_BLOCK_DATA | \
> +    I2C_FUNC_SMBUS_WRITE_BLOCK_DATA)
> +#define I2C_FUNC_SMBUS_I2C_BLOCK (I2C_FUNC_SMBUS_READ_I2C_BLOCK | \
> +    I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)
> +
> +#define I2C_FUNC_SMBUS_EMUL (I2C_FUNC_SMBUS_QUICK | \
> +    I2C_FUNC_SMBUS_BYTE | \
> +    I2C_FUNC_SMBUS_BYTE_DATA | \
> +    I2C_FUNC_SMBUS_WORD_DATA | \
> +    I2C_FUNC_SMBUS_PROC_CALL | \
> +    I2C_FUNC_SMBUS_WRITE_BLOCK_DATA | \
> +    I2C_FUNC_SMBUS_WRITE_BLOCK_DATA_PEC | \
> +    I2C_FUNC_SMBUS_I2C_BLOCK)
> +
> +#define DeviceOutVendor ((USB_DIR_OUT|USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8)
> +#define DeviceInVendor  ((USB_DIR_IN|USB_TYPE_VENDOR|USB_RECIP_DEVICE)<<8)
> +
> +typedef struct {
> +    USBDevice dev;
> +    I2CBus *i2cbus;
> +} UsbI2cTinyState;

Acronyms in CamelCase should be all-caps, making this USBI2CTinyState.

> +
> +#define TYPE_USB_I2C_TINY "usb-i2c-dev"

do we need the -dev suffix?

> +#define USB_I2C_TINY_DEV(obj) OBJECT_CHECK(UsbI2cTinyState, \
> +    (obj), TYPE_USB_I2C_TINY)
> +
> +enum {
> +    STR_MANUFACTURER = 1,
> +    STR_PRODUCT_SERIAL,
> +    STR_SERIALNUMBER,
> +};
> +
> +static const USBDescStrings desc_strings = {
> +    [STR_MANUFACTURER]    = "QEMU",
> +    [STR_PRODUCT_SERIAL]  = "QEMU USB I2C Tiny",
> +    [STR_SERIALNUMBER]    = "1",
> +};
> +
> +static const USBDescIface desc_iface0 = {
> +    .bInterfaceNumber              = 1,
> +    .bNumEndpoints                 = 0,
> +    .bInterfaceClass               = 0xff,
> +    .bInterfaceSubClass            = 0xff,
> +    .bInterfaceProtocol            = 0xff,
> +    .eps = (USBDescEndpoint[]) {
> +    }
> +};
> +
> +static const USBDescDevice desc_device = {
> +    .bcdUSB                        = 0x0110,
> +    .bMaxPacketSize0               = 8,
> +    .bNumConfigurations            = 1,
> +    .confs = (const USBDescConfig[]) {
> +        {
> +            .bNumInterfaces        = 1,
> +            .bConfigurationValue   = 1,
> +            .bmAttributes          = USB_CFG_ATT_ONE,
> +            .bMaxPower             = 50,
> +            .nif = 1,
> +            .ifs = &desc_iface0,
> +        },
> +    },
> +};
> +
> +static const USBDesc desc_usb_i2c = {
> +    .id = {
> +        .idVendor          = 0x0403,
> +        .idProduct         = 0xc631,
> +        .bcdDevice         = 0x0205,
> +        .iManufacturer     = STR_MANUFACTURER,
> +        .iProduct          = STR_PRODUCT_SERIAL,
> +        .iSerialNumber     = STR_SERIALNUMBER,
> +    },
> +    .full = &desc_device,
> +    .str  = desc_strings,
> +};
> +
> +static void usb_i2c_handle_reset(USBDevice *dev)
> +{
> +    trace_usb_i2c_tiny_reset();
> +}
> +
> +static void usb_i2c_handle_control(USBDevice *dev, USBPacket *p,
> +               int request, int value, int index, int length, uint8_t *data)
> +{
> +    UsbI2cTinyState *s = (UsbI2cTinyState *)dev;
> +    int ret;
> +    int i;
> +    uint32_t func;
> +
> +    ret = usb_desc_handle_control(dev, p, request, value, index, length, 
> data);
> +    if (ret >= 0) {
> +        return;
> +    }
> +
> +    switch (request) {
> +    case EndpointOutRequest | USB_REQ_CLEAR_FEATURE:
> +        break;
> +    case 0x4102:
> +        /* set clock, ignore */
> +        trace_usb_i2c_tiny_set_clock();
> +        break;
> +
> +    case 0x4105:
> +        trace_usb_i2c_tiny_unknown_request(index, request, value, length);
> +        break;
> +
> +    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;
> +
> +    case 0xc101:
> +        /* thats what the real thing reports, FIXME: can we do better here? 
> */
> +        func = htole32(I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL);
> +        memcpy(data, &func, sizeof(func));
> +        p->actual_length = sizeof(func);
> +        trace_usb_i2c_tiny_functionality_read(length);
> +        break;
> +
> +    case 0xc103:
> +        trace_usb_i2c_tiny_unknown_request(index, request, value, length);
> +        p->actual_length = 1;
> +        break;
> +
> +    case 0xc106:
> +        if (i2c_start_transfer(s->i2cbus, /*address*/ index, 1)) {
> +            trace_usb_i2c_tiny_i2c_start_transfer_failed();
> +            p->actual_length = 0;
> +            break;
> +        }
> +        i2c_send(s->i2cbus, data[0]);
> +        i2c_start_transfer(s->i2cbus, /*address*/ index, 1);
> +        for (i = 0; i < length; i++) {
> +            data[i] = i2c_recv(s->i2cbus);
> +            trace_usb_i2c_tiny_read(request, index, i, data[i]);
> +        }
> +        i2c_nack(s->i2cbus);

This looks odd in that c106 is the only instruction format terminating
with nack.

> +        p->actual_length = length;
> +        i2c_end_transfer(s->i2cbus);
> +        break;
> +
> +    case 0xc107:

Your three transacting cases all have very similar functionality. 4107
and c107 only differ in data directionality and only one bit differs
in opcode encoding. Is request an expanding instruction format? The
code should at least be de-duplicated.

You may find this patch helps merge at least 4107 and c107:

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04237.html

Regards,
Peter

> +        if (i2c_start_transfer(s->i2cbus, /*address*/index, 1)) {
> +            trace_usb_i2c_tiny_i2c_start_transfer_failed();
> +            p->actual_length = 0; /* read failure */
> +            break;
> +        }
> +        for (i = 0; i < length; i++) {
> +            data[i] = i2c_recv(s->i2cbus);
> +            trace_usb_i2c_tiny_read(request, index, i, data[i]);
> +        }
> +        p->actual_length = length;
> +        i2c_end_transfer(s->i2cbus);
> +        break;
> +
> +    default:
> +        p->status = USB_RET_STALL;
> +        trace_usb_i2c_tiny_unknown_request(index, request, value, length);
> +        break;
> +    }
> +}
> +



reply via email to

[Prev in Thread] Current Thread [Next in Thread]