[PATCH RESEND 0/7] Introduce MEDIA_VERSION to end KENREL_VERSION abuse in media

Currently the media subsystem has a very creative abuse of the
KERNEL_VERSION macro to encode an arbitrary version triplet for media
drivers and device hardware revisions.

This series introduces a new macro called MEDIA_REVISION which encodes
a version triplet like KERNEL_VERSION does, but clearly has media
centric semantics and doesn't fool someone into thinking specific
parts are defined for a specific kernel version only like in out of
tree drivers.

Johannes Thumshirn (7):
  [media] media: introduce MEDIA_REVISION macro
  video: fbdev: don't use KERNEL_VERSION macro for MEDIA_REVISION
  [media] media: document the use of MEDIA_REVISION instead of
    KERNEL_VERSION
  [media] cx25821: use MEDIA_REVISION instead of KERNEL_VERSION
  [media] media: s3c-camif: Use MEDIA_REVISON instead of KERNEL_VERSION
  [media] media: bcm2048: use MEDIA_REVISION isntead of KERNEL_VERSION
  staging/atomisp: use MEDIA_VERSION instead of KERNEL_VERSION

 Documentation/media/uapi/cec/cec-ioc-adap-g-caps.rst        | 2 +-
 Documentation/media/uapi/mediactl/media-ioc-device-info.rst | 4 ++--
 Documentation/media/uapi/v4l/vidioc-querycap.rst            | 6 +++---
 drivers/media/pci/cx25821/cx25821.h                         | 2 +-
 drivers/media/platform/s3c-camif/camif-core.c               | 2 +-
 drivers/staging/media/atomisp/include/linux/atomisp.h       | 6 +++---
 drivers/staging/media/bcm2048/radio-bcm2048.c               | 2 +-
 drivers/video/fbdev/matrox/matroxfb_base.c                  | 3 ++-
 include/media/media-device.h                                | 5 ++---
 include/uapi/linux/media.h                                  | 4 +++-
 10 files changed, 19 insertions(+), 17 deletions(-)

-- 
2.12.3

Currently the media code abuses the KERNEL_VERSION macro to encode a
version triplet.

Introduce a MEDIA_REVISION macro to get rid of the confusing and
creative KERNEL_VERSION usage in the media subsystem.

Signed-off-by: Johannes Thumshirn <redacted>
---
 include/uapi/linux/media.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 4890787731b8..25e2ae4432bd 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h

@@ -30,7 +30,9 @@
 #include <linux/types.h>
 #include <linux/version.h>
 
-#define MEDIA_API_VERSION	KERNEL_VERSION(0, 1, 0)
+#define MEDIA_REVISION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
+
+#define MEDIA_API_VERSION	MEDIA_REVISION(0, 1, 0)
 
 struct media_device_info {
 	char driver[16];

-- 
2.12.3

Use MEDIA_REVISION instead of KERNEL_VERSION to encode the
CX25821_VERSION_CODE.

Signed-off-by: Johannes Thumshirn <redacted>
---
 drivers/media/pci/cx25821/cx25821.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/cx25821/cx25821.h b/drivers/media/pci/cx25821/cx25821.h
index 0f20e89b0cde..7fea6b07cf19 100644
--- a/drivers/media/pci/cx25821/cx25821.h
+++ b/drivers/media/pci/cx25821/cx25821.h

@@ -41,7 +41,7 @@
 #include <linux/version.h>
 #include <linux/mutex.h>
 
-#define CX25821_VERSION_CODE KERNEL_VERSION(0, 0, 106)
+#define CX25821_VERSION_CODE MEDIA_REVISION(0, 0, 106)
 
 #define UNSET (-1U)
 #define NO_SYNC_LINE (-1U)

-- 
2.12.3

Use MEDIA_VERSION instead of KERNEL_VERSION to encode the driver
version of the Atom ISP driver.

Signed-off-by: Johannes Thumshirn <redacted>
---
 drivers/staging/media/atomisp/include/linux/atomisp.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
index 35865462ccf9..0b664ee6f825 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp.h

@@ -30,9 +30,9 @@
 
 /* struct media_device_info.driver_version */
 #define ATOMISP_CSS_VERSION_MASK	0x00ffffff
-#define ATOMISP_CSS_VERSION_15		KERNEL_VERSION(1, 5, 0)
-#define ATOMISP_CSS_VERSION_20		KERNEL_VERSION(2, 0, 0)
-#define ATOMISP_CSS_VERSION_21		KERNEL_VERSION(2, 1, 0)
+#define ATOMISP_CSS_VERSION_15		MEDIA_REVISION(1, 5, 0)
+#define ATOMISP_CSS_VERSION_20		MEDIA_REVISION(2, 0, 0)
+#define ATOMISP_CSS_VERSION_21		MEDIA_REVISION(2, 1, 0)
 
 /* struct media_device_info.hw_revision */
 #define ATOMISP_HW_REVISION_MASK	0x0000ff00

-- 
2.12.3

Use MEDIA_REVISON instead of KERNEL_VERSION to encode the driver
version.

Signed-off-by: Johannes Thumshirn <redacted>
---
 drivers/media/platform/s3c-camif/camif-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s3c-camif/camif-core.c b/drivers/media/platform/s3c-camif/camif-core.c
index ec4001970313..98bd5719fdf5 100644
--- a/drivers/media/platform/s3c-camif/camif-core.c
+++ b/drivers/media/platform/s3c-camif/camif-core.c

@@ -317,7 +317,7 @@ static int camif_media_dev_init(struct camif_dev *camif)
 		 ip_rev = S3C6410_CAMIF_IP_REV ? "6410" : "244X");
 	strlcpy(md->bus_info, "platform", sizeof(md->bus_info));
 	md->hw_revision = ip_rev;
-	md->driver_version = KERNEL_VERSION(1, 0, 0);
+	md->driver_version = MEDIA_REVISION(1, 0, 0);
 
 	md->dev = camif->dev;
 

-- 
2.12.3

Use MEDIA_REVISION isntead of KERNEL_VERSION to encode the bcm2048
driver version.

Signed-off-by: Johannes Thumshirn <redacted>
---
 drivers/staging/media/bcm2048/radio-bcm2048.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/bcm2048/radio-bcm2048.c b/drivers/staging/media/bcm2048/radio-bcm2048.c
index 38f72d069e27..ffe9b63cbf59 100644
--- a/drivers/staging/media/bcm2048/radio-bcm2048.c
+++ b/drivers/staging/media/bcm2048/radio-bcm2048.c

@@ -48,7 +48,7 @@
 /* driver definitions */
 #define BCM2048_DRIVER_AUTHOR	"Eero Nurkkala <ext-eero.nurkkala@nokia.com>"
 #define BCM2048_DRIVER_NAME	BCM2048_NAME
-#define BCM2048_DRIVER_VERSION	KERNEL_VERSION(0, 0, 1)
+#define BCM2048_DRIVER_VERSION	MEDIA_REVISION(0, 0, 1)
 #define BCM2048_DRIVER_CARD	"Broadcom bcm2048 FM Radio Receiver"
 #define BCM2048_DRIVER_DESC	"I2C driver for BCM2048 FM Radio Receiver"
 

-- 
2.12.3

Update the documentation to introduce the use of MEDIA_REVISON instead
of KERNEL_VERSION for the verison triplets of a media drivers hardware
revision or driver version.

Signed-off-by: Johannes Thumshirn <redacted>
---
 Documentation/media/uapi/cec/cec-ioc-adap-g-caps.rst        | 2 +-
 Documentation/media/uapi/mediactl/media-ioc-device-info.rst | 4 ++--
 Documentation/media/uapi/v4l/vidioc-querycap.rst            | 6 +++---
 include/media/media-device.h                                | 5 ++---
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/Documentation/media/uapi/cec/cec-ioc-adap-g-caps.rst b/Documentation/media/uapi/cec/cec-ioc-adap-g-caps.rst
index a0e961f11017..749054f11c77 100644
--- a/Documentation/media/uapi/cec/cec-ioc-adap-g-caps.rst
+++ b/Documentation/media/uapi/cec/cec-ioc-adap-g-caps.rst

@@ -56,7 +56,7 @@ returns the information to the application. The ioctl never fails.
 	:ref:`cec-capabilities`.
     * - __u32
       - ``version``
-      - CEC Framework API version, formatted with the ``KERNEL_VERSION()``
+      - CEC Framework API version, formatted with the ``MEDIA_REVISION()``
 	macro.
 
 

diff --git a/Documentation/media/uapi/mediactl/media-ioc-device-info.rst b/Documentation/media/uapi/mediactl/media-ioc-device-info.rst
index f690f9afc470..7a18cb6dbe84 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-device-info.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-device-info.rst

@@ -96,7 +96,7 @@ ioctl never fails.
 
        -  ``media_version``
 
-       -  Media API version, formatted with the ``KERNEL_VERSION()`` macro.
+       -  Media API version, formatted with the ``MEDIA_REVISION()`` macro.
 
     -  .. row 6
 

@@ -113,7 +113,7 @@ ioctl never fails.
        -  ``driver_version``
 
        -  Media device driver version, formatted with the
-	  ``KERNEL_VERSION()`` macro. Together with the ``driver`` field
+	  ``MEDIA_REVISION()`` macro. Together with the ``driver`` field
 	  this identifies a particular driver.
 
     -  .. row 8

diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
index 12e0d9a63cd8..b66d9caa7211 100644
--- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst

@@ -90,13 +90,13 @@ specification the ioctl returns an ``EINVAL`` error code.
 	example, a stable or distribution-modified kernel uses the V4L2
 	stack from a newer kernel.
 
-	The version number is formatted using the ``KERNEL_VERSION()``
+	The version number is formatted using the ``MEDIA_REVISION()``
 	macro:
     * - :cspan:`2`
 
-	``#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))``
+	``#define MEDIA_REVISION(a,b,c) (((a) << 16) + ((b) << 8) + (c))``
 
-	``__u32 version = KERNEL_VERSION(0, 8, 1);``
+	``__u32 version = MEDIA_REVISION(0, 8, 1);``
 
 	``printf ("Version: %u.%u.%u\\n",``
 

diff --git a/include/media/media-device.h b/include/media/media-device.h
index 6896266031b9..6b3057266ad1 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h

@@ -247,9 +247,9 @@ void media_device_cleanup(struct media_device *mdev);
  *
  *  - &media_entity.hw_revision is the hardware device revision in a
  *    driver-specific format. When possible the revision should be formatted
- *    with the KERNEL_VERSION() macro.
+ *    with the MEDIA_REVISION() macro.
  *
- *  - &media_entity.driver_version is formatted with the KERNEL_VERSION()
+ *  - &media_entity.driver_version is formatted with the MEDIA_REVISION()
  *    macro. The version minor must be incremented when new features are added
  *    to the userspace API without breaking binary compatibility. The version
  *    major must be incremented when binary compatibility is broken.

@@ -265,7 +265,6 @@ void media_device_cleanup(struct media_device *mdev);
 int __must_check __media_device_register(struct media_device *mdev,
 					 struct module *owner);
 
-
 /**
  * media_device_register() - Registers a media device element
  *

-- 
2.12.3

Don't use the KERNEL_VERSION() macro for the v4l2 capabilities, use
MEDIA_REVISION instead.

Signed-off-by: Johannes Thumshirn <redacted>
---
 drivers/video/fbdev/matrox/matroxfb_base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/matrox/matroxfb_base.c b/drivers/video/fbdev/matrox/matroxfb_base.c
index 11eb094396ae..eb92a325033c 100644
--- a/drivers/video/fbdev/matrox/matroxfb_base.c
+++ b/drivers/video/fbdev/matrox/matroxfb_base.c

@@ -113,6 +113,7 @@
 #include <linux/interrupt.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/media.h>
 
 #ifdef CONFIG_PPC_PMAC
 #include <asm/machdep.h>

@@ -1091,7 +1092,7 @@ static int matroxfb_ioctl(struct fb_info *info,
 				strcpy(r.driver, "matroxfb");
 				strcpy(r.card, "Matrox");
 				sprintf(r.bus_info, "PCI:%s", pci_name(minfo->pcidev));
-				r.version = KERNEL_VERSION(1,0,0);
+				r.version = MEDIA_REVISION(1, 0, 0);
 				r.capabilities = V4L2_CAP_VIDEO_OUTPUT;
 				if (copy_to_user(argp, &r, sizeof(r)))
 					return -EFAULT;

-- 
2.12.3

Em Wed, 21 Jun 2017 10:08:05 +0200
Johannes Thumshirn [off-list ref] escreveu:

Currently the media subsystem has a very creative abuse of the
KERNEL_VERSION macro to encode an arbitrary version triplet for media
drivers and device hardware revisions.

This series introduces a new macro called MEDIA_REVISION which encodes
a version triplet like KERNEL_VERSION does, but clearly has media
centric semantics and doesn't fool someone into thinking specific
parts are defined for a specific kernel version only like in out of
tree drivers.

Sorry, but I can't see any advantage on it. On the downside, it
includes the media controller header file (media.h) where it
is not needed.

Johannes Thumshirn (7):
  [media] media: introduce MEDIA_REVISION macro
  video: fbdev: don't use KERNEL_VERSION macro for MEDIA_REVISION
  [media] media: document the use of MEDIA_REVISION instead of
    KERNEL_VERSION
  [media] cx25821: use MEDIA_REVISION instead of KERNEL_VERSION
  [media] media: s3c-camif: Use MEDIA_REVISON instead of KERNEL_VERSION
  [media] media: bcm2048: use MEDIA_REVISION isntead of KERNEL_VERSION
  staging/atomisp: use MEDIA_VERSION instead of KERNEL_VERSION

That's said, some of the above shouldn't be using KERNEL_VERSION
at all. The V4L2 core sets the version already. So, drivers like
cx25821, s3c-camif, bcm2048 and atomisp are likely doing the wrong
thing.

Thanks,
Mauro

On Sat, Jun 24, 2017 at 05:15:07PM -0300, Mauro Carvalho Chehab wrote:

Sorry, but I can't see any advantage on it. On the downside, it
includes the media controller header file (media.h) where it
is not needed.

My reasoning was the differences in semantics. KERNEL_VERSION() is for
encoding the kernel's version triplet not a API or Hardware or whatever
version. Other subsystems do this as well, for instance in NVMe we have the
NVME_VS() macro which is used to encode the NVMe Spec compliance from a human
readable form to the hardware's u32. Also KERNEL_VERISON() shouldn't have
in-tree users IMHO. Yes there is _one_ other user of it in-tree which is EXT4
and I already talked to Jan Kara about it and we decided to leave it in until
4.20.

Byte,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

On Thu, 29 Jun 2017 11:42:59 +0200
Johannes Thumshirn [off-list ref] wrote:

On Sat, Jun 24, 2017 at 05:15:07PM -0300, Mauro Carvalho Chehab wrote:

quoted

Sorry, but I can't see any advantage on it. On the downside, it
includes the media controller header file (media.h) where it
is not needed.  

My reasoning was the differences in semantics. KERNEL_VERSION() is for
encoding the kernel's version triplet not a API or Hardware or whatever
version. Other subsystems do this as well, for instance in NVMe we have the
NVME_VS() macro which is used to encode the NVMe Spec compliance from a human
readable form to the hardware's u32. Also KERNEL_VERISON() shouldn't have
in-tree users IMHO. Yes there is _one_ other user of it in-tree which is EXT4
and I already talked to Jan Kara about it and we decided to leave it in until
4.20.

Byte,
	Johannes

If you read Linus's comments on version.
Driver version is meaningless and there is a desire to rip it out of all
drivers. The reason is that drivers must always behave the same, i.e you
can't use version to change API/ABI behavior. 

Any upstream driver should never use KERNEL_VERSION().

On Thu, Jun 29, 2017 at 10:01:05AM -0700, Stephen Hemminger wrote:

If you read Linus's comments on version.
Driver version is meaningless and there is a desire to rip it out of all
drivers. The reason is that drivers must always behave the same, i.e you
can't use version to change API/ABI behavior. 

Indeed this causes more harm than good. We had support calls regarding the
mlx4 driver because of not incremented MODLE_VERSION()s. If we follow your
and Linus' path we shouldn't just get rid of the KERNEL_VERSION() usage
in media and replace it with a new version, but kill all the versioning
stuff out of media (and others) except for maybe the HW version.

Any upstream driver should never use KERNEL_VERSION().

Exactly my reasoning.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850