[PATCH 0/3] Qualcomm pm8xxx gpio driver

[PATCH 0/3] Qualcomm pm8xxx gpio driver

[Bjorn Andersson]

This is device tree bindings as well as a driver for the gpio blocks found in
Qualcomm pm8018, pm8038, pm8058, pm8917 and pm8921 pmic chips.

The first patch extends the pm8921-core to expose a function to read out the
"RT status" of an interrupt pin. This is the way input many input values are
exposed in these set of pmics.

Bjorn Andersson (3):
  mfd: pm8921: Expose pm8xxx_read_irq_status
  pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block
  pinctrl: Introduce pinctrl driver for Qualcomm pm8xxx

 .../bindings/pinctrl/qcom,pm8xxx-gpio.txt          |  230 ++++++
 drivers/mfd/pm8921-core.c                          |   36 +
 drivers/pinctrl/Kconfig                            |   11 +
 drivers/pinctrl/Makefile                           |    1 +
 drivers/pinctrl/pinctrl-pm8xxx-gpio.c              |  795 ++++++++++++++++++++
 include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h     |   34 +
 include/linux/mfd/pm8921-core.h                    |   32 +
 7 files changed, 1139 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
 create mode 100644 drivers/pinctrl/pinctrl-pm8xxx-gpio.c
 create mode 100644 include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
 create mode 100644 include/linux/mfd/pm8921-core.h

-- 
1.7.9.5

[PATCH 1/3] mfd: pm8921: Expose pm8xxx_read_irq_status

[Bjorn Andersson]

Most status bits, e.g. for GPIO and MPP input, is retrieved by reading
the interrupt status registers, so this needs to be exposed to clients.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/mfd/pm8921-core.c       |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/pm8921-core.h |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 include/linux/mfd/pm8921-core.h

diff --git a/drivers/mfd/pm8921-core.c b/drivers/mfd/pm8921-core.c
index 9595138..7dda0c2 100644
--- a/drivers/mfd/pm8921-core.c
+++ b/drivers/mfd/pm8921-core.c
@@ -26,6 +26,7 @@
 #include <linux/regmap.h>
 #include <linux/of_platform.h>
 #include <linux/mfd/core.h>
+#include <linux/mfd/pm8921-core.h>
 
 #define	SSBI_REG_ADDR_IRQ_BASE		0x1BB
 
@@ -65,6 +66,41 @@ struct pm_irq_chip {
 	u8			config[0];
 };
 
+int pm8xxx_read_irq_status(int irq)
+{
+	struct irq_data *d = irq_get_irq_data(irq);
+	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int pmirq = irqd_to_hwirq(d);
+	unsigned int bits;
+	int irq_bit;
+	u8 block;
+	int rc;
+
+	if (!chip) {
+		pr_err("Failed to resolve pm_irq_chip\n");
+		return -EINVAL;
+	}
+
+	block = pmirq / 8;
+	irq_bit = pmirq % 8;
+
+	spin_lock(&chip->pm_irq_lock);
+	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
+	if (rc) {
+		pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
+		goto bail;
+	}
+
+	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
+	if (rc)
+		pr_err("Failed Reading Status rc=%d\n", rc);
+bail:
+	spin_unlock(&chip->pm_irq_lock);
+
+	return rc ? rc : !!(bits & BIT(irq_bit));
+}
+EXPORT_SYMBOL(pm8xxx_read_irq_status);
+
 static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
 				 unsigned int *ip)
 {
diff --git a/include/linux/mfd/pm8921-core.h b/include/linux/mfd/pm8921-core.h
new file mode 100644
index 0000000..77f7cb5
--- /dev/null
+++ b/include/linux/mfd/pm8921-core.h
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __MFD_PM8921_CORE_H
+#define __MFD_PM8921_CORE_H
+
+#include <linux/err.h>
+
+#ifdef CONFIG_MFD_PM8921_CORE
+
+int pm8xxx_read_irq_status(int irq);
+
+#else
+
+static inline int pm8xxx_read_irq_status(int irq)
+{
+	return -ENOSYS;
+}
+
+#endif
+
+#endif
-- 
1.7.9.5

[PATCH 1/3] mfd: pm8921: Expose pm8xxx_read_irq_status

[Stephen Boyd]

On 07/07/14 18:26, Bjorn Andersson wrote:
> @@ -65,6 +66,41 @@ struct pm_irq_chip {
>  	u8			config[0];
>  };
>  
> +int pm8xxx_read_irq_status(int irq)
> +{
> +	struct irq_data *d = irq_get_irq_data(irq);
> +	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> +	unsigned int pmirq = irqd_to_hwirq(d);
> +	unsigned int bits;
> +	int irq_bit;
> +	u8 block;
> +	int rc;
> +
> +	if (!chip) {
> +		pr_err("Failed to resolve pm_irq_chip\n");
> +		return -EINVAL;
> +	}

Is this actually necessary? Presumably the driver wouldn't have even
probed unless there was a pmic to begin with.

> +
> +	block = pmirq / 8;
> +	irq_bit = pmirq % 8;
> +
> +	spin_lock(&chip->pm_irq_lock);
> +	rc = regmap_write(chip->regmap, SSBI_REG_ADDR_IRQ_BLK_SEL, block);
> +	if (rc) {
> +		pr_err("Failed Selecting Block %d rc=%d\n", block, rc);
> +		goto bail;
> +	}
> +
> +	rc = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
> +	if (rc)
> +		pr_err("Failed Reading Status rc=%d\n", rc);
> +bail:
> +	spin_unlock(&chip->pm_irq_lock);
> +
> +	return rc ? rc : !!(bits & BIT(irq_bit));
> +}
> +EXPORT_SYMBOL(pm8xxx_read_irq_status);
> +
>  static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
>  				 unsigned int *ip)
>  {
> diff --git a/include/linux/mfd/pm8921-core.h b/include/linux/mfd/pm8921-core.h
> new file mode 100644
> index 0000000..77f7cb5
> --- /dev/null
> +++ b/include/linux/mfd/pm8921-core.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2014, Sony Mobile Communications AB
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __MFD_PM8921_CORE_H
> +#define __MFD_PM8921_CORE_H
> +
> +#include <linux/err.h>
> +
> +#ifdef CONFIG_MFD_PM8921_CORE
> +
> +int pm8xxx_read_irq_status(int irq);
> +
> +#else
> +
> +static inline int pm8xxx_read_irq_status(int irq)
> +{
> +	return -ENOSYS;
> +}
> +
> +#endif

Sad, the header file came back. I guess there isn't a way to put the
pinctrl driver inside the core mfd driver? Then we wouldn't need to
expose an "irq read line" function.

Actually Abhijeet proposed such an API in 2011 but it didn't go
anywhere[1]. If we had that API we should be able to call
read_irq_line() from the pinctrl driver whenever we want to get the
state of the gpio, plus the API is generic. We're going to need that API
anyway for things like USB insertion detection so it might make sense to
add it sooner rather than later.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/048319.html

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

[PATCH 1/3] mfd: pm8921: Expose pm8xxx_read_irq_status

[Bjorn Andersson]

On Tue, Jul 8, 2014 at 4:20 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 07/07/14 18:26, Bjorn Andersson wrote:
>> @@ -65,6 +66,41 @@ struct pm_irq_chip {
>>       u8                      config[0];
>>  };
>>
>> +int pm8xxx_read_irq_status(int irq)
>> +{
>> +     struct irq_data *d = irq_get_irq_data(irq);
>> +     struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
>> +     unsigned int pmirq = irqd_to_hwirq(d);
>> +     unsigned int bits;
>> +     int irq_bit;
>> +     u8 block;
>> +     int rc;
>> +
>> +     if (!chip) {
>> +             pr_err("Failed to resolve pm_irq_chip\n");
>> +             return -EINVAL;
>> +     }
>
> Is this actually necessary? Presumably the driver wouldn't have even
> probed unless there was a pmic to begin with.
>

I had a bug in the calling driver, passing the wrong irq down to this
function. But now that I think more about it I should probably check
for "d" being non-NULL. On the other hand, that will still just catch
a minor set of bugs as both of those will in most cases produce
"valid" pointers.

Maybe it's okay to just trust the caller to pass a valid irq? Or do
you have any other suggestion of sanity checking the input value?
Preferably without also passing the pm_irq_chip pointer.

[...]
> Sad, the header file came back. I guess there isn't a way to put the
> pinctrl driver inside the core mfd driver? Then we wouldn't need to
> expose an "irq read line" function.
>

I continued my search and this needs to be accessed by gpio, mpp, adc,
charger, bms and usb(?). So we have to expose it in some form.

> Actually Abhijeet proposed such an API in 2011 but it didn't go
> anywhere[1]. If we had that API we should be able to call
> read_irq_line() from the pinctrl driver whenever we want to get the
> state of the gpio, plus the API is generic. We're going to need that API
> anyway for things like USB insertion detection so it might make sense to
> add it sooner rather than later.
>
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/048319.html

>From what I can see of this thread it was exposed as a way for drivers
to be able to query if an interrupt handler was called on raising or
falling edge. And based on the locking limitations of the
implementation we couldn't have used it anyways.

Our use case is different in that we're at any point in time
interested in reading out the status of the irq line, as the only way
of getting that status.

It might be a long shot, but let's spin a patch for our purpose and
run it by Tomas again.

Regards,
Bjorn

[PATCH 1/3] mfd: pm8921: Expose pm8xxx_read_irq_status

[Ivan T. Ivanov]

On Tue, 2014-07-08 at 16:43 -0700, Bjorn Andersson wrote:
> On Tue, Jul 8, 2014 at 4:20 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 07/07/14 18:26, Bjorn Andersson wrote:

<snip>

> [...]
> > Sad, the header file came back. I guess there isn't a way to put the
> > pinctrl driver inside the core mfd driver? Then we wouldn't need to
> > expose an "irq read line" function.
> >
> 
> I continued my search and this needs to be accessed by gpio, mpp, adc,
> charger, bms and usb(?). So we have to expose it in some form.
> 
> > Actually Abhijeet proposed such an API in 2011 but it didn't go
> > anywhere[1]. If we had that API we should be able to call
> > read_irq_line() from the pinctrl driver whenever we want to get the
> > state of the gpio, plus the API is generic. We're going to need that API
> > anyway for things like USB insertion detection so it might make sense to
> > add it sooner rather than later.
> >
> > [1]
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/048319.html
> 
> From what I can see of this thread it was exposed as a way for drivers
> to be able to query if an interrupt handler was called on raising or
> falling edge. And based on the locking limitations of the
> implementation we couldn't have used it anyways.
> 
> Our use case is different in that we're at any point in time
> interested in reading out the status of the irq line, as the only way
> of getting that status.

How about using extcon framework? 

Regards,
Ivan

[PATCH 1/3] mfd: pm8921: Expose pm8xxx_read_irq_status

[Linus Walleij]

On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:

> Most status bits, e.g. for GPIO and MPP input, is retrieved by reading
> the interrupt status registers, so this needs to be exposed to clients.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

Hm do you mean you read the input *values* from the interrupt status
registers?

What madness in that case.... :-)

Anyway, since the driver is based on regmap, can't the children just
get a regmap * somehow and then just go read the same register
instead of having to add a special function for it?

When I look at it it seems like it's doing regmap strangely or
something, like it's one write then one read operation to get
the register(s) and isn't that all supposed to be hidden behind
regmap so you don't need the local lock chip->pm_irq_lock?

Yours,
Linus Walleij

[PATCH 1/3] mfd: pm8921: Expose pm8xxx_read_irq_status

[Bjorn Andersson]

On Wed, Jul 9, 2014 at 12:59 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
>
>> Most status bits, e.g. for GPIO and MPP input, is retrieved by reading
>> the interrupt status registers, so this needs to be exposed to clients.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>
> Hm do you mean you read the input *values* from the interrupt status
> registers?
>

Due to the limited address space (I presume), many of the status bits
on the pm8xxx are not exposed in any other place than through a banked
register in the interrupt "block". So we have to read the interrupt
status in order to get information related to things like gpio status
or if a battery is present for charging.

> What madness in that case.... :-)
>

Totally!

> Anyway, since the driver is based on regmap, can't the children just
> get a regmap * somehow and then just go read the same register
> instead of having to add a special function for it?
>

That we have, and we have access to that part of the ssbi address
space. Unfortunately, like everything else in these pmics, things are
banked. So we need first a bank selector and then a read; so it's racy
with the interrupt handler code doing the same thing.

> When I look at it it seems like it's doing regmap strangely or
> something, like it's one write then one read operation to get
> the register(s) and isn't that all supposed to be hidden behind
> regmap so you don't need the local lock chip->pm_irq_lock?
>

