Re: [PATCH v6 4/4] platform/x86: int3472: Add support for GPIO type 0x02 (IR flood LED)

[Hans de Goede <johannes.goede@oss.qualcomm.com>]

Hi Marco,

On 31-Mar-26 09:52, Marco Nenciarini wrote:
> Add support for GPIO type 0x02, which controls an IR flood LED used
> for face authentication on some laptops (e.g. Dell Pro Max 16 Premium).
> 
> Without this patch, the kernel logs "GPIO type 0x02 unknown; the sensor
> may not work" and IR sensors paired with a flood LED cannot function.
> 
> The flood LED is registered through the LED subsystem like the existing
> privacy LED. Unlike the privacy LED, it does not have a lookup entry
> since there is no consumer driver expecting it via led_get().
> 
> To support multiple LEDs per INT3472 device, convert the single led
> struct member to an array with a counter.
> 
> Signed-off-by: Marco Nenciarini <mnencia@kcore.it>
> ---
> 
> The ACPI _DSM tables refer to this GPIO type as "strobe", hence the
> INT3472_GPIO_TYPE_STROBE define. The userspace-visible LED name uses
> "ir_flood" instead, as the hardware is an IR flood illuminator, not a
> flash strobe.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>  drivers/platform/x86/intel/int3472/discrete.c | 16 +++++++-
>  drivers/platform/x86/intel/int3472/led.c      | 39 +++++++++++--------
>  include/linux/platform_data/x86/int3472.h     | 10 +++--
>  3 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 57c3b2c..78111e5 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -215,6 +215,10 @@ static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3
>  		*con_id = "privacy-led";

How about changing this to "privacy" (this is only
used internally really, so we're free to change it).

Note this should already be done in patch 3/4
to reduce churn below.

>  		*gpio_flags = GPIO_ACTIVE_HIGH;
>  		break;
> +	case INT3472_GPIO_TYPE_STROBE:
> +		*con_id = "ir_flood";

and here keep using "ir_flood" / keep as is.

> +		*gpio_flags = GPIO_ACTIVE_HIGH;
> +		break;
>  	case INT3472_GPIO_TYPE_HOTPLUG_DETECT:
>  		*con_id = "hpd";
>  		*gpio_flags = GPIO_ACTIVE_HIGH;
> @@ -248,6 +252,7 @@ static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3
>   *
>   * 0x00 Reset
>   * 0x01 Power down
> + * 0x02 Strobe
>   * 0x0b Power enable
>   * 0x0c Clock enable
>   * 0x0d Privacy LED
> @@ -331,6 +336,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  		break;
>  	case INT3472_GPIO_TYPE_CLK_ENABLE:
>  	case INT3472_GPIO_TYPE_PRIVACY_LED:
> +	case INT3472_GPIO_TYPE_STROBE:
>  	case INT3472_GPIO_TYPE_POWER_ENABLE:
>  	case INT3472_GPIO_TYPE_HANDSHAKE:
>  		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
> @@ -348,7 +354,13 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  
>  			break;
>  		case INT3472_GPIO_TYPE_PRIVACY_LED:
> -			ret = skl_int3472_register_led(int3472, gpio, "privacy");
> +			ret = skl_int3472_register_led(int3472, gpio, "privacy", true);
> +			if (ret)
> +				err_msg = "Failed to register LED\n";
> +
> +			break;
> +		case INT3472_GPIO_TYPE_STROBE:
> +			ret = skl_int3472_register_led(int3472, gpio, "ir_flood", false);

and then here instead of having 2 almost identical cases just pass con_id
note please already use con_id here in patch 3/4 to avoid churn.

After changing that in 3/4 you only need to add a single:

+		case INT3472_GPIO_TYPE_STROBE:

here to make it also handle the strobe/ir_flood case.

As an added bonus you've already named the new skl_int3472_register_led()
parameter con_id, so now that will nicely line-up too :)

Regards,

Hans



