Re: [RFC PATCH 1/4] espi: add core bus framework
From: M, Krishnamoorthi
Date: Wed Aug 12 2026 - 08:55:40 EST
Hi,
On 8/6/2026 7:01 PM, Uwe Kleine-König wrote:
Hello,
On Tue, Aug 04, 2026 at 05:22:56PM +0530, Krishnamoorthi M wrote:
diff --git a/drivers/espi/Kconfig b/drivers/espi/Kconfig
new file mode 100644
index 000000000000..4411d8336e82
--- /dev/null
+++ b/drivers/espi/Kconfig
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# eSPI (Enhanced Serial Peripheral Interface) bus configuration
+#
+
+menuconfig ESPI
+ bool "eSPI (Enhanced Serial Peripheral Interface) bus support"
Can this be tristate instead?
Yes. CONFIG_SPI is bool, but CONFIG_I2C and CONFIG_I3C are both tristate, allowing the bus framework to be built as a module. Given that eSPI controller and slave drivers are expected to be modular, tristate is the better fit and is consistent with the majority of comparable bus frameworks.
+ help
+ Enhanced Serial Peripheral Interface (eSPI) bus framework.
+
[...]
+const struct bus_type espi_bus_type = {
+ .name = "espi",
+ .match = espi_bus_match,
+ .uevent = espi_bus_uevent,
+ .probe = espi_bus_probe,
+ .remove = espi_bus_remove,
+};
+EXPORT_SYMBOL_GPL(espi_bus_type);
Do you really need this exported?
Yes, it is required. Controller and slave drivers built as modules reference espi_bus_type directly when registering devices. Without the export they fail to link.
+static ssize_t supported_channels_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct espi_controller *ctrl = to_espi_controller(dev);
+
+ return sysfs_emit(buf, "0x%02x\n", ctrl->caps.supported_channels);
+}
+static DEVICE_ATTR_RO(supported_channels);
+
+static ssize_t max_freq_mhz_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct espi_controller *ctrl = to_espi_controller(dev);
+
+ return sysfs_emit(buf, "%u\n", ctrl->caps.max_freq_mhz);
Is the unit here mHz or MHz? Maybe pick a better name that answers this
question.
The value is in MHz. we will rename the attribute to make the unit explicit.
+}
+static DEVICE_ATTR_RO(max_freq_mhz);
+
+static ssize_t io_mode_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct espi_controller *ctrl = to_espi_controller(dev);
+ static const char * const modes[] = { "single", "dual", "quad" };
+ u8 m = ctrl->caps.io_mode;
+
+ if (WARN_ON_ONCE(m > ESPI_IO_MODE_QUAD))
+ return sysfs_emit(buf, "unknown\n");
+ return sysfs_emit(buf, "%s\n", modes[m]);
+}
+static DEVICE_ATTR_RO(io_mode);
+
+static ssize_t channel_enabled_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct espi_controller *ctrl = to_espi_controller(dev);
+
+ return sysfs_emit(buf, "0x%02x\n", READ_ONCE(ctrl->channel_enabled));
+}
+static DEVICE_ATTR_RO(channel_enabled);
+
+static struct attribute *espi_controller_attrs[] = {
+ &dev_attr_supported_channels.attr,
+ &dev_attr_max_freq_mhz.attr,
+ &dev_attr_io_mode.attr,
+ &dev_attr_channel_enabled.attr,
+ NULL,
No , after the list terminator please.
Ack.
+};
+ATTRIBUTE_GROUPS(espi_controller);
+
+static void espi_controller_release(struct device *dev)
+{
+ struct espi_controller *ctrl = to_espi_controller(dev);
+
+ mutex_destroy(&ctrl->lock);
+ mutex_destroy(&ctrl->device_list_lock);
+ kfree(ctrl);
+}
+
+static const struct device_type espi_controller_type = {
+ .groups = espi_controller_groups,
+ .release = espi_controller_release,
+};
+
+struct espi_controller *espi_controller_alloc(struct device *parent,
+ unsigned int size)
+{
+ struct espi_controller *ctrl;
+
+ if (!parent)
+ return ERR_PTR(-EINVAL);
+
+ ctrl = kzalloc(sizeof(*ctrl) + size, GFP_KERNEL);
You might want to align sizeof(*ctrl) to something like
ARCH_DMA_MINALIGN, to ensure that devdata below is aligned
appropriately. Also use size_add() instead of direct arithmetic with
sizes.
Agreed. I'll align the private-data offset with dma_get_cache_alignment() like SPI does and use size_add() for the allocation size. Will fix it.
+ if (!ctrl)
+ return ERR_PTR(-ENOMEM);
+
+ device_initialize(&ctrl->dev);
+ ctrl->dev.parent = parent;
+ ctrl->dev.type = &espi_controller_type;
+
+ mutex_init(&ctrl->lock);
+ INIT_LIST_HEAD(&ctrl->device_list);
+ mutex_init(&ctrl->device_list_lock);
+ BLOCKING_INIT_NOTIFIER_HEAD(&ctrl->notifier_list);
+
+ if (size)
+ espi_controller_set_devdata(ctrl, (void *)ctrl + sizeof(*ctrl));
+
+ return ctrl;
+}
+EXPORT_SYMBOL_GPL(espi_controller_alloc);
+
+int espi_controller_register(struct espi_controller *ctrl)
+{
+ int ret;
+ u32 id;
+
+ if (!ctrl || !ctrl->ops)
+ return -EINVAL;
+
+ ret = xa_alloc(&espi_controllers, &id, ctrl, xa_limit_31b,
+ GFP_KERNEL);
+ if (ret)
+ return ret;
+
+ ctrl->bus_num = id;
+ ret = dev_set_name(&ctrl->dev, "espi%d", ctrl->bus_num);
+ if (ret)
+ goto err_erase;
+
+ if (ctrl->ops->setup) {
+ ret = ctrl->ops->setup(ctrl);
+ if (ret) {
+ dev_err(&ctrl->dev, "controller setup failed: %d\n", ret);
+ goto err_erase;
+ }
+ }
is ops->setup supposed to be only called in espi_controller_register()?
If yes, why does it exist? The driver specific stuff in it can just be
done before espi_controller_register() is called, can it not?
You're right. ops->setup is only called from espi_controller_register() and nothing in it needs core state, so drivers can do that work before calling register. I will drop it.
+ ret = device_add(&ctrl->dev);
+ if (ret) {
+ dev_err(&ctrl->dev, "device_add failed: %d\n", ret);
Better use %pe for error codes.
Noted. Will change it.
+ if (ctrl->ops->cleanup)
+ ctrl->ops->cleanup(ctrl);
+ goto err_erase;
+ }
+
+ dev_info(&ctrl->dev, "registered: channels=0x%02x freq=%uMHz\n",
+ ctrl->caps.supported_channels, ctrl->caps.max_freq_mhz);
Please degrade that to dev_dbg. We're already have too many messages
during boot that are not really usefull once driver/subsystem debugging
is done.
Also I'd add a space between "%u" and "MHz".
Sure. I will change it to dev_dbg() and add the space.
+ return 0;
+
+err_erase:
+ xa_erase(&espi_controllers, ctrl->bus_num);
+ ctrl->bus_num = -1;
+ return ret;
+}
+EXPORT_SYMBOL_GPL(espi_controller_register);
+
+void espi_controller_unregister(struct espi_controller *ctrl)
+{
+ if (!ctrl)
+ return;
+ /*
+ * Remove from the lookup table before dropping the device reference,
+ * so a concurrent espi_controller_get_by_bus_num() can never take a
+ * reference on a controller that is going away.
+ */
+ xa_erase(&espi_controllers, ctrl->bus_num);
+ if (ctrl->ops && ctrl->ops->cleanup)
+ ctrl->ops->cleanup(ctrl);
+ device_unregister(&ctrl->dev);
+}
+EXPORT_SYMBOL_GPL(espi_controller_unregister);
+
+void espi_controller_put(struct espi_controller *ctrl)
+{
+ if (ctrl)
+ put_device(&ctrl->dev);
+}
+EXPORT_SYMBOL_GPL(espi_controller_put);
+
+struct espi_controller *espi_controller_get_by_bus_num(int bus_num)
+{
+ struct espi_controller *ctrl;
+
+ guard(spinlock)(&espi_controllers.xa_lock);
+ ctrl = xa_load(&espi_controllers, bus_num);
+ if (ctrl)
+ get_device(&ctrl->dev);
+ return ctrl;
+}
+EXPORT_SYMBOL_GPL(espi_controller_get_by_bus_num);
+
+int espi_get_capabilities(struct espi_controller *ctrl,
+ struct espi_capabilities *caps)
+{
+ if (!ctrl || !caps)
+ return -EINVAL;
+ guard(mutex)(&ctrl->lock);
+ *caps = ctrl->caps;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(espi_get_capabilities);
+
+bool espi_channel_is_enabled(struct espi_controller *ctrl, u8 channel)
+{
+ if (!ctrl || channel >= ESPI_CHANNEL_COUNT)
+ return false;
+ guard(mutex)(&ctrl->lock);
+ return !!(ctrl->channel_enabled & BIT(channel));
+}
+EXPORT_SYMBOL_GPL(espi_channel_is_enabled);
+
+int espi_get_configuration(struct espi_controller *ctrl,
+ u32 slave_reg_addr, u32 *config)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->get_configuration)
+ return -EOPNOTSUPP;
+ if (!config)
+ return -EINVAL;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->get_configuration(ctrl, slave_reg_addr, config);
+}
+EXPORT_SYMBOL_GPL(espi_get_configuration);
+
+int espi_set_configuration(struct espi_controller *ctrl,
+ u32 slave_reg_addr, u32 config)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->set_configuration)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->set_configuration(ctrl, slave_reg_addr, config);
+}
+EXPORT_SYMBOL_GPL(espi_set_configuration);
+
+int espi_inband_reset(struct espi_controller *ctrl)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->inband_reset)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->inband_reset(ctrl);
+}
+EXPORT_SYMBOL_GPL(espi_inband_reset);
+
+int espi_get_status(struct espi_controller *ctrl,
+ struct espi_slave_status *status)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->get_status)
+ return -EOPNOTSUPP;
+ if (!status)
+ return -EINVAL;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->get_status(ctrl, status);
+}
+EXPORT_SYMBOL_GPL(espi_get_status);
+
+int espi_enable_channel(struct espi_controller *ctrl, u8 channel)
+{
+ int ret;
+
+ if (!ctrl || !ctrl->ops || !ctrl->ops->enable_channel)
+ return -EOPNOTSUPP;
+ if (channel >= ESPI_CHANNEL_COUNT)
+ return -EINVAL;
+ guard(mutex)(&ctrl->lock);
+ ret = ctrl->ops->enable_channel(ctrl, channel);
+ if (!ret)
+ ctrl->channel_enabled |= BIT(channel);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(espi_enable_channel);
+
+int espi_disable_channel(struct espi_controller *ctrl, u8 channel)
+{
+ int ret;
+
+ if (!ctrl || !ctrl->ops || !ctrl->ops->disable_channel)
+ return -EOPNOTSUPP;
+ if (channel >= ESPI_CHANNEL_COUNT)
+ return -EINVAL;
+ guard(mutex)(&ctrl->lock);
+ ret = ctrl->ops->disable_channel(ctrl, channel);
+ if (!ret)
+ ctrl->channel_enabled &= ~BIT(channel);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(espi_disable_channel);
+
+int espi_periph_io_read(struct espi_controller *ctrl,
+ u16 port, u8 width, u32 *value)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->periph_io_read)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->periph_io_read(ctrl, port, width, value);
+}
+EXPORT_SYMBOL_GPL(espi_periph_io_read);
+
+int espi_periph_io_write(struct espi_controller *ctrl,
+ u16 port, u8 width, u32 value)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->periph_io_write)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->periph_io_write(ctrl, port, width, value);
+}
+EXPORT_SYMBOL_GPL(espi_periph_io_write);
+
+int espi_periph_mem_read(struct espi_controller *ctrl,
+ u32 addr, void *buf, size_t len)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->periph_mem_read)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->periph_mem_read(ctrl, addr, buf, len);
+}
+EXPORT_SYMBOL_GPL(espi_periph_mem_read);
+
+int espi_periph_mem_write(struct espi_controller *ctrl,
+ u32 addr, const void *buf, size_t len)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->periph_mem_write)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->periph_mem_write(ctrl, addr, buf, len);
+}
+EXPORT_SYMBOL_GPL(espi_periph_mem_write);
+
+int espi_vwire_get(struct espi_controller *ctrl, u8 index, u8 *value, u8 *valid)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->vwire_get)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->vwire_get(ctrl, index, value, valid);
+}
+EXPORT_SYMBOL_GPL(espi_vwire_get);
+
+int espi_vwire_put(struct espi_controller *ctrl, u8 index, u8 value, u8 valid)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->vwire_put)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->vwire_put(ctrl, index, value, valid);
+}
+EXPORT_SYMBOL_GPL(espi_vwire_put);
+
+int espi_oob_send(struct espi_controller *ctrl, const void *buf, size_t len, u8 tag)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->oob_send)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->oob_send(ctrl, buf, len, tag);
+}
+EXPORT_SYMBOL_GPL(espi_oob_send);
+
+int espi_oob_recv(struct espi_controller *ctrl, void *buf, size_t *len, u8 *tag)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->oob_recv)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->oob_recv(ctrl, buf, len, tag);
+}
+EXPORT_SYMBOL_GPL(espi_oob_recv);
+
+int espi_flash_read(struct espi_controller *ctrl, u32 offset, void *buf, size_t len)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->flash_read)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->flash_read(ctrl, offset, buf, len);
+}
+EXPORT_SYMBOL_GPL(espi_flash_read);
+
+int espi_flash_write(struct espi_controller *ctrl, u32 offset, const void *buf, size_t len)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->flash_write)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->flash_write(ctrl, offset, buf, len);
+}
+EXPORT_SYMBOL_GPL(espi_flash_write);
+
+int espi_flash_erase(struct espi_controller *ctrl, u32 offset, size_t len)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->flash_erase)
+ return -EOPNOTSUPP;
+ guard(mutex)(&ctrl->lock);
+ return ctrl->ops->flash_erase(ctrl, offset, len);
+}
+EXPORT_SYMBOL_GPL(espi_flash_erase);
+
+/*
+ * espi_handle_alert - dispatch a hardware alert to the controller
+ *
+ * Must be called from process context (threaded IRQ or workqueue).
+ *
+ * ctrl->lock is NOT held across ops->handle_alert so that the driver
+ * callback can call espi_notify_event() without deadlocking: notifier
+ * callbacks may in turn call channel APIs that also acquire ctrl->lock.
+ * The driver is responsible for taking ctrl->lock around any register
+ * accesses that need serialisation with the channel API.
+ */
+int espi_handle_alert(struct espi_controller *ctrl)
+{
+ if (!ctrl || !ctrl->ops || !ctrl->ops->handle_alert)
+ return -EOPNOTSUPP;
+ return ctrl->ops->handle_alert(ctrl);
+}
+EXPORT_SYMBOL_GPL(espi_handle_alert);
+
+int __espi_register_driver(struct module *owner, struct espi_driver *drv)
+{
+ drv->driver.owner = owner;
+ drv->driver.bus = &espi_bus_type;
+ return driver_register(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(__espi_register_driver);
+
+void espi_unregister_driver(struct espi_driver *drv)
+{
+ driver_unregister(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(espi_unregister_driver);
+
+static int __init espi_init(void)
+{
+ int ret = bus_register(&espi_bus_type);
+
+ if (ret)
+ pr_err("failed to register eSPI bus: %d\n", ret);
+ return ret;
+}
+postcore_initcall(espi_init);
+
+MODULE_AUTHOR("Krishnamoorthi M <krishnamoorthi.m@xxxxxxx>");
+MODULE_DESCRIPTION("eSPI core framework");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/espi/espi.h b/include/linux/espi/espi.h
new file mode 100644
index 000000000000..a191ddc10cdd
--- /dev/null
+++ b/include/linux/espi/espi.h
@@ -0,0 +1,345 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * eSPI (Enhanced Serial Peripheral Interface) framework
+ *
+ * Copyright (c) 2026, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ */
+#ifndef _LINUX_ESPI_ESPI_H
+#define _LINUX_ESPI_ESPI_H
+
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+#include <linux/mod_devicetable.h>
Please don't include <linux/mod_devicetable.h>. I think you're not even
using a symbol defined by it, so you can just drop it.
Ack.
[...]
+struct espi_device_id {
+ char name[ESPI_NAME_SIZE];
+ kernel_ulong_t driver_data;
There is an effort to replace .driver_data by an anonymous union for the
already existing *_device_id. See
https://lore.kernel.org/all/cover.1780048925.git.u.kleine-koenig@xxxxxxxxxxxx/
for details. It would be awesome if you'd do that from the start.
Sure. Will change it. Thanks for the pointer.
Thanks,
Krishna
+};
Best regards
Uwe