Add a camera regulator node(s) to allow sharing on CM4 and remove firmware nastiness #4051

6by9 · 2021-01-06T18:22:39Z

At the moment the firmware fixes up where the camera shutdown line goes, which means those overlays can't be dynamically loaded.
Because the overlays are claiming GPIOs it also means there isn't an easy way to load a very similar overlay for both CAM0 and CAM1 on a CM4 where the same GPIO is shared for both camera modules.

Adds a new cam1_reg node (and cam0_reg on CMs), and update those overlays that use regulators to use it.

6by9 · 2021-01-06T18:23:55Z

@naushir Could you give this a quick test on your system(s)? It works for me, but I've only really tested CM4 and Pi4 and IMX477 and OV9281. All the others are pretty much copy/paste, but any greater test coverage would be appreciated.

pelwell

From reading it through it looks like it would work, but there's a lot of repetition. Would you consider either pushing both regulators into a common base dtsi file (or files) or a new file created for the purpose, then overriding just the gpio declarations in the per-board files?

pelwell · 2021-01-06T19:40:42Z

arch/arm/boot/dts/bcm2710-rpi-cm3.dts

@@ -16,7 +31,7 @@

 &gpio {
 	spi0_pins: spi0_pins {
-		brcm,pins = <9 10 11>;
+		brcm,pins = <	9 10 11>;


Whitespace.

pelwell · 2021-01-06T19:40:53Z

arch/arm/boot/dts/bcm2711-rpi-4-b.dts

@@ -603,3 +611,4 @@
 			   <&spi0>, "dmas:8=", <&dma40>;
 	};
 };
+


pelwell · 2021-01-06T19:42:07Z

arch/arm/boot/dts/bcm2711-rpi-cm4.dts

+	};
+
+	aliases {
+		cam0_reg = &cam1_reg;


You'll have to explain this, especially as it's the only cam_reg alias.

This is probably where I've got it wrong between aliases and labels.

CM4 shares the same GPIO to control both CAM0 & CAM1, so I want the same regulator node to be referenced as either cam1_reg or cam0_reg from overlays.

Internal references usually require phandles, which are the compiled equivalent of labels. Just put a second, cam0_reg label on the cam1_reg node.

pelwell · 2021-01-06T19:46:45Z

arch/arm/boot/dts/bcm2711-rpi-cm4.dts

@@ -65,6 +65,18 @@
 		enable-active-high;
 		gpio = <&expgpio 6 GPIO_ACTIVE_HIGH>;
 	};
