Re: [PATCH v5 0/4] platform/x86: int3472: Add support for GPIO type 0x02 (strobe)

[johannes . goede]

Hi,

On 31-Mar-26 23:28, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Mar 31, 2026 at 12:15:55PM +0200, johannes.goede@xxxxxxxxxxxxxxxx wrote:
>> Hi Sakari,
>>
>> On 30-Mar-26 22:21, Sakari Ailus wrote:
>>> Hi Hans, Marco,
>>>
>>> On Mon, Mar 30, 2026 at 05:12:21PM +0200, johannes.goede@xxxxxxxxxxxxxxxx wrote:
>>>> Hi,
>>>>
>>>> On 30-Mar-26 16:55, Marco Nenciarini wrote:
>>>>> Hi Hans,
>>>>>
>>>>> Thank you for the detailed feedback.
>>>>>
>>>>>> My main remark on the current patch set is that IMHO
>>>>>> the LED name really should be: OVTIxxxx:00::ir_flood_led
>>>>>> since that is what it actually does, strobe is typically
>>>>>> related to flash LEDs which despite the naming in Intel's
>>>>>> side this is not.
>>>>>
>>>>> The series actually started with "ir_flood" in v1-v2. It was renamed to
>>>>> "strobe" in v3 to match the GPIO type name used in Intel's ACPI _DSM
>>>>> tables. But I agree with you that the userspace-visible LED name should
>>>>> describe what the hardware actually does, not mirror an internal ACPI
>>>>> label. I am happy to go back to "ir_flood" for the LED name.
>>>>>
>>>>> We could keep INT3472_GPIO_TYPE_STROBE for the define (matching the ACPI
>>>>> type value) but pass "ir_flood" as the con_id to the LED registration,
>>>>> so userspace sees OVTIxxxx:00::ir_flood_led. Would that work for
>>>>> everyone?
>>>>
>>>> That sounds good to me.
>>>
>>> Seems reasonable.
>>>
>>>>
>>>>>> We really need to get some input from the V4L2 maintainers
>>>>>> here on if this is a good idea, before merging this series.
>>>>>
>>>>> Agreed.
>>>>>
>>>>>> I can even imagine
>>>>>> a default simple mode where the v4l2-core just turns
>>>>>> on the LED when streaming starts and off again when
>>>>>> streaming stops (very much like the privacy LED) and
>>>>>> then in the future maybe we can extend this, e.g.
>>>>>> add a control on the sensor device to make this
>>>>>> configurable ?
>>>>>
>>>>> That makes a lot of sense. The current series intentionally keeps things
>>>>> minimal (just exposing the LED under /sys/class/leds with no V4L2
>>>>> integration), but this future direction sounds right.
>>>>>
>>>>> I will hold off on sending v6 until we have agreement on the naming and
>>>>> Sakari has had a chance to weigh in.
>>>>
>>>> Ack, lets wait for input from Sakari here.
>>>
>>> I wonder if there would be any deterministic ways to find the LED device
>>> based on the sensor. It'd probably require more information via MC / V4L2
>>> controls to allow that.
>>
>> Yes, the int3472 code adds a LED lookup table entry with the consumer-
>> device being the sensor.
>>
>> So just like v4l2-subdev.c currently does:
>>
>> 	sd->privacy_led = led_get(sd->dev, "privacy");
>>
>> it will also be able to do:
>>
>> 	sd->privacy_led = led_get(sd->dev, "ir_flood");
>>
>>> Alternatively we could use a boolean control for this, but I think I'd
>>> avoid adding that now and rely on LED API instead.
>>>
>>> Are there use cases for this LED, apart from Windows Hello? :-)
>>
>> As mentioned for now we could just treat it exactly the same
>> as the privacy LED and simply turn it on while streaming
>> inside the v4l2-core / v4l2-subdev code.
>>
>> This would nicely cover the face ID use-case for which
>> Linux implementations exist to.
>>
>> And then later of some other use-case comes up we could
>> make this a menu-control with on/off/auto values, defaulting
>> to auto which would preserve the turn it on while streaming
>> behavior.
>>
>> We do need to come up with something here, since use-cases
>> like Howdy (Linux face-id) will need it. My vote goes to
>> give it the same treatment as the privacy LED in the v4l2-core
>> for now making things "just work".
>>
>> Alternatively we could document that userspace needs to
>> find the LED and turn it on itself through the LED classdev
>> but that will be painful for userspace to do for various
>> reasons including classdev access requiring root rights.
> 
> That was actually my concern as well.
> 
> Still, if we bind this to the privacy LED now

To be clear my suggestion is to treat it the same as we
currently treat privacy-LEDs (auto-on while streaming) but
have a separate code-path, or at least a separate LED name
("ir_flood" vs "privacy") to allow different behavior in
the future.

> and the LED will effectively
> be powered whenever the camera is streaming,

Ack.

> we can't drop that interaction
> in the future as doing so would break existing users.

Ack.

> I'm not sure if there would ever be a need though.

Ack.

> Either way, I'm starting to feel the V4L2 control might actually be the
> best way to go from here.

I think we can postpone adding a ctrl to if a use-case needing
different behavior ever shows up (which is unlikely?) .

This ctrl can then be a menu ctrl with auto/on/off values
defaulting to auto and auto preserving the on-while-streaking
behavior, so not breaking existing users.

TL;DR: my proposal is:

1. Name the new LED classdev ir_flood[_led] including
   adding an "ir_flood" LED lookup with the sensor as
   consuming device of the LED.

2. Make v4l2-core turn the ir_flood on automatically while
   streaming, just like it does for a "privacy" LED currently
   (mostly re-using the existing privacy LED code).

3. If in the future userspace needs more fine-grained control
   at a ir_flood V4L2 menu control on the sensor with
   auto/on/off values, defaulting to auto which preserves
   the behavior from 2.

Regards,

Hans