Re: [PATCH v7 1/4] platform/x86: int3472: Use local variable for LED struct access

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

Hi,

On 31-Mar-26 15:44, Marco Nenciarini wrote:
> Introduce a local struct int3472_pled pointer in the LED registration,
> unregistration, and brightness callback functions to avoid repeatedly
> dereferencing int3472->pled. In the brightness callback, use
> container_of() to get the int3472_pled struct directly instead of
> going through int3472_discrete_device.
> 
> No functional change.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Marco Nenciarini <mnencia@kcore.it>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>  drivers/platform/x86/intel/int3472/led.c | 43 ++++++++++++------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c
> index b1d84b9..35abad9 100644
> --- a/drivers/platform/x86/intel/int3472/led.c
> +++ b/drivers/platform/x86/intel/int3472/led.c
> @@ -6,55 +6,56 @@
>  #include <linux/leds.h>
>  #include <linux/platform_data/x86/int3472.h>
>  
> -static int int3472_pled_set(struct led_classdev *led_cdev,
> -				     enum led_brightness brightness)
> +static int int3472_pled_set(struct led_classdev *led_cdev, enum led_brightness brightness)
>  {
> -	struct int3472_discrete_device *int3472 =
> -		container_of(led_cdev, struct int3472_discrete_device, pled.classdev);
> +	struct int3472_pled *led = container_of(led_cdev, struct int3472_pled, classdev);
>  
> -	gpiod_set_value_cansleep(int3472->pled.gpio, brightness);
> +	gpiod_set_value_cansleep(led->gpio, brightness);
>  	return 0;
>  }
>  
>  int skl_int3472_register_pled(struct int3472_discrete_device *int3472, struct gpio_desc *gpio)
>  {
> +	struct int3472_pled *led = &int3472->pled;
>  	char *p;
>  	int ret;
>  
> -	if (int3472->pled.classdev.dev)
> +	if (led->classdev.dev)
>  		return -EBUSY;
>  
> -	int3472->pled.gpio = gpio;
> +	led->gpio = gpio;
>  
>  	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
> -	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
> +	snprintf(led->name, sizeof(led->name),
>  		 "%s::privacy_led", acpi_dev_name(int3472->sensor));
> -	p = strchr(int3472->pled.name, ':');
> +	p = strchr(led->name, ':');
>  	if (p)
>  		*p = '_';
>  
> -	int3472->pled.classdev.name = int3472->pled.name;
> -	int3472->pled.classdev.max_brightness = 1;
> -	int3472->pled.classdev.brightness_set_blocking = int3472_pled_set;
> +	led->classdev.name = led->name;
> +	led->classdev.max_brightness = 1;
> +	led->classdev.brightness_set_blocking = int3472_pled_set;
>  
> -	ret = led_classdev_register(int3472->dev, &int3472->pled.classdev);
> +	ret = led_classdev_register(int3472->dev, &led->classdev);
>  	if (ret)
>  		return ret;
>  
> -	int3472->pled.lookup.provider = int3472->pled.name;
> -	int3472->pled.lookup.dev_id = int3472->sensor_name;
> -	int3472->pled.lookup.con_id = "privacy";
> -	led_add_lookup(&int3472->pled.lookup);
> +	led->lookup.provider = led->name;
> +	led->lookup.dev_id = int3472->sensor_name;
> +	led->lookup.con_id = "privacy";
> +	led_add_lookup(&led->lookup);
>  
>  	return 0;
>  }
>  
>  void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
>  {
> -	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
> +	struct int3472_pled *led = &int3472->pled;
> +
> +	if (IS_ERR_OR_NULL(led->classdev.dev))
>  		return;
>  
> -	led_remove_lookup(&int3472->pled.lookup);
> -	led_classdev_unregister(&int3472->pled.classdev);
> -	gpiod_put(int3472->pled.gpio);
> +	led_remove_lookup(&led->lookup);
> +	led_classdev_unregister(&led->classdev);
> +	gpiod_put(led->gpio);
>  }