Re: [PATCH v7 3/4] platform/x86: int3472: Parameterize LED con_id in registration

[Hans de Goede]

Hi,

On 31-Mar-26 15:44, Marco Nenciarini wrote:
> Add a con_id parameter to skl_int3472_register_led() to allow callers
> to specify both the LED name suffix and lookup con_id instead of
> hardcoding "privacy". This prepares for registering additional LED
> types with different names.
> 
> While at it, rename the privacy LED's GPIO con_id from "privacy-led"
> to "privacy" in int3472_get_con_id_and_polarity() and pass it
> directly to skl_int3472_register_led(), reducing churn when adding
> new LED types.
> 
> No functional change.
> 
> Signed-off-by: Marco Nenciarini <mnencia@xxxxxxxx>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>

Regards,

Hans



> ---
> 
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>  drivers/platform/x86/intel/int3472/discrete.c | 4 ++--
>  drivers/platform/x86/intel/int3472/led.c      | 7 ++++---
>  include/linux/platform_data/x86/int3472.h     | 3 ++-
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index cb24763..a45a930 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -212,7 +212,7 @@ static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3
>  		*gpio_flags = GPIO_ACTIVE_HIGH;
>  		break;
>  	case INT3472_GPIO_TYPE_PRIVACY_LED:
> -		*con_id = "privacy-led";
> +		*con_id = "privacy";
>  		*gpio_flags = GPIO_ACTIVE_HIGH;
>  		break;
>  	case INT3472_GPIO_TYPE_HOTPLUG_DETECT:
> @@ -348,7 +348,7 @@ 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);
> +			ret = skl_int3472_register_led(int3472, gpio, con_id);
>  			if (ret)
>  				err_msg = "Failed to register LED\n";
>  
> diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
> index fe412cb..22d0d6c 100644
> --- a/drivers/platform/x86/intel/int3472/led.c
> +++ b/drivers/platform/x86/intel/int3472/led.c
> @@ -14,7 +14,8 @@ static int int3472_led_set(struct led_classdev *led_cdev, enum led_brightness br
>  	return 0;
>  }
>  
> -int skl_int3472_register_led(struct int3472_discrete_device *int3472, struct gpio_desc *gpio)
> +int skl_int3472_register_led(struct int3472_discrete_device *int3472, struct gpio_desc *gpio,
> +			     const char *con_id)
>  {
>  	struct int3472_led *led = &int3472->led;
>  	char *p;
> @@ -27,7 +28,7 @@ int skl_int3472_register_led(struct int3472_discrete_device *int3472, struct gpi
>  
>  	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
>  	snprintf(led->name, sizeof(led->name),
> -		 "%s::privacy_led", acpi_dev_name(int3472->sensor));
> +		 "%s::%s_led", acpi_dev_name(int3472->sensor), con_id);
>  	p = strchr(led->name, ':');
>  	if (p)
>  		*p = '_';
> @@ -42,7 +43,7 @@ int skl_int3472_register_led(struct int3472_discrete_device *int3472, struct gpi
>  
>  	led->lookup.provider = led->name;
>  	led->lookup.dev_id = int3472->sensor_name;
> -	led->lookup.con_id = "privacy";
> +	led->lookup.con_id = con_id;
>  	led_add_lookup(&led->lookup);
>  
>  	return 0;
> diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
> index 7af6731..3ba0d56 100644
> --- a/include/linux/platform_data/x86/int3472.h
> +++ b/include/linux/platform_data/x86/int3472.h
> @@ -160,7 +160,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>  				   const char *second_sensor);
>  void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472);
>  
> -int skl_int3472_register_led(struct int3472_discrete_device *int3472, struct gpio_desc *gpio);
> +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);
>  
>  #endif