pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

Add simple pinctrl driver using device tree data.

Currently this driver only works on omap2+ series of processors, where there is either an 8 or 16-bit padconf register for each pin. Support for other similar pinmux controllers could be added.

Note that this patch does not yet support pinconf_ops or GPIO. Further.

Signed-off-by Tony Lindgren <[email protected]>

---
Here's this one finally updated to use the common pinctrl bindings. That sure cleaned up a bunch of the nasty things in this driver :) Nice job Stephen!
---

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt new file +125 -0

new file mode 100644
index 0000000..7b32263
@@ -0,0 +1,125 @@
+Generic simple device tree based pinctrl driver
+
+Required properties:
+- compatible :  one of:
+	- "pinctrl-simple"
+	- "ti,omap2420-padconf"
+	- "ti,omap2430-padconf"
+	- "ti,omap3-padconf"
+	- "ti,omap4-padconf"
+- reg : offset and length of the register set for the mux registers
+- #pinctrl-cells : width of the pinctrl array, currently only 2 is supported
+- pinctrl-simple,register-width : pinmux register access width
+- pinctrl-simple,function-mask : mask of allowed pinmux function bits
+- pinctrl-simple,function-off : function off mode for disabled state
+- pinctrl-simple,pinconf-mask : mask of allowed pinconf bits
+
+This driver uses the common pinctrl bindings as specified in
+pinctrl-bindings.txt document in this directory. The common bindings are used
+to specify the client device states using pinctrl-0 and pinctrl-names entries.
+
+This driver supports parsing one or more pinctrl functions as the subnodes of
+the pinctrl driver entry. One or more registers can be specified for each
+function, see uart2 and uart3 examples below. If you are concerned about boot
+time, parsing multiple registers in a single function is slightly faster.
+
+For setting all static board specific pins, see the pinmux_board_pins example
+below. If you are concerned about the boot time, set up the static pins in
+the bootloader, and only set up selected pins as device tree entries.
+
+This driver assumes currently that there is one register for each pin. If you
+have some pins with more complicated configuration, you can set up a separate
+hardware specific driver for those pins.
+
+Example:
+
+/* SoC common file, such as omap4.dtsi */
+omap4_pmx_core: pinmux at 4a100040 {
+	compatible = "ti,omap4-padconf";
+	reg = <0x4a100040 0x0196>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	#pinctrl-cells = <2>;
+	pinctrl-simple,register-width = <16>;
+	pinctrl-simple,function-mask = <0x7>;
+	pinctrl-simple,function-off = <0xffffffff>;
+	pinctrl-simple,pinconf-mask = <0xfff8>;
+};
+
+omap4_pmx_wkup: pinmux at 4a31e040 {
+	compatible = "ti,omap4-padconf";
+	reg = <0x4a31e040 0x0038>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	#pinctrl-cells = <2>;
+	pinctrl-simple,register-width = <16>;
+	pinctrl-simple,function-mask = <0x7>;
+	pinctrl-simple,function-off = <0xffffffff>;
+	pinctrl-simple,pinconf-mask = <0xfff8>;
+};
+
+
+/* board specific .dts file, such as omap4-sdp.dts */
+&omap4_pmx_core {
+
+	/*
+	 * map all board specific static pins enabled by the pinctrl driver
+	 * itself during the boot (or just set them up in the bootloader)
+	 */
+	pinctrl-names = "default";
+	pinctrl-0 = <&board_pins>;
+
+	board_pins: pinmux_board_pins {
+		board_static_pins {
+			pinctrl-simple,cells = <
+				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+			>;
+		};
+	};
+
+
+	/* map all uart2 pins as a single function */
+	uart2_pins: pinmux_uart2_pins {
+		uart2_pins {
+			pinctrl-simple,cells = <
+				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+			>;
+		};
+	};
+
+	/* map all uart3 pins as separate functions */
+	uart3_pins: pinmux_uart3_pins {
+		uart3_cts_rctx.uart3_cts_rctx {
+			pinctrl-simple,cells = <0x100 0x118>;	/* OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+                };
+
+		uart3_rts_sd.uart3_rts_sd {
+			pinctrl-simple,cells = <0x102 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+		};
+
+		uart3_rx_irrx.uart3_rx_irrx {
+			pinctrl-simple,cells = <0x104 0x100>;	/* OMAP_PIN_INPUT | OMAP_MUX_MODE0 */
+		};
+
+		uart3_tx_irtx.uart3_tx_irtx {
+			pinctrl-simple,cells = <0x106 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+		};
+	};
+};
+
+&uart2 {
+       pinctrl-names = "default";
+       pinctrl-0 = <&uart2_pins>;
+};
+
+&uart3 {
+       pinctrl-names = "default";
+       pinctrl-0 = <&uart3_pins>;
+};
+

drivers/pinctrl/Kconfig +8 -0

index f73a5ea..df01cc5 100644
@@ -83,6 +83,14 @@ config PINCTRL_COH901
 	  COH 901 335 and COH 901 571/3. They contain 3, 5 or 7
 	  ports of 8 GPIO pins each.
 
+config PINCTRL_SIMPLE
+	tristate "Simple device tree based pinctrl driver"
+	depends on OF
+	select PINMUX
+	select PINCONF
+	help
+	  This selects the device tree based generic pinctrl driver.
+
 endmenu
 
 endif

drivers/pinctrl/Makefile +1 -0

index 8e3c95a..1636faa 100644
@@ -19,3 +19,4 @@ obj-$(CONFIG_PINCTRL_TEGRA20)	+= pinctrl-tegra20.o
 obj-$(CONFIG_PINCTRL_TEGRA30)	+= pinctrl-tegra30.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o
 obj-$(CONFIG_PINCTRL_COH901)	+= pinctrl-coh901.o
+obj-$(CONFIG_PINCTRL_SIMPLE)	+= pinctrl-simple.o

drivers/pinctrl/pinctrl-simple.c new file +1027 -0

