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

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

[Johannes Thumshirn]

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

[PATCH RESEND 1/7] [media] media: introduce MEDIA_REVISION macro

[Johannes Thumshirn]

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 <jthumshirn@suse.de>
---
 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

[PATCH RESEND 2/7] video: fbdev: don't use KERNEL_VERSION macro for MEDIA_REVISION

[Johannes Thumshirn]

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

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 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

[PATCH RESEND 3/7] [media] media: document the use of MEDIA_REVISION instead of KERNEL_VERSION

[Johannes Thumshirn]

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 <jthumshirn@suse.de>
---
 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

[PATCH RESEND 4/7] [media] cx25821: use MEDIA_REVISION instead of KERNEL_VERSION

[Johannes Thumshirn]

Use MEDIA_REVISION instead of KERNEL_VERSION to encode the
CX25821_VERSION_CODE.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 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

[PATCH RESEND 5/7] [media] media: s3c-camif: Use MEDIA_REVISON instead of KERNEL_VERSION

[Johannes Thumshirn]

Use MEDIA_REVISON instead of KERNEL_VERSION to encode the driver
version.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 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

[PATCH RESEND 6/7] [media] media: bcm2048: use MEDIA_REVISION isntead of KERNEL_VERSION

[Johannes Thumshirn]

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

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 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

[PATCH RESEND 7/7] staging/atomisp: use MEDIA_VERSION instead of KERNEL_VERSION

[Johannes Thumshirn]

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

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 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

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

[Mauro Carvalho Chehab]

Em Wed, 21 Jun 2017 10:08:05 +0200
Johannes Thumshirn <jthumshirn@suse.de> 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

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

[Johannes Thumshirn]

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

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

[Stephen Hemminger]

On Thu, 29 Jun 2017 11:42:59 +0200
Johannes Thumshirn <jthumshirn@suse.de> wrote:

> 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

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().

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

[Johannes Thumshirn]

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