diff mbox series

[net-next,v5,04/15] leds: Provide stubs for when CLASS_LED is disabled

Message ID 20230319191814.22067-5-ansuelsmth@gmail.com
State New
Headers
Series net: Add basic LED support for switch/phy |

Commit Message

Christian Marangi March 19, 2023, 7:18 p.m. UTC 
  From: Andrew Lunn <andrew@lunn.ch>

Provide stubs for devm_led_classdev_register_ext() and
led_init_default_state_get() so that LED drivers embedded within other
drivers such as PHYs and Ethernet switches still build when LEDS_CLASS
is disabled. This also helps with Kconfig dependencies, which are
somewhat hairy for phylib and mdio and only get worse when adding a
dependency on LED_CLASS.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/leds.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Andrew Lunn March 19, 2023, 10:49 p.m. UTC | #1 
> +#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
Christian Marangi March 20, 2023, 5:52 p.m. UTC | #2 
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)
Andrew Lunn March 20, 2023, 7:31 p.m. UTC | #3 
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
Christian Marangi March 21, 2023, 7:55 a.m. UTC | #4 
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)
Andrew Lunn March 21, 2023, 3:58 p.m. UTC | #5 
> 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
Andrew Lunn March 21, 2023, 4:02 p.m. UTC | #6 
> 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
Christian Marangi March 21, 2023, 4:13 p.m. UTC | #7 
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 mbox series

Patch

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)