new file mode 100644
index 0000000..9d24109
@@ -0,0 +1,1027 @@
+/*
+ * Generic simple device tree based pinctrl driver
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/list.h>
+
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+
+#define DRIVER_NAME			"pinctrl-simple"
+#define PCS_MUX_NAME			"pinctrl-simple,cells"
+#define PCS_MUX_CELLS			"#pinctrl-cells"
+#define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
+
+/**
+ * struct pcs_pingroup - pingroups for a function
+ * @np:		pingroup device node pointer
+ * @name:	pingroup name
+ * @gpins:	array of the pins in the group
+ * @ngpins:	number of pins in the group
+ */
+struct pcs_pingroup {
+	struct device_node *np;
+	const char *name;
+	int *gpins;
+	int ngpins;
+	struct list_head node;
+};
+
+/**
+ * struct pcs_func_vals - mux function register offset and value pair
+ * @reg:	register virtual address
+ * @defval:	default value
+ */
+struct pcs_func_vals {
+	void __iomem *reg;
+	unsigned defval;
+};
+
+/**
+ * struct pcs_function - pinctrl function
+ * @name:	pinctrl function name
+ * @vals:	register and vals array
+ * @nvals:	number of entries in vals array
+ * @pgnames:	array of pingroup names the function uses
+ * @npgnames:	number of pingroup names the function uses
+ * @node:	list node
+ */
+struct pcs_function {
+	const char *name;
+	struct pcs_func_vals *vals;
+	unsigned nvals;
+	const char **pgnames;
+	int npgnames;
+	struct list_head node;
+};
+
+/**
+ * struct pcs_data - wrapper for data needed by pinctrl framework
+ * @pa:		pindesc array
+ * @cur:	index to current element
+ */
+struct pcs_data {
+	struct pinctrl_pin_desc *pa;
+	int cur;
+};
+
+/**
+ * struct pcs_name - register name for a pin
+ * @name:	name of the pinctrl register
+ */
+struct pcs_name {
+	char name[PCS_REG_NAME_LEN];
+};
+
+/**
+ * struct pcs_device - mux device instance
+ * @res:	resources
+ * @base:	virtual address of the controller
+ * @size:	size of the ioremapped area
+ * @dev:	device entry
+ * @pctl:	pin controller device
+ * @mutex:	mutex protecting the lists
+ * @width:	bits per mux register
+ * @fmask:	function register mask
+ * @fshift:	function register shift
+ * @foff:	value to turn mux off
+ * @cmask:	pinconf mask
+ * @fmax:	max number of functions in fmask
+ * @cells:	width of the mux array
+ * @names:	array of register names for pins
+ * @pins:	physical pins on the SoC
+ * @pgtree:	pingroup index radix tree
+ * @ftree:	function index radix tree
+ * @pingroups:	list of pingroups
+ * @functions:	list of functions
+ * @ngroups:	number of pingroups
+ * @nfuncs:	number of functions
+ * @desc:	pin controller descriptor
+ * @read:	register read function to use
+ * @write:	register write function to use
+ */
+struct pcs_device {
+	struct resource *res;
+	void __iomem *base;
+	unsigned size;
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	struct mutex mutex;
+
+	unsigned width;
+	unsigned fmask;
+	unsigned fshift;
+	unsigned foff;
+	unsigned cmask;
+	unsigned fmax;
+	unsigned cells;
+
+	struct pcs_name *names;
+	struct pcs_data pins;
+	struct radix_tree_root pgtree;
+	struct radix_tree_root ftree;
+
+	struct list_head pingroups;
+	struct list_head functions;
+
+	unsigned ngroups;
+	unsigned nfuncs;
+
+	struct pinctrl_desc *desc;
+
+	unsigned (*read)(void __iomem *reg);
+	void (*write)(unsigned val, void __iomem *reg);
+};
+
+static unsigned __maybe_unused pcs_readb(void __iomem *reg)
+{
+	return readb(reg);
+}
+
+static unsigned __maybe_unused pcs_readw(void __iomem *reg)
+{
+	return readw(reg);
+}
+
+static unsigned __maybe_unused pcs_readl(void __iomem *reg)
+{
+	return readl(reg);
+}
+
+static void __maybe_unused pcs_writeb(unsigned val, void __iomem *reg)
+{
+	writeb(val, reg);
+}
+
+static void __maybe_unused pcs_writew(unsigned val, void __iomem *reg)
+{
+	writew(val, reg);
+}
+
+static void __maybe_unused pcs_writel(unsigned val, void __iomem *reg)
+{
+	writel(val, reg);
+}
+
+static int pcs_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct pcs_device *pcs;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+
+	return pcs->ngroups;
+}
+
+static const char *pcs_get_group_name(struct pinctrl_dev *pctldev,
+					unsigned gselector)
+{
+	struct pcs_device *pcs;
+	struct pcs_pingroup *group;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	group = radix_tree_lookup(&pcs->pgtree, gselector);
+	if (!group) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, gselector);
+		return NULL;
+	}
+
+	return group->name;
+}
+
+static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
+					unsigned gselector,
+					const unsigned **pins,
+					unsigned *npins)
+{
+	struct pcs_device *pcs;
+	struct pcs_pingroup *group;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	group = radix_tree_lookup(&pcs->pgtree, gselector);
+	if (!group) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, gselector);
+		return -EINVAL;
+	}
+
+	*pins = group->gpins;
+	*npins = group->ngpins;
+
+	return 0;
+}
+
+static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
+					struct seq_file *s,
+					unsigned offset)
+{
+	seq_printf(s, " " DRIVER_NAME);
+}
+
+static void pcs_dt_free_map(struct pinctrl_dev *pctldev,
+				struct pinctrl_map *map, unsigned num_maps)
+{
+	struct pcs_device *pcs;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	devm_kfree(pcs->dev, map);
+}
+
+static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
+				struct device_node *np_config,
+				struct pinctrl_map **map, unsigned *num_maps);
+
+static struct pinctrl_ops pcs_pinctrl_ops = {
+	.get_groups_count = pcs_get_groups_count,
+	.get_group_name = pcs_get_group_name,
+	.get_group_pins = pcs_get_group_pins,
+	.pin_dbg_show = pcs_pin_dbg_show,
+	.dt_node_to_map = pcs_dt_node_to_map,
+	.dt_free_map = pcs_dt_free_map,
+};
+
+static int pcs_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	struct pcs_device *pcs;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+
+	return pcs->nfuncs;
+}
+
+static const char *pcs_get_function_name(struct pinctrl_dev *pctldev,
+						unsigned fselector)
+{
+	struct pcs_device *pcs;
+	struct pcs_function *func;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	func = radix_tree_lookup(&pcs->ftree, fselector);
+	if (!func) {
+		dev_err(pcs->dev, "%s could not find function%i\n",
+			__func__, fselector);
+		return NULL;
+	}
+
+	return func->name;
+}
+
+static int pcs_get_function_groups(struct pinctrl_dev *pctldev,
+					unsigned fselector,
+					const char * const **groups,
+					unsigned * const ngroups)
+{
+	struct pcs_device *pcs;
+	struct pcs_function *func;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	func = radix_tree_lookup(&pcs->ftree, fselector);
+	if (!func) {
+		dev_err(pcs->dev, "%s could not find function%i\n",
+			__func__, fselector);
+		return -EINVAL;
+	}
+	*groups = func->pgnames;
+	*ngroups = func->npgnames;
+
+	return 0;
+}
+
+static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector,
+	unsigned group)
+{
+	struct pcs_device *pcs;
+	struct pcs_function *func;
+	int i;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	func = radix_tree_lookup(&pcs->ftree, fselector);
+	if (!func)
+		return -EINVAL;
+
+	dev_dbg(pcs->dev, "enabling function%i %s\n",
+		fselector, func->name);
+
+	for (i = 0; i < func->nvals; i++) {
+		struct pcs_func_vals *vals;
+		unsigned val;
+
+		vals = &func->vals[i];
+		val = pcs->read(vals->reg);
+		val &= ~(pcs->cmask | pcs->fmask);
+		val |= vals->defval;
+		pcs->write(val, vals->reg);
+	}
+
+	return 0;
+}
+
+static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
+					unsigned group)
+{
+	struct pcs_device *pcs;
+	struct pcs_function *func;
+	int i;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	func = radix_tree_lookup(&pcs->ftree, fselector);
+	if (!func) {
+		dev_err(pcs->dev, "%s could not find function%i\n",
+			__func__, fselector);
+		return;
+	}
+
+	/*
+	 * Do not touch modes if off mode is larger than supported
+	 * modes. Some hardware does not have clearly defined off modes.
+	 */
+	if ((pcs->foff << pcs->fshift) > pcs->fshift) {
+		dev_dbg(pcs->dev, "not updating mode for disable\n");
+		return;
+	}
+
+	dev_dbg(pcs->dev, "disabling function%i %s\n",
+		fselector, func->name);
+
+	for (i = 0; i < func->nvals; i++) {
+		struct pcs_func_vals *vals;
+		unsigned val;
+
+		vals = &func->vals[i];
+		val = pcs->read(vals->reg);
+		val &= ~(pcs->cmask | pcs->fmask);
+		val |= pcs->foff << pcs->fshift;
+		pcs->write(val, vals->reg);
+	}
+}
+
+static int pcs_request_gpio(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range, unsigned offset)
+{
+	return -ENOTSUPP;
+}
+
+static struct pinmux_ops pcs_pinmux_ops = {
+	.get_functions_count = pcs_get_functions_count,
+	.get_function_name = pcs_get_function_name,
+	.get_function_groups = pcs_get_function_groups,
+	.enable = pcs_enable,
+	.disable = pcs_disable,
+	.gpio_request_enable = pcs_request_gpio,
+};
+
+static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
+				unsigned pin, unsigned long *config)
+{
+	return -ENOTSUPP;
+}
+
+static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
+				unsigned pin, unsigned long config)
+{
+	return -ENOTSUPP;
+}
+
+static int pcs_pinconf_group_get(struct pinctrl_dev *pctldev,
+				unsigned group, unsigned long *config)
+{
+	return -ENOTSUPP;
+}
+
+static int pcs_pinconf_group_set(struct pinctrl_dev *pctldev,
+				unsigned group, unsigned long config)
+{
+	return -ENOTSUPP;
+}
+
+static void pcs_pinconf_dbg_show(struct pinctrl_dev *pctldev,
+				struct seq_file *s, unsigned offset)
+{
+}
+
+static void pcs_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
+				struct seq_file *s, unsigned selector)
+{
+}
+
+static struct pinconf_ops pcs_pinconf_ops = {
+	.pin_config_get = pcs_pinconf_get,
+	.pin_config_set = pcs_pinconf_set,
+	.pin_config_group_get = pcs_pinconf_group_get,
+	.pin_config_group_set = pcs_pinconf_group_set,
+	.pin_config_dbg_show = pcs_pinconf_dbg_show,
+	.pin_config_group_dbg_show = pcs_pinconf_group_dbg_show,
+};
+
+/**
+ * pcs_add_pin() - add a pin to the static per controller pin array
+ * @pcs: pcs driver instance
+ * @offset: register offset from base
+ */
+static int __devinit pcs_add_pin(struct pcs_device *pcs, unsigned offset)
+{
+	struct pinctrl_pin_desc *pin;
+	struct pcs_name *pn;
+	char *name;
+	int i;
+
+	i = pcs->pins.cur;
+	if (i >= pcs->desc->npins) {
+		dev_err(pcs->dev, "too many pins, max %i\n",
+			pcs->desc->npins);
+		return -ENOMEM;
+	}
+
+	pin = &pcs->pins.pa[i];
+	pn = &pcs->names[i];
+	name = pn->name;
+	sprintf(name, "%lx",
+		(unsigned long)pcs->res->start + offset);
+	pin->name = name;
+	pin->number = i;
+	pcs->pins.cur++;
+
+	return i;
+}
+
+/**
+ * pcs_allocate_pin_table() - adds all the pins for the pinctrl driver
+ * @pcs: pcs driver instance
+ *
+ * In case of errors, resources are freed in pcs_free_resources.
+ *
+ * If your hardware needs holes in the address space, then just set
+ * up multiple driver instances.
+ *
+ * REVISIT: Handle the address space better for multiple registers
+ * per pin type hardware.
+ */
+static int __devinit pcs_allocate_pin_table(struct pcs_device *pcs)
+{
+	int mux_bytes, nr_pins, i;
+
+	mux_bytes = pcs->width / BITS_PER_BYTE;
+	nr_pins = pcs->size / mux_bytes;
+
+	dev_dbg(pcs->dev, "allocating %i pins\n", nr_pins);
+	pcs->pins.pa = devm_kzalloc(pcs->dev,
+				sizeof(*pcs->pins.pa) * nr_pins,
+				GFP_KERNEL);
+	if (!pcs->pins.pa)
+		return -ENOMEM;
+
+	pcs->names = devm_kzalloc(pcs->dev,
+				sizeof(struct pcs_name) * nr_pins,
+				GFP_KERNEL);
+	if (!pcs->names)
+		return -ENOMEM;
+
+	pcs->desc->pins = pcs->pins.pa;
+	pcs->desc->npins = nr_pins;
+
+	for (i = 0; i < pcs->desc->npins; i++) {
+		unsigned offset;
+		int res;
+
+		offset = i * mux_bytes;
+		res = pcs_add_pin(pcs, offset);
+		if (res < 0) {
+			dev_err(pcs->dev, "error adding pins: %i\n", res);
+			return res;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * pcs_add_function() - adds a new function to the function list
+ * @pcs: pcs driver instance
+ * @np: device node of the mux entry
+ * @name: name of the function
+ * @vals: array of mux register value pairs used by the function
+ * @nvals: number of mux register value pairs
+ * @pgnames: array of pingroup names for the function
+ * @npgnames: number of pingroup names
+ */
+static struct pcs_function *pcs_add_function(struct pcs_device *pcs,
+					struct device_node *np,
+					const char *name,
+					struct pcs_func_vals *vals,
+					unsigned nvals,
+					const char **pgnames,
+					unsigned npgnames)
+{
+	struct pcs_function *function;
+
+	function = devm_kzalloc(pcs->dev, sizeof(*function), GFP_KERNEL);
+	if (!function)
+		return NULL;
+
+	function->name = name;
+	function->vals = vals;
+	function->nvals = nvals;
+	function->pgnames = pgnames;
+	function->npgnames = npgnames;
+
+	mutex_lock(&pcs->mutex);
+	list_add_tail(&function->node, &pcs->functions);
+	radix_tree_insert(&pcs->ftree, pcs->nfuncs, function);
+	pcs->nfuncs++;
+	mutex_unlock(&pcs->mutex);
+
+	return function;
+}
+
+static void pcs_remove_function(struct pcs_device *pcs,
+				struct pcs_function *function)
+{
+	int i;
+
+	mutex_lock(&pcs->mutex);
+	for (i = 0; i < pcs->nfuncs; i++) {
+		struct pcs_function *found;
+
+		found = radix_tree_lookup(&pcs->ftree, i);
+		if (found == function)
+			radix_tree_delete(&pcs->ftree, i);
+	}
+	list_del(&function->node);
+	mutex_unlock(&pcs->mutex);
+}
+
+/**
+ * pcs_add_pingroup() - add a pingroup to the pingroup list
+ * @pcs: pcs driver instance
+ * @np: device node of the mux entry
+ * @name: name of the pingroup
+ * @gpins: array of the pins that belong to the group
+ * @ngpins: number of pins in the group
+ */
+static int pcs_add_pingroup(struct pcs_device *pcs,
+					struct device_node *np,
+					const char *name,
+					int *gpins,
+					int ngpins)
+{
+	struct pcs_pingroup *pingroup;
+
+	pingroup = devm_kzalloc(pcs->dev, sizeof(*pingroup), GFP_KERNEL);
+	if (!pingroup)
+		return -ENOMEM;
+
+	pingroup->name = name;
+	pingroup->np = np;
+	pingroup->gpins = gpins;
+	pingroup->ngpins = ngpins;
+
+	mutex_lock(&pcs->mutex);
+	list_add_tail(&pingroup->node, &pcs->pingroups);
+	radix_tree_insert(&pcs->pgtree, pcs->ngroups, pingroup);
+	pcs->ngroups++;
+	mutex_unlock(&pcs->mutex);
+
+	return 0;
+}
+
+/**
+ * pcs_get_pin_by_offset() - get a pin index based on the register offset
+ * @pcs: pcs driver instance
+ * @offset: register offset from the base
+ *
+ * Note that this is OK as long as the pins are in a static array.
+ */
+static int pcs_get_pin_by_offset(struct pcs_device *pcs, unsigned offset)
+{
+	unsigned index;
+
+	if (offset >= pcs->size) {
+		dev_err(pcs->dev, "mux offset out of range: 0x%x (0x%x)\n",
+			offset, pcs->size);
+		return -EINVAL;
+	}
+
+	index = offset / (pcs->width / BITS_PER_BYTE);
+
+	return index;
+}
+
+/**
+ * smux_parse_one_pinctrl_entry() - parses a device tree mux entry
+ * @pcs: pinctrl driver instance
+ * @np: device node of the mux entry
+ * @map: map entry
+ * @pgnames: pingroup names
+ *
+ * Note that this currently supports only #pinctrl-cells = 2.
+ * This could be improved to parse controllers that have additional
+ * auxilary registers per mux.
+ */
+static int __init pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
+						struct device_node *np,
+						struct pinctrl_map **map,
+						const char **pgnames)
+{
+	struct pcs_func_vals *vals;
+	const __be32 *mux;
+	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
+	struct pcs_function *function;
+
+	if (pcs->cells != 2) {
+		dev_err(pcs->dev, "unhandled %s: %i\n", PCS_MUX_CELLS,
+			pcs->cells);
+		return -EINVAL;
+	}
+
+	mux = of_get_property(np, PCS_MUX_NAME, &size);
+	if ((!mux) || (size < sizeof(*mux) * pcs->cells)) {
+		dev_err(pcs->dev, "bad data for mux %s\n",
+			np->name);
+		return -EINVAL;
+	}
+
+	size /= sizeof(*mux);	/* Number of elements in array */
+	rows = size / pcs->cells;
+
+	vals = devm_kzalloc(pcs->dev, sizeof(*vals) * rows, GFP_KERNEL);
+	if (!vals)
+		return -ENOMEM;
+
+	pins = devm_kzalloc(pcs->dev, sizeof(*pins) * rows, GFP_KERNEL);
+	if (!pins)
+		goto free_vals;
+
+	while (index < size) {
+		unsigned offset, defval;
+		int pin;
+
+		offset = be32_to_cpup(mux + index++);
+		defval = be32_to_cpup(mux + index++);
+		vals[found].reg = pcs->base + offset;
+		vals[found].defval = defval;
+
+		pin = pcs_get_pin_by_offset(pcs, offset);
+		if (pin < 0) {
+			dev_err(pcs->dev,
+				"could not add functions for %s %ux\n",
+				np->name, offset);
+			break;
+		}
+		pins[found++] = pin;
+	}
+
+	pgnames[0] = np->name;
+	function = pcs_add_function(pcs, np, np->name, vals, found, pgnames, 1);
+	if (!function)
+		goto free_pins;
+
+	res = pcs_add_pingroup(pcs, np, np->name, pins, found);
+	if (res < 0)
+		goto free_function;
+
+	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
+	(*map)->data.mux.group = np->name;
+	(*map)->data.mux.function = np->name;
+
+	return 0;
+
+free_function:
+	pcs_remove_function(pcs, function);
+
+free_pins:
+	devm_kfree(pcs->dev, pins);
+
+free_vals:
+	devm_kfree(pcs->dev, vals);
+
+	return res;
+}
+/**
+ * pcs_dt_node_to_map() - allocates and parses pinctrl maps
+ * @pctldev: pinctrl instance
+ * @np_config: device tree pinmux entry
+ * @map: array of map entries
+ * @num_maps: number of maps
+ */
+static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
+				struct device_node *np_config,
+				struct pinctrl_map **map, unsigned *num_maps)
+{
+	struct pcs_device *pcs;
+	struct device_node *np;
+	struct pinctrl_map *cur_map;
+	const char **pgnames, **cur_name;
+	int ret, found_maps = 0, added_maps = 0;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+
+	for_each_child_of_node(np_config, np)
+		found_maps++;
+
+	*map = devm_kzalloc(pcs->dev, sizeof(**map) * found_maps,
+				GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	cur_map = *map;
+	*num_maps = 0;
+
+	pgnames = devm_kzalloc(pcs->dev, sizeof(*pgnames) * found_maps,
+				GFP_KERNEL);
+	if (!pgnames) {
+		devm_kfree(pcs->dev, *map);
+		return -ENOMEM;
+	}
+
+	cur_name = pgnames;
+
+	for_each_child_of_node(np_config, np) {
+		ret = pcs_parse_one_pinctrl_entry(pcs, np, &cur_map, cur_name);
+		if (ret < 0) {
+			dev_err(pcs->dev, "added only %i/%i entries for %s\n",
+				added_maps, found_maps, np_config->name);
+			break;
+		}
+		cur_map++;
+		cur_name++;
+		added_maps++;
+	}
+
+	*num_maps = added_maps;
+
+	return 0;
+}
+
+/**
+ * pcs_free_funcs() - free memory used by functions
+ * @pcs: pcs driver instance
+ */
+static void pcs_free_funcs(struct pcs_device *pcs)
+{
+	struct list_head *pos, *tmp;
+	int i;
+
+	mutex_lock(&pcs->mutex);
+	for (i = 0; i < pcs->nfuncs; i++) {
+		struct pcs_function *func;
+
+		func = radix_tree_lookup(&pcs->ftree, i);
+		if (!func)
+			continue;
+		radix_tree_delete(&pcs->ftree, i);
+	}
+	list_for_each_safe(pos, tmp, &pcs->functions) {
+		struct pcs_function *function;
+
+		function = list_entry(pos, struct pcs_function, node);
+		list_del(&function->node);
+	}
+	mutex_unlock(&pcs->mutex);
+}
+
+/**
+ * pcs_free_pingroups() - free memory used by pingroups
+ * @pcs: pcs driver instance
+ */
+static void pcs_free_pingroups(struct pcs_device *pcs)
+{
+	struct list_head *pos, *tmp;
+	int i;
+
+	mutex_lock(&pcs->mutex);
+	for (i = 0; i < pcs->ngroups; i++) {
+		struct pcs_pingroup *pingroup;
+
+		pingroup = radix_tree_lookup(&pcs->pgtree, i);
+		if (!pingroup)
+			continue;
+		radix_tree_delete(&pcs->pgtree, i);
+	}
+	list_for_each_safe(pos, tmp, &pcs->pingroups) {
+		struct pcs_pingroup *pingroup;
+
+		pingroup = list_entry(pos, struct pcs_pingroup, node);
+		list_del(&pingroup->node);
+	}
+	mutex_unlock(&pcs->mutex);
+}
+
+/**
+ * pcs_free_resources() - free memory used by this driver
+ * @pcs: pcs driver instance
+ */
+static void pcs_free_resources(struct pcs_device *pcs)
+{
+	if (pcs->pctl)
+		pinctrl_unregister(pcs->pctl);
+
+	pcs_free_funcs(pcs);
+	pcs_free_pingroups(pcs);
+}
+
+/**
+ * pcs_register() - initializes and registers with pinctrl framework
+ * @pcs: pcs driver instance
+ */
+static int __devinit pcs_register(struct pcs_device *pcs)
+{
+	int ret;
+
+	if (!pcs->dev->of_node)
+		return -ENODEV;
+
+	pcs->desc = devm_kzalloc(pcs->dev, sizeof(*pcs->desc), GFP_KERNEL);
+	if (!pcs->desc)
+		return -ENOMEM;
+	pcs->desc->name = DRIVER_NAME;
+	pcs->desc->pctlops = &pcs_pinctrl_ops;
+	pcs->desc->pmxops = &pcs_pinmux_ops;
+	pcs->desc->confops = &pcs_pinconf_ops;
+	pcs->desc->owner = THIS_MODULE;
+
+	ret = pcs_allocate_pin_table(pcs);
+	if (ret < 0)
+		goto free;
+
+	pcs->pctl = pinctrl_register(pcs->desc, pcs->dev, pcs);
+	if (!pcs->pctl) {
+		dev_err(pcs->dev, "could not register simple pinctrl driver\n");
+		ret = -EINVAL;
+		goto free;
+	}
+
+	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
+		 pcs->desc->npins, pcs->base, pcs->size);
+
+	return 0;
+
+free:
+	pcs_free_resources(pcs);
+
+	return ret;
+}
+
+#define PCS_GET_PROP_U32(name, reg, err)				\
+	do {								\
+		ret = of_property_read_u32(np, name, reg);		\
+		if (ret) {						\
+			dev_err(pcs->dev, err);				\
+			goto out;					\
+		}							\
+	} while (0);
+
+static struct of_device_id pcs_of_match[];
+
+/*
+ * REVISIT: Handle hardware with multiple registers per pin
+ * better by adding other masks.
+ */
+static int __devinit pcs_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
+	struct resource res;
+	struct pcs_device *pcs;
+	int ret;
+
+	match = of_match_device(pcs_of_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+
+	pcs = devm_kzalloc(&pdev->dev, sizeof(*pcs), GFP_KERNEL);
+	if (!pcs) {
+		dev_err(&pdev->dev, "could not allocate\n");
+		return -ENOMEM;
+	}
+	pcs->dev = &pdev->dev;
+	mutex_init(&pcs->mutex);
+	INIT_LIST_HEAD(&pcs->pingroups);
+	INIT_LIST_HEAD(&pcs->functions);
+
+	PCS_GET_PROP_U32("pinctrl-simple,register-width", &pcs->width,
+			 "register width not specified\n");
+
+	PCS_GET_PROP_U32("pinctrl-simple,function-mask", &pcs->fmask,
+			 "function register mask not specified\n");
+	pcs->fshift = ffs(pcs->fmask) - 1;
+	pcs->fmax = pcs->fmask >> pcs->fshift;
+
+	PCS_GET_PROP_U32("pinctrl-simple,function-off", &pcs->foff,
+			 "function off mode not specified\n");
+
+	PCS_GET_PROP_U32("pinctrl-simple,pinconf-mask", &pcs->cmask,
+			 "pinconf mask not specified\n");
+
+	PCS_GET_PROP_U32("#pinctrl-cells", &pcs->cells,
+			 "#pinctrl-cells not specified\n");
+
+	ret = of_address_to_resource(pdev->dev.of_node, 0, &res);
+	if (ret) {
+		dev_err(pcs->dev, "could not get resource\n");
+		goto out;
+	}
+
+	pcs->res = devm_request_mem_region(pcs->dev, res.start,
+			resource_size(&res), DRIVER_NAME);
+	if (!pcs->res) {
+		dev_err(pcs->dev, "could not get mem_region\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	pcs->size = resource_size(pcs->res);
+	pcs->base = devm_ioremap(pcs->dev, pcs->res->start, pcs->size);
+	if (!pcs->base) {
+		dev_err(pcs->dev, "could not ioremap\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	INIT_RADIX_TREE(&pcs->pgtree, GFP_KERNEL);
+	INIT_RADIX_TREE(&pcs->ftree, GFP_KERNEL);
+	platform_set_drvdata(pdev, pcs);
+
+	switch (pcs->width) {
+	case 8:
+		pcs->read = pcs_readb;
+		pcs->write = pcs_writeb;
+		break;
+	case 16:
+		pcs->read = pcs_readw;
+		pcs->write = pcs_writew;
+		break;
+	case 32:
+		pcs->read = pcs_readl;
+		pcs->write = pcs_writel;
+		break;
+	default:
+		break;
+	}
+
+	ret = pcs_register(pcs);
+	if (ret < 0) {
+		dev_err(pcs->dev, "could not add mux registers: %i\n", ret);
+		goto out;
+	}
+
+	return 0;
+
+out:
+	return ret;
+}
+
+static int __devexit pcs_remove(struct platform_device *pdev)
+{
+	struct pcs_device *pcs = platform_get_drvdata(pdev);
+
+	if (!pcs)
+		return 0;
+
+	pcs_free_resources(pcs);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct of_device_id pcs_of_match[] __devinitdata = {
+	{ .compatible = DRIVER_NAME, },
+	{ .compatible = "ti,omap2420-padconf", },
+	{ .compatible = "ti,omap2430-padconf", },
+	{ .compatible = "ti,omap3-padconf", },
+	{ .compatible = "ti,omap4-padconf", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pcs_of_match);
+
+static struct platform_driver pcs_driver = {
+	.probe		= pcs_probe,
+	.remove		= __devexit_p(pcs_remove),
+	.driver = {
+		.owner		= THIS_MODULE,
+		.name		= DRIVER_NAME,
+		.of_match_table	= pcs_of_match,
+	},
+};
+
+module_platform_driver(pcs_driver);
+
+MODULE_AUTHOR("Tony Lindgren <[email protected]>");
+MODULE_DESCRIPTION("Simple device tree pinctrl driver");
+MODULE_LICENSE("GPL");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I really like it

I was working on something simillar

but can we split the group management so we can use it on other bindings

Hi,

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

I really like it

I was working on something simillar

Quoted 2 lines

but can we split the group management so we can use it on other bindings

Hmm I'm not sure I follow on the group management splitting, can you specify what you have in mind here?

If you mean moving more things into pinctrl fwk, then yeah I'd assume that will happen eventually and drivers like this will end up becoming more minimal.

Quoted 8 lines · Tony Lindgren

Hi,

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

I really like it

I was working on something simillar

Quoted 8 lines

Quoted 2 lines

but can we split the group management so we can use it on other bindings

Hmm I'm not sure I follow on the group management splitting, can you specify what you have in mind here?

If you mean moving more things into pinctrl fwk, then yeah I'd assume that will happen eventually and drivers like this will end up becoming more minimal.

Jean-Christophe, forgive me if I'm putting words in your mouth, but I assume the following is what you mean:

There are two pieces of data required by the pinctrl subsystem:

a) The set of pins, functions, and groups that exist.

b) The specific function to select for each pin/group on a given board.

Item (a) can be represented in the pinctrl driver (e.g. as in the Tegra driver), or can be represented in device tree in order to avoid large tables in the driver.

Item (b) has to be represented in device tree, since the whole point is that it's board-specific.

For all DT bindings I've seen that choose to represent (a) in the DT rather than in the driver, the DT represents (b) directly, and (a) is implicitly extracted/created based on (a).

When I was first thinking about DT bindings for pinctrl, I had hoped that even if (a) was represented in DT, the DT nodes/properties for (a) and (b) would be entirely separate, so that the binding for (b) could be completely common across all SoCs, even though the binding for (a) would perhaps be different across SoCs (if it existed at all).

So, perhaps Jean-Christophe is talking about splitting up (a) and (b) in device tree?

Or perhaps Jean-Christophe only refers to the code that creates the group and function definitions from (b), and not the actual DT binding itself?

In my mind in the driver we do not have to care how to list register/unregister the group. We just need to be able to do this

pinctrl_register_group(...)

or

pinctrl_unregistewr_group(...)

On at91 we have this type of controller

one pin can have multiple function and each function can be on different pin and we need to program and represent each of them one by one

And each pin have different parameter

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

and use macro as basicaly we are just this

and this can be applied to tegra too as you will just refer the pin in this hw pin block

Quoted 11 lines · Jean-Christophe PLAGNIOL-VILLARD

In my mind in the driver we do not have to care how to list register/unregister the group. We just need to be able to do this

pinctrl_register_group(...)

or

pinctrl_unregistewr_group(...)

On at91 we have this type of controller

Ah I see. Yeah makes sense. Also I think we should let the pinctrl core eventually manage the pins more too. Right now the pins are a static array in the driver, which makes things unnecessarily complex for the DT case. It would be nice to also have something like pinctrl_register/unregister_pin instead of requiring them all be registered while registering with the framework initially.

But all that can be improved later on once we get the binding down..

Quoted 6 lines

one pin can have multiple function and each function can be on different pin and we need to program and represent each of them one by one

And each pin have different parameter

Quoted 6 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

Quoted 5 lines

and use macro as basicaly we are just this

and this can be applied to tegra too as you will just refer the pin in this hw pin block

I was thinking of adding gpio eventually as a separate attribute with something like the following. Here cam_d10 pin is used as gpio109:

cam_d10.gpio_109 {
	pinctrl-simple,cells = <0xfa 0x104>;	/* OMAP_PIN_INPUT | OMAP_MUX_MODE4 */
	gpio = <&gpio4 13 0>;			/* gpio109 */
};

The reasoning for this is that as we may not care about the gpio number for all pins, it should be optional. Would that work for you?

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

no pin number in the bank because it could not be gpio

	pioD: gpio at fffff800 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffff800 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	pioE: gpio at fffffa00 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffffa00 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	dbgu {
		pins = < &pioB 12 0 0
			 &pioB 13 0 2 >;
	/* with macro */
		pins = < &pioB 12 MUX_A NO_PULL_UP
			 &pioB 13 MUX_A PULL_UP >;
	};

	/* and also the notion of linked group
	 * as on uart of network you have often the same subset of pin use.
	 *
	 * As example on uart rxd/txd is use for the group without rts/cts
	 * and the one with it
	 * on ethernet the RMII pin are use also on MII
	 */

	uart0_rxd_txd {
		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
	};

	uart0_rts_cts {
		groups = < &uart0_rxd_txd >;
		pins = < &pioB 17 MUX_B NO_PULL_UP	/* rts */
			 &pioB 15 MUX_B NO_PULL_UP >;	/* cts */
	};

	uart0_rts_cts_external_pull_up {
		groups = < &uart0_rts_cts >;
		gpios = <&pioC 1 0>;
	};
};

The idea is to avoid duplication the xlate for pins will be driver specific with maybe a common implementation

the 3 or 4 first fix as done on gpio

Quoted 11 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

Quoted 18 lines

	pioD: gpio at fffff800 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffff800 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	pioE: gpio at fffffa00 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffffa00 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

Quoted 7 lines

	dbgu {
		pins = < &pioB 12 0 0
			 &pioB 13 0 2 >;
	/* with macro */
		pins = < &pioB 12 MUX_A NO_PULL_UP
			 &pioB 13 MUX_A PULL_UP >;
	};

I could change to use this too no problem. The only concern I have is that is "&pioB 12" immutable for all gpio controllers?

Grepping the *.dts* files, at least exynos is using the following for gpios:

gpios = <&gpx2 0 0 0 2>;

If we can conclude that phandle to the gpio controller instance and the register offset is always enough here, then I'm OK changing to that format. It would actually save some parsing in most cases.

Quoted 26 lines

 
	/* and also the notion of linked group
	 * as on uart of network you have often the same subset of pin use.
	 *
	 * As example on uart rxd/txd is use for the group without rts/cts
	 * and the one with it
	 * on ethernet the RMII pin are use also on MII
	 */

	uart0_rxd_txd {
		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
	};

	uart0_rts_cts {
		groups = < &uart0_rxd_txd >;
		pins = < &pioB 17 MUX_B NO_PULL_UP	/* rts */
			 &pioB 15 MUX_B NO_PULL_UP >;	/* cts */
	};

	uart0_rts_cts_external_pull_up {
		groups = < &uart0_rts_cts >;
		gpios = <&pioC 1 0>;
	};
};

Quoted 4 lines

The idea is to avoid duplication the xlate for pins will be driver specific with maybe a common implementation

the 3 or 4 first fix as done on gpio

Yeah sounds doable to me, but can probably be added later?

Regarding grouping, basically for most cases it makes sense to have three states: default, active, idle instead of just active and idle. The reason is that for most cases the default pins only need to be set once for each devices. Only few pins need to toggle between active and idle states.

Quoted 7 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 6 lines · Tony Lindgren

Quoted 6 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

}

Hmm I assume the "12" above the gpio number?

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

Quoted 18 lines

	pioD: gpio at fffff800 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffff800 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	pioE: gpio at fffffa00 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffffa00 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

Quoted 7 lines

	dbgu {
		pins = < &pioB 12 0 0
			 &pioB 13 0 2 >;
	/* with macro */
		pins = < &pioB 12 MUX_A NO_PULL_UP
			 &pioB 13 MUX_A PULL_UP >;
	};

I could change to use this too no problem. The only concern I have is that is "&pioB 12" immutable for all gpio controllers?

Grepping the *.dts* files, at least exynos is using the following for gpios:

gpios = <&gpx2 0 0 0 2>;

If we can conclude that phandle to the gpio controller instance and the register offset is always enough here, then I'm OK changing to that format. It would actually save some parsing in most cases.

Quoted 12 lines

	/* and also the notion of linked group
	 * as on uart of network you have often the same subset of pin use.
	 *
	 * As example on uart rxd/txd is use for the group without rts/cts
	 * and the one with it
	 * on ethernet the RMII pin are use also on MII
	 */

	uart0_rxd_txd {
		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
	};

Quoted 15 lines

	uart0_rts_cts {
		groups = < &uart0_rxd_txd >;
		pins = < &pioB 17 MUX_B NO_PULL_UP	/* rts */
			 &pioB 15 MUX_B NO_PULL_UP >;	/* cts */
	};

	uart0_rts_cts_external_pull_up {
		groups = < &uart0_rts_cts >;
		gpios = <&pioC 1 0>;
	};
};

The idea is to avoid duplication the xlate for pins will be driver specific with maybe a common implementation

Quoted 1 line

the 3 or 4 first fix as done on gpio

Yeah sounds doable to me, but can probably be added later?

Regarding grouping, basically for most cases it makes sense to have three states: default, active, idle instead of just active and idle.

Quoted 11 lines · Tony Lindgren

Quoted 10 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

Quoted 4 lines

Quoted 1 line

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

I'd prefer to avoid any references to GPIOs here; not all muxable pins are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts independent.

Quoted 17 lines · Stephen Warren

Quoted 15 lines · Tony Lindgren

Quoted 11 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

Quoted 3 lines

I'd prefer to avoid any references to GPIOs here; not all muxable pins are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts independent.

And it seems that &pioA 12 is not always enough information for the pinctrl driver to request a GPIO. So it's best to specify it separately.

Quoted 18 lines · Tony Lindgren

Quoted 17 lines · Stephen Warren

Quoted 15 lines · Tony Lindgren

Quoted 11 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

Quoted 6 lines

Quoted 3 lines

I'd prefer to avoid any references to GPIOs here; not all muxable pins are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts independent.

And it seems that &pioA 12 is not always enough information for the pinctrl driver to request a GPIO. So it's best to specify it separately.

Why would a pinctrl driver "request a GPIO"?

Quoted 23 lines · Stephen Warren

Quoted 22 lines · Tony Lindgren

Quoted 20 lines · Stephen Warren

Quoted 8 lines · Tony Lindgren

Quoted 7 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 6 lines · Tony Lindgren

Quoted 6 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

}

