[PATCH v3 0/2] [media] atmel-isc: add driver for Atmel ISC

[PATCH v3 0/2] [media] atmel-isc: add driver for Atmel ISC

[Songjun Wu]

The Image Sensor Controller driver includes two parts.
1) Driver code to implement the ISC function.
2) Device tree binding documentation, it describes how
   to add the ISC in device tree.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.

Changes in v2:
- Add "depends on COMMON_CLK" and "VIDEO_V4L2_SUBDEV_API"
  in Kconfig file.
- Correct typos and coding style according to Laurent's remarks.
- Delete the loop while in 'isc_clk_enable' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
  with 'pfe_cfg0' in struct isc_subdev_entity.
- Add the code to support VIDIOC_CREATE_BUFS in
  'isc_queue_setup' function.
- Invoke isc_config to configure register in
  'isc_start_streaming' function.
- Add the struct completion 'comp' to synchronize with
  the frame end interrupt in 'isc_stop_streaming' function.
- Check the return value of the clk_prepare_enable
  in 'isc_open' function.
- Set the default format in 'isc_open' function.
- Add an exit condition in the loop while in 'isc_config'.
- Delete the hardware setup operation in 'isc_set_format'.
- Refuse format modification during streaming
  in 'isc_s_fmt_vid_cap' function.
- Invoke v4l2_subdev_alloc_pad_config to allocate and
  initialize the pad config in 'isc_async_complete' function.
- Remove the '.owner  = THIS_MODULE,' in atmel_isc_driver.
- Replace the module_platform_driver_probe() with
  module_platform_driver().
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.

Songjun Wu (2):
  [media] atmel-isc: add the Image Sensor Controller code
  [media] atmel-isc: DT binding for Image Sensor Controller driver

 .../devicetree/bindings/media/atmel-isc.txt        |   88 ++
 drivers/media/platform/Kconfig                     |    1 +
 drivers/media/platform/Makefile                    |    2 +
 drivers/media/platform/atmel/Kconfig               |    9 +
 drivers/media/platform/atmel/Makefile              |    1 +
 drivers/media/platform/atmel/atmel-isc-regs.h      |  276 ++++
 drivers/media/platform/atmel/atmel-isc.c           | 1610 ++++++++++++++++++++
 7 files changed, 1987 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
 create mode 100644 drivers/media/platform/atmel/Kconfig
 create mode 100644 drivers/media/platform/atmel/Makefile
 create mode 100644 drivers/media/platform/atmel/atmel-isc-regs.h
 create mode 100644 drivers/media/platform/atmel/atmel-isc.c

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver

[Songjun Wu]

DT binding documentation for ISC driver.

Signed-off-by: Songjun Wu <songjun.wu@atmel.com>
---

Changes in v3:
- Remove the 'atmel,sensor-preferred'.
- Modify the isc clock node according to the Rob's remarks.

Changes in v2:
- Remove the unit address of the endpoint.
- Add the unit address to the clock node.
- Avoid using underscores in node names.
- Drop the "0x" in the unit address of the i2c node.
- Modify the description of 'atmel,sensor-preferred'.
- Add the description for the ISC internal clock.

 .../devicetree/bindings/media/atmel-isc.txt        | 88 ++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt

diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
new file mode 100644
index 0000000..2ae1d60
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,88 @@
+Atmel Image Sensor Controller (ISC)
+----------------------------------------------
+
+Required properties for ISC:
+- compatible
+	Must be "atmel,sama5d2-isc"
+- reg
+	Physical base address and length of the registers set for the device;
+- interrupts
+	Should contain IRQ line for the ISI;
+- clocks
+	List of clock specifiers, corresponding to entries in
+	the clock-names property;
+	Please refer to clock-bindings.txt.
+- clock-names
+	Required elements: "hclock", "ispck".
+- pinctrl-names, pinctrl-0
+	Please refer to pinctrl-bindings.txt.
+- isc-ispck
+	The clock for the ISC digital pipeline.
+	- compatible
+		Must be "atmel,sama5d2-isc-ispck".
+	- clock-cells
+		From common clock binding; should be set to 0.
+	- clocks
+		The clock source phandles.
+- isc-mck
+	The clock for the image sensor.
+	- compatible
+		Must be "atmel,sama5d2-isc-mck".
+	- clock-cells
+		From common clock binding; should be set to 0.
+	- clocks
+		The clock source phandles.
+
+ISC supports a single port node with parallel bus. It should contain one
+'port' child node with child 'endpoint' node. Please refer to the bindings
+defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+isc: isc@f0008000 {
+	compatible = "atmel,sama5d2-isc";
+	reg = <0xf0008000 0x4000>;
+	interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+	clocks = <&isc_clk>, <&isc_ispck>;
+	clock-names = "hclock", "ispck";
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		isc_0: endpoint {
+			remote-endpoint = <&ov7740_0>;
+			hsync-active = <1>;
+			vsync-active = <0>;
+			pclk-sample = <1>;
+		};
+	};
+
+	isc_ispck: isc-ispck@0 {
+		compatible = "atmel,sama5d2-isc-ispck";
+		#clock-cells = <0>;
+		clocks = <&isc_clk>, <&iscck>;
+	};
+
+	isc_mck: isc-mck@1 {
+		compatible = "atmel,sama5d2-isc-mck";
+		#clock-cells = <0>;
+		clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+	};
+};
+
+i2c1: i2c@fc028000 {
+	ov7740: camera@21 {
+	compatible = "ovti,ov7740";
+	reg = <0x21>;
+
+	clocks = <&isc_mck>;
+	clock-names = "xvclk";
+	assigned-clocks = <&isc_mck>;
+	assigned-clock-rates = <24000000>;
+
+	port {
+		ov7740_0: endpoint {
+			remote-endpoint = <&isc_0>;
+		};
+	};
+};
-- 
2.7.4

Re: [PATCH v3 0/2] [media] atmel-isc: add driver for Atmel ISC

[Geert Uytterhoeven]

Hi Songjun,

On Tue, May 31, 2016 at 8:58 AM, Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> The Image Sensor Controller driver includes two parts.
> 1) Driver code to implement the ISC function.
> 2) Device tree binding documentation, it describes how
>    to add the ISC in device tree.

Please reduce the CC list. Not everyone who ever touched a line in the
files you modified is working on media or atmel devices.

get_maintainer.pl gives hints, not exact lists of people.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver

[Rob Herring]

On Tue, May 31, 2016 at 02:58:23PM +0800, Songjun Wu wrote:
> DT binding documentation for ISC driver.
> 
> Signed-off-by: Songjun Wu <songjun.wu@atmel.com>
> ---
> 
> Changes in v3:
> - Remove the 'atmel,sensor-preferred'.
> - Modify the isc clock node according to the Rob's remarks.
> 
> Changes in v2:
> - Remove the unit address of the endpoint.
> - Add the unit address to the clock node.
> - Avoid using underscores in node names.
> - Drop the "0x" in the unit address of the i2c node.
> - Modify the description of 'atmel,sensor-preferred'.
> - Add the description for the ISC internal clock.
> 
>  .../devicetree/bindings/media/atmel-isc.txt        | 88 ++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> new file mode 100644
> index 0000000..2ae1d60
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -0,0 +1,88 @@
> +Atmel Image Sensor Controller (ISC)
> +----------------------------------------------
> +
> +Required properties for ISC:
> +- compatible
> +	Must be "atmel,sama5d2-isc"
> +- reg
> +	Physical base address and length of the registers set for the device;
> +- interrupts
> +	Should contain IRQ line for the ISI;
> +- clocks
> +	List of clock specifiers, corresponding to entries in
> +	the clock-names property;
> +	Please refer to clock-bindings.txt.
> +- clock-names
> +	Required elements: "hclock", "ispck".
> +- pinctrl-names, pinctrl-0
> +	Please refer to pinctrl-bindings.txt.
> +- isc-ispck
> +	The clock for the ISC digital pipeline.
> +	- compatible
> +		Must be "atmel,sama5d2-isc-ispck".
> +	- clock-cells
> +		From common clock binding; should be set to 0.
> +	- clocks
> +		The clock source phandles.
> +- isc-mck
> +	The clock for the image sensor.
> +	- compatible
> +		Must be "atmel,sama5d2-isc-mck".
> +	- clock-cells
> +		From common clock binding; should be set to 0.
> +	- clocks
> +		The clock source phandles.
> +
> +ISC supports a single port node with parallel bus. It should contain one
> +'port' child node with child 'endpoint' node. Please refer to the bindings
> +defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +isc: isc@f0008000 {
> +	compatible = "atmel,sama5d2-isc";
> +	reg = <0xf0008000 0x4000>;
> +	interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
> +	clocks = <&isc_clk>, <&isc_ispck>;
> +	clock-names = "hclock", "ispck";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		isc_0: endpoint {
> +			remote-endpoint = <&ov7740_0>;
> +			hsync-active = <1>;
> +			vsync-active = <0>;
> +			pclk-sample = <1>;
> +		};
> +	};
> +
> +	isc_ispck: isc-ispck@0 {

Drop the unit-address. You should only have one if you have a reg 
property.

> +		compatible = "atmel,sama5d2-isc-ispck";
> +		#clock-cells = <0>;
> +		clocks = <&isc_clk>, <&iscck>;
> +	};
> +
> +	isc_mck: isc-mck@1 {

ditto.

I still think these should be implied by atmel,sama5d2-isc and not in 
DT. The fact that you don't have any registers for them pretty much 
indicates that. It is also strange that isc-ispck is used by isc. If 
that is the only user, there is certainly no need to put it in DT.


> +		compatible = "atmel,sama5d2-isc-mck";
> +		#clock-cells = <0>;
> +		clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
> +	};
> +};
> +
> +i2c1: i2c@fc028000 {
> +	ov7740: camera@21 {
> +	compatible = "ovti,ov7740";
> +	reg = <0x21>;
> +
> +	clocks = <&isc_mck>;
> +	clock-names = "xvclk";
> +	assigned-clocks = <&isc_mck>;
> +	assigned-clock-rates = <24000000>;
> +
> +	port {
> +		ov7740_0: endpoint {
> +			remote-endpoint = <&isc_0>;
> +		};
> +	};
> +};
> -- 
> 2.7.4
> 

Re: [PATCH v3 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver

[Boris Brezillon]

On Thu, 2 Jun 2016 18:16:09 -0500
Rob Herring <robh@kernel.org> wrote:

> On Tue, May 31, 2016 at 02:58:23PM +0800, Songjun Wu wrote:
> > DT binding documentation for ISC driver.
> > 
> > Signed-off-by: Songjun Wu <songjun.wu@atmel.com>
> > ---
> > 
> > Changes in v3:
> > - Remove the 'atmel,sensor-preferred'.
> > - Modify the isc clock node according to the Rob's remarks.
> > 
> > Changes in v2:
> > - Remove the unit address of the endpoint.
> > - Add the unit address to the clock node.
> > - Avoid using underscores in node names.
> > - Drop the "0x" in the unit address of the i2c node.
> > - Modify the description of 'atmel,sensor-preferred'.
> > - Add the description for the ISC internal clock.
> > 
> >  .../devicetree/bindings/media/atmel-isc.txt        | 88 ++++++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > new file mode 100644
> > index 0000000..2ae1d60
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > @@ -0,0 +1,88 @@
> > +Atmel Image Sensor Controller (ISC)
> > +----------------------------------------------
> > +
> > +Required properties for ISC:
> > +- compatible
> > +	Must be "atmel,sama5d2-isc"
> > +- reg
> > +	Physical base address and length of the registers set for the device;
> > +- interrupts
> > +	Should contain IRQ line for the ISI;
> > +- clocks
> > +	List of clock specifiers, corresponding to entries in
> > +	the clock-names property;
> > +	Please refer to clock-bindings.txt.
> > +- clock-names
> > +	Required elements: "hclock", "ispck".
> > +- pinctrl-names, pinctrl-0
> > +	Please refer to pinctrl-bindings.txt.
> > +- isc-ispck
> > +	The clock for the ISC digital pipeline.
> > +	- compatible
> > +		Must be "atmel,sama5d2-isc-ispck".
> > +	- clock-cells
> > +		From common clock binding; should be set to 0.
> > +	- clocks
> > +		The clock source phandles.
> > +- isc-mck
> > +	The clock for the image sensor.
> > +	- compatible
> > +		Must be "atmel,sama5d2-isc-mck".
> > +	- clock-cells
> > +		From common clock binding; should be set to 0.
> > +	- clocks
> > +		The clock source phandles.
> > +
> > +ISC supports a single port node with parallel bus. It should contain one
> > +'port' child node with child 'endpoint' node. Please refer to the bindings
> > +defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
> > +
> > +Example:
> > +isc: isc@f0008000 {
> > +	compatible = "atmel,sama5d2-isc";
> > +	reg = <0xf0008000 0x4000>;
> > +	interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
> > +	clocks = <&isc_clk>, <&isc_ispck>;
> > +	clock-names = "hclock", "ispck";
> > +
> > +	port {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		isc_0: endpoint {
> > +			remote-endpoint = <&ov7740_0>;
> > +			hsync-active = <1>;
> > +			vsync-active = <0>;
> > +			pclk-sample = <1>;
> > +		};
> > +	};
> > +
> > +	isc_ispck: isc-ispck@0 {  
> 
> Drop the unit-address. You should only have one if you have a reg 
> property.
> 
> > +		compatible = "atmel,sama5d2-isc-ispck";
> > +		#clock-cells = <0>;
> > +		clocks = <&isc_clk>, <&iscck>;
> > +	};
> > +
> > +	isc_mck: isc-mck@1 {  
> 
> ditto.
> 
> I still think these should be implied by atmel,sama5d2-isc and not in 
> DT. The fact that you don't have any registers for them pretty much 
> indicates that. It is also strange that isc-ispck is used by isc. If 
> that is the only user, there is certainly no need to put it in DT.

I had a look at the block diagram, and it seems you are partially right.
The only clock that is really exported by the ISC is isc_mck (isc_pck is
not).
So I'd suggest dropping these clk sub-nodes, putting the #clock-cells,
and clock-names properties in the isc node, and then referencing the
isc node in you i2c device.

isc: isc@f0008000 {
	compatible = "atmel,sama5d2-isc";
	reg = <0xf0008000 0x4000>;
	interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
	clocks = <&isc_clk>, <&isc_ispck>;
	clock-names = "hclock", "ispck";
	#clock-cells = <0>;
	clock-output-names = "isc-mck";
};

i2c1: i2c@fc028000 {
	/* ... */
	ov7740: camera@21 {
		/* ... */
		clocks = <&isc>;
		clock-names = "xvclk";
	}
};

If I'm wrong and the ISC IP is really exposing several clks, you just
have to change #clock-cells to 1, and use clocks = <&isc X>; in the i2c
device node.

Rob, Songjun, would you agree on this representation?

> 
> 
> > +		compatible = "atmel,sama5d2-isc-mck";
> > +		#clock-cells = <0>;
> > +		clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
> > +	};
> > +};
> > +
> > +i2c1: i2c@fc028000 {
> > +	ov7740: camera@21 {
> > +	compatible = "ovti,ov7740";
> > +	reg = <0x21>;
> > +
> > +	clocks = <&isc_mck>;
> > +	clock-names = "xvclk";
> > +	assigned-clocks = <&isc_mck>;
> > +	assigned-clock-rates = <24000000>;
> > +
> > +	port {
> > +		ov7740_0: endpoint {
> > +			remote-endpoint = <&isc_0>;
> > +		};
> > +	};
> > +};
> > -- 
> > 2.7.4
> >   
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



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

Re: [PATCH v3 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver

[Wu, Songjun]

Hi Rob,

Thank you for your comments.

On 6/3/2016 07:16, Rob Herring wrote:
> On Tue, May 31, 2016 at 02:58:23PM +0800, Songjun Wu wrote:
>> DT binding documentation for ISC driver.
>>
>> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> Changes in v3:
>> - Remove the 'atmel,sensor-preferred'.
>> - Modify the isc clock node according to the Rob's remarks.
>>
>> Changes in v2:
>> - Remove the unit address of the endpoint.
>> - Add the unit address to the clock node.
>> - Avoid using underscores in node names.
>> - Drop the "0x" in the unit address of the i2c node.
>> - Modify the description of 'atmel,sensor-preferred'.
>> - Add the description for the ISC internal clock.
>>
>>  .../devicetree/bindings/media/atmel-isc.txt        | 88 ++++++++++++++++++++++
>>  1 file changed, 88 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> new file mode 100644
>> index 0000000..2ae1d60
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> @@ -0,0 +1,88 @@
>> +Atmel Image Sensor Controller (ISC)
>> +----------------------------------------------
>> +
>> +Required properties for ISC:
>> +- compatible
>> +	Must be "atmel,sama5d2-isc"
>> +- reg
>> +	Physical base address and length of the registers set for the device;
>> +- interrupts
>> +	Should contain IRQ line for the ISI;
>> +- clocks
>> +	List of clock specifiers, corresponding to entries in
>> +	the clock-names property;
>> +	Please refer to clock-bindings.txt.
>> +- clock-names
>> +	Required elements: "hclock", "ispck".
>> +- pinctrl-names, pinctrl-0
>> +	Please refer to pinctrl-bindings.txt.
>> +- isc-ispck
>> +	The clock for the ISC digital pipeline.
>> +	- compatible
>> +		Must be "atmel,sama5d2-isc-ispck".
>> +	- clock-cells
>> +		From common clock binding; should be set to 0.
>> +	- clocks
>> +		The clock source phandles.
>> +- isc-mck
>> +	The clock for the image sensor.
>> +	- compatible
>> +		Must be "atmel,sama5d2-isc-mck".
>> +	- clock-cells
>> +		From common clock binding; should be set to 0.
>> +	- clocks
>> +		The clock source phandles.
>> +
>> +ISC supports a single port node with parallel bus. It should contain one
>> +'port' child node with child 'endpoint' node. Please refer to the bindings
>> +defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +isc: isc@f0008000 {
>> +	compatible = "atmel,sama5d2-isc";
>> +	reg = <0xf0008000 0x4000>;
>> +	interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
>> +	clocks = <&isc_clk>, <&isc_ispck>;
>> +	clock-names = "hclock", "ispck";
>> +
>> +	port {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		isc_0: endpoint {
>> +			remote-endpoint = <&ov7740_0>;
>> +			hsync-active = <1>;
>> +			vsync-active = <0>;
>> +			pclk-sample = <1>;
>> +		};
>> +	};
>> +
>> +	isc_ispck: isc-ispck@0 {
>
> Drop the unit-address. You should only have one if you have a reg
> property.
>
Accept. Thank you.

>> +		compatible = "atmel,sama5d2-isc-ispck";
>> +		#clock-cells = <0>;
>> +		clocks = <&isc_clk>, <&iscck>;
>> +	};
>> +
>> +	isc_mck: isc-mck@1 {
>
> ditto.
>
> I still think these should be implied by atmel,sama5d2-isc and not in
> DT. The fact that you don't have any registers for them pretty much
> indicates that. It is also strange that isc-ispck is used by isc. If
> that is the only user, there is certainly no need to put it in DT.
>
The isc-ispck in only used by isc, it can be removed from DT.
Thank you.
>
>> +		compatible = "atmel,sama5d2-isc-mck";
>> +		#clock-cells = <0>;
>> +		clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
>> +	};
>> +};
>> +
>> +i2c1: i2c@fc028000 {
>> +	ov7740: camera@21 {
>> +	compatible = "ovti,ov7740";
>> +	reg = <0x21>;
>> +
>> +	clocks = <&isc_mck>;
>> +	clock-names = "xvclk";
>> +	assigned-clocks = <&isc_mck>;
>> +	assigned-clock-rates = <24000000>;
>> +
>> +	port {
>> +		ov7740_0: endpoint {
>> +			remote-endpoint = <&isc_0>;
>> +		};
>> +	};
>> +};
>> --
>> 2.7.4
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver

[Wu, Songjun]

On 6/3/2016 19:10, Boris Brezillon wrote:
> On Thu, 2 Jun 2016 18:16:09 -0500
> Rob Herring <robh@kernel.org> wrote:
>
>> On Tue, May 31, 2016 at 02:58:23PM +0800, Songjun Wu wrote:
>>> DT binding documentation for ISC driver.
>>>
>>> Signed-off-by: Songjun Wu <songjun.wu@atmel.com>
>>> ---
>>>
>>> Changes in v3:
>>> - Remove the 'atmel,sensor-preferred'.
>>> - Modify the isc clock node according to the Rob's remarks.
>>>
>>> Changes in v2:
>>> - Remove the unit address of the endpoint.
>>> - Add the unit address to the clock node.
>>> - Avoid using underscores in node names.
>>> - Drop the "0x" in the unit address of the i2c node.
>>> - Modify the description of 'atmel,sensor-preferred'.
>>> - Add the description for the ISC internal clock.
>>>
>>>  .../devicetree/bindings/media/atmel-isc.txt        | 88 ++++++++++++++++++++++
>>>  1 file changed, 88 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> new file mode 100644
>>> index 0000000..2ae1d60
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>>> @@ -0,0 +1,88 @@
>>> +Atmel Image Sensor Controller (ISC)
>>> +----------------------------------------------
>>> +
>>> +Required properties for ISC:
>>> +- compatible
>>> +	Must be "atmel,sama5d2-isc"
>>> +- reg
>>> +	Physical base address and length of the registers set for the device;
>>> +- interrupts
>>> +	Should contain IRQ line for the ISI;
>>> +- clocks
>>> +	List of clock specifiers, corresponding to entries in
>>> +	the clock-names property;
>>> +	Please refer to clock-bindings.txt.
>>> +- clock-names
>>> +	Required elements: "hclock", "ispck".
>>> +- pinctrl-names, pinctrl-0
>>> +	Please refer to pinctrl-bindings.txt.
>>> +- isc-ispck
>>> +	The clock for the ISC digital pipeline.
>>> +	- compatible
>>> +		Must be "atmel,sama5d2-isc-ispck".
>>> +	- clock-cells
>>> +		From common clock binding; should be set to 0.
>>> +	- clocks
>>> +		The clock source phandles.
>>> +- isc-mck
>>> +	The clock for the image sensor.
>>> +	- compatible
>>> +		Must be "atmel,sama5d2-isc-mck".
>>> +	- clock-cells
>>> +		From common clock binding; should be set to 0.
>>> +	- clocks
>>> +		The clock source phandles.
>>> +
>>> +ISC supports a single port node with parallel bus. It should contain one
>>> +'port' child node with child 'endpoint' node. Please refer to the bindings
>>> +defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
>>> +
>>> +Example:
>>> +isc: isc@f0008000 {
>>> +	compatible = "atmel,sama5d2-isc";
>>> +	reg = <0xf0008000 0x4000>;
>>> +	interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
>>> +	clocks = <&isc_clk>, <&isc_ispck>;
>>> +	clock-names = "hclock", "ispck";
>>> +
>>> +	port {
>>> +		#address-cells = <1>;
>>> +		#size-cells = <0>;
>>> +
>>> +		isc_0: endpoint {
>>> +			remote-endpoint = <&ov7740_0>;
>>> +			hsync-active = <1>;
>>> +			vsync-active = <0>;
>>> +			pclk-sample = <1>;
>>> +		};
>>> +	};
>>> +
>>> +	isc_ispck: isc-ispck@0 {
>>
>> Drop the unit-address. You should only have one if you have a reg
>> property.
>>
>>> +		compatible = "atmel,sama5d2-isc-ispck";
>>> +		#clock-cells = <0>;
>>> +		clocks = <&isc_clk>, <&iscck>;
>>> +	};
>>> +
>>> +	isc_mck: isc-mck@1 {
>>
>> ditto.
>>
>> I still think these should be implied by atmel,sama5d2-isc and not in
>> DT. The fact that you don't have any registers for them pretty much
>> indicates that. It is also strange that isc-ispck is used by isc. If
>> that is the only user, there is certainly no need to put it in DT.
>
> I had a look at the block diagram, and it seems you are partially right.
> The only clock that is really exported by the ISC is isc_mck (isc_pck is
> not).
> So I'd suggest dropping these clk sub-nodes, putting the #clock-cells,
> and clock-names properties in the isc node, and then referencing the
> isc node in you i2c device.
>
> isc: isc@f0008000 {
> 	compatible = "atmel,sama5d2-isc";
> 	reg = <0xf0008000 0x4000>;
> 	interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
> 	clocks = <&isc_clk>, <&isc_ispck>;
> 	clock-names = "hclock", "ispck";
> 	#clock-cells = <0>;
> 	clock-output-names = "isc-mck";
> };
>
> i2c1: i2c@fc028000 {
> 	/* ... */
> 	ov7740: camera@21 {
> 		/* ... */
> 		clocks = <&isc>;
> 		clock-names = "xvclk";
> 	}
> };
>
> If I'm wrong and the ISC IP is really exposing several clks, you just
> have to change #clock-cells to 1, and use clocks = <&isc X>; in the i2c
> device node.
>
> Rob, Songjun, would you agree on this representation?
>
Agree, I think it's a good solution.
Thank you for your comments.
>>
>>
>>> +		compatible = "atmel,sama5d2-isc-mck";
>>> +		#clock-cells = <0>;
>>> +		clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
>>> +	};
>>> +};
>>> +
>>> +i2c1: i2c@fc028000 {
>>> +	ov7740: camera@21 {
>>> +	compatible = "ovti,ov7740";
>>> +	reg = <0x21>;
>>> +
>>> +	clocks = <&isc_mck>;
>>> +	clock-names = "xvclk";
>>> +	assigned-clocks = <&isc_mck>;
>>> +	assigned-clock-rates = <24000000>;
>>> +
>>> +	port {
>>> +		ov7740_0: endpoint {
>>> +			remote-endpoint = <&isc_0>;
>>> +		};
>>> +	};
>>> +};
>>> --
>>> 2.7.4
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>