[PATCH] platform/x86: int3472: Add handshake GPIO function

[PATCH] platform/x86: int3472: Add handshake GPIO function

[Hao Yao]

Handshake pin is used for Lattice MIPI aggregator to enable the
camera sensor. After pulled up, recommend to wail ~250ms to get
everything ready.

Signed-off-by: Hao Yao <hao.yao@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/common.h   | 1 +
 drivers/platform/x86/intel/int3472/discrete.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 655ae3ec0593..3ad4c72afb45 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -23,6 +23,7 @@
 #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
 #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
 #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
+#define INT3472_GPIO_TYPE_HANDSHAKE				0x12
 
 #define INT3472_PDEV_MAX_NAME_LEN				23
 #define INT3472_MAX_SENSOR_GPIOS				3
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index b644ce65c990..4753161b4080 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -111,6 +111,10 @@ static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polar
 		*func = "power-enable";
 		*polarity = GPIO_ACTIVE_HIGH;
 		break;
+	case INT3472_GPIO_TYPE_HANDSHAKE:
+		*func = "handshake";
+		*polarity = GPIO_ACTIVE_HIGH;
+		break;
 	default:
 		*func = "unknown";
 		*polarity = GPIO_ACTIVE_HIGH;
@@ -201,6 +205,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
 	case INT3472_GPIO_TYPE_POWERDOWN:
+	case INT3472_GPIO_TYPE_HANDSHAKE:
 		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
 		if (ret)
 			err_msg = "Failed to map GPIO pin to sensor\n";
-- 
2.34.1

Re: [PATCH] platform/x86: int3472: Add handshake GPIO function

[Bingbu Cao]

Hao, 

On 10/7/23 10:12 AM, Hao Yao wrote:
> Handshake pin is used for Lattice MIPI aggregator to enable the
> camera sensor. After pulled up, recommend to wail ~250ms to get
> everything ready.

Is the delay for specific camera or requirement from Lattice.
250ms is bad for camera.

> 
> Signed-off-by: Hao Yao <hao.yao@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel/int3472/common.h   | 1 +
>  drivers/platform/x86/intel/int3472/discrete.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
> index 655ae3ec0593..3ad4c72afb45 100644
> --- a/drivers/platform/x86/intel/int3472/common.h
> +++ b/drivers/platform/x86/intel/int3472/common.h
> @@ -23,6 +23,7 @@
>  #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
>  #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
>  #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
> +#define INT3472_GPIO_TYPE_HANDSHAKE				0x12
>  
>  #define INT3472_PDEV_MAX_NAME_LEN				23
>  #define INT3472_MAX_SENSOR_GPIOS				3
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index b644ce65c990..4753161b4080 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -111,6 +111,10 @@ static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polar
>  		*func = "power-enable";
>  		*polarity = GPIO_ACTIVE_HIGH;
>  		break;
> +	case INT3472_GPIO_TYPE_HANDSHAKE:
> +		*func = "handshake";
> +		*polarity = GPIO_ACTIVE_HIGH;
> +		break;
>  	default:
>  		*func = "unknown";
>  		*polarity = GPIO_ACTIVE_HIGH;
> @@ -201,6 +205,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  	switch (type) {
>  	case INT3472_GPIO_TYPE_RESET:
>  	case INT3472_GPIO_TYPE_POWERDOWN:
> +	case INT3472_GPIO_TYPE_HANDSHAKE:
>  		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
>  		if (ret)
>  			err_msg = "Failed to map GPIO pin to sensor\n";
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH] platform/x86: int3472: Add handshake GPIO function

[Hao Yao]

Thank you Bingbu,

On 2023/10/10 15:17, Bingbu Cao wrote:
> Hao,
> 
> On 10/7/23 10:12 AM, Hao Yao wrote:
>> Handshake pin is used for Lattice MIPI aggregator to enable the
>> camera sensor. After pulled up, recommend to wail ~250ms to get
>> everything ready.
> 
> Is the delay for specific camera or requirement from Lattice.
> 250ms is bad for camera.
> 

Actually the handshake pin is used by both Altek M1 and Lattice chips. 
As far as I know, Altek M1 required ~250ms delay while recently Lattice 
team told me they don't need delay. However I don't know if there were 
any devices using Altek M1.

>>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel/int3472/common.h   | 1 +
>>   drivers/platform/x86/intel/int3472/discrete.c | 5 +++++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
>> index 655ae3ec0593..3ad4c72afb45 100644
>> --- a/drivers/platform/x86/intel/int3472/common.h
>> +++ b/drivers/platform/x86/intel/int3472/common.h
>> @@ -23,6 +23,7 @@
>>   #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
>>   #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
>>   #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
>> +#define INT3472_GPIO_TYPE_HANDSHAKE				0x12
>>   
>>   #define INT3472_PDEV_MAX_NAME_LEN				23
>>   #define INT3472_MAX_SENSOR_GPIOS				3
>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>> index b644ce65c990..4753161b4080 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -111,6 +111,10 @@ static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polar
>>   		*func = "power-enable";
>>   		*polarity = GPIO_ACTIVE_HIGH;
>>   		break;
>> +	case INT3472_GPIO_TYPE_HANDSHAKE:
>> +		*func = "handshake";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>>   	default:
>>   		*func = "unknown";
>>   		*polarity = GPIO_ACTIVE_HIGH;
>> @@ -201,6 +205,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>>   	switch (type) {
>>   	case INT3472_GPIO_TYPE_RESET:
>>   	case INT3472_GPIO_TYPE_POWERDOWN:
>> +	case INT3472_GPIO_TYPE_HANDSHAKE:
>>   		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
>>   		if (ret)
>>   			err_msg = "Failed to map GPIO pin to sensor\n";
>>
> 


Best Regards,
Hao Yao

Re: [PATCH] platform/x86: int3472: Add handshake GPIO function

[Hans de Goede]

Hi,

On 10/7/23 04:12, Hao Yao wrote:
> Handshake pin is used for Lattice MIPI aggregator to enable the
> camera sensor. After pulled up, recommend to wail ~250ms to get
> everything ready.

If this is a pin on the "Lattice MIPI aggregator" and
not on the sensor itself then this really should be
modeled as such and should not be registered as a GPIO
consumed by the sensor since the actual sensor does not
have a handshake pin at all.

Also we really don't want to need to patch all involved
sensor drivers to toggle a handshake pin, especially since
the sensor itself does not physically have this pin.

Can you explain a bit more:

1. What the "Lattice MIPI aggregator" is 
2. What its functions are, does this control reset + pwdn
   GPIOs for the sensor? Voltages to the sensor? Clk
   to the sensor ?
3. How the aggregator is connected to both the main
   CPU/SoC as well as how it is connected to the sensor ?
   Some example diagram would be really helpful here.

Then with this info in hand we can try to come up
with a way how to model this.

Assuming this controls the entire power-up sequence
for the sensor then I think it could be modelled
as a GPIO regulator. This also allows making the
regulator core take care of the necessary delay
between setting the GPIO and trying to talk to
the sensor.

Regards,

Hans



> 
> Signed-off-by: Hao Yao <hao.yao@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel/int3472/common.h   | 1 +
>  drivers/platform/x86/intel/int3472/discrete.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
> index 655ae3ec0593..3ad4c72afb45 100644
> --- a/drivers/platform/x86/intel/int3472/common.h
> +++ b/drivers/platform/x86/intel/int3472/common.h
> @@ -23,6 +23,7 @@
>  #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
>  #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
>  #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
> +#define INT3472_GPIO_TYPE_HANDSHAKE				0x12
>  
>  #define INT3472_PDEV_MAX_NAME_LEN				23
>  #define INT3472_MAX_SENSOR_GPIOS				3
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index b644ce65c990..4753161b4080 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -111,6 +111,10 @@ static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polar
>  		*func = "power-enable";
>  		*polarity = GPIO_ACTIVE_HIGH;
>  		break;
> +	case INT3472_GPIO_TYPE_HANDSHAKE:
> +		*func = "handshake";
> +		*polarity = GPIO_ACTIVE_HIGH;
> +		break;
>  	default:
>  		*func = "unknown";
>  		*polarity = GPIO_ACTIVE_HIGH;
> @@ -201,6 +205,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  	switch (type) {
>  	case INT3472_GPIO_TYPE_RESET:
>  	case INT3472_GPIO_TYPE_POWERDOWN:
> +	case INT3472_GPIO_TYPE_HANDSHAKE:
>  		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] platform/x86: int3472: Add handshake GPIO function

[Hao Yao]

Hans,

Sorry for the late response.

The power control logic settings in ACPI table are the same between 
Windows OS and Linux, so I directly add another "handshake" pin handling 
to Linux power control driver as what Windows power control driver did.

On 2023/10/12 20:22, Hans de Goede wrote:
> Hi,
> 
> On 10/7/23 04:12, Hao Yao wrote:
>> Handshake pin is used for Lattice MIPI aggregator to enable the
>> camera sensor. After pulled up, recommend to wail ~250ms to get
>> everything ready.
> > If this is a pin on the "Lattice MIPI aggregator" and
> not on the sensor itself then this really should be
> modeled as such and should not be registered as a GPIO
> consumed by the sensor since the actual sensor does not
> have a handshake pin at all.
> 

Yes. This pin is actually connects to Lattice, not sensor.

> Also we really don't want to need to patch all involved
> sensor drivers to toggle a handshake pin, especially since
> the sensor itself does not physically have this pin.

I agree. Adding GPIO pin controlling code in all sensor drivers is not a 
good idea.

> Can you explain a bit more:
> 
> 1. What the "Lattice MIPI aggregator" is

It actually manages all MIPI lanes, both RGB and IR cameras. It is 
something like the iVSC + LJCA combination which are in kernel v6.6 
mainline.

> 2. What its functions are, does this control reset + pwdn
>     GPIOs for the sensor? Voltages to the sensor? Clk
>     to the sensor ?

It starts outputing MIPI packages when we pull handshake pins high. It 
also has USB-IO function which can supply virtual I2C and GPIO for host 
to control the camera (not physically as actually Lattice handles them). 
I didn't get the schematics, but I believe the GPIOs/Voltages/clocks are 
all controlled by Lattice.

> 3. How the aggregator is connected to both the main
>     CPU/SoC as well as how it is connected to the sensor ?
>     Some example diagram would be really helpful here.

In this case Lattice stands between host SoC and camera sensors. All 
MIPI lanes from sensor and all reset/power pins are managed by Lattice. 
Once one of the "handshake" pins on Lattice is pulled high, Lattice 
start to passthrough MIPI packages from corresponding sensor to host SoC.

> Then with this info in hand we can try to come up
> with a way how to model this.
> 
> Assuming this controls the entire power-up sequence
> for the sensor then I think it could be modelled
> as a GPIO regulator. This also allows making the
> regulator core take care of the necessary delay
> between setting the GPIO and trying to talk to
> the sensor.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel/int3472/common.h   | 1 +
>>   drivers/platform/x86/intel/int3472/discrete.c | 5 +++++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
>> index 655ae3ec0593..3ad4c72afb45 100644
>> --- a/drivers/platform/x86/intel/int3472/common.h
>> +++ b/drivers/platform/x86/intel/int3472/common.h
>> @@ -23,6 +23,7 @@
>>   #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
>>   #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
>>   #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
>> +#define INT3472_GPIO_TYPE_HANDSHAKE				0x12
>>   
>>   #define INT3472_PDEV_MAX_NAME_LEN				23
>>   #define INT3472_MAX_SENSOR_GPIOS				3
>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>> index b644ce65c990..4753161b4080 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -111,6 +111,10 @@ static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polar
>>   		*func = "power-enable";
>>   		*polarity = GPIO_ACTIVE_HIGH;
>>   		break;
>> +	case INT3472_GPIO_TYPE_HANDSHAKE:
>> +		*func = "handshake";
>> +		*polarity = GPIO_ACTIVE_HIGH;
>> +		break;
>>   	default:
>>   		*func = "unknown";
>>   		*polarity = GPIO_ACTIVE_HIGH;
>> @@ -201,6 +205,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>>   	switch (type) {
>>   	case INT3472_GPIO_TYPE_RESET:
>>   	case INT3472_GPIO_TYPE_POWERDOWN:
>> +	case INT3472_GPIO_TYPE_HANDSHAKE:
>>   		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
>>   		if (ret)
>>   			err_msg = "Failed to map GPIO pin to sensor\n";
> 

Best Regards,
Hao Yao

Re: [PATCH] platform/x86: int3472: Add handshake GPIO function

[Anthony I Gilea]

Hello,

I'm trying to get camera on HP Spectre x360 14-eu0xxx (2024) laptop to work.
I was able to make main sensor driver (ov08x40) to play nice with IPU6,
INT3472 and libcamera-SoftISP and the resulting image quality is absolutely
usable and even surprisingly good.

This laptop also uses this "MIPI aggregator".


> Hi,
> 
> On 10/7/23 04:12, Hao Yao wrote:
> > Handshake pin is used for Lattice MIPI aggregator to enable the
> > camera sensor. After pulled up, recommend to wail ~250ms to get
> > everything ready.
> 
> If this is a pin on the "Lattice MIPI aggregator" and
> not on the sensor itself then this really should be
> modeled as such and should not be registered as a GPIO
> consumed by the sensor since the actual sensor does not
> have a handshake pin at all.
> 
> Also we really don't want to need to patch all involved
> sensor drivers to toggle a handshake pin, especially since
> the sensor itself does not physically have this pin.
> 
> Can you explain a bit more:
> 
> 1. What the "Lattice MIPI aggregator" is.
> 2. What its functions are, does this control reset + pwdn
>    GPIOs for the sensor? Voltages to the sensor? Clk
>    to the sensor ?

It acts like MIPI switch as no MIPI data gets from the sensor to IPU6 if
handshake signal is not asserted. Eventually IPU6 times out with "start stream
of firmware failed" message. Any further attempts to start streaming lead to
a panic.

I2C communication is not affected by the handshake signal but it looks like
reset signal is also going through this "MIPI aggregator" as it takes about
150ms for the sensor to reliably start responding via I2C after the reset
is deasserted. It should be about few ms if the reset signal was connected to
the sensor directly.

> 3. How the aggregator is connected to both the main
>    CPU/SoC as well as how it is connected to the sensor ?
>    Some example diagram would be really helpful here.
> 
> Then with this info in hand we can try to come up
> with a way how to model this.
> 
> Assuming this controls the entire power-up sequence
> for the sensor then I think it could be modelled
> as a GPIO regulator. This also allows making the
> regulator core take care of the necessary delay
> between setting the GPIO and trying to talk to
> the sensor.

Are there any updates on how this signal should be implemented? For now I'm
just applying this patch and asserting it from the sensor driver.

Regards


> 
> Regards,
> 
> Hans
> 
> 
> 
> > 
> > Signed-off-by: Hao Yao <hao.yao@intel.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel/int3472/common.h   | 1 +
> >  drivers/platform/x86/intel/int3472/discrete.c | 5 +++++
> >  2 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
> > index 655ae3ec0593..3ad4c72afb45 100644
> > --- a/drivers/platform/x86/intel/int3472/common.h
> > +++ b/drivers/platform/x86/intel/int3472/common.h
> > @@ -23,6 +23,7 @@
> >  #define INT3472_GPIO_TYPE_POWER_ENABLE				0x0b
> >  #define INT3472_GPIO_TYPE_CLK_ENABLE				0x0c
> >  #define INT3472_GPIO_TYPE_PRIVACY_LED				0x0d
> > +#define INT3472_GPIO_TYPE_HANDSHAKE				0x12
> >  
> >  #define INT3472_PDEV_MAX_NAME_LEN				23
> >  #define INT3472_MAX_SENSOR_GPIOS				3
> > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> > index b644ce65c990..4753161b4080 100644
> > --- a/drivers/platform/x86/intel/int3472/discrete.c
> > +++ b/drivers/platform/x86/intel/int3472/discrete.c
> > @@ -111,6 +111,10 @@ static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polar
> >  		*func = "power-enable";
> >  		*polarity = GPIO_ACTIVE_HIGH;
> >  		break;
> > +	case INT3472_GPIO_TYPE_HANDSHAKE:
> > +		*func = "handshake";
> > +		*polarity = GPIO_ACTIVE_HIGH;
> > +		break;
> >  	default:
> >  		*func = "unknown";
> >  		*polarity = GPIO_ACTIVE_HIGH;
> > @@ -201,6 +205,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
> >  	switch (type) {
> >  	case INT3472_GPIO_TYPE_RESET:
> >  	case INT3472_GPIO_TYPE_POWERDOWN:
> > +	case INT3472_GPIO_TYPE_HANDSHAKE:
> >  		ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity);
> >  		if (ret)
> >  			err_msg = "Failed to map GPIO pin to sensor\n";
>