Subject: [PATCH v2 1/3] pinctrl: Add Qualcomm TLMM driver

From: [email-masked] (Stephen Boyd)
Date: Fri, 06 Dec 2013 13:40:08 -0800

General nitpick: There seems to be a lot of checks for invalid input in
the op functions. I hope that they're all unnecessary debugging that can
be removed.

On 12/05/13 18:10, Bjorn Andersson wrote:
> This adds a pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm TLMM block.
>
> Signed-off-by: Bjorn Andersson <[email-masked]>
> ---
>  drivers/pinctrl/Kconfig       |    6 +
>  drivers/pinctrl/Makefile      |    1 +
>  drivers/pinctrl/pinctrl-msm.c | 1028 +++++++++++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-msm.h |  123 +++++
>  4 files changed, 1158 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-msm.c
>  create mode 100644 drivers/pinctrl/pinctrl-msm.h
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 33f9dc1..d0b6846 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -202,6 +202,12 @@ config PINCTRL_IMX28
>  	bool
>  	select PINCTRL_MXS
>  
> +config PINCTRL_MSM
> +	bool

Why not tristate?

> +	select PINMUX
> +	select PINCONF
> +	select GENERIC_PINCONF
> +
>  config PINCTRL_NOMADIK
>  	bool "Nomadik pin controller driver"
>  	depends on ARCH_U8500 || ARCH_NOMADIK
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 4f7be29..a525f8b 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_PINCTRL_FALCON)	+= pinctrl-falcon.o
>  obj-$(CONFIG_PINCTRL_MXS)	+= pinctrl-mxs.o
>  obj-$(CONFIG_PINCTRL_IMX23)	+= pinctrl-imx23.o
>  obj-$(CONFIG_PINCTRL_IMX28)	+= pinctrl-imx28.o
> +obj-$(CONFIG_PINCTRL_MSM)	+= pinctrl-msm.o
>  obj-$(CONFIG_PINCTRL_NOMADIK)	+= pinctrl-nomadik.o
>  obj-$(CONFIG_PINCTRL_STN8815)	+= pinctrl-nomadik-stn8815.o
>  obj-$(CONFIG_PINCTRL_DB8500)	+= pinctrl-nomadik-db8500.o
> diff --git a/drivers/pinctrl/pinctrl-msm.c b/drivers/pinctrl/pinctrl-msm.c
> new file mode 100644
> index 0000000..28b90ab
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-msm.c
> @@ -0,0 +1,1028 @@
> +/*
> + * Copyright (c) 2013, Sony Mobile Communications AB.
> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/irqdomain.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/of_irq.h>
> +#include <linux/spinlock.h>
> +
> +#include "core.h"
> +#include "pinconf.h"
> +#include "pinctrl-msm.h"
> +#include "pinctrl-utils.h"
> +
> +/**
> + * struct msm_pinctrl - state for a pinctrl-msm device
> + * @dev:            device handle.
> + * @pctrl:          pinctrl handle.
> + * @domain:         irqdomain handle.
> + * @chip:           gpiochip handle.
> + * @irq:            parent irq for the TLMM irq_chip.
> + * @lock:           Spinlock to protect register resources as well
> + *                  as msm_pinctrl data structures.
> + * @enabled_irqs:   Bitmap of currently enabled irqs.
> + * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> + *                  detection.
> + * @wake_irqs:      Bitmap of irqs with requested as wakeup source.
> + * @soc;            Reference to soc_data of platform specific data.
> + * @regs:           Base address for the TLMM register map.
> + */
> +struct msm_pinctrl {
> +	struct device *dev;
> +	struct pinctrl_dev *pctrl;
> +	struct irq_domain *domain;
> +	struct gpio_chip chip;
> +	unsigned irq;
> +
> +	spinlock_t lock;
> +
> +	unsigned long *enabled_irqs;
> +	unsigned long *dual_edge_irqs;
> +	unsigned long *wake_irqs;

In the gpio driver we went with a static upper limit on the bitmap. That
allowed us to avoid small allocations fragmenting the heap. Please do
the same here (look at gpio-msm-v2.c).

> +
> +	const struct msm_pinctrl_soc_data *soc;
> +	void __iomem *regs;
> +};
> +
> +static int msm_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->soc->ngroups;
> +}
> +
> +static const char *msm_get_group_name(struct pinctrl_dev *pctldev,
> +				      unsigned group)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->soc->groups[group].name;
> +}
> +
> +static int msm_get_group_pins(struct pinctrl_dev *pctldev,
> +			      unsigned group,
> +			      const unsigned **pins,
> +			      unsigned *num_pins)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*pins = pctrl->soc->groups[group].pins;
> +	*num_pins = pctrl->soc->groups[group].npins;
> +	return 0;
> +}
> +
> +static struct pinctrl_ops msm_pinctrl_ops = {

const?

> +	.get_groups_count	= msm_get_groups_count,
> +	.get_group_name		= msm_get_group_name,
> +	.get_group_pins		= msm_get_group_pins,
> +	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
> +	.dt_free_map		= pinctrl_utils_dt_free_map,
> +};
> +
> +static int msm_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->soc->nfunctions;
> +}
> +
> +static const char *msm_get_function_name(struct pinctrl_dev *pctldev,
> +					 unsigned function)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->soc->functions[function].name;
> +}
> +
> +static int msm_get_function_groups(struct pinctrl_dev *pctldev,
> +				   unsigned function,
> +				   const char * const **groups,
> +				   unsigned * const num_groups)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*groups = pctrl->soc->functions[function].groups;
> +	*num_groups = pctrl->soc->functions[function].ngroups;
> +	return 0;
> +}
> +
> +static int msm_pinmux_enable(struct pinctrl_dev *pctldev,
> +			     unsigned function,
> +			     unsigned group)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct msm_pingroup *g;
> +	unsigned long flags;
> +	u32 val;
> +	int i;
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	if (WARN_ON(g->mux_bit < 0))
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(g->funcs); i++) {
> +		if (g->funcs[i] == function)
> +			break;
> +	}
> +
> +	if (WARN_ON(i == ARRAY_SIZE(g->funcs)))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->ctl_reg);
> +	val &= ~(0x7 << g->mux_bit);
> +	val |= i << g->mux_bit;
> +	writel(val, pctrl->regs + g->ctl_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void msm_pinmux_disable(struct pinctrl_dev *pctldev,
> +			       unsigned function,
> +			       unsigned group)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct msm_pingroup *g;
> +	unsigned long flags;
> +	u32 val;
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	if (WARN_ON(g->mux_bit < 0))
> +		return;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	/* Clear the mux bits to select gpio mode */
> +	val = readl(pctrl->regs + g->ctl_reg);
> +	val &= ~(0x7 << g->mux_bit);
> +	writel(val, pctrl->regs + g->ctl_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static struct pinmux_ops msm_pinmux_ops = {

const?

> +	.get_functions_count	= msm_get_functions_count,
> +	.get_function_name	= msm_get_function_name,
> +	.get_function_groups	= msm_get_function_groups,
> +	.enable			= msm_pinmux_enable,
> +	.disable		= msm_pinmux_disable,
> +};
> +
> +static int msm_config_reg(struct msm_pinctrl *pctrl,
> +			  const struct msm_pingroup *g,
> +			  unsigned param,
> +			  unsigned *reg,
> +			  unsigned *mask,
> +			  unsigned *bit)
> +{
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		*reg = g->ctl_reg;
> +		*bit = g->pull_bit;
> +		*mask = 3;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		*reg = g->ctl_reg;
> +		*bit = g->pull_bit;
> +		*mask = 3;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		*reg = g->ctl_reg;
> +		*bit = g->pull_bit;
> +		*mask = 3;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		*reg = g->ctl_reg;
> +		*bit = g->drv_bit;
> +		*mask = 7;
> +		break;
> +	default:
> +		dev_err(pctrl->dev, "Invalid config param %04x\n", param);
> +		return -ENOTSUPP;
> +	}
> +
> +	if (*reg < 0) {
> +		dev_err(pctrl->dev, "Config param %04x not supported on group %s\n",
> +			param, g->name);
> +		return -ENOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int msm_config_get(struct pinctrl_dev *pctldev,
> +			  unsigned int pin,
> +			  unsigned long *config)
> +{
> +	dev_err(pctldev->dev, "pin_config_set op not supported\n");
> +	return -ENOTSUPP;
> +}
> +
> +static int msm_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> +				unsigned long *configs, unsigned num_configs)
> +{
> +	dev_err(pctldev->dev, "pin_config_set op not supported\n");
> +	return -ENOTSUPP;
> +}
> +
> +#define MSM_NO_PULL	0
> +#define MSM_PULL_DOWN	1
> +#define MSM_PULL_UP	3
> +
> +static const unsigned msm_regval_to_drive[] = { 2, 4, 6, 8, 10, 12, 14, 16 };
> +static const unsigned msm_drive_to_regval[] = { -1, -1, 0, -1, 1, -1, 2, -1, 3, -1, 4, -1, 5, -1, 6, -1, 7 };
> +
> +static int msm_config_group_get(struct pinctrl_dev *pctldev,
> +				unsigned int group,
> +				unsigned long *config)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned param = pinconf_to_config_param(*config);
> +	unsigned mask;
> +	unsigned arg;
> +	unsigned bit;
> +	unsigned reg;
> +	int ret;
> +	u32 val;
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	ret = msm_config_reg(pctrl, g, param, &reg, &mask, &bit);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = readl(pctrl->regs + reg);
> +	arg = (val >> bit) & mask;
> +
> +	/* Convert register value to pinconf value */
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		arg = arg == MSM_NO_PULL;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		arg = arg == MSM_PULL_DOWN;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		arg = arg == MSM_PULL_UP;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		arg = msm_regval_to_drive[arg];
> +		break;
> +	default:
> +		dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
> +			param);
> +		return -EINVAL;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, arg);
> +
> +	return 0;
> +}
> +
> +static int msm_config_group_set(struct pinctrl_dev *pctldev,
> +				unsigned group,
> +				unsigned long *configs,
> +				unsigned num_configs)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned long flags;
> +	unsigned param;
> +	unsigned mask;
> +	unsigned arg;
> +	unsigned bit;
> +	unsigned reg;
> +	int ret;
> +	u32 val;
> +	int i;
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		arg = pinconf_to_config_argument(configs[i]);
> +
> +		ret = msm_config_reg(pctrl, g, param, &reg, &mask, &bit);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Convert pinconf values to register values */
> +		switch (param) {
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			arg = MSM_NO_PULL;
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			arg = MSM_PULL_DOWN;
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +			arg = MSM_PULL_UP;
> +			break;
> +		case PIN_CONFIG_DRIVE_STRENGTH:
> +			/* Check for invalid values */
> +			if (arg > ARRAY_SIZE(msm_drive_to_regval))
> +				arg = -1;
> +			else
> +				arg = msm_drive_to_regval[arg];
> +			break;
> +		default:
> +			dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
> +				param);
> +			return -EINVAL;
> +		}
> +
> +		/* Range-check user-supplied value */
> +		if (arg & ~mask) {
> +			dev_err(pctrl->dev, "config %x: %x is invalid\n", param, arg);
> +			return -EINVAL;
> +		}
> +
> +		spin_lock_irqsave(&pctrl->lock, flags);
> +		val = readl(pctrl->regs + reg);
> +		val &= ~(mask << bit);
> +		val |= arg << bit;
> +		writel(val, pctrl->regs + reg);
> +		spin_unlock_irqrestore(&pctrl->lock, flags);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct pinconf_ops msm_pinconf_ops = {

const?

> +	.pin_config_get		= msm_config_get,
> +	.pin_config_set		= msm_config_set,
> +	.pin_config_group_get	= msm_config_group_get,
> +	.pin_config_group_set	= msm_config_group_set,
> +};
> +
> +static struct pinctrl_desc msm_pinctrl_desc = {
> +	.pctlops = &msm_pinctrl_ops,
> +	.pmxops = &msm_pinmux_ops,
> +	.confops = &msm_pinconf_ops,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (WARN_ON(offset >= pctrl->soc->ngroups))
> +		return -EINVAL;
> +
> +	g = &pctrl->soc->groups[offset];
> +
> +	if (WARN_ON(g->oe_bit < 0))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->ctl_reg);
> +	val &= ~BIT(g->oe_bit);
> +	writel(val, pctrl->regs + g->ctl_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (WARN_ON(offset >= pctrl->soc->ngroups))
> +		return -EINVAL;

How is this possible?

> +
> +	g = &pctrl->soc->groups[offset];
> +
> +	if (WARN_ON(g->oe_bit < 0))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	writel(value ? BIT(g->out_bit) : 0, pctrl->regs + g->io_reg);
> +
> +	val = readl(pctrl->regs + g->ctl_reg);
> +	val |= BIT(g->oe_bit);
> +	writel(val, pctrl->regs + g->ctl_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int msm_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	u32 val;
> +
> +	if (WARN_ON(offset >= pctrl->soc->ngroups))
> +		return -EINVAL;
> +
> +	g = &pctrl->soc->groups[offset];
> +
> +	val = readl(pctrl->regs + g->io_reg);
> +	return !!(val & BIT(g->in_bit));
> +}
> +
> +static void msm_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (WARN_ON(offset >= pctrl->soc->ngroups))
> +		return;
> +
> +	g = &pctrl->soc->groups[offset];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->io_reg);
> +	val |= BIT(g->out_bit);
> +	writel(val, pctrl->regs + g->io_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +
> +	return irq_find_mapping(pctrl->domain, offset);
> +}
> +
> +static int msm_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +	int gpio = chip->base + offset;
> +	return pinctrl_request_gpio(gpio);
> +}
> +
> +static void msm_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +	int gpio = chip->base + offset;
> +	return pinctrl_free_gpio(gpio);
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +#include <linux/seq_file.h>
> +
> +static void msm_gpio_dbg_show_one(struct seq_file *s,
> +				  struct pinctrl_dev *pctldev,
> +				  struct gpio_chip *chip,
> +				  unsigned offset,
> +				  unsigned gpio)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	unsigned func;
> +	int is_out;
> +	int drive;
> +	int pull;
> +	u32 ctl_reg;
> +
> +	const char *pulls[] = {

static const char * const ?

> +		"no pull",
> +		"pull down",
> +		"keeper",
> +		"pull up"
> +	};
> +
> +	g = &pctrl->soc->groups[offset];
> +	ctl_reg = readl(pctrl->regs + g->ctl_reg);
> +
> +	is_out = !!(ctl_reg & BIT(g->oe_bit));
> +	func = (ctl_reg >> g->mux_bit) & 7;
> +	drive = (ctl_reg >> g->drv_bit) & 7;
> +	pull = (ctl_reg >> g->pull_bit) & 3;
> +
> +	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
> +	seq_printf(s, " %dmA", msm_regval_to_drive[drive]);
> +	seq_printf(s, " %s", pulls[pull]);
> +}
> +
> +static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> +	unsigned gpio = chip->base;
> +	unsigned i;
> +
> +	for (i = 0; i < chip->ngpio; i++, gpio++) {
> +		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
> +		seq_printf(s, "\n");

seq_puts()?

> +	}
> +}
> +
> +#else
> +#define msm_gpio_dbg_show NULL
> +#endif
> +
> +static struct gpio_chip msm_gpio_template = {
> +	.direction_input  = msm_gpio_direction_input,
> +	.direction_output = msm_gpio_direction_output,
> +	.get              = msm_gpio_get,
> +	.set              = msm_gpio_set,
> +	.to_irq           = msm_gpio_to_irq,
> +	.request          = msm_gpio_request,
> +	.free             = msm_gpio_free,
> +	.dbg_show         = msm_gpio_dbg_show,
> +};
> +
> +/* For dual-edge interrupts in software, since some hardware has no
> + * such support:
> + *
> + * At appropriate moments, this function may be called to flip the polarity
> + * settings of both-edge irq lines to try and catch the next edge.
> + *
> + * The attempt is considered successful if:
> + * - the status bit goes high, indicating that an edge was caught, or
> + * - the input value of the gpio doesn't change during the attempt.
> + * If the value changes twice during the process, that would cause the first
> + * test to fail but would force the second, as two opposite
> + * transitions would cause a detection no matter the polarity setting.
> + *
> + * The do-loop tries to sledge-hammer closed the timing hole between
> + * the initial value-read and the polarity-write - if the line value changes
> + * during that window, an interrupt is lost, the new polarity setting is
> + * incorrect, and the first success test will fail, causing a retry.
> + *
> + * Algorithm comes from Google's msmgpio driver.
> + */
> +static void msm_gpio_update_dual_edge_pos(struct msm_pinctrl *pctrl,
> +					  const struct msm_pingroup *g,
> +					  struct irq_data *d)
> +{
> +	int loop_limit = 100;
> +	unsigned val, val2, intstat;
> +	unsigned pol;
> +
> +	do {
> +		val = readl(pctrl->regs + g->io_reg) & BIT(g->in_bit);
> +
> +		pol = readl(pctrl->regs + g->intr_cfg_reg);
> +		pol ^= BIT(g->intr_polarity_bit);
> +		writel(pol, pctrl->regs + g->intr_cfg_reg);
> +
> +		val2 = readl(pctrl->regs + g->io_reg) & BIT(g->in_bit);
> +		intstat = readl(pctrl->regs + g->intr_status_reg);
> +		if (intstat || (val == val2))
> +			return;
> +	} while (loop_limit-- > 0);
> +	dev_err(pctrl->dev, "dual-edge irq failed to stabilize, %#08x != %#08x\n",
> +		val, val2);
> +}
> +
> +static void msm_gpio_irq_mask(struct irq_data *d)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	u32 val;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return;
> +
> +	if (WARN_ON(d->hwirq >= pctrl->soc->ngroups))
> +		return;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->intr_cfg_reg);
> +	val &= ~BIT(g->intr_enable_bit);
> +	writel(val, pctrl->regs + g->intr_cfg_reg);
> +
> +	clear_bit(d->hwirq, pctrl->enabled_irqs);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static void msm_gpio_irq_unmask(struct irq_data *d)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	u32 val;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return;
> +
> +	if (WARN_ON(d->hwirq >= pctrl->soc->ngroups))
> +		return;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->intr_status_reg);
> +	val &= ~BIT(g->intr_status_bit);
> +	writel(val, pctrl->regs + g->intr_status_reg);
> +
> +	val = readl(pctrl->regs + g->intr_cfg_reg);
> +	val |= BIT(g->intr_enable_bit);
> +	writel(val, pctrl->regs + g->intr_cfg_reg);
> +
> +	set_bit(d->hwirq, pctrl->enabled_irqs);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static void msm_gpio_irq_ack(struct irq_data *d)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	u32 val;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return;
> +
> +	if (WARN_ON(d->hwirq >= pctrl->soc->ngroups))
> +		return;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->intr_status_reg);
> +	val &= ~BIT(g->intr_status_bit);
> +	writel(val, pctrl->regs + g->intr_status_reg);
> +
> +	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> +		msm_gpio_update_dual_edge_pos(pctrl, g, d);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +#define INTR_TARGET_PROC_APPS    4
> +
> +static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	u32 val;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return -EINVAL;

How is this possible?

> +
> +	if (WARN_ON(d->hwirq >= pctrl->soc->ngroups))
> +		return -EINVAL;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	/*
> +	 * For hw without possibility of detecting both edges
> +	 */
> +	if (g->intr_detection_width == 1 && type == IRQ_TYPE_EDGE_BOTH)
> +		set_bit(d->hwirq, pctrl->dual_edge_irqs);
> +	else
> +		clear_bit(d->hwirq, pctrl->dual_edge_irqs);
> +
> +	/* Route interrupts to application cpu */
> +	val = readl(pctrl->regs + g->intr_target_reg);
> +	val &= ~(7 << g->intr_target_bit);
> +	val |= INTR_TARGET_PROC_APPS << g->intr_target_bit;
> +	writel(val, pctrl->regs + g->intr_target_reg);
> +
> +	/* Update configuration for gpio.
> +	 * RAW_STATUS_EN is left on for all gpio irqs. Due to the
> +	 * internal circuitry of TLMM, toggling the RAW_STATUS
> +	 * could cause the INTR_STATUS to be set for EDGE interrupts.
> +	 */
> +	val = readl(pctrl->regs + g->intr_cfg_reg);
> +	val |= BIT(g->intr_raw_status_bit);
> +	if (g->intr_detection_width == 2) {
> +		val &= ~(3 << g->intr_detection_bit);
> +		val &= ~(1 << g->intr_polarity_bit);
> +		switch (type) {
> +		case IRQ_TYPE_EDGE_RISING:
> +			val |= 1 << g->intr_detection_bit;
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		case IRQ_TYPE_EDGE_FALLING:
> +			val |= 2 << g->intr_detection_bit;
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		case IRQ_TYPE_EDGE_BOTH:
> +			val |= 3 << g->intr_detection_bit;
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		case IRQ_TYPE_LEVEL_LOW:
> +			break;
> +		case IRQ_TYPE_LEVEL_HIGH:
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		}
> +	} else if (g->intr_detection_width == 1) {
> +		val &= ~(1 << g->intr_detection_bit);
> +		val &= ~(1 << g->intr_polarity_bit);
> +		switch (type) {
> +		case IRQ_TYPE_EDGE_RISING:
> +			val |= BIT(g->intr_detection_bit);
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		case IRQ_TYPE_EDGE_FALLING:
> +			val |= BIT(g->intr_detection_bit);
> +			break;
> +		case IRQ_TYPE_EDGE_BOTH:
> +			val |= BIT(g->intr_detection_bit);
> +			break;
> +		case IRQ_TYPE_LEVEL_LOW:
> +			break;
> +		case IRQ_TYPE_LEVEL_HIGH:
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		}
> +	} else {
> +		BUG();
> +	}

