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

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

[Hans de Goede]

Hi All,

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

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

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

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

This rewrite makes 2 significant changes:

1. Don't break things on IPU3 platforms

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

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

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

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

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

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

Regards,

Hans


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

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

-- 
2.38.1

[PATCH 1/3] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede]

Make the GPIO to sensor mapping more generic 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>
---
 drivers/platform/x86/intel/int3472/discrete.c | 31 ++++++++++++++-----
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 974a132db651..bc6c62f3f3bf 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -184,6 +184,24 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
 	return 0;
 }
 
+static const char *int3472_dsm_type_to_func(u8 type)
+{
+	switch (type) {
+	case INT3472_GPIO_TYPE_RESET:
+		return "reset";
+	case INT3472_GPIO_TYPE_POWERDOWN:
+		return "powerdown";
+	case INT3472_GPIO_TYPE_CLK_ENABLE:
+		return "clken";
+	case INT3472_GPIO_TYPE_PRIVACY_LED:
+		return "pled";
+	case INT3472_GPIO_TYPE_POWER_ENABLE:
+		return "power-enable";
+	}
+
+	return "unknown";
+}
+
 /**
  * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
  * @ares: A pointer to a &struct acpi_resource
@@ -223,6 +241,7 @@ 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;
 	int ret;
 	u8 type;
 
@@ -246,19 +265,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	type = obj->integer.value & 0xff;
 
+	func = int3472_dsm_type_to_func(type);
+
 	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",
+		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func,
 						     GPIO_ACTIVE_LOW);
 		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 1/3] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Andy Shevchenko]

On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Make the GPIO to sensor mapping more generic 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.

...

> +static const char *int3472_dsm_type_to_func(u8 type)
> +{
> +       switch (type) {
> +       case INT3472_GPIO_TYPE_RESET:
> +               return "reset";
> +       case INT3472_GPIO_TYPE_POWERDOWN:
> +               return "powerdown";
> +       case INT3472_GPIO_TYPE_CLK_ENABLE:
> +               return "clken";
> +       case INT3472_GPIO_TYPE_PRIVACY_LED:
> +               return "pled";
> +       case INT3472_GPIO_TYPE_POWER_ENABLE:
> +               return "power-enable";

default:
  return "unknown";

?

> +       }
> +
> +       return "unknown";
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede]

Hi,

Thank you for the reviews!

On 11/24/22 21:09, Andy Shevchenko wrote:
> On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Make the GPIO to sensor mapping more generic 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.
> 
> ...
> 
>> +static const char *int3472_dsm_type_to_func(u8 type)
>> +{
>> +       switch (type) {
>> +       case INT3472_GPIO_TYPE_RESET:
>> +               return "reset";
>> +       case INT3472_GPIO_TYPE_POWERDOWN:
>> +               return "powerdown";
>> +       case INT3472_GPIO_TYPE_CLK_ENABLE:
>> +               return "clken";
>> +       case INT3472_GPIO_TYPE_PRIVACY_LED:
>> +               return "pled";
>> +       case INT3472_GPIO_TYPE_POWER_ENABLE:
>> +               return "power-enable";
> 
> default:
>   return "unknown";
> 
> ?
> 
>> +       }
>> +
>> +       return "unknown";
>> +}
> 

In the passed some compiler versions complained about the non-void
function not ending with a return statement.

I guess I can give your variant a try (I agree it is more readable)
and of we get compiler warnings we can switch back.

I'll fix this up in the next version or when merging this,
depending on further feedback on the series.

Regards,

Hans

Re: [PATCH 1/3] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Andy Shevchenko]

On Thu, Nov 24, 2022 at 10:20 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/24/22 21:09, Andy Shevchenko wrote:
> > On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> +static const char *int3472_dsm_type_to_func(u8 type)
> >> +{
> >> +       switch (type) {
> >> +       case INT3472_GPIO_TYPE_RESET:
> >> +               return "reset";
> >> +       case INT3472_GPIO_TYPE_POWERDOWN:
> >> +               return "powerdown";
> >> +       case INT3472_GPIO_TYPE_CLK_ENABLE:
> >> +               return "clken";
> >> +       case INT3472_GPIO_TYPE_PRIVACY_LED:
> >> +               return "pled";
> >> +       case INT3472_GPIO_TYPE_POWER_ENABLE:
> >> +               return "power-enable";
> >
> > default:
> >   return "unknown";
> >
> > ?
> >
> >> +       }
> >> +
> >> +       return "unknown";
> >> +}
> >
>
> In the passed some compiler versions complained about the non-void
> function not ending with a return statement.
>
> I guess I can give your variant a try (I agree it is more readable)
> and of we get compiler warnings we can switch back.
>
> I'll fix this up in the next version or when merging this,
> depending on further feedback on the series.

I believe it's not the case for a long time. (Esp. when the kernel
requires GCC 5.1 as bare minimum). We have a lot of (relatively) new
code that uses switch-cases like I suggested and I have heard none of
the complains from anybody, including all CIs crawling through the
kernel code.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Dan Scally]

Hi Hans

On 24/11/2022 20:00, Hans de Goede wrote:
> Make the GPIO to sensor mapping more generic 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>
> ---


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Tested-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/platform/x86/intel/int3472/discrete.c | 31 ++++++++++++++-----
>   1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 974a132db651..bc6c62f3f3bf 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -184,6 +184,24 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
>   	return 0;
>   }
>   
> +static const char *int3472_dsm_type_to_func(u8 type)
> +{
> +	switch (type) {
> +	case INT3472_GPIO_TYPE_RESET:
> +		return "reset";
> +	case INT3472_GPIO_TYPE_POWERDOWN:
> +		return "powerdown";
> +	case INT3472_GPIO_TYPE_CLK_ENABLE:
> +		return "clken";
> +	case INT3472_GPIO_TYPE_PRIVACY_LED:
> +		return "pled";
> +	case INT3472_GPIO_TYPE_POWER_ENABLE:
> +		return "power-enable";
> +	}
> +
> +	return "unknown";
> +}
> +
>   /**
>    * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
>    * @ares: A pointer to a &struct acpi_resource
> @@ -223,6 +241,7 @@ 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;
>   	int ret;
>   	u8 type;
>   
> @@ -246,19 +265,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>   
>   	type = obj->integer.value & 0xff;
>   
> +	func = int3472_dsm_type_to_func(type);
> +
>   	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",
> +		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func,
>   						     GPIO_ACTIVE_LOW);
>   		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:

Re: [PATCH 1/3] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping

[Hans de Goede]

Hi,

On 11/25/22 17:00, Dan Scally wrote:
> Hi Hans
> 
> On 24/11/2022 20:00, Hans de Goede wrote:
>> Make the GPIO to sensor mapping more generic 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>
>> ---
> 
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>

Thank you.

Note I have made some (not insignificant) changes to this patch for the v2
series which I'm working on, so I have decided to not add these tags
because of the changes.

Regards,

Hans



> 
>>   drivers/platform/x86/intel/int3472/discrete.c | 31 ++++++++++++++-----
>>   1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>> index 974a132db651..bc6c62f3f3bf 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -184,6 +184,24 @@ static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
>>       return 0;
>>   }
>>   +static const char *int3472_dsm_type_to_func(u8 type)
>> +{
>> +    switch (type) {
>> +    case INT3472_GPIO_TYPE_RESET:
>> +        return "reset";
>> +    case INT3472_GPIO_TYPE_POWERDOWN:
>> +        return "powerdown";
>> +    case INT3472_GPIO_TYPE_CLK_ENABLE:
>> +        return "clken";
>> +    case INT3472_GPIO_TYPE_PRIVACY_LED:
>> +        return "pled";
>> +    case INT3472_GPIO_TYPE_POWER_ENABLE:
>> +        return "power-enable";
>> +    }
>> +
>> +    return "unknown";
>> +}
>> +
>>   /**
>>    * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
>>    * @ares: A pointer to a &struct acpi_resource
>> @@ -223,6 +241,7 @@ 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;
>>       int ret;
>>       u8 type;
>>   @@ -246,19 +265,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>>         type = obj->integer.value & 0xff;
>>   +    func = int3472_dsm_type_to_func(type);
>> +
>>       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",
>> +        ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func,
>>                                GPIO_ACTIVE_LOW);
>>           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:
> 

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

[Hans de Goede]

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

On IPU6 the polarity is encoded in the _DSM entry rather then being
hardcoded to GPIO_ACTIVE_LOW.

Using the _DSM entry for this on IPU3 leads to regressions, so only
use the _DSM entry for this on non IPU3 devices.

Note there is a whole bunch of PCI-ids for the IPU6 which is why
the check is for the IPU3-CIO2, because the CIO2 there has a unique
PCI-id which can be used for this.

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

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index bc6c62f3f3bf..9159291be28a 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/overflow.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/uuid.h>
 
@@ -36,6 +37,19 @@ static const guid_t cio2_sensor_module_guid =
 	GUID_INIT(0x822ace8f, 0x2814, 0x4174,
 		  0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
 
+/* IPU3 vs IPU6 needs to be handled differently */
+#define IPU3_CIO2_PCI_ID				0x9d32
+
+static const struct pci_device_id ipu3_cio2_pci_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, IPU3_CIO2_PCI_ID) },
+	{ }
+};
+
+static int ipu3_present(void)
+{
+	return pci_dev_present(ipu3_cio2_pci_id_table);
+}
+
 /*
  * Here follows platform specific mapping information that we can pass to
  * the functions mapping resources to the sensors. Where the sensors have
@@ -242,6 +256,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	union acpi_object *obj;
 	const char *err_msg;
 	const char *func;
+	u32 polarity;
 	int ret;
 	u8 type;
 
@@ -265,13 +280,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	type = obj->integer.value & 0xff;
 
+	/* IPU3 always uses active-low, IPU6 polarity is encoded in the _DSM entry. */
+	if (ipu3_present())
+		polarity = GPIO_ACTIVE_LOW;
+	else
+		polarity = ((obj->integer.value >> 24) & 0xff) ? GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW;
+
 	func = int3472_dsm_type_to_func(type);
 
+	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:
-		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func,
-						     GPIO_ACTIVE_LOW);
+		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
 		if (ret)
 			err_msg = "Failed to map GPIO pin to sensor\n";
 
-- 
2.38.1

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

[Andy Shevchenko]

On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The out of tree IPU6 driver has moved to also using the in kernel INT3472
> code for doing power-ctrl rather then doing their own thing (good!).
>
> On IPU6 the polarity is encoded in the _DSM entry rather then being
> hardcoded to GPIO_ACTIVE_LOW.
>
> Using the _DSM entry for this on IPU3 leads to regressions, so only
> use the _DSM entry for this on non IPU3 devices.
>
> Note there is a whole bunch of PCI-ids for the IPU6 which is why
> the check is for the IPU3-CIO2, because the CIO2 there has a unique
> PCI-id which can be used for this.

...

> +/* IPU3 vs IPU6 needs to be handled differently */
> +#define IPU3_CIO2_PCI_ID                               0x9d32

If it makes more than a single driver, perhaps pci_ids.h is a good
place to keep it there?

...

> +       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");

Parentheses are not needed, right?

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 11/24/22 21:13, Andy Shevchenko wrote:
> On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The out of tree IPU6 driver has moved to also using the in kernel INT3472
>> code for doing power-ctrl rather then doing their own thing (good!).
>>
>> On IPU6 the polarity is encoded in the _DSM entry rather then being
>> hardcoded to GPIO_ACTIVE_LOW.
>>
>> Using the _DSM entry for this on IPU3 leads to regressions, so only
>> use the _DSM entry for this on non IPU3 devices.
>>
>> Note there is a whole bunch of PCI-ids for the IPU6 which is why
>> the check is for the IPU3-CIO2, because the CIO2 there has a unique
>> PCI-id which can be used for this.
> 
> ...
> 
>> +/* IPU3 vs IPU6 needs to be handled differently */
>> +#define IPU3_CIO2_PCI_ID                               0x9d32
> 
> If it makes more than a single driver, perhaps pci_ids.h is a good
> place to keep it there?

Yes, I've added a note to my TODO to clean this up in a follow-up
patch (the pci-ids.h maintainers want to see multiple users of
an id before accepting new ids in there).

> 
> ...
> 
>> +       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");
> 
> Parentheses are not needed, right?

Right, but I prefer to use them in conditional statements like these,
because I personally find that they make things easier to read.

Regards,

Hans

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

[Dan Scally]

Hello

On 24/11/2022 20:26, Hans de Goede wrote:
> Hi,
>
> On 11/24/22 21:13, Andy Shevchenko wrote:
>> On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>> The out of tree IPU6 driver has moved to also using the in kernel INT3472
>>> code for doing power-ctrl rather then doing their own thing (good!).
>>>
>>> On IPU6 the polarity is encoded in the _DSM entry rather then being
>>> hardcoded to GPIO_ACTIVE_LOW.
>>>
>>> Using the _DSM entry for this on IPU3 leads to regressions, so only
>>> use the _DSM entry for this on non IPU3 devices.
>>>
>>> Note there is a whole bunch of PCI-ids for the IPU6 which is why
>>> the check is for the IPU3-CIO2, because the CIO2 there has a unique
>>> PCI-id which can be used for this.
>> ...
>>
>>> +/* IPU3 vs IPU6 needs to be handled differently */
>>> +#define IPU3_CIO2_PCI_ID                               0x9d32
>> If it makes more than a single driver, perhaps pci_ids.h is a good
>> place to keep it there?
> Yes, I've added a note to my TODO to clean this up in a follow-up
> patch (the pci-ids.h maintainers want to see multiple users of
> an id before accepting new ids in there).


fwiw this in drivers/media/pci/intel/ipu3/ipu3-cio2.h already as 
CIO2_PCI_ID, so it will have multiple users with this change.

>> ...
>>
>>> +       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");
>> Parentheses are not needed, right?
> Right, but I prefer to use them in conditional statements like these,
> because I personally find that they make things easier to read.


Seconded.

> Regards,
>
> Hans
>
>

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

[Dan Scally]

Hi Hans

On 24/11/2022 20:00, Hans de Goede wrote:
> The out of tree IPU6 driver has moved to also using the in kernel INT3472
> code for doing power-ctrl rather then doing their own thing (good!).


Neat!

>
> On IPU6 the polarity is encoded in the _DSM entry rather then being
> hardcoded to GPIO_ACTIVE_LOW.
>
> Using the _DSM entry for this on IPU3 leads to regressions, so only
> use the _DSM entry for this on non IPU3 devices.


Shame; some consistency might have been nice

> Note there is a whole bunch of PCI-ids for the IPU6 which is why
> the check is for the IPU3-CIO2, because the CIO2 there has a unique
> PCI-id which can be used for this.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Tested-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/platform/x86/intel/int3472/discrete.c | 28 +++++++++++++++++--
>   1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index bc6c62f3f3bf..9159291be28a 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -11,6 +11,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/overflow.h>
> +#include <linux/pci.h>
>   #include <linux/platform_device.h>
>   #include <linux/uuid.h>
>   
> @@ -36,6 +37,19 @@ static const guid_t cio2_sensor_module_guid =
>   	GUID_INIT(0x822ace8f, 0x2814, 0x4174,
>   		  0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
>   
> +/* IPU3 vs IPU6 needs to be handled differently */
> +#define IPU3_CIO2_PCI_ID				0x9d32
> +
> +static const struct pci_device_id ipu3_cio2_pci_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, IPU3_CIO2_PCI_ID) },
> +	{ }
> +};
> +
> +static int ipu3_present(void)
> +{
> +	return pci_dev_present(ipu3_cio2_pci_id_table);
> +}
> +
>   /*
>    * Here follows platform specific mapping information that we can pass to
>    * the functions mapping resources to the sensors. Where the sensors have
> @@ -242,6 +256,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>   	union acpi_object *obj;
>   	const char *err_msg;
>   	const char *func;
> +	u32 polarity;
>   	int ret;
>   	u8 type;
>   
> @@ -265,13 +280,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>   
>   	type = obj->integer.value & 0xff;
>   
> +	/* IPU3 always uses active-low, IPU6 polarity is encoded in the _DSM entry. */
> +	if (ipu3_present())
> +		polarity = GPIO_ACTIVE_LOW;
> +	else
> +		polarity = ((obj->integer.value >> 24) & 0xff) ? GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW;
> +
>   	func = int3472_dsm_type_to_func(type);
>   
> +	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:
> -		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func,
> -						     GPIO_ACTIVE_LOW);
> +		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
>   		if (ret)
>   			err_msg = "Failed to map GPIO pin to sensor\n";
>   

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

[Hans de Goede]

Hi All,

On 11/24/22 21:00, Hans de Goede wrote:
> The out of tree IPU6 driver has moved to also using the in kernel INT3472
> code for doing power-ctrl rather then doing their own thing (good!).
> 
> On IPU6 the polarity is encoded in the _DSM entry rather then being
> hardcoded to GPIO_ACTIVE_LOW.
> 
> Using the _DSM entry for this on IPU3 leads to regressions, so only
> use the _DSM entry for this on non IPU3 devices.

So it turns out that the reason why this does not work on IPU3 is
because looking at this as polarity = (bits 31-24) ? high:low is not
correct.

The correct way of looking at this really is:

	polarity = default-polarity-for-pin;
	if ((bits 31-24) == 0)
		polarity = !polarity;

The: "polarity = (bits 31-24) ? high:low" thing did work with IPU6
because the out of tree bundled drivers set reset and poweroff
to 1 on power-on and to 0 on power-off. IOW they apply the
default active-low-ness of these pins at the sensor driver level
rather then letting the GPIO core handle this. Which is actually
the wrong thing to do...

For the new series replacing this one I'm going to go with the:

	if ((bits 31-24) == 0)
		polarity = !polarity;

Approach which works on both IPU3 and IPU6. I'll also make this
the last patch in the series and I'll probably merge it later
then the rest of the series so that it can get some extra testing.

Regards,

Hans


> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 28 +++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index bc6c62f3f3bf..9159291be28a 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/overflow.h>
> +#include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/uuid.h>
>  
> @@ -36,6 +37,19 @@ static const guid_t cio2_sensor_module_guid =
>  	GUID_INIT(0x822ace8f, 0x2814, 0x4174,
>  		  0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
>  
> +/* IPU3 vs IPU6 needs to be handled differently */
> +#define IPU3_CIO2_PCI_ID				0x9d32
> +
> +static const struct pci_device_id ipu3_cio2_pci_id_table[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, IPU3_CIO2_PCI_ID) },
> +	{ }
> +};
> +
> +static int ipu3_present(void)
> +{
> +	return pci_dev_present(ipu3_cio2_pci_id_table);
> +}
> +
>  /*
>   * Here follows platform specific mapping information that we can pass to
>   * the functions mapping resources to the sensors. Where the sensors have
> @@ -242,6 +256,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  	union acpi_object *obj;
>  	const char *err_msg;
>  	const char *func;
> +	u32 polarity;
>  	int ret;
>  	u8 type;
>  
> @@ -265,13 +280,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  
>  	type = obj->integer.value & 0xff;
>  
> +	/* IPU3 always uses active-low, IPU6 polarity is encoded in the _DSM entry. */
> +	if (ipu3_present())
> +		polarity = GPIO_ACTIVE_LOW;
> +	else
> +		polarity = ((obj->integer.value >> 24) & 0xff) ? GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW;
> +
>  	func = int3472_dsm_type_to_func(type);
>  
> +	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:
> -		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func,
> -						     GPIO_ACTIVE_LOW);
> +		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
>  		if (ret)
>  			err_msg = "Failed to map GPIO pin to sensor\n";
>  

[PATCH 3/3] platform/x86: int3472/discrete: Add support for sensor-drivers which expect clken + pled GPIOs

[Hans de Goede]

The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6
driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather
then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver
all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then
it being turned on/off at the same time as the clk.

Adjust how we handle the GPIOs on these sensors accordingly, for now at
least, so that the out of tree driver can work with standard distro kernels
through e.g. dkms. Otherwise users need to run a patched kernel just for
this small difference.

This of course needs to be revisited when we mainline these sensor drivers,
I can imagine the drivers getting clk-framework support when they are
mainlined and then at that same time their acpi HID can be dropped from
the use_gpio_for_clk_acpi_ids[] array.

Note there already is a mainline driver for the ov2740, but that is not
impacted by this change since atm it uses neither the clk framework nor
a "clken" GPIO.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Maybe we should patch the sensor drivers for sensors supported with
the IPU3 to also expect the privacy-led to always be a separate GPIO?

This way we can also avoid the camera LED briefly going on at boot,
when the driver is powering things up to read the sensor's ID register.

And I have also put looking at making the mainline ov2740 driver suitable
for use with the (out of tree) IPU6 driver on my TODO list.
---
 drivers/platform/x86/intel/int3472/common.h   |  2 +-
 drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++----
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 53270d19c73a..58647d3084b9 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -23,7 +23,7 @@
 #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
 
 #define INT3472_PDEV_MAX_NAME_LEN				23
-#define INT3472_MAX_SENSOR_GPIOS				3
+#define INT3472_MAX_SENSOR_GPIOS				4
 
 #define GPIO_REGULATOR_NAME_LENGTH				21
 #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 9159291be28a..bfcf8184db16 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type)
 	return "unknown";
 }
 
+/*
+ * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver,
+ * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and
+ * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled
+ * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk.
+ *
+ * Note there also is a mainline driver for the ov2740, but that does not use
+ * the clk framework atm either.
+ *
+ * Adjust how we handle the GPIOs on these sensors accordingly, for now at least.
+ * This needs to be revisited when we mainline these sensor drivers / when we merge
+ * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6.
+ */
+static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = {
+	{ "HIMX11B1" }, /* hm11b1 */
+	{ "OVTI01AS" }, /* ov01a1s */
+	{ "INT3474" },  /* ov2740 */
+	{}
+};
+
 /**
  * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
  * @ares: A pointer to a &struct acpi_resource
@@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
 
 	switch (type) {
+	case INT3472_GPIO_TYPE_CLK_ENABLE:
+	case INT3472_GPIO_TYPE_PRIVACY_LED:
+		if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) {
+			ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
+			if (ret)
+				err_msg = "Failed to map GPIO to clock\n";
+
+			break;
+		}
+		fallthrough;
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
 		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);
-		if (ret)
-			err_msg = "Failed to map GPIO to clock\n";
-
 		break;
 	case INT3472_GPIO_TYPE_POWER_ENABLE:
 		ret = skl_int3472_register_regulator(int3472, agpio);
-- 
2.38.1

Re: [PATCH 3/3] platform/x86: int3472/discrete: Add support for sensor-drivers which expect clken + pled GPIOs

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thu, Nov 24, 2022 at 09:00:07PM +0100, Hans de Goede wrote:
> The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6
> driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather
> then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver
> all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then
> it being turned on/off at the same time as the clk.

I don't like this idea much. I see this as opening the door to other
hacks in mainline just for the purpose of supporting out-of-tree
drivers. That's not how we should operate upstream.

Why can't we patch the out-of-tree drivers to use the clock framework,
given that's what it will need to do in mainline ? That shouldn't be a
too intrusive change.

> Adjust how we handle the GPIOs on these sensors accordingly, for now at
> least, so that the out of tree driver can work with standard distro kernels
> through e.g. dkms. Otherwise users need to run a patched kernel just for
> this small difference.
> 
> This of course needs to be revisited when we mainline these sensor drivers,
> I can imagine the drivers getting clk-framework support when they are
> mainlined and then at that same time their acpi HID can be dropped from
> the use_gpio_for_clk_acpi_ids[] array.
> 
> Note there already is a mainline driver for the ov2740, but that is not
> impacted by this change since atm it uses neither the clk framework nor
> a "clken" GPIO.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Maybe we should patch the sensor drivers for sensors supported with
> the IPU3 to also expect the privacy-led to always be a separate GPIO?
> 
> This way we can also avoid the camera LED briefly going on at boot,
> when the driver is powering things up to read the sensor's ID register.

To fix the privacy LED flickering problem correctly we need to avoid
powering up sensor at probe time, as there are hardware designs that
wire the privacy LED to the sensor power rails without any way to
disable it in software.

> And I have also put looking at making the mainline ov2740 driver suitable
> for use with the (out of tree) IPU6 driver on my TODO list.
> ---
>  drivers/platform/x86/intel/int3472/common.h   |  2 +-
>  drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++----
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
> index 53270d19c73a..58647d3084b9 100644
> --- a/drivers/platform/x86/intel/int3472/common.h
> +++ b/drivers/platform/x86/intel/int3472/common.h
> @@ -23,7 +23,7 @@
>  #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
>  
>  #define INT3472_PDEV_MAX_NAME_LEN				23
> -#define INT3472_MAX_SENSOR_GPIOS				3
> +#define INT3472_MAX_SENSOR_GPIOS				4
>  
>  #define GPIO_REGULATOR_NAME_LENGTH				21
>  #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 9159291be28a..bfcf8184db16 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type)
>  	return "unknown";
>  }
>  
> +/*
> + * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver,
> + * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and
> + * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled
> + * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk.
> + *
> + * Note there also is a mainline driver for the ov2740, but that does not use
> + * the clk framework atm either.
> + *
> + * Adjust how we handle the GPIOs on these sensors accordingly, for now at least.
> + * This needs to be revisited when we mainline these sensor drivers / when we merge
> + * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6.
> + */
> +static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = {
> +	{ "HIMX11B1" }, /* hm11b1 */
> +	{ "OVTI01AS" }, /* ov01a1s */
> +	{ "INT3474" },  /* ov2740 */
> +	{}
> +};
> +
>  /**
>   * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
>   * @ares: A pointer to a &struct acpi_resource
> @@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
>  
>  	switch (type) {
> +	case INT3472_GPIO_TYPE_CLK_ENABLE:
> +	case INT3472_GPIO_TYPE_PRIVACY_LED:
> +		if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) {
> +			ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
> +			if (ret)
> +				err_msg = "Failed to map GPIO to clock\n";
> +
> +			break;
> +		}
> +		fallthrough;
>  	case INT3472_GPIO_TYPE_RESET:
>  	case INT3472_GPIO_TYPE_POWERDOWN:
>  		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);
> -		if (ret)
> -			err_msg = "Failed to map GPIO to clock\n";
> -
>  		break;
>  	case INT3472_GPIO_TYPE_POWER_ENABLE:
>  		ret = skl_int3472_register_regulator(int3472, agpio);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] platform/x86: int3472/discrete: Add support for sensor-drivers which expect clken + pled GPIOs

[Dan Scally]

Hi Hans

On 24/11/2022 20:00, Hans de Goede wrote:
> The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6
> driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather
> then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver
> all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then
> it being turned on/off at the same time as the clk.
>
> Adjust how we handle the GPIOs on these sensors accordingly, for now at
> least, so that the out of tree driver can work with standard distro kernels
> through e.g. dkms. Otherwise users need to run a patched kernel just for
> this small difference.
>
> This of course needs to be revisited when we mainline these sensor drivers,
> I can imagine the drivers getting clk-framework support when they are
> mainlined and then at that same time their acpi HID can be dropped from
> the use_gpio_for_clk_acpi_ids[] array.
>
> Note there already is a mainline driver for the ov2740, but that is not
> impacted by this change since atm it uses neither the clk framework nor
> a "clken" GPIO.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Maybe we should patch the sensor drivers for sensors supported with
> the IPU3 to also expect the privacy-led to always be a separate GPIO?
>
> This way we can also avoid the camera LED briefly going on at boot,
> when the driver is powering things up to read the sensor's ID register.
>
> And I have also put looking at making the mainline ov2740 driver suitable
> for use with the (out of tree) IPU6 driver on my TODO list.
> ---
>   drivers/platform/x86/intel/int3472/common.h   |  2 +-
>   drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++----
>   2 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
> index 53270d19c73a..58647d3084b9 100644
> --- a/drivers/platform/x86/intel/int3472/common.h
> +++ b/drivers/platform/x86/intel/int3472/common.h
> @@ -23,7 +23,7 @@
>   #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
>   
>   #define INT3472_PDEV_MAX_NAME_LEN				23
> -#define INT3472_MAX_SENSOR_GPIOS				3
> +#define INT3472_MAX_SENSOR_GPIOS				4
>   
>   #define GPIO_REGULATOR_NAME_LENGTH				21
>   #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH			9
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 9159291be28a..bfcf8184db16 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type)
>   	return "unknown";
>   }
>   
> +/*
> + * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver,
> + * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and
> + * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled
> + * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk.
> + *
> + * Note there also is a mainline driver for the ov2740, but that does not use
> + * the clk framework atm either.
> + *
> + * Adjust how we handle the GPIOs on these sensors accordingly, for now at least.
> + * This needs to be revisited when we mainline these sensor drivers / when we merge
> + * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6.
> + */


I'm not really sure about this one though; wouldn't it be better to 
alter those sensor drivers to use the clock framework properly?


Thanks

Dan

> +static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = {
> +	{ "HIMX11B1" }, /* hm11b1 */
> +	{ "OVTI01AS" }, /* ov01a1s */
> +	{ "INT3474" },  /* ov2740 */
> +	{}
> +};
> +
>   /**
>    * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
>    * @ares: A pointer to a &struct acpi_resource
> @@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>   		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
>   
>   	switch (type) {
> +	case INT3472_GPIO_TYPE_CLK_ENABLE:
> +	case INT3472_GPIO_TYPE_PRIVACY_LED:
> +		if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) {
> +			ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
> +			if (ret)
> +				err_msg = "Failed to map GPIO to clock\n";
> +
> +			break;
> +		}
> +		fallthrough;
>   	case INT3472_GPIO_TYPE_RESET:
>   	case INT3472_GPIO_TYPE_POWERDOWN:
>   		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);
> -		if (ret)
> -			err_msg = "Failed to map GPIO to clock\n";
> -
>   		break;
>   	case INT3472_GPIO_TYPE_POWER_ENABLE:
>   		ret = skl_int3472_register_regulator(int3472, agpio);

Re: [PATCH 3/3] platform/x86: int3472/discrete: Add support for sensor-drivers which expect clken + pled GPIOs

[Hans de Goede]

Hi,

On 11/25/22 17:07, Dan Scally wrote:
> Hi Hans
> 
> On 24/11/2022 20:00, Hans de Goede wrote:
>> The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6
>> driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather
>> then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver
>> all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then
>> it being turned on/off at the same time as the clk.
>>
>> Adjust how we handle the GPIOs on these sensors accordingly, for now at
>> least, so that the out of tree driver can work with standard distro kernels
>> through e.g. dkms. Otherwise users need to run a patched kernel just for
>> this small difference.
>>
>> This of course needs to be revisited when we mainline these sensor drivers,
>> I can imagine the drivers getting clk-framework support when they are
>> mainlined and then at that same time their acpi HID can be dropped from
>> the use_gpio_for_clk_acpi_ids[] array.
>>
>> Note there already is a mainline driver for the ov2740, but that is not
>> impacted by this change since atm it uses neither the clk framework nor
>> a "clken" GPIO.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Maybe we should patch the sensor drivers for sensors supported with
>> the IPU3 to also expect the privacy-led to always be a separate GPIO?
>>
>> This way we can also avoid the camera LED briefly going on at boot,
>> when the driver is powering things up to read the sensor's ID register.
>>
>> And I have also put looking at making the mainline ov2740 driver suitable
>> for use with the (out of tree) IPU6 driver on my TODO list.
>> ---
>>   drivers/platform/x86/intel/int3472/common.h   |  2 +-
>>   drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++----
>>   2 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
>> index 53270d19c73a..58647d3084b9 100644
>> --- a/drivers/platform/x86/intel/int3472/common.h
>> +++ b/drivers/platform/x86/intel/int3472/common.h
>> @@ -23,7 +23,7 @@
>>   #define INT3472_GPIO_TYPE_PRIVACY_LED                0x0d
>>     #define INT3472_PDEV_MAX_NAME_LEN                23
>> -#define INT3472_MAX_SENSOR_GPIOS                3
>> +#define INT3472_MAX_SENSOR_GPIOS                4
>>     #define GPIO_REGULATOR_NAME_LENGTH                21
>>   #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH            9
>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>> index 9159291be28a..bfcf8184db16 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type)
>>       return "unknown";
>>   }
>>   +/*
>> + * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver,
>> + * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and
>> + * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled
>> + * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk.
>> + *
>> + * Note there also is a mainline driver for the ov2740, but that does not use
>> + * the clk framework atm either.
>> + *
>> + * Adjust how we handle the GPIOs on these sensors accordingly, for now at least.
>> + * This needs to be revisited when we mainline these sensor drivers / when we merge
>> + * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6.
>> + */
> 
> 
> I'm not really sure about this one though; wouldn't it be better to alter those sensor drivers to use the clock framework properly?

Yes, Laurent more or less said the same thing; and I was already starting
to think in this direction myself when typing the cover letter.

So yes I agree with you and Laurent. That still leaves the question of what to do
with devices with just a privacy LED without a clk-en though.

Dan, do you have a list of sensors which currently are known to work / be used
together with the IPU3 (and the int3472 discrete code) ?

I know I will need to modifi the ov5693 code, but I wonder what other drivers
I will need to modify ?

I think I might just move those sensor-drivers over to using a GPIO
for the privacy LED and just always register a GPIO for the privacy LED
pin, does that sound like a good idea to you ?

Anyways it is weekend now and I've already worked too many hours this week,
so I'll take a look at this on Monday.

Regards,

Hans



>> +static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = {
>> +    { "HIMX11B1" }, /* hm11b1 */
>> +    { "OVTI01AS" }, /* ov01a1s */
>> +    { "INT3474" },  /* ov2740 */
>> +    {}
>> +};
>> +
>>   /**
>>    * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
>>    * @ares: A pointer to a &struct acpi_resource
>> @@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>>           (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
>>         switch (type) {
>> +    case INT3472_GPIO_TYPE_CLK_ENABLE:
>> +    case INT3472_GPIO_TYPE_PRIVACY_LED:
>> +        if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) {
>> +            ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
>> +            if (ret)
>> +                err_msg = "Failed to map GPIO to clock\n";
>> +
>> +            break;
>> +        }
>> +        fallthrough;
>>       case INT3472_GPIO_TYPE_RESET:
>>       case INT3472_GPIO_TYPE_POWERDOWN:
>>           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);
>> -        if (ret)
>> -            err_msg = "Failed to map GPIO to clock\n";
>> -
>>           break;
>>       case INT3472_GPIO_TYPE_POWER_ENABLE:
>>           ret = skl_int3472_register_regulator(int3472, agpio);
> 

Re: [PATCH 3/3] platform/x86: int3472/discrete: Add support for sensor-drivers which expect clken + pled GPIOs

[Dan Scally]

Morning Hans

On 25/11/2022 18:38, Hans de Goede wrote:
> Hi,
>
> On 11/25/22 17:07, Dan Scally wrote:
>> Hi Hans
>>
>> On 24/11/2022 20:00, Hans de Goede wrote:
>>> The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6
>>> driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather
>>> then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver
>>> all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then
>>> it being turned on/off at the same time as the clk.
>>>
>>> Adjust how we handle the GPIOs on these sensors accordingly, for now at
>>> least, so that the out of tree driver can work with standard distro kernels
>>> through e.g. dkms. Otherwise users need to run a patched kernel just for
>>> this small difference.
>>>
>>> This of course needs to be revisited when we mainline these sensor drivers,
>>> I can imagine the drivers getting clk-framework support when they are
>>> mainlined and then at that same time their acpi HID can be dropped from
>>> the use_gpio_for_clk_acpi_ids[] array.
>>>
>>> Note there already is a mainline driver for the ov2740, but that is not
>>> impacted by this change since atm it uses neither the clk framework nor
>>> a "clken" GPIO.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Maybe we should patch the sensor drivers for sensors supported with
>>> the IPU3 to also expect the privacy-led to always be a separate GPIO?
>>>
>>> This way we can also avoid the camera LED briefly going on at boot,
>>> when the driver is powering things up to read the sensor's ID register.
>>>
>>> And I have also put looking at making the mainline ov2740 driver suitable
>>> for use with the (out of tree) IPU6 driver on my TODO list.
>>> ---
>>>    drivers/platform/x86/intel/int3472/common.h   |  2 +-
>>>    drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++----
>>>    2 files changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
>>> index 53270d19c73a..58647d3084b9 100644
>>> --- a/drivers/platform/x86/intel/int3472/common.h
>>> +++ b/drivers/platform/x86/intel/int3472/common.h
>>> @@ -23,7 +23,7 @@
>>>    #define INT3472_GPIO_TYPE_PRIVACY_LED                0x0d
>>>      #define INT3472_PDEV_MAX_NAME_LEN                23
>>> -#define INT3472_MAX_SENSOR_GPIOS                3
>>> +#define INT3472_MAX_SENSOR_GPIOS                4
>>>      #define GPIO_REGULATOR_NAME_LENGTH                21
>>>    #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH            9
>>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>>> index 9159291be28a..bfcf8184db16 100644
>>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>>> @@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type)
>>>        return "unknown";
>>>    }
>>>    +/*
>>> + * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver,
>>> + * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and
>>> + * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled
>>> + * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk.
>>> + *
>>> + * Note there also is a mainline driver for the ov2740, but that does not use
>>> + * the clk framework atm either.
>>> + *
>>> + * Adjust how we handle the GPIOs on these sensors accordingly, for now at least.
>>> + * This needs to be revisited when we mainline these sensor drivers / when we merge
>>> + * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6.
>>> + */
>>
>> I'm not really sure about this one though; wouldn't it be better to alter those sensor drivers to use the clock framework properly?
> Yes, Laurent more or less said the same thing; and I was already starting
> to think in this direction myself when typing the cover letter.
>
> So yes I agree with you and Laurent. That still leaves the question of what to do
> with devices with just a privacy LED without a clk-en though.
>
> Dan, do you have a list of sensors which currently are known to work / be used
> together with the IPU3 (and the int3472 discrete code) ?
>
> I know I will need to modifi the ov5693 code, but I wonder what other drivers
> I will need to modify ?


The ov5693, ov8865 and ov7251 are the upstream working ones. There's a 
couple more that need changes upstreaming, but I can handle those during 
that process.

>
> I think I might just move those sensor-drivers over to using a GPIO
> for the privacy LED and just always register a GPIO for the privacy LED
> pin, does that sound like a good idea to you ?


Well if we can't handle it during the int3472 code then yes - I 
certainly don't have  a better idea.

> Anyways it is weekend now and I've already worked too many hours this week,
> so I'll take a look at this on Monday.
>
> Regards,
>
> Hans
>
>
>
>>> +static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = {
>>> +    { "HIMX11B1" }, /* hm11b1 */
>>> +    { "OVTI01AS" }, /* ov01a1s */
>>> +    { "INT3474" },  /* ov2740 */
>>> +    {}
>>> +};
>>> +
>>>    /**
>>>     * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
>>>     * @ares: A pointer to a &struct acpi_resource
>>> @@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>>>            (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
>>>          switch (type) {
>>> +    case INT3472_GPIO_TYPE_CLK_ENABLE:
>>> +    case INT3472_GPIO_TYPE_PRIVACY_LED:
>>> +        if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) {
>>> +            ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
>>> +            if (ret)
>>> +                err_msg = "Failed to map GPIO to clock\n";
>>> +
>>> +            break;
>>> +        }
>>> +        fallthrough;
>>>        case INT3472_GPIO_TYPE_RESET:
>>>        case INT3472_GPIO_TYPE_POWERDOWN:
>>>            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);
>>> -        if (ret)
>>> -            err_msg = "Failed to map GPIO to clock\n";
>>> -
>>>            break;
>>>        case INT3472_GPIO_TYPE_POWER_ENABLE:
>>>            ret = skl_int3472_register_regulator(int3472, agpio);

Re: [PATCH 3/3] platform/x86: int3472/discrete: Add support for sensor-drivers which expect clken + pled GPIOs

[Hans de Goede]

Hi,

On 11/28/22 08:39, Dan Scally wrote:
> Morning Hans
> 
> On 25/11/2022 18:38, Hans de Goede wrote:
>> Hi,
>>
>> On 11/25/22 17:07, Dan Scally wrote:
>>> Hi Hans
>>>
>>> On 24/11/2022 20:00, Hans de Goede wrote:
>>>> The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6
>>>> driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather
>>>> then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver
>>>> all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then
>>>> it being turned on/off at the same time as the clk.
>>>>
>>>> Adjust how we handle the GPIOs on these sensors accordingly, for now at
>>>> least, so that the out of tree driver can work with standard distro kernels
>>>> through e.g. dkms. Otherwise users need to run a patched kernel just for
>>>> this small difference.
>>>>
>>>> This of course needs to be revisited when we mainline these sensor drivers,
>>>> I can imagine the drivers getting clk-framework support when they are
>>>> mainlined and then at that same time their acpi HID can be dropped from
>>>> the use_gpio_for_clk_acpi_ids[] array.
>>>>
>>>> Note there already is a mainline driver for the ov2740, but that is not
>>>> impacted by this change since atm it uses neither the clk framework nor
>>>> a "clken" GPIO.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Maybe we should patch the sensor drivers for sensors supported with
>>>> the IPU3 to also expect the privacy-led to always be a separate GPIO?
>>>>
>>>> This way we can also avoid the camera LED briefly going on at boot,
>>>> when the driver is powering things up to read the sensor's ID register.
>>>>
>>>> And I have also put looking at making the mainline ov2740 driver suitable
>>>> for use with the (out of tree) IPU6 driver on my TODO list.
>>>> ---
>>>>    drivers/platform/x86/intel/int3472/common.h   |  2 +-
>>>>    drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++----
>>>>    2 files changed, 31 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
>>>> index 53270d19c73a..58647d3084b9 100644
>>>> --- a/drivers/platform/x86/intel/int3472/common.h
>>>> +++ b/drivers/platform/x86/intel/int3472/common.h
>>>> @@ -23,7 +23,7 @@
>>>>    #define INT3472_GPIO_TYPE_PRIVACY_LED                0x0d
>>>>      #define INT3472_PDEV_MAX_NAME_LEN                23
>>>> -#define INT3472_MAX_SENSOR_GPIOS                3
>>>> +#define INT3472_MAX_SENSOR_GPIOS                4
>>>>      #define GPIO_REGULATOR_NAME_LENGTH                21
>>>>    #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH            9
>>>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>>>> index 9159291be28a..bfcf8184db16 100644
>>>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>>>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>>>> @@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type)
>>>>        return "unknown";
>>>>    }
>>>>    +/*
>>>> + * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver,
>>>> + * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and
>>>> + * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled
>>>> + * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk.
>>>> + *
>>>> + * Note there also is a mainline driver for the ov2740, but that does not use
>>>> + * the clk framework atm either.
>>>> + *
>>>> + * Adjust how we handle the GPIOs on these sensors accordingly, for now at least.
>>>> + * This needs to be revisited when we mainline these sensor drivers / when we merge
>>>> + * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6.
>>>> + */
>>>
>>> I'm not really sure about this one though; wouldn't it be better to alter those sensor drivers to use the clock framework properly?
>> Yes, Laurent more or less said the same thing; and I was already starting
>> to think in this direction myself when typing the cover letter.
>>
>> So yes I agree with you and Laurent. That still leaves the question of what to do
>> with devices with just a privacy LED without a clk-en though.
>>
>> Dan, do you have a list of sensors which currently are known to work / be used
>> together with the IPU3 (and the int3472 discrete code) ?
>>
>> I know I will need to modifi the ov5693 code, but I wonder what other drivers
>> I will need to modify ?
> 
> 
> The ov5693, ov8865 and ov7251 are the upstream working ones. There's a couple more that need changes upstreaming, but I can handle those during that process.

Ok thanks.

>> I think I might just move those sensor-drivers over to using a GPIO
>> for the privacy LED and just always register a GPIO for the privacy LED
>> pin, does that sound like a good idea to you ?
> 
> 
> Well if we can't handle it during the int3472 code then yes - I certainly don't have  a better idea.

I will patch the 3 sensors listed above to take an optional privacy LED GPIO then.

I do plan to work on the sensor_power helper library which I discussed
soon-ish since that should make things a lot easier, but for now I'll
just do this the quick and dirty way, also to make backporting easier
for distros (so that they can have a kernel which works with both
IPU3 and the out-of-tree IPU6 stuff).

Regards,

Hans





> 
>> Anyways it is weekend now and I've already worked too many hours this week,
>> so I'll take a look at this on Monday.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>> +static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = {
>>>> +    { "HIMX11B1" }, /* hm11b1 */
>>>> +    { "OVTI01AS" }, /* ov01a1s */
>>>> +    { "INT3474" },  /* ov2740 */
>>>> +    {}
>>>> +};
>>>> +
>>>>    /**
>>>>     * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
>>>>     * @ares: A pointer to a &struct acpi_resource
>>>> @@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>>>>            (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
>>>>          switch (type) {
>>>> +    case INT3472_GPIO_TYPE_CLK_ENABLE:
>>>> +    case INT3472_GPIO_TYPE_PRIVACY_LED:
>>>> +        if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) {
>>>> +            ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type);
>>>> +            if (ret)
>>>> +                err_msg = "Failed to map GPIO to clock\n";
>>>> +
>>>> +            break;
>>>> +        }
>>>> +        fallthrough;
>>>>        case INT3472_GPIO_TYPE_RESET:
>>>>        case INT3472_GPIO_TYPE_POWERDOWN:
>>>>            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);
>>>> -        if (ret)
>>>> -            err_msg = "Failed to map GPIO to clock\n";
>>>> -
>>>>            break;
>>>>        case INT3472_GPIO_TYPE_POWER_ENABLE:
>>>>            ret = skl_int3472_register_regulator(int3472, agpio);
> 

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

[Dan Scally]

Morning Hans - thanks for the set

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


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

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


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

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

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

[Laurent Pinchart]

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

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

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

-- 
Regards,

Laurent Pinchart

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

[Dan Scally]

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


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

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

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

[Andy Shevchenko]

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

...

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Dan Scally]

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

I think the answer to that is yes:


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

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

[Hans de Goede]

Hi,

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

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

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

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

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

Regards,

Hans

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

[Dan Scally]

Hi

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


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


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

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


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

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


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

>
> Regards,
>
> Hans
>
>

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

[Hans de Goede]

Hi,

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

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

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

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

My conclusion here is:

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

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

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

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

Regards,

Hans

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

[Hans de Goede]

Hi,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Regards,

Hans











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

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

[Laurent Pinchart]

Hi Hans,

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

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

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

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

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

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

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

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

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

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

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

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

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

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

<snip>

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

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

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

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

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

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

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

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

Regards,

Hans




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

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

[Laurent Pinchart]

Hi Hans,

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

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

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

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

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

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi,

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

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

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

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

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

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

And then all need is to have something call:

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

And we are done.

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

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

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

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

Regards,

Hans

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

[Laurent Pinchart]

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

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

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

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

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

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi,

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

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

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

so this is all mute.

Regards,

Hans