[PATCH 1/2] media: platform: Refactor interrupt status registers
Isaac Scott
isaac.scott at ideasonboard.com
Fri Jun 6 08:47:04 PDT 2025
Hi Rui,
On Fri, 2025-06-06 at 14:56 +0100, Rui Miguel Silva wrote:
> Hey Isaac,
> Thanks for the patch.
>
> On Fri Jun 6, 2025 at 1:14 PM WEST, Isaac Scott wrote:
>
> > The NXP i.MX 8 MP CSI-2 receiver features multiple interrupt and
> > debug
> > status sources which span multiple registers. The driver currently
> > supports two interrupt source registers, and attributes the
> > mipi_csis_event event entries to those registers through a boolean
> > debug
> > field that indicate if the event relates to the main interrupt
> > status
> > (false) or debug interrupt status (true) register. To make it
> > easier to
> > add new event fields, replace the debug bool with a 'status index'
> > integer than indicates the index of the corresponding status
> > register.
> >
> > Signed-off-by: Isaac Scott <isaac.scott at ideasonboard.com>
> > ---
> > drivers/media/platform/nxp/imx-mipi-csis.c | 64 +++++++++++-------
> > ----
> > 1 file changed, 31 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c
> > b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index d060eadebc7a..bbc549c22aff 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -249,7 +249,7 @@
> > #define MIPI_CSI2_DATA_TYPE_USER(x) (0x30 + (x))
> >
> > struct mipi_csis_event {
> > - bool debug;
> > + unsigned int status_index;
> > u32 mask;
> > const char * const name;
> > unsigned int counter;
> > @@ -257,30 +257,30 @@ struct mipi_csis_event {
> >
> > static const struct mipi_csis_event mipi_csis_events[] = {
> > /* Errors */
> > - { false, MIPI_CSIS_INT_SRC_ERR_SOT_HS, "SOT
> > Error" },
> > - { false,
> > MIPI_CSIS_INT_SRC_ERR_LOST_FS, "Lost Frame Start Error" },
> > - { false,
> > MIPI_CSIS_INT_SRC_ERR_LOST_FE, "Lost Frame End Error" },
> > - { false, MIPI_CSIS_INT_SRC_ERR_OVER, "FIFO
> > Overflow Error" },
> > - { false, MIPI_CSIS_INT_SRC_ERR_WRONG_CFG, "Wrong
> > Configuration Error" },
> > - { false, MIPI_CSIS_INT_SRC_ERR_ECC, "ECC
> > Error" },
> > - { false, MIPI_CSIS_INT_SRC_ERR_CRC, "CRC
> > Error" },
> > - { false,
> > MIPI_CSIS_INT_SRC_ERR_UNKNOWN, "Unknown Error" },
> > - { true, MIPI_CSIS_DBG_INTR_SRC_DT_NOT_SUPPORT, "Data Type
> > Not Supported" },
> > - { true, MIPI_CSIS_DBG_INTR_SRC_DT_IGNORE, "Data Type
> > Ignored" },
> > - { true, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE, "Frame
> > Size Error" },
> > - { true,
> > MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME, "Truncated Frame" },
> > - { true, MIPI_CSIS_DBG_INTR_SRC_EARLY_FE, "Early
> > Frame End" },
> > - { true, MIPI_CSIS_DBG_INTR_SRC_EARLY_FS, "Early
> > Frame Start" },
> > + { 0, MIPI_CSIS_INT_SRC_ERR_SOT_HS, "SOT
> > Error"},
>
> Maybe instead of 0,1,2 (magic indexes)... we could give a meaningful
> index
> enums names, don't know, like: main, debug, user??? or something that
> you think is better.
Thanks for the review! I have updated v2 to include an enum instead of
magic numbers.
Best wishes,
Isaac
>
> Cheers,
> Rui
>
> > + { 0, MIPI_CSIS_INT_SRC_ERR_LOST_FS, "Lost
> > Frame Start Error"},
> > + { 0, MIPI_CSIS_INT_SRC_ERR_LOST_FE, "Lost
> > Frame End Error"},
> > + { 0, MIPI_CSIS_INT_SRC_ERR_OVER, "FIFO
> > Overflow Error"},
> > + { 0, MIPI_CSIS_INT_SRC_ERR_WRONG_CFG, "Wrong
> > Configuration Error"},
> > + { 0,
> > MIPI_CSIS_INT_SRC_ERR_ECC, "ECC Error"},
> > + { 0,
> > MIPI_CSIS_INT_SRC_ERR_CRC, "CRC Error"},
> > + { 0, MIPI_CSIS_INT_SRC_ERR_UNKNOWN, "Unknown
> > Error"},
> > + { 1, MIPI_CSIS_DBG_INTR_SRC_DT_NOT_SUPPORT, "Data Type
> > Not Supported"},
> > + { 1, MIPI_CSIS_DBG_INTR_SRC_DT_IGNORE, "Data Type
> > Ignored"},
> > + { 1, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE, "Frame
> > Size Error"},
> > + { 1, MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME, "Truncated
> > Frame"},
> > + { 1, MIPI_CSIS_DBG_INTR_SRC_EARLY_FE, "Early
> > Frame End"},
> > + { 1, MIPI_CSIS_DBG_INTR_SRC_EARLY_FS, "Early
> > Frame Start"},
> > /* Non-image data receive events */
> > - { false,
> > MIPI_CSIS_INT_SRC_EVEN_BEFORE, "Non-image data before even frame" },
> > - { false, MIPI_CSIS_INT_SRC_EVEN_AFTER, "Non-image
> > data after even frame" },
> > - { false, MIPI_CSIS_INT_SRC_ODD_BEFORE, "Non-image
> > data before odd frame" },
> > - { false, MIPI_CSIS_INT_SRC_ODD_AFTER, "Non-image
> > data after odd frame" },
> > + { 0, MIPI_CSIS_INT_SRC_EVEN_BEFORE, "Non-image
> > data before even frame"},
> > + { 0, MIPI_CSIS_INT_SRC_EVEN_AFTER, "Non-image
> > data after even frame"},
> > + { 0, MIPI_CSIS_INT_SRC_ODD_BEFORE, "Non-image
> > data before odd frame"},
> > + { 0, MIPI_CSIS_INT_SRC_ODD_AFTER, "Non-image
> > data after odd frame"},
> > /* Frame start/end */
> > - { false,
> > MIPI_CSIS_INT_SRC_FRAME_START, "Frame Start" },
> > - { false, MIPI_CSIS_INT_SRC_FRAME_END, "Frame
> > End" },
> > - { true, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL, "VSYNC
> > Falling Edge" },
> > - { true, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE, "VSYNC
> > Rising Edge" },
> > + { 0, MIPI_CSIS_INT_SRC_FRAME_START, "Frame
> > Start"},
> > + { 0, MIPI_CSIS_INT_SRC_FRAME_END, "Frame
> > End"},
> > + { 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL, "VSYNC
> > Falling Edge"},
> > + { 1, MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE, "VSYNC
> > Rising Edge"},
> > };
> >
> > #define MIPI_CSIS_NUM_EVENTS ARRAY_SIZE(mipi_csis_events)
> > @@ -765,32 +765,30 @@ 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;
> > - u32 dbg_status;
> > + u32 status[2];
> >
> > - status = mipi_csis_read(csis, MIPI_CSIS_INT_SRC);
> > - dbg_status = mipi_csis_read(csis, MIPI_CSIS_DBG_INTR_SRC);
> > + status[0] = mipi_csis_read(csis, MIPI_CSIS_INT_SRC);
> > + status[1] = mipi_csis_read(csis, MIPI_CSIS_DBG_INTR_SRC);
> >
> > spin_lock_irqsave(&csis->slock, flags);
> >
> > /* Update the event/error counters */
> > - if ((status & MIPI_CSIS_INT_SRC_ERRORS) || csis-
> > >debug.enable) {
> > + if ((status[0] & MIPI_CSIS_INT_SRC_ERRORS) || csis-
> > >debug.enable) {
> > for (i = 0; i < MIPI_CSIS_NUM_EVENTS; i++) {
> > struct mipi_csis_event *event = &csis-
> > >events[i];
> >
> > - if ((!event->debug && (status & event-
> > >mask)) ||
> > - (event->debug && (dbg_status & event-
> > >mask)))
> > + if (status[event->status_index] & event-
> > >mask)
> > event->counter++;
> > }
> > }
> >
> > - if (status & MIPI_CSIS_INT_SRC_FRAME_START)
> > + if (status[0] & 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);
> > - mipi_csis_write(csis, MIPI_CSIS_DBG_INTR_SRC, dbg_status);
> > + mipi_csis_write(csis, MIPI_CSIS_INT_SRC, status[0]);
> > + mipi_csis_write(csis, MIPI_CSIS_DBG_INTR_SRC, status[1]);
> >
> > return IRQ_HANDLED;
> > }
> > --
> > 2.43.0
>
>
More information about the linux-arm-kernel
mailing list