On 2021/3/2 12:22, Viresh Kumar wrote:> On 02-03-21, 09:31, Viresh Kumar wrote: >> On 01-03-21, 16:19, Arnd Bergmann wrote: >>> On Mon, Mar 1, 2021 at 7:41 AM Jie Deng <jie.deng at intel.com> wrote: >>> >>>> --- /dev/null >>>> +++ b/include/uapi/linux/virtio_i2c.h >>>> @@ -0,0 +1,56 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */ >>>> +/* >>>> + * Definitions for virtio I2C Adpter >>>> + * >>>> + * Copyright (c) 2021 Intel Corporation. All rights reserved. >>>> + */ >>>> + >>>> +#ifndef _UAPI_LINUX_VIRTIO_I2C_H >>>> +#define _UAPI_LINUX_VIRTIO_I2C_H >>> Why is this a uapi header? Can't this all be moved into the driver >>> itself? >>> >>>> +/** >>>> + * struct virtio_i2c_req - the virtio I2C request structure >>>> + * @out_hdr: the OUT header of the virtio I2C message >>>> + * @write_buf: contains one I2C segment being written to the device >>>> + * @read_buf: contains one I2C segment being read from the device >>>> + * @in_hdr: the IN header of the virtio I2C message >>>> + */ >>>> +struct virtio_i2c_req { >>>> + struct virtio_i2c_out_hdr out_hdr; >>>> + u8 *write_buf; >>>> + u8 *read_buf; >>>> + struct virtio_i2c_in_hdr in_hdr; >>>> +}; >>> In particular, this structure looks like it is only ever usable between >>> the transfer functions in the driver itself, it is shared with neither >>> user space nor the virtio host side. >> Why is it so ? Won't you expect hypervisors or userspace apps to use >> these ? > This comment applies only for the first two structures as the third > one is never exchanged over virtio.Yeah. Actually, the backend only needs "struct virtio_i2c_out_hdr out_hdr" and "struct virtio_i2c_in_hdr in_hdr" for communication. So we only need to keep the first two in uapi and move "struct virtio_i2c_req" into the driver. But Jason wanted to include "struct virtio_i2c_req" in uapi. He explained in this link https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html. Do you agree with that explanation ?