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

Andy Shevchenko
2022-12-16T14:16:35Z

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