[PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

[PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

[Hans de Goede]

Hi All,

The out of tree IPU6 driver has moved to using the in kernel INT3472
code for doing power-ctrl rather then doing their own thing (good!).

Some of the IPU6 devices with a discrete INT3472 ACPI device have a
privacy-led GPIO. but no clk-enable GPIO. To make this work this series
moves the privacy LED control from being integrated with the clk-provider
to modelling the privacy LED as a separate GPIO. This also brings the
discrete INT3472 ACPI device privacy LED handling inline with the privacy
LED handling for INT3472 TPS68470 PMIC devices which I posted here:

https://lore.kernel.org/platform-driver-x86/20221128214408.165726-1-hdegoede@redhat.com/

This obsoletes my previous "[PATCH 0/3] platform/x86: int3472/discrete:
Make it work with IPU6" series:

https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/

Mauro since laptops with IPU6 cameras are becoming more and more
popular I would like to get this merged for 6.2 so that with 6.2
users will be able to build the out of tree IPU6 driver without
requiring patching their main kernel. I realize we are a bit
late in the cycle, but can you please still take the ov5693 patch
for 6.2 ? It is quite small / straight-forward and since it used
gpiod_get_optional() it is a no-op without the rest of this series.

This series has been tested on:

- Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED
- Dell Latitude 9420, IPU 6 with privacy LED on front
- Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED,
                              back: ov8865 with privacy LED

Regards,

Hans


Hans de Goede (6):
  media: ov5693: Add support for a privacy-led GPIO
  platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
  platform/x86: int3472/discrete: Treat privacy LED as regular GPIO
  platform/x86: int3472/discrete: Move GPIO request to
    skl_int3472_register_clock()
  platform/x86: int3472/discrete: Ensure the clk/power enable pins are
    in output mode
  platform/x86: int3472/discrete: Get the polarity from the _DSM entry

 drivers/media/i2c/ov5693.c                    | 10 ++
 .../x86/intel/int3472/clk_and_regulator.c     | 35 +++++--
 drivers/platform/x86/intel/int3472/common.h   |  4 +-
 drivers/platform/x86/intel/int3472/discrete.c | 95 ++++++++-----------
 4 files changed, 80 insertions(+), 64 deletions(-)

-- 
2.38.1

[PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Hans de Goede]

Add support for a privacy-led GPIO.

Making the privacy LED to controlable from userspace, as using the LED
class subsystem would do, would make it too easy for spy-ware to disable
the LED.

To avoid this have the sensor driver directly control the LED.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note an additional advantage of directly controlling the GPIO is that
GPIOs are tied directly to consumer devices. Where as with a LED class
device, there would need to be some mechanism to tie the right LED
(e.g front or back) to the right sensor.
---
 drivers/media/i2c/ov5693.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index a97ec132ba3a..e3c3bed69ad6 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -156,6 +156,7 @@ struct ov5693_device {
 
 	struct gpio_desc *reset;
 	struct gpio_desc *powerdown;
+	struct gpio_desc *privacy_led;
 	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
 	struct clk *xvclk;
 
@@ -789,6 +790,7 @@ static int ov5693_sensor_init(struct ov5693_device *ov5693)
 
 static void ov5693_sensor_powerdown(struct ov5693_device *ov5693)
 {
+	gpiod_set_value_cansleep(ov5693->privacy_led, 0);
 	gpiod_set_value_cansleep(ov5693->reset, 1);
 	gpiod_set_value_cansleep(ov5693->powerdown, 1);
 
@@ -818,6 +820,7 @@ static int ov5693_sensor_powerup(struct ov5693_device *ov5693)
 
 	gpiod_set_value_cansleep(ov5693->powerdown, 0);
 	gpiod_set_value_cansleep(ov5693->reset, 0);
+	gpiod_set_value_cansleep(ov5693->privacy_led, 1);
 
 	usleep_range(5000, 7500);
 
@@ -1325,6 +1328,13 @@ static int ov5693_configure_gpios(struct ov5693_device *ov5693)
 		return PTR_ERR(ov5693->powerdown);
 	}
 
+	ov5693->privacy_led = devm_gpiod_get_optional(ov5693->dev, "privacy-led",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(ov5693->privacy_led)) {
+		dev_err(ov5693->dev, "Error fetching privacy-led GPIO\n");
+		return PTR_ERR(ov5693->privacy_led);
+	}
+
 	return 0;
 }
 
-- 
2.38.1

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Sakari Ailus]

Hi Hans,

On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
> Add support for a privacy-led GPIO.
> 
> Making the privacy LED to controlable from userspace, as using the LED
> class subsystem would do, would make it too easy for spy-ware to disable
> the LED.
> 
> To avoid this have the sensor driver directly control the LED.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note an additional advantage of directly controlling the GPIO is that
> GPIOs are tied directly to consumer devices. Where as with a LED class
> device, there would need to be some mechanism to tie the right LED
> (e.g front or back) to the right sensor.

Thanks for the patch.

This approach has the drawback that support needs to be added for each
sensor separately. Any idea how many sensor drivers might need this?

Most implementations have privacy LED hard-wired to the sensor's power
rails so it'll be lit whenever the sensor is powered on.

If there would be more than just a couple of these I'd instead create a LED
class device and hook it up to the sensor in V4L2.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Hans de Goede]

Hi Sakari,

On 11/30/22 14:41, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
>> Add support for a privacy-led GPIO.
>>
>> Making the privacy LED to controlable from userspace, as using the LED
>> class subsystem would do, would make it too easy for spy-ware to disable
>> the LED.
>>
>> To avoid this have the sensor driver directly control the LED.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note an additional advantage of directly controlling the GPIO is that
>> GPIOs are tied directly to consumer devices. Where as with a LED class
>> device, there would need to be some mechanism to tie the right LED
>> (e.g front or back) to the right sensor.
> 
> Thanks for the patch.
> 
> This approach has the drawback that support needs to be added for each
> sensor separately. Any idea how many sensor drivers might need this?

Quite a few probably. But as discussed here I plan to write a generic
sensor_power helper library since many sensor drivers have a lot of
boilerplate code to get clks + regulators + enable/reset gpios. The plan
is to add support for a "privacy-led" to this library so that all sensors
which use this get support for free.

Laurent pointed out that some sensors may have more complex power-up
sequence demands, which is true. But looking at existing drivers
then many follow a std simple pattern which can be supported in
a helper-library.

> Most implementations have privacy LED hard-wired to the sensor's power
> rails so it'll be lit whenever the sensor is powered on.
> 
> If there would be more than just a couple of these I'd instead create a LED
> class device and hook it up to the sensor in V4L2.


A LED cladd device will allow userspace to override the privacy-led
value which is considered bad from a privacy point of view. This
was actually already discussed here:

https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/

See the part of the thread on the cover-letter with Dan, Laurent
and me participating.

And a LED class device also will be a challenge to bind to the right
sensor on devices with more then one sensor, where as mentioned
above using GPIO-mappings give us the binding to the right sensor
for free.

Regards,

Hans

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Sakari Ailus]

Hi Hans,

On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
> Hi Sakari,
> 
> On 11/30/22 14:41, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
> >> Add support for a privacy-led GPIO.
> >>
> >> Making the privacy LED to controlable from userspace, as using the LED
> >> class subsystem would do, would make it too easy for spy-ware to disable
> >> the LED.
> >>
> >> To avoid this have the sensor driver directly control the LED.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Note an additional advantage of directly controlling the GPIO is that
> >> GPIOs are tied directly to consumer devices. Where as with a LED class
> >> device, there would need to be some mechanism to tie the right LED
> >> (e.g front or back) to the right sensor.
> > 
> > Thanks for the patch.
> > 
> > This approach has the drawback that support needs to be added for each
> > sensor separately. Any idea how many sensor drivers might need this?
> 
> Quite a few probably. But as discussed here I plan to write a generic
> sensor_power helper library since many sensor drivers have a lot of
> boilerplate code to get clks + regulators + enable/reset gpios. The plan
> is to add support for a "privacy-led" to this library so that all sensors
> which use this get support for free.

I'm not sure how well this could be generalised. While most sensors do
something similar there are subtle differences. If those can be taken into
account I guess it should be doable. But would it simplify things or reduce
the number of lines of code as a whole?

The privacy LED is separate from sensor, including its power on/off
sequences which suggests it could be at least as well be handled
separately.

> 
> Laurent pointed out that some sensors may have more complex power-up
> sequence demands, which is true. But looking at existing drivers
> then many follow a std simple pattern which can be supported in
> a helper-library.
> 
> > Most implementations have privacy LED hard-wired to the sensor's power
> > rails so it'll be lit whenever the sensor is powered on.
> > 
> > If there would be more than just a couple of these I'd instead create a LED
> > class device and hook it up to the sensor in V4L2.
> 
> 
> A LED cladd device will allow userspace to override the privacy-led
> value which is considered bad from a privacy point of view. This
> was actually already discussed here:
> 
> https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/
> 
> See the part of the thread on the cover-letter with Dan, Laurent
> and me participating.
> 
> And a LED class device also will be a challenge to bind to the right
> sensor on devices with more then one sensor, where as mentioned
> above using GPIO-mappings give us the binding to the right sensor
> for free.

Whether the privacy LED is controlled via the LED framework or GPIO doesn't
really matter from this PoV, it could be controlled via the V4L2 framework
in both cases. It might not be very pretty but I think I'd prefer that than
putting this in either drivers or some sensor power sequence helper
library.

-- 
Kind regardsm

Sakari Ailus

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Laurent Pinchart]

On Wed, Nov 30, 2022 at 02:52:50PM +0000, Sakari Ailus wrote:
> On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
> > On 11/30/22 14:41, Sakari Ailus wrote:
> > > On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
> > >> Add support for a privacy-led GPIO.
> > >>
> > >> Making the privacy LED to controlable from userspace, as using the LED
> > >> class subsystem would do, would make it too easy for spy-ware to disable
> > >> the LED.
> > >>
> > >> To avoid this have the sensor driver directly control the LED.
> > >>
> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >> ---
> > >> Note an additional advantage of directly controlling the GPIO is that
> > >> GPIOs are tied directly to consumer devices. Where as with a LED class
> > >> device, there would need to be some mechanism to tie the right LED
> > >> (e.g front or back) to the right sensor.
> > > 
> > > Thanks for the patch.
> > > 
> > > This approach has the drawback that support needs to be added for each
> > > sensor separately. Any idea how many sensor drivers might need this?
> > 
> > Quite a few probably. But as discussed here I plan to write a generic
> > sensor_power helper library since many sensor drivers have a lot of
> > boilerplate code to get clks + regulators + enable/reset gpios. The plan
> > is to add support for a "privacy-led" to this library so that all sensors
> > which use this get support for free.
> 
> I'm not sure how well this could be generalised. While most sensors do
> something similar there are subtle differences. If those can be taken into
> account I guess it should be doable. But would it simplify things or reduce
> the number of lines of code as a whole?

While I think we need a camera sensor helper, I also doubt managing the
power sequence in the helper would help much. The privacy LED, however,
could be handled there.

> The privacy LED is separate from sensor, including its power on/off
> sequences which suggests it could be at least as well be handled
> separately.

And if the privacy LED is controllable through a GPIO, I think it should
be turned on at stream on time, not at power on time. That would allow
things like reading the OTP data from the sensor without flashing the
privacy LED.

> > Laurent pointed out that some sensors may have more complex power-up
> > sequence demands, which is true. But looking at existing drivers
> > then many follow a std simple pattern which can be supported in
> > a helper-library.
> > 
> > > Most implementations have privacy LED hard-wired to the sensor's power
> > > rails so it'll be lit whenever the sensor is powered on.
> > > 
> > > If there would be more than just a couple of these I'd instead create a LED
> > > class device and hook it up to the sensor in V4L2.
> > 
> > A LED cladd device will allow userspace to override the privacy-led
> > value which is considered bad from a privacy point of view. This
> > was actually already discussed here:
> > 
> > https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/
> > 
> > See the part of the thread on the cover-letter with Dan, Laurent
> > and me participating.
> > 
> > And a LED class device also will be a challenge to bind to the right
> > sensor on devices with more then one sensor, where as mentioned
> > above using GPIO-mappings give us the binding to the right sensor
> > for free.
> 
> Whether the privacy LED is controlled via the LED framework or GPIO doesn't
> really matter from this PoV, it could be controlled via the V4L2 framework
> in both cases. It might not be very pretty but I think I'd prefer that than
> putting this in either drivers or some sensor power sequence helper
> library.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 05:20:11PM +0200, Laurent Pinchart wrote:
> On Wed, Nov 30, 2022 at 02:52:50PM +0000, Sakari Ailus wrote:
> > On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:

...

> > The privacy LED is separate from sensor, including its power on/off
> > sequences which suggests it could be at least as well be handled
> > separately.
> 
> And if the privacy LED is controllable through a GPIO, I think it should
> be turned on at stream on time, not at power on time. That would allow
> things like reading the OTP data from the sensor without flashing the
> privacy LED.

The malicious software may power up camera and drive it via user space /
separate code flow in the kernel, no?

I would stick with power on as it's the most secure side. Even if we 100% know
we are _not_ streaming this LED should indicate that it may be turned on at any
time, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Laurent Pinchart]

On Wed, Nov 30, 2022 at 06:07:51PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 05:20:11PM +0200, Laurent Pinchart wrote:
> > On Wed, Nov 30, 2022 at 02:52:50PM +0000, Sakari Ailus wrote:
> > > On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
> 
> ...
> 
> > > The privacy LED is separate from sensor, including its power on/off
> > > sequences which suggests it could be at least as well be handled
> > > separately.
> > 
> > And if the privacy LED is controllable through a GPIO, I think it should
> > be turned on at stream on time, not at power on time. That would allow
> > things like reading the OTP data from the sensor without flashing the
> > privacy LED.
> 
> The malicious software may power up camera and drive it via user space /
> separate code flow in the kernel, no?

With correctly written drivers, there should be no way to power up the
camera from userspace through the V4L2 API without starting streaming.
Also, programming the camera sensor won't be enough to capture images,
you need to deal with all the other camera-related IP cores which are
controlled through V4L2, and doing so will start streaming in the camera
sensor driver through the normal API anyway.

> I would stick with power on as it's the most secure side. Even if we 100% know
> we are _not_ streaming this LED should indicate that it may be turned on at any
> time, no?

Ideally, the privacy LED should be controlled automatically by the
hardware without software intervention, and should be wired to a camera
streaming signal. In many cases it's wired to the power rails instead,
which is extremely annoying. I'd rather avoid this annoyance when the
LED is GPIO-controlled.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Hans de Goede]

Hi,

On 11/30/22 16:20, Laurent Pinchart wrote:
> On Wed, Nov 30, 2022 at 02:52:50PM +0000, Sakari Ailus wrote:
>> On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
>>> On 11/30/22 14:41, Sakari Ailus wrote:
>>>> On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
>>>>> Add support for a privacy-led GPIO.
>>>>>
>>>>> Making the privacy LED to controlable from userspace, as using the LED
>>>>> class subsystem would do, would make it too easy for spy-ware to disable
>>>>> the LED.
>>>>>
>>>>> To avoid this have the sensor driver directly control the LED.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> Note an additional advantage of directly controlling the GPIO is that
>>>>> GPIOs are tied directly to consumer devices. Where as with a LED class
>>>>> device, there would need to be some mechanism to tie the right LED
>>>>> (e.g front or back) to the right sensor.
>>>>
>>>> Thanks for the patch.
>>>>
>>>> This approach has the drawback that support needs to be added for each
>>>> sensor separately. Any idea how many sensor drivers might need this?
>>>
>>> Quite a few probably. But as discussed here I plan to write a generic
>>> sensor_power helper library since many sensor drivers have a lot of
>>> boilerplate code to get clks + regulators + enable/reset gpios. The plan
>>> is to add support for a "privacy-led" to this library so that all sensors
>>> which use this get support for free.
>>
>> I'm not sure how well this could be generalised. While most sensors do
>> something similar there are subtle differences. If those can be taken into
>> account I guess it should be doable. But would it simplify things or reduce
>> the number of lines of code as a whole?
> 
> While I think we need a camera sensor helper, I also doubt managing the
> power sequence in the helper would help much. The privacy LED, however,
> could be handled there.

From a quick peek most of the sensor drivers I've looked at
(which is only a few) do:

-bulk-enable-regulators
-enable 1 clk
-set bunch of gpios
-sleep a bit

Since this requires to first get all the resources for this, which
needs error checking + reporting and then requires also error
checking the actual enabling + rollback on failure this is quite
a bit of code duplicated against many sensor drivers.

I agree that if a sensor does not fit in this model, that it then
should not use the helper and just open code the sequence but
I believe that for a bunch of sensor drivers with a simple
power-on sequence this can remove a bunch of code duplication.

Anways this is a clear case of the proof is in the tasting of
the pudding. So when I can make some time for this I'll submit
a patch series with the helper + converting a couple of sensors
(those which I can test) and then we can see from there.

>> The privacy LED is separate from sensor, including its power on/off
>> sequences which suggests it could be at least as well be handled
>> separately.
> 
> And if the privacy LED is controllable through a GPIO, I think it should
> be turned on at stream on time, not at power on time. That would allow
> things like reading the OTP data from the sensor without flashing the
> privacy LED.

Agreed.

Regards,

Hans

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Hans de Goede]

Hi,

On 11/30/22 15:52, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
>> Hi Sakari,
>>
>> On 11/30/22 14:41, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
>>>> Add support for a privacy-led GPIO.
>>>>
>>>> Making the privacy LED to controlable from userspace, as using the LED
>>>> class subsystem would do, would make it too easy for spy-ware to disable
>>>> the LED.
>>>>
>>>> To avoid this have the sensor driver directly control the LED.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Note an additional advantage of directly controlling the GPIO is that
>>>> GPIOs are tied directly to consumer devices. Where as with a LED class
>>>> device, there would need to be some mechanism to tie the right LED
>>>> (e.g front or back) to the right sensor.
>>>
>>> Thanks for the patch.
>>>
>>> This approach has the drawback that support needs to be added for each
>>> sensor separately. Any idea how many sensor drivers might need this?
>>
>> Quite a few probably. But as discussed here I plan to write a generic
>> sensor_power helper library since many sensor drivers have a lot of
>> boilerplate code to get clks + regulators + enable/reset gpios. The plan
>> is to add support for a "privacy-led" to this library so that all sensors
>> which use this get support for free.
> 
> I'm not sure how well this could be generalised. While most sensors do
> something similar there are subtle differences. If those can be taken into
> account I guess it should be doable. But would it simplify things or reduce
> the number of lines of code as a whole?
> 
> The privacy LED is separate from sensor, including its power on/off
> sequences which suggests it could be at least as well be handled
> separately.
> 
>>
>> Laurent pointed out that some sensors may have more complex power-up
>> sequence demands, which is true. But looking at existing drivers
>> then many follow a std simple pattern which can be supported in
>> a helper-library.
>>
>>> Most implementations have privacy LED hard-wired to the sensor's power
>>> rails so it'll be lit whenever the sensor is powered on.
>>>
>>> If there would be more than just a couple of these I'd instead create a LED
>>> class device and hook it up to the sensor in V4L2.
>>
>>
>> A LED cladd device will allow userspace to override the privacy-led
>> value which is considered bad from a privacy point of view. This
>> was actually already discussed here:
>>
>> https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/
>>
>> See the part of the thread on the cover-letter with Dan, Laurent
>> and me participating.
>>
>> And a LED class device also will be a challenge to bind to the right
>> sensor on devices with more then one sensor, where as mentioned
>> above using GPIO-mappings give us the binding to the right sensor
>> for free.
> 
> Whether the privacy LED is controlled via the LED framework or GPIO doesn't
> really matter from this PoV, it could be controlled via the V4L2 framework
> in both cases. It might not be very pretty but I think I'd prefer that than
> putting this in either drivers or some sensor power sequence helper
> library.

In sensors described in ACPI, esp. the straight forward described sensors
on atomisp2 devices, the GPIO resources inluding the LED one are listed
as resources of the i2c_client for the sensor.

And in a sense the same applies to later IPU3 / IPU6 devices where there
is a separate INT3472 device describing all the GPIOS which is also
tied to a specific sensor and we currently map all the GPIOs from
the INT3472 device to the sensor.

So it looks like that at least for x86/ACPI windows devices if the
LED has its own GPIO the hardware description clearly counts that
as part of the sensor's GPIOs. So the sensor driver has direct
access to this, where as any v4l2 framework driver would needed
to start poking inside the fwnode of the sensor which really
isn't pretty.

Where as if you look at this patch set adding the privacy-LED GPIO
from the INT3472 (IPU3 / IPU6) to the sensor fwnode is a 1 line change.

This really by far is the most KISS solution and we have so much
other things which need work that I believe that over-engineering
this is not doing ourselves any favours.

Regards,

Hans

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Laurent Pinchart]

Hi Hans,

On Wed, Nov 30, 2022 at 05:34:55PM +0100, Hans de Goede wrote:
> On 11/30/22 15:52, Sakari Ailus wrote:
> > On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
> >> On 11/30/22 14:41, Sakari Ailus wrote:
> >>> On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
> >>>> Add support for a privacy-led GPIO.
> >>>>
> >>>> Making the privacy LED to controlable from userspace, as using the LED
> >>>> class subsystem would do, would make it too easy for spy-ware to disable
> >>>> the LED.
> >>>>
> >>>> To avoid this have the sensor driver directly control the LED.
> >>>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>> ---
> >>>> Note an additional advantage of directly controlling the GPIO is that
> >>>> GPIOs are tied directly to consumer devices. Where as with a LED class
> >>>> device, there would need to be some mechanism to tie the right LED
> >>>> (e.g front or back) to the right sensor.
> >>>
> >>> Thanks for the patch.
> >>>
> >>> This approach has the drawback that support needs to be added for each
> >>> sensor separately. Any idea how many sensor drivers might need this?
> >>
> >> Quite a few probably. But as discussed here I plan to write a generic
> >> sensor_power helper library since many sensor drivers have a lot of
> >> boilerplate code to get clks + regulators + enable/reset gpios. The plan
> >> is to add support for a "privacy-led" to this library so that all sensors
> >> which use this get support for free.
> > 
> > I'm not sure how well this could be generalised. While most sensors do
> > something similar there are subtle differences. If those can be taken into
> > account I guess it should be doable. But would it simplify things or reduce
> > the number of lines of code as a whole?
> > 
> > The privacy LED is separate from sensor, including its power on/off
> > sequences which suggests it could be at least as well be handled
> > separately.
> > 
> >> Laurent pointed out that some sensors may have more complex power-up
> >> sequence demands, which is true. But looking at existing drivers
> >> then many follow a std simple pattern which can be supported in
> >> a helper-library.
> >>
> >>> Most implementations have privacy LED hard-wired to the sensor's power
> >>> rails so it'll be lit whenever the sensor is powered on.
> >>>
> >>> If there would be more than just a couple of these I'd instead create a LED
> >>> class device and hook it up to the sensor in V4L2.
> >>
> >> A LED cladd device will allow userspace to override the privacy-led
> >> value which is considered bad from a privacy point of view. This
> >> was actually already discussed here:
> >>
> >> https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/
> >>
> >> See the part of the thread on the cover-letter with Dan, Laurent
> >> and me participating.
> >>
> >> And a LED class device also will be a challenge to bind to the right
> >> sensor on devices with more then one sensor, where as mentioned
> >> above using GPIO-mappings give us the binding to the right sensor
> >> for free.
> > 
> > Whether the privacy LED is controlled via the LED framework or GPIO doesn't
> > really matter from this PoV, it could be controlled via the V4L2 framework
> > in both cases. It might not be very pretty but I think I'd prefer that than
> > putting this in either drivers or some sensor power sequence helper
> > library.
> 
> In sensors described in ACPI, esp. the straight forward described sensors
> on atomisp2 devices, the GPIO resources inluding the LED one are listed
> as resources of the i2c_client for the sensor.
> 
> And in a sense the same applies to later IPU3 / IPU6 devices where there
> is a separate INT3472 device describing all the GPIOS which is also
> tied to a specific sensor and we currently map all the GPIOs from
> the INT3472 device to the sensor.
> 
> So it looks like that at least for x86/ACPI windows devices if the
> LED has its own GPIO the hardware description clearly counts that
> as part of the sensor's GPIOs. So the sensor driver has direct
> access to this, where as any v4l2 framework driver would needed
> to start poking inside the fwnode of the sensor which really
> isn't pretty.

