2024-02-11 19:03:56

by Mehdi Djait

[permalink] [raw]
Subject: [RESEND Patch v13 0/3] media: rockchip: Add a driver for Rockchip's camera interface

Hello everyone,

V13 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_stage, base-commit: 8c64f4cdf4e6cc5682c52523713af8c39c94e6d5

V12 => V13:
cif/cif-capture.c
- changed the vb2_queue member min_buffer_needed to min_queued_buffers

V11 => V12:
cif/cif-capture.c cif/cif-dev.c cif/cif-common.h cif/cif-capture.h:
- changed the name of the files to add "cif-"
- made use of the reset array in the pmruntime suspend and resume
functions
- made the subdev stop before the cif in cif_stop_streaming()
- removed pinctrl_pm_select_sleep_state()
- removed cif_soft_reset()
- changed the dt-binding filename in the MAINTAINERS file
- removed remote_info->mbus
rockchip,px30-vip.yaml:
- removed the undocumented compatible in the example

V10 => V11:
cif/capture.c cif/dev.c cif/common.h cif/capture.h:
- removed the csi_fmt_val and all the CSI reg defines
- removed the setting of buffer numbers in the queue_setup vb2_ops
callback
- changed the v4l2_fwnode_endpoint declaration to V4L2_MBUS_UNKNOWN:
letting the device tree decide which bus is being used
- split dev.h into common.h and capture.h

rockchip,px30-vip.yaml:
- renamed rockchip,rk3066-cif.yaml back to rockchip,px30-vip.yaml as
suggested by Conor
- added the description of the port's endpoint bus-type property
- extended the example to include the definition of the corresponding
video-decoder

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/[email protected]/

I used v4l-utils with HEAD: commit db9478a91120dccc18d1388fe9b812567e33b6bb

# v4l2-compliance
v4l2-compliance 1.27.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.7.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.7.0
Hardware revision: 0x00000000 (0)
Driver version : 6.7.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 CREATE_BUFS maximum buffers: OK
test VIDIOC_EXPBUF: OK
test Requests: OK (Not Supported)

Total for rockchip-cif device /dev/video0: 47, Succeeded: 47, 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 px30 camera interface

.../bindings/media/rockchip,px30-vip.yaml | 123 ++
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 | 14 +
drivers/media/platform/rockchip/cif/Makefile | 3 +
.../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
.../media/platform/rockchip/cif/cif-capture.h | 20 +
.../media/platform/rockchip/cif/cif-common.h | 128 ++
drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
.../media/platform/rockchip/cif/cif-regs.h | 127 ++
12 files changed, 1855 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.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/cif-capture.c
create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h

--
2.43.0



2024-02-11 19:04:04

by Mehdi Djait

[permalink] [raw]
Subject: [RESEND Patch v13 1/3] media: dt-bindings: media: add bindings for Rockchip CIF

From: Mehdi Djait <[email protected]>

Add a documentation for the Rockchip Camera Interface binding.

Reviewed-by: Conor Dooley <[email protected]>
Reviewed-by: Michael Riesch <[email protected]>
Signed-off-by: Mehdi Djait <[email protected]>
Signed-off-by: Mehdi Djait <[email protected]>
---
.../bindings/media/rockchip,px30-vip.yaml | 123 ++++++++++++++++++
1 file changed, 123 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml

diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
new file mode 100644
index 000000000000..6af4a9b6774a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
@@ -0,0 +1,123 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/rockchip,px30-vip.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip Camera Interface (CIF)
+
+maintainers:
+ - Mehdi Djait <[email protected]>
+
+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
+
+ ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+
+ properties:
+ port@0:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description: input port on the parallel interface
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ bus-type:
+ enum: [5, 6]
+
+ required:
+ - bus-type
+
+ required:
+ - port@0
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - ports
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/px30-cru.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/media/video-interfaces.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";
+ power-domains = <&power PX30_PD_VI>;
+ resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
+ reset-names = "axi", "ahb", "pclkin";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ cif_in: endpoint {
+ remote-endpoint = <&tw9900_out>;
+ bus-type = <MEDIA_BUS_TYPE_BT656>;
+ };
+ };
+ };
+ };
+ };
+...
--
2.43.0


2024-02-11 19:04:35

by Mehdi Djait

[permalink] [raw]
Subject: [RESEND Patch v13 3/3] arm64: dts: rockchip: Add the px30 camera interface

From: Mehdi Djait <[email protected]>

The px30 has a video capture component, supporting the BT.656
parallel interface. Add a DT description for it.

Reviewed-by: Michael Riesch <[email protected]>
Reviewed-by: Paul Kocialkowski <[email protected]>
Signed-off-by: Mehdi Djait <[email protected]>
Signed-off-by: Mehdi Djait <[email protected]>
---
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 d0905515399b..a8eb5371235b 100644
--- a/arch/arm64/boot/dts/rockchip/px30.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
@@ -1280,6 +1280,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.43.0


2024-02-11 19:04:44

by Mehdi Djait

[permalink] [raw]
Subject: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

From: Mehdi Djait <[email protected]>

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.

Reviewed-by: Michael Riesch <[email protected]>
Signed-off-by: Mehdi Djait <[email protected]>
Signed-off-by: Mehdi Djait <[email protected]>
---
MAINTAINERS | 7 +
drivers/media/platform/rockchip/Kconfig | 1 +
drivers/media/platform/rockchip/Makefile | 1 +
drivers/media/platform/rockchip/cif/Kconfig | 14 +
drivers/media/platform/rockchip/cif/Makefile | 3 +
.../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
.../media/platform/rockchip/cif/cif-capture.h | 20 +
.../media/platform/rockchip/cif/cif-common.h | 128 ++
drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
.../media/platform/rockchip/cif/cif-regs.h | 127 ++
10 files changed, 1720 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/cif-capture.c
create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1a89e0d2ac61..989a39c94eac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18882,6 +18882,13 @@ F: Documentation/ABI/*/sysfs-driver-hid-roccat*
F: drivers/hid/hid-roccat*
F: include/linux/hid-roccat*

+ROCKCHIP CIF DRIVER
+M: Mehdi Djait <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
+F: drivers/media/platform/rockchip/cif/
+
ROCKCHIP CRYPTO DRIVERS
M: Corentin Labbe <[email protected]>
L: [email protected]
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..1dfe9a167341
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/Kconfig
@@ -0,0 +1,14 @@
+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 MEDIA_CONTROLLER
+ 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..02a88f17d153
--- /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 += cif-dev.o cif-capture.o
diff --git a/drivers/media/platform/rockchip/cif/cif-capture.c b/drivers/media/platform/rockchip/cif/cif-capture.c
new file mode 100644
index 000000000000..2c7716684de0
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/cif-capture.c
@@ -0,0 +1,1111 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Rockchip CIF Camera Interface Driver
+ *
+ * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
+ * Copyright (C) 2020 Maxime Chevallier <[email protected]>
+ * Copyright (C) 2023 Mehdi Djait <[email protected]>
+ */
+
+#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 "cif-capture.h"
+#include "cif-common.h"
+#include "cif-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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .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,
+ .fmt_type = CIF_FMT_TYPE_RAW,
+ .field = V4L2_FIELD_NONE,
+ }, {
+ .mbus_code = MEDIA_BUS_FMT_RGB888_1X24,
+ .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,
+ .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,
+ .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,
+ .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;
+
+ if (*num_planes)
+ return sizes[0] < stream->pix.sizeimage ? -EINVAL : 0;
+
+ *num_planes = 1;
+ sizes[0] = stream->pix.sizeimage;
+
+ 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 = cif_dev->remote.sd;
+ int ret;
+
+ v4l2_subdev_call(sd, video, s_stream, 0);
+
+ stream->stopping = true;
+ ret = wait_event_timeout(stream->wq_stopped,
+ !stream->stopping,
+ msecs_to_jiffies(1000));
+ if (!ret)
+ cif_stream_stop(stream);
+
+ 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, fmt_type, xfer_mode = 0;
+ struct cif_device *cif_dev = stream->cifdev;
+ struct cif_remote *remote_info = &cif_dev->remote;
+ int ret;
+ u32 input_mode;
+
+ stream->frame_idx = 0;
+ stream->frame_phase = 0;
+
+ fmt_type = stream->cif_fmt_in->fmt_type;
+ input_mode = (remote_info->std == V4L2_STD_NTSC) ?
+ CIF_FORMAT_INPUT_MODE_NTSC :
+ CIF_FORMAT_INPUT_MODE_PAL;
+
+ val = input_mode | stream->cif_fmt_out->fmt_val |
+ stream->cif_fmt_in->dvp_fmt_val | xfer_mode;
+ 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_resume_and_get(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);
+ if (!stream->cif_fmt_in)
+ goto runtime_put;
+
+ ret = cif_stream_start(stream);
+ if (ret < 0)
+ goto stop_stream;
+
+ ret = v4l2_subdev_call(sd, video, s_stream, 1);
+ if (ret < 0)
+ goto stop_stream;
+
+ return 0;
+
+stop_stream:
+ cif_stream_stop(stream);
+runtime_put:
+ pm_runtime_put(cif_dev->dev);
+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_queued_buffers = 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 && ret != -EOPNOTSUPP)
+ 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_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_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_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/cif-capture.h b/drivers/media/platform/rockchip/cif/cif-capture.h
new file mode 100644
index 000000000000..0f457693a7a5
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/cif-capture.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Rockchip CIF Driver
+ *
+ * Copyright (C) 2023 Mehdi Djait <[email protected]>
+ */
+
+#ifndef _CIF_CAPTURE_H
+#define _CIF_CAPTURE_H
+
+struct cif_device;
+
+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);
+
+#endif
diff --git a/drivers/media/platform/rockchip/cif/cif-common.h b/drivers/media/platform/rockchip/cif/cif-common.h
new file mode 100644
index 000000000000..89ed9dd6f36d
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/cif-common.h
@@ -0,0 +1,128 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Rockchip CIF Driver
+ *
+ * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
+ * Copyright (C) 2023 Mehdi Djait <[email protected]>
+ */
+
+#ifndef _CIF_COMMON_H
+#define _CIF_COMMON_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_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;
+ 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;
+ 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];
+
+ /* Lock used by the V4L core. */
+ 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);
+}
+
+#endif
diff --git a/drivers/media/platform/rockchip/cif/cif-dev.c b/drivers/media/platform/rockchip/cif/cif-dev.c
new file mode 100644
index 000000000000..660e28397916
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/cif-dev.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Rockchip CIF Camera Interface Driver
+ *
+ * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
+ * Copyright (C) 2020 Maxime Chevallier <[email protected]>
+ * Copyright (C) 2023 Mehdi Djait <[email protected]>
+ */
+
+#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 "cif-capture.h"
+#include "cif-common.h"
+#include "cif-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_UNKNOWN,
+ };
+ 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 -ENODEV;
+
+ ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+ if (ret)
+ goto complete;
+
+ if (vep.bus_type != V4L2_MBUS_BT656 &&
+ vep.bus_type != V4L2_MBUS_PARALLEL) {
+ v4l2_err(&cif_dev->v4l2_dev, "unsupported bus type\n");
+ goto complete;
+ }
+
+ asd = v4l2_async_nf_add_fwnode_remote(ntf, ep,
+ struct v4l2_async_connection);
+ if (IS_ERR(asd)) {
+ ret = PTR_ERR(asd);
+ goto complete;
+ }
+
+ ntf->ops = &subdev_notifier_ops;
+
+ ret = v4l2_async_nf_register(ntf);
+ if (ret)
+ v4l2_async_nf_cleanup(ntf);
+
+complete:
+ fwnode_handle_put(ep);
+
+ return ret;
+}
+
+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_plat_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct v4l2_device *v4l2_dev;
+ struct cif_device *cif_dev;
+ struct resource *res;
+ 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;
+
+ 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);
+
+ 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_set_suspended(&pdev->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);
+
+ media_device_unregister(&cif_dev->media_dev);
+ v4l2_device_unregister(&cif_dev->v4l2_dev);
+ cif_unregister_stream_vdev(cif_dev);
+
+ pm_runtime_disable(&pdev->dev);
+
+ return 0;
+}
+
+static int 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);
+
+ reset_control_assert(cif_dev->cif_rst);
+
+ return 0;
+}
+
+static int cif_runtime_resume(struct device *dev)
+{
+ struct cif_device *cif_dev = dev_get_drvdata(dev);
+ int ret;
+
+ ret = reset_control_deassert(cif_dev->cif_rst);
+ if (ret) {
+ dev_err(dev, "failed to deassert reset\n");
+ return ret;
+ }
+
+ ret = clk_bulk_prepare_enable(cif_dev->match_data->clks_num,
+ cif_dev->match_data->clks);
+ if (ret) {
+ dev_err(dev, "failed to enable module clock\n");
+ goto err_reset;
+ }
+
+ return 0;
+
+err_reset:
+ reset_control_assert(cif_dev->cif_rst);
+
+ return ret;
+}
+
+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/cif-regs.h b/drivers/media/platform/rockchip/cif/cif-regs.h
new file mode 100644
index 000000000000..b8500f0a9ac1
--- /dev/null
+++ b/drivers/media/platform/rockchip/cif/cif-regs.h
@@ -0,0 +1,127 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Rockchip CIF Driver
+ *
+ * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
+ * Copyright (C) 2023 Mehdi Djait <[email protected]>
+ */
+
+#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
+
+#endif
--
2.43.0


