The logic seems sound but somehow CFLAGS_LINUXKPI is odd to me, like CFLAGS_* should contain flags rather than be a boolean. E.g. we have CFLAGS_NO_SIMD= -mno-mmx -mno-sse...
Hrm, but we also have e.g. SSP_CFLAGS?= -fstack-protector-strong -fstack-clash-protection and that form is more common.
ADD_LINUXKPI_CFLAGS?

In D60207#1381676, @emaste wrote:

The logic seems sound but somehow CFLAGS_LINUXKPI is odd to me, like CFLAGS_* should contain flags rather than be a boolean. E.g. we have CFLAGS_NO_SIMD= -mno-mmx -mno-sse...
Hrm, but we also have e.g. SSP_CFLAGS?= -fstack-protector-strong -fstack-clash-protection and that form is more common.
ADD_LINUXKPI_CFLAGS?

I was looking at the *EXTRA versions we have initially and figured we do use suffixes in other parts like MODULES_EXTRA, _COPTFLAGS_EXTRA, CWARNEXTRA (no _), KERNEL_EXTRA. But CFLAGS_EXTRA wouldn't work so it became CFLAGS_LINUXKPI.

Maybe just LINUXKPI_CFLAGS without the ADD keeping the "LINUXKPI" name space like in LINUXKPI_GENSRC and LINUXKPI_INCLUDES?
Then I'd likely remove the LINUXKPI_CFLAGS= version from the docs and make sure it is set to something at least (YES or -I...)?

In the end I'll do the s///g to whatever people think is best.

In the end I'll do the s///g to whatever people think is best.

Unfortunately I don't have a great idea of a better name, it just looked a little odd. So I suspect you'll have to pick something and nobody will object. If you want to leave it as is that's fine with me too. It looks a bit strange but it's obvious in context that we're not literally adding "YES" to CFLAGS.

I was going to say that e.g. LINUXKPI_CFLAGS=YES would be my suggestion before discovering that most of the literal CFLAGS additions use that form already. I do like that it's in a LINUXKPI namespace though.

In D60207#1381711, @emaste wrote:

In the end I'll do the s///g to whatever people think is best.

Unfortunately I don't have a great idea of a better name, it just looked a little odd. So I suspect you'll have to pick something and nobody will object. If you want to leave it as is that's fine with me too. It looks a bit strange but it's obvious in context that we're not literally adding "YES" to CFLAGS.

I was going to say that e.g. LINUXKPI_CFLAGS=YES would be my suggestion before discovering that most of the literal CFLAGS additions use that form already. I do like that it's in a LINUXKPI namespace though.

I had the same immediate thought after looking at the patch and not yet reading the discussion. CLFAGS_XXX sounds as something that should be used on the compiler command line.

Might be LINUXKPI_SOURCES?

What if I'll change it one step further and call it LINUXKPI_ADD and we add GENSRC to that too along with INCLUDES?

I'd then also add a VER= tag and it will be very helpful in the future to conditionally add for USB CFLAGS based on a USB flag?
The VER= tag I love as it will help us in a point in the future to enforce that all modules have a version defined; that way we get a very good picture of what the minimum version we support actually is and changes like D59688 will be a lot easier in the future...

Exemplary Makefile:

LINUXKPI_ADD=   GENSRCS INCLUDES USB VER=70300

.if defined(LINUXKPI_ADD)
.if ${LINUXKPI_ADD:MINCLUDES}
# Append LinuxKPI "-include", etc., which must happen after the global
# "-include" as otherwise, e.g., opt_global.h will not be visible in
# kconfig.h leading to unexpected results.
CFLAGS+=        LINUXKPI_INCLUDES
.endif
.if ${LINUXKPI_ADD:MGENSRCS}
SRCS+=  LINUXKPI_GENSRCS
.endif
.if ${LINUXKPI_ADD:MUSB}
CFLAGS+=  LINUXKPI_INCLUDES_USB
.endif
.if ${LINUXKPI_ADD:MVER=[0-9]*}
CFLAGS+=        -DLINUXKPI_VERSION=${LINUXKPI_ADD:MVER=[0-9]*:S/VER=//1}
.endif
.endif

all:
        @echo "CFLAGS: ${CFLAGS}"
        @echo "SRCS  : ${SRCS}"

In D60207#1385948, @bz wrote:

What if I'll change it one step further and call it LINUXKPI_ADD and we add GENSRC to that too along with INCLUDES?

I'd then also add a VER= tag and it will be very helpful in the future to conditionally add for USB CFLAGS based on a USB flag?
The VER= tag I love as it will help us in a point in the future to enforce that all modules have a version defined; that way we get a very good picture of what the minimum version we support actually is and changes like D59688 will be a lot easier in the future...

Exemplary Makefile:

LINUXKPI_ADD=   GENSRCS INCLUDES USB VER=70300

.if defined(LINUXKPI_ADD)
.if ${LINUXKPI_ADD:MINCLUDES}
# Append LinuxKPI "-include", etc., which must happen after the global
# "-include" as otherwise, e.g., opt_global.h will not be visible in
# kconfig.h leading to unexpected results.
CFLAGS+=        LINUXKPI_INCLUDES
.endif
.if ${LINUXKPI_ADD:MGENSRCS}
SRCS+=  LINUXKPI_GENSRCS
.endif
.if ${LINUXKPI_ADD:MUSB}
CFLAGS+=  LINUXKPI_INCLUDES_USB
.endif
.if ${LINUXKPI_ADD:MVER=[0-9]*}
CFLAGS+=        -DLINUXKPI_VERSION=${LINUXKPI_ADD:MVER=[0-9]*:S/VER=//1}
.endif
.endif

all:
        @echo "CFLAGS: ${CFLAGS}"
        @echo "SRCS  : ${SRCS}"

It looks fine to me. I would not call it LINUXKPI_ADD. Might be, USE_LINUXKPI ?

In D60207#1387061, @kib wrote:

It looks fine to me. I would not call it LINUXKPI_ADD. Might be, USE_LINUXKPI ?

Thanks a lot for the reply!

The problem with USE_ is that it's a colliding namespace again with ports I believe and drm-kmod and nvidia and possibly other consumers are coming out of ports so I am hesitant.
I had that problem with the driver Makefiles before that I used "WITH_" which obviously collides with src.conf.

I can turn it around and make it LINUXKPI_USE?

Might be LINUXKPI_FEATURES then? It is more aligned with the feature list that the variable takes.

In D60207#1387495, @kib wrote:

Might be LINUXKPI_FEATURES then? It is more aligned with the feature list that the variable takes.

I'd take that; @emaste also d'accord?

Adding @dumbbell given the recent drm-kmod build issues after my changes to LinuxKPI as this will be interesting there (though a lot harder to introduce given drm-kmod also needs to compile on older branches).