Re: [PATCH 1/2] drivers: create a pin control subsystem v9

On Mon, Oct 03, 2011 at 10:17:42AM +0200, Linus Walleij wrote:

From: Linus Walleij linus.walleij@linaro.org

This creates a subsystem for handling of pin control devices. These are devices that control different aspects of package pins.

Currently it handles pinmuxing, i.e. assigning electronic functions to groups of pins on primarily PGA and BGA type of chip packages which are common in embedded systems.

The plan is to also handle other I/O pin control aspects such as biasing, driving, input properties such as schmitt-triggering, load capacitance etc within this subsystem, to remove a lot of ARM arch code as well as feature-creepy GPIO drivers which are implementing the same thing over and over again.

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/pinctrl.txt file that is part of this patch for more details.

Cc: Grant Likely grant.likely@secretlab.ca Cc: Stijn Devriendt highguy@gmail.com Cc: Joe Perches joe@perches.com Cc: Russell King linux@arm.linux.org.uk Acked-by: Stephen Warren swarren@nvidia.com Tested-by: Barry Song 21cnbao@gmail.com Signed-off-by: Linus Walleij linus.walleij@linaro.org

---

ChangeLog v8->v9:

• Drop the bus_type and the sysfs attributes and all, we're not on the clear about how this should be used for e.g. userspace interfaces so let us save this for the future.

• Use the right name in MAINTAINERS, PIN CONTROL rather than PINMUX

• Don't kfree() the device state holder, let the .remove() callback handle this.

• Fix up numerous kerneldoc headers to have one line for the function description and more verbose documentation below the parameters

Nit: put the changelog above the s-o-b lines so it will appear in the linux commit log.

---

Documentation/pinctrl.txt | 951 +++++++++++++++++++++++++++++++ MAINTAINERS | 5 + drivers/Kconfig | 4 + drivers/Makefile | 2 + drivers/pinctrl/Kconfig | 29 + drivers/pinctrl/Makefile | 6 + drivers/pinctrl/core.c | 602 ++++++++++++++++++++ drivers/pinctrl/core.h | 73 +++ drivers/pinctrl/pinmux.c | 1179 +++++++++++++++++++++++++++++++++++++++ drivers/pinctrl/pinmux.h | 47 ++ include/linux/pinctrl/machine.h | 107 ++++ include/linux/pinctrl/pinctrl.h | 133 +++++ include/linux/pinctrl/pinmux.h | 117 ++++ 13 files changed, 3255 insertions(+), 0 deletions(-) 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 drivers/pinctrl/core.h create mode 100644 drivers/pinctrl/pinmux.c create mode 100644 drivers/pinctrl/pinmux.h create mode 100644 include/linux/pinctrl/machine.h create mode 100644 include/linux/pinctrl/pinctrl.h create mode 100644 include/linux/pinctrl/pinmux.h

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt new file mode 100644 index 0000000..2915fea --- /dev/null +++ b/Documentation/pinctrl.txt @@ -0,0 +1,951 @@

[...]

+The pin control subsystem will call the .list_groups() function repeatedly +beginning on 0 until it returns non-zero to determine legal selectors, then +it will call the other functions to retrieve the name and pins of the group. +Maintaining the data structure of the groups is up to the driver, this is +just a simple example - in practice you may need more entries in your group +structure, for example specific register ranges associated with each group +and so on.

+Interaction with the GPIO subsystem +===================================

+The GPIO drivers may want to perform operations of various types on the same +physical pins that are also registered as GPIO pins.

...also registered as PINMUX pins?

diff --git a/drivers/Kconfig b/drivers/Kconfig index 95b9e7e..40d3e16 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

This shouldn't actually be an ordering constraint. It might be a temporary restriction on the Makefiles, but it isn't at all for Kconfig.

+source "drivers/pinctrl/Kconfig"

