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

[PATCH v2 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 v2:
- Add "depends on COMMON_CLK && VIDEO_V4L2_SUBDEV_API"
  in Kconfig file.
- Delete the redundant blank in atmel-isc-regs.h
- Enclose the expression in parentheses in some macros.
- Sort the header file alphabetically.
- Move the 'to_isc_clk(hw)' right after the struct
  isc_clk definition
- Move the global variables into the struct isc_device.
- Change some int variable to unsigned int variable.
- Replace pr_debug() with dev_dbg();
- Delete the loop while in 'isc_clk_enable' function.
- Rename the 'tmp_rate' and 'tmp_diff' to 'rate' and
  'diff' in 'isc_clk_determine_rate' function.
- Delete the inner parentheses in 'isc_clk_register'
  function.
- Delete the variable i in 'isc_parse_dt' function.
- Replace 'hsync_active', 'vsync_active' and 'pclk_sample'
  with 'pfe_cfg0' in struct isc_subdev_entity.
- Move the 'isc_parse_dt'function down right above the
  'atmel_isc_probe' function.
- Add the code to support VIDIOC_CREATE_BUFS in
  'isc_queue_setup' function.
- Replace the variable type 'unsigned long' with 'dma_addr_t'
  in 'isc_start_dma' function.
- Remove the list_del call, turn list_for_each_entry_safe
  into list_for_each entry, and add a INIT_LIST_HEAD after
  the loop in 'isc_start_streaming', 'isc_stop_streaming'
  and 'isc_subdev_cleanup' 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 check of the '!res' since devm_ioremap_resource()
  will check for it.
- Remove the error message in case of failure
  when call devm_ioremap_resource.
- Remove the cast the isc pointer to (void *).
- 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        |  100 ++
 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           | 1583 ++++++++++++++++++++
 7 files changed, 1972 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 v2 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 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        | 100 +++++++++++++++++++++
 1 file changed, 100 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..9e65395
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,100 @@
+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", "ispck".
+- pinctrl-names, pinctrl-0
+	Please refer to pinctrl-bindings.txt.
+- clk-in-isc
+	ISC internal clock node, it includes two clock nodes,
+	isc-ispck and isc-mck.
+- atmel,sensor-preferred
+	ISC may convert the raw format to the specified format when the sensor
+	outputs the raw format, and the sensor may output the specified format
+	directly. If sensor is preferred to output the specified format
+	directly, the value should be 1 (1-preferred, 0-not).
+	The default value is 1.
+
+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.
+
+Required properties for the ISC internal clocks:
+- #address-cells
+	should be 1 (reg is used to encode clk id).
+- #size-cells
+	should be 0 (reg is used to encode clk id).
+- name
+	device tree node describing a ISC internal clock.
+	* #clock-cells: from common clock binding; should be set to 0.
+	* reg: clock id, there are two values,
+	       <0> is ISP clock, <1> is master clock.
+	* clocks: shall be the isc internal clock source phandles.
+		  e.g. clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
+
+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";
+	atmel,sensor-preferred = <1>;
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		isc_0: endpoint {
+			remote-endpoint = <&ov7740_0>;
+			hsync-active = <1>;
+			vsync-active = <0>;
+			pclk-sample = <1>;
+		};
+	};
+
+	clk-in-isc {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		isc_ispck: isc-ispck@0 {
+			#clock-cells = <0>;
+			reg = <0>;
+			clocks = <&isc_clk>, <&iscck>;
+		};
+
+		isc_mck: isc-mck@1 {
+			#clock-cells = <0>;
+			reg = <1>;
+			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 v2 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver

[Rob Herring]

On Wed, May 18, 2016 at 03:46:29PM +0800, Songjun Wu wrote:
> DT binding documentation for ISC driver.
> 
> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
> 
> 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        | 100 +++++++++++++++++++++
>  1 file changed, 100 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..9e65395
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -0,0 +1,100 @@
> +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", "ispck".
> +- pinctrl-names, pinctrl-0
> +	Please refer to pinctrl-bindings.txt.
> +- clk-in-isc
> +	ISC internal clock node, it includes two clock nodes,
> +	isc-ispck and isc-mck.

And you need to describe each of these nodes and the properties they 
have. But more on this below...

> +- atmel,sensor-preferred
> +	ISC may convert the raw format to the specified format when the sensor
> +	outputs the raw format, and the sensor may output the specified format
> +	directly. If sensor is preferred to output the specified format
> +	directly, the value should be 1 (1-preferred, 0-not).
> +	The default value is 1.

I don't think this belongs in DT. It sounds more like a configuration 
and userspace decision. It is still not clear what you mean by 
"specified format".

> +
> +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.
> +
> +Required properties for the ISC internal clocks:
> +- #address-cells
> +	should be 1 (reg is used to encode clk id).
> +- #size-cells
> +	should be 0 (reg is used to encode clk id).
> +- name
> +	device tree node describing a ISC internal clock.
> +	* #clock-cells: from common clock binding; should be set to 0.
> +	* reg: clock id, there are two values,
> +	       <0> is ISP clock, <1> is master clock.
> +	* clocks: shall be the isc internal clock source phandles.
> +		  e.g. clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
> +
> +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";
> +	atmel,sensor-preferred = <1>;
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		isc_0: endpoint {
> +			remote-endpoint = <&ov7740_0>;
> +			hsync-active = <1>;
> +			vsync-active = <0>;
> +			pclk-sample = <1>;
> +		};
> +	};
> +
> +	clk-in-isc {

Is this really separate from the rest of the isc block. It would be 
much more simple to define all the input clocks at the parent and then 
reference these 2 clocks with <&isc 0> and <&isc 1>.

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		isc_ispck: isc-ispck@0 {

These would need a compatible string otherwise.

> +			#clock-cells = <0>;
> +			reg = <0>;

Are 0 and 1 meaningful in terms of the hardware description?

> +			clocks = <&isc_clk>, <&iscck>;
> +		};
> +
> +		isc_mck: isc-mck@1 {
> +			#clock-cells = <0>;
> +			reg = <1>;
> +			clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
> +		};
> +	};
> +};
--
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 v2 2/2] [media] atmel-isc: DT binding for Image Sensor Controller driver

[Wu, Songjun]

Hi Rob,

Thank you for your comments.

On 5/19/2016 07:05, Rob Herring wrote:
> On Wed, May 18, 2016 at 03:46:29PM +0800, Songjun Wu wrote:
>> DT binding documentation for ISC driver.
>>
>> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> 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        | 100 +++++++++++++++++++++
>>  1 file changed, 100 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..9e65395
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> @@ -0,0 +1,100 @@
>> +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", "ispck".
>> +- pinctrl-names, pinctrl-0
>> +	Please refer to pinctrl-bindings.txt.
>> +- clk-in-isc
>> +	ISC internal clock node, it includes two clock nodes,
>> +	isc-ispck and isc-mck.
>
> And you need to describe each of these nodes and the properties they
> have. But more on this below...
>
Should I move the description of clock node below to here?

>> +- atmel,sensor-preferred
>> +	ISC may convert the raw format to the specified format when the sensor
>> +	outputs the raw format, and the sensor may output the specified format
>> +	directly. If sensor is preferred to output the specified format
>> +	directly, the value should be 1 (1-preferred, 0-not).
>> +	The default value is 1.
>
> I don't think this belongs in DT. It sounds more like a configuration
> and userspace decision. It is still not clear what you mean by
> "specified format".
>
Accept, I will remove this.

>> +
>> +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.
>> +
>> +Required properties for the ISC internal clocks:
>> +- #address-cells
>> +	should be 1 (reg is used to encode clk id).
>> +- #size-cells
>> +	should be 0 (reg is used to encode clk id).
>> +- name
>> +	device tree node describing a ISC internal clock.
>> +	* #clock-cells: from common clock binding; should be set to 0.
>> +	* reg: clock id, there are two values,
>> +	       <0> is ISP clock, <1> is master clock.
>> +	* clocks: shall be the isc internal clock source phandles.
>> +		  e.g. clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
>> +
>> +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";
>> +	atmel,sensor-preferred = <1>;
>> +
>> +	port {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		isc_0: endpoint {
>> +			remote-endpoint = <&ov7740_0>;
>> +			hsync-active = <1>;
>> +			vsync-active = <0>;
>> +			pclk-sample = <1>;
>> +		};
>> +	};
>> +
>> +	clk-in-isc {
>
> Is this really separate from the rest of the isc block. It would be
> much more simple to define all the input clocks at the parent and then
> reference these 2 clocks with <&isc 0> and <&isc 1>.
>
Yes, it's separate from the rest of isc block.
It will register two clocks, one is used in ISC internal processor, the 
other will provide the clock to the sensor outside.
I think it's better that the parent clock is included in node 
'clk-in-isc' node. I will try to optimize this node.

>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		isc_ispck: isc-ispck@0 {
>
> These would need a compatible string otherwise.
>
>> +			#clock-cells = <0>;
>> +			reg = <0>;
>
> Are 0 and 1 meaningful in terms of the hardware description?
>
Yes, It's meaningful.
The register field is different, 0 and 1 can distinguish the different 
clock.

>> +			clocks = <&isc_clk>, <&iscck>;
>> +		};
>> +
>> +		isc_mck: isc-mck@1 {
>> +			#clock-cells = <0>;
>> +			reg = <1>;
>> +			clocks = <&isc_clk>, <&iscck>, <&isc_gclk>;
>> +		};
>> +	};
>> +};
--
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