On Mon, Feb 24, 2025 at 12:45:46PM +0530, Jai Luthra wrote:
> Hi Abhilash,
>
> Thanks for the patch.
>
> On Fri, Feb 21, 2025 at 05:33:37PM +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@xxxxxx>
> > ---
> >
> > Changes in v3:
> > - Address Jai's review comment to enable FIFO overflow bits in the mask
> > only for the source pads that have an active remote.
> > - Drop TI-specific interrupt and have support for only two interrupts
> > that are common across all vendors.
> > - Address Changhuang's review to use pdev directly to get the interrupt.
> > - Set the interrupt mask register only if the interrupt is defined in the DT.
> >
> >
> > drivers/media/platform/cadence/cdns-csi2rx.c | 125 +++++++++++++++++++
> > 1 file changed, 125 insertions(+)
> >
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index cebcae196eec..81375f11a32f 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -57,6 +57,25 @@
> > #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)
> > +
> > enum csi2rx_pads {
> > CSI2RX_PAD_SINK,
> > CSI2RX_PAD_SOURCE_STREAM0,
> > @@ -71,9 +90,32 @@ 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;
> > + int error_irq;
> >
> > /*
> > * Used to prevent race conditions between multiple,
> > @@ -95,6 +137,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 +167,50 @@ static const struct csi2rx_fmt formats[] = {
> > { .code = MEDIA_BUS_FMT_BGR888_1X24, .bpp = 24, },
> > };
> >
> > +static void csi2rx_configure_error_irq_mask(void __iomem *base, struct csi2rx_priv *csi2rx)
I think this can easily be split into the next line without making it harder
to read. Please configure your editor with max line length setting.
You can also run your patches through checkpatch with --max-line-length=80
> > +{
> > + u32 error_irq_mask = 0;
> > +
> > + error_irq_mask |= CSI2RX_ECC_ERRORS;
> > + error_irq_mask |= CSI2RX_PACKET_ERRORS;
> > +
> > + /*
> > + * iterate through all source pads and check if they are linked
>
> nit: s/iterate/Iterate
>
> > + * to an active remote pad. If an active remote pad is found,
> > + * calculate the corresponding bit position and set it in
> > + * mask, enabling the stream overflow error in the mask.
> > + */
> > +
>
> nit: drop this extra whitespace
>
> > + for (int i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) {
> > + struct media_pad *remote_pad = media_pad_remote_pad_first(&csi2rx->pads[i]);
Same for this line.
> > +
> > + if (remote_pad) {
> > + int bit_position = 16 + (i - CSI2RX_PAD_SOURCE_STREAM0);
>
> It would be cleaner to not use the magic number of 16 here, instead using the
> already defined macro:
>
> error_irq_mask |= (CSI2RX_STREAM0_FIFO_OVERFLOW_IRQ
> << (i - CSI2RX_PAD_SOURCE_STREAM0));
>
> > +
> > + error_irq_mask |= (1 << bit_position);
> > + }
> > + }
> > +
> > + writel(error_irq_mask, 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(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;
> > @@ -220,6 +307,9 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> > reset_control_deassert(csi2rx->p_rst);
> > csi2rx_reset(csi2rx);
> >
> > + if (csi2rx->error_irq >= 0)
> > + csi2rx_configure_error_irq_mask(csi2rx->base, csi2rx);
> > +
> > reg = csi2rx->num_lanes << 8;
> > for (i = 0; i < csi2rx->num_lanes; i++) {
> > reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);
> > @@ -332,6 +422,8 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> > reset_control_assert(csi2rx->sys_rst);
> > clk_disable_unprepare(csi2rx->sys_clk);
> >
> > + writel(0, csi2rx->base + CSI2RX_ERROR_IRQS_MASK_REG);
> > +
> > for (i = 0; i < csi2rx->max_streams; i++) {
> > writel(CSI2RX_STREAM_CTRL_STOP,
> > csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> > @@ -363,6 +455,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);
> > @@ -468,7 +575,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,
> > };
> > @@ -705,6 +817,19 @@ static int csi2rx_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_cleanup;
> >
> > + csi2rx->error_irq = platform_get_irq_byname_optional(pdev, "error_irq");
> > +
> > + if (csi2rx->error_irq < 0) {
> > + dev_dbg(csi2rx->dev, "Optional interrupt not defined, proceeding without it\n");
> > + } else {
> > + ret = devm_request_irq(csi2rx->dev, csi2rx->error_irq, csi2rx_irq_handler, 0,
> > + "csi2rx-error-irq", csi2rx);
>
> Why "csi2rx-error-irq" is passed in the devname argument instead of the device
> name `dev_name(&pdev->dev)`? Also there is an alignment issue:
>
> CHECK: Alignment should match open parenthesis
> #204: FILE: drivers/media/platform/cadence/cdns-csi2rx.c:824:
> + ret = devm_request_irq(csi2rx->dev, csi2rx->error_irq, csi2rx_irq_handler, 0,
> + "csi2rx-error-irq", csi2rx);
>
> > + if (ret) {
> > + dev_err(csi2rx->dev, "Unable to request interrupt: %d\n", ret);
> > + return ret;
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/71558443/viewer#L2317
This should not return directly, but instead cleanup the acquired resources
using "goto err_cleanup".
> > + }
> > + }
> > +
> > ret = v4l2_subdev_init_finalize(&csi2rx->subdev);
> > if (ret)
> > goto err_cleanup;
> > --
> > 2.34.1
> >
Thanks,
Jai
Attachment:
signature.asc
Description: PGP signature
