| Message ID | 20231007021225.9240-1-hao.yao@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: int3472: Add handshake GPIO function | expand |
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"; >
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
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";
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
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"; >
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";