Re: [PATCH 17/55] media: rkisp1: Fix sensor source pad retrieval at bound time

[Dafna Hirschfeld <dafna@fastmail.com>]

On 15.06.2022 04:10, Paul Elder wrote:
>From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>When a sensor is bound, its source pad is retrieved in the .bound()
>operation with a call to media_entity_get_fwnode_pad(). The function
>should be called with the source endpoint fwnode of the sensor, but is
>instead called with the sensor's device fwnode.
>
>Fix this, which involves storing a reference to the source endpoint
>fwnode in the rkisp1_sensor_async structure, and thus implementing the
>subdev notifier .destroy() operation to release the reference.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>---
> .../platform/rockchip/rkisp1/rkisp1-common.h  |  2 ++
> .../platform/rockchip/rkisp1/rkisp1-dev.c     | 28 ++++++++++++++++---
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>index ba11baf75fa9..60c5462e1746 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
>@@ -117,6 +117,7 @@ struct rkisp1_info {
>  *
>  * @asd:		async_subdev variable for the sensor
>  * @index:		index of the sensor (counting sensor found in DT)
>+ * @source_ep:		fwnode for the sensor source endpoint
>  * @lanes:		number of lanes
>  * @mbus_type:		type of bus (currently only CSI2 is supported)
>  * @mbus_flags:		media bus (V4L2_MBUS_*) flags
>@@ -127,6 +128,7 @@ struct rkisp1_info {
> struct rkisp1_sensor_async {
> 	struct v4l2_async_subdev asd;
> 	unsigned int index;
>+	struct fwnode_handle *source_ep;
> 	unsigned int lanes;
> 	enum v4l2_mbus_type mbus_type;
> 	unsigned int mbus_flags;
>diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>index 386c1c17aec2..0f3e45cdbf2a 100644
>--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>@@ -138,7 +138,7 @@ static int rkisp1_subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> 	phy_init(s_asd->dphy);
>
> 	/* Create the link to the sensor. */
>-	source_pad = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode,
>+	source_pad = media_entity_get_fwnode_pad(&sd->entity, s_asd->source_ep,
> 						 MEDIA_PAD_FL_SOURCE);
> 	if (source_pad < 0) {
> 		dev_err(rkisp1->dev, "failed to find source pad for %s\n",
>@@ -170,10 +170,19 @@ static int rkisp1_subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> 	return v4l2_device_register_subdev_nodes(&rkisp1->v4l2_dev);
> }
>
>+static void rkisp1_subdev_notifier_destroy(struct v4l2_async_subdev *asd)
>+{
>+	struct rkisp1_sensor_async *rk_asd =
>+		container_of(asd, struct rkisp1_sensor_async, asd);
>+
>+	fwnode_handle_put(rk_asd->source_ep);
>+}
>+
> static const struct v4l2_async_notifier_operations rkisp1_subdev_notifier_ops = {
> 	.bound = rkisp1_subdev_notifier_bound,
> 	.unbind = rkisp1_subdev_notifier_unbind,
> 	.complete = rkisp1_subdev_notifier_complete,
>+	.destroy = rkisp1_subdev_notifier_destroy,
> };
>
> static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
>@@ -190,6 +199,7 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
> 			.bus_type = V4L2_MBUS_CSI2_DPHY
> 		};
> 		struct rkisp1_sensor_async *rk_asd;
>+		struct fwnode_handle *source = NULL;
> 		struct fwnode_handle *ep;
>
> 		ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(rkisp1->dev),
>@@ -202,15 +212,24 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
> 		if (ret)
> 			goto err_parse;
>
>-		rk_asd = v4l2_async_nf_add_fwnode_remote(ntf, ep,
>-							 struct
>-							 rkisp1_sensor_async);
>+		source = fwnode_graph_get_remote_endpoint(ep);
>+		if (!source) {
>+			dev_err(rkisp1->dev,
>+				"endpoint %pfw has no remote endpoint\n",
>+				ep);
>+			ret = -ENODEV;
>+			goto err_parse;
>+		}
>+
>+		rk_asd = v4l2_async_nf_add_fwnode(ntf, source,
>+						  struct rkisp1_sensor_async);
> 		if (IS_ERR(rk_asd)) {
> 			ret = PTR_ERR(rk_asd);
> 			goto err_parse;
> 		}
>
> 		rk_asd->index = index++;
>+		rk_asd->source_ep = source;

here do 'source = NULL', see reason below

> 		rk_asd->mbus_type = vep.bus_type;
> 		rk_asd->mbus_flags = vep.bus.mipi_csi2.flags;
> 		rk_asd->lanes = vep.bus.mipi_csi2.num_data_lanes;
>@@ -225,6 +244,7 @@ static int rkisp1_subdev_notifier_register(struct rkisp1_device *rkisp1)
> 		continue;
> err_parse:
> 		fwnode_handle_put(ep);
>+		fwnode_handle_put(source);

if v4l2_fwnode_endpoint_parse fails then here you put the source of previous iteration

thanks,
Dafna

> 		v4l2_async_nf_cleanup(ntf);
> 		return ret;
> 	}
>-- 
>2.30.2
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip