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

[PATCH v5 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 v5:
- Modify the macro definition and the related code.
- Add clock names.

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.
- 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        |   64 +
 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      |  165 ++
 drivers/media/platform/atmel/atmel-isc.c           | 1569 ++++++++++++++++++++
 7 files changed, 1811 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

[PATCH v5 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 v5:
- Add clock names.

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        | 64 ++++++++++++++++++++++
 1 file changed, 64 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..9558a77
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,64 @@
+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", "iscck", "gck";
+	#clock-cells = <0>;
+	clock-output-names = "isc-mck";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_isc_base &pinctrl_isc_data_8bit &pinctrl_isc_data_9_10 &pinctrl_isc_data_11_12>;
+
+	port {
+		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

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

[Rob Herring]

On Fri, Jun 17, 2016 at 04:57:14PM +0800, Songjun Wu wrote:
> DT binding documentation for ISC driver.
> 
> Signed-off-by: Songjun Wu <songjun.wu@atmel.com>
> ---
> 
> Changes in v5:
> - Add clock names.
> 
> 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        | 64 ++++++++++++++++++++++
>  1 file changed, 64 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..9558a77
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -0,0 +1,64 @@
> +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".

What about the 2 other clocks in the example?

> +- #clock-cells
> +	Should be 0.
> +- clock-output-names
> +	Should contain the name of the clock driving the sensor master clock.

State what the name is.

> +- 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", "iscck", "gck";
> +	#clock-cells = <0>;
> +	clock-output-names = "isc-mck";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_isc_base &pinctrl_isc_data_8bit &pinctrl_isc_data_9_10 &pinctrl_isc_data_11_12>;
> +
> +	port {
> +		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";

Indentation is still wrong here...

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

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

[Wu, Songjun]

Hi Rob,

Thank you for your comments.

On 6/20/2016 21:25, Rob Herring wrote:
> On Fri, Jun 17, 2016 at 04:57:14PM +0800, Songjun Wu wrote:
>> DT binding documentation for ISC driver.
>>
>> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> Changes in v5:
>> - Add clock names.
>>
>> 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        | 64 ++++++++++++++++++++++
>>  1 file changed, 64 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..9558a77
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>> @@ -0,0 +1,64 @@
>> +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".
>
> What about the 2 other clocks in the example?
>
The other clocks is optional, not required.
Do you have any suggestion?

>> +- #clock-cells
>> +	Should be 0.
>> +- clock-output-names
>> +	Should contain the name of the clock driving the sensor master clock.
>
> State what the name is.
>
"isc-mck" will be added.

>> +- 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", "iscck", "gck";
>> +	#clock-cells = <0>;
>> +	clock-output-names = "isc-mck";
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_isc_base &pinctrl_isc_data_8bit &pinctrl_isc_data_9_10 &pinctrl_isc_data_11_12>;
>> +
>> +	port {
>> +		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";
>
> Indentation is still wrong here...
>
Sorry, my mistake.
It should be fixed.

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

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

[Boris Brezillon]

On Fri, 24 Jun 2016 13:54:09 +0800
"Wu, Songjun" <songjun.wu@atmel.com> wrote:

> Hi Rob,
> 
> Thank you for your comments.
> 
> On 6/20/2016 21:25, Rob Herring wrote:
> > On Fri, Jun 17, 2016 at 04:57:14PM +0800, Songjun Wu wrote:  
> >> DT binding documentation for ISC driver.
> >>
> >> Signed-off-by: Songjun Wu <songjun.wu@atmel.com>
> >> ---
> >>
> >> Changes in v5:
> >> - Add clock names.
> >>
> >> 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        | 64 ++++++++++++++++++++++
> >>  1 file changed, 64 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..9558a77
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> >> @@ -0,0 +1,64 @@
> >> +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".  
> >
> > What about the 2 other clocks in the example?
> >  
> The other clocks is optional, not required.
> Do you have any suggestion?

Just add a second look at the sama5d2 and iscck gck are not optional.
What we call optional clocks are clocks that are not necessarily
required depending on the IP revision or external clocks that are board
dependent. This is not the case here: iscck and gck are internal to the
SoC, and are always available. Whether you want to use them or not is a
different question, and should not be encoded in the clocks/clock-names
properties.

To sum-up, define those 3 clocks as required and document their names.

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

[Wu, Songjun]

On 6/24/2016 15:35, Boris Brezillon wrote:
> On Fri, 24 Jun 2016 13:54:09 +0800
> "Wu, Songjun" <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>
>> Hi Rob,
>>
>> Thank you for your comments.
>>
>> On 6/20/2016 21:25, Rob Herring wrote:
>>> On Fri, Jun 17, 2016 at 04:57:14PM +0800, Songjun Wu wrote:
>>>> DT binding documentation for ISC driver.
>>>>
>>>> Signed-off-by: Songjun Wu <songjun.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>
>>>> Changes in v5:
>>>> - Add clock names.
>>>>
>>>> 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        | 64 ++++++++++++++++++++++
>>>>  1 file changed, 64 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..9558a77
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
>>>> @@ -0,0 +1,64 @@
>>>> +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".
>>>
>>> What about the 2 other clocks in the example?
>>>
>> The other clocks is optional, not required.
>> Do you have any suggestion?
>
> Just add a second look at the sama5d2 and iscck gck are not optional.
> What we call optional clocks are clocks that are not necessarily
> required depending on the IP revision or external clocks that are board
> dependent. This is not the case here: iscck and gck are internal to the
> SoC, and are always available. Whether you want to use them or not is a
> different question, and should not be encoded in the clocks/clock-names
> properties.
>
> To sum-up, define those 3 clocks as required and document their names.
>
Thank you for your suggestion.
--
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 v5 0/2] [media] atmel-isc: add driver for Atmel ISC

[Hans Verkuil]

Hi Songjun,

First of all, please CC patch 2/2 to linux-media as well the next time you post this.
I only see 1/2 on the mailinglist, and we need both.

Secondly, before I can accept it you need to run the v4l2-compliance test first and
I need to see the output of that test.

The compliance test is here: https://git.linuxtv.org/v4l-utils.git. Always compile it from
the repository so you know you are using the latest most up to date version.

Since this driver supports multiple pixelformats you need to test with the -f option,
which tests streaming for all pixelformats.

Obviously, there shouldn't be any FAILs :-)

I greatly simplifies the code review if I know it passes the compliance test.

Regards,

	Hans

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

[Wu, Songjun]

On 7/1/2016 20:20, Hans Verkuil wrote:
> Hi Songjun,
>
> First of all, please CC patch 2/2 to linux-media as well the next time you post this.
> I only see 1/2 on the mailinglist, and we need both.
>
> Secondly, before I can accept it you need to run the v4l2-compliance test first and
> I need to see the output of that test.
>
> The compliance test is here: https://git.linuxtv.org/v4l-utils.git. Always compile it from
> the repository so you know you are using the latest most up to date version.
>
> Since this driver supports multiple pixelformats you need to test with the -f option,
> which tests streaming for all pixelformats.
>
> Obviously, there shouldn't be any FAILs :-)
>
> I greatly simplifies the code review if I know it passes the compliance test.
>
Hi Hans,

You suggestion is very helpful to me.
I will give the output of the compliance test in next version.

> Regards,
>
> 	Hans
>