Re: [PATCH 3/5] mmc: sdhi: Add write16_hook

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

On Tue, Jun 21, 2011 at 09:59:37AM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 9:50 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Jun 21, 2011 at 09:36:03AM +0900, Magnus Damm wrote:
> >> On Tue, Jun 21, 2011 at 8:00 AM, Simon Horman <horms@verge.net.au> wrote:

[snip]

> >> > index 5a90266..0dc9804 100644
> >> > --- a/include/linux/mfd/tmio.h
> >> > +++ b/include/linux/mfd/tmio.h
> >> > @@ -94,6 +101,7 @@ struct tmio_mmc_data {
> >> >        void (*set_pwr)(struct platform_device *host, int state);
> >> >        void (*set_clk_div)(struct platform_device *host, int state);
> >> >        int (*get_cd)(struct platform_device *host);
> >> > +       int (*write16_hook)(struct tmio_mmc_host *host, int addr);
> >> >  };
> >> >
> >> >  static inline void tmio_mmc_cd_wakeup(struct tmio_mmc_data *pdata)
> >>
> >> What's the reason behind passing "struct tmio_mmc_host *"  as an
> >> argument to the new hook? Performance? All other callbacks seem to
> >> take a "struct platform_device *", so being consistent here may be
> >> good unless it comes with too much overhead.
> >
> > The reason is that
> > 1) The hook is called from sd_ctrl_write16 which takes
> >   struct tmio_mmc_host * as its first argument and;
> > 2) The hook that has been implemented calls sd_ctrl_read16() which takes a
> >   struct tmio_mmc_host * as its first argument.
> > So it seemed logical to pass that down.
> >
> > In the caes of 1) we can get the struct platform_device * using host->pdev.
> > However, in the case of 2) is it less clear to me how we can get the
> > struct tmio_mmc_host * from a struct platform_device *.
> 
> Have a look at the code in tmio_mmc_host_suspend() for some code that
> does struct device * -> struct tmio_mmc_host *:
> int tmio_mmc_host_suspend(struct device *dev)
> {
> 	struct mmc_host *mmc = dev_get_drvdata(dev);
> 	struct tmio_mmc_host *host = mmc_priv(mmc);
> 
> You can easily change the dev_get_drvdata() to platform_get_drvdata(),
> see include/linux/platform_device.h

Thanks, I'm happy to make that change if you think it is worth it.
(I will need to re-test on AG5, which I could do this afternoon
 if it is free)

> I guess a similar conversion can be done in tmio_mmc_enable_dma() to
> move from writew() to sd_ctrl_write16()?

Are you proposing changing tmio_mmc_enable_dma() to take
a struct platform_device * as its first argument?

tmio_mmc_enable_dma() is already altered in one of the
patches in this series to use sd_ctrl_write16() without
altering the arguments taht tmio_mmc_enable_dma() takes.

static void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
{
#if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)
	/* Switch DMA mode on or off - SuperH specific? */
	sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
#endif
}