[PATCHv2 0/5] media: drop driver_version from media_device

[PATCHv2 0/5] media: drop driver_version from media_device

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Just a little thing that always annoyed me: the driver_version shouldn't
be set in drivers.

The version number never, ever gets updated in drivers. We saw that in
the other media subsystems and now the core always sets it, not drivers.

This works much better, and also works well when backporting the media
code to an older kernel using the media_build system, where the driver
version is set to the kernel version you are backporting from.

So just set the driver_version in media_device_get_info() to
LINUX_VERSION_CODE and drop the driver_version field from struct
media_device.

Regards,

	Hans

Changes since v2:

- just set driver_version in media_device_get_info() and drop it
  in struct media_device.
- combine two lines in atomisp_v4l2.c

Hans Verkuil (5):
  media-device: set driver_version directly
  s3c-camif: don't set driver_version
  uvc: don't set driver_version
  atomisp2: don't set driver_version
  media-device: remove driver_version

 drivers/media/media-device.c                              | 5 +----
 drivers/media/platform/s3c-camif/camif-core.c             | 1 -
 drivers/media/usb/uvc/uvc_driver.c                        | 1 -
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c | 6 +-----
 include/media/media-device.h                              | 2 --
 5 files changed, 2 insertions(+), 13 deletions(-)

-- 
2.13.2

[PATCHv2 1/5] media-device: set driver_version directly

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Don't use driver_version from struct media_device, just return
LINUX_VERSION_CODE as the other media subsystems do.

The driver_version field in struct media_device will be removed
in the following patches.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index fce91b543c14..7ff8e2d5bb07 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -71,7 +71,7 @@ static int media_device_get_info(struct media_device *dev,
 
 	info->media_version = MEDIA_API_VERSION;
 	info->hw_revision = dev->hw_revision;
-	info->driver_version = dev->driver_version;
+	info->driver_version = LINUX_VERSION_CODE;
 
 	return 0;
 }
-- 
2.13.2

[PATCHv2 2/5] s3c-camif: don't set driver_version

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This field will be removed as it is not needed anymore.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/s3c-camif/camif-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/s3c-camif/camif-core.c b/drivers/media/platform/s3c-camif/camif-core.c
index 8f0414041e81..c4ab63986c8f 100644
--- a/drivers/media/platform/s3c-camif/camif-core.c
+++ b/drivers/media/platform/s3c-camif/camif-core.c
@@ -317,7 +317,6 @@ 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 = LINUX_VERSION_CODE;
 
 	md->dev = camif->dev;
 
-- 
2.13.2

[PATCHv2 3/5] uvc: don't set driver_version

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This field will be removed as it is not needed anymore.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 70842c5af05b..4f463bf2b877 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2096,7 +2096,6 @@ static int uvc_probe(struct usb_interface *intf,
 			sizeof(dev->mdev.serial));
 	strcpy(dev->mdev.bus_info, udev->devpath);
 	dev->mdev.hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
-	dev->mdev.driver_version = LINUX_VERSION_CODE;
 	media_device_init(&dev->mdev);
 
 	dev->vdev.mdev = &dev->mdev;
-- 
2.13.2

[PATCHv2 4/5] atomisp2: don't set driver_version

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This field will be removed as it is not needed anymore.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
index 2f49562377e6..663aa916e3ca 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
@@ -1099,9 +1099,7 @@ atomisp_load_firmware(struct atomisp_device *isp)
 		fw_path = "shisp_2400b0_v21.bin";
 
 	if (!fw_path) {
-		dev_err(isp->dev,
-			"Unsupported driver_version 0x%x, hw_revision 0x%x\n",
-			isp->media_dev.driver_version,
+		dev_err(isp->dev, "Unsupported hw_revision 0x%x\n",
 			isp->media_dev.hw_revision);
 		return NULL;
 	}
@@ -1249,8 +1247,6 @@ static int atomisp_pci_probe(struct pci_dev *dev,
 	/* This is not a true PCI device on SoC, so the delay is not needed. */
 	isp->pdev->d3_delay = 0;
 
-	isp->media_dev.driver_version = LINUX_VERSION_CODE;
-
 	switch (id->device & ATOMISP_PCI_DEVICE_SOC_MASK) {
 	case ATOMISP_PCI_DEVICE_SOC_MRFLD:
 		isp->media_dev.hw_revision =
-- 
2.13.2

[PATCHv2 5/5] media-device: remove driver_version

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Since the driver_version field in struct media_device is no longer
used, just remove it.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 3 ---
 include/media/media-device.h | 2 --
 2 files changed, 5 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 7ff8e2d5bb07..979e4307d248 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -833,8 +833,6 @@ void media_device_pci_init(struct media_device *mdev,
 	mdev->hw_revision = (pci_dev->subsystem_vendor << 16)
 			    | pci_dev->subsystem_device;
 
-	mdev->driver_version = LINUX_VERSION_CODE;
-
 	media_device_init(mdev);
 }
 EXPORT_SYMBOL_GPL(media_device_pci_init);
@@ -862,7 +860,6 @@ void __media_device_usb_init(struct media_device *mdev,
 		strlcpy(mdev->serial, udev->serial, sizeof(mdev->serial));
 	usb_make_path(udev, mdev->bus_info, sizeof(mdev->bus_info));
 	mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
-	mdev->driver_version = LINUX_VERSION_CODE;
 
 	media_device_init(mdev);
 }
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 6896266031b9..7d268802cc2e 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -68,7 +68,6 @@ struct media_device_ops {
  * @serial:	Device serial number (optional)
  * @bus_info:	Unique and stable device location identifier
  * @hw_revision: Hardware device revision
- * @driver_version: Device driver version
  * @topology_version: Monotonic counter for storing the version of the graph
  *		topology. Should be incremented each time the topology changes.
  * @id:		Unique ID used on the last registered graph object
@@ -134,7 +133,6 @@ struct media_device {
 	char serial[40];
 	char bus_info[32];
 	u32 hw_revision;
-	u32 driver_version;
 
 	u64 topology_version;
 
-- 
2.13.2

Re: [PATCHv2 1/5] media-device: set driver_version directly

[Hans Verkuil]

On 21/07/17 12:57, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Don't use driver_version from struct media_device, just return
> LINUX_VERSION_CODE as the other media subsystems do.
> 
> The driver_version field in struct media_device will be removed
> in the following patches.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index fce91b543c14..7ff8e2d5bb07 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -71,7 +71,7 @@ static int media_device_get_info(struct media_device *dev,
>  
>  	info->media_version = MEDIA_API_VERSION;

Related question about media_version: would it make sense to change this to
LINUX_VERSION_CODE as well? This too has never been changed from when it was
first introduced, making it pointless as a way for applications to detect
when features were added.

Unfortunately MEDIA_API_VERSION is defined in the public media.h header, but
we can mark it unused. This define isn't documented in the spec, BTW.

Regards,

	Hans

>  	info->hw_revision = dev->hw_revision;
> -	info->driver_version = dev->driver_version;
> +	info->driver_version = LINUX_VERSION_CODE;
>  
>  	return 0;
>  }
> 

Re: [PATCHv2 1/5] media-device: set driver_version directly

[Laurent Pinchart]

Hi Hans,

On Friday 21 Jul 2017 13:06:38 Hans Verkuil wrote:
> On 21/07/17 12:57, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Don't use driver_version from struct media_device, just return
> > LINUX_VERSION_CODE as the other media subsystems do.
> > 
> > The driver_version field in struct media_device will be removed
> > in the following patches.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> > 
> >  drivers/media/media-device.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index fce91b543c14..7ff8e2d5bb07 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -71,7 +71,7 @@ static int media_device_get_info(struct media_device
> > *dev,> 
> >  	info->media_version = MEDIA_API_VERSION;
> 
> Related question about media_version: would it make sense to change this to
> LINUX_VERSION_CODE as well? This too has never been changed from when it was
> first introduced, making it pointless as a way for applications to detect
> when features were added.

Yes, I think it would make sense to do so.

> Unfortunately MEDIA_API_VERSION is defined in the public media.h header, but
> we can mark it unused. This define isn't documented in the spec, BTW.

We really went a long way since the first days of the media controller when it 
comes to API design. That being said, we still have a long way to go :-)

> >  	info->hw_revision = dev->hw_revision;
> > 
> > -	info->driver_version = dev->driver_version;
> > +	info->driver_version = LINUX_VERSION_CODE;
> > 
> >  	return 0;
> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 1/5] media-device: set driver_version directly

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday 21 Jul 2017 12:57:02 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Don't use driver_version from struct media_device, just return
> LINUX_VERSION_CODE as the other media subsystems do.
> 
> The driver_version field in struct media_device will be removed
> in the following patches.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index fce91b543c14..7ff8e2d5bb07 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -71,7 +71,7 @@ static int media_device_get_info(struct media_device *dev,
> 
>  	info->media_version = MEDIA_API_VERSION;
>  	info->hw_revision = dev->hw_revision;
> -	info->driver_version = dev->driver_version;
> +	info->driver_version = LINUX_VERSION_CODE;
> 
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 5/5] media-device: remove driver_version

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday 21 Jul 2017 12:57:06 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Since the driver_version field in struct media_device is no longer
> used, just remove it.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-device.c | 3 ---
>  include/media/media-device.h | 2 --
>  2 files changed, 5 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 7ff8e2d5bb07..979e4307d248 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -833,8 +833,6 @@ void media_device_pci_init(struct media_device *mdev,
>  	mdev->hw_revision = (pci_dev->subsystem_vendor << 16)
> 
>  			    | pci_dev->subsystem_device;
> 
> -	mdev->driver_version = LINUX_VERSION_CODE;
> -
>  	media_device_init(mdev);
>  }
>  EXPORT_SYMBOL_GPL(media_device_pci_init);
> @@ -862,7 +860,6 @@ void __media_device_usb_init(struct media_device *mdev,
>  		strlcpy(mdev->serial, udev->serial, sizeof(mdev->serial));
>  	usb_make_path(udev, mdev->bus_info, sizeof(mdev->bus_info));
>  	mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
> -	mdev->driver_version = LINUX_VERSION_CODE;
> 
>  	media_device_init(mdev);
>  }
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 6896266031b9..7d268802cc2e 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -68,7 +68,6 @@ struct media_device_ops {
>   * @serial:	Device serial number (optional)
>   * @bus_info:	Unique and stable device location identifier
>   * @hw_revision: Hardware device revision
> - * @driver_version: Device driver version
>   * @topology_version: Monotonic counter for storing the version of the
> graph *		topology. Should be incremented each time the topology 
changes. *
> @id:		Unique ID used on the last registered graph object
> @@ -134,7 +133,6 @@ struct media_device {
>  	char serial[40];
>  	char bus_info[32];
>  	u32 hw_revision;
> -	u32 driver_version;
> 
>  	u64 topology_version;

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 2/5] s3c-camif: don't set driver_version

[Sylwester Nawrocki]

On 07/21/2017 12:57 PM, Hans Verkuil wrote:
> From: Hans Verkuil<hans.verkuil@cisco.com>
> 
> This field will be removed as it is not needed anymore.
> 
> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCHv2 5/5] media-device: remove driver_version

[Sylwester Nawrocki]

On 07/21/2017 12:57 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Since the driver_version field in struct media_device is no longer
> used, just remove it.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [PATCHv2 5/5] media-device: remove driver_version

[Sylwester Nawrocki]

On 07/21/2017 12:57 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Since the driver_version field in struct media_device is no longer
> used, just remove it.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>   drivers/media/media-device.c | 3 ---
>   include/media/media-device.h | 2 --
>   2 files changed, 5 deletions(-)

> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 6896266031b9..7d268802cc2e 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -68,7 +68,6 @@ struct media_device_ops {
>    * @serial:	Device serial number (optional)
>    * @bus_info:	Unique and stable device location identifier
>    * @hw_revision: Hardware device revision
> - * @driver_version: Device driver version
>    * @topology_version: Monotonic counter for storing the version of the graph
>    *		topology. Should be incremented each time the topology changes.
>    * @id:		Unique ID used on the last registered graph object
> @@ -134,7 +133,6 @@ struct media_device {
>   	char serial[40];
>   	char bus_info[32];
>   	u32 hw_revision;
> -	u32 driver_version;

It seems we still have such paragraph in include/media/media-device.h:

  *  - &media_entity.driver_version is formatted with the KERNEL_VERSION()
  *    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.

Shouldn't this also be removed?

--
Regards,
Sylwester

Re: [PATCHv2 5/5] media-device: remove driver_version

[Hans Verkuil]

On 22/07/17 13:16, Sylwester Nawrocki wrote:
> On 07/21/2017 12:57 PM, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Since the driver_version field in struct media_device is no longer
>> used, just remove it.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>   drivers/media/media-device.c | 3 ---
>>   include/media/media-device.h | 2 --
>>   2 files changed, 5 deletions(-)
> 
>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>> index 6896266031b9..7d268802cc2e 100644
>> --- a/include/media/media-device.h
>> +++ b/include/media/media-device.h
>> @@ -68,7 +68,6 @@ struct media_device_ops {
>>    * @serial:    Device serial number (optional)
>>    * @bus_info:    Unique and stable device location identifier
>>    * @hw_revision: Hardware device revision
>> - * @driver_version: Device driver version
>>    * @topology_version: Monotonic counter for storing the version of the graph
>>    *        topology. Should be incremented each time the topology changes.
>>    * @id:        Unique ID used on the last registered graph object
>> @@ -134,7 +133,6 @@ struct media_device {
>>       char serial[40];
>>       char bus_info[32];
>>       u32 hw_revision;
>> -    u32 driver_version;
> 
> It seems we still have such paragraph in include/media/media-device.h:
> 
>  *  - &media_entity.driver_version is formatted with the KERNEL_VERSION()
>  *    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.
> 
> Shouldn't this also be removed?

Good catch! Yes, that should be removed as well.

Regards,

	Hans