Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-531-gfdfa13a06d-fm-20230703.001-gfdfa13a0
Mime-Version: 1.0
Message-Id: <9b51bd0a-271a-4830-9422-89ad853a67b6@app.fastmail.com>
In-Reply-To: <886e29ec-a290-993a-d99a-d5fd90662e99@linaro.org>
References: <20230605-ep93xx-v3-0-3d63a5f1103e@maquefel.me>
 <20230605-ep93xx-v3-7-3d63a5f1103e@maquefel.me>
 <0ec81b89-4d4c-4e13-5ea0-697f5c0af835@linaro.org>
 <4073daf401a820c0f64e6fa813b5253bd0427bcd.camel@maquefel.me>
 <886e29ec-a290-993a-d99a-d5fd90662e99@linaro.org>
Date:   Mon, 24 Jul 2023 20:04:06 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>,
        "Nikita Shubin" <nikita.shubin@maquefel.me>,
        "Andy Shevchenko" <andy@kernel.org>,
        "Alexander Sverdlin" <alexander.sverdlin@gmail.com>,
        "Linus Walleij" <linus.walleij@linaro.org>,
        "Michael Peters" <mpeters@embeddedTS.com>,
        "Kris Bahnsen" <kris@embeddedTS.com>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 24, 2023, at 18:31, Krzysztof Kozlowski wrote:
> On 24/07/2023 17:02, Nikita Shubin wrote:
>>>
>>> There is no even need for this, because this code does not use it!
>>=20
>> It's a little emotional, so i can hardly understand the exact reason =
of
>> dissatisfaction.
>>=20
>> Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my fault
>> it's not needed as both PINCTRL and CLK (the only users of this code)
>> can't be built as modules.
>
> I am against any exported symbols and most of functions visible outside
> of this driver.

I think it's a reasonable compromise here, this all ancient code
and we don't need to rewrite all of it as part of the DT conversion,
even if we would do it differently for a new platform.

I really just want to merge the series before Nikita runs out
of motivation to finish the work, after having done almost all
of it.

>>>> +
>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pr_info("EP93xx SoC revi=
sion %s\n", attrs->revision);
>>>> +
>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0;
>>>> +}
>>>> +core_initcall(ep93xx_soc_init);
>>>
>>> That's not the way to add soc driver. You need a proper driver for it
>>=20
>> What is a proper driver for these ?=C2=A0
>
> Usually platform_driver, e.g. exynos-chipid.

Using a platform driver sounds like the right thing to do here,
it's cleaner and should not be hard to do if the driver just matches
the cirrus,ep9301-syscon node. I would have just merged
the v3 version, but this is something that makes sense changing
in v4.

>> Even the latest additions in drivers/soc/* go with *_initcall.
>
> Which one? Ones which call platform_driver_register()? That's quite
> different story, isn't it? I don't see other recent cases, except
> regulator couplers. They might be, some people send and accept poor
> code, so what do you expect from us? Crappy code got in once, so more =
of
> it can go?

That's not what is happening here, this is all part of an incremental
improvement of the existing code. If the DT bindings make sense, I'm
happy to take some shortcuts with the implementation that keep it
closer to the existing implementation and either clean them up over
time or just throw out the platform in five or ten years when the last
machines are dead.

      Arnd