+
+	cam1_reg: cam1_reg {


I'd prefer these patches to go after the "Downstream rpi- changes" comment below.

6by9 · 2021-01-06T20:33:57Z

Sure, happy to make whatever changes you want. It was easier to create the PR and get comments than to discuss abstracts.

pelwell · 2021-01-06T21:22:05Z

Yes - I'd much rather discuss something concrete.

6by9 · 2021-01-11T16:19:18Z

Looking at this, the CM4 config which wants

cam0_reg: cam1_reg: cam1_reg {
  // Stuff
};

vs the rest which want

cam1_reg: cam1_reg {
  // Stuff
};

is the awkward bit.

A couple of options I can think of:

One include for all platforms EXCEPT CMs where it is written out in full. The GPIO gets set in the individual platform file.
An include that populates the boilerplate of the regulator ie

compatible = "regulator-fixed";
regulator-name = "cam1-reg";
enable-active-high;
status = "disabled";

so each platform could then do

cam1_reg: cam1_reg {
   #include "bcm283x-camera-regulator.dtsi"
};

except that the included file isn't a complete dt description.

Preference? I'm assuming the former.

6by9 · 2021-01-11T16:45:48Z

On the assumption that the former approach was going to be preferred, I've made the updates (and removed the whitespace issues). It also makes things clearer as it's rebased the branch.

pelwell

It is technically possible to have a common include file that declares both regulators. The labels can be attached by the including file by changing:

&cam1_reg {

to:

cam1_reg: /cam1_reg {

This is not a request, just something to consider.

Having said that, I'm largely happy with the current state of the PR except for the bits that still need deleting and (from what I could see) the absence of the file to be included.

arch/arm/boot/dts/bcm2709-rpi-2-b.dts

arch/arm/boot/dts/bcm2710-rpi-3-b-plus.dts

6by9 · 2021-01-11T17:23:51Z

As cam0 physically doesn't exist on most of these platforms I'd prefer for the regulator node not to exist either, otherwise it's likely to cause confusion.

And the good old forgetting to git add the new file - oops!

The current firmware fixup of camera sensor overlays is not particularly nice, and it stops you being able to load them dynamically. It's also incompatible with creating a simple DT that can be loaded for both CAM1 and CAM0 on a CM as they would both try to claim the one GPIO. Almost all sensors have a hook of some form for a regulator, so it's relatively straightforward to convert them all to use a fixed regulator with GPIO control. Add a fixed regulator node for each platform with the GPIO correctly configured for the camera shutdown line. (The LED line is ignored). Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Update those overlays that use the regulator framework to use the new cam1_reg node to control the camera shutdown line, and remove the firmware workaround nodes. Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

6by9 · 2021-01-11T17:28:12Z

Updated.

pelwell · 2021-01-11T17:29:18Z

Do you think someone might want an overlay that can target either camera port, selectable by a parameter? I believe that the lack of a label in the base DTB that is referred to be the overlay will currently prevent such an overlay being applied, even if it is the target of a dormant fragment. On second thoughts, it might be better to fix this case in the overlay application mechanisms.

6by9 · 2021-01-11T17:37:35Z

I was considering whether we can parameterise some of the overlays as it would be nice for them to be generic.
Trying to get a pair of OV9281 sensors running on a CM4 was what kicked this off as the firmware fixup is lacking because two sensors/regulators get assigned the same GPIO.

I'm rebuilding due to the bump to 5.10.6 at present, but will experiment when complete to see if it fails.

I think it may make the overlay overly complex anyway, as the csiN endpoint has to point to the sensor endpoint and vice versa. Then again aren't those labels only local to the overlay, so irrelevant whilst applying the second one?

eg https://github.com/raspberrypi/linux/blob/rpi-5.10.y/arch/arm/boot/dts/overlays/ov9281-overlay.dts

pelwell · 2021-01-11T17:40:46Z

I'm happy to merge as is - things can still be changed should the need arise.

6by9 · 2021-01-11T17:42:39Z

I'm happy if you're happy!

The intent of having cam1_reg and cam0_reg should remain from now on, but the exact use of those from within overlays can be tweaked.

kernel: overlays: seeed-can-fd-hat: clarify how to identify HAT version See: raspberrypi/linux#4072 kernel: Add a camera regulator node(s) to allow sharing on CM4 and remove firmware nastiness See: raspberrypi/linux#4051 firmware: isp: Fix handling of different YUV colour spaces firmware: poe_hat: Actually close the I2C handle

kernel: overlays: seeed-can-fd-hat: clarify how to identify HAT version See: raspberrypi/linux#4072 kernel: Add a camera regulator node(s) to allow sharing on CM4 and remove firmware nastiness See: raspberrypi/linux#4051 firmware: isp: Fix handling of different YUV colour spaces firmware: poe_hat: Actually close the I2C handle Signed-off-by: Christian Stewart <christian@paral.in>

6by9 mentioned this pull request Jan 6, 2021

Update libcamera README.md raspberrypi/documentation#1765

Closed

pelwell reviewed Jan 6, 2021

View reviewed changes

popcornmix force-pushed the rpi-5.10.y branch 2 times, most recently from 3403b48 to ab9e2a6 Compare January 11, 2021 14:55

6by9 force-pushed the rpi-5.10.y-ov9281 branch from e742136 to 9a4a1be Compare January 11, 2021 16:45

6by9 force-pushed the rpi-5.10.y-ov9281 branch from 9a4a1be to e267db1 Compare January 11, 2021 16:48

pelwell reviewed Jan 11, 2021

View reviewed changes

arch/arm/boot/dts/bcm2709-rpi-2-b.dts Outdated Show resolved Hide resolved

arch/arm/boot/dts/bcm2710-rpi-3-b-plus.dts Outdated Show resolved Hide resolved

6by9 added 2 commits January 11, 2021 17:23

6by9 force-pushed the rpi-5.10.y-ov9281 branch from e267db1 to 82d65b6 Compare January 11, 2021 17:27

pelwell merged commit 8b5e9ea into raspberrypi:rpi-5.10.y Jan 11, 2021

raspberrypi deleted a comment from pepe-invest-git Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a camera regulator node(s) to allow sharing on CM4 and remove firmware nastiness #4051

Add a camera regulator node(s) to allow sharing on CM4 and remove firmware nastiness #4051

6by9 commented Jan 6, 2021

6by9 commented Jan 6, 2021

pelwell left a comment

pelwell Jan 6, 2021

pelwell Jan 6, 2021

pelwell Jan 6, 2021

6by9 Jan 6, 2021

pelwell Jan 6, 2021

pelwell Jan 6, 2021

6by9 commented Jan 6, 2021

pelwell commented Jan 6, 2021

6by9 commented Jan 11, 2021

6by9 commented Jan 11, 2021

pelwell left a comment

6by9 commented Jan 11, 2021

6by9 commented Jan 11, 2021

pelwell commented Jan 11, 2021

6by9 commented Jan 11, 2021

pelwell commented Jan 11, 2021

6by9 commented Jan 11, 2021

Add a camera regulator node(s) to allow sharing on CM4 and remove firmware nastiness #4051

Add a camera regulator node(s) to allow sharing on CM4 and remove firmware nastiness #4051

Conversation

6by9 commented Jan 6, 2021

6by9 commented Jan 6, 2021

pelwell left a comment

Choose a reason for hiding this comment

pelwell Jan 6, 2021

Choose a reason for hiding this comment

pelwell Jan 6, 2021

Choose a reason for hiding this comment

pelwell Jan 6, 2021

Choose a reason for hiding this comment

6by9 Jan 6, 2021

Choose a reason for hiding this comment

pelwell Jan 6, 2021

Choose a reason for hiding this comment

pelwell Jan 6, 2021

Choose a reason for hiding this comment

6by9 commented Jan 6, 2021

pelwell commented Jan 6, 2021

6by9 commented Jan 11, 2021

6by9 commented Jan 11, 2021

pelwell left a comment

Choose a reason for hiding this comment

6by9 commented Jan 11, 2021

6by9 commented Jan 11, 2021

pelwell commented Jan 11, 2021

6by9 commented Jan 11, 2021

pelwell commented Jan 11, 2021

6by9 commented Jan 11, 2021