Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1675550 > unrolled thread

[PATCH RFC 0/2] Initial Allwinner V3s CSI Support

Back to article view | Back to linux.kernel

Contents

  [PATCH RFC 0/2] Initial Allwinner V3s CSI Support Yong Deng <yong.deng@magewell.com> - 2017-06-27 13:10 +0200
    [PATCH RFC 2/2] dt-bindings: add binding documentation for Allwinner CSI Yong Deng <yong.deng@magewell.com> - 2017-06-27 13:10 +0200
      Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Rob Herring <robh@kernel.org> - 2017-06-29 23:30 +0200
        Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Chen-Yu Tsai <wens@csie.org> - 2017-06-30 05:50 +0200
          Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Yong <yong.deng@magewell.com> - 2017-06-30 10:00 +0200
          Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Rob Herring <robh@kernel.org> - 2017-06-30 18:10 +0200
    Re: [PATCH RFC 1/2] media: V3s: Add support for Allwinner CSI. Hans Verkuil <hverkuil@xs4all.nl> - 2017-07-03 12:30 +0200
      Re: [PATCH RFC 1/2] media: V3s: Add support for Allwinner CSI. Yong <yong.deng@magewell.com> - 2017-07-03 13:10 +0200
        Re: [PATCH RFC 1/2] media: V3s: Add support for Allwinner CSI. Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-07-03 13:30 +0200

#1675550 — [PATCH RFC 0/2] Initial Allwinner V3s CSI Support

This patchset add initial support for Allwinner V3s CSI.

Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
and CSI1 is used for parallel interface. This is not documented in
datatsheet but by testing and guess.

This patchset implement a v4l2 framework driver and add a binding 
documentation for it.

Currently, the driver only support the parallel interface. And has been
tested with a BT1120 signal which generating from FPGA. The following
fetures are not support with this patchset:
  - ISP
  - MIPI-CSI2
  - Master clock for camera sensor
  - Power regulator for the front end IC

Yong Deng (2):
  media: V3s: Add support for Allwinner CSI.
  dt-bindings: add binding documentation for Allwinner CSI

 .../devicetree/bindings/media/sunxi-csi.txt        |  51 ++
 drivers/media/platform/Kconfig                     |   1 +
 drivers/media/platform/Makefile                    |   2 +
 drivers/media/platform/sunxi-csi/Kconfig           |   8 +
 drivers/media/platform/sunxi-csi/Makefile          |   3 +
 drivers/media/platform/sunxi-csi/sunxi_csi.c       | 535 +++++++++++++
 drivers/media/platform/sunxi-csi/sunxi_csi.h       | 203 +++++
 drivers/media/platform/sunxi-csi/sunxi_csi_v3s.c   | 827 +++++++++++++++++++++
 drivers/media/platform/sunxi-csi/sunxi_csi_v3s.h   | 206 +++++
 drivers/media/platform/sunxi-csi/sunxi_video.c     | 667 +++++++++++++++++
 drivers/media/platform/sunxi-csi/sunxi_video.h     |  61 ++
 11 files changed, 2564 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/sunxi-csi.txt
 create mode 100644 drivers/media/platform/sunxi-csi/Kconfig
 create mode 100644 drivers/media/platform/sunxi-csi/Makefile
 create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi.c
 create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi.h
 create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi_v3s.c
 create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi_v3s.h
 create mode 100644 drivers/media/platform/sunxi-csi/sunxi_video.c
 create mode 100644 drivers/media/platform/sunxi-csi/sunxi_video.h

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1675552 — [PATCH RFC 2/2] dt-bindings: add binding documentation for Allwinner CSI

Add binding documentation for Allwinner CSI.

Signed-off-by: Yong Deng <yong.deng@magewell.com>
---
 .../devicetree/bindings/media/sunxi-csi.txt        | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/sunxi-csi.txt

diff --git a/Documentation/devicetree/bindings/media/sunxi-csi.txt b/Documentation/devicetree/bindings/media/sunxi-csi.txt
new file mode 100644
index 0000000..770be0e
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/sunxi-csi.txt
@@ -0,0 +1,51 @@
+Allwinner V3s Camera Sensor Interface
+------------------------------
+
+Required properties:
+  - compatible: value must be "allwinner,sun8i-v3s-csi"
+  - reg: base address and size of the memory-mapped region.
+  - interrupts: interrupt associated to this IP
+  - clocks: phandles to the clocks feeding the CSI
+    * ahb: the CSI interface clock
+    * mod: the CSI module clock
+    * ram: the CSI DRAM clock
+  - clock-names: the clock names mentioned above
+  - resets: phandles to the reset line driving the CSI
+
+- ports: A ports node with endpoint definitions as defined in
+  Documentation/devicetree/bindings/media/video-interfaces.txt. The
+  first port should be the input endpoints, the second one the outputs
+
+Example:
+
+	csi1: csi@01cb4000 {
+		compatible = "allwinner,sun8i-v3s-csi";
+		reg = <0x01cb4000 0x1000>;
+		interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&ccu CLK_BUS_CSI>,
+			 <&ccu CLK_CSI1_SCLK>,
+			 <&ccu CLK_DRAM_CSI>;
+		clock-names = "ahb", "mod", "ram";
+		resets = <&ccu RST_BUS_CSI>;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			/* Parallel bus endpoint */
+			csi1_0: endpoint@0 {
+				reg = <0>;
+				remote = <&adv7611_1>;
+				bus-width = <16>;
+				data-shift = <0>;
+
+				/* If hsync-active/vsync-active are missing,
+				   embedded BT.656 sync is used */
+				hsync-active = <0>; /* Active low */
+				vsync-active = <0>; /* Active low */
+				data-active = <1>;  /* Active high */
+				pclk-sample = <1>;  /* Rising */
+			};
+		};
+	};
+
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1678208 — Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for Allwinner CSI

On Tue, Jun 27, 2017 at 07:07:34PM +0800, Yong Deng wrote:
> Add binding documentation for Allwinner CSI.

For the subject:

dt-bindings: media: Add Allwinner Camera Sensor Interface (CSI)

"binding documentation" is redundant.

> 
> Signed-off-by: Yong Deng <yong.deng@magewell.com>
> ---
>  .../devicetree/bindings/media/sunxi-csi.txt        | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/sunxi-csi.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/sunxi-csi.txt b/Documentation/devicetree/bindings/media/sunxi-csi.txt
> new file mode 100644
> index 0000000..770be0e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/sunxi-csi.txt
> @@ -0,0 +1,51 @@
> +Allwinner V3s Camera Sensor Interface
> +------------------------------
> +
> +Required properties:
> +  - compatible: value must be "allwinner,sun8i-v3s-csi"
> +  - reg: base address and size of the memory-mapped region.
> +  - interrupts: interrupt associated to this IP
> +  - clocks: phandles to the clocks feeding the CSI
> +    * ahb: the CSI interface clock
> +    * mod: the CSI module clock
> +    * ram: the CSI DRAM clock
> +  - clock-names: the clock names mentioned above
> +  - resets: phandles to the reset line driving the CSI
> +
> +- ports: A ports node with endpoint definitions as defined in
> +  Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +  first port should be the input endpoints, the second one the outputs

Is there more than one endpoint for each port? If so, need to define 
that numbering too.

> +
> +Example:
> +
> +	csi1: csi@01cb4000 {
> +		compatible = "allwinner,sun8i-v3s-csi";
> +		reg = <0x01cb4000 0x1000>;
> +		interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&ccu CLK_BUS_CSI>,
> +			 <&ccu CLK_CSI1_SCLK>,
> +			 <&ccu CLK_DRAM_CSI>;
> +		clock-names = "ahb", "mod", "ram";
> +		resets = <&ccu RST_BUS_CSI>;
> +
> +		port {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			/* Parallel bus endpoint */
> +			csi1_0: endpoint@0 {
> +				reg = <0>;

Don't need this and everything associated with it for a single endpoint. 

> +				remote = <&adv7611_1>;
> +				bus-width = <16>;
> +				data-shift = <0>;
> +
> +				/* If hsync-active/vsync-active are missing,
> +				   embedded BT.656 sync is used */
> +				hsync-active = <0>; /* Active low */
> +				vsync-active = <0>; /* Active low */
> +				data-active = <1>;  /* Active high */
> +				pclk-sample = <1>;  /* Rising */
> +			};
> +		};
> +	};
> +
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1678442 — Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for Allwinner CSI

On Fri, Jun 30, 2017 at 5:19 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jun 27, 2017 at 07:07:34PM +0800, Yong Deng wrote:
>> Add binding documentation for Allwinner CSI.
>
> For the subject:
>
> dt-bindings: media: Add Allwinner Camera Sensor Interface (CSI)
>
> "binding documentation" is redundant.
>
>>
>> Signed-off-by: Yong Deng <yong.deng@magewell.com>
>> ---
>>  .../devicetree/bindings/media/sunxi-csi.txt        | 51 ++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/sunxi-csi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/sunxi-csi.txt b/Documentation/devicetree/bindings/media/sunxi-csi.txt
>> new file mode 100644
>> index 0000000..770be0e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/sunxi-csi.txt
>> @@ -0,0 +1,51 @@
>> +Allwinner V3s Camera Sensor Interface
>> +------------------------------
>> +
>> +Required properties:
>> +  - compatible: value must be "allwinner,sun8i-v3s-csi"
>> +  - reg: base address and size of the memory-mapped region.
>> +  - interrupts: interrupt associated to this IP
>> +  - clocks: phandles to the clocks feeding the CSI
>> +    * ahb: the CSI interface clock
>> +    * mod: the CSI module clock
>> +    * ram: the CSI DRAM clock
>> +  - clock-names: the clock names mentioned above
>> +  - resets: phandles to the reset line driving the CSI
>> +
>> +- ports: A ports node with endpoint definitions as defined in
>> +  Documentation/devicetree/bindings/media/video-interfaces.txt. The
>> +  first port should be the input endpoints, the second one the outputs
>
> Is there more than one endpoint for each port? If so, need to define
> that numbering too.

It is possible to have multiple camera sensors connected to the same
bus. Think front and back cameras on a cell phone or tablet.

I don't think any kind of numbering makes much sense though. The
system is free to use just one sensor at a time, or use many with
some time multiplexing scheme. What might matter to the end user
is where the camera is placed. But using the position or orientation
as a numbering scheme might not work well either. Someone may end
up using two sensors with the same orientation for stereoscopic
vision.

>
>> +
>> +Example:
>> +
>> +     csi1: csi@01cb4000 {
>> +             compatible = "allwinner,sun8i-v3s-csi";
>> +             reg = <0x01cb4000 0x1000>;

Yong, the address range size is 0x4000, including the CCI (I2C)
controller at offset 0x3000. You should also consider this in
the device tree binding, and the driver.

ChenYu

>> +             interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
>> +             clocks = <&ccu CLK_BUS_CSI>,
>> +                      <&ccu CLK_CSI1_SCLK>,
>> +                      <&ccu CLK_DRAM_CSI>;
>> +             clock-names = "ahb", "mod", "ram";
>> +             resets = <&ccu RST_BUS_CSI>;
>> +
>> +             port {
>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +
>> +                     /* Parallel bus endpoint */
>> +                     csi1_0: endpoint@0 {
>> +                             reg = <0>;
>
> Don't need this and everything associated with it for a single endpoint.
>
>> +                             remote = <&adv7611_1>;
>> +                             bus-width = <16>;
>> +                             data-shift = <0>;
>> +
>> +                             /* If hsync-active/vsync-active are missing,
>> +                                embedded BT.656 sync is used */
>> +                             hsync-active = <0>; /* Active low */
>> +                             vsync-active = <0>; /* Active low */
>> +                             data-active = <1>;  /* Active high */
>> +                             pclk-sample = <1>;  /* Rising */
>> +                     };
>> +             };
>> +     };
>> +
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1678574 — Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for Allwinner CSI

On Fri, 30 Jun 2017 11:41:50 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

> On Fri, Jun 30, 2017 at 5:19 AM, Rob Herring <robh@kernel.org> wrote:
> > On Tue, Jun 27, 2017 at 07:07:34PM +0800, Yong Deng wrote:
> >> Add binding documentation for Allwinner CSI.
> >
> > For the subject:
> >
> > dt-bindings: media: Add Allwinner Camera Sensor Interface (CSI)
> >
> > "binding documentation" is redundant.

OK.

> >
> >>
> >> Signed-off-by: Yong Deng <yong.deng@magewell.com>
> >> ---
> >>  .../devicetree/bindings/media/sunxi-csi.txt        | 51 ++++++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/media/sunxi-csi.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/sunxi-csi.txt b/Documentation/devicetree/bindings/media/sunxi-csi.txt
> >> new file mode 100644
> >> index 0000000..770be0e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/sunxi-csi.txt
> >> @@ -0,0 +1,51 @@
> >> +Allwinner V3s Camera Sensor Interface
> >> +------------------------------
> >> +
> >> +Required properties:
> >> +  - compatible: value must be "allwinner,sun8i-v3s-csi"
> >> +  - reg: base address and size of the memory-mapped region.
> >> +  - interrupts: interrupt associated to this IP
> >> +  - clocks: phandles to the clocks feeding the CSI
> >> +    * ahb: the CSI interface clock
> >> +    * mod: the CSI module clock
> >> +    * ram: the CSI DRAM clock
> >> +  - clock-names: the clock names mentioned above
> >> +  - resets: phandles to the reset line driving the CSI
> >> +
> >> +- ports: A ports node with endpoint definitions as defined in
> >> +  Documentation/devicetree/bindings/media/video-interfaces.txt. The
> >> +  first port should be the input endpoints, the second one the outputs
> >
> > Is there more than one endpoint for each port? If so, need to define
> > that numbering too.

I made a mistake here.
"The first port should be the input endpoints, the second one the outputs"
is wrong and redundant.

> 
> It is possible to have multiple camera sensors connected to the same
> bus. Think front and back cameras on a cell phone or tablet.
> 
> I don't think any kind of numbering makes much sense though. The
> system is free to use just one sensor at a time, or use many with
> some time multiplexing scheme. What might matter to the end user
> is where the camera is placed. But using the position or orientation
> as a numbering scheme might not work well either. Someone may end
> up using two sensors with the same orientation for stereoscopic
> vision.

It is! But multiple sensors can't be working on the same bus 
simultaneously. So, the driver should have capability to switch
input device. But my driver does not support switching between 
multiple camera sensors for now! The driver only pickup the first
valid one. Maybe it's a feature to implement in the future.

Maybe like this:
- port: A port node with endpoint definitions as defined in
  Documentation/devicetree/bindings/media/video-interfaces.txt.
  A CSI have only one port. But you can define multiple endpoint
  under the port. The driver will pick up the first valid one.

> 
> >
> >> +
> >> +Example:
> >> +
> >> +     csi1: csi@01cb4000 {
> >> +             compatible = "allwinner,sun8i-v3s-csi";
> >> +             reg = <0x01cb4000 0x1000>;
> 
> Yong, the address range size is 0x4000, including the CCI (I2C)
> controller at offset 0x3000. You should also consider this in
> the device tree binding, and the driver.
> 
> ChenYu

For V3s, the reg range <0x01cb0000 0x4000> of CSI0 have 4 module:
  * <0x01cb0000 0x1000>	-- CSI0
  * <0x01cb1000 0x1000>	-- MIPI-CSI
  * <0x01cb2000 0x1000>	-- MIPI-DPHY
  * <0x01cb3000 0x1000>	-- CCI
the reg range <0x01cb4000 0x4000> of CSI1 have 1 module:
  * <0x01cb4000 0x1000>	-- CSI1

And, I think the CCI, MIPI-CSI, MIPI-DPHY should have their own
device tree binding and driver.

> 
> >> +             interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> >> +             clocks = <&ccu CLK_BUS_CSI>,
> >> +                      <&ccu CLK_CSI1_SCLK>,
> >> +                      <&ccu CLK_DRAM_CSI>;
> >> +             clock-names = "ahb", "mod", "ram";
> >> +             resets = <&ccu RST_BUS_CSI>;
> >> +
> >> +             port {
> >> +                     #address-cells = <1>;
> >> +                     #size-cells = <0>;
> >> +
> >> +                     /* Parallel bus endpoint */
> >> +                     csi1_0: endpoint@0 {
> >> +                             reg = <0>;
> >
> > Don't need this and everything associated with it for a single endpoint.

Like this?
            csi1_ep: endpoint {
                remote-endpoint = <&adv7611_ep>;
                bus-width = <16>;
                data-shift = <0>;
    
                /* If hsync-active/vsync-active are missing,
                   embedded BT.656 sync is used */
                hsync-active = <0>; /* Active low */
                vsync-active = <0>; /* Active low */
                data-active = <1>;  /* Active high */
                pclk-sample = <1>;  /* Rising */
            };

The property "remote" should be "remote-endpoint".
Is there a mistake in video-interfaces.txt's example?

> >
> >> +                             remote = <&adv7611_1>;
> >> +                             bus-width = <16>;
> >> +                             data-shift = <0>;
> >> +
> >> +                             /* If hsync-active/vsync-active are missing,
> >> +                                embedded BT.656 sync is used */
> >> +                             hsync-active = <0>; /* Active low */
> >> +                             vsync-active = <0>; /* Active low */
> >> +                             data-active = <1>;  /* Active high */
> >> +                             pclk-sample = <1>;  /* Rising */
> >> +                     };
> >> +             };
> >> +     };
> >> +
> >> --
> >> 1.8.3.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1678953 — Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for Allwinner CSI

On Thu, Jun 29, 2017 at 10:41 PM, Chen-Yu Tsai <wens@csie.org> wrote:
> On Fri, Jun 30, 2017 at 5:19 AM, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Jun 27, 2017 at 07:07:34PM +0800, Yong Deng wrote:
>>> Add binding documentation for Allwinner CSI.
>>
>> For the subject:
>>
>> dt-bindings: media: Add Allwinner Camera Sensor Interface (CSI)
>>
>> "binding documentation" is redundant.
>>
>>>
>>> Signed-off-by: Yong Deng <yong.deng@magewell.com>
>>> ---
>>>  .../devicetree/bindings/media/sunxi-csi.txt        | 51 ++++++++++++++++++++++
>>>  1 file changed, 51 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/sunxi-csi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/sunxi-csi.txt b/Documentation/devicetree/bindings/media/sunxi-csi.txt
>>> new file mode 100644
>>> index 0000000..770be0e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/sunxi-csi.txt
>>> @@ -0,0 +1,51 @@
>>> +Allwinner V3s Camera Sensor Interface
>>> +------------------------------
>>> +
>>> +Required properties:
>>> +  - compatible: value must be "allwinner,sun8i-v3s-csi"
>>> +  - reg: base address and size of the memory-mapped region.
>>> +  - interrupts: interrupt associated to this IP
>>> +  - clocks: phandles to the clocks feeding the CSI
>>> +    * ahb: the CSI interface clock
>>> +    * mod: the CSI module clock
>>> +    * ram: the CSI DRAM clock
>>> +  - clock-names: the clock names mentioned above
>>> +  - resets: phandles to the reset line driving the CSI
>>> +
>>> +- ports: A ports node with endpoint definitions as defined in
>>> +  Documentation/devicetree/bindings/media/video-interfaces.txt. The
>>> +  first port should be the input endpoints, the second one the outputs
>>
>> Is there more than one endpoint for each port? If so, need to define
>> that numbering too.
>
> It is possible to have multiple camera sensors connected to the same
> bus. Think front and back cameras on a cell phone or tablet.
>
> I don't think any kind of numbering makes much sense though. The
> system is free to use just one sensor at a time, or use many with
> some time multiplexing scheme. What might matter to the end user
> is where the camera is placed. But using the position or orientation
> as a numbering scheme might not work well either. Someone may end
> up using two sensors with the same orientation for stereoscopic
> vision.

Well, for muxing, you need to no which endpoint is which mux input,
but if the muxing is at the board level, then that's really outside
this binding. For stereoscopic, don't you need both sensors to work at
the same time (i.e. not muxed). That would be multiple ports.

When would you have 2 output endpoints though? That could be to
different processing blocks, but those connections are internal,
fixed, and known. So you should document the numbering in that case.

Rob

[toc] | [prev] | [next] | [standalone]

#1679868 — Re: [PATCH RFC 1/2] media: V3s: Add support for Allwinner CSI.

Hi Yong Deng,

Thanks for contributing this driver!

First a high-level comment: you need to rebase this to the latest media_tree
master branch (https://git.linuxtv.org/media_tree.git/) since v4l2-of.h has
been replaced by v4l2-fwnode.h. So this driver will not apply as-is.

Also missing is a patch adding a new entry to the MAINTAINERS file.

I have some more comments below:

On 06/27/2017 01:07 PM, Yong Deng wrote:
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datatsheet but by testing and guess.

datatsheet -> datasheet

> 
> This patch implement a v4l2 framework driver for it.
> 
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
> 
> Signed-off-by: Yong Deng <yong.deng@magewell.com>
> ---
>   drivers/media/platform/Kconfig                   |   1 +
>   drivers/media/platform/Makefile                  |   2 +
>   drivers/media/platform/sunxi-csi/Kconfig         |   8 +
>   drivers/media/platform/sunxi-csi/Makefile        |   3 +
>   drivers/media/platform/sunxi-csi/sunxi_csi.c     | 535 +++++++++++++++
>   drivers/media/platform/sunxi-csi/sunxi_csi.h     | 203 ++++++
>   drivers/media/platform/sunxi-csi/sunxi_csi_v3s.c | 827 +++++++++++++++++++++++
>   drivers/media/platform/sunxi-csi/sunxi_csi_v3s.h | 206 ++++++
>   drivers/media/platform/sunxi-csi/sunxi_video.c   | 667 ++++++++++++++++++
>   drivers/media/platform/sunxi-csi/sunxi_video.h   |  61 ++
>   10 files changed, 2513 insertions(+)
>   create mode 100644 drivers/media/platform/sunxi-csi/Kconfig
>   create mode 100644 drivers/media/platform/sunxi-csi/Makefile
>   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi.c
>   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi.h
>   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi_v3s.c
>   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi_v3s.h
>   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_video.c
>   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_video.h

<snip>

> diff --git a/drivers/media/platform/sunxi-csi/Kconfig b/drivers/media/platform/sunxi-csi/Kconfig
> new file mode 100644
> index 0000000..f26592a
> --- /dev/null
> +++ b/drivers/media/platform/sunxi-csi/Kconfig
> @@ -0,0 +1,8 @@
> +config VIDEO_SUNXI_CSI
> +	tristate "Allwinner Camera Sensor Interface driver"
> +	depends on VIDEO_V4L2 && COMMON_CLK && VIDEO_V4L2_SUBDEV_API && HAS_DMA
> +	depends on ARCH_SUNXI

If possible change this to:

	depends on ARCH_SUNXI || COMPILE_TEST

to allow this driver to be compiled on e.g. Intel for compile testing.

> +	select VIDEOBUF2_DMA_CONTIG
> +	select REGMAP_MMIO
> +	---help---
> +	   Support for the Allwinner Camera Sensor Interface Controller.

This controller is the same for all Allwinner SoC models?

<snip>

> diff --git a/drivers/media/platform/sunxi-csi/sunxi_video.c b/drivers/media/platform/sunxi-csi/sunxi_video.c
> new file mode 100644
> index 0000000..57d7563
> --- /dev/null
> +++ b/drivers/media/platform/sunxi-csi/sunxi_video.c
> @@ -0,0 +1,667 @@
> +/*
> + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
> + * All rights reserved.
> + * Author: Yong Deng <yong.deng@magewell.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/of.h>
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mc.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#include "sunxi_csi.h"
> +#include "sunxi_video.h"
> +
> +struct sunxi_csi_buffer {
> +	struct vb2_v4l2_buffer		vb;
> +	struct list_head		list;
> +
> +	dma_addr_t			dma_addr;
> +};
> +
> +static struct sunxi_csi_format *
> +find_format_by_fourcc(struct sunxi_video *video, unsigned int fourcc)
> +{
> +	unsigned int num_formats = video->num_formats;
> +	struct sunxi_csi_format *fmt;
> +	unsigned int i;
> +
> +	for (i = 0; i < num_formats; i++) {
> +		fmt = &video->formats[i];
> +		if (fmt->fourcc == fourcc)
> +			return fmt;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct v4l2_subdev *
> +sunxi_video_remote_subdev(struct sunxi_video *video, u32 *pad)
> +{
> +	struct media_pad *remote;
> +
> +	remote = media_entity_remote_pad(&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 sunxi_video_queue_setup(struct vb2_queue *vq,
> +				 unsigned int *nbuffers, unsigned int *nplanes,
> +				 unsigned int sizes[],
> +				 struct device *alloc_devs[])
> +{
> +	struct sunxi_video *video = vb2_get_drv_priv(vq);
> +	unsigned int size = video->fmt.fmt.pix.sizeimage;
> +
> +	if (*nplanes)
> +		return sizes[0] < size ? -EINVAL : 0;
> +
> +	*nplanes = 1;
> +	sizes[0] = size;
> +
> +	return 0;
> +}
> +
> +static int sunxi_video_buffer_prepare(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct sunxi_csi_buffer *buf =
> +			container_of(vbuf, struct sunxi_csi_buffer, vb);
> +	struct sunxi_video *video = vb2_get_drv_priv(vb->vb2_queue);
> +	unsigned long size = video->fmt.fmt.pix.sizeimage;
> +
> +	if (vb2_plane_size(vb, 0) < size) {
> +		v4l2_err(video->vdev.v4l2_dev, "buffer too small (%lu < %lu)\n",
> +			 vb2_plane_size(vb, 0), size);
> +		return -EINVAL;
> +	}
> +
> +	vb2_set_plane_payload(vb, 0, size);
> +
> +	buf->dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
> +
> +	vbuf->field = video->fmt.fmt.pix.field;
> +
> +	return 0;
> +}
> +
> +static int sunxi_pipeline_set_stream(struct sunxi_video *video, bool enable)
> +{
> +	struct media_entity *entity;
> +	struct media_pad *pad;
> +	struct v4l2_subdev *subdev;
> +	int ret;
> +
> +	entity = &video->vdev.entity;
> +	while (1) {
> +		pad = &entity->pads[0];
> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> +			break;
> +
> +		pad = media_entity_remote_pad(pad);
> +		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, enable);
> +		if (enable && ret < 0 && ret != -ENOIOCTLCMD)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunxi_video_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> +	struct sunxi_video *video = vb2_get_drv_priv(vq);
> +	struct sunxi_csi_buffer *buf;
> +	struct sunxi_csi_config config;
> +	unsigned long flags;
> +	int ret;
> +
> +	video->sequence = 0;
> +
> +	ret = media_pipeline_start(&video->vdev.entity, &video->vdev.pipe);
> +	if (ret < 0)
> +		goto err_start_pipeline;
> +
> +	ret = sunxi_pipeline_set_stream(video, true);
> +	if (ret < 0)
> +		goto err_start_stream;
> +
> +	config.pixelformat = video->fmt.fmt.pix.pixelformat;
> +	config.code = video->current_fmt->mbus_code;
> +	config.field = video->fmt.fmt.pix.field;
> +	config.width = video->fmt.fmt.pix.width;
> +	config.height = video->fmt.fmt.pix.height;
> +
> +	ret = sunxi_csi_update_config(video->csi, &config);
> +	if (ret < 0)
> +		goto err_update_config;
> +
> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> +	video->cur_frm = list_first_entry(&video->dma_queue,
> +					  struct sunxi_csi_buffer, list);
> +	list_del(&video->cur_frm->list);
> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> +
> +	ret = sunxi_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> +	if (ret < 0)
> +		goto err_update_addr;
> +
> +	ret = sunxi_csi_set_stream(video->csi, true);
> +	if (ret < 0)
> +		goto err_csi_stream;
> +
> +	return 0;
> +
> +err_csi_stream:
> +err_update_addr:
> +err_update_config:
> +	sunxi_pipeline_set_stream(video, false);
> +err_start_stream:
> +	media_pipeline_stop(&video->vdev.entity);
> +err_start_pipeline:
> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> +	list_for_each_entry(buf, &video->dma_queue, list)
> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> +	INIT_LIST_HEAD(&video->dma_queue);
> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> +
> +	return ret;
> +}
> +
> +static void sunxi_video_stop_streaming(struct vb2_queue *vq)
> +{
> +	struct sunxi_video *video = vb2_get_drv_priv(vq);
> +	unsigned long flags;
> +	struct sunxi_csi_buffer *buf;
> +
> +	sunxi_pipeline_set_stream(video, false);
> +
> +	sunxi_csi_set_stream(video->csi, false);
> +
> +	media_pipeline_stop(&video->vdev.entity);
> +
> +	/* Release all active buffers */
> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> +	if (unlikely(video->cur_frm)) {
> +		vb2_buffer_done(&video->cur_frm->vb.vb2_buf,
> +				VB2_BUF_STATE_ERROR);
> +		video->cur_frm = NULL;
> +	}
> +	list_for_each_entry(buf, &video->dma_queue, list)
> +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +	INIT_LIST_HEAD(&video->dma_queue);
> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> +}
> +
> +static void sunxi_video_buffer_queue(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct sunxi_csi_buffer *buf =
> +			container_of(vbuf, struct sunxi_csi_buffer, vb);
> +	struct sunxi_video *video = vb2_get_drv_priv(vb->vb2_queue);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> +	if (!video->cur_frm && list_empty(&video->dma_queue) &&
> +		vb2_is_streaming(vb->vb2_queue)) {
> +		video->cur_frm = buf;
> +		sunxi_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> +		sunxi_csi_set_stream(video->csi, 1);
> +	} else
> +		list_add_tail(&buf->list, &video->dma_queue);
> +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> +}
> +
> +void sunxi_video_frame_done(struct sunxi_video *video)
> +{
> +	spin_lock(&video->dma_queue_lock);
> +
> +	if (video->cur_frm) {
> +		struct vb2_v4l2_buffer *vbuf = &video->cur_frm->vb;
> +		struct vb2_buffer *vb = &vbuf->vb2_buf;
> +
> +		vb->timestamp = ktime_get_ns();
> +		vbuf->sequence = video->sequence++;
> +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> +		video->cur_frm = NULL;
> +	}
> +
> +	if (!list_empty(&video->dma_queue)
> +	    && vb2_is_streaming(&video->vb2_vidq)) {
> +		video->cur_frm = list_first_entry(&video->dma_queue,
> +				struct sunxi_csi_buffer, list);
> +		list_del(&video->cur_frm->list);
> +		sunxi_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> +	} else
> +		sunxi_csi_set_stream(video->csi, 0);
> +
> +	spin_unlock(&video->dma_queue_lock);
> +}
> +
> +static struct vb2_ops sunxi_csi_vb2_ops = {
> +	.queue_setup		= sunxi_video_queue_setup,
> +	.wait_prepare		= vb2_ops_wait_prepare,
> +	.wait_finish		= vb2_ops_wait_finish,
> +	.buf_prepare		= sunxi_video_buffer_prepare,
> +	.start_streaming	= sunxi_video_start_streaming,
> +	.stop_streaming		= sunxi_video_stop_streaming,
> +	.buf_queue		= sunxi_video_buffer_queue,
> +};
> +
> +static int vidioc_querycap(struct file *file, void *priv,
> +				struct v4l2_capability *cap)
> +{
> +	struct sunxi_video *video = video_drvdata(file);
> +
> +	strlcpy(cap->driver, "sunxi-video", sizeof(cap->driver));
> +	strlcpy(cap->card, video->vdev.name, sizeof(cap->card));
> +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> +		 video->csi->dev->of_node->name);
> +
> +	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;

Drop these two lines. You already set device_caps in the video_device struct,
and the V4L2 core will fill in these two v4l2_capability fields based on that.

> +
> +	return 0;
> +}
> +
> +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
> +				   struct v4l2_fmtdesc *f)
> +{
> +	struct sunxi_video *video = video_drvdata(file);
> +	u32 index = f->index;
> +
> +	if (index >= video->num_formats)
> +		return -EINVAL;
> +
> +	f->pixelformat = video->formats[index].fourcc;
> +
> +	return 0;
> +}
> +
> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> +				struct v4l2_format *fmt)
> +{
> +	struct sunxi_video *video = video_drvdata(file);
> +
> +	*fmt = video->fmt;
> +
> +	return 0;
> +}
> +
> +static int sunxi_video_try_fmt(struct sunxi_video *video, struct v4l2_format *f,
> +			       struct sunxi_csi_format **current_fmt)
> +{
> +	struct sunxi_csi_format *csi_fmt;
> +	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
> +	struct v4l2_subdev_format format;
> +	struct v4l2_subdev *subdev;
> +	u32 pad;
> +	int ret;
> +
> +	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;

Drop this test. It's not needed.

> +
> +	subdev = sunxi_video_remote_subdev(video, &pad);
> +	if (subdev == NULL)
> +		return -ENXIO;
> +
> +	csi_fmt = find_format_by_fourcc(video, pixfmt->pixelformat);
> +	if (csi_fmt == NULL)
> +		return -EINVAL;
> +
> +	format.pad = pad;
> +	format.which = V4L2_SUBDEV_FORMAT_TRY;
> +	v4l2_fill_mbus_format(&format.format, pixfmt, csi_fmt->mbus_code);
> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format);
> +	if (ret)
> +		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;

Just do 'return ret'.

> +
> +	v4l2_fill_pix_format(pixfmt, &format.format);
> +
> +	pixfmt->bytesperline = (pixfmt->width * csi_fmt->bpp) >> 3;
> +	pixfmt->sizeimage = (pixfmt->width * csi_fmt->bpp * pixfmt->height) / 8;
> +
> +	if (current_fmt)
> +		*current_fmt = csi_fmt;
> +
> +	return 0;
> +}
> +
> +static int sunxi_video_set_fmt(struct sunxi_video *video, struct v4l2_format *f)
> +{
> +	struct v4l2_subdev_format format;
> +	struct sunxi_csi_format *current_fmt;
> +	struct v4l2_subdev *subdev;
> +	u32 pad;
> +	int ret;
> +
> +	subdev = sunxi_video_remote_subdev(video, &pad);
> +	if (subdev == NULL)
> +		return -ENXIO;
> +
> +	ret = sunxi_video_try_fmt(video, f, &current_fmt);
> +	if (ret)
> +		return ret;
> +
> +	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	v4l2_fill_mbus_format(&format.format, &f->fmt.pix,
> +			      current_fmt->mbus_code);
> +	ret = v4l2_subdev_call(subdev, pad, set_fmt, NULL, &format);
> +	if (ret < 0)
> +		return ret;
> +
> +	video->fmt = *f;
> +	video->current_fmt = current_fmt;
> +
> +	return 0;
> +}
> +
> +static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> +				struct v4l2_format *f)
> +{
> +	struct sunxi_video *video = video_drvdata(file);
> +
> +	if (vb2_is_streaming(&video->vb2_vidq))
> +		return -EBUSY;
> +
> +	return sunxi_video_set_fmt(video, f);
> +}
> +
> +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> +				  struct v4l2_format *f)
> +{
> +	struct sunxi_video *video = video_drvdata(file);
> +
> +	return sunxi_video_try_fmt(video, f, NULL);
> +}
> +
> +static const struct v4l2_ioctl_ops sunxi_video_ioctl_ops = {
> +	.vidioc_querycap		= vidioc_querycap,
> +	.vidioc_enum_fmt_vid_cap	= vidioc_enum_fmt_vid_cap,
> +	.vidioc_g_fmt_vid_cap		= vidioc_g_fmt_vid_cap,
> +	.vidioc_s_fmt_vid_cap		= vidioc_s_fmt_vid_cap,
> +	.vidioc_try_fmt_vid_cap		= vidioc_try_fmt_vid_cap,
> +
> +	.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,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * V4L2 file operations
> + */
> +static int sunxi_video_open(struct file *file)
> +{
> +	struct sunxi_video *video = video_drvdata(file);
> +	struct v4l2_format format;
> +	int ret;
> +
> +	if (mutex_lock_interruptible(&video->lock))
> +		return -ERESTARTSYS;
> +
> +	ret = v4l2_fh_open(file);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	ret = v4l2_pipeline_pm_use(&video->vdev.entity, 1);
> +	if (ret < 0)
> +		goto fh_release;
> +
> +	if (!v4l2_fh_is_singular_file(file))
> +		goto unlock;
> +
> +	ret = sunxi_csi_set_power(video->csi, true);
> +	if (ret < 0)
> +		goto fh_release;
> +
> +	/* setup default format */
> +	if (video->num_formats > 0) {
> +		format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +		format.fmt.pix.width = 1280;
> +		format.fmt.pix.height = 720;
> +		format.fmt.pix.pixelformat = video->formats[0].fourcc;
> +		sunxi_video_set_fmt(video, &format);
> +	}
> +
> +	mutex_unlock(&video->lock);
> +	return 0;
> +
> +fh_release:
> +	v4l2_fh_release(file);
> +unlock:
> +	mutex_unlock(&video->lock);
> +	return ret;
> +}
> +
> +static int sunxi_video_close(struct file *file)
> +{
> +	struct sunxi_video *video = video_drvdata(file);
> +
> +	mutex_lock(&video->lock);
> +
> +	_vb2_fop_release(file, NULL);
> +
> +	v4l2_pipeline_pm_use(&video->vdev.entity, 0);
> +
> +	if (v4l2_fh_is_singular_file(file))

This is the wrong order. You need to check this before _vb2_fop_release is called.

E.g.:

	bool last_fh = v4l2_fh_is_singular_file(file);

	_vb2_fop_release(file, NULL);
	...
	if (last_fh)
		sunxi_csi_set_power(video->csi, false);

> +		sunxi_csi_set_power(video->csi, false);
> +
> +	mutex_unlock(&video->lock);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_file_operations sunxi_video_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= sunxi_video_open,
> +	.release	= sunxi_video_close,
> +	.unlocked_ioctl	= video_ioctl2,
> +	.read		= vb2_fop_read,

You haven't signalled read() support in the capabilities or in the vb2 io_modes, so
either drop this from the video_fops, or add it to the device_caps and io_modes fields.

> +	.mmap		= vb2_fop_mmap,
> +	.poll		= vb2_fop_poll
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Media Operations
> + */
> +static int sunxi_video_formats_init(struct sunxi_video *video)
> +{
> +	struct v4l2_subdev_mbus_code_enum mbus_code = { 0 };
> +	struct sunxi_csi *csi = video->csi;
> +	struct v4l2_subdev *subdev;
> +	u32 pad;
> +	const u32 *pixformats;
> +	int pixformat_count = 0;
> +	u32 subdev_codes[32]; /* subdev format codes, 32 should be enough */
> +	int codes_count = 0;
> +	int num_fmts = 0;
> +	int i, j;
> +
> +	subdev = sunxi_video_remote_subdev(video, &pad);
> +	if (subdev == NULL)
> +		return -ENXIO;
> +
> +	/* Get supported pixformats of CSI */
> +	pixformat_count = sunxi_csi_get_supported_pixformats(csi, &pixformats);
> +	if (pixformat_count <= 0)
> +		return -ENXIO;
> +
> +	/* Get subdev formats codes */
> +	mbus_code.pad = pad;
> +	mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL,
> +			&mbus_code)) {
> +		subdev_codes[codes_count] = mbus_code.code;
> +		codes_count++;
> +		mbus_code.index++;
> +	}
> +
> +	if (!codes_count)
> +		return -ENXIO;
> +
> +	/* Get supported formats count */
> +	for (i = 0; i < codes_count; i++) {
> +		for (j = 0; j < pixformat_count; j++) {
> +			if (!sunxi_csi_is_format_support(csi, pixformats[j],
> +					mbus_code.code)) {
> +				continue;
> +			}
> +			num_fmts++;
> +		}
> +	}
> +
> +	if (!num_fmts)
> +		return -ENXIO;
> +
> +	video->num_formats = num_fmts;
> +	video->formats = devm_kcalloc(video->csi->dev, num_fmts,
> +			sizeof(struct sunxi_csi_format), GFP_KERNEL);
> +	if (!video->formats) {
> +		dev_err(video->csi->dev, "could not allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Get supported formats */
> +	num_fmts = 0;
> +	for (i = 0; i < codes_count; i++) {
> +		for (j = 0; j < pixformat_count; j++) {
> +			if (!sunxi_csi_is_format_support(csi, pixformats[j],
> +					mbus_code.code)) {
> +				continue;
> +			}
> +
> +			video->formats[num_fmts].fourcc = pixformats[j];
> +			video->formats[num_fmts].mbus_code =
> +					mbus_code.code;
> +			video->formats[num_fmts].bpp =
> +					v4l2_pixformat_get_bpp(pixformats[j]);
> +			num_fmts++;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunxi_video_link_setup(struct media_entity *entity,
> +				  const struct media_pad *local,
> +				  const struct media_pad *remote, u32 flags)
> +{
> +	struct video_device *vdev = media_entity_to_video_device(entity);
> +	struct sunxi_video *video = video_get_drvdata(vdev);
> +
> +	if (WARN_ON(video == NULL))
> +		return 0;
> +
> +	return sunxi_video_formats_init(video);
> +}
> +
> +static const struct media_entity_operations sunxi_video_media_ops = {
> +	.link_setup = sunxi_video_link_setup,
> +};
> +
> +int sunxi_video_init(struct sunxi_video *video, struct sunxi_csi *csi,
> +		     const char *name)
> +{
> +	struct video_device *vdev = &video->vdev;
> +	struct vb2_queue *vidq = &video->vb2_vidq;
> +	int ret;
> +
> +	video->csi = csi;
> +
> +	/* Initialize the media entity... */
> +	video->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> +	vdev->entity.ops = &sunxi_video_media_ops;
> +	ret = media_entity_pads_init(&vdev->entity, 1, &video->pad);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_init(&video->lock);
> +
> +	INIT_LIST_HEAD(&video->dma_queue);
> +	spin_lock_init(&video->dma_queue_lock);
> +
> +	video->cur_frm = NULL;
> +	video->sequence = 0;
> +	video->num_formats = 0;
> +
> +	/* Initialize videobuf2 queue */
> +	vidq->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	vidq->io_modes			= VB2_MMAP | VB2_DMABUF;
> +	vidq->drv_priv			= video;
> +	vidq->buf_struct_size		= sizeof(struct sunxi_csi_buffer);
> +	vidq->ops			= &sunxi_csi_vb2_ops;
> +	vidq->mem_ops			= &vb2_dma_contig_memops;
> +	vidq->timestamp_flags		= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +	vidq->lock			= &video->lock;
> +	vidq->min_buffers_needed	= 1;
> +	vidq->dev			= csi->dev;
> +
> +	ret = vb2_queue_init(vidq);
> +	if (ret) {
> +		v4l2_err(&csi->v4l2_dev, "vb2_queue_init failed: %d\n", ret);
> +		goto error;
> +	}
> +
> +	/* Register video device */
> +	strlcpy(vdev->name, name, sizeof(vdev->name));
> +	vdev->release		= video_device_release_empty;
> +	vdev->fops		= &sunxi_video_fops;
> +	vdev->ioctl_ops		= &sunxi_video_ioctl_ops;
> +	vdev->vfl_type		= VFL_TYPE_GRABBER;
> +	vdev->vfl_dir		= VFL_DIR_RX;
> +	vdev->v4l2_dev		= &csi->v4l2_dev;
> +	vdev->queue		= vidq;
> +	vdev->lock		= &video->lock;
> +	vdev->device_caps	= V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE;
> +	video_set_drvdata(vdev, video);
> +
> +	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> +	if (ret < 0) {
> +		v4l2_err(&csi->v4l2_dev,
> +			 "video_register_device failed: %d\n", ret);
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	sunxi_video_cleanup(video);
> +	return ret;
> +}
> +
> +void sunxi_video_cleanup(struct sunxi_video *video)
> +{
> +	if (video_is_registered(&video->vdev))
> +		video_unregister_device(&video->vdev);
> +
> +	media_entity_cleanup(&video->vdev.entity);
> +}

<snip>

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1679893 — Re: [PATCH RFC 1/2] media: V3s: Add support for Allwinner CSI.

Hi Hans,

Thanks for your review.

On Mon, 3 Jul 2017 12:18:55 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Hi Yong Deng,
> 
> Thanks for contributing this driver!
> 
> First a high-level comment: you need to rebase this to the latest media_tree
> master branch (https://git.linuxtv.org/media_tree.git/) since v4l2-of.h has
> been replaced by v4l2-fwnode.h. So this driver will not apply as-is.
>
 
OK.

> Also missing is a patch adding a new entry to the MAINTAINERS file.
> 
> I have some more comments below:
> 
> On 06/27/2017 01:07 PM, Yong Deng wrote:
> > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > and CSI1 is used for parallel interface. This is not documented in
> > datatsheet but by testing and guess.
> 
> datatsheet -> datasheet

OK.

> 
> > 
> > This patch implement a v4l2 framework driver for it.
> > 
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> > 
> > Signed-off-by: Yong Deng <yong.deng@magewell.com>
> > ---
> >   drivers/media/platform/Kconfig                   |   1 +
> >   drivers/media/platform/Makefile                  |   2 +
> >   drivers/media/platform/sunxi-csi/Kconfig         |   8 +
> >   drivers/media/platform/sunxi-csi/Makefile        |   3 +
> >   drivers/media/platform/sunxi-csi/sunxi_csi.c     | 535 +++++++++++++++
> >   drivers/media/platform/sunxi-csi/sunxi_csi.h     | 203 ++++++
> >   drivers/media/platform/sunxi-csi/sunxi_csi_v3s.c | 827 +++++++++++++++++++++++
> >   drivers/media/platform/sunxi-csi/sunxi_csi_v3s.h | 206 ++++++
> >   drivers/media/platform/sunxi-csi/sunxi_video.c   | 667 ++++++++++++++++++
> >   drivers/media/platform/sunxi-csi/sunxi_video.h   |  61 ++
> >   10 files changed, 2513 insertions(+)
> >   create mode 100644 drivers/media/platform/sunxi-csi/Kconfig
> >   create mode 100644 drivers/media/platform/sunxi-csi/Makefile
> >   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi.c
> >   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi.h
> >   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi_v3s.c
> >   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_csi_v3s.h
> >   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_video.c
> >   create mode 100644 drivers/media/platform/sunxi-csi/sunxi_video.h
> 
> <snip>
> 
> > diff --git a/drivers/media/platform/sunxi-csi/Kconfig b/drivers/media/platform/sunxi-csi/Kconfig
> > new file mode 100644
> > index 0000000..f26592a
> > --- /dev/null
> > +++ b/drivers/media/platform/sunxi-csi/Kconfig
> > @@ -0,0 +1,8 @@
> > +config VIDEO_SUNXI_CSI
> > +	tristate "Allwinner Camera Sensor Interface driver"
> > +	depends on VIDEO_V4L2 && COMMON_CLK && VIDEO_V4L2_SUBDEV_API && HAS_DMA
> > +	depends on ARCH_SUNXI
> 
> If possible change this to:
> 
> 	depends on ARCH_SUNXI || COMPILE_TEST

OK.

> 
> to allow this driver to be compiled on e.g. Intel for compile testing.
> 
> > +	select VIDEOBUF2_DMA_CONTIG
> > +	select REGMAP_MMIO
> > +	---help---
> > +	   Support for the Allwinner Camera Sensor Interface Controller.
> 
> This controller is the same for all Allwinner SoC models?

No.
I will change the Kconfig and Makefile.

> 
> <snip>
> 
> > diff --git a/drivers/media/platform/sunxi-csi/sunxi_video.c b/drivers/media/platform/sunxi-csi/sunxi_video.c
> > new file mode 100644
> > index 0000000..57d7563
> > --- /dev/null
> > +++ b/drivers/media/platform/sunxi-csi/sunxi_video.c
> > @@ -0,0 +1,667 @@
> > +/*
> > + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
> > + * All rights reserved.
> > + * Author: Yong Deng <yong.deng@magewell.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/of.h>
> > +
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/v4l2-mc.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +
> > +#include "sunxi_csi.h"
> > +#include "sunxi_video.h"
> > +
> > +struct sunxi_csi_buffer {
> > +	struct vb2_v4l2_buffer		vb;
> > +	struct list_head		list;
> > +
> > +	dma_addr_t			dma_addr;
> > +};
> > +
> > +static struct sunxi_csi_format *
> > +find_format_by_fourcc(struct sunxi_video *video, unsigned int fourcc)
> > +{
> > +	unsigned int num_formats = video->num_formats;
> > +	struct sunxi_csi_format *fmt;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < num_formats; i++) {
> > +		fmt = &video->formats[i];
> > +		if (fmt->fourcc == fourcc)
> > +			return fmt;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static struct v4l2_subdev *
> > +sunxi_video_remote_subdev(struct sunxi_video *video, u32 *pad)
> > +{
> > +	struct media_pad *remote;
> > +
> > +	remote = media_entity_remote_pad(&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 sunxi_video_queue_setup(struct vb2_queue *vq,
> > +				 unsigned int *nbuffers, unsigned int *nplanes,
> > +				 unsigned int sizes[],
> > +				 struct device *alloc_devs[])
> > +{
> > +	struct sunxi_video *video = vb2_get_drv_priv(vq);
> > +	unsigned int size = video->fmt.fmt.pix.sizeimage;
> > +
> > +	if (*nplanes)
> > +		return sizes[0] < size ? -EINVAL : 0;
> > +
> > +	*nplanes = 1;
> > +	sizes[0] = size;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunxi_video_buffer_prepare(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct sunxi_csi_buffer *buf =
> > +			container_of(vbuf, struct sunxi_csi_buffer, vb);
> > +	struct sunxi_video *video = vb2_get_drv_priv(vb->vb2_queue);
> > +	unsigned long size = video->fmt.fmt.pix.sizeimage;
> > +
> > +	if (vb2_plane_size(vb, 0) < size) {
> > +		v4l2_err(video->vdev.v4l2_dev, "buffer too small (%lu < %lu)\n",
> > +			 vb2_plane_size(vb, 0), size);
> > +		return -EINVAL;
> > +	}
> > +
> > +	vb2_set_plane_payload(vb, 0, size);
> > +
> > +	buf->dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
> > +
> > +	vbuf->field = video->fmt.fmt.pix.field;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunxi_pipeline_set_stream(struct sunxi_video *video, bool enable)
> > +{
> > +	struct media_entity *entity;
> > +	struct media_pad *pad;
> > +	struct v4l2_subdev *subdev;
> > +	int ret;
> > +
> > +	entity = &video->vdev.entity;
> > +	while (1) {
> > +		pad = &entity->pads[0];
> > +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > +			break;
> > +
> > +		pad = media_entity_remote_pad(pad);
> > +		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, enable);
> > +		if (enable && ret < 0 && ret != -ENOIOCTLCMD)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunxi_video_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > +	struct sunxi_video *video = vb2_get_drv_priv(vq);
> > +	struct sunxi_csi_buffer *buf;
> > +	struct sunxi_csi_config config;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	video->sequence = 0;
> > +
> > +	ret = media_pipeline_start(&video->vdev.entity, &video->vdev.pipe);
> > +	if (ret < 0)
> > +		goto err_start_pipeline;
> > +
> > +	ret = sunxi_pipeline_set_stream(video, true);
> > +	if (ret < 0)
> > +		goto err_start_stream;
> > +
> > +	config.pixelformat = video->fmt.fmt.pix.pixelformat;
> > +	config.code = video->current_fmt->mbus_code;
> > +	config.field = video->fmt.fmt.pix.field;
> > +	config.width = video->fmt.fmt.pix.width;
> > +	config.height = video->fmt.fmt.pix.height;
> > +
> > +	ret = sunxi_csi_update_config(video->csi, &config);
> > +	if (ret < 0)
> > +		goto err_update_config;
> > +
> > +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > +	video->cur_frm = list_first_entry(&video->dma_queue,
> > +					  struct sunxi_csi_buffer, list);
> > +	list_del(&video->cur_frm->list);
> > +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > +
> > +	ret = sunxi_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> > +	if (ret < 0)
> > +		goto err_update_addr;
> > +
> > +	ret = sunxi_csi_set_stream(video->csi, true);
> > +	if (ret < 0)
> > +		goto err_csi_stream;
> > +
> > +	return 0;
> > +
> > +err_csi_stream:
> > +err_update_addr:
> > +err_update_config:
> > +	sunxi_pipeline_set_stream(video, false);
> > +err_start_stream:
> > +	media_pipeline_stop(&video->vdev.entity);
> > +err_start_pipeline:
> > +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > +	list_for_each_entry(buf, &video->dma_queue, list)
> > +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> > +	INIT_LIST_HEAD(&video->dma_queue);
> > +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > +
> > +	return ret;
> > +}
> > +
> > +static void sunxi_video_stop_streaming(struct vb2_queue *vq)
> > +{
> > +	struct sunxi_video *video = vb2_get_drv_priv(vq);
> > +	unsigned long flags;
> > +	struct sunxi_csi_buffer *buf;
> > +
> > +	sunxi_pipeline_set_stream(video, false);
> > +
> > +	sunxi_csi_set_stream(video->csi, false);
> > +
> > +	media_pipeline_stop(&video->vdev.entity);
> > +
> > +	/* Release all active buffers */
> > +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > +	if (unlikely(video->cur_frm)) {
> > +		vb2_buffer_done(&video->cur_frm->vb.vb2_buf,
> > +				VB2_BUF_STATE_ERROR);
> > +		video->cur_frm = NULL;
> > +	}
> > +	list_for_each_entry(buf, &video->dma_queue, list)
> > +		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> > +	INIT_LIST_HEAD(&video->dma_queue);
> > +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > +}
> > +
> > +static void sunxi_video_buffer_queue(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct sunxi_csi_buffer *buf =
> > +			container_of(vbuf, struct sunxi_csi_buffer, vb);
> > +	struct sunxi_video *video = vb2_get_drv_priv(vb->vb2_queue);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > +	if (!video->cur_frm && list_empty(&video->dma_queue) &&
> > +		vb2_is_streaming(vb->vb2_queue)) {
> > +		video->cur_frm = buf;
> > +		sunxi_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> > +		sunxi_csi_set_stream(video->csi, 1);
> > +	} else
> > +		list_add_tail(&buf->list, &video->dma_queue);
> > +	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > +}
> > +
> > +void sunxi_video_frame_done(struct sunxi_video *video)
> > +{
> > +	spin_lock(&video->dma_queue_lock);
> > +
> > +	if (video->cur_frm) {
> > +		struct vb2_v4l2_buffer *vbuf = &video->cur_frm->vb;
> > +		struct vb2_buffer *vb = &vbuf->vb2_buf;
> > +
> > +		vb->timestamp = ktime_get_ns();
> > +		vbuf->sequence = video->sequence++;
> > +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> > +		video->cur_frm = NULL;
> > +	}
> > +
> > +	if (!list_empty(&video->dma_queue)
> > +	    && vb2_is_streaming(&video->vb2_vidq)) {
> > +		video->cur_frm = list_first_entry(&video->dma_queue,
> > +				struct sunxi_csi_buffer, list);
> > +		list_del(&video->cur_frm->list);
> > +		sunxi_csi_update_buf_addr(video->csi, video->cur_frm->dma_addr);
> > +	} else
> > +		sunxi_csi_set_stream(video->csi, 0);
> > +
> > +	spin_unlock(&video->dma_queue_lock);
> > +}
> > +
> > +static struct vb2_ops sunxi_csi_vb2_ops = {
> > +	.queue_setup		= sunxi_video_queue_setup,
> > +	.wait_prepare		= vb2_ops_wait_prepare,
> > +	.wait_finish		= vb2_ops_wait_finish,
> > +	.buf_prepare		= sunxi_video_buffer_prepare,
> > +	.start_streaming	= sunxi_video_start_streaming,
> > +	.stop_streaming		= sunxi_video_stop_streaming,
> > +	.buf_queue		= sunxi_video_buffer_queue,
> > +};
> > +
> > +static int vidioc_querycap(struct file *file, void *priv,
> > +				struct v4l2_capability *cap)
> > +{
> > +	struct sunxi_video *video = video_drvdata(file);
> > +
> > +	strlcpy(cap->driver, "sunxi-video", sizeof(cap->driver));
> > +	strlcpy(cap->card, video->vdev.name, sizeof(cap->card));
> > +	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > +		 video->csi->dev->of_node->name);
> > +
> > +	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> > +	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> 
> Drop these two lines. You already set device_caps in the video_device struct,
> and the V4L2 core will fill in these two v4l2_capability fields based on that.

OK.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
> > +				   struct v4l2_fmtdesc *f)
> > +{
> > +	struct sunxi_video *video = video_drvdata(file);
> > +	u32 index = f->index;
> > +
> > +	if (index >= video->num_formats)
> > +		return -EINVAL;
> > +
> > +	f->pixelformat = video->formats[index].fourcc;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
> > +				struct v4l2_format *fmt)
> > +{
> > +	struct sunxi_video *video = video_drvdata(file);
> > +
> > +	*fmt = video->fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunxi_video_try_fmt(struct sunxi_video *video, struct v4l2_format *f,
> > +			       struct sunxi_csi_format **current_fmt)
> > +{
> > +	struct sunxi_csi_format *csi_fmt;
> > +	struct v4l2_pix_format *pixfmt = &f->fmt.pix;
> > +	struct v4l2_subdev_format format;
> > +	struct v4l2_subdev *subdev;
> > +	u32 pad;
> > +	int ret;
> > +
> > +	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> > +		return -EINVAL;
> 
> Drop this test. It's not needed.

OK.

> 
> > +
> > +	subdev = sunxi_video_remote_subdev(video, &pad);
> > +	if (subdev == NULL)
> > +		return -ENXIO;
> > +
> > +	csi_fmt = find_format_by_fourcc(video, pixfmt->pixelformat);
> > +	if (csi_fmt == NULL)
> > +		return -EINVAL;
> > +
> > +	format.pad = pad;
> > +	format.which = V4L2_SUBDEV_FORMAT_TRY;
> > +	v4l2_fill_mbus_format(&format.format, pixfmt, csi_fmt->mbus_code);
> > +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format);
> > +	if (ret)
> > +		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
> 
> Just do 'return ret'.

OK.

> 
> > +
> > +	v4l2_fill_pix_format(pixfmt, &format.format);
> > +
> > +	pixfmt->bytesperline = (pixfmt->width * csi_fmt->bpp) >> 3;
> > +	pixfmt->sizeimage = (pixfmt->width * csi_fmt->bpp * pixfmt->height) / 8;
> > +
> > +	if (current_fmt)
> > +		*current_fmt = csi_fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunxi_video_set_fmt(struct sunxi_video *video, struct v4l2_format *f)
> > +{
> > +	struct v4l2_subdev_format format;
> > +	struct sunxi_csi_format *current_fmt;
> > +	struct v4l2_subdev *subdev;
> > +	u32 pad;
> > +	int ret;
> > +
> > +	subdev = sunxi_video_remote_subdev(video, &pad);
> > +	if (subdev == NULL)
> > +		return -ENXIO;
> > +
> > +	ret = sunxi_video_try_fmt(video, f, &current_fmt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	v4l2_fill_mbus_format(&format.format, &f->fmt.pix,
> > +			      current_fmt->mbus_code);
> > +	ret = v4l2_subdev_call(subdev, pad, set_fmt, NULL, &format);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	video->fmt = *f;
> > +	video->current_fmt = current_fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
> > +				struct v4l2_format *f)
> > +{
> > +	struct sunxi_video *video = video_drvdata(file);
> > +
> > +	if (vb2_is_streaming(&video->vb2_vidq))
> > +		return -EBUSY;
> > +
> > +	return sunxi_video_set_fmt(video, f);
> > +}
> > +
> > +static int vidioc_try_fmt_vid_cap(struct file *file, void *priv,
> > +				  struct v4l2_format *f)
> > +{
> > +	struct sunxi_video *video = video_drvdata(file);
> > +
> > +	return sunxi_video_try_fmt(video, f, NULL);
> > +}
> > +
> > +static const struct v4l2_ioctl_ops sunxi_video_ioctl_ops = {
> > +	.vidioc_querycap		= vidioc_querycap,
> > +	.vidioc_enum_fmt_vid_cap	= vidioc_enum_fmt_vid_cap,
> > +	.vidioc_g_fmt_vid_cap		= vidioc_g_fmt_vid_cap,
> > +	.vidioc_s_fmt_vid_cap		= vidioc_s_fmt_vid_cap,
> > +	.vidioc_try_fmt_vid_cap		= vidioc_try_fmt_vid_cap,
> > +
> > +	.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,
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * V4L2 file operations
> > + */
> > +static int sunxi_video_open(struct file *file)
> > +{
> > +	struct sunxi_video *video = video_drvdata(file);
> > +	struct v4l2_format format;
> > +	int ret;
> > +
> > +	if (mutex_lock_interruptible(&video->lock))
> > +		return -ERESTARTSYS;
> > +
> > +	ret = v4l2_fh_open(file);
> > +	if (ret < 0)
> > +		goto unlock;
> > +
> > +	ret = v4l2_pipeline_pm_use(&video->vdev.entity, 1);
> > +	if (ret < 0)
> > +		goto fh_release;
> > +
> > +	if (!v4l2_fh_is_singular_file(file))
> > +		goto unlock;
> > +
> > +	ret = sunxi_csi_set_power(video->csi, true);
> > +	if (ret < 0)
> > +		goto fh_release;
> > +
> > +	/* setup default format */
> > +	if (video->num_formats > 0) {
> > +		format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +		format.fmt.pix.width = 1280;
> > +		format.fmt.pix.height = 720;
> > +		format.fmt.pix.pixelformat = video->formats[0].fourcc;
> > +		sunxi_video_set_fmt(video, &format);
> > +	}
> > +
> > +	mutex_unlock(&video->lock);
> > +	return 0;
> > +
> > +fh_release:
> > +	v4l2_fh_release(file);
> > +unlock:
> > +	mutex_unlock(&video->lock);
> > +	return ret;
> > +}
> > +
> > +static int sunxi_video_close(struct file *file)
> > +{
> > +	struct sunxi_video *video = video_drvdata(file);
> > +
> > +	mutex_lock(&video->lock);
> > +
> > +	_vb2_fop_release(file, NULL);
> > +
> > +	v4l2_pipeline_pm_use(&video->vdev.entity, 0);
> > +
> > +	if (v4l2_fh_is_singular_file(file))
> 
> This is the wrong order. You need to check this before _vb2_fop_release is called.
> 
> E.g.:
> 
> 	bool last_fh = v4l2_fh_is_singular_file(file);
> 
> 	_vb2_fop_release(file, NULL);
> 	...
> 	if (last_fh)
> 		sunxi_csi_set_power(video->csi, false);

OK.

> 
> > +		sunxi_csi_set_power(video->csi, false);
> > +
> > +	mutex_unlock(&video->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_file_operations sunxi_video_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.open		= sunxi_video_open,
> > +	.release	= sunxi_video_close,
> > +	.unlocked_ioctl	= video_ioctl2,
> > +	.read		= vb2_fop_read,
> 
> You haven't signalled read() support in the capabilities or in the vb2 io_modes, so
> either drop this from the video_fops, or add it to the device_caps and io_modes fields.

OK.

> 
> > +	.mmap		= vb2_fop_mmap,
> > +	.poll		= vb2_fop_poll
> > +};
> > +
> > +/* -----------------------------------------------------------------------------
> > + * Media Operations
> > + */
> > +static int sunxi_video_formats_init(struct sunxi_video *video)
> > +{
> > +	struct v4l2_subdev_mbus_code_enum mbus_code = { 0 };
> > +	struct sunxi_csi *csi = video->csi;
> > +	struct v4l2_subdev *subdev;
> > +	u32 pad;
> > +	const u32 *pixformats;
> > +	int pixformat_count = 0;
> > +	u32 subdev_codes[32]; /* subdev format codes, 32 should be enough */
> > +	int codes_count = 0;
> > +	int num_fmts = 0;
> > +	int i, j;
> > +
> > +	subdev = sunxi_video_remote_subdev(video, &pad);
> > +	if (subdev == NULL)
> > +		return -ENXIO;
> > +
> > +	/* Get supported pixformats of CSI */
> > +	pixformat_count = sunxi_csi_get_supported_pixformats(csi, &pixformats);
> > +	if (pixformat_count <= 0)
> > +		return -ENXIO;
> > +
> > +	/* Get subdev formats codes */
> > +	mbus_code.pad = pad;
> > +	mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL,
> > +			&mbus_code)) {
> > +		subdev_codes[codes_count] = mbus_code.code;
> > +		codes_count++;
> > +		mbus_code.index++;
> > +	}
> > +
> > +	if (!codes_count)
> > +		return -ENXIO;
> > +
> > +	/* Get supported formats count */
> > +	for (i = 0; i < codes_count; i++) {
> > +		for (j = 0; j < pixformat_count; j++) {
> > +			if (!sunxi_csi_is_format_support(csi, pixformats[j],
> > +					mbus_code.code)) {
> > +				continue;
> > +			}
> > +			num_fmts++;
> > +		}
> > +	}
> > +
> > +	if (!num_fmts)
> > +		return -ENXIO;
> > +
> > +	video->num_formats = num_fmts;
> > +	video->formats = devm_kcalloc(video->csi->dev, num_fmts,
> > +			sizeof(struct sunxi_csi_format), GFP_KERNEL);
> > +	if (!video->formats) {
> > +		dev_err(video->csi->dev, "could not allocate memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Get supported formats */
> > +	num_fmts = 0;
> > +	for (i = 0; i < codes_count; i++) {
> > +		for (j = 0; j < pixformat_count; j++) {
> > +			if (!sunxi_csi_is_format_support(csi, pixformats[j],
> > +					mbus_code.code)) {
> > +				continue;
> > +			}
> > +
> > +			video->formats[num_fmts].fourcc = pixformats[j];
> > +			video->formats[num_fmts].mbus_code =
> > +					mbus_code.code;
> > +			video->formats[num_fmts].bpp =
> > +					v4l2_pixformat_get_bpp(pixformats[j]);
> > +			num_fmts++;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int sunxi_video_link_setup(struct media_entity *entity,
> > +				  const struct media_pad *local,
> > +				  const struct media_pad *remote, u32 flags)
> > +{
> > +	struct video_device *vdev = media_entity_to_video_device(entity);
> > +	struct sunxi_video *video = video_get_drvdata(vdev);
> > +
> > +	if (WARN_ON(video == NULL))
> > +		return 0;
> > +
> > +	return sunxi_video_formats_init(video);
> > +}
> > +
> > +static const struct media_entity_operations sunxi_video_media_ops = {
> > +	.link_setup = sunxi_video_link_setup,
> > +};
> > +
> > +int sunxi_video_init(struct sunxi_video *video, struct sunxi_csi *csi,
> > +		     const char *name)
> > +{
> > +	struct video_device *vdev = &video->vdev;
> > +	struct vb2_queue *vidq = &video->vb2_vidq;
> > +	int ret;
> > +
> > +	video->csi = csi;
> > +
> > +	/* Initialize the media entity... */
> > +	video->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
> > +	vdev->entity.ops = &sunxi_video_media_ops;
> > +	ret = media_entity_pads_init(&vdev->entity, 1, &video->pad);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mutex_init(&video->lock);
> > +
> > +	INIT_LIST_HEAD(&video->dma_queue);
> > +	spin_lock_init(&video->dma_queue_lock);
> > +
> > +	video->cur_frm = NULL;
> > +	video->sequence = 0;
> > +	video->num_formats = 0;
> > +
> > +	/* Initialize videobuf2 queue */
> > +	vidq->type			= V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > +	vidq->io_modes			= VB2_MMAP | VB2_DMABUF;
> > +	vidq->drv_priv			= video;
> > +	vidq->buf_struct_size		= sizeof(struct sunxi_csi_buffer);
> > +	vidq->ops			= &sunxi_csi_vb2_ops;
> > +	vidq->mem_ops			= &vb2_dma_contig_memops;
> > +	vidq->timestamp_flags		= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > +	vidq->lock			= &video->lock;
> > +	vidq->min_buffers_needed	= 1;
> > +	vidq->dev			= csi->dev;
> > +
> > +	ret = vb2_queue_init(vidq);
> > +	if (ret) {
> > +		v4l2_err(&csi->v4l2_dev, "vb2_queue_init failed: %d\n", ret);
> > +		goto error;
> > +	}
> > +
> > +	/* Register video device */
> > +	strlcpy(vdev->name, name, sizeof(vdev->name));
> > +	vdev->release		= video_device_release_empty;
> > +	vdev->fops		= &sunxi_video_fops;
> > +	vdev->ioctl_ops		= &sunxi_video_ioctl_ops;
> > +	vdev->vfl_type		= VFL_TYPE_GRABBER;
> > +	vdev->vfl_dir		= VFL_DIR_RX;
> > +	vdev->v4l2_dev		= &csi->v4l2_dev;
> > +	vdev->queue		= vidq;
> > +	vdev->lock		= &video->lock;
> > +	vdev->device_caps	= V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_CAPTURE;
> > +	video_set_drvdata(vdev, video);
> > +
> > +	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> > +	if (ret < 0) {
> > +		v4l2_err(&csi->v4l2_dev,
> > +			 "video_register_device failed: %d\n", ret);
> > +		goto error;
> > +	}
> > +
> > +	return 0;
> > +
> > +error:
> > +	sunxi_video_cleanup(video);
> > +	return ret;
> > +}
> > +
> > +void sunxi_video_cleanup(struct sunxi_video *video)
> > +{
> > +	if (video_is_registered(&video->vdev))
> > +		video_unregister_device(&video->vdev);
> > +
> > +	media_entity_cleanup(&video->vdev.entity);
> > +}
> 
> <snip>
> 
> Regards,
> 
> 	Hans

Yong

[toc] | [prev] | [next] | [standalone]

#1679916 — Re: [PATCH RFC 1/2] media: V3s: Add support for Allwinner CSI.

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Mon, Jul 03, 2017 at 06:59:52PM +0800, Yong wrote:
> > > +	select VIDEOBUF2_DMA_CONTIG
> > > +	select REGMAP_MMIO
> > > +	---help---
> > > +	   Support for the Allwinner Camera Sensor Interface Controller.
> > 
> > This controller is the same for all Allwinner SoC models?
> 
> No.
> I will change the Kconfig and Makefile.

This is basically a design that has been introduced in the A31 (sun6i
family). I guess we should just call the driver and Kconfig symbols
sun6i_csi (even though we don't support it yet). It also used on the
A23, A33, A80, A83T, H3, and probably the H5 and A64.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web