[PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Guennadi Liakhovetski]

Video (sub)devices, connecting to SoCs over generic i2c busses cannot 
provide a pointer to struct v4l2_device in i2c-adapter driver_data, and 
provide their own i2c_board_info data, including a platform_data field. 
Add a v4l2_i2c_new_dev_subdev() API function that does exactly the same as 
v4l2_i2c_new_subdev() but uses different parameters, and make 
v4l2_i2c_new_subdev() a wrapper around it.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
index 1da8cb8..c55fc99 100644
--- a/drivers/media/video/v4l2-common.c
+++ b/drivers/media/video/v4l2-common.c
@@ -783,8 +783,6 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
 }
 EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
 
-
-
 /* Load an i2c sub-device. It assumes that i2c_get_adapdata(adapter)
    returns the v4l2_device and that i2c_get_clientdata(client)
    returns the v4l2_subdev. */
@@ -792,23 +790,34 @@ struct v4l2_subdev *v4l2_i2c_new_subdev(struct i2c_adapter *adapter,
 		const char *module_name, const char *client_type, u8 addr)
 {
 	struct v4l2_device *dev = i2c_get_adapdata(adapter);
-	struct v4l2_subdev *sd = NULL;
-	struct i2c_client *client;
 	struct i2c_board_info info;
 
-	BUG_ON(!dev);
-
-	if (module_name)
-		request_module(module_name);
-
 	/* Setup the i2c board info with the device type and
 	   the device address. */
 	memset(&info, 0, sizeof(info));
 	strlcpy(info.type, client_type, sizeof(info.type));
 	info.addr = addr;
 
+	return v4l2_i2c_new_dev_subdev(adapter, module_name, &info, dev);
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
+
+/* Load an i2c sub-device. It assumes that i2c_get_clientdata(client)
+   returns the v4l2_subdev. */
+struct v4l2_subdev *v4l2_i2c_new_dev_subdev(struct i2c_adapter *adapter,
+		const char *module_name, const struct i2c_board_info *info,
+		struct v4l2_device *dev)
+{
+	struct v4l2_subdev *sd = NULL;
+	struct i2c_client *client;
+
+	BUG_ON(!dev);
+
+	if (module_name)
+		request_module(module_name);
+
 	/* Create the i2c client */
-	client = i2c_new_device(adapter, &info);
+	client = i2c_new_device(adapter, info);
 	/* Note: it is possible in the future that
 	   c->driver is NULL if the driver is still being loaded.
 	   We need better support from the kernel so that we
@@ -835,7 +844,7 @@ error:
 		i2c_unregister_device(client);
 	return sd;
 }
-EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
+EXPORT_SYMBOL_GPL(v4l2_i2c_new_dev_subdev);
 
 /* Probe and load an i2c sub-device. It assumes that i2c_get_adapdata(adapter)
    returns the v4l2_device and that i2c_get_clientdata(client)
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 3a69056..0722b00 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -131,6 +131,7 @@ struct i2c_driver;
 struct i2c_adapter;
 struct i2c_client;
 struct i2c_device_id;
+struct i2c_board_info;
 struct v4l2_device;
 struct v4l2_subdev;
 struct v4l2_subdev_ops;
@@ -144,6 +145,10 @@ int v4l2_i2c_attach(struct i2c_adapter *adapter, int address, struct i2c_driver
    The client_type argument is the name of the chip that's on the adapter. */
 struct v4l2_subdev *v4l2_i2c_new_subdev(struct i2c_adapter *adapter,
 		const char *module_name, const char *client_type, u8 addr);
+/* Same as above but uses user-provided v4l2_device and i2c_board_info */
+struct v4l2_subdev *v4l2_i2c_new_dev_subdev(struct i2c_adapter *adapter,
+		const char *module_name, const struct i2c_board_info *info,
+		struct v4l2_device *dev);
 /* Probe and load an i2c module and return an initialized v4l2_subdev struct.
    Only call request_module if module_name != NULL.
    The client_type argument is the name of the chip that's on the adapter. */

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Hans Verkuil]

> Video (sub)devices, connecting to SoCs over generic i2c busses cannot
> provide a pointer to struct v4l2_device in i2c-adapter driver_data, and
> provide their own i2c_board_info data, including a platform_data field.
> Add a v4l2_i2c_new_dev_subdev() API function that does exactly the same as
> v4l2_i2c_new_subdev() but uses different parameters, and make
> v4l2_i2c_new_subdev() a wrapper around it.

Huh? Against what repository are you compiling? The v4l2_device pointer
has already been added!

Regards,

         Hans

>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> diff --git a/drivers/media/video/v4l2-common.c
> b/drivers/media/video/v4l2-common.c
> index 1da8cb8..c55fc99 100644
> --- a/drivers/media/video/v4l2-common.c
> +++ b/drivers/media/video/v4l2-common.c
> @@ -783,8 +783,6 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd,
> struct i2c_client *client,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
>
> -
> -
>  /* Load an i2c sub-device. It assumes that i2c_get_adapdata(adapter)
>     returns the v4l2_device and that i2c_get_clientdata(client)
>     returns the v4l2_subdev. */
> @@ -792,23 +790,34 @@ struct v4l2_subdev *v4l2_i2c_new_subdev(struct
> i2c_adapter *adapter,
>  		const char *module_name, const char *client_type, u8 addr)
>  {
>  	struct v4l2_device *dev = i2c_get_adapdata(adapter);
> -	struct v4l2_subdev *sd = NULL;
> -	struct i2c_client *client;
>  	struct i2c_board_info info;
>
> -	BUG_ON(!dev);
> -
> -	if (module_name)
> -		request_module(module_name);
> -
>  	/* Setup the i2c board info with the device type and
>  	   the device address. */
>  	memset(&info, 0, sizeof(info));
>  	strlcpy(info.type, client_type, sizeof(info.type));
>  	info.addr = addr;
>
> +	return v4l2_i2c_new_dev_subdev(adapter, module_name, &info, dev);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
> +
> +/* Load an i2c sub-device. It assumes that i2c_get_clientdata(client)
> +   returns the v4l2_subdev. */
> +struct v4l2_subdev *v4l2_i2c_new_dev_subdev(struct i2c_adapter *adapter,
> +		const char *module_name, const struct i2c_board_info *info,
> +		struct v4l2_device *dev)
> +{
> +	struct v4l2_subdev *sd = NULL;
> +	struct i2c_client *client;
> +
> +	BUG_ON(!dev);
> +
> +	if (module_name)
> +		request_module(module_name);
> +
>  	/* Create the i2c client */
> -	client = i2c_new_device(adapter, &info);
> +	client = i2c_new_device(adapter, info);
>  	/* Note: it is possible in the future that
>  	   c->driver is NULL if the driver is still being loaded.
>  	   We need better support from the kernel so that we
> @@ -835,7 +844,7 @@ error:
>  		i2c_unregister_device(client);
>  	return sd;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
> +EXPORT_SYMBOL_GPL(v4l2_i2c_new_dev_subdev);
>
>  /* Probe and load an i2c sub-device. It assumes that
> i2c_get_adapdata(adapter)
>     returns the v4l2_device and that i2c_get_clientdata(client)
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 3a69056..0722b00 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -131,6 +131,7 @@ struct i2c_driver;
>  struct i2c_adapter;
>  struct i2c_client;
>  struct i2c_device_id;
> +struct i2c_board_info;
>  struct v4l2_device;
>  struct v4l2_subdev;
>  struct v4l2_subdev_ops;
> @@ -144,6 +145,10 @@ int v4l2_i2c_attach(struct i2c_adapter *adapter, int
> address, struct i2c_driver
>     The client_type argument is the name of the chip that's on the
> adapter. */
>  struct v4l2_subdev *v4l2_i2c_new_subdev(struct i2c_adapter *adapter,
>  		const char *module_name, const char *client_type, u8 addr);
> +/* Same as above but uses user-provided v4l2_device and i2c_board_info */
> +struct v4l2_subdev *v4l2_i2c_new_dev_subdev(struct i2c_adapter *adapter,
> +		const char *module_name, const struct i2c_board_info *info,
> +		struct v4l2_device *dev);
>  /* Probe and load an i2c module and return an initialized v4l2_subdev
> struct.
>     Only call request_module if module_name != NULL.
>     The client_type argument is the name of the chip that's on the
> adapter. */
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Guennadi Liakhovetski]

On Tue, 21 Apr 2009, Hans Verkuil wrote:

> 
> > Video (sub)devices, connecting to SoCs over generic i2c busses cannot
> > provide a pointer to struct v4l2_device in i2c-adapter driver_data, and
> > provide their own i2c_board_info data, including a platform_data field.
> > Add a v4l2_i2c_new_dev_subdev() API function that does exactly the same as
> > v4l2_i2c_new_subdev() but uses different parameters, and make
> > v4l2_i2c_new_subdev() a wrapper around it.
> 
> Huh? Against what repository are you compiling? The v4l2_device pointer
> has already been added!

Ok, have to rebase then. I guess, it still would be better to do the way I 
propose in this patch - to add a new function, with i2c_board_info as a 
parameter and convert v4l2_i2c_new_subdev() to a wrapper around it, than 
to convert all existing users, agree? Do you also agree with the name?

Thanks
Guennadi

> 
> Regards,
> 
>          Hans
> 
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > diff --git a/drivers/media/video/v4l2-common.c
> > b/drivers/media/video/v4l2-common.c
> > index 1da8cb8..c55fc99 100644
> > --- a/drivers/media/video/v4l2-common.c
> > +++ b/drivers/media/video/v4l2-common.c
> > @@ -783,8 +783,6 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd,
> > struct i2c_client *client,
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
> >
> > -
> > -
> >  /* Load an i2c sub-device. It assumes that i2c_get_adapdata(adapter)
> >     returns the v4l2_device and that i2c_get_clientdata(client)
> >     returns the v4l2_subdev. */
> > @@ -792,23 +790,34 @@ struct v4l2_subdev *v4l2_i2c_new_subdev(struct
> > i2c_adapter *adapter,
> >  		const char *module_name, const char *client_type, u8 addr)
> >  {
> >  	struct v4l2_device *dev = i2c_get_adapdata(adapter);
> > -	struct v4l2_subdev *sd = NULL;
> > -	struct i2c_client *client;
> >  	struct i2c_board_info info;
> >
> > -	BUG_ON(!dev);
> > -
> > -	if (module_name)
> > -		request_module(module_name);
> > -
> >  	/* Setup the i2c board info with the device type and
> >  	   the device address. */
> >  	memset(&info, 0, sizeof(info));
> >  	strlcpy(info.type, client_type, sizeof(info.type));
> >  	info.addr = addr;
> >
> > +	return v4l2_i2c_new_dev_subdev(adapter, module_name, &info, dev);
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
> > +
> > +/* Load an i2c sub-device. It assumes that i2c_get_clientdata(client)
> > +   returns the v4l2_subdev. */
> > +struct v4l2_subdev *v4l2_i2c_new_dev_subdev(struct i2c_adapter *adapter,
> > +		const char *module_name, const struct i2c_board_info *info,
> > +		struct v4l2_device *dev)
> > +{
> > +	struct v4l2_subdev *sd = NULL;
> > +	struct i2c_client *client;
> > +
> > +	BUG_ON(!dev);
> > +
> > +	if (module_name)
> > +		request_module(module_name);
> > +
> >  	/* Create the i2c client */
> > -	client = i2c_new_device(adapter, &info);
> > +	client = i2c_new_device(adapter, info);
> >  	/* Note: it is possible in the future that
> >  	   c->driver is NULL if the driver is still being loaded.
> >  	   We need better support from the kernel so that we
> > @@ -835,7 +844,7 @@ error:
> >  		i2c_unregister_device(client);
> >  	return sd;
> >  }
> > -EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
> > +EXPORT_SYMBOL_GPL(v4l2_i2c_new_dev_subdev);
> >
> >  /* Probe and load an i2c sub-device. It assumes that
> > i2c_get_adapdata(adapter)
> >     returns the v4l2_device and that i2c_get_clientdata(client)
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index 3a69056..0722b00 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -131,6 +131,7 @@ struct i2c_driver;
> >  struct i2c_adapter;
> >  struct i2c_client;
> >  struct i2c_device_id;
> > +struct i2c_board_info;
> >  struct v4l2_device;
> >  struct v4l2_subdev;
> >  struct v4l2_subdev_ops;
> > @@ -144,6 +145,10 @@ int v4l2_i2c_attach(struct i2c_adapter *adapter, int
> > address, struct i2c_driver
> >     The client_type argument is the name of the chip that's on the
> > adapter. */
> >  struct v4l2_subdev *v4l2_i2c_new_subdev(struct i2c_adapter *adapter,
> >  		const char *module_name, const char *client_type, u8 addr);
> > +/* Same as above but uses user-provided v4l2_device and i2c_board_info */
> > +struct v4l2_subdev *v4l2_i2c_new_dev_subdev(struct i2c_adapter *adapter,
> > +		const char *module_name, const struct i2c_board_info *info,
> > +		struct v4l2_device *dev);
> >  /* Probe and load an i2c module and return an initialized v4l2_subdev
> > struct.
> >     Only call request_module if module_name != NULL.
> >     The client_type argument is the name of the chip that's on the
> > adapter. */
> >
> 
> 
> -- 
> Hans Verkuil - video4linux developer - sponsored by TANDBERG
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Hans Verkuil]

> On Tue, 21 Apr 2009, Hans Verkuil wrote:
>
>>
>> > Video (sub)devices, connecting to SoCs over generic i2c busses cannot
>> > provide a pointer to struct v4l2_device in i2c-adapter driver_data,
>> and
>> > provide their own i2c_board_info data, including a platform_data
>> field.
>> > Add a v4l2_i2c_new_dev_subdev() API function that does exactly the
>> same as
>> > v4l2_i2c_new_subdev() but uses different parameters, and make
>> > v4l2_i2c_new_subdev() a wrapper around it.
>>
>> Huh? Against what repository are you compiling? The v4l2_device pointer
>> has already been added!
>
> Ok, have to rebase then. I guess, it still would be better to do the way I
> propose in this patch - to add a new function, with i2c_board_info as a
> parameter and convert v4l2_i2c_new_subdev() to a wrapper around it, than
> to convert all existing users, agree? Do you also agree with the name?

I like the idea of passing in a board_info struct instead of an
addr/client_type. Just make sure when preparing a patch for the v4l-dvb
repo that this new function is for kernels >= 2.6.26 only.

I prefer a name like v4l2_i2c_subdev_board(). I will probably at some
stage remove that '_new' part of the existing functions anyway as that
doesn't add anything to the name.

Regards,

       Hans

>
> Thanks
> Guennadi
>
>>
>> Regards,
>>
>>          Hans
>>
>> >
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > ---
>> > diff --git a/drivers/media/video/v4l2-common.c
>> > b/drivers/media/video/v4l2-common.c
>> > index 1da8cb8..c55fc99 100644
>> > --- a/drivers/media/video/v4l2-common.c
>> > +++ b/drivers/media/video/v4l2-common.c
>> > @@ -783,8 +783,6 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd,
>> > struct i2c_client *client,
>> >  }
>> >  EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
>> >
>> > -
>> > -
>> >  /* Load an i2c sub-device. It assumes that i2c_get_adapdata(adapter)
>> >     returns the v4l2_device and that i2c_get_clientdata(client)
>> >     returns the v4l2_subdev. */
>> > @@ -792,23 +790,34 @@ struct v4l2_subdev *v4l2_i2c_new_subdev(struct
>> > i2c_adapter *adapter,
>> >  		const char *module_name, const char *client_type, u8 addr)
>> >  {
>> >  	struct v4l2_device *dev = i2c_get_adapdata(adapter);
>> > -	struct v4l2_subdev *sd = NULL;
>> > -	struct i2c_client *client;
>> >  	struct i2c_board_info info;
>> >
>> > -	BUG_ON(!dev);
>> > -
>> > -	if (module_name)
>> > -		request_module(module_name);
>> > -
>> >  	/* Setup the i2c board info with the device type and
>> >  	   the device address. */
>> >  	memset(&info, 0, sizeof(info));
>> >  	strlcpy(info.type, client_type, sizeof(info.type));
>> >  	info.addr = addr;
>> >
>> > +	return v4l2_i2c_new_dev_subdev(adapter, module_name, &info, dev);
>> > +}
>> > +EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
>> > +
>> > +/* Load an i2c sub-device. It assumes that i2c_get_clientdata(client)
>> > +   returns the v4l2_subdev. */
>> > +struct v4l2_subdev *v4l2_i2c_new_dev_subdev(struct i2c_adapter
>> *adapter,
>> > +		const char *module_name, const struct i2c_board_info *info,
>> > +		struct v4l2_device *dev)
>> > +{
>> > +	struct v4l2_subdev *sd = NULL;
>> > +	struct i2c_client *client;
>> > +
>> > +	BUG_ON(!dev);
>> > +
>> > +	if (module_name)
>> > +		request_module(module_name);
>> > +
>> >  	/* Create the i2c client */
>> > -	client = i2c_new_device(adapter, &info);
>> > +	client = i2c_new_device(adapter, info);
>> >  	/* Note: it is possible in the future that
>> >  	   c->driver is NULL if the driver is still being loaded.
>> >  	   We need better support from the kernel so that we
>> > @@ -835,7 +844,7 @@ error:
>> >  		i2c_unregister_device(client);
>> >  	return sd;
>> >  }
>> > -EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
>> > +EXPORT_SYMBOL_GPL(v4l2_i2c_new_dev_subdev);
>> >
>> >  /* Probe and load an i2c sub-device. It assumes that
>> > i2c_get_adapdata(adapter)
>> >     returns the v4l2_device and that i2c_get_clientdata(client)
>> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
>> > index 3a69056..0722b00 100644
>> > --- a/include/media/v4l2-common.h
>> > +++ b/include/media/v4l2-common.h
>> > @@ -131,6 +131,7 @@ struct i2c_driver;
>> >  struct i2c_adapter;
>> >  struct i2c_client;
>> >  struct i2c_device_id;
>> > +struct i2c_board_info;
>> >  struct v4l2_device;
>> >  struct v4l2_subdev;
>> >  struct v4l2_subdev_ops;
>> > @@ -144,6 +145,10 @@ int v4l2_i2c_attach(struct i2c_adapter *adapter,
>> int
>> > address, struct i2c_driver
>> >     The client_type argument is the name of the chip that's on the
>> > adapter. */
>> >  struct v4l2_subdev *v4l2_i2c_new_subdev(struct i2c_adapter *adapter,
>> >  		const char *module_name, const char *client_type, u8 addr);
>> > +/* Same as above but uses user-provided v4l2_device and
>> i2c_board_info */
>> > +struct v4l2_subdev *v4l2_i2c_new_dev_subdev(struct i2c_adapter
>> *adapter,
>> > +		const char *module_name, const struct i2c_board_info *info,
>> > +		struct v4l2_device *dev);
>> >  /* Probe and load an i2c module and return an initialized v4l2_subdev
>> > struct.
>> >     Only call request_module if module_name != NULL.
>> >     The client_type argument is the name of the chip that's on the
>> > adapter. */
>> >
>>
>>
>> --
>> Hans Verkuil - video4linux developer - sponsored by TANDBERG
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Agustin]

Hi,

--- On 21/4/09, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Video (sub)devices, connecting to SoCs over generic i2c busses cannot 
> provide a pointer to struct v4l2_device in i2c-adapter driver_data, and 
> provide their own i2c_board_info data, including a platform_data field. 
> Add a v4l2_i2c_new_dev_subdev() API function that does exactly the same
> as v4l2_i2c_new_subdev() but uses different parameters, and make 
> v4l2_i2c_new_subdev() a wrapper around it.

[snip]

I am wondering about this ongoing effort and its pursued goal: is it to hierarchize the v4l architecture, adding new abstraction levels? If so, what for?

To me, as an eventual driver developer, this makes it harder to integrate my own drivers, as I use I2C and V4L in my system but I don't want them to be tightly coupled.

Of course I can ignore this "subdev" stuff and just link against soc-camera which is what I need, and manage I2C without V4L knowing about it. Which is what I do.

So, which is the point I am missing?

Regards,
--Agustín.

--
Agustin Ferrin Pozuelo
Embedded Systems Consultant
http://embedded.ferrin.org
Tel. +34 610502587

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Guennadi Liakhovetski]

On Tue, 21 Apr 2009, Hans Verkuil wrote:

> 
> > On Tue, 21 Apr 2009, Hans Verkuil wrote:
> >
> >>
> >> > Video (sub)devices, connecting to SoCs over generic i2c busses cannot
> >> > provide a pointer to struct v4l2_device in i2c-adapter driver_data,
> >> and
> >> > provide their own i2c_board_info data, including a platform_data
> >> field.
> >> > Add a v4l2_i2c_new_dev_subdev() API function that does exactly the
> >> same as
> >> > v4l2_i2c_new_subdev() but uses different parameters, and make
> >> > v4l2_i2c_new_subdev() a wrapper around it.
> >>
> >> Huh? Against what repository are you compiling? The v4l2_device pointer
> >> has already been added!
> >
> > Ok, have to rebase then. I guess, it still would be better to do the way I
> > propose in this patch - to add a new function, with i2c_board_info as a
> > parameter and convert v4l2_i2c_new_subdev() to a wrapper around it, than
> > to convert all existing users, agree? Do you also agree with the name?
> 
> I like the idea of passing in a board_info struct instead of an
> addr/client_type.

And converting v4l2_i2c_new_subdev() to a wrapper is ok too?

> Just make sure when preparing a patch for the v4l-dvb
> repo that this new function is for kernels >= 2.6.26 only.

Why and how? I am not adding any new structs or fields with this patch, am 
I? So it should build with all kernels, with which the current 
v4l2_i2c_new_subdev() builds.

> I prefer a name like v4l2_i2c_subdev_board(). I will probably at some
> stage remove that '_new' part of the existing functions anyway as that
> doesn't add anything to the name.

Ok, will do.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Guennadi Liakhovetski]

On Tue, 21 Apr 2009, Agustin wrote:

> 
> Hi,
> 
> --- On 21/4/09, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Video (sub)devices, connecting to SoCs over generic i2c busses cannot 
> > provide a pointer to struct v4l2_device in i2c-adapter driver_data, and 
> > provide their own i2c_board_info data, including a platform_data field. 
> > Add a v4l2_i2c_new_dev_subdev() API function that does exactly the same
> > as v4l2_i2c_new_subdev() but uses different parameters, and make 
> > v4l2_i2c_new_subdev() a wrapper around it.
> 
> [snip]
> 
> I am wondering about this ongoing effort and its pursued goal: is it to 
> hierarchize the v4l architecture, adding new abstraction levels? If so, 
> what for?

Driver-reuse. soc-camera framework will be able to use all available and 
new v4l2-subdev drivers, and vice versa.

> To me, as an eventual driver developer, this makes it harder to 
> integrate my own drivers, as I use I2C and V4L in my system but I don't 
> want them to be tightly coupled.

This conversion shouldn't make the coupling any tighter.

> Of course I can ignore this "subdev" stuff and just link against 
> soc-camera which is what I need, and manage I2C without V4L knowing 
> about it. Which is what I do.

You won't be able to. The only link to woc-camera will be the v4l2-subdev 
link. Already now with this patch many essential soc-camera API operations 
are gone.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Hans Verkuil]

> On Tue, 21 Apr 2009, Hans Verkuil wrote:
>
>>
>> > On Tue, 21 Apr 2009, Hans Verkuil wrote:
>> >
>> >>
>> >> > Video (sub)devices, connecting to SoCs over generic i2c busses
>> cannot
>> >> > provide a pointer to struct v4l2_device in i2c-adapter driver_data,
>> >> and
>> >> > provide their own i2c_board_info data, including a platform_data
>> >> field.
>> >> > Add a v4l2_i2c_new_dev_subdev() API function that does exactly the
>> >> same as
>> >> > v4l2_i2c_new_subdev() but uses different parameters, and make
>> >> > v4l2_i2c_new_subdev() a wrapper around it.
>> >>
>> >> Huh? Against what repository are you compiling? The v4l2_device
>> pointer
>> >> has already been added!
>> >
>> > Ok, have to rebase then. I guess, it still would be better to do the
>> way I
>> > propose in this patch - to add a new function, with i2c_board_info as
>> a
>> > parameter and convert v4l2_i2c_new_subdev() to a wrapper around it,
>> than
>> > to convert all existing users, agree? Do you also agree with the name?
>>
>> I like the idea of passing in a board_info struct instead of an
>> addr/client_type.
>
> And converting v4l2_i2c_new_subdev() to a wrapper is ok too?

Sure.

>> Just make sure when preparing a patch for the v4l-dvb
>> repo that this new function is for kernels >= 2.6.26 only.
>
> Why and how? I am not adding any new structs or fields with this patch, am
> I? So it should build with all kernels, with which the current
> v4l2_i2c_new_subdev() builds.

The board_info struct didn't appear until 2.6.22, so that's certainly a
cut-off point. Since the probe version of this call does not work on
kernels < 2.6.26 the autoprobing mechanism is still used for those older
kernels. I think it makes life much easier to require that everything that
uses board_info needs kernel 2.6.26 at the minimum. I don't think that is
an issue anyway for soc-camera. Unless there is a need to use soc-camera
from v4l-dvb with kernels <2.6.26?

Regards,

          Hans

>> I prefer a name like v4l2_i2c_subdev_board(). I will probably at some
>> stage remove that '_new' part of the existing functions anyway as that
>> doesn't add anything to the name.
>
> Ok, will do.
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Guennadi Liakhovetski]

On Tue, 21 Apr 2009, Hans Verkuil wrote:

> The board_info struct didn't appear until 2.6.22, so that's certainly a
> cut-off point. Since the probe version of this call does not work on
> kernels < 2.6.26 the autoprobing mechanism is still used for those older
> kernels. I think it makes life much easier to require that everything that
> uses board_info needs kernel 2.6.26 at the minimum. I don't think that is
> an issue anyway for soc-camera. Unless there is a need to use soc-camera
> from v4l-dvb with kernels <2.6.26?

No, most definitely ultimately undoubtedly NOT.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Agustin]

On 21/4/09, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Tue, 21 Apr 2009, Agustin wrote:
> > 
> > Hi,
> > 
> > On 21/4/09, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > > Video (sub)devices, connecting to SoCs over generic i2c busses cannot 
> > > provide a pointer to struct v4l2_device in i2c-adapter driver_data, and 
> > > provide their own i2c_board_info data, including a platform_data field. 
> > > Add a v4l2_i2c_new_dev_subdev() API function that does exactly the same
> > > as v4l2_i2c_new_subdev() but uses different parameters, and make 
> > > v4l2_i2c_new_subdev() a wrapper around it.
> > 
> > [snip]
> > 
> > I am wondering about this ongoing effort and its pursued goal: is it
> > to hierarchize the v4l architecture, adding new abstraction levels?
> > If so, what for?

> Driver-reuse. soc-camera framework will be able to use all available and 
> new v4l2-subdev drivers, and vice versa.

Well, "Driver reuse." sounds more as a mantra than a reason for me. Then I can't find any "available" v4l2-subdev driver in 2.6.29.

I assume this subdev stuff plays a mayor role in current V4L2 architecture refactorization. Then we probably should take this opportunity to relieve V4L APIs from all its explicit I2C mangling, because...

