Re: [PATCH v15 2/2] spi: spi-qpic: add driver for QCOM SPI NAND flash Interface

[Geert Uytterhoeven]

Hi,

On Mon, 24 Feb 2025 at 12:15, Md Sadre Alam <quic_mdalam@xxxxxxxxxxx> wrote:
> This driver implements support for the SPI-NAND mode of QCOM NAND Flash
> Interface as a SPI-MEM controller with pipelined ECC capability.
>
> Co-developed-by: Sricharan Ramabadhran <quic_srichara@xxxxxxxxxxx>
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@xxxxxxxxxxx>
> Co-developed-by: Varadarajan Narayanan <quic_varada@xxxxxxxxxxx>
> Signed-off-by: Varadarajan Narayanan <quic_varada@xxxxxxxxxxx>
> Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>

Thanks for your patch, which is now commit 7304d1909080ef0c
("spi: spi-qpic: add driver for QCOM SPI NAND flash Interface")
upstream.

> Change in [v11]
>
> * Changed "depends on MTD" to "select MTD" in
>   drivers/spi/Kconfig file

Why? This is the only driver that selects MTD instead of depending on it.

> Change in [v7]
>
> * Made CONFIG_SPI_QPIC_SNAND as bool

Why? The driver uses MODULE_*, so a janitor may remove the latter.

> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -936,6 +936,15 @@ config SPI_QCOM_QSPI
>         help
>           QSPI(Quad SPI) driver for Qualcomm QSPI controller.
>
> +config SPI_QPIC_SNAND
> +       bool "QPIC SNAND controller"
> +       depends on ARCH_QCOM || COMPILE_TEST
> +       select MTD

As SPI_QPIC_SNAND is bool, this forces MTD (and various related symbols)
to be built-in, as seen in an allmodconfig kernel.

So I think SPI_QPIC_SNAND should be tristate, and it should depend on
MTD to avoid circular dependency issues.

> +       help
> +         QPIC_SNAND (QPIC SPI NAND) driver for Qualcomm QPIC controller.
> +         QPIC controller supports both parallel nand and serial nand.
> +         This config will enable serial nand driver for QPIC controller.
> +
>  config SPI_QUP
>         tristate "Qualcomm SPI controller with QUP interface"
>         depends on ARCH_QCOM || COMPILE_TEST

--- /dev/null
+++ b/drivers/spi/spi-qpic-snand.c

> +static const struct of_device_id qcom_snandc_of_match[] = {
> +       {
> +               .compatible = "qcom,ipq9574-snand",
> +               .data = &ipq9574_snandc_props,
> +       },
> +       {}
> +}

Missing semicolon, so that's why you are forcing the driver built-in? ;-)

> +MODULE_DEVICE_TABLE(of, qcom_snandc_of_match);
> +
> +static struct platform_driver qcom_spi_driver = {
> +       .driver = {
> +               .name           = "qcom_snand",
> +               .of_match_table = qcom_snandc_of_match,
> +       },
> +       .probe = qcom_spi_probe,
> +       .remove = qcom_spi_remove,
> +};
> +module_platform_driver(qcom_spi_driver);
> +
> +MODULE_DESCRIPTION("SPI driver for QPIC QSPI cores");
> +MODULE_AUTHOR("Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");

I have sent a patch to fix these issues
https://lore.kernel.org/b63db431cbf35223a4400e44c296293d32c4543c.1742998909.git.geert+renesas@xxxxxxxxx

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds