Hi Boris
On 01/08/19 11:16 AM, Boris Brezillon wrote:
> On Thu, 1 Aug 2019 10:00:50 +0530
> Vignesh Raghavendra <vigne...@ti.com> wrote:
>
>> spi-mem layer expects all buffers passed to it to be DMA'able. But
>> spi-nor layer mostly allocates buffers on stack for reading/writing to
>> registers and therefore are not DMA'able. Introduce bounce buffer to be
>> used to read/write to registers. This ensures that buffer passed to
>> spi-mem layer during register read/writes is DMA'able. With this change
>> nor->cmd-buf is no longer used, so drop it.
>>
>> Signed-off-by: Vignesh Raghavendra <vigne...@ti.com>
>> ---
>> v3: new patch
>>
[...]
>> @@ -2180,11 +2179,13 @@ static const struct flash_info
>> *spi_nor_read_id(struct spi_nor *nor)
>> u8 id[SPI_NOR_MAX_ID_LEN];
>> const struct flash_info *info;
>>
>> - tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN);
>> + tmp = nor->read_reg(nor, SPINOR_OP_RDID, nor->bouncebuf,
>> + SPI_NOR_MAX_ID_LEN);
>> if (tmp < 0) {
>> dev_err(nor->dev, "error %d reading JEDEC ID\n", tmp);
>> return ERR_PTR(tmp);
>> }
>> + memcpy(id, nor->bouncebuf, SPI_NOR_MAX_ID_LEN);
>
> Why not directly including the change you have in patch 2 (id is a
> pointer that points directly to ->bouncebuf) so you can get rid of this
> memcpy() here?
>
Ok will do that in next version.
>>
>> for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>> info = &spi_nor_ids[tmp];
>> @@ -4121,6 +4122,16 @@ int spi_nor_scan(struct spi_nor *nor, const char
>> *name,
>> nor->read_proto = SNOR_PROTO_1_1_1;
>> nor->write_proto = SNOR_PROTO_1_1_1;
>>
>> + /*
>> + * We need the bounce buffer early to read/write registers when going
>> + * through the spi-mem layer (buffers have to be DMA-able).
>
> You should probably extend this comment in patch 2 to explain why 4k
> should be enough for regular read/write operations.
>
Will update
> The patch looks good otherwise.
>
> Reviewed-by: Boris Brezillon <boris.brezil...@collabora.com>
>
Thanks for the review!
--
Regards
Vignesh
