Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Nikita Shubin]

Hello Arnd, Krzysztof !

On Mon, 2023-07-24 at 20:04 +0200, Arnd Bergmann wrote:
> 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!
> > > 
> > > It's a little emotional, so i can hardly understand the exact
> > > reason of
> > > dissatisfaction.
> > > 
> > > 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.

Thank you for your kind words, i think i have steam for at least for a
couple more versions.

I agree with Krzysztof, through i don't see any good options to dial
with "swlocked" registers (these means we have to write a magic value
to some register just before writing to "swlocked" register) without
exposing some functions - the only variants i can think of:

- introduce a "fake" hwlock, so the drivers will provide magic
themselves while holding the lock
- convert SYSCON, CLK, PINCTRL into "Auxiliary" (suggested by Stephen)
but this introduces more problems:
  - as AFAIK CLK can use SYSCON node - but i am not sure about PINCTRL
  - it still will have a common header for CLK and PINCTRL (no
functions - just structs however)

> 
> > > > > +
> > > > > +       pr_info("EP93xx SoC revision %s\n", attrs->revision);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +core_initcall(ep93xx_soc_init);
> > > > 
> > > > That's not the way to add soc driver. You need a proper driver
> > > > for it
> > > 
> > > What is a proper driver for these ? 
> > 
> > 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.

Indeed.

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