Re: [REVIEWv2 PATCH 2/2] DocBook: improve the error_idx field documentation.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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