Hi Michal, On Tue, Jun 14, 2016 at 4:35 PM, Michal Suchanek <hramrach@xxxxxxxxx> wrote: > Hello, > > On 14 June 2016 at 07:45, Julian Calaby <julian.calaby@xxxxxxxxx> wrote: >> Hi Michal, >> >> On Tue, Jun 14, 2016 at 3:28 PM, Michal Suchanek <hramrach@xxxxxxxxx> wrote: >>> On 14 June 2016 at 06:47, Julian Calaby <julian.calaby@xxxxxxxxx> wrote: >>>> Hi Michal, >>>> >>>> On Tue, Jun 14, 2016 at 2:34 PM, Michal Suchanek <hramrach@xxxxxxxxx> wrote: >>>>> Hello, >>>>> >>>>> On 14 June 2016 at 01:43, Julian Calaby <julian.calaby@xxxxxxxxx> wrote: >>>>>> Hi Michal, >>>>>> >>>>>> On Tue, Jun 14, 2016 at 3:46 AM, Michal Suchanek <hramrach@xxxxxxxxx> wrote: >>>>>>> The drivers are very similar and share multiple flaws which needed >>>>>>> separate fixes for both drivers. >>>>>>> >>>>>>> Signed-off-by: Michal Suchanek <hramrach@xxxxxxxxx> >>>>>>> --- >>>>>>> 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@xxxxxxxxx Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
