* [PATCH 0/5] media: Allwinner A10 CSI support
@ 2018-11-13 8:24 Maxime Ripard
2018-11-13 8:24 ` [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding Maxime Ripard
` (6 more replies)
0 siblings, 7 replies; 36+ messages in thread
From: Maxime Ripard @ 2018-11-13 8:24 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand, Maxime Ripard
Hi,
Here is a series introducing the support for the A10 (and SoCs of the same
generation) CMOS Sensor Interface (called CSI, not to be confused with
MIPI-CSI, which isn't support by that IP).
That interface is pretty straightforward, but the driver has a few issues
that I wanted to bring up:
* The only board I've been testing this with has an ov5640 sensor
attached, which doesn't work with the upstream driver. Copying the
Allwinner init sequence works though, and this is how it has been
tested. Testing with a second sensor would allow to see if it's an
issue on the CSI side or the sensor side.
* When starting a capture, the last buffer to capture will fail due to
double buffering being used, and we don't have a next buffer for the
last frame. I'm not sure how to deal with that though. It seems like
some drivers use a scratch buffer in such a case, some don't care, so
I'm not sure which solution should be preferred.
* We don't have support for the ISP at the moment, but this can be added
eventually.
* How to model the CSI module clock isn't really clear to me. It looks
like it goes through the CSI controller and then is muxed to one of the
CSI pin so that it can clock the sensor. I'm not quite sure how to
model it, if it should be a clock, the CSI driver being a clock
provider, or if the sensor should just use the module clock directly.
Here is the v4l2-compliance output:
v4l2-compliance SHA : 339d550e92ac15de8668f32d66d16f198137006c
Driver Info:
Driver name : sun4i_csi
Card type : sun4i-csi
Bus info : platform:1c09000.csi
Driver version: 4.19.0
Capabilities : 0x84201000
Video Capture Multiplanar
Streaming
Extended Pix Format
Device Capabilities
Device Caps : 0x04201000
Video Capture Multiplanar
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 (Not Supported)
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 (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0
Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK (Not Supported)
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK
Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK
Test input 0:
Total: 43, Succeeded: 43, Failed: 0, Warnings: 0
Let me know what you think,
Maxime
Maxime Ripard (5):
dt-bindings: media: Add Allwinner A10 CSI binding
media: sunxi: Refactor the Makefile and Kconfig
media: sunxi: Add A10 CSI driver
ARM: dts: sun7i: Add CSI0 controller
DO NOT MERGE: ARM: dts: bananapi: Add Camera support
Documentation/devicetree/bindings/media/sun4i-csi.txt | 71 ++-
arch/arm/boot/dts/sun7i-a20-bananapi.dts | 98 +++-
arch/arm/boot/dts/sun7i-a20.dtsi | 13 +-
drivers/media/platform/Kconfig | 2 +-
drivers/media/platform/Makefile | 2 +-
drivers/media/platform/sunxi/Kconfig | 2 +-
drivers/media/platform/sunxi/Makefile | 2 +-
drivers/media/platform/sunxi/sun4i-csi/Kconfig | 12 +-
drivers/media/platform/sunxi/sun4i-csi/Makefile | 5 +-
drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 275 ++++++++-
drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h | 137 ++++-
drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 383 +++++++++++-
drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c | 287 ++++++++-
13 files changed, 1287 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/media/sun4i-csi.txt
create mode 100644 drivers/media/platform/sunxi/Kconfig
create mode 100644 drivers/media/platform/sunxi/Makefile
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Kconfig
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Makefile
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
base-commit: 94517eaa3d43005472864615dfd17f1ef6ca3935
--
git-series 0.9.1
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding
2018-11-13 8:24 [PATCH 0/5] media: Allwinner A10 CSI support Maxime Ripard
@ 2018-11-13 8:24 ` Maxime Ripard
2018-11-13 8:38 ` Sakari Ailus
2018-11-13 10:34 ` Sakari Ailus
2018-11-13 8:24 ` [PATCH 2/5] media: sunxi: Refactor the Makefile and Kconfig Maxime Ripard
` (5 subsequent siblings)
6 siblings, 2 replies; 36+ messages in thread
From: Maxime Ripard @ 2018-11-13 8:24 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand, Maxime Ripard
The Allwinner A10 CMOS Sensor Interface is a camera capture interface also
used in later (A10s, A13, A20, R8 and GR8) SoCs.
On some SoCs, like the A10, there's multiple instances of that controller,
with one instance supporting more channels and having an ISP.
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
Documentation/devicetree/bindings/media/sun4i-csi.txt | 71 ++++++++++++-
1 file changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/sun4i-csi.txt
diff --git a/Documentation/devicetree/bindings/media/sun4i-csi.txt b/Documentation/devicetree/bindings/media/sun4i-csi.txt
new file mode 100644
index 000000000000..3d96bcbef9d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/sun4i-csi.txt
@@ -0,0 +1,71 @@
+Allwinner A10 CMOS Sensor Interface
+-------------------------------------
+
+The Allwinner A10 SoC features two camera capture interfaces, one
+featuring an ISP and the other without. Later SoCs built upon that
+design and used similar SoCs.
+
+Required properties:
+ - compatible: value must be one of:
+ * allwinner,sun4i-a10-csi
+ * allwinner,sun5i-a13-csi, allwinner,sun4i-a10-csi
+ * allwinner,sun7i-a20-csi, allwinner,sun4i-a10-csi
+ - reg: base address and size of the memory-mapped region.
+ - interrupts: interrupt associated to this IP
+ - clocks: phandles to the clocks feeding the CSI
+ * ahb: the CSI interface clock
+ * mod: the CSI module clock
+ * ram: the CSI DRAM clock
+ - clock-names: the clock names mentioned above
+ - resets: phandles to the reset line driving the CSI
+
+Optional properties:
+ - allwinner,csi-channels: Number of channels available in the CSI
+ controller. If not present, the default
+ will be 1.
+ - allwinner,has-isp: Whether the CSI controller has an ISP
+ associated to it or not
+
+If allwinner,has-isp is set, an additional "isp" clock is needed,
+being a phandle to the clock driving the ISP.
+
+The CSI node should contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt. The
+endpoint's bus type must be parallel or BT656.
+
+Endpoint node properties for CSI
+---------------------------------
+
+- remote-endpoint : (required) a phandle to the bus receiver's endpoint
+ node
+- bus-width: : (required) must be 8
+- pclk-sample : (optional) (default: sample on falling edge)
+- hsync-active : (only required for parallel)
+- vsync-active : (only required for parallel)
+
+Example:
+
+csi0: csi@1c09000 {
+ compatible = "allwinner,sun7i-a20-csi",
+ "allwinner,sun4i-a10-csi";
+ reg = <0x01c09000 0x1000>;
+ interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
+ <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
+ clock-names = "ahb", "mod", "isp", "ram";
+ resets = <&ccu RST_CSI0>;
+ allwinner,csi-channels = <4>;
+ allwinner,has-isp;
+
+ port {
+ csi_from_ov5640: endpoint {
+ remote-endpoint = <&ov5640_to_csi>;
+ bus-width = <8>;
+ data-shift = <2>;
+ hsync-active = <1>; /* Active high */
+ vsync-active = <0>; /* Active low */
+ pclk-sample = <1>; /* Rising */
+ };
+ };
+};
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 2/5] media: sunxi: Refactor the Makefile and Kconfig
2018-11-13 8:24 [PATCH 0/5] media: Allwinner A10 CSI support Maxime Ripard
2018-11-13 8:24 ` [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding Maxime Ripard
@ 2018-11-13 8:24 ` Maxime Ripard
2018-11-13 8:24 ` [PATCH 3/5] media: sunxi: Add A10 CSI driver Maxime Ripard
` (4 subsequent siblings)
6 siblings, 0 replies; 36+ messages in thread
From: Maxime Ripard @ 2018-11-13 8:24 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand, Maxime Ripard
The Makefile and Kconfig for the sun6i CSI driver are included in the main
Makefile / KConfig file. Since we're going to add a new CSI driver for an
older chip, and the Cedrus driver eventually, it makes more sense to put
those in our directory.
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
drivers/media/platform/Kconfig | 2 +-
drivers/media/platform/Makefile | 2 +-
drivers/media/platform/sunxi/Kconfig | 1 +
drivers/media/platform/sunxi/Makefile | 1 +
4 files changed, 4 insertions(+), 2 deletions(-)
create mode 100644 drivers/media/platform/sunxi/Kconfig
create mode 100644 drivers/media/platform/sunxi/Makefile
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 3c8b15ea957f..52cc41f63df3 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -137,7 +137,7 @@ source "drivers/media/platform/am437x/Kconfig"
source "drivers/media/platform/xilinx/Kconfig"
source "drivers/media/platform/rcar-vin/Kconfig"
source "drivers/media/platform/atmel/Kconfig"
-source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
+source "drivers/media/platform/sunxi/Kconfig"
config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 2662a230f681..2083bbd35584 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -99,4 +99,4 @@ obj-y += meson/
obj-y += cros-ec-cec/
-obj-$(CONFIG_VIDEO_SUN6I_CSI) += sunxi/sun6i-csi/
+obj-y += sunxi/
diff --git a/drivers/media/platform/sunxi/Kconfig b/drivers/media/platform/sunxi/Kconfig
new file mode 100644
index 000000000000..1b6e89cb78b2
--- /dev/null
+++ b/drivers/media/platform/sunxi/Kconfig
@@ -0,0 +1 @@
+source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
diff --git a/drivers/media/platform/sunxi/Makefile b/drivers/media/platform/sunxi/Makefile
new file mode 100644
index 000000000000..8d06f98500ee
--- /dev/null
+++ b/drivers/media/platform/sunxi/Makefile
@@ -0,0 +1 @@
+obj-y += sun6i-csi/
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 8:24 [PATCH 0/5] media: Allwinner A10 CSI support Maxime Ripard
2018-11-13 8:24 ` [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding Maxime Ripard
2018-11-13 8:24 ` [PATCH 2/5] media: sunxi: Refactor the Makefile and Kconfig Maxime Ripard
@ 2018-11-13 8:24 ` Maxime Ripard
2018-11-13 8:57 ` Sakari Ailus
` (2 more replies)
2018-11-13 8:24 ` [PATCH 4/5] ARM: dts: sun7i: Add CSI0 controller Maxime Ripard
` (3 subsequent siblings)
6 siblings, 3 replies; 36+ messages in thread
From: Maxime Ripard @ 2018-11-13 8:24 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand, Maxime Ripard
The older CSI drivers have camera capture interface different from the one
in the newer ones.
This IP is pretty simple. Some variants (one controller out of two
instances on some SoCs) have an ISP embedded, but there's no code that make
use of it, so we ignored that part for now.
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
drivers/media/platform/sunxi/Kconfig | 1 +-
drivers/media/platform/sunxi/Makefile | 1 +-
drivers/media/platform/sunxi/sun4i-csi/Kconfig | 12 +-
drivers/media/platform/sunxi/sun4i-csi/Makefile | 5 +-
drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 275 +++++++++-
drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h | 137 +++++-
drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 383 +++++++++++++-
drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c | 287 ++++++++++-
8 files changed, 1101 insertions(+)
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Kconfig
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Makefile
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
diff --git a/drivers/media/platform/sunxi/Kconfig b/drivers/media/platform/sunxi/Kconfig
index 1b6e89cb78b2..71808e93ac2e 100644
--- a/drivers/media/platform/sunxi/Kconfig
+++ b/drivers/media/platform/sunxi/Kconfig
@@ -1 +1,2 @@
+source "drivers/media/platform/sunxi/sun4i-csi/Kconfig"
source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
diff --git a/drivers/media/platform/sunxi/Makefile b/drivers/media/platform/sunxi/Makefile
index 8d06f98500ee..a05127529006 100644
--- a/drivers/media/platform/sunxi/Makefile
+++ b/drivers/media/platform/sunxi/Makefile
@@ -1 +1,2 @@
+obj-y += sun4i-csi/
obj-y += sun6i-csi/
diff --git a/drivers/media/platform/sunxi/sun4i-csi/Kconfig b/drivers/media/platform/sunxi/sun4i-csi/Kconfig
new file mode 100644
index 000000000000..841a6f4d9c99
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun4i-csi/Kconfig
@@ -0,0 +1,12 @@
+config VIDEO_SUN4I_CSI
+ tristate "Allwinner A10 CMOS Sensor Interface Support"
+ depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
+ depends on ARCH_SUNXI || COMPILE_TEST
+ select VIDEOBUF2_DMA_CONTIG
+ select V4L2_FWNODE
+ select V4L2_MEM2MEM_DEV
+ help
+ This is a V4L2 driver for the Allwinner A10 CSI
+
+ To compile this driver as a module, choose M here: the module
+ will be called sun4i_csi.
diff --git a/drivers/media/platform/sunxi/sun4i-csi/Makefile b/drivers/media/platform/sunxi/sun4i-csi/Makefile
new file mode 100644
index 000000000000..7c790a57f5ee
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun4i-csi/Makefile
@@ -0,0 +1,5 @@
+sun4i-csi-y += sun4i_csi.o
+sun4i-csi-y += sun4i_dma.o
+sun4i-csi-y += sun4i_v4l2.o
+
+obj-$(CONFIG_VIDEO_SUN4I_CSI) += sun4i-csi.o
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
new file mode 100644
index 000000000000..3e0a65ccbbe2
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2016 NextThing Co
+ * Copyright (C) 2016-2018 Bootlin
+ *
+ * Author: Maxime Ripard <maxime.ripard@bootlin.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/videodev2.h>
+
+#include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-mediabus.h>
+
+#include <media/videobuf2-core.h>
+#include <media/videobuf2-dma-contig.h>
+
+#include "sun4i_csi.h"
+
+static int csi_notify_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_subdev *asd)
+{
+ struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi,
+ notifier);
+
+ csi->src_subdev = subdev;
+ csi->src_pad = media_entity_get_fwnode_pad(&subdev->entity,
+ subdev->fwnode,
+ MEDIA_PAD_FL_SOURCE);
+ if (csi->src_pad < 0) {
+ dev_err(csi->dev, "Couldn't find output pad for subdev %s\n",
+ subdev->name);
+ return csi->src_pad;
+ }
+
+ dev_dbg(csi->dev, "Bound %s pad: %d\n", subdev->name, csi->src_pad);
+ return 0;
+}
+
+static int csi_notify_complete(struct v4l2_async_notifier *notifier)
+{
+ struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi,
+ notifier);
+ int ret;
+
+ if (notifier->num_subdevs != 1)
+ return -EINVAL;
+
+ ret = v4l2_device_register_subdev_nodes(&csi->v4l);
+ if (ret < 0)
+ return ret;
+
+ ret = csi_v4l2_register(csi);
+ if (ret < 0)
+ return ret;
+
+ return media_create_pad_link(&csi->src_subdev->entity, csi->src_pad,
+ &csi->vdev.entity, 0,
+ MEDIA_LNK_FL_ENABLED |
+ MEDIA_LNK_FL_IMMUTABLE);
+}
+
+static const struct v4l2_async_notifier_operations csi_notify_ops = {
+ .bound = csi_notify_bound,
+ .complete = csi_notify_complete,
+};
+
+static int sun4i_csi_async_parse(struct device *dev,
+ struct v4l2_fwnode_endpoint *vep,
+ struct v4l2_async_subdev *asd)
+{
+ struct sun4i_csi *csi = dev_get_drvdata(dev);
+
+ if (vep->base.port || vep->base.id)
+ return -EINVAL;
+
+ if (vep->bus_type != V4L2_MBUS_PARALLEL)
+ return -EINVAL;
+
+ csi->bus = vep->bus.parallel;
+
+ return 0;
+}
+
+static int csi_probe(struct platform_device *pdev)
+{
+ struct sun4i_csi *csi;
+ struct resource *res;
+ int ret;
+ int irq;
+
+ csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
+ if (!csi)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, csi);
+ csi->dev = &pdev->dev;
+
+ csi->mdev.dev = csi->dev;
+ strlcpy(csi->mdev.model, "Allwinner Video Capture Device",
+ sizeof(csi->mdev.model));
+ csi->mdev.hw_revision = 0;
+ media_device_init(&csi->mdev);
+
+ csi->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
+ ret = media_entity_pads_init(&csi->vdev.entity, 1, &csi->pad);
+ if (ret < 0)
+ return 0;
+
+ csi->has_isp = of_property_read_bool(pdev->dev.of_node,
+ "allwinner,has-isp");
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ csi->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(csi->regs))
+ return PTR_ERR(csi->regs);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ csi->ahb_clk = devm_clk_get(&pdev->dev, "ahb");
+ if (IS_ERR(csi->ahb_clk)) {
+ dev_err(&pdev->dev, "Couldn't get our ahb clock\n");
+ return PTR_ERR(csi->ahb_clk);
+ }
+
+ if (csi->has_isp) {
+ csi->isp_clk = devm_clk_get(&pdev->dev, "isp");
+ if (IS_ERR(csi->isp_clk)) {
+ dev_err(&pdev->dev, "Couldn't get our ISP clock\n");
+ return PTR_ERR(csi->isp_clk);
+ }
+ }
+
+ csi->mod_clk = devm_clk_get(&pdev->dev, "mod");
+ if (IS_ERR(csi->mod_clk)) {
+ dev_err(&pdev->dev, "Couldn't get our mod clock\n");
+ return PTR_ERR(csi->mod_clk);
+ }
+
+ csi->ram_clk = devm_clk_get(&pdev->dev, "ram");
+ if (IS_ERR(csi->ram_clk)) {
+ dev_err(&pdev->dev, "Couldn't get our ram clock\n");
+ return PTR_ERR(csi->ram_clk);
+ }
+
+ csi->rst = devm_reset_control_get(&pdev->dev, NULL);
+ if (IS_ERR(csi->rst)) {
+ dev_err(&pdev->dev, "Couldn't get our reset line\n");
+ return PTR_ERR(csi->rst);
+ }
+
+ ret = csi_dma_register(csi, irq);
+ if (ret)
+ return ret;
+
+ csi->v4l.mdev = &csi->mdev;
+
+ ret = media_device_register(&csi->mdev);
+ if (ret)
+ goto err_unregister_dma;
+
+ ret = v4l2_async_notifier_parse_fwnode_endpoints(csi->dev,
+ &csi->notifier,
+ sizeof(struct v4l2_async_subdev),
+ sun4i_csi_async_parse);
+ if (ret)
+ goto err_unregister_media;
+ csi->notifier.ops = &csi_notify_ops;
+
+ ret = v4l2_async_notifier_register(&csi->v4l, &csi->notifier);
+ if (ret) {
+ dev_err(csi->dev,
+ "Couldn't register our v4l2-async notifier\n");
+ goto err_free_notifier;
+ }
+
+ pm_runtime_enable(&pdev->dev);
+
+ return 0;
+
+err_free_notifier:
+ v4l2_async_notifier_cleanup(&csi->notifier);
+
+err_unregister_media:
+ media_device_unregister(&csi->mdev);
+
+err_unregister_dma:
+ csi_dma_unregister(csi);
+ return ret;
+}
+
+static int csi_remove(struct platform_device *pdev)
+{
+ struct sun4i_csi *csi = platform_get_drvdata(pdev);
+
+ v4l2_async_notifier_unregister(&csi->notifier);
+ v4l2_async_notifier_cleanup(&csi->notifier);
+ media_device_unregister(&csi->mdev);
+ csi_dma_unregister(csi);
+
+ return 0;
+}
+
+static const struct of_device_id csi_of_match[] = {
+ { .compatible = "allwinner,sun4i-a10-csi" },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, csi_of_match);
+
+static int csi_runtime_resume(struct device *dev)
+{
+ struct sun4i_csi *csi = dev_get_drvdata(dev);
+
+ reset_control_deassert(csi->rst);
+ clk_prepare_enable(csi->ahb_clk);
+ clk_prepare_enable(csi->ram_clk);
+
+ if (csi->has_isp) {
+ clk_set_rate(csi->isp_clk, 80000000);
+ clk_prepare_enable(csi->isp_clk);
+ }
+
+ clk_set_rate(csi->mod_clk, 24000000);
+ clk_prepare_enable(csi->mod_clk);
+
+ writel(1, csi->regs + CSI_EN_REG);
+
+ return 0;
+}
+
+static int csi_runtime_suspend(struct device *dev)
+{
+ struct sun4i_csi *csi = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(csi->mod_clk);
+
+ if (csi->has_isp)
+ clk_disable_unprepare(csi->isp_clk);
+
+ clk_disable_unprepare(csi->ram_clk);
+ clk_disable_unprepare(csi->ahb_clk);
+
+ reset_control_assert(csi->rst);
+
+ return 0;
+}
+
+static const struct dev_pm_ops csi_pm_ops = {
+ .runtime_resume = csi_runtime_resume,
+ .runtime_suspend = csi_runtime_suspend,
+};
+
+static struct platform_driver csi_driver = {
+ .probe = csi_probe,
+ .remove = csi_remove,
+ .driver = {
+ .name = "sun4i-csi",
+ .of_match_table = csi_of_match,
+ .pm = &csi_pm_ops,
+ },
+};
+module_platform_driver(csi_driver);
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
new file mode 100644
index 000000000000..80ced01567fa
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
@@ -0,0 +1,137 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2016 NextThing Co
+ * Copyright (C) 2016-2018 Bootlin
+ *
+ * Author: Maxime Ripard <maxime.ripard@bootlin.com>
+ */
+
+#ifndef _SUN4I_CSI_H_
+#define _SUN4I_CSI_H_
+
+#include <media/media-device.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/videobuf2-core.h>
+
+#define CSI_EN_REG 0x00
+
+#define CSI_CFG_REG 0x04
+#define CSI_CFG_INPUT_FMT(fmt) ((fmt) << 20)
+#define CSI_CFG_OUTPUT_FMT(fmt) ((fmt) << 16)
+#define CSI_CFG_YUV_DATA_SEQ(seq) ((seq) << 8)
+#define CSI_CFG_VSYNC_POL(pol) ((pol) << 2)
+#define CSI_CFG_HSYNC_POL(pol) ((pol) << 1)
+#define CSI_CFG_PCLK_POL(pol) ((pol) << 0)
+
+#define CSI_CPT_CTRL_REG 0x08
+#define CSI_CPT_CTRL_VIDEO_START BIT(1)
+#define CSI_CPT_CTRL_IMAGE_START BIT(0)
+
+#define CSI_BUF_ADDR_REG(fifo, buf) (0x10 + (0x8 * (fifo)) + (0x4 * (buf)))
+
+#define CSI_BUF_CTRL_REG 0x28
+#define CSI_BUF_CTRL_DBN BIT(2)
+#define CSI_BUF_CTRL_DBS BIT(1)
+#define CSI_BUF_CTRL_DBE BIT(0)
+
+#define CSI_INT_EN_REG 0x30
+#define CSI_INT_FRM_DONE BIT(1)
+#define CSI_INT_CPT_DONE BIT(0)
+
+#define CSI_INT_STA_REG 0x34
+
+#define CSI_WIN_CTRL_W_REG 0x40
+#define CSI_WIN_CTRL_W_ACTIVE(w) ((w) << 16)
+
+#define CSI_WIN_CTRL_H_REG 0x44
+#define CSI_WIN_CTRL_H_ACTIVE(h) ((h) << 16)
+
+#define CSI_BUF_LEN_REG 0x48
+
+#define CSI_MAX_BUFFER 2
+
+enum csi_input {
+ CSI_INPUT_RAW = 0,
+ CSI_INPUT_BT656 = 2,
+ CSI_INPUT_YUV = 3,
+};
+
+enum csi_output_raw {
+ CSI_OUTPUT_RAW_PASSTHROUGH = 0,
+};
+
+enum csi_output_yuv {
+ CSI_OUTPUT_YUV_422_PLANAR = 0,
+ CSI_OUTPUT_YUV_420_PLANAR = 1,
+ CSI_OUTPUT_YUV_422_UV = 4,
+ CSI_OUTPUT_YUV_420_UV = 5,
+ CSI_OUTPUT_YUV_422_MACRO = 8,
+ CSI_OUTPUT_YUV_420_MACRO = 9,
+};
+
+enum csi_yuv_data_seq {
+ CSI_YUV_DATA_SEQ_YUYV = 0,
+ CSI_YUV_DATA_SEQ_YVYU = 1,
+ CSI_YUV_DATA_SEQ_UYVY = 2,
+ CSI_YUV_DATA_SEQ_VYUY = 3,
+};
+
+struct sun4i_csi_format {
+ u32 mbus;
+ u32 fourcc;
+ enum csi_input input;
+ u32 output;
+ unsigned int num_planes;
+ u8 bpp[3];
+ unsigned int hsub;
+ unsigned int vsub;
+};
+
+struct sun4i_csi {
+ // Device resources
+ struct device *dev;
+
+ void __iomem *regs;
+ struct clk *ahb_clk;
+ struct clk *isp_clk;
+ struct clk *mod_clk;
+ struct clk *ram_clk;
+ struct reset_control *rst;
+ unsigned char has_isp;
+
+ const struct sun4i_csi_format *p_fmt;
+ struct v4l2_pix_format_mplane v_fmt;
+
+ struct vb2_v4l2_buffer *current_buf[CSI_MAX_BUFFER];
+
+ struct media_device mdev;
+ struct media_pad pad;
+
+ struct v4l2_fwnode_bus_parallel bus;
+
+ // V4L2 Async variables
+ struct v4l2_async_notifier notifier;
+ struct v4l2_subdev *src_subdev;
+ unsigned int src_pad;
+
+ // V4L2 variables
+ struct mutex lock;
+ struct v4l2_device v4l;
+ struct video_device vdev;
+
+ // Videobuf2
+ struct vb2_queue queue;
+ struct list_head buf_list;
+ spinlock_t qlock;
+ unsigned int sequence;
+};
+
+int csi_dma_register(struct sun4i_csi *csi, int irq);
+void csi_dma_unregister(struct sun4i_csi *csi);
+
+int csi_v4l2_register(struct sun4i_csi *csi);
+
+#endif /* _SUN4I_CSI_H_ */
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
new file mode 100644
index 000000000000..989b46968e87
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
@@ -0,0 +1,383 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2016 NextThing Co
+ * Copyright (C) 2016-2018 Bootlin
+ *
+ * Author: Maxime Ripard <maxime.ripard@bootlin.com>
+ */
+
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+#include <media/videobuf2-dma-contig.h>
+#include <media/videobuf2-v4l2.h>
+
+#include "sun4i_csi.h"
+
+struct csi_buffer {
+ struct vb2_v4l2_buffer vb;
+ struct list_head list;
+};
+
+static inline struct csi_buffer *vb2_v4l2_to_csi_buffer(const struct vb2_v4l2_buffer *p)
+{
+ return container_of(p, struct csi_buffer, vb);
+}
+
+static inline struct csi_buffer *vb2_to_csi_buffer(const struct vb2_buffer *p)
+{
+ return vb2_v4l2_to_csi_buffer(to_vb2_v4l2_buffer(p));
+}
+
+static void csi_capture_start(struct sun4i_csi *csi)
+{
+ writel(CSI_CPT_CTRL_VIDEO_START, csi->regs + CSI_CPT_CTRL_REG);
+}
+
+static void csi_capture_stop(struct sun4i_csi *csi)
+{
+ writel(0, csi->regs + CSI_CPT_CTRL_REG);
+}
+
+static int csi_queue_setup(struct vb2_queue *vq,
+ unsigned int *nbuffers,
+ unsigned int *nplanes,
+ unsigned int sizes[],
+ struct device *alloc_devs[])
+
+{
+ struct sun4i_csi *csi = vb2_get_drv_priv(vq);
+ unsigned int i;
+
+ if (*nbuffers < 3)
+ *nbuffers = 3;
+
+ *nplanes = csi->p_fmt->num_planes;
+
+ for (i = 0; i < *nplanes; i++)
+ sizes[i] = csi->v_fmt.plane_fmt[i].sizeimage;
+
+ return 0;
+};
+
+static int csi_buffer_prepare(struct vb2_buffer *vb)
+{
+ struct sun4i_csi *csi = vb2_get_drv_priv(vb->vb2_queue);
+ unsigned int i;
+
+ for (i = 0; i < csi->p_fmt->num_planes; i++) {
+ unsigned long size = csi->v_fmt.plane_fmt[i].sizeimage;
+
+ if (vb2_plane_size(vb, i) < size) {
+ dev_err(csi->dev, "buffer too small (%lu < %lu)\n",
+ vb2_plane_size(vb, i), size);
+ return -EINVAL;
+ }
+
+ vb2_set_plane_payload(vb, i, size);
+ }
+
+ return 0;
+}
+
+static int csi_buffer_fill_slot(struct sun4i_csi *csi, unsigned int slot)
+{
+ struct csi_buffer *c_buf;
+ struct vb2_v4l2_buffer *v_buf;
+ unsigned int plane;
+
+ /*
+ * We should never end up in a situation where we overwrite an
+ * already filled slot.
+ */
+ if (WARN_ON(csi->current_buf[slot]))
+ return -EINVAL;
+
+ if (list_empty(&csi->buf_list)) {
+ csi->current_buf[slot] = NULL;
+
+ dev_warn(csi->dev, "Running out of buffers...\n");
+ return -ENOMEM;
+ }
+
+ c_buf = list_first_entry(&csi->buf_list, struct csi_buffer, list);
+ list_del_init(&c_buf->list);
+
+ v_buf = &c_buf->vb;
+ csi->current_buf[slot] = v_buf;
+
+ for (plane = 0; plane < csi->p_fmt->num_planes; plane++) {
+ dma_addr_t buf_addr;
+
+ buf_addr = vb2_dma_contig_plane_dma_addr(&v_buf->vb2_buf,
+ plane);
+ writel(buf_addr, csi->regs + CSI_BUF_ADDR_REG(plane, slot));
+ }
+
+ return 0;
+}
+
+static int csi_buffer_fill_all(struct sun4i_csi *csi)
+{
+ unsigned int slot;
+ int ret;
+
+ for (slot = 0; slot < CSI_MAX_BUFFER; slot++) {
+ ret = csi_buffer_fill_slot(csi, slot);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static void csi_buffer_mark_done(struct sun4i_csi *csi,
+ unsigned int slot,
+ unsigned int sequence)
+{
+ struct vb2_v4l2_buffer *v_buf;
+
+ if (WARN_ON(!csi->current_buf[slot]))
+ return;
+
+ v_buf = csi->current_buf[slot];
+ v_buf->field = csi->v_fmt.field;
+ v_buf->sequence = sequence;
+ v_buf->vb2_buf.timestamp = ktime_get_ns();
+ vb2_buffer_done(&v_buf->vb2_buf, VB2_BUF_STATE_DONE);
+
+ csi->current_buf[slot] = NULL;
+}
+
+static int csi_buffer_flip(struct sun4i_csi *csi, unsigned int sequence)
+{
+ u32 reg = readl(csi->regs + CSI_BUF_CTRL_REG);
+ int curr, next;
+
+ /* Our next buffer is not the current buffer */
+ curr = !!(reg & CSI_BUF_CTRL_DBS);
+ next = !curr;
+
+ /* Report the previous buffer as done */
+ csi_buffer_mark_done(csi, next, sequence);
+
+ /* Put a new buffer in there */
+ return csi_buffer_fill_slot(csi, next);
+}
+
+static void csi_buffer_queue(struct vb2_buffer *vb)
+{
+ struct sun4i_csi *csi = vb2_get_drv_priv(vb->vb2_queue);
+ struct csi_buffer *buf = vb2_to_csi_buffer(vb);
+ unsigned long flags;
+
+ spin_lock_irqsave(&csi->qlock, flags);
+ list_add_tail(&buf->list, &csi->buf_list);
+ spin_unlock_irqrestore(&csi->qlock, flags);
+}
+
+static void return_all_buffers(struct sun4i_csi *csi,
+ enum vb2_buffer_state state)
+{
+ struct csi_buffer *buf, *node;
+ unsigned int slot;
+
+ list_for_each_entry_safe(buf, node, &csi->buf_list, list) {
+ vb2_buffer_done(&buf->vb.vb2_buf, state);
+ list_del(&buf->list);
+ }
+
+ for (slot = 0; slot < CSI_MAX_BUFFER; slot++) {
+ struct vb2_v4l2_buffer *v_buf = csi->current_buf[slot];
+
+ if (!v_buf)
+ continue;
+
+ vb2_buffer_done(&v_buf->vb2_buf, state);
+ csi->current_buf[slot] = NULL;
+ }
+}
+
+static int csi_start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+ struct sun4i_csi *csi = vb2_get_drv_priv(vq);
+ struct v4l2_fwnode_bus_parallel *bus = &csi->bus;
+ unsigned long hsync_pol, pclk_pol, vsync_pol;
+ unsigned long flags;
+ int ret = 0;
+
+ csi->sequence = 0;
+
+ if (count < 2)
+ return -ENOBUFS;
+
+ ret = media_pipeline_start(&csi->vdev.entity, &csi->vdev.pipe);
+ if (ret < 0)
+ goto clear_dma_queue;
+
+ dev_dbg(csi->dev, "Starting capture\n");
+
+ spin_lock_irqsave(&csi->qlock, flags);
+
+ /* Setup timings */
+ writel(CSI_WIN_CTRL_W_ACTIVE(csi->v_fmt.width * 2),
+ csi->regs + CSI_WIN_CTRL_W_REG);
+ writel(CSI_WIN_CTRL_H_ACTIVE(csi->v_fmt.height),
+ csi->regs + CSI_WIN_CTRL_H_REG);
+
+ hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
+ pclk_pol = !!(bus->flags & V4L2_MBUS_DATA_ACTIVE_HIGH);
+ vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
+ writel(CSI_CFG_INPUT_FMT(csi->p_fmt->input) |
+ CSI_CFG_OUTPUT_FMT(csi->p_fmt->output) |
+ CSI_CFG_VSYNC_POL(vsync_pol) |
+ CSI_CFG_HSYNC_POL(hsync_pol) |
+ CSI_CFG_PCLK_POL(pclk_pol),
+ csi->regs + CSI_CFG_REG);
+
+ /* Setup buffer length */
+ writel(csi->v_fmt.plane_fmt[0].bytesperline,
+ csi->regs + CSI_BUF_LEN_REG);
+
+ /* Prepare our buffers in hardware */
+ ret = csi_buffer_fill_all(csi);
+ if (ret) {
+ spin_unlock_irqrestore(&csi->qlock, flags);
+ goto disable_pipeline;
+ }
+
+ /* Enable double buffering */
+ writel(CSI_BUF_CTRL_DBE, csi->regs + CSI_BUF_CTRL_REG);
+
+ /* Clear the pending interrupts */
+ writel(CSI_INT_FRM_DONE, csi->regs + 0x34);
+
+ /* Enable frame done interrupt */
+ writel(CSI_INT_FRM_DONE, csi->regs + CSI_INT_EN_REG);
+
+ csi_capture_start(csi);
+
+ spin_unlock_irqrestore(&csi->qlock, flags);
+
+ ret = v4l2_subdev_call(csi->src_subdev, video, s_stream, 1);
+ if (ret < 0 && ret != -ENOIOCTLCMD)
+ goto disable_pipeline;
+
+ return 0;
+
+disable_pipeline:
+ media_pipeline_stop(&csi->vdev.entity);
+
+clear_dma_queue:
+ return_all_buffers(csi, VB2_BUF_STATE_QUEUED);
+
+ return ret;
+}
+
+static void csi_stop_streaming(struct vb2_queue *vq)
+{
+ struct sun4i_csi *csi = vb2_get_drv_priv(vq);
+ unsigned long flags;
+
+ dev_dbg(csi->dev, "Stopping capture\n");
+
+ v4l2_subdev_call(csi->src_subdev, video, s_stream, 0);
+ csi_capture_stop(csi);
+
+ /* Release all active buffers */
+ spin_lock_irqsave(&csi->qlock, flags);
+ return_all_buffers(csi, VB2_BUF_STATE_ERROR);
+ spin_unlock_irqrestore(&csi->qlock, flags);
+
+ media_pipeline_stop(&csi->vdev.entity);
+}
+
+static struct vb2_ops csi_qops = {
+ .queue_setup = csi_queue_setup,
+ .buf_prepare = csi_buffer_prepare,
+ .buf_queue = csi_buffer_queue,
+ .start_streaming = csi_start_streaming,
+ .stop_streaming = csi_stop_streaming,
+ .wait_prepare = vb2_ops_wait_prepare,
+ .wait_finish = vb2_ops_wait_finish,
+};
+
+static irqreturn_t csi_irq(int irq, void *data)
+{
+ struct sun4i_csi *csi = data;
+ unsigned int sequence;
+ u32 reg;
+
+ reg = readl(csi->regs + CSI_INT_STA_REG);
+
+ /* Acknowledge the interrupts */
+ writel(reg, csi->regs + CSI_INT_STA_REG);
+
+ sequence = csi->sequence++;
+
+ if (!(reg & CSI_INT_FRM_DONE))
+ goto out;
+
+ spin_lock(&csi->qlock);
+ if (csi_buffer_flip(csi, sequence)) {
+ dev_warn(csi->dev, "%s: Flip failed\n", __func__);
+ csi_capture_stop(csi);
+ }
+ spin_unlock(&csi->qlock);
+
+out:
+ return IRQ_HANDLED;
+}
+
+int csi_dma_register(struct sun4i_csi *csi, int irq)
+{
+ struct vb2_queue *q = &csi->queue;
+ int ret;
+ int i;
+
+ ret = v4l2_device_register(csi->dev, &csi->v4l);
+ if (ret) {
+ dev_err(csi->dev, "Couldn't register the v4l2 device\n");
+ return ret;
+ }
+
+ spin_lock_init(&csi->qlock);
+ mutex_init(&csi->lock);
+
+ INIT_LIST_HEAD(&csi->buf_list);
+ for (i = 0; i < CSI_MAX_BUFFER; i++)
+ csi->current_buf[i] = NULL;
+
+ q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+ q->io_modes = VB2_MMAP;
+ q->lock = &csi->lock;
+ q->drv_priv = csi;
+ q->buf_struct_size = sizeof(struct csi_buffer);
+ q->ops = &csi_qops;
+ q->mem_ops = &vb2_dma_contig_memops;
+ q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+ q->gfp_flags = GFP_DMA32;
+ q->dev = csi->dev;
+
+ ret = vb2_queue_init(q);
+ if (ret < 0) {
+ dev_err(csi->dev, "failed to initialize VB2 queue\n");
+ return ret;
+ }
+
+ ret = devm_request_irq(csi->dev, irq, csi_irq, 0,
+ dev_name(csi->dev), csi);
+ if (ret) {
+ dev_err(csi->dev, "Couldn't register our interrupt\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+void csi_dma_unregister(struct sun4i_csi *csi)
+{
+ v4l2_device_unregister(&csi->v4l);
+}
+
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
new file mode 100644
index 000000000000..0b9487830e6d
--- /dev/null
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
@@ -0,0 +1,287 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2016 NextThing Co
+ * Copyright (C) 2016-2018 Bootlin
+ *
+ * Author: Maxime Ripard <maxime.ripard@bootlin.com>
+ */
+
+#include <linux/device.h>
+#include <linux/pm_runtime.h>
+
+#include <media/v4l2-ioctl.h>
+#include <media/videobuf2-v4l2.h>
+
+#include "sun4i_csi.h"
+
+#define CSI_DEFAULT_FORMAT V4L2_PIX_FMT_YUV420M
+#define CSI_DEFAULT_WIDTH 640
+#define CSI_DEFAULT_HEIGHT 480
+
+#define CSI_MAX_HEIGHT 8192U
+#define CSI_MAX_WIDTH 8192U
+
+static const struct sun4i_csi_format csi_formats[] = {
+ /* YUV422 inputs */
+ {
+ .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
+ .fourcc = V4L2_PIX_FMT_YUV420M,
+ .input = CSI_INPUT_YUV,
+ .output = CSI_OUTPUT_YUV_420_PLANAR,
+ .num_planes = 3,
+ .bpp = { 8, 8, 8 },
+ .hsub = 2,
+ .vsub = 2,
+ },
+};
+
+static const struct sun4i_csi_format *
+csi_get_format_by_fourcc(struct sun4i_csi *csi, u32 fourcc)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(csi_formats); i++)
+ if (csi_formats[i].fourcc == fourcc)
+ return &csi_formats[i];
+
+ return NULL;
+}
+
+static int csi_querycap(struct file *file, void *priv,
+ struct v4l2_capability *cap)
+{
+ struct sun4i_csi *csi = video_drvdata(file);
+
+ strscpy(cap->driver, KBUILD_MODNAME, sizeof(cap->driver));
+ strscpy(cap->card, "sun4i-csi", sizeof(cap->card));
+ snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
+ dev_name(csi->dev));
+ cap->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING;
+ cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
+
+ return 0;
+}
+
+static int csi_enum_input(struct file *file, void *priv,
+ struct v4l2_input *inp)
+{
+ if (inp->index != 0)
+ return -EINVAL;
+
+ inp->type = V4L2_INPUT_TYPE_CAMERA;
+ strlcpy(inp->name, "Camera", sizeof(inp->name));
+
+ return 0;
+}
+
+static int csi_g_input(struct file *file, void *fh,
+ unsigned int *i)
+{
+ *i = 0;
+
+ return 0;
+}
+
+static int csi_s_input(struct file *file, void *fh,
+ unsigned int i)
+{
+ if (i != 0)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int _csi_try_fmt(struct sun4i_csi *csi,
+ struct v4l2_pix_format_mplane *pix,
+ const struct sun4i_csi_format **fmt)
+{
+ const struct sun4i_csi_format *_fmt;
+ unsigned int width = pix->width;
+ unsigned int height = pix->height;
+ int i;
+
+ _fmt = csi_get_format_by_fourcc(csi, pix->pixelformat);
+ if (!_fmt)
+ _fmt = csi_get_format_by_fourcc(csi, csi_formats[0].fourcc);
+
+ pix->field = V4L2_FIELD_NONE;
+ pix->colorspace = V4L2_COLORSPACE_SRGB;
+ pix->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(pix->colorspace);
+ pix->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
+ pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true, pix->colorspace,
+ pix->ycbcr_enc);
+
+ pix->num_planes = _fmt->num_planes;
+ pix->pixelformat = _fmt->fourcc;
+
+ memset(pix->reserved, 0, sizeof(pix->reserved));
+
+ /* Align the width and height on the subsampling */
+ width = ALIGN(width, _fmt->hsub);
+ height = ALIGN(height, _fmt->vsub);
+
+ /* Clamp the width and height to our capabilities */
+ pix->width = clamp(width, _fmt->hsub, CSI_MAX_WIDTH);
+ pix->height = clamp(height, _fmt->vsub, CSI_MAX_HEIGHT);
+
+ for (i = 0; i < _fmt->num_planes; i++) {
+ unsigned int hsub = i > 0 ? _fmt->hsub : 1;
+ unsigned int vsub = i > 0 ? _fmt->vsub : 1;
+ unsigned int bpl;
+
+ bpl = pix->width / hsub * _fmt->bpp[i] / 8;
+ pix->plane_fmt[i].bytesperline = bpl;
+ pix->plane_fmt[i].sizeimage = bpl * pix->height / vsub;
+ memset(pix->plane_fmt[i].reserved, 0,
+ sizeof(pix->plane_fmt[i].reserved));
+ }
+
+ if (fmt)
+ *fmt = _fmt;
+
+ return 0;
+}
+
+static int csi_try_fmt_vid_cap(struct file *file, void *priv,
+ struct v4l2_format *f)
+{
+ struct sun4i_csi *csi = video_drvdata(file);
+
+ if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+ return -EINVAL;
+
+ return _csi_try_fmt(csi, &f->fmt.pix_mp, NULL);
+}
+
+static int csi_s_fmt_vid_cap(struct file *file, void *priv,
+ struct v4l2_format *f)
+{
+ const struct sun4i_csi_format *fmt;
+ struct sun4i_csi *csi = video_drvdata(file);
+ int ret;
+
+ if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+ return -EINVAL;
+
+ ret = _csi_try_fmt(csi, &f->fmt.pix_mp, &fmt);
+ if (ret)
+ return ret;
+
+ csi->v_fmt = f->fmt.pix_mp;
+ csi->p_fmt = fmt;
+
+ return 0;
+}
+
+static int csi_g_fmt_vid_cap(struct file *file, void *priv,
+ struct v4l2_format *f)
+{
+ struct sun4i_csi *csi = video_drvdata(file);
+
+ if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+ return -EINVAL;
+
+ f->fmt.pix_mp = csi->v_fmt;
+
+ return 0;
+}
+
+static int csi_enum_fmt_vid_cap(struct file *file, void *priv,
+ struct v4l2_fmtdesc *f)
+{
+ if (f->index >= ARRAY_SIZE(csi_formats))
+ return -EINVAL;
+
+ f->pixelformat = csi_formats[f->index].fourcc;
+
+ return 0;
+}
+
+static const struct v4l2_ioctl_ops csi_ioctl_ops = {
+ .vidioc_querycap = csi_querycap,
+
+ .vidioc_enum_fmt_vid_cap_mplane = csi_enum_fmt_vid_cap,
+ .vidioc_g_fmt_vid_cap_mplane = csi_g_fmt_vid_cap,
+ .vidioc_s_fmt_vid_cap_mplane = csi_s_fmt_vid_cap,
+ .vidioc_try_fmt_vid_cap_mplane = csi_try_fmt_vid_cap,
+
+ .vidioc_enum_input = csi_enum_input,
+ .vidioc_g_input = csi_g_input,
+ .vidioc_s_input = csi_s_input,
+
+ .vidioc_reqbufs = vb2_ioctl_reqbufs,
+ .vidioc_create_bufs = vb2_ioctl_create_bufs,
+ .vidioc_querybuf = vb2_ioctl_querybuf,
+ .vidioc_qbuf = vb2_ioctl_qbuf,
+ .vidioc_dqbuf = vb2_ioctl_dqbuf,
+ .vidioc_expbuf = vb2_ioctl_expbuf,
+ .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
+ .vidioc_streamon = vb2_ioctl_streamon,
+ .vidioc_streamoff = vb2_ioctl_streamoff,
+};
+
+static int csi_open(struct file *file)
+{
+ struct sun4i_csi *csi = video_drvdata(file);
+ int ret;
+
+ pm_runtime_get_sync(csi->dev);
+
+ ret = v4l2_subdev_call(csi->src_subdev, core, s_power, 1);
+ if (ret < 0 && ret != -ENOIOCTLCMD)
+ return ret;
+
+ return v4l2_fh_open(file);
+}
+
+static int csi_release(struct file *file)
+{
+ struct sun4i_csi *csi = video_drvdata(file);
+
+ pm_runtime_put(csi->dev);
+
+ return vb2_fop_release(file);
+}
+
+static const struct v4l2_file_operations csi_fops = {
+ .owner = THIS_MODULE,
+ .open = csi_open,
+ .release = csi_release,
+ .unlocked_ioctl = video_ioctl2,
+ .read = vb2_fop_read,
+ .write = vb2_fop_write,
+ .poll = vb2_fop_poll,
+ .mmap = vb2_fop_mmap,
+};
+
+int csi_v4l2_register(struct sun4i_csi *csi)
+{
+ struct video_device *vdev = &csi->vdev;
+ int ret;
+
+ vdev->v4l2_dev = &csi->v4l;
+ vdev->queue = &csi->queue;
+ strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
+ vdev->release = video_device_release_empty;
+ vdev->lock = &csi->lock;
+
+ /* Set a default format */
+ csi->v_fmt.pixelformat = CSI_DEFAULT_FORMAT;
+ csi->v_fmt.width = CSI_DEFAULT_WIDTH;
+ csi->v_fmt.height = CSI_DEFAULT_HEIGHT;
+ _csi_try_fmt(csi, &csi->v_fmt, NULL);
+
+ vdev->fops = &csi_fops;
+ vdev->ioctl_ops = &csi_ioctl_ops;
+ video_set_drvdata(vdev, csi);
+
+ ret = video_register_device(&csi->vdev, VFL_TYPE_GRABBER, -1);
+ if (ret)
+ return ret;
+
+ dev_info(csi->dev, "Device registered as %s\n",
+ video_device_node_name(vdev));
+
+ return 0;
+}
+
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 4/5] ARM: dts: sun7i: Add CSI0 controller
2018-11-13 8:24 [PATCH 0/5] media: Allwinner A10 CSI support Maxime Ripard
` (2 preceding siblings ...)
2018-11-13 8:24 ` [PATCH 3/5] media: sunxi: Add A10 CSI driver Maxime Ripard
@ 2018-11-13 8:24 ` Maxime Ripard
2018-11-13 8:24 ` [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support Maxime Ripard
` (2 subsequent siblings)
6 siblings, 0 replies; 36+ messages in thread
From: Maxime Ripard @ 2018-11-13 8:24 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand, Maxime Ripard
The CSI controller embedded in the A20 can be supported by our new driver.
Let's add it to our DT.
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
arch/arm/boot/dts/sun7i-a20.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 9c52712af241..50a9e30ee18c 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -364,6 +364,19 @@
num-cs = <1>;
};
+ csi0: csi@1c09000 {
+ compatible = "allwinner,sun7i-a20-csi",
+ "allwinner,sun4i-a10-csi";
+ reg = <0x01c09000 0x1000>;
+ interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
+ <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
+ clock-names = "ahb", "mod", "isp", "ram";
+ resets = <&ccu RST_CSI0>;
+ allwinner,csi-channels = <4>;
+ allwinner,has-isp;
+ };
+
emac: ethernet@1c0b000 {
compatible = "allwinner,sun4i-a10-emac";
reg = <0x01c0b000 0x1000>;
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
2018-11-13 8:24 [PATCH 0/5] media: Allwinner A10 CSI support Maxime Ripard
` (3 preceding siblings ...)
2018-11-13 8:24 ` [PATCH 4/5] ARM: dts: sun7i: Add CSI0 controller Maxime Ripard
@ 2018-11-13 8:24 ` Maxime Ripard
2018-11-27 6:56 ` Jagan Teki
2018-11-13 12:30 ` [PATCH 0/5] media: Allwinner A10 CSI support Hans Verkuil
2018-11-14 3:24 ` Chen-Yu Tsai
6 siblings, 1 reply; 36+ messages in thread
From: Maxime Ripard @ 2018-11-13 8:24 UTC (permalink / raw)
To: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand, Maxime Ripard
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
arch/arm/boot/dts/sun7i-a20-bananapi.dts | 98 +++++++++++++++++++++++++-
1 file changed, 98 insertions(+)
diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
index 70dfc4ac0bb5..18dbff9f1ce9 100644
--- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
+++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
@@ -54,6 +54,9 @@
compatible = "lemaker,bananapi", "allwinner,sun7i-a20";
aliases {
+ i2c0 = &i2c0;
+ i2c1 = &i2c1;
+ i2c2 = &i2c2;
serial0 = &uart0;
serial1 = &uart3;
serial2 = &uart7;
@@ -63,6 +66,41 @@
stdout-path = "serial0:115200n8";
};
+ reg_cam: cam {
+ compatible = "regulator-fixed";
+ regulator-name = "cam";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ vin-supply = <®_vcc5v0>;
+ gpio = <&pio 7 16 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ regulator-always-on;
+ };
+
+ reg_cam_avdd: cam-avdd {
+ compatible = "regulator-fixed";
+ regulator-name = "cam500b-avdd";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ vin-supply = <®_cam>;
+ };
+
+ reg_cam_dovdd: cam-dovdd {
+ compatible = "regulator-fixed";
+ regulator-name = "cam500b-dovdd";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ vin-supply = <®_cam>;
+ };
+
+ reg_cam_dvdd: cam-dvdd {
+ compatible = "regulator-fixed";
+ regulator-name = "cam500b-dvdd";
+ regulator-min-microvolt = <1500000>;
+ regulator-max-microvolt = <1500000>;
+ vin-supply = <®_cam>;
+ };
+
hdmi-connector {
compatible = "hdmi-connector";
type = "a";
@@ -120,6 +158,27 @@
>;
};
+&csi0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&csi0_pins_a>;
+ status = "okay";
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ csi_from_ov5640: endpoint {
+ remote-endpoint = <&ov5640_to_csi>;
+ bus-width = <8>;
+ data-shift = <2>;
+ hsync-active = <1>; /* Active high */
+ vsync-active = <0>; /* Active low */
+ data-active = <1>; /* Active high */
+ pclk-sample = <1>; /* Rising */
+ };
+ };
+};
+
&de {
status = "okay";
};
@@ -167,6 +226,39 @@
};
};
+&i2c1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c1_pins_a>;
+ status = "okay";
+
+ camera: camera@21 {
+ compatible = "ovti,ov5640";
+ reg = <0x21>;
+ clocks = <&ccu CLK_CSI0>;
+ clock-names = "xclk";
+ assigned-clocks = <&ccu CLK_CSI0>;
+ assigned-clock-rates = <24000000>;
+
+ reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>;
+ powerdown-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>;
+ AVDD-supply = <®_cam_avdd>;
+ DOVDD-supply = <®_cam_dovdd>;
+ DVDD-supply = <®_cam_dvdd>;
+
+ port {
+ ov5640_to_csi: endpoint {
+ remote-endpoint = <&csi_from_ov5640>;
+ bus-width = <8>;
+ data-shift = <2>;
+ hsync-active = <1>; /* Active high */
+ vsync-active = <0>; /* Active low */
+ data-active = <1>; /* Active high */
+ pclk-sample = <1>; /* Rising */
+ };
+ };
+ };
+};
+
&i2c2 {
pinctrl-names = "default";
pinctrl-0 = <&i2c2_pins_a>;
@@ -252,6 +344,12 @@
"IO-6", "IO-3", "IO-2", "IO-0", "", "", "", "",
"", "", "", "", "", "", "", "";
+ csi0_pins_a: csi_pins_a@0 {
+ pins = "PE0", "PE1", "PE2", "PE3", "PE4", "PE5",
+ "PE6", "PE7", "PE8", "PE9", "PE10", "PE11";
+ function = "csi0";
+ };
+
usb0_id_detect_pin: usb0_id_detect_pin@0 {
pins = "PH4";
function = "gpio_in";
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding
2018-11-13 8:24 ` [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding Maxime Ripard
@ 2018-11-13 8:38 ` Sakari Ailus
2018-11-15 19:04 ` Maxime Ripard
2018-11-13 10:34 ` Sakari Ailus
1 sibling, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2018-11-13 8:38 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Mauro Carvalho Chehab, Thomas Petazzoni,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
Hi Maxime,
On Tue, Nov 13, 2018 at 09:24:13AM +0100, Maxime Ripard wrote:
> The Allwinner A10 CMOS Sensor Interface is a camera capture interface also
> used in later (A10s, A13, A20, R8 and GR8) SoCs.
>
> On some SoCs, like the A10, there's multiple instances of that controller,
> with one instance supporting more channels and having an ISP.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
> Documentation/devicetree/bindings/media/sun4i-csi.txt | 71 ++++++++++++-
> 1 file changed, 71 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/sun4i-csi.txt
>
> diff --git a/Documentation/devicetree/bindings/media/sun4i-csi.txt b/Documentation/devicetree/bindings/media/sun4i-csi.txt
> new file mode 100644
> index 000000000000..3d96bcbef9d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/sun4i-csi.txt
> @@ -0,0 +1,71 @@
> +Allwinner A10 CMOS Sensor Interface
> +-------------------------------------
> +
> +The Allwinner A10 SoC features two camera capture interfaces, one
> +featuring an ISP and the other without. Later SoCs built upon that
> +design and used similar SoCs.
> +
> +Required properties:
> + - compatible: value must be one of:
> + * allwinner,sun4i-a10-csi
> + * allwinner,sun5i-a13-csi, allwinner,sun4i-a10-csi
> + * allwinner,sun7i-a20-csi, allwinner,sun4i-a10-csi
> + - reg: base address and size of the memory-mapped region.
> + - interrupts: interrupt associated to this IP
> + - clocks: phandles to the clocks feeding the CSI
> + * ahb: the CSI interface clock
> + * mod: the CSI module clock
> + * ram: the CSI DRAM clock
> + - clock-names: the clock names mentioned above
> + - resets: phandles to the reset line driving the CSI
> +
> +Optional properties:
> + - allwinner,csi-channels: Number of channels available in the CSI
> + controller. If not present, the default
> + will be 1.
> + - allwinner,has-isp: Whether the CSI controller has an ISP
> + associated to it or not
Is the ISP a part of the same device? It sounds like that this is actually
a different device if it contains an ISP as well, and that should be
apparent from the compatible string. What do you think?
> +
> +If allwinner,has-isp is set, an additional "isp" clock is needed,
> +being a phandle to the clock driving the ISP.
> +
> +The CSI node should contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +endpoint's bus type must be parallel or BT656.
> +
> +Endpoint node properties for CSI
> +---------------------------------
> +
> +- remote-endpoint : (required) a phandle to the bus receiver's endpoint
> + node
Rob's opinion has been (AFAIU) that this is not needed as it's already a
part of the graph bindings. Unless you want to say that it's required, that
is --- the graph bindings document it as optional.
> +- bus-width: : (required) must be 8
If this is the only value the hardware supports, I don't see why you should
specify it here.
> +- pclk-sample : (optional) (default: sample on falling edge)
> +- hsync-active : (only required for parallel)
> +- vsync-active : (only required for parallel)
> +
> +Example:
> +
> +csi0: csi@1c09000 {
> + compatible = "allwinner,sun7i-a20-csi",
> + "allwinner,sun4i-a10-csi";
> + reg = <0x01c09000 0x1000>;
> + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
> + <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
> + clock-names = "ahb", "mod", "isp", "ram";
> + resets = <&ccu RST_CSI0>;
> + allwinner,csi-channels = <4>;
> + allwinner,has-isp;
> +
> + port {
> + csi_from_ov5640: endpoint {
> + remote-endpoint = <&ov5640_to_csi>;
> + bus-width = <8>;
> + data-shift = <2>;
data-shift needs to be documented above if it's relevant for the device.
> + hsync-active = <1>; /* Active high */
> + vsync-active = <0>; /* Active low */
> + pclk-sample = <1>; /* Rising */
> + };
> + };
> +};
--
Kind regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 8:24 ` [PATCH 3/5] media: sunxi: Add A10 CSI driver Maxime Ripard
@ 2018-11-13 8:57 ` Sakari Ailus
2018-11-13 12:24 ` Hans Verkuil
2018-11-13 12:48 ` Fabio Estevam
2 siblings, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2018-11-13 8:57 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Mauro Carvalho Chehab, Thomas Petazzoni,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
Hi Maxime,
On Tue, Nov 13, 2018 at 09:24:15AM +0100, Maxime Ripard wrote:
> The older CSI drivers have camera capture interface different from the one
> in the newer ones.
>
> This IP is pretty simple. Some variants (one controller out of two
> instances on some SoCs) have an ISP embedded, but there's no code that make
> use of it, so we ignored that part for now.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
> drivers/media/platform/sunxi/Kconfig | 1 +-
> drivers/media/platform/sunxi/Makefile | 1 +-
> drivers/media/platform/sunxi/sun4i-csi/Kconfig | 12 +-
> drivers/media/platform/sunxi/sun4i-csi/Makefile | 5 +-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 275 +++++++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h | 137 +++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 383 +++++++++++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c | 287 ++++++++++-
> 8 files changed, 1101 insertions(+)
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Kconfig
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Makefile
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
>
> diff --git a/drivers/media/platform/sunxi/Kconfig b/drivers/media/platform/sunxi/Kconfig
> index 1b6e89cb78b2..71808e93ac2e 100644
> --- a/drivers/media/platform/sunxi/Kconfig
> +++ b/drivers/media/platform/sunxi/Kconfig
> @@ -1 +1,2 @@
> +source "drivers/media/platform/sunxi/sun4i-csi/Kconfig"
> source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
> diff --git a/drivers/media/platform/sunxi/Makefile b/drivers/media/platform/sunxi/Makefile
> index 8d06f98500ee..a05127529006 100644
> --- a/drivers/media/platform/sunxi/Makefile
> +++ b/drivers/media/platform/sunxi/Makefile
> @@ -1 +1,2 @@
> +obj-y += sun4i-csi/
> obj-y += sun6i-csi/
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/Kconfig b/drivers/media/platform/sunxi/sun4i-csi/Kconfig
> new file mode 100644
> index 000000000000..841a6f4d9c99
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/Kconfig
> @@ -0,0 +1,12 @@
> +config VIDEO_SUN4I_CSI
> + tristate "Allwinner A10 CMOS Sensor Interface Support"
> + depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
> + depends on ARCH_SUNXI || COMPILE_TEST
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_FWNODE
> + select V4L2_MEM2MEM_DEV
> + help
> + This is a V4L2 driver for the Allwinner A10 CSI
> +
> + To compile this driver as a module, choose M here: the module
> + will be called sun4i_csi.
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/Makefile b/drivers/media/platform/sunxi/sun4i-csi/Makefile
> new file mode 100644
> index 000000000000..7c790a57f5ee
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/Makefile
> @@ -0,0 +1,5 @@
> +sun4i-csi-y += sun4i_csi.o
> +sun4i-csi-y += sun4i_dma.o
> +sun4i-csi-y += sun4i_v4l2.o
> +
> +obj-$(CONFIG_VIDEO_SUN4I_CSI) += sun4i-csi.o
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> new file mode 100644
> index 000000000000..3e0a65ccbbe2
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2016 NextThing Co
> + * Copyright (C) 2016-2018 Bootlin
> + *
> + * Author: Maxime Ripard <maxime.ripard@bootlin.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mediabus.h>
> +
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +#include "sun4i_csi.h"
> +
> +static int csi_notify_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi,
> + notifier);
> +
> + csi->src_subdev = subdev;
> + csi->src_pad = media_entity_get_fwnode_pad(&subdev->entity,
> + subdev->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (csi->src_pad < 0) {
> + dev_err(csi->dev, "Couldn't find output pad for subdev %s\n",
> + subdev->name);
> + return csi->src_pad;
> + }
> +
> + dev_dbg(csi->dev, "Bound %s pad: %d\n", subdev->name, csi->src_pad);
> + return 0;
> +}
> +
> +static int csi_notify_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi,
> + notifier);
> + int ret;
> +
> + if (notifier->num_subdevs != 1)
> + return -EINVAL;
> +
> + ret = v4l2_device_register_subdev_nodes(&csi->v4l);
> + if (ret < 0)
> + return ret;
> +
> + ret = csi_v4l2_register(csi);
> + if (ret < 0)
> + return ret;
> +
> + return media_create_pad_link(&csi->src_subdev->entity, csi->src_pad,
> + &csi->vdev.entity, 0,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static const struct v4l2_async_notifier_operations csi_notify_ops = {
> + .bound = csi_notify_bound,
> + .complete = csi_notify_complete,
> +};
> +
> +static int sun4i_csi_async_parse(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd)
> +{
> + struct sun4i_csi *csi = dev_get_drvdata(dev);
> +
> + if (vep->base.port || vep->base.id)
> + return -EINVAL;
> +
> + if (vep->bus_type != V4L2_MBUS_PARALLEL)
> + return -EINVAL;
> +
> + csi->bus = vep->bus.parallel;
> +
> + return 0;
> +}
> +
> +static int csi_probe(struct platform_device *pdev)
> +{
> + struct sun4i_csi *csi;
> + struct resource *res;
> + int ret;
> + int irq;
> +
> + csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
> + if (!csi)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, csi);
> + csi->dev = &pdev->dev;
> +
> + csi->mdev.dev = csi->dev;
> + strlcpy(csi->mdev.model, "Allwinner Video Capture Device",
> + sizeof(csi->mdev.model));
> + csi->mdev.hw_revision = 0;
> + media_device_init(&csi->mdev);
> +
> + csi->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> + ret = media_entity_pads_init(&csi->vdev.entity, 1, &csi->pad);
> + if (ret < 0)
> + return 0;
> +
> + csi->has_isp = of_property_read_bool(pdev->dev.of_node,
> + "allwinner,has-isp");
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + csi->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(csi->regs))
> + return PTR_ERR(csi->regs);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + csi->ahb_clk = devm_clk_get(&pdev->dev, "ahb");
> + if (IS_ERR(csi->ahb_clk)) {
> + dev_err(&pdev->dev, "Couldn't get our ahb clock\n");
> + return PTR_ERR(csi->ahb_clk);
> + }
> +
> + if (csi->has_isp) {
> + csi->isp_clk = devm_clk_get(&pdev->dev, "isp");
> + if (IS_ERR(csi->isp_clk)) {
> + dev_err(&pdev->dev, "Couldn't get our ISP clock\n");
> + return PTR_ERR(csi->isp_clk);
> + }
> + }
> +
> + csi->mod_clk = devm_clk_get(&pdev->dev, "mod");
> + if (IS_ERR(csi->mod_clk)) {
> + dev_err(&pdev->dev, "Couldn't get our mod clock\n");
> + return PTR_ERR(csi->mod_clk);
> + }
> +
> + csi->ram_clk = devm_clk_get(&pdev->dev, "ram");
> + if (IS_ERR(csi->ram_clk)) {
> + dev_err(&pdev->dev, "Couldn't get our ram clock\n");
> + return PTR_ERR(csi->ram_clk);
> + }
> +
> + csi->rst = devm_reset_control_get(&pdev->dev, NULL);
> + if (IS_ERR(csi->rst)) {
> + dev_err(&pdev->dev, "Couldn't get our reset line\n");
> + return PTR_ERR(csi->rst);
> + }
> +
> + ret = csi_dma_register(csi, irq);
> + if (ret)
> + return ret;
> +
> + csi->v4l.mdev = &csi->mdev;
> +
> + ret = media_device_register(&csi->mdev);
> + if (ret)
> + goto err_unregister_dma;
> +
> + ret = v4l2_async_notifier_parse_fwnode_endpoints(csi->dev,
> + &csi->notifier,
> + sizeof(struct v4l2_async_subdev),
> + sun4i_csi_async_parse);
> + if (ret)
> + goto err_unregister_media;
> + csi->notifier.ops = &csi_notify_ops;
> +
> + ret = v4l2_async_notifier_register(&csi->v4l, &csi->notifier);
> + if (ret) {
> + dev_err(csi->dev,
> + "Couldn't register our v4l2-async notifier\n");
> + goto err_free_notifier;
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> +
> + return 0;
> +
> +err_free_notifier:
> + v4l2_async_notifier_cleanup(&csi->notifier);
> +
> +err_unregister_media:
> + media_device_unregister(&csi->mdev);
> +
> +err_unregister_dma:
> + csi_dma_unregister(csi);
> + return ret;
> +}
> +
> +static int csi_remove(struct platform_device *pdev)
> +{
> + struct sun4i_csi *csi = platform_get_drvdata(pdev);
> +
> + v4l2_async_notifier_unregister(&csi->notifier);
> + v4l2_async_notifier_cleanup(&csi->notifier);
> + media_device_unregister(&csi->mdev);
> + csi_dma_unregister(csi);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id csi_of_match[] = {
> + { .compatible = "allwinner,sun4i-a10-csi" },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, csi_of_match);
> +
> +static int csi_runtime_resume(struct device *dev)
> +{
> + struct sun4i_csi *csi = dev_get_drvdata(dev);
> +
> + reset_control_deassert(csi->rst);
> + clk_prepare_enable(csi->ahb_clk);
> + clk_prepare_enable(csi->ram_clk);
> +
> + if (csi->has_isp) {
> + clk_set_rate(csi->isp_clk, 80000000);
> + clk_prepare_enable(csi->isp_clk);
> + }
> +
> + clk_set_rate(csi->mod_clk, 24000000);
> + clk_prepare_enable(csi->mod_clk);
> +
> + writel(1, csi->regs + CSI_EN_REG);
> +
> + return 0;
> +}
> +
> +static int csi_runtime_suspend(struct device *dev)
> +{
> + struct sun4i_csi *csi = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(csi->mod_clk);
> +
> + if (csi->has_isp)
> + clk_disable_unprepare(csi->isp_clk);
> +
> + clk_disable_unprepare(csi->ram_clk);
> + clk_disable_unprepare(csi->ahb_clk);
> +
> + reset_control_assert(csi->rst);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops csi_pm_ops = {
> + .runtime_resume = csi_runtime_resume,
> + .runtime_suspend = csi_runtime_suspend,
> +};
> +
> +static struct platform_driver csi_driver = {
> + .probe = csi_probe,
> + .remove = csi_remove,
> + .driver = {
> + .name = "sun4i-csi",
> + .of_match_table = csi_of_match,
> + .pm = &csi_pm_ops,
> + },
> +};
> +module_platform_driver(csi_driver);
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> new file mode 100644
> index 000000000000..80ced01567fa
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> @@ -0,0 +1,137 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2016 NextThing Co
> + * Copyright (C) 2016-2018 Bootlin
> + *
> + * Author: Maxime Ripard <maxime.ripard@bootlin.com>
> + */
> +
> +#ifndef _SUN4I_CSI_H_
> +#define _SUN4I_CSI_H_
> +
> +#include <media/media-device.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/videobuf2-core.h>
> +
> +#define CSI_EN_REG 0x00
> +
> +#define CSI_CFG_REG 0x04
> +#define CSI_CFG_INPUT_FMT(fmt) ((fmt) << 20)
> +#define CSI_CFG_OUTPUT_FMT(fmt) ((fmt) << 16)
> +#define CSI_CFG_YUV_DATA_SEQ(seq) ((seq) << 8)
> +#define CSI_CFG_VSYNC_POL(pol) ((pol) << 2)
> +#define CSI_CFG_HSYNC_POL(pol) ((pol) << 1)
> +#define CSI_CFG_PCLK_POL(pol) ((pol) << 0)
> +
> +#define CSI_CPT_CTRL_REG 0x08
> +#define CSI_CPT_CTRL_VIDEO_START BIT(1)
> +#define CSI_CPT_CTRL_IMAGE_START BIT(0)
> +
> +#define CSI_BUF_ADDR_REG(fifo, buf) (0x10 + (0x8 * (fifo)) + (0x4 * (buf)))
> +
> +#define CSI_BUF_CTRL_REG 0x28
> +#define CSI_BUF_CTRL_DBN BIT(2)
> +#define CSI_BUF_CTRL_DBS BIT(1)
> +#define CSI_BUF_CTRL_DBE BIT(0)
> +
> +#define CSI_INT_EN_REG 0x30
> +#define CSI_INT_FRM_DONE BIT(1)
> +#define CSI_INT_CPT_DONE BIT(0)
> +
> +#define CSI_INT_STA_REG 0x34
> +
> +#define CSI_WIN_CTRL_W_REG 0x40
> +#define CSI_WIN_CTRL_W_ACTIVE(w) ((w) << 16)
> +
> +#define CSI_WIN_CTRL_H_REG 0x44
> +#define CSI_WIN_CTRL_H_ACTIVE(h) ((h) << 16)
> +
> +#define CSI_BUF_LEN_REG 0x48
> +
> +#define CSI_MAX_BUFFER 2
> +
> +enum csi_input {
> + CSI_INPUT_RAW = 0,
> + CSI_INPUT_BT656 = 2,
> + CSI_INPUT_YUV = 3,
> +};
> +
> +enum csi_output_raw {
> + CSI_OUTPUT_RAW_PASSTHROUGH = 0,
> +};
> +
> +enum csi_output_yuv {
> + CSI_OUTPUT_YUV_422_PLANAR = 0,
> + CSI_OUTPUT_YUV_420_PLANAR = 1,
> + CSI_OUTPUT_YUV_422_UV = 4,
> + CSI_OUTPUT_YUV_420_UV = 5,
> + CSI_OUTPUT_YUV_422_MACRO = 8,
> + CSI_OUTPUT_YUV_420_MACRO = 9,
> +};
> +
> +enum csi_yuv_data_seq {
> + CSI_YUV_DATA_SEQ_YUYV = 0,
> + CSI_YUV_DATA_SEQ_YVYU = 1,
> + CSI_YUV_DATA_SEQ_UYVY = 2,
> + CSI_YUV_DATA_SEQ_VYUY = 3,
> +};
> +
> +struct sun4i_csi_format {
> + u32 mbus;
> + u32 fourcc;
> + enum csi_input input;
> + u32 output;
> + unsigned int num_planes;
> + u8 bpp[3];
> + unsigned int hsub;
> + unsigned int vsub;
> +};
> +
> +struct sun4i_csi {
> + // Device resources
> + struct device *dev;
> +
> + void __iomem *regs;
> + struct clk *ahb_clk;
> + struct clk *isp_clk;
> + struct clk *mod_clk;
> + struct clk *ram_clk;
> + struct reset_control *rst;
> + unsigned char has_isp;
> +
> + const struct sun4i_csi_format *p_fmt;
> + struct v4l2_pix_format_mplane v_fmt;
> +
> + struct vb2_v4l2_buffer *current_buf[CSI_MAX_BUFFER];
> +
> + struct media_device mdev;
> + struct media_pad pad;
> +
> + struct v4l2_fwnode_bus_parallel bus;
> +
> + // V4L2 Async variables
/* Comment */
> + struct v4l2_async_notifier notifier;
> + struct v4l2_subdev *src_subdev;
> + unsigned int src_pad;
> +
> + // V4L2 variables
> + struct mutex lock;
> + struct v4l2_device v4l;
> + struct video_device vdev;
> +
> + // Videobuf2
> + struct vb2_queue queue;
> + struct list_head buf_list;
> + spinlock_t qlock;
> + unsigned int sequence;
> +};
> +
> +int csi_dma_register(struct sun4i_csi *csi, int irq);
> +void csi_dma_unregister(struct sun4i_csi *csi);
> +
> +int csi_v4l2_register(struct sun4i_csi *csi);
> +
> +#endif /* _SUN4I_CSI_H_ */
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> new file mode 100644
> index 000000000000..989b46968e87
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2016 NextThing Co
> + * Copyright (C) 2016-2018 Bootlin
> + *
> + * Author: Maxime Ripard <maxime.ripard@bootlin.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#include "sun4i_csi.h"
> +
> +struct csi_buffer {
> + struct vb2_v4l2_buffer vb;
> + struct list_head list;
> +};
> +
> +static inline struct csi_buffer *vb2_v4l2_to_csi_buffer(const struct vb2_v4l2_buffer *p)
> +{
> + return container_of(p, struct csi_buffer, vb);
> +}
> +
> +static inline struct csi_buffer *vb2_to_csi_buffer(const struct vb2_buffer *p)
> +{
> + return vb2_v4l2_to_csi_buffer(to_vb2_v4l2_buffer(p));
> +}
> +
> +static void csi_capture_start(struct sun4i_csi *csi)
> +{
> + writel(CSI_CPT_CTRL_VIDEO_START, csi->regs + CSI_CPT_CTRL_REG);
> +}
> +
> +static void csi_capture_stop(struct sun4i_csi *csi)
> +{
> + writel(0, csi->regs + CSI_CPT_CTRL_REG);
> +}
> +
> +static int csi_queue_setup(struct vb2_queue *vq,
> + unsigned int *nbuffers,
> + unsigned int *nplanes,
> + unsigned int sizes[],
> + struct device *alloc_devs[])
> +
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vq);
> + unsigned int i;
> +
> + if (*nbuffers < 3)
> + *nbuffers = 3;
> +
> + *nplanes = csi->p_fmt->num_planes;
> +
> + for (i = 0; i < *nplanes; i++)
> + sizes[i] = csi->v_fmt.plane_fmt[i].sizeimage;
> +
> + return 0;
> +};
> +
> +static int csi_buffer_prepare(struct vb2_buffer *vb)
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vb->vb2_queue);
> + unsigned int i;
> +
> + for (i = 0; i < csi->p_fmt->num_planes; i++) {
> + unsigned long size = csi->v_fmt.plane_fmt[i].sizeimage;
> +
> + if (vb2_plane_size(vb, i) < size) {
> + dev_err(csi->dev, "buffer too small (%lu < %lu)\n",
> + vb2_plane_size(vb, i), size);
> + return -EINVAL;
> + }
> +
> + vb2_set_plane_payload(vb, i, size);
> + }
> +
> + return 0;
> +}
> +
> +static int csi_buffer_fill_slot(struct sun4i_csi *csi, unsigned int slot)
> +{
> + struct csi_buffer *c_buf;
> + struct vb2_v4l2_buffer *v_buf;
> + unsigned int plane;
> +
> + /*
> + * We should never end up in a situation where we overwrite an
> + * already filled slot.
> + */
> + if (WARN_ON(csi->current_buf[slot]))
> + return -EINVAL;
> +
> + if (list_empty(&csi->buf_list)) {
> + csi->current_buf[slot] = NULL;
> +
> + dev_warn(csi->dev, "Running out of buffers...\n");
> + return -ENOMEM;
> + }
> +
> + c_buf = list_first_entry(&csi->buf_list, struct csi_buffer, list);
> + list_del_init(&c_buf->list);
> +
> + v_buf = &c_buf->vb;
> + csi->current_buf[slot] = v_buf;
> +
> + for (plane = 0; plane < csi->p_fmt->num_planes; plane++) {
> + dma_addr_t buf_addr;
> +
> + buf_addr = vb2_dma_contig_plane_dma_addr(&v_buf->vb2_buf,
> + plane);
> + writel(buf_addr, csi->regs + CSI_BUF_ADDR_REG(plane, slot));
> + }
> +
> + return 0;
> +}
> +
> +static int csi_buffer_fill_all(struct sun4i_csi *csi)
> +{
> + unsigned int slot;
> + int ret;
> +
> + for (slot = 0; slot < CSI_MAX_BUFFER; slot++) {
> + ret = csi_buffer_fill_slot(csi, slot);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void csi_buffer_mark_done(struct sun4i_csi *csi,
> + unsigned int slot,
> + unsigned int sequence)
> +{
> + struct vb2_v4l2_buffer *v_buf;
> +
> + if (WARN_ON(!csi->current_buf[slot]))
> + return;
> +
> + v_buf = csi->current_buf[slot];
> + v_buf->field = csi->v_fmt.field;
> + v_buf->sequence = sequence;
> + v_buf->vb2_buf.timestamp = ktime_get_ns();
> + vb2_buffer_done(&v_buf->vb2_buf, VB2_BUF_STATE_DONE);
> +
> + csi->current_buf[slot] = NULL;
> +}
> +
> +static int csi_buffer_flip(struct sun4i_csi *csi, unsigned int sequence)
> +{
> + u32 reg = readl(csi->regs + CSI_BUF_CTRL_REG);
> + int curr, next;
unsigned int
> +
> + /* Our next buffer is not the current buffer */
> + curr = !!(reg & CSI_BUF_CTRL_DBS);
> + next = !curr;
> +
> + /* Report the previous buffer as done */
> + csi_buffer_mark_done(csi, next, sequence);
> +
> + /* Put a new buffer in there */
> + return csi_buffer_fill_slot(csi, next);
> +}
> +
> +static void csi_buffer_queue(struct vb2_buffer *vb)
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vb->vb2_queue);
> + struct csi_buffer *buf = vb2_to_csi_buffer(vb);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&csi->qlock, flags);
> + list_add_tail(&buf->list, &csi->buf_list);
> + spin_unlock_irqrestore(&csi->qlock, flags);
> +}
> +
> +static void return_all_buffers(struct sun4i_csi *csi,
> + enum vb2_buffer_state state)
> +{
> + struct csi_buffer *buf, *node;
> + unsigned int slot;
> +
> + list_for_each_entry_safe(buf, node, &csi->buf_list, list) {
> + vb2_buffer_done(&buf->vb.vb2_buf, state);
> + list_del(&buf->list);
> + }
> +
> + for (slot = 0; slot < CSI_MAX_BUFFER; slot++) {
> + struct vb2_v4l2_buffer *v_buf = csi->current_buf[slot];
> +
> + if (!v_buf)
> + continue;
> +
> + vb2_buffer_done(&v_buf->vb2_buf, state);
> + csi->current_buf[slot] = NULL;
> + }
> +}
> +
> +static int csi_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vq);
> + struct v4l2_fwnode_bus_parallel *bus = &csi->bus;
> + unsigned long hsync_pol, pclk_pol, vsync_pol;
> + unsigned long flags;
> + int ret = 0;
> +
> + csi->sequence = 0;
> +
> + if (count < 2)
> + return -ENOBUFS;
> +
> + ret = media_pipeline_start(&csi->vdev.entity, &csi->vdev.pipe);
> + if (ret < 0)
> + goto clear_dma_queue;
> +
> + dev_dbg(csi->dev, "Starting capture\n");
> +
> + spin_lock_irqsave(&csi->qlock, flags);
> +
> + /* Setup timings */
> + writel(CSI_WIN_CTRL_W_ACTIVE(csi->v_fmt.width * 2),
> + csi->regs + CSI_WIN_CTRL_W_REG);
> + writel(CSI_WIN_CTRL_H_ACTIVE(csi->v_fmt.height),
> + csi->regs + CSI_WIN_CTRL_H_REG);
> +
> + hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
> + pclk_pol = !!(bus->flags & V4L2_MBUS_DATA_ACTIVE_HIGH);
> + vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
> + writel(CSI_CFG_INPUT_FMT(csi->p_fmt->input) |
> + CSI_CFG_OUTPUT_FMT(csi->p_fmt->output) |
> + CSI_CFG_VSYNC_POL(vsync_pol) |
> + CSI_CFG_HSYNC_POL(hsync_pol) |
> + CSI_CFG_PCLK_POL(pclk_pol),
> + csi->regs + CSI_CFG_REG);
> +
> + /* Setup buffer length */
> + writel(csi->v_fmt.plane_fmt[0].bytesperline,
> + csi->regs + CSI_BUF_LEN_REG);
> +
> + /* Prepare our buffers in hardware */
> + ret = csi_buffer_fill_all(csi);
> + if (ret) {
> + spin_unlock_irqrestore(&csi->qlock, flags);
> + goto disable_pipeline;
> + }
> +
> + /* Enable double buffering */
> + writel(CSI_BUF_CTRL_DBE, csi->regs + CSI_BUF_CTRL_REG);
> +
> + /* Clear the pending interrupts */
> + writel(CSI_INT_FRM_DONE, csi->regs + 0x34);
> +
> + /* Enable frame done interrupt */
> + writel(CSI_INT_FRM_DONE, csi->regs + CSI_INT_EN_REG);
> +
> + csi_capture_start(csi);
> +
> + spin_unlock_irqrestore(&csi->qlock, flags);
> +
> + ret = v4l2_subdev_call(csi->src_subdev, video, s_stream, 1);
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + goto disable_pipeline;
> +
> + return 0;
> +
> +disable_pipeline:
> + media_pipeline_stop(&csi->vdev.entity);
> +
> +clear_dma_queue:
> + return_all_buffers(csi, VB2_BUF_STATE_QUEUED);
I guess this might be ok without taking the spinlock but I'd still do that,
if for nothing else, then for the sake of consistency.
> +
> + return ret;
> +}
> +
> +static void csi_stop_streaming(struct vb2_queue *vq)
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vq);
> + unsigned long flags;
> +
> + dev_dbg(csi->dev, "Stopping capture\n");
> +
> + v4l2_subdev_call(csi->src_subdev, video, s_stream, 0);
> + csi_capture_stop(csi);
> +
> + /* Release all active buffers */
> + spin_lock_irqsave(&csi->qlock, flags);
> + return_all_buffers(csi, VB2_BUF_STATE_ERROR);
> + spin_unlock_irqrestore(&csi->qlock, flags);
> +
> + media_pipeline_stop(&csi->vdev.entity);
> +}
> +
> +static struct vb2_ops csi_qops = {
> + .queue_setup = csi_queue_setup,
> + .buf_prepare = csi_buffer_prepare,
> + .buf_queue = csi_buffer_queue,
> + .start_streaming = csi_start_streaming,
> + .stop_streaming = csi_stop_streaming,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> +};
> +
> +static irqreturn_t csi_irq(int irq, void *data)
> +{
> + struct sun4i_csi *csi = data;
> + unsigned int sequence;
> + u32 reg;
> +
> + reg = readl(csi->regs + CSI_INT_STA_REG);
> +
> + /* Acknowledge the interrupts */
> + writel(reg, csi->regs + CSI_INT_STA_REG);
> +
> + sequence = csi->sequence++;
> +
> + if (!(reg & CSI_INT_FRM_DONE))
> + goto out;
> +
> + spin_lock(&csi->qlock);
> + if (csi_buffer_flip(csi, sequence)) {
> + dev_warn(csi->dev, "%s: Flip failed\n", __func__);
> + csi_capture_stop(csi);
> + }
> + spin_unlock(&csi->qlock);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +int csi_dma_register(struct sun4i_csi *csi, int irq)
How about picking a bit less generic-looking name for the global symbols?
E.g. a part of the device's name.
> +{
> + struct vb2_queue *q = &csi->queue;
> + int ret;
> + int i;
> +
> + ret = v4l2_device_register(csi->dev, &csi->v4l);
> + if (ret) {
> + dev_err(csi->dev, "Couldn't register the v4l2 device\n");
> + return ret;
> + }
> +
> + spin_lock_init(&csi->qlock);
> + mutex_init(&csi->lock);
> +
> + INIT_LIST_HEAD(&csi->buf_list);
> + for (i = 0; i < CSI_MAX_BUFFER; i++)
> + csi->current_buf[i] = NULL;
> +
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> + q->io_modes = VB2_MMAP;
> + q->lock = &csi->lock;
> + q->drv_priv = csi;
> + q->buf_struct_size = sizeof(struct csi_buffer);
> + q->ops = &csi_qops;
> + q->mem_ops = &vb2_dma_contig_memops;
> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + q->gfp_flags = GFP_DMA32;
> + q->dev = csi->dev;
> +
> + ret = vb2_queue_init(q);
> + if (ret < 0) {
> + dev_err(csi->dev, "failed to initialize VB2 queue\n");
> + return ret;
> + }
> +
> + ret = devm_request_irq(csi->dev, irq, csi_irq, 0,
> + dev_name(csi->dev), csi);
> + if (ret) {
> + dev_err(csi->dev, "Couldn't register our interrupt\n");
vb2_queue_release(q) here?
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +void csi_dma_unregister(struct sun4i_csi *csi)
> +{
> + v4l2_device_unregister(&csi->v4l);
> +}
> +
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> new file mode 100644
> index 000000000000..0b9487830e6d
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> @@ -0,0 +1,287 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2016 NextThing Co
> + * Copyright (C) 2016-2018 Bootlin
> + *
> + * Author: Maxime Ripard <maxime.ripard@bootlin.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#include "sun4i_csi.h"
> +
> +#define CSI_DEFAULT_FORMAT V4L2_PIX_FMT_YUV420M
> +#define CSI_DEFAULT_WIDTH 640
> +#define CSI_DEFAULT_HEIGHT 480
> +
> +#define CSI_MAX_HEIGHT 8192U
> +#define CSI_MAX_WIDTH 8192U
> +
> +static const struct sun4i_csi_format csi_formats[] = {
> + /* YUV422 inputs */
> + {
> + .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> + .fourcc = V4L2_PIX_FMT_YUV420M,
> + .input = CSI_INPUT_YUV,
> + .output = CSI_OUTPUT_YUV_420_PLANAR,
> + .num_planes = 3,
> + .bpp = { 8, 8, 8 },
> + .hsub = 2,
> + .vsub = 2,
> + },
> +};
> +
> +static const struct sun4i_csi_format *
> +csi_get_format_by_fourcc(struct sun4i_csi *csi, u32 fourcc)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(csi_formats); i++)
> + if (csi_formats[i].fourcc == fourcc)
> + return &csi_formats[i];
> +
> + return NULL;
> +}
> +
> +static int csi_querycap(struct file *file, void *priv,
> + struct v4l2_capability *cap)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> +
> + strscpy(cap->driver, KBUILD_MODNAME, sizeof(cap->driver));
> + strscpy(cap->card, "sun4i-csi", sizeof(cap->card));
> + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> + dev_name(csi->dev));
> + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING;
> + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> +
> + return 0;
> +}
> +
> +static int csi_enum_input(struct file *file, void *priv,
> + struct v4l2_input *inp)
> +{
> + if (inp->index != 0)
> + return -EINVAL;
> +
> + inp->type = V4L2_INPUT_TYPE_CAMERA;
> + strlcpy(inp->name, "Camera", sizeof(inp->name));
> +
> + return 0;
> +}
> +
> +static int csi_g_input(struct file *file, void *fh,
> + unsigned int *i)
> +{
> + *i = 0;
> +
> + return 0;
> +}
> +
> +static int csi_s_input(struct file *file, void *fh,
> + unsigned int i)
> +{
> + if (i != 0)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int _csi_try_fmt(struct sun4i_csi *csi,
> + struct v4l2_pix_format_mplane *pix,
> + const struct sun4i_csi_format **fmt)
> +{
> + const struct sun4i_csi_format *_fmt;
> + unsigned int width = pix->width;
> + unsigned int height = pix->height;
> + int i;
unsigned int ?
> +
> + _fmt = csi_get_format_by_fourcc(csi, pix->pixelformat);
> + if (!_fmt)
> + _fmt = csi_get_format_by_fourcc(csi, csi_formats[0].fourcc);
> +
> + pix->field = V4L2_FIELD_NONE;
> + pix->colorspace = V4L2_COLORSPACE_SRGB;
> + pix->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(pix->colorspace);
> + pix->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
> + pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true, pix->colorspace,
> + pix->ycbcr_enc);
> +
> + pix->num_planes = _fmt->num_planes;
> + pix->pixelformat = _fmt->fourcc;
> +
> + memset(pix->reserved, 0, sizeof(pix->reserved));
> +
> + /* Align the width and height on the subsampling */
> + width = ALIGN(width, _fmt->hsub);
> + height = ALIGN(height, _fmt->vsub);
> +
> + /* Clamp the width and height to our capabilities */
> + pix->width = clamp(width, _fmt->hsub, CSI_MAX_WIDTH);
> + pix->height = clamp(height, _fmt->vsub, CSI_MAX_HEIGHT);
> +
> + for (i = 0; i < _fmt->num_planes; i++) {
> + unsigned int hsub = i > 0 ? _fmt->hsub : 1;
> + unsigned int vsub = i > 0 ? _fmt->vsub : 1;
> + unsigned int bpl;
> +
> + bpl = pix->width / hsub * _fmt->bpp[i] / 8;
> + pix->plane_fmt[i].bytesperline = bpl;
> + pix->plane_fmt[i].sizeimage = bpl * pix->height / vsub;
> + memset(pix->plane_fmt[i].reserved, 0,
> + sizeof(pix->plane_fmt[i].reserved));
> + }
> +
> + if (fmt)
> + *fmt = _fmt;
> +
> + return 0;
> +}
> +
> +static int csi_try_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> +
> + if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
> +
> + return _csi_try_fmt(csi, &f->fmt.pix_mp, NULL);
> +}
> +
> +static int csi_s_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + const struct sun4i_csi_format *fmt;
> + struct sun4i_csi *csi = video_drvdata(file);
> + int ret;
> +
> + if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
> +
> + ret = _csi_try_fmt(csi, &f->fmt.pix_mp, &fmt);
> + if (ret)
> + return ret;
> +
> + csi->v_fmt = f->fmt.pix_mp;
> + csi->p_fmt = fmt;
> +
> + return 0;
> +}
> +
> +static int csi_g_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> +
> + if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
> +
> + f->fmt.pix_mp = csi->v_fmt;
> +
> + return 0;
> +}
> +
> +static int csi_enum_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_fmtdesc *f)
> +{
> + if (f->index >= ARRAY_SIZE(csi_formats))
> + return -EINVAL;
> +
> + f->pixelformat = csi_formats[f->index].fourcc;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops csi_ioctl_ops = {
> + .vidioc_querycap = csi_querycap,
> +
> + .vidioc_enum_fmt_vid_cap_mplane = csi_enum_fmt_vid_cap,
> + .vidioc_g_fmt_vid_cap_mplane = csi_g_fmt_vid_cap,
> + .vidioc_s_fmt_vid_cap_mplane = csi_s_fmt_vid_cap,
> + .vidioc_try_fmt_vid_cap_mplane = csi_try_fmt_vid_cap,
> +
> + .vidioc_enum_input = csi_enum_input,
> + .vidioc_g_input = csi_g_input,
> + .vidioc_s_input = csi_s_input,
> +
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> + .vidioc_querybuf = vb2_ioctl_querybuf,
> + .vidioc_qbuf = vb2_ioctl_qbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_expbuf = vb2_ioctl_expbuf,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> +};
> +
> +static int csi_open(struct file *file)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> + int ret;
> +
> + pm_runtime_get_sync(csi->dev);
> +
> + ret = v4l2_subdev_call(csi->src_subdev, core, s_power, 1);
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + return ret;
> +
> + return v4l2_fh_open(file);
Do pm_runtime_put() if v4l2_fh_open() fails. Also v4l2_subdev_call(...,
s_power, 0).
> +}
> +
> +static int csi_release(struct file *file)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> +
> + pm_runtime_put(csi->dev);
> +
> + return vb2_fop_release(file);
> +}
> +
> +static const struct v4l2_file_operations csi_fops = {
> + .owner = THIS_MODULE,
> + .open = csi_open,
> + .release = csi_release,
> + .unlocked_ioctl = video_ioctl2,
> + .read = vb2_fop_read,
> + .write = vb2_fop_write,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +};
> +
> +int csi_v4l2_register(struct sun4i_csi *csi)
Ditto.
> +{
> + struct video_device *vdev = &csi->vdev;
> + int ret;
> +
> + vdev->v4l2_dev = &csi->v4l;
> + vdev->queue = &csi->queue;
> + strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> + vdev->release = video_device_release_empty;
> + vdev->lock = &csi->lock;
> +
> + /* Set a default format */
> + csi->v_fmt.pixelformat = CSI_DEFAULT_FORMAT;
> + csi->v_fmt.width = CSI_DEFAULT_WIDTH;
> + csi->v_fmt.height = CSI_DEFAULT_HEIGHT;
> + _csi_try_fmt(csi, &csi->v_fmt, NULL);
> +
> + vdev->fops = &csi_fops;
> + vdev->ioctl_ops = &csi_ioctl_ops;
> + video_set_drvdata(vdev, csi);
> +
> + ret = video_register_device(&csi->vdev, VFL_TYPE_GRABBER, -1);
> + if (ret)
> + return ret;
> +
> + dev_info(csi->dev, "Device registered as %s\n",
> + video_device_node_name(vdev));
> +
> + return 0;
> +}
> +
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding
2018-11-13 8:24 ` [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding Maxime Ripard
2018-11-13 8:38 ` Sakari Ailus
@ 2018-11-13 10:34 ` Sakari Ailus
1 sibling, 0 replies; 36+ messages in thread
From: Sakari Ailus @ 2018-11-13 10:34 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Mauro Carvalho Chehab, Thomas Petazzoni,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
Hi Maxime,
On Tue, Nov 13, 2018 at 09:24:13AM +0100, Maxime Ripard wrote:
> The Allwinner A10 CMOS Sensor Interface is a camera capture interface also
...
> +Optional properties:
> + - allwinner,csi-channels: Number of channels available in the CSI
> + controller. If not present, the default
> + will be 1.
Is this virtual channels or something else btw.?
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 8:24 ` [PATCH 3/5] media: sunxi: Add A10 CSI driver Maxime Ripard
2018-11-13 8:57 ` Sakari Ailus
@ 2018-11-13 12:24 ` Hans Verkuil
2018-11-13 15:19 ` Joe Perches
2018-11-15 20:51 ` Maxime Ripard
2018-11-13 12:48 ` Fabio Estevam
2 siblings, 2 replies; 36+ messages in thread
From: Hans Verkuil @ 2018-11-13 12:24 UTC (permalink / raw)
To: Maxime Ripard, Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
Hi Maxime,
A quick code review:
On 11/13/18 09:24, Maxime Ripard wrote:
> The older CSI drivers have camera capture interface different from the one
> in the newer ones.
>
> This IP is pretty simple. Some variants (one controller out of two
> instances on some SoCs) have an ISP embedded, but there's no code that make
> use of it, so we ignored that part for now.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
> drivers/media/platform/sunxi/Kconfig | 1 +-
> drivers/media/platform/sunxi/Makefile | 1 +-
> drivers/media/platform/sunxi/sun4i-csi/Kconfig | 12 +-
> drivers/media/platform/sunxi/sun4i-csi/Makefile | 5 +-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 275 +++++++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h | 137 +++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 383 +++++++++++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c | 287 ++++++++++-
Missing MAINTAINERS file update.
> 8 files changed, 1101 insertions(+)
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Kconfig
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Makefile
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
>
> diff --git a/drivers/media/platform/sunxi/Kconfig b/drivers/media/platform/sunxi/Kconfig
> index 1b6e89cb78b2..71808e93ac2e 100644
> --- a/drivers/media/platform/sunxi/Kconfig
> +++ b/drivers/media/platform/sunxi/Kconfig
> @@ -1 +1,2 @@
> +source "drivers/media/platform/sunxi/sun4i-csi/Kconfig"
> source "drivers/media/platform/sunxi/sun6i-csi/Kconfig"
> diff --git a/drivers/media/platform/sunxi/Makefile b/drivers/media/platform/sunxi/Makefile
> index 8d06f98500ee..a05127529006 100644
> --- a/drivers/media/platform/sunxi/Makefile
> +++ b/drivers/media/platform/sunxi/Makefile
> @@ -1 +1,2 @@
> +obj-y += sun4i-csi/
> obj-y += sun6i-csi/
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/Kconfig b/drivers/media/platform/sunxi/sun4i-csi/Kconfig
> new file mode 100644
> index 000000000000..841a6f4d9c99
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/Kconfig
> @@ -0,0 +1,12 @@
> +config VIDEO_SUN4I_CSI
> + tristate "Allwinner A10 CMOS Sensor Interface Support"
> + depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
> + depends on ARCH_SUNXI || COMPILE_TEST
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_FWNODE
> + select V4L2_MEM2MEM_DEV
> + help
> + This is a V4L2 driver for the Allwinner A10 CSI
> +
> + To compile this driver as a module, choose M here: the module
> + will be called sun4i_csi.
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/Makefile b/drivers/media/platform/sunxi/sun4i-csi/Makefile
> new file mode 100644
> index 000000000000..7c790a57f5ee
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/Makefile
> @@ -0,0 +1,5 @@
> +sun4i-csi-y += sun4i_csi.o
> +sun4i-csi-y += sun4i_dma.o
> +sun4i-csi-y += sun4i_v4l2.o
> +
> +obj-$(CONFIG_VIDEO_SUN4I_CSI) += sun4i-csi.o
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> new file mode 100644
> index 000000000000..3e0a65ccbbe2
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2016 NextThing Co
> + * Copyright (C) 2016-2018 Bootlin
> + *
> + * Author: Maxime Ripard <maxime.ripard@bootlin.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mediabus.h>
> +
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +#include "sun4i_csi.h"
> +
> +static int csi_notify_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi,
> + notifier);
> +
> + csi->src_subdev = subdev;
> + csi->src_pad = media_entity_get_fwnode_pad(&subdev->entity,
> + subdev->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (csi->src_pad < 0) {
> + dev_err(csi->dev, "Couldn't find output pad for subdev %s\n",
> + subdev->name);
> + return csi->src_pad;
> + }
> +
> + dev_dbg(csi->dev, "Bound %s pad: %d\n", subdev->name, csi->src_pad);
> + return 0;
> +}
> +
> +static int csi_notify_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi,
> + notifier);
> + int ret;
> +
> + if (notifier->num_subdevs != 1)
> + return -EINVAL;
> +
> + ret = v4l2_device_register_subdev_nodes(&csi->v4l);
> + if (ret < 0)
> + return ret;
> +
> + ret = csi_v4l2_register(csi);
> + if (ret < 0)
> + return ret;
> +
> + return media_create_pad_link(&csi->src_subdev->entity, csi->src_pad,
> + &csi->vdev.entity, 0,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static const struct v4l2_async_notifier_operations csi_notify_ops = {
> + .bound = csi_notify_bound,
> + .complete = csi_notify_complete,
> +};
> +
> +static int sun4i_csi_async_parse(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd)
> +{
> + struct sun4i_csi *csi = dev_get_drvdata(dev);
> +
> + if (vep->base.port || vep->base.id)
> + return -EINVAL;
> +
> + if (vep->bus_type != V4L2_MBUS_PARALLEL)
> + return -EINVAL;
> +
> + csi->bus = vep->bus.parallel;
> +
> + return 0;
> +}
> +
> +static int csi_probe(struct platform_device *pdev)
> +{
> + struct sun4i_csi *csi;
> + struct resource *res;
> + int ret;
> + int irq;
> +
> + csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
devm_kzalloc is not recommended: all devm_ memory is freed when the driver
is unbound, but a filehandle might still have a reference open.
> + if (!csi)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, csi);
> + csi->dev = &pdev->dev;
> +
> + csi->mdev.dev = csi->dev;
> + strlcpy(csi->mdev.model, "Allwinner Video Capture Device",
> + sizeof(csi->mdev.model));
Use strscpy instead of strcpy, strncpy or strlcpy. We're in the process
of converting all of those string copy functions in drivers/media to
strscpy.
> + csi->mdev.hw_revision = 0;
> + media_device_init(&csi->mdev);
> +
> + csi->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> + ret = media_entity_pads_init(&csi->vdev.entity, 1, &csi->pad);
> + if (ret < 0)
> + return 0;
> +
> + csi->has_isp = of_property_read_bool(pdev->dev.of_node,
> + "allwinner,has-isp");
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + csi->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(csi->regs))
> + return PTR_ERR(csi->regs);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + csi->ahb_clk = devm_clk_get(&pdev->dev, "ahb");
> + if (IS_ERR(csi->ahb_clk)) {
> + dev_err(&pdev->dev, "Couldn't get our ahb clock\n");
> + return PTR_ERR(csi->ahb_clk);
> + }
> +
> + if (csi->has_isp) {
> + csi->isp_clk = devm_clk_get(&pdev->dev, "isp");
> + if (IS_ERR(csi->isp_clk)) {
> + dev_err(&pdev->dev, "Couldn't get our ISP clock\n");
> + return PTR_ERR(csi->isp_clk);
> + }
> + }
> +
> + csi->mod_clk = devm_clk_get(&pdev->dev, "mod");
> + if (IS_ERR(csi->mod_clk)) {
> + dev_err(&pdev->dev, "Couldn't get our mod clock\n");
> + return PTR_ERR(csi->mod_clk);
> + }
> +
> + csi->ram_clk = devm_clk_get(&pdev->dev, "ram");
> + if (IS_ERR(csi->ram_clk)) {
> + dev_err(&pdev->dev, "Couldn't get our ram clock\n");
> + return PTR_ERR(csi->ram_clk);
> + }
> +
> + csi->rst = devm_reset_control_get(&pdev->dev, NULL);
> + if (IS_ERR(csi->rst)) {
> + dev_err(&pdev->dev, "Couldn't get our reset line\n");
> + return PTR_ERR(csi->rst);
> + }
> +
> + ret = csi_dma_register(csi, irq);
> + if (ret)
> + return ret;
> +
> + csi->v4l.mdev = &csi->mdev;
> +
> + ret = media_device_register(&csi->mdev);
> + if (ret)
> + goto err_unregister_dma;
> +
> + ret = v4l2_async_notifier_parse_fwnode_endpoints(csi->dev,
> + &csi->notifier,
> + sizeof(struct v4l2_async_subdev),
> + sun4i_csi_async_parse);
> + if (ret)
> + goto err_unregister_media;
> + csi->notifier.ops = &csi_notify_ops;
> +
> + ret = v4l2_async_notifier_register(&csi->v4l, &csi->notifier);
> + if (ret) {
> + dev_err(csi->dev,
> + "Couldn't register our v4l2-async notifier\n");
> + goto err_free_notifier;
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> +
> + return 0;
> +
> +err_free_notifier:
> + v4l2_async_notifier_cleanup(&csi->notifier);
> +
> +err_unregister_media:
> + media_device_unregister(&csi->mdev);
> +
> +err_unregister_dma:
> + csi_dma_unregister(csi);
> + return ret;
> +}
> +
> +static int csi_remove(struct platform_device *pdev)
> +{
> + struct sun4i_csi *csi = platform_get_drvdata(pdev);
> +
> + v4l2_async_notifier_unregister(&csi->notifier);
> + v4l2_async_notifier_cleanup(&csi->notifier);
> + media_device_unregister(&csi->mdev);
> + csi_dma_unregister(csi);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id csi_of_match[] = {
> + { .compatible = "allwinner,sun4i-a10-csi" },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, csi_of_match);
> +
> +static int csi_runtime_resume(struct device *dev)
> +{
> + struct sun4i_csi *csi = dev_get_drvdata(dev);
> +
> + reset_control_deassert(csi->rst);
> + clk_prepare_enable(csi->ahb_clk);
> + clk_prepare_enable(csi->ram_clk);
> +
> + if (csi->has_isp) {
> + clk_set_rate(csi->isp_clk, 80000000);
> + clk_prepare_enable(csi->isp_clk);
> + }
> +
> + clk_set_rate(csi->mod_clk, 24000000);
> + clk_prepare_enable(csi->mod_clk);
> +
> + writel(1, csi->regs + CSI_EN_REG);
> +
> + return 0;
> +}
> +
> +static int csi_runtime_suspend(struct device *dev)
> +{
> + struct sun4i_csi *csi = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(csi->mod_clk);
> +
> + if (csi->has_isp)
> + clk_disable_unprepare(csi->isp_clk);
> +
> + clk_disable_unprepare(csi->ram_clk);
> + clk_disable_unprepare(csi->ahb_clk);
> +
> + reset_control_assert(csi->rst);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops csi_pm_ops = {
> + .runtime_resume = csi_runtime_resume,
> + .runtime_suspend = csi_runtime_suspend,
> +};
> +
> +static struct platform_driver csi_driver = {
> + .probe = csi_probe,
> + .remove = csi_remove,
> + .driver = {
> + .name = "sun4i-csi",
> + .of_match_table = csi_of_match,
> + .pm = &csi_pm_ops,
> + },
> +};
> +module_platform_driver(csi_driver);
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> new file mode 100644
> index 000000000000..80ced01567fa
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> @@ -0,0 +1,137 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2016 NextThing Co
> + * Copyright (C) 2016-2018 Bootlin
> + *
> + * Author: Maxime Ripard <maxime.ripard@bootlin.com>
> + */
> +
> +#ifndef _SUN4I_CSI_H_
> +#define _SUN4I_CSI_H_
> +
> +#include <media/media-device.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/videobuf2-core.h>
> +
> +#define CSI_EN_REG 0x00
> +
> +#define CSI_CFG_REG 0x04
> +#define CSI_CFG_INPUT_FMT(fmt) ((fmt) << 20)
> +#define CSI_CFG_OUTPUT_FMT(fmt) ((fmt) << 16)
> +#define CSI_CFG_YUV_DATA_SEQ(seq) ((seq) << 8)
> +#define CSI_CFG_VSYNC_POL(pol) ((pol) << 2)
> +#define CSI_CFG_HSYNC_POL(pol) ((pol) << 1)
> +#define CSI_CFG_PCLK_POL(pol) ((pol) << 0)
> +
> +#define CSI_CPT_CTRL_REG 0x08
> +#define CSI_CPT_CTRL_VIDEO_START BIT(1)
> +#define CSI_CPT_CTRL_IMAGE_START BIT(0)
> +
> +#define CSI_BUF_ADDR_REG(fifo, buf) (0x10 + (0x8 * (fifo)) + (0x4 * (buf)))
> +
> +#define CSI_BUF_CTRL_REG 0x28
> +#define CSI_BUF_CTRL_DBN BIT(2)
> +#define CSI_BUF_CTRL_DBS BIT(1)
> +#define CSI_BUF_CTRL_DBE BIT(0)
> +
> +#define CSI_INT_EN_REG 0x30
> +#define CSI_INT_FRM_DONE BIT(1)
> +#define CSI_INT_CPT_DONE BIT(0)
> +
> +#define CSI_INT_STA_REG 0x34
> +
> +#define CSI_WIN_CTRL_W_REG 0x40
> +#define CSI_WIN_CTRL_W_ACTIVE(w) ((w) << 16)
> +
> +#define CSI_WIN_CTRL_H_REG 0x44
> +#define CSI_WIN_CTRL_H_ACTIVE(h) ((h) << 16)
> +
> +#define CSI_BUF_LEN_REG 0x48
> +
> +#define CSI_MAX_BUFFER 2
> +
> +enum csi_input {
> + CSI_INPUT_RAW = 0,
> + CSI_INPUT_BT656 = 2,
> + CSI_INPUT_YUV = 3,
> +};
> +
> +enum csi_output_raw {
> + CSI_OUTPUT_RAW_PASSTHROUGH = 0,
> +};
> +
> +enum csi_output_yuv {
> + CSI_OUTPUT_YUV_422_PLANAR = 0,
> + CSI_OUTPUT_YUV_420_PLANAR = 1,
> + CSI_OUTPUT_YUV_422_UV = 4,
> + CSI_OUTPUT_YUV_420_UV = 5,
> + CSI_OUTPUT_YUV_422_MACRO = 8,
> + CSI_OUTPUT_YUV_420_MACRO = 9,
> +};
> +
> +enum csi_yuv_data_seq {
> + CSI_YUV_DATA_SEQ_YUYV = 0,
> + CSI_YUV_DATA_SEQ_YVYU = 1,
> + CSI_YUV_DATA_SEQ_UYVY = 2,
> + CSI_YUV_DATA_SEQ_VYUY = 3,
> +};
> +
> +struct sun4i_csi_format {
> + u32 mbus;
> + u32 fourcc;
> + enum csi_input input;
> + u32 output;
> + unsigned int num_planes;
> + u8 bpp[3];
> + unsigned int hsub;
> + unsigned int vsub;
> +};
> +
> +struct sun4i_csi {
> + // Device resources
> + struct device *dev;
> +
> + void __iomem *regs;
> + struct clk *ahb_clk;
> + struct clk *isp_clk;
> + struct clk *mod_clk;
> + struct clk *ram_clk;
> + struct reset_control *rst;
> + unsigned char has_isp;
> +
> + const struct sun4i_csi_format *p_fmt;
> + struct v4l2_pix_format_mplane v_fmt;
> +
> + struct vb2_v4l2_buffer *current_buf[CSI_MAX_BUFFER];
> +
> + struct media_device mdev;
> + struct media_pad pad;
> +
> + struct v4l2_fwnode_bus_parallel bus;
> +
> + // V4L2 Async variables
> + struct v4l2_async_notifier notifier;
> + struct v4l2_subdev *src_subdev;
> + unsigned int src_pad;
> +
> + // V4L2 variables
> + struct mutex lock;
> + struct v4l2_device v4l;
> + struct video_device vdev;
> +
> + // Videobuf2
Doesn't checkpatch.pl --strict complain about the use of '//'?
> + struct vb2_queue queue;
> + struct list_head buf_list;
> + spinlock_t qlock;
> + unsigned int sequence;
> +};
> +
> +int csi_dma_register(struct sun4i_csi *csi, int irq);
> +void csi_dma_unregister(struct sun4i_csi *csi);
> +
> +int csi_v4l2_register(struct sun4i_csi *csi);
> +
> +#endif /* _SUN4I_CSI_H_ */
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> new file mode 100644
> index 000000000000..989b46968e87
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2016 NextThing Co
> + * Copyright (C) 2016-2018 Bootlin
> + *
> + * Author: Maxime Ripard <maxime.ripard@bootlin.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#include "sun4i_csi.h"
> +
> +struct csi_buffer {
> + struct vb2_v4l2_buffer vb;
> + struct list_head list;
> +};
> +
> +static inline struct csi_buffer *vb2_v4l2_to_csi_buffer(const struct vb2_v4l2_buffer *p)
> +{
> + return container_of(p, struct csi_buffer, vb);
> +}
> +
> +static inline struct csi_buffer *vb2_to_csi_buffer(const struct vb2_buffer *p)
> +{
> + return vb2_v4l2_to_csi_buffer(to_vb2_v4l2_buffer(p));
> +}
> +
> +static void csi_capture_start(struct sun4i_csi *csi)
> +{
> + writel(CSI_CPT_CTRL_VIDEO_START, csi->regs + CSI_CPT_CTRL_REG);
> +}
> +
> +static void csi_capture_stop(struct sun4i_csi *csi)
> +{
> + writel(0, csi->regs + CSI_CPT_CTRL_REG);
> +}
> +
> +static int csi_queue_setup(struct vb2_queue *vq,
> + unsigned int *nbuffers,
> + unsigned int *nplanes,
> + unsigned int sizes[],
> + struct device *alloc_devs[])
> +
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vq);
> + unsigned int i;
> +
> + if (*nbuffers < 3)
> + *nbuffers = 3;
> +
> + *nplanes = csi->p_fmt->num_planes;
> +
> + for (i = 0; i < *nplanes; i++)
> + sizes[i] = csi->v_fmt.plane_fmt[i].sizeimage;
This function doesn't appear to support the VIDIOC_CREATEBUFS ioctl.
It is recommended to add support for that ioctl.
> +
> + return 0;
> +};
> +
> +static int csi_buffer_prepare(struct vb2_buffer *vb)
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vb->vb2_queue);
> + unsigned int i;
> +
> + for (i = 0; i < csi->p_fmt->num_planes; i++) {
> + unsigned long size = csi->v_fmt.plane_fmt[i].sizeimage;
> +
> + if (vb2_plane_size(vb, i) < size) {
> + dev_err(csi->dev, "buffer too small (%lu < %lu)\n",
> + vb2_plane_size(vb, i), size);
> + return -EINVAL;
> + }
> +
> + vb2_set_plane_payload(vb, i, size);
> + }
> +
> + return 0;
> +}
> +
> +static int csi_buffer_fill_slot(struct sun4i_csi *csi, unsigned int slot)
> +{
> + struct csi_buffer *c_buf;
> + struct vb2_v4l2_buffer *v_buf;
> + unsigned int plane;
> +
> + /*
> + * We should never end up in a situation where we overwrite an
> + * already filled slot.
> + */
> + if (WARN_ON(csi->current_buf[slot]))
> + return -EINVAL;
> +
> + if (list_empty(&csi->buf_list)) {
> + csi->current_buf[slot] = NULL;
> +
> + dev_warn(csi->dev, "Running out of buffers...\n");
> + return -ENOMEM;
> + }
> +
> + c_buf = list_first_entry(&csi->buf_list, struct csi_buffer, list);
> + list_del_init(&c_buf->list);
> +
> + v_buf = &c_buf->vb;
> + csi->current_buf[slot] = v_buf;
> +
> + for (plane = 0; plane < csi->p_fmt->num_planes; plane++) {
> + dma_addr_t buf_addr;
> +
> + buf_addr = vb2_dma_contig_plane_dma_addr(&v_buf->vb2_buf,
> + plane);
> + writel(buf_addr, csi->regs + CSI_BUF_ADDR_REG(plane, slot));
> + }
> +
> + return 0;
> +}
> +
> +static int csi_buffer_fill_all(struct sun4i_csi *csi)
> +{
> + unsigned int slot;
> + int ret;
> +
> + for (slot = 0; slot < CSI_MAX_BUFFER; slot++) {
> + ret = csi_buffer_fill_slot(csi, slot);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void csi_buffer_mark_done(struct sun4i_csi *csi,
> + unsigned int slot,
> + unsigned int sequence)
> +{
> + struct vb2_v4l2_buffer *v_buf;
> +
> + if (WARN_ON(!csi->current_buf[slot]))
> + return;
> +
> + v_buf = csi->current_buf[slot];
> + v_buf->field = csi->v_fmt.field;
> + v_buf->sequence = sequence;
> + v_buf->vb2_buf.timestamp = ktime_get_ns();
> + vb2_buffer_done(&v_buf->vb2_buf, VB2_BUF_STATE_DONE);
> +
> + csi->current_buf[slot] = NULL;
> +}
> +
> +static int csi_buffer_flip(struct sun4i_csi *csi, unsigned int sequence)
> +{
> + u32 reg = readl(csi->regs + CSI_BUF_CTRL_REG);
> + int curr, next;
> +
> + /* Our next buffer is not the current buffer */
> + curr = !!(reg & CSI_BUF_CTRL_DBS);
> + next = !curr;
> +
> + /* Report the previous buffer as done */
> + csi_buffer_mark_done(csi, next, sequence);
> +
> + /* Put a new buffer in there */
> + return csi_buffer_fill_slot(csi, next);
> +}
> +
> +static void csi_buffer_queue(struct vb2_buffer *vb)
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vb->vb2_queue);
> + struct csi_buffer *buf = vb2_to_csi_buffer(vb);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&csi->qlock, flags);
> + list_add_tail(&buf->list, &csi->buf_list);
> + spin_unlock_irqrestore(&csi->qlock, flags);
> +}
> +
> +static void return_all_buffers(struct sun4i_csi *csi,
> + enum vb2_buffer_state state)
> +{
> + struct csi_buffer *buf, *node;
> + unsigned int slot;
> +
> + list_for_each_entry_safe(buf, node, &csi->buf_list, list) {
> + vb2_buffer_done(&buf->vb.vb2_buf, state);
> + list_del(&buf->list);
> + }
> +
> + for (slot = 0; slot < CSI_MAX_BUFFER; slot++) {
> + struct vb2_v4l2_buffer *v_buf = csi->current_buf[slot];
> +
> + if (!v_buf)
> + continue;
> +
> + vb2_buffer_done(&v_buf->vb2_buf, state);
> + csi->current_buf[slot] = NULL;
> + }
> +}
> +
> +static int csi_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vq);
> + struct v4l2_fwnode_bus_parallel *bus = &csi->bus;
> + unsigned long hsync_pol, pclk_pol, vsync_pol;
> + unsigned long flags;
> + int ret = 0;
> +
> + csi->sequence = 0;
> +
> + if (count < 2)
> + return -ENOBUFS;
Shouldn't be needed. Just fill in the min_buffers_needed field of struct vb2_queue.
> +
> + ret = media_pipeline_start(&csi->vdev.entity, &csi->vdev.pipe);
> + if (ret < 0)
> + goto clear_dma_queue;
> +
> + dev_dbg(csi->dev, "Starting capture\n");
> +
> + spin_lock_irqsave(&csi->qlock, flags);
> +
> + /* Setup timings */
> + writel(CSI_WIN_CTRL_W_ACTIVE(csi->v_fmt.width * 2),
> + csi->regs + CSI_WIN_CTRL_W_REG);
> + writel(CSI_WIN_CTRL_H_ACTIVE(csi->v_fmt.height),
> + csi->regs + CSI_WIN_CTRL_H_REG);
> +
> + hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
> + pclk_pol = !!(bus->flags & V4L2_MBUS_DATA_ACTIVE_HIGH);
> + vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
> + writel(CSI_CFG_INPUT_FMT(csi->p_fmt->input) |
> + CSI_CFG_OUTPUT_FMT(csi->p_fmt->output) |
> + CSI_CFG_VSYNC_POL(vsync_pol) |
> + CSI_CFG_HSYNC_POL(hsync_pol) |
> + CSI_CFG_PCLK_POL(pclk_pol),
> + csi->regs + CSI_CFG_REG);
> +
> + /* Setup buffer length */
> + writel(csi->v_fmt.plane_fmt[0].bytesperline,
> + csi->regs + CSI_BUF_LEN_REG);
> +
> + /* Prepare our buffers in hardware */
> + ret = csi_buffer_fill_all(csi);
> + if (ret) {
> + spin_unlock_irqrestore(&csi->qlock, flags);
> + goto disable_pipeline;
> + }
> +
> + /* Enable double buffering */
> + writel(CSI_BUF_CTRL_DBE, csi->regs + CSI_BUF_CTRL_REG);
> +
> + /* Clear the pending interrupts */
> + writel(CSI_INT_FRM_DONE, csi->regs + 0x34);
> +
> + /* Enable frame done interrupt */
> + writel(CSI_INT_FRM_DONE, csi->regs + CSI_INT_EN_REG);
> +
> + csi_capture_start(csi);
> +
> + spin_unlock_irqrestore(&csi->qlock, flags);
> +
> + ret = v4l2_subdev_call(csi->src_subdev, video, s_stream, 1);
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + goto disable_pipeline;
> +
> + return 0;
> +
> +disable_pipeline:
> + media_pipeline_stop(&csi->vdev.entity);
> +
> +clear_dma_queue:
> + return_all_buffers(csi, VB2_BUF_STATE_QUEUED);
> +
> + return ret;
> +}
> +
> +static void csi_stop_streaming(struct vb2_queue *vq)
> +{
> + struct sun4i_csi *csi = vb2_get_drv_priv(vq);
> + unsigned long flags;
> +
> + dev_dbg(csi->dev, "Stopping capture\n");
> +
> + v4l2_subdev_call(csi->src_subdev, video, s_stream, 0);
> + csi_capture_stop(csi);
> +
> + /* Release all active buffers */
> + spin_lock_irqsave(&csi->qlock, flags);
> + return_all_buffers(csi, VB2_BUF_STATE_ERROR);
> + spin_unlock_irqrestore(&csi->qlock, flags);
> +
> + media_pipeline_stop(&csi->vdev.entity);
> +}
> +
> +static struct vb2_ops csi_qops = {
> + .queue_setup = csi_queue_setup,
> + .buf_prepare = csi_buffer_prepare,
> + .buf_queue = csi_buffer_queue,
> + .start_streaming = csi_start_streaming,
> + .stop_streaming = csi_stop_streaming,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> +};
> +
> +static irqreturn_t csi_irq(int irq, void *data)
> +{
> + struct sun4i_csi *csi = data;
> + unsigned int sequence;
> + u32 reg;
> +
> + reg = readl(csi->regs + CSI_INT_STA_REG);
> +
> + /* Acknowledge the interrupts */
> + writel(reg, csi->regs + CSI_INT_STA_REG);
> +
> + sequence = csi->sequence++;
> +
> + if (!(reg & CSI_INT_FRM_DONE))
> + goto out;
> +
> + spin_lock(&csi->qlock);
> + if (csi_buffer_flip(csi, sequence)) {
> + dev_warn(csi->dev, "%s: Flip failed\n", __func__);
> + csi_capture_stop(csi);
> + }
> + spin_unlock(&csi->qlock);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +int csi_dma_register(struct sun4i_csi *csi, int irq)
> +{
> + struct vb2_queue *q = &csi->queue;
> + int ret;
> + int i;
> +
> + ret = v4l2_device_register(csi->dev, &csi->v4l);
> + if (ret) {
> + dev_err(csi->dev, "Couldn't register the v4l2 device\n");
> + return ret;
> + }
> +
> + spin_lock_init(&csi->qlock);
> + mutex_init(&csi->lock);
> +
> + INIT_LIST_HEAD(&csi->buf_list);
> + for (i = 0; i < CSI_MAX_BUFFER; i++)
> + csi->current_buf[i] = NULL;
> +
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> + q->io_modes = VB2_MMAP;
> + q->lock = &csi->lock;
> + q->drv_priv = csi;
> + q->buf_struct_size = sizeof(struct csi_buffer);
> + q->ops = &csi_qops;
> + q->mem_ops = &vb2_dma_contig_memops;
> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + q->gfp_flags = GFP_DMA32;
This is only needed for 64 bit architectures that require 32 bit DMA.
Since this is a 32 bit architecture, you don't need to set this field.
> + q->dev = csi->dev;
> +
> + ret = vb2_queue_init(q);
> + if (ret < 0) {
> + dev_err(csi->dev, "failed to initialize VB2 queue\n");
> + return ret;
> + }
> +
> + ret = devm_request_irq(csi->dev, irq, csi_irq, 0,
> + dev_name(csi->dev), csi);
> + if (ret) {
> + dev_err(csi->dev, "Couldn't register our interrupt\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +void csi_dma_unregister(struct sun4i_csi *csi)
> +{
> + v4l2_device_unregister(&csi->v4l);
> +}
> +
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> new file mode 100644
> index 000000000000..0b9487830e6d
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> @@ -0,0 +1,287 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2016 NextThing Co
> + * Copyright (C) 2016-2018 Bootlin
> + *
> + * Author: Maxime Ripard <maxime.ripard@bootlin.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#include "sun4i_csi.h"
> +
> +#define CSI_DEFAULT_FORMAT V4L2_PIX_FMT_YUV420M
> +#define CSI_DEFAULT_WIDTH 640
> +#define CSI_DEFAULT_HEIGHT 480
> +
> +#define CSI_MAX_HEIGHT 8192U
> +#define CSI_MAX_WIDTH 8192U
> +
> +static const struct sun4i_csi_format csi_formats[] = {
> + /* YUV422 inputs */
> + {
> + .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> + .fourcc = V4L2_PIX_FMT_YUV420M,
> + .input = CSI_INPUT_YUV,
> + .output = CSI_OUTPUT_YUV_420_PLANAR,
> + .num_planes = 3,
> + .bpp = { 8, 8, 8 },
> + .hsub = 2,
> + .vsub = 2,
> + },
> +};
> +
> +static const struct sun4i_csi_format *
> +csi_get_format_by_fourcc(struct sun4i_csi *csi, u32 fourcc)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(csi_formats); i++)
> + if (csi_formats[i].fourcc == fourcc)
> + return &csi_formats[i];
> +
> + return NULL;
> +}
> +
> +static int csi_querycap(struct file *file, void *priv,
> + struct v4l2_capability *cap)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> +
> + strscpy(cap->driver, KBUILD_MODNAME, sizeof(cap->driver));
> + strscpy(cap->card, "sun4i-csi", sizeof(cap->card));
> + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> + dev_name(csi->dev));
> + cap->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_STREAMING;
> + cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
Don't set device_caps/capabilities here. Instead, fill in device_caps in
struct video_device and the core will fill in these fields for you.
> +
> + return 0;
> +}
> +
> +static int csi_enum_input(struct file *file, void *priv,
> + struct v4l2_input *inp)
> +{
> + if (inp->index != 0)
> + return -EINVAL;
> +
> + inp->type = V4L2_INPUT_TYPE_CAMERA;
> + strlcpy(inp->name, "Camera", sizeof(inp->name));
strscpy.
Please do a search and replace.
> +
> + return 0;
> +}
> +
> +static int csi_g_input(struct file *file, void *fh,
> + unsigned int *i)
> +{
> + *i = 0;
> +
> + return 0;
> +}
> +
> +static int csi_s_input(struct file *file, void *fh,
> + unsigned int i)
> +{
> + if (i != 0)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int _csi_try_fmt(struct sun4i_csi *csi,
> + struct v4l2_pix_format_mplane *pix,
> + const struct sun4i_csi_format **fmt)
> +{
> + const struct sun4i_csi_format *_fmt;
> + unsigned int width = pix->width;
> + unsigned int height = pix->height;
> + int i;
> +
> + _fmt = csi_get_format_by_fourcc(csi, pix->pixelformat);
> + if (!_fmt)
> + _fmt = csi_get_format_by_fourcc(csi, csi_formats[0].fourcc);
> +
> + pix->field = V4L2_FIELD_NONE;
> + pix->colorspace = V4L2_COLORSPACE_SRGB;
> + pix->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(pix->colorspace);
> + pix->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
> + pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true, pix->colorspace,
> + pix->ycbcr_enc);
> +
> + pix->num_planes = _fmt->num_planes;
> + pix->pixelformat = _fmt->fourcc;
> +
> + memset(pix->reserved, 0, sizeof(pix->reserved));
> +
> + /* Align the width and height on the subsampling */
> + width = ALIGN(width, _fmt->hsub);
> + height = ALIGN(height, _fmt->vsub);
> +
> + /* Clamp the width and height to our capabilities */
> + pix->width = clamp(width, _fmt->hsub, CSI_MAX_WIDTH);
> + pix->height = clamp(height, _fmt->vsub, CSI_MAX_HEIGHT);
> +
> + for (i = 0; i < _fmt->num_planes; i++) {
> + unsigned int hsub = i > 0 ? _fmt->hsub : 1;
> + unsigned int vsub = i > 0 ? _fmt->vsub : 1;
> + unsigned int bpl;
> +
> + bpl = pix->width / hsub * _fmt->bpp[i] / 8;
> + pix->plane_fmt[i].bytesperline = bpl;
> + pix->plane_fmt[i].sizeimage = bpl * pix->height / vsub;
> + memset(pix->plane_fmt[i].reserved, 0,
> + sizeof(pix->plane_fmt[i].reserved));
> + }
> +
> + if (fmt)
> + *fmt = _fmt;
> +
> + return 0;
> +}
> +
> +static int csi_try_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> +
> + if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
No need for this check.
> +
> + return _csi_try_fmt(csi, &f->fmt.pix_mp, NULL);
> +}
> +
> +static int csi_s_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + const struct sun4i_csi_format *fmt;
> + struct sun4i_csi *csi = video_drvdata(file);
> + int ret;
> +
> + if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
No need for this check.
> +
> + ret = _csi_try_fmt(csi, &f->fmt.pix_mp, &fmt);
> + if (ret)
> + return ret;
> +
> + csi->v_fmt = f->fmt.pix_mp;
> + csi->p_fmt = fmt;
> +
> + return 0;
> +}
> +
> +static int csi_g_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> +
> + if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> + return -EINVAL;
Drop check.
> +
> + f->fmt.pix_mp = csi->v_fmt;
> +
> + return 0;
> +}
> +
> +static int csi_enum_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_fmtdesc *f)
> +{
> + if (f->index >= ARRAY_SIZE(csi_formats))
> + return -EINVAL;
> +
> + f->pixelformat = csi_formats[f->index].fourcc;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops csi_ioctl_ops = {
> + .vidioc_querycap = csi_querycap,
> +
> + .vidioc_enum_fmt_vid_cap_mplane = csi_enum_fmt_vid_cap,
> + .vidioc_g_fmt_vid_cap_mplane = csi_g_fmt_vid_cap,
> + .vidioc_s_fmt_vid_cap_mplane = csi_s_fmt_vid_cap,
> + .vidioc_try_fmt_vid_cap_mplane = csi_try_fmt_vid_cap,
> +
> + .vidioc_enum_input = csi_enum_input,
> + .vidioc_g_input = csi_g_input,
> + .vidioc_s_input = csi_s_input,
> +
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
Ah, create_bufs is supported here, but in that case queue_setup
is wrong. Look at other drivers on how to do this.
> + .vidioc_querybuf = vb2_ioctl_querybuf,
> + .vidioc_qbuf = vb2_ioctl_qbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_expbuf = vb2_ioctl_expbuf,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> +};
> +
> +static int csi_open(struct file *file)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> + int ret;
> +
> + pm_runtime_get_sync(csi->dev);
> +
> + ret = v4l2_subdev_call(csi->src_subdev, core, s_power, 1);
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + return ret;
> +
> + return v4l2_fh_open(file);
> +}
> +
> +static int csi_release(struct file *file)
> +{
> + struct sun4i_csi *csi = video_drvdata(file);
> +
> + pm_runtime_put(csi->dev);
No call to v4l2_subdev_call(csi->src_subdev, core, s_power, 0); needed here if
the last fh is closed?
> +
> + return vb2_fop_release(file);
> +}
> +
> +static const struct v4l2_file_operations csi_fops = {
> + .owner = THIS_MODULE,
> + .open = csi_open,
> + .release = csi_release,
> + .unlocked_ioctl = video_ioctl2,
> + .read = vb2_fop_read,
> + .write = vb2_fop_write,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +};
> +
> +int csi_v4l2_register(struct sun4i_csi *csi)
> +{
> + struct video_device *vdev = &csi->vdev;
> + int ret;
> +
> + vdev->v4l2_dev = &csi->v4l;
> + vdev->queue = &csi->queue;
> + strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> + vdev->release = video_device_release_empty;
> + vdev->lock = &csi->lock;
> +
> + /* Set a default format */
> + csi->v_fmt.pixelformat = CSI_DEFAULT_FORMAT;
> + csi->v_fmt.width = CSI_DEFAULT_WIDTH;
> + csi->v_fmt.height = CSI_DEFAULT_HEIGHT;
> + _csi_try_fmt(csi, &csi->v_fmt, NULL);
> +
> + vdev->fops = &csi_fops;
> + vdev->ioctl_ops = &csi_ioctl_ops;
> + video_set_drvdata(vdev, csi);
> +
> + ret = video_register_device(&csi->vdev, VFL_TYPE_GRABBER, -1);
> + if (ret)
> + return ret;
> +
> + dev_info(csi->dev, "Device registered as %s\n",
> + video_device_node_name(vdev));
> +
> + return 0;
> +}
> +
>
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-13 8:24 [PATCH 0/5] media: Allwinner A10 CSI support Maxime Ripard
` (4 preceding siblings ...)
2018-11-13 8:24 ` [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support Maxime Ripard
@ 2018-11-13 12:30 ` Hans Verkuil
2018-11-13 13:52 ` Maxime Ripard
2018-11-14 3:24 ` Chen-Yu Tsai
6 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2018-11-13 12:30 UTC (permalink / raw)
To: Maxime Ripard, Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
On 11/13/18 09:24, Maxime Ripard wrote:
> Hi,
>
> Here is a series introducing the support for the A10 (and SoCs of the same
> generation) CMOS Sensor Interface (called CSI, not to be confused with
> MIPI-CSI, which isn't support by that IP).
>
> That interface is pretty straightforward, but the driver has a few issues
> that I wanted to bring up:
>
> * The only board I've been testing this with has an ov5640 sensor
> attached, which doesn't work with the upstream driver. Copying the
> Allwinner init sequence works though, and this is how it has been
> tested. Testing with a second sensor would allow to see if it's an
> issue on the CSI side or the sensor side.
> * When starting a capture, the last buffer to capture will fail due to
> double buffering being used, and we don't have a next buffer for the
> last frame. I'm not sure how to deal with that though. It seems like
> some drivers use a scratch buffer in such a case, some don't care, so
> I'm not sure which solution should be preferred.
> * We don't have support for the ISP at the moment, but this can be added
> eventually.
>
> * How to model the CSI module clock isn't really clear to me. It looks
> like it goes through the CSI controller and then is muxed to one of the
> CSI pin so that it can clock the sensor. I'm not quite sure how to
> model it, if it should be a clock, the CSI driver being a clock
> provider, or if the sensor should just use the module clock directly.
>
> Here is the v4l2-compliance output:
Test v4l2-compliance with the -s option so you test streaming as well.
Even better is -f where it tests streaming with all available formats.
> v4l2-compliance SHA : 339d550e92ac15de8668f32d66d16f198137006c
Hmm, I can't find this SHA. Was this built from the main v4l-utils repo?
Regards,
Hans
>
> Driver Info:
> Driver name : sun4i_csi
> Card type : sun4i-csi
> Bus info : platform:1c09000.csi
> Driver version: 4.19.0
> Capabilities : 0x84201000
> Video Capture Multiplanar
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps : 0x04201000
> Video Capture Multiplanar
> 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 (Not Supported)
>
> 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 (Not Supported)
> test VIDIOC_QUERYCTRL: OK (Not Supported)
> test VIDIOC_G/S_CTRL: OK (Not Supported)
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 0 Private Controls: 0
>
> Format ioctls:
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK (Not Supported)
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> test Scaling: OK
>
> Codec ioctls:
> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>
> Buffer ioctls:
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> test VIDIOC_EXPBUF: OK
>
> Test input 0:
>
> Total: 43, Succeeded: 43, Failed: 0, Warnings: 0
>
> Let me know what you think,
> Maxime
>
> Maxime Ripard (5):
> dt-bindings: media: Add Allwinner A10 CSI binding
> media: sunxi: Refactor the Makefile and Kconfig
> media: sunxi: Add A10 CSI driver
> ARM: dts: sun7i: Add CSI0 controller
> DO NOT MERGE: ARM: dts: bananapi: Add Camera support
>
> Documentation/devicetree/bindings/media/sun4i-csi.txt | 71 ++-
> arch/arm/boot/dts/sun7i-a20-bananapi.dts | 98 +++-
> arch/arm/boot/dts/sun7i-a20.dtsi | 13 +-
> drivers/media/platform/Kconfig | 2 +-
> drivers/media/platform/Makefile | 2 +-
> drivers/media/platform/sunxi/Kconfig | 2 +-
> drivers/media/platform/sunxi/Makefile | 2 +-
> drivers/media/platform/sunxi/sun4i-csi/Kconfig | 12 +-
> drivers/media/platform/sunxi/sun4i-csi/Makefile | 5 +-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 275 ++++++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h | 137 ++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 383 +++++++++++-
> drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c | 287 ++++++++-
> 13 files changed, 1287 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/media/sun4i-csi.txt
> create mode 100644 drivers/media/platform/sunxi/Kconfig
> create mode 100644 drivers/media/platform/sunxi/Makefile
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Kconfig
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/Makefile
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> create mode 100644 drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
>
> base-commit: 94517eaa3d43005472864615dfd17f1ef6ca3935
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 8:24 ` [PATCH 3/5] media: sunxi: Add A10 CSI driver Maxime Ripard
2018-11-13 8:57 ` Sakari Ailus
2018-11-13 12:24 ` Hans Verkuil
@ 2018-11-13 12:48 ` Fabio Estevam
2018-11-13 13:37 ` Hans Verkuil
2 siblings, 1 reply; 36+ messages in thread
From: Fabio Estevam @ 2018-11-13 12:48 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mark Rutland, Rob Herring, Frank Rowand
On Tue, Nov 13, 2018 at 6:25 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
According to Documentation/process/license-rules.rst this should be:
+// SPDX-License-Identifier: GPL-2.0+
Same applies to other places in this patch.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 12:48 ` Fabio Estevam
@ 2018-11-13 13:37 ` Hans Verkuil
2018-11-13 14:13 ` Fabio Estevam
0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2018-11-13 13:37 UTC (permalink / raw)
To: Fabio Estevam, Maxime Ripard
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mark Rutland, Rob Herring, Frank Rowand
On 11/13/18 13:48, Fabio Estevam wrote:
> On Tue, Nov 13, 2018 at 6:25 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
>> --- /dev/null
>> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
>> @@ -0,0 +1,275 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>
> According to Documentation/process/license-rules.rst this should be:
>
> +// SPDX-License-Identifier: GPL-2.0+
>
> Same applies to other places in this patch.
>
Actually, LICENSES/preferred/GPL-2.0 has GPL-2.0-or-later
as a valid license:
Valid-License-Identifier: GPL-2.0-or-later
Personally I very much prefer GPL-2.0-or-later since I think it is
much clearer.
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-13 12:30 ` [PATCH 0/5] media: Allwinner A10 CSI support Hans Verkuil
@ 2018-11-13 13:52 ` Maxime Ripard
2018-11-13 14:01 ` Hans Verkuil
0 siblings, 1 reply; 36+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:52 UTC (permalink / raw)
To: Thomas Petazzoni, Hans Verkuil
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]
Hi Hans,
On Tue, Nov 13, 2018 at 01:30:49PM +0100, Hans Verkuil wrote:
> On 11/13/18 09:24, Maxime Ripard wrote:
> > Hi,
> >
> > Here is a series introducing the support for the A10 (and SoCs of the same
> > generation) CMOS Sensor Interface (called CSI, not to be confused with
> > MIPI-CSI, which isn't support by that IP).
> >
> > That interface is pretty straightforward, but the driver has a few issues
> > that I wanted to bring up:
> >
> > * The only board I've been testing this with has an ov5640 sensor
> > attached, which doesn't work with the upstream driver. Copying the
> > Allwinner init sequence works though, and this is how it has been
> > tested. Testing with a second sensor would allow to see if it's an
> > issue on the CSI side or the sensor side.
> > * When starting a capture, the last buffer to capture will fail due to
> > double buffering being used, and we don't have a next buffer for the
> > last frame. I'm not sure how to deal with that though. It seems like
> > some drivers use a scratch buffer in such a case, some don't care, so
> > I'm not sure which solution should be preferred.
> > * We don't have support for the ISP at the moment, but this can be added
> > eventually.
> >
> > * How to model the CSI module clock isn't really clear to me. It looks
> > like it goes through the CSI controller and then is muxed to one of the
> > CSI pin so that it can clock the sensor. I'm not quite sure how to
> > model it, if it should be a clock, the CSI driver being a clock
> > provider, or if the sensor should just use the module clock directly.
> >
> > Here is the v4l2-compliance output:
>
> Test v4l2-compliance with the -s option so you test streaming as well.
> Even better is -f where it tests streaming with all available formats.
I will, thanks for the tip!
> > v4l2-compliance SHA : 339d550e92ac15de8668f32d66d16f198137006c
>
> Hmm, I can't find this SHA. Was this built from the main v4l-utils repo?
It was, but using Buildroot. The version packaged in the latest stable
version I was using (2018.08) is 1.14.2.
Looking at the Makefile from v4l2-compliance, it looks like it just
invokes git to retrieve the git commit and uses that as the hash. In
Buildroot's case, since buildroot will download the tarball, this will
end up returning the SHA commit of the buildroot repo building the
sources, not the version of the sources themselves.
I'm not sure how to address that properly though. Thomas, how do you
usually deal with this?
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-13 13:52 ` Maxime Ripard
@ 2018-11-13 14:01 ` Hans Verkuil
2018-11-13 15:52 ` Maxime Ripard
0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2018-11-13 14:01 UTC (permalink / raw)
To: Maxime Ripard, Thomas Petazzoni, Hans Verkuil
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
On 11/13/18 14:52, Maxime Ripard wrote:
> Hi Hans,
>
> On Tue, Nov 13, 2018 at 01:30:49PM +0100, Hans Verkuil wrote:
>> On 11/13/18 09:24, Maxime Ripard wrote:
>>> Hi,
>>>
>>> Here is a series introducing the support for the A10 (and SoCs of the same
>>> generation) CMOS Sensor Interface (called CSI, not to be confused with
>>> MIPI-CSI, which isn't support by that IP).
>>>
>>> That interface is pretty straightforward, but the driver has a few issues
>>> that I wanted to bring up:
>>>
>>> * The only board I've been testing this with has an ov5640 sensor
>>> attached, which doesn't work with the upstream driver. Copying the
>>> Allwinner init sequence works though, and this is how it has been
>>> tested. Testing with a second sensor would allow to see if it's an
>>> issue on the CSI side or the sensor side.
>>> * When starting a capture, the last buffer to capture will fail due to
>>> double buffering being used, and we don't have a next buffer for the
>>> last frame. I'm not sure how to deal with that though. It seems like
>>> some drivers use a scratch buffer in such a case, some don't care, so
>>> I'm not sure which solution should be preferred.
>>> * We don't have support for the ISP at the moment, but this can be added
>>> eventually.
>>>
>>> * How to model the CSI module clock isn't really clear to me. It looks
>>> like it goes through the CSI controller and then is muxed to one of the
>>> CSI pin so that it can clock the sensor. I'm not quite sure how to
>>> model it, if it should be a clock, the CSI driver being a clock
>>> provider, or if the sensor should just use the module clock directly.
>>>
>>> Here is the v4l2-compliance output:
>>
>> Test v4l2-compliance with the -s option so you test streaming as well.
>> Even better is -f where it tests streaming with all available formats.
>
> I will, thanks for the tip!
>
>>> v4l2-compliance SHA : 339d550e92ac15de8668f32d66d16f198137006c
>>
>> Hmm, I can't find this SHA. Was this built from the main v4l-utils repo?
>
> It was, but using Buildroot. The version packaged in the latest stable
> version I was using (2018.08) is 1.14.2.
That's seriously out of date. That's why I show the SHA, to see if
someone is testing with a recent version of the utility, so it served
its purpose here :-)
Latest release is 1.16.2.
But when submitting new drivers you really need to build it yourself from
the master branch, that's the only way to be sure you have all the latest
compliance checks.
>
> Looking at the Makefile from v4l2-compliance, it looks like it just
> invokes git to retrieve the git commit and uses that as the hash. In
> Buildroot's case, since buildroot will download the tarball, this will
> end up returning the SHA commit of the buildroot repo building the
> sources, not the version of the sources themselves.
>
> I'm not sure how to address that properly though. Thomas, how do you
> usually deal with this?
Note that cec-compliance and cec-follower do the same, for the same
reason.
Where does the tarball come from?
Regards,
Hans
>
> Thanks!
> Maxime
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 13:37 ` Hans Verkuil
@ 2018-11-13 14:13 ` Fabio Estevam
2018-11-13 14:46 ` Thomas Petazzoni
0 siblings, 1 reply; 36+ messages in thread
From: Fabio Estevam @ 2018-11-13 14:13 UTC (permalink / raw)
To: Hans Verkuil, Greg Kroah-Hartman
Cc: Maxime Ripard, Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mark Rutland, Rob Herring, Frank Rowand
Hi Hans,
On Tue, Nov 13, 2018 at 11:37 AM Hans Verkuil <hansverk@cisco.com> wrote:
>
> On 11/13/18 13:48, Fabio Estevam wrote:
> > On Tue, Nov 13, 2018 at 6:25 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> >> --- /dev/null
> >> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> >> @@ -0,0 +1,275 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >
> > According to Documentation/process/license-rules.rst this should be:
> >
> > +// SPDX-License-Identifier: GPL-2.0+
> >
> > Same applies to other places in this patch.
> >
>
> Actually, LICENSES/preferred/GPL-2.0 has GPL-2.0-or-later
> as a valid license:
>
> Valid-License-Identifier: GPL-2.0-or-later
>
> Personally I very much prefer GPL-2.0-or-later since I think it is
> much clearer.
I saw feedback from Greg to use the SPDX style from
Documentation/process/license-rules.rst.
Please check:
https://lkml.org/lkml/2018/11/10/232
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 14:13 ` Fabio Estevam
@ 2018-11-13 14:46 ` Thomas Petazzoni
0 siblings, 0 replies; 36+ messages in thread
From: Thomas Petazzoni @ 2018-11-13 14:46 UTC (permalink / raw)
To: Fabio Estevam
Cc: Hans Verkuil, Greg Kroah-Hartman, Maxime Ripard, Hans Verkuil,
Sakari Ailus, Mauro Carvalho Chehab, Laurent Pinchart,
linux-media, Andrzej Hajda, Chen-Yu Tsai, linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mark Rutland, Rob Herring, Frank Rowand
Hello,
On Tue, 13 Nov 2018 12:13:09 -0200, Fabio Estevam wrote:
> > Actually, LICENSES/preferred/GPL-2.0 has GPL-2.0-or-later
> > as a valid license:
> >
> > Valid-License-Identifier: GPL-2.0-or-later
> >
> > Personally I very much prefer GPL-2.0-or-later since I think it is
> > much clearer.
>
> I saw feedback from Greg to use the SPDX style from
> Documentation/process/license-rules.rst.
>
> Please check:
> https://lkml.org/lkml/2018/11/10/232
According to https://spdx.org/licenses/, i.e the reference SPDX site,
GPL-2.0-or-later is the right thing to use, and GPL-2.0+ is
"deprecated". But apparently Greg's feedback is "let's not used the
SDPX style, as we don't want to change this all over".
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 12:24 ` Hans Verkuil
@ 2018-11-13 15:19 ` Joe Perches
2018-11-13 15:39 ` Hans Verkuil
2018-11-15 20:51 ` Maxime Ripard
1 sibling, 1 reply; 36+ messages in thread
From: Joe Perches @ 2018-11-13 15:19 UTC (permalink / raw)
To: Hans Verkuil, Maxime Ripard, Hans Verkuil, Sakari Ailus,
Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
On Tue, 2018-11-13 at 13:24 +0100, Hans Verkuil wrote:
> On 11/13/18 09:24, Maxime Ripard wrote:
> > The older CSI drivers have camera capture interface different from the one
> > in the newer ones.
[]
> > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
[]
> > + // Videobuf2
>
> Doesn't checkpatch.pl --strict complain about the use of '//'?
No, not since
commit dadf680de3c2eb4cba9840619991eda0cfe98778
Author: Joe Perches <joe@perches.com>
Date: Tue Aug 2 14:04:33 2016 -0700
checkpatch: allow c99 style // comments
Sanitise the lines that contain c99 comments so that the error doesn't
get emitted.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 15:19 ` Joe Perches
@ 2018-11-13 15:39 ` Hans Verkuil
0 siblings, 0 replies; 36+ messages in thread
From: Hans Verkuil @ 2018-11-13 15:39 UTC (permalink / raw)
To: Joe Perches, Hans Verkuil, Maxime Ripard, Hans Verkuil,
Sakari Ailus, Mauro Carvalho Chehab
Cc: Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
On 11/13/18 16:19, Joe Perches wrote:
> On Tue, 2018-11-13 at 13:24 +0100, Hans Verkuil wrote:
>> On 11/13/18 09:24, Maxime Ripard wrote:
>>> The older CSI drivers have camera capture interface different from the one
>>> in the newer ones.
> []
>>> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> []
>>> + // Videobuf2
>>
>> Doesn't checkpatch.pl --strict complain about the use of '//'?
>
> No, not since
>
> commit dadf680de3c2eb4cba9840619991eda0cfe98778
> Author: Joe Perches <joe@perches.com>
> Date: Tue Aug 2 14:04:33 2016 -0700
>
> checkpatch: allow c99 style // comments
>
> Sanitise the lines that contain c99 comments so that the error doesn't
> get emitted.
>
>
Huh, I'm really out of date. But the good news is that I learned something
new today!
Thank you,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-13 14:01 ` Hans Verkuil
@ 2018-11-13 15:52 ` Maxime Ripard
2018-11-13 16:00 ` Hans Verkuil
0 siblings, 1 reply; 36+ messages in thread
From: Maxime Ripard @ 2018-11-13 15:52 UTC (permalink / raw)
To: Hans Verkuil
Cc: Thomas Petazzoni, Hans Verkuil, Hans Verkuil, Sakari Ailus,
Mauro Carvalho Chehab, Laurent Pinchart, linux-media,
Andrzej Hajda, Chen-Yu Tsai, linux-kernel, linux-arm-kernel,
devicetree, Mark Rutland, Rob Herring, Frank Rowand
[-- Attachment #1: Type: text/plain, Size: 3640 bytes --]
On Tue, Nov 13, 2018 at 03:01:45PM +0100, Hans Verkuil wrote:
> On 11/13/18 14:52, Maxime Ripard wrote:
> > Hi Hans,
> >
> > On Tue, Nov 13, 2018 at 01:30:49PM +0100, Hans Verkuil wrote:
> >> On 11/13/18 09:24, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> Here is a series introducing the support for the A10 (and SoCs of the same
> >>> generation) CMOS Sensor Interface (called CSI, not to be confused with
> >>> MIPI-CSI, which isn't support by that IP).
> >>>
> >>> That interface is pretty straightforward, but the driver has a few issues
> >>> that I wanted to bring up:
> >>>
> >>> * The only board I've been testing this with has an ov5640 sensor
> >>> attached, which doesn't work with the upstream driver. Copying the
> >>> Allwinner init sequence works though, and this is how it has been
> >>> tested. Testing with a second sensor would allow to see if it's an
> >>> issue on the CSI side or the sensor side.
> >>> * When starting a capture, the last buffer to capture will fail due to
> >>> double buffering being used, and we don't have a next buffer for the
> >>> last frame. I'm not sure how to deal with that though. It seems like
> >>> some drivers use a scratch buffer in such a case, some don't care, so
> >>> I'm not sure which solution should be preferred.
> >>> * We don't have support for the ISP at the moment, but this can be added
> >>> eventually.
> >>>
> >>> * How to model the CSI module clock isn't really clear to me. It looks
> >>> like it goes through the CSI controller and then is muxed to one of the
> >>> CSI pin so that it can clock the sensor. I'm not quite sure how to
> >>> model it, if it should be a clock, the CSI driver being a clock
> >>> provider, or if the sensor should just use the module clock directly.
> >>>
> >>> Here is the v4l2-compliance output:
> >>
> >> Test v4l2-compliance with the -s option so you test streaming as well.
> >> Even better is -f where it tests streaming with all available formats.
> >
> > I will, thanks for the tip!
> >
> >>> v4l2-compliance SHA : 339d550e92ac15de8668f32d66d16f198137006c
> >>
> >> Hmm, I can't find this SHA. Was this built from the main v4l-utils repo?
> >
> > It was, but using Buildroot. The version packaged in the latest stable
> > version I was using (2018.08) is 1.14.2.
>
> That's seriously out of date. That's why I show the SHA, to see if
> someone is testing with a recent version of the utility, so it served
> its purpose here :-)
>
> Latest release is 1.16.2.
>
> But when submitting new drivers you really need to build it yourself from
> the master branch, that's the only way to be sure you have all the latest
> compliance checks.
Ack, I'll update it and test again then.
> >
> > Looking at the Makefile from v4l2-compliance, it looks like it just
> > invokes git to retrieve the git commit and uses that as the hash. In
> > Buildroot's case, since buildroot will download the tarball, this will
> > end up returning the SHA commit of the buildroot repo building the
> > sources, not the version of the sources themselves.
> >
> > I'm not sure how to address that properly though. Thomas, how do you
> > usually deal with this?
>
> Note that cec-compliance and cec-follower do the same, for the same
> reason.
>
> Where does the tarball come from?
This is the official tarball from linuxtv:
https://git.buildroot.net/buildroot/tree/package/libv4l/libv4l.mk?h=2018.08.2#n8
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-13 15:52 ` Maxime Ripard
@ 2018-11-13 16:00 ` Hans Verkuil
2018-11-13 16:55 ` Thomas Petazzoni
0 siblings, 1 reply; 36+ messages in thread
From: Hans Verkuil @ 2018-11-13 16:00 UTC (permalink / raw)
To: Maxime Ripard
Cc: Thomas Petazzoni, Hans Verkuil, Hans Verkuil, Sakari Ailus,
Mauro Carvalho Chehab, Laurent Pinchart, linux-media,
Andrzej Hajda, Chen-Yu Tsai, linux-kernel, linux-arm-kernel,
devicetree, Mark Rutland, Rob Herring, Frank Rowand
On 11/13/18 16:52, Maxime Ripard wrote:
> On Tue, Nov 13, 2018 at 03:01:45PM +0100, Hans Verkuil wrote:
>> On 11/13/18 14:52, Maxime Ripard wrote:
>>> Hi Hans,
>>>
>>> On Tue, Nov 13, 2018 at 01:30:49PM +0100, Hans Verkuil wrote:
>>>> On 11/13/18 09:24, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> Here is a series introducing the support for the A10 (and SoCs of the same
>>>>> generation) CMOS Sensor Interface (called CSI, not to be confused with
>>>>> MIPI-CSI, which isn't support by that IP).
>>>>>
>>>>> That interface is pretty straightforward, but the driver has a few issues
>>>>> that I wanted to bring up:
>>>>>
>>>>> * The only board I've been testing this with has an ov5640 sensor
>>>>> attached, which doesn't work with the upstream driver. Copying the
>>>>> Allwinner init sequence works though, and this is how it has been
>>>>> tested. Testing with a second sensor would allow to see if it's an
>>>>> issue on the CSI side or the sensor side.
>>>>> * When starting a capture, the last buffer to capture will fail due to
>>>>> double buffering being used, and we don't have a next buffer for the
>>>>> last frame. I'm not sure how to deal with that though. It seems like
>>>>> some drivers use a scratch buffer in such a case, some don't care, so
>>>>> I'm not sure which solution should be preferred.
>>>>> * We don't have support for the ISP at the moment, but this can be added
>>>>> eventually.
>>>>>
>>>>> * How to model the CSI module clock isn't really clear to me. It looks
>>>>> like it goes through the CSI controller and then is muxed to one of the
>>>>> CSI pin so that it can clock the sensor. I'm not quite sure how to
>>>>> model it, if it should be a clock, the CSI driver being a clock
>>>>> provider, or if the sensor should just use the module clock directly.
>>>>>
>>>>> Here is the v4l2-compliance output:
>>>>
>>>> Test v4l2-compliance with the -s option so you test streaming as well.
>>>> Even better is -f where it tests streaming with all available formats.
>>>
>>> I will, thanks for the tip!
>>>
>>>>> v4l2-compliance SHA : 339d550e92ac15de8668f32d66d16f198137006c
>>>>
>>>> Hmm, I can't find this SHA. Was this built from the main v4l-utils repo?
>>>
>>> It was, but using Buildroot. The version packaged in the latest stable
>>> version I was using (2018.08) is 1.14.2.
>>
>> That's seriously out of date. That's why I show the SHA, to see if
>> someone is testing with a recent version of the utility, so it served
>> its purpose here :-)
>>
>> Latest release is 1.16.2.
>>
>> But when submitting new drivers you really need to build it yourself from
>> the master branch, that's the only way to be sure you have all the latest
>> compliance checks.
>
> Ack, I'll update it and test again then.
>
>>>
>>> Looking at the Makefile from v4l2-compliance, it looks like it just
>>> invokes git to retrieve the git commit and uses that as the hash. In
>>> Buildroot's case, since buildroot will download the tarball, this will
>>> end up returning the SHA commit of the buildroot repo building the
>>> sources, not the version of the sources themselves.
>>>
>>> I'm not sure how to address that properly though. Thomas, how do you
>>> usually deal with this?
>>
>> Note that cec-compliance and cec-follower do the same, for the same
>> reason.
>>
>> Where does the tarball come from?
>
> This is the official tarball from linuxtv:
> https://git.buildroot.net/buildroot/tree/package/libv4l/libv4l.mk?h=2018.08.2#n8
Weird, if I build directly from that tarball, then v4l2-compliance should say:
v4l2-compliance SHA: not available, 64 bits
So that's what I expect to see from buildroot as well.
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-13 16:00 ` Hans Verkuil
@ 2018-11-13 16:55 ` Thomas Petazzoni
2018-11-13 17:15 ` Hans Verkuil
0 siblings, 1 reply; 36+ messages in thread
From: Thomas Petazzoni @ 2018-11-13 16:55 UTC (permalink / raw)
To: Hans Verkuil
Cc: Maxime Ripard, Hans Verkuil, Hans Verkuil, Sakari Ailus,
Mauro Carvalho Chehab, Laurent Pinchart, linux-media,
Andrzej Hajda, Chen-Yu Tsai, linux-kernel, linux-arm-kernel,
devicetree, Mark Rutland, Rob Herring, Frank Rowand
Hello,
On Tue, 13 Nov 2018 17:00:25 +0100, Hans Verkuil wrote:
> Weird, if I build directly from that tarball, then v4l2-compliance should say:
>
> v4l2-compliance SHA: not available, 64 bits
>
> So that's what I expect to see from buildroot as well.
Indeed, that's strange, I see that the v4l2-compliance Makefile does:
version.h:
@if git -C $(srcdir) rev-parse HEAD >/dev/null 2>&1; then \
echo -n "#define SHA " >$@ ; \
git -C $(srcdir) rev-parse HEAD >>$@ ; \
else \
touch $@ ; \
fi
which correctly uses $(srcdir), so it shouldn't go "up" the libv4l
build folder and pick up the latest Buildroot commit SHA1. I'll have a
quick look.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-13 16:55 ` Thomas Petazzoni
@ 2018-11-13 17:15 ` Hans Verkuil
0 siblings, 0 replies; 36+ messages in thread
From: Hans Verkuil @ 2018-11-13 17:15 UTC (permalink / raw)
To: Thomas Petazzoni, Hans Verkuil
Cc: Maxime Ripard, Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
On 11/13/2018 05:55 PM, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 13 Nov 2018 17:00:25 +0100, Hans Verkuil wrote:
>
>> Weird, if I build directly from that tarball, then v4l2-compliance should say:
>>
>> v4l2-compliance SHA: not available, 64 bits
>>
>> So that's what I expect to see from buildroot as well.
>
> Indeed, that's strange, I see that the v4l2-compliance Makefile does:
>
> version.h:
> @if git -C $(srcdir) rev-parse HEAD >/dev/null 2>&1; then \
> echo -n "#define SHA " >$@ ; \
> git -C $(srcdir) rev-parse HEAD >>$@ ; \
> else \
> touch $@ ; \
> fi
>
> which correctly uses $(srcdir), so it shouldn't go "up" the libv4l
> build folder and pick up the latest Buildroot commit SHA1. I'll have a
> quick look.
I think it does, actually. If the tar archive is unpacked inside the
checked-out buildroot git tree, then it will pick up the buildroot SHA.
I fixed v4l-utils to be a bit smarter about this:
https://git.linuxtv.org/v4l-utils.git/patch/?id=98b4c9f276a18535b5691e5f350f59ffbf5a9aa5
Regards,
Hans
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-13 8:24 [PATCH 0/5] media: Allwinner A10 CSI support Maxime Ripard
` (5 preceding siblings ...)
2018-11-13 12:30 ` [PATCH 0/5] media: Allwinner A10 CSI support Hans Verkuil
@ 2018-11-14 3:24 ` Chen-Yu Tsai
2018-11-15 19:10 ` Maxime Ripard
6 siblings, 1 reply; 36+ messages in thread
From: Chen-Yu Tsai @ 2018-11-14 3:24 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, Laurent Pinchart, Linux Media Mailing List,
Andrzej Hajda, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
On Tue, Nov 13, 2018 at 4:24 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi,
>
> Here is a series introducing the support for the A10 (and SoCs of the same
> generation) CMOS Sensor Interface (called CSI, not to be confused with
> MIPI-CSI, which isn't support by that IP).
>
> That interface is pretty straightforward, but the driver has a few issues
> that I wanted to bring up:
>
> * The only board I've been testing this with has an ov5640 sensor
> attached, which doesn't work with the upstream driver. Copying the
> Allwinner init sequence works though, and this is how it has been
> tested. Testing with a second sensor would allow to see if it's an
> issue on the CSI side or the sensor side.
> * When starting a capture, the last buffer to capture will fail due to
> double buffering being used, and we don't have a next buffer for the
> last frame. I'm not sure how to deal with that though. It seems like
> some drivers use a scratch buffer in such a case, some don't care, so
> I'm not sure which solution should be preferred.
> * We don't have support for the ISP at the moment, but this can be added
> eventually.
>
> * How to model the CSI module clock isn't really clear to me. It looks
> like it goes through the CSI controller and then is muxed to one of the
> CSI pin so that it can clock the sensor. I'm not quite sure how to
> model it, if it should be a clock, the CSI driver being a clock
> provider, or if the sensor should just use the module clock directly.
Which clock are you talking about? MCLK? This seems to be fed directly from
the CCU, as there doesn't seem to be controls for it within the CSI hardware
block, and the diagram doesn't list it either. IMO you don't have to model it.
The camera sensor device node would just take a reference to it directly. You
would probably enable the (separate) pinmux setting in the CSI controller node.
ChenYu
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding
2018-11-13 8:38 ` Sakari Ailus
@ 2018-11-15 19:04 ` Maxime Ripard
2018-11-21 21:56 ` Sakari Ailus
0 siblings, 1 reply; 36+ messages in thread
From: Maxime Ripard @ 2018-11-15 19:04 UTC (permalink / raw)
To: Sakari Ailus
Cc: Hans Verkuil, Mauro Carvalho Chehab, Thomas Petazzoni,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]
Hi Sakari,
On Tue, Nov 13, 2018 at 10:38:55AM +0200, Sakari Ailus wrote:
> > + - allwinner,has-isp: Whether the CSI controller has an ISP
> > + associated to it or not
>
> Is the ISP a part of the same device? It sounds like that this is actually
> a different device if it contains an ISP as well, and that should be
> apparent from the compatible string. What do you think?
I guess we can see it as both. It seems to be the exact same register
set, except for the fact that the first instance has that ISP in
addition, and several channels, as you pointed out in your other mail.
What these channels are is not exactly clear. It looks like it's
related to the BT656 interface where you could interleave channel
bytes over the bus. I haven't really looked into it, and it doesn't
look like we have any code (or hardware) able to do that though.
> > +
> > +If allwinner,has-isp is set, an additional "isp" clock is needed,
> > +being a phandle to the clock driving the ISP.
> > +
> > +The CSI node should contain one 'port' child node with one child
> > +'endpoint' node, according to the bindings defined in
> > +Documentation/devicetree/bindings/media/video-interfaces.txt. The
> > +endpoint's bus type must be parallel or BT656.
> > +
> > +Endpoint node properties for CSI
> > +---------------------------------
> > +
> > +- remote-endpoint : (required) a phandle to the bus receiver's endpoint
> > + node
>
> Rob's opinion has been (AFAIU) that this is not needed as it's already a
> part of the graph bindings. Unless you want to say that it's required, that
> is --- the graph bindings document it as optional.
Ok, I'll remove it.
> > +- bus-width: : (required) must be 8
>
> If this is the only value the hardware supports, I don't see why you should
> specify it here.
Ditto :)
> > +- pclk-sample : (optional) (default: sample on falling edge)
> > +- hsync-active : (only required for parallel)
> > +- vsync-active : (only required for parallel)
> > +
> > +Example:
> > +
> > +csi0: csi@1c09000 {
> > + compatible = "allwinner,sun7i-a20-csi",
> > + "allwinner,sun4i-a10-csi";
> > + reg = <0x01c09000 0x1000>;
> > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
> > + <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
> > + clock-names = "ahb", "mod", "isp", "ram";
> > + resets = <&ccu RST_CSI0>;
> > + allwinner,csi-channels = <4>;
> > + allwinner,has-isp;
> > +
> > + port {
> > + csi_from_ov5640: endpoint {
> > + remote-endpoint = <&ov5640_to_csi>;
> > + bus-width = <8>;
> > + data-shift = <2>;
>
> data-shift needs to be documented above if it's relevant for the device.
It's not really related to the CSI device in that case but the sensor,
so I'll just leave it out.
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/5] media: Allwinner A10 CSI support
2018-11-14 3:24 ` Chen-Yu Tsai
@ 2018-11-15 19:10 ` Maxime Ripard
0 siblings, 0 replies; 36+ messages in thread
From: Maxime Ripard @ 2018-11-15 19:10 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, Laurent Pinchart, Linux Media Mailing List,
Andrzej Hajda, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]
Hi,
On Wed, Nov 14, 2018 at 11:24:48AM +0800, Chen-Yu Tsai wrote:
> On Tue, Nov 13, 2018 at 4:24 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > Here is a series introducing the support for the A10 (and SoCs of the same
> > generation) CMOS Sensor Interface (called CSI, not to be confused with
> > MIPI-CSI, which isn't support by that IP).
> >
> > That interface is pretty straightforward, but the driver has a few issues
> > that I wanted to bring up:
> >
> > * The only board I've been testing this with has an ov5640 sensor
> > attached, which doesn't work with the upstream driver. Copying the
> > Allwinner init sequence works though, and this is how it has been
> > tested. Testing with a second sensor would allow to see if it's an
> > issue on the CSI side or the sensor side.
> > * When starting a capture, the last buffer to capture will fail due to
> > double buffering being used, and we don't have a next buffer for the
> > last frame. I'm not sure how to deal with that though. It seems like
> > some drivers use a scratch buffer in such a case, some don't care, so
> > I'm not sure which solution should be preferred.
> > * We don't have support for the ISP at the moment, but this can be added
> > eventually.
> >
> > * How to model the CSI module clock isn't really clear to me. It looks
> > like it goes through the CSI controller and then is muxed to one of the
> > CSI pin so that it can clock the sensor. I'm not quite sure how to
> > model it, if it should be a clock, the CSI driver being a clock
> > provider, or if the sensor should just use the module clock directly.
>
> Which clock are you talking about? MCLK? This seems to be fed directly from
> the CCU, as there doesn't seem to be controls for it within the CSI hardware
> block, and the diagram doesn't list it either. IMO you don't have to model it.
> The camera sensor device node would just take a reference to it directly.
Yeah, that what I went for, I guess we agree :)
> You would probably enable the (separate) pinmux setting in the CSI
> controller node.
I'll change that.
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-13 12:24 ` Hans Verkuil
2018-11-13 15:19 ` Joe Perches
@ 2018-11-15 20:51 ` Maxime Ripard
2018-11-21 22:01 ` Sakari Ailus
1 sibling, 1 reply; 36+ messages in thread
From: Maxime Ripard @ 2018-11-15 20:51 UTC (permalink / raw)
To: Hans Verkuil
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
[-- Attachment #1: Type: text/plain, Size: 739 bytes --]
Hi Hans,
Thanks for your review! I'll address the other comments you made.
On Tue, Nov 13, 2018 at 01:24:47PM +0100, Hans Verkuil wrote:
> > +static int csi_probe(struct platform_device *pdev)
> > +{
> > + struct sun4i_csi *csi;
> > + struct resource *res;
> > + int ret;
> > + int irq;
> > +
> > + csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
>
> devm_kzalloc is not recommended: all devm_ memory is freed when the driver
> is unbound, but a filehandle might still have a reference open.
How would a !devm variant with a kfree in the remove help? We would
still fall in the same case, right?
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding
2018-11-15 19:04 ` Maxime Ripard
@ 2018-11-21 21:56 ` Sakari Ailus
2018-11-27 15:04 ` Maxime Ripard
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2018-11-21 21:56 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Mauro Carvalho Chehab, Thomas Petazzoni,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
Hi Maxime,
On Thu, Nov 15, 2018 at 08:04:24PM +0100, Maxime Ripard wrote:
> Hi Sakari,
>
> On Tue, Nov 13, 2018 at 10:38:55AM +0200, Sakari Ailus wrote:
> > > + - allwinner,has-isp: Whether the CSI controller has an ISP
> > > + associated to it or not
> >
> > Is the ISP a part of the same device? It sounds like that this is actually
> > a different device if it contains an ISP as well, and that should be
> > apparent from the compatible string. What do you think?
>
> I guess we can see it as both. It seems to be the exact same register
> set, except for the fact that the first instance has that ISP in
> addition, and several channels, as you pointed out in your other mail.
I'm simply referring to existing practices as far as I know them. If it's a
different device, it should have a different compatible string, not a
vendor-specific property to tell it's somehow different.
Many SoCs also separate the DMA and the CSI-2 receivers, and thus they have
separate drivers. I don't know about your case, but the ISP requiring a
different clock is a hint.
>
> What these channels are is not exactly clear. It looks like it's
> related to the BT656 interface where you could interleave channel
> bytes over the bus. I haven't really looked into it, and it doesn't
> look like we have any code (or hardware) able to do that though.
>
> > > +
> > > +If allwinner,has-isp is set, an additional "isp" clock is needed,
> > > +being a phandle to the clock driving the ISP.
> > > +
> > > +The CSI node should contain one 'port' child node with one child
> > > +'endpoint' node, according to the bindings defined in
> > > +Documentation/devicetree/bindings/media/video-interfaces.txt. The
> > > +endpoint's bus type must be parallel or BT656.
> > > +
> > > +Endpoint node properties for CSI
> > > +---------------------------------
> > > +
> > > +- remote-endpoint : (required) a phandle to the bus receiver's endpoint
> > > + node
> >
> > Rob's opinion has been (AFAIU) that this is not needed as it's already a
> > part of the graph bindings. Unless you want to say that it's required, that
> > is --- the graph bindings document it as optional.
>
> Ok, I'll remove it.
>
> > > +- bus-width: : (required) must be 8
> >
> > If this is the only value the hardware supports, I don't see why you should
> > specify it here.
>
> Ditto :)
>
> > > +- pclk-sample : (optional) (default: sample on falling edge)
> > > +- hsync-active : (only required for parallel)
> > > +- vsync-active : (only required for parallel)
> > > +
> > > +Example:
> > > +
> > > +csi0: csi@1c09000 {
> > > + compatible = "allwinner,sun7i-a20-csi",
> > > + "allwinner,sun4i-a10-csi";
> > > + reg = <0x01c09000 0x1000>;
> > > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> > > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
> > > + <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
> > > + clock-names = "ahb", "mod", "isp", "ram";
> > > + resets = <&ccu RST_CSI0>;
> > > + allwinner,csi-channels = <4>;
> > > + allwinner,has-isp;
> > > +
> > > + port {
> > > + csi_from_ov5640: endpoint {
> > > + remote-endpoint = <&ov5640_to_csi>;
> > > + bus-width = <8>;
> > > + data-shift = <2>;
> >
> > data-shift needs to be documented above if it's relevant for the device.
>
> It's not really related to the CSI device in that case but the sensor,
> so I'll just leave it out.
Hmm. data-shift should only be relevant for the receiver, shoudn't it?
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-15 20:51 ` Maxime Ripard
@ 2018-11-21 22:01 ` Sakari Ailus
2018-11-22 13:58 ` Maxime Ripard
0 siblings, 1 reply; 36+ messages in thread
From: Sakari Ailus @ 2018-11-21 22:01 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Hans Verkuil, Mauro Carvalho Chehab,
Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
On Thu, Nov 15, 2018 at 09:51:06PM +0100, Maxime Ripard wrote:
> Hi Hans,
>
> Thanks for your review! I'll address the other comments you made.
>
> On Tue, Nov 13, 2018 at 01:24:47PM +0100, Hans Verkuil wrote:
> > > +static int csi_probe(struct platform_device *pdev)
> > > +{
> > > + struct sun4i_csi *csi;
> > > + struct resource *res;
> > > + int ret;
> > > + int irq;
> > > +
> > > + csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
> >
> > devm_kzalloc is not recommended: all devm_ memory is freed when the driver
> > is unbound, but a filehandle might still have a reference open.
>
> How would a !devm variant with a kfree in the remove help? We would
> still fall in the same case, right?
Not quite. For video nodes this is handled: the release callback gets called
once there are no file handles open to the device. That may well be much
later than the device has been unbound from the driver.
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/5] media: sunxi: Add A10 CSI driver
2018-11-21 22:01 ` Sakari Ailus
@ 2018-11-22 13:58 ` Maxime Ripard
0 siblings, 0 replies; 36+ messages in thread
From: Maxime Ripard @ 2018-11-22 13:58 UTC (permalink / raw)
To: Sakari Ailus
Cc: Hans Verkuil, Hans Verkuil, Mauro Carvalho Chehab,
Thomas Petazzoni, Laurent Pinchart, linux-media, Andrzej Hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]
Hi Sakari,
On Thu, Nov 22, 2018 at 12:01:03AM +0200, Sakari Ailus wrote:
> On Thu, Nov 15, 2018 at 09:51:06PM +0100, Maxime Ripard wrote:
> > Hi Hans,
> >
> > Thanks for your review! I'll address the other comments you made.
> >
> > On Tue, Nov 13, 2018 at 01:24:47PM +0100, Hans Verkuil wrote:
> > > > +static int csi_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct sun4i_csi *csi;
> > > > + struct resource *res;
> > > > + int ret;
> > > > + int irq;
> > > > +
> > > > + csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
> > >
> > > devm_kzalloc is not recommended: all devm_ memory is freed when the driver
> > > is unbound, but a filehandle might still have a reference open.
> >
> > How would a !devm variant with a kfree in the remove help? We would
> > still fall in the same case, right?
>
> Not quite. For video nodes this is handled: the release callback gets called
> once there are no file handles open to the device. That may well be much
> later than the device has been unbound from the driver.
I might be missing something, but how the release callback will be
able to get the reference to the structure we allocated here so that
it can free it?
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
2018-11-13 8:24 ` [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support Maxime Ripard
@ 2018-11-27 6:56 ` Jagan Teki
2018-11-27 10:31 ` Maxime Ripard
0 siblings, 1 reply; 36+ messages in thread
From: Jagan Teki @ 2018-11-27 6:56 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, laurent.pinchart, linux-media, a.hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, frowand.list
On Tue, Nov 13, 2018 at 1:54 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
> arch/arm/boot/dts/sun7i-a20-bananapi.dts | 98 +++++++++++++++++++++++++-
> 1 file changed, 98 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> index 70dfc4ac0bb5..18dbff9f1ce9 100644
> --- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> @@ -54,6 +54,9 @@
> compatible = "lemaker,bananapi", "allwinner,sun7i-a20";
>
> aliases {
> + i2c0 = &i2c0;
> + i2c1 = &i2c1;
> + i2c2 = &i2c2;
> serial0 = &uart0;
> serial1 = &uart3;
> serial2 = &uart7;
> @@ -63,6 +66,41 @@
> stdout-path = "serial0:115200n8";
> };
>
> + reg_cam: cam {
> + compatible = "regulator-fixed";
> + regulator-name = "cam";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + vin-supply = <®_vcc5v0>;
> + gpio = <&pio 7 16 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> + regulator-always-on;
> + };
> +
> + reg_cam_avdd: cam-avdd {
> + compatible = "regulator-fixed";
> + regulator-name = "cam500b-avdd";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + vin-supply = <®_cam>;
> + };
> +
> + reg_cam_dovdd: cam-dovdd {
> + compatible = "regulator-fixed";
> + regulator-name = "cam500b-dovdd";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + vin-supply = <®_cam>;
> + };
> +
> + reg_cam_dvdd: cam-dvdd {
> + compatible = "regulator-fixed";
> + regulator-name = "cam500b-dvdd";
> + regulator-min-microvolt = <1500000>;
> + regulator-max-microvolt = <1500000>;
> + vin-supply = <®_cam>;
> + };
> +
> hdmi-connector {
> compatible = "hdmi-connector";
> type = "a";
> @@ -120,6 +158,27 @@
> >;
> };
>
> +&csi0 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&csi0_pins_a>;
> + status = "okay";
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + csi_from_ov5640: endpoint {
> + remote-endpoint = <&ov5640_to_csi>;
> + bus-width = <8>;
> + data-shift = <2>;
> + hsync-active = <1>; /* Active high */
> + vsync-active = <0>; /* Active low */
> + data-active = <1>; /* Active high */
> + pclk-sample = <1>; /* Rising */
> + };
> + };
> +};
> +
> &de {
> status = "okay";
> };
> @@ -167,6 +226,39 @@
> };
> };
>
> +&i2c1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c1_pins_a>;
> + status = "okay";
> +
> + camera: camera@21 {
> + compatible = "ovti,ov5640";
> + reg = <0x21>;
> + clocks = <&ccu CLK_CSI0>;
> + clock-names = "xclk";
> + assigned-clocks = <&ccu CLK_CSI0>;
> + assigned-clock-rates = <24000000>;
> +
> + reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>;
> + powerdown-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>;
> + AVDD-supply = <®_cam_avdd>;
> + DOVDD-supply = <®_cam_dovdd>;
> + DVDD-supply = <®_cam_dvdd>;
> +
> + port {
> + ov5640_to_csi: endpoint {
> + remote-endpoint = <&csi_from_ov5640>;
> + bus-width = <8>;
> + data-shift = <2>;
> + hsync-active = <1>; /* Active high */
> + vsync-active = <0>; /* Active low */
> + data-active = <1>; /* Active high */
> + pclk-sample = <1>; /* Rising */
> + };
> + };
> + };
Does ov5640 need any further patches, wrt linux-next? I'm trying to
test this on top of linux-next but the slave id seems not detecting.
[ 2.304711] ov5640 1-0021: Linked as a consumer to regulator.5
[ 2.310639] ov5640 1-0021: Linked as a consumer to regulator.6
[ 2.316592] ov5640 1-0021: Linked as a consumer to regulator.4
[ 2.351540] ov5640 1-0021: ov5640_init_slave_id: failed with -6
[ 2.357543] ov5640 1-0021: Dropping the link to regulator.5
[ 2.363224] ov5640 1-0021: Dropping the link to regulator.6
[ 2.368829] ov5640 1-0021: Dropping the link to regulator.4
Here is the full log [1], please let me know if I miss anything, I
even tried to remove MCLK pin
[1] https://paste.ubuntu.com/p/yfy5cvs32x/
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
2018-11-27 6:56 ` Jagan Teki
@ 2018-11-27 10:31 ` Maxime Ripard
2018-11-27 11:00 ` Jagan Teki
0 siblings, 1 reply; 36+ messages in thread
From: Maxime Ripard @ 2018-11-27 10:31 UTC (permalink / raw)
To: Jagan Teki
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, laurent.pinchart, linux-media, a.hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, frowand.list
[-- Attachment #1: Type: text/plain, Size: 5965 bytes --]
On Tue, Nov 27, 2018 at 12:26:09PM +0530, Jagan Teki wrote:
> On Tue, Nov 13, 2018 at 1:54 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> > arch/arm/boot/dts/sun7i-a20-bananapi.dts | 98 +++++++++++++++++++++++++-
> > 1 file changed, 98 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> > index 70dfc4ac0bb5..18dbff9f1ce9 100644
> > --- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> > +++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> > @@ -54,6 +54,9 @@
> > compatible = "lemaker,bananapi", "allwinner,sun7i-a20";
> >
> > aliases {
> > + i2c0 = &i2c0;
> > + i2c1 = &i2c1;
> > + i2c2 = &i2c2;
> > serial0 = &uart0;
> > serial1 = &uart3;
> > serial2 = &uart7;
> > @@ -63,6 +66,41 @@
> > stdout-path = "serial0:115200n8";
> > };
> >
> > + reg_cam: cam {
> > + compatible = "regulator-fixed";
> > + regulator-name = "cam";
> > + regulator-min-microvolt = <5000000>;
> > + regulator-max-microvolt = <5000000>;
> > + vin-supply = <®_vcc5v0>;
> > + gpio = <&pio 7 16 GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + regulator-always-on;
> > + };
> > +
> > + reg_cam_avdd: cam-avdd {
> > + compatible = "regulator-fixed";
> > + regulator-name = "cam500b-avdd";
> > + regulator-min-microvolt = <2800000>;
> > + regulator-max-microvolt = <2800000>;
> > + vin-supply = <®_cam>;
> > + };
> > +
> > + reg_cam_dovdd: cam-dovdd {
> > + compatible = "regulator-fixed";
> > + regulator-name = "cam500b-dovdd";
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <1800000>;
> > + vin-supply = <®_cam>;
> > + };
> > +
> > + reg_cam_dvdd: cam-dvdd {
> > + compatible = "regulator-fixed";
> > + regulator-name = "cam500b-dvdd";
> > + regulator-min-microvolt = <1500000>;
> > + regulator-max-microvolt = <1500000>;
> > + vin-supply = <®_cam>;
> > + };
> > +
> > hdmi-connector {
> > compatible = "hdmi-connector";
> > type = "a";
> > @@ -120,6 +158,27 @@
> > >;
> > };
> >
> > +&csi0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&csi0_pins_a>;
> > + status = "okay";
> > +
> > + port {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + csi_from_ov5640: endpoint {
> > + remote-endpoint = <&ov5640_to_csi>;
> > + bus-width = <8>;
> > + data-shift = <2>;
> > + hsync-active = <1>; /* Active high */
> > + vsync-active = <0>; /* Active low */
> > + data-active = <1>; /* Active high */
> > + pclk-sample = <1>; /* Rising */
> > + };
> > + };
> > +};
> > +
> > &de {
> > status = "okay";
> > };
> > @@ -167,6 +226,39 @@
> > };
> > };
> >
> > +&i2c1 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c1_pins_a>;
> > + status = "okay";
> > +
> > + camera: camera@21 {
> > + compatible = "ovti,ov5640";
> > + reg = <0x21>;
> > + clocks = <&ccu CLK_CSI0>;
> > + clock-names = "xclk";
> > + assigned-clocks = <&ccu CLK_CSI0>;
> > + assigned-clock-rates = <24000000>;
> > +
> > + reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>;
> > + powerdown-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>;
> > + AVDD-supply = <®_cam_avdd>;
> > + DOVDD-supply = <®_cam_dovdd>;
> > + DVDD-supply = <®_cam_dvdd>;
> > +
> > + port {
> > + ov5640_to_csi: endpoint {
> > + remote-endpoint = <&csi_from_ov5640>;
> > + bus-width = <8>;
> > + data-shift = <2>;
> > + hsync-active = <1>; /* Active high */
> > + vsync-active = <0>; /* Active low */
> > + data-active = <1>; /* Active high */
> > + pclk-sample = <1>; /* Rising */
> > + };
> > + };
> > + };
>
> Does ov5640 need any further patches, wrt linux-next? I'm trying to
> test this on top of linux-next but the slave id seems not detecting.
>
> [ 2.304711] ov5640 1-0021: Linked as a consumer to regulator.5
> [ 2.310639] ov5640 1-0021: Linked as a consumer to regulator.6
> [ 2.316592] ov5640 1-0021: Linked as a consumer to regulator.4
> [ 2.351540] ov5640 1-0021: ov5640_init_slave_id: failed with -6
> [ 2.357543] ov5640 1-0021: Dropping the link to regulator.5
> [ 2.363224] ov5640 1-0021: Dropping the link to regulator.6
> [ 2.368829] ov5640 1-0021: Dropping the link to regulator.4
>
> Here is the full log [1], please let me know if I miss anything, I
> even tried to remove MCLK pin
You seem to have made local modifications to your tree, what are they?
This indicates that the communication over i2c doesn't work, what is
your setup?
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
2018-11-27 10:31 ` Maxime Ripard
@ 2018-11-27 11:00 ` Jagan Teki
2018-11-27 15:19 ` Maxime Ripard
0 siblings, 1 reply; 36+ messages in thread
From: Jagan Teki @ 2018-11-27 11:00 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, laurent.pinchart, linux-media, a.hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, frowand.list
On Tue, Nov 27, 2018 at 4:01 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Tue, Nov 27, 2018 at 12:26:09PM +0530, Jagan Teki wrote:
> > On Tue, Nov 13, 2018 at 1:54 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > ---
> > > arch/arm/boot/dts/sun7i-a20-bananapi.dts | 98 +++++++++++++++++++++++++-
> > > 1 file changed, 98 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> > > index 70dfc4ac0bb5..18dbff9f1ce9 100644
> > > --- a/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> > > +++ b/arch/arm/boot/dts/sun7i-a20-bananapi.dts
> > > @@ -54,6 +54,9 @@
> > > compatible = "lemaker,bananapi", "allwinner,sun7i-a20";
> > >
> > > aliases {
> > > + i2c0 = &i2c0;
> > > + i2c1 = &i2c1;
> > > + i2c2 = &i2c2;
> > > serial0 = &uart0;
> > > serial1 = &uart3;
> > > serial2 = &uart7;
> > > @@ -63,6 +66,41 @@
> > > stdout-path = "serial0:115200n8";
> > > };
> > >
> > > + reg_cam: cam {
> > > + compatible = "regulator-fixed";
> > > + regulator-name = "cam";
> > > + regulator-min-microvolt = <5000000>;
> > > + regulator-max-microvolt = <5000000>;
> > > + vin-supply = <®_vcc5v0>;
> > > + gpio = <&pio 7 16 GPIO_ACTIVE_HIGH>;
> > > + enable-active-high;
> > > + regulator-always-on;
> > > + };
> > > +
> > > + reg_cam_avdd: cam-avdd {
> > > + compatible = "regulator-fixed";
> > > + regulator-name = "cam500b-avdd";
> > > + regulator-min-microvolt = <2800000>;
> > > + regulator-max-microvolt = <2800000>;
> > > + vin-supply = <®_cam>;
> > > + };
> > > +
> > > + reg_cam_dovdd: cam-dovdd {
> > > + compatible = "regulator-fixed";
> > > + regulator-name = "cam500b-dovdd";
> > > + regulator-min-microvolt = <1800000>;
> > > + regulator-max-microvolt = <1800000>;
> > > + vin-supply = <®_cam>;
> > > + };
> > > +
> > > + reg_cam_dvdd: cam-dvdd {
> > > + compatible = "regulator-fixed";
> > > + regulator-name = "cam500b-dvdd";
> > > + regulator-min-microvolt = <1500000>;
> > > + regulator-max-microvolt = <1500000>;
> > > + vin-supply = <®_cam>;
> > > + };
> > > +
> > > hdmi-connector {
> > > compatible = "hdmi-connector";
> > > type = "a";
> > > @@ -120,6 +158,27 @@
> > > >;
> > > };
> > >
> > > +&csi0 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&csi0_pins_a>;
> > > + status = "okay";
> > > +
> > > + port {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + csi_from_ov5640: endpoint {
> > > + remote-endpoint = <&ov5640_to_csi>;
> > > + bus-width = <8>;
> > > + data-shift = <2>;
> > > + hsync-active = <1>; /* Active high */
> > > + vsync-active = <0>; /* Active low */
> > > + data-active = <1>; /* Active high */
> > > + pclk-sample = <1>; /* Rising */
> > > + };
> > > + };
> > > +};
> > > +
> > > &de {
> > > status = "okay";
> > > };
> > > @@ -167,6 +226,39 @@
> > > };
> > > };
> > >
> > > +&i2c1 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&i2c1_pins_a>;
> > > + status = "okay";
> > > +
> > > + camera: camera@21 {
> > > + compatible = "ovti,ov5640";
> > > + reg = <0x21>;
> > > + clocks = <&ccu CLK_CSI0>;
> > > + clock-names = "xclk";
> > > + assigned-clocks = <&ccu CLK_CSI0>;
> > > + assigned-clock-rates = <24000000>;
> > > +
> > > + reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>;
> > > + powerdown-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>;
> > > + AVDD-supply = <®_cam_avdd>;
> > > + DOVDD-supply = <®_cam_dovdd>;
> > > + DVDD-supply = <®_cam_dvdd>;
> > > +
> > > + port {
> > > + ov5640_to_csi: endpoint {
> > > + remote-endpoint = <&csi_from_ov5640>;
> > > + bus-width = <8>;
> > > + data-shift = <2>;
> > > + hsync-active = <1>; /* Active high */
> > > + vsync-active = <0>; /* Active low */
> > > + data-active = <1>; /* Active high */
> > > + pclk-sample = <1>; /* Rising */
> > > + };
> > > + };
> > > + };
> >
> > Does ov5640 need any further patches, wrt linux-next? I'm trying to
> > test this on top of linux-next but the slave id seems not detecting.
> >
> > [ 2.304711] ov5640 1-0021: Linked as a consumer to regulator.5
> > [ 2.310639] ov5640 1-0021: Linked as a consumer to regulator.6
> > [ 2.316592] ov5640 1-0021: Linked as a consumer to regulator.4
> > [ 2.351540] ov5640 1-0021: ov5640_init_slave_id: failed with -6
> > [ 2.357543] ov5640 1-0021: Dropping the link to regulator.5
> > [ 2.363224] ov5640 1-0021: Dropping the link to regulator.6
> > [ 2.368829] ov5640 1-0021: Dropping the link to regulator.4
> >
> > Here is the full log [1], please let me know if I miss anything, I
> > even tried to remove MCLK pin
>
> You seem to have made local modifications to your tree, what are they?
> This indicates that the communication over i2c doesn't work, what is
> your setup?
I just used your commits on linux-next [2], with the setup similar in
Page 5 on datasheet[3]. The only difference is csi build issue, I have
updated similar fix you mentioned on sun6i_csi [4]
[2] https://github.com/amarula/linux-amarula/commits/CSI-A20
[3] https://www.tme.eu/gb/Document/187887186b98a8f78b47da2774a34f4c/BPI-CAMERA.pdf
[4] https://github.com/amarula/linux-amarula/commit/a6762ecd38f000e2bd02dd255f6fd0c1ae755429#diff-0809a7f97ca58771c1cda186e73ec657
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding
2018-11-21 21:56 ` Sakari Ailus
@ 2018-11-27 15:04 ` Maxime Ripard
0 siblings, 0 replies; 36+ messages in thread
From: Maxime Ripard @ 2018-11-27 15:04 UTC (permalink / raw)
To: Sakari Ailus
Cc: Hans Verkuil, Mauro Carvalho Chehab, Thomas Petazzoni,
Laurent Pinchart, linux-media, Andrzej Hajda, Chen-Yu Tsai,
linux-kernel, linux-arm-kernel, devicetree, Mark Rutland,
Rob Herring, Frank Rowand
[-- Attachment #1: Type: text/plain, Size: 4372 bytes --]
Hi,
On Wed, Nov 21, 2018 at 11:56:50PM +0200, Sakari Ailus wrote:
> On Thu, Nov 15, 2018 at 08:04:24PM +0100, Maxime Ripard wrote:
> > On Tue, Nov 13, 2018 at 10:38:55AM +0200, Sakari Ailus wrote:
> > > > + - allwinner,has-isp: Whether the CSI controller has an ISP
> > > > + associated to it or not
> > >
> > > Is the ISP a part of the same device? It sounds like that this is actually
> > > a different device if it contains an ISP as well, and that should be
> > > apparent from the compatible string. What do you think?
> >
> > I guess we can see it as both. It seems to be the exact same register
> > set, except for the fact that the first instance has that ISP in
> > addition, and several channels, as you pointed out in your other mail.
>
> I'm simply referring to existing practices as far as I know them. If it's a
> different device, it should have a different compatible string, not a
> vendor-specific property to tell it's somehow different.
The line is blurrier than that. Different devices are indeed
represented using different compatibles, but different features set
can be represented using properties.
> Many SoCs also separate the DMA and the CSI-2 receivers, and thus they have
> separate drivers. I don't know about your case, but the ISP requiring a
> different clock is a hint.
In this particular case, the datasheet represents the ISP as part of
the DMA, so it looks like a feature. And since we don't have any
source code for this, we can only do guesswork.
> > What these channels are is not exactly clear. It looks like it's
> > related to the BT656 interface where you could interleave channel
> > bytes over the bus. I haven't really looked into it, and it doesn't
> > look like we have any code (or hardware) able to do that though.
> >
> > > > +
> > > > +If allwinner,has-isp is set, an additional "isp" clock is needed,
> > > > +being a phandle to the clock driving the ISP.
> > > > +
> > > > +The CSI node should contain one 'port' child node with one child
> > > > +'endpoint' node, according to the bindings defined in
> > > > +Documentation/devicetree/bindings/media/video-interfaces.txt. The
> > > > +endpoint's bus type must be parallel or BT656.
> > > > +
> > > > +Endpoint node properties for CSI
> > > > +---------------------------------
> > > > +
> > > > +- remote-endpoint : (required) a phandle to the bus receiver's endpoint
> > > > + node
> > >
> > > Rob's opinion has been (AFAIU) that this is not needed as it's already a
> > > part of the graph bindings. Unless you want to say that it's required, that
> > > is --- the graph bindings document it as optional.
> >
> > Ok, I'll remove it.
> >
> > > > +- bus-width: : (required) must be 8
> > >
> > > If this is the only value the hardware supports, I don't see why you should
> > > specify it here.
> >
> > Ditto :)
> >
> > > > +- pclk-sample : (optional) (default: sample on falling edge)
> > > > +- hsync-active : (only required for parallel)
> > > > +- vsync-active : (only required for parallel)
> > > > +
> > > > +Example:
> > > > +
> > > > +csi0: csi@1c09000 {
> > > > + compatible = "allwinner,sun7i-a20-csi",
> > > > + "allwinner,sun4i-a10-csi";
> > > > + reg = <0x01c09000 0x1000>;
> > > > + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> > > > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
> > > > + <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
> > > > + clock-names = "ahb", "mod", "isp", "ram";
> > > > + resets = <&ccu RST_CSI0>;
> > > > + allwinner,csi-channels = <4>;
> > > > + allwinner,has-isp;
> > > > +
> > > > + port {
> > > > + csi_from_ov5640: endpoint {
> > > > + remote-endpoint = <&ov5640_to_csi>;
> > > > + bus-width = <8>;
> > > > + data-shift = <2>;
> > >
> > > data-shift needs to be documented above if it's relevant for the device.
> >
> > It's not really related to the CSI device in that case but the sensor,
> > so I'll just leave it out.
>
> Hmm. data-shift should only be relevant for the receiver, shoudn't it?
Sorry, I mispoke. We're not using it anywhere in either drivers, so
it's totally useless.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
2018-11-27 11:00 ` Jagan Teki
@ 2018-11-27 15:19 ` Maxime Ripard
2018-11-27 15:34 ` Jagan Teki
0 siblings, 1 reply; 36+ messages in thread
From: Maxime Ripard @ 2018-11-27 15:19 UTC (permalink / raw)
To: Jagan Teki
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, laurent.pinchart, linux-media, a.hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, frowand.list
[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]
On Tue, Nov 27, 2018 at 04:30:55PM +0530, Jagan Teki wrote:
> > > > +&i2c1 {
> > > > + pinctrl-names = "default";
> > > > + pinctrl-0 = <&i2c1_pins_a>;
> > > > + status = "okay";
> > > > +
> > > > + camera: camera@21 {
> > > > + compatible = "ovti,ov5640";
> > > > + reg = <0x21>;
> > > > + clocks = <&ccu CLK_CSI0>;
> > > > + clock-names = "xclk";
> > > > + assigned-clocks = <&ccu CLK_CSI0>;
> > > > + assigned-clock-rates = <24000000>;
> > > > +
> > > > + reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>;
> > > > + powerdown-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>;
> > > > + AVDD-supply = <®_cam_avdd>;
> > > > + DOVDD-supply = <®_cam_dovdd>;
> > > > + DVDD-supply = <®_cam_dvdd>;
> > > > +
> > > > + port {
> > > > + ov5640_to_csi: endpoint {
> > > > + remote-endpoint = <&csi_from_ov5640>;
> > > > + bus-width = <8>;
> > > > + data-shift = <2>;
> > > > + hsync-active = <1>; /* Active high */
> > > > + vsync-active = <0>; /* Active low */
> > > > + data-active = <1>; /* Active high */
> > > > + pclk-sample = <1>; /* Rising */
> > > > + };
> > > > + };
> > > > + };
> > >
> > > Does ov5640 need any further patches, wrt linux-next? I'm trying to
> > > test this on top of linux-next but the slave id seems not detecting.
> > >
> > > [ 2.304711] ov5640 1-0021: Linked as a consumer to regulator.5
> > > [ 2.310639] ov5640 1-0021: Linked as a consumer to regulator.6
> > > [ 2.316592] ov5640 1-0021: Linked as a consumer to regulator.4
> > > [ 2.351540] ov5640 1-0021: ov5640_init_slave_id: failed with -6
> > > [ 2.357543] ov5640 1-0021: Dropping the link to regulator.5
> > > [ 2.363224] ov5640 1-0021: Dropping the link to regulator.6
> > > [ 2.368829] ov5640 1-0021: Dropping the link to regulator.4
> > >
> > > Here is the full log [1], please let me know if I miss anything, I
> > > even tried to remove MCLK pin
> >
> > You seem to have made local modifications to your tree, what are they?
> > This indicates that the communication over i2c doesn't work, what is
> > your setup?
>
> I just used your commits on linux-next [2], with the setup similar in
> Page 5 on datasheet[3]. The only difference is csi build issue, I have
> updated similar fix you mentioned on sun6i_csi [4]
>
> [2] https://github.com/amarula/linux-amarula/commits/CSI-A20
> [3] https://www.tme.eu/gb/Document/187887186b98a8f78b47da2774a34f4c/BPI-CAMERA.pdf
> [4] https://github.com/amarula/linux-amarula/commit/a6762ecd38f000e2bd02dd255f6fd0c1ae755429#diff-0809a7f97ca58771c1cda186e73ec657
That branch doesn't have any commit with the same ID that you have in
your boot log.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
2018-11-27 15:19 ` Maxime Ripard
@ 2018-11-27 15:34 ` Jagan Teki
0 siblings, 0 replies; 36+ messages in thread
From: Jagan Teki @ 2018-11-27 15:34 UTC (permalink / raw)
To: Maxime Ripard
Cc: Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab,
Thomas Petazzoni, laurent.pinchart, linux-media, a.hajda,
Chen-Yu Tsai, linux-kernel, linux-arm-kernel, devicetree,
Mark Rutland, Rob Herring, Frank Rowand
On Tue, Nov 27, 2018 at 8:49 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Tue, Nov 27, 2018 at 04:30:55PM +0530, Jagan Teki wrote:
> > > > > +&i2c1 {
> > > > > + pinctrl-names = "default";
> > > > > + pinctrl-0 = <&i2c1_pins_a>;
> > > > > + status = "okay";
> > > > > +
> > > > > + camera: camera@21 {
> > > > > + compatible = "ovti,ov5640";
> > > > > + reg = <0x21>;
> > > > > + clocks = <&ccu CLK_CSI0>;
> > > > > + clock-names = "xclk";
> > > > > + assigned-clocks = <&ccu CLK_CSI0>;
> > > > > + assigned-clock-rates = <24000000>;
> > > > > +
> > > > > + reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>;
> > > > > + powerdown-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>;
> > > > > + AVDD-supply = <®_cam_avdd>;
> > > > > + DOVDD-supply = <®_cam_dovdd>;
> > > > > + DVDD-supply = <®_cam_dvdd>;
> > > > > +
> > > > > + port {
> > > > > + ov5640_to_csi: endpoint {
> > > > > + remote-endpoint = <&csi_from_ov5640>;
> > > > > + bus-width = <8>;
> > > > > + data-shift = <2>;
> > > > > + hsync-active = <1>; /* Active high */
> > > > > + vsync-active = <0>; /* Active low */
> > > > > + data-active = <1>; /* Active high */
> > > > > + pclk-sample = <1>; /* Rising */
> > > > > + };
> > > > > + };
> > > > > + };
> > > >
> > > > Does ov5640 need any further patches, wrt linux-next? I'm trying to
> > > > test this on top of linux-next but the slave id seems not detecting.
> > > >
> > > > [ 2.304711] ov5640 1-0021: Linked as a consumer to regulator.5
> > > > [ 2.310639] ov5640 1-0021: Linked as a consumer to regulator.6
> > > > [ 2.316592] ov5640 1-0021: Linked as a consumer to regulator.4
> > > > [ 2.351540] ov5640 1-0021: ov5640_init_slave_id: failed with -6
> > > > [ 2.357543] ov5640 1-0021: Dropping the link to regulator.5
> > > > [ 2.363224] ov5640 1-0021: Dropping the link to regulator.6
> > > > [ 2.368829] ov5640 1-0021: Dropping the link to regulator.4
> > > >
> > > > Here is the full log [1], please let me know if I miss anything, I
> > > > even tried to remove MCLK pin
> > >
> > > You seem to have made local modifications to your tree, what are they?
> > > This indicates that the communication over i2c doesn't work, what is
> > > your setup?
> >
> > I just used your commits on linux-next [2], with the setup similar in
> > Page 5 on datasheet[3]. The only difference is csi build issue, I have
> > updated similar fix you mentioned on sun6i_csi [4]
> >
> > [2] https://github.com/amarula/linux-amarula/commits/CSI-A20
> > [3] https://www.tme.eu/gb/Document/187887186b98a8f78b47da2774a34f4c/BPI-CAMERA.pdf
> > [4] https://github.com/amarula/linux-amarula/commit/a6762ecd38f000e2bd02dd255f6fd0c1ae755429#diff-0809a7f97ca58771c1cda186e73ec657
>
> That branch doesn't have any commit with the same ID that you have in
> your boot log.
I have created this branch for your reference, here is the clean log on this [5]
[5] https://paste.ubuntu.com/p/4bkFs5WG6c/
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2018-11-27 15:34 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-13 8:24 [PATCH 0/5] media: Allwinner A10 CSI support Maxime Ripard
2018-11-13 8:24 ` [PATCH 1/5] dt-bindings: media: Add Allwinner A10 CSI binding Maxime Ripard
2018-11-13 8:38 ` Sakari Ailus
2018-11-15 19:04 ` Maxime Ripard
2018-11-21 21:56 ` Sakari Ailus
2018-11-27 15:04 ` Maxime Ripard
2018-11-13 10:34 ` Sakari Ailus
2018-11-13 8:24 ` [PATCH 2/5] media: sunxi: Refactor the Makefile and Kconfig Maxime Ripard
2018-11-13 8:24 ` [PATCH 3/5] media: sunxi: Add A10 CSI driver Maxime Ripard
2018-11-13 8:57 ` Sakari Ailus
2018-11-13 12:24 ` Hans Verkuil
2018-11-13 15:19 ` Joe Perches
2018-11-13 15:39 ` Hans Verkuil
2018-11-15 20:51 ` Maxime Ripard
2018-11-21 22:01 ` Sakari Ailus
2018-11-22 13:58 ` Maxime Ripard
2018-11-13 12:48 ` Fabio Estevam
2018-11-13 13:37 ` Hans Verkuil
2018-11-13 14:13 ` Fabio Estevam
2018-11-13 14:46 ` Thomas Petazzoni
2018-11-13 8:24 ` [PATCH 4/5] ARM: dts: sun7i: Add CSI0 controller Maxime Ripard
2018-11-13 8:24 ` [PATCH 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support Maxime Ripard
2018-11-27 6:56 ` Jagan Teki
2018-11-27 10:31 ` Maxime Ripard
2018-11-27 11:00 ` Jagan Teki
2018-11-27 15:19 ` Maxime Ripard
2018-11-27 15:34 ` Jagan Teki
2018-11-13 12:30 ` [PATCH 0/5] media: Allwinner A10 CSI support Hans Verkuil
2018-11-13 13:52 ` Maxime Ripard
2018-11-13 14:01 ` Hans Verkuil
2018-11-13 15:52 ` Maxime Ripard
2018-11-13 16:00 ` Hans Verkuil
2018-11-13 16:55 ` Thomas Petazzoni
2018-11-13 17:15 ` Hans Verkuil
2018-11-14 3:24 ` Chen-Yu Tsai
2018-11-15 19:10 ` Maxime Ripard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).