[PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[Mark Balantzyan]

Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>
---
This patch adds a custom function to release video device in assignment to vdev->release member in tw686x driver.

 drivers/media/pci/tw686x/tw686x-video.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
index 3a06c000..3631d0f5 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,6 +1151,21 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned long requests,
 	}
 }
 
+
+
+void tw686x_video_device_release(struct tw686x_video_channel *vc) {
+	struct tw686x_dev *dev = vc->dev;
+	unsigned int ch, pb;
+
+	for (ch = 0; ch < max_channels(dev); ch++) {
+		struct tw686x_video_channel *vc = &dev->video_channels[ch];
+
+	dev->dma_ops->free;
+	
+	video_device_release((struct video_device*)dev);
+
+}
+
 void tw686x_video_free(struct tw686x_dev *dev)
 {
 	unsigned int ch, pb;
@@ -1160,9 +1175,6 @@ void tw686x_video_free(struct tw686x_dev *dev)
 
 		video_unregister_device(vc->device);
 
-		if (dev->dma_ops->free)
-			for (pb = 0; pb < 2; pb++)
-				dev->dma_ops->free(vc, pb);
 	}
 }
 
@@ -1277,7 +1289,7 @@ int tw686x_video_init(struct tw686x_dev *dev)
 		snprintf(vdev->name, sizeof(vdev->name), "%s video", dev->name);
 		vdev->fops = &tw686x_video_fops;
 		vdev->ioctl_ops = &tw686x_video_ioctl_ops;
-		vdev->release = video_device_release;
+		vdev->release = tw686x_video_device_release;
 		vdev->v4l2_dev = &dev->v4l2_dev;
 		vdev->queue = &vc->vidq;
 		vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
-- 
2.17.1

Re: [PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[Ezequiel Garcia]

Hi Mark,

On Wed, 24 Jul 2019 at 12:10, Mark Balantzyan <mbalant3@gmail.com> wrote:
>

This commit needs to be thoroughly explained in order to make sense.

> Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>
> ---
> This patch adds a custom function to release video device in assignment to vdev->release member in tw686x driver.
>
>  drivers/media/pci/tw686x/tw686x-video.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 3a06c000..3631d0f5 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -1151,6 +1151,21 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned long requests,
>         }
>  }
>
> +
> +
> +void tw686x_video_device_release(struct tw686x_video_channel *vc) {
> +       struct tw686x_dev *dev = vc->dev;
> +       unsigned int ch, pb;
> +
> +       for (ch = 0; ch < max_channels(dev); ch++) {
> +               struct tw686x_video_channel *vc = &dev->video_channels[ch];
> +
> +       dev->dma_ops->free;
> +

While I certainly appreciate your intention, you should at least test build
your patch.

You might want to read Documentation/process/ and get some insight on
upstreaming good practices.

Thanks,
Eze

> +       video_device_release((struct video_device*)dev);
> +
> +}
> +
>  void tw686x_video_free(struct tw686x_dev *dev)
>  {
>         unsigned int ch, pb;
> @@ -1160,9 +1175,6 @@ void tw686x_video_free(struct tw686x_dev *dev)
>
>                 video_unregister_device(vc->device);
>
> -               if (dev->dma_ops->free)
> -                       for (pb = 0; pb < 2; pb++)
> -                               dev->dma_ops->free(vc, pb);
>         }
>  }
>
> @@ -1277,7 +1289,7 @@ int tw686x_video_init(struct tw686x_dev *dev)
>                 snprintf(vdev->name, sizeof(vdev->name), "%s video", dev->name);
>                 vdev->fops = &tw686x_video_fops;
>                 vdev->ioctl_ops = &tw686x_video_ioctl_ops;
> -               vdev->release = video_device_release;
> +               vdev->release = tw686x_video_device_release;
>                 vdev->v4l2_dev = &dev->v4l2_dev;
>                 vdev->queue = &vc->vidq;
>                 vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
> --
> 2.17.1
>

