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

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

Hey Sergio,

On 3.03.2023 09:34, Sergio Paracuellos wrote:
> On Fri, Mar 3, 2023 at 7:17 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
>>
>>   Hi Arınç,
>>
>> On Fri, Mar 3, 2023 at 1:30 AM <arinc9.unal@gmail.com> wrote:
>>>
>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>
>>> 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@arinc9.com>
>>> ---
>>>   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.

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.

If the driver is independent of Ralink architecture code, you're 
limiting the driver to be compiled only when a MIPS platform is selected.

Arınç