Re: [PATCH 5/5] mtd: spi-nor: Add Synopsys DesignWare Octal SPI driver

[Sascha Hauer <sha@pengutronix.de>]

Hi Jules,

On Mon, Jul 17, 2023 at 06:54:28PM +0200, Jules Maselbas wrote:
> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> ---
>  drivers/mtd/spi-nor/Kconfig       |   6 +
>  drivers/mtd/spi-nor/Makefile      |   1 +
>  drivers/mtd/spi-nor/dw-ospi-nor.c | 960 ++++++++++++++++++++++++++++++
>  3 files changed, 967 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/dw-ospi-nor.c
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 2beb26006e..b34c69203e 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -26,4 +26,10 @@ config SPI_CADENCE_QUADSPI
>  	help
>  	  This enables support for the Cadence Quad SPI controller and NOR flash.
>  
> +config SPI_SYNOPSYS_OCTALSPI_NOR
> +	tristate "Synopsys DesignWare Octal SPI controller"
> +	help
> +	  This enables support for the Synopsys DesignWare Octal SPI controller
> +	  and NOR flash.
> +
>  endif
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 457a2f0488..61cf789182 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_MTD_SPI_NOR)		+= spi-nor.o
>  obj-$(CONFIG_SPI_CADENCE_QUADSPI)	+= cadence-quadspi.o
> +obj-$(CONFIG_SPI_SYNOPSYS_OCTALSPI_NOR) += dw-ospi-nor.o
> diff --git a/drivers/mtd/spi-nor/dw-ospi-nor.c b/drivers/mtd/spi-nor/dw-ospi-nor.c
> new file mode 100644
> index 0000000000..13033f9500
> --- /dev/null
> +static int dw_spi_find_chipselect(struct spi_nor *nor)
> +{
> +	int cs = -1;
> +	struct dw_spi_nor *dw_spi = nor->priv;

Better put a pointer to dw_spi_flash_pdata into nor->priv...
(or use container_of() to get dw_spi_flash_pdata from *nor)

> +
> +	for (cs = 0; cs < dw_spi->supported_cs; cs++)
> +		if (nor == &dw_spi->f_pdata[cs].nor)
> +			break;
> +	return cs;
> +}

...and put cs into struct dw_spi_flash_pdata. This way you don't have to
iterate to get the current chip select.

> +static int dw_spi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> +{
> +	int i, ret;
> +
> +	dev_dbg(nor->dev, "write_reg opcode 0x%02x: ", opcode);
> +	for (i = 0; i < len; i++)
> +		pr_debug("%02x ", buf[i]);
> +	pr_debug("\n");
> +
> +	if (!IS_ENABLED(CONFIG_MTD_WRITE))
> +		return -ENOTSUPP;

This function looks like it's supposed to write a register. Are you sure
this is not needed when MTD write support is disabled?

> +
> +	ret = dw_spi_prep_std(nor, SPI_TMOD_TO);
> +	if (ret)
> +		return ret;
> +
> +	return dw_spi_write_std(nor, &opcode, 1, buf, len);
> +}

[...]

> +
> +	if (!dev->of_node) {
> +		f_pdata = &dw_spi->f_pdata[0];
> +
> +		ret = dw_spi_setup_flash(dev, f_pdata, np);
> +		if (ret)
> +			goto probe_failed;

Do we need probing without device tree? If not, please remove this case.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |