Re: [PATCH v2 14/17] media: v4l2-async: better describe match union at async match struct

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Fri, 13 Oct 2017 15:49:25 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Thursday, 28 September 2017 00:46:57 EEST Mauro Carvalho Chehab wrote:
> > Now that kernel-doc handles nested unions, better document the
> > match union at struct v4l2_async_subdev.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > ---
> >  include/media/v4l2-async.h | 35 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 32 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index e66a3521596f..62c2d572ec23 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -46,10 +46,39 @@ enum v4l2_async_match_type {
> >  /**
> >   * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge
> >   *
> > - * @match_type:	type of match that will be used
> > - * @match:	union of per-bus type matching data sets
> > + * @match_type:
> > + *	type of match that will be used
> > + * @match:
> > + *	union of per-bus type matching data sets  
> 
> The lines don't exceed the 80 columnes limit, you can keep them as-is.

OK.

> 
> > + * @match.fwnode:
> > + *		pointer to &struct fwnode_handle to be matched.
> > + *		Used if @match_type is %V4L2_ASYNC_MATCH_FWNODE.
> > + * @match.device_name:
> > + *		string containing the device name to be matched.
> > + *		Used if @match_type is %V4L2_ASYNC_MATCH_DEVNAME.
> > + * @match.i2c:
> > + *		embedded struct with I2C parameters to be matched.
> > + * 		Both @match.i2c.adapter_id and @match.i2c.address
> > + *		should be matched.
> > + *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.  
> 
> Do you really need to document this ? Isn't it enough to document 
> @match.i2c.adapter_id and @match.i2c.address ?

No. If we don't document a non-anonymous struct, kernel-doc will complain
that a field description is missed. Same applies to the custom field
below.

There's a way to get rid of it: converting it to an anonymous
struct, but I guess that will make the usage of the match rule
harder to understand.

> 
> > + * @match.i2c.adapter_id:
> > + *		I2C adapter ID to be matched.
> > + *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> > + * @match.i2c.address:
> > + *		I2C address to be matched.
> > + *		Used if @match_type is %V4L2_ASYNC_MATCH_I2C.
> > + * @match.custom:
> > + *		Driver-specific match criteria.
> > + *		Used if @match_type is %V4L2_ASYNC_MATCH_CUSTOM.  
> 
> Same here.
> 
> > + * @match.custom.match:
> > + *		Driver-specific match function to be used if
> > + *		%V4L2_ASYNC_MATCH_CUSTOM.
> > + * @match.custom.priv:
> > + *		Driver-specific private struct with match parameters
> > + *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
> >   * @list:	used to link struct v4l2_async_subdev objects, waiting to be
> > - *		probed, to a notifier->waiting list
> > + *		probed, to a notifier->waiting list.
> > + *		Not to be used by drivers.
> >   */
> >  struct v4l2_async_subdev {
> >  	enum v4l2_async_match_type match_type;  
> 

Thanks,
Mauro