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

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

[Shawn Guo]

On Mon, Oct 03, 2011 at 10:17:42AM +0200, Linus Walleij wrote:
[...]
> diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> new file mode 100644
> index 0000000..2cd4033
> --- /dev/null
> +++ b/include/linux/pinctrl/machine.h
> @@ -0,0 +1,107 @@
> +/*
> + * 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 <linus.walleij@linaro.org>
> + *
> + * 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
> + * @name: the name of this specific map entry for the particular machine.
> + *	This is the second parameter passed to pinmux_get() when you want
> + *	to have several mappings to the same device
> + * @ctrl_dev: the pin control device to be used by this mapping, may be NULL
> + *	if you provide .ctrl_dev_name instead (this is more common)
> + * @ctrl_dev_name: the name of the device controlling this specific mapping,
> + *	the name must be the same as in your struct device*, may be NULL if
> + *	you provide .ctrl_dev instead
> + * @function: a function in the driver to use for this mapping, the driver
> + *	will lookup the function referenced by this ID on the specified
> + *	pin control device
> + * @group: sometimes a function can map to different pin groups, so this
> + *	selects a certain specific pin group to activate for the function, if
> + *	left as NULL, the first applicable group will be used
> + * @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 as in your struct device*, may be NULL if you
> + *	provide .dev instead
> + * @hog_on_boot: if this is set to true, the regulator subsystem will itself
						^^^^^^^^^
s/regulator/pinmux?

> + *	hog the mappings as the pinmux device drivers are attched, so this is
> + *	typically used with system maps (mux mappings without an assigned
> + *	device) that you want to get hogged and enabled by default as soon as
> + *	a pinmux device supporting it is registered. These maps will not be
> + *	disabled and put until the system shuts down.
> + */
> +struct pinmux_map {
> +	const char *name;
> +	struct device *ctrl_dev;
> +	const char *ctrl_dev_name;
> +	const char *function;
> +	const char *group;
> +	struct device *dev;
> +	const char *dev_name;
> +	const bool hog_on_boot;
> +};
> +
> +/*
> + * Convenience macro to set a simple map from a certain pin controller and a
> + * certain function to a named device
> + */
> +#define PINMUX_MAP(a, b, c, d) \
> +	{ .name = a, .ctrl_dev_name = b, .function = c, .dev_name = d }
> +
> +/*
> + * Convenience macro to map a system function onto a certain pinctrl device.
> + * System functions are not assigned to a particular device.
> + */
> +#define PINMUX_MAP_SYS(a, b, c) \
> +	{ .name = a, .ctrl_dev_name = b, .function = c }
> +
> +/*
> + * Convenience macro to map a function onto the primary device pinctrl device
> + * this is especially helpful on systems that have only one pin controller
> + * or need to set up a lot of mappings on the primary controller.
> + */
> +#define PINMUX_MAP_PRIMARY(a, b, c) \
> +	{ .name = a, .ctrl_dev_name = "pinctrl.0", .function = b, \
> +	  .dev_name = c }
> +
> +/*
> + * Convenience macro to map a system function onto the primary pinctrl device.
> + * System functions are not assigned to a particular device.
> + */
> +#define PINMUX_MAP_PRIMARY_SYS(a, b) \
> +	{ .name = a, .ctrl_dev_name = "pinctrl.0", .function = b }
> +
> +/*
> + * Convenience macro to map a system function onto the primary pinctrl device,
> + * to be hogged by the pinmux core until the system shuts down.
> + */
> +#define PINMUX_MAP_PRIMARY_SYS_HOG(a, b) \
> +	{ .name = a, .ctrl_dev_name = "pinctrl.0", .function = b, \
> +	  .hog_on_boot = true }
> +
> +
> +#ifdef CONFIG_PINMUX
> +
> +extern int pinmux_register_mappings(struct pinmux_map const *map,
> +				unsigned num_maps);
> +
> +#else
> +
> +static inline int pinmux_register_mappings(struct pinmux_map const *map,
> +					   unsigned num_maps)
> +{
> +	return 0;
> +}
> +
> +#endif /* !CONFIG_PINCTRL */

s/!CONFIG_PINCTRL/CONFIG_PINMUX?

> +#endif

-- 
Regards,
Shawn

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

[Linus Walleij]

On Sun, Oct 9, 2011 at 11:36 AM, Shawn Guo <shawn.guo@freescale.com> wrote:

>> + * @hog_on_boot: if this is set to true, the regulator subsystem will itself
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^^^^^^^
> s/regulator/pinmux?

Yep!

>> +#endif /* !CONFIG_PINCTRL */
>
> s/!CONFIG_PINCTRL/CONFIG_PINMUX?

Yep!

Foldes into original patch with a fixup comment.

Can I have your Reviewed-by: tag on this subsystem?

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, Oct 10, 2011 at 2:30 PM, Shawn Guo <shawn.guo@freescale.com> wrote:

>> +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
>
> It should just be pinctrl_register_pins()?

Yep, fixed it.

>> +System pinmux hogging
>> +=====================
>> +
>> +A system pinmux map entry, i.e. a pinmux setting that does not have a device
>> +associated with it, can be hogged by the core when the pin controller is
>> +registered. This means that the core will attempt to call regulator_get() and
>> +regulator_enable() on it immediately after the pin control device has been
>> +registered.
>
> s/regulator_get/pinmux_get, and s/regulator_enable/pinmux_enable

Fixed this too, sorry for being so hung up on the regulator subsystem,
must be that I really like it...

Thanks,
Linus Walleij

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

[Shawn Guo]

On Mon, Oct 10, 2011 at 10:23:53AM +0200, Linus Walleij wrote:
> On Sun, Oct 9, 2011 at 11:36 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> 
> >> + * @hog_on_boot: if this is set to true, the regulator subsystem will itself
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^^^^^^^
> > s/regulator/pinmux?
> 
> Yep!
> 
> >> +#endif /* !CONFIG_PINCTRL */
> >
> > s/!CONFIG_PINCTRL/CONFIG_PINMUX?
> 
> Yep!
> 
> Foldes into original patch with a fixup comment.
> 
> Can I have your Reviewed-by: tag on this subsystem?
> 
Sorry, not yet.  This is not a full review.  I'm trying to migrate imx
pinctrl to the subsystem.  And all these are just some random spotting.

Another couple of typo on pinctrl.txt?

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

It should just be pinctrl_register_pins()?

> +earlier.

[...]

> +System pinmux hogging
> +=====================
> +
> +A system pinmux map entry, i.e. a pinmux setting that does not have a device
> +associated with it, can be hogged by the core when the pin controller is
> +registered. This means that the core will attempt to call regulator_get() and
> +regulator_enable() on it immediately after the pin control device has been
> +registered.

s/regulator_get/pinmux_get, and s/regulator_enable/pinmux_enable

-- 
Regards,
Shawn

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

[Chanho Park]

> +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.
> +
> +Since the pin controller subsystem have its pinspace local to the pin
> +controller we need a mapping so that the pin control subsystem can figure out
> +which pin controller handles control of a certain GPIO pin. Since a single
> +pin controller may be muxing several GPIO ranges (typically SoCs that have
> +one set of pins but internally several GPIO silicon blocks, each modeled as
> +a struct gpio_chip) any number of GPIO ranges can be added to a pin controller
> +instance like this:
> +
> +struct gpio_chip chip_a;
> +struct gpio_chip chip_b;
> +
> +static struct pinctrl_gpio_range gpio_range_a = {
> + ? ? ? .name = "chip a",
> + ? ? ? .id = 0,
> + ? ? ? .base = 32,
> + ? ? ? .npins = 16,
> + ? ? ? .gc = &chip_a;
> +};
> +
> +static struct pinctrl_gpio_range gpio_range_a = {
> + ? ? ? .name = "chip b",
> + ? ? ? .id = 0,
> + ? ? ? .base = 48,
> + ? ? ? .npins = 8,
> + ? ? ? .gc = &chip_b;
> +};
> +
> +
> +{
> + ? ? ? struct pinctrl_dev *pctl;
> + ? ? ? ...
> + ? ? ? pinctrl_add_gpio_range(pctl, &gpio_range_a);
> + ? ? ? pinctrl_add_gpio_range(pctl, &gpio_range_b);
> +}
> +
> +So this complex system has one pin controller handling two different
> +GPIO chips. Chip a has 16 pins and chip b has 8 pins. They are mapped in
> +the global GPIO pin space at:
> +
> +chip a: [32 .. 47]
> +chip b: [48 .. 55]
> +
> +When GPIO-specific functions in the pin control subsystem are called, these
> +ranges will be used to look up the apropriate pin controller by inspecting
> +and matching the pin to the pin ranges across all controllers. When a
> +pin controller handling the matching range is found, GPIO-specific functions
> +will be called on that specific pin controller.
> +
> +For all functionalities dealing with pin biasing, pin muxing etc, the pin
> +controller subsystem will subtract the range's .base offset from the passed
> +in gpio pin number, and pass that on to the pin control driver, so the driver
> +will get an offset into its handled number range. Further it is also passed
> +the range ID value, so that the pin controller knows which range it should
> +deal with.
> +
> +For example: if a user issues pinctrl_gpio_set_foo(50), the pin control
> +subsystem will find that the second range on this pin controller matches,
> +subtract the base 48 and call the
> +pinctrl_driver_gpio_set_foo(pinctrl, range, 2) where the latter function has
> +this signature:
> +
> +int pinctrl_driver_gpio_set_foo(struct pinctrl_dev *pctldev,
> + ? ?struct pinctrl_gpio_range *rangeid,
> + ? ?unsigned offset);
> +
> +Now the driver knows that we want to do some GPIO-specific operation on the
> +second GPIO range handled by "chip b", at offset 2 in that specific range.
> +
> +(If the GPIO subsystem is ever refactored to use a local per-GPIO controller
> +pin space, this mapping will need to be augmented accordingly.)
> +

