[PATCH V3 0/2] dt-bindings: pinctrl: pins, groups & functions

[PATCH V3 0/2] dt-bindings: pinctrl: pins, groups & functions

[Rafał Miłecki]

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

This is my minimalized attempt of setting up pins, groups & functions
bindings.

I've been warned that Linux pinctrl subsystem may require refactoring
before it's ready to handle such bindings properly and that appears to
be correct. I'll need to invest more time into reorganizing Linux
structs. Right now it's not ready for tree-like design and it uses
more-or-less magic numbers to handle pins <-> groups relation.

Meanwhile I'd like to get dt-bindings reviewed & possibly merged. To be
honest - my initial reason for that work was developing U-Boot drivers.

Please kindly review those bindings and optionally treat my WIP work on
Linux implementation as proof of concept.

Reference:
RFC: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211110231436.8866-1-zajec5@gmail.com/
V1: https://patchwork.ozlabs.org/project/linux-gpio/list/?series=272685&submitter=&state=*&q=&archive=&delegate=
V2: https://patchwork.ozlabs.org/project/linux-gpio/list/?series=273711&submitter=&state=*&q=&archive=&delegate=

Rafał Miłecki (2):
  dt-bindings: pinctrl: support specifying pins, groups & functions
  dt-bindings: pinctrl: brcm,ns-pinmux: describe pins, groups &
    functions

 .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 38 ++++++++++++++++++-
 .../devicetree/bindings/pinctrl/pinctrl.yaml  | 34 +++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH V3 0/2] dt-bindings: pinctrl: pins, groups & functions

[Rafał Miłecki]

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

This is my minimalized attempt of setting up pins, groups & functions
bindings.

I've been warned that Linux pinctrl subsystem may require refactoring
before it's ready to handle such bindings properly and that appears to
be correct. I'll need to invest more time into reorganizing Linux
structs. Right now it's not ready for tree-like design and it uses
more-or-less magic numbers to handle pins <-> groups relation.

Meanwhile I'd like to get dt-bindings reviewed & possibly merged. To be
honest - my initial reason for that work was developing U-Boot drivers.

Please kindly review those bindings and optionally treat my WIP work on
Linux implementation as proof of concept.

Reference:
RFC: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211110231436.8866-1-zajec5@gmail.com/
V1: https://patchwork.ozlabs.org/project/linux-gpio/list/?series=272685&submitter=&state=*&q=&archive=&delegate=
V2: https://patchwork.ozlabs.org/project/linux-gpio/list/?series=273711&submitter=&state=*&q=&archive=&delegate=

Rafał Miłecki (2):
  dt-bindings: pinctrl: support specifying pins, groups & functions
  dt-bindings: pinctrl: brcm,ns-pinmux: describe pins, groups &
    functions

 .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 38 ++++++++++++++++++-
 .../devicetree/bindings/pinctrl/pinctrl.yaml  | 34 +++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V3 1/2] 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 a common syntax for "pins", "groups" & "functions"
nodes. Every node allows specifying its entries.

That design is meant to be extendable and minimalistic enough to be
generic (matching any hardware). Relations between pins, groups and
functions are expected to be the same for every hardware.

Using subnode objects allows extending this binding to cover hardware
specific needs (e.g. custom values). Example to consider:

pins {
    foo: foo {
        vendor,magic = <0xc0fe>;
    };
}

groups {
    bar {
        pins = <&foo>;
        vendor,secret = <0xbeaf>;
    };
};

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/pinctrl/pinctrl.yaml  | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
index d471563119a9..e36662cb1f3b 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
@@ -42,4 +42,38 @@ 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
+        description: Pin named by node name
+
+  groups:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+        description: Group named by node name
+
+        properties:
+          pins:
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+            description: Array of pins belonging to this group
+
+  functions:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+        description: Function named by node name
+
+        properties:
+          groups:
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+            description: Array of groups used by this function
+
 additionalProperties: true
-- 
2.31.1

[PATCH V3 1/2] 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 a common syntax for "pins", "groups" & "functions"
nodes. Every node allows specifying its entries.

That design is meant to be extendable and minimalistic enough to be
generic (matching any hardware). Relations between pins, groups and
functions are expected to be the same for every hardware.

Using subnode objects allows extending this binding to cover hardware
specific needs (e.g. custom values). Example to consider:

pins {
    foo: foo {
        vendor,magic = <0xc0fe>;
    };
}

groups {
    bar {
        pins = <&foo>;
        vendor,secret = <0xbeaf>;
    };
};

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/pinctrl/pinctrl.yaml  | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
index d471563119a9..e36662cb1f3b 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml
@@ -42,4 +42,38 @@ 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
+        description: Pin named by node name
+
+  groups:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+        description: Group named by node name
+
+        properties:
+          pins:
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+            description: Array of pins belonging to this group
+
+  functions:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+        description: Function named by node name
+
+        properties:
+          groups:
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+            description: Array of groups used by this function
+
 additionalProperties: true
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V3 2/2] dt-bindings: pinctrl: brcm,ns-pinmux: describe pins, groups & functions

[Rafał Miłecki]

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

Use and extend generic pinctrl binding to include info about pins,
groups & functions.

Northstar platform pins have numbers assigned to them (they are needed
for mux programming) so add a custom "number" property for that.

Extend example to provide a complete binding of a single / random
function.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 38 ++++++++++++++++++-
 1 file changed, 37 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..82d3e52a2229 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
@@ -30,6 +30,20 @@ properties:
   reg-names:
     const: cru_gpio_control
 
+  pins:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+
+        properties:
+          number:
+            description: Pin number
+            $ref: /schemas/types.yaml#/definitions/uint32
+
+        unevaluatedProperties: false
+
 patternProperties:
   '-pins$':
     type: object
@@ -74,7 +88,7 @@ required:
   - reg
   - reg-names
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
@@ -83,6 +97,28 @@ examples:
         reg = <0x1800c1c0 0x24>;
         reg-names = "cru_gpio_control";
 
+        pins {
+            i2c_scl: i2c_scl {
+                number = <4>;
+            };
+
+            i2c_sda: i2c_sda {
+                number = <5>;
+            };
+        };
+
+        groups {
+            i2c_grp: i2c_grp {
+                pins = <&i2c_scl &i2c_sda>;
+            };
+        };
+
+        functions {
+            i2c {
+                groups = <&i2c_grp>;
+            };
+        };
+
         spi-pins {
             function = "spi";
             groups = "spi_grp";
-- 
2.31.1

[PATCH V3 2/2] dt-bindings: pinctrl: brcm, ns-pinmux: describe pins, groups & functions

[Rafał Miłecki]

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

Use and extend generic pinctrl binding to include info about pins,
groups & functions.

Northstar platform pins have numbers assigned to them (they are needed
for mux programming) so add a custom "number" property for that.

Extend example to provide a complete binding of a single / random
function.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 38 ++++++++++++++++++-
 1 file changed, 37 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..82d3e52a2229 100644
--- a/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.yaml
@@ -30,6 +30,20 @@ properties:
   reg-names:
     const: cru_gpio_control
 
+  pins:
+    type: object
+
+    patternProperties:
+      "^.*$":
+        type: object
+
+        properties:
+          number:
+            description: Pin number
+            $ref: /schemas/types.yaml#/definitions/uint32
+
+        unevaluatedProperties: false
+
 patternProperties:
   '-pins$':
     type: object
@@ -74,7 +88,7 @@ required:
   - reg
   - reg-names
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
@@ -83,6 +97,28 @@ examples:
         reg = <0x1800c1c0 0x24>;
         reg-names = "cru_gpio_control";
 
+        pins {
+            i2c_scl: i2c_scl {
+                number = <4>;
+            };
+
+            i2c_sda: i2c_sda {
+                number = <5>;
+            };
+        };
+
+        groups {
+            i2c_grp: i2c_grp {
+                pins = <&i2c_scl &i2c_sda>;
+            };
+        };
+
+        functions {
+            i2c {
+                groups = <&i2c_grp>;
+            };
+        };
+
         spi-pins {
             function = "spi";
             groups = "spi_grp";
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 2/2] dt-bindings: pinctrl: brcm,ns-pinmux: describe pins, groups & functions

[Rob Herring]

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

On Fri, 10 Dec 2021 12:42:22 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Use and extend generic pinctrl binding to include info about pins,
> groups & functions.
> 
> Northstar platform pins have numbers assigned to them (they are needed
> for mux programming) so add a custom "number" property for that.
> 
> Extend example to provide a complete binding of a single / random
> function.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 38 ++++++++++++++++++-
>  1 file changed, 37 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:
/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#
schemas/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: 
/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
Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.example.dt.yaml:0:0: /example-0/pin-controller@1800c1c0: failed to match any schema with compatible: ['brcm,bcm4708-pinmux']
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/1566458

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.

Re: [PATCH V3 2/2] dt-bindings: pinctrl: brcm, ns-pinmux: describe pins, groups & functions

[Rob Herring]

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

On Fri, 10 Dec 2021 12:42:22 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Use and extend generic pinctrl binding to include info about pins,
> groups & functions.
> 
> Northstar platform pins have numbers assigned to them (they are needed
> for mux programming) so add a custom "number" property for that.
> 
> Extend example to provide a complete binding of a single / random
> function.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  .../bindings/pinctrl/brcm,ns-pinmux.yaml      | 38 ++++++++++++++++++-
>  1 file changed, 37 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:
/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#
schemas/pinctrl/brcm,ns-pinmux.yaml: ignoring, error in schema: 
/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
Documentation/devicetree/bindings/pinctrl/brcm,ns-pinmux.example.dt.yaml:0:0: /example-0/pin-controller@1800c1c0: failed to match any schema with compatible: ['brcm,bcm4708-pinmux']
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/1566458

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.



[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Linus Walleij]

On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote:

> 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.

So what this does is to take a large chunk of data that we known to be
associated with the compatible string (names of pins, groups and functions,
etc) and put it into the device tree instead of the alternative, which is
what most drivers do, and that is to compile in the data into the
operating system and just look it up by using a compatible
string.

The DT maintainers have already indicated that this is not desirable
and I don't see it getting merged before it has a Reviewed-by
tag from one of the DT binding maintainers.

I think we need to know what the USP (unique selling point) is?

Would it be something like not having to duplicate work across some
boot loaders and operating systems? (Well they all need to parse this
type of description but that can be put into a library.)

Or something else?

Yours,
Linus Walleij

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

[Linus Walleij]

On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote:

> 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.

So what this does is to take a large chunk of data that we known to be
associated with the compatible string (names of pins, groups and functions,
etc) and put it into the device tree instead of the alternative, which is
what most drivers do, and that is to compile in the data into the
operating system and just look it up by using a compatible
string.

The DT maintainers have already indicated that this is not desirable
and I don't see it getting merged before it has a Reviewed-by
tag from one of the DT binding maintainers.

I think we need to know what the USP (unique selling point) is?

Would it be something like not having to duplicate work across some
boot loaders and operating systems? (Well they all need to parse this
type of description but that can be put into a library.)

Or something else?

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 2/2] dt-bindings: pinctrl: brcm,ns-pinmux: describe pins, groups & functions

[Tony Lindgren]

* Rafał Miłecki <zajec5@gmail.com> [211210 11:43]:
> @@ -83,6 +97,28 @@ examples:
>          reg = <0x1800c1c0 0x24>;
>          reg-names = "cru_gpio_control";
>  
> +        pins {
> +            i2c_scl: i2c_scl {
> +                number = <4>;
> +            };
> +
> +            i2c_sda: i2c_sda {
> +                number = <5>;
> +            };
> +        };

Please don't add custom properties for something that can be done with
standard register based addressing using a hardware offset based reg
property. Your driver can easily translate it. Also, please don't use
custom node names, instead do:

pins {
	i2c_scl: pin@0x1234 {
		/* Any generic standard properties or numbers here please :)d */
	};
	...
};

I think I've already commented on the register addressing twice before..

Regards,

Tony

Re: [PATCH V3 2/2] dt-bindings: pinctrl: brcm,ns-pinmux: describe pins, groups & functions

[Tony Lindgren]

* Rafał Miłecki <zajec5@gmail.com> [211210 11:43]:
> @@ -83,6 +97,28 @@ examples:
>          reg = <0x1800c1c0 0x24>;
>          reg-names = "cru_gpio_control";
>  
> +        pins {
> +            i2c_scl: i2c_scl {
> +                number = <4>;
> +            };
> +
> +            i2c_sda: i2c_sda {
> +                number = <5>;
> +            };
> +        };

Please don't add custom properties for something that can be done with
standard register based addressing using a hardware offset based reg
property. Your driver can easily translate it. Also, please don't use
custom node names, instead do:

pins {
	i2c_scl: pin@0x1234 {
		/* Any generic standard properties or numbers here please :)d */
	};
	...
};

I think I've already commented on the register addressing twice before..

Regards,

Tony

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rafał Miłecki]

Rob: please kindly comment on this idea of storing pins/groups/functions
in DT.

For a sample Linux implementation you can check (incomplete):
[PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT
https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/

For a real life DT usage you can check:
[PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions
https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/

Also see below inline comments.


On 11.12.2021 00:26, Linus Walleij wrote:
> On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> 
>> 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.
> 
> So what this does is to take a large chunk of data that we known to be
> associated with the compatible string (names of pins, groups and functions,
> etc) and put it into the device tree instead of the alternative, which is
> what most drivers do, and that is to compile in the data into the
> operating system and just look it up by using a compatible
> string.

Correct. It changes the place of storing platform specific data.


> The DT maintainers have already indicated that this is not desirable
> and I don't see it getting merged before it has a Reviewed-by
> tag from one of the DT binding maintainers.

Tony pointed out that it was back in 2011. It's worth reconsidering.
https://patchwork.ozlabs.org/comment/2786915/

Rob said it depends on whether "data be static (complete) and correct"
https://patchwork.ozlabs.org/comment/2786688/

I find it absolutely required to get Rob's Reviewed-by before we merge
it. I hope we can get Rob's opinion on this.


> I think we need to know what the USP (unique selling point) is?
> 
> Would it be something like not having to duplicate work across some
> boot loaders and operating systems? (Well they all need to parse this
> type of description but that can be put into a library.)
> 
> Or something else?

There are two reasons for me to work on this binding:


1. I think it's cleaner to keep pinctrl details in DT

DT seems more natural (than C code) for:
a) Translating info from datasheets
b) Storing pin/group/function custom values
c) Defining relations (phandles)
d) Handling chip differences (adding new pins in newer revisions)

Last time I learnt that pins don't always/usually have numbers (in
datasheets) but are rather named. Still in the "pinctrl_pin_desc" we
have "unsigned number" just to enumerate them and reference in groups.

Adding or removing pins/groups/functions in DT is as simple as
adding/deleting nodes. That also means less logic in C code.


2. It avoids data duplication

It allows me to keep pins/groups/functions data in one place (DT) and
use it in both: Linux and U-Boot. No duplication & easier maintenance.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rafał Miłecki]

Rob: please kindly comment on this idea of storing pins/groups/functions
in DT.

For a sample Linux implementation you can check (incomplete):
[PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT
https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/

For a real life DT usage you can check:
[PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions
https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/

Also see below inline comments.


On 11.12.2021 00:26, Linus Walleij wrote:
> On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> 
>> 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.
> 
> So what this does is to take a large chunk of data that we known to be
> associated with the compatible string (names of pins, groups and functions,
> etc) and put it into the device tree instead of the alternative, which is
> what most drivers do, and that is to compile in the data into the
> operating system and just look it up by using a compatible
> string.

Correct. It changes the place of storing platform specific data.


> The DT maintainers have already indicated that this is not desirable
> and I don't see it getting merged before it has a Reviewed-by
> tag from one of the DT binding maintainers.

Tony pointed out that it was back in 2011. It's worth reconsidering.
https://patchwork.ozlabs.org/comment/2786915/

Rob said it depends on whether "data be static (complete) and correct"
https://patchwork.ozlabs.org/comment/2786688/

I find it absolutely required to get Rob's Reviewed-by before we merge
it. I hope we can get Rob's opinion on this.


> I think we need to know what the USP (unique selling point) is?
> 
> Would it be something like not having to duplicate work across some
> boot loaders and operating systems? (Well they all need to parse this
> type of description but that can be put into a library.)
> 
> Or something else?

There are two reasons for me to work on this binding:


1. I think it's cleaner to keep pinctrl details in DT

DT seems more natural (than C code) for:
a) Translating info from datasheets
b) Storing pin/group/function custom values
c) Defining relations (phandles)
d) Handling chip differences (adding new pins in newer revisions)

Last time I learnt that pins don't always/usually have numbers (in
datasheets) but are rather named. Still in the "pinctrl_pin_desc" we
have "unsigned number" just to enumerate them and reference in groups.

Adding or removing pins/groups/functions in DT is as simple as
adding/deleting nodes. That also means less logic in C code.


2. It avoids data duplication

It allows me to keep pins/groups/functions data in one place (DT) and
use it in both: Linux and U-Boot. No duplication & easier maintenance.

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

[Rob Herring]

On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote:
> Rob: please kindly comment on this idea of storing pins/groups/functions
> in DT.

I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a 
one time stuffing of register values. And given how many things run 
before getting to the kernel, doing proper pin configuration in the 
kernel is much too late (or redundant because it was actually already 
done).

> 
> For a sample Linux implementation you can check (incomplete):
> [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT
> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/
> 
> For a real life DT usage you can check:
> [PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions
> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/

What about h/w with no concept of 'groups'?


> Also see below inline comments.
> 
> 
> On 11.12.2021 00:26, Linus Walleij wrote:
> > On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> > 
> > > 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.
> > 
> > So what this does is to take a large chunk of data that we known to be
> > associated with the compatible string (names of pins, groups and functions,
> > etc) and put it into the device tree instead of the alternative, which is
> > what most drivers do, and that is to compile in the data into the
> > operating system and just look it up by using a compatible
> > string.
> 
> Correct. It changes the place of storing platform specific data.
> 
> 
> > The DT maintainers have already indicated that this is not desirable
> > and I don't see it getting merged before it has a Reviewed-by
> > tag from one of the DT binding maintainers.
> 
> Tony pointed out that it was back in 2011. It's worth reconsidering.
> https://patchwork.ozlabs.org/comment/2786915/
> 
> Rob said it depends on whether "data be static (complete) and correct"
> https://patchwork.ozlabs.org/comment/2786688/

I haven't seen an answer for that question...

That and working for multiple platforms (from different vendors) are the 
main things that matter to me. 

> I find it absolutely required to get Rob's Reviewed-by before we merge
> it. I hope we can get Rob's opinion on this.
> 
> 
> > I think we need to know what the USP (unique selling point) is?
> > 
> > Would it be something like not having to duplicate work across some
> > boot loaders and operating systems? (Well they all need to parse this
> > type of description but that can be put into a library.)
> > 
> > Or something else?
> 
> There are two reasons for me to work on this binding:
> 
> 
> 1. I think it's cleaner to keep pinctrl details in DT
> 
> DT seems more natural (than C code) for:
> a) Translating info from datasheets
> b) Storing pin/group/function custom values
> c) Defining relations (phandles)
> d) Handling chip differences (adding new pins in newer revisions)
> 
> Last time I learnt that pins don't always/usually have numbers (in
> datasheets) but are rather named. Still in the "pinctrl_pin_desc" we
> have "unsigned number" just to enumerate them and reference in groups.
> 
> Adding or removing pins/groups/functions in DT is as simple as
> adding/deleting nodes. That also means less logic in C code.
> 
> 
> 2. It avoids data duplication
> 
> It allows me to keep pins/groups/functions data in one place (DT) and
> use it in both: Linux and U-Boot. No duplication & easier maintenance.
> 

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

[Rob Herring]

On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote:
> Rob: please kindly comment on this idea of storing pins/groups/functions
> in DT.

I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a 
one time stuffing of register values. And given how many things run 
before getting to the kernel, doing proper pin configuration in the 
kernel is much too late (or redundant because it was actually already 
done).

> 
> For a sample Linux implementation you can check (incomplete):
> [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT
> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/
> 
> For a real life DT usage you can check:
> [PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions
> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/

What about h/w with no concept of 'groups'?


> Also see below inline comments.
> 
> 
> On 11.12.2021 00:26, Linus Walleij wrote:
> > On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> > 
> > > 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.
> > 
> > So what this does is to take a large chunk of data that we known to be
> > associated with the compatible string (names of pins, groups and functions,
> > etc) and put it into the device tree instead of the alternative, which is
> > what most drivers do, and that is to compile in the data into the
> > operating system and just look it up by using a compatible
> > string.
> 
> Correct. It changes the place of storing platform specific data.
> 
> 
> > The DT maintainers have already indicated that this is not desirable
> > and I don't see it getting merged before it has a Reviewed-by
> > tag from one of the DT binding maintainers.
> 
> Tony pointed out that it was back in 2011. It's worth reconsidering.
> https://patchwork.ozlabs.org/comment/2786915/
> 
> Rob said it depends on whether "data be static (complete) and correct"
> https://patchwork.ozlabs.org/comment/2786688/

I haven't seen an answer for that question...

That and working for multiple platforms (from different vendors) are the 
main things that matter to me. 

> I find it absolutely required to get Rob's Reviewed-by before we merge
> it. I hope we can get Rob's opinion on this.
> 
> 
> > I think we need to know what the USP (unique selling point) is?
> > 
> > Would it be something like not having to duplicate work across some
> > boot loaders and operating systems? (Well they all need to parse this
> > type of description but that can be put into a library.)
> > 
> > Or something else?
> 
> There are two reasons for me to work on this binding:
> 
> 
> 1. I think it's cleaner to keep pinctrl details in DT
> 
> DT seems more natural (than C code) for:
> a) Translating info from datasheets
> b) Storing pin/group/function custom values
> c) Defining relations (phandles)
> d) Handling chip differences (adding new pins in newer revisions)
> 
> Last time I learnt that pins don't always/usually have numbers (in
> datasheets) but are rather named. Still in the "pinctrl_pin_desc" we
> have "unsigned number" just to enumerate them and reference in groups.
> 
> Adding or removing pins/groups/functions in DT is as simple as
> adding/deleting nodes. That also means less logic in C code.
> 
> 
> 2. It avoids data duplication
> 
> It allows me to keep pins/groups/functions data in one place (DT) and
> use it in both: Linux and U-Boot. No duplication & easier maintenance.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rafał Miłecki]

On 14.12.2021 20:59, Rob Herring wrote:
> On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote:
>> Rob: please kindly comment on this idea of storing pins/groups/functions
>> in DT.
> 
> I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a
> one time stuffing of register values. And given how many things run
> before getting to the kernel, doing proper pin configuration in the
> kernel is much too late (or redundant because it was actually already
> done).

OK, thanks for sharing that. Given a pretty limited optimism on this
approach I'll simply drop it and do things the old good way.

I thought it's a better desing but I probably was wrong. It was still
worth a try :)

Thanks to everyone involved in this discussion.


>> For a sample Linux implementation you can check (incomplete):
>> [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT
>> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/
>>
>> For a real life DT usage you can check:
>> [PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions
>> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/
> 
> What about h/w with no concept of 'groups'?

It could probably be handled with sth like

functions {
	bar {
		pins = <&foo>;
	}
}

but my binding didn't cover that indeed.


>> Also see below inline comments.
>>
>>
>> On 11.12.2021 00:26, Linus Walleij wrote:
>>> On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote:
>>>
>>>> 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.
>>>
>>> So what this does is to take a large chunk of data that we known to be
>>> associated with the compatible string (names of pins, groups and functions,
>>> etc) and put it into the device tree instead of the alternative, which is
>>> what most drivers do, and that is to compile in the data into the
>>> operating system and just look it up by using a compatible
>>> string.
>>
>> Correct. It changes the place of storing platform specific data.
>>
>>
>>> The DT maintainers have already indicated that this is not desirable
>>> and I don't see it getting merged before it has a Reviewed-by
>>> tag from one of the DT binding maintainers.
>>
>> Tony pointed out that it was back in 2011. It's worth reconsidering.
>> https://patchwork.ozlabs.org/comment/2786915/
>>
>> Rob said it depends on whether "data be static (complete) and correct"
>> https://patchwork.ozlabs.org/comment/2786688/
> 
> I haven't seen an answer for that question...
> 
> That and working for multiple platforms (from different vendors) are the
> main things that matter to me.

I thought my design description & BCM5301X DTS patch may be a proof of
that but apparently it wasn't enough ;)

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

[Rafał Miłecki]

On 14.12.2021 20:59, Rob Herring wrote:
> On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote:
>> Rob: please kindly comment on this idea of storing pins/groups/functions
>> in DT.
> 
> I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a
> one time stuffing of register values. And given how many things run
> before getting to the kernel, doing proper pin configuration in the
> kernel is much too late (or redundant because it was actually already
> done).

OK, thanks for sharing that. Given a pretty limited optimism on this
approach I'll simply drop it and do things the old good way.

I thought it's a better desing but I probably was wrong. It was still
worth a try :)

Thanks to everyone involved in this discussion.


>> For a sample Linux implementation you can check (incomplete):
>> [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT
>> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/
>>
>> For a real life DT usage you can check:
>> [PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions
>> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/
> 
> What about h/w with no concept of 'groups'?

It could probably be handled with sth like

functions {
	bar {
		pins = <&foo>;
	}
}

but my binding didn't cover that indeed.


>> Also see below inline comments.
>>
>>
>> On 11.12.2021 00:26, Linus Walleij wrote:
>>> On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote:
>>>
>>>> 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.
>>>
>>> So what this does is to take a large chunk of data that we known to be
>>> associated with the compatible string (names of pins, groups and functions,
>>> etc) and put it into the device tree instead of the alternative, which is
>>> what most drivers do, and that is to compile in the data into the
>>> operating system and just look it up by using a compatible
>>> string.
>>
>> Correct. It changes the place of storing platform specific data.
>>
>>
>>> The DT maintainers have already indicated that this is not desirable
>>> and I don't see it getting merged before it has a Reviewed-by
>>> tag from one of the DT binding maintainers.
>>
>> Tony pointed out that it was back in 2011. It's worth reconsidering.
>> https://patchwork.ozlabs.org/comment/2786915/
>>
>> Rob said it depends on whether "data be static (complete) and correct"
>> https://patchwork.ozlabs.org/comment/2786688/
> 
> I haven't seen an answer for that question...
> 
> That and working for multiple platforms (from different vendors) are the
> main things that matter to me.

I thought my design description & BCM5301X DTS patch may be a proof of
that but apparently it wasn't enough ;)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rafał Miłecki]

On 14.12.2021 21:10, Rafał Miłecki wrote:
> On 14.12.2021 20:59, Rob Herring wrote:
>> On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote:
>>> Rob: please kindly comment on this idea of storing pins/groups/functions
>>> in DT.
>>
>> I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a
>> one time stuffing of register values. And given how many things run
>> before getting to the kernel, doing proper pin configuration in the
>> kernel is much too late (or redundant because it was actually already
>> done).
> 
> OK, thanks for sharing that. Given a pretty limited optimism on this
> approach I'll simply drop it and do things the old good way.

I feel I need to post one more comment though.

***

What I find a really clean DT code for defining some BCM4908 groups:

groups {
	led_0_grp {
		pins = <&pin0 3>;
	};

	led_1_grp {
		pins = <&pin1 3>;
	};

	nand_grp {
		pins = <&pin32 0>, <&pin33 0>, <&pin34 0>, <&pin43 0>, <&pin44 0>, <&pin45 0>, <&pin56 1>;
	};
};

***

Gets a bit cumbersome (for me) when using ANSI C structs. I remain
unconvinced about ANSI C being a good place for storing such data.

Maybe I'm just getting too old & grumpy ;)

struct bcm4908_pinctrl_pin_setup {
	unsigned number;
	unsigned function;
};

static const struct bcm4908_pinctrl_pin_setup led_0_pins[] = {
	{ 0, 3 },
};

static const struct bcm4908_pinctrl_pin_setup led_1_pins[] = {
	{ 0, 3 },
};

static const struct bcm4908_pinctrl_pin_setup nand_pins[] = {
	{ 32, 0 }, { 33, 0 }, { 34, 0 }, { 43, 0 }, { 44, 0 }, { 45, 0 }, { 56, 1 },
};

struct bcm4908_pinctrl_grp {
	const char *name;
	const struct bcm4908_pinctrl_pin_setup *pins;
	const unsigned int num_pins;
};

static const struct bcm4908_pinctrl_grp bcm4908_pinctrl_grps[] = {
	{ "led_0_grp", led_0_pins, ARRAY_SIZE(led_0_pins) },
	{ "led_1_grp", led_1_pins, ARRAY_SIZE(led_1_pins) },
	{ "nand_grp", nand_pins, ARRAY_SIZE(nand_pins) },
};

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

[Rafał Miłecki]

On 14.12.2021 21:10, Rafał Miłecki wrote:
> On 14.12.2021 20:59, Rob Herring wrote:
>> On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote:
>>> Rob: please kindly comment on this idea of storing pins/groups/functions
>>> in DT.
>>
>> I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a
>> one time stuffing of register values. And given how many things run
>> before getting to the kernel, doing proper pin configuration in the
>> kernel is much too late (or redundant because it was actually already
>> done).
> 
> OK, thanks for sharing that. Given a pretty limited optimism on this
> approach I'll simply drop it and do things the old good way.

I feel I need to post one more comment though.

***

What I find a really clean DT code for defining some BCM4908 groups:

groups {
	led_0_grp {
		pins = <&pin0 3>;
	};

	led_1_grp {
		pins = <&pin1 3>;
	};

	nand_grp {
		pins = <&pin32 0>, <&pin33 0>, <&pin34 0>, <&pin43 0>, <&pin44 0>, <&pin45 0>, <&pin56 1>;
	};
};

***

Gets a bit cumbersome (for me) when using ANSI C structs. I remain
unconvinced about ANSI C being a good place for storing such data.

Maybe I'm just getting too old & grumpy ;)

struct bcm4908_pinctrl_pin_setup {
	unsigned number;
	unsigned function;
};

static const struct bcm4908_pinctrl_pin_setup led_0_pins[] = {
	{ 0, 3 },
};

static const struct bcm4908_pinctrl_pin_setup led_1_pins[] = {
	{ 0, 3 },
};

static const struct bcm4908_pinctrl_pin_setup nand_pins[] = {
	{ 32, 0 }, { 33, 0 }, { 34, 0 }, { 43, 0 }, { 44, 0 }, { 45, 0 }, { 56, 1 },
};

struct bcm4908_pinctrl_grp {
	const char *name;
	const struct bcm4908_pinctrl_pin_setup *pins;
	const unsigned int num_pins;
};

static const struct bcm4908_pinctrl_grp bcm4908_pinctrl_grps[] = {
	{ "led_0_grp", led_0_pins, ARRAY_SIZE(led_0_pins) },
	{ "led_1_grp", led_1_pins, ARRAY_SIZE(led_1_pins) },
	{ "nand_grp", nand_pins, ARRAY_SIZE(nand_pins) },
};

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel