irs1125 infineon tof camera support #3261

Coimbra1984 · 2019-10-02T13:12:08Z

As discussed very early this year (january) with @6by9 this pull reuquest adds support for the infineon tof sensor irs1125, which is used in our 3D tof frontend for raspberry pi (see https://www.conrad.de/de/p/makerfactory-x-pieye-3d-cmos-farb-kameramodul-passend-fuer-raspberry-pi-2141283.html).

The changes in bcm2835-unicam.c are some kind of hack from my side. I guess some more discussions might be necessary there.

6by9

This needs to be split into multiple patches for the different bits of the system.

bcm2835-unicam, with reasoning for the change.
Add IRS1125 driver
Add IRS1125 to defconfigs
Add IRS1125 overlays

Kernel coding is against the kernel coding style docs (https://github.com/raspberrypi/linux/tree/rpi-4.19.y/Documentation/process). There is a script at scripts/checkpatch.pl that will check your patch for places where the coding style hasn't been followed. When run with the --strict flag it throws up 474 errors and 5032 warnings with your patch. Many of those will be whitespace issues that can be fixed quickly.
You also need to check licence headers on several files, and the patches need a Signed-off-by line (use git commit -s for git to do it automatically).

The change to bcm2835-unicam.c needs a touch of finessing. I believe you're trying to unpack the CSI2 raw 12 data format into the 16bpp V4L2_PIX_FMT_Y12 format. I think I'd prefer to see a new V4L2_PIX_FMT_Y12P format added in the same format as V4L2_PIX_FMT_Y10P, but that is a load of faffing.

I do need to revisit the driver to determine the correct format for repacked 16bpp data. I think the better way is to add a 2nd fourcc for the repacked format as we will only ever repack to 16bpp. It needs to support having the base fourcc being 0 for your case, and the alternate fourcc being 0 for cases such as RGB and YUYV where repacking is not applicable. Doing that should be relatively straightforward, so I'll see what I can sort quickly so you can drop your changes.

drivers/media/i2c/irs1125_regs.h

arch/arm/boot/dts/overlays/irs1125-overlay.dts

drivers/media/i2c/irs1125.c

drivers/media/i2c/irs1125_regs.h

6by9 · 2019-10-02T17:01:45Z

FWIW I've pushed a quick set of mods to the Unicam driver that should support advertising all permutations of V4L2 formats based on unpacking. It's not quite right as requesting an unsupported format doesn't do the right thing, but it's close.
https://github.com/6by9/linux/tree/unicam-repacking

And I've fallen into the same trap of doing too many things at once - the same patch removes the caching of active formats and adds support for repacking. They should have been two patches, so the branch will get updated.

6by9 · 2019-10-03T15:43:54Z

https://github.com/6by9/linux/tree/unicam-repacking is updated to add what I hope is the required unpacking options for your code. It's taken a couple of attempts to untangle my patches (you have to "love" git), but I think they're all good.

I'll create a PR with RFC (Request For Comments) tag against it, but will wait until you confirm that it works with your sensor before merging it.

6by9 · 2019-10-03T16:02:57Z

PR #3265 created for the Unicam changes.

Coimbra1984 · 2019-10-04T15:02:38Z

drivers/media/i2c/irs1125.h

@@ -0,0 +1,71 @@
+/*


I'm not sure about this file. I use this one also from user space (currently I just copy it into my user space project), but as far as I remember there is also a location, maybe include/video to put these header files which are shared between kernel and user space?

/include/uapi/linux
Once "published" then uapi defines are pretty much set in stone to avoid regressions, so ensure you've got it right before you push it to there.

That's the correct way, but you'll often find that applications end up having to include the latest and greatest headers from the point at which they were built in order to avoid a #ifdef hell trying to remove code that relies on it.

Okay, than I definitely will leave it in drivers/media/i2c
We are only using a subset of the imager's possibilities so far.

Coimbra1984 · 2019-10-04T15:06:28Z

This needs to be split into multiple patches for the different bits of the system.

bcm2835-unicam, with reasoning for the change.

Add IRS1125 driver

Add IRS1125 to defconfigs

Add IRS1125 overlays

Okay, I'll do that. I'll need a bit for that as I'm only familiar with the basic git commands (I guess I need this one: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History)

6by9 · 2019-10-04T15:36:05Z

This needs to be split into multiple patches for the different bits of the system.

bcm2835-unicam, with reasoning for the change.

Add IRS1125 driver

Add IRS1125 to defconfigs

Add IRS1125 overlays

Okay, I'll do that. I'll need a bit for that as I'm only familiar with the basic git commands (I guess I need this one: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History)

It can be a tricky one, but in this case I suggest you:

git fetch origin, git rebase to rebase your branch against the top our tree.
Note the commit hash for the top of your branch (currently 1914668, but you've made changes and will have just rebased).
Check out a clean branch of rpi-4.19.y so that you retain your existing branch as a backup.
Use git checkout 191466821501b23c168f025da35b0acdfeb49aa9 drivers/media/i2c (replace 1914668 with the hash you noted earlier). This should pick up and stage all the changes to that directory on your branch. (You may need to repeat with include/uapi too if you've moved your header).
git commit -s to commit those staged changes. Use an appropriate commit text. The first line should be something like "media: i2c: Add driver for Infineon IRS1125 Time of flight depth sensor".
Use git checkout 191466821501b23c168f025da35b0acdfeb49aa9 arch/arm/configs to pick up your config changes.
git commit -s to commit those staged changes. Again use a sensible commit text.

Repeat for the remainig requested patch splits.

If you've committed them in the wrong order, then use git rebase -i to reorder the commits. (Rearrange the order of the lines in the editor that it opens).

I've missed one patch that will be required for upstream - a device tree binding for how to load and configure the driver. See fd5109b for the IMX219 binding patch (not that I've upstreamed that yet).
Device tree bindings are also considered to be set in stone once published. Changes must always be backwards compatible (which can be a right pain in the neck).

Coimbra1984 · 2019-10-07T06:41:16Z

It can be a tricky one, but in this case I suggest you:

Okay, maybe it's better in this case that you first look whether the content is good, and after that I make clean commits

6by9

A few minor comments, but looking pretty good.
It still needs to be broken out into independent patches though.

6by9 · 2019-10-08T20:17:25Z

Documentation/devicetree/bindings/media/i2c/irs1125.txt

+The Infineon irs1125 is a time of flight digital image sensor with
+an active array size of 352H x 286V. It is programmable through I2C
+interface. The I2C address defaults to 0x3D, but can be set to 0x3C or
+none standard mipi address 0x41.


non standard.
What do you mean by a mipi address?

I mean mipi csi-2. I changed this description a bit. Mipi CSI-2 suggests these I2C addresses:

6.2 CCI Slave Addresses

For camera modules having only raw Bayer output the 7-bit slave address should be 011011Xb, where X =
0 or 1. For all other camera modules the 7-bit slave address should be 011110Xb

Wow, never noticed that bit in the CSI spec, and rarely seen it followed! (Having said that, OV5647 is on 0x36, which is 011011Xb)

TBH I'd drop reference to non-standard MIPI addresses. Specify the I2C addresses that the module can be configured to, and leave it at that.
"The I2C address will be one of 0x3C, 0x3D, or 0x41, selected by ."
You'll get more detailed feedback on the DT bindings should you try and upstream them.

Okay, maybe we have another problem here. The default address after power on is 0x3D, you can than change the address to be either 0x3C, 0x3D or 0x41 by writing to a register. Would it be valid to support only 0x3D or how should that one be modeled?

Joys! It's one of them.
I seem to recall one other sensor driver does support dynamic changing of the I2C address because people were wanting to use it for stereoscopic capture, but it gets very messy in doing so.

Yes, supporting only 0x3D in the driver is valid. DT describes what the hardware can potentially do. Drivers don't have to implement all of that (eg DT describes a clock source, but the driver can reject it should the clock not be the one it wanted).

"The I2C address defaults to 0x3D, but can be reconfigured to address 0x3C or 0x41 via I2C commands"

arch/arm/boot/dts/overlays/Makefile

6by9 · 2019-10-08T20:32:07Z

arch/arm/configs/bcm2709_defconfig

@@ -919,6 +919,7 @@ CONFIG_VIDEO_TW9906=m
 CONFIG_VIDEO_IMX219=m
 CONFIG_VIDEO_OV5647=m
 CONFIG_VIDEO_OV7640=m
+CONFIG_VIDEO_IRS1125=m


Again, Phil will ask that this has been set in the right place via make savedefconfig. Is this in the right place, or is this a manual edit?

Manual edit :) I'll change it...

Good, because I'm very strict. ;-)

@pelwell I fully understand.

That's strange I end up on the same line. So what I did is:

Remove irs1125 entry from bcm2709_defconfig

make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- bcm2709_defconfig

vim .config -> there was a line # CONFIG_VIDEO_IRS1125 is not set

changed that line to CONFIG_VIDEO_IRS1125=m

make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- savedefconfig

vim defconfig and search for IRS1125 which is again on line 922:

920 CONFIG_VIDEO_OV5647=m
921 CONFIG_VIDEO_OV7640=m
922 CONFIG_VIDEO_IRS1125=m
923 CONFIG_VIDEO_MT9V011=m
924 CONFIG_DRM=m

Is this the workflow I should follow?

Yes - there are other ways to manipulate the .config file (see scripts/config), but the savedefconfig is the important part. You either have excellent instincts or it was a lucky guess.

I guess it's a sign to play lottery today

drivers/media/i2c/irs1125.c

6by9 · 2019-10-08T20:40:16Z

drivers/media/i2c/irs1125.c

+			goto out;
+		}
+	}
+


This may be my omission from imx219, but do we not have power down code?

Okay, I can definitly set the reset pin here. This is also the recommended way in the user manual to save power. I'll change and test it.

line 444 } else if (!on && irs1125->power_count == 1) {
otherwise the first call to turn the sensor off will always turn it off, even if multiple power on calls have been made.

ON/ON/OFF/ON will currently leave the sensor OFF, because the last ON call will have failed line 427 as !irs1125->power_count is false.

6by9 · 2019-10-08T20:45:56Z

drivers/media/i2c/irs1125.c

+	}
+
+	xclk_freq = clk_get_rate(sensor->xclk);
+	if (xclk_freq != 25000000) {


So the binding gives 24MHz in the example, the register array says 26MHz, and this now checks against 25MHz. Which one should it be?

Okay, sorry my mistake. These are actually copy-paste artifacts. We have a reference clock of 26MHz on our hardware, which is also recommended by infineon.
To be honest, I did not fully understand the device-tree entry. But now having read fixed-clock.txt and clock-bindings.txt these entries describe an external clock "producer".
So in my case it must be a fixed-clock with frequency 26000000 and clock-cells=0 (only one clock output)

It is a bit obscure, and DT is a bit of a steep learning curve, but yes it is defining the clock frequency of the oscillator on your board.

In theory that clock could be derived from any of a number of places, and the driver could cope with a number of source clock frequencies, adjusting PLLs appropriately.
If Infineon recommend 26MHz, then it is reasonable for the driver to only work with that one value, but it shouldn't be reflected in the binding as that only describes the potential configuration options.

drivers/media/i2c/irs1125.c

6by9

One final bit of code left lying around, and I think I'm good other than splitting up the patches. If you want then I'm happy to split them up for you, push to a branch of mine, and then you take ownership of them back (I'll give you some instructions). Having said that, the instructions I gave earlier in the review process should be relatively simple to follow.

If (hopefully when) you try to upstream it there will be a number of really annoying requests to meet their coding style. Things like local variables in a file being declared with the longest line at the top and getting shorter.
I lose track of them all, and they are local to the individual Linux subsystems (in this case linux-media). It's the point I often give up on my efforts because it has near zero real value :-/

6by9 · 2019-10-10T15:46:53Z

drivers/media/i2c/irs1125.c

+	},
+	.probe		= irs1125_probe,
+	.remove		= irs1125_remove,
+	.id_table	= irs1125_id,


id_table shouldn't be required as loading is controlled by device tree, and hence struct i2c_device_id irs1125_id and MODULE_DEVICE_TABLE(i2c, irs1125_id); above can also be deleted.

6by9 · 2019-10-10T16:13:37Z

Thank you.
I think I'm happy with the overall content, so it's just breaking it up into standalone commits, as I commented at #3261 (review). The offer is there if you want me to do it.

@pelwell Did you want to do a review of the overall change now, or once split?

pelwell · 2019-10-10T16:15:23Z

Split, please - it's easier once the changes are separated.

Coimbra1984 · 2019-10-10T16:17:28Z

I think I'm happy with the overall content, so it's just breaking it up into standalone commits, as I commented at #3261 (review). The offer is there if you want me to do it.

Yes please :) I tried to rebase and make them look like these commits: https://github.com/raspberrypi/linux/pull/3188/commits
but somehow I fail at the end when I try to push my rewritten history to my branch. So if you can do it, it will be much faster I guess

6by9 · 2019-10-10T17:03:08Z

OK, branch https://github.com/6by9/linux/tree/pieye should have all your changes from this PR, but under my name.
Hopefully the following will rewrite the history on them to show you as the author.

git remote add 6by9 https://github.com/6by9/linux
git remote fetch 6by9
git checkout -t -b split 6by9/pieye

Please check that the tree builds and works at this point. If it does then we can rewrite the history.

git rebase -i HEAD~3

This will present you with an editor looking something like

pick 3d51849 dt-bindings: Add binding for the Infineon IRS1125 sensor
pick 085fe80 media: i2c: Add a driver for the Infineon IRS1125 depth sensor
pick ffcbb30 configs: Add CONFIG_VIDEO_IRS1125 to the defconfigs
pick ab737c9 dtoverlays: Add an overlay for the Infineon IRS1125

Replace the word "pick" at the start of each line with "e" or "edit" (no quotes).
Save and exit the editor.
Git should now reset to each commit in turn and give you a chance to edit them. You want to run
git commit --amend --reset-author -s
on each one in turn to rewrite the commiter details. Then use git rebase --continue to step forward to the next commit. Repeat those two commands 4 times over for the 4 commits.

git push -f pieye HEAD:rpi-4.19.y (replace pieye with whatever you called your remote) to force update your branch on github, and it should update this PR in the same way your other pushes have done.
NB This does destroy your working branch on github, but that shouldn't matter as we really care about the final result rather than how we got there.

6by9 · 2019-10-10T17:06:06Z

Of coure being git there have to be at least 3 ways to do the same thing.

It may be quicker/easier to do

git remote add 6by9 https://github.com/6by9/linux
git remote fetch 6by9
git checkout -t -b split origin/rpi-4.19.y

to get a clean Pi kernel tree.
Then cherrypick the relevant commits from my branch:

git cherry-pick 3d51849 
git commit --amend --reset-author -s
git cherry-pick 085fe80 
git commit --amend --reset-author -s
git cherry-pick ffcbb30 
git commit --amend --reset-author -s
git cherry-pick ab737c9 
git commit --amend --reset-author -s

and push to github

git push -f pieye HEAD:rpi-4.19.y

Coimbra1984 · 2019-10-10T19:38:30Z

Okay I have finally tested everything on a raspberry pi 3B+ and a raspberry pi 4. Works as expected 👍

Coimbra1984 · 2019-10-10T19:38:57Z

Yes please :) I tried to rebase and make them look like these commits: https://github.com/raspberrypi/linux/pull/3188/commits
but somehow I fail at the end when I try to push my rewritten history to my branch. So if you can do it, it will be much faster I guess

I guess my mistake was a missing -f on git push ...

Coimbra1984 · 2019-10-15T06:28:10Z

Hi @6by9 , just to make sure, there is currently no pending todo from my side, right?

6by9 · 2019-10-15T06:32:06Z

Sorry, hadn't checked your push, but it looks fine.
@pelwell All split out into nice commits now.

Coimbra1984 · 2019-10-18T04:21:41Z

@pelwell do you have any review comments?

pelwell

After @6by9 has been through the code I'm usually left with error handling and stylistic issues, and this was no exception. Fix these few minor niggles and I'll merge.

arch/arm/boot/dts/overlays/irs1125-overlay.dts

pelwell · 2019-10-18T08:05:04Z

arch/arm/boot/dts/overlays/README

@@ -1346,6 +1346,17 @@ Params: card_name               Override the default, "IQAudIODigi", card name.
        dai_stream_name         Override the default, "IQAudIO Digi HiFi",
                                dai stream name.



Sorry, but there must be two blank lines between entries.

pelwell · 2019-10-18T08:17:09Z

arch/arm/boot/dts/overlays/irs1125-overlay.dts

+				reg = <0x3D>;
+				status = "okay";
+
+				pwdn-gpios = <&expgpio 5 0>;


The gpio values are going to be overwritten by the firmware so what I'm describing isn't a bug, but it would help my verification script if the default version resolves against all Pi DTBs (it currently won't apply for Pis without a GPIO expander), even if it won't work as written - replacing &expgpio with &gpio fixes it for me.

okay, I changed this

pelwell · 2019-10-18T08:26:46Z

drivers/media/i2c/irs1125.c

+
+static int irs1125_set_get_fmt(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_pad_config *cfg,
+		struct v4l2_subdev_format *format)


Indentation.

pelwell · 2019-10-18T08:27:21Z

drivers/media/i2c/irs1125.c

+}
+
+static const struct v4l2_subdev_video_ops irs1125_subdev_video_ops = {
+	.s_stream =				 irs1125_s_stream,


With nothing to align with, the extra spaces are just a distraction.

Sure, removed.

pelwell · 2019-10-18T08:30:58Z

drivers/media/i2c/irs1125.c

+
+	ret = irs1125_ctrls_init(sensor, dev);
+	if (ret < 0)
+		goto error;


Shouldn't this be goto mutex_remove;?

you are right, I changed it

pelwell · 2019-10-18T08:32:55Z

drivers/media/i2c/irs1125.c

+		return ret;
+	}
+
+	if (read != 0x0a12) {


A macro constant named for what 0x0a12 represents would be nice, especially since the value is used twice.

Adds a binding for the Infineon IRS1125 time-of-flight depth sensor. Signed-off-by: Markus Proeller <markus.proeller@pieye.org>

The Infineon IRS1125 is a time of flight depth sensor that has a CSI-2 interface. Add a V4L2 subdevice driver for this device. Signed-off-by: Markus Proeller <markus.proeller@pieye.org>

Adds the Infineon IRS1125 driver module to the downstream defconfigs. Signed-off-by: Markus Proeller <markus.proeller@pieye.org>

The Infineon IRS1125 is a CSI2 time of flight depth sensor which has a suitable V4L2 subdevice driver. Add an overlay for configuring it. Signed-off-by: Markus Proeller <markus.proeller@pieye.org>

kernel: irs1125 infineon tof camera support See: raspberrypi/linux#3261 kernel: unicam fixes for YUYV one to many mappings See: raspberrypi/linux#3290 kernel: Add w5500 support (for #3276) See: raspberrypi/linux#3278 firmware: arm_loader: Add os_prefix option See: raspberrypi/linux#3237 firmware: Add support for arbitrary memory specification

6by9 requested changes Oct 2, 2019

View reviewed changes

6by9 mentioned this pull request Oct 3, 2019

Unicam repacking #3265

Merged

Coimbra1984 commented Oct 4, 2019

View reviewed changes

6by9 reviewed Oct 8, 2019

View reviewed changes

6by9 reviewed Oct 10, 2019

View reviewed changes

pelwell requested changes Oct 18, 2019

View reviewed changes

Coimbra1984 added 4 commits October 18, 2019 12:13

dt-bindings: Add binding for the Infineon IRS1125 sensor

617aacb

Adds a binding for the Infineon IRS1125 time-of-flight depth sensor. Signed-off-by: Markus Proeller <markus.proeller@pieye.org>

media: i2c: Add a driver for the Infineon IRS1125 depth sensor

5db64d6

The Infineon IRS1125 is a time of flight depth sensor that has a CSI-2 interface. Add a V4L2 subdevice driver for this device. Signed-off-by: Markus Proeller <markus.proeller@pieye.org>

configs: Add CONFIG_VIDEO_IRS1125 to the defconfigs

aaa00db

Adds the Infineon IRS1125 driver module to the downstream defconfigs. Signed-off-by: Markus Proeller <markus.proeller@pieye.org>

dtoverlays: Add an overlay for the Infineon IRS1125

2730d94

The Infineon IRS1125 is a CSI2 time of flight depth sensor which has a suitable V4L2 subdevice driver. Add an overlay for configuring it. Signed-off-by: Markus Proeller <markus.proeller@pieye.org>

pelwell approved these changes Oct 18, 2019

View reviewed changes

pelwell merged commit 6018f7e into raspberrypi:rpi-4.19.y Oct 18, 2019

		@@ -1346,6 +1346,17 @@ Params: card_name Override the default, "IQAudIODigi", card name.
		dai_stream_name Override the default, "IQAudIO Digi HiFi",
		dai stream name.

irs1125 infineon tof camera support #3261

irs1125 infineon tof camera support #3261

Conversation

Coimbra1984 commented Oct 2, 2019

6by9 left a comment

Choose a reason for hiding this comment

6by9 commented Oct 2, 2019

6by9 commented Oct 3, 2019

6by9 commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Coimbra1984 commented Oct 4, 2019

6by9 commented Oct 4, 2019 • edited Loading

Coimbra1984 commented Oct 7, 2019

6by9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6by9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6by9 commented Oct 10, 2019

pelwell commented Oct 10, 2019

Coimbra1984 commented Oct 10, 2019

6by9 commented Oct 10, 2019 • edited Loading

6by9 commented Oct 10, 2019

Coimbra1984 commented Oct 10, 2019

Coimbra1984 commented Oct 10, 2019

Coimbra1984 commented Oct 15, 2019

6by9 commented Oct 15, 2019

Coimbra1984 commented Oct 18, 2019

pelwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6by9 commented Oct 4, 2019 •

edited

Loading

6by9 commented Oct 10, 2019 •

edited

Loading