Re: [PATCH v3 1/2] dt-bindings: pinctrl: mediatek: add support for mt8196

On Thu, Jan 16, 2025 at 4:19 PM Cathy Xu (许华婷) [off-list ref] wrote:

On Thu, 2025-01-16 at 08:28 +0100, Krzysztof Kozlowski wrote:

quoted

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


On 16/01/2025 03:20, Cathy Xu (许华婷) wrote:

quoted

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.

Not much improved.

  I have removed the content related to 'resistance value', we use
'RSEL' instead of 'resistance value'.

So the value in Ohms was removed? I assume above do not have known
value
in Ohms?

  Yes, value in Ohns was removed, no code have knowm value.

That's sad. We went through a lot during the MT8195 cycle to get the
paris driver library to support proper SI unit values [1] to replace
the RSEL values (200 ~ 207). Why can't this be supported anymore?

Also we never got around to getting rid the PUPD/R0/R1 values (100 ~ 103).

quoted

quoted

quoted

quoted

+            description: |
+              For pull down type is normal, it doesn't need
add
RSEL & R1R0.
+              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 PD/RSEL, it can add RSEL
define to set
+              different resistance. 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.

diff --git a/include/dt-bindings/pinctrl/mt8196-pinfunc.h

b/include/dt-bindings/pinctrl/mt8196-pinfunc.h
new file mode 100644
index 000000000000..bf0c8374407c

--- /dev/null
+++ 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)

I do not see how you resolved my comment from v1. In v2 I
reminded
about
it, so you responded that yopu will change something, but I do
not
see
any changes.

So explain: how did you resolve my comment?

These two examples where you claim you will change something, but
send
the same. I skipped the rest of the patch.

  Thank you for your patient response, here is my explanation for
you
question:

  In v1, I undertand that you meant I didn't sent a real binding,
and

The comment is under specific lines, so I said these defines are not
a
real binding. You sent them again, but they are still not bindings,
because they are not used in the driver. Maybe the usage is
convoluted,
so which part of implementation are these connecting with DTS? IOW,
which part of driver relies on the binding?

  I got you. This binding define many macros, which will be used for
'pinmux' setting in the DTS. The usage like this:

  adsp_uart_pins: adsp-uart-pins {
                pins-tx-rx {
                        pinmux = <PINMUX_GPIO35__FUNC_O_ADSP_UTXD0>,
                                 <PINMUX_GPIO36__FUNC_I1_ADSP_URXD0>;
                };
        };

The only binding between the DT and the driver is the structure of
the value, given as "(MTK_PIN_NO(<N>) | <function mux value>)".

The whole list of "PINMUX_GPIOxxx__FUNC_xyz" macros is just a convenience
table for developers, and not used by the driver. The driver simply takes
the values from the two bit fields and uses them directly.

That's why Krzysztof is saying the macros are not used in the driver
and therefore not a binding.

Please move the header file to under "arch/arm64/boot/dts/mediatek",
and split it out as a separate commit with a subject like:

    arm64: dts: mediatek: mt8196: Add pinmux macro header file


ChenYu

quoted

quoted

the bindings should be separated from driver. In addition, I should
run scripts/checkpatch.pl and scripts/get_maintainers.pl. So in v2,
I
sent a real binding(mediatek,mt8196-pinctrl.yaml), and sent two
separate patches, one for driver and one for bindings, also ran
scripts/get_maintainers.pl get necessary people and sent to them.

  In v2, I understand that I need refer to git history to modify
the
commit msgs, so I made the changes in v3. Then you asked me about
the
difference between 'RSEL' and 'resistance value'. I replied that
the
'resistance value' method is no longer use, so in v3, I removed all
content about it(include entire 'rsel-resistance-in-si-unit'
property
and the parts mentioned in bias-pull-up/down).

Yes, thank you this I saw, the comments appear under specific places,
so
only these places are discussed.

  ok, thank you, we can discuss if there are any issues.

quoted

Best regards,
Krzysztof