lkml.org 
[lkml]   [2023]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 05/20] pinctrl: ralink: move to mediatek as mtmips
    From
    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.

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

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

    Arınç

    \
     
     \ /
      Last update: 2023-03-27 00:43    [W:2.713 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site