[PATCH V2 0/6] pinctrl: support platform (e.g. DT) stored pins, groups & functions

[PATCH V2 0/6] pinctrl: support platform (e.g. DT) stored pins, groups & functions

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Two weeks ago I sent
[PATCH RFC] dt-bindings: pinctrl: support specifying pins
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211110231436.8866-1-zajec5@gmail.com/

and week ago I sent
[PATCH 0/5] pinctrl: allow storing pins, groups & functions in DT
https://patchwork.ozlabs.org/project/linux-gpio/list/?series=272685

Initially I planned to allow putting some pinctrl hw details in DT and
later that evolved into a slightly more generic API.

Again:
Please note it's about describing hardware elements and not actual
programming way. It may be used with pinctrl-single.c one day but it's
designed as a generic solution for data.

Patches 1-5 are for linux-pinctrl.git. Patch 6 I found worth including
as DT big example. It can go through Linus with Florian's Ack or I can
send it to Florian later.

Rafał Miłecki (6):
  dt-bindings: pinctrl: support specifying pins, groups & functions
  dt-bindings: pinctrl: brcm,ns-pinmux: extend example
  pinctrl: prepare API for reading pins, groups & functions
  pinctrl: support reading pins, groups & functions from DT
  pinctrl: bcm: pinctrl-ns: supoprt DT specified pins, groups &
    functions
  ARM: dts: BCM5301X: add pinctrl pins, groups & functions

 .../bindings/pinctrl/brcm,ns-pinmux.yaml      |  24 +++-
 .../devicetree/bindings/pinctrl/pinctrl.yaml  |  40 ++++++
 arch/arm/boot/dts/bcm4709.dtsi                |  67 +++++++++
 arch/arm/boot/dts/bcm47094.dtsi               |  11 +-
 arch/arm/boot/dts/bcm5301x.dtsi               | 109 +++++++++++++++
 drivers/pinctrl/bcm/pinctrl-ns.c              |  90 ++++++++----
 drivers/pinctrl/core.c                        |  18 +++
 drivers/pinctrl/core.h                        |   4 +
 drivers/pinctrl/devicetree.c                  | 131 ++++++++++++++++++
 drivers/pinctrl/devicetree.h                  |  29 ++++
 drivers/pinctrl/pinmux.c                      |  10 ++
 drivers/pinctrl/pinmux.h                      |   2 +
 12 files changed, 494 insertions(+), 41 deletions(-)

-- 
2.31.1

[PATCH V2 1/6] dt-bindings: pinctrl: support specifying pins, groups & functions

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This binding change is meant to introduce a generic way of describing
pinctrl blocks details. Every pinmux block is expected to have:
1. Named pins
2. Named groups containing one or more pins
3. Named functions referencing one or more groups

It doesn't describe how hw should be programmed. That remains binding
and driver specific.

This commit describes syntax for "pins", "groups" & "functions" nodes in
a standard pinctrl binding. Every above node allows specifying its
entries and it's done using subnodes.

That design it meant to be generic and extendable (hw specific
properties can be added). Using subnodes allows storing hw specific
pin/group/function configuration.

While it would be possible to have:
groups {
    foo-pins = <0 1>;
    bar-pins = <2 3>;
};
that doesn't allow hw specific quirks.

Introduced design allows e.g.:
groups {
    foo {
        pins = <0 1>;
        vendor,magic = <0xbeaf>;
    };
};

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
Rob: this patch depends on another one I sent 2 weeks ago:
[PATCH 2/2] dt-bindings: pinctrl: use pinctrl.yaml
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211110165720.30242-2-zajec5@gmail.com/

V2: Don't use "reg" property as explained by Tony
---
 .../devicetree/bindings/pinctrl/pinctrl.yaml  | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
index d471563119a9..25d8188fbb26 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
@@ -42,4 +42,44 @@ properties:
       This property can be set either globally for the pin controller or in
       child nodes for individual pin group control.
 
+  pins:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+
+        properties:
+          number:
+            description: Pin number
+            $ref: /schemas/types.yaml#/definitions/uint32
+
+        additionalProperties: false
+
+  groups:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+        description: Group identified by node name
+
+        properties:
+          pins:
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            description: Array of pins belonging to this group
+
+  functions:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+        description: Function identified by node name
+
+        properties:
+          groups:
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+            description: Array of pins groups used by this function
+
 additionalProperties: true
-- 
2.31.1

Re: [PATCH V2 1/6] dt-bindings: pinctrl: support specifying pins, groups & functions

[Tony Lindgren]

Hi,

* Rafał Miłecki <zajec5@gmail.com> [211124 23:05]:
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
> @@ -42,4 +42,44 @@ properties:
>        This property can be set either globally for the pin controller or in
>        child nodes for individual pin group control.
>  
> +  pins:
> +    type: object
> +
> +    patternProperties:
> +      "^.*$":
> +        type: object
> +
> +        properties:
> +          number:
> +            description: Pin number
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +
> +        additionalProperties: false

