Re: [PATCH v12 06/33] rcar-vin: move subdevice handling to async callbacks

2018-03-09 Thread Hans Verkuil
On 07/03/18 23:04, 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 
> Reviewed-by: Laurent Pinchart 
> ---
>  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 */

shuld -> should

Actually, I'd say 'shall' instead of 'should'.

After that trivial change you can add my:

Reviewed-by: Hans Verkuil 

Regards,

Hans


[PATCH v12 06/33] rcar-vin: move subdevice handling to async callbacks

2018-03-07 Thread Niklas Söderlund
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 
Reviewed-by: Laurent Pinchart 
---
 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.pad = vin->digital->source_pad;
+   while (!vin->digital->code &&
+  !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, )) {
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, >vdev.tvnorms);
+   if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
+   return ret;
+
+   /* Add the controls */
+   ret = v4l2_ctrl_handler_init(>ctrl_handler, 16);
+   if (ret < 0)
+   return ret;
+
+   ret = v4l2_ctrl_add_handler(>ctrl_handler, subdev->ctrl_handler,
+   NULL);
+   if (ret < 0) {
+   v4l2_ctrl_handler_free(>ctrl_handler);
+   return ret;
+   }
+
+   vin->vdev.ctrl_handler = >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(>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(>v4l2_dev);
if (ret < 0) {
@@ -103,8 +145,10 @@ static void rvin_digital_notify_unbind(struct 
v4l2_async_notifier *notifier,
struct rvin_dev