Cleanup proposal for media/gspca

Cleanup proposal for media/gspca

[Ezequiel García]

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.

Re: Cleanup proposal for media/gspca

[Jean-Francois Moine]

On Wed, 16 Nov 2011 15:19:04 -0300
Ezequiel García <elezegarcia@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/

Re: Cleanup proposal for media/gspca

[Ezequiel]

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 <elezegarcia@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.

Re: Cleanup proposal for media/gspca

[Ezequiel]

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 <elezegarcia@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.

Re: Cleanup proposal for media/gspca

[Jean-Francois Moine]

On Sat, 19 Nov 2011 15:59:50 -0300
Ezequiel <elezegarcia@gmail.com> 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]
> 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...

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

Re: Cleanup proposal for media/gspca

[Ezequiel]

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.