Cleanup proposal for media/gspca

PATCH Ezequiel García 2 participants 6 messages Nov 16, 2011 – Nov 21, 2011 1 message not in the archive Media (V4L/DVB)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20111116013445.GA5273@localhost

Ezequiel García

author

2011-11-16 18:19 UTC #1 lore raw

Hi folks,

In 'media/video/gspca/gspca.c' I really hated this cast (maybe because I am too dumb to understand it):

  gspca_dev = (struct gspca_dev *) video_devdata(file);

wich is only legal because a struct video_device is the first member of gspca_dev. IMHO, this is 'unnecesary obfuscation'.
The thing is the driver is surely working fine and there is no good reasong for the change.

Is it ok to submit a patchset to change this? Something like this:

(quoted diff) +0 -0

b/drivers/media/video/gspca/gspca.c
index 881e04c..5d962ce 100644

drivers/media/video/gspca/gspca.c +3 -1

@@ -1304,9 +1306,11 @@ static void gspca_release(struct video_device *vfd)
 static int dev_open(struct file *file)
 {
 	struct gspca_dev *gspca_dev;
+	struct video_device *vdev;
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	vdev = video_devdata(file);
+	gspca_dev = video_get_drvdata(vdev);
 	if (!gspca_dev->present)

Thanks,
Ezequiel.

Jean-Francois Moine

2011-11-17 10:06 UTC #2 lore raw

Quoted 18 lines · On Wed, 16 Nov 2011 15:19:04 -0300 Ezequiel García <[email protected]>…

In 'media/video/gspca/gspca.c' I really hated this cast (maybe because I am too dumb to understand it):
  gspca_dev = (struct gspca_dev *) video_devdata(file);
wich is only legal because a struct video_device is the first member of gspca_dev. IMHO, this is 'unnecesary obfuscation'.
The thing is the driver is surely working fine and there is no good reasong for the change.
Is it ok to submit a patchset to change this? Something like this:
drivers/media/video/gspca/gspca.c +0 -0
index 881e04c..5d962ce 100644

drivers/media/video/gspca/gspca.c static void gspca_release(struct video_device *vfd) · line 1314

Quoted 10 lines

drivers/media/video/gspca/gspca.c +3 -1

@@ -1304,9 +1306,11 @@ static void gspca_release(struct video_device *vfd)
 static int dev_open(struct file *file)
 {
 	struct gspca_dev *gspca_dev;
+	struct video_device *vdev;
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	vdev = video_devdata(file);
+	gspca_dev = video_get_drvdata(vdev);
 	if (!gspca_dev->present)

Hi Ezequiel,

You are right, the cast is not a good way (and there are a lot of them in the gspca subdrivers), but your patch does not work because the 'private_data' of the device is not initialized (there is no call to video_set_drvdata).

So, a possible cleanup could be:

drivers/media/video/gspca/gspca.c static void gspca_release(struct video_device *vfd) · line 1315

Quoted 2 lines

drivers/media/video/gspca/gspca.c +1 -1

-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	gspca_dev = container_of(video_devdata(file), struct gspca_dev, vdev);

Is it OK for you?

Signature · 2 lines

Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Ezequiel

author

2011-11-17 17:57 UTC #3 lore raw

Quoted 39 lines · Jean-Francois Moine

Quoted 18 lines · On Wed, 16 Nov 2011 15:19:04 -0300 Ezequiel Garc??a <[email protected]>…
In 'media/video/gspca/gspca.c' I really hated this cast (maybe because I am too dumb to understand it):
  gspca_dev = (struct gspca_dev *) video_devdata(file);
wich is only legal because a struct video_device is the first member of gspca_dev. IMHO, this is 'unnecesary obfuscation'.
The thing is the driver is surely working fine and there is no good reasong for the change.
Is it ok to submit a patchset to change this? Something like this:
drivers/media/video/gspca/gspca.c +0 -0
index 881e04c..5d962ce 100644
drivers/media/video/gspca/gspca.c static void gspca_release(struct video_device *vfd) · line 1314
Quoted 10 lines
drivers/media/video/gspca/gspca.c +3 -1
@@ -1304,9 +1306,11 @@ static void gspca_release(struct video_device *vfd)
 static int dev_open(struct file *file)
 {
 	struct gspca_dev *gspca_dev;
+	struct video_device *vdev;
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	vdev = video_devdata(file);
+	gspca_dev = video_get_drvdata(vdev);
 	if (!gspca_dev->present)
Hi Ezequiel,
You are right, the cast is not a good way (and there are a lot of them in the gspca subdrivers), but your patch does not work because the 'private_data' of the device is not initialized (there is no call to video_set_drvdata).

Quoted 6 lines

So, a possible cleanup could be:
drivers/media/video/gspca/gspca.c static void gspca_release(struct video_device *vfd) · line 1315
Quoted 2 lines
drivers/media/video/gspca/gspca.c +1 -1
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	gspca_dev = container_of(video_devdata(file), struct gspca_dev, vdev);
Is it OK for you?

Hi, and thanks a lot for your comments. Actually the _sample_ patch I sent was just to exemplify the real patch I had in mind, and not wasn't meant to work.

Maybe later I can send the whole patch properly formatted. I know there are more of that in gspca, but right now I made changes just in gspca.c, tested with my pac7302 camera, so far so good: it is working.

Anyway, I am _very_ noob and just starting with this kernel programming thing so any comments of any kind are welcome.

Thanks,
Ezequiel.

Ezequiel

author

2011-11-19 19:00 UTC #4 lore raw

In reply to Jean-Francois Moine

Quoted 39 lines · Jean-Francois Moine

Quoted 18 lines · On Wed, 16 Nov 2011 15:19:04 -0300 Ezequiel Garc??a <[email protected]>…
In 'media/video/gspca/gspca.c' I really hated this cast (maybe because I am too dumb to understand it):
  gspca_dev = (struct gspca_dev *) video_devdata(file);
wich is only legal because a struct video_device is the first member of gspca_dev. IMHO, this is 'unnecesary obfuscation'.
The thing is the driver is surely working fine and there is no good reasong for the change.
Is it ok to submit a patchset to change this? Something like this:
drivers/media/video/gspca/gspca.c +0 -0
index 881e04c..5d962ce 100644
drivers/media/video/gspca/gspca.c static void gspca_release(struct video_device *vfd) · line 1314
Quoted 10 lines
drivers/media/video/gspca/gspca.c +3 -1
@@ -1304,9 +1306,11 @@ static void gspca_release(struct video_device *vfd)
 static int dev_open(struct file *file)
 {
 	struct gspca_dev *gspca_dev;
+	struct video_device *vdev;
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	vdev = video_devdata(file);
+	gspca_dev = video_get_drvdata(vdev);
 	if (!gspca_dev->present)
Hi Ezequiel,
You are right, the cast is not a good way (and there are a lot of them in the gspca subdrivers), but your patch does not work because the 'private_data' of the device is not initialized (there is no call to video_set_drvdata).

Quoted 7 lines

So, a possible cleanup could be:
drivers/media/video/gspca/gspca.c static void gspca_release(struct video_device *vfd) · line 1315
Quoted 2 lines
drivers/media/video/gspca/gspca.c +1 -1
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	gspca_dev = container_of(video_devdata(file), struct gspca_dev, vdev);
Is it OK for you?

Hi Jef,

I just sent a patch to linux-media for this little issue.

I realize it is only a very minor patch,
so I am not sure If I am helping or just annoying the developers ;)

Anyway, if you could check the patch I would appreciate it.

A few questions arose:

* I made the patch on this tree: git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
not sure if this is ok.

* Should I send gspca's patches to anyone else besides you and the list?

* I have this on my MAINTANIERS file: git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-2.6.git
But that repo seems no longer alive, maybe MAINTAINERS need some
updating.

Again, hope the patch helps,

Thanks,
Ezequiel.

Jean-Francois Moine

2011-11-20 07:24 UTC #5 lore raw

Quoted 8 lines · On Sat, 19 Nov 2011 15:59:50 -0300 Ezequiel <[email protected]> wrote:

Hi Jef,
I just sent a patch to linux-media for this little issue.
I realize it is only a very minor patch,
so I am not sure If I am helping or just annoying the developers ;)
Anyway, if you could check the patch I would appreciate it.

	[snip]

Quoted 1 line

Again, hope the patch helps,

Hi Ezequiel,

It is not a minor patch, but maybe you don't know about object programming.

As it is defined, a gspca device _is_ a video device, as a gspca subdriver is a gspca device, and as a video device is a device: each lower structure is contained in a higher one.

Your patch defines the gspca structure as a separate entity which is somewhat related to a video device by two reverse pointers. It complexifies the structure accesses, adds more code and hides the nature of a gspca device.

No, your patch does not help...

Signature · 2 lines

Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Ezequiel

author

2011-11-21 23:18 UTC #6 lore raw

Quoted 6 lines · Jean-Francois Moine

Hi Ezequiel,
It is not a minor patch, but maybe you don't know about object programming.

Quoted 9 lines

As it is defined, a gspca device _is_ a video device, as a gspca subdriver is a gspca device, and as a video device is a device: each lower structure is contained in a higher one.
Your patch defines the gspca structure as a separate entity which is somewhat related to a video device by two reverse pointers. It complexifies the structure accesses, adds more code and hides the nature of a gspca device.

Hi Jef,

Thanks for the explanation, I have things much clear now. I didn't realize linux coding style enforces so explicitly OOP.

I based my patch on tm6000 driver and your
previous mail about the -supposedly- ugly cast:

  gspca_dev = (struct gspca_dev *) video_devdata(file);

Now it doesn't seems so ugly, I guess I went too far. Still, maybe the 'container_of' trick could make thins easier to understand.

Thanks again for your patience,
Ezequiel.