Hi Laurent,
On 15/06/2020 00:59, Laurent Pinchart wrote:
> Create and register V4L2 sudbevs for the CAMERARX instances, and link
s/sudbevs/subdevs/
> them in the media graph to the sensors and video nodes. The subdev API
> is not exposed to userspace at this point, and no subdev operation is
> implemented, but the media controller graph is visible to applications.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/ti-vpe/cal-camerarx.c | 42 +++++++++++++++++++-
> drivers/media/platform/ti-vpe/cal-video.c | 12 ++++++
> drivers/media/platform/ti-vpe/cal.c | 31 ++++++++++++---
> drivers/media/platform/ti-vpe/cal.h | 9 ++++-
> 4 files changed, 86 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c
> index a7e4b81c9734..9be432ff87b2 100644
> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c
> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c
> @@ -526,8 +526,8 @@ static int cal_camerarx_regmap_init(struct cal_dev *cal,
> static int cal_camerarx_parse_dt(struct cal_camerarx *phy)
> {
> struct v4l2_fwnode_endpoint *endpoint = &phy->endpoint;
> - struct device_node *ep_node;
> char data_lanes[V4L2_FWNODE_CSI2_MAX_DATA_LANES * 2];
> + struct device_node *ep_node;
> unsigned int i;
> int ret;
>
> @@ -571,7 +571,8 @@ static int cal_camerarx_parse_dt(struct cal_camerarx *phy)
> endpoint->bus.mipi_csi2.flags);
>
> /* Retrieve the connected device and store it for later use. */
> - phy->sensor_node = of_graph_get_remote_port_parent(ep_node);
> + phy->sensor_ep_node = of_graph_get_remote_endpoint(ep_node);
> + phy->sensor_node = of_graph_get_port_parent(phy->sensor_ep_node);
> if (!phy->sensor_node) {
> phy_dbg(3, phy, "Can't get remote parent\n");
> goto done;
> @@ -580,15 +581,30 @@ static int cal_camerarx_parse_dt(struct cal_camerarx *phy)
> phy_dbg(1, phy, "Found connected device %pOFn\n", phy->sensor_node);
>
> done:
> + of_node_put(phy->sensor_ep_node);
> of_node_put(ep_node);
> return ret;
> }
>
> +/* ------------------------------------------------------------------
> + * V4L2 Subdev Operations
> + * ------------------------------------------------------------------
> + */
> +
> +static const struct v4l2_subdev_ops cal_camerarx_subdev_ops = {
> +};
> +
> +/* ------------------------------------------------------------------
> + * Create and Destroy
> + * ------------------------------------------------------------------
> + */
> +
> struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
> unsigned int instance)
> {
> struct platform_device *pdev = to_platform_device(cal->dev);
> struct cal_camerarx *phy;
> + struct v4l2_subdev *sd;
> int ret;
>
> phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> @@ -620,9 +636,28 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
> if (ret)
> goto error;
>
> + /* Initialize the V4L2 subdev and media entity. */
> + sd = &phy->subdev;
> + v4l2_subdev_init(sd, &cal_camerarx_subdev_ops);
> + sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> + snprintf(sd->name, sizeof(sd->name), "CAMERARX%u", instance);
> + sd->dev = cal->dev;
> +
> + phy->pads[CAL_CAMERARX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + phy->pads[CAL_CAMERARX_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(phy->pads),
> + phy->pads);
> + if (ret)
> + goto error;
> +
> + ret = v4l2_device_register_subdev(&cal->v4l2_dev, sd);
> + if (ret)
> + goto error;
> +
> return phy;
>
> error:
> + media_entity_cleanup(&phy->subdev.entity);
> kfree(phy);
> return ERR_PTR(ret);
> }
> @@ -632,6 +667,9 @@ void cal_camerarx_destroy(struct cal_camerarx *phy)
> if (!phy)
> return;
>
> + v4l2_device_unregister_subdev(&phy->subdev);
> + media_entity_cleanup(&phy->subdev.entity);
> + of_node_put(phy->sensor_ep_node);
> of_node_put(phy->sensor_node);
> kfree(phy);
> }
> diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c
> index df472a175e83..0a1a11692208 100644
> --- a/drivers/media/platform/ti-vpe/cal-video.c
> +++ b/drivers/media/platform/ti-vpe/cal-video.c
> @@ -809,6 +809,18 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx)
> return ret;
> }
>
> + ret = media_create_pad_link(&ctx->phy->subdev.entity,
> + CAL_CAMERARX_PAD_SOURCE,
> + &vfd->entity, 0,
> + MEDIA_LNK_FL_IMMUTABLE |
> + MEDIA_LNK_FL_ENABLED);
> + if (ret) {
> + ctx_err(ctx, "Failed to create media link for context %u\n",
> + ctx->index);
> + video_unregister_device(vfd);
> + return ret;
> + }
> +
> ctx_info(ctx, "V4L2 device registered as %s\n",
> video_device_node_name(vfd));
>
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index ca8576aa2646..bf1734d4d800 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -416,6 +416,8 @@ static int cal_async_notifier_bound(struct v4l2_async_notifier *notifier,
> struct v4l2_async_subdev *asd)
> {
> struct cal_camerarx *phy = to_cal_asd(asd)->phy;
> + int pad;
> + int ret;
>
> if (phy->sensor) {
> phy_info(phy, "Rejecting subdev %s (Already set!!)",
> @@ -426,6 +428,25 @@ static int cal_async_notifier_bound(struct v4l2_async_notifier *notifier,
> phy->sensor = subdev;
> phy_dbg(1, phy, "Using sensor %s for capture\n", subdev->name);
>
> + pad = media_entity_get_fwnode_pad(&subdev->entity,
> + of_fwnode_handle(phy->sensor_ep_node),
> + MEDIA_PAD_FL_SOURCE);
> + if (pad < 0) {
> + phy_err(phy, "Sensor %s has no connected source pad\n",
> + subdev->name);
> + return pad;
> + }
> +
> + ret = media_create_pad_link(&subdev->entity, pad,
> + &phy->subdev.entity, CAL_CAMERARX_PAD_SINK,
> + MEDIA_LNK_FL_IMMUTABLE |
> + MEDIA_LNK_FL_ENABLED);
> + if (ret) {
> + phy_err(phy, "Failed to create media link for sensor %s\n",
> + subdev->name);
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -796,6 +817,11 @@ static int cal_probe(struct platform_device *pdev)
> cal_get_hwinfo(cal);
> pm_runtime_put_sync(&pdev->dev);
>
> + /* Initialize the media device. */
> + ret = cal_media_init(cal);
> + if (ret < 0)
> + goto error_camerarx;
This code moves now uses the wrong error label.
Moreover, moving it - now means that the loop creating the CAMERARX PHYs
should now also cleanup the media device upon failure.
I have the following fixup patch which you could squash in here if you wish.
>From e6fc5364d92d0ded26f3d8bb6c06a650fcb1ba84 Mon Sep 17 00:00:00 2001
From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
Date: Tue, 16 Jun 2020 15:00:19 +0100
Subject: [PATCH] media: ti-vpe: cal: Fix error path jumps
The error paths in cal_probe() incorrectly called through the
error_camerarx before the camerarx instances were created, and neglected
to call the error_media/cal_media_cleanup() paths if a camera failed to
be created.
Tidy up the error paths, removing the now redundant error_media label.
Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
---
drivers/media/platform/ti-vpe/cal.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/cal.c
b/drivers/media/platform/ti-vpe/cal.c
index caea3e129c87..d00dc241804b 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1026,14 +1026,14 @@ static int cal_probe(struct platform_device *pdev)
/* Initialize the media device. */
ret = cal_media_init(cal);
if (ret < 0)
- goto error_camerarx;
+ goto error_pm_runtime;
/* Create CAMERARX PHYs. */
for (i = 0; i < cal_data_get_num_csi2_phy(cal); ++i) {
cal->phy[i] = cal_camerarx_create(cal, i);
if (IS_ERR(cal->phy[i])) {
ret = PTR_ERR(cal->phy[i]);
- goto error_camerarx;
+ goto error_media;
}
}
@@ -1063,8 +1063,6 @@ static int cal_probe(struct platform_device *pdev)
error_media:
cal_media_cleanup(cal);
-
-error_camerarx:
for (i = 0; i < cal->data->num_csi2_phy; i++)
cal_camerarx_destroy(cal->phy[i]);
--
2.25.1
With that fixed,
Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> +
> /* Create CAMERARX PHYs. */
> for (i = 0; i < cal_data_get_num_csi2_phy(cal); ++i) {
> cal->phy[i] = cal_camerarx_create(cal, i);
> @@ -805,11 +831,6 @@ static int cal_probe(struct platform_device *pdev)
> }
> }
>
> - /* Initialize the media device. */
> - ret = cal_media_init(cal);
> - if (ret < 0)
> - goto error_camerarx;
> -
> /* Create contexts. */
> for (i = 0; i < cal_data_get_num_csi2_phy(cal); ++i)
> cal->ctx[i] = cal_ctx_create(cal, i);
> diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h
> index cb167bfc2773..bf31dbf24523 100644
> --- a/drivers/media/platform/ti-vpe/cal.h
> +++ b/drivers/media/platform/ti-vpe/cal.h
> @@ -24,6 +24,7 @@
> #include <media/v4l2-dev.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> #include <media/videobuf2-v4l2.h>
>
> #define CAL_MODULE_NAME "cal"
> @@ -33,12 +34,14 @@
> #define MAX_WIDTH_BYTES (8192 * 8)
> #define MAX_HEIGHT_LINES 16383
>
> +#define CAL_CAMERARX_PAD_SINK 0
> +#define CAL_CAMERARX_PAD_SOURCE 1
> +
> struct device;
> struct device_node;
> struct resource;
> struct regmap;
> struct regmap_fied;
> -struct v4l2_subdev;
>
> /* CTRL_CORE_CAMERRX_CONTROL register field id */
> enum cal_camerarx_field {
> @@ -108,8 +111,12 @@ struct cal_camerarx {
> unsigned int instance;
>
> struct v4l2_fwnode_endpoint endpoint;
> + struct device_node *sensor_ep_node;
> struct device_node *sensor_node;
> struct v4l2_subdev *sensor;
> +
> + struct v4l2_subdev subdev;
> + struct media_pad pads[2];
> };
>
> struct cal_dev {
>
--
Regards
--
Kieran
