| Message ID | 1510743363-25798-2-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Jacopo, A couple of minor language fixups inline. On 15/11/17 10:55, Jacopo Mondi wrote: > Add bindings documentation for Renesas Capture Engine Unit (CEU). > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > .../devicetree/bindings/media/renesas,ceu.txt | 87 ++++++++++++++++++++++ > 1 file changed, 87 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/renesas,ceu.txt > > diff --git a/Documentation/devicetree/bindings/media/renesas,ceu.txt b/Documentation/devicetree/bindings/media/renesas,ceu.txt > new file mode 100644 > index 0000000..a88e9cb > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt > @@ -0,0 +1,87 @@ > +Renesas Capture Engine Unit (CEU) > +---------------------------------------------- > + > +The Capture Engine Unit is the image capture interface found on Renesas > +RZ chip series and on SH Mobile ones. > + > +The interface supports a single parallel input with up 8/16bits data bus width. s/with up 8/16bits/with either 8 or 16 bits/ ? > + > +Required properties: > +- compatible > + Must be "renesas,renesas-ceu". > +- reg > + Physical address base and size. > +- interrupts > + The interrupt line number. > +- pinctrl-names, pinctrl-0 > + phandle of pin controller sub-node configuring pins for CEU operations. > + > +CEU supports a single parallel input and should contain a single 'port' subnode > +with a single 'endpoint'. Optional endpoint properties applicable to parallel > +input bus are described in "video-interfaces.txt". > + > +Example: > + > +The example describes the connection between the Capture Engine Unit and a s/and a/and an/ > +OV7670 image sensor sitting on bus i2c1 with an on-board 24Mhz clock. > + > +ceu: ceu@e8210000 { > + reg = <0xe8210000 0x209c>; > + compatible = "renesas,renesas-ceu"; > + interrupts = <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vio_pins>; > + > + status = "okay"; > + > + port { > + ceu_in: endpoint { > + remote-endpoint = <&ov7670_out>; > + > + bus-width = <8>; > + hsync-active = <1>; > + vsync-active = <1>; > + pclk-sample = <1>; > + data-active = <1>; > + }; > + }; > +}; > + > +i2c1: i2c@fcfee400 { > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c1_pins>; > + > + status = "okay"; > + clock-frequency = <100000>; > + > + ov7670: camera@21 { > + compatible = "ovti,ov7670"; > + reg = <0x21>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&vio_pins>; > + > + reset-gpios = <&port3 11 GPIO_ACTIVE_LOW>; > + powerdown-gpios = <&port3 12 GPIO_ACTIVE_HIGH>; > + > + clocks = <&xclk>; > + clock-names = "xclk"; > + > + xclk: fixed_clk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <24000000>; > + }; > + > + port { > + ov7670_out: endpoint { > + remote-endpoint = <&ceu_in>; > + > + bus-width = <8>; > + hsync-active = <1>; > + vsync-active = <1>; > + pclk-sample = <1>; > + data-active = <1>; > + }; > + }; > + }; >
Hi Jacopo, Thanks for the patchset. Please see my comments below. On Wed, Nov 15, 2017 at 11:55:54AM +0100, Jacopo Mondi wrote: > Add bindings documentation for Renesas Capture Engine Unit (CEU). > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > .../devicetree/bindings/media/renesas,ceu.txt | 87 ++++++++++++++++++++++ > 1 file changed, 87 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/renesas,ceu.txt > > diff --git a/Documentation/devicetree/bindings/media/renesas,ceu.txt b/Documentation/devicetree/bindings/media/renesas,ceu.txt > new file mode 100644 > index 0000000..a88e9cb > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt > @@ -0,0 +1,87 @@ > +Renesas Capture Engine Unit (CEU) > +---------------------------------------------- > + > +The Capture Engine Unit is the image capture interface found on Renesas > +RZ chip series and on SH Mobile ones. > + > +The interface supports a single parallel input with up 8/16bits data bus width. > + > +Required properties: > +- compatible > + Must be "renesas,renesas-ceu". > +- reg > + Physical address base and size. > +- interrupts > + The interrupt line number. > +- pinctrl-names, pinctrl-0 > + phandle of pin controller sub-node configuring pins for CEU operations. > + > +CEU supports a single parallel input and should contain a single 'port' subnode > +with a single 'endpoint'. Optional endpoint properties applicable to parallel > +input bus are described in "video-interfaces.txt". Could you list which ones they are? For someone not familiar with the parallel bus this might not be obvious; also not all hardware can make use of every one of these properties. > + > +Example: > + > +The example describes the connection between the Capture Engine Unit and a > +OV7670 image sensor sitting on bus i2c1 with an on-board 24Mhz clock. > + > +ceu: ceu@e8210000 { > + reg = <0xe8210000 0x209c>; > + compatible = "renesas,renesas-ceu"; > + interrupts = <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vio_pins>; > + > + status = "okay"; > + > + port { > + ceu_in: endpoint { > + remote-endpoint = <&ov7670_out>; > + > + bus-width = <8>; > + hsync-active = <1>; > + vsync-active = <1>; > + pclk-sample = <1>; > + data-active = <1>; > + }; > + }; > +}; > + > +i2c1: i2c@fcfee400 { > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c1_pins>; > + > + status = "okay"; > + clock-frequency = <100000>; > + > + ov7670: camera@21 { > + compatible = "ovti,ov7670"; > + reg = <0x21>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&vio_pins>; > + > + reset-gpios = <&port3 11 GPIO_ACTIVE_LOW>; > + powerdown-gpios = <&port3 12 GPIO_ACTIVE_HIGH>; > + > + clocks = <&xclk>; > + clock-names = "xclk"; > + > + xclk: fixed_clk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <24000000>; > + }; What's the purpose of the fixed_clk node here? > + > + port { > + ov7670_out: endpoint { > + remote-endpoint = <&ceu_in>; > + > + bus-width = <8>; > + hsync-active = <1>; > + vsync-active = <1>; > + pclk-sample = <1>; > + data-active = <1>; > + }; > + }; > + };
Hi Jacopo, CC devicetree folks On Wed, Nov 15, 2017 at 11:55 AM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > Add bindings documentation for Renesas Capture Engine Unit (CEU). > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > .../devicetree/bindings/media/renesas,ceu.txt | 87 ++++++++++++++++++++++ > 1 file changed, 87 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/renesas,ceu.txt > > diff --git a/Documentation/devicetree/bindings/media/renesas,ceu.txt b/Documentation/devicetree/bindings/media/renesas,ceu.txt > new file mode 100644 > index 0000000..a88e9cb > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt > @@ -0,0 +1,87 @@ > +Renesas Capture Engine Unit (CEU) > +---------------------------------------------- > + > +The Capture Engine Unit is the image capture interface found on Renesas > +RZ chip series and on SH Mobile ones. > + > +The interface supports a single parallel input with up 8/16bits data bus width. ... with data bus widths up to 8/16 bits? > + > +Required properties: > +- compatible > + Must be "renesas,renesas-ceu". The double "renesas" part looks odd to me. What about "renesas,ceu"? Shouldn't you add SoC-specific compatible values like "renesas,r7s72100-ceu", too? > +- reg > + Physical address base and size. > +- interrupts > + The interrupt line number. interrupt specifier > +- pinctrl-names, pinctrl-0 > + phandle of pin controller sub-node configuring pins for CEU operations. > + > +CEU supports a single parallel input and should contain a single 'port' subnode > +with a single 'endpoint'. Optional endpoint properties applicable to parallel > +input bus are described in "video-interfaces.txt". > + > +Example: > + > +The example describes the connection between the Capture Engine Unit and a ... an > +OV7670 image sensor sitting on bus i2c1 with an on-board 24Mhz clock. > + > +ceu: ceu@e8210000 { > + reg = <0xe8210000 0x209c>; > + compatible = "renesas,renesas-ceu"; > + interrupts = <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vio_pins>; > + > + status = "okay"; > + > + port { > + ceu_in: endpoint { > + remote-endpoint = <&ov7670_out>; > + > + bus-width = <8>; > + hsync-active = <1>; > + vsync-active = <1>; > + pclk-sample = <1>; > + data-active = <1>; > + }; > + }; > +}; > + > +i2c1: i2c@fcfee400 { > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c1_pins>; > + > + status = "okay"; > + clock-frequency = <100000>; > + > + ov7670: camera@21 { > + compatible = "ovti,ov7670"; > + reg = <0x21>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&vio_pins>; > + > + reset-gpios = <&port3 11 GPIO_ACTIVE_LOW>; > + powerdown-gpios = <&port3 12 GPIO_ACTIVE_HIGH>; > + > + clocks = <&xclk>; > + clock-names = "xclk"; > + > + xclk: fixed_clk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <24000000>; > + }; > + > + port { > + ov7670_out: endpoint { > + remote-endpoint = <&ceu_in>; > + > + bus-width = <8>; > + hsync-active = <1>; > + vsync-active = <1>; > + pclk-sample = <1>; > + data-active = <1>; > + }; > + }; > + }; > -- > 2.7.4 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Wed, Nov 15, 2017 at 02:07:31PM +0100, Geert Uytterhoeven wrote: > Hi Jacopo, > > CC devicetree folks Yeah, sorry I forgot them. Sorry about this and thanks for adding the address back! > > On Wed, Nov 15, 2017 at 11:55 AM, Jacopo Mondi > <jacopo+renesas@jmondi.org> wrote: > > Add bindings documentation for Renesas Capture Engine Unit (CEU). > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > .../devicetree/bindings/media/renesas,ceu.txt | 87 ++++++++++++++++++++++ > > 1 file changed, 87 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/renesas,ceu.txt > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,ceu.txt b/Documentation/devicetree/bindings/media/renesas,ceu.txt > > new file mode 100644 > > index 0000000..a88e9cb > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt > > @@ -0,0 +1,87 @@ > > +Renesas Capture Engine Unit (CEU) > > +---------------------------------------------- > > + > > +The Capture Engine Unit is the image capture interface found on Renesas > > +RZ chip series and on SH Mobile ones. > > + > > +The interface supports a single parallel input with up 8/16bits data bus width. > > ... with data bus widths up to 8/16 bits? > > > + > > +Required properties: > > +- compatible > > + Must be "renesas,renesas-ceu". > > The double "renesas" part looks odd to me. What about "renesas,ceu"? I'm totally open for better "compatible" strings here, so yeah, let's got for the shorter one you proposed... > Shouldn't you add SoC-specific compatible values like "renesas,r7s72100-ceu", > too? Well, I actually have no SoC-specific data in the driver, so I don't need SoC specific "compatible" values. But if it's a good practice to have them anyway, I will add those in next spin.. > > > +- reg > > + Physical address base and size. > > +- interrupts > > + The interrupt line number. > > interrupt specifier Yeah, it's not just the line number... > [snip] > > +i2c1: i2c@fcfee400 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&i2c1_pins>; > > + > > + status = "okay"; > > + clock-frequency = <100000>; > > + > > + ov7670: camera@21 { > > + compatible = "ovti,ov7670"; > > + reg = <0x21>; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&vio_pins>; > > + > > + reset-gpios = <&port3 11 GPIO_ACTIVE_LOW>; > > + powerdown-gpios = <&port3 12 GPIO_ACTIVE_HIGH>; > > + > > + clocks = <&xclk>; > > + clock-names = "xclk"; > > + > > + xclk: fixed_clk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <24000000>; > > + }; As Sakari pointed out in his review, this fixed clock is a detail specific to the sensor used in the example (ov7670). For sake of simplicity I can remove it. Thanks j
Hi Jacopo, On Wed, Nov 15, 2017 at 7:15 PM, jacopo mondi <jacopo@jmondi.org> wrote: > On Wed, Nov 15, 2017 at 02:07:31PM +0100, Geert Uytterhoeven wrote: >> On Wed, Nov 15, 2017 at 11:55 AM, Jacopo Mondi >> <jacopo+renesas@jmondi.org> wrote: >> > --- /dev/null >> > +++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt >> > @@ -0,0 +1,87 @@ >> > +Renesas Capture Engine Unit (CEU) >> > +---------------------------------------------- >> > + >> > +The Capture Engine Unit is the image capture interface found on Renesas >> > +RZ chip series and on SH Mobile ones. >> > + >> > +The interface supports a single parallel input with up 8/16bits data bus width. >> >> ... with data bus widths up to 8/16 bits? >> >> > + >> > +Required properties: >> > +- compatible >> > + Must be "renesas,renesas-ceu". >> >> The double "renesas" part looks odd to me. What about "renesas,ceu"? > > I'm totally open for better "compatible" strings here, so yeah, let's > got for the shorter one you proposed... > >> Shouldn't you add SoC-specific compatible values like "renesas,r7s72100-ceu", >> too? > > Well, I actually have no SoC-specific data in the driver, so I don't > need SoC specific "compatible" values. But if it's a good practice > to have them anyway, I will add those in next spin.. You don't necessarily need them in the driver, but in the bindings and DTS, just in case a difference is discovered later. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hello, On Wednesday, 15 November 2017 14:33:12 EET Sakari Ailus wrote: > On Wed, Nov 15, 2017 at 11:55:54AM +0100, Jacopo Mondi wrote: > > Add bindings documentation for Renesas Capture Engine Unit (CEU). > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > > > .../devicetree/bindings/media/renesas,ceu.txt | 87 +++++++++++++++++ > > 1 file changed, 87 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/media/renesas,ceu.txt > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,ceu.txt > > b/Documentation/devicetree/bindings/media/renesas,ceu.txt new file mode > > 100644 > > index 0000000..a88e9cb > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt > > @@ -0,0 +1,87 @@ > > +Renesas Capture Engine Unit (CEU) > > +---------------------------------------------- > > + > > +The Capture Engine Unit is the image capture interface found on Renesas > > +RZ chip series and on SH Mobile ones. > > + > > +The interface supports a single parallel input with up 8/16bits data bus > > width. > > + > > +Required properties: > > +- compatible > > + Must be "renesas,renesas-ceu". > > +- reg > > + Physical address base and size. > > +- interrupts > > + The interrupt line number. > > +- pinctrl-names, pinctrl-0 > > + phandle of pin controller sub-node configuring pins for CEU operations. > > + > > +CEU supports a single parallel input and should contain a single 'port' > > subnode > > +with a single 'endpoint'. Optional endpoint properties applicable to > > parallel > > +input bus are described in "video-interfaces.txt". > > Could you list which ones they are? For someone not familiar with the > parallel bus this might not be obvious; also not all hardware can make use > of every one of these properties. Agreed, you should list the properties here and reference video-interfaces.txt for the detailed description. > > + > > +Example: > > + > > +The example describes the connection between the Capture Engine Unit and > > a > > +OV7670 image sensor sitting on bus i2c1 with an on-board 24Mhz clock. > > + > > +ceu: ceu@e8210000 { > > + reg = <0xe8210000 0x209c>; > > + compatible = "renesas,renesas-ceu"; > > + interrupts = <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&vio_pins>; > > + > > + status = "okay"; > > + > > + port { > > + ceu_in: endpoint { > > + remote-endpoint = <&ov7670_out>; > > + > > + bus-width = <8>; > > + hsync-active = <1>; > > + vsync-active = <1>; > > + pclk-sample = <1>; > > + data-active = <1>; > > + }; > > + }; > > +}; > > + > > +i2c1: i2c@fcfee400 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&i2c1_pins>; > > + > > + status = "okay"; > > + clock-frequency = <100000>; > > + > > + ov7670: camera@21 { > > + compatible = "ovti,ov7670"; > > + reg = <0x21>; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&vio_pins>; > > + > > + reset-gpios = <&port3 11 GPIO_ACTIVE_LOW>; > > + powerdown-gpios = <&port3 12 GPIO_ACTIVE_HIGH>; > > + > > + clocks = <&xclk>; > > + clock-names = "xclk"; > > + > > + xclk: fixed_clk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <24000000>; > > + }; > > What's the purpose of the fixed_clk node here? The sensor is clocked by a 24MHz oscillator. The clock isn't provided by the sensor, so it should be located at the root of the device tree, not as a child of the sensor DT node. > > + > > + port { > > + ov7670_out: endpoint { > > + remote-endpoint = <&ceu_in>; > > + > > + bus-width = <8>; > > + hsync-active = <1>; > > + vsync-active = <1>; > > + pclk-sample = <1>; > > + data-active = <1>; > > + }; > > + }; > > + };
diff --git a/Documentation/devicetree/bindings/media/renesas,ceu.txt b/Documentation/devicetree/bindings/media/renesas,ceu.txt new file mode 100644 index 0000000..a88e9cb --- /dev/null +++ b/Documentation/devicetree/bindings/media/renesas,ceu.txt @@ -0,0 +1,87 @@ +Renesas Capture Engine Unit (CEU) +---------------------------------------------- + +The Capture Engine Unit is the image capture interface found on Renesas +RZ chip series and on SH Mobile ones. + +The interface supports a single parallel input with up 8/16bits data bus width. + +Required properties: +- compatible + Must be "renesas,renesas-ceu". +- reg + Physical address base and size. +- interrupts + The interrupt line number. +- pinctrl-names, pinctrl-0 + phandle of pin controller sub-node configuring pins for CEU operations. + +CEU supports a single parallel input and should contain a single 'port' subnode +with a single 'endpoint'. Optional endpoint properties applicable to parallel +input bus are described in "video-interfaces.txt". + +Example: + +The example describes the connection between the Capture Engine Unit and a +OV7670 image sensor sitting on bus i2c1 with an on-board 24Mhz clock. + +ceu: ceu@e8210000 { + reg = <0xe8210000 0x209c>; + compatible = "renesas,renesas-ceu"; + interrupts = <GIC_SPI 332 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&vio_pins>; + + status = "okay"; + + port { + ceu_in: endpoint { + remote-endpoint = <&ov7670_out>; + + bus-width = <8>; + hsync-active = <1>; + vsync-active = <1>; + pclk-sample = <1>; + data-active = <1>; + }; + }; +}; + +i2c1: i2c@fcfee400 { + pinctrl-names = "default"; + pinctrl-0 = <&i2c1_pins>; + + status = "okay"; + clock-frequency = <100000>; + + ov7670: camera@21 { + compatible = "ovti,ov7670"; + reg = <0x21>; + + pinctrl-names = "default"; + pinctrl-0 = <&vio_pins>; + + reset-gpios = <&port3 11 GPIO_ACTIVE_LOW>; + powerdown-gpios = <&port3 12 GPIO_ACTIVE_HIGH>; + + clocks = <&xclk>; + clock-names = "xclk"; + + xclk: fixed_clk { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <24000000>; + }; + + port { + ov7670_out: endpoint { + remote-endpoint = <&ceu_in>; + + bus-width = <8>; + hsync-active = <1>; + vsync-active = <1>; + pclk-sample = <1>; + data-active = <1>; + }; + }; + };
Add bindings documentation for Renesas Capture Engine Unit (CEU). Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- .../devicetree/bindings/media/renesas,ceu.txt | 87 ++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/renesas,ceu.txt