I guess one could have exposed a regmap that instead of exposing the
ssbi address space presented a logical view of the pm8xxx registers;
something like the bitplanes on Amiga. I prefer having a ssbi regmap
for simplicity (and sanity) but then we need to have this read in the
irq driver.

Regards,
Bjorn

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Bjorn Andersson]

This introduced the device tree bindings for the gpio block found in
pm8018, pm8038, pm8058, pm8917 and pm8921 pmics from Qualcomm.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 .../bindings/pinctrl/qcom,pm8xxx-gpio.txt          |  230 ++++++++++++++++++++
 include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h     |   34 +++
 2 files changed, 264 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
 create mode 100644 include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
new file mode 100644
index 0000000..0035dd8
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,pm8xxx-gpio.txt
@@ -0,0 +1,230 @@
+Qualcomm PM8XXX GPIO block
+
+This binding describes the GPIO block(s) found in the 8xxx series of pmics from
+Qualcomm.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+			"qcom,pm8018-gpio"
+			"qcom,pm8038-gpio"
+			"qcom,pm8058-gpio"
+			"qcom,pm8917-gpio"
+			"qcom,pm8921-gpio"
+
+- 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:
+			gpio1-gpio6 for pm8018
+			gpio1-gpio12 for pm8038
+			gpio1-gpio40 for pm8058
+			gpio1-gpio38 for pm8917
+			gpio1-gpio44 for pm8921
+
+- function:
+	Usage: optional
+	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"
+
+- bias-disable:
+	Usage: optional
+	Value type: <none>
+	Definition: The specified pins should be configued as no pull.
+
+- bias-pull-down:
+	Usage: optional
+	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>
+	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
+		    power sources are, as defined in
+		    <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>:
+			0: bb (PM8XXX_GPIO_VIN_BB)
+				valid for pm8038, pm8058, pm8917, pm8921
+			1: ldo2 (PM8XXX_GPIO_VIN_L2)
+				valid for pm8018, pm8038, pm8917,pm8921
+			2: ldo3 (PM8XXX_GPIO_VIN_L3)
+				valid for pm8038, pm8058, pm8917, pm8921
+			3: ldo4 (PM8XXX_GPIO_VIN_L4)
+				valid for pm8018, pm8917, pm8921
+			4: ldo5 (PM8XXX_GPIO_VIN_L5)
+				valid for pm8018, pm8058
+			5: ldo6 (PM8XXX_GPIO_VIN_L6)
+				valid for pm8018, pm8058
+			6: ldo7 (PM8XXX_GPIO_VIN_L7)
+				valid for pm8058
+			7: ldo8 (PM8XXX_GPIO_VIN_L8)
+				valid for pm8018
+			8: ldo11 (PM8XXX_GPIO_VIN_L11)
+				valid for pm8038
+			9: ldo14 (PM8XXX_GPIO_VIN_L14)
+				valid for pm8018
+			10: ldo15 (PM8XXX_GPIO_VIN_L15)
+				valid for pm8038, pm8917, pm8921
+			11: ldo17 (PM8XXX_GPIO_VIN_L17)
+				valid for pm8038, pm8917, pm8921
+			12: smps3 (PM8XXX_GPIO_VIN_S3)
+				valid for pm8018, pm8058
+			13: smps4 (PM8XXX_GPIO_VIN_S4)
+				valid for pm8921
+			14: vph (PM8XXX_GPIO_VIN_VPH)
+				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>
+	Definition: The specified pins are configured in push-pull mode.
+
+- drive-open-drain:
+	Usage: optional
+	Value type: <none>
+	Definition: The specified pins are configured in open-drain mode.
+
+
+Example:
+
+	pm8921_gpio: gpio at 150 {
+		compatible = "qcom,pm8921-gpio";
+		reg = <0x150>;
+		interrupts = <192 1>, <193 1>, <194 1>,
+			     <195 1>, <196 1>, <197 1>,
+			     <198 1>, <199 1>, <200 1>,
+			     <201 1>, <202 1>, <203 1>,
+			     <204 1>, <205 1>, <206 1>,
+			     <207 1>, <208 1>, <209 1>,
+			     <210 1>, <211 1>, <212 1>,
+			     <213 1>, <214 1>, <215 1>,
+			     <216 1>, <217 1>, <218 1>,
+			     <219 1>, <220 1>, <221 1>,
+			     <222 1>, <223 1>, <224 1>,
+			     <225 1>, <226 1>, <227 1>,
+			     <228 1>, <229 1>, <230 1>,
+			     <231 1>, <232 1>, <233 1>,
+			     <234 1>, <235 1>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		pm8921_gpio_keys: gpio-keys {
+			volume-keys {
+				pins = "gpio20", "gpio21";
+				function = "normal";
+
+				input-enable;
+				bias-pull-up;
+				drive-push-pull;
+				drive-strength = <PM8XXX_GPIO_STRENGTH_NO>;
+				power-source = <PM8XXX_GPIO_VIN_S4>;
+			};
+		};
+	};
diff --git a/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
new file mode 100644
index 0000000..6b66fff
--- /dev/null
+++ b/include/dt-bindings/pinctrl/qcom,pm8xxx-gpio.h
@@ -0,0 +1,34 @@
+/*
+ * This header provides constants for the pm8xxx gpio binding.
+ */
+
+#ifndef _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
+#define _DT_BINDINGS_PINCTRL_QCOM_PM8XXX_GPIO_H
+
+#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
+
+#define	PM8XXX_GPIO_STRENGTH_NO		0
+#define	PM8XXX_GPIO_STRENGTH_HIGH	1
+#define	PM8XXX_GPIO_STRENGTH_MED	2
+#define	PM8XXX_GPIO_STRENGTH_LOW	3
+
+#endif
-- 
1.7.9.5

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Linus Walleij]

On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:

> This introduced the device tree bindings for the gpio block found in
> pm8018, pm8038, pm8058, pm8917 and pm8921 pmics from Qualcomm.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
(...)
> +- interrupts:
> +       Usage: required
> +       Value type: <prop-encoded-array>
> +       Definition: Must contain an array of encoded interrupt specifiers for
> +                   each available gpio

So each GPIO has its own interrupt line looped back from the PMIC
and into some other SoC or so handling the actual interrupt?
This seems expensive? (Albeit efficient and fast.)

(...)
> +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.

*is* listed below?

> +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:
> +                       gpio1-gpio6 for pm8018
> +                       gpio1-gpio12 for pm8038
> +                       gpio1-gpio40 for pm8058
> +                       gpio1-gpio38 for pm8917
> +                       gpio1-gpio44 for pm8921

I usually name pins with CAPITAL LETTERS so would be
"GPIO1", "GPIO2" etc, lowercase may be confusing as these are
names not functions or groups.

> +- function:
> +       Usage: optional
> +       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"

These are a bit ambigous, why doesn't the driver present functions that
are more specific than "func1", "func2"? Or "dtest1"?

I guess the following just redefines or extends the generic pinconf
bindings (which is fully OK).

> +- bias-disable:
> +       Usage: optional
> +       Value type: <none>

Isn't the type simply bool?

> +- bias-pull-down:
> +       Usage: optional
> +       Value type: <none>

bool?

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

Hm, I don't know of the internal kernel API or so, but I'm thinking that
for the DT bindings, this definition should be generic in
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
and put in SI units like uA.

So I would prefer:

bias-pull-up = <30>;

for 30 uA. Maybe we want nA even? I'm uncertain about the proper granularity
here :-/

Magic enumerators 1,2,3,4 doesn't seem so good, that seems more like it's
trying to match the magic value that is to be poked into a register or
something like that.

> +- bias-high-impedance:
> +       Usage: optional
> +       Value type: <none>

bool

> +- input-enable:
> +       Usage: optional
> +       Value type: <none>

bool

> +- output-high:
> +       Usage: optional
> +       Value type: <none>

bool

> +- output-low:
> +       Usage: optional
> +       Value type: <none>

bool

> +- power-source:
> +       Usage: optional
> +       Value type: <u32>
> +       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)
> +                               valid for pm8038, pm8058, pm8917, pm8921
> +                       1: ldo2 (PM8XXX_GPIO_VIN_L2)
> +                               valid for pm8018, pm8038, pm8917,pm8921
> +                       2: ldo3 (PM8XXX_GPIO_VIN_L3)
> +                               valid for pm8038, pm8058, pm8917, pm8921
> +                       3: ldo4 (PM8XXX_GPIO_VIN_L4)
> +                               valid for pm8018, pm8917, pm8921
> +                       4: ldo5 (PM8XXX_GPIO_VIN_L5)
> +                               valid for pm8018, pm8058
> +                       5: ldo6 (PM8XXX_GPIO_VIN_L6)
> +                               valid for pm8018, pm8058
> +                       6: ldo7 (PM8XXX_GPIO_VIN_L7)
> +                               valid for pm8058
> +                       7: ldo8 (PM8XXX_GPIO_VIN_L8)
> +                               valid for pm8018
> +                       8: ldo11 (PM8XXX_GPIO_VIN_L11)
> +                               valid for pm8038
> +                       9: ldo14 (PM8XXX_GPIO_VIN_L14)
> +                               valid for pm8018
> +                       10: ldo15 (PM8XXX_GPIO_VIN_L15)
> +                               valid for pm8038, pm8917, pm8921
> +                       11: ldo17 (PM8XXX_GPIO_VIN_L17)
> +                               valid for pm8038, pm8917, pm8921
> +                       12: smps3 (PM8XXX_GPIO_VIN_S3)
> +                               valid for pm8018, pm8058
> +                       13: smps4 (PM8XXX_GPIO_VIN_S4)
> +                               valid for pm8921
> +                       14: vph (PM8XXX_GPIO_VIN_VPH)
> +                               valid for pm8018, pm8038, pm8058, pm8917 pm8921

These are more apropriate to have in custom format selectors
I think, so this is OK.

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

I would really prefer to have these in mA, because the genric pinconf
bindings say they should be! SI units are so much more understandable.

> +- drive-push-pull:
> +       Usage: optional
> +       Value type: <none>

bool

> +- drive-open-drain:
> +       Usage: optional
> +       Value type: <none>

bool

> +Example:
> +
> +       pm8921_gpio: gpio at 150 {
> +               compatible = "qcom,pm8921-gpio";
> +               reg = <0x150>;
> +               interrupts = <192 1>, <193 1>, <194 1>,
> +                            <195 1>, <196 1>, <197 1>,
> +                            <198 1>, <199 1>, <200 1>,
> +                            <201 1>, <202 1>, <203 1>,
> +                            <204 1>, <205 1>, <206 1>,
> +                            <207 1>, <208 1>, <209 1>,
> +                            <210 1>, <211 1>, <212 1>,
> +                            <213 1>, <214 1>, <215 1>,
> +                            <216 1>, <217 1>, <218 1>,
> +                            <219 1>, <220 1>, <221 1>,
> +                            <222 1>, <223 1>, <224 1>,
> +                            <225 1>, <226 1>, <227 1>,
> +                            <228 1>, <229 1>, <230 1>,
> +                            <231 1>, <232 1>, <233 1>,
> +                            <234 1>, <235 1>;


So this looks a bit weird. But if I just get to understand the hardware
I guess it won't anymore.

So there is an interrupt parent to which the IRQ lines from the PMIC
are routed back through external lines to IRQ offsets 192 thru 235?

Yours,
Linus Walleij

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Bjorn Andersson]

On Wed, Jul 9, 2014 at 1:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
>
>> This introduced the device tree bindings for the gpio block found in
>> pm8018, pm8038, pm8058, pm8917 and pm8921 pmics from Qualcomm.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> (...)
>> +- interrupts:
>> +       Usage: required
>> +       Value type: <prop-encoded-array>
>> +       Definition: Must contain an array of encoded interrupt specifiers for
>> +                   each available gpio
>
> So each GPIO has its own interrupt line looped back from the PMIC
> and into some other SoC or so handling the actual interrupt?
> This seems expensive? (Albeit efficient and fast.)
>
> (...)

Almost. The pmic have a shared interrupt and the pm8921-core driver has a
chained irq handler. Each gpio get it's own interrupt on that level.

>> +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.
>
> *is* listed below?
>

*as*

>> +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:
>> +                       gpio1-gpio6 for pm8018
>> +                       gpio1-gpio12 for pm8038
>> +                       gpio1-gpio40 for pm8058
>> +                       gpio1-gpio38 for pm8917
>> +                       gpio1-gpio44 for pm8921
>
> I usually name pins with CAPITAL LETTERS so would be
> "GPIO1", "GPIO2" etc, lowercase may be confusing as these are
> names not functions or groups.
>

I was hoping to be able to follow the pattern used in pinctrl-msm; can I say
something nice to have you agree on this? There's no difference between pins
and groups here.

>> +- function:
>> +       Usage: optional
>> +       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"
>
> These are a bit ambigous, why doesn't the driver present functions that
> are more specific than "func1", "func2"? Or "dtest1"?
>

I agree, unfortunately I have only seen traces of the actual function matrix;
for pm8xxx I have no documentation and for pm8x41 they are only listed as
func[1-2] and dtest[1-4].

Maybe if someone at Qualcomm could release such a list we could provide a
proper table instead.

> I guess the following just redefines or extends the generic pinconf
> bindings (which is fully OK).
>
>> +- bias-disable:
>> +       Usage: optional
>> +       Value type: <none>
>
> Isn't the type simply bool?
>

No, the property is bool but the actual value is void. But looking an extra
time in the epapr I see that it's supposed to be "empty" - so will update.

[...]
>> +- 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)
>
> Hm, I don't know of the internal kernel API or so, but I'm thinking that
> for the DT bindings, this definition should be generic in
> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> and put in SI units like uA.
>

Totally agree with you; and this is already specified in pinctrl-binding.txt as
being Ohm.

So I first did a spin with the strength as a separate property, but as that
because the only part that pinconf-generic didn't parse for me I merged it and
wanted your comment on it.

> So I would prefer:
>
> bias-pull-up = <30>;
>

Yeah, but that's the easy one ;)

How do you say 1.5 or 31.5 and how do you differ that from 1.5 + 30 boot?

> for 30 uA. Maybe we want nA even? I'm uncertain about the proper granularity
> here :-/
>
> Magic enumerators 1,2,3,4 doesn't seem so good, that seems more like it's
> trying to match the magic value that is to be poked into a register or
> something like that.
>

The stuff going into the hardware is a value 0-3 for pull up; so these values
are "only" an enum with the additional benefit of saying "bias-pull-up;"
results in 30uA pull up which is the most commonly used form (hence being
optional).

[...]
>> +- 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)
>
> I would really prefer to have these in mA, because the genric pinconf
> bindings say they should be! SI units are so much more understandable.
>

This is all the information to be found in the available documentation and
code. Maybe someone from Qualcomm can shed some light on it?

[...]
>> +
>> +       pm8921_gpio: gpio at 150 {
>> +               compatible = "qcom,pm8921-gpio";
>> +               reg = <0x150>;
>> +               interrupts = <192 1>, <193 1>, <194 1>,
>> +                            <195 1>, <196 1>, <197 1>,
>> +                            <198 1>, <199 1>, <200 1>,
>> +                            <201 1>, <202 1>, <203 1>,
>> +                            <204 1>, <205 1>, <206 1>,
>> +                            <207 1>, <208 1>, <209 1>,
>> +                            <210 1>, <211 1>, <212 1>,
>> +                            <213 1>, <214 1>, <215 1>,
>> +                            <216 1>, <217 1>, <218 1>,
>> +                            <219 1>, <220 1>, <221 1>,
>> +                            <222 1>, <223 1>, <224 1>,
>> +                            <225 1>, <226 1>, <227 1>,
>> +                            <228 1>, <229 1>, <230 1>,
>> +                            <231 1>, <232 1>, <233 1>,
>> +                            <234 1>, <235 1>;
>
>
> So this looks a bit weird. But if I just get to understand the hardware
> I guess it won't anymore.
>
> So there is an interrupt parent to which the IRQ lines from the PMIC
> are routed back through external lines to IRQ offsets 192 thru 235?
>

The pm8921-core exposes 256 interrupts, the listed 44 interrupts here are what
comes out of that.

I was really reluctant to list all the interrupts, but I think it turned out
nicer than any of my other attempts; like only providing a base and then
relying on interrupts being consecutive.

Suggestions on how this "should" be solved are welcome, as we have the same
setup for the newer pmics (Ivan's patches) and the TLMM hardware (pinctrl-msm)
supports using dedicated interrupts for certain gpio pins (instead of passing
through the chain handler).

Regards,
Bjorn

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Linus Walleij]

On Wed, Jul 9, 2014 at 11:18 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Wed, Jul 9, 2014 at 1:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
>> <bjorn.andersson@sonymobile.com> wrote:

>>> +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:
>>> +                       gpio1-gpio6 for pm8018
>>> +                       gpio1-gpio12 for pm8038
>>> +                       gpio1-gpio40 for pm8058
>>> +                       gpio1-gpio38 for pm8917
>>> +                       gpio1-gpio44 for pm8921
>>
>> I usually name pins with CAPITAL LETTERS so would be
>> "GPIO1", "GPIO2" etc, lowercase may be confusing as these are
>> names not functions or groups.
>
> I was hoping to be able to follow the pattern used in pinctrl-msm; can I say
> something nice to have you agree on this? There's no difference between pins
> and groups here.

That's OK.

>>> +- function:
>>> +       Usage: optional
>>> +       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"
>>
>> These are a bit ambigous, why doesn't the driver present functions that
>> are more specific than "func1", "func2"? Or "dtest1"?
>
> I agree, unfortunately I have only seen traces of the actual function matrix;
> for pm8xxx I have no documentation and for pm8x41 they are only listed as
> func[1-2] and dtest[1-4].
>
> Maybe if someone at Qualcomm could release such a list we could provide a
> proper table instead.

I guess Stephen Boyd can help us. (?)

>> Isn't the type simply bool?
>>
>
> No, the property is bool but the actual value is void. But looking an extra
> time in the epapr I see that it's supposed to be "empty" - so will update.

OK.

>>> +- 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)
>>
>> Hm, I don't know of the internal kernel API or so, but I'm thinking that
>> for the DT bindings, this definition should be generic in
>> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> and put in SI units like uA.
>
> Totally agree with you; and this is already specified in pinctrl-binding.txt as
> being Ohm.
>
> So I first did a spin with the strength as a separate property, but as that
> because the only part that pinconf-generic didn't parse for me I merged it and
> wanted your comment on it.

Yeah. And thinking of it.... how can it be uA? It has to be Ohms... it's a pull
up resistor thing after all. I suspect the uA value is just something like the
maximum current drawn through the pullup given a certain voltage?

>> So I would prefer:
>>
>> bias-pull-up = <30>;
>>
>
> Yeah, but that's the easy one ;)
>
> How do you say 1.5 or 31.5 and how do you differ that from 1.5 + 30 boot?

It needs to be set using Ohms.

>> for 30 uA. Maybe we want nA even? I'm uncertain about the proper granularity
>> here :-/
>>
>> Magic enumerators 1,2,3,4 doesn't seem so good, that seems more like it's
>> trying to match the magic value that is to be poked into a register or
>> something like that.
>
> The stuff going into the hardware is a value 0-3 for pull up; so these values
> are "only" an enum with the additional benefit of saying "bias-pull-up;"
> results in 30uA pull up which is the most commonly used form (hence being
> optional).

What is the nominal voltage of these pins? GIven that you can figure
out the Ohms. And I suspect it to be something very close to N times
the resistance of a depleted transistor in this technology.

>>> +- 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)
>>
>> I would really prefer to have these in mA, because the genric pinconf
>> bindings say they should be! SI units are so much more understandable.
>
> This is all the information to be found in the available documentation and
> code. Maybe someone from Qualcomm can shed some light on it?

Stephen?

>>> +                            <234 1>, <235 1>;
>>
>>
>> So this looks a bit weird. But if I just get to understand the hardware
>> I guess it won't anymore.
>>
>> So there is an interrupt parent to which the IRQ lines from the PMIC
>> are routed back through external lines to IRQ offsets 192 thru 235?
>
> The pm8921-core exposes 256 interrupts, the listed 44 interrupts here are what
> comes out of that.

I get it. It makes sense to handle all IRQs in the core rather than spawning
yet another irqchip for the pin control driver.

> I was really reluctant to list all the interrupts, but I think it turned out
> nicer than any of my other attempts; like only providing a base and then
> relying on interrupts being consecutive.

I agree.

Yours,
Linus Walleij

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Stephen Boyd]

On 07/10/14 02:53, Linus Walleij wrote:
> On Wed, Jul 9, 2014 at 11:18 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
>> On Wed, Jul 9, 2014 at 1:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
>>> <bjorn.andersson@sonymobile.com> wrote:
>>>
>>> +- function:
>>> +       Usage: optional
>>> +       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"
>>> These are a bit ambigous, why doesn't the driver present functions that
>>> are more specific than "func1", "func2"? Or "dtest1"?
>> I agree, unfortunately I have only seen traces of the actual function matrix;
>> for pm8xxx I have no documentation and for pm8x41 they are only listed as
>> func[1-2] and dtest[1-4].
>>
>> Maybe if someone at Qualcomm could release such a list we could provide a
>> proper table instead.
> I guess Stephen Boyd can help us. (?)

Ok. "normal" is pretty much gpio mode, i.e. don't mux anything. "paired"
is where we take the output of the gpio next to it and loop it back into
this gpio (and vice versa). So gpio1 is paired with gpio2, gpio 3 is
paired with gpio 4, etc. This allows us to make level translators by
choosing different supply voltages for the paired gpios. "func1" and
"func2" are used for muxing things internally. "dtest" is used to mux
specific things out for testing purposes, not really used in any
end-products but still useful while debugging. I can provide the
function to pin mapping if necessary. There are lots of pmics.

>
>>>> +- 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)
>>> Hm, I don't know of the internal kernel API or so, but I'm thinking that
>>> for the DT bindings, this definition should be generic in
>>> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>>> and put in SI units like uA.
>> Totally agree with you; and this is already specified in pinctrl-binding.txt as
>> being Ohm.
>>
>> So I first did a spin with the strength as a separate property, but as that
>> because the only part that pinconf-generic didn't parse for me I merged it and
>> wanted your comment on it.
> Yeah. And thinking of it.... how can it be uA? It has to be Ohms... it's a pull
> up resistor thing after all. I suspect the uA value is just something like the
> maximum current drawn through the pullup given a certain voltage?
>
>>> So I would prefer:
>>>
>>> bias-pull-up = <30>;
>>>
>> Yeah, but that's the easy one ;)
>>
>> How do you say 1.5 or 31.5 and how do you differ that from 1.5 + 30 boot?
> It needs to be set using Ohms.
>
>>> for 30 uA. Maybe we want nA even? I'm uncertain about the proper granularity
>>> here :-/
>>>
>>> Magic enumerators 1,2,3,4 doesn't seem so good, that seems more like it's
>>> trying to match the magic value that is to be poked into a register or
>>> something like that.
>> The stuff going into the hardware is a value 0-3 for pull up; so these values
>> are "only" an enum with the additional benefit of saying "bias-pull-up;"
>> results in 30uA pull up which is the most commonly used form (hence being
>> optional).
> What is the nominal voltage of these pins? GIven that you can figure
> out the Ohms. And I suspect it to be something very close to N times
> the resistance of a depleted transistor in this technology.

I believe the nominal voltage changes depending on which supply you
choose (power-source in this document). Basically the gpio can be
connected to different regulators on the pmic so you can choose
different voltages, i.e. 1.8V, 3.0V, 3.3V etc. Furthermore, some of the
regulators you can choose have variable voltage, although it may not be
variable enough to have much effect on this. So it would seem that the
pull-up resistance would be directly affected by which power-source is
chosen. Maybe we just shouldn't use the generic properties for this?

BTW, I see that power-source has made a comeback. What are the units? Is
that in mV? If it is I'm slightly concerned that we're not accurately
describing the hardware in cases where the voltage can actually be
different. And I worry about configurations where we may have the same
power source muxed into the gpio twice from different places on the
pmic. Sometimes we do this and actually need to choose the "right" power
source even though they're technically running at the same voltage (one
may be slightly cleaner signal or something).

>>>> +- 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)
>>> I would really prefer to have these in mA, because the genric pinconf
>>> bindings say they should be! SI units are so much more understandable.
>> This is all the information to be found in the available documentation and
>> code. Maybe someone from Qualcomm can shed some light on it?
> Stephen?

I've emailed the hardware engineers. I'm pretty sure this is the same
story as the pull-up though. It varies depending on the input voltage. I
hope to get more information soon.

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

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Ivan T. Ivanov]

On Fri, 2014-07-11 at 18:56 -0700, Stephen Boyd wrote:
> On 07/10/14 02:53, Linus Walleij wrote:
> > On Wed, Jul 9, 2014 at 11:18 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> >> On Wed, Jul 9, 2014 at 1:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >>> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
> >>> <bjorn.andersson@sonymobile.com> wrote:
> >>>
> >>> +- function:
> >>> +       Usage: optional
> >>> +       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"
> >>> These are a bit ambigous, why doesn't the driver present functions that
> >>> are more specific than "func1", "func2"? Or "dtest1"?
> >> I agree, unfortunately I have only seen traces of the actual function matrix;
> >> for pm8xxx I have no documentation and for pm8x41 they are only listed as
> >> func[1-2] and dtest[1-4].
> >>
> >> Maybe if someone at Qualcomm could release such a list we could provide a
> >> proper table instead.
> > I guess Stephen Boyd can help us. (?)
> 
> Ok. "normal" is pretty much gpio mode, i.e. don't mux anything. "paired"
> is where we take the output of the gpio next to it and loop it back into
> this gpio (and vice versa). So gpio1 is paired with gpio2, gpio 3 is
> paired with gpio 4, etc. This allows us to make level translators by
> choosing different supply voltages for the paired gpios. "func1" and
> "func2" are used for muxing things internally. "dtest" is used to mux
> specific things out for testing purposes, not really used in any
> end-products but still useful while debugging. I can provide the
> function to pin mapping if necessary. There are lots of pmics.

Thank you Stephen. If understand it right, this is more like option for
the pin when it is GPIO. Next generation of PMIC's have support for pin
acting like analog-input/output and current sink. So I will like
to keep "function" property for selecting one of the above functions.
Choosing between "normal", "paired"... options in QPNP pinctrl driver
is supported trough passing values, defined in DT header file, to
"output-high" property. Please don't kill me :-).

> 
> >
> >>>> +- 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)
> >>> Hm, I don't know of the internal kernel API or so, but I'm thinking that
> >>> for the DT bindings, this definition should be generic in
> >>> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> >>> and put in SI units like uA.
> >> Totally agree with you; and this is already specified in pinctrl-binding.txt as
> >> being Ohm.
> >>
> >> So I first did a spin with the strength as a separate property, but as that
> >> because the only part that pinconf-generic didn't parse for me I merged it and
> >> wanted your comment on it.
> > Yeah. And thinking of it.... how can it be uA? It has to be Ohms... it's a pull
> > up resistor thing after all. I suspect the uA value is just something like the
> > maximum current drawn through the pullup given a certain voltage?
> >
> >>> So I would prefer:
> >>>
> >>> bias-pull-up = <30>;
> >>>
> >> Yeah, but that's the easy one ;)
> >>
> >> How do you say 1.5 or 31.5 and how do you differ that from 1.5 + 30 boot?
> > It needs to be set using Ohms.
> >
> >>> for 30 uA. Maybe we want nA even? I'm uncertain about the proper granularity
> >>> here :-/
> >>>
> >>> Magic enumerators 1,2,3,4 doesn't seem so good, that seems more like it's
> >>> trying to match the magic value that is to be poked into a register or
> >>> something like that.
> >> The stuff going into the hardware is a value 0-3 for pull up; so these values
> >> are "only" an enum with the additional benefit of saying "bias-pull-up;"
> >> results in 30uA pull up which is the most commonly used form (hence being
> >> optional).
> > What is the nominal voltage of these pins? GIven that you can figure
> > out the Ohms. And I suspect it to be something very close to N times
> > the resistance of a depleted transistor in this technology.
> 
> I believe the nominal voltage changes depending on which supply you
> choose (power-source in this document). Basically the gpio can be
> connected to different regulators on the pmic so you can choose
> different voltages, i.e. 1.8V, 3.0V, 3.3V etc. Furthermore, some of the
> regulators you can choose have variable voltage, although it may not be
> variable enough to have much effect on this. So it would seem that the
> pull-up resistance would be directly affected by which power-source is
> chosen. Maybe we just shouldn't use the generic properties for this?

+1 for custom property. "current-generator"? 

> 
> BTW, I see that power-source has made a comeback. What are the units? 

Documentation states it is "a custom format" :-).


Regards,
Ivan

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Stephen Boyd]

On 07/14/14 06:58, Ivan T. Ivanov wrote:
> On Fri, 2014-07-11 at 18:56 -0700, Stephen Boyd wrote:
>> On 07/10/14 02:53, Linus Walleij wrote:
>>> On Wed, Jul 9, 2014 at 11:18 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
>>>> On Wed, Jul 9, 2014 at 1:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
>>>>> <bjorn.andersson@sonymobile.com> wrote:
>>>>>
>>>>> +- function:
>>>>> +       Usage: optional
>>>>> +       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"
>>>>> These are a bit ambigous, why doesn't the driver present functions that
>>>>> are more specific than "func1", "func2"? Or "dtest1"?
>>>> I agree, unfortunately I have only seen traces of the actual function matrix;
>>>> for pm8xxx I have no documentation and for pm8x41 they are only listed as
>>>> func[1-2] and dtest[1-4].
>>>>
>>>> Maybe if someone at Qualcomm could release such a list we could provide a
>>>> proper table instead.
>>> I guess Stephen Boyd can help us. (?)
>> Ok. "normal" is pretty much gpio mode, i.e. don't mux anything. "paired"
>> is where we take the output of the gpio next to it and loop it back into
>> this gpio (and vice versa). So gpio1 is paired with gpio2, gpio 3 is
>> paired with gpio 4, etc. This allows us to make level translators by
>> choosing different supply voltages for the paired gpios. "func1" and
>> "func2" are used for muxing things internally. "dtest" is used to mux
>> specific things out for testing purposes, not really used in any
>> end-products but still useful while debugging. I can provide the
>> function to pin mapping if necessary. There are lots of pmics.
> Thank you Stephen. If understand it right, this is more like option for
> the pin when it is GPIO. Next generation of PMIC's have support for pin
> acting like analog-input/output and current sink.

Isn't this document only for the gpios? I think you're talking about the
MPPs, which also exist on these generation of pmics. We should probably
avoid mixing the two (gpios and mpps) in one binding because they're
really different hardware.

>  So I will like
> to keep "function" property for selecting one of the above functions.
> Choosing between "normal", "paired"... options in QPNP pinctrl driver
> is supported trough passing values, defined in DT header file, to
> "output-high" property. Please don't kill me :-).

Overloading output-high to choose the MPP mode doesn't seem to follow
the generic pinconfig binding. Does output-high even take a value? Why
can't we use the function property?

>> BTW, I see that power-source has made a comeback. What are the units? 
> Documentation states it is "a custom format" :-).
>
>

Ah, I was looking in the binding document. It looks like the code
comment around PIN_CONFIG_POWER_SOURCE says custom format. It's
concerning though. From what I can tell commit 5b81d55c4ccf (pinctrl:
remove bindings for pinconf options needing more thought, 2013-06-25)
removed this option along with slew-rate because they specifically
didn't have any units associated with them. But then slew-rate came back
in commit 8ba3f4d00078 (pinctrl: Adds slew-rate, input-enable/disable,
2013-12-11) even though it didn't have units and then power-source came
back without units too. Linus, what's going on?

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

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Ivan T. Ivanov]

On Mon, 2014-07-14 at 14:20 -0700, Stephen Boyd wrote:
> On 07/14/14 06:58, Ivan T. Ivanov wrote:
> > On Fri, 2014-07-11 at 18:56 -0700, Stephen Boyd wrote:
> >> On 07/10/14 02:53, Linus Walleij wrote:
> >>> On Wed, Jul 9, 2014 at 11:18 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> >>>> On Wed, Jul 9, 2014 at 1:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
> >>>>> <bjorn.andersson@sonymobile.com> wrote:
> >>>>>
> >>>>> +- function:
> >>>>> +       Usage: optional
> >>>>> +       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"
> >>>>> These are a bit ambigous, why doesn't the driver present functions that
> >>>>> are more specific than "func1", "func2"? Or "dtest1"?
> >>>> I agree, unfortunately I have only seen traces of the actual function matrix;
> >>>> for pm8xxx I have no documentation and for pm8x41 they are only listed as
> >>>> func[1-2] and dtest[1-4].
> >>>>
> >>>> Maybe if someone at Qualcomm could release such a list we could provide a
> >>>> proper table instead.
> >>> I guess Stephen Boyd can help us. (?)
> >> Ok. "normal" is pretty much gpio mode, i.e. don't mux anything. "paired"
> >> is where we take the output of the gpio next to it and loop it back into
> >> this gpio (and vice versa). So gpio1 is paired with gpio2, gpio 3 is
> >> paired with gpio 4, etc. This allows us to make level translators by
> >> choosing different supply voltages for the paired gpios. "func1" and
> >> "func2" are used for muxing things internally. "dtest" is used to mux
> >> specific things out for testing purposes, not really used in any
> >> end-products but still useful while debugging. I can provide the
> >> function to pin mapping if necessary. There are lots of pmics.
> > Thank you Stephen. If understand it right, this is more like option for
> > the pin when it is GPIO. Next generation of PMIC's have support for pin
> > acting like analog-input/output and current sink.
> 
> Isn't this document only for the gpios? I think you're talking about the
> MPPs, which also exist on these generation of pmics. We should probably
> avoid mixing the two (gpios and mpps) in one binding because they're
> really different hardware.

I don't know. For me "gpio" looks like function of the pin hardware.

> 
> >  So I will like
> > to keep "function" property for selecting one of the above functions.
> > Choosing between "normal", "paired"... options in QPNP pinctrl driver
> > is supported trough passing values, defined in DT header file, to
> > "output-high" property. Please don't kill me :-).
> 
> Overloading output-high to choose the MPP mode doesn't seem to follow
> the generic pinconfig binding. Does output-high even take a value? Why
> can't we use the function property?

No, no.  using value of the output-high|low" is just to select
"normal", "paired"... thing. Function selection is via "function" 
property. Currently QPNP support following functions "gpio", "mpp-ain",
"mpp-aout", "mpp-cs".

Regards,
Ivan

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Bjorn Andersson]

On Mon, Jul 14, 2014 at 11:35 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> On Mon, 2014-07-14 at 14:20 -0700, Stephen Boyd wrote:
[..]
>> Isn't this document only for the gpios? I think you're talking about the
>> MPPs, which also exist on these generation of pmics. We should probably
>> avoid mixing the two (gpios and mpps) in one binding because they're
>> really different hardware.
>
> I don't know. For me "gpio" looks like function of the pin hardware.
>
>>
>> >  So I will like
>> > to keep "function" property for selecting one of the above functions.
>> > Choosing between "normal", "paired"... options in QPNP pinctrl driver
>> > is supported trough passing values, defined in DT header file, to
>> > "output-high" property. Please don't kill me :-).
>>
>> Overloading output-high to choose the MPP mode doesn't seem to follow
>> the generic pinconfig binding. Does output-high even take a value? Why
>> can't we use the function property?
>
> No, no.  using value of the output-high|low" is just to select
> "normal", "paired"... thing. Function selection is via "function"
> property. Currently QPNP support following functions "gpio", "mpp-ain",
> "mpp-aout", "mpp-cs".
>

Hi Ivan,

>From your comment I presume that you don't have access to the
documentation for these blocks.

The pmic sports two types of pins; gpios and mpps (multi-purpose-pin).
These are different hardware blocks; i.e. not a configuration thing.

The gpios can be input, output or both and they can be configured as
gpio, paired, function 1 or function 2 (+ some test modes). So here it
makes sense to have the functions "gpio", "paired" and the valid
function combinations.

The mpps can be input, output or both; in either digital or analog
mode. Or they can be a current sink. When configured as analog input
you select which adc the pin should be routed to. Here it makes sense
to have the functions "digital", "analog" and "current-sink" I think.

Regards,
Bjorn

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Ivan T. Ivanov]

On Tue, 2014-07-15 at 17:23 -0700, Bjorn Andersson wrote:
> On Mon, Jul 14, 2014 at 11:35 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > On Mon, 2014-07-14 at 14:20 -0700, Stephen Boyd wrote:
> [..]
> >> Isn't this document only for the gpios? I think you're talking about the
> >> MPPs, which also exist on these generation of pmics. We should probably
> >> avoid mixing the two (gpios and mpps) in one binding because they're
> >> really different hardware.
> >
> > I don't know. For me "gpio" looks like function of the pin hardware.
> >
> >>
> >> >  So I will like
> >> > to keep "function" property for selecting one of the above functions.
> >> > Choosing between "normal", "paired"... options in QPNP pinctrl driver
> >> > is supported trough passing values, defined in DT header file, to
> >> > "output-high" property. Please don't kill me :-).
> >>
> >> Overloading output-high to choose the MPP mode doesn't seem to follow
> >> the generic pinconfig binding. Does output-high even take a value? Why
> >> can't we use the function property?
> >
> > No, no.  using value of the output-high|low" is just to select
> > "normal", "paired"... thing. Function selection is via "function"
> > property. Currently QPNP support following functions "gpio", "mpp-ain",
> > "mpp-aout", "mpp-cs".
> >
> 
> Hi Ivan,
> 
> From your comment I presume that you don't have access to the
> documentation for these blocks.
> 
> The pmic sports two types of pins; gpios and mpps (multi-purpose-pin).
> These are different hardware blocks; i.e. not a configuration thing.

