From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753480AbbKLEeA (ORCPT ); Wed, 11 Nov 2015 23:34:00 -0500 Received: from bear.ext.ti.com ([192.94.94.41]:41005 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753072AbbKLEd5 (ORCPT ); Wed, 11 Nov 2015 23:33:57 -0500 Subject: Re: [PATCH v3 1/5] spi: introduce mmap read support for spi flash devices To: Brian Norris References: <1447133399-25658-1-git-send-email-vigneshr@ti.com> <1447133399-25658-2-git-send-email-vigneshr@ti.com> <20151111192441.GG12143@google.com> CC: Mark Brown , Tony Lindgren , Rob Herring , Michal Suchanek , Russell King , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-spi@vger.kernel.org" , "linux-mtd@lists.infradead.org" , "linux-omap@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Marek Vasut From: Vignesh R Message-ID: <56441678.6040504@ti.com> Date: Thu, 12 Nov 2015 10:02:56 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151111192441.GG12143@google.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Brian, On 11/12/2015 12:54 AM, Brian Norris wrote: > In addition to my other comments: > [...] >> + int (*spi_mtd_mmap_read)(struct spi_device *spi, >> + loff_t from, size_t len, >> + size_t *retlen, u_char *buf, >> + u8 read_opcode, u8 addr_width, >> + u8 dummy_bytes); > > This is seeming to be a longer and longer list of arguments. I know MTD > has a bad habit of long argument lists (which then cause a ton of > unnecessary churn when things need changed in the API), but perhaps we > can limit the damage to the SPI layer. Perhaps this deserves a struct to > encapsulate all the flash read arguments? Like: > > struct spi_flash_read_message { > loff_t from; > size_t len; > size_t *retlen; > void *buf; > u8 read_opcode; > u8 addr_width; > u8 dummy_bits; > // additional fields to describe rx_nbits for opcode/addr/data > }; > > struct spi_master { > ... > int (*spi_flash_read)(struct spi_device *spi, > struct spi_flash_message *msg); > }; Yeah.. I think struct encapsulation helps, this can also be used to pass sg lists for dma in future. I will rework the series with your suggestion to include nbits for opcode/addr/data. Also, will add validation logic (similar to __spi_validate()) to check whether master supports dual/quad mode for opcode/addr/data. I am planning to add this validation code to spi_flash_read_validate(in place of spi_mmap_read_supported()) Thanks! -- Regards Vignesh