BCM283x Camera Receiver driver

Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as Unicam) on BCM283x SoCs.

Signed-off-by Dave Stevenson <[email protected]>

---
 .../devicetree/bindings/media/bcm2835-unicam.txt   | 107 +++++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +0 -0

new file mode 100644
index 0000000..2ee5af7

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +6 -0

+is no involvement with the VideoCore firmware. Unicam receives CSI-2
+(or CCP2) data and writes it into SDRAM. There is no additional processing
+performed.
+It should be possible to connect it to any sensor with a
+suitable output interface and V4L2 subdevice driver.
+

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +4 -0

+
+			csi1_ep: endpoint {
+				remote-endpoint = <&tc358743_0>;
+				data-lanes = <1 2>;

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +22 -0

+
+			clocks = <&tc358743_clk>;
+			clock-names = "refclk";
+
+			tc358743_clk: bridge-clk {
+				compatible = "fixed-clock";
+				#clock-cells = <0>;
+				clock-frequency = <27000000>;
+			};
+
+			port {
+				tc358743_0: endpoint {
+					remote-endpoint = <&csi1_ep>;
+					clock-lanes = <0>;
+					data-lanes = <1 2>;
+					clock-noncontinuous;
+					link-frequencies =
+						/bits/ 64 <297000000>;
+				};
+			};
+		};
+	};

Signature · 1 line

2.7.4

Quoted 15 lines · Dave Stevenson

Document the DT bindings for the CSI2/CCP2 receiver peripheral (known as Unicam) on BCM283x SoCs.

Signed-off-by Dave Stevenson <[email protected]>

---
 .../devicetree/bindings/media/bcm2835-unicam.txt   | 107 +++++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +0 -0

new file mode 100644
index 0000000..2ee5af7

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 8

Quoted 8 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +8 -0

@@ -0,0 +1,107 @@
+Broadcom BCM283x Camera Interface (Unicam)
+------------------------------------------
+
+The Unicam block on BCM283x SoCs is the receiver for either
+CSI-2 or CCP2 data from image sensors or similar devices.
+
+There are two camera drivers in the kernel for BCM283x - this one
+and bcm2835-camera (currently in staging).

Linux detail that is n/a for bindings.

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 10

Quoted 2 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +2 -0

+
+This driver is purely the kernel controlling the Unicam peripheral - there

Bindings describe h/w blocks, not drivers.

Quoted 12 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +12 -0

+is no involvement with the VideoCore firmware. Unicam receives CSI-2
+(or CCP2) data and writes it into SDRAM. There is no additional processing
+performed.
+It should be possible to connect it to any sensor with a
+suitable output interface and V4L2 subdevice driver.
+
+bcm2835-camera uses the VideoCore firmware to control the sensor,
+Unicam, ISP, and various tuner control loops. Fully processed frames are
+delivered to the driver by the firmware. It only has sensor drivers
+for Omnivision OV5647, and Sony IMX219 sensors, and is closed source.
+
+The two drivers are mutually exclusive for the same Unicam instance.

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 26

Quoted 4 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +4 -0

+The firmware checks the device tree configuration during boot. If
+it finds device tree nodes called csi0 or csi1 then it will block the
+firmware from accessing the peripheral, and bcm2835-camera will
+not be able to stream data.

All interesting, but irrelavent to the binding other than the part about node name. Reword to just state the requirements to get the firmware to ignore things.

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 28

Quoted 2 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +2 -0

+It should be possible to use bcm2835-camera on one camera interface
+and bcm2835-unicam on the other interface if there is a need to.

For upstream, I don't think we care to support that. We don't need 2 bindings.

Quoted 5 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +5 -0

+
+Required properties:
+===================
+- compatible : must be "brcm,bcm2835-unicam".
+- reg                : physical base address and length of the register sets for the

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 39

Quoted 6 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +6 -0

+               device.
+- interrupts : should contain the IRQ line for this Unicam instance.
+- clocks     : list of clock specifiers, corresponding to entries in
+               clock-names property.
+- clock-names        : must contain an "lp_clock" entry, matching entries
+               in the clocks property.

_clock is redundant.

Quoted 8 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +8 -0

+
+Unicam supports a single port node. It should contain one 'port' child node
+with child 'endpoint' node. Please refer to the bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Within the endpoint node, the following properties are mandatory:
+- remote-endpoint    : links to the source device endpoint.
+- data-lanes         : An array denoting how many data lanes are physically

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 52

Quoted 5 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +5 -0

+                       present for this CSI-2 receiver instance. This can
+                       be limited by either the SoC itself, or by the
+                       breakout on the platform.
+                       Lane reordering is not supported, so lanes must be
+                       in order, starting at 1.

Just refer to docs for standard properties. Just add any info on limits of values like number of cells.

Quoted 4 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +4 -0

+
+Lane reordering is not supported on the clock lane, so the optional property
+"clock-lane" will implicitly be <0>.
+Similarly lane inversion is not supported, therefore "lane-polarities" will

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 61

Quoted 5 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +5 -0

+implicitly be <0 0 0 0 0>.
+Neither of these values will be checked.
+
+Example:
+     csi1: csi@7e801000 {

I thought the node had to be called csi0 or csi1. The label (csi1) will be gone from the compiled dtb.

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 71

Quoted 10 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +10 -0

+             compatible = "brcm,bcm2835-unicam";
+             reg = <0x7e801000 0x800>,
+                   <0x7e802004 0x4>;
+             interrupts = <2 7>;
+             clocks = <&clocks BCM2835_CLOCK_CAM1>;
+             clock-names = "lp_clock";
+
+             port {
+                     #address-cells = <1>;
+                     #size-cells = <0>;

Don't need these with a single endpoint.

Quoted 8 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +8 -0

+
+                     csi1_ep: endpoint {
+                             remote-endpoint = <&tc358743_0>;
+                             data-lanes = <1 2>;
+                     };
+             };
+     };
+

Documentation/devicetree/bindings/media/bcm2835-unicam.txt line 85

Quoted 6 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +6 -0

+     i2c0: i2c@7e205000 {
+
+             tc358743: csi-hdmi-bridge@0f {
+                     compatible = "toshiba,tc358743";
+                     reg = <0x0f>;
+                     status = "okay";

Don't show status in examples.

Quoted 17 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +17 -0

+
+                     clocks = <&tc358743_clk>;
+                     clock-names = "refclk";
+
+                     tc358743_clk: bridge-clk {
+                             compatible = "fixed-clock";
+                             #clock-cells = <0>;
+                             clock-frequency = <27000000>;
+                     };
+
+                     port {
+                             tc358743_0: endpoint {
+                                     remote-endpoint = <&csi1_ep>;
+                                     clock-lanes = <0>;
+                                     data-lanes = <1 2>;
+                                     clock-noncontinuous;
+                                     link-frequencies =

Quoted 8 lines

Documentation/devicetree/bindings/media/bcm2835-unicam.txt new file +5 -1

+                                             /bits/ 64 <297000000>;
+                             };
+                     };
+             };
+     };
--

2.7.4

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +0 -0

new file mode 100644
index 0000000..5b1adc3

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +34 -0

+static int unicam_probe(struct platform_device *pdev)
+{
+	struct unicam_cfg *unicam_cfg;
+	struct unicam_device *unicam;
+	struct v4l2_ctrl_handler *hdl;
+	struct resource	*res;
+	int ret;
+
+	unicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);
+	if (!unicam)
+		return -ENOMEM;
+
+	unicam->pdev = pdev;
+	unicam_cfg = &unicam->cfg;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	unicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(unicam_cfg->base)) {
+		unicam_err(unicam, "Failed to get main io block\n");
+		return PTR_ERR(unicam_cfg->base);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	unicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(unicam_cfg->clk_gate_base)) {
+		unicam_err(unicam, "Failed to get 2nd io block\n");
+		return PTR_ERR(unicam_cfg->clk_gate_base);
+	}
+
+	unicam->clock = devm_clk_get(&pdev->dev, "lp_clock");
+	if (IS_ERR(unicam->clock)) {
+		unicam_err(unicam, "Failed to get clock\n");
+		return PTR_ERR(unicam->clock);
+	}

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +20 -0

+	if (ret) {
+		dev_err(&pdev->dev, "Unable to request interrupt\n");
+		return -EINVAL;
+	}
+
+	ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);
+	if (ret) {
+		unicam_err(unicam,
+			   "Unable to register v4l2 device.\n");
+		return ret;
+	}
+
+	/* Reserve space for the controls */
+	hdl = &unicam->ctrl_handler;
+	ret = v4l2_ctrl_handler_init(hdl, 16);
+	if (ret < 0)
+		goto probe_out_v4l2_unregister;
+	unicam->v4l2_dev.ctrl_handler = hdl;
+
+	/* set the driver data in platform device */

Quoted 11 lines · Dave Stevenson

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +5 -0

new file mode 100644
index 0000000..5b1adc3
@@ -0,0 +1,2192 @@
+/*
+ * BCM2835 Unicam capture Driver
+ *
+ * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
+ *

drivers/media/platform/bcm2835/bcm2835-unicam.c line 10

Quoted 5 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +5 -0

+ * Dave Stevenson <[email protected]>
+ *
+ * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
+ * TI CAL camera interface driver by Benoit Parrot.
+ *

Possible future improvement: this description of the driver is really nice and could be turned into kernel-doc.

drivers/media/platform/bcm2835/bcm2835-unicam.c line 15

Quoted 4 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +4 -0

+ * There are two camera drivers in the kernel for BCM283x - this one
+ * and bcm2835-camera (currently in staging).
+ *
+ * This driver is purely the kernel control the Unicam peripheral - there

Maybe "This driver directly controls..."?

drivers/media/platform/bcm2835/bcm2835-unicam.c line 23

Quoted 8 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +8 -0

+ * is no involvement with the VideoCore firmware. Unicam receives CSI-2
+ * or CCP2 data and writes it into SDRAM. The only processing options are
+ * to repack Bayer data into an alternate format, and applying windowing
+ * (currently not implemented).
+ * It should be possible to connect it to any sensor with a
+ * suitable output interface and V4L2 subdevice driver.
+ *
+ * bcm2835-camera uses with the VideoCore firmware to control the sensor,

"uses the"

drivers/media/platform/bcm2835/bcm2835-unicam.c line 32

Quoted 9 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +9 -0

+ * Unicam, ISP, and all tuner control loops. Fully processed frames are
+ * delivered to the driver by the firmware. It only has sensor drivers
+ * for Omnivision OV5647, and Sony IMX219 sensors.
+ *
+ * The two drivers are mutually exclusive for the same Unicam instance.
+ * The VideoCore firmware checks the device tree configuration during boot.
+ * If it finds device tree nodes called csi0 or csi1 it will block the
+ * firmware from accessing the peripheral, and bcm2835-camera will
+ * not be able to stream data.

Thanks for describing this here!

drivers/media/platform/bcm2835/bcm2835-unicam.c line 143

Quoted 11 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +11 -0

+/*
+ * The peripheral can unpack and repack between several of
+ * the Bayer raw formats, so any Bayer format can be advertised
+ * as the same Bayer order in each of the supported bit depths.
+ * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
+ * formats.
+ */
+#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
+#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
+#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
+#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')

Should thes fourccs be defined in a common v4l2 header, to reserve it from clashing with others later?

This is really the only question I have about this driver before seeing it merged. As far as me wearing my platform maintainer hat, I'm happy with the driver, and my other little notes are optional.

Quoted 38 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +38 -0

+static int unicam_probe(struct platform_device *pdev)
+{
+     struct unicam_cfg *unicam_cfg;
+     struct unicam_device *unicam;
+     struct v4l2_ctrl_handler *hdl;
+     struct resource *res;
+     int ret;
+
+     unicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);
+     if (!unicam)
+             return -ENOMEM;
+
+     unicam->pdev = pdev;
+     unicam_cfg = &unicam->cfg;
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     unicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);
+     if (IS_ERR(unicam_cfg->base)) {
+             unicam_err(unicam, "Failed to get main io block\n");
+             return PTR_ERR(unicam_cfg->base);
+     }
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     unicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);
+     if (IS_ERR(unicam_cfg->clk_gate_base)) {
+             unicam_err(unicam, "Failed to get 2nd io block\n");
+             return PTR_ERR(unicam_cfg->clk_gate_base);
+     }
+
+     unicam->clock = devm_clk_get(&pdev->dev, "lp_clock");
+     if (IS_ERR(unicam->clock)) {
+             unicam_err(unicam, "Failed to get clock\n");
+             return PTR_ERR(unicam->clock);
+     }
+
+     ret = platform_get_irq(pdev, 0);
+     if (ret <= 0) {
+             dev_err(&pdev->dev, "No IRQ resource\n");

drivers/media/platform/bcm2835/bcm2835-unicam.c line 2113

Quoted 6 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +6 -0

+             return -ENODEV;
+     }
+     unicam_cfg->irq = ret;
+
+     ret = devm_request_irq(&pdev->dev, unicam_cfg->irq, unicam_isr, 0,
+                            "unicam_capture0", unicam);

Looks like there's no need to keep "irq" in the device private struct.

Quoted 25 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +25 -0

+     if (ret) {
+             dev_err(&pdev->dev, "Unable to request interrupt\n");
+             return -EINVAL;
+     }
+
+     ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);
+     if (ret) {
+             unicam_err(unicam,
+                        "Unable to register v4l2 device.\n");
+             return ret;
+     }
+
+     /* Reserve space for the controls */
+     hdl = &unicam->ctrl_handler;
+     ret = v4l2_ctrl_handler_init(hdl, 16);
+     if (ret < 0)
+             goto probe_out_v4l2_unregister;
+     unicam->v4l2_dev.ctrl_handler = hdl;
+
+     /* set the driver data in platform device */
+     platform_set_drvdata(pdev, unicam);
+
+     ret = of_unicam_connect_subdevs(unicam);
+     if (ret) {
+             dev_err(&pdev->dev, "Failed to connect subdevs\n");

drivers/media/platform/bcm2835/bcm2835-unicam.c line 2143

Quoted 5 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +5 -0

+             goto free_hdl;
+     }
+
+     /* Enabling module functional clock */
+     pm_runtime_enable(&pdev->dev);

I think pm_runtime is only controlling the power domain for the PHY, not the clock (which you're handling manually).

Hi Eric.

Thanks for the review.

Quoted 12 lines · Eric Anholt

Quoted 11 lines · Dave Stevenson

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +5 -0

new file mode 100644
index 0000000..5b1adc3
@@ -0,0 +1,2192 @@
+/*
+ * BCM2835 Unicam capture Driver
+ *
+ * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
+ *

Quoted 8 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 10

Quoted 5 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +5 -0

+ * Dave Stevenson <[email protected]>
+ *
+ * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
+ * TI CAL camera interface driver by Benoit Parrot.
+ *

Possible future improvement: this description of the driver is really nice and could be turned into kernel-doc.

Documentation?! Surely not :-)
For now I'll leave it as a task for another day.

Quoted 6 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 15

Quoted 4 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +4 -0

+ * There are two camera drivers in the kernel for BCM283x - this one
+ * and bcm2835-camera (currently in staging).
+ *
+ * This driver is purely the kernel control the Unicam peripheral - there

Maybe "This driver directly controls..."?

Will do in v3.

Quoted 10 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 23

Quoted 8 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +8 -0

+ * is no involvement with the VideoCore firmware. Unicam receives CSI-2
+ * or CCP2 data and writes it into SDRAM. The only processing options are
+ * to repack Bayer data into an alternate format, and applying windowing
+ * (currently not implemented).
+ * It should be possible to connect it to any sensor with a
+ * suitable output interface and V4L2 subdevice driver.
+ *
+ * bcm2835-camera uses with the VideoCore firmware to control the sensor,

"uses the"

Will do in v3.

Quoted 12 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 32

Quoted 9 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +9 -0

+ * Unicam, ISP, and all tuner control loops. Fully processed frames are
+ * delivered to the driver by the firmware. It only has sensor drivers
+ * for Omnivision OV5647, and Sony IMX219 sensors.
+ *
+ * The two drivers are mutually exclusive for the same Unicam instance.
+ * The VideoCore firmware checks the device tree configuration during boot.
+ * If it finds device tree nodes called csi0 or csi1 it will block the
+ * firmware from accessing the peripheral, and bcm2835-camera will
+ * not be able to stream data.

Thanks for describing this here!

Quoted 14 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 143

Quoted 11 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +11 -0

+/*
+ * The peripheral can unpack and repack between several of
+ * the Bayer raw formats, so any Bayer format can be advertised
+ * as the same Bayer order in each of the supported bit depths.
+ * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
+ * formats.
+ */
+#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
+#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
+#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
+#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')

Should thes fourccs be defined in a common v4l2 header, to reserve it from clashing with others later?

I'm only using them as flags and probably in a manner that nothing else is likely to copy, so it seems a little excessive to put them in a common header.
Perhaps it's better to switch to 0xFFFFFFF0 to 0xFFFFFFF3 or other value that won't come up as a fourcc under any normal circumstance. Any thoughts from other people?

Quoted 43 lines

This is really the only question I have about this driver before seeing it merged. As far as me wearing my platform maintainer hat, I'm happy with the driver, and my other little notes are optional.

Quoted 38 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +38 -0

+static int unicam_probe(struct platform_device *pdev)
+{
+     struct unicam_cfg *unicam_cfg;
+     struct unicam_device *unicam;
+     struct v4l2_ctrl_handler *hdl;
+     struct resource *res;
+     int ret;
+
+     unicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);
+     if (!unicam)
+             return -ENOMEM;
+
+     unicam->pdev = pdev;
+     unicam_cfg = &unicam->cfg;
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     unicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);
+     if (IS_ERR(unicam_cfg->base)) {
+             unicam_err(unicam, "Failed to get main io block\n");
+             return PTR_ERR(unicam_cfg->base);
+     }
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     unicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);
+     if (IS_ERR(unicam_cfg->clk_gate_base)) {
+             unicam_err(unicam, "Failed to get 2nd io block\n");
+             return PTR_ERR(unicam_cfg->clk_gate_base);
+     }
+
+     unicam->clock = devm_clk_get(&pdev->dev, "lp_clock");
+     if (IS_ERR(unicam->clock)) {
+             unicam_err(unicam, "Failed to get clock\n");
+             return PTR_ERR(unicam->clock);
+     }
+
+     ret = platform_get_irq(pdev, 0);
+     if (ret <= 0) {
+             dev_err(&pdev->dev, "No IRQ resource\n");

Quoted 8 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 2113

Quoted 6 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +6 -0

+             return -ENODEV;
+     }
+     unicam_cfg->irq = ret;
+
+     ret = devm_request_irq(&pdev->dev, unicam_cfg->irq, unicam_isr, 0,
+                            "unicam_capture0", unicam);

