[PATCH v2 0/3] platform/x86: int3472: Increase ov08x40 handshake GPIO delay to 45 ms

[PATCH v2 0/3] platform/x86: int3472: Increase ov08x40 handshake GPIO delay to 45 ms

[Hans de Goede]

Hi All,

Here is v2 of the patch-series to fix ov08x40 based cameras not working
on several HP laptop models.

Changes in v2:
- Convert int3472_gpio_map to use C99 initializers
- s/enable_time/enable_time_us/
- Move enable_time above con_id for better struct packing

Regards,

Hans


Hans de Goede (3):
  platform/x86: int3472: Convert int3472_gpio_map to use C99
    initializers
  platform/x86: int3472: Rework regulator enable-time handling
  platform/x86: int3472: Increase ov08x40 handshake GPIO delay to 45 ms

 drivers/platform/x86/intel/int3472/discrete.c | 58 ++++++++++++-------
 1 file changed, 38 insertions(+), 20 deletions(-)

-- 
2.49.0

[PATCH v2 1/3] platform/x86: int3472: Convert int3472_gpio_map to use C99 initializers

[Hans de Goede]

Convert int3472_gpio_map to use C99 initializers to make it clearer which
struct field is set to which value.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/platform/x86/intel/int3472/discrete.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 4c0aed6e626f..4ffd7330e9ea 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -142,10 +142,18 @@ struct int3472_gpio_map {
 };
 
 static const struct int3472_gpio_map int3472_gpio_map[] = {
-	/* mt9m114 designs declare a powerdown pin which controls the regulators */
-	{ "INT33F0", INT3472_GPIO_TYPE_POWERDOWN, INT3472_GPIO_TYPE_POWER_ENABLE, false, "vdd" },
-	/* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
-	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, false, "enable" },
+	{	/* mt9m114 designs declare a powerdown pin which controls the regulators */
+		.hid = "INT33F0",
+		.type_from = INT3472_GPIO_TYPE_POWERDOWN,
+		.type_to = INT3472_GPIO_TYPE_POWER_ENABLE,
+		.con_id = "vdd",
+	},
+	{	/* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
+		.hid = "INT347E",
+		.type_from = INT3472_GPIO_TYPE_RESET,
+		.type_to = INT3472_GPIO_TYPE_RESET,
+		.con_id = "enable"
+	},
 };
 
 static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3472, u8 *type,
-- 
2.49.0

[PATCH v2 2/3] platform/x86: int3472: Rework regulator enable-time handling

[Hans de Goede]

Instead of hardcoding the regulator enable-time for INT3472_GPIO_TYPE-
POWER_ENABLE and -HANDSHAKE, make int3472_get_con_id_and_polarity()
set the enable-time.

This will allow overriding the enable time through quirks in
the int3472_gpio_map[].

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v2:
- s/enable_time/enable_time_us/
- Move enable_time above con_id for better struct packing
---
 drivers/platform/x86/intel/int3472/discrete.c | 35 ++++++++++---------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 4ffd7330e9ea..74239ce38805 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -129,6 +129,7 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
  * @hid: The ACPI HID of the device without the instance number e.g. INT347E
  * @type_from: The GPIO type from ACPI ?SDT
  * @type_to: The assigned GPIO type, typically same as @type_from
+ * @enable_time_us: Enable time in usec for GPIOs mapped to regulators
  * @con_id: The name of the GPIO for the device
  * @polarity_low: GPIO_ACTIVE_LOW true if the @polarity_low is true,
  * GPIO_ACTIVE_HIGH otherwise
@@ -138,6 +139,7 @@ struct int3472_gpio_map {
 	u8 type_from;
 	u8 type_to;
 	bool polarity_low;
+	unsigned int enable_time_us;
 	const char *con_id;
 };
 
@@ -147,6 +149,7 @@ static const struct int3472_gpio_map int3472_gpio_map[] = {
 		.type_from = INT3472_GPIO_TYPE_POWERDOWN,
 		.type_to = INT3472_GPIO_TYPE_POWER_ENABLE,
 		.con_id = "vdd",
+		.enable_time_us = GPIO_REGULATOR_ENABLE_TIME,
 	},
 	{	/* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
 		.hid = "INT347E",
@@ -157,7 +160,8 @@ static const struct int3472_gpio_map int3472_gpio_map[] = {
 };
 
 static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3472, u8 *type,
-					    const char **con_id, unsigned long *gpio_flags)
+					    const char **con_id, unsigned long *gpio_flags,
+					    unsigned int *enable_time_us)
 {
 	struct acpi_device *adev = int3472->sensor;
 	unsigned int i;
@@ -181,9 +185,12 @@ static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3
 		*gpio_flags = int3472_gpio_map[i].polarity_low ?
 			      GPIO_ACTIVE_LOW : GPIO_ACTIVE_HIGH;
 		*con_id = int3472_gpio_map[i].con_id;
+		*enable_time_us = int3472_gpio_map[i].enable_time_us;
 		return;
 	}
 
+	*enable_time_us = GPIO_REGULATOR_ENABLE_TIME;
+
 	switch (*type) {
 	case INT3472_GPIO_TYPE_RESET:
 		*con_id = "reset";
@@ -208,6 +215,8 @@ static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3
 	case INT3472_GPIO_TYPE_HANDSHAKE:
 		*con_id = "dvdd";
 		*gpio_flags = GPIO_ACTIVE_HIGH;
+		/* Setups using a handshake pin need 25 ms enable delay */
+		*enable_time_us = 25 * USEC_PER_MSEC;
 		break;
 	default:
 		*con_id = "unknown";
@@ -252,13 +261,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 					     void *data)
 {
 	struct int3472_discrete_device *int3472 = data;
+	const char *second_sensor = NULL;
 	struct acpi_resource_gpio *agpio;
+	unsigned int enable_time_us;
 	u8 active_value, pin, type;
+	unsigned long gpio_flags;
 	union acpi_object *obj;
 	struct gpio_desc *gpio;
 	const char *err_msg;
 	const char *con_id;
-	unsigned long gpio_flags;
 	int ret;
 
 	if (!acpi_gpio_get_io_resource(ares, &agpio))
@@ -281,7 +292,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	type = FIELD_GET(INT3472_GPIO_DSM_TYPE, obj->integer.value);
 
-	int3472_get_con_id_and_polarity(int3472, &type, &con_id, &gpio_flags);
+	int3472_get_con_id_and_polarity(int3472, &type, &con_id, &gpio_flags, &enable_time_us);
 
 	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
 	/* Pin field is not really used under Windows and wraps around at 8 bits */
@@ -330,21 +341,13 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 			break;
 		case INT3472_GPIO_TYPE_POWER_ENABLE:
-			ret = skl_int3472_register_regulator(int3472, gpio,
-							     GPIO_REGULATOR_ENABLE_TIME,
-							     con_id,
-							     int3472->quirks.avdd_second_sensor);
-			if (ret)
-				err_msg = "Failed to map power-enable to sensor\n";
-
-			break;
+			second_sensor = int3472->quirks.avdd_second_sensor;
+			fallthrough;
 		case INT3472_GPIO_TYPE_HANDSHAKE:
-			/* Setups using a handshake pin need 25 ms enable delay */
-			ret = skl_int3472_register_regulator(int3472, gpio,
-							     25 * USEC_PER_MSEC,
-							     con_id, NULL);
+			ret = skl_int3472_register_regulator(int3472, gpio, enable_time_us,
+							     con_id, second_sensor);
 			if (ret)
-				err_msg = "Failed to map handshake to sensor\n";
+				err_msg = "Failed to register regulator\n";
 
 			break;
 		default: /* Never reached */
-- 
2.49.0

[PATCH v2 3/3] platform/x86: int3472: Increase ov08x40 handshake GPIO delay to 45 ms

[Hans de Goede]

On HP laptops with an ov08x40 sensor the 25 ms delay coming from Intel's
out of tree drivers is not enough. Testing has confirmed that 45 ms does
work.

Add a quirk to the int3472_gpio_map[] to increase the delay to 45 ms to fix
probing of the ov08x40 sensor failing on these laptops.

Note this only impacts laptops which actually use an ov08x40 sensor with
a handshake GPIO.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2333331
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/platform/x86/intel/int3472/discrete.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 74239ce38805..876bb75d4cf1 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -157,6 +157,13 @@ static const struct int3472_gpio_map int3472_gpio_map[] = {
 		.type_to = INT3472_GPIO_TYPE_RESET,
 		.con_id = "enable"
 	},
+	{	/* ov08x40's handshake pin needs a 45 ms delay on some HP laptops */
+		.hid = "OVTI08F4",
+		.type_from = INT3472_GPIO_TYPE_HANDSHAKE,
+		.type_to = INT3472_GPIO_TYPE_HANDSHAKE,
+		.con_id = "dvdd",
+		.enable_time_us = 45 * USEC_PER_MSEC,
+	},
 };
 
 static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3472, u8 *type,
-- 
2.49.0

Re: [PATCH v2 1/3] platform/x86: int3472: Convert int3472_gpio_map to use C99 initializers

[Andy Shevchenko]

On Fri, Jul 25, 2025 at 11:53 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Convert int3472_gpio_map to use C99 initializers to make it clearer which
> struct field is set to which value.

...

> +       {       /* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
> +               .hid = "INT347E",
> +               .type_from = INT3472_GPIO_TYPE_RESET,
> +               .type_to = INT3472_GPIO_TYPE_RESET,
> +               .con_id = "enable"

+ comma. But no need to resend just for that reason, I hope Ilpo can
tweak it whilst applying.

> +       },
>  };


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] platform/x86: int3472: Increase ov08x40 handshake GPIO delay to 45 ms

[Andy Shevchenko]

On Fri, Jul 25, 2025 at 11:53 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Hi All,
>
> Here is v2 of the patch-series to fix ov08x40 based cameras not working
> on several HP laptop models.
>
> Changes in v2:
> - Convert int3472_gpio_map to use C99 initializers
> - s/enable_time/enable_time_us/
> - Move enable_time above con_id for better struct packing

This version LGTM, FWIW,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

(one nit-pick in one patch, but it is minor thing)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] platform/x86: int3472: Increase ov08x40 handshake GPIO delay to 45 ms

[Sakari Ailus]

Hi Hans,

On Fri, Jul 25, 2025 at 11:52:56PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is v2 of the patch-series to fix ov08x40 based cameras not working
> on several HP laptop models.
> 
> Changes in v2:
> - Convert int3472_gpio_map to use C99 initializers
> - s/enable_time/enable_time_us/
> - Move enable_time above con_id for better struct packing

Thanks for the set.

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

-- 
Sakari Ailus

Re: [PATCH v2 0/3] platform/x86: int3472: Increase ov08x40 handshake GPIO delay to 45 ms

[Ilpo Järvinen]

On Fri, 25 Jul 2025 23:52:56 +0200, Hans de Goede wrote:

> Here is v2 of the patch-series to fix ov08x40 based cameras not working
> on several HP laptop models.
> 
> Changes in v2:
> - Convert int3472_gpio_map to use C99 initializers
> - s/enable_time/enable_time_us/
> - Move enable_time above con_id for better struct packing
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo-next branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-next branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/3] platform/x86: int3472: Convert int3472_gpio_map to use C99 initializers
      commit: b33b696f86f6d17c9083d69a13d7f32380d818d3
[2/3] platform/x86: int3472: Rework regulator enable-time handling
      commit: f11f8948a826afba66355302ba7e4b3e36333038
[3/3] platform/x86: int3472: Increase ov08x40 handshake GPIO delay to 45 ms
      commit: 30359c239ba8394c8e774151d26914db18dc4976

--
 i.