2024-02-13 12:05:59

by Sakari Ailus

[permalink] [raw]
Subject: Re: [RESEND Patch v13 1/3] media: dt-bindings: media: add bindings for Rockchip CIF

Hi Mehdi,

Thanks for the patchset.

On Sun, Feb 11, 2024 at 08:03:30PM +0100, Mehdi Djait wrote:
> From: Mehdi Djait <[email protected]>
>
> Add a documentation for the Rockchip Camera Interface binding.
>
> Reviewed-by: Conor Dooley <[email protected]>
> Reviewed-by: Michael Riesch <[email protected]>
> Signed-off-by: Mehdi Djait <[email protected]>
> Signed-off-by: Mehdi Djait <[email protected]>
> ---
> .../bindings/media/rockchip,px30-vip.yaml | 123 ++++++++++++++++++
> 1 file changed, 123 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> new file mode 100644
> index 000000000000..6af4a9b6774a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/rockchip,px30-vip.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip Camera Interface (CIF)
> +
> +maintainers:
> + - Mehdi Djait <[email protected]>
> +
> +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
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + unevaluatedProperties: false
> + description: input port on the parallel interface
> +
> + properties:
> + endpoint:
> + $ref: video-interfaces.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + bus-type:
> + enum: [5, 6]
> +
> + required:
> + - bus-type

What about the vsync-active, hsync-active and data-active properties?
Aren't they relevant for this device? Are there default values? This should
be part of the bindings for the device, shouldn't it?

> +
> + required:
> + - port@0
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/px30-cru.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/media/video-interfaces.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";
> + power-domains = <&power PX30_PD_VI>;
> + resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
> + reset-names = "axi", "ahb", "pclkin";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + cif_in: endpoint {
> + remote-endpoint = <&tw9900_out>;
> + bus-type = <MEDIA_BUS_TYPE_BT656>;
> + };
> + };
> + };
> + };
> + };
> +...

--
Kind regards,

Sakari Ailus

2024-02-13 13:38:02

by Sakari Ailus

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi Mahdi,

On Sun, Feb 11, 2024 at 08:03:31PM +0100, Mehdi Djait wrote:
> From: Mehdi Djait <[email protected]>
>
> 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.
>
> Reviewed-by: Michael Riesch <[email protected]>
> Signed-off-by: Mehdi Djait <[email protected]>
> Signed-off-by: Mehdi Djait <[email protected]>
> ---
> MAINTAINERS | 7 +
> drivers/media/platform/rockchip/Kconfig | 1 +
> drivers/media/platform/rockchip/Makefile | 1 +
> drivers/media/platform/rockchip/cif/Kconfig | 14 +
> drivers/media/platform/rockchip/cif/Makefile | 3 +
> .../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
> .../media/platform/rockchip/cif/cif-capture.h | 20 +
> .../media/platform/rockchip/cif/cif-common.h | 128 ++
> drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
> .../media/platform/rockchip/cif/cif-regs.h | 127 ++
> 10 files changed, 1720 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/cif-capture.c
> create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
> create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
> create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
> create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1a89e0d2ac61..989a39c94eac 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18882,6 +18882,13 @@ F: Documentation/ABI/*/sysfs-driver-hid-roccat*
> F: drivers/hid/hid-roccat*
> F: include/linux/hid-roccat*
>
> +ROCKCHIP CIF DRIVER
> +M: Mehdi Djait <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> +F: drivers/media/platform/rockchip/cif/
> +
> ROCKCHIP CRYPTO DRIVERS
> M: Corentin Labbe <[email protected]>
> L: [email protected]
> 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..1dfe9a167341
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/Kconfig
> @@ -0,0 +1,14 @@
> +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 MEDIA_CONTROLLER
> + 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..02a88f17d153
> --- /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 += cif-dev.o cif-capture.o
> diff --git a/drivers/media/platform/rockchip/cif/cif-capture.c b/drivers/media/platform/rockchip/cif/cif-capture.c
> new file mode 100644
> index 000000000000..2c7716684de0
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/cif-capture.c
> @@ -0,0 +1,1111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Camera Interface Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2020 Maxime Chevallier <[email protected]>
> + * Copyright (C) 2023 Mehdi Djait <[email protected]>
> + */
> +
> +#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 "cif-capture.h"
> +#include "cif-common.h"
> +#include "cif-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[] = {

const

> + {
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .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,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_RGB888_1X24,
> + .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,
> + .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,
> + .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,
> + .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;

unsigned int?

> +
> + 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;
> +
> + if (*num_planes)
> + return sizes[0] < stream->pix.sizeimage ? -EINVAL : 0;
> +
> + *num_planes = 1;
> + sizes[0] = stream->pix.sizeimage;
> +
> + 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;

unsigned int

> +
> + 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 = cif_dev->remote.sd;
> + int ret;
> +
> + v4l2_subdev_call(sd, video, s_stream, 0);
> +
> + stream->stopping = true;
> + ret = wait_event_timeout(stream->wq_stopped,
> + !stream->stopping,
> + msecs_to_jiffies(1000));
> + if (!ret)
> + cif_stream_stop(stream);
> +
> + 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, fmt_type, xfer_mode = 0;
> + struct cif_device *cif_dev = stream->cifdev;
> + struct cif_remote *remote_info = &cif_dev->remote;
> + int ret;
> + u32 input_mode;
> +
> + stream->frame_idx = 0;
> + stream->frame_phase = 0;
> +
> + fmt_type = stream->cif_fmt_in->fmt_type;
> + input_mode = (remote_info->std == V4L2_STD_NTSC) ?
> + CIF_FORMAT_INPUT_MODE_NTSC :
> + CIF_FORMAT_INPUT_MODE_PAL;
> +
> + val = input_mode | stream->cif_fmt_out->fmt_val |
> + stream->cif_fmt_in->dvp_fmt_val | xfer_mode;
> + 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_resume_and_get(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);

You should use the format on the local pad, not get it from a remote
sub-device.

Link validation ensures they're the same (or at least compatible).

Speaking of which---you don't have link_validate callbacks set for the
sub-device. See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2.c for an
example.

> + if (!stream->cif_fmt_in)
> + goto runtime_put;
> +
> + ret = cif_stream_start(stream);
> + if (ret < 0)
> + goto stop_stream;
> +
> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> + if (ret < 0)
> + goto stop_stream;
> +
> + return 0;
> +
> +stop_stream:
> + cif_stream_stop(stream);
> +runtime_put:
> + pm_runtime_put(cif_dev->dev);
> +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_queued_buffers = 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);

The user space is responsible for controlling the sensor i.e. you shouldn't
call set_fmt sub-device op from this driver.

As the driver is MC-enabled, generally the sub-devices act as a control
interface and the V4L2 video nodes are a data interface.

> + 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 && ret != -EOPNOTSUPP)
> + 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_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_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_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)

I'd drop the name "stream" in this context as it's different from what
streams API uses it for.

> +{
> + 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/cif-capture.h b/drivers/media/platform/rockchip/cif/cif-capture.h
> new file mode 100644
> index 000000000000..0f457693a7a5
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/cif-capture.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2023 Mehdi Djait <[email protected]>
> + */
> +
> +#ifndef _CIF_CAPTURE_H
> +#define _CIF_CAPTURE_H
> +
> +struct cif_device;
> +
> +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);
> +
> +#endif
> diff --git a/drivers/media/platform/rockchip/cif/cif-common.h b/drivers/media/platform/rockchip/cif/cif-common.h
> new file mode 100644
> index 000000000000..89ed9dd6f36d
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/cif-common.h
> @@ -0,0 +1,128 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2023 Mehdi Djait <[email protected]>
> + */
> +
> +#ifndef _CIF_COMMON_H
> +#define _CIF_COMMON_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_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;
> + 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;
> + 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];
> +
> + /* Lock used by the V4L core. */
> + 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);
> +}
> +
> +#endif
> diff --git a/drivers/media/platform/rockchip/cif/cif-dev.c b/drivers/media/platform/rockchip/cif/cif-dev.c
> new file mode 100644
> index 000000000000..660e28397916
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/cif-dev.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Camera Interface Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2020 Maxime Chevallier <[email protected]>
> + * Copyright (C) 2023 Mehdi Djait <[email protected]>
> + */
> +
> +#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 "cif-capture.h"
> +#include "cif-common.h"
> +#include "cif-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_UNKNOWN,
> + };
> + 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 -ENODEV;
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + if (ret)
> + goto complete;
> +
> + if (vep.bus_type != V4L2_MBUS_BT656 &&
> + vep.bus_type != V4L2_MBUS_PARALLEL) {
> + v4l2_err(&cif_dev->v4l2_dev, "unsupported bus type\n");
> + goto complete;
> + }
> +
> + asd = v4l2_async_nf_add_fwnode_remote(ntf, ep,
> + struct v4l2_async_connection);
> + if (IS_ERR(asd)) {
> + ret = PTR_ERR(asd);
> + goto complete;
> + }
> +
> + ntf->ops = &subdev_notifier_ops;
> +
> + ret = v4l2_async_nf_register(ntf);
> + if (ret)
> + v4l2_async_nf_cleanup(ntf);
> +
> +complete:
> + fwnode_handle_put(ep);
> +
> + return ret;
> +}
> +
> +static struct clk_bulk_data px30_cif_clks[] = {

const?

> + { .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_plat_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct v4l2_device *v4l2_dev;
> + struct cif_device *cif_dev;
> + struct resource *res;
> + 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;
> +
> + 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);
> +
> + 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_set_suspended(&pdev->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);

media_device_cleanup(...);

> + return ret;
> +}
> +
> +static int cif_plat_remove(struct platform_device *pdev)
> +{
> + struct cif_device *cif_dev = platform_get_drvdata(pdev);
> +
> + media_device_unregister(&cif_dev->media_dev);

media_device_cleanup(...);

> + v4l2_device_unregister(&cif_dev->v4l2_dev);
> + cif_unregister_stream_vdev(cif_dev);
> +
> + pm_runtime_disable(&pdev->dev);
> +
> + return 0;
> +}
> +
> +static int 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);
> +
> + reset_control_assert(cif_dev->cif_rst);
> +
> + return 0;
> +}
> +
> +static int cif_runtime_resume(struct device *dev)
> +{
> + struct cif_device *cif_dev = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = reset_control_deassert(cif_dev->cif_rst);
> + if (ret) {
> + dev_err(dev, "failed to deassert reset\n");
> + return ret;
> + }
> +
> + ret = clk_bulk_prepare_enable(cif_dev->match_data->clks_num,
> + cif_dev->match_data->clks);
> + if (ret) {
> + dev_err(dev, "failed to enable module clock\n");
> + goto err_reset;
> + }
> +
> + return 0;
> +
> +err_reset:
> + reset_control_assert(cif_dev->cif_rst);
> +
> + return ret;
> +}
> +
> +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/cif-regs.h b/drivers/media/platform/rockchip/cif/cif-regs.h
> new file mode 100644
> index 000000000000..b8500f0a9ac1
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/cif-regs.h
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2023 Mehdi Djait <[email protected]>
> + */
> +
> +#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
> +
> +#endif

--
Kind regards,

Sakari Ailus

2024-02-20 17:04:28

by Mehdi Djait

[permalink] [raw]
Subject: Re: [RESEND Patch v13 1/3] media: dt-bindings: media: add bindings for Rockchip CIF

Hi Sakari,

Thank you for the review.

On Tue, Feb 13, 2024 at 12:04:58PM +0000, Sakari Ailus wrote:
> Hi Mehdi,
>
> Thanks for the patchset.
>
> On Sun, Feb 11, 2024 at 08:03:30PM +0100, Mehdi Djait wrote:
> > From: Mehdi Djait <[email protected]>
> >
> > Add a documentation for the Rockchip Camera Interface binding.
> >
> > Reviewed-by: Conor Dooley <[email protected]>
> > Reviewed-by: Michael Riesch <[email protected]>
> > Signed-off-by: Mehdi Djait <[email protected]>
> > Signed-off-by: Mehdi Djait <[email protected]>
> > ---
> > .../bindings/media/rockchip,px30-vip.yaml | 123 ++++++++++++++++++
> > 1 file changed, 123 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > new file mode 100644
> > index 000000000000..6af4a9b6774a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > @@ -0,0 +1,123 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/rockchip,px30-vip.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip Camera Interface (CIF)
> > +
> > +maintainers:
> > + - Mehdi Djait <[email protected]>
> > +
> > +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
> > +
> > + ports:
> > + $ref: /schemas/graph.yaml#/properties/ports
> > +
> > + properties:
> > + port@0:
> > + $ref: /schemas/graph.yaml#/$defs/port-base
> > + unevaluatedProperties: false
> > + description: input port on the parallel interface
> > +
> > + properties:
> > + endpoint:
> > + $ref: video-interfaces.yaml#
> > + unevaluatedProperties: false
> > +
> > + properties:
> > + bus-type:
> > + enum: [5, 6]
> > +
> > + required:
> > + - bus-type
>
> What about the vsync-active, hsync-active and data-active properties?
> Aren't they relevant for this device? Are there default values? This should
> be part of the bindings for the device, shouldn't it?
>

From what I gathered from the Rockchip PX30 TRM and the other
available documents from Rockchip, I will add the following:

diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
index 6af4a9b6774a..6920b0cb0507 100644
--- a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
@@ -69,6 +69,14 @@ properties:
bus-type:
enum: [5, 6]

+ hsync-active:
+ enum: [0, 1]
+ default: 1
+
+ vsync-active:
+ enum: [0, 1]
+ default: 0
+

@dt-maintainers, @Conor does this warrant a drop of the reviewed-by tags
in the V14 ?

--
Kind Regards
Mehdi Djait

2024-02-20 19:31:24

by Sakari Ailus

[permalink] [raw]
Subject: Re: [RESEND Patch v13 1/3] media: dt-bindings: media: add bindings for Rockchip CIF

Hi Mehdi,

On Tue, Feb 20, 2024 at 06:04:10PM +0100, Mehdi Djait wrote:
> Hi Sakari,
>
> Thank you for the review.

You're welcome!

>
> On Tue, Feb 13, 2024 at 12:04:58PM +0000, Sakari Ailus wrote:
> > Hi Mehdi,
> >
> > Thanks for the patchset.
> >
> > On Sun, Feb 11, 2024 at 08:03:30PM +0100, Mehdi Djait wrote:
> > > From: Mehdi Djait <[email protected]>
> > >
> > > Add a documentation for the Rockchip Camera Interface binding.
> > >
> > > Reviewed-by: Conor Dooley <[email protected]>
> > > Reviewed-by: Michael Riesch <[email protected]>
> > > Signed-off-by: Mehdi Djait <[email protected]>
> > > Signed-off-by: Mehdi Djait <[email protected]>
> > > ---
> > > .../bindings/media/rockchip,px30-vip.yaml | 123 ++++++++++++++++++
> > > 1 file changed, 123 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > new file mode 100644
> > > index 000000000000..6af4a9b6774a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > @@ -0,0 +1,123 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/rockchip,px30-vip.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rockchip Camera Interface (CIF)
> > > +
> > > +maintainers:
> > > + - Mehdi Djait <[email protected]>
> > > +
> > > +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
> > > +
> > > + ports:
> > > + $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > + properties:
> > > + port@0:
> > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > + unevaluatedProperties: false
> > > + description: input port on the parallel interface
> > > +
> > > + properties:
> > > + endpoint:
> > > + $ref: video-interfaces.yaml#
> > > + unevaluatedProperties: false
> > > +
> > > + properties:
> > > + bus-type:
> > > + enum: [5, 6]
> > > +
> > > + required:
> > > + - bus-type
> >
> > What about the vsync-active, hsync-active and data-active properties?
> > Aren't they relevant for this device? Are there default values? This should
> > be part of the bindings for the device, shouldn't it?
> >
>
> From what I gathered from the Rockchip PX30 TRM and the other
> available documents from Rockchip, I will add the following:
>
> diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> index 6af4a9b6774a..6920b0cb0507 100644
> --- a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> @@ -69,6 +69,14 @@ properties:
> bus-type:
> enum: [5, 6]
>
> + hsync-active:
> + enum: [0, 1]
> + default: 1
> +
> + vsync-active:
> + enum: [0, 1]
> + default: 0

