Re: [PATCH 05/20] pinctrl: ralink: move to mediatek as mtmips

From: Arınç ÜNAL
Date: Mon Mar 06 2023 - 12:01:39 EST

Next message: Lux Aliaga: "[PATCH v7 2/6] dt-bindings: phy: Add QMP UFS PHY compatible for SM6125"
Previous message: Lux Aliaga: "[PATCH v7 3/6] phy: qcom-qmp: Add SM6125 UFS PHY support"
In reply to: Sergio Paracuellos: "Re: [PATCH 05/20] pinctrl: ralink: move to mediatek as mtmips"
Next in thread: arinc9 . unal: "[PATCH 06/20] pinctrl: mediatek: remove OF_GPIO as reverse dependency"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 6.03.2023 19:15, Sergio Paracuellos wrote:

On Mon, Mar 6, 2023 at 4:06 PM Arınç ÜNAL <arinc.unal@xxxxxxxxxx> wrote:

On 6.03.2023 17:07, Sergio Paracuellos wrote:

On Fri, Mar 3, 2023 at 3:18 PM Arınç ÜNAL <arinc.unal@xxxxxxxxxx> wrote:

Heyo,

On 3.03.2023 13:57, Sergio Paracuellos wrote:

Hi Arınç,

On Fri, Mar 3, 2023 at 9:16 AM Arınç ÜNAL <arinc.unal@xxxxxxxxxx> wrote:

Hey Sergio,

On 3.03.2023 09:34, Sergio Paracuellos wrote:

On Fri, Mar 3, 2023 at 7:17 AM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:

Hi Arınç,

On Fri, Mar 3, 2023 at 1:30 AM <arinc9.unal@xxxxxxxxx> wrote:

From: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>

This platform from Ralink was acquired by MediaTek in 2011. Then, MediaTek
introduced new SoCs which utilise this platform. Move the driver to
mediatek pinctrl directory. Rename the ralink core driver to mtmips.

Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
---
drivers/pinctrl/Kconfig | 1 -
drivers/pinctrl/Makefile | 1 -
drivers/pinctrl/mediatek/Kconfig | 51 ++++++++++-
drivers/pinctrl/mediatek/Makefile | 63 +++++++------
.../{ralink => mediatek}/pinctrl-mt7620.c | 34 +++----
.../{ralink => mediatek}/pinctrl-mt7621.c | 30 +++----
.../{ralink => mediatek}/pinctrl-mt76x8.c | 60 ++++++-------
.../pinctrl-mtmips.c} | 90 +++++++++----------
.../pinctrl-mtmips.h} | 16 ++--
.../{ralink => mediatek}/pinctrl-rt2880.c | 20 ++---
.../{ralink => mediatek}/pinctrl-rt305x.c | 44 ++++-----
.../{ralink => mediatek}/pinctrl-rt3883.c | 28 +++---
drivers/pinctrl/ralink/Kconfig | 40 ---------
drivers/pinctrl/ralink/Makefile | 9 --
14 files changed, 246 insertions(+), 241 deletions(-)
rename drivers/pinctrl/{ralink => mediatek}/pinctrl-mt7620.c (81%)
rename drivers/pinctrl/{ralink => mediatek}/pinctrl-mt7621.c (80%)
rename drivers/pinctrl/{ralink => mediatek}/pinctrl-mt76x8.c (81%)
rename drivers/pinctrl/{ralink/pinctrl-ralink.c => mediatek/pinctrl-mtmips.c} (74%)
rename drivers/pinctrl/{ralink/pinctrl-ralink.h => mediatek/pinctrl-mtmips.h} (75%)
rename drivers/pinctrl/{ralink => mediatek}/pinctrl-rt2880.c (71%)
rename drivers/pinctrl/{ralink => mediatek}/pinctrl-rt305x.c (75%)
rename drivers/pinctrl/{ralink => mediatek}/pinctrl-rt3883.c (80%)
delete mode 100644 drivers/pinctrl/ralink/Kconfig
delete mode 100644 drivers/pinctrl/ralink/Makefile

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index dcb53c4a9584..8a6012770640 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -537,7 +537,6 @@ source "drivers/pinctrl/nomadik/Kconfig"
source "drivers/pinctrl/nuvoton/Kconfig"
source "drivers/pinctrl/pxa/Kconfig"
source "drivers/pinctrl/qcom/Kconfig"
-source "drivers/pinctrl/ralink/Kconfig"
source "drivers/pinctrl/renesas/Kconfig"
source "drivers/pinctrl/samsung/Kconfig"
source "drivers/pinctrl/spear/Kconfig"
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index d5939840bb2a..ada6ed1d4e91 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -66,7 +66,6 @@ obj-y += nomadik/
obj-y += nuvoton/
obj-$(CONFIG_PINCTRL_PXA) += pxa/
obj-$(CONFIG_ARCH_QCOM) += qcom/
-obj-$(CONFIG_PINCTRL_RALINK) += ralink/
obj-$(CONFIG_PINCTRL_RENESAS) += renesas/
obj-$(CONFIG_PINCTRL_SAMSUNG) += samsung/
obj-$(CONFIG_PINCTRL_SPEAR) += spear/
diff --git a/drivers/pinctrl/mediatek/Kconfig b/drivers/pinctrl/mediatek/Kconfig
index a71874fed3d6..2eeb55010563 100644
--- a/drivers/pinctrl/mediatek/Kconfig
+++ b/drivers/pinctrl/mediatek/Kconfig
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
menu "MediaTek pinctrl drivers"
- depends on ARCH_MEDIATEK || COMPILE_TEST
+ depends on ARCH_MEDIATEK || RALINK || COMPILE_TEST

config EINT_MTK
tristate "MediaTek External Interrupt Support"
@@ -22,6 +22,12 @@ config PINCTRL_MTK
config PINCTRL_MTK_V2
tristate

+config PINCTRL_MTK_MTMIPS
+ bool
+ depends on RALINK
+ select PINMUX
+ select GENERIC_PINCONF
+
config PINCTRL_MTK_MOORE
bool
depends on OF
@@ -43,6 +49,49 @@ config PINCTRL_MTK_PARIS
select OF_GPIO
select PINCTRL_MTK_V2

+# For MIPS SoCs
+config PINCTRL_MT7620
+ bool "MediaTek MT7620 pin control"
+ depends on SOC_MT7620 || COMPILE_TEST
+ depends on RALINK
+ default SOC_MT7620
+ select PINCTRL_MTK_MTMIPS
+
+config PINCTRL_MT7621
+ bool "MediaTek MT7621 pin control"
+ depends on SOC_MT7621 || COMPILE_TEST
+ depends on RALINK
+ default SOC_MT7621
+ select PINCTRL_MTK_MTMIPS
+
+config PINCTRL_MT76X8
+ bool "MediaTek MT76X8 pin control"
+ depends on SOC_MT7620 || COMPILE_TEST
+ depends on RALINK
+ default SOC_MT7620
+ select PINCTRL_MTK_MTMIPS
+
+config PINCTRL_RT2880
+ bool "Ralink RT2880 pin control"
+ depends on SOC_RT288X || COMPILE_TEST
+ depends on RALINK
+ default SOC_RT288X
+ select PINCTRL_MTK_MTMIPS
+
+config PINCTRL_RT305X
+ bool "Ralink RT305X pin control"
+ depends on SOC_RT305X || COMPILE_TEST
+ depends on RALINK
+ default SOC_RT305X
+ select PINCTRL_MTK_MTMIPS
+
+config PINCTRL_RT3883
+ bool "Ralink RT3883 pin control"
+ depends on SOC_RT3883 || COMPILE_TEST
+ depends on RALINK
+ default SOC_RT3883
+ select PINCTRL_MTK_MTMIPS
+

I am not a Kconfig expert at all but...

Should not all of these be depends on SOC_XXX || (COMPILE_TEST &&
RALINK) and avoid the " depends on RALINK" next line in all of them?

This seems to do the same thing but I'm following the "either change
them all or fit into the crowd" ideology.

Just asking since we have yet arch read and write register operations
in pinctrl common ralink code. Having in this way, when we address
this arch thing in the next series just removing the "&& RALINK" part
makes the review pretty obvious.

You'd have to change RALINK with OF since we're still depending on that.
RALINK selects OF by default so it's currently a hidden dependency.

Other than that, changes look good to me.

I think "depends on SOC_XXX || (COMPILE_TEST && MIPS)" would work also
and might be more accurate for compile testing targets.

Are you sure? SOC_XXX here is already being enabled only if RALINK is
already enabled, right? [0]

I'm not sure who's your reply to, or what it's about here.

Bad insertion between lines, sorry :). I was just trying to explain to
you that SOC_RTXX ralink stuff is only available when RALINK is
already selected.

Makes sense. However, I believe what I said below is still true. This
option will be available to compile if a Ralink SoC (and therefore
RALINK) is enabled, OR, COMPILE_TEST and MIPS is enabled. The latter
will fail to compile if the enabled MIPS platform is not RALINK.

This is not OK in both cases. If the driver is dependent on Ralink
architecture code, choosing any other MIPS platform will make the driver
available to compile, which will fail.

SOC_XXX is already dependent on RALINK for real uses but the driver is
going to be selected for other MIPS platforms only for COMPILE_TEST
targets. Ideally drivers should be arch agnostic so can be selected
for any single arch build. Now we have arch dependent read and write
calls in the code, so you need the right headers to be properly found
to be able to compile testing. I think MIPS is enough dependency here
to properly find them. But if not, this should be (COMPILE_TEST &&
RALINK)

I expect below to work without requiring the MIPS option.

ifeq ($(CONFIG_COMPILE_TEST),y)
CFLAGS_pinctrl-mtmips.o += -I$(srctree)/arch/mips/include
endif

Yes, this will work but won't be necessary at all when we get rid of
ralink arch dependent code in the next series.

Oh, you plan to completely get rid of it, including headers. That's better!

I'd really love to get rid of all of that, yes.

However, rt305x_pinctrl_probe() on pinctrl-rt305x.c needs them to find
out the SoC to match the pinmux data. Sure, splitting the driver further
will work but I'm wondering if you've got something else in mind to
address this.

I know. Sharing the same compatible string makes really hard to do
this easily. One of my thoughts was to split also that in the driver
as you are pointing out here. I have also submitted this series [0] to
be able to make use of soc_match stuff instead of relying on
compatible strings for these kinds of situations. However I am not
also sure that would be a valid approach. Let's see. At the end we can
end up splitting the driver if nothing seems to work.

Sounds good, I appreciate your efforts.

Arınç

Next message: Lux Aliaga: "[PATCH v7 2/6] dt-bindings: phy: Add QMP UFS PHY compatible for SM6125"
Previous message: Lux Aliaga: "[PATCH v7 3/6] phy: qcom-qmp: Add SM6125 UFS PHY support"
In reply to: Sergio Paracuellos: "Re: [PATCH 05/20] pinctrl: ralink: move to mediatek as mtmips"
Next in thread: arinc9 . unal: "[PATCH 06/20] pinctrl: mediatek: remove OF_GPIO as reverse dependency"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]