LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: ChiaWei Wang <email@example.com>
To: Jeremy Kerr <firstname.lastname@example.org>,
Subject: RE: [PATCH v3 3/4] soc: aspeed: Add eSPI driver
Date: Fri, 27 Aug 2021 08:49:13 +0000 [thread overview]
Message-ID: <HK0PR06MB3779859F4193D2F34759AFEC91C89@HK0PR06MB3779.apcprd06.prod.outlook.com> (raw)
> From: Jeremy Kerr <email@example.com>
> 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
> 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.
next prev parent reply other threads:[~2021-08-27 8:49 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
2021-08-27 8:49 ` ChiaWei Wang [this message]
2021-08-26 6:16 ` [PATCH v3 4/4] ARM: dts: aspeed: Add eSPI node Chia-Wei Wang
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='RE: [PATCH v3 3/4] soc: aspeed: Add eSPI driver' \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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).