I'd use the same default for both, whether it's 0 or 1.

> +
>
> @dt-maintainers, @Conor does this warrant a drop of the reviewed-by tags
> in the V14 ?
>

--
Regards,

Sakari Ailus

2024-02-20 20:48:25

by Mehdi Djait

[permalink] [raw]
Subject: Re: [RESEND Patch v13 1/3] media: dt-bindings: media: add bindings for Rockchip CIF

Hi Sakari,

On Tue, Feb 20, 2024 at 07:30:53PM +0000, Sakari Ailus wrote:
> Hi Mehdi,
>
> On Tue, Feb 20, 2024 at 06:04:10PM +0100, Mehdi Djait wrote:
> > Hi Sakari,
> >
> > Thank you for the review.
>
> You're welcome!
>
> >
> > On Tue, Feb 13, 2024 at 12:04:58PM +0000, Sakari Ailus wrote:
> > > Hi Mehdi,
> > >
> > > Thanks for the patchset.
> > >
> > > On Sun, Feb 11, 2024 at 08:03:30PM +0100, Mehdi Djait wrote:
> > > > From: Mehdi Djait <[email protected]>
> > > >
> > > > Add a documentation for the Rockchip Camera Interface binding.
> > > >
> > > > Reviewed-by: Conor Dooley <[email protected]>
> > > > Reviewed-by: Michael Riesch <[email protected]>
> > > > Signed-off-by: Mehdi Djait <[email protected]>
> > > > Signed-off-by: Mehdi Djait <[email protected]>
> > > > ---
> > > > .../bindings/media/rockchip,px30-vip.yaml | 123 ++++++++++++++++++
> > > > 1 file changed, 123 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > > new file mode 100644
> > > > index 000000000000..6af4a9b6774a
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > > @@ -0,0 +1,123 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/rockchip,px30-vip.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Rockchip Camera Interface (CIF)
> > > > +
> > > > +maintainers:
> > > > + - Mehdi Djait <[email protected]>
> > > > +
> > > > +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
> > > > +
> > > > + ports:
> > > > + $ref: /schemas/graph.yaml#/properties/ports
> > > > +
> > > > + properties:
> > > > + port@0:
> > > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > > + unevaluatedProperties: false
> > > > + description: input port on the parallel interface
> > > > +
> > > > + properties:
> > > > + endpoint:
> > > > + $ref: video-interfaces.yaml#
> > > > + unevaluatedProperties: false
> > > > +
> > > > + properties:
> > > > + bus-type:
> > > > + enum: [5, 6]
> > > > +
> > > > + required:
> > > > + - bus-type
> > >
> > > What about the vsync-active, hsync-active and data-active properties?
> > > Aren't they relevant for this device? Are there default values? This should
> > > be part of the bindings for the device, shouldn't it?
> > >
> >
> > From what I gathered from the Rockchip PX30 TRM and the other
> > available documents from Rockchip, I will add the following:
> >
> > diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > index 6af4a9b6774a..6920b0cb0507 100644
> > --- a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > @@ -69,6 +69,14 @@ properties:
> > bus-type:
> > enum: [5, 6]
> >
> > + hsync-active:
> > + enum: [0, 1]
> > + default: 1
> > +
> > + vsync-active:
> > + enum: [0, 1]
> > + default: 0
>
> I'd use the same default for both, whether it's 0 or 1.
>

Is this supposed to be the default value HIGH or LOW if it is not
configured by the driver ? because the manual states the following:

HREF_POL
Href input polarity:
1'b0 : high active
1'b1 : low active
RESET VALUE: 0x0

VSYNC_POL
Vsync input polarity:
1'b0 : low active
1'b1 : high active
RESET VALUE: 0x0

And that's why I chose 1 for hsync and 0 for vsync

--
Kind Regards
Mehdi Djait

2024-02-21 10:58:34

by Sakari Ailus

[permalink] [raw]
Subject: Re: [RESEND Patch v13 1/3] media: dt-bindings: media: add bindings for Rockchip CIF

Hi Mehdi,

On Tue, Feb 20, 2024 at 09:48:06PM +0100, Mehdi Djait wrote:
> Hi Sakari,
>
> On Tue, Feb 20, 2024 at 07:30:53PM +0000, Sakari Ailus wrote:
> > Hi Mehdi,
> >
> > On Tue, Feb 20, 2024 at 06:04:10PM +0100, Mehdi Djait wrote:
> > > Hi Sakari,
> > >
> > > Thank you for the review.
> >
> > You're welcome!
> >
> > >
> > > On Tue, Feb 13, 2024 at 12:04:58PM +0000, Sakari Ailus wrote:
> > > > Hi Mehdi,
> > > >
> > > > Thanks for the patchset.
> > > >
> > > > On Sun, Feb 11, 2024 at 08:03:30PM +0100, Mehdi Djait wrote:
> > > > > From: Mehdi Djait <[email protected]>
> > > > >
> > > > > Add a documentation for the Rockchip Camera Interface binding.
> > > > >
> > > > > Reviewed-by: Conor Dooley <[email protected]>
> > > > > Reviewed-by: Michael Riesch <[email protected]>
> > > > > Signed-off-by: Mehdi Djait <[email protected]>
> > > > > Signed-off-by: Mehdi Djait <[email protected]>
> > > > > ---
> > > > > .../bindings/media/rockchip,px30-vip.yaml | 123 ++++++++++++++++++
> > > > > 1 file changed, 123 insertions(+)
> > > > > create mode 100644 Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..6af4a9b6774a
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > > > @@ -0,0 +1,123 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/media/rockchip,px30-vip.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Rockchip Camera Interface (CIF)
> > > > > +
> > > > > +maintainers:
> > > > > + - Mehdi Djait <[email protected]>
> > > > > +
> > > > > +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
> > > > > +
> > > > > + ports:
> > > > > + $ref: /schemas/graph.yaml#/properties/ports
> > > > > +
> > > > > + properties:
> > > > > + port@0:
> > > > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > > > + unevaluatedProperties: false
> > > > > + description: input port on the parallel interface
> > > > > +
> > > > > + properties:
> > > > > + endpoint:
> > > > > + $ref: video-interfaces.yaml#
> > > > > + unevaluatedProperties: false
> > > > > +
> > > > > + properties:
> > > > > + bus-type:
> > > > > + enum: [5, 6]
> > > > > +
> > > > > + required:
> > > > > + - bus-type
> > > >
> > > > What about the vsync-active, hsync-active and data-active properties?
> > > > Aren't they relevant for this device? Are there default values? This should
> > > > be part of the bindings for the device, shouldn't it?
> > > >
> > >
> > > From what I gathered from the Rockchip PX30 TRM and the other
> > > available documents from Rockchip, I will add the following:
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > index 6af4a9b6774a..6920b0cb0507 100644
> > > --- a/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > +++ b/Documentation/devicetree/bindings/media/rockchip,px30-vip.yaml
> > > @@ -69,6 +69,14 @@ properties:
> > > bus-type:
> > > enum: [5, 6]
> > >
> > > + hsync-active:
> > > + enum: [0, 1]
> > > + default: 1
> > > +
> > > + vsync-active:
> > > + enum: [0, 1]
> > > + default: 0
> >
> > I'd use the same default for both, whether it's 0 or 1.
> >
>
> Is this supposed to be the default value HIGH or LOW if it is not
> configured by the driver ? because the manual states the following:
>
> HREF_POL
> Href input polarity:
> 1'b0 : high active
> 1'b1 : low active
> RESET VALUE: 0x0
>
> VSYNC_POL
> Vsync input polarity:
> 1'b0 : low active
> 1'b1 : high active
> RESET VALUE: 0x0
>
> And that's why I chose 1 for hsync and 0 for vsync

The hardware reset values do not have to reflect defaults in DT bindings.

--
Kind regards,

Sakari Ailus

2024-02-21 17:56:14

by Mehdi Djait

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi Sakari,

Thank you for the review!

On Tue, Feb 13, 2024 at 01:37:39PM +0000, Sakari Ailus wrote:
> Hi Mahdi,
>
> On Sun, Feb 11, 2024 at 08:03:31PM +0100, Mehdi Djait wrote:
> > From: Mehdi Djait <[email protected]>
> >
> > 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.
> >
> > Reviewed-by: Michael Riesch <[email protected]>
> > Signed-off-by: Mehdi Djait <[email protected]>
> > Signed-off-by: Mehdi Djait <[email protected]>
> > ---
> > MAINTAINERS | 7 +
> > drivers/media/platform/rockchip/Kconfig | 1 +
> > drivers/media/platform/rockchip/Makefile | 1 +
> > drivers/media/platform/rockchip/cif/Kconfig | 14 +
> > drivers/media/platform/rockchip/cif/Makefile | 3 +
> > .../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
> > .../media/platform/rockchip/cif/cif-capture.h | 20 +
> > .../media/platform/rockchip/cif/cif-common.h | 128 ++
> > drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
> > .../media/platform/rockchip/cif/cif-regs.h | 127 ++
> > 10 files changed, 1720 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/cif-capture.c
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
> > create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h
> >
> > +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_resume_and_get(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);
>
> You should use the format on the local pad, not get it from a remote
> sub-device.
>
> Link validation ensures they're the same (or at least compatible).
>
> Speaking of which---you don't have link_validate callbacks set for the
> sub-device. See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2.c for an
> example.
>

..

> > + if (!stream->cif_fmt_in)
> > + goto runtime_put;
> > +
> > + ret = cif_stream_start(stream);
> > + if (ret < 0)
> > + goto stop_stream;
> > +
> > + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> > + if (ret < 0)
> > + goto stop_stream;
> > +
> > + return 0;
> > +
> > +stop_stream:
> > + cif_stream_stop(stream);
> > +runtime_put:
> > + pm_runtime_put(cif_dev->dev);
> > +destroy_buf:
> > + cif_return_all_buffers(stream, VB2_BUF_STATE_QUEUED);
> > +
> > + return ret;
> > +}
> > +
> > +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);
>
> The user space is responsible for controlling the sensor i.e. you shouldn't
> call set_fmt sub-device op from this driver.
>
> As the driver is MC-enabled, generally the sub-devices act as a control
> interface and the V4L2 video nodes are a data interface.
>

While this is true for MC-centric (Media Controller) drivers, this driver is
video-node-centric (I mentioned this in the commit msg)

From the Kernel Documentation: https://docs.kernel.org/userspace-api/media/v4l/open.html

1 - The devices that are fully controlled via V4L2 device nodes are called video-node-centric.

2- Note:
A video-node-centric may still provide media-controller and sub-device interfaces as well.
However, in that case the media-controller and the sub-device interfaces are read-only and just
provide information about the device. The actual configuration is done via the video nodes.

> > + if (ret)
> > + return ret;
> > +
> > + cif_update_pix(stream, fmt, pix);
> > + stream->pix = *pix;
> > + stream->cif_fmt_out = fmt;
> > +
> > + return 0;
> > +}

--
Kind Regards
Mehdi Djait

2024-02-27 10:24:16

by Sakari Ailus

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi Mehdi,

On Wed, Feb 21, 2024 at 06:55:54PM +0100, Mehdi Djait wrote:
> Hi Sakari,
>
> Thank you for the review!
>
> On Tue, Feb 13, 2024 at 01:37:39PM +0000, Sakari Ailus wrote:
> > Hi Mahdi,
> >
> > On Sun, Feb 11, 2024 at 08:03:31PM +0100, Mehdi Djait wrote:
> > > From: Mehdi Djait <[email protected]>
> > >
> > > 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.
> > >
> > > Reviewed-by: Michael Riesch <[email protected]>
> > > Signed-off-by: Mehdi Djait <[email protected]>
> > > Signed-off-by: Mehdi Djait <[email protected]>
> > > ---
> > > MAINTAINERS | 7 +
> > > drivers/media/platform/rockchip/Kconfig | 1 +
> > > drivers/media/platform/rockchip/Makefile | 1 +
> > > drivers/media/platform/rockchip/cif/Kconfig | 14 +
> > > drivers/media/platform/rockchip/cif/Makefile | 3 +
> > > .../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
> > > .../media/platform/rockchip/cif/cif-capture.h | 20 +
> > > .../media/platform/rockchip/cif/cif-common.h | 128 ++
> > > drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
> > > .../media/platform/rockchip/cif/cif-regs.h | 127 ++
> > > 10 files changed, 1720 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/cif-capture.c
> > > create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
> > > create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
> > > create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
> > > create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h
> > >
> > > +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_resume_and_get(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);
> >
> > You should use the format on the local pad, not get it from a remote
> > sub-device.
> >
> > Link validation ensures they're the same (or at least compatible).
> >
> > Speaking of which---you don't have link_validate callbacks set for the
> > sub-device. See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2.c for an
> > example.
> >
>
> ...
>
> > > + if (!stream->cif_fmt_in)
> > > + goto runtime_put;
> > > +
> > > + ret = cif_stream_start(stream);
> > > + if (ret < 0)
> > > + goto stop_stream;
> > > +
> > > + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> > > + if (ret < 0)
> > > + goto stop_stream;
> > > +
> > > + return 0;
> > > +
> > > +stop_stream:
> > > + cif_stream_stop(stream);
> > > +runtime_put:
> > > + pm_runtime_put(cif_dev->dev);
> > > +destroy_buf:
> > > + cif_return_all_buffers(stream, VB2_BUF_STATE_QUEUED);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +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);
> >
> > The user space is responsible for controlling the sensor i.e. you shouldn't
> > call set_fmt sub-device op from this driver.
> >
> > As the driver is MC-enabled, generally the sub-devices act as a control
> > interface and the V4L2 video nodes are a data interface.
> >
>
> While this is true for MC-centric (Media Controller) drivers, this driver is
> video-node-centric (I mentioned this in the commit msg)
>
> From the Kernel Documentation:
> https://docs.kernel.org/userspace-api/media/v4l/open.html
>
> 1 - The devices that are fully controlled via V4L2 device nodes are
> called video-node-centric.
>
> 2- Note: A video-node-centric may still provide media-controller and
> sub-device interfaces as well. However, in that case the media-controller
> and the sub-device interfaces are read-only and just provide information
> about the device. The actual configuration is done via the video nodes.

Are you sure you even want to do this?

It'll limit what kind of sensors you can attach to the device and even more
so in the future as we're reworking the sensor APIs to allow better control
of the sensors, using internal pads (that require MC).

There have been some such drivers in the past but many have been already
converted, or in some cases the newer hardware generation uses MC. Keeping
API compatibility is a requirement so you can't just "add support" later
on.

--
Regards,

Sakari Ailus

2024-03-02 11:51:40

by Mehdi Djait

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi Sakari,

On Tue, Feb 27, 2024 at 10:23:46AM +0000, Sakari Ailus wrote:
> Hi Mehdi,
>
> On Wed, Feb 21, 2024 at 06:55:54PM +0100, Mehdi Djait wrote:
> > Hi Sakari,
> >
> > Thank you for the review!
> >
> > On Tue, Feb 13, 2024 at 01:37:39PM +0000, Sakari Ailus wrote:
> > > Hi Mahdi,
> > >
> > > On Sun, Feb 11, 2024 at 08:03:31PM +0100, Mehdi Djait wrote:
> > > > From: Mehdi Djait <[email protected]>
> > > >
> > > > 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.
> > > >
> > > > Reviewed-by: Michael Riesch <[email protected]>
> > > > Signed-off-by: Mehdi Djait <[email protected]>
> > > > Signed-off-by: Mehdi Djait <[email protected]>
> > > > ---
> > > > MAINTAINERS | 7 +
> > > > drivers/media/platform/rockchip/Kconfig | 1 +
> > > > drivers/media/platform/rockchip/Makefile | 1 +
> > > > drivers/media/platform/rockchip/cif/Kconfig | 14 +
> > > > drivers/media/platform/rockchip/cif/Makefile | 3 +
> > > > .../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
> > > > .../media/platform/rockchip/cif/cif-capture.h | 20 +
> > > > .../media/platform/rockchip/cif/cif-common.h | 128 ++
> > > > drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
> > > > .../media/platform/rockchip/cif/cif-regs.h | 127 ++
> > > > 10 files changed, 1720 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/cif-capture.c
> > > > create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
> > > > create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
> > > > create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
> > > > create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h
> > > >
> > > > +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_resume_and_get(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);
> > >
> > > You should use the format on the local pad, not get it from a remote
> > > sub-device.
> > >
> > > Link validation ensures they're the same (or at least compatible).
> > >
> > > Speaking of which---you don't have link_validate callbacks set for the
> > > sub-device. See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2.c for an
> > > example.
> > >
> >
> > ...
> >
> > > > + if (!stream->cif_fmt_in)
> > > > + goto runtime_put;
> > > > +
> > > > + ret = cif_stream_start(stream);
> > > > + if (ret < 0)
> > > > + goto stop_stream;
> > > > +
> > > > + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> > > > + if (ret < 0)
> > > > + goto stop_stream;
> > > > +
> > > > + return 0;
> > > > +
> > > > +stop_stream:
> > > > + cif_stream_stop(stream);
> > > > +runtime_put:
> > > > + pm_runtime_put(cif_dev->dev);
> > > > +destroy_buf:
> > > > + cif_return_all_buffers(stream, VB2_BUF_STATE_QUEUED);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +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);
> > >
> > > The user space is responsible for controlling the sensor i.e. you shouldn't
> > > call set_fmt sub-device op from this driver.
> > >
> > > As the driver is MC-enabled, generally the sub-devices act as a control
> > > interface and the V4L2 video nodes are a data interface.
> > >
> >
> > While this is true for MC-centric (Media Controller) drivers, this driver is
> > video-node-centric (I mentioned this in the commit msg)
> >
> > From the Kernel Documentation:
> > https://docs.kernel.org/userspace-api/media/v4l/open.html
> >
> > 1 - The devices that are fully controlled via V4L2 device nodes are
> > called video-node-centric.
> >
> > 2- Note: A video-node-centric may still provide media-controller and
> > sub-device interfaces as well. However, in that case the media-controller
> > and the sub-device interfaces are read-only and just provide information
> > about the device. The actual configuration is done via the video nodes.
>
> Are you sure you even want to do this?
>
> It'll limit what kind of sensors you can attach to the device and even more
> so in the future as we're reworking the sensor APIs to allow better control
> of the sensors, using internal pads (that require MC).
>
> There have been some such drivers in the past but many have been already
> converted, or in some cases the newer hardware generation uses MC. Keeping
> API compatibility is a requirement so you can't just "add support" later
> on.

I totally agree that using the MC approach is better but this has nothing to
do with me wanting this but due to constraints I unfortunately cannot control
it is impossible to convert it now.

I would say the px30 driver is still very useful and people are going to use it: a follow-up patch series to
add support for the Rockchip RK3568 Video Capture has already been sent:
https://lore.kernel.org/linux-media/20240220-v6-8-topic-rk3568-vicap-v1-0-2680a1fa640b@wolfvision.net/

--
Kind Regards
Mehdi Djait

2024-03-04 09:28:39

by Michael Riesch

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi Mehdi, Sakari,

On 3/2/24 12:51, Mehdi Djait wrote:
> Hi Sakari,
>
> On Tue, Feb 27, 2024 at 10:23:46AM +0000, Sakari Ailus wrote:
>> Hi Mehdi,
>>
>> On Wed, Feb 21, 2024 at 06:55:54PM +0100, Mehdi Djait wrote:
>>> Hi Sakari,
>>>
>>> Thank you for the review!
>>>
>>> On Tue, Feb 13, 2024 at 01:37:39PM +0000, Sakari Ailus wrote:
>>>> Hi Mahdi,
>>>>
>>>> On Sun, Feb 11, 2024 at 08:03:31PM +0100, Mehdi Djait wrote:
>>>>> From: Mehdi Djait <[email protected]>
>>>>>
>>>>> 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.
>>>>>
>>>>> Reviewed-by: Michael Riesch <[email protected]>
>>>>> Signed-off-by: Mehdi Djait <[email protected]>
>>>>> Signed-off-by: Mehdi Djait <[email protected]>
>>>>> ---
>>>>> MAINTAINERS | 7 +
>>>>> drivers/media/platform/rockchip/Kconfig | 1 +
>>>>> drivers/media/platform/rockchip/Makefile | 1 +
>>>>> drivers/media/platform/rockchip/cif/Kconfig | 14 +
>>>>> drivers/media/platform/rockchip/cif/Makefile | 3 +
>>>>> .../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
>>>>> .../media/platform/rockchip/cif/cif-capture.h | 20 +
>>>>> .../media/platform/rockchip/cif/cif-common.h | 128 ++
>>>>> drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
>>>>> .../media/platform/rockchip/cif/cif-regs.h | 127 ++
>>>>> 10 files changed, 1720 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/cif-capture.c
>>>>> create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
>>>>> create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
>>>>> create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
>>>>> create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h
>>>>>
>>>>> +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_resume_and_get(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);
>>>>
>>>> You should use the format on the local pad, not get it from a remote
>>>> sub-device.
>>>>
>>>> Link validation ensures they're the same (or at least compatible).
>>>>
>>>> Speaking of which---you don't have link_validate callbacks set for the
>>>> sub-device. See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2.c for an
>>>> example.
>>>>
>>>
>>> ...
>>>
>>>>> + if (!stream->cif_fmt_in)
>>>>> + goto runtime_put;
>>>>> +
>>>>> + ret = cif_stream_start(stream);
>>>>> + if (ret < 0)
>>>>> + goto stop_stream;
>>>>> +
>>>>> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
>>>>> + if (ret < 0)
>>>>> + goto stop_stream;
>>>>> +
>>>>> + return 0;
>>>>> +
>>>>> +stop_stream:
>>>>> + cif_stream_stop(stream);
>>>>> +runtime_put:
>>>>> + pm_runtime_put(cif_dev->dev);
>>>>> +destroy_buf:
>>>>> + cif_return_all_buffers(stream, VB2_BUF_STATE_QUEUED);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +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);
>>>>
>>>> The user space is responsible for controlling the sensor i.e. you shouldn't
>>>> call set_fmt sub-device op from this driver.
>>>>
>>>> As the driver is MC-enabled, generally the sub-devices act as a control
>>>> interface and the V4L2 video nodes are a data interface.
>>>>
>>>
>>> While this is true for MC-centric (Media Controller) drivers, this driver is
>>> video-node-centric (I mentioned this in the commit msg)
>>>
>>> From the Kernel Documentation:
>>> https://docs.kernel.org/userspace-api/media/v4l/open.html
>>>
>>> 1 - The devices that are fully controlled via V4L2 device nodes are
>>> called video-node-centric.
>>>
>>> 2- Note: A video-node-centric may still provide media-controller and
>>> sub-device interfaces as well. However, in that case the media-controller
>>> and the sub-device interfaces are read-only and just provide information
>>> about the device. The actual configuration is done via the video nodes.
>>
>> Are you sure you even want to do this?
>>
>> It'll limit what kind of sensors you can attach to the device and even more
>> so in the future as we're reworking the sensor APIs to allow better control
>> of the sensors, using internal pads (that require MC).
>>
>> There have been some such drivers in the past but many have been already
>> converted, or in some cases the newer hardware generation uses MC. Keeping
>> API compatibility is a requirement so you can't just "add support" later
>> on.
>
> I totally agree that using the MC approach is better but this has nothing to
> do with me wanting this but due to constraints I unfortunately cannot control
> it is impossible to convert it now.
>
> I would say the px30 driver is still very useful and people are going to use it: a follow-up patch series to
> add support for the Rockchip RK3568 Video Capture has already been sent:
> https://lore.kernel.org/linux-media/20240220-v6-8-topic-rk3568-vicap-v1-0-2680a1fa640b@wolfvision.net/

The driver is indeed useful as is, therefore I was rather hoping that it
would be accepted quickly to facilitate further additions (such as the
aforementioned RK3568 support series).

However, I was not aware that the video node centric vs. media
controller centric approach has significant implications on user space
and hence on backwards compatibility. Now that Sakari has pointed out
that one, I am leaning towards converting the driver to MC before it is
integrated in mainline.

I fully understand, though, that Mehdi is not in the position to make
the required changes due to time constraints. Maybe I can fill in and
invest some time in that, provided that
- it is OK for Mehdi and the Bootlin people that I take over the series
at hand, leaving the authorship intact of course, but adding my
Co-developed-by:
- Sakari (or someone else from the linux-media community) can provide a
brief overview of what exactly needs to be done to do the conversion
It should be noted that right now I have no clue what needs to be
changed, which implies that the conversion will not happen any time soon.

What do you think?

Best regards,
Michael

2024-03-04 11:10:45

by Sakari Ailus

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi Michael, Mehdi,

On Mon, Mar 04, 2024 at 10:25:23AM +0100, Michael Riesch wrote:
> Hi Mehdi, Sakari,
>
> On 3/2/24 12:51, Mehdi Djait wrote:
> > Hi Sakari,
> >
> > On Tue, Feb 27, 2024 at 10:23:46AM +0000, Sakari Ailus wrote:
> >> Hi Mehdi,
> >>
> >> On Wed, Feb 21, 2024 at 06:55:54PM +0100, Mehdi Djait wrote:
> >>> Hi Sakari,
> >>>
> >>> Thank you for the review!
> >>>
> >>> On Tue, Feb 13, 2024 at 01:37:39PM +0000, Sakari Ailus wrote:
> >>>> Hi Mahdi,
> >>>>
> >>>> On Sun, Feb 11, 2024 at 08:03:31PM +0100, Mehdi Djait wrote:
> >>>>> From: Mehdi Djait <[email protected]>
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>> Reviewed-by: Michael Riesch <[email protected]>
> >>>>> Signed-off-by: Mehdi Djait <[email protected]>
> >>>>> Signed-off-by: Mehdi Djait <[email protected]>
> >>>>> ---
> >>>>> MAINTAINERS | 7 +
> >>>>> drivers/media/platform/rockchip/Kconfig | 1 +
> >>>>> drivers/media/platform/rockchip/Makefile | 1 +
> >>>>> drivers/media/platform/rockchip/cif/Kconfig | 14 +
> >>>>> drivers/media/platform/rockchip/cif/Makefile | 3 +
> >>>>> .../media/platform/rockchip/cif/cif-capture.c | 1111 +++++++++++++++++
> >>>>> .../media/platform/rockchip/cif/cif-capture.h | 20 +
> >>>>> .../media/platform/rockchip/cif/cif-common.h | 128 ++
> >>>>> drivers/media/platform/rockchip/cif/cif-dev.c | 308 +++++
> >>>>> .../media/platform/rockchip/cif/cif-regs.h | 127 ++
> >>>>> 10 files changed, 1720 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/cif-capture.c
> >>>>> create mode 100644 drivers/media/platform/rockchip/cif/cif-capture.h
> >>>>> create mode 100644 drivers/media/platform/rockchip/cif/cif-common.h
> >>>>> create mode 100644 drivers/media/platform/rockchip/cif/cif-dev.c
> >>>>> create mode 100644 drivers/media/platform/rockchip/cif/cif-regs.h
> >>>>>
> >>>>> +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_resume_and_get(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);
> >>>>
> >>>> You should use the format on the local pad, not get it from a remote
> >>>> sub-device.
> >>>>
> >>>> Link validation ensures they're the same (or at least compatible).
> >>>>
> >>>> Speaking of which---you don't have link_validate callbacks set for the
> >>>> sub-device. See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2.c for an
> >>>> example.
> >>>>
> >>>
> >>> ...
> >>>
> >>>>> + if (!stream->cif_fmt_in)
> >>>>> + goto runtime_put;
> >>>>> +
> >>>>> + ret = cif_stream_start(stream);
> >>>>> + if (ret < 0)
> >>>>> + goto stop_stream;
> >>>>> +
> >>>>> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> >>>>> + if (ret < 0)
> >>>>> + goto stop_stream;
> >>>>> +
> >>>>> + return 0;
> >>>>> +
> >>>>> +stop_stream:
> >>>>> + cif_stream_stop(stream);
> >>>>> +runtime_put:
> >>>>> + pm_runtime_put(cif_dev->dev);
> >>>>> +destroy_buf:
> >>>>> + cif_return_all_buffers(stream, VB2_BUF_STATE_QUEUED);
> >>>>> +
> >>>>> + return ret;
> >>>>> +}
> >>>>> +
> >>>>> +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);
> >>>>
> >>>> The user space is responsible for controlling the sensor i.e. you shouldn't
> >>>> call set_fmt sub-device op from this driver.
> >>>>
> >>>> As the driver is MC-enabled, generally the sub-devices act as a control
> >>>> interface and the V4L2 video nodes are a data interface.
> >>>>
> >>>
> >>> While this is true for MC-centric (Media Controller) drivers, this driver is
> >>> video-node-centric (I mentioned this in the commit msg)
> >>>
> >>> From the Kernel Documentation:
> >>> https://docs.kernel.org/userspace-api/media/v4l/open.html
> >>>
> >>> 1 - The devices that are fully controlled via V4L2 device nodes are
> >>> called video-node-centric.
> >>>
> >>> 2- Note: A video-node-centric may still provide media-controller and
> >>> sub-device interfaces as well. However, in that case the media-controller
> >>> and the sub-device interfaces are read-only and just provide information
> >>> about the device. The actual configuration is done via the video nodes.
> >>
> >> Are you sure you even want to do this?
> >>
> >> It'll limit what kind of sensors you can attach to the device and even more
> >> so in the future as we're reworking the sensor APIs to allow better control
> >> of the sensors, using internal pads (that require MC).
> >>
> >> There have been some such drivers in the past but many have been already
> >> converted, or in some cases the newer hardware generation uses MC. Keeping
> >> API compatibility is a requirement so you can't just "add support" later
> >> on.
> >
> > I totally agree that using the MC approach is better but this has nothing to
> > do with me wanting this but due to constraints I unfortunately cannot control
> > it is impossible to convert it now.
> >
> > I would say the px30 driver is still very useful and people are going to use it: a follow-up patch series to
> > add support for the Rockchip RK3568 Video Capture has already been sent:
> > https://lore.kernel.org/linux-media/20240220-v6-8-topic-rk3568-vicap-v1-0-2680a1fa640b@wolfvision.net/
>
> The driver is indeed useful as is, therefore I was rather hoping that it
> would be accepted quickly to facilitate further additions (such as the
> aforementioned RK3568 support series).
>
> However, I was not aware that the video node centric vs. media
> controller centric approach has significant implications on user space
> and hence on backwards compatibility. Now that Sakari has pointed out
> that one, I am leaning towards converting the driver to MC before it is
> integrated in mainline.
>
> I fully understand, though, that Mehdi is not in the position to make
> the required changes due to time constraints. Maybe I can fill in and
> invest some time in that, provided that
> - it is OK for Mehdi and the Bootlin people that I take over the series
> at hand, leaving the authorship intact of course, but adding my
> Co-developed-by:
> - Sakari (or someone else from the linux-media community) can provide a
> brief overview of what exactly needs to be done to do the conversion
> It should be noted that right now I have no clue what needs to be
> changed, which implies that the conversion will not happen any time soon.

You need to make the driver Media device centric. The V4L2 video nodes will
remain a data interface only. In practice this mostly involves, from the
current driver state, adding a sub-device for CIF device and removing
sensor control via video node. The driver already registers the media
device, that's good

See e.g. drivers/media/pci/intel/ipu3/ipu3-cio2.c for an example. There are
probably other minor interface related matters, such as the use of
V4L2_CAP_IO_MC capability flag.

>
> What do you think?

--
Sakari Ailus

2024-05-30 00:27:58

by Val Packett

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi,

On Sun, Feb 11 2024 at 20:03:31 +01:00:00, Mehdi Djait
<[email protected]> wrote:
> This introduces a V4L2 driver for the Rockchip CIF video capture
> controller.
>
> This controller can be found on RK3066, PX30, RK1808, RK3128 and
> RK3288,
> but for now it's only been tested on the PX30.

I've been testing it on an RK3066 tablet I'm bringing up mainline on,
recently figured out the clock situation that made the cameras appear
on I2C and got a camera driver to attach, but still not getting a frame
and will need to debug more.. Since you're familiar with the hardware,
could you quickly tell me what exactly causes the CIF interrupt to
fire? Is it a signal from the camera?

Anyway, two things I noticed with the code:

> +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 && ret != -EOPNOTSUPP)
> + return ret;

v4l2_subdev_call actually returns -ENOIOCTLCMD when the camera driver
does not handle the requested call.

> +static int cif_plat_probe(struct platform_device *pdev)
> +{
[?]
> + ret = devm_clk_bulk_get(dev, cif_dev->match_data->clks_num,
> + cif_dev->match_data->clks);

This looks like it's just modifying the `static` clocks array shared
between all instances of the driver and not copying the clock
references into the instance's private struct.

Thanks,
~val



2024-06-12 05:25:35

by Val Packett

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi,

a couple more comments on this driver:

On Sun, Feb 11 2024 at 20:03:31 +01:00:00, Mehdi Djait
<[email protected]> wrote:
> +static int cif_stream_start(struct cif_stream *stream)
> +{
> + u32 val, fmt_type, xfer_mode = 0;
> + struct cif_device *cif_dev = stream->cifdev;
> + struct cif_remote *remote_info = &cif_dev->remote;
> + int ret;
> + u32 input_mode;
> +
> + stream->frame_idx = 0;
> + stream->frame_phase = 0;
> +
> + fmt_type = stream->cif_fmt_in->fmt_type;
> + input_mode = (remote_info->std == V4L2_STD_NTSC) ?
> + CIF_FORMAT_INPUT_MODE_NTSC :
> + CIF_FORMAT_INPUT_MODE_PAL;

Mode logic needs to be expanded for cameras; I'm trying to get it
working correctly,
so far managed to get some cursed selfies with the wrong pixel format
:) but either
way I could send a patch when I have it working well.

> +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);
> +

Potential deadlock with this lock:

[ 3.438667] ======================================================
[ 3.438672] WARNING: possible circular locking dependency detected
[ 3.438677] 6.10.0-rc1-next-20240531 #151 Not tainted
[ 3.438684] ------------------------------------------------------
[ 3.438687] kworker/u8:1/25 is trying to acquire lock:
[ 3.438695] c152d41c (videodev_lock){+.+.}-{4:4}, at:
__video_register_device+0xf4/0x15b0
[ 3.438737]
[ 3.438737] but task is already holding lock:
[ 3.438740] c31981fc (&mdev->graph_mutex){+.+.}-{4:4}, at:
subdev_notifier_complete+0x20/0x80
[ 3.438765]
[ 3.438765] which lock already depends on the new lock.
[ 3.438765]
[ 3.438769]
[ 3.438769] the existing dependency chain (in reverse order) is:
[ 3.438772]
[ 3.438772] -> #1 (&mdev->graph_mutex){+.+.}-{4:4}:
[ 3.438786] lock_acquire+0x110/0x374
[ 3.438809] __mutex_lock+0xac/0xf2c
[ 3.438828] mutex_lock_nested+0x1c/0x24
[ 3.438843] media_device_register_entity+0x80/0x1e8
[ 3.438857] __video_register_device+0xab0/0x15b0
[ 3.438869] cif_register_stream_vdev+0x158/0x18c
[ 3.438880] cif_plat_probe+0x20c/0x424
[ 3.438888] platform_probe+0x5c/0xb0
[ 3.438905] really_probe+0xc8/0x2cc
[ 3.438916] __driver_probe_device+0x88/0x1a0
[ 3.438926] driver_probe_device+0x30/0x108
[ 3.438936] __driver_attach+0x94/0x184
[ 3.438946] bus_for_each_dev+0x7c/0xcc
[ 3.438955] bus_add_driver+0xcc/0x1ec
[ 3.438964] driver_register+0x7c/0x114
[ 3.438975] do_one_initcall+0x7c/0x2f8
[ 3.438989] kernel_init_freeable+0x1b0/0x20c
[ 3.439010] kernel_init+0x14/0x12c
[ 3.439024] ret_from_fork+0x14/0x28
[ 3.439033]
[ 3.439033] -> #0 (videodev_lock){+.+.}-{4:4}:
[ 3.439048] check_prev_add+0x134/0x17d8
[ 3.439065] __lock_acquire+0x17e0/0x21fc
[ 3.439080] lock_acquire+0x110/0x374
[ 3.439095] __mutex_lock+0xac/0xf2c
[ 3.439109] mutex_lock_nested+0x1c/0x24
[ 3.439123] __video_register_device+0xf4/0x15b0
[ 3.439135] __v4l2_device_register_subdev_nodes+0xd8/0x1a0
[ 3.439152] subdev_notifier_complete+0x2c/0x80
[ 3.439160] __v4l2_async_register_subdev+0xa8/0x1a0
[ 3.439176] gc0308_probe+0x654/0x6f4
[ 3.439187] i2c_device_probe+0x168/0x268
[ 3.439201] really_probe+0xc8/0x2cc
[ 3.439211] __driver_probe_device+0x88/0x1a0
[ 3.439221] driver_probe_device+0x30/0x108
[ 3.439231] __device_attach_driver+0x94/0x10c
[ 3.439241] bus_for_each_drv+0x90/0xe4
[ 3.439250] __device_attach+0xac/0x1b0
[ 3.439260] bus_probe_device+0x8c/0x90
[ 3.439269] deferred_probe_work_func+0x7c/0xac
[ 3.439279] process_one_work+0x23c/0x6bc
[ 3.439295] worker_thread+0x190/0x3d8
[ 3.439308] kthread+0xf8/0x114
[ 3.439321] ret_from_fork+0x14/0x28
[ 3.439330]
[ 3.439330] other info that might help us debug this:
[ 3.439330]
[ 3.439333] Possible unsafe locking scenario:
[ 3.439333]
[ 3.439336] CPU0 CPU1
[ 3.439339] ---- ----
[ 3.439341] lock(&mdev->graph_mutex);
[ 3.439349] lock(videodev_lock);
[ 3.439356] lock(&mdev->graph_mutex);
[ 3.439363] lock(videodev_lock);
[ 3.439370]
[ 3.439370] *** DEADLOCK ***
[ 3.439370]
[ 3.439373] 5 locks held by kworker/u8:1/25:
[ 3.439379] #0: c28106b4
((wq_completion)events_unbound){+.+.}-{0:0}, at:
process_one_work+0x1ac/0x6bc
[ 3.439408] #1: f0889f20 (deferred_probe_work){+.+.}-{0:0}, at:
process_one_work+0x1d4/0x6bc
[ 3.439436] #2: c303ec9c (&dev->mutex){....}-{4:4}, at:
__device_attach+0x30/0x1b0
[ 3.439461] #3: c152d3d0 (list_lock){+.+.}-{4:4}, at:
__v4l2_async_register_subdev+0x50/0x1a0
[ 3.439491] #4: c31981fc (&mdev->graph_mutex){+.+.}-{4:4}, at:
subdev_notifier_complete+0x20/0x80

>



2024-06-12 22:02:04

by Mehdi Djait

[permalink] [raw]
Subject: Re: [RESEND Patch v13 2/3] media: rockchip: Add a driver for Rockchip's camera interface

Hi Val :)

And sorry for the late response!

On Wed, Jun 12, 2024 at 02:23:13AM -0300, Val Packett wrote:
> Hi,
>
> a couple more comments on this driver:
>
> On Sun, Feb 11 2024 at 20:03:31 +01:00:00, Mehdi Djait
> <[email protected]> wrote:
> > +static int cif_stream_start(struct cif_stream *stream)
> > +{
> > + u32 val, fmt_type, xfer_mode = 0;
> > + struct cif_device *cif_dev = stream->cifdev;
> > + struct cif_remote *remote_info = &cif_dev->remote;
> > + int ret;
> > + u32 input_mode;
> > +
> > + stream->frame_idx = 0;
> > + stream->frame_phase = 0;
> > +
> > + fmt_type = stream->cif_fmt_in->fmt_type;
> > + input_mode = (remote_info->std == V4L2_STD_NTSC) ?
> > + CIF_FORMAT_INPUT_MODE_NTSC :
> > + CIF_FORMAT_INPUT_MODE_PAL;
>
> Mode logic needs to be expanded for cameras; I'm trying to get it working
> correctly,
> so far managed to get some cursed selfies with the wrong pixel format :) but
> either
> way I could send a patch when I have it working well.
>

Do you mind sharing which sensor is connected on one end to the camera
and the other end to cif ?

I developed this driver using the tw9900 NTSC/PAL/SECAM Video Decoder
and I know that more patches are needed to make this driver work
with other sensors. Look at the series from Michael Riesch where he
adds support for other sensors on top of this series:
https://lore.kernel.org/linux-media/20240220-v6-8-topic-rk3568-vicap-v1-0-2680a1fa640b@wolfvision.net/

PLEASE NOTE: For the moment being I am not working on this driver as I
don't have access to the hardware.

I need a rockchip board that can boot upstream kernel AND that has
DVP (Digital Video Port) camera interface to continue working on CIF.
I have some candidates but I am still not entirely sure how to proceed
further.

ALSO NOTE: This driver still need some more work as you can see in the
last round of reviews: it should be converted to a media device centric
driver that correctly uses the Media Controller.

Look at the discussion here: https://lore.kernel.org/linux-media/[email protected]/

> > +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);
> > +
>
> Potential deadlock with this lock:

I will look more into this when/if I contine working on this driver

Still I am happy that other people are taking a look at this series and
doing something with it!

--
Kind Regards
Mehdi Djait

>
> [ 3.438667] ======================================================
> [ 3.438672] WARNING: possible circular locking dependency detected
> [ 3.438677] 6.10.0-rc1-next-20240531 #151 Not tainted
> [ 3.438684] ------------------------------------------------------
> [ 3.438687] kworker/u8:1/25 is trying to acquire lock:
> [ 3.438695] c152d41c (videodev_lock){+.+.}-{4:4}, at:
> __video_register_device+0xf4/0x15b0
> [ 3.438737]
> [ 3.438737] but task is already holding lock:
> [ 3.438740] c31981fc (&mdev->graph_mutex){+.+.}-{4:4}, at:
> subdev_notifier_complete+0x20/0x80
> [ 3.438765]
> [ 3.438765] which lock already depends on the new lock.
> [ 3.438765]
> [ 3.438769]
> [ 3.438769] the existing dependency chain (in reverse order) is:
> [ 3.438772]
> [ 3.438772] -> #1 (&mdev->graph_mutex){+.+.}-{4:4}:
> [ 3.438786] lock_acquire+0x110/0x374
> [ 3.438809] __mutex_lock+0xac/0xf2c
> [ 3.438828] mutex_lock_nested+0x1c/0x24
> [ 3.438843] media_device_register_entity+0x80/0x1e8
> [ 3.438857] __video_register_device+0xab0/0x15b0
> [ 3.438869] cif_register_stream_vdev+0x158/0x18c
> [ 3.438880] cif_plat_probe+0x20c/0x424
> [ 3.438888] platform_probe+0x5c/0xb0
> [ 3.438905] really_probe+0xc8/0x2cc
> [ 3.438916] __driver_probe_device+0x88/0x1a0
> [ 3.438926] driver_probe_device+0x30/0x108
> [ 3.438936] __driver_attach+0x94/0x184
> [ 3.438946] bus_for_each_dev+0x7c/0xcc
> [ 3.438955] bus_add_driver+0xcc/0x1ec
> [ 3.438964] driver_register+0x7c/0x114
> [ 3.438975] do_one_initcall+0x7c/0x2f8
> [ 3.438989] kernel_init_freeable+0x1b0/0x20c
> [ 3.439010] kernel_init+0x14/0x12c
> [ 3.439024] ret_from_fork+0x14/0x28
> [ 3.439033]
> [ 3.439033] -> #0 (videodev_lock){+.+.}-{4:4}:
> [ 3.439048] check_prev_add+0x134/0x17d8
> [ 3.439065] __lock_acquire+0x17e0/0x21fc
> [ 3.439080] lock_acquire+0x110/0x374
> [ 3.439095] __mutex_lock+0xac/0xf2c
> [ 3.439109] mutex_lock_nested+0x1c/0x24
> [ 3.439123] __video_register_device+0xf4/0x15b0
> [ 3.439135] __v4l2_device_register_subdev_nodes+0xd8/0x1a0
> [ 3.439152] subdev_notifier_complete+0x2c/0x80
> [ 3.439160] __v4l2_async_register_subdev+0xa8/0x1a0
> [ 3.439176] gc0308_probe+0x654/0x6f4
> [ 3.439187] i2c_device_probe+0x168/0x268
> [ 3.439201] really_probe+0xc8/0x2cc
> [ 3.439211] __driver_probe_device+0x88/0x1a0
> [ 3.439221] driver_probe_device+0x30/0x108
> [ 3.439231] __device_attach_driver+0x94/0x10c
> [ 3.439241] bus_for_each_drv+0x90/0xe4
> [ 3.439250] __device_attach+0xac/0x1b0
> [ 3.439260] bus_probe_device+0x8c/0x90
> [ 3.439269] deferred_probe_work_func+0x7c/0xac
> [ 3.439279] process_one_work+0x23c/0x6bc
> [ 3.439295] worker_thread+0x190/0x3d8
> [ 3.439308] kthread+0xf8/0x114
> [ 3.439321] ret_from_fork+0x14/0x28
> [ 3.439330]
> [ 3.439330] other info that might help us debug this:
> [ 3.439330]
> [ 3.439333] Possible unsafe locking scenario:
> [ 3.439333]
> [ 3.439336] CPU0 CPU1
> [ 3.439339] ---- ----
> [ 3.439341] lock(&mdev->graph_mutex);
> [ 3.439349] lock(videodev_lock);
> [ 3.439356] lock(&mdev->graph_mutex);
> [ 3.439363] lock(videodev_lock);
> [ 3.439370]
> [ 3.439370] *** DEADLOCK ***
> [ 3.439370]
> [ 3.439373] 5 locks held by kworker/u8:1/25:
> [ 3.439379] #0: c28106b4 ((wq_completion)events_unbound){+.+.}-{0:0},
> at: process_one_work+0x1ac/0x6bc
> [ 3.439408] #1: f0889f20 (deferred_probe_work){+.+.}-{0:0}, at:
> process_one_work+0x1d4/0x6bc
> [ 3.439436] #2: c303ec9c (&dev->mutex){....}-{4:4}, at:
> __device_attach+0x30/0x1b0
> [ 3.439461] #3: c152d3d0 (list_lock){+.+.}-{4:4}, at:
> __v4l2_async_register_subdev+0x50/0x1a0
> [ 3.439491] #4: c31981fc (&mdev->graph_mutex){+.+.}-{4:4}, at:
> subdev_notifier_complete+0x20/0x80
>
> >
>
>