[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] chardev/char-i2c: Implement Linux I2C charac
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2] chardev/char-i2c: Implement Linux I2C character device |
Date: |
Tue, 07 May 2019 19:33:09 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Ernest Esene <address@hidden> 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 <address@hidden>
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 <address@hidden>
> S: Maintained
> F: chardev/baum.c
>
> +Character Devices (Linux I2C)
> +M: Ernest Esene <address@hidden>
> +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 <address@hidden>
> 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 <address@hidden>
> + *
> + * 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 <address@hidden>
> + *
> + * 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.