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.
>> 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.
> 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().
Cheers,
/ magnus
--
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