Let me try to understand it better. Looking at the platforms you mention
above, it seems that the way to retrieve the GPIO is platform-specific,
isn't it ? Can the atomisp2 (is that IPU2 ?), IPU3 and IPU6 expose the
GPIO in the same way, or would we need code that, for instance, acquires
the GPIO through different names (or even different APIs) for the same
sensor on different platforms ?

> Where as if you look at this patch set adding the privacy-LED GPIO
> from the INT3472 (IPU3 / IPU6) to the sensor fwnode is a 1 line change.
> 
> This really by far is the most KISS solution and we have so much
> other things which need work that I believe that over-engineering
> this is not doing ourselves any favours.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Hans de Goede]

Hi,

On 12/2/22 11:54, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Nov 30, 2022 at 05:34:55PM +0100, Hans de Goede wrote:
>> On 11/30/22 15:52, Sakari Ailus wrote:
>>> On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
>>>> On 11/30/22 14:41, Sakari Ailus wrote:
>>>>> On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
>>>>>> Add support for a privacy-led GPIO.
>>>>>>
>>>>>> Making the privacy LED to controlable from userspace, as using the LED
>>>>>> class subsystem would do, would make it too easy for spy-ware to disable
>>>>>> the LED.
>>>>>>
>>>>>> To avoid this have the sensor driver directly control the LED.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> Note an additional advantage of directly controlling the GPIO is that
>>>>>> GPIOs are tied directly to consumer devices. Where as with a LED class
>>>>>> device, there would need to be some mechanism to tie the right LED
>>>>>> (e.g front or back) to the right sensor.
>>>>>
>>>>> Thanks for the patch.
>>>>>
>>>>> This approach has the drawback that support needs to be added for each
>>>>> sensor separately. Any idea how many sensor drivers might need this?
>>>>
>>>> Quite a few probably. But as discussed here I plan to write a generic
>>>> sensor_power helper library since many sensor drivers have a lot of
>>>> boilerplate code to get clks + regulators + enable/reset gpios. The plan
>>>> is to add support for a "privacy-led" to this library so that all sensors
>>>> which use this get support for free.
>>>
>>> I'm not sure how well this could be generalised. While most sensors do
>>> something similar there are subtle differences. If those can be taken into
>>> account I guess it should be doable. But would it simplify things or reduce
>>> the number of lines of code as a whole?
>>>
>>> The privacy LED is separate from sensor, including its power on/off
>>> sequences which suggests it could be at least as well be handled
>>> separately.
>>>
>>>> Laurent pointed out that some sensors may have more complex power-up
>>>> sequence demands, which is true. But looking at existing drivers
>>>> then many follow a std simple pattern which can be supported in
>>>> a helper-library.
>>>>
>>>>> Most implementations have privacy LED hard-wired to the sensor's power
>>>>> rails so it'll be lit whenever the sensor is powered on.
>>>>>
>>>>> If there would be more than just a couple of these I'd instead create a LED
>>>>> class device and hook it up to the sensor in V4L2.
>>>>
>>>> A LED cladd device will allow userspace to override the privacy-led
>>>> value which is considered bad from a privacy point of view. This
>>>> was actually already discussed here:
>>>>
>>>> https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/
>>>>
>>>> See the part of the thread on the cover-letter with Dan, Laurent
>>>> and me participating.
>>>>
>>>> And a LED class device also will be a challenge to bind to the right
>>>> sensor on devices with more then one sensor, where as mentioned
>>>> above using GPIO-mappings give us the binding to the right sensor
>>>> for free.
>>>
>>> Whether the privacy LED is controlled via the LED framework or GPIO doesn't
>>> really matter from this PoV, it could be controlled via the V4L2 framework
>>> in both cases. It might not be very pretty but I think I'd prefer that than
>>> putting this in either drivers or some sensor power sequence helper
>>> library.
>>
>> In sensors described in ACPI, esp. the straight forward described sensors
>> on atomisp2 devices, the GPIO resources inluding the LED one are listed
>> as resources of the i2c_client for the sensor.
>>
>> And in a sense the same applies to later IPU3 / IPU6 devices where there
>> is a separate INT3472 device describing all the GPIOS which is also
>> tied to a specific sensor and we currently map all the GPIOs from
>> the INT3472 device to the sensor.
>>
>> So it looks like that at least for x86/ACPI windows devices if the
>> LED has its own GPIO the hardware description clearly counts that
>> as part of the sensor's GPIOs. So the sensor driver has direct
>> access to this, where as any v4l2 framework driver would needed
>> to start poking inside the fwnode of the sensor which really
>> isn't pretty.
> 
> Let me try to understand it better. Looking at the platforms you mention
> above, it seems that the way to retrieve the GPIO is platform-specific,
> isn't it ? Can the atomisp2 (is that IPU2 ?)

Yes, sorta, Intel back then called it an ISP not an IPU, but the
Android x86 code which we have for it also refers to work enabling
IPU3 support, so definitely the same lineage of ISPs/IPUs.

> , IPU3 and IPU6 expose the
> GPIO in the same way, or would we need code that, for instance, acquires
> the GPIO through different names (or even different APIs) for the same
> sensor on different platforms ?

Long answer:

On the atomisp2 platforms the GPIO is directly listed as a GPIO resource
of the i2c_client. Now ACPI resources use GPIO-indexes where as
the standard Linux GPIO APIs use GPIO names, so we need an index -> name
map in drivers/platform/x86 glue code.

Note the need for an index -> name map is standard for all GPIOs
on ACPI platforms.

