Re: Cleanup proposal for media/gspca

[Jean-Francois Moine <moinejf@free.fr>]

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/