Hmm I assume the "12" above the gpio number?

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

I'd prefer to avoid any references to GPIOs here; not all muxable pins are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts independent.

Quoted 4 lines

Quoted 2 lines

And it seems that &pioA 12 is not always enough information for the pinctrl driver to request a GPIO. So it's best to specify it separately.

Why would a pinctrl driver "request a GPIO"?

Hmm what would pinctrl_request_gpio do if the GPIO driver is separate driver?

Quoted 27 lines · Tony Lindgren

Quoted 26 lines · Stephen Warren

Quoted 24 lines · Tony Lindgren

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Tony Lindgren

Quoted 10 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

I'd prefer to avoid any references to GPIOs here; not all muxable pins are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts independent.

And it seems that &pioA 12 is not always enough information for the pinctrl driver to request a GPIO. So it's best to specify it separately.

Quoted 3 lines

Quoted 1 line

Why would a pinctrl driver "request a GPIO"?

Hmm what would pinctrl_request_gpio do if the GPIO driver is separate driver?

Well, that's a GPIO driver requesting a GPIO from the pinctrl system, rather than the pinctrl driver requesting a GPIO (sorry to be picky).

It wasn't at all obvious to me from your binding proposal that you intended the pinctrl-simple driver to support the GPIO operations at all. If you do want this, I think you'd need some properties (perhaps some kind of explicit table) in order to set up the GPIO ID -> pinctrl pin ID mapping. I don't recall seeing those; did I just miss them? I think we'd want this to be explicit because:

