[PATCH v2 09/87] Rename ARCH_NPCM7xx

[Simon Glass]

Hi,

On Wed, 1 Feb 2023 at 09:22, Simon Glass <sjg at chromium.org> wrote:
>
> Hi,
>
> On Wed, 1 Feb 2023 at 08:48, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Wed, Feb 01, 2023 at 01:19:51PM +0100, Rasmus Villemoes wrote:
> > > On 31/01/2023 15.16, Simon Glass wrote:
> > > > Hi Rasmus,
> > > >
> > > > On Mon, 30 Jan 2023 at 14:12, Rasmus Villemoes
> > > > <rasmus.villemoes at prevas.dk> wrote:
> > > >>
> > > >> On 30/01/2023 16.54, Tom Rini wrote:
> > > >>> On Sun, Jan 29, 2023 at 10:57:28PM +0100, Rasmus Villemoes wrote:
> > > >>>> On 29/01/2023 01.57, Simon Glass wrote:
> > > >>>>> CONFIG options must not use lower-case letter.
> > > >>>>
> > > >>>> Why?
> > > >>>
> > > >>> So, kconfiglib complains about these.
> > > >>
> > > >> Which IMO would be a bug in kconfiglib. Can you point me at where that
> > > >> warning is in kconfiglib.py and how it looks and when one would
> > > >> encounter it?
> > > >>
> > > >>> However, I can't find a formal
> > > >>> language definition and the kernel documentation doesn't specify, merely
> > > >>> imply that it should always be all uppercase.
> > > >>
> > > >> Well, yes, mostly, but since the de facto specification (namely, the
> > > >> kernel's implementation) doesn't complain and the kernel's Kconfig files
> > > >> do contain several examples of config symbols with lowercase characters,
> > > >> why deviate? In particular, since we share a lot of code, if some piece
> > > >> of kernel code has an IS_ENABLED(CONFIG_FOO876xx), why make it harder to
> > > >> import and keep that in sync?
> > > >>
> > > >> Perhaps we can get Masahiro to tell us whether lowercase characters are
> > > >> allowed in kconfig symbols or not.
> > > >>
> > > >> For reference, another kconfig-using project decided to fix their own
> > > >> infrastructure around kconfig instead of enforcing uppercase symbols:
> > > >>
> > > >> https://github.com/zephyrproject-rtos/zephyr/issues/40420
> > > >
> > > > That's all good context, thank you.
> > > >
> > > > When we use #define it is normally with an upper-case string. The is
> > > > the convention in U-Boot and Linux (and many other projects), I
> > > > believe.
> > >
> > > I don't disagree with the "normally", but that very word is precisely my
> > > point: It's not universal, and there can be good reasons to deviate.
> > > Both projects frown upon camelcase, so you don't see a lot of mixed-case
> > > macros, but there's definitely a lot of all-lowercase ones (e.g.
> > > iterators), and to pick just one example of a justified mixed-case one,
> > > there's a "#define RANGE_mA" in some iio device.
> > >
> > >  Also, having lower and upper case strings does become
> > > > confusing, and inconsistent.
> > > >
> > > > I was unaware that lower-case was allowed in Linux. It seems there are
> > > > 35 cases of this in Linux.
> > >
> > > Dunno how you reach that number, I can easily find 174. Among them stuff
>
> Yes I was missing an underscore.
>
> git grep -E "CONFIG_([A-Za-z0-9_]*)\b" |sed -n
> "s/.*\bCONFIG_\([A-Za-z0-9_]*\)\b.*/\1/p" | sort  |uniq  | grep
> "[a-z]"  |wc -l
> 187
>
> But some are not real.
>
> > > like "config FONT_6x11" which is definitely more readable than with an
> > > uppercase X. But yes, either way it's a very small fraction of all
> > > config symbols, but OTOH I highly doubt linux would accept patches to
> > > start renaming those without a very strong reason - it would break
> > > out-of-tree defconfig files.
>
> FONT_6X11 isn't that bad :-)
>
> > >
> > > > But perhaps we should not allow it in U-Boot?
> > >
> > > Discourage, definitely, perhaps even add something to checkpatch. But
> > > renaming existing seems to be pointless churn, and definitely needs a
> > > better rationale than "is not allowed" when that is manifestly not true.
> > >
> > > Maybe prepend a patch updating codingstyle.rst and amend this to say "is
> > > not allowed by the U-Boot coding standard". Or otherwise please provide
> > > some reference answering my "why".
>
> OK would you like to have a crack at these?
>
> >
> > I suspect the answer here is that we should:
> > - Patch (and submit upstream) kconfiglib.py to accept lowercase [a-z] as
> >   valid symbol names, as they are.
>
> That project is dead, unfortunately. I've had a PR outstanding for
> over a year. It looks like someone here might take it on, once we can
> figure out naming, etc.
>
> Rasmus, would you like to take a look at the one in U-Boot?
>
> Here is what Zephyr did:
>
> https://github.com/zephyrproject-rtos/zephyr/issues/40420
>
> > - Add a sentence or two to codingstyle.rst about discouraging lowercase,
> >   as suggested by Rasmus
> > - Drop the rename parts of this series.
>
> If people still want lower case that's OK with me. We have so few I
> felt it was just confusing, plus the fact that the widely use
> kconfiglib doesn't support it...

Actually that's not true, it does support lower-case Kconfig options,
or at least the one we do does. I'll send a v3 with these patches
dropped.

Regards,
Simon