[PATCH 1/4 v5] drivers: create a pin control subsystem v5
Jamie Iles
jamie at jamieiles.com
Tue Aug 30 06:02:45 EDT 2011
Hi Linus,
This is looking really great! A couple of pedantic nits inline, but
with the gpio ranges support I think this covers all of the bases that
we need from pin control, so thanks!
Jamie
On Mon, Aug 29, 2011 at 11:10:01AM +0200, Linus Walleij wrote:
[...]
> 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..762e9e8
> --- /dev/null
> +++ b/drivers/pinctrl/core.c
> @@ -0,0 +1,539 @@
> +/*
> + * 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 at 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);
> +
> +/* sysfs interaction */
> +static ssize_t pinctrl_name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pinctrl_dev *pctldev = dev_get_drvdata(dev);
As you have a struct device in pctldev, you should be able to do:
#define to_pinctrl_dev(__dev) \
container_of(__dev, struct pinctrl_dev, dev)
struct pinctrl_dev *pctldev = to_pinctrl_dev(dev);
then you don't need to use the driver data.
[...]
> +static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
> +{
> + static struct dentry *device_root;
Does device_root need to be static?
> +
> + device_root = debugfs_create_dir(dev_name(&pctldev->dev),
> + debugfs_root);
> + if (IS_ERR(device_root) || !device_root) {
> + pr_warn("failed to create debugfs directory for %s\n",
> + dev_name(&pctldev->dev));
> + return;
> + }
> + debugfs_create_file("pins", S_IFREG | S_IRUGO,
> + device_root, pctldev, &pinctrl_pins_ops);
> + debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
> + device_root, pctldev, &pinctrl_gpioranges_ops);
> + pinmux_init_device_debugfs(device_root, pctldev);
> +}
> +
> +static void pinctrl_init_debugfs(void)
> +{
> + debugfs_root = debugfs_create_dir("pinctrl", NULL);
> + if (IS_ERR(debugfs_root) || !debugfs_root) {
IS_ERR_OR_NULL()?
> + pr_warn("failed to create debugfs directory\n");
> + debugfs_root = NULL;
> + return;
> + }
> +
> + debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
> + debugfs_root, NULL, &pinctrl_devices_ops);
> + pinmux_init_debugfs(debugfs_root);
> +}
> +
> +#else /* CONFIG_DEBUG_FS */
> +
> +static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
> +{
> +}
> +
> +static void pinctrl_init_debugfs(void)
> +{
> +}
> +
> +#endif
> +
> +/**
> + * 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);
> + 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)
> + 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);
> + spin_lock_init(&pctldev->gpio_ranges_lock);
> +
> + /* Register device with sysfs */
> + pctldev->dev.parent = dev;
> + pctldev->dev.bus = &pinctrl_bus;
> + pctldev->dev.type = &pinctrl_type;
> + dev_set_name(&pctldev->dev, "pinctrl.%d",
> + atomic_inc_return(&pinmux_no) - 1);
> + ret = device_register(&pctldev->dev);
> + if (ret != 0) {
> + pr_err("error in device registration\n");
> + put_device(&pctldev->dev);
> + kfree(pctldev);
I don't think you need the kfree() here as it should get called in the
devices release method.
> + 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);
> + return pctldev;
> +
> +out_err:
> + put_device(&pctldev->dev);
> + kfree(pctldev);
I think this needs to be a device_unregister() rather than put device,
and the kfree() can be dropped.
Possibly also do a dev_set_drvdata(&pctldev->dev, NULL) here too?
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> new file mode 100644
> index 0000000..310d344
> --- /dev/null
> +++ b/drivers/pinctrl/pinmux.c
> @@ -0,0 +1,811 @@
> +/*
> + * Core driver for the pin muxing portions of 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 at linaro.org>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +#define pr_fmt(fmt) "pinmux 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>
> +#include "core.h"
> +
> +/* Global list of pinmuxes */
> +static DEFINE_MUTEX(pinmux_list_mutex);
> +static LIST_HEAD(pinmux_list);
> +
> +/**
> + * struct pinmux - per-device pinmux state holder
> + * @node: global list node - only for internal use
> + * @dev: the device using 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)
> + * @pctldev: pin control device handling this pinmux
> + * @pmxdev_selector: the function selector for the pinmux device handling
> + * this pinmux
> + * @pmxdev_position: the function position for the pinmux device and
> + * selector 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 pinctrl_dev *pctldev;
> + unsigned pmxdev_selector;
> + unsigned pmxdev_position;
> + struct mutex mutex;
> +};
> +
> +/**
> + * 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
> + * @gpio_range: the range matching the GPIO pin if this is a request for a
> + * single GPIO pin
> + */
> +static int pin_request(struct pinctrl_dev *pctldev,
> + int pin, const char *function, bool gpio,
> + struct pinctrl_gpio_range *gpio_range)
@gpio_range is only valid if @gpio == true, so we could drop @gpio and
only do gpio_request_enable if gpio_range != NULL? At least that way we
can't call gpio_request_enable with a NULL gpio_range (which I don't
think is valid?).
[...]
> +/**
> + * 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 pinctrl_dev *pctldev;
> + const struct pinmux_ops *ops;
> + int ret = 0;
> +
> + if (pmx == NULL)
> + return -ENODEV;
> +
> + pctldev = pmx->pctldev;
> + ops = pctldev->desc->pmxops;
> +
> + /* This operation is not mandatory to implement */
> + if (ops->config) {
> + mutex_lock(&pmx->mutex);
> + ret = ops->config(pctldev, pmx->pmxdev_selector, param, data);
> + mutex_unlock(&pmx->mutex);
> + }
> +
> + return 0;
Shouldn't this return ret from ops->config()?
More information about the linux-arm-kernel
mailing list