Re: [PATCH v7 10/13] media: ti: Add CSI2RX support for J721E

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


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.


[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