[PATCH v2 0/4] New Qualcomm PMIC pin controller drivers

[PATCH v2 0/4] New Qualcomm PMIC pin controller drivers

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

Hi, 

This is second version of the patches posted earlier[1].

Patches could be applied on top of the Bjorn Andersson
qcom,pm8xxx-gpio driver, which could be found here[2].

Fist patch modify Bjorn's driver and bindings to make room
for newer Qualcomm PMIC chips. It is not completely ready.

Changes:
- Remove registers values cache.
- Merge pm8941 and pm8841 into qpnp-pinctrl driver.
- Rebase on top of the "Qualcomm pm8xxx gpio driver" to reuse
  "qcom,pm8xxx-gpio" DT document.
- Split compatibles string to MPP's and GPIO's to use
  similar naming convention like "Qualcomm pm8xxx gpio driver"[2]
- Add support for pma8084 chip

Short description:

Patches adds pin control drivers for Multi-purpose pin (MPP) and
General-purpose pin (GPIO) controllers found in Qualcomm SPMI
based PMIC chips.

MPP's are enhanced GPIO's with analog circuits, which support 
following functions in addition to digital input/output: analog
input/output and current sinks. 

[1] http://lwn.net/Articles/604637/
[2] https://lkml.org/lkml/2014/7/7/945

Ivan T. Ivanov (4):
  pinctrl: Update Qualcomm PMXXX GPIO parameters definitions
  pinctrl: qpnp: Qualcomm PMIC pin controller driver
  pinctrl: qcom: Add documentation for pinctrl-qpnp driver bindings
  ARM: dts: qcom: Add PM8941 and PM8841 pinctrl nodes

 .../bindings/pinctrl/qcom,pm8xxx-gpio.txt          |   97 +-
 .../bindings/pinctrl/qcom,pm8xxx-mpp.txt           |  199 +++
 arch/arm/boot/dts/qcom-msm8974.dtsi                |   61 +
 drivers/pinctrl/Kconfig                            |   12 +
 drivers/pinctrl/Makefile                           |    1 +
 drivers/pinctrl/pinctrl-pm8xxx-gpio.c              |   34 +-
 drivers/pinctrl/pinctrl-qpnp.c                     | 1565 ++++++++++++++++++++
 include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h     |   33 +-
 include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h      |   34 +
 9 files changed, 1953 insertions(+), 83 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-mpp.txt
 create mode 100644 drivers/pinctrl/pinctrl-qpnp.c
 create mode 100644 include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h

-- 
1.8.3.2

[PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

This is the pinctrl, pinmux, pinconf and gpiolib driver for the
Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/pinctrl/Kconfig                       |   12 +
 drivers/pinctrl/Makefile                      |    1 +
 drivers/pinctrl/pinctrl-qpnp.c                | 1565 +++++++++++++++++++++++++
 include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h |   34 +
 4 files changed, 1612 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-qpnp.c
 create mode 100644 include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index c173db6..72083e1 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -394,6 +394,18 @@ config PINCTRL_PALMAS
 	  open drain configuration for the Palmas series devices like
 	  TPS65913, TPS80036 etc.
 
+config PINCTRL_QPNP
+	tristate "Qualcomm QPNP PMIC pin controller driver"
+	depends on OF
+	select PINMUX
+	select PINCONF
+	select GENERIC_PINCONF
+	select GPIOLIB
+	help
+	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
+	  Qualcomm GPIO and MPP blocks found in the Qualcomm PMIC's chips,
+	  which are using SPMI for communication with SoC.
+
 config PINCTRL_S3C24XX
 	bool "Samsung S3C24XX SoC pinctrl driver"
 	depends on ARCH_S3C24XX
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 806f6ad..4e89d2a 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_PINCTRL_DB8500)	+= pinctrl-nomadik-db8500.o
 obj-$(CONFIG_PINCTRL_DB8540)	+= pinctrl-nomadik-db8540.o
 obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
 obj-$(CONFIG_PINCTRL_PM8XXX_GPIO)	+= pinctrl-pm8xxx-gpio.o
