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

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

[Hans de Goede]

Hi All,

These 2 patches together fix ov08x40 based cameras not working on several
HP laptop models.

It took quite a while to get to the bottom of this, see:
https://bugzilla.redhat.com/show_bug.cgi?id=2333331

But the fix is quite simpel :)

Regards,

Hans


Hans de Goede (2):
  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 | 45 +++++++++++--------
 1 file changed, 27 insertions(+), 18 deletions(-)

-- 
2.49.0

[PATCH 1/2] 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>
---
 drivers/platform/x86/intel/int3472/discrete.c | 39 ++++++++++---------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 4c0aed6e626f..bc442944be7f 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -130,7 +130,8 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472,
  * @type_from: The GPIO type from ACPI ?SDT
  * @type_to: The assigned GPIO type, typically same as @type_from
  * @con_id: The name of the GPIO for the device
- * @polarity_low: GPIO_ACTIVE_LOW true if the @polarity_low is true,
+ * @polarity_low: GPIO_ACTIVE_LOW true if the @polarity_low is true
+ * @enable_time: Enable time in usec for GPIOs mapped to regulators
  * GPIO_ACTIVE_HIGH otherwise
  */
 struct int3472_gpio_map {
@@ -139,17 +140,20 @@ struct int3472_gpio_map {
 	u8 type_to;
 	bool polarity_low;
 	const char *con_id;
+	unsigned int enable_time;
 };
 
 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" },
+	{ "INT33F0", INT3472_GPIO_TYPE_POWERDOWN, INT3472_GPIO_TYPE_POWER_ENABLE,
+	  false, "vdd", GPIO_REGULATOR_ENABLE_TIME },
 	/* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
 	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, false, "enable" },
 };
 
 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)
 {
 	struct acpi_device *adev = int3472->sensor;
 	unsigned int i;
@@ -173,9 +177,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 = int3472_gpio_map[i].enable_time;
 		return;
 	}
 
+	*enable_time = GPIO_REGULATOR_ENABLE_TIME;
+
 	switch (*type) {
 	case INT3472_GPIO_TYPE_RESET:
 		*con_id = "reset";
@@ -200,6 +207,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 = 25 * USEC_PER_MSEC;
 		break;
 	default:
 		*con_id = "unknown";
@@ -244,13 +253,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;
 	u8 active_value, pin, type;
+	unsigned long gpio_flags;
+	unsigned int enable_time;
 	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))
@@ -273,7 +284,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);
 
 	pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value);
 	/* Pin field is not really used under Windows and wraps around at 8 bits */
@@ -322,21 +333,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,
+							     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

Re: [PATCH 1/2] platform/x86: int3472: Rework regulator enable-time handling

[Andy Shevchenko]

On Fri, Jul 25, 2025 at 4:44 PM Hans de Goede <hansg@kernel.org> wrote:
>
> 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[].

...

> + * @enable_time: Enable time in usec for GPIOs mapped to regulators
>   * GPIO_ACTIVE_HIGH otherwise

Make it enable_time_us, it will immediately give a hint in the code
without going to here to get units.

...

>  struct int3472_gpio_map {

>         u8 type_to;
>         bool polarity_low;
>         const char *con_id;
> +       unsigned int enable_time;

Wondering if this can be moved before con_id to save a few bytes on 64-bit.

>  };

...

>  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" },
> +       { "INT33F0", INT3472_GPIO_TYPE_POWERDOWN, INT3472_GPIO_TYPE_POWER_ENABLE,
> +         false, "vdd", GPIO_REGULATOR_ENABLE_TIME },
>         /* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
>         { "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, false, "enable" },

Can we actually either use a macro or at least C99 initializers?

>  };

...

> +                                           unsigned int *enable_time)

_us

...

> +       *enable_time = GPIO_REGULATOR_ENABLE_TIME;

_us
_US

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] platform/x86: int3472: Rework regulator enable-time handling

[Hans de Goede]

Hi Andy,

Thank you for your review.

On 25-Jul-25 4:49 PM, Andy Shevchenko wrote:
> On Fri, Jul 25, 2025 at 4:44 PM Hans de Goede <hansg@kernel.org> wrote:
>>
>> 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[].
> 
> ...
> 
>> + * @enable_time: Enable time in usec for GPIOs mapped to regulators
>>   * GPIO_ACTIVE_HIGH otherwise
> 
> Make it enable_time_us, it will immediately give a hint in the code
> without going to here to get units.
> 
> ...
> 
>>  struct int3472_gpio_map {
> 
>>         u8 type_to;
>>         bool polarity_low;
>>         const char *con_id;
>> +       unsigned int enable_time;
> 
> Wondering if this can be moved before con_id to save a few bytes on 64-bit.
> 
>>  };
> 
> ...
> 
>>  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" },
>> +       { "INT33F0", INT3472_GPIO_TYPE_POWERDOWN, INT3472_GPIO_TYPE_POWER_ENABLE,
>> +         false, "vdd", GPIO_REGULATOR_ENABLE_TIME },
>>         /* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
>>         { "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, false, "enable" },
> 
> Can we actually either use a macro or at least C99 initializers?
> 
>>  };
> 
> ...
> 
>> +                                           unsigned int *enable_time)
> 
> _us
> 
> ...
> 
>> +       *enable_time = GPIO_REGULATOR_ENABLE_TIME;
> 
> _us
> _US

Ack,

I agree with all your remarks. I'll fix these for v2.

Regards,

Hans

[PATCH 2/2] 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index bc442944be7f..e7ca1d83f45a 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -149,6 +149,12 @@ static const struct int3472_gpio_map int3472_gpio_map[] = {
 	  false, "vdd", GPIO_REGULATOR_ENABLE_TIME },
 	/* ov7251 driver / DT-bindings expect "enable" as con_id for reset */
 	{ "INT347E", INT3472_GPIO_TYPE_RESET, INT3472_GPIO_TYPE_RESET, false, "enable" },
+	/*
+	 * ov08x40 sensor with a handshake pin needs a 45 ms delay on some HP laptops
+	 * https://bugzilla.redhat.com/show_bug.cgi?id=2333331
+	 */
+	{ "OVTI08F4", INT3472_GPIO_TYPE_HANDSHAKE, INT3472_GPIO_TYPE_HANDSHAKE,
+	  false, "dvdd", 45 * USEC_PER_MSEC },
 };
 
 static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3472, u8 *type,
-- 
2.49.0

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

[Andy Shevchenko]

On Fri, Jul 25, 2025 at 4:44 PM Hans de Goede <hansg@kernel.org> wrote:
>
> 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.

...

> +       /*
> +        * ov08x40 sensor with a handshake pin needs a 45 ms delay on some HP laptops
> +        * https://bugzilla.redhat.com/show_bug.cgi?id=2333331
> +        */
> +       { "OVTI08F4", INT3472_GPIO_TYPE_HANDSHAKE, INT3472_GPIO_TYPE_HANDSHAKE,
> +         false, "dvdd", 45 * USEC_PER_MSEC },
>  };

My gut feeling is that this might be needed for most of the cameras
with the handshake signal. Do you have ones that work without this
delay?

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

Hi Hans, Andy,

On Fri, Jul 25, 2025 at 04:52:26PM +0200, Andy Shevchenko wrote:
> On Fri, Jul 25, 2025 at 4:44 PM Hans de Goede <hansg@kernel.org> wrote:
> >
> > 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.
> 
> ...
> 
> > +       /*
> > +        * ov08x40 sensor with a handshake pin needs a 45 ms delay on some HP laptops
> > +        * https://bugzilla.redhat.com/show_bug.cgi?id=2333331
> > +        */
> > +       { "OVTI08F4", INT3472_GPIO_TYPE_HANDSHAKE, INT3472_GPIO_TYPE_HANDSHAKE,
> > +         false, "dvdd", 45 * USEC_PER_MSEC },
> >  };
> 
> My gut feeling is that this might be needed for most of the cameras
> with the handshake signal. Do you have ones that work without this
> delay?

I'd expect this to depend on the CV chip, if not solely then primarily at
least (firmware could play a part maybe??). Isn't there a way to figure
that out?

-- 
Regards,

Sakari Ailus

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

[Hans de Goede]

Hi,

On 25-Jul-25 7:47 PM, Sakari Ailus wrote:
> Hi Hans, Andy,
> 
> On Fri, Jul 25, 2025 at 04:52:26PM +0200, Andy Shevchenko wrote:
>> On Fri, Jul 25, 2025 at 4:44 PM Hans de Goede <hansg@kernel.org> wrote:
>>>
>>> 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.
>>
>> ...
>>
>>> +       /*
>>> +        * ov08x40 sensor with a handshake pin needs a 45 ms delay on some HP laptops
>>> +        * https://bugzilla.redhat.com/show_bug.cgi?id=2333331
>>> +        */
>>> +       { "OVTI08F4", INT3472_GPIO_TYPE_HANDSHAKE, INT3472_GPIO_TYPE_HANDSHAKE,
>>> +         false, "dvdd", 45 * USEC_PER_MSEC },
>>>  };
>>
>> My gut feeling is that this might be needed for most of the cameras
>> with the handshake signal. Do you have ones that work without this
>> delay?

For those laptops with the CV chip that Sakari talks about below,
25 ms, which is what is currently the default delay seems to be
enough.

> I'd expect this to depend on the CV chip, if not solely then primarily at
> least (firmware could play a part maybe??). Isn't there a way to figure
> that out?

These HP laptops do not appear to use a CV chip at all, yet they do
have a handshake signal... Specifically there is no CV / USBIO chip
on the USB bus and the I2c and GPIOs for the camera sensor come
directly from the main SoC.

With that said, even if there were a CV chip, then given all
the problems we're having with those I do not expect us to be
able to get this info from the CV chip.

Just getting the basic IO-expander functionality upstream has
been and still is a very troublesome process.

Which includes very different behavior between USBIO chips which report
the exact same firmware version, it looks like the fw-version reported
over USB does not always get updated in fw updates...

Regards,

Hans

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

[Sakari Ailus]

Hi Hans,

On Fri, Jul 25, 2025 at 10:43:17PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 25-Jul-25 7:47 PM, Sakari Ailus wrote:
> > Hi Hans, Andy,
> > 
> > On Fri, Jul 25, 2025 at 04:52:26PM +0200, Andy Shevchenko wrote:
> >> On Fri, Jul 25, 2025 at 4:44 PM Hans de Goede <hansg@kernel.org> wrote:
> >>>
> >>> 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.
> >>
> >> ...
> >>
> >>> +       /*
> >>> +        * ov08x40 sensor with a handshake pin needs a 45 ms delay on some HP laptops
> >>> +        * https://bugzilla.redhat.com/show_bug.cgi?id=2333331
> >>> +        */
> >>> +       { "OVTI08F4", INT3472_GPIO_TYPE_HANDSHAKE, INT3472_GPIO_TYPE_HANDSHAKE,
> >>> +         false, "dvdd", 45 * USEC_PER_MSEC },
> >>>  };
> >>
> >> My gut feeling is that this might be needed for most of the cameras
> >> with the handshake signal. Do you have ones that work without this
> >> delay?
> 
> For those laptops with the CV chip that Sakari talks about below,
> 25 ms, which is what is currently the default delay seems to be
> enough.
> 
> > I'd expect this to depend on the CV chip, if not solely then primarily at
> > least (firmware could play a part maybe??). Isn't there a way to figure
> > that out?
> 
> These HP laptops do not appear to use a CV chip at all, yet they do
> have a handshake signal... Specifically there is no CV / USBIO chip
> on the USB bus and the I2c and GPIOs for the camera sensor come
> directly from the main SoC.
> 
> With that said, even if there were a CV chip, then given all
> the problems we're having with those I do not expect us to be
> able to get this info from the CV chip.

I wouldn't expect it from the chip, but knowing which chip is there could
be useful in determining this.

It'd be good to understand what's going on, even if that change fixes the
problem. Given there may be more pressing problems it may well remain a
mystery. :-(

But it's not the sensor I presume, 5 ms should be enough since lifting
XSHUTDOWN. It'd be good to add a comment on which models this applies to,
besides the bug URL.

I'll review v2 later this week.

> 
> Just getting the basic IO-expander functionality upstream has
> been and still is a very troublesome process.
> 
> Which includes very different behavior between USBIO chips which report
> the exact same firmware version, it looks like the fw-version reported
> over USB does not always get updated in fw updates...

Ouch!

-- 
Regards,

Sakari Ailus