> > To me, as an eventual driver developer, this makes it harder to 
> > integrate my own drivers, as I use I2C and V4L in my system but I
> > don't want them to be tightly coupled.

> This conversion shouldn't make the coupling any tighter.

But still I think V4L system should not be aware of I2C at all.

To me, V4L is a kernel subsystem in charge of moving multimedia data from/to userspace/hardware, and the APIs should reflect just that.

If one V4L driver uses I2C for device control, what does V4L have to say about that, really? Or why V4L would never care about usb or SPI links?

I2C and V4L should stay cleanly and clearly apart.

> > Of course I can ignore this "subdev" stuff and just link against 
> > soc-camera which is what I need, and manage I2C without V4L knowing 
> > about it. Which is what I do.

> You won't be able to. The only link to woc-camera will be the
> v4l2-subdev link. Already now with this patch many essential
> soc-camera API operations are gone.

I guess you mean that I will have to use v4l2-subdev interface to connect to soc-camera, and not to surrender my I2C management to an I2C-extraneous subsystem. Is that right?

(Sorry for arriving this late to the discussion just to critizise your good efforts.)

Regards,
--Agustín.

Re: [PATCH] v4l2-subdev: add a v4l2_i2c_new_dev_subdev() function

[Hans Verkuil]

>
> On 21/4/09, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>> On Tue, 21 Apr 2009, Agustin wrote:
>> >
>> > Hi,
>> >
>> > On 21/4/09, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>> > > Video (sub)devices, connecting to SoCs over generic i2c busses
>> cannot
>> > > provide a pointer to struct v4l2_device in i2c-adapter driver_data,
>> and
>> > > provide their own i2c_board_info data, including a platform_data
>> field.
>> > > Add a v4l2_i2c_new_dev_subdev() API function that does exactly the
>> same
>> > > as v4l2_i2c_new_subdev() but uses different parameters, and make
>> > > v4l2_i2c_new_subdev() a wrapper around it.
>> >
>> > [snip]
>> >
>> > I am wondering about this ongoing effort and its pursued goal: is it
>> > to hierarchize the v4l architecture, adding new abstraction levels?
>> > If so, what for?
>
>> Driver-reuse. soc-camera framework will be able to use all available and
>> new v4l2-subdev drivers, and vice versa.
>
> Well, "Driver reuse." sounds more as a mantra than a reason for me. Then I
> can't find any "available" v4l2-subdev driver in 2.6.29.

Look in 2.6.30. Also look in Documentation/video4linux/v4l2-framework.txt
which documents the new framework.

> I assume this subdev stuff plays a mayor role in current V4L2 architecture
> refactorization. Then we probably should take this opportunity to relieve
> V4L APIs from all its explicit I2C mangling, because...

That is the case if you use v4l2_subdev. That's completely bus independent.

>> > To me, as an eventual driver developer, this makes it harder to
>> > integrate my own drivers, as I use I2C and V4L in my system but I
>> > don't want them to be tightly coupled.
>
>> This conversion shouldn't make the coupling any tighter.
>
> But still I think V4L system should not be aware of I2C at all.
>
> To me, V4L is a kernel subsystem in charge of moving multimedia data
> from/to userspace/hardware, and the APIs should reflect just that.
>
> If one V4L driver uses I2C for device control, what does V4L have to say
> about that, really? Or why V4L would never care about usb or SPI links?
>
> I2C and V4L should stay cleanly and clearly apart.

Again, that's what v4l2_subdev does. And in fact it is used like that
already in the ivtv and cx18 drivers.

>> > Of course I can ignore this "subdev" stuff and just link against
>> > soc-camera which is what I need, and manage I2C without V4L knowing
>> > about it. Which is what I do.
>
>> You won't be able to. The only link to woc-camera will be the
>> v4l2-subdev link. Already now with this patch many essential
>> soc-camera API operations are gone.
>
> I guess you mean that I will have to use v4l2-subdev interface to connect
> to soc-camera, and not to surrender my I2C management to an I2C-extraneous
> subsystem. Is that right?
>
> (Sorry for arriving this late to the discussion just to critizise your
> good efforts.)

All i2c v4l drivers should use v4l2_subdev so that they can be reused in
other PCI/USB/platform/whatever drivers. Currently sensor drivers such as
mt9m111 are tied to soc-camera and are impossible to reuse in e.g. a USB
webcam driver. In 2.6.30 all i2c v4l drivers used by PCI and USB drivers
are converted to v4l2_subdev. By 2.6.31 I hope that the soc-camera i2c
drivers and the i2c drivers based on v4l2-int-device.h are also converted,
thus making the i2c v4l drivers completely reusable.

In addition, the flexibility of v4l2_subdev allows it to be used for other
non-i2c devices as well.

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG