[0/3] platform/x86: int3472/discrete: Make it work with IPU6

  Hi All,

Here is a small set of patches to make the int3472/discrete code
work with the sensor drivers bundled with the (unfortunately out of tree)
IPU6 driver.

There are parts of the out of tree IPU6 code, like the sensor drivers,
which can be moved to the mainline and I do plan to work on this at some
point and then some of this might need to change. But for now the goal is
to make the out of tree driver work with standard mainline distro kernels
through e.g. dkms. Otherwise users need to run a patched kernel just for
a couple of small differences.

This is basically a rewrite of this patch:
https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch

Wich users who want to use the IPU6 driver so far have had to manually
apply to their kernels which is quite inconvenient.

This rewrite makes 2 significant changes:

1. Don't break things on IPU3 platforms

2. Instead of extending the int3472_sensor_configs[] quirks table for each
model which needs "clken" and "pled" GPIOs, do this based on matching
the ACPI HID of the ACPI device describing the sensor.

The need for these GPIOs is a property of the specific sensor driver which
binds using this same HID, so by using this we avoid having to extend the
int3472_sensor_configs[] quirks table all the time.

This allows roling back the behavior to at least use a clk-framework
clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
the sensor drivers, assuming that the drivers are switched over to the
clk framework as part of their mainlining.

A bigger question is what to do with the privacy-led GPIO on IPU3
we so far have turned the LED on/off at the same as te clock,
but at least on some IPU6 models this won't work, because they only
have a privacy-led GPIO and no clk_en GPIO (there is no sensor
clk-control at all on some models).

I think we should maybe move all models, including IPU3 based
models over to using a normal GPIO for controlling the privacy-led
to make things consistent.

And likewise (eventually) completely drop the "clken" GPIO this
patch series introduces (with some sensors) and instead always model
this through the clk-framework.

Regards,

Hans


Hans de Goede (3):
  platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
  platform/x86: int3472/discrete: Get the polarity from the _DSM entry
  platform/x86: int3472/discrete: Add support for sensor-drivers which
    expect clken + pled GPIOs

 drivers/platform/x86/intel/int3472/common.h   |  2 +-
 drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
 2 files changed, 78 insertions(+), 16 deletions(-)
  

Morning Hans - thanks for the set

On 24/11/2022 20:00, Hans de Goede wrote:
> Hi All,
>
> Here is a small set of patches to make the int3472/discrete code
> work with the sensor drivers bundled with the (unfortunately out of tree)
> IPU6 driver.
>
> There are parts of the out of tree IPU6 code, like the sensor drivers,
> which can be moved to the mainline and I do plan to work on this at some
> point and then some of this might need to change. But for now the goal is
> to make the out of tree driver work with standard mainline distro kernels
> through e.g. dkms. Otherwise users need to run a patched kernel just for
> a couple of small differences.
>
> This is basically a rewrite of this patch:
> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>
> Wich users who want to use the IPU6 driver so far have had to manually
> apply to their kernels which is quite inconvenient.
>
> This rewrite makes 2 significant changes:
>
> 1. Don't break things on IPU3 platforms
>
> 2. Instead of extending the int3472_sensor_configs[] quirks table for each
> model which needs "clken" and "pled" GPIOs, do this based on matching
> the ACPI HID of the ACPI device describing the sensor.
>
> The need for these GPIOs is a property of the specific sensor driver which
> binds using this same HID, so by using this we avoid having to extend the
> int3472_sensor_configs[] quirks table all the time.
>
> This allows roling back the behavior to at least use a clk-framework
> clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
> the sensor drivers, assuming that the drivers are switched over to the
> clk framework as part of their mainlining.
>
> A bigger question is what to do with the privacy-led GPIO on IPU3
> we so far have turned the LED on/off at the same as te clock,
> but at least on some IPU6 models this won't work, because they only
> have a privacy-led GPIO and no clk_en GPIO (there is no sensor
> clk-control at all on some models).


Ah how annoying, we hadn't come across any situations for IPU3 with a 
privacy LED but no clock GPIO

>
> I think we should maybe move all models, including IPU3 based
> models over to using a normal GPIO for controlling the privacy-led
> to make things consistent.


I think they probably should be represented as LED devices then, and 
have the media subsytem call some framework to find associated LEDs and 
cycle them at power on time in the sensor drivers. I know there's the 
v4l2_flash structure at the moment, but not sure if a privacy one exists.

>
> And likewise (eventually) completely drop the "clken" GPIO this
> patch series introduces (with some sensors) and instead always model
> this through the clk-framework.
>
> Regards,
>
> Hans
>
>
> Hans de Goede (3):
>    platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
>    platform/x86: int3472/discrete: Get the polarity from the _DSM entry
>    platform/x86: int3472/discrete: Add support for sensor-drivers which
>      expect clken + pled GPIOs
>
>   drivers/platform/x86/intel/int3472/common.h   |  2 +-
>   drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
>   2 files changed, 78 insertions(+), 16 deletions(-)
>
  