a) It may well be the case that not all users of pinctrl-simple actually mux/control GPIOs at all. It's certainly possible to only mux "special functions", and have dedicated pins for a GPIO controller.

b) Even when GPIOs do come into the picture, it may be that only some of the pins are available as GPIOs.

Also, were you intending pinctrl-simple to actually be the GPIO controller itself? That'd be another case that one might consider fairly simple, but then extends to being gpio-simple as well as pinctrl-simple...

Quoted 28 lines · Stephen Warren

Quoted 27 lines · Tony Lindgren

Quoted 26 lines · Stephen Warren

Quoted 24 lines · Tony Lindgren

Quoted 12 lines · Stephen Warren

Quoted 11 lines · Tony Lindgren

Quoted 10 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

I'd prefer to avoid any references to GPIOs here; not all muxable pins are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts independent.

And it seems that &pioA 12 is not always enough information for the pinctrl driver to request a GPIO. So it's best to specify it separately.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Why would a pinctrl driver "request a GPIO"?

Hmm what would pinctrl_request_gpio do if the GPIO driver is separate driver?

Well, that's a GPIO driver requesting a GPIO from the pinctrl system, rather than the pinctrl driver requesting a GPIO (sorry to be picky).

Oh sorry maybe I misunderstood what pinctrl_request_gpio is doing. Seems like that should work the same way from binding point of view.

Quoted 7 lines

It wasn't at all obvious to me from your binding proposal that you intended the pinctrl-simple driver to support the GPIO operations at all. If you do want this, I think you'd need some properties (perhaps some kind of explicit table) in order to set up the GPIO ID -> pinctrl pin ID mapping. I don't recall seeing those; did I just miss them? I think we'd want this to be explicit because:

Quoted 6 lines

a) It may well be the case that not all users of pinctrl-simple actually mux/control GPIOs at all. It's certainly possible to only mux "special functions", and have dedicated pins for a GPIO controller.

b) Even when GPIOs do come into the picture, it may be that only some of the pins are available as GPIOs.

Right, we need some pinctrl to gpio mapping eventually. That comes automatically from DT for the pins and gpios we care about.

And that's why the binding needs to be flexible enough so we can add optional pin specific options as needed in addition to parsing a larger set of pins with no options.

The mapping of GPIO to pinctrl pins would presumably be driven solely by the HW design of the pin controller and GPIO, and not by the mux selection in the pin controller (otherwise, I'd argue this isn't a simple case that should be handled by pinctrl-simple).

As such, I'd expect some properties/table at the top-level of the pin controller object to describe the GPIO mapping. In turn, that implies that the individual per-pin mux-selection/configuration nodes don't need to describe any GPIO-related information.

