From patchwork Tue Jun 21 02:09:20 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Simon Horman X-Patchwork-Id: 899672 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p5L29RuZ010518 for ; Tue, 21 Jun 2011 02:09:28 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755353Ab1FUCJ1 (ORCPT ); Mon, 20 Jun 2011 22:09:27 -0400 Received: from kirsty.vergenet.net ([202.4.237.240]:52314 "EHLO kirsty.vergenet.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755009Ab1FUCJ0 (ORCPT ); Mon, 20 Jun 2011 22:09:26 -0400 Received: from joe.akashicho.tokyo.vergenet.net (219-109-213-121.bitcat.net [219.109.213.121]) by kirsty.vergenet.net (Postfix) with ESMTP id AC1C724051; Tue, 21 Jun 2011 12:09:23 +1000 (EST) Received: by joe.akashicho.tokyo.vergenet.net (Postfix, from userid 7100) id AE7E457413C; Tue, 21 Jun 2011 11:09:20 +0900 (JST) Date: Tue, 21 Jun 2011 11:09:20 +0900 From: Simon Horman To: Magnus Damm Cc: linux-mmc@vger.kernel.org, linux-sh@vger.kernel.org, Guennadi Liakhovetski , Paul Mundt , Chris Ball Subject: Re: [PATCH 3/5] mmc: sdhi: Add write16_hook Message-ID: <20110621020913.GE16230@verge.net.au> References: <1308610812-3479-1-git-send-email-horms@verge.net.au> <1308610812-3479-4-git-send-email-horms@verge.net.au> <20110621005020.GB16230@verge.net.au> <20110621011257.GD16230@verge.net.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Organisation: Horms Solutions Ltd. User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-mmc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Tue, 21 Jun 2011 02:09:28 +0000 (UTC) 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 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 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 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: Acked-by: Magnus Damm --- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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 }