Hi, On 12/5/22 22:26, Laurent Pinchart wrote: > 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. Ok, that is good to know. Going the GPIO route would be a lot more KISS, so that has my preference too. Linus would modelling these simple on/off LEDs as GPIOs be acceptable to you too ? Regards, Hans > >> 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. >
