Re: [PATCH v4 2/2] mtd: spi-nor: use spi-mem dirmap API

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

Hi, Sergei,

Looks good. Few nits below that I can fix when applying. Let me know if 
you're ok with the changes.

On Monday, January 27, 2020 10:29:06 PM EET Sergei Shtylyov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Make use of the spi-mem direct mapping API to let advanced controllers
> optimize read/write operations when they support direct mapping.
> 
> Based on the original patch by Boris Brezillon
> <boris.brezillon@xxxxxxxxxxx>.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> 
> ---
> Changes in version 4:
> - moved the spi_mem_dirmap_{read|write}() calls closer to the ending of
>   spi_nor_{read|write}(), adapting to the chnages caused by the new patch
>   splitting spi_nor_spimem_xfer_data()...
> 
> Changes in version 3:
> - simplified the way spi_mem_dirmap_{read|write}() are called;
> - added Boris' tag.
> 
> Changes in version 2:
> - moved the spi_mem_dirmap_{read|write}() calls from spi_nor_{read|write}()
> to spi_nor_spimem_{read|write}_data().
> 
>  drivers/mtd/spi-nor/spi-nor.c |   97
> ++++++++++++++++++++++++++++++++++++++---- include/linux/mtd/spi-nor.h   | 
>   5 ++
>  2 files changed, 93 insertions(+), 9 deletions(-)
> 
> Index: linux/drivers/mtd/spi-nor/spi-nor.c
> ===================================================================
> --- linux.orig/drivers/mtd/spi-nor/spi-nor.c
> +++ linux/drivers/mtd/spi-nor/spi-nor.c
> @@ -306,6 +306,7 @@ static ssize_t spi_nor_spimem_read_data(
>                            SPI_MEM_OP_DUMMY(nor->read_dummy, 1),
>                            SPI_MEM_OP_DATA_IN(len, buf, 1));
>         bool usebouncebuf;
> +       ssize_t nbytes;
>         int error;
> 
>         /* get transfer protocols. */
> @@ -319,14 +320,23 @@ static ssize_t spi_nor_spimem_read_data(
> 
>         usebouncebuf = spi_nor_spimem_bounce(nor, &op);
> 
> -       error = spi_nor_spimem_exec_op(nor, &op);
> -       if (error)
> -               return error;
> +       if (nor->dirmap.rdesc) {
> +               nbytes = spi_mem_dirmap_read(nor->dirmap.rdesc, op.addr.val,

op.data.nbytes = spi_mem_dirmap_read() ?

This way we can get rid of the local variable nbytes.

> +                                            op.data.nbytes,
> op.data.buf.in); +               if (nbytes < 0)
> +                       return nbytes;
> +       } else {
> +               error = spi_nor_spimem_exec_op(nor, &op);
> +               if (error)
> +                       return error;
> +
> +               nbytes = op.data.nbytes;
> +       }
> 
>         if (usebouncebuf)
> -               memcpy(buf, op.data.buf.in, op.data.nbytes);
> +               memcpy(buf, op.data.buf.in, nbytes);
> 
> -       return op.data.nbytes;
> +       return nbytes;
>  }
> 
>  /**
> @@ -366,6 +376,7 @@ static ssize_t spi_nor_spimem_write_data
>                            SPI_MEM_OP_NO_DUMMY,
>                            SPI_MEM_OP_DATA_OUT(len, buf, 1));
>         bool usebouncebuf;
> +       ssize_t nbytes;
>         int error;
> 
>         op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto);
> @@ -379,11 +390,19 @@ static ssize_t spi_nor_spimem_write_data
>         if (usebouncebuf)
>                 memcpy(nor->bouncebuf, buf, op.data.nbytes);
> 
> -       error = spi_nor_spimem_exec_op(nor, &op);
> -       if (error)
> -               return error;
> +       if (nor->dirmap.wdesc) {
> +               nbytes = spi_mem_dirmap_write(nor->dirmap.wdesc,
> op.addr.val, +                                          op.data.nbytes,

I'll align this to "("

op.data.nbytes = spi_mem_dirmap_write() ?

This way we can get rid of the local variable nbytes.

> op.data.buf.out); +               if (nbytes < 0)
> +                       return nbytes;

you already return nbytes below, we can drop this check.

> +       } else {
> +               error = spi_nor_spimem_exec_op(nor, &op);
> +               if (error)
> +                       return error;
> +               nbytes = op.data.nbytes;
> +       }
> 
> -       return op.data.nbytes;
> +       return nbytes;
>  }

Cheers,
ta


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/