| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <89416ebd-5da3-7155-94d1-44c7d2019e02@arinc9.com>
Date: Mon, 6 Mar 2023 19:43:27 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Sergio Paracuellos <sergio.paracuellos@...il.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
linux-mediatek@...ts.infradead.org, linux-mips@...r.kernel.org,
linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Matthias Brugger <matthias.bgg@...il.com>,
Sean Wang <sean.wang@...nel.org>,
William Dean <williamsukatube@...il.com>,
Daniel Golle <daniel@...rotopia.org>,
Daniel Santos <daniel.santos@...ox.com>,
Luiz Angelo Daros de Luca <luizluca@...il.com>,
Frank Wunderlich <frank-w@...lic-files.de>,
Landen Chao <Landen.Chao@...iatek.com>,
DENG Qingfang <dqfext@...il.com>,
Sean Wang <sean.wang@...iatek.com>, erkin.bozoglu@...ont.com
Subject: Re: [PATCH 05/20] pinctrl: ralink: move to mediatek as mtmips
On 6.03.2023 19:15, Sergio Paracuellos wrote:
> On Mon, Mar 6, 2023 at 4:06 PM Arınç ÜNAL <arinc.unal@...nc9.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@...nc9.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@...nc9.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@...il.com> wrote:
>>>>>>>>
>>>>>>>> Hi Arınç,
>>>>>>>>
>>>>>>>> On Fri, Mar 3, 2023 at 1:30 AM <arinc9.unal@...il.com> wrote:
>>>>>>>>>
>>>>>>>>> From: Arınç ÜNAL <arinc.unal@...nc9.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@...nc9.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ç
Powered by blists - more mailing lists