source "drivers/gpio/Kconfig" source "drivers/w1/Kconfig" diff --git a/drivers/Makefile b/drivers/Makefile index 7fa433a..e7afb3a 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..7fa0fe0 --- /dev/null +++ b/drivers/pinctrl/Kconfig @@ -0,0 +1,29 @@ +# +# PINCTRL infrastructure and drivers +#

+menuconfig PINCTRL

• bool "PINCTRL Support"
• depends on SYSFS && EXPERIMENTAL

Why depends on SYSFS? That shouldn't be a consideration at all.

• 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 PINMUX

• bool "Support pinmux controllers"
• help

•  Say Y here if you want the pincontrol subsystem to handle pin
  

•  multiplexing drivers.
  

+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.
  

+endif diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile new file mode 100644 index 0000000..596ce9f --- /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) += pinmux.o diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c new file mode 100644 index 0000000..ff0c68c --- /dev/null +++ b/drivers/pinctrl/core.c @@ -0,0 +1,602 @@ +/*

• • Core driver for the pin control 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 linus.walleij@linaro.org
• • 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/pinctrl.h> +#include <linux/pinctrl/machine.h> +#include "core.h" +#include "pinmux.h"

+/* Global list of pin control devices */ +static DEFINE_MUTEX(pinctrldev_list_mutex); +static LIST_HEAD(pinctrldev_list);

+static void pinctrl_dev_release(struct device *dev) +{

• struct pinctrl_dev *pctldev = dev_get_drvdata(dev);
• kfree(pctldev);

+}

+const char *pctldev_get_name(struct pinctrl_dev *pctldev) +{

• /* We're not allowed to register devices without name */
• return pctldev->desc->name;

+} +EXPORT_SYMBOL_GPL(pctldev_get_name);

+void *pctldev_get_drvdata(struct pinctrl_dev *pctldev) +{

• return pctldev->driver_data;

+} +EXPORT_SYMBOL_GPL(pctldev_get_drvdata);

+/**

• • get_pctldev_from_dev() - look up pin controller device

Naming abbreviation is a little agressive. Please use pinctrl everywhere instead of a mix between pctl and pinctrl.

• • @dev: a device pointer, this may be NULL but then devname needs to be
• • defined instead
• • @devname: the name of a device instance, as returned by dev_name(), this
• • may be NULL but then dev needs to be defined instead
• • Looks up a pin control device matching a certain device name or pure device
• • pointer, the pure device pointer will take precedence.
• */

+struct pinctrl_dev *get_pctldev_from_dev(struct device *dev,

• 			 const char *devname)
  

+{

• struct pinctrl_dev *pctldev = NULL;
• bool found = false;
• mutex_lock(&pinctrldev_list_mutex);
• list_for_each_entry(pctldev, &pinctrldev_list, node) {

• if (dev &&  &pctldev->dev == dev) {
  

• 	/* Matched on device pointer */
  

• 	found = true;
  

• 	break;
  

• }
  

• if (devname &&
  

•     !strcmp(dev_name(&pctldev->dev), devname)) {
  

• 	/* Matched on device name */
  

• 	found = true;
  

• 	break;
  

• }
  

• }
• mutex_unlock(&pinctrldev_list_mutex);
• if (found)

• return pctldev;
  

• return NULL;

or simply:

return found ? pctldev : NULL;

+/**

• • pinctrl_get_device_gpio_range() - find device for GPIO range
• • @gpio: the pin to locate the pin controller for
• • @outdev: the pin control device if found
• • @outrange: the GPIO range if found
• • Find the pin controller handling a certain GPIO pin from the pinspace of
• • the GPIO subsystem, return the device and the matching GPIO range. Returns
• • negative if the GPIO range could not be found in any device.
• */

+int pinctrl_get_device_gpio_range(unsigned gpio,

• 		struct pinctrl_dev **outdev,
  

• 		struct pinctrl_gpio_range **outrange)
  

+{

• struct pinctrl_dev *pctldev = NULL;
• /* Loop over the pin controllers */
• mutex_lock(&pinctrldev_list_mutex);
• list_for_each_entry(pctldev, &pinctrldev_list, node) {

• struct pinctrl_gpio_range *range;
  

• range = pinctrl_match_gpio_range(pctldev, gpio);
  

• if (range != NULL) {
  

• 	*outdev = pctldev;
  

• 	*outrange = range;
  

• 	return 0;
  

Neglected to drop mutex

• }
  

• }
• mutex_unlock(&pinctrldev_list_mutex);
• return -EINVAL;

+}

+/**

• • pinctrl_register() - register a pin controller device
• • @pctldesc: descriptor for this pin controller
• • @dev: parent device for this pin controller
• • @driver_data: private pin controller data for this pin controller
• */

+struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,

• 		    struct device *dev, void *driver_data)
  

