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

[Markus Armbruster <armbru@redhat.com>]

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 ;)