public inbox for linux-media@vger.kernel.org
 help / color / mirror / Atom feed
* DocBook fixes
@ 2013-01-11 11:26 Hans Verkuil
  2013-01-11 11:26 ` [REVIEWv2 PATCH 1/2] DocBook: fix various validation errors Hans Verkuil
  0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2013-01-11 11:26 UTC (permalink / raw)
  To: linux-media; +Cc: Laurent Pinchart

Two patches: the first fixes a number of validation errors in the DocBook
code (unchanged from v1), the second clarifies how the error_idx is used.

The second patch is almost rewritten completely from v1 based on the
comments from Laurent.

Regards,

        Hans


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [REVIEWv2 PATCH 1/2] DocBook: fix various validation errors
  2013-01-11 11:26 DocBook fixes Hans Verkuil
@ 2013-01-11 11:26 ` Hans Verkuil
  2013-01-11 11:26   ` [REVIEWv2 PATCH 2/2] DocBook: improve the error_idx field documentation Hans Verkuil
  0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2013-01-11 11:26 UTC (permalink / raw)
  To: linux-media; +Cc: Laurent Pinchart, Hans Verkuil

From: Hans Verkuil <hans.verkuil@cisco.com>

Fixed the following errors (with exception of the SVG errors):

  GEN     /home/hans/work/src/v4l/media-git/Documentation/DocBook//v4l2.xml
  rm -rf Documentation/DocBook/index.html; echo '<h1>Linux Kernel HTML Documentation</h1>' >> Documentation/DocBook/index.html && echo '<h2>Kernel Version: 3.8.0-rc1</h2>' >> Documentation/DocBook/index.html && cat Documentation/DocBook/media_api.html >> Documentation/DocBook/index.html
  /tmp/x.xml:883: element revremark: validity error : Element structname is not declared in revremark list of possible children
  /tmp/x.xml:883: element revremark: validity error : Element xref is not declared in revremark list of possible children
  /tmp/x.xml:1829: element footnote: validity error : Element footnote content does not follow the DTD, expecting (calloutlist | glosslist | itemizedlist | orderedlist | segmentedlist | simplelist | variablelist | literallayout | programlisting | programlistingco | screen | screenco | screenshot | synopsis | cmdsynopsis | funcsynopsis | classsynopsis | fieldsynopsis | constructorsynopsis | destructorsynopsis | methodsynopsis | formalpara | para | simpara | address | blockquote | graphic | graphicco | mediaobject | mediaobjectco | informalequation | informalexample | informalfigure | informaltable)+, got (para para errorcode CDATA para )
  /tmp/x.xml:9580: element xref: validity error : Element xref was declared EMPTY this one has content
  /tmp/x.xml:13508: element link: validity error : Element link does not carry attribute linkend
  /tmp/x.xml:13508: element link: validity error : No declaration for attribute linked of element link
  /tmp/x.xml:16986: element imagedata: validity error : Value "SVG" for attribute format of imagedata is not among the enumerated set
  /tmp/x.xml:17003: element imagedata: validity error : Value "SVG" for attribute format of imagedata is not among the enumerated set
  /tmp/x.xml:17022: element imagedata: validity error : Value "SVG" for attribute format of imagedata is not among the enumerated set
  /tmp/x.xml:26795: element refsect1: validity error : Element refsect1 content does not follow the DTD, expecting (refsect1info? , (title , subtitle? , titleabbrev?) , (((calloutlist | glosslist | itemizedlist | orderedlist | segmentedlist | simplelist | variablelist | caution | important | note | tip | warning | literallayout | programlisting | programlistingco | screen | screenco | screenshot | synopsis | cmdsynopsis | funcsynopsis | classsynopsis | fieldsynopsis | constructorsynopsis | destructorsynopsis | methodsynopsis | formalpara | para | simpara | address | blockquote | graphic | graphicco | mediaobject | mediaobjectco | informalequation | informalexample | informalfigure | informaltable | equation | example | figure | table | msgset | procedure | sidebar | qandaset | anchor | bridgehead | remark | highlights | abstract | authorblurb | epigraph | indexterm | beginpage)+ , refsect2*) | refsect2+)), got (section )
  /tmp/x.xml:26852: element refsect1: validity error : Element refsect1 content does not follow the DTD, expecting (refsect1info? , (title , subtitle? , titleabbrev?) , (((calloutlist | glosslist | itemizedlist | orderedlist | segmentedlist | simplelist | variablelist | caution | important | note | tip | warning | literallayout | programlisting | programlistingco | screen | screenco | screenshot | synopsis | cmdsynopsis | funcsynopsis | classsynopsis | fieldsynopsis | constructorsynopsis | destructorsynopsis | methodsynopsis | formalpara | para | simpara | address | blockquote | graphic | graphicco | mediaobject | mediaobjectco | informalequation | informalexample | informalfigure | informaltable | equation | example | figure | table | msgset | procedure | sidebar | qandaset | anchor | bridgehead | remark | highlights | abstract | authorblurb | epigraph | indexterm | beginpage)+ , refsect2*) | refsect2+)), got (table )

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media/v4l/common.xml         |    2 +-
 Documentation/DocBook/media/v4l/io.xml             |    4 +--
 .../DocBook/media/v4l/pixfmt-srggb10alaw8.xml      |    2 +-
 Documentation/DocBook/media/v4l/v4l2.xml           |    5 +---
 Documentation/DocBook/media/v4l/vidioc-expbuf.xml  |   28 +++++++++-----------
 5 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/common.xml b/Documentation/DocBook/media/v4l/common.xml
index 73c6847..ae06afb 100644
--- a/Documentation/DocBook/media/v4l/common.xml
+++ b/Documentation/DocBook/media/v4l/common.xml
@@ -609,7 +609,7 @@ to zero and the <constant>VIDIOC_G_STD</constant>,
 	<para>Applications can make use of the <xref linkend="input-capabilities" /> and
 <xref linkend="output-capabilities"/> flags to determine whether the video standard ioctls
 are available for the device.</para>
-&ENOTTY;.
+
 	<para>See <xref linkend="buffer" /> for a rationale. Probably
 even USB cameras follow some well known video standard. It might have
 been better to explicitly indicate elsewhere if a device cannot live
diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 2c4646d..e6c5855 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -477,7 +477,7 @@ rest should be evident.</para>
 
     <note>
       <title>Experimental</title>
-      <para>This is an <link linkend="experimental"> experimental </link>
+      <para>This is an <link linkend="experimental">experimental</link>
       interface and may change in the future.</para>
     </note>
 
@@ -488,7 +488,7 @@ DMA buffer from userspace using a file descriptor previously exported for a
 different or the same device (known as the importer role), or both. This
 section describes the DMABUF importer role API in V4L2.</para>
 
-    <para>Refer to <link linked="vidioc-expbuf"> DMABUF exporting </link> for
+    <para>Refer to <link linkend="vidioc-expbuf">DMABUF exporting</link> for
 details about exporting V4L2 buffers as DMABUF file descriptors.</para>
 
 <para>Input and output devices support the streaming I/O method when the
diff --git a/Documentation/DocBook/media/v4l/pixfmt-srggb10alaw8.xml b/Documentation/DocBook/media/v4l/pixfmt-srggb10alaw8.xml
index c934192..29acc20 100644
--- a/Documentation/DocBook/media/v4l/pixfmt-srggb10alaw8.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt-srggb10alaw8.xml
@@ -29,6 +29,6 @@
 	    formats with 10 bits per color compressed to 8 bits each,
 	    using the A-LAW algorithm. Each color component consumes 8
 	    bits of memory. In other respects this format is similar to
-	    <xref linkend="V4L2-PIX-FMT-SRGGB8">.</xref></para>
+	    <xref linkend="V4L2-PIX-FMT-SRGGB8"></xref>.</para>
 	  </refsect1>
 	</refentry>
diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml
index 8fe2942..94ab0e1 100644
--- a/Documentation/DocBook/media/v4l/v4l2.xml
+++ b/Documentation/DocBook/media/v4l/v4l2.xml
@@ -143,10 +143,7 @@ applications. -->
 	<revnumber>3.9</revnumber>
 	<date>2012-12-03</date>
 	<authorinitials>sa</authorinitials>
-	<revremark>Added timestamp types to
-	<structname>v4l2_buffer</structname>, see <xref
-	linkend="buffer-flags" />.
-	</revremark>
+	<revremark>Added timestamp types to v4l2_buffer.</revremark>
       </revision>
 
       <revision>
diff --git a/Documentation/DocBook/media/v4l/vidioc-expbuf.xml b/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
index 72dfbd2..e287c8f 100644
--- a/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-expbuf.xml
@@ -83,15 +83,14 @@ descriptor. The application may pass it to other DMABUF-aware devices. Refer to
 <link linkend="dmabuf">DMABUF importing</link> for details about importing
 DMABUF files into V4L2 nodes. It is recommended to close a DMABUF file when it
 is no longer used to allow the associated memory to be reclaimed. </para>
-
   </refsect1>
+
   <refsect1>
-   <section>
-      <title>Examples</title>
+    <title>Examples</title>
 
-      <example>
-	<title>Exporting a buffer.</title>
-	<programlisting>
+    <example>
+      <title>Exporting a buffer.</title>
+      <programlisting>
 int buffer_export(int v4lfd, &v4l2-buf-type; bt, int index, int *dmafd)
 {
 	&v4l2-exportbuffer; expbuf;
@@ -108,12 +107,12 @@ int buffer_export(int v4lfd, &v4l2-buf-type; bt, int index, int *dmafd)
 
 	return 0;
 }
-        </programlisting>
-      </example>
+      </programlisting>
+    </example>
 
-      <example>
-	<title>Exporting a buffer using the multi-planar API.</title>
-	<programlisting>
+    <example>
+      <title>Exporting a buffer using the multi-planar API.</title>
+      <programlisting>
 int buffer_export_mp(int v4lfd, &v4l2-buf-type; bt, int index,
 	int dmafd[], int n_planes)
 {
@@ -137,12 +136,9 @@ int buffer_export_mp(int v4lfd, &v4l2-buf-type; bt, int index,
 
 	return 0;
 }
-        </programlisting>
-      </example>
-   </section>
-  </refsect1>
+      </programlisting>
+    </example>
 
-  <refsect1>
     <table pgwide="1" frame="none" id="v4l2-exportbuffer">
       <title>struct <structname>v4l2_exportbuffer</structname></title>
       <tgroup cols="3">
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [REVIEWv2 PATCH 2/2] DocBook: improve the error_idx field documentation.
  2013-01-11 11:26 ` [REVIEWv2 PATCH 1/2] DocBook: fix various validation errors Hans Verkuil
@ 2013-01-11 11:26   ` Hans Verkuil
  2013-01-11 12:13     ` Laurent Pinchart
  0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2013-01-11 11:26 UTC (permalink / raw)
  To: linux-media; +Cc: Laurent Pinchart, Hans Verkuil

From: Hans Verkuil <hans.verkuil@cisco.com>

The documentation of the error_idx field was incomplete and confusing.
This patch improves it.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |   44 ++++++++++++++++----
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
index 0a4b90f..e9f9735 100644
--- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
@@ -199,13 +199,43 @@ also be zero.</entry>
 	  <row>
 	    <entry>__u32</entry>
 	    <entry><structfield>error_idx</structfield></entry>
-	    <entry>Set by the driver in case of an error. If it is equal
-to <structfield>count</structfield>, then no actual changes were made to
-controls. In other words, the error was not associated with setting a particular
-control. If it is another value, then only the controls up to <structfield>error_idx-1</structfield>
-were modified and control <structfield>error_idx</structfield> is the one that
-caused the error. The <structfield>error_idx</structfield> value is undefined
-if the ioctl returned 0 (success).</entry>
+	    <entry><para>Set by the driver in case of an error. If the error is
+associated with a particular control, then <structfield>error_idx</structfield>
+is set to the index of that control. If the error is not related to a specific
+control, or the pre-validation step failed (see below), then
+<structfield>error_idx</structfield> is set to <structfield>count</structfield>.
+The value is undefined if the ioctl returned 0 (success).</para>
+
+<para>Before controls are read from/written to hardware a pre-validation step
+takes place: this checks if all controls in the list are all valid controls,
+if no attempt is made to write to a read-only control or read from a write-only
+control, and any other up-front checks that can be done without accessing the
+hardware.</para>
+
+<para>This check is done to avoid leaving the hardware in an inconsistent state due
+to easy-to-avoid problems. But it leads to another problem: the application needs to
+know whether an error came from the pre-validation step (meaning that the hardware
+was not touched) or from an error during the actual reading from/writing to hardware.</para>
+
+<para>The, in hindsight quite poor, solution for that is to set <structfield>error_idx</structfield>
+to <structfield>count</structfield> if the pre-validation failed. This has the
+unfortunate side-effect that it is not possible to see which control failed the
+pre-validation. If the pre-validation was successful and the error happened while
+accessing the hardware, then <structfield>error_idx</structfield> is less than
+<structfield>count</structfield> and only the controls up to
+<structfield>error_idx-1</structfield> were read or written correctly, and the
+state of the remaining controls is undefined.</para>
+
+<para>Since <constant>VIDIOC_TRY_EXT_CTRLS</constant> does not access hardware
+there is also no need to handle the pre-validation step in this special way,
+so <structfield>error_idx</structfield> will just be set to the control that
+failed the pre-validation step instead of to <structfield>count</structfield>.
+This means that if <constant>VIDIOC_S_EXT_CTRLS</constant> fails with
+<structfield>error_idx</structfield> set to <structfield>count</structfield>,
+then you can call <constant>VIDIOC_TRY_EXT_CTRLS</constant> to try to discover
+the actual control that failed the pre-validation step. Unfortunately, there
+is no <constant>TRY</constant> equivalent for <constant>VIDIOC_G_EXT_CTRLS</constant>.
+</para></entry>
 	  </row>
 	  <row>
 	    <entry>__u32</entry>
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [REVIEWv2 PATCH 2/2] DocBook: improve the error_idx field documentation.
  2013-01-11 11:26   ` [REVIEWv2 PATCH 2/2] DocBook: improve the error_idx field documentation Hans Verkuil
@ 2013-01-11 12:13     ` Laurent Pinchart
  2013-01-11 12:22       ` Hans Verkuil
  0 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2013-01-11 12:13 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media, Hans Verkuil

Hi Hans,

Thanks for the patch. This is much better in my opinion, please see below for 
two small comments.

On Friday 11 January 2013 12:26:03 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The documentation of the error_idx field was incomplete and confusing.
> This patch improves it.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |   44 +++++++++++++----
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index
> 0a4b90f..e9f9735 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> @@ -199,13 +199,43 @@ also be zero.</entry>
>  	  <row>
>  	    <entry>__u32</entry>
>  	    <entry><structfield>error_idx</structfield></entry>
> -	    <entry>Set by the driver in case of an error. If it is equal
> -to <structfield>count</structfield>, then no actual changes were made to
> -controls. In other words, the error was not associated with setting a
> -particular control. If it is another value, then only the controls up to
> -<structfield>error_idx-1</structfield> were modified and control
> -<structfield>error_idx</structfield> is the one that caused the error. The
> -<structfield>error_idx</structfield> value is undefined if the ioctl
> -returned 0 (success).</entry>
> +	    <entry><para>Set by the driver in case of an error. If the error is
> +associated with a particular control, then
> +<structfield>error_idx</structfield> is set to the index of that control.
> +If the error is not related to a specific control, or the pre-validation
> +step failed (see below), then <structfield>error_idx</structfield> is set
> +to <structfield>count</structfield>. The value is undefined if the ioctl
> +returned 0 (success).</para>
> +
> +<para>Before controls are read from/written to hardware a pre-validation

Maybe s/pre-validation/validation/ through the text ? We have a single 
validation step, it feels a bit weird to talk about pre-validation when 
there's no further validation :-)

> +step takes place: this checks if all controls in the list are all valid

s/all valid/valid/ ?

> +controls, if no attempt is made to write to a read-only control or read
> +from a write-only control, and any other up-front checks that can be done
> +without accessing the hardware.</para>
> +
> +<para>This check is done to avoid leaving the hardware in an inconsistent
> +state due to easy-to-avoid problems. But it leads to another problem: the
> +application needs to know whether an error came from the pre-validation
> +step (meaning that the hardware was not touched) or from an error during
> +the actual reading from/writing to hardware.</para>
> +
> +<para>The, in hindsight quite poor, solution for that is to set
> +<structfield>error_idx</structfield> to <structfield>count</structfield>
> +if the pre-validation failed. This has the unfortunate side-effect that it
> +is not possible to see which control failed the pre-validation. If the
> +pre-validation was successful and the error happened while accessing the
> +hardware, then <structfield>error_idx</structfield> is less than
> +<structfield>count</structfield> and only the controls up to
> +<structfield>error_idx-1</structfield> were read or written correctly, and
> +the state of the remaining controls is undefined.</para>
> +
> +<para>Since <constant>VIDIOC_TRY_EXT_CTRLS</constant> does not access
> +hardware there is also no need to handle the pre-validation step in this
> +special way, so <structfield>error_idx</structfield> will just be set to
> +the control that failed the pre-validation step instead of to
> +<structfield>count</structfield>. This means that if
> +<constant>VIDIOC_S_EXT_CTRLS</constant> fails with
> +<structfield>error_idx</structfield> set to
> +<structfield>count</structfield>, then you can call
> +<constant>VIDIOC_TRY_EXT_CTRLS</constant> to try to discover the actual
> +control that failed the pre-validation step. Unfortunately, there is no
> +<constant>TRY</constant> equivalent for
> +<constant>VIDIOC_G_EXT_CTRLS</constant>. </para></entry>
>  	  </row>
>  	  <row>
>  	    <entry>__u32</entry>
-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [REVIEWv2 PATCH 2/2] DocBook: improve the error_idx field documentation.
  2013-01-11 12:13     ` Laurent Pinchart
@ 2013-01-11 12:22       ` Hans Verkuil
  2013-01-11 12:27         ` Laurent Pinchart
  0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2013-01-11 12:22 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: linux-media, Hans Verkuil

On Fri January 11 2013 13:13:47 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch. This is much better in my opinion, please see below for 
> two small comments.
> 
> On Friday 11 January 2013 12:26:03 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > The documentation of the error_idx field was incomplete and confusing.
> > This patch improves it.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |   44 +++++++++++++----
> >  1 file changed, 37 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index
> > 0a4b90f..e9f9735 100644
> > --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > @@ -199,13 +199,43 @@ also be zero.</entry>
> >  	  <row>
> >  	    <entry>__u32</entry>
> >  	    <entry><structfield>error_idx</structfield></entry>
> > -	    <entry>Set by the driver in case of an error. If it is equal
> > -to <structfield>count</structfield>, then no actual changes were made to
> > -controls. In other words, the error was not associated with setting a
> > -particular control. If it is another value, then only the controls up to
> > -<structfield>error_idx-1</structfield> were modified and control
> > -<structfield>error_idx</structfield> is the one that caused the error. The
> > -<structfield>error_idx</structfield> value is undefined if the ioctl
> > -returned 0 (success).</entry>
> > +	    <entry><para>Set by the driver in case of an error. If the error is
> > +associated with a particular control, then
> > +<structfield>error_idx</structfield> is set to the index of that control.
> > +If the error is not related to a specific control, or the pre-validation
> > +step failed (see below), then <structfield>error_idx</structfield> is set
> > +to <structfield>count</structfield>. The value is undefined if the ioctl
> > +returned 0 (success).</para>
> > +
> > +<para>Before controls are read from/written to hardware a pre-validation
> 
> Maybe s/pre-validation/validation/ through the text ? We have a single 
> validation step, it feels a bit weird to talk about pre-validation when 
> there's no further validation :-)

