Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4 protocols

[Cyrille Pitchen]

Le 09/04/2017 à 22:46, Marek Vasut a écrit :
> On 04/09/2017 09:37 PM, Cyrille Pitchen wrote:
>> Hi Marek,
>>
>> Le 07/04/2017 à 01:37, Marek Vasut a écrit :
>>> On 03/23/2017 12:33 AM, Cyrille Pitchen wrote:
>>>> Before this patch, m25p80_read() supported few SPI protocols:
>>>> - regular SPI 1-1-1
>>>> - SPI Dual Output 1-1-2
>>>> - SPI Quad Output 1-1-4
>>>> On the other hand, m25p80_write() only supported SPI 1-1-1.
>>>>
>>>> This patch updates both m25p80_read() and m25p80_write() functions to let
>>>> them support SPI 1-2-2 and SPI 1-4-4 protocols for Fast Read and Page
>>>> Program SPI commands.
>>>>
>>>> It adopts a conservative approach to avoid regressions. Hence the new
>>>                                              ^ FYI, regression != bug
>>>
>>>> implementations try to be as close as possible to the old implementations,
>>>> so the main differences are:
>>>> - the tx_nbits values now being set properly for the spi_transfer
>>>>   structures carrying the (op code + address/dummy) bytes
>>>> - and the spi_transfer structure being split into 2 spi_transfer
>>>>   structures when the numbers of I/O lines are different for op code and
>>>>   for address/dummy byte transfers on the SPI bus.
>>>>
>>>> Besides, the current spi-nor framework supports neither the SPI 2-2-2 nor
>>>> the SPI 4-4-4 protocols. So, for now, we don't need to update the
>>>> m25p80_{read|write}_reg() functions as SPI 1-1-1 is the only one possible
>>>> protocol.
>>>>
>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>
>>>> ---
>>>>  drivers/mtd/devices/m25p80.c | 120 ++++++++++++++++++++++++++++++++-----------
>>>>  1 file changed, 90 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>>> index 68986a26c8fe..64d562efc25d 100644
>>>> --- a/drivers/mtd/devices/m25p80.c
>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>> @@ -34,6 +34,19 @@ struct m25p {
>>>>  	u8			command[MAX_CMD_SIZE];
>>>>  };
>>>>  
>>>> +static inline void m25p80_proto2nbits(enum spi_nor_protocol proto,
>>>> +				      unsigned int *inst_nbits,
>>>> +				      unsigned int *addr_nbits,
>>>> +				      unsigned int *data_nbits)
>>>> +{
>>>
>>> Why don't we just have some generic macros to extract the number of bits
>>> from $proto ?
>>>
>>
>> from Documentation/process/coding-style.rst:
>> "Generally, inline functions are preferable to macros resembling functions."
>>
>> inline functions provide better type checking of their arguments and/or
>> returned value than macros.
>>
>> Type checking is also the reason I have chosen to create the 'enum
>> spi_nor_protocol' rather than using constant macros.
> 
> That part I get (no, not really [1], inline is compiler _hint_ and for
> static function, the compiler is smart enough to figure out it should
> inline it, so drop it. Also cf. __always_inline).
> 
> What I don't quite get is why don't we just encode the proto as ie.
> 
> #define PROTO_1_1_4 0x00010204 /* (== BIT(16) | BIT(8) | BIT(2)) */
>

This is what I did in former versions of the patch: the scheme you
propose requires more bits to encode the number of I/O lines for
instruction, address and data: there would be less bits available for
future extensions.

Also using the notion of protocol class (1-1-N, 1-N-N, N-N-N) in the
encoding scheme prevents from setting impossible combinations like
4-1-4, 1-2-4, ...


> in which case this whole function would turn into constant-time
> 
> return (proto >> (n * 8)) & 0xff;
> 
> where n is 0 for data, 1 for address , 2 for command .
> 
> [1] https://lwn.net/Articles/166172/
> 
>>>> +	if (inst_nbits)
>>>> +		*inst_nbits = spi_nor_get_protocol_inst_width(proto);
>>>> +	if (addr_nbits)
>>>> +		*addr_nbits = spi_nor_get_protocol_addr_width(proto);
>>>> +	if (data_nbits)
>>>> +		*data_nbits = spi_nor_get_protocol_data_width(proto);
>>>> +}
>>>> +
> [...]
>