LKML Archive on lore.kernel.org help / color / mirror / Atom feed
From: Jeremy Kerr <firstname.lastname@example.org> To: ChiaWei Wang <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com>, "firstname.lastname@example.org" <email@example.com> Subject: Re: [PATCH v3 3/4] soc: aspeed: Add eSPI driver Date: Fri, 27 Aug 2021 12:36:37 +0800 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <HK0PR06MB377997422D9508894936E39691C89@HK0PR06MB3779.apcprd06.prod.outlook.com> 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 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. > 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(). 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. Cheers, Jeremy
next prev parent reply other threads:[~2021-08-27 4:36 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-26 6:16 [PATCH v3 0/4] arm: aspeed: Add eSPI support Chia-Wei Wang 2021-08-26 6:16 ` [PATCH v3 1/4] dt-bindings: aspeed: Add eSPI controller Chia-Wei Wang 2021-08-26 13:26 ` Rob Herring 2021-08-27 3:08 ` ChiaWei Wang 2021-08-26 6:16 ` [PATCH v3 2/4] MAINTAINER: Add ASPEED eSPI driver entry Chia-Wei Wang 2021-08-26 6:16 ` [PATCH v3 3/4] soc: aspeed: Add eSPI driver Chia-Wei Wang 2021-08-26 20:05 ` kernel test robot 2021-08-26 23:30 ` kernel test robot 2021-08-27 3:52 ` ChiaWei Wang 2021-08-27 5:48 ` Joel Stanley 2021-08-27 8:49 ` ChiaWei Wang 2021-08-27 3:20 ` Jeremy Kerr 2021-08-27 3:48 ` ChiaWei Wang 2021-08-27 4:36 ` Jeremy Kerr [this message] 2021-08-27 8:49 ` ChiaWei Wang 2021-08-26 6:16 ` [PATCH v3 4/4] ARM: dts: aspeed: Add eSPI node Chia-Wei Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).