Please don't introduce Linux kernel internal numbering here. It's
like bringing back the interrupt numbers again. Just make this into
a proper hardware offset from the controller base, so a reg property.
Sure in some cases the reg property is just an index depending on
the controller, we don't really care from the binding point of view.

We already have #pinctrl-cells, so plase do something like the four
ximaginary examples below:

	#pinctrl-cells = <1>;
	...
	pin@foo {
		reg = <0xf00 MUX_MODE0>;
		label = "foo_pin";
	};


	#pinctrl-cells = <2>;
	...
	pin@foo {
		reg = <0xf00 PIN_INPUT_PULLUP MUX_MODE3>;
	};


	#pinctrl-cells = <2>;
	...
	pin@f00 {
		reg = <0xf00 DELAY_PS(0) DELAY_PS(0)>;
	};


	#pinctrl-cells = <3>;
	...
	pin@f00 {
		reg = <0xf00 MUX_MODE3 PULL_UP_STRENGTH(36) PULL_DOWN_STRENGTH(20)>;
	};


Then let's attempt to use just standard numbers and defines for the
values where possible. Then a group of pins is just a list of the pin
phandles in the devicetree.

Regards,

Tony

Re: [PATCH V2 1/6] dt-bindings: pinctrl: support specifying pins, groups & functions

[Rafał Miłecki]

On 25.11.2021 09:49, Tony Lindgren wrote:
> * Rafał Miłecki <zajec5@gmail.com> [211124 23:05]:
>> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
>> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
>> @@ -42,4 +42,44 @@ properties:
>>         This property can be set either globally for the pin controller or in
>>         child nodes for individual pin group control.
>>   
>> +  pins:
>> +    type: object
>> +
>> +    patternProperties:
>> +      "^.*$":
>> +        type: object
>> +
>> +        properties:
>> +          number:
>> +            description: Pin number
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +        additionalProperties: false
> 
> Please don't introduce Linux kernel internal numbering here. It's
> like bringing back the interrupt numbers again.

This is a new bit to me and the reason why I got this binding that way.

I had no idea pin numbering is system specific thing. I always thought
pin numbers are present in every chip datasheets and that is just a part
of hardware.

Now I'm reading https://www.kernel.org/doc/Documentation/pinctrl.txt
again it indeed seems to mention that numbering is handled in a way not
related to specs: "I enumerated the pins from 0 in the upper left corner
to 63 in the lower right corner.".

Sorry for that, I hopefully understand your point correctly now.


> Just make this into
> a proper hardware offset from the controller base, so a reg property.
> Sure in some cases the reg property is just an index depending on
> the controller, we don't really care from the binding point of view.
> 
> We already have #pinctrl-cells, so plase do something like the four
> ximaginary examples below:
> 
> 	#pinctrl-cells = <1>;
> 	...
> 	pin@foo {
> 		reg = <0xf00 MUX_MODE0>;
> 		label = "foo_pin";
> 	};
> 
> 
> 	#pinctrl-cells = <2>;
> 	...
> 	pin@foo {
> 		reg = <0xf00 PIN_INPUT_PULLUP MUX_MODE3>;
> 	};
> 
> 
> 	#pinctrl-cells = <2>;
> 	...
> 	pin@f00 {
> 		reg = <0xf00 DELAY_PS(0) DELAY_PS(0)>;
> 	};
> 
> 
> 	#pinctrl-cells = <3>;
> 	...
> 	pin@f00 {
> 		reg = <0xf00 MUX_MODE3 PULL_UP_STRENGTH(36) PULL_DOWN_STRENGTH(20)>;
> 	};
> 
> 
> Then let's attempt to use just standard numbers and defines for the
> values where possible. Then a group of pins is just a list of the pin
> phandles in the devicetree.

I need to ask for help on understanding that reg = <...> syntax.

(Why) do we need to put that extra info in a "reg" property? That seems
like either:
1. Pin specific info
or
2. Phandle arguments

In the first case, instead of:
	pin@f00 {
		reg = <0xf00 MUX_MODE3 PULL_UP_STRENGTH(36) PULL_DOWN_STRENGTH(20)>;
	};
I'd rather use:
	pin@f00 {
		reg = <0xf00>;
		mux_mode3;
		pull_up_strength = <36>;
		pull_down_strength = <20>;
	};

In the second case, shouldn't that be something like:
	pins {
		bar: pin@f00 {
			reg = <0xf00>;
			#pinctrl-cells = <3>;
		};
	};

	groups {
		qux {
			pins = <&bar MUX_MODE3 PULL_UP_STRENGTH(36) PULL_DOWN_STRENGTH(20)>;
		}
	};

Re: [PATCH V2 1/6] dt-bindings: pinctrl: support specifying pins, groups & functions

[Linus Walleij]

On Thu, Nov 25, 2021 at 1:28 PM Rafał Miłecki <zajec5@gmail.com> wrote:

> This is a new bit to me and the reason why I got this binding that way.
>
> I had no idea pin numbering is system specific thing. I always thought
> pin numbers are present in every chip datasheets and that is just a part
> of hardware.

It's this way because some chips do not even have numbers on the
pins, just names, like "UARTTX" or so.

The numbering is often not numeral, more like a chessboard
at least for BGAs. So "C14" is a typical pin number.

If the datasheet had numbers we just include it in the string naming
that pin like "UARTTX_C14" or so.

Yours,
Linus Walleij

[PATCH V2 2/6] dt-bindings: pinctrl: brcm,ns-pinmux: extend example

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

pinctrl bindings allow specifying pins, groups & functions now. Put some
entries in binding example to help writing DTS files.

Specify pins, groups & functions in the example.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update "pins" to match new binding
---
 .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 24 ++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
index 8d1e5b1cdd5f..e5816a10938c 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
@@ -74,7 +74,7 @@ required:
   - reg
   - reg-names
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
@@ -83,6 +83,28 @@ examples:
         reg = <0x1800c1c0 0x24>;
         reg-names = "cru_gpio_control";
 
+        pins {
+            i2c_scl {
+                number = <4>;
+            };
+
+            i2c_sda {
+                number = <5>;
+            };
+        };
+
+        groups {
+            i2c_grp: i2c_grp {
+                pins = <4 5>;
+            };
+        };
+
+        functions {
+            i2c {
+                groups = <&i2c_grp>;
+            };
+        };
+
         spi-pins {
             function = "spi";
             groups = "spi_grp";
-- 
2.31.1

Re: [PATCH V2 2/6] dt-bindings: pinctrl: brcm,ns-pinmux: extend example

[Rob Herring]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4536 bytes --]

On Thu, 25 Nov 2021 00:04:35 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> pinctrl bindings allow specifying pins, groups & functions now. Put some
> entries in binding example to help writing DTS files.
> 
> Specify pins, groups & functions in the example.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Update "pins" to match new binding
> ---
>  .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 24 ++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
schemas/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: 
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml: 'additionalProperties' is a required property
	hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties"
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: 
warning: no schema found in file: ./Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
make[1]: *** Deleting file 'Documentation/devicetree/bindings/mfd/brcm,cru.example.dt.yaml'
schemas/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: 
Traceback (most recent call last):
  File "/usr/local/bin/dt-validate", line 170, in <module>
    sg.check_trees(filename, testtree)
  File "/usr/local/bin/dt-validate", line 119, in check_trees
    self.check_subtree(dt, subtree, False, "/", "/", filename)
  File "/usr/local/bin/dt-validate", line 110, in check_subtree
    self.check_subtree(tree, value, disabled, name, fullname + name, filename)
  File "/usr/local/bin/dt-validate", line 110, in check_subtree
    self.check_subtree(tree, value, disabled, name, fullname + name, filename)
  File "/usr/local/bin/dt-validate", line 105, in check_subtree
    self.check_node(tree, subtree, disabled, nodename, fullname, filename)
  File "/usr/local/bin/dt-validate", line 49, in check_node
    errors = sorted(dtschema.DTValidator(schema).iter_errors(node), key=lambda e: e.linecol)
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 766, in iter_errors
    for error in super().iter_errors(instance, _schema):
  File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 224, in iter_errors
    for error in errors:
  File "/usr/local/lib/python3.8/dist-packages/jsonschema/_validators.py", line 25, in patternProperties
    yield from validator.descend(
  File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 240, in descend
    for error in self.evolve(schema=schema).iter_errors(instance):
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 766, in iter_errors
    for error in super().iter_errors(instance, _schema):
  File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 224, in iter_errors
    for error in errors:
  File "/usr/local/lib/python3.8/dist-packages/jsonschema/_validators.py", line 298, in ref
    yield from validator.descend(instance, resolved)
  File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 240, in descend
    for error in self.evolve(schema=schema).iter_errors(instance):
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 766, in iter_errors
    for error in super().iter_errors(instance, _schema):
  File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 214, in iter_errors
    scope = id_of(_schema)
  File "/usr/local/lib/python3.8/dist-packages/jsonschema/validators.py", line 90, in _id_of
    return schema.get("$id", "")
AttributeError: 'NoneType' object has no attribute 'get'
make[1]: *** [scripts/Makefile.lib:373: Documentation/devicetree/bindings/mfd/brcm,cru.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1413: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1559467

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH V2 3/6] pinctrl: prepare API for reading pins, groups & functions

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

That API can be used for parsing platform data (coming from e.g. DT or
ACPI) into pinctrl generic structures.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: New patch in the series to address Andy's API review
---
 drivers/pinctrl/core.c   | 12 ++++++++++++
 drivers/pinctrl/core.h   |  4 ++++
 drivers/pinctrl/pinmux.c |  6 ++++++
 drivers/pinctrl/pinmux.h |  2 ++
 4 files changed, 24 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index ffe39336fcac..53b3e8b54a9b 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -515,8 +515,20 @@ void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_range);
 
+int pinctrl_generic_load_pins(struct pinctrl_desc *pctldesc, struct device *dev)
+{
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_load_pins);
+
 #ifdef CONFIG_GENERIC_PINCTRL_GROUPS
 
