Hi Stefan,
Thank you for the patch.
On Thu, Mar 14, 2024 at 10:36:50AM +0100, Stefan Klug wrote:
> The Samsung CSIS MIPI receiver provides a start-of-frame interrupt and
> a framecount register. As the CSI receiver is the hardware unit that lies
> closest to the sensor, the frame counter is the best we can get on these
> devices. In case of the ISI available on the i.MX8 M Plus it is also the
> only native start-of-frame signal available.
You still have either an extra line break or a missing blank line :-)
> This patch exposes the sof interrupt and the framecount as
> V4L2_EVENT_FRAME_SYNC event on the subdevice.
>
> It was tested on a Debix-Som-A with a 6.8-rc4 kernel.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Reviewed-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> Signed-off-by: Stefan Klug <stefan.klug@xxxxxxxxxxxxxxxx>
> ---
> Changes v1 -> v2:
> - fixed formatting issues from review
> - moved frame variable declaration to top of subscribe_event()
>
> Thanks all for the review!
>
> drivers/media/platform/nxp/imx-mipi-csis.c | 34 +++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index db8ff5f5c4d3..664be27c4224 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -30,6 +30,7 @@
>
> #include <media/v4l2-common.h>
> #include <media/v4l2-device.h>
> +#include <media/v4l2-event.h>
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-mc.h>
> #include <media/v4l2-subdev.h>
> @@ -742,6 +743,18 @@ static void mipi_csis_stop_stream(struct mipi_csis_device *csis)
> mipi_csis_system_enable(csis, false);
> }
>
> +static void mipi_csis_queue_event_sof(struct mipi_csis_device *csis)
> +{
> + u32 frame;
> + struct v4l2_event event = {
> + .type = V4L2_EVENT_FRAME_SYNC,
> + };
Nitpicking, we usually sort declarations by decreasing length. No need
for a v3.
> +
> + frame = mipi_csis_read(csis, MIPI_CSIS_FRAME_COUNTER_CH(0));
> + event.u.frame_sync.frame_sequence = frame;
> + v4l2_event_queue(csis->sd.devnode, &event);
> +}
> +
> static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> {
> struct mipi_csis_device *csis = dev_id;
> @@ -765,6 +778,10 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> event->counter++;
> }
> }
> +
> + if (status & MIPI_CSIS_INT_SRC_FRAME_START)
> + mipi_csis_queue_event_sof(csis);
> +
> spin_unlock_irqrestore(&csis->slock, flags);
>
> mipi_csis_write(csis, MIPI_CSIS_INT_SRC, status);
> @@ -1154,8 +1171,23 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
> return 0;
> }
>
> +static int mipi_csis_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + if (sub->type != V4L2_EVENT_FRAME_SYNC)
> + return -EINVAL;
> +
> + /* V4L2_EVENT_FRAME_SYNC doesn't require an id, so zero should be set */
> + if (sub->id != 0)
> + return -EINVAL;
> +
> + return v4l2_event_subscribe(fh, sub, 0, NULL);
> +}
> +
> static const struct v4l2_subdev_core_ops mipi_csis_core_ops = {
> .log_status = mipi_csis_log_status,
> + .subscribe_event = mipi_csis_subscribe_event,
> + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> };
>
> static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
> @@ -1358,7 +1390,7 @@ static int mipi_csis_subdev_init(struct mipi_csis_device *csis)
> snprintf(sd->name, sizeof(sd->name), "csis-%s",
> dev_name(csis->dev));
>
> - sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> sd->ctrl_handler = NULL;
>
> sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
--
Regards,
Laurent Pinchart
