Hi Hans, On Mon, Dec 05, 2022 at 04:01:20PM +0100, Hans de Goede wrote: > On 12/3/22 13:28, Hans de Goede wrote: > > On 12/3/22 10:32, Linus Walleij wrote: > >> On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede 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. Thank you for all this research. > 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. Probably, but for those, the effort will pay off better, as we need to control the flash from userspace. For the privacy LED, I doubt we'll ever seen a system where we'll have to control it through more than an enable bit (for instance, an RGB or intensity-controlled privacy LED sounds very unlikely), so a GPIO is fine with me. > 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. > > > ### > > > > 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, Laurent Pinchart