Looks like there's no need to keep "irq" in the device private struct.

Agreed. I'll remove in v3.

Quoted 25 lines

Quoted 25 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +25 -0

+     if (ret) {
+             dev_err(&pdev->dev, "Unable to request interrupt\n");
+             return -EINVAL;
+     }
+
+     ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);
+     if (ret) {
+             unicam_err(unicam,
+                        "Unable to register v4l2 device.\n");
+             return ret;
+     }
+
+     /* Reserve space for the controls */
+     hdl = &unicam->ctrl_handler;
+     ret = v4l2_ctrl_handler_init(hdl, 16);
+     if (ret < 0)
+             goto probe_out_v4l2_unregister;
+     unicam->v4l2_dev.ctrl_handler = hdl;
+
+     /* set the driver data in platform device */
+     platform_set_drvdata(pdev, unicam);
+
+     ret = of_unicam_connect_subdevs(unicam);
+     if (ret) {
+             dev_err(&pdev->dev, "Failed to connect subdevs\n");

Quoted 8 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 2143

Quoted 5 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +5 -0

+             goto free_hdl;
+     }
+
+     /* Enabling module functional clock */
+     pm_runtime_enable(&pdev->dev);

I think pm_runtime is only controlling the power domain for the PHY, not the clock (which you're handling manually).

You're right. Copy and paste from the driver I'd based this on. Will correct in v3.

Dave

Quoted 78 lines · Dave Stevenson

Hi Eric.

Thanks for the review.

Quoted 12 lines · Eric Anholt

Quoted 11 lines · Dave Stevenson

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +5 -0

new file mode 100644
index 0000000..5b1adc3
@@ -0,0 +1,2192 @@
+/*
+ * BCM2835 Unicam capture Driver
+ *
+ * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
+ *

Quoted 8 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 10

Quoted 5 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +5 -0

+ * Dave Stevenson <[email protected]>
+ *
+ * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
+ * TI CAL camera interface driver by Benoit Parrot.
+ *

Possible future improvement: this description of the driver is really nice and could be turned into kernel-doc.

Documentation?! Surely not :-)
For now I'll leave it as a task for another day.

Quoted 6 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 15

Quoted 4 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +4 -0

+ * There are two camera drivers in the kernel for BCM283x - this one
+ * and bcm2835-camera (currently in staging).
+ *
+ * This driver is purely the kernel control the Unicam peripheral - there

Maybe "This driver directly controls..."?

Will do in v3.

Quoted 10 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 23

Quoted 8 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +8 -0

+ * is no involvement with the VideoCore firmware. Unicam receives CSI-2
+ * or CCP2 data and writes it into SDRAM. The only processing options are
+ * to repack Bayer data into an alternate format, and applying windowing
+ * (currently not implemented).
+ * It should be possible to connect it to any sensor with a
+ * suitable output interface and V4L2 subdevice driver.
+ *
+ * bcm2835-camera uses with the VideoCore firmware to control the sensor,

"uses the"

Will do in v3.

Quoted 26 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 32

Quoted 9 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +9 -0

+ * Unicam, ISP, and all tuner control loops. Fully processed frames are
+ * delivered to the driver by the firmware. It only has sensor drivers
+ * for Omnivision OV5647, and Sony IMX219 sensors.
+ *
+ * The two drivers are mutually exclusive for the same Unicam instance.
+ * The VideoCore firmware checks the device tree configuration during boot.
+ * If it finds device tree nodes called csi0 or csi1 it will block the
+ * firmware from accessing the peripheral, and bcm2835-camera will
+ * not be able to stream data.

Thanks for describing this here!

drivers/media/platform/bcm2835/bcm2835-unicam.c line 143

Quoted 11 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +11 -0

+/*
+ * The peripheral can unpack and repack between several of
+ * the Bayer raw formats, so any Bayer format can be advertised
+ * as the same Bayer order in each of the supported bit depths.
+ * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
+ * formats.
+ */
+#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
+#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
+#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
+#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')

Should thes fourccs be defined in a common v4l2 header, to reserve it from clashing with others later?

Quoted 6 lines

I'm only using them as flags and probably in a manner that nothing else is likely to copy, so it seems a little excessive to put them in a common header.
Perhaps it's better to switch to 0xFFFFFFF0 to 0xFFFFFFF3 or other value that won't come up as a fourcc under any normal circumstance. Any thoughts from other people?

I think that's better, yes.

Quoted 88 lines

Quoted 43 lines

This is really the only question I have about this driver before seeing it merged. As far as me wearing my platform maintainer hat, I'm happy with the driver, and my other little notes are optional.

Quoted 38 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +38 -0

+static int unicam_probe(struct platform_device *pdev)
+{
+     struct unicam_cfg *unicam_cfg;
+     struct unicam_device *unicam;
+     struct v4l2_ctrl_handler *hdl;
+     struct resource *res;
+     int ret;
+
+     unicam = devm_kzalloc(&pdev->dev, sizeof(*unicam), GFP_KERNEL);
+     if (!unicam)
+             return -ENOMEM;
+
+     unicam->pdev = pdev;
+     unicam_cfg = &unicam->cfg;
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     unicam_cfg->base = devm_ioremap_resource(&pdev->dev, res);
+     if (IS_ERR(unicam_cfg->base)) {
+             unicam_err(unicam, "Failed to get main io block\n");
+             return PTR_ERR(unicam_cfg->base);
+     }
+
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     unicam_cfg->clk_gate_base = devm_ioremap_resource(&pdev->dev, res);
+     if (IS_ERR(unicam_cfg->clk_gate_base)) {
+             unicam_err(unicam, "Failed to get 2nd io block\n");
+             return PTR_ERR(unicam_cfg->clk_gate_base);
+     }
+
+     unicam->clock = devm_clk_get(&pdev->dev, "lp_clock");
+     if (IS_ERR(unicam->clock)) {
+             unicam_err(unicam, "Failed to get clock\n");
+             return PTR_ERR(unicam->clock);
+     }
+
+     ret = platform_get_irq(pdev, 0);
+     if (ret <= 0) {
+             dev_err(&pdev->dev, "No IRQ resource\n");

Quoted 8 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c line 2113

Quoted 6 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +6 -0

+             return -ENODEV;
+     }
+     unicam_cfg->irq = ret;
+
+     ret = devm_request_irq(&pdev->dev, unicam_cfg->irq, unicam_isr, 0,
+                            "unicam_capture0", unicam);

Looks like there's no need to keep "irq" in the device private struct.

Agreed. I'll remove in v3.

Quoted 33 lines

Quoted 20 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +20 -0

+     if (ret) {
+             dev_err(&pdev->dev, "Unable to request interrupt\n");
+             return -EINVAL;
+     }
+
+     ret = v4l2_device_register(&pdev->dev, &unicam->v4l2_dev);
+     if (ret) {
+             unicam_err(unicam,
+                        "Unable to register v4l2 device.\n");
+             return ret;
+     }
+
+     /* Reserve space for the controls */
+     hdl = &unicam->ctrl_handler;
+     ret = v4l2_ctrl_handler_init(hdl, 16);
+     if (ret < 0)
+             goto probe_out_v4l2_unregister;
+     unicam->v4l2_dev.ctrl_handler = hdl;
+
+     /* set the driver data in platform device */

drivers/media/platform/bcm2835/bcm2835-unicam.c line 2143

Quoted 10 lines

drivers/media/platform/bcm2835/bcm2835-unicam.c new file +10 -0

+     platform_set_drvdata(pdev, unicam);
+
+     ret = of_unicam_connect_subdevs(unicam);
+     if (ret) {
+             dev_err(&pdev->dev, "Failed to connect subdevs\n");
+             goto free_hdl;
+     }
+
+     /* Enabling module functional clock */
+     pm_runtime_enable(&pdev->dev);

I think pm_runtime is only controlling the power domain for the PHY, not the clock (which you're handling manually).

Quoted 5 lines

You're right. Copy and paste from the driver I'd based this on. Will correct in v3.

Dave

Dave, I plan to review this Friday or Monday. It would help me if you could post a v3 before Friday so that I'm reviewing the latest code.

It would be great if you can also post your tc358743 patches. I have an RPi with a tc358743 attached, so it would be very useful if I can review and test both this driver and the tc358743 changes.

I also have a selfish motive: I want to do a CEC demo next week during the Kernel Recipes conference with my RPi/tc358743 and your driver. It's nice if I can use the latest version for that.

Hi All.

This is v2 for adding a V4L2 subdevice driver for the CSI2/CCP2 camera receiver peripheral on BCM283x, as used on Raspberry Pi. Sorry for the delay since v1 - other tasks assigned, got sucked into investigating why some devices were misbehaving, and picking up on new features that had been added to the tree (eg CCP2).

v4l2-compliance results depend on the sensor subdevice connected to. I have results for TC358743, ADV7282M, and OV5647 that I'll send them as a follow up email.

OV647 and ADV7282M are now working with this driver, as well as TC358743. v1 of the driver only supported continuous clock mode which Unicam was failing to lock on to correctly.
The driver now checks the clock mode and adjusts termination accordingly. Something is still a little off for OV5647, but I'll investigate that later.

As per the v1 discussion with Hans, I have added text describing the differences between this driver and the one in staging/vc04_service. Addressing some of the issues in the bcm2835-camera driver is on my to-do list, and I'll add similar text there when I'm dealing with that.

For those wanting to see the driver in context,
https://github.com/6by9/upstream-linux/tree/unicam is the linux-media tree with my mods on top. It also includes a couple of TC358743 and OV5647 driver updates that I'll send to the list in the next few days.

Thanks in advance.
Dave

Changes from v1 to v2:
- Broken out a new helper function v4l2_fourcc2s as requested by Hans.
- Documented difference between this driver and the bcm2835-camera driver in staging/vc04_services.
- Corrected handling of s_dv_timings and s_std to update the current format but only if not streaming. This refactored some of the s_fmt code to remove duplication.
- Updated handling of sizeimage to include vertical padding. (Not updated the bytesperline calcs as the app can override).
- Added support for continuous clock mode (requires changes to lane termination configuration).
- Add support for CCP2 as Sakari's patches to support it have now been merged. I don't have a suitable sensor to test it with at present, but all settings have been taken from a known working configuration. If people would prefer I remove this until it has been proved against hardware then I'm happy to do so.
- Updated DT bindings to use <data-lanes> on the Unicam node to set the maximum number of lanes present instead of a having a custom property. Documents the mandatory endpoint properties.
- Removed RAW16 from the list of input formats as it isn't defined in the CSI-2 spec. The peripheral can still unpack the other Bayer formats to a 16 bit/pixel packing though.
- Added a log-status handler to get the status from the sensor.
- Automatically switch away from any interlaced formats reported via g_fmt, or that are attempted to be set via try/s_fmt.
- Addressed other more minor code review comments from v1.

Dave Stevenson (4):
[media] v4l2-common: Add helper function for fourcc to string [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface
MAINTAINERS: Add entry for BCM2835 camera driver

 .../devicetree/bindings/media/bcm2835-unicam.txt   |  107 +
 MAINTAINERS                                        |    7 +
 drivers/media/platform/Kconfig                     |    1 +
 drivers/media/platform/Makefile                    |    1 +
 drivers/media/platform/bcm2835/Kconfig             |   14 +
 drivers/media/platform/bcm2835/Makefile            |    3 +
 drivers/media/platform/bcm2835/bcm2835-unicam.c    | 2192 ++++++++++++++++++++
 drivers/media/platform/bcm2835/vc4-regs-unicam.h   |  264 +++
 drivers/media/v4l2-core/v4l2-common.c              |   18 +
 include/media/v4l2-common.h                        |    3 +
 10 files changed, 2610 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
 create mode 100644 drivers/media/platform/bcm2835/Kconfig
 create mode 100644 drivers/media/platform/bcm2835/Makefile
 create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
 create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h

--
2.7.4

OV5647

v4l2-compliance SHA : f6ecbc90656815d91dc6ba90aac0ad8193a14b38

Driver Info:

    Driver name   : unicam
    Card type     : unicam
    Bus info      : platform:unicam 3f801000.csi1
    Driver version: 4.13.0
    Capabilities  : 0x85200001
        Video Capture
        Read/Write
        Streaming
        Extended Pix Format
        Device Capabilities
    Device Caps   : 0x05200001
        Video Capture
        Read/Write
        Streaming
        Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
fail: v4l2-test-controls.cpp(587): g_ext_ctrls does not
support count == 0

Quoted 59 lines · Dave Stevenson

OV5647

v4l2-compliance SHA : f6ecbc90656815d91dc6ba90aac0ad8193a14b38

Driver Info:

    Driver name   : unicam
    Card type     : unicam
    Bus info      : platform:unicam 3f801000.csi1
    Driver version: 4.13.0
    Capabilities  : 0x85200001
        Video Capture
        Read/Write
        Streaming
        Extended Pix Format
        Device Capabilities
    Device Caps   : 0x05200001
        Video Capture
        Read/Write
        Streaming
        Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported)

Quoted 8 lines

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
fail: v4l2-test-controls.cpp(587): g_ext_ctrls does not
support count == 0

Huh. The issue here is that there are no controls at all, but the control API is present. The class_check() function in v4l2-ctrls.c expects that there are controls and if not it returns -EINVAL, causing this test to fail.

Try to apply this patch:

Signed-off-by Hans Verkuil <[email protected]>

---

drivers/media/v4l2-core/v4l2-ctrls.c +1 -1

index dd1db678718c..4e53a8654690 100644
@@ -2818,7 +2818,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
 {
        if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
-               return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
+               return 0;
        return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
 }
 

and see if it will pass the compliance test. There may be other issues. I think that the compliance test should handle the case where there are no controls, so this is a good test.

That said, another solution is that the driver detects that there are no controls in unicam_probe_complete() and sets unicam->v4l2_dev.ctrl_handler to NULL.

I think you should do this in v4. Having control ioctls but no actual controls is not wrong as such, but it is a bit misleading towards the application.

Quoted 92 lines · Hans Verkuil

Quoted 59 lines · Dave Stevenson

OV5647

v4l2-compliance SHA : f6ecbc90656815d91dc6ba90aac0ad8193a14b38

Driver Info:

    Driver name   : unicam
    Card type     : unicam
    Bus info      : platform:unicam 3f801000.csi1
    Driver version: 4.13.0
    Capabilities  : 0x85200001
        Video Capture
        Read/Write
        Streaming
        Extended Pix Format
        Device Capabilities
    Device Caps   : 0x05200001
        Video Capture
        Read/Write
        Streaming
        Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported)

Quoted 8 lines

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
fail: v4l2-test-controls.cpp(587): g_ext_ctrls does not
support count == 0

Huh. The issue here is that there are no controls at all, but the control API is present. The class_check() function in v4l2-ctrls.c expects that there are controls and if not it returns -EINVAL, causing this test to fail.

Try to apply this patch:

Signed-off-by Hans Verkuil <[email protected]>

---

drivers/media/v4l2-core/v4l2-ctrls.c +1 -1

index dd1db678718c..4e53a8654690 100644
@@ -2818,7 +2818,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
 {
        if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
-               return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
+               return 0;
        return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
 }
 

Quoted 3 lines

and see if it will pass the compliance test. There may be other issues. I think that the compliance test should handle the case where there are no controls, so this is a good test.

Fails.
fail: v4l2-test-controls.cpp(589): g_ext_ctrls worked even
when no controls are present
test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL

Quoted 7 lines

That said, another solution is that the driver detects that there are no controls in unicam_probe_complete() and sets unicam->v4l2_dev.ctrl_handler to NULL.

I think you should do this in v4. Having control ioctls but no actual controls is not wrong as such, but it is a bit misleading towards the application.

OK, will do.

Dave

Quoted 93 lines · Dave Stevenson

Quoted 92 lines · Hans Verkuil

Quoted 59 lines · Dave Stevenson

OV5647

v4l2-compliance SHA : f6ecbc90656815d91dc6ba90aac0ad8193a14b38

Driver Info:

    Driver name   : unicam
    Card type     : unicam
    Bus info      : platform:unicam 3f801000.csi1
    Driver version: 4.13.0
    Capabilities  : 0x85200001
        Video Capture
        Read/Write
        Streaming
        Extended Pix Format
        Device Capabilities
    Device Caps   : 0x05200001
        Video Capture
        Read/Write
        Streaming
        Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported)

Quoted 8 lines

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
fail: v4l2-test-controls.cpp(587): g_ext_ctrls does not
support count == 0

Huh. The issue here is that there are no controls at all, but the control API is present. The class_check() function in v4l2-ctrls.c expects that there are controls and if not it returns -EINVAL, causing this test to fail.

Try to apply this patch:

Signed-off-by Hans Verkuil <[email protected]>

---

drivers/media/v4l2-core/v4l2-ctrls.c +1 -1

index dd1db678718c..4e53a8654690 100644
@@ -2818,7 +2818,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
 {
        if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
-               return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
+               return 0;
        return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
 }
 

Quoted 8 lines

Quoted 3 lines

and see if it will pass the compliance test. There may be other issues. I think that the compliance test should handle the case where there are no controls, so this is a good test.

Fails.
fail: v4l2-test-controls.cpp(589): g_ext_ctrls worked even
when no controls are present
test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL

Try this patch:

-----------------

utils/v4l2-compliance/v4l2-test-controls.cpp +2 -4

index 7514459f..508daf05 100644
@@ -585,8 +585,6 @@ int testExtendedControls(struct node *node)
                return ret;
        if (ret)
                return fail("g_ext_ctrls does not support count == 0\n");
-       if (node->controls.empty())
-               return fail("g_ext_ctrls worked even when no controls are present\n");
        if (ctrls.which)
                return fail("field which changed\n");
        if (ctrls.count)

@@ -600,8 +598,6 @@ int testExtendedControls(struct node *node)
                return ret;
        if (ret)
                return fail("try_ext_ctrls does not support count == 0\n");
-       if (node->controls.empty())
-               return fail("try_ext_ctrls worked even when no controls are present\n");
        if (ctrls.which)
                return fail("field which changed\n");
        if (ctrls.count)

@@ -687,6 +683,8 @@ int testExtendedControls(struct node *node)
        }
 
        ctrls.which = 0;
+       ctrls.count = 1;
+       ctrls.controls = &ctrl;
        ctrl.id = 0;
        ctrl.size = 0;
        ret = doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls);