Hi Bayi,

A few small comments.

On Fri, Nov 06, 2015 at 11:48:08PM +0800, Bayi Cheng wrote:

add spi nor flash driver for mediatek controller

Signed-off-by: Bayi Cheng <redacted>
---
 drivers/mtd/spi-nor/Kconfig       |   7 +
 drivers/mtd/spi-nor/Makefile      |   1 +
 drivers/mtd/spi-nor/mtk-quadspi.c | 475 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 483 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/mtk-quadspi.c

...

+/* Can shift up to 48 bits (6 bytes) of TX/RX */
+#define MTK_NOR_MAX_SHIFT		6

...

+static int mt8173_nor_do_tx_rx(struct mt8173_nor *mt8173_nor, u8 op,
+			       u8 *tx, int txlen, u8 *rx, int rxlen)
+{
+	int len = 1 + txlen + rxlen;
+	int i, ret, idx;
+
+	if (len > MTK_NOR_MAX_SHIFT + 1)
+		return -EINVAL;

So I see you adjusted these bounds to add 1, which inspired one of my
questions on the cover letter.

Why do you allow len=7, which means you'd program 7*8 to the COUNT
register, when the SoC manual says it has a max of 48? Is the manual
wrong?

I notice you added the '+ 1' to your driver, so it allows:

	do_tx_rx( txlen = 0 , rxlen = 6) /* e.g., for READ ID */

but that means your driver also allows:

	do_tx_rx( txlen = 6, rxlen = 0) /* ERROR: this will allow out of
					   bounds write on PRGDATA
					   register -1 */

If I understand correctly, the following constraints are more correct:

	/* Can only transmit 1 opcode and 5 other bytes */
	if (1 + txlen > MTK_NOR_MAX_SHIFT)
		return -EINVAL;

	/* Can only receive 6 bytes after the opcode */
	if (rxlen > MTK_NOR_MAX_SHIFT)
		return -EINVAL;

	/* Can only handle XXX bytes total */
	/* How many bytes is the max for register MTK_NOR_CNT_REG ? */
	if (len > XXX)
		return -EINVAL;

+
+	writeb(len * 8, mt8173_nor->base + MTK_NOR_CNT_REG);
+
+	/* start at PRGDATA5, go down to PRGDATA0 */
+	idx = MTK_NOR_MAX_SHIFT - 1;
+
+	/* opcode */
+	writeb(op, mt8173_nor->base + MTK_NOR_PRG_REG(idx));
+	idx--;
+
+	/* program TX data */
+	for (i = 0; i < txlen; i++, idx--)
+		writeb(tx[i], mt8173_nor->base + MTK_NOR_PRG_REG(idx));
+
+	/* clear out rest of TX registers */
+	while (idx >= 0) {
+		writeb(0, mt8173_nor->base + MTK_NOR_PRG_REG(idx));
+		idx--;
+	}
+
+	ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PRG_CMD);
+	if (ret)
+		return ret;
+
+	/* restart at first RX byte */
+	idx = rxlen - 1;
+
+	/* read out RX data */
+	for (i = 0; i < rxlen; i++, idx--)
+		rx[i] = readb(mt8173_nor->base + MTK_NOR_SHREG(idx));
+
+	return 0;
+}
+

...

+static int __init mtk_nor_init(struct mt8173_nor *mt8173_nor,
+			       struct device_node *flash_node)
+{
+	struct mtd_part_parser_data ppdata = {
+		.of_node = flash_node,
+	};
+	int ret;
+	struct spi_nor *nor;

Normally we'd have a blank line here.

+	/* initialize controller to accept commands */
+	writel(MTK_NOR_ENABLE_SF_CMD, mt8173_nor->base + MTK_NOR_WRPROT_REG);
+
+	nor = &mt8173_nor->nor;
+	nor->dev = mt8173_nor->dev;
+	nor->priv = mt8173_nor;
+	nor->flash_node = flash_node;
+
+	/* fill the hooks to spi nor */
+	nor->read = mt8173_nor_read;
+	nor->read_reg = mt8173_nor_read_reg;
+	nor->write = mt8173_nor_write;
+	nor->write_reg = mt8173_nor_write_reg;
+	nor->mtd.owner = THIS_MODULE;
+	nor->mtd.name = "mtk_nor";
+	/* initialized with NULL */
+	ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
+	if (ret)
+		return ret;
+
+	return mtd_device_parse_register(&nor->mtd, NULL, &ppdata, NULL, 0);
+}

...

Brian