search

linux-surface / linux-surface

Linux Kernel for Surface Devices
4.53k stars 202 forks source link

Camera support #91

Closed jrevillard closed 3 months ago

jrevillard commented 4 years ago

I see that there is some activity here: https://github.com/jakeday/linux-surface/issues/145

I think it's better to trace it here isn't it ?

Best, Jerome

Maintainer note: There is a BountySource bounty available for this issue. If you'd like to see a camera driver consider donating to incentivise the work.

jhand2 commented 3 years ago

Alright folks, I've been doing more work on the SB2 drivers, but I'm a bit stuck. I have pushed all my current code here: https://github.com/jhand2/surface-camera. The README has some more information, and I have tried to add a decently descriptive commit message to each commit.

I have also opened a couple issues on that repo about what I think is currently going wrong. I would love it if anyone wants to take a look at what I have so far and offer any thoughts or maybe point out something I'm doing wrong.

Over the next week or two I will try to improve documentation and explain more of what I've done to make it easier for folks to help. Feel free to open an issue or reach out with any questions. Thanks!

kitakar5525 commented 3 years ago

Hi, I'm trying to get ov7251 working first: https://github.com/kitakar5525/surface-ipu3-cameras

Current status is, thanks to jhand2 for GPIO part, I can load ov7251 without error with DSDT overriding:

kern  :warn  : [   44.204435] ov7251 i2c-INT347E:00: supply vdddo not found, using dummy regulator
kern  :warn  : [   44.204478] ov7251 i2c-INT347E:00: supply vddd not found, using dummy regulator
kern  :warn  : [   44.204489] ov7251 i2c-INT347E:00: supply vdda not found, using dummy regulator
kern  :info  : [   44.204542] ov7251 i2c-INT347E:00: Dependent platform device found: INT3472:02
kern  :info  : [   44.209302] ov7251 i2c-INT347E:00: OV7251 revision 7 (1F) detected at address 0x60

However, the driver doesn't get listed by libcamera:

$ sudo cam -l
[0:37:08.337418547] [7527]  INFO Camera camera_manager.cpp:283 libcamera v0.0.0+1478-1023107b
Available cameras:

Not sure what's missing... Maybe DSDT mod isn't working well. Here is what I did for DSDT: https://github.com/kitakar5525/surface-ipu3-cameras/commits/master/dsdt-mods/SB1

kitakar5525 commented 3 years ago

Note: ov8856 supports both DT and ACPI at the same time

ACPI support is the first, then later added DT support by commit https://github.com/torvalds/linux/commit/0c2c7a1e0d69221b9d489bfd8cf53262d6f82446 ("media: ov8856: Add devicetree support"). This is helpful to add ACPI support into DT-only drivers. According to the commit, it seems to be not so hard. So, I did the same thing to ov7251.

kitakar5525 commented 3 years ago

By the way, sensors used are a little bit different by devices. From linux-surface's acpidumps repo and Surface firmware download files:

SP4 SP5 SP6 SP7 SB1 SB2 SB3

CAMF INT33BE OV5693 CAMR INT347A OV8865 CAM3 INT347E OV7251 # IR cam

S3

CAM6 APTA0330 AR0330 # Front cam CAM1 OVTI8835 OV8835 # Rear cam

SL1 SL2

CAMF OVTI9734 OV9734 CAM3 INT347E OV7251 # IR cam

SGO (and SGO2?)

ICAM INT33A3 (no sensor name in dsdt) CAM0 INT3471 IMX135 CAM1 INT3474 OV2740 (These are from DSDT. Downloaded firmware file contains OV5693, OV8865, and OV7251 instead. Why?)

and I noticed that ov2740 (ACPI ID INT3474) used on Surface Go has been added on 5.8-rc1: https://github.com/torvalds/linux/commit/866edc895171f1256aad3e81dce193447955c202 ("media: i2c: Add ov2740 image sensor driver") with ACPI INT3474 support. DSDT says PLD_Panel = "FRONT",

EDIT

It seems that at least SGO1 uses the same sensors as SP/SB series: https://github.com/linux-surface/linux-surface/issues/91#issuecomment-653853420 (Sensors used on SGO/SGO2 could vary from device to device? I don't really think so, but I can't deny it for now)

kitakar5525 commented 3 years ago

@jhand2 can you add support for ov8865 and ov7251 (and the other sensors for other devices in the future?) into your surface_camera platform driver ?

jhand2 commented 3 years ago

@kitakar5525 thanks for looking into the other drivers! yes, I agree adding support for the other sensors to the surface_camera platform driver is the right thing to do. The reason I wrote that driver is that it removes the need to patch the dsdt, you can instead represent the necessary portion of the device tree entirely in software. I’ll add the other drivers when I get a chance

I’ve had to take a bit of a break as things have gotten a bit hectic in my day job. Hopefully I’ll be able to resume working on the cameras in July.

kitakar5525 commented 3 years ago

OK, thanks!

kitakar5525 commented 3 years ago 
$ sudo cam -l
[0:37:08.337418547] [7527]  INFO Camera camera_manager.cpp:283 libcamera v0.0.0+1478-1023107b
Available cameras:

Not sure what's missing... Maybe DSDT mod isn't working well. Here is what I did for DSDT: https://github.com/kitakar5525/surface-ipu3-cameras/commits/master/dsdt-mods/SB1

