Re: [PATCH 14/28] media: ti-vpe: cal: catch VC errors

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

On Mon, Apr 19, 2021 at 02:19:01PM +0300, Tomi Valkeinen wrote:
> On 18/04/2021 15:38, Laurent Pinchart wrote:
> > 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@ideasonboard.com>
> >> ---
> >>   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.

It then all depends on your messiness gauge :-) For the code above,

 	cal_write(phy->cal, CAL_HL_IRQENABLE_SET(0),
		  CAL_HL_IRQ_CIO_MASK(phy->instance) |
		  CAL_HL_IRQ_VC_MASK(phy->instance));

doesn't seem messy at all to me (quite the contrary actually).

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

That would be messy, and I think it should either stay as-is, or the
function should be refactored with code broken out in multiple functions
(probably overkill for this specific example).

The next line, however, I would have written as

				dev_err_ratelimited(cal->dev,
						    "CIO%u VC error: %#08x\n",
						    i, vc_stat);

> 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? =)

So all we need is a metric that measure code mess, right ? :-)

-- 
Regards,

Laurent Pinchart