This would be better written as a collection of ifs so that
IRQ_TYPE_EDGE_BOTH doesn't have to be tested and we duplicate less code.

> +	writel(val, pctrl->regs + g->intr_cfg_reg);
> +
> +	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> +		msm_gpio_update_dual_edge_pos(pctrl, g, d);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> +		__irq_set_handler_locked(d->irq, handle_level_irq);
> +	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> +		__irq_set_handler_locked(d->irq, handle_edge_irq);
> +
> +	return 0;
> +}
> +
> +static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	unsigned ngpio;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return -EINVAL;
> +
> +	ngpio = pctrl->chip.ngpio;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	if (on) {
> +		if (bitmap_empty(pctrl->wake_irqs, ngpio))
> +			enable_irq_wake(pctrl->irq);
> +		set_bit(d->hwirq, pctrl->wake_irqs);
> +	} else {
> +		clear_bit(d->hwirq, pctrl->wake_irqs);
> +		if (bitmap_empty(pctrl->wake_irqs, ngpio))
> +			disable_irq_wake(pctrl->irq);
> +	}
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static unsigned int msm_gpio_irq_startup(struct irq_data *d)
> +{
> +	struct msm_pinctrl *pctrl = irq_data_get_irq_chip_data(d);
> +
> +	if (gpio_lock_as_irq(&pctrl->chip, d->hwirq)) {
> +		dev_err(pctrl->dev, "unable to lock HW IRQ %lu for IRQ\n",
> +			d->hwirq);
> +	}
> +	msm_gpio_irq_unmask(d);
> +	return 0;
> +}
> +
> +static void msm_gpio_irq_shutdown(struct irq_data *d)
> +{
> +	struct msm_pinctrl *pctrl = irq_data_get_irq_chip_data(d);
> +
> +	msm_gpio_irq_mask(d);
> +	gpio_unlock_as_irq(&pctrl->chip, d->hwirq);
> +}
> +
> +static struct irq_chip msm_gpio_irq_chip = {
> +	.name           = "msmgpio",
> +	.irq_mask       = msm_gpio_irq_mask,
> +	.irq_unmask     = msm_gpio_irq_unmask,
> +	.irq_ack        = msm_gpio_irq_ack,
> +	.irq_set_type   = msm_gpio_irq_set_type,
> +	.irq_set_wake   = msm_gpio_irq_set_wake,
> +	.irq_startup	= msm_gpio_irq_startup,
> +	.irq_shutdown	= msm_gpio_irq_shutdown,
> +};
> +
> +static void msm_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_get_chip(irq);
> +	int irq_pin;
> +	int handled = 0;
> +	u32 val;
> +	int i;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	/*
> +	 * Each pin have it's own IRQ status register, so use

s/have/has/

> +	 * enabled_irq bitmap to limit the number of reads.
> +	 */
> +	for_each_set_bit(i, pctrl->enabled_irqs, pctrl->chip.ngpio) {
> +		g = &pctrl->soc->groups[i];
> +		val = readl(pctrl->regs + g->intr_status_reg);
> +		if (val & BIT(g->intr_status_bit)) {
> +			irq_pin = irq_find_mapping(pctrl->domain, i);
> +			generic_handle_irq(irq_pin);
> +			handled++;
> +		}
> +	}
> +
> +	/* No interrutps where flagged */

s/where/were/
s/interrutps/interrupts/
> +	if (handled == 0)
> +		handle_bad_irq(irq, desc);
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int msm_gpio_init(struct msm_pinctrl *pctrl)
> +{
> +	struct gpio_chip *chip;
> +	int irq;
> +	int ret;
> +	int i;
> +	int r;
> +
> +	chip = &pctrl->chip;
> +	chip->base = 0;
> +	chip->ngpio = pctrl->soc->ngpios;
> +	chip->label = dev_name(pctrl->dev);
> +	chip->dev = pctrl->dev;
> +	chip->owner = THIS_MODULE;
> +	chip->of_node = pctrl->dev->of_node;
> +
> +	pctrl->enabled_irqs = devm_kzalloc(pctrl->dev,
> +					   sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> +					   GFP_KERNEL);

If you don't agree with how gpio-msm-v2.c does it, please use
devm_kcalloc().

> +	if (!pctrl->enabled_irqs) {
> +		dev_err(pctrl->dev, "Failed to allocate enabled_irqs bitmap\n");
> +		return -ENOMEM;
> +	}
> +
> +	pctrl->dual_edge_irqs = devm_kzalloc(pctrl->dev,
> +					     sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> +					     GFP_KERNEL);
> +	if (!pctrl->dual_edge_irqs) {
> +		dev_err(pctrl->dev, "Failed to allocate dual_edge_irqs bitmap\n");
> +		return -ENOMEM;
> +	}
> +
> +	pctrl->wake_irqs = devm_kzalloc(pctrl->dev,
> +					sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> +					GFP_KERNEL);
> +	if (!pctrl->wake_irqs) {
> +		dev_err(pctrl->dev, "Failed to allocate wake_irqs bitmap\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = gpiochip_add(&pctrl->chip);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed register gpiochip\n");
> +		return ret;
> +	}
> +
> +	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed to add pin range\n");
> +		return ret;
> +	}
> +
> +	pctrl->domain = irq_domain_add_linear(pctrl->dev->of_node, chip->ngpio,
> +					      &irq_domain_simple_ops, NULL);
> +	if (!pctrl->domain) {
> +		dev_err(pctrl->dev, "Failed to register irq domain\n");
> +		r = gpiochip_remove(&pctrl->chip);
> +		return -ENOSYS;
> +	}
> +
> +	for (i = 0; i < chip->ngpio; i++) {
> +		irq = irq_create_mapping(pctrl->domain, i);
> +		irq_set_chip_and_handler(irq, &msm_gpio_irq_chip, handle_edge_irq);
> +		irq_set_chip_data(irq, pctrl);
> +	}
> +
> +	irq_set_handler_data(pctrl->irq, pctrl);
> +	irq_set_chained_handler(pctrl->irq, msm_gpio_irq_handler);
> +
> +	return 0;
> +}
> +
> +int msm_pinctrl_probe(struct platform_device *pdev,
> +		      const struct msm_pinctrl_soc_data *soc_data)
> +{
> +	struct msm_pinctrl *pctrl;
> +	struct resource *res;
> +	int ret;
> +
> +	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
> +	if (!pctrl) {
> +		dev_err(&pdev->dev, "Can't allocate msm_pinctrl\n");
> +		return -ENOMEM;
> +	}
> +	pctrl->dev = &pdev->dev;
> +	pctrl->soc = soc_data;
> +	pctrl->chip = msm_gpio_template;
> +
> +	spin_lock_init(&pctrl->lock);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pctrl->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pctrl->regs))
> +		return PTR_ERR(pctrl->regs);
> +
> +	pctrl->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);