+int pinctrl_generic_load_groups(struct pinctrl_dev *pctldev)
+{
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_load_groups);
+
 /**
  * pinctrl_generic_get_group_count() - returns the number of pin groups
  * @pctldev: pin controller device
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 840103c40c14..def60d89d37b 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -182,6 +182,8 @@ struct pinctrl_maps {
 	unsigned num_maps;
 };
 
+int pinctrl_generic_load_pins(struct pinctrl_desc *pctldesc, struct device *dev);
+
 #ifdef CONFIG_GENERIC_PINCTRL_GROUPS
 
 /**
@@ -198,6 +200,8 @@ struct group_desc {
 	void *data;
 };
 
+int pinctrl_generic_load_groups(struct pinctrl_dev *pctldev);
+
 int pinctrl_generic_get_group_count(struct pinctrl_dev *pctldev);
 
 const char *pinctrl_generic_get_group_name(struct pinctrl_dev *pctldev,
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 6cdbd9ccf2f0..ef7d2cbf0946 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -788,6 +788,12 @@ void pinmux_init_device_debugfs(struct dentry *devroot,
 
 #ifdef CONFIG_GENERIC_PINMUX_FUNCTIONS
 
+int pinmux_generic_load_functions(struct pinctrl_dev *pctldev)
+{
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_load_functions);
+
 /**
  * pinmux_generic_get_function_count() - returns number of functions
  * @pctldev: pin controller device
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index 78c3a31be882..85e5e3a0222f 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -134,6 +134,8 @@ struct function_desc {
 	void *data;
 };
 
+int pinmux_generic_load_functions(struct pinctrl_dev *pctldev);
+
 int pinmux_generic_get_function_count(struct pinctrl_dev *pctldev);
 
 const char *
-- 
2.31.1

[PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

DT binding allows specifying pins, groups & functions now. That allows
storing them in DT instead of hardcoding in drivers.

This adds support for DT as data source to recently introduced API.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update pinctrl_generic_dt_load_pins() to support new binding
---
 drivers/pinctrl/core.c       |   6 ++
 drivers/pinctrl/devicetree.c | 131 +++++++++++++++++++++++++++++++++++
 drivers/pinctrl/devicetree.h |  29 ++++++++
 drivers/pinctrl/pinmux.c     |   4 ++
 4 files changed, 170 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 53b3e8b54a9b..4c39ca338896 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -517,6 +517,9 @@ EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_range);
 
 int pinctrl_generic_load_pins(struct pinctrl_desc *pctldesc, struct device *dev)
 {
+	if (dev->of_node)
+		return pinctrl_generic_dt_load_pins(pctldesc, dev);
+
 	return -ENOENT;
 }
 EXPORT_SYMBOL_GPL(pinctrl_generic_load_pins);
@@ -525,6 +528,9 @@ EXPORT_SYMBOL_GPL(pinctrl_generic_load_pins);
 
 int pinctrl_generic_load_groups(struct pinctrl_dev *pctldev)
 {
+	if (pctldev->dev->of_node)
+		return pinctrl_generic_load_dt_groups(pctldev);
+
 	return -ENOENT;
 }
 EXPORT_SYMBOL_GPL(pinctrl_generic_load_groups);
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index 3fb238714718..5e511e61449a 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -12,6 +12,7 @@
 
 #include "core.h"
 #include "devicetree.h"
+#include "pinmux.h"
 
 /**
  * struct pinctrl_dt_map - mapping table chunk parsed from device tree
@@ -27,6 +28,136 @@ struct pinctrl_dt_map {
 	unsigned num_maps;
 };
 
+int pinctrl_generic_dt_load_pins(struct pinctrl_desc *pctldesc,
+				struct device *dev)
+{
+	struct pinctrl_pin_desc *descs;
+	struct device_node *pins;
+	struct device_node *np;
+	int err = 0;
+	int i = 0;
+
+	pins = of_get_child_by_name(dev->of_node, "pins");
+	if (!pins) {
+		dev_err(dev, "failed to find \"pins\" DT node\n");
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	pctldesc->npins = of_get_available_child_count(pins);
+
+	descs = devm_kcalloc(dev, pctldesc->npins, sizeof(*descs), GFP_KERNEL);
+	if (!descs) {
+		err = -ENOMEM;
+		goto err_put_node;
+	}
+
+	for_each_available_child_of_node(pins, np) {
+		descs[i].name = np->name;
+
+		if (of_property_read_u32(np, "number", &descs[i].number)) {
+			dev_err(dev, "missing \"number\" property in %pOF\n", np);
+			err = -ENOENT;
+			goto err_put_node;
+		}
+
+		i++;
+	}
+
+	pctldesc->pins = descs;
+
+err_put_node:
+	of_node_put(pins);
+err_out:
+	return err;
+}
+
+#ifdef CONFIG_GENERIC_PINCTRL_GROUPS
+
+int pinctrl_generic_load_dt_groups(struct pinctrl_dev *pctldev)
+{
+	struct device *dev = pctldev->dev;
+	struct device_node *groups;
+	struct device_node *np;
+	int err = 0;
+
+	groups = of_get_child_by_name(dev->of_node, "groups");
+	if (!groups) {
+		dev_err(dev, "failed to find \"groups\" DT node\n");
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	for_each_available_child_of_node(groups, np) {
+		int num_pins;
+		u32 *pins;
+
+		num_pins = of_property_count_u32_elems(np, "pins");
+		pins = devm_kmalloc_array(dev, num_pins, sizeof(*pins), GFP_KERNEL);
+		if (!pins) {
+			err = -ENOMEM;
+			goto err_put_node;
+		}
+
+		if (of_property_read_u32_array(np, "pins", pins, num_pins)) {
+			err = -EIO;
+			goto err_put_node;
+		}
+
+		pinctrl_generic_add_group(pctldev, np->name, pins, num_pins, np);
+	}
+
+err_put_node:
+	of_node_put(groups);
+err_out:
+	return err;
+}
+
+#endif
+
+#ifdef CONFIG_GENERIC_PINMUX_FUNCTIONS
+int pinmux_generic_load_dt_functions(struct pinctrl_dev *pctldev)
+{
+	struct device *dev = pctldev->dev;
+	struct device_node *functions;
+	struct device_node *np;
+	int err = 0;
+
+	functions = of_get_child_by_name(dev->of_node, "functions");
+	if (!functions) {
+		dev_err(dev, "failed to find \"functions\" DT node\n");
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	for_each_available_child_of_node(functions, np) {
+		int num_groups = of_count_phandle_with_args(np, "groups", NULL);
+		struct of_phandle_iterator it;
+		const char **groups;
+		int ret;
+		int i;
+
+		groups = devm_kmalloc_array(dev, num_groups, sizeof(*groups), GFP_KERNEL);
+		if (!groups) {
+			err = -ENOMEM;
+			goto err_put_node;
+		}
+
+		i = 0;
+		of_for_each_phandle(&it, ret, np, "groups", NULL, 0) {
+			groups[i++] = it.node->name;
+		}
+
+		pinmux_generic_add_function(pctldev, np->name, groups, num_groups, np);
+	}
+
+err_put_node:
+	of_node_put(functions);
+err_out:
+	return err;
+}
+#endif
+
 static void dt_free_map(struct pinctrl_dev *pctldev,
 		     struct pinctrl_map *map, unsigned num_maps)
 {
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
index efa80779de4f..156f13896c39 100644
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -9,6 +9,15 @@ struct of_phandle_args;
 
 #ifdef CONFIG_OF
 
+int pinctrl_generic_dt_load_pins(struct pinctrl_desc *pctldesc,
+				struct device *dev);
+#ifdef CONFIG_GENERIC_PINCTRL_GROUPS
+int pinctrl_generic_load_dt_groups(struct pinctrl_dev *pctldev);
+#endif
+#ifdef CONFIG_GENERIC_PINMUX_FUNCTIONS
+int pinmux_generic_load_dt_functions(struct pinctrl_dev *pctldev);
+#endif
+
 void pinctrl_dt_free_maps(struct pinctrl *p);
 int pinctrl_dt_to_map(struct pinctrl *p, struct pinctrl_dev *pctldev);
 
@@ -21,6 +30,26 @@ int pinctrl_parse_index_with_args(const struct device_node *np,
 
 #else
 
+static inline int pinctrl_generic_dt_load_pins(struct pinctrl_desc *pctldesc,
+					      struct device *dev)
+{
+	return -EOPNOTSUPP;
+}
+
+#ifdef CONFIG_GENERIC_PINCTRL_GROUPS
+static inline int pinctrl_generic_load_dt_groups(struct pinctrl_dev *pctldev)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
+#ifdef CONFIG_GENERIC_PINMUX_FUNCTIONS
+static inline int pinmux_generic_load_dt_functions(struct pinctrl_dev *pctldev)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
 static inline int pinctrl_dt_to_map(struct pinctrl *p,
 				    struct pinctrl_dev *pctldev)
 {
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index ef7d2cbf0946..36a1d1af4a20 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -27,6 +27,7 @@
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinmux.h>
 #include "core.h"
+#include "devicetree.h"
 #include "pinmux.h"
 
 int pinmux_check_ops(struct pinctrl_dev *pctldev)
@@ -790,6 +791,9 @@ void pinmux_init_device_debugfs(struct dentry *devroot,
 
 int pinmux_generic_load_functions(struct pinctrl_dev *pctldev)
 {
+	if (pctldev->dev->of_node)
+		return pinmux_generic_load_dt_functions(pctldev);
+
 	return -ENOENT;
 }
 EXPORT_SYMBOL_GPL(pinmux_generic_load_functions);
-- 
2.31.1

Re: [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT

[Andy Shevchenko]

On Thu, Nov 25, 2021 at 1:04 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>
> From: Rafał Miłecki <rafal@milecki.pl>
>
> DT binding allows specifying pins, groups & functions now. That allows
> storing them in DT instead of hardcoding in drivers.
>
> This adds support for DT as data source to recently introduced API.

a data
the recently

...

>  int pinctrl_generic_load_pins(struct pinctrl_desc *pctldesc, struct device *dev)
>  {
> +       if (dev->of_node)

Again, it's a layering violation.

> +               return pinctrl_generic_dt_load_pins(pctldesc, dev);
> +
>         return -ENOENT;
>  }
>  EXPORT_SYMBOL_GPL(pinctrl_generic_load_pins);

>  int pinctrl_generic_load_groups(struct pinctrl_dev *pctldev)
>  {
> +       if (pctldev->dev->of_node)
> +               return pinctrl_generic_load_dt_groups(pctldev);
> +
>         return -ENOENT;
>  }
>  EXPORT_SYMBOL_GPL(pinctrl_generic_load_groups);

>  int pinmux_generic_load_functions(struct pinctrl_dev *pctldev)
>  {
> +       if (pctldev->dev->of_node)
> +               return pinmux_generic_load_dt_functions(pctldev);
> +
>         return -ENOENT;
>  }
>  EXPORT_SYMBOL_GPL(pinmux_generic_load_functions);

Have you thought about making ops structure (or ops inside existing
structure) and all above will be something like

  stuct ops *... = ...->ops;

  if (ops && ops->METHOD)
    return ops->METHOD(...);

  return -ERRNO;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT

[Andy Shevchenko]

On Thu, Nov 25, 2021 at 11:49 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Nov 25, 2021 at 1:04 AM Rafał Miłecki <zajec5@gmail.com> wrote:

> Have you thought about making ops structure (or ops inside existing
> structure) and all above will be something like
>
>   stuct ops *... = ...->ops;
>
>   if (ops && ops->METHOD)
>     return ops->METHOD(...);
>
>   return -ERRNO;

Forgot to add that ops assignment should happen at the pin control
enumeration or something like that. Quite early and quite globally.

-- 
With Best Regards,
Andy Shevchenko

[PATCH V2 5/6] pinctrl: bcm: pinctrl-ns: supoprt DT specified pins, groups & functions

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

It's now possible to specify hardware pins, groups & functions in DT
instead of hardcoding that info in a driver. Use pinctrl subsystem
helpers to extract that info from DT.

Keep hardcoded data as fallback method.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/pinctrl/bcm/pinctrl-ns.c | 90 +++++++++++++++++++++-----------
 1 file changed, 60 insertions(+), 30 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-ns.c b/drivers/pinctrl/bcm/pinctrl-ns.c
index 0897041b5ef1..9036d62c806f 100644
--- a/drivers/pinctrl/bcm/pinctrl-ns.c
+++ b/drivers/pinctrl/bcm/pinctrl-ns.c
@@ -213,7 +213,11 @@ static int ns_pinctrl_probe(struct platform_device *pdev)
 	struct ns_pinctrl *ns_pinctrl;
 	struct pinctrl_desc *pctldesc;
 	struct pinctrl_pin_desc *pin;
+	struct device_node *functions;
+	struct device_node *groups;
+	struct device_node *pins;
 	struct resource *res;
+	int err;
 	int i;
 
 	ns_pinctrl = devm_kzalloc(dev, sizeof(*ns_pinctrl), GFP_KERNEL);
@@ -243,19 +247,27 @@ static int ns_pinctrl_probe(struct platform_device *pdev)
 
 	/* Set pinctrl properties */
 