Connecting CIO2 to multiple sensors on the same I2C bus seems to break all connections between CIO2 and the sensors (including sensors on other I2C buses). In other words, I can't enable both CIO2-CAMR (ov8865) and CIO2-CAM3 (ov7251) connections at the same time, maybe because they're on the same bus (FIXME: bus? at least it's on same device according to DSDT: I2C3). (or it's possible that I'm simply doing wrong)

If I remove CIO2-CAMR connection, ov5693 (driver from jhand2) and ov7251 get detected by libcamera at the same time:

$ sudo cam -l
[0:01:47.649475906] [2977]  INFO Camera camera_manager.cpp:283 libcamera v0.0.0+1483-c3ed943c
[0:01:47.659353766] [2978]  INFO IPU3 ipu3.cpp:961 Registered Camera[0] "ov5693 7-0036" connected to CSI-2 receiver 0
[0:01:47.660017239] [2978] ERROR IPU3 ipu3.cpp:1455 Sensor ov7251 8-0060 has not format compatible with the IPU3
Available cameras:
1: ov5693 7-0036

However, as you see, ov7251 isn't available for libcamera, saying that incompatible format with IPU3. Regarding jhand2's ov5695, when I try to capture, I get the following message from libcamera:

$ sudo cam -c1 -C
[0:18:00.664685731] [5083]  INFO Camera camera_manager.cpp:283 libcamera v0.0.0+1483-c3ed943c
[0:18:00.676262497] [5084]  INFO IPU3 ipu3.cpp:961 Registered Camera[0] "ov5693 7-0036" connected to CSI-2 receiver 0
[0:18:00.676914177] [5084] ERROR IPU3 ipu3.cpp:1455 Sensor ov7251 8-0060 has not format compatible with the IPU3
Using camera ov5693 7-0036
[0:18:00.677515490] [5083]  INFO Camera camera.cpp:770 configuring streams: (0) 1280x720-0x3231564e
[0:18:00.677766013] [5084] ERROR MediaDevice media_device.cpp:802 /dev/media0[ipu3-imgu]: Failed to setup link: Invalid argument
Failed to configure camera
jhand2 commented 3 years ago

@kitakar5525 The settings I configured in the surface_camera driver are totally made up which is likely why they don't work. For example, I chose to say that the ov5693 is connected to CIO2 on port 0, but I have no clue if that is actually true. Same with data lanes, clock lanes, etc.

Another potential issue is that other ov drivers set the input clock speed, but in my testing, devm_get_clk fails, so I cant get/set the clock speed.

All of this stems from the fact that much of the hardware is undocumented. We have the ov5693 data sheet but it doesn't answer many of these questions.

qzed commented 3 years ago

For example, I chose to say that the ov5693 is connected to CIO2 on port 0, but I have no clue if that is actually true. Same with data lanes, clock lanes, etc.

Did you have a look at the Intel repo I linked somewhere above? Specifically https://github.com/intel/intel-camera-drivers/blob/e6392058d52c0d2777741f9d4dbe7c4de7a3951a/drivers/media/platform/intel-ipu4/intel-ipu4-acpi.c#L44-L73. Looks like the SSDB buffer contains all that info.

jhand2 commented 3 years ago

@qzed Ah I totally missed that link. Thanks for the pointer!

kitakar5525 commented 3 years ago

For example, I chose to say that the ov5693 is connected to CIO2 on port 0, but I have no clue if that is actually true. Same with data lanes, clock lanes, etc.

Did you have a look at the Intel repo I linked somewhere above? Specifically https://github.com/intel/intel-camera-drivers/blob/e6392058d52c0d2777741f9d4dbe7c4de7a3951a/drivers/media/platform/intel-ipu4/intel-ipu4-acpi.c#L44-L73. Looks like the SSDB buffer contains all that info.

I also tried get_acpi_ssdb_sensor_data() Here is the result on SB1:

ov5693 i2c-INT33BE:00: sensor acpi data: link 1, lanes 2, mclk 0, flash 2
ov5693 i2c-INT33BE:00: sensor_data.romtype: 1
ov5693 i2c-INT33BE:00: sensor_data.vcmtype: 0
ov5693 i2c-INT33BE:00: sensor_data.privacyled: 1
ov5693 i2c-INT33BE:00: sensor_data.degree: 1
ov5693 i2c-INT33BE:00: sensor_data.mipilinkdefined: 1
ov5693 i2c-INT33BE:00: sensor_data.mclkspeed: 19200000
ov5693 i2c-INT33BE:00: sensor_data.controllogicid: 1

ov8865 i2c-INT347A:00: sensor acpi data: link 0, lanes 4, mclk 0, flash 2
ov8865 i2c-INT347A:00: sensor_data.romtype: 1
ov8865 i2c-INT347A:00: sensor_data.vcmtype: 4
ov8865 i2c-INT347A:00: sensor_data.privacyled: 1
ov8865 i2c-INT347A:00: sensor_data.degree: 1
ov8865 i2c-INT347A:00: sensor_data.mipilinkdefined: 1
ov8865 i2c-INT347A:00: sensor_data.mclkspeed: 19200000
ov8865 i2c-INT347A:00: sensor_data.controllogicid: 0

ov7251 i2c-INT347E:00: sensor acpi data: link 2, lanes 1, mclk 0, flash 2
ov7251 i2c-INT347E:00: sensor_data.romtype: 0
ov7251 i2c-INT347E:00: sensor_data.vcmtype: 0
ov7251 i2c-INT347E:00: sensor_data.privacyled: 0
ov7251 i2c-INT347E:00: sensor_data.degree: 0
ov7251 i2c-INT347E:00: sensor_data.mipilinkdefined: 1
ov7251 i2c-INT347E:00: sensor_data.mclkspeed: 19200000
ov7251 i2c-INT347E:00: sensor_data.controllogicid: 2

The link number seems to be the port number of the CIO2 device that the sensor connects to.

Here is the full code: https://github.com/kitakar5525/surface-ipu3-cameras/tree/master/misc/get_acpi_ssdb_sensor_data

shinyquagsire23 commented 3 years ago

@kitakar5525 On the Surface Go in Win10 it has these details for the cameras,

IR Camera: OV7251, INT347E Front Camera: OV5693, INT33BE Rear Camera: OV8865, INT347A

I'm not sure what the deal is with the IMX135? Might try out things and see if I can get it going. Don't particularly trust the DSDT personally given some of the power hacks that have been needed for the touchscreen for whatever reason.

kitakar5525 commented 3 years ago

@shinyquagsire23

Yeah, I looked into SGO/SGO2 DSDT again and all of the ACPI ID are written there. So, can you post the output of ls -lh /sys/bus/i2c/devices ? This will show the sensors that actually exist.

shinyquagsire23 commented 3 years ago

@kitakar5525 fair warning, this is brunch ChromeOS so the kernel might be a bit dated,

chronos@localhost / $ ls -lh /sys/bus/i2c/devices
total 0
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-0 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-0
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-1 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-1
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-2 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-2
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-3 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card1/card1-eDP-1/i2c-3
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-4 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card1/card1-DP-1/i2c-4
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-5 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card1/card1-DP-2/i2c-5
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-6 -> ../../../devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-6
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-7 -> ../../../devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-7
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-8 -> ../../../devices/pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-9 -> ../../../devices/pci0000:00/0000:00:15.3/i2c_designware.3/i2c-9
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-ELAN9038:00 -> ../../../devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-7/i2c-ELAN9038:00
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-INT3472:05 -> ../../../devices/pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-INT3472:05
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-INT347A:00 -> ../../../devices/pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-INT347A:00
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-INT347E:00 -> ../../../devices/pci0000:00/0000:00:15.3/i2c_designware.3/i2c-9/i2c-INT347E:00
lrwxrwxrwx. 1 root root 0 Jul  5 00:29 i2c-NXP3001:00 -> ../../../devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-6/i2c-NXP3001:00
chronos@localhost / $ uname -a
Linux localhost 4.19.87-brunch-sebanc #1 SMP PREEMPT Sat Apr 4 09:07:43 CEST 2020 x86_64 Intel(R) Pentium(R) CPU 4415Y @ 1.60GHz GenuineIntel GNU/Linux

Seems like the front camera isn't matching the Win10 device info? And the NXP3001 i2c device is just the NFC chip they added with the Go.

EDIT: actually I guess maybe the front two cameras are just sharing the same bus? funky.

kitakar5525 commented 3 years ago

Hmm, There are INT3472 (PMIC) instead of INT33BE (ov5693) listed. Maybe INT33BE is listed at somewhere else instead: sudo find /sys -name "*INT33BE*"

Anyway, as you mentioned, it seems that SGO uses the same sensors as SP/SB series. I'll update my old comment.

shinyquagsire23 commented 3 years ago

hm, seems it's in there under PNP0A08 which is what Windows reported as well

chronos@localhost / $ sudo find /sys -name "*INT33BE*"
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT33BE:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT33BE:01
/sys/bus/acpi/devices/INT33BE:00
/sys/bus/acpi/devices/INT33BE:01
spoorun commented 3 years ago

BOUNTY for this bug now increased to $550!

https://www.bountysource.com/issues/88806607-camera-support

kitakar5525 commented 3 years ago

I'm currently completely stuck with the gpio-regulator and clk stuff that can be seen in ipu4-acpi and crlmodule driver...

By the way, Intel still maintains IPU4 (not ipu3) drivers and its dependent drivers (crlmodule, ipu4-acpi) in their repository up to v4.19 series: https://github.com/intel/linux-intel-lts/tree/4.19/base

The "CRL" in the "crlmodule" stands for "common/configurable register list"

and seems that this is a common driver for various sensors. Although the crlmodule currently doesn't support any of our sensors, I own another device (Acer Switch Alpha 12) that have IPU3 and ov5670 sensor. The ov5670 is supported by the crlmodule.

I'm curious if we can

kitakar5525 commented 3 years ago

Oh, old chromiumos commits have functionality that read SSDB and make a connection between sensors and cio2 for ipu3:

Also, I see some regulator/clock/gpio definitions, but unfortunately, those are never used. This is because some Chromebooks (soraka and nautilus) control PMIC via firmware:

So, I don't know how to get/configure regulators/clock for the PMIC yet.

By the way, some other Chromebooks (atlas and nocturne) even don't use PMIC tps68470 (or at least don't control it directly) but only use GPIO to set camera power sequence. This commit for nocturne is clear.

