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

[Sergio Paracuellos]

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.

>
> >
> >>
> >> 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.

>
> >
> >>
> >> If the driver is independent of Ralink architecture code, you're
> >> limiting the driver to be compiled only when a MIPS platform is selected.
> >
> > So... how are you planning to allow compile testing of the driver in
> > any single arch when we get rid of all the arch dependent code? If you
> > make everything dependent on RALINK you cannot.
>
> I intend to make it dependent on OF, not RALINK.

Ok, I see, thanks for clarification.

>
> Arınç

Best regards,
    Sergio Paracuellos