Message-ID: <20111013031819.GA15829@ponder.secretlab.ca>
Date:	Wed, 12 Oct 2011 21:18:19 -0600
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Linus Walleij <linus.walleij@...ricsson.com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Stephen Warren <swarren@...dia.com>,
	Barry Song <21cnbao@...il.com>,
	Lee Jones <lee.jones@...aro.org>,
	Joe Perches <joe@...ches.com>,
	Russell King <linux@....linux.org.uk>,
	Linaro Dev <linaro-dev@...ts.linaro.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	David Brown <davidb@...eaurora.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Stijn Devriendt <highguy@...il.com>
Subject: 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@...aro.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@...retlab.ca>
> Cc: Stijn Devriendt <highguy@...il.com>
> Cc: Joe Perches <joe@...ches.com>
> Cc: Russell King <linux@....linux.org.uk>
> Acked-by: Stephen Warren <swarren@...dia.com>
> Tested-by: Barry Song <21cnbao@...il.com>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.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@...aro.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@...retlab.ca>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists