[PATCH 1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs

[PATCH 1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

These two functions have stub implementations that are called when
NEW_LEDS and/or LEDS_CLASS are disabled, theorerically allowing drivers
to optionally use the LED subsystem.

However, this has never really worked because a built-in driver is
unable to link against these functions if the LED class is in a loadable
module. Heiner ran into this problem with a driver that newly gained
a LEDS_CLASS dependency and suggested using an IS_REACHABLE() check.

This is the reverse approach, removing the stub entirely to acknowledge
that it is pointless in its current form, and that not having it avoids
misleading developers into thinking that they can rely on it.

This survived around 1000 randconfig builds to validate that any callers
of the interface already have the correct Kconfig dependency already,
with the exception of the one that Heiner just added.

Cc: Heiner Kallweit <hkallweit1@gmail.com>
Link: https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@gmail.com/T/#u
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/leds.h | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index 4754b02d3a2c..7598d472903a 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -82,15 +82,7 @@ struct led_init_data {
 	bool devname_mandatory;
 };
 
-#if IS_ENABLED(CONFIG_NEW_LEDS)
 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;
@@ -279,20 +271,9 @@ 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)
 {
-- 
2.39.2

[PATCH 2/2] leds: make flash and multicolor dependencies unconditional

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Along the same lines as making devm_led_classdev_register() declared
extern unconditional, do the same thing for the two sub-classes
that have similar stubs.

The users of these interfaces go to great lengths to allow building
with both the generic leds API and the extended version, but realistically
there is not much use in this, so just simplify it to always rely
on it and remove the confusing fallback logic.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/leds/Kconfig                 |  4 ++--
 drivers/leds/flash/Kconfig           |  4 ++--
 drivers/staging/greybus/Kconfig      |  2 +-
 drivers/staging/greybus/light.c      | 21 --------------------
 include/linux/led-class-flash.h      | 24 -----------------------
 include/linux/led-class-multicolor.h | 29 ----------------------------
 6 files changed, 5 insertions(+), 79 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index d721b254e1e4..9613a45a35bd 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -395,7 +395,7 @@ config LEDS_LP3952
 config LEDS_LP50XX
 	tristate "LED Support for TI LP5036/30/24/18/12/09 LED driver chip"
 	depends on LEDS_CLASS && REGMAP_I2C
-	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
+	depends on LEDS_CLASS_MULTICOLOR
 	help
 	  If you say yes here you get support for the Texas Instruments
 	  LP5036, LP5030, LP5024, LP5018, LP5012 and LP5009 LED driver.
@@ -406,7 +406,7 @@ config LEDS_LP50XX
 config LEDS_LP55XX_COMMON
 	tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
 	depends on LEDS_CLASS
-	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
+	depends on LEDS_CLASS_MULTICOLOR
 	depends on OF
 	depends on I2C
 	select FW_LOADER
diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
index 4e08dbc05709..b95f90cd5749 100644
--- a/drivers/leds/flash/Kconfig
+++ b/drivers/leds/flash/Kconfig
@@ -51,8 +51,8 @@ config LEDS_MAX77693
 config LEDS_MT6360
 	tristate "LED Support for Mediatek MT6360 PMIC"
 	depends on LEDS_CLASS && OF
-	depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
-	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
+	depends on LEDS_CLASS_FLASH
+	depends on LEDS_CLASS_MULTICOLOR
 	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
 	depends on MFD_MT6360
 	help
diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
index 927cfa4bc989..1e745a8d439c 100644
--- a/drivers/staging/greybus/Kconfig
+++ b/drivers/staging/greybus/Kconfig
@@ -64,7 +64,7 @@ config GREYBUS_HID
 
 config GREYBUS_LIGHT
 	tristate "Greybus LED Class driver"
-	depends on LEDS_CLASS
+	depends on LEDS_CLASS_FLASH
 	help
 	  Select this option if you have a device that follows the
 	  Greybus LED Class specification.
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
index 87d36948c610..d62f97249aca 100644
--- a/drivers/staging/greybus/light.c
+++ b/drivers/staging/greybus/light.c
@@ -29,13 +29,9 @@ struct gb_channel {
 	struct attribute_group		*attr_group;
 	const struct attribute_group	**attr_groups;
 	struct led_classdev		*led;
-#if IS_REACHABLE(CONFIG_LEDS_CLASS_FLASH)
 	struct led_classdev_flash	fled;
 	struct led_flash_setting	intensity_uA;
 	struct led_flash_setting	timeout_us;
-#else
-	struct led_classdev		cled;
-#endif
 	struct gb_light			*light;
 	bool				is_registered;
 	bool				releasing;
@@ -84,7 +80,6 @@ static bool is_channel_flash(struct gb_channel *channel)
 				   | GB_CHANNEL_MODE_INDICATOR));
 }
 
-#if IS_REACHABLE(CONFIG_LEDS_CLASS_FLASH)
 static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
 {
 	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
@@ -153,22 +148,6 @@ static int __gb_lights_flash_brightness_set(struct gb_channel *channel)
 
 	return __gb_lights_flash_intensity_set(channel, intensity);
 }
-#else
-static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
-{
-	return container_of(cdev, struct gb_channel, cled);
-}
-
-static struct led_classdev *get_channel_cdev(struct gb_channel *channel)
-{
-	return &channel->cled;
-}
-
-static int __gb_lights_flash_brightness_set(struct gb_channel *channel)
-{
-	return 0;
-}
-#endif
 
 static int gb_lights_color_set(struct gb_channel *channel, u32 color);
 static int gb_lights_fade_set(struct gb_channel *channel);
diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index 612b4cab3819..36df927ec4b7 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -85,7 +85,6 @@ static inline struct led_classdev_flash *lcdev_to_flcdev(
 	return container_of(lcdev, struct led_classdev_flash, led_cdev);
 }
 
-#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH)
 /**
  * led_classdev_flash_register_ext - register a new object of LED class with
  *				     init data and with support for flash LEDs
@@ -116,29 +115,6 @@ int devm_led_classdev_flash_register_ext(struct device *parent,
 void devm_led_classdev_flash_unregister(struct device *parent,
 					struct led_classdev_flash *fled_cdev);
 
-#else
-
-static inline int led_classdev_flash_register_ext(struct device *parent,
-				    struct led_classdev_flash *fled_cdev,
-				    struct led_init_data *init_data)
-{
-	return 0;
-}
-
-static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {};
-static inline int devm_led_classdev_flash_register_ext(struct device *parent,
-				     struct led_classdev_flash *fled_cdev,
-				     struct led_init_data *init_data)
-{
-	return 0;
-}
-
-static inline void devm_led_classdev_flash_unregister(struct device *parent,
-					struct led_classdev_flash *fled_cdev)
-{};
-
-#endif  /* IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) */
-
 static inline int led_classdev_flash_register(struct device *parent,
 					   struct led_classdev_flash *fled_cdev)
 {
diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h
index 210d57bcd767..db9f34c6736e 100644
--- a/include/linux/led-class-multicolor.h
+++ b/include/linux/led-class-multicolor.h
@@ -30,7 +30,6 @@ static inline struct led_classdev_mc *lcdev_to_mccdev(
 	return container_of(led_cdev, struct led_classdev_mc, led_cdev);
 }
 
-#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR)
 /**
  * led_classdev_multicolor_register_ext - register a new object of led_classdev
  *				      class with support for multicolor LEDs
@@ -64,34 +63,6 @@ int devm_led_classdev_multicolor_register_ext(struct device *parent,
 
 void devm_led_classdev_multicolor_unregister(struct device *parent,
 					    struct led_classdev_mc *mcled_cdev);
-#else
-
-static inline int led_classdev_multicolor_register_ext(struct device *parent,
-					    struct led_classdev_mc *mcled_cdev,
-					    struct led_init_data *init_data)
-{
-	return 0;
-}
-
-static inline void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) {};
-static inline int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
-					       enum led_brightness brightness)
-{
-	return 0;
-}
-
-static inline int devm_led_classdev_multicolor_register_ext(struct device *parent,
-					  struct led_classdev_mc *mcled_cdev,
-					  struct led_init_data *init_data)
-{
-	return 0;
-}
-
-static inline void devm_led_classdev_multicolor_unregister(struct device *parent,
-					    struct led_classdev_mc *mcled_cdev)
-{};
-
-#endif  /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) */
 
 static inline int led_classdev_multicolor_register(struct device *parent,
 					    struct led_classdev_mc *mcled_cdev)
-- 
2.39.2

Re: [PATCH 2/2] leds: make flash and multicolor dependencies unconditional

[Rui Miguel Silva]

Hey Arnd,
Many thanks for the patch.

Arnd Bergmann <arnd@kernel.org> writes:

> From: Arnd Bergmann <arnd@arndb.de>
>
> Along the same lines as making devm_led_classdev_register() declared
> extern unconditional, do the same thing for the two sub-classes
> that have similar stubs.
>
> The users of these interfaces go to great lengths to allow building
> with both the generic leds API and the extended version, but realistically
> there is not much use in this, so just simplify it to always rely
> on it and remove the confusing fallback logic.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/leds/Kconfig                 |  4 ++--
>  drivers/leds/flash/Kconfig           |  4 ++--
>  drivers/staging/greybus/Kconfig      |  2 +-
>  drivers/staging/greybus/light.c      | 21 --------------------
>  include/linux/led-class-flash.h      | 24 -----------------------
>  include/linux/led-class-multicolor.h | 29 ----------------------------
>  6 files changed, 5 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index d721b254e1e4..9613a45a35bd 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -395,7 +395,7 @@ config LEDS_LP3952
>  config LEDS_LP50XX
>  	tristate "LED Support for TI LP5036/30/24/18/12/09 LED driver chip"
>  	depends on LEDS_CLASS && REGMAP_I2C
> -	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
> +	depends on LEDS_CLASS_MULTICOLOR
>  	help
>  	  If you say yes here you get support for the Texas Instruments
>  	  LP5036, LP5030, LP5024, LP5018, LP5012 and LP5009 LED driver.
> @@ -406,7 +406,7 @@ config LEDS_LP50XX
>  config LEDS_LP55XX_COMMON
>  	tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
>  	depends on LEDS_CLASS
> -	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
> +	depends on LEDS_CLASS_MULTICOLOR
>  	depends on OF
>  	depends on I2C
>  	select FW_LOADER
> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> index 4e08dbc05709..b95f90cd5749 100644
> --- a/drivers/leds/flash/Kconfig
> +++ b/drivers/leds/flash/Kconfig
> @@ -51,8 +51,8 @@ config LEDS_MAX77693
>  config LEDS_MT6360
>  	tristate "LED Support for Mediatek MT6360 PMIC"
>  	depends on LEDS_CLASS && OF
> -	depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
> -	depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
> +	depends on LEDS_CLASS_FLASH
> +	depends on LEDS_CLASS_MULTICOLOR
>  	depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>  	depends on MFD_MT6360
>  	help
> diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
> index 927cfa4bc989..1e745a8d439c 100644
> --- a/drivers/staging/greybus/Kconfig
> +++ b/drivers/staging/greybus/Kconfig
> @@ -64,7 +64,7 @@ config GREYBUS_HID
>  
>  config GREYBUS_LIGHT
>  	tristate "Greybus LED Class driver"
> -	depends on LEDS_CLASS
> +	depends on LEDS_CLASS_FLASH

Agree with the change for the greybus driver, one note, since you are
cleaning up this, maybe also remove the other #if down when setting
the flash specific operations, right?

#if IS_REACHABLE(CONFIG_LEDS_CLASS_FLASH)
/* Flash specific operations */
static int gb_lights_flash_intensity_set(struct led_classdev_flash *fcdev,
					 u32 brightness)
{
	struct gb_channel *channel = container_of(fcdev, struct gb_channel,
						  fled);
	int ret;
.
.
.
.

#else
static int gb_lights_channel_flash_config(struct gb_channel *channel)
{
	struct gb_connection *connection = get_conn_from_channel(channel);

	dev_err(&connection->bundle->dev, "no support for flash devices\n");
	return 0;
}
.
.
.


Thanks again.

Cheers,
    Rui

>  	help
>  	  Select this option if you have a device that follows the
>  	  Greybus LED Class specification.
> diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c
> index 87d36948c610..d62f97249aca 100644
> --- a/drivers/staging/greybus/light.c
> +++ b/drivers/staging/greybus/light.c
> @@ -29,13 +29,9 @@ struct gb_channel {
>  	struct attribute_group		*attr_group;
>  	const struct attribute_group	**attr_groups;
>  	struct led_classdev		*led;
> -#if IS_REACHABLE(CONFIG_LEDS_CLASS_FLASH)
>  	struct led_classdev_flash	fled;
>  	struct led_flash_setting	intensity_uA;
>  	struct led_flash_setting	timeout_us;
> -#else
> -	struct led_classdev		cled;
> -#endif
>  	struct gb_light			*light;
>  	bool				is_registered;
>  	bool				releasing;
> @@ -84,7 +80,6 @@ static bool is_channel_flash(struct gb_channel *channel)
>  				   | GB_CHANNEL_MODE_INDICATOR));
>  }
>  
> -#if IS_REACHABLE(CONFIG_LEDS_CLASS_FLASH)
>  static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
>  {
>  	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
> @@ -153,22 +148,6 @@ static int __gb_lights_flash_brightness_set(struct gb_channel *channel)
>  
>  	return __gb_lights_flash_intensity_set(channel, intensity);
>  }
> -#else
> -static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev)
> -{
> -	return container_of(cdev, struct gb_channel, cled);
> -}
> -
> -static struct led_classdev *get_channel_cdev(struct gb_channel *channel)
> -{
> -	return &channel->cled;
> -}
> -
> -static int __gb_lights_flash_brightness_set(struct gb_channel *channel)
> -{
> -	return 0;
> -}
> -#endif
>  
>  static int gb_lights_color_set(struct gb_channel *channel, u32 color);
>  static int gb_lights_fade_set(struct gb_channel *channel);
> diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
> index 612b4cab3819..36df927ec4b7 100644
> --- a/include/linux/led-class-flash.h
> +++ b/include/linux/led-class-flash.h
> @@ -85,7 +85,6 @@ static inline struct led_classdev_flash *lcdev_to_flcdev(
>  	return container_of(lcdev, struct led_classdev_flash, led_cdev);
>  }
>  
> -#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH)
>  /**
>   * led_classdev_flash_register_ext - register a new object of LED class with
>   *				     init data and with support for flash LEDs
> @@ -116,29 +115,6 @@ int devm_led_classdev_flash_register_ext(struct device *parent,
>  void devm_led_classdev_flash_unregister(struct device *parent,
>  					struct led_classdev_flash *fled_cdev);
>  
> -#else
> -
> -static inline int led_classdev_flash_register_ext(struct device *parent,
> -				    struct led_classdev_flash *fled_cdev,
> -				    struct led_init_data *init_data)
> -{
> -	return 0;
> -}
> -
> -static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {};
> -static inline int devm_led_classdev_flash_register_ext(struct device *parent,
> -				     struct led_classdev_flash *fled_cdev,
> -				     struct led_init_data *init_data)
> -{
> -	return 0;
> -}
> -
> -static inline void devm_led_classdev_flash_unregister(struct device *parent,
> -					struct led_classdev_flash *fled_cdev)
> -{};
> -
> -#endif  /* IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) */
> -
>  static inline int led_classdev_flash_register(struct device *parent,
>  					   struct led_classdev_flash *fled_cdev)
>  {
> diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h
> index 210d57bcd767..db9f34c6736e 100644
> --- a/include/linux/led-class-multicolor.h
> +++ b/include/linux/led-class-multicolor.h
> @@ -30,7 +30,6 @@ static inline struct led_classdev_mc *lcdev_to_mccdev(
>  	return container_of(led_cdev, struct led_classdev_mc, led_cdev);
>  }
>  
> -#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR)
>  /**
>   * led_classdev_multicolor_register_ext - register a new object of led_classdev
>   *				      class with support for multicolor LEDs
> @@ -64,34 +63,6 @@ int devm_led_classdev_multicolor_register_ext(struct device *parent,
>  
>  void devm_led_classdev_multicolor_unregister(struct device *parent,
>  					    struct led_classdev_mc *mcled_cdev);
> -#else
> -
> -static inline int led_classdev_multicolor_register_ext(struct device *parent,
> -					    struct led_classdev_mc *mcled_cdev,
> -					    struct led_init_data *init_data)
> -{
> -	return 0;
> -}
> -
> -static inline void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) {};
> -static inline int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
> -					       enum led_brightness brightness)
> -{
> -	return 0;
> -}
> -
> -static inline int devm_led_classdev_multicolor_register_ext(struct device *parent,
> -					  struct led_classdev_mc *mcled_cdev,
> -					  struct led_init_data *init_data)
> -{
> -	return 0;
> -}
> -
> -static inline void devm_led_classdev_multicolor_unregister(struct device *parent,
> -					    struct led_classdev_mc *mcled_cdev)
> -{};
> -
> -#endif  /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) */
>  
>  static inline int led_classdev_multicolor_register(struct device *parent,
>  					    struct led_classdev_mc *mcled_cdev)
> -- 
> 2.39.2

Re: [PATCH 2/2] leds: make flash and multicolor dependencies unconditional

[Greg Kroah-Hartman]

On Tue, Jan 09, 2024 at 10:06:40AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Along the same lines as making devm_led_classdev_register() declared
> extern unconditional, do the same thing for the two sub-classes
> that have similar stubs.
> 
> The users of these interfaces go to great lengths to allow building
> with both the generic leds API and the extended version, but realistically
> there is not much use in this, so just simplify it to always rely
> on it and remove the confusing fallback logic.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/leds/Kconfig                 |  4 ++--
>  drivers/leds/flash/Kconfig           |  4 ++--
>  drivers/staging/greybus/Kconfig      |  2 +-
>  drivers/staging/greybus/light.c      | 21 --------------------
>  include/linux/led-class-flash.h      | 24 -----------------------
>  include/linux/led-class-multicolor.h | 29 ----------------------------
>  6 files changed, 5 insertions(+), 79 deletions(-)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs

[Heiner Kallweit]

On 09.01.2024 10:06, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> These two functions have stub implementations that are called when
> NEW_LEDS and/or LEDS_CLASS are disabled, theorerically allowing drivers
> to optionally use the LED subsystem.
> 
> However, this has never really worked because a built-in driver is
> unable to link against these functions if the LED class is in a loadable
> module. Heiner ran into this problem with a driver that newly gained
> a LEDS_CLASS dependency and suggested using an IS_REACHABLE() check.
> 
> This is the reverse approach, removing the stub entirely to acknowledge
> that it is pointless in its current form, and that not having it avoids
> misleading developers into thinking that they can rely on it.
> 
> This survived around 1000 randconfig builds to validate that any callers
> of the interface already have the correct Kconfig dependency already,
> with the exception of the one that Heiner just added.
> 
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Link: https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@gmail.com/T/#u
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

For r8169 we have a Kconfig-based solution now, right. I had a brief look
at other drivers using LED functionality, and already the first one I looked
at seems to suffer from the same problem. input/keyboard/qt2160.c has the
following what should result in the same link error if qt2160 is built-in
and CONFIG_LEDS_CLASS=m. qt2160 has a Kconfig dependency only on I2C.

#ifdef CONFIG_LEDS_CLASS
static int qt2160_register_leds(struct qt2160_data *qt2160)
{
[...]
	error = devm_led_classdev_register(&client->dev, &led->cdev);
[...]		
}
#else

Personally I don't have a strong opinion pro/con IS_REACHABLE.
I see two options for now:
1. If outcome should be that stubs are a good thing, then the condition
   for the stub should make sure that the stub is chosen when needed.
   This would result in using IS_REACHABLE.
2. If stubs are removed (but also in the current situation, see example),
   then it seems some drivers need adding proper build dependencies.

Re: [PATCH 1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs

[Arnd Bergmann]

On Tue, Jan 9, 2024, at 11:10, Heiner Kallweit wrote:
> On 09.01.2024 10:06, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> These two functions have stub implementations that are called when
>> NEW_LEDS and/or LEDS_CLASS are disabled, theorerically allowing drivers
>> to optionally use the LED subsystem.
>> 
>> However, this has never really worked because a built-in driver is
>> unable to link against these functions if the LED class is in a loadable
>> module. Heiner ran into this problem with a driver that newly gained
>> a LEDS_CLASS dependency and suggested using an IS_REACHABLE() check.
>> 
>> This is the reverse approach, removing the stub entirely to acknowledge
>> that it is pointless in its current form, and that not having it avoids
>> misleading developers into thinking that they can rely on it.
>> 
>> This survived around 1000 randconfig builds to validate that any callers
>> of the interface already have the correct Kconfig dependency already,
>> with the exception of the one that Heiner just added.
>> 
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> Link: https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@gmail.com/T/#u
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>
> For r8169 we have a Kconfig-based solution now, right. I had a brief look
> at other drivers using LED functionality, and already the first one I looked
> at seems to suffer from the same problem. input/keyboard/qt2160.c has the
> following what should result in the same link error if qt2160 is built-in
> and CONFIG_LEDS_CLASS=m. qt2160 has a Kconfig dependency only on I2C.
>
> #ifdef CONFIG_LEDS_CLASS
> static int qt2160_register_leds(struct qt2160_data *qt2160)
> {
> [...]
> 	error = devm_led_classdev_register(&client->dev, &led->cdev);
> [...]		
> }
> #else

This is a bug, but I think a different one, with a similar effect.

Part of the problem in this driver is that it uses #ifdef instead
of "#if IS_ENABLED(CONFIG_LEDS_CLASS)". As a result, it just
never uses the LEDS when LEDS_CLASS=m, because that would
define CONFIG_LEDS_CLASS_MODULE but not CONFIG_LEDS_CLASS.

Changing it to IS_ENABLED() would cause the link failure
you describe, but would do it regardless of my change.

The same bug seems to be present in other files as well.

> 2. If stubs are removed (but also in the current situation, see example),
>    then it seems some drivers need adding proper build dependencies.

I don't see any driver that actually relies on the stub, since
that would only work a driver that can never be built-in.

If a driver can be built-in (like your r8169 code) and uses
the stub, we would have seen it fail to link in randconfig
kernels and added a LEDS_CLASS dependency.

     Arnd

Re: [PATCH 1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs

[Lee Jones]

On Tue, 09 Jan 2024 10:06:39 +0100, Arnd Bergmann wrote:
> These two functions have stub implementations that are called when
> NEW_LEDS and/or LEDS_CLASS are disabled, theorerically allowing drivers
> to optionally use the LED subsystem.
> 
> However, this has never really worked because a built-in driver is
> unable to link against these functions if the LED class is in a loadable
> module. Heiner ran into this problem with a driver that newly gained
> a LEDS_CLASS dependency and suggested using an IS_REACHABLE() check.
> 
> [...]

Applied, thanks!

[1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs
      commit: c0b64609dada48907b04b48873ba052efa8f121d
[2/2] leds: make flash and multicolor dependencies unconditional
      commit: 54602f38551e89b520611ffb9df05232d1bf73f8

--
Lee Jones [李琼斯]