On IPU3 / IPU6 most (all?) of the power-seq (and privacy-led) related
resources like GPIOs are all described in an INT3472 ACPI device,
and the drivers/platform/x86/intel/int3472/*.c code then adds
GPIO-lookup table entries to the sensor's i2c_client pointing
to these GPIOS.

So in the end for both the ISP2 and the IPU3/IPU6 which have
some code (outside of the media subsystem) abstracting away
all this platform specific shenanigans and mapping
the GPIOs to the sensor's i2c_client device so that a standard:

	sensor->pled_gpiod = gpiod_get(&i2c_client->dev, "privacy-led");

Call should work on all of ISP2/IPU3/IPU6 (and presumably also
IPU4 if we ever get around to that).

###

Short answer to your question:

"would we need code that, for instance, acquires the GPIO through
different names (or even different APIs) for the same
sensor on different platforms ?"

No the media subsystem sensor drivers should not need code to
deal with any platform differences, this should all be abstracted
away by the platform glue code under drivers/platform/x86, which
is glue which we need regardless of how we solve this.

With that glue in place, a simple / standard:

	sensor->pled_gpiod = gpiod_get(&i2c_client->dev, "privacy-led");

should work for all of ISP2 + IPU3 + IPU6 and this does already work
in my current testing done on IPU3 + IPU6.

Note this already works in my testing with both normal GPIOs from
the main SoC, as well as with the privacy LED attached to the TP68470
PMIC used for the back sensor on the Surface Go.

Regards,

Hans




> 
>> Where as if you look at this patch set adding the privacy-LED GPIO
>> from the INT3472 (IPU3 / IPU6) to the sensor fwnode is a 1 line change.
>>
>> This really by far is the most KISS solution and we have so much
>> other things which need work that I believe that over-engineering
>> this is not doing ourselves any favours.
> 

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Laurent Pinchart]

Hi Hans,

On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote:
> On 12/2/22 11:54, Laurent Pinchart wrote:
> > On Wed, Nov 30, 2022 at 05:34:55PM +0100, Hans de Goede wrote:
> >> On 11/30/22 15:52, Sakari Ailus wrote:
> >>> On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
> >>>> On 11/30/22 14:41, Sakari Ailus wrote:
> >>>>> On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
> >>>>>> Add support for a privacy-led GPIO.
> >>>>>>
> >>>>>> Making the privacy LED to controlable from userspace, as using the LED
> >>>>>> class subsystem would do, would make it too easy for spy-ware to disable
> >>>>>> the LED.
> >>>>>>
> >>>>>> To avoid this have the sensor driver directly control the LED.
> >>>>>>
> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>>> ---
> >>>>>> Note an additional advantage of directly controlling the GPIO is that
> >>>>>> GPIOs are tied directly to consumer devices. Where as with a LED class
> >>>>>> device, there would need to be some mechanism to tie the right LED
> >>>>>> (e.g front or back) to the right sensor.
> >>>>>
> >>>>> Thanks for the patch.
> >>>>>
> >>>>> This approach has the drawback that support needs to be added for each
> >>>>> sensor separately. Any idea how many sensor drivers might need this?
> >>>>
> >>>> Quite a few probably. But as discussed here I plan to write a generic
> >>>> sensor_power helper library since many sensor drivers have a lot of
> >>>> boilerplate code to get clks + regulators + enable/reset gpios. The plan
> >>>> is to add support for a "privacy-led" to this library so that all sensors
> >>>> which use this get support for free.
> >>>
> >>> I'm not sure how well this could be generalised. While most sensors do
> >>> something similar there are subtle differences. If those can be taken into
> >>> account I guess it should be doable. But would it simplify things or reduce
> >>> the number of lines of code as a whole?
> >>>
> >>> The privacy LED is separate from sensor, including its power on/off
> >>> sequences which suggests it could be at least as well be handled
> >>> separately.
> >>>
> >>>> Laurent pointed out that some sensors may have more complex power-up
> >>>> sequence demands, which is true. But looking at existing drivers
> >>>> then many follow a std simple pattern which can be supported in
> >>>> a helper-library.
> >>>>
> >>>>> Most implementations have privacy LED hard-wired to the sensor's power
> >>>>> rails so it'll be lit whenever the sensor is powered on.
> >>>>>
> >>>>> If there would be more than just a couple of these I'd instead create a LED
> >>>>> class device and hook it up to the sensor in V4L2.
> >>>>
> >>>> A LED cladd device will allow userspace to override the privacy-led
> >>>> value which is considered bad from a privacy point of view. This
> >>>> was actually already discussed here:
> >>>>
> >>>> https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/
> >>>>
> >>>> See the part of the thread on the cover-letter with Dan, Laurent
> >>>> and me participating.
> >>>>
> >>>> And a LED class device also will be a challenge to bind to the right
> >>>> sensor on devices with more then one sensor, where as mentioned
> >>>> above using GPIO-mappings give us the binding to the right sensor
> >>>> for free.
> >>>
> >>> Whether the privacy LED is controlled via the LED framework or GPIO doesn't
> >>> really matter from this PoV, it could be controlled via the V4L2 framework
> >>> in both cases. It might not be very pretty but I think I'd prefer that than
> >>> putting this in either drivers or some sensor power sequence helper
> >>> library.
> >>
> >> In sensors described in ACPI, esp. the straight forward described sensors
> >> on atomisp2 devices, the GPIO resources inluding the LED one are listed
> >> as resources of the i2c_client for the sensor.
> >>
> >> And in a sense the same applies to later IPU3 / IPU6 devices where there
> >> is a separate INT3472 device describing all the GPIOS which is also
> >> tied to a specific sensor and we currently map all the GPIOs from
> >> the INT3472 device to the sensor.
> >>
> >> So it looks like that at least for x86/ACPI windows devices if the
> >> LED has its own GPIO the hardware description clearly counts that
> >> as part of the sensor's GPIOs. So the sensor driver has direct
> >> access to this, where as any v4l2 framework driver would needed
> >> to start poking inside the fwnode of the sensor which really
> >> isn't pretty.
> > 
> > Let me try to understand it better. Looking at the platforms you mention
> > above, it seems that the way to retrieve the GPIO is platform-specific,
> > isn't it ? Can the atomisp2 (is that IPU2 ?)
> 
> Yes, sorta, Intel back then called it an ISP not an IPU, but the
> Android x86 code which we have for it also refers to work enabling
> IPU3 support, so definitely the same lineage of ISPs/IPUs.
> 
> > , IPU3 and IPU6 expose the
> > GPIO in the same way, or would we need code that, for instance, acquires
> > the GPIO through different names (or even different APIs) for the same
> > sensor on different platforms ?
> 
> Long answer:
> 
> On the atomisp2 platforms the GPIO is directly listed as a GPIO resource
> of the i2c_client. Now ACPI resources use GPIO-indexes where as
> the standard Linux GPIO APIs use GPIO names, so we need an index -> name
> map in drivers/platform/x86 glue code.
> 
> Note the need for an index -> name map is standard for all GPIOs
> on ACPI platforms.

It's funny how ARM platforms were criticized for their need of board
files, with x86/ACPI being revered as a saint. Now we have DT on ARM and
x86 needs board files :-)

> On IPU3 / IPU6 most (all?) of the power-seq (and privacy-led) related
> resources like GPIOs are all described in an INT3472 ACPI device,
> and the drivers/platform/x86/intel/int3472/*.c code then adds
> GPIO-lookup table entries to the sensor's i2c_client pointing
> to these GPIOS.
> 
> So in the end for both the ISP2 and the IPU3/IPU6 which have
> some code (outside of the media subsystem) abstracting away
> all this platform specific shenanigans and mapping
> the GPIOs to the sensor's i2c_client device so that a standard:
> 
> 	sensor->pled_gpiod = gpiod_get(&i2c_client->dev, "privacy-led");
> 
> Call should work on all of ISP2/IPU3/IPU6 (and presumably also
> IPU4 if we ever get around to that).
>
> ###
> 
> Short answer to your question:
> 
> "would we need code that, for instance, acquires the GPIO through
> different names (or even different APIs) for the same
> sensor on different platforms ?"
> 
> No the media subsystem sensor drivers should not need code to
> deal with any platform differences, this should all be abstracted
> away by the platform glue code under drivers/platform/x86, which
> is glue which we need regardless of how we solve this.
> 
> With that glue in place, a simple / standard:
> 
> 	sensor->pled_gpiod = gpiod_get(&i2c_client->dev, "privacy-led");
> 
> should work for all of ISP2 + IPU3 + IPU6 and this does already work
> in my current testing done on IPU3 + IPU6.

Can I assume that "privacy-led" will be the right GPIO name not only
across different platforms but also across different sensors ?

> Note this already works in my testing with both normal GPIOs from
> the main SoC, as well as with the privacy LED attached to the TP68470
> PMIC used for the back sensor on the Surface Go.
> 
> >> Where as if you look at this patch set adding the privacy-LED GPIO
> >> from the INT3472 (IPU3 / IPU6) to the sensor fwnode is a 1 line change.
> >>
> >> This really by far is the most KISS solution and we have so much
> >> other things which need work that I believe that over-engineering
> >> this is not doing ourselves any favours.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Andy Shevchenko]

On Fri, Dec 2, 2022 at 1:50 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote:
> > On 12/2/22 11:54, Laurent Pinchart wrote:

...

> > Note the need for an index -> name map is standard for all GPIOs
> > on ACPI platforms.
>
> It's funny how ARM platforms were criticized for their need of board
> files, with x86/ACPI being revered as a saint. Now we have DT on ARM and
> x86 needs board files :-)

I believe it's a misunderstanding here due to missing words at Hans'
statement, i..e.
"..., which do not provide the descriptions in _DSD() method."

So, no, x86 does not need board files generally speaking. The problem
here is some departments of some big companies that didn't get ACPI
properly or at all.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Laurent Pinchart]

On Fri, Dec 02, 2022 at 01:53:55PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 2, 2022 at 1:50 PM Laurent Pinchart wrote:
> > On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote:
> > > On 12/2/22 11:54, Laurent Pinchart wrote:
> 
> ...
> 
> > > Note the need for an index -> name map is standard for all GPIOs
> > > on ACPI platforms.
> >
> > It's funny how ARM platforms were criticized for their need of board
> > files, with x86/ACPI being revered as a saint. Now we have DT on ARM and
> > x86 needs board files :-)
> 
> I believe it's a misunderstanding here due to missing words at Hans'
> statement, i..e.
> "..., which do not provide the descriptions in _DSD() method."
> 
> So, no, x86 does not need board files generally speaking. The problem
> here is some departments of some big companies that didn't get ACPI
> properly or at all.

When it comes to camera support, that seems to cover an overwhelming
majority of systems, if not all of them.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Andy Shevchenko]

On Fri, Dec 2, 2022 at 2:14 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Fri, Dec 02, 2022 at 01:53:55PM +0200, Andy Shevchenko wrote:
> > On Fri, Dec 2, 2022 at 1:50 PM Laurent Pinchart wrote:
> > > On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote:
> > > > On 12/2/22 11:54, Laurent Pinchart wrote:

...

> > > > Note the need for an index -> name map is standard for all GPIOs
> > > > on ACPI platforms.
> > >
> > > It's funny how ARM platforms were criticized for their need of board
> > > files, with x86/ACPI being revered as a saint. Now we have DT on ARM and
> > > x86 needs board files :-)
> >
> > I believe it's a misunderstanding here due to missing words at Hans'
> > statement, i..e.
> > "..., which do not provide the descriptions in _DSD() method."
> >
> > So, no, x86 does not need board files generally speaking. The problem
> > here is some departments of some big companies that didn't get ACPI
> > properly or at all.
>
> When it comes to camera support, that seems to cover an overwhelming
> majority of systems, if not all of them.

Unfortunately :-(

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Sakari Ailus]

On Fri, Dec 02, 2022 at 02:14:46PM +0200, Laurent Pinchart wrote:
> On Fri, Dec 02, 2022 at 01:53:55PM +0200, Andy Shevchenko wrote:
> > On Fri, Dec 2, 2022 at 1:50 PM Laurent Pinchart wrote:
> > > On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote:
> > > > On 12/2/22 11:54, Laurent Pinchart wrote:
> > 
> > ...
> > 
> > > > Note the need for an index -> name map is standard for all GPIOs
> > > > on ACPI platforms.
> > >
> > > It's funny how ARM platforms were criticized for their need of board
> > > files, with x86/ACPI being revered as a saint. Now we have DT on ARM and
> > > x86 needs board files :-)
> > 
> > I believe it's a misunderstanding here due to missing words at Hans'
> > statement, i..e.
> > "..., which do not provide the descriptions in _DSD() method."
> > 
> > So, no, x86 does not need board files generally speaking. The problem
> > here is some departments of some big companies that didn't get ACPI
> > properly or at all.
> 
> When it comes to camera support, that seems to cover an overwhelming
> majority of systems, if not all of them.

Not those shipped with ChromeOS. In the future the BIOS folks would ideally
base this on MIPI DisCo for Imaging. The spec should be out soon:

<URL:https://www.mipi.org/specifications/mipi-disco-imaging>

-- 
Sakari Ailus

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Hans de Goede]

Hi,

On 12/2/22 12:49, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Fri, Dec 02, 2022 at 12:21:12PM +0100, Hans de Goede wrote:
>> On 12/2/22 11:54, Laurent Pinchart wrote:
>>> On Wed, Nov 30, 2022 at 05:34:55PM +0100, Hans de Goede wrote:
>>>> On 11/30/22 15:52, Sakari Ailus wrote:
>>>>> On Wed, Nov 30, 2022 at 02:56:46PM +0100, Hans de Goede wrote:
>>>>>> On 11/30/22 14:41, Sakari Ailus wrote:
>>>>>>> On Wed, Nov 30, 2022 at 12:11:44AM +0100, Hans de Goede wrote:
>>>>>>>> Add support for a privacy-led GPIO.
>>>>>>>>
>>>>>>>> Making the privacy LED to controlable from userspace, as using the LED
>>>>>>>> class subsystem would do, would make it too easy for spy-ware to disable
>>>>>>>> the LED.
>>>>>>>>
>>>>>>>> To avoid this have the sensor driver directly control the LED.
>>>>>>>>
>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>> ---
>>>>>>>> Note an additional advantage of directly controlling the GPIO is that
>>>>>>>> GPIOs are tied directly to consumer devices. Where as with a LED class
>>>>>>>> device, there would need to be some mechanism to tie the right LED
>>>>>>>> (e.g front or back) to the right sensor.
>>>>>>>
>>>>>>> Thanks for the patch.
>>>>>>>
>>>>>>> This approach has the drawback that support needs to be added for each
>>>>>>> sensor separately. Any idea how many sensor drivers might need this?
>>>>>>
>>>>>> Quite a few probably. But as discussed here I plan to write a generic
>>>>>> sensor_power helper library since many sensor drivers have a lot of
>>>>>> boilerplate code to get clks + regulators + enable/reset gpios. The plan
>>>>>> is to add support for a "privacy-led" to this library so that all sensors
>>>>>> which use this get support for free.
>>>>>
>>>>> I'm not sure how well this could be generalised. While most sensors do
>>>>> something similar there are subtle differences. If those can be taken into
>>>>> account I guess it should be doable. But would it simplify things or reduce
>>>>> the number of lines of code as a whole?
>>>>>
>>>>> The privacy LED is separate from sensor, including its power on/off
>>>>> sequences which suggests it could be at least as well be handled
>>>>> separately.
>>>>>
>>>>>> Laurent pointed out that some sensors may have more complex power-up
>>>>>> sequence demands, which is true. But looking at existing drivers
>>>>>> then many follow a std simple pattern which can be supported in
>>>>>> a helper-library.
>>>>>>
>>>>>>> Most implementations have privacy LED hard-wired to the sensor's power
>>>>>>> rails so it'll be lit whenever the sensor is powered on.
>>>>>>>
>>>>>>> If there would be more than just a couple of these I'd instead create a LED
>>>>>>> class device and hook it up to the sensor in V4L2.
>>>>>>
>>>>>> A LED cladd device will allow userspace to override the privacy-led
>>>>>> value which is considered bad from a privacy point of view. This
>>>>>> was actually already discussed here:
>>>>>>
>>>>>> https://lore.kernel.org/platform-driver-x86/e5d8913c-13ba-3b11-94bc-5d1ee1d736b0@ideasonboard.com/
>>>>>>
>>>>>> See the part of the thread on the cover-letter with Dan, Laurent
>>>>>> and me participating.
>>>>>>
>>>>>> And a LED class device also will be a challenge to bind to the right
>>>>>> sensor on devices with more then one sensor, where as mentioned
>>>>>> above using GPIO-mappings give us the binding to the right sensor
>>>>>> for free.
>>>>>
>>>>> Whether the privacy LED is controlled via the LED framework or GPIO doesn't
>>>>> really matter from this PoV, it could be controlled via the V4L2 framework
>>>>> in both cases. It might not be very pretty but I think I'd prefer that than
>>>>> putting this in either drivers or some sensor power sequence helper
>>>>> library.
>>>>
>>>> In sensors described in ACPI, esp. the straight forward described sensors
>>>> on atomisp2 devices, the GPIO resources inluding the LED one are listed
>>>> as resources of the i2c_client for the sensor.
>>>>
>>>> And in a sense the same applies to later IPU3 / IPU6 devices where there
>>>> is a separate INT3472 device describing all the GPIOS which is also
>>>> tied to a specific sensor and we currently map all the GPIOs from
>>>> the INT3472 device to the sensor.
>>>>
>>>> So it looks like that at least for x86/ACPI windows devices if the
>>>> LED has its own GPIO the hardware description clearly counts that
>>>> as part of the sensor's GPIOs. So the sensor driver has direct
>>>> access to this, where as any v4l2 framework driver would needed
>>>> to start poking inside the fwnode of the sensor which really
>>>> isn't pretty.
>>>
>>> Let me try to understand it better. Looking at the platforms you mention
>>> above, it seems that the way to retrieve the GPIO is platform-specific,
>>> isn't it ? Can the atomisp2 (is that IPU2 ?)
>>
>> Yes, sorta, Intel back then called it an ISP not an IPU, but the
>> Android x86 code which we have for it also refers to work enabling
>> IPU3 support, so definitely the same lineage of ISPs/IPUs.
>>
>>> , IPU3 and IPU6 expose the
>>> GPIO in the same way, or would we need code that, for instance, acquires
>>> the GPIO through different names (or even different APIs) for the same
>>> sensor on different platforms ?
>>
>> Long answer:
>>
>> On the atomisp2 platforms the GPIO is directly listed as a GPIO resource
>> of the i2c_client. Now ACPI resources use GPIO-indexes where as
>> the standard Linux GPIO APIs use GPIO names, so we need an index -> name
>> map in drivers/platform/x86 glue code.
>>
>> Note the need for an index -> name map is standard for all GPIOs
>> on ACPI platforms.
> 
> It's funny how ARM platforms were criticized for their need of board
> files, with x86/ACPI being revered as a saint. Now we have DT on ARM and
> x86 needs board files :-)

Yes this is a bit painful. Although most of the INT3472 code is not
board specific, it calls _DSM (device-specific-methods) which
the windows drivers use and then translates that to GPIO mappings.

For the non separate PMIC case the _DSM gives us a u8 containing a type
for each GPIO listed, which can be one of: /reset, clk-enable,
regulator-enable, /powerdown or privacy-led and then we "inject" those
into the fwnode for the i2c_client (with the clk / regulator using
the clk/regulator framework).

>> On IPU3 / IPU6 most (all?) of the power-seq (and privacy-led) related
>> resources like GPIOs are all described in an INT3472 ACPI device,
>> and the drivers/platform/x86/intel/int3472/*.c code then adds
>> GPIO-lookup table entries to the sensor's i2c_client pointing
>> to these GPIOS.
>>
>> So in the end for both the ISP2 and the IPU3/IPU6 which have
>> some code (outside of the media subsystem) abstracting away
>> all this platform specific shenanigans and mapping
>> the GPIOs to the sensor's i2c_client device so that a standard:
>>
>> 	sensor->pled_gpiod = gpiod_get(&i2c_client->dev, "privacy-led");
>>
>> Call should work on all of ISP2/IPU3/IPU6 (and presumably also
>> IPU4 if we ever get around to that).
>>
>> ###
>>
>> Short answer to your question:
>>
>> "would we need code that, for instance, acquires the GPIO through
>> different names (or even different APIs) for the same
>> sensor on different platforms ?"
>>
>> No the media subsystem sensor drivers should not need code to
>> deal with any platform differences, this should all be abstracted
>> away by the platform glue code under drivers/platform/x86, which
>> is glue which we need regardless of how we solve this.
>>
>> With that glue in place, a simple / standard:
>>
>> 	sensor->pled_gpiod = gpiod_get(&i2c_client->dev, "privacy-led");
>>
>> should work for all of ISP2 + IPU3 + IPU6 and this does already work
>> in my current testing done on IPU3 + IPU6.
> 
> Can I assume that "privacy-led" will be the right GPIO name not only
> across different platforms but also across different sensors ?

Yes. After this series we always map GPIO for which the _DSM returns 
the privacy-led value in the returned type field to a "privacy-led"
GPIO, the mapping code for this is sensor independent.

Regards,

Hans

Re: [PATCH 1/6] media: ov5693: Add support for a privacy-led GPIO

[Sakari Ailus]

Hi Hans,

On Wed, Nov 30, 2022 at 05:34:55PM +0100, Hans de Goede wrote:
> So it looks like that at least for x86/ACPI windows devices if the
> LED has its own GPIO the hardware description clearly counts that
> as part of the sensor's GPIOs. So the sensor driver has direct
> access to this, where as any v4l2 framework driver would needed
> to start poking inside the fwnode of the sensor which really
> isn't pretty.

Most of the common (e.g. camera sensor related) properties are parsed by
the V4L2 framework, not by drivers. I'm not saying no to having privacy-led
parsing in a single driver but instead of adding more of this in drivers we
should have a common solution for this.

-- 
Kind regards,

Sakari Ailus

[PATCH 2/6] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede]

Add a helper function to map the type returned by the _DSM
method to a function name + the default polarity for that function.

And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN
cases into a single generic case.

This is a preparation patch for further GPIO mapping changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Make the helper function doing the type -> function mapping,
  also return a default polarity for the function.
---
 drivers/platform/x86/intel/int3472/discrete.c | 44 +++++++++++++++----
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 974a132db651..1eb053d13353 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -184,6 +184,35 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
 	return 0;
 }
 
+static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
+{
+	switch (type) {
+	case INT3472_GPIO_TYPE_RESET:
+		*func = "reset";
+		*polarity = GPIO_ACTIVE_LOW;
+		break;
+	case INT3472_GPIO_TYPE_POWERDOWN:
+		*func = "powerdown";
+		*polarity = GPIO_ACTIVE_LOW;
+		break;
+	case INT3472_GPIO_TYPE_CLK_ENABLE:
+		*func = "clk-enable";
+		*polarity = GPIO_ACTIVE_HIGH;
+		break;
+	case INT3472_GPIO_TYPE_PRIVACY_LED:
+		*func = "privacy-led";
+		*polarity = GPIO_ACTIVE_HIGH;
+		break;
+	case INT3472_GPIO_TYPE_POWER_ENABLE:
+		*func = "power-enable";
+		*polarity = GPIO_ACTIVE_HIGH;
+		break;
+	default:
+		*func = "unknown";
+		*polarity = GPIO_ACTIVE_HIGH;
+	}
+}
+
 /**
  * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
  * @ares: A pointer to a &struct acpi_resource
@@ -223,6 +252,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	struct acpi_resource_gpio *agpio;
 	union acpi_object *obj;
 	const char *err_msg;
+	const char *func;
+	u32 polarity;
 	int ret;
 	u8 type;
 
@@ -246,19 +277,14 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	type = obj->integer.value & 0xff;
 
+	int3472_get_func_and_polarity(type, &func, &polarity);
+
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
-		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "reset",
-						     GPIO_ACTIVE_LOW);
-		if (ret)
-			err_msg = "Failed to map reset pin to sensor\n";
-
-		break;
 	case INT3472_GPIO_TYPE_POWERDOWN:
-		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "powerdown",
-						     GPIO_ACTIVE_LOW);
+		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
 		if (ret)
-			err_msg = "Failed to map powerdown pin to sensor\n";
+			err_msg = "Failed to map GPIO pin to sensor\n";
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-- 
2.38.1

Re: [PATCH 2/6] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add a helper function to map the type returned by the _DSM
> method to a function name + the default polarity for that function.
>
> And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN
> cases into a single generic case.
>
> This is a preparation patch for further GPIO mapping changes.

...

> +       switch (type) {
> +       case INT3472_GPIO_TYPE_RESET:
> +               *func = "reset";
> +               *polarity = GPIO_ACTIVE_LOW;
> +               break;
> +       case INT3472_GPIO_TYPE_POWERDOWN:
> +               *func = "powerdown";
> +               *polarity = GPIO_ACTIVE_LOW;
> +               break;
> +       case INT3472_GPIO_TYPE_CLK_ENABLE:
> +               *func = "clk-enable";
> +               *polarity = GPIO_ACTIVE_HIGH;
> +               break;
> +       case INT3472_GPIO_TYPE_PRIVACY_LED:
> +               *func = "privacy-led";
> +               *polarity = GPIO_ACTIVE_HIGH;
> +               break;
> +       case INT3472_GPIO_TYPE_POWER_ENABLE:
> +               *func = "power-enable";
> +               *polarity = GPIO_ACTIVE_HIGH;
> +               break;
> +       default:
> +               *func = "unknown";
> +               *polarity = GPIO_ACTIVE_HIGH;

A nit-pick: In long term maintenance it's always good to have a break
statement even in the default case.

> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/6] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede]

Hi,

On 11/30/22 10:49, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add a helper function to map the type returned by the _DSM
>> method to a function name + the default polarity for that function.
>>
>> And fold the INT3472_GPIO_TYPE_RESET and INT3472_GPIO_TYPE_POWERDOWN
>> cases into a single generic case.
>>
>> This is a preparation patch for further GPIO mapping changes.
> 
> ...
> 
>> +       switch (type) {
>> +       case INT3472_GPIO_TYPE_RESET:
>> +               *func = "reset";
>> +               *polarity = GPIO_ACTIVE_LOW;
>> +               break;
>> +       case INT3472_GPIO_TYPE_POWERDOWN:
>> +               *func = "powerdown";
>> +               *polarity = GPIO_ACTIVE_LOW;
>> +               break;
>> +       case INT3472_GPIO_TYPE_CLK_ENABLE:
>> +               *func = "clk-enable";
>> +               *polarity = GPIO_ACTIVE_HIGH;
>> +               break;
>> +       case INT3472_GPIO_TYPE_PRIVACY_LED:
>> +               *func = "privacy-led";
>> +               *polarity = GPIO_ACTIVE_HIGH;
>> +               break;
>> +       case INT3472_GPIO_TYPE_POWER_ENABLE:
>> +               *func = "power-enable";
>> +               *polarity = GPIO_ACTIVE_HIGH;
>> +               break;
>> +       default:
>> +               *func = "unknown";
>> +               *polarity = GPIO_ACTIVE_HIGH;
> 
> A nit-pick: In long term maintenance it's always good to have a break
> statement even in the default case.

Ack, I'll add this when merging this (unless there are other / bigger
reasons for a v2).

Regards,

Hans

[PATCH 3/6] platform/x86: int3472/discrete: Treat privacy LED as regular GPIO

[Hans de Goede]

On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
X1 Nano gen 2 there is no clock-enable pin, triggering the:
"No clk GPIO. The privacy LED won't work" warning and causing the privacy
LED to not work.

Fix this by treating the privacy LED as a regular GPIO rather then
integrating it with the registered clock.

Note this relies on the ov5693 driver change to support an (optional)
privacy-led GPIO to avoid the front cam privacy LED regressing on some
models.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     |  3 --
 drivers/platform/x86/intel/int3472/common.h   |  1 -
 drivers/platform/x86/intel/int3472/discrete.c | 46 ++++---------------
 3 files changed, 8 insertions(+), 42 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 1cf958983e86..e61119b17677 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -23,8 +23,6 @@ static int skl_int3472_clk_prepare(struct clk_hw *hw)
 	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
 
 	gpiod_set_value_cansleep(clk->ena_gpio, 1);
-	gpiod_set_value_cansleep(clk->led_gpio, 1);
-
 	return 0;
 }
 
@@ -33,7 +31,6 @@ static void skl_int3472_clk_unprepare(struct clk_hw *hw)
 	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
 
 	gpiod_set_value_cansleep(clk->ena_gpio, 0);
-	gpiod_set_value_cansleep(clk->led_gpio, 0);
 }
 
 static int skl_int3472_clk_enable(struct clk_hw *hw)
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 53270d19c73a..c31321a586d4 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -96,7 +96,6 @@ struct int3472_discrete_device {
 		struct clk_hw clk_hw;
 		struct clk_lookup *cl;
 		struct gpio_desc *ena_gpio;
-		struct gpio_desc *led_gpio;
 		u32 frequency;
 	} clock;
 
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 1eb053d13353..7887c6a4035e 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -155,33 +155,19 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 }
 
 static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
-				       struct acpi_resource_gpio *agpio, u8 type)
+				       struct acpi_resource_gpio *agpio)
 {
 	char *path = agpio->resource_source.string_ptr;
 	u16 pin = agpio->pin_table[0];
 	struct gpio_desc *gpio;
 
-	switch (type) {
-	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
-		if (IS_ERR(gpio))
-			return (PTR_ERR(gpio));
-
-		int3472->clock.ena_gpio = gpio;
-		break;
-	case INT3472_GPIO_TYPE_PRIVACY_LED:
-		gpio = acpi_get_and_request_gpiod(path, pin, "int3472,privacy-led");
-		if (IS_ERR(gpio))
-			return (PTR_ERR(gpio));
+	gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
+	if (IS_ERR(gpio))
+		return (PTR_ERR(gpio));
 
-		int3472->clock.led_gpio = gpio;
-		break;
-	default:
-		dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type);
-		break;
-	}
+	int3472->clock.ena_gpio = gpio;
 
-	return 0;
+	return skl_int3472_register_clock(int3472);
 }
 
 static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
@@ -282,14 +268,14 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
+	case INT3472_GPIO_TYPE_PRIVACY_LED:
 		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
 		if (ret)
 			err_msg = "Failed to map GPIO pin to sensor\n";
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-	case INT3472_GPIO_TYPE_PRIVACY_LED:
-		ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
+		ret = skl_int3472_map_gpio_to_clk(int3472, agpio);
 		if (ret)
 			err_msg = "Failed to map GPIO to clock\n";
 
@@ -336,21 +322,6 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
 
 	acpi_dev_free_resource_list(&resource_list);
 
-	/*
-	 * If we find no clock enable GPIO pin then the privacy LED won't work.
-	 * We've never seen that situation, but it's possible. Warn the user so
-	 * it's clear what's happened.
-	 */
-	if (int3472->clock.ena_gpio) {
-		ret = skl_int3472_register_clock(int3472);
-		if (ret)
-			return ret;
-	} else {
-		if (int3472->clock.led_gpio)
-			dev_warn(int3472->dev,
-				 "No clk GPIO. The privacy LED won't work\n");
-	}
-
 	int3472->gpios.dev_id = int3472->sensor_name;
 	gpiod_add_lookup_table(&int3472->gpios);
 
@@ -367,7 +338,6 @@ static int skl_int3472_discrete_remove(struct platform_device *pdev)
 		skl_int3472_unregister_clock(int3472);
 
 	gpiod_put(int3472->clock.ena_gpio);
-	gpiod_put(int3472->clock.led_gpio);
 
 	skl_int3472_unregister_regulator(int3472);
 
-- 
2.38.1

Re: [PATCH 3/6] platform/x86: int3472/discrete: Treat privacy LED as regular GPIO

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
> X1 Nano gen 2 there is no clock-enable pin, triggering the:
> "No clk GPIO. The privacy LED won't work" warning and causing the privacy
> LED to not work.
>
> Fix this by treating the privacy LED as a regular GPIO rather then
> integrating it with the registered clock.
>
> Note this relies on the ov5693 driver change to support an (optional)
> privacy-led GPIO to avoid the front cam privacy LED regressing on some
> models.

...

> -       case INT3472_GPIO_TYPE_PRIVACY_LED:
> -               gpio = acpi_get_and_request_gpiod(path, pin, "int3472,privacy-led");
> -               if (IS_ERR(gpio))
> -                       return (PTR_ERR(gpio));
>
> -               int3472->clock.led_gpio = gpio;
> -               break;

I'm not sure how the previous patch makes this one work without
regressions. We have a "privacy-led" GPIO name there and here it used
to be with a prefix. Maybe I'm missing something...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] platform/x86: int3472/discrete: Treat privacy LED as regular GPIO