Re: [PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[Mark Balançian]

Hi Ezequiel,

I just test built the kernel with my patch included and things run fine. 
To guide me in my assignment, could you please clarify what Hans Verkuil 
had been mentioning would be a good thing to code, he said in one of our 
previous emails:

What tw686x_video_free() does really should be done in the release function
of the video_device: vdev->release is currently set to video_device_release,
but that should be a custom function that calls dev->dma_ops->free.

But in order to provide a thorough explanation that makes sense, as you 
mentioned and that I'd like to do, does the custom function and edits I 
made provide the functionality Hans mentioned, as per how it is seen in 
the code below?:

(Please note I didn't provide a description this time as the purpose of 
this email is to get a better idea of how to make one)

 From 6d38673ca7d2206b21deb7d28971f52ee8453346 Mon Sep 17 00:00:00 2001
From: Mark Balantzyan <mbalant3@gmail.com>
Date: Wed, 24 Jul 2019 14:01:30 -0700
Subject: [PATCH] media input infrastructure:tw686x: Added custom 
function to provide dev->dma_ops->free for vdev->release in 
tw686x_video_init() in tw686x driver

---
  drivers/media/pci/tw686x/tw686x-video.c | 24 +++++++++++++++++++-----
  1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c
index 3a06c000..1b875a7c 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1151,18 +1151,32 @@ void tw686x_video_irq(struct tw686x_dev *dev, 
unsigned long requests,
      }
  }

+void tw686x_video_device_release(struct tw686x_video_channel *vc) {
+
+    struct tw686x_dev *dev = vc->dev;
+
+    unsigned int ch;
+
+    for (ch = 0; ch < max_channels(dev); ch++) {
+        struct tw686x_video_channel *vc = &dev->video_channels[ch];
+
+    }
+
+    dev->dma_ops->free;
+
+    video_device_release((struct video_device*)dev);
+
+}
+
  void tw686x_video_free(struct tw686x_dev *dev)
  {
-    unsigned int ch, pb;
+    unsigned int ch;

      for (ch = 0; ch < max_channels(dev); ch++) {
          struct tw686x_video_channel *vc = &dev->video_channels[ch];

          video_unregister_device(vc->device);

-        if (dev->dma_ops->free)
-            for (pb = 0; pb < 2; pb++)
-                dev->dma_ops->free(vc, pb);
      }
  }

@@ -1277,7 +1291,7 @@ int tw686x_video_init(struct tw686x_dev *dev)
          snprintf(vdev->name, sizeof(vdev->name), "%s video", dev->name);
          vdev->fops = &tw686x_video_fops;
          vdev->ioctl_ops = &tw686x_video_ioctl_ops;
-        vdev->release = video_device_release;
+        vdev->release = tw686x_video_device_release;
          vdev->v4l2_dev = &dev->v4l2_dev;
          vdev->queue = &vc->vidq;
          vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
-- 
2.17.1

Re: [PATCH] media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8839 bytes --]

Hi Mark,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.3-rc1 next-20190726]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mark-Balantzyan/media-input-infrastructure-tw686x-Added-Added-custom-function-to-set-vdev-release-in-tw686x-driver/20190727-005525
base:   git://linuxtv.org/media_tree.git master
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/media/pci/tw686x/tw686x-video.c: In function 'tw686x_video_device_release':
   drivers/media/pci/tw686x/tw686x-video.c:1156:14: warning: statement with no effect [-Wunused-value]
     dev->dma_ops->free;
     ~~~~~~~~~~~~^~~~~~
   drivers/media/pci/tw686x/tw686x-video.c:1154:32: warning: unused variable 'vc' [-Wunused-variable]
      struct tw686x_video_channel *vc = &dev->video_channels[ch];
                                   ^~
   drivers/media/pci/tw686x/tw686x-video.c: In function 'tw686x_video_free':
   drivers/media/pci/tw686x/tw686x-video.c:1164:19: warning: unused variable 'pb' [-Wunused-variable]
     unsigned int ch, pb;
                      ^~
   drivers/media/pci/tw686x/tw686x-video.c: In function 'tw686x_video_device_release':
>> drivers/media/pci/tw686x/tw686x-video.c:1162:1: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    void tw686x_video_free(struct tw686x_dev *dev)
    ^~~~
   drivers/media/pci/tw686x/tw686x-video.c: In function 'tw686x_video_init':
>> drivers/media/pci/tw686x/tw686x-video.c:1285:17: error: assignment from incompatible pointer type [-Werror=incompatible-pointer-types]
      vdev->release = tw686x_video_device_release;
                    ^
   drivers/media/pci/tw686x/tw686x-video.c: In function 'tw686x_video_device_release':
