[PATCHv2 0/4] Generic #pinctrl-cells and and pinctrl_parse_index_with_args

[PATCHv2 0/4] Generic #pinctrl-cells and and pinctrl_parse_index_with_args

[Tony Lindgren]

Hi all,

Here a repost of some pinctrl changes to introduce #pinctrl-cells and
a generic parser pinctrl_parse_index_with_args that the drivers can
optionally use.

Regards,

Tony

Changes from v1:

- Update based on comments from Linus Walleij to not pass
  #pinctrl-cells as an argument and improve the binding
  documentation

Tony Lindgren (4):
  pinctrl: Introduce generic #pinctrl-cells and
    pinctrl_parse_index_with_args
  pinctrl: single: Use generic parser and #pinctrl-cells for
    pinctrl-single,pins
  pinctrl: single: Use generic parser and #pinctrl-cells for
    pinctrl-single,bits
  ARM: dts: Add #pinctrl-cells for pinctrl-single instances

 .../bindings/pinctrl/pinctrl-bindings.txt          |  44 +++++-
 .../devicetree/bindings/pinctrl/pinctrl-single.txt |   3 +
 arch/arm/boot/dts/am33xx.dtsi                      |   2 +
 arch/arm/boot/dts/am3517.dtsi                      |   1 +
 arch/arm/boot/dts/am4372.dtsi                      |   1 +
 arch/arm/boot/dts/da850.dtsi                       |   1 +
 arch/arm/boot/dts/dm814x.dtsi                      |   1 +
 arch/arm/boot/dts/dm816x.dtsi                      |   2 +
 arch/arm/boot/dts/dra7.dtsi                        |   1 +
 arch/arm/boot/dts/hi3620.dtsi                      |   2 +
 arch/arm/boot/dts/keystone-k2g.dtsi                |   1 +
 arch/arm/boot/dts/keystone-k2l.dtsi                |   1 +
 arch/arm/boot/dts/omap2420.dtsi                    |   2 +
 arch/arm/boot/dts/omap2430.dtsi                    |   2 +
 arch/arm/boot/dts/omap3.dtsi                       |   2 +
 arch/arm/boot/dts/omap34xx.dtsi                    |   1 +
 arch/arm/boot/dts/omap36xx.dtsi                    |   1 +
 arch/arm/boot/dts/omap4.dtsi                       |   2 +
 arch/arm/boot/dts/omap5.dtsi                       |   2 +
 arch/arm/boot/dts/pxa3xx.dtsi                      |   1 +
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          |   3 +
 drivers/pinctrl/devicetree.c                       | 144 +++++++++++++++++++
 drivers/pinctrl/devicetree.h                       |  21 +++
 drivers/pinctrl/pinctrl-single.c                   | 159 +++++++++++++++------
 24 files changed, 357 insertions(+), 43 deletions(-)

--
2.10.2

[PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Tony Lindgren]

Introduce #pinctrl-cells helper binding and generic helper functions
pinctrl_count_index_with_args() and pinctrl_parse_index_with_args().

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../bindings/pinctrl/pinctrl-bindings.txt          |  44 ++++++-
 drivers/pinctrl/devicetree.c                       | 144 +++++++++++++++++++++
 drivers/pinctrl/devicetree.h                       |  21 +++
 3 files changed, 208 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -97,6 +97,11 @@ For example:
 	};
 
 == Pin controller devices ==
+Required properties: See the pin controller driver specific documentation
+
+Optional properties:
+#pinctrl-cells:	Number of pin control cells in addition to the index within the
+		pin controller device instance
 
 Pin controller devices should contain the pin configuration nodes that client
 devices reference.
@@ -119,7 +124,8 @@ For example:
 
 The contents of each of those pin configuration child nodes is defined
 entirely by the binding for the individual pin controller device. There
-exists no common standard for this content.
+exists no common standard for this content. The pinctrl framework only
+provides generic helper bindings that the pin controller driver can use.
 
 The pin configuration nodes need not be direct children of the pin controller
 device; they may be grandchildren, for example. Whether this is legal, and
@@ -156,6 +162,42 @@ state_2_node_a {
 	pins = "mfio29", "mfio30";
 };
 
+Optionally an altenative binding can be used if more suitable depending on the
+pin controller hardware. For hardaware where there is a large number of identical
+pin controller instances, naming each pin and function can easily become
+unmaintainable. This is especially the case if the same controller is used for
+different pins and functions depending on the SoC revision and packaging.
+
+For cases like this, the pin controller driver may use pinctrl-pin-array helper
+binding with a hardware based index and a number of pin configuration values:
+
+pincontroller {
+	... /* Standard DT properties for the device itself elided */
+	#pinctrl-cells = <2>;
+
+	state_0_node_a {
+		pinctrl-pin-array = <
+			0 A_DELAY_PS(0) G_DELAY_PS(120)
+			4 A_DELAY_PS(0) G_DELAY_PS(360)
+			...
+		>;
+	};
+	...
+};
+
+Above #pinctrl-cells specifies the number of value cells in addition to the
+index of the registers. This is similar to the interrupts-extended binding with
+one exception. There is no need to specify the phandle for each entry as that
+is already known as the defined pins are always children of the pin controller
+node. Further having the phandle pointing to another pin controller would not
+currently work as the pinctrl framework uses named modes to group pins for each
+pin control device.
+
+The index for pinctrl-pin-array must relate to the hardware for the pinctrl
+registers, and must not be a virtual index of pin instances. The reason for
+this is to avoid mapping of the index in the dts files and the pin controller
+driver as it can change.
+
 == Generic pin configuration node content ==
 
 Many data items that are represented in a pin configuration node are common
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -253,3 +253,147 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 	pinctrl_dt_free_maps(p);
 	return ret;
 }
+
+/*
+ * For pinctrl binding, typically #pinctrl-cells is for the pin controller
+ * device, so either parent or grandparent. See pinctrl-bindings.txt.
+ */
+static int pinctrl_find_cells_size(const struct device_node *np)
+{
+	const char *cells_name = "#pinctrl-cells";
+	int cells_size, error;
+
+	error = of_property_read_u32(np->parent, cells_name, &cells_size);
+	if (error) {
+		error = of_property_read_u32(np->parent->parent,
+					     cells_name, &cells_size);
+		if (error)
+			return -ENOENT;
+	}
+
+	return cells_size;
+}
+
+/**
+ * pinctrl_get_list_and_count - Gets the list and it's cell size and number
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ * @list: pointer for the list found
+ * @cells_size: pointer for the cell size found
+ * @nr_elements: pointer for the number of elements found
+ *
+ * Typically np is a single pinctrl entry containing the list.
+ */
+static int pinctrl_get_list_and_count(const struct device_node *np,
+				      const char *list_name,
+				      const __be32 **list,
+				      int *cells_size,
+				      int *nr_elements)
+{
+	int size;
+
+	*cells_size = 0;
+	*nr_elements = 0;
+
+	*list = of_get_property(np, list_name, &size);
+	if (!*list)
+		return -ENOENT;
+
+	*cells_size = pinctrl_find_cells_size(np);
+	if (*cells_size < 0)
+		return -ENOENT;
+
+	/* First element is always the index within the pinctrl device */
+	*nr_elements = (size / sizeof(**list)) / (*cells_size + 1);
+
+	return 0;
+}
+
+/**
+ * pinctrl_count_index_with_args - Count number of elements in a pinctrl entry
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ *
+ * Counts the number of elements in a pinctrl array consisting of an index
+ * within the controller and a number of u32 entries specified for each
+ * entry. Note that device_node is always for the parent pin controller device.
+ */
+int pinctrl_count_index_with_args(const struct device_node *np,
+				  const char *list_name)
+{
+	const __be32 *list;
+	int size, nr_cells, error;
+
+	error = pinctrl_get_list_and_count(np, list_name, &list,
+					   &nr_cells, &size);
+	if (error)
+		return error;
+
+	return size;
+}
+EXPORT_SYMBOL_GPL(pinctrl_count_index_with_args);
+
+/**
+ * pinctrl_copy_args - Populates of_phandle_args based on index
+ * @np: pointer to device node with the property
+ * @list: pointer to a list with the elements
+ * @index: entry within the list of elements
+ * @nr_cells: number of cells in the list
+ * @nr_elem: number of elements for each entry in the list
+ * @out_args: returned values
+ *
+ * Populates the of_phandle_args based on the index in the list.
+ */
+static int pinctrl_copy_args(const struct device_node *np,
+			     const __be32 *list,
+			     int index, int nr_cells, int nr_elem,
+			     struct of_phandle_args *out_args)
+{
+	int i;
+
+	memset(out_args, 0, sizeof(*out_args));
+	out_args->np = (struct device_node *)np;
+	out_args->args_count = nr_cells + 1;
+
+	if (index >= nr_elem)
+		return -EINVAL;
+
+	list += index * (nr_cells + 1);
+
+	for (i = 0; i < nr_cells + 1; i++)
+		out_args->args[i] = be32_to_cpup(list++);
+
+	return 0;
+}
+
+/**
+ * pinctrl_parse_index_with_args - Find a node pointed by index in a list
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ * @index: index within the list
+ * @out_arts: entries in the list pointed by index
+ *
+ * Finds the selected element in a pinctrl array consisting of an index
+ * within the controller and a number of u32 entries specified for each
+ * entry. Note that device_node is always for the parent pin controller device.
+ */
+int pinctrl_parse_index_with_args(const struct device_node *np,
+				  const char *list_name, int index,
+				  struct of_phandle_args *out_args)
+{
+	const __be32 *list;
+	int nr_elem, nr_cells, error;
+
+	error = pinctrl_get_list_and_count(np, list_name, &list,
+					   &nr_cells, &nr_elem);
+	if (error || !nr_cells)
+		return error;
+
+	error = pinctrl_copy_args(np, list, index, nr_cells, nr_elem,
+				  out_args);
+	if (error)
+		return error;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_parse_index_with_args);
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -21,6 +21,13 @@
 void pinctrl_dt_free_maps(struct pinctrl *p);
 int pinctrl_dt_to_map(struct pinctrl *p);
 
+int pinctrl_count_index_with_args(const struct device_node *np,
+				  const char *list_name);
+
+int pinctrl_parse_index_with_args(const struct device_node *np,
+				  const char *list_name, int index,
+				  struct of_phandle_args *out_args);
+
 #else
 
 static inline int pinctrl_dt_to_map(struct pinctrl *p)
@@ -32,4 +39,18 @@ static inline void pinctrl_dt_free_maps(struct pinctrl *p)
 {
 }
 
+static inline int pinctrl_count_index_with_args(const struct device_node *np,
+						const char *list_name)
+{
+	return -ENODEV;
+}
+
+static inline int
+pinctrl_parse_index_with_args(const struct device_node *np,
+			      const char *list_name, int index,
+			      struct of_phandle_args *out_args)
+{
+	return -ENODEV;
+}
+
 #endif
-- 
2.10.2

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

Hi Tony,

[auto build test WARNING on pinctrl/for-next]
[also build test WARNING on v4.9-rc3 next-20161028]
[cannot apply to robh/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Tony-Lindgren/Generic-pinctrl-cells-and-and-pinctrl_parse_index_with_args/20161104-004449
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git for-next
config: mips-generic_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All warnings (new ones prefixed by >>):

   In file included from drivers/pinctrl/core.c:36:0:
>> drivers/pinctrl/devicetree.h:29:14: warning: 'struct of_phandle_args' declared inside parameter list will not be visible outside of this definition or declaration
          struct of_phandle_args *out_args);
                 ^~~~~~~~~~~~~~~

vim +29 drivers/pinctrl/devicetree.h

    13	 * more details.
    14	 *
    15	 * You should have received a copy of the GNU General Public License
    16	 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
    17	 */
    18	
    19	#ifdef CONFIG_OF
    20	
    21	void pinctrl_dt_free_maps(struct pinctrl *p);
    22	int pinctrl_dt_to_map(struct pinctrl *p);
    23	
    24	int pinctrl_count_index_with_args(const struct device_node *np,
    25					  const char *list_name);
    26	
    27	int pinctrl_parse_index_with_args(const struct device_node *np,
    28					  const char *list_name, int index,
  > 29					  struct of_phandle_args *out_args);
    30	
    31	#else
    32	
    33	static inline int pinctrl_dt_to_map(struct pinctrl *p)
    34	{
    35		return 0;
    36	}
    37	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10604 bytes --]

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Tony Lindgren]

* kbuild test robot <lkp@intel.com> [161103 13:29]:
>    In file included from drivers/pinctrl/core.c:36:0:
> >> drivers/pinctrl/devicetree.h:29:14: warning: 'struct of_phandle_args' declared inside parameter list will not be visible outside of this definition or declaratio

Hmm maybe we should just include of.h in core.c?

Regards,

Tony

8< ----------------------
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -23,6 +23,7 @@
 #include <linux/list.h>
 #include <linux/sysfs.h>
 #include <linux/debugfs.h>
+#include <linux/of.h>
 #include <linux/seq_file.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/pinctrl/pinctrl.h>

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

Hi Tony,

[auto build test WARNING on pinctrl/for-next]
[also build test WARNING on v4.9-rc3 next-20161028]
[cannot apply to robh/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tony-Lindgren/Generic-pinctrl-cells-and-and-pinctrl_parse_index_with_args/20161104-004449
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git for-next
config: x86_64-randconfig-i0-201644 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from drivers/pinctrl/core.c:36:0:
>> drivers/pinctrl/devicetree.h:29:14: warning: 'struct of_phandle_args' declared inside parameter list
          struct of_phandle_args *out_args);
                 ^
>> drivers/pinctrl/devicetree.h:29:14: warning: its scope is only this definition or declaration, which is probably not what you want

vim +29 drivers/pinctrl/devicetree.h

    13	 * more details.
    14	 *
    15	 * You should have received a copy of the GNU General Public License
    16	 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
    17	 */
    18	
    19	#ifdef CONFIG_OF
    20	
    21	void pinctrl_dt_free_maps(struct pinctrl *p);
    22	int pinctrl_dt_to_map(struct pinctrl *p);
    23	
    24	int pinctrl_count_index_with_args(const struct device_node *np,
    25					  const char *list_name);
    26	
    27	int pinctrl_parse_index_with_args(const struct device_node *np,
    28					  const char *list_name, int index,
  > 29					  struct of_phandle_args *out_args);
    30	
    31	#else
    32	
    33	static inline int pinctrl_dt_to_map(struct pinctrl *p)
    34	{
    35		return 0;
    36	}
    37	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24875 bytes --]

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Linus Walleij]

On Thu, Nov 3, 2016 at 5:35 PM, Tony Lindgren <tony@atomide.com> wrote:

> Introduce #pinctrl-cells helper binding and generic helper functions
> pinctrl_count_index_with_args() and pinctrl_parse_index_with_args().
>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Ooops applied this v2 version instead of the v1.

* kbuild test robot <lkp@intel.com> [161103 13:29]:
>    In file included from drivers/pinctrl/core.c:36:0:
> >> drivers/pinctrl/devicetree.h:29:14: warning: 'struct of_phandle_args' declared inside parameter list will not be visible outside of this
definition or declaratio

> Hmm maybe we should just include of.h in core.c?

Nah. I just did this:

diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
index 7f0a5c4e15ad..c2d1a5505850 100644
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -16,6 +16,8 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */

+struct of_phandle_args;
+
 #ifdef CONFIG_OF

Let's see if it works!

Yours,
Linus Walleij

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [161104 14:50]:
> On Thu, Nov 3, 2016 at 5:35 PM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > Introduce #pinctrl-cells helper binding and generic helper functions
> > pinctrl_count_index_with_args() and pinctrl_parse_index_with_args().
> >
> > Acked-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Ooops applied this v2 version instead of the v1.

No problem, only the pinctrl-single patches needed
reposting.

> * kbuild test robot <lkp@intel.com> [161103 13:29]:
> >    In file included from drivers/pinctrl/core.c:36:0:
> > >> drivers/pinctrl/devicetree.h:29:14: warning: 'struct of_phandle_args' declared inside parameter list will not be visible outside of this
> definition or declaratio
> 
> > Hmm maybe we should just include of.h in core.c?
> 
> Nah. I just did this:
> 
> diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
> index 7f0a5c4e15ad..c2d1a5505850 100644
> --- a/drivers/pinctrl/devicetree.h
> +++ b/drivers/pinctrl/devicetree.h
> @@ -16,6 +16,8 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
> 
> +struct of_phandle_args;
> +
>  #ifdef CONFIG_OF
> 
> Let's see if it works!

OK so do we know now? It seems there was one more email
about it but it may have been without it.

Thanks,

Tony

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Linus Walleij]

On Mon, Nov 7, 2016 at 4:26 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Linus Walleij <linus.walleij@linaro.org> [161104 14:50]:

>> +struct of_phandle_args;
>> +
>>  #ifdef CONFIG_OF
>>
>> Let's see if it works!
>
> OK so do we know now? It seems there was one more email
> about it but it may have been without it.

I haven't seen anything, I think it JustWorks(TM).

Yours,
Linus Walleij

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [161108 03:32]:
> On Mon, Nov 7, 2016 at 4:26 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Linus Walleij <linus.walleij@linaro.org> [161104 14:50]:
> 
> >> +struct of_phandle_args;
> >> +
> >>  #ifdef CONFIG_OF
> >>
> >> Let's see if it works!
> >
> > OK so do we know now? It seems there was one more email
> > about it but it may have been without it.
> 
> I haven't seen anything, I think it JustWorks(TM).

OK good to to hear :)

Thanks,

Tony

[PATCH 2/4] pinctrl: single: Use generic parser and #pinctrl-cells for pinctrl-single,pins

[Tony Lindgren]

We can now use generic parser. To support the legacy binding without
#pinctrl-cells, add pcs_quirk_missing_pinctrl_cells() and warn about
missing #pinctrl-cells.

Let's also update the documentation for struct pcs_soc_data while at it
as that seems to be out of date.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/pinctrl-single.c | 111 ++++++++++++++++++++++++++++++++-------
 1 file changed, 93 insertions(+), 18 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -31,10 +31,10 @@
 #include <linux/platform_data/pinctrl-single.h>
 
 #include "core.h"
+#include "devicetree.h"
 #include "pinconf.h"
 
 #define DRIVER_NAME			"pinctrl-single"
-#define PCS_MUX_PINS_NAME		"pinctrl-single,pins"
 #define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
 #define PCS_OFF_DISABLED		~0U
 
@@ -162,8 +162,11 @@ struct pcs_soc_data {
  * @base:	virtual address of the controller
  * @size:	size of the ioremapped area
  * @dev:	device entry
+ * @np:		device tree node
  * @pctl:	pin controller device
  * @flags:	mask of PCS_FEAT_xxx values
+ * @missing_nr_pinctrl_cells: for legacy binding, may go away
+ * @socdata:	soc specific data
  * @lock:	spinlock for register access
  * @mutex:	mutex protecting the lists
  * @width:	bits per mux register
@@ -171,7 +174,8 @@ struct pcs_soc_data {
  * @fshift:	function register shift
  * @foff:	value to turn mux off
  * @fmax:	max number of functions in fmask
- * @bits_per_pin:number of bits per pin
+ * @bits_per_mux: number of bits per mux
+ * @bits_per_pin: number of bits per pin
  * @pins:	physical pins on the SoC
  * @pgtree:	pingroup index radix tree
  * @ftree:	function index radix tree
@@ -192,11 +196,13 @@ struct pcs_device {
 	void __iomem *base;
 	unsigned size;
 	struct device *dev;
+	struct device_node *np;
 	struct pinctrl_dev *pctl;
 	unsigned flags;
 #define PCS_QUIRK_SHARED_IRQ	(1 << 2)
 #define PCS_FEAT_IRQ		(1 << 1)
 #define PCS_FEAT_PINCONF	(1 << 0)
+	struct property *missing_nr_pinctrl_cells;
 	struct pcs_soc_data socdata;
 	raw_spinlock_t lock;
 	struct mutex mutex;
@@ -1125,20 +1131,14 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 						unsigned *num_maps,
 						const char **pgnames)
 {
+	const char *name = "pinctrl-single,pins";
 	struct pcs_func_vals *vals;
-	const __be32 *mux;
-	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
+	int rows, *pins, found = 0, res = -ENOMEM, i;
 	struct pcs_function *function;
 
-	mux = of_get_property(np, PCS_MUX_PINS_NAME, &size);
-	if ((!mux) || (size < sizeof(*mux) * 2)) {
-		dev_err(pcs->dev, "bad data for mux %s\n",
-			np->name);
-		return -EINVAL;
-	}
-
-	size /= sizeof(*mux);	/* Number of elements in array */
-	rows = size / 2;
+	rows = pinctrl_count_index_with_args(np, name);
+	if (rows == -EINVAL)
+		return rows;
 
 	vals = devm_kzalloc(pcs->dev, sizeof(*vals) * rows, GFP_KERNEL);
 	if (!vals)
@@ -1148,14 +1148,28 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	if (!pins)
 		goto free_vals;
 
-	while (index < size) {
-		unsigned offset, val;
+	for (i = 0; i < rows; i++) {
+		struct of_phandle_args pinctrl_spec;
+		unsigned int offset;
 		int pin;
 
-		offset = be32_to_cpup(mux + index++);
-		val = be32_to_cpup(mux + index++);
+		res = pinctrl_parse_index_with_args(np, name, i, &pinctrl_spec);
+		if (res)
+			return res;
+
+		if (pinctrl_spec.args_count < 2) {
+			dev_err(pcs->dev, "invalid args_count for spec: %i\n",
+				pinctrl_spec.args_count);
+			break;
+		}
+
+		/* Index plus one value cell */
+		offset = pinctrl_spec.args[0];
 		vals[found].reg = pcs->base + offset;
-		vals[found].val = val;
+		vals[found].val = pinctrl_spec.args[1];
+
+		dev_dbg(pcs->dev, "%s index: 0x%x value: 0x%x\n",
+			pinctrl_spec.np->name, offset, pinctrl_spec.args[1]);
 
 		pin = pcs_get_pin_by_offset(pcs, offset);
 		if (pin < 0) {
@@ -1473,6 +1487,10 @@ static void pcs_free_resources(struct pcs_device *pcs)
 	pinctrl_unregister(pcs->pctl);
 	pcs_free_funcs(pcs);
 	pcs_free_pingroups(pcs);
+#if IS_BUILTIN(CONFIG_PINCTRL_SINGLE)
+	if (pcs->missing_nr_pinctrl_cells)
+		of_remove_property(pcs->np, pcs->missing_nr_pinctrl_cells);
+#endif
 }
 
 static const struct of_device_id pcs_of_match[];
@@ -1790,6 +1808,55 @@ static int pinctrl_single_resume(struct platform_device *pdev)
 }
 #endif
 
+/**
+ * pcs_quirk_missing_pinctrl_cells - handle legacy binding
+ * @pcs: pinctrl driver instance
+ * @np: device tree node
+ * @cells: number of cells
+ *
+ * Handle legacy binding with no #pinctrl-cells. This should be
+ * always two pinctrl-single,bit-per-mux and one for others.
+ * At some point we may want to consider removing this.
+ */
+static int pcs_quirk_missing_pinctrl_cells(struct pcs_device *pcs,
+					   struct device_node *np,
+					   int cells)
+{
+	struct property *p;
+	const char *name = "#pinctrl-cells";
+	int error;
+	u32 val;
+
+	error = of_property_read_u32(np, name, &val);
+	if (!error)
+		return 0;
+
+	dev_warn(pcs->dev, "please update dts to use %s = <%i>\n",
+		 name, cells);
+
+	p = devm_kzalloc(pcs->dev, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	p->length = sizeof(__be32);
+	p->value = devm_kzalloc(pcs->dev, sizeof(__be32), GFP_KERNEL);
+	if (!p->value)
+		return -ENOMEM;
+	*(__be32 *)p->value = cpu_to_be32(cells);
+
+	p->name = devm_kstrdup(pcs->dev, name, GFP_KERNEL);
+	if (!p->name)
+		return -ENOMEM;
+
+	pcs->missing_nr_pinctrl_cells = p;
+
+#if IS_BUILTIN(CONFIG_PINCTRL_SINGLE)
+	error = of_add_property(np, pcs->missing_nr_pinctrl_cells);
+#endif
+
+	return error;
+}
+
 static int pcs_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -1810,6 +1877,7 @@ static int pcs_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 	pcs->dev = &pdev->dev;
+	pcs->np = np;
 	raw_spin_lock_init(&pcs->lock);
 	mutex_init(&pcs->mutex);
 	INIT_LIST_HEAD(&pcs->pingroups);
@@ -1846,6 +1914,13 @@ static int pcs_probe(struct platform_device *pdev)
 
 	pcs->bits_per_mux = of_property_read_bool(np,
 						  "pinctrl-single,bit-per-mux");
+	ret = pcs_quirk_missing_pinctrl_cells(pcs, np,
+					      pcs->bits_per_mux ? 2 : 1);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to patch #pinctrl-cells\n");
+
+		return ret;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
-- 
2.10.2

Re: [PATCH 2/4] pinctrl: single: Use generic parser and #pinctrl-cells for pinctrl-single,pins

[Linus Walleij]

On Thu, Nov 3, 2016 at 5:35 PM, Tony Lindgren <tony@atomide.com> wrote:

> We can now use generic parser. To support the legacy binding without
> #pinctrl-cells, add pcs_quirk_missing_pinctrl_cells() and warn about
> missing #pinctrl-cells.
>
> Let's also update the documentation for struct pcs_soc_data while at it
> as that seems to be out of date.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

This v2 patch applied.

Yours,
Linus Walleij

[PATCH 3/4] pinctrl: single: Use generic parser and #pinctrl-cells for pinctrl-single,bits

[Tony Lindgren]

We can now use generic parser and keep things compatible with the
old binding.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/pinctrl-single.c | 48 ++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -35,7 +35,6 @@
 #include "pinconf.h"
 
 #define DRIVER_NAME			"pinctrl-single"
-#define PCS_MUX_BITS_NAME		"pinctrl-single,bits"
 #define PCS_OFF_DISABLED		~0U
 
 /**
@@ -1219,36 +1218,22 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	return res;
 }
 
-#define PARAMS_FOR_BITS_PER_MUX 3
-
 static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs,
 						struct device_node *np,
 						struct pinctrl_map **map,
 						unsigned *num_maps,
 						const char **pgnames)
 {
+	const char *name = "pinctrl-single,pins";
 	struct pcs_func_vals *vals;
-	const __be32 *mux;
-	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
+	int rows, *pins, found = 0, res = -ENOMEM, i;
 	int npins_in_row;
 	struct pcs_function *function;
 
-	mux = of_get_property(np, PCS_MUX_BITS_NAME, &size);
-
-	if (!mux) {
-		dev_err(pcs->dev, "no valid property for %s\n", np->name);
-		return -EINVAL;
-	}
-
-	if (size < (sizeof(*mux) * PARAMS_FOR_BITS_PER_MUX)) {
-		dev_err(pcs->dev, "bad data for %s\n", np->name);
-		return -EINVAL;
-	}
-
-	/* Number of elements in array */
-	size /= sizeof(*mux);
+	rows = pinctrl_count_index_with_args(np, name);
+	if (rows == -EINVAL)
+		return rows;
 
-	rows = size / PARAMS_FOR_BITS_PER_MUX;
 	npins_in_row = pcs->width / pcs->bits_per_pin;
 
 	vals = devm_kzalloc(pcs->dev, sizeof(*vals) * rows * npins_in_row,
@@ -1261,15 +1246,30 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs,
 	if (!pins)
 		goto free_vals;
 
-	while (index < size) {
+	for (i = 0; i < rows; i++) {
+		struct of_phandle_args pinctrl_spec;
 		unsigned offset, val;
 		unsigned mask, bit_pos, val_pos, mask_pos, submask;
 		unsigned pin_num_from_lsb;
 		int pin;
 
-		offset = be32_to_cpup(mux + index++);
-		val = be32_to_cpup(mux + index++);
-		mask = be32_to_cpup(mux + index++);
+		res = pinctrl_parse_index_with_args(np, name, i, &pinctrl_spec);
+		if (res)
+			return res;
+
+		if (pinctrl_spec.args_count < 3) {
+			dev_err(pcs->dev, "invalid args_count for spec: %i\n",
+				pinctrl_spec.args_count);
+			break;
+		}
+
+		/* Index plus two value cells */
+		offset = pinctrl_spec.args[0];
+		val = pinctrl_spec.args[1];
+		mask = pinctrl_spec.args[2];
+
+		dev_dbg(pcs->dev, "%s index: 0x%x value: 0x%x mask: 0x%x\n",
+			pinctrl_spec.np->name, offset, val, mask);
 
 		/* Parse pins in each row from LSB */
 		while (mask) {
-- 
2.10.2

Re: [PATCH 3/4] pinctrl: single: Use generic parser and #pinctrl-cells for pinctrl-single,bits

[Linus Walleij]

On Thu, Nov 3, 2016 at 5:35 PM, Tony Lindgren <tony@atomide.com> wrote:

> We can now use generic parser and keep things compatible with the
> old binding.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

V2 Patch applied.

Yours,
Linus Walleij

[PATCH 4/4] ARM: dts: Add #pinctrl-cells for pinctrl-single instances

[Tony Lindgren]

Drivers using pinctrl-single,pins have #pinctrl-cells = <1>, while
pinctrl-single,bits need #pinctrl-cells = <2>.

Note that this patch can be optionally applied separately from the
driver changes as the driver supports also the legacy binding without
#pinctrl-cells.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt | 3 +++
 arch/arm/boot/dts/am33xx.dtsi                                | 2 ++
 arch/arm/boot/dts/am3517.dtsi                                | 1 +
 arch/arm/boot/dts/am4372.dtsi                                | 1 +
 arch/arm/boot/dts/da850.dtsi                                 | 1 +
 arch/arm/boot/dts/dm814x.dtsi                                | 1 +
 arch/arm/boot/dts/dm816x.dtsi                                | 2 ++
 arch/arm/boot/dts/dra7.dtsi                                  | 1 +
 arch/arm/boot/dts/hi3620.dtsi                                | 2 ++
 arch/arm/boot/dts/keystone-k2g.dtsi                          | 1 +
 arch/arm/boot/dts/keystone-k2l.dtsi                          | 1 +
 arch/arm/boot/dts/omap2420.dtsi                              | 2 ++
 arch/arm/boot/dts/omap2430.dtsi                              | 2 ++
 arch/arm/boot/dts/omap3.dtsi                                 | 2 ++
 arch/arm/boot/dts/omap34xx.dtsi                              | 1 +
 arch/arm/boot/dts/omap36xx.dtsi                              | 1 +
 arch/arm/boot/dts/omap4.dtsi                                 | 2 ++
 arch/arm/boot/dts/omap5.dtsi                                 | 2 ++
 arch/arm/boot/dts/pxa3xx.dtsi                                | 1 +
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi                    | 3 +++
 20 files changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
@@ -7,6 +7,9 @@ Required properties:
 
 - reg : offset and length of the register set for the mux registers
 
+- #pinctrl-cells : number of cells in addition to the index, set to 1
+  for pinctrl-single,pins and 2 for pinctrl-single,bits
+
 - pinctrl-single,register-width : pinmux register access width in bits
 
 - pinctrl-single,function-mask : mask of allowed pinmux function bits
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -130,6 +130,7 @@
 				reg = <0x210000 0x2000>;
 				#address-cells = <1>;
 				#size-cells = <1>;
+				#pinctrl-cells = <1>;
 				ranges = <0 0x210000 0x2000>;
 
 				am33xx_pinmux: pinmux@800 {
@@ -137,6 +138,7 @@
 					reg = <0x800 0x238>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					pinctrl-single,register-width = <32>;
 					pinctrl-single,function-mask = <0x7f>;
 				};
diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi
--- a/arch/arm/boot/dts/am3517.dtsi
+++ b/arch/arm/boot/dts/am3517.dtsi
@@ -66,6 +66,7 @@
 			reg = <0x480025d8 0x24>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			#pinctrl-cells = <1>;
 			#interrupt-cells = <1>;
 			interrupt-controller;
 			pinctrl-single,register-width = <16>;
diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -189,6 +189,7 @@
 					reg = <0x800 0x31c>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					#interrupt-cells = <1>;
 					interrupt-controller;
 					pinctrl-single,register-width = <32>;
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -36,6 +36,7 @@
 			reg = <0x14120 0x50>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			#pinctrl-cells = <2>;
 			pinctrl-single,bit-per-mux;
 			pinctrl-single,register-width = <32>;
 			pinctrl-single,function-mask = <0xf>;
diff --git a/arch/arm/boot/dts/dm814x.dtsi b/arch/arm/boot/dts/dm814x.dtsi
--- a/arch/arm/boot/dts/dm814x.dtsi
+++ b/arch/arm/boot/dts/dm814x.dtsi
@@ -374,6 +374,7 @@
 					reg = <0x800 0x438>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					pinctrl-single,register-width = <32>;
 					pinctrl-single,function-mask = <0x307ff>;
 				};
diff --git a/arch/arm/boot/dts/dm816x.dtsi b/arch/arm/boot/dts/dm816x.dtsi
--- a/arch/arm/boot/dts/dm816x.dtsi
+++ b/arch/arm/boot/dts/dm816x.dtsi
@@ -83,6 +83,7 @@
 			reg = <0x48140000 0x21000>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			#pinctrl-cells = <1>;
 			ranges = <0 0x48140000 0x21000>;
 
 			dm816x_pinmux: pinmux@800 {
@@ -90,6 +91,7 @@
 				reg = <0x800 0x50a>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				#pinctrl-cells = <1>;
 				pinctrl-single,register-width = <16>;
 				pinctrl-single,function-mask = <0xf>;
 			};
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -171,6 +171,7 @@
 					reg = <0x1400 0x0468>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					#interrupt-cells = <1>;
 					interrupt-controller;
 					pinctrl-single,register-width = <32>;
diff --git a/arch/arm/boot/dts/hi3620.dtsi b/arch/arm/boot/dts/hi3620.dtsi
--- a/arch/arm/boot/dts/hi3620.dtsi
+++ b/arch/arm/boot/dts/hi3620.dtsi
@@ -537,6 +537,7 @@
 			reg = <0x803000 0x188>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			#pinctrl-cells = <1>;
 			#gpio-range-cells = <3>;
 			ranges;
 
@@ -558,6 +559,7 @@
 			reg = <0x803800 0x2dc>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			#pinctrl-cells = <1>;
 			ranges;
 
 			pinctrl-single,register-width = <32>;
diff --git a/arch/arm/boot/dts/keystone-k2g.dtsi b/arch/arm/boot/dts/keystone-k2g.dtsi
--- a/arch/arm/boot/dts/keystone-k2g.dtsi
+++ b/arch/arm/boot/dts/keystone-k2g.dtsi
@@ -72,6 +72,7 @@
 	soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
+		#pinctrl-cells = <1>;
 		compatible = "ti,keystone","simple-bus";
 		ranges = <0x0 0x0 0x0 0xc0000000>;
 		dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;
diff --git a/arch/arm/boot/dts/keystone-k2l.dtsi b/arch/arm/boot/dts/keystone-k2l.dtsi
--- a/arch/arm/boot/dts/keystone-k2l.dtsi
+++ b/arch/arm/boot/dts/keystone-k2l.dtsi
@@ -59,6 +59,7 @@
 			reg = <0x02620690 0xc>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			#pinctrl-cells = <2>;
 			pinctrl-single,bit-per-mux;
 			pinctrl-single,register-width = <32>;
 			pinctrl-single,function-mask = <0x1>;
diff --git a/arch/arm/boot/dts/omap2420.dtsi b/arch/arm/boot/dts/omap2420.dtsi
--- a/arch/arm/boot/dts/omap2420.dtsi
+++ b/arch/arm/boot/dts/omap2420.dtsi
@@ -38,6 +38,7 @@
 				reg = <0x0 0x1000>;
 				#address-cells = <1>;
 				#size-cells = <1>;
+				#pinctrl-cells = <1>;
 				ranges = <0 0x0 0x1000>;
 
 				omap2420_pmx: pinmux@30 {
@@ -46,6 +47,7 @@
 					reg = <0x30 0x0113>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					pinctrl-single,register-width = <8>;
 					pinctrl-single,function-mask = <0x3f>;
 				};
diff --git a/arch/arm/boot/dts/omap2430.dtsi b/arch/arm/boot/dts/omap2430.dtsi
--- a/arch/arm/boot/dts/omap2430.dtsi
+++ b/arch/arm/boot/dts/omap2430.dtsi
@@ -38,6 +38,7 @@
 				reg = <0x2000 0x1000>;
 				#address-cells = <1>;
 				#size-cells = <1>;
+				#pinctrl-cells = <1>;
 				ranges = <0 0x2000 0x1000>;
 
 				omap2430_pmx: pinmux@30 {
@@ -46,6 +47,7 @@
 					reg = <0x30 0x0154>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					pinctrl-single,register-width = <8>;
 					pinctrl-single,function-mask = <0x3f>;
 				};
diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -106,6 +106,7 @@
 					reg = <0x30 0x238>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					#interrupt-cells = <1>;
 					interrupt-controller;
 					pinctrl-single,register-width = <16>;
@@ -145,6 +146,7 @@
 					reg = <0xa00 0x5c>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					#interrupt-cells = <1>;
 					interrupt-controller;
 					pinctrl-single,register-width = <16>;
diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
--- a/arch/arm/boot/dts/omap34xx.dtsi
+++ b/arch/arm/boot/dts/omap34xx.dtsi
@@ -34,6 +34,7 @@
 			reg = <0x480025d8 0x24>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			#pinctrl-cells = <1>;
 			#interrupt-cells = <1>;
 			interrupt-controller;
 			pinctrl-single,register-width = <16>;
diff --git a/arch/arm/boot/dts/omap36xx.dtsi b/arch/arm/boot/dts/omap36xx.dtsi
--- a/arch/arm/boot/dts/omap36xx.dtsi
+++ b/arch/arm/boot/dts/omap36xx.dtsi
@@ -66,6 +66,7 @@
 			reg = <0x480025a0 0x5c>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			#pinctrl-cells = <1>;
 			#interrupt-cells = <1>;
 			interrupt-controller;
 			pinctrl-single,register-width = <16>;
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -184,6 +184,7 @@
 					reg = <0x40 0x0196>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					#interrupt-cells = <1>;
 					interrupt-controller;
 					pinctrl-single,register-width = <16>;
@@ -256,6 +257,7 @@
 					reg = <0x1e040 0x0038>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					#interrupt-cells = <1>;
 					interrupt-controller;
 					pinctrl-single,register-width = <16>;
diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -171,6 +171,7 @@
 					reg = <0x40 0x01b6>;
 					#address-cells = <1>;
 					#size-cells = <0>;
+					#pinctrl-cells = <1>;
 					#interrupt-cells = <1>;
 					interrupt-controller;
 					pinctrl-single,register-width = <16>;
@@ -270,6 +271,7 @@
 				reg = <0xc840 0x003c>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				#pinctrl-cells = <1>;
 				#interrupt-cells = <1>;
 				interrupt-controller;
 				pinctrl-single,register-width = <16>;
diff --git a/arch/arm/boot/dts/pxa3xx.dtsi b/arch/arm/boot/dts/pxa3xx.dtsi
--- a/arch/arm/boot/dts/pxa3xx.dtsi
+++ b/arch/arm/boot/dts/pxa3xx.dtsi
@@ -138,6 +138,7 @@
 			reg = <0x40e10000 0xffff>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+			#pinctrl-cells = <1>;
 			pinctrl-single,register-width = <32>;
 			pinctrl-single,function-mask = <0x7>;
 		};
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -364,6 +364,7 @@
 			reg = <0x0 0xf7010000  0x0 0x27c>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			#pinctrl-cells = <1>;
 			#gpio-range-cells = <3>;
 			pinctrl-single,register-width = <32>;
 			pinctrl-single,function-mask = <7>;
@@ -402,6 +403,7 @@
 			reg = <0x0 0xf7010800 0x0 0x28c>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			#pinctrl-cells = <1>;
 			pinctrl-single,register-width = <32>;
 		};
 
@@ -410,6 +412,7 @@
 			reg = <0x0 0xf8001800 0x0 0x78>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+			#pinctrl-cells = <1>;
 			pinctrl-single,register-width = <32>;
 		};
 
-- 
2.10.2

Re: [PATCH 4/4] ARM: dts: Add #pinctrl-cells for pinctrl-single instances

[Linus Walleij]

On Thu, Nov 3, 2016 at 5:35 PM, Tony Lindgren <tony@atomide.com> wrote:

> Drivers using pinctrl-single,pins have #pinctrl-cells = <1>, while
> pinctrl-single,bits need #pinctrl-cells = <2>.
>
> Note that this patch can be optionally applied separately from the
> driver changes as the driver supports also the legacy binding without
> #pinctrl-cells.
>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Please take this through the OMAP tree to avoid hazzle.

Yours,
Linus Walleij

Re: [PATCH 4/4] ARM: dts: Add #pinctrl-cells for pinctrl-single instances

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [161104 14:55]:
> On Thu, Nov 3, 2016 at 5:35 PM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > Drivers using pinctrl-single,pins have #pinctrl-cells = <1>, while
> > pinctrl-single,bits need #pinctrl-cells = <2>.
> >
> > Note that this patch can be optionally applied separately from the
> > driver changes as the driver supports also the legacy binding without
> > #pinctrl-cells.
> >
> > Acked-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Please take this through the OMAP tree to avoid hazzle.

OK applied into omap-for-v4.10/pinctrl-cells.

Thanks,

Tony

[PATCH 0/4] Generic #pinctrl-cells and and pinctrl_parse_index_with_args

[Tony Lindgren]

Hi all,

Here are some pinctrl changes to introduce #pinctrl-cells and a generic
parser pinctrl_parse_index_with_args that the drivers can optionally use.

Regards,

Tony


Tony Lindgren (4):
  pinctrl: Introduce generic #pinctrl-cells and
    pinctrl_parse_index_with_args
  pinctrl: single: Use generic parser and #pinctrl-cells for
    pinctrl-single,pins
  pinctrl: single: Use generic parser and #pinctrl-cells for
    pinctrl-single,bits
  ARM: dts: Add #pinctrl-cells for pinctrl-single instances

 .../bindings/pinctrl/pinctrl-bindings.txt          |  42 +++++-
 .../devicetree/bindings/pinctrl/pinctrl-single.txt |   3 +
 arch/arm/boot/dts/am33xx.dtsi                      |   2 +
 arch/arm/boot/dts/am3517.dtsi                      |   1 +
 arch/arm/boot/dts/am4372.dtsi                      |   1 +
 arch/arm/boot/dts/da850.dtsi                       |   1 +
 arch/arm/boot/dts/dm814x.dtsi                      |   1 +
 arch/arm/boot/dts/dm816x.dtsi                      |   2 +
 arch/arm/boot/dts/dra7.dtsi                        |   1 +
 arch/arm/boot/dts/hi3620.dtsi                      |   2 +
 arch/arm/boot/dts/keystone-k2g.dtsi                |   1 +
 arch/arm/boot/dts/keystone-k2l.dtsi                |   1 +
 arch/arm/boot/dts/omap2420.dtsi                    |   2 +
 arch/arm/boot/dts/omap2430.dtsi                    |   2 +
 arch/arm/boot/dts/omap3.dtsi                       |   2 +
 arch/arm/boot/dts/omap34xx.dtsi                    |   1 +
 arch/arm/boot/dts/omap36xx.dtsi                    |   1 +
 arch/arm/boot/dts/omap4.dtsi                       |   2 +
 arch/arm/boot/dts/omap5.dtsi                       |   2 +
 arch/arm/boot/dts/pxa3xx.dtsi                      |   1 +
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          |   3 +
 drivers/pinctrl/devicetree.c                       | 151 ++++++++++++++++++++
 drivers/pinctrl/devicetree.h                       |  24 ++++
 drivers/pinctrl/pinctrl-single.c                   | 157 +++++++++++++++------
 24 files changed, 363 insertions(+), 43 deletions(-)

-- 
2.9.3

[PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Tony Lindgren]

Introduce #pinctrl-cells helper binding and generic helper functions
pinctrl_count_index_with_args() and pinctrl_parse_index_with_args().

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../bindings/pinctrl/pinctrl-bindings.txt          |  42 +++++-
 drivers/pinctrl/devicetree.c                       | 151 +++++++++++++++++++++
 drivers/pinctrl/devicetree.h                       |  24 ++++
 3 files changed, 216 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -97,6 +97,11 @@ For example:
 	};
 
 == Pin controller devices ==
+Required properties: See the pin controller driver specific documentation
+
+Optional properties:
+#pinctrl-cells:	Number of pin control cells in addition to the index within the
+		pin controller device instance
 
 Pin controller devices should contain the pin configuration nodes that client
 devices reference.
@@ -119,7 +124,8 @@ For example:
 
 The contents of each of those pin configuration child nodes is defined
 entirely by the binding for the individual pin controller device. There
-exists no common standard for this content.
+exists no common standard for this content. The pinctrl framework only
+provides generic helper bindings that the pin controller driver can use.
 
 The pin configuration nodes need not be direct children of the pin controller
 device; they may be grandchildren, for example. Whether this is legal, and
@@ -156,6 +162,40 @@ state_2_node_a {
 	pins = "mfio29", "mfio30";
 };
 
+For pin controller hardware with a large number of identical registers naming
+each bit both can be unmaintainable. Further there can be a large number of similar
+pinctrl hardware using the same registers for different purposes depending on the
+packaging. For cases like this, the pinctrl driver may use pinctrl-pin-array helper
+binding using a hardware based index and a number of configuration values:
+
+pincontroller {
+	... /* Standard DT properties for the device itself elided */
+	#pinctrl-cells = <2>;
+
+	state_0_node_a {
+		pinctrl-pin-array = <
+			0 A_DELAY_PS(0) G_DELAY_PS(120)
+			4 A_DELAY_PS(0) G_DELAY_PS(360)
+			...
+		>;
+	};
+	...
+};
+
+The #pinctrl-cells specifies the number of value cells in addition to the
+index of the registers. This is similar to the interrupts-extended binding with
+one exception. There is no need to specify the phandle for each entry as that
+is already known as the defined pins are always children of the pin controller
+node. Further having the phandle pointing to another pin controller would not
+currently work as the pinctrl framework uses named modes to group pins for each
+pin control device.
+
+The index for pinctrl-pin-array must relate to the hardware for the pinctrl
+registers, and must not be a virtual index of pin instances. The reason for
+this is to avoid mapping of the index in the dts files and the pin controller
+driver as it can change. And we want to avoid another case of interrupt
+numbering with pinctrl numbering.
+
 == Generic pin configuration node content ==
 
 Many data items that are represented in a pin configuration node are common
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -253,3 +253,154 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 	pinctrl_dt_free_maps(p);
 	return ret;
 }
+
+/*
+ * For pinctrl binding, typically #pinctrl-cells is for the pin controller
+ * device, so either parent or grandparent. See pinctrl-bindings.txt.
+ */
+static int pinctrl_find_cells_size(const struct device_node *np,
+				   const char *cells_name)
+{
+	int cells_size, error;
+
+	error = of_property_read_u32(np->parent, cells_name, &cells_size);
+	if (error) {
+		error = of_property_read_u32(np->parent->parent,
+					     cells_name, &cells_size);
+		if (error)
+			return -ENOENT;
+	}
+
+	return cells_size;
+}
+
+/**
+ * pinctrl_get_list_and_count - Gets the list and it's cell size and number
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ * @cells_name: property name that specifies phandle's argument count
+ * @list: pointer for the list found
+ * @cells_size: pointer for the cell size found
+ * @nr_elements: pointer for the number of elements found
+ *
+ * Typically np is a single pinctrl entry containing the list and
+ * cells_name is "#pinctrl-cells" for the parent pin controller.
+ */
+static int pinctrl_get_list_and_count(const struct device_node *np,
+				      const char *list_name,
+				      const char *cells_name,
+				      int nr_elem,
+				      const __be32 **list,
+				      int *cells_size,
+				      int *nr_elements)
+{
+	int size;
+
+	*cells_size = 0;
+	*nr_elements = 0;
+
+	*list = of_get_property(np, list_name, &size);
+	if (!*list)
+		return -ENOENT;
+
+	*cells_size = pinctrl_find_cells_size(np, cells_name);
+	if (*cells_size < 0)
+		return -ENOENT;
+
+	/* First element is always the index within the pinctrl device */
+	*nr_elements = (size / sizeof(**list)) / (*cells_size + 1);
+
+	return 0;
+}
+
+/**
+ * pinctrl_count_index_with_args - Count number of elements in a pinctrl entry
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ * @cells_name: property name that specifies the argument count
+ *
+ * Counts the number of elements in a pinctrl array consisting of an index
+ * within the controller and a number of u32 entries specified for each
+ * entry. Note that device_node is always for the parent pin controller device.
+ */
+int pinctrl_count_index_with_args(const struct device_node *np,
+				  const char *list_name,
+				  const char *cells_name)
+{
+	const __be32 *list;
+	int size, nr_cells, error;
+
+	error = pinctrl_get_list_and_count(np, list_name, cells_name, -1,
+					   &list, &nr_cells, &size);
+	if (error)
+		return error;
+
+	return size;
+}
+EXPORT_SYMBOL_GPL(pinctrl_count_index_with_args);
+
+/**
+ * pinctrl_copy_args - Populates of_phandle_args based on index
+ * @np: pointer to device node with the property
+ * @list: pointer to a list with the elements
+ * @index: entry within the list of elements
+ * @nr_cells: number of cells in the list
+ * @nr_elem: number of elements for each entry in the list
+ * @out_args: returned values
+ *
+ * Populates the of_phandle_args based on the index in the list.
+ */
+static int pinctrl_copy_args(const struct device_node *np,
+			     const __be32 *list,
+			     int index, int nr_cells, int nr_elem,
+			     struct of_phandle_args *out_args)
+{
+	int i;
+
+	memset(out_args, 0, sizeof(*out_args));
+	out_args->np = (struct device_node *)np;
+	out_args->args_count = nr_cells + 1;
+
+	if (index >= nr_elem)
+		return -EINVAL;
+
+	list += index * (nr_cells + 1);
+
+	for (i = 0; i < nr_cells + 1; i++)
+		out_args->args[i] = be32_to_cpup(list++);
+
+	return 0;
+}
+
+/**
+ * pinctrl_parse_index_with_args - Find a node pointed by index in a list
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ * @cells_name: property name that specifies phandle's argument count
+ * @index: index within the list
+ * @out_arts: entries in the list pointed by index
+ *
+ * Finds the selected element in a pinctrl array consisting of an index
+ * within the controller and a number of u32 entries specified for each
+ * entry. Note that device_node is always for the parent pin controller device.
+ */
+int pinctrl_parse_index_with_args(const struct device_node *np,
+				  const char *list_name, const char *cells_name,
+				  int index, struct of_phandle_args *out_args)
+{
+	const __be32 *list;
+	int nr_elem, nr_cells, error;
+
+	error = pinctrl_get_list_and_count(np, list_name, cells_name, -1,
+					   &list, &nr_cells, &nr_elem);
+	if (error || !nr_cells)
+		return error;
+
+	error = pinctrl_copy_args(np, list, index, nr_cells, nr_elem,
+				  out_args);
+	if (error)
+		return error;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_parse_index_with_args);
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -21,6 +21,14 @@
 void pinctrl_dt_free_maps(struct pinctrl *p);
 int pinctrl_dt_to_map(struct pinctrl *p);
 
+int pinctrl_count_index_with_args(const struct device_node *np,
+				  const char *list_name,
+				  const char *cells_name);
+
+int pinctrl_parse_index_with_args(const struct device_node *np,
+				  const char *list_name, const char *cells_name,
+				  int index, struct of_phandle_args *out_args);
+
 #else
 
 static inline int pinctrl_dt_to_map(struct pinctrl *p)
@@ -32,4 +40,20 @@ static inline void pinctrl_dt_free_maps(struct pinctrl *p)
 {
 }
 
+static inline int pinctrl_count_index_with_args(const struct device_node *np,
+						const char *list_name,
+						const char *cells_name)
+{
+	return -ENODEV;
+}
+
+static inline int
+pinctrl_parse_index_with_args(const struct device_node *np,
+			      const char *list_name,
+			      const char *cells_name, int index,
+			      struct of_phandle_args *out_args)
+{
+	return -ENODEV;
+}
+
 #endif
-- 
2.9.3

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Linus Walleij]

On Tue, Oct 25, 2016 at 6:45 PM, Tony Lindgren <tony@atomide.com> wrote:

I need some DT person to take a look at this binding and ACK it.

> +For pin controller hardware with a large number of identical registers naming
> +each bit both can be unmaintainable. Further there can be a large number of similar
> +pinctrl hardware using the same registers for different purposes depending on the
> +packaging. For cases like this, the pinctrl driver may use pinctrl-pin-array helper
> +binding using a hardware based index and a number of configuration values:

Maybe we can reword it a bit so that it is clear that this is an
either-or approach
for the pin controller, either they use the pins/groups/functions scheme
or they use this scheme.

> +pincontroller {
> +       ... /* Standard DT properties for the device itself elided */
> +       #pinctrl-cells = <2>;
> +
> +       state_0_node_a {
> +               pinctrl-pin-array = <
> +                       0 A_DELAY_PS(0) G_DELAY_PS(120)
> +                       4 A_DELAY_PS(0) G_DELAY_PS(360)
> +                       ...
> +               >;
> +       };
> +       ...
> +};

Looks all right to me. Sad to add to the binding mess, but on the other
hand, in the overall picture this nicely consolidates and structure
pinctrl-single.

> +The index for pinctrl-pin-array must relate to the hardware for the pinctrl
> +registers, and must not be a virtual index of pin instances. The reason for
> +this is to avoid mapping of the index in the dts files and the pin controller
> +driver as it can change.

OK

> And we want to avoid another case of interrupt
> +numbering with pinctrl numbering.

Maybe this file is not a good place for making technical arguments,
more describing what we agreed on, so cut that sentence IMO.

> +/*
> + * For pinctrl binding, typically #pinctrl-cells is for the pin controller
> + * device, so either parent or grandparent. See pinctrl-bindings.txt.
> + */
> +static int pinctrl_find_cells_size(const struct device_node *np,
> +                                  const char *cells_name)
> +{
> +       int cells_size, error;
> +
> +       error = of_property_read_u32(np->parent, cells_name, &cells_size);
> +       if (error) {
> +               error = of_property_read_u32(np->parent->parent,
> +                                            cells_name, &cells_size);
> +               if (error)
> +                       return -ENOENT;
> +       }
> +
> +       return cells_size;
> +}

Can't we just hardcode this to "#pinctrl-cells" and skip the cells_name
parameter? We can parametrize it the day we need it instead.

The rest of the helpers look nice and clean.

Yours,
Linus Walleij

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [161027 00:57]:
> On Tue, Oct 25, 2016 at 6:45 PM, Tony Lindgren <tony@atomide.com> wrote:
> 
> I need some DT person to take a look at this binding and ACK it.
> 
> > +For pin controller hardware with a large number of identical registers naming
> > +each bit both can be unmaintainable. Further there can be a large number of similar
> > +pinctrl hardware using the same registers for different purposes depending on the
> > +packaging. For cases like this, the pinctrl driver may use pinctrl-pin-array helper
> > +binding using a hardware based index and a number of configuration values:
> 
> Maybe we can reword it a bit so that it is clear that this is an
> either-or approach
> for the pin controller, either they use the pins/groups/functions scheme
> or they use this scheme.

Sure, this is just an optional helper.

> > +pincontroller {
> > +       ... /* Standard DT properties for the device itself elided */
> > +       #pinctrl-cells = <2>;
> > +
> > +       state_0_node_a {
> > +               pinctrl-pin-array = <
> > +                       0 A_DELAY_PS(0) G_DELAY_PS(120)
> > +                       4 A_DELAY_PS(0) G_DELAY_PS(360)
> > +                       ...
> > +               >;
> > +       };
> > +       ...
> > +};
> 
> Looks all right to me. Sad to add to the binding mess, but on the other
> hand, in the overall picture this nicely consolidates and structure
> pinctrl-single.
> 
> > +The index for pinctrl-pin-array must relate to the hardware for the pinctrl
> > +registers, and must not be a virtual index of pin instances. The reason for
> > +this is to avoid mapping of the index in the dts files and the pin controller
> > +driver as it can change.
> 
> OK
> 
> > And we want to avoid another case of interrupt
> > +numbering with pinctrl numbering.
> 
> Maybe this file is not a good place for making technical arguments,
> more describing what we agreed on, so cut that sentence IMO.

Sure :)

> > +/*
> > + * For pinctrl binding, typically #pinctrl-cells is for the pin controller
> > + * device, so either parent or grandparent. See pinctrl-bindings.txt.
> > + */
> > +static int pinctrl_find_cells_size(const struct device_node *np,
> > +                                  const char *cells_name)
> > +{
> > +       int cells_size, error;
> > +
> > +       error = of_property_read_u32(np->parent, cells_name, &cells_size);
> > +       if (error) {
> > +               error = of_property_read_u32(np->parent->parent,
> > +                                            cells_name, &cells_size);
> > +               if (error)
> > +                       return -ENOENT;
> > +       }
> > +
> > +       return cells_size;
> > +}
> 
> Can't we just hardcode this to "#pinctrl-cells" and skip the cells_name
> parameter? We can parametrize it the day we need it instead.

Sure we can do that.

> The rest of the helpers look nice and clean.

OK cool thanks,

Tony

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [161027 07:59]:
> * Linus Walleij <linus.walleij@linaro.org> [161027 00:57]:
> > On Tue, Oct 25, 2016 at 6:45 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > +/*
> > > + * For pinctrl binding, typically #pinctrl-cells is for the pin controller
> > > + * device, so either parent or grandparent. See pinctrl-bindings.txt.
> > > + */
> > > +static int pinctrl_find_cells_size(const struct device_node *np,
> > > +                                  const char *cells_name)
> > > +{
> > > +       int cells_size, error;
> > > +
> > > +       error = of_property_read_u32(np->parent, cells_name, &cells_size);
> > > +       if (error) {
> > > +               error = of_property_read_u32(np->parent->parent,
> > > +                                            cells_name, &cells_size);
> > > +               if (error)
> > > +                       return -ENOENT;
> > > +       }
> > > +
> > > +       return cells_size;
> > > +}
> > 
> > Can't we just hardcode this to "#pinctrl-cells" and skip the cells_name
> > parameter? We can parametrize it the day we need it instead.
> 
> Sure we can do that.
> 
> > The rest of the helpers look nice and clean.
> 
> OK cool thanks,

Below is an updated version of this patch with documentation updated
and cells_name removed. I'll repost the whole series once the DT
binding has been reviewed.

Regards,

Tony
8< -------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Tue, 25 Oct 2016 08:33:34 -0700
Subject: [PATCH] pinctrl: Introduce generic #pinctrl-cells and
 pinctrl_parse_index_with_args

Introduce #pinctrl-cells helper binding and generic helper functions
pinctrl_count_index_with_args() and pinctrl_parse_index_with_args().

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../bindings/pinctrl/pinctrl-bindings.txt          |  44 ++++++-
 drivers/pinctrl/devicetree.c                       | 144 +++++++++++++++++++++
 drivers/pinctrl/devicetree.h                       |  21 +++
 3 files changed, 208 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -97,6 +97,11 @@ For example:
 	};
 
 == Pin controller devices ==
+Required properties: See the pin controller driver specific documentation
+
+Optional properties:
+#pinctrl-cells:	Number of pin control cells in addition to the index within the
+		pin controller device instance
 
 Pin controller devices should contain the pin configuration nodes that client
 devices reference.
@@ -119,7 +124,8 @@ For example:
 
 The contents of each of those pin configuration child nodes is defined
 entirely by the binding for the individual pin controller device. There
-exists no common standard for this content.
+exists no common standard for this content. The pinctrl framework only
+provides generic helper bindings that the pin controller driver can use.
 
 The pin configuration nodes need not be direct children of the pin controller
 device; they may be grandchildren, for example. Whether this is legal, and
@@ -156,6 +162,42 @@ state_2_node_a {
 	pins = "mfio29", "mfio30";
 };
 
+Optionally an altenative binding can be used if more suitable depending on the
+pin controller hardware. For hardaware where there is a large number of identical
+pin controller instances, naming each pin and function can easily become
+unmaintainable. This is especially the case if the same controller is used for
+different pins and functions depending on the SoC revision and packaging.
+
+For cases like this, the pin controller driver may use pinctrl-pin-array helper
+binding with a hardware based index and a number of pin configuration values:
+
+pincontroller {
+	... /* Standard DT properties for the device itself elided */
+	#pinctrl-cells = <2>;
+
+	state_0_node_a {
+		pinctrl-pin-array = <
+			0 A_DELAY_PS(0) G_DELAY_PS(120)
+			4 A_DELAY_PS(0) G_DELAY_PS(360)
+			...
+		>;
+	};
+	...
+};
+
+Above #pinctrl-cells specifies the number of value cells in addition to the
+index of the registers. This is similar to the interrupts-extended binding with
+one exception. There is no need to specify the phandle for each entry as that
+is already known as the defined pins are always children of the pin controller
+node. Further having the phandle pointing to another pin controller would not
+currently work as the pinctrl framework uses named modes to group pins for each
+pin control device.
+
+The index for pinctrl-pin-array must relate to the hardware for the pinctrl
+registers, and must not be a virtual index of pin instances. The reason for
+this is to avoid mapping of the index in the dts files and the pin controller
+driver as it can change.
+
 == Generic pin configuration node content ==
 
 Many data items that are represented in a pin configuration node are common
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -253,3 +253,147 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 	pinctrl_dt_free_maps(p);
 	return ret;
 }
+
+/*
+ * For pinctrl binding, typically #pinctrl-cells is for the pin controller
+ * device, so either parent or grandparent. See pinctrl-bindings.txt.
+ */
+static int pinctrl_find_cells_size(const struct device_node *np)
+{
+	const char *cells_name = "#pinctrl-cells";
+	int cells_size, error;
+
+	error = of_property_read_u32(np->parent, cells_name, &cells_size);
+	if (error) {
+		error = of_property_read_u32(np->parent->parent,
+					     cells_name, &cells_size);
+		if (error)
+			return -ENOENT;
+	}
+
+	return cells_size;
+}
+
+/**
+ * pinctrl_get_list_and_count - Gets the list and it's cell size and number
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ * @list: pointer for the list found
+ * @cells_size: pointer for the cell size found
+ * @nr_elements: pointer for the number of elements found
+ *
+ * Typically np is a single pinctrl entry containing the list.
+ */
+static int pinctrl_get_list_and_count(const struct device_node *np,
+				      const char *list_name,
+				      const __be32 **list,
+				      int *cells_size,
+				      int *nr_elements)
+{
+	int size;
+
+	*cells_size = 0;
+	*nr_elements = 0;
+
+	*list = of_get_property(np, list_name, &size);
+	if (!*list)
+		return -ENOENT;
+
+	*cells_size = pinctrl_find_cells_size(np);
+	if (*cells_size < 0)
+		return -ENOENT;
+
+	/* First element is always the index within the pinctrl device */
+	*nr_elements = (size / sizeof(**list)) / (*cells_size + 1);
+
+	return 0;
+}
+
+/**
+ * pinctrl_count_index_with_args - Count number of elements in a pinctrl entry
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ *
+ * Counts the number of elements in a pinctrl array consisting of an index
+ * within the controller and a number of u32 entries specified for each
+ * entry. Note that device_node is always for the parent pin controller device.
+ */
+int pinctrl_count_index_with_args(const struct device_node *np,
+				  const char *list_name)
+{
+	const __be32 *list;
+	int size, nr_cells, error;
+
+	error = pinctrl_get_list_and_count(np, list_name, &list,
+					   &nr_cells, &size);
+	if (error)
+		return error;
+
+	return size;
+}
+EXPORT_SYMBOL_GPL(pinctrl_count_index_with_args);
+
+/**
+ * pinctrl_copy_args - Populates of_phandle_args based on index
+ * @np: pointer to device node with the property
+ * @list: pointer to a list with the elements
+ * @index: entry within the list of elements
+ * @nr_cells: number of cells in the list
+ * @nr_elem: number of elements for each entry in the list
+ * @out_args: returned values
+ *
+ * Populates the of_phandle_args based on the index in the list.
+ */
+static int pinctrl_copy_args(const struct device_node *np,
+			     const __be32 *list,
+			     int index, int nr_cells, int nr_elem,
+			     struct of_phandle_args *out_args)
+{
+	int i;
+
+	memset(out_args, 0, sizeof(*out_args));
+	out_args->np = (struct device_node *)np;
+	out_args->args_count = nr_cells + 1;
+
+	if (index >= nr_elem)
+		return -EINVAL;
+
+	list += index * (nr_cells + 1);
+
+	for (i = 0; i < nr_cells + 1; i++)
+		out_args->args[i] = be32_to_cpup(list++);
+
+	return 0;
+}
+
+/**
+ * pinctrl_parse_index_with_args - Find a node pointed by index in a list
+ * @np: pointer to device node with the property
+ * @list_name: property that contains the list
+ * @index: index within the list
+ * @out_arts: entries in the list pointed by index
+ *
+ * Finds the selected element in a pinctrl array consisting of an index
+ * within the controller and a number of u32 entries specified for each
+ * entry. Note that device_node is always for the parent pin controller device.
+ */
+int pinctrl_parse_index_with_args(const struct device_node *np,
+				  const char *list_name, int index,
+				  struct of_phandle_args *out_args)
+{
+	const __be32 *list;
+	int nr_elem, nr_cells, error;
+
+	error = pinctrl_get_list_and_count(np, list_name, &list,
+					   &nr_cells, &nr_elem);
+	if (error || !nr_cells)
+		return error;
+
+	error = pinctrl_copy_args(np, list, index, nr_cells, nr_elem,
+				  out_args);
+	if (error)
+		return error;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_parse_index_with_args);
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -21,6 +21,13 @@
 void pinctrl_dt_free_maps(struct pinctrl *p);
 int pinctrl_dt_to_map(struct pinctrl *p);
 
