Re: [RFC PATCH 2/3] media: cadence: csi2rx: Enable csi2rx_err_irq interrupt and add support for VIDIOC_LOG_STATUS

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

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
>>