[Hans de Goede]

Hi,

On 11/30/22 10:54, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
>> X1 Nano gen 2 there is no clock-enable pin, triggering the:
>> "No clk GPIO. The privacy LED won't work" warning and causing the privacy
>> LED to not work.
>>
>> Fix this by treating the privacy LED as a regular GPIO rather then
>> integrating it with the registered clock.
>>
>> Note this relies on the ov5693 driver change to support an (optional)
>> privacy-led GPIO to avoid the front cam privacy LED regressing on some
>> models.
> 
> ...
> 
>> -       case INT3472_GPIO_TYPE_PRIVACY_LED:
>> -               gpio = acpi_get_and_request_gpiod(path, pin, "int3472,privacy-led");
>> -               if (IS_ERR(gpio))
>> -                       return (PTR_ERR(gpio));
>>
>> -               int3472->clock.led_gpio = gpio;
>> -               break;
> 
> I'm not sure how the previous patch makes this one work without
> regressions. We have a "privacy-led" GPIO name there and here it used
> to be with a prefix. Maybe I'm missing something...

The GPIO used to be controlled as part of the clk-provider,
and the "int3472,privacy-led" name was the name of the consumer
of the GPIO shown in /sys/kernel/debug/gpio. The "int3472,privacy-led"
name has no lookup meaning since the pin is directly looked up by
GPIO chip ACPI path + pin offset here.

Since not all devices with a privacy LED also have a clk-enable GPIO
and thus a clk provider this did not work anywhere.

So this patch removes the code which controls the privacy LED
through the clk-provider (which used the "int3472,privacy-led"
and instead now adds an entry to the GPIO lookup table attached
to the sensor. That new GPIO lookup table entry uses the name
"privacy-led" since the LED no now longer is controlled by
the INT3472 code (*).  The matching sensor driver patch
(patch 1/6) to make the sensor driver directly control the
privacy-led also uses "privacy-led" when calling gpiod_get()
for it.

I hope this helps explain.

Regards,

Hans


*) all the INT3472 code now does is add the lookup table entry
gpio lookup table 

Re: [PATCH 3/6] platform/x86: int3472/discrete: Treat privacy LED as regular GPIO

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 11:34:57AM +0100, Hans de Goede wrote:
> On 11/30/22 10:54, Andy Shevchenko wrote:
> > On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad
> >> X1 Nano gen 2 there is no clock-enable pin, triggering the:
> >> "No clk GPIO. The privacy LED won't work" warning and causing the privacy
> >> LED to not work.
> >>
> >> Fix this by treating the privacy LED as a regular GPIO rather then
> >> integrating it with the registered clock.
> >>
> >> Note this relies on the ov5693 driver change to support an (optional)
> >> privacy-led GPIO to avoid the front cam privacy LED regressing on some
> >> models.
> > 
> > ...
> > 
> >> -       case INT3472_GPIO_TYPE_PRIVACY_LED:
> >> -               gpio = acpi_get_and_request_gpiod(path, pin, "int3472,privacy-led");
> >> -               if (IS_ERR(gpio))
> >> -                       return (PTR_ERR(gpio));
> >>
> >> -               int3472->clock.led_gpio = gpio;
> >> -               break;
> > 
> > I'm not sure how the previous patch makes this one work without
> > regressions. We have a "privacy-led" GPIO name there and here it used
> > to be with a prefix. Maybe I'm missing something...
> 
> The GPIO used to be controlled as part of the clk-provider,
> and the "int3472,privacy-led" name was the name of the consumer
> of the GPIO shown in /sys/kernel/debug/gpio. The "int3472,privacy-led"
> name has no lookup meaning since the pin is directly looked up by
> GPIO chip ACPI path + pin offset here.
> 
> Since not all devices with a privacy LED also have a clk-enable GPIO
> and thus a clk provider this did not work anywhere.
> 
> So this patch removes the code which controls the privacy LED
> through the clk-provider (which used the "int3472,privacy-led"
> and instead now adds an entry to the GPIO lookup table attached
> to the sensor. That new GPIO lookup table entry uses the name
> "privacy-led" since the LED no now longer is controlled by
> the INT3472 code (*).  The matching sensor driver patch
> (patch 1/6) to make the sensor driver directly control the
> privacy-led also uses "privacy-led" when calling gpiod_get()
> for it.
> 
> I hope this helps explain.

Definitely, thanks!

> *) all the INT3472 code now does is add the lookup table entry
> gpio lookup table

-- 
With Best Regards,
Andy Shevchenko

[PATCH 4/6] platform/x86: int3472/discrete: Move GPIO request to skl_int3472_register_clock()

[Hans de Goede]

Move the requesting of the clk-enable GPIO to skl_int3472_register_clock()
(and move the gpiod_put to unregister).

This mirrors the GPIO handling in skl_int3472_register_regulator() and
allows removing  skl_int3472_map_gpio_to_clk() from discrete.c .

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 23 +++++++++++++--
 drivers/platform/x86/intel/int3472/common.h   |  3 +-
 drivers/platform/x86/intel/int3472/discrete.c | 28 ++-----------------
 3 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index e61119b17677..89894ec873f2 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -86,18 +86,29 @@ static const struct clk_ops skl_int3472_clock_ops = {
 	.recalc_rate = skl_int3472_clk_recalc_rate,
 };
 
-int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
+int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
+			       struct acpi_resource_gpio *agpio)
 {
+	char *path = agpio->resource_source.string_ptr;
 	struct clk_init_data init = {
 		.ops = &skl_int3472_clock_ops,
 		.flags = CLK_GET_RATE_NOCACHE,
 	};
 	int ret;
 
+	int3472->clock.ena_gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
+							     "int3472,clk-enable");
+	if (IS_ERR(int3472->clock.ena_gpio)) {
+		ret = PTR_ERR(int3472->clock.ena_gpio);
+		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
+	}
+
 	init.name = kasprintf(GFP_KERNEL, "%s-clk",
 			      acpi_dev_name(int3472->adev));
-	if (!init.name)
-		return -ENOMEM;
+	if (!init.name) {
+		ret = -ENOMEM;
+		goto out_put_gpio;
+	}
 
 	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
 
@@ -123,14 +134,20 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
 	clk_unregister(int3472->clock.clk);
 out_free_init_name:
 	kfree(init.name);
+out_put_gpio:
+	gpiod_put(int3472->clock.ena_gpio);
 
 	return ret;
 }
 
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
 {
+	if (!int3472->clock.cl)
+		return;
+
 	clkdev_drop(int3472->clock.cl);
 	clk_unregister(int3472->clock.clk);
+	gpiod_put(int3472->clock.ena_gpio);
 }
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index c31321a586d4..434591313762 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -111,7 +111,8 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 					 struct acpi_device **sensor_adev_ret,
 					 const char **name_ret);
 
-int skl_int3472_register_clock(struct int3472_discrete_device *int3472);
+int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
+			       struct acpi_resource_gpio *agpio);
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 7887c6a4035e..6f5fef11cfb7 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -2,8 +2,6 @@
 /* Author: Dan Scally <djrscally@gmail.com> */
 
 #include <linux/acpi.h>
-#include <linux/clkdev.h>
-#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/machine.h>
@@ -154,22 +152,6 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347
 	return 0;
 }
 
-static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
-				       struct acpi_resource_gpio *agpio)
-{
-	char *path = agpio->resource_source.string_ptr;
-	u16 pin = agpio->pin_table[0];
-	struct gpio_desc *gpio;
-
-	gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable");
-	if (IS_ERR(gpio))
-		return (PTR_ERR(gpio));
-
-	int3472->clock.ena_gpio = gpio;
-
-	return skl_int3472_register_clock(int3472);
-}
-
 static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity)
 {
 	switch (type) {
@@ -275,9 +257,9 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		ret = skl_int3472_map_gpio_to_clk(int3472, agpio);
+		ret = skl_int3472_register_clock(int3472, agpio);
 		if (ret)
-			err_msg = "Failed to map GPIO to clock\n";
+			err_msg = "Failed to register clock\n";
 
 		break;
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
@@ -334,11 +316,7 @@ static int skl_int3472_discrete_remove(struct platform_device *pdev)
 
 	gpiod_remove_lookup_table(&int3472->gpios);
 
-	if (int3472->clock.cl)
-		skl_int3472_unregister_clock(int3472);
-
-	gpiod_put(int3472->clock.ena_gpio);
-
+	skl_int3472_unregister_clock(int3472);
 	skl_int3472_unregister_regulator(int3472);
 
 	return 0;
-- 
2.38.1

[PATCH 5/6] platform/x86: int3472/discrete: Ensure the clk/power enable pins are in output mode

[Hans de Goede]

acpi_get_and_request_gpiod() does not take a gpio_lookup_flags argument
specifying that the pins direction should be initialized to a specific
value.

This means that in some cases the pins might be left in input mode, causing
the gpiod_set() calls made to enable the clk / regulator to not work.

One example of this problem is the clk-enable GPIO for the ov01a1s sensor
on a Dell Latitude 9420 being left in input mode causing the clk to
never get enabled.

Explicitly set the direction of the pins to output to fix this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/clk_and_regulator.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 89894ec873f2..dc4ab7821b56 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -103,6 +103,9 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
 		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
 	}
 
+	/* Ensure the pin is in output mode */
+	gpiod_direction_output(int3472->clock.ena_gpio, 0);
+
 	init.name = kasprintf(GFP_KERNEL, "%s-clk",
 			      acpi_dev_name(int3472->adev));
 	if (!init.name) {
@@ -195,6 +198,9 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 		return PTR_ERR(int3472->regulator.gpio);
 	}
 
+	/* Ensure the pin is in output mode */
+	gpiod_direction_output(int3472->regulator.gpio, 0);
+
 	cfg.dev = &int3472->adev->dev;
 	cfg.init_data = &init_data;
 	cfg.ena_gpiod = int3472->regulator.gpio;
-- 
2.38.1

Re: [PATCH 5/6] platform/x86: int3472/discrete: Ensure the clk/power enable pins are in output mode

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> acpi_get_and_request_gpiod() does not take a gpio_lookup_flags argument
> specifying that the pins direction should be initialized to a specific
> value.
>
> This means that in some cases the pins might be left in input mode, causing
> the gpiod_set() calls made to enable the clk / regulator to not work.
>
> One example of this problem is the clk-enable GPIO for the ov01a1s sensor
> on a Dell Latitude 9420 being left in input mode causing the clk to
> never get enabled.
>
> Explicitly set the direction of the pins to output to fix this.

...

> +       /* Ensure the pin is in output mode */

...in output mode and non-active state */

> +       gpiod_direction_output(int3472->clock.ena_gpio, 0);

...

> +       /* Ensure the pin is in output mode */
> +       gpiod_direction_output(int3472->regulator.gpio, 0);

So, previously it was AS IS and now it's non-active state. I believe
this makes no regressions for the other laptops / platforms.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] platform/x86: int3472/discrete: Ensure the clk/power enable pins are in output mode

[Hans de Goede]

Hi,

On 11/30/22 10:59, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> acpi_get_and_request_gpiod() does not take a gpio_lookup_flags argument
>> specifying that the pins direction should be initialized to a specific
>> value.
>>
>> This means that in some cases the pins might be left in input mode, causing
>> the gpiod_set() calls made to enable the clk / regulator to not work.
>>
>> One example of this problem is the clk-enable GPIO for the ov01a1s sensor
>> on a Dell Latitude 9420 being left in input mode causing the clk to
>> never get enabled.
>>
>> Explicitly set the direction of the pins to output to fix this.
> 
> ...
> 
>> +       /* Ensure the pin is in output mode */
> 
> ...in output mode and non-active state */

Ack.

>> +       gpiod_direction_output(int3472->clock.ena_gpio, 0);
> 
> ...
> 
>> +       /* Ensure the pin is in output mode */
>> +       gpiod_direction_output(int3472->regulator.gpio, 0);
> 
> So, previously it was AS IS and now it's non-active state. I believe
> this makes no regressions for the other laptops / platforms.

Correct, and no regression intended indeed. I'll add the improved
comment when merging this (unless there are other / bigger
reasons for a v2).

Regards,

Hans

[PATCH 6/6] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Hans de Goede]

According to:
https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch

Bits 31-24 of the _DSM pin entry integer value codes the active-value,
that is the actual physical signal (0 or 1) which needs to be output on
the pin to turn the sensor chip on (to make it active).

So if bits 31-24 are 0 for a reset pin, then the actual value of the reset
pin needs to be 0 to take the chip out of reset. IOW in this case the reset
signal is active-high rather then the default active-low.

And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk
pin needs to be 0 to enable the clk. So in this case the clk-en signal
is active-low rather then the default active-high.

IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin
is inverted.

Add a check for this and also propagate this new polarity to the clock
registration.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
This patch is deliberately the last patch in the series, because this patch
could do with some more testing. By making it last it can be applied later
then the rest of the series.
---
 .../platform/x86/intel/int3472/clk_and_regulator.c  |  5 ++++-
 drivers/platform/x86/intel/int3472/common.h         |  2 +-
 drivers/platform/x86/intel/int3472/discrete.c       | 13 +++++++++++--
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index dc4ab7821b56..d56c56287414 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -87,7 +87,7 @@ static const struct clk_ops skl_int3472_clock_ops = {
 };
 
 int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
-			       struct acpi_resource_gpio *agpio)
+			       struct acpi_resource_gpio *agpio, u32 polarity)
 {
 	char *path = agpio->resource_source.string_ptr;
 	struct clk_init_data init = {
@@ -103,6 +103,9 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
 		return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n");
 	}
 
+	if (polarity == GPIO_ACTIVE_LOW)
+		gpiod_toggle_active_low(int3472->clock.ena_gpio);
+
 	/* Ensure the pin is in output mode */
 	gpiod_direction_output(int3472->clock.ena_gpio, 0);
 
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 434591313762..fb02fbcc18b7 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -112,7 +112,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 					 const char **name_ret);
 
 int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
-			       struct acpi_resource_gpio *agpio);
+			       struct acpi_resource_gpio *agpio, u32 polarity);
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 6f5fef11cfb7..321551ef4519 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -219,11 +219,11 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	struct int3472_discrete_device *int3472 = data;
 	struct acpi_resource_gpio *agpio;
 	union acpi_object *obj;
+	u8 active_value, type;
 	const char *err_msg;
 	const char *func;
 	u32 polarity;
 	int ret;
-	u8 type;
 
 	if (!acpi_gpio_get_io_resource(ares, &agpio))
 		return 1;
@@ -247,6 +247,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	int3472_get_func_and_polarity(type, &func, &polarity);
 
+	/* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */
+	active_value = obj->integer.value >> 24;
+	if (!active_value)
+		polarity ^= GPIO_ACTIVE_LOW;
+
+	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func,
+		agpio->resource_source.string_ptr, agpio->pin_table[0],
+		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
+
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
@@ -257,7 +266,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 		break;
 	case INT3472_GPIO_TYPE_CLK_ENABLE:
-		ret = skl_int3472_register_clock(int3472, agpio);
+		ret = skl_int3472_register_clock(int3472, agpio, polarity);
 		if (ret)
 			err_msg = "Failed to register clock\n";
 
-- 
2.38.1

Re: [PATCH 6/6] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> According to:
> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>
> Bits 31-24 of the _DSM pin entry integer value codes the active-value,
> that is the actual physical signal (0 or 1) which needs to be output on
> the pin to turn the sensor chip on (to make it active).
>
> So if bits 31-24 are 0 for a reset pin, then the actual value of the reset
> pin needs to be 0 to take the chip out of reset. IOW in this case the reset
> signal is active-high rather then the default active-low.
>
> And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk
> pin needs to be 0 to enable the clk. So in this case the clk-en signal
> is active-low rather then the default active-high.
>
> IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin
> is inverted.
>
> Add a check for this and also propagate this new polarity to the clock
> registration.

I like it in this form, thanks!

...

> +               (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");

Perhaps

static inline str_high_low(bool v)
{
  return v ? "high" : "low";
}

In the string_helpers.h? If you are okay with the idea, you may use my
tag ahead for that patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Hans de Goede]

Hi,

