[PATCH 0/3 v3] Add SDHCI driver for Tegra

[PATCH 0/3 v3] Add SDHCI driver for Tegra

[Olof Johansson]

Third time is a charm?

Mostly just minor fixups this time, and addition of 8-bit support.

Changes since v1:
        * Rewrote for sdhci-pltfm
	* Dropped/moved some quirks

Changes since v2:
        * Whitespace fixes
        * Changed order of test in get_ro
        * 8-bit support
        * Setup gpio directions for inputs

[PATCH 1/2] sdhci: add quirk for max len ADMA descriptors

[Olof Johansson]

Some controllers misparse segment length 0 as being 0, not 65536. Add
a quirk to deal with it.

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 drivers/mmc/host/sdhci.c  |   10 +++++++---
 include/linux/mmc/sdhci.h |    2 ++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a25db426..c0094c1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1928,10 +1928,14 @@ int sdhci_add_host(struct sdhci_host *host)
 	 * of bytes. When doing hardware scatter/gather, each entry cannot
 	 * be larger than 64 KiB though.
 	 */
-	if (host->flags & SDHCI_USE_ADMA)
-		mmc->max_seg_size = 65536;
-	else
+	if (host->flags & SDHCI_USE_ADMA) {
+		if (host->quirks & SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC)
+			mmc->max_seg_size = 65535;
+		else
+			mmc->max_seg_size = 65536;
+	} else {
 		mmc->max_seg_size = mmc->max_req_size;
+	}
 
 	/*
 	 * Maximum block size. This varies from controller to controller and
diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
index 1fdc673..dfb2106 100644
--- a/include/linux/mmc/sdhci.h
+++ b/include/linux/mmc/sdhci.h
@@ -83,6 +83,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12		(1<<28)
 /* Controller doesn't have HISPD bit field in HI-SPEED SD card */
 #define SDHCI_QUIRK_NO_HISPD_BIT			(1<<29)
+/* Controller treats ADMA descriptors with length 0000h incorrectly */
+#define SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC		(1<<30)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
1.7.3.GIT

Re: [PATCH 1/2] sdhci: add quirk for max len ADMA descriptors

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 537 bytes --]

On Thu, Dec 23, 2010 at 03:27:54AM -0600, Olof Johansson wrote:
> Some controllers misparse segment length 0 as being 0, not 65536. Add
> a quirk to deal with it.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>

I tend to NACK it (but I am not the maintainer). I'd prefer to see a
draft of your idea of a sdhci_add_host_fixup()-function :)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] sdhci: add quirk for max len ADMA descriptors

[Olof Johansson]

On Thu, Dec 23, 2010 at 3:26 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Thu, Dec 23, 2010 at 03:27:54AM -0600, Olof Johansson wrote:
>> Some controllers misparse segment length 0 as being 0, not 65536. Add
>> a quirk to deal with it.
>>
>> Signed-off-by: Olof Johansson <olof@lixom.net>
>
> I tend to NACK it (but I am not the maintainer). I'd prefer to see a
> draft of your idea of a sdhci_add_host_fixup()-function :)

See, I tend to get annoyed when I get asked to clean up others' messes
because something I do is all of the sudden held to a higher standard
than they have been.

So, I see one of three options:

* This go in, and I clean up some of the other drivers separately, _as promised_
* I do a sdhci_host_fixup() function, and only clean up my driver and
the rest will be cleaned up... never, probably
* I just remove this quirk, repost my driver without it, carry the
quirk out of tree and get on with other things that I actually care
about


So, don't overdo it. I already promised I would help clean up this
subsystem. Ok?


Thanks,

-Olof

Re: [PATCH 1/2] sdhci: add quirk for max len ADMA descriptors

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]

Hi Olof,

happy new 2011, first of all.

On Thu, Dec 23, 2010 at 10:26:16AM -0800, Olof Johansson wrote:
> > I tend to NACK it (but I am not the maintainer). I'd prefer to see a
> > draft of your idea of a sdhci_add_host_fixup()-function :)
> 
> See, I tend to get annoyed when I get asked to clean up others' messes
> because something I do is all of the sudden held to a higher standard
> than they have been.

If I wasn't stubborn, the bits would have run out in October already,
and you'd be in a worse situation now ;) That put aside, I anticipated
that Chris is going to pick up your patches nonetheless (which causes no
hard feelings here), that's why I wrote "I am not the maintainer".