+obj-$(CONFIG_PINCTRL_QPNP)	+= pinctrl-qpnp.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
diff --git a/drivers/pinctrl/pinctrl-qpnp.c b/drivers/pinctrl/pinctrl-qpnp.c
new file mode 100644
index 0000000..aedc72e
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-qpnp.c
@@ -0,0 +1,1565 @@
+/* Copyright (c) 2012-2014, 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/export.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <dt-bindings/pinctrl/qcom,pm8xxx-mpp.h>
+#include <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+
+/*
+ * Mode select - indicates whether the pin should be input, output, or both
+ * for GPIOs. MPP pins also support bidirectional, analog input, analog output
+ * and current sink.
+ */
+#define QPNP_PIN_MODE_DIG_IN			0
+#define QPNP_PIN_MODE_DIG_OUT			1
+#define QPNP_PIN_MODE_DIG_IN_OUT		2
+#define QPNP_PIN_MODE_BIDIR			3
+#define QPNP_PIN_MODE_AIN			4
+#define QPNP_PIN_MODE_AOUT			5
+#define QPNP_PIN_MODE_SINK			6
+
+/*
+ * Voltage select (GPIO, MPP) - specifies the voltage level when the output
+ * is set to 1. For an input GPIO specifies the voltage level at which
+ * the input is interpreted as a logical 1
+ * To be used with "power-source = <>"
+ */
+#define QPNP_PIN_VIN_4CH_INVALID		5
+#define QPNP_PIN_VIN_8CH_INVALID		8
+
+/*
+ * Analog Output - Set the analog output reference.
+ * See PM8XXX_MPP_AOUT_XXX. To be used with "qcom,aout = <>"
+ */
+#define QPNP_MPP_AOUT_INVALID			8
+
+/*
+ * Analog Input - Set the source for analog input.
+ * See PM8XXX_MPP_AIN_XXX. To be used with "qcom,ain = <>"
+ */
+#define QPNP_MPP_AIN_INVALID			8
+
+/*
+ * Output type - indicates pin should be configured as CMOS or
+ * open drain.
+ */
+#define QPNP_GPIO_OUT_BUF_CMOS			0
+#define QPNP_GPIO_OUT_BUF_OPEN_DRAIN_NMOS	1
+#define QPNP_GPIO_OUT_BUF_OPEN_DRAIN_PMOS	2
+
+/*
+ * Pull Up Values - it indicates whether a pull up or pull down
+ * should be applied. If a pull-up is required the current strength needs
+ * to be specified. Current values of 30uA, 1.5uA, 31.5uA, 1.5uA with 30uA
+ * boost are supported.
+ * Note that the hardware ignores this configuration if the GPIO is not set
+ * to input or output open-drain mode.
+ */
+#define QPNP_GPIO_PULL_UP_30			0
+#define QPNP_GPIO_PULL_UP_1P5			1
+#define QPNP_GPIO_PULL_UP_31P5			2
+#define QPNP_GPIO_PULL_UP_1P5_30		3
+#define QPNP_GPIO_PULL_DN			4
+#define QPNP_GPIO_PULL_NO			5
+
+/*
+ * Pull Up Values - it indicates whether a pull-up should be
+ * applied for bidirectional mode only. The hardware ignores the
+ * configuration when operating in other modes.
+ */
+#define QPNP_MPP_PULL_UP_0P6KOHM		0
+#define QPNP_MPP_PULL_UP_10KOHM			1
+#define QPNP_MPP_PULL_UP_30KOHM			2
+#define QPNP_MPP_PULL_UP_OPEN			3
+
+/* Out Strength (GPIO) - the amount of current supplied for an output GPIO */
+#define QPNP_GPIO_STRENGTH_LOW			1
+#define QPNP_GPIO_STRENGTH_MED			2
+#define QPNP_GPIO_STRENGTH_HIGH			3
+
+/*
+ * Master enable (GPIO, MPP) - Enable features within the pin block based on
+ * configurations. QPNP_PIN_MASTER_DISABLE = Completely disable the pin
+ * lock and let the pin float with high impedance regardless of other settings.
+ */
+#define QPNP_PIN_MASTER_DISABLE                 0
+#define QPNP_PIN_MASTER_ENABLE			1
+
+/* Current Sink. Set the the amount of current to sync in mA. */
+#define QPNP_MPP_CS_OUT_5MA			0
+#define QPNP_MPP_CS_OUT_10MA			1
+#define QPNP_MPP_CS_OUT_15MA			2
+#define QPNP_MPP_CS_OUT_20MA			3
+#define QPNP_MPP_CS_OUT_25MA			4
+#define QPNP_MPP_CS_OUT_30MA			5
+#define QPNP_MPP_CS_OUT_35MA			6
+#define QPNP_MPP_CS_OUT_40MA			7
+
+/* revision registers base address offsets */
+#define QPNP_REG_DIG_MINOR_REV			0x0
+#define QPNP_REG_DIG_MAJOR_REV			0x1
+#define QPNP_REG_ANA_MINOR_REV			0x2
+
+/* type registers base address offsets */
+#define QPNP_REG_TYPE				0x4
+#define QPNP_REG_SUBTYPE			0x5
+
+/* GPIO peripheral type and subtype values */
+#define QPNP_GPIO_TYPE				0x10
+#define QPNP_GPIO_SUBTYPE_GPIO_4CH		0x1
+#define QPNP_GPIO_SUBTYPE_GPIOC_4CH		0x5
+#define QPNP_GPIO_SUBTYPE_GPIO_8CH		0x9
+#define QPNP_GPIO_SUBTYPE_GPIOC_8CH		0xd
+
+/* mpp peripheral type and subtype values */
+#define QPNP_MPP_TYPE				0x11
+#define QPNP_MPP_SUBTYPE_4CH_NO_ANA_OUT		0x3
+#define QPNP_MPP_SUBTYPE_ULT_4CH_NO_ANA_OUT	0x4
+#define QPNP_MPP_SUBTYPE_4CH_NO_SINK		0x5
+#define QPNP_MPP_SUBTYPE_ULT_4CH_NO_SINK	0x6
+#define QPNP_MPP_SUBTYPE_4CH_FULL_FUNC		0x7
+#define QPNP_MPP_SUBTYPE_8CH_FULL_FUNC		0xf
+
+#define QPNP_REG_STATUS1			0x8
+#define QPNP_REG_STATUS1_VAL_MASK		0x1
+#define QPNP_REG_STATUS1_GPIO_EN_REV0_MASK	0x2
+#define QPNP_REG_STATUS1_GPIO_EN_MASK		0x80
+#define QPNP_REG_STATUS1_MPP_EN_MASK		0x80
+
+/* control register base address offsets */
+#define QPNP_REG_MODE_CTL			0x40
+#define QPNP_REG_DIG_VIN_CTL			0x41
+#define QPNP_REG_DIG_PULL_CTL			0x42
+#define QPNP_REG_DIG_IN_CTL			0x43
+#define QPNP_REG_DIG_OUT_CTL			0x45
+#define QPNP_REG_EN_CTL				0x46
+#define QPNP_REG_AOUT_CTL			0x4b
+#define QPNP_REG_AIN_CTL			0x4a
+#define QPNP_REG_SINK_CTL			0x4c
+
+/* QPNP_REG_MODE_CTL */
+#define QPNP_REG_OUT_SRC_SEL_SHIFT		0
+#define QPNP_REG_OUT_SRC_SEL_MASK		0xf
+#define QPNP_REG_MODE_SEL_SHIFT			4
+#define QPNP_REG_MODE_SEL_MASK			0x70
+
+/* QPNP_REG_DIG_VIN_CTL */
+#define QPNP_REG_VIN_SHIFT			0
+#define QPNP_REG_VIN_MASK			0x7
+
+/* QPNP_REG_DIG_PULL_CTL */
+#define QPNP_REG_PULL_SHIFT			0
+#define QPNP_REG_PULL_MASK			0x7
+
+/* QPNP_REG_DIG_OUT_CTL */
+#define QPNP_REG_OUT_STRENGTH_SHIFT		0
+#define QPNP_REG_OUT_STRENGTH_MASK		0x3
+#define QPNP_REG_OUT_TYPE_SHIFT			4
+#define QPNP_REG_OUT_TYPE_MASK			0x30
+
+/* QPNP_REG_EN_CTL */
+#define QPNP_REG_MASTER_EN_SHIFT		7
+#define QPNP_REG_MASTER_EN_MASK			0x80
+
+/* QPNP_REG_AOUT_CTL */
+#define QPNP_REG_AOUT_REF_SHIFT			0
+#define QPNP_REG_AOUT_REF_MASK			0x7
+
+/* QPNP_REG_AIN_CTL */
+#define QPNP_REG_AIN_ROUTE_SHIFT		0
+#define QPNP_REG_AIN_ROUTE_MASK			0x7
+
+/* QPNP_REG_SINK_CTL */
+#define QPNP_REG_CS_OUT_SHIFT			0
+#define QPNP_REG_CS_OUT_MASK			0x7
+
+/* Qualcomm specific pin configurations */
+#define QPNP_PINCONF_PARAM_PULL_UP		(PIN_CONFIG_END + 1)
+#define QPNP_PINCONF_PARAM_STRENGTH		(PIN_CONFIG_END + 2)
+#define QPNP_PINCONF_PARAM_AIN_CTRL		(PIN_CONFIG_END + 3)
+#define QPNP_PINCONF_PARAM_AOUT_CTRL		(PIN_CONFIG_END + 4)
+
+enum qpnp_functions {
+	QPNP_FUNC_GPIO,
+	QPNP_FUNC_AIN,
+	QPNP_FUNC_AOUT,
+	QPNP_FUNC_CS,
+	QPNP_FUNC_CNT,
+};
+
+struct qpnp_chipinfo {
+	unsigned npads;
+	unsigned base;
+};
+
+struct qpnp_padinfo {
+	u16 offset;		/* address offset in SPMI device */
+	int irq;
+	char name[8];
+	enum qpnp_functions funcs[QPNP_FUNC_CNT];
+	unsigned int type;	/* peripheral hardware type */
+	unsigned int subtype;	/* peripheral hardware subtype */
+	unsigned int major;	/* digital major version */
+};
+
+#define QPNP_REG_ADDR(pad, reg) ((pad)->offset + reg)
+#define QPNP_GET(buff, shift, mask) ((buff & mask) >> shift)
+
+struct qpnp_pingroup {
+	const char **names;
+	unsigned npins;
+};
+
+struct qpnp_pinctrl {
+	struct device *dev;
+	struct regmap *map;
+	struct pinctrl_dev *ctrl;
+	struct pinctrl_desc desc;
+	struct pinctrl_gpio_range range;
+	struct gpio_chip chip;
+
+	struct qpnp_padinfo *pads;
+	struct qpnp_pingroup groups[QPNP_FUNC_CNT];
+};
+
+static inline struct qpnp_pinctrl *to_qpnp_pinctrl(struct gpio_chip *chip)
+{
+	return container_of(chip, struct qpnp_pinctrl, chip);
+};
+
+struct qpnp_pinbindings {
+	const char *property;
+	unsigned param;
+	u32 default_value;
+};
+
+struct qpnp_pinattrib {
+	unsigned addr;
+	unsigned shift;
+	unsigned mask;
+	unsigned val;
+};
+
+static struct qpnp_pinbindings qpnp_pinbindings[] = {
+	/* PM8XXX_GPIO_PULL_UP_30...  */
+	{"qcom,pull-up",	QPNP_PINCONF_PARAM_PULL_UP, 0},
+	/* PM8XXX_GPIO_STRENGTH_NO... */
+	{"qcom,strength",	QPNP_PINCONF_PARAM_STRENGTH, 0},
+	/* PM8XXX_MPP_AIN_CH5 ... */
+	{"qcom,ain",		QPNP_PINCONF_PARAM_AIN_CTRL, 0},
+	/* PM8XXX_MPP_AOUT_1V25 ... */
+	{"qcom,aout",		QPNP_PINCONF_PARAM_AOUT_CTRL, 0},
+};
+
+static const char *const qpnp_functions_names[] = {
+	[QPNP_FUNC_GPIO] = "gpio",
+	[QPNP_FUNC_AIN]	 = "ain",
+	[QPNP_FUNC_AOUT] = "aout",
+	[QPNP_FUNC_CS]	 = "cs"
+};
+
+static inline struct qpnp_padinfo *qpnp_get_desc(struct qpnp_pinctrl *qctrl,
+						 unsigned pin)
+{
+	if (pin >= qctrl->desc.npins) {
+		dev_warn(qctrl->dev, "invalid pin number %d", pin);
+		return NULL;
+	}
+
+	return &qctrl->pads[pin];
+}
+
+static inline void QPNP_SET(unsigned int *buff, int shift, int mask, int value)
+{
+	*buff &= ~mask;
+	*buff |= (value << shift) & mask;
+}
+
+static int qpnp_control_init(struct qpnp_pinctrl *qctrl,
+			  struct qpnp_padinfo *pad)
+{
+	/* Assume PIN support all functions */
+	pad->funcs[QPNP_FUNC_GPIO] = QPNP_FUNC_GPIO;
+	pad->funcs[QPNP_FUNC_AIN] = QPNP_FUNC_AIN;
+	pad->funcs[QPNP_FUNC_AOUT] = QPNP_FUNC_AOUT;
+	pad->funcs[QPNP_FUNC_CS] = QPNP_FUNC_CS;
+
+	if (pad->type == QPNP_GPIO_TYPE) {
+		switch (pad->subtype) {
+		case QPNP_GPIO_SUBTYPE_GPIO_4CH:
+		case QPNP_GPIO_SUBTYPE_GPIOC_4CH:
+		case QPNP_GPIO_SUBTYPE_GPIO_8CH:
+		case QPNP_GPIO_SUBTYPE_GPIOC_8CH:
+
+			/* only GPIO is supported*/
+			pad->funcs[QPNP_FUNC_AIN] = QPNP_FUNC_GPIO;
+			pad->funcs[QPNP_FUNC_AOUT] = QPNP_FUNC_GPIO;
+			pad->funcs[QPNP_FUNC_CS] = QPNP_FUNC_GPIO;
+
+			qctrl->groups[QPNP_FUNC_GPIO].npins++;
+			break;
+		default:
+			dev_err(qctrl->dev, "invalid GPIO subtype 0x%x\n",
+				pad->subtype);
+			return -EINVAL;
+		}
+
+	} else if (pad->type == QPNP_MPP_TYPE) {
+		switch (pad->subtype) {
+		case QPNP_MPP_SUBTYPE_4CH_NO_SINK:
+		case QPNP_MPP_SUBTYPE_ULT_4CH_NO_SINK:
+
+			/* Current sink not supported*/
+			pad->funcs[QPNP_FUNC_CS] = QPNP_FUNC_GPIO;
+
+			qctrl->groups[QPNP_FUNC_GPIO].npins++;
+			qctrl->groups[QPNP_FUNC_AIN].npins++;
+			qctrl->groups[QPNP_FUNC_AOUT].npins++;
+			break;
+		case QPNP_MPP_SUBTYPE_4CH_NO_ANA_OUT:
+		case QPNP_MPP_SUBTYPE_ULT_4CH_NO_ANA_OUT:
+
+			/* Analog output not supported*/
+			pad->funcs[QPNP_FUNC_AOUT] = QPNP_FUNC_GPIO;
+
+			qctrl->groups[QPNP_FUNC_GPIO].npins++;
+			qctrl->groups[QPNP_FUNC_AIN].npins++;
+			qctrl->groups[QPNP_FUNC_CS].npins++;
+			break;
+		case QPNP_MPP_SUBTYPE_4CH_FULL_FUNC:
+		case QPNP_MPP_SUBTYPE_8CH_FULL_FUNC:
+
+			qctrl->groups[QPNP_FUNC_GPIO].npins++;
+			qctrl->groups[QPNP_FUNC_AIN].npins++;
+			qctrl->groups[QPNP_FUNC_AOUT].npins++;
+			qctrl->groups[QPNP_FUNC_CS].npins++;
+			break;
+		default:
+			dev_err(qctrl->dev, "invalid MPP subtype 0x%x\n",
+				pad->subtype);
+			return -EINVAL;
+		}
+	} else {
+		dev_err(qctrl->dev, "invalid type 0x%x\n", pad->type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qpnp_conv_to_pin(struct qpnp_pinctrl *qctrl,
+			   struct qpnp_padinfo *pad, unsigned param,
+			   unsigned val)
+{
+	struct qpnp_pinattrib attr[3];
+	unsigned int type, subtype;
+	int nattrs = 1, idx, ret;
+
+	type = pad->type;
+	subtype = pad->subtype;
+
+	switch (param) {
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		if (type != QPNP_GPIO_TYPE)
+			return -ENXIO;
+		attr[0].addr  = QPNP_REG_DIG_OUT_CTL;
+		attr[0].shift = QPNP_REG_OUT_TYPE_SHIFT;
+		attr[0].mask  = QPNP_REG_OUT_TYPE_MASK;
+		attr[0].val   = QPNP_GPIO_OUT_BUF_CMOS;
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		if (type != QPNP_GPIO_TYPE)
+			return -ENXIO;
+		if (subtype == QPNP_GPIO_SUBTYPE_GPIOC_4CH ||
+		    subtype == QPNP_GPIO_SUBTYPE_GPIOC_8CH)
+			return -EINVAL;
+		attr[0].addr  = QPNP_REG_DIG_OUT_CTL;
+		attr[0].shift = QPNP_REG_OUT_TYPE_SHIFT;
+		attr[0].mask  = QPNP_REG_OUT_TYPE_MASK;
+		attr[0].val   = QPNP_GPIO_OUT_BUF_OPEN_DRAIN_NMOS;
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+		if (type != QPNP_GPIO_TYPE)
+			return -ENXIO;
+		if (subtype == QPNP_GPIO_SUBTYPE_GPIOC_4CH ||
+		    subtype == QPNP_GPIO_SUBTYPE_GPIOC_8CH)
+			return -EINVAL;
+		attr[0].addr  = QPNP_REG_DIG_OUT_CTL;
+		attr[0].shift = QPNP_REG_OUT_TYPE_SHIFT;
+		attr[0].mask  = QPNP_REG_OUT_TYPE_MASK;
+		attr[0].val   = QPNP_GPIO_OUT_BUF_OPEN_DRAIN_PMOS;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		attr[0].addr  = QPNP_REG_DIG_PULL_CTL;
+		attr[0].shift = QPNP_REG_PULL_SHIFT;
+		attr[0].mask  = QPNP_REG_PULL_MASK;
+		if (type == QPNP_GPIO_TYPE)
+			attr[0].val = QPNP_GPIO_PULL_NO;
+		else
+			attr[0].val = QPNP_MPP_PULL_UP_OPEN;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (type != QPNP_MPP_TYPE)
+			return -EINVAL;
+		switch (val) {
+		case 0:
+			val = QPNP_MPP_PULL_UP_OPEN;
+			break;
+		case 600:
+			val = QPNP_MPP_PULL_UP_0P6KOHM;
+			break;
+		case 10000:
+			val = QPNP_MPP_PULL_UP_10KOHM;
+			break;
+		case 30000:
+			val = QPNP_MPP_PULL_UP_30KOHM;
+			break;
+		default:
+			return -EINVAL;
+			break;
+		}
+		attr[0].addr  = QPNP_REG_DIG_PULL_CTL;
+		attr[0].shift = QPNP_REG_PULL_SHIFT;
+		attr[0].mask  = QPNP_REG_PULL_MASK;
+		attr[0].val   = val;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (type != QPNP_GPIO_TYPE)
+			return -EINVAL;
+		attr[0].addr  = QPNP_REG_DIG_PULL_CTL;
+		attr[0].shift = QPNP_REG_PULL_SHIFT;
+		attr[0].mask  = QPNP_REG_PULL_MASK;
+		if (val)
+			attr[0].val = QPNP_GPIO_PULL_DN;
+		else
+			attr[0].val = QPNP_GPIO_PULL_NO;
+		break;
+	case PIN_CONFIG_POWER_SOURCE:
+		if (val >= QPNP_PIN_VIN_8CH_INVALID)
+			return -EINVAL;
+		if (val >= QPNP_PIN_VIN_4CH_INVALID) {
+			if (type == QPNP_GPIO_TYPE &&
+			   (subtype == QPNP_GPIO_SUBTYPE_GPIO_4CH ||
+			    subtype == QPNP_GPIO_SUBTYPE_GPIOC_4CH))
+				return -EINVAL;
+			if (type == QPNP_MPP_TYPE &&
+			   (subtype == QPNP_MPP_SUBTYPE_4CH_NO_ANA_OUT ||
+			    subtype == QPNP_MPP_SUBTYPE_4CH_NO_SINK ||
+			    subtype == QPNP_MPP_SUBTYPE_4CH_FULL_FUNC ||
+			    subtype == QPNP_MPP_SUBTYPE_ULT_4CH_NO_ANA_OUT ||
+			    subtype == QPNP_MPP_SUBTYPE_ULT_4CH_NO_SINK))
+				return -EINVAL;
+		}
+		attr[0].addr  = QPNP_REG_DIG_VIN_CTL;
+		attr[0].shift = QPNP_REG_VIN_SHIFT;
+		attr[0].mask  = QPNP_REG_VIN_MASK;
+		attr[0].val   = val;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		if (type != QPNP_MPP_TYPE)
+			return -EINVAL;
+		if (subtype == QPNP_MPP_SUBTYPE_4CH_NO_SINK ||
+		    subtype == QPNP_MPP_SUBTYPE_ULT_4CH_NO_SINK)
+			return -ENXIO;
+		if (val > 50)	/* mA */
+			return -EINVAL;
+		attr[0].addr  = QPNP_REG_SINK_CTL;
+		attr[0].shift = QPNP_REG_CS_OUT_SHIFT;
+		attr[0].mask  = QPNP_REG_CS_OUT_MASK;
+		attr[0].val   = (val / 5) - 1;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+		nattrs = 2;
+		attr[0].addr  = QPNP_REG_MODE_CTL;
+		attr[0].shift = QPNP_REG_MODE_SEL_SHIFT;
+		attr[0].mask  = QPNP_REG_MODE_SEL_MASK;
+		attr[0].val   = QPNP_PIN_MODE_DIG_IN;
+		attr[1].addr  = QPNP_REG_EN_CTL;
+		attr[1].shift = QPNP_REG_MASTER_EN_SHIFT;
+		attr[1].mask  = QPNP_REG_MASTER_EN_MASK;
+		attr[1].val   = 1;
+		if (val)
+			break;
+	/* Fallthrough */
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		attr[1].addr  = QPNP_REG_EN_CTL;
+		attr[1].shift = QPNP_REG_MASTER_EN_SHIFT;
+		attr[1].mask  = QPNP_REG_MASTER_EN_MASK;
+		attr[1].val   = 0;
+		break;
+	case PIN_CONFIG_OUTPUT:
+		nattrs = 3;
+		attr[0].addr  = QPNP_REG_MODE_CTL;
+		attr[0].shift = QPNP_REG_OUT_SRC_SEL_SHIFT;
+		attr[0].mask  = QPNP_REG_OUT_SRC_SEL_MASK;
+		attr[0].val   = !!val;
+		attr[1].addr  = QPNP_REG_MODE_CTL;
+		attr[1].shift = QPNP_REG_MODE_SEL_SHIFT;
+		attr[1].mask  = QPNP_REG_MODE_SEL_MASK;
+		attr[1].val   = QPNP_PIN_MODE_DIG_OUT;
+		attr[2].addr  = QPNP_REG_EN_CTL;
+		attr[2].shift = QPNP_REG_MASTER_EN_SHIFT;
+		attr[2].mask  = QPNP_REG_MASTER_EN_MASK;
+		attr[2].val   = 1;
+		break;
+	case QPNP_PINCONF_PARAM_PULL_UP:
+		if (type != QPNP_GPIO_TYPE)
+			return -EINVAL;
+		switch (val) {
+		default:
+			return -EINVAL;
+			break;
+		case 0:
+			val = QPNP_GPIO_PULL_NO;
+			break;
+		case PM8XXX_GPIO_PULL_UP_30:
+			val = QPNP_GPIO_PULL_UP_30;
+			break;
+		case PM8XXX_GPIO_PULL_UP_1P5:
+			val = QPNP_GPIO_PULL_UP_1P5;
+			break;
+		case PM8XXX_GPIO_PULL_UP_31P5:
+			val = QPNP_GPIO_PULL_UP_31P5;
+			break;
+		case PM8XXX_GPIO_PULL_UP_1P5_30:
+			val = QPNP_GPIO_PULL_UP_1P5_30;
+			break;
+		}
+		attr[0].addr  = QPNP_REG_DIG_PULL_CTL;
+		attr[0].shift = QPNP_REG_PULL_SHIFT;
+		attr[0].mask  = QPNP_REG_PULL_MASK;
+		attr[0].val   = val;
+		break;
+	case QPNP_PINCONF_PARAM_STRENGTH:
+		if (type != QPNP_GPIO_TYPE)
+			return -EINVAL;
+		switch (val) {
+		default:
+		case PM8XXX_GPIO_STRENGTH_NO:
+			return -EINVAL;
+			break;
+		case PM8XXX_GPIO_STRENGTH_LOW:
+			attr[0].val = QPNP_GPIO_STRENGTH_LOW;
+			break;
+		case PM8XXX_GPIO_STRENGTH_MED:
+			attr[0].val = QPNP_GPIO_STRENGTH_MED;
+			break;
+		case PM8XXX_GPIO_STRENGTH_HIGH:
+			attr[0].val = QPNP_GPIO_STRENGTH_HIGH;
+			break;
+		}
+		attr[0].addr  = QPNP_REG_DIG_OUT_CTL;
+		attr[0].shift = QPNP_REG_OUT_STRENGTH_SHIFT;
+		attr[0].mask  = QPNP_REG_OUT_STRENGTH_MASK;
+		break;
+	case QPNP_PINCONF_PARAM_AOUT_CTRL:
+		if (type != QPNP_MPP_TYPE)
+			return -ENXIO;
+		if (val >= QPNP_MPP_AOUT_INVALID)
+			return -EINVAL;
+		if (subtype == QPNP_MPP_SUBTYPE_4CH_NO_ANA_OUT ||
+		    subtype == QPNP_MPP_SUBTYPE_ULT_4CH_NO_ANA_OUT)
+			return -ENXIO;
+		attr[0].addr  = QPNP_REG_AOUT_CTL;
+		attr[0].shift = QPNP_REG_AOUT_REF_SHIFT;
+		attr[0].mask  = QPNP_REG_AOUT_REF_MASK;
+		attr[0].val   = val;
+		break;
+	case QPNP_PINCONF_PARAM_AIN_CTRL:
+		if (type != QPNP_MPP_TYPE)
+			return -ENXIO;
+		if (val >= QPNP_MPP_AIN_INVALID)
+			return -EINVAL;
+		attr[0].addr  = QPNP_REG_AIN_CTL;
+		attr[0].shift = QPNP_REG_AIN_ROUTE_SHIFT;
+		attr[0].mask  = QPNP_REG_AIN_ROUTE_MASK;
+		attr[0].val   = val;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	for (idx = 0; idx < nattrs; idx++) {
+		ret = regmap_update_bits(qctrl->map, attr[idx].addr,
+					 attr[idx].mask,
+					 attr[idx].val << attr[idx].shift);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+
+static int qpnp_conv_from_pin(struct qpnp_pinctrl *qctrl,
+			     struct qpnp_padinfo *pad,
+			     unsigned param, unsigned *val)
+{
+	struct qpnp_pinattrib attr;
+	unsigned int type, subtype, field;
+	unsigned int addr, buff;
+	int ret;
+
+	*val = 0;
+	type = pad->type;
+	subtype = pad->subtype;
+
+	switch (param) {
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+	case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+		if (type != QPNP_GPIO_TYPE)
+			return -ENXIO;
+		attr.addr  = QPNP_REG_DIG_OUT_CTL;
+		attr.shift = QPNP_REG_OUT_TYPE_SHIFT;
+		attr.mask  = QPNP_REG_OUT_TYPE_MASK;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (type != QPNP_GPIO_TYPE)
+			return -ENXIO;
+	/* Fallthrough */
+	case PIN_CONFIG_BIAS_DISABLE:
+	case PIN_CONFIG_BIAS_PULL_UP:
+		attr.addr  = QPNP_REG_DIG_PULL_CTL;
+		attr.shift = QPNP_REG_PULL_SHIFT;
+		attr.mask  = QPNP_REG_PULL_MASK;
+		break;
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		attr.addr  = QPNP_REG_EN_CTL;
+		attr.shift = QPNP_REG_MASTER_EN_SHIFT;
+		attr.mask  = QPNP_REG_MASTER_EN_MASK;
+		break;
+	case PIN_CONFIG_POWER_SOURCE:
+		attr.addr  = QPNP_REG_DIG_VIN_CTL;
+		attr.shift = QPNP_REG_VIN_SHIFT;
+		attr.mask  = QPNP_REG_VIN_MASK;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		if (type != QPNP_MPP_TYPE)
+			return -ENXIO;
+		attr.addr  = QPNP_REG_SINK_CTL;
+		attr.shift = QPNP_REG_CS_OUT_SHIFT;
+		attr.mask  = QPNP_REG_CS_OUT_MASK;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+		attr.addr  = QPNP_REG_EN_CTL;
+		attr.shift = QPNP_REG_MASTER_EN_SHIFT;
+		attr.mask  = QPNP_REG_MASTER_EN_MASK;
+		break;
+	case PIN_CONFIG_OUTPUT:
+		attr.addr  = QPNP_REG_MODE_CTL;
+		attr.shift = QPNP_REG_OUT_SRC_SEL_SHIFT;
+		attr.mask  = QPNP_REG_OUT_SRC_SEL_MASK;
+		break;
+	case QPNP_PINCONF_PARAM_PULL_UP:
+		if (type != QPNP_GPIO_TYPE)
+			return -ENXIO;
+		attr.addr  = QPNP_REG_DIG_PULL_CTL;
+		attr.shift = QPNP_REG_PULL_SHIFT;
+		attr.mask  = QPNP_REG_PULL_MASK;
+		break;
+	case QPNP_PINCONF_PARAM_STRENGTH:
+		if (type != QPNP_GPIO_TYPE)
+			return -ENXIO;
+		attr.addr  = QPNP_REG_DIG_OUT_CTL;
+		attr.shift = QPNP_REG_OUT_STRENGTH_SHIFT;
+		attr.mask  = QPNP_REG_OUT_STRENGTH_MASK;
+		break;
+	case QPNP_PINCONF_PARAM_AOUT_CTRL:
+		if (type != QPNP_MPP_TYPE)
+			return -ENXIO;
+		if (subtype == QPNP_MPP_SUBTYPE_4CH_NO_ANA_OUT ||
+		    subtype == QPNP_MPP_SUBTYPE_ULT_4CH_NO_ANA_OUT)
+			return -ENXIO;
+		attr.addr  = QPNP_REG_AOUT_CTL;
+		attr.shift = QPNP_REG_AOUT_REF_SHIFT;
+		attr.mask  = QPNP_REG_AOUT_REF_MASK;
+		break;
+	case QPNP_PINCONF_PARAM_AIN_CTRL:
+		if (type != QPNP_MPP_TYPE)
+			return -ENXIO;
+		attr.addr  = QPNP_REG_AIN_CTL;
+		attr.shift = QPNP_REG_AIN_ROUTE_SHIFT;
+		attr.mask  = QPNP_REG_AIN_ROUTE_MASK;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	addr = QPNP_REG_ADDR(pad, attr.addr);
+	ret = regmap_read(qctrl->map, addr, &buff);
+	if (ret < 0)
+		return ret;
+
+	field = QPNP_GET(buff, attr.shift, attr.mask);
+
+	switch (param) {
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		if (field == QPNP_GPIO_OUT_BUF_CMOS)
+			*val = 1;
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		if (field == QPNP_GPIO_OUT_BUF_OPEN_DRAIN_NMOS)
+			*val = 1;
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+		if (field == QPNP_GPIO_OUT_BUF_OPEN_DRAIN_PMOS)
+			*val = 1;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (type == QPNP_GPIO_TYPE) {
+			if (field == QPNP_GPIO_PULL_NO)
+				*val = 1;
+		} else {
+			if (field == QPNP_MPP_PULL_UP_OPEN)
+				*val = 1;
+		}
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		switch (field) {
+		default:
+		case QPNP_MPP_PULL_UP_OPEN:
+			*val = 0;
+			break;
+		case QPNP_MPP_PULL_UP_0P6KOHM:
+			*val = 600;
+			break;
+		case QPNP_MPP_PULL_UP_10KOHM:
+			*val = 10000;
+			break;
+		case QPNP_MPP_PULL_UP_30KOHM:
+			*val = 30000;
+			break;
+		}
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (field == QPNP_GPIO_PULL_DN)
+			*val = 1;
+		break;
+	case PIN_CONFIG_POWER_SOURCE:
+		*val = field;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		*val = (field + 1) * 5;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+		*val = field;
+		break;
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		if (field == QPNP_PIN_MASTER_DISABLE)
+			*val = 1;
+		break;
+	case PIN_CONFIG_OUTPUT:
+		*val = field;
+		break;
+	case QPNP_PINCONF_PARAM_PULL_UP:
+		switch (field) {
+		case QPNP_GPIO_PULL_NO:
+			field = 0;
+			break;
+		case QPNP_GPIO_PULL_UP_30:
+			field = PM8XXX_GPIO_PULL_UP_30;
+			break;
+		case QPNP_GPIO_PULL_UP_1P5:
+			field = PM8XXX_GPIO_PULL_UP_1P5;
+			break;
+		case QPNP_GPIO_PULL_UP_31P5:
+			field = PM8XXX_GPIO_PULL_UP_31P5;
+			break;
+
+		case QPNP_GPIO_PULL_UP_1P5_30:
+			field = PM8XXX_GPIO_PULL_UP_1P5_30;
+			break;
+		}
+		*val = field;
+		break;
+	case QPNP_PINCONF_PARAM_STRENGTH:
+		switch (field) {
+		case QPNP_GPIO_STRENGTH_HIGH:
+			field = PM8XXX_GPIO_STRENGTH_HIGH;
+			break;
+		case QPNP_GPIO_STRENGTH_MED:
+			field = PM8XXX_GPIO_STRENGTH_MED;
+			break;
+		case QPNP_GPIO_STRENGTH_LOW:
+			field = PM8XXX_GPIO_STRENGTH_LOW;
+			break;
+		}
+		*val = field;
+		break;
+	case QPNP_PINCONF_PARAM_AOUT_CTRL:
+	case QPNP_PINCONF_PARAM_AIN_CTRL:
+		*val = field;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qpnp_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct qpnp_pinctrl *qpctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	/* Every PIN is a group */
+	return qpctrl->desc.npins;
+}
+
+static const char *qpnp_get_group_name(struct pinctrl_dev *pctldev,
+				       unsigned pin)
+{
+	struct qpnp_pinctrl *qpctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	/* Every PIN is a group */
+	return qpctrl->desc.pins[pin].name;
+}
+
+static int qpnp_get_group_pins(struct pinctrl_dev *pctldev,
+			      unsigned pin,
+			      const unsigned **pins,
+			      unsigned *num_pins)
+{
+	struct qpnp_pinctrl *qpctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	/* Every PIN is a group */
+	*pins = &qpctrl->desc.pins[pin].number;
+	*num_pins = 1;
+	return 0;
+}
+
+static int qpnp_parse_dt_config(struct device *dev, struct device_node *np,
+			unsigned long **configs, unsigned int *nconfigs)
+{
+	struct qpnp_pinbindings *par;
+	unsigned long *cfg;
+	unsigned int ncfg = 0;
+	int ret, idx;
+	u32 val;
+
+	if (!np)
+		return -EINVAL;
+
+	/* allocate a temporary array big enough to hold one of each option */
+	cfg = kcalloc(ARRAY_SIZE(qpnp_pinbindings), sizeof(*cfg), GFP_KERNEL);
+	if (!cfg)
+		return -ENOMEM;
+
+	for (idx = 0; idx < ARRAY_SIZE(qpnp_pinbindings); idx++) {
+		par = &qpnp_pinbindings[idx];
+		ret = of_property_read_u32(np, par->property, &val);
+
+		/* property not found */
+		if (ret == -EINVAL)
+			continue;
+
+		/* use default value, when no value is specified */
+		if (ret)
+			val = par->default_value;
+
+		dev_dbg(dev, "found %s with value %u\n", par->property, val);
+		cfg[ncfg] = pinconf_to_config_packed(par->param, val);
+		ncfg++;
+	}
+
+	ret = 0;
+
+	/* no configs found at qchip->npads */
+	if (ncfg == 0) {
+		*configs = NULL;
+		*nconfigs = 0;
+		goto out;
+	}
+
+	/*
+	 * Now limit the number of configs to the real number of
+	 * found properties.
+	 */
+	*configs = kcalloc(ncfg, sizeof(unsigned long), GFP_KERNEL);
+	if (!*configs) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(*configs, cfg, ncfg * sizeof(unsigned long));
+	*nconfigs = ncfg;
+
+out:
+	kfree(cfg);
+	return ret;
+}
+
+static int qpnp_dt_subnode_to_map(struct pinctrl_dev *pctldev,
+				  struct device_node *np,
+				  struct pinctrl_map **map,
+				  unsigned *reserv, unsigned *nmaps,
+				  enum pinctrl_map_type type)
+{
+	unsigned long *configs = NULL;
+	unsigned num_configs = 0;
+	struct property *prop;
+	const char *group;
+	int ret;
+
+	ret = qpnp_parse_dt_config(pctldev->dev, np, &configs, &num_configs);
+	if (ret < 0)
+		return ret;
+
+	if (!num_configs)
+		return 0;
+
+	ret = of_property_count_strings(np, "pins");
+	if (ret < 0)
+		goto exit;
+
+	ret = pinctrl_utils_reserve_map(pctldev, map, reserv,
+					nmaps, ret);
+	if (ret < 0)
+		goto exit;
+
+	of_property_for_each_string(np, "pins", prop, group) {
+		ret = pinctrl_utils_add_map_configs(pctldev, map,
+				reserv, nmaps, group, configs,
+				num_configs, type);
+		if (ret < 0)
+			break;
+	}
+exit:
+	kfree(configs);
+	return ret;
+}
+
+static int qpnp_dt_node_to_map(struct pinctrl_dev *pctldev,
+			       struct device_node *np_config,
+			       struct pinctrl_map **map,
+			       unsigned *nmaps)
+{
+	struct device_node *np;
+	enum pinctrl_map_type type;
+	unsigned reserv;
+	int ret;
+
+	ret = 0;
+	*map = NULL;
+	*nmaps = 0;
+	reserv = 0;
+	type = PIN_MAP_TYPE_CONFIGS_PIN;
+
+	for_each_child_of_node(np_config, np) {
+
+		ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map,
+							&reserv, nmaps, type);
+		if (ret)
+			break;
+
+		ret = qpnp_dt_subnode_to_map(pctldev, np, map, &reserv,
+					     nmaps, type);
+		if (ret)
+			break;
+	}
+
+	if (ret < 0)
+		pinctrl_utils_dt_free_map(pctldev, *map, *nmaps);
+
+	return ret;
+}
+
+static const struct pinctrl_ops qpnp_pinctrl_ops = {
+	.get_groups_count	= qpnp_get_groups_count,
+	.get_group_name		= qpnp_get_group_name,
+	.get_group_pins		= qpnp_get_group_pins,
+	.dt_node_to_map		= qpnp_dt_node_to_map,
+	.dt_free_map		= pinctrl_utils_dt_free_map,
+};
+
+static int qpnp_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(qpnp_functions_names);
+}
+
+static const char *qpnp_get_function_name(struct pinctrl_dev *pctldev,
+					 unsigned function)
+{
+	return qpnp_functions_names[function];
+}
+
+static int qpnp_get_function_groups(struct pinctrl_dev *pctldev,
+				  unsigned function,
+				  const char *const **groups,
+				  unsigned *const num_qgroups)
+{
+	struct qpnp_pinctrl *qctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = qctrl->groups[function].names;
+	*num_qgroups = qctrl->groups[function].npins;
+	return 0;
+}
+
+static int qpnp_pinmux_enable(struct pinctrl_dev *pctldev,
+			     unsigned function,
+			     unsigned pin)
+{
+	struct qpnp_pinctrl *qctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct qpnp_padinfo *pad;
+	unsigned int addr, val, mask;
+	int idx, ret;
+
+	pad = qpnp_get_desc(qctrl, pin);
+	if (!pad)
+		return -EINVAL;
+
+	for (idx = 0; idx < ARRAY_SIZE(pad->funcs); idx++)
+		if (pad->funcs[idx] == function)
+			break;
+
+	if (WARN_ON(idx == ARRAY_SIZE(pad->funcs)))
+		return -EINVAL;
+
+
+	switch (function) {
+	case QPNP_FUNC_GPIO:
+		val = QPNP_PIN_MODE_DIG_IN_OUT;
+		break;
+	case QPNP_FUNC_AIN:
+		if (pad->type == QPNP_GPIO_TYPE)
+			return -EINVAL;
+		val = QPNP_PIN_MODE_AIN;
+		break;
+	case QPNP_FUNC_AOUT:
+		if (pad->type == QPNP_GPIO_TYPE)
+			return -EINVAL;
+		val = QPNP_PIN_MODE_AOUT;
+		break;
+	case QPNP_FUNC_CS:
+		if (pad->type == QPNP_GPIO_TYPE)
+			return -EINVAL;
+		val = QPNP_PIN_MODE_SINK;
+		break;
+	default:
+		return -EINVAL;
+		break;
+	}
+
+	addr = QPNP_REG_ADDR(pad, QPNP_REG_MODE_CTL);
+	val = val << QPNP_REG_MODE_SEL_SHIFT;
+	mask = QPNP_REG_MODE_SEL_MASK;
+	ret = regmap_update_bits(qctrl->map, addr, mask, val);
+	if (ret)
+		return ret;
+
+	addr = QPNP_REG_ADDR(pad, QPNP_REG_EN_CTL);
+	val = BIT(QPNP_REG_MASTER_EN_SHIFT);
+	mask = QPNP_REG_MASTER_EN_MASK;
+	ret = regmap_update_bits(qctrl->map, addr, mask, val);
+
+	return ret;
+}
+
+static const struct pinmux_ops qpnp_pinmux_ops = {
+	.get_functions_count	= qpnp_get_functions_count,
+	.get_function_name	= qpnp_get_function_name,
+	.get_function_groups	= qpnp_get_function_groups,
+	.enable			= qpnp_pinmux_enable,
+};
+
+static int qpnp_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
+	struct qpnp_padinfo *pad;
+	unsigned int val, en_mask, buff, addr;
+	int ret;
+
+	pad = qpnp_get_desc(qctrl, offset);
+	if (!pad)
+		return -EINVAL;
+
+	addr = QPNP_REG_ADDR(pad, QPNP_REG_MODE_CTL);
+	ret = regmap_read(qctrl->map, addr, &buff);
+	if (ret < 0)
+		return ret;
+
+	/* GPIO val is from RT status if input is enabled */
+	if ((buff & QPNP_REG_MODE_SEL_MASK) == QPNP_PIN_MODE_DIG_IN) {
+
+		addr = QPNP_REG_ADDR(pad, QPNP_REG_STATUS1);
+		ret = regmap_read(qctrl->map, addr, &val);
+		if (ret < 0)
+			return ret;
+
+		if (pad->type == QPNP_GPIO_TYPE && pad->major == 0)
+			en_mask = QPNP_REG_STATUS1_GPIO_EN_REV0_MASK;
+		else if (pad->type == QPNP_GPIO_TYPE &&
+			 pad->major > 0)
+			en_mask = QPNP_REG_STATUS1_GPIO_EN_MASK;
+		else		/* MPP */
+			en_mask = QPNP_REG_STATUS1_MPP_EN_MASK;
+
+		if (!(val & en_mask))
+			return -EPERM;
+
+		ret = val & QPNP_REG_STATUS1_VAL_MASK;
+
+	} else {
+		ret = buff & QPNP_REG_OUT_SRC_SEL_MASK;
+		ret =  ret >> QPNP_REG_OUT_SRC_SEL_SHIFT;
+	}
+
+	return !!ret;
+}
+
+static void qpnp_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
+	struct qpnp_padinfo *pad;
+	unsigned int addr, buff;
+
+	pad = qpnp_get_desc(qctrl, offset);
+	if (!pad)
+		return;
+
+	addr = QPNP_REG_ADDR(pad, QPNP_REG_MODE_CTL);
+	buff = !!value << QPNP_REG_OUT_SRC_SEL_SHIFT;
+
+	regmap_update_bits(qctrl->map, addr, QPNP_REG_OUT_SRC_SEL_MASK, buff);
+}
+
+static int qpnp_config_get(struct pinctrl_dev *pctldev,
+			  unsigned int pin,
+			  unsigned long *config)
+{
+	struct qpnp_pinctrl *qctrl = pinctrl_dev_get_drvdata(pctldev);
+	unsigned param = pinconf_to_config_param(*config);
+	struct qpnp_padinfo *pad;
+	unsigned arg;
+	int ret;
+
+	pad = qpnp_get_desc(qctrl, pin);
+	if (!pad)
+		return -EINVAL;
+
+	/* Convert pinconf values to register values */
+	ret = qpnp_conv_from_pin(qctrl, pad, param, &arg);
+	if (ret)
+		return ret;
+
+	/* Convert register value to pinconf value */
+	*config = pinconf_to_config_packed(param, arg);
+	return 0;
+}
+
+static int qpnp_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			  unsigned long *configs, unsigned num_configs)
+{
+	struct qpnp_pinctrl *qctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct qpnp_padinfo *pad;
+	unsigned param;
+	unsigned arg;
+	int idx, ret;
+
+	pad = qpnp_get_desc(qctrl, pin);
+	if (!pad)
+		return -EINVAL;
+
+	for (idx = 0; idx < num_configs; idx++) {
+		param = pinconf_to_config_param(configs[idx]);
+		arg = pinconf_to_config_argument(configs[idx]);
+
+		/* Convert pinconf values to register values */
+		ret = qpnp_conv_to_pin(qctrl, pad, param, arg);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void qpnp_config_dbg_show(struct pinctrl_dev *pctldev,
+				 struct seq_file *s, unsigned pin)
+{
+	struct qpnp_pinctrl *qctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct qpnp_padinfo *pad;
+	const char *mode = NULL, *name = NULL;
+	unsigned en, val;
+	unsigned int buff, addr;
+	int ret;
+
+	pad = qpnp_get_desc(qctrl, pin);
+	if (!pad)
+		return;
+
+	name = pad->name;
+
+	addr = QPNP_REG_ADDR(pad, QPNP_REG_MODE_CTL);
+	ret = regmap_read(qctrl->map, addr, &buff);
+	if (ret < 0) {
+		seq_printf(s, " %-8s: read error %d", name, ret);
+		return;
+	}
+	val = QPNP_GET(buff, QPNP_REG_MODE_SEL_SHIFT, QPNP_REG_MODE_SEL_MASK);
+
+	addr = QPNP_REG_ADDR(pad, QPNP_REG_EN_CTL);
+	ret = regmap_read(qctrl->map, addr, &buff);
+	if (ret < 0) {
+		seq_printf(s, " %-8s: read error %d", name, ret);
+		return;
+	}
+	en = QPNP_GET(buff, QPNP_REG_MASTER_EN_SHIFT, QPNP_REG_MASTER_EN_MASK);
+
+	switch (val) {
+	case QPNP_PIN_MODE_DIG_IN:
+		mode = "dig-in";
+		break;
+	case QPNP_PIN_MODE_DIG_OUT:
+		mode = "dig-out";
+		break;
+	case QPNP_PIN_MODE_DIG_IN_OUT:
+		mode = "dig-io";
+		break;
+	case QPNP_PIN_MODE_BIDIR:
+		mode = "ana-io";
+		break;
+	case QPNP_PIN_MODE_AIN:
+		mode = "ana-in";
+		break;
+	case QPNP_PIN_MODE_AOUT:
+		mode = "ana-out";
+		break;
+	case QPNP_PIN_MODE_SINK:
+		mode = "ana-sink";
+		break;
+	default:
+		return;
+	}
+
+	seq_printf(s, " %-8s: %-9s %s", name, mode, !en ? "high-Z" : "");
+}
+
+static const struct pinconf_ops qpnp_pinconf_ops = {
+	.pin_config_get		= qpnp_config_get,
+	.pin_config_set		= qpnp_config_set,
+	.pin_config_group_get	= qpnp_config_get,
+	.pin_config_group_set	= qpnp_config_set,
+	.pin_config_group_dbg_show = qpnp_config_dbg_show,
+};
+
+static int qpnp_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
+	unsigned long config;
+
+	config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 1);
+
+	return qpnp_config_set(qctrl->ctrl, offset, &config, 1);
+}
+
+static int qpnp_direction_output(struct gpio_chip *chip,
+			      unsigned offset, int val)
+{
+	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
+	unsigned long config;
+
+	config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT, val);
+
+	return qpnp_config_set(qctrl->ctrl, offset, &config, 1);
+}
+
+static int qpnp_request(struct gpio_chip *chip, unsigned offset)
+{
+	return pinctrl_request_gpio(chip->base + offset);
+}
+
+static void qpnp_free(struct gpio_chip *chip, unsigned offset)
+{
+	pinctrl_free_gpio(chip->base + offset);
+}
+
+static int qpnp_of_xlate(struct gpio_chip *chip,
+		       const struct of_phandle_args *gpio_desc, u32 *flags)
+{
+	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
+	struct qpnp_padinfo *pad;
+
+	if (chip->of_gpio_n_cells < 2) {
+		dev_err(qctrl->dev, "of_gpio_n_cells < 2\n");
+		return -EINVAL;
+	}
+
+	pad = qpnp_get_desc(qctrl, gpio_desc->args[0]);
+	if (!pad)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpio_desc->args[1];
+
+	return gpio_desc->args[0];
+}
+
+static int qpnp_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
+	struct qpnp_padinfo *pad;
+
+	pad = qpnp_get_desc(qctrl, offset);
+	if (!pad)
+		return -EINVAL;
+
+	return pad->irq;
+}
+
+static void qpnp_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
+	unsigned idx;
+
+	for (idx = 0; idx < chip->ngpio; idx++) {
+		qpnp_config_dbg_show(qctrl->ctrl, s, idx);
+		seq_puts(s, "\n");
+	}
+}
+
+static const struct gpio_chip qpnp_gpio_template = {
+	.direction_input  = qpnp_direction_input,
+	.direction_output = qpnp_direction_output,
+	.get              = qpnp_get,
+	.set              = qpnp_set,
+	.request          = qpnp_request,
+	.free             = qpnp_free,
+	.of_xlate	  = qpnp_of_xlate,
+	.to_irq		  = qpnp_to_irq,
+	.dbg_show         = qpnp_dbg_show,
+};
+
+static int qpnp_discover(struct platform_device *pdev,
+			struct qpnp_pinctrl *qctrl,
+			const struct qpnp_chipinfo *qchip)
+{
+	struct device *dev = qctrl->dev;
+	struct pinctrl_pin_desc *desc, *descs;
+	struct qpnp_padinfo *pad, *pads;
+	int idx, ret, cnt, gps, ais, aos, css;
+	const char **names, *format;
+	unsigned int addr;
+
+	pads = devm_kcalloc(dev, qchip->npads, sizeof(*pads), GFP_KERNEL);
+	if (!pads)
+		return -ENOMEM;
+
+	descs = devm_kcalloc(dev, qchip->npads, sizeof(*descs), GFP_KERNEL);
+	if (!descs)
+		return -ENOMEM;
+
+	for (idx = 0; idx < qchip->npads; idx++) {
+
+		pad = &pads[idx];
+		desc = &descs[idx];
+
+		pad->irq = platform_get_irq(pdev, idx);
+		if (pad->irq < 0)
+			return pad->irq;
+
+		pad->offset = qchip->base + (idx * 0x100);
+
+		addr = QPNP_REG_ADDR(pad, QPNP_REG_DIG_MAJOR_REV);
+		ret = regmap_read(qctrl->map, addr, &pad->major);
+		if (ret < 0)
+			return ret;
+
+		addr = QPNP_REG_ADDR(pad, QPNP_REG_TYPE);
+		ret = regmap_read(qctrl->map, addr, &pad->type);
+		if (ret < 0)
+			return ret;
+
+		addr = QPNP_REG_ADDR(pad, QPNP_REG_SUBTYPE);
+		ret = regmap_read(qctrl->map, addr, &pad->subtype);
+		if (ret < 0)
+			return ret;
+
+		ret = qpnp_control_init(qctrl, pad);
+		if (ret)
+			return ret;
+
+		if (pad->type == QPNP_GPIO_TYPE)
+			format = "gpio%d";
+		else
+			format = "mpp%d";
+
+		snprintf(pad->name, ARRAY_SIZE(pad->name), format, idx + 1);
+
+		desc->number = idx;
+		desc->name = pad->name;
+	}
+
+	for (idx = QPNP_FUNC_GPIO; idx < QPNP_FUNC_CNT; idx++) {
+		cnt = qctrl->groups[idx].npins;
+		if (!cnt)
+			continue;
+
+		names = devm_kcalloc(dev, cnt, sizeof(names), GFP_KERNEL);
+		if (!names)
+			return -ENOMEM;
+
+		qctrl->groups[idx].names = names;
+	}
+
+	gps = ais = aos = css = 0;
+	/* now scan through again and populate the lookup table */
+	for (idx = 0; idx < qchip->npads; idx++) {
+
+		pad = &pads[idx];
+
+		if (pad->funcs[QPNP_FUNC_GPIO] == QPNP_FUNC_GPIO)
+			qctrl->groups[QPNP_FUNC_GPIO].names[gps++] = pad->name;
+		if (pad->funcs[QPNP_FUNC_AIN] == QPNP_FUNC_AIN)
+			qctrl->groups[QPNP_FUNC_AIN].names[ais++] = pad->name;
+		if (pad->funcs[QPNP_FUNC_AOUT] == QPNP_FUNC_AOUT)
+			qctrl->groups[QPNP_FUNC_AOUT].names[aos++] = pad->name;
+		if (pad->funcs[QPNP_FUNC_CS] == QPNP_FUNC_CS)
+			qctrl->groups[QPNP_FUNC_CS].names[css++] = pad->name;
+	}
+
+
+	qctrl->pads = pads;
+
+	qctrl->chip = qpnp_gpio_template;
+	qctrl->chip.base = -1;
+	qctrl->chip.ngpio = qchip->npads;
+	qctrl->chip.label = dev_name(dev);
+	qctrl->chip.of_gpio_n_cells = 2;
+	qctrl->chip.can_sleep = true;
+
+	qctrl->desc.pctlops = &qpnp_pinctrl_ops,
+	qctrl->desc.pmxops = &qpnp_pinmux_ops,
+	qctrl->desc.confops = &qpnp_pinconf_ops,
+	qctrl->desc.owner = THIS_MODULE,
+	qctrl->desc.name = dev_name(dev);
+	qctrl->desc.pins = descs;
+	qctrl->desc.npins = qchip->npads;
+
+	qctrl->range.name = dev_name(dev);
+	qctrl->range.id = 0;
+	qctrl->range.base = 0;
+	qctrl->range.npins = qchip->npads;
+	qctrl->range.gc = &qctrl->chip;
+
+	ret = gpiochip_add(&qctrl->chip);
+	if (ret) {
+		dev_err(qctrl->dev, "can't add gpio chip\n");
+		return ret;
+	}
+
+	qctrl->ctrl = pinctrl_register(&qctrl->desc, dev, qctrl);
+	if (!qctrl->ctrl)
+		ret = -ENODEV;
+	else
+		pinctrl_add_gpio_range(qctrl->ctrl, &qctrl->range);
+
+	return ret;
+
+	return 0;
+}
+
+static const struct of_device_id qpnp_pinctrl_of_match[];
+
+static int qpnp_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct qpnp_chipinfo *qchip;
+	struct qpnp_pinctrl *qctrl;
+
+	qctrl = devm_kzalloc(dev, sizeof(*qctrl), GFP_KERNEL);
+	if (!qctrl)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, qctrl);
+
+	qchip = of_match_node(qpnp_pinctrl_of_match, dev->of_node)->data;
+
+	qctrl->dev = &pdev->dev;
+	qctrl->map = dev_get_regmap(dev->parent, NULL);
+
+	return qpnp_discover(pdev, qctrl, qchip);
+}
+
+static int qpnp_pinctrl_remove(struct platform_device *pdev)
+{
+	struct qpnp_pinctrl *qctrl = platform_get_drvdata(pdev);
+
+	pinctrl_unregister(qctrl->ctrl);
+
+	return gpiochip_remove(&qctrl->chip);
+}
+
+static const struct qpnp_chipinfo qpnp_pm8841_mpp_info = {
+	.npads	= 4,
+	.base	= 0xa000
+};
+
+static const struct qpnp_chipinfo qpnp_pm8941_gpio_info = {
+	.npads	= 36,
+	.base	= 0xc000,
+};
+
+static const struct qpnp_chipinfo qpnp_pm8941_mpp_info = {
+	.npads	= 8,
+	.base	= 0xa000
+};
+
+static const struct qpnp_chipinfo qpnp_pma8084_mpp_info = {
+	.npads	= 4,
+	.base	= 0xa000
+};
+
+static const struct qpnp_chipinfo qpnp_pma8084_gpio_info = {
+	.npads	= 22,
+	.base	= 0xc000,
+};
+
+static const struct of_device_id qpnp_pinctrl_of_match[] = {
+	{ .compatible = "qcom,pm8941-gpio", .data = &qpnp_pm8941_gpio_info },
+	{ .compatible = "qcom,pm8941-mpp", .data = &qpnp_pm8941_mpp_info },
+	{ .compatible = "qcom,pm8841-mpp", .data = &qpnp_pm8841_mpp_info },
+	{ .compatible = "qcom,pma8084-gpio", .data = &qpnp_pma8084_gpio_info },
+	{ .compatible = "qcom,pma8084-mpp", .data = &qpnp_pma8084_mpp_info },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, qpnp_pinctrl_of_match);
+
+static struct platform_driver qpnp_pinctrl_driver = {
+	.driver = {
+		.name = "qpnp-pinctrl",
+		.owner = THIS_MODULE,
+		.of_match_table = qpnp_pinctrl_of_match,
+	},
+	.probe = qpnp_pinctrl_probe,
+	.remove = qpnp_pinctrl_remove,
+};
+module_platform_driver(qpnp_pinctrl_driver);
+
+MODULE_AUTHOR("Ivan T. Ivanov <iivanov@mm-sol.com>");
+MODULE_DESCRIPTION("Qualcomm QPNP pin control driver");
+MODULE_ALIAS("platform:qpnp-pinctrl");
+MODULE_LICENSE("GPL v2");
diff --git a/include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h b/include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h
new file mode 100644
index 0000000..08def79
--- /dev/null
+++ b/include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h
@@ -0,0 +1,34 @@
+/*
+ * Provides constants for the pm8xxx multy-purpose pin (MPP) binding.
+ */
+
+#ifndef _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_MPP_H
+#define _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_MPP_H
+
+/*
+ * Analog Output - Set the analog output reference.
+ * To be used with "qcom,aout = <>"
+ */
+#define PM8XXX_MPP_AOUT_1V25			0
+#define PM8XXX_MPP_AOUT_0V625			1
+#define PM8XXX_MPP_AOUT_0V3125			2
+#define PM8XXX_MPP_AOUT_MPP			3
+#define PM8XXX_MPP_AOUT_ABUS1			4
+#define PM8XXX_MPP_AOUT_ABUS2			5
+#define PM8XXX_MPP_AOUT_ABUS3			6
+#define PM8XXX_MPP_AOUT_ABUS4			7
+
+/*
+ * Analog Input - Set the source for analog input.
+ * To be used with "qcom,ain = <>"
+ */
+#define PM8XXX_MPP_AIN_CH5			0
+#define PM8XXX_MPP_AIN_CH6			1
+#define PM8XXX_MPP_AIN_CH7			2
+#define PM8XXX_MPP_AIN_CH8			3
+#define PM8XXX_MPP_AIN_ABUS1			4
+#define PM8XXX_MPP_AIN_ABUS2			5
+#define PM8XXX_MPP_AIN_ABUS3			6
+#define PM8XXX_MPP_AIN_ABUS4			7
+
+#endif
-- 
1.8.3.2

