Re: [PATCH] media: do not use C++ style comments in uapi headers

From: Mauro Carvalho Chehab
Date: Tue Jun 04 2019 - 10:46:21 EST

Next message: Mr.Asita.Hussain: "Hello my good friend,"
Previous message: Catalin Marinas: "Re: [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture"
In reply to: Joe Perches: "Re: [PATCH] media: do not use C++ style comments in uapi headers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Tue, 4 Jun 2019 20:13:34 +0900
Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> escreveu:

> Linux kernel tolerates C++ style comments these days. Actually, the
> SPDX License tags for .c files start with //.
>
> On the other hand, uapi headers are written in more strict C, where
> the C++ comment style is forbidden.

I've seen that the current comments are more a discussion about
c++ style against "normal c" comments.

I don't have any strong opinion about using or not c++ style comments
on headers.

From my side, though, I would prefer to use // for SPDX everywhere,
as using it on C, but avoiding on headers doesn't seem a good idea
to me, as people can easily mess with it, but that's just my two
cents.

-

Yet, with respect to those headers:

include/uapi/linux/dvb/video.h
include/uapi/linux/dvb/osd.h
include/uapi/linux/dvb/audio.h

They belong to an old deprecated API, with just two drivers using it, with
was meant to control MPEG decoder hardware:

1) the av7110 DVB legacy driver - it is for a hardware developed on the
previous millennium and with is not manufactured anymore for a long time.
We didn't remove it yet just because we don't have any strong reason why
doing it;

2) by the V4L2 ivtv driver (with is also for a legacy driver) - with
doesn't support Digital TV. This was a first approach to control its
internal MPEG decoder and/or encoder. This was replaced several years
ago by another API designed to work with MPEG encoders/decoders,
using a more generic approach. The deprecated API is there just for
the sake of not breaking anything on userspace.

So, if you're working on it just to sanitize the headers, that's
fine, but please don't use them on any future project.

Regards,
Mauro

>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> ---
>
> include/uapi/linux/dvb/audio.h | 2 +-
> include/uapi/linux/dvb/osd.h | 170 ++++++++++++++++++++-------------
> 2 files changed, 103 insertions(+), 69 deletions(-)
>
> diff --git a/include/uapi/linux/dvb/audio.h b/include/uapi/linux/dvb/audio.h
> index afeae063e640..977bed135e22 100644
> --- a/include/uapi/linux/dvb/audio.h
> +++ b/include/uapi/linux/dvb/audio.h
> @@ -52,7 +52,7 @@ typedef enum {
> typedef struct audio_mixer {
> unsigned int volume_left;
> unsigned int volume_right;
> - // what else do we need? bass, pass-through, ...
> + /* what else do we need? bass, pass-through, ... */
> } audio_mixer_t;
>
>
> diff --git a/include/uapi/linux/dvb/osd.h b/include/uapi/linux/dvb/osd.h
> index e163508b9ae8..723824ce3622 100644
> --- a/include/uapi/linux/dvb/osd.h
> +++ b/include/uapi/linux/dvb/osd.h
> @@ -28,74 +28,108 @@
> #include <linux/compiler.h>
>
> typedef enum {
> - // All functions return -2 on "not open"
> - OSD_Close=1, // ()
> - // Disables OSD and releases the buffers
> - // returns 0 on success
> - OSD_Open, // (x0,y0,x1,y1,BitPerPixel[2/4/8](color&0x0F),mix[0..15](color&0xF0))
> - // Opens OSD with this size and bit depth
> - // returns 0 on success, -1 on DRAM allocation error, -2 on "already open"
> - OSD_Show, // ()
> - // enables OSD mode
> - // returns 0 on success
> - OSD_Hide, // ()
> - // disables OSD mode
> - // returns 0 on success
> - OSD_Clear, // ()
> - // Sets all pixel to color 0
> - // returns 0 on success
> - OSD_Fill, // (color)
> - // Sets all pixel to color <col>
> - // returns 0 on success
> - OSD_SetColor, // (color,R{x0},G{y0},B{x1},opacity{y1})
> - // set palette entry <num> to <r,g,b>, <mix> and <trans> apply
> - // R,G,B: 0..255
> - // R=Red, G=Green, B=Blue
> - // opacity=0: pixel opacity 0% (only video pixel shows)
> - // opacity=1..254: pixel opacity as specified in header
> - // opacity=255: pixel opacity 100% (only OSD pixel shows)
> - // returns 0 on success, -1 on error
> - OSD_SetPalette, // (firstcolor{color},lastcolor{x0},data)
> - // Set a number of entries in the palette
> - // sets the entries "firstcolor" through "lastcolor" from the array "data"
> - // data has 4 byte for each color:
> - // R,G,B, and a opacity value: 0->transparent, 1..254->mix, 255->pixel
> - OSD_SetTrans, // (transparency{color})
> - // Sets transparency of mixed pixel (0..15)
> - // returns 0 on success
> - OSD_SetPixel, // (x0,y0,color)
> - // sets pixel <x>,<y> to color number <col>
> - // returns 0 on success, -1 on error
> - OSD_GetPixel, // (x0,y0)
> - // returns color number of pixel <x>,<y>, or -1
> - OSD_SetRow, // (x0,y0,x1,data)
> - // fills pixels x0,y through x1,y with the content of data[]
> - // returns 0 on success, -1 on clipping all pixel (no pixel drawn)
> - OSD_SetBlock, // (x0,y0,x1,y1,increment{color},data)
> - // fills pixels x0,y0 through x1,y1 with the content of data[]
> - // inc contains the width of one line in the data block,
> - // inc<=0 uses blockwidth as linewidth
> - // returns 0 on success, -1 on clipping all pixel
> - OSD_FillRow, // (x0,y0,x1,color)
> - // fills pixels x0,y through x1,y with the color <col>
> - // returns 0 on success, -1 on clipping all pixel
> - OSD_FillBlock, // (x0,y0,x1,y1,color)
> - // fills pixels x0,y0 through x1,y1 with the color <col>
> - // returns 0 on success, -1 on clipping all pixel
> - OSD_Line, // (x0,y0,x1,y1,color)
> - // draw a line from x0,y0 to x1,y1 with the color <col>
> - // returns 0 on success
> - OSD_Query, // (x0,y0,x1,y1,xasp{color}}), yasp=11
> - // fills parameters with the picture dimensions and the pixel aspect ratio
> - // returns 0 on success
> - OSD_Test, // ()
> - // draws a test picture. for debugging purposes only
> - // returns 0 on success
> -// TODO: remove "test" in final version
> - OSD_Text, // (x0,y0,size,color,text)
> - OSD_SetWindow, // (x0) set window with number 0<x0<8 as current
> - OSD_MoveWindow, // move current window to (x0, y0)
> - OSD_OpenRaw, // Open other types of OSD windows
> + /* All functions return -2 on "not open" */
> + OSD_Close=1, /* () */
> + /*
> + * Disables OSD and releases the buffers
> + * returns 0 on success
> + */
> + OSD_Open, /* (x0,y0,x1,y1,BitPerPixel[2/4/8](color&0x0F),mix[0..15](color&0xF0)) */
> + /*
> + * Opens OSD with this size and bit depth
> + * returns 0 on success, -1 on DRAM allocation error, -2 on "already open"
> + */
> + OSD_Show, /* () */
> + /*
> + * enables OSD mode
> + * returns 0 on success
> + */
> + OSD_Hide, /* () */
> + /*
> + * disables OSD mode
> + * returns 0 on success
> + */
> + OSD_Clear, /* () */
> + /*
> + * Sets all pixel to color 0
> + * returns 0 on success
> + */
> + OSD_Fill, /* (color) */
> + /*
> + * Sets all pixel to color <col>
> + * returns 0 on success
> + */
> + OSD_SetColor, /* (color,R{x0},G{y0},B{x1},opacity{y1}) */
> + /*
> + * set palette entry <num> to <r,g,b>, <mix> and <trans> apply
> + * R,G,B: 0..255
> + * R=Red, G=Green, B=Blue
> + * opacity=0: pixel opacity 0% (only video pixel shows)
> + * opacity=1..254: pixel opacity as specified in header
> + * opacity=255: pixel opacity 100% (only OSD pixel shows)
> + * returns 0 on success, -1 on error
> + */
> + OSD_SetPalette, /* (firstcolor{color},lastcolor{x0},data) */
> + /*
> + * Set a number of entries in the palette
> + * sets the entries "firstcolor" through "lastcolor" from the array "data"
> + * data has 4 byte for each color:
> + * R,G,B, and a opacity value: 0->transparent, 1..254->mix, 255->pixel
> + */
> + OSD_SetTrans, /* (transparency{color}) */
> + /*
> + * Sets transparency of mixed pixel (0..15)
> + * returns 0 on success
> + */
> + OSD_SetPixel, /* (x0,y0,color) */
> + /*
> + * sets pixel <x>,<y> to color number <col>
> + * returns 0 on success, -1 on error
> + */
> + OSD_GetPixel, /* (x0,y0) */
> + /* returns color number of pixel <x>,<y>, or -1 */
> + OSD_SetRow, /* (x0,y0,x1,data) */
> + /*
> + * fills pixels x0,y through x1,y with the content of data[]
> + * returns 0 on success, -1 on clipping all pixel (no pixel drawn)
> + */
> + OSD_SetBlock, /* (x0,y0,x1,y1,increment{color},data) */
> + /*
> + * fills pixels x0,y0 through x1,y1 with the content of data[]
> + * inc contains the width of one line in the data block,
> + * inc<=0 uses blockwidth as linewidth
> + * returns 0 on success, -1 on clipping all pixel
> + */
> + OSD_FillRow, /* (x0,y0,x1,color) */
> + /*
> + * fills pixels x0,y through x1,y with the color <col>
> + * returns 0 on success, -1 on clipping all pixel
> + */
> + OSD_FillBlock, /* (x0,y0,x1,y1,color) */
> + /*
> + * fills pixels x0,y0 through x1,y1 with the color <col>
> + * returns 0 on success, -1 on clipping all pixel
> + */
> + OSD_Line, /* (x0,y0,x1,y1,color) */
> + /*
> + * draw a line from x0,y0 to x1,y1 with the color <col>
> + * returns 0 on success
> + */
> + OSD_Query, /* (x0,y0,x1,y1,xasp{color}}), yasp=11 */
> + /*
> + * fills parameters with the picture dimensions and the pixel aspect ratio
> + * returns 0 on success
> + */
> + OSD_Test, /* () */
> + /*
> + * draws a test picture. for debugging purposes only
> + * returns 0 on success
> + * TODO: remove "test" in final version
> + */
> + OSD_Text, /* (x0,y0,size,color,text) */
> + OSD_SetWindow, /* (x0) set window with number 0<x0<8 as current */
> + OSD_MoveWindow, /* move current window to (x0, y0) */
> + OSD_OpenRaw, /* Open other types of OSD windows */
> } OSD_Command;
>
> typedef struct osd_cmd_s {

Thanks,
Mauro

Next message: Mr.Asita.Hussain: "Hello my good friend,"
Previous message: Catalin Marinas: "Re: [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture"
In reply to: Joe Perches: "Re: [PATCH] media: do not use C++ style comments in uapi headers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]