Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Dan Carpenter 
On Sat, Dec 02, 2017 at 08:41:48PM +, Jeremy Sowden wrote:
> On 2017-12-02, at 10:35:06 +, Jeremy Sowden wrote:
> > On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> > > On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > > > -#define DEFAULT_PIPE_INFO \
> > > > -{ \
> > > > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > > > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > > > -   IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > > > -   { 0, 0},/* output system in res 
> > > > */ \
> > > > -   DEFAULT_SHADING_INFO,   /* shading_info */ \
> > > > -   DEFAULT_GRID_INFO,  /* grid_info */ \
> > > > -   0   /* num_invalid_frames 
> > > > */ \
> > > > -}
> > > > +#define DEFAULT_PIPE_INFO ( \
> > >
> > > Why does this have a ( now?  That can't compile can it??
> >
> > It does.
> 
> That was a bit terse: the macros expand to compound-literals, so
> putting parens around them is no different from:
> 
>   #define THREE (3)

Yeah.  Thanks.  I figured it out despite the terseness...  I try review
as fast as I can, so it means you get the stream of conciousness output
that often has mistakes.  Sorry about that.

regards,
dan carpenter

cron job: media_tree daily build: ERRORS

2017-12-02 Thread Hans Verkuil 
This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.

Results of the daily build of media_tree:

date:   Sun Dec  3 05:00:16 CET 2017
media-tree git hash:781b045baefdabf7e0bc9f33672ca830d3db9f27
media_build git hash:   320b9b80ebbf318a67a9479c18a0e4be244c8409
v4l-utils git hash: 26eca33b62f988ecbc4df8134ebdef20f9f75c97
gcc version:i686-linux-gcc (GCC) 7.1.0
sparse version: 0.5.1 (Debian: 0.5.1-2)
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.13.0-164

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-arm-stm32: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: ERRORS
linux-2.6.37.6-i686: ERRORS
linux-2.6.38.8-i686: ERRORS
linux-2.6.39.4-i686: ERRORS
linux-3.0.60-i686: ERRORS
linux-3.1.10-i686: ERRORS
linux-3.2.37-i686: ERRORS
linux-3.3.8-i686: ERRORS
linux-3.4.27-i686: ERRORS
linux-3.5.7-i686: ERRORS
linux-3.6.11-i686: ERRORS
linux-3.7.4-i686: ERRORS
linux-3.8-i686: ERRORS
linux-3.9.2-i686: ERRORS
linux-3.10.1-i686: ERRORS
linux-3.11.1-i686: ERRORS
linux-3.12.67-i686: ERRORS
linux-3.13.11-i686: ERRORS
linux-3.14.9-i686: ERRORS
linux-3.15.2-i686: ERRORS
linux-3.16.7-i686: ERRORS
linux-3.17.8-i686: ERRORS
linux-3.18.7-i686: ERRORS
linux-3.19-i686: ERRORS
linux-4.0.9-i686: ERRORS
linux-4.1.33-i686: ERRORS
linux-4.2.8-i686: ERRORS
linux-4.3.6-i686: ERRORS
linux-4.4.22-i686: ERRORS
linux-4.5.7-i686: ERRORS
linux-4.6.7-i686: ERRORS
linux-4.7.5-i686: ERRORS
linux-4.8-i686: ERRORS
linux-4.9.26-i686: ERRORS
linux-4.10.14-i686: ERRORS
linux-4.11-i686: ERRORS
linux-4.12.1-i686: ERRORS
linux-4.13-i686: ERRORS
linux-4.14-i686: ERRORS
linux-2.6.36.4-x86_64: ERRORS
linux-2.6.37.6-x86_64: ERRORS
linux-2.6.38.8-x86_64: ERRORS
linux-2.6.39.4-x86_64: ERRORS
linux-3.0.60-x86_64: ERRORS
linux-3.1.10-x86_64: ERRORS
linux-3.2.37-x86_64: ERRORS
linux-3.3.8-x86_64: ERRORS
linux-3.4.27-x86_64: ERRORS
linux-3.5.7-x86_64: ERRORS
linux-3.6.11-x86_64: ERRORS
linux-3.7.4-x86_64: ERRORS
linux-3.8-x86_64: ERRORS
linux-3.9.2-x86_64: ERRORS
linux-3.10.1-x86_64: ERRORS
linux-3.11.1-x86_64: ERRORS
linux-3.12.67-x86_64: ERRORS
linux-3.13.11-x86_64: ERRORS
linux-3.14.9-x86_64: ERRORS
linux-3.15.2-x86_64: ERRORS
linux-3.16.7-x86_64: ERRORS
linux-3.17.8-x86_64: ERRORS
linux-3.18.7-x86_64: ERRORS
linux-3.19-x86_64: ERRORS
linux-4.0.9-x86_64: ERRORS
linux-4.1.33-x86_64: ERRORS
linux-4.2.8-x86_64: ERRORS
linux-4.3.6-x86_64: ERRORS
linux-4.4.22-x86_64: ERRORS
linux-4.5.7-x86_64: ERRORS
linux-4.6.7-x86_64: ERRORS
linux-4.7.5-x86_64: ERRORS
linux-4.8-x86_64: ERRORS
linux-4.9.26-x86_64: ERRORS
linux-4.10.14-x86_64: ERRORS
linux-4.11-x86_64: ERRORS
linux-4.12.1-x86_64: ERRORS
linux-4.13-x86_64: ERRORS
linux-4.14-x86_64: ERRORS
apps: OK
spec-git: OK

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Sunday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Sunday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.html

Re: [GIT PULL] SAA716x DVB driver

2017-12-02 Thread Soeren Moch 
On 02.12.2017 20:49, Mauro Carvalho Chehab wrote:
> Em Sat, 2 Dec 2017 18:51:16 +
> Jemma Denson  escreveu:
>
>> Hi Mauro,
>>
>> On 27/11/17 11:24, Mauro Carvalho Chehab wrote:
>>> Em Fri, 24 Nov 2017 17:28:37 +0100
>>> Tycho Lürsen  escreveu:
>>>  
 Hi Mauro,

 afaik the last communication about submission of this driver was about
 two months ago.

 This driver is important to me, because I own several TurboSight cards
 that are saa716x based. I want to submit a patch that supports my cards.
 Of course I can only do so when you accept this driver in the first place.

 Any chance you and Sören agree about how to proceed about this driver
 anytime soon?  
>>> If we can reach an agreement about what should be done for the driver
>>> to be promoted from staging some day, I'll merge it. Otherwise,
>>> it can be kept maintained out of tree. This driver has been maintained
>>> OOT for a very long time, and it seems that people were happy with
>>> that, as only at the second half of this year someone is requesting
>>> to merge it.
>>>
>>> So, while I agree that the best is to merge it upstream and
>>> address the issues that made it OOT for a long time, we shouldn't
>>> rush it with the risk of doing more harm than good.
>>>
>>> Thanks,
>>> Mauro  
>> Would I be correct in thinking the main blocker to this is the *_ff features
>> used by the S2-6400 card? There's plenty of other cards using this chipset
>> that don't need that part.
>>
>> Would a solution for now to be a driver with the ff components stripped out,
>> and then the ff API work can be done later when / if there's any interest?
> Works for me. In such case (and provided that the driver without *_ff are
> in good shape), we could merge it under drivers/media (instead of merging
> it on staging).
All the entries in the TODO file are not specific for saa716x_ff.
>> I guess a problem would be finding a maintainer, I'm happy to put together
>> a stripped down driver just supporting the TBS card I use (I already have
>> one I use with dkms), but I'm not sure I have the time or knowledge of this
>> chipset to be a maintainer.
There is chipset specific stuff to fix, especially irq handling.
> As we're talking more about touching at uAPI, probably it doesn't require
> chipsed knowledge. Only time and interest on doing it.
>
> Please sync with Soeren. Perhaps if you both could help on it, it would
> make the task easier.
As I already wrote to different people off-list: I'm happy to support
more cards with the saa7160 bridge and maintain these in this driver. As
hobbyist programmer this of course makes no sense to me, if the hardware
I own (S2-6400) is not supported.

Regards,
Soeren
>> Unfortunately my workplace is phasing out
>> these cards otherwise I'd try and get them to sponsor me rather than do it
>> on my own time!
> Yeah, getting sponsored to do it would make things easier.
>
> Thanks,
> Mauro

Re: [GIT PULL] SAA716x DVB driver

2017-12-02 Thread Soeren Moch 
Mauro,

On 28.11.2017 19:35, Mauro Carvalho Chehab wrote:
> Em Mon, 25 Sep 2017 00:17:00 +0200
> Soeren Moch  escreveu:
>
>>>  What I'm saying is that,
>>> if we're adding it on staging, we need to have a plan to reimplement
>>> it to whatever API replaces the DVB video API, as this API likely
>>> won't stay upstream much longer.  
>> AFAIK it is not usual linux policy to remove existing drivers
>> with happy users and even someone who volunteered to
>> maintain this.
> The usual Linux policy doesn't apply to staging. The goal of staging is
> to add drivers that have problems, but are fixable, and whose someone
> is working to solve those issues. 
It is totally clear to me what staging is for. Therefore I submitted
this driver for staging.

I was talking about the ttpci driver. This drivers lives in
drivers/media/pci/ttpci, not in staging, it implements the DVB
audio/video/osd API, works great and has happy users. So the DVB video
API _is already_ part of the linux userspace API. This pull request does
not add anything to the already available in-kernel video API.
> The staging policies include adding a TODO file describing the problems
> that should be solved for the driver to be promoted. If such problems
> aren't solved, the driver can be removed.
Yes, of course. Therefore I added such TODO file and promised to solve
these issues.
>
> For example, this year, we removed some lirc staging drivers because
> no developers were interested (and/or had the hardware) to convert
> them to use the RC core (with is a Kernel's internal API).
>
> In the case of saa716x, the issue is that it uses a deprecated
> and undocumented userspace API, with is a way more serious issue.
Unfortunately there is no other API available which provides _all_ the
functionality of DVB audio/video/osd. And the TT S2-6400 hardware is
developed in a way, so that it is as similar as possible to ttpci cards.
In order that the vdr application can easily make use of it. So it is
not surprising, that the saa716x_ff driver needs to implement the same
APIs as the ttpci driver.
> I'm ok to add this driver to staging if we can agree on what
> should be fixed, and if someone commits to try fixing it, knowing,
> in advance, that, if it doesn't get fixed on a reasonable time, it
> can be removed on later Kernel versions.
I already agreed on all these points. I already promised to fix whatever
is necessary to get this driver out of staging, as long as the userspace
API remains the same so that vdr continues to work with these cards. And
once again, this API already is part of the kernel and not specific for
this driver.


The driver version in this pull request does not build anymore with
linux-4.15-rc1. Shall I provide a new pull request with this fixed?

Thanks,
Soeren

[PATCH v4 3/3] media: atomisp: delete empty default struct values.

2017-12-02 Thread Jeremy Sowden 
Removing zero-valued struct-members left a number of the default
struct-values empty.  These values have now been removed.

Signed-off-by: Jeremy Sowden 
---
 .../atomisp/pci/atomisp2/css2400/ia_css_pipe.h |  1 -
 .../atomisp/pci/atomisp2/css2400/ia_css_types.h|  1 -
 .../isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h |  5 ---
 .../kernels/sdis/common/ia_css_sdis_common_types.h | 41 --
 .../isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c   |  2 +-
 .../runtime/binary/interface/ia_css_binary.h   |  8 -
 .../isp_param/interface/ia_css_isp_param_types.h   | 12 ---
 .../media/atomisp/pci/atomisp2/css2400/sh_css.c|  9 +++--
 .../atomisp/pci/atomisp2/css2400/sh_css_legacy.h   |  4 ---
 .../atomisp/pci/atomisp2/css2400/sh_css_metrics.h  | 10 --
 10 files changed, 12 insertions(+), 81 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe.h 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe.h
index 522238ec8899..f6870fa7a18c 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe.h
@@ -163,7 +163,6 @@ struct ia_css_pipe {
 #define IA_CSS_DEFAULT_PIPE \
 (struct ia_css_pipe) { \
.config = DEFAULT_PIPE_CONFIG, \
-   .extra_config   = DEFAULT_PIPE_EXTRA_CONFIG, \
.info   = DEFAULT_PIPE_INFO, \
.mode   = IA_CSS_PIPE_ID_ACC, /* (pipe_id) */ \
.pipeline   = DEFAULT_PIPELINE, \
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_types.h 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_types.h
index a0ddf989fadf..259ab3f074ba 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_types.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_types.h
@@ -407,7 +407,6 @@ struct ia_css_grid_info {
 /* defaults for ia_css_grid_info structs */
 #define DEFAULT_GRID_INFO \
 (struct ia_css_grid_info) { \
-   .s3a_grid   = DEFAULT_3A_GRID_INFO, \
.dvs_grid   = DEFAULT_DVS_GRID_INFO, \
.vamem_type = IA_CSS_VAMEM_TYPE_1 \
 }
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h
index 988a0e272225..63e70669f085 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h
@@ -98,11 +98,6 @@ struct ia_css_3a_grid_info {
 };
 
 
-#define DEFAULT_3A_GRID_INFO \
-(struct ia_css_3a_grid_info) { \
-}
-
-
 /* This struct should be split into 3, for AE, AWB and AF.
  * However, that will require driver/ 3A lib modifications.
  */
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h
index e867dadd8a83..381e5730d405 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h
@@ -41,11 +41,6 @@ struct ia_css_sdis_info {
uint32_t deci_factor_log2;
 };
 
-#define IA_CSS_DEFAULT_SDIS_INFO \
-(struct ia_css_sdis_info) { \
-}
-
-
 /* DVS statistics grid
  *
  *  ISP block: SDVS1 (DIS/DVS Support for DIS/DVS ver.1 (2-axes))
@@ -200,37 +195,15 @@ struct ia_css_dvs_stat_grid_info {
 
 /* DVS statistics generated by accelerator default grid info
  */
-#define DEFAULT_DVS_STAT_PUBLIC_DVS_GLOBAL_CFG \
-(struct dvs_stat_public_dvs_global_cfg) { \
-}
-
-#define DEFAULT_DVS_STAT_PUBLIC_DVS_GRD_CFG \
-(struct dvs_stat_public_dvs_grd_cfg) { \
-}
-
-#define DEFAULT_DVS_STAT_PUBLIC_DVS_LEVEL_FE_ROI_CFG(X_START) \
-(struct dvs_stat_public_dvs_level_fe_roi_cfg) { \
-   .x_start = X_START, \
-}
-
-#define DEFAULT_DVS_STAT_GRID_INFO \
-(struct ia_css_dvs_stat_grid_info) { \
-   .dvs_gbl_cfg = DEFAULT_DVS_STAT_PUBLIC_DVS_GLOBAL_CFG, \
-   .grd_cfg = { \
-   DEFAULT_DVS_STAT_PUBLIC_DVS_GRD_CFG, \
-   DEFAULT_DVS_STAT_PUBLIC_DVS_GRD_CFG, \
-   DEFAULT_DVS_STAT_PUBLIC_DVS_GRD_CFG \
-   }, \
-   .fe_roi_cfg = { \
-   DEFAULT_DVS_STAT_PUBLIC_DVS_LEVEL_FE_ROI_CFG(0), \
-   DEFAULT_DVS_STAT_PUBLIC_DVS_LEVEL_FE_ROI_CFG(4), \
-   DEFAULT_DVS_STAT_PUBLIC_DVS_LEVEL_FE_ROI_CFG(0), \
-   } \
-}
-
 #define DEFAULT_DVS_GRID_INFO \
 (union ia_css_dvs_grid_u) { \
-   .dvs_stat_grid_info = DEFAULT_DVS_STAT_GRID_INFO, \
+   .dvs_stat_grid_info = (struct ia_css_dvs_stat_grid_info) { \
+   .fe_roi_cfg = { \
+   [1] = (struct

[PATCH v4 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Jeremy Sowden 
The CSS API uses a lot of nested anonymous structs defined in object
macros to assign default values to its data-structures.  These have been
changed to use compound-literals and designated initializers to make
them more comprehensible and less fragile.

The compound-literals can also be used in assignment, which means we can
get rid of some temporary variables whose only purpose is to be
initialized by one of these anonymous structs and then serve as the
rvalue in an assignment expression.

Signed-off-by: Jeremy Sowden 
---
 .../hive_isp_css_common/input_formatter_global.h   |  26 ++--
 .../pci/atomisp2/css2400/ia_css_frame_public.h |  46 ---
 .../atomisp/pci/atomisp2/css2400/ia_css_pipe.h | 150 +++--
 .../pci/atomisp2/css2400/ia_css_pipe_public.h  | 136 +--
 .../atomisp/pci/atomisp2/css2400/ia_css_types.h|  78 +--
 .../isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h | 134 +-
 .../kernels/sdis/common/ia_css_sdis_common_types.h |  86 +---
 .../isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c   |   3 +-
 .../runtime/binary/interface/ia_css_binary.h   | 140 +--
 .../atomisp2/css2400/runtime/binary/src/binary.c   |   3 +-
 .../isp_param/interface/ia_css_isp_param_types.h   |  19 ++-
 .../runtime/pipeline/interface/ia_css_pipeline.h   |  30 ++---
 .../css2400/runtime/pipeline/src/pipeline.c|   7 +-
 .../media/atomisp/pci/atomisp2/css2400/sh_css.c|  22 +--
 .../atomisp/pci/atomisp2/css2400/sh_css_legacy.h   |  16 +--
 .../atomisp/pci/atomisp2/css2400/sh_css_metrics.h  |  26 ++--
 16 files changed, 518 insertions(+), 404 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
index 5654d911db65..4ce352986296 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
@@ -108,19 +108,19 @@ struct input_formatter_cfg_s {
 };
 
 #define DEFAULT_IF_CONFIG \
-{ \
-   0,  /* start_line */\
-   0,  /* start_column */\
-   0,  /* left_padding */\
-   0,  /* cropped_height */\
-   0,  /* cropped_width */\
-   0,  /* deinterleaving */\
-   0,  /*.buf_vecs */\
-   0,  /* buf_start_index */\
-   0,  /* buf_increment */\
-   0,  /* buf_eol_offset */\
-   false,  /* is_yuv420_format */\
-   false   /* block_no_reqs */\
+(struct input_formatter_cfg_s) { \
+   .start_line = 0, \
+   .start_column   = 0, \
+   .left_padding   = 0, \
+   .cropped_height = 0, \
+   .cropped_width  = 0, \
+   .deinterleaving = 0, \
+   .buf_vecs   = 0, \
+   .buf_start_index= 0, \
+   .buf_increment  = 0, \
+   .buf_eol_offset = 0, \
+   .is_yuv420_format   = false, \
+   .block_no_reqs  = false \
 }
 
 extern const hrt_address HIVE_IF_SRST_ADDRESS[N_INPUT_FORMATTER_ID];
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
index ba7a076c3afa..634eca325f07 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
@@ -121,15 +121,19 @@ struct ia_css_frame_info {
 };
 
 #define IA_CSS_BINARY_DEFAULT_FRAME_INFO \
-{ \
-   {0,  /* width */ \
-0}, /* height */ \
-   0,   /* padded_width */ \
-   IA_CSS_FRAME_FORMAT_NUM, /* format */ \
-   0,   /* raw_bit_depth */ \
-   IA_CSS_BAYER_ORDER_NUM,  /* raw_bayer_order */ \
-   {0,   /*start col */ \
-0},   /*start line */ \
+(struct ia_css_frame_info) { \
+   .res= (struct ia_css_resolution) { \
+   .width = 0, \
+   .height = 0 \
+   }, \
+   .padded_width   = 0, \
+   .format = IA_CSS_FRAME_FORMAT_NUM,  \
+   .raw_bit_depth  = 0, \
+   .raw_bayer_order= IA_CSS_BAYER_ORDER_NUM, \
+   .crop_info  = (struct ia_css_crop_info) { \
+   .start_column   = 0, \
+   .start_line = 0 \
+   }, \
 }
 
 /**
@@ -190,18 +194,18 @@ struct ia_css_frame {
 };
 
 #define DEFAULT_FRAME \
-{ \
-   IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /*

[PATCH v4 2/3] media: atomisp: delete zero-valued struct members.

2017-12-02 Thread Jeremy Sowden 
A lot of the members of the default struct values used by the CSS API
were explicitly initialized to zero values.  Designated initializers
have allowed these members to be removed.

Signed-off-by: Jeremy Sowden 
---
 .../hive_isp_css_common/input_formatter_global.h   |  16 ---
 .../pci/atomisp2/css2400/ia_css_frame_public.h |  17 
 .../atomisp/pci/atomisp2/css2400/ia_css_pipe.h |  44 +
 .../pci/atomisp2/css2400/ia_css_pipe_public.h  |  78 ---
 .../atomisp/pci/atomisp2/css2400/ia_css_types.h|  47 +
 .../isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h | 109 -
 .../kernels/sdis/common/ia_css_sdis_common_types.h |  34 ---
 .../runtime/binary/interface/ia_css_binary.h   |  88 ++---
 .../isp_param/interface/ia_css_isp_param_types.h   |   9 --
 .../runtime/pipeline/interface/ia_css_pipeline.h   |   6 --
 .../atomisp/pci/atomisp2/css2400/sh_css_legacy.h   |   7 --
 .../atomisp/pci/atomisp2/css2400/sh_css_metrics.h  |   7 --
 12 files changed, 20 insertions(+), 442 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
index 4ce352986296..7558f4964313 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/input_formatter_global.h
@@ -107,22 +107,6 @@ struct input_formatter_cfg_s {
uint32_tblock_no_reqs;
 };
 
-#define DEFAULT_IF_CONFIG \
-(struct input_formatter_cfg_s) { \
-   .start_line = 0, \
-   .start_column   = 0, \
-   .left_padding   = 0, \
-   .cropped_height = 0, \
-   .cropped_width  = 0, \
-   .deinterleaving = 0, \
-   .buf_vecs   = 0, \
-   .buf_start_index= 0, \
-   .buf_increment  = 0, \
-   .buf_eol_offset = 0, \
-   .is_yuv420_format   = false, \
-   .block_no_reqs  = false \
-}
-
 extern const hrt_address HIVE_IF_SRST_ADDRESS[N_INPUT_FORMATTER_ID];
 extern const hrt_data HIVE_IF_SRST_MASK[N_INPUT_FORMATTER_ID];
 extern const uint8_t HIVE_IF_SWITCH_CODE[N_INPUT_FORMATTER_ID];
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
index 634eca325f07..0beb7347a4f3 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_frame_public.h
@@ -122,18 +122,8 @@ struct ia_css_frame_info {
 
 #define IA_CSS_BINARY_DEFAULT_FRAME_INFO \
 (struct ia_css_frame_info) { \
-   .res= (struct ia_css_resolution) { \
-   .width = 0, \
-   .height = 0 \
-   }, \
-   .padded_width   = 0, \
.format = IA_CSS_FRAME_FORMAT_NUM,  \
-   .raw_bit_depth  = 0, \
.raw_bayer_order= IA_CSS_BAYER_ORDER_NUM, \
-   .crop_info  = (struct ia_css_crop_info) { \
-   .start_column   = 0, \
-   .start_line = 0 \
-   }, \
 }
 
 /**
@@ -196,16 +186,9 @@ struct ia_css_frame {
 #define DEFAULT_FRAME \
 (struct ia_css_frame) { \
.info   = IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
-   .data   = 0, \
-   .data_bytes = 0, \
.dynamic_queue_id   = SH_CSS_INVALID_QUEUE_ID, \
.buf_type   = IA_CSS_BUFFER_TYPE_INVALID, \
.flash_state= IA_CSS_FRAME_FLASH_STATE_NONE, \
-   .exp_id = 0, \
-   .isp_config_id  = 0, \
-   .valid  = false, \
-   .contiguous = false, \
-   .planes = { 0 } \
 }
 
 /* @brief Fill a frame with zeros
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe.h 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe.h
index fa5e3f6b85a8..522238ec8899 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe.h
@@ -39,16 +39,11 @@ struct ia_css_preview_settings {
struct ia_css_pipe *acc_pipe;
 };
 
-#define IA_CSS_DEFAULT_PREVIEW_SETTINGS  \
+#define IA_CSS_DEFAULT_PREVIEW_SETTINGS \
 (struct ia_css_preview_settings) { \
.copy_binary= IA_CSS_BINARY_DEFAULT_SETTINGS, \
.preview_binary = IA_CSS_BINARY_DEFAULT_SETTINGS, \
.vf_pp_binary   = IA_CSS_BINARY_DEFAULT_SETTINGS, \
-   .delay_frames   = { NULL }, \
-   .tnr_frames = {

[PATCH v4 0/3] media: atomisp: clean up of data-structure initialization in the CSS API

2017-12-02 Thread Jeremy Sowden 
v4.

  Removed the parens around the compound-literals in the macro
  definitions.  They were superfluous and the errors reported by
  checkpatch.pl appear to have been false positives resulting from a
  bug.

v3.

  Rebased on to git://linuxtv.org/media_tree.git.  Patch-set applies cleanly to
  linux-next as of 20171201.

v2.

  Fixed a couple of bugs and addressed checkpatch errors.

v1.

  The CSS API uses a lot of nested anonymous structs defined in object
  macros to assign default values to its data-structures.  These have
  been changed to use compound-literals and designated initializers to
  make them more comprehensible and less fragile.

  The compound-literals can also be used in assignment, which made it
  possible get rid of some temporary variables whose only purpose is to
  be initialized by one of these anonymous structs and then serve as the
  rvalue in an assignment expression.

  The designated initializers also allow the removal of lots of
  struct-members initialized to zero values.

  I made the changes in three stages: firstly, I converted the default
  values to compound-literals and designated initializers and removed
  the temporary variables; secondly, I removed the zero-valued
  struct-members; finally, I removed some structs which had become
  empty.

Jeremy Sowden (3):
  media: atomisp: convert default struct values to use compound-literals
with designated initializers.
  media: atomisp: delete zero-valued struct members.
  media: atomisp: delete empty default struct values.

 .../hive_isp_css_common/input_formatter_global.h   |  16 ---
 .../pci/atomisp2/css2400/ia_css_frame_public.h |  29 ++
 .../atomisp/pci/atomisp2/css2400/ia_css_pipe.h | 113 -
 .../pci/atomisp2/css2400/ia_css_pipe_public.h  | 108 +++-
 .../atomisp/pci/atomisp2/css2400/ia_css_types.h|  64 +++-
 .../isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h |  50 +
 .../kernels/sdis/common/ia_css_sdis_common_types.h |  31 ++
 .../isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c   |   3 +-
 .../runtime/binary/interface/ia_css_binary.h   |  88 ++--
 .../atomisp2/css2400/runtime/binary/src/binary.c   |   3 +-
 .../isp_param/interface/ia_css_isp_param_types.h   |  10 --
 .../runtime/pipeline/interface/ia_css_pipeline.h   |  24 ++---
 .../css2400/runtime/pipeline/src/pipeline.c|   7 +-
 .../media/atomisp/pci/atomisp2/css2400/sh_css.c|  31 ++
 .../atomisp/pci/atomisp2/css2400/sh_css_legacy.h   |  11 --
 .../atomisp/pci/atomisp2/css2400/sh_css_metrics.h  |  21 
 16 files changed, 116 insertions(+), 493 deletions(-)


base-commit: 781b045baefdabf7e0bc9f33672ca830d3db9f27
-- 
2.15.0

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Jeremy Sowden 
On 2017-12-02, at 20:41:48 +, Jeremy Sowden wrote:
> On 2017-12-02, at 10:35:06 +, Jeremy Sowden wrote:
> > On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> > > On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > > > -#define DEFAULT_PIPE_INFO \
> > > > -{ \
> > > > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > > > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > > > -   IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > > > -   { 0, 0},/* output system in res 
> > > > */ \
> > > > -   DEFAULT_SHADING_INFO,   /* shading_info */ \
> > > > -   DEFAULT_GRID_INFO,  /* grid_info */ \
> > > > -   0   /* num_invalid_frames 
> > > > */ \
> > > > -}
> > > > +#define DEFAULT_PIPE_INFO ( \
> > >
> > > Why does this have a ( now?  That can't compile can it??
> > >
> > > > +   (struct ia_css_pipe_info) { \
> > > > +   .output_info= 
> > > > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > > > +   .vf_output_info = 
> > > > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > > > +   .raw_output_info= 
> > > > IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> > > > +   .output_system_in_res_info  = { 0, 0 }, \
> > > > +   .shading_info   = DEFAULT_SHADING_INFO, 
> > > > \
> > > > +   .grid_info  = DEFAULT_GRID_INFO, \
> > > > +   .num_invalid_frames = 0 \
> > > > +   } \
> > > > +)
> >
> > Checkpatch got quite shouty, e.g.:
> >
> >   ERROR: Macros with complex values should be enclosed in parentheses
> >   #826: FILE: 
> > drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h:215:
> >   +#define DEFAULT_DVS_STAT_PUBLIC_DVS_GLOBAL_CFG \
> >   +(struct dvs_stat_public_dvs_global_cfg) { \
> >   +   .kappa  = 0, \
> >   +   .match_shift= 0, \
> >   +   .ybin_mode  = 0, \
> >   +}
> >
> > so I just wrapped all of them.
>
> I've run checkpatch.pl against the unparenthesized patches, and it
> only objects to some of the macros.  I've also taken a look at the
> source of checkpatch.pl itself, and at first glance it appears that it
> should accept them.  I'll see if I can work out why it's complaining.

I think I've found a bug in checkpatch.pl.  I'll remove the parentheses
from my patches 'cause the error-reportd appear to be bogus, and pass on
my findings.

J.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Jeremy Sowden 
On 2017-12-02, at 10:35:06 +, Jeremy Sowden wrote:
> On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> > On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > > -#define DEFAULT_PIPE_INFO \
> > > -{ \
> > > - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > > - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > > - IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > > - { 0, 0},/* output system in res */ \
> > > - DEFAULT_SHADING_INFO,   /* shading_info */ \
> > > - DEFAULT_GRID_INFO,  /* grid_info */ \
> > > - 0   /* num_invalid_frames */ \
> > > -}
> > > +#define DEFAULT_PIPE_INFO ( \
> >
> > Why does this have a ( now?  That can't compile can it??
>
> It does.

That was a bit terse: the macros expand to compound-literals, so
putting parens around them is no different from:

  #define THREE (3)

It's also superfluous, of course.

> > > + (struct ia_css_pipe_info) { \
> > > + .output_info= 
> > > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > > + .vf_output_info = 
> > > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > > + .raw_output_info= 
> > > IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> > > + .output_system_in_res_info  = { 0, 0 }, \
> > > + .shading_info   = DEFAULT_SHADING_INFO, \
> > > + .grid_info  = DEFAULT_GRID_INFO, \
> > > + .num_invalid_frames = 0 \
> > > + } \
> > > +)
>
> Checkpatch got quite shouty, e.g.:
>
>   ERROR: Macros with complex values should be enclosed in parentheses
>   #826: FILE: 
> drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h:215:
>   +#define DEFAULT_DVS_STAT_PUBLIC_DVS_GLOBAL_CFG \
>   +(struct dvs_stat_public_dvs_global_cfg) { \
>   +   .kappa  = 0, \
>   +   .match_shift= 0, \
>   +   .ybin_mode  = 0, \
>   +}
>
> so I just wrapped all of them.

I've run checkpatch.pl against the unparenthesized patches, and it only
objects to some of the macros.  I've also taken a look at the source of
checkpatch.pl itself, and at first glance it appears that it should
accept them.  I'll see if I can work out why it's complaining.

J.


signature.asc
Description: PGP signature

Re: [GIT PULL] SAA716x DVB driver

2017-12-02 Thread Mauro Carvalho Chehab 
Em Sat, 2 Dec 2017 18:51:16 +
Jemma Denson  escreveu:

> Hi Mauro,
> 
> On 27/11/17 11:24, Mauro Carvalho Chehab wrote:
> > Em Fri, 24 Nov 2017 17:28:37 +0100
> > Tycho Lürsen  escreveu:
> >  
> >> Hi Mauro,
> >>
> >> afaik the last communication about submission of this driver was about
> >> two months ago.
> >>
> >> This driver is important to me, because I own several TurboSight cards
> >> that are saa716x based. I want to submit a patch that supports my cards.
> >> Of course I can only do so when you accept this driver in the first place.
> >>
> >> Any chance you and Sören agree about how to proceed about this driver
> >> anytime soon?  
> > If we can reach an agreement about what should be done for the driver
> > to be promoted from staging some day, I'll merge it. Otherwise,
> > it can be kept maintained out of tree. This driver has been maintained
> > OOT for a very long time, and it seems that people were happy with
> > that, as only at the second half of this year someone is requesting
> > to merge it.
> >
> > So, while I agree that the best is to merge it upstream and
> > address the issues that made it OOT for a long time, we shouldn't
> > rush it with the risk of doing more harm than good.
> >
> > Thanks,
> > Mauro  
> 
> Would I be correct in thinking the main blocker to this is the *_ff features
> used by the S2-6400 card? There's plenty of other cards using this chipset
> that don't need that part.
> 
> Would a solution for now to be a driver with the ff components stripped out,
> and then the ff API work can be done later when / if there's any interest?

Works for me. In such case (and provided that the driver without *_ff are
in good shape), we could merge it under drivers/media (instead of merging
it on staging).

> I guess a problem would be finding a maintainer, I'm happy to put together
> a stripped down driver just supporting the TBS card I use (I already have
> one I use with dkms), but I'm not sure I have the time or knowledge of this
> chipset to be a maintainer.

As we're talking more about touching at uAPI, probably it doesn't require
chipsed knowledge. Only time and interest on doing it.

Please sync with Soeren. Perhaps if you both could help on it, it would
make the task easier.

> Unfortunately my workplace is phasing out
> these cards otherwise I'd try and get them to sponsor me rather than do it
> on my own time!

Yeah, getting sponsored to do it would make things easier.

Thanks,
Mauro

Re: [GIT PULL] SAA716x DVB driver

2017-12-02 Thread Jemma Denson 

Hi Mauro,

On 27/11/17 11:24, Mauro Carvalho Chehab wrote:

Em Fri, 24 Nov 2017 17:28:37 +0100
Tycho Lürsen  escreveu:


Hi Mauro,

afaik the last communication about submission of this driver was about
two months ago.

This driver is important to me, because I own several TurboSight cards
that are saa716x based. I want to submit a patch that supports my cards.
Of course I can only do so when you accept this driver in the first place.

Any chance you and Sören agree about how to proceed about this driver
anytime soon?

If we can reach an agreement about what should be done for the driver
to be promoted from staging some day, I'll merge it. Otherwise,
it can be kept maintained out of tree. This driver has been maintained
OOT for a very long time, and it seems that people were happy with
that, as only at the second half of this year someone is requesting
to merge it.

So, while I agree that the best is to merge it upstream and
address the issues that made it OOT for a long time, we shouldn't
rush it with the risk of doing more harm than good.

Thanks,
Mauro


Would I be correct in thinking the main blocker to this is the *_ff features
used by the S2-6400 card? There's plenty of other cards using this chipset
that don't need that part.

Would a solution for now to be a driver with the ff components stripped out,
and then the ff API work can be done later when / if there's any interest?

I guess a problem would be finding a maintainer, I'm happy to put together
a stripped down driver just supporting the TBS card I use (I already have
one I use with dkms), but I'm not sure I have the time or knowledge of this
chipset to be a maintainer. Unfortunately my workplace is phasing out
these cards otherwise I'd try and get them to sponsor me rather than do it
on my own time!


Jemma.

Re: [PATCH v2 2/3] media: atomisp: delete zero-valued struct members.

2017-12-02 Thread Jeremy Sowden 
On 2017-12-01, at 17:41:50 +, Alan Cox wrote:
> > --- 
> > a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe_public.h
> > +++ 
> > b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_pipe_public.h
> > @@ -152,14 +152,6 @@ struct ia_css_pipe_config {
> >  };
> >
>
>
> Thani you that's a really good cleanup.

Thanks. :)

J.


signature.asc
Description: PGP signature

Re: [PATCH v12 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

2017-12-02 Thread Niklas Söderlund 
Hej Sakari,

Thanks for your feedback.

On 2017-12-02 16:05:08 +0200, Sakari Ailus wrote:
> Hejssan,
> 
> On Sat, Dec 02, 2017 at 12:08:21PM +0100, Niklas Söderlund wrote:
> ...
> > > > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> > > > +{
> > > > +   struct device_node *ep;
> > > > +   struct v4l2_fwnode_endpoint v4l2_ep;
> > > > +   int ret;
> > > > +
> > > > +   ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > > > +   if (!ep) {
> > > > +   dev_err(priv->dev, "Not connected to subdevice\n");
> > > > +   return -EINVAL;
> > > > +   }
> > > > +
> > > > +   ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), 
> > > > _ep);
> > > > +   if (ret) {
> > > > +   dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> > > > +   of_node_put(ep);
> > > > +   return -EINVAL;
> > > > +   }
> > > > +
> > > > +   ret = rcar_csi2_parse_v4l2(priv, _ep);
> > > > +   if (ret)
> > > > +   return ret;
> > > > +
> > > > +   priv->remote.match.fwnode.fwnode =
> > > > +   fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > > 
> > > To continue the discussion from v10 --- how does this work? The V4L2 async
> > > framework does still matching for the node of the device, not the 
> > > endpoint.
> > > 
> > > My suggestion is to handle this in match_fwnode until all drivers have 
> > > been
> > > converted. The V4L2 fwnode helper should be changed as well, then you 
> > > could
> > > use it here, too.
> > 
> > I agree that the V4L2 async framework should be changed to work with 
> > endpoints and not the node of the device. I also agree on how this 
> > change should be introduced.
> > 
> > But I don't feel that this change of the framework needs to block this 
> > patch-set. Once the framework is updated to work with endpoints it 
> > should be a trivial patch to change rcar-csi2 to use the new helper. Do 
> > you agree whit this or do you feel that this patch-set should depend on 
> > such change of the framework?
> 
> Without changes to the framework, I don't think this would work since the
> async framework (or individual drivers) will assign the device's fwnode,
> not that of the endpoint, to the fwnode against which to match the async
> sub-device.
> 
> Therefore you'll need these changes for this driver to work. Or if you
> introduce a sub-device driver that uses endpoints as well, then we have two
> non-interoperable sets of ISP (or bridge) and sub-device drivers. That'd be
> quite undesirable.

Such a driver is already upstream, the adv748x driver register its 
subdevices using endpoints rather then the node of the device itself.


int adv748x_csi2_init(...)
{

...

/* Ensure that matching is based upon the endpoint fwnodes */
tx->sd.fwnode = of_fwnode_handle(ep);

...
}


A related patch to when the adv748x driver was unstreamed where 
'v4l2-async: Match parent devices' by Kieran to make this change in
behavior not to cause the non-interoperable sets your mention. It seems 
however that that change have fallen thru the cracks.

> 
> Or, if you don't care whether it'd work with your use case right now, you
> could use the helper function without changes. :-)

The adv748x is the primary use-case for the rcar-csi2 driver at the 
moment. So I must either keep this workaround in the driver or make this 
patch-set depend on future framework fixes. I would prefer to be able to 
use the helper as it makes the driver less complex. At the same time I 
don't want yet another framework change as a blocker for this patch-set 
:-)

Once I'm back from my short vacation I will give the framework update a 
try and if it turns out OK I will make this patch-set dependent on those 
changes and squash in my patch which converts rcar-csi2 to use the 
helper which is already done in preparation of future framework updates.

If it turns out the changes needed are complex or get stuck in review I 
would prefer if it was possible to move forward with the workaround in 
the driver for now and move it to the helper once it's available. Do 
this sound like a agreeable plan to you?

> 
> > > > +{
> > > > +   const struct soc_device_attribute *attr;
> > > > +   struct rcar_csi2 *priv;
> > > > +   unsigned int i;
> > > > +   int ret;
> > > > +
> > > > +   priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> > > > +   if (!priv)
> > > > +   return -ENOMEM;
> > > > +
> > > > +   priv->info = of_device_get_match_data(>dev);
> > > > +
> > > > +   /* r8a7795 ES1.x behaves different then ES2.0+ but no own 
> > > > compat */
> > > > +   attr = soc_device_match(r8a7795es1);
> > > > +   if (attr)
> > > > +   priv->info = attr->data;
> > > > +
> > > > +   priv->dev = >dev;
> > > > +
> > > > +   mutex_init(>lock);
> > > > +   priv->stream_count = 0;
> > > > +
> > > > +   ret =

Re: [PATCH v12 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

2017-12-02 Thread Sakari Ailus 
Hejssan,

On Sat, Dec 02, 2017 at 12:08:21PM +0100, Niklas Söderlund wrote:
...
> > > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> > > +{
> > > + struct device_node *ep;
> > > + struct v4l2_fwnode_endpoint v4l2_ep;
> > > + int ret;
> > > +
> > > + ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > > + if (!ep) {
> > > + dev_err(priv->dev, "Not connected to subdevice\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), _ep);
> > > + if (ret) {
> > > + dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> > > + of_node_put(ep);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = rcar_csi2_parse_v4l2(priv, _ep);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + priv->remote.match.fwnode.fwnode =
> > > + fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > 
> > To continue the discussion from v10 --- how does this work? The V4L2 async
> > framework does still matching for the node of the device, not the endpoint.
> > 
> > My suggestion is to handle this in match_fwnode until all drivers have been
> > converted. The V4L2 fwnode helper should be changed as well, then you could
> > use it here, too.
> 
> I agree that the V4L2 async framework should be changed to work with 
> endpoints and not the node of the device. I also agree on how this 
> change should be introduced.
> 
> But I don't feel that this change of the framework needs to block this 
> patch-set. Once the framework is updated to work with endpoints it 
> should be a trivial patch to change rcar-csi2 to use the new helper. Do 
> you agree whit this or do you feel that this patch-set should depend on 
> such change of the framework?

Without changes to the framework, I don't think this would work since the
async framework (or individual drivers) will assign the device's fwnode,
not that of the endpoint, to the fwnode against which to match the async
sub-device.

Therefore you'll need these changes for this driver to work. Or if you
introduce a sub-device driver that uses endpoints as well, then we have two
non-interoperable sets of ISP (or bridge) and sub-device drivers. That'd be
quite undesirable.

Or, if you don't care whether it'd work with your use case right now, you
could use the helper function without changes. :-)

> > > +{
> > > + const struct soc_device_attribute *attr;
> > > + struct rcar_csi2 *priv;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> > > + if (!priv)
> > > + return -ENOMEM;
> > > +
> > > + priv->info = of_device_get_match_data(>dev);
> > > +
> > > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */
> > > + attr = soc_device_match(r8a7795es1);
> > > + if (attr)
> > > + priv->info = attr->data;
> > > +
> > > + priv->dev = >dev;
> > > +
> > > + mutex_init(>lock);
> > > + priv->stream_count = 0;
> > > +
> > > + ret = rcar_csi2_probe_resources(priv, pdev);
> > > + if (ret) {
> > > + dev_err(priv->dev, "Failed to get resources\n");
> > > + return ret;
> > > + }
> > > +
> > > + platform_set_drvdata(pdev, priv);
> > > +
> > > + ret = rcar_csi2_parse_dt(priv);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + priv->subdev.owner = THIS_MODULE;
> > > + priv->subdev.dev = >dev;
> > > + v4l2_subdev_init(>subdev, _csi2_subdev_ops);
> > > + v4l2_set_subdevdata(>subdev, >dev);
> > > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> > > +  KBUILD_MODNAME, dev_name(>dev));
> > > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +
> > > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> > > + priv->subdev.entity.ops = _csi2_entity_ops;
> > > +
> > > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> > > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> > > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > > +
> > > + ret = media_entity_pads_init(>subdev.entity, NR_OF_RCAR_CSI2_PAD,
> > > +  priv->pads);
> > > + if (ret)
> > > + goto error;
> > > +
> > > + ret = v4l2_async_register_subdev(>subdev);
> > > + if (ret < 0)
> > > + goto error;
> > > +
> > > + pm_runtime_enable(>dev);
> > 
> > Is this enough for platform devices? Just wondering.
> 
> As far as I can tell from the documentation this should be enough. I'm 
> no expert on PM so if I'm wrong please let me know.
> 
> Geert: do I understand the documentation correctly?
> 
> > 
> > > +
> > > + dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> > 
> > I'd use dev_dbg.
> 
> I'm thorn about this one. I agree that the information printed is not 
> critical. But I have found this useful when receiving logs from users 
> who have misconfigured there DTS with the wrong number of lines.
> 
> I'm open to changing this, but if it's a matter of taste I prefer to 
> keep it at a info level.

No

[PATCH] [media] vcodec: mediatek: Add MODULE_LICENSE to mtk_vcodec_util.c

2017-12-02 Thread Daniel Axtens 
This fixes the following warning in an allmodconfig build:
WARNING: modpost: missing MODULE_LICENSE() in 
drivers/media/platform/mtk-vcodec/mtk-vcodec-common.o

This matches the license at the top of the file.

Signed-off-by: Daniel Axtens 
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c 
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c
index 46768c056193..572efbbce7a9 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_util.c
@@ -115,3 +115,5 @@ struct mtk_vcodec_ctx *mtk_vcodec_get_curr_ctx(struct 
mtk_vcodec_dev *dev)
return ctx;
 }
 EXPORT_SYMBOL(mtk_vcodec_get_curr_ctx);
+
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH v12 2/2] rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

2017-12-02 Thread Niklas Söderlund 
Hej Sakari,

On 2017-12-01 15:01:36 +0200, Sakari Ailus wrote:
> Hej Niklas,
> 
> Tack för dina nya lappar!

Tack för dina kommentarer.

> 
> On Wed, Nov 29, 2017 at 08:32:35PM +0100, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the rcar-vin driver on R-Car Gen3 SoCs where separate CSI-2
> > hardware blocks are connected between the video sources and the video
> > grabbers (VIN).
> > 
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> > 
> > Signed-off-by: Niklas Söderlund 
> > Reviewed-by: Hans Verkuil 
> > ---
> >  drivers/media/platform/rcar-vin/Kconfig |  12 +
> >  drivers/media/platform/rcar-vin/Makefile|   1 +
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 898 
> > 
> >  3 files changed, 911 insertions(+)
> >  create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
> > 
> > diff --git a/drivers/media/platform/rcar-vin/Kconfig 
> > b/drivers/media/platform/rcar-vin/Kconfig
> > index af4c98b44d2e22cb..6875f30c1ae42631 100644
> > --- a/drivers/media/platform/rcar-vin/Kconfig
> > +++ b/drivers/media/platform/rcar-vin/Kconfig
> > @@ -1,3 +1,15 @@
> > +config VIDEO_RCAR_CSI2
> > +   tristate "R-Car MIPI CSI-2 Receiver"
> > +   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
> > +   depends on ARCH_RENESAS || COMPILE_TEST
> > +   select V4L2_FWNODE
> > +   ---help---
> > + Support for Renesas R-Car MIPI CSI-2 receiver.
> > + Supports R-Car Gen3 SoCs.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called rcar-csi2.
> > +
> >  config VIDEO_RCAR_VIN
> > tristate "R-Car Video Input (VIN) Driver"
> > depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA && 
> > MEDIA_CONTROLLER
> > diff --git a/drivers/media/platform/rcar-vin/Makefile 
> > b/drivers/media/platform/rcar-vin/Makefile
> > index 48c5632c21dc060b..5ab803d3e7c1aa57 100644
> > --- a/drivers/media/platform/rcar-vin/Makefile
> > +++ b/drivers/media/platform/rcar-vin/Makefile
> > @@ -1,3 +1,4 @@
> >  rcar-vin-objs = rcar-core.o rcar-dma.o rcar-v4l2.o
> >  
> > +obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o
> >  obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin.o
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > new file mode 100644
> > index ..30aafcbb7a3642c6
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -0,0 +1,898 @@
> > +/*
> > + * Driver for Renesas R-Car MIPI CSI-2 Receiver
> > + *
> > + * Copyright (C) 2017 Renesas Electronics Corp.
> > + *
> > + * This program is free software; you can redistribute  it and/or modify it
> > + * under  the terms of  the GNU General  Public License as published by the
> > + * Free Software Foundation;  either version 2 of the  License, or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* Register offsets and bits */
> > +
> > +/* Control Timing Select */
> > +#define TREF_REG   0x00
> > +#define TREF_TREF  BIT(0)
> > +
> > +/* Software Reset */
> > +#define SRST_REG   0x04
> > +#define SRST_SRST  BIT(0)
> > +
> > +/* PHY Operation Control */
> > +#define PHYCNT_REG 0x08
> > +#define PHYCNT_SHUTDOWNZ   BIT(17)
> > +#define PHYCNT_RSTZBIT(16)
> > +#define PHYCNT_ENABLECLK   BIT(4)
> > +#define PHYCNT_ENABLE_3BIT(3)
> > +#define PHYCNT_ENABLE_2BIT(2)
> > +#define PHYCNT_ENABLE_1BIT(1)
> > +#define PHYCNT_ENABLE_0BIT(0)
> > +
> > +/* Checksum Control */
> > +#define CHKSUM_REG 0x0c
> > +#define CHKSUM_ECC_EN  BIT(1)
> > +#define CHKSUM_CRC_EN  BIT(0)
> > +
> > +/*
> > + * Channel Data Type Select
> > + * VCDT[0-15]:  Channel 1 VCDT[16-31]:  Channel 2
> > + * VCDT2[0-15]: Channel 3 VCDT2[16-31]: Channel 4
> > + */
> > +#define VCDT_REG   0x10
> > +#define VCDT2_REG  0x14
> > +#define VCDT_VCDTN_EN  BIT(15)
> > +#define VCDT_SEL_VC(n) (((n) & 0x3) << 8)
> > +#define VCDT_SEL_DTN_ONBIT(6)
> > +#define VCDT_SEL_DT(n) (((n) & 0x3f) << 0)
> > +
> > +/* Frame Data Type Select */
> > +#define FRDT_REG   0x18
> > +
> > +/* Field Detection Control */
> > +#define FLD_REG0x1c
> > +#define FLD_FLD_NUM(n) (((n) & 0xff) << 16)
> > +#define FLD_FLD_EN4BIT(3)
> >

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Dan Carpenter 
On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> -#define DEFAULT_PIPE_INFO \
> -{ \
> - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> - IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> - { 0, 0},/* output system in res */ \
> - DEFAULT_SHADING_INFO,   /* shading_info */ \
> - DEFAULT_GRID_INFO,  /* grid_info */ \
> - 0   /* num_invalid_frames */ \
> -}
> +#define DEFAULT_PIPE_INFO ( \

Why does this have a ( now?  That can't compile can it??

> + (struct ia_css_pipe_info) { \
> + .output_info= 
> {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> + .vf_output_info = 
> {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> + .raw_output_info= 
> IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> + .output_system_in_res_info  = { 0, 0 }, \
> + .shading_info   = DEFAULT_SHADING_INFO, \
> + .grid_info  = DEFAULT_GRID_INFO, \
> + .num_invalid_frames = 0 \
> + } \
> +)

We need to get better compile test coverage on this...  :/  There are
some others as well.

regards,
dan carpenter

Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.

2017-12-02 Thread Jeremy Sowden 
On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > -#define DEFAULT_PIPE_INFO \
> > -{ \
> > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > -   {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > -   IA_CSS_BINARY_DEFAULT_FRAME_INFO,   /* raw_output_info */ \
> > -   { 0, 0},/* output system in res */ \
> > -   DEFAULT_SHADING_INFO,   /* shading_info */ \
> > -   DEFAULT_GRID_INFO,  /* grid_info */ \
> > -   0   /* num_invalid_frames */ \
> > -}
> > +#define DEFAULT_PIPE_INFO ( \
>
> Why does this have a ( now?  That can't compile can it??

It does.

> > +   (struct ia_css_pipe_info) { \
> > +   .output_info= 
> > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > +   .vf_output_info = 
> > {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> > +   .raw_output_info= 
> > IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> > +   .output_system_in_res_info  = { 0, 0 }, \
> > +   .shading_info   = DEFAULT_SHADING_INFO, \
> > +   .grid_info  = DEFAULT_GRID_INFO, \
> > +   .num_invalid_frames = 0 \
> > +   } \
> > +)

Checkpatch got quite shouty, e.g.:

  ERROR: Macros with complex values should be enclosed in parentheses
  #826: FILE: 
drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/common/ia_css_sdis_common_types.h:215:
  +#define DEFAULT_DVS_STAT_PUBLIC_DVS_GLOBAL_CFG \
  +(struct dvs_stat_public_dvs_global_cfg) { \
  +   .kappa  = 0, \
  +   .match_shift= 0, \
  +   .ybin_mode  = 0, \
  +}

so I just wrapped all of them.

> We need to get better compile test coverage on this...  :/  There are
> some others as well.

I have run a test-compilation.  Some of the code doesn't get built
because it's #ifdeffed off.  I did try adding -DISP2401 (which enables
most of it), that that just causes unrelated compilation failures.

J.


signature.asc
Description: PGP signature