cron job: media_tree daily build: WARNINGS

2017-09-04 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:   Tue Sep  5 05:00:17 CEST 2017
media-tree git hash:fce4b371fe5c99a9c05db8493d72f0d1a474ab26
media_build git hash:   767234f97b7e967bd0ce362957de82764937951c
v4l-utils git hash: 3296adfa7fa169111bf37c041c0ca70ac8506054
gcc version:i686-linux-gcc (GCC) 7.1.0
sparse version: v0.5.0
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.12.0-164

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

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

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

Re: [PATCH 1/1] docs-rst: media: Don't use \small for V4L2_PIX_FMT_SRGGB10 documentation

2017-09-04 Thread Mauro Carvalho Chehab 
Em Sun, 3 Sep 2017 22:40:02 -0300
Mauro Carvalho Chehab  escreveu:

> Em Sun,  3 Sep 2017 23:12:33 +0300
> Sakari Ailus  escreveu:
> 
> > There appears to be an issue in using \small in certain cases on Sphinx
> > 1.4 and 1.5. Other format documents don't use \small either, remove it
> > from here as well.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> > Hi Mauro,
> > 
> > What would you think of this as an alternative approach? No hacks needed.
> > Just a recognition \small could have issues. For what it's worth, I
> > couldn't reproduce the issue on Sphinx 1.4.9.
> 
> Btw, there are other places where \small runs smoothly. It is *just*
> on this table that it has issues.
> 
> 
> > 
> > Regards,
> > Sakari
> > 
> >  Documentation/media/uapi/v4l/pixfmt-srggb10p.rst | 11 ---
> >  1 file changed, 11 deletions(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst 
> > b/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
> > index 86cd07e5bfa3..368ee61ab209 100644
> > --- a/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
> > +++ b/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
> > @@ -33,13 +33,6 @@ of a small V4L2_PIX_FMT_SBGGR10P image:
> >  **Byte Order.**
> >  Each cell is one byte.
> >  
> > -
> > -.. raw:: latex
> > -
> > -\small
> 
> Interesting... yeah, that could be possible.
> 
> > -
> > -.. tabularcolumns:: 
> > |p{2.0cm}|p{1.0cm}|p{1.0cm}|p{1.0cm}|p{1.0cm}|p{10.0cm}|
> 
> Nah... Without tabularcolumns, LaTeX usually got sizes wrong and don't
> always place things at the right positions I'm actually considering 
> adding it to all media tables, in order to be less dependent on
> LaTex automatic cells resizing - with doesn't seem to work too well.
> 
> So, better to keep it, even if it works without
> \small. Btw, tried your patch here (without tabularcolumns) on
> Sphinx 1.6 (tomorrow, I'll do tests with other version). There, the
> last "(bits x-y)" ends by being wrapped to the next line.
> 
> Yet, I guess the enclosed diff (or something like that) would be
> good enough (applied after my own patch, just to quickly test it). 
> 
> I'll play more with it tomorrow.

OK, that works. Thanks!

I rebased your patch, keeping tabularcolumns and adding blank lines
to reduce the column size.

That works really better.

I also added a second patch doing the same for srggb12p.


Thanks,
Mauro

[PATCH 1/2] docs-rst: media: Don't use \small for V4L2_PIX_FMT_SRGGB10 documentation

2017-09-04 Thread Mauro Carvalho Chehab 
From: Sakari Ailus 

There appears to be an issue in using \small in certain cases on Sphinx
1.4 and 1.5. Other format documents don't use \small either, remove it
from here as well.

[mche...@s-opensource.com: kept tabularcolumns - readjusted - and
 add a few blank lines for it to display better]
Signed-off-by: Sakari Ailus 
Signed-off-by: Mauro Carvalho Chehab 
---
 Documentation/media/uapi/v4l/pixfmt-srggb10p.rst | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst 
b/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
index 9e52610aa954..d9e07a4b8b31 100644
--- a/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-srggb10p.rst
@@ -33,12 +33,7 @@ of a small V4L2_PIX_FMT_SBGGR10P image:
 **Byte Order.**
 Each cell is one byte.
 
-
-.. raw:: latex
-
-\newline\small
-
-.. tabularcolumns:: |p{2.0cm}|p{1.0cm}|p{1.0cm}|p{1.0cm}|p{1.0cm}|p{10.0cm}|
+.. tabularcolumns:: |p{2.0cm}|p{1.0cm}|p{1.0cm}|p{1.0cm}|p{1.0cm}|p{5.4cm}|
 
 .. flat-table::
 :header-rows:  0
@@ -51,6 +46,7 @@ Each cell is one byte.
   - B\ :sub:`02high`
   - G\ :sub:`03high`
   - G\ :sub:`03low`\ (bits 7--6) B\ :sub:`02low`\ (bits 5--4)
+
G\ :sub:`01low`\ (bits 3--2) B\ :sub:`00low`\ (bits 1--0)
 * - start + 5:
   - G\ :sub:`10high`
@@ -58,6 +54,7 @@ Each cell is one byte.
   - G\ :sub:`12high`
   - R\ :sub:`13high`
   - R\ :sub:`13low`\ (bits 7--6) G\ :sub:`12low`\ (bits 5--4)
+
R\ :sub:`11low`\ (bits 3--2) G\ :sub:`10low`\ (bits 1--0)
 * - start + 10:
   - B\ :sub:`20high`
@@ -65,6 +62,7 @@ Each cell is one byte.
   - B\ :sub:`22high`
   - G\ :sub:`23high`
   - G\ :sub:`23low`\ (bits 7--6) B\ :sub:`22low`\ (bits 5--4)
+
G\ :sub:`21low`\ (bits 3--2) B\ :sub:`20low`\ (bits 1--0)
 * - start + 15:
   - G\ :sub:`30high`
@@ -72,8 +70,5 @@ Each cell is one byte.
   - G\ :sub:`32high`
   - R\ :sub:`33high`
   - R\ :sub:`33low`\ (bits 7--6) G\ :sub:`32low`\ (bits 5--4)
+
R\ :sub:`31low`\ (bits 3--2) G\ :sub:`30low`\ (bits 1--0)
-
-.. raw:: latex
-
-\normalsize
-- 
2.13.5

[PATCH 2/2] media: pixfmt-srggb12p.rst: better format the table for PDF output

2017-09-04 Thread Mauro Carvalho Chehab 
Adjust the table to be better displayed on PDF output.

Signed-off-by: Mauro Carvalho Chehab 
---
 Documentation/media/uapi/v4l/pixfmt-srggb12p.rst | 59 +---
 1 file changed, 21 insertions(+), 38 deletions(-)

diff --git a/Documentation/media/uapi/v4l/pixfmt-srggb12p.rst 
b/Documentation/media/uapi/v4l/pixfmt-srggb12p.rst
index c0541e5acd01..59918a7913fe 100644
--- a/Documentation/media/uapi/v4l/pixfmt-srggb12p.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-srggb12p.rst
@@ -30,6 +30,7 @@ Below is an example of a small V4L2_PIX_FMT_SBGGR12P image:
 **Byte Order.**
 Each cell is one byte.
 
+.. tabularcolumns:: 
|p{2.0cm}|p{1.0cm}|p{1.0cm}|p{2.7cm}|p{1.0cm}|p{1.0cm}|p{2.7cm}|
 
 
 .. flat-table::
@@ -38,66 +39,48 @@ Each cell is one byte.
 :widths:   2 1 1 1 1 1 1
 
 
--  .. row 1
-
-   -  start + 0:
-
+-  -  start + 0:
-  B\ :sub:`00high`
-
-  G\ :sub:`01high`
+   -  G\ :sub:`01low`\ (bits 7--4)
 
-   -  G\ :sub:`01low`\ (bits 7--4) B\ :sub:`00low`\ (bits 3--0)
-
+  B\ :sub:`00low`\ (bits 3--0)
-  B\ :sub:`02high`
-
-  G\ :sub:`03high`
+   -  G\ :sub:`03low`\ (bits 7--4)
 
-   -  G\ :sub:`03low`\ (bits 7--4) B\ :sub:`02low`\ (bits 3--0)
-
--  .. row 2
-
-   -  start + 6:
+  B\ :sub:`02low`\ (bits 3--0)
 
+-  -  start + 6:
-  G\ :sub:`10high`
-
-  R\ :sub:`11high`
+   -  R\ :sub:`11low`\ (bits 7--4)
 
-   -  R\ :sub:`11low`\ (bits 7--4) G\ :sub:`10low`\ (bits 3--0)
-
+  G\ :sub:`10low`\ (bits 3--0)
-  G\ :sub:`12high`
-
-  R\ :sub:`13high`
+   -  R\ :sub:`13low`\ (bits 3--2)
 
-   -  R\ :sub:`13low`\ (bits 3--2) G\ :sub:`12low`\ (bits 3--0)
-
--  .. row 3
-
-   -  start + 12:
-
+  G\ :sub:`12low`\ (bits 3--0)
+-  -  start + 12:
-  B\ :sub:`20high`
-
-  G\ :sub:`21high`
+   -  G\ :sub:`21low`\ (bits 7--4)
 
-   -  G\ :sub:`21low`\ (bits 7--4) B\ :sub:`20low`\ (bits 3--0)
-
+  B\ :sub:`20low`\ (bits 3--0)
-  B\ :sub:`22high`
-
-  G\ :sub:`23high`
+   -  G\ :sub:`23low`\ (bits 7--4)
 
-   -  G\ :sub:`23low`\ (bits 7--4) B\ :sub:`22low`\ (bits 3--0)
-
--  .. row 4
-
-   -  start + 18:
-
+  B\ :sub:`22low`\ (bits 3--0)
+-  -  start + 18:
-  G\ :sub:`30high`
-
-  R\ :sub:`31high`
+   -  R\ :sub:`31low`\ (bits 7--4)
 
-   -  R\ :sub:`31low`\ (bits 7--4) G\ :sub:`30low`\ (bits 3--0)
-
+  G\ :sub:`30low`\ (bits 3--0)
-  G\ :sub:`32high`
-
-  R\ :sub:`33high`
+   -  R\ :sub:`33low`\ (bits 3--2)
 
-   -  R\ :sub:`33low`\ (bits 3--2) G\ :sub:`32low`\ (bits 3--0)
+  G\ :sub:`32low`\ (bits 3--0)
-- 
2.13.5

Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

2017-09-04 Thread Sakari Ailus 
On Mon, Sep 04, 2017 at 07:37:09PM +0200, Hans Verkuil wrote:
> On 09/04/2017 05:54 PM, Sakari Ailus wrote:
> >>> +/**
> >>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode 
> >>> endpoints in a
> >>> + *   device node
> >>> + * @dev: the device the endpoints of which are to be parsed
> >>> + * @notifier: notifier for @dev
> >>> + * @asd_struct_size: size of the driver's async sub-device struct, 
> >>> including
> >>> + *sizeof(struct v4l2_async_subdev). The 
> >>> + *v4l2_async_subdev shall be the first member of
> >>> + *the driver's async sub-device struct, i.e. both
> >>> + *begin at the same memory address.
> >>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> >>> + *   endpoint. Optional.
> >>> + *   Return: %0 on success
> >>> + *   %-ENOTCONN if the endpoint is to be skipped 
> >>> but this
> >>> + *  should not be considered as an 
> >>> error
> >>> + *   %-EINVAL if the endpoint configuration is 
> >>> invalid
> >>> + *
> >>> + * Parse the fwnode endpoints of the @dev device and populate the async 
> >>> sub-
> >>> + * devices array of the notifier. The @parse_endpoint callback function 
> >>> is
> >>> + * called for each endpoint with the corresponding async sub-device 
> >>> pointer to
> >>> + * let the caller initialize the driver-specific part of the async 
> >>> sub-device
> >>> + * structure.
> >>> + *
> >>> + * The notifier memory shall be zeroed before this function is called on 
> >>> the
> >>> + * notifier.
> >>> + *
> >>> + * This function may not be called on a registered notifier and may be 
> >>> called on
> >>> + * a notifier only once. When using this function, the user may not 
> >>> access the
> >>> + * notifier's subdevs array nor change notifier's num_subdevs field, 
> >>> these are
> >>> + * reserved for the framework's internal use only.
> >>
> >> I don't think the sentence "When using...only" makes any sense. How would 
> >> you
> >> even be able to access any of the notifier fields? You don't have access 
> >> to it
> >> from the parse_endpoint callback.
> > 
> > Not from the parse_endpoint callback. The notifier is first set up by the
> > driver, and this text applies to that --- whether or not parse_endpoint is
> > given.
> 
> What you mean is "After calling this function..." since 
> v4l2_async_notifier_release()
> needs this to release all the memory.

Right, that's a good point.

notifier->subdevs may be allocated by the driver as well, so
v4l2_async_notifier_release() must take that into account.
notifier->max_subdevs should be good for that.

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

[PATCH 6/6] [media] atmel-isi: Adjust a null pointer check in three functions

2017-09-04 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 4 Sep 2017 21:30:48 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring 
---
 drivers/media/platform/atmel/atmel-isi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isi.c 
b/drivers/media/platform/atmel/atmel-isi.c
index ac40defce1e7..f2e13dfb19a1 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -411,7 +411,7 @@ static void buffer_queue(struct vb2_buffer *vb)
spin_lock_irqsave(>irqlock, flags);
list_add_tail(>list, >video_buffer_list);
 
-   if (isi->active == NULL) {
+   if (!isi->active) {
isi->active = buf;
if (vb2_is_streaming(vb->vb2_queue))
start_dma(isi, buf);
@@ -1141,7 +1141,7 @@ static int isi_graph_init(struct atmel_isi *isi)
 
/* Register the subdevices notifier. */
subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
-   if (subdevs == NULL) {
+   if (!subdevs) {
of_node_put(isi->entity.node);
return -ENOMEM;
}
@@ -1200,7 +1200,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
return ret;
 
isi->vdev = video_device_alloc();
-   if (isi->vdev == NULL) {
+   if (!isi->vdev) {
ret = -ENOMEM;
goto err_vdev_alloc;
}
-- 
2.14.1

[PATCH 5/6] [media] atmel-isi: Improve three size determinations

2017-09-04 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 4 Sep 2017 21:27:45 +0200

Replace the specification of data types by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring 
---
 drivers/media/platform/atmel/atmel-isi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isi.c 
b/drivers/media/platform/atmel/atmel-isi.c
index 154e9c39b64f..ac40defce1e7 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1036,13 +1036,13 @@ static int isi_formats_init(struct atmel_isi *isi)
 
isi->num_user_formats = num_fmts;
isi->user_formats = devm_kcalloc(isi->dev,
-num_fmts, sizeof(struct isi_format *),
+num_fmts, sizeof(*isi->user_formats),
 GFP_KERNEL);
if (!isi->user_formats)
return -ENOMEM;
 
memcpy(isi->user_formats, isi_fmts,
-  num_fmts * sizeof(struct isi_format *));
+  num_fmts * sizeof(*isi->user_formats));
isi->current_fmt = isi->user_formats[0];
 
return 0;
@@ -1173,5 +1173,5 @@ static int atmel_isi_probe(struct platform_device *pdev)
struct resource *regs;
int ret, i;
 
-   isi = devm_kzalloc(>dev, sizeof(struct atmel_isi), GFP_KERNEL);
+   isi = devm_kzalloc(>dev, sizeof(*isi), GFP_KERNEL);
if (!isi)
-- 
2.14.1

[PATCH 4/6] [media] atmel-isi: Delete an error message for a failed memory allocation in two functions

2017-09-04 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 4 Sep 2017 21:21:25 +0200

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/media/platform/atmel/atmel-isi.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isi.c 
b/drivers/media/platform/atmel/atmel-isi.c
index 891fa2505efa..154e9c39b64f 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1041,7 +1041,5 @@ static int isi_formats_init(struct atmel_isi *isi)
-   if (!isi->user_formats) {
-   dev_err(isi->dev, "could not allocate memory\n");
+   if (!isi->user_formats)
return -ENOMEM;
-   }
 
memcpy(isi->user_formats, isi_fmts,
   num_fmts * sizeof(struct isi_format *));
@@ -1179,7 +1177,5 @@ static int atmel_isi_probe(struct platform_device *pdev)
-   if (!isi) {
-   dev_err(>dev, "Can't allocate interface!\n");
+   if (!isi)
return -ENOMEM;
-   }
 
isi->pclk = devm_clk_get(>dev, "isi_clk");
if (IS_ERR(isi->pclk))
-- 
2.14.1

[PATCH 3/6] [media] atmel-isc: Adjust three checks for null pointers

2017-09-04 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 4 Sep 2017 20:54:20 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring 
---
 drivers/media/platform/atmel/atmel-isc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index f16bab0105c2..b6048cedb6cc 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1590,7 +1590,7 @@ static int isc_async_complete(struct v4l2_async_notifier 
*notifier)
spin_lock_init(>dma_queue_lock);
 
sd_entity->config = v4l2_subdev_alloc_pad_config(sd_entity->sd);
-   if (sd_entity->config == NULL)
+   if (!sd_entity->config)
return -ENOMEM;
 
ret = isc_formats_init(isc);
@@ -1714,7 +1714,7 @@ static int isc_parse_dt(struct device *dev, struct 
isc_device *isc)
 
subdev_entity = devm_kzalloc(dev,
  sizeof(*subdev_entity), GFP_KERNEL);
-   if (subdev_entity == NULL) {
+   if (!subdev_entity) {
of_node_put(rem);
ret = -ENOMEM;
break;
@@ -1722,7 +1722,7 @@ static int isc_parse_dt(struct device *dev, struct 
isc_device *isc)
 
subdev_entity->asd = devm_kzalloc(dev,
 sizeof(*subdev_entity->asd), GFP_KERNEL);
-   if (subdev_entity->asd == NULL) {
+   if (!subdev_entity->asd) {
of_node_put(rem);
ret = -ENOMEM;
break;
-- 
2.14.1

[PATCH 2/6] [media] atmel-isc: Improve a size determination in isc_formats_init()

2017-09-04 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 4 Sep 2017 20:50:22 +0200

Replace the specification of a data type by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

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

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index f1e635edef72..f16bab0105c2 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1505,6 +1505,6 @@ static int isc_formats_init(struct isc_device *isc)
 
isc->num_user_formats = num_fmts;
isc->user_formats = devm_kcalloc(isc->dev,
-num_fmts, sizeof(struct isc_format *),
+num_fmts, sizeof(*isc->user_formats),
 GFP_KERNEL);
if (!isc->user_formats)
-- 
2.14.1

[PATCH 1/6] [media] atmel-isc: Delete an error message for a failed memory allocation in isc_formats_init()

2017-09-04 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 4 Sep 2017 20:33:58 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/media/platform/atmel/atmel-isc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index d4df3d4ccd85..f1e635edef72 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1510,7 +1510,5 @@ static int isc_formats_init(struct isc_device *isc)
-   if (!isc->user_formats) {
-   v4l2_err(>v4l2_dev, "could not allocate memory\n");
+   if (!isc->user_formats)
return -ENOMEM;
-   }
 
fmt = _formats[0];
for (i = 0, j = 0; i < ARRAY_SIZE(isc_formats); i++) {
-- 
2.14.1

[PATCH 0/6] [media] Atmel: Adjustments for seven function implementations

2017-09-04 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 4 Sep 2017 21:44:55 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (6):
  Delete an error message for a failed memory allocation in isc_formats_init()
  Improve a size determination in isc_formats_init()
  Adjust three checks for null pointers
  Delete an error message for a failed memory allocation in two functions
  Improve three size determinations
  Adjust a null pointer check in three functions

 drivers/media/platform/atmel/atmel-isc.c | 12 +---
 drivers/media/platform/atmel/atmel-isi.c | 20 
 2 files changed, 13 insertions(+), 19 deletions(-)

-- 
2.14.1

Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references

2017-09-04 Thread Hans Verkuil 
On 09/04/2017 06:34 PM, Sakari Ailus wrote:
>>>  MODULE_LICENSE("GPL");
>>>  MODULE_AUTHOR("Sakari Ailus ");
>>>  MODULE_AUTHOR("Sylwester Nawrocki ");
>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
>>> index 6d125f26ec84..52f528162818 100644
>>> --- a/include/media/v4l2-fwnode.h
>>> +++ b/include/media/v4l2-fwnode.h
>>> @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
>>>   struct v4l2_fwnode_endpoint *vep,
>>>   struct v4l2_async_subdev *asd));
>>>  
>>> +/**
>>> + * v4l2_fwnode_reference_parse - parse references for async sub-devices
>>> + * @dev: the device node the properties of which are parsed for references
>>
>> the device node whose properties are...
> 
> Both mean the same thing.

Yes, but I think your sentence is a bit awkward. Just my opinion, though.

Regards,

Hans

Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

2017-09-04 Thread Hans Verkuil 
On 09/04/2017 05:54 PM, Sakari Ailus wrote:
>>> +/**
>>> + * v4l2_async_notifier_parse_fwnode_endpoints - Parse V4L2 fwnode 
>>> endpoints in a
>>> + * device node
>>> + * @dev: the device the endpoints of which are to be parsed
>>> + * @notifier: notifier for @dev
>>> + * @asd_struct_size: size of the driver's async sub-device struct, 
>>> including
>>> + *  sizeof(struct v4l2_async_subdev). The 
>>> + *  v4l2_async_subdev shall be the first member of
>>> + *  the driver's async sub-device struct, i.e. both
>>> + *  begin at the same memory address.
>>> + * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
>>> + * endpoint. Optional.
>>> + * Return: %0 on success
>>> + * %-ENOTCONN if the endpoint is to be skipped but this
>>> + *should not be considered as an error
>>> + * %-EINVAL if the endpoint configuration is invalid
>>> + *
>>> + * Parse the fwnode endpoints of the @dev device and populate the async 
>>> sub-
>>> + * devices array of the notifier. The @parse_endpoint callback function is
>>> + * called for each endpoint with the corresponding async sub-device 
>>> pointer to
>>> + * let the caller initialize the driver-specific part of the async 
>>> sub-device
>>> + * structure.
>>> + *
>>> + * The notifier memory shall be zeroed before this function is called on 
>>> the
>>> + * notifier.
>>> + *
>>> + * This function may not be called on a registered notifier and may be 
>>> called on
>>> + * a notifier only once. When using this function, the user may not access 
>>> the
>>> + * notifier's subdevs array nor change notifier's num_subdevs field, these 
>>> are
>>> + * reserved for the framework's internal use only.
>>
>> I don't think the sentence "When using...only" makes any sense. How would you
>> even be able to access any of the notifier fields? You don't have access to 
>> it
>> from the parse_endpoint callback.
> 
> Not from the parse_endpoint callback. The notifier is first set up by the
> driver, and this text applies to that --- whether or not parse_endpoint is
> given.

What you mean is "After calling this function..." since 
v4l2_async_notifier_release()
needs this to release all the memory.

I'll take another look at this text when I see v8.

Regards,

Hans

> 
>>
>> I think it can just be dropped.
>>
>>> + *
>>> + * The @struct v4l2_fwnode_endpoint passed to the callback function
>>> + * @parse_endpoint is released once the function is finished. If there is 
>>> a need
>>> + * to retain that configuration, the user needs to allocate memory for it.
>>> + *
>>> + * Any notifier populated using this function must be released with a call 
>>> to
>>> + * v4l2_async_notifier_release() after it has been unregistered and the 
>>> async
>>> + * sub-devices are no longer in use.
>>> + *
>>> + * Return: %0 on success, including when no async sub-devices are found
>>> + *%-ENOMEM if memory allocation failed
>>> + *%-EINVAL if graph or endpoint parsing failed
>>> + *Other error codes as returned by @parse_endpoint
>>> + */
>>> +int v4l2_async_notifier_parse_fwnode_endpoints(
>>> +   struct device *dev, struct v4l2_async_notifier *notifier,
>>> +   size_t asd_struct_size,
>>> +   int (*parse_endpoint)(struct device *dev,
>>> + struct v4l2_fwnode_endpoint *vep,
>>> + struct v4l2_async_subdev *asd));
>>> +
>>>  #endif /* _V4L2_FWNODE_H */
>>>
>

Re: [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices

2017-09-04 Thread Sakari Ailus 
Hi Hans,

On Mon, Sep 04, 2017 at 04:56:29PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Registering a notifier has required the knowledge of struct v4l2_device
> > for the reason that sub-devices generally are registered to the
> > v4l2_device (as well as the media device, also available through
> > v4l2_device).
> > 
> > This information is not available for sub-device drivers at probe time.
> > 
> > What this patch does is that it allows registering notifiers without
> > having v4l2_device around. Instead the sub-device pointer is stored to the
> > notifier. Once the sub-device of the driver that registered the notifier
> > is registered, the notifier will gain the knowledge of the v4l2_device,
> > and the binding of async sub-devices from the sub-device driver's notifier
> > may proceed.
> > 
> > The master notifier's complete callback is only called when all sub-device
> > notifiers are completed.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 153 
> > +--
> >  include/media/v4l2-async.h   |  19 -
> >  2 files changed, 146 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 70d02378b48f..55d7886103d2 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -25,6 +25,10 @@
> >  #include 
> >  #include 
> >  
> > +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *sd,
> > + struct v4l2_async_subdev *asd);
> > +
> >  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev 
> > *asd)
> >  {
> >  #if IS_ENABLED(CONFIG_I2C)
> > @@ -101,14 +105,69 @@ static struct v4l2_async_subdev 
> > *v4l2_async_belongs(struct v4l2_async_notifier *
> > return NULL;
> >  }
> >  
> > +static bool v4l2_async_subdev_notifiers_complete(
> > +   struct v4l2_async_notifier *notifier)
> > +{
> > +   struct v4l2_async_notifier *n;
> > +
> > +   list_for_each_entry(n, >notifiers, notifiers) {
> > +   if (!n->master)
> > +   return false;
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +#define notifier_v4l2_dev(n) \
> > +   (!!(n)->v4l2_dev ? (n)->v4l2_dev : \
> > +!!(n)->master ? (n)->master->v4l2_dev : NULL)
> 
> Why '!!'?

I've since replaced this by a function.

My understanding is GCC 7 doesn't like a ? x : y construct where a is a
non-boolean. This will be effectively the same, but a boolean.

See e.g.

commit da48c948c263c9d87dfc64566b3373a858cc8aa2
Author: Arnd Bergmann 
Date:   Wed Jul 19 15:23:27 2017 -0400

media: fix warning on v4l2_subdev_call() result interpreted as bool

v4l2_subdev_call is a macro returning whatever the callback return
type is, usually 'int'. With gcc-7 and ccache, this can lead to
many wanings like:

media/platform/pxa_camera.c: In function 'pxa_mbus_build_fmts_xlate':
media/platform/pxa_camera.c:766:27: error: ?: using integer constants in 
boolean context [-Werror=int-in-bool-context]
  while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, )) {
media/atomisp/pci/atomisp2/atomisp_cmd.c: In function 'atomisp_s_ae_window':
media/atomisp/pci/atomisp2/atomisp_cmd.c:6414:52: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
  if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,

The problem here is that after preprocessing, we the compiler
sees a variation of

if (a ? 0 : 2)

that it thinks is suspicious.

This replaces the ?: operator with an different expression that
does the same thing in a more easily readable way that cannot
tigger the warning

Link: https://lkml.org/lkml/2017/7/14/156

Signed-off-by: Arnd Bergmann 
Signed-off-by: Mauro Carvalho Chehab 

> 
> > +
> > +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(
> > +   struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> > +{
> > +   struct v4l2_async_notifier *n;
> > +
> > +   list_for_each_entry(n, _list, list) {
> > +   if (n->sd == sd)
> > +   return n;
> > +   }
> > +
> > +   return NULL;
> > +}
> > +
> > +static int v4l2_async_notifier_test_all_subdevs(
> > +   struct v4l2_async_notifier *notifier)
> > +{
> > +   struct v4l2_subdev *sd, *tmp;
> > +
> > +   if (!notifier_v4l2_dev(notifier))
> > +   return 0;
> > +
> > +   list_for_each_entry_safe(sd, tmp, _list, async_list) {
> > +   struct v4l2_async_subdev *asd;
> > +   int ret;
> > +
> > +   asd = v4l2_async_belongs(notifier, sd);
> > +   if (!asd)
> > +   continue;
> > +
> > +   ret =

Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references

2017-09-04 Thread Sakari Ailus 
Hi Hans,

On Mon, Sep 04, 2017 at 04:44:48PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Add function v4l2_fwnode_reference_count() for counting external
> > references and v4l2_fwnode_reference_parse() for parsing them as async
> > sub-devices.
> > 
> > This can be done on e.g. flash or lens async sub-devices that are not part
> > of but are associated with a sensor.
> > 
> > struct v4l2_async_notifier.max_subdevs field is added to contain the
> > maximum number of sub-devices in a notifier to reflect the memory
> > allocated for the subdevs array.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 81 
> > +++
> >  include/media/v4l2-fwnode.h   | 28 
> >  2 files changed, 109 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index f8c7a9bc6a58..24f8020caf28 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -449,6 +449,87 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
> >  
> > +static void v4l2_fwnode_print_args(struct fwnode_reference_args *args)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < args->nargs; i++) {
> > +   pr_cont(" %u", args->args[i]);
> > +   if (i + 1 < args->nargs)
> > +   pr_cont(",");
> > +   }
> > +}
> > +
> > +int v4l2_fwnode_reference_parse(
> > +   struct device *dev, struct v4l2_async_notifier *notifier,
> > +   const char *prop, const char *nargs_prop, unsigned int nargs,
> > +   size_t asd_struct_size,
> > +   int (*parse_single)(struct device *dev,
> > +   struct fwnode_reference_args *args,
> > +   struct v4l2_async_subdev *asd))
> > +{
> > +   struct fwnode_reference_args args;
> > +   unsigned int index = 0;
> > +   int ret = -ENOENT;
> > +
> > +   if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > +   return -EINVAL;
> > +
> > +   for (; !fwnode_property_get_reference_args(
> > +dev_fwnode(dev), prop, nargs_prop, nargs,
> > +index, ); index++)
> > +   fwnode_handle_put(args.fwnode);
> > +
> > +   ret = v4l2_async_notifier_realloc(notifier,
> > + notifier->num_subdevs + index);
> > +   if (ret)
> > +   return -ENOMEM;
> > +
> > +   for (ret = -ENOENT, index = 0; !fwnode_property_get_reference_args(
> > +dev_fwnode(dev), prop, nargs_prop, nargs,
> > +index, ); index++) {
> > +   struct v4l2_async_subdev *asd;
> > +
> > +   if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> > +   break;
> 
> As mentioned elsewhere: I think this should return an error.

I'll use -EINVAL and put the fwnode as well, i.e. goto error.

> 
> > +
> > +   asd = kzalloc(asd_struct_size, GFP_KERNEL);
> > +   if (!asd) {
> > +   ret = -ENOMEM;
> > +   goto error;
> > +   }
> > +
> > +   ret = parse_single ? parse_single(dev, , asd) : 0;
> > +   if (ret == -ENOTCONN) {
> > +   dev_dbg(dev,
> > +   "ignoring reference prop \"%s\", nargs_prop 
> > \"%s\", nargs %u, index %u",
> > +   prop, nargs_prop, nargs, index);
> > +   v4l2_fwnode_print_args();
> > +   pr_cont("\n");
> 
> asd isn't freed.

Will fix both.

> 
> > +   continue;
> > +   } else if (ret < 0) {
> > +   dev_warn(dev,
> > +"driver could not parse reference prop \"%s\", 
> > nargs_prop \"%s\", nargs %u, index %u",
> > +prop, nargs_prop, nargs, index);
> > +   v4l2_fwnode_print_args();
> > +   pr_cont("\n");
> 
> Ditto.
> 
> > +   goto error;
> > +   }
> > +
> > +   notifier->subdevs[notifier->num_subdevs] = asd;
> > +   asd->match.fwnode.fwnode = args.fwnode;
> > +   asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +   notifier->num_subdevs++;
> > +   }
> > +
> > +   return 0;
> > +
> > +error:
> > +   fwnode_handle_put(args.fwnode);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
> > +
> >  MODULE_LICENSE("GPL");
> >  MODULE_AUTHOR("Sakari Ailus ");
> >  MODULE_AUTHOR("Sylwester Nawrocki ");
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index 6d125f26ec84..52f528162818 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> >   struct

Re: [PATCH v7 14/18] dt: bindings: Add lens-focus binding for image sensors

2017-09-04 Thread Sakari Ailus 
Hi Hans,

On Mon, Sep 04, 2017 at 04:37:29PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > The lens-focus property contains a phandle to the lens voice coil driver
> > that is associated to the sensor; typically both are contained in the same
> > camera module.
> 
> Just to be certain: this lens-focus phandle should also work fine for a motor
> driver, right?

Yes; the commit message specifies what this is used right now but nothing
prevents using this for different lens control technologies either. The
property itself does not mention voice coil modules.

> 
> We (Cisco) also have a camera that has an iris motor, but since nothing 
> upstream
> uses that I'm not sure if we should bother adding that as well.

Do you have one for focussing only or for zooming as well?

-- 
Regards,

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

Re: [PATCH v7 13/18] dt: bindings: Add a binding for flash devices associated to a sensor

2017-09-04 Thread Sakari Ailus 
On Mon, Sep 04, 2017 at 04:31:51PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Camera flash drivers (and LEDs) are separate from the sensor devices in
> > DT. In order to make an association between the two, provide the
> > association information to the software.
> > 
> > Signed-off-by: Sakari Ailus 
> > Acked-by: Rob Herring 
> > ---
> >  Documentation/devicetree/bindings/media/video-interfaces.txt | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> > b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index 852041a7480c..fee73cf2a714 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -67,6 +67,14 @@ are required in a relevant parent node:
> > identifier, should be 1.
> >   - #size-cells: should be zero.
> >  
> > +
> > +Optional properties
> > +---
> > +
> > +- flash: An array of phandles referring to the flash LED, a sub-node
> > +  of the LED driver device node.
> 
> If it is an array, then I guess it should say: "An array of phandles, each 
> referring to
> a flash LED,"

Sounds good, I'll use that in v8.

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

Re: [PATCH v7 10/18] v4l: async: Introduce macros for calling async ops callbacks

2017-09-04 Thread Sakari Ailus 
Hi Hans,

On Mon, Sep 04, 2017 at 03:50:52PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > Add two macros to call async operations callbacks. Besides simplifying
> > callbacks, this allows async notifiers to have no ops set, i.e. it can be
> > left NULL.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 19 +++
> >  include/media/v4l2-async.h   |  8 
> >  2 files changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 810f5e0273dc..91d04f00b4e4 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -107,16 +107,13 @@ static int v4l2_async_test_notify(struct 
> > v4l2_async_notifier *notifier,
> >  {
> > int ret;
> >  
> > -   if (notifier->ops->bound) {
> > -   ret = notifier->ops->bound(notifier, sd, asd);
> > -   if (ret < 0)
> > -   return ret;
> > -   }
> > +   ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
> 
> Hmm, I think this is rather ugly. We only have three ops, so why not make
> three macros:
> 
>   v4l2_async_notifier_call_bound/unbind/complete?
> 
> Much cleaner than _int_op(...bound...).

Works for me.

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

Re: [PATCH v7 07/18] omap3isp: Fix check for our own sub-devices

2017-09-04 Thread Sakari Ailus 
On Mon, Sep 04, 2017 at 03:28:04PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > We only want to link sub-devices that were bound to the async notifier the
> > isp driver registered but there may be other sub-devices in the
> > v4l2_device as well. Check for the correct async notifier.
> 
> Just to be sure I understand this correctly: the original code wasn't wrong 
> as such,
> but this new test is just more precise.

Well, it would be wrong very soon. :-)

So yes. As long as there's just a single user of the async notifiers in for
a V4L2 device, what used to be there works. The other drivers don't seem to
be affected.

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

Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

2017-09-04 Thread Sakari Ailus 
Hi Hans,

On Mon, Sep 04, 2017 at 03:19:10PM +0200, Hans Verkuil wrote:
> On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> > The current practice is that drivers iterate over their endpoints and
> > parse each endpoint separately. This is very similar in a number of
> > drivers, implement a generic function for the job. Driver specific matters
> > can be taken into account in the driver specific callback.
> > 
> > Signed-off-by: Sakari Ailus 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c  |  16 
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 136 
> > ++
> >  include/media/v4l2-async.h|  24 +-
> >  include/media/v4l2-fwnode.h   |  53 +
> >  4 files changed, 227 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 851f128eba22..6740a241d4a1 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -22,6 +22,7 @@
> >  
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  
> >  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev 
> > *asd)
> > @@ -278,6 +279,21 @@ void v4l2_async_notifier_unregister(struct 
> > v4l2_async_notifier *notifier)
> >  }
> >  EXPORT_SYMBOL(v4l2_async_notifier_unregister);
> >  
> > +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
> > +{
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < notifier->num_subdevs; i++)
> > +   kfree(notifier->subdevs[i]);
> > +
> > +   notifier->max_subdevs = 0;
> > +   notifier->num_subdevs = 0;
> > +
> > +   kvfree(notifier->subdevs);
> > +   notifier->subdevs = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
> > +
> >  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  {
> > struct v4l2_async_notifier *notifier;
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> > b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 706f9e7b90f1..f8c7a9bc6a58 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -19,6 +19,7 @@
> >   */
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -26,6 +27,7 @@
> >  #include 
> >  #include 
> >  
> > +#include 
> >  #include 
> >  
> >  enum v4l2_fwnode_bus_type {
> > @@ -313,6 +315,140 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link 
> > *link)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >  
> > +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier 
> > *notifier,
> > +  unsigned int max_subdevs)
> > +{
> > +   struct v4l2_async_subdev **subdevs;
> > +
> > +   if (max_subdevs <= notifier->max_subdevs)
> > +   return 0;
> > +
> > +   subdevs = kvmalloc_array(
> > +   max_subdevs, sizeof(*notifier->subdevs),
> > +   GFP_KERNEL | __GFP_ZERO);
> > +   if (!subdevs)
> > +   return -ENOMEM;
> > +
> > +   if (notifier->subdevs) {
> > +   memcpy(subdevs, notifier->subdevs,
> > +  sizeof(*subdevs) * notifier->num_subdevs);
> > +
> > +   kvfree(notifier->subdevs);
> > +   }
> > +
> > +   notifier->subdevs = subdevs;
> > +   notifier->max_subdevs = max_subdevs;
> > +
> > +   return 0;
> > +}
> > +
> > +static int v4l2_async_notifier_fwnode_parse_endpoint(
> > +   struct device *dev, struct v4l2_async_notifier *notifier,
> > +   struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> > +   int (*parse_endpoint)(struct device *dev,
> > +   struct v4l2_fwnode_endpoint *vep,
> > +   struct v4l2_async_subdev *asd))
> > +{
> > +   struct v4l2_async_subdev *asd;
> > +   struct v4l2_fwnode_endpoint *vep;
> > +   int ret = 0;
> > +
> > +   asd = kzalloc(asd_struct_size, GFP_KERNEL);
> > +   if (!asd)
> > +   return -ENOMEM;
> > +
> > +   asd->match.fwnode.fwnode =
> > +   fwnode_graph_get_remote_port_parent(endpoint);
> > +   if (!asd->match.fwnode.fwnode) {
> > +   dev_warn(dev, "bad remote port parent\n");
> > +   ret = -EINVAL;
> > +   goto out_err;
> > +   }
> > +
> > +   /* Ignore endpoints the parsing of which failed. */
> > +   vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> > +   if (IS_ERR(vep)) {
> > +   ret = PTR_ERR(vep);
> > +   dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> > +ret);
> > +   goto out_err;
> > +   }
> > +
> > +   ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;
> 
> How likely is it that parse_endpoint will be NULL? I ask because if it is
> common, then you could put everything from v4l2_fwnode_endpoint_alloc_parse
> until just before 'asd->match_type = V4L2_ASYNC_MATCH_FWNODE;' under
> 'if (parse_endpoint) {'.
> 
> The disadvantage is that you won't see parse errors, but on the other hand
> nobody

Re: [PATCH] dma-fence: fix dma_fence_get_rcu_safe

2017-09-04 Thread Christian König 
I really wonder what's wrong with my mail client, but it looks like this 
patch never made it at least to dri-devel.


Forwarding manually now,
Christian.

Am 04.09.2017 um 15:16 schrieb Christian König:

From: Christian König 

The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
acquire a reference it doesn't necessary mean that there is no fence at all.

It usually mean that the fence was replaced by a new one and in this situation
we certainly want to have the new one as result and *NOT* NULL.

Signed-off-by: Christian König 
Cc: Chris Wilson 
Cc: Daniel Vetter 
Cc: Sumit Semwal 
Cc: linux-media@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org
---
  include/linux/dma-fence.h | 23 ++-
  1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index a5195a7..37f3d67 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -246,27 +246,8 @@ dma_fence_get_rcu_safe(struct dma_fence * __rcu *fencep)
struct dma_fence *fence;
  
  		fence = rcu_dereference(*fencep);

-   if (!fence || !dma_fence_get_rcu(fence))
-   return NULL;
-
-   /* The atomic_inc_not_zero() inside dma_fence_get_rcu()
-* provides a full memory barrier upon success (such as now).
-* This is paired with the write barrier from assigning
-* to the __rcu protected fence pointer so that if that
-* pointer still matches the current fence, we know we
-* have successfully acquire a reference to it. If it no
-* longer matches, we are holding a reference to some other
-* reallocated pointer. This is possible if the allocator
-* is using a freelist like SLAB_TYPESAFE_BY_RCU where the
-* fence remains valid for the RCU grace period, but it
-* may be reallocated. When using such allocators, we are
-* responsible for ensuring the reference we get is to
-* the right fence, as below.
-*/
-   if (fence == rcu_access_pointer(*fencep))
-   return rcu_pointer_handoff(fence);
-
-   dma_fence_put(fence);
+   if (!fence || dma_fence_get_rcu(fence))
+   return fence;
} while (1);
  }

Re: [PATCH v7 01/18] v4l: fwnode: Move KernelDoc documentation to the header

2017-09-04 Thread Pavel Machek 
On Sun 2017-09-03 20:49:41, Sakari Ailus wrote:
> In V4L2 the practice is to have the KernelDoc documentation in the header
> and not in .c source code files. This consequientally makes the V4L2

consequientally: spelling?

> fwnode function documentation part of the Media documentation build.
> 
> Also correct the link related function and argument naming in
> documentation.
> 
> Signed-off-by: Sakari Ailus 
> Reviewed-by: Niklas Söderlund 

Something funny going on with utf-8 here.

Acked-by: Pavel Machek 
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 v2 14/14] [media] v4l: Document explicit synchronization behaviour

2017-09-04 Thread Gustavo Padovan 
2017-09-02 Hans Verkuil :

> On 09/01/2017 08:21 PM, Gustavo Padovan wrote:
> > Hi Hans,
> > 
> > 2017-09-01 Hans Verkuil :
> > 
> >> Hi Gustavo,
> >>
> >> I think I concentrate on this last patch first. Once I fully understand 
> >> this
> >> I can review the code. Remember, it's been a while for me since I last 
> >> looked
> >> at fences, so forgive me if I ask stupid questions. On the other hand, 
> >> others
> >> with a similar lack of understanding of fences probably have similar 
> >> questions,
> >> so it is a good indication where the documentation needs improvement :-)
> > 
> > Please ask as many as you want, those are the best questions. :)
> > 
> >>
> >> On 01/09/17 03:50, Gustavo Padovan wrote:
> >>> From: Gustavo Padovan 
> >>>
> >>> Add section to VIDIOC_QBUF about it
> >>>
> >>> Signed-off-by: Gustavo Padovan 
> >>> ---q
> >>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 30 
> >>> 
> >>>  1 file changed, 30 insertions(+)
> >>>
> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst 
> >>> b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>> index 1f3612637200..6bd960d3972b 100644
> >>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>> @@ -117,6 +117,36 @@ immediately with an ``EAGAIN`` error code when no 
> >>> buffer is available.
> >>>  The struct :c:type:`v4l2_buffer` structure is specified in
> >>>  :ref:`buffer`.
> >>>  
> >>> +Explicit Synchronization
> >>> +
> >>> +
> >>> +Explicit Synchronization allows us to control the synchronization of
> >>> +shared buffers from userspace by passing fences to the kernel and/or
> >>> +receiving them from it. Fences passed to the kernel are named in-fences 
> >>> and
> >>> +the kernel should wait them to signal before using the buffer, i.e., 
> >>> queueing
> >>> +it to the driver. On the other side, the kernel can create out-fences 
> >>> for the
> >>> +buffers it queues to the drivers, out-fences signal when the driver is
> >>> +finished with buffer, that is the buffer is ready.
> >>
> >> This should add a line explaining that fences are represented by file 
> >> descriptors.
> > 
> > Okay.
> > 
> >>
> >>> +
> >>> +The in-fences and out-fences are communicated at the ``VIDIOC_QBUF`` 
> >>> ioctl
> >>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` and ``V4L2_BUF_FLAG_OUT_FENCE`` 
> >>> buffer
> >>> +flags and the `fence_fd` field. If an in-fence needs to be passed to the 
> >>> kernel,
> >>> +`fence_fd` should be set to the fence file descriptor number and the
> >>> +``V4L2_BUF_FLAG_IN_FENCE`` should be set as well.
> >>
> >> Is it possible to have both flags at the same time? Or are they mutually 
> >> exclusive?
> >>
> >> If only V4L2_BUF_FLAG_IN_FENCE is set, then what is the value of fence_fd 
> >> after
> >> the QBUF call? -1?
> > 
> > Yes, it is -1.
> > 
> >>
> >>> +
> >>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag 
> >>> should
> >>> +be set and the `fence_fd` field will be returned with the out-fence file
> >>> +descriptor related to the next buffer to be queued internally to the V4L2
> >>> +driver. That means the out-fence may not be associated with the buffer 
> >>> in the
> >>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which 
> >>> videobuf2 core
> >>> +queues the buffers to the drivers can't be guaranteed. To become aware 
> >>> of the
> >>> +buffer with which the out-fence will be attached the 
> >>> ``V4L2_EVENT_BUF_QUEUED``
> >>> +should be used. It will trigger an event for every buffer queued to the 
> >>> V4L2
> >>> +driver.
> >>
> >> General question: does it even make sense to use an out-fence if you don't 
> >> know with
> >> what buffer is it associated? I mean, QBUF returns an out fence, but only 
> >> some
> >> time later are you informed about a buffer being queued. It also looks 
> >> like userspace
> >> has to keep track of the issued out-fences and the queued buffers and map 
> >> them
> >> accordingly.
> >>
> >> If the out-fence cannot be used until you know the buffer as well, then 
> >> wouldn't
> >> it make more sense if the out-fence and the buffer index are both sent by 
> >> the
> >> event? Of course, in that case the event can only be sent to the owner 
> >> file handle
> >> of the streaming device node, but that's OK, we have that.
> >>
> >> Setting the OUT_FENCE flag will just cause this event to be sent whenever 
> >> that
> >> buffer is queued internally.
> >>
> >> Sorry if this just shows a complete lack of understanding of fences on my 
> >> side,
> >> that's perfectly possible.
> > 
> > Right, I can not think of anything that prevents what you are saying to
> > work. I built it this way initially because on my lack of understanding
> > of V4L2 (seems we complement each other here :) because I thought we
> > needed to keep the QBUF ordering.

Re: [PATCH v7 16/18] v4l2-fwnode: Add convenience function for parsing common external refs

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Add v4l2_fwnode_parse_reference_sensor_common for parsing common
> sensor properties that refer to adjacent devices such as flash or lens
> driver chips.
> 
> As this is an association only, there's little a regular driver needs to
> know about these devices as such.
> 
> Signed-off-by: Sakari Ailus 

Acked-by: Hans Verkuil 

Regards,

Hans

> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 27 +++
>  include/media/v4l2-fwnode.h   |  3 +++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 24f8020caf28..f28be3b751d3 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -530,6 +530,33 @@ int v4l2_fwnode_reference_parse(
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
>  
> +int v4l2_fwnode_reference_parse_sensor_common(
> + struct device *dev, struct v4l2_async_notifier *notifier)
> +{
> + static const struct {
> + char *name;
> + char *cells;
> + unsigned int nr_cells;
> + } props[] = {
> + { "flash", NULL, 0 },
> + { "lens-focus", NULL, 0 },
> + };
> + unsigned int i;
> + int rval;
> +
> + for (i = 0; i < ARRAY_SIZE(props); i++) {
> + rval = v4l2_fwnode_reference_parse(
> + dev, notifier, props[i].name, props[i].cells,
> + props[i].nr_cells, sizeof(struct v4l2_async_subdev),
> + NULL);
> + if (rval < 0 && rval != -ENOENT)
> + return rval;
> + }
> +
> + return rval;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse_sensor_common);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus ");
>  MODULE_AUTHOR("Sylwester Nawrocki ");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 52f528162818..fd14f1d38966 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -282,4 +282,7 @@ int v4l2_fwnode_reference_parse(
>   struct fwnode_reference_args *args,
>   struct v4l2_async_subdev *asd));
>  
> +int v4l2_fwnode_reference_parse_sensor_common(
> + struct device *dev, struct v4l2_async_notifier *notifier);
> +
>  #endif /* _V4L2_FWNODE_H */
>

Re: [PATCH v7 12/18] v4l: async: Allow binding notifiers to sub-devices

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Registering a notifier has required the knowledge of struct v4l2_device
> for the reason that sub-devices generally are registered to the
> v4l2_device (as well as the media device, also available through
> v4l2_device).
> 
> This information is not available for sub-device drivers at probe time.
> 
> What this patch does is that it allows registering notifiers without
> having v4l2_device around. Instead the sub-device pointer is stored to the
> notifier. Once the sub-device of the driver that registered the notifier
> is registered, the notifier will gain the knowledge of the v4l2_device,
> and the binding of async sub-devices from the sub-device driver's notifier
> may proceed.
> 
> The master notifier's complete callback is only called when all sub-device
> notifiers are completed.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 153 
> +--
>  include/media/v4l2-async.h   |  19 -
>  2 files changed, 146 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 70d02378b48f..55d7886103d2 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -25,6 +25,10 @@
>  #include 
>  #include 
>  
> +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> +   struct v4l2_subdev *sd,
> +   struct v4l2_async_subdev *asd);
> +
>  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>  {
>  #if IS_ENABLED(CONFIG_I2C)
> @@ -101,14 +105,69 @@ static struct v4l2_async_subdev 
> *v4l2_async_belongs(struct v4l2_async_notifier *
>   return NULL;
>  }
>  
> +static bool v4l2_async_subdev_notifiers_complete(
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_async_notifier *n;
> +
> + list_for_each_entry(n, >notifiers, notifiers) {
> + if (!n->master)
> + return false;
> + }
> +
> + return true;
> +}
> +
> +#define notifier_v4l2_dev(n) \
> + (!!(n)->v4l2_dev ? (n)->v4l2_dev : \
> +  !!(n)->master ? (n)->master->v4l2_dev : NULL)

Why '!!'?

> +
> +static struct v4l2_async_notifier *v4l2_async_get_subdev_notifier(
> + struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd)
> +{
> + struct v4l2_async_notifier *n;
> +
> + list_for_each_entry(n, _list, list) {
> + if (n->sd == sd)
> + return n;
> + }
> +
> + return NULL;
> +}
> +
> +static int v4l2_async_notifier_test_all_subdevs(
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *sd, *tmp;
> +
> + if (!notifier_v4l2_dev(notifier))
> + return 0;
> +
> + list_for_each_entry_safe(sd, tmp, _list, async_list) {
> + struct v4l2_async_subdev *asd;
> + int ret;
> +
> + asd = v4l2_async_belongs(notifier, sd);
> + if (!asd)
> + continue;
> +
> + ret = v4l2_async_test_notify(notifier, sd, asd);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
>  static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> struct v4l2_subdev *sd,
> struct v4l2_async_subdev *asd)

A general note (not specific to this patch series): I really don't like
this function name. v4l2_async_match_notify is a much better name.

With 'test' I get association with 'testing something' and not that it is
a match.

I have a similar problem with v4l2_async_belongs: v4l2_async_find_match
makes a lot more sense.

>  {
> + struct v4l2_async_notifier *subdev_notifier;
>   int ret;
>  
> - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> - if (ret < 0)
> + ret = v4l2_device_register_subdev(notifier_v4l2_dev(notifier), sd);
> + if (ret)
>   return ret;
>  
>   ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);
> @@ -125,8 +184,26 @@ static int v4l2_async_test_notify(struct 
> v4l2_async_notifier *notifier,
>   /* Move from the global subdevice list to notifier's done */
>   list_move(>async_list, >done);
>  
> - if (list_empty(>waiting) && notifier->ops->complete)
> - return v4l2_async_notifier_call_int_op(notifier, complete);
> + subdev_notifier = v4l2_async_get_subdev_notifier(notifier, sd);
> + if (subdev_notifier && !subdev_notifier->master) {
> + subdev_notifier->master = notifier;
> + list_add(_notifier->notifiers, >notifiers);
> + ret = v4l2_async_notifier_test_all_subdevs(subdev_notifier);
> + if (ret)
> + return ret;
> + }
> +
> + if (list_empty(>waiting) &&
> +

Re: [PATCH v7 15/18] v4l2-fwnode: Add convenience function for parsing generic references

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Add function v4l2_fwnode_reference_count() for counting external
> references and v4l2_fwnode_reference_parse() for parsing them as async
> sub-devices.
> 
> This can be done on e.g. flash or lens async sub-devices that are not part
> of but are associated with a sensor.
> 
> struct v4l2_async_notifier.max_subdevs field is added to contain the
> maximum number of sub-devices in a notifier to reflect the memory
> allocated for the subdevs array.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 81 
> +++
>  include/media/v4l2-fwnode.h   | 28 
>  2 files changed, 109 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index f8c7a9bc6a58..24f8020caf28 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -449,6 +449,87 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
>  }
>  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
>  
> +static void v4l2_fwnode_print_args(struct fwnode_reference_args *args)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < args->nargs; i++) {
> + pr_cont(" %u", args->args[i]);
> + if (i + 1 < args->nargs)
> + pr_cont(",");
> + }
> +}
> +
> +int v4l2_fwnode_reference_parse(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + const char *prop, const char *nargs_prop, unsigned int nargs,
> + size_t asd_struct_size,
> + int (*parse_single)(struct device *dev,
> + struct fwnode_reference_args *args,
> + struct v4l2_async_subdev *asd))
> +{
> + struct fwnode_reference_args args;
> + unsigned int index = 0;
> + int ret = -ENOENT;
> +
> + if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> + return -EINVAL;
> +
> + for (; !fwnode_property_get_reference_args(
> +  dev_fwnode(dev), prop, nargs_prop, nargs,
> +  index, ); index++)
> + fwnode_handle_put(args.fwnode);
> +
> + ret = v4l2_async_notifier_realloc(notifier,
> +   notifier->num_subdevs + index);
> + if (ret)
> + return -ENOMEM;
> +
> + for (ret = -ENOENT, index = 0; !fwnode_property_get_reference_args(
> +  dev_fwnode(dev), prop, nargs_prop, nargs,
> +  index, ); index++) {
> + struct v4l2_async_subdev *asd;
> +
> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> + break;

As mentioned elsewhere: I think this should return an error.

> +
> + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> + if (!asd) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + ret = parse_single ? parse_single(dev, , asd) : 0;
> + if (ret == -ENOTCONN) {
> + dev_dbg(dev,
> + "ignoring reference prop \"%s\", nargs_prop 
> \"%s\", nargs %u, index %u",
> + prop, nargs_prop, nargs, index);
> + v4l2_fwnode_print_args();
> + pr_cont("\n");

asd isn't freed.

> + continue;
> + } else if (ret < 0) {
> + dev_warn(dev,
> +  "driver could not parse reference prop \"%s\", 
> nargs_prop \"%s\", nargs %u, index %u",
> +  prop, nargs_prop, nargs, index);
> + v4l2_fwnode_print_args();
> + pr_cont("\n");

Ditto.

> + goto error;
> + }
> +
> + notifier->subdevs[notifier->num_subdevs] = asd;
> + asd->match.fwnode.fwnode = args.fwnode;
> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> + notifier->num_subdevs++;
> + }
> +
> + return 0;
> +
> +error:
> + fwnode_handle_put(args.fwnode);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_reference_parse);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus ");
>  MODULE_AUTHOR("Sylwester Nawrocki ");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 6d125f26ec84..52f528162818 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -254,4 +254,32 @@ int v4l2_async_notifier_parse_fwnode_endpoints(
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd));
>  
> +/**
> + * v4l2_fwnode_reference_parse - parse references for async sub-devices
> + * @dev: the device node the properties of which are parsed for references

the device node whose properties are...

Re: [PATCH v7 14/18] dt: bindings: Add lens-focus binding for image sensors

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> The lens-focus property contains a phandle to the lens voice coil driver
> that is associated to the sensor; typically both are contained in the same
> camera module.

Just to be certain: this lens-focus phandle should also work fine for a motor
driver, right?

We (Cisco) also have a camera that has an iris motor, but since nothing upstream
uses that I'm not sure if we should bother adding that as well.

Regards,

Hans

> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Pavel Machek 
> Reviewed-by: Sebastian Reichel 
> Acked-by: Rob Herring 
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index fee73cf2a714..73d045127dcf 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -74,6 +74,8 @@ Optional properties
>  - flash: An array of phandles referring to the flash LED, a sub-node
>of the LED driver device node.
>  
> +- lens-focus: A phandle to the node of the focus lens controller.
> +
>  
>  Optional endpoint properties
>  
>

Re: [PATCH v7 13/18] dt: bindings: Add a binding for flash devices associated to a sensor

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Camera flash drivers (and LEDs) are separate from the sensor devices in
> DT. In order to make an association between the two, provide the
> association information to the software.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Rob Herring 
> ---
>  Documentation/devicetree/bindings/media/video-interfaces.txt | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt 
> b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index 852041a7480c..fee73cf2a714 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -67,6 +67,14 @@ are required in a relevant parent node:
>   identifier, should be 1.
>   - #size-cells: should be zero.
>  
> +
> +Optional properties
> +---
> +
> +- flash: An array of phandles referring to the flash LED, a sub-node
> +  of the LED driver device node.

If it is an array, then I guess it should say: "An array of phandles, each 
referring to
a flash LED,"

Regards,

Hans

> +
> +
>  Optional endpoint properties
>  
>  
>

Re: [PATCH v7 10/18] v4l: async: Introduce macros for calling async ops callbacks

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> Add two macros to call async operations callbacks. Besides simplifying
> callbacks, this allows async notifiers to have no ops set, i.e. it can be
> left NULL.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 19 +++
>  include/media/v4l2-async.h   |  8 
>  2 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 810f5e0273dc..91d04f00b4e4 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -107,16 +107,13 @@ static int v4l2_async_test_notify(struct 
> v4l2_async_notifier *notifier,
>  {
>   int ret;
>  
> - if (notifier->ops->bound) {
> - ret = notifier->ops->bound(notifier, sd, asd);
> - if (ret < 0)
> - return ret;
> - }
> + ret = v4l2_async_notifier_call_int_op(notifier, bound, sd, asd);

Hmm, I think this is rather ugly. We only have three ops, so why not make
three macros:

v4l2_async_notifier_call_bound/unbind/complete?

Much cleaner than _int_op(...bound...).

Regards,

Hans

> + if (ret < 0)
> + return ret;
>  
>   ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
>   if (ret < 0) {
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, asd);
> + v4l2_async_notifier_call_void_op(notifier, unbind, sd, asd);
>   return ret;
>   }
>  
> @@ -129,7 +126,7 @@ static int v4l2_async_test_notify(struct 
> v4l2_async_notifier *notifier,
>   list_move(>async_list, >done);
>  
>   if (list_empty(>waiting) && notifier->ops->complete)
> - return notifier->ops->complete(notifier);
> + return v4l2_async_notifier_call_int_op(notifier, complete);
>  
>   return 0;
>  }
> @@ -232,8 +229,7 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
>   /* If we handled USB devices, we'd have to lock the parent too 
> */
>   device_release_driver(d);
>  
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_void_op(notifier, unbind, sd, sd->asd);
>  
>   /*
>* Store device at the device cache, in order to call
> @@ -344,8 +340,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>  
>   v4l2_async_cleanup(sd);
>  
> - if (notifier->ops->unbind)
> - notifier->ops->unbind(notifier, sd, sd->asd);
> + v4l2_async_notifier_call_void_op(notifier, unbind, sd, sd->asd);
>  
>   mutex_unlock(_lock);
>  }
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 3c48f8b66d12..c3e001e0d1f1 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -95,6 +95,14 @@ struct v4l2_async_notifier_operations {
>  struct v4l2_async_subdev *asd);
>  };
>  
> +#define v4l2_async_notifier_call_int_op(n, op, ...)  \
> + (((n)->ops && (n)->ops->op) ? (n)->ops->op(n, ## __VA_ARGS__) : 0)
> +#define v4l2_async_notifier_call_void_op(n, op, ...)  \
> + do { \
> + if ((n)->ops && (n)->ops->op)\
> + (n)->ops->op(n, ## __VA_ARGS__); \
> + } while (false)
> +
>  /**
>   * struct v4l2_async_notifier - v4l2_device notifier data
>   *
>

Re: [PATCH v7 09/18] v4l: async: Move async subdev notifier operations to a separate structure

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> From: Laurent Pinchart 
> 
> The async subdev notifier .bound(), .unbind() and .complete() operations
> are function pointers stored directly in the v4l2_async_subdev
> structure. As the structure isn't immutable, this creates a potential
> security risk as the function pointers are mutable.
> 
> To fix this, move the function pointers to a new
> v4l2_async_subdev_operations structure that can be made const in
> drivers.
> 
> Signed-off-by: Laurent Pinchart 

Acked-by: Hans Verkuil 

Regards,

Hans

> ---
>  drivers/media/platform/am437x/am437x-vpfe.c|  8 +--
>  drivers/media/platform/atmel/atmel-isc.c   | 10 ++---
>  drivers/media/platform/atmel/atmel-isi.c   | 10 ++---
>  drivers/media/platform/davinci/vpif_capture.c  |  8 +--
>  drivers/media/platform/davinci/vpif_display.c  |  8 +--
>  drivers/media/platform/exynos4-is/media-dev.c  |  8 +--
>  drivers/media/platform/omap3isp/isp.c  |  6 +-
>  drivers/media/platform/pxa_camera.c|  8 +--
>  drivers/media/platform/qcom/camss-8x16/camss.c |  8 +--
>  drivers/media/platform/rcar-vin/rcar-core.c| 10 ++---
>  drivers/media/platform/rcar_drif.c | 10 ++---
>  drivers/media/platform/soc_camera/soc_camera.c | 14 +++--
>  drivers/media/platform/stm32/stm32-dcmi.c  | 10 ++---
>  drivers/media/platform/ti-vpe/cal.c|  8 +--
>  drivers/media/platform/xilinx/xilinx-vipp.c|  8 +--
>  drivers/media/v4l2-core/v4l2-async.c   | 20 +-
>  drivers/staging/media/imx/imx-media-dev.c  |  8 +--
>  include/media/v4l2-async.h | 29 
> +-
>  18 files changed, 131 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c 
> b/drivers/media/platform/am437x/am437x-vpfe.c
> index dfcc484cab89..0997c640191d 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2417,6 +2417,11 @@ static int vpfe_async_complete(struct 
> v4l2_async_notifier *notifier)
>   return vpfe_probe_complete(vpfe);
>  }
>  
> +static const struct v4l2_async_notifier_operations vpfe_async_ops = {
> + .bound = vpfe_async_bound,
> + .complete = vpfe_async_complete,
> +};
> +
>  static struct vpfe_config *
>  vpfe_get_pdata(struct platform_device *pdev)
>  {
> @@ -2590,8 +2595,7 @@ static int vpfe_probe(struct platform_device *pdev)
>  
>   vpfe->notifier.subdevs = vpfe->cfg->asd;
>   vpfe->notifier.num_subdevs = ARRAY_SIZE(vpfe->cfg->asd);
> - vpfe->notifier.bound = vpfe_async_bound;
> - vpfe->notifier.complete = vpfe_async_complete;
> + vpfe->notifier.ops = _async_ops;
>   ret = v4l2_async_notifier_register(>v4l2_dev,
>   >notifier);
>   if (ret) {
> diff --git a/drivers/media/platform/atmel/atmel-isc.c 
> b/drivers/media/platform/atmel/atmel-isc.c
> index d7103c5f92c3..48544c4137cb 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -1639,6 +1639,12 @@ static int isc_async_complete(struct 
> v4l2_async_notifier *notifier)
>   return 0;
>  }
>  
> +static const struct v4l2_async_notifier_operations isc_async_ops = {
> + .bound = isc_async_bound,
> + .unbind = isc_async_unbind,
> + .complete = isc_async_complete,
> +};
> +
>  static void isc_subdev_cleanup(struct isc_device *isc)
>  {
>   struct isc_subdev_entity *subdev_entity;
> @@ -1851,9 +1857,7 @@ static int atmel_isc_probe(struct platform_device *pdev)
>   list_for_each_entry(subdev_entity, >subdev_entities, list) {
>   subdev_entity->notifier.subdevs = _entity->asd;
>   subdev_entity->notifier.num_subdevs = 1;
> - subdev_entity->notifier.bound = isc_async_bound;
> - subdev_entity->notifier.unbind = isc_async_unbind;
> - subdev_entity->notifier.complete = isc_async_complete;
> + subdev_entity->notifier.ops = _async_ops;
>  
>   ret = v4l2_async_notifier_register(>v4l2_dev,
>  _entity->notifier);
> diff --git a/drivers/media/platform/atmel/atmel-isi.c 
> b/drivers/media/platform/atmel/atmel-isi.c
> index 891fa2505efa..eadbf9def358 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -1105,6 +1105,12 @@ static int isi_graph_notify_bound(struct 
> v4l2_async_notifier *notifier,
>   return 0;
>  }
>  
> +static const struct v4l2_async_notifier_operations isi_graph_notify_ops = {
> + .bound = isi_graph_notify_bound,
> + .unbind = isi_graph_notify_unbind,
> + .complete = isi_graph_notify_complete,
> +};
> +
>  static int isi_graph_parse(struct atmel_isi *isi,

Re: [PATCH] dma-fence: fix dma_fence_get_rcu_safe

2017-09-04 Thread Chris Wilson 
Quoting Christian König (2017-09-04 14:27:33)
> From: Christian König 
> 
> The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
> acquire a reference it doesn't necessary mean that there is no fence at all.
> 
> It usually mean that the fence was replaced by a new one and in this situation
> we certainly want to have the new one as result and *NOT* NULL.

Which is not guaranteed by the code you wrote either.

The point of the comment is that the mb is only inside the successful
kref_atomic_inc_unless_zero, and that only after that mb do you know
whether or not you have the current fence.

You can argue that you want to replace the
if (!dma_fence_get_rcu())
return NULL
with
if (!dma_fence_get_rcu()
continue;
but it would be incorrect to say that by simply ignoring the
post-condition check that you do have the right fence.
-Chris

Re: [PATCH v7 07/18] omap3isp: Fix check for our own sub-devices

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> We only want to link sub-devices that were bound to the async notifier the
> isp driver registered but there may be other sub-devices in the
> v4l2_device as well. Check for the correct async notifier.

Just to be sure I understand this correctly: the original code wasn't wrong as 
such,
but this new test is just more precise.

Right?

Hans

> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/platform/omap3isp/isp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index a546cf774d40..3b1a9cd0e591 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2155,7 +2155,7 @@ static int isp_subdev_notifier_complete(struct 
> v4l2_async_notifier *async)
>   return ret;
>  
>   list_for_each_entry(sd, _dev->subdevs, list) {
> - if (!sd->asd)
> + if (sd->notifier != >notifier)
>   continue;
>  
>   ret = isp_link_entity(isp, >entity,
>

[PATCH] dma-fence: fix dma_fence_get_rcu_safe

2017-09-04 Thread Christian König 
From: Christian König 

The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
acquire a reference it doesn't necessary mean that there is no fence at all.

It usually mean that the fence was replaced by a new one and in this situation
we certainly want to have the new one as result and *NOT* NULL.

Signed-off-by: Christian König 
Cc: Chris Wilson 
Cc: Daniel Vetter 
Cc: Sumit Semwal 
Cc: linux-media@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org
---
 include/linux/dma-fence.h | 23 ++-
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index a5195a7..37f3d67 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -246,27 +246,8 @@ dma_fence_get_rcu_safe(struct dma_fence * __rcu *fencep)
struct dma_fence *fence;
 
fence = rcu_dereference(*fencep);
-   if (!fence || !dma_fence_get_rcu(fence))
-   return NULL;
-
-   /* The atomic_inc_not_zero() inside dma_fence_get_rcu()
-* provides a full memory barrier upon success (such as now).
-* This is paired with the write barrier from assigning
-* to the __rcu protected fence pointer so that if that
-* pointer still matches the current fence, we know we
-* have successfully acquire a reference to it. If it no
-* longer matches, we are holding a reference to some other
-* reallocated pointer. This is possible if the allocator
-* is using a freelist like SLAB_TYPESAFE_BY_RCU where the
-* fence remains valid for the RCU grace period, but it
-* may be reallocated. When using such allocators, we are
-* responsible for ensuring the reference we get is to
-* the right fence, as below.
-*/
-   if (fence == rcu_access_pointer(*fencep))
-   return rcu_pointer_handoff(fence);
-
-   dma_fence_put(fence);
+   if (!fence || dma_fence_get_rcu(fence))
+   return fence;
} while (1);
 }
 
-- 
2.7.4

[PATCH] dma-fence: fix dma_fence_get_rcu_safe

2017-09-04 Thread Christian König 
From: Christian König 

The logic is buggy and unnecessary complex. When dma_fence_get_rcu() fails to
acquire a reference it doesn't necessary mean that there is no fence at all.

It usually mean that the fence was replaced by a new one and in this situation
we certainly want to have the new one as result and *NOT* NULL.

Signed-off-by: Christian König 
Cc: Chris Wilson 
Cc: Daniel Vetter 
Cc: Sumit Semwal 
Cc: linux-media@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: linaro-mm-...@lists.linaro.org
---
 include/linux/dma-fence.h | 23 ++-
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index a5195a7..37f3d67 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -246,27 +246,8 @@ dma_fence_get_rcu_safe(struct dma_fence * __rcu *fencep)
struct dma_fence *fence;
 
fence = rcu_dereference(*fencep);
-   if (!fence || !dma_fence_get_rcu(fence))
-   return NULL;
-
-   /* The atomic_inc_not_zero() inside dma_fence_get_rcu()
-* provides a full memory barrier upon success (such as now).
-* This is paired with the write barrier from assigning
-* to the __rcu protected fence pointer so that if that
-* pointer still matches the current fence, we know we
-* have successfully acquire a reference to it. If it no
-* longer matches, we are holding a reference to some other
-* reallocated pointer. This is possible if the allocator
-* is using a freelist like SLAB_TYPESAFE_BY_RCU where the
-* fence remains valid for the RCU grace period, but it
-* may be reallocated. When using such allocators, we are
-* responsible for ensuring the reference we get is to
-* the right fence, as below.
-*/
-   if (fence == rcu_access_pointer(*fencep))
-   return rcu_pointer_handoff(fence);
-
-   dma_fence_put(fence);
+   if (!fence || dma_fence_get_rcu(fence))
+   return fence;
} while (1);
 }
 
-- 
2.7.4

Re: [PATCH v7 04/18] v4l: fwnode: Support generic parsing of graph endpoints in a device

2017-09-04 Thread Hans Verkuil 
On 09/03/2017 07:49 PM, Sakari Ailus wrote:
> The current practice is that drivers iterate over their endpoints and
> parse each endpoint separately. This is very similar in a number of
> drivers, implement a generic function for the job. Driver specific matters
> can be taken into account in the driver specific callback.
> 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/v4l2-core/v4l2-async.c  |  16 
>  drivers/media/v4l2-core/v4l2-fwnode.c | 136 
> ++
>  include/media/v4l2-async.h|  24 +-
>  include/media/v4l2-fwnode.h   |  53 +
>  4 files changed, 227 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 851f128eba22..6740a241d4a1 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -22,6 +22,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> @@ -278,6 +279,21 @@ void v4l2_async_notifier_unregister(struct 
> v4l2_async_notifier *notifier)
>  }
>  EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>  
> +void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < notifier->num_subdevs; i++)
> + kfree(notifier->subdevs[i]);
> +
> + notifier->max_subdevs = 0;
> + notifier->num_subdevs = 0;
> +
> + kvfree(notifier->subdevs);
> + notifier->subdevs = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_release);
> +
>  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  {
>   struct v4l2_async_notifier *notifier;
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c 
> b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 706f9e7b90f1..f8c7a9bc6a58 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -19,6 +19,7 @@
>   */
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -26,6 +27,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  
>  enum v4l2_fwnode_bus_type {
> @@ -313,6 +315,140 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>  
> +static int v4l2_async_notifier_realloc(struct v4l2_async_notifier *notifier,
> +unsigned int max_subdevs)
> +{
> + struct v4l2_async_subdev **subdevs;
> +
> + if (max_subdevs <= notifier->max_subdevs)
> + return 0;
> +
> + subdevs = kvmalloc_array(
> + max_subdevs, sizeof(*notifier->subdevs),
> + GFP_KERNEL | __GFP_ZERO);
> + if (!subdevs)
> + return -ENOMEM;
> +
> + if (notifier->subdevs) {
> + memcpy(subdevs, notifier->subdevs,
> +sizeof(*subdevs) * notifier->num_subdevs);
> +
> + kvfree(notifier->subdevs);
> + }
> +
> + notifier->subdevs = subdevs;
> + notifier->max_subdevs = max_subdevs;
> +
> + return 0;
> +}
> +
> +static int v4l2_async_notifier_fwnode_parse_endpoint(
> + struct device *dev, struct v4l2_async_notifier *notifier,
> + struct fwnode_handle *endpoint, unsigned int asd_struct_size,
> + int (*parse_endpoint)(struct device *dev,
> + struct v4l2_fwnode_endpoint *vep,
> + struct v4l2_async_subdev *asd))
> +{
> + struct v4l2_async_subdev *asd;
> + struct v4l2_fwnode_endpoint *vep;
> + int ret = 0;
> +
> + asd = kzalloc(asd_struct_size, GFP_KERNEL);
> + if (!asd)
> + return -ENOMEM;
> +
> + asd->match.fwnode.fwnode =
> + fwnode_graph_get_remote_port_parent(endpoint);
> + if (!asd->match.fwnode.fwnode) {
> + dev_warn(dev, "bad remote port parent\n");
> + ret = -EINVAL;
> + goto out_err;
> + }
> +
> + /* Ignore endpoints the parsing of which failed. */
> + vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> + if (IS_ERR(vep)) {
> + ret = PTR_ERR(vep);
> + dev_warn(dev, "unable to parse V4L2 fwnode endpoint (%d)\n",
> +  ret);
> + goto out_err;
> + }
> +
> + ret = parse_endpoint ? parse_endpoint(dev, vep, asd) : 0;

How likely is it that parse_endpoint will be NULL? I ask because if it is
common, then you could put everything from v4l2_fwnode_endpoint_alloc_parse
until just before 'asd->match_type = V4L2_ASYNC_MATCH_FWNODE;' under
'if (parse_endpoint) {'.

The disadvantage is that you won't see parse errors, but on the other hand
nobody apparently needs it, so why bother. I'm not sure what is the right thing
to do here.

> + v4l2_fwnode_endpoint_free(vep);

Here vep is freed,

> + if (ret == -ENOTCONN) {
> + dev_dbg(dev, "ignoring endpoint %u,%u\n", vep->base.port,
> +

[PATCH v3 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings

2017-09-04 Thread Maxime Ripard 
The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up to
4 CSI-2 lanes, and can route the frames to up to 4 streams, depending on
the hardware implementation.

It can operate with an external D-PHY, an internal one or no D-PHY at all
in some configurations.

Acked-by: Rob Herring 
Acked-by: Benoit Parrot 
Signed-off-by: Maxime Ripard 
---
 .../devicetree/bindings/media/cdns-csi2rx.txt  | 98 ++
 1 file changed, 98 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt

diff --git a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt 
b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
new file mode 100644
index ..2395030d8c72
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
@@ -0,0 +1,98 @@
+Cadence MIPI-CSI2 RX controller
+===
+
+The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to 4 CSI
+lanes in input, and 4 different pixel streams in output.
+
+Required properties:
+  - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible
+  - reg: base address and size of the memory mapped region
+  - clocks: phandles to the clocks driving the controller
+  - clock-names: must contain:
+* sys_clk: main clock
+* p_clk: register bank clock
+* pixel_if[0-3]_clk: pixel stream output clock, one for each stream
+ implemented in hardware, between 0 and 3
+
+Optional properties:
+  - phys: phandle to the external D-PHY, phy-names must be provided
+  - phy-names: must contain dphy, if the implementation uses an
+   external D-PHY
+
+Required subnodes:
+  - ports: A ports node with endpoint definitions as defined in
+   Documentation/devicetree/bindings/media/video-interfaces.txt. The
+   first port subnode should be the input endpoint, the next ones the
+   output, one for each stream supported by the CSI2-RX controller.
+   The ports ID must be the stream output number used in the
+   implementation, plus 1.
+
+Example:
+
+csi2rx: csi-bridge@0d06 {
+   compatible = "cdns,csi2rx";
+   reg = <0x0d06 0x1000>;
+   clocks = <>, <>
+<>, <>,
+<>, <>;
+   clock-names = "sys_clk", "p_clk",
+ "pixel_if0_clk", "pixel_if1_clk",
+ "pixel_if2_clk", "pixel_if3_clk";
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0>;
+
+   csi2rx_in_sensor: endpoint {
+   remote-endpoint = <_out_csi2rx>;
+   clock-lanes = <0>;
+   data-lanes = <1 2>;
+   };
+   };
+
+   port@1 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <1>;
+
+   csi2rx_out_grabber0: endpoint {
+   remote-endpoint = <_in_csi2rx>;
+   };
+   };
+
+   port@2 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <2>;
+
+   csi2rx_out_grabber1: endpoint {
+   remote-endpoint = <_in_csi2rx>;
+   };
+   };
+
+   port@3 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <3>;
+
+   csi2rx_out_grabber2: endpoint {
+   remote-endpoint = <_in_csi2rx>;
+   };
+   };
+
+   port@4 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <4>;
+
+   csi2rx_out_grabber3: endpoint {
+   remote-endpoint = <_in_csi2rx>;
+   };
+   };
+   };
+};
-- 
2.13.5

[PATCH v3 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 RX

2017-09-04 Thread Maxime Ripard 
Hi,

Here is an attempt at supporting the MIPI-CSI2 RX block from Cadence.

This IP block is able to receive CSI data over up to 4 lanes, and
split it to over 4 streams. Those streams are basically the interfaces
to the video grabbers that will perform the capture.

It is able to map streams to both CSI datatypes and virtual channels,
dynamically. This is unclear at this point what the right way to
support it would be, so the driver only uses a static mapping between
the virtual channels and streams, and ignores the data types.

This serie depends on the version 5 of the serie "v4l2-async: add subnotifier
registration for subdevices" from Niklas Söderlund.

Let me know what you think!
Maxime

Changes from v2:
  - Added reference counting for the controller initialisation
  - Fixed checkpatch warnings
  - Moved the sensor initialisation after the DPHY configuration
  - Renamed the sensor fields to source for consistency
  - Defined some variables
  - Renamed a few structures variables
  - Added internal and external phy errors messages
  - Reworked the binding slighty by making the external D-PHY optional
  - Moved the notifier registration in the probe function
  - Removed some clocks that are not system clocks
  - Added clocks enabling where needed
  - Added the code to remap the data lanes
  - Changed the memory allocator for the non-devm function, and a
comment explaining why
  - Reworked the binding wording

Changes from v1:
  - Amended the DT bindings as suggested by Rob
  - Rebase on top of 4.13-rc1 and latest Niklas' serie iteration

Maxime Ripard (2):
  dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
  v4l: cadence: Add Cadence MIPI-CSI2 RX driver

 .../devicetree/bindings/media/cdns-csi2rx.txt  |  98 
 drivers/media/platform/Kconfig |   1 +
 drivers/media/platform/Makefile|   2 +
 drivers/media/platform/cadence/Kconfig |  12 +
 drivers/media/platform/cadence/Makefile|   1 +
 drivers/media/platform/cadence/cdns-csi2rx.c   | 494 +
 6 files changed, 608 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt
 create mode 100644 drivers/media/platform/cadence/Kconfig
 create mode 100644 drivers/media/platform/cadence/Makefile
 create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c

-- 
2.13.5

[PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

2017-09-04 Thread Maxime Ripard 
The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
bridge between a CSI-2 bus and pixel grabbers.

It supports operating with internal or external D-PHY, with up to 4 lanes,
or without any D-PHY. The current code only supports the former case.

It also support dynamic mapping of the CSI-2 virtual channels to the
associated pixel grabbers, but that isn't allowed at the moment either.

Signed-off-by: Maxime Ripard 
---
 drivers/media/platform/Kconfig   |   1 +
 drivers/media/platform/Makefile  |   2 +
 drivers/media/platform/cadence/Kconfig   |  12 +
 drivers/media/platform/cadence/Makefile  |   1 +
 drivers/media/platform/cadence/cdns-csi2rx.c | 494 +++
 5 files changed, 510 insertions(+)
 create mode 100644 drivers/media/platform/cadence/Kconfig
 create mode 100644 drivers/media/platform/cadence/Makefile
 create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 1313cd533436..a79d96e9b723 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -26,6 +26,7 @@ config VIDEO_VIA_CAMERA
 #
 # Platform multimedia device configuration
 #
+source "drivers/media/platform/cadence/Kconfig"
 
 source "drivers/media/platform/davinci/Kconfig"
 
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 9beadc760467..1d31eb51e9bb 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -2,6 +2,8 @@
 # Makefile for the video capture/playback device drivers.
 #
 
+obj-$(CONFIG_VIDEO_CADENCE)+= cadence/
+
 obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
 
 obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
diff --git a/drivers/media/platform/cadence/Kconfig 
b/drivers/media/platform/cadence/Kconfig
new file mode 100644
index ..d1b6bbb6a0eb
--- /dev/null
+++ b/drivers/media/platform/cadence/Kconfig
@@ -0,0 +1,12 @@
+config VIDEO_CADENCE
+   bool "Cadence Video Devices"
+
+if VIDEO_CADENCE
+
+config VIDEO_CADENCE_CSI2RX
+   tristate "Cadence MIPI-CSI2 RX Controller v1.3"
+   depends on MEDIA_CONTROLLER
+   depends on VIDEO_V4L2_SUBDEV_API
+   select V4L2_FWNODE
+
+endif
diff --git a/drivers/media/platform/cadence/Makefile 
b/drivers/media/platform/cadence/Makefile
new file mode 100644
index ..99a4086b7448
--- /dev/null
+++ b/drivers/media/platform/cadence/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_CADENCE_CSI2RX) += cdns-csi2rx.o
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c 
b/drivers/media/platform/cadence/cdns-csi2rx.c
new file mode 100644
index ..e662b1890bba
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -0,0 +1,494 @@
+/*
+ * Driver for Cadence MIPI-CSI2 RX Controller v1.3
+ *
+ * Copyright (C) 2017 Cadence Design Systems Inc.
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define CSI2RX_DEVICE_CFG_REG  0x000
+
+#define CSI2RX_SOFT_RESET_REG  0x004
+#define CSI2RX_SOFT_RESET_PROTOCOL BIT(1)
+#define CSI2RX_SOFT_RESET_FRONTBIT(0)
+
+#define CSI2RX_STATIC_CFG_REG  0x008
+#define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane)  ((plane) << (16 + 
(llane) * 4))
+#define CSI2RX_STATIC_CFG_LANES_MASK   GENMASK(11, 8)
+
+#define CSI2RX_STREAM_BASE(n)  (((n) + 1) * 0x100)
+
+#define CSI2RX_STREAM_CTRL_REG(n)  (CSI2RX_STREAM_BASE(n) + 0x000)
+#define CSI2RX_STREAM_CTRL_START   BIT(0)
+
+#define CSI2RX_STREAM_DATA_CFG_REG(n)  (CSI2RX_STREAM_BASE(n) + 0x008)
+#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECTBIT(31)
+#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)BIT((n) + 16)
+
+#define CSI2RX_STREAM_CFG_REG(n)   (CSI2RX_STREAM_BASE(n) + 0x00c)
+#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF  (1 << 8)
+
+#define CSI2RX_STREAMS_MAX 4
+
+enum csi2rx_pads {
+   CSI2RX_PAD_SINK,
+   CSI2RX_PAD_SOURCE_STREAM0,
+   CSI2RX_PAD_SOURCE_STREAM1,
+   CSI2RX_PAD_SOURCE_STREAM2,
+   CSI2RX_PAD_SOURCE_STREAM3,
+   CSI2RX_PAD_MAX,
+};
+
+struct csi2rx_priv {
+   struct device   *dev;
+   atomic_tcount;
+
+   void __iomem*base;
+   struct clk  *sys_clk;
+   struct clk  *p_clk;
+   struct clk  *pixel_clk[CSI2RX_STREAMS_MAX];
+

Re: UVC property auto update

2017-09-04 Thread Guennadi Liakhovetski 
On Mon, 4 Sep 2017, Edgar Thier wrote:

> Hi Guennadi,
> 
> 
> > But that patch only re-reads the flags. What does that give you? Do those 
> > flags change? In which way? As far as I understand the UVC Autoupdate
> > feature, a change in GET_INFO data is only one possibility, (arguably) a 
> > more important one is changes in GET_CUR data.

Sorry, I misunderstood, please, ignore that.

> My understanding of the driver is rather narrow, so please correct me if I am 
> wrong.
> From what I can see the uvc driver is currently not asking the device if a 
> property has self
> modifying properties (and thus would require the AUTO_UPDATE flag).
> This is only done for properties in the extension unit but not for 'standard' 
> properties.
> Thus properties exhibit different behavior depending on where they are 
> defined.
> By changing this the driver now assumes that a property with AUTO_UPDATE has 
> to be re-read when
> getting/setting a property and does not rely on cached values, no matter if 
> extension unit or not.
> 
> I did not consider the possibility that a lower level change would be 
> necessary or that a more
> previce update mechanism for different property parts was possible.
> 
> Basically the entry point for my change would be here:
> https://git.linuxtv.org/media_tree.git/tree/drivers/media/usb/uvc/uvc_ctrl.c#n1405
> 
> How an update is handled by the driver did not seem important for this patch 
> as the retrieval of a
> correct property value seemed more important.

Ok, looking more at the spec, the driver and your patch, here's what I 
come up with:

1. UVC defines which standard controls should have which flags. Among 
those flags it specifies, which controls should specify the Autoupdate 
flag. E.g. see the first of them as it appears in my copy of the spec 
"4.2.2.4.8 Average Bit Rate Control"
2. The driver could read out flags from descriptors, but it hard-codes 
them instead. So, (arguably), there's no need to actually read them at 
probe time. XUs on the other hand aren't standard, therefore their flags 
have to be read out.
3. In your patch you provide gain as an example. Do you mean the 
PU_GAIN_CONTROL? The spec doesn't specify, that it should have Autoupdate 
set. Now, whether that means, that using that flag with PU_GAIN_CONTROL is 
a violation of the spec - I'm not sure about.

So, I think, the question really is - are hard-coded flags a proper and 
sufficient approach or should flags be read from descriptors?

> > In either case, this should 
> > be implemented using the UVC Interrupt endpoint. Here's my latest 
> > asynchronous control patch:
> > 
> > https://patchwork.linuxtv.org/patch/42800/
> > 
> > As you can see, it only handles the VALUE_CHANGE (GET_CUR) case. I would 
> > suggest implementing a patch on top of it to add support for INFO_CHANGE 
> > and you'd be the best person to test it then!
> 
> I will try it out. I should be able to give you feedback tomorrow.

Thanks.

> I will also ask the firmware developer if only value changes are available or 
> flag changes are also
> a possibility.

Well, flags aren't likely to change, perhaps. I think min and max values 
are more likely to be updated.

Regards
Guennadi

> Cheers,
> Edgar

[PATCH 0/2] Documents the two remaining CA ioctls

2017-09-04 Thread Mauro Carvalho Chehab 
Thanks to some discussions I had with Honza, I got some ideas about how to
document the last three undocumented ioctls from ca.h, together with the
two data structures.

With this series (and the previous one), everything at CA, Net, Demux 
and DVBv5 Frontend DVB APIs are now documented. That's IMHO
a very good reason to celebrate! Yay!

Please notice that are still gaps at the DVB API documentation, but
those are related to legacy stuff that aren't touched for ages
(DVBv3 frontend API, audio.h, osd.h and video.h). The legacy
 DVBv3 frontend API was completely replaced by DVBv5 API.
The other ones are used only by a single driver (av7110).

Mauro Carvalho Chehab (2):
  media: ca docs: document CA_SET_DESCR ioctl and structs
  media: ca.h: document ca_msg and the corresponding ioctls

 Documentation/media/uapi/dvb/ca-get-msg.rst   | 19 ++-
 Documentation/media/uapi/dvb/ca-send-msg.rst  |  6 +-
 Documentation/media/uapi/dvb/ca-set-descr.rst | 15 ++-
 include/uapi/linux/dvb/ca.h   | 20 ++--
 4 files changed, 31 insertions(+), 29 deletions(-)

-- 
2.13.5

[PATCH 2/2] media: ca.h: document ca_msg and the corresponding ioctls

2017-09-04 Thread Mauro Carvalho Chehab 
Usually, CA messages are sent/received via reading/writing at
the CA device node. However, two drivers (dst_ca and firedtv-ci)
also implement it via ioctls.

Apparently, on both cases, the net result is the same.

Anyway, let's document it.

Signed-off-by: Mauro Carvalho Chehab 
---
 Documentation/media/uapi/dvb/ca-get-msg.rst  | 19 ++-
 Documentation/media/uapi/dvb/ca-send-msg.rst |  6 +-
 include/uapi/linux/dvb/ca.h  | 11 ++-
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/Documentation/media/uapi/dvb/ca-get-msg.rst 
b/Documentation/media/uapi/dvb/ca-get-msg.rst
index bdb116552068..ceeda623ce93 100644
--- a/Documentation/media/uapi/dvb/ca-get-msg.rst
+++ b/Documentation/media/uapi/dvb/ca-get-msg.rst
@@ -28,22 +28,15 @@ Arguments
 ``msg``
   Pointer to struct :c:type:`ca_msg`.
 
-.. c:type:: ca_msg
-
-.. code-block:: c
-
-/* a message to/from a CI-CAM */
-struct ca_msg {
-   unsigned int index;
-   unsigned int type;
-   unsigned int length;
-   unsigned char msg[256];
-};
-
 Description
 ---
 
-.. note:: This ioctl is undocumented. Documentation is welcome.
+Receives a message via a CI CA module.
+
+.. note::
+
+   Please notice that, on most drivers, this is done by reading from
+   the /dev/adapter?/ca? device node.
 
 
 Return Value
diff --git a/Documentation/media/uapi/dvb/ca-send-msg.rst 
b/Documentation/media/uapi/dvb/ca-send-msg.rst
index 644b6bda1aea..9e91287b7bbc 100644
--- a/Documentation/media/uapi/dvb/ca-send-msg.rst
+++ b/Documentation/media/uapi/dvb/ca-send-msg.rst
@@ -32,8 +32,12 @@ Arguments
 Description
 ---
 
-.. note:: This ioctl is undocumented. Documentation is welcome.
+Sends a message via a CI CA module.
 
+.. note::
+
+   Please notice that, on most drivers, this is done by writing
+   to the /dev/adapter?/ca? device node.
 
 Return Value
 
diff --git a/include/uapi/linux/dvb/ca.h b/include/uapi/linux/dvb/ca.h
index a62ddf0cebcd..9bcd4cad497b 100644
--- a/include/uapi/linux/dvb/ca.h
+++ b/include/uapi/linux/dvb/ca.h
@@ -101,7 +101,16 @@ struct ca_caps {
unsigned int descr_type;
 };
 
-/* a message to/from a CI-CAM */
+/**
+ * struct ca_msg - a message to/from a CI-CAM
+ *
+ * @index: unused
+ * @type:  unused
+ * @length:length of the message
+ * @msg:   message
+ *
+ * This struct carries a message to be send/received from a CI CA module.
+ */
 struct ca_msg {
unsigned int index;
unsigned int type;
-- 
2.13.5

[PATCH 1/2] media: ca docs: document CA_SET_DESCR ioctl and structs

2017-09-04 Thread Mauro Carvalho Chehab 
The av7110 driver uses CA_SET_DESCR to store the descrambler
control words at the CA descrambler slots.

Document it.

Thanks-to: Honza Petrouš 
Signed-off-by: Mauro Carvalho Chehab 
---
 Documentation/media/uapi/dvb/ca-set-descr.rst | 15 ++-
 include/uapi/linux/dvb/ca.h   |  9 -
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/Documentation/media/uapi/dvb/ca-set-descr.rst 
b/Documentation/media/uapi/dvb/ca-set-descr.rst
index 9c484317d55c..a6c47205ffd8 100644
--- a/Documentation/media/uapi/dvb/ca-set-descr.rst
+++ b/Documentation/media/uapi/dvb/ca-set-descr.rst
@@ -28,22 +28,11 @@ Arguments
 ``msg``
   Pointer to struct :c:type:`ca_descr`.
 
-.. c:type:: ca_descr
-
-.. code-block:: c
-
-struct ca_descr {
-   unsigned int index;
-   unsigned int parity;
-   unsigned char cw[8];
-};
-
-
 Description
 ---
 
-.. note:: This ioctl is undocumented. Documentation is welcome.
-
+CA_SET_DESCR is used for feeding descrambler CA slots with descrambling
+keys (refered as control words).
 
 Return Value
 
diff --git a/include/uapi/linux/dvb/ca.h b/include/uapi/linux/dvb/ca.h
index f66ed53f4dc7..a62ddf0cebcd 100644
--- a/include/uapi/linux/dvb/ca.h
+++ b/include/uapi/linux/dvb/ca.h
@@ -109,9 +109,16 @@ struct ca_msg {
unsigned char msg[256];
 };
 
+/**
+ * struct ca_descr - CA descrambler control words info
+ *
+ * @index: CA Descrambler slot
+ * @parity: control words parity, where 0 means even and 1 means odd
+ * @cw: CA Descrambler control words
+ */
 struct ca_descr {
unsigned int index;
-   unsigned int parity;/* 0 == even, 1 == odd */
+   unsigned int parity;
unsigned char cw[8];
 };
 
-- 
2.13.5

Re: [PATCH v2 00/26] Improve DVB documentation and reduce its gap

2017-09-04 Thread Mauro Carvalho Chehab 
Em Mon, 4 Sep 2017 11:40:59 +0200
Honza Petrouš  escreveu:

> > So, IMHO, the interface is broken by design. Perhaps that's
> > the reason why no upstream driver uses it.  
> 
> I have the same feeling regarding brokenness.
> 
> >
> > What seems to be a much better design would be to use the demux
> > set filter ioctls and route the PIDs to the right CA.
> >  
> 
> I don't have access to any programmer reference documentation
> for any modern DVB-enabled SoC, but I see two possible scenario
> of connecting descramblers to the demuxes (most of modern SoCs
> have more then one demux) - static one, when every demux has
> predefined descramblers already connected to it and dynamic ones,
> when any descrambler can be connected to the any demux.

I don't have access to the documentation either, but I know
some designs that have multiple demods that are dynamically set.
Some hardware even allow to dynamically change the maximum amount
of filters per demod at runtime.

> From that reason I vote to have some descrambler specific ioctl,
> which allow more flexibility then if we add it to the filter set ioctl.

I suspect that doing it at the demod does a lot more sense.

Anyway, someone should come with a driver requiring it upstream
for us to discuss and find the better alternatives to support.

Thanks,
Mauro

Re: [PATCH 00/15] Improve DVB documentation and reduce its gap

2017-09-04 Thread Mauro Carvalho Chehab 
Em Mon, 4 Sep 2017 02:55:15 +0200
Soeren Moch  escreveu:

> Hi Mauro,
> 
> On 01.09.2017 11:32, Mauro Carvalho Chehab wrote:
> > Em Fri, 1 Sep 2017 10:40:28 +0200
> > Honza Petrouš  escreveu:
> >  
> >> 2017-09-01 1:46 GMT+02:00 Mauro Carvalho Chehab 
> >> :  
> >>> The DVB documentation was negligected for a long time, with
> >>> resulted on several gaps between the API description and its
> >>> documentation.
> >>>
> >>> I'm doing a new reading at the documentation. As result of it,
> >>> this series:
> >>>
> >>> - improves the introductory chapter, making it more generic;
> >>> - Do some adjustments at the frontend API, using kernel-doc
> >>>   when possible.
> >>> - Remove unused APIs at DVB demux. I suspect that the drivers
> >>>   implementing such APIs were either never merged upstream,
> >>>   or the API itself  were never used or was deprecated a long
> >>>   time ago. In any case, it doesn't make any sense to carry
> >>>   on APIs that aren't properly documented, nor are used on the
> >>>   upstream Kernel.
> >>>
> >>> With this patch series, the gap between documentation and
> >>> code is solved for 3 DVB APIs:
> >>>
> >>>   - Frontend API;
> >>>   - Demux API;
> >>>   - Net API.
> >>>
> >>> There is still a gap at the CA API that I'll try to address when I
> >>> have some time[1].
> >>>
> >>> [1] There's a gap also on the legacy audio, video and OSD APIs,
> >>> but, as those are used only by a single very old deprecated
> >>> hardware (av7110), it is probably not worth the efforts.
> >>>
> av7110 cards may be old, but there are still users of these cards. 
> For instance I'm watching TV received and decoded with such card in this 
> moment.
> So what do you mean with "deprecated hardware"?

Nobody is telling otherwise. What I mean by "deprecated" is that it is
not a product that you could got to a shop and buy a new one. Its 
production stopped a long time ago.

> >> I agree that av7110 is very very old piece of hw (but it is already
> >> in my hall of fame because of its Skystar 1 incarnation as
> >> first implementation of DVB in Linux) and it is sad that we still
> >> don't have at least one driver for any SoC with embedded DVB
> >> devices.  
> > Yeah, av7110 made history. Please notice that this series doesn't
> > remove any API that it is used by it. All it removes are the APIs
> > that there's no Kernel driver using.
> >
> > Carry on APIs without client is usually a very bad idea, as nobody
> > could be certain about how to use it. It is even worse when such
> > APIs are not properly documented (with is the case here).
> >  
> >> I understand that the main issue is that no any DVB-enabled
> >> SoC vendor is interested in upstreaming theirs code, but I still hope
> >> it will change in near future(*)  
> > We have one driver for a SoC DVB hardware at:
> > drivers/media/platform/sti/c8sectpfe/
> >
> > I guess it still doesn't cover the entire SoC, but this is a WiP. If I
> > remember well, at the midia part of the SoC, they started by submitting
> > the Remote Controller code.
> >  
> >> Without having full-featured DVB device in vanilla, we surely don't
> >> get some parts of DVB API covered. I can imagine that  when
> >> somebody comes with such full-featured device he wants to reinvent
> >> just removed bits.  
> > Re-adding the removed bits is easy. However, the API defined for
> > av7110 is old and it is showing its age: it assumes a limited number
> > of possible inputs/outputs. Modern SoC has a lot more ways link the
> > audio/video IP blocks than what the API provides. On a modern SoC,
> > not only DVB is supported, but also analog inputs (to support things
> > like composite input), cameras, HDMI inputs and even analog TV.
> > All of them interconnected to a media ISP. The current FF API can't
> > represent such hardware.
> >
> > The best API to represent those pipelines that exist on SoC for
> > multimedia is the media controller, where all IP blocks and their
> > links (whatever they are) can be represented, if needed.
> >
> > The audio and video DVB API is also too limited: it hasn't
> > evolved since when it was added. For audio, the ALSA API
> > allows a way more control of the hardware; for video, the
> > V4L2 API nowadays has all the bits to control video decoding
> > and encoding. Both APIs provide support for audio and video
> > inputs commonly found on those devices.  
> The real advantage of the DVB audio/video/osd API is the possibility
> of frame synchronous audio/video/overlay output for high-quality
> audio/video playback, maybe with picture-in-picture functionality.
> 
> Especially audio synchronization is not easy when the audio format
> changes from compressed audio (e.g. AC-3) to PCM (stereo), e.g. on
> HDMI output. While HDMI output hardware usually takes video frames and
> audio packets (and AVI info frames for audio/video format signalization)
> synchronously, V4L2 and ALSA rip these

Re: UVC property auto update

2017-09-04 Thread Edgar Thier 
Hi Guennadi,


> But that patch only re-reads the flags. What does that give you? Do those > 
> flags change? In which way? As far as I understand the UVC Autoupdate
> feature, a change in GET_INFO data is only one possibility, (arguably) a 
> more important one is changes in GET_CUR data.

My understanding of the driver is rather narrow, so please correct me if I am 
wrong.
>From what I can see the uvc driver is currently not asking the device if a 
>property has self
modifying properties (and thus would require the AUTO_UPDATE flag).
This is only done for properties in the extension unit but not for 'standard' 
properties.
Thus properties exhibit different behavior depending on where they are defined.
By changing this the driver now assumes that a property with AUTO_UPDATE has to 
be re-read when
getting/setting a property and does not rely on cached values, no matter if 
extension unit or not.

I did not consider the possibility that a lower level change would be necessary 
or that a more
previce update mechanism for different property parts was possible.

Basically the entry point for my change would be here:
https://git.linuxtv.org/media_tree.git/tree/drivers/media/usb/uvc/uvc_ctrl.c#n1405

How an update is handled by the driver did not seem important for this patch as 
the retrieval of a
correct property value seemed more important.

> In either case, this should 
> be implemented using the UVC Interrupt endpoint. Here's my latest 
> asynchronous control patch:
> 
> https://patchwork.linuxtv.org/patch/42800/
> 
> As you can see, it only handles the VALUE_CHANGE (GET_CUR) case. I would 
> suggest implementing a patch on top of it to add support for INFO_CHANGE 
> and you'd be the best person to test it then!

I will try it out. I should be able to give you feedback tomorrow.
I will also ask the firmware developer if only value changes are available or 
flag changes are also
a possibility.

Cheers,

Edgar

Re: [PATCH v6 5/5] v4l: fwnode: Support generic parsing of graph endpoints in a single port

2017-09-04 Thread Laurent Pinchart 
Hi Sakari,

On Sunday, 3 September 2017 10:43:39 EEST Sakari Ailus wrote:
> On Sat, Sep 02, 2017 at 12:52:47PM +0300, Laurent Pinchart wrote:
> > On Saturday, 2 September 2017 01:57:48 EEST Sakari Ailus wrote:
> >> On Fri, Sep 01, 2017 at 01:28:40PM +0200, Hans Verkuil wrote:

[sinp]

> >>> I'm lost. What's the relationship between
> >>> v4l2_async_notifier_parse_fwnode_endpoints and this function? When do
> >>> you use which? When you should zero the notifier?
> >> 
> >> I thought there would be advantages in this approach as it lets you to
> >> choose which endpoints specifically you want to parse. That said, the
> >> expectation is that the device has no endpoints that aren't supported in
> >> hardware either.
> >> 
> >> Some drivers currently iterate over all the endpoints and then validate
> >> them whereas others poke for some endpoints only (port 0, endpoint 0,
> >> for the rcar-vin driver, for instance). In DT binding documentation the
> >> endpoint numbers are currently not specified nor drivers have checked
> >> them. Common sense tells to use zero if there's no reason to do
> >> otherwise, but still this hasn't been documented nor validated in the
> >> past. So if we add that now, there could be a chance of breaking
> >> something.
> >> 
> >> Additionally, specifying the endpoints to parse explicitly has been seen
> >> beneficial (or even necessary) in parsing endpoints for devices that
> >> have both input and output interfaces. Perhaps Niklas can comment on
> >> that.
> >> 
> >> What I though was to introduce a specific error code (EPERM, better
> >> suggestions are taken)
> > 
> > Maybe ENOTCONN ?
> 
> Sounds good to me.
> 
> >> for the driver callback function (parse_endpoint) to silently skip
> >> endpoints the driver doesn't like for reason or another. This lets
> >> drivers to use the endpoint parser function added by the previous patch
> >> and still maintain the old behaviour, i.e. ignore endpoints that aren't
> >> explicitly recognised by the driver.
> >> 
> >> I'll drop this patch from the next version.
> > 
> > Parsing a specific endpoint of a specific port is probably indeed a bit
> > too fine-grained, but I think there are use cases for parsing at the port
> > level instead of parsing all ports.
> 
> Could you elaborate?
> 
> If a driver would be interested in skipping endpoints in a subset of ports,
> in which case only a single port would be excluded from this?

I meant that I see use cases for parsing specific ports only (for instance in 
the R-Car case parsing only the sink ports in a CSI-2 receiver DT node), but 
not really for parsing specific endpoints only.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: extend UVC_QUIRK_FIX_BANDWIDTH to MJPEG streams

2017-09-04 Thread Laurent Pinchart 
Hi Pavel,

Thank you for the patch.

On Monday, 4 September 2017 11:14:17 EEST Pavel Rojtberg wrote:
> From: Pavel Rojtberg 
> 
> attaching two Logitech C615 webcams currently results in
> VIDIOC_STREAMON: No space left on device
> as the required bandwidth is not estimated correctly by the device.
> In fact it always requests 3060 bytes - no matter the format or resolution.
> 
> setting UVC_QUIRK_FIX_BANDWIDTH does not help either as it is only
> implemented for uncompressed streams.
> 
> This patch extends UVC_QUIRK_FIX_BANDWIDTH to MJPEG streams by making a
> (conservative) assumption of 4bpp for MJPEG streams.
> As the actual compression ration is often closer to 1bpp this can be
> overridden via the new mjpeg_bpp parameter.
> 
> Based on:
> https://www.mail-archive.com/linux-uvc-devel@lists.berlios.de/msg05724.html
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 14 +-
>  drivers/media/usb/uvc/uvc_video.c  |  3 ++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 70842c5..f7b759e 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -37,6 +37,7 @@ unsigned int uvc_no_drop_param;
>  static unsigned int uvc_quirks_param = -1;
>  unsigned int uvc_trace_param;
>  unsigned int uvc_timeout_param = UVC_CTRL_STREAMING_TIMEOUT;
> +static unsigned int uvc_mjpeg_bpp_param;
> 
>  /* 
> * Video formats
> @@ -463,7 +464,16 @@ static int uvc_parse_format(struct uvc_device *dev,
>   strlcpy(format->name, "MJPEG", sizeof format->name);
>   format->fcc = V4L2_PIX_FMT_MJPEG;
>   format->flags = UVC_FMT_FLAG_COMPRESSED;
> - format->bpp = 0;
> + if ((uvc_mjpeg_bpp_param >= 1) && (uvc_mjpeg_bpp_param <= 16)) {
> + format->bpp = uvc_mjpeg_bpp_param;
> + } else {
> + /* conservative estimate. Actual values are around 1bpp.
> +  * see e.g.
> +  * 
> https://developers.google.com/speed/webp/docs/webp_study
> +  */
> + format->bpp = 4;
> + }
> +
>   ftype = UVC_VS_FRAME_MJPEG;
>   break;
> 
> @@ -2274,6 +2284,8 @@ module_param_named(trace, uvc_trace_param, uint,
> S_IRUGO|S_IWUSR); MODULE_PARM_DESC(trace, "Trace level bitmask");
>  module_param_named(timeout, uvc_timeout_param, uint, S_IRUGO|S_IWUSR);
>  MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
> +module_param_named(mjpeg_bpp, uvc_mjpeg_bpp_param, uint, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(mjpeg_bpp, "MJPEG bits per pixel for bandwidth quirk");

I'd rather avoid adding a new module parameter for this, it would be confusing 
for users. What is your use case to make the MJPEG average BPP configurable ? 
Can't we come up with a heuristic that would calculate it automatically ?

>  /* 
> * Driver initialization and cleanup
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index fb86d6a..382a0be 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -127,7 +127,8 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming
> *stream, if ((ctrl->dwMaxPayloadTransferSize & 0x) == 0x)
> ctrl->dwMaxPayloadTransferSize &= ~0x;
> 
> - if (!(format->flags & UVC_FMT_FLAG_COMPRESSED) &&
> + if ((!(format->flags & UVC_FMT_FLAG_COMPRESSED) ||
> + (format->fcc == V4L2_PIX_FMT_MJPEG)) &&
>   stream->dev->quirks & UVC_QUIRK_FIX_BANDWIDTH &&
>   stream->intf->num_altsetting > 1) {
>   u32 interval;


-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 00/26] Improve DVB documentation and reduce its gap

2017-09-04 Thread Honza Petrouš 
2017-09-04 11:06 GMT+02:00 Mauro Carvalho Chehab :
> Em Mon, 4 Sep 2017 09:12:49 +0200
> Honza Petrouš  escreveu:
>
>> 2017-09-04 2:54 GMT+02:00 Mauro Carvalho Chehab :
>> > Em Sun, 3 Sep 2017 22:05:23 +0200
>> > Honza Petrouš  escreveu:
>> >
>> >> 1) #define CA_SET_DESCR  _IOW('o', 134, ca_descr_t)
>> >> 
>> >>
>> >> CA_SET_DESCR is used for feeding descrambler device
>> >> with correct keys (called here "control words") what
>> >> allows to get services unscrambled.
>> >>
>> >> The best docu is:
>> >>
>> >> "Digital Video Broadcasting (DVB);
>> >> Support for use of the DVB Scrambling Algorithm version 3
>> >> within digital broadcasting systems"
>> >>
>> >> Defined as DVB Document A125 and publicly
>> >> available here:
>> >>
>> >> https://www.dvb.org/resources/public/standards/a125_dvb-csa3.pdf
>> >>
>> >>
>> >> typedef struct ca_descr {
>> >> unsigned int index;
>> >> unsigned int parity;/* 0 == even, 1 == odd */
>> >> unsigned char cw[8];
>> >> } ca_descr_t;
>> >>
>> >> The 'index' is adress of the descrambler instance, as there exist
>> >> limited number of them (retieved by CA_GET_DESCR_INFO).
>> >
>> > Thanks for the info. If I understood well, the enclosed patch should
>> > be documenting it.
>> >
>> >
>> > Thanks,
>> > Mauro
>> >
>> > [PATCH] media: ca docs: document CA_SET_DESCR ioctl and structs
>> >
>> > The av7110 driver uses CA_SET_DESCR to store the descrambler
>> > control words at the CA descrambler slots.
>> >
>> > Document it.
>> >
>> > Thanks-to: Honza Petrouš 
>> > Signed-off-by: Mauro Carvalho Chehab 
>> >
>> > diff --git a/Documentation/media/uapi/dvb/ca-set-descr.rst 
>> > b/Documentation/media/uapi/dvb/ca-set-descr.rst
>> > index 9c484317d55c..a6c47205ffd8 100644
>> > --- a/Documentation/media/uapi/dvb/ca-set-descr.rst
>> > +++ b/Documentation/media/uapi/dvb/ca-set-descr.rst
>> > @@ -28,22 +28,11 @@ Arguments
>> >  ``msg``
>> >Pointer to struct :c:type:`ca_descr`.
>> >
>> > -.. c:type:: ca_descr
>> > -
>> > -.. code-block:: c
>> > -
>> > -struct ca_descr {
>> > -   unsigned int index;
>> > -   unsigned int parity;
>> > -   unsigned char cw[8];
>> > -};
>> > -
>> > -
>> >  Description
>> >  ---
>> >
>> > -.. note:: This ioctl is undocumented. Documentation is welcome.
>> > -
>> > +CA_SET_DESCR is used for feeding descrambler CA slots with descrambling
>> > +keys (refered as control words).
>> >
>> >  Return Value
>> >  
>> > diff --git a/include/uapi/linux/dvb/ca.h b/include/uapi/linux/dvb/ca.h
>> > index f66ed53f4dc7..a62ddf0cebcd 100644
>> > --- a/include/uapi/linux/dvb/ca.h
>> > +++ b/include/uapi/linux/dvb/ca.h
>> > @@ -109,9 +109,16 @@ struct ca_msg {
>> > unsigned char msg[256];
>> >  };
>> >
>> > +/**
>> > + * struct ca_descr - CA descrambler control words info
>> > + *
>> > + * @index: CA Descrambler slot
>> > + * @parity: control words parity, where 0 means even and 1 means odd
>> > + * @cw: CA Descrambler control words
>> > + */
>> >  struct ca_descr {
>> > unsigned int index;
>> > -   unsigned int parity;/* 0 == even, 1 == odd */
>> > +   unsigned int parity;
>> > unsigned char cw[8];
>> >  };
>> >
>> >
>>
>> Yeh, it should be that way.
>
> Good! I'll add this patch to the series.
>
>> BTW, the only issue I have in mind is how to link particular
>> descrambler with the PID
>> after your removal of the CA_SET_PID. And yes, I know that currently we have
>> no any user of such ioctl in our driver base :)
>
> Well, I don't think that an ioctl like CA_SET_PID would solve it.
>
> On a generic case with is quite common nowadays on embedded hardware,
> We have K demods and M CIs (where K may be different than M).
>
> Also, You may need to route N PIDs to O descramblers.

TBH that is exactly most common use-case = most Digital TV
vendors are scrambling per-service, what requires one descrambler
for all scrambled PIDs (usually only A/V PIDs are scrambled)
for particular service. So we have to add more PIDs to one descrambler

What was possible by multiple call of CA_SET_PID (I agree that much
better would be name like CA_ADD_PID)
>
> As user switch channels, the N PIDs should be unset, and another
> set of N' pids will be routed.
>
> CA_SET_PID allows to set just one PID, without identifying from
> what demod it would be received, and doesn't have a "reset"
> function to undo.

Here I can agree - it looks like the value -1 in 'index' should
do the job, but it, unfortunately, looses info from which descrambler
it should be removed (see note in struct ca_pid for value -1)

>
> So, IMHO, the interface is broken by design. Perhaps that's
> the reason why no upstream driver uses it.

I have the same feeling regarding brokenness.

>
> What seems to be a much better design would be to use the

Re: [PATCH v2 00/26] Improve DVB documentation and reduce its gap

2017-09-04 Thread Mauro Carvalho Chehab 
Em Mon, 4 Sep 2017 09:12:49 +0200
Honza Petrouš  escreveu:

> 2017-09-04 2:54 GMT+02:00 Mauro Carvalho Chehab :
> > Em Sun, 3 Sep 2017 22:05:23 +0200
> > Honza Petrouš  escreveu:
> >  
> >> 1) #define CA_SET_DESCR  _IOW('o', 134, ca_descr_t)
> >> 
> >>
> >> CA_SET_DESCR is used for feeding descrambler device
> >> with correct keys (called here "control words") what
> >> allows to get services unscrambled.
> >>
> >> The best docu is:
> >>
> >> "Digital Video Broadcasting (DVB);
> >> Support for use of the DVB Scrambling Algorithm version 3
> >> within digital broadcasting systems"
> >>
> >> Defined as DVB Document A125 and publicly
> >> available here:
> >>
> >> https://www.dvb.org/resources/public/standards/a125_dvb-csa3.pdf
> >>
> >>
> >> typedef struct ca_descr {
> >> unsigned int index;
> >> unsigned int parity;/* 0 == even, 1 == odd */
> >> unsigned char cw[8];
> >> } ca_descr_t;
> >>
> >> The 'index' is adress of the descrambler instance, as there exist
> >> limited number of them (retieved by CA_GET_DESCR_INFO).  
> >
> > Thanks for the info. If I understood well, the enclosed patch should
> > be documenting it.
> >
> >
> > Thanks,
> > Mauro
> >
> > [PATCH] media: ca docs: document CA_SET_DESCR ioctl and structs
> >
> > The av7110 driver uses CA_SET_DESCR to store the descrambler
> > control words at the CA descrambler slots.
> >
> > Document it.
> >
> > Thanks-to: Honza Petrouš 
> > Signed-off-by: Mauro Carvalho Chehab 
> >
> > diff --git a/Documentation/media/uapi/dvb/ca-set-descr.rst 
> > b/Documentation/media/uapi/dvb/ca-set-descr.rst
> > index 9c484317d55c..a6c47205ffd8 100644
> > --- a/Documentation/media/uapi/dvb/ca-set-descr.rst
> > +++ b/Documentation/media/uapi/dvb/ca-set-descr.rst
> > @@ -28,22 +28,11 @@ Arguments
> >  ``msg``
> >Pointer to struct :c:type:`ca_descr`.
> >
> > -.. c:type:: ca_descr
> > -
> > -.. code-block:: c
> > -
> > -struct ca_descr {
> > -   unsigned int index;
> > -   unsigned int parity;
> > -   unsigned char cw[8];
> > -};
> > -
> > -
> >  Description
> >  ---
> >
> > -.. note:: This ioctl is undocumented. Documentation is welcome.
> > -
> > +CA_SET_DESCR is used for feeding descrambler CA slots with descrambling
> > +keys (refered as control words).
> >
> >  Return Value
> >  
> > diff --git a/include/uapi/linux/dvb/ca.h b/include/uapi/linux/dvb/ca.h
> > index f66ed53f4dc7..a62ddf0cebcd 100644
> > --- a/include/uapi/linux/dvb/ca.h
> > +++ b/include/uapi/linux/dvb/ca.h
> > @@ -109,9 +109,16 @@ struct ca_msg {
> > unsigned char msg[256];
> >  };
> >
> > +/**
> > + * struct ca_descr - CA descrambler control words info
> > + *
> > + * @index: CA Descrambler slot
> > + * @parity: control words parity, where 0 means even and 1 means odd
> > + * @cw: CA Descrambler control words
> > + */
> >  struct ca_descr {
> > unsigned int index;
> > -   unsigned int parity;/* 0 == even, 1 == odd */
> > +   unsigned int parity;
> > unsigned char cw[8];
> >  };
> >
> >  
> 
> Yeh, it should be that way.

Good! I'll add this patch to the series.

> BTW, the only issue I have in mind is how to link particular
> descrambler with the PID
> after your removal of the CA_SET_PID. And yes, I know that currently we have
> no any user of such ioctl in our driver base :)

Well, I don't think that an ioctl like CA_SET_PID would solve it.

On a generic case with is quite common nowadays on embedded hardware,
We have K demods and M CIs (where K may be different than M).

Also, You may need to route N PIDs to O descramblers.

As user switch channels, the N PIDs should be unset, and another
set of N' pids will be routed.

CA_SET_PID allows to set just one PID, without identifying from
what demod it would be received, and doesn't have a "reset" 
function to undo.

So, IMHO, the interface is broken by design. Perhaps that's
the reason why no upstream driver uses it.

What seems to be a much better design would be to use the demux
set filter ioctls and route the PIDs to the right CA.


Thanks,
Mauro

Re: UVC property auto update

2017-09-04 Thread Guennadi Liakhovetski 
Hi Edgar,

Thanks for the explanation.

On Mon, 4 Sep 2017, Edgar Thier wrote:

> Hi Guennadi,
> 
> The cameras in question are USB-3.0 industrial cameras from The Imaging 
> Source.
> The ones I tested were the DFK UX250 and DFK UX264 models.
> I do not know if there are other devices that have the AUTO_UPDATE flag for 
> various properties.
> 
> Since I received no immediate answer I tried fixing it myself.
> The result can be found here:
> https://patchwork.linuxtv.org/patch/43289/

But that patch only re-reads the flags. What does that give you? Do those 
flags change? In which way? As far as I understand the UVC Autoupdate 
feature, a change in GET_INFO data is only one possibility, (arguably) a 
more important one is changes in GET_CUR data. In either case, this should 
be implemented using the UVC Interrupt endpoint. Here's my latest 
asynchronous control patch:

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

As you can see, it only handles the VALUE_CHANGE (GET_CUR) case. I would 
suggest implementing a patch on top of it to add support for INFO_CHANGE 
and you'd be the best person to test it then!

Thanks
Guennadi

> Cheers,
> 
> Edgar

[PATCH] uvcvideo: extend UVC_QUIRK_FIX_BANDWIDTH to MJPEG streams

2017-09-04 Thread Pavel Rojtberg 
From: Pavel Rojtberg 

attaching two Logitech C615 webcams currently results in
VIDIOC_STREAMON: No space left on device
as the required bandwidth is not estimated correctly by the device.
In fact it always requests 3060 bytes - no matter the format or resolution.

setting UVC_QUIRK_FIX_BANDWIDTH does not help either as it is only implemented
for uncompressed streams.

This patch extends UVC_QUIRK_FIX_BANDWIDTH to MJPEG streams by making a
(conservative) assumption of 4bpp for MJPEG streams.
As the actual compression ration is often closer to 1bpp this can be overridden
 via the new mjpeg_bpp parameter.

Based on:
https://www.mail-archive.com/linux-uvc-devel@lists.berlios.de/msg05724.html
---
 drivers/media/usb/uvc/uvc_driver.c | 14 +-
 drivers/media/usb/uvc/uvc_video.c  |  3 ++-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c 
b/drivers/media/usb/uvc/uvc_driver.c
index 70842c5..f7b759e 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -37,6 +37,7 @@ unsigned int uvc_no_drop_param;
 static unsigned int uvc_quirks_param = -1;
 unsigned int uvc_trace_param;
 unsigned int uvc_timeout_param = UVC_CTRL_STREAMING_TIMEOUT;
+static unsigned int uvc_mjpeg_bpp_param;
 
 /* 
  * Video formats
@@ -463,7 +464,16 @@ static int uvc_parse_format(struct uvc_device *dev,
strlcpy(format->name, "MJPEG", sizeof format->name);
format->fcc = V4L2_PIX_FMT_MJPEG;
format->flags = UVC_FMT_FLAG_COMPRESSED;
-   format->bpp = 0;
+   if ((uvc_mjpeg_bpp_param >= 1) && (uvc_mjpeg_bpp_param <= 16)) {
+   format->bpp = uvc_mjpeg_bpp_param;
+   } else {
+   /* conservative estimate. Actual values are around 1bpp.
+* see e.g.
+* 
https://developers.google.com/speed/webp/docs/webp_study
+*/
+   format->bpp = 4;
+   }
+
ftype = UVC_VS_FRAME_MJPEG;
break;
 
@@ -2274,6 +2284,8 @@ module_param_named(trace, uvc_trace_param, uint, 
S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(trace, "Trace level bitmask");
 module_param_named(timeout, uvc_timeout_param, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(timeout, "Streaming control requests timeout");
+module_param_named(mjpeg_bpp, uvc_mjpeg_bpp_param, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(mjpeg_bpp, "MJPEG bits per pixel for bandwidth quirk");
 
 /* 
  * Driver initialization and cleanup
diff --git a/drivers/media/usb/uvc/uvc_video.c 
b/drivers/media/usb/uvc/uvc_video.c
index fb86d6a..382a0be 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -127,7 +127,8 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming 
*stream,
if ((ctrl->dwMaxPayloadTransferSize & 0x) == 0x)
ctrl->dwMaxPayloadTransferSize &= ~0x;
 
-   if (!(format->flags & UVC_FMT_FLAG_COMPRESSED) &&
+   if ((!(format->flags & UVC_FMT_FLAG_COMPRESSED) ||
+   (format->fcc == V4L2_PIX_FMT_MJPEG)) &&
stream->dev->quirks & UVC_QUIRK_FIX_BANDWIDTH &&
stream->intf->num_altsetting > 1) {
u32 interval;
-- 
2.7.4

Re: UVC property auto update

2017-09-04 Thread Edgar Thier 
Hi Guennadi,

The cameras in question are USB-3.0 industrial cameras from The Imaging Source.
The ones I tested were the DFK UX250 and DFK UX264 models.
I do not know if there are other devices that have the AUTO_UPDATE flag for 
various properties.

Since I received no immediate answer I tried fixing it myself.
The result can be found here:
https://patchwork.linuxtv.org/patch/43289/

Cheers,

Edgar

Re: [PATCH v7 00/18] Unified fwnode endpoint parser, async sub-device notifier support, N9 flash DTS

2017-09-04 Thread Sakari Ailus 
Oh, and this set depends on this patch, on its way to 4.14-rc1 I believe:



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

Re: [PATCH v2 00/26] Improve DVB documentation and reduce its gap

2017-09-04 Thread Honza Petrouš 
2017-09-04 2:54 GMT+02:00 Mauro Carvalho Chehab :
> Em Sun, 3 Sep 2017 22:05:23 +0200
> Honza Petrouš  escreveu:
>
>> 1) #define CA_SET_DESCR  _IOW('o', 134, ca_descr_t)
>> 
>>
>> CA_SET_DESCR is used for feeding descrambler device
>> with correct keys (called here "control words") what
>> allows to get services unscrambled.
>>
>> The best docu is:
>>
>> "Digital Video Broadcasting (DVB);
>> Support for use of the DVB Scrambling Algorithm version 3
>> within digital broadcasting systems"
>>
>> Defined as DVB Document A125 and publicly
>> available here:
>>
>> https://www.dvb.org/resources/public/standards/a125_dvb-csa3.pdf
>>
>>
>> typedef struct ca_descr {
>> unsigned int index;
>> unsigned int parity;/* 0 == even, 1 == odd */
>> unsigned char cw[8];
>> } ca_descr_t;
>>
>> The 'index' is adress of the descrambler instance, as there exist
>> limited number of them (retieved by CA_GET_DESCR_INFO).
>
> Thanks for the info. If I understood well, the enclosed patch should
> be documenting it.
>
>
> Thanks,
> Mauro
>
> [PATCH] media: ca docs: document CA_SET_DESCR ioctl and structs
>
> The av7110 driver uses CA_SET_DESCR to store the descrambler
> control words at the CA descrambler slots.
>
> Document it.
>
> Thanks-to: Honza Petrouš 
> Signed-off-by: Mauro Carvalho Chehab 
>
> diff --git a/Documentation/media/uapi/dvb/ca-set-descr.rst 
> b/Documentation/media/uapi/dvb/ca-set-descr.rst
> index 9c484317d55c..a6c47205ffd8 100644
> --- a/Documentation/media/uapi/dvb/ca-set-descr.rst
> +++ b/Documentation/media/uapi/dvb/ca-set-descr.rst
> @@ -28,22 +28,11 @@ Arguments
>  ``msg``
>Pointer to struct :c:type:`ca_descr`.
>
> -.. c:type:: ca_descr
> -
> -.. code-block:: c
> -
> -struct ca_descr {
> -   unsigned int index;
> -   unsigned int parity;
> -   unsigned char cw[8];
> -};
> -
> -
>  Description
>  ---
>
> -.. note:: This ioctl is undocumented. Documentation is welcome.
> -
> +CA_SET_DESCR is used for feeding descrambler CA slots with descrambling
> +keys (refered as control words).
>
>  Return Value
>  
> diff --git a/include/uapi/linux/dvb/ca.h b/include/uapi/linux/dvb/ca.h
> index f66ed53f4dc7..a62ddf0cebcd 100644
> --- a/include/uapi/linux/dvb/ca.h
> +++ b/include/uapi/linux/dvb/ca.h
> @@ -109,9 +109,16 @@ struct ca_msg {
> unsigned char msg[256];
>  };
>
> +/**
> + * struct ca_descr - CA descrambler control words info
> + *
> + * @index: CA Descrambler slot
> + * @parity: control words parity, where 0 means even and 1 means odd
> + * @cw: CA Descrambler control words
> + */
>  struct ca_descr {
> unsigned int index;
> -   unsigned int parity;/* 0 == even, 1 == odd */
> +   unsigned int parity;
> unsigned char cw[8];
>  };
>
>

Yeh, it should be that way.

BTW, the only issue I have in mind is how to link particular
descrambler with the PID
after your removal of the CA_SET_PID. And yes, I know that currently we have
no any user of such ioctl in our driver base :)

/Honza