+{

• static atomic_t pinmux_no = ATOMIC_INIT(0);
• struct pinctrl_dev *pctldev;
• int ret;
• if (pctldesc == NULL)

• return ERR_PTR(-EINVAL);
  

I urge you to consider carefully before relying on the ERR_PTR() pattern. It isn't easy to for a compiler or a human to check if ERR_PTR values are being handled properly, and is therefore a likely source of bugs. Unless it is *absolutely critical* to return an error code instead of NULL on error, I strongly recommend returning NULL from this function on failure.

From what I see from this function, the error codes are less useful

that using pr_err() calls.

• if (pctldesc->name == NULL)

• return ERR_PTR(-EINVAL);
  

• /* If we're implementing pinmuxing, check the ops for sanity */
• if (pctldesc->pmxops) {

• ret = pinmux_check_ops(pctldesc->pmxops);
  

• if (ret) {
  

• 	pr_err("%s pinmux ops lacks necessary functions\n",
  

• 	       pctldesc->name);
  

• 	return ERR_PTR(ret);
  

• }
  

• }
• pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
• if (pctldev == NULL)

• return ERR_PTR(-ENOMEM);
  

• /* Initialize pin control device struct */
• pctldev->owner = pctldesc->owner;
• pctldev->desc = pctldesc;
• pctldev->driver_data = driver_data;
• INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
• spin_lock_init(&pctldev->pin_desc_tree_lock);
• INIT_LIST_HEAD(&pctldev->gpio_ranges);
• mutex_init(&pctldev->gpio_ranges_lock);
• /* Register device */
• pctldev->dev.parent = dev;
• dev_set_name(&pctldev->dev, "pinctrl.%d",

•      atomic_inc_return(&pinmux_no) - 1);
  

• pctldev->dev.release = pinctrl_dev_release;
• ret = device_register(&pctldev->dev);
• if (ret != 0) {

• pr_err("error in device registration\n");
  

• put_device(&pctldev->dev);
  

• kfree(pctldev);
  

• goto out_err;
  

• }
• dev_set_drvdata(&pctldev->dev, pctldev);
• /* Register all the pins */
• pr_debug("try to register %d pins on %s...\n",

•  pctldesc->npins, pctldesc->name);
  

• ret = pinctrl_register_pins(pctldev, pctldesc->pins, pctldesc->npins);
• if (ret) {

• pr_err("error during pin registration\n");
  

• pinctrl_free_pindescs(pctldev, pctldesc->pins,
  

• 		      pctldesc->npins);
  

• goto out_err;
  

• }
• pinctrl_init_device_debugfs(pctldev);
• mutex_lock(&pinctrldev_list_mutex);
• list_add(&pctldev->node, &pinctrldev_list);
• mutex_unlock(&pinctrldev_list_mutex);
• pinmux_hog_maps(pctldev);
• return pctldev;

+out_err:

• put_device(&pctldev->dev);
• return ERR_PTR(ret);

+} +EXPORT_SYMBOL_GPL(pinctrl_register);

I think this is pretty close to ready. If you address the comments I've made above then you can add my Acked-by:

Acked-by: Grant Likely grant.likely@secretlab.ca