Quoted 10 lines · Stephen Warren

The mapping of GPIO to pinctrl pins would presumably be driven solely by the HW design of the pin controller and GPIO, and not by the mux selection in the pin controller (otherwise, I'd argue this isn't a simple case that should be handled by pinctrl-simple).

As such, I'd expect some properties/table at the top-level of the pin controller object to describe the GPIO mapping. In turn, that implies that the individual per-pin mux-selection/configuration nodes don't need to describe any GPIO-related information.

Yes good point. I agree it's a HW design issue, and could be in the properties for the pin controller object.

Just to summarize, the things to consider with the GPIO to mux mapping are:

1. Having this table as static data in the driver is is not a nice solution as it seems that we'd currently need six mapping tables for omap2+ alone.

2. This table is not needed for most of the (hundreds of) pins, it's only needed for a few selected pins, let's say ten or so on an average device. So there's no need to stuff the kernel with information about the unused GPIO pins.

It seems that the conclusion here is that we don't need to worry about GPIOs in the pinctrl-simple binding for now, and it can be added later without having to change the basic binding.

Quoted 5 lines · I wrote:

From a binding perspective, I don't see why you'd want to allow two cases:

1) One node with multiple entries in pinctrl-simple,cells
2) Multiple nodes each with a single entry in pinctrl-simple,cells

Quoted 1 line

Why not only allow (1)?

Because we need to specify GPIO for some pins. There may be additional flags too, we do have external DMA request lines for few pins available.. I'm not saying pinctrl fwk should know about that, but it's a similar mapping of pins to GPIO lines.

Quoted 21 lines · Tony Lindgren

Quoted 10 lines · Stephen Warren

The mapping of GPIO to pinctrl pins would presumably be driven solely by the HW design of the pin controller and GPIO, and not by the mux selection in the pin controller (otherwise, I'd argue this isn't a simple case that should be handled by pinctrl-simple).

As such, I'd expect some properties/table at the top-level of the pin controller object to describe the GPIO mapping. In turn, that implies that the individual per-pin mux-selection/configuration nodes don't need to describe any GPIO-related information.

Yes good point. I agree it's a HW design issue, and could be in the properties for the pin controller object.

Just to summarize, the things to consider with the GPIO to mux mapping are:

1. Having this table as static data in the driver is is not a nice solution as it seems that we'd currently need six mapping tables for omap2+ alone.

Quoted 8 lines

2. This table is not needed for most of the (hundreds of) pins, it's only needed for a few selected pins, let's say ten or so on an average device. So there's no need to stuff the kernel with information about the unused GPIO pins.

It seems that the conclusion here is that we don't need to worry about GPIOs in the pinctrl-simple binding for now, and it can be added later without having to change the basic binding.

The one thing I wanted to resolve here wasn't so much the binding for GPIO interaction here, but the following comment:

Quoted 8 lines · You

Quoted 6 lines · I wrote:

From a binding perspective, I don't see why you'd want to allow two cases:

1) One node with multiple entries in pinctrl-simple,cells
2) Multiple nodes each with a single entry in pinctrl-simple,cells

Why not only allow (1)?

Quoted 4 lines

Because we need to specify GPIO for some pins. There may be additional flags too, we do have external DMA request lines for few pins available.. I'm not saying pinctrl fwk should know about that, but it's a similar mapping of pins to GPIO lines.

I'm asserting that since any GPIO mapping information would be at the top-level of the pinctrl-simple binding, we can in fact only allow option (1) above for the individual pin configuration nodes.

Also, were you intending pinctrl-simple to actually be the GPIO controller itself? That'd be another case that one might consider fairly simple, but then extends to being gpio-simple as well as pinctrl-simple...

Quoted 3 lines · Stephen Warren

Also, were you intending pinctrl-simple to actually be the GPIO controller itself? That'd be another case that one might consider fairly simple, but then extends to being gpio-simple as well as pinctrl-simple...

We have some pinctrl drivers implementing gpiolib too already, and it's unavoidable I think, as some recent discussion about matcing struct gpio_chip and pinctrl GPIO ranges shows.

Maybe "-simple" isn't such a good name for this thing. Noone thinks any kind of pin control is simple in any sense of the word anyway :-D

Tony, would pinctrl-dt-only.c be a better name perhaps?

Quoted 9 lines · Linus Walleij

Quoted 3 lines · Stephen Warren

Also, were you intending pinctrl-simple to actually be the GPIO controller itself? That'd be another case that one might consider fairly simple, but then extends to being gpio-simple as well as pinctrl-simple...

