Re: [PATCH 00/14] serial: langtiq: Add CCF suppport

[Wu, Songjun]

On 10/16/2018 5:58 AM, Paul Burton wrote:

Hi Songjun,

On Mon, Sep 24, 2018 at 06:27:49PM +0800, Songjun Wu wrote:

This patch series is for adding common clock framework support
for langtiq serial driver, mainly includes:

s/langtiq/lantiq/ ...

Thanks, it will be fixed.

1) Add common clock framework support.
2) Modify the dts file according to the DT conventions.
3) Replace the platform dependent functions with kernel functions

Songjun Wu (14):
   MIPS: dts: Change upper case to lower case
   MIPS: dts: Add aliases node for lantiq danube serial
   serial: lantiq: Get serial id from dts
   serial: lantiq: Change ltq_w32_mask to asc_update_bits
   MIPS: lantiq: Unselect SWAP_IO_SPACE when LANTIQ is selected
   serial: lantiq: Use readl/writel instead of ltq_r32/ltq_w32
   serial: lantiq: Rename fpiclk to freqclk
   serial: lantiq: Replace clk_enable/clk_disable with clk generic API
   serial: lantiq: Add CCF support
   serial: lantiq: Reorder the head files
   include: Add lantiq.h in include/linux/
   serial: lantiq: Replace lantiq_soc.h with lantiq.h
   serial: lantiq: Change init_lqasc to static declaration
   dt-bindings: serial: lantiq: Add optional properties for CCF

It appears that you only copied me on patches 1, 2 & 5. I've applied
patch 1 to mips-next for 4.20, but I have no clue whether your other
patches were deemed acceptable by serial or DT maintainers & I have no
context for the changes being made, so I can neither apply nor ack
patches 2 & 5. Please copy me on the whole series next time.

Thanks,
 Paul

Thanks.
I will resend the patches and cc all the patches to you.

Re: [PATCH 00/14] serial: langtiq: Add CCF suppport

[Wu, Songjun]

On 10/16/2018 5:58 AM, Paul Burton wrote:

Hi Songjun,

On Mon, Sep 24, 2018 at 06:27:49PM +0800, Songjun Wu wrote:

This patch series is for adding common clock framework support
for langtiq serial driver, mainly includes:

s/langtiq/lantiq/ ...

Thanks, it will be fixed.

1) Add common clock framework support.
2) Modify the dts file according to the DT conventions.
3) Replace the platform dependent functions with kernel functions

Songjun Wu (14):
   MIPS: dts: Change upper case to lower case
   MIPS: dts: Add aliases node for lantiq danube serial
   serial: lantiq: Get serial id from dts
   serial: lantiq: Change ltq_w32_mask to asc_update_bits
   MIPS: lantiq: Unselect SWAP_IO_SPACE when LANTIQ is selected
   serial: lantiq: Use readl/writel instead of ltq_r32/ltq_w32
   serial: lantiq: Rename fpiclk to freqclk
   serial: lantiq: Replace clk_enable/clk_disable with clk generic API
   serial: lantiq: Add CCF support
   serial: lantiq: Reorder the head files
   include: Add lantiq.h in include/linux/
   serial: lantiq: Replace lantiq_soc.h with lantiq.h
   serial: lantiq: Change init_lqasc to static declaration
   dt-bindings: serial: lantiq: Add optional properties for CCF

It appears that you only copied me on patches 1, 2 & 5. I've applied
patch 1 to mips-next for 4.20, but I have no clue whether your other
patches were deemed acceptable by serial or DT maintainers & I have no
context for the changes being made, so I can neither apply nor ack
patches 2 & 5. Please copy me on the whole series next time.

Thanks,
 Paul

Thanks.
I will resend the patches and cc all the patches to you.

Re: [PATCH v2 08/18] serial: intel: Get serial id from dts

[Wu, Songjun]

On 8/8/2018 4:33 PM, Geert Uytterhoeven wrote:

Hi Songjun,

On Wed, Aug 8, 2018 at 6:05 AM Wu, Songjun  wrote:

On 8/7/2018 3:33 PM, Geert Uytterhoeven wrote:

On Fri, Aug 3, 2018 at 5:04 AM Songjun Wu  wrote:

Get serial id from dts.

"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.

arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h

If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.

Signed-off-by: Songjun Wu 

Thanks for your patch!


@@ -699,9 +700,19 @@ lqasc_probe(struct platform_device *pdev)
  return -ENODEV;
  }

-   /* check if this is the console port */
-   if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
-   line = 1;
+   /* get serial id */
+   line = of_alias_get_id(node, "serial");
+   if (line < 0) {
+#ifdef CONFIG_LANTIQ
+   if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
+   line = 0;
+   else
+   line = 1;
+#else
+   dev_err(>dev, "failed to get alias id, errno %d\n", line);
+   return line;

Please note that not providing a fallback here makes life harder when using
DT overlays.
See the description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
for dynamic instances") for background info.

Thanks for your comment.
The logic in commit 7678f4c20fa7670f is not suitable here.
We need to know which serial instance is used for console.
We cannot use dynamic serial instance here.

Why does the driver need to use which serial instance is used for the console?
Hardcoding that is not an option, as the board DTS may specify the console using
chosen/stdout-path.

In legacy platform in open source, it only defined asc1 in dts.
There's no asc0 in legacy dts.While in the new platform, asc0
is defined in dts. There's no asc1 in new platform dts.
To avoid hard code in driver, alias serial0 is used to unified
driver code. Actually only one serial is supported in SoC.

aliases {
        serial0 = 
};

chosen {
    bootargs = "earlycon  clk_ignore_unused";
    stdout-path = "serial0";
};

Re: [PATCH v2 08/18] serial: intel: Get serial id from dts

[Wu, Songjun]

On 8/8/2018 4:33 PM, Geert Uytterhoeven wrote:

Hi Songjun,

On Wed, Aug 8, 2018 at 6:05 AM Wu, Songjun  wrote:

On 8/7/2018 3:33 PM, Geert Uytterhoeven wrote:

On Fri, Aug 3, 2018 at 5:04 AM Songjun Wu  wrote:

Get serial id from dts.

"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.

arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h

If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.

Signed-off-by: Songjun Wu 

Thanks for your patch!


@@ -699,9 +700,19 @@ lqasc_probe(struct platform_device *pdev)
  return -ENODEV;
  }

-   /* check if this is the console port */
-   if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
-   line = 1;
+   /* get serial id */
+   line = of_alias_get_id(node, "serial");
+   if (line < 0) {
+#ifdef CONFIG_LANTIQ
+   if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
+   line = 0;
+   else
+   line = 1;
+#else
+   dev_err(>dev, "failed to get alias id, errno %d\n", line);
+   return line;

Please note that not providing a fallback here makes life harder when using
DT overlays.
See the description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
for dynamic instances") for background info.

Thanks for your comment.
The logic in commit 7678f4c20fa7670f is not suitable here.
We need to know which serial instance is used for console.
We cannot use dynamic serial instance here.

Why does the driver need to use which serial instance is used for the console?
Hardcoding that is not an option, as the board DTS may specify the console using
chosen/stdout-path.

In legacy platform in open source, it only defined asc1 in dts.
There's no asc0 in legacy dts.While in the new platform, asc0
is defined in dts. There's no asc1 in new platform dts.
To avoid hard code in driver, alias serial0 is used to unified
driver code. Actually only one serial is supported in SoC.

aliases {
        serial0 = 
};

chosen {
    bootargs = "earlycon  clk_ignore_unused";
    stdout-path = "serial0";
};

Re: [PATCH v2 08/18] serial: intel: Get serial id from dts

[Wu, Songjun]

On 8/7/2018 3:33 PM, Geert Uytterhoeven wrote:

Hi Songjun,

On Fri, Aug 3, 2018 at 5:04 AM Songjun Wu  wrote:

Get serial id from dts.

"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.

arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h

If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.

Signed-off-by: Songjun Wu 

Thanks for your patch!


@@ -699,9 +700,19 @@ lqasc_probe(struct platform_device *pdev)
 return -ENODEV;
 }

-   /* check if this is the console port */
-   if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
-   line = 1;
+   /* get serial id */
+   line = of_alias_get_id(node, "serial");
+   if (line < 0) {
+#ifdef CONFIG_LANTIQ
+   if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
+   line = 0;
+   else
+   line = 1;
+#else
+   dev_err(>dev, "failed to get alias id, errno %d\n", line);
+   return line;

Please note that not providing a fallback here makes life harder when using
DT overlays.
See the description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
for dynamic instances") for background info.

Thanks for your comment.
The logic in commit 7678f4c20fa7670f is not suitable here.
We need to know which serial instance is used for console.
We cannot use dynamic serial instance here.

Re: [PATCH v2 08/18] serial: intel: Get serial id from dts

[Wu, Songjun]

On 8/7/2018 3:33 PM, Geert Uytterhoeven wrote:

Hi Songjun,

On Fri, Aug 3, 2018 at 5:04 AM Songjun Wu  wrote:

Get serial id from dts.

"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.

arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h

If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.

Signed-off-by: Songjun Wu 

Thanks for your patch!


@@ -699,9 +700,19 @@ lqasc_probe(struct platform_device *pdev)
 return -ENODEV;
 }

-   /* check if this is the console port */
-   if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
-   line = 1;
+   /* get serial id */
+   line = of_alias_get_id(node, "serial");
+   if (line < 0) {
+#ifdef CONFIG_LANTIQ
+   if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
+   line = 0;
+   else
+   line = 1;
+#else
+   dev_err(>dev, "failed to get alias id, errno %d\n", line);
+   return line;

Please note that not providing a fallback here makes life harder when using
DT overlays.
See the description of commit 7678f4c20fa7670f ("serial: sh-sci: Add support
for dynamic instances") for background info.

Thanks for your comment.
The logic in commit 7678f4c20fa7670f is not suitable here.
We need to know which serial instance is used for console.
We cannot use dynamic serial instance here.

Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Wu, Songjun]

On 8/6/2018 5:29 PM, Geert Uytterhoeven wrote:

Hi Songjun,

On Mon, Aug 6, 2018 at 10:58 AM Wu, Songjun  wrote:

On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote:

On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun  wrote:

On 8/5/2018 5:03 AM, Arnd Bergmann wrote:

On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
 wrote:

On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:

On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:

This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.

1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.

I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file.  Checkpatch might have even
warned you about it, right?

So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel.  You are not unique here.

The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.

Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.

>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as

   clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);

so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?

 Arnd

We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.

To support CCF in legacy product is another topic, is not included in
this patch.

The implementation is as following：
#ifdef CONFIG_LANTIQ
#include 
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0

static inline struct clk *clk_get_fpi(void)
{
   return NULL;
}
#endif

Why not use clkdev_add(), as Arnd suggested?
That would be a 3-line patch without introducing a new header file and an ugly
#ifdef, which complicates compile coverage testing?


The reason we add a new head file is also for two macros(LTQ_EARLY_ASC
and CPHYSADDR)
used by legacy product. We need to provide the no-op stub for these two
macro for new product.

No you don't. The line number should not be obtained by comparing the
resource address with a hardcoded base address.

This is the previous code. Now the line number is obtained from dts.
We keep this code for the compatibility.

Referring to the conditional-compilation part in coding-style,
We add a header file to avoid using “#ifdef” in C file.

Perhaps the override of port->line should just be removed, as IIRC, the serial
core has already filled in that field with the (next available) line number?

Gr{oetje,eeting}s,

 Geert

Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Wu, Songjun]

On 8/6/2018 5:29 PM, Geert Uytterhoeven wrote:

Hi Songjun,

On Mon, Aug 6, 2018 at 10:58 AM Wu, Songjun  wrote:

On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote:

On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun  wrote:

On 8/5/2018 5:03 AM, Arnd Bergmann wrote:

On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
 wrote:

On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:

On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:

This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.

1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.

I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file.  Checkpatch might have even
warned you about it, right?

So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel.  You are not unique here.

The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.

Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.

>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as

   clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);

so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?

 Arnd

We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.

To support CCF in legacy product is another topic, is not included in
this patch.

The implementation is as following：
#ifdef CONFIG_LANTIQ
#include 
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0

static inline struct clk *clk_get_fpi(void)
{
   return NULL;
}
#endif

Why not use clkdev_add(), as Arnd suggested?
That would be a 3-line patch without introducing a new header file and an ugly
#ifdef, which complicates compile coverage testing?


The reason we add a new head file is also for two macros(LTQ_EARLY_ASC
and CPHYSADDR)
used by legacy product. We need to provide the no-op stub for these two
macro for new product.

No you don't. The line number should not be obtained by comparing the
resource address with a hardcoded base address.

This is the previous code. Now the line number is obtained from dts.
We keep this code for the compatibility.

Referring to the conditional-compilation part in coding-style,
We add a header file to avoid using “#ifdef” in C file.

Perhaps the override of port->line should just be removed, as IIRC, the serial
core has already filled in that field with the (next available) line number?

Gr{oetje,eeting}s,

 Geert

Re: [PATCH v2 08/18] serial: intel: Get serial id from dts

[Wu, Songjun]

On 8/3/2018 1:43 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 11:02:27AM +0800, Songjun Wu wrote:

Get serial id from dts.

"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.

arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h

If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.

Signed-off-by: Songjun Wu 
---

Changes in v2: None

  drivers/tty/serial/lantiq.c | 19 +++
  1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 044128277248..836ca51460f2 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -6,6 +6,7 @@
   * Copyright (C) 2007 Felix Fietkau 
   * Copyright (C) 2007 John Crispin 
   * Copyright (C) 2010 Thomas Langer, 
+ * Copyright (C) 2018 Intel Corporation.

Your changes here do not warrent the addition of a copyright line, don't
you agree?  If not, please get a signed-off-by from your corporate
lawyer who does this this is warrented when you resend this patch.

thanks,

greg k-h


Thanks.
The copyright line will be removed when we resend this patch.

Re: [PATCH v2 08/18] serial: intel: Get serial id from dts

[Wu, Songjun]

On 8/3/2018 1:43 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 11:02:27AM +0800, Songjun Wu wrote:

Get serial id from dts.

"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.

arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h

If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.

Signed-off-by: Songjun Wu 
---

Changes in v2: None

  drivers/tty/serial/lantiq.c | 19 +++
  1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 044128277248..836ca51460f2 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -6,6 +6,7 @@
   * Copyright (C) 2007 Felix Fietkau 
   * Copyright (C) 2007 John Crispin 
   * Copyright (C) 2010 Thomas Langer, 
+ * Copyright (C) 2018 Intel Corporation.

Your changes here do not warrent the addition of a copyright line, don't
you agree?  If not, please get a signed-off-by from your corporate
lawyer who does this this is warrented when you resend this patch.

thanks,

greg k-h


Thanks.
The copyright line will be removed when we resend this patch.

Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Wu, Songjun]

On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote:

Hi Songjun,

On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun  wrote:

On 8/5/2018 5:03 AM, Arnd Bergmann wrote:

On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
 wrote:

On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:

On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:

This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.

1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.

I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file.  Checkpatch might have even
warned you about it, right?

So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel.  You are not unique here.

The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.

Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.

>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as

  clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);

so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?

Arnd

We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.

To support CCF in legacy product is another topic, is not included in
this patch.

The implementation is as following：
#ifdef CONFIG_LANTIQ
#include 
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0

static inline struct clk *clk_get_fpi(void)
{
  return NULL;
}
#endif

Why not use clkdev_add(), as Arnd suggested?
That would be a 3-line patch without introducing a new header file and an ugly
#ifdef, which complicates compile coverage testing?

Gr{oetje,eeting}s,

 Geert
The reason we add a new head file is also for two macros(LTQ_EARLY_ASC 
and CPHYSADDR)
used by legacy product. We need to provide the no-op stub for these two 
macro for new product.

Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Wu, Songjun]

On 8/6/2018 3:20 PM, Geert Uytterhoeven wrote:

Hi Songjun,

On Mon, Aug 6, 2018 at 9:15 AM Wu, Songjun  wrote:

On 8/5/2018 5:03 AM, Arnd Bergmann wrote:

On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
 wrote:

On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:

On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:

This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.

1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.

I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file.  Checkpatch might have even
warned you about it, right?

So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel.  You are not unique here.

The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.

Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.

>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as

  clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);

so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?

Arnd

We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in
lantiq.h,
also providing no-op stub functions in the #else case, then call those
functions
unconditionally from lantiq.c to avoid #ifdef in C file.

To support CCF in legacy product is another topic, is not included in
this patch.

The implementation is as following：
#ifdef CONFIG_LANTIQ
#include 
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0

static inline struct clk *clk_get_fpi(void)
{
  return NULL;
}
#endif

Why not use clkdev_add(), as Arnd suggested?
That would be a 3-line patch without introducing a new header file and an ugly
#ifdef, which complicates compile coverage testing?

Gr{oetje,eeting}s,

 Geert
The reason we add a new head file is also for two macros(LTQ_EARLY_ASC 
and CPHYSADDR)
used by legacy product. We need to provide the no-op stub for these two 
macro for new product.

Re: [PATCH v2 15/18] serial: intel: Support more platform

[Wu, Songjun]

On 8/5/2018 4:37 PM, Christoph Hellwig wrote:

The subject line also seems odd, your are changing deps on the lantiq
driver, not some (nonexistent) intel serial driver.


Your suggestion is reasonable, it will be changed to "serial: lantiq".

Re: [PATCH v2 15/18] serial: intel: Support more platform

[Wu, Songjun]

On 8/5/2018 4:37 PM, Christoph Hellwig wrote:

The subject line also seems odd, your are changing deps on the lantiq
driver, not some (nonexistent) intel serial driver.


Your suggestion is reasonable, it will be changed to "serial: lantiq".

Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Wu, Songjun]

On 8/5/2018 5:03 AM, Arnd Bergmann wrote:

On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
 wrote:

On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:

On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:

This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.

1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.

I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file.  Checkpatch might have even
warned you about it, right?

So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel.  You are not unique here.

The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.

Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.

>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as

 clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);

so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?

   Arnd

We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in 
lantiq.h,
also providing no-op stub functions in the #else case, then call those 
functions

unconditionally from lantiq.c to avoid #ifdef in C file.

To support CCF in legacy product is another topic, is not included in 
this patch.


The implementation is as following：
#ifdef CONFIG_LANTIQ
#include 
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0

static inline struct clk *clk_get_fpi(void)
{
    return NULL;
}
#endif

Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Wu, Songjun]

On 8/5/2018 5:03 AM, Arnd Bergmann wrote:

On Sat, Aug 4, 2018 at 2:43 PM, Greg Kroah-Hartman
 wrote:

On Sat, Aug 04, 2018 at 12:54:22PM +0200, Hauke Mehrtens wrote:

On 08/03/2018 12:30 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 03:33:38PM +0800, Wu, Songjun wrote:

This patch makes it possible to use it with the legacy lantiq code and
also with the common clock framework. I see multiple options to fix this
problem.

1. The current approach to have it as a compile variant for a) legacy
lantiq arch code without common clock framework and b) support for SoCs
using the common clock framework.
2. Convert the lantiq arch code to the common clock framework. This
would be a good approach, but it need some efforts.
3. Remove the arch/mips/lantiq code. There are still users of this code.
4. Use the old APIs also for the new xRX500 SoC, I do not like this
approach.
5. Move lantiq_soc.h to somewhere in include/linux/ so it is globally
available and provide some better wrapper code.

I don't really care what you do at this point in time, but you all
should know better than the crazy #ifdef is not allowed to try to
prevent/allow the inclusion of a .h file.  Checkpatch might have even
warned you about it, right?

So do it correctly, odds are #5 is correct, as that makes it work like
any other device in the kernel.  You are not unique here.

The best approach here would clearly be 2. We don't want platform
specific header files for doing things that should be completely generic.

Converting lantiq to the common-clk framework obviously requires
some work, but then again the whole arch/mips/lantiq/clk.c file
is fairly short and maybe not that hard to convert.

>From looking at arch/mips/lantiq/xway/sysctrl.c, it appears that you
already use the clkdev lookup mechanism for some devices without
using COMMON_CLK, so I would assume that you can also use those
for the remaining clks, which would be much simpler. It registers
one anonymous clk there as

 clkdev_add_pmu("1e100c00.serial", NULL, 0, 0, PMU_ASC1);

so why not add replace that with two named clocks and just use
the same names in the DT for the newer chip?

   Arnd

We discussed internally and have another solution for this issue.
Add one lantiq.h in the serial folder, and use "#ifdef preprocessor" in 
lantiq.h,
also providing no-op stub functions in the #else case, then call those 
functions

unconditionally from lantiq.c to avoid #ifdef in C file.

To support CCF in legacy product is another topic, is not included in 
this patch.


The implementation is as following：
#ifdef CONFIG_LANTIQ
#include 
#else
#define LTQ_EARLY_ASC 0
#define CPHYSADDR(_val) 0

static inline struct clk *clk_get_fpi(void)
{
    return NULL;
}
#endif

Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Wu, Songjun]

On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote:

Previous implementation uses platform-dependent API to get the clock.
Those functions are not available for other SoC which uses the same IP.
The CCF (Common Clock Framework) have an abstraction based APIs for
clock. In future, the platform specific code will be removed when the
legacy soc use CCF as well.
Change to use CCF APIs to get clock and rate. So that different SoCs
can use the same driver.

Signed-off-by: Songjun Wu 
---

Changes in v2: None

  drivers/tty/serial/lantiq.c | 11 +++
  1 file changed, 11 insertions(+)

diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 36479d66fb7c..35518ab3a80d 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -26,7 +26,9 @@
  #include 
  #include 
  
+#ifdef CONFIG_LANTIQ

  #include 
+#endif

That is never how you do this in Linux, you know better.

Please go and get this patchset reviewed and signed-off-by from other
internal Intel kernel developers before resending it next time.  It is
their job to find and fix your basic errors like this, not ours.

Thank you for your comment.
Actually, we have discussed this issue internally.
We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit
message in "[PATCH v2 08/18] serial: intel: Get serial id from dts".
Please refer the commit message below.

"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.

arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h

If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.

Re: [PATCH v2 14/18] serial: intel: Add CCF support

[Wu, Songjun]

On 8/3/2018 1:56 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 11:02:33AM +0800, Songjun Wu wrote:

Previous implementation uses platform-dependent API to get the clock.
Those functions are not available for other SoC which uses the same IP.
The CCF (Common Clock Framework) have an abstraction based APIs for
clock. In future, the platform specific code will be removed when the
legacy soc use CCF as well.
Change to use CCF APIs to get clock and rate. So that different SoCs
can use the same driver.

Signed-off-by: Songjun Wu 
---

Changes in v2: None

  drivers/tty/serial/lantiq.c | 11 +++
  1 file changed, 11 insertions(+)

diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 36479d66fb7c..35518ab3a80d 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -26,7 +26,9 @@
  #include 
  #include 
  
+#ifdef CONFIG_LANTIQ

  #include 
+#endif

That is never how you do this in Linux, you know better.

Please go and get this patchset reviewed and signed-off-by from other
internal Intel kernel developers before resending it next time.  It is
their job to find and fix your basic errors like this, not ours.

Thank you for your comment.
Actually, we have discussed this issue internally.
We put the reason why we use "#ifdef CONFIG_LANTIQ" preprocessor in commit
message in "[PATCH v2 08/18] serial: intel: Get serial id from dts".
Please refer the commit message below.

"#ifdef CONFIG_LANTIQ" preprocessor is used because LTQ_EARLY_ASC
macro is defined in lantiq_soc.h.
lantiq_soc.h is in arch path for legacy product support.

arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h

If "#ifdef preprocessor" is changed to
"if (IS_ENABLED(CONFIG_LANTIQ))", when CONFIG_LANTIQ is not enabled,
code using LTQ_EARLY_ASC is compiled.
Compilation will fail for no LTQ_EARLY_ASC defined.

Re: [PATCH v2 15/18] serial: intel: Support more platform

[Wu, Songjun]

On 8/3/2018 1:57 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 11:02:34AM +0800, Songjun Wu wrote:

Support more platform.

Signed-off-by: Songjun Wu 
---

Your changelog text makes no sense, sorry.

Thanks for your comment.
I will describe it more clearly.

Re: [PATCH v2 15/18] serial: intel: Support more platform

[Wu, Songjun]

On 8/3/2018 1:57 PM, Greg Kroah-Hartman wrote:

On Fri, Aug 03, 2018 at 11:02:34AM +0800, Songjun Wu wrote:

Support more platform.

Signed-off-by: Songjun Wu 
---

Your changelog text makes no sense, sorry.

Thanks for your comment.
I will describe it more clearly.

Re: [PATCH 1/7] MIPS: dts: Add aliases node for lantiq danube serial

[Wu, Songjun]

On 6/18/2018 6:59 PM, Arnd Bergmann wrote:

On Mon, Jun 18, 2018 at 11:42 AM, Wu, Songjun
 wrote:

On 6/14/2018 6:03 PM, Arnd Bergmann wrote:

On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu 
wrote:

Previous implementation uses a hard-coded register value to check if
the current serial entity is the console entity.
Now the lantiq serial driver uses the aliases for the index of the
serial port.
The lantiq danube serial dts are updated with aliases to support this.

Signed-off-by: Songjun Wu 
---

   arch/mips/boot/dts/lantiq/danube.dtsi | 6 +-
   1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/lantiq/danube.dtsi
b/arch/mips/boot/dts/lantiq/danube.dtsi
index 2dd950181f8a..7a9e15da6bd0 100644
--- a/arch/mips/boot/dts/lantiq/danube.dtsi
+++ b/arch/mips/boot/dts/lantiq/danube.dtsi
@@ -4,6 +4,10 @@
  #size-cells = <1>;
  compatible = "lantiq,xway", "lantiq,danube";

+   aliases {
+   serial0 = 
+   };
+

You generally want the aliases to be part of the board specific file,
not every board numbers their serial ports in the same way.


In this chip only asc1 can be used as console,  so serial0 is defined in
chip specific file.

This was a more general comment about 'aliases' being board specific
in principle (though we've had exceptions in the past). Even if there
is only one uart on the chip, I'd recommend following the same
conventions as the other chips that have more than one uart.

Arnd

Accept, 'aliases' will be move to the board specific file.

Re: [PATCH 1/7] MIPS: dts: Add aliases node for lantiq danube serial

[Wu, Songjun]

On 6/18/2018 6:59 PM, Arnd Bergmann wrote:

On Mon, Jun 18, 2018 at 11:42 AM, Wu, Songjun
 wrote:

On 6/14/2018 6:03 PM, Arnd Bergmann wrote:

On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu 
wrote:

Previous implementation uses a hard-coded register value to check if
the current serial entity is the console entity.
Now the lantiq serial driver uses the aliases for the index of the
serial port.
The lantiq danube serial dts are updated with aliases to support this.

Signed-off-by: Songjun Wu 
---

   arch/mips/boot/dts/lantiq/danube.dtsi | 6 +-
   1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/lantiq/danube.dtsi
b/arch/mips/boot/dts/lantiq/danube.dtsi
index 2dd950181f8a..7a9e15da6bd0 100644
--- a/arch/mips/boot/dts/lantiq/danube.dtsi
+++ b/arch/mips/boot/dts/lantiq/danube.dtsi
@@ -4,6 +4,10 @@
  #size-cells = <1>;
  compatible = "lantiq,xway", "lantiq,danube";

+   aliases {
+   serial0 = 
+   };
+

You generally want the aliases to be part of the board specific file,
not every board numbers their serial ports in the same way.


In this chip only asc1 can be used as console,  so serial0 is defined in
chip specific file.

This was a more general comment about 'aliases' being board specific
in principle (though we've had exceptions in the past). Even if there
is only one uart on the chip, I'd recommend following the same
conventions as the other chips that have more than one uart.

Arnd

Accept, 'aliases' will be move to the board specific file.

Re: [PATCH 1/7] MIPS: dts: Add aliases node for lantiq danube serial

[Wu, Songjun]

On 6/14/2018 6:03 PM, Arnd Bergmann wrote:

On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu  wrote:

Previous implementation uses a hard-coded register value to check if
the current serial entity is the console entity.
Now the lantiq serial driver uses the aliases for the index of the
serial port.
The lantiq danube serial dts are updated with aliases to support this.

Signed-off-by: Songjun Wu 
---

  arch/mips/boot/dts/lantiq/danube.dtsi | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/lantiq/danube.dtsi 
b/arch/mips/boot/dts/lantiq/danube.dtsi
index 2dd950181f8a..7a9e15da6bd0 100644
--- a/arch/mips/boot/dts/lantiq/danube.dtsi
+++ b/arch/mips/boot/dts/lantiq/danube.dtsi
@@ -4,6 +4,10 @@
 #size-cells = <1>;
 compatible = "lantiq,xway", "lantiq,danube";

+   aliases {
+   serial0 = 
+   };
+

You generally want the aliases to be part of the board specific file,
not every board numbers their serial ports in the same way.

Arnd
In this chip only asc1 can be used as console,  so serial0 is defined in 
chip specific file.

Re: [PATCH 1/7] MIPS: dts: Add aliases node for lantiq danube serial

[Wu, Songjun]

On 6/14/2018 6:03 PM, Arnd Bergmann wrote:

On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu  wrote:

Previous implementation uses a hard-coded register value to check if
the current serial entity is the console entity.
Now the lantiq serial driver uses the aliases for the index of the
serial port.
The lantiq danube serial dts are updated with aliases to support this.

Signed-off-by: Songjun Wu 
---

  arch/mips/boot/dts/lantiq/danube.dtsi | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/lantiq/danube.dtsi 
b/arch/mips/boot/dts/lantiq/danube.dtsi
index 2dd950181f8a..7a9e15da6bd0 100644
--- a/arch/mips/boot/dts/lantiq/danube.dtsi
+++ b/arch/mips/boot/dts/lantiq/danube.dtsi
@@ -4,6 +4,10 @@
 #size-cells = <1>;
 compatible = "lantiq,xway", "lantiq,danube";

+   aliases {
+   serial0 = 
+   };
+

You generally want the aliases to be part of the board specific file,
not every board numbers their serial ports in the same way.

Arnd
In this chip only asc1 can be used as console,  so serial0 is defined in 
chip specific file.

Re: [PATCH 4/7] tty: serial: lantiq: Always use readl()/writel()

[Wu, Songjun]

On 6/14/2018 6:07 PM, Arnd Bergmann wrote:

On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu  wrote:

Previous implementation uses platform-dependent functions
ltq_w32()/ltq_r32() to access registers. Those functions are not
available for other SoC which uses the same IP.
Change to OS provided readl()/writel() and readb()/writeb(), so
that different SoCs can use the same driver.

Signed-off-by: Songjun Wu 

Are there any big-endian machines using this driver? The original definition
of ltq_r32() uses non-byteswapping __raw_readl() etc, which suggests
that the registers might be wired up in a way that matches the CPU
endianess (this is usally a bad idea in hardware design, but nothing
we can influence in the OS).

When you change it to readl(), that will breaks all machines that rely
on the old behavior on big-endian kernels.

   Arnd

It will not break existing big-endian SoC as SWAP_IO_SPACE is disabled.

Disable SWAP_IO_SPACE will not impact ltq_r32 as it uses non-byte 
swapping __raw_readl() and it makes readl work in big-endian kernel too.


The old Lantiq platform enable SWAP_IO_SPACE to support PCI as it's a 
little-endian bus plus PCI TX/RX swap enable impacted both data and 
control path.
Alternatively PCI device driver has to do endian swap, It is better to 
let PCI device driver to do endian swap instead because SWAP_IO_SPACE is 
global wide macro.
Once we set it, other peripheral such as USB has to change its register 
access as well

Re: [PATCH 4/7] tty: serial: lantiq: Always use readl()/writel()

[Wu, Songjun]

On 6/14/2018 6:07 PM, Arnd Bergmann wrote:

On Tue, Jun 12, 2018 at 7:40 AM, Songjun Wu  wrote:

Previous implementation uses platform-dependent functions
ltq_w32()/ltq_r32() to access registers. Those functions are not
available for other SoC which uses the same IP.
Change to OS provided readl()/writel() and readb()/writeb(), so
that different SoCs can use the same driver.

Signed-off-by: Songjun Wu 

Are there any big-endian machines using this driver? The original definition
of ltq_r32() uses non-byteswapping __raw_readl() etc, which suggests
that the registers might be wired up in a way that matches the CPU
endianess (this is usally a bad idea in hardware design, but nothing
we can influence in the OS).

When you change it to readl(), that will breaks all machines that rely
on the old behavior on big-endian kernels.

   Arnd

It will not break existing big-endian SoC as SWAP_IO_SPACE is disabled.

Disable SWAP_IO_SPACE will not impact ltq_r32 as it uses non-byte 
swapping __raw_readl() and it makes readl work in big-endian kernel too.


The old Lantiq platform enable SWAP_IO_SPACE to support PCI as it's a 
little-endian bus plus PCI TX/RX swap enable impacted both data and 
control path.
Alternatively PCI device driver has to do endian swap, It is better to 
let PCI device driver to do endian swap instead because SWAP_IO_SPACE is 
global wide macro.
Once we set it, other peripheral such as USB has to change its register 
access as well

Re: [PATCH 4/7] tty: serial: lantiq: Always use readl()/writel()

[Wu, Songjun]

On 6/12/2018 4:13 PM, Andy Shevchenko wrote:

On Tue, Jun 12, 2018 at 8:40 AM, Songjun Wu  wrote:

Previous implementation uses platform-dependent functions
ltq_w32()/ltq_r32() to access registers. Those functions are not
available for other SoC which uses the same IP.
Change to OS provided readl()/writel() and readb()/writeb(), so
that different SoCs can use the same driver.

This patch consists 2 or even 3 ones:
- whitespace shuffling (indentation fixes, blank lines), I dunno if
it's needed at all
- some new registers / bits
- actual switch to readl() / writel()

Please, split.
It will be split to four patches, coding style, new registers, 
readl()/writel() and asc_update_bits.

+#define asc_w32_mask(clear, set, reg)  \
+   ({ typeof(reg) reg_ = (reg);\
+   writel((readl(reg_) & ~(clear)) | (set), reg_); })

This would be better as a static inline helper, and name is completely
misleading, it doesn't mask the register bits, it _updates_ them.

It will be changed to asc_update_bits.

Re: [PATCH 4/7] tty: serial: lantiq: Always use readl()/writel()

[Wu, Songjun]

On 6/12/2018 4:13 PM, Andy Shevchenko wrote:

On Tue, Jun 12, 2018 at 8:40 AM, Songjun Wu  wrote:

Previous implementation uses platform-dependent functions
ltq_w32()/ltq_r32() to access registers. Those functions are not
available for other SoC which uses the same IP.
Change to OS provided readl()/writel() and readb()/writeb(), so
that different SoCs can use the same driver.

This patch consists 2 or even 3 ones:
- whitespace shuffling (indentation fixes, blank lines), I dunno if
it's needed at all
- some new registers / bits
- actual switch to readl() / writel()

Please, split.
It will be split to four patches, coding style, new registers, 
readl()/writel() and asc_update_bits.

+#define asc_w32_mask(clear, set, reg)  \
+   ({ typeof(reg) reg_ = (reg);\
+   writel((readl(reg_) & ~(clear)) | (set), reg_); })

This would be better as a static inline helper, and name is completely
misleading, it doesn't mask the register bits, it _updates_ them.

It will be changed to asc_update_bits.

Re: [PATCH 7/7] tty: serial: lantiq: Add CCF support

[Wu, Songjun]

On 6/13/2018 6:39 AM, Rob Herring wrote:

On Tue, Jun 12, 2018 at 01:40:34PM +0800, Songjun Wu wrote:

Previous implementation uses platform-dependent API to get the clock.
Those functions are not available for other SoC which uses the same IP.
The CCF (Common Clock Framework) have an abstraction based APIs
for clock.
Change to use CCF APIs to get clock and rate.
So that different SoCs can use the same driver.
Clocks and clock-names are updated in device tree binding.

Signed-off-by: Songjun Wu 

---

  .../devicetree/bindings/serial/lantiq_asc.txt  |  15 +++

Please split bindings to separate patch.

Thanks.
It will be split to two separate patches, one for bindings, the other 
for code.

  drivers/tty/serial/Kconfig |   2 +-
  drivers/tty/serial/lantiq.c| 101 +
  3 files changed, 98 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/lantiq_asc.txt 
b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
index 3acbd309ab9d..608f0c87a4af 100644
--- a/Documentation/devicetree/bindings/serial/lantiq_asc.txt
+++ b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
@@ -6,6 +6,10 @@ Required properties:
  - interrupts: the 3 (tx rx err) interrupt numbers. The interrupt specifier
depends on the interrupt-parent interrupt controller.
  
+Optional properties:

+- clocks: Should contain frequency clock and gate clock
+- clock-names: Should be "freq" and "asc"
+
  Example:
  
  asc1: serial@e100c00 {

@@ -14,3 +18,14 @@ asc1: serial@e100c00 {
interrupt-parent = <>;
interrupts = <112 113 114>;
  };
+
+asc0: serial@60 {
+   compatible = "lantiq,asc";
+   reg = <0x60 0x10>;

1MB of address space? That wastes a lot of virtual space on 32-bit
systems. Just make the size the actual used range.

The size of address space will be updated to the actual used range.

+   interrupt-parent = <>;
+   interrupts = ,
+   ,
+   ;
+   clocks = < SSX4_CLK>, < GATE_URT_CLK>;
+   clock-names = "freq", "asc";
+};

Re: [PATCH 7/7] tty: serial: lantiq: Add CCF support

[Wu, Songjun]

On 6/13/2018 6:39 AM, Rob Herring wrote:

On Tue, Jun 12, 2018 at 01:40:34PM +0800, Songjun Wu wrote:

Previous implementation uses platform-dependent API to get the clock.
Those functions are not available for other SoC which uses the same IP.
The CCF (Common Clock Framework) have an abstraction based APIs
for clock.
Change to use CCF APIs to get clock and rate.
So that different SoCs can use the same driver.
Clocks and clock-names are updated in device tree binding.

Signed-off-by: Songjun Wu 

---

  .../devicetree/bindings/serial/lantiq_asc.txt  |  15 +++

Please split bindings to separate patch.

Thanks.
It will be split to two separate patches, one for bindings, the other 
for code.

  drivers/tty/serial/Kconfig |   2 +-
  drivers/tty/serial/lantiq.c| 101 +
  3 files changed, 98 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/lantiq_asc.txt 
b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
index 3acbd309ab9d..608f0c87a4af 100644
--- a/Documentation/devicetree/bindings/serial/lantiq_asc.txt
+++ b/Documentation/devicetree/bindings/serial/lantiq_asc.txt
@@ -6,6 +6,10 @@ Required properties:
  - interrupts: the 3 (tx rx err) interrupt numbers. The interrupt specifier
depends on the interrupt-parent interrupt controller.
  
+Optional properties:

+- clocks: Should contain frequency clock and gate clock
+- clock-names: Should be "freq" and "asc"
+
  Example:
  
  asc1: serial@e100c00 {

@@ -14,3 +18,14 @@ asc1: serial@e100c00 {
interrupt-parent = <>;
interrupts = <112 113 114>;
  };
+
+asc0: serial@60 {
+   compatible = "lantiq,asc";
+   reg = <0x60 0x10>;

1MB of address space? That wastes a lot of virtual space on 32-bit
systems. Just make the size the actual used range.

The size of address space will be updated to the actual used range.

+   interrupt-parent = <>;
+   interrupts = ,
+   ,
+   ;
+   clocks = < SSX4_CLK>, < GATE_URT_CLK>;
+   clock-names = "freq", "asc";
+};

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

On 3/13/2017 17:25, Hans Verkuil wrote:

On 03/13/2017 06:53 AM, Wu, Songjun wrote:



On 3/9/2017 18:57, Hans Verkuil wrote:

Hi Songjun,

On 08/03/17 03:25, Wu, Songjun wrote:

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.


Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.


Hi Hans,

I see this patch is merged in git://linuxtv.org/media_tree.git.
So I do not need submit isc-pipeline-v3 patch, just submit the patches,
based on the current master branch?


Huh? Where do you see that this patch is merged? I don't see it in the 
media_tree master
branch.


Hi Hans,

I see this patch on the master branch in media_tree.
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/atmel/atmel-isc.c


Regards,

Hans




Regards,

Hans



On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
 regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

 *hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
 *hist_count += i * (*hist_entry++);
 }

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

On 3/13/2017 17:25, Hans Verkuil wrote:

On 03/13/2017 06:53 AM, Wu, Songjun wrote:



On 3/9/2017 18:57, Hans Verkuil wrote:

Hi Songjun,

On 08/03/17 03:25, Wu, Songjun wrote:

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.


Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.


Hi Hans,

I see this patch is merged in git://linuxtv.org/media_tree.git.
So I do not need submit isc-pipeline-v3 patch, just submit the patches,
based on the current master branch?


Huh? Where do you see that this patch is merged? I don't see it in the 
media_tree master
branch.


Hi Hans,

I see this patch on the master branch in media_tree.
https://git.linuxtv.org/media_tree.git/tree/drivers/media/platform/atmel/atmel-isc.c


Regards,

Hans




Regards,

Hans



On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King 

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King 
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
 regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

 *hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
 *hist_count += i * (*hist_entry++);
 }

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

On 3/9/2017 18:57, Hans Verkuil wrote:

Hi Songjun,

On 08/03/17 03:25, Wu, Songjun wrote:

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.


Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.


Hi Hans,

I see this patch is merged in git://linuxtv.org/media_tree.git.
So I do not need submit isc-pipeline-v3 patch, just submit the patches, 
based on the current master branch?



Regards,

Hans



On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
 regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

 *hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
 *hist_count += i * (*hist_entry++);
 }

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

On 3/9/2017 18:57, Hans Verkuil wrote:

Hi Songjun,

On 08/03/17 03:25, Wu, Songjun wrote:

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.


Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.


Hi Hans,

I see this patch is merged in git://linuxtv.org/media_tree.git.
So I do not need submit isc-pipeline-v3 patch, just submit the patches, 
based on the current master branch?



Regards,

Hans



On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King 

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King 
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
 regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

 *hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
 *hist_count += i * (*hist_entry++);
 }

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

On 3/9/2017 19:50, Colin Ian King wrote:

On 09/03/17 11:49, walter harms wrote:



Am 09.03.2017 11:57, schrieb Hans Verkuil:

Hi Songjun,

On 08/03/17 03:25, Wu, Songjun wrote:

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.


Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.

Regards,

Hans





perhaps he will make it a bit more readable, like:

*hist_count += i * (*hist_entry++);

*hist_count += hist_entry[i]*i;


As long as it gets fixed somehow, then I'm happy.


You suggestion is very good, I will modify it like this.
Thank you.


Colin



re,
 wh


On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King <colin.k...@canonical.com>

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
 regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

 *hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
 *hist_count += i * (*hist_entry++);
 }










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

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

On 3/9/2017 19:50, Colin Ian King wrote:

On 09/03/17 11:49, walter harms wrote:



Am 09.03.2017 11:57, schrieb Hans Verkuil:

Hi Songjun,

On 08/03/17 03:25, Wu, Songjun wrote:

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.


Do you mean that you will provide a new patch for this? Is there anything
wrong with this patch? It seems reasonable to me.

Regards,

Hans





perhaps he will make it a bit more readable, like:

*hist_count += i * (*hist_entry++);

*hist_count += hist_entry[i]*i;


As long as it gets fixed somehow, then I'm happy.


You suggestion is very good, I will modify it like this.
Thank you.


Colin



re,
 wh


On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King 

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King 
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
 regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

 *hist_count = 0;
-for (i = 0; i <= HIST_ENTRIES; i++)
+for (i = 0; i < HIST_ENTRIES; i++)
 *hist_count += i * (*hist_entry++);
 }










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

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.

On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King 

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King 
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

*hist_count = 0;
-   for (i = 0; i <= HIST_ENTRIES; i++)
+   for (i = 0; i < HIST_ENTRIES; i++)
*hist_count += i * (*hist_entry++);
 }

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[Wu, Songjun]

Hi Colin,

Thank you for your comment.
It is a bug, will be fixed in the next patch.

On 3/7/2017 22:30, Colin King wrote:

From: Colin Ian King 

The are only HIST_ENTRIES worth of entries in  hist_entry however the
for-loop is iterating one too many times leasing to a read access off
the end off the array ctrls->hist_entry.  Fix this by iterating by
the correct number of times.

Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")

Signed-off-by: Colin Ian King 
---
 drivers/media/platform/atmel/atmel-isc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index b380a7d..7dacf8c 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);

*hist_count = 0;
-   for (i = 0; i <= HIST_ENTRIES; i++)
+   for (i = 0; i < HIST_ENTRIES; i++)
*hist_count += i * (*hist_entry++);
 }

Re: [PATCH] [media] atmel-isc: add the isc pipeline function

[Wu, Songjun]

Hi Hans,

Thank you for your comments.

On 1/9/2017 20:10, Hans Verkuil wrote:

+
+static int isc_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct isc_device *isc = container_of(ctrl->handler,
+struct isc_device, ctrls.handler);
+   struct isc_ctrls *ctrls = >ctrls;
+
+   switch (ctrl->id) {
+   case V4L2_CID_BRIGHTNESS:
+   ctrls->brightness = ctrl->val & ISC_CBC_BRIGHT_MASK;
+   break;
+   case V4L2_CID_CONTRAST:
+   ctrls->contrast = (ctrl->val << 8) & ISC_CBC_CONTRAST_MASK;

As I understand it only bits 11-8 contain the contrast in the register?

Wouldn't '(ctrl->val & ISC_CBC_CONTRAST_MASK) << 8' be more readable?

Either that or the mask should be 0xf00, not 0xfff.

Actually, bits 12-0 contain the contrast, it is a fixed-point 
number(signed 12 bits 1:3:8), ranges from -2048 to 2047. Then only the 
integral part is output to be adjusted.

Maybe both the fractional part and integral part should be output.
Then the contrast control should be written as below.

v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -2048, 2047, 1, 1);

Do you have any good suggestion?


+   break;
+   case V4L2_CID_GAMMA:
+   ctrls->gamma_index = ctrl->val;
+   break;
+   case V4L2_CID_AUTO_WHITE_BALANCE:
+   ctrls->awb = ctrl->val;
+   if (ctrls->hist_stat != HIST_ENABLED) {
+   ctrls->r_gain = 0x1 << 9;
+   ctrls->b_gain = 0x1 << 9;
+   }
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
+static const struct v4l2_ctrl_ops isc_ctrl_ops = {
+   .s_ctrl = isc_s_ctrl,
+};
+
+static int isc_ctrl_init(struct isc_device *isc)
+{
+   const struct v4l2_ctrl_ops *ops = _ctrl_ops;
+   struct isc_ctrls *ctrls = >ctrls;
+   struct v4l2_ctrl_handler *hdl = >handler;
+   int ret;
+
+   ctrls->hist_stat = HIST_INIT;
+
+   ret = v4l2_ctrl_handler_init(hdl, 4);
+   if (ret < 0)
+   return ret;
+
+   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -1024, 1023, 1, 0);
+   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -8, 7, 1, 1);
+   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAMMA, 0, GAMMA_MAX - 1, 1, 2);

Why is the maximum GAMMA_MAX - 1? I would assume that GAMMA_MAX is the maximum.

Looks weird. It's either a bug or it needs a comment.


+   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
+
+   v4l2_ctrl_handler_setup(hdl);
+
+   return 0;
+}
+
+
 static int isc_async_bound(struct v4l2_async_notifier *notifier,
struct v4l2_subdev *subdev,
struct v4l2_async_subdev *asd)
@@ -1047,10 +1435,11 @@ static void isc_async_unbind(struct v4l2_async_notifier 
*notifier,
 {
struct isc_device *isc = container_of(notifier->v4l2_dev,
  struct isc_device, v4l2_dev);
-
+   cancel_work_sync(>awb_work);
video_unregister_device(>video_dev);
if (isc->current_subdev->config)
v4l2_subdev_free_pad_config(isc->current_subdev->config);
+   v4l2_ctrl_handler_free(>ctrls.handler);
 }

 static struct isc_format *find_format_by_code(unsigned int code, int *index)
@@ -1081,7 +1470,9 @@ static int isc_formats_init(struct isc_device *isc)

fmt = _formats[0];
for (i = 0; i < ARRAY_SIZE(isc_formats); i++) {
-   fmt->support = false;
+   fmt->isc_support = false;
+   fmt->sd_support = false;
+
fmt++;
}

@@ -1092,8 +1483,22 @@ static int isc_formats_init(struct isc_device *isc)
if (!fmt)
continue;

-   fmt->support = true;
-   num_fmts++;
+   fmt->sd_support = true;
+
+   if (i <= RAW_FMT_INDEX_END) {
+   for (j = ISC_FMT_INDEX_START;
+j <= ISC_FMT_INDEX_END; j++)

Just merge these two lines, easier to read.

Do you mean merge these two lines like 'for (j = ISC_FMT_INDEX_START; j 
<= ISC_FMT_INDEX_END; j++)', but the line is over 80 characters



+   isc_formats[j].isc_support = true;
+
+   isc->raw_fmt = fmt;
+   }

Re: [PATCH] [media] atmel-isc: add the isc pipeline function

[Wu, Songjun]

Hi Hans,

Thank you for your comments.

On 1/9/2017 20:10, Hans Verkuil wrote:

+
+static int isc_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+   struct isc_device *isc = container_of(ctrl->handler,
+struct isc_device, ctrls.handler);
+   struct isc_ctrls *ctrls = >ctrls;
+
+   switch (ctrl->id) {
+   case V4L2_CID_BRIGHTNESS:
+   ctrls->brightness = ctrl->val & ISC_CBC_BRIGHT_MASK;
+   break;
+   case V4L2_CID_CONTRAST:
+   ctrls->contrast = (ctrl->val << 8) & ISC_CBC_CONTRAST_MASK;

As I understand it only bits 11-8 contain the contrast in the register?

Wouldn't '(ctrl->val & ISC_CBC_CONTRAST_MASK) << 8' be more readable?

Either that or the mask should be 0xf00, not 0xfff.

Actually, bits 12-0 contain the contrast, it is a fixed-point 
number(signed 12 bits 1:3:8), ranges from -2048 to 2047. Then only the 
integral part is output to be adjusted.

Maybe both the fractional part and integral part should be output.
Then the contrast control should be written as below.

v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -2048, 2047, 1, 1);

Do you have any good suggestion?


+   break;
+   case V4L2_CID_GAMMA:
+   ctrls->gamma_index = ctrl->val;
+   break;
+   case V4L2_CID_AUTO_WHITE_BALANCE:
+   ctrls->awb = ctrl->val;
+   if (ctrls->hist_stat != HIST_ENABLED) {
+   ctrls->r_gain = 0x1 << 9;
+   ctrls->b_gain = 0x1 << 9;
+   }
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
+static const struct v4l2_ctrl_ops isc_ctrl_ops = {
+   .s_ctrl = isc_s_ctrl,
+};
+
+static int isc_ctrl_init(struct isc_device *isc)
+{
+   const struct v4l2_ctrl_ops *ops = _ctrl_ops;
+   struct isc_ctrls *ctrls = >ctrls;
+   struct v4l2_ctrl_handler *hdl = >handler;
+   int ret;
+
+   ctrls->hist_stat = HIST_INIT;
+
+   ret = v4l2_ctrl_handler_init(hdl, 4);
+   if (ret < 0)
+   return ret;
+
+   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -1024, 1023, 1, 0);
+   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -8, 7, 1, 1);
+   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAMMA, 0, GAMMA_MAX - 1, 1, 2);

Why is the maximum GAMMA_MAX - 1? I would assume that GAMMA_MAX is the maximum.

Looks weird. It's either a bug or it needs a comment.


+   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
+
+   v4l2_ctrl_handler_setup(hdl);
+
+   return 0;
+}
+
+
 static int isc_async_bound(struct v4l2_async_notifier *notifier,
struct v4l2_subdev *subdev,
struct v4l2_async_subdev *asd)
@@ -1047,10 +1435,11 @@ static void isc_async_unbind(struct v4l2_async_notifier 
*notifier,
 {
struct isc_device *isc = container_of(notifier->v4l2_dev,
  struct isc_device, v4l2_dev);
-
+   cancel_work_sync(>awb_work);
video_unregister_device(>video_dev);
if (isc->current_subdev->config)
v4l2_subdev_free_pad_config(isc->current_subdev->config);
+   v4l2_ctrl_handler_free(>ctrls.handler);
 }

 static struct isc_format *find_format_by_code(unsigned int code, int *index)
@@ -1081,7 +1470,9 @@ static int isc_formats_init(struct isc_device *isc)

fmt = _formats[0];
for (i = 0; i < ARRAY_SIZE(isc_formats); i++) {
-   fmt->support = false;
+   fmt->isc_support = false;
+   fmt->sd_support = false;
+
fmt++;
}

@@ -1092,8 +1483,22 @@ static int isc_formats_init(struct isc_device *isc)
if (!fmt)
continue;

-   fmt->support = true;
-   num_fmts++;
+   fmt->sd_support = true;
+
+   if (i <= RAW_FMT_INDEX_END) {
+   for (j = ISC_FMT_INDEX_START;
+j <= ISC_FMT_INDEX_END; j++)

Just merge these two lines, easier to read.

Do you mean merge these two lines like 'for (j = ISC_FMT_INDEX_START; j 
<= ISC_FMT_INDEX_END; j++)', but the line is over 80 characters



+   isc_formats[j].isc_support = true;
+
+   isc->raw_fmt = fmt;
+   }

Re: [PATCH] kobject: set state_initialized to 0 in kobject_cleanup

[Wu, Songjun]

On 11/1/2016 22:56, Greg KH wrote:

On Tue, Nov 01, 2016 at 06:41:44PM +0800, Songjun Wu wrote:

If state_initialized is not set to 0 when a kobject is
released, a device is registered, unregistered, and
registered again, the error below will occur.

kobject (dec04bb0): tried to init an initialized object,
something is seriously wrong.


Yes, your code is wrong, don't try to change the kernel core to work
around it :)

That message is there for a reason, and this patch has been rejected
many times in the past.  kobjects can NOT ever be reused, and should
never be static (but yes, there are lots of in-kernel users with static
kobjects, they just never get reused...)

What code is emitting this message?  I'll be glad to help you fix it up
if you can point me at it.


Thank you very much.
I will not use the static kobjects to fix the error.
:)

thanks,

greg k-h

Re: [PATCH] kobject: set state_initialized to 0 in kobject_cleanup

[Wu, Songjun]

On 11/1/2016 22:56, Greg KH wrote:

On Tue, Nov 01, 2016 at 06:41:44PM +0800, Songjun Wu wrote:

If state_initialized is not set to 0 when a kobject is
released, a device is registered, unregistered, and
registered again, the error below will occur.

kobject (dec04bb0): tried to init an initialized object,
something is seriously wrong.


Yes, your code is wrong, don't try to change the kernel core to work
around it :)

That message is there for a reason, and this patch has been rejected
many times in the past.  kobjects can NOT ever be reused, and should
never be static (but yes, there are lots of in-kernel users with static
kobjects, they just never get reused...)

What code is emitting this message?  I'll be glad to help you fix it up
if you can point me at it.


Thank you very much.
I will not use the static kobjects to fix the error.
:)

thanks,

greg k-h

Re: [PATCH] [media] atmel-isc: release the filehandle if it's not the only one.

[Wu, Songjun]

Sorry, my mistake, the device should be able to opened multiple times.
It's a wrong patch.

On 11/1/2016 16:52, Hans Verkuil wrote:

On 01/11/16 09:08, Songjun Wu wrote:

Release the filehandle in 'isc_open' if it's not the only filehandle
opened for the associated video_device.


What's wrong with that? You should always be able to open the device
multiple times. v4l2-compliance will fail after this patch. I'm not sure
what you intended to do here, but this patch is wrong.

Regards,

Hans



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

 drivers/media/platform/atmel/atmel-isc.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index 8e25d3f..5e08404 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -926,21 +926,21 @@ static int isc_open(struct file *file)
 if (ret < 0)
 goto unlock;

-if (!v4l2_fh_is_singular_file(file))
-goto unlock;
+ret = !v4l2_fh_is_singular_file(file);
+if (ret)
+goto fh_rel;

 ret = v4l2_subdev_call(sd, core, s_power, 1);
-if (ret < 0 && ret != -ENOIOCTLCMD) {
-v4l2_fh_release(file);
-goto unlock;
-}
+if (ret < 0 && ret != -ENOIOCTLCMD)
+goto fh_rel;

 ret = isc_set_fmt(isc, >fmt);
-if (ret) {
+if (ret)
 v4l2_subdev_call(sd, core, s_power, 0);
-v4l2_fh_release(file);
-}

+fh_rel:
+if (ret)
+v4l2_fh_release(file);
 unlock:
 mutex_unlock(>lock);
 return ret;

Re: [PATCH] [media] atmel-isc: release the filehandle if it's not the only one.

[Wu, Songjun]

Sorry, my mistake, the device should be able to opened multiple times.
It's a wrong patch.

On 11/1/2016 16:52, Hans Verkuil wrote:

On 01/11/16 09:08, Songjun Wu wrote:

Release the filehandle in 'isc_open' if it's not the only filehandle
opened for the associated video_device.


What's wrong with that? You should always be able to open the device
multiple times. v4l2-compliance will fail after this patch. I'm not sure
what you intended to do here, but this patch is wrong.

Regards,

Hans



Signed-off-by: Songjun Wu 
---

 drivers/media/platform/atmel/atmel-isc.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index 8e25d3f..5e08404 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -926,21 +926,21 @@ static int isc_open(struct file *file)
 if (ret < 0)
 goto unlock;

-if (!v4l2_fh_is_singular_file(file))
-goto unlock;
+ret = !v4l2_fh_is_singular_file(file);
+if (ret)
+goto fh_rel;

 ret = v4l2_subdev_call(sd, core, s_power, 1);
-if (ret < 0 && ret != -ENOIOCTLCMD) {
-v4l2_fh_release(file);
-goto unlock;
-}
+if (ret < 0 && ret != -ENOIOCTLCMD)
+goto fh_rel;

 ret = isc_set_fmt(isc, >fmt);
-if (ret) {
+if (ret)
 v4l2_subdev_call(sd, core, s_power, 0);
-v4l2_fh_release(file);
-}

+fh_rel:
+if (ret)
+v4l2_fh_release(file);
 unlock:
 mutex_unlock(>lock);
 return ret;

Re: [PATCH 2/2] [media] atmel-isc: mark PM functions as __maybe_unused

[Wu, Songjun]

Hi Arnd,

Thank you for your patch.
I think it's better to add switch CONFIG_PM, but the PM feature is a 
must, or the ISC can not work, maybe the best choice is to add 'depends 
on PM' in Kconfig.


#ifdef CONFIG_PM
isc_runtime_suspend
{
XXX
}

isc_runtime_resume
{
XXX
}

static const struct dev_pm_ops atmel_isc_dev_pm_ops = {
SET_RUNTIME_PM_OPS(isc_runtime_suspend, isc_runtime_resume, NULL)
};
#define ATMEL_ISC_PM_OPS(_isc_dev_pm_ops)
#else
#define ATMEL_ISC_PM_OPSNULL
#endif

static struct platform_driver atmel_isc_driver = {
.probe  = atmel_isc_probe,
.remove = atmel_isc_remove,
.driver = {
.name   = ATMEL_ISC_NAME,
.pm = ATMEL_ISC_PM_OPS,
.of_match_table = of_match_ptr(atmel_isc_of_match),
},
};

On 9/12/2016 23:32, Arnd Bergmann wrote:

The newly added atmel-isc driver uses SET_RUNTIME_PM_OPS() to
refer to its suspend/resume functions, causing a warning when
CONFIG_PM is not set:

media/platform/atmel/atmel-isc.c:1477:12: error: 'isc_runtime_resume' defined 
but not used [-Werror=unused-function]
media/platform/atmel/atmel-isc.c:1467:12: error: 'isc_runtime_suspend' defined 
but not used [-Werror=unused-function]

This adds __maybe_unused annotations to avoid the warning without
adding an error-prone #ifdef around it.

Signed-off-by: Arnd Bergmann 
---
 drivers/media/platform/atmel/atmel-isc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index db6773de92f0..a9ab7ae89f04 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1464,7 +1464,7 @@ static int atmel_isc_remove(struct platform_device *pdev)
return 0;
 }

-static int isc_runtime_suspend(struct device *dev)
+static int __maybe_unused isc_runtime_suspend(struct device *dev)
 {
struct isc_device *isc = dev_get_drvdata(dev);

@@ -1474,7 +1474,7 @@ static int isc_runtime_suspend(struct device *dev)
return 0;
 }

-static int isc_runtime_resume(struct device *dev)
+static int __maybe_unused isc_runtime_resume(struct device *dev)
 {
struct isc_device *isc = dev_get_drvdata(dev);
int ret;

Re: [PATCH 2/2] [media] atmel-isc: mark PM functions as __maybe_unused

[Wu, Songjun]

Hi Arnd,

Thank you for your patch.
I think it's better to add switch CONFIG_PM, but the PM feature is a 
must, or the ISC can not work, maybe the best choice is to add 'depends 
on PM' in Kconfig.


#ifdef CONFIG_PM
isc_runtime_suspend
{
XXX
}

isc_runtime_resume
{
XXX
}

static const struct dev_pm_ops atmel_isc_dev_pm_ops = {
SET_RUNTIME_PM_OPS(isc_runtime_suspend, isc_runtime_resume, NULL)
};
#define ATMEL_ISC_PM_OPS(_isc_dev_pm_ops)
#else
#define ATMEL_ISC_PM_OPSNULL
#endif

static struct platform_driver atmel_isc_driver = {
.probe  = atmel_isc_probe,
.remove = atmel_isc_remove,
.driver = {
.name   = ATMEL_ISC_NAME,
.pm = ATMEL_ISC_PM_OPS,
.of_match_table = of_match_ptr(atmel_isc_of_match),
},
};

On 9/12/2016 23:32, Arnd Bergmann wrote:

The newly added atmel-isc driver uses SET_RUNTIME_PM_OPS() to
refer to its suspend/resume functions, causing a warning when
CONFIG_PM is not set:

media/platform/atmel/atmel-isc.c:1477:12: error: 'isc_runtime_resume' defined 
but not used [-Werror=unused-function]
media/platform/atmel/atmel-isc.c:1467:12: error: 'isc_runtime_suspend' defined 
but not used [-Werror=unused-function]

This adds __maybe_unused annotations to avoid the warning without
adding an error-prone #ifdef around it.

Signed-off-by: Arnd Bergmann 
---
 drivers/media/platform/atmel/atmel-isc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index db6773de92f0..a9ab7ae89f04 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1464,7 +1464,7 @@ static int atmel_isc_remove(struct platform_device *pdev)
return 0;
 }

-static int isc_runtime_suspend(struct device *dev)
+static int __maybe_unused isc_runtime_suspend(struct device *dev)
 {
struct isc_device *isc = dev_get_drvdata(dev);

@@ -1474,7 +1474,7 @@ static int isc_runtime_suspend(struct device *dev)
return 0;
 }

-static int isc_runtime_resume(struct device *dev)
+static int __maybe_unused isc_runtime_resume(struct device *dev)
 {
struct isc_device *isc = dev_get_drvdata(dev);
int ret;

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

[Wu, Songjun]

On 8/15/2016 15:34, Hans Verkuil wrote:

On 08/11/2016 09:06 AM, Songjun Wu 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.


So close...

Running checkpatch gives me:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#133:
new file mode 100644

Can you make a patch adding an entry to MAINTAINERS? No need to repost the other
two.


Thank you.
Nicolas or I will make a patch to add an entry to MAINTAINERS.


Regards,

Hans

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

[Wu, Songjun]

On 8/15/2016 15:34, Hans Verkuil wrote:

On 08/11/2016 09:06 AM, Songjun Wu 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.


So close...

Running checkpatch gives me:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#133:
new file mode 100644

Can you make a patch adding an entry to MAINTAINERS? No need to repost the other
two.


Thank you.
Nicolas or I will make a patch to add an entry to MAINTAINERS.


Regards,

Hans

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

[Wu, Songjun]

On 8/15/2016 15:15, Hans Verkuil wrote:

On 08/15/2016 08:09 AM, Wu, Songjun wrote:



On 8/12/2016 15:32, Hans Verkuil wrote:

One quick question:

On 08/11/2016 09:06 AM, Songjun Wu 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.

Test result with v4l-utils.
# v4l2-compliance -f
v4l2-compliance SHA   : not available

Driver Info:
Driver name   : atmel_isc
Card type : Atmel Image Sensor Controller
Bus info  : platform:atmel_isc f0008000.isc
Driver version: 4.7.0
Capabilities  : 0x8421
Video Capture
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0421
Video Capture
Streaming
Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0


Can you confirm that the sensor subdevice you are using does not have any 
controls?
I ask since that is fairly unusual, so I want to make sure that controls are 
really
not supported in this setup.


Sorry for the late reply.
The subdevice I use supports controls, but I did not develop the v4l2
controls in the sensor driver.


So you mean the sensor hardware has controls, but the sensor driver doesn't 
implement
them? Do I understand you correctly?


Yes, your understanding is correct.


Should I add the v4l2 controls and test again?


If the sensor driver does not implement controls (i.e. has a struct 
v4l2_ctrl_handler),
then everything is fine and the v4l2-compliance output is correct.

Please confirm this. I just want to be 100% certain about this before I make 
the pull
request.

I can confirm this. I use the sensor ov7740, and the driver is developed 
by myself, I did not add the v4l2 controls into the sensor driver for now.



Thanks,

Hans

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

[Wu, Songjun]

On 8/15/2016 15:15, Hans Verkuil wrote:

On 08/15/2016 08:09 AM, Wu, Songjun wrote:



On 8/12/2016 15:32, Hans Verkuil wrote:

One quick question:

On 08/11/2016 09:06 AM, Songjun Wu 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.

Test result with v4l-utils.
# v4l2-compliance -f
v4l2-compliance SHA   : not available

Driver Info:
Driver name   : atmel_isc
Card type : Atmel Image Sensor Controller
Bus info  : platform:atmel_isc f0008000.isc
Driver version: 4.7.0
Capabilities  : 0x8421
Video Capture
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0421
Video Capture
Streaming
Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0


Can you confirm that the sensor subdevice you are using does not have any 
controls?
I ask since that is fairly unusual, so I want to make sure that controls are 
really
not supported in this setup.


Sorry for the late reply.
The subdevice I use supports controls, but I did not develop the v4l2
controls in the sensor driver.


So you mean the sensor hardware has controls, but the sensor driver doesn't 
implement
them? Do I understand you correctly?


Yes, your understanding is correct.


Should I add the v4l2 controls and test again?


If the sensor driver does not implement controls (i.e. has a struct 
v4l2_ctrl_handler),
then everything is fine and the v4l2-compliance output is correct.

Please confirm this. I just want to be 100% certain about this before I make 
the pull
request.

I can confirm this. I use the sensor ov7740, and the driver is developed 
by myself, I did not add the v4l2 controls into the sensor driver for now.



Thanks,

Hans

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

[Wu, Songjun]

On 8/12/2016 15:32, Hans Verkuil wrote:

One quick question:

On 08/11/2016 09:06 AM, Songjun Wu 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.

Test result with v4l-utils.
# v4l2-compliance -f
v4l2-compliance SHA   : not available

Driver Info:
Driver name   : atmel_isc
Card type : Atmel Image Sensor Controller
Bus info  : platform:atmel_isc f0008000.isc
Driver version: 4.7.0
Capabilities  : 0x8421
Video Capture
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0421
Video Capture
Streaming
Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0


Can you confirm that the sensor subdevice you are using does not have any 
controls?
I ask since that is fairly unusual, so I want to make sure that controls are 
really
not supported in this setup.


Sorry for the late reply.
The subdevice I use supports controls, but I did not develop the v4l2 
controls in the sensor driver.

Should I add the v4l2 controls and test again?


Regards,

Hans

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

[Wu, Songjun]

On 8/12/2016 15:32, Hans Verkuil wrote:

One quick question:

On 08/11/2016 09:06 AM, Songjun Wu 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.

Test result with v4l-utils.
# v4l2-compliance -f
v4l2-compliance SHA   : not available

Driver Info:
Driver name   : atmel_isc
Card type : Atmel Image Sensor Controller
Bus info  : platform:atmel_isc f0008000.isc
Driver version: 4.7.0
Capabilities  : 0x8421
Video Capture
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0421
Video Capture
Streaming
Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0


Can you confirm that the sensor subdevice you are using does not have any 
controls?
I ask since that is fairly unusual, so I want to make sure that controls are 
really
not supported in this setup.


Sorry for the late reply.
The subdevice I use supports controls, but I did not develop the v4l2 
controls in the sensor driver.

Should I add the v4l2 controls and test again?


Regards,

Hans

Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/10/2016 15:12, Hans Verkuil wrote:

On 08/10/2016 07:36 AM, Wu, Songjun wrote:



On 8/8/2016 17:56, Hans Verkuil wrote:

On 08/08/2016 11:37 AM, Hans Verkuil wrote:

On 08/03/2016 10:08 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

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

Changes in v8:
- Power on the sensor on the first open in function
  'isc_open'.
- Power off the sensor on the last release in function
  'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1503 +
 6 files changed, 1681 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c





+static int isc_set_default_fmt(struct isc_device *isc)
+{
+   u32 index = isc->num_user_formats - 1;


Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.


+   struct v4l2_format f = {
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+   .fmt.pix = {
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
+   .field  = V4L2_FIELD_NONE,
+   .pixelformat= isc->user_formats[index]->fourcc,
+   },
+   };
+
+   return isc_set_fmt(isc, );
+}
+
+static int isc_open(struct file *file)
+{
+   struct isc_device *isc = video_drvdata(file);
+   struct v4l2_subdev *sd = isc->current_subdev->sd;
+   int ret;
+
+   if (mutex_lock_interruptible(>lock))
+   return -ERESTARTSYS;
+
+   ret = v4l2_fh_open(file);
+   if (ret < 0)
+   goto unlock;
+
+   if (!v4l2_fh_is_singular_file(file))
+   goto unlock;
+
+   ret = v4l2_subdev_call(sd, core, s_power, 1);
+   if (ret < 0 && ret != -ENOIOCTLCMD)
+   goto unlock;
+
+   ret = isc_set_default_fmt(isc);


This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.


Actually, you do need to set the format here since here is where you turn on
the sensor power, b

Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/10/2016 15:12, Hans Verkuil wrote:

On 08/10/2016 07:36 AM, Wu, Songjun wrote:



On 8/8/2016 17:56, Hans Verkuil wrote:

On 08/08/2016 11:37 AM, Hans Verkuil wrote:

On 08/03/2016 10:08 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu 
---

Changes in v8:
- Power on the sensor on the first open in function
  'isc_open'.
- Power off the sensor on the last release in function
  'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1503 +
 6 files changed, 1681 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c





+static int isc_set_default_fmt(struct isc_device *isc)
+{
+   u32 index = isc->num_user_formats - 1;


Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.


+   struct v4l2_format f = {
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+   .fmt.pix = {
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
+   .field  = V4L2_FIELD_NONE,
+   .pixelformat= isc->user_formats[index]->fourcc,
+   },
+   };
+
+   return isc_set_fmt(isc, );
+}
+
+static int isc_open(struct file *file)
+{
+   struct isc_device *isc = video_drvdata(file);
+   struct v4l2_subdev *sd = isc->current_subdev->sd;
+   int ret;
+
+   if (mutex_lock_interruptible(>lock))
+   return -ERESTARTSYS;
+
+   ret = v4l2_fh_open(file);
+   if (ret < 0)
+   goto unlock;
+
+   if (!v4l2_fh_is_singular_file(file))
+   goto unlock;
+
+   ret = v4l2_subdev_call(sd, core, s_power, 1);
+   if (ret < 0 && ret != -ENOIOCTLCMD)
+   goto unlock;
+
+   ret = isc_set_default_fmt(isc);


This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.


Actually, you do need to set the format here since here is where you turn on
the sensor power, but it should be the current format, n

Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/8/2016 17:56, Hans Verkuil wrote:

On 08/08/2016 11:37 AM, Hans Verkuil wrote:

On 08/03/2016 10:08 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

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

Changes in v8:
- Power on the sensor on the first open in function
  'isc_open'.
- Power off the sensor on the last release in function
  'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1503 +
 6 files changed, 1681 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c





+static int isc_set_default_fmt(struct isc_device *isc)
+{
+   u32 index = isc->num_user_formats - 1;


Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.


+   struct v4l2_format f = {
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+   .fmt.pix = {
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
+   .field  = V4L2_FIELD_NONE,
+   .pixelformat= isc->user_formats[index]->fourcc,
+   },
+   };
+
+   return isc_set_fmt(isc, );
+}
+
+static int isc_open(struct file *file)
+{
+   struct isc_device *isc = video_drvdata(file);
+   struct v4l2_subdev *sd = isc->current_subdev->sd;
+   int ret;
+
+   if (mutex_lock_interruptible(>lock))
+   return -ERESTARTSYS;
+
+   ret = v4l2_fh_open(file);
+   if (ret < 0)
+   goto unlock;
+
+   if (!v4l2_fh_is_singular_file(file))
+   goto unlock;
+
+   ret = v4l2_subdev_call(sd, core, s_power, 1);
+   if (ret < 0 && ret != -ENOIOCTLCMD)
+   goto unlock;
+
+   ret = isc_set_default_fmt(isc);


This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.


Actually, you do need to set the format here since here is where you turn on
the sensor power, but it should be the current format, not the default format.

And in isc_set_default_fmt I recomme

Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/8/2016 17:56, Hans Verkuil wrote:

On 08/08/2016 11:37 AM, Hans Verkuil wrote:

On 08/03/2016 10:08 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu 
---

Changes in v8:
- Power on the sensor on the first open in function
  'isc_open'.
- Power off the sensor on the last release in function
  'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1503 +
 6 files changed, 1681 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c





+static int isc_set_default_fmt(struct isc_device *isc)
+{
+   u32 index = isc->num_user_formats - 1;


Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.


+   struct v4l2_format f = {
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+   .fmt.pix = {
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
+   .field  = V4L2_FIELD_NONE,
+   .pixelformat= isc->user_formats[index]->fourcc,
+   },
+   };
+
+   return isc_set_fmt(isc, );
+}
+
+static int isc_open(struct file *file)
+{
+   struct isc_device *isc = video_drvdata(file);
+   struct v4l2_subdev *sd = isc->current_subdev->sd;
+   int ret;
+
+   if (mutex_lock_interruptible(>lock))
+   return -ERESTARTSYS;
+
+   ret = v4l2_fh_open(file);
+   if (ret < 0)
+   goto unlock;
+
+   if (!v4l2_fh_is_singular_file(file))
+   goto unlock;
+
+   ret = v4l2_subdev_call(sd, core, s_power, 1);
+   if (ret < 0 && ret != -ENOIOCTLCMD)
+   goto unlock;
+
+   ret = isc_set_default_fmt(isc);


This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.


Actually, you do need to set the format here since here is where you turn on
the sensor power, but it should be the current format, not the default format.

And in isc_set_default_fmt I recommend that you call the try fmt of the subdev
in order to let the subdev adjust the 

Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/8/2016 17:56, Hans Verkuil wrote:

On 08/08/2016 11:37 AM, Hans Verkuil wrote:

On 08/03/2016 10:08 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

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

Changes in v8:
- Power on the sensor on the first open in function
  'isc_open'.
- Power off the sensor on the last release in function
  'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1503 +
 6 files changed, 1681 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c





+static int isc_set_default_fmt(struct isc_device *isc)
+{
+   u32 index = isc->num_user_formats - 1;


Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.


+   struct v4l2_format f = {
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+   .fmt.pix = {
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
+   .field  = V4L2_FIELD_NONE,
+   .pixelformat= isc->user_formats[index]->fourcc,
+   },
+   };
+
+   return isc_set_fmt(isc, );
+}
+
+static int isc_open(struct file *file)
+{
+   struct isc_device *isc = video_drvdata(file);
+   struct v4l2_subdev *sd = isc->current_subdev->sd;
+   int ret;
+
+   if (mutex_lock_interruptible(>lock))
+   return -ERESTARTSYS;
+
+   ret = v4l2_fh_open(file);
+   if (ret < 0)
+   goto unlock;
+
+   if (!v4l2_fh_is_singular_file(file))
+   goto unlock;
+
+   ret = v4l2_subdev_call(sd, core, s_power, 1);
+   if (ret < 0 && ret != -ENOIOCTLCMD)
+   goto unlock;
+
+   ret = isc_set_default_fmt(isc);


This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.


Actually, you do need to set the format here since here is where you turn on
the sensor power, but it should be the current format, not the default format.

And in isc_set_default_fmt I recomme

Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/8/2016 17:56, Hans Verkuil wrote:

On 08/08/2016 11:37 AM, Hans Verkuil wrote:

On 08/03/2016 10:08 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu 
---

Changes in v8:
- Power on the sensor on the first open in function
  'isc_open'.
- Power off the sensor on the last release in function
  'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1503 +
 6 files changed, 1681 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c





+static int isc_set_default_fmt(struct isc_device *isc)
+{
+   u32 index = isc->num_user_formats - 1;


Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.


+   struct v4l2_format f = {
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+   .fmt.pix = {
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
+   .field  = V4L2_FIELD_NONE,
+   .pixelformat= isc->user_formats[index]->fourcc,
+   },
+   };
+
+   return isc_set_fmt(isc, );
+}
+
+static int isc_open(struct file *file)
+{
+   struct isc_device *isc = video_drvdata(file);
+   struct v4l2_subdev *sd = isc->current_subdev->sd;
+   int ret;
+
+   if (mutex_lock_interruptible(>lock))
+   return -ERESTARTSYS;
+
+   ret = v4l2_fh_open(file);
+   if (ret < 0)
+   goto unlock;
+
+   if (!v4l2_fh_is_singular_file(file))
+   goto unlock;
+
+   ret = v4l2_subdev_call(sd, core, s_power, 1);
+   if (ret < 0 && ret != -ENOIOCTLCMD)
+   goto unlock;
+
+   ret = isc_set_default_fmt(isc);


This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.


Actually, you do need to set the format here since here is where you turn on
the sensor power, but it should be the current format, not the default format.

And in isc_set_default_fmt I recommend that you call the try fmt of the subdev
in order to let the subdev adjust the 

Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/8/2016 17:37, Hans Verkuil wrote:

On 08/03/2016 10:08 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

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

Changes in v8:
- Power on the sensor on the first open in function
  'isc_open'.
- Power off the sensor on the last release in function
  'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1503 +
 6 files changed, 1681 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c





+static int isc_set_default_fmt(struct isc_device *isc)
+{
+   u32 index = isc->num_user_formats - 1;


Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.


Accept, thank you.
The raw format is listed on the beginning, in order to select non-raw 
format, the last format is picked. But it's not very important, it's 
better to use the beginning of the format list.



+   struct v4l2_format f = {
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+   .fmt.pix = {
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
+   .field  = V4L2_FIELD_NONE,
+   .pixelformat= isc->user_formats[index]->fourcc,
+   },
+   };
+
+   return isc_set_fmt(isc, );
+}
+
+static int isc_open(struct file *file)
+{
+   struct isc_device *isc = video_drvdata(file);
+   struct v4l2_subdev *sd = isc->current_subdev->sd;
+   int ret;
+
+   if (mutex_lock_interruptible(>lock))
+   return -ERESTARTSYS;
+
+   ret = v4l2_fh_open(file);
+   if (ret < 0)
+   goto unlock;
+
+   if (!v4l2_fh_is_singular_file(file))
+   goto unlock;
+
+   ret = v4l2_subdev_call(sd, core, s_power, 1);
+   if (ret < 0 && ret != -ENOIOCTLCMD)
+   goto unlock;
+
+   ret = isc_set_default_fmt(isc);


This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.

Only on th

Re: [PATCH v8 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/8/2016 17:37, Hans Verkuil wrote:

On 08/03/2016 10:08 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu 
---

Changes in v8:
- Power on the sensor on the first open in function
  'isc_open'.
- Power off the sensor on the last release in function
  'isc_release'.
- Remove the switch of the pipeline.

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1503 +
 6 files changed, 1681 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..d99d4a5
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c





+static int isc_set_default_fmt(struct isc_device *isc)
+{
+   u32 index = isc->num_user_formats - 1;


Why pick the last format? Strictly speaking it doesn't matter, but in practice
the most common formats tend to be at the beginning of the format list.


Accept, thank you.
The raw format is listed on the beginning, in order to select non-raw 
format, the last format is picked. But it's not very important, it's 
better to use the beginning of the format list.



+   struct v4l2_format f = {
+   .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+   .fmt.pix = {
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
+   .field  = V4L2_FIELD_NONE,
+   .pixelformat= isc->user_formats[index]->fourcc,
+   },
+   };
+
+   return isc_set_fmt(isc, );
+}
+
+static int isc_open(struct file *file)
+{
+   struct isc_device *isc = video_drvdata(file);
+   struct v4l2_subdev *sd = isc->current_subdev->sd;
+   int ret;
+
+   if (mutex_lock_interruptible(>lock))
+   return -ERESTARTSYS;
+
+   ret = v4l2_fh_open(file);
+   if (ret < 0)
+   goto unlock;
+
+   if (!v4l2_fh_is_singular_file(file))
+   goto unlock;
+
+   ret = v4l2_subdev_call(sd, core, s_power, 1);
+   if (ret < 0 && ret != -ENOIOCTLCMD)
+   goto unlock;
+
+   ret = isc_set_default_fmt(isc);


This doesn't belong here, this needs to be done in isc_async_complete().

Having the code here means that every time you open the device, the format
changes back to the default. That's not what you want.

Only on the first open, the default format will be set. If it's done in 
isc_async_complete(), there is a problem, 

Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/2/2016 15:32, Hans Verkuil wrote:



On 08/02/2016 08:20 AM, Wu, Songjun wrote:

+static unsigned int sensor_preferred = 1;
+module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(sensor_preferred,
+"Sensor is preferred to output the specified format (1-on 0-off) 
default 1");


I have no idea what this means. Can you elaborate? Why would you want to set 
this to 0?


ISC can convert the raw format to the other format, e.g. YUYV.
If we want to output YUYV format, there are two choices, one is the
sensor output YUYV format, ISC bypass the data to the memory, the other
is the sensor output raw format, ISC convert raw format to YUYV.

So I provide a module parameter to user to select.
I prefer to select the sensor to output the specified format, then I set
this parameter to '1', not '0'.


Does this only apply to YUYV?

The reason I am hesitant about this option is that I am not convinced you need
it. The default (sensor preferred) makes sense and that's what other drivers
do as well. Unless you know of a real use-case where you want to set this to 0,
I would just drop this option.

If there *is* a real use-case, then split off adding this module option into a
separate patch so we can discuss it more without blocking getting this driver
into mainline. I don't like the way this is done here.

This does not only apply to YUYV, ISC IP defines some formats that can 
be converted from raw format.
In some scenarios, ISC can extend the formats, e.g. if the sensor does 
not support YUYV, but raw format, the ISC can output YUYV.


OK, I will remove the related code.
After this driver is accepted, I will submit another patch to add this 
feature.


Thank you.


Regards,

Hans

Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/2/2016 15:32, Hans Verkuil wrote:



On 08/02/2016 08:20 AM, Wu, Songjun wrote:

+static unsigned int sensor_preferred = 1;
+module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(sensor_preferred,
+"Sensor is preferred to output the specified format (1-on 0-off) 
default 1");


I have no idea what this means. Can you elaborate? Why would you want to set 
this to 0?


ISC can convert the raw format to the other format, e.g. YUYV.
If we want to output YUYV format, there are two choices, one is the
sensor output YUYV format, ISC bypass the data to the memory, the other
is the sensor output raw format, ISC convert raw format to YUYV.

So I provide a module parameter to user to select.
I prefer to select the sensor to output the specified format, then I set
this parameter to '1', not '0'.


Does this only apply to YUYV?

The reason I am hesitant about this option is that I am not convinced you need
it. The default (sensor preferred) makes sense and that's what other drivers
do as well. Unless you know of a real use-case where you want to set this to 0,
I would just drop this option.

If there *is* a real use-case, then split off adding this module option into a
separate patch so we can discuss it more without blocking getting this driver
into mainline. I don't like the way this is done here.

This does not only apply to YUYV, ISC IP defines some formats that can 
be converted from raw format.
In some scenarios, ISC can extend the formats, e.g. if the sensor does 
not support YUYV, but raw format, the ISC can output YUYV.


OK, I will remove the related code.
After this driver is accepted, I will submit another patch to add this 
feature.


Thank you.


Regards,

Hans

Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/1/2016 17:47, Hans Verkuil wrote:

Hi Songjun,

Some more comments below. Except for one in the open/release functions
it's all small things.

On 07/29/2016 09:54 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

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

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1611 +
 6 files changed, 1789 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..f2ef664
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -0,0 +1,1611 @@





+static unsigned int sensor_preferred = 1;
+module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(sensor_preferred,
+"Sensor is preferred to output the specified format (1-on 0-off) 
default 1");


I have no idea what this means. Can you elaborate? Why would you want to set 
this to 0?


ISC can convert the raw format to the other format, e.g. YUYV.
If we want to output YUYV format, there are two choices, one is the 
sensor output YUYV format, ISC bypass the data to the memory, the other 
is the sensor output raw format, ISC convert raw format to YUYV.


So I provide a module parameter to user to select.
I prefer to select the sensor to output the specified format, then I set 
this parameter to '1', not '0'.






+static inline bool sensor_is_preferred(const struct isc_format *isc_fmt)
+{
+   if ((sensor_preferred && isc_fmt->sd_support) ||
+   !isc_fmt->isc_support)


I'd just do:

return (sensor_preferred && isc_fmt->sd_support) ||
   !isc_fmt->isc_support;


Accept, thank you.


+   return true;
+   else
+   return false;
+}
+





+static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
+   struct isc_format **current_fmt, u32 *code)
+{
+   struct isc_format *isc_fmt;
+   struct v4l2_pix_format *pixfmt = >fmt.pix;
+   struct v4l2_subdev_format format = {
+   .which = V4L2_SUBDEV_FORMAT_TRY,
+   };
+   u32 mbus_code;
+   int ret;
+
+   if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+   return -EINVAL;
+
+   isc_fmt = find_format_by_fourcc(isc, pixfmt->pixelformat);
+   if (!isc_fmt) {
+   v4l2_warn(>v4l2_dev, "Format 0x%x not found\

Re: [PATCH v7 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 8/1/2016 17:47, Hans Verkuil wrote:

Hi Songjun,

Some more comments below. Except for one in the open/release functions
it's all small things.

On 07/29/2016 09:54 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu 
---

Changes in v7:
- Add enum_framesizes and enum_frameintervals.
- Call s_stream(0) when stream start fail.
- Fill the device_caps field of struct video_device
  with V4L2_CAP_STREAMING and V4L2_CAP_VIDEO_CAPTURE.
- Initialize the dev of struct vb2_queue.
- Set field to FIELD_NONE if the pix field is not supported.
- Return the result directly when call g/s_parm of subdev.

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1611 +
 6 files changed, 1789 insertions(+)
 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






diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
new file mode 100644
index 000..f2ef664
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -0,0 +1,1611 @@





+static unsigned int sensor_preferred = 1;
+module_param(sensor_preferred, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(sensor_preferred,
+"Sensor is preferred to output the specified format (1-on 0-off) 
default 1");


I have no idea what this means. Can you elaborate? Why would you want to set 
this to 0?


ISC can convert the raw format to the other format, e.g. YUYV.
If we want to output YUYV format, there are two choices, one is the 
sensor output YUYV format, ISC bypass the data to the memory, the other 
is the sensor output raw format, ISC convert raw format to YUYV.


So I provide a module parameter to user to select.
I prefer to select the sensor to output the specified format, then I set 
this parameter to '1', not '0'.






+static inline bool sensor_is_preferred(const struct isc_format *isc_fmt)
+{
+   if ((sensor_preferred && isc_fmt->sd_support) ||
+   !isc_fmt->isc_support)


I'd just do:

return (sensor_preferred && isc_fmt->sd_support) ||
   !isc_fmt->isc_support;


Accept, thank you.


+   return true;
+   else
+   return false;
+}
+





+static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
+   struct isc_format **current_fmt, u32 *code)
+{
+   struct isc_format *isc_fmt;
+   struct v4l2_pix_format *pixfmt = >fmt.pix;
+   struct v4l2_subdev_format format = {
+   .which = V4L2_SUBDEV_FORMAT_TRY,
+   };
+   u32 mbus_code;
+   int ret;
+
+   if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+   return -EINVAL;
+
+   isc_fmt = find_format_by_fourcc(isc, pixfmt->pixelformat);
+   if (!isc_fmt) {
+   v4l2_warn(>v4l2_dev, "Format 0x%x not found\n",
+ pixfmt->pixelformat);
+   isc_fmt = isc->user_formats[isc->num_user_formats 

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

[Wu, Songjun]

On 7/30/2016 05:44, Rob Herring wrote:

On Fri, Jul 29, 2016 at 03:54:08PM +0800, Songjun Wu wrote:

DT binding documentation for ISC driver.

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

Changes in v7: None
Changes in v6:
- Add "iscck" and "gck" to clock-names.

Changes in v5:
- Add clock-output-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| 65 ++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt


Please add acks when posting new versions.

Rob


Hi Rob,

Thank you for your reminder.

Should I Add 'Acked-by: Rob Herring <r...@kernel.org>' behind 
'Signed-off-by: Songjun Wu <songjun...@microchip.com>'?


Should I resend this patch?

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

[Wu, Songjun]

On 7/30/2016 05:44, Rob Herring wrote:

On Fri, Jul 29, 2016 at 03:54:08PM +0800, Songjun Wu wrote:

DT binding documentation for ISC driver.

Signed-off-by: Songjun Wu 
---

Changes in v7: None
Changes in v6:
- Add "iscck" and "gck" to clock-names.

Changes in v5:
- Add clock-output-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| 65 ++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/atmel-isc.txt


Please add acks when posting new versions.

Rob


Hi Rob,

Thank you for your reminder.

Should I Add 'Acked-by: Rob Herring ' behind 
'Signed-off-by: Songjun Wu '?


Should I resend this patch?

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

[Wu, Songjun]

On 7/21/2016 16:41, Hans Verkuil wrote:



On 07/21/2016 10:14 AM, Songjun Wu 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.

Test result with v4l-utils 1.10.1


Please compile from the v4l-utils repository. The version you used here is 
out-of-date.
I continually add new tests, so always compile the latest version from the repo.


OK, I will compile the latest version from the v4l-utils repository.
Thank you.


Regards,

Hans


Driver Info:
Driver name   : atmel_isc
Card type : Atmel Image Sensor Controller
Bus info  : platform:atmel_isc f0008000.isc
Driver version: 4.7.0
Capabilities  : 0x8421
Video Capture
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0421
Video Capture
Streaming
Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

Test input 0:

Stream using all formats:
test MMAP for Format BA81, Frame Size 640x480:
Stride 640, Field None: OK
test MMAP for Format YUYV, Frame Size 640x480:
Stride 1280, Field None: OK

Total: 44, Succeeded: 44, Failed: 0, Warnings: 0

Changes in v6:
- Add "iscck" and "gck" to clock-names.

Changes in v5:
- Modify the macro definition and the related code.
- Add clock-output-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 

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

[Wu, Songjun]

On 7/21/2016 16:41, Hans Verkuil wrote:



On 07/21/2016 10:14 AM, Songjun Wu 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.

Test result with v4l-utils 1.10.1


Please compile from the v4l-utils repository. The version you used here is 
out-of-date.
I continually add new tests, so always compile the latest version from the repo.


OK, I will compile the latest version from the v4l-utils repository.
Thank you.


Regards,

Hans


Driver Info:
Driver name   : atmel_isc
Card type : Atmel Image Sensor Controller
Bus info  : platform:atmel_isc f0008000.isc
Driver version: 4.7.0
Capabilities  : 0x8421
Video Capture
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0421
Video Capture
Streaming
Extended Pix Format

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
test VIDIOC_QUERYCTRL: OK (Not Supported)
test VIDIOC_G/S_CTRL: OK (Not Supported)
test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 0 Private Controls: 0

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

Test input 0:

Stream using all formats:
test MMAP for Format BA81, Frame Size 640x480:
Stride 640, Field None: OK
test MMAP for Format YUYV, Frame Size 640x480:
Stride 1280, Field None: OK

Total: 44, Succeeded: 44, Failed: 0, Warnings: 0

Changes in v6:
- Add "iscck" and "gck" to clock-names.

Changes in v5:
- Modify the macro definition and the related code.
- Add clock-output-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 

Re: [PATCH v6 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 7/21/2016 17:13, Hans Verkuil wrote:



On 07/21/2016 10:14 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

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

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1554 +
 6 files changed, 1732 insertions(+)
 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

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index f25344b..b23db17 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -111,6 +111,7 @@ source "drivers/media/platform/s5p-tv/Kconfig"
 source "drivers/media/platform/am437x/Kconfig"
 source "drivers/media/platform/xilinx/Kconfig"
 source "drivers/media/platform/rcar-vin/Kconfig"
+source "drivers/media/platform/atmel/Kconfig"

 config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 21771c1..37b6c75 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -58,6 +58,8 @@ obj-$(CONFIG_VIDEO_XILINX)+= xilinx/

 obj-$(CONFIG_VIDEO_RCAR_VIN)   += rcar-vin/

+obj-$(CONFIG_VIDEO_ATMEL_ISC)  += atmel/
+
 ccflags-y += -I$(srctree)/drivers/media/i2c

 obj-$(CONFIG_VIDEO_MEDIATEK_VPU)   += mtk-vpu/
diff --git a/drivers/media/platform/atmel/Kconfig 
b/drivers/media/platform/atmel/Kconfig
new file mode 100644
index 000..867dca2
--- /dev/null
+++ b/drivers/media/platform/atmel/Kconfig
@@ -0,0 +1,9 @@
+config VIDEO_ATMEL_ISC
+   tristate "ATMEL Image Sensor Controller (ISC) support"
+   depends on VIDEO_V4L2 && COMMON_CLK && VIDEO_V4L2_SUBDEV_API && HAS_DMA
+   depends on ARCH_AT91 || COMPILE_TEST
+   select VIDEOBUF2_DMA_CONTIG
+   select REGMAP_MMIO
+   help
+  This module makes the ATMEL Image Sensor Controller available
+  as a v4l2 device.
\ No newline at end of file
diff --git a/drivers/media/platform/atmel/Makefile 
b/drivers/media/platform/atmel/Makefile
new file mode 100644
index 000..9d7c999
--- /dev/null
+++ b/drivers/media/platform/atmel/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h 
b/drivers/media/platform/atmel/atmel-isc-regs.h
new file mode 100644
index 000..00c4497
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc-regs.h
@@ -0,0 +1,165 @@
+#ifndef __ATMEL_ISC_REGS_H
+#define __ATMEL_ISC_REGS_H
+
+#include 
+
+/* ISC Control Enable Register 0 */
+#define ISC_CTRLEN  0x
+
+/* ISC Control Disable Register 0 */
+#define ISC_CTRLDIS 0x0004
+
+/* ISC Control Status Register 0 */
+#define ISC_CTRLSR  0x0008
+
+#define ISC_CTRL_CAPTURE   BIT(0)
+#define IS

Re: [PATCH v6 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 7/21/2016 17:13, Hans Verkuil wrote:



On 07/21/2016 10:14 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu 
---

Changes in v6: None
Changes in v5:
- Modify the macro definition and the related code.

Changes in v4:
- Modify the isc clock code since the dt is changed.

Changes in v3:
- Add pm runtime feature.
- Modify the isc clock code since the dt is changed.

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().

 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  | 1554 +
 6 files changed, 1732 insertions(+)
 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

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index f25344b..b23db17 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -111,6 +111,7 @@ source "drivers/media/platform/s5p-tv/Kconfig"
 source "drivers/media/platform/am437x/Kconfig"
 source "drivers/media/platform/xilinx/Kconfig"
 source "drivers/media/platform/rcar-vin/Kconfig"
+source "drivers/media/platform/atmel/Kconfig"

 config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 21771c1..37b6c75 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -58,6 +58,8 @@ obj-$(CONFIG_VIDEO_XILINX)+= xilinx/

 obj-$(CONFIG_VIDEO_RCAR_VIN)   += rcar-vin/

+obj-$(CONFIG_VIDEO_ATMEL_ISC)  += atmel/
+
 ccflags-y += -I$(srctree)/drivers/media/i2c

 obj-$(CONFIG_VIDEO_MEDIATEK_VPU)   += mtk-vpu/
diff --git a/drivers/media/platform/atmel/Kconfig 
b/drivers/media/platform/atmel/Kconfig
new file mode 100644
index 000..867dca2
--- /dev/null
+++ b/drivers/media/platform/atmel/Kconfig
@@ -0,0 +1,9 @@
+config VIDEO_ATMEL_ISC
+   tristate "ATMEL Image Sensor Controller (ISC) support"
+   depends on VIDEO_V4L2 && COMMON_CLK && VIDEO_V4L2_SUBDEV_API && HAS_DMA
+   depends on ARCH_AT91 || COMPILE_TEST
+   select VIDEOBUF2_DMA_CONTIG
+   select REGMAP_MMIO
+   help
+  This module makes the ATMEL Image Sensor Controller available
+  as a v4l2 device.
\ No newline at end of file
diff --git a/drivers/media/platform/atmel/Makefile 
b/drivers/media/platform/atmel/Makefile
new file mode 100644
index 000..9d7c999
--- /dev/null
+++ b/drivers/media/platform/atmel/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h 
b/drivers/media/platform/atmel/atmel-isc-regs.h
new file mode 100644
index 000..00c4497
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc-regs.h
@@ -0,0 +1,165 @@
+#ifndef __ATMEL_ISC_REGS_H
+#define __ATMEL_ISC_REGS_H
+
+#include 
+
+/* ISC Control Enable Register 0 */
+#define ISC_CTRLEN  0x
+
+/* ISC Control Disable Register 0 */
+#define ISC_CTRLDIS 0x0004
+
+/* ISC Control Status Register 0 */
+#define ISC_CTRLSR  0x0008
+
+#define ISC_CTRL_CAPTURE   BIT(0)
+#define ISC_CTRL_UPPRO BIT(1)
+#define ISC_CTRL_HISREQBIT(2)
+#define ISC_CTRL_HISCLRBIT(3)
+
+/* ISC Parallel Front 

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

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

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...@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...@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 000..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.

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

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 000..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.

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...@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 000..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 = <_clk>, <>, <_gclk>;
+   clock-names = "hclock", "iscck", "gck";
+   #clock-cells = <0>;
+   clock-output-names = "isc-mck";
+   pinctrl-names = "default";
+   pinctrl-0 = <_isc_base _isc_data_8bit _isc_data_9_10 
_isc_data_11_12>;
+
+   port {
+   isc_0: endpoint {
+   remote-endpoint = <_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 = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_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 
---

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 000..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 = <_clk>, <>, <_gclk>;
+   clock-names = "hclock", "iscck", "gck";
+   #clock-cells = <0>;
+   clock-output-names = "isc-mck";
+   pinctrl-names = "default";
+   pinctrl-0 = <_isc_base _isc_data_8bit _isc_data_9_10 
_isc_data_11_12>;
+
+   port {
+   isc_0: endpoint {
+   remote-endpoint = <_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 = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};
--
2.7.4

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

[Wu, Songjun]

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...@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


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 000..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 = <_clk>, <>, <_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 = <_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 = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};
--
2.7.4

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

[Wu, Songjun]

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

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 000..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 = <_clk>, <>, <_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 = <_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 = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};
--
2.7.4

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

[Wu, Songjun]

On 6/12/2016 15:23, Boris Brezillon wrote:

On Sun, 12 Jun 2016 11:04:27 +0800
"Wu, Songjun" <songjun...@atmel.com> wrote:


On 6/9/2016 05:57, Boris Brezillon wrote:

On Tue, 7 Jun 2016 15:11:53 +0800
Songjun Wu <songjun...@atmel.com> wrote:


DT binding documentation for ISC driver.

Signed-off-by: Songjun Wu <songjun...@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 000..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 = <_clk>, <>, <_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 = <_0>;
+   hsync-active = <1>;
+   vsync-active = <0>;
+   pclk-sample = <1>;
+   };
+   };
+
+
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};

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

[Wu, Songjun]

On 6/12/2016 15:23, Boris Brezillon wrote:

On Sun, 12 Jun 2016 11:04:27 +0800
"Wu, Songjun"  wrote:


On 6/9/2016 05:57, Boris Brezillon wrote:

On Tue, 7 Jun 2016 15:11:53 +0800
Songjun Wu  wrote:


DT binding documentation for ISC driver.

Signed-off-by: Songjun Wu 
---

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 000..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 = <_clk>, <>, <_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 = <_0>;
+   hsync-active = <1>;
+   vsync-active = <0>;
+   pclk-sample = <1>;
+   };
+   };
+
+
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};

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

[Wu, Songjun]

On 6/9/2016 05:57, Boris Brezillon wrote:

On Tue, 7 Jun 2016 15:11:53 +0800
Songjun Wu <songjun...@atmel.com> wrote:


DT binding documentation for ISC driver.

Signed-off-by: Songjun Wu <songjun...@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 000..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 = <_clk>, <>, <_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 = <_0>;
+   hsync-active = <1>;
+   vsync-active = <0>;
+   pclk-sample = <1>;
+   };
+   };
+
+
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};

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

[Wu, Songjun]

On 6/9/2016 05:57, Boris Brezillon wrote:

On Tue, 7 Jun 2016 15:11:53 +0800
Songjun Wu  wrote:


DT binding documentation for ISC driver.

Signed-off-by: Songjun Wu 
---

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 000..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 = <_clk>, <>, <_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 = <_0>;
+   hsync-active = <1>;
+   vsync-active = <0>;
+   pclk-sample = <1>;
+   };
+   };
+
+
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <>;
+   clock-names = "xvclk";
+   assigned-clocks = <>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};

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 <r...@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...@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 000..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 = <_clk>, <_ispck>;
+   clock-names = "hclock", "ispck";
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   isc_0: endpoint {
+   remote-endpoint = <_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 = <_clk>, <>;
+   };
+
+   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 = <_clk>, <_ispck>;
clock-names = "hclock", "ispck";
#clock-cells = <0>;
clock-output-names = "isc-mck";
};

i2c1: i2c@fc028000 {
/* ... */
ov7740: camera@21 {
/* ... */
clocks = <>;
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 = < 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.

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

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 000..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 = <_clk>, <_ispck>;
+   clock-names = "hclock", "ispck";
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   isc_0: endpoint {
+   remote-endpoint = <_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 = <_clk>, <>;
+   };
+
+   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 = <_clk>, <_ispck>;
clock-names = "hclock", "ispck";
#clock-cells = <0>;
clock-output-names = "isc-mck";
};

i2c1: i2c@fc028000 {
/* ... */
ov7740: camera@21 {
/* ... */
clocks = <>;
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 = < 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 = <_clk>, <>, <_gclk>;
+   };
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <_mck>;
+   clock-names = "xvclk";
+

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...@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 000..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 = <_clk>, <_ispck>;
+   clock-names = "hclock", "ispck";
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   isc_0: endpoint {
+   remote-endpoint = <_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 = <_clk>, <>;
+   };
+
+   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 = <_clk>, <>, <_gclk>;
+   };
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <_mck>;
+   clock-names = "xvclk";
+   assigned-clocks = <_mck>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};
--
2.7.4

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

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 000..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 = <_clk>, <_ispck>;
+   clock-names = "hclock", "ispck";
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   isc_0: endpoint {
+   remote-endpoint = <_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 = <_clk>, <>;
+   };
+
+   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 = <_clk>, <>, <_gclk>;
+   };
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@21 {
+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <_mck>;
+   clock-names = "xvclk";
+   assigned-clocks = <_mck>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};
--
2.7.4

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...@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 000..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 = <_clk>, <>, <_gclk>;
+
+Example:
+isc: isc@f0008000 {
+   compatible = "atmel,sama5d2-isc";
+   reg = <0xf0008000 0x4000>;
+   interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+   clocks = <_clk>, <_ispck>;
+   clock-names = "hclock", "ispck";
+   atmel,sensor-preferred = <1>;
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   isc_0: endpoint {
+   remote-endpoint = <_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 < 0> and < 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 = <_clk>, <>;
+   };
+
+   isc_mck: isc-mck@1 {
+   #clock-cells = <0>;
+   reg = <1>;
+   clocks = <_clk>, <>, <_gclk>;
+   };
+   };
+};

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

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 000..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 = <_clk>, <>, <_gclk>;
+
+Example:
+isc: isc@f0008000 {
+   compatible = "atmel,sama5d2-isc";
+   reg = <0xf0008000 0x4000>;
+   interrupts = <46 IRQ_TYPE_LEVEL_HIGH 5>;
+   clocks = <_clk>, <_ispck>;
+   clock-names = "hclock", "ispck";
+   atmel,sensor-preferred = <1>;
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   isc_0: endpoint {
+   remote-endpoint = <_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 < 0> and < 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 = <_clk>, <>;
+   };
+
+   isc_mck: isc-mck@1 {
+   #clock-cells = <0>;
+   reg = <1>;
+   clocks = <_clk>, <>, <_gclk>;
+   };
+   };
+};

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

[Wu, Songjun]

On 4/14/2016 23:29, Rob Herring wrote:

On Wed, Apr 13, 2016 at 03:44:20PM +0800, Songjun Wu wrote:

DT binding documentation for ISC driver.

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

  .../devicetree/bindings/media/atmel-isc.txt| 84 ++
  1 file changed, 84 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 000..449f05f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,84 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties:
+- 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.
+- clk_in_isc


No underscores and this needs a better explanation.


Accept.
This is a node includes the isc internal clocks.
Maybe it should be 'List of ISC interal clocks'.
Do you think it's better?
Thank you.


+   ISC internal clock node, it includes the isc_ispck and isc_mck.
+   Please refer to clock-bindings.txt.
+- atmel,sensor-preferred
+   Sensor is preferred to process image (1-preferred, 0-not).
+   The default value is 1.


This seems a bit questionable.

ISC has an internal image processor, it can convert raw format to the 
other format, like YUYV format. If user want to output YUYV format, ISC 
driver has two choices, one is sensor output YUYV format, ISC does not 
do any process, the other is sensor output raw format, ISC converts raw 
format to YUYV format.
But how to choose? I set a option 'atmel,sensor-preferred' in dts to let 
user decide.



+
+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 = <_clk>, <_ispck>;
+   clock-names = "hclock", "ispck";
+   atmel,sensor-preferred = <1>;
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   isc_0: endpoint@0 {


Don't need a unit address for a single endpoint.


Yes, it's a single endpoint.
But we can create more endpoints, but only one endpoint will be 
effective. If you want to change the sensor, you can use the same dtb 
file. It's convenient for user.



+   remote-endpoint = <_0>;
+   hsync-active = <1>;
+   vsync-active = <0>;
+   pclk-sample = <1>;


Are these documented? They are really properties of the sensor and
should be part of the sensor node.


Accept. It should be part of the sensor node.
Thank you.


+   };
+   };
+
+   clk_in_isc {


Completely missed this is a node from the above description. I should be
able to write or validate the example from the description.

I think perhaps you should just drop all this from the DT, just list the
input clocks, and make the ISC node the clock controller.

I think this node is needed. ISC will provide the clock to sensor, we 
need create the corresponding clock node int DT file, then the sensor 
will get this clock and set the clock rate in DT file



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


No underscores in node names.

Accept, thank you.




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


Drop or you need a unit address.


OK, I will set a unit address.


+   clocks = <_clk>, <>;
+   };
+
+   isc_mck: isc_mck {
+   #clock-cells = <0>;
+   reg = <1>;
+   clocks = <_clk>, <>, <_gclk>;
+   };
+   };
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@0x21 {


Drop "0x"


Accept, thank you.


+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <_mck>;
+   clock-names = "xvclk";
+   assigned-clocks = <_mck>;
+   assigned-clock-rates = <2400>;
+

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

[Wu, Songjun]

On 4/14/2016 23:29, Rob Herring wrote:

On Wed, Apr 13, 2016 at 03:44:20PM +0800, Songjun Wu wrote:

DT binding documentation for ISC driver.

Signed-off-by: Songjun Wu 
---

  .../devicetree/bindings/media/atmel-isc.txt| 84 ++
  1 file changed, 84 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 000..449f05f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -0,0 +1,84 @@
+Atmel Image Sensor Controller (ISC)
+--
+
+Required properties:
+- 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.
+- clk_in_isc


No underscores and this needs a better explanation.


Accept.
This is a node includes the isc internal clocks.
Maybe it should be 'List of ISC interal clocks'.
Do you think it's better?
Thank you.


+   ISC internal clock node, it includes the isc_ispck and isc_mck.
+   Please refer to clock-bindings.txt.
+- atmel,sensor-preferred
+   Sensor is preferred to process image (1-preferred, 0-not).
+   The default value is 1.


This seems a bit questionable.

ISC has an internal image processor, it can convert raw format to the 
other format, like YUYV format. If user want to output YUYV format, ISC 
driver has two choices, one is sensor output YUYV format, ISC does not 
do any process, the other is sensor output raw format, ISC converts raw 
format to YUYV format.
But how to choose? I set a option 'atmel,sensor-preferred' in dts to let 
user decide.



+
+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 = <_clk>, <_ispck>;
+   clock-names = "hclock", "ispck";
+   atmel,sensor-preferred = <1>;
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   isc_0: endpoint@0 {


Don't need a unit address for a single endpoint.


Yes, it's a single endpoint.
But we can create more endpoints, but only one endpoint will be 
effective. If you want to change the sensor, you can use the same dtb 
file. It's convenient for user.



+   remote-endpoint = <_0>;
+   hsync-active = <1>;
+   vsync-active = <0>;
+   pclk-sample = <1>;


Are these documented? They are really properties of the sensor and
should be part of the sensor node.


Accept. It should be part of the sensor node.
Thank you.


+   };
+   };
+
+   clk_in_isc {


Completely missed this is a node from the above description. I should be
able to write or validate the example from the description.

I think perhaps you should just drop all this from the DT, just list the
input clocks, and make the ISC node the clock controller.

I think this node is needed. ISC will provide the clock to sensor, we 
need create the corresponding clock node int DT file, then the sensor 
will get this clock and set the clock rate in DT file



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


No underscores in node names.

Accept, thank you.




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


Drop or you need a unit address.


OK, I will set a unit address.


+   clocks = <_clk>, <>;
+   };
+
+   isc_mck: isc_mck {
+   #clock-cells = <0>;
+   reg = <1>;
+   clocks = <_clk>, <>, <_gclk>;
+   };
+   };
+};
+
+i2c1: i2c@fc028000 {
+   ov7740: camera@0x21 {


Drop "0x"


Accept, thank you.


+   compatible = "ovti,ov7740";
+   reg = <0x21>;
+
+   clocks = <_mck>;
+   clock-names = "xvclk";
+   assigned-clocks = <_mck>;
+   assigned-clock-rates = <2400>;
+
+   port {
+   ov7740_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+};
--
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More 

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 4/15/2016 00:21, Laurent Pinchart wrote:

>+   return -EINVAL;
>+
>+   parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
>+   if (!parent_names)
>+   return -ENOMEM;
>+
>+   of_clk_parent_fill(np, parent_names, num_parents);
>+
>+   init.parent_names   = parent_names;
>+   init.num_parents= num_parents;
>+   init.name   = clk_name;
>+   init.ops= _clk_ops;
>+   init.flags  = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE;
>+
>+   isc_clk = >isc_clks[id];
>+   isc_clk->hw.init = 
>+   isc_clk->regmap  = regmap;
>+   isc_clk->lock= lock;
>+   isc_clk->id  = id;
>+
>+   isc_clk->clk = clk_register(NULL, _clk->hw);
>+   if (!IS_ERR(isc_clk->clk))
>+   of_clk_add_provider(np, of_clk_src_simple_get, isc_clk->clk);
>+   else {
>+   dev_err(isc->dev, "%s: clock register fail\n", clk_name);
>+   ret = PTR_ERR(isc_clk->clk);
>+   goto free_parent_names;
>+   }
>+
>+free_parent_names:
>+   kfree(parent_names);
>+   return ret;
>+}
>+
>+static int isc_clk_init(struct isc_device *isc)
>+{
>+   struct device_node *np = of_get_child_by_name(isc->dev->of_node,
>+ "clk_in_isc");

Do you really need the clk_in_isc DT node ? I would have assumed that the
clock topology inside the ISC is fixed, and that it would be enough to just
specify the three parent clocks in the ISC DT node and create the two internal
clocks in the driver without needing a DT description.


Hi Laurent,

I think more, and the clk_in_isc DT node should be needed. The clock 
topology inside the ISC is fixed, but isc will provide the clock to 
sensor, we need create the corresponding clock node int DT file, then 
the sensor will get this clock and set the clock rate in DT file.

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 4/15/2016 00:21, Laurent Pinchart wrote:

>+   return -EINVAL;
>+
>+   parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
>+   if (!parent_names)
>+   return -ENOMEM;
>+
>+   of_clk_parent_fill(np, parent_names, num_parents);
>+
>+   init.parent_names   = parent_names;
>+   init.num_parents= num_parents;
>+   init.name   = clk_name;
>+   init.ops= _clk_ops;
>+   init.flags  = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE;
>+
>+   isc_clk = >isc_clks[id];
>+   isc_clk->hw.init = 
>+   isc_clk->regmap  = regmap;
>+   isc_clk->lock= lock;
>+   isc_clk->id  = id;
>+
>+   isc_clk->clk = clk_register(NULL, _clk->hw);
>+   if (!IS_ERR(isc_clk->clk))
>+   of_clk_add_provider(np, of_clk_src_simple_get, isc_clk->clk);
>+   else {
>+   dev_err(isc->dev, "%s: clock register fail\n", clk_name);
>+   ret = PTR_ERR(isc_clk->clk);
>+   goto free_parent_names;
>+   }
>+
>+free_parent_names:
>+   kfree(parent_names);
>+   return ret;
>+}
>+
>+static int isc_clk_init(struct isc_device *isc)
>+{
>+   struct device_node *np = of_get_child_by_name(isc->dev->of_node,
>+ "clk_in_isc");

Do you really need the clk_in_isc DT node ? I would have assumed that the
clock topology inside the ISC is fixed, and that it would be enough to just
specify the three parent clocks in the ISC DT node and create the two internal
clocks in the driver without needing a DT description.


Hi Laurent,

I think more, and the clk_in_isc DT node should be needed. The clock 
topology inside the ISC is fixed, but isc will provide the clock to 
sensor, we need create the corresponding clock node int DT file, then 
the sensor will get this clock and set the clock rate in DT file.

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 4/18/2016 15:24, Hans Verkuil wrote:

On 04/13/2016 09:44 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

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


Hi Songjun,

Before this driver can be accepted it has to pass the v4l2-compliance test.
The v4l2-compliance utility is here:

git://linuxtv.org/v4l-utils.git

Compile the utility straight from this repository so you're up to date.

First fix any failures you get when running 'v4l2-compliance', then do the
same when running 'v4l2-compliance -s' (now it is streaming as well) and
finally when running 'v4l2-compliance -f' (streaming and testing all available
formats).

I would like to see the output of 'v4l2-compliance -f' as part of the cover
letter of the next patch series.

Looking at the code I see that it will definitely fail a few tests :-)

Certainly the VIDIOC_CREATE_BUFS support in the queue_setup function is missing.
Read the comments for queue_setup in videobuf2-core.h for more information.

Thank you very much, I will pass the v4l2-compliance test before I send 
the version 2 patch.



Regards,

Hans

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 4/18/2016 15:24, Hans Verkuil wrote:

On 04/13/2016 09:44 AM, Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu 


Hi Songjun,

Before this driver can be accepted it has to pass the v4l2-compliance test.
The v4l2-compliance utility is here:

git://linuxtv.org/v4l-utils.git

Compile the utility straight from this repository so you're up to date.

First fix any failures you get when running 'v4l2-compliance', then do the
same when running 'v4l2-compliance -s' (now it is streaming as well) and
finally when running 'v4l2-compliance -f' (streaming and testing all available
formats).

I would like to see the output of 'v4l2-compliance -f' as part of the cover
letter of the next patch series.

Looking at the code I see that it will definitely fail a few tests :-)

Certainly the VIDIOC_CREATE_BUFS support in the queue_setup function is missing.
Read the comments for queue_setup in videobuf2-core.h for more information.

Thank you very much, I will pass the v4l2-compliance test before I send 
the version 2 patch.



Regards,

Hans

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 4/15/2016 00:21, Laurent Pinchart wrote:

Hello Songjun,

Thank you for the patch.

On Wednesday 13 Apr 2016 15:44:19 Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

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

  drivers/media/platform/Kconfig|1 +
  drivers/media/platform/Makefile   |2 +
  drivers/media/platform/atmel/Kconfig  |9 +
  drivers/media/platform/atmel/Makefile |3 +
  drivers/media/platform/atmel/atmel-isc-regs.h |  280 +
  drivers/media/platform/atmel/atmel-isc.c  | 1537 ++
  6 files changed, 1832 insertions(+)
  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

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 201f5c2..1b50ed1 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -110,6 +110,7 @@ source "drivers/media/platform/exynos4-is/Kconfig"
  source "drivers/media/platform/s5p-tv/Kconfig"
  source "drivers/media/platform/am437x/Kconfig"
  source "drivers/media/platform/xilinx/Kconfig"
+source "drivers/media/platform/atmel/Kconfig"

  config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile
b/drivers/media/platform/Makefile index bbb7bd1..ad8f471 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -55,4 +55,6 @@ obj-$(CONFIG_VIDEO_AM437X_VPFE)   += am437x/

  obj-$(CONFIG_VIDEO_XILINX)+= xilinx/

+obj-$(CONFIG_VIDEO_ATMEL_ISC)  += atmel/
+
  ccflags-y += -I$(srctree)/drivers/media/i2c
diff --git a/drivers/media/platform/atmel/Kconfig
b/drivers/media/platform/atmel/Kconfig new file mode 100644
index 000..5ebc4a6
--- /dev/null
+++ b/drivers/media/platform/atmel/Kconfig
@@ -0,0 +1,9 @@
+config VIDEO_ATMEL_ISC
+   tristate "ATMEL Image Sensor Controller (ISC) support"
+   depends on VIDEO_V4L2 && HAS_DMA
+   depends on ARCH_AT91 || COMPILE_TEST


As commented separately, you're missing "depends on COMMON_CLK".


Accept, thank you.


+   select VIDEOBUF2_DMA_CONTIG
+   select REGMAP_MMIO
+   help
+  This module makes the ATMEL Image Sensor Controller available
+  as a v4l2 device.
\ No newline at end of file
diff --git a/drivers/media/platform/atmel/Makefile
b/drivers/media/platform/atmel/Makefile new file mode 100644
index 000..eb8cdbb
--- /dev/null
+++ b/drivers/media/platform/atmel/Makefile
@@ -0,0 +1,3 @@
+# Makefile for ATMEL ISC driver


The makefile isn't limited to the ISC driver, even if that's the only one
currently located in the atmel directory. The atmel-isi driver should be
placed here when it will move away from soc-camera. I would just write
"Makefile for Atmel drivers", or even remove the comment completely.


Accept.
atmel-isi driver will be added into this folder later.
I think "Makefile for Atmel drivers" is better.
Thank you.


+obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h
b/drivers/media/platform/atmel/atmel-isc-regs.h new file mode 100644
index 000..8be9e4a
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc-regs.h
@@ -0,0 +1,280 @@
+


No need for a blank line here.


Accept, thank you.


+#ifndef __ATMEL_ISC_REGS_H
+#define __ATMEL_ISC_REGS_H
+
+#include 


[snip]


+/* ISC Clock Configuration Register */
+#define ISC_CLKCFG  0x0024
+#define ISC_CLKCFG_DIV_SHIFT(n) (n*16)


As n can be an expression, you should enclose it in parentheses, ((n)*16).
Same for tall the macros below.


Accept, thank you.


+#define ISC_CLKCFG_DIV_MASK(n)  GENMASK((n*16 + 7), n*16)
+#define ISC_CLKCFG_SEL_SHIFT(n) (n*16 + 8)
+#define ISC_CLKCFG_SEL_MASK(n)  GENMASK((n*17 + 8), (n*16 + 8))
+


[snip]


diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c new file mode 100644
index 000..4ffbfc9
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -0,0 +1,1537 @@
+/*
+ * Atmel Image Sensor Controller (ISC) driver
+ *
+ * Copyright (C) 2016 Atmel
+ *
+ * Author: Songjun Wu <songjun...@atmel.com>
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * Sensor-->PFE-->WB-->CFA-->CC-->GAM-->CSC-->CBC-->SUB-->RLP-->DMA
+ *

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 4/15/2016 00:21, Laurent Pinchart wrote:

Hello Songjun,

Thank you for the patch.

On Wednesday 13 Apr 2016 15:44:19 Songjun Wu wrote:

Add driver for the Image Sensor Controller. It manages
incoming data from a parallel based CMOS/CCD sensor.
It has an internal image processor, also integrates a
triple channel direct memory access controller master
interface.

Signed-off-by: Songjun Wu 
---

  drivers/media/platform/Kconfig|1 +
  drivers/media/platform/Makefile   |2 +
  drivers/media/platform/atmel/Kconfig  |9 +
  drivers/media/platform/atmel/Makefile |3 +
  drivers/media/platform/atmel/atmel-isc-regs.h |  280 +
  drivers/media/platform/atmel/atmel-isc.c  | 1537 ++
  6 files changed, 1832 insertions(+)
  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

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 201f5c2..1b50ed1 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -110,6 +110,7 @@ source "drivers/media/platform/exynos4-is/Kconfig"
  source "drivers/media/platform/s5p-tv/Kconfig"
  source "drivers/media/platform/am437x/Kconfig"
  source "drivers/media/platform/xilinx/Kconfig"
+source "drivers/media/platform/atmel/Kconfig"

  config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile
b/drivers/media/platform/Makefile index bbb7bd1..ad8f471 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -55,4 +55,6 @@ obj-$(CONFIG_VIDEO_AM437X_VPFE)   += am437x/

  obj-$(CONFIG_VIDEO_XILINX)+= xilinx/

+obj-$(CONFIG_VIDEO_ATMEL_ISC)  += atmel/
+
  ccflags-y += -I$(srctree)/drivers/media/i2c
diff --git a/drivers/media/platform/atmel/Kconfig
b/drivers/media/platform/atmel/Kconfig new file mode 100644
index 000..5ebc4a6
--- /dev/null
+++ b/drivers/media/platform/atmel/Kconfig
@@ -0,0 +1,9 @@
+config VIDEO_ATMEL_ISC
+   tristate "ATMEL Image Sensor Controller (ISC) support"
+   depends on VIDEO_V4L2 && HAS_DMA
+   depends on ARCH_AT91 || COMPILE_TEST


As commented separately, you're missing "depends on COMMON_CLK".


Accept, thank you.


+   select VIDEOBUF2_DMA_CONTIG
+   select REGMAP_MMIO
+   help
+  This module makes the ATMEL Image Sensor Controller available
+  as a v4l2 device.
\ No newline at end of file
diff --git a/drivers/media/platform/atmel/Makefile
b/drivers/media/platform/atmel/Makefile new file mode 100644
index 000..eb8cdbb
--- /dev/null
+++ b/drivers/media/platform/atmel/Makefile
@@ -0,0 +1,3 @@
+# Makefile for ATMEL ISC driver


The makefile isn't limited to the ISC driver, even if that's the only one
currently located in the atmel directory. The atmel-isi driver should be
placed here when it will move away from soc-camera. I would just write
"Makefile for Atmel drivers", or even remove the comment completely.


Accept.
atmel-isi driver will be added into this folder later.
I think "Makefile for Atmel drivers" is better.
Thank you.


+obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h
b/drivers/media/platform/atmel/atmel-isc-regs.h new file mode 100644
index 000..8be9e4a
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc-regs.h
@@ -0,0 +1,280 @@
+


No need for a blank line here.


Accept, thank you.


+#ifndef __ATMEL_ISC_REGS_H
+#define __ATMEL_ISC_REGS_H
+
+#include 


[snip]


+/* ISC Clock Configuration Register */
+#define ISC_CLKCFG  0x0024
+#define ISC_CLKCFG_DIV_SHIFT(n) (n*16)


As n can be an expression, you should enclose it in parentheses, ((n)*16).
Same for tall the macros below.


Accept, thank you.


+#define ISC_CLKCFG_DIV_MASK(n)  GENMASK((n*16 + 7), n*16)
+#define ISC_CLKCFG_SEL_SHIFT(n) (n*16 + 8)
+#define ISC_CLKCFG_SEL_MASK(n)  GENMASK((n*17 + 8), (n*16 + 8))
+


[snip]


diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c new file mode 100644
index 000..4ffbfc9
--- /dev/null
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -0,0 +1,1537 @@
+/*
+ * Atmel Image Sensor Controller (ISC) driver
+ *
+ * Copyright (C) 2016 Atmel
+ *
+ * Author: Songjun Wu 
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * Sensor-->PFE-->WB-->CFA-->CC-->GAM-->CSC-->CBC-->SUB-->RLP-->DMA
+ *
+ * ISC video pipeline integrates the following submodules:
+ * PFE: Parallel Front End to sample the camera sensor input stream
+ * WB:  Programmable white balance in the Bayer domain
+ * CFA: 

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 4/14/2016 22:14, Laurent Pinchart wrote:

Hello Songjun,

On Thursday 14 Apr 2016 13:44:27 Wu, Songjun wrote:

The option 'CONFIG_COMMON_CLK=y' is needed to add to '.config'.
But I do not validate, '.config' will be generated automatically and
overwritten when it is changed.


Your driver's Kconfig entry should then contain "depends on COMMON_CLK".


Accept.
Thank you.

On 4/14/2016 00:01, kbuild test robot wrote:

Hi Songjun,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.6-rc3 next-20160413]
[if your patch is applied to the wrong git tree, please drop us a note to
help improving the system]

url:
https://github.com/0day-ci/linux/commits/Songjun-Wu/atmel-isc-add-driver-> > 
for-Atmel-ISC/20160413-155337 base:   git://linuxtv.org/media_tree.git
master
config: powerpc-allyesconfig (attached as .config)

reproduce:
  wget
  https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/p
  lain/sbin/make.cross -O ~/bin/make.cross chmod +x
  ~/bin/make.cross
  # save the attached .config to linux build tree
  make.cross ARCH=powerpc

All errors (new ones prefixed by >>):
  from include/linux/of.h:21,

  from drivers/media/platform/atmel/atmel-isc.c:27:
 drivers/media/platform/atmel/atmel-isc.c: In function
 'isc_clk_enable':
 include/linux/kernel.h:824:48: error: initialization from incompatible
 pointer type [-Werror=incompatible-pointer-types]>
   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 include/linux/kernel.h:824:48: note: (near initialization for
 'isc_clk')

   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 drivers/media/platform/atmel/atmel-isc.c: In function
 'isc_clk_disable':
 include/linux/kernel.h:824:48: error: initialization from incompatible
 pointer type [-Werror=incompatible-pointer-types]>
   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 include/linux/kernel.h:824:48: note: (near initialization for
 'isc_clk')

   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 drivers/media/platform/atmel/atmel-isc.c: In function
 'isc_clk_is_enabled':
 include/linux/kernel.h:824:48: error: initialization from incompatible
 pointer type [-Werror=incompatible-pointer-types]>
   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 include/linux/kernel.h:824:48: note: (near initialization for
 'isc_clk')

   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expan

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

On 4/14/2016 22:14, Laurent Pinchart wrote:

Hello Songjun,

On Thursday 14 Apr 2016 13:44:27 Wu, Songjun wrote:

The option 'CONFIG_COMMON_CLK=y' is needed to add to '.config'.
But I do not validate, '.config' will be generated automatically and
overwritten when it is changed.


Your driver's Kconfig entry should then contain "depends on COMMON_CLK".


Accept.
Thank you.

On 4/14/2016 00:01, kbuild test robot wrote:

Hi Songjun,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.6-rc3 next-20160413]
[if your patch is applied to the wrong git tree, please drop us a note to
help improving the system]

url:
https://github.com/0day-ci/linux/commits/Songjun-Wu/atmel-isc-add-driver-> > 
for-Atmel-ISC/20160413-155337 base:   git://linuxtv.org/media_tree.git
master
config: powerpc-allyesconfig (attached as .config)

reproduce:
  wget
  https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/p
  lain/sbin/make.cross -O ~/bin/make.cross chmod +x
  ~/bin/make.cross
  # save the attached .config to linux build tree
  make.cross ARCH=powerpc

All errors (new ones prefixed by >>):
  from include/linux/of.h:21,

  from drivers/media/platform/atmel/atmel-isc.c:27:
 drivers/media/platform/atmel/atmel-isc.c: In function
 'isc_clk_enable':
 include/linux/kernel.h:824:48: error: initialization from incompatible
 pointer type [-Werror=incompatible-pointer-types]>
   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 include/linux/kernel.h:824:48: note: (near initialization for
 'isc_clk')

   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 drivers/media/platform/atmel/atmel-isc.c: In function
 'isc_clk_disable':
 include/linux/kernel.h:824:48: error: initialization from incompatible
 pointer type [-Werror=incompatible-pointer-types]>
   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 include/linux/kernel.h:824:48: note: (near initialization for
 'isc_clk')

   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 drivers/media/platform/atmel/atmel-isc.c: In function
 'isc_clk_is_enabled':
 include/linux/kernel.h:824:48: error: initialization from incompatible
 pointer type [-Werror=incompatible-pointer-types]>
   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of
 macro 'container_of'>
  #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)

 ^

 drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of
 macro 'to_isc_clk'>
   struct isc_clk *isc_clk = to_isc_clk(hw);

 ^

 include/linux/kernel.h:824:48: note: (near initialization for
 'isc_clk')

   const typeof( ((type *)0)->member ) *__mptr = (ptr); \

 ^

 drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expan

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

The option 'CONFIG_COMMON_CLK=y' is needed to add to '.config'.
But I do not validate, '.config' will be generated automatically and 
overwritten when it is changed.


On 4/14/2016 00:01, kbuild test robot wrote:

Hi Songjun,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.6-rc3 next-20160413]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Songjun-Wu/atmel-isc-add-driver-for-Atmel-ISC/20160413-155337
base:   git://linuxtv.org/media_tree.git master
config: powerpc-allyesconfig (attached as .config)
reproduce:
 wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
 chmod +x ~/bin/make.cross
 # save the attached .config to linux build tree
 make.cross ARCH=powerpc

All errors (new ones prefixed by >>):

 from include/linux/of.h:21,
 from drivers/media/platform/atmel/atmel-isc.c:27:
drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_enable':
include/linux/kernel.h:824:48: error: initialization from incompatible 
pointer type [-Werror=incompatible-pointer-types]
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk')
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_disable':
include/linux/kernel.h:824:48: error: initialization from incompatible 
pointer type [-Werror=incompatible-pointer-types]
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk')
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_is_enabled':
include/linux/kernel.h:824:48: error: initialization from incompatible 
pointer type [-Werror=incompatible-pointer-types]
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk')
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_recalc_rate':
include/linux/kernel.h:824:48: error: initialization from incompatible 
pointer type 

Re: [PATCH 1/2] [media] atmel-isc: add the Image Sensor Controller code

[Wu, Songjun]

The option 'CONFIG_COMMON_CLK=y' is needed to add to '.config'.
But I do not validate, '.config' will be generated automatically and 
overwritten when it is changed.


On 4/14/2016 00:01, kbuild test robot wrote:

Hi Songjun,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.6-rc3 next-20160413]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/Songjun-Wu/atmel-isc-add-driver-for-Atmel-ISC/20160413-155337
base:   git://linuxtv.org/media_tree.git master
config: powerpc-allyesconfig (attached as .config)
reproduce:
 wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
 chmod +x ~/bin/make.cross
 # save the attached .config to linux build tree
 make.cross ARCH=powerpc

All errors (new ones prefixed by >>):

 from include/linux/of.h:21,
 from drivers/media/platform/atmel/atmel-isc.c:27:
drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_enable':
include/linux/kernel.h:824:48: error: initialization from incompatible 
pointer type [-Werror=incompatible-pointer-types]
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk')
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:247:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_disable':
include/linux/kernel.h:824:48: error: initialization from incompatible 
pointer type [-Werror=incompatible-pointer-types]
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk')
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:280:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_is_enabled':
include/linux/kernel.h:824:48: error: initialization from incompatible 
pointer type [-Werror=incompatible-pointer-types]
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
include/linux/kernel.h:824:48: note: (near initialization for 'isc_clk')
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
^
drivers/media/platform/atmel/atmel-isc.c:55:24: note: in expansion of macro 
'container_of'
 #define to_isc_clk(hw) container_of(hw, struct isc_clk, hw)
^
drivers/media/platform/atmel/atmel-isc.c:295:28: note: in expansion of 
macro 'to_isc_clk'
  struct isc_clk *isc_clk = to_isc_clk(hw);
^
drivers/media/platform/atmel/atmel-isc.c: In function 'isc_clk_recalc_rate':
include/linux/kernel.h:824:48: error: initialization from incompatible 
pointer type