Messages in this thread |  | | Subject | Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4 protocols | From | Marek Vasut <> | Date | Sun, 9 Apr 2017 23:46:13 +0200 |
| |
On 04/09/2017 11:30 PM, Cyrille Pitchen wrote: > 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@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)) */ >> > > 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.
Are we ever gonna reach 128bit SPI ? I don't think so. Yes, it requires more bits, but it also makes it easier to extract information from it without some elaborate loops.
> 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, ...
I'd suspect that the review process would catch this.
>> 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
|  |