On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:
> Morning Hans - thanks for the set
> 
> On 24/11/2022 20:00, Hans de Goede wrote:
> > Hi All,
> >
> > Here is a small set of patches to make the int3472/discrete code
> > work with the sensor drivers bundled with the (unfortunately out of tree)
> > IPU6 driver.
> >
> > There are parts of the out of tree IPU6 code, like the sensor drivers,
> > which can be moved to the mainline and I do plan to work on this at some
> > point and then some of this might need to change. But for now the goal is
> > to make the out of tree driver work with standard mainline distro kernels
> > through e.g. dkms. Otherwise users need to run a patched kernel just for
> > a couple of small differences.
> >
> > This is basically a rewrite of this patch:
> > https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
> >
> > Wich users who want to use the IPU6 driver so far have had to manually
> > apply to their kernels which is quite inconvenient.
> >
> > This rewrite makes 2 significant changes:
> >
> > 1. Don't break things on IPU3 platforms
> >
> > 2. Instead of extending the int3472_sensor_configs[] quirks table for each
> > model which needs "clken" and "pled" GPIOs, do this based on matching
> > the ACPI HID of the ACPI device describing the sensor.
> >
> > The need for these GPIOs is a property of the specific sensor driver which
> > binds using this same HID, so by using this we avoid having to extend the
> > int3472_sensor_configs[] quirks table all the time.
> >
> > This allows roling back the behavior to at least use a clk-framework
> > clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
> > the sensor drivers, assuming that the drivers are switched over to the
> > clk framework as part of their mainlining.
> >
> > A bigger question is what to do with the privacy-led GPIO on IPU3
> > we so far have turned the LED on/off at the same as te clock,
> > but at least on some IPU6 models this won't work, because they only
> > have a privacy-led GPIO and no clk_en GPIO (there is no sensor
> > clk-control at all on some models).
> 
> Ah how annoying, we hadn't come across any situations for IPU3 with a 
> privacy LED but no clock GPIO
> 
> > I think we should maybe move all models, including IPU3 based
> > models over to using a normal GPIO for controlling the privacy-led
> > to make things consistent.
> 
> I think they probably should be represented as LED devices then, and 
> have the media subsytem call some framework to find associated LEDs and 
> cycle them at power on time in the sensor drivers. I know there's the 
> v4l2_flash structure at the moment, but not sure if a privacy one exists.

The whole point of a privacy LED is to be controlled automatically (and
ideally without software intervention, but that's a different story).
Can the LED framework be used without having the LED exposed to
userspace ?

> > And likewise (eventually) completely drop the "clken" GPIO this
> > patch series introduces (with some sensors) and instead always model
> > this through the clk-framework.
> >
> > Regards,
> >
> > Hans
> >
> >
> > Hans de Goede (3):
> >    platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
> >    platform/x86: int3472/discrete: Get the polarity from the _DSM entry
> >    platform/x86: int3472/discrete: Add support for sensor-drivers which
> >      expect clken + pled GPIOs
> >
> >   drivers/platform/x86/intel/int3472/common.h   |  2 +-
> >   drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
> >   2 files changed, 78 insertions(+), 16 deletions(-)
  

Hi,

On 11/25/22 11:17, Dan Scally wrote:
> Morning Hans - thanks for the set
> 
> On 24/11/2022 20:00, Hans de Goede wrote:
>> Hi All,
>>
>> Here is a small set of patches to make the int3472/discrete code
>> work with the sensor drivers bundled with the (unfortunately out of tree)
>> IPU6 driver.
>>
>> There are parts of the out of tree IPU6 code, like the sensor drivers,
>> which can be moved to the mainline and I do plan to work on this at some
>> point and then some of this might need to change. But for now the goal is
>> to make the out of tree driver work with standard mainline distro kernels
>> through e.g. dkms. Otherwise users need to run a patched kernel just for
>> a couple of small differences.
>>
>> This is basically a rewrite of this patch:
>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>>
>> Wich users who want to use the IPU6 driver so far have had to manually
>> apply to their kernels which is quite inconvenient.
>>
>> This rewrite makes 2 significant changes:
>>
>> 1. Don't break things on IPU3 platforms
>>
>> 2. Instead of extending the int3472_sensor_configs[] quirks table for each
>> model which needs "clken" and "pled" GPIOs, do this based on matching
>> the ACPI HID of the ACPI device describing the sensor.
>>
>> The need for these GPIOs is a property of the specific sensor driver which
>> binds using this same HID, so by using this we avoid having to extend the
>> int3472_sensor_configs[] quirks table all the time.
>>
>> This allows roling back the behavior to at least use a clk-framework
>> clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
>> the sensor drivers, assuming that the drivers are switched over to the
>> clk framework as part of their mainlining.
>>
>> A bigger question is what to do with the privacy-led GPIO on IPU3
>> we so far have turned the LED on/off at the same as te clock,
>> but at least on some IPU6 models this won't work, because they only
>> have a privacy-led GPIO and no clk_en GPIO (there is no sensor
>> clk-control at all on some models).
> 
> 
> Ah how annoying, we hadn't come across any situations for IPU3 with a privacy LED but no clock GPIO
> 
>>
>> I think we should maybe move all models, including IPU3 based
>> models over to using a normal GPIO for controlling the privacy-led
>> to make things consistent.
> 
> 
> I think they probably should be represented as LED devices then, and have the media subsytem call some framework to find associated LEDs and cycle them at power on time in the sensor drivers. I know there's the v4l2_flash structure at the moment, but not sure if a privacy one exists.

That is actually a pretty good idea, the LED subsystem has the notion
of triggers, which are identified simply with a string.

So we could simple add a LED class device which sets it default trigger
to "camera-front" and then have either the sensor-drivers start-stream
or maybe even something more generic, so in the media subsystem code
somewhere set the led trigger.

See e.g. the LED class device in drivers/hid/hid-lenovo.c and how
it sets data->led_micmute.default_trigger = "audio-micmute",
combined with the drivers/leds/trigger/ledtrig-audio.c code,
where the ALSA kernel code just does, e.g.:

ledtrig_audio_set(LED_AUDIO_MUTE, 1);
or:
ledtrig_audio_set(LED_AUDIO_MUTE, 0);

We would then probably need to do something like rename
the drivers/leds/trigger/ledtrig-audio.c code and extend the
enum led_audio type with front + back cameras. That or copy the
code, but just renaming it seems better then adding a copy.

And then all need is to have something call:

ledtrig_multimedia_set(LED_CAMERA_FRONT, 0);
ledtrig_multimedia_set(LED_CAMERA_FRONT, 1);

And we are done.

I think the biggest question here is where to put those
ledtrig_multimedia_set() calls ?

These calls will be no-ops when no LED has its trigger
set to "camera-front", so there is no need to worry
about making them conditional (other then them being
conditional (or stubbed out?) when the LED subsystem
is disabled).

Note I would like to move forward with this patch set as is,
to unblock distros wanting to package the out of tree
IPU6 driver for now and then we can convert things to this
model later.

Please let me know if there are any objections with going
with this patch-set as an intermediate solution.

Regards,

Hans
  

On 25/11/2022 10:58, Laurent Pinchart wrote:
> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:
>> Morning Hans - thanks for the set
>>
>> On 24/11/2022 20:00, Hans de Goede wrote:
>>> Hi All,
>>>
>>> Here is a small set of patches to make the int3472/discrete code
>>> work with the sensor drivers bundled with the (unfortunately out of tree)
>>> IPU6 driver.
>>>
>>> There are parts of the out of tree IPU6 code, like the sensor drivers,
>>> which can be moved to the mainline and I do plan to work on this at some
>>> point and then some of this might need to change. But for now the goal is
>>> to make the out of tree driver work with standard mainline distro kernels
>>> through e.g. dkms. Otherwise users need to run a patched kernel just for
>>> a couple of small differences.
>>>
>>> This is basically a rewrite of this patch:
>>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>>>
>>> Wich users who want to use the IPU6 driver so far have had to manually
>>> apply to their kernels which is quite inconvenient.
>>>
>>> This rewrite makes 2 significant changes:
>>>
>>> 1. Don't break things on IPU3 platforms
>>>
>>> 2. Instead of extending the int3472_sensor_configs[] quirks table for each
>>> model which needs "clken" and "pled" GPIOs, do this based on matching
>>> the ACPI HID of the ACPI device describing the sensor.
>>>
>>> The need for these GPIOs is a property of the specific sensor driver which
>>> binds using this same HID, so by using this we avoid having to extend the
>>> int3472_sensor_configs[] quirks table all the time.
>>>
>>> This allows roling back the behavior to at least use a clk-framework
>>> clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
>>> the sensor drivers, assuming that the drivers are switched over to the
>>> clk framework as part of their mainlining.
>>>
>>> A bigger question is what to do with the privacy-led GPIO on IPU3
>>> we so far have turned the LED on/off at the same as te clock,
>>> but at least on some IPU6 models this won't work, because they only
>>> have a privacy-led GPIO and no clk_en GPIO (there is no sensor
>>> clk-control at all on some models).
>> Ah how annoying, we hadn't come across any situations for IPU3 with a
>> privacy LED but no clock GPIO
>>
>>> I think we should maybe move all models, including IPU3 based
>>> models over to using a normal GPIO for controlling the privacy-led
>>> to make things consistent.
>> I think they probably should be represented as LED devices then, and
>> have the media subsytem call some framework to find associated LEDs and
>> cycle them at power on time in the sensor drivers. I know there's the
>> v4l2_flash structure at the moment, but not sure if a privacy one exists.
> The whole point of a privacy LED is to be controlled automatically (and
> ideally without software intervention, but that's a different story).
> Can the LED framework be used without having the LED exposed to
> userspace ?


I think that's what led_sysfs_disable() is for, though I haven't gotten 
far enough down that route to know that for sure.

>>> And likewise (eventually) completely drop the "clken" GPIO this
>>> patch series introduces (with some sensors) and instead always model
>>> this through the clk-framework.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>> Hans de Goede (3):
>>>     platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
>>>     platform/x86: int3472/discrete: Get the polarity from the _DSM entry
>>>     platform/x86: int3472/discrete: Add support for sensor-drivers which
>>>       expect clken + pled GPIOs
>>>
>>>    drivers/platform/x86/intel/int3472/common.h   |  2 +-
>>>    drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
>>>    2 files changed, 78 insertions(+), 16 deletions(-)
  

On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote:
> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:

...

> Can the LED framework be used without having the LED exposed to
> userspace ?

I believe the correct question here is "can the states of some leds be
read-only from user perspective" (this way any changes into led subsystems
looks less intrusive, esp. taking into account that subsystem is de facto
unmaintained).
  

On 25/11/2022 11:06, Andy Shevchenko wrote:
> On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote:
>> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:
> ...
>
>> Can the LED framework be used without having the LED exposed to
>> userspace ?
> I believe the correct question here is "can the states of some leds be
> read-only from user perspective" (this way any changes into led subsystems
> looks less intrusive, esp. taking into account that subsystem is de facto
> unmaintained).
>

I think the answer to that is yes:


https://elixir.bootlin.com/linux/latest/source/drivers/leds/led-class.c#L47
  

Hi,

On 11/25/22 11:58, Laurent Pinchart wrote:
> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:
>> Morning Hans - thanks for the set
>>
>> On 24/11/2022 20:00, Hans de Goede wrote:
>>> Hi All,
>>>
>>> Here is a small set of patches to make the int3472/discrete code
>>> work with the sensor drivers bundled with the (unfortunately out of tree)
>>> IPU6 driver.
>>>
>>> There are parts of the out of tree IPU6 code, like the sensor drivers,
>>> which can be moved to the mainline and I do plan to work on this at some
>>> point and then some of this might need to change. But for now the goal is
>>> to make the out of tree driver work with standard mainline distro kernels
>>> through e.g. dkms. Otherwise users need to run a patched kernel just for
>>> a couple of small differences.
>>>
>>> This is basically a rewrite of this patch:
>>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>>>
>>> Wich users who want to use the IPU6 driver so far have had to manually
>>> apply to their kernels which is quite inconvenient.
>>>
>>> This rewrite makes 2 significant changes:
>>>
>>> 1. Don't break things on IPU3 platforms
>>>
>>> 2. Instead of extending the int3472_sensor_configs[] quirks table for each
>>> model which needs "clken" and "pled" GPIOs, do this based on matching
>>> the ACPI HID of the ACPI device describing the sensor.
>>>
>>> The need for these GPIOs is a property of the specific sensor driver which
>>> binds using this same HID, so by using this we avoid having to extend the
>>> int3472_sensor_configs[] quirks table all the time.
>>>
>>> This allows roling back the behavior to at least use a clk-framework
>>> clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
>>> the sensor drivers, assuming that the drivers are switched over to the
>>> clk framework as part of their mainlining.
>>>
>>> A bigger question is what to do with the privacy-led GPIO on IPU3
>>> we so far have turned the LED on/off at the same as te clock,
>>> but at least on some IPU6 models this won't work, because they only
>>> have a privacy-led GPIO and no clk_en GPIO (there is no sensor
>>> clk-control at all on some models).
>>
>> Ah how annoying, we hadn't come across any situations for IPU3 with a 
>> privacy LED but no clock GPIO
>>
>>> I think we should maybe move all models, including IPU3 based
>>> models over to using a normal GPIO for controlling the privacy-led
>>> to make things consistent.
>>
>> I think they probably should be represented as LED devices then, and 
>> have the media subsytem call some framework to find associated LEDs and 
>> cycle them at power on time in the sensor drivers. I know there's the 
>> v4l2_flash structure at the moment, but not sure if a privacy one exists.
> 
> The whole point of a privacy LED is to be controlled automatically (and
> ideally without software intervention, but that's a different story).
> Can the LED framework be used without having the LED exposed to
> userspace ?

AFAIK using the LED framework will automatically expose the LED
to userspace; and using triggers as I mentioned in my other email
will also allow the user to unset the trigger or even use a different
trigger.

I understand where you are coming from, but I was actually seeing
this (exposed to userspace) as a feature. Users may want to repurpose
the LED, maybe make it blink when the camera is on for extra obviousness
the camera is on. Maybe always have it off because it is too annoying,
etc...  ?

My vision here is that ideally the LED should be hardwired to go on
together with some enable pin or power-supply of the sensor.

But if it is actually just a GPIO, then there is something to be said
for giving the user full-control. OTOH this would make writing spy-ware
where the LED never goes on a lot easier...

Typing this out I'm afraid that I have to agree with you and that
the spyware argument likely wins over how giving the user more control
would be nice :(

Which would bring us back to just making it a GPIO, which would then
need to be turned on+off by the sensor driver I guess.

There seems to be a bunch of GPIO/clk/regulator boilerplate duplicated
in all the sensor drivers. I think a little helper-library  for this might
be in order. E.g. Something like this (in the .h file)

struct camera_sensor_pwr_helper {
	// bunch of stuff here, this should be fixed size so that the
	// sensor drivers can embed it into their driver-data struct
};

int camera_sensor_pwr_helper_init(struct camera_sensor_pwr_helper *helper,
				  const char *supply_names, int supply_count,
				  const char* clk_name.
				  /* other stuff which I'm probably forgetting right now */);

// turn_on_privacy_led should be false when called from probe(), must be true when
// called on stream_on().
int camera_sensor_pwr_helper_on(struct camera_sensor_pwr_helper *helper, bool turn_on_privacy_led);
int camera_sensor_pwr_helper_off(struct camera_sensor_pwr_helper *helper);

// maybe, or make everything devm managed? :
int camera_sensor_pwr_helper_exit(struct camera_sensor_pwr_helper *helper);

Just is just a really really quick n dirty design. For one I could use
suggestions for a better name for the thing :)

I think something like this will be helpfull to reduce a whole bunch
of boilerplate code related to powering on/off the sensor in all
the drivers; and it would give us a central place to drive an
(optional) privacy-led GPIO.

Regards,

Hans











> 
>>> And likewise (eventually) completely drop the "clken" GPIO this
>>> patch series introduces (with some sensors) and instead always model
>>> this through the clk-framework.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>> Hans de Goede (3):
>>>    platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
>>>    platform/x86: int3472/discrete: Get the polarity from the _DSM entry
>>>    platform/x86: int3472/discrete: Add support for sensor-drivers which
>>>      expect clken + pled GPIOs
>>>
>>>   drivers/platform/x86/intel/int3472/common.h   |  2 +-
>>>   drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
>>>   2 files changed, 78 insertions(+), 16 deletions(-)
>
  

Hi,

On 11/25/22 12:11, Dan Scally wrote:
> 
> On 25/11/2022 11:06, Andy Shevchenko wrote:
>> On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote:
>>> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:
>> ...
>>
>>> Can the LED framework be used without having the LED exposed to
>>> userspace ?
>> I believe the correct question here is "can the states of some leds be
>> read-only from user perspective" (this way any changes into led subsystems
>> looks less intrusive, esp. taking into account that subsystem is de facto
>> unmaintained).
>>
> 
> I think the answer to that is yes:
> 
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/leds/led-class.c#L47

