diff mbox series

[1/2] linux/kconfig.h: replace IF_ENABLED() with PTR_IF() in <linux/kernel.h>

Message ID 20210408205858.51751-2-masahiroy@kernel.org
State New
Headers show
Series linux/kconfig.h: move IF_ENABLED() out of <linux/kconfig.h> | expand

Commit Message

Masahiro Yamada April 8, 2021, 8:58 p.m. UTC
<linux/kconfig.h> is included from all the kernel-space source files,
including C, assembly, linker scripts. It is intended to contain minimal
set of macros to evaluate CONFIG options.

IF_ENABLED() is an intruder here because (x ? y : z) is C code, which
should not be included from assembly files or linker scripts.

Also, <linux/kconfig.h> is no longer self-contained because NULL is
defined in <linux/stddef.h>.

Move IF_ENABLED() out to <linux/kernel.h> as PTR_IF().

PTR_IF(IS_ENABLED(CONFIG_FOO), ...) is slightly longer than
IF_ENABLED(CONFIG_FOO, ...), but it is not a big deal because
sub-systems often define dedicated macros such as of_match_ptr(),
pm_ptr() etc. for common use-cases.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 drivers/pinctrl/pinctrl-ingenic.c | 20 ++++++++++----------
 include/linux/kconfig.h           |  6 ------
 include/linux/kernel.h            |  2 ++
 3 files changed, 12 insertions(+), 16 deletions(-)

Comments

Paul Cercueil April 9, 2021, 8:15 a.m. UTC | #1
Hi Masahiro,

Le ven. 9 avril 2021 à 5:58, Masahiro Yamada <masahiroy@kernel.org> a 
écrit :
> <linux/kconfig.h> is included from all the kernel-space source files,

> including C, assembly, linker scripts. It is intended to contain 

> minimal

> set of macros to evaluate CONFIG options.

> 

> IF_ENABLED() is an intruder here because (x ? y : z) is C code, which

> should not be included from assembly files or linker scripts.

> 

> Also, <linux/kconfig.h> is no longer self-contained because NULL is

> defined in <linux/stddef.h>.

> 

> Move IF_ENABLED() out to <linux/kernel.h> as PTR_IF().

> 

> PTR_IF(IS_ENABLED(CONFIG_FOO), ...) is slightly longer than

> IF_ENABLED(CONFIG_FOO, ...), but it is not a big deal because

> sub-systems often define dedicated macros such as of_match_ptr(),

> pm_ptr() etc. for common use-cases.


What's the idea behind changing IF_ENABLED() to PTR_IF()? You didn't 
explain that. What's wrong with IF_ENABLED()?

Cheers,
-Paul

> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

> ---

> 

>  drivers/pinctrl/pinctrl-ingenic.c | 20 ++++++++++----------

>  include/linux/kconfig.h           |  6 ------

>  include/linux/kernel.h            |  2 ++

>  3 files changed, 12 insertions(+), 16 deletions(-)

> 

> diff --git a/drivers/pinctrl/pinctrl-ingenic.c 

> b/drivers/pinctrl/pinctrl-ingenic.c

> index f2746125b077..b21e2ae4528d 100644

> --- a/drivers/pinctrl/pinctrl-ingenic.c

> +++ b/drivers/pinctrl/pinctrl-ingenic.c

> @@ -2496,43 +2496,43 @@ static int __init 

> ingenic_pinctrl_probe(struct platform_device *pdev)

>  static const struct of_device_id ingenic_pinctrl_of_match[] = {

>  	{

>  		.compatible = "ingenic,jz4740-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_JZ4740, &jz4740_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4740), &jz4740_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,jz4725b-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_JZ4725B, &jz4725b_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4725B), &jz4725b_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,jz4760-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_JZ4760, &jz4760_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4760), &jz4760_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,jz4760b-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_JZ4760, &jz4760_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4760), &jz4760_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,jz4770-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_JZ4770, &jz4770_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4770), &jz4770_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,jz4780-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_JZ4780, &jz4780_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4780), &jz4780_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,x1000-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_X1000, &x1000_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1000), &x1000_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,x1000e-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_X1000, &x1000_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1000), &x1000_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,x1500-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_X1500, &x1500_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1500), &x1500_chip_info)

>  	},

>  	{

>  		.compatible = "ingenic,x1830-pinctrl",

> -		.data = IF_ENABLED(CONFIG_MACH_X1830, &x1830_chip_info)

> +		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1830), &x1830_chip_info)

>  	},

>  	{ /* sentinel */ },

>  };

> diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h

> index 24a59cb06963..cc8fa109cfa3 100644

> --- a/include/linux/kconfig.h

> +++ b/include/linux/kconfig.h

> @@ -70,10 +70,4 @@

>   */

>  #define IS_ENABLED(option) __or(IS_BUILTIN(option), 

> IS_MODULE(option))

> 

> -/*

> - * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is 

> set to 'y'

> - * or 'm', NULL otherwise.

> - */

> -#define IF_ENABLED(option, ptr) (IS_ENABLED(option) ? (ptr) : NULL)

> -

>  #endif /* __LINUX_KCONFIG_H */

> diff --git a/include/linux/kernel.h b/include/linux/kernel.h

> index 5b7ed6dc99ac..8685ca4cf287 100644

> --- a/include/linux/kernel.h

> +++ b/include/linux/kernel.h

> @@ -38,6 +38,8 @@

>  #define PTR_ALIGN_DOWN(p, a)	((typeof(p))ALIGN_DOWN((unsigned 

> long)(p), (a)))

>  #define IS_ALIGNED(x, a)		(((x) & ((typeof(x))(a) - 1)) == 0)

> 

> +#define PTR_IF(cond, ptr)	((cond) ? (ptr) : NULL)

> +

>  /* generic data direction definitions */

>  #define READ			0

>  #define WRITE			1

> --

> 2.27.0

>
Masahiro Yamada April 9, 2021, 8:30 a.m. UTC | #2
On Fri, Apr 9, 2021 at 5:15 PM Paul Cercueil <paul@crapouillou.net> wrote:
>

> Hi Masahiro,

>

> Le ven. 9 avril 2021 à 5:58, Masahiro Yamada <masahiroy@kernel.org> a

> écrit :

> > <linux/kconfig.h> is included from all the kernel-space source files,

> > including C, assembly, linker scripts. It is intended to contain

> > minimal

> > set of macros to evaluate CONFIG options.

> >

> > IF_ENABLED() is an intruder here because (x ? y : z) is C code, which

> > should not be included from assembly files or linker scripts.

> >

> > Also, <linux/kconfig.h> is no longer self-contained because NULL is

> > defined in <linux/stddef.h>.

> >

> > Move IF_ENABLED() out to <linux/kernel.h> as PTR_IF().

> >

> > PTR_IF(IS_ENABLED(CONFIG_FOO), ...) is slightly longer than

> > IF_ENABLED(CONFIG_FOO, ...), but it is not a big deal because

> > sub-systems often define dedicated macros such as of_match_ptr(),

> > pm_ptr() etc. for common use-cases.

>

> What's the idea behind changing IF_ENABLED() to PTR_IF()? You didn't

> explain that. What's wrong with IF_ENABLED()?



PTR_IF() is a more generalized variant, which I believe is
a better fit in <linux/kernel.h>
The first parameter does not need to be a CONFIG option,
but any expression.







> Cheers,

> -Paul

>

> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

> > ---

> >

> >  drivers/pinctrl/pinctrl-ingenic.c | 20 ++++++++++----------

> >  include/linux/kconfig.h           |  6 ------

> >  include/linux/kernel.h            |  2 ++

> >  3 files changed, 12 insertions(+), 16 deletions(-)

> >

> > diff --git a/drivers/pinctrl/pinctrl-ingenic.c

> > b/drivers/pinctrl/pinctrl-ingenic.c

> > index f2746125b077..b21e2ae4528d 100644

> > --- a/drivers/pinctrl/pinctrl-ingenic.c

> > +++ b/drivers/pinctrl/pinctrl-ingenic.c

> > @@ -2496,43 +2496,43 @@ static int __init

> > ingenic_pinctrl_probe(struct platform_device *pdev)

> >  static const struct of_device_id ingenic_pinctrl_of_match[] = {

> >       {

> >               .compatible = "ingenic,jz4740-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_JZ4740, &jz4740_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4740), &jz4740_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,jz4725b-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_JZ4725B, &jz4725b_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4725B), &jz4725b_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,jz4760-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_JZ4760, &jz4760_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4760), &jz4760_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,jz4760b-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_JZ4760, &jz4760_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4760), &jz4760_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,jz4770-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_JZ4770, &jz4770_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4770), &jz4770_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,jz4780-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_JZ4780, &jz4780_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4780), &jz4780_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,x1000-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_X1000, &x1000_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1000), &x1000_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,x1000e-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_X1000, &x1000_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1000), &x1000_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,x1500-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_X1500, &x1500_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1500), &x1500_chip_info)

> >       },

> >       {

> >               .compatible = "ingenic,x1830-pinctrl",

> > -             .data = IF_ENABLED(CONFIG_MACH_X1830, &x1830_chip_info)

> > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1830), &x1830_chip_info)

> >       },

> >       { /* sentinel */ },

> >  };

> > diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h

> > index 24a59cb06963..cc8fa109cfa3 100644

> > --- a/include/linux/kconfig.h

> > +++ b/include/linux/kconfig.h

> > @@ -70,10 +70,4 @@

> >   */

> >  #define IS_ENABLED(option) __or(IS_BUILTIN(option),

> > IS_MODULE(option))

> >

> > -/*

> > - * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is

> > set to 'y'

> > - * or 'm', NULL otherwise.

> > - */

> > -#define IF_ENABLED(option, ptr) (IS_ENABLED(option) ? (ptr) : NULL)

> > -

> >  #endif /* __LINUX_KCONFIG_H */

> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h

> > index 5b7ed6dc99ac..8685ca4cf287 100644

> > --- a/include/linux/kernel.h

> > +++ b/include/linux/kernel.h

> > @@ -38,6 +38,8 @@

> >  #define PTR_ALIGN_DOWN(p, a) ((typeof(p))ALIGN_DOWN((unsigned

> > long)(p), (a)))

> >  #define IS_ALIGNED(x, a)             (((x) & ((typeof(x))(a) - 1)) == 0)

> >

> > +#define PTR_IF(cond, ptr)    ((cond) ? (ptr) : NULL)

> > +

> >  /* generic data direction definitions */

> >  #define READ                 0

> >  #define WRITE                        1

> > --

> > 2.27.0

> >

>

>



-- 
Best Regards
Masahiro Yamada
Andy Shevchenko April 9, 2021, 9:24 a.m. UTC | #3
On Fri, Apr 9, 2021 at 12:00 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>

> <linux/kconfig.h> is included from all the kernel-space source files,

> including C, assembly, linker scripts. It is intended to contain minimal


a minimal

> set of macros to evaluate CONFIG options.

>

> IF_ENABLED() is an intruder here because (x ? y : z) is C code, which

> should not be included from assembly files or linker scripts.

>

> Also, <linux/kconfig.h> is no longer self-contained because NULL is

> defined in <linux/stddef.h>.

>

> Move IF_ENABLED() out to <linux/kernel.h> as PTR_IF().

>

> PTR_IF(IS_ENABLED(CONFIG_FOO), ...) is slightly longer than

> IF_ENABLED(CONFIG_FOO, ...), but it is not a big deal because

> sub-systems often define dedicated macros such as of_match_ptr(),

> pm_ptr() etc. for common use-cases.


>  include/linux/kernel.h            |  2 ++


Why kernel.h? Shouldn't it belong to a particular domain with a
respective header file?

Really what we have in the kernel.h right now is a complete train
wreck of something.
We have to define what exactly is kernel.h for?

Arnd? Others? Shall we start a wider discussion on the topic?

-- 
With Best Regards,
Andy Shevchenko
Paul Cercueil April 9, 2021, 9:38 a.m. UTC | #4
Le ven. 9 avril 2021 à 17:30, Masahiro Yamada <masahiroy@kernel.org> a 
écrit :
> On Fri, Apr 9, 2021 at 5:15 PM Paul Cercueil <paul@crapouillou.net> 

> wrote:

>> 

>>  Hi Masahiro,

>> 

>>  Le ven. 9 avril 2021 à 5:58, Masahiro Yamada 

>> <masahiroy@kernel.org> a

>>  écrit :

>>  > <linux/kconfig.h> is included from all the kernel-space source 

>> files,

>>  > including C, assembly, linker scripts. It is intended to contain

>>  > minimal

>>  > set of macros to evaluate CONFIG options.

>>  >

>>  > IF_ENABLED() is an intruder here because (x ? y : z) is C code, 

>> which

>>  > should not be included from assembly files or linker scripts.

>>  >

>>  > Also, <linux/kconfig.h> is no longer self-contained because NULL 

>> is

>>  > defined in <linux/stddef.h>.

>>  >

>>  > Move IF_ENABLED() out to <linux/kernel.h> as PTR_IF().

>>  >

>>  > PTR_IF(IS_ENABLED(CONFIG_FOO), ...) is slightly longer than

>>  > IF_ENABLED(CONFIG_FOO, ...), but it is not a big deal because

>>  > sub-systems often define dedicated macros such as of_match_ptr(),

>>  > pm_ptr() etc. for common use-cases.

>> 

>>  What's the idea behind changing IF_ENABLED() to PTR_IF()? You didn't

>>  explain that. What's wrong with IF_ENABLED()?

> 

> 

> PTR_IF() is a more generalized variant, which I believe is

> a better fit in <linux/kernel.h>

> The first parameter does not need to be a CONFIG option,

> but any expression.


Fair enough, but we could still have IF_ENABLED as a specialized 
variant of PTR_IF:

#define IF_ENABLED(cfg, ptr) PTR_IF(IS_ENABLED(cfg), (ptr))

-Paul

> 

> 

>>  Cheers,

>>  -Paul

>> 

>>  > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

>>  > ---

>>  >

>>  >  drivers/pinctrl/pinctrl-ingenic.c | 20 ++++++++++----------

>>  >  include/linux/kconfig.h           |  6 ------

>>  >  include/linux/kernel.h            |  2 ++

>>  >  3 files changed, 12 insertions(+), 16 deletions(-)

>>  >

>>  > diff --git a/drivers/pinctrl/pinctrl-ingenic.c

>>  > b/drivers/pinctrl/pinctrl-ingenic.c

>>  > index f2746125b077..b21e2ae4528d 100644

>>  > --- a/drivers/pinctrl/pinctrl-ingenic.c

>>  > +++ b/drivers/pinctrl/pinctrl-ingenic.c

>>  > @@ -2496,43 +2496,43 @@ static int __init

>>  > ingenic_pinctrl_probe(struct platform_device *pdev)

>>  >  static const struct of_device_id ingenic_pinctrl_of_match[] = {

>>  >       {

>>  >               .compatible = "ingenic,jz4740-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_JZ4740, 

>> &jz4740_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4740), 

>> &jz4740_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,jz4725b-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_JZ4725B, 

>> &jz4725b_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4725B), 

>> &jz4725b_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,jz4760-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_JZ4760, 

>> &jz4760_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4760), 

>> &jz4760_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,jz4760b-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_JZ4760, 

>> &jz4760_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4760), 

>> &jz4760_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,jz4770-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_JZ4770, 

>> &jz4770_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4770), 

>> &jz4770_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,jz4780-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_JZ4780, 

>> &jz4780_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4780), 

>> &jz4780_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,x1000-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_X1000, 

>> &x1000_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1000), 

>> &x1000_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,x1000e-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_X1000, 

>> &x1000_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1000), 

>> &x1000_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,x1500-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_X1500, 

>> &x1500_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1500), 

>> &x1500_chip_info)

>>  >       },

>>  >       {

>>  >               .compatible = "ingenic,x1830-pinctrl",

>>  > -             .data = IF_ENABLED(CONFIG_MACH_X1830, 

>> &x1830_chip_info)

>>  > +             .data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1830), 

>> &x1830_chip_info)

>>  >       },

>>  >       { /* sentinel */ },

>>  >  };

>>  > diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h

>>  > index 24a59cb06963..cc8fa109cfa3 100644

>>  > --- a/include/linux/kconfig.h

>>  > +++ b/include/linux/kconfig.h

>>  > @@ -70,10 +70,4 @@

>>  >   */

>>  >  #define IS_ENABLED(option) __or(IS_BUILTIN(option),

>>  > IS_MODULE(option))

>>  >

>>  > -/*

>>  > - * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO 

>> is

>>  > set to 'y'

>>  > - * or 'm', NULL otherwise.

>>  > - */

>>  > -#define IF_ENABLED(option, ptr) (IS_ENABLED(option) ? (ptr) : 

>> NULL)

>>  > -

>>  >  #endif /* __LINUX_KCONFIG_H */

>>  > diff --git a/include/linux/kernel.h b/include/linux/kernel.h

>>  > index 5b7ed6dc99ac..8685ca4cf287 100644

>>  > --- a/include/linux/kernel.h

>>  > +++ b/include/linux/kernel.h

>>  > @@ -38,6 +38,8 @@

>>  >  #define PTR_ALIGN_DOWN(p, a) ((typeof(p))ALIGN_DOWN((unsigned

>>  > long)(p), (a)))

>>  >  #define IS_ALIGNED(x, a)             (((x) & ((typeof(x))(a) - 

>> 1)) == 0)

>>  >

>>  > +#define PTR_IF(cond, ptr)    ((cond) ? (ptr) : NULL)

>>  > +

>>  >  /* generic data direction definitions */

>>  >  #define READ                 0

>>  >  #define WRITE                        1

>>  > --

