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

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

On 02/17/2020 02:03 AM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:

> 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
[...]
>> @@ -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() ?

   op.data.nbytes is *unsigned int*, spi_mem_dirmap_read() returns ssize_t.

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

   op.data.nbytes can't carry the error codes, not even supposed to be of a signed
type...

[...]
>> @@ -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 "("

   Sorry about missing that. Copy&paste from the read function played its role here...

> op.data.nbytes = spi_mem_dirmap_write() ?

   Same comments as in the read function...

> 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.

   Yeah, sorry about that! We've already copied from the bounce buffer

>> +       } 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

MBR, Sergei

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