Re: [PATCH 15/18] [media] uapi/media.h: Rename entities types to functions

From: Hans Verkuil <hverkuil@xs4all.nl>
To: Mauro Carvalho Chehab <mchehab@osg.samsung.com>,
	Linux Media Mailing List <linux-media@vger.kernel.org>
Cc: "Sakari Ailus" <sakari.ailus@iki.fi>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	"Kyungmin Park" <kyungmin.park@samsung.com>,
	"Heungjun Kim" <riverful.kim@samsung.com>,
	"Lad, Prabhakar" <prabhakar.csengg@gmail.com>,
	"Andrzej Hajda" <a.hajda@samsung.com>,
	"Hyun Kwon" <hyun.kwon@xilinx.com>,
	"Michal Simek" <michal.simek@xilinx.com>,
	"Sören Brinkmann" <soren.brinkmann@xilinx.com>,
	"Hans Verkuil" <hans.verkuil@cisco.com>,
	"Rafael Lourenço de Lima Chehab" <chehabrafael@gmail.com>,
	"Krzysztof Kozlowski" <k.kozlowski@samsung.com>,
	"Boris BREZILLON" <boris.brezillon@free-electrons.com>,
	"Joe Perches" <joe@perches.com>,
	"Sylwester Nawrocki" <s.nawrocki@samsung.com>,
	"Ricardo Ribalda Delgado" <ricardo.ribalda@gmail.com>,
	"Guennadi Liakhovetski" <g.liakhovetski@gmx.de>,
	"Axel Lin" <axel.lin@ingics.com>,
	"Shuah Khan" <shuahkh@osg.samsung.com>,
	"Julia Lawall" <Julia.Lawall@lip6.fr>,
	"Markus Elfring" <elfring@users.sourceforge.net>,
	"Matthias Schwarzott" <zzam@gentoo.org>,
	"Antti Palosaari" <crope@iki.fi>,
	"Olli Salonen" <olli.salonen@iki.fi>,
	"Tommi Rantala" <tt.rantala@gmail.com>,
	"Scott Jiang" <scott.jiang.linux@gmail.com>,
	"Bryan Wu" <cooloney@gmail.com>,
	"Jacek Anaszewski" <j.anaszewski@samsung.com>,
	linux-arm-kernel@lists.infradead.org, linux-api@vger.kernel.org
Subject: Re: [PATCH 15/18] [media] uapi/media.h: Rename entities types to functions
Date: Fri, 11 Sep 2015 17:36:42 +0200	[thread overview]
Message-ID: <55F2F50A.3030502@xs4all.nl> (raw)
In-Reply-To: <0545064c26ab320b6019adf1ff24e8d69339d682.1441559233.git.mchehab@osg.samsung.com>

On 09/06/2015 07:30 PM, Mauro Carvalho Chehab wrote:
> Rename the userspace types from MEDIA_ENT_T_ to MEDIA_ENT_F_
> and add the backward compatibility bits.
> 
> The changes at the .c files was generated by the following
> coccinelle script:
> 

> @@
> -MEDIA_ENT_T_DVB_DEMUX
> +MEDIA_ENT_F_MPEG_TS_DEMUX

I'm not sure about the 'MPEG_' part here. I think that in general a transport stream
can contain non-MPEG streams as well. Why not just say _F_TS_DEMUX?

> @@
> @@
> -MEDIA_ENT_T_DVB_TSOUT
> +MEDIA_ENT_F_DTV_TSOUT

Shouldn't this be MEDIA_ENT_F_IO?

> @@
> @@
> -MEDIA_ENT_T_DVB_CA
> +MEDIA_ENT_F_DTV_CA
> @@
> @@
> -MEDIA_ENT_T_DVB_NET_DECAP
> +MEDIA_ENT_F_DTV_NET_DECAP
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index e925909bc99e..8527fc40e6a0 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -229,7 +229,7 @@ static int dvb_create_tsout_entity(struct dvb_device *dvbdev,
>  		if (!entity->name)
>  			return ret;
>  
> -		entity->function = MEDIA_ENT_T_DVB_TSOUT;
> +		entity->function = MEDIA_ENT_F_IO;
>  		pads->flags = MEDIA_PAD_FL_SINK;
>  
>  		ret = media_entity_init(entity, 1, pads);
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index d232cc680c67..90e90a6e62bf 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -46,87 +46,86 @@ struct media_device_info {
>   * Initial value to be used when a new entity is created
>   * Drivers should change it to something useful
>   */
> -#define MEDIA_ENT_T_UNKNOWN	0x00000000
> +#define MEDIA_ENT_F_UNKNOWN	0x00000000
>  
>  /*
> - * Base numbers for entity types
> + * Base number ranges for entity functions
>   *
> - * Please notice that the huge gap of 16 bits for each base is overkill!
> - * 8 bits is more than enough to avoid starving entity types for each
> - * subsystem.
> - *
> - * However, It is kept this way just to avoid binary breakages with the
> - * namespace provided on legacy versions of this header.
> + * NOTE: those ranges and entity function number are spased just to

s/spased/phased/

> + * make easier to maintain this file. Userspace should not rely on

s/make/make it/

> + * the ranges to identify a group of function types, as newer
> + * functions can be added with any name within the full u32 range.
>   */
> -#define MEDIA_ENT_T_DVB_BASE		0x00000000
> -#define MEDIA_ENT_T_V4L2_BASE		0x00010000
> -#define MEDIA_ENT_T_V4L2_SUBDEV_BASE	0x00020000
> -#define MEDIA_ENT_T_CONNECTOR_BASE	0x00030000
> +#define MEDIA_ENT_F_BASE		0x00000000
> +#define MEDIA_ENT_F_OLD_BASE		0x00010000
> +#define MEDIA_ENT_F_OLD_SUBDEV_BASE	0x00020000
>  
>  /*
> - * V4L2 entities - Those are used for DMA (mmap/DMABUF) and
> - *	read()/write() data I/O associated with the V4L2 devnodes.
> + * DVB entities
>   */
> -#define MEDIA_ENT_T_V4L2_VIDEO		(MEDIA_ENT_T_V4L2_BASE + 1)
> -	/*
> -	 * Please notice that numbers between MEDIA_ENT_T_V4L2_BASE + 2 and
> -	 * MEDIA_ENT_T_V4L2_BASE + 4 can't be used, as those values used
> -	 * to be declared for FB, ALSA and DVB entities.
> -	 * As those values were never actually used in practice, we're just
> -	 * adding them as backward compatibility macros and keeping the
> -	 * numberspace clean here. This way, we avoid breaking compilation,
> -	 * in the case of having some userspace application using the old
> -	 * symbols.
> -	 */
> -#define MEDIA_ENT_T_V4L2_VBI		(MEDIA_ENT_T_V4L2_BASE + 5)
> -#define MEDIA_ENT_T_V4L2_SWRADIO	(MEDIA_ENT_T_V4L2_BASE + 6)
> -
> -/* V4L2 Sub-device entities */
> -
> -	/*
> -	 * Subdevs are initialized with MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN,
> -	 * in order to preserve backward compatibility.
> -	 * Drivers should change to the proper subdev type before
> -	 * registering the entity.
> -	 */
> -#define MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_T_V4L2_SUBDEV_BASE
> -
> -#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 1)
> -#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 2)
> -#define MEDIA_ENT_T_V4L2_SUBDEV_LENS	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 3)
> -	/* A converter of analogue video to its digital representation. */
> -#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 4)
> -	/* Tuner entity is actually both V4L2 and DVB subdev */
> -#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	(MEDIA_ENT_T_V4L2_SUBDEV_BASE + 5)
> +#define MEDIA_ENT_F_DTV_DEMOD		(MEDIA_ENT_F_BASE + 1)
> +#define MEDIA_ENT_F_MPEG_TS_DEMUX	(MEDIA_ENT_F_BASE + 2)
> +#define MEDIA_ENT_F_DTV_CA		(MEDIA_ENT_F_BASE + 3)
> +#define MEDIA_ENT_F_DTV_NET_DECAP	(MEDIA_ENT_F_BASE + 4)
>  
> -/* DVB entities */
> -#define MEDIA_ENT_T_DVB_DEMOD		(MEDIA_ENT_T_DVB_BASE + 1)
> -#define MEDIA_ENT_T_DVB_DEMUX		(MEDIA_ENT_T_DVB_BASE + 2)
> -#define MEDIA_ENT_T_DVB_TSOUT		(MEDIA_ENT_T_DVB_BASE + 3)
> -#define MEDIA_ENT_T_DVB_CA		(MEDIA_ENT_T_DVB_BASE + 4)
> -#define MEDIA_ENT_T_DVB_NET_DECAP	(MEDIA_ENT_T_DVB_BASE + 5)
> -
> -/* Connectors */
> -#define MEDIA_ENT_T_CONN_RF		(MEDIA_ENT_T_CONNECTOR_BASE)
> -#define MEDIA_ENT_T_CONN_SVIDEO		(MEDIA_ENT_T_CONNECTOR_BASE + 1)
> -#define MEDIA_ENT_T_CONN_COMPOSITE	(MEDIA_ENT_T_CONNECTOR_BASE + 2)
> +/*
> + * Connectors
> + */
> +#define MEDIA_ENT_F_CONN_RF		(MEDIA_ENT_F_BASE + 21)
> +#define MEDIA_ENT_F_CONN_SVIDEO		(MEDIA_ENT_F_BASE + 22)
> +#define MEDIA_ENT_F_CONN_COMPOSITE	(MEDIA_ENT_F_BASE + 23)
>  	/* For internal test signal generators and other debug connectors */
> -#define MEDIA_ENT_T_CONN_TEST		(MEDIA_ENT_T_CONNECTOR_BASE + 3)
> +#define MEDIA_ENT_F_CONN_TEST		(MEDIA_ENT_F_BASE + 24)
> +
> +/*
> + * Don't touch on those. The ranges MEDIA_ENT_F_OLD_BASE and
> + * MEDIA_ENT_F_OLD_SUBDEV_BASE are kept to keep backward compatibility
> + * with the legacy v1 API.The number range is out of range by purpose:
> + * several previously reserved numbers got excluded from this range.
> + *
> + * Subdevs are initialized with MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN,
> + * in order to preserve backward compatibility.
> + * Drivers should change to the proper subdev type before
> + * registering the entity.
> + */
> +
> +#define MEDIA_ENT_F_IO  		(MEDIA_ENT_F_OLD_BASE + 1)
> +
> +#define MEDIA_ENT_F_CAM_SENSOR		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 1)
> +#define MEDIA_ENT_F_FLASH		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
> +#define MEDIA_ENT_F_LENS		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
> +#define MEDIA_ENT_F_ATV_DECODER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
> +#define MEDIA_ENT_F_TUNER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
> +
> +#define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_F_OLD_SUBDEV_BASE
>  
>  #ifndef __KERNEL__
> -/* Legacy symbols used to avoid userspace compilation breakages */
> +
> +/*
> + * Legacy symbols used to avoid userspace compilation breakages
> + *
> + * Those symbols map the entity function into types and should be
> + * used only on legacy programs for legacy hardware. Don't rely
> + * on those for MEDIA_IOC_G_TOPOLOGY.
> + */
>  #define MEDIA_ENT_TYPE_SHIFT		16
>  #define MEDIA_ENT_TYPE_MASK		0x00ff0000
>  #define MEDIA_ENT_SUBTYPE_MASK		0x0000ffff
>  
> -#define MEDIA_ENT_T_DEVNODE		MEDIA_ENT_T_V4L2_BASE
> -#define MEDIA_ENT_T_V4L2_SUBDEV		MEDIA_ENT_T_V4L2_SUBDEV_BASE
> -
> -#define MEDIA_ENT_T_DEVNODE_V4L		MEDIA_ENT_T_V4L2_VIDEO
> -
> +#define MEDIA_ENT_T_DEVNODE		MEDIA_ENT_F_OLD_BASE
> +#define MEDIA_ENT_T_DEVNODE_V4L		MEDIA_ENT_F_IO
>  #define MEDIA_ENT_T_DEVNODE_FB		(MEDIA_ENT_T_DEVNODE + 2)
>  #define MEDIA_ENT_T_DEVNODE_ALSA	(MEDIA_ENT_T_DEVNODE + 3)
>  #define MEDIA_ENT_T_DEVNODE_DVB		(MEDIA_ENT_T_DEVNODE + 4)
> +
> +#define MEDIA_ENT_T_UNKNOWN		MEDIA_ENT_F_UNKNOWN
> +#define MEDIA_ENT_T_V4L2_VIDEO		MEDIA_ENT_F_IO
> +#define MEDIA_ENT_T_V4L2_SUBDEV		MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN
> +#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR	MEDIA_ENT_F_CAM_SENSOR
> +#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH	MEDIA_ENT_F_FLASH
> +#define MEDIA_ENT_T_V4L2_SUBDEV_LENS	MEDIA_ENT_F_LENS
> +#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	MEDIA_ENT_F_ATV_DECODER
> +#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	MEDIA_ENT_F_TUNER
>  #endif
>  
>  /* Entity flags */
> 

Overall I really like this.

Regards,

	Hans