On 18/04/2021 15:38, Laurent Pinchart wrote:
Hi Tomi,
Thank you for the patch.
On Mon, Apr 12, 2021 at 02:34:43PM +0300, Tomi Valkeinen wrote:
CAL driver currently ignores VC related errors. To help catch error
conditions, enable all the VC error interrupts and handle them in the
interrupt handler by printing an error.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
drivers/media/platform/ti-vpe/cal-camerarx.c | 23 ++++++++++++++++----
drivers/media/platform/ti-vpe/cal.c | 9 ++++++++
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c
index 974fcbb19547..0354f311c5d2 100644
--- a/drivers/media/platform/ti-vpe/cal-camerarx.c
+++ b/drivers/media/platform/ti-vpe/cal-camerarx.c
@@ -226,24 +226,39 @@ static void cal_camerarx_enable_irqs(struct cal_camerarx *phy)
CAL_CSI2_COMPLEXIO_IRQ_FIFO_OVR_MASK |
CAL_CSI2_COMPLEXIO_IRQ_SHORT_PACKET_MASK |
CAL_CSI2_COMPLEXIO_IRQ_ECC_NO_CORRECTION_MASK;
-
- /* Enable CIO error IRQs. */
+ const u32 vc_err_mask =
+ CAL_CSI2_VC_IRQ_CS_IRQ_MASK(0) |
+ CAL_CSI2_VC_IRQ_CS_IRQ_MASK(1) |
+ CAL_CSI2_VC_IRQ_CS_IRQ_MASK(2) |
+ CAL_CSI2_VC_IRQ_CS_IRQ_MASK(3) |
+ CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(0) |
+ CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(1) |
+ CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(2) |
+ CAL_CSI2_VC_IRQ_ECC_CORRECTION_IRQ_MASK(3);
+
+ /* Enable CIO & VC error IRQs. */
cal_write(phy->cal, CAL_HL_IRQENABLE_SET(0),
- CAL_HL_IRQ_CIO_MASK(phy->instance));
+ CAL_HL_IRQ_CIO_MASK(phy->instance) | CAL_HL_IRQ_VC_MASK(phy->instance));
Line wrap ? Same in multiple places below. I know there's no strict 80
columns limit anymore, but I don't think longer lines help with
readability in this patch (not to mention the coding style inconsistency
with the rest of the driver).
Well, I disagree, but I guess that's in the eye of the beholder.
If we have a coding style with strict 80 column limit, then there are
other places I need to start fixing too. My personal coding style is
such that around 80 columns I start thinking about splitting if it can
be done without any messiness, around 100 I seriously try to split it,
and around 120 I think it's broken.
I can change this and the other similar line, the end result is only
slightly messier, but...
+
+ if (status & CAL_HL_IRQ_VC_MASK(i)) {
+ u32 vc_stat = cal_read(cal, CAL_CSI2_VC_IRQSTATUS(i));
+
+ dev_err_ratelimited(cal->dev,
+ "CIO%u VC error: %#08x\n", i, vc_stat);
+
+ cal_write(cal, CAL_CSI2_VC_IRQSTATUS(i), vc_stat);
+ }
...especially for this part sticking to 80 columns uglifies the code.
u32 vc_stat =
cal_read(cal,
CAL_CSI2_VC_IRQSTATUS(i));
or
u32 cio_stat = cal_read(cal,
CAL_CSI2_COMPLEXIO_IRQSTATUS(i));
I could split parts to a separate function, but I don't think the end
result would be better.
I think we discuss the 80-column problem almost in every series. Maybe
we need to agree to some clear predefined rules to avoid future
discussions about this? =)
Tomi
