[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: |
Tim Sander |
|
Subject: |
Re: [Qemu-devel] [PATCH] i2c-tiny-usb: add new usb to i2c bridge |
|
Date: |
Wed, 13 Jan 2016 17:07:06 +0100 |
|
User-agent: |
KMail/4.14.3 (Linux/4.4.0-rc8-00001-g1ec18c8; KDE/4.14.13; x86_64; ; ) |
Hi
Am Donnerstag, 7. Januar 2016, 02:14:23 schrieb Peter Crosthwaite:
> Patch subject prefix should contain the version number. Use the
> --subject-prefix or -v options to git format-patch.
Ok, i will try to remember this next time.
>
> 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.
Ok.
> > 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.
Will do.
> > +
> > +#define TYPE_USB_I2C_TINY "usb-i2c-dev"
>
> do we need the -dev suffix?
This was just resembling the file name in reverse but i will remove it.
> > +#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.
This kind of resembles smbus_receive_byte for byte adressing.
As i want to return multiple bytes i could not use smbus_receive_byte directly
AFAIK.
>
> > + 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
Is it ok to use the conditional operator for this? Haven't found anything
about it in the coding style.
Best Regards
Tim
> > + 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;
> > + }
> > +}
> > +