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