Hello,
Some gpio-ranges doesn't match with pin numbers.
For example, gpio_range_b starts gpio 48.
However, a pin base number of gpio_range_b is 96. It isn't same with gpio base.
A pinctrl driver must know this pin_space-gpio_range mappings.

static struct pinctrl_gpio_range gpio_range_a = {
 ? ? ? .name = "chip b",
 ? ? ? .id = 0,
 ? ? ? .base = 48,
       .pin_base = 96,
 ? ? ? .npins = 8,
 ? ? ? .gc = &chip_b;
};

To solve this sparse gpio-range issue, you will implement
above pinctrl_driver_gpio_xxx which not yet implemented.
How about below simple patch just added pin_base instead
using pinctrl_driver_gpio_xxx function?

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index a0a90a2..7d7f7c0 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -223,7 +223,7 @@ int pinmux_request_gpio(unsigned gpio)
 		return -EINVAL;

 	/* Convert to the pin controllers number space */
-	pin = gpio - range->base;
+	pin = gpio - range->base + range->pin_base;

 	/* Conjure some name stating what chip and pin this is taken by */
 	snprintf(gpiostr, 15, "%s:%d", range->name, gpio);
@@ -248,7 +248,7 @@ void pinmux_free_gpio(unsigned gpio)
                return;

        /* Convert to the pin controllers number space */
-       pin = gpio - range->base;
+       pin = gpio - range->base + range->pin_base;

        pin_free(pctldev, pin);
 }
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 1e6f67e..6041fd4 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -53,6 +53,7 @@ struct pinctrl_gpio_range {
 	const char *name;
 	unsigned int id;
 	unsigned int base;
+	unsigned int pin_base;
 	unsigned int npins;
 	struct gpio_chip *gc;
 };


...snip...

+/**
+ * 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;

missing mutex_unlock

+                       return 0;
+               }
+       }
+       mutex_unlock(&pinctrldev_list_mutex);
+
+       return -EINVAL;
+}

-- 
Best Regards,
Chanho Park

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

[Grant Likely]

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.

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

[Linus Walleij]

On Thu, Oct 13, 2011 at 2:55 AM, Chanho Park <parkch98@gmail.com> wrote:

> Some gpio-ranges doesn't match with pin numbers.
> For example, gpio_range_b starts gpio 48.
> However, a pin base number of gpio_range_b is 96. It isn't same with gpio base.
> A pinctrl driver must know this pin_space-gpio_range mappings.

The GPIO pin space is one global space, and the only thing
the struct pinctrl_gpio_range does is to say that this pin range
is handled by this chip. You can register several ranges to the
same pin controller.

Then the pin control driver gets called like this:

pin_request(pctldev, pin, gpiostr, true, range);
ops->gpio_request_enable(pctldev, gpio_range, pin);

In this case you know that pin is an index into the range
supplied, note that we always get the range into the driver.

These ranges are defined by the driver as well, and it has
an .id field. Thus the driver shall add/subtract whatever offset
it needs to map that GPIO pin into a proper pin number,
there can be so many strange ways of doing this that the
pin control framework is not dealing with it.

So I think this should be done by the driver, and a clever
way is to use the .id field of the range as index to offset
arrays etc.

> +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;
>
> missing mutex_unlock

Argh!

Thanks, fixed it.

Yours,
Linus Walleij

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

[Linus Walleij]

On Thu, Oct 13, 2011 at 5:18 AM, Grant Likely <grant.likely@secretlab.ca> wrote:

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

OK I'll collect all of them and stash them into it for v10

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

Yes.

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

True, was blinded by similar comments in that Kconfig, such as
this:
# input before char - char/joystick depends on it. As does USB.

Anyway I took out the comment.

>> +menuconfig PINCTRL
>> + ? ? bool "PINCTRL Support"
>> + ? ? depends on SYSFS && EXPERIMENTAL
>
> Why depends on SYSFS? ?That shouldn't be a consideration at all.

Dropped last week. Leftover from when I registered this
bus...

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

OK. Renamed a few functions accordingly.
Also added a fixup to the SirfPrimaII driver to use
the new names.

>> + ? ? if (found)
>> + ? ? ? ? ? ? return pctldev;
>> +
>> + ? ? return NULL;
>
> or simply:
>
> ? ? ? ?return found ? pctldev : NULL;

OK.

>> + ? ? /* 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

Yep noted by another reviewe also today...

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

Agreed, we return NULL on error instead.
Design pattern comes from clocks and regulators, it's not
that useful in this case, maybe not in the other cases either.

Fixed usage in U300 and Sirf controller as well.

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

Thanks a lot Grant!

Yours,
Linus Walleij

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

[Chanho Park]

2011/10/13 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Oct 13, 2011 at 2:55 AM, Chanho Park <parkch98@gmail.com> wrote:
>
>> Some gpio-ranges doesn't match with pin numbers.
>> For example, gpio_range_b starts gpio 48.
>> However, a pin base number of gpio_range_b is 96. It isn't same with gpio base.
>> A pinctrl driver must know this pin_space-gpio_range mappings.
>
> The GPIO pin space is one global space, and the only thing
> the struct pinctrl_gpio_range does is to say that this pin range
> is handled by this chip. You can register several ranges to the
> same pin controller.
>
> Then the pin control driver gets called like this:
>
> pin_request(pctldev, pin, gpiostr, true, range);
> ops->gpio_request_enable(pctldev, gpio_range, pin);
>
> In this case you know that pin is an index into the range
> supplied, note that we always get the range into the driver.
>
> These ranges are defined by the driver as well, and it has
> an .id field. Thus the driver shall add/subtract whatever offset
> it needs to map that GPIO pin into a proper pin number,
> there can be so many strange ways of doing this that the
> pin control framework is not dealing with it.
>
> So I think this should be done by the driver, and a clever
> way is to use the .id field of the range as index to offset
> arrays etc.
>

The pinmux_request_gpio function requires a gpio number.
The function converts the gpio number to the proper pin number.
I considered offset array to convert gpio-pin mapping in the my pinmux driver.
However, the pinmux_request_gpio function checks
whether pin is requested by converted pin number.

For example, I have three gpio ranges.

static struct pinctrl_gpio_range gpio_range0 = {
	.name = "MYGPIO*",
	.id = 0,
	.base = 0,
	.npins = 32,
};

static struct pinctrl_gpio_range gpio_range1 = {
	.name = "MYGPIO*",
	.id = 1,
	.base = 32,
	.npins = 8,
};

static struct pinctrl_gpio_range gpio_range2 = {
	.name = "MYGPIO*",
	.id = 2,
	.base = 40,
	.npins = 16,
};

A gpio base number of gpio_range0 is 0 and pin base number is also 0.
Converting gpio-pin number is no problem about the gpio_range0.
A gpio base number of gpio_range1 is 32 but pin base number is 40(not 32).

Let's suppose pin number 32 is already requested previously.
If you want to request gpio 32(pin number : 40),
it is failed because the pinmux_request_gpio converts gpio 32 to pin number 32.

+	pin = gpio - range->base;

Thus we require to convert a requested gpio number to a proper pin number
before the pinmux driver does it.

-- 
Best Regards,
Chanho Park

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

[Linus Walleij]

2011/10/14 Chanho Park <parkch98@gmail.com>:

> A gpio base number of gpio_range0 is 0 and pin base number is also 0.
> Converting gpio-pin number is no problem about the gpio_range0.
> A gpio base number of gpio_range1 is 32 but pin base number is 40(not 32).

Aha, I understand now, sorry I'm too slow in the head. :-/

Can you make a patch on top of the stuff that is currently
in linux-next adding the pin_base that you need?

I don't think there is any hurry of getting this in, but we sure
need to have it along with the first driver having this problem,
which I guess could be your driver.

Yours,
Linus Walleij