[v5,6/7] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Document MT6735 pin controller

Message ID 20221118113028.145348-7-y.oudjana@protonmail.com
State New
Headers
Series MediaTek pinctrl DT binding cleanup and MT6735 pinctrl support |

Commit Message

Yassine Oudjana Nov. 18, 2022, 11:30 a.m. UTC
  From: Yassine Oudjana <y.oudjana@protonmail.com>

Add bindings for the pin controller found on MediaTek MT6735 and
MT6735M SoCs, including describing a method to manually specify
a pin and function in the pinmux property making defining bindings
for each pin/function combination unnecessary. The pin controllers
on those SoCs are generally identical, with the only difference
being the lack of MSDC2 pins (198-203) on MT6735M.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../pinctrl/mediatek,mt6779-pinctrl.yaml      | 55 ++++++++++++++++++-
 MAINTAINERS                                   |  6 ++
 2 files changed, 60 insertions(+), 1 deletion(-)
  

Comments

AngeloGioacchino Del Regno Nov. 21, 2022, 1:48 p.m. UTC | #1
Il 18/11/22 12:30, Yassine Oudjana ha scritto:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> Add bindings for the pin controller found on MediaTek MT6735 and
> MT6735M SoCs, including describing a method to manually specify
> a pin and function in the pinmux property making defining bindings
> for each pin/function combination unnecessary. The pin controllers
> on those SoCs are generally identical, with the only difference
> being the lack of MSDC2 pins (198-203) on MT6735M.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>   .../pinctrl/mediatek,mt6779-pinctrl.yaml      | 55 ++++++++++++++++++-
>   MAINTAINERS                                   |  6 ++
>   2 files changed, 60 insertions(+), 1 deletion(-)
> 

..snip..

> @@ -352,18 +391,32 @@ examples:
>               };
>   
>               /* GPIO0 set as multifunction GPIO0 */
> -            gpio-pins {
> +            gpio0-pins {
>                   pins {
>                       pinmux = <PINMUX_GPIO0__FUNC_GPIO0>;
>                   };
>               };
>   
> +            /* GPIO1 set to function 0 (GPIO) */
> +            gpio1-pins {
> +                pins {
> +                    pinmux = <(MTK_PIN_NO(1) | 0)>;

Please follow the same format that you can find in all of the
mtXXXX-pinfunc.h.

What you wrote here (MTK_PIN_NO(x) | func) is defined in there for the purpose
of providing a definition name that actually means something (for both readability
and documentation purposes).

This means that your GPIO1 set to function 0 (gpio) should be

			pinmux = <PINMUX_GPIO1__FUNC_GPIO1>;

> +                };
> +            };
> +
>               /* GPIO52 set as multifunction SDA0 */
>               i2c0-pins {
>                   pins {
>                     pinmux = <PINMUX_GPIO52__FUNC_SDA0>;
>                   };
>               };
> +
> +            /* GPIO62 set to function 1 (primary function) */
> +            i2c1-pins {
> +                pins {
> +                    pinmux = <(MTK_PIN_NO(62) | 1)>;

pinmux = <PINMUX_GPIO62__FUNC_SDA1>; (is it sda1??)

This means that you should as well add a mediatek,mt6735-pinfunc.h binding...

Regards,
Angelo
  
Yassine Oudjana Nov. 21, 2022, 4:01 p.m. UTC | #2
On Mon, Nov 21 2022 at 14:48:58 +01:00:00, AngeloGioacchino Del Regno 
<angelogioacchino.delregno@collabora.com> wrote:
> Il 18/11/22 12:30, Yassine Oudjana ha scritto:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>> Add bindings for the pin controller found on MediaTek MT6735 and
>> MT6735M SoCs, including describing a method to manually specify
>> a pin and function in the pinmux property making defining bindings
>> for each pin/function combination unnecessary. The pin controllers
>> on those SoCs are generally identical, with the only difference
>> being the lack of MSDC2 pins (198-203) on MT6735M.
>> 
>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>   .../pinctrl/mediatek,mt6779-pinctrl.yaml      | 55 
>> ++++++++++++++++++-
>>   MAINTAINERS                                   |  6 ++
>>   2 files changed, 60 insertions(+), 1 deletion(-)
>> 
> 
> ..snip..
> 
>> @@ -352,18 +391,32 @@ examples:
>>               };
>>                 /* GPIO0 set as multifunction GPIO0 */
>> -            gpio-pins {
>> +            gpio0-pins {
>>                   pins {
>>                       pinmux = <PINMUX_GPIO0__FUNC_GPIO0>;
>>                   };
>>               };
>>   +            /* GPIO1 set to function 0 (GPIO) */
>> +            gpio1-pins {
>> +                pins {
>> +                    pinmux = <(MTK_PIN_NO(1) | 0)>;
> 
> Please follow the same format that you can find in all of the
> mtXXXX-pinfunc.h.
> 
> What you wrote here (MTK_PIN_NO(x) | func) is defined in there for 
> the purpose
> of providing a definition name that actually means something (for 
> both readability
> and documentation purposes).
> 
> This means that your GPIO1 set to function 0 (gpio) should be
> 
> 			pinmux = <PINMUX_GPIO1__FUNC_GPIO1>;
> 
>> +                };
>> +            };
>> +
>>               /* GPIO52 set as multifunction SDA0 */
>>               i2c0-pins {
>>                   pins {
>>                     pinmux = <PINMUX_GPIO52__FUNC_SDA0>;
>>                   };
>>               };
>> +
>> +            /* GPIO62 set to function 1 (primary function) */
>> +            i2c1-pins {
>> +                pins {
>> +                    pinmux = <(MTK_PIN_NO(62) | 1)>;
> 
> pinmux = <PINMUX_GPIO62__FUNC_SDA1>; (is it sda1??)
> 
> This means that you should as well add a mediatek,mt6735-pinfunc.h 
> binding...

This is pretty much what I was trying to avoid by doing this. 
Originally I tried to have something similar to qualcomm pin 
controllers which use "pins" and "function" properties (but probably 
with integer values rather than strings) without making any major 
changes to pinctrl-paris or related DT bindings, but it quickly became 
obvious that it wouldn't be possible when looking at how it does things 
currently. pinctrl-moore was better in this aspect, actually making use 
of pin groups to describe how sets of pins have shared functions 
instead of making a group for each pin, and taking "groups" and 
"function" properties. However, it wasn't fully suitable for the 
hardware so I had to stick with pinctrl-paris. At that point I thought 
of this to be the simplest way of doing it. I think it is unnecessary 
to define every single pin-function combination. Yes, doing it this way 
doesn't make it clear what function is being set right away, but a 
quick look at pinctrl-mtk-mt6735.h is all it takes to find out. 
Furthermore, in most cases functions 0 (GPIO) and 1 (primary, pin named 
after it) are the only ones used so knowing the pin names is all it 
takes to figure out the functions.

With all of that being said however, I guess I don't mind following the 
current convention for the time being. The pinctrl subsystem (not just 
mediatek pin controllers) has some of the most inconsistent DT bindings 
from what I've seen, specifically when it comes to specifying pin 
functions, and I think it will end up having some major cleanup down 
the line anyway.
  

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
index c2238a86b7fe..4fe95b0107ba 100644
--- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
@@ -10,6 +10,7 @@  maintainers:
   - Andy Teng <andy.teng@mediatek.com>
   - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
   - Sean Wang <sean.wang@kernel.org>
+  - Yassine Oudjana <y.oudjana@protonmail.com>
 
 description:
   The MediaTek pin controller on MT6779 is used to control pin
@@ -18,6 +19,8 @@  description:
 properties:
   compatible:
     enum:
+      - mediatek,mt6735-pinctrl
+      - mediatek,mt6735m-pinctrl
       - mediatek,mt6765-pinctrl
       - mediatek,mt6779-pinctrl
       - mediatek,mt6795-pinctrl
@@ -60,6 +63,42 @@  required:
 
 allOf:
   - $ref: "pinctrl.yaml#"
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt6735-pinctrl
+              - mediatek,mt6735m-pinctrl
+    then:
+      properties:
+        reg:
+          minItems: 8
+          maxItems: 8
+
+        reg-names:
+          items:
+            - const: gpio
+            - const: iocfg0
+            - const: iocfg1
+            - const: iocfg2
+            - const: iocfg3
+            - const: iocfg4
+            - const: iocfg5
+            - const: eint
+
+        interrupts:
+          items:
+            - description: EINT interrupt
+
+      patternProperties:
+        '-pins$':
+          patternProperties:
+            '^pins':
+              properties:
+                drive-strength:
+                  enum: [1, 2, 4, 8, 16]
+
   - if:
       properties:
         compatible:
@@ -352,18 +391,32 @@  examples:
             };
 
             /* GPIO0 set as multifunction GPIO0 */
-            gpio-pins {
+            gpio0-pins {
                 pins {
                     pinmux = <PINMUX_GPIO0__FUNC_GPIO0>;
                 };
             };
 
+            /* GPIO1 set to function 0 (GPIO) */
+            gpio1-pins {
+                pins {
+                    pinmux = <(MTK_PIN_NO(1) | 0)>;
+                };
+            };
+
             /* GPIO52 set as multifunction SDA0 */
             i2c0-pins {
                 pins {
                   pinmux = <PINMUX_GPIO52__FUNC_SDA0>;
                 };
             };
+
+            /* GPIO62 set to function 1 (primary function) */
+            i2c1-pins {
+                pins {
+                    pinmux = <(MTK_PIN_NO(62) | 1)>;
+                };
+            };
         };
 
         mmc0 {
diff --git a/MAINTAINERS b/MAINTAINERS
index 1df62c469bd9..f9223c6cd5a8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16462,6 +16462,12 @@  F:	Documentation/devicetree/bindings/pinctrl/mediatek,mt7622-pinctrl.yaml
 F:	Documentation/devicetree/bindings/pinctrl/mediatek,mt8183-pinctrl.yaml
 F:	drivers/pinctrl/mediatek/
 
+PIN CONTROLLER - MEDIATEK MT6735
+M:	Yassine Oudjana <y.oudjana@protonmail.com>
+L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
+
 PIN CONTROLLER - MICROCHIP AT91
 M:	Ludovic Desroches <ludovic.desroches@microchip.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)