Re: Cleanup proposal for media/gspca
On Sun, Nov 20, 2011 at 08:24:29AM +0100, Jean-Francois Moine wrote: 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. 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. -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Cleanup proposal for media/gspca
On Thu, Nov 17, 2011 at 11:07:16AM +0100, Jean-Francois Moine wrote:
On Wed, 16 Nov 2011 15:19:04 -0300
Ezequiel Garc??a elezegar...@gmail.com wrote:
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:
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index 881e04c..5d962ce 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -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:
- 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.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Cleanup proposal for media/gspca
On Wed, 16 Nov 2011 15:19:04 -0300
Ezequiel García elezegar...@gmail.com wrote:
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:
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index 881e04c..5d962ce 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -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:
- gspca_dev = (struct gspca_dev *) video_devdata(file);
+ gspca_dev = container_of(video_devdata(file), struct gspca_dev, vdev);
Is it OK for you?
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Cleanup proposal for media/gspca
On Thu, Nov 17, 2011 at 11:07:16AM +0100, Jean-Francois Moine wrote:
On Wed, 16 Nov 2011 15:19:04 -0300
Ezequiel Garc??a elezegar...@gmail.com wrote:
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:
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index 881e04c..5d962ce 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -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:
- 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.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Cleanup proposal for media/gspca
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:
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index 881e04c..5d962ce 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -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.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
