| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250607183847.GD27510@pendragon.ideasonboard.com>
Date: Sat, 7 Jun 2025 21:38:47 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Isaac Scott <isaac.scott@...asonboard.com>
Cc: kieran.bingham@...asonboard.com, rmfrfs@...il.com, martink@...teo.de,
kernel@...i.sm, mchehab@...nel.org, shawnguo@...nel.org,
s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
linux-media@...r.kernel.org, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] media: platform: Refactor interrupt status
registers
Hi Isaac,
Thank you for the patch.
The subject line should start with "media: imx-mipi-csis: ".
On Fri, Jun 06, 2025 at 04:44:13PM +0100, 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@...asonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>
> ---
>
> Changes since v1:
> - Switched from magic numbers to enum.
> ---
> drivers/media/platform/nxp/imx-mipi-csis.c | 69 +++++++++++-----------
> 1 file changed, 36 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..394987d72c64 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -248,8 +248,13 @@
> #define MIPI_CSI2_DATA_TYPE_RAW14 0x2d
> #define MIPI_CSI2_DATA_TYPE_USER(x) (0x30 + (x))
>
> +enum mipi_csis_event_type {
> + MAIN = 0,
> + DEBUG = 1,
Those names are too generic and prone to namespace clashes.
MIPI_CSIS_EVENT_TYPE_MAIN and MIPI_CSIS_EVENT_TYPE_DEBUG would be
better.
No need to resubmit for this, I'll update the code when applying.
> +};
> +
> struct mipi_csis_event {
> - bool debug;
> + enum mipi_csis_event_type status_index;
> u32 mask;
> const char * const name;
> unsigned int counter;
> @@ -257,30 +262,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" },
> + { MAIN, MIPI_CSIS_INT_SRC_ERR_SOT_HS, "SOT Error"},
> + { MAIN, MIPI_CSIS_INT_SRC_ERR_LOST_FS, "Lost Frame Start Error"},
> + { MAIN, MIPI_CSIS_INT_SRC_ERR_LOST_FE, "Lost Frame End Error"},
> + { MAIN, MIPI_CSIS_INT_SRC_ERR_OVER, "FIFO Overflow Error"},
> + { MAIN, MIPI_CSIS_INT_SRC_ERR_WRONG_CFG, "Wrong Configuration Error"},
> + { MAIN, MIPI_CSIS_INT_SRC_ERR_ECC, "ECC Error"},
> + { MAIN, MIPI_CSIS_INT_SRC_ERR_CRC, "CRC Error"},
> + { MAIN, MIPI_CSIS_INT_SRC_ERR_UNKNOWN, "Unknown Error"},
> + { DEBUG, MIPI_CSIS_DBG_INTR_SRC_DT_NOT_SUPPORT, "Data Type Not Supported"},
> + { DEBUG, MIPI_CSIS_DBG_INTR_SRC_DT_IGNORE, "Data Type Ignored"},
> + { DEBUG, MIPI_CSIS_DBG_INTR_SRC_ERR_FRAME_SIZE, "Frame Size Error"},
> + { DEBUG, MIPI_CSIS_DBG_INTR_SRC_TRUNCATED_FRAME, "Truncated Frame"},
> + { DEBUG, MIPI_CSIS_DBG_INTR_SRC_EARLY_FE, "Early Frame End"},
> + { DEBUG, 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" },
> + { MAIN, MIPI_CSIS_INT_SRC_EVEN_BEFORE, "Non-image data before even frame"},
> + { MAIN, MIPI_CSIS_INT_SRC_EVEN_AFTER, "Non-image data after even frame"},
> + { MAIN, MIPI_CSIS_INT_SRC_ODD_BEFORE, "Non-image data before odd frame"},
> + { MAIN, 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" },
> + { MAIN, MIPI_CSIS_INT_SRC_FRAME_START, "Frame Start"},
> + { 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"},
> };
>
> #define MIPI_CSIS_NUM_EVENTS ARRAY_SIZE(mipi_csis_events)
> @@ -765,32 +770,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[MAIN] = mipi_csis_read(csis, MIPI_CSIS_INT_SRC);
> + status[DEBUG] = 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[MAIN] & 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[MAIN] & 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[MAIN]);
> + mipi_csis_write(csis, MIPI_CSIS_DBG_INTR_SRC, status[DEBUG]);
>
> return IRQ_HANDLED;
> }
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists