From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752114AbcFNLUn (ORCPT ); Tue, 14 Jun 2016 07:20:43 -0400 Received: from mail-it0-f67.google.com ([209.85.214.67]:33868 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751914AbcFNLUd (ORCPT ); Tue, 14 Jun 2016 07:20:33 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Julian Calaby Date: Tue, 14 Jun 2016 21:20:12 +1000 Message-ID: Subject: Re: [linux-sunxi] [PATCH v3 10/13] spi: sunxi: merge sun4i and sun6i SPI driver To: Michal Suchanek Cc: linux-sunxi , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Maxime Ripard , Chen-Yu Tsai , Russell King , Mark Brown , Arnd Bergmann , Olof Johansson , Krzysztof Kozlowski , Javier Martinez Canillas , Simon Horman , Sjoerd Simons , Thierry Reding , Alison Wang , Timo Sigurdsson , Jonathan Liu , Gerhard Bertelsmann , Priit Laes , devicetree , "Mailing List, Arm" , "linux-kernel@vger.kernel.org" , linux-spi Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Michal, On Tue, Jun 14, 2016 at 4:35 PM, Michal Suchanek wrote: > Hello, > > On 14 June 2016 at 07:45, Julian Calaby wrote: >> Hi Michal, >> >> On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek wrote: >>> On 14 June 2016 at 06:47, Julian Calaby wrote: >>>> Hi Michal, >>>> >>>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek wrote: >>>>> Hello, >>>>> >>>>> On 14 June 2016 at 01:43, Julian Calaby wrote: >>>>>> Hi Michal, >>>>>> >>>>>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek wrote: >>>>>>> The drivers are very similar and share multiple flaws which needed >>>>>>> separate fixes for both drivers. >>>>>>> >>>>>>> Signed-off-by: Michal Suchanek >>>>>>> --- >>>>>>> drivers/spi/Kconfig | 8 +- >>>>>>> drivers/spi/Makefile | 1 - >>>>>>> drivers/spi/spi-sun4i.c | 156 +++++++++++-- >>>>>>> drivers/spi/spi-sun6i.c | 598 ------------------------------------------------ >>>>>>> 4 files changed, 143 insertions(+), 620 deletions(-) >>>>>>> delete mode 100644 drivers/spi/spi-sun6i.c >>>>>>> >>>>>>> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c >>>>>>> index 0b8e6c6..c76f8e4 100644 >>>>>>> --- a/drivers/spi/spi-sun4i.c >>>>>>> +++ b/drivers/spi/spi-sun4i.c >>>>>>> @@ -279,9 +321,14 @@ static int sunxi_spi_transfer_one(struct spi_master *master, >>>>>>> reg = sunxi_spi_read(sspi, SUNXI_TFR_CTL_REG); >>>>>>> >>>>>>> /* Reset FIFOs */ >>>>>>> - sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, >>>>>>> - reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) | >>>>>>> - sspi_bits(sspi, SUNXI_CTL_TF_RST)); >>>>>>> + if (sspi->type == SPI_SUN4I) >>>>>>> + sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, >>>>>>> + reg | sspi_bits(sspi, SUNXI_CTL_RF_RST) | >>>>>>> + sspi_bits(sspi, SUNXI_CTL_TF_RST)); >>>>>>> + else >>>>>>> + sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG, >>>>>>> + sspi_bits(sspi, SUNXI_CTL_RF_RST) | >>>>>>> + sspi_bits(sspi, SUNXI_CTL_TF_RST)); >>>>>> >>>>>> If we're already doing different stuff for each generation of the IP, >>>>>> why not just use the register offsets and bit definitions directly? >>>>> >>>>> Because having (*sspi->regmap)[SUNXI_FIFO_CTL_REG] all over the place >>>>> makes my eyes bleed and you cannot use the check that you are >>>>> accessing a register that actually exists. >>>> >>>> I mean removing SUNXI_CTL_RF_RST and SUNXI_CTL_TF_RST from all of the >>>> indirection you added and using them directly, i.e. >>>> >>>> #define SUN4I_TFR_CTL_RF_RST BIT(x) >>>> #define SUN4I_TFR_CTL_TF_RST BIT(x) >>>> #define SUN6I_FIFO_CTL_RF_RST BIT(x) >>>> #define SUN6I_FIFO_CTL_TF_RST BIT(x) >>>> >>>> then >>>> >>>> if (sspi->type == SPI_SUN4I) >>>> sunxi_spi_write(sspi, SUNXI_TFR_CTL_REG, reg | >>>> SUN4I_TFR_CTL_RF_RST | SUN4I_TFR_CTL_TF_RST); >>>> else >>>> sunxi_spi_write(sspi, SUNXI_FIFO_CTL_REG, reg | >>>> SUN6I_FIFO_CTL_RF_RST | SUN6I_FIFO_CTL_TF_RST); >>>> >>>> I.e. the bits that need setting are in different registers, so you >>>> have to have an if statement and separate calls. Therefore there's no >>>> real benefit from the indirection you've introduced here, unless >>>> you're expecting the SUN8I variant to use different bits in one of >>>> those two registers. >>>> >>> >>> That looks nice for this particular case. >> >> There was another case I pointed out in this driver that could >> potentially benefit from this. >> >>> Still you will have to remember that these bits are specified directly >>> while other bits on the register are mapped which is not so nice. >> >> True. I'm not sure which path is best in this case. To my eye, your >> indirection scheme seems like the "heavy" solution, however I have no >> idea what form a "lighter" solution would take. >> >> Other drivers I've seen which have tackled similar problems have used >> a large struct to hold all the variant-specific stuff, whether that's >> function pointers, register offsets, constants, etc. (so this code >> could theoretically be re-written as sunxi_spi_write(sspi, >> sspi->fifo_reg, reg | sspi->fifo_reset_arg) > > This won't do. There is fifo_reg and tfr_reg on both IPs but some bits > from one migrated to the other. > >> or sspi->reset_fifo()) > > This would work but would require more thorough rewrite and heavier > adaptation layer. > > As it is where different register is used in different IP revision it > is specified explicitly in the code while register names to numbers > are mapped in read() and write(). Value bits are mapped by explicitly > calling a function on the name. > > This gives nice 1:1 mapping to the old code and allows checking that > both the register and the bit value in question exist on the IP. And your method is almost beautiful from that perspective, however I still feel that it's too "heavy". That said, neither of my suggestions are much better. I provided them in the hope that they might be illuminating. > Matching the value to register relies on the driver code, however. Of course. I'm not sure what the state-of-the-art for dealing with variants of devices where the manufacturer keeps scrambling the registers is, however I feel we can do better, though I'm not sure how. Thanks, -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/