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>

---
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
+
 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.
+ */
+#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;
+
+        if (addr > CHR_I2C_ADDR_7BIT_MAX) {
+            /*
+             * TODO: check if adapter support 10-bit addr
+             * I2C_FUNC_10BIT_ADDR
+             */
+            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);
+    qemu_chr_open_fd(chr, fd, fd);
+    addr = (void *) (long) i2c->address;
+    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;
+    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;
+    }
+    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;
+}
+
+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);
+
+#endif /* ifndef CHAR_I2C_H */
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',
-- 
2.14.2

On Sat, May 04, 2019 at 07:11:19PM +0100, Ernest Esene wrote:
> 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

There is a mixture of "linux-i2c" and "char-i2c" names in this patch,
which I find confusing.  Maybe you changed your mind while writing this
code.  There are two options:

1. Call it "linux-i2c".  Other host operating systems will need their
   own equivalent objects.

2. Call it "char-i2c" and make all the parameters optional since they
   are likely to work differently on other host operating systems.

I tend towards the second approach because I think I2C is simple enough
that a single user-visible object can work on all host operating
systems.

Please make the naming consistent in the next revision of this patch.

> 
> Signed-off-by: Ernest Esene <eroken1@gmail.com>
> 
> ---
> 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

Please update the qemu-options.texi user documentation.

On Thu, May 09, 2019 at 02:00:56PM +0100, Stefan Hajnoczi wrote:
> On Sat, May 04, 2019 at 07:11:19PM +0100, Ernest Esene wrote:
> > 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
> 
> There is a mixture of "linux-i2c" and "char-i2c" names in this patch,
> which I find confusing.  Maybe you changed your mind while writing this
> code.  There are two options:
> 
> 1. Call it "linux-i2c".  Other host operating systems will need their
>    own equivalent objects.
> 
> 2. Call it "char-i2c" and make all the parameters optional since they
>    are likely to work differently on other host operating systems.
> 
> I tend towards the second approach because I think I2C is simple enough
> that a single user-visible object can work on all host operating
> systems.
> 
> Please make the naming consistent in the next revision of this patch.

My coding skills is limited to only Linux, I prefer the first approach
as is makes it easy.

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.

On Tue, May 07, 2019 at 07:33:09PM +0200, Markus Armbruster wrote:
> 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?
QEMU supports emulation of I2C devices in software but currently can't
passthrough to real I2C devices. This feature is needed by developers
using QEMU for writing and testing software for I2C devices.

> 
> Any particular reason not to use GPLv2+?
No, I used the wrong script. I'll update the licence.

> > +
> > +        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?
Not all I2C adapters supports 10-bit address.
> Should it be mentioned in the commit message?
I have handled it already.

> > +        return;
> > +    }
> > +    qemu_set_block(fd);
> 
> Sure we want *blocking* I/O?  No other character device does.
No, it is a mistake.
> 
> > +    qemu_chr_open_fd(chr, fd, fd);
> > +    addr = (void *) (long) i2c->address;
> 
> 
> Why not make option "addr" QEMU_OPT_NUMBER and use
> qemu_opt_get_number()?
I never knew QEMU_OPT_NUMBER can handle inputs such: "0x08 and 8",
appropriately.
> 
> Missing: documentation update for qemu-options.hx.
I don't know much about this format (.hx), I'll be happy to have any
useful documentation on this.

Thank you.
-Ernest Esene

Ernest Esene <eroken1@gmail.com> writes:

> On Tue, May 07, 2019 at 07:33:09PM +0200, Markus Armbruster wrote:
>> 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?
> QEMU supports emulation of I2C devices in software but currently can't
> passthrough to real I2C devices. This feature is needed by developers
> using QEMU for writing and testing software for I2C devices.

Please work that into your commit message.  Remember, you commit message
is also your patch's sales pitch.

>> Any particular reason not to use GPLv2+?
> No, I used the wrong script. I'll update the licence.

Thanks!

>> > +
>> > +        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?
> Not all I2C adapters supports 10-bit address.
>> Should it be mentioned in the commit message?
> I have handled it already.

Cool.

>> > +        return;
>> > +    }
>> > +    qemu_set_block(fd);
>> 
>> Sure we want *blocking* I/O?  No other character device does.
> No, it is a mistake.
>> 
>> > +    qemu_chr_open_fd(chr, fd, fd);
>> > +    addr = (void *) (long) i2c->address;
>> 
>> 
>> Why not make option "addr" QEMU_OPT_NUMBER and use
>> qemu_opt_get_number()?
> I never knew QEMU_OPT_NUMBER can handle inputs such: "0x08 and 8",
> appropriately.
>> 
>> Missing: documentation update for qemu-options.hx.
> I don't know much about this format (.hx), I'll be happy to have any
> useful documentation on this.

I don't think we have any documentation on it, let alone useful
documentation.

Here's what you need to do there for your new chardev backend type,
using your voodoo coding skills.

Find this line:
DEFHEADING(Character device options:)

This is where option -chardev is defined and documented.

Now pick an existing chardev backend.  Picking one that's vaguely
similar to yours is best.  Let's pick "serial", because it also passes
through a host device.

First occurence is this:

    #ifdef _WIN32
        "-chardev console,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
        "-chardev serial,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
    #else
        "-chardev pty,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
        "-chardev stdio,id=id[,mux=on|off][,signal=on|off][,logfile=PATH][,logappend=on|off]\n"
    #endif

You see _WIN32, and immediately skip to the next one:

    #if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \
            || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__)
        "-chardev serial,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
        "-chardev tty,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
    #endif


Using your vodoo coding skills, you add

    #ifdef CONFIG_POSIX
        "-chardev i2c,id=id,path=path,address=address[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
    #endif

CONFIG_POSIX, because that's what you used in Makefile.objs.  Your
commit message says "Linux I2C character device", so maybe you should
use CONFIG_LINUX instead of CONFIG_POSIX throughout, but what do I know.

The next occurence is

    The general form of a character device option is:
    @table @option
    @item -chardev @var{backend},id=@var{id}[,mux=on|off][,@var{options}]
    @findex -chardev
    Backend is one of:
    @option{null},
    @option{socket},
    @option{udp},
    @option{msmouse},
    @option{vc},
    @option{ringbuf},
    @option{file},
    @option{pipe},
    @option{console},
    @option{serial},

So you add

    @option{i2c},

I think you get the idea.  If not, the time-honored way to get more help
is to post a patch that's not quite right ;)

On 5/7/19 12:33 PM, Markus Armbruster wrote:

>> +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 you're trying to pass an integer through void*, it's probably best to
write:

addr = (intptr_t) arg;

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org