Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752695AbdDIVqY (ORCPT ); Sun, 9 Apr 2017 17:46:24 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:36308 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751307AbdDIVqR (ORCPT ); Sun, 9 Apr 2017 17:46:17 -0400 Subject: Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4 protocols To: Cyrille Pitchen , Cyrille Pitchen , linux-mtd@lists.infradead.org, jartur@cadence.com, kdasu.kdev@gmail.com, mar.krzeminski@gmail.com References: <3426fd033830e2df15eae27c1b5284983961fa8e.1490220411.git.cyrille.pitchen@atmel.com> <49375a87-5eca-a1c3-26f9-81075dd9c7f9@gmail.com> <10463831-9caf-f9df-ea15-2b77e01bd4b5@gmail.com> Cc: boris.brezillon@free-electrons.com, richard@nod.at, nicolas.ferre@microchip.com, linux-kernel@vger.kernel.org, computersforpeace@gmail.com, dwmw2@infradead.org From: Marek Vasut Message-ID: <87d64978-62f6-1524-41b0-0ef45c4c370b@gmail.com> Date: Sun, 9 Apr 2017 23:46:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4290 Lines: 113 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 >>>>> --- >>>>> 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