Re: [PATCH 2/7] soc: aspeed: Introduce core eSPI controller support

Philipp Zabel
2026-03-16T09:58:15Z

On Fr, 2026-03-13 at 18:07 +0800, aspeedyh wrote:
> Add core eSPI controller support and common code for ASPEED SoCs. The
> eSPI engine is a slave device in BMC to communicate with the Host over
> the eSPI interface.
>
> The initial support includes basic eSPI driver probe/remove operations,
> and provides operators for ASPEED SoCs to implement their own eSPI slave
> device drivers that are different among SoC models.
>
> Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
> ---
> drivers/soc/aspeed/Kconfig | 7 ++
> drivers/soc/aspeed/Makefile | 1 +
> drivers/soc/aspeed/espi/Makefile | 1 +
> drivers/soc/aspeed/espi/aspeed-espi.c | 143 ++++++++++++++++++++++++++++++++++
> drivers/soc/aspeed/espi/aspeed-espi.h | 27 +++++++
> 5 files changed, 179 insertions(+)
>
[...]
> diff --git a/drivers/soc/aspeed/espi/aspeed-espi.c b/drivers/soc/aspeed/espi/aspeed-espi.c
> new file mode 100644
> index 000000000000..15d58b38bbe4
> --- /dev/null
> +++ b/drivers/soc/aspeed/espi/aspeed-espi.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Unified Aspeed eSPI driver framework for different generation SoCs
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +#include "aspeed-espi.h"
> +
> +struct aspeed_espi_ops {
> + void (*espi_pre_init)(struct aspeed_espi *espi);
> + void (*espi_post_init)(struct aspeed_espi *espi);
> + void (*espi_deinit)(struct aspeed_espi *espi);
> + irqreturn_t (*espi_isr)(int irq, void *espi);
> +};
> +
> +static const struct of_device_id aspeed_espi_of_matches[] = {
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_espi_of_matches);
> +
> +static int aspeed_espi_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + struct aspeed_espi *espi;
> + struct resource *res;
> + struct device *dev;
> + int rc;
> +
> + dev = &pdev->dev;
> + espi = devm_kzalloc(dev, sizeof(*espi), GFP_KERNEL);
> + if (!espi)
> + return -ENOMEM;
> +
[...]
> +
> + espi->irq = platform_get_irq(pdev, 0);
> + if (espi->irq < 0) {
> + dev_err(dev, "cannot get IRQ number\n");
> + return espi->irq;
> + }
> +
> + espi->rst = devm_reset_control_get_optional(dev, NULL);

Please use devm_reset_control_get_optional_exclusive() directly.

> + if (IS_ERR(espi->rst)) {
> + dev_err(dev, "cannot get reset control\n");
> + return PTR_ERR(espi->rst);

Consider using dev_err_probe, same for the other errors.
That way the driver won't print incorrect error messages on
-EPROBE_DEFER.

[...]
> diff --git a/drivers/soc/aspeed/espi/aspeed-espi.h b/drivers/soc/aspeed/espi/aspeed-espi.h
> new file mode 100644
> index 000000000000..f4ad7f61fef6
> --- /dev/null
> +++ b/drivers/soc/aspeed/espi/aspeed-espi.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Unified eSPI driver header file and data structures
> + * Copyright 2026 Aspeed Technology Inc.
> + */
> +#ifndef ASPEED_ESPI_H
> +#define ASPEED_ESPI_H
> +
> +#include <linux/irqreturn.h>
> +#include <linux/miscdevice.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#define DEVICE_NAME "aspeed-espi"
> +
> +struct aspeed_espi {
> + struct platform_device *pdev;
> + struct device *dev;

Storing both pdev and &pdev->dev seems unnecessary.
Is pdev used at all?

> + void __iomem *regs;
> + struct reset_control *rst;

This is missing a forward declaration for struct reset_control.

regards
Philipp