Received-SPF: pass (google.com: domain of linux-kernel+bounces-102519-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 13 Mar 2024 23:10:10 +0200
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Cc: Stefan Klug <stefan.klug@ideasonboard.com>,
	libcamera-devel@lists.libcamera.org, linux-media@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	Rui Miguel Silva <rmfrfs@gmail.com>,
	Martin Kepplinger <martink@posteo.de>,
	Purism Kernel Team <kernel@puri.sm>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>
Subject: Re: [PATCH] mipi-csis: Emit V4L2_EVENT_FRAME_SYNC events
Message-ID: <20240313195408.GE8399@pendragon.ideasonboard.com>
References: <20240313153058.189684-1-stefan.klug@ideasonboard.com>
 <axrx5ynlyoyjgkhj3j4r5b6xfaxwetx4c7u5ngq5wunin5ynv6@qmvav4y3xczq>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <axrx5ynlyoyjgkhj3j4r5b6xfaxwetx4c7u5ngq5wunin5ynv6@qmvav4y3xczq>

On Wed, Mar 13, 2024 at 05:49:12PM +0100, Jacopo Mondi wrote:
> Hi Stefan
> 
> In Subject: missing the 'media:' prefix
> 
> On Wed, Mar 13, 2024 at 04:30:58PM +0100, Stefan Klug wrote:
> > The Samsung CSIS Mipi receiver provides a start-of-frame interrupt and
> 
> s/Mipi/MIPI/
> 
> > 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.

Nitpicking: if you intended to start a new paragraph, add a blank line.
If not, don't add a line break between sentences.

> > In case of the ISI available on the i.MX8 M Plus it is also the only
> > native start-of-frame signal available.
> >
> > 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.
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  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..caeb1622f741 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)
> > +{
> > +	struct v4l2_event event = {
> > +		.type = V4L2_EVENT_FRAME_SYNC,
> > +	};
> > +

No need for a blank line.

> > +	u32 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)
> 
> Please align to open ( on the previous line
> 
> All minors, with the above fixed
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If you don't want to send a v2, I can address the minor comments when
applying this to my tree. Please let me know what you prefer.

> > +{
> > +	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