[PATCH 0/4] media: set driver_version in media_device_init

[PATCH 0/4] media: set driver_version in media_device_init

[Hans Verkuil]

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

Just a little thing that always annoyed me: the driver_version should
be set in media_device_init, not in the drivers themselves.

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.

Note: the media_device driver_version does not appear to be set in the
omap3isp driver, so I assume it returns 0 as the version? Is that indeed
the case or did I miss something?

Regards,

	Hans

Hans Verkuil (4):
  media-device: set driver_version in media_device_init
  s3c-camif: don't set driver_version anymore
  uvc: don't set driver_version anymore
  atomisp2: don't set driver_version anymore

 drivers/media/media-device.c                              | 4 +---
 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 | 5 +----
 4 files changed, 2 insertions(+), 9 deletions(-)

-- 
2.13.2

[PATCH 1/4] media-device: set driver_version in media_device_init

[Hans Verkuil]

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

Set the driver_version to LINUX_VERSION_CODE in the media_device_init
call, just as the other media subsystems do.

There is no point in doing anything else, since version numbers that
are set by drivers are never, ever updated. LINUX_VERSION_CODE will
be updated, and is also set correctly when backporting the media
subsystem to an older kernel using the media_build system.

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

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index fce91b543c14..2beffe3e3464 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -681,6 +681,7 @@ void media_device_init(struct media_device *mdev)
 	INIT_LIST_HEAD(&mdev->entity_notify);
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
+	mdev->driver_version = LINUX_VERSION_CODE;
 
 	dev_dbg(mdev->dev, "Media device initialized\n");
 }
@@ -833,8 +834,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 +861,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);
 }
-- 
2.13.2

[PATCH 2/4] s3c-camif: don't set driver_version anymore

[Hans Verkuil]

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

This is now set by media_device_init.

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

[PATCH 3/4] uvc: don't set driver_version anymore

[Hans Verkuil]

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

This is now set by media_device_init.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.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

[PATCH 4/4] atomisp2: don't set driver_version anymore

[Hans Verkuil]

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

This is now set by media_device_init.

Drop the print of driver_version in the error message: the driver
version is 1) not yet set at this point (the media_device_init call
comes later AFAICS), and 2) irrelevant here, since it is the hw_revision
that is important, not the driver version (which is identical to the
kernel version anyway).

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c | 5 +----
 1 file changed, 1 insertion(+), 4 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..29387c03fae9 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
@@ -1100,8 +1100,7 @@ atomisp_load_firmware(struct atomisp_device *isp)
 
 	if (!fw_path) {
 		dev_err(isp->dev,
-			"Unsupported driver_version 0x%x, hw_revision 0x%x\n",
-			isp->media_dev.driver_version,
+			"Unsupported hw_revision 0x%x\n",
 			isp->media_dev.hw_revision);
 		return NULL;
 	}
@@ -1249,8 +1248,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

Re: [PATCH 4/4] atomisp2: don't set driver_version anymore

[Laurent Pinchart]

Hi Hans,

On Friday 21 Jul 2017 11:02:34 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This is now set by media_device_init.
> 
> Drop the print of driver_version in the error message: the driver
> version is 1) not yet set at this point (the media_device_init call
> comes later AFAICS), and 2) irrelevant here, since it is the hw_revision
> that is important, not the driver version (which is identical to the
> kernel version anyway).
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c | 5 +----
>  1 file changed, 1 insertion(+), 4 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..29387c03fae9 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> @@ -1100,8 +1100,7 @@ atomisp_load_firmware(struct atomisp_device *isp)
> 
>  	if (!fw_path) {
>  		dev_err(isp->dev,
> -			"Unsupported driver_version 0x%x, hw_revision 0x%x\n",
> -			isp->media_dev.driver_version,
> +			"Unsupported hw_revision 0x%x\n",

Nitpicking, you can now merge the first two lines.

>  			isp->media_dev.hw_revision);
>  		return NULL;
>  	}
> @@ -1249,8 +1248,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 =

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] uvc: don't set driver_version anymore

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday 21 Jul 2017 11:02:33 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This is now set by media_device_init.
> 
> 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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media-device: set driver_version in media_device_init

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Friday 21 Jul 2017 11:02:31 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Set the driver_version to LINUX_VERSION_CODE in the media_device_init
> call, just as the other media subsystems do.
> 
> There is no point in doing anything else, since version numbers that
> are set by drivers are never, ever updated. LINUX_VERSION_CODE will
> be updated, and is also set correctly when backporting the media
> subsystem to an older kernel using the media_build system.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Wouldn't it be even better to drop the driver_version field completely from 
struct media_device, as it's now hardcoded ?

> ---
>  drivers/media/media-device.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index fce91b543c14..2beffe3e3464 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -681,6 +681,7 @@ void media_device_init(struct media_device *mdev)
>  	INIT_LIST_HEAD(&mdev->entity_notify);
>  	mutex_init(&mdev->graph_mutex);
>  	ida_init(&mdev->entity_internal_idx);
> +	mdev->driver_version = LINUX_VERSION_CODE;
> 
>  	dev_dbg(mdev->dev, "Media device initialized\n");
>  }
> @@ -833,8 +834,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 +861,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);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media-device: set driver_version in media_device_init

[Hans Verkuil]

On 21/07/17 12:12, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Friday 21 Jul 2017 11:02:31 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Set the driver_version to LINUX_VERSION_CODE in the media_device_init
>> call, just as the other media subsystems do.
>>
>> There is no point in doing anything else, since version numbers that
>> are set by drivers are never, ever updated. LINUX_VERSION_CODE will
>> be updated, and is also set correctly when backporting the media
>> subsystem to an older kernel using the media_build system.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Wouldn't it be even better to drop the driver_version field completely from 
> struct media_device, as it's now hardcoded ?

True, good point. I'll change this.

Regards,

	Hans

> 
>> ---
>>  drivers/media/media-device.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index fce91b543c14..2beffe3e3464 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -681,6 +681,7 @@ void media_device_init(struct media_device *mdev)
>>  	INIT_LIST_HEAD(&mdev->entity_notify);
>>  	mutex_init(&mdev->graph_mutex);
>>  	ida_init(&mdev->entity_internal_idx);
>> +	mdev->driver_version = LINUX_VERSION_CODE;
>>
>>  	dev_dbg(mdev->dev, "Media device initialized\n");
>>  }
>> @@ -833,8 +834,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 +861,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);
>>  }
>