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

  Hi All,

While working on the earlier discussed changes for dealing with
the privacy LED for "discrete" INT3472 ACPI devices I was wondering
"how are we dealing with the privacy LED when the INT3472 ACPI device
is a tps68470?".

Well it turns out we were not dealing with this at all, leading to
the privacy LED on the back of the Surface Go series not lighting up
when the back camera is on.

This series fixes this, it consists of:

Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
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.

There is one small issue here, I believe that patches 1-3 need to land before
4 + 5 do. Once 4 + 5 have landed the ov8865 driver will try to get a
GPIO with pin number 10 from the gpio-tps68470 provider and without patch 3,
that will fail because only pins 0-9 exist until patch 3 lands.

The easiest way to avoid this issue would be for me to merge patches 1-3 +
5 through the pdx86 tree. GPIO subsystem maintainers, may I have your ack
for this ?

Note patch 4 is not a problem without patch 5, it uses gpiod_get_optional,
so as long as there is no lookup entry for a "privacy-led" GPIO it is
a no-op.

Regards,

Hans



Hans de Goede (5):
  gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong
    register
  gpio: tps68470: Make tps68470_gpio_output() always set the initial
    value
  gpio: tps68470: Add support for the indicator LED outputs
  media: ov8865: Add support for a privacy-led GPIO
  platform/x86: int3472: Add support for the back privacy LED on Surface
    Go models

 drivers/gpio/gpio-tps68470.c                  | 52 +++++++++++++------
 drivers/media/i2c/ov8865.c                    |  9 ++++
 .../x86/intel/int3472/tps68470_board_data.c   |  1 +
 include/linux/mfd/tps68470.h                  |  4 ++
 4 files changed, 49 insertions(+), 17 deletions(-)
  

On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede <hdegoede@redhat.com> 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?

IIUC the leds subsystem has gained support for leds as resources.

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.

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.

Yours,
Linus Walleij
  

Hi,

On 12/3/22 10:32, Linus Walleij wrote:
> On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com/

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
  

Hi Hans,

On Mon, Nov 28, 2022 at 10:44:03PM +0100, Hans de Goede wrote:
> Hi All,
> 
> While working on the earlier discussed changes for dealing with
> the privacy LED for "discrete" INT3472 ACPI devices I was wondering
> "how are we dealing with the privacy LED when the INT3472 ACPI device
> is a tps68470?".
> 
> Well it turns out we were not dealing with this at all, leading to
> the privacy LED on the back of the Surface Go series not lighting up
> when the back camera is on.

The tps68470 on ACPI was originally only supported via the OP region
interface and on all of those systems the privacy LED was hard wired. So
the LED outputs were not supported. Thanks for adding them.

> 
> This series fixes this, it consists of:
> 
> Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
> 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.
> 
> There is one small issue here, I believe that patches 1-3 need to land before
> 4 + 5 do. Once 4 + 5 have landed the ov8865 driver will try to get a
> GPIO with pin number 10 from the gpio-tps68470 provider and without patch 3,
> that will fail because only pins 0-9 exist until patch 3 lands.
> 
> The easiest way to avoid this issue would be for me to merge patches 1-3 +
> 5 through the pdx86 tree. GPIO subsystem maintainers, may I have your ack
> for this ?
> 
> Note patch 4 is not a problem without patch 5, it uses gpiod_get_optional,
> so as long as there is no lookup entry for a "privacy-led" GPIO it is
> a no-op.

For the patches:

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Although I'd still like to see another, better scaling solution for
controlling privacy LEDs.
  

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@redhat.com> 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@redhat.com/
> 
> 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
>
  

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@redhat.com/
> > 
> > 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.
  

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@redhat.com/
>>>
>>> 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.
>
  

On Mon, Dec 5, 2022 at 4:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

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

I think this looks like a good idea.

We attach other resources such as clocks, regulators,
DMA channels, GPIOs exactly this way. So why not LEDs?

As GPIO maintainer I every now and then get a suggestion
like this to "just let this pass as a GPIO because it makes
my life so much easier", but it is the same curse as the
input subsystem has: it is versatile and well engineered so
it starts to look like a golden hammer (everything start to
look like nails).

But we are two GPIO maintainers and right now Bartosz
does the majority of the work, and if he thinks it's the best
idea ever I will certainly reconsider.

Yours,
Linus Walleij
  

Hi,

On 12/7/22 01:25, Linus Walleij wrote:
> On Mon, Dec 5, 2022 at 4:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> 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.
> 
> I think this looks like a good idea.
> 
> We attach other resources such as clocks, regulators,
> DMA channels, GPIOs exactly this way. So why not LEDs?
> 
> As GPIO maintainer I every now and then get a suggestion
> like this to "just let this pass as a GPIO because it makes
> my life so much easier", but it is the same curse as the
> input subsystem has: it is versatile and well engineered so
> it starts to look like a golden hammer (everything start to
> look like nails).
> 
> But we are two GPIO maintainers and right now Bartosz
> does the majority of the work, and if he thinks it's the best
> idea ever I will certainly reconsider.

Ok, I will give the LED lookup table approach a go and
post a new series replacing this one when its ready.

Feel free to drop this series from your queue.

(Note given that that is going to be a whole new approach
I plan to start the new series at v1 again).

Regards,

Hans