Re: [RFC 4/7] mmc: tmio: Add UHS-I mode support

[Wolfram Sang <wsa@the-dreams.de>]

Hi Ulf,

> Okay, let me elaborate a bit more on what I really meant.
> 
> In tmio_mmc_of_parse(), the mmc->ops gets assigned to the
> &tmio_mmc_ops. Before doing that, you can conditionally assign
> ->start_signal_voltage_switch() callback within the &tmio_mmc_ops,
> right!?

So, lets talk with code. Do you mean something like this on top of my
series? (Compile tested only to see if I got you right):

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 5d2a852f17eebd..89e0c5e9fcc668 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -210,9 +210,10 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 	clk_disable_unprepare(priv->clk);
 }
 
-static int sh_mobile_sdhi_start_signal_voltage_switch(struct tmio_mmc_host *host,
+static int sh_mobile_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
 						      struct mmc_ios *ios)
 {
+	struct tmio_mmc_host *host = mmc_priv(mmc);
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
 	struct pinctrl_state *pin_state;
 	int ret;
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 759bf015d09b84..b1819c74965b47 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -101,7 +101,7 @@ struct tmio_mmc_host {
 	void (*clk_disable)(struct tmio_mmc_host *host);
 	int (*multi_io_quirk)(struct mmc_card *card,
 			      unsigned int direction, int blk_size);
-	int (*start_signal_voltage_switch)(struct tmio_mmc_host *host,
+	int (*start_signal_voltage_switch)(struct mmc_host *mmc,
 					   struct mmc_ios *ios);
 };
 
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 8dee637638c31e..8e2fef7c5e481c 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -1014,16 +1014,6 @@ static int tmio_multi_io_quirk(struct mmc_card *card,
 	return blk_size;
 }
 
-static int tmio_mmc_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
-{
-	struct tmio_mmc_host *host = mmc_priv(mmc);
-
-	if (!host->start_signal_voltage_switch)
-		return 0;
-
-	return host->start_signal_voltage_switch(host, ios);
-}
-
 static int tmio_mmc_card_busy(struct mmc_host *mmc)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
@@ -1031,14 +1021,12 @@ static int tmio_mmc_card_busy(struct mmc_host *mmc)
 	return !(sd_ctrl_read32(host, CTL_STATUS2) & TMIO_STATUS2_DAT0);
 }
 
-static const struct mmc_host_ops tmio_mmc_ops = {
+static 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,
-	.start_signal_voltage_switch
-			= tmio_mmc_start_signal_voltage_switch,
 	.card_busy	= tmio_mmc_card_busy,
 	.multi_io_quirk	= tmio_multi_io_quirk,
 };
@@ -1138,7 +1126,9 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,
 		goto host_free;
 	}
 
+	tmio_mmc_ops.start_signal_voltage_switch = _host->start_signal_voltage_switch;
 	mmc->ops = &tmio_mmc_ops;
+
 	mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
 	mmc->caps2 |= pdata->capabilities2;
 	mmc->max_segs = 32;


> Why am I suggesting this? I want to avoid us evolving the code in the
> wrong direction, similar what has happened to SDHCI. *One* way to
> address this, is to minimize unnecessary variant specific callbacks.

Well, I can't test the original tmio_mmc implementation, only the sdhi
variant. So, we need a variant specific start_signal_voltage_switch().
As I see it, the above patch reduces one layer of abstraction. This is
nice, but the variant specific callback is still there. or?

Or i still got you wrong, then sorry!

Thanks,

   Wolfram