Ernest Esene <erok...@gmail.com> writes: > Add support for Linux I2C character device for I2C device passthrough > For example: > -chardev linux-i2c,address=0x46,path=/dev/i2c-N,id=i2c-chardev > > Signed-off-by: Ernest Esene <erok...@gmail.com>
Could you explain briefly how passing through a host's I2C device can be useful? > > --- > v2: > * Fix errors > * update "MAINTAINERS" file. > --- > MAINTAINERS | 6 ++ > chardev/Makefile.objs | 1 + > chardev/char-i2c.c | 140 > +++++++++++++++++++++++++++++++++++++++++++++ > chardev/char.c | 3 + > include/chardev/char-i2c.h | 35 ++++++++++++ > include/chardev/char.h | 1 + > qapi/char.json | 18 ++++++ > 7 files changed, 204 insertions(+) > create mode 100644 chardev/char-i2c.c > create mode 100644 include/chardev/char-i2c.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 7dd71e0a2d..b79d9b8edf 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1801,6 +1801,12 @@ M: Samuel Thibault <samuel.thiba...@ens-lyon.org> > S: Maintained > F: chardev/baum.c > > +Character Devices (Linux I2C) > +M: Ernest Esene <erok...@gmail.com> > +S: Maintained > +F: chardev/char-i2c.c > +F: include/chardev/char-i2c.h > + Thanks for backing your contribution with an offer to maintain it. Accepting the offer is up to the Character device backends maintainer Marc-André, I think. > Command line option argument parsing > M: Markus Armbruster <arm...@redhat.com> > S: Supported > diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs > index d68e1347f9..6c96b9a353 100644 > --- a/chardev/Makefile.objs > +++ b/chardev/Makefile.objs > @@ -16,6 +16,7 @@ chardev-obj-y += char-stdio.o > chardev-obj-y += char-udp.o > chardev-obj-$(CONFIG_WIN32) += char-win.o > chardev-obj-$(CONFIG_WIN32) += char-win-stdio.o > +chardev-obj-$(CONFIG_POSIX) +=char-i2c.o > > common-obj-y += msmouse.o wctablet.o testdev.o > common-obj-$(CONFIG_BRLAPI) += baum.o > diff --git a/chardev/char-i2c.c b/chardev/char-i2c.c > new file mode 100644 > index 0000000000..4b068b0124 > --- /dev/null > +++ b/chardev/char-i2c.c > @@ -0,0 +1,140 @@ > +/* > + * QEMU System Emulator > + * > + * Copyright (c) 2019 Ernest Esene <erok...@gmail.com> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > copy > + * of this software and associated documentation files (the "Software"), to > deal > + * in the Software without restriction, including without limitation the > rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ Any particular reason not to use GPLv2+? My knowledge of I2C rounds to zero, so I can only review for basic sanity. > +#include "qemu/osdep.h" > +#include "qapi/error.h" > +#include "qemu/option.h" > +#include "qemu-common.h" > +#include "io/channel-file.h" > +#include "qemu/cutils.h" > + > +#include "chardev/char-fd.h" > +#include "chardev/char-i2c.h" > +#include "chardev/char.h" > + > +#include <sys/ioctl.h> > +#include <linux/i2c-dev.h> > + > +static int i2c_ioctl(Chardev *chr, int cmd, void *arg) > +{ > + FDChardev *fd_chr = FD_CHARDEV(chr); > + QIOChannelFile *floc = QIO_CHANNEL_FILE(fd_chr->ioc_in); > + int fd = floc->fd; > + int addr; > + > + switch (cmd) { > + case CHR_IOCTL_I2C_SET_ADDR: > + addr = (int) (long) arg; Would (int)arg make the compiler unhappy? > + > + if (addr > CHR_I2C_ADDR_7BIT_MAX) { > + /* > + * TODO: check if adapter support 10-bit addr > + * I2C_FUNC_10BIT_ADDR > + */ What's the impact of not having done this TODO? Should it be mentioned in the commit message? > + if (ioctl(fd, I2C_TENBIT, addr) < 0) { > + goto err; > + } > + } else { > + if (ioctl(fd, I2C_SLAVE, addr) < 0) { > + goto err; > + } > + } > + break; > + > + default: > + return -ENOTSUP; > + } > + return 0; > +err: > + return -ENOTSUP; > +} > + > +static void qmp_chardev_open_i2c(Chardev *chr, ChardevBackend *backend, > + bool *be_opened, Error **errp) > +{ > + ChardevI2c *i2c = backend->u.i2c.data; > + void *addr; > + int fd; > + > + fd = qmp_chardev_open_file_source(i2c->device, O_RDWR | O_NONBLOCK, > + errp); > + if (fd < 0) { > + return; > + } > + qemu_set_block(fd); Sure we want *blocking* I/O? No other character device does. > + qemu_chr_open_fd(chr, fd, fd); > + addr = (void *) (long) i2c->address; Would (void *)i2c->address make the compiler unhappy? > + i2c_ioctl(chr, CHR_IOCTL_I2C_SET_ADDR, addr); > +} > + > +static void qemu_chr_parse_i2c(QemuOpts *opts, ChardevBackend *backend, > + Error **errp) > +{ > + const char *device = qemu_opt_get(opts, "path"); > + const char *addr = qemu_opt_get(opts, "address"); > + long address; > + ChardevI2c *i2c; Blank line between declarations and statements, please. > + if (device == NULL) { > + error_setg(errp, "chardev: linux-i2c: no device path given"); > + return; > + } > + if (addr == NULL) { > + error_setg(errp, "chardev: linux-i2c: no device address given"); > + return; > + } > + if (qemu_strtol(addr, NULL, 0, &address) != 0) { > + error_setg(errp, "chardev: linux-i2c: invalid device address given"); > + return; > + } Why not make option "addr" QEMU_OPT_NUMBER and use qemu_opt_get_number()? > + if (address < 0 || address > CHR_I2C_ADDR_10BIT_MAX) { > + error_setg(errp, "chardev: linux-i2c: device address out of range"); > + return; > + } > + backend->type = CHARDEV_BACKEND_KIND_I2C; > + i2c = backend->u.i2c.data = g_new0(ChardevI2c, 1); > + qemu_chr_parse_common(opts, qapi_ChardevI2c_base(i2c)); > + i2c->device = g_strdup(device); > + i2c->address = (int16_t) address; No space between (int16_t) and address, please. Same for other type casts elsewhere. > +} > + > +static void char_i2c_class_init(ObjectClass *oc, void *data) > +{ > + ChardevClass *cc = CHARDEV_CLASS(oc); > + > + cc->parse = qemu_chr_parse_i2c; > + cc->open = qmp_chardev_open_i2c; > + cc->chr_ioctl = i2c_ioctl; > +} > + > +static const TypeInfo char_i2c_type_info = { > + .name = TYPE_CHARDEV_I2C, > + .parent = TYPE_CHARDEV_FD, > + .class_init = char_i2c_class_init, > +}; > + > +static void register_types(void) > +{ > + type_register_static(&char_i2c_type_info); > +} > + > +type_init(register_types); > diff --git a/chardev/char.c b/chardev/char.c > index 54724a56b1..93732a9909 100644 > --- a/chardev/char.c > +++ b/chardev/char.c > @@ -926,6 +926,9 @@ QemuOptsList qemu_chardev_opts = { > },{ > .name = "logappend", > .type = QEMU_OPT_BOOL, > + },{ > + .name = "address", > + .type = QEMU_OPT_STRING, > }, > { /* end of list */ } > }, > diff --git a/include/chardev/char-i2c.h b/include/chardev/char-i2c.h > new file mode 100644 > index 0000000000..81e97b7556 > --- /dev/null > +++ b/include/chardev/char-i2c.h > @@ -0,0 +1,35 @@ > + > +/* > + * QEMU System Emulator > + * > + * Copyright (c) 2019 Ernest Esene <erok...@gmail.com> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > copy > + * of this software and associated documentation files (the "Software"), to > deal > + * in the Software without restriction, including without limitation the > rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > +#ifndef CHAR_I2C_H > +#define CHAR_I2C_H > + > +#define CHR_IOCTL_I2C_SET_ADDR 1 > + > +#define CHR_I2C_ADDR_10BIT_MAX 1023 > +#define CHR_I2C_ADDR_7BIT_MAX 127 > + > +void qemu_set_block(int fd); Declaring qemu_set_block() again is inappropriate. Include qemu/sockets.h instead. > + > +#endif /* ifndef CHAR_I2C_H */ This header is included only by chardev/char.c. Why does it exist? > diff --git a/include/chardev/char.h b/include/chardev/char.h > index c0b57f7685..880614391f 100644 > --- a/include/chardev/char.h > +++ b/include/chardev/char.h > @@ -245,6 +245,7 @@ int qemu_chr_wait_connected(Chardev *chr, Error **errp); > #define TYPE_CHARDEV_SERIAL "chardev-serial" > #define TYPE_CHARDEV_SOCKET "chardev-socket" > #define TYPE_CHARDEV_UDP "chardev-udp" > +#define TYPE_CHARDEV_I2C "chardev-linux-i2c" > > #define CHARDEV_IS_RINGBUF(chr) \ > object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_RINGBUF) > diff --git a/qapi/char.json b/qapi/char.json > index a6e81ac7bc..2c05d6a93e 100644 > --- a/qapi/char.json > +++ b/qapi/char.json > @@ -240,6 +240,23 @@ > 'data': { 'device': 'str' }, > 'base': 'ChardevCommon' } > > +## > +# @ChardevI2c: > +# > +# Configuration info for i2c chardev. > +# > +# @device: The name of the special file for the device, > +# i.e. /dev/i2c-0 on linux > +# @address: The address of the i2c device on the host. > +# > +# Since: 4.1 > +## > +{ 'struct': 'ChardevI2c', > + 'data': { 'device': 'str', > + 'address': 'int16'}, > + 'base': 'ChardevCommon', > + 'if': 'defined(CONFIG_LINUX)'} > + > ## > # @ChardevSocket: > # > @@ -398,6 +415,7 @@ > 'data': { 'file': 'ChardevFile', > 'serial': 'ChardevHostdev', > 'parallel': 'ChardevHostdev', > + 'i2c': 'ChardevI2c', > 'pipe': 'ChardevHostdev', > 'socket': 'ChardevSocket', > 'udp': 'ChardevUdp', Missing: documentation update for qemu-options.hx.