Hi,
On 12/16/22 15:16, Andy Shevchenko wrote:
> 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?
Ack will fix for v4.
>
>> + 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(...);
That goes over 100 chars.
>
>> + }
>
> ...
>
>> + /* 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().
Please look more careful, quoting from the strreplace() docs:
* strreplace - Replace all occurrences of character in string.
Notice the *all* and we only want to replace the first ':' here,
because the ':' char has a special meaning in LED class-device-names.
>
> ...
>
>> +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?
Right.
>
>> + led_remove_lookup(&int3472->pled.lookup);
>
> With list_del_init() I believe the above check can be droped.
No it cannot, list_del_init() inside led_remove_lookup() would
protect against double led_remove_lookup() calls.
But here we may have a completely uninitialized list_head on
devices without an INT3472 privacy-led, which will trigger
either __list_del_entry_valid() errors or lead to NULL
pointer derefs.
>
>> + led_classdev_unregister(&int3472->pled.classdev);
>> + gpiod_put(int3472->pled.gpio);
>> +}
>
Regards.
Hans
