Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:09 EET Niklas Söderlund wrote:
> In preparation for Gen3 support move the subdevice initialization and
> clean up from rvin_v4l2_{register,unregister}() directly to the async
> callbacks. This simplifies the addition of Gen3 support as the
> rvin_v4l2_register() can be shared for both Gen2 and Gen3 while direct
> subdevice control are only used on Gen2.
>
> While moving this code drop a large comment which is copied from the
> framework documentation and fold rvin_mbus_supported() into its only
> caller. Also move the initialization and cleanup code to separate
> functions to increase readability.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---
> drivers/media/platform/rcar-vin/rcar-core.c | 108 ++++++++++++++++++------
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 35 ---------
> 2 files changed, 74 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 47f06acde2e698f2..663309ca9c04f208 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -46,46 +46,88 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int
> direction) return -EINVAL;
> }
>
> -static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
> +/* The vin lock shuld be held when calling the subdevice attach and detach
> */ +static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
> + struct v4l2_subdev *subdev)
> {
> - struct v4l2_subdev *sd = entity->subdev;
> struct v4l2_subdev_mbus_code_enum code = {
> .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> };
> + 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->digital->code = 0;
> code.index = 0;
> - code.pad = entity->source_pad;
> - while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) {
> + code.pad = vin->digital->source_pad;
> + while (!vin->digital->code &&
> + !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> code.index++;
> switch (code.code) {
> case MEDIA_BUS_FMT_YUYV8_1X16:
> case MEDIA_BUS_FMT_UYVY8_2X8:
> case MEDIA_BUS_FMT_UYVY10_2X10:
> case MEDIA_BUS_FMT_RGB888_1X24:
> - entity->code = code.code;
> - return true;
> + vin->digital->code = code.code;
> + vin_dbg(vin, "Found media bus format for %s: %d\n",
> + subdev->name, vin->digital->code);
> + break;
> default:
> break;
> }
> }
>
> - return false;
> -}
> -
> -static int rvin_digital_notify_complete(struct v4l2_async_notifier
> *notifier) -{
> - struct rvin_dev *vin = notifier_to_vin(notifier);
> - int ret;
> -
> - /* Verify subdevices mbus format */
> - if (!rvin_mbus_supported(vin->digital)) {
> + if (!vin->digital->code) {
> vin_err(vin, "Unsupported media bus format for %s\n",
> - vin->digital->subdev->name);
> + subdev->name);
> return -EINVAL;
> }
>
> - vin_dbg(vin, "Found media bus format for %s: %d\n",
> - vin->digital->subdev->name, vin->digital->code);
> + /* Read tvnorms */
> + ret = v4l2_subdev_call(subdev, video, g_tvnorms, &vin->vdev.tvnorms);
> + if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> + return ret;
> +
> + /* Add the controls */
> + ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> + if (ret < 0)
> + return ret;
> +
> + ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> + NULL);
> + if (ret < 0) {
> + v4l2_ctrl_handler_free(&vin->ctrl_handler);
> + return ret;
> + }
> +
> + vin->vdev.ctrl_handler = &vin->ctrl_handler;
> +
> + vin->digital->subdev = subdev;
> +
> + return 0;
> +}
> +
> +static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
> +{
> + 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);
> + int ret;
>
> ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> if (ret < 0) {
> @@ -103,8 +145,10 @@ static void rvin_digital_notify_unbind(struct
> v4l2_async_notifier *notifier, struct rvin_dev *vin =
> notifier_to_vin(notifier);
>
> vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
> - rvin_v4l2_unregister(vin);
> - vin->digital->subdev = NULL;
> +
> + mutex_lock(&vin->lock);
> + rvin_digital_subdevice_detach(vin);
> + mutex_unlock(&vin->lock);
> }
>
> static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> @@ -114,33 +158,27 @@ static int rvin_digital_notify_bound(struct
> v4l2_async_notifier *notifier, struct rvin_dev *vin =
> notifier_to_vin(notifier);
> int ret;
>
> + mutex_lock(&vin->lock);
> + ret = rvin_digital_subdevice_attach(vin, subdev);
> + mutex_unlock(&vin->lock);
> + if (ret)
> + return ret;
> +
> v4l2_set_subdev_hostdata(subdev, vin);
>
> - /* 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;
> -
> - vin->digital->subdev = subdev;
> -
> vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
> subdev->name, vin->digital->source_pad,
> vin->digital->sink_pad);
>
> return 0;
> }
> +
> static const struct v4l2_async_notifier_operations rvin_digital_notify_ops
> = { .bound = rvin_digital_notify_bound,
> .unbind = rvin_digital_notify_unbind,
> .complete = rvin_digital_notify_complete,
> };
>
> -
> static int rvin_digital_parse_v4l2(struct device *dev,
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd)
> @@ -277,6 +315,8 @@ static int rcar_vin_remove(struct platform_device *pdev)
> v4l2_async_notifier_unregister(&vin->notifier);
> v4l2_async_notifier_cleanup(&vin->notifier);
>
> + v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +
> rvin_dma_unregister(vin);
>
> return 0;
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> 32a658214f48fa49..4a0610a6b4503501 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -847,9 +847,6 @@ void rvin_v4l2_unregister(struct rvin_dev *vin)
> v4l2_info(&vin->v4l2_dev, "Removing %s\n",
> video_device_node_name(&vin->vdev));
>
> - /* Checks internaly if handlers have been init or not */
> - v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -
> /* Checks internaly if vdev have been init or not */
> video_unregister_device(&vin->vdev);
> }
> @@ -872,41 +869,10 @@ static void rvin_notify(struct v4l2_subdev *sd,
> int rvin_v4l2_register(struct rvin_dev *vin)
> {
> struct video_device *vdev = &vin->vdev;
> - struct v4l2_subdev *sd = vin_to_source(vin);
> int ret;
>
> - v4l2_set_subdev_hostdata(sd, vin);
> -
> vin->v4l2_dev.notify = rvin_notify;
>
> - ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> - if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> - return ret;
> -
> - if (vin->vdev.tvnorms == 0) {
> - /* Disable the STD API if there are no tvnorms defined */
> - v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> - v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> - v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> - v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> - }
> -
> - /* Add the controls */
> - /*
> - * Currently the subdev with the largest number of controls (13) is
> - * ov6550. So let's pick 16 as a hint for the control handler. Note
> - * that this is a hint only: too large and you waste some memory, too
> - * small and there is a (very) small performance hit when looking up
> - * controls in the internal hash.
> - */
> - ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> - if (ret < 0)
> - return ret;
> -
> - ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
> - if (ret < 0)
> - return ret;
> -
> /* video node */
> vdev->fops = &rvin_fops;
> vdev->v4l2_dev = &vin->v4l2_dev;
> @@ -915,7 +881,6 @@ int rvin_v4l2_register(struct rvin_dev *vin)
> vdev->release = video_device_release_empty;
> vdev->ioctl_ops = &rvin_ioctl_ops;
> vdev->lock = &vin->lock;
> - vdev->ctrl_handler = &vin->ctrl_handler;
> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
> V4L2_CAP_READWRITE;
--
Regards,
Laurent Pinchart
