Hi Sakari, Thanks for the patch. On Monday 20 February 2012 03:56:56 Sakari Ailus wrote: > v4l2_subdev_link_validate() is the default op for validating a link. In V4L2 > subdev context, it is used to call a pad op which performs the proper link > check without much extra work. > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx> > --- > Documentation/video4linux/v4l2-framework.txt | 12 +++++ > drivers/media/video/v4l2-subdev.c | 69 > ++++++++++++++++++++++++++ include/media/v4l2-subdev.h | > 12 +++++ > 3 files changed, 93 insertions(+), 0 deletions(-) > > diff --git a/Documentation/video4linux/v4l2-framework.txt > b/Documentation/video4linux/v4l2-framework.txt index f06c563..9d341bc > 100644 > --- a/Documentation/video4linux/v4l2-framework.txt > +++ b/Documentation/video4linux/v4l2-framework.txt > @@ -312,6 +312,18 @@ If the subdev driver intends to process video and > integrate with the media framework, it must implement format related > functionality using > v4l2_subdev_pad_ops instead of v4l2_subdev_video_ops. > > +In that case, the subdev driver may set the link_validate field to provide > +its own link validation function. The link validation function is called > for +every link in the pipeline where both of the ends of the links are > V4L2 +sub-devices. The driver is still responsible for validating the > correctness +of the format configuration between sub-devices and video > nodes. > + > +If link_validate op is not set, the default function > +v4l2_subdev_link_validate_default() is used instead. This function ensures > +that width, height and the media bus pixel code are equal on both source > and +sink of the link. Subdev drivers are also free to use this function to > +perform the checks mentioned above in addition to their own checks. + > A device (bridge) driver needs to register the v4l2_subdev with the > v4l2_device: > > diff --git a/drivers/media/video/v4l2-subdev.c > b/drivers/media/video/v4l2-subdev.c index ef27144..d2ec552 100644 > --- a/drivers/media/video/v4l2-subdev.c > +++ b/drivers/media/video/v4l2-subdev.c > @@ -379,6 +379,75 @@ const struct v4l2_file_operations v4l2_subdev_fops = { > .poll = subdev_poll, > }; > > +#ifdef CONFIG_MEDIA_CONTROLLER > +int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd, > + struct media_link *link, > + struct v4l2_subdev_format *source_fmt, > + struct v4l2_subdev_format *sink_fmt) > +{ > + if (source_fmt->format.width != sink_fmt->format.width > + || source_fmt->format.height != sink_fmt->format.height > + || source_fmt->format.code != sink_fmt->format.code) > + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default); > + > +static struct v4l2_subdev_format > +*v4l2_subdev_link_validate_get_format(struct media_pad *pad, > + struct v4l2_subdev_format *fmt) Why do you return a pointer to the passed struct v4l2_subdev_format instead of just an error code ? > +{ > + int rval; I would have used ret instead of rval as that's the preferred variable name in this file (OK, I'm cheating, there's a single instance of ret, and I wrote that one ;-)). > + > + switch (media_entity_type(pad->entity)) { > + case MEDIA_ENT_T_V4L2_SUBDEV: > + fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE; > + fmt->pad = pad->index; > + rval = v4l2_subdev_call(media_entity_to_v4l2_subdev( > + pad->entity), > + pad, get_fmt, NULL, fmt); > + if (rval < 0) > + return NULL; > + return fmt; > + default: > + WARN(1, "Driver bug! Wrong media entity type %d, entity %s\n", > + media_entity_type(pad->entity), pad->entity->name); > + /* Fall through */ > + case MEDIA_ENT_T_DEVNODE_V4L: > + return NULL; > + } > +} > + > +int v4l2_subdev_link_validate(struct media_link *link) > +{ > + struct v4l2_subdev *sink; > + struct v4l2_subdev_format _sink_fmt, _source_fmt; > + struct v4l2_subdev_format *sink_fmt, *source_fmt; > + int rval; > + > + source_fmt = v4l2_subdev_link_validate_get_format( > + link->source, &_source_fmt); > + if (!source_fmt) > + return 0; > + > + sink_fmt = v4l2_subdev_link_validate_get_format( > + link->sink, &_sink_fmt); > + if (!sink_fmt) > + return 0; > + > + sink = media_entity_to_v4l2_subdev(link->sink->entity); > + > + rval = v4l2_subdev_call(sink, pad, link_validate, link, > + source_fmt, sink_fmt); > + if (rval < 0 && rval != -ENOIOCTLCMD) > + return rval; > + return v4l2_subdev_link_validate_default(sink, link, source_fmt, > + sink_fmt); I don't think you should call v4l2_subdev_link_validate_default() if the link_validate operation is implemented and returns that the link is valid. The subdev might have weaker requirements than the default. > +} > +EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate); > +#endif /* CONFIG_MEDIA_CONTROLLER */ > + > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops > *ops) { > INIT_LIST_HEAD(&sd->list); > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index d48dae5..f115608 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -470,6 +470,11 @@ struct v4l2_subdev_pad_ops { > struct v4l2_subdev_selection *sel); > int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, > struct v4l2_subdev_selection *sel); > +#ifdef CONFIG_MEDIA_CONTROLLER > + int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link, > + struct v4l2_subdev_format *source_fmt, > + struct v4l2_subdev_format *sink_fmt); > +#endif /* CONFIG_MEDIA_CONTROLLER */ > }; > > struct v4l2_subdev_ops { > @@ -603,6 +608,13 @@ static inline void *v4l2_get_subdev_hostdata(const > struct v4l2_subdev *sd) return sd->host_priv; > } > > +#ifdef CONFIG_MEDIA_CONTROLLER > +int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd, > + struct media_link *link, > + struct v4l2_subdev_format *source_fmt, > + struct v4l2_subdev_format *sink_fmt); > +int v4l2_subdev_link_validate(struct media_link *link); > +#endif /* CONFIG_MEDIA_CONTROLLER */ > void v4l2_subdev_init(struct v4l2_subdev *sd, > const struct v4l2_subdev_ops *ops); -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html