[PATCH 0/4] Additional iProc GPIO support

[PATCH 0/4] Additional iProc GPIO support

[Ray Jui]

Add support to the iProc GPIO driver to allow PINCONF functions to be
disabled individually. Also add a new compatible string "brcm,iproc-gpio-only"
that allows this driver to be used as a pure GPIO driver without any PINCONF
functions enabled

Patch series is developed based on Linux v4.6-rc1 and available at:
https://github.com/Broadcom/cygnus-linux/tree/iproc-gpio-v1

Ray Jui (4):
  dt-bindings: Update iProc GPIO bindings
  pinctrl: iproc: Allow certain PINCONF functions to be disabled
  dt-bindings: Update iProc GPIO bindings
  pinctrl: iproc: Allow PINCONF to be disabled completely

 .../bindings/pinctrl/brcm,iproc-gpio.txt           |  12 ++-
 drivers/pinctrl/bcm/pinctrl-iproc-gpio.c           | 106 +++++++++++++++++++--
 include/dt-bindings/pinctrl/brcm,iproc-gpio.h      |  52 ++++++++++
 3 files changed, 162 insertions(+), 8 deletions(-)
 create mode 100644 include/dt-bindings/pinctrl/brcm,iproc-gpio.h

-- 
2.1.4

[PATCH 1/4] dt-bindings: Update iProc GPIO bindings

[Ray Jui]

Update the iProc GPIO binding document to add a new optional property
'brcm,pinconf-func-off' that allows certain PINCONF parameters to be
disabled in certain iProc SoCs where these PINCONF functions are not
supported

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Jon Mason <jon.mason@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
index e427792..ddaa1b0 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
@@ -37,6 +37,11 @@ Optional properties:
     3  Pin-control base pin offset.
     4. number of gpio pins which are linearly mapped from pin base.
 
+- brcm,pinconf-func-off:
+    Certain iProc SoCs might have some of the PINCONF functions disabled in
+the chip when the iProc GPIO controller is integrated. This optional property
+allows unsupported PINCONF functions to be disabled
+
 Supported generic PINCONF properties in child nodes:
 
 - pins:
-- 
2.1.4

[PATCH 2/4] pinctrl: iproc: Allow certain PINCONF functions to be disabled

[Ray Jui]

The iProc GPIO controller is shared among multiple iProc based SoCs. In
some of these SoCs, certain PINCONF functions are disabled and registers
associated with these functions are reserved. This patch adds support to
the iProc GPIO/PINCOF driver to allow these unsupported functions to be
disabled through device tree property 'brcm,pinconf-func-off'. Without
disabling these unsupported PINCONF functions, user can potentially
access these functions through sysfs debug entries; as a result, these
reserved registers can be accessed

This patch is developed based on the initial work from Yendapally Reddy
Dhananjaya <yrdreddy@broadcom.com> who attempted to disable drive
strength configuration for the iProc based NSP chip. In addition,
Pramod Kumar <pramodku@broadcom.com> also contributed to make the
support more generic across all currently supported PINCONF functions in
the iProc GPIO/PINCONF driver

Signed-off-by: Pramod Kumar <pramodku@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Jon Mason <jon.mason@broadcom.com>
Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/pinctrl/bcm/pinctrl-iproc-gpio.c      | 91 ++++++++++++++++++++++++++-
 include/dt-bindings/pinctrl/brcm,iproc-gpio.h | 52 +++++++++++++++
 2 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/pinctrl/brcm,iproc-gpio.h

diff --git a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
index d530ab4..12a8922 100644
--- a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
@@ -34,6 +34,7 @@
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinconf-generic.h>
 
+#include <dt-bindings/pinctrl/brcm,iproc-gpio.h>
 #include "../pinctrl-utils.h"
 
 #define IPROC_GPIO_DATA_IN_OFFSET   0x00
@@ -78,6 +79,10 @@
  * @num_banks: number of GPIO banks, each bank supports up to 32 GPIOs
  * @pinmux_is_supported: flag to indicate this GPIO controller contains pins
  * that can be individually muxed to GPIO
+ * @pinconf_disable: contains a list of PINCONF parameters that need to be
+ * disabled
+ * @nr_pinconf_disable: total number of PINCONF parameters that need to be
+ * disabled
  * @pctl: pointer to pinctrl_dev
  * @pctldesc: pinctrl descriptor
  */
@@ -94,6 +99,9 @@ struct iproc_gpio {
 
 	bool pinmux_is_supported;
 
+	enum pin_config_param *pinconf_disable;
+	unsigned nr_pinconf_disable;
+
 	struct pinctrl_dev *pctl;
 	struct pinctrl_desc pctldesc;
 };
@@ -360,6 +368,65 @@ static int iproc_gpio_get(struct gpio_chip *gc, unsigned gpio)
 	return !!(readl(chip->base + offset) & BIT(shift));
 }
 
+/*
+ * Mapping of the iProc PINCONF parameters to the generic pin configuration
+ * parameters
+ */
+static const enum pin_config_param iproc_pinconf_disable_map[] = {
+	[IPROC_PIN_DRIVE_STRENGTH] = PIN_CONFIG_DRIVE_STRENGTH,
+	[IPROC_PIN_BIAS_DISABLE] = PIN_CONFIG_BIAS_DISABLE,
+	[IPROC_PIN_BIAS_PULL_UP] = PIN_CONFIG_BIAS_PULL_UP,
+	[IPROC_PIN_BIAS_PULL_DOWN] = PIN_CONFIG_BIAS_PULL_DOWN,
+};
+
+static bool iproc_pinconf_param_is_disabled(struct iproc_gpio *chip,
+					    enum pin_config_param param)
+{
+	unsigned i;
+
+	if (!chip->nr_pinconf_disable)
+		return false;
+
+	for (i = 0; i < chip->nr_pinconf_disable; i++)
+		if (chip->pinconf_disable[i] == param)
+			return true;
+
+	return false;
+}
+
+static int iproc_pinconf_disable_map_create(struct iproc_gpio *chip,
+					    unsigned long disable_mask)
+{
+	unsigned int map_size = ARRAY_SIZE(iproc_pinconf_disable_map);
+	unsigned int bit, nbits = 0;
+
+	/* figure out total number of PINCONF parameters to disable */
+	for_each_set_bit(bit, &disable_mask, map_size)
+		nbits++;
+
+	if (!nbits)
+		return 0;
+
+	/*
+	 * Allocate an array to store PINCONF parameters that need to be
+	 * disabled
+	 */
+	chip->pinconf_disable = devm_kcalloc(chip->dev, nbits,
+					     sizeof(*chip->pinconf_disable),
+					     GFP_KERNEL);
+	if (!chip->pinconf_disable)
+		return -ENOMEM;
+
+	chip->nr_pinconf_disable = nbits;
+
+	/* now store these parameters */
+	nbits = 0;
+	for_each_set_bit(bit, &disable_mask, map_size)
+		chip->pinconf_disable[nbits++] = iproc_pinconf_disable_map[bit];
+
+	return 0;
+}
+
 static int iproc_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	return 1;
@@ -500,6 +567,9 @@ static int iproc_pin_config_get(struct pinctrl_dev *pctldev, unsigned pin,
 	bool disable, pull_up;
 	int ret;
 
+	if (iproc_pinconf_param_is_disabled(chip, param))
+		return -ENOTSUPP;
+
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
 		iproc_gpio_get_pull(chip, gpio, &disable, &pull_up);
@@ -548,6 +618,10 @@ static int iproc_pin_config_set(struct pinctrl_dev *pctldev, unsigned pin,
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
+
+		if (iproc_pinconf_param_is_disabled(chip, param))
+			return -ENOTSUPP;
+
 		arg = pinconf_to_config_argument(configs[i]);
 
 		switch (param) {
@@ -651,7 +725,7 @@ static int iproc_gpio_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct iproc_gpio *chip;
 	struct gpio_chip *gc;
-	u32 ngpios;
+	u32 ngpios, pinconf_disable_mask;
 	int irq, ret;
 
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
@@ -713,6 +787,21 @@ static int iproc_gpio_probe(struct platform_device *pdev)
 		goto err_rm_gpiochip;
 	}
 
+	/*
+	 * Optional DT property to disable unsupported pinconf parameters for
+	 * a particular iProc SoC
+	 */
+	ret = of_property_read_u32(dev->of_node, "brcm,pinconf-func-off",
+				   &pinconf_disable_mask);
+	if (!ret) {
+		ret = iproc_pinconf_disable_map_create(chip,
+						       pinconf_disable_mask);
+		if (ret) {
+			dev_err(dev, "unable to create pinconf disable map\n");
+			goto err_unregister_pinconf;
+		}
+	}
+
 	/* optional GPIO interrupt support */
 	irq = platform_get_irq(pdev, 0);
 	if (irq) {
diff --git a/include/dt-bindings/pinctrl/brcm,iproc-gpio.h b/include/dt-bindings/pinctrl/brcm,iproc-gpio.h
new file mode 100644
index 0000000..82b449e
--- /dev/null
+++ b/include/dt-bindings/pinctrl/brcm,iproc-gpio.h
@@ -0,0 +1,52 @@
+/*
+ *  BSD LICENSE
+ *
+ *  Copyright (C) 2016 Broadcom.  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions
+ *  are met:
+ *
+ *    * Redistributions of source code must retain the above copyright
+ *      notice, this list of conditions and the following disclaimer.
+ *    * Redistributions in binary form must reproduce the above copyright
+ *      notice, this list of conditions and the following disclaimer in
+ *      the documentation and/or other materials provided with the
+ *      distribution.
+ *    * Neither the name of Broadcom Corporation nor the names of its
+ *      contributors may be used to endorse or promote products derived
+ *      from this software without specific prior written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *  A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *  OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *  SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *  LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *  DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *  THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef __DT_BINDINGS_PINCTRL_BRCM_IPROC_GPIO_H__
+#define __DT_BINDINGS_PINCTRL_BRCM_IPROC_GPIO_H__
+
+/*
+ * Bit position for PINCONF functions to be disabled for a given iProc SoC
+ */
+#define IPROC_PIN_DRIVE_STRENGTH        0
+#define IPROC_PIN_BIAS_DISABLE          1
+#define IPROC_PIN_BIAS_PULL_UP          2
+#define IPROC_PIN_BIAS_PULL_DOWN        3
+
+/*
+ * Bit mask for DT property "brcm,pinconf-func-off"
+ */
+#define IPROC_PIN_OFF_DRIVE_STRENGTH    (1 << IPROC_PIN_DRIVE_STRENGTH)
+#define IPROC_PIN_OFF_BIAS_DISABLE      (1 << IPROC_PIN_BIAS_DISABLE)
+#define IPROC_PIN_OFF_BIAS_PULL_UP      (1 << IPROC_PIN_BIAS_PULL_UP)
+#define IPROC_PIN_OFF_BIAS_PULL_DOWN    (1 << IPROC_PIN_BIAS_PULL_DOWN)
+
+#endif
-- 
2.1.4

[PATCH 3/4] dt-bindings: Update iProc GPIO bindings

[Ray Jui]

Update the iProc GPIO binding document to introduce a new compatible
string "brcm,iproc-gpio-only", that allows the generic pinconf function
to be disabled completely

Signed-off-by: Ray Jui <ray.jui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Jon Mason <jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
index ddaa1b0..75a4370 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
@@ -3,8 +3,11 @@ Broadcom iProc GPIO/PINCONF Controller
 Required properties:
 
 - compatible:
-    Must be "brcm,cygnus-ccm-gpio", "brcm,cygnus-asiu-gpio",
-    "brcm,cygnus-crmu-gpio" or "brcm,iproc-gpio"
+    For Cygnus, it must be brcm,cygnus-ccm-gpio", "brcm,cygnus-asiu-gpio", or
+"brcm,cygnus-crmu-gpio"
+    For non-Cygnus iProc SoCs, it must be either "brcm,iproc-gpio-only" (if
+only GPIO is supported) or "brcm,iproc-gpio" (if both generic pinconf and GPIO
+are supported)
 
 - reg:
     Define the base and range of the I/O address space that contains SoC
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] dt-bindings: Update iProc GPIO bindings

[Ray Jui]

Update the iProc GPIO binding document to introduce a new compatible
string "brcm,iproc-gpio-only", that allows the generic pinconf function
to be disabled completely

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Jon Mason <jon.mason@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
index ddaa1b0..75a4370 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
@@ -3,8 +3,11 @@ Broadcom iProc GPIO/PINCONF Controller
 Required properties:
 
 - compatible:
-    Must be "brcm,cygnus-ccm-gpio", "brcm,cygnus-asiu-gpio",
-    "brcm,cygnus-crmu-gpio" or "brcm,iproc-gpio"
+    For Cygnus, it must be brcm,cygnus-ccm-gpio", "brcm,cygnus-asiu-gpio", or
+"brcm,cygnus-crmu-gpio"
+    For non-Cygnus iProc SoCs, it must be either "brcm,iproc-gpio-only" (if
+only GPIO is supported) or "brcm,iproc-gpio" (if both generic pinconf and GPIO
+are supported)
 
 - reg:
     Define the base and range of the I/O address space that contains SoC
-- 
2.1.4

[PATCH 4/4] pinctrl: iproc: Allow PINCONF to be disabled completely

[Ray Jui]

In some of the future iProc based SoCs, pinconf is handled by another
block and the iProc GPIO controller is solely used as a GPIO controller.
This patch adds support of a new compatible string "brcm,iproc-gpio-only",
that is introduced to handle this case, where pinconf functions in this
driver are completely disabled

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
Reviewed-by: Jon Mason <jon.mason@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/pinctrl/bcm/pinctrl-iproc-gpio.c | 41 ++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
index 12a8922..bb5cfd9 100644
--- a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
@@ -716,7 +716,8 @@ static const struct of_device_id iproc_gpio_of_match[] = {
 	{ .compatible = "brcm,cygnus-asiu-gpio" },
 	{ .compatible = "brcm,cygnus-crmu-gpio" },
 	{ .compatible = "brcm,iproc-gpio" },
-	{ }
+	{ .compatible = "brcm,iproc-gpio-only" },
+	{ /* sentinel */ }
 };
 
 static int iproc_gpio_probe(struct platform_device *pdev)
@@ -781,24 +782,28 @@ static int iproc_gpio_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = iproc_gpio_register_pinconf(chip);
-	if (ret) {
-		dev_err(dev, "unable to register pinconf\n");
-		goto err_rm_gpiochip;
-	}
-
-	/*
-	 * Optional DT property to disable unsupported pinconf parameters for
-	 * a particular iProc SoC
-	 */
-	ret = of_property_read_u32(dev->of_node, "brcm,pinconf-func-off",
-				   &pinconf_disable_mask);
-	if (!ret) {
-		ret = iproc_pinconf_disable_map_create(chip,
-						       pinconf_disable_mask);
+	if (!of_device_is_compatible(dev->of_node, "brcm,iproc-gpio-only")) {
+		ret = iproc_gpio_register_pinconf(chip);
 		if (ret) {
-			dev_err(dev, "unable to create pinconf disable map\n");
-			goto err_unregister_pinconf;
+			dev_err(dev, "unable to register pinconf\n");
+			goto err_rm_gpiochip;
+		}
+
+		/*
+		 * Optional DT property to disable unsupported pinconf
+		 * parameters for a particular iProc SoC
+		 */
+		ret = of_property_read_u32(dev->of_node,
+					   "brcm,pinconf-func-off",
+					   &pinconf_disable_mask);
+		if (!ret) {
+			ret = iproc_pinconf_disable_map_create(chip,
+							 pinconf_disable_mask);
+			if (ret) {
+				dev_err(dev,
+					"unable to create pinconf disable map\n");
+				goto err_unregister_pinconf;
+			}
 		}
 	}
 
-- 
2.1.4

Re: [PATCH 3/4] dt-bindings: Update iProc GPIO bindings

[Rob Herring]

On Tue, Apr 12, 2016 at 05:15:22PM -0700, Ray Jui wrote:
> Update the iProc GPIO binding document to introduce a new compatible
> string "brcm,iproc-gpio-only", that allows the generic pinconf function
> to be disabled completely
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Jon Mason <jon.mason@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
> index ddaa1b0..75a4370 100644
> --- a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
> @@ -3,8 +3,11 @@ Broadcom iProc GPIO/PINCONF Controller
>  Required properties:
>  
>  - compatible:
> -    Must be "brcm,cygnus-ccm-gpio", "brcm,cygnus-asiu-gpio",
> -    "brcm,cygnus-crmu-gpio" or "brcm,iproc-gpio"
> +    For Cygnus, it must be brcm,cygnus-ccm-gpio", "brcm,cygnus-asiu-gpio", or
> +"brcm,cygnus-crmu-gpio"
> +    For non-Cygnus iProc SoCs, it must be either "brcm,iproc-gpio-only" (if
> +only GPIO is supported) or "brcm,iproc-gpio" (if both generic pinconf and GPIO
> +are supported)

No. That's not how compatible strings work. Use SoC specific compatible 
strings if you need to distinguish this.

Rob

Re: [PATCH 2/4] pinctrl: iproc: Allow certain PINCONF functions to be disabled

[Linus Walleij]

On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@broadcom.com> wrote:

> The iProc GPIO controller is shared among multiple iProc based SoCs. In
> some of these SoCs, certain PINCONF functions are disabled and registers
> associated with these functions are reserved. This patch adds support to
> the iProc GPIO/PINCOF driver to allow these unsupported functions to be

^PINCONF

> disabled through device tree property 'brcm,pinconf-func-off'. Without
> disabling these unsupported PINCONF functions, user can potentially
> access these functions through sysfs debug entries; as a result, these
> reserved registers can be accessed
>
> This patch is developed based on the initial work from Yendapally Reddy
> Dhananjaya <yrdreddy@broadcom.com> who attempted to disable drive
> strength configuration for the iProc based NSP chip. In addition,
> Pramod Kumar <pramodku@broadcom.com> also contributed to make the
> support more generic across all currently supported PINCONF functions in
> the iProc GPIO/PINCONF driver
>
> Signed-off-by: Pramod Kumar <pramodku@broadcom.com>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Jon Mason <jon.mason@broadcom.com>
> Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>

OK I see. Hm I think it is better to use a per-soc compatible string
like Rob Herring indicates to determine if we should loose these
pin config parameters. Does it seem reasonable to you?

I'm thinking that it is a property of how the hardware was synthesized
in that SoC so since it is a different version of the hardware it should
have a unique compatible string.

> +/*
> + * Bit position for PINCONF functions to be disabled for a given iProc SoC
> + */
> +#define IPROC_PIN_DRIVE_STRENGTH        0
> +#define IPROC_PIN_BIAS_DISABLE          1
> +#define IPROC_PIN_BIAS_PULL_UP          2
> +#define IPROC_PIN_BIAS_PULL_DOWN        3
> +
> +/*
> + * Bit mask for DT property "brcm,pinconf-func-off"
> + */
> +#define IPROC_PIN_OFF_DRIVE_STRENGTH    (1 << IPROC_PIN_DRIVE_STRENGTH)
> +#define IPROC_PIN_OFF_BIAS_DISABLE      (1 << IPROC_PIN_BIAS_DISABLE)
> +#define IPROC_PIN_OFF_BIAS_PULL_UP      (1 << IPROC_PIN_BIAS_PULL_UP)
> +#define IPROC_PIN_OFF_BIAS_PULL_DOWN    (1 << IPROC_PIN_BIAS_PULL_DOWN)
> +
> +#endif

So this stuff should not be in the device tree, the driver should know,
from the compatible string, what config options that are unavailable.

Then if someone tries to use that in the DT, they should get an
error code -ENOTSUPP, but AFAICT that is what the patch achieves.

Yours,
Linus Walleij

Re: [PATCH 4/4] pinctrl: iproc: Allow PINCONF to be disabled completely

[Linus Walleij]

On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@broadcom.com> wrote:

> In some of the future iProc based SoCs, pinconf is handled by another
> block and the iProc GPIO controller is solely used as a GPIO controller.
> This patch adds support of a new compatible string "brcm,iproc-gpio-only",
> that is introduced to handle this case, where pinconf functions in this
> driver are completely disabled
>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
> Reviewed-by: Jon Mason <jon.mason@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>

If this was entirely true, then the driver should end up only executing
[devm_]gpiochip_add_data() but that does not seem to be the case.

You are still registering a pin controller, right? Just disabling some of
the pin config options. The pin multiplexing is still there, right?
Then it is not "solely a GPIO controller". Not at all.

This patch set needs some elaboration I think.

Yours,
Linus Walleij

Re: [PATCH 3/4] dt-bindings: Update iProc GPIO bindings

[Ray Jui]

On 4/14/2016 7:38 AM, Rob Herring wrote:
> On Tue, Apr 12, 2016 at 05:15:22PM -0700, Ray Jui wrote:
>> Update the iProc GPIO binding document to introduce a new compatible
>> string "brcm,iproc-gpio-only", that allows the generic pinconf function
>> to be disabled completely
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Jon Mason <jon.mason@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>>  Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
>> index ddaa1b0..75a4370 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,iproc-gpio.txt
>> @@ -3,8 +3,11 @@ Broadcom iProc GPIO/PINCONF Controller
>>  Required properties:
>>
>>  - compatible:
>> -    Must be "brcm,cygnus-ccm-gpio", "brcm,cygnus-asiu-gpio",
>> -    "brcm,cygnus-crmu-gpio" or "brcm,iproc-gpio"
>> +    For Cygnus, it must be brcm,cygnus-ccm-gpio", "brcm,cygnus-asiu-gpio", or
>> +"brcm,cygnus-crmu-gpio"
>> +    For non-Cygnus iProc SoCs, it must be either "brcm,iproc-gpio-only" (if
>> +only GPIO is supported) or "brcm,iproc-gpio" (if both generic pinconf and GPIO
>> +are supported)
>
> No. That's not how compatible strings work. Use SoC specific compatible
> strings if you need to distinguish this.
>
> Rob
>

Okay. Let me reply this to the email from Linus so we can consolidate 
all related discussions.

Thanks,

Ray

Re: [PATCH 2/4] pinctrl: iproc: Allow certain PINCONF functions to be disabled

[Ray Jui]

Hi Linus/Rob,

On 4/15/2016 1:20 AM, Linus Walleij wrote:
> On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@broadcom.com> wrote:
>
>> The iProc GPIO controller is shared among multiple iProc based SoCs. In
>> some of these SoCs, certain PINCONF functions are disabled and registers
>> associated with these functions are reserved. This patch adds support to
>> the iProc GPIO/PINCOF driver to allow these unsupported functions to be
>
> ^PINCONF
>

Will fix this. Thanks.

>> disabled through device tree property 'brcm,pinconf-func-off'. Without
>> disabling these unsupported PINCONF functions, user can potentially
>> access these functions through sysfs debug entries; as a result, these
>> reserved registers can be accessed
>>
>> This patch is developed based on the initial work from Yendapally Reddy
>> Dhananjaya <yrdreddy@broadcom.com> who attempted to disable drive
>> strength configuration for the iProc based NSP chip. In addition,
>> Pramod Kumar <pramodku@broadcom.com> also contributed to make the
>> support more generic across all currently supported PINCONF functions in
>> the iProc GPIO/PINCONF driver
>>
>> Signed-off-by: Pramod Kumar <pramodku@broadcom.com>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Jon Mason <jon.mason@broadcom.com>
>> Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>
> OK I see. Hm I think it is better to use a per-soc compatible string
> like Rob Herring indicates to determine if we should loose these
> pin config parameters. Does it seem reasonable to you?
>
> I'm thinking that it is a property of how the hardware was synthesized
> in that SoC so since it is a different version of the hardware it should
> have a unique compatible string.
>

We use the same iProc GPIO controller in various iProc SoCs. Like you 
said, the same iProc GPIO controller is then synthesized slightly 
differently in these SoCs:

In NSP, for example, the iProc GPIO controller supports multiple PINCONF 
functions except drive strength.

In an upcoming iProc chip, the iProc GPIO controller only supports GPIO 
related operations and all PINCONF related operations are supported by a 
different HW block.

The use of compatible strings "brcm,iproc-gpio-only" and 
"brcm,iproc-gpio" models the existing pinctrl-single driver:

Required properties:
- compatible : "pinctrl-single" or "pinconf-single".
"pinctrl-single" means that pinconf isn't supported.
"pinconf-single" means that generic pinconf is supported.

Note compatible string "brcm,iproc-gpio" was already accepted and merged 
upstream. I believe the current debate is that you do not think a 
compatible string of "brcm,iproc-gpio-only" should be introduced to deal 
with an iProc SoC whose GPIO controller is only used as GPIO controller 
without PINCONF functions, correct?

If so, may I suggest the following:

1. keep "brcm,iproc-gpio" for full featured iProc GPIO and PINCONF 
controller
2. Introduce "brcm,iproc-gpio-v2" for NSP, where GPIO and PINCONF 
functions are supported but drive strength specific PINCONF feature is 
disabled
3. Introduce "brcm,iproc-gpio-v3" for upcoming iProc SoCs that move the 
PINCONF support out of this GPIO controller to a completely separate HW 
block

It's my understanding that the naming of a compatible string is best 
tight to the name of the underlying HW block that it is dealing with, 
instead of the name of a SoC where the HW block belongs to. I'm 
uncertain whether this particular case falls into the same category, 
where the same GPIO controller is used but synthesized differently 
across different SoCs.

Please let me know if the above proposed naming is acceptable?

>> +/*
>> + * Bit position for PINCONF functions to be disabled for a given iProc SoC
>> + */
>> +#define IPROC_PIN_DRIVE_STRENGTH        0
>> +#define IPROC_PIN_BIAS_DISABLE          1
>> +#define IPROC_PIN_BIAS_PULL_UP          2
>> +#define IPROC_PIN_BIAS_PULL_DOWN        3
>> +
>> +/*
>> + * Bit mask for DT property "brcm,pinconf-func-off"
>> + */
>> +#define IPROC_PIN_OFF_DRIVE_STRENGTH    (1 << IPROC_PIN_DRIVE_STRENGTH)
>> +#define IPROC_PIN_OFF_BIAS_DISABLE      (1 << IPROC_PIN_BIAS_DISABLE)
>> +#define IPROC_PIN_OFF_BIAS_PULL_UP      (1 << IPROC_PIN_BIAS_PULL_UP)
>> +#define IPROC_PIN_OFF_BIAS_PULL_DOWN    (1 << IPROC_PIN_BIAS_PULL_DOWN)
>> +
>> +#endif
>
> So this stuff should not be in the device tree, the driver should know,
> from the compatible string, what config options that are unavailable.

Okay.

>
> Then if someone tries to use that in the DT, they should get an
> error code -ENOTSUPP, but AFAICT that is what the patch achieves.
>
> Yours,
> Linus Walleij
>

Thanks,

Ray

Re: [PATCH 4/4] pinctrl: iproc: Allow PINCONF to be disabled completely

[Ray Jui]

Hi Linus,

On 4/15/2016 1:24 AM, Linus Walleij wrote:
> On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@broadcom.com> wrote:
>
>> In some of the future iProc based SoCs, pinconf is handled by another
>> block and the iProc GPIO controller is solely used as a GPIO controller.
>> This patch adds support of a new compatible string "brcm,iproc-gpio-only",
>> that is introduced to handle this case, where pinconf functions in this
>> driver are completely disabled
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
>> Reviewed-by: Jon Mason <jon.mason@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>
> If this was entirely true, then the driver should end up only executing
> [devm_]gpiochip_add_data() but that does not seem to be the case.

Yes, in the case of compatible string "brcm,iproc-gpio-only" is 
detected, the driver only registers 'gpiochip_add_data'. Please check 
patch 2/4 of this series, which takes care of it.

>
> You are still registering a pin controller, right? Just disabling some of
> the pin config options. The pin multiplexing is still there, right?
> Then it is not "solely a GPIO controller". Not at all.

This driver does not register itself as a PINCONF driver if 
"brcm,iproc-gpio-only" compatible string is detected. This is addressed 
in patch 2/4 of this series.

Pin based IOMUX GPIO override is only activated when 
'chip->pinmux_is_supported' is true, and it is only true if the optional 
DT property "gpio-ranges" is defined.

>
> This patch set needs some elaboration I think.
>
> Yours,
> Linus Walleij
>

I believe the current issue with this patch series is now only on the 
naming of the new compatible string "brcm,iproc-gpio-only". Please 
correct me if I'm wrong.

Thanks,

Ray

Re: [PATCH 4/4] pinctrl: iproc: Allow PINCONF to be disabled completely

[Linus Walleij]

On Mon, Apr 18, 2016 at 9:30 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> On 4/15/2016 1:24 AM, Linus Walleij wrote:
>>
>> On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@broadcom.com> wrote:
>>
>>> In some of the future iProc based SoCs, pinconf is handled by another
>>> block and the iProc GPIO controller is solely used as a GPIO controller.
>>> This patch adds support of a new compatible string
>>> "brcm,iproc-gpio-only",
>>> that is introduced to handle this case, where pinconf functions in this
>>> driver are completely disabled
>>>
>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>> Reviewed-by: Yendapally Reddy Dhananjaya Reddy
>>> <yendapally.reddy@broadcom.com>
>>> Reviewed-by: Jon Mason <jon.mason@broadcom.com>
>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>
>>
>> If this was entirely true, then the driver should end up only executing
>> [devm_]gpiochip_add_data() but that does not seem to be the case.
>
> Yes, in the case of compatible string "brcm,iproc-gpio-only" is detected,
> the driver only registers 'gpiochip_add_data'. Please check patch 2/4 of
> this series, which takes care of it.

OK.

>> You are still registering a pin controller, right? Just disabling some of
>> the pin config options. The pin multiplexing is still there, right?
>> Then it is not "solely a GPIO controller". Not at all.
>
> This driver does not register itself as a PINCONF driver if
> "brcm,iproc-gpio-only" compatible string is detected. This is addressed in
> patch 2/4 of this series.
>
> Pin based IOMUX GPIO override is only activated when
> 'chip->pinmux_is_supported' is true, and it is only true if the optional DT
> property "gpio-ranges" is defined.

OK.

> I believe the current issue with this patch series is now only on the naming
> of the new compatible string "brcm,iproc-gpio-only". Please correct me if
> I'm wrong.

Yeah I think I get it now. The patch set makes sense.
Looking forward to the next iteration!

Yours,
Linus Walleij