* [RFC PATCH 0/3] Enable support for error detection in CSI2RX
@ 2025-02-12 13:12 Yemike Abhilash Chandra
2025-02-12 13:12 ` [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx Yemike Abhilash Chandra
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Yemike Abhilash Chandra @ 2025-02-12 13:12 UTC (permalink / raw)
To: linux-media, linux-kernel, devicetree
Cc: mripard, mchehab, jai.luthra, robh, krzk+dt, conor+dt, devarsht,
vaishnav.a, r-donadkar, u-kumar1, y-abhilashchandra
This patch series enables the csi2rx_err_irq interrupt to record any errors
that occur during streaming. It also adds support for the VIDIOC_LOG_STATUS
ioctl, which outputs the current device status to the kernel log.
The IRQ handler records any errors encountered during streaming.
Additionally, VIDIOC_LOG_STATUS can be invoked from user space to retrieve
the latest status.
Logs with interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/58ced4df0158efad6f453b4d96463723
Logs without interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/d807230b2a624b7a38effed89efdd148
Yemike Abhilash Chandra (3):
dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for
cdns-csi2rx
media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add
support for VIDIOC_LOG_STATUS
media: ti: j721e-csi2rx: Add support for VIDIOC_LOG_STATUS
.../bindings/media/cdns,csi2rx.yaml | 11 ++
drivers/media/platform/cadence/cdns-csi2rx.c | 104 +++++++++++++++++-
.../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 10 ++
3 files changed, 124 insertions(+), 1 deletion(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx
2025-02-12 13:12 [RFC PATCH 0/3] Enable support for error detection in CSI2RX Yemike Abhilash Chandra
@ 2025-02-12 13:12 ` Yemike Abhilash Chandra
2025-02-12 19:28 ` Krzysztof Kozlowski
2025-02-12 13:12 ` [RFC PATCH 2/3] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS Yemike Abhilash Chandra
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Yemike Abhilash Chandra @ 2025-02-12 13:12 UTC (permalink / raw)
To: linux-media, linux-kernel, devicetree
Cc: mripard, mchehab, jai.luthra, robh, krzk+dt, conor+dt, devarsht,
vaishnav.a, r-donadkar, u-kumar1, y-abhilashchandra
The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
Enabling these interrupts will provide additional information about a CSI
packet or an individual frame. So, add support for optional interrupts
and interrupt-names properties.
[0]: http://www.ti.com/lit/pdf/spruil1
Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---
.../devicetree/bindings/media/cdns,csi2rx.yaml | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
index 2008a47c0580..a3acf4f861c2 100644
--- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
+++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
@@ -24,6 +24,17 @@ properties:
reg:
maxItems: 1
+ interrupts:
+ minItems: 1
+ maxItems: 3
+
+ interrupt-names:
+ minItems: 1
+ items:
+ - const: info
+ - const: error
+ - const: monitor
+
clocks:
items:
- description: CSI2Rx system clock
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFC PATCH 2/3] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS
2025-02-12 13:12 [RFC PATCH 0/3] Enable support for error detection in CSI2RX Yemike Abhilash Chandra
2025-02-12 13:12 ` [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx Yemike Abhilash Chandra
@ 2025-02-12 13:12 ` Yemike Abhilash Chandra
2025-02-14 6:14 ` Jai Luthra
2025-02-12 13:12 ` [RFC PATCH 3/3] media: ti: j721e-csi2rx: Add " Yemike Abhilash Chandra
2025-02-12 19:16 ` [RFC PATCH 0/3] Enable support for error detection in CSI2RX Conor Dooley
3 siblings, 1 reply; 13+ messages in thread
From: Yemike Abhilash Chandra @ 2025-02-12 13:12 UTC (permalink / raw)
To: linux-media, linux-kernel, devicetree
Cc: mripard, mchehab, jai.luthra, robh, krzk+dt, conor+dt, devarsht,
vaishnav.a, r-donadkar, u-kumar1, y-abhilashchandra
Enable the csi2rx_err_irq interrupt to record any errors during streaming
and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
ioctl can be invoked from user space to retrieve the device status,
including details about any errors.
Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---
drivers/media/platform/cadence/cdns-csi2rx.c | 104 ++++++++++++++++++-
1 file changed, 103 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 4d64df829e75..b3d76f0678fa 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -57,6 +57,28 @@
#define CSI2RX_LANES_MAX 4
#define CSI2RX_STREAMS_MAX 4
+#define CSI2RX_ERROR_IRQS_REG 0x28
+#define CSI2RX_ERROR_IRQS_MASK_REG 0x2C
+
+#define CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ BIT(19)
+#define CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ BIT(18)
+#define CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ BIT(17)
+#define CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ BIT(16)
+#define CSI2RX_FRONT_TRUNC_HDR_IRQ BIT(12)
+#define CSI2RX_PROT_TRUNCATED_PACKET_IRQ BIT(11)
+#define CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ BIT(10)
+#define CSI2RX_SP_INVALID_RCVD_IRQ BIT(9)
+#define CSI2RX_DATA_ID_IRQ BIT(7)
+#define CSI2RX_HEADER_CORRECTED_ECC_IRQ BIT(6)
+#define CSI2RX_HEADER_ECC_IRQ BIT(5)
+#define CSI2RX_PAYLOAD_CRC_IRQ BIT(4)
+
+#define CSI2RX_ECC_ERRORS GENMASK(7, 4)
+#define CSI2RX_PACKET_ERRORS GENMASK(12, 9)
+#define CSI2RX_STREAM_ERRORS GENMASK(19, 16)
+#define CSI2RX_ERRORS (CSI2RX_ECC_ERRORS | CSI2RX_PACKET_ERRORS | \
+ CSI2RX_STREAM_ERRORS)
+
enum csi2rx_pads {
CSI2RX_PAD_SINK,
CSI2RX_PAD_SOURCE_STREAM0,
@@ -71,6 +93,28 @@ struct csi2rx_fmt {
u8 bpp;
};
+struct csi2rx_event {
+ u32 mask;
+ const char *name;
+};
+
+static const struct csi2rx_event csi2rx_events[] = {
+ { CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 3 FIFO detected" },
+ { CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 2 FIFO detected" },
+ { CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 1 FIFO detected" },
+ { CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 0 FIFO detected" },
+ { CSI2RX_FRONT_TRUNC_HDR_IRQ, "A truncated header [short or long] has been received" },
+ { CSI2RX_PROT_TRUNCATED_PACKET_IRQ, "A truncated long packet has been received" },
+ { CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ, "A truncated long packet has been received. No payload" },
+ { CSI2RX_SP_INVALID_RCVD_IRQ, "A reserved or invalid short packet has been received" },
+ { CSI2RX_DATA_ID_IRQ, "Data ID error in the header packet" },
+ { CSI2RX_HEADER_CORRECTED_ECC_IRQ, "ECC error detected and corrected" },
+ { CSI2RX_HEADER_ECC_IRQ, "Unrecoverable ECC error" },
+ { CSI2RX_PAYLOAD_CRC_IRQ, "CRC error" },
+};
+
+#define CSI2RX_NUM_EVENTS ARRAY_SIZE(csi2rx_events)
+
struct csi2rx_priv {
struct device *dev;
unsigned int count;
@@ -95,6 +139,7 @@ struct csi2rx_priv {
u8 max_lanes;
u8 max_streams;
bool has_internal_dphy;
+ u32 events[CSI2RX_NUM_EVENTS];
struct v4l2_subdev subdev;
struct v4l2_async_notifier notifier;
@@ -124,6 +169,29 @@ static const struct csi2rx_fmt formats[] = {
{ .code = MEDIA_BUS_FMT_BGR888_1X24, .bpp = 24, },
};
+static void csi2rx_configure_err_irq_mask(void __iomem *base)
+{
+ writel(CSI2RX_ERRORS, base + CSI2RX_ERROR_IRQS_MASK_REG);
+}
+
+static irqreturn_t csi2rx_irq_handler(int irq, void *dev_id)
+{
+ struct csi2rx_priv *csi2rx = dev_id;
+ int i;
+ u32 error_status;
+
+ error_status = readl(csi2rx->base + CSI2RX_ERROR_IRQS_REG);
+
+ for (i = 0; i < CSI2RX_NUM_EVENTS; i++)
+ if (error_status & csi2rx_events[i].mask)
+ csi2rx->events[i]++;
+
+ writel(CSI2RX_ERRORS & error_status,
+ csi2rx->base + CSI2RX_ERROR_IRQS_REG);
+
+ return IRQ_HANDLED;
+}
+
static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
{
unsigned int i;
@@ -209,12 +277,26 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
unsigned int i;
unsigned long lanes_used = 0;
u32 reg;
- int ret;
+ int irq, ret;
ret = clk_prepare_enable(csi2rx->p_clk);
if (ret)
return ret;
+ irq = platform_get_irq_byname_optional(to_platform_device(csi2rx->dev), "error");
+
+ if (irq < 0) {
+ dev_warn(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
+ } else {
+ csi2rx_configure_err_irq_mask(csi2rx->base);
+ ret = devm_request_irq(csi2rx->dev, irq, csi2rx_irq_handler, 0,
+ "csi2rx-irq", csi2rx);
+ if (ret) {
+ dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
+ return ret;
+ }
+ }
+
reset_control_deassert(csi2rx->p_rst);
csi2rx_reset(csi2rx);
@@ -361,6 +443,21 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
}
}
+static int csi2rx_log_status(struct v4l2_subdev *sd)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
+ unsigned int i;
+
+ for (i = 0; i < CSI2RX_NUM_EVENTS; i++) {
+ if (csi2rx->events[i])
+ dev_info(csi2rx->dev, "%s events: %d\n",
+ csi2rx_events[i].name,
+ csi2rx->events[i]);
+ }
+
+ return 0;
+}
+
static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
{
struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
@@ -466,7 +563,12 @@ static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
.s_stream = csi2rx_s_stream,
};
+static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
+ .log_status = csi2rx_log_status,
+};
+
static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
+ .core = &csi2rx_core_ops,
.video = &csi2rx_video_ops,
.pad = &csi2rx_pad_ops,
};
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFC PATCH 3/3] media: ti: j721e-csi2rx: Add support for VIDIOC_LOG_STATUS
2025-02-12 13:12 [RFC PATCH 0/3] Enable support for error detection in CSI2RX Yemike Abhilash Chandra
2025-02-12 13:12 ` [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx Yemike Abhilash Chandra
2025-02-12 13:12 ` [RFC PATCH 2/3] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS Yemike Abhilash Chandra
@ 2025-02-12 13:12 ` Yemike Abhilash Chandra
2025-02-13 13:26 ` Jai Luthra
2025-02-12 19:16 ` [RFC PATCH 0/3] Enable support for error detection in CSI2RX Conor Dooley
3 siblings, 1 reply; 13+ messages in thread
From: Yemike Abhilash Chandra @ 2025-02-12 13:12 UTC (permalink / raw)
To: linux-media, linux-kernel, devicetree
Cc: mripard, mchehab, jai.luthra, robh, krzk+dt, conor+dt, devarsht,
vaishnav.a, r-donadkar, u-kumar1, y-abhilashchandra
The VIDIOC_LOG_STATUS ioctl outputs the current status of a device to the
kernel log. When this ioctl is called on a video device, the current
implementation queries the log status for all connected subdevices in the
media pipeline.
Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---
drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
index 6412a00be8ea..946704458fee 100644
--- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
+++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
@@ -377,6 +377,15 @@ static int ti_csi2rx_enum_framesizes(struct file *file, void *fh,
return 0;
}
+static int ti_csi2rx_log_status(struct file *file, void *fh)
+{
+ struct ti_csi2rx_dev *csi = video_drvdata(file);
+
+ v4l2_device_call_all(&csi->v4l2_dev, 0, core, log_status);
+
+ return 0;
+}
+
static const struct v4l2_ioctl_ops csi_ioctl_ops = {
.vidioc_querycap = ti_csi2rx_querycap,
.vidioc_enum_fmt_vid_cap = ti_csi2rx_enum_fmt_vid_cap,
@@ -393,6 +402,7 @@ static const struct v4l2_ioctl_ops csi_ioctl_ops = {
.vidioc_expbuf = vb2_ioctl_expbuf,
.vidioc_streamon = vb2_ioctl_streamon,
.vidioc_streamoff = vb2_ioctl_streamoff,
+ .vidioc_log_status = ti_csi2rx_log_status,
};
static const struct v4l2_file_operations csi_fops = {
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 0/3] Enable support for error detection in CSI2RX
2025-02-12 13:12 [RFC PATCH 0/3] Enable support for error detection in CSI2RX Yemike Abhilash Chandra
` (2 preceding siblings ...)
2025-02-12 13:12 ` [RFC PATCH 3/3] media: ti: j721e-csi2rx: Add " Yemike Abhilash Chandra
@ 2025-02-12 19:16 ` Conor Dooley
2025-02-13 7:10 ` Yemike Abhilash Chandra
3 siblings, 1 reply; 13+ messages in thread
From: Conor Dooley @ 2025-02-12 19:16 UTC (permalink / raw)
To: Yemike Abhilash Chandra
Cc: linux-media, linux-kernel, devicetree, mripard, mchehab,
jai.luthra, robh, krzk+dt, conor+dt, devarsht, vaishnav.a,
r-donadkar, u-kumar1
[-- Attachment #1: Type: text/plain, Size: 774 bytes --]
On Wed, Feb 12, 2025 at 06:42:41PM +0530, Yemike Abhilash Chandra wrote:
> This patch series enables the csi2rx_err_irq interrupt to record any errors
> that occur during streaming. It also adds support for the VIDIOC_LOG_STATUS
> ioctl, which outputs the current device status to the kernel log.
>
> The IRQ handler records any errors encountered during streaming.
> Additionally, VIDIOC_LOG_STATUS can be invoked from user space to retrieve
> the latest status.
>
> Logs with interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/58ced4df0158efad6f453b4d96463723
> Logs without interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/d807230b2a624b7a38effed89efdd148
What is actually RFC about this patchset, rather than just being v1?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx
2025-02-12 13:12 ` [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx Yemike Abhilash Chandra
@ 2025-02-12 19:28 ` Krzysztof Kozlowski
2025-02-13 7:16 ` Yemike Abhilash Chandra
0 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2025-02-12 19:28 UTC (permalink / raw)
To: Yemike Abhilash Chandra, linux-media, linux-kernel, devicetree
Cc: mripard, mchehab, jai.luthra, robh, krzk+dt, conor+dt, devarsht,
vaishnav.a, r-donadkar, u-kumar1
On 12/02/2025 14:12, Yemike Abhilash Chandra wrote:
> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
> Enabling these interrupts will provide additional information about a CSI
> packet or an individual frame. So, add support for optional interrupts
> and interrupt-names properties.
>
> [0]: http://www.ti.com/lit/pdf/spruil1
Why is this RFC?
>
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---
> .../devicetree/bindings/media/cdns,csi2rx.yaml | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> index 2008a47c0580..a3acf4f861c2 100644
> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> @@ -24,6 +24,17 @@ properties:
> reg:
> maxItems: 1
>
> + interrupts:
> + minItems: 1
> + maxItems: 3
I understand interrupts might be unused by driver, but are you sure they
are optionally connected one-by-one? IOW, why is this flexible?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 0/3] Enable support for error detection in CSI2RX
2025-02-12 19:16 ` [RFC PATCH 0/3] Enable support for error detection in CSI2RX Conor Dooley
@ 2025-02-13 7:10 ` Yemike Abhilash Chandra
0 siblings, 0 replies; 13+ messages in thread
From: Yemike Abhilash Chandra @ 2025-02-13 7:10 UTC (permalink / raw)
To: Conor Dooley
Cc: linux-media, linux-kernel, devicetree, mripard, mchehab,
jai.luthra, robh, krzk+dt, conor+dt, devarsht, vaishnav.a,
r-donadkar, u-kumar1
On 13/02/25 00:46, Conor Dooley wrote:
> On Wed, Feb 12, 2025 at 06:42:41PM +0530, Yemike Abhilash Chandra wrote:
>> This patch series enables the csi2rx_err_irq interrupt to record any errors
>> that occur during streaming. It also adds support for the VIDIOC_LOG_STATUS
>> ioctl, which outputs the current device status to the kernel log.
>>
>> The IRQ handler records any errors encountered during streaming.
>> Additionally, VIDIOC_LOG_STATUS can be invoked from user space to retrieve
>> the latest status.
>>
>> Logs with interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/58ced4df0158efad6f453b4d96463723
>> Logs without interrupt in DT: https://gist.github.com/Yemike-Abhilash-Chandra/d807230b2a624b7a38effed89efdd148
>
> What is actually RFC about this patchset, rather than just being v1?
I sent this as an RFC to gather input from different vendors using the
cdns,csi2rx driver and its device tree bindings. so I just wanted to
get their feedback as well. If there are no concerns from any of the them,
I will proceed with sending this as v1.
Thanks and Regards,
Yemike Abhilash Chandra
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx
2025-02-12 19:28 ` Krzysztof Kozlowski
@ 2025-02-13 7:16 ` Yemike Abhilash Chandra
2025-02-13 7:36 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Yemike Abhilash Chandra @ 2025-02-13 7:16 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-media, linux-kernel, devicetree
Cc: mripard, mchehab, jai.luthra, robh, krzk+dt, conor+dt, devarsht,
vaishnav.a, r-donadkar, u-kumar1
On 13/02/25 00:58, Krzysztof Kozlowski wrote:
> On 12/02/2025 14:12, Yemike Abhilash Chandra wrote:
>> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
>> Enabling these interrupts will provide additional information about a CSI
>> packet or an individual frame. So, add support for optional interrupts
>> and interrupt-names properties.
>>
>> [0]: http://www.ti.com/lit/pdf/spruil1
>
>
> Why is this RFC?
>
I sent this as an RFC to gather input from different vendors using the
cdns,csi2rx driver
and its device tree bindings. so I just wanted to get their feedback as
well.
If there are no concerns from any of the them, I will proceed with
sending this as v1.
>>
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> ---
>> .../devicetree/bindings/media/cdns,csi2rx.yaml | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>> index 2008a47c0580..a3acf4f861c2 100644
>> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>> @@ -24,6 +24,17 @@ properties:
>> reg:
>> maxItems: 1
>>
>> + interrupts:
>> + minItems: 1
>> + maxItems: 3
>
> I understand interrupts might be unused by driver, but are you sure they
> are optionally connected one-by-one? IOW, why is this flexible?
>
I understand that this flexibility is not needed, and I will correct
that while sending v1.
Thanks and Regards,
Yemike Abhilash Chandra
>
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx
2025-02-13 7:16 ` Yemike Abhilash Chandra
@ 2025-02-13 7:36 ` Krzysztof Kozlowski
0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2025-02-13 7:36 UTC (permalink / raw)
To: Yemike Abhilash Chandra, linux-media, linux-kernel, devicetree
Cc: mripard, mchehab, jai.luthra, robh, krzk+dt, conor+dt, devarsht,
vaishnav.a, r-donadkar, u-kumar1
On 13/02/2025 08:16, Yemike Abhilash Chandra wrote:
>
> On 13/02/25 00:58, Krzysztof Kozlowski wrote:
>> On 12/02/2025 14:12, Yemike Abhilash Chandra wrote:
>>> The Cadence CSI2RX IP exposes 3 interrupts [0] 12.7 camera subsystem.
>>> Enabling these interrupts will provide additional information about a CSI
>>> packet or an individual frame. So, add support for optional interrupts
>>> and interrupt-names properties.
>>>
>>> [0]: http://www.ti.com/lit/pdf/spruil1
>>
>>
>> Why is this RFC?
>>
>
> I sent this as an RFC to gather input from different vendors using the
> cdns,csi2rx driver
> and its device tree bindings. so I just wanted to get their feedback as
> well.
Then document it clearly that you do not expect review.
> If there are no concerns from any of the them, I will proceed with
> sending this as v1.
No, this was v1.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 3/3] media: ti: j721e-csi2rx: Add support for VIDIOC_LOG_STATUS
2025-02-12 13:12 ` [RFC PATCH 3/3] media: ti: j721e-csi2rx: Add " Yemike Abhilash Chandra
@ 2025-02-13 13:26 ` Jai Luthra
2025-02-17 8:07 ` Yemike Abhilash Chandra
0 siblings, 1 reply; 13+ messages in thread
From: Jai Luthra @ 2025-02-13 13:26 UTC (permalink / raw)
To: Yemike Abhilash Chandra
Cc: linux-media, linux-kernel, devicetree, mripard, mchehab, robh,
krzk+dt, conor+dt, devarsht, vaishnav.a, r-donadkar, u-kumar1
[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]
Hi Abhilash,
On Wed, Feb 12, 2025 at 06:42:44PM +0530, Yemike Abhilash Chandra wrote:
> The VIDIOC_LOG_STATUS ioctl outputs the current status of a device to the
> kernel log. When this ioctl is called on a video device, the current
> implementation queries the log status for all connected subdevices in the
> media pipeline.
>
What is the benefit of doing this for a video node? The user can directly
check the status on the cdns-csi2rx subdev for CSI errors.
As far as I understand, the video node corresponds to a particular stream, but
the cdns-csi2rx source pad is shared for all video nodes, so it will report
the total errors seen for all video nodes in multi-camera scenarios.
This approach will also give you v4l2 control handler status from a few
sensors (like OV5640) that implement the ioctl using
v4l2_ctrl_subdev_log_status(), which is probably just noise for the case where
a user wants to check for stream errors.
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---
> drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index 6412a00be8ea..946704458fee 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> @@ -377,6 +377,15 @@ static int ti_csi2rx_enum_framesizes(struct file *file, void *fh,
> return 0;
> }
>
> +static int ti_csi2rx_log_status(struct file *file, void *fh)
> +{
> + struct ti_csi2rx_dev *csi = video_drvdata(file);
> +
> + v4l2_device_call_all(&csi->v4l2_dev, 0, core, log_status);
> +
> + return 0;
> +}
> +
> static const struct v4l2_ioctl_ops csi_ioctl_ops = {
> .vidioc_querycap = ti_csi2rx_querycap,
> .vidioc_enum_fmt_vid_cap = ti_csi2rx_enum_fmt_vid_cap,
> @@ -393,6 +402,7 @@ static const struct v4l2_ioctl_ops csi_ioctl_ops = {
> .vidioc_expbuf = vb2_ioctl_expbuf,
> .vidioc_streamon = vb2_ioctl_streamon,
> .vidioc_streamoff = vb2_ioctl_streamoff,
> + .vidioc_log_status = ti_csi2rx_log_status,
> };
>
> static const struct v4l2_file_operations csi_fops = {
> --
> 2.34.1
>
Thanks,
Jai
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/3] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS
2025-02-12 13:12 ` [RFC PATCH 2/3] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS Yemike Abhilash Chandra
@ 2025-02-14 6:14 ` Jai Luthra
2025-02-17 7:57 ` Yemike Abhilash Chandra
0 siblings, 1 reply; 13+ messages in thread
From: Jai Luthra @ 2025-02-14 6:14 UTC (permalink / raw)
To: Yemike Abhilash Chandra
Cc: linux-media, linux-kernel, devicetree, mripard, mchehab, robh,
krzk+dt, conor+dt, devarsht, vaishnav.a, r-donadkar, u-kumar1
[-- Attachment #1: Type: text/plain, Size: 6539 bytes --]
Hi Abhilash,
On Wed, Feb 12, 2025 at 06:42:43PM +0530, Yemike Abhilash Chandra wrote:
> Enable the csi2rx_err_irq interrupt to record any errors during streaming
> and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
> ioctl can be invoked from user space to retrieve the device status,
> including details about any errors.
>
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---
> drivers/media/platform/cadence/cdns-csi2rx.c | 104 ++++++++++++++++++-
> 1 file changed, 103 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 4d64df829e75..b3d76f0678fa 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -57,6 +57,28 @@
> #define CSI2RX_LANES_MAX 4
> #define CSI2RX_STREAMS_MAX 4
>
> +#define CSI2RX_ERROR_IRQS_REG 0x28
> +#define CSI2RX_ERROR_IRQS_MASK_REG 0x2C
> +
> +#define CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ BIT(19)
> +#define CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ BIT(18)
> +#define CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ BIT(17)
> +#define CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ BIT(16)
> +#define CSI2RX_FRONT_TRUNC_HDR_IRQ BIT(12)
> +#define CSI2RX_PROT_TRUNCATED_PACKET_IRQ BIT(11)
> +#define CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ BIT(10)
> +#define CSI2RX_SP_INVALID_RCVD_IRQ BIT(9)
> +#define CSI2RX_DATA_ID_IRQ BIT(7)
> +#define CSI2RX_HEADER_CORRECTED_ECC_IRQ BIT(6)
> +#define CSI2RX_HEADER_ECC_IRQ BIT(5)
> +#define CSI2RX_PAYLOAD_CRC_IRQ BIT(4)
> +
> +#define CSI2RX_ECC_ERRORS GENMASK(7, 4)
> +#define CSI2RX_PACKET_ERRORS GENMASK(12, 9)
> +#define CSI2RX_STREAM_ERRORS GENMASK(19, 16)
> +#define CSI2RX_ERRORS (CSI2RX_ECC_ERRORS | CSI2RX_PACKET_ERRORS | \
> + CSI2RX_STREAM_ERRORS)
> +
> enum csi2rx_pads {
> CSI2RX_PAD_SINK,
> CSI2RX_PAD_SOURCE_STREAM0,
> @@ -71,6 +93,28 @@ struct csi2rx_fmt {
> u8 bpp;
> };
>
> +struct csi2rx_event {
> + u32 mask;
> + const char *name;
> +};
> +
> +static const struct csi2rx_event csi2rx_events[] = {
> + { CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 3 FIFO detected" },
> + { CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 2 FIFO detected" },
> + { CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 1 FIFO detected" },
> + { CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 0 FIFO detected" },
> + { CSI2RX_FRONT_TRUNC_HDR_IRQ, "A truncated header [short or long] has been received" },
> + { CSI2RX_PROT_TRUNCATED_PACKET_IRQ, "A truncated long packet has been received" },
> + { CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ, "A truncated long packet has been received. No payload" },
> + { CSI2RX_SP_INVALID_RCVD_IRQ, "A reserved or invalid short packet has been received" },
> + { CSI2RX_DATA_ID_IRQ, "Data ID error in the header packet" },
> + { CSI2RX_HEADER_CORRECTED_ECC_IRQ, "ECC error detected and corrected" },
> + { CSI2RX_HEADER_ECC_IRQ, "Unrecoverable ECC error" },
> + { CSI2RX_PAYLOAD_CRC_IRQ, "CRC error" },
> +};
> +
> +#define CSI2RX_NUM_EVENTS ARRAY_SIZE(csi2rx_events)
> +
> struct csi2rx_priv {
> struct device *dev;
> unsigned int count;
> @@ -95,6 +139,7 @@ struct csi2rx_priv {
> u8 max_lanes;
> u8 max_streams;
> bool has_internal_dphy;
> + u32 events[CSI2RX_NUM_EVENTS];
>
> struct v4l2_subdev subdev;
> struct v4l2_async_notifier notifier;
> @@ -124,6 +169,29 @@ static const struct csi2rx_fmt formats[] = {
> { .code = MEDIA_BUS_FMT_BGR888_1X24, .bpp = 24, },
> };
>
> +static void csi2rx_configure_err_irq_mask(void __iomem *base)
> +{
> + writel(CSI2RX_ERRORS, base + CSI2RX_ERROR_IRQS_MASK_REG);
> +}
> +
> +static irqreturn_t csi2rx_irq_handler(int irq, void *dev_id)
> +{
> + struct csi2rx_priv *csi2rx = dev_id;
> + int i;
> + u32 error_status;
> +
> + error_status = readl(csi2rx->base + CSI2RX_ERROR_IRQS_REG);
> +
> + for (i = 0; i < CSI2RX_NUM_EVENTS; i++)
> + if (error_status & csi2rx_events[i].mask)
> + csi2rx->events[i]++;
> +
> + writel(CSI2RX_ERRORS & error_status,
> + csi2rx->base + CSI2RX_ERROR_IRQS_REG);
> +
> + return IRQ_HANDLED;
> +}
> +
> static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> {
> unsigned int i;
> @@ -209,12 +277,26 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> unsigned int i;
> unsigned long lanes_used = 0;
> u32 reg;
> - int ret;
> + int irq, ret;
>
> ret = clk_prepare_enable(csi2rx->p_clk);
> if (ret)
> return ret;
>
> + irq = platform_get_irq_byname_optional(to_platform_device(csi2rx->dev), "error");
Why is this interrupt acquired everytime somebody starts the stream, as
opposed to once at probe-time?
> +
> + if (irq < 0) {
> + dev_warn(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
Given this is an optional interrupt, and different SoC vendors may or may not
integerate it, I don't think bothering the end-user with a warning everytime
is best. This could be dev_dbg.
> + } else {
> + csi2rx_configure_err_irq_mask(csi2rx->base);
> + ret = devm_request_irq(csi2rx->dev, irq, csi2rx_irq_handler, 0,
> + "csi2rx-irq", csi2rx);
> + if (ret) {
> + dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
> + return ret;
> + }
> + }
> +
> reset_control_deassert(csi2rx->p_rst);
> csi2rx_reset(csi2rx);
>
> @@ -361,6 +443,21 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> }
> }
>
> +static int csi2rx_log_status(struct v4l2_subdev *sd)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> + unsigned int i;
> +
> + for (i = 0; i < CSI2RX_NUM_EVENTS; i++) {
> + if (csi2rx->events[i])
> + dev_info(csi2rx->dev, "%s events: %d\n",
> + csi2rx_events[i].name,
> + csi2rx->events[i]);
> + }
> +
> + return 0;
> +}
> +
> static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> {
> struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> @@ -466,7 +563,12 @@ static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> .s_stream = csi2rx_s_stream,
> };
>
> +static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
> + .log_status = csi2rx_log_status,
> +};
> +
> static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
> + .core = &csi2rx_core_ops,
> .video = &csi2rx_video_ops,
> .pad = &csi2rx_pad_ops,
> };
> --
> 2.34.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/3] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS
2025-02-14 6:14 ` Jai Luthra
@ 2025-02-17 7:57 ` Yemike Abhilash Chandra
0 siblings, 0 replies; 13+ messages in thread
From: Yemike Abhilash Chandra @ 2025-02-17 7:57 UTC (permalink / raw)
To: Jai Luthra
Cc: linux-media, linux-kernel, devicetree, mripard, mchehab, robh,
krzk+dt, conor+dt, devarsht, vaishnav.a, r-donadkar, u-kumar1
Hi Jai,
Thank you for the review.
On 14/02/25 11:44, Jai Luthra wrote:
> Hi Abhilash,
>
> On Wed, Feb 12, 2025 at 06:42:43PM +0530, Yemike Abhilash Chandra wrote:
>> Enable the csi2rx_err_irq interrupt to record any errors during streaming
>> and also add support for VIDIOC_LOG_STATUS ioctl. The VIDIOC_LOG_STATUS
>> ioctl can be invoked from user space to retrieve the device status,
>> including details about any errors.
>>
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> ---
>> drivers/media/platform/cadence/cdns-csi2rx.c | 104 ++++++++++++++++++-
>> 1 file changed, 103 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
>> index 4d64df829e75..b3d76f0678fa 100644
>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
>> @@ -57,6 +57,28 @@
>> #define CSI2RX_LANES_MAX 4
>> #define CSI2RX_STREAMS_MAX 4
>>
>> +#define CSI2RX_ERROR_IRQS_REG 0x28
>> +#define CSI2RX_ERROR_IRQS_MASK_REG 0x2C
>> +
>> +#define CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ BIT(19)
>> +#define CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ BIT(18)
>> +#define CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ BIT(17)
>> +#define CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ BIT(16)
>> +#define CSI2RX_FRONT_TRUNC_HDR_IRQ BIT(12)
>> +#define CSI2RX_PROT_TRUNCATED_PACKET_IRQ BIT(11)
>> +#define CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ BIT(10)
>> +#define CSI2RX_SP_INVALID_RCVD_IRQ BIT(9)
>> +#define CSI2RX_DATA_ID_IRQ BIT(7)
>> +#define CSI2RX_HEADER_CORRECTED_ECC_IRQ BIT(6)
>> +#define CSI2RX_HEADER_ECC_IRQ BIT(5)
>> +#define CSI2RX_PAYLOAD_CRC_IRQ BIT(4)
>> +
>> +#define CSI2RX_ECC_ERRORS GENMASK(7, 4)
>> +#define CSI2RX_PACKET_ERRORS GENMASK(12, 9)
>> +#define CSI2RX_STREAM_ERRORS GENMASK(19, 16)
>> +#define CSI2RX_ERRORS (CSI2RX_ECC_ERRORS | CSI2RX_PACKET_ERRORS | \
>> + CSI2RX_STREAM_ERRORS)
>> +
>> enum csi2rx_pads {
>> CSI2RX_PAD_SINK,
>> CSI2RX_PAD_SOURCE_STREAM0,
>> @@ -71,6 +93,28 @@ struct csi2rx_fmt {
>> u8 bpp;
>> };
>>
>> +struct csi2rx_event {
>> + u32 mask;
>> + const char *name;
>> +};
>> +
>> +static const struct csi2rx_event csi2rx_events[] = {
>> + { CSI2RX_STREAM3_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 3 FIFO detected" },
>> + { CSI2RX_STREAM2_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 2 FIFO detected" },
>> + { CSI2RX_STREAM1_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 1 FIFO detected" },
>> + { CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ, "Overflow of the Stream 0 FIFO detected" },
>> + { CSI2RX_FRONT_TRUNC_HDR_IRQ, "A truncated header [short or long] has been received" },
>> + { CSI2RX_PROT_TRUNCATED_PACKET_IRQ, "A truncated long packet has been received" },
>> + { CSI2RX_FRONT_LP_NO_PAYLOAD_IRQ, "A truncated long packet has been received. No payload" },
>> + { CSI2RX_SP_INVALID_RCVD_IRQ, "A reserved or invalid short packet has been received" },
>> + { CSI2RX_DATA_ID_IRQ, "Data ID error in the header packet" },
>> + { CSI2RX_HEADER_CORRECTED_ECC_IRQ, "ECC error detected and corrected" },
>> + { CSI2RX_HEADER_ECC_IRQ, "Unrecoverable ECC error" },
>> + { CSI2RX_PAYLOAD_CRC_IRQ, "CRC error" },
>> +};
>> +
>> +#define CSI2RX_NUM_EVENTS ARRAY_SIZE(csi2rx_events)
>> +
>> struct csi2rx_priv {
>> struct device *dev;
>> unsigned int count;
>> @@ -95,6 +139,7 @@ struct csi2rx_priv {
>> u8 max_lanes;
>> u8 max_streams;
>> bool has_internal_dphy;
>> + u32 events[CSI2RX_NUM_EVENTS];
>>
>> struct v4l2_subdev subdev;
>> struct v4l2_async_notifier notifier;
>> @@ -124,6 +169,29 @@ static const struct csi2rx_fmt formats[] = {
>> { .code = MEDIA_BUS_FMT_BGR888_1X24, .bpp = 24, },
>> };
>>
>> +static void csi2rx_configure_err_irq_mask(void __iomem *base)
>> +{
>> + writel(CSI2RX_ERRORS, base + CSI2RX_ERROR_IRQS_MASK_REG);
>> +}
>> +
>> +static irqreturn_t csi2rx_irq_handler(int irq, void *dev_id)
>> +{
>> + struct csi2rx_priv *csi2rx = dev_id;
>> + int i;
>> + u32 error_status;
>> +
>> + error_status = readl(csi2rx->base + CSI2RX_ERROR_IRQS_REG);
>> +
>> + for (i = 0; i < CSI2RX_NUM_EVENTS; i++)
>> + if (error_status & csi2rx_events[i].mask)
>> + csi2rx->events[i]++;
>> +
>> + writel(CSI2RX_ERRORS & error_status,
>> + csi2rx->base + CSI2RX_ERROR_IRQS_REG);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
>> {
>> unsigned int i;
>> @@ -209,12 +277,26 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>> unsigned int i;
>> unsigned long lanes_used = 0;
>> u32 reg;
>> - int ret;
>> + int irq, ret;
>>
>> ret = clk_prepare_enable(csi2rx->p_clk);
>> if (ret)
>> return ret;
>>
>> + irq = platform_get_irq_byname_optional(to_platform_device(csi2rx->dev), "error");
>
> Why is this interrupt acquired everytime somebody starts the stream, as
> opposed to once at probe-time?
This was a mistake. Thanks for pointing this out.
In v2, I will correct this by acquiring the interrupt in the probe
function and the interrupt will only be enabled by writing to the
mask register in start_stream() and disabled in stop_stream().
>
>> +
>> + if (irq < 0) {
>> + dev_warn(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
>
> Given this is an optional interrupt, and different SoC vendors may or may not
> integerate it, I don't think bothering the end-user with a warning everytime
> is best. This could be dev_dbg.
Yes, understood. I will change dev_warn() to dev_dbg() in v2.
Thanks and Regards,
Yemike Abhilash Chandra
>
>> + } else {
>> + csi2rx_configure_err_irq_mask(csi2rx->base);
>> + ret = devm_request_irq(csi2rx->dev, irq, csi2rx_irq_handler, 0,
>> + "csi2rx-irq", csi2rx);
>> + if (ret) {
>> + dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> reset_control_deassert(csi2rx->p_rst);
>> csi2rx_reset(csi2rx);
>>
>> @@ -361,6 +443,21 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>> }
>> }
>>
>> +static int csi2rx_log_status(struct v4l2_subdev *sd)
>> +{
>> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
>> + unsigned int i;
>> +
>> + for (i = 0; i < CSI2RX_NUM_EVENTS; i++) {
>> + if (csi2rx->events[i])
>> + dev_info(csi2rx->dev, "%s events: %d\n",
>> + csi2rx_events[i].name,
>> + csi2rx->events[i]);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>> {
>> struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> @@ -466,7 +563,12 @@ static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
>> .s_stream = csi2rx_s_stream,
>> };
>>
>> +static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
>> + .log_status = csi2rx_log_status,
>> +};
>> +
>> static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
>> + .core = &csi2rx_core_ops,
>> .video = &csi2rx_video_ops,
>> .pad = &csi2rx_pad_ops,
>> };
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 3/3] media: ti: j721e-csi2rx: Add support for VIDIOC_LOG_STATUS
2025-02-13 13:26 ` Jai Luthra
@ 2025-02-17 8:07 ` Yemike Abhilash Chandra
0 siblings, 0 replies; 13+ messages in thread
From: Yemike Abhilash Chandra @ 2025-02-17 8:07 UTC (permalink / raw)
To: Jai Luthra
Cc: linux-media, linux-kernel, devicetree, mripard, mchehab, robh,
krzk+dt, conor+dt, devarsht, vaishnav.a, r-donadkar, u-kumar1
Hi Jai,
Thank you for the review.
On 13/02/25 18:56, Jai Luthra wrote:
> Hi Abhilash,
>
> On Wed, Feb 12, 2025 at 06:42:44PM +0530, Yemike Abhilash Chandra wrote:
>> The VIDIOC_LOG_STATUS ioctl outputs the current status of a device to the
>> kernel log. When this ioctl is called on a video device, the current
>> implementation queries the log status for all connected subdevices in the
>> media pipeline.
>>
>
> What is the benefit of doing this for a video node? The user can directly
> check the status on the cdns-csi2rx subdev for CSI errors.
>
> As far as I understand, the video node corresponds to a particular stream, but
> the cdns-csi2rx source pad is shared for all video nodes, so it will report
> the total errors seen for all video nodes in multi-camera scenarios.
>
> This approach will also give you v4l2 control handler status from a few
> sensors (like OV5640) that implement the ioctl using
> v4l2_ctrl_subdev_log_status(), which is probably just noise for the case where
> a user wants to check for stream errors.
I understand that this change does not add any value and may introduce
unnecessary noise. Given that the user can directly check the status on the
cdns-csi2rx subdevice for stream errors, I will remove this change in v2.
Thanks and Regards,
Yemike Abhilash Chandra
>
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> ---
>> drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> index 6412a00be8ea..946704458fee 100644
>> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> @@ -377,6 +377,15 @@ static int ti_csi2rx_enum_framesizes(struct file *file, void *fh,
>> return 0;
>> }
>>
>> +static int ti_csi2rx_log_status(struct file *file, void *fh)
>> +{
>> + struct ti_csi2rx_dev *csi = video_drvdata(file);
>> +
>> + v4l2_device_call_all(&csi->v4l2_dev, 0, core, log_status);
>> +
>> + return 0;
>> +}
>> +
>> static const struct v4l2_ioctl_ops csi_ioctl_ops = {
>> .vidioc_querycap = ti_csi2rx_querycap,
>> .vidioc_enum_fmt_vid_cap = ti_csi2rx_enum_fmt_vid_cap,
>> @@ -393,6 +402,7 @@ static const struct v4l2_ioctl_ops csi_ioctl_ops = {
>> .vidioc_expbuf = vb2_ioctl_expbuf,
>> .vidioc_streamon = vb2_ioctl_streamon,
>> .vidioc_streamoff = vb2_ioctl_streamoff,
>> + .vidioc_log_status = ti_csi2rx_log_status,
>> };
>>
>> static const struct v4l2_file_operations csi_fops = {
>> --
>> 2.34.1
>>
>
> Thanks,
> Jai
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-02-17 8:08 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-12 13:12 [RFC PATCH 0/3] Enable support for error detection in CSI2RX Yemike Abhilash Chandra
2025-02-12 13:12 ` [RFC PATCH 1/3] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx Yemike Abhilash Chandra
2025-02-12 19:28 ` Krzysztof Kozlowski
2025-02-13 7:16 ` Yemike Abhilash Chandra
2025-02-13 7:36 ` Krzysztof Kozlowski
2025-02-12 13:12 ` [RFC PATCH 2/3] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS Yemike Abhilash Chandra
2025-02-14 6:14 ` Jai Luthra
2025-02-17 7:57 ` Yemike Abhilash Chandra
2025-02-12 13:12 ` [RFC PATCH 3/3] media: ti: j721e-csi2rx: Add " Yemike Abhilash Chandra
2025-02-13 13:26 ` Jai Luthra
2025-02-17 8:07 ` Yemike Abhilash Chandra
2025-02-12 19:16 ` [RFC PATCH 0/3] Enable support for error detection in CSI2RX Conor Dooley
2025-02-13 7:10 ` Yemike Abhilash Chandra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.