[PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Stephen Warren]

Update gpio.txt based on recent discussions regarding interaction with the
pinctrl subsystem.

Previously, gpio_request() was described as explicitly not performing any
required mux setup operations etc.

Now, gpio_request() is explicitly as explicitly performing any required mux
setup operations where possible. In the case it isn't, platform code is
required to have set up any required muxing or other configuration prior to
gpio_request() being called, in order to maintain the same semantics.

This is achieved by gpiolib drivers calling e.g. pinctrl_request_gpio() in
their .request() operation.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 Documentation/gpio.txt |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index 792faa3..3c4a702 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -271,9 +271,25 @@ Some platforms may also use knowledge about what GPIOs are active for
 power management, such as by powering down unused chip sectors and, more
 easily, gating off unused clocks.
 
-Note that requesting a GPIO does NOT cause it to be configured in any
-way; it just marks that GPIO as in use.  Separate code must handle any
-pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown).
+Requesting a GPIO should cause any pin multiplexing hardware to be programmed
+to route the GPIO signal to the appropriate pin. In some cases, this requires
+programming separate pin multiplexing hardware outside the GPIO controller.
+Equally, for GPIOs that use pins known to the pinctrl subsystem, that
+subsystem should be informed of their use. These requirements may be satisfied
+by having a gpiolib driver's .request() operation call pinctrl_request_gpio().
+Similarly, a gpiolib driver's .free() operation may call pinctrl_free_gpio().
+
+Some platforms allow some or all GPIO signals to be routed to different pins.
+Similarly, other aspects of the GPIO or pin may need to be configured, such as
+pullup/pulldown. Platform software should arrange that any such details are
+configured priored to gpio_request() being called for those GPIOs, such that
+GPIO users need not be aware of these details.
+
+gpiolib drivers may need to call additional pinctrl APIs to implement certain
+operations. This would be the case if e.g. GPIO input/output value is
+controlled by a GPIO HW module, whereas GPIO direction is controlled by a
+separate pin controller HW module. Functions pinctrl_gpio_direction_input()
+and pinctrl_gpio_direction_output() may be used to implement this.
 
 Also note that it's your responsibility to have stopped using a GPIO
 before you free it.
-- 
1.7.5.4

[PATCH 2/2] gpio: tegra: Delete tegra_gpio_enable/disable()

[Stephen Warren]

Recent pinctrl discussions concluded that gpio_request() and other gpiolib
APIs should in fact do whatever is required to mux a GPIO onto pins, by
calling pinctrl APIs if required. This change implements this for the
Tegra GPIO driver, and removes the old Tegra-specific APIs which did this.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
I don't believe this depends on other patches.

There may be some slight context dependencies; this patch was generated
against next-20120217, with my recently posted "gpio: tegra: Fix build
issue due to irq_domain rework." applied on top.

 arch/arm/mach-tegra/board-dt-tegra20.c        |    1 -
 arch/arm/mach-tegra/board-harmony-pinmux.c    |   17 ---------
 arch/arm/mach-tegra/board-paz00-pinmux.c      |   14 --------
 arch/arm/mach-tegra/board-pinmux.c            |   34 +------------------
 arch/arm/mach-tegra/board-pinmux.h            |    5 ---
 arch/arm/mach-tegra/board-seaboard-pinmux.c   |   32 ------------------
 arch/arm/mach-tegra/board-seaboard.c          |    1 -
 arch/arm/mach-tegra/board-trimslice-pinmux.c  |   12 -------
 arch/arm/mach-tegra/include/mach/gpio-tegra.h |    9 -----
 arch/arm/mach-tegra/usb_phy.c                 |    1 -
 drivers/gpio/gpio-tegra.c                     |   44 +++++++++++++-----------
 drivers/mmc/host/sdhci-tegra.c                |   24 +++-----------
 12 files changed, 31 insertions(+), 163 deletions(-)

diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c b/arch/arm/mach-tegra/board-dt-tegra20.c
index 7a95e0b..6d1dd19 100644
--- a/arch/arm/mach-tegra/board-dt-tegra20.c
+++ b/arch/arm/mach-tegra/board-dt-tegra20.c
@@ -55,7 +55,6 @@ void ventana_pinmux_init(void);
 
 struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("nvidia,tegra20-pinmux", TEGRA_APB_MISC_BASE + 0x14, "tegra-pinmux", NULL),
-	OF_DEV_AUXDATA("nvidia,tegra20-gpio", TEGRA_GPIO_BASE, "tegra-gpio", NULL),
 	OF_DEV_AUXDATA("nvidia,tegra20-sdhci", TEGRA_SDMMC1_BASE, "sdhci-tegra.0", NULL),
 	OF_DEV_AUXDATA("nvidia,tegra20-sdhci", TEGRA_SDMMC2_BASE, "sdhci-tegra.1", NULL),
 	OF_DEV_AUXDATA("nvidia,tegra20-sdhci", TEGRA_SDMMC3_BASE, "sdhci-tegra.2", NULL),
diff --git a/arch/arm/mach-tegra/board-harmony-pinmux.c b/arch/arm/mach-tegra/board-harmony-pinmux.c
index 465808c..77a6006 100644
--- a/arch/arm/mach-tegra/board-harmony-pinmux.c
+++ b/arch/arm/mach-tegra/board-harmony-pinmux.c
@@ -15,13 +15,11 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/gpio.h>
 #include <linux/of.h>
 
 #include <mach/pinmux.h>
 #include <mach/pinmux-tegra20.h>
 
-#include "gpio-names.h"
 #include "board-harmony.h"
 #include "board-pinmux.h"
 
@@ -144,24 +142,9 @@ static struct tegra_pingroup_config harmony_pinmux[] = {
 	{TEGRA_PINGROUP_XM2D,  TEGRA_MUX_NONE,          TEGRA_PUPD_NORMAL,    TEGRA_TRI_NORMAL},
 };
 
-static struct tegra_gpio_table gpio_table[] = {
-	{ .gpio = TEGRA_GPIO_SD2_CD,		.enable = true	},
-	{ .gpio = TEGRA_GPIO_SD2_WP,		.enable = true	},
-	{ .gpio = TEGRA_GPIO_SD2_POWER,		.enable = true	},
-	{ .gpio = TEGRA_GPIO_SD4_CD,		.enable = true	},
-	{ .gpio = TEGRA_GPIO_SD4_WP,		.enable = true	},
-	{ .gpio = TEGRA_GPIO_SD4_POWER,		.enable = true	},
-	{ .gpio = TEGRA_GPIO_CDC_IRQ,		.enable = true	},
-	{ .gpio = TEGRA_GPIO_HP_DET,		.enable = true	},
-	{ .gpio = TEGRA_GPIO_INT_MIC_EN,	.enable = true	},
-	{ .gpio = TEGRA_GPIO_EXT_MIC_EN,	.enable = true	},
-};
-
 static struct tegra_board_pinmux_conf conf = {
 	.pgs = harmony_pinmux,
 	.pg_count = ARRAY_SIZE(harmony_pinmux),
-	.gpios = gpio_table,
-	.gpio_count = ARRAY_SIZE(gpio_table),
 };
 
 void harmony_pinmux_init(void)
