Hi Jeremy > From: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx> > Sent: Friday, August 27, 2021 12:37 PM > > Hi Chiawei, > > > The eSPI slave device comprises four channels, where each of them has > > individual functionality. Putting the four channels driver code into > > a single file makes it hard to maintain and trace. > > Yep, understood. > > > We did consider to make them standard .c files. > > But it requires to export channel functions into kernel space although > > they are dedicated only to this eSPI driver. > > What do you mean by "export into kernel space" here? The function prototypes The channel functions will be visible to all kernel driver files. > need to be available to your main (-ctrl.c) file, regardless of whether you're > putting the entire functions in a header file, or just the prototype. There's > doesn't need to be any difference in visibility outside of your own module if > you were to do this the usual way. Maybe I was trying to make channels function visible only to espi-ctrl.c too far. I will revise the driver to present in the usual .c way. > > > As espi-ctrl needs to invoke corresponding channel functions when it > > is interrupted by eSPI events. > > > > To avoid polluting kernel space, we decided to put driver code in > > header files and make the channel functions 'static'. > > > > BTW, I once encountered .c file inclusion in other projects. Is it > > proper for Linux driver development? > > It can be, just that in this case it's a bit unusual, and I can't see a good reason > for doing so. This could just be a standard multiple-source-file module. > > > eSPI communication is based on the its cycle packet format. > > We intended to let userspace decided how to interpret and compose > > TX/RX packets including header, tag, length (encoded), and data. > > IOCTL comes to our first mind as it also works in the 'packet' like > > paradigm. > > But you're not always exposing a packet-like interface for this. For example, > your virtual-wire interface just has a get/set interface for bits in a register > (plus some PCH event handling, which may not be applicable to all > platforms...). > > The other channels do look like more of a packet interface though, but in that > case I'm not convinced that an ioctl interface is the best way to go for that. > You're essentially sending a (length, pointer) pair over the ioctls there, which > sounds more like a write() than an ioctl(). In most cases, yes. Currently only the peripheral channel has more than the 2 (put tx/get rx) IOCTL code. We think it might be a good idea to make the user interfaces of all channels consistent using IOCTL. > > Regardless of the choice of interface though, this will definitely need some > documentation or description of the API, and the ioc header to be somewhere > useful for userspace to consume. > > With that documented, we'd have a better idea of how the new ABI is > supposed to work. Sure. more comments will be added in aspeed-espi-ioc.h to describe the usage and the purpose. Thanks for your feedback. Chiawei
