On 24/03/2023 20:14, Laurent Pinchart wrote:
+static int ti_csi2rx_link_validate_get_fmt(struct media_pad *pad,
+ struct v4l2_subdev_format *fmt)
+{
+ if (is_media_entity_v4l2_subdev(pad->entity)) {
+ struct v4l2_subdev *sd =
+ media_entity_to_v4l2_subdev(pad->entity);
+
+ fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ fmt->pad = pad->index;
+ return v4l2_subdev_call(sd, pad, get_fmt, NULL, fmt);
This will crash with any source that uses the subdev active state. You
can't pass NULL for the state here.
How about exporting v4l2_subdev_link_validate_get_format() instead, to
replace this function ?
I don't think that's a good idea, or needed.
v4l2_subdev_link_validate_get_format() is an internal helper. Exporting
it would potentially make maintenance more difficult in the future.
Here, you know the source is the cdns csi2rx. It's always a subdev, so
is_media_entity_v4l2_subdev() is not needed (I'm not sure why it's used
in v4l2_subdev_link_validate_get_format either...).
You can use v4l2_subdev_call_state_active() to call the get_fmt here.
Also, the fmt doesn't seem to be initialized to zero, so it'll contain
garbage in, e.g. the stream field.
Moving to another topic, cdns-csi2rx driver is already (before this
series) in upstream, but afaics j7 is the only user for it. In other
words, it's not in use before this series.
I think it would make sense to convert cdns-csi2rx to use subdev state
first, before adding the j7 csi2rx. That shouldn't be much work, and
will clean up the driver nicely. With that you could also simplify the
ti_csi2rx_link_validate by just locking the cdns csi2rx state, and using
it directly, without using get_fmt and getting the copy of the format.
It would also help later when adding streams support.
Personally I'd go straight to streams support, as adding it afterwards
might have some pain points (based on my CAL experience...). At least
WIP streams patches on top would be a very good thing to have, as they
will highlight if there are any major changes needed.
Tomi
