Arnd Bergmann
2021-Mar-01 15:19 UTC
[PATCH v5] i2c: virtio: add a virtio i2c frontend driver
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_HWhy 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. Arnd
On 2021/3/1 23: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. > > ArndPlease check this link. https://lists.linuxfoundation.org/pipermail/virtualization/2020-October/050222.html Jason/Stefan, could you please double confirm about the following ? **************************************************************************>>>>>/I think following definition in uAPI for the status is enough. />>>>>/There is no need to provide a "u8" status in the structure. />>>>>//>>>>>//* The final status written by the device */ />>>>>/#define VIRTIO_I2C_MSG_OK??? 0 />>>>>/#define VIRTIO_I2C_MSG_ERR??? 1 />>>>>//>>>>>/You can see an example in virtio_blk. />>>>>//>>>>>/In the spec: />>>>>//>>>>>/struct virtio_blk_req { />>>>>/le32 type; />>>>>/le32 reserved; />>>>>/le64 sector; />>>>>/u8 data[]; />>>>>/u8 status; />>>>>/}; />>>>>//>>>>>/In virtio_blk.h, there is only following definitions. />>>>>//>>>>>/#define VIRTIO_BLK_S_OK??? ??? 0 />>>>>/#define VIRTIO_BLK_S_IOERR??? 1 />>>>>/#define VIRTIO_BLK_S_UNSUPP??? 2 />>>>>//>>>>//>>>>/virtio-blk is a bad example, it's just too late to fix. For any new />>>>/introduced uAPI it should be a complete one. />>>>//>>>>/Thanks />>>>//>>>/I checked a relatively new device "virtio_fs". />>>/I found following definition in spec but not in uAPI also. />>>//>>>/struct virtio_fs_req { />>>/// Device -readable part />>>/struct fuse_in_header in; />>>/u8 datain[]; />>>/// Device -writable part />>>/struct fuse_out_header out; />>>/u8 dataout[]; />>>/}; />>>//>>>/So is this also a bad example which has not been fixed yet. />>//>>//>>/Cc Stefan for the answer. />>//>>//>>>/Or what's your mean about "complete" here ? Is there any definition />>>/about "complete uAPI" ? />>//>>//>>/My understanding it should contain all the fields defined in the />>/virtio spec. />>//>>/Thanks />>//>/OK. I noticed this isn't strictly implemented in the current virtio />/codes. />/I'm not sure if this is already a consensus. I will follow it if this />/is the opinion of the majority. /Please do that, this forces us to maintain uABI compatibility which is what Linux try to maintain for ever. ****************************************************************************** -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210302/a00bcd34/attachment.html>