* [PATCH RFC 1/3] davinci: vpif: capture: add V4L2-async support
2013-01-09 13:41 [PATCH RFC 0/3] vpif capture support for async subdevice probing Lad, Prabhakar
@ 2013-01-09 13:41 ` Lad, Prabhakar
2013-01-09 15:42 ` Hans Verkuil
2013-01-09 13:41 ` [PATCH RFC 2/3] tvp514x: support asynchronous probing Lad, Prabhakar
2013-01-09 13:41 ` [PATCH RFC 3/3] ARM: da850/omap-l138: vpif capture convert to asynchronously register of subdev Lad, Prabhakar
2 siblings, 1 reply; 6+ messages in thread
From: Lad, Prabhakar @ 2013-01-09 13:41 UTC (permalink / raw)
To: LMML
Cc: LKML, Mauro Carvalho Chehab, Laurent Pinchart, Hans Verkuil,
Sekhar Nori, DLOS, Guennadi Liakhovetski, LAK, Lad, Prabhakar,
Sakari Ailus, Mauro Carvalho Chehab
Add support for asynchronous subdevice probing, using the v4l2-async API.
The legacy synchronous mode is still supported too, which allows to
gradually update drivers and platforms. The selected approach adds a
notifier for each struct soc_camera_device instance, i.e. for each video
device node, even when there are multiple such instances registered with a
single soc-camera host simultaneously.
Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/media/platform/davinci/vpif_capture.c | 171 ++++++++++++++++++-------
drivers/media/platform/davinci/vpif_capture.h | 2 +
include/media/davinci/vpif_types.h | 2 +
3 files changed, 128 insertions(+), 47 deletions(-)
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 5892d2b..a8b6588 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -34,6 +34,8 @@
#include <linux/platform_device.h>
#include <linux/io.h>
#include <linux/slab.h>
+
+#include <media/v4l2-async.h>
#include <media/v4l2-device.h>
#include <media/v4l2-ioctl.h>
#include <media/v4l2-chip-ident.h>
@@ -2054,6 +2056,96 @@ vpif_init_free_channel_objects:
return err;
}
+int vpif_async_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_async_subdev_list *asdl)
+{
+ int i = 0;
+
+ if (!asdl->subdev) {
+ v4l2_err(vpif_dev->driver,
+ "%s(): Subdevice driver hasn't set subdev pointer!\n",
+ __func__);
+ return -EINVAL;
+ }
+ v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
+ asdl->subdev->name);
+
+ for (i = 0; i < vpif_obj.config->subdev_count; i++)
+ if (!strcmp(vpif_obj.config->subdev_info[i].name,
+ asdl->subdev->name)) {
+ vpif_obj.sd[i] = asdl->subdev;
+ break;
+ }
+
+ if (i >= vpif_obj.config->subdev_count)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int vpif_probe_complete(void)
+{
+ struct common_obj *common;
+ struct channel_obj *ch;
+ int i, j, err, k;
+
+ for (j = 0; j < VPIF_CAPTURE_MAX_DEVICES; j++) {
+ ch = vpif_obj.dev[j];
+ ch->channel_id = j;
+ common = &(ch->common[VPIF_VIDEO_INDEX]);
+ spin_lock_init(&common->irqlock);
+ mutex_init(&common->lock);
+ ch->video_dev->lock = &common->lock;
+ /* Initialize prio member of channel object */
+ v4l2_prio_init(&ch->prio);
+ video_set_drvdata(ch->video_dev, ch);
+
+ /* select input 0 */
+ err = vpif_set_input(vpif_obj.config, ch, 0);
+ if (err)
+ goto probe_out;
+
+ err = video_register_device(ch->video_dev,
+ VFL_TYPE_GRABBER, (j ? 1 : 0));
+ if (err)
+ goto probe_out;
+ }
+
+ v4l2_info(&vpif_obj.v4l2_dev, "VPIF capture driver initialized\n");
+ return 0;
+
+probe_out:
+ for (k = 0; k < j; k++) {
+ /* Get the pointer to the channel object */
+ ch = vpif_obj.dev[k];
+ /* Unregister video device */
+ video_unregister_device(ch->video_dev);
+ }
+ kfree(vpif_obj.sd);
+ for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
+ ch = vpif_obj.dev[i];
+ /* Note: does nothing if ch->video_dev == NULL */
+ video_device_release(ch->video_dev);
+ }
+ v4l2_device_unregister(&vpif_obj.v4l2_dev);
+
+ return err;
+}
+
+int vpif_async_complete(struct v4l2_async_notifier *notifier)
+{
+ return vpif_probe_complete();
+}
+
+void vpif_async_unbind(struct v4l2_async_notifier *notifier,
+ struct v4l2_async_subdev_list *asdl)
+{
+ /*FIXME: Do we need this callback ? */
+ v4l2_info(&vpif_obj.v4l2_dev, "unregistered sub device %s\n",
+ asdl->subdev->name);
+ return;
+}
+
/**
* vpif_probe : This function probes the vpif capture driver
* @pdev: platform device pointer
@@ -2064,12 +2156,10 @@ vpif_init_free_channel_objects:
static __init int vpif_probe(struct platform_device *pdev)
{
struct vpif_subdev_info *subdevdata;
- struct vpif_capture_config *config;
- int i, j, k, err;
+ int i, j, err;
int res_idx = 0;
struct i2c_adapter *i2c_adap;
struct channel_obj *ch;
- struct common_obj *common;
struct video_device *vfd;
struct resource *res;
int subdev_count;
@@ -2146,10 +2236,9 @@ static __init int vpif_probe(struct platform_device *pdev)
}
}
- i2c_adap = i2c_get_adapter(1);
- config = pdev->dev.platform_data;
+ vpif_obj.config = pdev->dev.platform_data;
- subdev_count = config->subdev_count;
+ subdev_count = vpif_obj.config->subdev_count;
vpif_obj.sd = kzalloc(sizeof(struct v4l2_subdev *) * subdev_count,
GFP_KERNEL);
if (vpif_obj.sd == NULL) {
@@ -2158,53 +2247,41 @@ static __init int vpif_probe(struct platform_device *pdev)
goto vpif_sd_error;
}
- for (i = 0; i < subdev_count; i++) {
- subdevdata = &config->subdev_info[i];
- vpif_obj.sd[i] =
- v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
- i2c_adap,
- &subdevdata->board_info,
- NULL);
+ if (!vpif_obj.config->asd_sizes) {
+ i2c_adap = i2c_get_adapter(1);
+ for (i = 0; i < subdev_count; i++) {
+ subdevdata = &vpif_obj.config->subdev_info[i];
+ vpif_obj.sd[i] =
+ v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
+ i2c_adap,
+ &subdevdata->board_info,
+ NULL);
- if (!vpif_obj.sd[i]) {
- vpif_err("Error registering v4l2 subdevice\n");
- goto probe_subdev_out;
+ if (!vpif_obj.sd[i]) {
+ vpif_err("Error registering v4l2 subdevice\n");
+ goto probe_subdev_out;
+ }
+ v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
+ subdevdata->name);
+ }
+ vpif_probe_complete();
+ } else {
+ vpif_obj.notifier.subdev = vpif_obj.config->asd;
+ vpif_obj.notifier.subdev_num = vpif_obj.config->asd_sizes[0];
+ vpif_obj.notifier.bound = vpif_async_bound;
+ vpif_obj.notifier.complete = vpif_async_complete;
+ vpif_obj.notifier.unbind = vpif_async_unbind;
+ err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,
+ &vpif_obj.notifier);
+ if (err) {
+ vpif_err("Error registering async notifier\n");
+ err = -EINVAL;
+ goto vpif_sd_error;
}
- v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
- subdevdata->name);
}
- for (j = 0; j < VPIF_CAPTURE_MAX_DEVICES; j++) {
- ch = vpif_obj.dev[j];
- ch->channel_id = j;
- common = &(ch->common[VPIF_VIDEO_INDEX]);
- spin_lock_init(&common->irqlock);
- mutex_init(&common->lock);
- ch->video_dev->lock = &common->lock;
- /* Initialize prio member of channel object */
- v4l2_prio_init(&ch->prio);
- video_set_drvdata(ch->video_dev, ch);
-
- /* select input 0 */
- err = vpif_set_input(config, ch, 0);
- if (err)
- goto probe_out;
-
- err = video_register_device(ch->video_dev,
- VFL_TYPE_GRABBER, (j ? 1 : 0));
- if (err)
- goto probe_out;
- }
- v4l2_info(&vpif_obj.v4l2_dev, "VPIF capture driver initialized\n");
return 0;
-probe_out:
- for (k = 0; k < j; k++) {
- /* Get the pointer to the channel object */
- ch = vpif_obj.dev[k];
- /* Unregister video device */
- video_unregister_device(ch->video_dev);
- }
probe_subdev_out:
/* free sub devices memory */
kfree(vpif_obj.sd);
diff --git a/drivers/media/platform/davinci/vpif_capture.h b/drivers/media/platform/davinci/vpif_capture.h
index 3d3c1e5..1be47ab 100644
--- a/drivers/media/platform/davinci/vpif_capture.h
+++ b/drivers/media/platform/davinci/vpif_capture.h
@@ -145,6 +145,8 @@ struct vpif_device {
struct v4l2_device v4l2_dev;
struct channel_obj *dev[VPIF_CAPTURE_NUM_CHANNELS];
struct v4l2_subdev **sd;
+ struct v4l2_async_notifier notifier;
+ struct vpif_capture_config *config;
};
struct vpif_config_params {
diff --git a/include/media/davinci/vpif_types.h b/include/media/davinci/vpif_types.h
index 3882e06..e08bcde 100644
--- a/include/media/davinci/vpif_types.h
+++ b/include/media/davinci/vpif_types.h
@@ -81,5 +81,7 @@ struct vpif_capture_config {
struct vpif_subdev_info *subdev_info;
int subdev_count;
const char *card_name;
+ struct v4l2_async_subdev **asd; /* Flat array, arranged in groups */
+ int *asd_sizes; /* 0-terminated array of asd group sizes */
};
#endif /* _VPIF_TYPES_H */
--
1.7.4.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC 1/3] davinci: vpif: capture: add V4L2-async support
2013-01-09 13:41 ` [PATCH RFC 1/3] davinci: vpif: capture: add V4L2-async support Lad, Prabhakar
@ 2013-01-09 15:42 ` Hans Verkuil
2013-01-09 17:49 ` Prabhakar Lad
0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2013-01-09 15:42 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: LMML, LKML, Mauro Carvalho Chehab, Laurent Pinchart,
Hans Verkuil, Sekhar Nori, DLOS, Guennadi Liakhovetski, LAK, Lad,
Prabhakar, Sakari Ailus, Mauro Carvalho Chehab
On Wed 9 January 2013 14:41:25 Lad, Prabhakar wrote:
> Add support for asynchronous subdevice probing, using the v4l2-async API.
> The legacy synchronous mode is still supported too, which allows to
> gradually update drivers and platforms. The selected approach adds a
> notifier for each struct soc_camera_device instance, i.e. for each video
> device node, even when there are multiple such instances registered with a
> single soc-camera host simultaneously.
This comment was obviously copy-and-pasted from somewhere else :-)
>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
> drivers/media/platform/davinci/vpif_capture.c | 171 ++++++++++++++++++-------
> drivers/media/platform/davinci/vpif_capture.h | 2 +
> include/media/davinci/vpif_types.h | 2 +
> 3 files changed, 128 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index 5892d2b..a8b6588 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -34,6 +34,8 @@
> #include <linux/platform_device.h>
> #include <linux/io.h>
> #include <linux/slab.h>
> +
> +#include <media/v4l2-async.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-ioctl.h>
> #include <media/v4l2-chip-ident.h>
> @@ -2054,6 +2056,96 @@ vpif_init_free_channel_objects:
> return err;
> }
>
> +int vpif_async_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev_list *asdl)
> +{
> + int i = 0;
> +
> + if (!asdl->subdev) {
> + v4l2_err(vpif_dev->driver,
> + "%s(): Subdevice driver hasn't set subdev pointer!\n",
> + __func__);
> + return -EINVAL;
> + }
> + v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
> + asdl->subdev->name);
This v4l2_info shouldn't be necessary: when the subdev is loaded it will already
report that it is registered, so this would just duplicate things.
> +
> + for (i = 0; i < vpif_obj.config->subdev_count; i++)
> + if (!strcmp(vpif_obj.config->subdev_info[i].name,
> + asdl->subdev->name)) {
> + vpif_obj.sd[i] = asdl->subdev;
> + break;
> + }
> +
> + if (i >= vpif_obj.config->subdev_count)
> + return -EINVAL;
> +
> + return 0;
This function feels unnecessary. What you basically do here is to fill in
the vpif_obj.sd[i] pointer. Wouldn't it be easier if we added a function to
v4l2-device.c that will return a v4l2_subdev pointer based on the subdev name
or possibly that of a struct v4l2_async_hw_device by walking the subdevice
list that is stored in v4l2_device?
Then you could do something like this in vpif_probe_complete:
for (i = 0; i < vpif_obj.config->subdev_count; i++)
vpif_obj.sd[i] = v4l2_device_get_subdev_by_name(v4l2_dev,
vpif_obj.config->subdev_info[i].name);
and there would be no need for a bound callback.
Passing a struct v4l2_async_hw_device can be useful too: then you can
walk the list of subdevs passed in struct v4l2_async_notifier and you
don't need to fiddle with subdev names.
It's just a suggestion, but I think it will improve the code as the control
flow is more logical that way (async callbacks are always harder to understand).
> +}
> +
> +static int vpif_probe_complete(void)
> +{
> + struct common_obj *common;
> + struct channel_obj *ch;
> + int i, j, err, k;
> +
> + for (j = 0; j < VPIF_CAPTURE_MAX_DEVICES; j++) {
> + ch = vpif_obj.dev[j];
> + ch->channel_id = j;
> + common = &(ch->common[VPIF_VIDEO_INDEX]);
> + spin_lock_init(&common->irqlock);
> + mutex_init(&common->lock);
> + ch->video_dev->lock = &common->lock;
> + /* Initialize prio member of channel object */
> + v4l2_prio_init(&ch->prio);
> + video_set_drvdata(ch->video_dev, ch);
> +
> + /* select input 0 */
> + err = vpif_set_input(vpif_obj.config, ch, 0);
> + if (err)
> + goto probe_out;
> +
> + err = video_register_device(ch->video_dev,
> + VFL_TYPE_GRABBER, (j ? 1 : 0));
> + if (err)
> + goto probe_out;
> + }
> +
> + v4l2_info(&vpif_obj.v4l2_dev, "VPIF capture driver initialized\n");
> + return 0;
> +
> +probe_out:
> + for (k = 0; k < j; k++) {
> + /* Get the pointer to the channel object */
> + ch = vpif_obj.dev[k];
> + /* Unregister video device */
> + video_unregister_device(ch->video_dev);
> + }
> + kfree(vpif_obj.sd);
> + for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
> + ch = vpif_obj.dev[i];
> + /* Note: does nothing if ch->video_dev == NULL */
> + video_device_release(ch->video_dev);
> + }
> + v4l2_device_unregister(&vpif_obj.v4l2_dev);
> +
> + return err;
> +}
> +
> +int vpif_async_complete(struct v4l2_async_notifier *notifier)
> +{
> + return vpif_probe_complete();
Why this extra indirection? I'd remove it.
> +}
> +
> +void vpif_async_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev_list *asdl)
> +{
> + /*FIXME: Do we need this callback ? */
I think this callback can be removed.
> + v4l2_info(&vpif_obj.v4l2_dev, "unregistered sub device %s\n",
> + asdl->subdev->name);
> + return;
> +}
> +
> /**
> * vpif_probe : This function probes the vpif capture driver
> * @pdev: platform device pointer
> @@ -2064,12 +2156,10 @@ vpif_init_free_channel_objects:
> static __init int vpif_probe(struct platform_device *pdev)
> {
> struct vpif_subdev_info *subdevdata;
> - struct vpif_capture_config *config;
> - int i, j, k, err;
> + int i, j, err;
> int res_idx = 0;
> struct i2c_adapter *i2c_adap;
> struct channel_obj *ch;
> - struct common_obj *common;
> struct video_device *vfd;
> struct resource *res;
> int subdev_count;
> @@ -2146,10 +2236,9 @@ static __init int vpif_probe(struct platform_device *pdev)
> }
> }
>
> - i2c_adap = i2c_get_adapter(1);
> - config = pdev->dev.platform_data;
> + vpif_obj.config = pdev->dev.platform_data;
>
> - subdev_count = config->subdev_count;
> + subdev_count = vpif_obj.config->subdev_count;
> vpif_obj.sd = kzalloc(sizeof(struct v4l2_subdev *) * subdev_count,
> GFP_KERNEL);
> if (vpif_obj.sd == NULL) {
> @@ -2158,53 +2247,41 @@ static __init int vpif_probe(struct platform_device *pdev)
> goto vpif_sd_error;
> }
>
> - for (i = 0; i < subdev_count; i++) {
> - subdevdata = &config->subdev_info[i];
> - vpif_obj.sd[i] =
> - v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
> - i2c_adap,
> - &subdevdata->board_info,
> - NULL);
> + if (!vpif_obj.config->asd_sizes) {
> + i2c_adap = i2c_get_adapter(1);
> + for (i = 0; i < subdev_count; i++) {
> + subdevdata = &vpif_obj.config->subdev_info[i];
> + vpif_obj.sd[i] =
> + v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
> + i2c_adap,
> + &subdevdata->board_info,
> + NULL);
>
> - if (!vpif_obj.sd[i]) {
> - vpif_err("Error registering v4l2 subdevice\n");
> - goto probe_subdev_out;
> + if (!vpif_obj.sd[i]) {
> + vpif_err("Error registering v4l2 subdevice\n");
> + goto probe_subdev_out;
> + }
> + v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
> + subdevdata->name);
> + }
> + vpif_probe_complete();
> + } else {
> + vpif_obj.notifier.subdev = vpif_obj.config->asd;
> + vpif_obj.notifier.subdev_num = vpif_obj.config->asd_sizes[0];
> + vpif_obj.notifier.bound = vpif_async_bound;
> + vpif_obj.notifier.complete = vpif_async_complete;
> + vpif_obj.notifier.unbind = vpif_async_unbind;
> + err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,
> + &vpif_obj.notifier);
> + if (err) {
> + vpif_err("Error registering async notifier\n");
> + err = -EINVAL;
> + goto vpif_sd_error;
> }
> - v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
> - subdevdata->name);
> }
>
> - for (j = 0; j < VPIF_CAPTURE_MAX_DEVICES; j++) {
> - ch = vpif_obj.dev[j];
> - ch->channel_id = j;
> - common = &(ch->common[VPIF_VIDEO_INDEX]);
> - spin_lock_init(&common->irqlock);
> - mutex_init(&common->lock);
> - ch->video_dev->lock = &common->lock;
> - /* Initialize prio member of channel object */
> - v4l2_prio_init(&ch->prio);
> - video_set_drvdata(ch->video_dev, ch);
> -
> - /* select input 0 */
> - err = vpif_set_input(config, ch, 0);
> - if (err)
> - goto probe_out;
> -
> - err = video_register_device(ch->video_dev,
> - VFL_TYPE_GRABBER, (j ? 1 : 0));
> - if (err)
> - goto probe_out;
> - }
> - v4l2_info(&vpif_obj.v4l2_dev, "VPIF capture driver initialized\n");
> return 0;
>
> -probe_out:
> - for (k = 0; k < j; k++) {
> - /* Get the pointer to the channel object */
> - ch = vpif_obj.dev[k];
> - /* Unregister video device */
> - video_unregister_device(ch->video_dev);
> - }
> probe_subdev_out:
> /* free sub devices memory */
> kfree(vpif_obj.sd);
> diff --git a/drivers/media/platform/davinci/vpif_capture.h b/drivers/media/platform/davinci/vpif_capture.h
> index 3d3c1e5..1be47ab 100644
> --- a/drivers/media/platform/davinci/vpif_capture.h
> +++ b/drivers/media/platform/davinci/vpif_capture.h
> @@ -145,6 +145,8 @@ struct vpif_device {
> struct v4l2_device v4l2_dev;
> struct channel_obj *dev[VPIF_CAPTURE_NUM_CHANNELS];
> struct v4l2_subdev **sd;
> + struct v4l2_async_notifier notifier;
> + struct vpif_capture_config *config;
> };
>
> struct vpif_config_params {
> diff --git a/include/media/davinci/vpif_types.h b/include/media/davinci/vpif_types.h
> index 3882e06..e08bcde 100644
> --- a/include/media/davinci/vpif_types.h
> +++ b/include/media/davinci/vpif_types.h
> @@ -81,5 +81,7 @@ struct vpif_capture_config {
> struct vpif_subdev_info *subdev_info;
> int subdev_count;
> const char *card_name;
> + struct v4l2_async_subdev **asd; /* Flat array, arranged in groups */
> + int *asd_sizes; /* 0-terminated array of asd group sizes */
> };
> #endif /* _VPIF_TYPES_H */
>
Regards,
Hans
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC 1/3] davinci: vpif: capture: add V4L2-async support
2013-01-09 15:42 ` Hans Verkuil
@ 2013-01-09 17:49 ` Prabhakar Lad
0 siblings, 0 replies; 6+ messages in thread
From: Prabhakar Lad @ 2013-01-09 17:49 UTC (permalink / raw)
To: Hans Verkuil
Cc: LMML, LKML, Mauro Carvalho Chehab, Laurent Pinchart,
Hans Verkuil, Sekhar Nori, DLOS, Guennadi Liakhovetski, LAK, Lad,
Prabhakar, Sakari Ailus, Mauro Carvalho Chehab
Hi Hans,
Thanks for the review!
On Wed, Jan 9, 2013 at 9:12 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Wed 9 January 2013 14:41:25 Lad, Prabhakar wrote:
>> Add support for asynchronous subdevice probing, using the v4l2-async API.
>> The legacy synchronous mode is still supported too, which allows to
>> gradually update drivers and platforms. The selected approach adds a
>> notifier for each struct soc_camera_device instance, i.e. for each video
>> device node, even when there are multiple such instances registered with a
>> single soc-camera host simultaneously.
>
> This comment was obviously copy-and-pasted from somewhere else :-)
>
ah my bad :-)
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>> ---
>> drivers/media/platform/davinci/vpif_capture.c | 171 ++++++++++++++++++-------
>> drivers/media/platform/davinci/vpif_capture.h | 2 +
>> include/media/davinci/vpif_types.h | 2 +
>> 3 files changed, 128 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>> index 5892d2b..a8b6588 100644
>> --- a/drivers/media/platform/davinci/vpif_capture.c
>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>> @@ -34,6 +34,8 @@
>> #include <linux/platform_device.h>
>> #include <linux/io.h>
>> #include <linux/slab.h>
>> +
>> +#include <media/v4l2-async.h>
>> #include <media/v4l2-device.h>
>> #include <media/v4l2-ioctl.h>
>> #include <media/v4l2-chip-ident.h>
>> @@ -2054,6 +2056,96 @@ vpif_init_free_channel_objects:
>> return err;
>> }
>>
>> +int vpif_async_bound(struct v4l2_async_notifier *notifier,
>> + struct v4l2_async_subdev_list *asdl)
>> +{
>> + int i = 0;
>> +
>> + if (!asdl->subdev) {
>> + v4l2_err(vpif_dev->driver,
>> + "%s(): Subdevice driver hasn't set subdev pointer!\n",
>> + __func__);
>> + return -EINVAL;
>> + }
>> + v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
>> + asdl->subdev->name);
>
> This v4l2_info shouldn't be necessary: when the subdev is loaded it will already
> report that it is registered, so this would just duplicate things.
>
Ok
>> +
>> + for (i = 0; i < vpif_obj.config->subdev_count; i++)
>> + if (!strcmp(vpif_obj.config->subdev_info[i].name,
>> + asdl->subdev->name)) {
>> + vpif_obj.sd[i] = asdl->subdev;
>> + break;
>> + }
>> +
>> + if (i >= vpif_obj.config->subdev_count)
>> + return -EINVAL;
>> +
>> + return 0;
>
> This function feels unnecessary. What you basically do here is to fill in
> the vpif_obj.sd[i] pointer. Wouldn't it be easier if we added a function to
> v4l2-device.c that will return a v4l2_subdev pointer based on the subdev name
> or possibly that of a struct v4l2_async_hw_device by walking the subdevice
> list that is stored in v4l2_device?
>
> Then you could do something like this in vpif_probe_complete:
>
> for (i = 0; i < vpif_obj.config->subdev_count; i++)
> vpif_obj.sd[i] = v4l2_device_get_subdev_by_name(v4l2_dev,
> vpif_obj.config->subdev_info[i].name);
>
> and there would be no need for a bound callback.
>
> Passing a struct v4l2_async_hw_device can be useful too: then you can
> walk the list of subdevs passed in struct v4l2_async_notifier and you
> don't need to fiddle with subdev names.
>
> It's just a suggestion, but I think it will improve the code as the control
> flow is more logical that way (async callbacks are always harder to understand).
>
the bound callback, is being called on the subdev registration, the driver
is designed in such a way that there is subdev list and input list, and
each input may or may not have a subdev associated with it, when a input is
selected looping through the subdev list the appropriate is choose,
this was done by you :), the above code adds the subdev pointer in appropriate
index. To do this the bound callback would be required.
The second member of bound ie, asdl itself has pointer to subdev, so there isnt
a necessity to have a function returning a subdev by matching a name.
>> +}
>> +
>> +static int vpif_probe_complete(void)
>> +{
>> + struct common_obj *common;
>> + struct channel_obj *ch;
>> + int i, j, err, k;
>> +
>> + for (j = 0; j < VPIF_CAPTURE_MAX_DEVICES; j++) {
>> + ch = vpif_obj.dev[j];
>> + ch->channel_id = j;
>> + common = &(ch->common[VPIF_VIDEO_INDEX]);
>> + spin_lock_init(&common->irqlock);
>> + mutex_init(&common->lock);
>> + ch->video_dev->lock = &common->lock;
>> + /* Initialize prio member of channel object */
>> + v4l2_prio_init(&ch->prio);
>> + video_set_drvdata(ch->video_dev, ch);
>> +
>> + /* select input 0 */
>> + err = vpif_set_input(vpif_obj.config, ch, 0);
>> + if (err)
>> + goto probe_out;
>> +
>> + err = video_register_device(ch->video_dev,
>> + VFL_TYPE_GRABBER, (j ? 1 : 0));
>> + if (err)
>> + goto probe_out;
>> + }
>> +
>> + v4l2_info(&vpif_obj.v4l2_dev, "VPIF capture driver initialized\n");
>> + return 0;
>> +
>> +probe_out:
>> + for (k = 0; k < j; k++) {
>> + /* Get the pointer to the channel object */
>> + ch = vpif_obj.dev[k];
>> + /* Unregister video device */
>> + video_unregister_device(ch->video_dev);
>> + }
>> + kfree(vpif_obj.sd);
>> + for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
>> + ch = vpif_obj.dev[i];
>> + /* Note: does nothing if ch->video_dev == NULL */
>> + video_device_release(ch->video_dev);
>> + }
>> + v4l2_device_unregister(&vpif_obj.v4l2_dev);
>> +
>> + return err;
>> +}
>> +
>> +int vpif_async_complete(struct v4l2_async_notifier *notifier)
>> +{
>> + return vpif_probe_complete();
>
> Why this extra indirection? I'd remove it.
>
The complete notifier is called when all the subdevices have been registered
this is only when the video_register_device() is called this piece of code which
is common for asynchronous and synchronous probing so included in a function
so that its used by both.
>> +}
>> +
>> +void vpif_async_unbind(struct v4l2_async_notifier *notifier,
>> + struct v4l2_async_subdev_list *asdl)
>> +{
>> + /*FIXME: Do we need this callback ? */
>
> I think this callback can be removed.
>
Yes
Regards,
--Prabhakar
>> + v4l2_info(&vpif_obj.v4l2_dev, "unregistered sub device %s\n",
>> + asdl->subdev->name);
>> + return;
>> +}
>> +
>> /**
>> * vpif_probe : This function probes the vpif capture driver
>> * @pdev: platform device pointer
>> @@ -2064,12 +2156,10 @@ vpif_init_free_channel_objects:
>> static __init int vpif_probe(struct platform_device *pdev)
>> {
>> struct vpif_subdev_info *subdevdata;
>> - struct vpif_capture_config *config;
>> - int i, j, k, err;
>> + int i, j, err;
>> int res_idx = 0;
>> struct i2c_adapter *i2c_adap;
>> struct channel_obj *ch;
>> - struct common_obj *common;
>> struct video_device *vfd;
>> struct resource *res;
>> int subdev_count;
>> @@ -2146,10 +2236,9 @@ static __init int vpif_probe(struct platform_device *pdev)
>> }
>> }
>>
>> - i2c_adap = i2c_get_adapter(1);
>> - config = pdev->dev.platform_data;
>> + vpif_obj.config = pdev->dev.platform_data;
>>
>> - subdev_count = config->subdev_count;
>> + subdev_count = vpif_obj.config->subdev_count;
>> vpif_obj.sd = kzalloc(sizeof(struct v4l2_subdev *) * subdev_count,
>> GFP_KERNEL);
>> if (vpif_obj.sd == NULL) {
>> @@ -2158,53 +2247,41 @@ static __init int vpif_probe(struct platform_device *pdev)
>> goto vpif_sd_error;
>> }
>>
>> - for (i = 0; i < subdev_count; i++) {
>> - subdevdata = &config->subdev_info[i];
>> - vpif_obj.sd[i] =
>> - v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
>> - i2c_adap,
>> - &subdevdata->board_info,
>> - NULL);
>> + if (!vpif_obj.config->asd_sizes) {
>> + i2c_adap = i2c_get_adapter(1);
>> + for (i = 0; i < subdev_count; i++) {
>> + subdevdata = &vpif_obj.config->subdev_info[i];
>> + vpif_obj.sd[i] =
>> + v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
>> + i2c_adap,
>> + &subdevdata->board_info,
>> + NULL);
>>
>> - if (!vpif_obj.sd[i]) {
>> - vpif_err("Error registering v4l2 subdevice\n");
>> - goto probe_subdev_out;
>> + if (!vpif_obj.sd[i]) {
>> + vpif_err("Error registering v4l2 subdevice\n");
>> + goto probe_subdev_out;
>> + }
>> + v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
>> + subdevdata->name);
>> + }
>> + vpif_probe_complete();
>> + } else {
>> + vpif_obj.notifier.subdev = vpif_obj.config->asd;
>> + vpif_obj.notifier.subdev_num = vpif_obj.config->asd_sizes[0];
>> + vpif_obj.notifier.bound = vpif_async_bound;
>> + vpif_obj.notifier.complete = vpif_async_complete;
>> + vpif_obj.notifier.unbind = vpif_async_unbind;
>> + err = v4l2_async_notifier_register(&vpif_obj.v4l2_dev,
>> + &vpif_obj.notifier);
>> + if (err) {
>> + vpif_err("Error registering async notifier\n");
>> + err = -EINVAL;
>> + goto vpif_sd_error;
>> }
>> - v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
>> - subdevdata->name);
>> }
>>
>> - for (j = 0; j < VPIF_CAPTURE_MAX_DEVICES; j++) {
>> - ch = vpif_obj.dev[j];
>> - ch->channel_id = j;
>> - common = &(ch->common[VPIF_VIDEO_INDEX]);
>> - spin_lock_init(&common->irqlock);
>> - mutex_init(&common->lock);
>> - ch->video_dev->lock = &common->lock;
>> - /* Initialize prio member of channel object */
>> - v4l2_prio_init(&ch->prio);
>> - video_set_drvdata(ch->video_dev, ch);
>> -
>> - /* select input 0 */
>> - err = vpif_set_input(config, ch, 0);
>> - if (err)
>> - goto probe_out;
>> -
>> - err = video_register_device(ch->video_dev,
>> - VFL_TYPE_GRABBER, (j ? 1 : 0));
>> - if (err)
>> - goto probe_out;
>> - }
>> - v4l2_info(&vpif_obj.v4l2_dev, "VPIF capture driver initialized\n");
>> return 0;
>>
>> -probe_out:
>> - for (k = 0; k < j; k++) {
>> - /* Get the pointer to the channel object */
>> - ch = vpif_obj.dev[k];
>> - /* Unregister video device */
>> - video_unregister_device(ch->video_dev);
>> - }
>> probe_subdev_out:
>> /* free sub devices memory */
>> kfree(vpif_obj.sd);
>> diff --git a/drivers/media/platform/davinci/vpif_capture.h b/drivers/media/platform/davinci/vpif_capture.h
>> index 3d3c1e5..1be47ab 100644
>> --- a/drivers/media/platform/davinci/vpif_capture.h
>> +++ b/drivers/media/platform/davinci/vpif_capture.h
>> @@ -145,6 +145,8 @@ struct vpif_device {
>> struct v4l2_device v4l2_dev;
>> struct channel_obj *dev[VPIF_CAPTURE_NUM_CHANNELS];
>> struct v4l2_subdev **sd;
>> + struct v4l2_async_notifier notifier;
>> + struct vpif_capture_config *config;
>> };
>>
>> struct vpif_config_params {
>> diff --git a/include/media/davinci/vpif_types.h b/include/media/davinci/vpif_types.h
>> index 3882e06..e08bcde 100644
>> --- a/include/media/davinci/vpif_types.h
>> +++ b/include/media/davinci/vpif_types.h
>> @@ -81,5 +81,7 @@ struct vpif_capture_config {
>> struct vpif_subdev_info *subdev_info;
>> int subdev_count;
>> const char *card_name;
>> + struct v4l2_async_subdev **asd; /* Flat array, arranged in groups */
>> + int *asd_sizes; /* 0-terminated array of asd group sizes */
>> };
>> #endif /* _VPIF_TYPES_H */
>>
>
> Regards,
>
> Hans
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH RFC 2/3] tvp514x: support asynchronous probing
2013-01-09 13:41 [PATCH RFC 0/3] vpif capture support for async subdevice probing Lad, Prabhakar
2013-01-09 13:41 ` [PATCH RFC 1/3] davinci: vpif: capture: add V4L2-async support Lad, Prabhakar
@ 2013-01-09 13:41 ` Lad, Prabhakar
2013-01-09 13:41 ` [PATCH RFC 3/3] ARM: da850/omap-l138: vpif capture convert to asynchronously register of subdev Lad, Prabhakar
2 siblings, 0 replies; 6+ messages in thread
From: Lad, Prabhakar @ 2013-01-09 13:41 UTC (permalink / raw)
To: LMML
Cc: LKML, Mauro Carvalho Chehab, Laurent Pinchart, Hans Verkuil,
Sekhar Nori, DLOS, Guennadi Liakhovetski, LAK, Lad, Prabhakar,
Hans Verkuil, Sakari Ailus, Mauro Carvalho Chehab
Both synchronous and asynchronous tvp514x subdevice probing is supported by
this patch.
Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/media/i2c/tvp514x.c | 20 ++++++++++++++------
1 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index aa94ebc..a4f0a70 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -34,6 +34,7 @@
#include <linux/videodev2.h>
#include <linux/module.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-device.h>
#include <media/v4l2-common.h>
#include <media/v4l2-mediabus.h>
@@ -102,6 +103,7 @@ struct tvp514x_decoder {
struct v4l2_ctrl_handler hdl;
struct tvp514x_reg tvp514x_regs[ARRAY_SIZE(tvp514x_reg_list_default)];
const struct tvp514x_platform_data *pdata;
+ struct v4l2_async_subdev_list asdl;
int ver;
int streaming;
@@ -941,22 +943,22 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
struct tvp514x_decoder *decoder;
struct v4l2_subdev *sd;
+ int ret;
/* Check if the adapter supports the needed features */
if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
return -EIO;
- if (!client->dev.platform_data) {
- v4l2_err(client, "No platform data!!\n");
- return -ENODEV;
- }
-
decoder = devm_kzalloc(&client->dev, sizeof(*decoder), GFP_KERNEL);
if (!decoder)
return -ENOMEM;
/* Initialize the tvp514x_decoder with default configuration */
*decoder = tvp514x_dev;
+ if (!client->dev.platform_data) {
+ v4l2_err(client, "No platform data!!\n");
+ return -EPROBE_DEFER;
+ }
/* Copy default register configuration */
memcpy(decoder->tvp514x_regs, tvp514x_reg_list_default,
sizeof(tvp514x_reg_list_default));
@@ -980,6 +982,11 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
/* Register with V4L2 layer as slave device */
sd = &decoder->sd;
+ decoder->asdl.subdev = &decoder->sd;
+ decoder->asdl.dev = &client->dev;
+ ret = v4l2_async_subdev_bind(&decoder->asdl);
+ if (ret < 0)
+ return ret;
v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
v4l2_ctrl_handler_init(&decoder->hdl, 5);
@@ -1004,7 +1011,7 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
- return 0;
+ return v4l2_async_subdev_bound(&decoder->asdl);
}
@@ -1020,6 +1027,7 @@ static int tvp514x_remove(struct i2c_client *client)
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct tvp514x_decoder *decoder = to_decoder(sd);
+ v4l2_async_subdev_unbind(&decoder->asdl);
v4l2_device_unregister_subdev(sd);
v4l2_ctrl_handler_free(&decoder->hdl);
return 0;
--
1.7.4.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH RFC 3/3] ARM: da850/omap-l138: vpif capture convert to asynchronously register of subdev
2013-01-09 13:41 [PATCH RFC 0/3] vpif capture support for async subdevice probing Lad, Prabhakar
2013-01-09 13:41 ` [PATCH RFC 1/3] davinci: vpif: capture: add V4L2-async support Lad, Prabhakar
2013-01-09 13:41 ` [PATCH RFC 2/3] tvp514x: support asynchronous probing Lad, Prabhakar
@ 2013-01-09 13:41 ` Lad, Prabhakar
2 siblings, 0 replies; 6+ messages in thread
From: Lad, Prabhakar @ 2013-01-09 13:41 UTC (permalink / raw)
To: LMML
Cc: LKML, Mauro Carvalho Chehab, Laurent Pinchart, Hans Verkuil,
Sekhar Nori, DLOS, Guennadi Liakhovetski, LAK, Lad, Prabhakar
Register the tvp514x decoder devices directly in board platform
data instead of letting the vpif capture driver register them at
their run-time. This uses the V4L2 asynchronous subdevice probing capability.
Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
Cc: Sekhar Nori <nsekhar@ti.com>
---
arch/arm/mach-davinci/board-da850-evm.c | 57 +++++++++++++++++++++++++++----
1 files changed, 50 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 0299915..089c127 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -49,6 +49,7 @@
#include <media/tvp514x.h>
#include <media/adv7343.h>
+#include <media/v4l2-async.h>
#define DA850_EVM_PHY_ID "davinci_mdio-0:00"
#define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
@@ -732,6 +733,12 @@ static struct pca953x_platform_data da850_evm_bb_expander_info = {
.names = da850_evm_bb_exp,
};
+static struct tvp514x_platform_data tvp5146_pdata = {
+ .clk_polarity = 0,
+ .hs_polarity = 1,
+ .vs_polarity = 1,
+};
+
static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
{
I2C_BOARD_INFO("tlv320aic3x", 0x18),
@@ -744,6 +751,14 @@ static struct i2c_board_info __initdata da850_evm_i2c_devices[] = {
I2C_BOARD_INFO("tca6416", 0x21),
.platform_data = &da850_evm_bb_expander_info,
},
+ {
+ I2C_BOARD_INFO("tvp5146", 0x5c),
+ .platform_data = &tvp5146_pdata,
+ }, {
+ I2C_BOARD_INFO("tvp5146", 0x5d),
+ .platform_data = &tvp5146_pdata,
+ },
+
};
static struct davinci_i2c_platform_data da850_evm_i2c_0_pdata = {
@@ -1170,15 +1185,10 @@ static __init int da850_evm_init_cpufreq(void) { return 0; }
#if defined(CONFIG_DA850_UI_SD_VIDEO_PORT)
-#define TVP5147_CH0 "tvp514x-0"
-#define TVP5147_CH1 "tvp514x-1"
+#define TVP5147_CH0 "tvp514x 1-005d"
+#define TVP5147_CH1 "tvp514x 1-005c"
/* VPIF capture configuration */
-static struct tvp514x_platform_data tvp5146_pdata = {
- .clk_polarity = 0,
- .hs_polarity = 1,
- .vs_polarity = 1,
-};
#define TVP514X_STD_ALL (V4L2_STD_NTSC | V4L2_STD_PAL)
@@ -1229,6 +1239,37 @@ static struct vpif_subdev_info da850_vpif_capture_sdev_info[] = {
},
};
+static struct v4l2_async_subdev tvp1_sd = {
+ .hw = {
+ .bus_type = V4L2_ASYNC_BUS_I2C,
+ .match.i2c = {
+ .adapter_id = 1,
+ .address = 0x5c,
+ },
+ },
+};
+
+static struct v4l2_async_subdev tvp2_sd = {
+ .hw = {
+ .bus_type = V4L2_ASYNC_BUS_I2C,
+ .match.i2c = {
+ .adapter_id = 1,
+ .address = 0x5d,
+ },
+ },
+};
+
+static struct v4l2_async_subdev *vpif_capture_async_subdevs[] = {
+ /* Single 2-element group */
+ &tvp1_sd,
+ &tvp2_sd,
+};
+
+static int vpif_capture_async_subdev_sizes[] = {
+ ARRAY_SIZE(vpif_capture_async_subdevs),
+ 0,
+};
+
static struct vpif_capture_config da850_vpif_capture_config = {
.subdev_info = da850_vpif_capture_sdev_info,
.subdev_count = ARRAY_SIZE(da850_vpif_capture_sdev_info),
@@ -1253,6 +1294,8 @@ static struct vpif_capture_config da850_vpif_capture_config = {
},
},
.card_name = "DA850/OMAP-L138 Video Capture",
+ .asd = vpif_capture_async_subdevs,
+ .asd_sizes = vpif_capture_async_subdev_sizes,
};
/* VPIF display configuration */
--
1.7.4.1
^ permalink raw reply [flat|nested] 6+ messages in thread