| Message ID | 1498561654-14658-3-git-send-email-yong.deng@magewell.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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 -- 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
Hi Yong, I am trying to get this driver working on the Olimex A33 OLinuXino. I didn't get it working yet, but I had some progress. See the comment below on one issue I encountered. On Tue, Jun 27, 2017 at 07:07:34PM +0800, Yong Deng wrote: > 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>; You use platform_get_resource_byname() to get this IO resource. This requires adding mandatory reg-names = "csi"; But is it actually needed? Wouldn't a simple platform_get_resource() be enough? Thanks, baruch > + 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 */ > + }; > + }; > + }; > +
On Tue, 18 Jul 2017 14:55:30 +0300 Baruch Siach <baruch@tkos.co.il> wrote: > Hi Yong, > > I am trying to get this driver working on the Olimex A33 OLinuXino. I didn't > get it working yet, but I had some progress. See the comment below on one > issue I encountered. > > On Tue, Jun 27, 2017 at 07:07:34PM +0800, Yong Deng wrote: > > 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>; > > You use platform_get_resource_byname() to get this IO resource. This requires > adding mandatory > > reg-names = "csi"; > > But is it actually needed? Wouldn't a simple platform_get_resource() be > enough? You are right. This will be fixed in the next version. I am waiting for more comments for the sunxi-csi.h. It's pleasure if you have any suggestions about it. -- 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
Hi Yong, On Wed, Jul 19, 2017 at 09:22:49AM +0800, Yong wrote: > On Tue, 18 Jul 2017 14:55:30 +0300 > Baruch Siach <baruch@tkos.co.il> wrote: > > I am trying to get this driver working on the Olimex A33 OLinuXino. I > > didn't get it working yet, but I had some progress. See the comment below > > on one issue I encountered. > > > > On Tue, Jun 27, 2017 at 07:07:34PM +0800, Yong Deng wrote: > > > Add binding documentation for Allwinner CSI. > > > > > > Signed-off-by: Yong Deng <yong.deng@magewell.com> > > > --- [...] > > > +Example: > > > + > > > + csi1: csi@01cb4000 { > > > + compatible = "allwinner,sun8i-v3s-csi"; > > > + reg = <0x01cb4000 0x1000>; > > > > You use platform_get_resource_byname() to get this IO resource. This requires > > adding mandatory > > > > reg-names = "csi"; > > > > But is it actually needed? Wouldn't a simple platform_get_resource() be > > enough? > > You are right. > This will be fixed in the next version. > I am waiting for more comments for the sunxi-csi.h. It's pleasure if > you have any suggestions about it. You mean sunxi_csi.h, right? Why do you need the sunxi_csi_ops indirection? Do you expect to add alternative implementations of these ops at some point? baruch
Hi Baruch, On Wed, 19 Jul 2017 07:49:23 +0300 Baruch Siach <baruch@tkos.co.il> wrote: > Hi Yong, > > On Wed, Jul 19, 2017 at 09:22:49AM +0800, Yong wrote: > > On Tue, 18 Jul 2017 14:55:30 +0300 > > Baruch Siach <baruch@tkos.co.il> wrote: > > > I am trying to get this driver working on the Olimex A33 OLinuXino. I > > > didn't get it working yet, but I had some progress. See the comment below > > > on one issue I encountered. > > > > > > On Tue, Jun 27, 2017 at 07:07:34PM +0800, Yong Deng wrote: > > > > Add binding documentation for Allwinner CSI. > > > > > > > > Signed-off-by: Yong Deng <yong.deng@magewell.com> > > > > --- > > [...] > > > > > +Example: > > > > + > > > > + csi1: csi@01cb4000 { > > > > + compatible = "allwinner,sun8i-v3s-csi"; > > > > + reg = <0x01cb4000 0x1000>; > > > > > > You use platform_get_resource_byname() to get this IO resource. This requires > > > adding mandatory > > > > > > reg-names = "csi"; > > > > > > But is it actually needed? Wouldn't a simple platform_get_resource() be > > > enough? > > > > You are right. > > This will be fixed in the next version. > > I am waiting for more comments for the sunxi-csi.h. It's pleasure if > > you have any suggestions about it. > > You mean sunxi_csi.h, right? Yes. My spelling mistake. > > Why do you need the sunxi_csi_ops indirection? Do you expect to add > alternative implementations of these ops at some point? I want to seperate the sunxi_video.c and sunxi_csi_v3s.c. sunxi_csi_v3s.c is Soc specific. Maybe there will be sunxi_csi_r40.c in the futrue. But the sunxi_video.c and sunxi_csi.c are common. > > baruch > > -- > http://baruch.siach.name/blog/ ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - Thanks, Yong -- 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
Hi Yong, On Wed, Jul 19, 2017 at 02:21:20PM +0800, Yong wrote: > On Wed, 19 Jul 2017 07:49:23 +0300 > Baruch Siach <baruch@tkos.co.il> wrote: > > On Wed, Jul 19, 2017 at 09:22:49AM +0800, Yong wrote: > > > I am waiting for more comments for the sunxi-csi.h. It's pleasure if > > > you have any suggestions about it. > > > > You mean sunxi_csi.h, right? > > Yes. My spelling mistake. > > > Why do you need the sunxi_csi_ops indirection? Do you expect to add > > alternative implementations of these ops at some point? > > I want to seperate the sunxi_video.c and sunxi_csi_v3s.c. > sunxi_csi_v3s.c is Soc specific. Maybe there will be sunxi_csi_r40.c > in the futrue. But the sunxi_video.c and sunxi_csi.c are common. I'd say it is a premature optimization. The file separation is fine, IMO, but the added csi_ops indirection makes the code less readable. Someone with access to R40 hardware with CSI setup would be a better position to abstract the platform specific code. But I'd defer to the media maintainers on that. Thanks, baruch
On Wed, Jul 19, 2017 at 09:33:49AM +0300, Baruch Siach wrote: > Hi Yong, > > On Wed, Jul 19, 2017 at 02:21:20PM +0800, Yong wrote: > > On Wed, 19 Jul 2017 07:49:23 +0300 > > Baruch Siach <baruch@tkos.co.il> wrote: > > > On Wed, Jul 19, 2017 at 09:22:49AM +0800, Yong wrote: > > > > I am waiting for more comments for the sunxi-csi.h. It's pleasure if > > > > you have any suggestions about it. > > > > > > You mean sunxi_csi.h, right? > > > > Yes. My spelling mistake. > > > > > Why do you need the sunxi_csi_ops indirection? Do you expect to add > > > alternative implementations of these ops at some point? > > > > I want to seperate the sunxi_video.c and sunxi_csi_v3s.c. > > sunxi_csi_v3s.c is Soc specific. Maybe there will be sunxi_csi_r40.c > > in the futrue. But the sunxi_video.c and sunxi_csi.c are common. > > I'd say it is a premature optimization. The file separation is fine, IMO, but > the added csi_ops indirection makes the code less readable. Someone with > access to R40 hardware with CSI setup would be a better position to abstract > the platform specific code. I agree Maxime
On Wed, 19 Jul 2017 08:50:19 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, Jul 19, 2017 at 09:33:49AM +0300, Baruch Siach wrote: > > Hi Yong, > > > > On Wed, Jul 19, 2017 at 02:21:20PM +0800, Yong wrote: > > > On Wed, 19 Jul 2017 07:49:23 +0300 > > > Baruch Siach <baruch@tkos.co.il> wrote: > > > > On Wed, Jul 19, 2017 at 09:22:49AM +0800, Yong wrote: > > > > > I am waiting for more comments for the sunxi-csi.h. It's pleasure if > > > > > you have any suggestions about it. > > > > > > > > You mean sunxi_csi.h, right? > > > > > > Yes. My spelling mistake. > > > > > > > Why do you need the sunxi_csi_ops indirection? Do you expect to add > > > > alternative implementations of these ops at some point? > > > > > > I want to seperate the sunxi_video.c and sunxi_csi_v3s.c. > > > sunxi_csi_v3s.c is Soc specific. Maybe there will be sunxi_csi_r40.c > > > in the futrue. But the sunxi_video.c and sunxi_csi.c are common. > > > > I'd say it is a premature optimization. The file separation is fine, IMO, but > > the added csi_ops indirection makes the code less readable. Someone with > > access to R40 hardware with CSI setup would be a better position to abstract > > the platform specific code. > > I agree Well, I made things complicated. So, the initial version is just to make V3s CSI working. Beside csi_ops, are there some comments for sunxi_csi_v3s.c? I will send a new version in the next few days. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Thanks, Yong -- 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
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 */ + }; + }; + }; +
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