[RFC,2/2] dt-bindings: add binding documentation for Allwinner CSI

Commit Message

yong June 27, 2017, 11:07 a.m. UTC

Add binding documentation for Allwinner CSI.

Signed-off-by: Yong Deng <[e-mail]>
---
 .../devicetree/bindings/media/sunxi-csi.txt        | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/sunxi-csi.txt

Comments

Rob Herring June 29, 2017, 9:19 p.m. UTC | #1

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 <[e-mail]>
> ---
>  .../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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chen-Yu Tsai June 30, 2017, 3:41 a.m. UTC | #2

On Fri, Jun 30, 2017 at 5:19 AM, Rob Herring <[e-mail]> 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 <[e-mail]>
>> ---
>>  .../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 [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

yong June 30, 2017, 7:55 a.m. UTC | #3

On Fri, 30 Jun 2017 11:41:50 +0800
Chen-Yu Tsai <[e-mail]> wrote:

> On Fri, Jun 30, 2017 at 5:19 AM, Rob Herring <[e-mail]> 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 <[e-mail]>
> >> ---
> >>  .../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 [e-mail]
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring June 30, 2017, 4:05 p.m. UTC | #4

On Thu, Jun 29, 2017 at 10:41 PM, Chen-Yu Tsai <[e-mail]> wrote:
> On Fri, Jun 30, 2017 at 5:19 AM, Rob Herring <[e-mail]> 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 <[e-mail]>
>>> ---
>>>  .../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

Baruch Siach July 18, 2017, 11:55 a.m. UTC | #5

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 <[e-mail]>
> ---
>  .../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 */
> +			};
> +		};
> +	};
> +

yong July 19, 2017, 1:22 a.m. UTC | #6

On Tue, 18 Jul 2017 14:55:30 +0300
Baruch Siach <[e-mail]> 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 <[e-mail]>
> > ---
> >  .../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.

Baruch Siach July 19, 2017, 4:49 a.m. UTC | #7

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 <[e-mail]> 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 <[e-mail]>
> > > ---

[...]

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

yong July 19, 2017, 6:21 a.m. UTC | #8

Hi Baruch,

On Wed, 19 Jul 2017 07:49:23 +0300
Baruch Siach <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > > ---
> 
> [...]
> 
> > > > +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------------{=
>    - [e-mail] - tel: [phone], http://www.tkos.co.il -


Thanks,
Yong

Baruch Siach July 19, 2017, 6:33 a.m. UTC | #9

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 <[e-mail]> 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

Maxime Ripard July 19, 2017, 6:50 a.m. UTC | #10

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 <[e-mail]> 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

yong July 19, 2017, 7 a.m. UTC | #11

On Wed, 19 Jul 2017 08:50:19 +0200
Maxime Ripard <[e-mail]> 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 <[e-mail]> 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

Patch

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 */
+			};
+		};
+	};
+