| Message ID | 20230319191814.22067-5-ansuelsmth@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v5,01/15] net: dsa: qca8k: move qca8k_port_to_phy() to header | expand |
On Sun, Mar 19, 2023 at 11:49:02PM +0100, Andrew Lunn wrote: > > +#if IS_ENABLED(CONFIG_LEDS_CLASS) > > enum led_default_state led_init_default_state_get(struct fwnode_handle *fwnode); > > +#else > > +static inline enum led_default_state > > +led_init_default_state_get(struct fwnode_handle *fwnode) > > +{ > > + return LEDS_DEFSTATE_OFF; > > +} > > +#endif > > 0-day is telling me i have this wrong. The function is in led-core.c, > so this should be CONFIG_NEW_LEDS, not CONFIG_LEDS_CLASS. > Any idea why? NEW_LEDS just enable LEDS_CLASS selection so why we need to use that? Should not make a difference (in theory) Anyway hoping every other patch and Documentation patch gets some review tag, v6 should be last revision I hope? (so we can move to LEDs stuff)
On Mon, Mar 20, 2023 at 06:52:47PM +0100, Christian Marangi wrote: > On Sun, Mar 19, 2023 at 11:49:02PM +0100, Andrew Lunn wrote: > > > +#if IS_ENABLED(CONFIG_LEDS_CLASS) > > > enum led_default_state led_init_default_state_get(struct fwnode_handle *fwnode); > > > +#else > > > +static inline enum led_default_state > > > +led_init_default_state_get(struct fwnode_handle *fwnode) > > > +{ > > > + return LEDS_DEFSTATE_OFF; > > > +} > > > +#endif > > > > 0-day is telling me i have this wrong. The function is in led-core.c, > > so this should be CONFIG_NEW_LEDS, not CONFIG_LEDS_CLASS. > > > > Any idea why? NEW_LEDS just enable LEDS_CLASS selection so why we need > to use that? Should not make a difference (in theory) 0-day came up with a configuration which resulted in NEW_LEDS enabled but LEDS_CLASS disabled. That then resulted in multiple definitions of led_init_default_state_get() when linking. I _guess_ this is because select is used, which is not mandatory. So randconfig can turn off something which is enabled by select. I updated my tree, and so far 0-day has not complained, but it can take a few days when it is busy. Andrew
> Also why IS_ENABLED instead of a simple ifdef? (in leds.h there is a mix > of both so I wonder if we should use one or the other) /* * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm', * 0 otherwise. Note that CONFIG_FOO=y results in "#define CONFIG_FOO 1" in * autoconf.h, while CONFIG_FOO=m results in "#define CONFIG_FOO_MODULE 1". */ #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option)) It cleanly handles the module case, which i guess most people would get wrong. Andrew
On Tue, Mar 21, 2023 at 05:02:42PM +0100, Andrew Lunn wrote: > > BTW yes I repro the problem. > > > > Checked the makefile and led-core.c is compiled with NEW_LEDS and > > led-class is compiled with LEDS_CLASS. > > > > led_init_default_state_get is in led-core.c and this is the problem with > > using LEDS_CLASS instead of NEW_LEDS... > > > > But actually why we are putting led_init_default_state_get behind a > > config? IMHO we should compile it anyway. > > It is pointless if you don't have any LED support. To make it always > compiled, you would probably need to move it into leds.h. And then you > bloat every user with some code which is not hot path. > Ok think just to be safe we should wait one more day for the 0 day bot and then finally send v6?
diff --git a/include/linux/leds.h b/include/linux/leds.h index d71201a968b6..f6dec57453da 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -82,7 +82,15 @@ struct led_init_data { bool devname_mandatory; }; +#if IS_ENABLED(CONFIG_LEDS_CLASS) enum led_default_state led_init_default_state_get(struct fwnode_handle *fwnode); +#else +static inline enum led_default_state +led_init_default_state_get(struct fwnode_handle *fwnode) +{ + return LEDS_DEFSTATE_OFF; +} +#endif struct led_hw_trigger_type { int dummy; @@ -217,9 +225,19 @@ static inline int led_classdev_register(struct device *parent, return led_classdev_register_ext(parent, led_cdev, NULL); } +#if IS_ENABLED(CONFIG_LEDS_CLASS) int devm_led_classdev_register_ext(struct device *parent, struct led_classdev *led_cdev, struct led_init_data *init_data); +#else +static inline int +devm_led_classdev_register_ext(struct device *parent, + struct led_classdev *led_cdev, + struct led_init_data *init_data) +{ + return 0; +} +#endif static inline int devm_led_classdev_register(struct device *parent, struct led_classdev *led_cdev)