Re: [PATCH v2 13/19] spi: dw: Move Non-DMA code to the DW PCIe-SPI driver

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, May 15, 2020 at 01:47:52PM +0300, Serge Semin wrote:
> This is a preparation patch before adding the DW DMA support into the
> DW SPI MMIO driver. We need to unpin the Non-DMA-specific code from the
> intended to be generic DW APB SSI DMA code. This isn't that hard,
> since the most part of the spi-dw-mid.c driver in fact implements a
> generic DMA interface for the DW SPI controller driver. The only Intel
> MID specifics concern getting the max frequency from the MRST Clock
> Control Unit and fetching the DMA controller channels from
> corresponding PCIe DMA controller. Since first one is related with the
> SPI interface configuration we moved it' implementation into the
> DW PCIe-SPI driver module. After that former spi-dw-mid.c file
> can be just renamed to be the DW SPI DMA module optionally compiled in to
> the DW APB SSI core driver.

Cc list here is huge!

I think this patch should go immediately after bunch of fixes.

...

> -obj-$(CONFIG_SPI_DESIGNWARE)		+= spi-dw.o
> +obj-$(CONFIG_SPI_DESIGNWARE)		+= spi-dw-core.o
> +spi-dw-core-y				:= spi-dw.o
> +spi-dw-core-$(CONFIG_SPI_DW_DMA)	+= spi-dw-dma.o

We may leave module name the same, right?

obj-$(CONFIG_SPI_DESIGNWARE)		+= spi-dw.o
spi-dw-y				:= spi-dw-core.o
spi-dw-$(CONFIG_SPI_DW_DMA)		+= spi-dw-dma.o

>  obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
> -obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
> -spi-dw-midpci-objs			:= spi-dw-pci.o spi-dw-mid.o
> +obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-pci.o

> -/* Some specific info for SPI0 controller on Intel MID */
> -
> -/* HW info for MRST Clk Control Unit, 32b reg per controller */
> -#define MRST_SPI_CLK_BASE	100000000	/* 100m */
> -#define MRST_CLK_SPI_REG	0xff11d86c
> -#define CLK_SPI_BDIV_OFFSET	0
> -#define CLK_SPI_BDIV_MASK	0x00000007
> -#define CLK_SPI_CDIV_OFFSET	9
> -#define CLK_SPI_CDIV_MASK	0x00000e00
> -#define CLK_SPI_DISABLE_OFFSET	8
> -
> -int dw_spi_mid_init_mfld(struct dw_spi *dws)
> -{
> -	void __iomem *clk_reg;
> -	u32 clk_cdiv;
> -
> -	clk_reg = ioremap(MRST_CLK_SPI_REG, 16);
> -	if (!clk_reg)
> -		return -ENOMEM;
> -
> -	/* Get SPI controller operating freq info */
> -	clk_cdiv = readl(clk_reg + dws->bus_num * sizeof(u32));
> -	clk_cdiv &= CLK_SPI_CDIV_MASK;
> -	clk_cdiv >>= CLK_SPI_CDIV_OFFSET;
> -	dws->max_freq = MRST_SPI_CLK_BASE / (clk_cdiv + 1);
> -
> -	iounmap(clk_reg);
> -
> -	/* Register hook to configure CTRLR0 */
> -	dws->update_cr0 = dw_spi_update_cr0;
> -
> -	dw_spi_mid_setup_dma_mfld(dws);
> -	return 0;
> -}
> -
> -int dw_spi_mid_init_generic(struct dw_spi *dws)
> -{
> -	/* Register hook to configure CTRLR0 */
> -	dws->update_cr0 = dw_spi_update_cr0;
> -
> -	dw_spi_mid_setup_dma_generic(dws);
> -	return 0;
> -}
> +EXPORT_SYMBOL_GPL(dw_spi_mid_setup_dma_generic);
> diff --git a/drivers/spi/spi-dw-pci.c b/drivers/spi/spi-dw-pci.c
> index dde54a918b5d..c13707b8493e 100644
> --- a/drivers/spi/spi-dw-pci.c
> +++ b/drivers/spi/spi-dw-pci.c
> @@ -15,6 +15,15 @@
>  
>  #define DRIVER_NAME "dw_spi_pci"
>  
> +/* HW info for MRST Clk Control Unit, 32b reg per controller */
> +#define MRST_SPI_CLK_BASE	100000000	/* 100m */
> +#define MRST_CLK_SPI_REG	0xff11d86c
> +#define CLK_SPI_BDIV_OFFSET	0
> +#define CLK_SPI_BDIV_MASK	0x00000007
> +#define CLK_SPI_CDIV_OFFSET	9
> +#define CLK_SPI_CDIV_MASK	0x00000e00
> +#define CLK_SPI_DISABLE_OFFSET	8
> +
>  struct spi_pci_desc {
>  	int	(*setup)(struct dw_spi *);
>  	u16	num_cs;
> @@ -22,20 +31,55 @@ struct spi_pci_desc {
>  	u32	max_freq;
>  };
>  
> +static int spi_mid_init(struct dw_spi *dws)
> +{
> +	void __iomem *clk_reg;
> +	u32 clk_cdiv;
> +
> +	clk_reg = ioremap(MRST_CLK_SPI_REG, 16);
> +	if (!clk_reg)
> +		return -ENOMEM;
> +
> +	/* Get SPI controller operating freq info */
> +	clk_cdiv = readl(clk_reg + dws->bus_num * sizeof(u32));
> +	clk_cdiv &= CLK_SPI_CDIV_MASK;
> +	clk_cdiv >>= CLK_SPI_CDIV_OFFSET;
> +	dws->max_freq = MRST_SPI_CLK_BASE / (clk_cdiv + 1);
> +
> +	iounmap(clk_reg);
> +
> +	/* Register hook to configure CTRLR0 */
> +	dws->update_cr0 = dw_spi_update_cr0;
> +
> +	dw_spi_mid_setup_dma_mfld(dws);
> +
> +	return 0;
> +}
> +
> +static int spi_generic_init(struct dw_spi *dws)
> +{
> +	/* Register hook to configure CTRLR0 */
> +	dws->update_cr0 = dw_spi_update_cr0;
> +
> +	dw_spi_mid_setup_dma_generic(dws);
> +
> +	return 0;
> +}
> +
>  static struct spi_pci_desc spi_pci_mid_desc_1 = {
> -	.setup = dw_spi_mid_init_mfld,
> +	.setup = spi_mid_init,
>  	.num_cs = 5,
>  	.bus_num = 0,
>  };
>  
>  static struct spi_pci_desc spi_pci_mid_desc_2 = {
> -	.setup = dw_spi_mid_init_mfld,
> +	.setup = spi_mid_init,
>  	.num_cs = 2,
>  	.bus_num = 1,
>  };
>  
>  static struct spi_pci_desc spi_pci_ehl_desc = {
> -	.setup = dw_spi_mid_init_generic,
> +	.setup = spi_generic_init,
>  	.num_cs = 2,
>  	.bus_num = -1,
>  	.max_freq = 100000000,
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index d0c8b7d3a5d2..75fdcc5e7642 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -265,8 +265,16 @@ extern u32 dw_spi_update_cr0_v1_01a(struct spi_controller *master,
>  				    struct spi_device *spi,
>  				    struct spi_transfer *transfer);
>  
> -/* platform related setup */
> -extern int dw_spi_mid_init_mfld(struct dw_spi *dws);
> -extern int dw_spi_mid_init_generic(struct dw_spi *dws);
> +#ifdef CONFIG_SPI_DW_DMA
> +
> +extern void dw_spi_mid_setup_dma_mfld(struct dw_spi *dws);
> +extern void dw_spi_mid_setup_dma_generic(struct dw_spi *dws);
> +

I would drop blank lines and extern keywords.

> +#else

> +
> +static inline void dw_spi_mid_setup_dma_mfld(struct dw_spi *dws) {}
> +static inline void dw_spi_mid_setup_dma_generic(struct dw_spi *dws) {}
> +

Ditto for blank lines.

> +#endif /* !CONFIG_SPI_DW_DMA */
>  
>  #endif /* DW_SPI_HEADER_H */
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko