Skip to contentSkip to conversation

Add StarFive Camera Subsystem driver

PATCH Superseded Jack Zhu 3 participants 16 messages May 25, 2023 – Jun 19, 2023

Subsystems (2)

v6 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

Jack Zhu

author
#0 lore raw

v6 cover Add StarFive Camera Subsystem driver

Hi,

This series is the v6 series that attempts to support the Camera Subsystem found on StarFive JH7110 SoC.

Changes in v6:
- Added 'bus-type' in bindings example.
- Corrected spelling errors.
- As reviewed by Bryan, used 'nclks' and 'nrsts' variables.
- Added lccf config for ISP.

v5 link: https://lore.kernel.org/all/[email protected]/

Changes in v5:
- Rebased on v6.4-rc1.
- Added new patch.
- Modified ISP driver.

v4 link: https://lore.kernel.org/all/[email protected]/

Previous cover letter from v4:

This patch series adds support for the StarFive Camera Subsystem found on StarFive JH7110 SoC.

The driver implements V4L2, Media controller and V4L2 subdev interfaces. Camera sensor using V4L2 subdev interface in the kernel is supported.

The driver is tested on VisionFive V2 board with IMX219 camera sensor. GStreamer 1.18.5 with v4l2src plugin is supported.

Changes since v3:
Patch 1:
- Modified port@0 and port@1 properties.
- Extended the port@0 example with appropriate properties.
- Added 'port@0' for 'required'
Patch 2:
- Modified spelling errors.
Patch 3:
- Merged patch 5 into the patch with an explanation for compatible in commit msg.
Patch 6:
- Asserted pixel_rst[i] reset in the loop after the err_disable_pixclk label.
- Modified Code Style for getting sys_rst and p_rst.
- Renamed clk_name to name and modified the relevant code. Patch 9:
- Added static for stfcamss_get_mem_res function.
- Added static for isp_close function.
- Fixed implicit conversion warning for stf_vin_map_isp_pad function.
- Dropped unused variables.

v3: https://lore.kernel.org/all/[email protected]/

Changes since v2:
- Rebased on v6.3-rc1.
Patch 1:
- Modified spelling errors.
- Added port@0.
- Modified '$ref' of port.
- Added 'ports' to 'required'.
- Dropped 'stfcamss' label in example.
- Added port@0 in example.
- Added MAINTAINERS file.
Patch 2:
- Split this patch into three new patches.
- Modified compatible property.
- Replaced clock names with the existing names.
- Modified 'bus-type' and 'clock-lanes'
- Added port@2 - port@4
- Dropped 'csi2rx' label in example.
Patch 3:
- Updated rst and dot file as three pipelines were deleted. Patch 4:
- Split this patch into three new patches.
- Dropped .s_power() and .get_fmt().
- Dropped CSI-2 DT support.
- Dropped v4l2_device_register_subdev_nodes().
- Used assigned-clock-rates in DT to set clk value.
- Modified 'compatible' field.
Patch 5:
- Deleted three pipelines.
- Modified 'stfcamss_clocks'/'stfcamss_resets' struct.
- Dropped stfcamss_find_sensor() function.
- Removed redundant code from stfcamss_of_parse_endpoint_node().
- Modified spelling errors.
- Rewrote stfcamss_reg_media_subdev_node() function.
- Modified stfcamss_subdev_notifier_bound().
- Modified stfcamss_probe() function.
- Dropped stfcamss_suspend() and stfcamss_resume().
- Dropped dev_info() in stfcamss_remove() function.
- Added 'stf_' prefix for enum subdev_type.
- Moved all includes to the top in stf_camss.h file.
- Dropped unused fields in stfcamss struct.
- Replaced Custom logging macros with regular macros.
- Rewrote register read and write functions.
- Used lowercase for all hex constants.
- Used macro to name registers.
- Dropped unused ioctl and stf_isp_ioctl.h file.

v2: https://lore.kernel.org/all/[email protected]/

Changes since v1:
- Deleted starfive,jh7110-mipi-csi2.yaml.
- Converted cdns,csi2rx.txt to cdns,csi2rx.yaml and added ‘resets’ properties.
- Added ‘cdns,csi2rx.yaml’ in ‘CADENCE MIPI-CSI2 BRIDGES’ entry.
- The following contents were modified in starfive,jh7110-camss.yaml:
dropped quotes from ’id’ and ‘schema’; dropped ‘|’ for ‘description’; corrected the wrong or redundant words: ‘a ISP’, ‘PD ISP’; dropped ‘minItems’ for ‘reg’, ‘clocks’, ‘resets’ and ‘interrupts’; dropped the '_clk' and 'rst_' prefix about the 'clock-names' and 'reset-names';
changed ‘endpoint@1’ to ‘endpoint’; updated examples;
- Updated Subject for some patches.
- Merged patch 6, 7, 8, 9, 10, 11 into one patch.

Jack Zhu (6):
media: dt-bindings: Add JH7110 Camera Subsystem
media: admin-guide: Add starfive_camss.rst for Starfive Camera
Subsystem
media: starfive: Add basic driver
media: starfive: Add video driver
media: starfive: Add ISP driver
media: starfive: Add VIN driver

 .../admin-guide/media/starfive_camss.rst      |   57 +
 .../media/starfive_camss_graph.dot            |   16 +
 .../admin-guide/media/v4l-drivers.rst         |    1 +
 .../bindings/media/starfive,jh7110-camss.yaml |  180 +++
 MAINTAINERS                                   |    9 +
 drivers/media/platform/Kconfig                |    1 +
 drivers/media/platform/Makefile               |    1 +
 drivers/media/platform/starfive/Kconfig       |   19 +
 drivers/media/platform/starfive/Makefile      |   14 +
 drivers/media/platform/starfive/stf_camss.c   |  476 +++++++
 drivers/media/platform/starfive/stf_camss.h   |  157 +++
 drivers/media/platform/starfive/stf_common.h  |   18 +
 drivers/media/platform/starfive/stf_isp.c     |  550 ++++++++
 drivers/media/platform/starfive/stf_isp.h     |  483 +++++++
 .../media/platform/starfive/stf_isp_hw_ops.c  |  468 +++++++
 drivers/media/platform/starfive/stf_video.c   |  864 +++++++++++++
 drivers/media/platform/starfive/stf_video.h   |   95 ++
 drivers/media/platform/starfive/stf_vin.c     | 1134 +++++++++++++++++
 drivers/media/platform/starfive/stf_vin.h     |  180 +++
 .../media/platform/starfive/stf_vin_hw_ops.c  |  241 ++++
 20 files changed, 4964 insertions(+)
 create mode 100644 Documentation/admin-guide/media/starfive_camss.rst
 create mode 100644 Documentation/admin-guide/media/starfive_camss_graph.dot
 create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
 create mode 100644 drivers/media/platform/starfive/Kconfig
 create mode 100644 drivers/media/platform/starfive/Makefile
 create mode 100644 drivers/media/platform/starfive/stf_camss.c
 create mode 100644 drivers/media/platform/starfive/stf_camss.h
 create mode 100644 drivers/media/platform/starfive/stf_common.h
 create mode 100644 drivers/media/platform/starfive/stf_isp.c
 create mode 100644 drivers/media/platform/starfive/stf_isp.h
 create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
 create mode 100644 drivers/media/platform/starfive/stf_video.c
 create mode 100644 drivers/media/platform/starfive/stf_video.h
 create mode 100644 drivers/media/platform/starfive/stf_vin.c
 create mode 100644 drivers/media/platform/starfive/stf_vin.h
 create mode 100644 drivers/media/platform/starfive/stf_vin_hw_ops.c
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v6 1/6 media: dt-bindings: Add JH7110 Camera Subsystem

Add the bindings documentation for Starfive JH7110 Camera Subsystem which is used for handing image sensor data.

---
 .../bindings/media/starfive,jh7110-camss.yaml | 180 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 187 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +180 -0
new file mode 100644
index 000000000000..c66586d90fa2
@@ -0,0 +1,180 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/starfive,jh7110-camss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Starfive SoC CAMSS ISP
+
+maintainers:
+  - Jack Zhu <[email protected]>
+  - Changhuang Liang <[email protected]>
+
+description:
+  The Starfive CAMSS ISP is a Camera interface for Starfive JH7110 SoC. It
+  consists of a VIN controller (Video In Controller, a top-level control unit)
+  and an ISP.
+
+properties:
+  compatible:
+    const: starfive,jh7110-camss
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: syscon
+      - const: isp
+
+  clocks:
+    maxItems: 7
+
+  clock-names:
+    items:
+      - const: apb_func
+      - const: wrapper_clk_c
+      - const: dvp_inv
+      - const: axiwr
+      - const: mipi_rx0_pxl
+      - const: ispcore_2x
+      - const: isp_axi
+
+  resets:
+    maxItems: 6
+
+  reset-names:
+    items:
+      - const: wrapper_p
+      - const: wrapper_c
+      - const: axird
+      - const: axiwr
+      - const: isp_top_n
+      - const: isp_top_axi
+
+  power-domains:
+    items:
+      - description: JH7110 ISP Power Domain Switch Controller.
+
+  interrupts:
+    maxItems: 4
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description: Input port for receiving DVP data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              bus-type:
+                enum: [5, 6]
+
+              bus-width:
+                enum: [8, 10, 12]
+
+              data-shift:
+                enum: [0, 2]
+                default: 0
+
+              hsync-active:
+                enum: [0, 1]
+                default: 1
+
+              vsync-active:
+                enum: [0, 1]
+                default: 1
+
+            required:
+              - bus-type
+              - bus-width
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Input port for receiving CSI data.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - power-domains
+  - interrupts
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    isp@19840000 {
+        compatible = "starfive,jh7110-camss";
+        reg = <0x19840000 0x10000>,
+              <0x19870000 0x30000>;
+        reg-names = "syscon", "isp";
+        clocks = <&ispcrg 0>,
+                 <&ispcrg 13>,
+                 <&ispcrg 2>,
+                 <&ispcrg 12>,
+                 <&ispcrg 1>,
+                 <&syscrg 51>,
+                 <&syscrg 52>;
+        clock-names = "apb_func",
+                      "wrapper_clk_c",
+                      "dvp_inv",
+                      "axiwr",
+                      "mipi_rx0_pxl",
+                      "ispcore_2x",
+                      "isp_axi";
+        resets = <&ispcrg 0>,
+                 <&ispcrg 1>,
+                 <&ispcrg 10>,
+                 <&ispcrg 11>,
+                 <&syscrg 41>,
+                 <&syscrg 42>;
+        reset-names = "wrapper_p",
+                      "wrapper_c",
+                      "axird",
+                      "axiwr",
+                      "isp_top_n",
+                      "isp_top_axi";
+        power-domains = <&pwrc 5>;
+        interrupts = <92>, <87>, <88>, <90>;
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            port@0 {
+                reg = <0>;
+                vin_from_sc2235: endpoint {
+                    remote-endpoint = <&sc2235_to_vin>;
+                    bus-type = <5>;
+                    bus-width = <8>;
+                    data-shift = <2>;
+                    hsync-active = <1>;
+                    vsync-active = <0>;
+                    pclk-sample = <1>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                vin_from_csi2rx: endpoint {
+                    remote-endpoint = <&csi2rx_to_vin>;
+                };
+            };
+        };
+    };
MAINTAINERS +7 -0
index 869e1c7fc869..9ef5d0e5dff0 100644
@@ -20070,6 +20070,13 @@ M:	Ion Badulescu <[email protected]>
 S:	Odd Fixes
 F:	drivers/net/ethernet/adaptec/starfire*
 
+STARFIVE CAMERA SUBSYSTEM DRIVER
+M:	Jack Zhu <[email protected]>
+M:	Changhuang Liang <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
+
 STARFIVE DEVICETREES
 M:	Emil Renner Berthing <[email protected]>
 S:	Maintained
Signature · 1 line
2.34.1
Reviewed-by on v6 1/6
#2 lore raw

Re: [PATCH v6 1/6] media: dt-bindings: Add JH7110 Camera Subsystem

Hi Jack,

Thank you for the patch.

Quoted 5 lines · Jack Zhu

Add the bindings documentation for Starfive JH7110 Camera Subsystem which is used for handing image sensor data.

Quoted 210 lines
---
 .../bindings/media/starfive,jh7110-camss.yaml | 180 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 187 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml new file +180 -0
new file mode 100644
index 000000000000..c66586d90fa2
@@ -0,0 +1,180 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/starfive,jh7110-camss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Starfive SoC CAMSS ISP
+
+maintainers:
+  - Jack Zhu <[email protected]>
+  - Changhuang Liang <[email protected]>
+
+description:
+  The Starfive CAMSS ISP is a Camera interface for Starfive JH7110 SoC. It
+  consists of a VIN controller (Video In Controller, a top-level control unit)
+  and an ISP.
+
+properties:
+  compatible:
+    const: starfive,jh7110-camss
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: syscon
+      - const: isp
+
+  clocks:
+    maxItems: 7
+
+  clock-names:
+    items:
+      - const: apb_func
+      - const: wrapper_clk_c
+      - const: dvp_inv
+      - const: axiwr
+      - const: mipi_rx0_pxl
+      - const: ispcore_2x
+      - const: isp_axi
+
+  resets:
+    maxItems: 6
+
+  reset-names:
+    items:
+      - const: wrapper_p
+      - const: wrapper_c
+      - const: axird
+      - const: axiwr
+      - const: isp_top_n
+      - const: isp_top_axi
+
+  power-domains:
+    items:
+      - description: JH7110 ISP Power Domain Switch Controller.
+
+  interrupts:
+    maxItems: 4
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description: Input port for receiving DVP data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              bus-type:
+                enum: [5, 6]
+
+              bus-width:
+                enum: [8, 10, 12]
+
+              data-shift:
+                enum: [0, 2]
+                default: 0
+
+              hsync-active:
+                enum: [0, 1]
+                default: 1
+
+              vsync-active:
+                enum: [0, 1]
+                default: 1
+
+            required:
+              - bus-type
+              - bus-width
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Input port for receiving CSI data.
+
+    required:
+      - port@0
+      - port@1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - power-domains
+  - interrupts
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    isp@19840000 {
+        compatible = "starfive,jh7110-camss";
+        reg = <0x19840000 0x10000>,
+              <0x19870000 0x30000>;
+        reg-names = "syscon", "isp";
+        clocks = <&ispcrg 0>,
+                 <&ispcrg 13>,
+                 <&ispcrg 2>,
+                 <&ispcrg 12>,
+                 <&ispcrg 1>,
+                 <&syscrg 51>,
+                 <&syscrg 52>;
+        clock-names = "apb_func",
+                      "wrapper_clk_c",
+                      "dvp_inv",
+                      "axiwr",
+                      "mipi_rx0_pxl",
+                      "ispcore_2x",
+                      "isp_axi";
+        resets = <&ispcrg 0>,
+                 <&ispcrg 1>,
+                 <&ispcrg 10>,
+                 <&ispcrg 11>,
+                 <&syscrg 41>,
+                 <&syscrg 42>;
+        reset-names = "wrapper_p",
+                      "wrapper_c",
+                      "axird",
+                      "axiwr",
+                      "isp_top_n",
+                      "isp_top_axi";
+        power-domains = <&pwrc 5>;
+        interrupts = <92>, <87>, <88>, <90>;
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            port@0 {
+                reg = <0>;
+                vin_from_sc2235: endpoint {
+                    remote-endpoint = <&sc2235_to_vin>;
+                    bus-type = <5>;
+                    bus-width = <8>;
+                    data-shift = <2>;
+                    hsync-active = <1>;
+                    vsync-active = <0>;
+                    pclk-sample = <1>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+                vin_from_csi2rx: endpoint {
+                    remote-endpoint = <&csi2rx_to_vin>;
+                };
+            };
+        };
+    };
MAINTAINERS +7 -0
index 869e1c7fc869..9ef5d0e5dff0 100644
@@ -20070,6 +20070,13 @@ M:	Ion Badulescu <[email protected]>
 S:	Odd Fixes
 F:	drivers/net/ethernet/adaptec/starfire*
 
+STARFIVE CAMERA SUBSYSTEM DRIVER
+M:	Jack Zhu <[email protected]>
+M:	Changhuang Liang <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
+
 STARFIVE DEVICETREES
 M:	Emil Renner Berthing <[email protected]>
 S:	Maintained
Signature · 3 lines
Regards,

Laurent Pinchart
author
#3 lore raw

PATCH v6 2/6 media: admin-guide: Add starfive_camss.rst for Starfive Camera Subsystem

In reply to Jack Zhu

Add starfive_camss.rst file that documents the Starfive Camera Subsystem driver which is used for handing image sensor data.

---
 .../admin-guide/media/starfive_camss.rst      | 57 +++++++++++++++++++
 .../media/starfive_camss_graph.dot            | 16 ++++++
 .../admin-guide/media/v4l-drivers.rst         |  1 +
 MAINTAINERS                                   |  1 +
 4 files changed, 75 insertions(+)
 create mode 100644 Documentation/admin-guide/media/starfive_camss.rst
 create mode 100644 Documentation/admin-guide/media/starfive_camss_graph.dot
Documentation/admin-guide/media/starfive_camss.rst new file +57 -0
new file mode 100644
index 000000000000..a6378849384f
@@ -0,0 +1,57 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. include:: <isonum.txt>
+
+================================
+Starfive Camera Subsystem driver
+================================
+
+Introduction
+------------
+
+This file documents the driver for the Starfive Camera Subsystem found on
+Starfive JH7110 SoC. The driver is located under drivers/media/platform/
+starfive.
+
+The driver implements V4L2, Media controller and v4l2_subdev interfaces.
+Camera sensor using V4L2 subdev interface in the kernel is supported.
+
+The driver has been successfully used on the Gstreamer 1.18.5 with
+v4l2src plugin.
+
+
+Starfive Camera Subsystem hardware
+----------------------------------
+
+The Starfive Camera Subsystem hardware consists of:
+
+- MIPI DPHY Receiver: receives mipi data from a MIPI camera sensor.
+- MIPI CSIRx Controller: is responsible for handling and decoding CSI2 protocol
+  based camera sensor data stream.
+- ISP: handles the image data streams from the MIPI CSIRx Controller.
+- VIN(Video In): a top-level module, is responsible for controlling power
+  and clocks to other modules, dumps the input data to memory or transfers the
+  input data to ISP.
+
+
+Topology
+--------
+
+The media controller pipeline graph is as follows:
+
+.. _starfive_camss_graph:
+
+.. kernel-figure:: starfive_camss_graph.dot
+    :alt:   starfive_camss_graph.dot
+    :align: center
+
+The driver has 2 video devices:
+
+- stf_vin0_wr_video0: capture device for images directly from the VIN module.
+- stf_vin0_isp0_video1: capture device for images without scaling.
+
+The driver has 3 subdevices:
+
+- stf_isp0: is responsible for all the isp operations.
+- stf_vin0_wr: used to dump RAW images to memory.
+- stf_vin0_isp0: used to capture images for the stf_vin0_isp0_video1 device.
Documentation/admin-guide/media/starfive_camss_graph.dot new file +16 -0
new file mode 100644
index 000000000000..850be912cdeb
@@ -0,0 +1,16 @@
+digraph board {
+	rankdir=TB
+	n00000001 [label="{{<port0> 0} | stf_isp0\n/dev/v4l-subdev0 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
+	n00000001:port1 -> n0000000d:port0
+	n00000004 [label="{{<port0> 0} | stf_vin0_wr\n/dev/v4l-subdev1 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
+	n00000004:port1 -> n00000007 [style=bold]
+	n00000007 [label="stf_vin0_wr_video0\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
+	n0000000d [label="{{<port0> 0} | stf_vin0_isp0\n/dev/v4l-subdev2 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
+	n0000000d:port1 -> n00000010 [style=bold]
+	n00000010 [label="stf_vin0_isp0_video1\n/dev/video1", shape=box, style=filled, fillcolor=yellow]
+	n00000018 [label="{{<port0> 0} | cdns_csi2rx.19800000.csi-bridge\n | {<port1> 1 | <port2> 2 | <port3> 3 | <port4> 4}}", shape=Mrecord, style=filled, fillcolor=green]
+	n00000018:port1 -> n00000004:port0 [style=dashed]
+	n00000018:port1 -> n00000001:port0
+	n00000028 [label="{{} | imx219 6-0010\n | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
+	n00000028:port0 -> n00000018:port0 [style=bold]
+}
Documentation/admin-guide/media/v4l-drivers.rst +1 -0
index 1c41f87c3917..2274fd29c1d7 100644
@@ -27,6 +27,7 @@ Video4Linux (V4L) driver-specific documentation
 	si470x
 	si4713
 	si476x
+	starfive_camss
 	vimc
 	visl
 	vivid
MAINTAINERS +1 -0
index 9ef5d0e5dff0..71291dc58671 100644
@@ -20075,6 +20075,7 @@ M:	Jack Zhu <[email protected]>
 M:	Changhuang Liang <[email protected]>
 L:	[email protected]
 S:	Maintained
+F:	Documentation/admin-guide/media/starfive_camss.rst
 F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
 
 STARFIVE DEVICETREES
Signature · 1 line
2.34.1
author
#4 lore raw

PATCH v6 3/6 media: starfive: Add basic driver

In reply to Jack Zhu

Add basic platform driver for StarFive Camera Subsystem.

---
 MAINTAINERS                                  |   1 +
 drivers/media/platform/Kconfig               |   1 +
 drivers/media/platform/Makefile              |   1 +
 drivers/media/platform/starfive/Kconfig      |  19 +
 drivers/media/platform/starfive/Makefile     |   9 +
 drivers/media/platform/starfive/stf_camss.c  | 372 +++++++++++++++++++
 drivers/media/platform/starfive/stf_camss.h  | 153 ++++++++
 drivers/media/platform/starfive/stf_common.h |  18 +
 8 files changed, 574 insertions(+)
 create mode 100644 drivers/media/platform/starfive/Kconfig
 create mode 100644 drivers/media/platform/starfive/Makefile
 create mode 100644 drivers/media/platform/starfive/stf_camss.c
 create mode 100644 drivers/media/platform/starfive/stf_camss.h
 create mode 100644 drivers/media/platform/starfive/stf_common.h
MAINTAINERS +1 -0
index 71291dc58671..33a752569942 100644
@@ -20077,6 +20077,7 @@ L:	[email protected]
 S:	Maintained
 F:	Documentation/admin-guide/media/starfive_camss.rst
 F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
+F:	drivers/media/platform/starfive/
 
 STARFIVE DEVICETREES
 M:	Emil Renner Berthing <[email protected]>
drivers/media/platform/Kconfig +1 -0
index ee579916f874..627eaa0ab3ee 100644
@@ -80,6 +80,7 @@ source "drivers/media/platform/renesas/Kconfig"
 source "drivers/media/platform/rockchip/Kconfig"
 source "drivers/media/platform/samsung/Kconfig"
 source "drivers/media/platform/st/Kconfig"
+source "drivers/media/platform/starfive/Kconfig"
 source "drivers/media/platform/sunxi/Kconfig"
 source "drivers/media/platform/ti/Kconfig"
 source "drivers/media/platform/verisilicon/Kconfig"
drivers/media/platform/Makefile +1 -0
index 5453bb868e67..5a038498a370 100644
@@ -23,6 +23,7 @@ obj-y += renesas/
 obj-y += rockchip/
 obj-y += samsung/
 obj-y += st/
+obj-y += starfive/
 obj-y += sunxi/
 obj-y += ti/
 obj-y += verisilicon/
drivers/media/platform/starfive/Kconfig new file +19 -0
new file mode 100644
index 000000000000..1a2293475bd2
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+comment "Starfive media platform drivers"
+
+config VIDEO_STARFIVE_CAMSS
+	tristate "Starfive Camera Subsystem driver"
+	depends on V4L_PLATFORM_DRIVERS
+	depends on VIDEO_DEV && OF
+	depends on DMA_CMA
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select VIDEOBUF2_DMA_CONTIG
+	select V4L2_FWNODE
+	help
+	   Enable this to support for the Starfive Camera subsystem
+	   found on Starfive JH7110 SoC.
+
+	   To compile this driver as a module, choose M here: the
+	   module will be called stf-camss.
drivers/media/platform/starfive/Makefile new file +9 -0
new file mode 100644
index 000000000000..796775fa52f4
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for StarFive camera subsystem driver.
+#
+
+starfive-camss-objs += \
+		stf_camss.o
+
+obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o \
drivers/media/platform/starfive/stf_camss.c new file +372 -0

372 lines not on this page · raw message

drivers/media/platform/starfive/stf_camss.h new file +153 -0

153 lines not on this page · raw message

drivers/media/platform/starfive/stf_common.h new file +18 -0
new file mode 100644
index 000000000000..e04fca2d9cd4
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * stf_common.h
+ *
+ * StarFive Camera Subsystem - Common definitions
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#ifndef STF_COMMON_H
+#define STF_COMMON_H
+
+enum stf_subdev_type {
+	STF_SUBDEV_TYPE_VIN,
+	STF_SUBDEV_TYPE_ISP,
+};
+
+#endif /* STF_COMMON_H */
Signature · 1 line
2.34.1
#5 lore raw

Re: [PATCH v6 3/6] media: starfive: Add basic driver

Hi Jack,

Thank you for the patch.

Quoted 62 lines · Jack Zhu

Add basic platform driver for StarFive Camera Subsystem.

---
 MAINTAINERS                                  |   1 +
 drivers/media/platform/Kconfig               |   1 +
 drivers/media/platform/Makefile              |   1 +
 drivers/media/platform/starfive/Kconfig      |  19 +
 drivers/media/platform/starfive/Makefile     |   9 +
 drivers/media/platform/starfive/stf_camss.c  | 372 +++++++++++++++++++
 drivers/media/platform/starfive/stf_camss.h  | 153 ++++++++
 drivers/media/platform/starfive/stf_common.h |  18 +
 8 files changed, 574 insertions(+)
 create mode 100644 drivers/media/platform/starfive/Kconfig
 create mode 100644 drivers/media/platform/starfive/Makefile
 create mode 100644 drivers/media/platform/starfive/stf_camss.c
 create mode 100644 drivers/media/platform/starfive/stf_camss.h
 create mode 100644 drivers/media/platform/starfive/stf_common.h
MAINTAINERS +1 -0
index 71291dc58671..33a752569942 100644
@@ -20077,6 +20077,7 @@ L:	[email protected]
 S:	Maintained
 F:	Documentation/admin-guide/media/starfive_camss.rst
 F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
+F:	drivers/media/platform/starfive/
 
 STARFIVE DEVICETREES
 M:	Emil Renner Berthing <[email protected]>
drivers/media/platform/Kconfig +1 -0
index ee579916f874..627eaa0ab3ee 100644
@@ -80,6 +80,7 @@ source "drivers/media/platform/renesas/Kconfig"
 source "drivers/media/platform/rockchip/Kconfig"
 source "drivers/media/platform/samsung/Kconfig"
 source "drivers/media/platform/st/Kconfig"
+source "drivers/media/platform/starfive/Kconfig"
 source "drivers/media/platform/sunxi/Kconfig"
 source "drivers/media/platform/ti/Kconfig"
 source "drivers/media/platform/verisilicon/Kconfig"
drivers/media/platform/Makefile +1 -0
index 5453bb868e67..5a038498a370 100644
@@ -23,6 +23,7 @@ obj-y += renesas/
 obj-y += rockchip/
 obj-y += samsung/
 obj-y += st/
+obj-y += starfive/
 obj-y += sunxi/
 obj-y += ti/
 obj-y += verisilicon/
drivers/media/platform/starfive/Kconfig new file +0 -0
new file mode 100644
index 000000000000..1a2293475bd2

drivers/media/platform/starfive/Kconfig line 9

Quoted 9 lines
drivers/media/platform/starfive/Kconfig new file +9 -0
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+comment "Starfive media platform drivers"
+
+config VIDEO_STARFIVE_CAMSS
+	tristate "Starfive Camera Subsystem driver"
+	depends on V4L_PLATFORM_DRIVERS
+	depends on VIDEO_DEV && OF
+	depends on DMA_CMA

I think you can drop this, the driver doesn't depend on DMA. It uses the DMA mapping API though, so you should depend on HAS_DMA instead.

Quoted 94 lines
drivers/media/platform/starfive/Kconfig new file +10 -0
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select VIDEOBUF2_DMA_CONTIG
+	select V4L2_FWNODE
+	help
+	   Enable this to support for the Starfive Camera subsystem
+	   found on Starfive JH7110 SoC.
+
+	   To compile this driver as a module, choose M here: the
+	   module will be called stf-camss.
drivers/media/platform/starfive/Makefile new file +9 -0
new file mode 100644
index 000000000000..796775fa52f4
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for StarFive camera subsystem driver.
+#
+
+starfive-camss-objs += \
+		stf_camss.o
+
+obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o \
drivers/media/platform/starfive/stf_camss.c new file +63 -0
new file mode 100644
index 000000000000..cb2b62d49416
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * stf_camss.c
+ *
+ * Starfive Camera Subsystem driver
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/videodev2.h>
+#include <media/media-device.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mc.h>
+
+#include "stf_camss.h"
+
+static const char * const stfcamss_clocks[] = {
+	"clk_apb_func",
+	"clk_wrapper_clk_c",
+	"clk_dvp_inv",
+	"clk_axiwr",
+	"clk_mipi_rx0_pxl",
+	"clk_ispcore_2x",
+	"clk_isp_axi",
+};
+
+static const char * const stfcamss_resets[] = {
+	"rst_wrapper_p",
+	"rst_wrapper_c",
+	"rst_axird",
+	"rst_axiwr",
+	"rst_isp_top_n",
+	"rst_isp_top_axi",
+};
+
+static int stfcamss_get_mem_res(struct platform_device *pdev,
+				struct stfcamss *stfcamss)
+{
+	stfcamss->syscon_base =
+		devm_platform_ioremap_resource_byname(pdev, "syscon");
+	if (IS_ERR(stfcamss->syscon_base))
+		return PTR_ERR(stfcamss->syscon_base);
+
+	stfcamss->isp_base =
+		devm_platform_ioremap_resource_byname(pdev, "isp");
+	if (IS_ERR(stfcamss->isp_base))
+		return PTR_ERR(stfcamss->isp_base);
+
+	return 0;
+}
+
+/*
+ * stfcamss_of_parse_endpoint_node - Parse port endpoint node
+ * @dev: Device
+ * @node: Device node to be parsed
+ * @csd: Parsed data from port endpoint node
+ *
+ * Return 0 on success or a negative error code on failure

drivers/media/platform/starfive/stf_camss.c line 73

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+ */
+static int stfcamss_of_parse_endpoint_node(struct device *dev,
+					   struct device_node *node,
+					   struct stfcamss_async_subdev *csd)
+{
+	struct v4l2_fwnode_endpoint vep = { { 0 } };
+
+	v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &vep);
+	dev_dbg(dev, "vep.base.port = 0x%x, id = 0x%x\n",
+		vep.base.port, vep.base.id);

You can print the port and id as decimal numbers, they're indices.

Quoted 26 lines
drivers/media/platform/starfive/stf_camss.c new file +26 -0
+
+	csd->port = vep.base.port;
+
+	return 0;
+}
+
+/*
+ * stfcamss_of_parse_ports - Parse ports node
+ * @stfcamss: STFCAMSS device
+ *
+ * Return number of "port" nodes found in "ports" node
+ */
+static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
+{
+	struct device *dev = stfcamss->dev;
+	struct device_node *node = NULL;
+	struct device_node *remote = NULL;
+	int ret, num_subdevs = 0;
+
+	for_each_endpoint_of_node(dev->of_node, node) {
+		struct stfcamss_async_subdev *csd;
+
+		if (!of_device_is_available(node))
+			continue;
+
+		remote = of_graph_get_remote_port_parent(node);

drivers/media/platform/starfive/stf_camss.c line 109

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+		if (!remote) {
+			dev_err(dev, "Cannot get remote parent\n");
+			ret = -EINVAL;
+			goto err_cleanup;
+		}
+
+		csd = v4l2_async_nf_add_fwnode(&stfcamss->notifier,
+					       of_fwnode_handle(remote),
+					       struct stfcamss_async_subdev);
+		of_node_put(remote);

You can use v4l2_async_nf_add_fwnode_remote() to avoid the need to call of_graph_get_remote_port_parent() yourself.

Quoted 18 lines
drivers/media/platform/starfive/stf_camss.c new file +18 -0
+		if (IS_ERR(csd)) {
+			ret = PTR_ERR(csd);
+			goto err_cleanup;
+		}
+
+		ret = stfcamss_of_parse_endpoint_node(dev, node, csd);
+		if (ret < 0)
+			goto err_cleanup;
+
+		num_subdevs++;
+	}
+
+	return num_subdevs;
+
+err_cleanup:
+	of_node_put(node);
+	return ret;
+}

drivers/media/platform/starfive/stf_camss.c line 137

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+
+static int stfcamss_subdev_notifier_bound(struct v4l2_async_notifier *async,
+					  struct v4l2_subdev *subdev,
+					  struct v4l2_async_subdev *asd)
+{
+	struct stfcamss *stfcamss =
+		container_of(async, struct stfcamss, notifier);
+	struct host_data *host_data = &stfcamss->host_data;
+	struct media_entity *source;
+	int i, j;

i and j are never negative, you can make them unsigned int.

drivers/media/platform/starfive/stf_camss.c line 139

Quoted 2 lines
drivers/media/platform/starfive/stf_camss.c new file +2 -0
+
+	source = &subdev->entity;

You can assign the variable when declaring it:

	struct media_entity *source = &subdev->entity;
Quoted 22 lines
drivers/media/platform/starfive/stf_camss.c new file +22 -0
+
+	for (i = 0; i < source->num_pads; i++) {
+		if (source->pads[i].flags & MEDIA_PAD_FL_SOURCE)
+			break;
+	}
+
+	if (i == source->num_pads) {
+		dev_err(stfcamss->dev, "No source pad in external entity\n");
+		return -EINVAL;
+	}
+
+	for (j = 0; host_data->host_entity[j] && (j < HOST_ENTITY_MAX); j++) {
+		struct media_entity *input;
+		int ret;
+
+		input = host_data->host_entity[j];
+
+		ret = media_create_pad_link(
+			source,
+			i,
+			input,
+			STF_PAD_SINK,

drivers/media/platform/starfive/stf_camss.c line 171

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+			source->function == MEDIA_ENT_F_CAM_SENSOR ?
+			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED :
+			0);
+		if (ret < 0) {
+			dev_err(stfcamss->dev,
+				"Failed to link %s->%s entities: %d\n",
+				source->name, input->name, ret);
+			return ret;
+		}
+	}

I think you can simplify this. First of all, the port to which the newly bound subdev is attached is available in the stfcamss_async_subdev structure. You can cast the asd to a stfcamss_async_subdev with container_of(), access the port number from there, and get the corresponding sink subdev and pad (for the DVP or CSI-2 port). Then, I think you can use v4l2_create_fwnode_links_to_pad(), which will search for the source pad in the source subdev, instead of doing it yourself.

I believe the stfcamss.host_data field can be dropped, as it's only used in this function.

Quoted 21 lines
drivers/media/platform/starfive/stf_camss.c new file +21 -0
+
+	return 0;
+}
+
+static int stfcamss_subdev_notifier_complete(struct v4l2_async_notifier *ntf)
+{
+	struct stfcamss *stfcamss =
+		container_of(ntf, struct stfcamss, notifier);
+
+	return v4l2_device_register_subdev_nodes(&stfcamss->v4l2_dev);
+}
+
+static const struct v4l2_async_notifier_operations
+stfcamss_subdev_notifier_ops = {
+	.bound = stfcamss_subdev_notifier_bound,
+	.complete = stfcamss_subdev_notifier_complete,
+};
+
+static const struct media_device_ops stfcamss_media_ops = {
+	.link_notify = v4l2_pipeline_link_notify,
+};

drivers/media/platform/starfive/stf_camss.c line 202

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+
+static void stfcamss_mc_init(struct platform_device *pdev,
+			     struct stfcamss *stfcamss)
+{
+	stfcamss->media_dev.dev = stfcamss->dev;
+	strscpy(stfcamss->media_dev.model, "Starfive Camera Subsystem",
+		sizeof(stfcamss->media_dev.model));
+	snprintf(stfcamss->media_dev.bus_info,
+		 sizeof(stfcamss->media_dev.bus_info),
+		 "%s:%s", dev_bus_name(&pdev->dev), pdev->name);

No need to set bus_info manually, the framework does if for you in media_device_init().

drivers/media/platform/starfive/stf_camss.c line 203

Quoted 1 line
drivers/media/platform/starfive/stf_camss.c new file +1 -0
+	stfcamss->media_dev.hw_revision = 0x01;

Would the hardware have a version or revision register that you could use to fill this, by any chance ?

Quoted 7 lines
drivers/media/platform/starfive/stf_camss.c new file +7 -0
+	stfcamss->media_dev.ops = &stfcamss_media_ops;
+	media_device_init(&stfcamss->media_dev);
+
+	stfcamss->v4l2_dev.mdev = &stfcamss->media_dev;
+}
+
+/*

drivers/media/platform/starfive/stf_camss.c line 220

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+ * stfcamss_probe - Probe STFCAMSS platform device
+ * @pdev: Pointer to STFCAMSS platform device
+ *
+ * Return 0 on success or a negative error code on failure
+ */
+static int stfcamss_probe(struct platform_device *pdev)
+{
+	struct stfcamss *stfcamss;
+	struct device *dev = &pdev->dev;
+	int ret = 0, i, num_subdevs;

No need to initialize ret to 0.

i is never negative, make it an unsigned int.

drivers/media/platform/starfive/stf_camss.c line 230

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+
+	stfcamss = devm_kzalloc(dev, sizeof(*stfcamss), GFP_KERNEL);
+	if (!stfcamss)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(stfcamss->irq); ++i) {
+		stfcamss->irq[i] = platform_get_irq(pdev, i);
+		if (stfcamss->irq[i] < 0)
+			return dev_err_probe(&pdev->dev, stfcamss->irq[i],
+					     "Failed to get clock%d", i);

I think you meant "irq %u", not clock.

Quoted 26 lines
drivers/media/platform/starfive/stf_camss.c new file +26 -0
+	}
+
+	stfcamss->nclks = ARRAY_SIZE(stfcamss->sys_clk);
+	for (i = 0; i < stfcamss->nclks; ++i)
+		stfcamss->sys_clk[i].id = stfcamss_clocks[i];
+	ret = devm_clk_bulk_get(dev, stfcamss->nclks, stfcamss->sys_clk);
+	if (ret) {
+		dev_err(dev, "Failed to get clk controls\n");
+		return ret;
+	}
+
+	stfcamss->nrsts = ARRAY_SIZE(stfcamss->sys_rst);
+	for (i = 0; i < stfcamss->nrsts; ++i)
+		stfcamss->sys_rst[i].id = stfcamss_resets[i];
+	ret = devm_reset_control_bulk_get_shared(dev, stfcamss->nrsts,
+						 stfcamss->sys_rst);
+	if (ret) {
+		dev_err(dev, "Failed to get reset controls\n");
+		return ret;
+	}
+
+	ret = stfcamss_get_mem_res(pdev, stfcamss);
+	if (ret) {
+		dev_err(dev, "Could not map registers\n");
+		return ret;
+	}

drivers/media/platform/starfive/stf_camss.c line 266

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+
+	stfcamss->dev = dev;
+	platform_set_drvdata(pdev, stfcamss);
+
+	v4l2_async_nf_init(&stfcamss->notifier);
+
+	num_subdevs = stfcamss_of_parse_ports(stfcamss);
+	if (num_subdevs < 0) {
+		dev_err(dev, "Failed to find subdevices\n");
+		return -ENODEV;

I would

		ret = -ENODEV;
		goto err_cleanup_notifier;

here.

Quoted 15 lines
drivers/media/platform/starfive/stf_camss.c new file +15 -0
+	}
+
+	stfcamss_mc_init(pdev, stfcamss);
+
+	ret = v4l2_device_register(stfcamss->dev, &stfcamss->v4l2_dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
+		goto err_cleanup_notifier;
+	}
+
+	ret = media_device_register(&stfcamss->media_dev);
+	if (ret) {
+		dev_err(dev, "Failed to register media device: %d\n", ret);
+		goto err_unregister_device;
+	}

drivers/media/platform/starfive/stf_camss.c line 291

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+
+	stfcamss->notifier.ops = &stfcamss_subdev_notifier_ops;
+	ret = v4l2_async_nf_register(&stfcamss->v4l2_dev, &stfcamss->notifier);
+	if (ret) {
+		dev_err(dev, "Failed to register async subdev nodes: %d\n",
+			ret);
+		goto err_unregister_media_dev;
+	}
+
+	pm_runtime_enable(dev);

You should enable runtime PM before registering the notifier, otherwise there's a race condition where userspace could start using the device before runtime PM gets enabled.

Quoted 44 lines
drivers/media/platform/starfive/stf_camss.c new file +44 -0
+
+	return 0;
+
+err_unregister_media_dev:
+	media_device_unregister(&stfcamss->media_dev);
+err_unregister_device:
+	v4l2_device_unregister(&stfcamss->v4l2_dev);
+err_cleanup_notifier:
+	v4l2_async_nf_cleanup(&stfcamss->notifier);
+	return ret;
+}
+
+/*
+ * stfcamss_remove - Remove STFCAMSS platform device
+ * @pdev: Pointer to STFCAMSS platform device
+ *
+ * Always returns 0.
+ */
+static int stfcamss_remove(struct platform_device *pdev)
+{
+	struct stfcamss *stfcamss = platform_get_drvdata(pdev);
+
+	v4l2_device_unregister(&stfcamss->v4l2_dev);
+	media_device_cleanup(&stfcamss->media_dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id stfcamss_of_match[] = {
+	{ .compatible = "starfive,jh7110-camss" },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, stfcamss_of_match);
+
+static int __maybe_unused stfcamss_runtime_suspend(struct device *dev)
+{
+	struct stfcamss *stfcamss = dev_get_drvdata(dev);
+
+	reset_control_assert(stfcamss->sys_rst[STF_RST_ISP_TOP_AXI].rstc);
+	reset_control_assert(stfcamss->sys_rst[STF_RST_ISP_TOP_N].rstc);
+	clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_ISP_AXI].clk);
+	clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_ISPCORE_2X].clk);

drivers/media/platform/starfive/stf_camss.c line 345

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+
+	return 0;
+}
+
+static int __maybe_unused stfcamss_runtime_resume(struct device *dev)
+{
+	struct stfcamss *stfcamss = dev_get_drvdata(dev);
+
+	clk_prepare_enable(stfcamss->sys_clk[STF_CLK_ISPCORE_2X].clk);
+	clk_prepare_enable(stfcamss->sys_clk[STF_CLK_ISP_AXI].clk);

Error handling is missing for those two calls.

Quoted 39 lines
drivers/media/platform/starfive/stf_camss.c new file +27 -0
+	reset_control_deassert(stfcamss->sys_rst[STF_RST_ISP_TOP_N].rstc);
+	reset_control_deassert(stfcamss->sys_rst[STF_RST_ISP_TOP_AXI].rstc);
+
+	return 0;
+}
+
+static const struct dev_pm_ops stfcamss_pm_ops = {
+	SET_RUNTIME_PM_OPS(stfcamss_runtime_suspend,
+			   stfcamss_runtime_resume,
+			   NULL)
+};
+
+static struct platform_driver stfcamss_driver = {
+	.probe = stfcamss_probe,
+	.remove = stfcamss_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.pm = &stfcamss_pm_ops,
+		.of_match_table = of_match_ptr(stfcamss_of_match),
+	},
+};
+
+module_platform_driver(stfcamss_driver);
+
+MODULE_AUTHOR("StarFive Corporation");
+MODULE_DESCRIPTION("StarFive Camera Subsystem driver");
+MODULE_LICENSE("GPL");
drivers/media/platform/starfive/stf_camss.h new file +6 -0
new file mode 100644
index 000000000000..8f39cd65950c
@@ -0,0 +1,153 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * stf_camss.h
+ *
+ * Starfive Camera Subsystem driver
+ *

drivers/media/platform/starfive/stf_camss.h line 16

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.h new file +10 -0
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#ifndef STF_CAMSS_H
+#define STF_CAMSS_H
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/reset.h>
+#include <media/v4l2-device.h>

You should include media/media-device.h, media/media-entity.h and media/v4l2-async.h.

drivers/media/platform/starfive/stf_camss.h

Quoted 8 lines
drivers/media/platform/starfive/stf_camss.h new file +6 -0
+	struct device *dev;
 
+};
 
+
+#include "stf_common.h"
+
+#define DRV_NAME     "starfive-camss"

DRV_NAME is used in stfcamss_driver.driver.name only. I would drop the macro and write

.name = "starfive-camss",

above, it will be more readable.

drivers/media/platform/starfive/stf_camss.h line 32

Quoted 12 lines
drivers/media/platform/starfive/stf_camss.h new file +12 -0
+#define STF_DVP_NAME "stf_dvp"
+#define STF_CSI_NAME "cdns_csi2rx"
+#define STF_ISP_NAME "stf_isp"
+#define STF_VIN_NAME "stf_vin"
+
+#define STF_PAD_SINK   0
+#define STF_PAD_SRC    1
+#define STF_PADS_NUM   2
+
+enum port_num {
+	PORT_NUMBER_DVP_SENSOR = 0,
+	PORT_NUMBER_CSI2RX

Please name this stf_port_num and STF_PORT_*, to avoid namespace clashes. I would drop "NUMBER_", and name the two ports STF_PORT_DVP and STF_PORT_CSI2RX.

Quoted 23 lines
drivers/media/platform/starfive/stf_camss.h new file +23 -0
+};
+
+enum stf_clk {
+	STF_CLK_APB_FUNC = 0,
+	STF_CLK_WRAPPER_CLK_C,
+	STF_CLK_DVP_INV,
+	STF_CLK_AXIWR,
+	STF_CLK_MIPI_RX0_PXL,
+	STF_CLK_ISPCORE_2X,
+	STF_CLK_ISP_AXI,
+	STF_CLK_NUM
+};
+
+enum stf_rst {
+	STF_RST_WRAPPER_P = 0,
+	STF_RST_WRAPPER_C,
+	STF_RST_AXIRD,
+	STF_RST_AXIWR,
+	STF_RST_ISP_TOP_N,
+	STF_RST_ISP_TOP_AXI,
+	STF_RST_NUM
+};
+

drivers/media/platform/starfive/stf_camss.h line 65

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.h new file +10 -0
+enum stf_irq {
+	STF_IRQ_VINWR = 0,
+	STF_IRQ_ISP,
+	STF_IRQ_ISPCSIL,
+	STF_IRQ_NUM
+};
+
+#define HOST_ENTITY_MAX		2
+
+struct host_data {

Same here, please name this stf_host_data.

Quoted 100 lines
drivers/media/platform/starfive/stf_camss.h new file +88 -0
+	struct media_entity *host_entity[HOST_ENTITY_MAX];
+};
+
+struct stfcamss {
+	struct v4l2_device v4l2_dev;
+	struct media_device media_dev;
+	struct media_pipeline pipe;
+	struct device *dev;
+	struct v4l2_async_notifier notifier;
+	struct host_data host_data;
+	void __iomem *syscon_base;
+	void __iomem *isp_base;
+	int irq[STF_IRQ_NUM];
+	struct clk_bulk_data sys_clk[STF_CLK_NUM];
+	int nclks;
+	struct reset_control_bulk_data sys_rst[STF_RST_NUM];
+	int nrsts;
+};
+
+struct stfcamss_async_subdev {
+	struct v4l2_async_subdev asd;  /* must be first */
+	enum port_num port;
+};
+
+static inline u32 stf_isp_reg_read(struct stfcamss *stfcamss, u32 reg)
+{
+	return ioread32(stfcamss->isp_base + reg);
+}
+
+static inline void stf_isp_reg_write(struct stfcamss *stfcamss,
+				     u32 reg, u32 val)
+{
+	iowrite32(val, stfcamss->isp_base + reg);
+}
+
+static inline void stf_isp_reg_write_delay(struct stfcamss *stfcamss,
+					   u32 reg, u32 val, u32 delay)
+{
+	iowrite32(val, stfcamss->isp_base + reg);
+	usleep_range(1000 * delay, 1000 * delay + 100);
+}
+
+static inline void stf_isp_reg_set_bit(struct stfcamss *stfcamss,
+				       u32 reg, u32 mask, u32 val)
+{
+	u32 value;
+
+	value = ioread32(stfcamss->isp_base + reg) & ~mask;
+	val &= mask;
+	val |= value;
+	iowrite32(val, stfcamss->isp_base + reg);
+}
+
+static inline void stf_isp_reg_set(struct stfcamss *stfcamss, u32 reg, u32 mask)
+{
+	iowrite32(ioread32(stfcamss->isp_base + reg) | mask,
+		  stfcamss->isp_base + reg);
+}
+
+static inline u32 stf_syscon_reg_read(struct stfcamss *stfcamss, u32 reg)
+{
+	return ioread32(stfcamss->syscon_base + reg);
+}
+
+static inline void stf_syscon_reg_write(struct stfcamss *stfcamss,
+					u32 reg, u32 val)
+{
+	iowrite32(val, stfcamss->syscon_base + reg);
+}
+
+static inline void stf_syscon_reg_set_bit(struct stfcamss *stfcamss,
+					  u32 reg, u32 bit_mask)
+{
+	u32 value;
+
+	value = ioread32(stfcamss->syscon_base + reg);
+	iowrite32(value | bit_mask, stfcamss->syscon_base + reg);
+}
+
+static inline void stf_syscon_reg_clear_bit(struct stfcamss *stfcamss,
+					    u32 reg, u32 bit_mask)
+{
+	u32 value;
+
+	value = ioread32(stfcamss->syscon_base + reg);
+	iowrite32(value & ~bit_mask, stfcamss->syscon_base + reg);
+}
+#endif /* STF_CAMSS_H */
drivers/media/platform/starfive/stf_common.h new file +6 -0
new file mode 100644
index 000000000000..e04fca2d9cd4
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * stf_common.h
+ *
+ * StarFive Camera Subsystem - Common definitions
+ *

drivers/media/platform/starfive/stf_common.h line 16

Quoted 10 lines
drivers/media/platform/starfive/stf_common.h new file +10 -0
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#ifndef STF_COMMON_H
+#define STF_COMMON_H
+
+enum stf_subdev_type {
+	STF_SUBDEV_TYPE_VIN,
+	STF_SUBDEV_TYPE_ISP,
+};

Looking at the whole series, these two values are set in the struct vin_line and stf_isp_dev sdev_type fields, but they are never read. You can drop those two fields, drop this enum, and drop this header.

Quoted 3 lines
drivers/media/platform/starfive/stf_common.h new file +2 -0
+
+#endif /* STF_COMMON_H */
Signature · 3 lines
Regards,

Laurent Pinchart
author
#6 lore raw

Re: [PATCH v6 3/6] media: starfive: Add basic driver

Hi Laurent,

Thank you for your comments.

Quoted 71 lines · Laurent Pinchart

Hi Jack,

Thank you for the patch.

Quoted 66 lines · Jack Zhu

Add basic platform driver for StarFive Camera Subsystem.

---
 MAINTAINERS                                  |   1 +
 drivers/media/platform/Kconfig               |   1 +
 drivers/media/platform/Makefile              |   1 +
 drivers/media/platform/starfive/Kconfig      |  19 +
 drivers/media/platform/starfive/Makefile     |   9 +
 drivers/media/platform/starfive/stf_camss.c  | 372 +++++++++++++++++++
 drivers/media/platform/starfive/stf_camss.h  | 153 ++++++++
 drivers/media/platform/starfive/stf_common.h |  18 +
 8 files changed, 574 insertions(+)
 create mode 100644 drivers/media/platform/starfive/Kconfig
 create mode 100644 drivers/media/platform/starfive/Makefile
 create mode 100644 drivers/media/platform/starfive/stf_camss.c
 create mode 100644 drivers/media/platform/starfive/stf_camss.h
 create mode 100644 drivers/media/platform/starfive/stf_common.h
MAINTAINERS +1 -0
index 71291dc58671..33a752569942 100644
@@ -20077,6 +20077,7 @@ L:	[email protected]
 S:	Maintained
 F:	Documentation/admin-guide/media/starfive_camss.rst
 F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
+F:	drivers/media/platform/starfive/
 
 STARFIVE DEVICETREES
 M:	Emil Renner Berthing <[email protected]>
drivers/media/platform/Kconfig +1 -0
index ee579916f874..627eaa0ab3ee 100644
@@ -80,6 +80,7 @@ source "drivers/media/platform/renesas/Kconfig"
 source "drivers/media/platform/rockchip/Kconfig"
 source "drivers/media/platform/samsung/Kconfig"
 source "drivers/media/platform/st/Kconfig"
+source "drivers/media/platform/starfive/Kconfig"
 source "drivers/media/platform/sunxi/Kconfig"
 source "drivers/media/platform/ti/Kconfig"
 source "drivers/media/platform/verisilicon/Kconfig"
drivers/media/platform/Makefile +1 -0
index 5453bb868e67..5a038498a370 100644
@@ -23,6 +23,7 @@ obj-y += renesas/
 obj-y += rockchip/
 obj-y += samsung/
 obj-y += st/
+obj-y += starfive/
 obj-y += sunxi/
 obj-y += ti/
 obj-y += verisilicon/
drivers/media/platform/starfive/Kconfig new file +4 -0
new file mode 100644
index 000000000000..1a2293475bd2
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+comment "Starfive media platform drivers"
+
Quoted 9 lines

drivers/media/platform/starfive/Kconfig line 9

Quoted 5 lines
drivers/media/platform/starfive/Kconfig new file +5 -0
+config VIDEO_STARFIVE_CAMSS
+	tristate "Starfive Camera Subsystem driver"
+	depends on V4L_PLATFORM_DRIVERS
+	depends on VIDEO_DEV && OF
+	depends on DMA_CMA

I think you can drop this, the driver doesn't depend on DMA. It uses the DMA mapping API though, so you should depend on HAS_DMA instead.

Okay, I will use HAS_DMA.

Quoted 98 lines
Quoted 98 lines
drivers/media/platform/starfive/Kconfig new file +10 -0
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select VIDEOBUF2_DMA_CONTIG
+	select V4L2_FWNODE
+	help
+	   Enable this to support for the Starfive Camera subsystem
+	   found on Starfive JH7110 SoC.
+
+	   To compile this driver as a module, choose M here: the
+	   module will be called stf-camss.
drivers/media/platform/starfive/Makefile new file +9 -0
new file mode 100644
index 000000000000..796775fa52f4
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for StarFive camera subsystem driver.
+#
+
+starfive-camss-objs += \
+		stf_camss.o
+
+obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o \
drivers/media/platform/starfive/stf_camss.c new file +67 -0
new file mode 100644
index 000000000000..cb2b62d49416
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * stf_camss.c
+ *
+ * Starfive Camera Subsystem driver
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/videodev2.h>
+#include <media/media-device.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mc.h>
+
+#include "stf_camss.h"
+
+static const char * const stfcamss_clocks[] = {
+	"clk_apb_func",
+	"clk_wrapper_clk_c",
+	"clk_dvp_inv",
+	"clk_axiwr",
+	"clk_mipi_rx0_pxl",
+	"clk_ispcore_2x",
+	"clk_isp_axi",
+};
+
+static const char * const stfcamss_resets[] = {
+	"rst_wrapper_p",
+	"rst_wrapper_c",
+	"rst_axird",
+	"rst_axiwr",
+	"rst_isp_top_n",
+	"rst_isp_top_axi",
+};
+
+static int stfcamss_get_mem_res(struct platform_device *pdev,
+				struct stfcamss *stfcamss)
+{
+	stfcamss->syscon_base =
+		devm_platform_ioremap_resource_byname(pdev, "syscon");
+	if (IS_ERR(stfcamss->syscon_base))
+		return PTR_ERR(stfcamss->syscon_base);
+
+	stfcamss->isp_base =
+		devm_platform_ioremap_resource_byname(pdev, "isp");
+	if (IS_ERR(stfcamss->isp_base))
+		return PTR_ERR(stfcamss->isp_base);
+
+	return 0;
+}
+
+/*
+ * stfcamss_of_parse_endpoint_node - Parse port endpoint node
+ * @dev: Device
+ * @node: Device node to be parsed
+ * @csd: Parsed data from port endpoint node
+ *
+ * Return 0 on success or a negative error code on failure
+ */
+static int stfcamss_of_parse_endpoint_node(struct device *dev,
+					   struct device_node *node,
+					   struct stfcamss_async_subdev *csd)
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.c line 73

Quoted 6 lines
drivers/media/platform/starfive/stf_camss.c new file +6 -0
+{
+	struct v4l2_fwnode_endpoint vep = { { 0 } };
+
+	v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &vep);
+	dev_dbg(dev, "vep.base.port = 0x%x, id = 0x%x\n",
+		vep.base.port, vep.base.id);

You can print the port and id as decimal numbers, they're indices.

Okay, I will modify it.

Quoted 31 lines
Quoted 31 lines
drivers/media/platform/starfive/stf_camss.c new file +31 -0
+
+	csd->port = vep.base.port;
+
+	return 0;
+}
+
+/*
+ * stfcamss_of_parse_ports - Parse ports node
+ * @stfcamss: STFCAMSS device
+ *
+ * Return number of "port" nodes found in "ports" node
+ */
+static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
+{
+	struct device *dev = stfcamss->dev;
+	struct device_node *node = NULL;
+	struct device_node *remote = NULL;
+	int ret, num_subdevs = 0;
+
+	for_each_endpoint_of_node(dev->of_node, node) {
+		struct stfcamss_async_subdev *csd;
+
+		if (!of_device_is_available(node))
+			continue;
+
+		remote = of_graph_get_remote_port_parent(node);
+		if (!remote) {
+			dev_err(dev, "Cannot get remote parent\n");
+			ret = -EINVAL;
+			goto err_cleanup;
+		}
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.c line 109

Quoted 5 lines
drivers/media/platform/starfive/stf_camss.c new file +5 -0
+
+		csd = v4l2_async_nf_add_fwnode(&stfcamss->notifier,
+					       of_fwnode_handle(remote),
+					       struct stfcamss_async_subdev);
+		of_node_put(remote);

You can use v4l2_async_nf_add_fwnode_remote() to avoid the need to call of_graph_get_remote_port_parent() yourself.

Okay, I will fix it.

Quoted 22 lines
Quoted 22 lines
drivers/media/platform/starfive/stf_camss.c new file +22 -0
+		if (IS_ERR(csd)) {
+			ret = PTR_ERR(csd);
+			goto err_cleanup;
+		}
+
+		ret = stfcamss_of_parse_endpoint_node(dev, node, csd);
+		if (ret < 0)
+			goto err_cleanup;
+
+		num_subdevs++;
+	}
+
+	return num_subdevs;
+
+err_cleanup:
+	of_node_put(node);
+	return ret;
+}
+
+static int stfcamss_subdev_notifier_bound(struct v4l2_async_notifier *async,
+					  struct v4l2_subdev *subdev,
+					  struct v4l2_async_subdev *asd)
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.c line 137

Quoted 6 lines
drivers/media/platform/starfive/stf_camss.c new file +6 -0
+{
+	struct stfcamss *stfcamss =
+		container_of(async, struct stfcamss, notifier);
+	struct host_data *host_data = &stfcamss->host_data;
+	struct media_entity *source;
+	int i, j;

i and j are never negative, you can make them unsigned int.

Okay, I will fix it.

Quoted 7 lines

drivers/media/platform/starfive/stf_camss.c line 139

Quoted 2 lines
drivers/media/platform/starfive/stf_camss.c new file +2 -0
+
+	source = &subdev->entity;

You can assign the variable when declaring it:

	struct media_entity *source = &subdev->entity;

Okay, I will fix it.

Quoted 41 lines
Quoted 22 lines
drivers/media/platform/starfive/stf_camss.c new file +22 -0
+
+	for (i = 0; i < source->num_pads; i++) {
+		if (source->pads[i].flags & MEDIA_PAD_FL_SOURCE)
+			break;
+	}
+
+	if (i == source->num_pads) {
+		dev_err(stfcamss->dev, "No source pad in external entity\n");
+		return -EINVAL;
+	}
+
+	for (j = 0; host_data->host_entity[j] && (j < HOST_ENTITY_MAX); j++) {
+		struct media_entity *input;
+		int ret;
+
+		input = host_data->host_entity[j];
+
+		ret = media_create_pad_link(
+			source,
+			i,
+			input,
+			STF_PAD_SINK,

drivers/media/platform/starfive/stf_camss.c line 171

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+			source->function == MEDIA_ENT_F_CAM_SENSOR ?
+			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED :
+			0);
+		if (ret < 0) {
+			dev_err(stfcamss->dev,
+				"Failed to link %s->%s entities: %d\n",
+				source->name, input->name, ret);
+			return ret;
+		}
+	}

I think you can simplify this. First of all, the port to which the newly bound subdev is attached is available in the stfcamss_async_subdev structure. You can cast the asd to a stfcamss_async_subdev with container_of(), access the port number from there, and get the corresponding sink subdev and pad (for the DVP or CSI-2 port). Then, I think you can use v4l2_create_fwnode_links_to_pad(), which will search for the source pad in the source subdev, instead of doing it yourself.

Quoted 3 lines

I believe the stfcamss.host_data field can be dropped, as it's only used in this function.

Okay, I will simplify the code.

Quoted 26 lines
Quoted 26 lines
drivers/media/platform/starfive/stf_camss.c new file +26 -0
+
+	return 0;
+}
+
+static int stfcamss_subdev_notifier_complete(struct v4l2_async_notifier *ntf)
+{
+	struct stfcamss *stfcamss =
+		container_of(ntf, struct stfcamss, notifier);
+
+	return v4l2_device_register_subdev_nodes(&stfcamss->v4l2_dev);
+}
+
+static const struct v4l2_async_notifier_operations
+stfcamss_subdev_notifier_ops = {
+	.bound = stfcamss_subdev_notifier_bound,
+	.complete = stfcamss_subdev_notifier_complete,
+};
+
+static const struct media_device_ops stfcamss_media_ops = {
+	.link_notify = v4l2_pipeline_link_notify,
+};
+
+static void stfcamss_mc_init(struct platform_device *pdev,
+			     struct stfcamss *stfcamss)
+{
+	stfcamss->media_dev.dev = stfcamss->dev;
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.c line 202

Quoted 5 lines
drivers/media/platform/starfive/stf_camss.c new file +5 -0
+	strscpy(stfcamss->media_dev.model, "Starfive Camera Subsystem",
+		sizeof(stfcamss->media_dev.model));
+	snprintf(stfcamss->media_dev.bus_info,
+		 sizeof(stfcamss->media_dev.bus_info),
+		 "%s:%s", dev_bus_name(&pdev->dev), pdev->name);

No need to set bus_info manually, the framework does if for you in media_device_init().

Okay, I will drop it.

Quoted 5 lines

drivers/media/platform/starfive/stf_camss.c line 203

Quoted 1 line
drivers/media/platform/starfive/stf_camss.c new file +1 -0
+	stfcamss->media_dev.hw_revision = 0x01;

Would the hardware have a version or revision register that you could use to fill this, by any chance ?

There are no revision registers. Currently, this version number is not useful and I will delete it.

Quoted 20 lines
Quoted 7 lines
drivers/media/platform/starfive/stf_camss.c new file +7 -0
+	stfcamss->media_dev.ops = &stfcamss_media_ops;
+	media_device_init(&stfcamss->media_dev);
+
+	stfcamss->v4l2_dev.mdev = &stfcamss->media_dev;
+}
+
+/*

drivers/media/platform/starfive/stf_camss.c line 220

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+ * stfcamss_probe - Probe STFCAMSS platform device
+ * @pdev: Pointer to STFCAMSS platform device
+ *
+ * Return 0 on success or a negative error code on failure
+ */
+static int stfcamss_probe(struct platform_device *pdev)
+{
+	struct stfcamss *stfcamss;
+	struct device *dev = &pdev->dev;
+	int ret = 0, i, num_subdevs;

No need to initialize ret to 0.

Quoted 2 lines

i is never negative, make it an unsigned int.

Okay, I will fix it.

Quoted 4 lines
Quoted 4 lines
drivers/media/platform/starfive/stf_camss.c new file +4 -0
+
+	stfcamss = devm_kzalloc(dev, sizeof(*stfcamss), GFP_KERNEL);
+	if (!stfcamss)
+		return -ENOMEM;
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.c line 230

Quoted 6 lines
drivers/media/platform/starfive/stf_camss.c new file +6 -0
+
+	for (i = 0; i < ARRAY_SIZE(stfcamss->irq); ++i) {
+		stfcamss->irq[i] = platform_get_irq(pdev, i);
+		if (stfcamss->irq[i] < 0)
+			return dev_err_probe(&pdev->dev, stfcamss->irq[i],
+					     "Failed to get clock%d", i);

I think you meant "irq %u", not clock.

Okay, I will fix it.

Quoted 39 lines
Quoted 26 lines
drivers/media/platform/starfive/stf_camss.c new file +26 -0
+	}
+
+	stfcamss->nclks = ARRAY_SIZE(stfcamss->sys_clk);
+	for (i = 0; i < stfcamss->nclks; ++i)
+		stfcamss->sys_clk[i].id = stfcamss_clocks[i];
+	ret = devm_clk_bulk_get(dev, stfcamss->nclks, stfcamss->sys_clk);
+	if (ret) {
+		dev_err(dev, "Failed to get clk controls\n");
+		return ret;
+	}
+
+	stfcamss->nrsts = ARRAY_SIZE(stfcamss->sys_rst);
+	for (i = 0; i < stfcamss->nrsts; ++i)
+		stfcamss->sys_rst[i].id = stfcamss_resets[i];
+	ret = devm_reset_control_bulk_get_shared(dev, stfcamss->nrsts,
+						 stfcamss->sys_rst);
+	if (ret) {
+		dev_err(dev, "Failed to get reset controls\n");
+		return ret;
+	}
+
+	ret = stfcamss_get_mem_res(pdev, stfcamss);
+	if (ret) {
+		dev_err(dev, "Could not map registers\n");
+		return ret;
+	}

drivers/media/platform/starfive/stf_camss.c line 266

Quoted 10 lines
drivers/media/platform/starfive/stf_camss.c new file +10 -0
+
+	stfcamss->dev = dev;
+	platform_set_drvdata(pdev, stfcamss);
+
+	v4l2_async_nf_init(&stfcamss->notifier);
+
+	num_subdevs = stfcamss_of_parse_ports(stfcamss);
+	if (num_subdevs < 0) {
+		dev_err(dev, "Failed to find subdevices\n");
+		return -ENODEV;

I would

Quoted 5 lines
		ret = -ENODEV;
		goto err_cleanup_notifier;

here.

Okay, I will modify the code.

Quoted 21 lines
Quoted 21 lines
drivers/media/platform/starfive/stf_camss.c new file +21 -0
+	}
+
+	stfcamss_mc_init(pdev, stfcamss);
+
+	ret = v4l2_device_register(stfcamss->dev, &stfcamss->v4l2_dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
+		goto err_cleanup_notifier;
+	}
+
+	ret = media_device_register(&stfcamss->media_dev);
+	if (ret) {
+		dev_err(dev, "Failed to register media device: %d\n", ret);
+		goto err_unregister_device;
+	}
+
+	stfcamss->notifier.ops = &stfcamss_subdev_notifier_ops;
+	ret = v4l2_async_nf_register(&stfcamss->v4l2_dev, &stfcamss->notifier);
+	if (ret) {
+		dev_err(dev, "Failed to register async subdev nodes: %d\n",
+			ret);
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.c line 291

Quoted 4 lines
drivers/media/platform/starfive/stf_camss.c new file +4 -0
+		goto err_unregister_media_dev;
+	}
+
+	pm_runtime_enable(dev);

You should enable runtime PM before registering the notifier, otherwise there's a race condition where userspace could start using the device before runtime PM gets enabled.

Okay, will fix it.

Quoted 48 lines
Quoted 48 lines
drivers/media/platform/starfive/stf_camss.c new file +48 -0
+
+	return 0;
+
+err_unregister_media_dev:
+	media_device_unregister(&stfcamss->media_dev);
+err_unregister_device:
+	v4l2_device_unregister(&stfcamss->v4l2_dev);
+err_cleanup_notifier:
+	v4l2_async_nf_cleanup(&stfcamss->notifier);
+	return ret;
+}
+
+/*
+ * stfcamss_remove - Remove STFCAMSS platform device
+ * @pdev: Pointer to STFCAMSS platform device
+ *
+ * Always returns 0.
+ */
+static int stfcamss_remove(struct platform_device *pdev)
+{
+	struct stfcamss *stfcamss = platform_get_drvdata(pdev);
+
+	v4l2_device_unregister(&stfcamss->v4l2_dev);
+	media_device_cleanup(&stfcamss->media_dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id stfcamss_of_match[] = {
+	{ .compatible = "starfive,jh7110-camss" },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, stfcamss_of_match);
+
+static int __maybe_unused stfcamss_runtime_suspend(struct device *dev)
+{
+	struct stfcamss *stfcamss = dev_get_drvdata(dev);
+
+	reset_control_assert(stfcamss->sys_rst[STF_RST_ISP_TOP_AXI].rstc);
+	reset_control_assert(stfcamss->sys_rst[STF_RST_ISP_TOP_N].rstc);
+	clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_ISP_AXI].clk);
+	clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_ISPCORE_2X].clk);
+
+	return 0;
+}
+
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.c line 345

Quoted 6 lines
drivers/media/platform/starfive/stf_camss.c new file +6 -0
+static int __maybe_unused stfcamss_runtime_resume(struct device *dev)
+{
+	struct stfcamss *stfcamss = dev_get_drvdata(dev);
+
+	clk_prepare_enable(stfcamss->sys_clk[STF_CLK_ISPCORE_2X].clk);
+	clk_prepare_enable(stfcamss->sys_clk[STF_CLK_ISP_AXI].clk);

Error handling is missing for those two calls.

Okay, I will add error handing for them.

Quoted 44 lines
Quoted 44 lines
drivers/media/platform/starfive/stf_camss.c new file +27 -0
+	reset_control_deassert(stfcamss->sys_rst[STF_RST_ISP_TOP_N].rstc);
+	reset_control_deassert(stfcamss->sys_rst[STF_RST_ISP_TOP_AXI].rstc);
+
+	return 0;
+}
+
+static const struct dev_pm_ops stfcamss_pm_ops = {
+	SET_RUNTIME_PM_OPS(stfcamss_runtime_suspend,
+			   stfcamss_runtime_resume,
+			   NULL)
+};
+
+static struct platform_driver stfcamss_driver = {
+	.probe = stfcamss_probe,
+	.remove = stfcamss_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.pm = &stfcamss_pm_ops,
+		.of_match_table = of_match_ptr(stfcamss_of_match),
+	},
+};
+
+module_platform_driver(stfcamss_driver);
+
+MODULE_AUTHOR("StarFive Corporation");
+MODULE_DESCRIPTION("StarFive Camera Subsystem driver");
+MODULE_LICENSE("GPL");
drivers/media/platform/starfive/stf_camss.h new file +11 -0
new file mode 100644
index 000000000000..8f39cd65950c
@@ -0,0 +1,153 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * stf_camss.h
+ *
+ * Starfive Camera Subsystem driver
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#ifndef STF_CAMSS_H
+#define STF_CAMSS_H
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.h line 16

Quoted 5 lines
drivers/media/platform/starfive/stf_camss.h new file +5 -0
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/reset.h>
+#include <media/v4l2-device.h>

You should include media/media-device.h, media/media-entity.h and media/v4l2-async.h.

Okay, I will add them.

Quoted 12 lines

drivers/media/platform/starfive/stf_camss.h

Quoted 8 lines
drivers/media/platform/starfive/stf_camss.h new file +6 -0
+	struct device *dev;
 
+};
 
+
+#include "stf_common.h"
+
+#define DRV_NAME     "starfive-camss"

DRV_NAME is used in stfcamss_driver.driver.name only. I would drop the macro and write

Quoted 4 lines

.name = "starfive-camss",

above, it will be more readable.

Yes, I will modify the code.

Quoted 8 lines
Quoted 8 lines
drivers/media/platform/starfive/stf_camss.h new file +8 -0
+#define STF_DVP_NAME "stf_dvp"
+#define STF_CSI_NAME "cdns_csi2rx"
+#define STF_ISP_NAME "stf_isp"
+#define STF_VIN_NAME "stf_vin"
+
+#define STF_PAD_SINK   0
+#define STF_PAD_SRC    1
+#define STF_PADS_NUM   2
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.h line 32

Quoted 4 lines
drivers/media/platform/starfive/stf_camss.h new file +4 -0
+
+enum port_num {
+	PORT_NUMBER_DVP_SENSOR = 0,
+	PORT_NUMBER_CSI2RX

Please name this stf_port_num and STF_PORT_*, to avoid namespace clashes. I would drop "NUMBER_", and name the two ports STF_PORT_DVP and STF_PORT_CSI2RX.

Okay, I will fix it.

Quoted 27 lines
Quoted 27 lines
drivers/media/platform/starfive/stf_camss.h new file +27 -0
+};
+
+enum stf_clk {
+	STF_CLK_APB_FUNC = 0,
+	STF_CLK_WRAPPER_CLK_C,
+	STF_CLK_DVP_INV,
+	STF_CLK_AXIWR,
+	STF_CLK_MIPI_RX0_PXL,
+	STF_CLK_ISPCORE_2X,
+	STF_CLK_ISP_AXI,
+	STF_CLK_NUM
+};
+
+enum stf_rst {
+	STF_RST_WRAPPER_P = 0,
+	STF_RST_WRAPPER_C,
+	STF_RST_AXIRD,
+	STF_RST_AXIWR,
+	STF_RST_ISP_TOP_N,
+	STF_RST_ISP_TOP_AXI,
+	STF_RST_NUM
+};
+
+enum stf_irq {
+	STF_IRQ_VINWR = 0,
+	STF_IRQ_ISP,
+	STF_IRQ_ISPCSIL,
Quoted 9 lines

drivers/media/platform/starfive/stf_camss.h line 65

Quoted 6 lines
drivers/media/platform/starfive/stf_camss.h new file +6 -0
+	STF_IRQ_NUM
+};
+
+#define HOST_ENTITY_MAX		2
+
+struct host_data {

Same here, please name this stf_host_data.

According to your previous suggestion, the optimized new code should not use this structure, and I will delete it.

Quoted 106 lines
Quoted 106 lines
drivers/media/platform/starfive/stf_camss.h new file +88 -0
+	struct media_entity *host_entity[HOST_ENTITY_MAX];
+};
+
+struct stfcamss {
+	struct v4l2_device v4l2_dev;
+	struct media_device media_dev;
+	struct media_pipeline pipe;
+	struct device *dev;
+	struct v4l2_async_notifier notifier;
+	struct host_data host_data;
+	void __iomem *syscon_base;
+	void __iomem *isp_base;
+	int irq[STF_IRQ_NUM];
+	struct clk_bulk_data sys_clk[STF_CLK_NUM];
+	int nclks;
+	struct reset_control_bulk_data sys_rst[STF_RST_NUM];
+	int nrsts;
+};
+
+struct stfcamss_async_subdev {
+	struct v4l2_async_subdev asd;  /* must be first */
+	enum port_num port;
+};
+
+static inline u32 stf_isp_reg_read(struct stfcamss *stfcamss, u32 reg)
+{
+	return ioread32(stfcamss->isp_base + reg);
+}
+
+static inline void stf_isp_reg_write(struct stfcamss *stfcamss,
+				     u32 reg, u32 val)
+{
+	iowrite32(val, stfcamss->isp_base + reg);
+}
+
+static inline void stf_isp_reg_write_delay(struct stfcamss *stfcamss,
+					   u32 reg, u32 val, u32 delay)
+{
+	iowrite32(val, stfcamss->isp_base + reg);
+	usleep_range(1000 * delay, 1000 * delay + 100);
+}
+
+static inline void stf_isp_reg_set_bit(struct stfcamss *stfcamss,
+				       u32 reg, u32 mask, u32 val)
+{
+	u32 value;
+
+	value = ioread32(stfcamss->isp_base + reg) & ~mask;
+	val &= mask;
+	val |= value;
+	iowrite32(val, stfcamss->isp_base + reg);
+}
+
+static inline void stf_isp_reg_set(struct stfcamss *stfcamss, u32 reg, u32 mask)
+{
+	iowrite32(ioread32(stfcamss->isp_base + reg) | mask,
+		  stfcamss->isp_base + reg);
+}
+
+static inline u32 stf_syscon_reg_read(struct stfcamss *stfcamss, u32 reg)
+{
+	return ioread32(stfcamss->syscon_base + reg);
+}
+
+static inline void stf_syscon_reg_write(struct stfcamss *stfcamss,
+					u32 reg, u32 val)
+{
+	iowrite32(val, stfcamss->syscon_base + reg);
+}
+
+static inline void stf_syscon_reg_set_bit(struct stfcamss *stfcamss,
+					  u32 reg, u32 bit_mask)
+{
+	u32 value;
+
+	value = ioread32(stfcamss->syscon_base + reg);
+	iowrite32(value | bit_mask, stfcamss->syscon_base + reg);
+}
+
+static inline void stf_syscon_reg_clear_bit(struct stfcamss *stfcamss,
+					    u32 reg, u32 bit_mask)
+{
+	u32 value;
+
+	value = ioread32(stfcamss->syscon_base + reg);
+	iowrite32(value & ~bit_mask, stfcamss->syscon_base + reg);
+}
+#endif /* STF_CAMSS_H */
drivers/media/platform/starfive/stf_common.h new file +12 -0
new file mode 100644
index 000000000000..e04fca2d9cd4
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * stf_common.h
+ *
+ * StarFive Camera Subsystem - Common definitions
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#ifndef STF_COMMON_H
+#define STF_COMMON_H
+
Quoted 9 lines

drivers/media/platform/starfive/stf_common.h line 16

Quoted 4 lines
drivers/media/platform/starfive/stf_common.h new file +4 -0
+enum stf_subdev_type {
+	STF_SUBDEV_TYPE_VIN,
+	STF_SUBDEV_TYPE_ISP,
+};

Looking at the whole series, these two values are set in the struct vin_line and stf_isp_dev sdev_type fields, but they are never read. You can drop those two fields, drop this enum, and drop this header.

Okay, I will drop them.

Quoted 4 lines
Quoted 3 lines
drivers/media/platform/starfive/stf_common.h new file +2 -0
+
+#endif /* STF_COMMON_H */
#7 lore raw

Re: [PATCH v6 3/6] media: starfive: Add basic driver

In reply to Jack Zhu
Quoted 4 lines · Jack Zhu

Add basic platform driver for StarFive Camera Subsystem.

One thing here is the patch title and hence commit message.

"media: starfive:" doesn't really tell you this is a camera versus say a encoder/decoder.

I see you've used the name "camss" for your driver, which I think is a perfectly good and logical choice - however if you started to make commits along the lines of "media: camss" that would conflict with the qcom camss.

How about for starfive and qcom by the way, we do what Laurent did in

commit 3e8537b4c15172bfe1b285c3155ed5c37d523cd3

Author: Laurent Pinchart <[email protected]>
Date:   Wed Dec 21 10:33:39 2022 +0100

media: ti: omap3isp: Use media_pipeline_for_each_entity()

i.e. future StarFive commits for camera would be "media: starfive: camss" and similarly for Qualcomm "media: qualcomm: camss"

The point being the commit title namespace should be instructive and specific re: 3e8537b4c15172bfe1b285c3155ed5c37d523cd3

---
bod

author
#8 lore raw

Re: [PATCH v6 3/6] media: starfive: Add basic driver

Hi Bryan,

Thank you for your comments.

Quoted 18 lines · Bryan O'Donoghue
Quoted 4 lines · Jack Zhu

Add basic platform driver for StarFive Camera Subsystem.

One thing here is the patch title and hence commit message.

"media: starfive:" doesn't really tell you this is a camera versus say a encoder/decoder.

I see you've used the name "camss" for your driver, which I think is a perfectly good and logical choice - however if you started to make commits along the lines of "media: camss" that would conflict with the qcom camss.

How about for starfive and qcom by the way, we do what Laurent did in

commit 3e8537b4c15172bfe1b285c3155ed5c37d523cd3
Author: Laurent Pinchart <[email protected]> Date:   Wed Dec 21 10:33:39 2022 +0100

Quoted 6 lines

    media: ti: omap3isp: Use media_pipeline_for_each_entity()

i.e. future StarFive commits for camera would be "media: starfive: camss" and similarly for Qualcomm "media: qualcomm: camss"

The point being the commit title namespace should be instructive and specific re: 3e8537b4c15172bfe1b285c3155ed5c37d523cd3

Okay, I will add a camss directory under the starfive directory and modify the patch title.

Quoted 2 lines

---
bod

Signature · 3 lines
Regards,

Jack Zhu
author
#9 lore raw

PATCH v6 4/6 media: starfive: Add video driver

In reply to Jack Zhu

Add video driver for StarFive Camera Subsystem.

---
 drivers/media/platform/starfive/Makefile    |   3 +-
 drivers/media/platform/starfive/stf_video.c | 864 ++++++++++++++++++++
 drivers/media/platform/starfive/stf_video.h |  95 +++
 3 files changed, 961 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/platform/starfive/stf_video.c
 create mode 100644 drivers/media/platform/starfive/stf_video.h
drivers/media/platform/starfive/Makefile +2 -1
index 796775fa52f4..d7a42b3a938c 100644
@@ -4,6 +4,7 @@
 #
 
 starfive-camss-objs += \
-		stf_camss.o
+		stf_camss.o \
+		stf_video.o
 
 obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o \
drivers/media/platform/starfive/stf_video.c new file +864 -0

864 lines not on this page · raw message

drivers/media/platform/starfive/stf_video.h new file +95 -0

95 lines not on this page · raw message

Signature · 1 line
2.34.1
#10 lore raw

Re: [PATCH v6 4/6] media: starfive: Add video driver

Hi Jack,

Thank you for the patch.

Quoted 52 lines · Jack Zhu

Add video driver for StarFive Camera Subsystem.

---
 drivers/media/platform/starfive/Makefile    |   3 +-
 drivers/media/platform/starfive/stf_video.c | 864 ++++++++++++++++++++
 drivers/media/platform/starfive/stf_video.h |  95 +++
 3 files changed, 961 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/platform/starfive/stf_video.c
 create mode 100644 drivers/media/platform/starfive/stf_video.h
drivers/media/platform/starfive/Makefile +2 -1
index 796775fa52f4..d7a42b3a938c 100644
@@ -4,6 +4,7 @@
 #
 
 starfive-camss-objs += \
-		stf_camss.o
+		stf_camss.o \
+		stf_video.o
 
 obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o \
drivers/media/platform/starfive/stf_video.c new file +22 -0
new file mode 100644
index 000000000000..f027c7308dfb
@@ -0,0 +1,864 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * stf_video.c
+ *
+ * StarFive Camera Subsystem - V4L2 device node
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-mc.h>
+#include <media/videobuf2-dma-contig.h>
+
+#include "stf_camss.h"
+#include "stf_video.h"
+
+static const struct stfcamss_format_info formats_pix_wr[] = {
+	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10, 1,
+	  { { 1, 1 } }, { { 1, 1 } }, { 10 } },
+	{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10, 1,

drivers/media/platform/starfive/stf_video.c line 32

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+	  { { 1, 1 } }, { { 1, 1 } }, { 10 } },
+	{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10, 1,
+	  { { 1, 1 } }, { { 1, 1 } }, { 10 } },
+	{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10, 1,
+	  { { 1, 1 } }, { { 1, 1 } }, { 10 } },
+};
+
+static const struct stfcamss_format_info formats_pix_isp[] = {
+	{ MEDIA_BUS_FMT_Y12_1X12, V4L2_PIX_FMT_NV12, 1,
+	  { { 1, 1 } }, { { 2, 3 } }, { 8 } },

The [hv]sub values look weird. NV12 subsamples by 2 in both directions for the chroma plane. The result of your bytesperline calculation is right before the chroma plane is subsampled by 2 horizontally, but contains both U and V components, so the number of bytes equals the width in pixels for both the luma and chroma planes. In the vertical direction, the calculation is fine too for NV12 as you're calculating sizeimage for both planes.

I would recommend something along the following lines:

struct stfcamss_format_info {
	u32 code;
	u32 pixelformat;
	u8 planes;
	u8 vsub[3];
	u8 bpp;
};
static const struct stfcamss_format_info formats_pix_isp[] = {
	{
		.code = MEDIA_BUS_FMT_Y12_1X12,
		.pixelformat = V4L2_PIX_FMT_NV12,
		.planes = 2,
		.vsub = { 1, 2 },
		.bpp = 8,
	},
};
{
	unsigned int bytesperline;
	unsigned int sizeimage = 0;

	bytesperline = width * info->bpp / 8;

	for (i = 0; i < info->planes; ++i)
		sizeimage += bytesperline * height / info->vsub[i];
}

drivers/media/platform/starfive/stf_video.c line 41

Quoted 9 lines
drivers/media/platform/starfive/stf_video.c new file +9 -0
+};
+
+/* -----------------------------------------------------------------------------
+ * Helper functions
+ */
+
+static int video_find_format(u32 code, u32 pixelformat,
+			     const struct stfcamss_format_info *formats,
+			     unsigned int nformats)

I would pass the stfcamss_video pointer to this function instead of formats and nformats.

Quoted 260 lines
drivers/media/platform/starfive/stf_video.c new file +260 -0
+{
+	int i;
+
+	for (i = 0; i < nformats; i++) {
+		if (formats[i].code == code &&
+		    formats[i].pixelformat == pixelformat)
+			return i;
+	}
+
+	for (i = 0; i < nformats; i++)
+		if (formats[i].code == code)
+			return i;
+
+	for (i = 0; i < nformats; i++)
+		if (formats[i].pixelformat == pixelformat)
+			return i;
+
+	return -EINVAL;
+}
+
+static int __video_try_fmt(struct stfcamss_video *video, struct v4l2_format *f)
+{
+	struct v4l2_pix_format *pix;
+	const struct stfcamss_format_info *fi;
+	u32 width, height;
+	u32 bpl;
+	int i;
+
+	pix = &f->fmt.pix;
+
+	for (i = 0; i < video->nformats; i++)
+		if (pix->pixelformat == video->formats[i].pixelformat)
+			break;
+
+	if (i == video->nformats)
+		i = 0; /* default format */
+
+	fi = &video->formats[i];
+	width = pix->width;
+	height = pix->height;
+
+	memset(pix, 0, sizeof(*pix));
+
+	pix->pixelformat = fi->pixelformat;
+	pix->width = clamp_t(u32, width, STFCAMSS_FRAME_MIN_WIDTH,
+			     STFCAMSS_FRAME_MAX_WIDTH);
+	pix->height = clamp_t(u32, height, STFCAMSS_FRAME_MIN_HEIGHT,
+			      STFCAMSS_FRAME_MAX_HEIGHT);
+	bpl = pix->width / fi->hsub[0].numerator *
+	      fi->hsub[0].denominator * fi->bpp[0] / 8;
+	bpl = ALIGN(bpl, video->bpl_alignment);
+	pix->bytesperline = bpl;
+	pix->sizeimage = pix->height / fi->vsub[0].numerator *
+			 fi->vsub[0].denominator * bpl;
+
+	pix->field = V4L2_FIELD_NONE;
+	pix->colorspace = V4L2_COLORSPACE_SRGB;
+	pix->flags = 0;
+	pix->ycbcr_enc =
+		V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
+	pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+							  pix->colorspace,
+							  pix->ycbcr_enc);
+	pix->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(pix->colorspace);
+
+	return 0;
+}
+
+static int stf_video_init_format(struct stfcamss_video *video)
+{
+	int ret;
+	struct v4l2_format format = {
+		.type = video->type,
+		.fmt.pix = {
+			.width = 1920,
+			.height = 1080,
+			.pixelformat = V4L2_PIX_FMT_RGB565,
+		},
+	};
+
+	ret = __video_try_fmt(video, &format);
+
+	if (ret < 0)
+		return ret;
+
+	video->active_fmt = format;
+
+	return 0;
+}
+
+/* -----------------------------------------------------------------------------
+ * Video queue operations
+ */
+
+static int video_queue_setup(struct vb2_queue *q,
+			     unsigned int *num_buffers,
+			     unsigned int *num_planes,
+			     unsigned int sizes[],
+			     struct device *alloc_devs[])
+{
+	struct stfcamss_video *video = vb2_get_drv_priv(q);
+	const struct v4l2_pix_format *format = &video->active_fmt.fmt.pix;
+
+	if (*num_planes) {
+		if (*num_planes != 1)
+			return -EINVAL;
+
+		if (sizes[0] < format->sizeimage)
+			return -EINVAL;
+	}
+
+	*num_planes = 1;
+	sizes[0] = format->sizeimage;
+	if (!sizes[0])
+		dev_err(video->stfcamss->dev,
+			"%s: error size is zero!!!\n", __func__);
+
+	dev_dbg(video->stfcamss->dev, "planes = %d, size = %d\n",
+		*num_planes, sizes[0]);
+
+	return 0;
+}
+
+static int video_buf_init(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
+	struct stfcamss_buffer *buffer =
+		container_of(vbuf, struct stfcamss_buffer, vb);
+	const struct v4l2_pix_format *fmt = &video->active_fmt.fmt.pix;
+	dma_addr_t *paddr;
+
+	buffer->sizeimage = 0;
+
+	paddr = vb2_plane_cookie(vb, 0);
+	buffer->sizeimage = vb2_plane_size(vb, 0);
+	buffer->addr[0] = *paddr;
+	if (fmt->pixelformat == V4L2_PIX_FMT_NV12 ||
+	    fmt->pixelformat == V4L2_PIX_FMT_NV21 ||
+	    fmt->pixelformat == V4L2_PIX_FMT_NV16 ||
+	    fmt->pixelformat == V4L2_PIX_FMT_NV61)
+		buffer->addr[1] =
+			buffer->addr[0] + fmt->bytesperline * fmt->height;
+
+	return 0;
+}
+
+static int video_buf_prepare(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
+	const struct v4l2_pix_format *fmt = &video->active_fmt.fmt.pix;
+
+	if (fmt->sizeimage > vb2_plane_size(vb, 0)) {
+		dev_err(video->stfcamss->dev,
+			"sizeimage = %d, plane size = %d\n",
+			fmt->sizeimage, (unsigned int)vb2_plane_size(vb, 0));
+		return -EINVAL;
+	}
+	vb2_set_plane_payload(vb, 0, fmt->sizeimage);
+
+	vbuf->field = V4L2_FIELD_NONE;
+
+	return 0;
+}
+
+static void video_buf_queue(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
+	struct stfcamss_buffer *buffer =
+		container_of(vbuf, struct stfcamss_buffer, vb);
+
+	video->ops->queue_buffer(video, buffer);
+}
+
+/*
+ * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
+ * @mbus: v4l2_mbus_framefmt format (input)
+ * @pix: v4l2_pix_format_mplane format (output)
+ * @f: a pointer to formats array element to be used for the conversion
+ * @alignment: bytesperline alignment value
+ *
+ * Fill the output pix structure with information from the input mbus format.
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int video_mbus_to_pix(const struct v4l2_mbus_framefmt *mbus,
+			     struct v4l2_pix_format *pix,
+			     const struct stfcamss_format_info *f,
+			     unsigned int alignment)
+{
+	u32 bytesperline;
+
+	memset(pix, 0, sizeof(*pix));
+	v4l2_fill_pix_format(pix, mbus);
+	pix->pixelformat = f->pixelformat;
+	bytesperline = pix->width / f->hsub[0].numerator *
+		       f->hsub[0].denominator * f->bpp[0] / 8;
+	bytesperline = ALIGN(bytesperline, alignment);
+	pix->bytesperline = bytesperline;
+	pix->sizeimage = pix->height / f->vsub[0].numerator *
+			 f->vsub[0].denominator * bytesperline;
+	return 0;
+}
+
+static struct v4l2_subdev *video_remote_subdev(struct stfcamss_video *video,
+					       u32 *pad)
+{
+	struct media_pad *remote;
+
+	remote = media_pad_remote_pad_first(&video->pad);
+
+	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
+		return NULL;
+
+	if (pad)
+		*pad = remote->index;
+
+	return media_entity_to_v4l2_subdev(remote->entity);
+}
+
+static int video_get_subdev_format(struct stfcamss_video *video,
+				   struct v4l2_format *format)
+{
+	struct v4l2_pix_format *pix = &video->active_fmt.fmt.pix;
+	struct v4l2_subdev_format fmt;
+	struct v4l2_subdev *subdev;
+	u32 pixelformat;
+	u32 pad;
+	int ret;
+
+	subdev = video_remote_subdev(video, &pad);
+	if (!subdev)
+		return -EPIPE;
+
+	fmt.pad = pad;
+	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret)
+		return ret;
+
+	pixelformat = pix->pixelformat;
+	ret = video_find_format(fmt.format.code, pixelformat,
+				video->formats, video->nformats);
+	if (ret < 0)
+		return ret;
+
+	format->type = video->type;
+
+	return video_mbus_to_pix(&fmt.format, &format->fmt.pix,
+				 &video->formats[ret], video->bpl_alignment);
+}
+
+static int video_check_format(struct stfcamss_video *video)
+{
+	struct v4l2_pix_format *pix = &video->active_fmt.fmt.pix;
+	struct v4l2_format format;
+	struct v4l2_pix_format *sd_pix = &format.fmt.pix;

drivers/media/platform/starfive/stf_video.c line 311

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+	int ret;
+
+	sd_pix->pixelformat = pix->pixelformat;
+	ret = video_get_subdev_format(video, &format);
+	if (ret < 0)
+		return ret;
+
+	if (pix->pixelformat != sd_pix->pixelformat ||
+	    pix->height > sd_pix->height ||
+	    pix->width > sd_pix->width ||

The width and height must be identical.

Quoted 40 lines
drivers/media/platform/starfive/stf_video.c new file +40 -0
+	    pix->field != format.fmt.pix.field) {
+		dev_err(video->stfcamss->dev,
+			"%s, not match:\n"
+			"0x%x 0x%x\n0x%x 0x%x\n0x%x 0x%x\n",
+			__func__,
+			pix->pixelformat, sd_pix->pixelformat,
+			pix->height, sd_pix->height,
+			pix->field, format.fmt.pix.field);
+		return -EPIPE;
+	}
+
+	return 0;
+}
+
+static int video_start_streaming(struct vb2_queue *q, unsigned int count)
+{
+	struct stfcamss_video *video = vb2_get_drv_priv(q);
+	struct video_device *vdev = &video->vdev;
+	struct media_entity *entity;
+	struct media_pad *pad;
+	struct v4l2_subdev *subdev;
+	int ret;
+
+	ret = video_device_pipeline_start(vdev, &video->stfcamss->pipe);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to media_pipeline_start: %d\n", ret);
+		return ret;
+	}
+
+	ret = video_check_format(video);
+	if (ret < 0)
+		goto error;
+	entity = &vdev->entity;
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_pad_remote_pad_first(pad);

drivers/media/platform/starfive/stf_video.c line 361

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		ret = v4l2_subdev_call(subdev, video, s_stream, 1);
+		if (ret < 0 && ret != -ENOIOCTLCMD)
+			goto error;
+	}

While you're free to decide how to implement the start streaming operation for all the subdevs in your driver, you must not call .s_stream() on all *external* subdevs. Imagine a case where the device connected to your VIN input would be a parallel-to-CSI-2 bridge. This driver must call .s_stream() on the bridge, but must not call .s_stream() on the device at the input of the bridge, as the bridge driver will propagate the .s_stream() call itself.

I will be able to advice better on how to handle stream start once you provide the media graph of the device.

Quoted 50 lines
drivers/media/platform/starfive/stf_video.c new file +50 -0
+	return 0;
+
+error:
+	video_device_pipeline_stop(vdev);
+	video->ops->flush_buffers(video, VB2_BUF_STATE_QUEUED);
+	return ret;
+}
+
+static void video_stop_streaming(struct vb2_queue *q)
+{
+	struct stfcamss_video *video = vb2_get_drv_priv(q);
+	struct video_device *vdev = &video->vdev;
+	struct media_entity *entity;
+	struct media_pad *pad;
+	struct v4l2_subdev *subdev;
+
+	entity = &vdev->entity;
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_pad_remote_pad_first(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		v4l2_subdev_call(subdev, video, s_stream, 0);
+	}
+
+	video_device_pipeline_stop(vdev);
+	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
+}
+
+static const struct vb2_ops stf_video_vb2_q_ops = {
+	.queue_setup     = video_queue_setup,
+	.wait_prepare    = vb2_ops_wait_prepare,
+	.wait_finish     = vb2_ops_wait_finish,
+	.buf_init        = video_buf_init,
+	.buf_prepare     = video_buf_prepare,
+	.buf_queue       = video_buf_queue,
+	.start_streaming = video_start_streaming,
+	.stop_streaming  = video_stop_streaming,
+};
+
+/* -----------------------------------------------------------------------------
+ * V4L2 ioctls
+ */

drivers/media/platform/starfive/stf_video.c line 421

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+
+static int video_querycap(struct file *file, void *fh,
+			  struct v4l2_capability *cap)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+
+	strscpy(cap->driver, "stf camss", sizeof(cap->driver));
+	strscpy(cap->card, "Starfive Camera Subsystem", sizeof(cap->card));
+	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
+		 dev_name(video->stfcamss->dev));

No need to set bus_info manually, it's done automatically in v4l_querycap().

Quoted 140 lines
drivers/media/platform/starfive/stf_video.c new file +140 -0
+	return 0;
+}
+
+static int video_get_pfmt_by_index(struct stfcamss_video *video, int ndx)
+{
+	int i, j, k;
+
+	/* find index "i" of "k"th unique pixelformat in formats array */
+	k = -1;
+	for (i = 0; i < video->nformats; i++) {
+		for (j = 0; j < i; j++) {
+			if (video->formats[i].pixelformat ==
+			    video->formats[j].pixelformat)
+				break;
+		}
+
+		if (j == i)
+			k++;
+
+		if (k == ndx)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int video_get_pfmt_by_mcode(struct stfcamss_video *video, u32 mcode)
+{
+	int i;
+
+	for (i = 0; i < video->nformats; i++) {
+		if (video->formats[i].code == mcode)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	int i;
+
+	if (f->type != video->type)
+		return -EINVAL;
+	if (f->index >= video->nformats)
+		return -EINVAL;
+
+	if (f->mbus_code) {
+		/* Each entry in formats[] table has unique mbus_code */
+		if (f->index > 0)
+			return -EINVAL;
+
+		i = video_get_pfmt_by_mcode(video, f->mbus_code);
+	} else {
+		i = video_get_pfmt_by_index(video, f->index);
+	}
+
+	if (i < 0)
+		return -EINVAL;
+
+	f->pixelformat = video->formats[i].pixelformat;
+
+	return 0;
+}
+
+static int video_enum_framesizes(struct file *file, void *fh,
+				 struct v4l2_frmsizeenum *fsize)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	int i;
+
+	if (fsize->index)
+		return -EINVAL;
+
+	for (i = 0; i < video->nformats; i++) {
+		if (video->formats[i].pixelformat == fsize->pixel_format)
+			break;
+	}
+
+	if (i == video->nformats)
+		return -EINVAL;
+
+	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
+	fsize->stepwise.min_width = STFCAMSS_FRAME_MIN_WIDTH;
+	fsize->stepwise.max_width = STFCAMSS_FRAME_MAX_WIDTH;
+	fsize->stepwise.min_height = STFCAMSS_FRAME_MIN_HEIGHT;
+	fsize->stepwise.max_height = STFCAMSS_FRAME_MAX_HEIGHT;
+	fsize->stepwise.step_width = 1;
+	fsize->stepwise.step_height = 1;
+
+	return 0;
+}
+
+static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+
+	*f = video->active_fmt;
+
+	return 0;
+}
+
+static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	int ret;
+
+	if (vb2_is_busy(&video->vb2_q))
+		return -EBUSY;
+
+	ret = __video_try_fmt(video, f);
+	if (ret < 0)
+		return ret;
+
+	video->active_fmt = *f;
+
+	return 0;
+}
+
+static int video_try_fmt(struct file *file, void *fh, struct v4l2_format *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+
+	return __video_try_fmt(video, f);
+}
+
+static int video_enum_input(struct file *file, void *fh,
+			    struct v4l2_input *input)
+{
+	if (input->index > 0)
+		return -EINVAL;
+
+	strscpy(input->name, "camera", sizeof(input->name));
+	input->type = V4L2_INPUT_TYPE_CAMERA;
+
+	return 0;
+}
+
+static int video_g_input(struct file *file, void *fh, unsigned int *input)

drivers/media/platform/starfive/stf_video.c line 571

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+{
+	*input = 0;
+
+	return 0;
+}
+
+static int video_s_input(struct file *file, void *fh, unsigned int input)
+{
+	return input == 0 ? 0 : -EINVAL;
+}

You can drop enum_input, g_input and s_input, they make little sense for this device.

Quoted 15 lines
drivers/media/platform/starfive/stf_video.c new file +15 -0
+
+static int video_g_parm(struct file *file, void *priv,
+			struct v4l2_streamparm *p)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	struct video_device *vdev = &video->vdev;
+	struct media_entity *entity;
+	struct v4l2_subdev *subdev;
+	struct media_pad *pad;
+	int ret, is_support = 0;
+
+	entity = &vdev->entity;
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))

drivers/media/platform/starfive/stf_video.c line 596

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+			break;
+
+		pad = media_pad_remote_pad_first(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		ret = v4l2_g_parm_cap(vdev, subdev, p);

Unless I'm missing something, the g_parm operation isn't implemented in the connected subdev (VIN or ISP), and neither is s_parm. You can just drop video_g_parm() and video_s_parm(), you don't have to implement them.

Quoted 89 lines
drivers/media/platform/starfive/stf_video.c new file +89 -0
+		if (ret < 0 && ret != -ENOIOCTLCMD)
+			break;
+		if (!ret)
+			is_support = 1;
+	}
+
+	return is_support ? 0 : ret;
+}
+
+static int video_s_parm(struct file *file, void *priv,
+			struct v4l2_streamparm *p)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	struct video_device *vdev = &video->vdev;
+	struct media_entity *entity;
+	struct v4l2_subdev *subdev;
+	struct media_pad *pad;
+	struct v4l2_streamparm tmp_p;
+	int ret, is_support = 0;
+
+	entity = &vdev->entity;
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_pad_remote_pad_first(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		tmp_p = *p;
+		ret = v4l2_s_parm_cap(vdev, subdev, &tmp_p);
+		if (ret < 0 && ret != -ENOIOCTLCMD)
+			break;
+		if (!ret) {
+			is_support = 1;
+			*p = tmp_p;
+		}
+	}
+
+	return is_support ? 0 : ret;
+}
+
+static const struct v4l2_ioctl_ops stf_vid_vin_ioctl_ops = {
+	.vidioc_querycap                = video_querycap,
+	.vidioc_enum_fmt_vid_cap        = video_enum_fmt,
+	.vidioc_enum_fmt_vid_out        = video_enum_fmt,
+	.vidioc_enum_framesizes         = video_enum_framesizes,
+	.vidioc_g_fmt_vid_cap           = video_g_fmt,
+	.vidioc_s_fmt_vid_cap           = video_s_fmt,
+	.vidioc_try_fmt_vid_cap         = video_try_fmt,
+	.vidioc_g_fmt_vid_out           = video_g_fmt,
+	.vidioc_s_fmt_vid_out           = video_s_fmt,
+	.vidioc_try_fmt_vid_out         = video_try_fmt,
+	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
+	.vidioc_querybuf                = vb2_ioctl_querybuf,
+	.vidioc_qbuf                    = vb2_ioctl_qbuf,
+	.vidioc_expbuf                  = vb2_ioctl_expbuf,
+	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
+	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
+	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
+	.vidioc_streamon                = vb2_ioctl_streamon,
+	.vidioc_streamoff               = vb2_ioctl_streamoff,
+	.vidioc_enum_input              = video_enum_input,
+	.vidioc_g_input                 = video_g_input,
+	.vidioc_s_input                 = video_s_input,
+	.vidioc_g_parm                  = video_g_parm,
+	.vidioc_s_parm                  = video_s_parm,
+};
+
+static const struct v4l2_ioctl_ops stf_vid_isp_ioctl_ops = {
+	.vidioc_querycap                = video_querycap,
+	.vidioc_enum_fmt_vid_cap        = video_enum_fmt,
+	.vidioc_enum_fmt_vid_out        = video_enum_fmt,
+	.vidioc_enum_framesizes         = video_enum_framesizes,
+	.vidioc_g_fmt_vid_cap           = video_g_fmt,
+	.vidioc_s_fmt_vid_cap           = video_s_fmt,
+	.vidioc_try_fmt_vid_cap         = video_try_fmt,
+	.vidioc_g_fmt_vid_out           = video_g_fmt,
+	.vidioc_s_fmt_vid_out           = video_s_fmt,
+	.vidioc_try_fmt_vid_out         = video_try_fmt,
+	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
+	.vidioc_querybuf                = vb2_ioctl_querybuf,
+	.vidioc_qbuf                    = vb2_ioctl_qbuf,
+	.vidioc_expbuf                  = vb2_ioctl_expbuf,
+	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,

drivers/media/platform/starfive/stf_video.c line 695

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
+	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
+	.vidioc_streamon                = vb2_ioctl_streamon,
+	.vidioc_streamoff               = vb2_ioctl_streamoff,
+	.vidioc_enum_input              = video_enum_input,
+	.vidioc_g_input                 = video_g_input,
+	.vidioc_s_input                 = video_s_input,
+	.vidioc_g_parm                  = video_g_parm,
+	.vidioc_s_parm                  = video_s_parm,
+};

Unless I'm mistaken the two structures are identical, you can have a single one.

Quoted 16 lines
drivers/media/platform/starfive/stf_video.c new file +16 -0
+
+/* -----------------------------------------------------------------------------
+ * V4L2 file operations
+ */
+
+static int video_open(struct file *file)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct stfcamss_video *video = video_drvdata(file);
+	struct v4l2_fh *vfh;
+	int ret;
+
+	mutex_lock(&video->lock);
+
+	vfh = kzalloc(sizeof(*vfh), GFP_KERNEL);
+	if (!vfh) {

drivers/media/platform/starfive/stf_video.c line 721

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+		ret = -ENOMEM;
+		goto error_alloc;
+	}
+
+	v4l2_fh_init(vfh, vdev);
+	v4l2_fh_add(vfh);
+
+	file->private_data = vfh;
+
+	ret = v4l2_pipeline_pm_get(&vdev->entity);

This is a deprecated API, you can drop the call. It will greatly simplify video_open(), which can be replaced with v4l2_fh_open(). Same for release, you can use v4l2_fh_release().

Regarding power management, you should use runtime PM, calling pm_runtime_resume_and_get() when starting streaming (in video_start_streaming()) and pm_runtime_put() when stopping streaming. As a result of dropping v4l2_pipeline_pm_get() the .s_power() subdev operations won't be called automatically. They are deprecated, so they should be dropped, and the relevant code moved to the PM runtime resume/suspend handlers. You will be able to drop the power_count fields.

Quoted 151 lines
drivers/media/platform/starfive/stf_video.c new file +143 -0
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to power up pipeline: %d\n", ret);
+		goto error_pm_use;
+	}
+	mutex_unlock(&video->lock);
+
+	return 0;
+
+error_pm_use:
+	v4l2_fh_release(file);
+error_alloc:
+	mutex_unlock(&video->lock);
+	return ret;
+}
+
+static int video_release(struct file *file)
+{
+	struct video_device *vdev = video_devdata(file);
+
+	vb2_fop_release(file);
+	v4l2_pipeline_pm_put(&vdev->entity);
+	file->private_data = NULL;
+
+	return 0;
+}
+
+static const struct v4l2_file_operations stf_vid_fops = {
+	.owner          = THIS_MODULE,
+	.unlocked_ioctl = video_ioctl2,
+	.open           = video_open,
+	.release        = video_release,
+	.poll           = vb2_fop_poll,
+	.mmap           = vb2_fop_mmap,
+	.read           = vb2_fop_read,
+};
+
+/* -----------------------------------------------------------------------------
+ * STFCAMSS video core
+ */
+
+static void stf_video_release(struct video_device *vdev)
+{
+	struct stfcamss_video *video = video_get_drvdata(vdev);
+
+	media_entity_cleanup(&vdev->entity);
+
+	mutex_destroy(&video->q_lock);
+	mutex_destroy(&video->lock);
+}
+
+int stf_video_register(struct stfcamss_video *video,
+		       struct v4l2_device *v4l2_dev, const char *name)
+{
+	struct video_device *vdev;
+	struct vb2_queue *q;
+	struct media_pad *pad = &video->pad;
+	int ret;
+
+	vdev = &video->vdev;
+
+	mutex_init(&video->q_lock);
+
+	q = &video->vb2_q;
+	q->drv_priv = video;
+	q->mem_ops = &vb2_dma_contig_memops;
+	q->ops = &stf_video_vb2_q_ops;
+	q->type = video->type;
+	q->io_modes = VB2_DMABUF | VB2_MMAP | VB2_READ;
+	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	q->buf_struct_size = sizeof(struct stfcamss_buffer);
+	q->dev = video->stfcamss->dev;
+	q->lock = &video->q_lock;
+	q->min_buffers_needed = STFCAMSS_MIN_BUFFERS;
+	ret = vb2_queue_init(q);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to init vb2 queue: %d\n", ret);
+		goto err_vb2_init;
+	}
+
+	pad->flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_pads_init(&vdev->entity, 1, pad);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to init video entity: %d\n", ret);
+		goto err_vb2_init;
+	}
+
+	mutex_init(&video->lock);
+
+	if (video->id == STF_V_LINE_WR) {
+		video->formats = formats_pix_wr;
+		video->nformats = ARRAY_SIZE(formats_pix_wr);
+		video->bpl_alignment = STFCAMSS_FRAME_WIDTH_ALIGN_8;
+		vdev->ioctl_ops = &stf_vid_vin_ioctl_ops;
+	} else {
+		video->formats = formats_pix_isp;
+		video->nformats = ARRAY_SIZE(formats_pix_isp);
+		video->bpl_alignment = STFCAMSS_FRAME_WIDTH_ALIGN_8;
+		vdev->ioctl_ops = &stf_vid_isp_ioctl_ops;
+	}
+
+	ret = stf_video_init_format(video);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to init format: %d\n", ret);
+		goto err_vid_init_format;
+	}
+
+	vdev->fops = &stf_vid_fops;
+	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE;
+	vdev->vfl_dir = VFL_DIR_RX;
+	vdev->device_caps |= V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
+	vdev->release = stf_video_release;
+	vdev->v4l2_dev = v4l2_dev;
+	vdev->queue = &video->vb2_q;
+	vdev->lock = &video->lock;
+	strscpy(vdev->name, name, sizeof(vdev->name));
+
+	ret = video_register_device(vdev, VFL_TYPE_VIDEO, video->id);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to register video device: %d\n", ret);
+		goto err_vid_reg;
+	}
+
+	video_set_drvdata(vdev, video);
+	return 0;
+
+err_vid_reg:
+err_vid_init_format:
+	media_entity_cleanup(&vdev->entity);
+	mutex_destroy(&video->lock);
+err_vb2_init:
+	mutex_destroy(&video->q_lock);
+	return ret;
+}
+
+void stf_video_unregister(struct stfcamss_video *video)
+{
+	vb2_video_unregister_device(&video->vdev);
+}
drivers/media/platform/starfive/stf_video.h new file +2 -0
new file mode 100644
index 000000000000..63a9a7706a03
@@ -0,0 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*

drivers/media/platform/starfive/stf_video.h line 12

Quoted 10 lines
drivers/media/platform/starfive/stf_video.h new file +10 -0
+ * stf_video.h
+ *
+ * StarFive Camera Subsystem - V4L2 device node
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#ifndef STF_VIDEO_H
+#define STF_VIDEO_H
+

You should include linux/list.h for struct list_head.

Quoted 18 lines
drivers/media/platform/starfive/stf_video.h new file +18 -0
+#include <linux/mutex.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-ioctl.h>
+#include <media/videobuf2-v4l2.h>
+
+#define STFCAMSS_FRAME_MIN_WIDTH		64
+#define STFCAMSS_FRAME_MAX_WIDTH		1920
+#define STFCAMSS_FRAME_MIN_HEIGHT		64
+#define STFCAMSS_FRAME_MAX_HEIGHT		1080
+#define STFCAMSS_FRAME_WIDTH_ALIGN_8		8
+#define STFCAMSS_FRAME_WIDTH_ALIGN_128		128
+#define STFCAMSS_MIN_BUFFERS			2
+
+#define STFCAMSS_MAX_ENTITY_NAME_LEN		27
+
+enum stf_v_line_id {

drivers/media/platform/starfive/stf_video.h line 40

Quoted 10 lines
drivers/media/platform/starfive/stf_video.h new file +10 -0
+	STF_V_LINE_WR = 0,
+	STF_V_LINE_ISP,
+	STF_V_LINE_MAX,
+};
+
+struct stfcamss_buffer {
+	struct vb2_v4l2_buffer vb;
+	dma_addr_t addr[3];
+	struct list_head queue;
+	int sizeimage;

As far as I can tell, this is set but never used.

Quoted 12 lines
drivers/media/platform/starfive/stf_video.h new file +12 -0
+};
+
+struct fract {
+	u8 numerator;
+	u8 denominator;
+};
+
+/*
+ * struct stfcamss_format_info - ISP media bus format information
+ * @code: V4L2 media bus format code
+ * @pixelformat: V4L2 pixel format FCC identifier
+ * @planes: Number of planes

drivers/media/platform/starfive/stf_video.h line 62

Quoted 10 lines
drivers/media/platform/starfive/stf_video.h new file +10 -0
+ * @hsub: Horizontal subsampling (for each plane)
+ * @vsub: Vertical subsampling (for each plane)
+ * @bpp: Bits per pixel when stored in memory (for each plane)
+ */
+struct stfcamss_format_info {
+	u32 code;
+	u32 pixelformat;
+	u8 planes;
+	struct fract hsub[3];
+	struct fract vsub[3];

Only the first element of these arrays is used.

drivers/media/platform/starfive/stf_video.h line 72

Quoted 10 lines
drivers/media/platform/starfive/stf_video.h new file +10 -0
+	u8 bpp[3];
+};
+
+struct stfcamss_video {
+	struct stfcamss *stfcamss;
+	u8 id;
+	struct vb2_queue vb2_q;
+	struct video_device vdev;
+	struct media_pad pad;
+	struct media_pipeline pipe;

This isn't used.

Quoted 23 lines
drivers/media/platform/starfive/stf_video.h new file +23 -0
+	struct v4l2_format active_fmt;
+	enum v4l2_buf_type type;
+	const struct stfcamss_video_ops *ops;
+	struct mutex lock;	 /* serialize device access */
+	struct mutex q_lock;	 /* protects the queue */
+	unsigned int bpl_alignment;
+	const struct stfcamss_format_info *formats;
+	unsigned int nformats;
+};
+
+struct stfcamss_video_ops {
+	int (*queue_buffer)(struct stfcamss_video *vid,
+			    struct stfcamss_buffer *buf);
+	int (*flush_buffers)(struct stfcamss_video *vid,
+			     enum vb2_buffer_state state);
+};
+
+int stf_video_register(struct stfcamss_video *video,
+		       struct v4l2_device *v4l2_dev, const char *name);
+
+void stf_video_unregister(struct stfcamss_video *video);
+
+#endif /* STF_VIDEO_H */
Signature · 3 lines
Regards,

Laurent Pinchart
author
#11 lore raw

Re: [PATCH v6 4/6] media: starfive: Add video driver

Hi Laurent,

Thank you for your comments.

Quoted 96 lines · Laurent Pinchart

Hi Jack,

Thank you for the patch.

Quoted 52 lines · Jack Zhu

Add video driver for StarFive Camera Subsystem.

---
 drivers/media/platform/starfive/Makefile    |   3 +-
 drivers/media/platform/starfive/stf_video.c | 864 ++++++++++++++++++++
 drivers/media/platform/starfive/stf_video.h |  95 +++
 3 files changed, 961 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/platform/starfive/stf_video.c
 create mode 100644 drivers/media/platform/starfive/stf_video.h
drivers/media/platform/starfive/Makefile +2 -1
index 796775fa52f4..d7a42b3a938c 100644
@@ -4,6 +4,7 @@
 #
 
 starfive-camss-objs += \
-		stf_camss.o
+		stf_camss.o \
+		stf_video.o
 
 obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o \
drivers/media/platform/starfive/stf_video.c new file +22 -0
new file mode 100644
index 000000000000..f027c7308dfb
@@ -0,0 +1,864 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * stf_video.c
+ *
+ * StarFive Camera Subsystem - V4L2 device node
+ *
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-mc.h>
+#include <media/videobuf2-dma-contig.h>
+
+#include "stf_camss.h"
+#include "stf_video.h"
+
+static const struct stfcamss_format_info formats_pix_wr[] = {
+	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_PIX_FMT_SRGGB10, 1,
+	  { { 1, 1 } }, { { 1, 1 } }, { 10 } },
+	{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2_PIX_FMT_SGRBG10, 1,

drivers/media/platform/starfive/stf_video.c line 32

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+	  { { 1, 1 } }, { { 1, 1 } }, { 10 } },
+	{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2_PIX_FMT_SGBRG10, 1,
+	  { { 1, 1 } }, { { 1, 1 } }, { 10 } },
+	{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_PIX_FMT_SBGGR10, 1,
+	  { { 1, 1 } }, { { 1, 1 } }, { 10 } },
+};
+
+static const struct stfcamss_format_info formats_pix_isp[] = {
+	{ MEDIA_BUS_FMT_Y12_1X12, V4L2_PIX_FMT_NV12, 1,
+	  { { 1, 1 } }, { { 2, 3 } }, { 8 } },

The [hv]sub values look weird. NV12 subsamples by 2 in both directions for the chroma plane. The result of your bytesperline calculation is right before the chroma plane is subsampled by 2 horizontally, but contains both U and V components, so the number of bytes equals the width in pixels for both the luma and chroma planes. In the vertical direction, the calculation is fine too for NV12 as you're calculating sizeimage for both planes.

I would recommend something along the following lines:

struct stfcamss_format_info {
	u32 code;
	u32 pixelformat;
	u8 planes;
	u8 vsub[3];
	u8 bpp;
};
static const struct stfcamss_format_info formats_pix_isp[] = {
	{
		.code = MEDIA_BUS_FMT_Y12_1X12,
		.pixelformat = V4L2_PIX_FMT_NV12,
		.planes = 2,
		.vsub = { 1, 2 },
		.bpp = 8,
	},
};
Quoted 10 lines
{
	unsigned int bytesperline;
	unsigned int sizeimage = 0;

	bytesperline = width * info->bpp / 8;

	for (i = 0; i < info->planes; ++i)
		sizeimage += bytesperline * height / info->vsub[i];
}

Okay, I will fix it.

Quoted 4 lines
Quoted 4 lines
drivers/media/platform/starfive/stf_video.c new file +4 -0
+};
+
+/* -----------------------------------------------------------------------------
+ * Helper functions
Quoted 9 lines

drivers/media/platform/starfive/stf_video.c line 41

Quoted 5 lines
drivers/media/platform/starfive/stf_video.c new file +5 -0
+ */
+
+static int video_find_format(u32 code, u32 pixelformat,
+			     const struct stfcamss_format_info *formats,
+			     unsigned int nformats)

I would pass the stfcamss_video pointer to this function instead of formats and nformats.

Okay, I will modify the function.

Quoted 264 lines
Quoted 264 lines
drivers/media/platform/starfive/stf_video.c new file +264 -0
+{
+	int i;
+
+	for (i = 0; i < nformats; i++) {
+		if (formats[i].code == code &&
+		    formats[i].pixelformat == pixelformat)
+			return i;
+	}
+
+	for (i = 0; i < nformats; i++)
+		if (formats[i].code == code)
+			return i;
+
+	for (i = 0; i < nformats; i++)
+		if (formats[i].pixelformat == pixelformat)
+			return i;
+
+	return -EINVAL;
+}
+
+static int __video_try_fmt(struct stfcamss_video *video, struct v4l2_format *f)
+{
+	struct v4l2_pix_format *pix;
+	const struct stfcamss_format_info *fi;
+	u32 width, height;
+	u32 bpl;
+	int i;
+
+	pix = &f->fmt.pix;
+
+	for (i = 0; i < video->nformats; i++)
+		if (pix->pixelformat == video->formats[i].pixelformat)
+			break;
+
+	if (i == video->nformats)
+		i = 0; /* default format */
+
+	fi = &video->formats[i];
+	width = pix->width;
+	height = pix->height;
+
+	memset(pix, 0, sizeof(*pix));
+
+	pix->pixelformat = fi->pixelformat;
+	pix->width = clamp_t(u32, width, STFCAMSS_FRAME_MIN_WIDTH,
+			     STFCAMSS_FRAME_MAX_WIDTH);
+	pix->height = clamp_t(u32, height, STFCAMSS_FRAME_MIN_HEIGHT,
+			      STFCAMSS_FRAME_MAX_HEIGHT);
+	bpl = pix->width / fi->hsub[0].numerator *
+	      fi->hsub[0].denominator * fi->bpp[0] / 8;
+	bpl = ALIGN(bpl, video->bpl_alignment);
+	pix->bytesperline = bpl;
+	pix->sizeimage = pix->height / fi->vsub[0].numerator *
+			 fi->vsub[0].denominator * bpl;
+
+	pix->field = V4L2_FIELD_NONE;
+	pix->colorspace = V4L2_COLORSPACE_SRGB;
+	pix->flags = 0;
+	pix->ycbcr_enc =
+		V4L2_MAP_YCBCR_ENC_DEFAULT(pix->colorspace);
+	pix->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+							  pix->colorspace,
+							  pix->ycbcr_enc);
+	pix->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(pix->colorspace);
+
+	return 0;
+}
+
+static int stf_video_init_format(struct stfcamss_video *video)
+{
+	int ret;
+	struct v4l2_format format = {
+		.type = video->type,
+		.fmt.pix = {
+			.width = 1920,
+			.height = 1080,
+			.pixelformat = V4L2_PIX_FMT_RGB565,
+		},
+	};
+
+	ret = __video_try_fmt(video, &format);
+
+	if (ret < 0)
+		return ret;
+
+	video->active_fmt = format;
+
+	return 0;
+}
+
+/* -----------------------------------------------------------------------------
+ * Video queue operations
+ */
+
+static int video_queue_setup(struct vb2_queue *q,
+			     unsigned int *num_buffers,
+			     unsigned int *num_planes,
+			     unsigned int sizes[],
+			     struct device *alloc_devs[])
+{
+	struct stfcamss_video *video = vb2_get_drv_priv(q);
+	const struct v4l2_pix_format *format = &video->active_fmt.fmt.pix;
+
+	if (*num_planes) {
+		if (*num_planes != 1)
+			return -EINVAL;
+
+		if (sizes[0] < format->sizeimage)
+			return -EINVAL;
+	}
+
+	*num_planes = 1;
+	sizes[0] = format->sizeimage;
+	if (!sizes[0])
+		dev_err(video->stfcamss->dev,
+			"%s: error size is zero!!!\n", __func__);
+
+	dev_dbg(video->stfcamss->dev, "planes = %d, size = %d\n",
+		*num_planes, sizes[0]);
+
+	return 0;
+}
+
+static int video_buf_init(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
+	struct stfcamss_buffer *buffer =
+		container_of(vbuf, struct stfcamss_buffer, vb);
+	const struct v4l2_pix_format *fmt = &video->active_fmt.fmt.pix;
+	dma_addr_t *paddr;
+
+	buffer->sizeimage = 0;
+
+	paddr = vb2_plane_cookie(vb, 0);
+	buffer->sizeimage = vb2_plane_size(vb, 0);
+	buffer->addr[0] = *paddr;
+	if (fmt->pixelformat == V4L2_PIX_FMT_NV12 ||
+	    fmt->pixelformat == V4L2_PIX_FMT_NV21 ||
+	    fmt->pixelformat == V4L2_PIX_FMT_NV16 ||
+	    fmt->pixelformat == V4L2_PIX_FMT_NV61)
+		buffer->addr[1] =
+			buffer->addr[0] + fmt->bytesperline * fmt->height;
+
+	return 0;
+}
+
+static int video_buf_prepare(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
+	const struct v4l2_pix_format *fmt = &video->active_fmt.fmt.pix;
+
+	if (fmt->sizeimage > vb2_plane_size(vb, 0)) {
+		dev_err(video->stfcamss->dev,
+			"sizeimage = %d, plane size = %d\n",
+			fmt->sizeimage, (unsigned int)vb2_plane_size(vb, 0));
+		return -EINVAL;
+	}
+	vb2_set_plane_payload(vb, 0, fmt->sizeimage);
+
+	vbuf->field = V4L2_FIELD_NONE;
+
+	return 0;
+}
+
+static void video_buf_queue(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct stfcamss_video *video = vb2_get_drv_priv(vb->vb2_queue);
+	struct stfcamss_buffer *buffer =
+		container_of(vbuf, struct stfcamss_buffer, vb);
+
+	video->ops->queue_buffer(video, buffer);
+}
+
+/*
+ * video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
+ * @mbus: v4l2_mbus_framefmt format (input)
+ * @pix: v4l2_pix_format_mplane format (output)
+ * @f: a pointer to formats array element to be used for the conversion
+ * @alignment: bytesperline alignment value
+ *
+ * Fill the output pix structure with information from the input mbus format.
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int video_mbus_to_pix(const struct v4l2_mbus_framefmt *mbus,
+			     struct v4l2_pix_format *pix,
+			     const struct stfcamss_format_info *f,
+			     unsigned int alignment)
+{
+	u32 bytesperline;
+
+	memset(pix, 0, sizeof(*pix));
+	v4l2_fill_pix_format(pix, mbus);
+	pix->pixelformat = f->pixelformat;
+	bytesperline = pix->width / f->hsub[0].numerator *
+		       f->hsub[0].denominator * f->bpp[0] / 8;
+	bytesperline = ALIGN(bytesperline, alignment);
+	pix->bytesperline = bytesperline;
+	pix->sizeimage = pix->height / f->vsub[0].numerator *
+			 f->vsub[0].denominator * bytesperline;
+	return 0;
+}
+
+static struct v4l2_subdev *video_remote_subdev(struct stfcamss_video *video,
+					       u32 *pad)
+{
+	struct media_pad *remote;
+
+	remote = media_pad_remote_pad_first(&video->pad);
+
+	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
+		return NULL;
+
+	if (pad)
+		*pad = remote->index;
+
+	return media_entity_to_v4l2_subdev(remote->entity);
+}
+
+static int video_get_subdev_format(struct stfcamss_video *video,
+				   struct v4l2_format *format)
+{
+	struct v4l2_pix_format *pix = &video->active_fmt.fmt.pix;
+	struct v4l2_subdev_format fmt;
+	struct v4l2_subdev *subdev;
+	u32 pixelformat;
+	u32 pad;
+	int ret;
+
+	subdev = video_remote_subdev(video, &pad);
+	if (!subdev)
+		return -EPIPE;
+
+	fmt.pad = pad;
+	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret)
+		return ret;
+
+	pixelformat = pix->pixelformat;
+	ret = video_find_format(fmt.format.code, pixelformat,
+				video->formats, video->nformats);
+	if (ret < 0)
+		return ret;
+
+	format->type = video->type;
+
+	return video_mbus_to_pix(&fmt.format, &format->fmt.pix,
+				 &video->formats[ret], video->bpl_alignment);
+}
+
+static int video_check_format(struct stfcamss_video *video)
+{
+	struct v4l2_pix_format *pix = &video->active_fmt.fmt.pix;
+	struct v4l2_format format;
+	struct v4l2_pix_format *sd_pix = &format.fmt.pix;
+	int ret;
+
+	sd_pix->pixelformat = pix->pixelformat;
+	ret = video_get_subdev_format(video, &format);
Quoted 9 lines

drivers/media/platform/starfive/stf_video.c line 311

Quoted 6 lines
drivers/media/platform/starfive/stf_video.c new file +6 -0
+	if (ret < 0)
+		return ret;
+
+	if (pix->pixelformat != sd_pix->pixelformat ||
+	    pix->height > sd_pix->height ||
+	    pix->width > sd_pix->width ||

The width and height must be identical.

Okay, I will fix it.

Quoted 59 lines
Quoted 40 lines
drivers/media/platform/starfive/stf_video.c new file +40 -0
+	    pix->field != format.fmt.pix.field) {
+		dev_err(video->stfcamss->dev,
+			"%s, not match:\n"
+			"0x%x 0x%x\n0x%x 0x%x\n0x%x 0x%x\n",
+			__func__,
+			pix->pixelformat, sd_pix->pixelformat,
+			pix->height, sd_pix->height,
+			pix->field, format.fmt.pix.field);
+		return -EPIPE;
+	}
+
+	return 0;
+}
+
+static int video_start_streaming(struct vb2_queue *q, unsigned int count)
+{
+	struct stfcamss_video *video = vb2_get_drv_priv(q);
+	struct video_device *vdev = &video->vdev;
+	struct media_entity *entity;
+	struct media_pad *pad;
+	struct v4l2_subdev *subdev;
+	int ret;
+
+	ret = video_device_pipeline_start(vdev, &video->stfcamss->pipe);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to media_pipeline_start: %d\n", ret);
+		return ret;
+	}
+
+	ret = video_check_format(video);
+	if (ret < 0)
+		goto error;
+	entity = &vdev->entity;
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_pad_remote_pad_first(pad);

drivers/media/platform/starfive/stf_video.c line 361

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		ret = v4l2_subdev_call(subdev, video, s_stream, 1);
+		if (ret < 0 && ret != -ENOIOCTLCMD)
+			goto error;
+	}

While you're free to decide how to implement the start streaming operation for all the subdevs in your driver, you must not call .s_stream() on all *external* subdevs. Imagine a case where the device connected to your VIN input would be a parallel-to-CSI-2 bridge. This driver must call .s_stream() on the bridge, but must not call .s_stream() on the device at the input of the bridge, as the bridge driver will propagate the .s_stream() call itself.

Quoted 3 lines

I will be able to advice better on how to handle stream start once you provide the media graph of the device.

Okay, I will provide the media graph of the device.

Quoted 55 lines
Quoted 55 lines
drivers/media/platform/starfive/stf_video.c new file +55 -0
+	return 0;
+
+error:
+	video_device_pipeline_stop(vdev);
+	video->ops->flush_buffers(video, VB2_BUF_STATE_QUEUED);
+	return ret;
+}
+
+static void video_stop_streaming(struct vb2_queue *q)
+{
+	struct stfcamss_video *video = vb2_get_drv_priv(q);
+	struct video_device *vdev = &video->vdev;
+	struct media_entity *entity;
+	struct media_pad *pad;
+	struct v4l2_subdev *subdev;
+
+	entity = &vdev->entity;
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_pad_remote_pad_first(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		v4l2_subdev_call(subdev, video, s_stream, 0);
+	}
+
+	video_device_pipeline_stop(vdev);
+	video->ops->flush_buffers(video, VB2_BUF_STATE_ERROR);
+}
+
+static const struct vb2_ops stf_video_vb2_q_ops = {
+	.queue_setup     = video_queue_setup,
+	.wait_prepare    = vb2_ops_wait_prepare,
+	.wait_finish     = vb2_ops_wait_finish,
+	.buf_init        = video_buf_init,
+	.buf_prepare     = video_buf_prepare,
+	.buf_queue       = video_buf_queue,
+	.start_streaming = video_start_streaming,
+	.stop_streaming  = video_stop_streaming,
+};
+
+/* -----------------------------------------------------------------------------
+ * V4L2 ioctls
+ */
+
+static int video_querycap(struct file *file, void *fh,
+			  struct v4l2_capability *cap)
+{
+	struct stfcamss_video *video = video_drvdata(file);
Quoted 9 lines

drivers/media/platform/starfive/stf_video.c line 421

Quoted 5 lines
drivers/media/platform/starfive/stf_video.c new file +5 -0
+
+	strscpy(cap->driver, "stf camss", sizeof(cap->driver));
+	strscpy(cap->card, "Starfive Camera Subsystem", sizeof(cap->card));
+	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
+		 dev_name(video->stfcamss->dev));

No need to set bus_info manually, it's done automatically in v4l_querycap().

Okay, I will drop it.

Quoted 145 lines
Quoted 145 lines
drivers/media/platform/starfive/stf_video.c new file +145 -0
+	return 0;
+}
+
+static int video_get_pfmt_by_index(struct stfcamss_video *video, int ndx)
+{
+	int i, j, k;
+
+	/* find index "i" of "k"th unique pixelformat in formats array */
+	k = -1;
+	for (i = 0; i < video->nformats; i++) {
+		for (j = 0; j < i; j++) {
+			if (video->formats[i].pixelformat ==
+			    video->formats[j].pixelformat)
+				break;
+		}
+
+		if (j == i)
+			k++;
+
+		if (k == ndx)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int video_get_pfmt_by_mcode(struct stfcamss_video *video, u32 mcode)
+{
+	int i;
+
+	for (i = 0; i < video->nformats; i++) {
+		if (video->formats[i].code == mcode)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int video_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	int i;
+
+	if (f->type != video->type)
+		return -EINVAL;
+	if (f->index >= video->nformats)
+		return -EINVAL;
+
+	if (f->mbus_code) {
+		/* Each entry in formats[] table has unique mbus_code */
+		if (f->index > 0)
+			return -EINVAL;
+
+		i = video_get_pfmt_by_mcode(video, f->mbus_code);
+	} else {
+		i = video_get_pfmt_by_index(video, f->index);
+	}
+
+	if (i < 0)
+		return -EINVAL;
+
+	f->pixelformat = video->formats[i].pixelformat;
+
+	return 0;
+}
+
+static int video_enum_framesizes(struct file *file, void *fh,
+				 struct v4l2_frmsizeenum *fsize)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	int i;
+
+	if (fsize->index)
+		return -EINVAL;
+
+	for (i = 0; i < video->nformats; i++) {
+		if (video->formats[i].pixelformat == fsize->pixel_format)
+			break;
+	}
+
+	if (i == video->nformats)
+		return -EINVAL;
+
+	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
+	fsize->stepwise.min_width = STFCAMSS_FRAME_MIN_WIDTH;
+	fsize->stepwise.max_width = STFCAMSS_FRAME_MAX_WIDTH;
+	fsize->stepwise.min_height = STFCAMSS_FRAME_MIN_HEIGHT;
+	fsize->stepwise.max_height = STFCAMSS_FRAME_MAX_HEIGHT;
+	fsize->stepwise.step_width = 1;
+	fsize->stepwise.step_height = 1;
+
+	return 0;
+}
+
+static int video_g_fmt(struct file *file, void *fh, struct v4l2_format *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+
+	*f = video->active_fmt;
+
+	return 0;
+}
+
+static int video_s_fmt(struct file *file, void *fh, struct v4l2_format *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	int ret;
+
+	if (vb2_is_busy(&video->vb2_q))
+		return -EBUSY;
+
+	ret = __video_try_fmt(video, f);
+	if (ret < 0)
+		return ret;
+
+	video->active_fmt = *f;
+
+	return 0;
+}
+
+static int video_try_fmt(struct file *file, void *fh, struct v4l2_format *f)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+
+	return __video_try_fmt(video, f);
+}
+
+static int video_enum_input(struct file *file, void *fh,
+			    struct v4l2_input *input)
+{
+	if (input->index > 0)
+		return -EINVAL;
+
+	strscpy(input->name, "camera", sizeof(input->name));
+	input->type = V4L2_INPUT_TYPE_CAMERA;
+
+	return 0;
+}
+
+static int video_g_input(struct file *file, void *fh, unsigned int *input)
+{
+	*input = 0;
+
+	return 0;
+}
Quoted 9 lines

drivers/media/platform/starfive/stf_video.c line 571

Quoted 5 lines
drivers/media/platform/starfive/stf_video.c new file +5 -0
+
+static int video_s_input(struct file *file, void *fh, unsigned int input)
+{
+	return input == 0 ? 0 : -EINVAL;
+}

You can drop enum_input, g_input and s_input, they make little sense for this device.

Okay, I will drop them.

Quoted 22 lines
Quoted 22 lines
drivers/media/platform/starfive/stf_video.c new file +22 -0
+
+static int video_g_parm(struct file *file, void *priv,
+			struct v4l2_streamparm *p)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	struct video_device *vdev = &video->vdev;
+	struct media_entity *entity;
+	struct v4l2_subdev *subdev;
+	struct media_pad *pad;
+	int ret, is_support = 0;
+
+	entity = &vdev->entity;
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_pad_remote_pad_first(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
Quoted 9 lines

drivers/media/platform/starfive/stf_video.c line 596

Quoted 3 lines
drivers/media/platform/starfive/stf_video.c new file +3 -0
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		ret = v4l2_g_parm_cap(vdev, subdev, p);

Unless I'm missing something, the g_parm operation isn't implemented in the connected subdev (VIN or ISP), and neither is s_parm. You can just drop video_g_parm() and video_s_parm(), you don't have to implement them.

Okay, I will drop them.

Quoted 94 lines
Quoted 94 lines
drivers/media/platform/starfive/stf_video.c new file +94 -0
+		if (ret < 0 && ret != -ENOIOCTLCMD)
+			break;
+		if (!ret)
+			is_support = 1;
+	}
+
+	return is_support ? 0 : ret;
+}
+
+static int video_s_parm(struct file *file, void *priv,
+			struct v4l2_streamparm *p)
+{
+	struct stfcamss_video *video = video_drvdata(file);
+	struct video_device *vdev = &video->vdev;
+	struct media_entity *entity;
+	struct v4l2_subdev *subdev;
+	struct media_pad *pad;
+	struct v4l2_streamparm tmp_p;
+	int ret, is_support = 0;
+
+	entity = &vdev->entity;
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_pad_remote_pad_first(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		tmp_p = *p;
+		ret = v4l2_s_parm_cap(vdev, subdev, &tmp_p);
+		if (ret < 0 && ret != -ENOIOCTLCMD)
+			break;
+		if (!ret) {
+			is_support = 1;
+			*p = tmp_p;
+		}
+	}
+
+	return is_support ? 0 : ret;
+}
+
+static const struct v4l2_ioctl_ops stf_vid_vin_ioctl_ops = {
+	.vidioc_querycap                = video_querycap,
+	.vidioc_enum_fmt_vid_cap        = video_enum_fmt,
+	.vidioc_enum_fmt_vid_out        = video_enum_fmt,
+	.vidioc_enum_framesizes         = video_enum_framesizes,
+	.vidioc_g_fmt_vid_cap           = video_g_fmt,
+	.vidioc_s_fmt_vid_cap           = video_s_fmt,
+	.vidioc_try_fmt_vid_cap         = video_try_fmt,
+	.vidioc_g_fmt_vid_out           = video_g_fmt,
+	.vidioc_s_fmt_vid_out           = video_s_fmt,
+	.vidioc_try_fmt_vid_out         = video_try_fmt,
+	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
+	.vidioc_querybuf                = vb2_ioctl_querybuf,
+	.vidioc_qbuf                    = vb2_ioctl_qbuf,
+	.vidioc_expbuf                  = vb2_ioctl_expbuf,
+	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
+	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
+	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
+	.vidioc_streamon                = vb2_ioctl_streamon,
+	.vidioc_streamoff               = vb2_ioctl_streamoff,
+	.vidioc_enum_input              = video_enum_input,
+	.vidioc_g_input                 = video_g_input,
+	.vidioc_s_input                 = video_s_input,
+	.vidioc_g_parm                  = video_g_parm,
+	.vidioc_s_parm                  = video_s_parm,
+};
+
+static const struct v4l2_ioctl_ops stf_vid_isp_ioctl_ops = {
+	.vidioc_querycap                = video_querycap,
+	.vidioc_enum_fmt_vid_cap        = video_enum_fmt,
+	.vidioc_enum_fmt_vid_out        = video_enum_fmt,
+	.vidioc_enum_framesizes         = video_enum_framesizes,
+	.vidioc_g_fmt_vid_cap           = video_g_fmt,
+	.vidioc_s_fmt_vid_cap           = video_s_fmt,
+	.vidioc_try_fmt_vid_cap         = video_try_fmt,
+	.vidioc_g_fmt_vid_out           = video_g_fmt,
+	.vidioc_s_fmt_vid_out           = video_s_fmt,
+	.vidioc_try_fmt_vid_out         = video_try_fmt,
+	.vidioc_reqbufs                 = vb2_ioctl_reqbufs,
+	.vidioc_querybuf                = vb2_ioctl_querybuf,
+	.vidioc_qbuf                    = vb2_ioctl_qbuf,
+	.vidioc_expbuf                  = vb2_ioctl_expbuf,
+	.vidioc_dqbuf                   = vb2_ioctl_dqbuf,
+	.vidioc_create_bufs             = vb2_ioctl_create_bufs,
+	.vidioc_prepare_buf             = vb2_ioctl_prepare_buf,
+	.vidioc_streamon                = vb2_ioctl_streamon,
+	.vidioc_streamoff               = vb2_ioctl_streamoff,
+	.vidioc_enum_input              = video_enum_input,
Quoted 9 lines

drivers/media/platform/starfive/stf_video.c line 668

Quoted 5 lines
drivers/media/platform/starfive/stf_video.c new file +5 -0
+	.vidioc_g_input                 = video_g_input,
+	.vidioc_s_input                 = video_s_input,
+	.vidioc_g_parm                  = video_g_parm,
+	.vidioc_s_parm                  = video_s_parm,
+};

Unless I'm mistaken the two structures are identical, you can have a single one.

Okay, I will use a single structure.

Quoted 31 lines
Quoted 16 lines
drivers/media/platform/starfive/stf_video.c new file +16 -0
+
+/* -----------------------------------------------------------------------------
+ * V4L2 file operations
+ */
+
+static int video_open(struct file *file)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct stfcamss_video *video = video_drvdata(file);
+	struct v4l2_fh *vfh;
+	int ret;
+
+	mutex_lock(&video->lock);
+
+	vfh = kzalloc(sizeof(*vfh), GFP_KERNEL);
+	if (!vfh) {

drivers/media/platform/starfive/stf_video.c line 721

Quoted 10 lines
drivers/media/platform/starfive/stf_video.c new file +10 -0
+		ret = -ENOMEM;
+		goto error_alloc;
+	}
+
+	v4l2_fh_init(vfh, vdev);
+	v4l2_fh_add(vfh);
+
+	file->private_data = vfh;
+
+	ret = v4l2_pipeline_pm_get(&vdev->entity);

This is a deprecated API, you can drop the call. It will greatly simplify video_open(), which can be replaced with v4l2_fh_open(). Same for release, you can use v4l2_fh_release().

Quoted 9 lines

Regarding power management, you should use runtime PM, calling pm_runtime_resume_and_get() when starting streaming (in video_start_streaming()) and pm_runtime_put() when stopping streaming. As a result of dropping v4l2_pipeline_pm_get() the .s_power() subdev operations won't be called automatically. They are deprecated, so they should be dropped, and the relevant code moved to the PM runtime resume/suspend handlers. You will be able to drop the power_count fields.

Okay, I will fix it.

Quoted 155 lines
Quoted 155 lines
drivers/media/platform/starfive/stf_video.c new file +143 -0
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to power up pipeline: %d\n", ret);
+		goto error_pm_use;
+	}
+	mutex_unlock(&video->lock);
+
+	return 0;
+
+error_pm_use:
+	v4l2_fh_release(file);
+error_alloc:
+	mutex_unlock(&video->lock);
+	return ret;
+}
+
+static int video_release(struct file *file)
+{
+	struct video_device *vdev = video_devdata(file);
+
+	vb2_fop_release(file);
+	v4l2_pipeline_pm_put(&vdev->entity);
+	file->private_data = NULL;
+
+	return 0;
+}
+
+static const struct v4l2_file_operations stf_vid_fops = {
+	.owner          = THIS_MODULE,
+	.unlocked_ioctl = video_ioctl2,
+	.open           = video_open,
+	.release        = video_release,
+	.poll           = vb2_fop_poll,
+	.mmap           = vb2_fop_mmap,
+	.read           = vb2_fop_read,
+};
+
+/* -----------------------------------------------------------------------------
+ * STFCAMSS video core
+ */
+
+static void stf_video_release(struct video_device *vdev)
+{
+	struct stfcamss_video *video = video_get_drvdata(vdev);
+
+	media_entity_cleanup(&vdev->entity);
+
+	mutex_destroy(&video->q_lock);
+	mutex_destroy(&video->lock);
+}
+
+int stf_video_register(struct stfcamss_video *video,
+		       struct v4l2_device *v4l2_dev, const char *name)
+{
+	struct video_device *vdev;
+	struct vb2_queue *q;
+	struct media_pad *pad = &video->pad;
+	int ret;
+
+	vdev = &video->vdev;
+
+	mutex_init(&video->q_lock);
+
+	q = &video->vb2_q;
+	q->drv_priv = video;
+	q->mem_ops = &vb2_dma_contig_memops;
+	q->ops = &stf_video_vb2_q_ops;
+	q->type = video->type;
+	q->io_modes = VB2_DMABUF | VB2_MMAP | VB2_READ;
+	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	q->buf_struct_size = sizeof(struct stfcamss_buffer);
+	q->dev = video->stfcamss->dev;
+	q->lock = &video->q_lock;
+	q->min_buffers_needed = STFCAMSS_MIN_BUFFERS;
+	ret = vb2_queue_init(q);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to init vb2 queue: %d\n", ret);
+		goto err_vb2_init;
+	}
+
+	pad->flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_pads_init(&vdev->entity, 1, pad);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to init video entity: %d\n", ret);
+		goto err_vb2_init;
+	}
+
+	mutex_init(&video->lock);
+
+	if (video->id == STF_V_LINE_WR) {
+		video->formats = formats_pix_wr;
+		video->nformats = ARRAY_SIZE(formats_pix_wr);
+		video->bpl_alignment = STFCAMSS_FRAME_WIDTH_ALIGN_8;
+		vdev->ioctl_ops = &stf_vid_vin_ioctl_ops;
+	} else {
+		video->formats = formats_pix_isp;
+		video->nformats = ARRAY_SIZE(formats_pix_isp);
+		video->bpl_alignment = STFCAMSS_FRAME_WIDTH_ALIGN_8;
+		vdev->ioctl_ops = &stf_vid_isp_ioctl_ops;
+	}
+
+	ret = stf_video_init_format(video);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to init format: %d\n", ret);
+		goto err_vid_init_format;
+	}
+
+	vdev->fops = &stf_vid_fops;
+	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE;
+	vdev->vfl_dir = VFL_DIR_RX;
+	vdev->device_caps |= V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
+	vdev->release = stf_video_release;
+	vdev->v4l2_dev = v4l2_dev;
+	vdev->queue = &video->vb2_q;
+	vdev->lock = &video->lock;
+	strscpy(vdev->name, name, sizeof(vdev->name));
+
+	ret = video_register_device(vdev, VFL_TYPE_VIDEO, video->id);
+	if (ret < 0) {
+		dev_err(video->stfcamss->dev,
+			"Failed to register video device: %d\n", ret);
+		goto err_vid_reg;
+	}
+
+	video_set_drvdata(vdev, video);
+	return 0;
+
+err_vid_reg:
+err_vid_init_format:
+	media_entity_cleanup(&vdev->entity);
+	mutex_destroy(&video->lock);
+err_vb2_init:
+	mutex_destroy(&video->q_lock);
+	return ret;
+}
+
+void stf_video_unregister(struct stfcamss_video *video)
+{
+	vb2_video_unregister_device(&video->vdev);
+}
drivers/media/platform/starfive/stf_video.h new file +6 -0
new file mode 100644
index 000000000000..63a9a7706a03
@@ -0,0 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * stf_video.h
+ *
+ * StarFive Camera Subsystem - V4L2 device node
+ *
Quoted 9 lines

drivers/media/platform/starfive/stf_video.h line 12

Quoted 6 lines
drivers/media/platform/starfive/stf_video.h new file +6 -0
+ * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
+ */
+
+#ifndef STF_VIDEO_H
+#define STF_VIDEO_H
+

You should include linux/list.h for struct list_head.

Okay, I will add it.

Quoted 22 lines
Quoted 22 lines
drivers/media/platform/starfive/stf_video.h new file +22 -0
+#include <linux/mutex.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-ioctl.h>
+#include <media/videobuf2-v4l2.h>
+
+#define STFCAMSS_FRAME_MIN_WIDTH		64
+#define STFCAMSS_FRAME_MAX_WIDTH		1920
+#define STFCAMSS_FRAME_MIN_HEIGHT		64
+#define STFCAMSS_FRAME_MAX_HEIGHT		1080
+#define STFCAMSS_FRAME_WIDTH_ALIGN_8		8
+#define STFCAMSS_FRAME_WIDTH_ALIGN_128		128
+#define STFCAMSS_MIN_BUFFERS			2
+
+#define STFCAMSS_MAX_ENTITY_NAME_LEN		27
+
+enum stf_v_line_id {
+	STF_V_LINE_WR = 0,
+	STF_V_LINE_ISP,
+	STF_V_LINE_MAX,
+};
Quoted 9 lines

drivers/media/platform/starfive/stf_video.h line 40

Quoted 6 lines
drivers/media/platform/starfive/stf_video.h new file +6 -0
+
+struct stfcamss_buffer {
+	struct vb2_v4l2_buffer vb;
+	dma_addr_t addr[3];
+	struct list_head queue;
+	int sizeimage;

As far as I can tell, this is set but never used.

Yes, I will drop it.

Quoted 16 lines
Quoted 16 lines
drivers/media/platform/starfive/stf_video.h new file +16 -0
+};
+
+struct fract {
+	u8 numerator;
+	u8 denominator;
+};
+
+/*
+ * struct stfcamss_format_info - ISP media bus format information
+ * @code: V4L2 media bus format code
+ * @pixelformat: V4L2 pixel format FCC identifier
+ * @planes: Number of planes
+ * @hsub: Horizontal subsampling (for each plane)
+ * @vsub: Vertical subsampling (for each plane)
+ * @bpp: Bits per pixel when stored in memory (for each plane)
+ */
Quoted 9 lines

drivers/media/platform/starfive/stf_video.h line 62

Quoted 6 lines
drivers/media/platform/starfive/stf_video.h new file +6 -0
+struct stfcamss_format_info {
+	u32 code;
+	u32 pixelformat;
+	u8 planes;
+	struct fract hsub[3];
+	struct fract vsub[3];

Only the first element of these arrays is used.

Yes, I will fix it.

Quoted 4 lines
Quoted 4 lines
drivers/media/platform/starfive/stf_video.h new file +4 -0
+	u8 bpp[3];
+};
+
+struct stfcamss_video {
Quoted 9 lines

drivers/media/platform/starfive/stf_video.h line 72

Quoted 6 lines
drivers/media/platform/starfive/stf_video.h new file +6 -0
+	struct stfcamss *stfcamss;
+	u8 id;
+	struct vb2_queue vb2_q;
+	struct video_device vdev;
+	struct media_pad pad;
+	struct media_pipeline pipe;

This isn't used.

Okay, I will drop it.

Quoted 24 lines
Quoted 23 lines
drivers/media/platform/starfive/stf_video.h new file +23 -0
+	struct v4l2_format active_fmt;
+	enum v4l2_buf_type type;
+	const struct stfcamss_video_ops *ops;
+	struct mutex lock;	 /* serialize device access */
+	struct mutex q_lock;	 /* protects the queue */
+	unsigned int bpl_alignment;
+	const struct stfcamss_format_info *formats;
+	unsigned int nformats;
+};
+
+struct stfcamss_video_ops {
+	int (*queue_buffer)(struct stfcamss_video *vid,
+			    struct stfcamss_buffer *buf);
+	int (*flush_buffers)(struct stfcamss_video *vid,
+			     enum vb2_buffer_state state);
+};
+
+int stf_video_register(struct stfcamss_video *video,
+		       struct v4l2_device *v4l2_dev, const char *name);
+
+void stf_video_unregister(struct stfcamss_video *video);
+
+#endif /* STF_VIDEO_H */
author
#12 lore raw

PATCH v6 5/6 media: starfive: Add ISP driver

In reply to Jack Zhu

Add ISP driver for StarFive Camera Subsystem.

---
 drivers/media/platform/starfive/Makefile      |   2 +
 drivers/media/platform/starfive/stf_camss.c   |  71 ++-
 drivers/media/platform/starfive/stf_camss.h   |   2 +
 drivers/media/platform/starfive/stf_isp.c     | 550 ++++++++++++++++++
 drivers/media/platform/starfive/stf_isp.h     | 483 +++++++++++++++
 .../media/platform/starfive/stf_isp_hw_ops.c  | 468 +++++++++++++++
 6 files changed, 1574 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/platform/starfive/stf_isp.c
 create mode 100644 drivers/media/platform/starfive/stf_isp.h
 create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
drivers/media/platform/starfive/Makefile +2 -0
index d7a42b3a938c..60a3018f4994 100644
@@ -5,6 +5,8 @@
 
 starfive-camss-objs += \
 		stf_camss.o \
+		stf_isp.o \
+		stf_isp_hw_ops.o \
 		stf_video.o
 
 obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o \
drivers/media/platform/starfive/stf_camss.c +69 -2

125 lines not on this page · raw message

drivers/media/platform/starfive/stf_camss.h +2 -0
index 8f39cd65950c..c9d94be92ea8 100644
@@ -16,6 +16,7 @@
 #include <media/v4l2-device.h>
 
 #include "stf_common.h"
+#include "stf_isp.h"
 
 #define DRV_NAME     "starfive-camss"
 #define STF_DVP_NAME "stf_dvp"
@@ -71,6 +72,7 @@ struct stfcamss {
 	struct media_device media_dev;
 	struct media_pipeline pipe;
 	struct device *dev;
+	struct stf_isp_dev isp_dev;
 	struct v4l2_async_notifier notifier;
 	struct host_data host_data;
 	void __iomem *syscon_base;
drivers/media/platform/starfive/stf_isp.c new file +550 -0

550 lines not on this page · raw message

drivers/media/platform/starfive/stf_isp.h new file +483 -0

483 lines not on this page · raw message

drivers/media/platform/starfive/stf_isp_hw_ops.c new file +468 -0

468 lines not on this page · raw message

Signature · 1 line
2.34.1
author
#13 lore raw

PATCH v6 6/6 media: starfive: Add VIN driver

In reply to Jack Zhu

Add Video In Controller driver for StarFive Camera Subsystem.

---
 drivers/media/platform/starfive/Makefile      |    4 +-
 drivers/media/platform/starfive/stf_camss.c   |   39 +-
 drivers/media/platform/starfive/stf_camss.h   |    2 +
 drivers/media/platform/starfive/stf_vin.c     | 1134 +++++++++++++++++
 drivers/media/platform/starfive/stf_vin.h     |  180 +++
 .../media/platform/starfive/stf_vin_hw_ops.c  |  241 ++++
 6 files changed, 1598 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/platform/starfive/stf_vin.c
 create mode 100644 drivers/media/platform/starfive/stf_vin.h
 create mode 100644 drivers/media/platform/starfive/stf_vin_hw_ops.c
drivers/media/platform/starfive/Makefile +3 -1
index 60a3018f4994..bf497dd24bbb 100644
@@ -7,6 +7,8 @@ starfive-camss-objs += \
 		stf_camss.o \
 		stf_isp.o \
 		stf_isp_hw_ops.o \
-		stf_video.o
+		stf_video.o \
+		stf_vin.o \
+		stf_vin_hw_ops.o
 
 obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o \
drivers/media/platform/starfive/stf_camss.c +38 -1
index 0d8406ba6a40..266de75f55b2 100644
@@ -142,27 +142,61 @@ static int stfcamss_init_subdevices(struct stfcamss *stfcamss)
 		return ret;
 	}
 
+	ret = stf_vin_subdev_init(stfcamss);
+	if (ret < 0) {
+		dev_err(stfcamss->dev, "Failed to init vin subdev: %d\n", ret);
+		return ret;
+	}
 	return ret;
 }
 
 static int stfcamss_register_subdevices(struct stfcamss *stfcamss)
 {
 	int ret;
+	struct stf_vin_dev *vin_dev = &stfcamss->vin_dev;
 	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
 
 	ret = stf_isp_register(isp_dev, &stfcamss->v4l2_dev);
 	if (ret < 0) {
 		dev_err(stfcamss->dev,
 			"Failed to register stf isp%d entity: %d\n", 0, ret);
-		return ret;
+		goto err_reg_isp;
+	}
+
+	ret = stf_vin_register(vin_dev, &stfcamss->v4l2_dev);
+	if (ret < 0) {
+		dev_err(stfcamss->dev,
+			"Failed to register vin entity: %d\n", ret);
+		goto err_reg_vin;
 	}
 
+	ret = media_create_pad_link(&isp_dev->subdev.entity,
+				    STF_ISP_PAD_SRC,
+				    &vin_dev->line[VIN_LINE_ISP].subdev.entity,
+				    STF_VIN_PAD_SINK,
+				    0);
+	if (ret < 0) {
+		dev_err(stfcamss->dev,
+			"Failed to link %s->%s entities: %d\n",
+			isp_dev->subdev.entity.name,
+			vin_dev->line[VIN_LINE_ISP].subdev.entity.name, ret);
+		goto err_link;
+	}
+
+	return ret;
+
+err_link:
+	stf_vin_unregister(&stfcamss->vin_dev);
+err_reg_vin:
+	stf_isp_unregister(&stfcamss->isp_dev);
+err_reg_isp:
 	return ret;
 }
 
 static void stfcamss_unregister_subdevices(struct stfcamss *stfcamss)
 {
 	stf_isp_unregister(&stfcamss->isp_dev);
+	stf_vin_unregister(&stfcamss->vin_dev);
 }
 
 static int stfcamss_subdev_notifier_bound(struct v4l2_async_notifier *async,
@@ -175,11 +209,14 @@ static int stfcamss_subdev_notifier_bound(struct v4l2_async_notifier *async,
 		container_of(asd, struct stfcamss_async_subdev, asd);
 	enum port_num port = csd->port;
 	struct stf_isp_dev *isp_dev = &stfcamss->isp_dev;
+	struct stf_vin_dev *vin_dev = &stfcamss->vin_dev;
 	struct host_data *host_data = &stfcamss->host_data;
 	struct media_entity *source;
 	int i, j;
 
 	if (port == PORT_NUMBER_CSI2RX) {
+		host_data->host_entity[0] =
+			&vin_dev->line[VIN_LINE_WR].subdev.entity;
 		host_data->host_entity[1] = &isp_dev->subdev.entity;
 	} else if (port == PORT_NUMBER_DVP_SENSOR) {
 		dev_err(stfcamss->dev, "Not support DVP sensor\n");
drivers/media/platform/starfive/stf_camss.h +2 -0
index c9d94be92ea8..688bb3b7b6d0 100644
@@ -17,6 +17,7 @@
 
 #include "stf_common.h"
 #include "stf_isp.h"
+#include "stf_vin.h"
 
 #define DRV_NAME     "starfive-camss"
 #define STF_DVP_NAME "stf_dvp"
@@ -72,6 +73,7 @@ struct stfcamss {
 	struct media_device media_dev;
 	struct media_pipeline pipe;
 	struct device *dev;
+	struct stf_vin_dev vin_dev;
 	struct stf_isp_dev isp_dev;
 	struct v4l2_async_notifier notifier;
 	struct host_data host_data;
drivers/media/platform/starfive/stf_vin.c new file +1134 -0

1,134 lines not on this page · raw message

drivers/media/platform/starfive/stf_vin.h new file +180 -0

180 lines not on this page · raw message

drivers/media/platform/starfive/stf_vin_hw_ops.c new file +241 -0

241 lines not on this page · raw message

Signature · 1 line
2.34.1
#14 lore raw
In reply to Jack Zhu

Hi Jack,

Thank you for the patches, I'm happy to see the ISP driver progressing.

Quoted 4 lines · Jack Zhu

Hi,

This series is the v6 series that attempts to support the Camera Subsystem found on StarFive JH7110 SoC.

Could you please share the media graph for the device ? I'd like the output of both media-ctl --print-dot -d /dev/media0 and media-ctl -p -d /dev/media0 ?

Also, have you run v4l2-compliance to test the driver ? Could you please share the v4l2-compliance output ?

Quoted 158 lines

Changes in v6:
- Added 'bus-type' in bindings example.
- Corrected spelling errors.
- As reviewed by Bryan, used 'nclks' and 'nrsts' variables.
- Added lccf config for ISP.

v5 link: https://lore.kernel.org/all/[email protected]/

Changes in v5:
- Rebased on v6.4-rc1.
- Added new patch.
- Modified ISP driver.

v4 link: https://lore.kernel.org/all/[email protected]/

Previous cover letter from v4:

This patch series adds support for the StarFive Camera Subsystem found on StarFive JH7110 SoC.

The driver implements V4L2, Media controller and V4L2 subdev interfaces. Camera sensor using V4L2 subdev interface in the kernel is supported.

The driver is tested on VisionFive V2 board with IMX219 camera sensor. GStreamer 1.18.5 with v4l2src plugin is supported.

Changes since v3:
Patch 1:
- Modified port@0 and port@1 properties.
- Extended the port@0 example with appropriate properties.
- Added 'port@0' for 'required'
Patch 2:
- Modified spelling errors.
Patch 3:
- Merged patch 5 into the patch with an explanation for compatible in commit msg.
Patch 6:
- Asserted pixel_rst[i] reset in the loop after the err_disable_pixclk label.
- Modified Code Style for getting sys_rst and p_rst.
- Renamed clk_name to name and modified the relevant code. Patch 9:
- Added static for stfcamss_get_mem_res function.
- Added static for isp_close function.
- Fixed implicit conversion warning for stf_vin_map_isp_pad function.
- Dropped unused variables.

v3: https://lore.kernel.org/all/[email protected]/

Changes since v2:
- Rebased on v6.3-rc1.
Patch 1:
- Modified spelling errors.
- Added port@0.
- Modified '$ref' of port.
- Added 'ports' to 'required'.
- Dropped 'stfcamss' label in example.
- Added port@0 in example.
- Added MAINTAINERS file.
Patch 2:
- Split this patch into three new patches.
- Modified compatible property.
- Replaced clock names with the existing names.
- Modified 'bus-type' and 'clock-lanes'
- Added port@2 - port@4
- Dropped 'csi2rx' label in example.
Patch 3:
- Updated rst and dot file as three pipelines were deleted. Patch 4:
- Split this patch into three new patches.
- Dropped .s_power() and .get_fmt().
- Dropped CSI-2 DT support.
- Dropped v4l2_device_register_subdev_nodes().
- Used assigned-clock-rates in DT to set clk value.
- Modified 'compatible' field.
Patch 5:
- Deleted three pipelines.
- Modified 'stfcamss_clocks'/'stfcamss_resets' struct.
- Dropped stfcamss_find_sensor() function.
- Removed redundant code from stfcamss_of_parse_endpoint_node().
- Modified spelling errors.
- Rewrote stfcamss_reg_media_subdev_node() function.
- Modified stfcamss_subdev_notifier_bound().
- Modified stfcamss_probe() function.
- Dropped stfcamss_suspend() and stfcamss_resume().
- Dropped dev_info() in stfcamss_remove() function.
- Added 'stf_' prefix for enum subdev_type.
- Moved all includes to the top in stf_camss.h file.
- Dropped unused fields in stfcamss struct.
- Replaced Custom logging macros with regular macros.
- Rewrote register read and write functions.
- Used lowercase for all hex constants.
- Used macro to name registers.
- Dropped unused ioctl and stf_isp_ioctl.h file.

v2: https://lore.kernel.org/all/[email protected]/

Changes since v1:
- Deleted starfive,jh7110-mipi-csi2.yaml.
- Converted cdns,csi2rx.txt to cdns,csi2rx.yaml and added ‘resets’ properties.
- Added ‘cdns,csi2rx.yaml’ in ‘CADENCE MIPI-CSI2 BRIDGES’ entry.
- The following contents were modified in starfive,jh7110-camss.yaml:
dropped quotes from ’id’ and ‘schema’; dropped ‘|’ for ‘description’; corrected the wrong or redundant words: ‘a ISP’, ‘PD ISP’; dropped ‘minItems’ for ‘reg’, ‘clocks’, ‘resets’ and ‘interrupts’; dropped the '_clk' and 'rst_' prefix about the 'clock-names' and 'reset-names';
changed ‘endpoint@1’ to ‘endpoint’; updated examples;
- Updated Subject for some patches.
- Merged patch 6, 7, 8, 9, 10, 11 into one patch.

Jack Zhu (6):
media: dt-bindings: Add JH7110 Camera Subsystem
media: admin-guide: Add starfive_camss.rst for Starfive Camera
Subsystem
media: starfive: Add basic driver
media: starfive: Add video driver
media: starfive: Add ISP driver
media: starfive: Add VIN driver

 .../admin-guide/media/starfive_camss.rst      |   57 +
 .../media/starfive_camss_graph.dot            |   16 +
 .../admin-guide/media/v4l-drivers.rst         |    1 +
 .../bindings/media/starfive,jh7110-camss.yaml |  180 +++
 MAINTAINERS                                   |    9 +
 drivers/media/platform/Kconfig                |    1 +
 drivers/media/platform/Makefile               |    1 +
 drivers/media/platform/starfive/Kconfig       |   19 +
 drivers/media/platform/starfive/Makefile      |   14 +
 drivers/media/platform/starfive/stf_camss.c   |  476 +++++++
 drivers/media/platform/starfive/stf_camss.h   |  157 +++
 drivers/media/platform/starfive/stf_common.h  |   18 +
 drivers/media/platform/starfive/stf_isp.c     |  550 ++++++++
 drivers/media/platform/starfive/stf_isp.h     |  483 +++++++
 .../media/platform/starfive/stf_isp_hw_ops.c  |  468 +++++++
 drivers/media/platform/starfive/stf_video.c   |  864 +++++++++++++
 drivers/media/platform/starfive/stf_video.h   |   95 ++
 drivers/media/platform/starfive/stf_vin.c     | 1134 +++++++++++++++++
 drivers/media/platform/starfive/stf_vin.h     |  180 +++
 .../media/platform/starfive/stf_vin_hw_ops.c  |  241 ++++
 20 files changed, 4964 insertions(+)
 create mode 100644 Documentation/admin-guide/media/starfive_camss.rst
 create mode 100644 Documentation/admin-guide/media/starfive_camss_graph.dot
 create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
 create mode 100644 drivers/media/platform/starfive/Kconfig
 create mode 100644 drivers/media/platform/starfive/Makefile
 create mode 100644 drivers/media/platform/starfive/stf_camss.c
 create mode 100644 drivers/media/platform/starfive/stf_camss.h
 create mode 100644 drivers/media/platform/starfive/stf_common.h
 create mode 100644 drivers/media/platform/starfive/stf_isp.c
 create mode 100644 drivers/media/platform/starfive/stf_isp.h
 create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
 create mode 100644 drivers/media/platform/starfive/stf_video.c
 create mode 100644 drivers/media/platform/starfive/stf_video.h
 create mode 100644 drivers/media/platform/starfive/stf_vin.c
 create mode 100644 drivers/media/platform/starfive/stf_vin.h
 create mode 100644 drivers/media/platform/starfive/stf_vin_hw_ops.c
Signature · 3 lines
Regards,

Laurent Pinchart
author
#15 lore raw

Hi Laurent,

Thank you for your comments.

Quoted 10 lines · Laurent Pinchart

Hi Jack,

Thank you for the patches, I'm happy to see the ISP driver progressing.

Quoted 4 lines · Jack Zhu

Hi,

This series is the v6 series that attempts to support the Camera Subsystem found on StarFive JH7110 SoC.

Quoted 7 lines

Could you please share the media graph for the device ? I'd like the output of both media-ctl --print-dot -d /dev/media0 and media-ctl -p -d /dev/media0 ?

Also, have you run v4l2-compliance to test the driver ? Could you please share the v4l2-compliance output ?

Yes, I will provide the media graph for the device and v4l2-compliance output.

Quoted 159 lines
Quoted 158 lines

Changes in v6:
- Added 'bus-type' in bindings example.
- Corrected spelling errors.
- As reviewed by Bryan, used 'nclks' and 'nrsts' variables.
- Added lccf config for ISP.

v5 link: https://lore.kernel.org/all/[email protected]/

Changes in v5:
- Rebased on v6.4-rc1.
- Added new patch.
- Modified ISP driver.

v4 link: https://lore.kernel.org/all/[email protected]/

Previous cover letter from v4:

This patch series adds support for the StarFive Camera Subsystem found on StarFive JH7110 SoC.

The driver implements V4L2, Media controller and V4L2 subdev interfaces. Camera sensor using V4L2 subdev interface in the kernel is supported.

The driver is tested on VisionFive V2 board with IMX219 camera sensor. GStreamer 1.18.5 with v4l2src plugin is supported.

Changes since v3:
Patch 1:
- Modified port@0 and port@1 properties.
- Extended the port@0 example with appropriate properties.
- Added 'port@0' for 'required'
Patch 2:
- Modified spelling errors.
Patch 3:
- Merged patch 5 into the patch with an explanation for compatible in commit msg.
Patch 6:
- Asserted pixel_rst[i] reset in the loop after the err_disable_pixclk label.
- Modified Code Style for getting sys_rst and p_rst.
- Renamed clk_name to name and modified the relevant code. Patch 9:
- Added static for stfcamss_get_mem_res function.
- Added static for isp_close function.
- Fixed implicit conversion warning for stf_vin_map_isp_pad function.
- Dropped unused variables.

v3: https://lore.kernel.org/all/[email protected]/

Changes since v2:
- Rebased on v6.3-rc1.
Patch 1:
- Modified spelling errors.
- Added port@0.
- Modified '$ref' of port.
- Added 'ports' to 'required'.
- Dropped 'stfcamss' label in example.
- Added port@0 in example.
- Added MAINTAINERS file.
Patch 2:
- Split this patch into three new patches.
- Modified compatible property.
- Replaced clock names with the existing names.
- Modified 'bus-type' and 'clock-lanes'
- Added port@2 - port@4
- Dropped 'csi2rx' label in example.
Patch 3:
- Updated rst and dot file as three pipelines were deleted. Patch 4:
- Split this patch into three new patches.
- Dropped .s_power() and .get_fmt().
- Dropped CSI-2 DT support.
- Dropped v4l2_device_register_subdev_nodes().
- Used assigned-clock-rates in DT to set clk value.
- Modified 'compatible' field.
Patch 5:
- Deleted three pipelines.
- Modified 'stfcamss_clocks'/'stfcamss_resets' struct.
- Dropped stfcamss_find_sensor() function.
- Removed redundant code from stfcamss_of_parse_endpoint_node().
- Modified spelling errors.
- Rewrote stfcamss_reg_media_subdev_node() function.
- Modified stfcamss_subdev_notifier_bound().
- Modified stfcamss_probe() function.
- Dropped stfcamss_suspend() and stfcamss_resume().
- Dropped dev_info() in stfcamss_remove() function.
- Added 'stf_' prefix for enum subdev_type.
- Moved all includes to the top in stf_camss.h file.
- Dropped unused fields in stfcamss struct.
- Replaced Custom logging macros with regular macros.
- Rewrote register read and write functions.
- Used lowercase for all hex constants.
- Used macro to name registers.
- Dropped unused ioctl and stf_isp_ioctl.h file.

v2: https://lore.kernel.org/all/[email protected]/

Changes since v1:
- Deleted starfive,jh7110-mipi-csi2.yaml.
- Converted cdns,csi2rx.txt to cdns,csi2rx.yaml and added ‘resets’ properties.
- Added ‘cdns,csi2rx.yaml’ in ‘CADENCE MIPI-CSI2 BRIDGES’ entry.
- The following contents were modified in starfive,jh7110-camss.yaml:
dropped quotes from ’id’ and ‘schema’; dropped ‘|’ for ‘description’; corrected the wrong or redundant words: ‘a ISP’, ‘PD ISP’; dropped ‘minItems’ for ‘reg’, ‘clocks’, ‘resets’ and ‘interrupts’; dropped the '_clk' and 'rst_' prefix about the 'clock-names' and 'reset-names';
changed ‘endpoint@1’ to ‘endpoint’; updated examples;
- Updated Subject for some patches.
- Merged patch 6, 7, 8, 9, 10, 11 into one patch.

Jack Zhu (6):
media: dt-bindings: Add JH7110 Camera Subsystem
media: admin-guide: Add starfive_camss.rst for Starfive Camera
Subsystem
media: starfive: Add basic driver
media: starfive: Add video driver
media: starfive: Add ISP driver
media: starfive: Add VIN driver

 .../admin-guide/media/starfive_camss.rst      |   57 +
 .../media/starfive_camss_graph.dot            |   16 +
 .../admin-guide/media/v4l-drivers.rst         |    1 +
 .../bindings/media/starfive,jh7110-camss.yaml |  180 +++
 MAINTAINERS                                   |    9 +
 drivers/media/platform/Kconfig                |    1 +
 drivers/media/platform/Makefile               |    1 +
 drivers/media/platform/starfive/Kconfig       |   19 +
 drivers/media/platform/starfive/Makefile      |   14 +
 drivers/media/platform/starfive/stf_camss.c   |  476 +++++++
 drivers/media/platform/starfive/stf_camss.h   |  157 +++
 drivers/media/platform/starfive/stf_common.h  |   18 +
 drivers/media/platform/starfive/stf_isp.c     |  550 ++++++++
 drivers/media/platform/starfive/stf_isp.h     |  483 +++++++
 .../media/platform/starfive/stf_isp_hw_ops.c  |  468 +++++++
 drivers/media/platform/starfive/stf_video.c   |  864 +++++++++++++
 drivers/media/platform/starfive/stf_video.h   |   95 ++
 drivers/media/platform/starfive/stf_vin.c     | 1134 +++++++++++++++++
 drivers/media/platform/starfive/stf_vin.h     |  180 +++
 .../media/platform/starfive/stf_vin_hw_ops.c  |  241 ++++
 20 files changed, 4964 insertions(+)
 create mode 100644 Documentation/admin-guide/media/starfive_camss.rst
 create mode 100644 Documentation/admin-guide/media/starfive_camss_graph.dot
 create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
 create mode 100644 drivers/media/platform/starfive/Kconfig
 create mode 100644 drivers/media/platform/starfive/Makefile
 create mode 100644 drivers/media/platform/starfive/stf_camss.c
 create mode 100644 drivers/media/platform/starfive/stf_camss.h
 create mode 100644 drivers/media/platform/starfive/stf_common.h
 create mode 100644 drivers/media/platform/starfive/stf_isp.c
 create mode 100644 drivers/media/platform/starfive/stf_isp.h
 create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
 create mode 100644 drivers/media/platform/starfive/stf_video.c
 create mode 100644 drivers/media/platform/starfive/stf_video.h
 create mode 100644 drivers/media/platform/starfive/stf_vin.c
 create mode 100644 drivers/media/platform/starfive/stf_vin.h
 create mode 100644 drivers/media/platform/starfive/stf_vin_hw_ops.c