Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 12/3/22 10:32, Linus Walleij wrote:
> On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> 
>> Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
> 
> Good, please merge this.
> 
>> Patch3:      Add support for the indicator LED outputs on the tps68470 as GPIOs
>> Patch4:      Add support for a privacy LED to the ov8865 sensor driver
>> Patch5:      Add gpio-lookup table entry for the privacy LED.
> 
> OK so I have to call out the hippo in the room:
> 
> these "gpios" are not called "gpios" anywhere else than in this
> patch. General purpose input/output, remember. These are special
> purpose LED control registers.
> 
> So can you provide a good explanation why these registers aren't
> handled in the drivers/led subsystem instead?

This was discussed in another thread:

https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@xxxxxxxxxx/

There were 2 problems identified which has lead to the current
direction of just modelling at as an (output only) GPIO:

1. The LED class allows userspace control of the LED which is
bad from a privacy pov. This will make it easy, too easy
(was the conclusion) for spy-ware to turn on the camera without
the LED turning on.

Later in the thread it was pointed out that there is a flag to
suspend userspace control, we could use this to permanently disable
userspace control which I guess would be some what of a solution,
although we would then also need to look into disallow changing
triggers, because I think those could still be used as a way around
this.

2. GPIO(s) can be tied directly to the device so that on a device
with both front and back privacy-LEDs (real world example) doing
 gpiod_get(dev, "privacy-led") gets us the right privacy-led,
where as with LED class devices tying the sensor and LED class
device is going to be tricky.

> IIUC the leds subsystem has gained support for leds as resources.

Interesting that would mitigate problem 2 from above and since
people keep circling back to "its a LED please use the LED class",
this is definitely worth looking into.

Do you have any pointers / examples about led class devices as
resources?

###

Note though that these indicator LED outputs, both functionality
wise as well as at the register level of this PMIC only support
turning them on/off. So this maps pretty well to the GPIO subsystem
and all the functionality of the LED class subsystem is mostly
functionality which we want to avoid since we don't want userspace
control, neither directly through sysfs or by attaching triggers.

So this does map pretty well to just modelling it as a GPIO,
if we model this as a LED then we end up having to workaround
a bunch of stuff the LED subsytem does which we do not want in
this case. And this may even require patches to the LED subsystem
to disallow userspace changing the trigger (I would need to check).

So from my pov modelling this as an output-only GPIO pin is
actually a more KISS solution then involving the LED subsystem...

> I don't mind a LED driver inside of the GPIO driver if that is what
> it takes as a compromise, just that it should be handled by the right
> subsystem.

The PMIC already is a MFD device, so if we go the LED class route
we can just add a separate MFD child device for the new LED driver
to bind to.

> Given that flash leds which are used by cameras are already in
> drivers/leds/flash this should be no different and there will be more
> cameras with these privacy leds.

Actually this patch is for the back camera privacy LED on a
Microsoft Surface Go tablet. The front camera privacy LED is
directly attached to a GPIO of the main SoC. So for that camera
just adding a GPIO lookup table entry to map the ACPI provided
GPIO info to a "privacy-led" GPIO on the sensor i2c_client device
(which we already do for the "reset" and "powerdown" gpios) also
by far is the most KISS approach.

Doing things this way in the code translating the ACPI "magic"
to standard Linux device-model stuff is literary a single line
of code (add an extra case: to an existing list of cases in a
switch-case). Where as instantiating a LED class device for this
and then somehow tying that to the i2c_client for the sensor will
be more code.

So again treating these on/off only LEDs, where we want to
*disallow* userspace control, as a GPIO is by far the most KISS
solution.

Regards,

Hans




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux