Hi All, On 12/3/22 13:28, Hans de Goede wrote: > 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. I have spend today looking into the feasibility of using the LED class subsystem instead of modelling these on/off only LEDs as a GPIO. Good news, there is a LED_SYSFS_DISABLE flag which also stops userspace from messing with the trigger of the LED, so this first issue can easily be fixed. > 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? I have been looking into this, but atm the only way to tie a led-classdev to a device is through a fwnode reference. Since this is x86 where there is no DTS file where we can easily add this, I have been looking into doing this with swnode-s. LED directly attached to main SoC GPIO ====================================== For the simple LED is attached to a GPIO on the main SoC case, this requires 2 steps: 1. Have the INT3472 code register a LED classdev for the privacy-led instead of a GPIO lookup table entry. This LED classdev must have a swnode as fwnode, so that we can put a reference to that swnode in a "leds" reference-array property on the sensor i2c_client. This is about a 100 lines of extra code and seems fine / doable. 2. Add a "leds" reference-array property on the i2c_client device by adding a swnode with this property to the i2c_client device. This sounds straight forward (once we have the swnode for the LED class-device to point to) but this is actually not straight forward at all. There is a whole bunch of properties which needs to be added on the sensor to describe the media-graph between the sensor and the IPU, as well as what VCM (if any) is paired up with the sensor. These properties are all added through adding a swnode from the CSI bridge driver: drivers/media/pci/intel/ipu3/cio2-bridge.c But a device can only have one swnode added. So we cannot add a swnode to the sensor i2c_client in the INT3472 code. Instead the only thing which we could do is give the swnode for the privacy LED classdev a predictable name, derived from the sensor's device name and then have drivers/media/pci/intel/ipu3/cio2-bridge.c call software_node_find_by_name() to get the swnode and have it add the "leds" reference-array property on the i2c_client device for the sensor. However the INT3472 code is shared between multiple ISP/IPU implementations, so then we would need to duplicate this code for the other IPU versions (currently IPU6 which is out of tree), further complicating things. And this is for the direct usage of a SoC GPIO case. LED attached to TPS68470 PMIC indicator LED pin =============================================== In this case the LED-classdev should be instantiated by a driver for a new TPS68470 MFD cell. But this also introduces a bunch of probe ordering systems, so modelling things as a LED classdev here would involve: 1. Making the IN3472 code create + register a swnode for the LED classdev, this must be done here because of probe ordering. 2. Make the IN3472 code create a new TPS68470 MFD cell and pass the swnode as fwnode to this cell. 3. Write a new driver for the new TPS68470 MFD cell, which registers a LED classdev using the fwnode from the MFD cell as fwnode for the LED classdev. And this still does not solve the issue of how to get the privacy-LED as LED classdev model to work on the IPU6. Alternative approach ==================== An alternative approach, would be to add support for LED lookup tables to the LED class code (like we already have for GPIOs) and use this to allow tying a LED classdev to a struct device on non devicetree platforms. Given the problems with the swnode approach from above I believe that this would actually be better then the swnode approach. Lookup tables like this use device-names, so we don't need to have swnode-s ready for both the provider and the consumer at the time of adding the lookup table entry. Instead all that is necessary is to know the device-names of both the provider and the consumer which are both known in advance. Is this really worth all the trouble ? ====================================== So I really have to wonder what is using the LED classdev / framework actually buying us over using modelling these on/off only LEDs as a GPIO ? I know that some (x86) have a flash-LED for the back camera and given the experience with trying to tie a LED class dev to a specific struct device (to the sensor's i2c_client) I guess we are eventually going to need some sort of lookup tables for tying LED class devices to a specific device anyways. That and we want to avoid moving from the current approach (for some INT3472 using devices) of tying the privacy LED on/off to the INT3472 registered clk being enabled/disabled to modelling this as GPIOs, to then later modelling it as LED class devices after all. To avoid this double conversion issue I'm going to give the LED class route a second go, replacing the swnode approach which I tried today with a lookup-table approach. Regards, Hans > ### > > 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 >
