Re: [PATCH 0/5] Add support for STM32F4 SPI

[Cezary GapiÅski]

Hi Amelie,

On Mon, 2018-12-10 at 12:37 +0000, Amelie DELAUNAY wrote:
> Hi Cezary,
> 
> On 12/9/18 2:53 PM, cezary.gapinski@xxxxxxxxx wrote:
> > 
> > From: Cezary Gapinski <cezary.gapinski@xxxxxxxxx>
> > 
> > This series of patches adds support for first generation of SPI
> > interface
> > for STM32F4 family.
> > 
> First of all, thanks for adding STM32F4 SPI support.

Thanks for your answer and hints for correct approach to this driver.

> > 
> > This version of driver is mostly different to STM32H7 one. Based on
> > linux
> > kernel I2C drivers for STM32 where drivers were splited into
> > STM32F4 and
> > STM32F7 family the same approach seems to be sufficient for SPI
> > STM32
> > drivers. Therefore STM32H7 driver was moved to spi-stm32h7.c file
> > and
> > register and functions were renamed to be more specific to STM32H7.
> > 
> You're right, STM32F4 SPI is slightly different from STM32H7 one:Â
> register map/bits offsets are different and STM32H7 has an RX and TX
> FIFO.
> But if you have a look on STM32F7 SPI [1], you'll see that STM32F7
> SPIÂ
> is based on STM32F4 SPI with new features (data frames & FIFOs) alsoÂ
> available on STM32H7 SPI.
> 
> That's why STM32H7 SPI driver was called spi-stm32. The goal was to
> useÂ
> compatible & match data to differentiate each STM32Fx specificities.
> 
> You can have a look on how it is managed in drivers/rtc/rtc-stm32.c
> (theÂ
> same driver covers 2 HW version of STM32 RTC), or inÂ
> drivers/iio/dac/stm32-dac-core.c and stm32-dac.c (the same driver
> alsoÂ
> covers 2 HW version of STM32 DAC).
> As your spi-stm32f4.c file is highly based on the existing spi-
> stm32.cÂ
> file, I think that common code could be factored and specificities
> couldÂ
> be handled with compatible and match data.

I have seen these drivers before. I have been trying to do this for my
previous approach and it is still on my second branch. There was not
too much common parts and I have stuck with complex compatible data
configurations. I thought that was too verbose, therefore I resigned
for it and have gone into idea with two totally different files. I
think I need to get down again to idea you proposed.Â

It seems to be more difficult approach and it is gonna take a while
before I send second version of these patches.

Regards,
Cezary

> Regards,
> Amelie
> [1]Â
> https://www.st.com/content/ccc/resource/technical/document/reference_
> manual/c5/cf/ef/52/c0/f1/4b/fa/DM00124865.pdf/files/DM00124865.pdf/jc
> r:content/translations/en.DM00124865.pdf
> 
> > 
> > For current version master mode with full-duplex and 8/16 bit data
> > frame
> > format are supported. There is no TX and RX FIFOs like in STM32H7.
> > DMA capabilility is supported for messages longer than arbitrary
> > number
> > of bytes (that is set already to 16 bytes) when TX and RX channels
> > are
> > set at the same time.
> > 
> > Cezary Gapinski (5):
> > ÂÂÂspi: stm32: rename STM32 SPI registers and functions to STM32H7
> > ÂÂÂspi: stm32: rename spi-stm32 to spi-stm32h7
> > ÂÂÂspi: stm32: add driver for STM32F4 controller
> > ÂÂÂARM: dts: stm32: add SPI support on STM32F429 SoC
> > ÂÂÂspi: stm32: add description about STM32F4 bindings
> > 
> > Â .../devicetree/bindings/spi/spi-stm32.txtÂÂÂÂÂÂÂÂÂÂ|ÂÂÂÂ9 +-
> > Â arch/arm/boot/dts/stm32f429.dtsiÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂ60 +
> > Â drivers/spi/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂ18 +-
> > Â drivers/spi/MakefileÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂÂ3 +-
> > Â drivers/spi/spi-stm32.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ| 1322 -----
> > --------------
> > Â drivers/spi/spi-stm32f4.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ| 1002
> > +++++++++++++++
> > Â drivers/spi/spi-stm32h7.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ| 1340
> > ++++++++++++++++++++
> > Â 7 files changed, 2424 insertions(+), 1330 deletions(-)
> > Â delete mode 100644 drivers/spi/spi-stm32.c
> > Â create mode 100644 drivers/spi/spi-stm32f4.c
> > Â create mode 100644 drivers/spi/spi-stm32h7.c