| Message ID | 1405228870-5088-2-git-send-email-Vincent.Yang@tw.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jul 13, 2014 at 01:21:07PM +0800, Vincent Yang wrote: > @@ -1763,6 +1763,11 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host, > ctrl |= SDHCI_CTRL_VDD_180; > sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); > > + /* Some controller need to do more when switching */ > + if ((host->quirks2 & SDHCI_QUIRK2_VOLTAGE_SWITCH) && > + host->ops->voltage_switch) > + host->ops->voltage_switch(host); > + Why do you heed SDHCI_QUIRK2_VOLTAGE_SWITCH? Isn't populating ops->voltage_switch enough? to indicate that something needs to be done? It would also be better to turn sdhci.c into a library, and have the platform driver call the appropriate functions in sdhci rather than having sdhci be a core driver with loads of quirks. This is what I've done in my previous series where I changed stuff such as the set_bus_width(), set_uhs_signaling() and similar callbacks. So, it probably makes more sense to split sdhci_do_start_signal_voltage_switch() into a load of smaller library functions which drivers can call in an appropriate sequence, rather than having a quirk hook. The problem with quirk hooks is that what is right for one device is not right for another device - eventually you end up with lots of quirk callbacks scattered on every alternate line. That doesn't scale. Experienced kernel programmers know this and this is why words like "framework" fill those who have encountered this problem with dread. sdhci.c is a prime example of this kind of design mistake.
>-----Original Message----- >From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk] >On Sun, Jul 13, 2014 at 01:21:07PM +0800, Vincent Yang wrote: >> @@ -1763,6 +1763,11 @@ static int sdhci_do_start_signal_voltage_switch(struct >sdhci_host *host, >> ctrl |= SDHCI_CTRL_VDD_180; >> sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); >> >> + /* Some controller need to do more when switching */ >> + if ((host->quirks2 & SDHCI_QUIRK2_VOLTAGE_SWITCH) && >> + host->ops->voltage_switch) >> + host->ops->voltage_switch(host); >> + > >Why do you heed SDHCI_QUIRK2_VOLTAGE_SWITCH? Isn't populating >ops->voltage_switch enough? to indicate that something needs to be >done? Hi Russell, I will update it as below and remove SDHCI_QUIRK2_VOLTAGE_SWITCH in next version. + /* Some controller need to do more when switching */ + if (host->ops->voltage_switch) + host->ops->voltage_switch(host); + Thanks a lot for your review! Best regards, Vincent Yang > >It would also be better to turn sdhci.c into a library, and have >the platform driver call the appropriate functions in sdhci rather >than having sdhci be a core driver with loads of quirks. This is >what I've done in my previous series where I changed stuff such as >the set_bus_width(), set_uhs_signaling() and similar callbacks. > >So, it probably makes more sense to split >sdhci_do_start_signal_voltage_switch() into a load of smaller library >functions which drivers can call in an appropriate sequence, rather >than having a quirk hook. > >The problem with quirk hooks is that what is right for one device >is not right for another device - eventually you end up with lots >of quirk callbacks scattered on every alternate line. That doesn't >scale. > >Experienced kernel programmers know this and this is why words like >"framework" fill those who have encountered this problem with dread. >sdhci.c is a prime example of this kind of design mistake. > >-- >FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up >according to speedtest.net.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 47055f3..d62262b 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1763,6 +1763,11 @@ static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host, ctrl |= SDHCI_CTRL_VDD_180; sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2); + /* Some controller need to do more when switching */ + if ((host->quirks2 & SDHCI_QUIRK2_VOLTAGE_SWITCH) && + host->ops->voltage_switch) + host->ops->voltage_switch(host); + /* Wait for 5ms */ usleep_range(5000, 5500); diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index 4a5cd5e..63c7a46 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -292,6 +292,7 @@ struct sdhci_ops { void (*adma_workaround)(struct sdhci_host *host, u32 intmask); void (*platform_init)(struct sdhci_host *host); void (*card_event)(struct sdhci_host *host); + void (*voltage_switch)(struct sdhci_host *host); }; #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h index 08abe99..5433f04 100644 --- a/include/linux/mmc/sdhci.h +++ b/include/linux/mmc/sdhci.h @@ -98,6 +98,8 @@ struct sdhci_host { #define SDHCI_QUIRK2_BROKEN_HS200 (1<<6) /* Controller does not support DDR50 */ #define SDHCI_QUIRK2_BROKEN_DDR50 (1<<7) +/* Do a callback when switching voltages so do controller-specific actions */ +#define SDHCI_QUIRK2_VOLTAGE_SWITCH (1<<8) int irq; /* Device IRQ */ void __iomem *ioaddr; /* Mapped address */
This patch defines a quirk to do a callback when switching voltages so do controller-specific actions. It is a preparation and will be used by Fujitsu SDHCI controller f_sdh30 driver. Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com> --- drivers/mmc/host/sdhci.c | 5 +++++ drivers/mmc/host/sdhci.h | 1 + include/linux/mmc/sdhci.h | 2 ++ 3 files changed, 8 insertions(+)