OK.

> > +step takes place: this checks if all controls in the list are all valid
> 
> s/all valid/valid/ ?

Indeed.

> 
> > +controls, if no attempt is made to write to a read-only control or read
> > +from a write-only control, and any other up-front checks that can be done
> > +without accessing the hardware.</para>

How about adding this sentence to the end of the paragraph:

"The exact validations done during this step are driver dependent since some
checks might require hardware access for some devices, thus making it impossible
to do those checks up-front. However, drivers should make a best-effort to do
as many up-front checks as possible."

Regards,

	Hans

> > +
> > +<para>This check is done to avoid leaving the hardware in an inconsistent
> > +state due to easy-to-avoid problems. But it leads to another problem: the
> > +application needs to know whether an error came from the pre-validation
> > +step (meaning that the hardware was not touched) or from an error during
> > +the actual reading from/writing to hardware.</para>
> > +
> > +<para>The, in hindsight quite poor, solution for that is to set
> > +<structfield>error_idx</structfield> to <structfield>count</structfield>
> > +if the pre-validation failed. This has the unfortunate side-effect that it
> > +is not possible to see which control failed the pre-validation. If the
> > +pre-validation was successful and the error happened while accessing the
> > +hardware, then <structfield>error_idx</structfield> is less than
> > +<structfield>count</structfield> and only the controls up to
> > +<structfield>error_idx-1</structfield> were read or written correctly, and
> > +the state of the remaining controls is undefined.</para>
> > +
> > +<para>Since <constant>VIDIOC_TRY_EXT_CTRLS</constant> does not access
> > +hardware there is also no need to handle the pre-validation step in this
> > +special way, so <structfield>error_idx</structfield> will just be set to
> > +the control that failed the pre-validation step instead of to
> > +<structfield>count</structfield>. This means that if
> > +<constant>VIDIOC_S_EXT_CTRLS</constant> fails with
> > +<structfield>error_idx</structfield> set to
> > +<structfield>count</structfield>, then you can call
> > +<constant>VIDIOC_TRY_EXT_CTRLS</constant> to try to discover the actual
> > +control that failed the pre-validation step. Unfortunately, there is no
> > +<constant>TRY</constant> equivalent for
> > +<constant>VIDIOC_G_EXT_CTRLS</constant>. </para></entry>
> >  	  </row>
> >  	  <row>
> >  	    <entry>__u32</entry>
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [REVIEWv2 PATCH 2/2] DocBook: improve the error_idx field documentation.
  2013-01-11 12:22       ` Hans Verkuil
@ 2013-01-11 12:27         ` Laurent Pinchart
  0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2013-01-11 12:27 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media, Hans Verkuil

Hi Hans,

On Friday 11 January 2013 13:22:08 Hans Verkuil wrote:
> On Fri January 11 2013 13:13:47 Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > Thanks for the patch. This is much better in my opinion, please see below
> > for two small comments.
> > 
> > On Friday 11 January 2013 12:26:03 Hans Verkuil wrote:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > The documentation of the error_idx field was incomplete and confusing.
> > > This patch improves it.
> > > 
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > > 
> > >  .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |   44
> > >  +++++++++++++----
> > >  1 file changed, 37 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > > b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index
> > > 0a4b90f..e9f9735 100644
> > > --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > > +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
> > > @@ -199,13 +199,43 @@ also be zero.</entry>
> > > 
> > >  	  <row>
> > >  	  
> > >  	    <entry>__u32</entry>
> > >  	    <entry><structfield>error_idx</structfield></entry>
> > > 
> > > -	    <entry>Set by the driver in case of an error. If it is equal
> > > -to <structfield>count</structfield>, then no actual changes were made
> > > -to controls. In other words, the error was not associated with setting
> > > -a particular control. If it is another value, then only the controls up
> > > -to <structfield>error_idx-1</structfield> were modified and control
> > > -<structfield>error_idx</structfield> is the one that caused the error.
> > > -The <structfield>error_idx</structfield> value is undefined if the
> > > -ioctl returned 0 (success).</entry>
> > > +	    <entry><para>Set by the driver in case of an error. If the error
> > > +is associated with a particular control, then
> > > +<structfield>error_idx</structfield> is set to the index of that
> > > +control. If the error is not related to a specific control, or the
> > > +pre-validation step failed (see below), then
> > > +<structfield>error_idx</structfield> is set to
> > > +<structfield>count</structfield>. The value is undefined if the ioctl
> > > +returned 0 (success).</para>
> > > +
> > > +<para>Before controls are read from/written to hardware a
> > > +pre-validation
> > 
> > Maybe s/pre-validation/validation/ through the text ? We have a single
> > validation step, it feels a bit weird to talk about pre-validation when
> > there's no further validation :-)
> 
> OK.
> 
> > > +step takes place: this checks if all controls in the list are all valid
> > 
> > s/all valid/valid/ ?
> 
> Indeed.
> 
> > > +controls, if no attempt is made to write to a read-only control or read
> > > +from a write-only control, and any other up-front checks that can be
> > > done
> > > +without accessing the hardware.</para>
> 
> How about adding this sentence to the end of the paragraph:
> 
> "The exact validations done during this step are driver dependent since some
> checks might require hardware access for some devices, thus making it
> impossible to do those checks up-front. However, drivers should make a
> best-effort to do as many up-front checks as possible."

Sounds very good to me. With all those changes,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-01-11 12:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-11 11:26 DocBook fixes Hans Verkuil
2013-01-11 11:26 ` [REVIEWv2 PATCH 1/2] DocBook: fix various validation errors Hans Verkuil
2013-01-11 11:26   ` [REVIEWv2 PATCH 2/2] DocBook: improve the error_idx field documentation Hans Verkuil
2013-01-11 12:13     ` Laurent Pinchart
2013-01-11 12:22       ` Hans Verkuil
2013-01-11 12:27         ` Laurent Pinchart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox