On Mon, 2025-01-13 at 14:20 +0100, AngeloGioacchino Del Regno wrote:

External email : Please do not click links or open attachments until
you have verified the sender or the content.


Il 11/01/25 10:41, Krzysztof Kozlowski ha scritto:

quoted

On Fri, Jan 10, 2025 at 06:42:28PM +0800, Cathy Xu wrote:

quoted

1.Add pinctrl file on MediaTek mt8196.

Where? What is pinctrl file?

quoted

2.Add the new binding document for pinctrl on MediaTek mt8196.

Look at git history how commit msgs are written for such changes.

quoted

Signed-off-by: Guodong Liu <redacted>
Signed-off-by: Cathy Xu <redacted>
---
  .../pinctrl/mediatek,mt8196-pinctrl.yaml      |  266 +++
  include/dt-bindings/pinctrl/mt8196-pinfunc.h  | 1572
+++++++++++++++++
  2 files changed, 1838 insertions(+)
  create mode 100644
Documentation/devicetree/bindings/pinctrl/mediatek,mt8196-
pinctrl.yaml
  create mode 100644 include/dt-bindings/pinctrl/mt8196-pinfunc.h

diff --git
a/Documentation/devicetree/bindings/pinctrl/mediatek,mt8196-
pinctrl.yaml
b/Documentation/devicetree/bindings/pinctrl/mediatek,mt8196-
pinctrl.yaml
new file mode 100644
index 000000000000..abeb0d942cc4

--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt8196-

pinctrl.yaml

@@ -0,0 +1,266 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: 

https://urldefense.com/v3/__http://devicetree.org/schemas/pinctrl/mediatek,mt8196-pinctrl.yaml*__;Iw!!CTRNKA9wMg0ARbw!laNiB1eOycilYcAp5iBZ1fOGQrXEg2lr8Rkt727TE54e_9DlPoANkwssDGD16Q1Jif0EdQ8vaAVHNG1MMMi9U5rB9oYBgyMDZA$
+$schema: 
https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!laNiB1eOycilYcAp5iBZ1fOGQrXEg2lr8Rkt727TE54e_9DlPoANkwssDGD16Q1Jif0EdQ8vaAVHNG1MMMi9U5rB9oZoUiw09w$
+
+title: MediaTek MT8196 Pin Controller
+
+maintainers:
+  - Lei Xue [off-list ref]
+  - Cathy Xu [off-list ref]
+
+description:
+  The MediaTek's MT8196 Pin controller is used to control SoC
pins.
+
+properties:
+  compatible:
+    const: mediatek,mt8196-pinctrl
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    description:
+      Number of cells in GPIO specifier, should be two. The
first cell is the
+      pin number, the second cell is used to specify optional
parameters which
+      are defined in <dt-bindings/gpio/gpio.h>.
+    const: 2
+
+  gpio-ranges:
+    maxItems: 1
+
+  gpio-line-names: true
+
+  reg:
+    items:
+      - description: gpio registers base address
+      - description: rt group io configuration registers base
address
+      - description: rm1 group io configuration registers base
address
+      - description: rm2 group io configuration registers base
address
+      - description: rb group io configuration registers base
address
+      - description: bm1 group io configuration registers base
address
+      - description: bm2 group io configuration registers base
address
+      - description: bm3 group io configuration registers base
address
+      - description: lt group io configuration registers base
address
+      - description: lm1 group io configuration registers base
address
+      - description: lm2 group io configuration registers base
address
+      - description: lb1 group io configuration registers base
address
+      - description: lb2 group io configuration registers base
address
+      - description: tm1 group io configuration registers base
address
+      - description: tm2 group io configuration registers base
address
+      - description: tm3 group io configuration registers base
address
+
+  reg-names:
+    items:
+      - const: iocfg0
+      - const: iocfg_rt
+      - const: iocfg_rm1
+      - const: iocfg_rm2
+      - const: iocfg_rb
+      - const: iocfg_bm1
+      - const: iocfg_bm2
+      - const: iocfg_bm3
+      - const: iocfg_lt
+      - const: iocfg_lm1
+      - const: iocfg_lm2
+      - const: iocfg_lb1
+      - const: iocfg_lb2
+      - const: iocfg_tm1
+      - const: iocfg_tm2
+      - const: iocfg_tm3

Are you sure these are separate address spaces?

Those are different partitions of the GPIO controller, of which, each
one does
provide full control and different functions that are partition-
global and not
controller-global.

So, I can confirm that these are indeed separate address spaces -
this is right.

  Thank you for helping me respond!

quoted

quoted

+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupts:
+    description: The interrupt outputs to sysirq.
+    maxItems: 1
+
+  mediatek,rsel-resistance-in-si-unit:
+    type: boolean
+    description:
+      We provide two methods to select the resistance for I2C
when pull up or
+      pull down. The first is by RSEL definition value, another
one is by
+      resistance value(ohm). This flag is used to identify if
the method is
+      resistance(si unit) value.

What is the point of choosing it? This is one hardware, one SoC, so
how
different boards can have different units? No, just use Ohms

That's legacy from when the paris driver didn't specify the RSEL in
ohms, and I
agree about deprecating that property and making it a default.

Cathy, please add a `has_legacy_rsel` boolean in the platform data
and assign that
to true in *all* MediaTek pinctrl drivers that are not MT8196, then
in
pinctrl-paris:

if (hw->soc->has_legacy_rsel)
        hw->rsel_si_unit = of_property_read_bool(....)
else
        hw->rsel_si_unit = true;

