[PATCH v2 09/87] Rename ARCH_NPCM7xx

[Simon Glass]

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

Regards,
Simon