RE: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[kiran.padwal]

Hi,

On Thursday, July 17, 2014 11:25am, "Ivan T. Ivanov" <iivanov@mm-sol.com> said:

> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
> 
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  drivers/pinctrl/Kconfig                       |   12 +
>  drivers/pinctrl/Makefile                      |    1 +
>  drivers/pinctrl/pinctrl-qpnp.c                | 1565 +++++++++++++++++++++++++
.
<snip>
.
> diff --git a/drivers/pinctrl/pinctrl-qpnp.c b/drivers/pinctrl/pinctrl-qpnp.c
> new file mode 100644
> index 0000000..aedc72e
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-qpnp.c
.
<snip>
.
> +#define QPNP_MPP_CS_OUT_35MA			6
> +#define QPNP_MPP_CS_OUT_40MA			7
> +
> +/* revision registers base address offsets */

unused define, can you please remove it

> +#define QPNP_REG_DIG_MINOR_REV			0x0
> +#define QPNP_REG_DIG_MAJOR_REV			0x1

ditto

> +#define QPNP_REG_ANA_MINOR_REV			0x2
> +
> +/* type registers base address offsets */
> +#define QPNP_REG_TYPE				0x4
> +#define QPNP_REG_SUBTYPE			0x5
> +
> +/* GPIO peripheral type and subtype values */
> +#define QPNP_GPIO_TYPE				0x10
> +#define QPNP_GPIO_SUBTYPE_GPIO_4CH		0x1
> +#define QPNP_GPIO_SUBTYPE_GPIOC_4CH		0x5
> +#define QPNP_GPIO_SUBTYPE_GPIO_8CH		0x9
> +#define QPNP_GPIO_SUBTYPE_GPIOC_8CH		0xd
> +
> +/* mpp peripheral type and subtype values */
> +#define QPNP_MPP_TYPE				0x11
> +#define QPNP_MPP_SUBTYPE_4CH_NO_ANA_OUT		0x3
> +#define QPNP_MPP_SUBTYPE_ULT_4CH_NO_ANA_OUT	0x4
> +#define QPNP_MPP_SUBTYPE_4CH_NO_SINK		0x5
> +#define QPNP_MPP_SUBTYPE_ULT_4CH_NO_SINK	0x6
> +#define QPNP_MPP_SUBTYPE_4CH_FULL_FUNC		0x7
> +#define QPNP_MPP_SUBTYPE_8CH_FULL_FUNC		0xf
> +
> +#define QPNP_REG_STATUS1			0x8
> +#define QPNP_REG_STATUS1_VAL_MASK		0x1
> +#define QPNP_REG_STATUS1_GPIO_EN_REV0_MASK	0x2
> +#define QPNP_REG_STATUS1_GPIO_EN_MASK		0x80
> +#define QPNP_REG_STATUS1_MPP_EN_MASK		0x80
> +
> +/* control register base address offsets */
> +#define QPNP_REG_MODE_CTL			0x40
> +#define QPNP_REG_DIG_VIN_CTL			0x41
> +#define QPNP_REG_DIG_PULL_CTL			0x42

ditto

> +#define QPNP_REG_DIG_IN_CTL			0x43
> +#define QPNP_REG_DIG_OUT_CTL			0x45
> +#define QPNP_REG_EN_CTL				0x46
> +#define QPNP_REG_AOUT_CTL			0x4b
> +#define QPNP_REG_AIN_CTL			0x4a
> +#define QPNP_REG_SINK_CTL			0x4c
> +
.
<snip>
.
> +#define PM8XXX_MPP_AIN_ABUS3			6
> +#define PM8XXX_MPP_AIN_ABUS4			7
> +
> +#endif
> --
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Regards,
--Kiran

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[divya ojha]

Hi,

On Thu, Jul 17, 2014 at 8:55 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
>
> This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  drivers/pinctrl/Kconfig                       |   12 +
>  drivers/pinctrl/Makefile                      |    1 +
>  drivers/pinctrl/pinctrl-qpnp.c                | 1565 +++++++++++++++++++++++++
>  include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h |   34 +
..
+       struct device *dev = qctrl->dev;
> +       struct pinctrl_pin_desc *desc, *descs;
> +       struct qpnp_padinfo *pad, *pads;
> +       int idx, ret, cnt, gps, ais, aos, css;
> +       const char **names, *format;
> +       unsigned int addr;
> +
> +       pads = devm_kcalloc(dev, qchip->npads, sizeof(*pads), GFP_KERNEL);

when do we free these structures..?

> +       if (!pads)
> +               return -ENOMEM;
> +
> +       descs = devm_kcalloc(dev, qchip->npads, sizeof(*descs), GFP_KERNEL);

ditto..

> +       if (!descs)
> +               return -ENOMEM;
> +
..
> +               cnt = qctrl->groups[idx].npins;
> +               if (!cnt)
> +                       continue;
> +
> +               names = devm_kcalloc(dev, cnt, sizeof(names), GFP_KERNEL);

ditto..

> +               if (!names)
> +                       return -ENOMEM;

> +#define PM8XXX_MPP_AIN_ABUS4                   7
> +
> +#endif
> --
> 1.8.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Regards
Divya

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[pramod gurav]

On Mon, Jul 21, 2014 at 9:32 PM, divya ojha <odivya77@gmail.com> wrote:
> Hi,
>
> On Thu, Jul 17, 2014 at 8:55 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
>> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
>>
>> This is the pinctrl, pinmux, pinconf and gpiolib driver for the
>> Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
>>
>> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
>> ---
>>  drivers/pinctrl/Kconfig                       |   12 +
>>  drivers/pinctrl/Makefile                      |    1 +
>>  drivers/pinctrl/pinctrl-qpnp.c                | 1565 +++++++++++++++++++++++++
>>  include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h |   34 +
> ..
> +       struct device *dev = qctrl->dev;
>> +       struct pinctrl_pin_desc *desc, *descs;
>> +       struct qpnp_padinfo *pad, *pads;
>> +       int idx, ret, cnt, gps, ais, aos, css;
>> +       const char **names, *format;
>> +       unsigned int addr;
>> +
>> +       pads = devm_kcalloc(dev, qchip->npads, sizeof(*pads), GFP_KERNEL);
>
> when do we free these structures..?
>
devm_kmalloc is the function that is called in the end. Memory
allocated with this API will be automatically released after device
exit.
Read http://lxr.free-electrons.com/source/drivers/base/devres.c#L774 for more.


>> +       if (!pads)
>> +               return -ENOMEM;
>> +
>> +       descs = devm_kcalloc(dev, qchip->npads, sizeof(*descs), GFP_KERNEL);
>
> ditto..
>
>> +       if (!descs)
>> +               return -ENOMEM;
>> +
> ..
>> +               cnt = qctrl->groups[idx].npins;
>> +               if (!cnt)
>> +                       continue;
>> +
>> +               names = devm_kcalloc(dev, cnt, sizeof(names), GFP_KERNEL);
>
> ditto..
>
>> +               if (!names)
>> +                       return -ENOMEM;
>
>> +#define PM8XXX_MPP_AIN_ABUS4                   7
>> +
>> +#endif
>> --
>> 1.8.3.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> Regards
> Divya
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Thanks and Regards
Pramod

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[Ivan T. Ivanov]

On Mon, 2014-07-21 at 21:32 +0530, divya ojha wrote:
> Hi,
> 
> On Thu, Jul 17, 2014 at 8:55 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> >
> > This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> > Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
> >
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > ---
> >  drivers/pinctrl/Kconfig                       |   12 +
> >  drivers/pinctrl/Makefile                      |    1 +
> >  drivers/pinctrl/pinctrl-qpnp.c                | 1565 +++++++++++++++++++++++++
> >  include/dt-bindings/pinctrl/qcom,pm8xxx-mpp.h |   34 +
> ..
> +       struct device *dev = qctrl->dev;
> > +       struct pinctrl_pin_desc *desc, *descs;
> > +       struct qpnp_padinfo *pad, *pads;
> > +       int idx, ret, cnt, gps, ais, aos, css;
> > +       const char **names, *format;
> > +       unsigned int addr;
> > +
> > +       pads = devm_kcalloc(dev, qchip->npads, sizeof(*pads), GFP_KERNEL);
> 
> when do we free these structures..?

Good description could be found here[1].

Regards,
Ivan

[1] Documentation/driver-model/devres.txt

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[Linus Walleij]

On Thu, Jul 17, 2014 at 5:25 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:

> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
>
> This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
(...)
> +static int qpnp_pinctrl_remove(struct platform_device *pdev)
> +{
> +       struct qpnp_pinctrl *qctrl = platform_get_drvdata(pdev);
> +
> +       pinctrl_unregister(qctrl->ctrl);
> +
> +       return gpiochip_remove(&qctrl->chip);

We're removing the return value from gpiochip_remove() and I
have dropped the __must_check attribute today, so just remove
it unconditionally and return 0;

Sorry for short and lame comment on a large patch I should look
closer at, but atleast it's something.

Yours,
Linus Walleij

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[Ivan T. Ivanov]

On Wed, 2014-07-23 at 17:27 +0200, Linus Walleij wrote:
> On Thu, Jul 17, 2014 at 5:25 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> 
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> >
> > This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> > Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
> >
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> (...)
> > +static int qpnp_pinctrl_remove(struct platform_device *pdev)
> > +{
> > +       struct qpnp_pinctrl *qctrl = platform_get_drvdata(pdev);
> > +
> > +       pinctrl_unregister(qctrl->ctrl);
> > +
> > +       return gpiochip_remove(&qctrl->chip);
> 
> We're removing the return value from gpiochip_remove() and I
> have dropped the __must_check attribute today, so just remove
> it unconditionally and return 0;

Yes, I know. Patches are based on v3.16-rc6 tag.

> 
> Sorry for short and lame comment on a large patch I should look
> closer at, but atleast it's something.

Thanks, 
Ivan

> 
> Yours,
> Linus Walleij

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[David Collins]

On 07/17/2014 08:25 AM, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
> QPNP_REG_STATUS1_GPIO_EN_REV0_MASK
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>

(...)
> +static int qpnp_conv_to_pin(struct qpnp_pinctrl *qctrl,
> +			   struct qpnp_padinfo *pad, unsigned param,
> +			   unsigned val)
(...)
> +	switch (param) {
(...)
> +	case PIN_CONFIG_OUTPUT:
> +		nattrs = 3;
> +		attr[0].addr  = QPNP_REG_MODE_CTL;
> +		attr[0].shift = QPNP_REG_OUT_SRC_SEL_SHIFT;
> +		attr[0].mask  = QPNP_REG_OUT_SRC_SEL_MASK;
> +		attr[0].val   = !!val;

It seems that this patch provides no means to configure the output source
select bits to be anything besides 0 (constant low) or 1 (constant high).
 Some non-generic property is needed to configure this for both GPIOs and
MPPs.  Passing the value in via the output-high property does not seem
like a good approach since that is a generic pin config property that is
defined to take no value.  The special functions available for GPIOs (e.g.
PWM/LPG, clock, keypad, etc.) which are configured via this register are
used by many boards.

Something else to consider is that QPNP_REG_OUT_SRC_SEL_MASK is being
defined as 0xf which would imply that there are 16 possible output source
select options.  While technically true, this makes the situation more
complicated since half of those options are the inverted version of the
other half.  In the GPIO hardware this corresponds to an 8-way mux
followed by an XOR gate to conditionally invert the mux output.  If output
source select is handled this way, then the following values would need to
be supported in device tree for GPIOs:
	* 0:  constant low (already supported via output-low;)
	* 1:  constant high (already supported via output-high;)
	* 2:  paired GPIO
	* 3:  inverted paired GPIO
	* 4:  special function 1
	* 5:  inverted special function 1
	* 6:  special function 2
	* 7:  inverted special function 2
	* 8:  dtest1
	* 9:  inverted dtest1
	* 10: dtest2
	* 11: inverted dtest2
	* 12: dtest3
	* 13: inverted dtest3
	* 14: dtest4
	* 15: inverted dtest4
The same options are supported by MPPs except for special function 1,
inverted special function 1, special function 2, and inverted special
function 2.

If the output source select register parameter is instead treated as a
3-bit value along with an inversion bit, then the list of output selection
options that needs to be supported in device tree is cut in half:
	* 0:  constant (already supported)
	* 1:  paired GPIO
	* 2:  special function 1
	* 3:  special function 2
	* 4:  dtest1
	* 5:  dtest2
	* 6:  dtest3
	* 7:  dtest4
Another DT pin configuration property would then need to be used to
specify if the signal should be inverted or not.

> +		attr[1].addr  = QPNP_REG_MODE_CTL;
> +		attr[1].shift = QPNP_REG_MODE_SEL_SHIFT;
> +		attr[1].mask  = QPNP_REG_MODE_SEL_MASK;
> +		attr[1].val   = QPNP_PIN_MODE_DIG_OUT;
> +		attr[2].addr  = QPNP_REG_EN_CTL;
> +		attr[2].shift = QPNP_REG_MASTER_EN_SHIFT;
> +		attr[2].mask  = QPNP_REG_MASTER_EN_MASK;
> +		attr[2].val   = 1;
> +		break;

(...)

> +static int qpnp_of_xlate(struct gpio_chip *chip,
> +		       const struct of_phandle_args *gpio_desc, u32 *flags)
> +{
> +	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
> +	struct qpnp_padinfo *pad;
> +
> +	if (chip->of_gpio_n_cells < 2) {
> +		dev_err(qctrl->dev, "of_gpio_n_cells < 2\n");
> +		return -EINVAL;
> +	}
> +
> +	pad = qpnp_get_desc(qctrl, gpio_desc->args[0]);
> +	if (!pad)
> +		return -EINVAL;
> +
> +	if (flags)
> +		*flags = gpio_desc->args[1];
> +
> +	return gpio_desc->args[0];
> +}

This of_xlate callback function will result in the following situation:
If for example, a device tree consumer node wishes to use PM8941 GPIO 7
within gpiolib, then it would need to specify a gpiospec like this:
	<&pm8941_gpio 6 0>
There is an off-by-one issue with the indexing between the hardware GPIO
numbers (1-based) and the gpiolib gpio offsets (0-based).  Do you agree
that the indexing used within the device tree gpiospecs should match the
hardware numbering scheme?  I feel like this would be much less confusing
for users to work with.  If so, I think that a change to qpnp_of_xlate
like this would achieve it:

+#define QPNP_PIN_PHYSICAL_OFFSET	1

 static int qpnp_of_xlate(struct gpio_chip *chip,
 		     const struct of_phandle_args *gpio_desc, u32 *flags)
 {
 	struct qpnp_pinctrl *qctrl = to_qpnp_pinctrl(chip);
 	struct qpnp_padinfo *pad;
+	unsigned pin = gpio_desc->args[0] - QPNP_PIN_PHYSICAL_OFFSET;

 	if (chip->of_gpio_n_cells < 2) {
 		dev_err(qctrl->dev, "of_gpio_n_cells < 2\n");
 		return -EINVAL;
 	}

-	pad = qpnp_get_desc(qctrl, gpio_desc->args[0]);
+	pad = qpnp_get_desc(qctrl, pin);
 	if (!pad)
 		return -EINVAL;

 	if (flags)
 		*flags = gpio_desc->args[1];

-	return gpio_desc->args[0];
+	return pin;
 }

Take care,
David Collins

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

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[Ivan T. Ivanov]

On Fri, 2014-07-25 at 18:43 -0700, David Collins wrote:
> On 07/17/2014 08:25 AM, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > 
> > This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> > Qualcomm GPIO and MPP sub-function blocks found in the PMIC chips.
> > QPNP_REG_STATUS1_GPIO_EN_REV0_MASK
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> 
> (...)
> > +static int qpnp_conv_to_pin(struct qpnp_pinctrl *qctrl,
> > +			   struct qpnp_padinfo *pad, unsigned param,
> > +			   unsigned val)
> (...)
> > +	switch (param) {
> (...)
> > +	case PIN_CONFIG_OUTPUT:
> > +		nattrs = 3;
> > +		attr[0].addr  = QPNP_REG_MODE_CTL;
> > +		attr[0].shift = QPNP_REG_OUT_SRC_SEL_SHIFT;
> > +		attr[0].mask  = QPNP_REG_OUT_SRC_SEL_MASK;
> > +		attr[0].val   = !!val;
> 
> It seems that this patch provides no means to configure the output source
> select bits to be anything besides 0 (constant low) or 1 (constant high).
>  Some non-generic property is needed to configure this for both GPIOs and
> MPPs.  Passing the value in via the output-high property does not seem
> like a good approach since that is a generic pin config property that is
> defined to take no value. 

True.

>  The special functions available for GPIOs (e.g.
> PWM/LPG, clock, keypad, etc.) which are configured via this register are
> used by many boards.

I was not sure what those features are and how to connect the numbers to
the function, which is why I have restricted the values ​​of 0 and 1.

> 
> Something else to consider is that QPNP_REG_OUT_SRC_SEL_MASK is being
> defined as 0xf which would imply that there are 16 possible output source
> select options.  While technically true, this makes the situation more
> complicated since half of those options are the inverted version of the
> other half.  In the GPIO hardware this corresponds to an 8-way mux
> followed by an XOR gate to conditionally invert the mux output.  If output
> source select is handled this way, then the following values would need to
> be supported in device tree for GPIOs:
> 	* 0:  constant low (already supported via output-low;)
> 	* 1:  constant high (already supported via output-high;)
> 	* 2:  paired GPIO
> 	* 3:  inverted paired GPIO
> 	* 4:  special function 1
> 	* 5:  inverted special function 1
> 	* 6:  special function 2
> 	* 7:  inverted special function 2
> 	* 8:  dtest1
> 	* 9:  inverted dtest1
> 	* 10: dtest2
> 	* 11: inverted dtest2
> 	* 12: dtest3
> 	* 13: inverted dtest3
> 	* 14: dtest4
> 	* 15: inverted dtest4
> The same options are supported by MPPs except for special function 1,
> inverted special function 1, special function 2, and inverted special
> function 2.

<snip>

I am working on proposal from Stephen Boyd to encode GPIO/MPP mode and 
source select into combined function. Something like this one:

#define PM8XXX_DIGITAL_IN		0
#define PM8XXX_DIGITAL_OUT		1
#define PM8XXX_DIGITAL_IN_OUT		2

...

/* mode and source select */
#define PM8XXX_FUNCTION(m,s)		((m) << 16 | (s))

#define PM8921_GPIO1_14_KYPD_SNS	PM8XXX_FUNCTION(PM8XXX_DIGITAL_IN, 1)
#define PM8921_GPIO9_14_KYPD_DRV	PM8XXX_FUNCTION(PM8XXX_DIGITAL_OUT, 1)
#define PM8921_GPIO33_35_PWM		PM8XXX_FUNCTION(PM8XXX_DIGITAL_OUT, 3)

..

<snip>

> There is an off-by-one issue with the indexing between the hardware GPIO
> numbers (1-based) and the gpiolib gpio offsets (0-based).  Do you agree
> that the indexing used within the device tree gpiospecs should match the
> hardware numbering scheme?  I feel like this would be much less confusing
> for users to work with.  


Yep, will fix it. Thank you for review.

Regards,
Ivan

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[Stephen Boyd]

On 07/28/14 01:39, Ivan T. Ivanov wrote:
> I am working on proposal from Stephen Boyd to encode GPIO/MPP mode and 
> source select into combined function. Something like this one:
>
> #define PM8XXX_DIGITAL_IN		0
> #define PM8XXX_DIGITAL_OUT		1
> #define PM8XXX_DIGITAL_IN_OUT		2
>
> ...
>
> /* mode and source select */
> #define PM8XXX_FUNCTION(m,s)		((m) << 16 | (s))
>
> #define PM8921_GPIO1_14_KYPD_SNS	PM8XXX_FUNCTION(PM8XXX_DIGITAL_IN, 1)
> #define PM8921_GPIO9_14_KYPD_DRV	PM8XXX_FUNCTION(PM8XXX_DIGITAL_OUT, 1)
> #define PM8921_GPIO33_35_PWM		PM8XXX_FUNCTION(PM8XXX_DIGITAL_OUT, 3)
>
> ..
>
>

This isn't what I was suggesting at all. The function should be
something like KYPD, PWM and those should just be defined to be 1 or 3.
The mode should be some other property like input or output, and the
driver should or the two together and put it into the register.
Basically we shouldn't see any shifts or ors in the #defines, just
convenient numbers that correspond to the value of the field in the
register.

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

Re: [PATCH v2 2/4] pinctrl: qpnp: Qualcomm PMIC pin controller driver

[Ivan T. Ivanov]

On Mon, 2014-08-04 at 18:36 -0700, Stephen Boyd wrote:
> On 07/28/14 01:39, Ivan T. Ivanov wrote:
> > I am working on proposal from Stephen Boyd to encode GPIO/MPP mode and 
> > source select into combined function. Something like this one:
> >
> > #define PM8XXX_DIGITAL_IN		0
> > #define PM8XXX_DIGITAL_OUT		1
> > #define PM8XXX_DIGITAL_IN_OUT		2
> >
> > ...
> >
> > /* mode and source select */
> > #define PM8XXX_FUNCTION(m,s)		((m) << 16 | (s))
> >
> > #define PM8921_GPIO1_14_KYPD_SNS	PM8XXX_FUNCTION(PM8XXX_DIGITAL_IN, 1)
> > #define PM8921_GPIO9_14_KYPD_DRV	PM8XXX_FUNCTION(PM8XXX_DIGITAL_OUT, 1)
> > #define PM8921_GPIO33_35_PWM		PM8XXX_FUNCTION(PM8XXX_DIGITAL_OUT, 3)
> >
> > ..
> >
> >
> 
> This isn't what I was suggesting at all. The function should be
> something like KYPD, PWM and those should just be defined to be 1 or 3.
> The mode should be some other property like input or output, and the
> driver should or the two together and put it into the register.

Well, we can do this. I was just thought that will more convenient if
function is fully described in to one property.

Regards,
Ivan

[PATCH v2 3/4] pinctrl: qcom: Add documentation for pinctrl-qpnp driver bindings

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

DT binding documentation for qcom,pm8xxx-mpp pinctrl drivers.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 .../bindings/pinctrl/qcom,pm8xxx-mpp.txt           | 199 +++++++++++++++++++++
 1 file changed, 199 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-mpp.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-mpp.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-mpp.txt
new file mode 100644
index 0000000..a797d41
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-mpp.txt
@@ -0,0 +1,199 @@
+Qualcomm PM8XXX Multi-Purpose Pin (MPP) block
+
+This binding describes the MPP block(s) found in the 8xxx series of pmics from
+Qualcomm.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,pm8841-mpp"
+		    "qcom,pm8941-mpp"
+		    "qcom,pma8084-mpp"
+
+- reg:
+	Usage: required
+	Value type: <u32>
+	Definition: Register base of the gpio block
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Must contain an array of encoded interrupt specifiers for
+		    each available gpio
+
+- gpio-controller:
+	Usage: required
+	Value type: <none>
+	Definition: Mark the device node as a GPIO controller
+
+- #gpio-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: Must be 2;
+		    the first cell will be used to define gpio number and the
+		    second denotes the flags for this gpio
+
+Please refer to ../gpio/gpio.txt and ../interrupt-controller/interrupts.txt for
+a general description of GPIO and interrupt bindings.
+
+Please refer to pinctrl-bindings.txt in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning of the
+phrase "pin configuration node".
+
+The pin configuration nodes act as a container for an abitrary number of
+subnodes. Each of these subnodes represents some desired configuration for a
+pin or a list of pins. This configuration can include the
+mux function to select on those pin(s), and various pin configuration
+parameters, s listed below.
+
+
+SUBNODES:
+
+The name of each subnode is not important; all subnodes should be enumerated
+and processed purely based on their content.
+
+Each subnode only affects those parameters that are explicitly listed. In
+other words, a subnode that lists a mux function but no pin configuration
+parameters implies no information about any pin configuration parameters.
+Similarly, a pin subnode that describes a pullup parameter implies no
+information about e.g. the mux function.
+
+The following generic properties as defined in pinctrl-bindings.txt are valid
+to specify in a pin configuration subnode:
+
+- pins:
+	Usage: required
+	Value type: <string-array>
+	Definition: List of gpio pins affected by the properties specified in
+		    this subnode.  Valid pins are:
+			mpp1-mpp4 for pm8841
+			mpp1-mpp8 for pm8941
+			mpp1-mpp4 for pma8084
+
+- function:
+	Usage: mandatory
+	Value type: <string>
+	Definition: Specify the alternative function to be configured for the
+		    specified pins.  Valid values are:
+		    "gpio"  - digital input, output
+		    "ain"   - analog input
+		    "aout"  - analog output
+		    "cs"    - current sink
+
+- bias-disable:
+	Usage: optional
+	Value type: <none>
+	Definition: The specified pins should be configued as no pull.
+
+- bias-pull-up:
+	Usage: optional
+	Value type:  <u32>
+	Definition: The specified pins should be configued as pull up.
+			Valid values are 600, 10000 and 30000.
+
+- bias-high-impedance:
+	Usage: optional
+	Value type: <none>
+	Definition: The specified pins will put in high-Z mode and disabled.
+
+- input-enable:
+	Usage: optional
+	Value type: <none>
+	Definition: The specified pins are put in input mode.
+
+- output-high:
+	Usage: optional
+	Value type: <none>
+	Definition: The specified pins are configured in output mode, driven
+		    high.
+
+- output-low:
+	Usage: optional
+	Value type: <none>
+	Definition: The specified pins are configured in output mode, driven
+		    low.
+
+- power-source:
+	Usage: optional
+	Value type: <u32>
+	Definition: Selects the power source for the specified pins. Valid only
+			when pin operate in Digital I/O mode ("gpio"). Valid
+		    power sources are, as defined in
+		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
+			0: (PM8XXX_GPIO_VIN0)
+			1: (PM8XXX_GPIO_VIN1)
+			2: (PM8XXX_GPIO_VIN2)
+			3: (PM8XXX_GPIO_VIN3)
+			4: (PM8XXX_GPIO_VIN4)
+			5: (PM8XXX_GPIO_VIN5)
+			6: (PM8XXX_GPIO_VIN6)
+			7: (PM8XXX_GPIO_VIN7)
+
+- drive-strength:
+	Usage: optional
+	Value type: <u32>
+	Definition: Selects the drive strength for the specified pins. Value
+		    drive strengths are: 5-40mA with 5mA step
+
+- qcom,ain:
+	Usage: optional
+	Value type: <u32>
+	Definition: Selects the source for analog input. Valid values as
+			defined in <dt-bindings/pinctrl/qcom,pm8xxx-mpp.h>:
+			0: (PM8XXX_MPP_AIN_CH5)
+			1: (PM8XXX_MPP_AIN_CH6)
+			2: (PM8XXX_MPP_AIN_CH7)
+			3: (PM8XXX_MPP_AIN_CH8)
+			4: (PM8XXX_MPP_AIN_ABUS1)
+			5: (PM8XXX_MPP_AIN_ABUS2)
+			6: (PM8XXX_MPP_AIN_ABUS3)
+			7: (PM8XXX_MPP_AIN_ABUS4)
+
+- qcom,aout:
+	Usage: optional
+	Value type: <u32>
+	Definition: Selects the analog output reference. Valid values as
+			defined in <dt-bindings/pinctrl/qcom,pm8xxx-mpp.h>:
+			0: (PM8XXX_MPP_AOUT_1V25)
+			1: (PM8XXX_MPP_AOUT_0V625)
+			2: (PM8XXX_MPP_AOUT_0V3125)
+			3: (PM8XXX_MPP_AOUT_MPP)
+			4: (PM8XXX_MPP_AOUT_ABUS1)
+			5: (PM8XXX_MPP_AOUT_ABUS2)
+			6: (PM8XXX_MPP_AOUT_ABUS3)
+			7: (PM8XXX_MPP_AOUT_ABUS4)
+
+Example:
+
+	mpps@a000 {
+		compatible = "qcom,pm8841-mpp";
+		reg = <0xa000>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupts = <4 0xa0 0 0>, <4 0xa1 0 0>, <4 0xa2 0 0>, <4 0xa3 0 0>;
+
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&pm8841_default>;
+		pinctrl-1 = <&pm8841_sleep>;
+
+		pm8841_sleep: sleep {
+			gpio {
+				pins = "mpp1", "mpp2", "mpp3", "mpp4";
+				function = "gpio";
+
+				input-enable;
+
+				power-source = <PM8XXX_GPIO_VIN0>;
+				qcom,pull-up = <PM8XXX_GPIO_STRENGTH_NO>;
+				qcom,strength = <PM8XXX_GPIO_PULL_UP_30>;
+			};
+		};
+
+		pm8841_default: default {
+			ain {
+				pins = "mpp1", "mpp2", "mpp3", "mpp4";
+				function = "ain";
+			};
+		};
+	};
-- 
1.8.3.2

[PATCH v2 4/4] ARM: dts: qcom: Add PM8941 and PM8841 pinctrl nodes

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

Add nodes for PM8941 and PM8841 GPIO and MPP sub-functions.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 arch/arm/boot/dts/qcom-msm8974.dtsi | 61 +++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index c7ae7ba..b0164f9 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -6,6 +6,8 @@
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/reset/qcom,gcc-msm8974.h>
 #include <dt-bindings/spmi/spmi.h>
+#include <dt-bindings/pinctrl/qcom,pm8xxx-mpp.h>
+#include <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>
 
 / {
 	model = "Qualcomm MSM8974";
@@ -267,6 +269,33 @@
 					reg-names = "rtc", "alarm";
 					interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
 				};
+
+				gpios@c000 {
+					compatible = "qcom,pm8941-gpio";
+					reg = <0xc000>;
+					gpio-controller;
+					#gpio-cells = <2>;
+					label = "pm8941-gpio";
+					interrupts = <0 0xc0 0 0>, <0 0xc1 0 0>, <0 0xc2 0 0>, <0 0xc3 0 0>,
+						     <0 0xc4 0 0>, <0 0xc5 0 0>, <0 0xc6 0 0>, <0 0xc7 0 0>,
+						     <0 0xc8 0 0>, <0 0xc9 0 0>, <0 0xca 0 0>, <0 0xcb 0 0>,
+						     <0 0xcc 0 0>, <0 0xcd 0 0>, <0 0xce 0 0>, <0 0xcf 0 0>,
+						     <0 0xd0 0 0>, <0 0xd1 0 0>, <0 0xd2 0 0>, <0 0xd3 0 0>,
+						     <0 0xd4 0 0>, <0 0xd5 0 0>, <0 0xd6 0 0>, <0 0xd7 0 0>,
+						     <0 0xd8 0 0>, <0 0xd9 0 0>, <0 0xda 0 0>, <0 0xdb 0 0>,
+						     <0 0xdc 0 0>, <0 0xdd 0 0>, <0 0xde 0 0>, <0 0xdf 0 0>,
+						     <0 0xe0 0 0>, <0 0xe1 0 0>, <0 0xe2 0 0>, <0 0xe3 0 0>;
+				};
+
+				mpps@a000 {
+					compatible = "qcom,pm8941-mpp";
+					reg = <0xa000>;
+					gpio-controller;
+					#gpio-cells = <2>;
+					label = "pm8941-mpp";
+					interrupts = <0 0xa0 0 0>, <0 0xa1 0 0>, <0 0xa2 0 0>, <0 0xa3 0 0>,
+						     <0 0xa4 0 0>, <0 0xa5 0 0>, <0 0xa6 0 0>, <0 0xa7 0 0>;
+				};
 			};
 
 			usid1: pm8941@1 {
@@ -281,6 +310,38 @@
 				reg = <0x4 SPMI_USID>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+
+				mpps@a000 {
+					compatible = "qcom,pm8841-mpp";
+					reg = <0xa000>;
+					gpio-controller;
+					#gpio-cells = <2>;
+					interrupts = <4 0xa0 0 0>, <4 0xa1 0 0>, <4 0xa2 0 0>, <4 0xa3 0 0>;
+
+					pinctrl-names = "default", "sleep";
+					pinctrl-0 = <&pm8841_default>;
+					pinctrl-1 = <&pm8841_sleep>;
+
+					pm8841_sleep: sleep {
+						gpio {
+							pins = "mpp1", "mpp2", "mpp3", "mpp4";
+							function = "gpio";
+
+							input-enable;
+
+							power-source = <PM8XXX_GPIO_VIN0>;
+							qcom,pull-up = <PM8XXX_GPIO_STRENGTH_NO>;
+							qcom,strength = <PM8XXX_GPIO_PULL_UP_30>;
+						};
+					};
+
+					pm8841_default: default {
+						ain {
+							pins = "mpp1", "mpp2", "mpp3", "mpp4";
+							function = "ain";
+						};
+					};
+				};
 			};
 
 			usid5: pm8841@5 {
-- 
1.8.3.2

[PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

Available 'power-source' labels differ between chips.
Use just VIN0-VIN14 in the input source names.

PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
support only one function 'gpio'. Currently GPIO's will
support only 'normal' mode. Rest of the modes will be added
later, if needed.

We can not use generic drive-strength because Qualcomm hardware
define those values as low, medium and high. Use qcom,strength
for this.

We can not use generic bias-pull-up because Qualcomm hardware
define those values in uA's. Use qcom,pull-up for this.

Add qcom,pm8941-gpio and qcom,pma8084-gpio to chips, which
support these DT bindings.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 .../bindings/pinctrl/qcom,pm8xxx-gpio.txt          | 97 +++++++++++-----------
 drivers/pinctrl/pinctrl-pm8xxx-gpio.c              | 34 ++++----
 include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h     | 33 ++++----
 3 files changed, 81 insertions(+), 83 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
index 0035dd8..f17580a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
@@ -12,6 +12,8 @@ Qualcomm.
 			"qcom,pm8058-gpio"
 			"qcom,pm8917-gpio"
 			"qcom,pm8921-gpio"
+			"qcom,pm8941-gpio"
+			"qcom,pma8084-gpio"
 
 - reg:
 	Usage: required
@@ -74,20 +76,14 @@ to specify in a pin configuration subnode:
 			gpio1-gpio40 for pm8058
 			gpio1-gpio38 for pm8917
 			gpio1-gpio44 for pm8921
+			gpio1-gpio36 for pm8941
+			gpio1-gpio22 for pma8084
 
 - function:
-	Usage: optional
+	Usage: mandatory
 	Value type: <string>
 	Definition: Specify the alternative function to be configured for the
-		    specified pins.  Valid values are:
-			"normal",
-			"paired",
-			"func1",
-			"func2",
-			"dtest1",
-			"dtest2",
-			"dtest3",
-			"dtest4"
+		    specified pins.  Valid values is: "gpio"
 
 - bias-disable:
 	Usage: optional
@@ -99,18 +95,6 @@ to specify in a pin configuration subnode:
 	Value type: <none>
 	Definition: The specified pins should be configued as pull down.
 
-- bias-pull-up:
-	Usage: optional
-	Value type: <u32> (optional)
-	Definition: The specified pins should be configued as pull up. An
-		    optional argument can be used to configure the strength.
-		    Valid values are; as defined in
-		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
-		    1: 30uA			(PM8XXX_GPIO_PULL_UP_30)
-		    2: 1.5uA			(PM8XXX_GPIO_PULL_UP_1P5)
-		    3: 31.5uA			(PM8XXX_GPIO_PULL_UP_31P5)
-		    4: 1.5uA + 30uA boost	(PM8XXX_GPIO_PULL_UP_1P5_30)
-
 - bias-high-impedance:
 	Usage: optional
 	Value type: <none>
@@ -139,47 +123,37 @@ to specify in a pin configuration subnode:
 	Definition: Selects the power source for the specified pins. Valid
 		    power sources are, as defined in
 		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
-			0: bb (PM8XXX_GPIO_VIN_BB)
+			0: bb (PM8XXX_GPIO_VIN0)
 				valid for pm8038, pm8058, pm8917, pm8921
-			1: ldo2 (PM8XXX_GPIO_VIN_L2)
+			1: ldo2 (PM8XXX_GPIO_VIN1)
 				valid for pm8018, pm8038, pm8917,pm8921
-			2: ldo3 (PM8XXX_GPIO_VIN_L3)
+			2: ldo3 (PM8XXX_GPIO_VIN2)
 				valid for pm8038, pm8058, pm8917, pm8921
-			3: ldo4 (PM8XXX_GPIO_VIN_L4)
+			3: ldo4 (PM8XXX_GPIO_VIN3)
 				valid for pm8018, pm8917, pm8921
-			4: ldo5 (PM8XXX_GPIO_VIN_L5)
+			4: ldo5 (PM8XXX_GPIO_VIN4)
 				valid for pm8018, pm8058
-			5: ldo6 (PM8XXX_GPIO_VIN_L6)
+			5: ldo6 (PM8XXX_GPIO_VIN5)
 				valid for pm8018, pm8058
-			6: ldo7 (PM8XXX_GPIO_VIN_L7)
+			6: ldo7 (PM8XXX_GPIO_VIN6)
 				valid for pm8058
-			7: ldo8 (PM8XXX_GPIO_VIN_L8)
+			7: ldo8 (PM8XXX_GPIO_VIN7)
 				valid for pm8018
-			8: ldo11 (PM8XXX_GPIO_VIN_L11)
+			8: ldo11 (PM8XXX_GPIO_VIN8)
 				valid for pm8038
-			9: ldo14 (PM8XXX_GPIO_VIN_L14)
+			9: ldo14 (PM8XXX_GPIO_VIN9)
 				valid for pm8018
-			10: ldo15 (PM8XXX_GPIO_VIN_L15)
+			10: ldo15 (PM8XXX_GPIO_VIN10)
 				valid for pm8038, pm8917, pm8921
-			11: ldo17 (PM8XXX_GPIO_VIN_L17)
+			11: ldo17 (PM8XXX_GPIO_VIN11)
 				valid for pm8038, pm8917, pm8921
-			12: smps3 (PM8XXX_GPIO_VIN_S3)
+			12: smps3 (PM8XXX_GPIO_VIN12)
 				valid for pm8018, pm8058
-			13: smps4 (PM8XXX_GPIO_VIN_S4)
+			13: smps4 (PM8XXX_GPIO_VIN13)
 				valid for pm8921
-			14: vph (PM8XXX_GPIO_VIN_VPH)
+			14: vph (PM8XXX_GPIO_VIN14)
 				valid for pm8018, pm8038, pm8058, pm8917 pm8921
 
-- drive-strength:
-	Usage: optional
-	Value type: <u32>
-	Definition: Selects the drive strength for the specified pins. Value
-		    drive strengths are:
-			0: no	(PM8XXX_GPIO_STRENGTH_NO)
-			1: high	(PM8XXX_GPIO_STRENGTH_HIGH)
-			2: medium	(PM8XXX_GPIO_STRENGTH_MED)
-			3: low	(PM8XXX_GPIO_STRENGTH_LOW)
-
 - drive-push-pull:
 	Usage: optional
 	Value type: <none>
@@ -190,6 +164,28 @@ to specify in a pin configuration subnode:
 	Value type: <none>
 	Definition: The specified pins are configured in open-drain mode.
 
+- qcom,pull-up:
+	Usage: optional
+	Value type: <u32>
+	Definition: The specified pins should be configued as pull up. An
+		    optional argument can be used to configure the strength.
+		    Valid values are as defined in
+		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
+		    1: 30uA			(PM8XXX_GPIO_PULL_UP_30)
+		    2: 1.5uA			(PM8XXX_GPIO_PULL_UP_1P5)
+		    3: 31.5uA			(PM8XXX_GPIO_PULL_UP_31P5)
+		    4: 1.5uA + 30uA boost	(PM8XXX_GPIO_PULL_UP_1P5_30)
+
+- qcom,strength:
+	Usage: optional
+	Value type: <u32>
+	Definition: Selects the drive strength for the specified pins.
+		    Valid values are as defined in
+		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
+			0: no	(PM8XXX_GPIO_STRENGTH_NO)
+			1: high	(PM8XXX_GPIO_STRENGTH_HIGH)
+			2: medium	(PM8XXX_GPIO_STRENGTH_MED)
+			3: low	(PM8XXX_GPIO_STRENGTH_LOW)
 
 Example:
 
@@ -218,13 +214,14 @@ Example:
 		pm8921_gpio_keys: gpio-keys {
 			volume-keys {
 				pins = "gpio20", "gpio21";
-				function = "normal";
+				function = "gpio";
 
 				input-enable;
 				bias-pull-up;
 				drive-push-pull;
-				drive-strength = <PM8XXX_GPIO_STRENGTH_NO>;
-				power-source = <PM8XXX_GPIO_VIN_S4>;
+
+				power-source = <PM8XXX_GPIO_VIN13>;
+				qcom,strength = <PM8XXX_GPIO_STRENGTH_NO>;
 			};
 		};
 	};
diff --git a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
index 5aaf914..68feb2f 100644
--- a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
+++ b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
@@ -95,9 +95,7 @@ static const char * const pm8xxx_gpio_groups[PM8XXX_MAX_GPIOS] = {
 };
 
 static const char * const pm8xxx_gpio_functions[] = {
-	"normal", "paired",
-	"func1", "func2",
-	"dtest1", "dtest2", "dtest3", "dtest4",
+	"gpio",
 };
 
 static int pm8xxx_gpio_read(struct pm8xxx_gpio *pctrl, int pin, int bank)
@@ -622,9 +620,9 @@ static int pm8xxx_gpio_populate(struct pm8xxx_gpio *pctrl)
 static const struct pm8xxx_gpio_data pm8018_gpio_data = {
 	.ngpio = 6,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L14, PM8XXX_GPIO_VIN_S3,
-		PM8XXX_GPIO_VIN_L6, PM8XXX_GPIO_VIN_L2, PM8XXX_GPIO_VIN_L5,
-		PM8XXX_GPIO_VIN_L8, PM8XXX_GPIO_VIN_VPH
+		PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN9, PM8XXX_GPIO_VIN12,
+		PM8XXX_GPIO_VIN5, PM8XXX_GPIO_VIN1, PM8XXX_GPIO_VIN4,
+		PM8XXX_GPIO_VIN7, PM8XXX_GPIO_VIN14
 	},
 	.npower_sources = 8,
 };
@@ -632,9 +630,9 @@ static const struct pm8xxx_gpio_data pm8018_gpio_data = {
 static const struct pm8xxx_gpio_data pm8038_gpio_data = {
 	.ngpio = 12,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_L11,
-		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
-		PM8XXX_GPIO_VIN_L17
+		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN8,
+		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
+		PM8XXX_GPIO_VIN11
 	},
 	.npower_sources = 7,
 };
@@ -642,18 +640,18 @@ static const struct pm8xxx_gpio_data pm8038_gpio_data = {
 static const struct pm8xxx_gpio_data pm8058_gpio_data = {
 	.ngpio = 40,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S3,
-		PM8XXX_GPIO_VIN_L3, PM8XXX_GPIO_VIN_L7, PM8XXX_GPIO_VIN_L6,
-		PM8XXX_GPIO_VIN_L5, PM8XXX_GPIO_VIN_L2
+		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN12,
+		PM8XXX_GPIO_VIN2, PM8XXX_GPIO_VIN6, PM8XXX_GPIO_VIN5,
+		PM8XXX_GPIO_VIN4, PM8XXX_GPIO_VIN1
 	},
 	.npower_sources = 8,
 };
 static const struct pm8xxx_gpio_data pm8917_gpio_data = {
 	.ngpio = 38,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S4,
-		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
-		PM8XXX_GPIO_VIN_L17
+		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN13,
+		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
+		PM8XXX_GPIO_VIN11
 	},
 	.npower_sources = 7,
 };
@@ -661,9 +659,9 @@ static const struct pm8xxx_gpio_data pm8917_gpio_data = {
 static const struct pm8xxx_gpio_data pm8921_gpio_data = {
 	.ngpio = 44,
 	.power_sources = (int[]) {
-		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S4,
-		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
-		PM8XXX_GPIO_VIN_L17
+		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN13,
+		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
+		PM8XXX_GPIO_VIN11
 	},
 	.npower_sources = 7,
 };
diff --git a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
index 6b66fff..564fd05 100644
--- a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
+++ b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
@@ -5,27 +5,30 @@
 #ifndef _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
 #define _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
 
+/* To be used with "qcom,pull-up = <>" */
 #define PM8XXX_GPIO_PULL_UP_30		1
 #define PM8XXX_GPIO_PULL_UP_1P5		2
 #define PM8XXX_GPIO_PULL_UP_31P5	3
 #define PM8XXX_GPIO_PULL_UP_1P5_30	4
 
-#define PM8XXX_GPIO_VIN_BB		0
-#define PM8XXX_GPIO_VIN_L2		1
-#define PM8XXX_GPIO_VIN_L3		2
-#define PM8XXX_GPIO_VIN_L4		3
-#define PM8XXX_GPIO_VIN_L5		4
-#define PM8XXX_GPIO_VIN_L6		5
-#define PM8XXX_GPIO_VIN_L7		6
-#define PM8XXX_GPIO_VIN_L8		7
-#define PM8XXX_GPIO_VIN_L11		8
-#define PM8XXX_GPIO_VIN_L14		9
-#define PM8XXX_GPIO_VIN_L15		10
-#define PM8XXX_GPIO_VIN_L17		11
-#define PM8XXX_GPIO_VIN_S3		12
-#define PM8XXX_GPIO_VIN_S4		13
-#define PM8XXX_GPIO_VIN_VPH		14
+/* power-source */
+#define PM8XXX_GPIO_VIN0		0
+#define PM8XXX_GPIO_VIN1		1
+#define PM8XXX_GPIO_VIN2		2
+#define PM8XXX_GPIO_VIN3		3
+#define PM8XXX_GPIO_VIN4		4
+#define PM8XXX_GPIO_VIN5		5
+#define PM8XXX_GPIO_VIN6		6
+#define PM8XXX_GPIO_VIN7		7
+#define PM8XXX_GPIO_VIN8		8
+#define PM8XXX_GPIO_VIN9		9
+#define PM8XXX_GPIO_VIN10		10
+#define PM8XXX_GPIO_VIN11		11
+#define PM8XXX_GPIO_VIN12		12
+#define PM8XXX_GPIO_VIN13		13
+#define PM8XXX_GPIO_VIN14		14
 
+/* To be used with "qcom,strength = <>" */
 #define	PM8XXX_GPIO_STRENGTH_NO		0
 #define	PM8XXX_GPIO_STRENGTH_HIGH	1
 #define	PM8XXX_GPIO_STRENGTH_MED	2
-- 
1.8.3.2

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Ivan T. Ivanov]

On Thu, 2014-07-17 at 22:41 +0300, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> Available 'power-source' labels differ between chips.
> Use just VIN0-VIN14 in the input source names.
> 
> PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
> support only one function 'gpio'. Currently GPIO's will
> support only 'normal' mode. Rest of the modes will be added
> later, if needed.
> 
> We can not use generic drive-strength because Qualcomm hardware
> define those values as low, medium and high. Use qcom,strength
> for this.
> 
> We can not use generic bias-pull-up because Qualcomm hardware
> define those values in uA's. Use qcom,pull-up for this.
> 
> Add qcom,pm8941-gpio and qcom,pma8084-gpio to chips, which
> support these DT bindings.
> 

Hi Bjorn,  

Are you ok with these changes? I plan to send next version which
adds parsing code for new "qcom,strength" and "qcom,pull-up".

Regards,
Ivan


> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  .../bindings/pinctrl/qcom,pm8xxx-gpio.txt          | 97 +++++++++++-----------
>  drivers/pinctrl/pinctrl-pm8xxx-gpio.c              | 34 ++++----
>  include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h     | 33 ++++----
>  3 files changed, 81 insertions(+), 83 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
> index 0035dd8..f17580a 100644
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
> @@ -12,6 +12,8 @@ Qualcomm.
>  			"qcom,pm8058-gpio"
>  			"qcom,pm8917-gpio"
>  			"qcom,pm8921-gpio"
> +			"qcom,pm8941-gpio"
> +			"qcom,pma8084-gpio"
>  
>  - reg:
>  	Usage: required
> @@ -74,20 +76,14 @@ to specify in a pin configuration subnode:
>  			gpio1-gpio40 for pm8058
>  			gpio1-gpio38 for pm8917
>  			gpio1-gpio44 for pm8921
> +			gpio1-gpio36 for pm8941
> +			gpio1-gpio22 for pma8084
>  
>  - function:
> -	Usage: optional
> +	Usage: mandatory
>  	Value type: <string>
>  	Definition: Specify the alternative function to be configured for the
> -		    specified pins.  Valid values are:
> -			"normal",
> -			"paired",
> -			"func1",
> -			"func2",
> -			"dtest1",
> -			"dtest2",
> -			"dtest3",
> -			"dtest4"
> +		    specified pins.  Valid values is: "gpio"
>  
>  - bias-disable:
>  	Usage: optional
> @@ -99,18 +95,6 @@ to specify in a pin configuration subnode:
>  	Value type: <none>
>  	Definition: The specified pins should be configued as pull down.
>  
> -- bias-pull-up:
> -	Usage: optional
> -	Value type: <u32> (optional)
> -	Definition: The specified pins should be configued as pull up. An
> -		    optional argument can be used to configure the strength.
> -		    Valid values are; as defined in
> -		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> -		    1: 30uA			(PM8XXX_GPIO_PULL_UP_30)
> -		    2: 1.5uA			(PM8XXX_GPIO_PULL_UP_1P5)
> -		    3: 31.5uA			(PM8XXX_GPIO_PULL_UP_31P5)
> -		    4: 1.5uA + 30uA boost	(PM8XXX_GPIO_PULL_UP_1P5_30)
> -
>  - bias-high-impedance:
>  	Usage: optional
>  	Value type: <none>
> @@ -139,47 +123,37 @@ to specify in a pin configuration subnode:
>  	Definition: Selects the power source for the specified pins. Valid
>  		    power sources are, as defined in
>  		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> -			0: bb (PM8XXX_GPIO_VIN_BB)
> +			0: bb (PM8XXX_GPIO_VIN0)
>  				valid for pm8038, pm8058, pm8917, pm8921
> -			1: ldo2 (PM8XXX_GPIO_VIN_L2)
> +			1: ldo2 (PM8XXX_GPIO_VIN1)
>  				valid for pm8018, pm8038, pm8917,pm8921
> -			2: ldo3 (PM8XXX_GPIO_VIN_L3)
> +			2: ldo3 (PM8XXX_GPIO_VIN2)
>  				valid for pm8038, pm8058, pm8917, pm8921
> -			3: ldo4 (PM8XXX_GPIO_VIN_L4)
> +			3: ldo4 (PM8XXX_GPIO_VIN3)
>  				valid for pm8018, pm8917, pm8921
> -			4: ldo5 (PM8XXX_GPIO_VIN_L5)
> +			4: ldo5 (PM8XXX_GPIO_VIN4)
>  				valid for pm8018, pm8058
> -			5: ldo6 (PM8XXX_GPIO_VIN_L6)
> +			5: ldo6 (PM8XXX_GPIO_VIN5)
>  				valid for pm8018, pm8058
> -			6: ldo7 (PM8XXX_GPIO_VIN_L7)
> +			6: ldo7 (PM8XXX_GPIO_VIN6)
>  				valid for pm8058
> -			7: ldo8 (PM8XXX_GPIO_VIN_L8)
> +			7: ldo8 (PM8XXX_GPIO_VIN7)
>  				valid for pm8018
> -			8: ldo11 (PM8XXX_GPIO_VIN_L11)
> +			8: ldo11 (PM8XXX_GPIO_VIN8)
>  				valid for pm8038
> -			9: ldo14 (PM8XXX_GPIO_VIN_L14)
> +			9: ldo14 (PM8XXX_GPIO_VIN9)
>  				valid for pm8018
> -			10: ldo15 (PM8XXX_GPIO_VIN_L15)
> +			10: ldo15 (PM8XXX_GPIO_VIN10)
>  				valid for pm8038, pm8917, pm8921
> -			11: ldo17 (PM8XXX_GPIO_VIN_L17)
> +			11: ldo17 (PM8XXX_GPIO_VIN11)
>  				valid for pm8038, pm8917, pm8921
> -			12: smps3 (PM8XXX_GPIO_VIN_S3)
> +			12: smps3 (PM8XXX_GPIO_VIN12)
>  				valid for pm8018, pm8058
> -			13: smps4 (PM8XXX_GPIO_VIN_S4)
> +			13: smps4 (PM8XXX_GPIO_VIN13)
>  				valid for pm8921
> -			14: vph (PM8XXX_GPIO_VIN_VPH)
> +			14: vph (PM8XXX_GPIO_VIN14)
>  				valid for pm8018, pm8038, pm8058, pm8917 pm8921
>  
> -- drive-strength:
> -	Usage: optional
> -	Value type: <u32>
> -	Definition: Selects the drive strength for the specified pins. Value
> -		    drive strengths are:
> -			0: no	(PM8XXX_GPIO_STRENGTH_NO)
> -			1: high	(PM8XXX_GPIO_STRENGTH_HIGH)
> -			2: medium	(PM8XXX_GPIO_STRENGTH_MED)
> -			3: low	(PM8XXX_GPIO_STRENGTH_LOW)
> -
>  - drive-push-pull:
>  	Usage: optional
>  	Value type: <none>
> @@ -190,6 +164,28 @@ to specify in a pin configuration subnode:
>  	Value type: <none>
>  	Definition: The specified pins are configured in open-drain mode.
>  
> +- qcom,pull-up:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: The specified pins should be configued as pull up. An
> +		    optional argument can be used to configure the strength.
> +		    Valid values are as defined in
> +		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> +		    1: 30uA			(PM8XXX_GPIO_PULL_UP_30)
> +		    2: 1.5uA			(PM8XXX_GPIO_PULL_UP_1P5)
> +		    3: 31.5uA			(PM8XXX_GPIO_PULL_UP_31P5)
> +		    4: 1.5uA + 30uA boost	(PM8XXX_GPIO_PULL_UP_1P5_30)
> +
> +- qcom,strength:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: Selects the drive strength for the specified pins.
> +		    Valid values are as defined in
> +		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> +			0: no	(PM8XXX_GPIO_STRENGTH_NO)
> +			1: high	(PM8XXX_GPIO_STRENGTH_HIGH)
> +			2: medium	(PM8XXX_GPIO_STRENGTH_MED)
> +			3: low	(PM8XXX_GPIO_STRENGTH_LOW)
>  
>  Example:
>  
> @@ -218,13 +214,14 @@ Example:
>  		pm8921_gpio_keys: gpio-keys {
>  			volume-keys {
>  				pins = "gpio20", "gpio21";
> -				function = "normal";
> +				function = "gpio";
>  
>  				input-enable;
>  				bias-pull-up;
>  				drive-push-pull;
> -				drive-strength = <PM8XXX_GPIO_STRENGTH_NO>;
> -				power-source = <PM8XXX_GPIO_VIN_S4>;
> +
> +				power-source = <PM8XXX_GPIO_VIN13>;
> +				qcom,strength = <PM8XXX_GPIO_STRENGTH_NO>;
>  			};
>  		};
>  	};
> diff --git a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
> index 5aaf914..68feb2f 100644
> --- a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
> +++ b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
> @@ -95,9 +95,7 @@ static const char * const pm8xxx_gpio_groups[PM8XXX_MAX_GPIOS] = {
>  };
>  
>  static const char * const pm8xxx_gpio_functions[] = {
> -	"normal", "paired",
> -	"func1", "func2",
> -	"dtest1", "dtest2", "dtest3", "dtest4",
> +	"gpio",
>  };
>  
>  static int pm8xxx_gpio_read(struct pm8xxx_gpio *pctrl, int pin, int bank)
> @@ -622,9 +620,9 @@ static int pm8xxx_gpio_populate(struct pm8xxx_gpio *pctrl)
>  static const struct pm8xxx_gpio_data pm8018_gpio_data = {
>  	.ngpio = 6,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L14, PM8XXX_GPIO_VIN_S3,
> -		PM8XXX_GPIO_VIN_L6, PM8XXX_GPIO_VIN_L2, PM8XXX_GPIO_VIN_L5,
> -		PM8XXX_GPIO_VIN_L8, PM8XXX_GPIO_VIN_VPH
> +		PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN9, PM8XXX_GPIO_VIN12,
> +		PM8XXX_GPIO_VIN5, PM8XXX_GPIO_VIN1, PM8XXX_GPIO_VIN4,
> +		PM8XXX_GPIO_VIN7, PM8XXX_GPIO_VIN14
>  	},
>  	.npower_sources = 8,
>  };
> @@ -632,9 +630,9 @@ static const struct pm8xxx_gpio_data pm8018_gpio_data = {
>  static const struct pm8xxx_gpio_data pm8038_gpio_data = {
>  	.ngpio = 12,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_L11,
> -		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
> -		PM8XXX_GPIO_VIN_L17
> +		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN8,
> +		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
> +		PM8XXX_GPIO_VIN11
>  	},
>  	.npower_sources = 7,
>  };
> @@ -642,18 +640,18 @@ static const struct pm8xxx_gpio_data pm8038_gpio_data = {
>  static const struct pm8xxx_gpio_data pm8058_gpio_data = {
>  	.ngpio = 40,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S3,
> -		PM8XXX_GPIO_VIN_L3, PM8XXX_GPIO_VIN_L7, PM8XXX_GPIO_VIN_L6,
> -		PM8XXX_GPIO_VIN_L5, PM8XXX_GPIO_VIN_L2
> +		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN12,
> +		PM8XXX_GPIO_VIN2, PM8XXX_GPIO_VIN6, PM8XXX_GPIO_VIN5,
> +		PM8XXX_GPIO_VIN4, PM8XXX_GPIO_VIN1
>  	},
>  	.npower_sources = 8,
>  };
>  static const struct pm8xxx_gpio_data pm8917_gpio_data = {
>  	.ngpio = 38,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S4,
> -		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
> -		PM8XXX_GPIO_VIN_L17
> +		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN13,
> +		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
> +		PM8XXX_GPIO_VIN11
>  	},
>  	.npower_sources = 7,
>  };
> @@ -661,9 +659,9 @@ static const struct pm8xxx_gpio_data pm8917_gpio_data = {
>  static const struct pm8xxx_gpio_data pm8921_gpio_data = {
>  	.ngpio = 44,
>  	.power_sources = (int[]) {
> -		PM8XXX_GPIO_VIN_VPH, PM8XXX_GPIO_VIN_BB, PM8XXX_GPIO_VIN_S4,
> -		PM8XXX_GPIO_VIN_L15, PM8XXX_GPIO_VIN_L4, PM8XXX_GPIO_VIN_L3,
> -		PM8XXX_GPIO_VIN_L17
> +		PM8XXX_GPIO_VIN14, PM8XXX_GPIO_VIN0, PM8XXX_GPIO_VIN13,
> +		PM8XXX_GPIO_VIN10, PM8XXX_GPIO_VIN3, PM8XXX_GPIO_VIN2,
> +		PM8XXX_GPIO_VIN11
>  	},
>  	.npower_sources = 7,
>  };
> diff --git a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
> index 6b66fff..564fd05 100644
> --- a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
> +++ b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
> @@ -5,27 +5,30 @@
>  #ifndef _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
>  #define _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
>  
> +/* To be used with "qcom,pull-up = <>" */
>  #define PM8XXX_GPIO_PULL_UP_30		1
>  #define PM8XXX_GPIO_PULL_UP_1P5		2
>  #define PM8XXX_GPIO_PULL_UP_31P5	3
>  #define PM8XXX_GPIO_PULL_UP_1P5_30	4
>  
> -#define PM8XXX_GPIO_VIN_BB		0
> -#define PM8XXX_GPIO_VIN_L2		1
> -#define PM8XXX_GPIO_VIN_L3		2
> -#define PM8XXX_GPIO_VIN_L4		3
> -#define PM8XXX_GPIO_VIN_L5		4
> -#define PM8XXX_GPIO_VIN_L6		5
> -#define PM8XXX_GPIO_VIN_L7		6
> -#define PM8XXX_GPIO_VIN_L8		7
> -#define PM8XXX_GPIO_VIN_L11		8
> -#define PM8XXX_GPIO_VIN_L14		9
> -#define PM8XXX_GPIO_VIN_L15		10
> -#define PM8XXX_GPIO_VIN_L17		11
> -#define PM8XXX_GPIO_VIN_S3		12
> -#define PM8XXX_GPIO_VIN_S4		13
> -#define PM8XXX_GPIO_VIN_VPH		14
> +/* power-source */
> +#define PM8XXX_GPIO_VIN0		0
> +#define PM8XXX_GPIO_VIN1		1
> +#define PM8XXX_GPIO_VIN2		2
> +#define PM8XXX_GPIO_VIN3		3
> +#define PM8XXX_GPIO_VIN4		4
> +#define PM8XXX_GPIO_VIN5		5
> +#define PM8XXX_GPIO_VIN6		6
> +#define PM8XXX_GPIO_VIN7		7
> +#define PM8XXX_GPIO_VIN8		8
> +#define PM8XXX_GPIO_VIN9		9
> +#define PM8XXX_GPIO_VIN10		10
> +#define PM8XXX_GPIO_VIN11		11
> +#define PM8XXX_GPIO_VIN12		12
> +#define PM8XXX_GPIO_VIN13		13
> +#define PM8XXX_GPIO_VIN14		14
>  
> +/* To be used with "qcom,strength = <>" */
>  #define	PM8XXX_GPIO_STRENGTH_NO		0
>  #define	PM8XXX_GPIO_STRENGTH_HIGH	1
>  #define	PM8XXX_GPIO_STRENGTH_MED	2

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Bjorn Andersson]

On Thu, Jul 17, 2014 at 12:41 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
>

Hi Ivan,

Sorry for the slow response, I wanted to respin my pm8xxx-gpio driver to figure
out some resonable answers to you.

> Available 'power-source' labels differ between chips.
> Use just VIN0-VIN14 in the input source names.
>

The documentation uses VIN0-VIN7 to define the actual register value 0-7. As
with most other things in DT we don't want to encode the actual bits that
should go in the register, but rather give them some human readable name. This
is why I have the mapping tables in my proposal.

Inventing some magic mapping where you're supposed to know that you should type
VIN14 when you mean VPH on pm8921 but VIN0 on pm8941 doesn't make sense.

So, either we put the actual register values in the binding, or we use the enum
(as I proposed) to encode human readable names.

For pm8941 the valid power supply values are:
 GPIO 1-14
   0: VPH
   2: SMPS3
   3: LDO6

 GPIO 15-18
  2: SMPS3
  3: LDO6

 GPIO 19-36
  0: VPH
  1: VDD_TORCH
  2: SPMS3
  3: LDO6

 MPP 1-8
  0: VPH
  1: LDO1
  2: SPMS3
  3: LDO6

For pma8084 the valid power supply values are:
 GPIO 1-22
  0: VPH
  2: SPMS4
  3: LDO6

 MPP 1-8
  0: VPH
  1: LDO1
  2: SMPS4
  3: LDO6