BTW I'd say cleaning up mess from the past is not that uncommon in
kernel-development (think BKL), and raising standards is a good thing as
well, no?

I see your point, though, that if you introduce a fixup()-function now,
nobody will convert an existing driver to it. It would be nice to have
the cleanup in one go, but until someone is driving/funding this bigger
step, we have to fiddle with incremental improvements which need a bit
more negotiation, it seems.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/2] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

SDHCI driver for Tegra. This driver plugs in as a new variant of
sdhci-pltfm, using the platform data structure passed in to specify the
GPIOs to use for card detect, write protect and card power enablement.

Original driver (of which only the header file is left):
Signed-off-by: Yvonne Yip <y@palm.com>

The rest, which has been rewritten by now:
Signed-off-by: Olof Johansson <olof@lixom.net>
Reviewed-by: Wolfram Sang <w.sang@pengutronix.de>
Acked-by: Mike Rapoport <mike@compulab.co.il>

---

SDHCI driver for Tegra. Pretty straight forward, a few pieces of
functionality left to fill in but nothing that stops it from going
upstream. Board enablement submitted separately.

Changes since v1:
	* Rewrote for sdhci-pltfm

Changes since v2:
	* Whitespace fixes
	* Changed order of test in get_ro
	* 8-bit support
	* Set gpio directions

 arch/arm/mach-tegra/include/mach/sdhci.h |   28 ++++
 drivers/mmc/host/Kconfig                 |   10 ++
 drivers/mmc/host/Makefile                |    1 +
 drivers/mmc/host/sdhci-pltfm.c           |    3 +
 drivers/mmc/host/sdhci-pltfm.h           |    1 +
 drivers/mmc/host/sdhci-tegra.c           |  251 ++++++++++++++++++++++++++++++
 6 files changed, 294 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-tegra/include/mach/sdhci.h
 create mode 100644 drivers/mmc/host/sdhci-tegra.c

diff --git a/arch/arm/mach-tegra/include/mach/sdhci.h b/arch/arm/mach-tegra/include/mach/sdhci.h
new file mode 100644
index 0000000..02f6ef27
--- /dev/null
+++ b/arch/arm/mach-tegra/include/mach/sdhci.h
@@ -0,0 +1,28 @@
+/*
+ * include/asm-arm/arch-tegra/include/mach/sdhci.h
+ *
+ * Copyright (C) 2009 Palm, Inc.
+ * Author: Yvonne Yip <y@palm.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#ifndef __ASM_ARM_ARCH_TEGRA_SDHCI_H
+#define __ASM_ARM_ARCH_TEGRA_SDHCI_H
+
+#include <linux/mmc/host.h>
+
+struct tegra_sdhci_platform_data {
+	int cd_gpio;
+	int wp_gpio;
+	int power_gpio;
+};
+
+#endif
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index d618e86..25c6a2a 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -140,6 +140,16 @@ config MMC_SDHCI_ESDHC_IMX
 
 	  If unsure, say N.
 
+config MMC_SDHCI_TEGRA
+	tristate "SDHCI platform support for the Tegra SD/MMC Controller"
+	depends on MMC_SDHCI_PLTFM && ARCH_TEGRA
+	select MMC_SDHCI_IO_ACCESSORS
+	help
+	  This selects the Tegra SD/MMC controller. If you have a Tegra
+	  platform with SD or MMC devices, say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_SDHCI_S3C
 	tristate "SDHCI support on Samsung S3C SoC"
 	depends on MMC_SDHCI && PLAT_SAMSUNG
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 7b645ff..fc8f8f0 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_MMC_SDHCI_PLTFM)			+= sdhci-platform.o
 sdhci-platform-y				:= sdhci-pltfm.o
 sdhci-platform-$(CONFIG_MMC_SDHCI_CNS3XXX)	+= sdhci-cns3xxx.o
 sdhci-platform-$(CONFIG_MMC_SDHCI_ESDHC_IMX)	+= sdhci-esdhc-imx.o
+sdhci-platform-$(CONFIG_MMC_SDHCI_TEGRA)	+= sdhci-tegra.o
 
 obj-$(CONFIG_MMC_SDHCI_OF)	+= sdhci-of.o
 sdhci-of-y				:= sdhci-of-core.o
diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 0502f89..d9e6e88 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -170,6 +170,9 @@ static const struct platform_device_id sdhci_pltfm_ids[] = {
 #ifdef CONFIG_MMC_SDHCI_ESDHC_IMX
 	{ "sdhci-esdhc-imx", (kernel_ulong_t)&sdhci_esdhc_imx_pdata },
 #endif
+#ifdef CONFIG_MMC_SDHCI_TEGRA
+	{ "sdhci-tegra", (kernel_ulong_t)&sdhci_tegra_pdata },
+#endif
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, sdhci_pltfm_ids);
diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h
index c1bfe48..6f631e3 100644
--- a/drivers/mmc/host/sdhci-pltfm.h
+++ b/drivers/mmc/host/sdhci-pltfm.h
@@ -22,5 +22,6 @@ struct sdhci_pltfm_host {
 
 extern struct sdhci_pltfm_data sdhci_cns3xxx_pdata;
 extern struct sdhci_pltfm_data sdhci_esdhc_imx_pdata;
+extern struct sdhci_pltfm_data sdhci_tegra_pdata;
 
 #endif /* _DRIVERS_MMC_SDHCI_PLTFM_H */
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
new file mode 100644
index 0000000..de8f4ab
--- /dev/null
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -0,0 +1,251 @@
+/*
+ * Copyright (C) 2010 The Chromium OS Authors <chromium-os-dev@chromium.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/mmc/card.h>
+#include <linux/mmc/host.h>
+
+#include <mach/gpio.h>
+#include <mach/sdhci.h>
+
+#include "sdhci.h"
+#include "sdhci-pltfm.h"
+
+static u32 tegra_sdhci_readl(struct sdhci_host *host, int reg)
+{
+	u32 val;
+
+	if (unlikely(reg == SDHCI_PRESENT_STATE)) {
+		/* Use wp_gpio here instead? */
+		val = readl(host->ioaddr + reg);
+		return val | SDHCI_WRITE_PROTECT;
+	}
+
+	return readl(host->ioaddr + reg);
+}
+
+static u16 tegra_sdhci_readw(struct sdhci_host *host, int reg)
+{
+	if (unlikely(reg == SDHCI_HOST_VERSION)) {
+		/* Erratum: Version register is invalid in HW. */
+		return SDHCI_SPEC_200;
+	}
+
+	return readw(host->ioaddr + reg);
+}
+
+static void tegra_sdhci_writel(struct sdhci_host *host, u32 val, int reg)
+{
+	/* Seems like we're getting spurious timeout and crc errors, so
+	 * disable signalling of them. In case of real errors software
+	 * timers should take care of eventually detecting them.
+	 */
+	if (unlikely(reg == SDHCI_SIGNAL_ENABLE))
+		val &= ~(SDHCI_INT_TIMEOUT|SDHCI_INT_CRC);
+
+	writel(val, host->ioaddr + reg);
+
+	if (unlikely(reg == SDHCI_INT_ENABLE)) {
+		/* Erratum: Must enable block gap interrupt detection */
+		u8 gap_ctrl = readb(host->ioaddr + SDHCI_BLOCK_GAP_CONTROL);
+		if (val & SDHCI_INT_CARD_INT)
+			gap_ctrl |= 0x8;
+		else
+			gap_ctrl &= ~0x8;
+		writeb(gap_ctrl, host->ioaddr + SDHCI_BLOCK_GAP_CONTROL);
+	}
+}
+
+static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci)
+{
+	struct platform_device *pdev = to_platform_device(mmc_dev(sdhci->mmc));
+	struct tegra_sdhci_platform_data *plat;
+
+	plat = pdev->dev.platform_data;
+
+	if (!gpio_is_valid(plat->wp_gpio))
+		return -1;
+
+	return gpio_get_value(plat->wp_gpio);
+}
+
+static irqreturn_t carddetect_irq(int irq, void *data)
+{
+	struct sdhci_host *sdhost = (struct sdhci_host *)data;
+
+	tasklet_schedule(&sdhost->card_tasklet);
+	return IRQ_HANDLED;
+};
+
+static int tegra_sdhci_8bit(struct sdhci_host *host, int bus_width)
+{
+	u32 ctrl;
+
+	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
+	if (bus_width == MMC_BUS_WIDTH_8) {
+		ctrl &= ~SDHCI_CTRL_4BITBUS;
+		ctrl |= SDHCI_CTRL_8BITBUS;
+	} else {
+		ctrl &= ~SDHCI_CTRL_8BITBUS;
+		if (bus_width == MMC_BUS_WIDTH_4)
+			ctrl |= SDHCI_CTRL_4BITBUS;
+		else
+			ctrl &= ~SDHCI_CTRL_4BITBUS;
+	}
+	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+	return 0;
+}
+
+
+static int tegra_sdhci_pltfm_init(struct sdhci_host *host,
+				  struct sdhci_pltfm_data *pdata)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
+	struct tegra_sdhci_platform_data *plat;
+	struct clk *clk;
+	int rc;
+
+	plat = pdev->dev.platform_data;
+	if (plat == NULL) {
+		dev_err(mmc_dev(host->mmc), "missing platform data\n");
+		return -ENXIO;
+	}
+
+	if (gpio_is_valid(plat->power_gpio)) {
+		rc = gpio_request(plat->power_gpio, "sdhci_power");
+		if (rc) {
+			dev_err(mmc_dev(host->mmc),
+				"failed to allocate power gpio\n");
+			goto out;
+		}
+		tegra_gpio_enable(plat->power_gpio);
+		gpio_direction_output(plat->power_gpio, 1);
+	}
+
+	if (gpio_is_valid(plat->cd_gpio)) {
+		rc = gpio_request(plat->cd_gpio, "sdhci_cd");
+		if (rc) {
+			dev_err(mmc_dev(host->mmc),
+				"failed to allocate cd gpio\n");
+			goto out_power;
+		}
+		tegra_gpio_enable(plat->cd_gpio);
+		gpio_direction_input(plat->cd_gpio);
+
+		rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
+				 IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+				 mmc_hostname(host->mmc), host);
+
+		if (rc)	{
+			dev_err(mmc_dev(host->mmc), "request irq error\n");
+			goto out_cd;
+		}
+
+	}
+
+	if (gpio_is_valid(plat->wp_gpio)) {
+		rc = gpio_request(plat->wp_gpio, "sdhci_wp");
+		if (rc) {
+			dev_err(mmc_dev(host->mmc),
+				"failed to allocate wp gpio\n");
+			goto out_cd;
+		}
+		tegra_gpio_enable(plat->wp_gpio);
+		gpio_direction_input(plat->wp_gpio);
+	}
+
+	clk = clk_get(mmc_dev(host->mmc), NULL);
+	if (IS_ERR(clk)) {
+		dev_err(mmc_dev(host->mmc), "clk err\n");
+		rc = PTR_ERR(clk);
+		goto out_wp;
+	}
+	clk_enable(clk);
+	pltfm_host->clk = clk;
+	host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+
+	return 0;
+
+out_wp:
+	if (gpio_is_valid(plat->wp_gpio)) {
+		tegra_gpio_disable(plat->wp_gpio);
+		gpio_free(plat->wp_gpio);
+	}
+
+out_cd:
+	if (gpio_is_valid(plat->cd_gpio)) {
+		tegra_gpio_disable(plat->cd_gpio);
+		gpio_free(plat->cd_gpio);
+	}
+
+out_power:
+	if (gpio_is_valid(plat->power_gpio)) {
+		tegra_gpio_disable(plat->power_gpio);
+		gpio_free(plat->power_gpio);
+	}
+
+out:
+	return rc;
+}
+
+static void tegra_sdhci_pltfm_exit(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
+	struct tegra_sdhci_platform_data *plat;
+
+	plat = pdev->dev.platform_data;
+
+	if (gpio_is_valid(plat->wp_gpio)) {
+		tegra_gpio_disable(plat->wp_gpio);
+		gpio_free(plat->wp_gpio);
+	}
+
+	if (gpio_is_valid(plat->cd_gpio)) {
+		tegra_gpio_disable(plat->cd_gpio);
+		gpio_free(plat->cd_gpio);
+	}
+
+	if (gpio_is_valid(plat->power_gpio)) {
+		tegra_gpio_disable(plat->power_gpio);
+		gpio_free(plat->power_gpio);
+	}
+
+	clk_disable(pltfm_host->clk);
+	clk_put(pltfm_host->clk);
+}
+
+static struct sdhci_ops tegra_sdhci_ops = {
+	.get_ro     = tegra_sdhci_get_ro,
+	.read_l     = tegra_sdhci_readl,
+	.read_w     = tegra_sdhci_readw,
+	.write_l    = tegra_sdhci_writel,
+	.platform_8bit_width = tegra_sdhci_8bit,
+};
+
+struct sdhci_pltfm_data sdhci_tegra_pdata = {
+	.quirks = SDHCI_QUIRK_BROKEN_TIMEOUT_VAL |
+		  SDHCI_QUIRK_SINGLE_POWER_WRITE |
+		  SDHCI_QUIRK_NO_HISPD_BIT |
+		  SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC,
+	.ops  = &tegra_sdhci_ops,
+	.init = tegra_sdhci_pltfm_init,
+	.exit = tegra_sdhci_pltfm_exit,
+};
-- 
1.7.3.GIT

Re: [PATCH 2/2] mmc: add sdhci-tegra driver for Tegra SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]


> Changes since v2:
> 	* Whitespace fixes
> 	* Changed order of test in get_ro

What benefit has the reordering? (And just to make sure: You still
return -1 meaning "read-only". I assume this is intentional)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/2] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

On Thu, Dec 23, 2010 at 3:23 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>
>> Changes since v2:
>>       * Whitespace fixes
>>       * Changed order of test in get_ro
>
> What benefit has the reordering? (And just to make sure: You still
> return -1 meaning "read-only". I assume this is intentional)\

Uh, thanks for the vague feedback last time. I thought that's what
your concern with it was.

But yeah, defaulting to RW makes more sense. v4 coming once I know if
you want me to rip out the new quirk or not.


-Olof

Re: [PATCH 2/2] mmc: add sdhci-tegra driver for Tegra SoCs

[Chris Ball]

Hi Olof,

On Thu, Dec 23, 2010 at 10:27:29AM -0800, Olof Johansson wrote:
> But yeah, defaulting to RW makes more sense. v4 coming once I know if
> you want me to rip out the new quirk or not.

I'll take a v4 with the remaining quirk for now, and we'd be extremely
interested in seeing sdhci_ops->fixup work and quirk removal from you.

For what it's worth, Wolfram's not singling you out -- recently we've
been consistently rejecting patches that add new SDHCI quirks and
asking for rework.  (See Takashi's JMicron 388 patch, for example.)

Thanks!

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 2/2] mmc: add sdhci-tegra driver for Tegra SoCs

[Olof Johansson]

On Thu, Dec 23, 2010 at 3:23 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>
>> Changes since v2:
>>       * Whitespace fixes
>>       * Changed order of test in get_ro
>
> What benefit has the reordering? (And just to make sure: You still
> return -1 meaning "read-only". I assume this is intentional)

Looking at drivers/mmc/core/sd.c:mmc_sd_setup_card(), returning <0
means no RO detection, and will print a warning to that effect. If no
other RO-related quirks are included (i.e. such as
SDHCI_QUIRK_INVERTED_WRITE_PROTECT), the value will be passed up
through sdhci_get_ro and thus handled appropriately.

So on second look the code as it is seems correct to me.

Still, I have a 8-bit fix to incorporate, so I'll repost a v4 anyway.


-Olof

Re: [PATCH 2/2] mmc: add sdhci-tegra driver for Tegra SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On Sat, Jan 01, 2011 at 04:28:46PM -0800, Olof Johansson wrote:
> On Thu, Dec 23, 2010 at 3:23 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> >
> >> Changes since v2:
> >>       * Whitespace fixes
> >>       * Changed order of test in get_ro
> >
> > What benefit has the reordering? (And just to make sure: You still
> > return -1 meaning "read-only". I assume this is intentional)
> 
> Looking at drivers/mmc/core/sd.c:mmc_sd_setup_card(), returning <0
> means no RO detection, and will print a warning to that effect. If no
> other RO-related quirks are included (i.e. such as
> SDHCI_QUIRK_INVERTED_WRITE_PROTECT), the value will be passed up
> through sdhci_get_ro and thus handled appropriately.
> 
> So on second look the code as it is seems correct to me.

ACK. I misread the code before :( No wonder my comment seemed vague to
you, sorry for that. I agree that the code is correct and the reordering
helps readability. The only nitpick I'd have now is to return
-ESOMETHING (-EINVAL?) instead of -1 to make the fault more obvious.

Kind regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]