From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752719AbdDIVtg (ORCPT ); Sun, 9 Apr 2017 17:49:36 -0400 Received: from 5.mo1.mail-out.ovh.net ([178.33.45.107]:34135 "EHLO 5.mo1.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751307AbdDIVt2 (ORCPT ); Sun, 9 Apr 2017 17:49:28 -0400 Subject: Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4 protocols To: Marek Vasut , 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: Cyrille Pitchen Message-ID: Date: Sun, 9 Apr 2017 23:30:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <10463831-9caf-f9df-ea15-2b77e01bd4b5@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Ovh-Tracer-Id: 12100609251174537061 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrfeeliedruddtgdduieduucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuqfggjfdpvefjgfevmfevgfenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddm Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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); >>>> +} >>>> + > [...] >