media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver
Subsystems (2)
PATCH 1/1 media input infrastructure:tw686x: Added Added custom function to set vdev->release in tw686x driver
---
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(-)
drivers/media/pci/tw686x/tw686x-video.c +16 -4
@@ -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 ;
Signature · 1 line
2.17.1
Ezequiel Garcia
maintainerHi Mark,
Quoted 1 line · Mark Balantzyan
This commit needs to be thoroughly explained in order to make sense.
Quoted 16 lines
Signed-off-by Mark Balantzyan <[email protected]>---
This patch adds a custom function to release video device in assignment tovdev->releasemember in tw686x driver.drivers/media/pci/tw686x/tw686x-video.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)drivers/media/pci/tw686x/tw686x-video.c +1
-0@@ -1151,6 +1151,21 @@ void tw686x_video_irq(struct tw686x_dev *dev, unsigned long requests, } } +
drivers/media/pci/tw686x/tw686x-video.c void tw686x_video_irq(struct tw686x_dev *dev, unsigned long requests, · line 1164
Quoted 10 lines
drivers/media/pci/tw686x/tw686x-video.c +10
-0+ +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
Quoted 29 lines
drivers/media/pci/tw686x/tw686x-video.c +5
-5+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
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 <[email protected]>
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(-)
drivers/media/pci/tw686x/tw686x-video.c +0 -0
@@ -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;
Signature · 1 line
2.17.1
kbuild test robot
build botThank you for the patch! Yet something to improve:
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
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