diff --git a/arch/arm/mach-tegra/board-paz00-pinmux.c b/arch/arm/mach-tegra/board-paz00-pinmux.c
index c775572..f0ec466 100644
--- a/arch/arm/mach-tegra/board-paz00-pinmux.c
+++ b/arch/arm/mach-tegra/board-paz00-pinmux.c
@@ -15,13 +15,11 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/gpio.h>
 #include <linux/of.h>
 
 #include <mach/pinmux.h>
 #include <mach/pinmux-tegra20.h>
 
-#include "gpio-names.h"
 #include "board-paz00.h"
 #include "board-pinmux.h"
 
@@ -144,21 +142,9 @@ static struct tegra_pingroup_config paz00_pinmux[] = {
 	{TEGRA_PINGROUP_XM2D,  TEGRA_MUX_NONE,          TEGRA_PUPD_NORMAL,    TEGRA_TRI_NORMAL},
 };
 
-static struct tegra_gpio_table gpio_table[] = {
-	{ .gpio = TEGRA_GPIO_SD1_CD,	.enable = true },
-	{ .gpio = TEGRA_GPIO_SD1_WP,	.enable = true },
-	{ .gpio = TEGRA_GPIO_SD1_POWER,	.enable = true },
-	{ .gpio = TEGRA_ULPI_RST,	.enable = true },
-	{ .gpio = TEGRA_WIFI_PWRN,	.enable = true },
-	{ .gpio = TEGRA_WIFI_RST,	.enable = true },
-	{ .gpio = TEGRA_WIFI_LED,	.enable = true },
-};
-
 static struct tegra_board_pinmux_conf conf = {
 	.pgs = paz00_pinmux,
 	.pg_count = ARRAY_SIZE(paz00_pinmux),
-	.gpios = gpio_table,
-	.gpio_count = ARRAY_SIZE(gpio_table),
 };
 
 void paz00_pinmux_init(void)
diff --git a/arch/arm/mach-tegra/board-pinmux.c b/arch/arm/mach-tegra/board-pinmux.c
index adc3efe..e5e3a24 100644
--- a/arch/arm/mach-tegra/board-pinmux.c
+++ b/arch/arm/mach-tegra/board-pinmux.c
@@ -18,7 +18,6 @@
 #include <linux/of.h>
 #include <linux/string.h>
 
-#include <mach/gpio-tegra.h>
 #include <mach/pinmux.h>
 
 #include "board-pinmux.h"
@@ -26,18 +25,6 @@
 
 struct tegra_board_pinmux_conf *confs[2];
 
-static void tegra_board_pinmux_setup_gpios(void)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(confs); i++) {
-		if (!confs[i])
-			continue;
-
-		tegra_gpio_config(confs[i]->gpios, confs[i]->gpio_count);
-	}
-}
-
 static void tegra_board_pinmux_setup_pinmux(void)
 {
 	int i;
@@ -57,29 +44,12 @@ static void tegra_board_pinmux_setup_pinmux(void)
 static int tegra_board_pinmux_bus_notify(struct notifier_block *nb,
 					 unsigned long event, void *vdev)
 {
-	static bool had_gpio;
-	static bool had_pinmux;
-
-	struct device *dev = vdev;
-	const char *devname;
-
 	if (event != BUS_NOTIFY_BOUND_DRIVER)
 		return NOTIFY_DONE;
 
-	devname = dev_name(dev);
+	tegra_board_pinmux_setup_pinmux();
 
-	if (!had_gpio && !strcmp(devname, GPIO_DEV)) {
-		tegra_board_pinmux_setup_gpios();
-		had_gpio = true;
-	} else if (!had_pinmux && !strcmp(devname, PINMUX_DEV)) {
-		tegra_board_pinmux_setup_pinmux();
-		had_pinmux = true;
-	}
-
-	if (had_gpio && had_pinmux)
-		return NOTIFY_STOP_MASK;
-	else
-		return NOTIFY_DONE;
+	return NOTIFY_STOP_MASK;
 }
 
 static struct notifier_block nb = {
diff --git a/arch/arm/mach-tegra/board-pinmux.h b/arch/arm/mach-tegra/board-pinmux.h
index 4aac735..e08214d 100644
--- a/arch/arm/mach-tegra/board-pinmux.h
+++ b/arch/arm/mach-tegra/board-pinmux.h
@@ -15,11 +15,9 @@
 #ifndef __MACH_TEGRA_BOARD_PINMUX_H
 #define __MACH_TEGRA_BOARD_PINMUX_H
 
-#define GPIO_DEV "tegra-gpio"
 #define PINMUX_DEV "tegra-pinmux"
 
 struct tegra_pingroup_config;
-struct tegra_gpio_table;
 
 struct tegra_board_pinmux_conf {
 	struct tegra_pingroup_config *pgs;
@@ -27,9 +25,6 @@ struct tegra_board_pinmux_conf {
 
 	struct tegra_drive_pingroup_config *drives;
 	int drive_count;
-
-	struct tegra_gpio_table *gpios;
-	int gpio_count;
 };
 
 void tegra_board_pinmux_init(struct tegra_board_pinmux_conf *conf_a,
diff --git a/arch/arm/mach-tegra/board-seaboard-pinmux.c b/arch/arm/mach-tegra/board-seaboard-pinmux.c
index 55e7e43..3bf7e97 100644
--- a/arch/arm/mach-tegra/board-seaboard-pinmux.c
+++ b/arch/arm/mach-tegra/board-seaboard-pinmux.c
@@ -15,13 +15,11 @@
 
 #include <linux/kernel.h>
 #include <linux/init.h>
-#include <linux/gpio.h>
 #include <linux/of.h>
 
 #include <mach/pinmux.h>
 #include <mach/pinmux-tegra20.h>
 
-#include "gpio-names.h"
 #include "board-pinmux.h"
 #include "board-seaboard.h"
 
@@ -179,35 +177,9 @@ static struct tegra_pingroup_config ventana_pinmux[] = {
 	{TEGRA_PINGROUP_SPIG, TEGRA_MUX_SPI2_ALT, TEGRA_PUPD_NORMAL,    TEGRA_TRI_TRISTATE},
 };
 
-static struct tegra_gpio_table common_gpio_table[] = {
-	{ .gpio = TEGRA_GPIO_SD2_CD,		.enable = true },
-	{ .gpio = TEGRA_GPIO_SD2_WP,		.enable = true },
-	{ .gpio = TEGRA_GPIO_SD2_POWER,		.enable = true },
-	{ .gpio = TEGRA_GPIO_CDC_IRQ,		.enable = true },
-};
-
-static struct tegra_gpio_table seaboard_gpio_table[] = {
-	{ .gpio = TEGRA_GPIO_LIDSWITCH,		.enable = true },
-	{ .gpio = TEGRA_GPIO_POWERKEY,		.enable = true },
-	{ .gpio = TEGRA_GPIO_HP_DET,		.enable = true },
-	{ .gpio = TEGRA_GPIO_ISL29018_IRQ,	.enable = true },
-	{ .gpio = TEGRA_GPIO_USB1,		.enable = true },
-};
-
-static struct tegra_gpio_table ventana_gpio_table[] = {
-	/* hp_det */
-	{ .gpio = TEGRA_GPIO_PW2,		.enable = true },
-	/* int_mic_en */
-	{ .gpio = TEGRA_GPIO_PX0,		.enable = true },
-	/* ext_mic_en */
-	{ .gpio = TEGRA_GPIO_PX1,		.enable = true },
-};
-
 static struct tegra_board_pinmux_conf common_conf = {
 	.pgs = common_pinmux,
 	.pg_count = ARRAY_SIZE(common_pinmux),
-	.gpios = common_gpio_table,
-	.gpio_count = ARRAY_SIZE(common_gpio_table),
 };
 
 static struct tegra_board_pinmux_conf seaboard_conf = {
@@ -215,15 +187,11 @@ static struct tegra_board_pinmux_conf seaboard_conf = {
 	.pg_count = ARRAY_SIZE(seaboard_pinmux),
 	.drives = seaboard_drive_pinmux,
 	.drive_count = ARRAY_SIZE(seaboard_drive_pinmux),
-	.gpios = seaboard_gpio_table,
-	.gpio_count = ARRAY_SIZE(seaboard_gpio_table),
 };
 
 static struct tegra_board_pinmux_conf ventana_conf = {
 	.pgs = ventana_pinmux,
 	.pg_count = ARRAY_SIZE(ventana_pinmux),
-	.gpios = ventana_gpio_table,
-	.gpio_count = ARRAY_SIZE(ventana_gpio_table),
 };
 
 void seaboard_pinmux_init(void)
diff --git a/arch/arm/mach-tegra/board-seaboard.c b/arch/arm/mach-tegra/board-seaboard.c
index d669847..74a9db8 100644
--- a/arch/arm/mach-tegra/board-seaboard.c
+++ b/arch/arm/mach-tegra/board-seaboard.c
@@ -261,7 +261,6 @@ static void __init tegra_kaen_init(void)
 	debug_uart_platform_data[0].irq = INT_UARTB;
 
 	seaboard_audio_pdata.gpio_hp_mute = TEGRA_GPIO_KAEN_HP_MUTE;
-	tegra_gpio_enable(TEGRA_GPIO_KAEN_HP_MUTE);
 
 	seaboard_common_init();
 
diff --git a/arch/arm/mach-tegra/board-trimslice-pinmux.c b/arch/arm/mach-tegra/board-trimslice-pinmux.c
index a21a2be..a1902d4 100644
--- a/arch/arm/mach-tegra/board-trimslice-pinmux.c
+++ b/arch/arm/mach-tegra/board-trimslice-pinmux.c
@@ -13,7 +13,6 @@
  * GNU General Public License for more details.
  *
  */
-#include <linux/gpio.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/of.h>
@@ -21,7 +20,6 @@
 #include <mach/pinmux.h>
 #include <mach/pinmux-tegra20.h>
 
-#include "gpio-names.h"
 #include "board-pinmux.h"
 #include "board-trimslice.h"
 
@@ -144,19 +142,9 @@ static struct tegra_pingroup_config trimslice_pinmux[] = {
 	{TEGRA_PINGROUP_XM2D,  TEGRA_MUX_NONE,          TEGRA_PUPD_NORMAL,      TEGRA_TRI_NORMAL},
 };
 
-static struct tegra_gpio_table gpio_table[] = {
-	{ .gpio = TRIMSLICE_GPIO_SD4_CD, .enable = true	}, /* mmc4 cd */
-	{ .gpio = TRIMSLICE_GPIO_SD4_WP, .enable = true	}, /* mmc4 wp */
-
-	{ .gpio = TRIMSLICE_GPIO_USB1_MODE, .enable = true }, /* USB1 mode */
-	{ .gpio = TRIMSLICE_GPIO_USB2_RST,  .enable = true }, /* USB2 PHY rst */
-};
-
 static struct tegra_board_pinmux_conf conf = {
 	.pgs = trimslice_pinmux,
 	.pg_count = ARRAY_SIZE(trimslice_pinmux),
-	.gpios = gpio_table,
-	.gpio_count = ARRAY_SIZE(gpio_table),
 };
 
 void trimslice_pinmux_init(void)
diff --git a/arch/arm/mach-tegra/include/mach/gpio-tegra.h b/arch/arm/mach-tegra/include/mach/gpio-tegra.h
index 6140820..a978b3c 100644
--- a/arch/arm/mach-tegra/include/mach/gpio-tegra.h
+++ b/arch/arm/mach-tegra/include/mach/gpio-tegra.h
@@ -25,13 +25,4 @@
 
 #define TEGRA_NR_GPIOS		INT_GPIO_NR
 
-struct tegra_gpio_table {
-	int	gpio;	/* GPIO number */
-	bool	enable;	/* Enable for GPIO at init? */
-};
-
-void tegra_gpio_config(struct tegra_gpio_table *table, int num);
-void tegra_gpio_enable(int gpio);
-void tegra_gpio_disable(int gpio);
-
 #endif
diff --git a/arch/arm/mach-tegra/usb_phy.c b/arch/arm/mach-tegra/usb_phy.c
index 37576a7..9de6c6b 100644
--- a/arch/arm/mach-tegra/usb_phy.c
+++ b/arch/arm/mach-tegra/usb_phy.c
@@ -710,7 +710,6 @@ struct tegra_usb_phy *tegra_usb_phy_open(int instance, void __iomem *regs,
 			err = -ENXIO;
 			goto err1;
 		}
-		tegra_gpio_enable(ulpi_config->reset_gpio);
 		gpio_request(ulpi_config->reset_gpio, "ulpi_phy_reset_b");
 		gpio_direction_output(ulpi_config->reset_gpio, 0);
 		phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
index 6f17671..1177199 100644
--- a/drivers/gpio/gpio-tegra.c
+++ b/drivers/gpio/gpio-tegra.c
@@ -29,7 +29,6 @@
 
 #include <asm/mach/irq.h>
 
-#include <mach/gpio-tegra.h>
 #include <mach/iomap.h>
 #include <mach/suspend.h>
 
@@ -105,14 +104,32 @@ static void tegra_gpio_mask_write(u32 reg, int gpio, int value)
 	tegra_gpio_writel(val, reg);
 }
 
-void tegra_gpio_enable(int gpio)
+int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
 {
-	tegra_gpio_mask_write(GPIO_MSK_CNF(gpio), gpio, 1);
+	/*
+	 * We should call pinctrl_request_gpio() here. However, we can't. Tegra
+	 * muxes pins in groups not individually, and real boards exist where
+	 * most of a pin group needs to be used for some function, yet some of
+	 * the pins aren't actually used by that function in the mode the
+	 * controller operates on the board, and so those pins can be used as
+	 * GPIOs.
+	 *
+	 * This is true in practice on Seaboard/Springbank, where pin group ATC
+	 * is used by the NAND controller, but pin GMI_IORDY_PI5 is used as a
+	 * GPIO for the SD card slot's CD signal.
+	 *
+	 * On Tegra30, pins are muxed in groups and hence we could call
+	 * pinctrl here, but we don't for now.
+	 */
+	tegra_gpio_mask_write(GPIO_MSK_CNF(offset), offset, 1);
+
+	return 0;
 }
 
-void tegra_gpio_disable(int gpio)
+void tegra_gpio_free(struct gpio_chip *chip, unsigned offset)
 {
-	tegra_gpio_mask_write(GPIO_MSK_CNF(gpio), gpio, 0);
+	tegra_gpio_mask_write(GPIO_MSK_CNF(offset), offset, 0);
+	/* Similarly, we should call pinctrl_free_gpio() here */
 }
 
 static void tegra_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
@@ -146,13 +163,14 @@ static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 
 static struct gpio_chip tegra_gpio_chip = {
 	.label			= "tegra-gpio",
+	.request		= tegra_gpio_request,
+	.free			= tegra_gpio_free,
 	.direction_input	= tegra_gpio_direction_input,
 	.get			= tegra_gpio_get,
 	.direction_output	= tegra_gpio_direction_output,
 	.set			= tegra_gpio_set,
 	.to_irq			= tegra_gpio_to_irq,
 	.base			= 0,
-	.ngpio			= TEGRA_NR_GPIOS,
 };
 
 static void tegra_gpio_irq_ack(struct irq_data *d)
@@ -459,20 +477,6 @@ static int __init tegra_gpio_init(void)
 }
 postcore_initcall(tegra_gpio_init);
 
-void __init tegra_gpio_config(struct tegra_gpio_table *table, int num)
-{
-	int i;
-
-	for (i = 0; i < num; i++) {
-		int gpio = table[i].gpio;
-
-		if (table[i].enable)
-			tegra_gpio_enable(gpio);
-		else
-			tegra_gpio_disable(gpio);
-	}
-}
-
 #ifdef	CONFIG_DEBUG_FS
 
 #include <linux/debugfs.h>
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index ccbca0b..1f63a07 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -270,7 +270,6 @@ static int __devinit sdhci_tegra_probe(struct platform_device *pdev)
 				"failed to allocate power gpio\n");
 			goto err_power_req;
 		}
-		tegra_gpio_enable(plat->power_gpio);
 		gpio_direction_output(plat->power_gpio, 1);
 	}
 
@@ -281,7 +280,6 @@ static int __devinit sdhci_tegra_probe(struct platform_device *pdev)
 				"failed to allocate cd gpio\n");
 			goto err_cd_req;
 		}
-		tegra_gpio_enable(plat->cd_gpio);
 		gpio_direction_input(plat->cd_gpio);
 
 		rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
@@ -302,7 +300,6 @@ static int __devinit sdhci_tegra_probe(struct platform_device *pdev)
 				"failed to allocate wp gpio\n");
 			goto err_wp_req;
 		}
-		tegra_gpio_enable(plat->wp_gpio);
 		gpio_direction_input(plat->wp_gpio);
 	}
 
@@ -330,23 +327,17 @@ err_add_host:
 	clk_disable(pltfm_host->clk);
 	clk_put(pltfm_host->clk);
 err_clk_get:
-	if (gpio_is_valid(plat->wp_gpio)) {
-		tegra_gpio_disable(plat->wp_gpio);
+	if (gpio_is_valid(plat->wp_gpio))
 		gpio_free(plat->wp_gpio);
-	}
 err_wp_req:
 	if (gpio_is_valid(plat->cd_gpio))
 		free_irq(gpio_to_irq(plat->cd_gpio), host);
 err_cd_irq_req:
-	if (gpio_is_valid(plat->cd_gpio)) {
-		tegra_gpio_disable(plat->cd_gpio);
+	if (gpio_is_valid(plat->cd_gpio))
 		gpio_free(plat->cd_gpio);
-	}
 err_cd_req:
-	if (gpio_is_valid(plat->power_gpio)) {
-		tegra_gpio_disable(plat->power_gpio);
+	if (gpio_is_valid(plat->power_gpio))
 		gpio_free(plat->power_gpio);
-	}
 err_power_req:
 err_no_plat:
 	sdhci_pltfm_free(pdev);
@@ -363,21 +354,16 @@ static int __devexit sdhci_tegra_remove(struct platform_device *pdev)
 
 	sdhci_remove_host(host, dead);
 
-	if (gpio_is_valid(plat->wp_gpio)) {
-		tegra_gpio_disable(plat->wp_gpio);
+	if (gpio_is_valid(plat->wp_gpio))
 		gpio_free(plat->wp_gpio);
-	}
 
 	if (gpio_is_valid(plat->cd_gpio)) {
 		free_irq(gpio_to_irq(plat->cd_gpio), host);
-		tegra_gpio_disable(plat->cd_gpio);
 		gpio_free(plat->cd_gpio);
 	}
 
-	if (gpio_is_valid(plat->power_gpio)) {
-		tegra_gpio_disable(plat->power_gpio);
+	if (gpio_is_valid(plat->power_gpio))
 		gpio_free(plat->power_gpio);
-	}
 
 	clk_disable(pltfm_host->clk);
 	clk_put(pltfm_host->clk);
-- 
1.7.5.4

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Russell King - ARM Linux]

On Sun, Feb 19, 2012 at 11:27:42PM -0700, Stephen Warren wrote:
> Update gpio.txt based on recent discussions regarding interaction with the
> pinctrl subsystem.
> 
> Previously, gpio_request() was described as explicitly not performing any
> required mux setup operations etc.
> 
> Now, gpio_request() is explicitly as explicitly performing any required mux
> setup operations where possible. In the case it isn't, platform code is
> required to have set up any required muxing or other configuration prior to
> gpio_request() being called, in order to maintain the same semantics.

So what if you need to have the pin as a GPIO, manipulate it as a GPIO,
and then hand it off to a special function, and then take it back as
a GPIO before you shut the special function down ?

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Linus Walleij]

On Mon, Feb 20, 2012 at 8:39 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Feb 19, 2012 at 11:27:42PM -0700, Stephen Warren wrote:
>> Update gpio.txt based on recent discussions regarding interaction with the
>> pinctrl subsystem.
>>
>> Previously, gpio_request() was described as explicitly not performing any
>> required mux setup operations etc.
>>
>> Now, gpio_request() is explicitly as explicitly performing any required mux
>> setup operations where possible. In the case it isn't, platform code is
>> required to have set up any required muxing or other configuration prior to
>> gpio_request() being called, in order to maintain the same semantics.
>
> So what if you need to have the pin as a GPIO, manipulate it as a GPIO,
> and then hand it off to a special function, and then take it back as
> a GPIO before you shut the special function down ?

I remember this case very well and we designed for it, so it should be handled
by pin control and GPIO thusly:

Example: use pins 1,2 as I2C, then convert them to GPIO for a while
then back again:

// This call looks up a map containing pins 1,2 and reserve them
p = pinctrl_get(dev, "i2c");
if (IS_ERR(p))
  ...
pinctrl_enable(p);
pinctrl_disable(p);
// This will free up the pins again
pinctrl_put(p);
// So now we can do this...
// NB: the GPIO driver calls pinctr_request_gpio() to check
// that it can take these pins
gpio_request(1, "gpio1"):
gpio_request(2, "gpio2");
// This will trigger a reset or something
gpio_direction_output(1, 1);
gpio_direction_output(2, 1);
// Release pins again
gpio_free(1);
gpio_free(2);
// Take them back for this function
p = pinctrl_get(dev, "i2c");

It's a bit kludgy but works and makes sure the pins are only used
for one thing at a time.

BTW: Russell, which specific platform and driver was it that had this
usecase? I'd like to have a look at the code to educate myself.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: tegra: Delete tegra_gpio_enable/disable()

[Linus Walleij]

On Mon, Feb 20, 2012 at 7:27 AM, Stephen Warren <swarren@nvidia.com> wrote:

> Recent pinctrl discussions concluded that gpio_request() and other gpiolib
> APIs should in fact do whatever is required to mux a GPIO onto pins, by
> calling pinctrl APIs if required. This change implements this for the
> Tegra GPIO driver, and removes the old Tegra-specific APIs which did this.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> I don't believe this depends on other patches.
>
> There may be some slight context dependencies; this patch was generated
> against next-20120217, with my recently posted "gpio: tegra: Fix build
> issue due to irq_domain rework." applied on top.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Since it mainly touches mach-tegra stuff I think it's best to take
this through the Tegra tree, Olof can you queue it?

Yours,
Linus Walleij

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Linus Walleij]

On Mon, Feb 20, 2012 at 7:27 AM, Stephen Warren <swarren@nvidia.com> wrote:

> Update gpio.txt based on recent discussions regarding interaction with the
> pinctrl subsystem.
>
> Previously, gpio_request() was described as explicitly not performing any
> required mux setup operations etc.
>
> Now, gpio_request() is explicitly as explicitly performing any required mux
> setup operations where possible. In the case it isn't, platform code is
> required to have set up any required muxing or other configuration prior to
> gpio_request() being called, in order to maintain the same semantics.
>
> This is achieved by gpiolib drivers calling e.g. pinctrl_request_gpio() in
> their .request() operation.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Grant can you take this one? I'd prefer for you to have a look at
it as well.

Yours,
Linus Walleij

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Russell King - ARM Linux]

On Tue, Feb 21, 2012 at 11:41:31AM +0100, Linus Walleij wrote:
> I remember this case very well and we designed for it, so it should
> be handled by pin control and GPIO thusly:
> 
> Example: use pins 1,2 as I2C, then convert them to GPIO for a while
> then back again:
> 
> // This call looks up a map containing pins 1,2 and reserve them
> p = pinctrl_get(dev, "i2c");
> if (IS_ERR(p))
>   ...
> pinctrl_enable(p);
> pinctrl_disable(p);
> // This will free up the pins again
> pinctrl_put(p);
> // So now we can do this...
> // NB: the GPIO driver calls pinctr_request_gpio() to check
> // that it can take these pins
> gpio_request(1, "gpio1"):
> gpio_request(2, "gpio2");
> // This will trigger a reset or something
> gpio_direction_output(1, 1);
> gpio_direction_output(2, 1);
> // Release pins again
> gpio_free(1);
> gpio_free(2);
> // Take them back for this function
> p = pinctrl_get(dev, "i2c");
> 
> It's a bit kludgy but works and makes sure the pins are only used
> for one thing at a time.

No.  The case which I'm thinking of is for the Assabet audio, where
we have the following situation.

We have the SSP bus [TXD,RXD,SFRM,SCLK] which is handled by the SSP core.
This is connected through a FPGA to a UDA1341 codec.  The UDA1341 codec
has DI,DO,WS,BCLK signals.  WS (which is effectively the LRCK and therefore
identifies the left and right channels) is connected through a flip-flop
in the FPGA which toggles at every SFRM pulse.

WS is held high for the left sample.  This is the default setting.
Unfortunately, some FPGA builds out there do not tristate the WS output
when the codec is powered off.

This leads to a high leakage current from the FPGA, into the UDA1341 codec
and into other devices.

The solution is to wait for SSP to finish, and while SSP is still enabled,
switch the pins from the SSP block to the GPIO block and toggle the SFRM
signal to flip the WS output before removing power.  When starting up,
with the pins under control of GPIO, power up and them toggle hte SFRM
signal to restore the WS state before giving it back to the SSP block.

However, here's the thing: SSP must be enabled at the point in time when
the pins are given or taken away from it, otherwise SFRM is indeterminant.
We can't allow gpio_request() to fail at the points where we toggle this
pin - failure is not really an option where causing hardware stress is
involved.  We can't allow the GPIO block to have the state of these pins
change state while the pins are given back to the GPIO block before the
GPIOs have been requested (glitches on the SFRM line will cause the state
of the FIFO to change.)

Another case where this kind of run-time switching needs to occur is the
PXA IrDA driver, where the pins need to be switched between their FIR and
SIR modes.

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Linus Walleij]

On Tue, Feb 21, 2012 at 12:06 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

>> It's a bit kludgy but works and makes sure the pins are only used
>> for one thing at a time.
>
> No.  The case which I'm thinking of is for the Assabet audio, where
> we have the following situation.

OK... (thanks for the explanation!)

> The solution is to wait for SSP to finish, and while SSP is still enabled,
> switch the pins from the SSP block to the GPIO block and toggle the SFRM
> signal to flip the WS output before removing power.  When starting up,
> with the pins under control of GPIO, power up and them toggle hte SFRM
> signal to restore the WS state before giving it back to the SSP block.
>
> However, here's the thing: SSP must be enabled at the point in time when
> the pins are given or taken away from it, otherwise SFRM is indeterminant.
> We can't allow gpio_request() to fail at the points where we toggle this
> pin - failure is not really an option where causing hardware stress is
> involved.  We can't allow the GPIO block to have the state of these pins
> change state while the pins are given back to the GPIO block before the
> GPIOs have been requested (glitches on the SFRM line will cause the state
> of the FIFO to change.)

This can probably be done, mainly I'd say that control over the pins
need not mean that they are decoupled from some specific hardware, or
that the hardware is disabled or so, the semantics of doing say
pinctrl_get/enable/disable/put aren't set in stone, it's just callbacks into
the device driver so it could avoid doing anything in this case, keep
the pins muxed for the SSP if the transition goes from SSP->GPIO
and back.

However I guess it would be conceptually closer if say the pins could
be used for a device and GPIO at the *same time*.

I had this similar example where a pin on U300 could be in GPIO mode
and "spy" on the peripheral using that pin, by just sampling the value
or triggering on edge transitions.

Since each pin has a pin descriptor, we could solve this by marking
pins as "gpio dualmode", i.e. add some bool dualmode to the
descriptor and augment the core to allow pins with this property to be in
GPIO and "for device X" mode at the same time. So in
pin_request() in pinmux.c we check for this property and allow
dual-mode on a case-by-case basis (assuming this will be pretty rare,
so the default is not to use it).