We have some pinctrl drivers implementing gpiolib too already, and it's unavoidable I think, as some recent discussion about matcing struct gpio_chip and pinctrl GPIO ranges shows.

I strongly believe we should only do this when the exact same HW module is both pinctrl and GPIO.

When there are separate HW modules, we should have separate drivers. The fact that the two drivers need to co-ordinate with each-other isn't a good argument to make them one driver.

And irrespective of how the drivers are structured, if there are two HW modules, we really need two separate nodes in DT to describe them, since the SW architecture (1 vs. 2 drivers) shouldn't influence the DT representation unduly.

Quoted 4 lines

Maybe "-simple" isn't such a good name for this thing. Noone thinks any kind of pin control is simple in any sense of the word anyway :-D

Tony, would pinctrl-dt-only.c be a better name perhaps?

That might be OK for the filename, but it doesn't seem like a useful change for the DT compatible value.

Quoted 4 lines · Linus Walleij

We have some pinctrl drivers implementing gpiolib too already, and it's unavoidable I think, as some recent discussion about matcing struct gpio_chip and pinctrl GPIO ranges shows.

I strongly believe we should only do this when the exact same HW module is both pinctrl and GPIO.

When there are separate HW modules, we should have separate drivers. The fact that the two drivers need to co-ordinate with each-other isn't a good argument to make them one driver.

Quoted 4 lines

Maybe "-simple" isn't such a good name for this thing. Noone thinks any kind of pin control is simple in any sense of the word anyway :-D

Tony, would pinctrl-dt-only.c be a better name perhaps?

That might be OK for the filename, but it doesn't seem like a useful change for the DT compatible value.

Quoted 1 line · Stephen Warren

On 05/12/2012 05:49 PM, Linus Walleij wrote:

Quoted 7 lines

Quoted 4 lines

Maybe "-simple" isn't such a good name for this thing. Noone thinks any kind of pin control is simple in any sense of the word anyway :-D

Tony, would pinctrl-dt-only.c be a better name perhaps?

That might be OK for the filename, but it doesn't seem like a useful change for the DT compatible value.

Oh I didn't think of these. Hm from a generic world running Windows Mobile etc I take it that this is seen as simple mappings then?

Quoted 11 lines · Tony Lindgren

Quoted 10 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

Quoted 4 lines

Quoted 1 line

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

I'd prefer to avoid any references to GPIOs here; not all muxable pins are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts independent.

Quoted 27 lines

Quoted 19 lines

	pioD: gpio at fffff800 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffff800 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	pioE: gpio at fffffa00 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffffa00 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

Quoted 7 lines

	dbgu {
		pins = < &pioB 12 0 0
			 &pioB 13 0 2 >;
	/* with macro */
		pins = < &pioB 12 MUX_A NO_PULL_UP
			 &pioB 13 MUX_A PULL_UP >;
	};

Quoted 2 lines

I could change to use this too no problem. The only concern I have is that is "&pioB 12" immutable for all gpio controllers?

You mean is the number of cells used to specify a GPIO the same everywhere? No. It's defined by #gpio-cells in the GPIO controller node.

But again, the GPIO binding shouldn't be related to the pinctrl binding directly.

Quoted 21 lines

Grepping the *.dts* files, at least exynos is using the following for gpios:

gpios = <&gpx2 0 0 0 2>;

If we can conclude that phandle to the gpio controller instance and the register offset is always enough here, then I'm OK changing to that format. It would actually save some parsing in most cases.

Quoted 12 lines

	/* and also the notion of linked group
	 * as on uart of network you have often the same subset of pin use.
	 *
	 * As example on uart rxd/txd is use for the group without rts/cts
	 * and the one with it
	 * on ethernet the RMII pin are use also on MII
	 */

	uart0_rxd_txd {
		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
	};

I don't really see how that DT format represents pins, functions, and nodes directly, and separately from which of those a board chooses to use. I think this binding and the one Tony originally proposed are eseentially semantically identical.

Going back to my idea of separating SoC and board configurations, if we did that, I'd expect to see something more like:

soc.dtsi or board.dts:

This is the data that the pin controller driver needs to export to pinctrl core. This can be completely enumerated in the soc.dtsi, or perhaps for uncommonly used pins/groups/functions, only included in the board.dts that actually uses it to cut down on soc.dtsi's size:

This data is not needed for SoCs whose pinctrl drivers include it in their driver file instead of DT.

Quoted 17 lines · Stephen Warren

Quoted 15 lines · Tony Lindgren

Quoted 11 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 9 lines · Tony Lindgren

Quoted 7 lines

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

Hmm I assume the "12" above the gpio number?

no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register. Got it.

Quoted 3 lines

I'd prefer to avoid any references to GPIOs here; not all muxable pins are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts independent.

my idea was to have a phandle pinctrl specific to represent the bank and use it in the same way as done on gpio

Quoted 15 lines

I don't really see how that DT format represents pins, functions, and nodes directly, and separately from which of those a board chooses to use. I think this binding and the one Tony originally proposed are eseentially semantically identical.

Going back to my idea of separating SoC and board configurations, if we did that, I'd expect to see something more like:

soc.dtsi or board.dts:

This is the data that the pin controller driver needs to export to pinctrl core. This can be completely enumerated in the soc.dtsi, or perhaps for uncommonly used pins/groups/functions, only included in the board.dts that actually uses it to cut down on soc.dtsi's size:

Quoted 2 lines

This data is not needed for SoCs whose pinctrl drivers include it in their driver file instead of DT.

I agree on at91 I propose exactly this but get the following comment tat we are going to have too much node.

so the idea I propoose with the pins array is to avoid this issue

my first bindings on at91

1) we describe one functin per pin

functions {
	rxd_pb12 {
		atmel,pin-id = <&pioB 12>;
		atmel,mux = <0>;
	};

	txd_pb13 {
		atmel,pin-id = <&piaB 13>;
		atmel,pull = <2>;
		atmel,mux = <0>;
	};

Also I think we should let the pinctrl
core eventually manage the pins more too. Right now the pins are a static array in the driver, which makes things unnecessarily complex for the DT case. It would be nice to also have something like pinctrl_register/unregister_pin instead of requiring them all be registered while registering with the framework initially.

Quoted 6 lines · Tony Lindgren

Also I think we should let the pinctrl
core eventually manage the pins more too. Right now the pins are a static array in the driver, which makes things unnecessarily complex for the DT case. It would be nice to also have something like pinctrl_register/unregister_pin instead of requiring them all be registered while registering with the framework initially.

Not instead of but in addition to :-)

One does not exclude the other. But a good general idea if you need this, just introduce it.

Add simple pinctrl driver using device tree data.

Currently this driver only works on omap2+ series of processors, where there is either an 8 or 16-bit padconf register for each pin. Support for other similar pinmux controllers could be added.

Note that this patch does not yet support pinconf_ops or GPIO. Further.

Signed-off-by Tony Lindgren <[email protected]>

---
Here's this one finally updated to use the common pinctrl bindings. That sure cleaned up a bunch of the nasty things in this driver :) Nice job Stephen!

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +23 -0

+				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+			>;
+		};
+	};
+
+
+	/* map all uart2 pins as a single function */
+	uart2_pins: pinmux_uart2_pins {
+		uart2_pins {
+			pinctrl-simple,cells = <
+				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+			>;
+		};
+	};
+
+	/* map all uart3 pins as separate functions */
+	uart3_pins: pinmux_uart3_pins {

From a binding perspective, I don't see why you'd want to allow two cases:

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +17 -0

+		uart3_cts_rctx.uart3_cts_rctx {
+			pinctrl-simple,cells = <0x100 0x118>;	/* OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+             };
+
+		uart3_rts_sd.uart3_rts_sd {
+			pinctrl-simple,cells = <0x102 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+		};
+
+		uart3_rx_irrx.uart3_rx_irrx {
+			pinctrl-simple,cells = <0x104 0x100>;	/* OMAP_PIN_INPUT | OMAP_MUX_MODE0 */
+		};
+
+		uart3_tx_irtx.uart3_tx_irtx {
+			pinctrl-simple,cells = <0x106 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+		};
+	};
+};