Interesting, I did not know that. But what is the added value of
using the LED subsytem then for a simple only on/off LED driven
by a GPIO?

One of the challenges with using LED triggers for the privacy led,
is that we need at least 2 triggers: "camera-front" and "camera-back"
and then somehow to let what ever code sets the triggers know if
it is dealing with the front or back sensor.

Where as with GPIO-s we *bind* them to the sensor i2c_client so if
we just have the sensor-driver look for an optional GPIO called
"privacy-led" then we don't have this how to we bind the LED to
the sensor problem; and if we drop the sysfs interface I fail to
see the value in using the LED subsystem for GPIO a driven LED.

Also see my other reply for a proposal to be able to share the
code dealing with this between sensor drivers (and also remove
some other gpio/clk/regulator boilerplate from sensor drivers).

Regards,

Hans
  

Hi

On 25/11/2022 11:23, Hans de Goede wrote:
> Hi,
>
> On 11/25/22 12:11, Dan Scally wrote:
>> On 25/11/2022 11:06, Andy Shevchenko wrote:
>>> On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote:
>>>> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:
>>> ...
>>>
>>>> Can the LED framework be used without having the LED exposed to
>>>> userspace ?
>>> I believe the correct question here is "can the states of some leds be
>>> read-only from user perspective" (this way any changes into led subsystems
>>> looks less intrusive, esp. taking into account that subsystem is de facto
>>> unmaintained).
>>>
>> I think the answer to that is yes:
>>
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/leds/led-class.c#L47
> Interesting, I did not know that. But what is the added value of
> using the LED subsytem then for a simple only on/off LED driven
> by a GPIO?


Well I suppose it depends on the LED. In the flash case the v4l2 
framework disables the sysfs interface for the LED whilst it holds the 
flash subdev open, which should mean that no nefarious program could 
turn off the LED whilst it was running the camera but because the sysfs 
is enabled whilst the v4l2 subdev is closed [1] you could still use that 
LED as a torch outside of camera streaming. That's probably not a 
situation that's likely to occur with a privacy LED given they're likely 
to be much less bright though I suppose, and maybe it's right to treat 
them differently.


[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-flash-led-class.c#L632

> One of the challenges with using LED triggers for the privacy led,
> is that we need at least 2 triggers: "camera-front" and "camera-back"
> and then somehow to let what ever code sets the triggers know if
> it is dealing with the front or back sensor.


Yes, that is a problem, my plan was to connect them with fwnode and 
ancillary links, in the same way for example we connected the VCM to the 
cameras. I think that the int3472-discrete driver would have to do that.

>
> Where as with GPIO-s we *bind* them to the sensor i2c_client so if
> we just have the sensor-driver look for an optional GPIO called
> "privacy-led" then we don't have this how to we bind the LED to
> the sensor problem; and if we drop the sysfs interface I fail to
> see the value in using the LED subsystem for GPIO a driven LED.
>
> Also see my other reply for a proposal to be able to share the
> code dealing with this between sensor drivers (and also remove
> some other gpio/clk/regulator boilerplate from sensor drivers).


Yes I certainly find that idea appealing, there is of lot of boilerplate 
that could be reduced with that idea.

>
> Regards,
>
> Hans
>
>
  

Hi,

On 11/25/22 12:42, Dan Scally wrote:
> Hi
> 
> On 25/11/2022 11:23, Hans de Goede wrote:
>> Hi,
>>
>> On 11/25/22 12:11, Dan Scally wrote:
>>> On 25/11/2022 11:06, Andy Shevchenko wrote:
>>>> On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote:
>>>>> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:
>>>> ...
>>>>
>>>>> Can the LED framework be used without having the LED exposed to
>>>>> userspace ?
>>>> I believe the correct question here is "can the states of some leds be
>>>> read-only from user perspective" (this way any changes into led subsystems
>>>> looks less intrusive, esp. taking into account that subsystem is de facto
>>>> unmaintained).
>>>>
>>> I think the answer to that is yes:
>>>
>>>
>>> https://elixir.bootlin.com/linux/latest/source/drivers/leds/led-class.c#L47
>> Interesting, I did not know that. But what is the added value of
>> using the LED subsytem then for a simple only on/off LED driven
>> by a GPIO?
> 
> 
> Well I suppose it depends on the LED. In the flash case the v4l2 framework disables the sysfs interface for the LED whilst it holds the flash subdev open, which should mean that no nefarious program could turn off the LED whilst it was running the camera but because the sysfs is enabled whilst the v4l2 subdev is closed [1] you could still use that LED as a torch outside of camera streaming. That's probably not a situation that's likely to occur with a privacy LED given they're likely to be much less bright though I suppose, and maybe it's right to treat them differently.
> 
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-flash-led-class.c#L632

If we only disable the sysfs access temporarily then spy-ware in userspace
can still just clear the trigger, so we would need to permanently
disable userspace access (or decide to not disable userspace access
at all).

>> One of the challenges with using LED triggers for the privacy led,
>> is that we need at least 2 triggers: "camera-front" and "camera-back"
>> and then somehow to let what ever code sets the triggers know if
>> it is dealing with the front or back sensor.
> 
> 
> Yes, that is a problem, my plan was to connect them with fwnode and ancillary links, in the same way for example we connected the VCM to the cameras. I think that the int3472-discrete driver would have to do that.

Which would involve a bunch of non trivial code. Where as if we
just model the LED as a GPIO for the sensor-driver to consume
we get this for free.

My conclusion here is:

1. We don't want userspace access because we don't want to make things
easier for spy-ware.

2. Without userspace access there is no added value in using the LED
subsystem and just modelling this as a GPIO is easier / more KISS.

>> Where as with GPIO-s we *bind* them to the sensor i2c_client so if
>> we just have the sensor-driver look for an optional GPIO called
>> "privacy-led" then we don't have this how to we bind the LED to
>> the sensor problem; and if we drop the sysfs interface I fail to
>> see the value in using the LED subsystem for GPIO a driven LED.
>>
>> Also see my other reply for a proposal to be able to share the
>> code dealing with this between sensor drivers (and also remove
>> some other gpio/clk/regulator boilerplate from sensor drivers).
> 
> 
> Yes I certainly find that idea appealing, there is of lot of boilerplate that could be reduced with that idea.

I glad you like the idea. Any suggestions for a better name
for the helper lib / namespace ?

Regards,

Hans
  

On Thu, Nov 24, 2022 at 09:00:04PM +0100, Hans de Goede wrote:
> Hi All,
> 
> Here is a small set of patches to make the int3472/discrete code
> work with the sensor drivers bundled with the (unfortunately out of tree)
> IPU6 driver.
> 
> There are parts of the out of tree IPU6 code, like the sensor drivers,
> which can be moved to the mainline and I do plan to work on this at some
> point and then some of this might need to change. But for now the goal is
> to make the out of tree driver work with standard mainline distro kernels
> through e.g. dkms. Otherwise users need to run a patched kernel just for
> a couple of small differences.
> 
> This is basically a rewrite of this patch:
> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
> 
> Wich users who want to use the IPU6 driver so far have had to manually
> apply to their kernels which is quite inconvenient.
> 
> This rewrite makes 2 significant changes:
> 
> 1. Don't break things on IPU3 platforms
> 
> 2. Instead of extending the int3472_sensor_configs[] quirks table for each
> model which needs "clken" and "pled" GPIOs, do this based on matching
> the ACPI HID of the ACPI device describing the sensor.

How can we be sure that a given sensor model will always be wired to the
same GPIOs on all platforms that integrate it with an IPU6 (or IPU3) ?

> The need for these GPIOs is a property of the specific sensor driver which
> binds using this same HID, so by using this we avoid having to extend the
> int3472_sensor_configs[] quirks table all the time.
> 
> This allows roling back the behavior to at least use a clk-framework
> clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
> the sensor drivers, assuming that the drivers are switched over to the
> clk framework as part of their mainlining.
> 
> A bigger question is what to do with the privacy-led GPIO on IPU3
> we so far have turned the LED on/off at the same as te clock,
> but at least on some IPU6 models this won't work, because they only
> have a privacy-led GPIO and no clk_en GPIO (there is no sensor
> clk-control at all on some models).

Can we turn it on at the same time as the power then ?

> I think we should maybe move all models, including IPU3 based
> models over to using a normal GPIO for controlling the privacy-led
> to make things consistent.
> 
> And likewise (eventually) completely drop the "clken" GPIO this
> patch series introduces (with some sensors) and instead always model
> this through the clk-framework.
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (3):
>   platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
>   platform/x86: int3472/discrete: Get the polarity from the _DSM entry
>   platform/x86: int3472/discrete: Add support for sensor-drivers which
>     expect clken + pled GPIOs
> 
>  drivers/platform/x86/intel/int3472/common.h   |  2 +-
>  drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
>  2 files changed, 78 insertions(+), 16 deletions(-)
  

Hi Hans,

