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

[Krzysztof Kozlowski]

On 24/07/2023 17:02, Nikita Shubin wrote:
>>> +static DEFINE_SPINLOCK(ep93xx_swlock);
>>> +
>>> +/* EP93xx System Controller software locked register write */
>>> +void ep93xx_syscon_swlocked_write(struct regmap *map, unsigned int
>>> reg, unsigned int val)
>>> +{
>>> +       unsigned long flags;
>>> +
>>> +       spin_lock_irqsave(&ep93xx_swlock, flags);
>>> +
>>> +       regmap_write(map, EP93XX_SYSCON_SWLOCK,
>>> EP93XX_SWLOCK_MAGICK);
>>> +       regmap_write(map, reg, val);
>>> +
>>> +       spin_unlock_irqrestore(&ep93xx_swlock, flags);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(ep93xx_syscon_swlocked_write, EP93XX_SOC);
>>
>> I doubt that your code compiles. Didn't you add a user of this in
>> some
>> earlier patch?
>>
>> Anyway, no, drop it, don't export some weird calls from core initcall
>> to
>> drivers. You violate layering and driver encapsulation. There is no
>> dependency/probe ordering.
>>
>> 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.

...

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

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

> 
> The only i can see is:
>  ./versatile/soc-
> realview.c:132:builtin_platform_driver(realview_soc_driver);
> 
> By Linus =).

20 years ago module parameters were quite popular. Try to add one now, I
know what comment you will get. Just because something was added by
someone some time ago, is not a reason to do the same. Things change,
Linux changed.

Best regards,
Krzysztof