Of course it assumes the SA1100 being converted to use pin control,
I looked at it a bit and it seems simple enough since the GAFR
register is a single "GPIO or something else"-switch for the GPIOs.
(It'd probably need the SA1100 to be a bit more strict in using
gpiolib in place for the direct assignments though, else the
abstractions get a bit pointless anyway.)

> Another case where this kind of run-time switching needs to occur is the
> PXA IrDA driver, where the pins need to be switched between their FIR and
> SIR modes.

This sounds more mutually exclusive and should be possible to support
with a get/enable/disable/put sequence AFAICT.

Yours,
Linus Walleij

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Russell King - ARM Linux]

On Tue, Feb 21, 2012 at 01:40:05PM +0100, Linus Walleij wrote:
> Of course it assumes the SA1100 being converted to use pin control,
> I looked at it a bit and it seems simple enough since the GAFR
> register is a single "GPIO or something else"-switch for the GPIOs.
> (It'd probably need the SA1100 to be a bit more strict in using
> gpiolib in place for the direct assignments though, else the
> abstractions get a bit pointless anyway.)

That's mostly happened through my recent set of 100 or so patches.
There's a few areas where that's not quite as easy as it should be,
but on the whole, it's mostly complete.

The other thing I forgot to mention, and I suspect it's particular to
SA11x0, is that the GPDR must be set correctly according to the special
function as well as GAFR.  So, if a special function involves driving
a pin, the pin must be set as an output in GPDR.  Conversely, if the
special function involves input only, the pin must be set as an input
in GPDR.

So, on SA11x0, gpio and pin configuration are intimately linked.

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Russell King - ARM Linux]

On Tue, Feb 21, 2012 at 12:44:09PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 21, 2012 at 01:40:05PM +0100, Linus Walleij wrote:
> > Of course it assumes the SA1100 being converted to use pin control,
> > I looked at it a bit and it seems simple enough since the GAFR
> > register is a single "GPIO or something else"-switch for the GPIOs.
> > (It'd probably need the SA1100 to be a bit more strict in using
> > gpiolib in place for the direct assignments though, else the
> > abstractions get a bit pointless anyway.)
> 
> That's mostly happened through my recent set of 100 or so patches.
> There's a few areas where that's not quite as easy as it should be,
> but on the whole, it's mostly complete.
> 
> The other thing I forgot to mention, and I suspect it's particular to
> SA11x0, is that the GPDR must be set correctly according to the special
> function as well as GAFR.  So, if a special function involves driving
> a pin, the pin must be set as an output in GPDR.  Conversely, if the
> special function involves input only, the pin must be set as an input
> in GPDR.
> 
> So, on SA11x0, gpio and pin configuration are intimately linked.

I should have added - the only places which directly accesses one of the
GPDR/GPSR/GPCR registers are:

drivers/pcmcia/sa1100_shannon.c:        unsigned long levels = GPLR;
drivers/video/sa1100fb.c:           GPDR |= mask;
drivers/input/touchscreen/jornada720_ts.c:      if (GPLR & GPIO_GPIO(9)) {
drivers/input/touchscreen/h3600_ts_input.c:     int down = (GPLR & GPIO_BITSY_ACTION_BUTTON) ? 0 : 1;
drivers/input/touchscreen/h3600_ts_input.c:     int down = (GPLR & GPIO_BITSY_NPOWER_BUTTON) ? 0 : 1;

The shannon thing looks like a bug in my PCMCIA patch series - as soc_common
now deals with GPIOs itself (which I've now fixed.)

The sa1100fb thing is a case of what I described above (correctly
configuring the direction for the pins for the special function in use.)

The touchscreen stuff needs someone who knows that stuff to fix it -
I think the jornada folk have been around recently so maybe they can
look at their driver.

The h3600 ts stuff also looks fairly easy to convert to gpiolib if
someone has the time.  Again, maybe if there's an interested party
with a device that they could test, it could happen.

All other cases of direct GPDR/GPLR/GPSR/GPCR access are in platform
initialization code in arch/arm/mach-sa1100.  So, we're actually very
close to having sa11x0 fully converted to gpiolib.

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Linus Walleij]

On Tue, Feb 21, 2012 at 1:44 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

>> (It'd probably need the SA1100 to be a bit more strict in using
>> gpiolib in place for the direct assignments though, else the
>> abstractions get a bit pointless anyway.)
>
> That's mostly happened through my recent set of 100 or so patches.
> There's a few areas where that's not quite as easy as it should be,
> but on the whole, it's mostly complete.

Excellent!

> The other thing I forgot to mention, and I suspect it's particular to
> SA11x0, is that the GPDR must be set correctly according to the special
> function as well as GAFR.  So, if a special function involves driving
> a pin, the pin must be set as an output in GPDR.  Conversely, if the
> special function involves input only, the pin must be set as an input
> in GPDR.
>
> So, on SA11x0, gpio and pin configuration are intimately linked.

It's quite common I think, many platforms have an intimate
relation between GPIO and muxes/altfunctions. For example
it is common that the hardware engineer doesn't helpfully
enable on-die pull-ups on the I2C bus even though the I2C
block is muxed in, you have to go in and set the pull-up bits
separately from muxing the I2C in...

Basically it's expected from a generic pad I/O cell being
arrayed into a GPIO block to expose these things in the same
set of registers.

I made some presentation last week partly describing how
some hardware engineers I know go about creating GPIO
controllers from simpler I/O pad cells:
http://www.df.lth.se/~triad/papers/pincontrol.pdf

Yours,
Linus Walleij

RE: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Stephen Warren]

Linus Walleij wrote at Tuesday, February 21, 2012 3:42 AM:
> On Mon, Feb 20, 2012 at 8:39 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Feb 19, 2012 at 11:27:42PM -0700, Stephen Warren wrote:
> >> Update gpio.txt based on recent discussions regarding interaction with the
> >> pinctrl subsystem.
> >>
> >> Previously, gpio_request() was described as explicitly not performing any
> >> required mux setup operations etc.
> >>
> >> Now, gpio_request() is explicitly as explicitly performing any required mux
> >> setup operations where possible. In the case it isn't, platform code is
> >> required to have set up any required muxing or other configuration prior to
> >> gpio_request() being called, in order to maintain the same semantics.
> >
> > So what if you need to have the pin as a GPIO, manipulate it as a GPIO,
> > and then hand it off to a special function, and then take it back as
> > a GPIO before you shut the special function down ?
> 
> I remember this case very well and we designed for it, so it should be handled
> by pin control and GPIO thusly:
> 
> Example: use pins 1,2 as I2C, then convert them to GPIO for a while
> then back again:

The code below doesn't necessarily do exactly what Russell needs...

> // This call looks up a map containing pins 1,2 and reserve them
> p = pinctrl_get(dev, "i2c");
> if (IS_ERR(p))
>   ...
> pinctrl_enable(p);
> pinctrl_disable(p);

Here, the pinctrl core will call the pinctrl driver's pinmux_ops.free()
function. The whole point of this function is to change the pin's mux
selection to something that is guaranteed not to conflict with any other
pin's mux setting.

Now, if the HW only allows each signal to be routed to a specific pin
(or not routed), then free() might be a no-op.

However, if the HW allows the I2C module's signals to be routed to pins
A+B or X+Y, then free() on pins A/B would need to reprogram the mux to
route something else to pins A+B (or disable those pins or whatever the
HW needs) so that if I2C was later selected on pins X+Y, there would be
no conflict; it wouldn't be the case that both pins A+B and X+Y had I2C
mux'd on to them.

Hence, in general at least, pinctrl_disable() might glitch the output
signals.

As far as how to fix this; see my future responses to your future emails
:-)

> // This will free up the pins again
> pinctrl_put(p);
> // So now we can do this...
> // NB: the GPIO driver calls pinctr_request_gpio() to check
> // that it can take these pins
> gpio_request(1, "gpio1"):
> gpio_request(2, "gpio2");
> // This will trigger a reset or something
> gpio_direction_output(1, 1);
> gpio_direction_output(2, 1);
> // Release pins again
> gpio_free(1);
> gpio_free(2);
> // Take them back for this function
> p = pinctrl_get(dev, "i2c");
...

-- 
nvpublic

RE: [PATCH 2/2] gpio: tegra: Delete tegra_gpio_enable/disable()

[Stephen Warren]

Linus Walleij wrote at Tuesday, February 21, 2012 3:45 AM:
> On Mon, Feb 20, 2012 at 7:27 AM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > Recent pinctrl discussions concluded that gpio_request() and other gpiolib
> > APIs should in fact do whatever is required to mux a GPIO onto pins, by
> > calling pinctrl APIs if required. This change implements this for the
> > Tegra GPIO driver, and removes the old Tegra-specific APIs which did this.
> >
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> > ---
> > I don't believe this depends on other patches.
> >
> > There may be some slight context dependencies; this patch was generated
> > against next-20120217, with my recently posted "gpio: tegra: Fix build
> > issue due to irq_domain rework." applied on top.
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Since it mainly touches mach-tegra stuff I think it's best to take
> this through the Tegra tree, Olof can you queue it?

That makes sense to me, but just to be clear for Olof, best to defer
applying this until the discussion for patch 1 in the series is resolved;
this patch might need rework depending on the decisions there.

-- 
nvpublic

RE: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Stephen Warren]

Linus Walleij wrote at Tuesday, February 21, 2012 5:40 AM:
> On Tue, Feb 21, 2012 at 12:06 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
> >> It's a bit kludgy but works and makes sure the pins are only used
> >> for one thing at a time.
> >
> > No.  The case which I'm thinking of is for the Assabet audio, where
> > we have the following situation.
> 
> OK... (thanks for the explanation!)
> 
> > The solution is to wait for SSP to finish, and while SSP is still enabled,
> > switch the pins from the SSP block to the GPIO block and toggle the SFRM
> > signal to flip the WS output before removing power.  When starting up,
> > with the pins under control of GPIO, power up and them toggle hte SFRM
> > signal to restore the WS state before giving it back to the SSP block.
> >
> > However, here's the thing: SSP must be enabled at the point in time when
> > the pins are given or taken away from it, otherwise SFRM is indeterminant.
> > We can't allow gpio_request() to fail at the points where we toggle this
> > pin - failure is not really an option where causing hardware stress is
> > involved.  We can't allow the GPIO block to have the state of these pins
> > change state while the pins are given back to the GPIO block before the
> > GPIOs have been requested (glitches on the SFRM line will cause the state
> > of the FIFO to change.)
> 
> This can probably be done, mainly I'd say that control over the pins
> need not mean that they are decoupled from some specific hardware, or
> that the hardware is disabled or so, the semantics of doing say
> pinctrl_get/enable/disable/put aren't set in stone, it's just callbacks into
> the device driver so it could avoid doing anything in this case, keep
> the pins muxed for the SSP if the transition goes from SSP->GPIO
> and back.
> 
> However I guess it would be conceptually closer if say the pins could
> be used for a device and GPIO at the *same time*.

If we allowed that, it would also solve the following comment in patch
2 in this series, in the Tegra GPIO driver:

int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
{
	/*
	 * We should call pinctrl_request_gpio() here. However, we can't. Tegra
	 * muxes pins in groups not individually, and real boards exist where
	 * most of a pin group needs to be used for some function, yet some of
	 * the pins aren't actually used by that function in the mode the
	 * controller operates on the board, and so those pins can be used as
	 * GPIOs.
	 *
	 * This is true in practice on Seaboard/Springbank, where pin group ATC
	 * is used by the NAND controller, but pin GMI_IORDY_PI5 is used as a
	 * GPIO for the SD card slot's CD signal.
	 *
	 * On Tegra30, pins are muxed in groups and hence we could call
	 * pinctrl here, but we don't for now.
	 */
	tegra_gpio_mask_write(GPIO_MSK_CNF(offset), offset, 1);

	return 0;
}

i.e. we actually /could/ call pinctrl_request_gpio() here.

And in turn, the I2C driver (in your example) and SPI driver (in Russell's)
would never have to call pinctrl_disable() and hence the issue I raised
in my previous email would not occur.

The one downside here:

Ignoring WARs like we're discussing, it's typically true that a given pin
should either be a special function or a GPIO for any given board. If
we do allow a pin to be owned/used by both, then how do we indicate, on
a per-board rather than per-SoC basis, which pins we should allow both
gpio_request() and pinmux usage? The following considerations exist:

a) On Tegra, a pin group might include 10 pins that are mux'd as a group,
hence all owned e.g. by a NAND driver. If a few of those aren't used on a
particular board due to the way the NAND is hooked up and the driver
configured, do we only allow gpio_request() only on those pins we know the
NAND driver isn't actually using, to prevent someone using unexpected
pins as GPIO? We'd need a per-GPIO per-board way to represent this if
we care about this level of error-checking.

b) In Linus's snooping example, how do we know the SoC can physically
implement enabling a pin as both a GPIO and a special function, such
that the gpio_request() for the snooping won't interfere with the mux'd
function? On Tegra, any GPIO usage is mutually exclusive with mux usage;
enabling GPIO even for input, disconnects the input side of the pin from
any HW module other than GPIO irrespective of mux function. IIRC,
somewhere some per-pin data was mentioned for this.

We could either:

1) Just ignore these issues, and allow both mux and GPIO ownership at
once of any pin, and leave it up to platform data or device tree authors
to give the correct GPIO IDs to drivers. This is possibly reasonable, but
I just want us to make this a conscious decision.

2) Extend the pinctrl mapping table to explicitly represent GPIO usage.

This is required anyway for HW where there isn't a 1:1 mapping between
GPIO ID and pin ID, e.g. 1 pin could be used for two different GPIO
signals, e.g. 1 from each of 2 different GPIO HW modules, with different
capabilities, or where 1 GPIO could be routed to one of 2 or more pins.

In other words, make "gpio" an explicit mux function, thus allowing
pinctrl_select_state() to directly transition between special function
and GPIO usage, rather than having separate pinctrl and GPIO APIs for
this. This way would entail dropping my Documentation/gpio.txt patch
completely, I think.

