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

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.

> +	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