djrscally commented 3 years ago

@jhand @kitakar5525 following along closely as I'm trying to get my webcams on my surface knockoff (Lenovo Miix 510) working, which seems to be similar state. I followed your work getting the INT3472's struct device by evaluating the ACPI _DEPs to control the GPIO pins in my DSDT, but some SO questions led me to believe that it was better to let the INT3472's MFD driver enable those (to turn on the PMIC) and control the pins provided by the PMIC in the camera's driver itself:

    static struct gpiod_lookup_table ov2680_gpios = {
        .dev_id = "i2c-OVTI2680:00",
        .table = {
            GPIO_LOOKUP_IDX("tps68470-gpio", 7, "s_enable", 0, GPIO_ACTIVE_HIGH),
            GPIO_LOOKUP_IDX("tps68470-gpio", 8, "s_idle", 0, GPIO_ACTIVE_HIGH),
            GPIO_LOOKUP_IDX("tps68470-gpio", 9, "s_resetn", 0, GPIO_ACTIVE_HIGH),
            { },
        },
    };

    gpiod_add_lookup_table(&ov2680_gpios);

I'm not completely sure which pins from the PMIC are wired to what pins on the sensor though of course; have you figured out a way to diagnose that? For example a register that resets when XSHUTDN puts it into standby mode or anything? As I've not so far managed to confirm that that definitely works.

Re. regulators, my guess at the moment is we need to add a tps68470-regulator driver to the mfd driver, something akin to the tps65086-regulator driver controlled by that module's mfd driver. But I don't really know what I'm talking about, so that might be hokum :P

Clocks, maybe likewise? See Maxim 77686/802 drivers:

MFD Regulators Clock

kitakar5525 commented 3 years ago

it was better to let the INT3472's MFD driver enable those (to turn on the PMIC) and control the pins provided by the PMIC in the camera's driver itself:

If it's doable, it's definitely better, but simply I don't know how to achieve this :)

Another problem I noticed is that, the PMIC MFD driver doesn't probe our PMIC. If it's probed, this line from mfd/tps68470.c driver should appear, but it doesn't. When I look at DSDT of SB1 (Surface Book 1), CAMR (Rear cam) has two I2cSerialBusV2 definitions in _CRS. I don't know if a single I2C device has the multiple definitions is normal.

If I try moving the second I2cSerialBusV2 definition to one of PMIC scope (in my case, SKC2), the linux kernel enumerates i2c device of the PMIC under /sys/bus/i2c/devices/ as i2c-INT3472:02 and the MFD driver probes the device:

tps68470 i2c-INT3472:02: TPS68470 REVID: 0x0

I'm not completely sure which pins from the PMIC are wired to what pins on the sensor though of course; have you figured out a way to diagnose that?

I don't have any idea for now.

Re. regulators, my guess at the moment is we need to add a tps68470-regulator driver to the mfd driver,

Intel has both regulator and clk drivers for tps68470 in their v4.14 branch:

However, it seems that those are never used, I don't see any reference in Kconfig, Makefile, and MFD driver.

djrscally commented 3 years ago

If it's doable, it's definitely better, but simply I don't know how to achieve this :)

I think the code I highlighted is working...at least no part of it throws an error and once I've done that I can get valid pointers to gpio_descs using:

/* ov2680 is a struct ov2680_device containing, amongst other things... */
struct ov2680_device {
    gpio_desc            *s_enable;
    struct i2c_device    *client;
}; 

ov2680->s_idle = gpiod_get_index(&ov2680->client->dev, "s_idle", 0, GPIOD_OUT_HIGH);

I just haven't figured out a way to confirm it yet.

Another problem I noticed is that, the PMIC MFD driver don't probe our PMIC. If it's probed, this line from mfd/tps68470.c driver should appear, but it doesn't. When I look at DSDT of SB1 (Surface Book 1), CAMR (Rear cam) has two I2cSerialBusV2 definitions in _CRS. I don't know if a single I2C device has the multiple definitions is normal.