>  			if (ret)
>  				err_msg = "Failed to register LED\n";
>  
> @@ -422,7 +434,7 @@ void int3472_discrete_cleanup(struct int3472_discrete_device *int3472)
>  	gpiod_remove_lookup_table(&int3472->gpios);
>  
>  	skl_int3472_unregister_clock(int3472);
> -	skl_int3472_unregister_led(int3472);
> +	skl_int3472_unregister_leds(int3472);
>  	skl_int3472_unregister_regulator(int3472);
>  }
>  EXPORT_SYMBOL_NS_GPL(int3472_discrete_cleanup, "INTEL_INT3472_DISCRETE");
> diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
> index 39466d4..e7cd144 100644
> --- a/drivers/platform/x86/intel/int3472/led.c
> +++ b/drivers/platform/x86/intel/int3472/led.c
> @@ -4,6 +4,7 @@
>  #include <linux/acpi.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/leds.h>
> +#include <linux/list.h>
>  #include <linux/platform_data/x86/int3472.h>
>  
>  static int int3472_led_set(struct led_classdev *led_cdev,
> @@ -16,16 +17,19 @@ static int int3472_led_set(struct led_classdev *led_cdev,
>  }
>  
>  int skl_int3472_register_led(struct int3472_discrete_device *int3472,
> -			     struct gpio_desc *gpio, const char *con_id)
> +			     struct gpio_desc *gpio, const char *con_id,
> +			     bool add_lookup)
>  {
> -	struct int3472_led *led = &int3472->led;
> +	struct int3472_led *led;
>  	char *p;
>  	int ret;
>  
> -	if (led->classdev.dev)
> -		return -EBUSY;
> +	if (int3472->n_leds >= INT3472_MAX_LEDS)
> +		return -ENOSPC;
>  
> +	led = &int3472->leds[int3472->n_leds];
>  	led->gpio = gpio;
> +	INIT_LIST_HEAD(&led->lookup.list);
>  
>  	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
>  	snprintf(led->name, sizeof(led->name),
> @@ -42,22 +46,25 @@ int skl_int3472_register_led(struct int3472_discrete_device *int3472,
>  	if (ret)
>  		return ret;
>  
> -	led->lookup.provider = led->name;
> -	led->lookup.dev_id = int3472->sensor_name;
> -	led->lookup.con_id = con_id;
> -	led_add_lookup(&led->lookup);
> +	if (add_lookup) {
> +		led->lookup.provider = led->name;
> +		led->lookup.dev_id = int3472->sensor_name;
> +		led->lookup.con_id = con_id;
> +		led_add_lookup(&led->lookup);
> +	}
>  
> +	int3472->n_leds++;
>  	return 0;
>  }
>  
> -void skl_int3472_unregister_led(struct int3472_discrete_device *int3472)
> +void skl_int3472_unregister_leds(struct int3472_discrete_device *int3472)
>  {
> -	struct int3472_led *led = &int3472->led;
> +	for (unsigned int i = 0; i < int3472->n_leds; i++) {
> +		struct int3472_led *led = &int3472->leds[i];
>  
> -	if (IS_ERR_OR_NULL(led->classdev.dev))
> -		return;
> -
> -	led_remove_lookup(&led->lookup);
> -	led_classdev_unregister(&led->classdev);
> -	gpiod_put(led->gpio);
> +		if (!list_empty(&led->lookup.list))
> +			led_remove_lookup(&led->lookup);
> +		led_classdev_unregister(&led->classdev);
> +		gpiod_put(led->gpio);
> +	}
>  }
> diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
> index 5213911..7366a25 100644
> --- a/include/linux/platform_data/x86/int3472.h
> +++ b/include/linux/platform_data/x86/int3472.h
> @@ -23,6 +23,7 @@
>  /* PMIC GPIO Types */
>  #define INT3472_GPIO_TYPE_RESET					0x00
>  #define INT3472_GPIO_TYPE_POWERDOWN				0x01
> +#define INT3472_GPIO_TYPE_STROBE				0x02
>  #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
>  #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
>  #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
> @@ -31,6 +32,7 @@
>  
>  #define INT3472_PDEV_MAX_NAME_LEN				23
>  #define INT3472_MAX_SENSOR_GPIOS				3
> +#define INT3472_MAX_LEDS					2
>  #define INT3472_MAX_REGULATORS					3
>  
>  /* E.g. "avdd\0" */
> @@ -126,11 +128,12 @@ struct int3472_discrete_device {
>  		struct led_lookup_data lookup;
>  		char name[INT3472_LED_MAX_NAME_LEN];
>  		struct gpio_desc *gpio;
> -	} led;
> +	} leds[INT3472_MAX_LEDS];
>  
>  	struct int3472_discrete_quirks quirks;
>  
>  	unsigned int ngpios; /* how many GPIOs have we seen */
> +	unsigned int n_leds; /* how many LEDs have we registered */
>  	unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
>  	unsigned int n_regulator_gpios; /* how many have we mapped to a regulator */
>  	struct gpiod_lookup_table gpios;
> @@ -161,7 +164,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>  void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472);
>  
>  int skl_int3472_register_led(struct int3472_discrete_device *int3472,
> -			     struct gpio_desc *gpio, const char *con_id);
> -void skl_int3472_unregister_led(struct int3472_discrete_device *int3472);
> +			     struct gpio_desc *gpio, const char *con_id,
> +			     bool add_lookup);
> +void skl_int3472_unregister_leds(struct int3472_discrete_device *int3472);
>  
>  #endif