Please add these constants to the table of valid power-source values and use
something like I did to translate them to register values - it makes the DT
much more readable.

> PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
> support only one function 'gpio'. Currently GPIO's will
> support only 'normal' mode. Rest of the modes will be added
> later, if needed.
>

This is not true.

As I said before, there is no such thing as "pin controller hardware"; both on
pm8xxx and qpnp-pin there are two different HW blocks, one for GPIO and one for
MPP. And if you look in your pinconf_set function you will see that they are
very different.

I'm still trying to figure out the correct pinmux mapping for the various
pmics, but the current indication is a list that looks like this:
  "gpio"
  "paired"
  "ext_reg_en"
  "ext_smps_en"
  "fclk"
  "kypd_drv"
  "kypd_sns"
  "lpa"
  "lpg"
  "mp3_clk"
  "sleep_clk"
  "uart"
  "uim"
  "upl"

I haven't looked through the dts files for 8974 and 8084, but it's not possible
to describe the previous Qualcomm reference formfactor devices (MTP) with only
"gpio".

> We can not use generic drive-strength because Qualcomm hardware
> define those values as low, medium and high. Use qcom,strength
> for this.
>
> We can not use generic bias-pull-up because Qualcomm hardware
> define those values in uA's. Use qcom,pull-up for this.
>

I'm not to fond of the lack of symetry we introduce by having "bias-disable",
"bias-pull-down" and "qcom,pull-up". Furhter more, at least for 8x60, 8960 and
8064 almost all pins are configured with 30uA.

So my suggestion is that we keep the symmetry by continue to select the pull up
mode by the use of "bias-pull-up" and then we introduce a property named
"qcom,pull-up-strength" that optionally can be used to select a different
strength than the 30uA.

[...]
>  - function:
> -       Usage: optional
> +       Usage: mandatory

Usage: required

>         Value type: <string>
>         Definition: Specify the alternative function to be configured for the
> -                   specified pins.  Valid values are:
> -                       "normal",
> -                       "paired",
> -                       "func1",
> -                       "func2",
> -                       "dtest1",
> -                       "dtest2",
> -                       "dtest3",
> -                       "dtest4"
> +                   specified pins.  Valid values is: "gpio"
>
>  - bias-disable:
>         Usage: optional
> @@ -99,18 +95,6 @@ to specify in a pin configuration subnode:
>         Value type: <none>
>         Definition: The specified pins should be configued as pull down.
>
> -- bias-pull-up:
> -       Usage: optional
> -       Value type: <u32> (optional)
> -       Definition: The specified pins should be configued as pull up. An
> -                   optional argument can be used to configure the strength.
> -                   Valid values are; as defined in
> -                   <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> -                   1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
> -                   2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
> -                   3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
> -                   4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
> -

As described above, I belive we should make this:

- bias-pull-up:
	Usage: optional
	Value type: <empty>
	Definition: The specified pins should be configured as pull up.

- qcom,pull-up-strength:
	Usage: optional
	Value type: <u32>
	Definition: Specifies the strength to use for pull up, if selected.
                    Valid values are; as defined in
                    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
                    1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
                    2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
                    3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
                    4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
		    If this property is ommited 30uA strength will be used if
		    pull up is selected.

>  - bias-high-impedance:
>         Usage: optional
>         Value type: <none>
> @@ -139,47 +123,37 @@ to specify in a pin configuration subnode:
>         Definition: Selects the power source for the specified pins. Valid
>                     power sources are, as defined in
>                     <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> -                       0: bb (PM8XXX_GPIO_VIN_BB)
> +                       0: bb (PM8XXX_GPIO_VIN0)
>                                 valid for pm8038, pm8058, pm8917, pm8921
> -                       1: ldo2 (PM8XXX_GPIO_VIN_L2)
> +                       1: ldo2 (PM8XXX_GPIO_VIN1)
>                                 valid for pm8018, pm8038, pm8917,pm8921
> -                       2: ldo3 (PM8XXX_GPIO_VIN_L3)
> +                       2: ldo3 (PM8XXX_GPIO_VIN2)
>                                 valid for pm8038, pm8058, pm8917, pm8921
> -                       3: ldo4 (PM8XXX_GPIO_VIN_L4)
> +                       3: ldo4 (PM8XXX_GPIO_VIN3)
>                                 valid for pm8018, pm8917, pm8921
> -                       4: ldo5 (PM8XXX_GPIO_VIN_L5)
> +                       4: ldo5 (PM8XXX_GPIO_VIN4)
>                                 valid for pm8018, pm8058
> -                       5: ldo6 (PM8XXX_GPIO_VIN_L6)
> +                       5: ldo6 (PM8XXX_GPIO_VIN5)
>                                 valid for pm8018, pm8058
> -                       6: ldo7 (PM8XXX_GPIO_VIN_L7)
> +                       6: ldo7 (PM8XXX_GPIO_VIN6)
>                                 valid for pm8058
> -                       7: ldo8 (PM8XXX_GPIO_VIN_L8)
> +                       7: ldo8 (PM8XXX_GPIO_VIN7)
>                                 valid for pm8018
> -                       8: ldo11 (PM8XXX_GPIO_VIN_L11)
> +                       8: ldo11 (PM8XXX_GPIO_VIN8)
>                                 valid for pm8038
> -                       9: ldo14 (PM8XXX_GPIO_VIN_L14)
> +                       9: ldo14 (PM8XXX_GPIO_VIN9)
>                                 valid for pm8018
> -                       10: ldo15 (PM8XXX_GPIO_VIN_L15)
> +                       10: ldo15 (PM8XXX_GPIO_VIN10)
>                                 valid for pm8038, pm8917, pm8921
> -                       11: ldo17 (PM8XXX_GPIO_VIN_L17)
> +                       11: ldo17 (PM8XXX_GPIO_VIN11)
>                                 valid for pm8038, pm8917, pm8921
> -                       12: smps3 (PM8XXX_GPIO_VIN_S3)
> +                       12: smps3 (PM8XXX_GPIO_VIN12)
>                                 valid for pm8018, pm8058
> -                       13: smps4 (PM8XXX_GPIO_VIN_S4)
> +                       13: smps4 (PM8XXX_GPIO_VIN13)
>                                 valid for pm8921
> -                       14: vph (PM8XXX_GPIO_VIN_VPH)
> +                       14: vph (PM8XXX_GPIO_VIN14)
>                                 valid for pm8018, pm8038, pm8058, pm8917 pm8921

As described this doesn't make sense, please add the necessary enumeration for
your pins or make an argument for just using register values directly here.

If we choose to go with register values directly in the dt binding we should
document the valid values and their mapping/meaning here.

>
> -- drive-strength:
> -       Usage: optional
> -       Value type: <u32>
> -       Definition: Selects the drive strength for the specified pins. Value
> -                   drive strengths are:
> -                       0: no   (PM8XXX_GPIO_STRENGTH_NO)
> -                       1: high (PM8XXX_GPIO_STRENGTH_HIGH)
> -                       2: medium       (PM8XXX_GPIO_STRENGTH_MED)
> -                       3: low  (PM8XXX_GPIO_STRENGTH_LOW)
> -
>  - drive-push-pull:
>         Usage: optional
>         Value type: <none>
> @@ -190,6 +164,28 @@ to specify in a pin configuration subnode:
>         Value type: <none>
>         Definition: The specified pins are configured in open-drain mode.
>
> +- qcom,pull-up:
> +       Usage: optional
> +       Value type: <u32>
> +       Definition: The specified pins should be configued as pull up. An
> +                   optional argument can be used to configure the strength.
> +                   Valid values are as defined in
> +                   <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> +                   1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
> +                   2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
> +                   3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
> +                   4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
> +
> +- qcom,strength:

Better follow the standard naming and use "qcom,drive-strength" to actually
specify what strength we're talking about.

> +       Usage: optional
> +       Value type: <u32>
> +       Definition: Selects the drive strength for the specified pins.
> +                   Valid values are as defined in
> +                   <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
> +                       0: no   (PM8XXX_GPIO_STRENGTH_NO)
> +                       1: high (PM8XXX_GPIO_STRENGTH_HIGH)
To be clearer what these actually means we could probably add the following:
				0.9mA @ 1.8V - 1.9mA @ 2.6V
> +                       2: medium       (PM8XXX_GPIO_STRENGTH_MED)
				0.6mA @ 1.8V - 1.25mA @ 2.6V
> +                       3: low  (PM8XXX_GPIO_STRENGTH_LOW)
				0.15mA @ 1.8V - 0.3mA @ 2.6V
>
>  Example:
>
> @@ -218,13 +214,14 @@ Example:
>                 pm8921_gpio_keys: gpio-keys {
>                         volume-keys {
>                                 pins = "gpio20", "gpio21";
> -                               function = "normal";
> +                               function = "gpio";

Sounds reasonable.

>
>                                 input-enable;
>                                 bias-pull-up;
>                                 drive-push-pull;
> -                               drive-strength = <PM8XXX_GPIO_STRENGTH_NO>;
> -                               power-source = <PM8XXX_GPIO_VIN_S4>;
> +
> +                               power-source = <PM8XXX_GPIO_VIN13>;

Here you can see why VIN13 doesn't make any sense; anyone writing a dts for
this would expect this to either be <VIN_S4> or <2>.

> +                               qcom,strength = <PM8XXX_GPIO_STRENGTH_NO>;
>                         };
>                 };
>         };

Please let me know what you think and I can send out an updated version of my
binding documentation.

Regards,
Bjorn

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Ivan T. Ivanov]

On Tue, 2014-07-22 at 14:46 -0700, Bjorn Andersson wrote:
> On Thu, Jul 17, 2014 at 12:41 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> >
> 
> Hi Ivan,
> 
> Sorry for the slow response, I wanted to respin my pm8xxx-gpio driver to figure
> out some resonable answers to you.
> 

<snip>

> 
> > PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
> > support only one function 'gpio'. Currently GPIO's will
> > support only 'normal' mode. Rest of the modes will be added
> > later, if needed.
> >
> 
> This is not true.
> 
> As I said before, there is no such thing as "pin controller hardware"; both on
> pm8xxx and qpnp-pin there are two different HW blocks, one for GPIO and one for
> MPP. And if you look in your pinconf_set function you will see that they are
> very different.
> 
> I'm still trying to figure out the correct pinmux mapping for the various
> pmics, but the current indication is a list that looks like this:
>   "gpio"
>   "paired"
>   "ext_reg_en"
>   "ext_smps_en"
>   "fclk"
>   "kypd_drv"
>   "kypd_sns"
>   "lpa"
>   "lpg"
>   "mp3_clk"
>   "sleep_clk"
>   "uart"
>   "uim"
>   "upl"
> 
> I haven't looked through the dts files for 8974 and 8084, but it's not possible
> to describe the previous Qualcomm reference formfactor devices (MTP) with only
> "gpio".

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Ivan T. Ivanov]

Hi,  

I have accidentally pressed send in the earlier message.

On Wed, 2014-07-23 at 15:47 +0300, Ivan T. Ivanov wrote:
> On Tue, 2014-07-22 at 14:46 -0700, Bjorn Andersson wrote:
> > On Thu, Jul 17, 2014 at 12:41 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > >
> > 
> > Hi Ivan,
> > 
> > Sorry for the slow response, I wanted to respin my pm8xxx-gpio driver to figure
> > out some resonable answers to you.
> > 
> 
> <snip>
> 
> > 
> > > PM8018, PM8038, PM8058, PM8917, PM8921 pin controller hardware
> > > support only one function 'gpio'. Currently GPIO's will
> > > support only 'normal' mode. Rest of the modes will be added
> > > later, if needed.
> > >
> > 
> > This is not true.
> > 
> > As I said before, there is no such thing as "pin controller hardware"; 

This is matter of interpretation :-)

> both on
> > pm8xxx and qpnp-pin there are two different HW blocks, one for GPIO and one for
> > MPP. And if you look in your pinconf_set function you will see that they are
> > very different.

I bet that the hardware blocks are almost identical, just register
map is different.

> > 
> > I'm still trying to figure out the correct pinmux mapping for the various
> > pmics, but the current indication is a list that looks like this:
> >   "gpio"
> >   "paired"
> >   "ext_reg_en"
> >   "ext_smps_en"
> >   "fclk"
> >   "kypd_drv"
> >   "kypd_sns"
> >   "lpa"
> >   "lpg"
> >   "mp3_clk"
> >   "sleep_clk"
> >   "uart"
> >   "uim"
> >   "upl"
> > 

Ok, I see. But lets make things simpler. 

It seems that "uim" could be "SDC_UIM_VBIAS" on DB8074, which looks like MPP in
analog-output mode/function, with additional selection for voltage level (qcom,aout?)

Closest to "uart" function, that I have found, is that one of the SPI buses on the
above board is level translated trough several MPP's, but this still did not make them
act like a UART.

However, I seems that some of the GPIO's could act like clock and PWM sources. 

> > I haven't looked through the dts files for 8974 and 8084, but it's not possible
> > to describe the previous Qualcomm reference formfactor devices (MTP) with only
> > "gpio".

I believe that these DTS files are using qcom,mode(DIG_IN|DIG_OUT|AIN|AOUT..) and 
qcom,src-sel to select desired function. For example:

apq8074-dragonboard:
/* GPIO 1 */
qcom,mode = <0>
qcom,src-sel = <0>

apq8084-cdp:
qcom,mode = <1>;                /* Digital output */
qcom,src-sel = <2>;             /* Special Function 1=LPG 3 */

apq8084-mtp:
/* NFC clk request */
qcom,mode = <0>;                /* QPNP_PIN_MODE_DIG_IN */
qcom,src-sel = <2>;             /* QPNP_PIN_SEL_FUNC_1 */

apq8084-sbc:
/* BACKLIGHT2_PWM */
qcom,mode = <1>;                /* Digital output */
qcom,src-sel = <2>;             /* Special Function 1=LPG 5 */

