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

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

On 6.03.2023 19:15, Sergio Paracuellos wrote:
> On Mon, Mar 6, 2023 at 4:06 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> On 6.03.2023 17:07, Sergio Paracuellos wrote:
>>> On Fri, Mar 3, 2023 at 3:18 PM Arınç ÜNAL <arinc.unal@arinc9.com> 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@arinc9.com> 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@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.
>>>>>
>>>>> 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ç