-	pctldesc->pins = devm_kcalloc(dev, ARRAY_SIZE(ns_pinctrl_pins),
-				      sizeof(struct pinctrl_pin_desc),
-				      GFP_KERNEL);
-	if (!pctldesc->pins)
-		return -ENOMEM;
-	for (i = 0, pin = (struct pinctrl_pin_desc *)&pctldesc->pins[0];
-	     i < ARRAY_SIZE(ns_pinctrl_pins); i++) {
-		const struct pinctrl_pin_desc *src = &ns_pinctrl_pins[i];
-		unsigned int chipsets = (uintptr_t)src->drv_data;
-
-		if (chipsets & ns_pinctrl->chipset_flag) {
-			memcpy(pin++, src, sizeof(*src));
-			pctldesc->npins++;
+	pins = of_get_child_by_name(dev->of_node, "pins");
+	if (pins) {
+		err = pinctrl_generic_load_pins(pctldesc, dev);
+		of_node_put(pins);
+		if (err)
+			return err;
+	} else {
+		pctldesc->pins = devm_kcalloc(dev, ARRAY_SIZE(ns_pinctrl_pins),
+					sizeof(struct pinctrl_pin_desc),
+					GFP_KERNEL);
+		if (!pctldesc->pins)
+			return -ENOMEM;
+		for (i = 0, pin = (struct pinctrl_pin_desc *)&pctldesc->pins[0];
+		     i < ARRAY_SIZE(ns_pinctrl_pins); i++) {
+			const struct pinctrl_pin_desc *src = &ns_pinctrl_pins[i];
+			unsigned int chipsets = (uintptr_t)src->drv_data;
+
+			if (chipsets & ns_pinctrl->chipset_flag) {
+				memcpy(pin++, src, sizeof(*src));
+				pctldesc->npins++;
+			}
 		}
 	}
 
@@ -267,25 +279,43 @@ static int ns_pinctrl_probe(struct platform_device *pdev)
 		return PTR_ERR(ns_pinctrl->pctldev);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(ns_pinctrl_groups); i++) {
-		const struct ns_pinctrl_group *group = &ns_pinctrl_groups[i];
-
-		if (!(group->chipsets & ns_pinctrl->chipset_flag))
-			continue;
-
-		pinctrl_generic_add_group(ns_pinctrl->pctldev, group->name,
-					  group->pins, group->num_pins, NULL);
+	groups = of_get_child_by_name(dev->of_node, "groups");
+	if (groups) {
+		err = pinctrl_generic_load_groups(ns_pinctrl->pctldev);
+		of_node_put(groups);
+		if (err)
+			return err;
+	} else {
+		for (i = 0; i < ARRAY_SIZE(ns_pinctrl_groups); i++) {
+			const struct ns_pinctrl_group *group = &ns_pinctrl_groups[i];
+
+			if (!(group->chipsets & ns_pinctrl->chipset_flag))
+				continue;
+
+			pinctrl_generic_add_group(ns_pinctrl->pctldev,
+						  group->name, group->pins,
+						  group->num_pins, NULL);
+		}
 	}
 
-	for (i = 0; i < ARRAY_SIZE(ns_pinctrl_functions); i++) {
-		const struct ns_pinctrl_function *function = &ns_pinctrl_functions[i];
-
-		if (!(function->chipsets & ns_pinctrl->chipset_flag))
-			continue;
-
-		pinmux_generic_add_function(ns_pinctrl->pctldev, function->name,
-					    function->groups,
-					    function->num_groups, NULL);
+	functions = of_get_child_by_name(dev->of_node, "functions");
+	if (functions) {
+		err = pinmux_generic_load_functions(ns_pinctrl->pctldev);
+		of_node_put(functions);
+		if (err)
+			return err;
+	} else {
+		for (i = 0; i < ARRAY_SIZE(ns_pinctrl_functions); i++) {
+			const struct ns_pinctrl_function *function = &ns_pinctrl_functions[i];
+
+			if (!(function->chipsets & ns_pinctrl->chipset_flag))
+				continue;
+
+			pinmux_generic_add_function(ns_pinctrl->pctldev,
+						    function->name,
+						    function->groups,
+						    function->num_groups, NULL);
+		}
 	}
 
 	return 0;
-- 
2.31.1

[PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

They can now be described in DT so do that.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update "pins" to match updated binding
---
 arch/arm/boot/dts/bcm4709.dtsi  |  67 ++++++++++++++++++++
 arch/arm/boot/dts/bcm47094.dtsi |  11 +---
 arch/arm/boot/dts/bcm5301x.dtsi | 109 ++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/bcm4709.dtsi b/arch/arm/boot/dts/bcm4709.dtsi
index cba3d910bed8..481dc3e9353b 100644
--- a/arch/arm/boot/dts/bcm4709.dtsi
+++ b/arch/arm/boot/dts/bcm4709.dtsi
@@ -10,6 +10,73 @@ &uart0 {
 	status = "okay";
 };
 
+&pinctrl {
+	compatible = "brcm,bcm4709-pinmux";
+
+	pins {
+		mdc {
+			number = <6>;
+		};
+
+		mdio {
+			number = <7>;
+		};
+
+		uart2_rx {
+			number = <16>;
+		};
+
+		uart2_tx {
+			number = <17>;
+		};
+
+		/* TODO
+		 * xtal_out {
+		 * };
+		 */
+
+		sdio_pwr {
+			number = <22>;
+		};
+
+		sdio_en_1p8v {
+			number = <23>;
+		};
+	};
+
+	groups {
+		mdio_grp: mdio_grp {
+			pins = <6 7>;
+		};
+
+		uart2_grp: uart2_grp {
+			pins = <16 17>;
+		};
+
+		sdio_pwr_grp: sdio_pwr_grp {
+			pins = <22>;
+		};
+
+		sdio_1p8v_grp: sdio_1p8v_grp {
+			pins = <23>;
+		};
+	};
+
+	functions {
+		mdio {
+			groups = <&mdio_grp>;
+		};
+
+		uart2 {
+			groups = <&uart2_grp>;
+		};
+
+		sdio {
+			groups = <&sdio_pwr_grp &sdio_1p8v_grp>;
+		};
+	};
+};
+
 &srab {
 	compatible = "brcm,bcm53012-srab", "brcm,bcm5301x-srab";
 };
diff --git a/arch/arm/boot/dts/bcm47094.dtsi b/arch/arm/boot/dts/bcm47094.dtsi
index 6282363313e1..239c1c1b0268 100644
--- a/arch/arm/boot/dts/bcm47094.dtsi
+++ b/arch/arm/boot/dts/bcm47094.dtsi
@@ -3,14 +3,12 @@
  * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
  */
 
-#include "bcm4708.dtsi"
+#include "bcm4709.dtsi"
 
 / {
 };
 
 &pinctrl {
-	compatible = "brcm,bcm4709-pinmux";
-
 	pinmux_mdio: mdio-pins {
 		groups = "mdio_grp";
 		function = "mdio";
@@ -21,11 +19,4 @@ &usb3_phy {
 	compatible = "brcm,ns-bx-usb3-phy";
 };
 
-&uart0 {
-	clock-frequency = <125000000>;
-	status = "okay";
-};
 
-&srab {
-	compatible = "brcm,bcm53012-srab", "brcm,bcm5301x-srab";
-};
diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
index d4f355015e3c..dc89d2f5fa8f 100644
--- a/arch/arm/boot/dts/bcm5301x.dtsi
+++ b/arch/arm/boot/dts/bcm5301x.dtsi
@@ -473,6 +473,115 @@ pinmux_uart1: uart1-pins {
 					groups = "uart1_grp";
 					function = "uart1";
 				};
+
+				pins {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					spi_clk {
+						number = <0>;
+					};
+
+					spi_ss {
+						number = <1>;
+					};
+
+					spi_mosi {
+						number = <2>;
+					};
+
+					spi_miso {
+						number = <3>;
+					};
+
+					i2c_scl {
+						number = <4>;
+					};
+
+					i2c_sda {
+						number = <5>;
+					};
+
+					pwm0 {
+						number = <8>;
+					};
+
+					pwm1 {
+						number = <9>;
+					};
+
+					pwm2 {
+						number = <10>;
+					};
+
+					pwm3 {
+						number = <11>;
+					};
+
+					uart1_rx {
+						number = <12>;
+					};
+
+					uart1_tx {
+						number = <13>;
+					};
+
+					uart1_cts {
+						number = <14>;
+					};
+
+					uart1_rts {
+						number = <15>;
+					};
+				};
+
+				groups {
+					spi_grp: spi_grp {
+						pins = <0 1 2 3>;
+					};
+
+					i2c_grp: i2c_grp {
+						pins = <4 5>;
+					};
+
+					pwm0_grp: pwm0_grp {
+						pins = <8>;
+					};
+
+					pwm1_grp: pwm1_grp {
+						pins = <9>;
+					};
+
+					pwm2_grp: pwm2_grp {
+						pins = <10>;
+					};
+
+					pwm3_grp: pwm3_grp {
+						pins = <11>;
+					};
+
+					uart1_grp: uart1_grp {
+						pins = <12 13 14 15>;
+					};
+				};
+
+				functions {
+					spi {
+						groups = <&spi_grp>;
+					};
+
+					i2c {
+						groups = <&i2c_grp>;
+					};
+
+					pwm {
+						groups = <&pwm0_grp &pwm1_grp &pwm2_grp &pwm3_grp>;
+					};
+
+					uart1 {
+						groups = <&uart1_grp>;
+					};
+				};
 			};
 
 			thermal: thermal@2c0 {
-- 
2.31.1

Re: [PATCH V2 0/6] pinctrl: support platform (e.g. DT) stored pins, groups & functions

[Andy Shevchenko]

On Thu, Nov 25, 2021 at 1:04 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Two weeks ago I sent
> [PATCH RFC] dt-bindings: pinctrl: support specifying pins
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211110231436.8866-1-zajec5@gmail.com/
>
> and week ago I sent
> [PATCH 0/5] pinctrl: allow storing pins, groups & functions in DT
> https://patchwork.ozlabs.org/project/linux-gpio/list/?series=272685
>
> Initially I planned to allow putting some pinctrl hw details in DT and
> later that evolved into a slightly more generic API.
>
> Again:
> Please note it's about describing hardware elements and not actual
> programming way. It may be used with pinctrl-single.c one day but it's
> designed as a generic solution for data.
>
> Patches 1-5 are for linux-pinctrl.git. Patch 6 I found worth including
> as DT big example. It can go through Linus with Florian's Ack or I can
> send it to Florian later.

Thank you for an update! What I would like to see in the cover letter
and esp. in the updated documentation of the pin control subsystem is
the architectural (design) point of view. There is no sense to discuss
the following patches without a big picture. For example, should we
allow some of the DT information to be passed on ACPI based platforms
(due to OF related enumeration of the devices in ACPI environment, see
PRP0001 for the details)? Or i.o.w. Do all these properties make sense
only in the realm of the provider? I believe it's true for pin
controller devices and false for consumer devices, so, the question
is, does pin controller device support any type of hogs or it's only
property of GPIO? It's all not clear to me with this cover letter and
absence of the documentation.

Besides that, consider test cases to be added (OF has its unittest
built-in into the kernel).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V2 0/6] pinctrl: support platform (e.g. DT) stored pins, groups & functions

[Rafał Miłecki]

On 25.11.2021 10:58, Andy Shevchenko wrote:
> Besides that, consider test cases to be added (OF has its unittest
> built-in into the kernel).

I'm confused here. We indeed have drivers/of/unittest.c but that seems
to cover in-kernel DT API. All kind of of_*() helper functions.

What I add is a simple binding support. I don't extend kernel API. What
possibly should I add to unit tests?