| Message ID | 877g1l78v9.wl%kuninori.morimoto.gx@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 3 September 2014 04:09, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com> wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Now, tmio_mmc can use .multi_io_quirk callback > instead of MMC_CAP2_NO_MULTI_READ flags. > let's use it. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > v2 -> v3 > > - blk_size_workaround -> multi_io_quirk > > drivers/mmc/host/tmio_mmc_pio.c | 13 +++++++++++++ > include/linux/mfd/tmio.h | 4 ++++ "mfd"? That was an odd place for storing the tmio mmc specific platform data. So, we need an ack for the mfd maintainer on this one as well. Lee, can you have a look? Future wise, I would also suggest the include/linux/mfd/tmio.h to be split up in proper pieces. > 2 files changed, 17 insertions(+) > > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > index ba45413..ff5ff0f 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -970,12 +970,25 @@ static int tmio_mmc_get_ro(struct mmc_host *mmc) > return ret; > } > > +static int tmio_multi_io_quirk(struct mmc_card *card, > + unsigned int direction, int blk_size) > +{ > + struct tmio_mmc_host *host = mmc_priv(card->host); > + struct tmio_mmc_data *pdata = host->pdata; > + > + if (pdata->multi_io_quirk) > + return pdata->multi_io_quirk(card, direction, blk_size); > + > + return blk_size; > +} > + > static const struct mmc_host_ops tmio_mmc_ops = { > .request = tmio_mmc_request, > .set_ios = tmio_mmc_set_ios, > .get_ro = tmio_mmc_get_ro, > .get_cd = mmc_gpio_get_cd, > .enable_sdio_irq = tmio_mmc_enable_sdio_irq, > + .multi_io_quirk = tmio_multi_io_quirk, > }; > > static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) > diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h > index 90436d5..dec750e 100644 > --- a/include/linux/mfd/tmio.h > +++ b/include/linux/mfd/tmio.h > @@ -1,10 +1,12 @@ > #ifndef MFD_TMIO_H > #define MFD_TMIO_H > > +#include <linux/blkdev.h> This isn't needed, right? > #include <linux/device.h> > #include <linux/fb.h> > #include <linux/io.h> > #include <linux/jiffies.h> > +#include <linux/mmc/card.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > > @@ -142,6 +144,8 @@ struct tmio_mmc_data { > /* clock management callbacks */ > int (*clk_enable)(struct platform_device *pdev, unsigned int *f); > void (*clk_disable)(struct platform_device *pdev); > + int (*multi_io_quirk)(struct mmc_card *card, > + unsigned int direction, int blk_size); Do you really need to invent new platform callbacks for this? Wouldn't it be possible to let the driver handle the quirk by itself? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 08 Sep 2014, Ulf Hansson wrote: > On 3 September 2014 04:09, Kuninori Morimoto > <kuninori.morimoto.gx@gmail.com> wrote: > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > > > Now, tmio_mmc can use .multi_io_quirk callback > > instead of MMC_CAP2_NO_MULTI_READ flags. > > let's use it. > > > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > --- > > v2 -> v3 > > > > - blk_size_workaround -> multi_io_quirk > > > > drivers/mmc/host/tmio_mmc_pio.c | 13 +++++++++++++ > > include/linux/mfd/tmio.h | 4 ++++ > > "mfd"? That was an odd place for storing the tmio mmc specific platform data. > > So, we need an ack for the mfd maintainer on this one as well. Lee, > can you have a look? > > Future wise, I would also suggest the include/linux/mfd/tmio.h to be > split up in proper pieces. struct tmio_mmc_data looks like it's populated in the MFD driver, thus this is probably the correct place for it. > > 2 files changed, 17 insertions(+) > > > > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > > index ba45413..ff5ff0f 100644 > > --- a/drivers/mmc/host/tmio_mmc_pio.c > > +++ b/drivers/mmc/host/tmio_mmc_pio.c > > @@ -970,12 +970,25 @@ static int tmio_mmc_get_ro(struct mmc_host *mmc) > > return ret; > > } > > > > +static int tmio_multi_io_quirk(struct mmc_card *card, > > + unsigned int direction, int blk_size) > > +{ > > + struct tmio_mmc_host *host = mmc_priv(card->host); > > + struct tmio_mmc_data *pdata = host->pdata; > > + > > + if (pdata->multi_io_quirk) > > + return pdata->multi_io_quirk(card, direction, blk_size); > > + > > + return blk_size; > > +} > > + > > static const struct mmc_host_ops tmio_mmc_ops = { > > .request = tmio_mmc_request, > > .set_ios = tmio_mmc_set_ios, > > .get_ro = tmio_mmc_get_ro, > > .get_cd = mmc_gpio_get_cd, > > .enable_sdio_irq = tmio_mmc_enable_sdio_irq, > > + .multi_io_quirk = tmio_multi_io_quirk, > > }; > > > > static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) > > diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h > > index 90436d5..dec750e 100644 > > --- a/include/linux/mfd/tmio.h > > +++ b/include/linux/mfd/tmio.h > > @@ -1,10 +1,12 @@ > > #ifndef MFD_TMIO_H > > #define MFD_TMIO_H > > > > +#include <linux/blkdev.h> > > This isn't needed, right? > > > #include <linux/device.h> > > #include <linux/fb.h> > > #include <linux/io.h> > > #include <linux/jiffies.h> > > +#include <linux/mmc/card.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > > > @@ -142,6 +144,8 @@ struct tmio_mmc_data { > > /* clock management callbacks */ > > int (*clk_enable)(struct platform_device *pdev, unsigned int *f); > > void (*clk_disable)(struct platform_device *pdev); > > + int (*multi_io_quirk)(struct mmc_card *card, > > + unsigned int direction, int blk_size); > > Do you really need to invent new platform callbacks for this? Wouldn't > it be possible to let the driver handle the quirk by itself? Obviously I can't Ack this patch until Ulf is satisfied.
On 8 September 2014 13:11, Lee Jones <lee.jones@linaro.org> wrote: > On Mon, 08 Sep 2014, Ulf Hansson wrote: > >> On 3 September 2014 04:09, Kuninori Morimoto >> <kuninori.morimoto.gx@gmail.com> wrote: >> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> > >> > Now, tmio_mmc can use .multi_io_quirk callback >> > instead of MMC_CAP2_NO_MULTI_READ flags. >> > let's use it. >> > >> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> > --- >> > v2 -> v3 >> > >> > - blk_size_workaround -> multi_io_quirk >> > >> > drivers/mmc/host/tmio_mmc_pio.c | 13 +++++++++++++ >> > include/linux/mfd/tmio.h | 4 ++++ >> >> "mfd"? That was an odd place for storing the tmio mmc specific platform data. >> >> So, we need an ack for the mfd maintainer on this one as well. Lee, >> can you have a look? >> >> Future wise, I would also suggest the include/linux/mfd/tmio.h to be >> split up in proper pieces. > > struct tmio_mmc_data looks like it's populated in the MFD driver, thus > this is probably the correct place for it. You are right, just that we normally put these in include/linux/platform_data/* nowadays. Similar applies to struct tmio_nand_data and struct tmio_fb_data. Additionally, struct tmio_mmc_dma, could be moved to mmc and may be forward declared at wherever struct tmio_mmc_data is put. I also found tmio_core_mmc_enable(), tmio_core_mmc_resume(), tmio_core_mmc_pwr() and tmio_core_mmc_clk_div() - those could stay in a local mfd subsystem header file instead of in /include/linux/... Anyway, I just thought it make sense to clean it up, to possibly prevent further unnecessary cross subsystem acks. :-) Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf > > @@ -142,6 +144,8 @@ struct tmio_mmc_data { > > /* clock management callbacks */ > > int (*clk_enable)(struct platform_device *pdev, unsigned int *f); > > void (*clk_disable)(struct platform_device *pdev); > > + int (*multi_io_quirk)(struct mmc_card *card, > > + unsigned int direction, int blk_size); > > Do you really need to invent new platform callbacks for this? Wouldn't > it be possible to let the driver handle the quirk by itself? sh_mobile_sdhi is based on tmio_mmc_xxx, and sh_mobile_sdhi side needs this quirk. I don't know original tmio_mmc_xxx HW. Of course we can add new flag to tmio driver, but, it is same way as before ? Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 September 2014 02:14, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com> wrote: > > Hi Ulf > >> > @@ -142,6 +144,8 @@ struct tmio_mmc_data { >> > /* clock management callbacks */ >> > int (*clk_enable)(struct platform_device *pdev, unsigned int *f); >> > void (*clk_disable)(struct platform_device *pdev); >> > + int (*multi_io_quirk)(struct mmc_card *card, >> > + unsigned int direction, int blk_size); >> >> Do you really need to invent new platform callbacks for this? Wouldn't >> it be possible to let the driver handle the quirk by itself? > > sh_mobile_sdhi is based on tmio_mmc_xxx, > and sh_mobile_sdhi side needs this quirk. > I don't know original tmio_mmc_xxx HW. > > Of course we can add new flag to tmio driver, > but, it is same way as before ? You are right, let's keep this patch as is, besides the "#include <linux/blkdev.h>", which isn't needed. Sorry for the noise. Kind regards Uffe > > Best regards > --- > Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ulf > > sh_mobile_sdhi is based on tmio_mmc_xxx, > > and sh_mobile_sdhi side needs this quirk. > > I don't know original tmio_mmc_xxx HW. > > > > Of course we can add new flag to tmio driver, > > but, it is same way as before ? > > You are right, let's keep this patch as is, besides the "#include > <linux/blkdev.h>", which isn't needed. > > Sorry for the noise. no problem Please let me know if you need v4 patch Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index ba45413..ff5ff0f 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -970,12 +970,25 @@ static int tmio_mmc_get_ro(struct mmc_host *mmc) return ret; } +static int tmio_multi_io_quirk(struct mmc_card *card, + unsigned int direction, int blk_size) +{ + struct tmio_mmc_host *host = mmc_priv(card->host); + struct tmio_mmc_data *pdata = host->pdata; + + if (pdata->multi_io_quirk) + return pdata->multi_io_quirk(card, direction, blk_size); + + return blk_size; +} + static const struct mmc_host_ops tmio_mmc_ops = { .request = tmio_mmc_request, .set_ios = tmio_mmc_set_ios, .get_ro = tmio_mmc_get_ro, .get_cd = mmc_gpio_get_cd, .enable_sdio_irq = tmio_mmc_enable_sdio_irq, + .multi_io_quirk = tmio_multi_io_quirk, }; static int tmio_mmc_init_ocr(struct tmio_mmc_host *host) diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h index 90436d5..dec750e 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -1,10 +1,12 @@ #ifndef MFD_TMIO_H #define MFD_TMIO_H +#include <linux/blkdev.h> #include <linux/device.h> #include <linux/fb.h> #include <linux/io.h> #include <linux/jiffies.h> +#include <linux/mmc/card.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> @@ -142,6 +144,8 @@ struct tmio_mmc_data { /* clock management callbacks */ int (*clk_enable)(struct platform_device *pdev, unsigned int *f); void (*clk_disable)(struct platform_device *pdev); + int (*multi_io_quirk)(struct mmc_card *card, + unsigned int direction, int blk_size); }; /*