Why not use platform_get_irq()?

> +	if (pctrl->irq < 0) {
> +		dev_err(&pdev->dev, "No interrupt defined for msmgpio\n");
> +		return pctrl->irq;
> +	}
> +
> +	msm_pinctrl_desc.name = dev_name(&pdev->dev);
> +	msm_pinctrl_desc.pins = pctrl->soc->pins;
> +	msm_pinctrl_desc.npins = pctrl->soc->npins;
> +	pctrl->pctrl = pinctrl_register(&msm_pinctrl_desc, &pdev->dev, pctrl);
> +	if (!pctrl->pctrl) {
> +		dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = msm_gpio_init(pctrl);
> +	if (ret) {
> +		pinctrl_unregister(pctrl->pctrl);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pctrl);
> +
> +	dev_dbg(&pdev->dev, "Probed Qualcomm pinctrl driver\n");
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(msm_pinctrl_probe);
> +
> +int msm_pinctrl_remove(struct platform_device *pdev)
> +{
> +	struct msm_pinctrl *pctrl = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	irq_set_chained_handler(pctrl->irq, NULL);
> +	irq_domain_remove(pctrl->domain);
> +	ret = gpiochip_remove(&pctrl->chip);
> +	pinctrl_unregister(pctrl->pctrl);
> +
> +	return 0;

return ret?

> +}
> +EXPORT_SYMBOL(msm_pinctrl_remove);
> +
> diff --git a/drivers/pinctrl/pinctrl-msm.h b/drivers/pinctrl/pinctrl-msm.h
> new file mode 100644
> index 0000000..13b7463
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-msm.h
> @@ -0,0 +1,123 @@
> +/*
> + * Copyright (c) 2013, Sony Mobile Communications AB.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#ifndef __PINCTRL_MSM_H__
> +#define __PINCTRL_MSM_H__
> +
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/machine.h>

Are any of these includes actually necessary? Can't we just forward
declare struct pinctrl_pin_desc?

> +
> +struct msm_pingroup {
> +	const char *name;
> +	const unsigned *pins;
> +	unsigned npins;
> +
> +	unsigned funcs[8];
> +
> +	s16 ctl_reg;
> +	s16 io_reg;
> +	s16 intr_cfg_reg;
> +	s16 intr_status_reg;
> +	s16 intr_target_reg;

Why are these signed? Because the macros assign -1 to them? Perhaps make
them unsigned and make the macro assign ~0U?

> +
> +	unsigned mux_bit:5;
> +
> +	unsigned pull_bit:5;
> +	unsigned drv_bit:5;
> +
> +	unsigned oe_bit:5;
> +	unsigned in_bit:5;
> +	unsigned out_bit:5;
> +
> +	unsigned intr_enable_bit:5;
> +	unsigned intr_status_bit:5;
> +
> +	unsigned intr_target_bit:5;
> +	unsigned intr_raw_status_bit:5;
> +	unsigned intr_polarity_bit:5;
> +	unsigned intr_detection_bit:5;
> +	unsigned intr_detection_width:5;
> +};
> +
> +/**
> + * struct msm_pinctrl_soc_data - Qualcomm pin controller driver configuration
> + * @pins:       An array describing all pins the pin controller affects.
> + * @npins:      The number of entries in @pins.
> + * @functions:  An array describing all mux functions the SoC supports.
> + * @nfunctions: The number of entries in @functions.
> + * @groups:     An array describing all pin groups the pin SoC supports.
> + * @ngroups:    The numbmer of entries in @groups.
> + * @ngpio:      The number of pingroups the driver should expose as GPIOs.
> + */
> +struct msm_pinctrl_soc_data {
> +	const struct pinctrl_pin_desc *pins;
> +	unsigned npins;
> +	const struct msm_function *functions;
> +	unsigned nfunctions;
> +	const struct msm_pingroup *groups;
> +	unsigned ngroups;
> +	unsigned ngpios;
> +};
> +
> +int msm_pinctrl_probe(struct platform_device *pdev,
> +		      const struct msm_pinctrl_soc_data *soc_data);
> +int msm_pinctrl_remove(struct platform_device *pdev);
> +
> +#endif
> +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation