cron job: media_tree daily build: ERRORS

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

Results of the daily build of media_tree:

date:   Wed Nov 28 05:00:12 CET 2018
media-tree git hash:708d75fe1c7c6e9abc5381b6fcc32b49830383d0
media_build git hash:   466e4e6f12eeffd6e9f6d91378c9169f7e6b8527
v4l-utils git hash: ff41164e10010be7209d3b7b98c209f98407a320
edid-decode git hash:   5eeb151a748788666534d6ea3da07f90400d24c2
gcc version:i686-linux-gcc (GCC) 8.2.0
sparse version: 0.5.2
smatch version: 0.5.1
host hardware:  x86_64
host os:4.18.0-2-amd64

linux-git-arm-at91: WARNINGS
linux-git-arm-davinci: WARNINGS
linux-git-arm-multi: WARNINGS
linux-git-arm-pxa: WARNINGS
linux-git-arm-stm32: WARNINGS
linux-git-arm64: OK
linux-git-i686: WARNINGS
linux-git-mips: OK
linux-git-powerpc64: WARNINGS
linux-git-sh: OK
linux-git-x86_64: WARNINGS
Check COMPILE_TEST: OK
linux-3.10.108-i686: OK
linux-3.10.108-x86_64: OK
linux-3.11.10-i686: OK
linux-3.11.10-x86_64: OK
linux-3.12.74-i686: OK
linux-3.12.74-x86_64: OK
linux-3.13.11-i686: OK
linux-3.13.11-x86_64: OK
linux-3.14.79-i686: OK
linux-3.14.79-x86_64: OK
linux-3.15.10-i686: OK
linux-3.15.10-x86_64: OK
linux-3.16.57-i686: OK
linux-3.16.57-x86_64: OK
linux-3.17.8-i686: OK
linux-3.17.8-x86_64: OK
linux-3.18.123-i686: OK
linux-3.18.123-x86_64: OK
linux-3.19.8-i686: ERRORS
linux-3.19.8-x86_64: ERRORS
linux-4.0.9-i686: ERRORS
linux-4.0.9-x86_64: ERRORS
linux-4.1.52-i686: OK
linux-4.1.52-x86_64: OK
linux-4.2.8-i686: OK
linux-4.2.8-x86_64: OK
linux-4.3.6-i686: OK
linux-4.3.6-x86_64: OK
linux-4.4.159-i686: OK
linux-4.4.159-x86_64: OK
linux-4.5.7-i686: OK
linux-4.5.7-x86_64: OK
linux-4.6.7-i686: OK
linux-4.6.7-x86_64: OK
linux-4.7.10-i686: OK
linux-4.7.10-x86_64: OK
linux-4.8.17-i686: OK
linux-4.8.17-x86_64: OK
linux-4.9.131-i686: OK
linux-4.9.131-x86_64: OK
linux-4.10.17-i686: OK
linux-4.10.17-x86_64: OK
linux-4.11.12-i686: OK
linux-4.11.12-x86_64: OK
linux-4.12.14-i686: OK
linux-4.12.14-x86_64: OK
linux-4.13.16-i686: OK
linux-4.13.16-x86_64: OK
linux-4.14.74-i686: OK
linux-4.14.74-x86_64: OK
linux-4.15.18-i686: OK
linux-4.15.18-x86_64: OK
linux-4.16.18-i686: OK
linux-4.16.18-x86_64: OK
linux-4.17.19-i686: OK
linux-4.17.19-x86_64: OK
linux-4.18.12-i686: OK
linux-4.18.12-x86_64: OK
linux-4.19.1-i686: OK
linux-4.19.1-x86_64: OK
linux-4.20-rc1-i686: OK
linux-4.20-rc1-x86_64: OK
apps: OK
spec-git: OK
sparse: WARNINGS

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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


[PATCH v2] v4l2-ioctl: Zero v4l2_plane_pix_format reserved fields

2018-11-27 Thread Ezequiel Garcia
Make the core set the reserved fields to zero in
vv4l2_pix_format_mplane.4l2_plane_pix_format,
for _MPLANE queue types.

Moving this to the core avoids having to do so in each
and every driver.

Suggested-by: Tomasz Figa 
Signed-off-by: Ezequiel Garcia 
--
v2:
  * Drop unneeded clear in g_fmt.
The sturct was already being cleared here.
  * Only zero plane_fmt reserved fields.
  * Use CLEAR_FIELD_MACRO.

 drivers/media/v4l2-core/v4l2-ioctl.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index e384142d2826..2506b602481f 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1512,6 +1512,7 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
struct v4l2_format *p = arg;
struct video_device *vfd = video_devdata(file);
int ret = check_fmt(file, p->type);
+   int i;
 
if (ret)
return ret;
@@ -1536,6 +1537,8 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
if (unlikely(!ops->vidioc_s_fmt_vid_cap_mplane))
break;
CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
+   for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
+   CLEAR_AFTER_FIELD(p, 
fmt.pix_mp.plane_fmt[i].bytesperline);
return ops->vidioc_s_fmt_vid_cap_mplane(file, fh, arg);
case V4L2_BUF_TYPE_VIDEO_OVERLAY:
if (unlikely(!ops->vidioc_s_fmt_vid_overlay))
@@ -1564,6 +1567,8 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
if (unlikely(!ops->vidioc_s_fmt_vid_out_mplane))
break;
CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
+   for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
+   CLEAR_AFTER_FIELD(p, 
fmt.pix_mp.plane_fmt[i].bytesperline);
return ops->vidioc_s_fmt_vid_out_mplane(file, fh, arg);
case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
if (unlikely(!ops->vidioc_s_fmt_vid_out_overlay))
@@ -1604,6 +1609,7 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
 {
struct v4l2_format *p = arg;
int ret = check_fmt(file, p->type);
+   int i;
 
if (ret)
return ret;
@@ -1623,6 +1629,8 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
if (unlikely(!ops->vidioc_try_fmt_vid_cap_mplane))
break;
CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
+   for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
+   CLEAR_AFTER_FIELD(p, 
fmt.pix_mp.plane_fmt[i].bytesperline);
return ops->vidioc_try_fmt_vid_cap_mplane(file, fh, arg);
case V4L2_BUF_TYPE_VIDEO_OVERLAY:
if (unlikely(!ops->vidioc_try_fmt_vid_overlay))
@@ -1651,6 +1659,8 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
if (unlikely(!ops->vidioc_try_fmt_vid_out_mplane))
break;
CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
+   for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
+   CLEAR_AFTER_FIELD(p, 
fmt.pix_mp.plane_fmt[i].bytesperline);
return ops->vidioc_try_fmt_vid_out_mplane(file, fh, arg);
case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
if (unlikely(!ops->vidioc_try_fmt_vid_out_overlay))
-- 
2.19.1



Re: [PATCH v5 0/9] Asynchronous UVC

2018-11-27 Thread Laurent Pinchart
Hi Pavel,

On Tuesday, 27 November 2018 22:17:30 EET Pavel Machek wrote:
> On Tue 2018-11-06 21:27:11, Kieran Bingham wrote:
> > From: Kieran Bingham 
> > 
> > The Linux UVC driver has long provided adequate performance capabilities
> > for web-cams and low data rate video devices in Linux while resolutions
> > were low.
> > 
> > Modern USB cameras are now capable of high data rates thanks to USB3 with
> > 1080p, and even 4k capture resolutions supported.
> > 
> > Cameras such as the Stereolabs ZED (bulk transfers) or the Logitech BRIO
> > (isochronous transfers) can generate more data than an embedded ARM core
> > is able to process on a single core, resulting in frame loss.
> > 
> > A large part of this performance impact is from the requirement to
> > ‘memcpy’ frames out from URB packets to destination frames. This
> > unfortunate requirement is due to the UVC protocol allowing a variable
> > length header, and thus it is not possible to provide the target frame
> > buffers directly.
> > 
> > Extra throughput is possible by moving the actual memcpy actions to a work
> > queue, and moving the memcpy out of interrupt context thus allowing work
> > tasks to be scheduled across multiple cores.
> 
> Hmm. Doing memcpy() on many cores is improvement but... not really.
> Would it be possible to improve kernel<->user interface, so it says
> "data is in this buffer, and it starts here" and so that memcpy in
> kernel is not neccessary?

Unfortunately not, as the UVC protocol segments the frame in a large number of 
small packets, each prefixed with a variable-length header. It's a poorly 
designed protocol from that point of view.

-- 
Regards,

Laurent Pinchart





Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity

2018-11-27 Thread Sakari Ailus
On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> From: Enrico Scholz 
> 
> The chip can be configured to output data transitions on the
> rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> falling edge.
> 
> Parsing the fw-node is made in a subfunction to bundle all (future)
> dt-parsing / fw-parsing stuff.
> 
> Signed-off-by: Enrico Scholz 
> (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> per default. Set bit to 0 (enable mask bit without value) to enable
> falling edge sampling.)
> Signed-off-by: Michael Grzeschik 
> (m.fel...@pengutronix.de: use fwnode helpers)
> (m.fel...@pengutronix.de: mv fw parsing into own function)
> (m.fel...@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch 

This one as well:

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 005fc2bd0d05..902c3cabf44c 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -859,6 +859,7 @@ config VIDEO_MT9M032
 config VIDEO_MT9M111
tristate "mt9m111, mt9m112 and mt9m131 support"
depends on I2C && VIDEO_V4L2
+   select V4L2_FWNODE
help
  This driver supports MT9M111, MT9M112 and MT9M131 cameras from
  Micron/Aptina

-- 
Sakari Ailus
sakari.ai...@linux.intel.com


Re: [PATCH v5 0/9] Asynchronous UVC

2018-11-27 Thread Pavel Machek
On Tue 2018-11-06 21:27:11, Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> The Linux UVC driver has long provided adequate performance capabilities for
> web-cams and low data rate video devices in Linux while resolutions were low.
> 
> Modern USB cameras are now capable of high data rates thanks to USB3 with
> 1080p, and even 4k capture resolutions supported.
> 
> Cameras such as the Stereolabs ZED (bulk transfers) or the Logitech BRIO
> (isochronous transfers) can generate more data than an embedded ARM core is
> able to process on a single core, resulting in frame loss.
> 
> A large part of this performance impact is from the requirement to
> ‘memcpy’ frames out from URB packets to destination frames. This unfortunate
> requirement is due to the UVC protocol allowing a variable length header, and
> thus it is not possible to provide the target frame buffers directly.
> 
> Extra throughput is possible by moving the actual memcpy actions to a work
> queue, and moving the memcpy out of interrupt context thus allowing work tasks
> to be scheduled across multiple cores.

Hmm. Doing memcpy() on many cores is improvement but... not really.
Would it be possible to improve kernel<->user interface, so it says
"data is in this buffer, and it starts here" and so that memcpy in
kernel is not neccessary?
Pavel

-- 
(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 6/6] media: mt9m111: allow to setup pixclk polarity

2018-11-27 Thread Sakari Ailus
On Tue, Nov 27, 2018 at 03:12:29PM +0100, Philipp Zabel wrote:
> Hi Sakari,
> 
> On Tue, 2018-11-27 at 15:50 +0200, Sakari Ailus wrote:
> > Hi Philipp,
> > 
> > On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote:
> > > Hi Sakari,
> > > 
> > > On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> > > > > From: Enrico Scholz 
> > > > > 
> > > > > The chip can be configured to output data transitions on the
> > > > > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on 
> > > > > the
> > > > > falling edge.
> > > > > 
> > > > > Parsing the fw-node is made in a subfunction to bundle all (future)
> > > > > dt-parsing / fw-parsing stuff.
> > > > > 
> > > > > Signed-off-by: Enrico Scholz 
> > > > > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit 
> > > > > is set
> > > > > per default. Set bit to 0 (enable mask bit without value) to enable
> > > > > falling edge sampling.)
> > > > > Signed-off-by: Michael Grzeschik 
> > > > > (m.fel...@pengutronix.de: use fwnode helpers)
> > > > > (m.fel...@pengutronix.de: mv fw parsing into own function)
> > > > > (m.fel...@pengutronix.de: adapt commit msg)
> > > > > Signed-off-by: Marco Felsch 
> > > > 
> > > > Applied with the following diff:
> > > > 
> > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > index 2ef332b9b914..b6011bfddde8 100644
> > > > --- a/drivers/media/i2c/mt9m111.c
> > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct 
> > > > i2c_client *client)
> > > >  
> > > >  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 
> > > > *mt9m111)
> > > >  {
> > > > -   struct v4l2_fwnode_endpoint *bus_cfg;
> > > > +   struct v4l2_fwnode_endpoint bus_cfg = {
> > > > +   .bus_type = V4L2_MBUS_PARALLEL
> > > > +   };
> > > > struct fwnode_handle *np;
> > > > -   int ret = 0;
> > > > +   int ret;
> > > >  
> > > > np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), 
> > > > NULL);
> > > > if (!np)
> > > > return -EINVAL;
> > > >  
> > > > -   bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> > > > -   if (IS_ERR(bus_cfg)) {
> > > > -   ret = PTR_ERR(bus_cfg);
> > > > +   ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
> > > 
> > > Should that be
> > > 
> > > + ret = v4l2_fwnode_endpoint_parse(np, _cfg);
> > > 
> > > intead?
> > 
> > Could be. I'd expect the driver to need the link frequency at some point
> > after which you'd need the variable size properties anyway. But that's not
> > the case now.
> 
> I don't think the link-frequencies property will be used, this is just a
> parallel device. But Marco chose to use _alloc_parse because of what the
> v4l2_fwnode_endpoint_parse() documentation says:
> 
> /*
>  * NOTE: This function does not parse properties the size of which is variable
>  * without a low fixed limit. Please use v4l2_fwnode_endpoint_alloc_parse() 
> in
>  * new drivers instead.
>  */
> 
> So maybe we want to use v4l2_fwnode_endpoint_alloc_parse() always. There
> is no unnecessary allocation, just a lookup of the non-existing link-
> frequencies property.

There could be other properties in the future.

When I wrote that, I guess I ignored that the link frequency might not be
relevant for some devices such as CSI-2 receivers. I think it'd make sense
to remove the latter sentence; I can send a patch. The first sentence that
tells the limitations of the function is enough IMO.

-- 
Sakari Ailus
sakari.ai...@linux.intel.com


Re: [PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property

2018-11-27 Thread Sakari Ailus
On Tue, Nov 27, 2018 at 03:21:05PM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 18-11-27 15:13, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Tue, Nov 27, 2018 at 11:02:52AM +0100, Marco Felsch wrote:
> > > Add the pclk-sample property to the list of optional properties
> > > for the mt9m111 camera sensor.
> > > 
> > > Signed-off-by: Marco Felsch 
> > > Reviewed-by: Rob Herring 
> > > ---
> > >  Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt 
> > > b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > index a431fb45704b..d0bed6fa901a 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > @@ -14,6 +14,10 @@ sub-node for its digital output video port, in 
> > > accordance with the video
> > >  interface bindings defined in:
> > >  Documentation/devicetree/bindings/media/video-interfaces.txt
> > >  
> > > +Optional endpoint properties:
> > > +- pclk-sample: For information see ../video-interfaces.txt. The value is 
> > > set to
> > > +  0 if it isn't specified.
> > 
> > How about the data-active, hsync-active and vsync-active properties? Does
> > the hardware have a fixed configuration, or can this be set? It appears the
> > driver assumes active high for all.
> 
> As I understood it correctly the data-active, hsync-active and vsync-active
> I/O lines are always ACTIVE_HIGH.

Ack. Thanks!

> 
> Kind regards,
> Marco
>  
> > If there's something to change, this should be a separate patch IMO.
> > 
> > > +
> > >  Example:
> > >  
> > >   i2c_master {
> > > @@ -26,6 +30,7 @@ Example:
> > >   port {
> > >   mt9m111_1: endpoint {
> > >   remote-endpoint = <_camera>;
> > > + pclk-sample = <1>;
> > >   };
> > >   };
> > >   };
> > 
> > -- 
> > Sakari Ailus
> > sakari.ai...@linux.intel.com
> > 
> 
> -- 
> Pengutronix e.K.   | |
> Industrial Linux Solutions | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

-- 
Sakari Ailus
sakari.ai...@linux.intel.com


Re: [PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property

2018-11-27 Thread Marco Felsch
Hi Sakari,

On 18-11-27 15:13, Sakari Ailus wrote:
> Hi Marco,
> 
> On Tue, Nov 27, 2018 at 11:02:52AM +0100, Marco Felsch wrote:
> > Add the pclk-sample property to the list of optional properties
> > for the mt9m111 camera sensor.
> > 
> > Signed-off-by: Marco Felsch 
> > Reviewed-by: Rob Herring 
> > ---
> >  Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt 
> > b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > index a431fb45704b..d0bed6fa901a 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > @@ -14,6 +14,10 @@ sub-node for its digital output video port, in 
> > accordance with the video
> >  interface bindings defined in:
> >  Documentation/devicetree/bindings/media/video-interfaces.txt
> >  
> > +Optional endpoint properties:
> > +- pclk-sample: For information see ../video-interfaces.txt. The value is 
> > set to
> > +  0 if it isn't specified.
> 
> How about the data-active, hsync-active and vsync-active properties? Does
> the hardware have a fixed configuration, or can this be set? It appears the
> driver assumes active high for all.

As I understood it correctly the data-active, hsync-active and vsync-active
I/O lines are always ACTIVE_HIGH.

Kind regards,
Marco
 
> If there's something to change, this should be a separate patch IMO.
> 
> > +
> >  Example:
> >  
> > i2c_master {
> > @@ -26,6 +30,7 @@ Example:
> > port {
> > mt9m111_1: endpoint {
> > remote-endpoint = <_camera>;
> > +   pclk-sample = <1>;
> > };
> > };
> > };
> 
> -- 
> Sakari Ailus
> sakari.ai...@linux.intel.com
> 

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity

2018-11-27 Thread Philipp Zabel
Hi Sakari,

On Tue, 2018-11-27 at 15:50 +0200, Sakari Ailus wrote:
> Hi Philipp,
> 
> On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote:
> > Hi Sakari,
> > 
> > On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> > > > From: Enrico Scholz 
> > > > 
> > > > The chip can be configured to output data transitions on the
> > > > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> > > > falling edge.
> > > > 
> > > > Parsing the fw-node is made in a subfunction to bundle all (future)
> > > > dt-parsing / fw-parsing stuff.
> > > > 
> > > > Signed-off-by: Enrico Scholz 
> > > > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is 
> > > > set
> > > > per default. Set bit to 0 (enable mask bit without value) to enable
> > > > falling edge sampling.)
> > > > Signed-off-by: Michael Grzeschik 
> > > > (m.fel...@pengutronix.de: use fwnode helpers)
> > > > (m.fel...@pengutronix.de: mv fw parsing into own function)
> > > > (m.fel...@pengutronix.de: adapt commit msg)
> > > > Signed-off-by: Marco Felsch 
> > > 
> > > Applied with the following diff:
> > > 
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index 2ef332b9b914..b6011bfddde8 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client 
> > > *client)
> > >  
> > >  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 
> > > *mt9m111)
> > >  {
> > > - struct v4l2_fwnode_endpoint *bus_cfg;
> > > + struct v4l2_fwnode_endpoint bus_cfg = {
> > > + .bus_type = V4L2_MBUS_PARALLEL
> > > + };
> > >   struct fwnode_handle *np;
> > > - int ret = 0;
> > > + int ret;
> > >  
> > >   np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
> > >   if (!np)
> > >   return -EINVAL;
> > >  
> > > - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> > > - if (IS_ERR(bus_cfg)) {
> > > - ret = PTR_ERR(bus_cfg);
> > > + ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
> > 
> > Should that be
> > 
> > +   ret = v4l2_fwnode_endpoint_parse(np, _cfg);
> > 
> > intead?
> 
> Could be. I'd expect the driver to need the link frequency at some point
> after which you'd need the variable size properties anyway. But that's not
> the case now.

I don't think the link-frequencies property will be used, this is just a
parallel device. But Marco chose to use _alloc_parse because of what the
v4l2_fwnode_endpoint_parse() documentation says:

/*
 * NOTE: This function does not parse properties the size of which is variable
 * without a low fixed limit. Please use v4l2_fwnode_endpoint_alloc_parse() in  
  
 * new drivers instead.
 */

So maybe we want to use v4l2_fwnode_endpoint_alloc_parse() always. There
is no unnecessary allocation, just a lookup of the non-existing link-
frequencies property.

regards
Philipp


Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity

2018-11-27 Thread Sakari Ailus
Hi Philipp,

On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote:
> Hi Sakari,
> 
> On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> > > From: Enrico Scholz 
> > > 
> > > The chip can be configured to output data transitions on the
> > > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> > > falling edge.
> > > 
> > > Parsing the fw-node is made in a subfunction to bundle all (future)
> > > dt-parsing / fw-parsing stuff.
> > > 
> > > Signed-off-by: Enrico Scholz 
> > > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> > > per default. Set bit to 0 (enable mask bit without value) to enable
> > > falling edge sampling.)
> > > Signed-off-by: Michael Grzeschik 
> > > (m.fel...@pengutronix.de: use fwnode helpers)
> > > (m.fel...@pengutronix.de: mv fw parsing into own function)
> > > (m.fel...@pengutronix.de: adapt commit msg)
> > > Signed-off-by: Marco Felsch 
> > 
> > Applied with the following diff:
> > 
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 2ef332b9b914..b6011bfddde8 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client 
> > *client)
> >  
> >  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 
> > *mt9m111)
> >  {
> > -   struct v4l2_fwnode_endpoint *bus_cfg;
> > +   struct v4l2_fwnode_endpoint bus_cfg = {
> > +   .bus_type = V4L2_MBUS_PARALLEL
> > +   };
> > struct fwnode_handle *np;
> > -   int ret = 0;
> > +   int ret;
> >  
> > np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
> > if (!np)
> > return -EINVAL;
> >  
> > -   bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> > -   if (IS_ERR(bus_cfg)) {
> > -   ret = PTR_ERR(bus_cfg);
> > +   ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
> 
> Should that be
> 
> + ret = v4l2_fwnode_endpoint_parse(np, _cfg);
> 
> intead?

Could be. I'd expect the driver to need the link frequency at some point
after which you'd need the variable size properties anyway. But that's not
the case now.

With the change, the v4l2_fwnode_endpoint_free() becomes redundant. So the
diff on that diff:

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index b6011bfddde8..d639b9bcf64a 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1182,15 +1182,13 @@ static int mt9m111_probe_fw(struct i2c_client *client, 
struct mt9m111 *mt9m111)
if (!np)
return -EINVAL;
 
-   ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
+   ret = v4l2_fwnode_endpoint_parse(np, _cfg);
if (ret)
goto out_put_fw;
 
mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
  V4L2_MBUS_PCLK_SAMPLE_RISING);
 
-   v4l2_fwnode_endpoint_free(_cfg);
-
 out_put_fw:
fwnode_handle_put(np);
return ret;


> 
> > +   if (ret)
> > goto out_put_fw;
> > -   }
> >  
> > -   mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> > +   mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
> >   V4L2_MBUS_PCLK_SAMPLE_RISING);
> >  
> > -   v4l2_fwnode_endpoint_free(bus_cfg);
> > +   v4l2_fwnode_endpoint_free(_cfg);
> >  
> >  out_put_fw:
> > fwnode_handle_put(np);
> > 
> > Please base on current media tree master on the next time. Thanks.
> 
> regards
> Philipp

-- 
Sakari Ailus
sakari.ai...@linux.intel.com


Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity

2018-11-27 Thread Marco Felsch
Hi Sakari,

On 18-11-27 15:19, Sakari Ailus wrote:
> Hi Marco,
> 
> On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> > From: Enrico Scholz 
> > 
> > The chip can be configured to output data transitions on the
> > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> > falling edge.
> > 
> > Parsing the fw-node is made in a subfunction to bundle all (future)
> > dt-parsing / fw-parsing stuff.
> > 
> > Signed-off-by: Enrico Scholz 
> > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> > per default. Set bit to 0 (enable mask bit without value) to enable
> > falling edge sampling.)
> > Signed-off-by: Michael Grzeschik 
> > (m.fel...@pengutronix.de: use fwnode helpers)
> > (m.fel...@pengutronix.de: mv fw parsing into own function)
> > (m.fel...@pengutronix.de: adapt commit msg)
> > Signed-off-by: Marco Felsch 
> 
> Applied with the following diff:
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 2ef332b9b914..b6011bfddde8 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client 
> *client)
>  
>  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 
> *mt9m111)
>  {
> - struct v4l2_fwnode_endpoint *bus_cfg;
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_PARALLEL
> + };
>   struct fwnode_handle *np;
> - int ret = 0;
> + int ret;
>  
>   np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
>   if (!np)
>   return -EINVAL;
>  
> - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> - if (IS_ERR(bus_cfg)) {
> - ret = PTR_ERR(bus_cfg);
> + ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
> + if (ret)
>   goto out_put_fw;
> - }
>  
> - mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> + mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
> V4L2_MBUS_PCLK_SAMPLE_RISING);
>  
> - v4l2_fwnode_endpoint_free(bus_cfg);
> + v4l2_fwnode_endpoint_free(_cfg);
>  
>  out_put_fw:
>   fwnode_handle_put(np);
> 
> Please base on current media tree master on the next time. Thanks.

Sorry, thanks for the inline fix :)

Kind regards,
Marco

> -- 
> Kind regards,
> 
> Sakari Ailus
> sakari.ai...@linux.intel.com


Re: [PATCH v4 2/2] dt-bindings: Document the Rockchip RK1608 bindings

2018-11-27 Thread Sakari Ailus
Hi Leo,

I found this, and thought of replying. It's an old patch. Feel free to ping
if you don't get replies.

On Thu, Mar 08, 2018 at 02:38:42PM +0800, Wen Nuan wrote:
> From: Leo Wen 
> 
> Add DT bindings documentation for Rockchip RK1608.
> 
> Changes V4:
> - Revise the comment of node.
> - Revise the comment of 'endpoint@1'.
> 
> Signed-off-by: Leo Wen 
> ---
>  Documentation/devicetree/bindings/media/rk1608.txt | 95 
> ++
>  MAINTAINERS|  1 +
>  2 files changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/rk1608.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/rk1608.txt 
> b/Documentation/devicetree/bindings/media/rk1608.txt
> new file mode 100644
> index 000..ea23d2d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/rk1608.txt
> @@ -0,0 +1,95 @@
> +Rockchip RK1608 as a PreISP to link on Soc
> +--
> +
> +Required properties:
> +
> +- compatible : "rockchip,rk1608";
> +- reg: SPI slave address of the rk1608;
> +- clocks : Must contain an entry for each entry in clock-names;
> +- clock-names: Must contain "mclk" for the device's master 
> clock;
> +- reset-gpios: GPIO connected to reset pin;
> +- irq-gpios  : GPIO connected to irq pin;
> +- sleepst-gpios  : GPIO connected to sleepst pin;

There are quite a few sleep-gpios in the current bindings. How about
aligning the naming?

> +- wakeup-gpios   : GPIO connected to wakeup pin;
> +- powerdown-gpios: GPIO connected to powerdown pin;
> +- pinctrl-names  : Should contain only one value - "default";
> +- pinctrl-0  : Pin control group to be used for this controller;
> +
> +Optional properties:
> +
> +- spi-max-frequency  : Maximum SPI clocking speed of the device;
> +
> +The device node should contain one or two port child nodes with child
> +'endpoint' node. There are two ports then port@0 must
> +describe the output and port@1 input channels. Please refer to the
> +bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.

I think that to be meaningful, you actually need two port nodes. Must ->
shall.

> +
> +endpoint node
> +-
> +
> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
> +video-interfaces.txt. If present it should be <1> - the device
> +supports only one data lane without re-mapping.

You still have four lanes in the example below.

> +
> +Note1: Since no data is generated in RK1608,so this is meaningful that you 
> need
> +a extra sensor (such as a camera) mounted on RK1608. You need to use 
> endpoint@x
> +to match these sensors.
> +
> +Note2:You must set the current value of the spi pins to be 8mA, if they are 
> not.
> +
> +Example:
> + {
> + status = "okay";
> + spi_rk1608@00 {
> + compatible =  "rockchip,rk1608";
> + status = "okay";
> + reg = <0>;
> + spi-max-frequency = <2400>;
> + link-freqs = /bits/ 64 ;
> + clocks = < SCLK_SPI0>, < SCLK_VIP_OUT>,
> + < DCLK_VOP0>, < ACLK_VIP>, < HCLK_VIP>,
> + < PCLK_ISP_IN>, < PCLK_ISP_IN>,
> + < PCLK_ISP_IN>, < SCLK_MIPIDSI_24M>,
> + < PCLK_MIPI_CSI>;
> + clock-names = "mclk", "mipi_clk",  "pd_cif", "aclk_cif",
> + "hclk_cif", "cif0_in", "g_pclkin_cif",
> + "cif0_out", "clk_mipi_24m", "hclk_mipiphy";
> + reset-gpios = < 0 GPIO_ACTIVE_HIGH>;
> + irq-gpios = < 2 GPIO_ACTIVE_HIGH>;
> + sleepst-gpios = < 1 GPIO_ACTIVE_HIGH>;
> + wakeup-gpios = < 4 GPIO_ACTIVE_HIGH>;
> + powerdown-gpios = < 0 GPIO_ACTIVE_HIGH>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_irq_gpios _wake_gpios
> +  _sleep_gpios>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {

reg missing here and the port below.

> + mipi_dphy_out0: endpoint@0 {

No need for "@0" as you only have a single endpoint.

> + remote-endpoint = <_dphy_in0>;
> + clock-lanes = <0>;
> + data-lanes = <1 2 3 4>;
> + clock-noncontinuous;
> + link-frequencies =
> + /bits/ 64 ;
> + };
> + };
> +
> + port@1 {
> + sensor_in0: endpoint@0 {

reg missing here and below.

> + remote-endpoint = <_out0>;
> + 

Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity

2018-11-27 Thread Philipp Zabel
Hi Sakari,

On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> Hi Marco,
> 
> On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> > From: Enrico Scholz 
> > 
> > The chip can be configured to output data transitions on the
> > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> > falling edge.
> > 
> > Parsing the fw-node is made in a subfunction to bundle all (future)
> > dt-parsing / fw-parsing stuff.
> > 
> > Signed-off-by: Enrico Scholz 
> > (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> > per default. Set bit to 0 (enable mask bit without value) to enable
> > falling edge sampling.)
> > Signed-off-by: Michael Grzeschik 
> > (m.fel...@pengutronix.de: use fwnode helpers)
> > (m.fel...@pengutronix.de: mv fw parsing into own function)
> > (m.fel...@pengutronix.de: adapt commit msg)
> > Signed-off-by: Marco Felsch 
> 
> Applied with the following diff:
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 2ef332b9b914..b6011bfddde8 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client 
> *client)
>  
>  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 
> *mt9m111)
>  {
> - struct v4l2_fwnode_endpoint *bus_cfg;
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_PARALLEL
> + };
>   struct fwnode_handle *np;
> - int ret = 0;
> + int ret;
>  
>   np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
>   if (!np)
>   return -EINVAL;
>  
> - bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> - if (IS_ERR(bus_cfg)) {
> - ret = PTR_ERR(bus_cfg);
> + ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);

Should that be

+   ret = v4l2_fwnode_endpoint_parse(np, _cfg);

intead?

> + if (ret)
>   goto out_put_fw;
> - }
>  
> - mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> + mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
> V4L2_MBUS_PCLK_SAMPLE_RISING);
>  
> - v4l2_fwnode_endpoint_free(bus_cfg);
> + v4l2_fwnode_endpoint_free(_cfg);
>  
>  out_put_fw:
>   fwnode_handle_put(np);
> 
> Please base on current media tree master on the next time. Thanks.

regards
Philipp


Re: [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields

2018-11-27 Thread Ezequiel Garcia
On Tue, 2018-11-27 at 16:59 +0900, Tomasz Figa wrote:
> On Tue, Nov 27, 2018 at 8:29 AM Ezequiel Garcia  
> wrote:
> > On Mon, 2018-11-26 at 13:14 +0900, Tomasz Figa wrote:
> > > Hi Ezequiel,
> > > 
> > > On Sat, Nov 24, 2018 at 2:20 AM Ezequiel Garcia  
> > > wrote:
> > > > Make the core set the reserved fields to zero in
> > > > v4l2_pix_format_mplane and v4l2_plane_pix_format structs,
> > > > for _MPLANE queue types.
> > > > 
> > > > Moving this to the core avoids having to do so in each
> > > > and every driver.
> > > > 
> > > > Suggested-by: Tomasz Figa 
> > > > Signed-off-by: Ezequiel Garcia 
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-ioctl.c | 51 
> > > >  1 file changed, 45 insertions(+), 6 deletions(-)
> > > > 
> > > 
> > > Thanks for the patch. Please see my comments inline.
> > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
> > > > b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > index 10b862dcbd86..3858fffc3e68 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > > @@ -1420,6 +1420,7 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops 
> > > > *ops,
> > > >  {
> > > > struct v4l2_format *p = arg;
> > > > int ret = check_fmt(file, p->type);
> > > > +   int i;
> > > > 
> > > > if (ret)
> > > > return ret;
> > > > @@ -1458,7 +1459,13 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops 
> > > > *ops,
> > > > p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > > > return ret;
> > > > case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > > > -   return ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg);
> > > > +   ret = ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg);
> > > > +   memset(p->fmt.pix_mp.reserved, 0,
> > > > +  sizeof(p->fmt.pix_mp.reserved));
> > > > +   for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> > > > +   memset(p->fmt.pix_mp.plane_fmt[i].reserved, 0,
> > > > +  
> > > > sizeof(p->fmt.pix_mp.plane_fmt[i].reserved));
> > > > +   return ret;
> > > > case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> > > > return ops->vidioc_g_fmt_vid_overlay(file, fh, arg);
> > > > case V4L2_BUF_TYPE_VBI_CAPTURE:
> > > > @@ -1474,7 +1481,13 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops 
> > > > *ops,
> > > > p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> > > > return ret;
> > > > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > > > -   return ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg);
> > > > +   ret = ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg);
> > > > +   memset(p->fmt.pix_mp.reserved, 0,
> > > > +  sizeof(p->fmt.pix_mp.reserved));
> > > > +   for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> > > > +   memset(p->fmt.pix_mp.plane_fmt[i].reserved, 0,
> > > > +  
> > > > sizeof(p->fmt.pix_mp.plane_fmt[i].reserved));
> > > > +   return ret;
> > > 
> > > I wonder if we need this for G_FMT. The driver can just memset() the
> > > whole struct itself and then just initialize the fields it cares
> > > about, but actually in many cases the driver will just include an
> > > instance of the pix_fmt(_mp) struct in its internal state (which has
> > > the reserved fields already zeroed) and just copy it to the target
> > > struct in the callback.
> > > 
> > 
> > Perhaps in many cases, but from code inspection it seems not
> > all of them (randomly opened vicodec & mtk-jpeg and both need
> > a memset!).
> > 
> > I'm thinkig it'd best to keep it this way for consistency
> > and to avoid having the worry at all about this in the drivers.
> 
> I guess it makes sense indeed. The structure isn't terribly big, so
> there shouldn't be any significant performance penalty I suppose.

Actually, after some experiments, I realized the entire v4l2_format struct
is already being memset to 0 in v4l_g_fmt.

Thanks,
Eze



Re: [PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity

2018-11-27 Thread Sakari Ailus
Hi Marco,

On Tue, Nov 27, 2018 at 11:02:53AM +0100, Marco Felsch wrote:
> From: Enrico Scholz 
> 
> The chip can be configured to output data transitions on the
> rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> falling edge.
> 
> Parsing the fw-node is made in a subfunction to bundle all (future)
> dt-parsing / fw-parsing stuff.
> 
> Signed-off-by: Enrico Scholz 
> (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> per default. Set bit to 0 (enable mask bit without value) to enable
> falling edge sampling.)
> Signed-off-by: Michael Grzeschik 
> (m.fel...@pengutronix.de: use fwnode helpers)
> (m.fel...@pengutronix.de: mv fw parsing into own function)
> (m.fel...@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch 

Applied with the following diff:

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 2ef332b9b914..b6011bfddde8 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client 
*client)
 
 static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
 {
-   struct v4l2_fwnode_endpoint *bus_cfg;
+   struct v4l2_fwnode_endpoint bus_cfg = {
+   .bus_type = V4L2_MBUS_PARALLEL
+   };
struct fwnode_handle *np;
-   int ret = 0;
+   int ret;
 
np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
if (!np)
return -EINVAL;
 
-   bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
-   if (IS_ERR(bus_cfg)) {
-   ret = PTR_ERR(bus_cfg);
+   ret = v4l2_fwnode_endpoint_alloc_parse(np, _cfg);
+   if (ret)
goto out_put_fw;
-   }
 
-   mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
+   mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
  V4L2_MBUS_PCLK_SAMPLE_RISING);
 
-   v4l2_fwnode_endpoint_free(bus_cfg);
+   v4l2_fwnode_endpoint_free(_cfg);
 
 out_put_fw:
fwnode_handle_put(np);

Please base on current media tree master on the next time. Thanks.

-- 
Kind regards,

Sakari Ailus
sakari.ai...@linux.intel.com


[PATCH] dvb-sat: rename Astra 1E to Astra 19.2 E and move it to beginning

2018-11-27 Thread Mauro Carvalho Chehab
The "European Universal" LNBf was now replaced by the model with
also supports the Astra satellites in almost all EU. We're keeping
seeing people reporting problems on Kaffeine and other digital TV
software due to that.

So, in order to make easier for new people that just want to make
their Satellite-based TV to work in Europe, let's move the Astra
entry to be the first one and giving it a better name, as the
Astra 1E satellite was retired a long time ago, and, since 2008,
the satellites that replaced it are known as "Astra 19.2 E",
in order to reflect their orbital position.

Signed-off-by: Mauro Carvalho Chehab 
---
 lib/libdvbv5/dvb-sat.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/lib/libdvbv5/dvb-sat.c b/lib/libdvbv5/dvb-sat.c
index 8c04f66f973b..18e2359c053b 100644
--- a/lib/libdvbv5/dvb-sat.c
+++ b/lib/libdvbv5/dvb-sat.c
@@ -53,7 +53,24 @@ struct dvb_sat_lnb_priv {
 static const struct dvb_sat_lnb_priv lnb_array[] = {
{
.desc = {
-   .name = N_("Universal, Europe"),
+   .name = N_("Astra 19.2E, European Universal Ku 
(extended)"),
+   .alias = "EXTENDED",
+   // Legacy fields - kept just to avoid API/ABI breakages
+   .lowfreq = 9750,
+   .highfreq = 10600,
+   .rangeswitch = 11700,
+   .freqrange = {
+   { 10700, 11700 },
+   { 11700, 12750 },
+   },
+   },
+   .freqrange = {
+   { 10700, 11700, 9750, 11700},
+   { 11700, 12750, 10600, 0 },
+   }
+   }, {
+   .desc = {
+   .name = N_("Old European Universal. Nowadays mostly 
replaced by Astra 19.2E"),
.alias = "UNIVERSAL",
// Legacy fields - kept just to avoid API/ABI breakages
.lowfreq = 9750,
@@ -81,23 +98,6 @@ static const struct dvb_sat_lnb_priv lnb_array[] = {
.freqrange = {
{ 12200, 12700, 11250 }
}
-   }, {
-   .desc = {
-   .name = N_("Astra 1E, European Universal Ku 
(extended)"),
-   .alias = "EXTENDED",
-   // Legacy fields - kept just to avoid API/ABI breakages
-   .lowfreq = 9750,
-   .highfreq = 10600,
-   .rangeswitch = 11700,
-   .freqrange = {
-   { 10700, 11700 },
-   { 11700, 12750 },
-   },
-   },
-   .freqrange = {
-   { 10700, 11700, 9750, 11700},
-   { 11700, 12750, 10600, 0 },
-   }
}, {
.desc = {
.name = N_("Standard"),
-- 
2.19.1




Re: [PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property

2018-11-27 Thread Sakari Ailus
Hi Marco,

On Tue, Nov 27, 2018 at 11:02:52AM +0100, Marco Felsch wrote:
> Add the pclk-sample property to the list of optional properties
> for the mt9m111 camera sensor.
> 
> Signed-off-by: Marco Felsch 
> Reviewed-by: Rob Herring 
> ---
>  Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt 
> b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> index a431fb45704b..d0bed6fa901a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> @@ -14,6 +14,10 @@ sub-node for its digital output video port, in accordance 
> with the video
>  interface bindings defined in:
>  Documentation/devicetree/bindings/media/video-interfaces.txt
>  
> +Optional endpoint properties:
> +- pclk-sample: For information see ../video-interfaces.txt. The value is set 
> to
> +  0 if it isn't specified.

How about the data-active, hsync-active and vsync-active properties? Does
the hardware have a fixed configuration, or can this be set? It appears the
driver assumes active high for all.

If there's something to change, this should be a separate patch IMO.

> +
>  Example:
>  
>   i2c_master {
> @@ -26,6 +30,7 @@ Example:
>   port {
>   mt9m111_1: endpoint {
>   remote-endpoint = <_camera>;
> + pclk-sample = <1>;
>   };
>   };
>   };

-- 
Sakari Ailus
sakari.ai...@linux.intel.com


Re: [RESEND PATCH 1/1] media: Use common test pattern menu entries

2018-11-27 Thread Sakari Ailus
On Tue, Nov 27, 2018 at 01:11:42PM +0100, Luca Ceresoli wrote:
> Hi Sakari, Bingbu,
> 
> On 27/11/18 10:34, Sakari Ailus wrote:
> > While the test pattern menu itself is not standardised, many devices
> > support the same test patterns. Aligning the menu entries helps the user
> > space to use the interface, and adding macros for the menu entry strings
> > helps to keep them aligned.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> > Fixed Andy's email.
> > 
> >  drivers/media/i2c/imx258.c | 10 +-
> >  drivers/media/i2c/imx319.c | 10 +-
> >  drivers/media/i2c/imx355.c | 10 +-
> >  drivers/media/i2c/ov2640.c |  4 ++--
> >  drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
> >  include/uapi/linux/v4l2-controls.h |  5 +
> >  6 files changed, 27 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index f86ae18bc104..c795d4c4c0e4 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = 
> > {
> >  };
> >  
> >  static const char * const imx258_test_pattern_menu[] = {
> > -   "Disabled",
> > -   "Solid Colour",
> > -   "Eight Vertical Colour Bars",
> > -   "Colour Bars With Fade to Grey",
> > -   "Pseudorandom Sequence (PN9)",
> > +   V4L2_TEST_PATTERN_DISABLED,
> > +   V4L2_TEST_PATTERN_SOLID_COLOUR,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> > +   V4L2_TEST_PATTERN_PN9,
> >  };
> >  
> >  /* Configurations for supported link frequencies */
> > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> > index 17c2e4b41221..eddaf69a67b6 100644
> > --- a/drivers/media/i2c/imx319.c
> > +++ b/drivers/media/i2c/imx319.c
> > @@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] 
> > = {
> >  };
> >  
> >  static const char * const imx319_test_pattern_menu[] = {
> > -   "Disabled",
> > -   "Solid Colour",
> > -   "Eight Vertical Colour Bars",
> > -   "Colour Bars With Fade to Grey",
> > -   "Pseudorandom Sequence (PN9)",
> > +   V4L2_TEST_PATTERN_DISABLED,
> > +   V4L2_TEST_PATTERN_SOLID_COLOUR,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> > +   V4L2_TEST_PATTERN_PN9,
> >  };
> >  
> >  /* supported link frequencies */
> > diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> > index bed293b60e50..824d07156f9c 100644
> > --- a/drivers/media/i2c/imx355.c
> > +++ b/drivers/media/i2c/imx355.c
> > @@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
> >  };
> >  
> >  static const char * const imx355_test_pattern_menu[] = {
> > -   "Disabled",
> > -   "Solid Colour",
> > -   "Eight Vertical Colour Bars",
> > -   "Colour Bars With Fade to Grey",
> > -   "Pseudorandom Sequence (PN9)",
> > +   V4L2_TEST_PATTERN_DISABLED,
> > +   V4L2_TEST_PATTERN_SOLID_COLOUR,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> > +   V4L2_TEST_PATTERN_PN9,
> >  };
> >  
> >  /* supported link frequencies */
> > diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> > index 5d2d6735cc78..507ec7176a7d 100644
> > --- a/drivers/media/i2c/ov2640.c
> > +++ b/drivers/media/i2c/ov2640.c
> > @@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
> >  }
> >  
> >  static const char * const ov2640_test_pattern_menu[] = {
> > -   "Disabled",
> > -   "Eight Vertical Colour Bars",
> > +   V4L2_TEST_PATTERN_DISABLED,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> >  };
> >  
> >  /*
> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> > b/drivers/media/i2c/smiapp/smiapp-core.c
> > index 58a45c353e27..f6a92b9f178c 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct 
> > smiapp_sensor *sensor)
> >  }
> >  
> >  static const char * const smiapp_test_patterns[] = {
> > -   "Disabled",
> > -   "Solid Colour",
> > -   "Eight Vertical Colour Bars",
> > -   "Colour Bars With Fade to Grey",
> > -   "Pseudorandom Sequence (PN9)",
> > +   V4L2_TEST_PATTERN_DISABLED,
> > +   V4L2_TEST_PATTERN_SOLID_COLOUR,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> > +   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> > +   V4L2_TEST_PATTERN_PN9,
> >  };
> >  
> >  static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
> > diff --git a/include/uapi/linux/v4l2-controls.h 
> > b/include/uapi/linux/v4l2-controls.h
> > index 998983a6e6b7..a74ff6f1ac88 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
> >  #define V4L2_CID_LINK_FREQ (V4L2_CID_IMAGE_PROC_CLASS_BASE 
> > + 1)
> >  #define 

Re: [RESEND PATCH 1/1] media: Use common test pattern menu entries

2018-11-27 Thread Luca Ceresoli
Hi Sakari, Bingbu,

On 27/11/18 10:34, Sakari Ailus wrote:
> While the test pattern menu itself is not standardised, many devices
> support the same test patterns. Aligning the menu entries helps the user
> space to use the interface, and adding macros for the menu entry strings
> helps to keep them aligned.
> 
> Signed-off-by: Sakari Ailus 
> ---
> Fixed Andy's email.
> 
>  drivers/media/i2c/imx258.c | 10 +-
>  drivers/media/i2c/imx319.c | 10 +-
>  drivers/media/i2c/imx355.c | 10 +-
>  drivers/media/i2c/ov2640.c |  4 ++--
>  drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
>  include/uapi/linux/v4l2-controls.h |  5 +
>  6 files changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index f86ae18bc104..c795d4c4c0e4 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = {
>  };
>  
>  static const char * const imx258_test_pattern_menu[] = {
> - "Disabled",
> - "Solid Colour",
> - "Eight Vertical Colour Bars",
> - "Colour Bars With Fade to Grey",
> - "Pseudorandom Sequence (PN9)",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_SOLID_COLOUR,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> + V4L2_TEST_PATTERN_PN9,
>  };
>  
>  /* Configurations for supported link frequencies */
> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> index 17c2e4b41221..eddaf69a67b6 100644
> --- a/drivers/media/i2c/imx319.c
> +++ b/drivers/media/i2c/imx319.c
> @@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] = 
> {
>  };
>  
>  static const char * const imx319_test_pattern_menu[] = {
> - "Disabled",
> - "Solid Colour",
> - "Eight Vertical Colour Bars",
> - "Colour Bars With Fade to Grey",
> - "Pseudorandom Sequence (PN9)",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_SOLID_COLOUR,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> + V4L2_TEST_PATTERN_PN9,
>  };
>  
>  /* supported link frequencies */
> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index bed293b60e50..824d07156f9c 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
>  };
>  
>  static const char * const imx355_test_pattern_menu[] = {
> - "Disabled",
> - "Solid Colour",
> - "Eight Vertical Colour Bars",
> - "Colour Bars With Fade to Grey",
> - "Pseudorandom Sequence (PN9)",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_SOLID_COLOUR,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> + V4L2_TEST_PATTERN_PN9,
>  };
>  
>  /* supported link frequencies */
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 5d2d6735cc78..507ec7176a7d 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
>  }
>  
>  static const char * const ov2640_test_pattern_menu[] = {
> - "Disabled",
> - "Eight Vertical Colour Bars",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
>  };
>  
>  /*
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index 58a45c353e27..f6a92b9f178c 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct 
> smiapp_sensor *sensor)
>  }
>  
>  static const char * const smiapp_test_patterns[] = {
> - "Disabled",
> - "Solid Colour",
> - "Eight Vertical Colour Bars",
> - "Colour Bars With Fade to Grey",
> - "Pseudorandom Sequence (PN9)",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_SOLID_COLOUR,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> + V4L2_TEST_PATTERN_PN9,
>  };
>  
>  static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
> diff --git a/include/uapi/linux/v4l2-controls.h 
> b/include/uapi/linux/v4l2-controls.h
> index 998983a6e6b7..a74ff6f1ac88 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_LINK_FREQ   (V4L2_CID_IMAGE_PROC_CLASS_BASE 
> + 1)
>  #define V4L2_CID_PIXEL_RATE  (V4L2_CID_IMAGE_PROC_CLASS_BASE 
> + 2)
>  #define V4L2_CID_TEST_PATTERN
> (V4L2_CID_IMAGE_PROC_CLASS_BASE + 3)
> +#define V4L2_TEST_PATTERN_DISABLED   "Disabled"
> +#define 

[PATCH for v4.20] vb2: skip request checks for VIDIOC_PREPARE_BUF

2018-11-27 Thread Hans Verkuil
VIDIOC_PREPARE_BUF should ignore V4L2_BUF_FLAG_REQUEST_FD since it isn't
doing anything with requests. So inform vb2_queue_or_prepare_buf whether
it is called from vb2_prepare_buf or vb2_qbuf and just return 0 in the
first case.

This was found when adding new v4l2-compliance checks.

Signed-off-by: Hans Verkuil 
---
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c 
b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 1244c246d0c4..1ac1b3f334f6 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -332,10 +332,10 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer 
*vb, struct v4l2_buffer *b
 }

 static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device 
*mdev,
-   struct v4l2_buffer *b,
-   const char *opname,
+   struct v4l2_buffer *b, bool is_prepare,
struct media_request **p_req)
 {
+   const char *opname = is_prepare ? "prepare_buf" : "qbuf";
struct media_request *req;
struct vb2_v4l2_buffer *vbuf;
struct vb2_buffer *vb;
@@ -377,6 +377,9 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, 
struct media_device *md
return ret;
}

+   if (is_prepare)
+   return 0;
+
if (!(b->flags & V4L2_BUF_FLAG_REQUEST_FD)) {
if (q->uses_requests) {
dprintk(1, "%s: queue uses requests\n", opname);
@@ -656,7 +659,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct 
media_device *mdev,
if (b->flags & V4L2_BUF_FLAG_REQUEST_FD)
return -EINVAL;

-   ret = vb2_queue_or_prepare_buf(q, mdev, b, "prepare_buf", NULL);
+   ret = vb2_queue_or_prepare_buf(q, mdev, b, true, NULL);

return ret ? ret : vb2_core_prepare_buf(q, b->index, b);
 }
@@ -728,7 +731,7 @@ int vb2_qbuf(struct vb2_queue *q, struct media_device *mdev,
return -EBUSY;
}

-   ret = vb2_queue_or_prepare_buf(q, mdev, b, "qbuf", );
+   ret = vb2_queue_or_prepare_buf(q, mdev, b, false, );
if (ret)
return ret;
ret = vb2_core_qbuf(q, b->index, b, req);


Re: [PATCH 1/1] media: Use common test pattern menu entries

2018-11-27 Thread Bingbu Cao



On 11/27/2018 05:33 PM, Sakari Ailus wrote:

While the test pattern menu itself is not standardised, many devices
support the same test patterns. Aligning the menu entries helps the user
space to use the interface, and adding macros for the menu entry strings
helps to keep them aligned.

I like this patch.


Signed-off-by: Sakari Ailus 
---
  drivers/media/i2c/imx258.c | 10 +-
  drivers/media/i2c/imx319.c | 10 +-
  drivers/media/i2c/imx355.c | 10 +-
  drivers/media/i2c/ov2640.c |  4 ++--
  drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
  include/uapi/linux/v4l2-controls.h |  5 +
  6 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index f86ae18bc104..c795d4c4c0e4 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = {
  };
  
  static const char * const imx258_test_pattern_menu[] = {

-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
  };
  
  /* Configurations for supported link frequencies */

diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221..eddaf69a67b6 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] = {
  };
  
  static const char * const imx319_test_pattern_menu[] = {

-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
  };
  
  /* supported link frequencies */

diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index bed293b60e50..824d07156f9c 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
  };
  
  static const char * const imx355_test_pattern_menu[] = {

-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
  };
  
  /* supported link frequencies */

diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 5d2d6735cc78..507ec7176a7d 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
  }
  
  static const char * const ov2640_test_pattern_menu[] = {

-   "Disabled",
-   "Eight Vertical Colour Bars",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
  };
  
  /*

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
b/drivers/media/i2c/smiapp/smiapp-core.c
index 58a45c353e27..f6a92b9f178c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct 
smiapp_sensor *sensor)
  }
  
  static const char * const smiapp_test_patterns[] = {

-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
  };
  
  static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)

diff --git a/include/uapi/linux/v4l2-controls.h 
b/include/uapi/linux/v4l2-controls.h
index 998983a6e6b7..a74ff6f1ac88 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
  #define V4L2_CID_LINK_FREQ(V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 1)
  #define V4L2_CID_PIXEL_RATE   (V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 2)
  #define V4L2_CID_TEST_PATTERN (V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 3)
+#define V4L2_TEST_PATTERN_DISABLED "Disabled"
+#define V4L2_TEST_PATTERN_SOLID_COLOUR "Solid Colour"
+#define V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS   "Eight Vertical Colour 
Bars"
+#define 

Re: [PATCH v2] media: unify some sony camera sensors pattern naming

2018-11-27 Thread Bingbu Cao




On 11/27/2018 05:33 PM, Sakari Ailus wrote:

On Tue, Nov 27, 2018 at 05:03:26PM +0800, bingbu@intel.com wrote:

From: Bingbu Cao 

Some Sony camera sensors have same pattern
definitions, this patch unify the pattern naming
to make it more clear to the userspace.

Suggested-by: Sakari Ailus 
Signed-off-by: Bingbu Cao 
Reviewed-by: l...@lucaceresoli.net

Hi Bing Bu,

Sorry, I prefer your v1. :-)

Thanks.
It really prevent me Google more about difference between 'colour' and 'color'. 
 :)






Re: [PATCH dvb v1 4/4] media: dvb_frontend: remove __func__ from dev_dbg()

2018-11-27 Thread Sean Young
On Tue, Oct 30, 2018 at 05:14:51PM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> As dynamic debug can be instructed to add the function name to the
> debug output using +f switch, we can remove __func__ from all
> dev_dbg() calls. If not, a user that sets +f in dynamic debug would
> get duplicated function name.
> 
> Signed-off-by: Victor Toso 
> ---
>  drivers/media/dvb-core/dvb_frontend.c | 142 +-
>  1 file changed, 69 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c 
> b/drivers/media/dvb-core/dvb_frontend.c
> index 961207cf09eb..ab6d778aa641 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -251,7 +251,7 @@ static void dvb_frontend_add_event(struct dvb_frontend 
> *fe,
>   struct dvb_frontend_event *e;
>   int wp;
>  
> - dev_dbg(fe->dvb->device, "%s:\n", __func__);
> + dev_dbg(fe->dvb->device, "\n");

Again same as 3/4. Either make the debug useful or delete it.


Sean

>  
>   if ((status & FE_HAS_LOCK) && has_get_frontend(fe))
>   dtv_get_frontend(fe, c, >parameters_out);
> @@ -293,7 +293,7 @@ static int dvb_frontend_get_event(struct dvb_frontend *fe,
>   struct dvb_frontend_private *fepriv = fe->frontend_priv;
>   struct dvb_fe_events *events = >events;
>  
> - dev_dbg(fe->dvb->device, "%s:\n", __func__);
> + dev_dbg(fe->dvb->device, "\n");
>  
>   if (events->overflow) {
>   events->overflow = 0;
> @@ -334,8 +334,8 @@ static void dvb_frontend_clear_events(struct dvb_frontend 
> *fe)
>  static void dvb_frontend_init(struct dvb_frontend *fe)
>  {
>   dev_dbg(fe->dvb->device,
> - "%s: initialising adapter %i frontend %i (%s)...\n",
> - __func__, fe->dvb->num, fe->id, fe->ops.info.name);
> + "initialising adapter %i frontend %i (%s)...\n",
> + fe->dvb->num, fe->id, fe->ops.info.name);
>  
>   if (fe->ops.init)
>   fe->ops.init(fe);
> @@ -362,7 +362,7 @@ static void dvb_frontend_swzigzag_update_delay(struct 
> dvb_frontend_private *fepr
>   int q2;
>   struct dvb_frontend *fe = fepriv->dvbdev->priv;
>  
> - dev_dbg(fe->dvb->device, "%s:\n", __func__);
> + dev_dbg(fe->dvb->device, "\n");
>  
>   if (locked)
>   (fepriv->quality) = (fepriv->quality * 220 + 36 * 256) / 256;
> @@ -458,8 +458,8 @@ static int dvb_frontend_swzigzag_autotune(struct 
> dvb_frontend *fe, int check_wra
>   }
>  
>   dev_dbg(fe->dvb->device,
> - "%s: drift:%i inversion:%i auto_step:%i auto_sub_step:%i 
> started_auto_step:%i\n",
> - __func__, fepriv->lnb_drift, fepriv->inversion,
> + "drift:%i inversion:%i auto_step:%i auto_sub_step:%i 
> started_auto_step:%i\n",
> + fepriv->lnb_drift, fepriv->inversion,
>   fepriv->auto_step, fepriv->auto_sub_step,
>   fepriv->started_auto_step);
>  
> @@ -661,7 +661,7 @@ static int dvb_frontend_thread(void *data)
>   bool re_tune = false;
>   bool semheld = false;
>  
> - dev_dbg(fe->dvb->device, "%s:\n", __func__);
> + dev_dbg(fe->dvb->device, "\n");
>  
>   fepriv->check_wrapped = 0;
>   fepriv->quality = 0;
> @@ -710,10 +710,10 @@ static int dvb_frontend_thread(void *data)
>   algo = fe->ops.get_frontend_algo(fe);
>   switch (algo) {
>   case DVBFE_ALGO_HW:
> - dev_dbg(fe->dvb->device, "%s: Frontend ALGO = 
> DVBFE_ALGO_HW\n", __func__);
> + dev_dbg(fe->dvb->device, "Frontend ALGO = 
> DVBFE_ALGO_HW\n");
>  
>   if (fepriv->state & FESTATE_RETUNE) {
> - dev_dbg(fe->dvb->device, "%s: Retune 
> requested, FESTATE_RETUNE\n", __func__);
> + dev_dbg(fe->dvb->device, "Retune 
> requested, FESTATE_RETUNE\n");
>   re_tune = true;
>   fepriv->state = FESTATE_TUNED;
>   } else {
> @@ -724,19 +724,21 @@ static int dvb_frontend_thread(void *data)
>   fe->ops.tune(fe, re_tune, 
> fepriv->tune_mode_flags, >delay, );
>  
>   if (s != fepriv->status && 
> !(fepriv->tune_mode_flags & FE_TUNE_MODE_ONESHOT)) {
> - dev_dbg(fe->dvb->device, "%s: state 
> changed, adding current state\n", __func__);
> + dev_dbg(fe->dvb->device, "state 
> changed, adding current state\n");
>   dvb_frontend_add_event(fe, s);
>   fepriv->status = s;
>   }
>   break;
>   case DVBFE_ALGO_SW:
> - dev_dbg(fe->dvb->device, "%s: Frontend 

Re: [PATCH dvb v1 3/4] media: dvb-usb-v2: remove __func__ from dev_dbg()

2018-11-27 Thread Sean Young
On Tue, Oct 30, 2018 at 05:14:50PM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> As dynamic debug can be instructed to add the function name to the
> debug output using +f switch, we can remove __func__ from all
> dev_dbg() calls. If not, a user that sets +f in dynamic debug would
> get duplicated function name.
> 
> Signed-off-by: Victor Toso 
> ---
>  drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 105 ++--
>  drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c  |   7 +-
>  2 files changed, 55 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c 
> b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> index d55ef016d418..ad554668cc86 100644
> --- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
> @@ -37,7 +37,7 @@ static int dvb_usbv2_download_firmware(struct 
> dvb_usb_device *d,
>  {
>   int ret;
>   const struct firmware *fw;
> - dev_dbg(>udev->dev, "%s:\n", __func__);
> + dev_dbg(>udev->dev, "\n");

How about "downloading firmware", or maybe deleting the line completely?

Without dynamic debug enabled, you end up with a pretty useless debug
message now. I think it would be better to convert these debug lines
to useful messages, rather than "executing this line of code". Some of
them should probably be deleted.

>  
>   if (!d->props->download_firmware) {
>   ret = -EINVAL;
> @@ -62,14 +62,14 @@ static int dvb_usbv2_download_firmware(struct 
> dvb_usb_device *d,
>  
>   return ret;
>  err:
> - dev_dbg(>udev->dev, "%s: failed=%d\n", __func__, ret);
> + dev_dbg(>udev->dev, "failed=%d\n", ret);

Again, just say what failed here. Ideally debug messages should be useful and
not just "hit this line of code".


Sean

>   return ret;
>  }
>  
>  static int dvb_usbv2_i2c_init(struct dvb_usb_device *d)
>  {
>   int ret;
> - dev_dbg(>udev->dev, "%s:\n", __func__);
> + dev_dbg(>udev->dev, "\n");
>  
>   if (!d->props->i2c_algo)
>   return 0;
> @@ -87,13 +87,13 @@ static int dvb_usbv2_i2c_init(struct dvb_usb_device *d)
>  
>   return 0;
>  err:
> - dev_dbg(>udev->dev, "%s: failed=%d\n", __func__, ret);
> + dev_dbg(>udev->dev, "failed=%d\n", ret);
>   return ret;
>  }
>  
>  static int dvb_usbv2_i2c_exit(struct dvb_usb_device *d)
>  {
> - dev_dbg(>udev->dev, "%s:\n", __func__);
> + dev_dbg(>udev->dev, "\n");
>  
>   if (d->i2c_adap.algo)
>   i2c_del_adapter(>i2c_adap);
> @@ -133,7 +133,7 @@ static int dvb_usbv2_remote_init(struct dvb_usb_device *d)
>  {
>   int ret;
>   struct rc_dev *dev;
> - dev_dbg(>udev->dev, "%s:\n", __func__);
> + dev_dbg(>udev->dev, "\n");
>  
>   if (dvb_usbv2_disable_rc_polling || !d->props->get_rc_config)
>   return 0;
> @@ -188,13 +188,13 @@ static int dvb_usbv2_remote_init(struct dvb_usb_device 
> *d)
>  
>   return 0;
>  err:
> - dev_dbg(>udev->dev, "%s: failed=%d\n", __func__, ret);
> + dev_dbg(>udev->dev, "failed=%d\n", ret);
>   return ret;
>  }
>  
>  static int dvb_usbv2_remote_exit(struct dvb_usb_device *d)
>  {
> - dev_dbg(>udev->dev, "%s:\n", __func__);
> + dev_dbg(>udev->dev, "\n");
>  
>   if (d->rc_dev) {
>   cancel_delayed_work_sync(>rc_query_work);
> @@ -232,7 +232,7 @@ static void dvb_usb_data_complete_raw(struct 
> usb_data_stream *stream, u8 *buf,
>  
>  static int dvb_usbv2_adapter_stream_init(struct dvb_usb_adapter *adap)
>  {
> - dev_dbg(_to_d(adap)->udev->dev, "%s: adap=%d\n", __func__,
> + dev_dbg(_to_d(adap)->udev->dev, "adap=%d\n",
>   adap->id);
>  
>   adap->stream.udev = adap_to_d(adap)->udev;
> @@ -244,7 +244,7 @@ static int dvb_usbv2_adapter_stream_init(struct 
> dvb_usb_adapter *adap)
>  
>  static int dvb_usbv2_adapter_stream_exit(struct dvb_usb_adapter *adap)
>  {
> - dev_dbg(_to_d(adap)->udev->dev, "%s: adap=%d\n", __func__,
> + dev_dbg(_to_d(adap)->udev->dev, "adap=%d\n",
>   adap->id);
>  
>   return usb_urb_exitv2(>stream);
> @@ -257,8 +257,8 @@ static int dvb_usb_start_feed(struct dvb_demux_feed 
> *dvbdmxfeed)
>   int ret = 0;
>   struct usb_data_stream_properties stream_props;
>   dev_dbg(>udev->dev,
> - "%s: adap=%d active_fe=%d feed_type=%d setting pid 
> [%s]: %04x (%04d) at index %d\n",
> - __func__, adap->id, adap->active_fe, dvbdmxfeed->type,
> + "adap=%d active_fe=%d feed_type=%d setting pid [%s]: 
> %04x (%04d) at index %d\n",
> + adap->id, adap->active_fe, dvbdmxfeed->type,
>   adap->pid_filtering ? "yes" : "no", dvbdmxfeed->pid,
>   dvbdmxfeed->pid, dvbdmxfeed->index);
>  
> @@ -334,7 +334,7 @@ static int dvb_usb_start_feed(struct dvb_demux_feed 
> *dvbdmxfeed)
>   }
>  
>   if (ret)
> - dev_dbg(>udev->dev, "%s: failed=%d\n", __func__, ret);
> + 

Re: [PATCH v10 4/4] media: add Rockchip VPU JPEG encoder driver

2018-11-27 Thread Tomasz Figa
On Fri, Nov 23, 2018 at 5:24 AM Ezequiel Garcia  wrote:
[snip]
> > > +const struct rockchip_vpu_variant rk3288_vpu_variant = {
> > > +   .enc_offset = 0x0,
> > > +   .enc_fmts = rk3288_vpu_enc_fmts,
> > > +   .num_enc_fmts = ARRAY_SIZE(rk3288_vpu_enc_fmts),
> > > +   .codec_ops = rk3288_vpu_codec_ops,
> > > +   .codec = RK_VPU_CODEC_JPEG,
> > > +   .vepu_irq = rk3288_vepu_irq,
> > > +   .init = rk3288_vpu_hw_init,
> > > +   .clk_names = {"aclk", "hclk"},
> >
> > nit: Spaces inside the brackets.
> >
>
> You mean you this style is prefered?
>
> .clk_names = { "aclk", "hclk" },
>
> Grepping thru sources, it seems there is no convention on this,
> so it's your call.
>

I thought this is a part of CodingStyle, but it doesn't seem to
mention it. I personally prefer to have the spaces there, but we can
go with your personal preferences here. :)
[snip]
> > > +* by .vidioc_s_fmt_vid_cap_mplane() callback
> > > +*/
> > > +   reg = VEPU_REG_IN_IMG_CTRL_ROW_LEN(pix_fmt->width);
> > > +   vepu_write_relaxed(vpu, reg, VEPU_REG_INPUT_LUMA_INFO);
> > > +
> > > +   reg = VEPU_REG_IN_IMG_CTRL_OVRFLR_D4(0) |
> > > + VEPU_REG_IN_IMG_CTRL_OVRFLB(0);
> >
> > For reference, this register controls the input crop, as the offset
> > from the right/bottom within the last macroblock. The offset from the
> > right must be divided by 4 and so the crop must be aligned to 4 pixels
> > horizontally.
> >
>
> OK, I'll add a comment.
>

I meant to refer to the comment Hans had, about input images of
resolutions that are not of full macroblocks, so the comment would
probably go to the TODO file together with Hans's note.
[snip]
> > > +static inline u32 vepu_read(struct rockchip_vpu_dev *vpu, u32 reg)
> > > +{
> > > +   u32 val = readl(vpu->enc_base + reg);
> > > +
> > > +   vpu_debug(6, "MARK: get reg[%03d]: %08x\n", reg / 4, val);
> >
> > I remember seeing this "MARK" in the logs when debugging. I don't
> > think it's desired here.
> >
> > How about printing "%s(%03d) = %08x\n" for reads and "%s(%08x,
> > %03d)\n" for writes?

Actually, I missed the 0x prefix for hex values and possibly also
changing the decimal register offset to hexadecimal:
"%s(0x%04x) = 0x%08x\n"

> >
>
> Makes sense, but why a %s string format?
>

For __func__, so that we end up with messages like:

vepu_write(0x12345678, 0x0123)
vepu_read(0x0123) = 0x12345678

[snip]
> > > +   dst->field = src->field;
> > > +   dst->timecode = src->timecode;
> >
> > Time code is only valid if the buffer has V4L2_BUF_FLAG_TIMECODE set.
> > I don't think there is any use case for mem2mem devices for it.
> >
>
> Right. Other mem2mem drivers seem to pass thru the timecode like this:
>
> if (in_vb->flags & V4L2_BUF_FLAG_TIMECODE)
> out_vb->timecode = in_vb->timecode;
>
> It fails a v4l2-compliance test without it.
>

Hmm, I don't see the spec defining it to be copied by a mem2mem device
and I wonder if it's actually of any use for those. Hans, could you
comment on this?

> > > +   dst->vb2_buf.timestamp = src->vb2_buf.timestamp;
> > > +   dst->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> > > +   dst->flags |= src->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> >
> > Not V4L2_BUF_FLAG_TIMESTAMP_COPY?
> >
>
> I believe v4l core should take care of it in __fill_v4l2_buffer,
> as timestamp_flags is set when the vb2_queue structs are init'ed.
>

Ah, okay, I confused this with V4L2_BUF_FLAG_TIMESTAMP_MASK.

> > > +
> > > +   if (bytesused) {
> >
> > Should we check whether bytesused (read from hardware) is not bigger
> > than size of the buffer?
> >
>
> Good catch, makes sense. OTOH, if bytesused is bigger than the dst
> buffer, it is also bigger than the bounce buffer. I guess the IOMMU
> helps prevents nasty issues?
>

Yes, in that case it would likely trigger an IOMMU fault.

Another thing is whether there isn't some special bit somewhere high
in the register we're reading from that we're not aware of, which
would make the bytesused bigger than the buffer.

[snip]
> > > +void rockchip_vpu_irq_done(struct rockchip_vpu_dev *vpu,
> > > +  unsigned int bytesused,
> > > +  enum vb2_buffer_state result)
> > > +{
> > > +   struct rockchip_vpu_ctx *ctx =
> > > +   (struct rockchip_vpu_ctx 
> > > *)v4l2_m2m_get_curr_priv(vpu->m2m_dev);
> >
> > I don't think we need to cast from void *?
> >
>
> Right.
>
> > > +
> > > +   /* Atomic watchdog cancel. The worker may still be
> > > +* running after calling this.
> > > +*/
> >
> > Wrong multi-line comment style.
> >
>
> Right.
>
> > > +   cancel_delayed_work(>watchdog_work);
> > > +   if (ctx)
> > > +   rockchip_vpu_job_finish(vpu, ctx, bytesused, result);
> > > +}
> > > +
> > > +void rockchip_vpu_watchdog(struct work_struct *work)
> > > +{
> > > +   struct rockchip_vpu_dev *vpu;
> > > +   struct rockchip_vpu_ctx *ctx;
> > > +
> > 

[PATCH v3 4/6] dt-bindings: media: mt9m111: adapt documentation to be more clear

2018-11-27 Thread Marco Felsch
Replace the vague binding by a more verbose. Remove the remote property
from the example since the driver don't support such a property. Also
remove the bus-width property from the endpoint since the driver don't
take care of it.

Signed-off-by: Marco Felsch 
Reviewed-by: Rob Herring 

---
Changelog:

v3:
- drop remote-endpoint docu, since it is documented by
  video-interfaces.txt.

v2:
- initial commit

 Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt 
b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
index 6b910036b57e..a431fb45704b 100644
--- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
+++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
@@ -9,8 +9,10 @@ Required Properties:
 - clocks: reference to the master clock.
 - clock-names: shall be "mclk".
 
-For further reading on port node refer to
-Documentation/devicetree/bindings/media/video-interfaces.txt.
+The device node must contain one 'port' child node with one 'endpoint' child
+sub-node for its digital output video port, in accordance with the video
+interface bindings defined in:
+Documentation/devicetree/bindings/media/video-interfaces.txt
 
 Example:
 
@@ -21,10 +23,8 @@ Example:
clocks = <>;
clock-names = "mclk";
 
-   remote = <_camera>;
port {
mt9m111_1: endpoint {
-   bus-width = <8>;
remote-endpoint = <_camera>;
};
};
-- 
2.19.1



[PATCH v3 2/6] media: mt9m111: add streaming check to set_fmt

2018-11-27 Thread Marco Felsch
From: Michael Grzeschik 

Currently set_fmt don't care about the streaming status, so the format
can be changed during streaming. This can lead into wrong behaviours.

Check if the device is already streaming and return -EBUSY to avoid
wrong behaviours.

Signed-off-by: Michael Grzeschik 
Signed-off-by: Marco Felsch 
Reviewed-by: Philipp Zabel 
---
 drivers/media/i2c/mt9m111.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 03559669de9f..9b0a3689fa98 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -563,6 +563,9 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
bool bayer;
int ret;
 
+   if (mt9m111->is_streaming)
+   return -EBUSY;
+
if (format->pad)
return -EINVAL;
 
-- 
2.19.1



[PATCH v3 6/6] media: mt9m111: allow to setup pixclk polarity

2018-11-27 Thread Marco Felsch
From: Enrico Scholz 

The chip can be configured to output data transitions on the
rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
falling edge.

Parsing the fw-node is made in a subfunction to bundle all (future)
dt-parsing / fw-parsing stuff.

Signed-off-by: Enrico Scholz 
(m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
per default. Set bit to 0 (enable mask bit without value) to enable
falling edge sampling.)
Signed-off-by: Michael Grzeschik 
(m.fel...@pengutronix.de: use fwnode helpers)
(m.fel...@pengutronix.de: mv fw parsing into own function)
(m.fel...@pengutronix.de: adapt commit msg)
Signed-off-by: Marco Felsch 

---
Changelog:

v3:
- call mt9m111_probe_fw() before v4l2_clk_get() to avoid error handling

v2:
- make use of fwnode_*() to drop OF dependency and ifdef's
- mt9m111_g_mbus_config: fix pclk_sample logic which I made due the
  conversion from Enrico's patch.
---
 drivers/media/i2c/mt9m111.c | 46 -
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index f97fd32181ed..2ef332b9b914 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -22,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * MT9M111, MT9M112 and MT9M131:
@@ -242,6 +244,8 @@ struct mt9m111 {
const struct mt9m111_datafmt *fmt;
int lastpage;   /* PageMap cache value */
bool is_streaming;
+   /* user point of view - 0: falling 1: rising edge */
+   unsigned int pclk_sample:1;
 #ifdef CONFIG_MEDIA_CONTROLLER
struct media_pad pad;
 #endif
@@ -594,6 +598,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
return -EINVAL;
}
 
+   /* receiver samples on falling edge, chip-hw default is rising */
+   if (mt9m111->pclk_sample == 0)
+   mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
+
ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
   data_outfmt2, mask_outfmt2);
if (!ret)
@@ -1084,9 +1092,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int 
enable)
 static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
struct v4l2_mbus_config *cfg)
 {
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
+   struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+
+   cfg->flags = V4L2_MBUS_MASTER |
V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
V4L2_MBUS_DATA_ACTIVE_HIGH;
+
+   cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_RISING :
+   V4L2_MBUS_PCLK_SAMPLE_FALLING;
+
cfg->type = V4L2_MBUS_PARALLEL;
 
return 0;
@@ -1156,6 +1170,32 @@ static int mt9m111_video_probe(struct i2c_client *client)
return ret;
 }
 
+static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
+{
+   struct v4l2_fwnode_endpoint *bus_cfg;
+   struct fwnode_handle *np;
+   int ret = 0;
+
+   np = fwnode_graph_get_next_endpoint(dev_fwnode(>dev), NULL);
+   if (!np)
+   return -EINVAL;
+
+   bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
+   if (IS_ERR(bus_cfg)) {
+   ret = PTR_ERR(bus_cfg);
+   goto out_put_fw;
+   }
+
+   mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
+ V4L2_MBUS_PCLK_SAMPLE_RISING);
+
+   v4l2_fwnode_endpoint_free(bus_cfg);
+
+out_put_fw:
+   fwnode_handle_put(np);
+   return ret;
+}
+
 static int mt9m111_probe(struct i2c_client *client,
 const struct i2c_device_id *did)
 {
@@ -1173,6 +1213,10 @@ static int mt9m111_probe(struct i2c_client *client,
if (!mt9m111)
return -ENOMEM;
 
+   ret = mt9m111_probe_fw(client, mt9m111);
+   if (ret)
+   return ret;
+
mt9m111->clk = v4l2_clk_get(>dev, "mclk");
if (IS_ERR(mt9m111->clk))
return PTR_ERR(mt9m111->clk);
-- 
2.19.1



[PATCH v3 1/6] media: mt9m111: add s_stream callback

2018-11-27 Thread Marco Felsch
Add callback to check if we are already streaming. Now other callbacks
can check the state and return -EBUSY if we already streaming.

Signed-off-by: Marco Felsch 
---
 drivers/media/i2c/mt9m111.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 58d134dcdf44..03559669de9f 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -220,6 +220,7 @@ struct mt9m111 {
int power_count;
const struct mt9m111_datafmt *fmt;
int lastpage;   /* PageMap cache value */
+   bool is_streaming;
 #ifdef CONFIG_MEDIA_CONTROLLER
struct media_pad pad;
 #endif
@@ -910,6 +911,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
return 0;
 }
 
+static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
+{
+   struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+
+   mt9m111->is_streaming = !!enable;
+   return 0;
+}
+
 static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
struct v4l2_mbus_config *cfg)
 {
@@ -923,6 +932,7 @@ static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
 
 static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = {
.g_mbus_config  = mt9m111_g_mbus_config,
+   .s_stream   = mt9m111_s_stream,
 };
 
 static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = {
-- 
2.19.1



[PATCH v3 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}

2018-11-27 Thread Marco Felsch
From: Michael Grzeschik 

This patch implements the framerate selection using the skipping and
readout power-modi features. The power-modi cut the framerate by half
and each context has an independent selection bit. The same applies to
the 2x skipping feature.

Signed-off-by: Michael Grzeschik 
Signed-off-by: Marco Felsch 

---
Changelog

v3:
- check if sensor window size is set to default and return if not
- check if requested fps is supported by image size and do not update
  the image size if requested fps is not supported by selected image
  size
- update fps mode only if a mode was found

v2:
- fix updating read mode register, use mt9m111_reg_mask() to update the
  relevant bits only. For this purpose add reg_mask field to
  struct mt9m111_mode_info.

 drivers/media/i2c/mt9m111.c | 165 
 1 file changed, 165 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 9b0a3689fa98..f97fd32181ed 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -129,6 +129,8 @@
 #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B(1 << 0)
 #define MT9M111_TPG_SEL_MASK   GENMASK(2, 0)
 #define MT9M111_EFFECTS_MODE_MASK  GENMASK(2, 0)
+#define MT9M111_RM_PWR_MASKBIT(10)
+#define MT9M111_RM_SKIP2_MASK  GENMASK(3, 2)
 
 /*
  * Camera control register addresses (0x200..0x2ff not implemented)
@@ -207,6 +209,23 @@ static const struct mt9m111_datafmt mt9m111_colour_fmts[] 
= {
{MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
 };
 
+enum mt9m111_mode_id {
+   MT9M111_MODE_SXGA_8FPS,
+   MT9M111_MODE_SXGA_15FPS,
+   MT9M111_MODE_QSXGA_30FPS,
+   MT9M111_NUM_MODES,
+};
+
+struct mt9m111_mode_info {
+   unsigned int sensor_w;
+   unsigned int sensor_h;
+   unsigned int max_image_w;
+   unsigned int max_image_h;
+   unsigned int max_fps;
+   unsigned int reg_val;
+   unsigned int reg_mask;
+};
+
 struct mt9m111 {
struct v4l2_subdev subdev;
struct v4l2_ctrl_handler hdl;
@@ -216,6 +235,8 @@ struct mt9m111 {
struct v4l2_clk *clk;
unsigned int width; /* output */
unsigned int height;/* sizes */
+   struct v4l2_fract frame_interval;
+   const struct mt9m111_mode_info *current_mode;
struct mutex power_lock; /* lock to protect power_count */
int power_count;
const struct mt9m111_datafmt *fmt;
@@ -226,6 +247,37 @@ struct mt9m111 {
 #endif
 };
 
+static const struct mt9m111_mode_info mt9m111_mode_data[MT9M111_NUM_MODES] = {
+   [MT9M111_MODE_SXGA_8FPS] = {
+   .sensor_w = 1280,
+   .sensor_h = 1024,
+   .max_image_w = 1280,
+   .max_image_h = 1024,
+   .max_fps = 8,
+   .reg_val = MT9M111_RM_LOW_POWER_RD,
+   .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
+   },
+   [MT9M111_MODE_SXGA_15FPS] = {
+   .sensor_w = 1280,
+   .sensor_h = 1024,
+   .max_image_w = 1280,
+   .max_image_h = 1024,
+   .max_fps = 15,
+   .reg_val = MT9M111_RM_FULL_POWER_RD,
+   .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
+   },
+   [MT9M111_MODE_QSXGA_30FPS] = {
+   .sensor_w = 1280,
+   .sensor_h = 1024,
+   .max_image_w = 640,
+   .max_image_h = 512,
+   .max_fps = 30,
+   .reg_val = MT9M111_RM_LOW_POWER_RD | MT9M111_RM_COL_SKIP_2X |
+  MT9M111_RM_ROW_SKIP_2X,
+   .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
+   },
+};
+
 /* Find a data format by a pixel code */
 static const struct mt9m111_datafmt *mt9m111_find_datafmt(struct mt9m111 
*mt9m111,
u32 code)
@@ -618,6 +670,61 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
return ret;
 }
 
+static const struct mt9m111_mode_info *
+mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
+ unsigned int width, unsigned int height)
+{
+   const struct mt9m111_mode_info *mode;
+   struct v4l2_rect *sensor_rect = >rect;
+   unsigned int gap, gap_best = (unsigned int) -1;
+   int i, best_gap_idx = MT9M111_MODE_SXGA_15FPS;
+   bool skip_30fps = false;
+
+   /*
+* The fps selection is based on the row, column skipping mechanism.
+* So ensure that the sensor window is set to default else the fps
+* aren't calculated correctly within the sensor hw.
+*/
+   if (sensor_rect->width != MT9M111_MAX_WIDTH ||
+   sensor_rect->height != MT9M111_MAX_HEIGHT) {
+   dev_info(mt9m111->subdev.dev,
+"Framerate selection is not supported for cropped "
+"images\n");
+   return NULL;
+   }
+
+   /* 30fps only supported 

[PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property

2018-11-27 Thread Marco Felsch
Add the pclk-sample property to the list of optional properties
for the mt9m111 camera sensor.

Signed-off-by: Marco Felsch 
Reviewed-by: Rob Herring 
---
 Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt 
b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
index a431fb45704b..d0bed6fa901a 100644
--- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
+++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
@@ -14,6 +14,10 @@ sub-node for its digital output video port, in accordance 
with the video
 interface bindings defined in:
 Documentation/devicetree/bindings/media/video-interfaces.txt
 
+Optional endpoint properties:
+- pclk-sample: For information see ../video-interfaces.txt. The value is set to
+  0 if it isn't specified.
+
 Example:
 
i2c_master {
@@ -26,6 +30,7 @@ Example:
port {
mt9m111_1: endpoint {
remote-endpoint = <_camera>;
+   pclk-sample = <1>;
};
};
};
-- 
2.19.1



[PATCH v3 0/6] media: mt9m111 features

2018-11-27 Thread Marco Felsch
Hi,

this v3 integrate the review  of my v2 [1]. I reordered the series as
mentioned by Sakari.

The patches are rebased on top of the actual media-tree/master.

[1] https://www.mail-archive.com/linux-media@vger.kernel.org/msg135932.html

Regards,
Marco


Enrico Scholz (1):
  media: mt9m111: allow to setup pixclk polarity

Marco Felsch (3):
  media: mt9m111: add s_stream callback
  dt-bindings: media: mt9m111: adapt documentation to be more clear
  dt-bindings: media: mt9m111: add pclk-sample property

Michael Grzeschik (2):
  media: mt9m111: add streaming check to set_fmt
  media: mt9m111: add support to select formats and fps for {Q,SXGA}

 .../devicetree/bindings/media/i2c/mt9m111.txt |  13 +-
 drivers/media/i2c/mt9m111.c   | 224 +-
 2 files changed, 232 insertions(+), 5 deletions(-)

-- 
2.19.1



[PATCH v4l-utils] v4l2-compliance needs fork

2018-11-27 Thread Fabrice Fontaine
v4l2-compliance uses fork, since
https://git.linuxtv.org/v4l-utils.git/commit/utils/v4l2-compliance/?id=79d98edd1a27233667a6bc38d3d7f8958c2ec02c

So don't build it if fork is not available

Fixes:
 - 
http://autobuild.buildroot.org/results/447d792ce21c0e33a36ca9384fee46e099435ed8

Signed-off-by: Fabrice Fontaine 
---
 configure.ac  | 5 -
 utils/Makefile.am | 6 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 5cc34c24..52ea5c6d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -478,7 +478,8 @@ AM_CONDITIONAL([WITH_QTGL], [test x${qt_pkgconfig_gl} = 
xtrue])
 AM_CONDITIONAL([WITH_GCONV],[test x$enable_gconv = xyes -a 
x$enable_shared == xyes -a x$with_gconvdir != x -a -f 
$with_gconvdir/gconv-modules])
 AM_CONDITIONAL([WITH_V4L2_CTL_LIBV4L], [test x${enable_v4l2_ctl_libv4l} != 
xno])
 AM_CONDITIONAL([WITH_V4L2_CTL_STREAM_TO], [test x${enable_v4l2_ctl_stream_to} 
!= xno])
-AM_CONDITIONAL([WITH_V4L2_COMPLIANCE_LIBV4L], [test 
x${enable_v4l2_compliance_libv4l} != xno])
+AM_CONDITIONAL([WITH_V4L2_COMPLIANCE], [test x$ac_cv_func_fork = xyes])
+AM_CONDITIONAL([WITH_V4L2_COMPLIANCE_LIBV4L], [test x$ac_cv_func_fork = xyes 
-a x${enable_v4l2_compliance_libv4l} != xno])
 AM_CONDITIONAL([WITH_BPF],  [test x$enable_bpf != xno -a 
x$libelf_pkgconfig = xyes -a x$CLANG = xclang])
 
 # append -static to libtool compile and link command to enforce static libs
@@ -509,6 +510,7 @@ AM_COND_IF([WITH_V4L_PLUGINS], [USE_V4L_PLUGINS="yes"
 AM_COND_IF([WITH_V4L_WRAPPERS], [USE_V4L_WRAPPERS="yes"], 
[USE_V4L_WRAPPERS="no"])
 AM_COND_IF([WITH_GCONV], [USE_GCONV="yes"], [USE_GCONV="no"])
 AM_COND_IF([WITH_V4L2_CTL_LIBV4L], [USE_V4L2_CTL_LIBV4L="yes"], 
[USE_V4L2_CTL_LIBV4L="no"])
+AM_COND_IF([WITH_V4L2_COMPLIANCE], [USE_V4L2_COMPLIANCE="yes"], 
[USE_V4L2_COMPLIANCE="no"])
 AM_COND_IF([WITH_V4L2_COMPLIANCE_LIBV4L], [USE_V4L2_COMPLIANCE_LIBV4L="yes"], 
[USE_V4L2_COMPLIANCE_LIBV4L="no"])
 AM_COND_IF([WITH_BPF], [USE_BPF="yes"
 AC_DEFINE([HAVE_BPF], [1], [BPF IR decoder 
support enabled])],
@@ -556,6 +558,7 @@ compile time options summary
 qv4l2  : $USE_QV4L2
 qvidcap: $USE_QVIDCAP
 v4l2-ctl uses libv4l   : $USE_V4L2_CTL_LIBV4L
+v4l2-compliance: $USE_V4L2_COMPLIANCE
 v4l2-compliance uses libv4l: $USE_V4L2_COMPLIANCE_LIBV4L
 BPF IR Decoders:   : $USE_BPF
 EOF
diff --git a/utils/Makefile.am b/utils/Makefile.am
index 2d507028..9c29926a 100644
--- a/utils/Makefile.am
+++ b/utils/Makefile.am
@@ -6,7 +6,6 @@ SUBDIRS = \
cx18-ctl \
keytable \
media-ctl \
-   v4l2-compliance \
v4l2-ctl \
v4l2-dbg \
v4l2-sysfs-path \
@@ -20,6 +19,11 @@ SUBDIRS += \
dvb
 endif
 
+if WITH_V4L2_COMPLIANCE
+SUBDIRS += \
+   v4l2-compliance
+endif
+
 if WITH_QV4L2
 SUBDIRS += qv4l2
 endif
-- 
2.17.1



[RESEND PATCH 1/1] media: Use common test pattern menu entries

2018-11-27 Thread Sakari Ailus
While the test pattern menu itself is not standardised, many devices
support the same test patterns. Aligning the menu entries helps the user
space to use the interface, and adding macros for the menu entry strings
helps to keep them aligned.

Signed-off-by: Sakari Ailus 
---
Fixed Andy's email.

 drivers/media/i2c/imx258.c | 10 +-
 drivers/media/i2c/imx319.c | 10 +-
 drivers/media/i2c/imx355.c | 10 +-
 drivers/media/i2c/ov2640.c |  4 ++--
 drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
 include/uapi/linux/v4l2-controls.h |  5 +
 6 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index f86ae18bc104..c795d4c4c0e4 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = {
 };
 
 static const char * const imx258_test_pattern_menu[] = {
-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
 };
 
 /* Configurations for supported link frequencies */
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221..eddaf69a67b6 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] = {
 };
 
 static const char * const imx319_test_pattern_menu[] = {
-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
 };
 
 /* supported link frequencies */
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index bed293b60e50..824d07156f9c 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
 };
 
 static const char * const imx355_test_pattern_menu[] = {
-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
 };
 
 /* supported link frequencies */
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 5d2d6735cc78..507ec7176a7d 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
 }
 
 static const char * const ov2640_test_pattern_menu[] = {
-   "Disabled",
-   "Eight Vertical Colour Bars",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
 };
 
 /*
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
b/drivers/media/i2c/smiapp/smiapp-core.c
index 58a45c353e27..f6a92b9f178c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct 
smiapp_sensor *sensor)
 }
 
 static const char * const smiapp_test_patterns[] = {
-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
 };
 
 static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
diff --git a/include/uapi/linux/v4l2-controls.h 
b/include/uapi/linux/v4l2-controls.h
index 998983a6e6b7..a74ff6f1ac88 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_LINK_FREQ (V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 1)
 #define V4L2_CID_PIXEL_RATE(V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 2)
 #define V4L2_CID_TEST_PATTERN  (V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 3)
+#define V4L2_TEST_PATTERN_DISABLED "Disabled"
+#define V4L2_TEST_PATTERN_SOLID_COLOUR "Solid Colour"
+#define V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS   "Eight Vertical Colour 
Bars"
+#define V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY "Colour Bars With 
Fade to Grey"
+#define 

Re: [PATCH v2] media: unify some sony camera sensors pattern naming

2018-11-27 Thread Sakari Ailus
On Tue, Nov 27, 2018 at 05:03:26PM +0800, bingbu@intel.com wrote:
> From: Bingbu Cao 
> 
> Some Sony camera sensors have same pattern
> definitions, this patch unify the pattern naming
> to make it more clear to the userspace.
> 
> Suggested-by: Sakari Ailus 
> Signed-off-by: Bingbu Cao 
> Reviewed-by: l...@lucaceresoli.net

Hi Bing Bu,

Sorry, I prefer your v1. :-)

-- 
Sakari Ailus
sakari.ai...@linux.intel.com


[PATCH 1/1] media: Use common test pattern menu entries

2018-11-27 Thread Sakari Ailus
While the test pattern menu itself is not standardised, many devices
support the same test patterns. Aligning the menu entries helps the user
space to use the interface, and adding macros for the menu entry strings
helps to keep them aligned.

Signed-off-by: Sakari Ailus 
---
 drivers/media/i2c/imx258.c | 10 +-
 drivers/media/i2c/imx319.c | 10 +-
 drivers/media/i2c/imx355.c | 10 +-
 drivers/media/i2c/ov2640.c |  4 ++--
 drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
 include/uapi/linux/v4l2-controls.h |  5 +
 6 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index f86ae18bc104..c795d4c4c0e4 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = {
 };
 
 static const char * const imx258_test_pattern_menu[] = {
-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
 };
 
 /* Configurations for supported link frequencies */
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221..eddaf69a67b6 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] = {
 };
 
 static const char * const imx319_test_pattern_menu[] = {
-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
 };
 
 /* supported link frequencies */
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index bed293b60e50..824d07156f9c 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
 };
 
 static const char * const imx355_test_pattern_menu[] = {
-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
 };
 
 /* supported link frequencies */
diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
index 5d2d6735cc78..507ec7176a7d 100644
--- a/drivers/media/i2c/ov2640.c
+++ b/drivers/media/i2c/ov2640.c
@@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
 }
 
 static const char * const ov2640_test_pattern_menu[] = {
-   "Disabled",
-   "Eight Vertical Colour Bars",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
 };
 
 /*
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
b/drivers/media/i2c/smiapp/smiapp-core.c
index 58a45c353e27..f6a92b9f178c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct 
smiapp_sensor *sensor)
 }
 
 static const char * const smiapp_test_patterns[] = {
-   "Disabled",
-   "Solid Colour",
-   "Eight Vertical Colour Bars",
-   "Colour Bars With Fade to Grey",
-   "Pseudorandom Sequence (PN9)",
+   V4L2_TEST_PATTERN_DISABLED,
+   V4L2_TEST_PATTERN_SOLID_COLOUR,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
+   V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
+   V4L2_TEST_PATTERN_PN9,
 };
 
 static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
diff --git a/include/uapi/linux/v4l2-controls.h 
b/include/uapi/linux/v4l2-controls.h
index 998983a6e6b7..a74ff6f1ac88 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_LINK_FREQ (V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 1)
 #define V4L2_CID_PIXEL_RATE(V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 2)
 #define V4L2_CID_TEST_PATTERN  (V4L2_CID_IMAGE_PROC_CLASS_BASE 
+ 3)
+#define V4L2_TEST_PATTERN_DISABLED "Disabled"
+#define V4L2_TEST_PATTERN_SOLID_COLOUR "Solid Colour"
+#define V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS   "Eight Vertical Colour 
Bars"
+#define V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY "Colour Bars With 
Fade to Grey"
+#define V4L2_TEST_PATTERN_PN9  

Re: [PATCH 2/2] media: imx355: fix wrong order in test pattern menus

2018-11-27 Thread Sakari Ailus
On Tue, Nov 27, 2018 at 10:45:02AM +0800, Bingbu Cao wrote:
> 
> 
> On 11/26/2018 04:57 PM, Sakari Ailus wrote:
> > Hi Bing Bu,
> > 
> > On Mon, Nov 26, 2018 at 03:43:34PM +0800, bingbu@intel.com wrote:
> > > From: Bingbu Cao 
> > > 
> > > current imx355 test pattern order in ctrl menu
> > > is not correct, this patch fixes it.
> > > 
> > > Signed-off-by: Bingbu Cao 
> > > ---
> > >   drivers/media/i2c/imx355.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> > > index 20c8eea5db4b..9c9559dfd3dd 100644
> > > --- a/drivers/media/i2c/imx355.c
> > > +++ b/drivers/media/i2c/imx355.c
> > > @@ -876,8 +876,8 @@ struct imx355 {
> > >   static const char * const imx355_test_pattern_menu[] = {
> > >   "Disabled",
> > > - "100% color bars",
> > >   "Solid color",
> > > + "100% color bars",
> > >   "Fade to gray color bars",
> > >   "PN9"
> > >   };
> > While at it, could you use the existing test pattern naming as well for the
> > drivers? That could be a separate patch.
> > 
> > >From drivers/media/i2c/smiapp/smiapp-core.c :
> > 
> > static const char * const smiapp_test_patterns[] = {
> > "Disabled",
> > "Solid Colour",
> > "Eight Vertical Colour Bars",
> > "Colour Bars With Fade to Grey",
> > "Pseudorandom Sequence (PN9)",
> > };
> > 
> > It's not strictly necessary from interface point of view, but for the user
> > space it'd be good to align the naming.
> Sakari, ask a question that not really related to this patch.
> I am wondering whether there are some standardization for the camera sensor
> pattern generator.
> Currently there are varied patterns and every vendor has its own pattern 
> types.

Some vendors (and some models) do have their specific patterns, but if you
look at a bunch of drivers, these ones repeat over and over. That's why I
think it'd make sense to have the menu entries aligned.

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi


Re: [PATCH] media: unify some sony camera sensors pattern naming

2018-11-27 Thread Luca Ceresoli
Hi Bingbu,

On 27/11/18 09:55, Bingbu Cao wrote:
> 
> 
> On 11/27/2018 04:05 PM, Luca Ceresoli wrote:
>> Hi Bingbu,
>>
>> On 27/11/18 05:01, bingbu@intel.com wrote:
>>> From: Bingbu Cao 
>>>
>>> Some Sony camera sensors have same test pattern
>>> definitions, this patch unify the pattern naming
>>> to make it more clear to the userspace.
>>>
>>> Suggested-by: Sakari Ailus 
>>> Signed-off-by: Bingbu Cao 
>>> ---
>>>   drivers/media/i2c/imx258.c | 8 
>>>   drivers/media/i2c/imx319.c | 8 
>>>   drivers/media/i2c/imx355.c | 8 
>>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>> index 31a1e2294843..a8a2880c6b4e 100644
>>> --- a/drivers/media/i2c/imx258.c
>>> +++ b/drivers/media/i2c/imx258.c
>>> @@ -504,10 +504,10 @@ struct imx258_mode {
>>>     static const char * const imx258_test_pattern_menu[] = {
>>>   "Disabled",
>>> -    "Color Bars",
>>> -    "Solid Color",
>>> -    "Grey Color Bars",
>>> -    "PN9"
>>> +    "Solid Colour",
>>> +    "Eight Vertical Colour Bars",
>>> +    "Colour Bars With Fade to Grey",
>>> +    "Pseudorandom Sequence (PN9)",
>> I had a look at imx274, it has many more values but definitely some
>> could be unified too.
>>
>> However I noticed something strange in that driver: The "Horizontal
>> Color Bars" pattern has vertical bars, side-by-side, as in .
>> "Vertical Color Bars" are one on top of the other, as in ==. Is it just
>> me crazy, or are they swapped?
> Luca, thanks for your review.
> I do not have the manual of imx274.
>  should be the 'Vertical Color Bars' without any rotation process.
> If not, I think the definitions there are swapped.

Definitely. I'll send a patch for imx274.

-- 
Luca


Re: [PATCH] media: unify some sony camera sensors pattern naming

2018-11-27 Thread Sakari Ailus
Hi Luca,

On Tue, Nov 27, 2018 at 09:05:34AM +0100, Luca Ceresoli wrote:
> Hi Bingbu,
> 
> On 27/11/18 05:01, bingbu@intel.com wrote:
> > From: Bingbu Cao 
> > 
> > Some Sony camera sensors have same test pattern
> > definitions, this patch unify the pattern naming
> > to make it more clear to the userspace.
> > 
> > Suggested-by: Sakari Ailus 
> > Signed-off-by: Bingbu Cao 
> > ---
> >  drivers/media/i2c/imx258.c | 8 
> >  drivers/media/i2c/imx319.c | 8 
> >  drivers/media/i2c/imx355.c | 8 
> >  3 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 31a1e2294843..a8a2880c6b4e 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -504,10 +504,10 @@ struct imx258_mode {
> >  
> >  static const char * const imx258_test_pattern_menu[] = {
> > "Disabled",
> > -   "Color Bars",
> > -   "Solid Color",
> > -   "Grey Color Bars",
> > -   "PN9"
> > +   "Solid Colour",
> > +   "Eight Vertical Colour Bars",
> > +   "Colour Bars With Fade to Grey",
> > +   "Pseudorandom Sequence (PN9)",
> 
> I had a look at imx274, it has many more values but definitely some
> could be unified too.
> 
> However I noticed something strange in that driver: The "Horizontal
> Color Bars" pattern has vertical bars, side-by-side, as in .
> "Vertical Color Bars" are one on top of the other, as in ==. Is it just
> me crazy, or are they swapped?
> 
> Only one minor nitpick about your patch. The USA spelling "color" seems
> a lot more frequent in the kernel sources than the UK "colour", so it's
> probably better to be consistent.

This has been around for some seven or so years in the smiapp driver, and
changing strings in uAPI isn't something we prefer to do in general.

I wonder what others think.

If that's changed, it should be a separate patch.

-- 
Sakari Ailus
sakari.ai...@linux.intel.com


[PATCH v2] media: unify some sony camera sensors pattern naming

2018-11-27 Thread bingbu . cao
From: Bingbu Cao 

Some Sony camera sensors have same pattern
definitions, this patch unify the pattern naming
to make it more clear to the userspace.

Suggested-by: Sakari Ailus 
Signed-off-by: Bingbu Cao 
Reviewed-by: l...@lucaceresoli.net
---
 drivers/media/i2c/imx258.c | 6 +++---
 drivers/media/i2c/imx319.c | 8 
 drivers/media/i2c/imx355.c | 8 
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 31a1e2294843..de399f69dc9b 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -504,10 +504,10 @@ struct imx258_mode {
 
 static const char * const imx258_test_pattern_menu[] = {
"Disabled",
-   "Color Bars",
"Solid Color",
-   "Grey Color Bars",
-   "PN9"
+   "Eight Vertical Color Bars",
+   "Color Bars With Fade to Grey",
+   "Pseudorandom Sequence (PN9)",
 };
 
 static const int imx258_test_pattern_val[] = {
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index acd988d2d7f1..c508887c4574 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -1648,10 +1648,10 @@ struct imx319 {
 
 static const char * const imx319_test_pattern_menu[] = {
"Disabled",
-   "Solid color",
-   "100% color bars",
-   "Fade to gray color bars",
-   "PN9"
+   "Solid Color",
+   "Eight Vertical Color Bars",
+   "Color Bars With Fade to Grey",
+   "Pseudorandom Sequence (PN9)",
 };
 
 /* supported link frequencies */
diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index 9c9559dfd3dd..608947dc1a65 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -876,10 +876,10 @@ struct imx355 {
 
 static const char * const imx355_test_pattern_menu[] = {
"Disabled",
-   "Solid color",
-   "100% color bars",
-   "Fade to gray color bars",
-   "PN9"
+   "Solid Color",
+   "Eight Vertical Color Bars",
+   "Color Bars With Fade to Grey",
+   "Pseudorandom Sequence (PN9)",
 };
 
 /* supported link frequencies */
-- 
1.9.1



Re: [PATCH] media: unify some sony camera sensors pattern naming

2018-11-27 Thread Bingbu Cao




On 11/27/2018 04:05 PM, Luca Ceresoli wrote:

Hi Bingbu,

On 27/11/18 05:01, bingbu@intel.com wrote:

From: Bingbu Cao 

Some Sony camera sensors have same test pattern
definitions, this patch unify the pattern naming
to make it more clear to the userspace.

Suggested-by: Sakari Ailus 
Signed-off-by: Bingbu Cao 
---
  drivers/media/i2c/imx258.c | 8 
  drivers/media/i2c/imx319.c | 8 
  drivers/media/i2c/imx355.c | 8 
  3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 31a1e2294843..a8a2880c6b4e 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -504,10 +504,10 @@ struct imx258_mode {
  
  static const char * const imx258_test_pattern_menu[] = {

"Disabled",
-   "Color Bars",
-   "Solid Color",
-   "Grey Color Bars",
-   "PN9"
+   "Solid Colour",
+   "Eight Vertical Colour Bars",
+   "Colour Bars With Fade to Grey",
+   "Pseudorandom Sequence (PN9)",

I had a look at imx274, it has many more values but definitely some
could be unified too.

However I noticed something strange in that driver: The "Horizontal
Color Bars" pattern has vertical bars, side-by-side, as in .
"Vertical Color Bars" are one on top of the other, as in ==. Is it just
me crazy, or are they swapped?

Luca, thanks for your review.
I do not have the manual of imx274.
 should be the 'Vertical Color Bars' without any rotation process.
If not, I think the definitions there are swapped.

Only one minor nitpick about your patch. The USA spelling "color" seems
a lot more frequent in the kernel sources than the UK "colour", so it's
probably better to be consistent.

Ack, I will update in next version.

Thanks!






Re: [PATCH] media: unify some sony camera sensors pattern naming

2018-11-27 Thread Luca Ceresoli
Hi Bingbu,

On 27/11/18 05:01, bingbu@intel.com wrote:
> From: Bingbu Cao 
> 
> Some Sony camera sensors have same test pattern
> definitions, this patch unify the pattern naming
> to make it more clear to the userspace.
> 
> Suggested-by: Sakari Ailus 
> Signed-off-by: Bingbu Cao 
> ---
>  drivers/media/i2c/imx258.c | 8 
>  drivers/media/i2c/imx319.c | 8 
>  drivers/media/i2c/imx355.c | 8 
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 31a1e2294843..a8a2880c6b4e 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -504,10 +504,10 @@ struct imx258_mode {
>  
>  static const char * const imx258_test_pattern_menu[] = {
>   "Disabled",
> - "Color Bars",
> - "Solid Color",
> - "Grey Color Bars",
> - "PN9"
> + "Solid Colour",
> + "Eight Vertical Colour Bars",
> + "Colour Bars With Fade to Grey",
> + "Pseudorandom Sequence (PN9)",

I had a look at imx274, it has many more values but definitely some
could be unified too.

However I noticed something strange in that driver: The "Horizontal
Color Bars" pattern has vertical bars, side-by-side, as in .
"Vertical Color Bars" are one on top of the other, as in ==. Is it just
me crazy, or are they swapped?

Only one minor nitpick about your patch. The USA spelling "color" seems
a lot more frequent in the kernel sources than the UK "colour", so it's
probably better to be consistent.

-- 
Luca