+int pinctrl_count_index_with_args(const struct device_node *np,
+				  const char *list_name);
+
+int pinctrl_parse_index_with_args(const struct device_node *np,
+				  const char *list_name, int index,
+				  struct of_phandle_args *out_args);
+
 #else
 
 static inline int pinctrl_dt_to_map(struct pinctrl *p)
@@ -32,4 +39,18 @@ static inline void pinctrl_dt_free_maps(struct pinctrl *p)
 {
 }
 
+static inline int pinctrl_count_index_with_args(const struct device_node *np,
+						const char *list_name)
+{
+	return -ENODEV;
+}
+
+static inline int
+pinctrl_parse_index_with_args(const struct device_node *np,
+			      const char *list_name, int index,
+			      struct of_phandle_args *out_args)
+{
+	return -ENODEV;
+}
+
 #endif
-- 
2.9.3

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Rob Herring]

On Fri, Oct 28, 2016 at 09:53:38AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [161027 07:59]:
> > * Linus Walleij <linus.walleij@linaro.org> [161027 00:57]:
> > > On Tue, Oct 25, 2016 at 6:45 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > > +/*
> > > > + * For pinctrl binding, typically #pinctrl-cells is for the pin controller
> > > > + * device, so either parent or grandparent. See pinctrl-bindings.txt.
> > > > + */
> > > > +static int pinctrl_find_cells_size(const struct device_node *np,
> > > > +                                  const char *cells_name)
> > > > +{
> > > > +       int cells_size, error;
> > > > +
> > > > +       error = of_property_read_u32(np->parent, cells_name, &cells_size);
> > > > +       if (error) {
> > > > +               error = of_property_read_u32(np->parent->parent,
> > > > +                                            cells_name, &cells_size);
> > > > +               if (error)
> > > > +                       return -ENOENT;
> > > > +       }
> > > > +
> > > > +       return cells_size;
> > > > +}
> > > 
> > > Can't we just hardcode this to "#pinctrl-cells" and skip the cells_name
> > > parameter? We can parametrize it the day we need it instead.
> > 
> > Sure we can do that.
> > 
> > > The rest of the helpers look nice and clean.
> > 
> > OK cool thanks,
> 
> Below is an updated version of this patch with documentation updated
> and cells_name removed. I'll repost the whole series once the DT
> binding has been reviewed.
> 
> Regards,
> 
> Tony
> 8< -------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@atomide.com>
> Date: Tue, 25 Oct 2016 08:33:34 -0700
> Subject: [PATCH] pinctrl: Introduce generic #pinctrl-cells and
>  pinctrl_parse_index_with_args
> 
> Introduce #pinctrl-cells helper binding and generic helper functions
> pinctrl_count_index_with_args() and pinctrl_parse_index_with_args().
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  .../bindings/pinctrl/pinctrl-bindings.txt          |  44 ++++++-

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/pinctrl/devicetree.c                       | 144 +++++++++++++++++++++
>  drivers/pinctrl/devicetree.h                       |  21 +++
>  3 files changed, 208 insertions(+), 1 deletion(-)

Re: [PATCH 1/4] pinctrl: Introduce generic #pinctrl-cells and pinctrl_parse_index_with_args

[Linus Walleij]

On Fri, Oct 28, 2016 at 6:53 PM, Tony Lindgren <tony@atomide.com> wrote:

> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@atomide.com>
> Date: Tue, 25 Oct 2016 08:33:34 -0700
> Subject: [PATCH] pinctrl: Introduce generic #pinctrl-cells and
>  pinctrl_parse_index_with_args
>
> Introduce #pinctrl-cells helper binding and generic helper functions
> pinctrl_count_index_with_args() and pinctrl_parse_index_with_args().
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

This updated version applied with Rob's ACK.

Yours,
Linus Walleij