[v6,2/2] dt-bindings: pinctrl: intel: Add for new SoC
diff mbox series

Message ID 96537f8702501a45501d5a59ca029f92e36a9e4a.1573455324.git.rahul.tanwar@linux.intel.com
State Superseded
Headers show
Series
  • pinctrl: Add new pinctrl/GPIO driver
Related show

Commit Message

Tanwar, Rahul Nov. 11, 2019, 10:11 a.m. UTC
Add dt bindings document for pinmux & GPIO controller driver of
Intel Lightning Mountain SoC.

Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
---
 .../bindings/pinctrl/intel,lgm-pinctrl.yaml        | 98 ++++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml

Comments

Rob Herring Nov. 12, 2019, 7:14 p.m. UTC | #1
On Mon, Nov 11, 2019 at 06:11:30PM +0800, Rahul Tanwar wrote:
> Add dt bindings document for pinmux & GPIO controller driver of
> Intel Lightning Mountain SoC.
> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  .../bindings/pinctrl/intel,lgm-pinctrl.yaml        | 98 ++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
> new file mode 100644
> index 000000000000..d54a3bda1f4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: GPL-2.0-only

For new bindings:

# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/pinctrl/intel,lgm-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel Lightning Mountain SoC pinmux & GPIO controller binding
> +
> +maintainers:
> +  - Rahul Tanwar <rahul.tanwar@linux.intel.com>
> +
> +description: |
> +  Pinmux & GPIO controller controls pin multiplexing & configuration including
> +  GPIO function selection & GPIO attributes configuration.
> +
> +  Please refer to [1] for details of the common pinctrl bindings used by the
> +  client devices.
> +
> +  [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +properties:
> +  compatible:
> +    const: intel,lgm-pinctrl
> +
> +  reg:
> +    maxItems: 1
> +
> +# Client device subnode's properties
> +patternProperties:
> +  '-pins$':
> +    type: object
> +    description:
> +      Pinctrl node's client devices use subnodes for desired pin configuration.
> +      Client device subnodes use below standard properties.
> +
> +    properties:
> +      function:
> +        $ref: /schemas/types.yaml#/definitions/string
> +        description:
> +          A string containing the name of the function to mux to the group.

Possible strings should be listed out here.

> +
> +      groups:
> +        $ref: /schemas/types.yaml#/definitions/string-array
> +        description:
> +          An array of strings identifying the list of groups.

Possible strings should be listed out here.

> +
> +      pins:
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        description:
> +          List of pins to select with this function.
> +
> +      pinmux:
> +        description: The applicable mux group.
> +        allOf:
> +          - $ref: "/schemas/types.yaml#/definitions/uint32-array"
> +
> +      bias-pull-up:
> +        type: boolean
> +      bias-pull-down:
> +        type: boolean
> +      drive-strength:
> +        type: boolean

Not a boolean. Need to define possible values.

> +      slew-rate:
> +        type: boolean

Not a boolean. Need to define possible values.

> +      drive-open-drain:
> +        type: boolean
> +      output-enable:
> +        type: boolean
> +
> +    required:
> +      - function
> +      - groups

For the -pins nodes too:

       additionalProperties: false

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  # Pinmux controller node
> +  - |
> +    pinctrl: pinctrl@e2880000 {
> +          compatible = "intel,lgm-pinctrl";
> +          reg = <0xe2880000 0x100000>;
> +
> +          # Client device subnode
> +          uart0-pins: uart0 {

This fails 'make dt_binding_check'. Please fix and run that.

> +                pins = <64>, /* UART_RX0 */
> +                       <65>; /* UART_TX0 */
> +                function = "CONSOLE_UART0";
> +                pinmux = <1>,
> +                         <1>;
> +                groups = "CONSOLE_UART0";
> +          };
> +    };
> +
> +...
> -- 
> 2.11.0
>
Tanwar, Rahul Nov. 13, 2019, 6:05 a.m. UTC | #2
Hi Rob,

Thanks for feedback.

On 13/11/2019 3:14 AM, Rob Herring wrote:
> On Mon, Nov 11, 2019 at 06:11:30PM +0800, Rahul Tanwar wrote:
>> Add dt bindings document for pinmux & GPIO controller driver of
>> Intel Lightning Mountain SoC.
>>
>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
>> ---
>>  .../bindings/pinctrl/intel,lgm-pinctrl.yaml        | 98 ++++++++++++++++++++++
>>  1 file changed, 98 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
>> new file mode 100644
>> index 000000000000..d54a3bda1f4f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
>> @@ -0,0 +1,98 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
> For new bindings:
>
> # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

Well noted.

>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/bindings/pinctrl/intel,lgm-pinctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Intel Lightning Mountain SoC pinmux & GPIO controller binding
>> +
>> +maintainers:
>> +  - Rahul Tanwar <rahul.tanwar@linux.intel.com>
>> +
>> +description: |
>> +  Pinmux & GPIO controller controls pin multiplexing & configuration including
>> +  GPIO function selection & GPIO attributes configuration.
>> +
>> +  Please refer to [1] for details of the common pinctrl bindings used by the
>> +  client devices.
>> +
>> +  [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> +
>> +properties:
>> +  compatible:
>> +    const: intel,lgm-pinctrl
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +# Client device subnode's properties
>> +patternProperties:
>> +  '-pins$':
>> +    type: object
>> +    description:
>> +      Pinctrl node's client devices use subnodes for desired pin configuration.
>> +      Client device subnodes use below standard properties.
>> +
>> +    properties:
>> +      function:
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +        description:
>> +          A string containing the name of the function to mux to the group.
> Possible strings should be listed out here.

Possible number of strings here is a huge number. I agree that it makes
sense to list out the possible strings here but when the possible strings
are huge, can we just omit specifying all of the strings ? I see many
examples here where they only specify the string in examples.

>> +
>> +      groups:
>> +        $ref: /schemas/types.yaml#/definitions/string-array
>> +        description:
>> +          An array of strings identifying the list of groups.
> Possible strings should be listed out here.

Same point for groups. Too many strings to list out here.

>> +
>> +      pins:
>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>> +        description:
>> +          List of pins to select with this function.
>> +
>> +      pinmux:
>> +        description: The applicable mux group.
>> +        allOf:
>> +          - $ref: "/schemas/types.yaml#/definitions/uint32-array"
>> +
>> +      bias-pull-up:
>> +        type: boolean
>> +      bias-pull-down:
>> +        type: boolean
>> +      drive-strength:
>> +        type: boolean
> Not a boolean. Need to define possible values.

Agree. My mistake. Will fix it on v7.

>> +      slew-rate:
>> +        type: boolean
> Not a boolean. Need to define possible values.

In our case, 0 here means slow slew & 1 means fast slew. There are no other
possible values. Probably, i can add it in description while keeping data
type as boolean.

>> +      drive-open-drain:
>> +        type: boolean
>> +      output-enable:
>> +        type: boolean
>> +
>> +    required:
>> +      - function
>> +      - groups
> For the -pins nodes too:
>
>        additionalProperties: false

Well noted.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  # Pinmux controller node
>> +  - |
>> +    pinctrl: pinctrl@e2880000 {
>> +          compatible = "intel,lgm-pinctrl";
>> +          reg = <0xe2880000 0x100000>;
>> +
>> +          # Client device subnode
>> +          uart0-pins: uart0 {
> This fails 'make dt_binding_check'. Please fix and run that.

Will run & fix it in v7. Thanks.

Regards,
Rahul
Linus Walleij Nov. 13, 2019, 2:46 p.m. UTC | #3
On Mon, Nov 11, 2019 at 11:11 AM Rahul Tanwar
<rahul.tanwar@linux.intel.com> wrote:

> Add dt bindings document for pinmux & GPIO controller driver of
> Intel Lightning Mountain SoC.
>
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
(...)

> +properties:
> +  compatible:
> +    const: intel,lgm-pinctrl

Just noted from another review where Rob noted that this name should
match the internal name in the datasheet for this hardware block. Is it
really called "lgm-pinctrl" inside Intel?

intel,lightning-mountain-io and similar are perfectly fine if that is the
name it has in your documentation.

Yours,
Linus Walleij
Tanwar, Rahul Nov. 14, 2019, 3:27 a.m. UTC | #4
Hi Linus,

On 13/11/2019 10:46 PM, Linus Walleij wrote:
> On Mon, Nov 11, 2019 at 11:11 AM Rahul Tanwar
> <rahul.tanwar@linux.intel.com> wrote:
>
>> Add dt bindings document for pinmux & GPIO controller driver of
>> Intel Lightning Mountain SoC.
>>
>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> (...)
>
>> +properties:
>> +  compatible:
>> +    const: intel,lgm-pinctrl
> Just noted from another review where Rob noted that this name should
> match the internal name in the datasheet for this hardware block. Is it
> really called "lgm-pinctrl" inside Intel?
>
> intel,lightning-mountain-io and similar are perfectly fine if that is the
> name it has in your documentation.

Our documentation does not have any specific names for these hardware
blocks. It names it in a very generic/standard manner like GPIO, pinmux..

To make the name explicit & self explanatory, i should probably change
the name as you suggested i.e. intel,lightning-mountain-io.

Regards,
Rahul
Rob Herring Nov. 14, 2019, 5:39 p.m. UTC | #5
On Wed, Nov 13, 2019 at 9:27 PM Tanwar, Rahul
<rahul.tanwar@linux.intel.com> wrote:
>
>
> Hi Linus,
>
> On 13/11/2019 10:46 PM, Linus Walleij wrote:
> > On Mon, Nov 11, 2019 at 11:11 AM Rahul Tanwar
> > <rahul.tanwar@linux.intel.com> wrote:
> >
> >> Add dt bindings document for pinmux & GPIO controller driver of
> >> Intel Lightning Mountain SoC.
> >>
> >> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> > (...)
> >
> >> +properties:
> >> +  compatible:
> >> +    const: intel,lgm-pinctrl
> > Just noted from another review where Rob noted that this name should
> > match the internal name in the datasheet for this hardware block. Is it
> > really called "lgm-pinctrl" inside Intel?
> >
> > intel,lightning-mountain-io and similar are perfectly fine if that is the
> > name it has in your documentation.
>
> Our documentation does not have any specific names for these hardware
> blocks. It names it in a very generic/standard manner like GPIO, pinmux..
>
> To make the name explicit & self explanatory, i should probably change
> the name as you suggested i.e. intel,lightning-mountain-io.

You should also be consistent with 'lgm' vs. 'lightning-mountain' use
across bindings some of which I think have already been accepted.

Rob
Tanwar, Rahul Nov. 15, 2019, 6:01 a.m. UTC | #6
On 15/11/2019 1:39 AM, Rob Herring wrote:
> On Wed, Nov 13, 2019 at 9:27 PM Tanwar, Rahul
> <rahul.tanwar@linux.intel.com> wrote:
>>
>> Hi Linus,
>>
>> On 13/11/2019 10:46 PM, Linus Walleij wrote:
>>> On Mon, Nov 11, 2019 at 11:11 AM Rahul Tanwar
>>> <rahul.tanwar@linux.intel.com> wrote:
>>>
>>>> Add dt bindings document for pinmux & GPIO controller driver of
>>>> Intel Lightning Mountain SoC.
>>>>
>>>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
>>> (...)
>>>
>>>> +properties:
>>>> +  compatible:
>>>> +    const: intel,lgm-pinctrl
>>> Just noted from another review where Rob noted that this name should
>>> match the internal name in the datasheet for this hardware block. Is it
>>> really called "lgm-pinctrl" inside Intel?
>>>
>>> intel,lightning-mountain-io and similar are perfectly fine if that is the
>>> name it has in your documentation.
>> Our documentation does not have any specific names for these hardware
>> blocks. It names it in a very generic/standard manner like GPIO, pinmux..
>>
>> To make the name explicit & self explanatory, i should probably change
>> the name as you suggested i.e. intel,lightning-mountain-io.
> You should also be consistent with 'lgm' vs. 'lightning-mountain' use
> across bindings some of which I think have already been accepted.
>

Yes, other accepted drivers/bindings use 'lgm'. I will rename it to
'intel,lgm-io'to be consistent.Thanks.

Regards,
Rahul

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
new file mode 100644
index 000000000000..d54a3bda1f4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/intel,lgm-pinctrl.yaml
@@ -0,0 +1,98 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bindings/pinctrl/intel,lgm-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Lightning Mountain SoC pinmux & GPIO controller binding
+
+maintainers:
+  - Rahul Tanwar <rahul.tanwar@linux.intel.com>
+
+description: |
+  Pinmux & GPIO controller controls pin multiplexing & configuration including
+  GPIO function selection & GPIO attributes configuration.
+
+  Please refer to [1] for details of the common pinctrl bindings used by the
+  client devices.
+
+  [1] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+
+properties:
+  compatible:
+    const: intel,lgm-pinctrl
+
+  reg:
+    maxItems: 1
+
+# Client device subnode's properties
+patternProperties:
+  '-pins$':
+    type: object
+    description:
+      Pinctrl node's client devices use subnodes for desired pin configuration.
+      Client device subnodes use below standard properties.
+
+    properties:
+      function:
+        $ref: /schemas/types.yaml#/definitions/string
+        description:
+          A string containing the name of the function to mux to the group.
+
+      groups:
+        $ref: /schemas/types.yaml#/definitions/string-array
+        description:
+          An array of strings identifying the list of groups.
+
+      pins:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        description:
+          List of pins to select with this function.
+
+      pinmux:
+        description: The applicable mux group.
+        allOf:
+          - $ref: "/schemas/types.yaml#/definitions/uint32-array"
+
+      bias-pull-up:
+        type: boolean
+      bias-pull-down:
+        type: boolean
+      drive-strength:
+        type: boolean
+      slew-rate:
+        type: boolean
+      drive-open-drain:
+        type: boolean
+      output-enable:
+        type: boolean
+
+    required:
+      - function
+      - groups
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  # Pinmux controller node
+  - |
+    pinctrl: pinctrl@e2880000 {
+          compatible = "intel,lgm-pinctrl";
+          reg = <0xe2880000 0x100000>;
+
+          # Client device subnode
+          uart0-pins: uart0 {
+                pins = <64>, /* UART_RX0 */
+                       <65>; /* UART_TX0 */
+                function = "CONSOLE_UART0";
+                pinmux = <1>,
+                         <1>;
+                groups = "CONSOLE_UART0";
+          };
+    };
+
+...