Re: [PATCH 05/19] media: adv748x: Implement set_routing()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
> > ---
> >  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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux