[PATCH RFC 1/3] davinci: vpif: capture: add V4L2-async support
Hans Verkuil
hverkuil at xs4all.nl
Wed Jan 9 10:42:38 EST 2013
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 at ti.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski at gmx.de>
> Cc: Hans Verkuil <hans.verkuil at cisco.com>
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus at iki.fi>
> Cc: Mauro Carvalho Chehab <mchehab at 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
More information about the linux-arm-kernel
mailing list