[05/19] media: adv748x: Implement set_routing()
Commit Message
Implement the set_routing() pad operation to control the MIPI CSI-2
Virtual Channel on which the video stream is sent on according to
the active route source stream number.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
drivers/media/i2c/adv748x/adv748x-csi2.c | 43 +++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
Comments
Hi Jacopo,
Thank you for the patch.
On Tue, Apr 30, 2024 at 12:39:41PM +0200, Jacopo Mondi wrote:
> Implement the set_routing() pad operation to control the MIPI CSI-2
> Virtual Channel on which the video stream is sent on according to
> the active route source stream number.
While 01/19 needs to implement .init_state(), you should only initialize
formats there. The routing initialization of 03/19 should be moved to
this patch.
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> drivers/media/i2c/adv748x/adv748x-csi2.c | 43 +++++++++++++++++++++++-
> 1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index ace4e1d904d9..7fa72340e66e 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -57,6 +57,38 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> return 0;
> }
>
> +static int adv748x_csi2_apply_routing(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + struct v4l2_subdev_krouting *routing)
> +{
> + struct v4l2_subdev_route *route;
> + int ret;
> +
> + /* Only one route at the time can be active. */
s/the time/a time/
> + if (routing->num_routes > 1)
> + return -EINVAL;
You should adjust routes instead of returning -EINVAL.
> +
> + /*
> + * Validate the route: sink pad and sink stream shall be 0 and only
> + * 4 source streams are supported (one for each supported MIPI CSI-2
> + * channel).
s/channel/virtual channel/
> + */
> + route = &routing->routes[0];
> +
> + if (route->sink_pad != ADV748X_CSI2_SINK || route->sink_stream)
> + return -EINVAL;
> + if (route->source_pad != ADV748X_CSI2_SOURCE ||
> + route->source_stream > 4)
> + return -EINVAL;
Adjust instead of returning an error. The pad checks can be dropped, as
the core ensures sink_pad and source_pad reference a valid sink and
source pad respectively.
I'm not sure the source stream check is right either. I understand
you'll use that to select a virtual channel, but the routing API isn't
meant to let userspace configure virtual channel numbers explicitly.
> +
> + ret = v4l2_subdev_routing_validate(sd, routing,
> + V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> + if (ret)
> + return ret;
> +
> + return v4l2_subdev_set_routing(sd, state, routing);
> +}
> +
> /* -----------------------------------------------------------------------------
> * v4l2_subdev_internal_ops
> */
> @@ -79,7 +111,7 @@ static int adv748x_csi2_init_state(struct v4l2_subdev *sd,
> .routes = routes,
> };
>
> - return v4l2_subdev_set_routing(sd, state, &routing);
> + return adv748x_csi2_apply_routing(sd, state, &routing);
> }
>
> /*
> @@ -200,10 +232,19 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
> return 0;
> }
>
> +static int adv748x_csi2_set_routing(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + enum v4l2_subdev_format_whence which,
> + struct v4l2_subdev_krouting *routing)
> +{
> + return adv748x_csi2_apply_routing(sd, state, routing);
> +}
> +
> static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = adv748x_csi2_set_format,
> .get_mbus_config = adv748x_csi2_get_mbus_config,
> + .set_routing = adv748x_csi2_set_routing,
> };
>
> /* -----------------------------------------------------------------------------
Hi Laurent
On Thu, May 02, 2024 at 08:49:59PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Apr 30, 2024 at 12:39:41PM +0200, Jacopo Mondi wrote:
> > Implement the set_routing() pad operation to control the MIPI CSI-2
> > Virtual Channel on which the video stream is sent on according to
> > the active route source stream number.
>
> While 01/19 needs to implement .init_state(), you should only initialize
> formats there. The routing initialization of 03/19 should be moved to
> this patch.
>
ok, even if I'm not sure I understand why routing initialization and
support for writing the routing table have to come together
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > drivers/media/i2c/adv748x/adv748x-csi2.c | 43 +++++++++++++++++++++++-
> > 1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index ace4e1d904d9..7fa72340e66e 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -57,6 +57,38 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> > return 0;
> > }
> >
> > +static int adv748x_csi2_apply_routing(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state,
> > + struct v4l2_subdev_krouting *routing)
> > +{
> > + struct v4l2_subdev_route *route;
> > + int ret;
> > +
> > + /* Only one route at the time can be active. */
>
> s/the time/a time/
>
> > + if (routing->num_routes > 1)
> > + return -EINVAL;
>
> You should adjust routes instead of returning -EINVAL.
>
> > +
> > + /*
> > + * Validate the route: sink pad and sink stream shall be 0 and only
> > + * 4 source streams are supported (one for each supported MIPI CSI-2
> > + * channel).
>
> s/channel/virtual channel/
>
> > + */
> > + route = &routing->routes[0];
> > +
> > + if (route->sink_pad != ADV748X_CSI2_SINK || route->sink_stream)
> > + return -EINVAL;
> > + if (route->source_pad != ADV748X_CSI2_SOURCE ||
> > + route->source_stream > 4)
> > + return -EINVAL;
>
> Adjust instead of returning an error. The pad checks can be dropped, as
> the core ensures sink_pad and source_pad reference a valid sink and
> source pad respectively.
>
ack
> I'm not sure the source stream check is right either. I understand
> you'll use that to select a virtual channel, but the routing API isn't
> meant to let userspace configure virtual channel numbers explicitly.
>
Ok, this surprises me, more on the next patch
> > +
> > + ret = v4l2_subdev_routing_validate(sd, routing,
> > + V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> > + if (ret)
> > + return ret;
> > +
> > + return v4l2_subdev_set_routing(sd, state, routing);
> > +}
> > +
> > /* -----------------------------------------------------------------------------
> > * v4l2_subdev_internal_ops
> > */
> > @@ -79,7 +111,7 @@ static int adv748x_csi2_init_state(struct v4l2_subdev *sd,
> > .routes = routes,
> > };
> >
> > - return v4l2_subdev_set_routing(sd, state, &routing);
> > + return adv748x_csi2_apply_routing(sd, state, &routing);
> > }
> >
> > /*
> > @@ -200,10 +232,19 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
> > return 0;
> > }
> >
> > +static int adv748x_csi2_set_routing(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state,
> > + enum v4l2_subdev_format_whence which,
> > + struct v4l2_subdev_krouting *routing)
> > +{
> > + return adv748x_csi2_apply_routing(sd, state, routing);
> > +}
> > +
> > static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> > .get_fmt = v4l2_subdev_get_fmt,
> > .set_fmt = adv748x_csi2_set_format,
> > .get_mbus_config = adv748x_csi2_get_mbus_config,
> > + .set_routing = adv748x_csi2_set_routing,
> > };
> >
> > /* -----------------------------------------------------------------------------
>
> --
> Regards,
>
> Laurent Pinchart
@@ -57,6 +57,38 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
return 0;
}
+static int adv748x_csi2_apply_routing(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ struct v4l2_subdev_krouting *routing)
+{
+ struct v4l2_subdev_route *route;
+ int ret;
+
+ /* Only one route at the time can be active. */
+ if (routing->num_routes > 1)
+ return -EINVAL;
+
+ /*
+ * Validate the route: sink pad and sink stream shall be 0 and only
+ * 4 source streams are supported (one for each supported MIPI CSI-2
+ * channel).
+ */
+ route = &routing->routes[0];
+
+ if (route->sink_pad != ADV748X_CSI2_SINK || route->sink_stream)
+ return -EINVAL;
+ if (route->source_pad != ADV748X_CSI2_SOURCE ||
+ route->source_stream > 4)
+ return -EINVAL;
+
+ ret = v4l2_subdev_routing_validate(sd, routing,
+ V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
+ if (ret)
+ return ret;
+
+ return v4l2_subdev_set_routing(sd, state, routing);
+}
+
/* -----------------------------------------------------------------------------
* v4l2_subdev_internal_ops
*/
@@ -79,7 +111,7 @@ static int adv748x_csi2_init_state(struct v4l2_subdev *sd,
.routes = routes,
};
- return v4l2_subdev_set_routing(sd, state, &routing);
+ return adv748x_csi2_apply_routing(sd, state, &routing);
}
/*
@@ -200,10 +232,19 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
return 0;
}
+static int adv748x_csi2_set_routing(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state,
+ enum v4l2_subdev_format_whence which,
+ struct v4l2_subdev_krouting *routing)
+{
+ return adv748x_csi2_apply_routing(sd, state, routing);
+}
+
static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
.get_fmt = v4l2_subdev_get_fmt,
.set_fmt = adv748x_csi2_set_format,
.get_mbus_config = adv748x_csi2_get_mbus_config,
+ .set_routing = adv748x_csi2_set_routing,
};
/* -----------------------------------------------------------------------------