Ipu3 cameras - latest version

[djrscally]

So this PR reverts the old version of the camera work and then adds the newest version, being the cio2-bridge code that was just picked up, plus the first revision to the INT3472 driver - that's become a new module that loads 2 separate drivers, so the Kconfig option has changed from INT3472 to INTEL_SKL_INT3472.

Note that we were wrong about what the type byte of the _DSM entries for each GPIO represent; only one type (0x0b) is a regulator enable pin, and it's not usually present (I could only find it on the Go1/2 and my Lenovo Miix in the DSDT dumps we have) so this should work out of the box without adding in the IDs for the sensor modules on the other platforms like we did before (as there's no device specific settings we need to make for them)

There's also a bugfix from Rafael that is needed to make this work properly (or the INT3472's never show up as platform devices) and I added the clk*() functions back into the ov5693 driver (turns out one GPIO line turns on the clock).

I have tested this on my Miix510 and my Go2, but some more devices tried out would be really nice :)

[fabwu]

I get the following error when I run this patch on my SB2:

[  109.000863] ov5693 i2c-INT33BE:00: ov5693_probe() called
[  109.000948] ov5693 i2c-INT33BE:00: supply avdd not found, using dummy regulator
[  109.000954] debugfs: Directory 'i2c-INT33BE:00-avdd' with parent 'reg-dummy-regulator-dummy' already present!
[  109.000959] ov5693 i2c-INT33BE:00: supply dovdd not found, using dummy regulator
[  109.000962] debugfs: Directory 'i2c-INT33BE:00-dovdd' with parent 'reg-dummy-regulator-dummy' already present!
[  109.000969] gpiod_set_value_cansleep: invalid GPIO (errorpointer)
[  109.035585] ov5693 i2c-INT33BE:00: read from offset 0x300a error -121
[  109.035589] ov5693 i2c-INT33BE:00: sensor_id_high = 0x0
[  109.035590] ov5693 i2c-INT33BE:00: ov5693_detect err s_config.
[  109.035598] gpiod_set_value_cansleep: invalid GPIO (errorpointer)
[  109.035601] ov5693 i2c-INT33BE:00: sensor power-gating failed

@djrscally I'm not very familiar with this part of the system so I might did some configuration wrong

[djrscally]

@fabwu thanks for testing!

Seems a bit strange that the dummy regulator directories already exist, but probably not a problem. gpiod_set_value_cansleep() being called against an error pointer is definitely a problem though. Did you definitely enable (M or Y, either should be fine) the INTEL_SKL_INT3472 option in config? Did it load ok, and you can see it in lsmod | grep int3472? And can you run dmesg | grep int3472 and post any output? Also, can you run cd /sys/bus/platform/devices/INT3472:00 && ls -l and post that output too?

[fabwu]

Edit: Sorry I commented out a code section which messed everything up so I let me start over...

After startup the int3472 module seems to load fine without any errors:

[me@box ~]$ lsmod | grep int3472
intel_skl_int3472      16384  0

dmesg | grep int3472
<NO OUTPUT>

[me@box ~]$ cd /sys/bus/platform/devices/INT3472:00 && ls -l
total 0
lrwxrwxrwx 1 root root    0 16. Jan 16:29 driver -> ../../../../bus/platform/drivers/int3472-discrete
-rw-r--r-- 1 root root 4096 16. Jan 16:29 driver_override
lrwxrwxrwx 1 root root    0 16. Jan 16:29 firmware_node -> ../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:74/INT3472:00
-r--r--r-- 1 root root 4096 16. Jan 16:29 modalias
drwxr-xr-x 2 root root    0 16. Jan 16:29 power
lrwxrwxrwx 1 root root    0 16. Jan 17:29 subsystem -> ../../../../bus/platform
-rw-r--r-- 1 root root 4096 16. Jan 17:29 uevent

The sensor prints the following output:

[me@box ~]$ dmesg | grep i2c-INT33BE
[    2.041030] ov5693 i2c-INT33BE:00: ov5693_probe() called
[    2.041182] ov5693 i2c-INT33BE:00: supply avdd not found, using dummy regulator
[    2.041214] ov5693 i2c-INT33BE:00: supply dovdd not found, using dummy regulator
[    2.077541] ov5693 i2c-INT33BE:00: read from offset 0x300a error -121
[    2.083655] ov5693 i2c-INT33BE:00: sensor_id_high = 0x0
[    2.089142] ov5693 i2c-INT33BE:00: ov5693_detect err s_config.
[    2.096363] ov5693 i2c-INT33BE:00: sensor power-gating failed

Looks like it's not powered on and can't read the sensor id.

I noticed that the driver calls a _DSM which is not present in the SB2 dsdt (see here). Shouldn't the driver read the GPIOs from the _CRS section?

[djrscally]

Sorry slow replies; weekends are largely kid-filled. Did you resolve the indicator-led not being found, or was that due to the commenting out you did?

[me@box ~]$ cd /sys/bus/platform/devices/INT3472:00 && ls -l total 0 lrwxrwxrwx 1 root root 0 16. Jan 16:29 driver -> ../../../../bus/platform/drivers/int3472-discrete -rw-r--r-- 1 root root 4096 16. Jan 16:29 driver_override lrwxrwxrwx 1 root root 0 16. Jan 16:29 firmware_node -> ../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:74/INT3472:00 -r--r--r-- 1 root root 4096 16. Jan 16:29 modalias drwxr-xr-x 2 root root 0 16. Jan 16:29 power lrwxrwxrwx 1 root root 0 16. Jan 17:29 subsystem -> ../../../../bus/platform -rw-r--r-- 1 root root 4096 16. Jan 17:29 uevent

So yeah, intel-skl-int3472 is loading and registering the int3472-discrete driver which is binding to the device fine; so far so good.

[me@box ~]$ dmesg | grep i2c-INT33BE
[    2.041030] ov5693 i2c-INT33BE:00: ov5693_probe() called
[    2.041182] ov5693 i2c-INT33BE:00: supply avdd not found, using dummy regulator
[    2.041214] ov5693 i2c-INT33BE:00: supply dovdd not found, using dummy regulator
[    2.077541] ov5693 i2c-INT33BE:00: read from offset 0x300a error -121
[    2.083655] ov5693 i2c-INT33BE:00: sensor_id_high = 0x0
[    2.089142] ov5693 i2c-INT33BE:00: ov5693_detect err s_config.
[    2.096363] ov5693 i2c-INT33BE:00: sensor power-gating failed

Looks like it's not powered on and can't read the sensor id.

But yes, that analysis is correct. Can you add some debug prints for me perhaps:

diff --git a/drivers/platform/x86/intel_skl_int3472_discrete.c b/drivers/platform/x86/intel_skl_int3472_discrete.c
index ea7e57f3e3f0..0273057a74a2 100644
--- a/drivers/platform/x86/intel_skl_int3472_discrete.c
+++ b/drivers/platform/x86/intel_skl_int3472_discrete.c
@@ -60,6 +60,8 @@ static int skl_int3472_clk_enable(struct clk_hw *hw)
 {
    struct int3472_gpio_clock *clk = to_int3472_clk(hw);

+   pr_info("Called clock enable\n");
+
    gpiod_set_value(clk->gpio, 1);

    return 0;
@@ -174,6 +176,8 @@ static int int3472_map_gpio_to_sensor(struct int3472_device *int3472,
    if (ret)
        return -ENODEV;

+   pr_info("Assigning GPIO line %llu from %s to %s as %s\n", ares->data.gpio.pin_table[0], acpi_dev_name(adev), int3472->sensor_name, func);
+
    table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev),
                               ares->data.gpio.pin_table[0],
                               func, 0, polarity);
@@ -343,6 +347,8 @@ static int int3472_handle_gpio_resources(struct acpi_resource *ares,
        return ENODEV;
    }

+   pr_info("Mapping GPIO type 0x%02llx for %s\n", obj->integer.value, acpi_dev_name(int3472->adev));
+
    switch (obj->integer.value & 0xff) {
    case INT3472_GPIO_TYPE_RESET:
        ret = int3472_map_gpio_to_sensor(int3472, ares, "reset",

I noticed that the driver calls a _DSM which is not present in the SB2 dsdt (see here). Shouldn't the driver read the GPIOs from the _CRS section?

It is reading them from _CRS; int3472_parse_crs() runs the function int3472_handle_gpio_resources() against every resource found in _CRS, and that function calls the _DSM to get the type information. The _DSM you linked is the int3472_gpio_guid variable in intel_skl_int3472_discrete.c, but I typed the comment above it wrong - there should be an extra hyphen after a5c1. Thanks for spotting that, please don't ask why I was stupid and typed it instead of copy-pasting :)

According to that DSDT the only GPIO lines on your device are reset (not needed to power on), clock enable and indicator-led. So, if the device isn't turning on, the clock enable call musn't be happening for some reason.

[fabwu]

Sorry slow replies;

No worries I really appreciate your help :)

Here's the debugging output:

[  360.646766] Mapping GPIO type 0x100540c for INT3472:00
[  360.646840] Mapping GPIO type 0x1004d00 for INT3472:00
[  360.646908] Assigning GPIO line 77 from INT344B:00 to i2c-INT33BE:00 as reset
[  360.646931] Mapping GPIO type 0x100160d for INT3472:00
[  360.646975] Assigning GPIO line 22 from INT344B:00 to i2c-INT33BE:00 as indicator-led
[  360.648703] Mapping GPIO type 0x100530c for INT3472:01
[  360.648742] Mapping GPIO type 0x1004e00 for INT3472:01
[  360.648792] Assigning GPIO line 78 from INT344B:00 to i2c-INT347A:00 as reset
[  360.648814] Mapping GPIO type 0x100110d for INT3472:01
[  360.648860] Assigning GPIO line 17 from INT344B:00 to i2c-INT347A:00 as indicator-led
[  360.650306] Mapping GPIO type 0x100550c for INT3472:02
[  360.650340] Mapping GPIO type 0x1008200 for INT3472:02
[  360.650391] Assigning GPIO line 130 from INT344B:00 to i2c-INT347E:00 as reset

I can't see the Called clock enable though.

[djrscally]

Are you using the ipu3-cameras branch directly, or did you just apply the commits to your own? If the latter, did you definitely apply the ones adding clk functions to the driver: https://github.com/linux-surface/kernel/pull/78/commits/15833ac94dd9a9c222404d52adffa3c0cddeddf3?

Clock registration failures should throw errors in dmesg and the ov593 driver like so:

[    2.727615] ov5693 i2c-INT33BE:00: ov5693_probe() called
[    2.727620] ov5693 i2c-INT33BE:00: Error getting clock

[fabwu]

Sorry it's not my day... Called clock enable is actually showing up in the logs.

If I remember correctly "reset" and "enable-clk" had to be enable in order to make the sensor work (see this comment. Could that be the problem?

Edit: Should we switch to IRC so we don't spam this PR?

[djrscally]

Sure, I'm on ##linux-surface. Bedtime routine calls though, so might be non responsive for the next hour.

Based on that comment, you're right. Can you try this:

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index bb29e178cc99..e54d6b464fda 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1035,6 +1035,8 @@ static int __power_up(struct v4l2_subdev *sd)
    if (ret)
        goto fail_power;

+   gpiod_set_value_cansleep(sensor->reset, 0);
+
    __cci_delay(up_delay);

    return 0;
@@ -1053,6 +1055,8 @@ static int power_down(struct v4l2_subdev *sd)

    dev->focus = OV5693_INVALID_CONFIG;

+   gpiod_set_value_cansleep(sensor->reset, 1);
+
    clk_disable_unprepare(dev->clk);

    if (dev->indicator_led)

Note that the polarity of the reset pin is ACTIVE_LOW, so the 0/1 settings are the right way round.

This raises an annoying issue, which is that this is not necessary on my Surface Go 2, which means that that line is not controlling XSHUTDOWN on that platform. Boo! However, not the first deviation from the documented stuff I've seen, on my other laptop the Reset pin is actually completely disconnected and the pin that should be for power down is the reset line instead.

[fabwu]

I had no luck with the reset GPIO so I'm running out of ideas...

@qzed Maybe you wanna give it a try on your SB2?

[djrscally]

@fabwu - sorry, turned up on IRC eventually.

It really has to be the reset GPIO, since we saw that the clock was enabled properly and that comment mentions both pins need to be enabled. The datasheet for the ov5693 definitely calls it out as active low...but idk, maybe try setting gpiod_set_value_cansleep(sensor->reset, 1) in __power_up() instead of 0 (and of course gpiod_set_value_cansleep(dev->reset, 0) in power_down() - also I just noticed that that last diff should read dev->reset not sensor->reset in power_down(), sorry.);. We've not seen actual schematics afaik, so apart from the enumeration the Intel guys gave me we don't actually know how it's wired, and we do know that they're not always done to spec.

[fabwu]

I must have been really tired yesterday but today I had more luck. I tried your first GPIO patch again and oh wonder it worked!

I would suggest that we handle this quirks in the ov5693 module as it is specific to this sensor. If that is ok for you I prepare a patch to detect my platform (Is there a common way to do that?) and enable the reset GPIO only on the SB2.

Another thing is that I can't cleanly unload the modules. Does unloading the modules (ov5693, intel_skl, ipu3-cio2) works fine on your side and if so in which order do you unload?

[djrscally]

EDIT: Fleshed out a bit now I'm kidfree...

I must have been really tired yesterday but today I had more luck. I tried your first GPIO patch again and oh wonder it worked!

Nice!

I would suggest that we handle this quirks in the ov5693 module as it is specific to this sensor. If that is ok for you I prepare a patch to detect my platform (Is there a common way to do that?) and enable the reset GPIO only on the SB2.

I've been handling them by calling the _DSM function that gets the module identifier; see drivers/platform/x86/intel_skl_int3472_discrete.c. Some suggestions to switch to DMI though last time I posted the code, but there wasn't a firm consensus what was best. In any case though, the quirk in this case is in the Go2, not the SB2. The OV5693 does have a reset line on the sensor, and that's what this reset pin is supposed to connect to, so that code should be in the driver. I need to add a quirk to the int3472-discrete driver code that stops it adding a reset pin for the Go2 since it's not actually there, and I guess make the ov5693 driver use gpiod_get_optional() instead.

So yeah, we just need to add those calls to this branch, which I've done (badly, by screwing the first commit up, sorry...). I've actually been working on adding runtime pm to the ov5693 sensor and I've done the reset handling there too, so further improvements in a couple days there.

Another thing is that I can't cleanly unload the modules. Does unloading the modules (ov5693, intel_skl, ipu3-cio2) works fine on your side and if so in which order do you unload?

I'll test unloading intel_skl_int3472 in a couple hours...I always forget to try that! ipu3-cio2 and ov5693 unloading definitely worked. OK, unloading should go in order ipu3-cio2 -> sensor driver -> intel_skl_int3472. Actually, unloading ov5693 doesn't work, it's the ov2680 driver on my miix that unloads cleanly.

[fabwu]

@djrscally It seems the ov5693 on my SB2 doesn't use a regulator so I removed all the regulator calls from the driver and now unloading works partially. I can unload/reload ipu3-cio2 and ov5693 without any errors but unloading intel_skl and then reloading the ov5693 throws a NullPointerException but I don't know if we should investigate this further as unloading the intel_skl shouldn't be necessary.

Could you check if your ov5693 works without the regulator:

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 3e54d98b7..677a175df 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -33,7 +33,6 @@
 #include <media/v4l2-device.h>
 #include <linux/io.h>
 #include <linux/acpi.h>
-#include <linux/regulator/consumer.h>

 #include "ov5693.h"
 #include "ad5823.h"
@@ -1031,10 +1030,6 @@ static int __power_up(struct v4l2_subdev *sd)
    if (sensor->indicator_led)
        gpiod_set_value_cansleep(sensor->indicator_led, 1);

-   ret = regulator_bulk_enable(OV5693_NUM_SUPPLIES, sensor->supplies);
-   if (ret)
-       goto fail_power;
-
    gpiod_set_value_cansleep(sensor->reset, 0);

    __cci_delay(up_delay);
@@ -1062,7 +1057,7 @@ static int power_down(struct v4l2_subdev *sd)
    if (dev->indicator_led)
        gpiod_set_value_cansleep(dev->indicator_led, 0);

-   return regulator_bulk_disable(OV5693_NUM_SUPPLIES, dev->supplies);
+   return 0;
 }

 static int power_up(struct v4l2_subdev *sd)
@@ -1510,16 +1505,12 @@ static int ov5693_remove(struct i2c_client *client)
 {
    struct v4l2_subdev *sd = i2c_get_clientdata(client);
    struct ov5693_device *ov5693 = to_ov5693_sensor(sd);
-   unsigned int i = OV5693_NUM_SUPPLIES;

    dev_info(&client->dev, "%s...\n", __func__);

    gpiod_put(ov5693->reset);
    gpiod_put(ov5693->indicator_led);

-   while (i--)
-       regulator_put(ov5693->supplies[i].consumer);
-
    v4l2_async_unregister_subdev(sd);

    media_entity_cleanup(&ov5693->sd.entity);
@@ -1590,18 +1581,6 @@ static int ov5693_configure_gpios(struct ov5693_device *ov5693)
    return 0;
 }

-static int ov5693_get_regulators(struct ov5693_device *ov5693)
-{
-   unsigned int i;
-
-   for (i = 0; i < OV5693_NUM_SUPPLIES; i++)
-       ov5693->supplies[i].supply = ov5693_supply_names[i];
-
-   return regulator_bulk_get(&ov5693->client->dev,
-                      OV5693_NUM_SUPPLIES,
-                      ov5693->supplies);
-}
-
 static int ov5693_probe(struct i2c_client *client)
 {
    struct ov5693_device *ov5693;
@@ -1633,10 +1612,6 @@ static int ov5693_probe(struct i2c_client *client)
    if (ret)
        goto out_free;

-   ret = ov5693_get_regulators(ov5693);
-   if (ret)
-       goto out_put_reset;
-
    ret = ov5693_s_config(&ov5693->sd, client->irq);
    if (ret)
        goto out_put_reset;
diff --git a/drivers/media/i2c/ov5693.h b/drivers/media/i2c/ov5693.h
index ee468d05a..e231b92ac 100644
--- a/drivers/media/i2c/ov5693.h
+++ b/drivers/media/i2c/ov5693.h
@@ -254,7 +254,6 @@ struct ov5693_device {

    struct gpio_desc *reset;
    struct gpio_desc *indicator_led;
-   struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];

    struct camera_sensor_platform_data *platform_data;
    ktime_t timestamp_t_focus_abs;

[djrscally]

@fabwu

@djrscally It seems the ov5693 on my SB2 doesn't use a regulator so I removed all the regulator calls from the driver and now unloading works partially.

That is particularly weird, because they should only be acting on dummy regulators since you don't actually have any...

I can unload/reload ipu3-cio2 and ov5693 without any errors but unloading intel_skl and then reloading the ov5693 throws a NullPointerException but I don't know if we should investigate this further as unloading the intel_skl shouldn't be necessary.

No, not really. I actually posted the series to the list yesterday, and got a suggestion that's going to change drastically how the regulator stuff works, so it probably isn't worth it unless it still occurs after those changes.

Could you check if your ov5693 works without the regulator:

On my platform, one of the GPIO lines in the INT3472 is actually a regulator, so the driver is using actually fetching a regulator I registered and enabling/disabling via that. So it won't work

[fabwu]

I get a message that they are registered as dummy regulator but unloading doesn't work. Anyway, I'll wait for the new regulator code and work with the removed regulators for now.

No, not really. I actually posted the series to the list yesterday, and got a suggestion that's going to change drastically how the regulator stuff works, so it probably isn't worth it unless it still occurs after those changes.

v99 of the patch ^^ I really appreciate your persistence and work it's really not easy to get all of this working.

btw. on which list is this patch? linux-media?

In the meantime I try to read the orientation/rotation with v4l2_fwnode_device_parse(). I'll keep you posted.

[djrscally]

I get a message that they are registered as dummy regulator but unloading doesn't work. Anyway, I'll wait for the new regulator code and work with the removed regulators for now.

Sweet, ok. It's weird; unloading works on my Miix for the front cam, and that uses the dummy regulators too (no 0x0b pin in the INT3472 dev for that one).

No, not really. I actually posted the series to the list yesterday, and got a suggestion that's going to change drastically how the regulator stuff works, so it probably isn't worth it unless it still occurs after those changes.

v99 of the patch ^^

Heh I lost count honestly, I'm on v440 of the kernel build though :P Although it's only a couple modules each time of course!

I really appreciate your persistence and work it's really not easy to get all of this working.

Hey my pleasure, it's been really cool experience, I'm having fun.

btw. on which list is this patch? linux-media?

This set didn't go to media; none of it actually touches their areas. It goes to a few lists, but platform-driver-x86@vger.kernel.org is the one it's really targeted to. Here's the link: https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrscally@gmail.com/T/#m1b76617c83f76ffdce79afda2d495c43a9bb6377

I can CC you in future if you like. Same for you @qzed; I did mean to ask if you wanted to be on them, but it slipped my mind, sorry.

In the meantime I try to read the orientation/rotation with v4l2_fwnode_device_parse(). I'll keep you posted.

Awesome, thank you!

What shall we do with this PR in the meantime btw? Is it worth merging, or should we just scrap this idea and just merge the commit fixing the GPIO handling in ov5693 so the current version in the 5.10* branches works?

[qzed]

Sorry I still haven't gotten a good look at this yet. I want to finish up some work on touch processing, hopefully that's done in the next two days or so.

I can CC you in future if you like. Same for you @qzed; I did mean to ask if you wanted to be on them, but it slipped my mind, sorry.

Yeah, that'd be great.

[fabwu]

I can CC you in future if you like. Same for you @qzed; I did mean to ask if you wanted to be on them, but it slipped my mind, sorry.

CC would be great thanks!

What shall we do with this PR in the meantime btw?

The current version in 5.10 works so no need to just merge the GPIO fix. Not sure if we should merge this PR. If anyone wants to test the cameras he can still use the ipu3 branch.

[qzed]

Sorry again for the delay. I was finally able to test this.

Seems like I have the same problem with unloading the drivers. Here's a log:

dmesg

``` # sudo modprobe -r intel_skl_int3472 [ 1077.165325] clk_unregister: unregistering prepared clock: INT3472:00-clk # sudo modprobe -r ipu3_cio2 # sudo modprobe -r ipu3_imgu [ 1091.957795] ipu3-imgu 0000:00:05.0: wait cio gate idle timeout [ 1091.958213] BUG: unable to handle page fault for address: ffffa038c12c2064 [ 1091.958222] #PF: supervisor read access in kernel mode [ 1091.958226] #PF: error_code(0x0000) - not-present page [ 1091.958229] PGD 100000067 P4D 100000067 PUD 1001c6067 PMD 10e45b067 PTE 0 [ 1091.958244] Oops: 0000 [#1] PREEMPT SMP PTI [ 1091.958252] CPU: 2 PID: 3463 Comm: modprobe Tainted: G C 5.10.0-1-surface-dev #1 [ 1091.958256] Hardware name: Microsoft Corporation Surface Book 2/Surface Book 2, BIOS 390.3279.768 06.15.2020 [ 1091.958271] RIP: 0010:imgu_css_irq_ack+0x59/0x150 [ipu3_imgu] [ 1091.958277] Code: 00 00 8b 85 08 48 00 00 44 8b 9d 08 49 00 00 48 69 d2 b0 04 00 00 41 89 c2 31 db 49 01 d5 8d 53 17 41 89 d8 49 0f a3 d2 73 25 <41> 8b 55 64 48 8d 94 15 00 80 00 00 44 8b 0a 41 8b 55 64 48 8d 94 [ 1091.958282] RSP: 0018:ffffa038e6b2bd78 EFLAGS: 00010087 [ 1091.958288] RAX: 00000000deadbeef RBX: 0000000000000000 RCX: 000000008015000e [ 1091.958292] RDX: 0000000000000017 RSI: ffff90a6ce410018 RDI: ffff90a6ce417868 [ 1091.958295] RBP: ffffa038c1400000 R08: 0000000000000000 R09: 0000000000000000 [ 1091.958299] R10: 00000000deadbeef R11: 00000000deadbeef R12: ffff90a6ce417868 [ 1091.958302] R13: ffffa038c12c2000 R14: 0000000000000000 R15: 00000000deadbeef [ 1091.958308] FS: 00007fd725608740(0000) GS:ffff90a827480000(0000) knlGS:0000000000000000 [ 1091.958312] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1091.958316] CR2: ffffa038c12c2064 CR3: 0000000184b00001 CR4: 00000000003706e0 [ 1091.958320] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1091.958323] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 1091.958326] Call Trace: [ 1091.958342] ? __synchronize_hardirq+0x83/0xb0 [ 1091.958355] imgu_isr+0x11/0x20 [ipu3_imgu] [ 1091.958361] free_irq+0x265/0x3b0 [ 1091.958371] release_nodes+0x1a5/0x1f0 [ 1091.958380] __device_release_driver+0x18a/0x230 [ 1091.958387] driver_detach+0xc9/0x110 [ 1091.958393] bus_remove_driver+0x58/0xd0 [ 1091.958403] pci_unregister_driver+0x3b/0x90 [ 1091.958412] __do_sys_delete_module+0x19e/0x300 [ 1091.958422] do_syscall_64+0x33/0x40 [ 1091.958429] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 1091.958435] RIP: 0033:0x7fd725735d5b [ 1091.958442] Code: 73 01 c3 48 8b 0d 15 11 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e5 10 0c 00 f7 d8 64 89 01 48 [ 1091.958446] RSP: 002b:00007ffd6aef3468 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0 [ 1091.958452] RAX: ffffffffffffffda RBX: 000055cfd0adeff0 RCX: 00007fd725735d5b [ 1091.958456] RDX: 0000000000000000 RSI: 0000000000000800 RDI: 000055cfd0adf058 [ 1091.958459] RBP: 000055cfd0adeff0 R08: 0000000000000000 R09: 0000000000000000 [ 1091.958462] R10: 00007fd7257a9ac0 R11: 0000000000000206 R12: 000055cfd0adf058 [ 1091.958466] R13: 0000000000000000 R14: 0000000000000000 R15: 000055cfd0ae6530 [ 1091.958472] Modules linked in: cmac algif_hash algif_skcipher af_alg bnep btusb btrtl btbcm btintel bluetooth ecdh_generic ecc crc16 snd_usb_audio nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib snd_usbmidi_lib snd_rawmidi snd_seq_device nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct uas usb_storage nft_chain_nat nf_tables hid_multitouch ip6table_nat ip6table_mangle ip6table_raw usbhid ip6table_security iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 iptable_mangle iptable_raw iptable_security cdc_ether usbnet r8152 nfnetlink mii ip6table_filter ip6_tables iptable_filter snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio hid_sensor_gyro_3d hid_sensor_accel_3d hid_sensor_als hid_sensor_rotation hid_sensor_trigger industrialio_triggered_buffer kfifo_buf hid_sensor_iio_common snd_soc_skl industrialio joydev hid_sensor_hub mousedev snd_soc_sst_ipc snd_soc_sst_dsp cros_ec_ishtp snd_hda_ext_core cros_ec surface_perfmode intel_ishtp_loader [ 1091.958602] snd_soc_acpi_intel_match snd_soc_acpi uinput intel_ishtp_hid snd_hda_intel x86_pkg_temp_thermal snd_intel_dspcfg intel_powerclamp soundwire_intel soundwire_generic_allocation soundwire_cadence mac_hid 8250_dw kvm_intel snd_hda_codec iTCO_wdt intel_pmc_bxt kvm iTCO_vendor_support mei_hdcp ipts intel_rapl_msr snd_hda_core snd_hwdep irqbypass soundwire_bus gpio_keys crct10dif_pclmul crc32_pclmul ghash_clmulni_intel surface_gpe snd_soc_core mwifiex_pcie aesni_intel snd_compress crypto_simd mwifiex cryptd glue_helper ac97_bus rapl snd_pcm_dmaengine nls_iso8859_1 intel_cstate vfat snd_pcm intel_uncore fat snd_timer pcspkr cfg80211 snd i2c_i801 soundcore i915 i2c_smbus rfkill ipu3_imgu(C-) videobuf2_dma_sg videobuf2_memops ov5693 videobuf2_v4l2 mei_me v4l2_fwnode i2c_algo_bit videobuf2_common intel_lpss_pci intel_lpss mei drm_kms_helper idma64 intel_pch_thermal videodev cec intel_gtt processor_thermal_device syscopyarea intel_rapl_common intel_ish_ipc sysfillrect [ 1091.958737] intel_xhci_usb_role_switch sysimgblt tpm_crb roles intel_ishtp fb_sys_fops intel_soc_dts_iosf mc surface_aggregator_registry surface_dtx surface_acpi_notify tpm_tis tpm_tis_core surfacepro3_button tpm surface_aggregator int3403_thermal video rng_core surface_hotplug dptf_power soc_button_array int3400_thermal acpi_pad acpi_tad int340x_thermal_zone acpi_thermal_rel drm coretemp sg fuse crypto_user agpgart bpf_preload ip_tables x_tables btrfs blake2b_generic libcrc32c crc32c_generic xor raid6_pq crc32c_intel xhci_pci xhci_pci_renesas [last unloaded: ipu3_cio2] [ 1091.958819] CR2: ffffa038c12c2064 [ 1091.958825] ---[ end trace 6ae2f6b982912ad0 ]--- [ 1092.013409] RIP: 0010:imgu_css_irq_ack+0x59/0x150 [ipu3_imgu] [ 1092.013410] Code: 00 00 8b 85 08 48 00 00 44 8b 9d 08 49 00 00 48 69 d2 b0 04 00 00 41 89 c2 31 db 49 01 d5 8d 53 17 41 89 d8 49 0f a3 d2 73 25 <41> 8b 55 64 48 8d 94 15 00 80 00 00 44 8b 0a 41 8b 55 64 48 8d 94 [ 1092.013411] RSP: 0018:ffffa038e6b2bd78 EFLAGS: 00010087 [ 1092.013413] RAX: 00000000deadbeef RBX: 0000000000000000 RCX: 000000008015000e [ 1092.013414] RDX: 0000000000000017 RSI: ffff90a6ce410018 RDI: ffff90a6ce417868 [ 1092.013415] RBP: ffffa038c1400000 R08: 0000000000000000 R09: 0000000000000000 [ 1092.013416] R10: 00000000deadbeef R11: 00000000deadbeef R12: ffff90a6ce417868 [ 1092.013416] R13: ffffa038c12c2000 R14: 0000000000000000 R15: 00000000deadbeef [ 1092.013418] FS: 00007fd725608740(0000) GS:ffff90a827480000(0000) knlGS:0000000000000000 [ 1092.013419] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1092.013420] CR2: ffffa038c12c2064 CR3: 0000000184b00001 CR4: 00000000003706e0 [ 1092.013420] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 1092.013421] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 ```

Apart from that, everything works (with the latest version of this PR).

What shall we do with this PR in the meantime btw? Is it worth merging, or should we just scrap this idea and just merge the commit fixing the GPIO handling in ov5693 so the current version in the 5.10* branches works?

At the very least this should fix https://github.com/linux-surface/linux-surface/issues/363, right? I'm in favor of merging this despite the current issue with unloading, pretty sure not many will attempt to unload those modules. Also this gives it a bit more coverage to test and see if things are breaking (not everyone can/wants to build their own kernel).

I'd place the ultimate decision of what to merge and what not with you two though, I'm not that familiar with the current status. So whatever you think is best.

[fabwu]

I wasn't aware of https://github.com/linux-surface/linux-surface/issues/363 so in this case +1 for merging.

[qzed]

So should I merge parts of this? If so, what do you consider ready? Up to (including) bc688ac?

[djrscally]

Yes I'd say so then; I can raise a new PR to add the clk_get_rate() support.

[qzed]

I've merged this manually in dcf274e.