drivers/pinctrl/pinctrl-simple.c +1 -0

 
+struct pcs_device {

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt line 9

Quoted 9 lines · Tony Lindgren

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +7 -0

 
+Required properties:
+- compatible :  one of:
+	- "pinctrl-simple"
+	- "ti,omap2420-padconf"
+	- "ti,omap2430-padconf"
+	- "ti,omap3-padconf"
+	- "ti,omap4-padconf"

I'm in two minds about this.

If this is truly intended to be generic, I would not document any of the ti compatible values here. Instead, I'd create a binding document for the TI controllers that basically just says "this uses the bindings in pinctrl-simple.txt, with the following additions", and list the compatible values as an addition.

On the other hand, I worry about whether using "pinctrl-simple" here as the compatible value is going to cause issues:

Certainly, this is a pretty simple driver, and most likely reasonably generic an applicable to many SoCs. However, it doesn't cover a bunch of cases that I'd still consider "simple". For example, what if each pin has a separate mux and pinconf register? There are probably many such small cases that would add up to something more complex. to cover those cases, will we be able to extend pinctrl-simple to cover them, and continue to be backwards compatible, or will we need to create a binding/driver for pinctrl-simple-1, pinctrl-simple-2, pinctrl-simple-3 each of which covers some different, yet still simple, configuration?

To resolve this, perhaps it would be best to rework this binding and driver as a set of utility code that other bindings and drivers can build upon, rather than making it a standalone directly usable driver and binding.

Honestly, I'm not really sure which way to go here.

Quoted 5 lines

+- reg : offset and length of the register set for the mux registers
+- #pinctrl-cells : width of the pinctrl array, currently only 2 is supported
+- pinctrl-simple,register-width : pinmux register access width
+- pinctrl-simple,function-mask : mask of allowed pinmux function bits
+- pinctrl-simple,function-off : function off mode for disabled state

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt line 19

Quoted 5 lines

+- pinctrl-simple,pinconf-mask : mask of allowed pinconf bits

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +4 -0

+
+This driver uses the common pinctrl bindings as specified in
+pinctrl-bindings.txt document in this directory. The common bindings are used
+to specify the client device states using pinctrl-0 and pinctrl-names entries.

I think just remove the second sentence there; it's implicit given the first.

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt line 73

Quoted 12 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +12 -0

+/* board specific .dts file, such as omap4-sdp.dts */
+&omap4_pmx_core {
+
+	/*
+	 * map all board specific static pins enabled by the pinctrl driver
+	 * itself during the boot (or just set them up in the bootloader)
+	 */
+	pinctrl-names = "default";
+	pinctrl-0 = <&board_pins>;
+
+	board_pins: pinmux_board_pins {
+		board_static_pins {

I'd like to see a little more explanation of the node structure here. I.e. what does each node represent, why are there two levels of nodes, etc.

Given that the "pinctrl-simple,cells" can specify both mux function and pin configuration for each pin, i.e. everything you need to, I don't see why you'd ever need two levels of nodes here. I'd expect each "pin configuration node" (in pinctrl core bindings terminology) to be a single level:

node {
    pinctrl-simple,cells = <...>;
};

where node is what's references in the pinctrl-0 property by pinctrl client drivers.

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt

Quoted 1 line

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +1 -0

+			pinctrl-simple,cells = <

"cells" here doesn't seem like the right word. Perhaps "pins" or "configuration"? "Cell"s to me seems semi-reserved for binding cell count description, like #gpio-cells, #address-cells, etc.

Also, there's nothing in the free-form text part of this binding document that says describes the set of properties that need to exist in these "pin configuration nodes", nor what their content is.

Quoted 13 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +13 -0

+				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+			>;
+		};
+	};
+
+
+	/* map all uart2 pins as a single function */
+	uart2_pins: pinmux_uart2_pins {
+		uart2_pins {
+			pinctrl-simple,cells = <

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt line 97

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +10 -0

+				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+			>;
+		};
+	};
+
+	/* map all uart3 pins as separate functions */
+	uart3_pins: pinmux_uart3_pins {

From a binding perspective, I don't see why you'd want to allow two cases:

1) One node with multiple entries in pinctrl-simple,cells
2) Multiple nodes each with a single entry in pinctrl-simple,cells

Why not only allow (1)?

Quoted 3 lines

drivers/pinctrl/pinctrl-simple.c +1 -0

 
+struct pcs_device {

...

drivers/pinctrl/pinctrl-simple.c line 151

Quoted 3 lines

drivers/pinctrl/pinctrl-simple.c +3 -0

+	unsigned (*read)(void __iomem *reg);
+	void (*write)(unsigned val, void __iomem *reg);
+};

Why not use regmap instead of writing your own:

drivers/pinctrl/pinctrl-simple.c line 158

Quoted 2 lines

drivers/pinctrl/pinctrl-simple.c +2 -0

+static unsigned __maybe_unused pcs_readb(void __iomem *reg)
+static unsigned __maybe_unused pcs_readw(void __iomem *reg)

...

regmap recently gained support for memory-mapped IO, so should work for this. Also, using regmap might allow this binding/driver to be easily extended to support e.g. I2C-/SPI-based pin controller hardware, since regmap would handle all the IO details for you.

Supporting these different IO modes might require a little more thought though. For example, an I2C device's reg refers to its location on the parent bus, rather than the address of the registers within the device. We'd probably need to augment parent busses with some method for child devices to acquire a regmap object for their own IO in order to hide this from drivers.

Quoted 3 lines · Stephen Warren

Quoted 1 line · Tony Lindgren

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +0 -0

Quoted 12 lines

On the other hand, I worry about whether using "pinctrl-simple" here as the compatible value is going to cause issues:

Certainly, this is a pretty simple driver, and most likely reasonably generic an applicable to many SoCs. However, it doesn't cover a bunch of cases that I'd still consider "simple". For example, what if each pin has a separate mux and pinconf register? There are probably many such small cases that would add up to something more complex. to cover those cases, will we be able to extend pinctrl-simple to cover them, and continue to be backwards compatible, or will we need to create a binding/driver for pinctrl-simple-1, pinctrl-simple-2, pinctrl-simple-3 each of which covers some different, yet still simple, configuration?

Yes getting the binding right is the critical part here, everything else can be added as needed. I was thinking about using separate properties for auxilary registers, but now thinking about it a bit more, it may not be sufficient.

How about we make some of these properties into arrays? For example:

#pinctrl-cells = 6;
pinctrl-simple,function-mask = <0x0000ffff 0x0000ffff 0xffff0000>; pinctrl-simple,function-off = <0x7 0x7 0x70000>; pinctrl-simple,pinconf-mask = <0xffff0000 0xffff0000 0x0000ffff>;

Then for handling the set/clear bits case of registers, we could do that automatically if both masks are 0 for some registers. We actually have that for some omap1 where there are three registers per mux with bits to set or clear. And these bits are not the same across all registers..

Quoted 26 lines

Quoted 13 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +13 -0

+				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+			>;
+		};
+	};
+
+
+	/* map all uart2 pins as a single function */
+	uart2_pins: pinmux_uart2_pins {
+		uart2_pins {
+			pinctrl-simple,cells = <

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt line 97

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +10 -0

+				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+			>;
+		};
+	};
+
+	/* map all uart3 pins as separate functions */
+	uart3_pins: pinmux_uart3_pins {

From a binding perspective, I don't see why you'd want to allow two cases:

Quoted 4 lines

1) One node with multiple entries in pinctrl-simple,cells
2) Multiple nodes each with a single entry in pinctrl-simple,cells

Why not only allow (1)?

Because we need to specify GPIO for some pins. There may be additional flags

too, we do have external DMA request lines for few pins available.. I'm not saying pinctrl fwk should know about that, but it's a similar mapping of pins to GPIO lines.

Quoted 4 lines · Tony Lindgren

Quoted 3 lines · Stephen Warren

Quoted 1 line · Tony Lindgren

Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt +0 -0

...

Quoted 18 lines

Quoted 12 lines

On the other hand, I worry about whether using "pinctrl-simple" here as the compatible value is going to cause issues:

Certainly, this is a pretty simple driver, and most likely reasonably generic an applicable to many SoCs. However, it doesn't cover a bunch of cases that I'd still consider "simple". For example, what if each pin has a separate mux and pinconf register? There are probably many such small cases that would add up to something more complex. to cover those cases, will we be able to extend pinctrl-simple to cover them, and continue to be backwards compatible, or will we need to create a binding/driver for pinctrl-simple-1, pinctrl-simple-2, pinctrl-simple-3 each of which covers some different, yet still simple, configuration?

Yes getting the binding right is the critical part here, everything else can be added as needed. I was thinking about using separate properties for auxilary registers, but now thinking about it a bit more, it may not be sufficient.

Quoted 6 lines

How about we make some of these properties into arrays? For example:

#pinctrl-cells = 6;
pinctrl-simple,function-mask = <0x0000ffff 0x0000ffff 0xffff0000>; pinctrl-simple,function-off = <0x7 0x7 0x70000>; pinctrl-simple,pinconf-mask = <0xffff0000 0xffff0000 0x0000ffff>;

I'm not sure what the 3 entries in the array are meant to describe?

Quoted 1 line

Because we need to specify GPIO for some pins. There may be additional flags

What do you mean by "specify GPIO"?

Nothing in this pinctrl-simple binding seems to imply that it's also a GPIO controller.

If "GPIO" is one of the functions that can be mux'd onto a pin, then I'd expect that to be represented in exactly the same way as any other function that could be mux'd onto the pin.

So, I'm not sure what GPIO-related information you want to represent.

Quoted 3 lines

too, we do have external DMA request lines for few pins available.. I'm not saying pinctrl fwk should know about that, but it's a similar mapping of pins to GPIO lines.

Aren't DMA request lines also just another function that can be mux'd onto a pin?