| Message ID | 20230319191814.22067-5-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: Add basic LED support for switch/phy | expand |
> +#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. Andrew
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
On Mon, Mar 20, 2023 at 08:31:36PM +0100, Andrew Lunn wrote: > 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. > 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. So my suggestion is to keep the LEDS_CLASS and just remove the part for led_init_default_state_get. 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)
> 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
> 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. 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)