In my case the INT3472 doesn't have an I2CSerialBus2, so I had to modify the PMIC driver to include an i2c_match_id and create the I2c device with echo INT3472 0x48 | sudo tee /sys/bus/i2c/devices/i2c-7/new_device - at that point the probe function is called and the GPIO driver it controls creates the new gpiochip1, which is what allows me to do the gpio lookup above. Somewhere in the documentation is a reference to how to deal with multiple I2cSerialBus2 in DSDT; it's basically a function to fetch the address by index...but I can't find it at the minute! If I do I'll link it. I have the same problem with one camera on my Miix510, but in my case the address of the first I2cSerialBus2 is right so at that point I stopped caring.

Intel has both regulator and clk drivers for tps68470 in their v4.14 branch:

https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/clk/clk-tps68470.c https://github.com/intel/linux-intel-lts/blob/4.14/base/drivers/regulator/tps68470-regulator.c

However, it seems that those are never used, I don't see any reference in Kconfig, Makefile, and MFD driver.

Oh, okaaaaay. Well maybe we can use them anyway; I'll try adding them to the MFD driver and see if I can make them load.

EDIT: Oh i see, they're not in the normal linux tree. OK, I'll add them in then.

Double EDIT: Some updating to do, my hopefulness in just dropping them into the 5.8 kernel source is dashed. I'll have a play and see where it gets me though.

kitakar5525 commented 3 years ago

I think the code I highlighted is working...at least no part of it throws an error and once I've done that I can get valid pointers to gpio_descs using:

I just haven't figured out a way to confirm it yet.

I saw your github repo. If the sensor id on the chip can be read, I think it means you have set up GPIO properly, at least regarding sensor power.

In my case the INT3472 doesn't have an I2CSerialBus2, so I had to modify the PMIC driver to include an i2c_match_id and create the I2c device with echo INT3472 0x48 | sudo tee /sys/bus/i2c/devices/i2c-7/new_device

(I forgot to add the link to DSDT, the DSDTs of Surface devices can be found here: https://github.com/linux-surface/acpidumps)

I've been misunderstanding. I thought 0x0c defined in the second I2cSerialBusV2 in CAMR is the PMIC address. After I saw what you have achieved, now I think the second I2cSerialBusV2 might be a VCM device (auto-focus stuff?).

djrscally commented 3 years ago

(I forgot to add the link to DSDT, the DSDTs of Surface devices can be found here: https://github.com/linux-surface/acpidumps)

I've been misunderstanding. I thought 0x0c defined in the second I2cSerialBusV2 in CAMR is the PMIC address. After I saw what you have achieved, now I think the second I2cSerialBusV2 might be a VCM device (auto-focus stuff?).

Yeah maybe, I never figured out what the 0x0c device is. My PMIC is definitely 0x48. I guess that would make sense for it to be a peripheral of the camera or something. Interesting that you guys have it too, I had begun to wonder if it was something unrelated. My DSDT table for that camera looks like this:

        Device (CAM0)
        {
            Name (_ADR, Zero)  // _ADR: Address
            Name (_HID, "OVTI5648")  // _HID: Hardware ID
            Name (_CID, "OVTI5648")  // _CID: Compatible ID
            Name (_DDN, "OV5648-CRDD")  // _DDN: DOS Device Name
            Name (_UID, "0")  // _UID: Unique ID
            Name (_DEP, Package (0x02)  // _DEP: Dependencies
            {
                PMI0, 
                I2C2
            })
            Name (_PLD, Package (0x01)  // _PLD: Physical Location of Device
            {
                ToPLD (
                    PLD_Revision           = 0x2,
                    PLD_IgnoreColor        = 0x1,
                    PLD_Red                = 0x0,
                    PLD_Green              = 0x0,
                    PLD_Blue               = 0x0,
                    PLD_Width              = 0x0,
                    PLD_Height             = 0x0,
                    PLD_UserVisible        = 0x1,
                    PLD_Dock               = 0x0,
                    PLD_Lid                = 0x0,
                    PLD_Panel              = "BACK",
                    PLD_VerticalPosition   = "CENTER",
                    PLD_HorizontalPosition = "RIGHT",
                    PLD_Shape              = "VERTICALRECTANGLE",
                    PLD_GroupOrientation   = 0x0,
                    PLD_GroupToken         = 0x0,
                    PLD_GroupPosition      = 0x0,
                    PLD_Bay                = 0x0,
                    PLD_Ejectable          = 0x1,
                    PLD_EjectRequired      = 0x1,
                    PLD_CabinetNumber      = 0x0,
                    PLD_CardCageNumber     = 0x0,
                    PLD_Reference          = 0x0,
                    PLD_Rotation           = 0x0,
                    PLD_Order              = 0x0,
                    PLD_VerticalOffset     = 0xFFFF,
                    PLD_HorizontalOffset   = 0xFFFF)

            })
            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (SBUF, ResourceTemplate ()
                {
                    I2cSerialBusV2 (0x0036, ControllerInitiated, 0x00061A80,
                        AddressingMode7Bit, "\\_SB.PCI0.I2C2",
                        0x00, ResourceConsumer, , Exclusive,
                        )
                    I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80,
                        AddressingMode7Bit, "\\_SB.PCI0.I2C2",
                        0x00, ResourceConsumer, , Exclusive,
                        )
                })
                Return (SBUF) /* \_SB_.PCI0.CAM0._CRS.SBUF */
            }

The camera is definitely at 0x36, so seems a bit too coincidental that both devices have a second address and it's the same address.

Regarding regulators, I bodged the regulator driver Intel made into something that would compile into 5.8, plus added it to the Makefile and Kconfig and also added it to the MFD driver's mfd cell. Patch here:

https://gist.github.com/djrscally/a195b1bf8d22779462ba72d01e55a91e

With that applied (note, I had to compile it into the kernel and reboot, it wouldn't insmod for me for some reason), I get a new regulator with 7 lanes in /sys/bus/platform/devices called tps68470-regulator. When I set the supply names in the camera driver to and add them with the function below...

static const char * const ov2680_supply_names[] = {
    "dovdd",        /* Digital I/O Power */
    "avdd",         /* Analog Power */
    "dvdd",         /* Digital Core Power */
    "CORE",
    "ANA",
    "VCM",
    "VIO",
    "VSIO",
    "AUX1",
    "AUX2",
};

static int ov2680_configure_regulators(struct ov2680_device *ov2680)
/*
 * Just get the power regulators.
*/
{
    int ret;
    int i;

    /* configure and enable regulators */
    for (i = 0; i < OV2680_NUM_SUPPLIES; i++) {
        ov2680->supplies[i].supply = ov2680_supply_names[i];
    }

    ret = devm_regulator_bulk_get(&ov2680->client->dev, OV2680_NUM_SUPPLIES, ov2680->supplies);

    return ret;
}

/* just noticed I need some error checking there... */

Then on insmod of the driver, DMESG tells me:

[  229.191319] ov2680: loading out-of-tree module taints kernel.
[  229.191414] ov2680: module verification failed: signature and/or required key missing - tainting kernel
[  229.194036] ov2680 i2c-OVTI2680:00: supply dovdd not found, using dummy regulator
[  229.194084] ov2680 i2c-OVTI2680:00: supply avdd not found, using dummy regulator
[  229.194111] ov2680 i2c-OVTI2680:00: supply dvdd not found, using dummy regulator
[  229.196157] ov2680 i2c-OVTI2680:00: xvclk clock missing or invalid.

Which to me looks a lot like the new regulators are working, since it's not complaining about being unable to find them or reporting any other kind of error like with the three existing ones. It might be better to find a way to specifically assign them to the struct device for the sensor; not sure. It isn't necessary because devm_regulator_bulk_get tries a bunch of methods to find the regulators, the final fallback being "forget matching the device, just find this name" which seems to be working ok.

So, idk. Looks like it's working? Again, not really sure how to check for certain! Either way I'm going to see if I can replicate that with the clock driver.

EDIT: Clock driver needs more of a re-write, I think it needs to be using regmap like the regulator driver. I'll see if I can get that sorted after work this evening.

djrscally commented 3 years ago

Turns out it wasn't too bad, lunchtime was enough. Link to patch below, again I had to enable this in config and compile it with the kernel to make it work; not sure why.

https://gist.github.com/djrscally/17e03ae87e4cd0822037796e20427588

So with this patch applied too, the call to get the clock becomes:

    xvclk = devm_clk_get(&ov2680->client->dev, "tps68470-clk");

    if (IS_ERR(xvclk)) {
        dev_err(&client->dev, "xvclk clock missing or invalid.\n");
    }

Forgot to change the variable name, oops. But the ID parameter passed to devm_clk_get becomes "tps68470-clk". When insmoding my camera driver module I now see this in dmesg:

[   59.785628] tps68470 7-0048: TPS68470 REVID: 0x0
[   59.788949] tps68470_pmic_opregion tps68470_pmic_opregion: acpi handle is NULL
[   59.789331] i2c i2c-7: new_device: Instantiated device INT3472 at 0x48
[   59.811143] tps68470 7-0048: Registered tps68470-clk clk
[  146.190950] ov2680: loading out-of-tree module taints kernel.
[  146.191066] ov2680: module verification failed: signature and/or required key missing - tainting kernel
[  146.197067] ov2680 i2c-OVTI2680:00: supply dovdd not found, using dummy regulator
[  146.197107] ov2680 i2c-OVTI2680:00: supply avdd not found, using dummy regulator
[  146.197126] ov2680 i2c-OVTI2680:00: supply dvdd not found, using dummy regulator
[  146.199664] ov2680: reading from addr 0x10, reg 0x300a
[  146.199907] ov2680: reading from addr 0x10, reg 0x300b
[  146.200338] ov2680: Chip ID Fetched = 0x2680
[  146.200344] ov2680: reading from addr 0x10, reg 0x302a
[  146.200580] detect ov2680 success
[  146.200586] media entity initialised as ov2680 7-0010.
[  146.200590] v4l2 subdev registered as ov2680 7-0010.

Error messsage about the clock being missing or invalid is gone, and there's a debug entry showing that the clock got registered with the system ok.

So, yeah. My fairly shaky understanding of this suggests that the approach is right, and the code seems to work (needs some tidying though, and the Intel guy's attributions added back in). Unfortunately, I can't get my cameras to even show up as a device for the V4L2 infrastructure to talk to; I think you guys had gotten that part to work though, right?

kitakar5525 commented 3 years ago

Wow, looks great progress!

Unfortunately, I can't get my cameras to even show up as a device for the V4L2 infrastructure to talk to; I think you guys had gotten that part to work though, right?

To try the camera, first, you need to connect sensors to ipu3-cio2 device. There are two known working ways and one potential way:

  1. Overriding DSDT Here is what I did for my SB1: https://github.com/kitakar5525/surface-ipu3-cameras/tree/master/dsdt-mods
  2. Using surface_camera driver from jhand2: https://github.com/jhand2/surface-camera Contrary to the name of the driver (surface_), it should work on any PC by replacing 5693 and its HID INT33BE with the sensor you want to use.
  3. Porting old implementation chromiumos had before into current ipu3-cio2 driver (utilize SSDB): https://github.com/linux-surface/linux-surface/issues/91#issuecomment-674760233

By the way, there may be no easy way to check if the connection is working. Adding debug output to the ipu3-cio2 driver makes this easier:

0001-media-ipu3-cio2-cio2_parse_firmware-add-debug-print.patch

```diff From d3f71aceb2d5692f5bf48e5eeeb9d7e18775ca38 Mon Sep 17 00:00:00 2001 From: Tsuchiya Yuto Date: Thu, 9 Jul 2020 17:03:29 +0900 Subject: [PATCH 1/3] media: ipu3-cio2: cio2_parse_firmware(): add debug print Signed-off-by: Tsuchiya Yuto --- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 92f5eadf2c99f..7098cfad9417b 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1491,12 +1491,21 @@ static int cio2_parse_firmware(struct cio2_device *cio2) dev_fwnode(&cio2->pci_dev->dev), i, 0, FWNODE_GRAPH_ENDPOINT_NEXT); - if (!ep) + if (!ep) { + dev_info(&cio2->pci_dev->dev, + "%s(), endpoint not available for CIO2 port %d\n", + __func__, i); continue; + } ret = v4l2_fwnode_endpoint_parse(ep, &vep); - if (ret) + if (ret) { + dev_info(&cio2->pci_dev->dev, + "%s(), v4l2_fwnode_endpoint_parse() failed for CIO2 port %d " + ": (ret: %d)\n", + __func__, i, ret); goto err_parse; + } s_asd = kzalloc(sizeof(*s_asd), GFP_KERNEL); if (!s_asd) { @@ -1509,11 +1518,20 @@ static int cio2_parse_firmware(struct cio2_device *cio2) ret = v4l2_async_notifier_add_fwnode_remote_subdev( &cio2->notifier, ep, &s_asd->asd); - if (ret) + if (ret) { + dev_info(&cio2->pci_dev->dev, + "%s(), v4l2_async_notifier_add_fwnode_remote_subdev() failed " + "for CIO2 port %d : (ret: %d)\n", + __func__, i, ret); goto err_parse; + } fwnode_handle_put(ep); + dev_info(&cio2->pci_dev->dev, + "%s(), CIO2 port %d available: csi2.port: %d csi2.lanes: %d\n", + __func__, i, s_asd->csi2.port, s_asd->csi2.lanes); + continue; err_parse: -- 2.27.0 ```

After you made the connection working, you can try libcamera like this:

# list available cameras:
$ sudo cam -l
[0:47:53.098159118] [30331]  INFO Camera camera_manager.cpp:287 libcamera v0.0.0+1735-54ca4696
[0:47:53.114206191] [30332]  INFO IPU3 ipu3.cpp:813 Registered Camera[0] "\_SB_.PCI0.I2C3.CAM3" connected to CSI-2 receiver 2
Available cameras:
1: \_SB_.PCI0.I2C3.CAM3

# try to capture image
$ sudo cam -c1 -C
[0:48:39.388667589] [30970]  INFO Camera camera_manager.cpp:287 libcamera v0.0.0+1735-54ca4696
[0:48:39.400900530] [30971]  INFO IPU3 ipu3.cpp:813 Registered Camera[0] "\_SB_.PCI0.I2C3.CAM3" connected to CSI-2 receiver 2
Using camera \_SB_.PCI0.I2C3.CAM3
[0:48:39.402596540] [30970] ERROR IPU3 imgu.cpp:407 Failed to calculate pipe configuration
[0:48:39.402662689] [30970] ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions.
Failed to get default stream configuration
kitakar5525 commented 3 years ago

Ah, before that, you need to get connection info (port number / lane amount, etc.) from SSDB defined in each sensor device in DSDT. To do so, using jhand2's ssdb_dump utility is a good way: https://github.com/jhand2/surface-camera/tree/master/tools

djrscally commented 3 years ago

@kitakar5525 Ah, great. Thanks. I'll take a look and see; a bit wary of faffing with DSDT tables as I have even less idea what I'm doing there, but the surface_camera driver looks easy enough to adapt. I'll give it a try.

EDIT: I'm definitely doing something wrong :D

[0:50:37.156970191] [22327]  INFO Camera camera_manager.cpp:287 libcamera v0.0.0+1735-54ca4696
Available cameras:

Alas, I'll have to do some more debugging.

kitakar5525 commented 3 years ago

Hmm, I can make devm_regulator_bulk_get() work, but I can't make regulator_bulk_enable() work. How is your output of /sys/kernel/debug/regulator/supply_map? On my side, it shows nothing.

djrscally commented 3 years ago

@kitakar5525 

root@djrscally-MIIX-510-12ISK:/sys/kernel/debug/regulator# cat supply_map
root@djrscally-MIIX-510-12ISK:/sys/kernel/debug/regulator# cat regulator_summary regulator                      use open bypass  opmode voltage current     min     max
---------------------------------------------------------------------------------------
 regulator-dummy                  3    5      0 unknown     0mV     0mA     0mV     0mV 
    i2c-OVTI2680:00-dvdd          0                                 0mA     0mV     0mV
    i2c-OVTI2680:00-avdd          0                                 0mA     0mV     0mV
    i2c-OVTI2680:00-dovdd         0                                 0mA     0mV     0mV
    i2c-WCOM508C:00-vddl          1                                 0mA     0mV     0mV
    i2c-WCOM508C:00-vdd           1                                 0mA     0mV     0mV
 CORE                             0    1      0 unknown     0mV     0mA     0mV     0mV 
    i2c-OVTI2680:00-CORE          0                                 0mA     0mV     0mV
 ANA                              0    1      0 unknown     0mV     0mA     0mV     0mV 
    i2c-OVTI2680:00-ANA           0                                 0mA     0mV     0mV
 VCM                              0    1      0 unknown     0mV     0mA     0mV     0mV 
    i2c-OVTI2680:00-VCM           0                                 0mA     0mV     0mV
 VIO                              0    1      0 unknown     0mV     0mA     0mV     0mV 
    i2c-OVTI2680:00-VIO           0                                 0mA     0mV     0mV
 VSIO                             0    1      0 unknown     0mV     0mA     0mV     0mV 
    i2c-OVTI2680:00-VSIO          0                                 0mA     0mV     0mV
 AUX1                             0    1      0 unknown     0mV     0mA     0mV     0mV 
    i2c-OVTI2680:00-AUX1          0                                 0mA     0mV     0mV
 AUX2                             0    1      0 unknown     0mV     0mA     0mV     0mV 
    i2c-OVTI2680:00-AUX2          0                                 0mA     0mV     0mV

Also nothing. So maybe not working after all. What happens in the call to regulator_bulk_enable()?

jhand2 commented 3 years ago

Great investigation! I've been using the ov5670 driver as a reference, which is used as the example in the IPU3 documentation implying that it works with IPU3. It doesn't use the regulators, which maybe makes me thing it isn't necessary for IPU3-based camera systems. I don't have any hard reasoning why, just a hunch.

Btw, I am still following the conversation, but things have ramped up for me at work and I probably won't have much time to devote to working on this for a while.

kitakar5525 commented 3 years ago

regulator_bulk_enable() just returns -1:

kern  :err   : [  688.600999] Failed to enable CORE: -1
kern  :err   : [  688.601001] Failed to enable ANA: -1
kern  :err   : [  688.601002] Failed to enable VCM: -1
kern  :err   : [  688.601003] Failed to enable VIO: -1
kern  :err   : [  688.601003] Failed to enable VSIO: -1
kern  :err   : [  688.601004] Failed to enable AUX1: -1
kern  :err   : [  688.601004] Failed to enable AUX2: -1
kern  :err   : [  688.601007] ov5693 i2c-INT33BE:00: Failed to enable regulators
kern  :err   : [  688.601009] ov5693 i2c-INT33BE:00: could not power on ov5693
djrscally commented 3 years ago

@jhand2 no drama, thanks for your work so far!

@kitakar5525 Do you have an i2c device attached to your PMIC on 0x48 (or whatever address it's at for you; its got like 12 possibilities!)

kitakar5525 commented 3 years ago

@jhand2

Great investigation! I've been using the ov5670 driver as a reference, which is used as the example in the IPU3 documentation implying that it works with IPU3. It doesn't use the regulators, which maybe makes me thing it isn't necessary for IPU3-based camera systems. I don't have any hard reasoning why, just a hunch.

The ov5670 driver is written mainly for Chromebooks (or kind of linux-friendly devices, if any). Chromebooks handle power managements by rather firmware, not by kernel drivers. I see some references to regulators in ACPI ASL file for some Kabylake Chromebook

Btw, I am still following the conversation, but things have ramped up for me at work and I probably won't have much time to devote to working on this for a while.

no worries!

Do you have an i2c device attached to your PMIC on 0x48 (or whatever address it's at for you; its got like 12 possibilities!)

@djrscally Yeah, for now, I added I2cSerialBusV2 with address 0x000c to SKC1 (PMIC attached to CAMF(ov5693)). So, attached to 0x0c. Regarding clock, I can get/set_rate/enable the clock.

By the way, have you tried address 0x0c? I think 0x0c will also work for you anyway. I tried 0x21 (appears after setting gpios related to CAMR) and 0x70 (appears after setting gpios related to CAM3), and 0x0c (always appears) and all of them pass mfd driver probe.

djrscally commented 3 years ago

No dice for me at 0x0c:

[ 4622.383299] tps68470 7-000c: Failed to read revision register: -121
[ 4622.383323] tps68470 7-000c: TPS68470 Init Error -121
[ 4622.383461] tps68470: probe of 7-000c failed with error -121
[ 4622.383514] i2c i2c-7: new_device: Instantiated device INT3472 at 0x0c

I'll turn on the GPIO pins for the ov5648 camera; that showed up at least one more device for me when I tried it before. Maybe that's another one.

kitakar5525 commented 3 years ago

Hmm... I need to try another addresses. I haven't tried with 0x21 and 0x70 for enabling regulators yet. For the ov5693 camera I'm trying to use, turning on gpio pins doesn't show up any extra device.

djrscally commented 3 years ago

Yeah I think a different address is the best bet...the probe function for the MFD driver doesn't actually check anything sensible, it just tries to read a register (0xFF) and assumes all is well. So passing probe doesn't necessarily mean it's the right device I think and that might be why it fails trying to write to those regs.

It probably would be better to have that function check a bunch of registers and compare them against the power-on defaults in the datasheet, to be certain we're talking to the right device.

djrscally commented 3 years ago

Ah; I messed up in my driver code; the size of the supplies array was harcoded as 3, so it wasn't actually trying to enable those other regulators at all! I get failures enabling the new ones also. I'll work on fixing that today; need to read the regmap code a bit more thoroughly; with the CLK I wrote my own update functions but the regulator one is using some helper code that I'm not 100% sure about.

kitakar5525 commented 3 years ago

Yeah, I added tps68470_init to probe(). Now, regulator_bulk_enable() is working.

$ sudo cat /sys/kernel/debug/regulator/regulator_summary                
 regulator                      use open bypass  opmode voltage current     min     max
---------------------------------------------------------------------------------------
 regulator-dummy                  3    8      0 unknown     0mV     0mA     0mV     0mV 
    i2c-INT347E:00-vdda           0                                 0mA     0mV     0mV
    i2c-INT347E:00-vddd           0                                 0mA     0mV     0mV
    i2c-INT347E:00-vdddo          0                                 0mA     0mV     0mV
    i2c-INT33BE:00-dvdd           0                                 0mA     0mV     0mV
    i2c-INT33BE:00-avdd           0                                 0mA     0mV     0mV
    i2c-INT33BE:00-dovdd          0                                 0mA     0mV     0mV
    i2c-MSHW0030:00-vddl          1                                 0mA     0mV     0mV
    i2c-MSHW0030:00-vdd           1                                 0mA     0mV     0mV
 CORE                             0    1      0 unknown   900mV     0mA   900mV  1950mV 
    i2c-INT33BE:00-CORE           0                                 0mA     0mV     0mV
 ANA                              0    1      0 unknown   875mV     0mA   875mV  3100mV 
    i2c-INT33BE:00-ANA            0                                 0mA     0mV     0mV
 VCM                              0    1      0 unknown   875mV     0mA   875mV  3100mV 
    i2c-INT33BE:00-VCM            0                                 0mA     0mV     0mV
 VIO                              0    1      0 unknown   875mV     0mA   875mV  3100mV 
    i2c-INT33BE:00-VIO            0                                 0mA     0mV     0mV
 VSIO                             0    1      0 unknown   875mV     0mA   875mV  3100mV 
    i2c-INT33BE:00-VSIO           0                                 0mA     0mV     0mV
 AUX1                             0    1      0 unknown   875mV     0mA   875mV  3100mV 
    i2c-INT33BE:00-AUX1           0                                 0mA     0mV     0mV
 AUX2                             0    1      0 unknown   875mV     0mA   875mV  3100mV 
    i2c-INT33BE:00-AUX2           0                                 0mA     0mV     0mV
$ sudo cat /sys/kernel/debug/regulator/supply_map
(shows nothing)

supply_map still shows nothing. I'm not sure if it's ok.

However... libcamera still isn't working:

$ sudo cam -c1 -C
[4:14:20.308150127] [44824]  INFO Camera camera_manager.cpp:287 libcamera v0.0.0+1735-54ca4696
[4:14:20.319581637] [44825]  INFO IPU3 ipu3.cpp:813 Registered Camera[0] "\_SB_.PCI0.I2C2.CAMF" connected to CSI-2 receiver 1
[4:14:20.321336017] [44825]  INFO IPU3 ipu3.cpp:813 Registered Camera[1] "\_SB_.PCI0.I2C3.CAM3" connected to CSI-2 receiver 2
Using camera \_SB_.PCI0.I2C2.CAMF
[4:14:20.326167312] [44824]  INFO Camera camera.cpp:793 configuring streams: (0) 1280x704-NV12
[4:14:20.326528371] [44825] ERROR MediaDevice media_device.cpp:802 /dev/media1[ipu3-imgu]: Failed to setup link: Invalid argument
Failed to configure camera
djrscally commented 3 years ago

Yeah, I added tps68470_init to probe(). Now, regulator_bulk_enable() is working.

Oh! What's the tps68470_init that you added then? To the regulator driver code?

supply_map still shows nothing. I'm not sure if it's ok.

Me either. But then, I'm not sure what it's supposed to show!

However... libcamera still isn't working:

Dang, I was a bit hopeful that it might work.

kitakar5525 commented 3 years ago

All the code is taken from Intel's original code.

Here is the patch (I changed indentation from spaces to tabs):

0008-regulator-tps68470-regulator-add-init_data.patch

```diff From a0d70028192d20d942b3b565d5865cb445ae1126 Mon Sep 17 00:00:00 2001 From: Tsuchiya Yuto Date: Thu, 20 Aug 2020 21:13:11 +0900 Subject: [PATCH 8/8] regulator: tps68470-regulator: add init_data Signed-off-by: Tsuchiya Yuto --- drivers/regulator/tps68470-regulator.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/drivers/regulator/tps68470-regulator.c b/drivers/regulator/tps68470-regulator.c index c17ed74a676c..76c21c922e20 100644 --- a/drivers/regulator/tps68470-regulator.c +++ b/drivers/regulator/tps68470-regulator.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include @@ -108,6 +109,27 @@ static const struct regulator_desc regulators[] = { ARRAY_SIZE(tps68470_ldo_ranges)), }; +#define tps68470_reg_init_data(_name, _min_uV, _max_uV)\ +{\ + .constraints = {\ + .name = (const char *)_name,\ + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE \ + | REGULATOR_CHANGE_STATUS,\ + .min_uV = _min_uV,\ + .max_uV = _max_uV,\ + },\ +} + +struct regulator_init_data tps68470_init[] = { + tps68470_reg_init_data("CORE", 900000, 1950000), + tps68470_reg_init_data("ANA", 875000, 3100000), + tps68470_reg_init_data("VCM", 875000, 3100000), + tps68470_reg_init_data("VIO", 875000, 3100000), + tps68470_reg_init_data("VSIO", 875000, 3100000), + tps68470_reg_init_data("AUX1", 875000, 3100000), + tps68470_reg_init_data("AUX2", 875000, 3100000), +}; + static int tps68470_regulator_probe(struct platform_device *pdev) { struct regulator_config config = {}; @@ -122,6 +144,8 @@ static int tps68470_regulator_probe(struct platform_device *pdev) for (i = 0; i < ARRAY_SIZE(regulators); i++) { + config.init_data = &tps68470_init[i]; + rdev = devm_regulator_register(&pdev->dev, ®ulators[i], &config); if (IS_ERR(rdev)) { -- 2.28.0 ```

djrscally commented 3 years ago

Good grief, how did I manage to miss all that out?! Alright thanks, I'll apply that and see if it enables them for me too.

Shame it's not working, but it does at least feel like progress.

EDIT: OK yep, thanks for that. After that patch I get identical output in regulator summary to you and the failures during insmod of the camera driver are gone too. Still nothing in supply_map

djrscally commented 3 years ago

Still struggling to achieve the link to the cpio2 device. When surface_camera module is inserted I get this output:

[  140.813968] ov2680 i2c-OVTI2680:00: surface_camera driver found the OVTI2680 device.
[  140.813976] ov2680 i2c-OVTI2680:00: surface_camera driver found camera device.
[  140.813987] ipu3-cio2 0000:00:14.3: surface_camera driver found cio2 device.
[  140.882155] ipu3-cio2 0000:00:14.3: device 0x9d32 (rev: 0x1)
[  140.882970] ipu3-cio2 0000:00:14.3: cio2_parse_firmware(), endpoint not available for CIO2 port 0
[  140.882976] ipu3-cio2 0000:00:14.3: cio2_parse_firmware(), endpoint not available for CIO2 port 1
[  140.882981] ipu3-cio2 0000:00:14.3: cio2_parse_firmware(), endpoint not available for CIO2 port 2
[  140.882984] ipu3-cio2 0000:00:14.3: cio2_parse_firmware(), endpoint not available for CIO2 port 3

I've added a couple more debugging prints. The only things I changed in the surface_camera driver was replacing the ov number to 2680 (which is cosmetic of course) and altering the macro for teh ACPI device to mine: OVTI2680. I then set the nodes array to this:

static const struct software_node nodes[] = {
    { SURFACE_OV2680_HID },
    { "port0", &nodes[0] },
    { "endpoint0", &nodes[1], ov2680_props },
    { SURFACE_CIO2_HID },
    { "port1", &nodes[3] },
    { "endpoint0", &nodes[4], cio2_props },
    { }
};

Which I set based on this output from ssdb_dump (after copying in the SSDB from my DSDT):

CAM0:clockdiv: 0
link: 0
lanes: 2
mclkspeed: 19200000
mclkport: 0

CAM1:clockdiv: 0
link: 1
lanes: 1
mclkspeed: 19200000
mclkport: 0

I'm targeting CAM1 at the moment. So far as I can see, I added endpoint0 to port1 with cio2_props ok, so not sure why it wouldn't find them.

kitakar5525 commented 3 years ago

It looks ok for me, not sure what is going wrong... I added surface_camera driver for your CAM1. You can try this: https://github.com/kitakar5525/surface-ipu3-cameras/tree/surface_camera_ov2680/surface_camera_from_jhand2

EDIT: (changed branch name later): https://github.com/kitakar5525/surface-ipu3-cameras/tree/lenovo_miix_510/surface_camera_from_jhand2/lenovo_miix_510_ov2680

djrscally commented 3 years ago

Thanks; same error though. Guess I'll compare my DSDT to the SB ones and see if I can spot some differences that might explain it. I wanted to try adding an I2cSerialBus2 entry to the PMIC anyway so I don't have to instantiate it every boot.

kitakar5525 commented 3 years ago

Ah, regarding the surface_camera driver, have you applied software-node patches? (0001 to 0003 in this dir: https://github.com/jhand2/surface-camera/tree/master/patches)

djrscally commented 3 years ago

Ah, nope, I'll apply those and test. I also tried the DSDT amendments from your repo but that wasn't working either - are jhand2's patches needed for that method too?

Need to catch up on how all this segment is working, haven't found time to read through the code yet

On Sat, 22 Aug 2020, 12:24 Hayataka, notifications@github.com wrote:

Ah, regarding the surface_camera driver, have you applied software-node patches? (0001 to 0003 in this dir: https://github.com/jhand2/surface-camera/tree/master/patches)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/linux-surface/linux-surface/issues/91#issuecomment-678628754, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDBE22WZEKVEWXZ5K5TYWLSB6TGTANCNFSM4KXUXK7A .

kitakar5525 commented 3 years ago

The swnode patches are required for the surface_camera driver to work, sorry I forgot to tell you.

Regarding DSDT amendments, swnode patches are not needed. It should work standalone. DSDT is device-specific. So, you can't use my DSDT as-is but you can refer to the commit history for what I did.

Can I add your DSDT (found on Stack Overflow) to my repo so that I can do the similar amendments done for my SB1?

djrscally commented 3 years ago

Sure go ahead. I didn't copy your modifications entirely, I was following the method of the modifications you made. But I have obviously done it wrong!

On Sat, 22 Aug 2020, 13:17 Hayataka, notifications@github.com wrote:

The swnode patches are required for the surface_camera driver to work, sorry I forgot to tell you.

Regarding DSDT amendments, swnode patches are not needed. It should work standalone. DSDT is device-specific. So, you can't use my DSDT as-is but you can refer to the commit history for what I did.

Can I add your DSDT (found on Stack Overflow) to my repo so that I can do the similar amendments done for my SB1?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/linux-surface/linux-surface/issues/91#issuecomment-678633509, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDBE26PKIIGUYCGCRPFB2DSB6ZNJANCNFSM4KXUXK7A .