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
