cron job: media_tree daily build: ERRORS

[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:   Fri May 19 05:00:25 CEST 2017
media-tree git hash:f2c61f98e0b5f8b53b8fb860e5dcdd661bde7d0b
media_build git hash:   ab988a3d089232ce9e1aec2f259e947c06983dbc
v4l-utils git hash: d16a17abd1d8d7885ca2f44fb295035278baa89c
gcc version:i686-linux-gcc (GCC) 7.1.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.9.0-164

linux-git-arm-at91: ERRORS
linux-git-arm-davinci: ERRORS
linux-git-arm-multi: ERRORS
linux-git-arm-pxa: ERRORS
linux-git-blackfin-bf561: ERRORS
linux-git-i686: WARNINGS
linux-git-m32r: OK
linux-git-mips: ERRORS
linux-git-powerpc64: OK
linux-git-sh: ERRORS
linux-git-x86_64: WARNINGS
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-rc1-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-rc1-x86_64: ERRORS
apps: WARNINGS
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

Re: [PATCH v5 2/3] platform: add video-multiplexer subdevice driver

[Pavel Machek]

On Wed 2017-05-17 17:15:06, Philipp Zabel wrote:
> This driver can handle SoC internal and external video bus multiplexers,
> controlled by mux controllers provided by the mux controller framework,
> such as MMIO register bitfields or GPIOs. The subdevice passes through
> the mbus configuration of the active input to the output side.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Philipp Zabel 
> Signed-off-by: Steve Longerbeam 

Acked-by: Pavel Machek 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v3.1 1/2] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

[Sakari Ailus]

Hi Laurent,

On Fri, May 19, 2017 at 12:59:12AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Friday 19 May 2017 00:05:17 Sakari Ailus wrote:
> > On Thu, May 18, 2017 at 11:54:46PM +0300, Laurent Pinchart wrote:
> > > On Thursday 18 May 2017 23:50:34 Sakari Ailus wrote:
> > >> On Thu, May 18, 2017 at 07:08:00PM +0300, Laurent Pinchart wrote:
> > >>> On Wednesday 17 May 2017 22:20:57 Sakari Ailus wrote:
> >  On Wed, May 17, 2017 at 04:38:14PM +0100, Kieran Bingham wrote:
> > > From: Kieran Bingham 
> > > 
> > > Return NULL, if a null entity is parsed for it's v4l2_subdev
> > > 
> > > Signed-off-by: Kieran Bingham
> > > 
> > > ---
> > > 
> > >  include/media/v4l2-subdev.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h
> > > b/include/media/v4l2-subdev.h
> > > index 5f1669c45642..72d7f28f38dc 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -829,7 +829,7 @@ struct v4l2_subdev {
> > >  };
> > >  
> > >  #define media_entity_to_v4l2_subdev(ent) \
> > > - container_of(ent, struct v4l2_subdev, entity)
> > > + (ent ? container_of(ent, struct v4l2_subdev, entity) : NULL)
> > >  #define vdev_to_v4l2_subdev(vdev) \
> > >   ((struct v4l2_subdev *)video_get_drvdata(vdev))
> >  
> >  The problem with this is that ent is now referenced twice. If the ent
> >  macro argument has side effect, this would introduce bugs. It's
> >  unlikely, but worth avoiding. Either use a macro or a function.
> >  
> >  I think I'd use function for there's little use for supporting for
> >  const and non-const arguments presumably. A simple static inline
> >  function should do.
> > >>> 
> > >>> Note that, if we want to keep using a macro, this could be written as
> > >>> 
> > >>> #define media_entity_to_v4l2_subdev(ent) ({ \
> > >>> typeof(ent) __ent = ent; \
> > > 
> > > I just realized that this should be written
> > > 
> > >   typeof(ent) __ent = (ent);
> > 
> > I don't think that really makes much of a difference. It's a little bit
> > safer still perhaps. I don't remember having seen a case where the function
> > argument would have required parentheses there though.
> > 
> > >>> __ent ? container_of(__ent, struct v4l2_subdev, entity) : NULL; 
> > >>> \
> > >>> 
> > >>> })
> > >>> 
> > >>> Bonus point if you can come up with a way to return a const struct
> > >>> v4l2_subdev pointer when then ent argument is const.
> > >> 
> > >> I can't think of a use case for that. I've never seen a const struct
> > >> v4l2_subdev anywhere. I could be just oblivious though. :-)
> > > 
> > > I agree with you, it's overkill, at least for now. Although I'd like to
> > > see how it could be done, for other similar constructs where both const
> > > and non- const versions are useful.
> > 
> > Yes, that approach is fine. Another example here (not merged yet):
> > 
> >  > d40c1c4632bcb457e5f580836922879>
> 
> The problem here is that the container_of() macro ends up casting the 
> argument 
> to a struct v4l2_subdev *, regardless of whether the original pointer was 
> const or not.

Uh, good point. Hmm.

> 
> > > > Better give a __ent a name that someone will not accidentally come up
> > > > with. That can lead to problems that are difficult to debug --- for the
> > > > code compiles, it just doesn't do what's expected.
> > > 
> > > Won't it generate a compilation error as the variable would be redefined
> > > by the macro ?
> > 
> > It's perfectly fine redefine local variables.
> 
> Of course, my bad. As the local __ent variable would shadow any variable of 
> the same name declared in a parent context, the only case where this would 
> cause a problem is if the ent argument to the macro references the __ent 
> variable. In the simplest case, if the ent argument is __ent, the construct 
> would lead to
> 
>   typeof(__ent) __ent = (__ent);
> 
> which interestingly enough compiles without generating a warning, but 
> generates incorrect code. Various macros in kernel.h seem to be subject to 
> the 
> same problem.
> 
> Do you have a suggestion for a better variable name ?

Documentation/process/coding-style.rst around line 760 is helpful:

5) namespace collisions when defining local variables in macros resembling
functions:

.. code-block:: c

#define FOO(x)  \
({  \
typeof(x) ret;  \
ret = calc_ret(x);  \
(ret);  \
})

ret is a common name for a local variable - __foo_ret is less likely
to collide with an existing variable.

-- 
Regards,

[media-pci-cx25821] question about value overwrite

[Gustavo A. R. Silva]

Hello everybody,

While looking into Coverity ID 1226903 I ran into the following piece  
of code at drivers/media/pci/cx25821/cx25821-medusa-video.c:393:


393int medusa_set_videostandard(struct cx25821_dev *dev)
394{
395int status = 0;
396u32 value = 0, tmp = 0;
397
398if (dev->tvnorm & V4L2_STD_PAL_BG || dev->tvnorm & V4L2_STD_PAL_DK)
399status = medusa_initialize_pal(dev);
400else
401status = medusa_initialize_ntsc(dev);
402
403/* Enable DENC_A output */
404value = cx25821_i2c_read(>i2c_bus[0], DENC_A_REG_4, );
405value = setBitAtPos(value, 4);
406status = cx25821_i2c_write(>i2c_bus[0], DENC_A_REG_4, value);
407
408/* Enable DENC_B output */
409value = cx25821_i2c_read(>i2c_bus[0], DENC_B_REG_4, );
410value = setBitAtPos(value, 4);
411status = cx25821_i2c_write(>i2c_bus[0], DENC_B_REG_4, value);
412
413return status;
414}

The issue is that the value stored in variable _status_ at lines 399  
and 401 is overwritten by the one stored at line 406 and then at line  
411, before it can be used.


My question is if the original intention was to ORed the return  
values, something like in the following patch:


index 0a9db05..226d14f 100644
--- a/drivers/media/pci/cx25821/cx25821-medusa-video.c
+++ b/drivers/media/pci/cx25821/cx25821-medusa-video.c
@@ -403,12 +403,12 @@ int medusa_set_videostandard(struct cx25821_dev *dev)
/* Enable DENC_A output */
value = cx25821_i2c_read(>i2c_bus[0], DENC_A_REG_4, );
value = setBitAtPos(value, 4);
-   status = cx25821_i2c_write(>i2c_bus[0], DENC_A_REG_4, value);
+   status |= cx25821_i2c_write(>i2c_bus[0], DENC_A_REG_4, value);

/* Enable DENC_B output */
value = cx25821_i2c_read(>i2c_bus[0], DENC_B_REG_4, );
value = setBitAtPos(value, 4);
-   status = cx25821_i2c_write(>i2c_bus[0], DENC_B_REG_4, value);
+   status |= cx25821_i2c_write(>i2c_bus[0], DENC_B_REG_4, value);

return status;
 }

What do you think?

I'd really appreciate any comment on this.

Thank you!
--
Gustavo A. R. Silva

Re: [PATCH v3.1 1/2] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

[Laurent Pinchart]

Hi Sakari,

On Friday 19 May 2017 00:05:17 Sakari Ailus wrote:
> On Thu, May 18, 2017 at 11:54:46PM +0300, Laurent Pinchart wrote:
> > On Thursday 18 May 2017 23:50:34 Sakari Ailus wrote:
> >> On Thu, May 18, 2017 at 07:08:00PM +0300, Laurent Pinchart wrote:
> >>> On Wednesday 17 May 2017 22:20:57 Sakari Ailus wrote:
>  On Wed, May 17, 2017 at 04:38:14PM +0100, Kieran Bingham wrote:
> > From: Kieran Bingham 
> > 
> > Return NULL, if a null entity is parsed for it's v4l2_subdev
> > 
> > Signed-off-by: Kieran Bingham
> > 
> > ---
> > 
> >  include/media/v4l2-subdev.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h
> > b/include/media/v4l2-subdev.h
> > index 5f1669c45642..72d7f28f38dc 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -829,7 +829,7 @@ struct v4l2_subdev {
> >  };
> >  
> >  #define media_entity_to_v4l2_subdev(ent) \
> > -   container_of(ent, struct v4l2_subdev, entity)
> > +   (ent ? container_of(ent, struct v4l2_subdev, entity) : NULL)
> >  #define vdev_to_v4l2_subdev(vdev) \
> > ((struct v4l2_subdev *)video_get_drvdata(vdev))
>  
>  The problem with this is that ent is now referenced twice. If the ent
>  macro argument has side effect, this would introduce bugs. It's
>  unlikely, but worth avoiding. Either use a macro or a function.
>  
>  I think I'd use function for there's little use for supporting for
>  const and non-const arguments presumably. A simple static inline
>  function should do.
> >>> 
> >>> Note that, if we want to keep using a macro, this could be written as
> >>> 
> >>> #define media_entity_to_v4l2_subdev(ent) ({ \
> >>>   typeof(ent) __ent = ent; \
> > 
> > I just realized that this should be written
> > 
> > typeof(ent) __ent = (ent);
> 
> I don't think that really makes much of a difference. It's a little bit
> safer still perhaps. I don't remember having seen a case where the function
> argument would have required parentheses there though.
> 
> >>>   __ent ? container_of(__ent, struct v4l2_subdev, entity) : NULL; \
> >>> 
> >>> })
> >>> 
> >>> Bonus point if you can come up with a way to return a const struct
> >>> v4l2_subdev pointer when then ent argument is const.
> >> 
> >> I can't think of a use case for that. I've never seen a const struct
> >> v4l2_subdev anywhere. I could be just oblivious though. :-)
> > 
> > I agree with you, it's overkill, at least for now. Although I'd like to
> > see how it could be done, for other similar constructs where both const
> > and non- const versions are useful.
> 
> Yes, that approach is fine. Another example here (not merged yet):
> 
>  d40c1c4632bcb457e5f580836922879>

The problem here is that the container_of() macro ends up casting the argument 
to a struct v4l2_subdev *, regardless of whether the original pointer was 
const or not.

> > > Better give a __ent a name that someone will not accidentally come up
> > > with. That can lead to problems that are difficult to debug --- for the
> > > code compiles, it just doesn't do what's expected.
> > 
> > Won't it generate a compilation error as the variable would be redefined
> > by the macro ?
> 
> It's perfectly fine redefine local variables.

Of course, my bad. As the local __ent variable would shadow any variable of 
the same name declared in a parent context, the only case where this would 
cause a problem is if the ent argument to the macro references the __ent 
variable. In the simplest case, if the ent argument is __ent, the construct 
would lead to

typeof(__ent) __ent = (__ent);

which interestingly enough compiles without generating a warning, but 
generates incorrect code. Various macros in kernel.h seem to be subject to the 
same problem.

Do you have a suggestion for a better variable name ?

> The compiler could just generate a warning and not an error.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/3] platform: add video-multiplexer subdevice driver

[Sakari Ailus]

Hi Philipp,

Thanks a lot for the update! Just a few really minor comments below. After
fixing them you can add:

Acked-by: Sakari Ailus 

On Wed, May 17, 2017 at 05:15:06PM +0200, Philipp Zabel wrote:
> This driver can handle SoC internal and external video bus multiplexers,
> controlled by mux controllers provided by the mux controller framework,
> such as MMIO register bitfields or GPIOs. The subdevice passes through
> the mbus configuration of the active input to the output side.
> 
> Signed-off-by: Sascha Hauer 
> Signed-off-by: Philipp Zabel 
> Signed-off-by: Steve Longerbeam 
> ---
> No changes since v4 [1]:
> 
> This patch depends on the mux subsystem [2] and on the mmio-mux driver [3]
> to work on i.MX6. The follow-up patch will make this usable until the mux
> framework is merged.
> 
> [1] https://patchwork.kernel.org/patch/9712131/
> [2] https://patchwork.kernel.org/patch/9725911/
> [3] https://patchwork.kernel.org/patch/9725893/
> ---
>  drivers/media/platform/Kconfig |   6 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-mux.c | 295 
> +
>  3 files changed, 303 insertions(+)
>  create mode 100644 drivers/media/platform/video-mux.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index ac026ee1ca074..259c0ff780937 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
> To compile this driver as a module, choose M here: the
> module will be called arv.
>  
> +config VIDEO_MUX
> + tristate "Video Multiplexer"
> + depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && 
> MULTIPLEXER

It'd be nice to have this split (over 80).

> + help
> +   This driver provides support for N:1 video bus multiplexers.
> +
>  config VIDEO_OMAP3
>   tristate "OMAP 3 Camera support"
>   depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 63303d63c64cf..a6363023f981e 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -28,6 +28,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)  += sh_veu.o
>  
>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)  += m2m-deinterlace.o
>  
> +obj-$(CONFIG_VIDEO_MUX)  += video-mux.o
> +
>  obj-$(CONFIG_VIDEO_S3C_CAMIF)+= s3c-camif/
>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS)   += exynos4-is/
>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG) += s5p-jpeg/
> diff --git a/drivers/media/platform/video-mux.c 
> b/drivers/media/platform/video-mux.c
> new file mode 100644
> index 0..e35ffa18126f3
> --- /dev/null
> +++ b/drivers/media/platform/video-mux.c
> @@ -0,0 +1,295 @@
> +/*
> + * video stream multiplexer controlled via mux control
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer 
> + * Copyright (C) 2016-2017 Pengutronix, Philipp Zabel 
> + *
> + * 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.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct video_mux {
> + struct v4l2_subdev subdev;
> + struct media_pad *pads;
> + struct v4l2_mbus_framefmt *format_mbus;
> + struct mux_control *mux;
> + struct mutex lock;
> + int active;
> +};
> +
> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev 
> *sd)
> +{
> + return container_of(sd, struct video_mux, subdev);
> +}
> +
> +static int video_mux_link_setup(struct media_entity *entity,
> + const struct media_pad *local,
> + const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> + int ret = 0;
> +
> + /*
> +  * The mux state is determined by the enabled sink pad link.
> +  * Enabling or disabling the source pad link has no effect.
> +  */
> + if (local->flags & MEDIA_PAD_FL_SOURCE)
> + return 0;
> +
> + dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> + remote->entity->name, remote->index, local->entity->name,
> + local->index, flags & 

Re: [PATCH v3 2/2] media: i2c: adv748x: add adv748x driver

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 17 May 2017 15:13:18 Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> Provide basic support for the ADV7481 and ADV7482.
> 
> The driver is modelled with 2 subdevices to allow simultaneous streaming
> from the AFE (Analog front end) and HDMI inputs.

Isn't that now four subdevices ?

> Presently the HDMI is hardcoded to link to the TXA CSI bus, whilst the
> AFE is linked to the TXB CSI bus.
> 
> The driver is based on a prototype by Koji Matsuoka in the Renesas BSP,
> and an earlier rework by Niklas Söderlund.
> 
> Signed-off-by: Kieran Bingham 

[snip]

> diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> b/Documentation/devicetree/bindings/media/i2c/adv748x.txt new file mode
> 100644
> index ..98d4600b7d26
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt
> @@ -0,0 +1,72 @@
> +* Analog Devices ADV748X video decoder with HDMI receiver
> +
> +The ADV7481, and ADV7482 are multi format video decoders with an integrated
> +HDMI receiver. It can output CSI-2 on two independent outputs TXA and TXB

s/It/They/ ?

> from +three input sources HDMI, analog and TTL.
> +
> +Required Properties:
> +
> +  - compatible: Must contain one of the following
> +- "adi,adv7481" for the ADV7481
> +- "adi,adv7482" for the ADV7482
> +
> +  - reg: I2C slave address
> +
> +  - interrupt-names: Should specify the interrupts as "intrq1" and/or
> "intrq2"
> + "intrq3" is only available on the adv7480 and adv7481

The bindings don't cover the ADV7480 yet, I wouldn't mention it here.

Which interrupt(s) are mandatory and which are optional ? If they're all 
mandatory (which I doubt) I would phrase it as 

  - interrupt-names: Should specify the "intrq1", "intrq2" and "intrq3" 
interrupts. The "intrq3" interrupt is only available on the adv7481.

If they're all optional, I would move it to the Optional Properties section 
and phrase it as

  - interrupt-names: Should specify the "intrq1", "intrq2" and/or "intrq3" 
interrupts. All interrupts are optional. The "intrq3" interrupt is only 
available on the adv7481.

If some of them only are mandatory,

  - interrupt-names: Should specify the "intrq1", "intrq2" and/or "intrq3" 
interrupts. The ... interrupts are mandatory, while the ... interrupts are 
optional. The "intrq3" interrupt is only available on the adv7481.

> +  - interrupts: Specify the interrupt lines for the ADV748x
> +
> +The device node must contain one 'port' child node per device input and
> output +port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt. The port
> nodes +are numbered as follows.
> +
> +  Name  TypePort
> +
> +  HDMI  sink0
> +  AIN1  sink1
> +  AIN2  sink2
> +  AIN3  sink3
> +  AIN4  sink4
> +  AIN5  sink5
> +  AIN6  sink6
> +  AIN7  sink7
> +  AIN8  sink8
> +  TTL   sink9
> +  TXA   source  10
> +  TXB   source  11
> +
> +The digital output port node must contain at least one source endpoint.

s/node/nodes/ ?
s/source //

> +Example:
> +
> +video_receiver@70 {
> +compatible = "adi,adv7482";
> +reg = <0x70>;
> +
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +interrupt-parent = <>;
> +interrupt-names = "intrq1", "intrq2";
> +interrupts = <30 IRQ_TYPE_LEVEL_LOW>,
> + <31 IRQ_TYPE_LEVEL_LOW>;
> +
> +port@10 {
> +reg = <10>;
> +adv7482_txa: endpoint@1 {

There's no need to number endpoints when there's a single one. Otherwise you'd 
need a reg property in the endpoint.

> +clock-lanes = <0>;
> +data-lanes = <1 2 3 4>;
> +remote-endpoint = <_in>;
> +};
> +};
> +
> +port@11 {
> +reg = <11>;
> +adv7482_txb: endpoint@1 {
> +clock-lanes = <0>;
> +data-lanes = <1>;
> +remote-endpoint = <_in>;
> +};
> +};

The example only shows ports 10 and 11. Should all ports be present, even if 
they have no endpoint, because they're present at the hardware level ? That's 
debatable, but if the ports are optional when not connected, I would document 
that explicitly above.

> +

Re: [PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[Michael Büsch]

On Thu, 18 May 2017 11:06:43 -0300
Mauro Carvalho Chehab  wrote:

> diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
> index 192b1c7740df..145407dee3db 100644
> --- a/drivers/media/tuners/fc0011.c
> +++ b/drivers/media/tuners/fc0011.c
> @@ -342,6 +342,7 @@ static int fc0011_set_params(struct dvb_frontend *fe)
>   switch (vco_sel) {
>   default:
>   WARN_ON(1);
> + return -EINVAL;
>   case 0:
>   if (vco_cal < 8) {
>   regs[FC11_REG_VCOSEL] &= ~(FC11_VCOSEL_1 | 
> FC11_VCOSEL_2);

This fall through is intentional, but I guess returning an error is OK,
too. This should not happen anyway.
I cannot test this, though.

Acked-by: Michael Büsch 

-- 
Michael


pgpfAl6Qz6id8.pgp
Description: OpenPGP digital signature

Re: [RFC PATCH v2 1/4] media: i2c: adv748x: add adv748x driver

[Sakari Ailus]

Hi Kieran,

On Thu, May 18, 2017 at 01:04:55PM +0100, Kieran Bingham wrote:
> Hi Sakari,
> 
> >>> In omap3isp/isp.c: isp_fwnodes_parse() the async notifier is registered 
> >>> looking
> >>> at the endpoints parent fwnode:
> >>>
> >>>   isd->asd.match.fwnode.fwnode =
> >>>   fwnode_graph_get_remote_port_parent(fwnode);
> >>>
> >>> This would therefore not support ADV748x ... (it wouldn't know which 
> >>> TX/CSI
> >>> entity to match against)
> >>>
> >>> So the way I see it is that all devices which currently register an async
> >>> notifier should now register the match against the endpoint instead of the
> >>> parent device:
> >>>
> >>>  - isd->asd.match.fwnode.fwnode = 
> >>> fwnode_graph_get_remote_port_parent(fwnode);
> >>>  + isd->asd.match.fwnode.fwnode = fwnode;
> >>>
> >>> And then if we adapt the match to check against:
> >>>fwnode == fwnode || fwnode == 
> >>> fwnode_graph_get_remote_port_parent(fwnode);
> >>
> >> That's not enough as a master driver may use device node whereas the
> >> sub-device driver uses endpoint node. You need to do it both ways.
> >>
> 
> I've worked through this and I'm convinced that it should not be both ways...
> 
> We are matching a Subdevice (SD) to an Async Subdevice Notifier (ASD)
> 
> Bringing in 'endpoints' gives us the following match combinations
> 
> 
>   SD   ASD   : Result
>   ep   ep: Match ... Subdevices can specify their endpoint node.
> Notifiers can be updated to also specify the (remote) 
> endpoint.
> 
>   dev  ep: Match  - We should take the parent of the ASD to match SD,
> to support subdevices which default to their device node.
> 
>   dev  dev   : Match - This is the current case for all other drivers
> but Notifier ASDs can be converted to EP's
> 
>   ep   dev   : No Match - We should *not* take the parent of the SD
> If a subdevice has specified it's endpoint, (ADV748x)
> Matching against the parent device is invalid.

Is it? You could argue that all CSI-2 receiver drivers that the ADV748x is
used need to be converted, but I suppose that other sub-device drivers will
gradually get converted as well whilst not all CSI-2 receiver drivers could
be converted yet.

I guess it would be also possible to perform a mass conversion but the
problem in those is always that you can't test all the affected hardware
anyway.

> 
> Finding and matching the parent of the SD would allow a driver to register a
> notifier ASD with a dev node, and expect to match a subdevice that has
> registered it's subdev with an EP node. This would erroneously allow drivers 
> to
> register a notifier that would match against *both* of the ADV748x CSI2 
> entities.

You'd first have to have a board that has the two devices, and then ignore
what the drivers are doing. :-) I guess an error would be encountered at
some point in driver initialisation.

Anyway, I don't think this situation should be a lasting one, and that
endpoint matching is the way to go.

> 
> I have posted an implementation of this as:
>  [PATCH v1 3/3] v4l: async: Match parent devices [0]
> 
> I believe this to be correct - but I'm aware that I'm only really considering
> the OF side here... Please let me know if there's something I'm not taking 
> into
> account.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v3.1 1/2] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

[Sakari Ailus]

Hi Laurent,

On Thu, May 18, 2017 at 11:54:46PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thursday 18 May 2017 23:50:34 Sakari Ailus wrote:
> > On Thu, May 18, 2017 at 07:08:00PM +0300, Laurent Pinchart wrote:
> > > On Wednesday 17 May 2017 22:20:57 Sakari Ailus wrote:
> > >> On Wed, May 17, 2017 at 04:38:14PM +0100, Kieran Bingham wrote:
> > >>> From: Kieran Bingham 
> > >>> 
> > >>> Return NULL, if a null entity is parsed for it's v4l2_subdev
> > >>> 
> > >>> Signed-off-by: Kieran Bingham
> > >>> 
> > >>> ---
> > >>> 
> > >>>  include/media/v4l2-subdev.h | 2 +-
> > >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>> 
> > >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > >>> index 5f1669c45642..72d7f28f38dc 100644
> > >>> --- a/include/media/v4l2-subdev.h
> > >>> +++ b/include/media/v4l2-subdev.h
> > >>> @@ -829,7 +829,7 @@ struct v4l2_subdev {
> > >>>  };
> > >>>  
> > >>>  #define media_entity_to_v4l2_subdev(ent) \
> > >>> -   container_of(ent, struct v4l2_subdev, entity)
> > >>> +   (ent ? container_of(ent, struct v4l2_subdev, entity) : NULL)
> > >>>  #define vdev_to_v4l2_subdev(vdev) \
> > >>> ((struct v4l2_subdev *)video_get_drvdata(vdev))
> > >> 
> > >> The problem with this is that ent is now referenced twice. If the ent
> > >> macro argument has side effect, this would introduce bugs. It's
> > >> unlikely, but worth avoiding. Either use a macro or a function.
> > >> 
> > >> I think I'd use function for there's little use for supporting for const
> > >> and non-const arguments presumably. A simple static inline function
> > >> should do.
> > >
> > > Note that, if we want to keep using a macro, this could be written as
> > > 
> > > #define media_entity_to_v4l2_subdev(ent) ({ \
> > > 
> > >   typeof(ent) __ent = ent; \
> 
> I just realized that this should be written
> 
>   typeof(ent) __ent = (ent);

I don't think that really makes much of a difference. It's a little bit
safer still perhaps. I don't remember having seen a case where the function
argument would have required parentheses there though.

> 
> > >   __ent ? container_of(__ent, struct v4l2_subdev, entity) : NULL; \
> > > 
> > > })
> > > 
> > > Bonus point if you can come up with a way to return a const struct
> > > v4l2_subdev pointer when then ent argument is const.
> > 
> > I can't think of a use case for that. I've never seen a const struct
> > v4l2_subdev anywhere. I could be just oblivious though. :-)
> 
> I agree with you, it's overkill, at least for now. Although I'd like to see 
> how it could be done, for other similar constructs where both const and non-
> const versions are useful.

Yes, that approach is fine. Another example here (not merged yet):



> 
> > Better give a __ent a name that someone will not accidentally come up with.
> > That can lead to problems that are difficult to debug --- for the code
> > compiles, it just doesn't do what's expected.
> 
> Won't it generate a compilation error as the variable would be redefined by 
> the macro ?

It's perfectly fine redefine local variables. The compiler could just
generate a warning and not an error.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v3.1 1/2] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

[Laurent Pinchart]

Hi Sakari,

On Thursday 18 May 2017 23:50:34 Sakari Ailus wrote:
> On Thu, May 18, 2017 at 07:08:00PM +0300, Laurent Pinchart wrote:
> > On Wednesday 17 May 2017 22:20:57 Sakari Ailus wrote:
> >> On Wed, May 17, 2017 at 04:38:14PM +0100, Kieran Bingham wrote:
> >>> From: Kieran Bingham 
> >>> 
> >>> Return NULL, if a null entity is parsed for it's v4l2_subdev
> >>> 
> >>> Signed-off-by: Kieran Bingham
> >>> 
> >>> ---
> >>> 
> >>>  include/media/v4l2-subdev.h | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>> 
> >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>> index 5f1669c45642..72d7f28f38dc 100644
> >>> --- a/include/media/v4l2-subdev.h
> >>> +++ b/include/media/v4l2-subdev.h
> >>> @@ -829,7 +829,7 @@ struct v4l2_subdev {
> >>>  };
> >>>  
> >>>  #define media_entity_to_v4l2_subdev(ent) \
> >>> - container_of(ent, struct v4l2_subdev, entity)
> >>> + (ent ? container_of(ent, struct v4l2_subdev, entity) : NULL)
> >>>  #define vdev_to_v4l2_subdev(vdev) \
> >>>   ((struct v4l2_subdev *)video_get_drvdata(vdev))
> >> 
> >> The problem with this is that ent is now referenced twice. If the ent
> >> macro argument has side effect, this would introduce bugs. It's
> >> unlikely, but worth avoiding. Either use a macro or a function.
> >> 
> >> I think I'd use function for there's little use for supporting for const
> >> and non-const arguments presumably. A simple static inline function
> >> should do.
> >
> > Note that, if we want to keep using a macro, this could be written as
> > 
> > #define media_entity_to_v4l2_subdev(ent) ({ \
> > 
> > typeof(ent) __ent = ent; \

I just realized that this should be written

typeof(ent) __ent = (ent);

> > __ent ? container_of(__ent, struct v4l2_subdev, entity) : NULL; \
> > 
> > })
> > 
> > Bonus point if you can come up with a way to return a const struct
> > v4l2_subdev pointer when then ent argument is const.
> 
> I can't think of a use case for that. I've never seen a const struct
> v4l2_subdev anywhere. I could be just oblivious though. :-)

I agree with you, it's overkill, at least for now. Although I'd like to see 
how it could be done, for other similar constructs where both const and non-
const versions are useful.

> Better give a __ent a name that someone will not accidentally come up with.
> That can lead to problems that are difficult to debug --- for the code
> compiles, it just doesn't do what's expected.

Won't it generate a compilation error as the variable would be redefined by 
the macro ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3.1 1/2] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

[Sakari Ailus]

Hi Laurent,

On Thu, May 18, 2017 at 07:08:00PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday 17 May 2017 22:20:57 Sakari Ailus wrote:
> > On Wed, May 17, 2017 at 04:38:14PM +0100, Kieran Bingham wrote:
> > > From: Kieran Bingham 
> > > 
> > > Return NULL, if a null entity is parsed for it's v4l2_subdev
> > > 
> > > Signed-off-by: Kieran Bingham 
> > > ---
> > > 
> > >  include/media/v4l2-subdev.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 5f1669c45642..72d7f28f38dc 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -829,7 +829,7 @@ struct v4l2_subdev {
> > > 
> > >  };
> > >  
> > >  #define media_entity_to_v4l2_subdev(ent) \
> > > - container_of(ent, struct v4l2_subdev, entity)
> > > + (ent ? container_of(ent, struct v4l2_subdev, entity) : NULL)
> > > 
> > >  #define vdev_to_v4l2_subdev(vdev) \
> > >   ((struct v4l2_subdev *)video_get_drvdata(vdev))
> > 
> > The problem with this is that ent is now referenced twice. If the ent macro
> > argument has side effect, this would introduce bugs. It's unlikely, but
> > worth avoiding. Either use a macro or a function.
> > 
> > I think I'd use function for there's little use for supporting for const and
> > non-const arguments presumably. A simple static inline function should do.
> 
> Note that, if we want to keep using a macro, this could be written as
> 
> #define media_entity_to_v4l2_subdev(ent) ({ \
>   typeof(ent) __ent = ent; \
>   __ent ? container_of(__ent, struct v4l2_subdev, entity) : NULL; \
> })
> 
> Bonus point if you can come up with a way to return a const struct 
> v4l2_subdev 
> pointer when then ent argument is const.

I can't think of a use case for that. I've never seen a const struct
v4l2_subdev anywhere. I could be just oblivious though. :-)

Better give a __ent a name that someone will not accidentally come up with.
That can lead to problems that are difficult to debug --- for the code
compiles, it just doesn't do what's expected.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH] Staging: media: fix missing blank line coding style issue in atomisp_tpg.c

[Greg KH]

On Thu, May 18, 2017 at 05:31:20PM +0100, Alan Cox wrote:
> On Wed, 2017-05-17 at 21:48 -0400, Manny Vindiola wrote:
> > This is a patch to the atomisp_tpg.c file that fixes up a missing
> > blank line warning found by the checkpatch.pl tool
> > 
> > Signed-off-by: Manny Vindiola 
> > ---
> >  drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> > b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> > index 996d1bd..48b9604 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> > @@ -56,6 +56,7 @@ static int tpg_set_fmt(struct v4l2_subdev *sd,
> >        struct v4l2_subdev_format *format)
> >  {
> >     struct v4l2_mbus_framefmt *fmt = >format;
> > +
> >     if (format->pad)
> >     return -EINVAL;
> >     /* only raw8 grbg is supported by TPG */
> 
> The TODO fille for this driver specifically says not to send formatting
> patches at this point.
> 
> There is no point making trivial spacing changes in code that needs
> lots of real work. It's like polishing your car when the doors have
> fallen off.

Unfortunatly, given that the code is in staging, that's not going to
happen, people are going to send cleanup patches, and that's ok.  They
should be easy to merge around, it's the price for being in the tree.

thanks,

greg k-h

Re: [media-dvb-usb-v2] question about value overwrite

[Gustavo A. R. Silva]

Hi Malcolm,

Quoting Malcolm Priestley :


Hi

On 18/05/17 20:09, Gustavo A. R. Silva wrote:


Hello everybody,

While looking into Coverity ID 1226934 I ran into the following  
piece of code at drivers/media/usb/dvb-usb-v2/lmedm04.c:205


205static int lme2510_stream_restart(struct dvb_usb_device *d)
206{
207struct lme2510_state *st = d->priv;
208u8 all_pids[] = LME_ALL_PIDS;
209u8 stream_on[] = LME_ST_ON_W;
210int ret;
211u8 rbuff[1];
212if (st->pid_off)
213ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
214rbuff, sizeof(rbuff));
215/*Restart Stream Command*/
216ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
217rbuff, sizeof(rbuff));
218return ret;
219}


It is a mistake it should have been ORed ad in |= as  
lme2510_usb_talk only returns three states.




I see now. The idea is to code something similar to the following  
piece of code in the same file:


242
243ret |= lme2510_usb_talk(d, pid_buff ,
244sizeof(pid_buff) , rbuf, sizeof(rbuf));
245
246if (st->stream_on)
247ret |= lme2510_stream_restart(d);
248
249return ret;

right?

So in this case, the following patch would properly fix the bug:

index 924adfd..3ab1754 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -207,13 +207,14 @@ static int lme2510_stream_restart(struct  
dvb_usb_device *d)

struct lme2510_state *st = d->priv;
u8 all_pids[] = LME_ALL_PIDS;
u8 stream_on[] = LME_ST_ON_W;
-   int ret;
+   int ret = 0;
u8 rbuff[1];
+
if (st->pid_off)
ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
rbuff, sizeof(rbuff));
/*Restart Stream Command*/
-   ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
+   ret |= lme2510_usb_talk(d, stream_on, sizeof(stream_on),
rbuff, sizeof(rbuff));
return ret;
 }

What do you think?


So if an error is in the running it will be returned to user.

The first of your patches is better and more or less the same, the  
second would break driver, restart is not an else condition.




Thank you for the clarification.
--
Gustavo A. R. Silva

Re: [media-dvb-usb-v2] question about value overwrite

[Malcolm Priestley]

Hi

On 18/05/17 20:09, Gustavo A. R. Silva wrote:


Hello everybody,

While looking into Coverity ID 1226934 I ran into the following piece of 
code at drivers/media/usb/dvb-usb-v2/lmedm04.c:205


205static int lme2510_stream_restart(struct dvb_usb_device *d)
206{
207struct lme2510_state *st = d->priv;
208u8 all_pids[] = LME_ALL_PIDS;
209u8 stream_on[] = LME_ST_ON_W;
210int ret;
211u8 rbuff[1];
212if (st->pid_off)
213ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
214rbuff, sizeof(rbuff));
215/*Restart Stream Command*/
216ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
217rbuff, sizeof(rbuff));
218return ret;
219}


It is a mistake it should have been ORed ad in |= as lme2510_usb_talk 
only returns three states.


So if an error is in the running it will be returned to user.

The first of your patches is better and more or less the same, the 
second would break driver, restart is not an else condition.


Regards


Malcolm

[media-dvb-usb-v2] question about value overwrite

[Gustavo A. R. Silva]

Hello everybody,

While looking into Coverity ID 1226934 I ran into the following piece  
of code at drivers/media/usb/dvb-usb-v2/lmedm04.c:205


205static int lme2510_stream_restart(struct dvb_usb_device *d)
206{
207struct lme2510_state *st = d->priv;
208u8 all_pids[] = LME_ALL_PIDS;
209u8 stream_on[] = LME_ST_ON_W;
210int ret;
211u8 rbuff[1];
212if (st->pid_off)
213ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
214rbuff, sizeof(rbuff));
215/*Restart Stream Command*/
216ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
217rbuff, sizeof(rbuff));
218return ret;
219}

The issue is that the value store in variable _ret_ at line 213 is  
overwritten by the one stored at line 216, before it can be used.


My question is if an _else_ statement is missing, or the variable  
assignment at line 213 should be removed, leaving just the call  
lme2510_usb_talk(d, all_pids, sizeof(all_pids), rbuff, sizeof(rbuff));  
in place.


Maybe either of the following patches could be applied:

index 924adfd..d573144 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -207,15 +207,15 @@ static int lme2510_stream_restart(struct  
dvb_usb_device *d)

struct lme2510_state *st = d->priv;
u8 all_pids[] = LME_ALL_PIDS;
u8 stream_on[] = LME_ST_ON_W;
-   int ret;
u8 rbuff[1];
+
if (st->pid_off)
-   ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
+   lme2510_usb_talk(d, all_pids, sizeof(all_pids),
rbuff, sizeof(rbuff));
+
/*Restart Stream Command*/
-   ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
+   return lme2510_usb_talk(d, stream_on, sizeof(stream_on),
rbuff, sizeof(rbuff));
-   return ret;
 }

index 924adfd..dd51f05 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -207,15 +207,15 @@ static int lme2510_stream_restart(struct  
dvb_usb_device *d)

struct lme2510_state *st = d->priv;
u8 all_pids[] = LME_ALL_PIDS;
u8 stream_on[] = LME_ST_ON_W;
-   int ret;
u8 rbuff[1];
+
if (st->pid_off)
-   ret = lme2510_usb_talk(d, all_pids, sizeof(all_pids),
+   return lme2510_usb_talk(d, all_pids, sizeof(all_pids),
rbuff, sizeof(rbuff));
-   /*Restart Stream Command*/
-   ret = lme2510_usb_talk(d, stream_on, sizeof(stream_on),
+   else
+   /*Restart Stream Command*/
+   return lme2510_usb_talk(d, stream_on, sizeof(stream_on),
rbuff, sizeof(rbuff));
-   return ret;
 }

What do you think?

I'd really appreciate any comment on this.

Thank you!
--
Gustavo A. R. Silva

Re: [PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[Andrey Utkin]

On Thu, May 18, 2017 at 11:06:43AM -0300, Mauro Carvalho Chehab wrote:
> As such macro will check if the expression is true, it may fall through, as
> warned:

> On both cases, it means an error, so, let's return an error
> code, to make gcc happy.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/pci/tw5864/tw5864-video.c | 1 +
>  drivers/media/tuners/fc0011.c   | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
> b/drivers/media/pci/tw5864/tw5864-video.c
> index 2a044be729da..e7bd2b8484e3 100644
> --- a/drivers/media/pci/tw5864/tw5864-video.c
> +++ b/drivers/media/pci/tw5864/tw5864-video.c
> @@ -545,6 +545,7 @@ static int tw5864_fmt_vid_cap(struct file *file, void 
> *priv,
>   switch (input->std) {
>   default:
>   WARN_ON_ONCE(1);
> + return -EINVAL;
>   case STD_NTSC:
>   f->fmt.pix.height = 480;
>   break;

Hi Mauro,

Thanks for the patch.

I actually meant it to fall through, but I agree this is not how it
should be.
I'm fine with this patch. Unfortunately I don't possess tw5864 hardware
now. CCing Anton Sviridenko whom I've handed it (I guess he's on
Bluecherry Maintainers groupmail as well).

Anton, just in case, could you please ensure the driver with this patch
works well in runtime?

Re: [PATCH 00/13] staging: media: atomisp queued up patches

[Alan Cox]

On Thu, 2017-05-18 at 11:10 -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 18 May 2017 15:50:09 +0200
> Greg Kroah-Hartman  escreveu:
> 
> > 
> > Hi Mauro,
> > 
> > Here's the set of accumulated atomisp staging patches that I had in
> > my
> > to-review mailbox.  After this, my queue is empty, the driver is
> > all
> > yours!
> 
> Thanks!
> 
> Alan, please let me know if you prefer if I don't apply any of
> such patches, otherwise I should be merging them tomorrow ;)

I will assume you've merged them and resync the internal patch queue I
have here to that. At the moment I'm still slowly trying to unthread
some of the fascinating layers of indirection without actually breaking
anything.

Alan

Re: [PATCH] Staging: media: fix missing blank line coding style issue in atomisp_tpg.c

[Alan Cox]

On Wed, 2017-05-17 at 21:48 -0400, Manny Vindiola wrote:
> This is a patch to the atomisp_tpg.c file that fixes up a missing
> blank line warning found by the checkpatch.pl tool
> 
> Signed-off-by: Manny Vindiola 
> ---
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> index 996d1bd..48b9604 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
> @@ -56,6 +56,7 @@ static int tpg_set_fmt(struct v4l2_subdev *sd,
>      struct v4l2_subdev_format *format)
>  {
>   struct v4l2_mbus_framefmt *fmt = >format;
> +
>   if (format->pad)
>   return -EINVAL;
>   /* only raw8 grbg is supported by TPG */

The TODO fille for this driver specifically says not to send formatting
patches at this point.

There is no point making trivial spacing changes in code that needs
lots of real work. It's like polishing your car when the doors have
fallen off.

Alan

Re: [PATCH v3.1 1/2] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

[Laurent Pinchart]

Hi Sakari,

On Wednesday 17 May 2017 22:20:57 Sakari Ailus wrote:
> On Wed, May 17, 2017 at 04:38:14PM +0100, Kieran Bingham wrote:
> > From: Kieran Bingham 
> > 
> > Return NULL, if a null entity is parsed for it's v4l2_subdev
> > 
> > Signed-off-by: Kieran Bingham 
> > ---
> > 
> >  include/media/v4l2-subdev.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 5f1669c45642..72d7f28f38dc 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -829,7 +829,7 @@ struct v4l2_subdev {
> > 
> >  };
> >  
> >  #define media_entity_to_v4l2_subdev(ent) \
> > -   container_of(ent, struct v4l2_subdev, entity)
> > +   (ent ? container_of(ent, struct v4l2_subdev, entity) : NULL)
> > 
> >  #define vdev_to_v4l2_subdev(vdev) \
> > ((struct v4l2_subdev *)video_get_drvdata(vdev))
> 
> The problem with this is that ent is now referenced twice. If the ent macro
> argument has side effect, this would introduce bugs. It's unlikely, but
> worth avoiding. Either use a macro or a function.
> 
> I think I'd use function for there's little use for supporting for const and
> non-const arguments presumably. A simple static inline function should do.

Note that, if we want to keep using a macro, this could be written as

#define media_entity_to_v4l2_subdev(ent) ({ \
typeof(ent) __ent = ent; \
__ent ? container_of(__ent, struct v4l2_subdev, entity) : NULL; \
})

Bonus point if you can come up with a way to return a const struct v4l2_subdev 
pointer when then ent argument is const.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/3] device property: Add fwnode_graph_get_port_parent

[Sakari Ailus]

Hi Kieran,

On Wed, May 17, 2017 at 04:03:38PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> V4L2 async notifiers can pass the endpoint fwnode rather than the device
> fwnode.
> 
> Provide a helper to obtain the parent device fwnode without first
> parsing the remote-endpoint as per fwnode_graph_get_remote_port_parent.
> 
> Signed-off-by: Kieran Bingham 

Could you rebase this on my fwnode cleanup patchset here, please? I'd be
happy to apply it on top of the set.



This function becomes quite simple, see
fwnode_graph_get_remote_port_parent().

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH 2/4] [media] imon: better code the pad_mouse toggling

[Sean Young]

On Thu, May 18, 2017 at 11:06:44AM -0300, Mauro Carvalho Chehab wrote:
> The logic with toggles the pad_mouse is confusing. Now, gcc 7.1
> complains about it:
> 
> drivers/media/rc/imon.c: In function 'imon_incoming_scancode':
> drivers/media/rc/imon.c:1725:23: warning: '~' on a boolean expression 
> [-Wbool-operation]
> ictx->pad_mouse = (~ictx->pad_mouse) & 0x1;
>^
> drivers/media/rc/imon.c:1725:23: note: did you mean to use logical not?
> ictx->pad_mouse = (~ictx->pad_mouse) & 0x1;
>^
>!
> 
> Rewrite it to be clearer for both code reviewers and gcc.

This was already spotted by Arnd.

https://patchwork.linuxtv.org/patch/41270/

Thanks
Sean

Re: [PATCH 00/13] staging: media: atomisp queued up patches

[Mauro Carvalho Chehab]

Em Thu, 18 May 2017 15:50:09 +0200
Greg Kroah-Hartman  escreveu:

> Hi Mauro,
> 
> Here's the set of accumulated atomisp staging patches that I had in my
> to-review mailbox.  After this, my queue is empty, the driver is all
> yours!

Thanks!

Alan, please let me know if you prefer if I don't apply any of
such patches, otherwise I should be merging them tomorrow ;)

> Good Luck! :)

Thanks!

Regards,
Mauro
> 
> thanks,
> 
> greg k-h
> 
> Avraham Shukron (3):
>   staging: media: atomisp: fixed sparse warnings
>   staging: media: atomisp: fixed coding style errors
>   staging: media: atomisp: fix coding style warnings
> 
> Dan Carpenter (2):
>   staging: media: atomisp: one char read beyond end of string
>   staging: media: atomisp: putting NULs in the wrong place
> 
> Fabrizio Perria (1):
>   staging: media: atomisp: Fix unnecessary initialization of static
> 
> Guru Das Srinagesh (2):
>   staging: media: atomisp: use logical AND, not bitwise
>   staging: media: atomisp: Make undeclared symbols static
> 
> Hans de Goede (1):
>   staging: media: atomisp: Fix -Werror=int-in-bool-context compile
> errors
> 
> Joe Perches (1):
>   staging: media: atomisp: Add __printf validation and fix fallout
> 
> Manny Vindiola (1):
>   staging: media: atomisp: fix missing blank line coding style issue in
> atomisp_tpg.c
> 
> Mauro Carvalho Chehab (1):
>   staging: media: atomisp: don't treat warnings as errors
> 
> Valentin Vidic (1):
>   staging: media: atomisp: drop unused qos variable
> 
>  drivers/staging/media/atomisp/i2c/Makefile |   2 -
>  drivers/staging/media/atomisp/i2c/imx/Makefile |   2 -
>  drivers/staging/media/atomisp/i2c/ov5693/Makefile  |   2 -
>  .../staging/media/atomisp/pci/atomisp2/Makefile|   2 +-
>  .../atomisp/pci/atomisp2/atomisp_compat_css20.c|   1 -
>  .../media/atomisp/pci/atomisp2/atomisp_fops.c  |  14 +-
>  .../media/atomisp/pci/atomisp2/atomisp_tpg.c   |   1 +
>  .../media/atomisp/pci/atomisp2/atomisp_v4l2.c  |   2 +-
>  .../css2400/hive_isp_css_include/math_support.h|   6 +-
>  .../css2400/hive_isp_css_include/string_support.h  |   9 +-
>  .../isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c   |   6 +-
>  .../isp/kernels/sdis/sdis_2/ia_css_sdis2.host.c|   2 +-
>  .../isp/kernels/tnr/tnr_1.0/ia_css_tnr.host.c  |   2 +-
>  .../atomisp2/css2400/runtime/binary/src/binary.c   |   2 +-
>  .../css2400/runtime/debug/interface/ia_css_debug.h |   1 +
>  .../css2400/runtime/debug/src/ia_css_debug.c   |   6 +-
>  .../media/atomisp/pci/atomisp2/css2400/sh_css.c|  19 +-
>  .../atomisp/pci/atomisp2/css2400/sh_css_mipi.c |   2 +-
>  .../atomisp/pci/atomisp2/css2400/sh_css_params.c   |  10 +-
>  .../platform/intel-mid/atomisp_gmin_platform.c | 210 
> +++--
>  .../platform/intel-mid/intel_mid_pcihelpers.c  |  12 +-
>  21 files changed, 162 insertions(+), 151 deletions(-)
> 



Thanks,
Mauro

[PATCH 4/4] [media] mtk_vcodec_dec: return error at mtk_vdec_pic_info_update()

[Mauro Carvalho Chehab]

Gcc 7.1 complains that:

drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c: In function 
'mtk_vdec_pic_info_update':
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c:284:6: warning: 
variable 'ret' set but not used [-Wunused-but-set-variable]
  int ret;
  ^~~

Indeed, if debug is disabled, "ret" is never used. The best
fix for it seems to make the fuction to return an error code.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c 
b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
index a60b538686ea..843510979ad8 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
@@ -278,7 +278,7 @@ static void mtk_vdec_flush_decoder(struct mtk_vcodec_ctx 
*ctx)
clean_free_buffer(ctx);
 }
 
-static void mtk_vdec_pic_info_update(struct mtk_vcodec_ctx *ctx)
+static int mtk_vdec_pic_info_update(struct mtk_vcodec_ctx *ctx)
 {
unsigned int dpbsize = 0;
int ret;
@@ -288,7 +288,7 @@ static void mtk_vdec_pic_info_update(struct mtk_vcodec_ctx 
*ctx)
>last_decoded_picinfo)) {
mtk_v4l2_err("[%d]Error!! Cannot get param : 
GET_PARAM_PICTURE_INFO ERR",
ctx->id);
-   return;
+   return -EINVAL;
}
 
if (ctx->last_decoded_picinfo.pic_w == 0 ||
@@ -296,12 +296,12 @@ static void mtk_vdec_pic_info_update(struct 
mtk_vcodec_ctx *ctx)
ctx->last_decoded_picinfo.buf_w == 0 ||
ctx->last_decoded_picinfo.buf_h == 0) {
mtk_v4l2_err("Cannot get correct pic info");
-   return;
+   return -EINVAL;
}
 
if ((ctx->last_decoded_picinfo.pic_w == ctx->picinfo.pic_w) ||
(ctx->last_decoded_picinfo.pic_h == ctx->picinfo.pic_h))
-   return;
+   return 0;
 
mtk_v4l2_debug(1,
"[%d]-> new(%d,%d), old(%d,%d), real(%d,%d)",
@@ -316,6 +316,8 @@ static void mtk_vdec_pic_info_update(struct mtk_vcodec_ctx 
*ctx)
mtk_v4l2_err("Incorrect dpb size, ret=%d", ret);
 
ctx->dpb_size = dpbsize;
+
+   return ret;
 }
 
 static void mtk_vdec_worker(struct work_struct *work)
-- 
2.9.3

[PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[Mauro Carvalho Chehab]

As such macro will check if the expression is true, it may fall through, as
warned:

In file included from ./include/uapi/linux/stddef.h:1:0,
 from ./include/linux/stddef.h:4,
 from ./include/uapi/linux/posix_types.h:4,
 from ./include/uapi/linux/types.h:13,
 from ./include/linux/types.h:5,
 from ./drivers/media/dvb-core/dvb_frontend.h:35,
 from drivers/media/tuners/fc0011.h:4,
 from drivers/media/tuners/fc0011.c:20:
drivers/media/tuners/fc0011.c: In function 'fc0011_set_params':
./include/linux/compiler.h:179:22: warning: this statement may fall through 
[-Wimplicit-fallthrough=]
 # define unlikely(x) __builtin_expect(!!(x), 0)
  ^~
./include/asm-generic/bug.h:109:2: note: in expansion of macro 'unlikely'
  unlikely(__ret_warn_on); \
  ^~~~
drivers/media/tuners/fc0011.c:344:3: note: in expansion of macro 'WARN_ON'
   WARN_ON(1);
   ^~~
drivers/media/tuners/fc0011.c:345:2: note: here
  case 0:
  ^~~~
In file included from ./include/uapi/linux/stddef.h:1:0,
 from ./include/linux/stddef.h:4,
 from ./include/uapi/linux/posix_types.h:4,
 from ./include/uapi/linux/types.h:13,
 from ./include/linux/types.h:5,
 from ./include/linux/list.h:4,
 from ./include/linux/module.h:9,
 from drivers/media/pci/tw5864/tw5864-video.c:17:
drivers/media/pci/tw5864/tw5864-video.c: In function 'tw5864_fmt_vid_cap':
./include/linux/compiler.h:179:22: warning: this statement may fall through 
[-Wimplicit-fallthrough=]
 # define unlikely(x) __builtin_expect(!!(x), 0)
  ^~
./include/asm-generic/bug.h:68:2: note: in expansion of macro 'unlikely'
  unlikely(__ret_warn_on);\
  ^~~~
drivers/media/pci/tw5864/tw5864-video.c:547:3: note: in expansion of macro 
'WARN_ON_ONCE'
   WARN_ON_ONCE(1);
   ^~~~
drivers/media/pci/tw5864/tw5864-video.c:548:2: note: here
  case STD_NTSC:
  ^~~~

On both cases, it means an error, so, let's return an error
code, to make gcc happy.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/pci/tw5864/tw5864-video.c | 1 +
 drivers/media/tuners/fc0011.c   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
b/drivers/media/pci/tw5864/tw5864-video.c
index 2a044be729da..e7bd2b8484e3 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -545,6 +545,7 @@ static int tw5864_fmt_vid_cap(struct file *file, void *priv,
switch (input->std) {
default:
WARN_ON_ONCE(1);
+   return -EINVAL;
case STD_NTSC:
f->fmt.pix.height = 480;
break;
diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
index 192b1c7740df..145407dee3db 100644
--- a/drivers/media/tuners/fc0011.c
+++ b/drivers/media/tuners/fc0011.c
@@ -342,6 +342,7 @@ static int fc0011_set_params(struct dvb_frontend *fe)
switch (vco_sel) {
default:
WARN_ON(1);
+   return -EINVAL;
case 0:
if (vco_cal < 8) {
regs[FC11_REG_VCOSEL] &= ~(FC11_VCOSEL_1 | 
FC11_VCOSEL_2);
-- 
2.9.3

[PATCH 2/4] [media] imon: better code the pad_mouse toggling

[Mauro Carvalho Chehab]

The logic with toggles the pad_mouse is confusing. Now, gcc 7.1
complains about it:

drivers/media/rc/imon.c: In function 'imon_incoming_scancode':
drivers/media/rc/imon.c:1725:23: warning: '~' on a boolean expression 
[-Wbool-operation]
ictx->pad_mouse = (~ictx->pad_mouse) & 0x1;
   ^
drivers/media/rc/imon.c:1725:23: note: did you mean to use logical not?
ictx->pad_mouse = (~ictx->pad_mouse) & 0x1;
   ^
   !

Rewrite it to be clearer for both code reviewers and gcc.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/rc/imon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 3489010601b5..9c510fe54b2a 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -1722,7 +1722,7 @@ static void imon_incoming_scancode(struct imon_context 
*ictx,
if (kc == KEY_KEYBOARD && !ictx->release_code) {
ictx->last_keycode = kc;
if (!nomouse) {
-   ictx->pad_mouse = ~(ictx->pad_mouse) & 0x1;
+   ictx->pad_mouse = !(ictx->pad_mouse & 0x1);
dev_dbg(dev, "toggling to %s mode\n",
ictx->pad_mouse ? "mouse" : "keyboard");
spin_unlock_irqrestore(>kc_lock, flags);
-- 
2.9.3

[PATCH 3/4] [media] s5p-jpeg: don't return a random width/height

[Mauro Carvalho Chehab]

Gcc 7.1 complains about:

drivers/media/platform/s5p-jpeg/jpeg-core.c: In function 
's5p_jpeg_parse_hdr.isra.9':
drivers/media/platform/s5p-jpeg/jpeg-core.c:1207:12: warning: 'width' may be 
used uninitialized in this function [-Wmaybe-uninitialized]
  result->w = width;
  ~~^~~
drivers/media/platform/s5p-jpeg/jpeg-core.c:1208:12: warning: 'height' may be 
used uninitialized in this function [-Wmaybe-uninitialized]
  result->h = height;
  ~~^~~~

Indeed the code would allow it to return a random value (although
it shouldn't happen, in practice). So, explicitly set both to zero,
just in case.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c 
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 52dc7941db65..1da2c94e1dca 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1099,10 +1099,10 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data 
*result,
   struct s5p_jpeg_ctx *ctx)
 {
int c, components = 0, notfound, n_dht = 0, n_dqt = 0;
-   unsigned int height, width, word, subsampling = 0, sos = 0, sof = 0,
-sof_len = 0;
-   unsigned int dht[S5P_JPEG_MAX_MARKER], dht_len[S5P_JPEG_MAX_MARKER],
-dqt[S5P_JPEG_MAX_MARKER], dqt_len[S5P_JPEG_MAX_MARKER];
+   unsigned int height = 0, width = 0, word, subsampling = 0;
+   unsigned int sos = 0, sof = 0, sof_len = 0;
+   unsigned int dht[S5P_JPEG_MAX_MARKER], dht_len[S5P_JPEG_MAX_MARKER];
+   unsigned int dqt[S5P_JPEG_MAX_MARKER], dqt_len[S5P_JPEG_MAX_MARKER];
long length;
struct s5p_jpeg_buffer jpeg_buffer;
 
-- 
2.9.3

Re: [PATCH v1 3/3] v4l: async: Match parent devices

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 17 May 2017 16:03:39 Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> Devices supporting multiple endpoints on a single device node must set
> their subdevice fwnode to the endpoint to allow distinct comparisons.
> 
> Adapt the match_fwnode call to compare against the provided fwnodes
> first, but also to search for a comparison against the parent fwnode.
> 
> This allows notifiers to pass the endpoint for comparison and still
> support existing subdevices which store their default parent device
> node.
> 
> Signed-off-by: Kieran Bingham 
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 20 
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index e1e181db90f7..65735a5c4350
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -41,14 +41,26 @@ static bool match_devname(struct v4l2_subdev *sd,
>   return !strcmp(asd->match.device_name.name, dev_name(sd->dev));
>  }
> 
/*
 * Check whether the two device_node pointers refer to the same OF node. We
 * can't compare pointers directly as they can differ if overlays have been
 * applied.
 */

> +static bool match_of(struct device_node *a, struct device_node *b)
> +{
> + return !of_node_cmp(of_node_full_name(a), of_node_full_name(b));
> +}
> +
>  static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)
> {
> + struct device_node *sdnode;
> + struct fwnode_handle *async_device;

I would name this asd_fwnode, and to be consistent rename sdnode to sd_ofnode.

> +
> + async_device = fwnode_graph_get_port_parent(asd->match.fwnode.fwnode);
> +
>   if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode))
> - return sd->fwnode == asd->match.fwnode.fwnode;
> + return sd->fwnode == asd->match.fwnode.fwnode ||
> +sd->fwnode == async_device;

I wonder whether we could simplify this by changing the 
fwnode_graph_get_port_parent() API. At the moment the function walks two or 
three levels up depending on whether there's a ports name or not. If we turned 
in into a function that accepts an endpoint, port or device node, and returns 
the device node unconditionally (basically, returning the argument if its name 
is not "port(@[0-9]+)?" or "endpoint(@[0-9]+)?", and walking up until it 
reaches the device node otherwise), you could write the above

asd_fwnode = fwnode_graph_get_port_parent(asd->match.fwnode.fwnode);

if (!is_of_node(sd->fwnode) || !is_of_node(asd_fwnode))
   sd->fwnode == asd_fwnode;

sdnode = to_of_node(sd->fwnode);
 
return match_of(sdnode, to_of_node(asd_node));

> +
> + sdnode = to_of_node(sd->fwnode);
> 
> - return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)),
> - of_node_full_name(
> - to_of_node(asd->match.fwnode.fwnode)));
> + return match_of(sdnode, to_of_node(asd->match.fwnode.fwnode)) ||
> +match_of(sdnode, to_of_node(async_device));

This is getting a bit complex, could you document the function ?

>  }
> 
>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/4] drm: rcar-du: Register a completion callback with VSP1

[Mauro Carvalho Chehab]

Em Fri,  5 May 2017 16:21:10 +0100
Kieran Bingham  escreveu:

> Currently we process page flip events on every display interrupt,
> however this does not take into consideration the processing time needed
> by the VSP1 utilised in the pipeline.
> 
> Register a callback with the VSP driver to obtain completion events, and
> track them so that we only perform page flips when the full display
> pipeline has completed for the frame.
> 
> Signed-off-by: Kieran Bingham 
> Reviewed-by: Laurent Pinchart 
> Signed-off-by: Laurent Pinchart 

For all parts of this series that touch drivers/media:

Acked-by: Mauro Carvalho Chehab 


> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  8 ++--
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  1 +
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  |  9 +
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 5f0664bcd12d..345eff72f581 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -378,7 +378,7 @@ static void rcar_du_crtc_update_planes(struct 
> rcar_du_crtc *rcrtc)
>   * Page Flip
>   */
>  
> -static void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
> +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
>  {
>   struct drm_pending_vblank_event *event;
>   struct drm_device *dev = rcrtc->crtc.dev;
> @@ -650,6 +650,7 @@ static const struct drm_crtc_funcs crtc_funcs = {
>  static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>  {
>   struct rcar_du_crtc *rcrtc = arg;
> + struct rcar_du_device *rcdu = rcrtc->group->dev;
>   irqreturn_t ret = IRQ_NONE;
>   u32 status;
>  
> @@ -658,7 +659,10 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>  
>   if (status & DSSR_FRM) {
>   drm_crtc_handle_vblank(>crtc);
> - rcar_du_crtc_finish_page_flip(rcrtc);
> +
> + if (rcdu->info->gen < 3)
> + rcar_du_crtc_finish_page_flip(rcrtc);
> +
>   ret = IRQ_HANDLED;
>   }
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h 
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 15871fae7445..b199ed5adf36 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -73,5 +73,6 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
>  
>  void rcar_du_crtc_route_output(struct drm_crtc *crtc,
>  enum rcar_du_output output);
> +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
>  
>  #endif /* __RCAR_DU_CRTC_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index b0ff304ce3dc..c7bb96fbfab1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -28,6 +28,13 @@
>  #include "rcar_du_kms.h"
>  #include "rcar_du_vsp.h"
>  
> +static void rcar_du_vsp_complete(void *private)
> +{
> + struct rcar_du_crtc *crtc = private;
> +
> + rcar_du_crtc_finish_page_flip(crtc);
> +}
> +
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  {
>   const struct drm_display_mode *mode = >crtc.state->adjusted_mode;
> @@ -35,6 +42,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>   struct vsp1_du_lif_config cfg = {
>   .width = mode->hdisplay,
>   .height = mode->vdisplay,
> + .callback = rcar_du_vsp_complete,
> + .callback_data = crtc,
>   };
>   struct rcar_du_plane_state state = {
>   .state = {



Thanks,
Mauro

[PATCH 08/13] staging: media: atomisp: Make undeclared symbols static

[Greg Kroah-Hartman]

From: Guru Das Srinagesh 

Fix sparse warnings: "symbol not declared; should it be static?"

Signed-off-by: Guru Das Srinagesh 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
index 7ce8803cf6f9..c151c848cf8f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_fops.c
@@ -130,9 +130,9 @@ static int atomisp_q_one_metadata_buffer(struct 
atomisp_sub_device *asd,
return 0;
 }
 
-int atomisp_q_one_s3a_buffer(struct atomisp_sub_device *asd,
-   enum atomisp_input_stream_id stream_id,
-   enum atomisp_css_pipe_id css_pipe_id)
+static int atomisp_q_one_s3a_buffer(struct atomisp_sub_device *asd,
+   enum atomisp_input_stream_id stream_id,
+   enum atomisp_css_pipe_id css_pipe_id)
 {
struct atomisp_s3a_buf *s3a_buf;
struct list_head *s3a_list;
@@ -172,9 +172,9 @@ int atomisp_q_one_s3a_buffer(struct atomisp_sub_device *asd,
return 0;
 }
 
-int atomisp_q_one_dis_buffer(struct atomisp_sub_device *asd,
-   enum atomisp_input_stream_id stream_id,
-   enum atomisp_css_pipe_id css_pipe_id)
+static int atomisp_q_one_dis_buffer(struct atomisp_sub_device *asd,
+   enum atomisp_input_stream_id stream_id,
+   enum atomisp_css_pipe_id css_pipe_id)
 {
struct atomisp_dis_buf *dis_buf;
unsigned long irqflags;
@@ -744,7 +744,7 @@ static void atomisp_subdev_init_struct(struct 
atomisp_sub_device *asd)
 /*
  * file operation functions
  */
-unsigned int atomisp_subdev_users(struct atomisp_sub_device *asd)
+static unsigned int atomisp_subdev_users(struct atomisp_sub_device *asd)
 {
return asd->video_out_preview.users +
   asd->video_out_vf.users +
-- 
2.13.0

[PATCH 10/13] staging: media: atomisp: one char read beyond end of string

[Greg Kroah-Hartman]

From: Dan Carpenter 

We should verify that "ix < max_len" before we test whether we have
reached the NUL terminator.

Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Reported-by: David Binderman 
Signed-off-by: Dan Carpenter 
Signed-off-by: Greg Kroah-Hartman 
---
 .../pci/atomisp2/css2400/hive_isp_css_include/string_support.h   | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
index 568631698a3d..74b5a1c7ac9a 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
@@ -72,9 +72,8 @@ static size_t strnlen_s(
return 0;
}
 
-   for (ix=0;
-   ((src_str[ix] != '\0') && (ix< max_len));
-   ++ix) /*Nothing else to do*/;
+   for (ix = 0; ix < max_len && src_str[ix] != '\0'; ix++)
+   ;
 
/* On Error, it will return src_size == max_len*/
return ix;
-- 
2.13.0

[PATCH 13/13] staging: media: atomisp: don't treat warnings as errors

[Greg Kroah-Hartman]

From: Mauro Carvalho Chehab 

Several atomisp files use:
 ccflags-y += -Werror

As, on media, our usual procedure is to use W=1, and atomisp
has *a lot* of warnings with such flag enabled,like:

./drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/system_local.h:62:26:
 warning: 'DDR_BASE' defined but not used [-Wunused-const-variable=]

At the end, it causes our build to fail, impacting our workflow.

So, remove this crap. If one wants to force -Werror, he
can still build with it enabled by passing a parameter to
make.

Signed-off-by: Mauro Carvalho Chehab 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/staging/media/atomisp/i2c/Makefile  | 2 --
 drivers/staging/media/atomisp/i2c/imx/Makefile  | 2 --
 drivers/staging/media/atomisp/i2c/ov5693/Makefile   | 2 --
 drivers/staging/media/atomisp/pci/atomisp2/Makefile | 2 +-
 4 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/Makefile 
b/drivers/staging/media/atomisp/i2c/Makefile
index 8ea01904c0ea..466517c7c8e6 100644
--- a/drivers/staging/media/atomisp/i2c/Makefile
+++ b/drivers/staging/media/atomisp/i2c/Makefile
@@ -19,5 +19,3 @@ obj-$(CONFIG_VIDEO_AP1302) += ap1302.o
 
 obj-$(CONFIG_VIDEO_LM3554) += lm3554.o
 
-ccflags-y += -Werror
-
diff --git a/drivers/staging/media/atomisp/i2c/imx/Makefile 
b/drivers/staging/media/atomisp/i2c/imx/Makefile
index 1d7f7ab94cac..6b13a3a66e49 100644
--- a/drivers/staging/media/atomisp/i2c/imx/Makefile
+++ b/drivers/staging/media/atomisp/i2c/imx/Makefile
@@ -4,5 +4,3 @@ imx1x5-objs := imx.o drv201.o ad5816g.o dw9714.o dw9719.o 
dw9718.o vcm.o otp.o o
 
 ov8858_driver-objs := ../ov8858.o dw9718.o vcm.o
 obj-$(CONFIG_VIDEO_OV8858) += ov8858_driver.o
-
-ccflags-y += -Werror
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/Makefile 
b/drivers/staging/media/atomisp/i2c/ov5693/Makefile
index fceb9e9b881b..c9c0e1245858 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/Makefile
+++ b/drivers/staging/media/atomisp/i2c/ov5693/Makefile
@@ -1,3 +1 @@
 obj-$(CONFIG_VIDEO_OV5693) += ov5693.o
-
-ccflags-y += -Werror
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/Makefile 
b/drivers/staging/media/atomisp/pci/atomisp2/Makefile
index 3fa7c1c1479f..f126a89a08e9 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/Makefile
+++ b/drivers/staging/media/atomisp/pci/atomisp2/Makefile
@@ -351,5 +351,5 @@ DEFINES := -DHRT_HW -DHRT_ISP_CSS_CUSTOM_HOST 
-DHRT_USE_VIR_ADDRS -D__HOST__
 DEFINES += -DATOMISP_POSTFIX=\"css2400b0_v21\" -DISP2400B0
 DEFINES += -DSYSTEM_hive_isp_css_2400_system -DISP2400
 
-ccflags-y += $(INCLUDES) $(DEFINES) -fno-common -Werror
+ccflags-y += $(INCLUDES) $(DEFINES) -fno-common
 
-- 
2.13.0

[PATCH 11/13] staging: media: atomisp: putting NULs in the wrong place

[Greg Kroah-Hartman]

From: Dan Carpenter 

We're putting the NUL terminators one space beyond where they belong.
This doesn't show up in testing because all but the callers put a NUL in
the correct place themselves.  LOL.  It causes a static checker warning
about buffer overflows.

Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
Signed-off-by: Dan Carpenter 
Signed-off-by: Greg Kroah-Hartman 
---
 .../pci/atomisp2/css2400/hive_isp_css_include/string_support.h| 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
index 74b5a1c7ac9a..c53241a7a281 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
@@ -117,7 +117,7 @@ STORAGE_CLASS_INLINE int strncpy_s(
 
/* dest_str is big enough for the len */
strncpy(dest_str, src_str, len);
-   dest_str[len+1] = '\0';
+   dest_str[len] = '\0';
return 0;
 }
 
@@ -157,7 +157,7 @@ STORAGE_CLASS_INLINE int strcpy_s(
 
/* dest_str is big enough for the len */
strncpy(dest_str, src_str, len);
-   dest_str[len+1] = '\0';
+   dest_str[len] = '\0';
return 0;
 }
 
-- 
2.13.0

[PATCH 12/13] staging: media: atomisp: fix missing blank line coding style issue in atomisp_tpg.c

[Greg Kroah-Hartman]

From: Manny Vindiola 

This is a patch to the atomisp_tpg.c file that fixes up a missing
blank line warning found by the checkpatch.pl tool

Signed-off-by: Manny Vindiola 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
index 996d1bdebad4..48b96048cab4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_tpg.c
@@ -56,6 +56,7 @@ static int tpg_set_fmt(struct v4l2_subdev *sd,
   struct v4l2_subdev_format *format)
 {
struct v4l2_mbus_framefmt *fmt = >format;
+
if (format->pad)
return -EINVAL;
/* only raw8 grbg is supported by TPG */
-- 
2.13.0

[PATCH 09/13] staging: media: atomisp: Fix -Werror=int-in-bool-context compile errors

[Greg Kroah-Hartman]

From: Hans de Goede 

With gcc-7.1.1 I was getting the following compile error:

error: ‘*’ in boolean context, suggest ‘&&’ instead

The problem is the definition of CEIL_DIV:
 #define CEIL_DIV(a, b)   ((b) ? ((a) + (b) - 1) / (b) : 0)

Which when called as: CEIL_DIV(x, y * z) triggers this error, note
we cannot do as the error suggests since b is evaluated multiple times.

This commit fixes these compile errors.

Signed-off-by: Hans de Goede 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c   | 1 -
 .../pci/atomisp2/css2400/hive_isp_css_include/math_support.h| 6 +++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
index b830b241e2e6..ad2c610d2ce3 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
@@ -2506,7 +2506,6 @@ static void __configure_capture_pp_input(struct 
atomisp_sub_device *asd,
struct ia_css_pipe_extra_config *pipe_extra_configs =
_env->pipe_extra_configs[pipe_id];
unsigned int hor_ds_factor = 0, ver_ds_factor = 0;
-#define CEIL_DIV(a, b)   ((b) ? ((a) + (b) - 1) / (b) : 0)
 
if (width == 0 && height == 0)
return;
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/math_support.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/math_support.h
index 48d84bc0ad9e..f74b405b0f39 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/math_support.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/math_support.h
@@ -62,15 +62,15 @@
 #define MAX(a, b)(((a) > (b)) ? (a) : (b))
 #define MIN(a, b)(((a) < (b)) ? (a) : (b))
 #ifdef ISP2401
-#define ROUND_DIV(a, b)  ((b) ? ((a) + ((b) >> 1)) / (b) : 0)
+#define ROUND_DIV(a, b)  (((b) != 0) ? ((a) + ((b) >> 1)) / (b) : 0)
 #endif
-#define CEIL_DIV(a, b)   ((b) ? ((a) + (b) - 1) / (b) : 0)
+#define CEIL_DIV(a, b)   (((b) != 0) ? ((a) + (b) - 1) / (b) : 0)
 #define CEIL_MUL(a, b)   (CEIL_DIV(a, b) * (b))
 #define CEIL_MUL2(a, b)  (((a) + (b) - 1) & ~((b) - 1))
 #define CEIL_SHIFT(a, b) (((a) + (1 << (b)) - 1)>>(b))
 #define CEIL_SHIFT_MUL(a, b) (CEIL_SHIFT(a, b) << (b))
 #ifdef ISP2401
-#define ROUND_HALF_DOWN_DIV(a, b)  ((b) ? ((a) + (b / 2) - 1) / (b) : 0)
+#define ROUND_HALF_DOWN_DIV(a, b)  (((b) != 0) ? ((a) + (b / 2) - 1) / (b) 
: 0)
 #define ROUND_HALF_DOWN_MUL(a, b)  (ROUND_HALF_DOWN_DIV(a, b) * (b))
 #endif
 
-- 
2.13.0

[PATCH 02/13] staging: media: atomisp: use logical AND, not bitwise

[Greg Kroah-Hartman]

From: Guru Das Srinagesh 

Fixes sparse warning "dubious: x & !y" in logical expression.

Signed-off-by: Guru Das Srinagesh 
Signed-off-by: Greg Kroah-Hartman 
---
 .../media/atomisp/pci/atomisp2/css2400/runtime/binary/src/binary.c  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/binary/src/binary.c
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/binary/src/binary.c
index a8b93a756e41..ae0b229c9fb8 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/binary/src/binary.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/binary/src/binary.c
@@ -1658,7 +1658,7 @@ ia_css_binary_find(struct ia_css_binary_descr *descr,
candidate->internal.max_height);
continue;
}
-   if (!candidate->enable.ds && need_ds & 
!(xcandidate->num_output_pins > 1)) {
+   if (!candidate->enable.ds && need_ds && 
!(xcandidate->num_output_pins > 1)) {
ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
"ia_css_binary_find() [%d] continue: !%d && 
%d\n",
__LINE__, candidate->enable.ds, (int)need_ds);
-- 
2.13.0

[PATCH 06/13] staging: media: atomisp: fixed coding style errors

[Greg Kroah-Hartman]

From: Avraham Shukron 

Fix for error (not warnings) reported by checkpatch.pl
Specifically:
 - missing whitespace around "=" and after ","
 - indentation with spaces instead of tabs
 - lines starting with a whitespace

This patch does not affect the compiled code in any way.

Signed-off-by: Avraham Shukron 
Signed-off-by: Greg Kroah-Hartman 
---
 .../platform/intel-mid/atomisp_gmin_platform.c | 156 ++---
 .../platform/intel-mid/intel_mid_pcihelpers.c  |   2 +-
 2 files changed, 79 insertions(+), 79 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c 
b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 15409e96449d..2a819ac6f9e2 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -51,7 +51,7 @@ struct gmin_subdev {
 
 static struct gmin_subdev gmin_subdevs[MAX_SUBDEVS];
 
-static enum { PMIC_UNSET = 0, PMIC_REGULATOR, PMIC_AXP, PMIC_TI ,
+static enum { PMIC_UNSET = 0, PMIC_REGULATOR, PMIC_AXP, PMIC_TI,
PMIC_CRYSTALCOVE } pmic_id;
 
 /* The atomisp uses type==0 for the end-of-list marker, so leave space. */
@@ -152,13 +152,13 @@ const struct camera_af_platform_data 
*camera_get_af_platform_data(void)
 EXPORT_SYMBOL_GPL(camera_get_af_platform_data);
 
 int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
-struct camera_sensor_platform_data *plat_data,
-enum intel_v4l2_subdev_type type)
+   struct camera_sensor_platform_data *plat_data,
+   enum intel_v4l2_subdev_type type)
 {
int i;
struct i2c_board_info *bi;
struct gmin_subdev *gs;
-struct i2c_client *client = v4l2_get_subdevdata(subdev);
+   struct i2c_client *client = v4l2_get_subdevdata(subdev);
struct acpi_device *adev;
 
dev_info(>dev, "register atomisp i2c module type %d\n", type);
@@ -172,7 +172,7 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
if (adev)
adev->power.flags.power_resources = 0;
 
-   for (i=0; i < MAX_SUBDEVS; i++)
+   for (i = 0; i < MAX_SUBDEVS; i++)
if (!pdata.subdevs[i].type)
break;
 
@@ -206,7 +206,7 @@ struct v4l2_subdev *atomisp_gmin_find_subdev(struct 
i2c_adapter *adapter,
 struct i2c_board_info *board_info)
 {
int i;
-   for (i=0; i < MAX_SUBDEVS && pdata.subdevs[i].type; i++) {
+   for (i = 0; i < MAX_SUBDEVS && pdata.subdevs[i].type; i++) {
struct intel_v4l2_subdev_table *sd = [i];
if (sd->v4l2_subdev.i2c_adapter_id == adapter->nr &&
sd->v4l2_subdev.board_info.addr == board_info->addr)
@@ -270,45 +270,45 @@ static const struct gmin_cfg_var t100_vars[] = {
 };
 
 static const struct gmin_cfg_var mrd7_vars[] = {
-{"INT33F8:00_CamType", "1"},
-{"INT33F8:00_CsiPort", "1"},
-{"INT33F8:00_CsiLanes","2"},
-{"INT33F8:00_CsiFmt","13"},
-{"INT33F8:00_CsiBayer", "0"},
-{"INT33F8:00_CamClk", "0"},
-{"INT33F9:00_CamType", "1"},
-{"INT33F9:00_CsiPort", "0"},
-{"INT33F9:00_CsiLanes","1"},
-{"INT33F9:00_CsiFmt","13"},
-{"INT33F9:00_CsiBayer", "0"},
-{"INT33F9:00_CamClk", "1"},
-{},
+   {"INT33F8:00_CamType", "1"},
+   {"INT33F8:00_CsiPort", "1"},
+   {"INT33F8:00_CsiLanes", "2"},
+   {"INT33F8:00_CsiFmt", "13"},
+   {"INT33F8:00_CsiBayer", "0"},
+   {"INT33F8:00_CamClk", "0"},
+   {"INT33F9:00_CamType", "1"},
+   {"INT33F9:00_CsiPort", "0"},
+   {"INT33F9:00_CsiLanes", "1"},
+   {"INT33F9:00_CsiFmt", "13"},
+   {"INT33F9:00_CsiBayer", "0"},
+   {"INT33F9:00_CamClk", "1"},
+   {},
 };
 
 static const struct gmin_cfg_var ecs7_vars[] = {
-{"INT33BE:00_CsiPort", "1"},
-{"INT33BE:00_CsiLanes","2"},
-{"INT33BE:00_CsiFmt","13"},
-{"INT33BE:00_CsiBayer", "2"},
-{"INT33BE:00_CamClk", "0"},
-{"INT33F0:00_CsiPort", "0"},
-{"INT33F0:00_CsiLanes","1"},
-{"INT33F0:00_CsiFmt","13"},
-{"INT33F0:00_CsiBayer", "0"},
-{"INT33F0:00_CamClk", "1"},
-{"gmin_V2P8GPIO","402"},
-{},
+   {"INT33BE:00_CsiPort", "1"},
+   {"INT33BE:00_CsiLanes", "2"},
+   {"INT33BE:00_CsiFmt", "13"},
+   {"INT33BE:00_CsiBayer", "2"},
+   {"INT33BE:00_CamClk", "0"},
+   {"INT33F0:00_CsiPort", "0"},
+   {"INT33F0:00_CsiLanes", "1"},
+   {"INT33F0:00_CsiFmt", "13"},
+   {"INT33F0:00_CsiBayer", "0"},
+   {"INT33F0:00_CamClk", "1"},
+   {"gmin_V2P8GPIO", "402"},
+   {},
 };
 
 
 static const struct gmin_cfg_var 

[PATCH 07/13] staging: media: atomisp: fix coding style warnings

[Greg Kroah-Hartman]

From: Avraham Shukron 

Fix for warnings reported by checkpatch.pl:
 - Multiline comment style
 - Bare "unsigned"
 - Missing blank line after declarations
 - Un-needed braces around single-statement branch

Signed-off-by: Avraham Shukron 
Signed-off-by: Greg Kroah-Hartman 
---
 .../platform/intel-mid/atomisp_gmin_platform.c | 45 ++
 .../platform/intel-mid/intel_mid_pcihelpers.c  |  9 +++--
 2 files changed, 35 insertions(+), 19 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c 
b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 2a819ac6f9e2..d68e9cf33aa7 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -119,7 +119,7 @@ static int af_power_ctrl(struct v4l2_subdev *subdev, int 
flag)
/*
 * The power here is used for dw9817,
 * regulator is from rear sensor
-   */
+*/
if (gs->v2p8_vcm_reg) {
if (flag)
return regulator_enable(gs->v2p8_vcm_reg);
@@ -167,7 +167,8 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
 * uses ACPI runtime power management for camera devices, but
 * we don't.  Disable it, or else the rails will be needlessly
 * tickled during suspend/resume.  This has caused power and
-* performance issues on multiple devices. */
+* performance issues on multiple devices.
+*/
adev = ACPI_COMPANION(>dev);
if (adev)
adev->power.flags.power_resources = 0;
@@ -182,7 +183,8 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
/* Note subtlety of initialization order: at the point where
 * this registration API gets called, the platform data
 * callbacks have probably already been invoked, so the
-* gmin_subdev struct is already initialized for us. */
+* gmin_subdev struct is already initialized for us.
+*/
gs = find_gmin_subdev(subdev);
 
pdata.subdevs[i].type = type;
@@ -206,8 +208,10 @@ struct v4l2_subdev *atomisp_gmin_find_subdev(struct 
i2c_adapter *adapter,
 struct i2c_board_info *board_info)
 {
int i;
+
for (i = 0; i < MAX_SUBDEVS && pdata.subdevs[i].type; i++) {
struct intel_v4l2_subdev_table *sd = [i];
+
if (sd->v4l2_subdev.i2c_adapter_id == adapter->nr &&
sd->v4l2_subdev.board_info.addr == board_info->addr)
return sd->subdev;
@@ -261,7 +265,8 @@ static const struct gmin_cfg_var ffrd8_vars[] = {
 };
 
 /* Cribbed from MCG defaults in the mt9m114 driver, not actually verified
- * vs. T100 hardware */
+ * vs. T100 hardware
+ */
 static const struct gmin_cfg_var t100_vars[] = {
{ "INT33F0:00_CsiPort",  "0" },
{ "INT33F0:00_CsiLanes", "1" },
@@ -345,10 +350,8 @@ static struct gmin_subdev *gmin_subdev_add(struct 
v4l2_subdev *subdev)
struct device *dev;
struct i2c_client *client = v4l2_get_subdevdata(subdev);
 
-   if (!pmic_id) {
-
-   pmic_id = PMIC_REGULATOR;
-   }
+   if (!pmic_id)
+   pmic_id = PMIC_REGULATOR;
 
if (!client)
return NULL;
@@ -401,7 +404,8 @@ static struct gmin_subdev *gmin_subdev_add(struct 
v4l2_subdev *subdev)
 * API is broken with the current drivers, returning
 * "1" for a regulator that will then emit a
 * "unbalanced disable" WARNing if we try to disable
-* it. */
+* it.
+*/
}
 
return _subdevs[i];
@@ -410,6 +414,7 @@ static struct gmin_subdev *gmin_subdev_add(struct 
v4l2_subdev *subdev)
 static struct gmin_subdev *find_gmin_subdev(struct v4l2_subdev *subdev)
 {
int i;
+
for (i = 0; i < MAX_SUBDEVS; i++)
if (gmin_subdevs[i].subdev == subdev)
return _subdevs[i];
@@ -419,6 +424,7 @@ static struct gmin_subdev *find_gmin_subdev(struct 
v4l2_subdev *subdev)
 static int gmin_gpio0_ctrl(struct v4l2_subdev *subdev, int on)
 {
struct gmin_subdev *gs = find_gmin_subdev(subdev);
+
if (gs && gs->gpio0) {
gpiod_set_value(gs->gpio0, on);
return 0;
@@ -429,6 +435,7 @@ static int gmin_gpio0_ctrl(struct v4l2_subdev *subdev, int 
on)
 static int gmin_gpio1_ctrl(struct v4l2_subdev *subdev, int on)
 {
struct gmin_subdev *gs = find_gmin_subdev(subdev);
+
if (gs && gs->gpio1) {
gpiod_set_value(gs->gpio1, on);
return 0;
@@ -532,6 +539,7 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, 
int on)
 {
int ret = 0;
struct gmin_subdev *gs 

[PATCH 01/13] staging: media: atomisp: Add __printf validation and fix fallout

[Greg Kroah-Hartman]

From: Joe Perches 

__printf validation adds format and argument validation.

Fix the various broken format/argument mismatches.

Signed-off-by: Joe Perches 
Signed-off-by: Greg Kroah-Hartman 
---
 .../isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c  |  6 +++---
 .../isp/kernels/sdis/sdis_2/ia_css_sdis2.host.c   |  2 +-
 .../css2400/isp/kernels/tnr/tnr_1.0/ia_css_tnr.host.c |  2 +-
 .../css2400/runtime/debug/interface/ia_css_debug.h|  1 +
 .../atomisp2/css2400/runtime/debug/src/ia_css_debug.c |  6 +++---
 .../media/atomisp/pci/atomisp2/css2400/sh_css.c   | 19 ++-
 .../media/atomisp/pci/atomisp2/css2400/sh_css_mipi.c  |  2 +-
 .../atomisp/pci/atomisp2/css2400/sh_css_params.c  | 10 +-
 8 files changed, 25 insertions(+), 23 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c
index 0daab1176865..9478c12abe89 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c
@@ -265,9 +265,9 @@ ia_css_translate_dvs_statistics(
assert(isp_stats->hor_proj != NULL);
assert(isp_stats->ver_proj != NULL);
 
-   IA_CSS_ENTER("hproj=%p, vproj=%p, haddr=%x, vaddr=%x",
-   host_stats->hor_proj, host_stats->ver_proj,
-   isp_stats->hor_proj, isp_stats->ver_proj);
+   IA_CSS_ENTER("hproj=%p, vproj=%p, haddr=%p, vaddr=%p",
+host_stats->hor_proj, host_stats->ver_proj,
+isp_stats->hor_proj, isp_stats->ver_proj);
 
hor_num_isp = host_stats->grid.aligned_height;
ver_num_isp = host_stats->grid.aligned_width;
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/sdis_2/ia_css_sdis2.host.c
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/sdis_2/ia_css_sdis2.host.c
index 5a0c103e9eb7..9bccb6473154 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/sdis_2/ia_css_sdis2.host.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/sdis/sdis_2/ia_css_sdis2.host.c
@@ -213,7 +213,7 @@ ia_css_translate_dvs2_statistics(
 "hor_coefs.even_real=%p, hor_coefs.even_imag=%p, "
 "ver_coefs.odd_real=%p, ver_coefs.odd_imag=%p, "
 "ver_coefs.even_real=%p, ver_coefs.even_imag=%p, "
-"haddr=%x, vaddr=%x",
+"haddr=%p, vaddr=%p",
host_stats->hor_prod.odd_real, host_stats->hor_prod.odd_imag,
host_stats->hor_prod.even_real, host_stats->hor_prod.even_imag,
host_stats->ver_prod.odd_real, host_stats->ver_prod.odd_imag,
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/tnr/tnr_1.0/ia_css_tnr.host.c
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/tnr/tnr_1.0/ia_css_tnr.host.c
index 804c19ab4485..222a7bd7f176 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/tnr/tnr_1.0/ia_css_tnr.host.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/isp/kernels/tnr/tnr_1.0/ia_css_tnr.host.c
@@ -55,7 +55,7 @@ ia_css_tnr_dump(
"tnr_coef", tnr->coef);
ia_css_debug_dtrace(level, "\t%-32s = %d\n",
"tnr_threshold_Y", tnr->threshold_Y);
-   ia_css_debug_dtrace(level, "\t%-32s = %d\n"
+   ia_css_debug_dtrace(level, "\t%-32s = %d\n",
"tnr_threshold_C", tnr->threshold_C);
 }
 
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/interface/ia_css_debug.h
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/interface/ia_css_debug.h
index be7df3a30c21..91c105cc6204 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/interface/ia_css_debug.h
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/interface/ia_css_debug.h
@@ -137,6 +137,7 @@ ia_css_debug_vdtrace(unsigned int level, const char *fmt, 
va_list args)
sh_css_vprint(fmt, args);
 }
 
+__printf(2, 3)
 extern void ia_css_debug_dtrace(unsigned int level, const char *fmt, ...);
 
 /*! @brief Dump sp thread's stack contents
diff --git 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
index 030810bd0878..bcc0d464084f 100644
--- 
a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
+++ 
b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
@@ -3148,8 +3148,8 @@ ia_css_debug_dump_pipe_config(

[PATCH 00/13] staging: media: atomisp queued up patches

[Greg Kroah-Hartman]

Hi Mauro,

Here's the set of accumulated atomisp staging patches that I had in my
to-review mailbox.  After this, my queue is empty, the driver is all
yours!  Good Luck! :)

thanks,

greg k-h

Avraham Shukron (3):
  staging: media: atomisp: fixed sparse warnings
  staging: media: atomisp: fixed coding style errors
  staging: media: atomisp: fix coding style warnings

Dan Carpenter (2):
  staging: media: atomisp: one char read beyond end of string
  staging: media: atomisp: putting NULs in the wrong place

Fabrizio Perria (1):
  staging: media: atomisp: Fix unnecessary initialization of static

Guru Das Srinagesh (2):
  staging: media: atomisp: use logical AND, not bitwise
  staging: media: atomisp: Make undeclared symbols static

Hans de Goede (1):
  staging: media: atomisp: Fix -Werror=int-in-bool-context compile
errors

Joe Perches (1):
  staging: media: atomisp: Add __printf validation and fix fallout

Manny Vindiola (1):
  staging: media: atomisp: fix missing blank line coding style issue in
atomisp_tpg.c

Mauro Carvalho Chehab (1):
  staging: media: atomisp: don't treat warnings as errors

Valentin Vidic (1):
  staging: media: atomisp: drop unused qos variable

 drivers/staging/media/atomisp/i2c/Makefile |   2 -
 drivers/staging/media/atomisp/i2c/imx/Makefile |   2 -
 drivers/staging/media/atomisp/i2c/ov5693/Makefile  |   2 -
 .../staging/media/atomisp/pci/atomisp2/Makefile|   2 +-
 .../atomisp/pci/atomisp2/atomisp_compat_css20.c|   1 -
 .../media/atomisp/pci/atomisp2/atomisp_fops.c  |  14 +-
 .../media/atomisp/pci/atomisp2/atomisp_tpg.c   |   1 +
 .../media/atomisp/pci/atomisp2/atomisp_v4l2.c  |   2 +-
 .../css2400/hive_isp_css_include/math_support.h|   6 +-
 .../css2400/hive_isp_css_include/string_support.h  |   9 +-
 .../isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c   |   6 +-
 .../isp/kernels/sdis/sdis_2/ia_css_sdis2.host.c|   2 +-
 .../isp/kernels/tnr/tnr_1.0/ia_css_tnr.host.c  |   2 +-
 .../atomisp2/css2400/runtime/binary/src/binary.c   |   2 +-
 .../css2400/runtime/debug/interface/ia_css_debug.h |   1 +
 .../css2400/runtime/debug/src/ia_css_debug.c   |   6 +-
 .../media/atomisp/pci/atomisp2/css2400/sh_css.c|  19 +-
 .../atomisp/pci/atomisp2/css2400/sh_css_mipi.c |   2 +-
 .../atomisp/pci/atomisp2/css2400/sh_css_params.c   |  10 +-
 .../platform/intel-mid/atomisp_gmin_platform.c | 210 +++--
 .../platform/intel-mid/intel_mid_pcihelpers.c  |  12 +-
 21 files changed, 162 insertions(+), 151 deletions(-)

-- 
2.13.0

[PATCH 04/13] staging: media: atomisp: fixed sparse warnings

[Greg Kroah-Hartman]

From: Avraham Shukron 

Added "static" storage class to 4 not-declared functions

Signed-off-by: Avraham Shukron 
Signed-off-by: Greg Kroah-Hartman 
---
 .../media/atomisp/platform/intel-mid/atomisp_gmin_platform.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git 
a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c 
b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 5b4506a71126..15409e96449d 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -436,7 +436,7 @@ static int gmin_gpio1_ctrl(struct v4l2_subdev *subdev, int 
on)
return -EINVAL;
 }
 
-int gmin_v1p2_ctrl(struct v4l2_subdev *subdev, int on)
+static int gmin_v1p2_ctrl(struct v4l2_subdev *subdev, int on)
 {
struct gmin_subdev *gs = find_gmin_subdev(subdev);
 
@@ -455,7 +455,8 @@ int gmin_v1p2_ctrl(struct v4l2_subdev *subdev, int on)
 
return -EINVAL;
 }
-int gmin_v1p8_ctrl(struct v4l2_subdev *subdev, int on)
+
+static int gmin_v1p8_ctrl(struct v4l2_subdev *subdev, int on)
 {
struct gmin_subdev *gs = find_gmin_subdev(subdev);
int ret;
@@ -491,7 +492,7 @@ int gmin_v1p8_ctrl(struct v4l2_subdev *subdev, int on)
return -EINVAL;
 }
 
-int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
+static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 {
struct gmin_subdev *gs = find_gmin_subdev(subdev);
int ret;
@@ -527,7 +528,7 @@ int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
return -EINVAL;
 }
 
-int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, int on)
+static int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, int on)
 {
int ret = 0;
struct gmin_subdev *gs = find_gmin_subdev(subdev);
-- 
2.13.0

[PATCH 05/13] staging: media: atomisp: drop unused qos variable

[Greg Kroah-Hartman]

From: Valentin Vidic 

Fixes a sparse warning:

drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c:35:5: 
warning: symbol 'qos' was not declared. Should it be static?

Signed-off-by: Valentin Vidic 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c | 1 -
 1 file changed, 1 deletion(-)

diff --git 
a/drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c 
b/drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c
index a6c0f5f8c3f8..b01463361943 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/intel_mid_pcihelpers.c
@@ -32,7 +32,6 @@ static DEFINE_SPINLOCK(msgbus_lock);
 
 static struct pci_dev *pci_root;
 static struct pm_qos_request pm_qos;
-int qos;
 
 #define DW_I2C_NEED_QOS(platform_is(INTEL_ATOM_BYT))
 
-- 
2.13.0

[PATCH 03/13] staging: media: atomisp: Fix unnecessary initialization of static

[Greg Kroah-Hartman]

From: Fabrizio Perria 

Fix checkpatch warning: removed unnecessary initialization of
static variable "skip_fwload" to 0 in source atomisp_v4l2.c

Signed-off-by: Fabrizio Perria 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c 
b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
index e3fdbdba0b34..a0478077a012 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
@@ -51,7 +51,7 @@
 /* G-Min addition: pull this in from intel_mid_pm.h */
 #define CSTATE_EXIT_LATENCY_C1  1
 
-static uint skip_fwload = 0;
+static uint skip_fwload;
 module_param(skip_fwload, uint, 0644);
 MODULE_PARM_DESC(skip_fwload, "Skip atomisp firmware load");
 
-- 
2.13.0

Re: [PATCH] ATOMISP: Tidies up code warnings and errors in file

[Greg KH]

On Mon, May 08, 2017 at 11:25:55PM +0100, Mark Railton wrote:
> Cleared up some errors and warnings in
> drivers/staging/media/atomisp/i2c/ap1302.c
> 
> Signed-off-by: Mark Railton 
> ---
>  drivers/staging/media/atomisp/i2c/ap1302.c | 83 
> ++
>  1 file changed, 50 insertions(+), 33 deletions(-)

Always be specific as to what exactly you are doing, and don't do
multiple different things in a single patch like you did here (hint,
"all warnings/errors isn't one thing".

thanks,

greg k-h

Re: [PATCH v1 2/3] device property: Add fwnode_graph_get_port_parent

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 17 May 2017 16:03:38 Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> V4L2 async notifiers can pass the endpoint fwnode rather than the device
> fwnode.

I'm not sure I would mention V4L2 in the commit message, as this is generic.

> Provide a helper to obtain the parent device fwnode without first
> parsing the remote-endpoint as per fwnode_graph_get_remote_port_parent.
> 
> Signed-off-by: Kieran Bingham 
> ---
>  drivers/base/property.c  | 25 +
>  include/linux/property.h |  2 ++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 627ebc9b570d..caf4316fe565 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1245,6 +1245,31 @@ fwnode_graph_get_next_endpoint(struct fwnode_handle
> *fwnode, EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
> 
>  /**
> + * fwnode_graph_get_port_parent - Return device node of a port endpoint
> + * @fwnode: Endpoint firmware node pointing of the port
> + *
> + * Extracts firmware node of the device the @fwnode belongs to.

I'm not too familiar with the fwnode API, but I know it's written in C, where 
functions don't extract something but return a value :-) How about

Return: the firmware node of the device the @endpoint belongs to.

> + */
> +struct fwnode_handle *
> +fwnode_graph_get_port_parent(struct fwnode_handle *fwnode)

This is akin to writing (unsigned int integer)

How about calling the variable endpoint ? That would also make the 
documentation clearer in my opinion, with "the @fwnode belongs to" replaced 
with "the @endpoint belongs to".

> +{
> + struct fwnode_handle *parent = NULL;
> +
> + if (is_of_node(fwnode)) {
> + struct device_node *node;
> +
> + node = of_graph_get_port_parent(to_of_node(fwnode));
> + if (node)
> + parent = >fwnode;

This part looks good to me, with the above small change,

Reviewed-by: Laurent Pinchart 

> + } else if (is_acpi_node(fwnode)) {
> + parent = acpi_node_get_parent(fwnode);

I can't comment on this one though.

> + }
> +
> + return parent;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_graph_get_port_parent);
> +
> +/**
>   * fwnode_graph_get_remote_port_parent - Return fwnode of a remote device
>   * @fwnode: Endpoint firmware node pointing to the remote endpoint
>   *
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 2f482616a2f2..624129b86c82 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -274,6 +274,8 @@ void *device_get_mac_address(struct device *dev, char
> *addr, int alen);
> 
>  struct fwnode_handle *fwnode_graph_get_next_endpoint(
>   struct fwnode_handle *fwnode, struct fwnode_handle *prev);
> +struct fwnode_handle *fwnode_graph_get_port_parent(
> + struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_graph_get_remote_port_parent(
>   struct fwnode_handle *fwnode);
>  struct fwnode_handle *fwnode_graph_get_remote_port(

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/3] of: base: Provide of_graph_get_port_parent()

[Laurent Pinchart]

Hi Kieran,

On Wednesday 17 May 2017 21:02:42 Kieran Bingham wrote:
> On 17/05/17 17:36, Rob Herring wrote:
> > On Wed, May 17, 2017 at 10:03 AM, Kieran Bingham wrote:
> >> From: Kieran Bingham 
> >> 
> >> When handling endpoints, the v4l2 async framework needs to identify the
> >> parent device of a port endpoint.
> >> 
> >> Adapt the existing of_graph_get_remote_port_parent() such that a caller
> >> can obtain the parent of a port without parsing the remote-endpoint
> >> first.
> > 
> > A similar patch is already applied as part of the ASoC graph card support.
> > 
> > Rob
> 
> Ah yes, a quick google finds it...
> 
> :  https://patchwork.kernel.org/patch/9658907/
> 
> Surprisingly similar patch ... and a familiar name.

Very similar indeed, down to identical problems ;-) Quoting your patch,

>  /**
> + * of_graph_get_port_parent() - get port's parent node
> + * @node: pointer to a local endpoint device_node
> + *
> + * Return: device node associated with endpoint @node.
> + *  Use of_node_put() on it when done.
> + */

The documentation of the return value is a bit confusing to me. I assume that, 
by device node, you meant the DT node corresponding to the device of the 
endpoint which is passed as an argument. However, device node cal also refer 
to struct device_node. Should this be clarified ?

> Morimoto-san - you beat me to it :D !
> 
> Thanks Rob, (And Morimoto!)

-- 
Regards,

Laurent Pinchart

[PATCH 3/5] [media] bt8xx: add missing break

[Mauro Carvalho Chehab]

The logic that handles CA_SET_PID is clearly missing a
break: it prints that the command succeeded, but, due to the
missing break, it would be returning -EOPNOTSUPP, as if the
driver weren't supporting such ioctl.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/pci/bt8xx/dst_ca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c
index 04d06c564602..90f4263452d3 100644
--- a/drivers/media/pci/bt8xx/dst_ca.c
+++ b/drivers/media/pci/bt8xx/dst_ca.c
@@ -637,6 +637,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int 
cmd, unsigned long ioct
goto free_mem_and_exit;
}
dprintk(verbose, DST_CA_INFO, 1, " -->CA_SET_PID Success !");
+   break;
default:
result = -EOPNOTSUPP;
}
-- 
2.9.3

[PATCH 1/5] [media] bcm3510: fix handling of VSB16 modulation

[Mauro Carvalho Chehab]

There's a missing break for VSB16 modulation logic, with would
cause it to return -EINVAL, instead of handling it.

Fix it.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/dvb-frontends/bcm3510.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/dvb-frontends/bcm3510.c 
b/drivers/media/dvb-frontends/bcm3510.c
index 617c5e29f919..30cfc0f2b575 100644
--- a/drivers/media/dvb-frontends/bcm3510.c
+++ b/drivers/media/dvb-frontends/bcm3510.c
@@ -538,6 +538,7 @@ static int bcm3510_set_frontend(struct dvb_frontend *fe)
cmd.ACQUIRE0.MODE = 0x9;
cmd.ACQUIRE1.SYM_RATE = 0x0;
cmd.ACQUIRE1.IF_FREQ = 0x0;
+   break;
default:
return -EINVAL;
}
-- 
2.9.3

[PATCH 2/5] [media] saa7164: better handle error codes

[Mauro Carvalho Chehab]

Right now, the driver is doing the right thing for
PVC_ERRORCODE_UNKNOWN and PVC_ERRORCODE_INVALID_CONTROL:
for both, it returns an error code (SAA_ERR_NOT_SUPPORTED).

However, it is printing two error messages instead of one
on those cases.

Fix the logic.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/pci/saa7164/saa7164-cmd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/saa7164/saa7164-cmd.c 
b/drivers/media/pci/saa7164/saa7164-cmd.c
index 175015ca79f2..dfebd77ada59 100644
--- a/drivers/media/pci/saa7164/saa7164-cmd.c
+++ b/drivers/media/pci/saa7164/saa7164-cmd.c
@@ -506,6 +506,8 @@ int saa7164_cmd_send(struct saa7164_dev *dev, u8 id, enum 
tmComResCmd command,
dprintk(DBGLVL_CMD,
"%s() UNKNOWN OR INVALID CONTROL\n",
__func__);
+   ret = SAA_ERR_NOT_SUPPORTED;
+   break;
default:
dprintk(DBGLVL_CMD, "%s() UNKNOWN\n", __func__);
ret = SAA_ERR_NOT_SUPPORTED;
-- 
2.9.3

[PATCH 4/5] [media] dvb-usb-remote: don't write bogus debug messages

[Mauro Carvalho Chehab]

When a REMOTE_KEY_PRESSED event happens, it does the right
thing. However, if debug is enabled, it will print a bogus
message warning that "key repeated".

Fix it.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/usb/dvb-usb/dvb-usb-remote.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/usb/dvb-usb/dvb-usb-remote.c 
b/drivers/media/usb/dvb-usb/dvb-usb-remote.c
index 059ded59208e..f05f1fc80729 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-remote.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-remote.c
@@ -131,6 +131,11 @@ static void legacy_dvb_usb_read_remote_control(struct 
work_struct *work)
case REMOTE_KEY_PRESSED:
deb_rc("key pressed\n");
d->last_event = event;
+   input_event(d->input_dev, EV_KEY, event, 1);
+   input_sync(d->input_dev);
+   input_event(d->input_dev, EV_KEY, d->last_event, 0);
+   input_sync(d->input_dev);
+   break;
case REMOTE_KEY_REPEAT:
deb_rc("key repeated\n");
input_event(d->input_dev, EV_KEY, event, 1);
-- 
2.9.3

Re: [RFC PATCH v2 1/4] media: i2c: adv748x: add adv748x driver

[Kieran Bingham]

Hi Sakari,

>>> In omap3isp/isp.c: isp_fwnodes_parse() the async notifier is registered 
>>> looking
>>> at the endpoints parent fwnode:
>>>
>>> isd->asd.match.fwnode.fwnode =
>>> fwnode_graph_get_remote_port_parent(fwnode);
>>>
>>> This would therefore not support ADV748x ... (it wouldn't know which TX/CSI
>>> entity to match against)
>>>
>>> So the way I see it is that all devices which currently register an async
>>> notifier should now register the match against the endpoint instead of the
>>> parent device:
>>>
>>>  - isd->asd.match.fwnode.fwnode = 
>>> fwnode_graph_get_remote_port_parent(fwnode);
>>>  + isd->asd.match.fwnode.fwnode = fwnode;
>>>
>>> And then if we adapt the match to check against:
>>>fwnode == fwnode || fwnode == 
>>> fwnode_graph_get_remote_port_parent(fwnode);
>>
>> That's not enough as a master driver may use device node whereas the
>> sub-device driver uses endpoint node. You need to do it both ways.
>>

I've worked through this and I'm convinced that it should not be both ways...

We are matching a Subdevice (SD) to an Async Subdevice Notifier (ASD)

Bringing in 'endpoints' gives us the following match combinations


  SD   ASD   : Result
  ep   ep: Match ... Subdevices can specify their endpoint node.
Notifiers can be updated to also specify the (remote) endpoint.

  dev  ep: Match  - We should take the parent of the ASD to match SD,
to support subdevices which default to their device node.

  dev  dev   : Match - This is the current case for all other drivers
but Notifier ASDs can be converted to EP's

  ep   dev   : No Match - We should *not* take the parent of the SD
If a subdevice has specified it's endpoint, (ADV748x)
Matching against the parent device is invalid.

Finding and matching the parent of the SD would allow a driver to register a
notifier ASD with a dev node, and expect to match a subdevice that has
registered it's subdev with an EP node. This would erroneously allow drivers to
register a notifier that would match against *both* of the ADV748x CSI2 
entities.

I have posted an implementation of this as:
 [PATCH v1 3/3] v4l: async: Match parent devices [0]

I believe this to be correct - but I'm aware that I'm only really considering
the OF side here... Please let me know if there's something I'm not taking into
account.

[0] https://www.spinics.net/lists/linux-media/msg115677.html

Regards
--
Kieran

[GIT PULL FOR v4.13] V4L2 fwnode support

[Sakari Ailus]

Hi Mauro,

This pull request introduces the V4L2 fwnode framework which has equivalent
functionality to V4L2 OF framework. The V4L2 OF framework users are
converted to use the V4L2 fwnode framework and the redundant V4L2 OF
framework is removed.

Please pull.

The following changes since commit f2c61f98e0b5f8b53b8fb860e5dcdd661bde7d0b:

  [media] tc358743: fix register i2c_rd/wr function fix (2017-05-18 08:00:56 
-0300)

are available in the git repository at:

  https://linuxtv.org/git/sailus/media_tree.git v4l2-acpi

for you to fetch changes up to e8519dce8869d48d9e935e1f6da82bc67899c9f5:

  v4l: Remove V4L2 OF framework in favour of V4L2 fwnode framework (2017-05-18 
14:18:33 +0300)


Sakari Ailus (7):
  v4l: fwnode: Support generic fwnode for parsing standardised properties
  v4l: async: Add fwnode match support
  v4l: flash led class: Use fwnode_handle instead of device_node in init
  v4l: Switch from V4L2 OF not V4L2 fwnode API
  docs-rst: media: Sort topic list alphabetically
  docs-rst: media: Switch documentation to V4L2 fwnode API
  v4l: Remove V4L2 OF framework in favour of V4L2 fwnode framework

 Documentation/media/kapi/v4l2-core.rst |  20 +-
 Documentation/media/kapi/v4l2-fwnode.rst   |   3 +
 Documentation/media/kapi/v4l2-of.rst   |   3 -
 drivers/leds/leds-aat1290.c|   5 +-
 drivers/leds/leds-max77693.c   |   5 +-
 drivers/media/i2c/Kconfig  |  11 +
 drivers/media/i2c/adv7604.c|   7 +-
 drivers/media/i2c/mt9v032.c|   7 +-
 drivers/media/i2c/ov2659.c |   8 +-
 drivers/media/i2c/ov5645.c |   7 +-
 drivers/media/i2c/ov5647.c |   7 +-
 drivers/media/i2c/s5c73m3/s5c73m3-core.c   |   7 +-
 drivers/media/i2c/s5k5baf.c|   6 +-
 drivers/media/i2c/smiapp/Kconfig   |   1 +
 drivers/media/i2c/smiapp/smiapp-core.c |  29 ++-
 drivers/media/i2c/tc358743.c   |  11 +-
 drivers/media/i2c/tvp514x.c|   6 +-
 drivers/media/i2c/tvp5150.c|   7 +-
 drivers/media/i2c/tvp7002.c|   6 +-
 drivers/media/platform/Kconfig |   3 +
 drivers/media/platform/am437x/Kconfig  |   1 +
 drivers/media/platform/am437x/am437x-vpfe.c|  15 +-
 drivers/media/platform/atmel/Kconfig   |   2 +
 drivers/media/platform/atmel/atmel-isc.c   |  13 +-
 drivers/media/platform/atmel/atmel-isi.c   |  11 +-
 drivers/media/platform/exynos4-is/Kconfig  |   2 +
 drivers/media/platform/exynos4-is/media-dev.c  |  13 +-
 drivers/media/platform/exynos4-is/mipi-csis.c  |   6 +-
 drivers/media/platform/omap3isp/isp.c  |  49 ++--
 drivers/media/platform/pxa_camera.c|  11 +-
 drivers/media/platform/rcar-vin/Kconfig|   1 +
 drivers/media/platform/rcar-vin/rcar-core.c|  19 +-
 drivers/media/platform/soc_camera/soc_camera.c |   7 +-
 drivers/media/platform/ti-vpe/cal.c|  15 +-
 drivers/media/platform/xilinx/Kconfig  |   1 +
 drivers/media/platform/xilinx/xilinx-vipp.c|  63 +++--
 drivers/media/v4l2-core/Kconfig|   3 +
 drivers/media/v4l2-core/Makefile   |   4 +-
 drivers/media/v4l2-core/v4l2-async.c   |  21 +-
 drivers/media/v4l2-core/v4l2-flash-led-class.c |  12 +-
 drivers/media/v4l2-core/v4l2-fwnode.c  | 345 +
 drivers/media/v4l2-core/v4l2-of.c  | 327 ---
 include/media/v4l2-async.h |   8 +-
 include/media/v4l2-flash-led-class.h   |   4 +-
 include/media/{v4l2-of.h => v4l2-fwnode.h} |  96 +++
 include/media/v4l2-subdev.h|   5 +-
 46 files changed, 634 insertions(+), 579 deletions(-)
 create mode 100644 Documentation/media/kapi/v4l2-fwnode.rst
 delete mode 100644 Documentation/media/kapi/v4l2-of.rst
 create mode 100644 drivers/media/v4l2-core/v4l2-fwnode.c
 delete mode 100644 drivers/media/v4l2-core/v4l2-of.c
 rename include/media/{v4l2-of.h => v4l2-fwnode.h} (50%)

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH 08/10] media: camss: Add files which handle the video device nodes

[Todor Tomov]

Hi Laurent,

On 01/19/2017 12:43 PM, Todor Tomov wrote:
> Hi Laurent,
> 
> Thank you for the detailed review.
> 
> On 12/05/2016 05:22 PM, Laurent Pinchart wrote:
>> Hi Todor,
>>
>> Thank you for the patch.
>>
>> On Friday 25 Nov 2016 16:57:20 Todor Tomov wrote:
>>> These files handle the video device nodes of the camss driver.
>>
>> camss is a quite generic, I'm a bit concerned about claiming that acronym in 
>> the global kernel namespace. Would it be too long if we prefixed symbols 
>> with 
>> msm_camss instead ?
> 
> Ok. Are you concerned about camss_enable_clocks() and camss_disable_clocks() 
> or
> you have something else in mind too?

Could you please add more details about this?

> 
>>
>>> Signed-off-by: Todor Tomov 
>>> ---
>>>  drivers/media/platform/qcom/camss-8x16/video.c | 597 ++
>>>  drivers/media/platform/qcom/camss-8x16/video.h |  67 +++
>>>  2 files changed, 664 insertions(+)
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.c
>>>  create mode 100644 drivers/media/platform/qcom/camss-8x16/video.h
>>>
>>> diff --git a/drivers/media/platform/qcom/camss-8x16/video.c
>>> b/drivers/media/platform/qcom/camss-8x16/video.c new file mode 100644
>>> index 000..0bf8ea9
>>> --- /dev/null
>>> +++ b/drivers/media/platform/qcom/camss-8x16/video.c



>>> +/* 
>>> + * V4L2 file operations
>>> + */
>>> +
>>> +/*
>>> + * video_init_format - Helper function to initialize format
>>> + *
>>> + * Initialize all pad formats with default values.
>>> + */
>>> +static int video_init_format(struct file *file, void *fh)
>>> +{
>>> +   struct v4l2_format format;
>>> +
>>> +   memset(, 0, sizeof(format));
>>> +   format.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +
>>> +   return video_s_fmt(file, fh, );
>>
>> This will set the active format every time you open the device node, I don't 
>> think that's what you want.
> 
> Well, actually this is what I wanted. I wanted to keep in sync the pixel 
> format
> on the video node and the media bus format on the subdev node (i.e. the pixel
> format will be always correct for the current media bus format). For the 
> current
> version there is a direct correspondence between the pixel format and the 
> media
> format so this will work I think. For the future there might be multiple pixel
> formats for one media bus format and a second open of the video node could 
> reset
> the pixel format to unwanted value so this will need a change. I'm wondering 
> about
> (and still not able to find) a good moment/event when to perform the 
> initialization
> of the format on the video node. As it gets the current format from the subdev
> node, the moment of the registration will be too early as the media link is 
> still
> not created. But after that I couldn't find a suitable callback/event where 
> to do
> it. If you can share any idea about this, please do :)

I still haven't found a better solution for this. If you have something in mind,
please share.

> 
>>
>>> +}
>>> +
>>> +static int video_open(struct file *file)
>>> +{
>>> +   struct video_device *vdev = video_devdata(file);
>>> +   struct camss_video *video = video_drvdata(file);
>>> +   struct camss_video_fh *handle;
>>> +   int ret;
>>> +
>>> +   handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>>> +   if (handle == NULL)
>>> +   return -ENOMEM;
>>> +
>>> +   v4l2_fh_init(>vfh, video->vdev);
>>> +   v4l2_fh_add(>vfh);
>>> +
>>> +   handle->video = video;
>>> +   file->private_data = >vfh;
>>> +
>>> +   ret = v4l2_pipeline_pm_use(>entity, 1);
>>> +   if (ret < 0) {
>>> +   dev_err(video->camss->dev, "Failed to power up pipeline\n");
>>> +   goto error_pm_use;
>>> +   }
>>> +
>>> +   ret = video_init_format(file, >vfh);
>>> +   if (ret < 0) {
>>> +   dev_err(video->camss->dev, "Failed to init format\n");
>>> +   goto error_init_format;
>>> +   }
>>> +
>>> +   return 0;
>>> +
>>> +error_init_format:
>>> +   v4l2_pipeline_pm_use(>entity, 0);
>>> +
>>> +error_pm_use:
>>> +   v4l2_fh_del(>vfh);
>>> +   kfree(handle);
>>> +
>>> +   return ret;
>>> +}

-- 
Best regards,
Todor Tomov

Re: [PATCH] [media] atomisp: don't treat warnings as errors

[Mauro Carvalho Chehab]

Em Thu, 18 May 2017 11:05:24 +0200
Arnd Bergmann  escreveu:

> On Thu, May 18, 2017 at 10:45 AM, Mauro Carvalho Chehab
>  wrote:
> > Several atomisp files use:
> >  ccflags-y += -Werror
> >
> > As, on media, our usual procedure is to use W=1, and atomisp
> > has *a lot* of warnings with such flag enabled,like:
> >
> > ./drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/system_local.h:62:26:
> >  warning: 'DDR_BASE' defined but not used [-Wunused-const-variable=]
> >
> > At the end, it causes our build to fail, impacting our workflow.
> >
> > So, remove this crap. If one wants to force -Werror, he
> > can still build with it enabled by passing a parameter to
> > make.
> >
> > Signed-off-by: Mauro Carvalho Chehab   
> 
> Good idea.
> 
> On a related note, I have some plans for more fine-grained and more consisten
> control of warning and error messages. The same way we already use W=1
> or W=12, I would like to allow E=0 E=01 etc to turn warnings of a particular
> W= level into errors, and possibly even allow this on a per-file or
> per-directory

That sounds very promising!

> basis. It depends on some infrastructure to replace scripts/Makefile.extrawarn
> with a include/linux/compiler-warnings.h using _Pragma("GCC diagnostic ..."),
> but that infrastructure has other benefits as well.
> 
> Would you be interested in having the equivalent of W=1 (some extra warnings)
> or E=0 (default warnings become errors) enabled for drivers/media if we had
> a good way of doing that?

Yeah, sure!

Thanks,
Mauro

Re: [PATCH] rc-core: cleanup rc_register_device (v2)

[Sean Young]

On Thu, May 18, 2017 at 09:55:14AM +0200, David Härdeman wrote:
> On Wed, May 17, 2017 at 09:09:57PM +0100, Sean Young wrote:
> >Hi David,
> >
> >On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
> >> The device core infrastructure is based on the presumption that
> >> once a driver calls device_add(), it must be ready to accept
> >> userspace interaction.
> >> 
> >> This requires splitting rc_setup_rx_device() into two functions
> >> and reorganizing rc_register_device() so that as much work
> >> as possible is performed before calling device_add().
> >> 
> >> Version 2: switch the order in which rc_prepare_rx_device() and
> >> ir_raw_event_prepare() gets called so that dev->change_protocol()
> >> gets called before device_add().
> >
> >I've looked at this patch and I don't see what problem it solves;
> >what user-space interaction is problematic?
> 
> It's a preparatory patch, the next patch ("rc-core: cleanup
> rc_register_device pt2") is the one which removes the dev->initialized
> hack (which currently papers over the fact that device_add() is called
> before userspace is actually ready to accept sysfs interaction).
> 
> Does that answer your question?

I suspected that but the commit message does not make it obvious.


Sean

Re: [PATCH] [media] atomisp: don't treat warnings as errors

[Arnd Bergmann]

On Thu, May 18, 2017 at 10:45 AM, Mauro Carvalho Chehab
 wrote:
> Several atomisp files use:
>  ccflags-y += -Werror
>
> As, on media, our usual procedure is to use W=1, and atomisp
> has *a lot* of warnings with such flag enabled,like:
>
> ./drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/system_local.h:62:26:
>  warning: 'DDR_BASE' defined but not used [-Wunused-const-variable=]
>
> At the end, it causes our build to fail, impacting our workflow.
>
> So, remove this crap. If one wants to force -Werror, he
> can still build with it enabled by passing a parameter to
> make.
>
> Signed-off-by: Mauro Carvalho Chehab 

Good idea.

On a related note, I have some plans for more fine-grained and more consisten
control of warning and error messages. The same way we already use W=1
or W=12, I would like to allow E=0 E=01 etc to turn warnings of a particular
W= level into errors, and possibly even allow this on a per-file or
per-directory
basis. It depends on some infrastructure to replace scripts/Makefile.extrawarn
with a include/linux/compiler-warnings.h using _Pragma("GCC diagnostic ..."),
but that infrastructure has other benefits as well.

Would you be interested in having the equivalent of W=1 (some extra warnings)
or E=0 (default warnings become errors) enabled for drivers/media if we had
a good way of doing that?

  Arnd

[PATCH] [media] atomisp: don't treat warnings as errors

[Mauro Carvalho Chehab]

Several atomisp files use:
 ccflags-y += -Werror

As, on media, our usual procedure is to use W=1, and atomisp
has *a lot* of warnings with such flag enabled,like:

./drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/system_local.h:62:26:
 warning: 'DDR_BASE' defined but not used [-Wunused-const-variable=]

At the end, it causes our build to fail, impacting our workflow.

So, remove this crap. If one wants to force -Werror, he
can still build with it enabled by passing a parameter to
make.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/staging/media/atomisp/i2c/Makefile  | 2 --
 drivers/staging/media/atomisp/i2c/imx/Makefile  | 2 --
 drivers/staging/media/atomisp/i2c/ov5693/Makefile   | 2 --
 drivers/staging/media/atomisp/pci/atomisp2/Makefile | 2 +-
 4 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/Makefile 
b/drivers/staging/media/atomisp/i2c/Makefile
index 8ea01904c0ea..466517c7c8e6 100644
--- a/drivers/staging/media/atomisp/i2c/Makefile
+++ b/drivers/staging/media/atomisp/i2c/Makefile
@@ -19,5 +19,3 @@ obj-$(CONFIG_VIDEO_AP1302) += ap1302.o
 
 obj-$(CONFIG_VIDEO_LM3554) += lm3554.o
 
-ccflags-y += -Werror
-
diff --git a/drivers/staging/media/atomisp/i2c/imx/Makefile 
b/drivers/staging/media/atomisp/i2c/imx/Makefile
index 1d7f7ab94cac..6b13a3a66e49 100644
--- a/drivers/staging/media/atomisp/i2c/imx/Makefile
+++ b/drivers/staging/media/atomisp/i2c/imx/Makefile
@@ -4,5 +4,3 @@ imx1x5-objs := imx.o drv201.o ad5816g.o dw9714.o dw9719.o 
dw9718.o vcm.o otp.o o
 
 ov8858_driver-objs := ../ov8858.o dw9718.o vcm.o
 obj-$(CONFIG_VIDEO_OV8858) += ov8858_driver.o
-
-ccflags-y += -Werror
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/Makefile 
b/drivers/staging/media/atomisp/i2c/ov5693/Makefile
index fceb9e9b881b..c9c0e1245858 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/Makefile
+++ b/drivers/staging/media/atomisp/i2c/ov5693/Makefile
@@ -1,3 +1 @@
 obj-$(CONFIG_VIDEO_OV5693) += ov5693.o
-
-ccflags-y += -Werror
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/Makefile 
b/drivers/staging/media/atomisp/pci/atomisp2/Makefile
index 3fa7c1c1479f..f126a89a08e9 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/Makefile
+++ b/drivers/staging/media/atomisp/pci/atomisp2/Makefile
@@ -351,5 +351,5 @@ DEFINES := -DHRT_HW -DHRT_ISP_CSS_CUSTOM_HOST 
-DHRT_USE_VIR_ADDRS -D__HOST__
 DEFINES += -DATOMISP_POSTFIX=\"css2400b0_v21\" -DISP2400B0
 DEFINES += -DSYSTEM_hive_isp_css_2400_system -DISP2400
 
-ccflags-y += $(INCLUDES) $(DEFINES) -fno-common -Werror
+ccflags-y += $(INCLUDES) $(DEFINES) -fno-common
 
-- 
2.9.3

[PATCH] cec: stih: fix typos in comments

[Benjamin Gaignard]

Minor fixes in comments

Signed-off-by: Benjamin Gaignard 
---
 drivers/media/platform/sti/cec/stih-cec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/sti/cec/stih-cec.c 
b/drivers/media/platform/sti/cec/stih-cec.c
index 39ff551..65ee143 100644
--- a/drivers/media/platform/sti/cec/stih-cec.c
+++ b/drivers/media/platform/sti/cec/stih-cec.c
@@ -1,6 +1,6 @@
 /*
  * STIH4xx CEC driver
- * Copyright (C) STMicroelectronic SA 2016
+ * Copyright (C) STMicroelectronics SA 2016
  *
  * 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
@@ -213,7 +213,8 @@ static int stih_cec_adap_transmit(struct cec_adapter *adap, 
u8 attempts,
for (i = 0; i < msg->len; i++)
writeb(msg->msg[i], cec->regs + CEC_TX_DATA_BASE + i);
 
-   /* Start transmission, configure hardware to add start and stop bits
+   /*
+* Start transmission, configure hardware to add start and stop bits
 * Signal free time is handled by the hardware
 */
writel(CEC_TX_AUTO_SOM_EN | CEC_TX_AUTO_EOM_EN | CEC_TX_START |
-- 
1.9.1

[PATCH] cec: stih: allow to use max CEC logical addresses

[Benjamin Gaignard]

Hardware could support up to 16 logical addresses which is more
than needed by CEC specifications.
Let use CEC_MAX_LOG_ADDRS instead of limited it on one.
stih_cec_adap_log_addr() function was alredy written to support
multiple addresses requests.

Signed-off-by: Benjamin Gaignard 
---
 drivers/media/platform/sti/cec/stih-cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/sti/cec/stih-cec.c 
b/drivers/media/platform/sti/cec/stih-cec.c
index 65ee143..6f9f036 100644
--- a/drivers/media/platform/sti/cec/stih-cec.c
+++ b/drivers/media/platform/sti/cec/stih-cec.c
@@ -354,7 +354,7 @@ static int stih_cec_probe(struct platform_device *pdev)
cec->adap = cec_allocate_adapter(_cec_adap_ops, cec,
CEC_NAME,
CEC_CAP_LOG_ADDRS | CEC_CAP_PASSTHROUGH |
-   CEC_CAP_TRANSMIT, 1);
+   CEC_CAP_TRANSMIT, CEC_MAX_LOG_ADDRS);
ret = PTR_ERR_OR_ZERO(cec->adap);
if (ret)
return ret;
-- 
1.9.1

Re: [PATCH] rc-core: cleanup rc_register_device (v2)

[David Härdeman]

On Wed, May 17, 2017 at 09:09:57PM +0100, Sean Young wrote:
>Hi David,
>
>On Wed, May 03, 2017 at 12:04:00PM +0200, David Härdeman wrote:
>> The device core infrastructure is based on the presumption that
>> once a driver calls device_add(), it must be ready to accept
>> userspace interaction.
>> 
>> This requires splitting rc_setup_rx_device() into two functions
>> and reorganizing rc_register_device() so that as much work
>> as possible is performed before calling device_add().
>> 
>> Version 2: switch the order in which rc_prepare_rx_device() and
>> ir_raw_event_prepare() gets called so that dev->change_protocol()
>> gets called before device_add().
>
>I've looked at this patch and I don't see what problem it solves;
>what user-space interaction is problematic?

It's a preparatory patch, the next patch ("rc-core: cleanup
rc_register_device pt2") is the one which removes the dev->initialized
hack (which currently papers over the fact that device_add() is called
before userspace is actually ready to accept sysfs interaction).

Does that answer your question?

//David