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

[johannes . goede]

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.

Regards,

Hans