Re: [LINUX PATCH v2 1/3] spi: Added dummy_cycle entry in the spi_transfer structure.

[Cyrille Pitchen <cyrille.pitchen@atmel.com>]

Hi Sai Krishna,

Le 13/04/2016 07:21, Lakshmi Sai Krishna Potthuri a écrit :
> Hi Cyrille and Mark,
> 
>> -----Original Message-----
>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>> Sent: Thursday, April 07, 2016 8:33 PM
>> To: Lakshmi Sai Krishna Potthuri <lakshmis@xilinx.com>; Michal Simek
>> <michals@xilinx.com>; Soren Brinkmann <sorenb@xilinx.com>; David
>> Woodhouse <dwmw2@infradead.org>; Brian Norris
>> <computersforpeace@gmail.com>; Mark Brown <broonie@kernel.org>;
>> Javier Martinez Canillas <javier@osg.samsung.com>; Boris Brezillon
>> <boris.brezillon@free-electrons.com>; Stephen Warren
>> <swarren@nvidia.com>; Geert Uytterhoeven <geert+renesas@glider.be>;
>> Andrew F. Davis <afd@ti.com>; Marek Vasut <marex@denx.de>; Jagan Teki
>> <jteki@openedev.com>; Rafał Miłecki <zajec5@gmail.com>
>> Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
>> spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Harini Katakam
>> <harinik@xilinx.com>; Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
>> Anirudha Sarangi <anirudh@xilinx.com>; Lakshmi Sai Krishna Potthuri
>> <lakshmis@xilinx.com>; R, Vignesh <vigneshr@ti.com>
>> Subject: Re: [LINUX PATCH v2 1/3] spi: Added dummy_cycle entry in the
>> spi_transfer structure.
>>
>> Hi all,
>>
>> Le 07/04/2016 16:39, P L Sai Krishna a écrit :
>>> This patch adds dummy_cycles entry in the spi_transfer structure.
>>> len field in the transfer structure contains dummy bytes along with
>>> actual data bytes, controllers which requires dummy bytes use len
>>> field and simply Ignore the dummy_cycles field. Controllers which
>>> expects dummy cycles won't work directly by using len field because
>>> host driver doesn't know that len field of a particular transfer
>>> includes dummy bytes or not (and also number of dummy bytes included
>>> in len field). In such cases host driver use this dummy_cycles field
>>> to identify the number of dummy cycles and based on that it will send
>>> the required number of dummy cycles.
>>
>> Dummy cycles are only used with SPI NOR flashes, aren't they?
> 
> Yes, with SPI NOR flashes.
> 
>> I guess so since there is also a patch dedicated to the m25p80 driver.
>>
>> So why not using the spi_flash_read() API already introduced by Vignesh
>> in the SPI layer?
> 
> ZynqMP GQSPI controller driver use "transfer_one" hook. Since this is generic
> Controller majorly interfaced to flash devices and it might extend to NAND
> flashes in future. This patch does not disturb the existing implementation of
> accessing the flash or other slave devices. It adds an additional optional
> feature. So there is no harm to controllers that don't need dummy cycles -
> they can ignore it and still work with the existing implementation.
> 
> Regards
> Sai Krishna

I understand but you propose to patch both the SPI layer and the m25p80 driver
to introduce some support which is already provided by the "spi_flash_read"
hook: struct spi_flash_read_message has already a "dummy_bytes" field.
IMHO, it looks redundant.

If you really want to use something close to the "transfer_one" hook, I guess
you could build spi_transfer structures from the content of the struct
spi_flash_read_message argument of spi_flash_read() then pass those
spi_transfer structures to your custom "transfer_one_ex":

int transfer_one_ex(struct spi_master *master, struct spi_device *spi,
		    struct spi_transfer *transfer, u32 dummy_cycles);

You just need to set dummy_cycles to 0 to implement the regular "transfer_one"
hook.

Best regards,

Cyrille

>>
>> struct spi_flash_read_message already includes a 'dummy_bytes' member.
>>
>>>
>>> Signed-off-by: P L Sai Krishna <lakshmis@xilinx.com>
>>> ---
>>> v2:
>>>  - Changed the structure member name from dummy to dummy_cycles.
>>>  - Updated the documentation of dummy_cycles.
>>>  - m25p80 changes split into another patch.
>>>
>>>  include/linux/spi/spi.h | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>>> index 857a9a1..63135b3 100644
>>> --- a/include/linux/spi/spi.h
>>> +++ b/include/linux/spi/spi.h
>>> @@ -664,6 +664,9 @@ extern void spi_res_release(struct spi_master
>> *master,
>>>   * @len: size of rx and tx buffers (in bytes)
>>>   * @speed_hz: Select a speed other than the device default for this
>>>   *      transfer. If 0 the default (from @spi_device) is used.
>>> + * @dummy_cycles: number of dummy cycles. If host controller requires
>>> + *  dummy cycles rather than dummy bytes which send along with Cmd
>>> + *  and address then this dummy_cycles is used.
>>>   * @bits_per_word: select a bits_per_word other than the device default
>>>   *      for this transfer. If 0 the default (from @spi_device) is used.
>>>   * @cs_change: affects chipselect after this transfer completes
>>> @@ -752,6 +755,7 @@ struct spi_transfer {
>>>      u8              bits_per_word;
>>>      u16             delay_usecs;
>>>      u32             speed_hz;
>>> +    u32             dummy_cycles;
>>>
>>>      struct list_head transfer_list;
>>>  };
>>>
>>
>> Best regards,
>>
>> Cyrille
> 
> 
> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
>