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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

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.
>