Re: [PATCH v2 08/17] media: v4l2-ioctl.h: convert debug macros into enum and document

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

Em Mon, 18 Dec 2017 17:32:11 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday, 18 December 2017 17:13:26 EET Mauro Carvalho Chehab wrote:
> > Em Fri, 13 Oct 2017 15:38:11 +0300 Laurent Pinchart escreveu:  
> > > On Thursday, 28 September 2017 00:46:51 EEST Mauro Carvalho Chehab wrote:  
> > >> Currently, there's no way to document #define foo <value>
> > >> with kernel-doc. So, convert it to an enum, and document.  
> > > 
> > > The documentation seems fine to me (except for one comment below).
> > > However, converting macros to an enum just to work around a defect of the
> > > documentation system doesn't seem like a good idea to me. I'd rather find
> > > a way to document macros.  
> > 
> > I agree that this limitation should be fixed.
> > 
> > Yet, in this specific case where we have an "array" of defines, all
> > associated to the same field (even being a bitmask), and assuming that
> > we would add a way for kernel-doc to parse this kind of defines
> > (not sure how easy/doable would be), then, in order to respect the
> > way kernel-doc markup is, the documentation for those macros would be:
> > 
> > 
> > /**
> >  * define: Just log the ioctl name + error code
> >  */
> > #define V4L2_DEV_DEBUG_IOCTL		0x01
> > /**
> >  * define: Log the ioctl name arguments + error code
> >  */
> > #define V4L2_DEV_DEBUG_IOCTL_ARG	0x02
> > /**
> >  * define: Log the file operations open, release, mmap and get_unmapped_area
> > */
> > #define V4L2_DEV_DEBUG_FOP		0x04
> > /**
> >  * define: Log the read and write file operations and the VIDIOC_(D)QBUF
> > ioctls */
> > #define V4L2_DEV_DEBUG_STREAMING	0x08
> > 
> > IMHO, this is a way easier to read/understand by humans, and a way more
> > coincise:
> > 
> > /**
> >  * enum v4l2_debug_flags - Device debug flags to be used with the video
> >  *	device debug attribute
> >  *
> >  * @V4L2_DEV_DEBUG_IOCTL:	Just log the ioctl name + error code.
> >  * @V4L2_DEV_DEBUG_IOCTL_ARG:	Log the ioctl name arguments + error code.
> >  * @V4L2_DEV_DEBUG_FOP:		Log the file operations and open, release,
> >  *				mmap and get_unmapped_area syscalls.
> >  * @V4L2_DEV_DEBUG_STREAMING:	Log the read and write syscalls and
> >  *				:c:ref:`VIDIOC_[Q|DQ]BUFF <VIDIOC_QBUF>` ioctls.
> >  */
> > 
> > It also underlines the aspect that those names are grouped altogether.
> > 
> > So, IMHO, the main reason to place them inside an enum and document
> > as such is that it looks a way better for humans to read.  
> 
> As we're talking about extending kerneldoc to document macros, we're free to 
> decide on a format that would make it easy and clear. Based on your above 
> example, we could write it
> 
> /**
>  * define v4l2_debug_flags - Device debug flags to be used with the video
>  *	device debug attribute
>  *
>  * @V4L2_DEV_DEBUG_IOCTL:	Just log the ioctl name + error code.
>  * @V4L2_DEV_DEBUG_IOCTL_ARG:	Log the ioctl name arguments + error code.
>  * @V4L2_DEV_DEBUG_FOP:		Log the file operations and open, release,
>  *				mmap and get_unmapped_area syscalls.
>  * @V4L2_DEV_DEBUG_STREAMING:	Log the read and write syscalls and
>  *				:c:ref:`VIDIOC_[Q|DQ]BUFF <VIDIOC_QBUF>` ioctls.
>  */
> 
> That would be simple, clear, and wouldn't require a code change to workaround 
> a limitation of the documentation system.

That works for me but that would be against the way other things are
parsed by kernel-doc.

Besides that, I suspect that a patchset adding support for it at
kernel-doc will be painful, as it currently assumes that a
	 /**
	  * ...
	  */

markup refers just to the first non-blank line just below it. So, such
patch will likely need to add a hack in order to support it, but I may
be wrong.

I guess we'll end by needing support for #define foo bar type of defines
some day. 

Anyway, I suspect that, for those debug macros, the best is to change
it to an enum of bits. I sent a proposal with such approach:

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

Thanks,
Mauro