On Fri, Nov 25, 2022 at 12:15:57PM +0100, Hans de Goede wrote:
> On 11/25/22 11:58, Laurent Pinchart wrote:
> > On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote:
> >> Morning Hans - thanks for the set
> >>
> >> On 24/11/2022 20:00, Hans de Goede wrote:
> >>> Hi All,
> >>>
> >>> Here is a small set of patches to make the int3472/discrete code
> >>> work with the sensor drivers bundled with the (unfortunately out of tree)
> >>> IPU6 driver.
> >>>
> >>> There are parts of the out of tree IPU6 code, like the sensor drivers,
> >>> which can be moved to the mainline and I do plan to work on this at some
> >>> point and then some of this might need to change. But for now the goal is
> >>> to make the out of tree driver work with standard mainline distro kernels
> >>> through e.g. dkms. Otherwise users need to run a patched kernel just for
> >>> a couple of small differences.
> >>>
> >>> This is basically a rewrite of this patch:
> >>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
> >>>
> >>> Wich users who want to use the IPU6 driver so far have had to manually
> >>> apply to their kernels which is quite inconvenient.
> >>>
> >>> This rewrite makes 2 significant changes:
> >>>
> >>> 1. Don't break things on IPU3 platforms
> >>>
> >>> 2. Instead of extending the int3472_sensor_configs[] quirks table for each
> >>> model which needs "clken" and "pled" GPIOs, do this based on matching
> >>> the ACPI HID of the ACPI device describing the sensor.
> >>>
> >>> The need for these GPIOs is a property of the specific sensor driver which
> >>> binds using this same HID, so by using this we avoid having to extend the
> >>> int3472_sensor_configs[] quirks table all the time.
> >>>
> >>> This allows roling back the behavior to at least use a clk-framework
> >>> clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
> >>> the sensor drivers, assuming that the drivers are switched over to the
> >>> clk framework as part of their mainlining.
> >>>
> >>> A bigger question is what to do with the privacy-led GPIO on IPU3
> >>> we so far have turned the LED on/off at the same as te clock,
> >>> but at least on some IPU6 models this won't work, because they only
> >>> have a privacy-led GPIO and no clk_en GPIO (there is no sensor
> >>> clk-control at all on some models).
> >>
> >> Ah how annoying, we hadn't come across any situations for IPU3 with a 
> >> privacy LED but no clock GPIO
> >>
> >>> I think we should maybe move all models, including IPU3 based
> >>> models over to using a normal GPIO for controlling the privacy-led
> >>> to make things consistent.
> >>
> >> I think they probably should be represented as LED devices then, and 
> >> have the media subsytem call some framework to find associated LEDs and 
> >> cycle them at power on time in the sensor drivers. I know there's the 
> >> v4l2_flash structure at the moment, but not sure if a privacy one exists.
> > 
> > The whole point of a privacy LED is to be controlled automatically (and
> > ideally without software intervention, but that's a different story).
> > Can the LED framework be used without having the LED exposed to
> > userspace ?
> 
> AFAIK using the LED framework will automatically expose the LED
> to userspace; and using triggers as I mentioned in my other email
> will also allow the user to unset the trigger or even use a different
> trigger.
> 
> I understand where you are coming from, but I was actually seeing
> this (exposed to userspace) as a feature. Users may want to repurpose
> the LED, maybe make it blink when the camera is on for extra obviousness
> the camera is on. Maybe always have it off because it is too annoying,
> etc...  ?

One use case for turning it off is avoiding reflection in glasses. I
however think this is outweighted by the privacy concerns. If the
privacy LED can be controlled from userspace, at least by default, then
it could as well be dropped completely.

> My vision here is that ideally the LED should be hardwired to go on
> together with some enable pin or power-supply of the sensor.

To make it secure it should be controlled by the hardware, yes.

> But if it is actually just a GPIO, then there is something to be said
> for giving the user full-control. OTOH this would make writing spy-ware
> where the LED never goes on a lot easier...
> 
> Typing this out I'm afraid that I have to agree with you and that
> the spyware argument likely wins over how giving the user more control
> would be nice :(

I also wish we could get both privacy and flexibility :-( I'm not
necessarily opposed to making it controllable by userspace, but that
shouldn't be the default. It could be controlled by a kernel command
line argument for instance. I doubt that would be worth it though.

> Which would bring us back to just making it a GPIO, which would then
> need to be turned on+off by the sensor driver I guess.
> 
> There seems to be a bunch of GPIO/clk/regulator boilerplate duplicated
> in all the sensor drivers. I think a little helper-library  for this might
> be in order. E.g. Something like this (in the .h file)

I fully agree that camera sensor helpers would be good to have.

> struct camera_sensor_pwr_helper {
> 	// bunch of stuff here, this should be fixed size so that the
> 	// sensor drivers can embed it into their driver-data struct
> };
> 
> int camera_sensor_pwr_helper_init(struct camera_sensor_pwr_helper *helper,
> 				  const char *supply_names, int supply_count,
> 				  const char* clk_name.
> 				  /* other stuff which I'm probably forgetting right now */);

There are all kind of constraints on the power on/off sequences, I don't
think we would be able to model this in a generic way without making it
so complicated that it would outweight the benefits.

What I think could help is moving all camera sensor drivers to runtime
PM, and having helpers to properly enable runtime PM in probe() in a way
that works on both ACPI and DT systems, with or without CONFIG_PM
enabled. It's way more complicated than it sounds.

> // turn_on_privacy_led should be false when called from probe(), must be true when
> // called on stream_on().
> int camera_sensor_pwr_helper_on(struct camera_sensor_pwr_helper *helper, bool turn_on_privacy_led);
> int camera_sensor_pwr_helper_off(struct camera_sensor_pwr_helper *helper);
> 
> // maybe, or make everything devm managed? :
> int camera_sensor_pwr_helper_exit(struct camera_sensor_pwr_helper *helper);
> 
> Just is just a really really quick n dirty design. For one I could use
> suggestions for a better name for the thing :)
> 
> I think something like this will be helpfull to reduce a whole bunch
> of boilerplate code related to powering on/off the sensor in all
> the drivers; and it would give us a central place to drive an
> (optional) privacy-led GPIO.
> 
> >>> And likewise (eventually) completely drop the "clken" GPIO this
> >>> patch series introduces (with some sensors) and instead always model
> >>> this through the clk-framework.
> >>>
> >>> Hans de Goede (3):
> >>>    platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
> >>>    platform/x86: int3472/discrete: Get the polarity from the _DSM entry
> >>>    platform/x86: int3472/discrete: Add support for sensor-drivers which
> >>>      expect clken + pled GPIOs
> >>>
> >>>   drivers/platform/x86/intel/int3472/common.h   |  2 +-
> >>>   drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
> >>>   2 files changed, 78 insertions(+), 16 deletions(-)
  

Hi,

On 11/25/22 15:40, Laurent Pinchart wrote:
> On Thu, Nov 24, 2022 at 09:00:04PM +0100, Hans de Goede wrote:
>> Hi All,
>>
>> Here is a small set of patches to make the int3472/discrete code
>> work with the sensor drivers bundled with the (unfortunately out of tree)
>> IPU6 driver.
>>
>> There are parts of the out of tree IPU6 code, like the sensor drivers,
>> which can be moved to the mainline and I do plan to work on this at some
>> point and then some of this might need to change. But for now the goal is
>> to make the out of tree driver work with standard mainline distro kernels
>> through e.g. dkms. Otherwise users need to run a patched kernel just for
>> a couple of small differences.
>>
>> This is basically a rewrite of this patch:
>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>>
>> Wich users who want to use the IPU6 driver so far have had to manually
>> apply to their kernels which is quite inconvenient.
>>
>> This rewrite makes 2 significant changes:
>>
>> 1. Don't break things on IPU3 platforms
>>
>> 2. Instead of extending the int3472_sensor_configs[] quirks table for each
>> model which needs "clken" and "pled" GPIOs, do this based on matching
>> the ACPI HID of the ACPI device describing the sensor.
> 
> How can we be sure that a given sensor model will always be wired to the
> same GPIOs on all platforms that integrate it with an IPU6 (or IPU3) ?

This is not about which GPIOs are actually there, this is about what the
driver expects. Specifically about if the driver expects the clock to
be modelled with the clk framework or as a clk-en GPIO which is
a property of the driver, not of the board design.

But as already mentioned I agree with Dan and you that modelling it
through the clk framework is correct and what needs to happen here is to
patch the IPU6 sensor drivers to move them to the clk framework.

so this is all mute.

Regards,

Hans
  

Hi Laurent,

On 11/25/22 15:46, Laurent Pinchart wrote:

<snip>

>> There seems to be a bunch of GPIO/clk/regulator boilerplate duplicated
>> in all the sensor drivers. I think a little helper-library  for this might
>> be in order. E.g. Something like this (in the .h file)
> 
> I fully agree that camera sensor helpers would be good to have.
> 
>> struct camera_sensor_pwr_helper {
>> 	// bunch of stuff here, this should be fixed size so that the
>> 	// sensor drivers can embed it into their driver-data struct
>> };
>>
>> int camera_sensor_pwr_helper_init(struct camera_sensor_pwr_helper *helper,
>> 				  const char *supply_names, int supply_count,
>> 				  const char* clk_name.
>> 				  /* other stuff which I'm probably forgetting right now */);
> 
> There are all kind of constraints on the power on/off sequences, I don't
> think we would be able to model this in a generic way without making it
> so complicated that it would outweight the benefits.

I know that for some ICs the power sequence can be quite complicated,
but I think that for most this order should work fine:

0. Force enable/reset GPIOs to disabled / reset-asserted (do this at GPIO request time ?)
1. Enable clk(s)
2. Enable regulators (using the bulk API, with supply-names passed
in by the sensor drivers, 
3. Set enable/reset GPIOs to enabled / reset de-asserted

I guess on some models we may need to swap 1 and 2, there could be
a flag for that.

Anything more complicated should just be coded out in the driver, but
I think just supporting this common pattern will already save us
quite a bit of code duplication.

> What I think could help is moving all camera sensor drivers to runtime
> PM, and having helpers to properly enable runtime PM in probe() in a way
> that works on both ACPI and DT systems, with or without CONFIG_PM
> enabled. It's way more complicated than it sounds.

I agree that we should move to runtime-pm and put the power-sequence
in the suspend/resume callback. This will be necessary for any sensors
used on atomisp2 devices, where there are actually ACPI _PS0 and _PS3
methods and/or ACPI power-resources doing the PM for us.

Note for some reason the current staging atomisp driver does not use this,
likely because it was developed for Android boards with broken ACPI
tables. But after having sampled the ACPI tables of a bunch of atomisp
windows devices I believe this should work fine for those.

Regards,

Hans




> 
>> // turn_on_privacy_led should be false when called from probe(), must be true when
>> // called on stream_on().
>> int camera_sensor_pwr_helper_on(struct camera_sensor_pwr_helper *helper, bool turn_on_privacy_led);
>> int camera_sensor_pwr_helper_off(struct camera_sensor_pwr_helper *helper);
>>
>> // maybe, or make everything devm managed? :
>> int camera_sensor_pwr_helper_exit(struct camera_sensor_pwr_helper *helper);
>>
>> Just is just a really really quick n dirty design. For one I could use
>> suggestions for a better name for the thing :)
>>
>> I think something like this will be helpfull to reduce a whole bunch
>> of boilerplate code related to powering on/off the sensor in all
>> the drivers; and it would give us a central place to drive an
>> (optional) privacy-led GPIO.
>>
>>>>> And likewise (eventually) completely drop the "clken" GPIO this
>>>>> patch series introduces (with some sensors) and instead always model
>>>>> this through the clk-framework.
>>>>>
>>>>> Hans de Goede (3):
>>>>>    platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
>>>>>    platform/x86: int3472/discrete: Get the polarity from the _DSM entry
>>>>>    platform/x86: int3472/discrete: Add support for sensor-drivers which
>>>>>      expect clken + pled GPIOs
>>>>>
>>>>>   drivers/platform/x86/intel/int3472/common.h   |  2 +-
>>>>>   drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
>>>>>   2 files changed, 78 insertions(+), 16 deletions(-)
>
  

Hi Hans,

On Mon, Nov 28, 2022 at 05:11:52PM +0100, Hans de Goede wrote:
> On 11/25/22 15:46, Laurent Pinchart wrote:
> 
> <snip>
> 
> >> There seems to be a bunch of GPIO/clk/regulator boilerplate duplicated
> >> in all the sensor drivers. I think a little helper-library  for this might
> >> be in order. E.g. Something like this (in the .h file)
> > 
> > I fully agree that camera sensor helpers would be good to have.
> > 
> >> struct camera_sensor_pwr_helper {
> >> 	// bunch of stuff here, this should be fixed size so that the
> >> 	// sensor drivers can embed it into their driver-data struct
> >> };
> >>
> >> int camera_sensor_pwr_helper_init(struct camera_sensor_pwr_helper *helper,
> >> 				  const char *supply_names, int supply_count,
> >> 				  const char* clk_name.
> >> 				  /* other stuff which I'm probably forgetting right now */);
> > 
> > There are all kind of constraints on the power on/off sequences, I don't
> > think we would be able to model this in a generic way without making it
> > so complicated that it would outweight the benefits.
> 
> I know that for some ICs the power sequence can be quite complicated,
> but I think that for most this order should work fine:
> 
> 0. Force enable/reset GPIOs to disabled / reset-asserted (do this at GPIO request time ?)
> 1. Enable clk(s)
> 2. Enable regulators (using the bulk API, with supply-names passed
> in by the sensor drivers, 
> 3. Set enable/reset GPIOs to enabled / reset de-asserted
> 
> I guess on some models we may need to swap 1 and 2, there could be
> a flag for that.

There are also various delays that may be needed between the different
steps, including between bringing up (and down) the different power
rails.

> Anything more complicated should just be coded out in the driver, but
> I think just supporting this common pattern will already save us
> quite a bit of code duplication.

There was an old attempt to code generic power sequences in DT which
didn't lead anywhere. I'm not quite sure doing so in a camera sensor
helper will have a much better fate. We can of course give it a try, but
as mentioned before, I think effort would be better focussed on first
moving sensor drivers to runtime PM (and runtime PM autosuspend).

> > What I think could help is moving all camera sensor drivers to runtime
> > PM, and having helpers to properly enable runtime PM in probe() in a way
> > that works on both ACPI and DT systems, with or without CONFIG_PM
> > enabled. It's way more complicated than it sounds.
> 
> I agree that we should move to runtime-pm and put the power-sequence
> in the suspend/resume callback. This will be necessary for any sensors
> used on atomisp2 devices, where there are actually ACPI _PS0 and _PS3
> methods and/or ACPI power-resources doing the PM for us.
> 
> Note for some reason the current staging atomisp driver does not use this,
> likely because it was developed for Android boards with broken ACPI
> tables. But after having sampled the ACPI tables of a bunch of atomisp
> windows devices I believe this should work fine for those.
> 
> >> // turn_on_privacy_led should be false when called from probe(), must be true when
> >> // called on stream_on().
> >> int camera_sensor_pwr_helper_on(struct camera_sensor_pwr_helper *helper, bool turn_on_privacy_led);
> >> int camera_sensor_pwr_helper_off(struct camera_sensor_pwr_helper *helper);
> >>
> >> // maybe, or make everything devm managed? :
> >> int camera_sensor_pwr_helper_exit(struct camera_sensor_pwr_helper *helper);
> >>
> >> Just is just a really really quick n dirty design. For one I could use
> >> suggestions for a better name for the thing :)
> >>
> >> I think something like this will be helpfull to reduce a whole bunch
> >> of boilerplate code related to powering on/off the sensor in all
> >> the drivers; and it would give us a central place to drive an
> >> (optional) privacy-led GPIO.
> >>
> >>>>> And likewise (eventually) completely drop the "clken" GPIO this
> >>>>> patch series introduces (with some sensors) and instead always model
> >>>>> this through the clk-framework.
> >>>>>
> >>>>> Hans de Goede (3):
> >>>>>    platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
> >>>>>    platform/x86: int3472/discrete: Get the polarity from the _DSM entry
> >>>>>    platform/x86: int3472/discrete: Add support for sensor-drivers which
> >>>>>      expect clken + pled GPIOs
> >>>>>
> >>>>>   drivers/platform/x86/intel/int3472/common.h   |  2 +-
> >>>>>   drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++---
> >>>>>   2 files changed, 78 insertions(+), 16 deletions(-)