That way we can finally drop this property from drivers for new SoCs,
including
the one for MT8196.

  'mediatek,rsel-resistance-in-si-unit' won't be used now, and will
remove it in next version.

quoted

quoted

+
+# PIN CONFIGURATION NODES
+patternProperties:
+  '-pins$':
+    type: object
+    additionalProperties: false
+
+    patternProperties:
+      '^pins':
+        type: object
+        $ref: /schemas/pinctrl/pincfg-node.yaml
+        additionalProperties: false
+        description:
+          A pinctrl node should contain at least one subnode
representing the
+          pinctrl groups available on the machine. Each subnode
will list the
+          pins it needs, and how they should be configured, with
regard to muxer
+          configuration, pullups, drive strength, input
enable/disable and input
+          schmitt.
+
+        properties:
+          pinmux:
+            description:
+              Integer array, represents gpio pin number and mux
setting.
+              Supported pin number and mux varies for different
SoCs, and are
+              defined as macros in dt-bindings/pinctrl/mt8196-
pinfunc.h
+              directly, for this SoC.
+
+          drive-strength:
+            enum: [2, 4, 6, 8, 10, 12, 14, 16]
+
+          drive-strength-microamp:
+            enum: [125, 250, 500, 1000]

Why duplicating properties? No, use only one.

The problem here is not entirely about duplicating properties, and
I'm not
sure that the reason is actually acceptable (but being this a special
case
the `description` field would be mandatory to have IMO!!).

So, the reason for this separation is that the drive-strength-
microamp does
activate a special feature in the controller called "advanced drive
strength
mode", which is switching to different shunts that will decrease the
power
efficiency of the chip (by an ignorable amount, if that's one pin -
but if
that goes to something like 100 pins, it's not ignorable anymore).

I'd be happy if we could let them retain both properties after
putting a
clear description of what's happening and why there are two of them.

  drive-strength-microamp is for i2c special current, and drive-
strength is for other pins current.

quoted

quoted

+
+          bias-pull-down:
+            oneOf:
+              - type: boolean
+              - enum: [100, 101, 102, 103]
+                description: mt8196 pull down PUPD/R0/R1 type
define value.
+              - enum: [200, 201, 202, 203, 204, 205, 206, 207]
+                description: mt8196 pull down RSEL type define
value.

These two must go away, RSEL shall be defined in Ohms unit in
DTs/bindings.

  It will be removed in next version.

quoted

quoted

+              - enum: [75000, 5000]
+                description: mt8196 pull down RSEL type si unit
value(ohm).
+            description: |
+              For pull down type is normal, it doesn't need add
RSEL & R1R0
+              define and resistance value.
+              For pull down type is PUPD/R0/R1 type, it can add
R1R0 define to
+              set different resistance. It can support
"MTK_PUPD_SET_R1R0_00" &
+              "MTK_PUPD_SET_R1R0_01" & "MTK_PUPD_SET_R1R0_10" &
+              "MTK_PUPD_SET_R1R0_11" define in mt8196.
+              For pull down type is RSEL, it can add RSEL define
& resistance
+              value(ohm) to set different resistance by
identifying property
+              "mediatek,rsel-resistance-in-si-unit". It can
support
+              "MTK_PULL_SET_RSEL_000" & "MTK_PULL_SET_RSEL_001"
&
+              "MTK_PULL_SET_RSEL_010" & "MTK_PULL_SET_RSEL_011"
&
+              "MTK_PULL_SET_RSEL_100" & "MTK_PULL_SET_RSEL_101"
&
+              "MTK_PULL_SET_RSEL_110" & "MTK_PULL_SET_RSEL_111"
define in
+              mt8196. It can also support resistance value(ohm)
"75000" & "5000"
+              in mt8196.
+
+          bias-pull-up:
+            oneOf:
+              - type: boolean
+              - enum: [100, 101, 102, 103]
+                description: mt8196 pull up PUPD/R0/R1 type
define value.
+              - enum: [200, 201, 202, 203, 204, 205, 206, 207]
+                description: mt8196 pull up RSEL type define
value.
+              - enum: [1000, 1500, 2000, 3000, 4000, 5000,
10000, 75000]
+                description: mt8196 pull up RSEL type si unit
value(ohm).

Same problems.

quoted

+++ b/include/dt-bindings/pinctrl/mt8196-pinfunc.h

@@ -0,0 +1,1572 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Copyright (C) 2025 Mediatek Inc.
+ * Author: Guodong Liu <Guodong.Liu@mediatek.com>
+ */
+
+#ifndef __MT8196_PINFUNC_H
+#define __MT8196_PINFUNC_H
+
+#include <dt-bindings/pinctrl/mt65xx.h>
+
+#define PINMUX_GPIO0__FUNC_GPIO0 (MTK_PIN_NO(0) | 0)
+#define PINMUX_GPIO0__FUNC_DMIC1_CLK (MTK_PIN_NO(0) | 1)
+#define PINMUX_GPIO0__FUNC_SPI3_A_MO (MTK_PIN_NO(0) | 3)
+#define PINMUX_GPIO0__FUNC_FMI2S_B_LRCK (MTK_PIN_NO(0) | 4)
+#define PINMUX_GPIO0__FUNC_SCP_DMIC1_CLK (MTK_PIN_NO(0) | 5)
+#define PINMUX_GPIO0__FUNC_TP_GPIO14_AO (MTK_PIN_NO(0) | 6)

You got comment, so respond to it. Sending the same and expecting
different results is fast way to get a grumpy response.

...and I agree about every other comment from Krzysztof.

  Ok, thank you for your review.

Cheers,
Angelo