I am looking on GPIO's hardware blocks as stripped down MPP's.

> 
> The gpios can be input, output or both and they can be configured as
> gpio, paired, function 1 or function 2 (+ some test modes). So here it
> makes sense to have the functions "gpio", "paired" and the valid
> function combinations.

I believe that MPP's also support these 'functions'.

> 
> The mpps can be input, output or both; in either digital or analog
> mode. Or they can be a current sink. When configured as analog input
> you select which adc the pin should be routed to. Here it makes sense
> to have the functions "digital", "analog" and "current-sink" I think.

My understanding is that MPP's are supporting everything which GPIO's
does, plus analog functionality. 

I don't want at the end MPP's functions to be something like:

analog-normal, analog-paired, analog-function-1, analog-function-2, 
digital-normal, digital-paired...

So better to leave normal/paired stuff as separate parameter (qcom,pair).

Plain GPIO hardware will just support 'gpio' function, while MPP's
will have 'gpio', 'analog-in', 'analog-out', 'current-sink' 

I am fine to split driver to two drivers (GPIO and MPP), which will
make code a little more clean, but I am pretty sure there will be
lot of duplication.

Regards,
Ivan

[PATCH 2/3] pinctrl: Device tree bindings for Qualcomm pm8xxx gpio block

[Ivan T. Ivanov]

On Wed, 2014-07-09 at 14:18 -0700, Bjorn Andersson wrote:
> On Wed, Jul 9, 2014 at 1:53 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
> > <bjorn.andersson@sonymobile.com> wrote:


> [...]
> >> +
> >> +       pm8921_gpio: gpio at 150 {
> >> +               compatible = "qcom,pm8921-gpio";
> >> +               reg = <0x150>;
> >> +               interrupts = <192 1>, <193 1>, <194 1>,
> >> +                            <195 1>, <196 1>, <197 1>,
> >> +                            <198 1>, <199 1>, <200 1>,
> >> +                            <201 1>, <202 1>, <203 1>,
> >> +                            <204 1>, <205 1>, <206 1>,
> >> +                            <207 1>, <208 1>, <209 1>,
> >> +                            <210 1>, <211 1>, <212 1>,
> >> +                            <213 1>, <214 1>, <215 1>,
> >> +                            <216 1>, <217 1>, <218 1>,
> >> +                            <219 1>, <220 1>, <221 1>,
> >> +                            <222 1>, <223 1>, <224 1>,
> >> +                            <225 1>, <226 1>, <227 1>,
> >> +                            <228 1>, <229 1>, <230 1>,
> >> +                            <231 1>, <232 1>, <233 1>,
> >> +                            <234 1>, <235 1>;
> >
> >
> > So this looks a bit weird. But if I just get to understand the hardware
> > I guess it won't anymore.
> >
> > So there is an interrupt parent to which the IRQ lines from the PMIC
> > are routed back through external lines to IRQ offsets 192 thru 235?
> >
> 
> The pm8921-core exposes 256 interrupts, the listed 44 interrupts here are what
> comes out of that.
> 
> I was really reluctant to list all the interrupts, but I think it turned out
> nicer than any of my other attempts; like only providing a base and then
> relying on interrupts being consecutive.
> 
> Suggestions on how this "should" be solved are welcome, as we have the same
> setup for the newer pmics (Ivan's patches) and the TLMM hardware (pinctrl-msm)
> supports using dedicated interrupts for certain gpio pins (instead of passing
> through the chain handler).

This is something that is already known in the driver, numbers did not
change at run time, right? Could we hard-code IRQ base in driver, like 
"ti,palmas-gpio" did? reg property is also not strictly required, but
this is different story :-).

Regards,
Ivan

[PATCH 3/3] pinctrl: Introduce pinctrl driver for Qualcomm pm8xxx

[Bjorn Andersson]

This introduces a pinctrl, pinconf, pinmux and gpio driver for the gpio
block found in pm8018, pm8038, pm8058, pm8917 and pm8921 pmics from
Qualcomm.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/pinctrl/Kconfig               |   11 +
 drivers/pinctrl/Makefile              |    1 +
 drivers/pinctrl/pinctrl-pm8xxx-gpio.c |  796 +++++++++++++++++++++++++++++++++
 3 files changed, 808 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-pm8xxx-gpio.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 0042ccb..c173db6 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -278,6 +278,17 @@ config PINCTRL_DB8540
 	bool "DB8540 pin controller driver"
 	depends on PINCTRL_NOMADIK && ARCH_U8500
 
+config PINCTRL_PM8XXX_GPIO
+	tristate "Qualcomm PM8xxx gpio driver"
+	depends on GPIOLIB && OF && (ARCH_QCOM || COMPILE_TEST)
+	select PINCONF
+	select PINMUX
+	select GENERIC_PINCONF
+	help
+	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
+	  Qualcomm GPIO blocks found in the pm8018, pm8038, pm8058, pm8917 and
+	  pm8921 pmics from Qualcomm.
+
 config PINCTRL_ROCKCHIP
 	bool
 	select PINMUX
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index c4b5d40..806f6ad 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_PINCTRL_STN8815)	+= pinctrl-nomadik-stn8815.o
 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_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
diff --git a/drivers/pinctrl/pinctrl-pm8xxx-gpio.c b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
new file mode 100644
index 0000000..5aaf914
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
@@ -0,0 +1,796 @@
+/*
+ * Copyright (c) 2014, Sony Mobile Communications AB.
+ * Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/slab.h>
+#include <linux/regmap.h>
+#include <linux/gpio.h>
+#include <linux/mfd/pm8921-core.h>
+
+#include <dt-bindings/pinctrl/qcom,pm8xxx-gpio.h>
+
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+
+/* direction */
+#define PM8XXX_GPIO_DIR_OUT		BIT(0)
+#define PM8XXX_GPIO_DIR_IN		BIT(1)
+
+/* output buffer */
+#define PM8XXX_GPIO_PUSH_PULL		0
+#define PM8XXX_GPIO_OPEN_DRAIN		1
+
+/* bias */
+#define PM8XXX_GPIO_BIAS_PU_30		0
+#define PM8XXX_GPIO_BIAS_PU_1P5		1
+#define PM8XXX_GPIO_BIAS_PU_31P5	2
+#define PM8XXX_GPIO_BIAS_PU_1P5_30	3
+#define PM8XXX_GPIO_BIAS_PD		4
+#define PM8XXX_GPIO_BIAS_NP		5
+
+/* GPIO registers */
+#define SSBI_REG_ADDR_GPIO_BASE		0x150
+#define SSBI_REG_ADDR_GPIO(n)		(SSBI_REG_ADDR_GPIO_BASE + n)
+
+#define PM8XXX_GPIO_WRITE		BIT(7)
+
+#define PM8XXX_MAX_GPIOS		44
+
+struct pm8xxx_gpio_pin {
+	int irq;
+
+	u8 power_source;
+	u8 direction;
+	u8 output_buffer;
+	u8 output_value;
+	u8 bias;
+	u8 output_strength;
+	u8 disable;
+	u8 function;
+	u8 non_inverted;
+};
+
+struct pm8xxx_gpio_data {
+	int ngpio;
+	const int *power_sources;
+	int npower_sources;
+};
+
+struct pm8xxx_gpio {
+	struct device *dev;
+	struct regmap *regmap;
+	struct pinctrl_dev *pctrl;
+	struct gpio_chip chip;
+
+	const struct pm8xxx_gpio_data *data;
+
+	struct pm8xxx_gpio_pin pins[PM8XXX_MAX_GPIOS];
+};
+
+static const char * const pm8xxx_gpio_groups[PM8XXX_MAX_GPIOS] = {
+	"gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", "gpio7", "gpio8",
+	"gpio9", "gpio10", "gpio11", "gpio12", "gpio13", "gpio14", "gpio15",
+	"gpio16", "gpio17", "gpio18", "gpio19", "gpio20", "gpio21", "gpio22",
+	"gpio23", "gpio24", "gpio25", "gpio26", "gpio27", "gpio28", "gpio29",
+	"gpio30", "gpio31", "gpio32", "gpio33", "gpio34", "gpio35", "gpio36",
+	"gpio37", "gpio38", "gpio39", "gpio40", "gpio41", "gpio42", "gpio43",
+	"gpio44",
+};
+
+static const char * const pm8xxx_gpio_functions[] = {
+	"normal", "paired",
+	"func1", "func2",
+	"dtest1", "dtest2", "dtest3", "dtest4",
+};
+
+static int pm8xxx_gpio_read(struct pm8xxx_gpio *pctrl, int pin, int bank)
+{
+	int reg = SSBI_REG_ADDR_GPIO(pin);
+	unsigned int val = bank << 4;
+	int ret;
+
+	ret = regmap_write(pctrl->regmap, reg, val);
+	if (ret) {
+		dev_err(pctrl->dev,
+			"failed to select bank %d of pin %d\n", bank, pin);
+		return ret;
+	}
+
+	ret = regmap_read(pctrl->regmap, reg, &val);
+	if (ret) {
+		dev_err(pctrl->dev,
+			"failed to read register %d of pin %d\n", bank, pin);
+		return ret;
+	}
+
+	return val;
+}
+
+static int pm8xxx_gpio_write(struct pm8xxx_gpio *pctrl,
+			     int pin, int bank, u8 val)
+{
+	int ret;
+
+	val |= PM8XXX_GPIO_WRITE;
+	val |= bank << 4;
+
+	ret = regmap_write(pctrl->regmap, SSBI_REG_ADDR_GPIO(pin), val);
+	if (ret)
+		dev_err(pctrl->dev, "failed to write register\n");
+
+	return ret;
+}
+
+static int pm8xxx_gpio_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctrl->data->ngpio;
+}
+
+static const char *pm8xxx_gpio_get_group_name(struct pinctrl_dev *pctldev,
+				      unsigned group)
+{
+	return pm8xxx_gpio_groups[group];
+}
+
+static const struct pinctrl_ops pm8xxx_gpio_pinctrl_ops = {
+	.get_groups_count	= pm8xxx_gpio_get_groups_count,
+	.get_group_name		= pm8xxx_gpio_get_group_name,
+	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
+	.dt_free_map		= pinctrl_utils_dt_free_map,
+};
+
+static int pm8xxx_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(pm8xxx_gpio_functions);
+}
+
+static const char *pm8xxx_get_function_name(struct pinctrl_dev *pctldev,
+					 unsigned function)
+{
+	return pm8xxx_gpio_functions[function];
+}
+
+static int pm8xxx_get_function_groups(struct pinctrl_dev *pctldev,
+				   unsigned function,
+				   const char * const **groups,
+				   unsigned * const num_groups)
+{
+	struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pm8xxx_gpio_groups;
+	*num_groups = pctrl->data->ngpio;
+	return 0;
+}
+
+static int pm8xxx_pinmux_enable(struct pinctrl_dev *pctldev,
+			     unsigned function,
+			     unsigned group)
+{
+	struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[group];
+	u8 val;
+
+	pin->function = function;
+	val = pin->function << 1;
+
+	pm8xxx_gpio_write(pctrl, group, 4, val);
+
+	return 0;
+}
+
+static void pm8xxx_pinmux_disable(struct pinctrl_dev *pctldev,
+			       unsigned function,
+			       unsigned group)
+{
+	struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[group];
+	u8 val;
+
+	pin->function = 0;
+	val = pin->function << 1;
+
+	pm8xxx_gpio_write(pctrl, group, 4, val);
+}
+
+static const struct pinmux_ops pm8xxx_pinmux_ops = {
+	.get_functions_count	= pm8xxx_get_functions_count,
+	.get_function_name	= pm8xxx_get_function_name,
+	.get_function_groups	= pm8xxx_get_function_groups,
+	.enable			= pm8xxx_pinmux_enable,
+	.disable		= pm8xxx_pinmux_disable,
+};
+
+static int pm8xxx_gpio_config_get(struct pinctrl_dev *pctldev,
+			  unsigned int offset,
+			  unsigned long *config)
+{
+	struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset];
+	unsigned param = pinconf_to_config_param(*config);
+	unsigned arg;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		arg = pin->bias == PM8XXX_GPIO_BIAS_NP;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		arg = pin->bias == PM8XXX_GPIO_BIAS_PD;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (pin->bias >= PM8XXX_GPIO_BIAS_PU_30 &&
+		    pin->bias <= PM8XXX_GPIO_BIAS_PU_1P5_30)
+			arg = PM8XXX_GPIO_PULL_UP_30 + pin->bias;
+		else
+			arg = 0;
+		break;
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		arg = pin->disable;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+		arg = pin->direction == PM8XXX_GPIO_DIR_IN;
+		break;
+	case PIN_CONFIG_OUTPUT:
+		arg = pin->output_value;
+		break;
+	case PIN_CONFIG_POWER_SOURCE:
+		arg = pin->power_source;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		arg = pin->output_strength;
+		break;
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		arg = pin->output_buffer == PM8XXX_GPIO_PUSH_PULL;
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		arg = pin->output_buffer == PM8XXX_GPIO_OPEN_DRAIN;
+		break;
+	default:
+		dev_err(pctrl->dev,
+			"unsupported config parameter: %x\n",
+			param);
+		return -EINVAL;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int resolve_power_source(struct pm8xxx_gpio *pctrl, unsigned arg)
+{
+	const struct pm8xxx_gpio_data *data = pctrl->data;
+	int i;
+
+	for (i = 0; i < data->npower_sources; i++) {
+		if (data->power_sources[i] == arg)
+			return i;
+	}
+
+	dev_err(pctrl->dev, "invalid power source\n");
+	return -EINVAL;
+}
+
+static int pm8xxx_gpio_config_set(struct pinctrl_dev *pctldev,
+				  unsigned int offset,
+				  unsigned long *configs,
+				  unsigned num_configs)
+{
+	struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset];
+	unsigned param;
+	unsigned arg;
+	unsigned i;
+	int ret;
+	u8 banks = 0;
+	u8 val;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_DISABLE:
+			pin->bias = PM8XXX_GPIO_BIAS_NP;
+			banks |= BIT(2);
+			pin->disable = 0;
+			banks |= BIT(3);
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			pin->bias = PM8XXX_GPIO_BIAS_PD;
+			banks |= BIT(2);
+			pin->disable = 0;
+			banks |= BIT(3);
+			break;
+		case PIN_CONFIG_BIAS_PULL_UP:
+			if (arg < PM8XXX_GPIO_PULL_UP_30 ||
+			    arg > PM8XXX_GPIO_PULL_UP_1P5_30) {
+				dev_err(pctrl->dev, "invalid pull-up level\n");
+				return -EINVAL;
+			}
+			pin->bias = arg - PM8XXX_GPIO_BIAS_PU_30;
+			banks |= BIT(2);
+			pin->disable = 0;
+			banks |= BIT(3);
+			break;
+		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+			pin->disable = 1;
+			banks |= BIT(3);
+			break;
+		case PIN_CONFIG_INPUT_ENABLE:
+			pin->direction = PM8XXX_GPIO_DIR_IN;
+			banks |= BIT(1);
+			break;
+		case PIN_CONFIG_OUTPUT:
+			pin->direction = PM8XXX_GPIO_DIR_OUT;
+			pin->output_value = !!arg;
+			banks |= BIT(1);
+			break;
+		case PIN_CONFIG_POWER_SOURCE:
+			/* Sanity check the power source */
+			ret = resolve_power_source(pctrl, arg);
+			if (ret < 0)
+				return ret;
+			pin->power_source = arg;
+			banks |= BIT(0);
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			if (arg > PM8XXX_GPIO_STRENGTH_LOW) {
+				dev_err(pctrl->dev, "invalid drive strength\n");
+				return -EINVAL;
+			}
+			pin->output_strength = arg;
+			banks |= BIT(3);
+			break;
+		case PIN_CONFIG_DRIVE_PUSH_PULL:
+			pin->output_buffer = PM8XXX_GPIO_PUSH_PULL;
+			banks |= BIT(1);
+			break;
+		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+			pin->output_buffer = PM8XXX_GPIO_OPEN_DRAIN;
+			banks |= BIT(1);
+			break;
+		default:
+			dev_err(pctrl->dev,
+					"unsupported config parameter: %x\n",
+					param);
+			return -EINVAL;
+		}
+	}
+
+	if (banks & BIT(0)) {
+		ret = resolve_power_source(pctrl, pin->power_source);
+		pm8xxx_gpio_write(pctrl, offset, 0, ret << 1);
+	}
+
+	if (banks & BIT(1)) {
+		val = pin->direction << 2;
+		val |= pin->output_buffer << 1;
+		val |= pin->output_value;
+		pm8xxx_gpio_write(pctrl, offset, 1, val);
+	}
+
+	if (banks & BIT(2)) {
+		val = pin->bias << 1;
+		pm8xxx_gpio_write(pctrl, offset, 2, val);
+	}
+
+	if (banks & BIT(3)) {
+		val = pin->output_strength << 2;
+		val |= pin->disable;
+		pm8xxx_gpio_write(pctrl, offset, 3, val);
+	}
+
+	if (banks & BIT(4)) {
+		val = pin->function << 1;
+		pm8xxx_gpio_write(pctrl, offset, 4, val);
+	}
+
+	if (banks & BIT(5)) {
+		val = 0;
+		if (pin->non_inverted)
+			val |= BIT(3);
+		pm8xxx_gpio_write(pctrl, offset, 5, val);
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops pm8xxx_gpio_pinconf_ops = {
+	.pin_config_group_get = pm8xxx_gpio_config_get,
+	.pin_config_group_set = pm8xxx_gpio_config_set,
+};
+
+static struct pinctrl_desc pm8xxx_gpio_desc = {
+	.pctlops = &pm8xxx_gpio_pinctrl_ops,
+	.pmxops = &pm8xxx_pinmux_ops,
+	.confops = &pm8xxx_gpio_pinconf_ops,
+	.owner = THIS_MODULE,
+};
+
+static int pm8xxx_gpio_direction_input(struct gpio_chip *chip,
+				       unsigned offset)
+{
+	struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
+	u8 val;
+
+	pin->direction = PM8XXX_GPIO_DIR_IN;
+	val = pin->direction << 2;
+
+	pm8xxx_gpio_write(pctrl, offset, 1, val);
+
+	return 0;
+}
+
+static int pm8xxx_gpio_direction_output(struct gpio_chip *chip,
+					unsigned offset,
+					int value)
+{
+	struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
+	u8 val;
+
+	pin->direction = PM8XXX_GPIO_DIR_OUT;
+	pin->output_value = !!value;
+
+	val = pin->direction << 2;
+	val |= pin->output_buffer << 1;
+	val |= pin->output_value;
+
+	pm8xxx_gpio_write(pctrl, offset, 1, val);
+
+	return 0;
+}
+
+static int pm8xxx_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
+
+	if (pin->direction == PM8XXX_GPIO_DIR_OUT)
+		return pin->output_value;
+	else
+		return pm8xxx_read_irq_status(pin->irq);
+}
+
+static void pm8xxx_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct pm8xxx_gpio *pctrl = container_of(gc, struct pm8xxx_gpio, chip);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
+	u8 val;
+
+	pin->output_value = !!value;
+
+	val = pin->direction << 2;
+	val |= pin->output_buffer << 1;
+	val |= pin->output_value;
+
+	pm8xxx_gpio_write(pctrl, offset, 1, val);
+}
+
+static int pm8xxx_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
+
+	return pin->irq;
+}
+
+#ifdef CONFIG_DEBUG_FS
+#include <linux/seq_file.h>
+
+static void pm8xxx_gpio_dbg_show_one(struct seq_file *s,
+				  struct pinctrl_dev *pctldev,
+				  struct gpio_chip *chip,
+				  unsigned offset,
+				  unsigned gpio)
+{
+	struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
+	struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset];
+
+	static const char * const directions[] = {
+		"off", "out", "in", "both"
+	};
+	static const char * const sources[] = {
+		"bb", "l2", "l3", "l4", "l5", "l6", "l7", "l8", "l11",
+		"l14", "l15", "l17", "s3", "s4", "vph"
+	};
+	static const char * const biases[] = {
+		"pull-up 30uA", "pull-up 1.5uA", "pull-up 31.5uA",
+		"pull-up 1.5uA + 30uA boost", "pull-down 10uA", "no pull"
+	};
+	static const char * const buffer_types[] = {
+		"push-pull", "open-drain"
+	};
+	static const char * const strengths[] = {
+		"no", "high", "medium", "low"
+	};
+
+	seq_printf(s, " gpio%-2d:", offset + 1);
+	if (pin->disable) {
+		seq_puts(s, " ---");
+	} else {
+		seq_printf(s, " %-4s", directions[pin->direction]);
+		seq_printf(s, " %-7s", pm8xxx_gpio_functions[pin->function]);
+		seq_printf(s, " %-3s", sources[pin->power_source]);
+		seq_printf(s, " %-27s", biases[pin->bias]);
+		seq_printf(s, " %-10s", buffer_types[pin->output_buffer]);
+		seq_printf(s, " %-4s", pin->output_value ? "high" : "low");
+		seq_printf(s, " %-7s", strengths[pin->output_strength]);
+		if (!pin->non_inverted)
+			seq_puts(s, " inverted");
+	}
+}
+
+static void pm8xxx_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+	unsigned gpio = chip->base;
+	unsigned i;
+
+	for (i = 0; i < chip->ngpio; i++, gpio++) {
+		pm8xxx_gpio_dbg_show_one(s, NULL, chip, i, gpio);
+		seq_puts(s, "\n");
+	}
+}
+
+#else
+#define msm_gpio_dbg_show NULL
+#endif
+
+static struct gpio_chip pm8xxx_gpio_template = {
+	.direction_input = pm8xxx_gpio_direction_input,
+	.direction_output = pm8xxx_gpio_direction_output,
+	.get = pm8xxx_gpio_get,
+	.set = pm8xxx_gpio_set,
+	.to_irq = pm8xxx_gpio_to_irq,
+	.dbg_show = pm8xxx_gpio_dbg_show,
+	.owner = THIS_MODULE,
+};
+
+static int pm8xxx_gpio_populate(struct pm8xxx_gpio *pctrl)
+{
+	const struct pm8xxx_gpio_data *data = pctrl->data;
+	struct pm8xxx_gpio_pin *pin;
+	int val;
+	int i;
+
+	for (i = 0; i < pctrl->data->ngpio; i++) {
+		pin = &pctrl->pins[i];
+
+		val = pm8xxx_gpio_read(pctrl, i, 0);
+		if (val < 0)
+			return val;
+
+		pin->power_source = data->power_sources[(val >> 1) & 0x7];
+
+		val = pm8xxx_gpio_read(pctrl, i, 1);
+		if (val < 0)
+			return val;
+
+		pin->direction = (val >> 2) & 0x3;
+		pin->output_buffer = !!(val & BIT(1));
+		pin->output_value = val & BIT(0);
+
+		val = pm8xxx_gpio_read(pctrl, i, 2);
+		if (val < 0)
+			return val;
+
+		pin->bias = (val >> 1) & 0x7;
+
+		val = pm8xxx_gpio_read(pctrl, i, 3);
+		if (val < 0)
+			return val;
+
+		pin->output_strength = (val >> 2) & 0x3;
+		pin->disable = val & BIT(0);
+
+		val = pm8xxx_gpio_read(pctrl, i, 4);
+		if (val < 0)
+			return val;
+
+		pin->function = (val >> 1) & 0x7;
+
+		val = pm8xxx_gpio_read(pctrl, i, 5);
+		if (val < 0)
+			return val;
+
+		pin->non_inverted = !!(val & BIT(3));
+	}
+
+	return 0;
+}
+
+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
+	},
+	.npower_sources = 8,
+};
+
+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
+	},
+	.npower_sources = 7,
+};
+
+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
+	},
+	.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
+	},
+	.npower_sources = 7,
+};
+
+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
+	},
+	.npower_sources = 7,
+};
+
+static const struct of_device_id pm8xxx_gpio_of_match[] = {
+	{ .compatible = "qcom,pm8018-gpio", .data = &pm8018_gpio_data },
+	{ .compatible = "qcom,pm8038-gpio", .data = &pm8038_gpio_data },
+	{ .compatible = "qcom,pm8058-gpio", .data = &pm8058_gpio_data },
+	{ .compatible = "qcom,pm8917-gpio", .data = &pm8917_gpio_data },
+	{ .compatible = "qcom,pm8921-gpio", .data = &pm8921_gpio_data },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pm8xxx_gpio_of_match);
+
+static int pm8xxx_gpio_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct pm8xxx_gpio *pctrl;
+	int ret;
+	int i;
+
+	match = of_match_node(pm8xxx_gpio_of_match, pdev->dev.of_node);
+	if (!match)
+		return -ENXIO;
+
+	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->dev = &pdev->dev;
+	pctrl->data = match->data;
+
+	BUG_ON(pctrl->data->ngpio > PM8XXX_MAX_GPIOS);
+
+	pctrl->chip = pm8xxx_gpio_template;
+	pctrl->chip.base = -1;
+	pctrl->chip.dev = &pdev->dev;
+	pctrl->chip.of_node = pdev->dev.of_node;
+	pctrl->chip.label = dev_name(pctrl->dev);
+	pctrl->chip.ngpio = pctrl->data->ngpio;
+
+	pctrl->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!pctrl->regmap) {
+		dev_err(&pdev->dev, "parent regmap unavailable\n");
+		return -ENXIO;
+	}
+
+	for (i = 0; i < pctrl->data->ngpio; i++) {
+		ret = platform_get_irq(pdev, i);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"missing interrupts for pin %d\n", i);
+			return ret;
+		}
+
+		pctrl->pins[i].irq = ret;
+	}
+
+	ret = pm8xxx_gpio_populate(pctrl);
+	if (ret)
+		return ret;
+
+	pm8xxx_gpio_desc.name = dev_name(&pdev->dev);
+	pctrl->pctrl = pinctrl_register(&pm8xxx_gpio_desc, &pdev->dev, pctrl);
+	if (!pctrl->pctrl) {
+		dev_err(&pdev->dev, "couldn't register pm8xxx gpio driver\n");
+		return -ENODEV;
+	}
+
+	ret = gpiochip_add(&pctrl->chip);
+	if (ret) {
+		dev_err(&pdev->dev, "failed register gpiochip\n");
+		goto unregister_pinctrl;
+	}
+
+	ret = gpiochip_add_pin_range(&pctrl->chip,
+				     dev_name(pctrl->dev),
+				     1, 0, pctrl->data->ngpio);
+	if (ret) {
+		dev_err(pctrl->dev, "failed to add pin range\n");
+		goto unregister_gpiochip;
+	}
+
+	platform_set_drvdata(pdev, pctrl);
+
+	dev_dbg(&pdev->dev, "Qualcomm pm8xxx gpio driver probed\n");
+
+	return 0;
+
+unregister_pinctrl:
+	pinctrl_unregister(pctrl->pctrl);
+
+unregister_gpiochip:
+	if (gpiochip_remove(&pctrl->chip))
+		dev_err(&pdev->dev, "unable to unregister gpiochip\n");
+
+	return ret;
+}
+
+static int pm8xxx_gpio_remove(struct platform_device *pdev)
+{
+	struct pm8xxx_gpio *pctrl = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = gpiochip_remove(&pctrl->chip);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to remove gpiochip\n");
+		return ret;
+	}
+
+	pinctrl_unregister(pctrl->pctrl);
+
+	return 0;
+}
+
+static struct platform_driver pm8xxx_gpio_driver = {
+	.driver = {
+		.name = "pm8xxx_gpio",
+		.owner = THIS_MODULE,
+		.of_match_table = pm8xxx_gpio_of_match,
+	},
+	.probe = pm8xxx_gpio_probe,
+	.remove = pm8xxx_gpio_remove,
+};
+
+module_platform_driver(pm8xxx_gpio_driver);
+
+MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>");
+MODULE_DESCRIPTION("Qualcomm PM8xxx GPIO driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH 3/3] pinctrl: Introduce pinctrl driver for Qualcomm pm8xxx

[Linus Walleij]

On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:

> This introduces a pinctrl, pinconf, pinmux and gpio driver for the gpio
> block found in pm8018, pm8038, pm8058, pm8917 and pm8921 pmics from
> Qualcomm.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

(...)

> +config PINCTRL_PM8XXX_GPIO
> +       tristate "Qualcomm PM8xxx gpio driver"

I would prefer PINCTRL_PM8XXX simply. GPIO is just one aspect of what
this block inside PM8XXX is doing, it is a bit confusing.

> +++ b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c

Likewise name the file and all functions just pinctrl-pm8xxx or so, reserve
the "gpio" string in function names for functions that really just deal with
the gpiochip.

> +struct pm8xxx_gpio_pin {
> +       int irq;
> +
> +       u8 power_source;
> +       u8 direction;
> +       u8 output_buffer;
> +       u8 output_value;
> +       u8 bias;
> +       u8 output_strength;
> +       u8 disable;
> +       u8 function;
> +       u8 non_inverted;
> +};

This struct really needs kerneldoc, as I guess it is what will be reflected
in dbg_view messages etc too.

Some u8:s seem dubious. Like why isn't "disable" a bool?

Why is there a property "non_inverted", like being inverted was the
normal state? Isn't it a bool inverted rather?

"direction" also seems a bit bool ... even "output_value".

Such things.

> +struct pm8xxx_gpio_data {
> +       int ngpio;

Can this be negative or should it be unsigned?
Plus the usage in the code seems to be npins rather than
ngpio. It indicates the number of pins this driver can handle,
that they also do GPIO is just one aspect of their full pin potential...

> +       const int *power_sources;
> +       int npower_sources;

Dito.

> +};
> +
> +struct pm8xxx_gpio {
> +       struct device *dev;
> +       struct regmap *regmap;
> +       struct pinctrl_dev *pctrl;
> +       struct gpio_chip chip;
> +
> +       const struct pm8xxx_gpio_data *data;
> +
> +       struct pm8xxx_gpio_pin pins[PM8XXX_MAX_GPIOS];

No thanks. Use the .pins in struct pinctrl_desc.
(Details below.)

> +};
> +
> +static const char * const pm8xxx_gpio_groups[PM8XXX_MAX_GPIOS] = {
> +       "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", "gpio7", "gpio8",
> +       "gpio9", "gpio10", "gpio11", "gpio12", "gpio13", "gpio14", "gpio15",
> +       "gpio16", "gpio17", "gpio18", "gpio19", "gpio20", "gpio21", "gpio22",
> +       "gpio23", "gpio24", "gpio25", "gpio26", "gpio27", "gpio28", "gpio29",
> +       "gpio30", "gpio31", "gpio32", "gpio33", "gpio34", "gpio35", "gpio36",
> +       "gpio37", "gpio38", "gpio39", "gpio40", "gpio41", "gpio42", "gpio43",
> +       "gpio44",
> +};
> +
> +static const char * const pm8xxx_gpio_functions[] = {
> +       "normal", "paired",
> +       "func1", "func2",
> +       "dtest1", "dtest2", "dtest3", "dtest4",
> +};

What is a bit strange is that the driver does not contain an array of the
actual pin names, just groups and functions? It is usually very helpful
to name all the individual pins.

> +static int pm8xxx_gpio_read(struct pm8xxx_gpio *pctrl, int pin, int bank)

I would prefer if this was named pm8xxx_pinctrl_read() or similar, so we
get the GPIO ambiguity out of the way.

> +{
> +       int reg = SSBI_REG_ADDR_GPIO(pin);

But I guess the registers may be named "GPIO" (something something)
in the datasheet because the HW and spec authors are in the gpiomode
pitfall we describe in the pinctrl.txt document. Like they think muxing
and electrical config is "something GPIO" you know. So if this is matching
a datasheet then keep that designation.

> +static int pm8xxx_gpio_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +       struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +       return pctrl->data->ngpio;
> +}

So I suggest renaming that ->npins.

> +static void pm8xxx_pinmux_disable(struct pinctrl_dev *pctldev,
> +                              unsigned function,
> +                              unsigned group)
> +{
> +       struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +       struct pm8xxx_gpio_pin *pin = &pctrl->pins[group];
> +       u8 val;
> +
> +       pin->function = 0;
> +       val = pin->function << 1;
> +
> +       pm8xxx_gpio_write(pctrl, group, 4, val);
> +}

We just got rid of this function from the ops becuase it was just
causing trouble in the concepts so just delete it :-)

> +static int pm8xxx_gpio_config_get(struct pinctrl_dev *pctldev,
> +                         unsigned int offset,
> +                         unsigned long *config)
> +{

Kudos for using generic pinconf, that makes everything much easier.

> +       struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +       struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset];
> +       unsigned param = pinconf_to_config_param(*config);
> +       unsigned arg;
> +
> +       switch (param) {
> +       case PIN_CONFIG_BIAS_DISABLE:
> +               arg = pin->bias == PM8XXX_GPIO_BIAS_NP;
> +               break;
> +       case PIN_CONFIG_BIAS_PULL_DOWN:
> +               arg = pin->bias == PM8XXX_GPIO_BIAS_PD;
> +               break;
> +       case PIN_CONFIG_BIAS_PULL_UP:
> +               if (pin->bias >= PM8XXX_GPIO_BIAS_PU_30 &&
> +                   pin->bias <= PM8XXX_GPIO_BIAS_PU_1P5_30)
> +                       arg = PM8XXX_GPIO_PULL_UP_30 + pin->bias;
> +               else
> +                       arg = 0;
> +               break;

So here I expect SI unit conversion instead.

> +       case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +               arg = pin->disable;
> +               break;
> +       case PIN_CONFIG_INPUT_ENABLE:
> +               arg = pin->direction == PM8XXX_GPIO_DIR_IN;
> +               break;
> +       case PIN_CONFIG_OUTPUT:
> +               arg = pin->output_value;
> +               break;
> +       case PIN_CONFIG_POWER_SOURCE:
> +               arg = pin->power_source;
> +               break;
> +       case PIN_CONFIG_DRIVE_STRENGTH:
> +               arg = pin->output_strength;
> +               break;

And here.

> +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> +               arg = pin->output_buffer == PM8XXX_GPIO_PUSH_PULL;
> +               break;
> +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +               arg = pin->output_buffer == PM8XXX_GPIO_OPEN_DRAIN;
> +               break;
> +       default:
> +               dev_err(pctrl->dev,
> +                       "unsupported config parameter: %x\n",
> +                       param);
> +               return -EINVAL;
> +       }
> +
> +       *config = pinconf_to_config_packed(param, arg);
> +
> +       return 0;
> +}

(...)
> +static int pm8xxx_gpio_config_set(struct pinctrl_dev *pctldev,
> +                                 unsigned int offset,
> +                                 unsigned long *configs,
> +                                 unsigned num_configs)
> +{
> +       struct pm8xxx_gpio *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +       struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset];
> +       unsigned param;
> +       unsigned arg;
> +       unsigned i;
> +       int ret;
> +       u8 banks = 0;
> +       u8 val;
> +
> +       for (i = 0; i < num_configs; i++) {
> +               param = pinconf_to_config_param(configs[i]);
> +               arg = pinconf_to_config_argument(configs[i]);
> +
> +               switch (param) {
> +               case PIN_CONFIG_BIAS_DISABLE:
> +                       pin->bias = PM8XXX_GPIO_BIAS_NP;
> +                       banks |= BIT(2);
> +                       pin->disable = 0;
> +                       banks |= BIT(3);
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_DOWN:
> +                       pin->bias = PM8XXX_GPIO_BIAS_PD;
> +                       banks |= BIT(2);
> +                       pin->disable = 0;
> +                       banks |= BIT(3);
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_UP:
> +                       if (arg < PM8XXX_GPIO_PULL_UP_30 ||
> +                           arg > PM8XXX_GPIO_PULL_UP_1P5_30) {
> +                               dev_err(pctrl->dev, "invalid pull-up level\n");
> +                               return -EINVAL;
> +                       }
> +                       pin->bias = arg - PM8XXX_GPIO_BIAS_PU_30;

Proper SI unit...

> +                       banks |= BIT(2);
> +                       pin->disable = 0;
> +                       banks |= BIT(3);
> +                       break;
> +               case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +                       pin->disable = 1;
> +                       banks |= BIT(3);
> +                       break;
> +               case PIN_CONFIG_INPUT_ENABLE:
> +                       pin->direction = PM8XXX_GPIO_DIR_IN;
> +                       banks |= BIT(1);
> +                       break;
> +               case PIN_CONFIG_OUTPUT:
> +                       pin->direction = PM8XXX_GPIO_DIR_OUT;
> +                       pin->output_value = !!arg;
> +                       banks |= BIT(1);
> +                       break;
> +               case PIN_CONFIG_POWER_SOURCE:
> +                       /* Sanity check the power source */
> +                       ret = resolve_power_source(pctrl, arg);
> +                       if (ret < 0)
> +                               return ret;
> +                       pin->power_source = arg;
> +                       banks |= BIT(0);
> +                       break;
> +               case PIN_CONFIG_DRIVE_STRENGTH:
> +                       if (arg > PM8XXX_GPIO_STRENGTH_LOW) {
> +                               dev_err(pctrl->dev, "invalid drive strength\n");
> +                               return -EINVAL;
> +                       }
> +                       pin->output_strength = arg;
> +                       banks |= BIT(3);
> +                       break;

SI units...

> +               case PIN_CONFIG_DRIVE_PUSH_PULL:
> +                       pin->output_buffer = PM8XXX_GPIO_PUSH_PULL;
> +                       banks |= BIT(1);
> +                       break;
> +               case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +                       pin->output_buffer = PM8XXX_GPIO_OPEN_DRAIN;
> +                       banks |= BIT(1);
> +                       break;
> +               default:
> +                       dev_err(pctrl->dev,
> +                                       "unsupported config parameter: %x\n",
> +                                       param);
> +                       return -EINVAL;
> +               }
> +       }

(...)

> +static struct pinctrl_desc pm8xxx_gpio_desc = {
> +       .pctlops = &pm8xxx_gpio_pinctrl_ops,
> +       .pmxops = &pm8xxx_pinmux_ops,
> +       .confops = &pm8xxx_gpio_pinconf_ops,
> +       .owner = THIS_MODULE,
> +};

So this (that should be named pm8xxx_pinctrl_desc) does not
contain .pins, .npins, or even .name (!). Why not?

.pins could be identical to the actually software-configurable
pins on the circuit, or a superset describing all pins on it
if you like. The .pins variable in the state container seems
to be sort of duplicating this.

Note:

struct pinctrl_pin_desc {
        unsigned number;
        const char *name;
        void *drv_data;
};

See .drv_data above? Instead of using your custom struct
for this, use pinctrl_pin_desc and dereference .drv_data.

Naming the pins is usually very helpful, and usually the debugfs
will look very nice, and the implementation of .pin_dbg_show() in
pinctrl_ops can print something meaningful.

> +static int pm8xxx_gpio_direction_input(struct gpio_chip *chip,
> +                                      unsigned offset)
> +{

These functions are OK to name with *gpio* infix as they relate to
the gpiochip.

> +static int pm8xxx_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
> +       struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
> +
> +       if (pin->direction == PM8XXX_GPIO_DIR_OUT)
> +               return pin->output_value;
> +       else
> +               return pm8xxx_read_irq_status(pin->irq);

Yeah that thing... brrrr...

> +static int pm8xxx_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
> +       struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
> +
> +       return pin->irq;
> +}

That's I guess proper if some totally different irqchip is managing the
IRQs and there is a line for each GPIO to its corresponding IRQ
on that irqchip.

> +static int pm8xxx_gpio_populate(struct pm8xxx_gpio *pctrl)
> +{
> +       const struct pm8xxx_gpio_data *data = pctrl->data;
> +       struct pm8xxx_gpio_pin *pin;
> +       int val;
> +       int i;
> +
> +       for (i = 0; i < pctrl->data->ngpio; i++) {
> +               pin = &pctrl->pins[i];
> +
> +               val = pm8xxx_gpio_read(pctrl, i, 0);
> +               if (val < 0)
> +                       return val;
> +
> +               pin->power_source = data->power_sources[(val >> 1) & 0x7];
> +
> +               val = pm8xxx_gpio_read(pctrl, i, 1);
> +               if (val < 0)
> +                       return val;
> +
> +               pin->direction = (val >> 2) & 0x3;
> +               pin->output_buffer = !!(val & BIT(1));
> +               pin->output_value = val & BIT(0);
> +
> +               val = pm8xxx_gpio_read(pctrl, i, 2);
> +               if (val < 0)
> +                       return val;
> +
> +               pin->bias = (val >> 1) & 0x7;
> +
> +               val = pm8xxx_gpio_read(pctrl, i, 3);
> +               if (val < 0)
> +                       return val;
> +
> +               pin->output_strength = (val >> 2) & 0x3;
> +               pin->disable = val & BIT(0);
> +
> +               val = pm8xxx_gpio_read(pctrl, i, 4);
> +               if (val < 0)
> +                       return val;
> +
> +               pin->function = (val >> 1) & 0x7;
> +
> +               val = pm8xxx_gpio_read(pctrl, i, 5);
> +               if (val < 0)
> +                       return val;
> +
> +               pin->non_inverted = !!(val & BIT(3));

See, it's bool, no u8. It seems the HW has the logic reversed here,
ho hum. Maybe it's right to name it "non_inverted" then.

That's all for now...

Yours,
Linus Walleij

[PATCH 3/3] pinctrl: Introduce pinctrl driver for Qualcomm pm8xxx

[Bjorn Andersson]

On Wed, Jul 9, 2014 at 2:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
[...]
>> +config PINCTRL_PM8XXX_GPIO
>> +       tristate "Qualcomm PM8xxx gpio driver"
>
> I would prefer PINCTRL_PM8XXX simply. GPIO is just one aspect of what
> this block inside PM8XXX is doing, it is a bit confusing.
>

There are two different blocks within these pmics, a block named "gpio" and a
block named "mpp" (multi purpose pins). Both are reasonable to model using
pinctrl + gpiolib, but I split them because I felt that the pinconf and pinmux
did not match up nicely.

Unless we merge them into one I feel that it makes sense to keep this naming.

>> +++ b/drivers/pinctrl/pinctrl-pm8xxx-gpio.c
>
> Likewise name the file and all functions just pinctrl-pm8xxx or so, reserve
> the "gpio" string in function names for functions that really just deal with
> the gpiochip.
>

The function naming is annoying and as it's all local functions anyways I think
I'll do so even if we have two "copies" of this.

>> +struct pm8xxx_gpio_pin {
>> +       int irq;
>> +
>> +       u8 power_source;
>> +       u8 direction;
>> +       u8 output_buffer;
>> +       u8 output_value;
>> +       u8 bias;
>> +       u8 output_strength;
>> +       u8 disable;
>> +       u8 function;
>> +       u8 non_inverted;
>> +};
>
> This struct really needs kerneldoc, as I guess it is what will be reflected
> in dbg_view messages etc too.
>

Agreed.

> Some u8:s seem dubious. Like why isn't "disable" a bool?
>

Agreed, sorry for being lazy here.

> Why is there a property "non_inverted", like being inverted was the
> normal state? Isn't it a bool inverted rather?
>

The magical line in the 3.4 codeaurora tree is:

  (param->inv_int_pol ? 0 : PM_GPIO_NON_INT_POL_INV);

It might be more logical to just name it "inverted" and do the flipping when
reading/writing the value though.

> "direction" also seems a bit bool ... even "output_value".
>

"direction" is 2 bits that can be both be set, unfortunately it's not entirely
clear to me what's expected when setting both; so I've copied the logic as good
as possible from the codeaurora driver.

"output_value" is bool.

> Such things.
>
>> +struct pm8xxx_gpio_data {
>> +       int ngpio;
>
> Can this be negative or should it be unsigned?
> Plus the usage in the code seems to be npins rather than
> ngpio. It indicates the number of pins this driver can handle,
> that they also do GPIO is just one aspect of their full pin potential...
>

unsigned npins it is.

>> +       const int *power_sources;
>> +       int npower_sources;
>
> Dito.
>
>> +};
>> +
>> +struct pm8xxx_gpio {
>> +       struct device *dev;
>> +       struct regmap *regmap;
>> +       struct pinctrl_dev *pctrl;
>> +       struct gpio_chip chip;
>> +
>> +       const struct pm8xxx_gpio_data *data;
>> +
>> +       struct pm8xxx_gpio_pin pins[PM8XXX_MAX_GPIOS];
>
> No thanks. Use the .pins in struct pinctrl_desc.
> (Details below.)
>

Will give it a spin and see how it turns out.

>> +};
>> +
>> +static const char * const pm8xxx_gpio_groups[PM8XXX_MAX_GPIOS] = {
>> +       "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", "gpio7", "gpio8",
>> +       "gpio9", "gpio10", "gpio11", "gpio12", "gpio13", "gpio14", "gpio15",
>> +       "gpio16", "gpio17", "gpio18", "gpio19", "gpio20", "gpio21", "gpio22",
>> +       "gpio23", "gpio24", "gpio25", "gpio26", "gpio27", "gpio28", "gpio29",
>> +       "gpio30", "gpio31", "gpio32", "gpio33", "gpio34", "gpio35", "gpio36",
>> +       "gpio37", "gpio38", "gpio39", "gpio40", "gpio41", "gpio42", "gpio43",
>> +       "gpio44",
>> +};
>> +
>> +static const char * const pm8xxx_gpio_functions[] = {
>> +       "normal", "paired",
>> +       "func1", "func2",
>> +       "dtest1", "dtest2", "dtest3", "dtest4",
>> +};
>
> What is a bit strange is that the driver does not contain an array of the
> actual pin names, just groups and functions? It is usually very helpful
> to name all the individual pins.
>

Stephen is looking into whether we can find these lists for each of the
supported pmics.

>> +static int pm8xxx_gpio_read(struct pm8xxx_gpio *pctrl, int pin, int bank)
>
> I would prefer if this was named pm8xxx_pinctrl_read() or similar, so we
> get the GPIO ambiguity out of the way.
>
>> +{
>> +       int reg = SSBI_REG_ADDR_GPIO(pin);
>
> But I guess the registers may be named "GPIO" (something something)
> in the datasheet because the HW and spec authors are in the gpiomode
> pitfall we describe in the pinctrl.txt document. Like they think muxing
> and electrical config is "something GPIO" you know. So if this is matching
> a datasheet then keep that designation.
>

Unfortunately that's exactly what we face...

[...]
>> +static int pm8xxx_gpio_config_get(struct pinctrl_dev *pctldev,
>> +                         unsigned int offset,
>> +                         unsigned long *config)
>> +{
>
> Kudos for using generic pinconf, that makes everything much easier.
>

Thanks, I hope we can figure out the SI units and keep it this way.

[...]
>> +static struct pinctrl_desc pm8xxx_gpio_desc = {
>> +       .pctlops = &pm8xxx_gpio_pinctrl_ops,
>> +       .pmxops = &pm8xxx_pinmux_ops,
>> +       .confops = &pm8xxx_gpio_pinconf_ops,
>> +       .owner = THIS_MODULE,
>> +};
>
> So this (that should be named pm8xxx_pinctrl_desc) does not
> contain .pins, .npins, or even .name (!). Why not?
>

Because I turned them into pingroups instead, as there was "no need" for pins.
We're facing the same thing here as in pinctrl-msm, that there is a list of
pins and then there's an identical list of pingroups. And if you look closely
at pinctrl-msm, the pins are really only there for shows because we do all
operations on the pingroups.

> .pins could be identical to the actually software-configurable
> pins on the circuit, or a superset describing all pins on it
> if you like. The .pins variable in the state container seems
> to be sort of duplicating this.
>
> Note:
>
> struct pinctrl_pin_desc {
>         unsigned number;
>         const char *name;
>         void *drv_data;
> };
>
> See .drv_data above? Instead of using your custom struct
> for this, use pinctrl_pin_desc and dereference .drv_data.
>

I would still need the same custom struct, just that the reference would be
indirect via the pin_desc. But as I add a list of pins I will give it a spin.

> Naming the pins is usually very helpful, and usually the debugfs
> will look very nice, and the implementation of .pin_dbg_show() in
> pinctrl_ops can print something meaningful.
>

Maybe the fact that my gpio_chip->dbg_show() is doing this makes me not see
that point, I'll give it a spin and see if I can keep the gpio output related
to the gpio parts instead(?).

Regarding naming pins, we used to have actual ball names in the schematics but
as you always needed some other translation table it stopped and these days you
would only find something like PM8921_GPIO1 there. This is why the names end up
being "gpio1" or similar.

>> +static int pm8xxx_gpio_direction_input(struct gpio_chip *chip,
>> +                                      unsigned offset)
>> +{
>
> These functions are OK to name with *gpio* infix as they relate to
> the gpiochip.
>
>> +static int pm8xxx_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
>> +       struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
>> +
>> +       if (pin->direction == PM8XXX_GPIO_DIR_OUT)
>> +               return pin->output_value;
>> +       else
>> +               return pm8xxx_read_irq_status(pin->irq);
>
> Yeah that thing... brrrr...
>
>> +static int pm8xxx_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>> +{
>> +       struct pm8xxx_gpio *pctrl = container_of(chip, struct pm8xxx_gpio, chip);
>> +       struct pm8xxx_gpio_pin *pin = &pctrl->pins[offset - 1];
>> +
>> +       return pin->irq;
>> +}
>
> That's I guess proper if some totally different irqchip is managing the
> IRQs and there is a line for each GPIO to its corresponding IRQ
> on that irqchip.
>

I'm open to suggestions regarding this, but I found that this seem to solve my
problem at least ;)

Thanks for the review, I'll respin with some pins once we sorted out how to do
the bindings.

Regards,
Bjorn

[PATCH 3/3] pinctrl: Introduce pinctrl driver for Qualcomm pm8xxx

[Linus Walleij]

On Tue, Jul 15, 2014 at 12:48 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> On Wed, Jul 9, 2014 at 2:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Jul 8, 2014 at 3:26 AM, Bjorn Andersson
> [...]

>> "direction" also seems a bit bool ... even "output_value".
>>
>
> "direction" is 2 bits that can be both be set, unfortunately it's not entirely
> clear to me what's expected when setting both; so I've copied the logic as good
> as possible from the codeaurora driver.

Oh, can you do this?

u8 foo:2;

?

Yours,
Linus Walleij