| Message ID | 20240430103956.60190-6-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/19] media: adv748x: Add support for active state | expand |
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
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. */ + 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, }; /* -----------------------------------------------------------------------------
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(-)