>> drivers/media/pci/tw686x/tw686x-video.c:1321:1: error: expected declaration or statement at end of input
    }
    ^
   drivers/media/pci/tw686x/tw686x-video.c:1151:19: warning: unused variable 'pb' [-Wunused-variable]
     unsigned int ch, pb;
                      ^~
   At top level:
   drivers/media/pci/tw686x/tw686x-video.c:1174:5: warning: 'tw686x_video_init' defined but not used [-Wunused-function]
    int tw686x_video_init(struct tw686x_dev *dev)
        ^~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +1285 drivers/media/pci/tw686x/tw686x-video.c

  1146	
  1147	
  1148	
  1149	void tw686x_video_device_release(struct tw686x_video_channel *vc) {
  1150		struct tw686x_dev *dev = vc->dev;
  1151		unsigned int ch, pb;
  1152	
  1153		for (ch = 0; ch < max_channels(dev); ch++) {
> 1154			struct tw686x_video_channel *vc = &dev->video_channels[ch];
  1155	
  1156		dev->dma_ops->free;
  1157		
  1158		video_device_release((struct video_device*)dev);
  1159	
  1160	}
  1161	
> 1162	void tw686x_video_free(struct tw686x_dev *dev)
  1163	{
> 1164		unsigned int ch, pb;
  1165	
  1166		for (ch = 0; ch < max_channels(dev); ch++) {
  1167			struct tw686x_video_channel *vc = &dev->video_channels[ch];
  1168	
  1169			video_unregister_device(vc->device);
  1170	
  1171		}
  1172	}
  1173	
  1174	int tw686x_video_init(struct tw686x_dev *dev)
  1175	{
  1176		unsigned int ch, val;
  1177		int err;
  1178	
  1179		if (dev->dma_mode == TW686X_DMA_MODE_MEMCPY)
  1180			dev->dma_ops = &memcpy_dma_ops;
  1181		else if (dev->dma_mode == TW686X_DMA_MODE_CONTIG)
  1182			dev->dma_ops = &contig_dma_ops;
  1183		else if (dev->dma_mode == TW686X_DMA_MODE_SG)
  1184			dev->dma_ops = &sg_dma_ops;
  1185		else
  1186			return -EINVAL;
  1187	
  1188		err = v4l2_device_register(&dev->pci_dev->dev, &dev->v4l2_dev);
  1189		if (err)
  1190			return err;
  1191	
  1192		if (dev->dma_ops->setup) {
  1193			err = dev->dma_ops->setup(dev);
  1194			if (err)
  1195				return err;
  1196		}
  1197	
  1198		/* Initialize vc->dev and vc->ch for the error path */
  1199		for (ch = 0; ch < max_channels(dev); ch++) {
  1200			struct tw686x_video_channel *vc = &dev->video_channels[ch];
  1201	
  1202			vc->dev = dev;
  1203			vc->ch = ch;
  1204		}
  1205	
  1206		for (ch = 0; ch < max_channels(dev); ch++) {
  1207			struct tw686x_video_channel *vc = &dev->video_channels[ch];
  1208			struct video_device *vdev;
  1209	
  1210			mutex_init(&vc->vb_mutex);
  1211			spin_lock_init(&vc->qlock);
  1212			INIT_LIST_HEAD(&vc->vidq_queued);
  1213	
  1214			/* default settings */
  1215			err = tw686x_set_standard(vc, V4L2_STD_NTSC);
  1216			if (err)
  1217				goto error;
  1218	
  1219			err = tw686x_set_format(vc, formats[0].fourcc,
  1220					TW686X_VIDEO_WIDTH,
  1221					TW686X_VIDEO_HEIGHT(vc->video_standard),
  1222					true);
  1223			if (err)
  1224				goto error;
  1225	
  1226			tw686x_set_input(vc, 0);
  1227			tw686x_set_framerate(vc, 30);
  1228			reg_write(dev, VDELAY_LO[ch], 0x14);
  1229			reg_write(dev, HACTIVE_LO[ch], 0xd0);
  1230			reg_write(dev, VIDEO_SIZE[ch], 0);
  1231	
  1232			vc->vidq.io_modes = VB2_READ | VB2_MMAP | VB2_DMABUF;
  1233			vc->vidq.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
  1234			vc->vidq.drv_priv = vc;
  1235			vc->vidq.buf_struct_size = sizeof(struct tw686x_v4l2_buf);
  1236			vc->vidq.ops = &tw686x_video_qops;
  1237			vc->vidq.mem_ops = dev->dma_ops->mem_ops;
  1238			vc->vidq.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
  1239			vc->vidq.min_buffers_needed = 2;
  1240			vc->vidq.lock = &vc->vb_mutex;
  1241			vc->vidq.gfp_flags = dev->dma_mode != TW686X_DMA_MODE_MEMCPY ?
  1242					     GFP_DMA32 : 0;
  1243			vc->vidq.dev = &dev->pci_dev->dev;
  1244	
  1245			err = vb2_queue_init(&vc->vidq);
  1246			if (err) {
  1247				v4l2_err(&dev->v4l2_dev,
  1248					 "dma%d: cannot init vb2 queue\n", ch);
  1249				goto error;
  1250			}
  1251	
  1252			err = v4l2_ctrl_handler_init(&vc->ctrl_handler, 4);
  1253			if (err) {
  1254				v4l2_err(&dev->v4l2_dev,
  1255					 "dma%d: cannot init ctrl handler\n", ch);
  1256				goto error;
  1257			}
  1258			v4l2_ctrl_new_std(&vc->ctrl_handler, &ctrl_ops,
  1259					  V4L2_CID_BRIGHTNESS, -128, 127, 1, 0);
  1260			v4l2_ctrl_new_std(&vc->ctrl_handler, &ctrl_ops,
  1261					  V4L2_CID_CONTRAST, 0, 255, 1, 100);
  1262			v4l2_ctrl_new_std(&vc->ctrl_handler, &ctrl_ops,
  1263					  V4L2_CID_SATURATION, 0, 255, 1, 128);
  1264			v4l2_ctrl_new_std(&vc->ctrl_handler, &ctrl_ops,
  1265					  V4L2_CID_HUE, -128, 127, 1, 0);
  1266			err = vc->ctrl_handler.error;
  1267			if (err)
  1268				goto error;
  1269	
  1270			err = v4l2_ctrl_handler_setup(&vc->ctrl_handler);
  1271			if (err)
  1272				goto error;
  1273	
  1274			vdev = video_device_alloc();
  1275			if (!vdev) {
  1276				v4l2_err(&dev->v4l2_dev,
  1277					 "dma%d: unable to allocate device\n", ch);
  1278				err = -ENOMEM;
  1279				goto error;
  1280			}
  1281	
  1282			snprintf(vdev->name, sizeof(vdev->name), "%s video", dev->name);
  1283			vdev->fops = &tw686x_video_fops;
  1284			vdev->ioctl_ops = &tw686x_video_ioctl_ops;
> 1285			vdev->release = tw686x_video_device_release;
  1286			vdev->v4l2_dev = &dev->v4l2_dev;
  1287			vdev->queue = &vc->vidq;
  1288			vdev->tvnorms = V4L2_STD_525_60 | V4L2_STD_625_50;
  1289			vdev->minor = -1;
  1290			vdev->lock = &vc->vb_mutex;
  1291			vdev->ctrl_handler = &vc->ctrl_handler;
  1292			vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE |
  1293					    V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
  1294			vc->device = vdev;
  1295			video_set_drvdata(vdev, vc);
  1296	
  1297			err = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
  1298			if (err < 0)
  1299				goto error;
  1300			vc->num = vdev->num;
  1301		}
  1302	
  1303		val = TW686X_DEF_PHASE_REF;
  1304		for (ch = 0; ch < max_channels(dev); ch++)
  1305			val |= dev->dma_ops->hw_dma_mode << (16 + ch * 2);
  1306		reg_write(dev, PHASE_REF, val);
  1307	
  1308		reg_write(dev, MISC2[0], 0xe7);
  1309		reg_write(dev, VCTRL1[0], 0xcc);
  1310		reg_write(dev, LOOP[0], 0xa5);
  1311		if (max_channels(dev) > 4) {
  1312			reg_write(dev, VCTRL1[1], 0xcc);
  1313			reg_write(dev, LOOP[1], 0xa5);
  1314			reg_write(dev, MISC2[1], 0xe7);
  1315		}
  1316		return 0;
  1317	
  1318	error:
  1319		tw686x_video_free(dev);
  1320		return err;
> 1321	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61455 bytes --]