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

[Hao Yao <hao.yao@intel.com>]

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