If we do this though, I think we'd want to extend pinctrl to allow muxing
on pins too, so that we don't need to create a pin group for every pin to
support per-pin GPIO ownership. This would also help out all the HW that
already muxes everything per-pin, since they need to create these single-
pin groups right now... I somewhat anticpated this in my naming of the
mapping table convenience macros in the most recent pinctrl patch I posted
to add pin config mapping table entries.

Thoughts?

-- 
nvpublic

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Linus Walleij]

On Tue, Feb 21, 2012 at 8:14 PM, Stephen Warren <swarren@nvidia.com> wrote:

> Ignoring WARs like we're discussing, it's typically true that a given pin
> should either be a special function or a GPIO for any given board. If
> we do allow a pin to be owned/used by both, then how do we indicate, on
> a per-board rather than per-SoC basis, which pins we should allow both
> gpio_request() and pinmux usage?

I was thinking about making this a property of the physical pin i.e.
struct pin_desc and not of the board data.

It seems to me like this arbitration has to be very close to the driver,
since not all or even many controllers will support it (AFAICT).

> The following considerations exist:
> a) On Tegra, a pin group might include 10 pins that are mux'd as a group,
> hence all owned e.g. by a NAND driver. If a few of those aren't used on a
> particular board due to the way the NAND is hooked up and the driver
> configured, do we only allow gpio_request() only on those pins we know the
> NAND driver isn't actually using, to prevent someone using unexpected
> pins as GPIO? We'd need a per-GPIO per-board way to represent this if
> we care about this level of error-checking.

In this case I would strive not to present unused pins to the functions
in the first place. But maybe this collides with the new paradigm to
assign aquire all possible pins on pinctrl_get() time?

> b) In Linus's snooping example, how do we know the SoC can physically
> implement enabling a pin as both a GPIO and a special function, such
> that the gpio_request() for the snooping won't interfere with the mux'd
> function?

I think on U300 and SA1100 we can flag that in the pin descriptor
as described above.

> 2) Extend the pinctrl mapping table to explicitly represent GPIO usage.

Seems like it could get complicated. Since the only hardwares we have
that can do this would be fine with having it in their pin descriptor we
can do it there?

> This is required anyway for HW where there isn't a 1:1 mapping between
> GPIO ID and pin ID, e.g. 1 pin could be used for two different GPIO
> signals, e.g. 1 from each of 2 different GPIO HW modules, with different
> capabilities, or where 1 GPIO could be routed to one of 2 or more pins.
>
> In other words, make "gpio" an explicit mux function, thus allowing
> pinctrl_select_state() to directly transition between special function
> and GPIO usage, rather than having separate pinctrl and GPIO APIs for
> this. This way would entail dropping my Documentation/gpio.txt patch
> completely, I think.
>
> If we do this though, I think we'd want to extend pinctrl to allow muxing
> on pins too, so that we don't need to create a pin group for every pin to
> support per-pin GPIO ownership. This would also help out all the HW that
> already muxes everything per-pin, since they need to create these single-
> pin groups right now... I somewhat anticpated this in my naming of the
> mapping table convenience macros in the most recent pinctrl patch I posted
> to add pin config mapping table entries.

Hm I think Tony also mentioned that he wanted to allow muxing of
single pins from the mapping sometime so I sort of like the idea.

Linus Walleij

RE: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Stephen Warren]

Linus Walleij wrote at Tuesday, February 21, 2012 11:01 PM:
> On Tue, Feb 21, 2012 at 8:14 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Ignoring WARs like we're discussing, it's typically true that a given pin
> > should either be a special function or a GPIO for any given board. If
> > we do allow a pin to be owned/used by both, then how do we indicate, on
> > a per-board rather than per-SoC basis, which pins we should allow both
> > gpio_request() and pinmux usage?
> 
> I was thinking about making this a property of the physical pin i.e.
> struct pin_desc and not of the board data.
> 
> It seems to me like this arbitration has to be very close to the driver,
> since not all or even many controllers will support it (AFAICT).

Well, there are two aspects:

a) Can the pin controller do it, which is something per-SoC/driver?
b) Does it make sense for the board, given what each pin is connected to?

So unless we decide to ignore (b) (which may be perfectly fine but as I
mentioned I'd just like to explicitly decide this), there needs to be some
per-SoC way of representing this capability, /and/ some per-board way.

> > The following considerations exist:
> > a) On Tegra, a pin group might include 10 pins that are mux'd as a group,
> > hence all owned e.g. by a NAND driver. If a few of those aren't used on a
> > particular board due to the way the NAND is hooked up and the driver
> > configured, do we only allow gpio_request() only on those pins we know the
> > NAND driver isn't actually using, to prevent someone using unexpected
> > pins as GPIO? We'd need a per-GPIO per-board way to represent this if
> > we care about this level of error-checking.
> 
> In this case I would strive not to present unused pins to the functions
> in the first place. But maybe this collides with the new paradigm to
> assign aquire all possible pins on pinctrl_get() time?

The pins are part of the pin group in HW. It doesn't make sense to say
that they aren't, and indeed the SoC driver has no board knowledge and
couldn't exclude them anyway.

-- 
nvpublic

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Grant Likely]

On Tue, 21 Feb 2012 11:46:03 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Feb 20, 2012 at 7:27 AM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > Update gpio.txt based on recent discussions regarding interaction with the
> > pinctrl subsystem.
> >
> > Previously, gpio_request() was described as explicitly not performing any
> > required mux setup operations etc.
> >
> > Now, gpio_request() is explicitly as explicitly performing any required mux
> > setup operations where possible. In the case it isn't, platform code is
> > required to have set up any required muxing or other configuration prior to
> > gpio_request() being called, in order to maintain the same semantics.
> >
> > This is achieved by gpiolib drivers calling e.g. pinctrl_request_gpio() in
> > their .request() operation.
> >
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Grant can you take this one? I'd prefer for you to have a look at
> it as well.

I've taken this one, but left the 2nd for Olof.

g.

Re: [PATCH 1/2] Documentation/gpio.txt: Explain expected pinctrl interaction

[Stephen Warren]

On 03/12/2012 10:32 AM, Grant Likely wrote:
> On Tue, 21 Feb 2012 11:46:03 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Feb 20, 2012 at 7:27 AM, Stephen Warren <swarren@nvidia.com> wrote:
>>
>>> Update gpio.txt based on recent discussions regarding interaction with the
>>> pinctrl subsystem.
>>>
>>> Previously, gpio_request() was described as explicitly not performing any
>>> required mux setup operations etc.
>>>
>>> Now, gpio_request() is explicitly as explicitly performing any required mux
>>> setup operations where possible. In the case it isn't, platform code is
>>> required to have set up any required muxing or other configuration prior to
>>> gpio_request() being called, in order to maintain the same semantics.
>>>
>>> This is achieved by gpiolib drivers calling e.g. pinctrl_request_gpio() in
>>> their .request() operation.
>>>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Grant can you take this one? I'd prefer for you to have a look at
>> it as well.
> 
> I've taken this one, but left the 2nd for Olof.

Grant, did you take V2 of this patch? I assume not since you replied to V1.

For reference, that's:
http://lkml.org/lkml/2012/3/5/533

Thanks.