Re: [Qemu-devel] [PATCH v2] chardev/char-i2c: Implement Linux I2C character device

[Markus Armbruster <armbru@redhat.com>]

Ernest Esene <eroken1@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 <eroken1@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.thibault@ens-lyon.org>
>  S: Maintained
>  F: chardev/baum.c
>  
> +Character Devices (Linux I2C)
> +M: Ernest Esene <eroken1@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 <armbru@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 <eroken1@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 <eroken1@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.