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.pitc...@atmel.com>
>>> ---
>>> 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)) */
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);
>>> +}
>>> +
[...]
--
Best regards,
Marek Vasut