apq8084-sbc:
/* DIV_CLK3 SLEEP_CLK */
qcom,mode = <1>;                /* Digital output */
qcom,src-sel = <3>;             /* Function 2 */

...

I could just guess what the functions 1 and 2 means.

In which case GPIO functions could be gpio, keypad, clk, pwm/lpg, ...

Rest of the pinctrl parameters prosed looks fine. 

Thanks,
Ivan

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Stephen Boyd]

On 07/23/14 09:05, Ivan T. Ivanov wrote:
>
>> both on
>>> pm8xxx and qpnp-pin there are two different HW blocks, one for GPIO and one for
>>> MPP. And if you look in your pinconf_set function you will see that they are
>>> very different.
> I bet that the hardware blocks are almost identical, just register
> map is different.

The hardware is different and not "almost identical". If it was the same
then we would have called them all GPIOs or MPPs and not had a distinction.

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

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Stephen Boyd]

On 07/22/14 14:46, Bjorn Andersson wrote:
> For pm8941 the valid power supply values are:
>  GPIO 1-14
>    0: VPH
>    2: SMPS3
>    3: LDO6
>
>  GPIO 15-18
>   2: SMPS3
>   3: LDO6
>
>  GPIO 19-36
>   0: VPH
>   1: VDD_TORCH
>   2: SPMS3
>   3: LDO6
>
>  MPP 1-8
>   0: VPH
>   1: LDO1
>   2: SPMS3
>   3: LDO6
>
> For pma8084 the valid power supply values are:
>  GPIO 1-22
>   0: VPH
>   2: SPMS4
>   3: LDO6
>
>  MPP 1-8
>   0: VPH
>   1: LDO1
>   2: SMPS4
>   3: LDO6
>
> Please add these constants to the table of valid power-source values and use
> something like I did to translate them to register values - it makes the DT
> much more readable.

The DT could be similarly readable if we had a bunch of #defines for the
different VIN settings that resolved to the final register value for
that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
etc. There would be a lot of them, but then the driver could be really
simple and just jam whatever value is in the DT into the register
without having to bounce through a mapping table in software to figure
out the register value. If we did this for the functions also then I
believe we achieve readability without requiring a bunch of drivers for
each and every single pmic?

>
>>         Value type: <string>
>>         Definition: Specify the alternative function to be configured for the
>> -                   specified pins.  Valid values are:
>> -                       "normal",
>> -                       "paired",
>> -                       "func1",
>> -                       "func2",
>> -                       "dtest1",
>> -                       "dtest2",
>> -                       "dtest3",
>> -                       "dtest4"
>> +                   specified pins.  Valid values is: "gpio"
>>
>>  - bias-disable:
>>         Usage: optional
>> @@ -99,18 +95,6 @@ to specify in a pin configuration subnode:
>>         Value type: <none>
>>         Definition: The specified pins should be configued as pull down.
>>
>> -- bias-pull-up:
>> -       Usage: optional
>> -       Value type: <u32> (optional)
>> -       Definition: The specified pins should be configued as pull up. An
>> -                   optional argument can be used to configure the strength.
>> -                   Valid values are; as defined in
>> -                   <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
>> -                   1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
>> -                   2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
>> -                   3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
>> -                   4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
>> -
> As described above, I belive we should make this:
>
> - bias-pull-up:
> 	Usage: optional
> 	Value type: <empty>
> 	Definition: The specified pins should be configured as pull up.
>
> - qcom,pull-up-strength:
> 	Usage: optional
> 	Value type: <u32>
> 	Definition: Specifies the strength to use for pull up, if selected.
>                     Valid values are; as defined in
>                     <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
>                     1: 30uA                     (PM8XXX_GPIO_PULL_UP_30)
>                     2: 1.5uA                    (PM8XXX_GPIO_PULL_UP_1P5)
>                     3: 31.5uA                   (PM8XXX_GPIO_PULL_UP_31P5)
>                     4: 1.5uA + 30uA boost       (PM8XXX_GPIO_PULL_UP_1P5_30)
> 		    If this property is ommited 30uA strength will be used if
> 		    pull up is selected.

Why is 30uA special? Just because most drivers use it? I'd prefer we
always be explicit about which pull-up we want so that nothing is left
up to the driver implementation.

Also according to the hw folks the 1.5uA + 30uA boost has never been
used so I say let's drop that feature. If we need it one day we can
always add a qcom,pull-up-boost or something (I highly doubt we'll need
it). Doing that allows us to specify this in actual SI units. Maybe even
allowing us to have a generic pull-up-strength (or
bias-pull-up-strength) specified in SI units of mA?

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

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Linus Walleij]

On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

>> Please add these constants to the table of valid power-source values and use
>> something like I did to translate them to register values - it makes the DT
>> much more readable.
>
> The DT could be similarly readable if we had a bunch of #defines for the
> different VIN settings that resolved to the final register value for
> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
> etc. There would be a lot of them, but then the driver could be really
> simple and just jam whatever value is in the DT into the register
> without having to bounce through a mapping table in software to figure
> out the register value. If we did this for the functions also then I
> believe we achieve readability without requiring a bunch of drivers for
> each and every single pmic?

Not sure but it sounds like you want to make the device tree a jam table,
(know about individual register offsets, sequences etc). That has been
throrougly NACKed in the past, because DT is not Open Firmware.

The exception is pinctrl-single which is restricted to single register
per pin use cases and is still a point of contention...

Yours,
Linus Walleij

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Stephen Boyd]

On 07/24/14 08:40, Linus Walleij wrote:
> On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
>>> Please add these constants to the table of valid power-source values and use
>>> something like I did to translate them to register values - it makes the DT
>>> much more readable.
>> The DT could be similarly readable if we had a bunch of #defines for the
>> different VIN settings that resolved to the final register value for
>> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
>> etc. There would be a lot of them, but then the driver could be really
>> simple and just jam whatever value is in the DT into the register
>> without having to bounce through a mapping table in software to figure
>> out the register value. If we did this for the functions also then I
>> believe we achieve readability without requiring a bunch of drivers for
>> each and every single pmic?
> Not sure but it sounds like you want to make the device tree a jam table,
> (know about individual register offsets, sequences etc). That has been
> throrougly NACKed in the past, because DT is not Open Firmware.
>
> The exception is pinctrl-single which is restricted to single register
> per pin use cases and is still a point of contention...
>

I'm not proposing a jam table. I'm proposing that we make the
function/source property convenient to the driver by having the actual
function field value encoded there instead of some string that has to be
translated through a table in a driver. There's still going to be
shifting and masking of bits in the driver to put the value in the right
place in the register, but we avoid needing N number of drivers for each
pmic just to translate strings into integers (for functions) and
integers into other integers (for the power source). From what I can
tell there isn't any benefit to having the function property be a string
vs. a #define number besides having a human readable string in pinctrl
debugfs. Is there some other benefit?

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

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Linus Walleij]

On Fri, Jul 25, 2014 at 2:23 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 07/24/14 08:40, Linus Walleij wrote:
>> On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>>>> Please add these constants to the table of valid power-source values and use
>>>> something like I did to translate them to register values - it makes the DT
>>>> much more readable.
>>> The DT could be similarly readable if we had a bunch of #defines for the
>>> different VIN settings that resolved to the final register value for
>>> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
>>> etc. There would be a lot of them, but then the driver could be really
>>> simple and just jam whatever value is in the DT into the register
>>> without having to bounce through a mapping table in software to figure
>>> out the register value. If we did this for the functions also then I
>>> believe we achieve readability without requiring a bunch of drivers for
>>> each and every single pmic?
>> Not sure but it sounds like you want to make the device tree a jam table,
>> (know about individual register offsets, sequences etc). That has been
>> throrougly NACKed in the past, because DT is not Open Firmware.
>>
>> The exception is pinctrl-single which is restricted to single register
>> per pin use cases and is still a point of contention...
>>
>
> I'm not proposing a jam table. I'm proposing that we make the
> function/source property convenient to the driver by having the actual
> function field value encoded there instead of some string that has to be
> translated through a table in a driver. There's still going to be
> shifting and masking of bits in the driver to put the value in the right
> place in the register, but we avoid needing N number of drivers for each
> pmic just to translate strings into integers (for functions) and
> integers into other integers (for the power source). From what I can
> tell there isn't any benefit to having the function property be a string
> vs. a #define number besides having a human readable string in pinctrl
> debugfs. Is there some other benefit?

OK I get it ... I think.

One good reason to use strings is that it apart from debugfs makes
for quite readable debug messages if used the right way.

I feel sort of lukewarm on the issue, so I'd let the driver author
decide the most elegant way to deal with this from an end-user
point of view. If it helps people configure and debug their board
set-ups is a crucial factor to me, and I suspect both Ivan and
Björn has some experience with this.

Yours,
Linus Walleij

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Ivan T. Ivanov]

On Thu, 2014-07-24 at 17:23 -0700, Stephen Boyd wrote:
> On 07/24/14 08:40, Linus Walleij wrote:
> > On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >
> >>> Please add these constants to the table of valid power-source values and use
> >>> something like I did to translate them to register values - it makes the DT
> >>> much more readable.
> >> The DT could be similarly readable if we had a bunch of #defines for the
> >> different VIN settings that resolved to the final register value for
> >> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
> >> etc. There would be a lot of them, but then the driver could be really
> >> simple and just jam whatever value is in the DT into the register
> >> without having to bounce through a mapping table in software to figure
> >> out the register value. If we did this for the functions also then I
> >> believe we achieve readability without requiring a bunch of drivers for
> >> each and every single pmic?
> > Not sure but it sounds like you want to make the device tree a jam table,
> > (know about individual register offsets, sequences etc). That has been
> > throrougly NACKed in the past, because DT is not Open Firmware.
> >
> > The exception is pinctrl-single which is restricted to single register
> > per pin use cases and is still a point of contention...
> >
> 
> I'm not proposing a jam table. I'm proposing that we make the
> function/source property convenient to the driver by having the actual
> function field value encoded there instead of some string that has to be
> translated through a table in a driver. There's still going to be
> shifting and masking of bits in the driver to put the value in the right
> place in the register, but we avoid needing N number of drivers for each
> pmic just to translate strings into integers (for functions) and
> integers into other integers (for the power source). 

This sounds good to me. Drawback is that we will have custom 
function parsing code, but I will try and see what that looks like.

Thanks,
Ivan


> From what I can
> tell there isn't any benefit to having the function property be a string
> vs. a #define number besides having a human readable string in pinctrl
> debugfs. Is there some other benefit?
> 

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Ivan T. Ivanov]

On Fri, 2014-07-25 at 18:15 +0300, Ivan T. Ivanov wrote:
> On Thu, 2014-07-24 at 17:23 -0700, Stephen Boyd wrote:
> > On 07/24/14 08:40, Linus Walleij wrote:
> > > On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > >
> > >>> Please add these constants to the table of valid power-source values and use
> > >>> something like I did to translate them to register values - it makes the DT
> > >>> much more readable.
> > >> The DT could be similarly readable if we had a bunch of #defines for the
> > >> different VIN settings that resolved to the final register value for
> > >> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
> > >> etc. There would be a lot of them, but then the driver could be really
> > >> simple and just jam whatever value is in the DT into the register
> > >> without having to bounce through a mapping table in software to figure
> > >> out the register value. 

This is ok.

> If we did this for the functions also then I
> > >> believe we achieve readability without requiring a bunch of drivers for
> > >> each and every single pmic?


> > > Not sure but it sounds like you want to make the device tree a jam table,
> > > (know about individual register offsets, sequences etc). That has been
> > > throrougly NACKed in the past, because DT is not Open Firmware.
> > >
> > > The exception is pinctrl-single which is restricted to single register
> > > per pin use cases and is still a point of contention...
> > >
> > 
> > I'm not proposing a jam table. I'm proposing that we make the
> > function/source property convenient to the driver by having the actual
> > function field value encoded there instead of some string that has to be
> > translated through a table in a driver. There's still going to be
> > shifting and masking of bits in the driver to put the value in the right
> > place in the register, but we avoid needing N number of drivers for each
> > pmic just to translate strings into integers (for functions) and
> > integers into other integers (for the power source). 
> 
> This sounds good to me. Drawback is that we will have custom 
> function parsing code, but I will try and see what that looks like.

Hm, I played with this, using numbers for functions. Unfortunately it is
not easy possible to hack existing DT pin-control parsing code to use
numbers instead strings, so I ended coping Samsung parsing code, but unless
this is separated in library we will end with huge code duplication for
little benefit.

Furthermore meaning of number 2 and 3, which represent PMIC GPIO Special
Function 1 and 2 are not consistent across PMIC chips. For example KEYPD
function in PM8038 is encoded with 3, but in PM8058 it is 2.

I tend to agree with Bjorn that "function" property should be "normal", 
"paired", "func1", "func2","dtest1", "dtest2", "dtest3", "dtest4" and we
can add new property "qcom,mode" which will select between digital/analog
input/output.

In DT include file we can still have something like this:

/* To be used with "function = <>" */
#define QCOM_GPIO_FUNC_NORMAL		"normal"
#define QCOM_GPIO_FUNC_PAIRED		"paired"
#define QCOM_GPIO_FUNC_FUNC1		"func1"
#define QCOM_GPIO_FUNC_FUNC2		"func2"
...

#define PM8038_GPIO1_2_LPG_DRV		QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO3_5V_BOOST_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO4_SSBI_ALT_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO5_6_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO10_11_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_7_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO9_BAT_ALRM_OUT	QCOM_GPIO_FUNC_FUNC1
#define PM8038_GPIO6_12_KYPD_DRV	QCOM_GPIO_FUNC_FUNC2

#define PM8058_GPIO7_8_MP3_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO7_8_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO9_26_KYPD_DRV	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO21_23_UART_TX	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO24_26_LPG_DRV	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO33_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO34_35_MP3_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO36_BCLK_19P2MHZ	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO37_UPL_OUT		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO37_UART_M_RX		QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO38_XO_SLEEP_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO38_39_CLK_32KHZ	QCOM_GPIO_FUNC_FUNC2
#define PM8058_GPIO39_MP3_CLK		QCOM_GPIO_FUNC_FUNC1
#define PM8058_GPIO40_EXT_BB_EN		QCOM_GPIO_FUNC_FUNC1

#define PM8917_GPIO9_18_KEYP_DRV	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO20_BAT_ALRM_OUT	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO21_23_UART_TX	QCOM_GPIO_FUNC_FUNC2
#define PM8917_GPIO25_26_EXT_REG_EN	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO37_38_XO_SLEEP_CLK	QCOM_GPIO_FUNC_FUNC1
#define PM8917_GPIO37_38_MP3_CLK	QCOM_GPIO_FUNC_FUNC2
...

Re: [PATCH RESEND v2 1/4] pinctrl: Update Qualcomm pm8xxx GPIO parameters definitions

[Bjorn Andersson]

On Wed, Aug 6, 2014 at 8:02 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> On Fri, 2014-07-25 at 18:15 +0300, Ivan T. Ivanov wrote:
>> On Thu, 2014-07-24 at 17:23 -0700, Stephen Boyd wrote:
>> > On 07/24/14 08:40, Linus Walleij wrote:
>> > > On Thu, Jul 24, 2014 at 1:47 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > >
>> > >>> Please add these constants to the table of valid power-source values and use
>> > >>> something like I did to translate them to register values - it makes the DT
>> > >>> much more readable.
>> > >> The DT could be similarly readable if we had a bunch of #defines for the
>> > >> different VIN settings that resolved to the final register value for
>> > >> that pmic. Something like PM8921_GPIO1_14_VPH, PM8921_GPIO19_36_VPH,
>> > >> etc. There would be a lot of them, but then the driver could be really
>> > >> simple and just jam whatever value is in the DT into the register
>> > >> without having to bounce through a mapping table in software to figure
>> > >> out the register value.
>
> This is ok.
>

I'm not sure I think the "optimization" is worth the cluttered names
of these defines, but I will give it a spin and see how it looks!

>> If we did this for the functions also then I
>> > >> believe we achieve readability without requiring a bunch of drivers for
>> > >> each and every single pmic?
>

Either we have a table like the other pinctrl drivers, or we just go
with custom parsing where we shove the register value straight into
devicetree. Although the latter would reduce the number of mapping
tables we need in the kernel, it seems to not follow the general way
of doing things with pinctrl.

[...]
>
> Furthermore meaning of number 2 and 3, which represent PMIC GPIO Special
> Function 1 and 2 are not consistent across PMIC chips. For example KEYPD
> function in PM8038 is encoded with 3, but in PM8058 it is 2.
>
> I tend to agree with Bjorn that "function" property should be "normal",
> "paired", "func1", "func2","dtest1", "dtest2", "dtest3", "dtest4" and we
> can add new property "qcom,mode" which will select between digital/analog
> input/output.
>

Note that for GPIOs we have normal/gpio, paried and a set of functions
(if we ignore the dtest ones). And for MPPs we have digital and analog
as paired and unpaired. Input/output should be controlled with the
separate means already available (gpio api, output-{low,high}.

> In DT include file we can still have something like this:
>
> /* To be used with "function = <>" */
> #define QCOM_GPIO_FUNC_NORMAL           "normal"
> #define QCOM_GPIO_FUNC_PAIRED           "paired"
> #define QCOM_GPIO_FUNC_FUNC1            "func1"
> #define QCOM_GPIO_FUNC_FUNC2            "func2"
> ...
>
> #define PM8038_GPIO1_2_LPG_DRV          QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO3_5V_BOOST_EN        QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO4_SSBI_ALT_CLK       QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO5_6_EXT_REG_EN       QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO10_11_EXT_REG_EN     QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO6_7_CLK              QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO9_BAT_ALRM_OUT       QCOM_GPIO_FUNC_FUNC1
> #define PM8038_GPIO6_12_KYPD_DRV        QCOM_GPIO_FUNC_FUNC2
>
> #define PM8058_GPIO7_8_MP3_CLK          QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO7_8_BCLK_19P2MHZ     QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO9_26_KYPD_DRV        QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO21_23_UART_TX        QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO24_26_LPG_DRV        QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO33_BCLK_19P2MHZ      QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO34_35_MP3_CLK        QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO36_BCLK_19P2MHZ      QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO37_UPL_OUT           QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO37_UART_M_RX         QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO38_XO_SLEEP_CLK      QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO38_39_CLK_32KHZ      QCOM_GPIO_FUNC_FUNC2
> #define PM8058_GPIO39_MP3_CLK           QCOM_GPIO_FUNC_FUNC1
> #define PM8058_GPIO40_EXT_BB_EN         QCOM_GPIO_FUNC_FUNC1
>
> #define PM8917_GPIO9_18_KEYP_DRV        QCOM_GPIO_FUNC_FUNC1
> #define PM8917_GPIO20_BAT_ALRM_OUT      QCOM_GPIO_FUNC_FUNC1
> #define PM8917_GPIO21_23_UART_TX        QCOM_GPIO_FUNC_FUNC2
> #define PM8917_GPIO25_26_EXT_REG_EN     QCOM_GPIO_FUNC_FUNC1
> #define PM8917_GPIO37_38_XO_SLEEP_CLK   QCOM_GPIO_FUNC_FUNC1
> #define PM8917_GPIO37_38_MP3_CLK        QCOM_GPIO_FUNC_FUNC2
> ...

If we're going to maintain this "table" in the kernel then I would
greatly prefer that we just stick with the standard way of
representing it in the pinctrl drivers. My concern is still related to
me lacking the appropriate documentation to create these tables.

I introduced the necessary tables for pm8058 and pm8921 and it looks
reasonable. I'll try to finish it up tomorrow and send out a copy for
you to have a look.

Regards,
Bjorn

Re: [PATCH v2 0/4] New Qualcomm PMIC pin controller drivers

[Linus Walleij]

On Thu, Jul 17, 2014 at 5:25 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:

> Patches could be applied on top of the Bjorn Andersson
> qcom,pm8xxx-gpio driver, which could be found here[2].
>
> Fist patch modify Bjorn's driver and bindings to make room
> for newer Qualcomm PMIC chips. It is not completely ready.

I like what I'm seeing :-)

Good cooperation and joint efforts. That's the spirit!

In the end, Björn can you provide a pullable branch for me?

This will likely be v3.18 development cycle material as
the v3.17 merge window is quite close.

Yours,
Linus Walleij