Re: [PATCH/RFC v3 5/6] mmc: renesas_sdhi: add support for R-Car Gen3 SDHI DMAC

[Simon Horman <horms@verge.net.au>]

On Wed, Jul 06, 2016 at 11:21:40PM +0200, Arnd Bergmann wrote:
> On Wednesday, July 6, 2016 10:23:29 PM CEST Simon Horman wrote:
> > @@ -117,6 +124,23 @@ void sdhi_sys_dmac_init_dma(void);
> >  static void sdhi_sys_dmac_init_dma(void) { }
> >  #endif
> >  
> > +#if IS_ENABLED(CONFIG_MMC_SDHI_INTERNAL_DMA)
> > +void sdhi_internal_dmac_init_dma(void);
> > +#else
> > +static void sdhi_internal_dmac_init_dma(void) { }
> > +#endif
> > +
> > +static void sh_mobile_sdhi_init_dma(enum tmio_mmc_dmac_type dmac_type)
> > +{
> > +       switch (dmac_type) {
> > +       case TMIO_MMC_INTERNAL_DMAC:
> > +               return sdhi_internal_dmac_init_dma();
> > +
> > +       case TMIO_MMC_SYSC_DMAC:
> > +               return sdhi_sys_dmac_init_dma();
> > +       }
> > +}
> > +
> >  static void sh_mobile_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
> >  {
> >         u32 val;
> > 
> 
> I've commented on this part for v2 already but got no reply. This
> time I took a little more care to understand the structure of the
> driver and how it gets modified.

Hi Arnd,

somehow I completely missed that feedback, sorry about that.

In general I think there is ample scope to restructure the TMIO/SDHI
drivers as they have grown in various directions over time. I'll look
over your proposal in more detail and see how it fits with my understanding
of TMIO/SDHI.

Wolfram, could you also take a look over this?

> 
> The way I see it, we have the MMC_TMIO_CORE driver that consists of
> tmio_mmc_pio.c and tmio_mmc_dma.c, which are tightly connected though the
> second one is optional, and two front-ends named tmio_mmc.c and
> sh_mobile_sdhi.c.
> 
> The first front-end never uses DMA, while the second one may or may
> not use DMA but is always built with the DMA support linked into the
> core driver.
> 
> I think abstracting the two DMA modes through a structure of
> function pointers as you do is the right strategy, but to build on
> top of that, we can change the link order:
> 
> - rename tmio_mmc_pio.c to tmio_mmc_core.c and make that the actual
>   driver core (without DMA)
> - tmio_mmc_dma.c becomes the main driver module for sh_mobile_sdhi
>   and gets the sh_mobile_sdhi_driver structure, sh_mobile_sdhi_of_match
>   table and module_platform_driver() statement
> - the existing sh_mobile_sdhi.c is a library module that exports
>   sh_mobile_sdhi_probe() and sh_mobile_sdhi_remove(), which now
>   gain a 'struct tmio_mmc_dma_ops *' and a 'struct sh_mobile_sdhi_of_data *'
>   argument and get called by the new probe function in what used to
>   be tmio_mmc_dma.c.
> - The new renesas_sdhi_internal_dmac.c becomes a third top-level
>   module that also calls sh_mobile_sdhi_probe() but passes its
>   own struct tmio_mmc_dma_ops and registers a different
>   platform_driver that only matches the of_rcar_gen3_compatible.
> 
> 	Arnd
>