From: Linus Walleij <[email protected]>
This creates a subsystem for handling of pinmux devices. These are
devices that enable and disable groups of pins on primarily PGA and
BGA type of chip packages and common in embedded systems.
This is being done to depopulate the arch/arm/* directory of such
custom drivers and try to abstract the infrastructure they all
need. See the Documentation/pinmux.txt file that is part of this
patch for more details.
Cc: Grant Likely <[email protected]>
Cc: Stephen Warren <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
---
ChangeLog v2->v3:
- Renamed subsystem folder to "pinctrl" since we will likely
want to keep other pin control such as biasing in this
subsystem too, so let us keep to something generic even though
we're mainly doing pinmux now.
- As a consequence, register pins as an abstract entity separate
from the pinmux. The muxing functions will claim pins out of the
pin pool and make sure they do not collide. Pins can now be
named by the pinctrl core.
- Converted the pin lookup from a static array into a radix tree,
I agreed with Grant Likely to try to avoid any static allocation
(which is crap for device tree stuff) so I just rewrote this
to be dynamic, just like irq number descriptors. The
platform-wide definition of number of pins goes away - this is
now just the sum total of the pins registered to the subsystem.
- Make sure mappings with only a function name and no device
works properly.
---
Documentation/ABI/testing/sysfs-class-pinmux | 11 +
Documentation/pinctrl.txt | 397 ++++++++++
MAINTAINERS | 5 +
drivers/Kconfig | 4 +
drivers/Makefile | 2 +
drivers/pinctrl/Kconfig | 29 +
drivers/pinctrl/Makefile | 6 +
drivers/pinctrl/core.c | 1028 ++++++++++++++++++++++++++
include/linux/pinctrl/machine.h | 57 ++
include/linux/pinctrl/pinmux.h | 180 +++++
10 files changed, 1719 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-pinmux
create mode 100644 Documentation/pinctrl.txt
create mode 100644 drivers/pinctrl/Kconfig
create mode 100644 drivers/pinctrl/Makefile
create mode 100644 drivers/pinctrl/core.c
create mode 100644 include/linux/pinctrl/machine.h
create mode 100644 include/linux/pinctrl/pinmux.h
diff --git a/Documentation/ABI/testing/sysfs-class-pinmux b/Documentation/ABI/testing/sysfs-class-pinmux
new file mode 100644
index 0000000..c2ea843
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-pinmux
@@ -0,0 +1,11 @@
+What: /sys/class/pinmux/.../name
+Date: May 2011
+KernelVersion: 3.1
+Contact: Linus Walleij <[email protected]>
+Description:
+ Each pinmux directory will contain a field called
+ name. This holds a string identifying the pinmux for
+ display purposes.
+
+ NOTE: this will be empty if no suitable name is provided
+ by platform or pinmux drivers.
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
new file mode 100644
index 0000000..c4b6f48
--- /dev/null
+++ b/Documentation/pinctrl.txt
@@ -0,0 +1,397 @@
+PINCTRL (PIN CONTROL) subsystem
+This document outlines the pin control subsystem in Linux
+
+This subsystem deals with:
+
+- Multiplexing of pins, pads, fingers (etc) see below for details
+
+The intention is to also deal with:
+
+- Software-controlled biasing and driving mode specific pins, such as
+ pull-up/down, open drain etc, load capacitance configuration when controlled
+ by software, etc.
+
+
+Top-level interface
+===================
+
+Definition of PIN:
+
+- PINS are equal to pads or fingers or whatever packaging output you want to
+ mux and these are denoted by unsigned integers in the range 0..MAX_INT. Every
+ pin on your system (or atleast every pin that can be muxed) should have a
+ unique number. The numberspace can span several chips if you have more chips
+ on your system that can be subject to muxing.
+
+All pins, pads, fingers, balls or other controllable units on the system shall
+be given a unique number in the global number space beginning on zero.
+
+All pins on the system need to be registered, typically by board code or a
+device tree. Board/machine code will #include <linux/pinctrl/machine.h>
+and use one of:
+
+ pinctrl_register_pins_sparse();
+ pinctrl_register_pins_dense();
+
+To register all the pins on the system. Both are supplied with a list of
+descriptor items, the difference is that the _dense version will also
+register anonymous pins for each "hole" in the pin map, from zero to the
+supplied extra argument giving the number of pins.
+
+Here is an example of a PGA (Pin Grid Array) chip seen from underneath:
+
+ A B C D E F G H
+
+ 8 o o o o o o o o
+
+ 7 o o o o o o o o
+
+ 6 o o o o o o o o
+
+ 5 o o o o o o o o
+
+ 4 o o o o o o o o
+
+ 3 o o o o o o o o
+
+ 2 o o o o o o o o
+
+ 1 o o o o o o o o
+
+To register and name all the pins on this package we can do this in a board
+file:
+
+#include <linux/pinctrl/machine.h>
+
+const struct pinctrl_pin_desc __initdata my_pins[] = {
+ PINCTRL_PIN(0, "A1"),
+ PINCTRL_PIN(1, "A2"),
+ PINCTRL_PIN(2, "A3"),
+ ...
+ PINCTRL_PIN(61, "H6"),
+ PINCTRL_PIN(62, "H7"),
+ PINCTRL_PIN(63, "H8"),
+};
+
+int __init register_pins(void)
+{
+ pinctrl_register_pins_dense(&my_pins, ARRAY_SIZE(my_pins), 64);
+}
+
+Pins usually have fancier names than this. You can find these in the dataheet
+for your chip. Notice that the core machine.h file provides a fancy macro
+called PINCTRL_PIN() to create the struct entries.
+
+
+PINMUX interfaces
+=================
+
+These calls use the pinmux_* naming prefix. No other calls should use that
+prefix.
+
+
+What is pinmuxing?
+==================
+
+Pinmux, also known as padmux, ballmux, alternate functions or mission modes
+is a way for chip vendors producing some kind of electrical packages to use
+a certain physical bin (ball, pad, finger, etc) for multiple mutually exclusive
+functions, depending on the application. By "application" in this context
+we usually mean a way of soldering or wiring the package into an electronic
+system, even though the framework makes it possible to handle also change the
+function at runtime.
+
+Here is an example of a PGA (Pin Grid Array) chip seen from underneath:
+
+ A B C D E F G H
+ +---+
+ 8 | o | o o o o o o o
+ | |
+ 7 | o | o o o o o o o
+ | |
+ 6 | o | o o o o o o o
+ +---+---+
+ 5 | o | o | o o o o o o
+ +---+---+ +---+
+ 4 o o o o o o | o | o
+ | |
+ 3 o o o o o o | o | o
+ | |
+ 2 o o o o o o | o | o
+ | |
+ 1 o o o o o o | o | o
+ +---+
+
+This is not tetris. The game to think of is chess. Not all PGA/BGA packages
+are chessboard-like, big ones have "holes" in some arrangement according to
+different design patterns, but we're using this as a simple example. Of the
+pins you see some will be taken by things like a few VCC and GND to feed power
+to the chip, and quite a few will be taken by large ports like an external
+memory interface. The remaining pins will often be subject to pin multiplexing.
+
+The example 8x8 PGA package above will have pin numbers 0 thru 63 assigned to
+its physical pins. It will name the pins { A1, A2, A3 ... H6, H7, H8 } using
+pinctrl_register_pins_[sparse|dense]() and a suitable data set as shown
+earlier.
+
+In this 8x8 BGA package the pins { A8, A7, A6, A5 } can be used as an SPI port
+(these are four pins: CLK, RXD, TXD, FRM). In that case, pin B5 can be used as
+some general-purpose GPIO pin. However, in another setting, pins { A5, B5 } can
+be used as an I2C port (these are just two pins: SCL, SDA). Needless to say,
+we cannot use the SPI port and I2C port at the same time. However in the inside
+of the package the silicon performing the SPI logic can alternatively be routed
+out on pins { G4, G3, G2, G1 }.
+
+This way the silicon blocks present inside the chip can be multiplexed "muxed"
+out on different pin ranges. Often contemporary SoC (systems on chip) will
+contain several I2C, SPI, SDIO/MMC, etc silicon blocks that can be routed to
+different pins by pinmux settings.
+
+Since general-purpose I/O pins (GPIO) are typically always in shortage, it is
+common to be able to use almost any pin as a GPIO pin if it is not currently
+in use by some other I/O port.
+
+
+Pinmux conventions
+==================
+
+The purpose of the pinmux subsystem is to abstract and provide pinmux settings
+to the devices you choose to instantiate in your machine configuration. It is
+inspired by the clk, GPIO and regulator subsystems, so devices will request
+their mux setting, but it's also possible to request a single pin for e.g.
+GPIO.
+
+Definitions:
+
+- FUNCTIONS can be switched in and out by a driver residing with the pinmux
+ subsystem in the drivers/pinmux/* directory of the kernel. The pinmux driver
+ knows the possible functions. In the example above you can identify three
+ pinmux functions, two for spi and one for i2c.
+
+- FUNCTIONS are assumed to be enumerable from zero in a one-dimensional array.
+ In this case the array could be something like: { spi0-0, spi0-1, i2c0-0 }
+ for the three available settings. The knowledge of this one-dimensional array
+ and it's machine-specific particulars is kept inside the pinmux driver,
+ from the outside only these enumerators are known, and the driver core
+ can request the name or the list of pins belonging to a certain enumerator.
+
+- FUNCTIONS are MAPPED to a certain device by the board file, device tree or
+ similar machine setup configuration mechanism, similar to how regulators are
+ connected to devices, usually by name. In the example case we can define
+ that this particular machine shall use device spi0 with pinmux setting
+ spi0-1 and i2c0 on i2c0-1, something like the two-tuple:
+ { {spi0, spi0-1}, {i2c0, i2c0-1} }
+
+- FUNCTIONS are provided on a first-come first-serve basis, so if some other
+ device mux setting or GPIO pin request has already taken your physical pin,
+ you will be denied the use of it. To get (activate) a new setting, the old
+ one has to be put (deactivated) first.
+
+Sometimes the documentation and hardware registers will be oriented around
+pads (or "fingers") rather than pins - these are the soldering surfaces on the
+silicon inside the package, and may or may not match the actual number of
+pins/balls underneath the capsule. Pick some enumeration that makes sense to
+you. Define enumerators only for the pins you can control if that makes sense.
+
+
+Pinmux drivers
+==============
+
+The driver will for all calls be provided an offset pin number into its own
+pin range. If you have 2 chips with 8x8 pins, the first chips pins will have
+numbers 0 thru 63 and the second one pins 64 thru 127, but the driver for the
+second chip will be passed numbers in the range 0 thru 63 anyway, base offset
+subtracted.
+
+Pinmux drivers are required to supply a few callback functions, some are
+optional. Usually the enable() and disable() functions are implemented,
+writing values into some certain registers to activate a certain mux setting
+for a certain pin.
+
+A simple driver for the above example will work by setting bits 0, 1 or 2
+into some register mamed MUX, so it enumerates its available settings and
+their pin assignments, and expose them like this:
+
+#include <linux/pinctrl/pinmux.h>
+
+struct foo_pmx_func {
+ char *name;
+ const unsigned int *pins;
+ const unsigned num_pins;
+};
+
+static unsigned int spi0_0_pins[] = { 0, 8, 16, 24 };
+static unsigned int i2c0_pins[] = { 24, 25 };
+static unsigned int spi0_1_pins[] = { 38, 46, 54, 62 };
+
+static struct foo_pmx_func myfuncs[] = {
+ {
+ .name = "spi0-0",
+ .pins = spi0_0_pins,
+ .num_pins = ARRAY_SIZE(spi0_1_pins),
+ },
+ {
+ .name = "i2c0",
+ .pins = i2c0_pins,
+ .num_pins = ARRAY_SIZE(i2c0_pins),
+ },
+ {
+ .name = "spi0-1",
+ .pins = spi0_1_pins,
+ .num_pins = ARRAY_SIZE(spi0_1_pins),
+ },
+};
+
+int foo_list(struct pinmux_dev *pmxdev, unsigned selector)
+{
+ if (selector >= ARRAY_SIZE(myfuncs))
+ return -EINVAL;
+ return 0;
+}
+
+const char *foo_get_fname(struct pinmux_dev *pmxdev, unsigned selector)
+{
+ if (selector >= ARRAY_SIZE(myfuncs))
+ return NULL;
+ return myfuncs[selector].name;
+}
+
+static int foo_get_pins(struct pinmux_dev *pmxdev, unsigned selector,
+ unsigned ** const pins, unsigned * const num_pins)
+{
+ if (selector >= ARRAY_SIZE(myfuncs))
+ return -EINVAL;
+ *pins = myfuncs[selector].pins;
+ *num_pins = myfuncs[selector].num_pins;
+ return 0;
+}
+
+
+int foo_enable(struct pinmux_dev *pmxdev, unsigned selector)
+{
+ if (selector < ARRAY_SIZE(myfuncs))
+ write((read(MUX)|(1<<selector)), MUX)
+ return 0;
+ }
+ return -EINVAL;
+}
+
+int foo_disable(struct pinmux_dev *pmxdev, unsigned selector)
+{
+ if (selector < ARRAY_SIZE(myfuncs))
+ write((read(MUX) & ~(1<<selector)), MUX)
+ return 0;
+ }
+ return -EINVAL;
+}
+
+struct pinmux_ops ops = {
+ .list_functions = foo_list,
+ .get_function_name = foo_get_fname,
+ .get_function_pins = foo_get_pins,
+ .enable = foo_enable,
+ .disable = foo_disable,
+};
+
+Now the able reader will say: "wait - the driver needs to make sure it
+can set this and that bit at the same time, because else it will collide
+and wreak havoc in my electronics, and make sure noone else is using the
+other setting that it's incompatible with".
+
+In the example activating muxing 0 and 1 at the same time setting bits
+0 and 1, uses one pin in common so they would collide.
+
+The beauty of the pinmux subsystem is that since it keeps track of all
+pins and who is using them, it will already have denied an impossible
+request like that, so the driver does not need to worry about such
+things - when it gets a selector passed in, the pinmux subsystem makes
+sure no other device or GPIO assignment is already using the selected
+pins.
+
+The above functions are mandatory to implement for a pinmux driver.
+
+The function list could become long, especially if you can convert every
+individual pin into a GPIO pin independent of any other pins, then your
+function array can become 64 entries for each GPIO setting and then the
+device functions. For this reason there is an additional function you
+can implement to enable only GPIO on an individual pin: gpio_enable().
+
+
+Pinmux board/machine configuration
+==================================
+
+Boards and machines define how a certain complete running system is put
+together, including how GPIOs and devices are muxed, how regulators are
+constrained and how the clock tree looks. Of course pinmux settings are also
+part of this.
+
+A pinmux config for a machine looks pretty much like a simple regulator
+configuration, so for the example array above we want to enable i2c and
+spi on the second function mapping:
+
+#include <linux/pinctrl/machine.h>
+
+static struct pinmux_map pmx_mapping[] = {
+ {
+ .function = "spi0-1",
+ .dev_name = "foo-spi.0",
+ },
+ {
+ .function = "i2c0",
+ .dev_name = "foo-i2c.0",
+ },
+};
+
+Since the above construct is pretty common there is a helper macro to make
+it even more compact:
+
+static struct pinmux_map pmx_mapping[] = {
+ PINMUX_MAP("spi0-1", "foo-spi.0"),
+ PINMUX_MAP("i2c0", "foo-i2c.0"),
+};
+
+The dev_name here matches to the unique device name that can be used to look
+up the device struct (just like with clockdev or regulators). The function name
+must match a function provided by the pinmux driver handling this pin range.
+You register this pinmux mapping to the pinmux subsystem by simply:
+
+ ret = pinmux_register_mappings(&pmx_mapping, ARRAY_SIZE(pmx_mapping));
+
+
+Pinmux requests from drivers
+============================
+
+A driver may request a certain mux to be activated, usually just the default
+mux like this:
+
+#include <linux/pinctrl/pinmux.h>
+
+foo_probe()
+{
+ /* Allocate a state holder named "state" etc */
+ struct pinmux pmx;
+
+ pmx = pinmux_get(&device, NULL);
+ if IS_ERR(pmx)
+ return PTR_ERR(pmx);
+ pinmux_enable(pmx);
+
+ state->pmx = pmx;
+}
+
+foo_remove()
+{
+ pinmux_disable(state->pmx);
+ pinmux_put(state->pmx);
+}
+
+If you want a specific mux setting and not just the first one found for this
+device you can specify a specific mux setting, for example in the above example
+the second i2c0 setting: pinmux_get(&device, "spi0-2");
+
+This get/enable/disable/put sequence can just as well be handled by bus drivers
+if you don't want each and every driver to handle it and you know the
+arrangement on your bus.
+
+The pins are allocated for your device when you issue the pinmux_get() call,
+after this you should be able to see this in the debugfs listing of all pins.
diff --git a/MAINTAINERS b/MAINTAINERS
index 29801f7..5caea5a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4933,6 +4933,11 @@ L: [email protected]
S: Maintained
F: drivers/mtd/devices/phram.c
+PINMUX SUBSYSTEM
+M: Linus Walleij <[email protected]>
+S: Maintained
+F: drivers/pinmux/
+
PKTCDVD DRIVER
M: Peter Osterlund <[email protected]>
S: Maintained
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 3bb154d..6998d78 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -56,6 +56,10 @@ source "drivers/pps/Kconfig"
source "drivers/ptp/Kconfig"
+# pinctrl before gpio - gpio drivers may need it
+
+source "drivers/pinctrl/Kconfig"
+
source "drivers/gpio/Kconfig"
source "drivers/w1/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 09f3232..a590a01 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -5,6 +5,8 @@
# Rewritten to use lists instead of if-statements.
#
+# GPIO must come after pinctrl as gpios may need to mux pins etc
+obj-y += pinctrl/
obj-y += gpio/
obj-$(CONFIG_PCI) += pci/
obj-$(CONFIG_PARISC) += parisc/
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
new file mode 100644
index 0000000..8050fdf
--- /dev/null
+++ b/drivers/pinctrl/Kconfig
@@ -0,0 +1,29 @@
+#
+# PINCTRL infrastructure and drivers
+#
+
+menuconfig PINCTRL
+ bool "PINCTRL Support"
+ depends on SYSFS && EXPERIMENTAL
+ help
+ This enables the PINCTRL subsystem for controlling pins
+ on chip packages, for example multiplexing pins on primarily
+ PGA and BGA packages for systems on chip.
+
+ If unsure, say N.
+
+if PINCTRL
+
+config DEBUG_PINCTRL
+ bool "Debug PINCTRL calls"
+ depends on DEBUG_KERNEL
+ help
+ Say Y here to add some extra checks and diagnostics to PINCTRL calls.
+
+config PINMUX_U300
+ bool "U300 pinmux driver"
+ depends on ARCH_U300
+ help
+ Say Y here to enable the U300 pinmux driver
+
+endif
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
new file mode 100644
index 0000000..44d8933
--- /dev/null
+++ b/drivers/pinctrl/Makefile
@@ -0,0 +1,6 @@
+# generic pinmux support
+
+ccflags-$(CONFIG_DEBUG_PINMUX) += -DDEBUG
+
+obj-$(CONFIG_PINCTRL) += core.o
+obj-$(CONFIG_PINMUX_U300) += pinmux-u300.o
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
new file mode 100644
index 0000000..8fd1437
--- /dev/null
+++ b/drivers/pinctrl/core.c
@@ -0,0 +1,1028 @@
+/*
+ * Core driver for the pinmux subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * Based on bits of regulator core, gpio core and clk core
+ *
+ * Author: Linus Walleij <[email protected]>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#define pr_fmt(fmt) "pinctrl core: " fmt
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/radix-tree.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+#include <linux/sysfs.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinmux.h>
+
+/* Global list of pinmuxes */
+static DEFINE_MUTEX(pinmux_list_mutex);
+static LIST_HEAD(pinmux_list);
+
+/* Global list of pinmux devices */
+static DEFINE_MUTEX(pinmuxdev_list_mutex);
+static LIST_HEAD(pinmuxdev_list);
+
+/**
+ * struct pin_desc - pin descriptor for each physical pin in the arch
+ * @pmxdev: corresponding pinmux device
+ * @requested: whether the pin is already requested by pinmux or not
+ * @name: a name for the pin, e.g. the name of the pin/pad/finger on a
+ * datasheet or such
+ * @function: a named muxing function for the pin that will be passed to
+ * subdrivers and shown in debugfs etc
+ */
+struct pin_desc {
+ struct pinmux_dev *pmxdev;
+ bool requested;
+ char name[16];
+ char function[16];
+};
+/* Global lookup of per-pin descriptors, one for each physical pin */
+static DEFINE_SPINLOCK(pin_desc_tree_lock);
+static RADIX_TREE(pin_desc_tree, GFP_KERNEL);
+static unsigned int num_pins = 0;
+
+/**
+ * struct pinmux - per-device pinmux state holder
+ * @node: global list node - only for internal use
+ * @dev: the device using this pinmux
+ * @pmxdev: the pinmux device controlling this pinmux
+ * @map: corresponding pinmux map active for this pinmux setting
+ * @usecount: the number of active users of this mux setting, used to keep
+ * track of nested use cases
+ * @pins: an array of discrete physical pins used in this mapping, taken
+ * from the global pin enumeration space (copied from pinmux map)
+ * @num_pins: the number of pins in this mapping array, i.e. the number of
+ * elements in .pins so we can iterate over that array (copied from
+ * pinmux map)
+ * @pmxdev: pinmux device handling this pinmux
+ * @pmxdev_selector: the selector for the pinmux device handling this pinmux
+ * @mutex: a lock for the pinmux state holder
+ */
+struct pinmux {
+ struct list_head node;
+ struct device *dev;
+ struct pinmux_map const *map;
+ unsigned usecount;
+ struct pinmux_dev *pmxdev;
+ unsigned pmxdev_selector;
+ struct mutex mutex;
+};
+
+int pin_is_valid(int pin)
+{
+ return pin >= 0 && pin < num_pins;
+}
+EXPORT_SYMBOL_GPL(pin_is_valid);
+
+static ssize_t pinmux_name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pinmux_dev *pmxdev = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%s\n", pmxdev_get_name(pmxdev));
+}
+
+static struct device_attribute pinmux_dev_attrs[] = {
+ __ATTR(name, 0444, pinmux_name_show, NULL),
+ __ATTR_NULL,
+};
+
+static void pinmux_dev_release(struct device *dev)
+{
+ struct pinmux_dev *pmxdev = dev_get_drvdata(dev);
+ kfree(pmxdev);
+}
+
+static struct class pinmux_class = {
+ .name = "pinmux",
+ .dev_release = pinmux_dev_release,
+ .dev_attrs = pinmux_dev_attrs,
+};
+
+/* Deletes a range of pin descriptors */
+static void pinctrl_free_pindescs(struct pinctrl_pin_desc const *pins,
+ unsigned num_pins)
+{
+ int i;
+
+ for (i = 0; i < num_pins; i++) {
+ struct pin_desc *pindesc;
+
+ spin_lock(&pin_desc_tree_lock);
+ pindesc = radix_tree_lookup(&pin_desc_tree, pins[i].number);
+ if (pindesc != NULL) {
+ radix_tree_delete(&pin_desc_tree, pins[i].number);
+ num_pins --;
+ }
+ spin_unlock(&pin_desc_tree_lock);
+ kfree(pindesc);
+ }
+}
+
+static int pinctrl_register_one_pin(unsigned number, const char *name)
+{
+ struct pin_desc *pindesc;
+
+ spin_lock(&pin_desc_tree_lock);
+ pindesc = radix_tree_lookup(&pin_desc_tree, number);
+ spin_unlock(&pin_desc_tree_lock);
+
+ if (pindesc != NULL) {
+ pr_err("pin %d already registered\n", number);
+ return -EINVAL;
+ }
+
+ pindesc = kzalloc(sizeof(*pindesc), GFP_KERNEL);
+ if (pindesc == NULL)
+ return -ENOMEM;
+
+ /* Copy optional basic pin info */
+ if (name) {
+ strncpy(pindesc->name, name, 16);
+ pindesc->name[15] = '\0';
+ }
+
+ spin_lock(&pin_desc_tree_lock);
+ radix_tree_insert(&pin_desc_tree, number, pindesc);
+ num_pins ++;
+ spin_unlock(&pin_desc_tree_lock);
+ return 0;
+}
+
+/* Passing in 0 num_pins means "sparse" */
+static int pinctrl_register_pins(struct pinctrl_pin_desc const *pins,
+ unsigned num_descs, unsigned num_pins)
+{
+ unsigned i;
+ int ret = 0;
+
+ for (i = 0; i < num_descs; i++) {
+ ret = pinctrl_register_one_pin(pins[i].number, pins[i].name);
+ if (ret)
+ return ret;
+ }
+
+ if (num_pins == 0)
+ return 0;
+
+ /*
+ * If we are registerering dense pinlists, fill in all holes with
+ * anonymous pins.
+ */
+ for (i = 0; i < num_pins; i++) {
+ char pinname[16];
+ struct pin_desc *pindesc;
+
+ spin_lock(&pin_desc_tree_lock);
+ pindesc = radix_tree_lookup(&pin_desc_tree, i);
+ spin_unlock(&pin_desc_tree_lock);
+ /* Already registered this one, take next */
+ if (pindesc)
+ continue;
+
+ snprintf(pinname, 15, "anonymous %u", i);
+ pinname[15] = '\0';
+
+ ret = pinctrl_register_one_pin(i, pinname);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+/**
+ * pinctrl_register_pins_sparse() - register a range of pins for a
+ * board/machine with potential holes in the pin map. The pins in
+ * the holes will not be usable.
+ * @pins: a range of pins to register
+ * @num_descs: the number of pins descriptors passed in through the previous
+ * pointer
+ */
+int pinctrl_register_pins_sparse(struct pinctrl_pin_desc const *pins,
+ unsigned num_descs)
+{
+ int ret;
+
+ ret = pinctrl_register_pins(pins, num_descs, 0);
+ if (ret)
+ pinctrl_free_pindescs(pins, num_descs);
+ return ret;
+
+}
+EXPORT_SYMBOL_GPL(pinctrl_register_pins);
+
+/**
+ * pinctrl_register_pins_dense() - register a range of pins for a
+ * board/machine, if there are holes in the pin map, they will be
+ * allocated by anonymous pins.
+ * @pins: a range of pins to register
+ * @num_descs: the number of pins descriptors passed in through the previous
+ * pointer
+ * @num_pins: the total number of pins including holes in the pin map and
+ * any "air" at the end of the map, all pins from 0 to this number
+ * will be allocated, the ones that does not have descriptors passed
+ * in will be marked "anonymous"
+ */
+int pinctrl_register_pins_dense(struct pinctrl_pin_desc const *pins,
+ unsigned num_descs, unsigned num_pins)
+{
+ int ret;
+ unsigned i;
+
+ ret = pinctrl_register_pins(pins, num_descs, num_pins);
+ if (ret) {
+ for (i = 0; i < num_pins; i++) {
+ struct pin_desc *pindesc;
+
+ spin_lock(&pin_desc_tree_lock);
+ pindesc = radix_tree_lookup(&pin_desc_tree, i);
+ if (pindesc != NULL) {
+ radix_tree_delete(&pin_desc_tree, i);
+ num_pins --;
+ }
+ spin_unlock(&pin_desc_tree_lock);
+ kfree(pindesc);
+ }
+ }
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pinctrl_register_pins_sparse);
+
+/**
+ * pin_request() - request a single pin to be muxed in, typically for GPIO
+ * @pin: the pin number in the global pin space
+ * @function: a functional name to give to this pin, passed to the driver
+ * so it knows what function to mux in, e.g. the string "gpioNN"
+ * means that you want to mux in the pin for use as GPIO number NN
+ * @gpio: if this request concerns a single GPIO pin
+ */
+static int pin_request(int pin, const char *function, bool gpio)
+{
+ struct pin_desc *desc;
+ struct pinmux_dev *pmxdev;
+ const struct pinmux_ops *ops;
+ int status = -EINVAL;
+ unsigned long flags;
+
+ pr_debug("request pin %d for %s\n", pin, function);
+ if (!pin_is_valid(pin)) {
+ pr_err("pin is invalid\n");
+ return -EINVAL;
+ }
+
+ if (!function) {
+ pr_err("no function name given\n");
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&pin_desc_tree_lock, flags);
+ desc = radix_tree_lookup(&pin_desc_tree, pin);
+ if (desc == NULL) {
+ pr_err("pin is not registered so it cannot be requested\n");
+ goto out;
+ }
+ pmxdev = desc->pmxdev;
+ if (desc->requested) {
+ pr_err("pin already requested\n");
+ goto out;
+ }
+ if (!pmxdev) {
+ pr_warn("no pinmux device is handling pin %d\n", pin);
+ goto out;
+ }
+ ops = pmxdev->desc->ops;
+
+ /* Let each pin increase references to this module */
+ if (!try_module_get(pmxdev->owner)) {
+ pr_err("could not increase module refcount for pin %d\n", pin);
+ status = -EINVAL;
+ goto out;
+ }
+
+ /*
+ * If there is no kind of request function for the pin we just assume
+ * we got it by default and proceed.
+ */
+ if (gpio && ops->gpio_request_enable)
+ /* This requests and enables a single GPIO pin */
+ status = ops->gpio_request_enable(pmxdev,
+ pin - pmxdev->desc->base);
+ else if (ops->request)
+ status = ops->request(pmxdev,
+ pin - pmxdev->desc->base);
+ else
+ status = 0;
+
+ if (status) {
+ pr_err("->request on device %s failed "
+ "for pin %d (offset %d)\n",
+ pmxdev->desc->name, pin,
+ pin - pmxdev->desc->base);
+ goto out;
+ }
+
+ desc->requested = true;
+ strncpy(desc->function, function, 16);
+ desc->function[15] = '\0';
+
+out:
+ spin_unlock_irqrestore(&pin_desc_tree_lock, flags);
+ if (status)
+ pr_err("pin-%d (%s) status %d\n",
+ pin, function ? : "?", status);
+
+ return status;
+}
+
+/**
+ * pin_free() - release a single muxed in pin so something else can be muxed in
+ * instead
+ * @pin: the pin to free
+ */
+static void pin_free(int pin)
+{
+ struct pin_desc *desc;
+ struct pinmux_dev *pmxdev;
+ unsigned long flags;
+
+ if (!pin_is_valid(pin))
+ return;
+
+ spin_lock_irqsave(&pin_desc_tree_lock, flags);
+ desc = radix_tree_lookup(&pin_desc_tree, pin);
+ if (desc == NULL) {
+ pr_err("pin is not registered so it cannot be freed\n");
+ goto out;
+ }
+ pmxdev = desc->pmxdev;
+ if (pmxdev) {
+ const struct pinmux_ops *ops = pmxdev->desc->ops;
+
+ if (ops->free)
+ ops->free(pmxdev, pin - pmxdev->desc->base);
+ }
+ desc->requested = false;
+ desc->function[0] = '\0';
+ module_put(pmxdev->owner);
+out:
+ spin_unlock_irqrestore(&pin_desc_tree_lock, flags);
+}
+
+/**
+ * pinmux_request_gpio() - request a single pin to be muxed in to be used
+ * as a GPIO pin
+ * @pin: the pin to mux in as GPIO
+ * @gpio: the corresponding GPIO pin number
+ */
+int pinmux_request_gpio(int pin, unsigned gpio)
+{
+ char gpiostr[16];
+
+ snprintf(gpiostr, 15, "gpio%d", gpio);
+ return pin_request(pin, gpiostr, true);
+}
+EXPORT_SYMBOL_GPL(pinmux_request_gpio);
+
+/**
+ * pinmux_free_gpio() - free a single pin, currently muxed in to be used
+ * as a GPIO pin
+ * @pin: the pin to mux out from GPIO
+ */
+void pinmux_free_gpio(int pin)
+{
+ return pin_free(pin);
+}
+EXPORT_SYMBOL_GPL(pinmux_free_gpio);
+
+int pinmux_register_mappings(struct pinmux_map const *maps, unsigned num_maps)
+{
+ int ret = 0;
+ int i;
+
+ pr_debug("add %d functions\n", num_maps);
+ for (i = 0; i < num_maps; i++) {
+ struct pinmux *pmx;
+
+ /* Sanity check the mapping */
+ if (!maps[i].function) {
+ pr_err("failed to register map %d - no function ID given\n", i);
+ ret = -EINVAL;
+ goto out;
+ }
+ if (!maps[i].dev && !maps[i].dev_name) {
+ pr_err("failed to register map %d - no device or device name given\n", i);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ /*
+ * create the state cookie holder struct pinmux for each
+ * mapping, this is what consumers will get when requesting
+ * a pinmux handle with pinmux_get()
+ */
+ pmx = kzalloc(sizeof(struct pinmux), GFP_KERNEL);
+ if (pmx == NULL) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ mutex_init(&pmx->mutex);
+ pmx->map = &maps[i];
+
+ /* Add the pinmux */
+ mutex_lock(&pinmux_list_mutex);
+ list_add(&pmx->node, &pinmux_list);
+ mutex_unlock(&pinmux_list_mutex);
+ pr_debug("add function %s\n", maps[i].function);
+ }
+
+out:
+ return ret;
+}
+
+/**
+ * acquire_pins() - acquire all the pins for a certain funcion on a certain
+ * pinmux device
+ * @pmxdev: the device to take the function on
+ * @selector: the selector to acquire the pins for
+ */
+int acquire_pins(struct pinmux_dev *pmxdev, unsigned selector)
+{
+ const struct pinmux_ops *ops = pmxdev->desc->ops;
+ unsigned *pins;
+ unsigned num_pins;
+ const char *func = ops->get_function_name(pmxdev, selector);
+ int ret;
+ int i;
+
+ ret = ops->get_function_pins(pmxdev, selector, &pins, &num_pins);
+ if (ret)
+ return ret;
+
+ /* Try to allocate all pins in this pinmux map, one by one */
+ for (i = 0; i < num_pins; i++) {
+ ret = pin_request(pins[i], func, false);
+ if (ret) {
+ pr_err("could not get pin %d for function %s "
+ "on device %s - conflicting mux mappings?\n",
+ pins[i], func ? : "(undefined)",
+ pmxdev->desc->name);
+ /* On error release all taken pins */
+ i--; /* this pin just failed */
+ for (; i >= 0; i--)
+ pin_free(pins[i]);
+ return -ENODEV;
+ }
+ }
+ return 0;
+}
+
+/**
+ * pinmux_get() - retrieves the pinmux for a certain device
+ * @dev: the device to get the pinmux for
+ * @func: an optional mux name or NULL, the name is only needed
+ * if a single device has multiple pinmux settings (i.e. if the
+ * same device can be muxed out on different sets of pins) or if
+ * you need an anonymous pinmux (not tied to any specific device)
+ */
+struct pinmux *pinmux_get(struct device *dev, const char *func)
+{
+ struct pinmux_map const *map = NULL;
+ struct pinmux_dev *pmxdev = NULL;
+ const char *devname = NULL;
+ struct pinmux *pmx;
+ bool found = false;
+ int ret = -ENODEV;
+
+ /* We must have dev or ID or both */
+ if (!dev && !func)
+ return ERR_PTR(-EINVAL);
+
+ mutex_lock(&pinmux_list_mutex);
+
+ if (dev)
+ devname = dev_name(dev);
+
+ /* Locate the pinmux map */
+ list_for_each_entry(pmx, &pinmux_list, node) {
+ map = pmx->map;
+
+ /* If an function is given, it MUST match */
+ if ((func != NULL) && strcmp(map->function, func))
+ continue;
+
+ /*
+ * This is for the case where no device name is given, we
+ * already know that the function name matches from above
+ * code.
+ */
+ if (!map->dev_name && (func != NULL)) {
+ found = true;
+ break;
+ }
+
+ /* If the mapping has a device set up it must match */
+ if (map->dev_name &&
+ (!devname || !strcmp(map->dev_name, devname))) {
+ /* MATCH! */
+ found = true;
+ break;
+ }
+ }
+
+ mutex_unlock(&pinmux_list_mutex);
+
+ if (!found) {
+ pr_err("could not find mux map for device %s, ID %s\n",
+ devname ? : "(anonymous)", func ? : "(undefined)");
+ goto out;
+ }
+
+ /* Make sure that noone else is using this function mapping */
+ mutex_lock(&pmx->mutex);
+ if (pmx->dev) {
+ if (pmx->dev != dev) {
+ mutex_unlock(&pmx->mutex);
+ pr_err("mapping already in use device %s, ID %s\n",
+ devname ? : "(anonymous)", func ? : "(undefined)");
+ goto out;
+ } else {
+ /* We already fetched this and requested pins */
+ mutex_unlock(&pmx->mutex);
+ ret = 0;
+ goto out;
+ }
+ }
+ mutex_unlock(&pmx->mutex);
+
+
+ /*
+ * Iterate over the drivers so see which ones that may handle this
+ * specific muxing. NOTE: there can be only one as of now.
+ */
+ list_for_each_entry(pmxdev, &pinmuxdev_list, node) {
+ const struct pinmux_ops *ops = pmxdev->desc->ops;
+ unsigned selector = 0;
+
+ /* See if this pmxdev has this function */
+ while (ops->list_functions(pmxdev, selector) >= 0) {
+ const char *fname = ops->get_function_name(pmxdev,
+ selector);
+
+ if (!strcmp(map->function, fname)) {
+ ret = acquire_pins(pmxdev, selector);
+ if (ret)
+ goto out;
+ /* Found it! */
+ mutex_lock(&pmx->mutex);
+ pmx->dev = dev;
+ pmx->pmxdev = pmxdev;
+ pmx->pmxdev_selector = selector;
+ mutex_unlock(&pmx->mutex);
+ ret = 0;
+ goto out;
+ }
+ selector++;
+ }
+ }
+ /* We couldn't find the driver for this pinmux */
+ ret = -ENODEV;
+
+out:
+ if (ret)
+ pmx = ERR_PTR(ret);
+
+ return pmx;
+}
+EXPORT_SYMBOL_GPL(pinmux_get);
+
+/**
+ * pinmux_put() - release a previously claimed pinmux
+ * @pmx: a pinmux previously claimed by pinmux_get()
+ */
+void pinmux_put(struct pinmux *pmx)
+{
+ if (pmx == NULL)
+ return;
+ mutex_lock(&pmx->mutex);
+ if (pmx->usecount)
+ pr_warn("pinmux: releasing pinmux with active users!\n");
+ pmx->dev = NULL;
+ pmx->pmxdev = NULL;
+ pmx->pmxdev_selector = 0;
+ mutex_unlock(&pmx->mutex);
+}
+EXPORT_SYMBOL_GPL(pinmux_put);
+
+/**
+ * pinmux_enable() - enable a certain pinmux setting
+ * @pmx: the pinmux to enable, previously claimed by pinmux_get()
+ */
+int pinmux_enable(struct pinmux *pmx)
+{
+ int ret = 0;
+
+ if (pmx == NULL)
+ return -EINVAL;
+ mutex_lock(&pmx->mutex);
+ if (pmx->usecount++ == 0) {
+ struct pinmux_dev *pmxdev = pmx->pmxdev;
+ const struct pinmux_ops *ops = pmxdev->desc->ops;
+
+ ret = ops->enable(pmxdev, pmx->pmxdev_selector);
+ if (ret)
+ pmx->usecount--;
+ }
+ mutex_unlock(&pmx->mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pinmux_enable);
+
+/**
+ * pinmux_disable() - disable a certain pinmux setting
+ * @pmx: the pinmux to disable, previously claimed by pinmux_get()
+ */
+void pinmux_disable(struct pinmux *pmx)
+{
+ if (pmx == NULL)
+ return;
+
+ mutex_lock(&pmx->mutex);
+ if (--pmx->usecount == 0) {
+ struct pinmux_dev *pmxdev = pmx->pmxdev;
+ const struct pinmux_ops *ops = pmxdev->desc->ops;
+
+ ops->disable(pmxdev, pmx->pmxdev_selector);
+ }
+ mutex_unlock(&pmx->mutex);
+}
+EXPORT_SYMBOL_GPL(pinmux_disable);
+
+/**
+ * pinmux_config() - configure a certain pinmux setting
+ * @pmx: the pinmux setting to configure
+ * @param: the parameter to configure
+ * @data: extra data to be passed to the configuration, also works as a
+ * pointer to data returned from the function on success
+ */
+int pinmux_config(struct pinmux *pmx, u16 param, unsigned long *data)
+{
+ struct pinmux_dev *pmxdev;
+ const struct pinmux_ops *ops;
+ int ret = 0;
+
+ if (pmx == NULL)
+ return -ENODEV;
+
+ pmxdev = pmx->pmxdev;
+ ops = pmxdev->desc->ops;
+
+ /* This operation is not mandatory to implement */
+ if (ops->config) {
+ mutex_lock(&pmx->mutex);
+ ret = ops->config(pmxdev, pmx->pmxdev_selector, param, data);
+ mutex_unlock(&pmx->mutex);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pinmux_config);
+
+static void pinmux_unclaim_pindescs(int first, int last)
+{
+ int i;
+
+ for (i = first; i < last; i++) {
+ struct pin_desc *pindesc;
+
+ spin_lock(&pin_desc_tree_lock);
+ pindesc = radix_tree_lookup(&pin_desc_tree, i);
+ spin_unlock(&pin_desc_tree_lock);
+ if (pindesc) {
+ /* Mark as unclaimed by drivers or functions */
+ pindesc->pmxdev = NULL;
+ pindesc->function[0] = '\0';
+ }
+ }
+}
+
+/* Helper to claim the individual descs for each pin on a pinmux device */
+static int pinmux_claim_pindescs(struct pinmux_desc *pmxdesc,
+ struct pinmux_dev *pmxdev)
+{
+ int i;
+
+ /* Put self as handler of the indicated pin range */
+ for (i = pmxdesc->base; i < (pmxdesc->base + pmxdesc->npins); i++) {
+ struct pin_desc *pindesc;
+
+ /* Check that none of the pins are already there */
+ spin_lock(&pin_desc_tree_lock);
+ pindesc = radix_tree_lookup(&pin_desc_tree, i);
+ spin_unlock(&pin_desc_tree_lock);
+ if (pindesc == NULL) {
+ dev_err(&pmxdev->dev, "pin %d is not registered, yet "
+ "attempted to add pinmux driver for it\n", i);
+ return -EINVAL;
+ }
+ if (pindesc->pmxdev != NULL) {
+ dev_err(&pmxdev->dev, "pin %d taken by other pinmux "
+ "device\n", i);
+ return -EINVAL;
+ }
+ pindesc->pmxdev = pmxdev;
+ }
+ return 0;
+}
+
+/**
+ * pinmux_register() - register a pinmux device
+ * @pmxdesc: descriptor for this pinmux
+ * @dev: parent device for this pinmux
+ * @driver_data: private pinmux data for this pinmux
+ */
+struct pinmux_dev *pinmux_register(struct pinmux_desc *pmxdesc,
+ struct device *dev, void *driver_data)
+{
+ static atomic_t pinmux_no = ATOMIC_INIT(0);
+ struct pinmux_dev *pmxdev;
+ int ret;
+
+ if (pmxdesc == NULL)
+ return ERR_PTR(-EINVAL);
+ if (pmxdesc->name == NULL || pmxdesc->ops == NULL)
+ return ERR_PTR(-EINVAL);
+ /* These functions are mandatory */
+ if (!pmxdesc->ops->list_functions ||
+ !pmxdesc->ops->get_function_name ||
+ !pmxdesc->ops->enable ||
+ !pmxdesc->ops->disable)
+ return ERR_PTR(-EINVAL);
+
+ pmxdev = kzalloc(sizeof(struct pinmux_dev), GFP_KERNEL);
+ if (pmxdev == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ mutex_lock(&pinmuxdev_list_mutex);
+ pmxdev->owner = pmxdesc->owner;
+ pmxdev->desc = pmxdesc;
+ pmxdev->driver_data = driver_data;
+
+ /* Register device with sysfs */
+ pmxdev->dev.class = &pinmux_class;
+ pmxdev->dev.parent = dev;
+ dev_set_name(&pmxdev->dev, "pinmux.%d",
+ atomic_inc_return(&pinmux_no) - 1);
+ ret = device_register(&pmxdev->dev);
+ if (ret != 0) {
+ put_device(&pmxdev->dev);
+ kfree(pmxdev);
+ goto out_err;
+ }
+ dev_set_drvdata(&pmxdev->dev, pmxdev);
+
+ ret = pinmux_claim_pindescs(pmxdesc, pmxdev);
+ if (ret)
+ goto out_err;
+
+ list_add(&pmxdev->node, &pinmuxdev_list);
+ mutex_unlock(&pinmuxdev_list_mutex);
+ return pmxdev;
+
+out_err:
+ mutex_unlock(&pinmuxdev_list_mutex);
+ put_device(&pmxdev->dev);
+ kfree(pmxdev);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(pinmux_register);
+
+/**
+ * pinmux_unregister() - unregister pinmux
+ * @pmxdev: pinmux to unregister
+ *
+ * Called by pinmux drivers to unregister a pinmux.
+ */
+void pinmux_unregister(struct pinmux_dev *pmxdev)
+{
+ if (pmxdev == NULL)
+ return;
+
+ mutex_lock(&pinmuxdev_list_mutex);
+ list_del(&pmxdev->node);
+ device_unregister(&pmxdev->dev);
+ mutex_unlock(&pinmuxdev_list_mutex);
+ pinmux_unclaim_pindescs(pmxdev->desc->base,
+ pmxdev->desc->base + pmxdev->desc->npins);
+}
+EXPORT_SYMBOL_GPL(pinmux_unregister);
+
+#ifdef CONFIG_DEBUG_FS
+
+static int pins_show(struct seq_file *s, void *what)
+{
+ unsigned pin;
+
+ seq_puts(s, "Pins:\n");
+ spin_lock(&pin_desc_tree_lock);
+ for (pin = 0; pin < num_pins; pin++) {
+ struct pin_desc *desc;
+
+ desc = radix_tree_lookup(&pin_desc_tree, pin);
+
+ seq_printf(s, "pin %d (%s)\n", pin,
+ desc->name ? desc->name : "(unnamed)");
+ }
+ spin_unlock(&pin_desc_tree_lock);
+
+ return 0;
+}
+
+static int pinmux_devices_show(struct seq_file *s, void *what)
+{
+ struct pinmux_dev *pmxdev;
+
+ seq_puts(s, "Available pinmux settings per pinmux device:\n");
+ list_for_each_entry(pmxdev, &pinmuxdev_list, node) {
+ const struct pinmux_ops *ops = pmxdev->desc->ops;
+ unsigned selector = 0;
+
+ seq_printf(s, "Device %s:\n", pmxdev->desc->name);
+ while (ops->list_functions(pmxdev, selector) >= 0) {
+ unsigned *pins;
+ unsigned num_pins;
+ const char *func = ops->get_function_name(pmxdev,
+ selector);
+ int ret;
+ int i;
+
+ ret = ops->get_function_pins(pmxdev, selector,
+ &pins, &num_pins);
+
+ if (ret)
+ seq_printf(s, "%s [ERROR GETTING PINS]\n",
+ func);
+
+ else {
+ seq_printf(s, "function: %s, pins = [ ", func);
+ for (i = 0; i < num_pins; i++)
+ seq_printf(s, "%d ", pins[i]);
+ seq_puts(s, "]\n");
+ }
+
+ selector++;
+
+ }
+ }
+
+ return 0;
+}
+
+static int pinmux_maps_show(struct seq_file *s, void *what)
+{
+ struct pinmux *pmx;
+ const struct pinmux_map *map;
+
+ seq_puts(s, "Pinmux maps:\n");
+ list_for_each_entry(pmx, &pinmux_list, node) {
+ map = pmx->map;
+
+ seq_printf(s, "map: %s -> %s\n", map->function,
+ pmx->dev ? dev_name(pmx->dev) : "(unassigned)");
+ }
+
+ return 0;
+}
+
+static int pinmux_pins_show(struct seq_file *s, void *what)
+{
+ unsigned pin;
+
+ seq_puts(s, "Pinmux settings per pin\n");
+ seq_puts(s, "Format: pin (name): pinmuxfunction pinmuxdriver\n");
+ spin_lock(&pin_desc_tree_lock);
+ for (pin = 0; pin < num_pins; pin++) {
+ struct pin_desc *desc;
+ struct pinmux_dev *pmxdev;
+
+ desc = radix_tree_lookup(&pin_desc_tree, pin);
+ pmxdev = desc->pmxdev;
+
+ seq_printf(s, "pin %d (%s): %s", pin,
+ desc->name ? desc->name : "(unnamed)",
+ desc->requested ? desc->function : "(unclaimed)");
+
+ if (pmxdev && pmxdev->desc->ops->dbg_show)
+ pmxdev->desc->ops->dbg_show(pmxdev, s,
+ pin - pmxdev->desc->base);
+
+ seq_puts(s, "\n");
+ }
+ spin_unlock(&pin_desc_tree_lock);
+
+ return 0;
+}
+
+static int pins_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, pins_show, NULL);
+}
+
+static int pinmux_devices_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, pinmux_devices_show, NULL);
+}
+
+static int pinmux_maps_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, pinmux_maps_show, NULL);
+}
+
+static int pinmux_pins_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, pinmux_pins_show, NULL);
+}
+
+static const struct file_operations pins_ops = {
+ .open = pins_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static const struct file_operations pinmux_devices_ops = {
+ .open = pinmux_devices_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static const struct file_operations pinmux_maps_ops = {
+ .open = pinmux_maps_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static const struct file_operations pinmux_pins_ops = {
+ .open = pinmux_pins_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static struct dentry *debugfs_root;
+
+static void pinctrl_init_debugfs(void)
+{
+ debugfs_root = debugfs_create_dir("pinctrl", NULL);
+ if (IS_ERR(debugfs_root) || !debugfs_root) {
+ pr_warn("failed to create debugfs directory\n");
+ debugfs_root = NULL;
+ return;
+ }
+
+ debugfs_create_file("pins", S_IFREG | S_IRUGO,
+ debugfs_root, NULL, &pins_ops);
+ debugfs_create_file("pinmux-devices", S_IFREG | S_IRUGO,
+ debugfs_root, NULL, &pinmux_devices_ops);
+ debugfs_create_file("pinmux-maps", S_IFREG | S_IRUGO,
+ debugfs_root, NULL, &pinmux_maps_ops);
+ debugfs_create_file("pinmux-pins", S_IFREG | S_IRUGO,
+ debugfs_root, NULL, &pinmux_pins_ops);
+}
+
+#else /* CONFIG_DEBUG_FS */
+
+static void pinctrl_init_debugfs(void)
+{
+}
+
+#endif
+
+static int __init pinctrl_init(void)
+{
+ int ret;
+
+ ret = class_register(&pinmux_class);
+ pr_info("initialized pinctrl subsystem\n");
+
+ pinctrl_init_debugfs();
+ return ret;
+}
+
+/* init early since many drivers really need to initialized pinmux early */
+core_initcall(pinctrl_init);
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
new file mode 100644
index 0000000..1bc29f7
--- /dev/null
+++ b/include/linux/pinctrl/machine.h
@@ -0,0 +1,57 @@
+/*
+ * Machine interface for the pinctrl subsystem.
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * Based on bits of regulator core, gpio core and clk core
+ *
+ * Author: Linus Walleij <[email protected]>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef __LINUX_PINMUX_MACHINE_H
+#define __LINUX_PINMUX_MACHINE_H
+
+/**
+ * struct pinmux_map - boards/machines shall provide this map for devices
+ * @function: a functional name for this mapping so it can be passed down
+ * to the driver to invoke that function and be referenced by this ID
+ * in e.g. pinmux_get()
+ * @dev: the device using this specific mapping, may be NULL if you provide
+ * .dev_name instead (this is more common)
+ * @dev_name: the name of the device using this specific mapping, the name
+ * must be the same that will return your struct device*
+ */
+struct pinmux_map {
+ const char *function;
+ struct device *dev;
+ const char *dev_name;
+};
+
+/* Convenience macro to set a simple map from a function to a named device */
+#define PINMUX_MAP(a, b) { .function = a, .dev_name = b }
+
+/**
+ * struct pinctrl_pin_desc - boards/machines provide information on their
+ * pins, pads or other muxable units in this struct
+ * @number: unique pin number from the global pin number space
+ * @name: a name for this pin
+ */
+struct pinctrl_pin_desc {
+ unsigned number;
+ const char *name;
+};
+
+/* Convenience macro to define a single named or anonymous pin descriptor */
+#define PINCTRL_PIN(a, b) { .number = a, .name = b }
+#define PINCTRL_PIN_ANON(a) { .number = a }
+
+extern int pinctrl_register_pins_sparse(struct pinctrl_pin_desc const *pins,
+ unsigned num_descs);
+extern int pinctrl_register_pins_dense(struct pinctrl_pin_desc const *pins,
+ unsigned num_descs, unsigned num_pins);
+extern int pinctrl_register_anon_pins(unsigned first, unsigned last);
+extern int pinmux_register_mappings(struct pinmux_map const *map,
+ unsigned num_maps);
+
+#endif
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
new file mode 100644
index 0000000..7b40893
--- /dev/null
+++ b/include/linux/pinctrl/pinmux.h
@@ -0,0 +1,180 @@
+/*
+ * Interface the pinmux subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * Based on bits of regulator core, gpio core and clk core
+ *
+ * Author: Linus Walleij <[email protected]>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef __LINUX_PINCTRL_PINMUX_H
+#define __LINUX_PINCTRL_PINMUX_H
+
+#include <linux/list.h>
+#include <linux/seq_file.h>
+
+struct pinmux;
+
+#ifdef CONFIG_PINCTRL
+
+struct pinmux_dev;
+
+/**
+ * struct pinmux_ops - pinmux operations, to be implemented by drivers
+ * @request: called by the core to see if a certain pin can be muxed in
+ * and made available in a certain mux setting The driver is allowed
+ * to answer "no" by returning a negative error code
+ * @list_functions: list the number of selectable named functions available
+ * in this pinmux driver, the core will begin on 0 and call this
+ * repeatedly as long as it returns >= 0 to enumerate mux settings
+ * @get_function_name: return the function name of the muxing selector,
+ * called by the core to figure out which mux setting it shall map a
+ * certain device to
+ * @get_function_pins: return an array of pins corresponding to a certain
+ * function selector in @pins, and the size of the array in @num_pins
+ * @enable: enable a certain muxing enumerator. The driver does not need to
+ * figure out whether enabling this function conflicts some other use
+ * of the pins, such collisions are handled by the pinmux subsystem
+ * @disable: disable a certain muxing enumerator
+ * @config: custom configuration function for a certain muxing enumerator -
+ * this works a bit like an ioctl() and can pass in and return arbitrary
+ * configuration data to the pinmux
+ * @gpio_request_enable: requests and enables GPIO on a certain pin.
+ * Implement this only if you can mux every pin individually as GPIO. If
+ * your gpio assignments are grouped, so you cannot control the GPIO
+ * muxing of every indvidual pin.
+ * @dbg_show: optional debugfs display hook that will provide per-device
+ * info for a certain pin in debugfs
+ */
+struct pinmux_ops {
+ int (*request) (struct pinmux_dev *pmxdev, unsigned offset);
+ int (*free) (struct pinmux_dev *pmxdev, unsigned offset);
+ int (*list_functions) (struct pinmux_dev *pmxdev, unsigned selector);
+ const char *(*get_function_name) (struct pinmux_dev *pmxdev,
+ unsigned selector);
+ int (*get_function_pins) (struct pinmux_dev *pmxdev, unsigned selector,
+ unsigned ** const pins, unsigned * const num_pins);
+ int (*enable) (struct pinmux_dev *pmxdev, unsigned selector);
+ void (*disable) (struct pinmux_dev *pmxdev, unsigned selector);
+ int (*config) (struct pinmux_dev *pmxdev, unsigned selector,
+ u16 param, unsigned long *data);
+ int (*gpio_request_enable) (struct pinmux_dev *pmxdev, unsigned offset);
+ void (*dbg_show) (struct pinmux_dev *pmxdev, struct seq_file *s,
+ unsigned offset);
+};
+
+/**
+ * struct pinmux_desc - pinmux descriptor, register this to pinmux subsystem
+ * @name: name for the pinmux
+ * @ops: pinmux operation table
+ * @owner: module providing the pinmux, used for refcounting
+ * @base: the number of the first pin handled by this pinmux, in the global
+ * pin space, subtracted from a given pin to get the offset into the range
+ * of a certain pinmux
+ * @npins: the number of pins handled by this pinmux - note that
+ * this is the number of possible pin settings, if your driver handles
+ * 8 pins that each can be muxed in 3 different ways, you reserve 24
+ * pins in the global pin space and set this to 24
+ */
+struct pinmux_desc {
+ const char *name;
+ struct pinmux_ops *ops;
+ struct module *owner;
+ int base;
+ int npins;
+};
+
+/**
+ * struct pinmux_dev - pinmux class device
+ * @desc: the descriptor supplied when initializing this pinmux
+ * @node: node to include this pinmux in the global pinmux list
+ * @dev: the device entry for this pinmux
+ * @owner: module providing the pinmux, used for refcounting
+ * @driver_data: driver data for drivers registering to the subsystem
+ *
+ * This should be dereferenced and used by the pinmux core ONLY
+ */
+struct pinmux_dev {
+ struct pinmux_desc *desc;
+ struct list_head node;
+ struct device dev;
+ struct module *owner;
+ void *driver_data;
+};
+
+/* These should only be used from drives */
+static inline const char *pmxdev_get_name(struct pinmux_dev *pmxdev)
+{
+ /* We're not allowed to register devices without name */
+ return pmxdev->desc->name;
+}
+
+static inline void *pmxdev_get_drvdata(struct pinmux_dev *pmxdev)
+{
+ return pmxdev->driver_data;
+}
+
+/* External interface to pinmux */
+extern int pin_is_valid(int pin);
+extern int pinmux_request_gpio(int pin, unsigned gpio);
+extern void pinmux_free_gpio(int pin);
+extern struct pinmux *pinmux_get(struct device *dev, const char *func);
+extern void pinmux_put(struct pinmux *pmx);
+extern int pinmux_enable(struct pinmux *pmx);
+extern void pinmux_disable(struct pinmux *pmx);
+extern int pinmux_config(struct pinmux *pmx, u16 param, unsigned long *data);
+extern struct pinmux_dev *pinmux_register(struct pinmux_desc *pmxdesc,
+ struct device *dev, void *driver_data);
+extern void pinmux_unregister(struct pinmux_dev *pmxdev);
+
+#else /* !CONFIG_PINMUX */
+
+static inline int pin_is_valid(int pin)
+{
+ return pin >= 0;
+}
+
+static inline int pinmux_request_gpio(int pin, unsigned gpio)
+{
+ return 0;
+}
+
+static inline void pinmux_free_gpio(int pin)
+{
+}
+
+static inline int pinmux_register_mappings(struct pinmux_map const *map,
+ unsigned num_maps)
+{
+ return 0;
+}
+
+static inline struct pinmux *pinmux_get(struct device *dev, const char *func)
+{
+ return NULL;
+}
+
+static inline void pinmux_put(struct pinmux *pmx)
+{
+}
+
+static inline int pinmux_enable(struct pinmux *pmx)
+{
+ return 0;
+}
+
+static inline void pinmux_disable(struct pinmux *pmx)
+{
+}
+
+static inline int pinmux_config(struct pinmux *pmx, u16 param,
+ unsigned long *data)
+{
+ return 0;
+}
+
+#endif /* CONFIG_PINMUX */
+
+#endif /* __LINUX_PINCTRL_PINMUX_H */
--
1.7.3.2
On Mon, 2011-06-13 at 18:58 +0200, Linus Walleij wrote:
> This creates a subsystem for handling of pinmux devices. These are
> devices that enable and disable groups of pins on primarily PGA and
> BGA type of chip packages and common in embedded systems.
Trivia only:
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
[]
> +int pin_is_valid(int pin)
> +{
> + return pin >= 0 && pin < num_pins;
> +}
> +EXPORT_SYMBOL_GPL(pin_is_valid);
bool pin_is_valid?
> +/* Deletes a range of pin descriptors */
> +static void pinctrl_free_pindescs(struct pinctrl_pin_desc const *pins,
> + unsigned num_pins)
const struct pinctrl_pin_desc *pins
> +{
> + int i;
> +
> + for (i = 0; i < num_pins; i++) {
> + struct pin_desc *pindesc;
> +
> + spin_lock(&pin_desc_tree_lock);
> + pindesc = radix_tree_lookup(&pin_desc_tree, pins[i].number);
> + if (pindesc != NULL) {
> + radix_tree_delete(&pin_desc_tree, pins[i].number);
> + num_pins --;
No space please
> + }
> + spin_unlock(&pin_desc_tree_lock);
> + kfree(pindesc);
> + }
> +}
Is it really worthwhile to have spin_lock/unlock in the loop?
> +static int pinctrl_register_one_pin(unsigned number, const char *name)
> +{
> + /* Copy optional basic pin info */
> + if (name) {
> + strncpy(pindesc->name, name, 16);
strlcpy
> + pindesc->name[15] = '\0';
> + }
> +
> + spin_lock(&pin_desc_tree_lock);
> + radix_tree_insert(&pin_desc_tree, number, pindesc);
> + num_pins ++;
No space please
> + spin_unlock(&pin_desc_tree_lock);
> + return 0;
> +}
> +
> +/* Passing in 0 num_pins means "sparse" */
> +static int pinctrl_register_pins(struct pinctrl_pin_desc const *pins,
> + unsigned num_descs, unsigned num_pins)
[]
> + * If we are registerering dense pinlists, fill in all holes with
registering
> + * anonymous pins.
> + */
> + for (i = 0; i < num_pins; i++) {
> + char pinname[16];
> + struct pin_desc *pindesc;
> +
> + spin_lock(&pin_desc_tree_lock);
> + pindesc = radix_tree_lookup(&pin_desc_tree, i);
> + spin_unlock(&pin_desc_tree_lock);
> + /* Already registered this one, take next */
> + if (pindesc)
> + continue;
> +
> + snprintf(pinname, 15, "anonymous %u", i);
> + pinname[15] = '\0';
strlcpy
> +int pinctrl_register_pins_dense(struct pinctrl_pin_desc const *pins,
> + unsigned num_descs, unsigned num_pins)
> +{
> + int ret;
> + unsigned i;
> +
> + ret = pinctrl_register_pins(pins, num_descs, num_pins);
> + if (ret) {
> + for (i = 0; i < num_pins; i++) {
> + struct pin_desc *pindesc;
> +
> + spin_lock(&pin_desc_tree_lock);
> + pindesc = radix_tree_lookup(&pin_desc_tree, i);
> + if (pindesc != NULL) {
> + radix_tree_delete(&pin_desc_tree, i);
> + num_pins --;
> + }
> + spin_unlock(&pin_desc_tree_lock);
> + kfree(pindesc);
> + }
Second use of this pattern. Maybe use pinctrl_free_pindescs?
On Mon, Jun 13, 2011 at 10:58 AM, Linus Walleij
<[email protected]> wrote:
> From: Linus Walleij <[email protected]>
>
> This creates a subsystem for handling of pinmux devices. These are
> devices that enable and disable groups of pins on primarily PGA and
> BGA type of chip packages and common in embedded systems.
>
> This is being done to depopulate the arch/arm/* directory of such
> custom drivers and try to abstract the infrastructure they all
> need. See the Documentation/pinmux.txt file that is part of this
> patch for more details.
Hi Linus,
Overall (but without a deep dive into the implementation details) I
think I generally like the approach. To sum up, it looks like the
conceptual model is thus:
- A pinmux driver enumerates and registers all the pins that it has
- Setup code and/or driver code requests blocks of pins (functions)
when it needs them
- If all the pins are available, it gets them, otherwise it the allocation fails
- pins cannot be claimed by more than one device
- it is up to the pinmux driver to actually program the device and
determine whether or not the requested pin mode is actually
configurable. Even if pins are available, it may be that other
constraints prevent it from actually being programmed
Ultimately, it still is up to the board designer and board port
engineer to ensure that the system can actually provide the requested
pin configuration.
My understanding is that in the majority of cases pinmux will probably
want/need to be setup and machine_init time, and device drivers won't
really know or care about pinmux; it will already be set up for them
when the driver is probed. Any power management issues will also be
handled by platform/soc code when the dependent devices are in PM
states.
How does that line up with your conceptual model of pinmux?
Other comments below.
>
> Cc: Grant Likely <[email protected]>
> Cc: Stephen Warren <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Russell King <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> ChangeLog v2->v3:
> - Renamed subsystem folder to "pinctrl" since we will likely
> ?want to keep other pin control such as biasing in this
> ?subsystem too, so let us keep to something generic even though
> ?we're mainly doing pinmux now.
> - As a consequence, register pins as an abstract entity separate
> ?from the pinmux. The muxing functions will claim pins out of the
> ?pin pool and make sure they do not collide. Pins can now be
> ?named by the pinctrl core.
> - Converted the pin lookup from a static array into a radix tree,
> ?I agreed with Grant Likely to try to avoid any static allocation
> ?(which is crap for device tree stuff) so I just rewrote this
> ?to be dynamic, just like irq number descriptors. The
> ?platform-wide definition of number of pins goes away - this is
> ?now just the sum total of the pins registered to the subsystem.
You should consider still using a bitmap for tracking which pins are
actually available, similar to how irqs are tracked. A bool in each
pinmux structure is a little wasteful, and requires a lock to be held
for a long time while checking all the structures.
> - Make sure mappings with only a function name and no device
> ?works properly.
> +Pinmux drivers
> +==============
> +
> +The driver will for all calls be provided an offset pin number into its own
> +pin range. If you have 2 chips with 8x8 pins, the first chips pins will have
> +numbers 0 thru 63 and the second one pins 64 thru 127, but the driver for the
> +second chip will be passed numbers in the range 0 thru 63 anyway, base offset
> +subtracted.
Wait, do you really want a global numberspace here? I'd rather see
callers have a direct reference to the pinmux controller instance, and
use local pin numbers. Given the choice, I would not go with global
numbers for GPIOs again, and I'm not so big a fan of them for irqs
either.
> +
> +Pinmux drivers are required to supply a few callback functions, some are
> +optional. Usually the enable() and disable() functions are implemented,
> +writing values into some certain registers to activate a certain mux setting
> +for a certain pin.
> +
> +A simple driver for the above example will work by setting bits 0, 1 or 2
> +into some register mamed MUX, so it enumerates its available settings and
> +their pin assignments, and expose them like this:
> +
> +#include <linux/pinctrl/pinmux.h>
> +
> +struct foo_pmx_func {
> + ? ? ? char *name;
> + ? ? ? const unsigned int *pins;
> + ? ? ? const unsigned num_pins;
> +};
> +
> +static unsigned int spi0_0_pins[] = { 0, 8, 16, 24 };
> +static unsigned int i2c0_pins[] = { 24, 25 };
> +static unsigned int spi0_1_pins[] = { 38, 46, 54, 62 };
> +
> +static struct foo_pmx_func myfuncs[] = {
> + ? ? ? {
> + ? ? ? ? ? ? ? .name = "spi0-0",
> + ? ? ? ? ? ? ? .pins = spi0_0_pins,
> + ? ? ? ? ? ? ? .num_pins = ARRAY_SIZE(spi0_1_pins),
> + ? ? ? },
> + ? ? ? {
> + ? ? ? ? ? ? ? .name = "i2c0",
> + ? ? ? ? ? ? ? .pins = i2c0_pins,
> + ? ? ? ? ? ? ? .num_pins = ARRAY_SIZE(i2c0_pins),
> + ? ? ? },
> + ? ? ? {
> + ? ? ? ? ? ? ? .name = "spi0-1",
> + ? ? ? ? ? ? ? .pins = spi0_1_pins,
> + ? ? ? ? ? ? ? .num_pins = ARRAY_SIZE(spi0_1_pins),
> + ? ? ? },
> +};
> +
> +int foo_list(struct pinmux_dev *pmxdev, unsigned selector)
> +{
> + ? ? ? if (selector >= ARRAY_SIZE(myfuncs))
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? return 0;
> +}
> +
> +const char *foo_get_fname(struct pinmux_dev *pmxdev, unsigned selector)
> +{
> + ? ? ? if (selector >= ARRAY_SIZE(myfuncs))
> + ? ? ? ? ? ? ? return NULL;
> + ? ? ? return myfuncs[selector].name;
> +}
Is there a method to lookup the function id from the name? Going from
name to number seems more useful to me than going the other way
around.
> +
> +static int foo_get_pins(struct pinmux_dev *pmxdev, unsigned selector,
> + ? ? ? ? ? ? ? ? ? ? ? unsigned ** const pins, unsigned * const num_pins)
> +{
> + ? ? ? if (selector >= ARRAY_SIZE(myfuncs))
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? *pins = myfuncs[selector].pins;
> + ? ? ? *num_pins = myfuncs[selector].num_pins;
> + ? ? ? return 0;
> +}
> +
> +
> +int foo_enable(struct pinmux_dev *pmxdev, unsigned selector)
> +{
> + ? ? ? if (selector < ARRAY_SIZE(myfuncs))
> + ? ? ? ? ? ? ? write((read(MUX)|(1<<selector)), MUX)
> + ? ? ? ? ? ? ? return 0;
> + ? ? ? }
> + ? ? ? return -EINVAL;
> +}
> +
> +int foo_disable(struct pinmux_dev *pmxdev, unsigned selector)
> +{
> + ? ? ? if (selector < ARRAY_SIZE(myfuncs))
> + ? ? ? ? ? ? ? write((read(MUX) & ~(1<<selector)), MUX)
> + ? ? ? ? ? ? ? return 0;
> + ? ? ? }
> + ? ? ? return -EINVAL;
> +}
> +
> +struct pinmux_ops ops = {
> + ? ? ? .list_functions = foo_list,
> + ? ? ? .get_function_name = foo_get_fname,
> + ? ? ? .get_function_pins = foo_get_pins,
> + ? ? ? .enable = foo_enable,
> + ? ? ? .disable = foo_disable,
Mixing callbacks with data here. Not bad, but maybe a little odd.
> +};
> +
> +Now the able reader will say: "wait - the driver needs to make sure it
> +can set this and that bit at the same time, because else it will collide
> +and wreak havoc in my electronics, and make sure noone else is using the
> +other setting that it's incompatible with".
> +
> +In the example activating muxing 0 and 1 at the same time setting bits
> +0 and 1, uses one pin in common so they would collide.
> +
> +The beauty of the pinmux subsystem is that since it keeps track of all
> +pins and who is using them, it will already have denied an impossible
> +request like that, so the driver does not need to worry about such
> +things - when it gets a selector passed in, the pinmux subsystem makes
> +sure no other device or GPIO assignment is already using the selected
> +pins.
Sometimes that isn't enough. Some functions may not actually collide
on the pins they select, but the modes will be mutually exclusive
anyway. There needs to be runtime checking that the mode can actually
be programmed when it is enabled (of course, it may just be that for
*this* example it doesn't need to worry about it, in which case my
comment is moot).
> +
> +The above functions are mandatory to implement for a pinmux driver.
> +
> +The function list could become long, especially if you can convert every
> +individual pin into a GPIO pin independent of any other pins, then your
> +function array can become 64 entries for each GPIO setting and then the
> +device functions. For this reason there is an additional function you
> +can implement to enable only GPIO on an individual pin: gpio_enable().
> +
> +
> +Pinmux board/machine configuration
> +==================================
> +
> +Boards and machines define how a certain complete running system is put
> +together, including how GPIOs and devices are muxed, how regulators are
> +constrained and how the clock tree looks. Of course pinmux settings are also
> +part of this.
> +
> +A pinmux config for a machine looks pretty much like a simple regulator
> +configuration, so for the example array above we want to enable i2c and
> +spi on the second function mapping:
> +
> +#include <linux/pinctrl/machine.h>
> +
> +static struct pinmux_map pmx_mapping[] = {
> + ? ? ? {
> + ? ? ? ? ? ? ? .function = "spi0-1",
> + ? ? ? ? ? ? ? .dev_name = "foo-spi.0",
> + ? ? ? },
> + ? ? ? {
> + ? ? ? ? ? ? ? .function = "i2c0",
> + ? ? ? ? ? ? ? .dev_name = "foo-i2c.0",
> + ? ? ? },
> +};
I'm wary about this approach, even though I know it is already used by
regulator and clk mappings. The reason I'm wary is that matching
devices by name becomes tricky for anything that isn't statically
created by the kernel, such as when enumerating from the device tree,
because it assumes that the device name is definitively known.
Specifically, Linux's preferred name for a device is not guaranteed to
be available from the device tree. We very purposefully do not encode
Linux kernel implementation details into the kernel so that
implementation detail changes don't force dt changes.
/me goes and thinks about the problem some more...
Okay, I think I've got a new approach for the DT domain so that Linux
gets the device names it wants for matching to clocks, regulators and
this stuff. I'm going to go and code it up now. I still don't
personally like matching devices by name, but by no measure is it a
show stopper for me.
> +
> +Since the above construct is pretty common there is a helper macro to make
> +it even more compact:
> +
> +static struct pinmux_map pmx_mapping[] = {
> + ? ? ? PINMUX_MAP("spi0-1", "foo-spi.0"),
> + ? ? ? PINMUX_MAP("i2c0", "foo-i2c.0"),
> +};
> +
> +The dev_name here matches to the unique device name that can be used to look
> +up the device struct (just like with clockdev or regulators). The function name
> +must match a function provided by the pinmux driver handling this pin range.
> +You register this pinmux mapping to the pinmux subsystem by simply:
> +
> + ? ? ? ret = pinmux_register_mappings(&pmx_mapping, ARRAY_SIZE(pmx_mapping));
> +
> +
> +Pinmux requests from drivers
> +============================
> +
> +A driver may request a certain mux to be activated, usually just the default
> +mux like this:
> +
> +#include <linux/pinctrl/pinmux.h>
> +
> +foo_probe()
> +{
> + ? ? ? /* Allocate a state holder named "state" etc */
> + ? ? ? struct pinmux pmx;
> +
> + ? ? ? pmx = pinmux_get(&device, NULL);
> + ? ? ? if IS_ERR(pmx)
> + ? ? ? ? ? ? ? return PTR_ERR(pmx);
> + ? ? ? pinmux_enable(pmx);
> +
> + ? ? ? state->pmx = pmx;
> +}
> +
> +foo_remove()
> +{
> + ? ? ? pinmux_disable(state->pmx);
> + ? ? ? pinmux_put(state->pmx);
> +}
> +
> +If you want a specific mux setting and not just the first one found for this
> +device you can specify a specific mux setting, for example in the above example
> +the second i2c0 setting: pinmux_get(&device, "spi0-2");
> +
> +This get/enable/disable/put sequence can just as well be handled by bus drivers
> +if you don't want each and every driver to handle it and you know the
> +arrangement on your bus.
I would *strongly* recommend against individual device drivers
accessing the pinmux api. This is system level configuration code,
and should be handled at the system level.
> +
> +The pins are allocated for your device when you issue the pinmux_get() call,
> +after this you should be able to see this in the debugfs listing of all pins.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 29801f7..5caea5a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4933,6 +4933,11 @@ L: ? ? ? [email protected]
> ?S: ? ? Maintained
> ?F: ? ? drivers/mtd/devices/phram.c
>
> +PINMUX SUBSYSTEM
> +M: ? ? Linus Walleij <[email protected]>
> +S: ? ? Maintained
> +F: ? ? drivers/pinmux/
> +
> ?PKTCDVD DRIVER
> ?M: ? ? Peter Osterlund <[email protected]>
> ?S: ? ? Maintained
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 3bb154d..6998d78 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -56,6 +56,10 @@ source "drivers/pps/Kconfig"
>
> ?source "drivers/ptp/Kconfig"
>
> +# pinctrl before gpio - gpio drivers may need it
GPIO controllers are just other devices, I don't think there is
anything special here when compared with SPI or I2C. I don't think
gpio drivers should be accessing the pinmux api directly.
In my mind, the gpio layer is only about abstracting the gpio control
interface to drivers. Whether or not or how the pin is routed outside
the chip package is irrelevant to the driver or the gpio api.
Besides, this is kconfig. The order of this file has zero bearing on
the resultant kernel. It does matter for the Makefile though.
> +
> +source "drivers/pinctrl/Kconfig"
> +
> ?source "drivers/gpio/Kconfig"
>
> ?source "drivers/w1/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 09f3232..a590a01 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -5,6 +5,8 @@
> ?# Rewritten to use lists instead of if-statements.
> ?#
>
> +# GPIO must come after pinctrl as gpios may need to mux pins etc
> +obj-y ? ? ? ? ? ? ? ? ? ? ? ? ?+= pinctrl/
> ?obj-y ? ? ? ? ? ? ? ? ? ? ? ? ?+= gpio/
> ?obj-$(CONFIG_PCI) ? ? ? ? ? ? ?+= pci/
> ?obj-$(CONFIG_PARISC) ? ? ? ? ? += parisc/
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> new file mode 100644
> index 0000000..8050fdf
> --- /dev/null
> +++ b/drivers/pinctrl/Kconfig
> @@ -0,0 +1,29 @@
> +#
> +# PINCTRL infrastructure and drivers
> +#
> +
> +menuconfig PINCTRL
> + ? ? ? bool "PINCTRL Support"
> + ? ? ? depends on SYSFS && EXPERIMENTAL
Hold off on the sysfs stuff. Lay down the basic API without sysfs,
and add the sysfs bits as a separate patch. This becomes a
kernel->userspace abi issue, and you don't want to mess that up.
Is a pinmux sysfs abi really needed? What is the use-case?
> + ? ? ? help
> + ? ? ? ? This enables the PINCTRL subsystem for controlling pins
> + ? ? ? ? on chip packages, for example multiplexing pins on primarily
> + ? ? ? ? PGA and BGA packages for systems on chip.
> +
> + ? ? ? ? If unsure, say N.
> +
> +if PINCTRL
> +
> +config DEBUG_PINCTRL
> + ? ? ? bool "Debug PINCTRL calls"
> + ? ? ? depends on DEBUG_KERNEL
> + ? ? ? help
> + ? ? ? ? Say Y here to add some extra checks and diagnostics to PINCTRL calls.
> +
> +config PINMUX_U300
> + ? ? ? bool "U300 pinmux driver"
> + ? ? ? depends on ARCH_U300
> + ? ? ? help
> + ? ? ? ? Say Y here to enable the U300 pinmux driver
> +
PINMUX_U300 should not be here in the infrastructure patch.
> +endif
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> new file mode 100644
> index 0000000..44d8933
> --- /dev/null
> +++ b/drivers/pinctrl/Makefile
> @@ -0,0 +1,6 @@
> +# generic pinmux support
> +
> +ccflags-$(CONFIG_DEBUG_PINMUX) += -DDEBUG
> +
> +obj-$(CONFIG_PINCTRL) ? ? ? ? ?+= core.o
Consider calling this pinmux.o; particularly if there is ever a chance
of it becoming a module. It is conceivable that pinmux will be used
for peripheral chips in a way that can/should be loaded at runtime.
> +obj-$(CONFIG_PINMUX_U300) ? ? ?+= pinmux-u300.o
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> new file mode 100644
> index 0000000..8fd1437
> --- /dev/null
> +++ b/drivers/pinctrl/core.c
> @@ -0,0 +1,1028 @@
> +/*
> + * Core driver for the pinmux subsystem
> + *
> + * Copyright (C) 2011 ST-Ericsson SA
> + * Written on behalf of Linaro for ST-Ericsson
> + * Based on bits of regulator core, gpio core and clk core
> + *
> + * Author: Linus Walleij <[email protected]>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +#define pr_fmt(fmt) "pinctrl core: " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/radix-tree.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <linux/sysfs.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +/* Global list of pinmuxes */
> +static DEFINE_MUTEX(pinmux_list_mutex);
> +static LIST_HEAD(pinmux_list);
> +
> +/* Global list of pinmux devices */
> +static DEFINE_MUTEX(pinmuxdev_list_mutex);
> +static LIST_HEAD(pinmuxdev_list);
> +
> +/**
> + * struct pin_desc - pin descriptor for each physical pin in the arch
> + * @pmxdev: corresponding pinmux device
> + * @requested: whether the pin is already requested by pinmux or not
> + * @name: a name for the pin, e.g. the name of the pin/pad/finger on a
> + * ? ? datasheet or such
> + * @function: a named muxing function for the pin that will be passed to
> + * ? ? subdrivers and shown in debugfs etc
> + */
> +struct pin_desc {
> + ? ? ? struct pinmux_dev *pmxdev;
> + ? ? ? bool ? ?requested;
> + ? ? ? char ? ?name[16];
> + ? ? ? char ? ?function[16];
> +};
> +/* Global lookup of per-pin descriptors, one for each physical pin */
> +static DEFINE_SPINLOCK(pin_desc_tree_lock);
> +static RADIX_TREE(pin_desc_tree, GFP_KERNEL);
The radix tree should probably be per-pinmux controller local. Of
course, if you make all the pinmux numbering local to the controller,
then the need for a radix tree could very well go away entirely, and
it would simplify everything.
> +static unsigned int num_pins = 0;
> +
> +/**
> + * struct pinmux - per-device pinmux state holder
> + * @node: global list node - only for internal use
> + * @dev: the device using this pinmux
> + * @pmxdev: the pinmux device controlling this pinmux
> + * @map: corresponding pinmux map active for this pinmux setting
> + * @usecount: the number of active users of this mux setting, used to keep
> + * ? ? track of nested use cases
> + * @pins: an array of discrete physical pins used in this mapping, taken
> + * ? ? from the global pin enumeration space (copied from pinmux map)
> + * @num_pins: the number of pins in this mapping array, i.e. the number of
> + * ? ? elements in .pins so we can iterate over that array (copied from
> + * ? ? pinmux map)
> + * @pmxdev: pinmux device handling this pinmux
> + * @pmxdev_selector: the selector for the pinmux device handling this pinmux
> + * @mutex: a lock for the pinmux state holder
> + */
> +struct pinmux {
> + ? ? ? struct list_head node;
> + ? ? ? struct device *dev;
> + ? ? ? struct pinmux_map const *map;
> + ? ? ? unsigned usecount;
> + ? ? ? struct pinmux_dev *pmxdev;
> + ? ? ? unsigned pmxdev_selector;
> + ? ? ? struct mutex mutex;
> +};
> +
> +int pin_is_valid(int pin)
> +{
> + ? ? ? return pin >= 0 && pin < num_pins;
> +}
> +EXPORT_SYMBOL_GPL(pin_is_valid);
A "pin_" prefix is very generic sounding. Though it doesn't read as
well, the pinmux_ prefix should probably be used consistently.
> +
> +static ssize_t pinmux_name_show(struct device *dev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct device_attribute *attr, char *buf)
> +{
> + ? ? ? struct pinmux_dev *pmxdev = dev_get_drvdata(dev);
> +
> + ? ? ? return sprintf(buf, "%s\n", pmxdev_get_name(pmxdev));
> +}
> +
> +static struct device_attribute pinmux_dev_attrs[] = {
> + ? ? ? __ATTR(name, 0444, pinmux_name_show, NULL),
> + ? ? ? __ATTR_NULL,
> +};
> +
> +static void pinmux_dev_release(struct device *dev)
> +{
> + ? ? ? struct pinmux_dev *pmxdev = dev_get_drvdata(dev);
> + ? ? ? kfree(pmxdev);
> +}
> +
> +static struct class pinmux_class = {
> + ? ? ? .name = "pinmux",
> + ? ? ? .dev_release = pinmux_dev_release,
> + ? ? ? .dev_attrs = pinmux_dev_attrs,
> +};
> +
> +/* Deletes a range of pin descriptors */
> +static void pinctrl_free_pindescs(struct pinctrl_pin_desc const *pins,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned num_pins)
> +{
> + ? ? ? int i;
> +
> + ? ? ? for (i = 0; i < num_pins; i++) {
> + ? ? ? ? ? ? ? struct pin_desc *pindesc;
> +
> + ? ? ? ? ? ? ? spin_lock(&pin_desc_tree_lock);
> + ? ? ? ? ? ? ? pindesc = radix_tree_lookup(&pin_desc_tree, pins[i].number);
> + ? ? ? ? ? ? ? if (pindesc != NULL) {
> + ? ? ? ? ? ? ? ? ? ? ? radix_tree_delete(&pin_desc_tree, pins[i].number);
> + ? ? ? ? ? ? ? ? ? ? ? num_pins --;
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? spin_unlock(&pin_desc_tree_lock);
> + ? ? ? ? ? ? ? kfree(pindesc);
> + ? ? ? }
> +}
> +
> +static int pinctrl_register_one_pin(unsigned number, const char *name)
> +{
> + ? ? ? struct pin_desc *pindesc;
> +
> + ? ? ? spin_lock(&pin_desc_tree_lock);
> + ? ? ? pindesc = radix_tree_lookup(&pin_desc_tree, number);
> + ? ? ? spin_unlock(&pin_desc_tree_lock);
> +
> + ? ? ? if (pindesc != NULL) {
> + ? ? ? ? ? ? ? pr_err("pin %d already registered\n", number);
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? pindesc = kzalloc(sizeof(*pindesc), GFP_KERNEL);
> + ? ? ? if (pindesc == NULL)
> + ? ? ? ? ? ? ? return -ENOMEM;
> +
> + ? ? ? /* Copy optional basic pin info */
> + ? ? ? if (name) {
> + ? ? ? ? ? ? ? strncpy(pindesc->name, name, 16);
> + ? ? ? ? ? ? ? pindesc->name[15] = '\0';
> + ? ? ? }
> +
> + ? ? ? spin_lock(&pin_desc_tree_lock);
> + ? ? ? radix_tree_insert(&pin_desc_tree, number, pindesc);
> + ? ? ? num_pins ++;
> + ? ? ? spin_unlock(&pin_desc_tree_lock);
> + ? ? ? return 0;
> +}
> +
> +/* Passing in 0 num_pins means "sparse" */
> +static int pinctrl_register_pins(struct pinctrl_pin_desc const *pins,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned num_descs, unsigned num_pins)
> +{
> + ? ? ? unsigned i;
> + ? ? ? int ret = 0;
> +
> + ? ? ? for (i = 0; i < num_descs; i++) {
> + ? ? ? ? ? ? ? ret = pinctrl_register_one_pin(pins[i].number, pins[i].name);
> + ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? return ret;
> + ? ? ? }
> +
> + ? ? ? if (num_pins == 0)
> + ? ? ? ? ? ? ? return 0;
> +
> + ? ? ? /*
> + ? ? ? ?* If we are registerering dense pinlists, fill in all holes with
> + ? ? ? ?* anonymous pins.
> + ? ? ? ?*/
> + ? ? ? for (i = 0; i < num_pins; i++) {
> + ? ? ? ? ? ? ? char pinname[16];
> + ? ? ? ? ? ? ? struct pin_desc *pindesc;
> +
> + ? ? ? ? ? ? ? spin_lock(&pin_desc_tree_lock);
> + ? ? ? ? ? ? ? pindesc = radix_tree_lookup(&pin_desc_tree, i);
> + ? ? ? ? ? ? ? spin_unlock(&pin_desc_tree_lock);
> + ? ? ? ? ? ? ? /* Already registered this one, take next */
> + ? ? ? ? ? ? ? if (pindesc)
> + ? ? ? ? ? ? ? ? ? ? ? continue;
> +
> + ? ? ? ? ? ? ? snprintf(pinname, 15, "anonymous %u", i);
> + ? ? ? ? ? ? ? pinname[15] = '\0';
> +
> + ? ? ? ? ? ? ? ret = pinctrl_register_one_pin(i, pinname);
> + ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? return ret;
> + ? ? ? }
> +
> + ? ? ? return 0;
> +}
> +
> +/**
> + * pinctrl_register_pins_sparse() - register a range of pins for a
> + * ? ? board/machine with potential holes in the pin map. The pins in
> + * ? ? the holes will not be usable.
> + * @pins: a range of pins to register
> + * @num_descs: the number of pins descriptors passed in through the previous
> + * ? ? pointer
> + */
> +int pinctrl_register_pins_sparse(struct pinctrl_pin_desc const *pins,
> + ? ? ? ? ? ? ? ? ? ? ? ? unsigned num_descs)
> +{
> + ? ? ? int ret;
> +
> + ? ? ? ret = pinctrl_register_pins(pins, num_descs, 0);
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? pinctrl_free_pindescs(pins, num_descs);
> + ? ? ? return ret;
> +
> +}
> +EXPORT_SYMBOL_GPL(pinctrl_register_pins);
> +
> +/**
> + * pinctrl_register_pins_dense() - register a range of pins for a
> + * ? ? board/machine, if there are holes in the pin map, they will be
> + * ? ? allocated by anonymous pins.
> + * @pins: a range of pins to register
> + * @num_descs: the number of pins descriptors passed in through the previous
> + * ? ? pointer
> + * @num_pins: the total number of pins including holes in the pin map and
> + * ? ? any "air" at the end of the map, all pins from 0 to this number
> + * ? ? will be allocated, the ones that does not have descriptors passed
> + * ? ? in will be marked "anonymous"
> + */
> +int pinctrl_register_pins_dense(struct pinctrl_pin_desc const *pins,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned num_descs, unsigned num_pins)
> +{
> + ? ? ? int ret;
> + ? ? ? unsigned i;
> +
> + ? ? ? ret = pinctrl_register_pins(pins, num_descs, num_pins);
> + ? ? ? if (ret) {
> + ? ? ? ? ? ? ? for (i = 0; i < num_pins; i++) {
> + ? ? ? ? ? ? ? ? ? ? ? struct pin_desc *pindesc;
> +
> + ? ? ? ? ? ? ? ? ? ? ? spin_lock(&pin_desc_tree_lock);
> + ? ? ? ? ? ? ? ? ? ? ? pindesc = radix_tree_lookup(&pin_desc_tree, i);
> + ? ? ? ? ? ? ? ? ? ? ? if (pindesc != NULL) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? radix_tree_delete(&pin_desc_tree, i);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? num_pins --;
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? ? ? ? ? spin_unlock(&pin_desc_tree_lock);
> + ? ? ? ? ? ? ? ? ? ? ? kfree(pindesc);
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> + ? ? ? return ret;
> +}
> +EXPORT_SYMBOL_GPL(pinctrl_register_pins_sparse);
Why two different methods for registering pins?
Also the previous two export symbol statements give the wrong functions.
Okay, enough comments for now. I think that covers the big stuff, and
I've got to get some other work done.
g.
Linus Walleij wrote at Monday, June 13, 2011 10:58 AM:
> This creates a subsystem for handling of pinmux devices. These are
> devices that enable and disable groups of pins on primarily PGA and
> BGA type of chip packages and common in embedded systems.
I'm a little confused by this version. In particular:
* I don't see some changes that I thought we'd agreed upon during earlier
review rounds, such as:
** Removing pinmux_ops members list_functions, get_function_name,
get_function_pins; it seems odd not to push this into the pinmux core
as data, but instead have the core pull it out of the driver in a
"polling" function.
** Similarly, I'd asked for at least some documentation about how to
handle the "multi-width bus" problem, but I didn't see that.
* I'm confused by the new data model even more than before:
** How can the board/machine name pins? That should be a function of the
chip (i.e. pinmux driver), since that's where the pins are located. A
chip's pins don't have different names on different boards; the names of
the signals on the PCB connected to those pins are defined by the board,
but not the names of the actual pins themselves.
** struct pinmux_map requires that each function name be globally unique,
since one can only specify "function" not "function on a specific chip".
This can't be a requirement; what if there are two instances of the same
chip on the board (think some expansion chip attached to a memory-mapped
bus rather than the primary SoC itself).
** Perhaps primarily due to the lack of consideration in the documentation,
I'm not convinced we have a clearly defined path to solve the "multi-width
bus" issue. This needs to be a feature of the core pinmux API, rather than
something that's deferred to the board/machine files setting up different
function mappings, since it's not possible to solve purely in function
mappins as they're defined today.
Some minor mainly typo, patch-separation, etc. feedback below.
...
> +Pinmux, also known as padmux, ballmux, alternate functions or mission modes
> +is a way for chip vendors producing some kind of electrical packages to use
> +a certain physical bin (ball, pad, finger, etc) for multiple mutually exclusive
s/bin/pin/
...
> +A simple driver for the above example will work by setting bits 0, 1 or 2
> +into some register mamed MUX, so it enumerates its available settings and
s/mamed/named
> +++ b/drivers/pinctrl/Kconfig
...
> +config PINMUX_U300
> + bool "U300 pinmux driver"
> + depends on ARCH_U300
> + help
> + Say Y here to enable the U300 pinmux driver
> +
> +endif
Shouldn't that portion be part of the second patch?
> +++ b/drivers/pinctrl/Makefile
...
> +obj-$(CONFIG_PINMUX_U300) += pinmux-u300.o
Same here.
> +++ b/include/linux/pinctrl/machine.h
> +/**
> + * struct pinmux_map - boards/machines shall provide this map for devices
> + * @function: a functional name for this mapping so it can be passed down
> + * to the driver to invoke that function and be referenced by this ID
> + * in e.g. pinmux_get()
> + * @dev: the device using this specific mapping, may be NULL if you provide
> + * .dev_name instead (this is more common)
> + * @dev_name: the name of the device using this specific mapping, the name
> + * must be the same that will return your struct device*
s/that will return/as in/ ?
> +++ b/include/linux/pinctrl/pinmux.h
> +struct pinmux;
> +
> +#ifdef CONFIG_PINCTRL
> +
> +struct pinmux_dev;
I think "struct pinmux_map" is needed outside that ifdef, or an include of
<pinctrl/machine.h>.
> +/**
> + * struct pinmux_ops - pinmux operations, to be implemented by drivers
> + * @request: called by the core to see if a certain pin can be muxed in
> + * and made available in a certain mux setting The driver is allowed
> + * to answer "no" by returning a negative error code
That says "and made available in a certain mux setting", but no mux setting
is passed in. s/a certain/the current/?
Documentation for @free is missing here
> +/**
> + * struct pinmux_desc - pinmux descriptor, register this to pinmux subsystem
> + * @name: name for the pinmux
> + * @ops: pinmux operation table
> + * @owner: module providing the pinmux, used for refcounting
> + * @base: the number of the first pin handled by this pinmux, in the global
> + * pin space, subtracted from a given pin to get the offset into the range
> + * of a certain pinmux
> + * @npins: the number of pins handled by this pinmux - note that
> + * this is the number of possible pin settings, if your driver handles
> + * 8 pins that each can be muxed in 3 different ways, you reserve 24
> + * pins in the global pin space and set this to 24
That's not correct, right; if you have 8 pins, you just say 8 here?
The multiplier for N functions per pin is through list_functions,
get_function_name, get_function_pins as I understand it.
> +static inline int pinmux_register_mappings(struct pinmux_map const *map,
> + unsigned num_maps)
> +{
> + return 0;
> +}
I think you moved that to <pinmux/machine.h>?
--
nvpublic
Hi Joe,
thanks for the review, excellent as always.
I fixed all except the below pattern, also searched the source to make sure
there were no other cases of the same errors.
On Mon, Jun 13, 2011 at 8:11 PM, Joe Perches <[email protected]> wrote:
>> +int pinctrl_register_pins_dense(struct pinctrl_pin_desc const *pins,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned num_descs, unsigned num_pins)
>> +{
>> + ? ? int ret;
>> + ? ? unsigned i;
>> +
>> + ? ? ret = pinctrl_register_pins(pins, num_descs, num_pins);
>> + ? ? if (ret) {
>> + ? ? ? ? ? ? for (i = 0; i < num_pins; i++) {
>> + ? ? ? ? ? ? ? ? ? ? struct pin_desc *pindesc;
>> +
>> + ? ? ? ? ? ? ? ? ? ? spin_lock(&pin_desc_tree_lock);
>> + ? ? ? ? ? ? ? ? ? ? pindesc = radix_tree_lookup(&pin_desc_tree, i);
>> + ? ? ? ? ? ? ? ? ? ? if (pindesc != NULL) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? radix_tree_delete(&pin_desc_tree, i);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? num_pins --;
>> + ? ? ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? ? ? ? spin_unlock(&pin_desc_tree_lock);
>> + ? ? ? ? ? ? ? ? ? ? kfree(pindesc);
>> + ? ? ? ? ? ? }
>
> Second use of this pattern. ?Maybe use pinctrl_free_pindescs?
It is quite different actually - in the second case here. we loop over a list
with holes, and we pick each one pin. We cannot loop over the entire
pin range because in this case we don't know the size of the range.
Thanks,
Linus Walleij
On Mon, Jun 13, 2011 at 9:57 PM, Grant Likely <[email protected]> wrote:
> (...)
>?To sum up, it looks like the conceptual model is thus:
>
> - A pinmux driver enumerates and registers all the pins that it has
> - Setup code and/or driver code requests blocks of pins (functions)
> when it needs them
> - If all the pins are available, it gets them, otherwise it the allocation fails
> - pins cannot be claimed by more than one device
> - it is up to the pinmux driver to actually program the device and
> determine whether or not the requested pin mode is actually
> configurable. ?Even if pins are available, it may be that other
> constraints prevent it from actually being programmed
>
> Ultimately, it still is up to the board designer and board port
> engineer to ensure that the system can actually provide the requested
> pin configuration.
>
> My understanding is that in the majority of cases pinmux will probably
> want/need to be setup and machine_init time, and device drivers won't
> really know or care about pinmux; it will already be set up for them
> when the driver is probed. ?Any power management issues will also be
> handled by platform/soc code when the dependent devices are in PM
> states.
>
> How does that line up with your conceptual model of pinmux?
100% I'd say, so far.
Devil is in the details, below.
>> - Converted the pin lookup from a static array into a radix tree,
>> ?I agreed with Grant Likely to try to avoid any static allocation
>> ?(which is crap for device tree stuff) so I just rewrote this
>> ?to be dynamic, just like irq number descriptors. The
>> ?platform-wide definition of number of pins goes away - this is
>> ?now just the sum total of the pins registered to the subsystem.
>
> You should consider still using a bitmap for tracking which pins are
> actually available, similar to how irqs are tracked.
Well that is not so simple, because that bitmap needs to have
a size. And that means the stuff we're tracking is not really
dynamic, but either static or a hybrid (roofed bitmap). Let's
recap:
- Either you have a defined number of IRQs (NR_IRQS) defined
system-wide and then allocate a fixed array of descriptors like
that: struct irq_desc irq_desc[NR_IRQS] (or for GPIOs:
static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];)
- Or you dynamically allocate descriptors such as is done with
sparse IRQs.
The first case is the same way that gpiolib depends on the global
ARCH_NR_GPIOS is used. It inevitably involves roofing the
number of IRQs/GPIOs on systems where they come and go.
That is static allocation, and that is what we wanted to get away
from.
Sparse IRQs also use a bitmap though. But it is not dynamic
at all, it's either static or an assumption-based hybrid.
To use a bitmap you need to know how many IRQs you have, so
you know how large bitmap you need to allocate.
c.f. kernel/irq/irqdesc.c:
static DECLARE_BITMAP(allocated_irqs, IRQ_BITMAP_BITS);
And IRQ_BITMAP_BITS comes from:
kernel/irq/internals.h;
#ifdef CONFIG_SPARSE_IRQ
# define IRQ_BITMAP_BITS (NR_IRQS + 8196)
#else
# define IRQ_BITMAP_BITS NR_IRQS
#endif
(Needless to say this assumes you never add more than
8196 irqs on top of the fixed number of IRQs)
So to use this I need to go back to a model where the system
knows how many pins there are anyway or just assume
something like "never more than 8192" and hardcode it
like for sparse IRQs above, then that consumes
8192/32 = 256 words of memory.
>?A bool in each
> pinmux structure is a little wasteful,
If I instead have it all-dynamic, and say the boolean field
even consumes 32bits in the desc (which we need anyway)
you need to use more than 256 pins in your system before
this hits you, 32 bits per pin. (Beware I have no clue how
booleans actually are stored in structs.)
It's not *that* bad, especially not compared to starting
to compile in every other driver to get a single booting
binary for several ARM systems, then this is peanuts ...
(OK maybe a crap argument I dunno. It feels to me like
footprint issues are out of fashion on recent ARM systems.)
> and requires a lock to be held
> for a long time while checking all the structures.
I don't get this part. The lock is held when looking up
the desc in the radix tree, just like in the IRQ subsystem
and the radix lookup is fast enough to be in the fastpath.
The only code that actually traverse the entire tree is in
debugfs code or error path, surely that must be an acceptable
traversal?
>> +The driver will for all calls be provided an offset pin number into its own
>> +pin range. If you have 2 chips with 8x8 pins, the first chips pins will have
>> +numbers 0 thru 63 and the second one pins 64 thru 127, but the driver for the
>> +second chip will be passed numbers in the range 0 thru 63 anyway, base offset
>> +subtracted.
>
> Wait, do you really want a global numberspace here? ?I'd rather see
> callers have a direct reference to the pinmux controller instance, and
> use local pin numbers.
The pinmux functions that are requested by machine, bus or device
indeed work that way.
>?Given the choice, I would not go with global
> numbers for GPIOs again, and I'm not so big a fan of them for irqs
> either.
Two reasons for a global numberspace:
1. We want to handle other stuff that relates to pincontrol in the
pinctrl API, such as biasing, driver levels, load capacitance and
whatever the funny pin engineers come up with. Not all pins
are simultaneously muxable, many are both controlled in
this way AND muxable, at the same time. Biasing etc
needs to happen at pin level rather than group/function level.
muxing and biasing may interact in driver level, BTW.
2. The second reason it is even there is to coexist with the GPIO
subsystem, since many, many systems need to mux in GPIO
on single pins today (ux500, OMAP, i.MX31 OTOMH).
Some assorted blather follows:
That is why requesting a singe pin for GPIO with
int pinmux_request_gpio(int pin, unsigned gpio);
that reserved the pin and goes all the way to an (optional)
callback to the driver in
int (*gpio_request_enable) (struct pinmux_dev *pmxdev, unsigned offset);
that asks the hardware to mux in that singe pin as GPIO.
The mentioned hardware indeed has per-pin muxing granularity
BTW, so this will be useful and quick for these usecases.
The alternative is to first define a function for every singular GPIO pin,
and then mux in that function for each pin in the GPIO driver.
This does not solve the biasing etc problems though.
If we were discussing footprint issues before defining a function for
each pin in the system dwarfs that :-)
If instead all GPIOs are requested from the GPIO driver in
ranges, say "these 32 as GPIO please" the problem goes away,
and you don't need to implement your stuff using these function at
all. However I feel it is a bit thick to require that, and in practical
cases I've seen you actually do want to get at the single pins.
(IIRC Sascha confirmed this assumption for i.MX)
>> +const char *foo_get_fname(struct pinmux_dev *pmxdev, unsigned selector)
>> +{
>> + if (selector >= ARRAY_SIZE(myfuncs))
>> + return NULL;
>> + return myfuncs[selector].name;
>> +}
> Is there a method to lookup the function id from the name? ?Going from
> name to number seems more useful to me than going the other way
> around.
I don't quite get it, this is based on how the regulator framework
asks it's drivers to enumerate voltages, the name is entirely optional,
more to get visibility of names. Enumerators is what the framework
is using and requiring from the drivers.
>> +struct pinmux_ops ops = {
>> + ? ? ? .list_functions = foo_list,
>> + ? ? ? .get_function_name = foo_get_fname,
>> + ? ? ? .get_function_pins = foo_get_pins,
>> + ? ? ? .enable = foo_enable,
>> + ? ? ? .disable = foo_disable,
>
> Mixing callbacks with data here. ?Not bad, but maybe a little odd.
? The above are all functions ?
>> +The beauty of the pinmux subsystem is that since it keeps track of all
>> +pins and who is using them, it will already have denied an impossible
>> +request like that, so the driver does not need to worry about such
>> +things - when it gets a selector passed in, the pinmux subsystem makes
>> +sure no other device or GPIO assignment is already using the selected
>> +pins.
>
> Sometimes that isn't enough. ?Some functions may not actually collide
> on the pins they select, but the modes will be mutually exclusive
> anyway. ?There needs to be runtime checking that the mode can actually
> be programmed when it is enabled (of course, it may just be that for
> *this* example it doesn't need to worry about it, in which case my
> comment is moot).
Yeah the driver will have to take care of that and return some
-EBUSY or so.
>> +#include <linux/pinctrl/machine.h>
>> +
>> +static struct pinmux_map pmx_mapping[] = {
>> + ? ? ? {
>> + ? ? ? ? ? ? ? .function = "spi0-1",
>> + ? ? ? ? ? ? ? .dev_name = "foo-spi.0",
>> + ? ? ? },
>> + ? ? ? {
>> + ? ? ? ? ? ? ? .function = "i2c0",
>> + ? ? ? ? ? ? ? .dev_name = "foo-i2c.0",
>> + ? ? ? },
>> +};
>
> I'm wary about this approach, even though I know it is already used by
> regulator and clk mappings. ?The reason I'm wary is that matching
> devices by name becomes tricky for anything that isn't statically
> created by the kernel, such as when enumerating from the device tree,
> because it assumes that the device name is definitively known.
> Specifically, Linux's preferred name for a device is not guaranteed to
> be available from the device tree. ?We very purposefully do not encode
> Linux kernel implementation details into the kernel so that
> implementation detail changes don't force dt changes.
>
> /me goes and thinks about the problem some more...
>
> Okay, I think I've got a new approach for the DT domain so that Linux
> gets the device names it wants for matching to clocks, regulators and
> this stuff. ?I'm going to go and code it up now. ?I still don't
> personally like matching devices by name, but by no measure is it a
> show stopper for me.
We can enforce it for DT-based pinmuxing and live with this
name matcing for existing boardfiles I assume?
>> +This get/enable/disable/put sequence can just as well be handled by bus drivers
>> +if you don't want each and every driver to handle it and you know the
>> +arrangement on your bus.
>
> I would *strongly* recommend against individual device drivers
> accessing the pinmux api. ?This is system level configuration code,
> and should be handled at the system level.
As explained by Russell in earlier mails (and in my own experience
too) there exist systems that need to alter muxing at runtime, say
mux this thing out, mux that thing in. So it was a design
requirement.
Some devices will need to demux in response to runtime_pm
hooks though. And the other functions of pincontrol, like
biasing, definately need to happen in rutime on request from
a single device.
But I can try to write the doc so that it emphasize doing this in
machine/system/board code or wherever you first have an
opportunity to ask for something with your struct device *
as an argument, OK?
It could also be in the system-specific callbacks to board
code, of course.
Another argument for system files to handled this is that
you often mux things out/in on suspend/resume, and that
may be suitable for an entity that knows about all devices.
Most systems I've seen don't have such centralized control
over device creation, and it seems this leads to the implicit
requirement that anyone wanting to so that from the board
need to move away from any statically allocated devices
first?
Or am I getting it backwards here?
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -56,6 +56,10 @@ source "drivers/pps/Kconfig"
>>
>> ?source "drivers/ptp/Kconfig"
>>
>> +# pinctrl before gpio - gpio drivers may need it
>
> GPIO controllers are just other devices, I don't think there is
> anything special here when compared with SPI or I2C. ?I don't think
> gpio drivers should be accessing the pinmux api directly.
>
> In my mind, the gpio layer is only about abstracting the gpio control
> interface to drivers. Whether or not or how the pin is routed outside
> the chip package is irrelevant to the driver or the gpio api.
>
> Besides, this is kconfig. The order of this file has zero bearing on
> the resultant kernel. It does matter for the Makefile though.
See earlier discussion, I think. If you look in the current
drivers/gpio/gpio-nomadik.c you see that all muxing is actually
done in that GPIO driver, since it is using the same register
range. (So it was natural not to break that part out, as there
was no other natural place to put it anyway.)
The only reason why others don't do that is (I guess) that
they have a special register range for muxing. In
arch/arm/mach-pxa/mfp-pxa.c you find the same kind
of strong coupling between GPIO and pinmux.
Actually some of my generalization work for GPIO is
exactly related to this, which is in practice a custom call to
the GPIO driver. Main problem - since the GPIO driver
can make any pin a GPIO, it needs to interact with
GPIO code to make sure it is not used for some GPIO -
since we don't have a pinmux subsystem that can take care
of that. So if I had that ... a bit chicken and egg problem
here. (There are other custom GPIO stuff for sure though,
not just this.)
And doing a generic pin control subsystem is also for this
reason - get biasing, drive modes and load capacitances out
of the gpio subsystem and into something apropriate.
I think doing this from GPIO drivers for individual pins need
to be some intermediate step to consolidate the functionality
to drivers/pinctrl/*, else too many requirements on how things
shall be done start spreading across the board, but I'll surely
try to make it as autonomous as possible.
>> +menuconfig PINCTRL
>> + ? ? ? bool "PINCTRL Support"
>> + ? ? ? depends on SYSFS && EXPERIMENTAL
>
> Hold off on the sysfs stuff. ?Lay down the basic API without sysfs,
> and add the sysfs bits as a separate patch. ?This becomes a
> kernel->userspace abi issue, and you don't want to mess that up.
It is just exposing a name right now, say "you have this pinmux
with this name"
> Is a pinmux sysfs abi really needed? ?What is the use-case?
Same as for userspace GPIO control I guess.
If you want to do GPIO control on a certain pin from userspace,
then surely you want to be able to mux it in, and I just heard
that the GPIO driver should not be able to do that itself... heh.
And configuring it statically muxed-in does not make sense
since you don't know whether userspace will actually use it
and default mux-enabling it will consumer more power in some
cases so doing it statically is not OK etc etc.
>> +config PINMUX_U300
>> + ? ? ? bool "U300 pinmux driver"
>> + ? ? ? depends on ARCH_U300
>> + ? ? ? help
>> + ? ? ? ? Say Y here to enable the U300 pinmux driver
>> +
>
> PINMUX_U300 should not be here in the infrastructure patch.
Mea culpa. I fix.
>> +obj-$(CONFIG_PINMUX_U300) += pinmux-u300.o
This too!
>> +# generic pinmux support
>> +
>> +ccflags-$(CONFIG_DEBUG_PINMUX) += -DDEBUG
>> +
>> +obj-$(CONFIG_PINCTRL) ? ? ? ? ?+= core.o
>
> Consider calling this pinmux.o; particularly if there is ever a chance
> of it becoming a module. ?It is conceivable that pinmux will be used
> for peripheral chips in a way that can/should be loaded at runtime.
Right now I haven't broken the pin registration API that will be
reused by generic pincontrol and actual pinmux framework into
different .c files, maybe I should?
The idea is that the pin infrastructure is static and pinmux
could actually be a module.
>> +/* Global lookup of per-pin descriptors, one for each physical pin */
>> +static DEFINE_SPINLOCK(pin_desc_tree_lock);
>> +static RADIX_TREE(pin_desc_tree, GFP_KERNEL);
>
> The radix tree should probably be per-pinmux controller local. ?Of
> course, if you make all the pinmux numbering local to the controller,
> then the need for a radix tree could very well go away entirely, and
> it would simplify everything.
See earlier discussion about a global numberspace I guess.
If we were only doing pinmuxing with no global numberspace to
avoid also correlating needs with biasing etc, this would be true,
but pinmux is too glued into other stuff IMHO.
>> +int pin_is_valid(int pin)
>> +{
>> + ? ? ? return pin >= 0 && pin < num_pins;
>> +}
>> +EXPORT_SYMBOL_GPL(pin_is_valid);
>
> A "pin_" prefix is very generic sounding. ?Though it doesn't read as
> well, the pinmux_ prefix should probably be used consistently.
This is changed. pin_* prefix is for the generic top-level
pinctrl API, pinmux_* is specifically for the pinmux stuff.
>> pinctrl_register_pins_sparse/dense
>
> Why two different methods for registering pins?
Explained in the kerneldoc I guess, basically some systems can control
all pins from 0 ... NR_PINS_ON_THIS_SYSTEM whereas others
actually have holes in this map, or may want to fill them in with
several pinctrl_register_pins_sparse() calls, such as for platforms
registering some pins that are common across a few boards and
then a few board-specific pins.
> Also the previous two export symbol statements give the wrong functions.
Fixed that, saw it on the previous review mail from Joe P.
> Okay, enough comments for now. ?I think that covers the big stuff, and
> I've got to get some other work done.
Thanks!
I bet that other work is deciding on whether to expose gpio_to_chip()
or not, hehe :-)
Yours,
Linus Walleij
On Tue, Jun 14, 2011 at 1:28 AM, Stephen Warren <[email protected]> wrote:
> I'm a little confused by this version.
Sorry, I'll try to clarify.
> In particular:
>
> * I don't see some changes that I thought we'd agreed upon during earlier
> review rounds, such as:
>
> ** Removing pinmux_ops members list_functions, get_function_name,
> get_function_pins; it seems odd not to push this into the pinmux core
> as data, but instead have the core pull it out of the driver in a
> "polling" function.
I don't find any particular discussion on that, did I miss something?
It might be that I simply do not agree...
Anyway, this is modeled exactly on how the regulator subsystem
interact with its drivers to enumerate voltages. It seems intuitive
to me, it's an established kernel design pattern that drivers know
the hardware particulars, and so I don't get the problem here.
Would you argue that the regulator subsystem has bad design
too or is this case different?
Can't you just send some patch or example .h file for the API
you would like to see so I understand how you think about
this?
I might be thinking inside the box of my current design here so
help me get out of it, I just have a hard time seeing how to
do it.
> ** Similarly, I'd asked for at least some documentation about how to
> handle the "multi-width bus" problem, but I didn't see that.
SORRY! Missed it.
So sure that can be added, so something like this?
A B C D E F G H
+---+
8 | o | o o o o o o o
| |
7 | o | o o o o o o o
| |
6 | o | o o o o o o o
+---+---+
5 | o | o | o o o o o o
+---+---+ +---+
4 o o o o o o | o | o
| |
3 o o o o o o | o | o
| |
2 o o o o o o | o | o
+-------+-------+-------+---+---+
1 | o o | o o | o o | o | o |
+-------+-------+-------+---+---+
(...)
On the botton row at { A1, B1, C1, D1, E1, F1, G1, H1 } we have something
special - it's an external MMC bus that can be 2, 4 or 8 bits wide, and it will
consume 2, 4 or 8 pins respectively, so either { A1, B1 } are taken or
{ A1, B1, C1, D1 } or all of them. If we use all 8 bits, we cannot use the SPI
port on pins { G4, G3, G2, G1 } of course.
(...)
A simple driver for the above example will work by setting bits 0, 1, 2, 3,
4 or 5 into some register named MUX, so it enumerates its available settings
and their pin assignments, and expose them like this:
#include <linux/pinctrl/pinmux.h>
struct foo_pmx_func {
char *name;
const unsigned int *pins;
const unsigned num_pins;
};
static unsigned int spi0_0_pins[] = { 0, 8, 16, 24 };
static unsigned int i2c0_pins[] = { 24, 25 };
static unsigned int spi0_1_pins[] = { 38, 46, 54, 62 };
static unsigned int mmc0_1_pins[] = { 56, 57 };
static unsigned int mmc0_2_pins[] = { 56, 57, 58, 59 };
static unsigned int mmc0_3_pins[] = { 56, 57, 58, 59, 60, 61, 62, 63 };
static struct foo_pmx_func myfuncs[] = {
{
.name = "spi0-0",
.pins = spi0_0_pins,
.num_pins = ARRAY_SIZE(spi0_1_pins),
},
{
.name = "i2c0",
.pins = i2c0_pins,
.num_pins = ARRAY_SIZE(i2c0_pins),
},
{
.name = "spi0-1",
.pins = spi0_1_pins,
.num_pins = ARRAY_SIZE(spi0_1_pins),
},
{
.name = "mmc0-2bit",
.pins = mmc0_1_pins,
.num_pins = ARRAY_SIZE(mmc0_1_pins),
},
{
.name = "mmc0-4bit",
.pins = mmc0_2_pins,
.num_pins = ARRAY_SIZE(mmc0_2_pins),
},
{
.name = "mmc0-8bit",
.pins = mmc0_3_pins,
.num_pins = ARRAY_SIZE(mmc0_3_pins),
},
};
Looks OK?
> ** How can the board/machine name pins? That should be a function of the
> chip (i.e. pinmux driver), since that's where the pins are located. A
> chip's pins don't have different names on different boards; the names of
> the signals on the PCB connected to those pins are defined by the board,
> but not the names of the actual pins themselves.
Actually I don't really like the use of the concept of board and machine
for chip packages either. But if I say that without devicetrees it
could be something like arch/arm/plat-nomadik/package-db8500.c
defining the pins. Then it is still coming from the outside, for
a particular platform, for a particular chip package.
Then the init function in that file gets called on relevant systems.
As you can see in the example implementation for U300 I actually
do this in the driver itself by including the machine.h file to that one.
So this driver first registers pins, then functions.
I think there is broad consensus that this should come in from
the device tree in the future. And if it comes from the device tree, it's
still the say arch/arm/plat-nomadik/package-db8500.c file that looks
up the pins from the device tree and registers them, just it gets the
data from the outside.
Possibly the core could be made to do this. I know too little about
device tree I think :-/
> ** struct pinmux_map requires that each function name be globally unique,
> since one can only specify "function" not "function on a specific chip".
> This can't be a requirement; what if there are two instances of the same
> chip on the board (think some expansion chip attached to a memory-mapped
> bus rather than the primary SoC itself).
Namespace clashes are a problem but the way I see it basically a
problem with how you design the namespace. It's just a string and
the document is just an example.
If namespacing is a big issue, we may need naming schemes, but
with only one driver as I have now I think that is overkill. Platforms
already need to take care of their namespaceing, so if I create
a kernel that have mmc ports on two blocks I would just prefix them,
say:
{
.name = "chip0-mmc0",
.pins = chip0_mmc0_pins,
.num_pins = ARRAY_SIZE(chip0_mmc0_pins),
},
{
.name = "chip1-mmc0",
.pins = chip1_mmc0_pins,
.num_pins = ARRAY_SIZE(chip1_mmc0_pins),
},
etc for the functions (the device names are namespaced by the
device driver core I believe).
> ** Perhaps primarily due to the lack of consideration in the documentation,
> I'm not convinced we have a clearly defined path to solve the "multi-width
> bus" issue. This needs to be a feature of the core pinmux API, rather than
> something that's deferred to the board/machine files setting up different
> function mappings, since it's not possible to solve purely in function
> mappins as they're defined today.
Can this be considered fixed with the above example?
(Which will be in v4)
> Some minor mainly typo, patch-separation, etc. feedback below.
Fixed most of them...
>> +++ b/include/linux/pinctrl/pinmux.h
>> +struct pinmux;
>> +
>> +#ifdef CONFIG_PINCTRL
>> +
>> +struct pinmux_dev;
>
> I think "struct pinmux_map" is needed outside that ifdef, or an include of
> <pinctrl/machine.h>.
Yeah and it should only be used in machine.h as you point out
later, plus I forgot to stub 2 functions for sparse/dense pin registration.
Thanks!
>> +/**
>> + * struct pinmux_ops - pinmux operations, to be implemented by drivers
>> + * @request: called by the core to see if a certain pin can be muxed in
>> + * ? and made available in a certain mux setting The driver is allowed
>> + * ? to answer "no" by returning a negative error code
>
> That says "and made available in a certain mux setting", but no mux setting
> is passed in. s/a certain/the current/?
Sorry the missing words are "for later being selected for a certain mux
setting". So the idea is that this request control over a specific pin
and then the enable() call can select it. So a first opportunity for
the driver to say "no" really.
I'm editing the text...
> Documentation for @free is missing here
Fixed it!
>> +/**
>> + * struct pinmux_desc - pinmux descriptor, register this to pinmux subsystem
>> + * @name: name for the pinmux
>> + * @ops: pinmux operation table
>> + * @owner: module providing the pinmux, used for refcounting
>> + * @base: the number of the first pin handled by this pinmux, in the global
>> + * ? pin space, subtracted from a given pin to get the offset into the range
>> + * ? of a certain pinmux
>> + * @npins: the number of pins handled by this pinmux - note that
>> + * ? this is the number of possible pin settings, if your driver handles
>> + * ? 8 pins that each can be muxed in 3 different ways, you reserve 24
>> + * ? pins in the global pin space and set this to 24
>
> That's not correct, right; if you have 8 pins, you just say 8 here?
You are right this is a leftover a previous designed where I used
a virtual pin range covering all possible mux settings. This design
was insane, so thanks for helping me smoking out the last
remnants.
> The multiplier for N functions per pin is through list_functions,
> get_function_name, get_function_pins as I understand it.
Right.
>> +static inline int pinmux_register_mappings(struct pinmux_map const *map,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned num_maps)
>> +{
>> + ? ? return 0;
>> +}
>
> I think you moved that to <pinmux/machine.h>?
Yep, fixed and added two missing stubs in machine.h too.
Thanks a lot!
Yours,
Linus Walleij
Linus Walleij wrote at Tuesday, June 14, 2011 8:26 AM:
> On Tue, Jun 14, 2011 at 1:28 AM, Stephen Warren <[email protected]> wrote:
>
> > I'm a little confused by this version.
>
> Sorry, I'll try to clarify.
>
> > In particular:
> >
> > * I don't see some changes that I thought we'd agreed upon during earlier
> > review rounds, such as:
> >
> > ** Removing pinmux_ops members list_functions, get_function_name,
> > get_function_pins; it seems odd not to push this into the pinmux core
> > as data, but instead have the core pull it out of the driver in a
> > "polling" function.
>
> I don't find any particular discussion on that, did I miss something?
> It might be that I simply do not agree...
That was:
https://lkml.org/lkml/2011/5/19/365
https://lkml.org/lkml/2011/5/19/379
> Anyway, this is modeled exactly on how the regulator subsystem
> interact with its drivers to enumerate voltages. It seems intuitive
> to me, it's an established kernel design pattern that drivers know
> the hardware particulars, and so I don't get the problem here.
> Would you argue that the regulator subsystem has bad design
> too or is this case different?
Well, I guess it does do something like this on a much smaller scale.
I'm surprised this style exists; I'd expect the following:
driver:
pinmux_register_driver(
ops
list of pins
list of functions);
to be much simpler than:
driver:
pinmux_register_driver(ops)
core:
for loop:
ops->list_functions
ops->get_function_name
ops->get_function_pins
Still, this is something that doesn't affect the publically visible
client interface, and is easy to change in the future without a lot
of work, so probably not a huge deal.
> Can't you just send some patch or example .h file for the API
> you would like to see so I understand how you think about
> this?
Are your patches in git somewhere? It's much easier for me to pull
at present than grab patches out of email; something I certainly need
to fix...
> I might be thinking inside the box of my current design here so
> help me get out of it, I just have a hard time seeing how to
> do it.
>
> > ** Similarly, I'd asked for at least some documentation about how to
> > handle the "multi-width bus" problem, but I didn't see that.
>
> SORRY! Missed it.
>
> So sure that can be added, so something like this?
>
> A B C D E F G H
> +---+
> 8 | o | o o o o o o o
> | |
> 7 | o | o o o o o o o
> | |
> 6 | o | o o o o o o o
> +---+---+
> 5 | o | o | o o o o o o
> +---+---+ +---+
> 4 o o o o o o | o | o
> | |
> 3 o o o o o o | o | o
> | |
> 2 o o o o o o | o | o
> +-------+-------+-------+---+---+
> 1 | o o | o o | o o | o | o |
> +-------+-------+-------+---+---+
>
> (...)
>
> On the botton row at { A1, B1, C1, D1, E1, F1, G1, H1 } we have something
> special - it's an external MMC bus that can be 2, 4 or 8 bits wide, and it will
> consume 2, 4 or 8 pins respectively, so either { A1, B1 } are taken or
> { A1, B1, C1, D1 } or all of them. If we use all 8 bits, we cannot use the SPI
> port on pins { G4, G3, G2, G1 } of course.
>
> (...)
>
> A simple driver for the above example will work by setting bits 0, 1, 2, 3,
> 4 or 5 into some register named MUX, so it enumerates its available
> settings
> and their pin assignments, and expose them like this:
>
> #include <linux/pinctrl/pinmux.h>
>
> struct foo_pmx_func {
> char *name;
> const unsigned int *pins;
> const unsigned num_pins;
> };
>
> static unsigned int spi0_0_pins[] = { 0, 8, 16, 24 };
> static unsigned int i2c0_pins[] = { 24, 25 };
> static unsigned int spi0_1_pins[] = { 38, 46, 54, 62 };
> static unsigned int mmc0_1_pins[] = { 56, 57 };
> static unsigned int mmc0_2_pins[] = { 56, 57, 58, 59 };
> static unsigned int mmc0_3_pins[] = { 56, 57, 58, 59, 60, 61, 62, 63 };
>
> static struct foo_pmx_func myfuncs[] = {
> {
> .name = "spi0-0",
> .pins = spi0_0_pins,
> .num_pins = ARRAY_SIZE(spi0_1_pins),
> },
> {
> .name = "i2c0",
> .pins = i2c0_pins,
> .num_pins = ARRAY_SIZE(i2c0_pins),
> },
> {
> .name = "spi0-1",
> .pins = spi0_1_pins,
> .num_pins = ARRAY_SIZE(spi0_1_pins),
> },
> {
> .name = "mmc0-2bit",
> .pins = mmc0_1_pins,
> .num_pins = ARRAY_SIZE(mmc0_1_pins),
> },
> {
> .name = "mmc0-4bit",
> .pins = mmc0_2_pins,
> .num_pins = ARRAY_SIZE(mmc0_2_pins),
> },
> {
> .name = "mmc0-8bit",
> .pins = mmc0_3_pins,
> .num_pins = ARRAY_SIZE(mmc0_3_pins),
> },
> };
>
> Looks OK?
I think that's exactly the setup I was looking to avoid. See the portion
of the thread starting from:
https://lkml.org/lkml/2011/5/13/424
In particular, in:
https://lkml.org/lkml/2011/5/18/33
I explained how I understood you intended to solve this (summarized just
below) and you appeared to agree with that.
The functions a pinmux driver exposes are purely driven by the smallest
set of pins the HW can control at once (pin-by-pin for many HW I imagine,
but groups of 1-10 pins on Tegra).
To provide the "bus" abstraction, struct pinmux_map will be expanded to
include N pinmux driver functions to activate per map entry.
(I agreed that the actual implementation could be deferred to a later
set of patches, but wanted the documentation to describe the final model
so that we didn't end up people implementing the example you wrote above).
So, in the example above:
static unsigned int mmc0_1_pins[] = { 56, 57 };
static unsigned int mmc0_2_pins[] = { 58, 59 };
static unsigned int mmc0_3_pins[] = { 60, 61, 62, 63 };
{
.name = "mmc0-1:0",
.pins = mmc0_1_pins,
.num_pins = ARRAY_SIZE(mmc0_1_pins),
},
{
.name = "mmc0-3:2",
.pins = mmc0_2_pins,
.num_pins = ARRAY_SIZE(mmc0_2_pins),
},
{
.name = "mmc0-7:4",
.pins = mmc0_3_pins,
.num_pins = ARRAY_SIZE(mmc0_3_pins),
},
So a board that happens to use a 2-bit bus would define:
static struct pinmux_map pmx_mapping[] = {
{
.functions = {"mmc0-1:0"},
.dev_name = "tegra-sdhci.0",
},
};
But a board that happens to use an 8-bit bus would define:
static struct pinmux_map pmx_mapping[] = {
{
.functions = {"mmc0-1:0", "mmc0-3:2", "mmc0-7:4"},
.dev_name = "tegra-sdhci.0",
},
};
... although struct pinmux_map would probably need to grow a separate
name field to match against pinmux_get's 2nd parameter, rather than
assuming the driver's function names and the mapping's function names
were the same.
Such a new name field would also introduce a second namespace that'd
satisfy my next concern.
> > ** How can the board/machine name pins? That should be a function of the
> > chip (i.e. pinmux driver), since that's where the pins are located. A
> > chip's pins don't have different names on different boards; the names of
> > the signals on the PCB connected to those pins are defined by the board,
> > but not the names of the actual pins themselves.
>
> Actually I don't really like the use of the concept of board and machine
> for chip packages either. But if I say that without devicetrees it
> could be something like arch/arm/plat-nomadik/package-db8500.c
> defining the pins. Then it is still coming from the outside, for
> a particular platform, for a particular chip package.
>
> Then the init function in that file gets called on relevant systems.
>
> As you can see in the example implementation for U300 I actually
> do this in the driver itself by including the machine.h file to that one.
> So this driver first registers pins, then functions.
>
> I think there is broad consensus that this should come in from
> the device tree in the future. And if it comes from the device tree, it's
> still the say arch/arm/plat-nomadik/package-db8500.c file that looks
> up the pins from the device tree and registers them, just it gets the
> data from the outside.
I think discussion of DeviceTree here is a bit of a distraction; if the
pinmux core APIs imply the correct data model, it doesn't seem especially
relevant.
For the pinmux drivers themselves, I expect the SoC's pinmux driver to
define the set of pins available, and the set of functions available.
A couple examples:
There will be a single static set of pins/functions for any Tegra 2
device. Tegra 3 will have a different set of pins/functions since the
HW changed quite a bit in this area. As such, DeviceTree doesn't really
help much, since there will always be a custom driver per SoC, and the
data a given driver uses will not vary between boards, so DeviceTree
helps little.
On the other hand, if you've got a very generic pinmux controller, and
it gets included in N (versions of) SoCs with the same register layout,
but the actual pin names related to each register bit
are different, then I can see it making sense for the pinmux driver
itself to parameterize itself with data from DeviceTree. Either way
though, this is purely an implementation detail within the pinmux driver,
and not something the core would even be aware of.
However, I do think the board-specific list of struct pinmux_map will
come from DeviceTree. This will vary from board to board with potentially
little commonality. DeviceTree seems a great solution to keep all that
varying data out of the kernel. The core pinmux code could well include
the common code to parse the DeviceTree for this data.
> Possibly the core could be made to do this. I know too little about
> device tree I think :-/
>
> > ** struct pinmux_map requires that each function name be globally unique,
> > since one can only specify "function" not "function on a specific chip".
> > This can't be a requirement; what if there are two instances of the same
> > chip on the board (think some expansion chip attached to a memory-mapped
> > bus rather than the primary SoC itself).
>
> Namespace clashes are a problem but the way I see it basically a
> problem with how you design the namespace. It's just a string and
> the document is just an example.
The issue here is that the pinmux driver defines a single namespace for
functions, and struct pinmux_map's definition passes that single namespace
through, since the value of .function comes from that same namespace, and
is also the search key for the list of driver-visible functions.
As I alluded to above, if struct pinmux_map had a separate field for the
search key (for pinmux_get's use) v.s. the set of driver functions that
key maps to, then there can be separate namespaces, and a place to resolve
conflicts:
struct pinmux_map_entry {
struct pinmux_dev *pmxdev
const char *function; // pmxdev's specific function name-space
};
struct pinmux_map {
const char *name; // global driver-visible function name-space
struct device *dev;
const char *dev_name;
int n_ functions;
const struct pinmux_map_entry * functions;
};
Thinking about this at a high level, this seems equivalent to the GPIO
driver: There are two name-spaces there:
1) One used by clients: A global name-space, equivalent to
pinmux_map.name.
2) One used by GPIO drivers: A per-driver name-space, equivalent to
pinmux_map_entry.function.
... and the GPIO core maps from (1) to (2) above internally.
In the pinmux case, since everything is named by a string rather than a
linear number range, the mapping process needs the pinmux_map_entry.function
field, rather than just substracting a GPIO driver's base GPIO number.
--
nvpublic
On Wed, Jun 15, 2011 at 12:11 AM, Stephen Warren <[email protected]> wrote:
> [Me]
>> Can't you just send some patch or example .h file for the API
>> you would like to see so I understand how you think about
>> this?
>
> Are your patches in git somewhere? It's much easier for me to pull
> at present than grab patches out of email; something I certainly need
> to fix...
Yep:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git
pinmux-subsystem
It's based on v3.0-rc3 as of now. I tend to rebase it though.
>> static unsigned int mmc0_1_pins[] = { 56, 57 };
>> static unsigned int mmc0_2_pins[] = { 56, 57, 58, 59 };
>> static unsigned int mmc0_3_pins[] = { 56, 57, 58, 59, 60, 61, 62, 63 };
>>
>> static struct foo_pmx_func myfuncs[] = {
>> ? ? ? {
>> ? ? ? ? ? ? ? .name = "mmc0-2bit",
>> ? ? ? ? ? ? ? .pins = mmc0_1_pins,
>> ? ? ? ? ? ? ? .num_pins = ARRAY_SIZE(mmc0_1_pins),
>> ? ? ? },
>> ? ? ? {
>> ? ? ? ? ? ? ? .name = "mmc0-4bit",
>> ? ? ? ? ? ? ? .pins = mmc0_2_pins,
>> ? ? ? ? ? ? ? .num_pins = ARRAY_SIZE(mmc0_2_pins),
>> ? ? ? },
>> ? ? ? {
>> ? ? ? ? ? ? ? .name = "mmc0-8bit",
>> ? ? ? ? ? ? ? .pins = mmc0_3_pins,
>> ? ? ? ? ? ? ? .num_pins = ARRAY_SIZE(mmc0_3_pins),
>> ? ? ? },
>> };
>>
>> Looks OK?
>
> I think that's exactly the setup I was looking to avoid. See the portion
> of the thread starting from:
Gah. Baudelaire once said something like the reason people think
they agree is that they misunderstand each other all the time.
Sorry for my lameness :-/
> static unsigned int mmc0_1_pins[] = { 56, 57 };
> static unsigned int mmc0_2_pins[] = { 58, 59 };
> static unsigned int mmc0_3_pins[] = { 60, 61, 62, 63 };
>
> ? ? ? ?{
> ? ? ? ? ? ? ? ?.name = "mmc0-1:0",
> ? ? ? ? ? ? ? ?.pins = mmc0_1_pins,
> ? ? ? ? ? ? ? ?.num_pins = ARRAY_SIZE(mmc0_1_pins),
> ? ? ? ?},
> ? ? ? ?{
> ? ? ? ? ? ? ? ?.name = "mmc0-3:2",
> ? ? ? ? ? ? ? ?.pins = mmc0_2_pins,
> ? ? ? ? ? ? ? ?.num_pins = ARRAY_SIZE(mmc0_2_pins),
> ? ? ? ?},
> ? ? ? ?{
> ? ? ? ? ? ? ? ?.name = "mmc0-7:4",
> ? ? ? ? ? ? ? ?.pins = mmc0_3_pins,
> ? ? ? ? ? ? ? ?.num_pins = ARRAY_SIZE(mmc0_3_pins),
> ? ? ? ?},
>
> So a board that happens to use a 2-bit bus would define:
>
> static struct pinmux_map pmx_mapping[] = {
> ? ? ? ?{
> ? ? ? ? ? ? ? ?.functions = {"mmc0-1:0"},
> ? ? ? ? ? ? ? ?.dev_name = "tegra-sdhci.0",
> ? ? ? ?},
> };
>
> But a board that happens to use an 8-bit bus would define:
>
> static struct pinmux_map pmx_mapping[] = {
> ? ? ? ?{
> ? ? ? ? ? ? ? ?.functions = {"mmc0-1:0", "mmc0-3:2", "mmc0-7:4"},
> ? ? ? ? ? ? ? ?.dev_name = "tegra-sdhci.0",
> ? ? ? ?},
> };
NOW I *think* I get it.
So we're basically talking about the function mapping API here.
So the idea is that one mapping can reference several functions.
So with this scheme actually both ways of doing this would be
possible IIUC.
Note that this will be *quite* tricky to implement, since you have
an array of undetermined size with elements of undetermined size
in .functions = {"mmc0-1:0", "mmc0-3:2", "mmc0-7:4"}.
You will need atleast to move the functions out of the mapping
something like this:
static char *mmc0_10[] = { "mmc0-1:0", "mmc0-3:2", "mmc0-7:4" };
static struct pinmux_map pmx_mapping[] = {
{
.functions = &mmc0_10,
.functions_len = ARRAY_SIZE(mmc0_10);
.dev_name = "tegra-sdhci.0",
},
};
Which sort of kludges up the syntax for all the simple cases that will
also have to add ARRAY_SIZE() and a separate string array for
each of its single-function maps.
So it has the downside of complicating the simple maps.
What I think we need to understand at this point is what will be the
most common case: that a mapping selects a single or multiple
functions. If the case is rare I'd opt for my scheme (i.e. defining
one function per bus width) to keep it simple, whereas if a vast
majority of the function tables and mappings for say Tegra will be
way simpler this way, it'll be worth the increase in complexity
of the function mapping API.
> ... although struct pinmux_map would probably need to grow a separate
> name field
I don't get this, the member "function" is a name of the function:
struct pinmux_map {
const char *function;
(...)
};
> to match against pinmux_get's 2nd parameter, rather than
> assuming the driver's function names and the mapping's function names
> were the same.
>
> Such a new name field would also introduce a second namespace that'd
> satisfy my next concern.
>
>> > ** How can the board/machine name pins? That should be a function of the
>> > chip (i.e. pinmux driver), since that's where the pins are located. A
>> > chip's pins don't have different names on different boards; the names of
>> > the signals on the PCB connected to those pins are defined by the board,
>> > but not the names of the actual pins themselves.
>>
>> Actually I don't really like the use of the concept of board and machine
>> for chip packages either. But if I say that without devicetrees it
>> could be something like arch/arm/plat-nomadik/package-db8500.c
>> defining the pins. Then it is still coming from the outside, for
>> a particular platform, for a particular chip package.
>>
>> Then the init function in that file gets called on relevant systems.
>>
>> As you can see in the example implementation for U300 I actually
>> do this in the driver itself by including the machine.h file to that one.
>> So this driver first registers pins, then functions.
>>
>> I think there is broad consensus that this should come in from
>> the device tree in the future. And if it comes from the device tree, it's
>> still the say arch/arm/plat-nomadik/package-db8500.c file that looks
>> up the pins from the device tree and registers them, just it gets the
>> data from the outside.
>
> I think discussion of DeviceTree here is a bit of a distraction; if the
> pinmux core APIs imply the correct data model, it doesn't seem especially
> relevant.
OK... maybe too much distraction here. All the DT noise
makes me dizzy too :-P
> For the pinmux drivers themselves, I expect the SoC's pinmux driver to
> define the set of pins available, and the set of functions available.
Yes, for those that map 1-to-1 for a certain platform.
I'm a little bit sceptical due to the risk of getting into a situation
where every implementation claims to be special and ending up
having to consolidate a lot of stuff that really wasn't that special a few
years down the road, like the ARM tree right now. But maybe that's
our destiny so no big deal :-)
> There will be a single static set of pins/functions for any Tegra 2
> device. Tegra 3 will have a different set of pins/functions since the
> HW changed quite a bit in this area. As such, DeviceTree doesn't really
> help much, since there will always be a custom driver per SoC, and the
> data a given driver uses will not vary between boards, so DeviceTree
> helps little.
Yep I get it. s/device tree/board file/g and the same reasoning holds
for anything coming from the platform/package/machine data.
> On the other hand, if you've got a very generic pinmux controller, and
> it gets included in N (versions of) SoCs with the same register layout,
> ?but the actual pin names related to each register bit
> are different, then I can see it making sense for the pinmux driver
> itself to parameterize itself with data from DeviceTree.
We have that situation already in Ux500. c.f.:
arch/arm/mach-ux500/pins-db5500.h
arch/arm/mach-ux500/pins-db8500.h
Two ASICs, same set of hardware registers, but vastly different pin
assignments.
> Either way
> though, this is purely an implementation detail within the pinmux driver,
> and not something the core would even be aware of.
OK with me for now atleast, I don't know about other people's
ambitions.
> However, I do think the board-specific list of struct pinmux_map will
> come from DeviceTree. This will vary from board to board with potentially
> little commonality. DeviceTree seems a great solution to keep all that
> varying data out of the kernel. The core pinmux code could well include
> the common code to parse the DeviceTree for this data.
Agreed.
>> > ** struct pinmux_map requires that each function name be globally unique,
>> > since one can only specify "function" not "function on a specific chip".
>> > This can't be a requirement; what if there are two instances of the same
>> > chip on the board (think some expansion chip attached to a memory-mapped
>> > bus rather than the primary SoC itself).
>>
>> Namespace clashes are a problem but the way I see it basically a
>> problem with how you design the namespace. It's just a string and
>> the document is just an example.
>
> The issue here is that the pinmux driver defines a single namespace for
> functions, and struct pinmux_map's definition passes that single namespace
> through, since the value of .function comes from that same namespace, and
> is also the search key for the list of driver-visible functions.
So this would be the case if you say had two identical chips on I2C, both
of which could mix some their pins.
That (in the sense of the present design) would be an argument to mandate
to always pass the function names from platform data to drivers that
handle more than once instance, so the function names are always
unique.
(This is more about understanding, really, tell me if I got it wrong.)
> As I alluded to above, if struct pinmux_map had a separate field for the
> search key (for pinmux_get's use) v.s. the set of driver functions that
> key maps to, then there can be separate namespaces, and a place to resolve
> conflicts:
>
> struct pinmux_map_entry {
> ? ? ? ?struct pinmux_dev *pmxdev
> ? ? ? ?const char *function; // pmxdev's specific function name-space
> };
This looks scary to me: we're already trying to avoid putting
struct device * into the map in favor of using dev_name to look
it up.
How do you intend to get struct pinmux_dev * into the map
entry?
Remember that the map is a static thing that is defined at
compile-time, whereas struct pinmux_dev * is something
that appears at runtime.
It seems smarter to me to use some naming convention,
but it definately need to keep track of instances, so we
need something like the device_name() from the
pinmux_dev:s struct device, that will be unique for each
instance, then match on that.
But maybe if you make a patch I can understand this better,
I might just not see how this could work.
> struct pinmux_map {
> ? ? ? ?const char *name; // global driver-visible function name-space
> ? ? ? ?struct device *dev;
This struct device *dev is curretly unused btw.
I even made my map a __initdata in the U300 case, and I think
that should be the norm, this would only be useful in runtime,
maybe with device trees.
> ? ? ? ?const char *dev_name;
> ? ? ? ?int n_ functions;
> ? ? ? ?const struct pinmux_map_entry * functions;
> };
>
> Thinking about this at a high level, this seems equivalent to the GPIO
> driver:
Sorry, not following. Which GPIO driver? Are you referring to
gpiolib?
> There are two name-spaces there:
>
> 1) One used by clients: A global name-space, equivalent to
> pinmux_map.name.
>
> 2) One used by GPIO drivers: A per-driver name-space, equivalent to
> pinmux_map_entry.function.
>
> ... and the GPIO core maps from (1) to (2) above internally.
>
> In the pinmux case, since everything is named by a string rather than a
> linear number range, the mapping process needs the pinmux_map_entry.function
> field, rather than just substracting a GPIO driver's base GPIO number.
I see you refer to the way that the global GPIO ID is mapped to an
offset into the respective GPIO driver by subtracting the offset of each
GPIO chip. (Correct me if wrong.)
I think I understand what you mean, but I would claim that that an
unsigned integer number space has quite different semantics from a
namespace for them to be compared in that way. For example
it is very simple and quick to map one into the other by a simple
subtraction of an offset.
In this case, with the design above you need to know two
namespaces instead of one when writing your platform data,
and that makes things more complex in my view.
So instead of using a simple 1D string "chip0::mmc0" to identify
some function in a specific driver, you use the tuple
("chip0", "mmc0") and I think it's getting complex and hard to
express as struct members, but that may be because I don't
quite see how clever this is.
To me it seems plausible that the easiest way
to do this is that if a driver wants a private namespace
it just defines a separator such as "::" in above and we provide
some simple string split function to get chipname and function
name from that.
Non-surprsingly, that is exactly what the drivers/leds subsystem
commonly does to identify LEDs on different chips:
drivers/leds$ grep '::' *
dell-led.c: .name = "dell::lid",
leds-ams-delta.c: .name = "ams-delta::camera",
leds-ams-delta.c: .name = "ams-delta::advert",
leds-ams-delta.c: .name = "ams-delta::email",
leds-ams-delta.c: .name = "ams-delta::handsfree",
leds-ams-delta.c: .name = "ams-delta::voicemail",
leds-ams-delta.c: .name = "ams-delta::voice",
leds-clevo-mail.c: .name = "clevo::mail",
leds-cobalt-qube.c: .name = "qube::front",
leds-cobalt-raq.c: .name = "raq::web",
leds-cobalt-raq.c: .name = "raq::power-off",
leds-net48xx.c: .name = "net48xx::error",
leds-wrap.c: .name = "wrap::power",
leds-wrap.c: .name = "wrap::error",
leds-wrap.c: .name = "wrap::extra",
So can't we use such a convention and some helpers
like drivers/leds does instead of trying to actually match
split the namespace to C struct members? It's still a
namespace, it's just not expressed in C, but in string
structure.
If the first part (before the "::") shall be unique per
chip instance, that can be generated in runtime from
the device name or some specific .init_name to make
sure it does not vary too much.
If you're uncertain what to think about this I can implement
this namespacing scheme for U300 so we have some example?
So to summarize there are two related areas of discussion
here:
1. Whether a pinmux map shall map one or 1..N functions
2. How to handle per-driver instance namespacing of functions
In both cases I'm currently using simple strings and claiming
that by namespacing these strings cleverly we can avoid
complexity. So my answer to these are:
1. Use several functions with ovelapping maps, just name
them differently
2. Use a string convention and namespace by using
platform/machine/package data and string conventions
such as a "::" separator
While I *think* (and DO correct me!) that you would argue:
1. Make it possible to map several functions to a single
device map
2. Namespace device instances by different map field
members referring to specific instances
Is this correctly understood, even if we may not agree?
Thanks,
Linus Walleij
Linus Walleij wrote at Thursday, June 16, 2011 6:47 AM:
> On Wed, Jun 15, 2011 at 12:11 AM, Stephen Warren <[email protected]> wrote:
> > [Me]
> >> Can't you just send some patch or example .h file for the API
> >> you would like to see so I understand how you think about
> >> this?
> >
> > Are your patches in git somewhere? It's much easier for me to pull
> > at present than grab patches out of email; something I certainly need
> > to fix...
>
> Yep:
> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git
> pinmux-subsystem
> It's based on v3.0-rc3 as of now. I tend to rebase it though.
Great. Thanks.
> >> static unsigned int mmc0_1_pins[] = { 56, 57 };
> >> static unsigned int mmc0_2_pins[] = { 56, 57, 58, 59 };
> >> static unsigned int mmc0_3_pins[] = { 56, 57, 58, 59, 60, 61, 62, 63 };
> >>
> >> static struct foo_pmx_func myfuncs[] = {
> >> {
> >> .name = "mmc0-2bit",
> >> .pins = mmc0_1_pins,
> >> .num_pins = ARRAY_SIZE(mmc0_1_pins),
> >> },
> >> {
> >> .name = "mmc0-4bit",
> >> .pins = mmc0_2_pins,
> >> .num_pins = ARRAY_SIZE(mmc0_2_pins),
> >> },
> >> {
> >> .name = "mmc0-8bit",
> >> .pins = mmc0_3_pins,
> >> .num_pins = ARRAY_SIZE(mmc0_3_pins),
> >> },
> >> };
> >>
> >> Looks OK?
> >
> > I think that's exactly the setup I was looking to avoid. See the portion
> > of the thread starting from:
>
> Gah. Baudelaire once said something like the reason people think
> they agree is that they misunderstand each other all the time.
> Sorry for my lameness :-/
>
> > static unsigned int mmc0_1_pins[] = { 56, 57 };
> > static unsigned int mmc0_2_pins[] = { 58, 59 };
> > static unsigned int mmc0_3_pins[] = { 60, 61, 62, 63 };
> >
> > {
> > .name = "mmc0-1:0",
> > .pins = mmc0_1_pins,
> > .num_pins = ARRAY_SIZE(mmc0_1_pins),
> > },
> > {
> > .name = "mmc0-3:2",
> > .pins = mmc0_2_pins,
> > .num_pins = ARRAY_SIZE(mmc0_2_pins),
> > },
> > {
> > .name = "mmc0-7:4",
> > .pins = mmc0_3_pins,
> > .num_pins = ARRAY_SIZE(mmc0_3_pins),
> > },
> >
> > So a board that happens to use a 2-bit bus would define:
> >
> > static struct pinmux_map pmx_mapping[] = {
> > {
> > .functions = {"mmc0-1:0"},
> > .dev_name = "tegra-sdhci.0",
> > },
> > };
> >
> > But a board that happens to use an 8-bit bus would define:
> >
> > static struct pinmux_map pmx_mapping[] = {
> > {
> > .functions = {"mmc0-1:0", "mmc0-3:2", "mmc0-7:4"},
> > .dev_name = "tegra-sdhci.0",
> > },
> > };
>
> NOW I *think* I get it.
>
> So we're basically talking about the function mapping API here.
Yes, basically.
In more detail, I'm talking about making the "functions" exposed to
clients of the pinmux be a different set than the "functions"
implemented by the pinmux driver. Perhaps we should call the former
"mappings" not make that clear; I see you did just below!
> So the idea is that one mapping can reference several functions.
Yes.
> So with this scheme actually both ways of doing this would be
> possible IIUC.
Yes, it's certainly possible for a board/machine/... to define
mappings that expose the bus in the chunks that the HW supports,
or aggregate the whole thing.
But, I think that's slightly missing the point. I would expect:
The pinmux driver itself to *always* expose functions as the raw chunks
that the HW supports (whether HW supports individual pins, or groups of
pins, whether multiple pins/groups end up being used as a bus or not).
For Tegra's keyboard controller, that might be the following functions
(actual names/widths probably incorrect; just for example):
row[3:0]
row[7:4]
row[9:8]
col[3:0]
col[7:4]
Recall that Tegra's keyboard controller module can be used with many
different combinations of those sets of pins. I assert the pinmux driver
itself needs to work identically, and expose the same data (functions,
pins) without any knowledge of the board it's being used on.
The mappings, provided by and specific to a particular board/machine
would always expose only the exact configurations actually used on that
board:
mapping device: "tegra-kbc"
mapping name: "config a"
mapping function list: row[7:4], row[3:0], col[3:0]
(note how I added a "mapping name" field here; more on this below. This
is related to mapping and function names being different things)
As a single mapping that the driver will just get and use.
Note that if the driver is complex, and can dynamically switch between
different setups at run-time on a specific board, then the mappings list
might still contain multiple entries for the same device, which the
device driver could get all of and switch between. However, I'd expect
that to be pretty rare:
mapping device: "tegra-kbc"
mapping name: "config a"
mapping function list: row[7:4], row[3:0], col[3:0]
mapping device: "tegra-kbc"
mapping name: "config b"
mapping function list: row[3:0], col[3:0]
> Note that this will be *quite* tricky to implement, since you have
> an array of undetermined size with elements of undetermined size
> in .functions = {"mmc0-1:0", "mmc0-3:2", "mmc0-7:4"}.
>
> You will need atleast to move the functions out of the mapping
> something like this:
>
> static char *mmc0_10[] = { "mmc0-1:0", "mmc0-3:2", "mmc0-7:4" };
>
> static struct pinmux_map pmx_mapping[] = {
> {
> .functions = &mmc0_10,
> .functions_len = ARRAY_SIZE(mmc0_10);
> .dev_name = "tegra-sdhci.0",
> },
> };
>
> Which sort of kludges up the syntax for all the simple cases that will
> also have to add ARRAY_SIZE() and a separate string array for
> each of its single-function maps.
>
> So it has the downside of complicating the simple maps.
Yes, you're right. I think I have a simpler solution that degenerates to
the same syntax as in your current patch. Starting with your original:
static struct pinmux_map pmx_mapping[] = {
{
.dev_name = "tegra-sdhci.0",
.function = "mmc0-1:0",
},
(where devices look up mappings by ".function", among other options)
We then add a new "mapping name" field; you'll see why soon:
static struct pinmux_map pmx_mapping[] = {
{
.dev_name = "tegra-sdhci.0",
.map_name = "2 bit",
.function = "mmc0-1:0",
},
(where we now use ".map_name" for searching the list instead of
".function", which ties into my comment above about having explicit
different sets of names for mapping entries and low-level pinmux driver
internal function names.
We can still set ".map_name" = NULL and omit it in the simple case where
drivers search based on ".dev_name" and don't specify any specific
.map_name to search for, and don't have multiple options for mappings
they can switch between.
Now, we can have multiple entries with the same .map_name:
static struct pinmux_map pmx_mapping[] = {
{
.dev_name = "tegra-sdhci.0",
.map_name = "4 bit", # Note this is 4 now
.function = "mmc0-1:0",
},
{
.dev_name = "tegra-sdhci.0",
.map_name = "4 bit",
.function = "mmc0-3:2",
},
This means that if a client request map name "4 bit", then both functions
"mmc0-1:0" and "mmc0-3:2" are part of that mapping.
If there is only a single logical mapping, and hence .map_name is omitted,
there could still be multiple entries in the list for the same dev_name;
all entries with .map_name==NULL are considered the same name.
Putting the above in database terms, the primary key (albeit not unique)
is the tuple (.dev_name, .map_name), and the set of .function values for
all rows with that same primary key are logically part of the same mapping.
(Yes, this isn't correct normalized form for a database, but it makes the
simpler cases simpler to represent)
We can still have multiple .map_name options for a given .dev_name,
by simply combining the previous two examples:
static struct pinmux_map pmx_mapping[] = {
{
.dev_name = "tegra-sdhci.0",
.map_name = "2 bit",
.function = "mmc0-1:0",
},
{
.dev_name = "tegra-sdhci.0",
.map_name = "4 bit",
.function = "mmc0-1:0",
},
{
.dev_name = "tegra-sdhci.0",
.map_name = "4 bit",
.function = "mmc0-3:2",
},
And as a final addition to this example, suppose we have a second MMC
controller, it can also have a "2 bit" map_name entry, since I'd like
to assert that ".map_name" be interpreted relative/within/for a given
.dev_name in most cases; lookup only by .map_name without a .dev_name
should be rare.
{
.dev_name = "tegra-sdhci.1",
.map_name = "2 bit",
.function = "mmc1-1:0",
},
In terms of implementation, this is still pretty easy; all we do when
reserving or activating a given mapping is to walk the entire list, find
*all* entries that match the client's search terms, and operate on all of
them, instead of stopping at the first one.
struct pinmux, as returned by pinmux_get(), would need some adjustments
to store either an array of map entries, or just the .map_name of the
found entry/entries, so the loop could be repeated later.
> What I think we need to understand at this point is what will be the
> most common case: that a mapping selects a single or multiple
> functions. If the case is rare I'd opt for my scheme (i.e. defining
> one function per bus width) to keep it simple, whereas if a vast
> majority of the function tables and mappings for say Tegra will be
> way simpler this way, it'll be worth the increase in complexity
> of the function mapping API.
So sorry but just to hammer home my point, the disadvantages of the pinmux
driver itself (as opposed to the mapping list) aggregating multiple pins
or groups-of-pins into functions for each supported combination are:
* Potential explosion of functions exposed.
* The same point, but think about a SW bit-banged bus consisting of 8
GPIOs for the bus and 100 pins on the device. Each permutation of 8 out
of 100 is scary... I'd love a board to be able to represent a single
mapping for a particular board's set of GPIOs in this case, but not for
the pinmux driver to expose all possibilities!
* By having the pinmux driver expose the pins/groups in exactly the way
the HW supports, the pinmux driver is purely a simple hardware driver,
and doesn't know anything much beyond "program this pin/group to be this
function". All the logic of aggregating sets of pins/groups-of-pins into
mappings is defined by the board/machine/... when it sets up the mapping
table. I like keeping the pinmux driver simpler, and having the boards
describe functions in terms of which pins/groups should be set to which
function, rather than picking for a pre-defined large list of all
possibilities.
> > ... although struct pinmux_map would probably need to grow a separate
> > name field
>
> I don't get this, the member "function" is a name of the function:
>
> struct pinmux_map {
> const char *function;
> (...)
> };
Hopefully I explained this well above; it's the difference between a
function name known to the pinmux driver vs. a mapping name known to the
pinmux clients.
> > to match against pinmux_get's 2nd parameter, rather than
> > assuming the driver's function names and the mapping's function names
> > were the same.
> >
> > Such a new name field would also introduce a second namespace that'd
> > satisfy my next concern.
> >
> >> > ** How can the board/machine name pins? That should be a function of the
> >> > chip (i.e. pinmux driver), since that's where the pins are located. A
> >> > chip's pins don't have different names on different boards; the names of
> >> > the signals on the PCB connected to those pins are defined by the board,
> >> > but not the names of the actual pins themselves.
> >>
> >> Actually I don't really like the use of the concept of board and machine
> >> for chip packages either. But if I say that without devicetrees it
> >> could be something like arch/arm/plat-nomadik/package-db8500.c
> >> defining the pins. Then it is still coming from the outside, for
> >> a particular platform, for a particular chip package.
> >>
> >> Then the init function in that file gets called on relevant systems.
> >>
> >> As you can see in the example implementation for U300 I actually
> >> do this in the driver itself by including the machine.h file to that one.
> >> So this driver first registers pins, then functions.
> >>
> >> I think there is broad consensus that this should come in from
> >> the device tree in the future. And if it comes from the device tree, it's
> >> still the say arch/arm/plat-nomadik/package-db8500.c file that looks
> >> up the pins from the device tree and registers them, just it gets the
> >> data from the outside.
> >
> > I think discussion of DeviceTree here is a bit of a distraction; if the
> > pinmux core APIs imply the correct data model, it doesn't seem especially
> > relevant.
>
> OK... maybe too much distraction here. All the DT noise
> makes me dizzy too :-P
>
> > For the pinmux drivers themselves, I expect the SoC's pinmux driver to
> > define the set of pins available, and the set of functions available.
>
> Yes, for those that map 1-to-1 for a certain platform.
Personally, I think in all cases.
For a pinmux driver that can apply to different platforms (e.g. the same
register set applies to n different SoCs with different pin layouts), I
think the pinmux driver itself can be parameterized with the mapping from
register bits/fields to function names exported to the pinmux core. The
parameterization could come from platform data, DeviceTree, ...
> I'm a little bit sceptical due to the risk of getting into a situation
> where every implementation claims to be special and ending up
> having to consolidate a lot of stuff that really wasn't that special a few
> years down the road, like the ARM tree right now. But maybe that's
> our destiny so no big deal :-)
>
> > There will be a single static set of pins/functions for any Tegra 2
> > device. Tegra 3 will have a different set of pins/functions since the
> > HW changed quite a bit in this area. As such, DeviceTree doesn't really
> > help much, since there will always be a custom driver per SoC, and the
> > data a given driver uses will not vary between boards, so DeviceTree
> > helps little.
>
> Yep I get it. s/device tree/board file/g and the same reasoning holds
> for anything coming from the platform/package/machine data.
>
> > On the other hand, if you've got a very generic pinmux controller, and
> > it gets included in N (versions of) SoCs with the same register layout,
> > but the actual pin names related to each register bit
> > are different, then I can see it making sense for the pinmux driver
> > itself to parameterize itself with data from DeviceTree.
>
> We have that situation already in Ux500. c.f.:
> arch/arm/mach-ux500/pins-db5500.h
> arch/arm/mach-ux500/pins-db8500.h
>
> Two ASICs, same set of hardware registers, but vastly different pin
> assignments.
OK, does the idea I floated a little above work; having the pinmux driver
itself get its register->function-name mapping from some data structure
provided to the pinmux driver?
As you'll no doubt realize, I think that works:-)
> > Either way
> > though, this is purely an implementation detail within the pinmux driver,
> > and not something the core would even be aware of.
>
> OK with me for now atleast, I don't know about other people's
> ambitions.
That sounds like you agree:-)
> > However, I do think the board-specific list of struct pinmux_map will
> > come from DeviceTree. This will vary from board to board with potentially
> > little commonality. DeviceTree seems a great solution to keep all that
> > varying data out of the kernel. The core pinmux code could well include
> > the common code to parse the DeviceTree for this data.
>
> Agreed.
Great.
> >> > ** struct pinmux_map requires that each function name be globally unique,
> >> > since one can only specify "function" not "function on a specific chip".
> >> > This can't be a requirement; what if there are two instances of the same
> >> > chip on the board (think some expansion chip attached to a memory-mapped
> >> > bus rather than the primary SoC itself).
> >>
> >> Namespace clashes are a problem but the way I see it basically a
> >> problem with how you design the namespace. It's just a string and
> >> the document is just an example.
> >
> > The issue here is that the pinmux driver defines a single namespace for
> > functions, and struct pinmux_map's definition passes that single namespace
> > through, since the value of .function comes from that same namespace, and
> > is also the search key for the list of driver-visible functions.
>
> So this would be the case if you say had two identical chips on I2C, both
> of which could m[u]x some their pins.
Yes.
> That (in the sense of the present design) would be an argument to mandate
> to always pass the function names from platform data to drivers that
> handle more than once instance, so the function names are always
> unique.
>
> (This is more about understanding, really, tell me if I got it wrong.)
I think function names should be interpreted only within the context of a
specific pinmux driver instance.
I think mapping table entry names should generally be used in conjunction
with a device name for lookup, so it's clear which device's mappings
you're searching for.
Now, the final question is, given a mapping entry:
{
.dev_name = "tegra-sdhci.0",
.map_name = "2 bit",
.function = "mmc0-1:0",
},
How do you know which pinmux driver to go to when activating the function?
I think the answer is to expand the mapping table again, to be:
{
.dev_name = "tegra-sdhci.0",
.map_name = "2 bit",
.pmx_dev_name = "tegra-pinmux.0",
.function = "mmc0-1:0",
},
The above entry means:
For device "tegra-sdhci.0",
when it wants to activate its pinmux mapping named "2 bit",
(where that name is optional if there's only 1 for the device)
go to pinmux driver names "tegra-pinmux.0",
and activate function "mmc0-1:0" there.
Each registered pinmux driver would need a name. To cover the case of
multiple instances of the exact same driver, the driver's name could
e.g. encode I2C bus number and address for example "[email protected]". I
think that gpiolib names gpiochips along those lines? The naming that
ASoC (ALSA for Systems on Chip; sound/soc/*) uses for I2C devices
such as codecs works just like this.
> > As I alluded to above, if struct pinmux_map had a separate field for the
> > search key (for pinmux_get's use) v.s. the set of driver functions that
> > key maps to, then there can be separate namespaces, and a place to resolve
> > conflicts:
> >
> > struct pinmux_map_entry {
> > struct pinmux_dev *pmxdev
> > const char *function; // pmxdev's specific function name-space
> > };
>
> This looks scary to me: we're already trying to avoid putting
> struct device * into the map in favor of using dev_name to look
> it up.
>
> How do you intend to get struct pinmux_dev * into the map
> entry?
Yes, having "struct pinmux_dev *" here isn't a great idea. Using a
device's name works much better.
> Remember that the map is a static thing that is defined at
> compile-time, whereas struct pinmux_dev * is something
> that appears at runtime.
>
> It seems smarter to me to use some naming convention,
> but it definately need to keep track of instances, so we
> need something like the device_name() from the
> pinmux_dev:s struct device, that will be unique for each
> instance, then match on that.
OK, I think you're suggesting basically what I wrote above; my email
reading lookahead buffer needs to be larger:-)
> But maybe if you make a patch I can understand this better,
> I might just not see how this could work.
>
> > struct pinmux_map {
> > const char *name; // global driver-visible function name-space
> > struct device *dev;
>
> This struct device *dev is curretly unused btw.
>
> I even made my map a __initdata in the U300 case, and I think
> that should be the norm, this would only be useful in runtime,
> maybe with device trees.
Hmm. I think the mapping table ends up being used at runtime a lot;
I think it's reasonable for drivers to be able to expect to pinmux_get()
at any time. New pinmux devices could be added/removed at arbitrary
times, e.g. if a hotplugged PCI or USB device contained pinmuxing
capabilities.
> > const char *dev_name;
> > int n_ functions;
> > const struct pinmux_map_entry * functions;
> > };
> >
> > Thinking about this at a high level, this seems equivalent to the GPIO
> > driver:
>
> Sorry, not following. Which GPIO driver? Are you referring to
> gpiolib?
Yes.
> > There are two name-spaces there:
> >
> > 1) One used by clients: A global name-space, equivalent to
> > pinmux_map.name.
> >
> > 2) One used by GPIO drivers: A per-driver name-space, equivalent to
> > pinmux_map_entry.function.
> >
> > ... and the GPIO core maps from (1) to (2) above internally.
> >
> > In the pinmux case, since everything is named by a string rather than a
> > linear number range, the mapping process needs the pinmux_map_entry.function
> > field, rather than just substracting a GPIO driver's base GPIO number.
>
> I see you refer to the way that the global GPIO ID is mapped to an
> offset into the respective GPIO driver by subtracting the offset of each
> GPIO chip. (Correct me if wrong.)
Yes.
> I think I understand what you mean, but I would claim that that an
> unsigned integer number space has quite different semantics from a
> namespace for them to be compared in that way. For example
> it is very simple and quick to map one into the other by a simple
> subtraction of an offset.
>
> In this case, with the design above you need to know two
> namespaces instead of one when writing your platform data,
> and that makes things more complex in my view.
Well, I suppose two namespaces is indeed more complex than 1.
However, in the simple case of no duplicates, you can probably get away
with using the same names in each namespace... And having the option to
use two separate namespaces solves a bunch of problems I've talked about
above.
> So instead of using a simple 1D string "chip0::mmc0" to identify
> some function in a specific driver, you use the tuple
> ("chip0", "mmc0") and I think it's getting complex and hard to
> express as struct members, but that may be because I don't
> quite see how clever this is.
>
> To me it seems plausible that the easiest way
> to do this is that if a driver wants a private namespace
> it just defines a separator such as "::" in above and we provide
> some simple string split function to get chipname and function
> name from that.
>
> Non-surprsingly, that is exactly what the drivers/leds subsystem
> commonly does to identify LEDs on different chips:
Interesting. Having a mapping be:
{
.dev_name = "tegra-sdhci.0",
.map_name = "2 bit",
.function = " tegra-pinmux.0::mmc0-1:0",
},
Instead of:
{
.dev_name = "tegra-sdhci.0",
.map_name = "2 bit",
.pmx_dev_name = "tegra-pinmux.0",
.function = "mmc0-1:0",
},
Seems fine to me.
However, having the pinmux drivers statically define their prefixes in
strings at compile-time doesn't seem correct; it prevents one from having
multiple instances of the same pinmux/led driver; having separate fields
makes this pretty easy, and avoids having to dynamically construct
prefixed strings at runtime.
> drivers/leds$ grep '::' *
> dell-led.c: .name = "dell::lid",
> leds-ams-delta.c: .name = "ams-delta::camera",
> leds-ams-delta.c: .name = "ams-delta::advert",
> leds-ams-delta.c: .name = "ams-delta::email",
> leds-ams-delta.c: .name = "ams-delta::handsfree",
> leds-ams-delta.c: .name = "ams-delta::voicemail",
> leds-ams-delta.c: .name = "ams-delta::voice",
> leds-clevo-mail.c: .name = "clevo::mail",
> leds-cobalt-qube.c: .name = "qube::front",
> leds-cobalt-raq.c: .name = "raq::web",
> leds-cobalt-raq.c: .name = "raq::power-off",
> leds-net48xx.c: .name = "net48xx::error",
> leds-wrap.c: .name = "wrap::power",
> leds-wrap.c: .name = "wrap::error",
> leds-wrap.c: .name = "wrap::extra",
>
> So can't we use such a convention and some helpers
> like drivers/leds does instead of trying to actually match
> split the namespace to C struct members? It's still a
> namespace, it's just not expressed in C, but in string
> structure.
>
> If the first part (before the "::") shall be unique per
> chip instance, that can be generated in runtime from
> the device name or some specific .init_name to make
> sure it does not vary too much.
>
> If you're uncertain what to think about this I can implement
> this namespacing scheme for U300 so we have some example?
>
> So to summarize there are two related areas of discussion
> here:
>
> 1. Whether a pinmux map shall map one or 1..N functions
> 2. How to handle per-driver instance namespacing of functions
>
> In both cases I'm currently using simple strings and claiming
> that by namespacing these strings cleverly we can avoid
> complexity. So my answer to these are:
>
> 1. Use several functions with ovelapping maps, just name
> them differently
> 2. Use a string convention and namespace by using
> platform/machine/package data and string conventions
> such as a "::" separator
OK. I think I've address what I see as the disadvantages in those
solutions in my rather long-wided discussions above:-)
> While I *think* (and DO correct me!) that you would argue:
>
> 1. Make it possible to map several functions to a single
> device map
Yes.
> 2. Namespace device instances by different map field
> members referring to specific instances
Yes.
> Is this correctly understood, even if we may not agree?
>
> Thanks,
> Linus Walleij
--
nvpublic
On Thu, Jun 16, 2011 at 9:10 PM, Stephen Warren <[email protected]> wrote:
> Linus Walleij wrote at Thursday, June 16, 2011 6:47 AM:
>> NOW I ?*think* I get it.
>>
>> So we're basically talking about the function mapping API here.
>
> Yes, basically.
>
> In more detail, I'm talking about making the "functions" exposed to
> clients of the pinmux be a different set than the "functions"
> implemented by the pinmux driver. Perhaps we should call the former
> "mappings" not make that clear; I see you did just below!
OK I get it now I believe.
> The mappings, provided by and specific to a particular board/machine
> would always expose only the exact configurations actually used on that
> board:
>
> mapping device: "tegra-kbc"
> mapping name: "config a"
> mapping function list: row[7:4], row[3:0], col[3:0]
>
> (note how I added a "mapping name" field here; more on this below. This
> is related to mapping and function names being different things)
OK so one mapping reference several functions in this way,
I see.
>> You will need atleast to move the functions out of the mapping
>> something like this:
>>
>> static char *mmc0_10[] = { "mmc0-1:0", "mmc0-3:2", "mmc0-7:4" };
>>
>> static struct pinmux_map pmx_mapping[] = {
>> ? ? ? ?{
>> ? ? ? ? ? ? ? ?.functions = &mmc0_10,
>> ? ? ? ? ? ? ? ?.functions_len = ARRAY_SIZE(mmc0_10);
>> ? ? ? ? ? ? ? ?.dev_name = "tegra-sdhci.0",
>> ? ? ? ?},
>> };
>>
>> Which sort of kludges up the syntax for all the simple cases that will
>> also have to add ARRAY_SIZE() and a separate string array for
>> each of its single-function maps.
>>
>> So it has the downside of complicating the simple maps.
>
> Yes, you're right. I think I have a simpler solution that degenerates to
> the same syntax as in your current patch. Starting with your original:
>
> static struct pinmux_map pmx_mapping[] = {
> ? ? ? {
> ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> ? ? ? ? ? ? ? .function = "mmc0-1:0",
> ? ? ? },
>
> (where devices look up mappings by ".function", among other options)
>
> We then add a new "mapping name" field; you'll see why soon:
>
> static struct pinmux_map pmx_mapping[] = {
> ? ? ? {
> ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> ? ? ? ? ? ? ? .map_name = "2 bit",
> ? ? ? ? ? ? ? .function = "mmc0-1:0",
> ? ? ? },
>
> (where we now use ".map_name" for searching the list instead of
> ".function", which ties into my comment above about having explicit
> different sets of names for mapping entries and low-level pinmux driver
> internal function names.
>
> We can still set ".map_name" = NULL and omit it in the simple case where
> drivers search based on ".dev_name" and don't specify any specific
> .map_name to search for, and don't have multiple options for mappings
> they can switch between.
>
> Now, we can have multiple entries with the same .map_name:
>
> static struct pinmux_map pmx_mapping[] = {
> ? ? ? {
> ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> ? ? ? ? ? ? ? .map_name = "4 bit", # Note this is 4 now
> ? ? ? ? ? ? ? .function = "mmc0-1:0",
> ? ? ? },
> ? ? ? {
> ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> ? ? ? ? ? ? ? .map_name = "4 bit",
> ? ? ? ? ? ? ? .function = "mmc0-3:2",
> ? ? ? },
>
> This means that if a client request map name "4 bit", then both functions
> "mmc0-1:0" and "mmc0-3:2" are part of that mapping.
(...)
> In terms of implementation, this is still pretty easy; all we do when
> reserving or activating a given mapping is to walk the entire list, find
> *all* entries that match the client's search terms, and operate on all of
> them, instead of stopping at the first one.
So:
pmx = pinmux_get(dev, "4 bit");
in this case would reserve pins for two functions on pinmux_get() and
activate two different functions after one another when
we pinmux_enable(pmx) on this, "mmc0-1:0" and "mmc0-3:2" in some
undefined order (inferenced across namespace).
I don't think it's as simple as you say, this gets hairy pretty quickly:
Since my previous pinmux_get() would create a struct pinmux * cookie
for each map entry, assuming a 1:1 relationship between map entries
and pinmuxes, we now break this, and you need to introduce
more complex logic to search through the pinmux_list and dynamically
add more functions to each entry with a matching map_name.
Then you need to take care of the case where acquiring pins for
one of the functions fail: then you need to go back and free the
already acquired pins in the error path.
I tried quickly to see if I could code this up, and it got very complex
real quick, sadly. Maybe if I can just get to iterate a v4 first, I
could venture down this track.
But if you can code up the patch for this, I'm pretty much game for
it!
> struct pinmux, as returned by pinmux_get(), would need some adjustments
> to store either an array of map entries, or just the .map_name of the
> found entry/entries, so the loop could be repeated later.
Yes if we can just write the code for it I buy into it. :-)
> So sorry but just to hammer home my point, the disadvantages of the pinmux
> driver itself (as opposed to the mapping list) aggregating multiple pins
> or groups-of-pins into functions for each supported combination are:
>
> * Potential explosion of functions exposed.
Yes I understand this, what I need to know if this is a problem in real
life, i.e. that it's not just a function explosion in theory on some 5000
pin chip with a plethora of mux capabilities, but on real chips existing
now, so it's worth all the extra abstraction and code incurred by this.
But I do trust you if you say it's a real problem on the Tegra.
> * The same point, but think about a SW bit-banged bus consisting of 8
> GPIOs for the bus and 100 pins on the device. Each permutation of 8 out
> of 100 is scary... I'd love a board to be able to represent a single
> mapping for a particular board's set of GPIOs in this case, but not for
> the pinmux driver to expose all possibilities!
Is this a real world problem or a theoretical one? Seems like the
latter, and as stated in the documentation this subsystem is not about
solving discrete maths permutation spaces, but practical enumerations.
> * By having the pinmux driver expose the pins/groups in exactly the way
> the HW supports, the pinmux driver is purely a simple hardware driver,
> and doesn't know anything much beyond "program this pin/group to be this
> function". All the logic of aggregating sets of pins/groups-of-pins into
> mappings is defined by the board/machine/... when it sets up the mapping
> table. I like keeping the pinmux driver simpler, and having the boards
> describe functions in terms of which pins/groups should be set to which
> function, rather than picking for a pre-defined large list of all
> possibilities.
This is a real good argument and I buy it wholesale, if the associated
cost in code complexity does not run amok.
> For a pinmux driver that can apply to different platforms (e.g. the same
> register set applies to n different SoCs with different pin layouts), I
> think the pinmux driver itself can be parameterized with the mapping from
> register bits/fields to function names exported to the pinmux core. The
> parameterization could come from platform data, DeviceTree, ...
(...)
>> We have that situation already in Ux500. c.f.:
>> arch/arm/mach-ux500/pins-db5500.h
>> arch/arm/mach-ux500/pins-db8500.h
>>
>> Two ASICs, same set of hardware registers, but vastly different pin
>> assignments.
>
> OK, does the idea I floated a little above work; having the pinmux driver
> itself get its register->function-name mapping from some data structure
> provided to the pinmux driver?
Yes I think so. We can adapt to that.
[next subject]
> I think mapping table entry names should generally be used in conjunction
> with a device name for lookup, so it's clear which device's mappings
> you're searching for.
>
> Now, the final question is, given a mapping entry:
>
> ? ? ? {
> ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> ? ? ? ? ? ? ? .map_name = "2 bit",
> ? ? ? ? ? ? ? .function = "mmc0-1:0",
> ? ? ? },
>
> How do you know which pinmux driver to go to when activating the function?
>
> I think the answer is to expand the mapping table again, to be:
>
> ? ? ? {
> ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> ? ? ? ? ? ? ? .map_name = "2 bit",
> ? ? ? ? ? ? ? .pmx_dev_name = "tegra-pinmux.0",
> ? ? ? ? ? ? ? .function = "mmc0-1:0",
> ? ? ? },
>
> The above entry means:
>
> For device "tegra-sdhci.0",
> when it wants to activate its pinmux mapping named "2 bit",
> ?(where that name is optional if there's only 1 for the device)
> go to pinmux driver names "tegra-pinmux.0",
> and activate function "mmc0-1:0" there.
>
> Each registered pinmux driver would need a name. To cover the case of
> multiple instances of the exact same driver, the driver's name could
> e.g. encode I2C bus number and address for example "[email protected]". I
> think that gpiolib names gpiochips along those lines? The naming that
> ASoC (ALSA for Systems on Chip; sound/soc/*) uses for I2C devices
> such as codecs works just like this.
OK sounds reasonable. But like we have an optional
struct device *dev in the mapping as an alternative to
.dev_name we should have an optional .pmx_dev too.
But I get the idea, and it'll probably work.
>> Non-surprsingly, that is exactly what the drivers/leds subsystem
>> commonly does to identify LEDs on different chips:
>
> Interesting. Having a mapping be:
>
> ? ? ? {
> ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> ? ? ? ? ? ? ? .map_name = "2 bit",
> ? ? ? ? ? ? ? .function = " tegra-pinmux.0::mmc0-1:0",
> ? ? ? },
>
> Instead of:
>
> ? ? ? {
> ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> ? ? ? ? ? ? ? .map_name = "2 bit",
> ? ? ? ? ? ? ? .pmx_dev_name = "tegra-pinmux.0",
> ? ? ? ? ? ? ? .function = "mmc0-1:0",
> ? ? ? },
>
> Seems fine to me.
No, the other idea with a pmx_dev_name is much better, I was
misguided in this. That namespace style only gets hard to maintain
I think.
> However, having the pinmux drivers statically define their prefixes in
> strings at compile-time doesn't seem correct; it prevents one from having
> multiple instances of the same pinmux/led driver; having separate fields
> makes this pretty easy, and avoids having to dynamically construct
> prefixed strings at runtime.
You are right.
>> While I *think* (and DO correct me!) that you would argue:
>>
>> 1. Make it possible to map several functions to a single
>> ? device map
>
> Yes.
>
>> 2. Namespace device instances by different map field
>> ? members referring to specific instances
>
> Yes.
Atleast we know what the remaining problem space is now, surely we
can iron this out. But I think I need some working code for that.
Now I will post some v4 version of the framework, with some minor
corrections and bugfixes, then we can do this larger redesign on
top of that, patches welcome! I'll put in a git link.
Thanks.
Linus Walleij
Linus Walleij wrote at Monday, June 27, 2011 8:35 AM:
> On Thu, Jun 16, 2011 at 9:10 PM, Stephen Warren <[email protected]> wrote:
...
> > Now, we can have multiple entries with the same .map_name:
> >
> > static struct pinmux_map pmx_mapping[] = {
> > ? ? ? {
> > ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> > ? ? ? ? ? ? ? .map_name = "4 bit", # Note this is 4 now
> > ? ? ? ? ? ? ? .function = "mmc0-1:0",
> > ? ? ? },
> > ? ? ? {
> > ? ? ? ? ? ? ? .dev_name = "tegra-sdhci.0",
> > ? ? ? ? ? ? ? .map_name = "4 bit",
> > ? ? ? ? ? ? ? .function = "mmc0-3:2",
> > ? ? ? },
> >
> > This means that if a client request map name "4 bit", then both functions
> > "mmc0-1:0" and "mmc0-3:2" are part of that mapping.
>
> (...)
>
> > In terms of implementation, this is still pretty easy; all we do when
> > reserving or activating a given mapping is to walk the entire list, find
> > *all* entries that match the client's search terms, and operate on all of
> > them, instead of stopping at the first one.
>
> So:
> pmx = pinmux_get(dev, "4 bit");
>
> in this case would reserve pins for two functions on pinmux_get() and
> activate two different functions after one another when
> we pinmux_enable(pmx) on this, "mmc0-1:0" and "mmc0-3:2" in some
> undefined order (inferenced across namespace).
>
> I don't think it's as simple as you say, this gets hairy pretty quickly:
>
> Since my previous pinmux_get() would create a struct pinmux * cookie
> for each map entry, assuming a 1:1 relationship between map entries
> and pinmuxes, we now break this, and you need to introduce
> more complex logic to search through the pinmux_list and dynamically
> add more functions to each entry with a matching map_name.
In the patch you posted, pinmux_get looked up a single specific
pinmux_map* and stored it in the struct pinmux. You're right that
continuing to do something similar means needing to store an array of
pinmux_map* in the struct pinmux, and dynamically adding more and more
entries as you search through the mapping table. A bit painful.
However, if instead of that, you store the original parameters to
pinmux_get() in struct pinmux, there's nothing to dynamically store
there. Instead, the implementation of pinmux_get and pinmux_enable is
very roughly:
pinmux_get:
Allocate struct pinmux, fill it in:
list_for_each_entry(mapping table)
if (matches search terms)
check_not_already_in_use
acquire_pins()
// Note: no break here
pinmux_enable:
// Note we redo the whole search here based on the original terms
// rather than having pinmux_get pre-calculate the search result
list_for_each_entry(mapping table)
if (matches search terms)
ops->enable()
In fact, I'd imagine that the list_for_each_entry call above could be
placed into a utility function that implements the searching logic, to
keep the code in one place, with a function parameter:
pinmux_search(terms, callback):
list_for_each_entry(mapping table)
if (matches search terms)
callback()
pinmux_get_body():
check_not_already_in_use
acquire_pins()
pinmux_get(terms):
Allocate struct pinmux, fill it in:
pinmux_search(terms, pinmux_get_body)
pinmux_enable_body():
ops->enable()
pinmux_enable():
pinmux_search(terms, pinmux_enable_body)
Alternatively, each map entry contains a list node that's set up.
The list is set up by pinmux_get, and contains all mapping entries
associated with the original search terms. Struct pinmux points at
the head of the list. pinmux_enable then just needs to iterate over
that list, not the whole mapping array, and doesn't need to implement
the search algorithm. That might actually be simpler. Since each
mapping entry would only be get'd once, there'd be no conflicts with
multiple things trying to use the one list node at once.
> Then you need to take care of the case where acquiring pins for
> one of the functions fail: then you need to go back and free the
> already acquired pins in the error path.
That's not that hard; just iterate over the whole list again, applying
a function that undoes what you did before. It's just like the error
handling code that already exists in acquire_pins for example. The only
hard part might be stopping when you get to the point you already got
to in the first loop, but actually that's just a matter of passing in
a "stop at this index" parameter to the pinmux_search() I mentioned
above.
> I tried quickly to see if I could code this up, and it got very complex
> real quick, sadly. Maybe if I can just get to iterate a v4 first, I
> could venture down this track.
>
> But if you can code up the patch for this, I'm pretty much game for
> it!
Sure, I can add the extra looping to the code if you want. It'd be easiest
if you set up the data structures in the initial patch such that all the
fields exist in the mapping table (i.e. how I showed in the concept header
files I posted). That way, there'd be no type or semantic changes in my
patch, just support for acting on N instead of just 1 mapping entry at a
time.
...
> > * The same point, but think about a SW bit-banged bus consisting of 8
> > GPIOs for the bus and 100 pins on the device. Each permutation of 8 out
> > of 100 is scary... I'd love a board to be able to represent a single
> > mapping for a particular board's set of GPIOs in this case, but not for
> > the pinmux driver to expose all possibilities!
>
> Is this a real world problem or a theoretical one? Seems like the
> latter, and as stated in the documentation this subsystem is not about
> solving discrete maths permutation spaces, but practical enumerations.
That's theoretical in terms of the boards/code I'm currently dealing with.
However, solving this seems pretty simple, and I can't imagine nobody will
ever want to do that.
--
nvpublic