Re: [PATCH v3] platform/x86: int3472: support the POWER1 GPIO type

[Hans de Goede]

Hi,

On 29-Aug-26 10:29, Jakob Berg Jespersen wrote:
> INT3472 can describe a second sensor power rail as a GPIO of type
> POWER1 (0x08), which the driver does not recognise, so the rail is left
> unmapped and never enabled:
> 
>   int3472-discrete INT3472:01: GPIO type 0x08 unknown; the sensor may
>   not work
> 
> On the Microsoft Surface Pro 7+ the rear camera's INT3472 (INT347A,
> ov8865) has such a pin; without it the ov8865 "dvdd" supply resolves to
> a dummy regulator and the sensor never probes.
> 
> Define the POWER0 (0x07) and POWER1 (0x08) GPIO types and map POWER1 to
> a regulator with con_id "dvdd" for all devices, the supply the in-tree
> ov8865 driver already requests. POWER0 is defined but left unmapped, as
> no device that uses it is known.
> 
> Out-of-tree work approached the same rail by exposing it to the sensor
> as a new "pwr1" supply (linux-surface PR #1867 for the Surface Pro 9,
> PR #2201 for the Pro 7+ ov8865); mapping POWER1 to the existing "dvdd"
> keeps the change contained to int3472.
> 
> With this change the Surface Pro 7+ rear camera probes and streams
> reliably.
> 
> Link: https://github.com/linux-surface/linux-surface/pull/1867
> Link: https://github.com/linux-surface/linux-surface/pull/2201
> Suggested-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Signed-off-by: Jakob Berg Jespersen <dev@xxxxxxx>


Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>

Regards,

Hans

> ---
> Changes in v3:
> - No functional changes; the patch is identical to v2, rebased onto
>   current pdx86/for-next (which has since gained the multi-HID GPIO map
>   entries and the IMX471 "vana" entries).
> - Resent with linux-media@xxxxxxxxxxxxxxx on Cc, as requested.
> - Link to v2: https://patch.msgid.link/20260729-sp7plus-int3472-v2-1-cdfaf97ac3ad@xxxxxxx
> 
> Changes in v2:
> 
> - Reworked from a per-device INT347A quirk into a generic mapping of the
>   POWER1 (0x08) GPIO type to "dvdd" for all devices, per review.
> - Added INT3472_GPIO_TYPE_POWER0 (0x07) and POWER1 (0x08) definitions;
>   POWER0 is defined but left unmapped for now.
> - Retested on the Surface Pro 7+ (kernel 7.1.5): the rear ov8865 probes,
>   instantiates its VCM, and streams.
> - Link to v1: https://patch.msgid.link/20260719-sp7plus-int3472-v1-1-521a43f5c191@xxxxxxx
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 8 ++++++++
>  include/linux/platform_data/x86/int3472.h     | 2 ++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 6c729fcfce5d..6ab8e589fcd3 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -266,6 +266,10 @@ static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3
>  		*con_id = "avdd";
>  		*gpio_flags = GPIO_ACTIVE_HIGH;
>  		break;
> +	case INT3472_GPIO_TYPE_POWER1:
> +		*con_id = "dvdd";
> +		*gpio_flags = GPIO_ACTIVE_HIGH;
> +		break;
>  	case INT3472_GPIO_TYPE_DOVDD:
>  		*con_id = "dovdd";
>  		*gpio_flags = GPIO_ACTIVE_HIGH;
> @@ -296,6 +300,8 @@ static void int3472_get_con_id_and_polarity(struct int3472_discrete_device *int3
>   * 0x00 Reset
>   * 0x01 Power down
>   * 0x02 Strobe
> + * 0x07 Power 0
> + * 0x08 Power 1
>   * 0x0b Power enable
>   * 0x0c Clock enable
>   * 0x0d Privacy LED
> @@ -382,6 +388,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  	case INT3472_GPIO_TYPE_PRIVACY_LED:
>  	case INT3472_GPIO_TYPE_STROBE:
>  	case INT3472_GPIO_TYPE_POWER_ENABLE:
> +	case INT3472_GPIO_TYPE_POWER1:
>  	case INT3472_GPIO_TYPE_DOVDD:
>  	case INT3472_GPIO_TYPE_HANDSHAKE:
>  		gpio = skl_int3472_gpiod_get_from_temp_lookup(int3472, agpio, con_id, gpio_flags);
> @@ -408,6 +415,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  		case INT3472_GPIO_TYPE_POWER_ENABLE:
>  			second_sensor = int3472->quirks.avdd_second_sensor;
>  			fallthrough;
> +		case INT3472_GPIO_TYPE_POWER1:
>  		case INT3472_GPIO_TYPE_DOVDD:
>  		case INT3472_GPIO_TYPE_HANDSHAKE:
>  			ret = skl_int3472_register_regulator(int3472, gpio, enable_time_us,
> diff --git a/include/linux/platform_data/x86/int3472.h b/include/linux/platform_data/x86/int3472.h
> index a73841dfae27..b1040e36deb8 100644
> --- a/include/linux/platform_data/x86/int3472.h
> +++ b/include/linux/platform_data/x86/int3472.h
> @@ -25,6 +25,8 @@
>  #define INT3472_GPIO_TYPE_RESET					0x00
>  #define INT3472_GPIO_TYPE_POWERDOWN				0x01
>  #define INT3472_GPIO_TYPE_STROBE				0x02
> +#define INT3472_GPIO_TYPE_POWER0				0x07
> +#define INT3472_GPIO_TYPE_POWER1				0x08
>  #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
>  #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
>  #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
> 
> ---
> base-commit: 5b05bb3f6c5716fab6911e12d60dd1f43ad9806a
> change-id: 20260719-sp7plus-int3472-8f014b8de79c
> 
> Best regards,
> --  
> Jakob Berg Jespersen <dev@xxxxxxx>
>