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

[Shulzhenko, Oleksandr]

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

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@xxxxxxxxxxxxxx>
---
[...]
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->dev = dev;
+       match = of_match_device(aspeed_espi_of_matches, dev);
+       if (!match)
+               return -ENODEV;
+
+       espi->pdev = pdev;
+       espi->ops = match->data;
+       if (!espi->ops || !espi->ops->espi_isr)
+               return -EINVAL;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       if (!res) {
+               dev_err(dev, "cannot get resource\n");
+               return -ENODEV;
+       }
+
+       espi->regs = devm_ioremap_resource(dev, res);
+       if (IS_ERR(espi->regs)) {
+               dev_err(dev, "cannot map registers\n");
+               return PTR_ERR(espi->regs);
+       }
+
+       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);
+       if (IS_ERR(espi->rst)) {
+               dev_err(dev, "cannot get reset control\n");
+               return PTR_ERR(espi->rst);
+       }
+
+       espi->clk = devm_clk_get(dev, NULL);
+       if (IS_ERR(espi->clk)) {
+               dev_err(dev, "cannot get clock control\n");
+               return PTR_ERR(espi->clk);
+       }
+
+       rc = clk_prepare_enable(espi->clk);
+       if (rc) {
+               dev_err(dev, "cannot enable clocks\n");
+               return rc;
+       }
> +
> +     if (espi->ops->espi_pre_init)
> +             espi->ops->espi_pre_init(espi);
is there a chance ops are unitialized? In any place you do this check
+
+       rc = devm_request_irq(dev, espi->irq, espi->ops->espi_isr, 0,
+                             dev_name(dev), espi);
+       if (rc) {
+               dev_err(dev, "cannot request IRQ\n");
+               goto err_deinit;
+       }
+
+       if (espi->ops->espi_post_init)
+               espi->ops->espi_post_init(espi);
+
+       platform_set_drvdata(pdev, espi);
+
+       dev_info(dev, "module loaded\n");
+
+       return 0;
+
+err_deinit:
+       if (espi->ops->espi_deinit)
+               espi->ops->espi_deinit(espi);
+       clk_disable_unprepare(espi->clk);
+
+       return rc;
+}
+
> +static void aspeed_espi_remove(struct platform_device *pdev)
> +{
> +     struct aspeed_espi *espi;
> +
> +     espi = platform_get_drvdata(pdev);
> +
> +     if (!espi)
> +             return;
espi pointer is unlikely to be null here, I suggest removing the NULL check
+
+       if (espi->ops->espi_deinit)
+               espi->ops->espi_deinit(espi);
+
+       clk_disable_unprepare(espi->clk);
+}
+
+static struct platform_driver aspeed_espi_driver = {
+       .driver = {
+               .name = "aspeed-espi",
+               .of_match_table = aspeed_espi_of_matches,
+       },
+       .probe = aspeed_espi_probe,
+       .remove = aspeed_espi_remove,
+};
+
+module_platform_driver(aspeed_espi_driver);
+
+MODULE_AUTHOR("Aspeed Technology Inc.");
+MODULE_DESCRIPTION("Aspeed eSPI controller");
+MODULE_LICENSE("GPL");
[...]

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.