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

YH Chung
2026-03-17T08:41:08Z

Hi Philipp,

Thanks for the review.

> 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.
>
I will update this to use devm_reset_control_get_optional_exclusive()

> > + 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.
I will switch these error paths to dev_err_probe() so that -EPROBE_DEFER can be handled correctly.

>
> [...]
> > 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?
>
Agreed, pdev is not used in the subsequent patches. I will remove it in the next revision.

> > + void __iomem *regs;
> > + struct reset_control *rst;
>
> This is missing a forward declaration for struct reset_control.
>
Will update to add <linux/reset.h> in the next revision to provide declaration for struct reset_control
>
> regards
> Philipp

Thanks,
Yun Hsuan.