On 11/30/22 11:01, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> According to:
>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>>
>> Bits 31-24 of the _DSM pin entry integer value codes the active-value,
>> that is the actual physical signal (0 or 1) which needs to be output on
>> the pin to turn the sensor chip on (to make it active).
>>
>> So if bits 31-24 are 0 for a reset pin, then the actual value of the reset
>> pin needs to be 0 to take the chip out of reset. IOW in this case the reset
>> signal is active-high rather then the default active-low.
>>
>> And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk
>> pin needs to be 0 to enable the clk. So in this case the clk-en signal
>> is active-low rather then the default active-high.
>>
>> IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin
>> is inverted.
>>
>> Add a check for this and also propagate this new polarity to the clock
>> registration.
> 
> I like it in this form, thanks!
> 
> ...
> 
>> +               (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
> 
> Perhaps
> 
> static inline str_high_low(bool v)
> {
>   return v ? "high" : "low";
> }
> 
> In the string_helpers.h? If you are okay with the idea, you may use my
> tag ahead for that patch.

That is an interesting idea. but IMHO best done in a follow-up series
after checking for similar code in the rest of the kernel.

This is based on the assumption that "selling" this new helper to
the string_helpers.h maintainer will be easier if there are multiple
consumers of it right away.

Regards,

Hans

Re: [PATCH 6/6] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 11:39:42AM +0100, Hans de Goede wrote:
> On 11/30/22 11:01, Andy Shevchenko wrote:
> > On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> According to:
> >> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
> >>
> >> Bits 31-24 of the _DSM pin entry integer value codes the active-value,
> >> that is the actual physical signal (0 or 1) which needs to be output on
> >> the pin to turn the sensor chip on (to make it active).
> >>
> >> So if bits 31-24 are 0 for a reset pin, then the actual value of the reset
> >> pin needs to be 0 to take the chip out of reset. IOW in this case the reset
> >> signal is active-high rather then the default active-low.
> >>
> >> And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk
> >> pin needs to be 0 to enable the clk. So in this case the clk-en signal
> >> is active-low rather then the default active-high.
> >>
> >> IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin
> >> is inverted.
> >>
> >> Add a check for this and also propagate this new polarity to the clock
> >> registration.
> > 
> > I like it in this form, thanks!
> > 
> > ...
> > 
> >> +               (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
> > 
> > Perhaps
> > 
> > static inline str_high_low(bool v)
> > {
> >   return v ? "high" : "low";
> > }
> > 
> > In the string_helpers.h? If you are okay with the idea, you may use my
> > tag ahead for that patch.
> 
> That is an interesting idea. but IMHO best done in a follow-up series
> after checking for similar code in the rest of the kernel.
> 
> This is based on the assumption that "selling" this new helper to
> the string_helpers.h maintainer will be easier if there are multiple
> consumers of it right away.

strings_helper.h maintainer is any known kernel subsystem maintainer + me
(as a reviewer / main contributor to that library). That's why I proposed
the solution and my tag would be enough to route it via your tree.

So, offer still stays.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 01:06:38PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 11:39:42AM +0100, Hans de Goede wrote:
> > On 11/30/22 11:01, Andy Shevchenko wrote:
> > > On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > >> +               (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
> > > 
> > > Perhaps
> > > 
> > > static inline str_high_low(bool v)
> > > {
> > >   return v ? "high" : "low";
> > > }
> > > 
> > > In the string_helpers.h? If you are okay with the idea, you may use my
> > > tag ahead for that patch.
> > 
> > That is an interesting idea. but IMHO best done in a follow-up series
> > after checking for similar code in the rest of the kernel.
> > 
> > This is based on the assumption that "selling" this new helper to
> > the string_helpers.h maintainer will be easier if there are multiple
> > consumers of it right away.
> 
> strings_helper.h maintainer is any known kernel subsystem maintainer + me
> (as a reviewer / main contributor to that library). That's why I proposed
> the solution and my tag would be enough to route it via your tree.
> 
> So, offer still stays.

(You may check last few commits against the header and see the tags)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

[Hans de Goede]

Hi,

On 11/30/22 12:06, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 11:39:42AM +0100, Hans de Goede wrote:
>> On 11/30/22 11:01, Andy Shevchenko wrote:
>>> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> According to:
>>>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>>>>
>>>> Bits 31-24 of the _DSM pin entry integer value codes the active-value,
>>>> that is the actual physical signal (0 or 1) which needs to be output on
>>>> the pin to turn the sensor chip on (to make it active).
>>>>
>>>> So if bits 31-24 are 0 for a reset pin, then the actual value of the reset
>>>> pin needs to be 0 to take the chip out of reset. IOW in this case the reset
>>>> signal is active-high rather then the default active-low.
>>>>
>>>> And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk
>>>> pin needs to be 0 to enable the clk. So in this case the clk-en signal
>>>> is active-low rather then the default active-high.
>>>>
>>>> IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin
>>>> is inverted.
>>>>
>>>> Add a check for this and also propagate this new polarity to the clock
>>>> registration.
>>>
>>> I like it in this form, thanks!
>>>
>>> ...
>>>
>>>> +               (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
>>>
>>> Perhaps
>>>
>>> static inline str_high_low(bool v)
>>> {
>>>   return v ? "high" : "low";
>>> }
>>>
>>> In the string_helpers.h? If you are okay with the idea, you may use my
>>> tag ahead for that patch.
>>
>> That is an interesting idea. but IMHO best done in a follow-up series
>> after checking for similar code in the rest of the kernel.
>>
>> This is based on the assumption that "selling" this new helper to
>> the string_helpers.h maintainer will be easier if there are multiple
>> consumers of it right away.
> 
> strings_helper.h maintainer is any known kernel subsystem maintainer + me
> (as a reviewer / main contributor to that library). That's why I proposed
> the solution and my tag would be enough to route it via your tree.
> 
> So, offer still stays.

Ok I'll add your patch adding the str_high_low() helper to the next
version of this series (or when I merge it, depending on how things go)
and then move this over to use the str_high_low() helper.

Regards,

Hans

Re: [PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> The out of tree IPU6 driver has moved to using the in kernel INT3472
> code for doing power-ctrl rather then doing their own thing (good!).

than

> Some of the IPU6 devices with a discrete INT3472 ACPI device have a
> privacy-led GPIO. but no clk-enable GPIO. To make this work this series
> moves the privacy LED control from being integrated with the clk-provider
> to modelling the privacy LED as a separate GPIO. This also brings the
> discrete INT3472 ACPI device privacy LED handling inline with the privacy
> LED handling for INT3472 TPS68470 PMIC devices which I posted here:
>
> https://lore.kernel.org/platform-driver-x86/20221128214408.165726-1-hdegoede@redhat.com/
>
> This obsoletes my previous "[PATCH 0/3] platform/x86: int3472/discrete:
> Make it work with IPU6" series:
>
> https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/
>
> Mauro since laptops with IPU6 cameras are becoming more and more
> popular I would like to get this merged for 6.2 so that with 6.2
> users will be able to build the out of tree IPU6 driver without
> requiring patching their main kernel. I realize we are a bit
> late in the cycle, but can you please still take the ov5693 patch
> for 6.2 ? It is quite small / straight-forward and since it used
> gpiod_get_optional() it is a no-op without the rest of this series.
>
> This series has been tested on:
>
> - Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED
> - Dell Latitude 9420, IPU 6 with privacy LED on front
> - Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED,

Microsoft?

>                               back: ov8865 with privacy LED

I like this series! Minimum invasion and code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

[Hans de Goede]

Hi,

On 11/30/22 11:03, Andy Shevchenko wrote:
> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> The out of tree IPU6 driver has moved to using the in kernel INT3472
>> code for doing power-ctrl rather then doing their own thing (good!).
> 
> than
> 
>> Some of the IPU6 devices with a discrete INT3472 ACPI device have a
>> privacy-led GPIO. but no clk-enable GPIO. To make this work this series
>> moves the privacy LED control from being integrated with the clk-provider
>> to modelling the privacy LED as a separate GPIO. This also brings the
>> discrete INT3472 ACPI device privacy LED handling inline with the privacy
>> LED handling for INT3472 TPS68470 PMIC devices which I posted here:
>>
>> https://lore.kernel.org/platform-driver-x86/20221128214408.165726-1-hdegoede@redhat.com/
>>
>> This obsoletes my previous "[PATCH 0/3] platform/x86: int3472/discrete:
>> Make it work with IPU6" series:
>>
>> https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/
>>
>> Mauro since laptops with IPU6 cameras are becoming more and more
>> popular I would like to get this merged for 6.2 so that with 6.2
>> users will be able to build the out of tree IPU6 driver without
>> requiring patching their main kernel. I realize we are a bit
>> late in the cycle, but can you please still take the ov5693 patch
>> for 6.2 ? It is quite small / straight-forward and since it used
>> gpiod_get_optional() it is a no-op without the rest of this series.
>>
>> This series has been tested on:
>>
>> - Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED
>> - Dell Latitude 9420, IPU 6 with privacy LED on front
>> - Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED,
> 
> Microsoft?

Ack.

>>                               back: ov8865 with privacy LED
> 
> I like this series! Minimum invasion and code.

I'm glad you like it and thank you for the review.

Regards,

Hans

Re: [PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

[Andy Shevchenko]

On Wed, Nov 30, 2022 at 12:11:43AM +0100, Hans de Goede wrote:
> Hi All,
> 
> The out of tree IPU6 driver has moved to using the in kernel INT3472
> code for doing power-ctrl rather then doing their own thing (good!).
> 
> Some of the IPU6 devices with a discrete INT3472 ACPI device have a
> privacy-led GPIO. but no clk-enable GPIO. To make this work this series
> moves the privacy LED control from being integrated with the clk-provider
> to modelling the privacy LED as a separate GPIO. This also brings the
> discrete INT3472 ACPI device privacy LED handling inline with the privacy
> LED handling for INT3472 TPS68470 PMIC devices which I posted here:
> 
> https://lore.kernel.org/platform-driver-x86/20221128214408.165726-1-hdegoede@redhat.com/
> 
> This obsoletes my previous "[PATCH 0/3] platform/x86: int3472/discrete:
> Make it work with IPU6" series:
> 
> https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/
> 
> Mauro since laptops with IPU6 cameras are becoming more and more
> popular I would like to get this merged for 6.2 so that with 6.2
> users will be able to build the out of tree IPU6 driver without
> requiring patching their main kernel. I realize we are a bit
> late in the cycle, but can you please still take the ov5693 patch
> for 6.2 ? It is quite small / straight-forward and since it used
> gpiod_get_optional() it is a no-op without the rest of this series.
> 
> This series has been tested on:
> 
> - Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED
> - Dell Latitude 9420, IPU 6 with privacy LED on front
> - Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED,
>                               back: ov8865 with privacy LED

FWIW,
Reviewed-by: Andy Shevchenko <andy@kernel.org>
assuming nit-picks will be addressed as agreed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

[Sakari Ailus]

Hi Hans,

On Wed, Nov 30, 2022 at 12:11:43AM +0100, Hans de Goede wrote:
> Hi All,
> 
> The out of tree IPU6 driver has moved to using the in kernel INT3472
> code for doing power-ctrl rather then doing their own thing (good!).

For the set, with comments addressed:

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

-- 
Sakari Ailus

Re: [PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

[Hans de Goede]

Hi All,

On 11/30/22 00:11, Hans de Goede wrote:
> Hi All,
> 
> The out of tree IPU6 driver has moved to using the in kernel INT3472
> code for doing power-ctrl rather then doing their own thing (good!).
> 
> Some of the IPU6 devices with a discrete INT3472 ACPI device have a
> privacy-led GPIO. but no clk-enable GPIO. To make this work this series
> moves the privacy LED control from being integrated with the clk-provider
> to modelling the privacy LED as a separate GPIO. This also brings the
> discrete INT3472 ACPI device privacy LED handling inline with the privacy
> LED handling for INT3472 TPS68470 PMIC devices which I posted here:
> 
> https://lore.kernel.org/platform-driver-x86/20221128214408.165726-1-hdegoede@redhat.com/
> 
> This obsoletes my previous "[PATCH 0/3] platform/x86: int3472/discrete:
> Make it work with IPU6" series:
> 
> https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/
> 
> Mauro since laptops with IPU6 cameras are becoming more and more
> popular I would like to get this merged for 6.2 so that with 6.2
> users will be able to build the out of tree IPU6 driver without
> requiring patching their main kernel. I realize we are a bit
> late in the cycle, but can you please still take the ov5693 patch
> for 6.2 ? It is quite small / straight-forward and since it used
> gpiod_get_optional() it is a no-op without the rest of this series.
> 
> This series has been tested on:
> 
> - Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED
> - Dell Latitude 9420, IPU 6 with privacy LED on front
> - Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED,
>                               back: ov8865 with privacy LED

There has once again been push-back against the concept using
plain GPIOs for the privacy LED controls rather then wrapping
this in a LED class device. This time in the related series
adding support for the privacy LED on the back of Surface Go
devices:

https://lore.kernel.org/platform-driver-x86/20221128214408.165726-1-hdegoede@redhat.com/

Given all the comments / requests to use the LED class for this
I'm going to attempt to do that, see the above thread for some
challenges which I already encountered while exploring LED class
usage for this + proposed solution for those (adding a lookup
table mechanism to the LED class code similar to the existing
GPIO lookup table support).

This will result in a partial rewrite of this series, so self
NACK for this version of the series.

Andy this also means that I will not be using your new str_high_low()
helper function. The code which could use this will likely stay
around, but given that I need to do a rewrite and then get ne
reviews, it would IMHO be better to just get your series starting with:

[PATCH v1 1/3] lib/string_helpers: Add missing header files to MAINTAINERS database

upstream independently and then later my code can be moved over
to the helper (or if the helper lands first maybe use it from
day one), either way it seems best to decouple the merging
of these 2 series from each other.

Regards,

Hans












> Hans de Goede (6):
>   media: ov5693: Add support for a privacy-led GPIO
>   platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
>   platform/x86: int3472/discrete: Treat privacy LED as regular GPIO
>   platform/x86: int3472/discrete: Move GPIO request to
>     skl_int3472_register_clock()
>   platform/x86: int3472/discrete: Ensure the clk/power enable pins are
>     in output mode
>   platform/x86: int3472/discrete: Get the polarity from the _DSM entry
> 
>  drivers/media/i2c/ov5693.c                    | 10 ++
>  .../x86/intel/int3472/clk_and_regulator.c     | 35 +++++--
>  drivers/platform/x86/intel/int3472/common.h   |  4 +-
>  drivers/platform/x86/intel/int3472/discrete.c | 95 ++++++++-----------
>  4 files changed, 80 insertions(+), 64 deletions(-)
> 

Re: [PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

[Andy Shevchenko]

On Wed, Dec 7, 2022 at 7:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/30/22 00:11, Hans de Goede wrote:

...

> Andy this also means that I will not be using your new str_high_low()
> helper function. The code which could use this will likely stay
> around, but given that I need to do a rewrite and then get ne
> reviews, it would IMHO be better to just get your series starting with:
>
> [PATCH v1 1/3] lib/string_helpers: Add missing header files to MAINTAINERS database
>
> upstream independently and then later my code can be moved over
> to the helper (or if the helper lands first maybe use it from
> day one), either way it seems best to decouple the merging
> of these 2 series from each other.

Sure, no problem and thank you for the information!

-- 
With Best Regards,
Andy Shevchenko