Hi Jacopo,
Thanks for your patch.
I only have one generic comment to this patch as I fear comments on 1/4
have the potential to change quiet a few things here :-(
On 2018-05-16 14:16:54 +0200, Jacopo Mondi wrote:
> Handle media-controller in the digital notifier operations (bound,
> unbind and complete) registered for VIN instances handling a digital
> input subdevice.
>
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 94
> ++++++++++++++++++++---------
> 1 file changed, 65 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c
> index 0ea21ab..1003c8c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -379,7 +379,7 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int
> direction)
> * Digital async notifier
> */
>
> -/* The vin lock should be held when calling the subdevice attach and detach
> */
> +/* The vin lock should be held when calling the subdevice attach. */
> static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> struct v4l2_subdev *subdev)
> {
> @@ -388,15 +388,6 @@ static int rvin_digital_subdevice_attach(struct rvin_dev
> *vin,
> };
> int ret;
>
> - /* Find source and sink pad of remote subdevice */
> - ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> - if (ret < 0)
> - return ret;
> - vin->digital->source_pad = ret;
> -
> - ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> - vin->digital->sink_pad = ret < 0 ? 0 : ret;
> -
> /* Find compatible subdevices mbus format */
> vin->mbus_code = 0;
> code.index = 0;
> @@ -450,23 +441,14 @@ static int rvin_digital_subdevice_attach(struct
> rvin_dev *vin,
>
> vin->vdev.ctrl_handler = &vin->ctrl_handler;
>
> - vin->digital->subdev = subdev;
> -
> return 0;
> }
>
> -static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
Please don't fold this function into the caller. I tired the same thing
when adding Gen3 support but keeping the _attach and _detach functions
keeps a nice abstraction on what happens when we add and remove a
parallel subdevice to the internal data structures separated from the
housekeeping stuff in the callbacks :-)
> -{
> - rvin_v4l2_unregister(vin);
> - v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -
> - vin->vdev.ctrl_handler = NULL;
> - vin->digital->subdev = NULL;
> -}
> -
> static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
> {
> struct rvin_dev *vin = notifier_to_vin(notifier);
> + struct media_entity *source;
> + struct media_entity *sink;
> int ret;
>
> ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> @@ -475,7 +457,26 @@ static int rvin_digital_notify_complete(struct
> v4l2_async_notifier *notifier)
> return ret;
> }
>
> - return rvin_v4l2_register(vin);
> + if (!video_is_registered(&vin->vdev)) {
> + ret = rvin_v4l2_register(vin);
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (!vin->info->use_mc)
> + return 0;
> +
> + /* If we're running with media controller, link the subdevice. */
> + source = &vin->digital->subdev->entity;
> + sink = &vin->vdev.entity;
> +
> + ret = media_create_pad_link(source, vin->digital->source_pad,
> + sink, vin->digital->sink_pad, 0);
> + if (ret)
> + vin_err(vin, "Error adding link from %s to %s\n",
> + source->name, sink->name);
> +
> + return ret;
> }
>
> static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
> @@ -487,7 +488,14 @@ static void rvin_digital_notify_unbind(struct
> v4l2_async_notifier *notifier,
> vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
>
> mutex_lock(&vin->lock);
> - rvin_digital_subdevice_detach(vin);
> +
> + rvin_v4l2_unregister(vin);
> + vin->digital->subdev = NULL;
> + if (!vin->info->use_mc) {
> + v4l2_ctrl_handler_free(&vin->ctrl_handler);
> + vin->vdev.ctrl_handler = NULL;
> + }
> +
> mutex_unlock(&vin->lock);
> }
>
> @@ -499,10 +507,29 @@ static int rvin_digital_notify_bound(struct
> v4l2_async_notifier *notifier,
> int ret;
>
> mutex_lock(&vin->lock);
> - ret = rvin_digital_subdevice_attach(vin, subdev);
> - mutex_unlock(&vin->lock);
> - if (ret)
> +
> + /* Find source and sink pad of remote subdevice */
> + ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> + if (ret < 0) {
> + mutex_unlock(&vin->lock);
> return ret;
> + }
> + vin->digital->source_pad = ret;
> +
> + ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> + vin->digital->sink_pad = ret < 0 ? 0 : ret;
> +
> + vin->digital->subdev = subdev;
> +
> + if (!vin->info->use_mc) {
> + ret = rvin_digital_subdevice_attach(vin, subdev);
> + if (ret) {
> + mutex_unlock(&vin->lock);
> + return ret;
> + }
> + }
> +
> + mutex_unlock(&vin->lock);
>
> v4l2_set_subdev_hostdata(subdev, vin);
>
> @@ -555,9 +582,10 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
> {
> int ret;
>
> - ret = v4l2_async_notifier_parse_fwnode_endpoints(
> + ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> vin->dev, &vin->notifier,
> - sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
> + sizeof(struct rvin_graph_entity), RVIN_PORT_DIGITAL,
> + rvin_digital_parse_v4l2);
> if (ret)
> return ret;
>
> @@ -567,6 +595,13 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
> vin_dbg(vin, "Found digital subdevice %pOF\n",
> to_of_node(vin->digital->asd.match.fwnode));
>
> + /*
> + * If we run with media-controller, notifiers will be registered
> + * later, once all VINs have probed.
> + */
> + if (vin->info->use_mc)
> + return 0;
> +
> vin->notifier.ops = &rvin_digital_notify_ops;
> ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
> if (ret < 0) {
> @@ -596,7 +631,8 @@ static int rvin_group_notify_complete(struct
> v4l2_async_notifier *notifier)
>
> /* Register all video nodes for the group. */
> for (i = 0; i < RCAR_VIN_NUM; i++) {
> - if (vin->group->vin[i]) {
> + if (vin->group->vin[i] &&
> + !video_is_registered(&vin->group->vin[i]->vdev)) {
> ret = rvin_v4l2_register(vin->group->vin[i]);
> if (ret)
> return ret;
> --
> 2.7.4
>
--
Regards,
Niklas Söderlund
