Re: [PATCH v3 08/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

[Andy Shevchenko <andy@xxxxxxxxxx>]

On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote:
> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
> X1 Nano gen 2 there is no clock-enable pin, triggering the:
> "No clk GPIO. The privacy LED won't work" warning and causing the privacy
> LED to not work.
> 
> Fix this by modeling the privacy LED as a LED class device rather then
> integrating it with the registered clock.
> 
> Note this relies on media subsys changes to actually turn the LED on/off
> when the sensor's v4l2_subdev's s_stream() operand gets called.

...

> +	struct int3472_pled {
> +		char name[INT3472_LED_MAX_NAME_LEN];
> +		struct led_lookup_data lookup;

> +		struct led_classdev classdev;

Why not putting this as a first member in the struct, so any container_of()
against it become no-op at compile time?

> +		struct gpio_desc *gpio;
> +	} pled;

...

> +	if (IS_ERR(int3472->pled.gpio)) {
> +		ret = PTR_ERR(int3472->pled.gpio);
> +		return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n");

	return dev_err_probe(...);

> +	}

...

> +	/* Generate the name, replacing the ':' in the ACPI devname with '_' */
> +	snprintf(int3472->pled.name, sizeof(int3472->pled.name),
> +		 "%s::privacy_led", acpi_dev_name(int3472->sensor));

> +	for (i = 0; int3472->pled.name[i]; i++) {
> +		if (int3472->pled.name[i] == ':') {
> +			int3472->pled.name[i] = '_';
> +			break;
> +		}
> +	}

NIH strreplace().

...

> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472)
> +{
> +	if (IS_ERR_OR_NULL(int3472->pled.classdev.dev))
> +		return;

This dups the check inside the _unregister() below, right?

> +	led_remove_lookup(&int3472->pled.lookup);

With list_del_init() I believe the above check can be droped.

> +	led_classdev_unregister(&int3472->pled.classdev);
> +	gpiod_put(int3472->pled.gpio);
> +}

-- 
With Best Regards,
Andy Shevchenko