Hi Laurent
On Thu, May 02, 2024 at 08:40:51PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Apr 30, 2024 at 12:39:39PM +0200, Jacopo Mondi wrote:
> > Initialize the CSI-2 subdevice with the V4L2_SUBDEV_FL_STREAMS flag
> > and initialize a simple routing table by implementing the .init_state()
> > operation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/adv748x/adv748x-csi2.c | 28 ++++++++++++++++++++++--
> > 1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 60bf1dc0f58b..d929db7e8ef2 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -59,7 +59,30 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> >
> > /* -----------------------------------------------------------------------------
> > * v4l2_subdev_internal_ops
> > - *
> > + */
> > +
> > +static int adv748x_csi2_init_state(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *state)
> > +{
> > + struct v4l2_subdev_route routes[] = {
> > + {
> > + .sink_pad = ADV748X_CSI2_SINK,
> > + .sink_stream = 0,
> > + .source_pad = ADV748X_CSI2_SOURCE,
> > + .source_stream = 0,
> > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > + },
> > + };
> > +
> > + struct v4l2_subdev_krouting routing = {
> > + .num_routes = ARRAY_SIZE(routes),
> > + .routes = routes,
> > + };
> > +
> > + return v4l2_subdev_set_routing(sd, state, &routing);
>
> You need to initialize formats too.
>
The adv748x driver handles formats very poorly, doesn't implement
enum_mbus_codes and does not allow userspace to change the format
(while at the same time it doesn't check that the format is the
expected one in set_format()).
This is from a freshly booted renesas-drivers/main
- entity 30: adv748x 4-0070 txa (2 pads, 3 links, 0 routes)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev5
pad0: Sink
[stream:0 fmt:unknown/0x0]
<- "adv748x 4-0070 afe":8 []
<- "adv748x 4-0070 hdmi":1 [ENABLED]
pad1: Source
[stream:0 fmt:unknown/0x0]
-> "rcar_csi2 feaa0000.csi2":0 [ENABLED,IMMUTABLE]
It would probably be better to handle the formats properly and the
introduce streams or use the introduction of streams to also fix the
format handling ?
> > +}
> > +
> > +/*
> > * We use the internal registered operation to be able to ensure that our
> > * incremental subdevices (not connected in the forward path) can be registered
> > * against the resulting video path and media device.
> > @@ -109,6 +132,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> > }
> >
> > static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> > + .init_state = adv748x_csi2_init_state,
>
> The .init_state() operation needs to be provided along with the call to
> v4l2_subdev_init_finalize() in patch 01/19.
>
I'll squash, however even if it might be a requirement for having a
fully working implementation, not having init_state() will not lead to
any crash and maybe smaller incremental patches are easier to handle.
if (sd->internal_ops && sd->internal_ops->init_state) {
/*
* There can be no race at this point, but we lock the state
* anyway to satisfy lockdep checks.
*/
v4l2_subdev_lock_state(state);
ret = sd->internal_ops->init_state(sd, state);
v4l2_subdev_unlock_state(state);
> > .registered = adv748x_csi2_registered,
> > };
> >
> > @@ -245,7 +269,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
> > return 0;
> >
> > adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops,
> > - MEDIA_ENT_F_VID_IF_BRIDGE, 0,
> > + MEDIA_ENT_F_VID_IF_BRIDGE, V4L2_SUBDEV_FL_STREAMS,
> > is_txa(tx) ? "txa" : "txb");
> >
> > /* Register internal ops for incremental subdev registration */
> > --
> > 2.44.0
> >
>
> --
> Regards,
>
> Laurent Pinchart
