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@xxxxxxxxx> > > The documentation of the error_idx field was incomplete and confusing. > This patch improves it. > > Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > --- > .../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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
