[PATCH v2 2/2] media: platform: Add user line interrupt to imx-mipi-csis driver
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 7 11:45:22 PDT 2025
Hi Isaac,
Thank you for the patch.
As for 1/2, The subject line should start with "media: imx-mipi-csis: ".
On Fri, Jun 06, 2025 at 04:44:14PM +0100, Isaac Scott wrote:
> The NXP i.MX 8M Plus features an interrupt that triggers after the MIPI
> CSI receiver counts a user-configurable number of lines. This is useful
> for debugging, as it allows users to check if the amount of lines per
> frame equals what they are expecting.
>
> Add support for this interrupt in the driver, and an entry into debugfs
> to allow the user to configure whether the interrupt is enabled, as well
> as the number of lines after which to trigger the interrupt.
>
> This debugfs control can be altered while a stream is in progress, with
> 0 disabling the interrupt and >0 setting a new desired line count.
>
> Signed-off-by: Isaac Scott <isaac.scott at ideasonboard.com>
>
> ---
>
> Changes since v1:
> - Moved from magic number to enum in status_index
> - Clear INT_MSK_1 in enable_interrupts() when on == false
You also modified the logic to enable the interrupt there. I don't think
that's a good idea, as it means the interrupt will always be enabled,
even when the feature isn't used. It can increase the load on the
system.
> - use local variable in set_params() as in the interrupt handler
> - move interrupt handling code outside of spinlock
> ---
> drivers/media/platform/nxp/imx-mipi-csis.c | 40 +++++++++++++++++++++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 394987d72c64..1b71f6c19fa8 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -88,6 +88,10 @@
> #define MIPI_CSIS_INT_MSK_ERR_CRC BIT(1)
> #define MIPI_CSIS_INT_MSK_ERR_UNKNOWN BIT(0)
>
> +/* CSIS Interrupt mask 1 */
> +#define MIPI_CSIS_INT_MSK_1 0x18
> +#define MIPI_CSIS_INT_MSK_1_LINE_END BIT(0)
> +
> /* CSIS Interrupt source */
> #define MIPI_CSIS_INT_SRC 0x14
> #define MIPI_CSIS_INT_SRC_EVEN_BEFORE BIT(31)
> @@ -109,6 +113,10 @@
> #define MIPI_CSIS_INT_SRC_ERR_UNKNOWN BIT(0)
> #define MIPI_CSIS_INT_SRC_ERRORS 0xfffff
>
> +/* CSIS Interrupt source 1 */
> +#define MIPI_CSIS_INT_SRC_1 0x1c
> +#define MIPI_CSIS_INT_SRC_1_LINE_END BIT(0)
> +
> /* D-PHY status control */
> #define MIPI_CSIS_DPHY_STATUS 0x20
> #define MIPI_CSIS_DPHY_STATUS_ULPS_DAT BIT(8)
> @@ -221,6 +229,7 @@
> #define MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE BIT(0)
>
> #define MIPI_CSIS_FRAME_COUNTER_CH(n) (0x0100 + (n) * 4)
> +#define MIPI_CSIS_LINE_INTERRUPT_RATIO(n) (0x0110 + (n) * 4)
>
> /* Non-image packet data buffers */
> #define MIPI_CSIS_PKTDATA_ODD 0x2000
> @@ -251,6 +260,7 @@
> enum mipi_csis_event_type {
> MAIN = 0,
> DEBUG = 1,
> + USER = 2,
I don't think "USER" really fits. The MIPI_CSI1_INTERRUPT_SOURCE_1
register is just the second interrupt source register. MAIN0 and MAIN1
may be a better fit.
> };
>
> struct mipi_csis_event {
> @@ -286,6 +296,8 @@ static const struct mipi_csis_event mipi_csis_events[] = {
> { MAIN, MIPI_CSIS_INT_SRC_FRAME_END, "Frame End"},
> { DEBUG, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL, "VSYNC Falling Edge"},
> { DEBUG, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE, "VSYNC Rising Edge"},
> + /* User Line interrupt */
> + { USER, MIPI_CSIS_INT_SRC_1_LINE_END, "Line End"}
> };
>
> #define MIPI_CSIS_NUM_EVENTS ARRAY_SIZE(mipi_csis_events)
> @@ -338,11 +350,14 @@ struct mipi_csis_device {
>
> spinlock_t slock; /* Protect events */
> struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> +
> struct dentry *debugfs_root;
> struct {
> bool enable;
> u32 hs_settle;
> u32 clk_settle;
> + u32 int_line;
> + u32 last_int_line;
> } debug;
> };
>
> @@ -533,6 +548,8 @@ static void mipi_csis_enable_interrupts(struct mipi_csis_device *csis, bool on)
> {
> mipi_csis_write(csis, MIPI_CSIS_INT_MSK, on ? 0xffffffff : 0);
> mipi_csis_write(csis, MIPI_CSIS_DBG_INTR_MSK, on ? 0xffffffff : 0);
> + mipi_csis_write(csis, MIPI_CSIS_INT_MSK_1,
> + on ? MIPI_CSIS_INT_MSK_1_LINE_END : 0);
> }
>
> static void mipi_csis_sw_reset(struct mipi_csis_device *csis)
> @@ -655,6 +672,7 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
> const struct csis_pix_format *csis_fmt)
> {
> int lanes = csis->bus.num_data_lanes;
> + u32 int_lines;
> u32 val;
>
> val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
> @@ -691,6 +709,13 @@ static void mipi_csis_set_params(struct mipi_csis_device *csis,
> MIPI_CSIS_DPHY_BCTRL_L_B_DPHYCTRL(20000000));
> mipi_csis_write(csis, MIPI_CSIS_DPHY_BCTRL_H, 0);
>
> + int_lines = READ_ONCE(csis->debug.int_line);
> + if (int_lines > 0)
> + mipi_csis_write(csis, MIPI_CSIS_LINE_INTERRUPT_RATIO(0),
> + max(int_lines, 1U) - 1);
The "int_lines > 0" condition and max(int_lines, 1U) are redundant. I'd
drop the former, in order to set the MIPI_CSIS_LINE_INTERRUPT_RATIO
register to 0 when int_line is 0, to match the logic in
mipi_csis_irq_handler().
> +
> + csis->debug.last_int_line = int_lines;
> +
> /* Update the shadow register. */
> val = mipi_csis_read(csis, MIPI_CSIS_CMN_CTRL);
> mipi_csis_write(csis, MIPI_CSIS_CMN_CTRL,
> @@ -770,10 +795,12 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> struct mipi_csis_device *csis = dev_id;
> unsigned long flags;
> unsigned int i;
> - u32 status[2];
> + u32 int_lines;
> + u32 status[3];
>
> status[MAIN] = mipi_csis_read(csis, MIPI_CSIS_INT_SRC);
> status[DEBUG] = mipi_csis_read(csis, MIPI_CSIS_DBG_INTR_SRC);
> + status[USER] = mipi_csis_read(csis, MIPI_CSIS_INT_SRC_1);
>
> spin_lock_irqsave(&csis->slock, flags);
>
> @@ -792,8 +819,16 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
>
> spin_unlock_irqrestore(&csis->slock, flags);
>
> + int_lines = READ_ONCE(csis->debug.int_line);
> + if (int_lines != csis->debug.last_int_line) {
> + mipi_csis_write(csis, MIPI_CSIS_LINE_INTERRUPT_RATIO(0),
> + max(int_lines, 1U) - 1);
> + csis->debug.last_int_line = int_lines;
> + }
> +
> mipi_csis_write(csis, MIPI_CSIS_INT_SRC, status[MAIN]);
> mipi_csis_write(csis, MIPI_CSIS_DBG_INTR_SRC, status[DEBUG]);
> + mipi_csis_write(csis, MIPI_CSIS_INT_SRC_1, status[USER]);
>
> return IRQ_HANDLED;
> }
> @@ -933,6 +968,7 @@ static void mipi_csis_debugfs_init(struct mipi_csis_device *csis)
> {
> csis->debug.hs_settle = UINT_MAX;
> csis->debug.clk_settle = UINT_MAX;
> + csis->debug.int_line = 0;
>
> csis->debugfs_root = debugfs_create_dir(dev_name(csis->dev), NULL);
>
> @@ -944,6 +980,8 @@ static void mipi_csis_debugfs_init(struct mipi_csis_device *csis)
> &csis->debug.clk_settle);
> debugfs_create_u32("ths_settle", 0600, csis->debugfs_root,
> &csis->debug.hs_settle);
> + debugfs_create_u32("int_line_0", 0600, csis->debugfs_root,
> + &csis->debug.int_line);
> }
>
> static void mipi_csis_debugfs_exit(struct mipi_csis_device *csis)
--
Regards,
Laurent Pinchart
More information about the linux-arm-kernel
mailing list