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

Groups > linux.kernel > #1690198 > unrolled thread

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

Started byBaruch Siach <baruch@tkos.co.il>
First post2017-07-18 14:00 +0200
Last post2017-07-19 09:10 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Baruch Siach <baruch@tkos.co.il> - 2017-07-18 14:00 +0200
    Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Yong <yong.deng@magewell.com> - 2017-07-19 03:30 +0200
      Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Baruch Siach <baruch@tkos.co.il> - 2017-07-19 06:50 +0200
        Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Yong <yong.deng@magewell.com> - 2017-07-19 08:30 +0200
          Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Baruch Siach <baruch@tkos.co.il> - 2017-07-19 08:40 +0200
            Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-07-19 09:00 +0200
              Re: [PATCH RFC 2/2] dt-bindings: add binding documentation for  Allwinner CSI Yong <yong.deng@magewell.com> - 2017-07-19 09:10 +0200

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

FromBaruch Siach <baruch@tkos.co.il>
Date2017-07-18 14:00 +0200
SubjectRe: [PATCH RFC 2/2] dt-bindings: add binding documentation for Allwinner CSI
Message-ID<u4zku-1WQ-3@gated-at.bofh.it>
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 */
> +			};
> +		};
> +	};
> +

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[toc] | [next] | [standalone]

#1690875

FromYong <yong.deng@magewell.com>
Date2017-07-19 03:30 +0200
Message-ID<u4LYl-1wx-7@gated-at.bofh.it>
In reply to#1690198
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.

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

#1690963

FromBaruch Siach <baruch@tkos.co.il>
Date2017-07-19 06:50 +0200
Message-ID<u4P5T-3vt-1@gated-at.bofh.it>
In reply to#1690875
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

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

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

#1691015

FromYong <yong.deng@magewell.com>
Date2017-07-19 08:30 +0200
Message-ID<u4QEG-4FE-21@gated-at.bofh.it>
In reply to#1690963
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

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

#1691022

FromBaruch Siach <baruch@tkos.co.il>
Date2017-07-19 08:40 +0200
Message-ID<u4QOm-4Jj-19@gated-at.bofh.it>
In reply to#1691015
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

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

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

#1691036

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2017-07-19 09:00 +0200
Message-ID<u4R7J-4R3-9@gated-at.bofh.it>
In reply to#1691022

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

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

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

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

#1691038

FromYong <yong.deng@magewell.com>
Date2017-07-19 09:10 +0200
Message-ID<u4Rhn-5as-1@gated-at.bofh.it>
In reply to#1691036
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web