devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] [media] atmel-isc: add driver for Atmel ISC
@ 2016-06-07  7:11 Songjun Wu
  2016-06-07  7:11 ` [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver Songjun Wu
  0 siblings, 1 reply; 8+ messages in thread
From: Songjun Wu @ 2016-06-07  7:11 UTC (permalink / raw)
  To: laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
	nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	robh-DgEjT+Ai2ygdnm+yROfE0A
  Cc: Songjun Wu, Fabien Dessenne, Ian Campbell,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Mauro Carvalho Chehab,
	Richard Röjfors, Benoit Parrot, Kumar Gala,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mikhail Ulyanov,
	Sudip Mukherjee, Rob Herring, Pawel Moll, Peter Griffin,
	Geert Uytterhoeven, Hans Verkuil, Mark Rutland,
	linux-media-u79uwXL29TY76Z2rM5mHXA, Simon Horman

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 v4:
- Modify the isc clock code since the dt is changed.
- Remove the isc clock nodes.

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.
- 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        |   69 +
 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           | 1580 ++++++++++++++++++++
 7 files changed, 1938 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
  2016-06-07  7:11 [PATCH v4 0/2] [media] atmel-isc: add driver for Atmel ISC Songjun Wu
@ 2016-06-07  7:11 ` Songjun Wu
       [not found]   ` <1465283513-30224-3-git-send-email-songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Songjun Wu @ 2016-06-07  7:11 UTC (permalink / raw)
  To: laurent.pinchart, nicolas.ferre, boris.brezillon,
	alexandre.belloni, robh
  Cc: Songjun Wu, devicetree, Kumar Gala, linux-kernel, Ian Campbell,
	Rob Herring, Pawel Moll, Mark Rutland

DT binding documentation for ISC driver.

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

Changes in v4:
- Remove the isc clock nodes.

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        | 69 ++++++++++++++++++++++
 1 file changed, 69 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..3f83524
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,69 @@
+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 ISC.
+- clocks
+	List of clock specifiers, corresponding to entries in
+	the clock-names property;
+	Please refer to clock-bindings.txt.
+- clock-names
+	Required elements: "hclock".
+- #clock-cells
+	Should be 0.
+- clock-output-names
+	Should contain the name of the clock driving the sensor master clock.
+- pinctrl-names, pinctrl-0
+	Please refer to pinctrl-bindings.txt.
+
+
+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>, <&iscck>, <&isc_gclk>;
+	clock-names = "hclock";
+	#clock-cells = <0>;
+	clock-output-names = "isc-mck";
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		isc_0: endpoint {
+			remote-endpoint = <&ov7740_0>;
+			hsync-active = <1>;
+			vsync-active = <0>;
+			pclk-sample = <1>;
+		};
+	};
+
+
+};
+
+i2c1: i2c@fc028000 {
+	ov7740: camera@21 {
+	compatible = "ovti,ov7740";
+	reg = <0x21>;
+
+	clocks = <&isc>;
+	clock-names = "xvclk";
+	assigned-clocks = <&isc>;
+	assigned-clock-rates = <24000000>;
+
+	port {
+		ov7740_0: endpoint {
+			remote-endpoint = <&isc_0>;
+		};
+	};
+};
-- 
2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
       [not found]   ` <1465283513-30224-3-git-send-email-songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
@ 2016-06-08 20:00     ` Rob Herring
  2016-06-12  9:35       ` Wu, Songjun
  2016-06-08 21:57     ` Boris Brezillon
  1 sibling, 1 reply; 8+ messages in thread
From: Rob Herring @ 2016-06-08 20:00 UTC (permalink / raw)
  To: Songjun Wu
  Cc: laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
	nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Kumar Gala,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ian Campbell, Pawel Moll,
	Mark Rutland

On Tue, Jun 07, 2016 at 03:11:53PM +0800, Songjun Wu wrote:
> DT binding documentation for ISC driver.
> 
> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
> 
> Changes in v4:
> - Remove the isc clock nodes.
> 
> 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        | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt

Almost there...

> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> new file mode 100644
> index 0000000..3f83524
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -0,0 +1,69 @@
> +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 ISC.
> +- clocks
> +	List of clock specifiers, corresponding to entries in
> +	the clock-names property;
> +	Please refer to clock-bindings.txt.
> +- clock-names
> +	Required elements: "hclock".
> +- #clock-cells
> +	Should be 0.
> +- clock-output-names
> +	Should contain the name of the clock driving the sensor master clock.
> +- pinctrl-names, pinctrl-0
> +	Please refer to pinctrl-bindings.txt.

Required, but not in your example.

> +
> +
> +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>, <&iscck>, <&isc_gclk>;
> +	clock-names = "hclock";
> +	#clock-cells = <0>;
> +	clock-output-names = "isc-mck";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;

These 2 you can drop.

> +
> +		isc_0: endpoint {
> +			remote-endpoint = <&ov7740_0>;
> +			hsync-active = <1>;
> +			vsync-active = <0>;
> +			pclk-sample = <1>;
> +		};
> +	};
> +
> +
> +};
> +
> +i2c1: i2c@fc028000 {
> +	ov7740: camera@21 {
> +	compatible = "ovti,ov7740";

Fix the indentation.

> +	reg = <0x21>;
> +
> +	clocks = <&isc>;
> +	clock-names = "xvclk";
> +	assigned-clocks = <&isc>;
> +	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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
       [not found]   ` <1465283513-30224-3-git-send-email-songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
  2016-06-08 20:00     ` Rob Herring
@ 2016-06-08 21:57     ` Boris Brezillon
  2016-06-12  3:04       ` Wu, Songjun
  1 sibling, 1 reply; 8+ messages in thread
From: Boris Brezillon @ 2016-06-08 21:57 UTC (permalink / raw)
  To: Songjun Wu
  Cc: laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
	nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	robh-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Kumar Gala, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ian Campbell,
	Rob Herring, Pawel Moll, Mark Rutland

On Tue, 7 Jun 2016 15:11:53 +0800
Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:

> DT binding documentation for ISC driver.
> 
> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
> 
> Changes in v4:
> - Remove the isc clock nodes.
> 
> 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        | 69 ++++++++++++++++++++++
>  1 file changed, 69 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..3f83524
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -0,0 +1,69 @@
> +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 ISC.
> +- clocks
> +	List of clock specifiers, corresponding to entries in
> +	the clock-names property;
> +	Please refer to clock-bindings.txt.
> +- clock-names
> +	Required elements: "hclock".
> +- #clock-cells
> +	Should be 0.
> +- clock-output-names
> +	Should contain the name of the clock driving the sensor master clock.
> +- pinctrl-names, pinctrl-0
> +	Please refer to pinctrl-bindings.txt.
> +
> +
> +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>, <&iscck>, <&isc_gclk>;
> +	clock-names = "hclock";

You have 3 clocks here and only one name. Are you sure this example is
actually working?

> +	#clock-cells = <0>;
> +	clock-output-names = "isc-mck";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		isc_0: endpoint {
> +			remote-endpoint = <&ov7740_0>;
> +			hsync-active = <1>;
> +			vsync-active = <0>;
> +			pclk-sample = <1>;
> +		};
> +	};
> +
> +
> +};
> +
> +i2c1: i2c@fc028000 {
> +	ov7740: camera@21 {
> +	compatible = "ovti,ov7740";
> +	reg = <0x21>;
> +
> +	clocks = <&isc>;
> +	clock-names = "xvclk";
> +	assigned-clocks = <&isc>;
> +	assigned-clock-rates = <24000000>;
> +
> +	port {
> +		ov7740_0: endpoint {
> +			remote-endpoint = <&isc_0>;
> +		};
> +	};
> +};



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
  2016-06-08 21:57     ` Boris Brezillon
@ 2016-06-12  3:04       ` Wu, Songjun
       [not found]         ` <6a878440-042f-c447-19cb-aa05b5dc535d-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Wu, Songjun @ 2016-06-12  3:04 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
	nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	robh-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Kumar Gala, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ian Campbell,
	Rob Herring, Pawel Moll, Mark Rutland



On 6/9/2016 05:57, Boris Brezillon wrote:
> On Tue, 7 Jun 2016 15:11:53 +0800
> Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>
>> DT binding documentation for ISC driver.
>>
>> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> Changes in v4:
>> - Remove the isc clock nodes.
>>
>> 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        | 69 ++++++++++++++++++++++
>>  1 file changed, 69 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..3f83524
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> @@ -0,0 +1,69 @@
>> +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 ISC.
>> +- clocks
>> +	List of clock specifiers, corresponding to entries in
>> +	the clock-names property;
>> +	Please refer to clock-bindings.txt.
>> +- clock-names
>> +	Required elements: "hclock".
>> +- #clock-cells
>> +	Should be 0.
>> +- clock-output-names
>> +	Should contain the name of the clock driving the sensor master clock.
>> +- pinctrl-names, pinctrl-0
>> +	Please refer to pinctrl-bindings.txt.
>> +
>> +
>> +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>, <&iscck>, <&isc_gclk>;
>> +	clock-names = "hclock";
>
> You have 3 clocks here and only one name. Are you sure this example is
> actually working?
>
The isc_clk is mandatory, but the other two clocks are optional, so I 
did not give their name. This example is tested.
Should I add the name for the other two clocks?

>> +	#clock-cells = <0>;
>> +	clock-output-names = "isc-mck";
>> +
>> +	port {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		isc_0: endpoint {
>> +			remote-endpoint = <&ov7740_0>;
>> +			hsync-active = <1>;
>> +			vsync-active = <0>;
>> +			pclk-sample = <1>;
>> +		};
>> +	};
>> +
>> +
>> +};
>> +
>> +i2c1: i2c@fc028000 {
>> +	ov7740: camera@21 {
>> +	compatible = "ovti,ov7740";
>> +	reg = <0x21>;
>> +
>> +	clocks = <&isc>;
>> +	clock-names = "xvclk";
>> +	assigned-clocks = <&isc>;
>> +	assigned-clock-rates = <24000000>;
>> +
>> +	port {
>> +		ov7740_0: endpoint {
>> +			remote-endpoint = <&isc_0>;
>> +		};
>> +	};
>> +};
>
>
>
--
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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
       [not found]         ` <6a878440-042f-c447-19cb-aa05b5dc535d-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
@ 2016-06-12  7:23           ` Boris Brezillon
  2016-06-12  9:36             ` Wu, Songjun
  0 siblings, 1 reply; 8+ messages in thread
From: Boris Brezillon @ 2016-06-12  7:23 UTC (permalink / raw)
  To: Wu, Songjun
  Cc: laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
	nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	robh-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Kumar Gala, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ian Campbell,
	Rob Herring, Pawel Moll, Mark Rutland

On Sun, 12 Jun 2016 11:04:27 +0800
"Wu, Songjun" <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:

> On 6/9/2016 05:57, Boris Brezillon wrote:
> > On Tue, 7 Jun 2016 15:11:53 +0800
> > Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> >  
> >> DT binding documentation for ISC driver.
> >>
> >> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> >> ---
> >>
> >> Changes in v4:
> >> - Remove the isc clock nodes.
> >>
> >> 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        | 69 ++++++++++++++++++++++
> >>  1 file changed, 69 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..3f83524
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> >> @@ -0,0 +1,69 @@
> >> +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 ISC.
> >> +- clocks
> >> +	List of clock specifiers, corresponding to entries in
> >> +	the clock-names property;
> >> +	Please refer to clock-bindings.txt.
> >> +- clock-names
> >> +	Required elements: "hclock".
> >> +- #clock-cells
> >> +	Should be 0.
> >> +- clock-output-names
> >> +	Should contain the name of the clock driving the sensor master clock.
> >> +- pinctrl-names, pinctrl-0
> >> +	Please refer to pinctrl-bindings.txt.
> >> +
> >> +
> >> +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>, <&iscck>, <&isc_gclk>;
> >> +	clock-names = "hclock";  
> >
> > You have 3 clocks here and only one name. Are you sure this example is
> > actually working?
> >  
> The isc_clk is mandatory, but the other two clocks are optional, so I 
> did not give their name. This example is tested.
> Should I add the name for the other two clocks?

It only works here because you're using these clocks as the parents of
the ispck ismck clocks, but that's an implementation detail, and
consumer are usually referencing their clocks by name. So yes, I'd
recommend specifying names here, even if it's not strictly required by
your current implementation.

> 
> >> +	#clock-cells = <0>;
> >> +	clock-output-names = "isc-mck";
> >> +
> >> +	port {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +
> >> +		isc_0: endpoint {
> >> +			remote-endpoint = <&ov7740_0>;
> >> +			hsync-active = <1>;
> >> +			vsync-active = <0>;
> >> +			pclk-sample = <1>;
> >> +		};
> >> +	};
> >> +
> >> +
> >> +};
> >> +
> >> +i2c1: i2c@fc028000 {
> >> +	ov7740: camera@21 {
> >> +	compatible = "ovti,ov7740";
> >> +	reg = <0x21>;
> >> +
> >> +	clocks = <&isc>;
> >> +	clock-names = "xvclk";
> >> +	assigned-clocks = <&isc>;
> >> +	assigned-clock-rates = <24000000>;
> >> +
> >> +	port {
> >> +		ov7740_0: endpoint {
> >> +			remote-endpoint = <&isc_0>;
> >> +		};
> >> +	};
> >> +};  
> >
> >
> >  



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
  2016-06-08 20:00     ` Rob Herring
@ 2016-06-12  9:35       ` Wu, Songjun
  0 siblings, 0 replies; 8+ messages in thread
From: Wu, Songjun @ 2016-06-12  9:35 UTC (permalink / raw)
  To: Rob Herring
  Cc: laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
	nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Kumar Gala,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ian Campbell, Pawel Moll,
	Mark Rutland



On 6/9/2016 04:00, Rob Herring wrote:
> On Tue, Jun 07, 2016 at 03:11:53PM +0800, Songjun Wu wrote:
>> DT binding documentation for ISC driver.
>>
>> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> Changes in v4:
>> - Remove the isc clock nodes.
>>
>> 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        | 69 ++++++++++++++++++++++
>>  1 file changed, 69 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt
>
> Almost there...
>
>> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> new file mode 100644
>> index 0000000..3f83524
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> @@ -0,0 +1,69 @@
>> +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 ISC.
>> +- clocks
>> +	List of clock specifiers, corresponding to entries in
>> +	the clock-names property;
>> +	Please refer to clock-bindings.txt.
>> +- clock-names
>> +	Required elements: "hclock".
>> +- #clock-cells
>> +	Should be 0.
>> +- clock-output-names
>> +	Should contain the name of the clock driving the sensor master clock.
>> +- pinctrl-names, pinctrl-0
>> +	Please refer to pinctrl-bindings.txt.
>
> Required, but not in your example.
>
I will add it to the example.

>> +
>> +
>> +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>, <&iscck>, <&isc_gclk>;
>> +	clock-names = "hclock";
>> +	#clock-cells = <0>;
>> +	clock-output-names = "isc-mck";
>> +
>> +	port {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>
> These 2 you can drop.
>
Accept. Thank you.

>> +
>> +		isc_0: endpoint {
>> +			remote-endpoint = <&ov7740_0>;
>> +			hsync-active = <1>;
>> +			vsync-active = <0>;
>> +			pclk-sample = <1>;
>> +		};
>> +	};
>> +
>> +
>> +};
>> +
>> +i2c1: i2c@fc028000 {
>> +	ov7740: camera@21 {
>> +	compatible = "ovti,ov7740";
>
> Fix the indentation.
>
Accept. Thank you.

>> +	reg = <0x21>;
>> +
>> +	clocks = <&isc>;
>> +	clock-names = "xvclk";
>> +	assigned-clocks = <&isc>;
>> +	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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver
  2016-06-12  7:23           ` Boris Brezillon
@ 2016-06-12  9:36             ` Wu, Songjun
  0 siblings, 0 replies; 8+ messages in thread
From: Wu, Songjun @ 2016-06-12  9:36 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
	nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
	alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	robh-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Kumar Gala, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ian Campbell,
	Rob Herring, Pawel Moll, Mark Rutland



On 6/12/2016 15:23, Boris Brezillon wrote:
> On Sun, 12 Jun 2016 11:04:27 +0800
> "Wu, Songjun" <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>
>> On 6/9/2016 05:57, Boris Brezillon wrote:
>>> On Tue, 7 Jun 2016 15:11:53 +0800
>>> Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> DT binding documentation for ISC driver.
>>>>
>>>> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>
>>>> Changes in v4:
>>>> - Remove the isc clock nodes.
>>>>
>>>> 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        | 69 ++++++++++++++++++++++
>>>>  1 file changed, 69 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..3f83524
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>>>> @@ -0,0 +1,69 @@
>>>> +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 ISC.
>>>> +- clocks
>>>> +	List of clock specifiers, corresponding to entries in
>>>> +	the clock-names property;
>>>> +	Please refer to clock-bindings.txt.
>>>> +- clock-names
>>>> +	Required elements: "hclock".
>>>> +- #clock-cells
>>>> +	Should be 0.
>>>> +- clock-output-names
>>>> +	Should contain the name of the clock driving the sensor master clock.
>>>> +- pinctrl-names, pinctrl-0
>>>> +	Please refer to pinctrl-bindings.txt.
>>>> +
>>>> +
>>>> +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>, <&iscck>, <&isc_gclk>;
>>>> +	clock-names = "hclock";
>>>
>>> You have 3 clocks here and only one name. Are you sure this example is
>>> actually working?
>>>
>> The isc_clk is mandatory, but the other two clocks are optional, so I
>> did not give their name. This example is tested.
>> Should I add the name for the other two clocks?
>
> It only works here because you're using these clocks as the parents of
> the ispck ismck clocks, but that's an implementation detail, and
> consumer are usually referencing their clocks by name. So yes, I'd
> recommend specifying names here, even if it's not strictly required by
> your current implementation.
>
Accept. The clock names will be specified.
Thank you.

>>
>>>> +	#clock-cells = <0>;
>>>> +	clock-output-names = "isc-mck";
>>>> +
>>>> +	port {
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <0>;
>>>> +
>>>> +		isc_0: endpoint {
>>>> +			remote-endpoint = <&ov7740_0>;
>>>> +			hsync-active = <1>;
>>>> +			vsync-active = <0>;
>>>> +			pclk-sample = <1>;
>>>> +		};
>>>> +	};
>>>> +
>>>> +
>>>> +};
>>>> +
>>>> +i2c1: i2c@fc028000 {
>>>> +	ov7740: camera@21 {
>>>> +	compatible = "ovti,ov7740";
>>>> +	reg = <0x21>;
>>>> +
>>>> +	clocks = <&isc>;
>>>> +	clock-names = "xvclk";
>>>> +	assigned-clocks = <&isc>;
>>>> +	assigned-clock-rates = <24000000>;
>>>> +
>>>> +	port {
>>>> +		ov7740_0: endpoint {
>>>> +			remote-endpoint = <&isc_0>;
>>>> +		};
>>>> +	};
>>>> +};
>>>
>>>
>>>
>
>
>
--
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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-06-12  9:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07  7:11 [PATCH v4 0/2] [media] atmel-isc: add driver for Atmel ISC Songjun Wu
2016-06-07  7:11 ` [PATCH v4 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver Songjun Wu
     [not found]   ` <1465283513-30224-3-git-send-email-songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
2016-06-08 20:00     ` Rob Herring
2016-06-12  9:35       ` Wu, Songjun
2016-06-08 21:57     ` Boris Brezillon
2016-06-12  3:04       ` Wu, Songjun
     [not found]         ` <6a878440-042f-c447-19cb-aa05b5dc535d-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
2016-06-12  7:23           ` Boris Brezillon
2016-06-12  9:36             ` Wu, Songjun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).