On Tue, Jun 21, 2011 at 10:36:05AM +0900, Magnus Damm wrote:
> Hi Simon,
>
> On Tue, Jun 21, 2011 at 10:13 AM, Simon Horman <horms@xxxxxxxxxxxx> wrote:
> > 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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)
>
> Hm, perhaps it can be done with incremental patches in the future?
>
> I think it's good to be consistent and use the same argument passing
> style as other callbacks, but at the same time I'm not 100% sure if
> passing a platform data pointer is the best approach. It probably made
> sense with the old tmio_mmc driver that only hooked up to MFD, but I'm
> not sure if that's the case anymore. I'm sure there is room for plenty
> of cleanups - but exactly what to do I don't know. =)
>
> At least passing a struct tmio_mmc_host * requires little conversion
> which should add minimal overhead.
Ok, lets stick with what we have for now.
Its a fairly trivial change to update the arguments later
if that is wanted. Testing is slightly less trivial
due to availability of hardware, but not a major problem.
Can you Acked-by, or Reviewed-by the patches in this series if
you are now happy with them?
> >> 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?
>
> No, not at all. I just recall someone pointing out that
> tmio_mmc_enable_dma() skipped the tmio_mmc specific I/O routines and
> used writew() directly. I suspected the reason behind this was the
> difficulty of converting between different pointer types, but that may
> not be true.
I was probably the person who pointed that out to you.
I'm unsure of the reason, but at least in its current form
it appears not to be related to the parameters involved.
> > 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.
>
> Ok, that's good.
>
> > 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
> > }
>
> Hm, perhaps it's my mail setup that's the issue, but the version of
> "[PATCH 1/5] mmc: tmio: name 0xd8 as CTL_DMA_ENABLE" that I'm looking
> at is still using writew().
The writew() -> sd_ctrl_write16() change is made by the following patch
in the series, "[PATCH 2/5] mmc: tmio: Share register access functions".
The last hunk looks like this:
diff --git a/drivers/mmc/host/tmio_mmc_dma.c
b/drivers/mmc/host/tmio_mmc_dma.c
index 9c4da66..f24a029 100644
--- a/drivers/mmc/host/tmio_mmc_dma.c
+++ b/drivers/mmc/host/tmio_mmc_dma.c
@@ -26,7 +26,7 @@ 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? */
- writew(enable ? 2 : 0, host->ctl + (CTL_DMA_ENABLE << host->bus_shift));
+ sd_ctrl_write16(host, enable ? 2 : 0, CTL_DMA_ENABLE);
#endif
}
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
