[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: |
Wed, 13 Jan 2016 12:59:11 -0800 |
On Wed, Jan 13, 2016 at 8:07 AM, Tim Sander <address@hidden> wrote:
> 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.
>
Conditional operator is sometimes discouraged but still OK (I am a fan
of conditional operator FWIW and think it should be used over one
liner if-else setters always). The problem with conditional operator
in this case was the prototype for the functions is different, with
the data path being either and argument or return value. That patch of
mine makes it consistent for both read and write with a pointer arg
and the return value is always just an error code.
If you want to use conditional operator for the de-duplication that
should be ok until we fix I2C. But this is second controller on list
now with this same problem, the other being the I2C AUX controller for
the Xilinx DP work from Fred Konrad, so we should fix that core API.
Regards,
Peter
> 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;
>> > + }
>> > +}
>> > +
>