Re: [PATCH v2 5/7] mtd: devices: m25p80: Use the spi-mem dirmap API

[Boris Brezillon <boris.brezillon@xxxxxxxxxxx>]

On Wed, 31 Oct 2018 11:49:51 +0100
Boris Brezillon <boris.brezillon@xxxxxxxxxxx> wrote:

> On Wed, 31 Oct 2018 10:28:04 +0000
> Yogesh Narayan Gaur <yogeshnarayan.gaur@xxxxxxx> wrote:
> 
> 
> > > +static int m25p_create_read_dirmap(struct m25p *flash) {
> > > +	struct spi_nor *nor = &flash->spi_nor;
> > > +	struct spi_mem_dirmap_info info = {
> > > +		.op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor-    
> > > >read_opcode, 1),    
> > > +				      SPI_MEM_OP_ADDR(nor->addr_width, 0, 1),
> > > +				      SPI_MEM_OP_DUMMY(nor->read_dummy,
> > > 1),
> > > +				      SPI_MEM_OP_DATA_IN(0, NULL, 1)),
> > > +		.offset = 0,
> > > +		.length = flash->spi_nor.mtd.size,    
> > 
> > I want to understand the sequence of calling up of this routine from drivers/mtd/spi-nor/spi-nor.c file.
> > Because till point of JedecID read and SFDP command read mtd.size is not being populated and having value as 0. This would make length as 0.  
> 
> When the dirmap is created mtd->size has been initialized already. But
> you're right, nor->read() might be called before the read dirmap has
> been created (SFDP read), which will lead to a NULL pointer exception.

The following version should fix the problem:

--->8---
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index c4a1d04b8c80..847188e8a99e 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -31,8 +31,70 @@
 struct m25p {
        struct spi_mem          *spimem;
        struct spi_nor          spi_nor;
+       struct {
+               struct spi_mem_dirmap_desc *rdesc;
+               struct spi_mem_dirmap_desc *wdesc;
+       } dirmap;
 };
 
+static int m25p_create_write_dirmap(struct m25p *flash)
+{
+       struct spi_nor *nor = &flash->spi_nor;
+       struct spi_mem_dirmap_info info = {
+               .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->program_opcode, 1),
+                                     SPI_MEM_OP_ADDR(nor->addr_width, 0, 1),
+                                     SPI_MEM_OP_NO_DUMMY,
+                                     SPI_MEM_OP_DATA_OUT(0, NULL, 1)),
+               .offset = 0,
+               .length = flash->spi_nor.mtd.size,
+       };
+       struct spi_mem_op *op = &info.op_tmpl;
+
+       /* get transfer protocols. */
+       op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto);
+       op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto);
+       op->dummy.buswidth = op->addr.buswidth;
+       op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto);
+
+       if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
+               op->addr.nbytes = 0;
+
+       flash->dirmap.wdesc = spi_mem_dirmap_create(flash->spimem, &info);
+       if (IS_ERR(flash->dirmap.wdesc))
+               return PTR_ERR(flash->dirmap.wdesc);
+
+       return 0;
+}
+
+static int m25p_create_read_dirmap(struct m25p *flash)
+{
+       struct spi_nor *nor = &flash->spi_nor;
+       struct spi_mem_dirmap_info info = {
+               .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1),
+                                     SPI_MEM_OP_ADDR(nor->addr_width, 0, 1),
+                                     SPI_MEM_OP_DUMMY(nor->read_dummy, 1),
+                                     SPI_MEM_OP_DATA_IN(0, NULL, 1)),
+               .offset = 0,
+               .length = flash->spi_nor.mtd.size,
+       };
+       struct spi_mem_op *op = &info.op_tmpl;
+
+       /* get transfer protocols. */
+       op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto);
+       op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
+       op->dummy.buswidth = op->addr.buswidth;
+       op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
+
+       /* convert the dummy cycles to the number of bytes */
+       op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8;
+
+       flash->dirmap.rdesc = spi_mem_dirmap_create(flash->spimem, &info);
+       if (IS_ERR(flash->dirmap.rdesc))
+               return PTR_ERR(flash->dirmap.rdesc);
+
+       return 0;
+}
+
 static int m25p80_read_reg(struct spi_nor *nor, u8 code, u8 *val, int len)
 {
        struct m25p *flash = nor->priv;
@@ -92,6 +154,9 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
                                   SPI_MEM_OP_DATA_OUT(len, buf, 1));
        int ret;
 
+       if (flash->dirmap.wdesc)
+               return spi_mem_dirmap_write(flash->dirmap.wdesc, to, len, buf);
+
        /* get transfer protocols. */
        op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto);
        op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto);
@@ -128,6 +193,9 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
        size_t remaining = len;
        int ret;
 
+       if (flash->dirmap.rdesc)
+               return spi_mem_dirmap_read(flash->dirmap.rdesc, from, len, buf);
+
        /* get transfer protocols. */
        op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto);
        op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
@@ -231,19 +299,47 @@ static int m25p_probe(struct spi_mem *spimem)
        if (ret)
                return ret;
 
-       return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
-                                  data ? data->nr_parts : 0);
+       ret = m25p_create_write_dirmap(flash);
+       if (ret)
+               return ret;
+
+       ret = m25p_create_read_dirmap(flash);
+       if (ret)
+               goto err_destroy_write_dirmap;
+
+       ret = mtd_device_register(&nor->mtd, data ? data->parts : NULL,
+                                 data ? data->nr_parts : 0);
+       if (ret)
+               goto err_destroy_read_dirmap;
+
+       return 0;
+
+err_destroy_read_dirmap:
+       spi_mem_dirmap_destroy(flash->dirmap.rdesc);
+
+err_destroy_write_dirmap:
+       spi_mem_dirmap_destroy(flash->dirmap.wdesc);
+
+       return ret;
 }
 
 
 static int m25p_remove(struct spi_mem *spimem)
 {
        struct m25p     *flash = spi_mem_get_drvdata(spimem);
+       int ret;
 
        spi_nor_restore(&flash->spi_nor);
 
        /* Clean up MTD stuff. */
-       return mtd_device_unregister(&flash->spi_nor.mtd);
+       ret = mtd_device_unregister(&flash->spi_nor.mtd);
+       if (ret)
+               return ret;
+
+       spi_mem_dirmap_destroy(flash->dirmap.rdesc);
+       spi_mem_dirmap_destroy(flash->dirmap.wdesc);
+
+       return 0;
 }
 
 static void m25p_shutdown(struct spi_mem *spimem)