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

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 <[email protected]>
> > ---
> >  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.