>>  > 2.27.0

>>  >

>> 

>> 

> 

> 

> --

> Best Regards

> Masahiro Yamada
Masahiro Yamada April 10, 2021, 3:52 a.m. UTC | #5
On Fri, Apr 9, 2021 at 6:24 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>

> On Fri, Apr 9, 2021 at 12:00 AM Masahiro Yamada <masahiroy@kernel.org> wrote:

> >

> > <linux/kconfig.h> is included from all the kernel-space source files,

> > including C, assembly, linker scripts. It is intended to contain minimal

>

> a minimal

>

> > set of macros to evaluate CONFIG options.

> >

> > IF_ENABLED() is an intruder here because (x ? y : z) is C code, which

> > should not be included from assembly files or linker scripts.

> >

> > Also, <linux/kconfig.h> is no longer self-contained because NULL is

> > defined in <linux/stddef.h>.

> >

> > Move IF_ENABLED() out to <linux/kernel.h> as PTR_IF().

> >

> > PTR_IF(IS_ENABLED(CONFIG_FOO), ...) is slightly longer than

> > IF_ENABLED(CONFIG_FOO, ...), but it is not a big deal because

> > sub-systems often define dedicated macros such as of_match_ptr(),

> > pm_ptr() etc. for common use-cases.

>

> >  include/linux/kernel.h            |  2 ++

>

> Why kernel.h? Shouldn't it belong to a particular domain with a

> respective header file?

>

> Really what we have in the kernel.h right now is a complete train

> wreck of something.

> We have to define what exactly is kernel.h for?



<linux/kernel.h> contains random utility macros.

I did not find a good header to put it in otherwise.


>

> Arnd? Others? Shall we start a wider discussion on the topic?

>

> --

> With Best Regards,

> Andy Shevchenko



--
Best Regards
Masahiro Yamada
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index f2746125b077..b21e2ae4528d 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -2496,43 +2496,43 @@  static int __init ingenic_pinctrl_probe(struct platform_device *pdev)
 static const struct of_device_id ingenic_pinctrl_of_match[] = {
 	{
 		.compatible = "ingenic,jz4740-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_JZ4740, &jz4740_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4740), &jz4740_chip_info)
 	},
 	{
 		.compatible = "ingenic,jz4725b-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_JZ4725B, &jz4725b_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4725B), &jz4725b_chip_info)
 	},
 	{
 		.compatible = "ingenic,jz4760-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_JZ4760, &jz4760_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4760), &jz4760_chip_info)
 	},
 	{
 		.compatible = "ingenic,jz4760b-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_JZ4760, &jz4760_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4760), &jz4760_chip_info)
 	},
 	{
 		.compatible = "ingenic,jz4770-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_JZ4770, &jz4770_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4770), &jz4770_chip_info)
 	},
 	{
 		.compatible = "ingenic,jz4780-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_JZ4780, &jz4780_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_JZ4780), &jz4780_chip_info)
 	},
 	{
 		.compatible = "ingenic,x1000-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_X1000, &x1000_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1000), &x1000_chip_info)
 	},
 	{
 		.compatible = "ingenic,x1000e-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_X1000, &x1000_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1000), &x1000_chip_info)
 	},
 	{
 		.compatible = "ingenic,x1500-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_X1500, &x1500_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1500), &x1500_chip_info)
 	},
 	{
 		.compatible = "ingenic,x1830-pinctrl",
-		.data = IF_ENABLED(CONFIG_MACH_X1830, &x1830_chip_info)
+		.data = PTR_IF(IS_ENABLED(CONFIG_MACH_X1830), &x1830_chip_info)
 	},
 	{ /* sentinel */ },
 };
diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index 24a59cb06963..cc8fa109cfa3 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -70,10 +70,4 @@ 
  */
 #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
 
-/*
- * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is set to 'y'
- * or 'm', NULL otherwise.
- */
-#define IF_ENABLED(option, ptr) (IS_ENABLED(option) ? (ptr) : NULL)
-
 #endif /* __LINUX_KCONFIG_H */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5b7ed6dc99ac..8685ca4cf287 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -38,6 +38,8 @@ 
 #define PTR_ALIGN_DOWN(p, a)	((typeof(p))ALIGN_DOWN((unsigned long)(p), (a)))
 #define IS_ALIGNED(x, a)		(((x) & ((typeof(x))(a) - 1)) == 0)
 
+#define PTR_IF(cond, ptr)	((cond) ? (ptr) : NULL)
+
 /* generic data direction definitions */
 #define READ			0
 #define WRITE			1