* [PATCH v10 0/3] media: rockchip: Add a driver for Rockchip's camera interface
@ 2023-11-08 16:38 Mehdi Djait
2023-11-08 16:38 ` [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF Mehdi Djait
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Mehdi Djait @ 2023-11-08 16:38 UTC (permalink / raw)
To: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt
Cc: linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski,
michael.riesch, Mehdi Djait
Hello everyone,
V10 for basic support of the Camera Interface found on the Rockchip PX30 SoC
Most of this driver was written following the BSP driver from rockchip,
removing the parts that either didn't fit correctly the guidelines, or
that couldn't be tested.
In the BSP, this driver is known as the "cif" driver, but this
controller was renamed to "vip" in the datasheet.
This version of the driver supports ONLY the parallel interface BT656
and was tested/implemented using an SDTV video decoder.
media_tree, base-commit: 94e27fbeca27d8c772fc2bc807730aaee5886055
V9 => V10:
cif/capture.c cif/dev.c cif/dev.h:
as suggested by Paul:
- ensured that the lock is still being held when accessing
stream->buffs[0,1]
- adjusted the comment explaining why the spinlock is used
as suggested by Michael:
- made the IRQ requested SHARED: the cif shares the IRQ with the io_mmu
rockchip,rk3066-cif.yaml:
- dropped the rk3066-cif compatible but kept the name and added the
reason for this in the commit msg: the name of the file rk3066 is the first
Rockchip SoC generation that uses cif instead of the px30 which is just one
of the many iterations of the unit.
V8 => V9:
cif/capture.c cif/dev.c cif/dev.h:
as suggested by Paul:
- changed the name from "vip" back to "cif"
- removed the scratch buffer and added frame dropping
- removed mplane, only single plane formats are supported anyway
- adjusted the Kconfig
- added the match_data to the stream struct
- some cosmetics, and error return codes changes
as suggested by Michael:
- changed the writel and readl helpers to be inline functions and
changed the name
- fixed typos in the commit message
- changed the cif_device struct element "sensor" to "remote"
rockchip,rk3066-cif.yaml:
- changed the compatible rockchip,px30-vip to rockchip,rk3066-cif:
rk3066 is the earliest Rockchip SoC that uses cif and it is the
first model starting the RK30 lineup.
- changed the node name to video-capture
- adjusted the description
V7 => V8:
vip/capture.c:
- fixed a warning: unused variable reported by the kernel test robot
V6 => V7:
vip/capture.c vip/dev.c vip/dev.h
- renamed all struct rk_vip_dev dev => struct rk_vip_dev vip_dev
- added some error when rk_vip_get_buffer() returns NULL
- removed a WARN_ON
- made the irq NOT shared
- dropped of_match_ptr
- added the rk_vip_get_resource() function
rockchip,px30-vip.yaml:
- changed filename to match the compatible
- dropped the mention of the other rockchip SoC in the dt-binding
description and added a more detailed description of VIP
- removed unused labels in the example
V5[1] => V6:
vip/capture.c vip/dev.c vip/dev.h
- added a video g_input_status subdev call, V4L2_IN_CAP_STD and the
supported stds in rk_vip_enum_input callback
- added rk_vip_g_std, rk_vip_s_std and rk_vip_querystd callbacks
- added the supported video_device->tvnorms
- s_std will now update the format as this depends on the standard
NTSC/PAL (as suggested by Hans in [1])
- removed STD_ATSC
- moved the colorimetry information to come from the subdev
- removed the core s_power subdev calls
- dropped cropping in rk_vip_stream struct
rockchip-vip.yaml:
- fixed a mistake in the name of third clock plckin -> plck
- changed the reg maxItems 2 -> 1
[1] https://lore.kernel.org/linux-media/20201229161724.511102-1-maxime.chevallier@bootlin.com/
I used v4l-utils with HEAD: commit 3d6682746de535d1f7aa71b43a30af40d52a539c
# v4l2-compliance
v4l2-compliance 1.25.0, 64 bits, 64-bit time_t
Compliance test for rockchip-cif device /dev/video0:
Driver Info:
Driver name : rockchip-cif
Card type : rockchip-cif
Bus info : platform:ff490000.video-capture
Driver version : 6.6.0
Capabilities : 0x84200001
Video Capture
Streaming
Extended Pix Format
Device Capabilities
Device Caps : 0x04200001
Video Capture
Streaming
Extended Pix Format
Media Driver Info:
Driver name : rockchip-cif
Model : cif
Serial :
Bus info : platform:ff490000.video-capture
Media version : 6.6.0
Hardware revision: 0x00000000 (0)
Driver version : 6.6.0
Interface Info:
ID : 0x03000003
Type : V4L Video
Entity Info:
ID : 0x00000001 (1)
Name : rockchip_cif
Function : V4L2 I/O
Pad 0x01000002 : 0: Sink
Link 0x02000009: from remote pad 0x1000006 of entity 'tw9900 2-0044' (Digital Video Decoder): Data, Enabled
Required ioctls:
test MC information (see 'Media Driver Info' above): OK
test VIDIOC_QUERYCAP: OK
test invalid ioctls: OK
Allow for multiple opens:
test second /dev/video0 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
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)
Control ioctls (Input 0):
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 (Input 0):
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 (Not Supported)
Codec ioctls (Input 0):
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 (Input 0):
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK
test Requests: OK (Not Supported)
Total for rockchip-cif device /dev/video0: 46, Succeeded: 46, Failed: 0, Warnings: 0
Mehdi Djait (3):
media: dt-bindings: media: add bindings for Rockchip CIF
media: rockchip: Add a driver for Rockchip's camera interface
arm64: dts: rockchip: Add the camera interface
.../bindings/media/rockchip,rk3066-cif.yaml | 94 ++
MAINTAINERS | 7 +
arch/arm64/boot/dts/rockchip/px30.dtsi | 12 +
drivers/media/platform/rockchip/Kconfig | 1 +
drivers/media/platform/rockchip/Makefile | 1 +
drivers/media/platform/rockchip/cif/Kconfig | 13 +
drivers/media/platform/rockchip/cif/Makefile | 3 +
drivers/media/platform/rockchip/cif/capture.c | 1152 +++++++++++++++++
drivers/media/platform/rockchip/cif/dev.c | 289 +++++
drivers/media/platform/rockchip/cif/dev.h | 139 ++
drivers/media/platform/rockchip/cif/regs.h | 192 +++
11 files changed, 1903 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
create mode 100644 drivers/media/platform/rockchip/cif/Kconfig
create mode 100644 drivers/media/platform/rockchip/cif/Makefile
create mode 100644 drivers/media/platform/rockchip/cif/capture.c
create mode 100644 drivers/media/platform/rockchip/cif/dev.c
create mode 100644 drivers/media/platform/rockchip/cif/dev.h
create mode 100644 drivers/media/platform/rockchip/cif/regs.h
--
2.41.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-08 16:38 [PATCH v10 0/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
@ 2023-11-08 16:38 ` Mehdi Djait
2023-11-09 17:24 ` Conor Dooley
2023-11-08 16:38 ` [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
2023-11-08 16:38 ` [PATCH v10 3/3] arm64: dts: rockchip: Add the " Mehdi Djait
2 siblings, 1 reply; 20+ messages in thread
From: Mehdi Djait @ 2023-11-08 16:38 UTC (permalink / raw)
To: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt
Cc: linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski,
michael.riesch, Mehdi Djait
Add a documentation for the Rockchip Camera Interface binding.
the name of the file rk3066 is the first Rockchip SoC generation that uses cif
instead of the px30 which is just one of the many iterations of the unit.
Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
---
.../bindings/media/rockchip,rk3066-cif.yaml | 94 +++++++++++++++++++
1 file changed, 94 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
new file mode 100644
index 000000000000..c3a5cd2baf71
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/rockchip,rk3066-cif.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip CIF Camera Interface
+
+maintainers:
+ - Mehdi Djait <mehdi.djait@bootlin.com>
+
+description:
+ CIF is a camera interface present on some rockchip SoCs. It receives the data
+ from Camera sensor or CCIR656 encoder and transfers it into system main memory
+ by AXI bus.
+
+properties:
+ compatible:
+ const: rockchip,px30-vip
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: ACLK
+ - description: HCLK
+ - description: PCLK
+
+ clock-names:
+ items:
+ - const: aclk
+ - const: hclk
+ - const: pclk
+
+ resets:
+ items:
+ - description: AXI
+ - description: AHB
+ - description: PCLK IN
+
+ reset-names:
+ items:
+ - const: axi
+ - const: ahb
+ - const: pclkin
+
+ power-domains:
+ maxItems: 1
+
+ port:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: A connection to a sensor or decoder
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/px30-cru.h>
+ #include <dt-bindings/power/px30-power.h>
+
+ parent {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ video-capture@ff490000 {
+ compatible = "rockchip,px30-vip";
+ reg = <0x0 0xff490000 0x0 0x200>;
+ interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cru ACLK_CIF>, <&cru HCLK_CIF>, <&cru PCLK_CIF>;
+ clock-names = "aclk", "hclk", "pclk";
+ resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
+ reset-names = "axi", "ahb", "pclkin";
+ power-domains = <&power PX30_PD_VI>;
+
+ port {
+ endpoint {
+ remote-endpoint = <&tw9900_out>;
+ };
+ };
+ };
+ };
+...
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface
2023-11-08 16:38 [PATCH v10 0/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
2023-11-08 16:38 ` [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF Mehdi Djait
@ 2023-11-08 16:38 ` Mehdi Djait
2023-11-10 12:50 ` Michael Riesch
2023-11-10 14:33 ` Michael Riesch
2023-11-08 16:38 ` [PATCH v10 3/3] arm64: dts: rockchip: Add the " Mehdi Djait
2 siblings, 2 replies; 20+ messages in thread
From: Mehdi Djait @ 2023-11-08 16:38 UTC (permalink / raw)
To: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt
Cc: linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski,
michael.riesch, Mehdi Djait
This introduces a V4L2 driver for the Rockchip CIF video capture controller.
This controller supports multiple parallel interfaces, but for now only the
BT.656 interface could be tested, hence it's the only one that's supported
in the first version of this driver.
This controller can be found on RK3066, PX30, RK1808, RK3128 and RK3288,
but for now it's only been tested on the PX30.
CIF is implemented as a video node-centric driver.
Most of this driver was written following the BSP driver from rockchip,
removing the parts that either didn't fit correctly the guidelines, or that
couldn't be tested.
This basic version doesn't support cropping nor scaling and is only
designed with one SDTV video decoder being attached to it at any time.
This version uses the "pingpong" mode of the controller, which is a
double-buffering mechanism.
Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
---
MAINTAINERS | 7 +
drivers/media/platform/rockchip/Kconfig | 1 +
drivers/media/platform/rockchip/Makefile | 1 +
drivers/media/platform/rockchip/cif/Kconfig | 13 +
drivers/media/platform/rockchip/cif/Makefile | 3 +
drivers/media/platform/rockchip/cif/capture.c | 1152 +++++++++++++++++
drivers/media/platform/rockchip/cif/dev.c | 289 +++++
drivers/media/platform/rockchip/cif/dev.h | 139 ++
drivers/media/platform/rockchip/cif/regs.h | 192 +++
9 files changed, 1797 insertions(+)
create mode 100644 drivers/media/platform/rockchip/cif/Kconfig
create mode 100644 drivers/media/platform/rockchip/cif/Makefile
create mode 100644 drivers/media/platform/rockchip/cif/capture.c
create mode 100644 drivers/media/platform/rockchip/cif/dev.c
create mode 100644 drivers/media/platform/rockchip/cif/dev.h
create mode 100644 drivers/media/platform/rockchip/cif/regs.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 3b47e0b56859..f47a828d08a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18523,6 +18523,13 @@ F: Documentation/ABI/*/sysfs-driver-hid-roccat*
F: drivers/hid/hid-roccat*
F: include/linux/hid-roccat*
+ROCKCHIP CIF DRIVER
+M: Mehdi Djait <mehdi.djait@bootlin.com>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
+F: drivers/media/platform/rockchip/cif/
+
ROCKCHIP CRYPTO DRIVERS
M: Corentin Labbe <clabbe@baylibre.com>
L: linux-crypto@vger.kernel.org
diff --git a/drivers/media/platform/rockchip/Kconfig b/drivers/media/platform/rockchip/Kconfig
index b41d3960c1b4..f73d68d1d2b6 100644
--- a/drivers/media/platform/rockchip/Kconfig
+++ b/drivers/media/platform/rockchip/Kconfig
@@ -2,5 +2,6 @@
comment "Rockchip media platform drivers"
+source "drivers/media/platform/rockchip/cif/Kconfig"
source "drivers/media/platform/rockchip/rga/Kconfig"
source "drivers/media/platform/rockchip/rkisp1/Kconfig"
diff --git a/drivers/media/platform/rockchip/Makefile b/drivers/media/platform/rockchip/Makefile
index 4f782b876ac9..1a7aa1f8e134 100644
--- a/drivers/media/platform/rockchip/Makefile
+++ b/drivers/media/platform/rockchip/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
+obj-y += cif/
obj-y += rga/
obj-y += rkisp1/
diff --git a/drivers/media/platform/rockchip/cif/Kconfig b/drivers/media/platform/rockchip/cif/Kconfig
new file mode 100644
index 000000000000..68fc50c51f1c
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/Kconfig
@@ -0,0 +1,13 @@
+config VIDEO_ROCKCHIP_CIF
+ tristate "Rockchip CIF Video Camera Interface"
+ depends on VIDEO_DEV
+ depends on ARCH_ROCKCHIP || COMPILE_TEST
+ depends on V4L_PLATFORM_DRIVERS
+ depends on PM && COMMON_CLK
+ select VIDEOBUF2_DMA_CONTIG
+ select V4L2_FWNODE
+ select VIDEO_V4L2_SUBDEV_API
+ help
+ This is a driver for Rockchip SoC Camera interface. It supports
+ parallel interfaces such as BT.656. This camera interface is both
+ called VIP and CIF.
diff --git a/drivers/media/platform/rockchip/cif/Makefile b/drivers/media/platform/rockchip/cif/Makefile
new file mode 100644
index 000000000000..e44ef687aeb6
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += rockchip-cif.o
+rockchip-cif-objs += dev.o capture.o
diff --git a/drivers/media/platform/rockchip/cif/capture.c b/drivers/media/platform/rockchip/cif/capture.c
new file mode 100644
index 000000000000..f5f30884ebcc
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/capture.c
@@ -0,0 +1,1152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Rockchip CIF Camera Interface Driver
+ *
+ * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
+ * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@bootlin.com>
+ * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-mc.h>
+#include <media/v4l2-subdev.h>
+#include <media/videobuf2-dma-contig.h>
+
+#include "dev.h"
+#include "regs.h"
+
+#define CIF_REQ_BUFS_MIN 2
+#define CIF_MIN_WIDTH 64
+#define CIF_MIN_HEIGHT 64
+#define CIF_MAX_WIDTH 8192
+#define CIF_MAX_HEIGHT 8192
+
+#define CIF_PLANE_Y 0
+#define CIF_PLANE_UV 1
+
+static struct cif_output_fmt out_fmts[] = {
+ {
+ .fourcc = V4L2_PIX_FMT_NV16,
+ .fmt_val = CIF_FORMAT_YUV_OUTPUT_422 |
+ CIF_FORMAT_UV_STORAGE_ORDER_UVUV,
+ .cplanes = 2,
+ }, {
+ .fourcc = V4L2_PIX_FMT_NV61,
+ .fmt_val = CIF_FORMAT_YUV_OUTPUT_422 |
+ CIF_FORMAT_UV_STORAGE_ORDER_VUVU,
+ .cplanes = 2,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_NV12,
+ .fmt_val = CIF_FORMAT_YUV_OUTPUT_420 |
+ CIF_FORMAT_UV_STORAGE_ORDER_UVUV,
+ .cplanes = 2,
+ },
+ {
+ .fourcc = V4L2_PIX_FMT_NV21,
+ .fmt_val = CIF_FORMAT_YUV_OUTPUT_420 |
+ CIF_FORMAT_UV_STORAGE_ORDER_VUVU,
+ .cplanes = 2,
+ }, {
+ .fourcc = V4L2_PIX_FMT_RGB24,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_RGB565,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_BGR666,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SRGGB8,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SGRBG8,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SGBRG8,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SBGGR8,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SRGGB10,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SGRBG10,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SGBRG10,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SBGGR10,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SRGGB12,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SGRBG12,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SGBRG12,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SBGGR12,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_SBGGR16,
+ .cplanes = 1,
+ }, {
+ .fourcc = V4L2_PIX_FMT_Y16,
+ .cplanes = 1,
+ }
+};
+
+static const struct cif_input_fmt in_fmts[] = {
+ {
+ .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
+ .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
+ CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
+ .fmt_type = CIF_FMT_TYPE_YUV,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
+ .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
+ CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
+ .fmt_type = CIF_FMT_TYPE_YUV,
+ .field = V4L2_FIELD_INTERLACED,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
+ .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
+ CIF_FORMAT_YUV_INPUT_ORDER_YVYU,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
+ .fmt_type = CIF_FMT_TYPE_YUV,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
+ .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
+ CIF_FORMAT_YUV_INPUT_ORDER_YVYU,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
+ .fmt_type = CIF_FMT_TYPE_YUV,
+ .field = V4L2_FIELD_INTERLACED,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
+ .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
+ CIF_FORMAT_YUV_INPUT_ORDER_UYVY,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
+ .fmt_type = CIF_FMT_TYPE_YUV,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
+ .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
+ CIF_FORMAT_YUV_INPUT_ORDER_UYVY,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
+ .fmt_type = CIF_FMT_TYPE_YUV,
+ .field = V4L2_FIELD_INTERLACED,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
+ .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
+ CIF_FORMAT_YUV_INPUT_ORDER_VYUY,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
+ .fmt_type = CIF_FMT_TYPE_YUV,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
+ .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
+ CIF_FORMAT_YUV_INPUT_ORDER_VYUY,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
+ .fmt_type = CIF_FMT_TYPE_YUV,
+ .field = V4L2_FIELD_INTERLACED,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_8,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_8,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_8,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_8,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_10,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_10,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_10,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_10,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_12,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_12,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_12,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_12,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_RGB888_1X24,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RGB888,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_Y8_1X8,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_8,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_Y10_1X10,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_10,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_Y12_1X12,
+ .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
+ CIF_FORMAT_RAW_DATA_WIDTH_12,
+ .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }
+};
+
+static const struct
+cif_input_fmt *get_input_fmt(struct v4l2_subdev *sd)
+{
+ struct v4l2_subdev_format fmt;
+ u32 i;
+
+ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ fmt.pad = 0;
+ v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
+
+ for (i = 0; i < ARRAY_SIZE(in_fmts); i++)
+ if (fmt.format.code == in_fmts[i].mbus_code &&
+ fmt.format.field == in_fmts[i].field)
+ return &in_fmts[i];
+
+ v4l2_err(sd->v4l2_dev, "remote's mbus code not supported\n");
+ return NULL;
+}
+
+static struct
+cif_output_fmt *find_output_fmt(struct cif_stream *stream, u32 pixelfmt)
+{
+ struct cif_output_fmt *fmt;
+ u32 i;
+
+ for (i = 0; i < ARRAY_SIZE(out_fmts); i++) {
+ fmt = &out_fmts[i];
+ if (fmt->fourcc == pixelfmt)
+ return fmt;
+ }
+
+ return NULL;
+}
+
+static struct cif_buffer *cif_get_buffer(struct cif_stream *stream)
+{
+ struct cif_buffer *buff;
+
+ lockdep_assert_held(&stream->vbq_lock);
+
+ if (list_empty(&stream->buf_head))
+ return NULL;
+
+ buff = list_first_entry(&stream->buf_head, struct cif_buffer, queue);
+ list_del(&buff->queue);
+
+ return buff;
+}
+
+static int cif_init_buffers(struct cif_stream *stream)
+{
+ struct cif_device *cif_dev = stream->cifdev;
+ unsigned long lock_flags;
+
+ spin_lock_irqsave(&stream->vbq_lock, lock_flags);
+
+ stream->buffs[0] = cif_get_buffer(stream);
+ stream->buffs[1] = cif_get_buffer(stream);
+
+ if (!(stream->buffs[0]) || !(stream->buffs[1])) {
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+ return -EINVAL;
+ }
+
+ stream->drop_frame = false;
+
+ cif_write(cif_dev, CIF_FRM0_ADDR_Y,
+ stream->buffs[0]->buff_addr[CIF_PLANE_Y]);
+ cif_write(cif_dev, CIF_FRM0_ADDR_UV,
+ stream->buffs[0]->buff_addr[CIF_PLANE_UV]);
+
+ cif_write(cif_dev, CIF_FRM1_ADDR_Y,
+ stream->buffs[1]->buff_addr[CIF_PLANE_Y]);
+ cif_write(cif_dev, CIF_FRM1_ADDR_UV,
+ stream->buffs[1]->buff_addr[CIF_PLANE_UV]);
+
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+
+ return 0;
+}
+
+static void cif_assign_new_buffer_pingpong(struct cif_stream *stream)
+{
+ struct cif_device *cif_dev = stream->cifdev;
+ struct cif_buffer *buffer = NULL;
+ u32 frm_addr_y, frm_addr_uv;
+ unsigned long lock_flags;
+
+ spin_lock_irqsave(&stream->vbq_lock, lock_flags);
+
+ buffer = cif_get_buffer(stream);
+
+ /*
+ * In Pingpong mode:
+ * After one frame0 captured, CIF will start to capture the next frame1
+ * automatically.
+ *
+ * If there is no buffer:
+ * 1. Make the next frame0 write to the buffer of frame1.
+ *
+ * 2. Drop the frame1: Don't return it to user-space, as it will be
+ * overwritten by the next frame0.
+ */
+ if (!buffer) {
+ stream->drop_frame = true;
+ buffer = stream->buffs[1 - stream->frame_phase];
+ } else {
+ stream->drop_frame = false;
+ }
+
+ stream->buffs[stream->frame_phase] = buffer;
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+
+ frm_addr_y = stream->frame_phase ? CIF_FRM1_ADDR_Y : CIF_FRM0_ADDR_Y;
+ frm_addr_uv = stream->frame_phase ? CIF_FRM1_ADDR_UV : CIF_FRM0_ADDR_UV;
+
+ cif_write(cif_dev, frm_addr_y, buffer->buff_addr[CIF_PLANE_Y]);
+ cif_write(cif_dev, frm_addr_uv, buffer->buff_addr[CIF_PLANE_UV]);
+}
+
+static void cif_stream_stop(struct cif_stream *stream)
+{
+ struct cif_device *cif_dev = stream->cifdev;
+ u32 val;
+
+ val = cif_read(cif_dev, CIF_CTRL);
+ cif_write(cif_dev, CIF_CTRL, val & (~CIF_CTRL_ENABLE_CAPTURE));
+ cif_write(cif_dev, CIF_INTEN, 0x0);
+ cif_write(cif_dev, CIF_INTSTAT, 0x3ff);
+ cif_write(cif_dev, CIF_FRAME_STATUS, 0x0);
+
+ stream->stopping = false;
+}
+
+static int cif_queue_setup(struct vb2_queue *queue,
+ unsigned int *num_buffers,
+ unsigned int *num_planes,
+ unsigned int sizes[],
+ struct device *alloc_devs[])
+{
+ struct cif_stream *stream = queue->drv_priv;
+ const struct v4l2_pix_format *pix;
+
+ pix = &stream->pix;
+
+ if (*num_planes) {
+ if (*num_planes != 1)
+ return -EINVAL;
+
+ if (sizes[0] < pix->sizeimage)
+ return -EINVAL;
+ return 0;
+ }
+
+ *num_planes = 1;
+
+ sizes[0] = pix->sizeimage;
+
+ *num_buffers = CIF_REQ_BUFS_MIN;
+
+ return 0;
+}
+
+static void cif_buf_queue(struct vb2_buffer *vb)
+{
+ struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+ struct cif_buffer *cifbuf = to_cif_buffer(vbuf);
+ struct vb2_queue *queue = vb->vb2_queue;
+ struct cif_stream *stream = queue->drv_priv;
+ struct v4l2_pix_format *pix = &stream->pix;
+ unsigned long lock_flags;
+ int i;
+
+ struct cif_output_fmt *fmt = stream->cif_fmt_out;
+
+ memset(cifbuf->buff_addr, 0, sizeof(cifbuf->buff_addr));
+
+ cifbuf->buff_addr[0] = vb2_dma_contig_plane_dma_addr(vb, 0);
+
+ for (i = 0; i < fmt->cplanes - 1; i++)
+ cifbuf->buff_addr[i + 1] = cifbuf->buff_addr[i] +
+ pix->bytesperline * pix->height;
+
+ spin_lock_irqsave(&stream->vbq_lock, lock_flags);
+ list_add_tail(&cifbuf->queue, &stream->buf_head);
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+}
+
+static void cif_return_all_buffers(struct cif_stream *stream,
+ enum vb2_buffer_state state)
+{
+ struct cif_buffer *buf;
+ unsigned long lock_flags;
+
+ spin_lock_irqsave(&stream->vbq_lock, lock_flags);
+
+ if (stream->buffs[0]) {
+ vb2_buffer_done(&stream->buffs[0]->vb.vb2_buf, state);
+ stream->buffs[0] = NULL;
+ }
+
+ if (stream->buffs[1]) {
+ if (!stream->drop_frame)
+ vb2_buffer_done(&stream->buffs[1]->vb.vb2_buf, state);
+
+ stream->buffs[1] = NULL;
+ }
+
+ while (!list_empty(&stream->buf_head)) {
+ buf = cif_get_buffer(stream);
+ vb2_buffer_done(&buf->vb.vb2_buf, state);
+ }
+
+ spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
+}
+
+static void cif_stop_streaming(struct vb2_queue *queue)
+{
+ struct cif_stream *stream = queue->drv_priv;
+ struct cif_device *cif_dev = stream->cifdev;
+ struct v4l2_subdev *sd;
+ int ret;
+
+ stream->stopping = true;
+ ret = wait_event_timeout(stream->wq_stopped,
+ !stream->stopping,
+ msecs_to_jiffies(1000));
+ if (!ret) {
+ cif_stream_stop(stream);
+ stream->stopping = false;
+ }
+
+ /* Stop the sub device. */
+ sd = cif_dev->remote.sd;
+ v4l2_subdev_call(sd, video, s_stream, 0);
+
+ pm_runtime_put(cif_dev->dev);
+
+ cif_return_all_buffers(stream, VB2_BUF_STATE_ERROR);
+}
+
+static int cif_stream_start(struct cif_stream *stream)
+{
+ u32 val, mbus_flags, href_pol, vsync_pol, fmt_type,
+ xfer_mode = 0, yc_swap = 0;
+ struct cif_device *cif_dev = stream->cifdev;
+ struct cif_remote *remote_info;
+ int ret;
+ u32 input_mode;
+
+ remote_info = &cif_dev->remote;
+ fmt_type = stream->cif_fmt_in->fmt_type;
+ stream->frame_idx = 0;
+ input_mode = (remote_info->std == V4L2_STD_NTSC) ?
+ CIF_FORMAT_INPUT_MODE_NTSC :
+ CIF_FORMAT_INPUT_MODE_PAL;
+ mbus_flags = remote_info->mbus.bus.parallel.flags;
+ href_pol = (mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) ?
+ 0 : CIF_FORMAT_HSY_LOW_ACTIVE;
+ vsync_pol = (mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) ?
+ CIF_FORMAT_VSY_HIGH_ACTIVE : 0;
+
+ val = vsync_pol | href_pol | input_mode | stream->cif_fmt_out->fmt_val |
+ stream->cif_fmt_in->dvp_fmt_val | xfer_mode | yc_swap;
+ cif_write(cif_dev, CIF_FOR, val);
+
+ val = stream->pix.width;
+ if (stream->cif_fmt_in->fmt_type == CIF_FMT_TYPE_RAW)
+ val = stream->pix.width * 2;
+
+ cif_write(cif_dev, CIF_VIR_LINE_WIDTH, val);
+ cif_write(cif_dev, CIF_SET_SIZE,
+ stream->pix.width | (stream->pix.height << 16));
+
+ cif_write(cif_dev, CIF_FRAME_STATUS, CIF_FRAME_STAT_CLS);
+ cif_write(cif_dev, CIF_INTSTAT, CIF_INTSTAT_CLS);
+ cif_write(cif_dev, CIF_SCL_CTRL, (fmt_type == CIF_FMT_TYPE_YUV) ?
+ CIF_SCL_CTRL_ENABLE_YUV_16BIT_BYPASS :
+ CIF_SCL_CTRL_ENABLE_RAW_16BIT_BYPASS);
+
+ ret = cif_init_buffers(stream);
+ if (ret)
+ return ret;
+
+ cif_write(cif_dev, CIF_INTEN, CIF_INTEN_FRAME_END_EN |
+ CIF_INTEN_LINE_ERR_EN |
+ CIF_INTEN_PST_INF_FRAME_END_EN);
+
+ cif_write(cif_dev, CIF_CTRL, CIF_CTRL_AXI_BURST_16 |
+ CIF_CTRL_MODE_PINGPONG |
+ CIF_CTRL_ENABLE_CAPTURE);
+
+ return 0;
+}
+
+static int cif_start_streaming(struct vb2_queue *queue, unsigned int count)
+{
+ struct cif_stream *stream = queue->drv_priv;
+ struct cif_device *cif_dev = stream->cifdev;
+ struct v4l2_device *v4l2_dev = &cif_dev->v4l2_dev;
+ struct v4l2_subdev *sd;
+ int ret;
+
+ if (!cif_dev->remote.sd) {
+ ret = -ENODEV;
+ v4l2_err(v4l2_dev, "No remote subdev detected\n");
+ goto destroy_buf;
+ }
+
+ ret = pm_runtime_get_sync(cif_dev->dev);
+ if (ret < 0) {
+ v4l2_err(v4l2_dev, "Failed to get runtime pm, %d\n", ret);
+ goto destroy_buf;
+ }
+
+ sd = cif_dev->remote.sd;
+ stream->cif_fmt_in = get_input_fmt(cif_dev->remote.sd);
+
+ ret = cif_stream_start(stream);
+ if (ret < 0)
+ goto stop_stream;
+
+ ret = v4l2_subdev_call(sd, video, s_stream, 1);
+ if (ret < 0)
+ goto runtime_put;
+
+ return 0;
+
+runtime_put:
+ pm_runtime_put(cif_dev->dev);
+stop_stream:
+ cif_stream_stop(stream);
+destroy_buf:
+ cif_return_all_buffers(stream, VB2_BUF_STATE_QUEUED);
+
+ return ret;
+}
+
+static const struct vb2_ops cif_vb2_ops = {
+ .queue_setup = cif_queue_setup,
+ .buf_queue = cif_buf_queue,
+ .wait_prepare = vb2_ops_wait_prepare,
+ .wait_finish = vb2_ops_wait_finish,
+ .stop_streaming = cif_stop_streaming,
+ .start_streaming = cif_start_streaming,
+};
+
+static int cif_init_vb2_queue(struct vb2_queue *q,
+ struct cif_stream *stream)
+{
+ q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ q->io_modes = VB2_MMAP | VB2_DMABUF;
+ q->drv_priv = stream;
+ q->ops = &cif_vb2_ops;
+ q->mem_ops = &vb2_dma_contig_memops;
+ q->buf_struct_size = sizeof(struct cif_buffer);
+ q->min_buffers_needed = CIF_REQ_BUFS_MIN;
+ q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+ q->lock = &stream->vlock;
+ q->dev = stream->cifdev->dev;
+
+ return vb2_queue_init(q);
+}
+
+static void cif_update_pix(struct cif_stream *stream,
+ struct cif_output_fmt *fmt,
+ struct v4l2_pix_format *pix)
+{
+ struct cif_remote *remote_info = &stream->cifdev->remote;
+ struct v4l2_subdev_format sd_fmt;
+ u32 width, height;
+
+ sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ sd_fmt.pad = 0;
+ v4l2_subdev_call(remote_info->sd, pad, get_fmt, NULL, &sd_fmt);
+
+ width = clamp_t(u32, sd_fmt.format.width,
+ CIF_MIN_WIDTH, CIF_MAX_WIDTH);
+ height = clamp_t(u32, sd_fmt.format.height,
+ CIF_MIN_HEIGHT, CIF_MAX_HEIGHT);
+
+ pix->width = width;
+ pix->height = height;
+ pix->field = sd_fmt.format.field;
+ pix->colorspace = sd_fmt.format.colorspace;
+ pix->ycbcr_enc = sd_fmt.format.ycbcr_enc;
+ pix->quantization = sd_fmt.format.quantization;
+ pix->xfer_func = sd_fmt.format.xfer_func;
+
+ v4l2_fill_pixfmt(pix, fmt->fourcc, pix->width, pix->height);
+}
+
+static int cif_set_fmt(struct cif_stream *stream,
+ struct v4l2_pix_format *pix)
+{
+ struct cif_device *cif_dev = stream->cifdev;
+ struct v4l2_subdev_format sd_fmt;
+ struct cif_output_fmt *fmt;
+ int ret;
+
+ if (vb2_is_streaming(&stream->buf_queue))
+ return -EBUSY;
+
+ fmt = find_output_fmt(stream, pix->pixelformat);
+ if (!fmt)
+ fmt = &out_fmts[0];
+
+ sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ sd_fmt.pad = 0;
+ sd_fmt.format.width = pix->width;
+ sd_fmt.format.height = pix->height;
+
+ ret = v4l2_subdev_call(cif_dev->remote.sd, pad, set_fmt, NULL, &sd_fmt);
+ if (ret)
+ return ret;
+
+ cif_update_pix(stream, fmt, pix);
+ stream->pix = *pix;
+ stream->cif_fmt_out = fmt;
+
+ return 0;
+}
+
+void cif_set_default_format(struct cif_device *cif_dev)
+{
+ struct cif_stream *stream = &cif_dev->stream;
+ struct v4l2_pix_format pix;
+
+ cif_dev->remote.std = V4L2_STD_NTSC;
+
+ pix.pixelformat = V4L2_PIX_FMT_NV12;
+ pix.width = CIF_DEFAULT_WIDTH;
+ pix.height = CIF_DEFAULT_HEIGHT;
+
+ cif_set_fmt(stream, &pix);
+}
+
+void cif_stream_init(struct cif_device *cif_dev)
+{
+ struct cif_stream *stream = &cif_dev->stream;
+ struct v4l2_pix_format pix;
+
+ memset(stream, 0, sizeof(*stream));
+ memset(&pix, 0, sizeof(pix));
+ stream->cifdev = cif_dev;
+
+ INIT_LIST_HEAD(&stream->buf_head);
+ spin_lock_init(&stream->vbq_lock);
+ init_waitqueue_head(&stream->wq_stopped);
+}
+
+static const struct v4l2_file_operations cif_fops = {
+ .open = v4l2_fh_open,
+ .release = vb2_fop_release,
+ .unlocked_ioctl = video_ioctl2,
+ .poll = vb2_fop_poll,
+ .mmap = vb2_fop_mmap,
+};
+
+static int cif_enum_input(struct file *file, void *priv,
+ struct v4l2_input *input)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct v4l2_subdev *sd = stream->cifdev->remote.sd;
+ int ret;
+
+ if (input->index > 0)
+ return -EINVAL;
+
+ ret = v4l2_subdev_call(sd, video, g_input_status, &input->status);
+ if (ret)
+ return ret;
+
+ strscpy(input->name, "Camera", sizeof(input->name));
+ input->type = V4L2_INPUT_TYPE_CAMERA;
+ input->std = stream->vdev.tvnorms;
+ input->capabilities = V4L2_IN_CAP_STD;
+
+ return 0;
+}
+
+static int cif_try_fmt_vid_cap(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct cif_output_fmt *fmt;
+
+ fmt = find_output_fmt(stream, f->fmt.pix.pixelformat);
+ if (!fmt)
+ fmt = &out_fmts[0];
+
+ cif_update_pix(stream, fmt, &f->fmt.pix);
+
+ return 0;
+}
+
+static int cif_g_std(struct file *file, void *fh, v4l2_std_id *norm)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct cif_remote *remote_info = &stream->cifdev->remote;
+
+ *norm = remote_info->std;
+
+ return 0;
+}
+
+static int cif_s_std(struct file *file, void *fh, v4l2_std_id norm)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct cif_remote *remote_info = &stream->cifdev->remote;
+ int ret;
+
+ if (norm == remote_info->std)
+ return 0;
+
+ if (vb2_is_busy(&stream->buf_queue))
+ return -EBUSY;
+
+ ret = v4l2_subdev_call(remote_info->sd, video, s_std, norm);
+ if (ret)
+ return ret;
+
+ remote_info->std = norm;
+
+ /* S_STD will update the format since that depends on the standard. */
+ cif_update_pix(stream, stream->cif_fmt_out, &stream->pix);
+
+ return 0;
+}
+
+static int cif_querystd(struct file *file, void *fh, v4l2_std_id *a)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct cif_remote *remote_info = &stream->cifdev->remote;
+
+ *a = V4L2_STD_UNKNOWN;
+
+ return v4l2_subdev_call(remote_info->sd, video, querystd, a);
+}
+
+static int cif_enum_fmt_vid_cap(struct file *file, void *priv,
+ struct v4l2_fmtdesc *f)
+{
+ struct cif_output_fmt *fmt = NULL;
+
+ if (f->index >= ARRAY_SIZE(out_fmts))
+ return -EINVAL;
+
+ fmt = &out_fmts[f->index];
+ f->pixelformat = fmt->fourcc;
+
+ return 0;
+}
+
+static int cif_s_fmt_vid_cap(struct file *file,
+ void *priv, struct v4l2_format *f)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ int ret;
+
+ if (vb2_is_busy(&stream->buf_queue))
+ return -EBUSY;
+
+ ret = cif_set_fmt(stream, &f->fmt.pix);
+
+ return ret;
+}
+
+static int cif_g_fmt_vid_cap(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct cif_stream *stream = video_drvdata(file);
+
+ f->fmt.pix = stream->pix;
+
+ return 0;
+}
+
+static int cif_querycap(struct file *file, void *priv,
+ struct v4l2_capability *cap)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct device *dev = stream->cifdev->dev;
+
+ strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
+ strscpy(cap->card, dev->driver->name, sizeof(cap->card));
+ snprintf(cap->bus_info, sizeof(cap->bus_info),
+ "platform:%s", dev_name(dev));
+
+ return 0;
+}
+
+static int cif_enum_framesizes(struct file *file, void *fh,
+ struct v4l2_frmsizeenum *fsize)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct cif_device *cif_dev = stream->cifdev;
+ struct v4l2_subdev_frame_size_enum fse = {
+ .index = fsize->index,
+ .which = V4L2_SUBDEV_FORMAT_ACTIVE,
+ };
+ struct cif_output_fmt *fmt;
+ int ret;
+
+ if (!cif_dev->remote.sd)
+ return -ENODEV;
+
+ fmt = find_output_fmt(stream, fsize->pixel_format);
+ if (!fmt)
+ return -EINVAL;
+
+ fse.code = fmt->mbus;
+
+ ret = v4l2_subdev_call(cif_dev->remote.sd, pad, enum_frame_size,
+ NULL, &fse);
+ if (ret)
+ return ret;
+
+ fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+ fsize->discrete.width = fse.max_width;
+ fsize->discrete.height = fse.max_height;
+
+ return 0;
+}
+
+static int cif_enum_frameintervals(struct file *file, void *fh,
+ struct v4l2_frmivalenum *fival)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct cif_device *cif_dev = stream->cifdev;
+ struct v4l2_subdev_frame_interval_enum fie = {
+ .index = fival->index,
+ .width = fival->width,
+ .height = fival->height,
+ .which = V4L2_SUBDEV_FORMAT_ACTIVE,
+ };
+ struct cif_output_fmt *fmt;
+ int ret;
+
+ if (!cif_dev->remote.sd)
+ return -ENODEV;
+
+ fmt = find_output_fmt(stream, fival->pixel_format);
+ if (!fmt)
+ return -EINVAL;
+
+ fie.code = fmt->mbus;
+
+ ret = v4l2_subdev_call(cif_dev->remote.sd, pad, enum_frame_interval,
+ NULL, &fie);
+ if (ret)
+ return ret;
+
+ fival->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+ fival->discrete = fie.interval;
+
+ return 0;
+}
+
+static int cif_g_input(struct file *file, void *fh, unsigned int *i)
+{
+ *i = 0;
+ return 0;
+}
+
+static int cif_s_input(struct file *file, void *fh, unsigned int i)
+{
+ if (i)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int cif_g_parm(struct file *file, void *priv, struct v4l2_streamparm *p)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct cif_device *cif_dev = stream->cifdev;
+
+ if (!cif_dev->remote.sd)
+ return -ENODEV;
+
+ return v4l2_g_parm_cap(video_devdata(file), cif_dev->remote.sd, p);
+}
+
+static int cif_s_parm(struct file *file, void *priv, struct v4l2_streamparm *p)
+{
+ struct cif_stream *stream = video_drvdata(file);
+ struct cif_device *cif_dev = stream->cifdev;
+
+ if (!cif_dev->remote.sd)
+ return -ENODEV;
+
+ return v4l2_s_parm_cap(video_devdata(file), cif_dev->remote.sd, p);
+}
+
+static const struct v4l2_ioctl_ops cif_v4l2_ioctl_ops = {
+ .vidioc_reqbufs = vb2_ioctl_reqbufs,
+ .vidioc_querybuf = vb2_ioctl_querybuf,
+ .vidioc_create_bufs = vb2_ioctl_create_bufs,
+ .vidioc_qbuf = vb2_ioctl_qbuf,
+ .vidioc_expbuf = vb2_ioctl_expbuf,
+ .vidioc_dqbuf = vb2_ioctl_dqbuf,
+ .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
+ .vidioc_streamon = vb2_ioctl_streamon,
+ .vidioc_streamoff = vb2_ioctl_streamoff,
+
+ .vidioc_g_std = cif_g_std,
+ .vidioc_s_std = cif_s_std,
+ .vidioc_querystd = cif_querystd,
+
+ .vidioc_enum_fmt_vid_cap = cif_enum_fmt_vid_cap,
+ .vidioc_try_fmt_vid_cap = cif_try_fmt_vid_cap,
+ .vidioc_s_fmt_vid_cap = cif_s_fmt_vid_cap,
+ .vidioc_g_fmt_vid_cap = cif_g_fmt_vid_cap,
+ .vidioc_querycap = cif_querycap,
+ .vidioc_enum_framesizes = cif_enum_framesizes,
+ .vidioc_enum_frameintervals = cif_enum_frameintervals,
+
+ .vidioc_enum_input = cif_enum_input,
+ .vidioc_g_input = cif_g_input,
+ .vidioc_s_input = cif_s_input,
+
+ .vidioc_g_parm = cif_g_parm,
+ .vidioc_s_parm = cif_s_parm,
+};
+
+void cif_unregister_stream_vdev(struct cif_device *cif_dev)
+{
+ struct cif_stream *stream = &cif_dev->stream;
+
+ media_entity_cleanup(&stream->vdev.entity);
+ video_unregister_device(&stream->vdev);
+}
+
+int cif_register_stream_vdev(struct cif_device *cif_dev)
+{
+ struct cif_stream *stream = &cif_dev->stream;
+ struct v4l2_device *v4l2_dev = &cif_dev->v4l2_dev;
+ struct video_device *vdev = &stream->vdev;
+ int ret;
+
+ strscpy(vdev->name, CIF_DRIVER_NAME, sizeof(vdev->name));
+ mutex_init(&stream->vlock);
+
+ vdev->ioctl_ops = &cif_v4l2_ioctl_ops;
+ vdev->release = video_device_release_empty;
+ vdev->fops = &cif_fops;
+ vdev->minor = -1;
+ vdev->v4l2_dev = v4l2_dev;
+ vdev->lock = &stream->vlock;
+ vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE |
+ V4L2_CAP_STREAMING;
+ vdev->tvnorms = V4L2_STD_NTSC | V4L2_STD_PAL;
+ video_set_drvdata(vdev, stream);
+ vdev->vfl_dir = VFL_DIR_RX;
+ stream->pad.flags = MEDIA_PAD_FL_SINK;
+
+ cif_init_vb2_queue(&stream->buf_queue, stream);
+
+ vdev->queue = &stream->buf_queue;
+ strscpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
+
+ ret = media_entity_pads_init(&vdev->entity, 1, &stream->pad);
+ if (ret < 0)
+ return ret;
+
+ ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
+ if (ret < 0)
+ v4l2_err(v4l2_dev,
+ "video_register_device failed with error %d\n", ret);
+
+ return ret;
+}
+
+static void cif_vb_done(struct cif_stream *stream,
+ struct vb2_v4l2_buffer *vb_done)
+{
+ vb2_set_plane_payload(&vb_done->vb2_buf, 0,
+ stream->pix.sizeimage);
+ vb_done->vb2_buf.timestamp = ktime_get_ns();
+ vb_done->sequence = stream->frame_idx;
+ vb2_buffer_done(&vb_done->vb2_buf, VB2_BUF_STATE_DONE);
+}
+
+static void cif_reset_stream(struct cif_device *cif_dev)
+{
+ u32 ctl = cif_read(cif_dev, CIF_CTRL);
+
+ cif_write(cif_dev, CIF_CTRL, ctl & (~CIF_CTRL_ENABLE_CAPTURE));
+ cif_write(cif_dev, CIF_CTRL, ctl | CIF_CTRL_ENABLE_CAPTURE);
+}
+
+irqreturn_t cif_irq_pingpong(int irq, void *ctx)
+{
+ struct device *dev = ctx;
+ struct cif_device *cif_dev = dev_get_drvdata(dev);
+ struct cif_stream *stream = &cif_dev->stream;
+ unsigned int intstat;
+ u32 lastline, lastpix, ctl, cif_frmst;
+
+ intstat = cif_read(cif_dev, CIF_INTSTAT);
+ cif_frmst = cif_read(cif_dev, CIF_FRAME_STATUS);
+ lastline = CIF_FETCH_Y_LAST_LINE(cif_read(cif_dev, CIF_LAST_LINE));
+ lastpix = CIF_FETCH_Y_LAST_LINE(cif_read(cif_dev, CIF_LAST_PIX));
+ ctl = cif_read(cif_dev, CIF_CTRL);
+
+ /*
+ * There are two irqs enabled:
+ * - PST_INF_FRAME_END: cif FIFO is ready,
+ * this is prior to FRAME_END
+ * - FRAME_END: cif has saved frame to memory,
+ * a frame ready
+ */
+
+ if (intstat & CIF_INTSTAT_PST_INF_FRAME_END) {
+ cif_write(cif_dev, CIF_INTSTAT,
+ CIF_INTSTAT_PST_INF_FRAME_END_CLR);
+
+ if (stream->stopping)
+ /* To stop CIF ASAP, before FRAME_END irq. */
+ cif_write(cif_dev, CIF_CTRL,
+ ctl & (~CIF_CTRL_ENABLE_CAPTURE));
+ }
+
+ if (intstat & CIF_INTSTAT_PRE_INF_FRAME_END)
+ cif_write(cif_dev, CIF_INTSTAT, CIF_INTSTAT_PRE_INF_FRAME_END);
+
+ if (intstat & (CIF_INTSTAT_LINE_ERR | CIF_INTSTAT_PIX_ERR)) {
+ v4l2_err(&cif_dev->v4l2_dev,
+ "LINE_ERR OR PIX_ERR detected, stream will be reset");
+ cif_write(cif_dev, CIF_INTSTAT, CIF_INTSTAT_LINE_ERR |
+ CIF_INTSTAT_PIX_ERR);
+ cif_reset_stream(cif_dev);
+ }
+
+ if (intstat & CIF_INTSTAT_FRAME_END) {
+ struct vb2_v4l2_buffer *vb_done = NULL;
+
+ cif_write(cif_dev, CIF_INTSTAT, CIF_INTSTAT_FRAME_END_CLR |
+ CIF_INTSTAT_LINE_END_CLR);
+
+ if (stream->stopping) {
+ cif_stream_stop(stream);
+ wake_up(&stream->wq_stopped);
+ return IRQ_HANDLED;
+ }
+
+ if (lastline != stream->pix.height) {
+ v4l2_err(&cif_dev->v4l2_dev,
+ "Bad frame, irq:%#x frmst:%#x size:%dx%d\n",
+ intstat, cif_frmst, lastpix, lastline);
+
+ cif_reset_stream(cif_dev);
+ }
+
+ if (cif_frmst & CIF_INTSTAT_F0_READY)
+ stream->frame_phase = 0;
+ else if (cif_frmst & CIF_INTSTAT_F1_READY)
+ stream->frame_phase = 1;
+ else
+ return IRQ_HANDLED;
+
+ vb_done = &stream->buffs[stream->frame_phase]->vb;
+ if (!stream->drop_frame) {
+ cif_vb_done(stream, vb_done);
+ stream->frame_idx++;
+ }
+
+ cif_assign_new_buffer_pingpong(stream);
+ }
+
+ return IRQ_HANDLED;
+}
diff --git a/drivers/media/platform/rockchip/cif/dev.c b/drivers/media/platform/rockchip/cif/dev.c
new file mode 100644
index 000000000000..f7d061a13577
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/dev.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Rockchip CIF Camera Interface Driver
+ *
+ * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
+ * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@bootlin.com>
+ * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
+ */
+
+#include "linux/platform_device.h"
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/of_platform.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/reset.h>
+#include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
+#include <media/v4l2-fwnode.h>
+
+#include "dev.h"
+#include "regs.h"
+
+static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
+{
+ struct cif_device *cif_dev;
+ struct v4l2_subdev *sd;
+ int ret;
+
+ cif_dev = container_of(notifier, struct cif_device, notifier);
+ sd = cif_dev->remote.sd;
+
+ mutex_lock(&cif_dev->media_dev.graph_mutex);
+
+ ret = v4l2_device_register_subdev_nodes(&cif_dev->v4l2_dev);
+ if (ret < 0)
+ goto unlock;
+
+ ret = media_create_pad_link(&sd->entity, 0,
+ &cif_dev->stream.vdev.entity, 0,
+ MEDIA_LNK_FL_ENABLED);
+ if (ret)
+ dev_err(cif_dev->dev, "failed to create link");
+
+unlock:
+ mutex_unlock(&cif_dev->media_dev.graph_mutex);
+ return ret;
+}
+
+static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_connection *asd)
+{
+ struct cif_device *cif_dev = container_of(notifier,
+ struct cif_device, notifier);
+ int pad;
+
+ cif_dev->remote.sd = subdev;
+ pad = media_entity_get_fwnode_pad(&subdev->entity, subdev->fwnode,
+ MEDIA_PAD_FL_SOURCE);
+ if (pad < 0)
+ return pad;
+
+ cif_dev->remote.pad = pad;
+
+ return 0;
+}
+
+static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
+ .bound = subdev_notifier_bound,
+ .complete = subdev_notifier_complete,
+};
+
+static int cif_subdev_notifier(struct cif_device *cif_dev)
+{
+ struct v4l2_async_notifier *ntf = &cif_dev->notifier;
+ struct device *dev = cif_dev->dev;
+ struct v4l2_async_connection *asd;
+ struct v4l2_fwnode_endpoint vep = {
+ .bus_type = V4L2_MBUS_PARALLEL,
+ };
+ struct fwnode_handle *ep;
+ int ret;
+
+ v4l2_async_nf_init(ntf, &cif_dev->v4l2_dev);
+
+ ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
+ FWNODE_GRAPH_ENDPOINT_NEXT);
+ if (!ep)
+ return -EINVAL;
+
+ ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+ if (ret)
+ return ret;
+
+ asd = v4l2_async_nf_add_fwnode_remote(ntf, ep,
+ struct v4l2_async_connection);
+ if (IS_ERR(asd))
+ return PTR_ERR(asd);
+
+ ntf->ops = &subdev_notifier_ops;
+
+ fwnode_handle_put(ep);
+
+ return v4l2_async_nf_register(ntf);
+}
+
+static struct clk_bulk_data px30_cif_clks[] = {
+ { .id = "aclk", },
+ { .id = "hclk", },
+ { .id = "pclk", },
+};
+
+static const struct cif_match_data px30_cif_match_data = {
+ .clks = px30_cif_clks,
+ .clks_num = ARRAY_SIZE(px30_cif_clks),
+};
+
+static const struct of_device_id cif_plat_of_match[] = {
+ {
+ .compatible = "rockchip,px30-vip",
+ .data = &px30_cif_match_data,
+ },
+ {},
+};
+
+static int cif_get_resource(struct platform_device *pdev,
+ struct cif_device *cif_dev)
+{
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev,
+ "Unable to allocate resources for device\n");
+ return -ENODEV;
+ }
+
+ cif_dev->base_addr = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(cif_dev->base_addr))
+ return PTR_ERR(cif_dev->base_addr);
+
+ return 0;
+}
+
+static int cif_plat_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct v4l2_device *v4l2_dev;
+ struct cif_device *cif_dev;
+ int ret, irq;
+
+ cif_dev = devm_kzalloc(dev, sizeof(*cif_dev), GFP_KERNEL);
+ if (!cif_dev)
+ return -ENOMEM;
+
+ cif_dev->match_data = of_device_get_match_data(dev);
+ if (!cif_dev->match_data)
+ return -ENODEV;
+
+ platform_set_drvdata(pdev, cif_dev);
+ cif_dev->dev = dev;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ret = devm_request_irq(dev, irq, cif_irq_pingpong, IRQF_SHARED,
+ dev_driver_string(dev), dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "request irq failed\n");
+
+ cif_dev->irq = irq;
+
+ ret = cif_get_resource(pdev, cif_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_clk_bulk_get(dev, cif_dev->match_data->clks_num,
+ cif_dev->match_data->clks);
+ if (ret)
+ return ret;
+
+ cif_dev->cif_rst = devm_reset_control_array_get(dev, false, false);
+ if (IS_ERR(cif_dev->cif_rst))
+ return PTR_ERR(cif_dev->cif_rst);
+
+ cif_stream_init(cif_dev);
+ strscpy(cif_dev->media_dev.model, "cif",
+ sizeof(cif_dev->media_dev.model));
+ cif_dev->media_dev.dev = &pdev->dev;
+ v4l2_dev = &cif_dev->v4l2_dev;
+ v4l2_dev->mdev = &cif_dev->media_dev;
+ strscpy(v4l2_dev->name, "rockchip-cif", sizeof(v4l2_dev->name));
+
+ ret = v4l2_device_register(cif_dev->dev, &cif_dev->v4l2_dev);
+ if (ret < 0)
+ return ret;
+
+ media_device_init(&cif_dev->media_dev);
+
+ ret = media_device_register(&cif_dev->media_dev);
+ if (ret < 0)
+ goto err_unreg_v4l2_dev;
+
+ /* Create & register platform subdev. */
+ ret = cif_register_stream_vdev(cif_dev);
+ if (ret < 0)
+ goto err_unreg_media_dev;
+
+ ret = cif_subdev_notifier(cif_dev);
+ if (ret < 0) {
+ v4l2_err(&cif_dev->v4l2_dev,
+ "Failed to register subdev notifier(%d)\n", ret);
+ goto err_unreg_stream_vdev;
+ }
+
+ cif_set_default_format(cif_dev);
+ pm_runtime_enable(&pdev->dev);
+
+ return 0;
+
+err_unreg_stream_vdev:
+ cif_unregister_stream_vdev(cif_dev);
+err_unreg_media_dev:
+ media_device_unregister(&cif_dev->media_dev);
+err_unreg_v4l2_dev:
+ v4l2_device_unregister(&cif_dev->v4l2_dev);
+ return ret;
+}
+
+static int cif_plat_remove(struct platform_device *pdev)
+{
+ struct cif_device *cif_dev = platform_get_drvdata(pdev);
+
+ pm_runtime_disable(&pdev->dev);
+
+ media_device_unregister(&cif_dev->media_dev);
+ v4l2_device_unregister(&cif_dev->v4l2_dev);
+ cif_unregister_stream_vdev(cif_dev);
+
+ return 0;
+}
+
+static int __maybe_unused cif_runtime_suspend(struct device *dev)
+{
+ struct cif_device *cif_dev = dev_get_drvdata(dev);
+
+ clk_bulk_disable_unprepare(cif_dev->match_data->clks_num,
+ cif_dev->match_data->clks);
+
+ return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int __maybe_unused cif_runtime_resume(struct device *dev)
+{
+ struct cif_device *cif_dev = dev_get_drvdata(dev);
+ int ret;
+
+ ret = pinctrl_pm_select_default_state(dev);
+ if (ret < 0)
+ return ret;
+
+ return clk_bulk_prepare_enable(cif_dev->match_data->clks_num,
+ cif_dev->match_data->clks);
+}
+
+static const struct dev_pm_ops cif_plat_pm_ops = {
+ .runtime_suspend = cif_runtime_suspend,
+ .runtime_resume = cif_runtime_resume,
+};
+
+static struct platform_driver cif_plat_drv = {
+ .driver = {
+ .name = CIF_DRIVER_NAME,
+ .of_match_table = cif_plat_of_match,
+ .pm = &cif_plat_pm_ops,
+ },
+ .probe = cif_plat_probe,
+ .remove = cif_plat_remove,
+};
+module_platform_driver(cif_plat_drv);
+
+MODULE_AUTHOR("Rockchip Camera/ISP team");
+MODULE_DESCRIPTION("Rockchip CIF platform driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/rockchip/cif/dev.h b/drivers/media/platform/rockchip/cif/dev.h
new file mode 100644
index 000000000000..3be3db08c75b
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/dev.h
@@ -0,0 +1,139 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Rockchip CIF Driver
+ *
+ * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
+ * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
+ */
+
+#ifndef _CIF_DEV_H
+#define _CIF_DEV_H
+
+#include <linux/clk.h>
+#include <linux/mutex.h>
+
+#include <media/media-device.h>
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/videobuf2-v4l2.h>
+
+#define CIF_DRIVER_NAME "rockchip-cif"
+
+#define CIF_MAX_BUS_CLK 8
+#define CIF_MAX_SENSOR 1
+#define CIF_MAX_RESET 5
+#define CIF_MAX_CSI_CHANNEL 4
+
+#define CIF_DEFAULT_WIDTH 640
+#define CIF_DEFAULT_HEIGHT 480
+
+struct cif_buffer {
+ struct vb2_v4l2_buffer vb;
+ struct list_head queue;
+ u32 buff_addr[VIDEO_MAX_PLANES];
+};
+
+static inline struct cif_buffer *to_cif_buffer(struct vb2_v4l2_buffer *vb)
+{
+ return container_of(vb, struct cif_buffer, vb);
+}
+
+struct cif_remote {
+ struct v4l2_subdev *sd;
+ int pad;
+ struct v4l2_mbus_config mbus;
+ int lanes;
+ v4l2_std_id std;
+};
+
+struct cif_output_fmt {
+ u32 fourcc;
+ u32 mbus;
+ u32 fmt_val;
+ u8 cplanes;
+};
+
+enum cif_fmt_type {
+ CIF_FMT_TYPE_YUV = 0,
+ CIF_FMT_TYPE_RAW,
+};
+
+struct cif_input_fmt {
+ u32 mbus_code;
+ u32 dvp_fmt_val;
+ u32 csi_fmt_val;
+ enum cif_fmt_type fmt_type;
+ enum v4l2_field field;
+};
+
+struct cif_stream {
+ struct cif_device *cifdev;
+ bool stopping;
+ wait_queue_head_t wq_stopped;
+ int frame_idx;
+ int frame_phase;
+ bool drop_frame;
+
+ /* Lock between irq and buf_queue, buffs. */
+ spinlock_t vbq_lock;
+ struct vb2_queue buf_queue;
+ struct list_head buf_head;
+ struct cif_buffer *buffs[2];
+
+ /* Vfd lock. */
+ struct mutex vlock;
+ struct video_device vdev;
+ struct media_pad pad;
+
+ struct cif_output_fmt *cif_fmt_out;
+ const struct cif_input_fmt *cif_fmt_in;
+ struct v4l2_pix_format pix;
+};
+
+static inline struct cif_stream *to_cif_stream(struct video_device *vdev)
+{
+ return container_of(vdev, struct cif_stream, vdev);
+}
+
+struct cif_match_data {
+ struct clk_bulk_data *clks;
+ int clks_num;
+};
+
+struct cif_device {
+ struct device *dev;
+ int irq;
+ void __iomem *base_addr;
+ struct reset_control *cif_rst;
+
+ struct v4l2_device v4l2_dev;
+ struct media_device media_dev;
+ struct v4l2_async_notifier notifier;
+ struct v4l2_async_connection asd;
+ struct cif_remote remote;
+
+ struct cif_stream stream;
+ const struct cif_match_data *match_data;
+};
+
+static inline void
+cif_write(struct cif_device *cif_dev, unsigned int addr, u32 val)
+{
+ writel(val, cif_dev->base_addr + addr);
+}
+
+static inline u32 cif_read(struct cif_device *cif_dev, unsigned int addr)
+{
+ return readl(cif_dev->base_addr + addr);
+}
+
+void cif_unregister_stream_vdev(struct cif_device *dev);
+int cif_register_stream_vdev(struct cif_device *dev);
+void cif_stream_init(struct cif_device *dev);
+void cif_set_default_format(struct cif_device *dev);
+
+irqreturn_t cif_irq_pingpong(int irq, void *ctx);
+void cif_soft_reset(struct cif_device *cif_dev);
+
+#endif
diff --git a/drivers/media/platform/rockchip/cif/regs.h b/drivers/media/platform/rockchip/cif/regs.h
new file mode 100644
index 000000000000..cc39bf9ce8da
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/regs.h
@@ -0,0 +1,192 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Rockchip CIF Driver
+ *
+ * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
+ * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
+ */
+
+#ifndef _CIF_REGS_H
+#define _CIF_REGS_H
+
+#define CIF_CTRL 0x00
+#define CIF_INTEN 0x04
+#define CIF_INTSTAT 0x08
+#define CIF_FOR 0x0c
+#define CIF_LINE_NUM_ADDR 0x10
+#define CIF_FRM0_ADDR_Y 0x14
+#define CIF_FRM0_ADDR_UV 0x18
+#define CIF_FRM1_ADDR_Y 0x1c
+#define CIF_FRM1_ADDR_UV 0x20
+#define CIF_VIR_LINE_WIDTH 0x24
+#define CIF_SET_SIZE 0x28
+#define CIF_SCM_ADDR_Y 0x2c
+#define CIF_SCM_ADDR_U 0x30
+#define CIF_SCM_ADDR_V 0x34
+#define CIF_WB_UP_FILTER 0x38
+#define CIF_WB_LOW_FILTER 0x3c
+#define CIF_WBC_CNT 0x40
+#define CIF_CROP 0x44
+#define CIF_SCL_CTRL 0x48
+#define CIF_SCL_DST 0x4c
+#define CIF_SCL_FCT 0x50
+#define CIF_SCL_VALID_NUM 0x54
+#define CIF_LINE_LOOP_CTR 0x58
+#define CIF_FRAME_STATUS 0x60
+#define CIF_CUR_DST 0x64
+#define CIF_LAST_LINE 0x68
+#define CIF_LAST_PIX 0x6c
+#define CIF_FETCH_Y_LAST_LINE(VAL) ((VAL) & 0x1fff)
+
+#define CIF_CTRL_ENABLE_CAPTURE BIT(0)
+#define CIF_CTRL_MODE_PINGPONG BIT(1)
+#define CIF_CTRL_MODE_LINELOOP BIT(2)
+#define CIF_CTRL_AXI_BURST_16 (0xf << 12)
+
+#define CIF_INTEN_FRAME_END_EN BIT(0)
+#define CIF_INTEN_LINE_ERR_EN BIT(2)
+#define CIF_INTEN_BUS_ERR_EN BIT(6)
+#define CIF_INTEN_SCL_ERR_EN BIT(7)
+#define CIF_INTEN_PST_INF_FRAME_END_EN BIT(9)
+
+#define CIF_INTSTAT_CLS 0x3ff
+#define CIF_INTSTAT_FRAME_END BIT(0)
+#define CIF_INTSTAT_LINE_END BIT(1)
+#define CIF_INTSTAT_LINE_ERR BIT(2)
+#define CIF_INTSTAT_PIX_ERR BIT(3)
+#define CIF_INTSTAT_DFIFO_OF BIT(5)
+#define CIF_INTSTAT_BUS_ERR BIT(6)
+#define CIF_INTSTAT_PRE_INF_FRAME_END BIT(8)
+#define CIF_INTSTAT_PST_INF_FRAME_END BIT(9)
+#define CIF_INTSTAT_FRAME_END_CLR BIT(0)
+#define CIF_INTSTAT_LINE_END_CLR BIT(1)
+#define CIF_INTSTAT_LINE_ERR_CLR BIT(2)
+#define CIF_INTSTAT_PST_INF_FRAME_END_CLR BIT(9)
+#define CIF_INTSTAT_ERR 0xfc
+
+#define CIF_FRAME_STAT_CLS 0x00
+#define CIF_FRAME_FRM0_STAT_CLS 0x20
+
+#define CIF_FORMAT_VSY_HIGH_ACTIVE BIT(0)
+#define CIF_FORMAT_HSY_LOW_ACTIVE BIT(1)
+
+#define CIF_FORMAT_INPUT_MODE_YUV (0x00 << 2)
+#define CIF_FORMAT_INPUT_MODE_PAL (0x02 << 2)
+#define CIF_FORMAT_INPUT_MODE_NTSC (0x03 << 2)
+#define CIF_FORMAT_INPUT_MODE_BT1120 (0x07 << 2)
+#define CIF_FORMAT_INPUT_MODE_RAW (0x04 << 2)
+#define CIF_FORMAT_INPUT_MODE_JPEG (0x05 << 2)
+#define CIF_FORMAT_INPUT_MODE_MIPI (0x06 << 2)
+
+#define CIF_FORMAT_YUV_INPUT_ORDER_UYVY (0x00 << 5)
+#define CIF_FORMAT_YUV_INPUT_ORDER_YVYU BIT(5)
+#define CIF_FORMAT_YUV_INPUT_ORDER_VYUY BIT(6)
+#define CIF_FORMAT_YUV_INPUT_ORDER_YUYV (0x03 << 5)
+#define CIF_FORMAT_YUV_INPUT_422 (0x00 << 7)
+#define CIF_FORMAT_YUV_INPUT_420 BIT(7)
+
+#define CIF_FORMAT_INPUT_420_ORDER_ODD BIT(8)
+
+#define CIF_FORMAT_CCIR_INPUT_ORDER_EVEN BIT(9)
+
+#define CIF_FORMAT_RAW_DATA_WIDTH_8 (0x00 << 11)
+#define CIF_FORMAT_RAW_DATA_WIDTH_10 BIT(11)
+#define CIF_FORMAT_RAW_DATA_WIDTH_12 (0x02 << 11)
+
+#define CIF_FORMAT_YUV_OUTPUT_422 (0x00 << 16)
+#define CIF_FORMAT_YUV_OUTPUT_420 BIT(16)
+
+#define CIF_FORMAT_OUTPUT_420_ORDER_EVEN (0x00 << 17)
+#define CIF_FORMAT_OUTPUT_420_ORDER_ODD BIT(17)
+
+#define CIF_FORMAT_RAWD_DATA_LITTLE_ENDIAN (0x00 << 18)
+#define CIF_FORMAT_RAWD_DATA_BIG_ENDIAN BIT(18)
+
+#define CIF_FORMAT_UV_STORAGE_ORDER_UVUV (0x00 << 19)
+#define CIF_FORMAT_UV_STORAGE_ORDER_VUVU BIT(19)
+
+#define CIF_FORMAT_BT1120_CLOCK_SINGLE_EDGES (0x00 << 24)
+#define CIF_FORMAT_BT1120_CLOCK_DOUBLE_EDGES BIT(24)
+#define CIF_FORMAT_BT1120_TRANSMIT_INTERFACE (0x00 << 25)
+#define CIF_FORMAT_BT1120_TRANSMIT_PROGRESS BIT(25)
+#define CIF_FORMAT_BT1120_YC_SWAP BIT(26)
+
+#define CIF_SCL_CTRL_ENABLE_SCL_DOWN BIT(0)
+#define CIF_SCL_CTRL_ENABLE_SCL_UP BIT(1)
+#define CIF_SCL_CTRL_ENABLE_YUV_16BIT_BYPASS BIT(4)
+#define CIF_SCL_CTRL_ENABLE_RAW_16BIT_BYPASS BIT(5)
+#define CIF_SCL_CTRL_ENABLE_32BIT_BYPASS BIT(6)
+#define CIF_SCL_CTRL_DISABLE_32BIT_BYPASS (0x00 << 6)
+
+#define CIF_INTSTAT_F0_READY BIT(0)
+#define CIF_INTSTAT_F1_READY BIT(1)
+
+#define CIF_CROP_Y_SHIFT 16
+#define CIF_CROP_X_SHIFT 0
+
+#define CIF_CSI_ENABLE_CAPTURE BIT(0)
+#define CIF_CSI_WRDDR_TYPE_RAW8 (0x0 << 1)
+#define CIF_CSI_WRDDR_TYPE_RAW10 BIT(1)
+#define CIF_CSI_WRDDR_TYPE_RAW12 (0x2 << 1)
+#define CIF_CSI_WRDDR_TYPE_RGB888 (0x3 << 1)
+#define CIF_CSI_WRDDR_TYPE_YUV422 (0x4 << 1)
+#define CIF_CSI_ENABLE_COMMAND_MODE BIT(4)
+#define CIF_CSI_ENABLE_CROP BIT(5)
+
+#define CIF_CSI_FRAME0_START_INTEN(id) (0x1 << ((id) * 2))
+#define CIF_CSI_FRAME1_START_INTEN(id) (0x1 << ((id) * 2 + 1))
+#define CIF_CSI_FRAME0_END_INTEN(id) (0x1 << ((id) * 2 + 8))
+#define CIF_CSI_FRAME1_END_INTEN(id) (0x1 << ((id) * 2 + 9))
+#define CIF_CSI_DMA_Y_FIFO_OVERFLOW_INTEN BIT(16)
+#define CIF_CSI_DMA_UV_FIFO_OVERFLOW_INTEN BIT(17)
+#define CIF_CSI_CONFIG_FIFO_OVERFLOW_INTEN BIT(18)
+#define CIF_CSI_BANDWIDTH_LACK_INTEN BIT(19)
+#define CIF_CSI_RX_FIFO_OVERFLOW_INTEN BIT(20)
+#define CIF_CSI_ALL_FRAME_START_INTEN (0xff << 0)
+#define CIF_CSI_ALL_FRAME_END_INTEN (0xff << 8)
+#define CIF_CSI_ALL_ERROR_INTEN (0x1f << 16)
+
+#define CIF_CSI_FRAME0_START_ID0 BIT(0)
+#define CIF_CSI_FRAME1_START_ID0 BIT(1)
+#define CIF_CSI_FRAME0_START_ID1 BIT(2)
+#define CIF_CSI_FRAME1_START_ID1 BIT(3)
+#define CIF_CSI_FRAME0_START_ID2 BIT(4)
+#define CIF_CSI_FRAME1_START_ID2 BIT(5)
+#define CIF_CSI_FRAME0_START_ID3 BIT(6)
+#define CIF_CSI_FRAME1_START_ID3 BIT(7)
+#define CIF_CSI_FRAME0_END_ID0 BIT(8)
+#define CIF_CSI_FRAME1_END_ID0 BIT(9)
+#define CIF_CSI_FRAME0_END_ID1 BIT(10)
+#define CIF_CSI_FRAME1_END_ID1 BIT(11)
+#define CIF_CSI_FRAME0_END_ID2 BIT(12)
+#define CIF_CSI_FRAME1_END_ID2 BIT(13)
+#define CIF_CSI_FRAME0_END_ID3 BIT(14)
+#define CIF_CSI_FRAME1_END_ID3 BIT(15)
+#define CIF_CSI_DMA_Y_FIFO_OVERFLOW BIT(16)
+#define CIF_CSI_DMA_UV_FIFO_OVERFLOW BIT(17)
+#define CIF_CSI_CONFIG_FIFO_OVERFLOW BIT(18)
+#define CIF_CSI_BANDWIDTH_LACK BIT(19)
+#define CIF_CSI_RX_FIFO_OVERFLOW BIT(20)
+
+#define CIF_CSI_FIFO_OVERFLOW (CIF_CSI_DMA_Y_FIFO_OVERFLOW | \
+ CIF_CSI_DMA_UV_FIFO_OVERFLOW | \
+ CIF_CSI_CONFIG_FIFO_OVERFLOW | \
+ CIF_CSI_RX_FIFO_OVERFLOW)
+
+#define CIF_CSIHOST_N_LANES 0x04
+#define CIF_CSIHOST_PHY_RSTZ 0x0c
+#define CIF_CSIHOST_RESETN 0x10
+#define CIF_CSIHOST_ERR1 0x20
+#define CIF_CSIHOST_ERR2 0x24
+#define CIF_CSIHOST_MSK1 0x28
+#define CIF_CSIHOST_MSK2 0x2c
+#define CIF_CSIHOST_CONTROL 0x40
+
+#define CIF_SW_CPHY_EN(x) ((x) << 0)
+#define CIF_SW_DSI_EN(x) ((x) << 4)
+#define CIF_SW_DATATYPE_FS(x) ((x) << 8)
+#define CIF_SW_DATATYPE_FE(x) ((x) << 14)
+#define CIF_SW_DATATYPE_LS(x) ((x) << 20)
+#define CIF_SW_DATATYPE_LE(x) ((x) << 26)
+
+#endif
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v10 3/3] arm64: dts: rockchip: Add the camera interface
2023-11-08 16:38 [PATCH v10 0/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
2023-11-08 16:38 ` [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF Mehdi Djait
2023-11-08 16:38 ` [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
@ 2023-11-08 16:38 ` Mehdi Djait
2 siblings, 0 replies; 20+ messages in thread
From: Mehdi Djait @ 2023-11-08 16:38 UTC (permalink / raw)
To: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt
Cc: linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski,
michael.riesch, Mehdi Djait
The PX30 has a video capture component, supporting the BT.656
parallel interface. Add a DT description for it.
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
---
arch/arm64/boot/dts/rockchip/px30.dtsi | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/px30.dtsi b/arch/arm64/boot/dts/rockchip/px30.dtsi
index 42ce78beb413..3a4e859e5a49 100644
--- a/arch/arm64/boot/dts/rockchip/px30.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
@@ -1281,6 +1281,18 @@ isp_mmu: iommu@ff4a8000 {
#iommu-cells = <0>;
};
+ cif: video-capture@ff490000 {
+ compatible = "rockchip,px30-vip";
+ reg = <0x0 0xff490000 0x0 0x200>;
+ interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cru ACLK_CIF>, <&cru HCLK_CIF>, <&cru PCLK_CIF>;
+ clock-names = "aclk", "hclk", "pclk";
+ power-domains = <&power PX30_PD_VI>;
+ resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
+ reset-names = "axi", "ahb", "pclkin";
+ status = "disabled";
+ };
+
qos_gmac: qos@ff518000 {
compatible = "rockchip,px30-qos", "syscon";
reg = <0x0 0xff518000 0x0 0x20>;
--
2.41.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-08 16:38 ` [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF Mehdi Djait
@ 2023-11-09 17:24 ` Conor Dooley
2023-11-09 17:45 ` Paul Kocialkowski
0 siblings, 1 reply; 20+ messages in thread
From: Conor Dooley @ 2023-11-09 17:24 UTC (permalink / raw)
To: Mehdi Djait
Cc: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt, linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski,
michael.riesch
[-- Attachment #1: Type: text/plain, Size: 3730 bytes --]
On Wed, Nov 08, 2023 at 05:38:56PM +0100, Mehdi Djait wrote:
> Add a documentation for the Rockchip Camera Interface binding.
>
> the name of the file rk3066 is the first Rockchip SoC generation that uses cif
> instead of the px30 which is just one of the many iterations of the unit.
I think this is becoming ridiculous. You've now removed the compatible
for the rk3066 but kept it in the filename. I don't understand the
hangup about naming the file after the px30-vip, but naming it after
something that is not documented here at all makes no sense to me.
Either document the rk3066 properly, or remove all mention of it IMO.
Cheers,
Conor.
>
> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> ---
> .../bindings/media/rockchip,rk3066-cif.yaml | 94 +++++++++++++++++++
> 1 file changed, 94 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> new file mode 100644
> index 000000000000..c3a5cd2baf71
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/rockchip,rk3066-cif.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip CIF Camera Interface
> +
> +maintainers:
> + - Mehdi Djait <mehdi.djait@bootlin.com>
> +
> +description:
> + CIF is a camera interface present on some rockchip SoCs. It receives the data
> + from Camera sensor or CCIR656 encoder and transfers it into system main memory
> + by AXI bus.
> +
> +properties:
> + compatible:
> + const: rockchip,px30-vip
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: ACLK
> + - description: HCLK
> + - description: PCLK
> +
> + clock-names:
> + items:
> + - const: aclk
> + - const: hclk
> + - const: pclk
> +
> + resets:
> + items:
> + - description: AXI
> + - description: AHB
> + - description: PCLK IN
> +
> + reset-names:
> + items:
> + - const: axi
> + - const: ahb
> + - const: pclkin
> +
> + power-domains:
> + maxItems: 1
> +
> + port:
> + $ref: /schemas/graph.yaml#/properties/port
> + description: A connection to a sensor or decoder
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - port
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/clock/px30-cru.h>
> + #include <dt-bindings/power/px30-power.h>
> +
> + parent {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + video-capture@ff490000 {
> + compatible = "rockchip,px30-vip";
> + reg = <0x0 0xff490000 0x0 0x200>;
> + interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru ACLK_CIF>, <&cru HCLK_CIF>, <&cru PCLK_CIF>;
> + clock-names = "aclk", "hclk", "pclk";
> + resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
> + reset-names = "axi", "ahb", "pclkin";
> + power-domains = <&power PX30_PD_VI>;
> +
> + port {
> + endpoint {
> + remote-endpoint = <&tw9900_out>;
> + };
> + };
> + };
> + };
> +...
> --
> 2.41.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-09 17:24 ` Conor Dooley
@ 2023-11-09 17:45 ` Paul Kocialkowski
2023-11-09 17:53 ` Krzysztof Kozlowski
0 siblings, 1 reply; 20+ messages in thread
From: Paul Kocialkowski @ 2023-11-09 17:45 UTC (permalink / raw)
To: Conor Dooley
Cc: Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt, linux-media,
devicetree, linux-kernel, thomas.petazzoni, alexandre.belloni,
maxime.chevallier, michael.riesch
[-- Attachment #1: Type: text/plain, Size: 5131 bytes --]
Hi,
On Thu 09 Nov 23, 17:24, Conor Dooley wrote:
> On Wed, Nov 08, 2023 at 05:38:56PM +0100, Mehdi Djait wrote:
> > Add a documentation for the Rockchip Camera Interface binding.
> >
> > the name of the file rk3066 is the first Rockchip SoC generation that uses cif
> > instead of the px30 which is just one of the many iterations of the unit.
>
> I think this is becoming ridiculous. You've now removed the compatible
> for the rk3066 but kept it in the filename. I don't understand the
> hangup about naming the file after the px30-vip, but naming it after
> something that is not documented here at all makes no sense to me.
> Either document the rk3066 properly, or remove all mention of it IMO.
I think the opposite is ridiculous. We have spent some time investigating the
history of this unit, to find out that RK3066 is the first occurence where
it exists. Since we want the binding to cover all generations of the same unit
and give it a name that reflects this, rk3066 is the natural choice that comes
to mind. As far as I understand, this is the normal thing to do to name
bindings: name after the earliest known occurence of the unit.
What is the rationale behind naming the file after a generation of the unit
that happens to be the one introducing the binding? This is neither the first
nor the last one to include this unit. The binding will be updated later to
cover other generations. Do we want to rename the file each time an a generation
earlier than px30 is introduced? That sounds quite ridiculous too.
We've done the research work to give it the most relevant name here.
I'd expect some strong arguments not to use it. Can you ellaborate?
Cheers,
Paul
> Cheers,
> Conor.
>
> >
> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > ---
> > .../bindings/media/rockchip,rk3066-cif.yaml | 94 +++++++++++++++++++
> > 1 file changed, 94 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> > new file mode 100644
> > index 000000000000..c3a5cd2baf71
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> > @@ -0,0 +1,94 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/rockchip,rk3066-cif.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip CIF Camera Interface
> > +
> > +maintainers:
> > + - Mehdi Djait <mehdi.djait@bootlin.com>
> > +
> > +description:
> > + CIF is a camera interface present on some rockchip SoCs. It receives the data
> > + from Camera sensor or CCIR656 encoder and transfers it into system main memory
> > + by AXI bus.
> > +
> > +properties:
> > + compatible:
> > + const: rockchip,px30-vip
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: ACLK
> > + - description: HCLK
> > + - description: PCLK
> > +
> > + clock-names:
> > + items:
> > + - const: aclk
> > + - const: hclk
> > + - const: pclk
> > +
> > + resets:
> > + items:
> > + - description: AXI
> > + - description: AHB
> > + - description: PCLK IN
> > +
> > + reset-names:
> > + items:
> > + - const: axi
> > + - const: ahb
> > + - const: pclkin
> > +
> > + power-domains:
> > + maxItems: 1
> > +
> > + port:
> > + $ref: /schemas/graph.yaml#/properties/port
> > + description: A connection to a sensor or decoder
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/clock/px30-cru.h>
> > + #include <dt-bindings/power/px30-power.h>
> > +
> > + parent {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + video-capture@ff490000 {
> > + compatible = "rockchip,px30-vip";
> > + reg = <0x0 0xff490000 0x0 0x200>;
> > + interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cru ACLK_CIF>, <&cru HCLK_CIF>, <&cru PCLK_CIF>;
> > + clock-names = "aclk", "hclk", "pclk";
> > + resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
> > + reset-names = "axi", "ahb", "pclkin";
> > + power-domains = <&power PX30_PD_VI>;
> > +
> > + port {
> > + endpoint {
> > + remote-endpoint = <&tw9900_out>;
> > + };
> > + };
> > + };
> > + };
> > +...
> > --
> > 2.41.0
> >
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-09 17:45 ` Paul Kocialkowski
@ 2023-11-09 17:53 ` Krzysztof Kozlowski
2023-11-09 18:07 ` Paul Kocialkowski
0 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2023-11-09 17:53 UTC (permalink / raw)
To: Paul Kocialkowski, Conor Dooley
Cc: Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt, linux-media,
devicetree, linux-kernel, thomas.petazzoni, alexandre.belloni,
maxime.chevallier, michael.riesch
On 09/11/2023 18:45, Paul Kocialkowski wrote:
> Hi,
>
> On Thu 09 Nov 23, 17:24, Conor Dooley wrote:
>> On Wed, Nov 08, 2023 at 05:38:56PM +0100, Mehdi Djait wrote:
>>> Add a documentation for the Rockchip Camera Interface binding.
>>>
>>> the name of the file rk3066 is the first Rockchip SoC generation that uses cif
>>> instead of the px30 which is just one of the many iterations of the unit.
>>
>> I think this is becoming ridiculous. You've now removed the compatible
>> for the rk3066 but kept it in the filename. I don't understand the
>> hangup about naming the file after the px30-vip, but naming it after
>> something that is not documented here at all makes no sense to me.
>> Either document the rk3066 properly, or remove all mention of it IMO.
>
> I think the opposite is ridiculous. We have spent some time investigating the
> history of this unit, to find out that RK3066 is the first occurence where
> it exists. Since we want the binding to cover all generations of the same unit
> and give it a name that reflects this, rk3066 is the natural choice that comes
> to mind. As far as I understand, this is the normal thing to do to name
> bindings: name after the earliest known occurence of the unit.
>
> What is the rationale behind naming the file after a generation of the unit
> that happens to be the one introducing the binding? This is neither the first
> nor the last one to include this unit. The binding will be updated later to
> cover other generations. Do we want to rename the file each time an a generation
> earlier than px30 is introduced? That sounds quite ridiculous too.
>
> We've done the research work to give it the most relevant name here.
> I'd expect some strong arguments not to use it. Can you ellaborate?
If you do not have rk3066 documented here, it might be added to entirely
different file (for whatever reasons, including that binding would be
quite different than px30). Thus you would have rk3066 in
rockchip,rk3066-cif-added-later.yaml and px30 in rockchip,rk3066-cif.yaml
Just use the filename matching the compatible. That's what we always
ask. In every review.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-09 17:53 ` Krzysztof Kozlowski
@ 2023-11-09 18:07 ` Paul Kocialkowski
2023-11-10 18:23 ` Conor Dooley
0 siblings, 1 reply; 20+ messages in thread
From: Paul Kocialkowski @ 2023-11-09 18:07 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Conor Dooley, Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt, linux-media,
devicetree, linux-kernel, thomas.petazzoni, alexandre.belloni,
maxime.chevallier, michael.riesch
[-- Attachment #1: Type: text/plain, Size: 3585 bytes --]
Hi,
On Thu 09 Nov 23, 18:53, Krzysztof Kozlowski wrote:
> On 09/11/2023 18:45, Paul Kocialkowski wrote:
> > Hi,
> >
> > On Thu 09 Nov 23, 17:24, Conor Dooley wrote:
> >> On Wed, Nov 08, 2023 at 05:38:56PM +0100, Mehdi Djait wrote:
> >>> Add a documentation for the Rockchip Camera Interface binding.
> >>>
> >>> the name of the file rk3066 is the first Rockchip SoC generation that uses cif
> >>> instead of the px30 which is just one of the many iterations of the unit.
> >>
> >> I think this is becoming ridiculous. You've now removed the compatible
> >> for the rk3066 but kept it in the filename. I don't understand the
> >> hangup about naming the file after the px30-vip, but naming it after
> >> something that is not documented here at all makes no sense to me.
> >> Either document the rk3066 properly, or remove all mention of it IMO.
> >
> > I think the opposite is ridiculous. We have spent some time investigating the
> > history of this unit, to find out that RK3066 is the first occurence where
> > it exists. Since we want the binding to cover all generations of the same unit
> > and give it a name that reflects this, rk3066 is the natural choice that comes
> > to mind. As far as I understand, this is the normal thing to do to name
> > bindings: name after the earliest known occurence of the unit.
> >
> > What is the rationale behind naming the file after a generation of the unit
> > that happens to be the one introducing the binding? This is neither the first
> > nor the last one to include this unit. The binding will be updated later to
> > cover other generations. Do we want to rename the file each time an a generation
> > earlier than px30 is introduced? That sounds quite ridiculous too.
> >
> > We've done the research work to give it the most relevant name here.
> > I'd expect some strong arguments not to use it. Can you ellaborate?
>
> If you do not have rk3066 documented here, it might be added to entirely
> different file (for whatever reasons, including that binding would be
> quite different than px30). Thus you would have rk3066 in
> rockchip,rk3066-cif-added-later.yaml and px30 in rockchip,rk3066-cif.yaml
As far as I could see we generally manage to include support for different
hardware setups in the same binding document using conditionals on the
compatible, so this feels a bit far-fetched.
Of course you're the maintainer and have significantly more experience here
so there might be a lot that I'm not seeing, but I'm not very convinced by this
reasoning to be honest.
> Just use the filename matching the compatible. That's what we always
> ask. In every review.
Yeah and we very often end up with naming that is less than optimal (to stay
polite). I'm generally quite appalled by the overall lack of interest that
naming gets, as if it was something secondary. Naming is one of the most
important and difficult things in our field of work and it needs to be
considered with care.
This is not just a problem with device-tree, it's a kernel-wide issue that
nobody seems to be interested in addressing. I'm quite unhappy to see that when
time is spent trying to improve the situation on one particular instance, we are
shown the door because it doesn't match what is generally done (and often done
wrong).
This is definitely a rant. I really want to express this issue loud and clear
and encourage everyone to consider it for what it is.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface
2023-11-08 16:38 ` [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
@ 2023-11-10 12:50 ` Michael Riesch
2023-11-13 11:06 ` Mehdi Djait
2023-11-10 14:33 ` Michael Riesch
1 sibling, 1 reply; 20+ messages in thread
From: Michael Riesch @ 2023-11-10 12:50 UTC (permalink / raw)
To: Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt
Cc: linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski
Hi Mehdi,
Thanks for your patches.
The good news first: with some hacks applied I was able to capture the
video stream from a HDMI receiver chip via BT.1120 on a Rockchip RK3568.
As a next step, I'll clean up the hacky RK3568 support and submit them
for review.
Still, there are some issues that needs to be addressed. Please find my
comments inline.
On 11/8/23 17:38, Mehdi Djait wrote:
> This introduces a V4L2 driver for the Rockchip CIF video capture controller.
>
> This controller supports multiple parallel interfaces, but for now only the
> BT.656 interface could be tested, hence it's the only one that's supported
> in the first version of this driver.
>
> This controller can be found on RK3066, PX30, RK1808, RK3128 and RK3288,
> but for now it's only been tested on the PX30.
>
> CIF is implemented as a video node-centric driver.
>
> Most of this driver was written following the BSP driver from rockchip,
> removing the parts that either didn't fit correctly the guidelines, or that
> couldn't be tested.
>
> This basic version doesn't support cropping nor scaling and is only
> designed with one SDTV video decoder being attached to it at any time.
>
> This version uses the "pingpong" mode of the controller, which is a
> double-buffering mechanism.
>
> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> ---
> MAINTAINERS | 7 +
> drivers/media/platform/rockchip/Kconfig | 1 +
> drivers/media/platform/rockchip/Makefile | 1 +
> drivers/media/platform/rockchip/cif/Kconfig | 13 +
> drivers/media/platform/rockchip/cif/Makefile | 3 +
> drivers/media/platform/rockchip/cif/capture.c | 1152 +++++++++++++++++
> drivers/media/platform/rockchip/cif/dev.c | 289 +++++
> drivers/media/platform/rockchip/cif/dev.h | 139 ++
> drivers/media/platform/rockchip/cif/regs.h | 192 +++
> 9 files changed, 1797 insertions(+)
> create mode 100644 drivers/media/platform/rockchip/cif/Kconfig
> create mode 100644 drivers/media/platform/rockchip/cif/Makefile
> create mode 100644 drivers/media/platform/rockchip/cif/capture.c
> create mode 100644 drivers/media/platform/rockchip/cif/dev.c
> create mode 100644 drivers/media/platform/rockchip/cif/dev.h
> create mode 100644 drivers/media/platform/rockchip/cif/regs.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3b47e0b56859..f47a828d08a5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18523,6 +18523,13 @@ F: Documentation/ABI/*/sysfs-driver-hid-roccat*
> F: drivers/hid/hid-roccat*
> F: include/linux/hid-roccat*
>
> +ROCKCHIP CIF DRIVER
> +M: Mehdi Djait <mehdi.djait@bootlin.com>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> +F: drivers/media/platform/rockchip/cif/
> +
> ROCKCHIP CRYPTO DRIVERS
> M: Corentin Labbe <clabbe@baylibre.com>
> L: linux-crypto@vger.kernel.org
> diff --git a/drivers/media/platform/rockchip/Kconfig b/drivers/media/platform/rockchip/Kconfig
> index b41d3960c1b4..f73d68d1d2b6 100644
> --- a/drivers/media/platform/rockchip/Kconfig
> +++ b/drivers/media/platform/rockchip/Kconfig
> @@ -2,5 +2,6 @@
>
> comment "Rockchip media platform drivers"
>
> +source "drivers/media/platform/rockchip/cif/Kconfig"
> source "drivers/media/platform/rockchip/rga/Kconfig"
> source "drivers/media/platform/rockchip/rkisp1/Kconfig"
> diff --git a/drivers/media/platform/rockchip/Makefile b/drivers/media/platform/rockchip/Makefile
> index 4f782b876ac9..1a7aa1f8e134 100644
> --- a/drivers/media/platform/rockchip/Makefile
> +++ b/drivers/media/platform/rockchip/Makefile
> @@ -1,3 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0-only
> +obj-y += cif/
> obj-y += rga/
> obj-y += rkisp1/
> diff --git a/drivers/media/platform/rockchip/cif/Kconfig b/drivers/media/platform/rockchip/cif/Kconfig
> new file mode 100644
> index 000000000000..68fc50c51f1c
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/Kconfig
> @@ -0,0 +1,13 @@
> +config VIDEO_ROCKCHIP_CIF
> + tristate "Rockchip CIF Video Camera Interface"
> + depends on VIDEO_DEV
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + depends on V4L_PLATFORM_DRIVERS
> + depends on PM && COMMON_CLK
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_FWNODE
> + select VIDEO_V4L2_SUBDEV_API
> + help
> + This is a driver for Rockchip SoC Camera interface. It supports
> + parallel interfaces such as BT.656. This camera interface is both
> + called VIP and CIF.
> diff --git a/drivers/media/platform/rockchip/cif/Makefile b/drivers/media/platform/rockchip/cif/Makefile
> new file mode 100644
> index 000000000000..e44ef687aeb6
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += rockchip-cif.o
> +rockchip-cif-objs += dev.o capture.o
> diff --git a/drivers/media/platform/rockchip/cif/capture.c b/drivers/media/platform/rockchip/cif/capture.c
> new file mode 100644
> index 000000000000..f5f30884ebcc
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/capture.c
> @@ -0,0 +1,1152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Camera Interface Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mc.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +#include "dev.h"
> +#include "regs.h"
> +
> +#define CIF_REQ_BUFS_MIN 2
> +#define CIF_MIN_WIDTH 64
> +#define CIF_MIN_HEIGHT 64
> +#define CIF_MAX_WIDTH 8192
> +#define CIF_MAX_HEIGHT 8192
> +
> +#define CIF_PLANE_Y 0
> +#define CIF_PLANE_UV 1
> +
> +static struct cif_output_fmt out_fmts[] = {
> + {
> + .fourcc = V4L2_PIX_FMT_NV16,
> + .fmt_val = CIF_FORMAT_YUV_OUTPUT_422 |
> + CIF_FORMAT_UV_STORAGE_ORDER_UVUV,
> + .cplanes = 2,
> + }, {
> + .fourcc = V4L2_PIX_FMT_NV61,
> + .fmt_val = CIF_FORMAT_YUV_OUTPUT_422 |
> + CIF_FORMAT_UV_STORAGE_ORDER_VUVU,
> + .cplanes = 2,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV12,
> + .fmt_val = CIF_FORMAT_YUV_OUTPUT_420 |
> + CIF_FORMAT_UV_STORAGE_ORDER_UVUV,
> + .cplanes = 2,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV21,
> + .fmt_val = CIF_FORMAT_YUV_OUTPUT_420 |
> + CIF_FORMAT_UV_STORAGE_ORDER_VUVU,
> + .cplanes = 2,
> + }, {
> + .fourcc = V4L2_PIX_FMT_RGB24,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_RGB565,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_BGR666,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB8,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG8,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG8,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR8,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB10,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG10,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG10,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR10,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB12,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG12,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG12,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR12,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR16,
> + .cplanes = 1,
> + }, {
> + .fourcc = V4L2_PIX_FMT_Y16,
> + .cplanes = 1,
> + }
> +};
> +
> +static const struct cif_input_fmt in_fmts[] = {
> + {
> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> + CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
What is the point of this csi_fmt_val field? If the strategy is to kick
everything out that is not explicitly required then this should be
removed (and added at a later stage, if needed).
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> + CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> + CIF_FORMAT_YUV_INPUT_ORDER_YVYU,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
What is the difference between this entry (in_fmts[2]) and in_fmts[0]?
Similarly, between in_fmts[1] and in_fmts[3]?
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> + CIF_FORMAT_YUV_INPUT_ORDER_YVYU,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> + CIF_FORMAT_YUV_INPUT_ORDER_UYVY,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> + CIF_FORMAT_YUV_INPUT_ORDER_UYVY,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> + CIF_FORMAT_YUV_INPUT_ORDER_VYUY,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> + CIF_FORMAT_YUV_INPUT_ORDER_VYUY,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_RGB888_1X24,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RGB888,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y8_1X8,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y10_1X10,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y12_1X12,
> + .dvp_fmt_val = CIF_FORMAT_INPUT_MODE_RAW |
> + CIF_FORMAT_RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }
> +};
> +
> +static const struct
> +cif_input_fmt *get_input_fmt(struct v4l2_subdev *sd)
> +{
> + struct v4l2_subdev_format fmt;
> + u32 i;
> +
> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + fmt.pad = 0;
> + v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> +
> + for (i = 0; i < ARRAY_SIZE(in_fmts); i++)
> + if (fmt.format.code == in_fmts[i].mbus_code &&
> + fmt.format.field == in_fmts[i].field)
> + return &in_fmts[i];
> +
> + v4l2_err(sd->v4l2_dev, "remote's mbus code not supported\n");
> + return NULL;
> +}
> +
> +static struct
> +cif_output_fmt *find_output_fmt(struct cif_stream *stream, u32 pixelfmt)
> +{
> + struct cif_output_fmt *fmt;
> + u32 i;
> +
> + for (i = 0; i < ARRAY_SIZE(out_fmts); i++) {
> + fmt = &out_fmts[i];
> + if (fmt->fourcc == pixelfmt)
> + return fmt;
> + }
> +
> + return NULL;
> +}
> +
> +static struct cif_buffer *cif_get_buffer(struct cif_stream *stream)
> +{
> + struct cif_buffer *buff;
> +
> + lockdep_assert_held(&stream->vbq_lock);
> +
> + if (list_empty(&stream->buf_head))
> + return NULL;
> +
> + buff = list_first_entry(&stream->buf_head, struct cif_buffer, queue);
> + list_del(&buff->queue);
> +
> + return buff;
> +}
> +
> +static int cif_init_buffers(struct cif_stream *stream)
> +{
> + struct cif_device *cif_dev = stream->cifdev;
> + unsigned long lock_flags;
> +
> + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> +
> + stream->buffs[0] = cif_get_buffer(stream);
> + stream->buffs[1] = cif_get_buffer(stream);
> +
> + if (!(stream->buffs[0]) || !(stream->buffs[1])) {
> + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> + return -EINVAL;
> + }
> +
> + stream->drop_frame = false;
> +
> + cif_write(cif_dev, CIF_FRM0_ADDR_Y,
> + stream->buffs[0]->buff_addr[CIF_PLANE_Y]);
> + cif_write(cif_dev, CIF_FRM0_ADDR_UV,
> + stream->buffs[0]->buff_addr[CIF_PLANE_UV]);
> +
> + cif_write(cif_dev, CIF_FRM1_ADDR_Y,
> + stream->buffs[1]->buff_addr[CIF_PLANE_Y]);
> + cif_write(cif_dev, CIF_FRM1_ADDR_UV,
> + stream->buffs[1]->buff_addr[CIF_PLANE_UV]);
> +
> + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> +
> + return 0;
> +}
> +
> +static void cif_assign_new_buffer_pingpong(struct cif_stream *stream)
> +{
> + struct cif_device *cif_dev = stream->cifdev;
> + struct cif_buffer *buffer = NULL;
> + u32 frm_addr_y, frm_addr_uv;
> + unsigned long lock_flags;
> +
> + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> +
> + buffer = cif_get_buffer(stream);
> +
> + /*
> + * In Pingpong mode:
> + * After one frame0 captured, CIF will start to capture the next frame1
> + * automatically.
> + *
> + * If there is no buffer:
> + * 1. Make the next frame0 write to the buffer of frame1.
> + *
> + * 2. Drop the frame1: Don't return it to user-space, as it will be
> + * overwritten by the next frame0.
> + */
> + if (!buffer) {
> + stream->drop_frame = true;
> + buffer = stream->buffs[1 - stream->frame_phase];
> + } else {
> + stream->drop_frame = false;
> + }
> +
> + stream->buffs[stream->frame_phase] = buffer;
> + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> +
> + frm_addr_y = stream->frame_phase ? CIF_FRM1_ADDR_Y : CIF_FRM0_ADDR_Y;
> + frm_addr_uv = stream->frame_phase ? CIF_FRM1_ADDR_UV : CIF_FRM0_ADDR_UV;
> +
> + cif_write(cif_dev, frm_addr_y, buffer->buff_addr[CIF_PLANE_Y]);
> + cif_write(cif_dev, frm_addr_uv, buffer->buff_addr[CIF_PLANE_UV]);
> +}
> +
> +static void cif_stream_stop(struct cif_stream *stream)
> +{
> + struct cif_device *cif_dev = stream->cifdev;
> + u32 val;
> +
> + val = cif_read(cif_dev, CIF_CTRL);
> + cif_write(cif_dev, CIF_CTRL, val & (~CIF_CTRL_ENABLE_CAPTURE));
> + cif_write(cif_dev, CIF_INTEN, 0x0);
> + cif_write(cif_dev, CIF_INTSTAT, 0x3ff);
> + cif_write(cif_dev, CIF_FRAME_STATUS, 0x0);
> +
> + stream->stopping = false;
> +}
> +
> +static int cif_queue_setup(struct vb2_queue *queue,
> + unsigned int *num_buffers,
> + unsigned int *num_planes,
> + unsigned int sizes[],
> + struct device *alloc_devs[])
> +{
> + struct cif_stream *stream = queue->drv_priv;
> + const struct v4l2_pix_format *pix;
> +
> + pix = &stream->pix;
> +
> + if (*num_planes) {
> + if (*num_planes != 1)
> + return -EINVAL;
> +
> + if (sizes[0] < pix->sizeimage)
> + return -EINVAL;
> + return 0;
> + }
> +
> + *num_planes = 1;
> +
> + sizes[0] = pix->sizeimage;
> +
> + *num_buffers = CIF_REQ_BUFS_MIN;
> +
> + return 0;
> +}
> +
> +static void cif_buf_queue(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct cif_buffer *cifbuf = to_cif_buffer(vbuf);
> + struct vb2_queue *queue = vb->vb2_queue;
> + struct cif_stream *stream = queue->drv_priv;
> + struct v4l2_pix_format *pix = &stream->pix;
> + unsigned long lock_flags;
> + int i;
> +
> + struct cif_output_fmt *fmt = stream->cif_fmt_out;
> +
> + memset(cifbuf->buff_addr, 0, sizeof(cifbuf->buff_addr));
> +
> + cifbuf->buff_addr[0] = vb2_dma_contig_plane_dma_addr(vb, 0);
> +
> + for (i = 0; i < fmt->cplanes - 1; i++)
> + cifbuf->buff_addr[i + 1] = cifbuf->buff_addr[i] +
> + pix->bytesperline * pix->height;
> +
> + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> + list_add_tail(&cifbuf->queue, &stream->buf_head);
> + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> +}
> +
> +static void cif_return_all_buffers(struct cif_stream *stream,
> + enum vb2_buffer_state state)
> +{
> + struct cif_buffer *buf;
> + unsigned long lock_flags;
> +
> + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> +
> + if (stream->buffs[0]) {
> + vb2_buffer_done(&stream->buffs[0]->vb.vb2_buf, state);
> + stream->buffs[0] = NULL;
> + }
> +
> + if (stream->buffs[1]) {
> + if (!stream->drop_frame)
> + vb2_buffer_done(&stream->buffs[1]->vb.vb2_buf, state);
> +
> + stream->buffs[1] = NULL;
> + }
> +
> + while (!list_empty(&stream->buf_head)) {
> + buf = cif_get_buffer(stream);
> + vb2_buffer_done(&buf->vb.vb2_buf, state);
> + }
> +
> + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> +}
> +
> +static void cif_stop_streaming(struct vb2_queue *queue)
> +{
> + struct cif_stream *stream = queue->drv_priv;
> + struct cif_device *cif_dev = stream->cifdev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + stream->stopping = true;
> + ret = wait_event_timeout(stream->wq_stopped,
> + !stream->stopping,
> + msecs_to_jiffies(1000));
> + if (!ret) {
> + cif_stream_stop(stream);
> + stream->stopping = false;
> + }
> +
> + /* Stop the sub device. */
> + sd = cif_dev->remote.sd;
> + v4l2_subdev_call(sd, video, s_stream, 0);
> +
> + pm_runtime_put(cif_dev->dev);
> +
> + cif_return_all_buffers(stream, VB2_BUF_STATE_ERROR);
> +}
> +
> +static int cif_stream_start(struct cif_stream *stream)
> +{
> + u32 val, mbus_flags, href_pol, vsync_pol, fmt_type,
> + xfer_mode = 0, yc_swap = 0;
> + struct cif_device *cif_dev = stream->cifdev;
> + struct cif_remote *remote_info;
> + int ret;
> + u32 input_mode;
> +
> + remote_info = &cif_dev->remote;
> + fmt_type = stream->cif_fmt_in->fmt_type;
> + stream->frame_idx = 0;
Those lines are somewhat mixed. The reset of the frame_idx should be
made more visible. The remote_info line could be integrated into the
declaration. For the fmt_type line please see the comment below.
> + input_mode = (remote_info->std == V4L2_STD_NTSC) ?
> + CIF_FORMAT_INPUT_MODE_NTSC :
> + CIF_FORMAT_INPUT_MODE_PAL;
This seems to be an oversimplification. How can one use BT.656 here?
(Aren't you using BT.656 as mbus format between your video decoder and
the PX30 VIP?)
You should not assume that the remote is capable of any TV standards
(this statement holds for the driver in general).
> + mbus_flags = remote_info->mbus.bus.parallel.flags;
> + href_pol = (mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) ?
> + 0 : CIF_FORMAT_HSY_LOW_ACTIVE;
> + vsync_pol = (mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) ?
> + CIF_FORMAT_VSY_HIGH_ACTIVE : 0;
> +
> + val = vsync_pol | href_pol | input_mode | stream->cif_fmt_out->fmt_val |
> + stream->cif_fmt_in->dvp_fmt_val | xfer_mode | yc_swap;
May be a matter of taste, but why don't we use ONE u32 variable for the
register content and OR everything into it? yc_swap is not set anyway, BTW.
> + cif_write(cif_dev, CIF_FOR, val);
> +
> + val = stream->pix.width;
> + if (stream->cif_fmt_in->fmt_type == CIF_FMT_TYPE_RAW)
> + val = stream->pix.width * 2;
> +
> + cif_write(cif_dev, CIF_VIR_LINE_WIDTH, val);
> + cif_write(cif_dev, CIF_SET_SIZE,
> + stream->pix.width | (stream->pix.height << 16));
> +
> + cif_write(cif_dev, CIF_FRAME_STATUS, CIF_FRAME_STAT_CLS);
> + cif_write(cif_dev, CIF_INTSTAT, CIF_INTSTAT_CLS);
> + cif_write(cif_dev, CIF_SCL_CTRL, (fmt_type == CIF_FMT_TYPE_YUV) ?
> + CIF_SCL_CTRL_ENABLE_YUV_16BIT_BYPASS :
> + CIF_SCL_CTRL_ENABLE_RAW_16BIT_BYPASS);
> +
> + ret = cif_init_buffers(stream);
> + if (ret)
> + return ret;
> +
> + cif_write(cif_dev, CIF_INTEN, CIF_INTEN_FRAME_END_EN |
> + CIF_INTEN_LINE_ERR_EN |
> + CIF_INTEN_PST_INF_FRAME_END_EN);
> +
> + cif_write(cif_dev, CIF_CTRL, CIF_CTRL_AXI_BURST_16 |
> + CIF_CTRL_MODE_PINGPONG |
> + CIF_CTRL_ENABLE_CAPTURE);
> +
> + return 0;
> +}
> +
> +static int cif_start_streaming(struct vb2_queue *queue, unsigned int count)
> +{
> + struct cif_stream *stream = queue->drv_priv;
> + struct cif_device *cif_dev = stream->cifdev;
> + struct v4l2_device *v4l2_dev = &cif_dev->v4l2_dev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + if (!cif_dev->remote.sd) {
> + ret = -ENODEV;
> + v4l2_err(v4l2_dev, "No remote subdev detected\n");
> + goto destroy_buf;
> + }
> +
> + ret = pm_runtime_get_sync(cif_dev->dev);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev, "Failed to get runtime pm, %d\n", ret);
> + goto destroy_buf;
> + }
> +
> + sd = cif_dev->remote.sd;
> + stream->cif_fmt_in = get_input_fmt(cif_dev->remote.sd);
Unknown format (stream->cif_fmt_in == NULL) must be handled appropriately.
> +
> + ret = cif_stream_start(stream);
> + if (ret < 0)
> + goto stop_stream;
> +
> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> + if (ret < 0)
> + goto runtime_put;
> +
> + return 0;
> +
> +runtime_put:
> + pm_runtime_put(cif_dev->dev);
> +stop_stream:
> + cif_stream_stop(stream);
> +destroy_buf:
> + cif_return_all_buffers(stream, VB2_BUF_STATE_QUEUED);
> +
> + return ret;
> +}
> +
> +static const struct vb2_ops cif_vb2_ops = {
> + .queue_setup = cif_queue_setup,
> + .buf_queue = cif_buf_queue,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .stop_streaming = cif_stop_streaming,
> + .start_streaming = cif_start_streaming,
> +};
> +
> +static int cif_init_vb2_queue(struct vb2_queue *q,
> + struct cif_stream *stream)
> +{
> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + q->io_modes = VB2_MMAP | VB2_DMABUF;
> + q->drv_priv = stream;
> + q->ops = &cif_vb2_ops;
> + q->mem_ops = &vb2_dma_contig_memops;
> + q->buf_struct_size = sizeof(struct cif_buffer);
> + q->min_buffers_needed = CIF_REQ_BUFS_MIN;
> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + q->lock = &stream->vlock;
> + q->dev = stream->cifdev->dev;
> +
> + return vb2_queue_init(q);
> +}
> +
> +static void cif_update_pix(struct cif_stream *stream,
> + struct cif_output_fmt *fmt,
> + struct v4l2_pix_format *pix)
> +{
> + struct cif_remote *remote_info = &stream->cifdev->remote;
> + struct v4l2_subdev_format sd_fmt;
> + u32 width, height;
> +
> + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + sd_fmt.pad = 0;
> + v4l2_subdev_call(remote_info->sd, pad, get_fmt, NULL, &sd_fmt);
> +
> + width = clamp_t(u32, sd_fmt.format.width,
> + CIF_MIN_WIDTH, CIF_MAX_WIDTH);
> + height = clamp_t(u32, sd_fmt.format.height,
> + CIF_MIN_HEIGHT, CIF_MAX_HEIGHT);
> +
> + pix->width = width;
> + pix->height = height;
> + pix->field = sd_fmt.format.field;
> + pix->colorspace = sd_fmt.format.colorspace;
> + pix->ycbcr_enc = sd_fmt.format.ycbcr_enc;
> + pix->quantization = sd_fmt.format.quantization;
> + pix->xfer_func = sd_fmt.format.xfer_func;
> +
> + v4l2_fill_pixfmt(pix, fmt->fourcc, pix->width, pix->height);
> +}
> +
> +static int cif_set_fmt(struct cif_stream *stream,
> + struct v4l2_pix_format *pix)
> +{
> + struct cif_device *cif_dev = stream->cifdev;
> + struct v4l2_subdev_format sd_fmt;
> + struct cif_output_fmt *fmt;
> + int ret;
> +
> + if (vb2_is_streaming(&stream->buf_queue))
> + return -EBUSY;
> +
> + fmt = find_output_fmt(stream, pix->pixelformat);
> + if (!fmt)
> + fmt = &out_fmts[0];
> +
> + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + sd_fmt.pad = 0;
> + sd_fmt.format.width = pix->width;
> + sd_fmt.format.height = pix->height;
> +
> + ret = v4l2_subdev_call(cif_dev->remote.sd, pad, set_fmt, NULL, &sd_fmt);
> + if (ret)
> + return ret;
> +
> + cif_update_pix(stream, fmt, pix);
> + stream->pix = *pix;
> + stream->cif_fmt_out = fmt;
> +
> + return 0;
> +}
> +
> +void cif_set_default_format(struct cif_device *cif_dev)
> +{
> + struct cif_stream *stream = &cif_dev->stream;
> + struct v4l2_pix_format pix;
> +
> + cif_dev->remote.std = V4L2_STD_NTSC;
Not every subdevice supports TV standards. Is this really reasonable?
> +
> + pix.pixelformat = V4L2_PIX_FMT_NV12;
> + pix.width = CIF_DEFAULT_WIDTH;
> + pix.height = CIF_DEFAULT_HEIGHT;
> +
> + cif_set_fmt(stream, &pix);
> +}
> +
> +void cif_stream_init(struct cif_device *cif_dev)
> +{
> + struct cif_stream *stream = &cif_dev->stream;
> + struct v4l2_pix_format pix;
> +
> + memset(stream, 0, sizeof(*stream));
> + memset(&pix, 0, sizeof(pix));
> + stream->cifdev = cif_dev;
> +
> + INIT_LIST_HEAD(&stream->buf_head);
> + spin_lock_init(&stream->vbq_lock);
> + init_waitqueue_head(&stream->wq_stopped);
> +}
> +
> +static const struct v4l2_file_operations cif_fops = {
> + .open = v4l2_fh_open,
> + .release = vb2_fop_release,
> + .unlocked_ioctl = video_ioctl2,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +};
> +
> +static int cif_enum_input(struct file *file, void *priv,
> + struct v4l2_input *input)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct v4l2_subdev *sd = stream->cifdev->remote.sd;
> + int ret;
> +
> + if (input->index > 0)
> + return -EINVAL;
> +
> + ret = v4l2_subdev_call(sd, video, g_input_status, &input->status);
> + if (ret)
> + return ret;
> +
> + strscpy(input->name, "Camera", sizeof(input->name));
> + input->type = V4L2_INPUT_TYPE_CAMERA;
Wait, we are a camera in any case? How does this fit together with your
video decoder setup?
> + input->std = stream->vdev.tvnorms;
> + input->capabilities = V4L2_IN_CAP_STD;
Not every subdevice supports TV standards. Is this really reasonable?
> +
> + return 0;
> +}
> +
> +static int cif_try_fmt_vid_cap(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct cif_output_fmt *fmt;
> +
> + fmt = find_output_fmt(stream, f->fmt.pix.pixelformat);
> + if (!fmt)
> + fmt = &out_fmts[0];
> +
> + cif_update_pix(stream, fmt, &f->fmt.pix);
> +
> + return 0;
> +}
> +
> +static int cif_g_std(struct file *file, void *fh, v4l2_std_id *norm)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct cif_remote *remote_info = &stream->cifdev->remote;
> +
> + *norm = remote_info->std;
> +
> + return 0;
> +}
> +
> +static int cif_s_std(struct file *file, void *fh, v4l2_std_id norm)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct cif_remote *remote_info = &stream->cifdev->remote;
> + int ret;
> +
> + if (norm == remote_info->std)
> + return 0;
> +
> + if (vb2_is_busy(&stream->buf_queue))
> + return -EBUSY;
> +
> + ret = v4l2_subdev_call(remote_info->sd, video, s_std, norm);
> + if (ret)
> + return ret;
> +
> + remote_info->std = norm;
> +
> + /* S_STD will update the format since that depends on the standard. */
> + cif_update_pix(stream, stream->cif_fmt_out, &stream->pix);
> +
> + return 0;
> +}
> +
> +static int cif_querystd(struct file *file, void *fh, v4l2_std_id *a)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct cif_remote *remote_info = &stream->cifdev->remote;
> +
> + *a = V4L2_STD_UNKNOWN;
> +
> + return v4l2_subdev_call(remote_info->sd, video, querystd, a);
> +}
> +
> +static int cif_enum_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_fmtdesc *f)
> +{
> + struct cif_output_fmt *fmt = NULL;
> +
> + if (f->index >= ARRAY_SIZE(out_fmts))
> + return -EINVAL;
> +
> + fmt = &out_fmts[f->index];
> + f->pixelformat = fmt->fourcc;
> +
> + return 0;
> +}
> +
> +static int cif_s_fmt_vid_cap(struct file *file,
> + void *priv, struct v4l2_format *f)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + int ret;
> +
> + if (vb2_is_busy(&stream->buf_queue))
> + return -EBUSY;
> +
> + ret = cif_set_fmt(stream, &f->fmt.pix);
> +
> + return ret;
> +}
> +
> +static int cif_g_fmt_vid_cap(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> +
> + f->fmt.pix = stream->pix;
> +
> + return 0;
> +}
> +
> +static int cif_querycap(struct file *file, void *priv,
> + struct v4l2_capability *cap)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct device *dev = stream->cifdev->dev;
> +
> + strscpy(cap->driver, dev->driver->name, sizeof(cap->driver));
> + strscpy(cap->card, dev->driver->name, sizeof(cap->card));
> + snprintf(cap->bus_info, sizeof(cap->bus_info),
> + "platform:%s", dev_name(dev));
> +
> + return 0;
> +}
> +
> +static int cif_enum_framesizes(struct file *file, void *fh,
> + struct v4l2_frmsizeenum *fsize)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct cif_device *cif_dev = stream->cifdev;
> + struct v4l2_subdev_frame_size_enum fse = {
> + .index = fsize->index,
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> + struct cif_output_fmt *fmt;
> + int ret;
> +
> + if (!cif_dev->remote.sd)
> + return -ENODEV;
> +
> + fmt = find_output_fmt(stream, fsize->pixel_format);
> + if (!fmt)
> + return -EINVAL;
> +
> + fse.code = fmt->mbus;
> +
> + ret = v4l2_subdev_call(cif_dev->remote.sd, pad, enum_frame_size,
> + NULL, &fse);
> + if (ret)
> + return ret;
> +
> + fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> + fsize->discrete.width = fse.max_width;
> + fsize->discrete.height = fse.max_height;
> +
> + return 0;
> +}
> +
> +static int cif_enum_frameintervals(struct file *file, void *fh,
> + struct v4l2_frmivalenum *fival)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct cif_device *cif_dev = stream->cifdev;
> + struct v4l2_subdev_frame_interval_enum fie = {
> + .index = fival->index,
> + .width = fival->width,
> + .height = fival->height,
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> + struct cif_output_fmt *fmt;
> + int ret;
> +
> + if (!cif_dev->remote.sd)
> + return -ENODEV;
> +
> + fmt = find_output_fmt(stream, fival->pixel_format);
> + if (!fmt)
> + return -EINVAL;
> +
> + fie.code = fmt->mbus;
> +
> + ret = v4l2_subdev_call(cif_dev->remote.sd, pad, enum_frame_interval,
> + NULL, &fie);
> + if (ret)
> + return ret;
> +
> + fival->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> + fival->discrete = fie.interval;
> +
> + return 0;
> +}
> +
> +static int cif_g_input(struct file *file, void *fh, unsigned int *i)
> +{
> + *i = 0;
> + return 0;
> +}
> +
> +static int cif_s_input(struct file *file, void *fh, unsigned int i)
> +{
> + if (i)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int cif_g_parm(struct file *file, void *priv, struct v4l2_streamparm *p)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct cif_device *cif_dev = stream->cifdev;
> +
> + if (!cif_dev->remote.sd)
> + return -ENODEV;
> +
> + return v4l2_g_parm_cap(video_devdata(file), cif_dev->remote.sd, p);
> +}
> +
> +static int cif_s_parm(struct file *file, void *priv, struct v4l2_streamparm *p)
> +{
> + struct cif_stream *stream = video_drvdata(file);
> + struct cif_device *cif_dev = stream->cifdev;
> +
> + if (!cif_dev->remote.sd)
> + return -ENODEV;
> +
> + return v4l2_s_parm_cap(video_devdata(file), cif_dev->remote.sd, p);
> +}
> +
> +static const struct v4l2_ioctl_ops cif_v4l2_ioctl_ops = {
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_querybuf = vb2_ioctl_querybuf,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> + .vidioc_qbuf = vb2_ioctl_qbuf,
> + .vidioc_expbuf = vb2_ioctl_expbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> +
> + .vidioc_g_std = cif_g_std,
> + .vidioc_s_std = cif_s_std,
> + .vidioc_querystd = cif_querystd,
> +
> + .vidioc_enum_fmt_vid_cap = cif_enum_fmt_vid_cap,
> + .vidioc_try_fmt_vid_cap = cif_try_fmt_vid_cap,
> + .vidioc_s_fmt_vid_cap = cif_s_fmt_vid_cap,
> + .vidioc_g_fmt_vid_cap = cif_g_fmt_vid_cap,
> + .vidioc_querycap = cif_querycap,
> + .vidioc_enum_framesizes = cif_enum_framesizes,
> + .vidioc_enum_frameintervals = cif_enum_frameintervals,
> +
> + .vidioc_enum_input = cif_enum_input,
> + .vidioc_g_input = cif_g_input,
> + .vidioc_s_input = cif_s_input,
> +
> + .vidioc_g_parm = cif_g_parm,
> + .vidioc_s_parm = cif_s_parm,
> +};
> +
> +void cif_unregister_stream_vdev(struct cif_device *cif_dev)
> +{
> + struct cif_stream *stream = &cif_dev->stream;
> +
> + media_entity_cleanup(&stream->vdev.entity);
> + video_unregister_device(&stream->vdev);
> +}
> +
> +int cif_register_stream_vdev(struct cif_device *cif_dev)
> +{
> + struct cif_stream *stream = &cif_dev->stream;
> + struct v4l2_device *v4l2_dev = &cif_dev->v4l2_dev;
> + struct video_device *vdev = &stream->vdev;
> + int ret;
> +
> + strscpy(vdev->name, CIF_DRIVER_NAME, sizeof(vdev->name));
> + mutex_init(&stream->vlock);
> +
> + vdev->ioctl_ops = &cif_v4l2_ioctl_ops;
> + vdev->release = video_device_release_empty;
> + vdev->fops = &cif_fops;
> + vdev->minor = -1;
> + vdev->v4l2_dev = v4l2_dev;
> + vdev->lock = &stream->vlock;
> + vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE |
> + V4L2_CAP_STREAMING;
> + vdev->tvnorms = V4L2_STD_NTSC | V4L2_STD_PAL;
> + video_set_drvdata(vdev, stream);
> + vdev->vfl_dir = VFL_DIR_RX;
> + stream->pad.flags = MEDIA_PAD_FL_SINK;
> +
> + cif_init_vb2_queue(&stream->buf_queue, stream);
> +
> + vdev->queue = &stream->buf_queue;
> + strscpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> +
> + ret = media_entity_pads_init(&vdev->entity, 1, &stream->pad);
> + if (ret < 0)
> + return ret;
> +
> + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> + if (ret < 0)
> + v4l2_err(v4l2_dev,
> + "video_register_device failed with error %d\n", ret);
> +
> + return ret;
> +}
> +
> +static void cif_vb_done(struct cif_stream *stream,
> + struct vb2_v4l2_buffer *vb_done)
> +{
> + vb2_set_plane_payload(&vb_done->vb2_buf, 0,
> + stream->pix.sizeimage);
> + vb_done->vb2_buf.timestamp = ktime_get_ns();
> + vb_done->sequence = stream->frame_idx;
> + vb2_buffer_done(&vb_done->vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
> +static void cif_reset_stream(struct cif_device *cif_dev)
> +{
> + u32 ctl = cif_read(cif_dev, CIF_CTRL);
> +
> + cif_write(cif_dev, CIF_CTRL, ctl & (~CIF_CTRL_ENABLE_CAPTURE));
> + cif_write(cif_dev, CIF_CTRL, ctl | CIF_CTRL_ENABLE_CAPTURE);
> +}
> +
> +irqreturn_t cif_irq_pingpong(int irq, void *ctx)
> +{
> + struct device *dev = ctx;
> + struct cif_device *cif_dev = dev_get_drvdata(dev);
> + struct cif_stream *stream = &cif_dev->stream;
> + unsigned int intstat;
> + u32 lastline, lastpix, ctl, cif_frmst;
> +
> + intstat = cif_read(cif_dev, CIF_INTSTAT);
> + cif_frmst = cif_read(cif_dev, CIF_FRAME_STATUS);
> + lastline = CIF_FETCH_Y_LAST_LINE(cif_read(cif_dev, CIF_LAST_LINE));
> + lastpix = CIF_FETCH_Y_LAST_LINE(cif_read(cif_dev, CIF_LAST_PIX));
> + ctl = cif_read(cif_dev, CIF_CTRL);
> +
> + /*
> + * There are two irqs enabled:
> + * - PST_INF_FRAME_END: cif FIFO is ready,
> + * this is prior to FRAME_END
> + * - FRAME_END: cif has saved frame to memory,
> + * a frame ready
> + */
> +
> + if (intstat & CIF_INTSTAT_PST_INF_FRAME_END) {
> + cif_write(cif_dev, CIF_INTSTAT,
> + CIF_INTSTAT_PST_INF_FRAME_END_CLR);
> +
> + if (stream->stopping)
> + /* To stop CIF ASAP, before FRAME_END irq. */
> + cif_write(cif_dev, CIF_CTRL,
> + ctl & (~CIF_CTRL_ENABLE_CAPTURE));
> + }
> +
> + if (intstat & CIF_INTSTAT_PRE_INF_FRAME_END)
> + cif_write(cif_dev, CIF_INTSTAT, CIF_INTSTAT_PRE_INF_FRAME_END);
> +
> + if (intstat & (CIF_INTSTAT_LINE_ERR | CIF_INTSTAT_PIX_ERR)) {
> + v4l2_err(&cif_dev->v4l2_dev,
> + "LINE_ERR OR PIX_ERR detected, stream will be reset");
> + cif_write(cif_dev, CIF_INTSTAT, CIF_INTSTAT_LINE_ERR |
> + CIF_INTSTAT_PIX_ERR);
> + cif_reset_stream(cif_dev);
> + }
> +
> + if (intstat & CIF_INTSTAT_FRAME_END) {
> + struct vb2_v4l2_buffer *vb_done = NULL;
> +
> + cif_write(cif_dev, CIF_INTSTAT, CIF_INTSTAT_FRAME_END_CLR |
> + CIF_INTSTAT_LINE_END_CLR);
> +
> + if (stream->stopping) {
> + cif_stream_stop(stream);
> + wake_up(&stream->wq_stopped);
> + return IRQ_HANDLED;
> + }
> +
> + if (lastline != stream->pix.height) {
> + v4l2_err(&cif_dev->v4l2_dev,
> + "Bad frame, irq:%#x frmst:%#x size:%dx%d\n",
> + intstat, cif_frmst, lastpix, lastline);
> +
> + cif_reset_stream(cif_dev);
> + }
> +
> + if (cif_frmst & CIF_INTSTAT_F0_READY)
> + stream->frame_phase = 0;
> + else if (cif_frmst & CIF_INTSTAT_F1_READY)
> + stream->frame_phase = 1;
> + else
> + return IRQ_HANDLED;
> +
> + vb_done = &stream->buffs[stream->frame_phase]->vb;
> + if (!stream->drop_frame) {
> + cif_vb_done(stream, vb_done);
> + stream->frame_idx++;
> + }
> +
> + cif_assign_new_buffer_pingpong(stream);
> + }
> +
> + return IRQ_HANDLED;
> +}
> diff --git a/drivers/media/platform/rockchip/cif/dev.c b/drivers/media/platform/rockchip/cif/dev.c
> new file mode 100644
> index 000000000000..f7d061a13577
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/dev.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Camera Interface Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
> + */
> +
> +#include "linux/platform_device.h"
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/reset.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "dev.h"
> +#include "regs.h"
> +
> +static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct cif_device *cif_dev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + cif_dev = container_of(notifier, struct cif_device, notifier);
> + sd = cif_dev->remote.sd;
> +
> + mutex_lock(&cif_dev->media_dev.graph_mutex);
> +
> + ret = v4l2_device_register_subdev_nodes(&cif_dev->v4l2_dev);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = media_create_pad_link(&sd->entity, 0,
> + &cif_dev->stream.vdev.entity, 0,
> + MEDIA_LNK_FL_ENABLED);
> + if (ret)
> + dev_err(cif_dev->dev, "failed to create link");
> +
> +unlock:
> + mutex_unlock(&cif_dev->media_dev.graph_mutex);
> + return ret;
> +}
> +
> +static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_connection *asd)
> +{
> + struct cif_device *cif_dev = container_of(notifier,
> + struct cif_device, notifier);
> + int pad;
> +
> + cif_dev->remote.sd = subdev;
> + pad = media_entity_get_fwnode_pad(&subdev->entity, subdev->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (pad < 0)
> + return pad;
> +
> + cif_dev->remote.pad = pad;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
> + .bound = subdev_notifier_bound,
> + .complete = subdev_notifier_complete,
> +};
> +
> +static int cif_subdev_notifier(struct cif_device *cif_dev)
> +{
> + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
> + struct device *dev = cif_dev->dev;
> + struct v4l2_async_connection *asd;
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_PARALLEL,
> + };
> + struct fwnode_handle *ep;
> + int ret;
> +
> + v4l2_async_nf_init(ntf, &cif_dev->v4l2_dev);
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
> + if (!ep)
> + return -EINVAL;
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + if (ret)
> + return ret;
> +
> + asd = v4l2_async_nf_add_fwnode_remote(ntf, ep,
> + struct v4l2_async_connection);
> + if (IS_ERR(asd))
> + return PTR_ERR(asd);
> +
> + ntf->ops = &subdev_notifier_ops;
> +
> + fwnode_handle_put(ep);
> +
> + return v4l2_async_nf_register(ntf);
> +}
> +
> +static struct clk_bulk_data px30_cif_clks[] = {
> + { .id = "aclk", },
> + { .id = "hclk", },
> + { .id = "pclk", },
> +};
> +
> +static const struct cif_match_data px30_cif_match_data = {
> + .clks = px30_cif_clks,
> + .clks_num = ARRAY_SIZE(px30_cif_clks),
> +};
> +
> +static const struct of_device_id cif_plat_of_match[] = {
> + {
> + .compatible = "rockchip,px30-vip",
> + .data = &px30_cif_match_data,
> + },
> + {},
> +};
> +
> +static int cif_get_resource(struct platform_device *pdev,
> + struct cif_device *cif_dev)
> +{
> + struct resource *res;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev,
> + "Unable to allocate resources for device\n");
> + return -ENODEV;
> + }
> +
> + cif_dev->base_addr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(cif_dev->base_addr))
> + return PTR_ERR(cif_dev->base_addr);
> +
> + return 0;
> +}
> +
> +static int cif_plat_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct v4l2_device *v4l2_dev;
> + struct cif_device *cif_dev;
> + int ret, irq;
> +
> + cif_dev = devm_kzalloc(dev, sizeof(*cif_dev), GFP_KERNEL);
> + if (!cif_dev)
> + return -ENOMEM;
> +
> + cif_dev->match_data = of_device_get_match_data(dev);
> + if (!cif_dev->match_data)
> + return -ENODEV;
> +
> + platform_set_drvdata(pdev, cif_dev);
> + cif_dev->dev = dev;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = devm_request_irq(dev, irq, cif_irq_pingpong, IRQF_SHARED,
> + dev_driver_string(dev), dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "request irq failed\n");
> +
> + cif_dev->irq = irq;
> +
> + ret = cif_get_resource(pdev, cif_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_clk_bulk_get(dev, cif_dev->match_data->clks_num,
> + cif_dev->match_data->clks);
> + if (ret)
> + return ret;
> +
> + cif_dev->cif_rst = devm_reset_control_array_get(dev, false, false);
> + if (IS_ERR(cif_dev->cif_rst))
> + return PTR_ERR(cif_dev->cif_rst);
> +
> + cif_stream_init(cif_dev);
> + strscpy(cif_dev->media_dev.model, "cif",
> + sizeof(cif_dev->media_dev.model));
> + cif_dev->media_dev.dev = &pdev->dev;
> + v4l2_dev = &cif_dev->v4l2_dev;
> + v4l2_dev->mdev = &cif_dev->media_dev;
> + strscpy(v4l2_dev->name, "rockchip-cif", sizeof(v4l2_dev->name));
> +
> + ret = v4l2_device_register(cif_dev->dev, &cif_dev->v4l2_dev);
> + if (ret < 0)
> + return ret;
> +
> + media_device_init(&cif_dev->media_dev);
> +
> + ret = media_device_register(&cif_dev->media_dev);
> + if (ret < 0)
> + goto err_unreg_v4l2_dev;
> +
> + /* Create & register platform subdev. */
> + ret = cif_register_stream_vdev(cif_dev);
> + if (ret < 0)
> + goto err_unreg_media_dev;
> +
> + ret = cif_subdev_notifier(cif_dev);
> + if (ret < 0) {
> + v4l2_err(&cif_dev->v4l2_dev,
> + "Failed to register subdev notifier(%d)\n", ret);
> + goto err_unreg_stream_vdev;
> + }
> +
> + cif_set_default_format(cif_dev);
> + pm_runtime_enable(&pdev->dev);
> +
> + return 0;
> +
> +err_unreg_stream_vdev:
> + cif_unregister_stream_vdev(cif_dev);
> +err_unreg_media_dev:
> + media_device_unregister(&cif_dev->media_dev);
> +err_unreg_v4l2_dev:
> + v4l2_device_unregister(&cif_dev->v4l2_dev);
> + return ret;
> +}
> +
> +static int cif_plat_remove(struct platform_device *pdev)
> +{
> + struct cif_device *cif_dev = platform_get_drvdata(pdev);
> +
> + pm_runtime_disable(&pdev->dev);
> +
> + media_device_unregister(&cif_dev->media_dev);
> + v4l2_device_unregister(&cif_dev->v4l2_dev);
> + cif_unregister_stream_vdev(cif_dev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused cif_runtime_suspend(struct device *dev)
> +{
> + struct cif_device *cif_dev = dev_get_drvdata(dev);
> +
> + clk_bulk_disable_unprepare(cif_dev->match_data->clks_num,
> + cif_dev->match_data->clks);
> +
> + return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int __maybe_unused cif_runtime_resume(struct device *dev)
> +{
> + struct cif_device *cif_dev = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = pinctrl_pm_select_default_state(dev);
> + if (ret < 0)
> + return ret;
> +
> + return clk_bulk_prepare_enable(cif_dev->match_data->clks_num,
> + cif_dev->match_data->clks);
> +}
> +
> +static const struct dev_pm_ops cif_plat_pm_ops = {
> + .runtime_suspend = cif_runtime_suspend,
> + .runtime_resume = cif_runtime_resume,
> +};
> +
> +static struct platform_driver cif_plat_drv = {
> + .driver = {
> + .name = CIF_DRIVER_NAME,
> + .of_match_table = cif_plat_of_match,
> + .pm = &cif_plat_pm_ops,
> + },
> + .probe = cif_plat_probe,
> + .remove = cif_plat_remove,
> +};
> +module_platform_driver(cif_plat_drv);
> +
> +MODULE_AUTHOR("Rockchip Camera/ISP team");
> +MODULE_DESCRIPTION("Rockchip CIF platform driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/rockchip/cif/dev.h b/drivers/media/platform/rockchip/cif/dev.h
> new file mode 100644
> index 000000000000..3be3db08c75b
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/dev.h
> @@ -0,0 +1,139 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
> + */
> +
> +#ifndef _CIF_DEV_H
> +#define _CIF_DEV_H
> +
> +#include <linux/clk.h>
> +#include <linux/mutex.h>
> +
> +#include <media/media-device.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#define CIF_DRIVER_NAME "rockchip-cif"
> +
> +#define CIF_MAX_BUS_CLK 8
> +#define CIF_MAX_SENSOR 1
> +#define CIF_MAX_RESET 5
> +#define CIF_MAX_CSI_CHANNEL 4
> +
> +#define CIF_DEFAULT_WIDTH 640
> +#define CIF_DEFAULT_HEIGHT 480
> +
> +struct cif_buffer {
> + struct vb2_v4l2_buffer vb;
> + struct list_head queue;
> + u32 buff_addr[VIDEO_MAX_PLANES];
> +};
> +
> +static inline struct cif_buffer *to_cif_buffer(struct vb2_v4l2_buffer *vb)
> +{
> + return container_of(vb, struct cif_buffer, vb);
> +}
> +
> +struct cif_remote {
> + struct v4l2_subdev *sd;
> + int pad;
> + struct v4l2_mbus_config mbus;
> + int lanes;
> + v4l2_std_id std;
> +};
> +
> +struct cif_output_fmt {
> + u32 fourcc;
> + u32 mbus;
> + u32 fmt_val;
> + u8 cplanes;
> +};
> +
> +enum cif_fmt_type {
> + CIF_FMT_TYPE_YUV = 0,
> + CIF_FMT_TYPE_RAW,
> +};
> +
> +struct cif_input_fmt {
> + u32 mbus_code;
> + u32 dvp_fmt_val;
> + u32 csi_fmt_val;
> + enum cif_fmt_type fmt_type;
> + enum v4l2_field field;
> +};
> +
> +struct cif_stream {
> + struct cif_device *cifdev;
> + bool stopping;
> + wait_queue_head_t wq_stopped;
> + int frame_idx;
> + int frame_phase;
> + bool drop_frame;
> +
> + /* Lock between irq and buf_queue, buffs. */
> + spinlock_t vbq_lock;
> + struct vb2_queue buf_queue;
> + struct list_head buf_head;
> + struct cif_buffer *buffs[2];
> +
> + /* Vfd lock. */
> + struct mutex vlock;
The comment adds an extra value of exactly two letters. Please explain
the purpose of this mutex properly or remove it.
> + struct video_device vdev;
> + struct media_pad pad;
> +
> + struct cif_output_fmt *cif_fmt_out;
> + const struct cif_input_fmt *cif_fmt_in;
> + struct v4l2_pix_format pix;
> +};
> +
> +static inline struct cif_stream *to_cif_stream(struct video_device *vdev)
> +{
> + return container_of(vdev, struct cif_stream, vdev);
> +}
> +
> +struct cif_match_data {
> + struct clk_bulk_data *clks;
> + int clks_num;
> +};
> +
> +struct cif_device {
> + struct device *dev;
> + int irq;
> + void __iomem *base_addr;
> + struct reset_control *cif_rst;
> +
> + struct v4l2_device v4l2_dev;
> + struct media_device media_dev;
> + struct v4l2_async_notifier notifier;
> + struct v4l2_async_connection asd;
> + struct cif_remote remote;
> +
> + struct cif_stream stream;
> + const struct cif_match_data *match_data;
> +};
> +
> +static inline void
> +cif_write(struct cif_device *cif_dev, unsigned int addr, u32 val)
I think the preferred style is
static inline void cif_write(struct cif_device *cif_dev, unsigned int addr,
u32 val)
{
...
Thanks a lot and best regards,
Michael
> +{
> + writel(val, cif_dev->base_addr + addr);
> +}
> +
> +static inline u32 cif_read(struct cif_device *cif_dev, unsigned int addr)
> +{
> + return readl(cif_dev->base_addr + addr);
> +}
> +
> +void cif_unregister_stream_vdev(struct cif_device *dev);
> +int cif_register_stream_vdev(struct cif_device *dev);
> +void cif_stream_init(struct cif_device *dev);
> +void cif_set_default_format(struct cif_device *dev);
> +
> +irqreturn_t cif_irq_pingpong(int irq, void *ctx);
> +void cif_soft_reset(struct cif_device *cif_dev);
> +
> +#endif
> diff --git a/drivers/media/platform/rockchip/cif/regs.h b/drivers/media/platform/rockchip/cif/regs.h
> new file mode 100644
> index 000000000000..cc39bf9ce8da
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/regs.h
> @@ -0,0 +1,192 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
> + */
> +
> +#ifndef _CIF_REGS_H
> +#define _CIF_REGS_H
> +
> +#define CIF_CTRL 0x00
> +#define CIF_INTEN 0x04
> +#define CIF_INTSTAT 0x08
> +#define CIF_FOR 0x0c
> +#define CIF_LINE_NUM_ADDR 0x10
> +#define CIF_FRM0_ADDR_Y 0x14
> +#define CIF_FRM0_ADDR_UV 0x18
> +#define CIF_FRM1_ADDR_Y 0x1c
> +#define CIF_FRM1_ADDR_UV 0x20
> +#define CIF_VIR_LINE_WIDTH 0x24
> +#define CIF_SET_SIZE 0x28
> +#define CIF_SCM_ADDR_Y 0x2c
> +#define CIF_SCM_ADDR_U 0x30
> +#define CIF_SCM_ADDR_V 0x34
> +#define CIF_WB_UP_FILTER 0x38
> +#define CIF_WB_LOW_FILTER 0x3c
> +#define CIF_WBC_CNT 0x40
> +#define CIF_CROP 0x44
> +#define CIF_SCL_CTRL 0x48
> +#define CIF_SCL_DST 0x4c
> +#define CIF_SCL_FCT 0x50
> +#define CIF_SCL_VALID_NUM 0x54
> +#define CIF_LINE_LOOP_CTR 0x58
> +#define CIF_FRAME_STATUS 0x60
> +#define CIF_CUR_DST 0x64
> +#define CIF_LAST_LINE 0x68
> +#define CIF_LAST_PIX 0x6c
> +#define CIF_FETCH_Y_LAST_LINE(VAL) ((VAL) & 0x1fff)
> +
> +#define CIF_CTRL_ENABLE_CAPTURE BIT(0)
> +#define CIF_CTRL_MODE_PINGPONG BIT(1)
> +#define CIF_CTRL_MODE_LINELOOP BIT(2)
> +#define CIF_CTRL_AXI_BURST_16 (0xf << 12)
> +
> +#define CIF_INTEN_FRAME_END_EN BIT(0)
> +#define CIF_INTEN_LINE_ERR_EN BIT(2)
> +#define CIF_INTEN_BUS_ERR_EN BIT(6)
> +#define CIF_INTEN_SCL_ERR_EN BIT(7)
> +#define CIF_INTEN_PST_INF_FRAME_END_EN BIT(9)
> +
> +#define CIF_INTSTAT_CLS 0x3ff
> +#define CIF_INTSTAT_FRAME_END BIT(0)
> +#define CIF_INTSTAT_LINE_END BIT(1)
> +#define CIF_INTSTAT_LINE_ERR BIT(2)
> +#define CIF_INTSTAT_PIX_ERR BIT(3)
> +#define CIF_INTSTAT_DFIFO_OF BIT(5)
> +#define CIF_INTSTAT_BUS_ERR BIT(6)
> +#define CIF_INTSTAT_PRE_INF_FRAME_END BIT(8)
> +#define CIF_INTSTAT_PST_INF_FRAME_END BIT(9)
> +#define CIF_INTSTAT_FRAME_END_CLR BIT(0)
> +#define CIF_INTSTAT_LINE_END_CLR BIT(1)
> +#define CIF_INTSTAT_LINE_ERR_CLR BIT(2)
> +#define CIF_INTSTAT_PST_INF_FRAME_END_CLR BIT(9)
> +#define CIF_INTSTAT_ERR 0xfc
> +
> +#define CIF_FRAME_STAT_CLS 0x00
> +#define CIF_FRAME_FRM0_STAT_CLS 0x20
> +
> +#define CIF_FORMAT_VSY_HIGH_ACTIVE BIT(0)
> +#define CIF_FORMAT_HSY_LOW_ACTIVE BIT(1)
> +
> +#define CIF_FORMAT_INPUT_MODE_YUV (0x00 << 2)
> +#define CIF_FORMAT_INPUT_MODE_PAL (0x02 << 2)
> +#define CIF_FORMAT_INPUT_MODE_NTSC (0x03 << 2)
> +#define CIF_FORMAT_INPUT_MODE_BT1120 (0x07 << 2)
> +#define CIF_FORMAT_INPUT_MODE_RAW (0x04 << 2)
> +#define CIF_FORMAT_INPUT_MODE_JPEG (0x05 << 2)
> +#define CIF_FORMAT_INPUT_MODE_MIPI (0x06 << 2)
> +
> +#define CIF_FORMAT_YUV_INPUT_ORDER_UYVY (0x00 << 5)
> +#define CIF_FORMAT_YUV_INPUT_ORDER_YVYU BIT(5)
> +#define CIF_FORMAT_YUV_INPUT_ORDER_VYUY BIT(6)
> +#define CIF_FORMAT_YUV_INPUT_ORDER_YUYV (0x03 << 5)
> +#define CIF_FORMAT_YUV_INPUT_422 (0x00 << 7)
> +#define CIF_FORMAT_YUV_INPUT_420 BIT(7)
> +
> +#define CIF_FORMAT_INPUT_420_ORDER_ODD BIT(8)
> +
> +#define CIF_FORMAT_CCIR_INPUT_ORDER_EVEN BIT(9)
> +
> +#define CIF_FORMAT_RAW_DATA_WIDTH_8 (0x00 << 11)
> +#define CIF_FORMAT_RAW_DATA_WIDTH_10 BIT(11)
> +#define CIF_FORMAT_RAW_DATA_WIDTH_12 (0x02 << 11)
> +
> +#define CIF_FORMAT_YUV_OUTPUT_422 (0x00 << 16)
> +#define CIF_FORMAT_YUV_OUTPUT_420 BIT(16)
> +
> +#define CIF_FORMAT_OUTPUT_420_ORDER_EVEN (0x00 << 17)
> +#define CIF_FORMAT_OUTPUT_420_ORDER_ODD BIT(17)
> +
> +#define CIF_FORMAT_RAWD_DATA_LITTLE_ENDIAN (0x00 << 18)
> +#define CIF_FORMAT_RAWD_DATA_BIG_ENDIAN BIT(18)
> +
> +#define CIF_FORMAT_UV_STORAGE_ORDER_UVUV (0x00 << 19)
> +#define CIF_FORMAT_UV_STORAGE_ORDER_VUVU BIT(19)
> +
> +#define CIF_FORMAT_BT1120_CLOCK_SINGLE_EDGES (0x00 << 24)
> +#define CIF_FORMAT_BT1120_CLOCK_DOUBLE_EDGES BIT(24)
> +#define CIF_FORMAT_BT1120_TRANSMIT_INTERFACE (0x00 << 25)
> +#define CIF_FORMAT_BT1120_TRANSMIT_PROGRESS BIT(25)
> +#define CIF_FORMAT_BT1120_YC_SWAP BIT(26)
> +
> +#define CIF_SCL_CTRL_ENABLE_SCL_DOWN BIT(0)
> +#define CIF_SCL_CTRL_ENABLE_SCL_UP BIT(1)
> +#define CIF_SCL_CTRL_ENABLE_YUV_16BIT_BYPASS BIT(4)
> +#define CIF_SCL_CTRL_ENABLE_RAW_16BIT_BYPASS BIT(5)
> +#define CIF_SCL_CTRL_ENABLE_32BIT_BYPASS BIT(6)
> +#define CIF_SCL_CTRL_DISABLE_32BIT_BYPASS (0x00 << 6)
> +
> +#define CIF_INTSTAT_F0_READY BIT(0)
> +#define CIF_INTSTAT_F1_READY BIT(1)
> +
> +#define CIF_CROP_Y_SHIFT 16
> +#define CIF_CROP_X_SHIFT 0
> +
> +#define CIF_CSI_ENABLE_CAPTURE BIT(0)
> +#define CIF_CSI_WRDDR_TYPE_RAW8 (0x0 << 1)
> +#define CIF_CSI_WRDDR_TYPE_RAW10 BIT(1)
> +#define CIF_CSI_WRDDR_TYPE_RAW12 (0x2 << 1)
> +#define CIF_CSI_WRDDR_TYPE_RGB888 (0x3 << 1)
> +#define CIF_CSI_WRDDR_TYPE_YUV422 (0x4 << 1)
> +#define CIF_CSI_ENABLE_COMMAND_MODE BIT(4)
> +#define CIF_CSI_ENABLE_CROP BIT(5)
> +
> +#define CIF_CSI_FRAME0_START_INTEN(id) (0x1 << ((id) * 2))
> +#define CIF_CSI_FRAME1_START_INTEN(id) (0x1 << ((id) * 2 + 1))
> +#define CIF_CSI_FRAME0_END_INTEN(id) (0x1 << ((id) * 2 + 8))
> +#define CIF_CSI_FRAME1_END_INTEN(id) (0x1 << ((id) * 2 + 9))
> +#define CIF_CSI_DMA_Y_FIFO_OVERFLOW_INTEN BIT(16)
> +#define CIF_CSI_DMA_UV_FIFO_OVERFLOW_INTEN BIT(17)
> +#define CIF_CSI_CONFIG_FIFO_OVERFLOW_INTEN BIT(18)
> +#define CIF_CSI_BANDWIDTH_LACK_INTEN BIT(19)
> +#define CIF_CSI_RX_FIFO_OVERFLOW_INTEN BIT(20)
> +#define CIF_CSI_ALL_FRAME_START_INTEN (0xff << 0)
> +#define CIF_CSI_ALL_FRAME_END_INTEN (0xff << 8)
> +#define CIF_CSI_ALL_ERROR_INTEN (0x1f << 16)
> +
> +#define CIF_CSI_FRAME0_START_ID0 BIT(0)
> +#define CIF_CSI_FRAME1_START_ID0 BIT(1)
> +#define CIF_CSI_FRAME0_START_ID1 BIT(2)
> +#define CIF_CSI_FRAME1_START_ID1 BIT(3)
> +#define CIF_CSI_FRAME0_START_ID2 BIT(4)
> +#define CIF_CSI_FRAME1_START_ID2 BIT(5)
> +#define CIF_CSI_FRAME0_START_ID3 BIT(6)
> +#define CIF_CSI_FRAME1_START_ID3 BIT(7)
> +#define CIF_CSI_FRAME0_END_ID0 BIT(8)
> +#define CIF_CSI_FRAME1_END_ID0 BIT(9)
> +#define CIF_CSI_FRAME0_END_ID1 BIT(10)
> +#define CIF_CSI_FRAME1_END_ID1 BIT(11)
> +#define CIF_CSI_FRAME0_END_ID2 BIT(12)
> +#define CIF_CSI_FRAME1_END_ID2 BIT(13)
> +#define CIF_CSI_FRAME0_END_ID3 BIT(14)
> +#define CIF_CSI_FRAME1_END_ID3 BIT(15)
> +#define CIF_CSI_DMA_Y_FIFO_OVERFLOW BIT(16)
> +#define CIF_CSI_DMA_UV_FIFO_OVERFLOW BIT(17)
> +#define CIF_CSI_CONFIG_FIFO_OVERFLOW BIT(18)
> +#define CIF_CSI_BANDWIDTH_LACK BIT(19)
> +#define CIF_CSI_RX_FIFO_OVERFLOW BIT(20)
> +
> +#define CIF_CSI_FIFO_OVERFLOW (CIF_CSI_DMA_Y_FIFO_OVERFLOW | \
> + CIF_CSI_DMA_UV_FIFO_OVERFLOW | \
> + CIF_CSI_CONFIG_FIFO_OVERFLOW | \
> + CIF_CSI_RX_FIFO_OVERFLOW)
> +
> +#define CIF_CSIHOST_N_LANES 0x04
> +#define CIF_CSIHOST_PHY_RSTZ 0x0c
> +#define CIF_CSIHOST_RESETN 0x10
> +#define CIF_CSIHOST_ERR1 0x20
> +#define CIF_CSIHOST_ERR2 0x24
> +#define CIF_CSIHOST_MSK1 0x28
> +#define CIF_CSIHOST_MSK2 0x2c
> +#define CIF_CSIHOST_CONTROL 0x40
> +
> +#define CIF_SW_CPHY_EN(x) ((x) << 0)
> +#define CIF_SW_DSI_EN(x) ((x) << 4)
> +#define CIF_SW_DATATYPE_FS(x) ((x) << 8)
> +#define CIF_SW_DATATYPE_FE(x) ((x) << 14)
> +#define CIF_SW_DATATYPE_LS(x) ((x) << 20)
> +#define CIF_SW_DATATYPE_LE(x) ((x) << 26)
> +
> +#endif
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface
2023-11-08 16:38 ` [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
2023-11-10 12:50 ` Michael Riesch
@ 2023-11-10 14:33 ` Michael Riesch
2023-11-13 15:28 ` Mehdi Djait
1 sibling, 1 reply; 20+ messages in thread
From: Michael Riesch @ 2023-11-10 14:33 UTC (permalink / raw)
To: Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt
Cc: linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski
Hi Mehdi,
Sorry, forgot one thing:
On 11/8/23 17:38, Mehdi Djait wrote:
> [...]
> diff --git a/drivers/media/platform/rockchip/cif/dev.c b/drivers/media/platform/rockchip/cif/dev.c
> new file mode 100644
> index 000000000000..f7d061a13577
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/dev.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Camera Interface Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
> + */
> +
> +#include "linux/platform_device.h"
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/reset.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "dev.h"
> +#include "regs.h"
> +
> +static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct cif_device *cif_dev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + cif_dev = container_of(notifier, struct cif_device, notifier);
> + sd = cif_dev->remote.sd;
> +
> + mutex_lock(&cif_dev->media_dev.graph_mutex);
> +
> + ret = v4l2_device_register_subdev_nodes(&cif_dev->v4l2_dev);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = media_create_pad_link(&sd->entity, 0,
> + &cif_dev->stream.vdev.entity, 0,
> + MEDIA_LNK_FL_ENABLED);
> + if (ret)
> + dev_err(cif_dev->dev, "failed to create link");
> +
> +unlock:
> + mutex_unlock(&cif_dev->media_dev.graph_mutex);
> + return ret;
> +}
> +
> +static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_connection *asd)
> +{
> + struct cif_device *cif_dev = container_of(notifier,
> + struct cif_device, notifier);
> + int pad;
> +
> + cif_dev->remote.sd = subdev;
> + pad = media_entity_get_fwnode_pad(&subdev->entity, subdev->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (pad < 0)
> + return pad;
> +
> + cif_dev->remote.pad = pad;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
> + .bound = subdev_notifier_bound,
> + .complete = subdev_notifier_complete,
> +};
> +
> +static int cif_subdev_notifier(struct cif_device *cif_dev)
> +{
> + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
> + struct device *dev = cif_dev->dev;
> + struct v4l2_async_connection *asd;
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_PARALLEL,
This is surprising. I had to set this to V4L2_MBUS_UNKNOWN, otherwise
v4l2_fwnode_endpoint_parse would yield -ENXIO, which indicates a bus
type mismatch. Does this really work for your (BT.656, right?) setup?
I think we should get the bus type from the device tree, right?
Thanks and best regards,
Michael
> [...]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-09 18:07 ` Paul Kocialkowski
@ 2023-11-10 18:23 ` Conor Dooley
2023-11-13 16:54 ` Paul Kocialkowski
0 siblings, 1 reply; 20+ messages in thread
From: Conor Dooley @ 2023-11-10 18:23 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Krzysztof Kozlowski, Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt, linux-media,
devicetree, linux-kernel, thomas.petazzoni, alexandre.belloni,
maxime.chevallier, michael.riesch
[-- Attachment #1: Type: text/plain, Size: 4347 bytes --]
On Thu, Nov 09, 2023 at 07:07:27PM +0100, Paul Kocialkowski wrote:
> On Thu 09 Nov 23, 18:53, Krzysztof Kozlowski wrote:
> > On 09/11/2023 18:45, Paul Kocialkowski wrote:
> > > On Thu 09 Nov 23, 17:24, Conor Dooley wrote:
> > >> On Wed, Nov 08, 2023 at 05:38:56PM +0100, Mehdi Djait wrote:
> > >>> Add a documentation for the Rockchip Camera Interface binding.
> > >>>
> > >>> the name of the file rk3066 is the first Rockchip SoC generation that uses cif
> > >>> instead of the px30 which is just one of the many iterations of the unit.
> > >>
> > >> I think this is becoming ridiculous. You've now removed the compatible
> > >> for the rk3066 but kept it in the filename. I don't understand the
> > >> hangup about naming the file after the px30-vip, but naming it after
> > >> something that is not documented here at all makes no sense to me.
> > >> Either document the rk3066 properly, or remove all mention of it IMO.
> > >
> > > I think the opposite is ridiculous. We have spent some time investigating the
> > > history of this unit, to find out that RK3066 is the first occurence where
> > > it exists. Since we want the binding to cover all generations of the same unit
> > > and give it a name that reflects this, rk3066 is the natural choice that comes
> > > to mind. As far as I understand, this is the normal thing to do to name
> > > bindings: name after the earliest known occurence of the unit.
> > >
> > > What is the rationale behind naming the file after a generation of the unit
> > > that happens to be the one introducing the binding? This is neither the first
> > > nor the last one to include this unit. The binding will be updated later to
> > > cover other generations. Do we want to rename the file each time an a generation
> > > earlier than px30 is introduced? That sounds quite ridiculous too.
> > >
> > > We've done the research work to give it the most relevant name here.
> > > I'd expect some strong arguments not to use it. Can you ellaborate?
> >
> > If you do not have rk3066 documented here, it might be added to entirely
> > different file (for whatever reasons, including that binding would be
> > quite different than px30). Thus you would have rk3066 in
> > rockchip,rk3066-cif-added-later.yaml and px30 in rockchip,rk3066-cif.yaml
>
> As far as I could see we generally manage to include support for different
> hardware setups in the same binding document using conditionals on the
> compatible, so this feels a bit far-fetched.
>
> Of course you're the maintainer and have significantly more experience here
> so there might be a lot that I'm not seeing, but I'm not very convinced by this
> reasoning to be honest.
>
> > Just use the filename matching the compatible. That's what we always
> > ask. In every review.
>
> Yeah and we very often end up with naming that is less than optimal (to stay
> polite). I'm generally quite appalled by the overall lack of interest that
> naming gets, as if it was something secondary. Naming is one of the most
> important and difficult things in our field of work and it needs to be
> considered with care.
>
> This is not just a problem with device-tree, it's a kernel-wide issue that
> nobody seems to be interested in addressing. I'm quite unhappy to see that when
> time is spent trying to improve the situation on one particular instance, we are
> shown the door because it doesn't match what is generally done (and often done
> wrong).
>
> This is definitely a rant. I really want to express this issue loud and clear
> and encourage everyone to consider it for what it is.
Look chief, I do understand your frustration here, with the seemingly
arbitrary naming etc. I'm apologise if using the word "ridiculous" earlier
pissed you off. I'm sure you can similarly understand why we don't want
to accept either having a compatible for the rk3066-cif in the file,
when you are not yet sure of the correct constraints, or given your
interest in naming, why calling it after something that it does not even
document is misleading.
Ultimately, I don't care what the file ends up being called when there
are multiple devices documented in it. I'd ack a patch renaming to the
œriginal incarnation of the IP when the documentation for that IP is
added without a second thought.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface
2023-11-10 12:50 ` Michael Riesch
@ 2023-11-13 11:06 ` Mehdi Djait
2023-11-13 15:41 ` Michael Riesch
0 siblings, 1 reply; 20+ messages in thread
From: Mehdi Djait @ 2023-11-13 11:06 UTC (permalink / raw)
To: Michael Riesch
Cc: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt, linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski
Hi Michael,
On Fri, Nov 10, 2023 at 01:50:01PM +0100, Michael Riesch wrote:
> Hi Mehdi,
>
> Thanks for your patches.
>
> The good news first: with some hacks applied I was able to capture the
> video stream from a HDMI receiver chip via BT.1120 on a Rockchip RK3568.
this is really cool!
>
> As a next step, I'll clean up the hacky RK3568 support and submit them
> for review.
>
> Still, there are some issues that needs to be addressed. Please find my
> comments inline.
>
> On 11/8/23 17:38, Mehdi Djait wrote:
> > This introduces a V4L2 driver for the Rockchip CIF video capture controller.
> >
> > This controller supports multiple parallel interfaces, but for now only the
> > BT.656 interface could be tested, hence it's the only one that's supported
> > in the first version of this driver.
> >
> > This controller can be found on RK3066, PX30, RK1808, RK3128 and RK3288,
> > but for now it's only been tested on the PX30.
> >
> > CIF is implemented as a video node-centric driver.
> >
> > Most of this driver was written following the BSP driver from rockchip,
> > removing the parts that either didn't fit correctly the guidelines, or that
> > couldn't be tested.
> >
> > This basic version doesn't support cropping nor scaling and is only
> > designed with one SDTV video decoder being attached to it at any time.
> >
> > This version uses the "pingpong" mode of the controller, which is a
> > double-buffering mechanism.
> >
> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > +static const struct cif_input_fmt in_fmts[] = {
> > + {
> > + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> > + CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
> > + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
>
> What is the point of this csi_fmt_val field? If the strategy is to kick
> everything out that is not explicitly required then this should be
> removed (and added at a later stage, if needed).
>
I can remove this but I don't really see the harm of keeping this.
It can even save some time for the person adding the support for CSI
later.
> > + .fmt_type = CIF_FMT_TYPE_YUV,
> > + .field = V4L2_FIELD_NONE,
> > + }, {
> > + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> > + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> > + CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
> > + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> > + .fmt_type = CIF_FMT_TYPE_YUV,
> > + .field = V4L2_FIELD_INTERLACED,
> > + }, {
> > + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> > + CIF_FORMAT_YUV_INPUT_ORDER_YVYU,
> > + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> > + .fmt_type = CIF_FMT_TYPE_YUV,
> > + .field = V4L2_FIELD_NONE,
>
> What is the difference between this entry (in_fmts[2]) and in_fmts[0]?
> Similarly, between in_fmts[1] and in_fmts[3]?
>
Between in_fmts[0] and in_fmts[2] is the order of Y U V components:
0 -> YUYV
2 -> YVYU
between in_fmts[1] and in_fmts[3]: the same thing:
1 -> YUYV
3 -> YVYU
> > + }, {
> > + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> > + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> > + CIF_FORMAT_YUV_INPUT_ORDER_YVYU,
> > + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> > + .fmt_type = CIF_FMT_TYPE_YUV,
> > + .field = V4L2_FIELD_INTERLACED,
> > + }, {
> > + .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> > + CIF_FORMAT_YUV_INPUT_ORDER_UYVY,
> > + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> > + .fmt_type = CIF_FMT_TYPE_YUV,
> > + .field = V4L2_FIELD_NONE,
> > + }, {
> > + .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> > + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> > + CIF_FORMAT_YUV_INPUT_ORDER_UYVY,
> > + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> > + .fmt_type = CIF_FMT_TYPE_YUV,
> > + .field = V4L2_FIELD_INTERLACED,
> > + }, {
> > + .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> > + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
> > + CIF_FORMAT_YUV_INPUT_ORDER_VYUY,
> > + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
> > + .fmt_type = CIF_FMT_TYPE_YUV,
> > + .field = V4L2_FIELD_NONE,
> > + }, {
> > +static const struct
> > +cif_input_fmt *get_input_fmt(struct v4l2_subdev *sd)
> > +{
> > + struct v4l2_subdev_format fmt;
> > + u32 i;
> > +
> > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + fmt.pad = 0;
> > + v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> > +
> > + for (i = 0; i < ARRAY_SIZE(in_fmts); i++)
> > + if (fmt.format.code == in_fmts[i].mbus_code &&
> > + fmt.format.field == in_fmts[i].field)
> > + return &in_fmts[i];
> > +
> > + v4l2_err(sd->v4l2_dev, "remote's mbus code not supported\n");
> > + return NULL;
> > +}
> > +
> > +static struct
> > +cif_output_fmt *find_output_fmt(struct cif_stream *stream, u32 pixelfmt)
> > +{
> > + struct cif_output_fmt *fmt;
> > + u32 i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(out_fmts); i++) {
> > + fmt = &out_fmts[i];
> > + if (fmt->fourcc == pixelfmt)
> > + return fmt;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static struct cif_buffer *cif_get_buffer(struct cif_stream *stream)
> > +{
> > + struct cif_buffer *buff;
> > +
> > + lockdep_assert_held(&stream->vbq_lock);
> > +
> > + if (list_empty(&stream->buf_head))
> > + return NULL;
> > +
> > + buff = list_first_entry(&stream->buf_head, struct cif_buffer, queue);
> > + list_del(&buff->queue);
> > +
> > + return buff;
> > +}
> > +
> > +static int cif_init_buffers(struct cif_stream *stream)
> > +{
> > + struct cif_device *cif_dev = stream->cifdev;
> > + unsigned long lock_flags;
> > +
> > + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> > +
> > + stream->buffs[0] = cif_get_buffer(stream);
> > + stream->buffs[1] = cif_get_buffer(stream);
> > +
> > + if (!(stream->buffs[0]) || !(stream->buffs[1])) {
> > + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> > + return -EINVAL;
> > + }
> > +
> > + stream->drop_frame = false;
> > +
> > + cif_write(cif_dev, CIF_FRM0_ADDR_Y,
> > + stream->buffs[0]->buff_addr[CIF_PLANE_Y]);
> > + cif_write(cif_dev, CIF_FRM0_ADDR_UV,
> > + stream->buffs[0]->buff_addr[CIF_PLANE_UV]);
> > +
> > + cif_write(cif_dev, CIF_FRM1_ADDR_Y,
> > + stream->buffs[1]->buff_addr[CIF_PLANE_Y]);
> > + cif_write(cif_dev, CIF_FRM1_ADDR_UV,
> > + stream->buffs[1]->buff_addr[CIF_PLANE_UV]);
> > +
> > + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> > +
> > + return 0;
> > +}
> > +
> > +static void cif_assign_new_buffer_pingpong(struct cif_stream *stream)
> > +{
> > + struct cif_device *cif_dev = stream->cifdev;
> > + struct cif_buffer *buffer = NULL;
> > + u32 frm_addr_y, frm_addr_uv;
> > + unsigned long lock_flags;
> > +
> > + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> > +
> > + buffer = cif_get_buffer(stream);
> > +
> > + /*
> > + * In Pingpong mode:
> > + * After one frame0 captured, CIF will start to capture the next frame1
> > + * automatically.
> > + *
> > + * If there is no buffer:
> > + * 1. Make the next frame0 write to the buffer of frame1.
> > + *
> > + * 2. Drop the frame1: Don't return it to user-space, as it will be
> > + * overwritten by the next frame0.
> > + */
> > + if (!buffer) {
> > + stream->drop_frame = true;
> > + buffer = stream->buffs[1 - stream->frame_phase];
> > + } else {
> > + stream->drop_frame = false;
> > + }
> > +
> > + stream->buffs[stream->frame_phase] = buffer;
> > + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> > +
> > + frm_addr_y = stream->frame_phase ? CIF_FRM1_ADDR_Y : CIF_FRM0_ADDR_Y;
> > + frm_addr_uv = stream->frame_phase ? CIF_FRM1_ADDR_UV : CIF_FRM0_ADDR_UV;
> > +
> > + cif_write(cif_dev, frm_addr_y, buffer->buff_addr[CIF_PLANE_Y]);
> > + cif_write(cif_dev, frm_addr_uv, buffer->buff_addr[CIF_PLANE_UV]);
> > +}
> > +
> > +static void cif_stream_stop(struct cif_stream *stream)
> > +{
> > + struct cif_device *cif_dev = stream->cifdev;
> > + u32 val;
> > +
> > + val = cif_read(cif_dev, CIF_CTRL);
> > + cif_write(cif_dev, CIF_CTRL, val & (~CIF_CTRL_ENABLE_CAPTURE));
> > + cif_write(cif_dev, CIF_INTEN, 0x0);
> > + cif_write(cif_dev, CIF_INTSTAT, 0x3ff);
> > + cif_write(cif_dev, CIF_FRAME_STATUS, 0x0);
> > +
> > + stream->stopping = false;
> > +}
> > +
> > +static int cif_queue_setup(struct vb2_queue *queue,
> > + unsigned int *num_buffers,
> > + unsigned int *num_planes,
> > + unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct cif_stream *stream = queue->drv_priv;
> > + const struct v4l2_pix_format *pix;
> > +
> > + pix = &stream->pix;
> > +
> > + if (*num_planes) {
> > + if (*num_planes != 1)
> > + return -EINVAL;
> > +
> > + if (sizes[0] < pix->sizeimage)
> > + return -EINVAL;
> > + return 0;
> > + }
> > +
> > + *num_planes = 1;
> > +
> > + sizes[0] = pix->sizeimage;
> > +
> > + *num_buffers = CIF_REQ_BUFS_MIN;
> > +
> > + return 0;
> > +}
> > +
> > +static void cif_buf_queue(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct cif_buffer *cifbuf = to_cif_buffer(vbuf);
> > + struct vb2_queue *queue = vb->vb2_queue;
> > + struct cif_stream *stream = queue->drv_priv;
> > + struct v4l2_pix_format *pix = &stream->pix;
> > + unsigned long lock_flags;
> > + int i;
> > +
> > + struct cif_output_fmt *fmt = stream->cif_fmt_out;
> > +
> > + memset(cifbuf->buff_addr, 0, sizeof(cifbuf->buff_addr));
> > +
> > + cifbuf->buff_addr[0] = vb2_dma_contig_plane_dma_addr(vb, 0);
> > +
> > + for (i = 0; i < fmt->cplanes - 1; i++)
> > + cifbuf->buff_addr[i + 1] = cifbuf->buff_addr[i] +
> > + pix->bytesperline * pix->height;
> > +
> > + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> > + list_add_tail(&cifbuf->queue, &stream->buf_head);
> > + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> > +}
> > +
> > +static void cif_return_all_buffers(struct cif_stream *stream,
> > + enum vb2_buffer_state state)
> > +{
> > + struct cif_buffer *buf;
> > + unsigned long lock_flags;
> > +
> > + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> > +
> > + if (stream->buffs[0]) {
> > + vb2_buffer_done(&stream->buffs[0]->vb.vb2_buf, state);
> > + stream->buffs[0] = NULL;
> > + }
> > +
> > + if (stream->buffs[1]) {
> > + if (!stream->drop_frame)
> > + vb2_buffer_done(&stream->buffs[1]->vb.vb2_buf, state);
> > +
> > + stream->buffs[1] = NULL;
> > + }
> > +
> > + while (!list_empty(&stream->buf_head)) {
> > + buf = cif_get_buffer(stream);
> > + vb2_buffer_done(&buf->vb.vb2_buf, state);
> > + }
> > +
> > + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> > +}
> > +
> > +static void cif_stop_streaming(struct vb2_queue *queue)
> > +{
> > + struct cif_stream *stream = queue->drv_priv;
> > + struct cif_device *cif_dev = stream->cifdev;
> > + struct v4l2_subdev *sd;
> > + int ret;
> > +
> > + stream->stopping = true;
> > + ret = wait_event_timeout(stream->wq_stopped,
> > + !stream->stopping,
> > + msecs_to_jiffies(1000));
> > + if (!ret) {
> > + cif_stream_stop(stream);
> > + stream->stopping = false;
> > + }
> > +
> > + /* Stop the sub device. */
> > + sd = cif_dev->remote.sd;
> > + v4l2_subdev_call(sd, video, s_stream, 0);
> > +
> > + pm_runtime_put(cif_dev->dev);
> > +
> > + cif_return_all_buffers(stream, VB2_BUF_STATE_ERROR);
> > +}
> > +
> > +static int cif_stream_start(struct cif_stream *stream)
> > +{
> > + u32 val, mbus_flags, href_pol, vsync_pol, fmt_type,
> > + xfer_mode = 0, yc_swap = 0;
> > + struct cif_device *cif_dev = stream->cifdev;
> > + struct cif_remote *remote_info;
> > + int ret;
> > + u32 input_mode;
> > +
> > + remote_info = &cif_dev->remote;
> > + fmt_type = stream->cif_fmt_in->fmt_type;
> > + stream->frame_idx = 0;
>
> Those lines are somewhat mixed. The reset of the frame_idx should be
> made more visible. The remote_info line could be integrated into the
> declaration. For the fmt_type line please see the comment below.
>
> > + input_mode = (remote_info->std == V4L2_STD_NTSC) ?
> > + CIF_FORMAT_INPUT_MODE_NTSC :
> > + CIF_FORMAT_INPUT_MODE_PAL;
>
> This seems to be an oversimplification. How can one use BT.656 here?
I don't quite understand the question. This is used to configure the
hardware, i.e., the INPUT_MODE of the format VIP_FOR
bits 4:2
INPUT_MODE Input mode:
3'b000 : YUV
3'b010 : PAL
3'b011 : NTSC
3'b100 : RAW
3'b101 : JPEG
3'b110 : MIPI
Other : invalid
> (Aren't you using BT.656 as mbus format between your video decoder and
> the PX30 VIP?)
I look into this. I will probably need to add this.
> You should not assume that the remote is capable of any TV standards
> (this statement holds for the driver in general).
>
But this is the support I am adding right now, for cif with a SDTV
decoder capable of TV standards. This statement will need to be
changed when support for sensors is added.
> > + mbus_flags = remote_info->mbus.bus.parallel.flags;
> > + href_pol = (mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) ?
> > + 0 : CIF_FORMAT_HSY_LOW_ACTIVE;
> > + vsync_pol = (mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) ?
> > + CIF_FORMAT_VSY_HIGH_ACTIVE : 0;
> > +
> > + val = vsync_pol | href_pol | input_mode | stream->cif_fmt_out->fmt_val |
> > + stream->cif_fmt_in->dvp_fmt_val | xfer_mode | yc_swap;
> > +void cif_set_default_format(struct cif_device *cif_dev)
> > +{
> > + struct cif_stream *stream = &cif_dev->stream;
> > + struct v4l2_pix_format pix;
> > +
> > + cif_dev->remote.std = V4L2_STD_NTSC;
>
> Not every subdevice supports TV standards. Is this really reasonable?
>
For the support I am adding right now it is reasonable but for future
support it needs to be changed.
> > +
> > + pix.pixelformat = V4L2_PIX_FMT_NV12;
> > + pix.width = CIF_DEFAULT_WIDTH;
> > + pix.height = CIF_DEFAULT_HEIGHT;
> > +
> > + cif_set_fmt(stream, &pix);
> > +}
> > +
> > +static int cif_enum_input(struct file *file, void *priv,
> > + struct v4l2_input *input)
> > +{
> > + struct cif_stream *stream = video_drvdata(file);
> > + struct v4l2_subdev *sd = stream->cifdev->remote.sd;
> > + int ret;
> > +
> > + if (input->index > 0)
> > + return -EINVAL;
> > +
> > + ret = v4l2_subdev_call(sd, video, g_input_status, &input->status);
> > + if (ret)
> > + return ret;
> > +
> > + strscpy(input->name, "Camera", sizeof(input->name));
> > + input->type = V4L2_INPUT_TYPE_CAMERA;
>
> Wait, we are a camera in any case? How does this fit together with your
> video decoder setup?
>
Yes the video decoder is attached to a camera.
From the kernel documentation:
https://docs.kernel.org/userspace-api/media/v4l/vidioc-enuminput.html?highlight=v4l2_input_type_camera
--------------------------------------------------------------------------------
V4L2_INPUT_TYPE_CAMERA
Any non-tuner video input, for example Composite Video, S-Video, HDMI, camera
sensor. The naming as _TYPE_CAMERA is historical, today we would have called it
_TYPE_VIDEO.
--------------------------------------------------------------------------------
> > + input->std = stream->vdev.tvnorms;
> > + input->capabilities = V4L2_IN_CAP_STD;
>
> Not every subdevice supports TV standards. Is this really reasonable?
>
see above answer.
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface
2023-11-10 14:33 ` Michael Riesch
@ 2023-11-13 15:28 ` Mehdi Djait
2023-11-13 16:05 ` Michael Riesch
0 siblings, 1 reply; 20+ messages in thread
From: Mehdi Djait @ 2023-11-13 15:28 UTC (permalink / raw)
To: Michael Riesch
Cc: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt, linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski
Hi Michael,
On Fri, Nov 10, 2023 at 03:33:34PM +0100, Michael Riesch wrote:
> Hi Mehdi,
>
> Sorry, forgot one thing:
>
> On 11/8/23 17:38, Mehdi Djait wrote:
> > +static int cif_subdev_notifier(struct cif_device *cif_dev)
> > +{
> > + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
> > + struct device *dev = cif_dev->dev;
> > + struct v4l2_async_connection *asd;
> > + struct v4l2_fwnode_endpoint vep = {
> > + .bus_type = V4L2_MBUS_PARALLEL,
>
> This is surprising. I had to set this to V4L2_MBUS_UNKNOWN, otherwise
> v4l2_fwnode_endpoint_parse would yield -ENXIO, which indicates a bus
> type mismatch. Does this really work for your (BT.656, right?) setup?
>
Yes it works.
> I think we should get the bus type from the device tree, right?
>
I am looking into this.
> Thanks and best regards,
> Michael
>
I assume you have a "bus-type = <MEDIA_BUS_TYPE_BT656>;" in the device
tree definition of your endpoint ? This caused the mismatch as the
v4l2_fwnode_endpoint is set to PARALLEL
--
Kind Regards
Mehdi Djait
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface
2023-11-13 11:06 ` Mehdi Djait
@ 2023-11-13 15:41 ` Michael Riesch
0 siblings, 0 replies; 20+ messages in thread
From: Michael Riesch @ 2023-11-13 15:41 UTC (permalink / raw)
To: Mehdi Djait
Cc: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt, linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski
Hi Mehdi,
On 11/13/23 12:06, Mehdi Djait wrote:
> Hi Michael,
>
> On Fri, Nov 10, 2023 at 01:50:01PM +0100, Michael Riesch wrote:
>> Hi Mehdi,
>>
>> Thanks for your patches.
>>
>> The good news first: with some hacks applied I was able to capture the
>> video stream from a HDMI receiver chip via BT.1120 on a Rockchip RK3568.
>
> this is really cool!
>
>>
>> As a next step, I'll clean up the hacky RK3568 support and submit them
>> for review.
>>
>> Still, there are some issues that needs to be addressed. Please find my
>> comments inline.
>>
>> On 11/8/23 17:38, Mehdi Djait wrote:
>>> This introduces a V4L2 driver for the Rockchip CIF video capture controller.
>>>
>>> This controller supports multiple parallel interfaces, but for now only the
>>> BT.656 interface could be tested, hence it's the only one that's supported
>>> in the first version of this driver.
>>>
>>> This controller can be found on RK3066, PX30, RK1808, RK3128 and RK3288,
>>> but for now it's only been tested on the PX30.
>>>
>>> CIF is implemented as a video node-centric driver.
>>>
>>> Most of this driver was written following the BSP driver from rockchip,
>>> removing the parts that either didn't fit correctly the guidelines, or that
>>> couldn't be tested.
>>>
>>> This basic version doesn't support cropping nor scaling and is only
>>> designed with one SDTV video decoder being attached to it at any time.
>>>
>>> This version uses the "pingpong" mode of the controller, which is a
>>> double-buffering mechanism.
>>>
>>> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
>>> +static const struct cif_input_fmt in_fmts[] = {
>>> + {
>>> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
>>> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
>>> + CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
>>> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
>>
>> What is the point of this csi_fmt_val field? If the strategy is to kick
>> everything out that is not explicitly required then this should be
>> removed (and added at a later stage, if needed).
>>
>
> I can remove this but I don't really see the harm of keeping this.
> It can even save some time for the person adding the support for CSI
> later.
IMHO removing it would be more consistent with your approach to kick
everything out that is not explicitly needed. I also think that this
approach is common practice.
>>> + .fmt_type = CIF_FMT_TYPE_YUV,
>>> + .field = V4L2_FIELD_NONE,
>>> + }, {
>>> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
>>> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
>>> + CIF_FORMAT_YUV_INPUT_ORDER_YUYV,
>>> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
>>> + .fmt_type = CIF_FMT_TYPE_YUV,
>>> + .field = V4L2_FIELD_INTERLACED,
>>> + }, {
>>> + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
>>> + .dvp_fmt_val = CIF_FORMAT_YUV_INPUT_422 |
>>> + CIF_FORMAT_YUV_INPUT_ORDER_YVYU,
>>> + .csi_fmt_val = CIF_CSI_WRDDR_TYPE_YUV422,
>>> + .fmt_type = CIF_FMT_TYPE_YUV,
>>> + .field = V4L2_FIELD_NONE,
>>
>> What is the difference between this entry (in_fmts[2]) and in_fmts[0]?
>> Similarly, between in_fmts[1] and in_fmts[3]?
>>
>
> Between in_fmts[0] and in_fmts[2] is the order of Y U V components:
> 0 -> YUYV
> 2 -> YVYU
>
> between in_fmts[1] and in_fmts[3]: the same thing:
> 1 -> YUYV
> 3 -> YVYU
Of course. Apparently I was staring at the code for too long. Sorry for
the noise.
> [...]
>>> + input_mode = (remote_info->std == V4L2_STD_NTSC) ?
>>> + CIF_FORMAT_INPUT_MODE_NTSC :
>>> + CIF_FORMAT_INPUT_MODE_PAL;
>>
>> This seems to be an oversimplification. How can one use BT.656 here?
>
> I don't quite understand the question. This is used to configure the
> hardware, i.e., the INPUT_MODE of the format VIP_FOR
>
> bits 4:2
>
> INPUT_MODE Input mode:
>
> 3'b000 : YUV
> 3'b010 : PAL
> 3'b011 : NTSC
> 3'b100 : RAW
> 3'b101 : JPEG
> 3'b110 : MIPI
> Other : invalid
OK, for the RK3568 VICAP the table reads
3'b000: BT601 YUV422
3'b010: BT656 YUV422
3'b100: BT601 RAW
3'b101: SONY RAW
3'b111: BT1120 YUV422
others: Reserved
The PX30 TRM states "Support CCIR656(PAL/NTSC) input" and I think
CCIR-656 equals ITU-R BT.656.
I think in the end the mbus format needs to be evaluated first. If it is
BT.656 AND if the subdevice supports TV standards, the decision between
PAL and NTSC can be made.
>> (Aren't you using BT.656 as mbus format between your video decoder and
>> the PX30 VIP?)
>
> I look into this. I will probably need to add this.
I think it would help if your device tree example in the YAML
documentation contained the complete setup (vip + video decoder chip).
>> You should not assume that the remote is capable of any TV standards
>> (this statement holds for the driver in general).
>>
>
> But this is the support I am adding right now, for cif with a SDTV
> decoder capable of TV standards. This statement will need to be
> changed when support for sensors is added.
>
>>> + mbus_flags = remote_info->mbus.bus.parallel.flags;
>>> + href_pol = (mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) ?
>>> + 0 : CIF_FORMAT_HSY_LOW_ACTIVE;
>>> + vsync_pol = (mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) ?
>>> + CIF_FORMAT_VSY_HIGH_ACTIVE : 0;
>>> +
>>> + val = vsync_pol | href_pol | input_mode | stream->cif_fmt_out->fmt_val |
>>> + stream->cif_fmt_in->dvp_fmt_val | xfer_mode | yc_swap;
>>> +void cif_set_default_format(struct cif_device *cif_dev)
>>> +{
>>> + struct cif_stream *stream = &cif_dev->stream;
>>> + struct v4l2_pix_format pix;
>>> +
>>> + cif_dev->remote.std = V4L2_STD_NTSC;
>>
>> Not every subdevice supports TV standards. Is this really reasonable?
>>
>
> For the support I am adding right now it is reasonable but for future
> support it needs to be changed.
OK then I guess I'll need to change that in my first batch of changes :-)
>>> +
>>> + pix.pixelformat = V4L2_PIX_FMT_NV12;
>>> + pix.width = CIF_DEFAULT_WIDTH;
>>> + pix.height = CIF_DEFAULT_HEIGHT;
>>> +
>>> + cif_set_fmt(stream, &pix);
>>> +}
>>> +
>>> +static int cif_enum_input(struct file *file, void *priv,
>>> + struct v4l2_input *input)
>>> +{
>>> + struct cif_stream *stream = video_drvdata(file);
>>> + struct v4l2_subdev *sd = stream->cifdev->remote.sd;
>>> + int ret;
>>> +
>>> + if (input->index > 0)
>>> + return -EINVAL;
>>> +
>>> + ret = v4l2_subdev_call(sd, video, g_input_status, &input->status);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + strscpy(input->name, "Camera", sizeof(input->name));
>>> + input->type = V4L2_INPUT_TYPE_CAMERA;
>>
>> Wait, we are a camera in any case? How does this fit together with your
>> video decoder setup?
>>
>
> Yes the video decoder is attached to a camera.
>
> From the kernel documentation:
> https://docs.kernel.org/userspace-api/media/v4l/vidioc-enuminput.html?highlight=v4l2_input_type_camera
> --------------------------------------------------------------------------------
> V4L2_INPUT_TYPE_CAMERA
> Any non-tuner video input, for example Composite Video, S-Video, HDMI, camera
> sensor. The naming as _TYPE_CAMERA is historical, today we would have called it
> _TYPE_VIDEO.
> --------------------------------------------------------------------------------
Huh, learned something. Thanks for the pointer!
>>> + input->std = stream->vdev.tvnorms;
>>> + input->capabilities = V4L2_IN_CAP_STD;
>>
>> Not every subdevice supports TV standards. Is this really reasonable?
>>
>
> see above answer.
OK. I think I can submit a series that makes the driver more general
based on your v11. Looking forward to that ;-)
Thanks and best regards,
Michael
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface
2023-11-13 15:28 ` Mehdi Djait
@ 2023-11-13 16:05 ` Michael Riesch
2023-11-15 9:16 ` Mehdi Djait
0 siblings, 1 reply; 20+ messages in thread
From: Michael Riesch @ 2023-11-13 16:05 UTC (permalink / raw)
To: Mehdi Djait
Cc: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt, linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski
Hi Mehdi,
On 11/13/23 16:28, Mehdi Djait wrote:
> Hi Michael,
>
> On Fri, Nov 10, 2023 at 03:33:34PM +0100, Michael Riesch wrote:
>> Hi Mehdi,
>>
>> Sorry, forgot one thing:
>>
>> On 11/8/23 17:38, Mehdi Djait wrote:
>>> +static int cif_subdev_notifier(struct cif_device *cif_dev)
>>> +{
>>> + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
>>> + struct device *dev = cif_dev->dev;
>>> + struct v4l2_async_connection *asd;
>>> + struct v4l2_fwnode_endpoint vep = {
>>> + .bus_type = V4L2_MBUS_PARALLEL,
>>
>> This is surprising. I had to set this to V4L2_MBUS_UNKNOWN, otherwise
>> v4l2_fwnode_endpoint_parse would yield -ENXIO, which indicates a bus
>> type mismatch. Does this really work for your (BT.656, right?) setup?
>>
>
> Yes it works.
>
>> I think we should get the bus type from the device tree, right?
>>
>
> I am looking into this.
>
>> Thanks and best regards,
>> Michael
>>
>
> I assume you have a "bus-type = <MEDIA_BUS_TYPE_BT656>;" in the device
> tree definition of your endpoint ? This caused the mismatch as the
> v4l2_fwnode_endpoint is set to PARALLEL
Yes that's correct.
The documentation is quite sparse here, but I would guess that the PX30
VIP accepts parallel data without embedded syncs (=
MEDIA_BUS_TYPE_PARALLEL) as well as parallel data with embedded syncs (=
MEDIA_BUS_TYPE_BT656). If this is actually the case, I think we should
put V4L2_MBUS_UNKNOWN and let the device tree decide.
We can be sure, however, that the PX30 VIP supports BT.656, so I guess
the safe approach would be to use .bus_type = V4L2_MBUS_BT656.
What do you think?
Best regards,
Michael
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-10 18:23 ` Conor Dooley
@ 2023-11-13 16:54 ` Paul Kocialkowski
2023-11-14 17:51 ` Conor Dooley
0 siblings, 1 reply; 20+ messages in thread
From: Paul Kocialkowski @ 2023-11-13 16:54 UTC (permalink / raw)
To: Conor Dooley
Cc: Krzysztof Kozlowski, Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt, linux-media,
devicetree, linux-kernel, thomas.petazzoni, alexandre.belloni,
maxime.chevallier, michael.riesch
[-- Attachment #1: Type: text/plain, Size: 5881 bytes --]
Hi Conor,
On Fri 10 Nov 23, 18:23, Conor Dooley wrote:
> On Thu, Nov 09, 2023 at 07:07:27PM +0100, Paul Kocialkowski wrote:
> > On Thu 09 Nov 23, 18:53, Krzysztof Kozlowski wrote:
> > > On 09/11/2023 18:45, Paul Kocialkowski wrote:
> > > > On Thu 09 Nov 23, 17:24, Conor Dooley wrote:
> > > >> On Wed, Nov 08, 2023 at 05:38:56PM +0100, Mehdi Djait wrote:
> > > >>> Add a documentation for the Rockchip Camera Interface binding.
> > > >>>
> > > >>> the name of the file rk3066 is the first Rockchip SoC generation that uses cif
> > > >>> instead of the px30 which is just one of the many iterations of the unit.
> > > >>
> > > >> I think this is becoming ridiculous. You've now removed the compatible
> > > >> for the rk3066 but kept it in the filename. I don't understand the
> > > >> hangup about naming the file after the px30-vip, but naming it after
> > > >> something that is not documented here at all makes no sense to me.
> > > >> Either document the rk3066 properly, or remove all mention of it IMO.
> > > >
> > > > I think the opposite is ridiculous. We have spent some time investigating the
> > > > history of this unit, to find out that RK3066 is the first occurence where
> > > > it exists. Since we want the binding to cover all generations of the same unit
> > > > and give it a name that reflects this, rk3066 is the natural choice that comes
> > > > to mind. As far as I understand, this is the normal thing to do to name
> > > > bindings: name after the earliest known occurence of the unit.
> > > >
> > > > What is the rationale behind naming the file after a generation of the unit
> > > > that happens to be the one introducing the binding? This is neither the first
> > > > nor the last one to include this unit. The binding will be updated later to
> > > > cover other generations. Do we want to rename the file each time an a generation
> > > > earlier than px30 is introduced? That sounds quite ridiculous too.
> > > >
> > > > We've done the research work to give it the most relevant name here.
> > > > I'd expect some strong arguments not to use it. Can you ellaborate?
> > >
> > > If you do not have rk3066 documented here, it might be added to entirely
> > > different file (for whatever reasons, including that binding would be
> > > quite different than px30). Thus you would have rk3066 in
> > > rockchip,rk3066-cif-added-later.yaml and px30 in rockchip,rk3066-cif.yaml
> >
> > As far as I could see we generally manage to include support for different
> > hardware setups in the same binding document using conditionals on the
> > compatible, so this feels a bit far-fetched.
> >
> > Of course you're the maintainer and have significantly more experience here
> > so there might be a lot that I'm not seeing, but I'm not very convinced by this
> > reasoning to be honest.
> >
> > > Just use the filename matching the compatible. That's what we always
> > > ask. In every review.
> >
> > Yeah and we very often end up with naming that is less than optimal (to stay
> > polite). I'm generally quite appalled by the overall lack of interest that
> > naming gets, as if it was something secondary. Naming is one of the most
> > important and difficult things in our field of work and it needs to be
> > considered with care.
> >
> > This is not just a problem with device-tree, it's a kernel-wide issue that
> > nobody seems to be interested in addressing. I'm quite unhappy to see that when
> > time is spent trying to improve the situation on one particular instance, we are
> > shown the door because it doesn't match what is generally done (and often done
> > wrong).
> >
> > This is definitely a rant. I really want to express this issue loud and clear
> > and encourage everyone to consider it for what it is.
>
> Look chief, I do understand your frustration here, with the seemingly
> arbitrary naming etc. I'm apologise if using the word "ridiculous" earlier
> pissed you off.
Sorry if my reply was a bit harsh too, it was of course nothing personal but
I really felt like this issue was not being considered seriously.
> I'm sure you can similarly understand why we don't want
> to accept either having a compatible for the rk3066-cif in the file,
> when you are not yet sure of the correct constraints, or given your
> interest in naming,
Sure I understand that we don't want to introduce a compatible for which we
don't have a clear idea of the resources/hardware constraints. I fully agree
that there shouldn't be a fallback on a rk3066-cif for the px30.
> why calling it after something that it does not even
> document is misleading.
My opinion is that the two things are separate and that as long as we know
it's the same unit, it should be fine to name the file after the first
generation even if it is not yet described in the binding. I find that it's a
bit unusual, yes, but I wouldn't go as far as calling it confusing.
The description text can clearly mention that the first occurrence of the unit
is in the RK3066 and thus it will be clear why the file is named after it.
Also people looking for binding documentation for the px30 vip will still easily
find it with the usual git grep. So I don't really see any serious downside.
> Ultimately, I don't care what the file ends up being called when there
> are multiple devices documented in it. I'd ack a patch renaming to the
> œriginal incarnation of the IP when the documentation for that IP is
> added without a second thought.
That would be agreeable to me if my proposal still ends up feeling unreasonable
to you. But I might very well take you at your word since I ended up purchasing
a RK3066 board in a moment of weakness last week.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-13 16:54 ` Paul Kocialkowski
@ 2023-11-14 17:51 ` Conor Dooley
2023-11-15 15:07 ` Paul Kocialkowski
0 siblings, 1 reply; 20+ messages in thread
From: Conor Dooley @ 2023-11-14 17:51 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Krzysztof Kozlowski, Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt, linux-media,
devicetree, linux-kernel, thomas.petazzoni, alexandre.belloni,
maxime.chevallier, michael.riesch
[-- Attachment #1: Type: text/plain, Size: 679 bytes --]
On Mon, Nov 13, 2023 at 05:54:08PM +0100, Paul Kocialkowski wrote:
> > Ultimately, I don't care what the file ends up being called when there
> > are multiple devices documented in it. I'd ack a patch renaming to the
> > œriginal incarnation of the IP when the documentation for that IP is
> > added without a second thought.
>
> That would be agreeable to me if my proposal still ends up feeling unreasonable
> to you. But I might very well take you at your word since I ended up purchasing
> a RK3066 board in a moment of weakness last week.
The ideal outcome I suppose would be documenting both variants. If
you've gone ahead and bought one, give that a go.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface
2023-11-13 16:05 ` Michael Riesch
@ 2023-11-15 9:16 ` Mehdi Djait
0 siblings, 0 replies; 20+ messages in thread
From: Mehdi Djait @ 2023-11-15 9:16 UTC (permalink / raw)
To: Michael Riesch
Cc: mchehab, heiko, hverkuil-cisco, krzysztof.kozlowski+dt, robh+dt,
conor+dt, linux-media, devicetree, linux-kernel, thomas.petazzoni,
alexandre.belloni, maxime.chevallier, paul.kocialkowski
On Mon, Nov 13, 2023 at 05:05:12PM +0100, Michael Riesch wrote:
> Hi Mehdi,
>
> On 11/13/23 16:28, Mehdi Djait wrote:
> > Hi Michael,
> >
> > On Fri, Nov 10, 2023 at 03:33:34PM +0100, Michael Riesch wrote:
> >> Hi Mehdi,
> >>
> >> Sorry, forgot one thing:
> >>
> >> On 11/8/23 17:38, Mehdi Djait wrote:
> >>> +static int cif_subdev_notifier(struct cif_device *cif_dev)
> >>> +{
> >>> + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
> >>> + struct device *dev = cif_dev->dev;
> >>> + struct v4l2_async_connection *asd;
> >>> + struct v4l2_fwnode_endpoint vep = {
> >>> + .bus_type = V4L2_MBUS_PARALLEL,
> >>
> >> This is surprising. I had to set this to V4L2_MBUS_UNKNOWN, otherwise
> >> v4l2_fwnode_endpoint_parse would yield -ENXIO, which indicates a bus
> >> type mismatch. Does this really work for your (BT.656, right?) setup?
> >>
> >
> > Yes it works.
> >
> >> I think we should get the bus type from the device tree, right?
> >>
> >
> > I am looking into this.
> >
> >> Thanks and best regards,
> >> Michael
> >>
> >
> > I assume you have a "bus-type = <MEDIA_BUS_TYPE_BT656>;" in the device
> > tree definition of your endpoint ? This caused the mismatch as the
> > v4l2_fwnode_endpoint is set to PARALLEL
>
> Yes that's correct.
>
> The documentation is quite sparse here, but I would guess that the PX30
> VIP accepts parallel data without embedded syncs (=
> MEDIA_BUS_TYPE_PARALLEL) as well as parallel data with embedded syncs (=
> MEDIA_BUS_TYPE_BT656). If this is actually the case, I think we should
> put V4L2_MBUS_UNKNOWN and let the device tree decide.
Yes, I will do this.
>
> We can be sure, however, that the PX30 VIP supports BT.656, so I guess
> the safe approach would be to use .bus_type = V4L2_MBUS_BT656.
>
> What do you think?
I agree
--
Kind Regards
Mehdi Djait
>
> Best regards,
> Michael
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-14 17:51 ` Conor Dooley
@ 2023-11-15 15:07 ` Paul Kocialkowski
2023-11-15 20:16 ` Conor Dooley
0 siblings, 1 reply; 20+ messages in thread
From: Paul Kocialkowski @ 2023-11-15 15:07 UTC (permalink / raw)
To: Conor Dooley
Cc: Krzysztof Kozlowski, Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt, linux-media,
devicetree, linux-kernel, thomas.petazzoni, alexandre.belloni,
maxime.chevallier, michael.riesch
[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]
Hi,
On Tue 14 Nov 23, 17:51, Conor Dooley wrote:
> On Mon, Nov 13, 2023 at 05:54:08PM +0100, Paul Kocialkowski wrote:
>
> > > Ultimately, I don't care what the file ends up being called when there
> > > are multiple devices documented in it. I'd ack a patch renaming to the
> > > œriginal incarnation of the IP when the documentation for that IP is
> > > added without a second thought.
> >
> > That would be agreeable to me if my proposal still ends up feeling unreasonable
> > to you. But I might very well take you at your word since I ended up purchasing
> > a RK3066 board in a moment of weakness last week.
>
> The ideal outcome I suppose would be documenting both variants. If
> you've gone ahead and bought one, give that a go.
Yeah I'll try to do that eventually, but we really want to have this series
merged as soon as possible. So it wouldn't be reasonable for us to wait for
RK3066 support.
What's your final decision for now: is it okay to keep the file named
rockchip,rk3066-cif.yaml (without this compatible in the file) or do you still
want it called rockchip,px30-vip.yaml?
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF
2023-11-15 15:07 ` Paul Kocialkowski
@ 2023-11-15 20:16 ` Conor Dooley
0 siblings, 0 replies; 20+ messages in thread
From: Conor Dooley @ 2023-11-15 20:16 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Krzysztof Kozlowski, Mehdi Djait, mchehab, heiko, hverkuil-cisco,
krzysztof.kozlowski+dt, robh+dt, conor+dt, linux-media,
devicetree, linux-kernel, thomas.petazzoni, alexandre.belloni,
maxime.chevallier, michael.riesch
[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]
On Wed, Nov 15, 2023 at 04:07:07PM +0100, Paul Kocialkowski wrote:
> Hi,
>
> On Tue 14 Nov 23, 17:51, Conor Dooley wrote:
> > On Mon, Nov 13, 2023 at 05:54:08PM +0100, Paul Kocialkowski wrote:
> >
> > > > Ultimately, I don't care what the file ends up being called when there
> > > > are multiple devices documented in it. I'd ack a patch renaming to the
> > > > œriginal incarnation of the IP when the documentation for that IP is
> > > > added without a second thought.
> > >
> > > That would be agreeable to me if my proposal still ends up feeling unreasonable
> > > to you. But I might very well take you at your word since I ended up purchasing
> > > a RK3066 board in a moment of weakness last week.
> >
> > The ideal outcome I suppose would be documenting both variants. If
> > you've gone ahead and bought one, give that a go.
>
> Yeah I'll try to do that eventually, but we really want to have this series
> merged as soon as possible. So it wouldn't be reasonable for us to wait for
> RK3066 support.
>
> What's your final decision for now: is it okay to keep the file named
> rockchip,rk3066-cif.yaml (without this compatible in the file) or do you still
> want it called rockchip,px30-vip.yaml?
I'd still rather it was called after the only compatible in the file and
subsequently renamed, sorry.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-11-15 20:16 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-08 16:38 [PATCH v10 0/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
2023-11-08 16:38 ` [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF Mehdi Djait
2023-11-09 17:24 ` Conor Dooley
2023-11-09 17:45 ` Paul Kocialkowski
2023-11-09 17:53 ` Krzysztof Kozlowski
2023-11-09 18:07 ` Paul Kocialkowski
2023-11-10 18:23 ` Conor Dooley
2023-11-13 16:54 ` Paul Kocialkowski
2023-11-14 17:51 ` Conor Dooley
2023-11-15 15:07 ` Paul Kocialkowski
2023-11-15 20:16 ` Conor Dooley
2023-11-08 16:38 ` [PATCH v10 2/3] media: rockchip: Add a driver for Rockchip's camera interface Mehdi Djait
2023-11-10 12:50 ` Michael Riesch
2023-11-13 11:06 ` Mehdi Djait
2023-11-13 15:41 ` Michael Riesch
2023-11-10 14:33 ` Michael Riesch
2023-11-13 15:28 ` Mehdi Djait
2023-11-13 16:05 ` Michael Riesch
2023-11-15 9:16 ` Mehdi Djait
2023-11-08 16:38 ` [PATCH v10 3/3] arm64: dts: rockchip: Add the " Mehdi Djait
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).