| Message ID | 20210412113457.328012-15-tomi.valkeinen@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ti-vpe: cal: prepare for multistream support | expand |
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@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). Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > cal_write(phy->cal, CAL_CSI2_COMPLEXIO_IRQENABLE(phy->instance), > cio_err_mask); > + cal_write(phy->cal, CAL_CSI2_VC_IRQENABLE(phy->instance), > + vc_err_mask); > } > > static void cal_camerarx_disable_irqs(struct cal_camerarx *phy) > { > /* Disable CIO error irqs */ > cal_write(phy->cal, CAL_HL_IRQENABLE_CLR(0), > - CAL_HL_IRQ_CIO_MASK(phy->instance)); > + CAL_HL_IRQ_CIO_MASK(phy->instance) | CAL_HL_IRQ_VC_MASK(phy->instance)); > cal_write(phy->cal, CAL_CSI2_COMPLEXIO_IRQENABLE(phy->instance), 0); > + cal_write(phy->cal, CAL_CSI2_VC_IRQENABLE(phy->instance), 0); > } > > static void cal_camerarx_ppi_enable(struct cal_camerarx *phy) > { > + cal_write_field(phy->cal, CAL_CSI2_PPI_CTRL(phy->instance), > + 1, CAL_CSI2_PPI_CTRL_ECC_EN_MASK); > + > cal_write_field(phy->cal, CAL_CSI2_PPI_CTRL(phy->instance), > 1, CAL_CSI2_PPI_CTRL_IF_EN_MASK); > } > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 0abcc83841c6..092041ddbcfb 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -577,6 +577,15 @@ static irqreturn_t cal_irq(int irq_cal, void *data) > cal_write(cal, CAL_CSI2_COMPLEXIO_IRQSTATUS(i), > cio_stat); > } > + > + 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); > + } > } > } >
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@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. 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
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 ? :-)
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)); cal_write(phy->cal, CAL_CSI2_COMPLEXIO_IRQENABLE(phy->instance), cio_err_mask); + cal_write(phy->cal, CAL_CSI2_VC_IRQENABLE(phy->instance), + vc_err_mask); } static void cal_camerarx_disable_irqs(struct cal_camerarx *phy) { /* Disable CIO error irqs */ cal_write(phy->cal, CAL_HL_IRQENABLE_CLR(0), - CAL_HL_IRQ_CIO_MASK(phy->instance)); + CAL_HL_IRQ_CIO_MASK(phy->instance) | CAL_HL_IRQ_VC_MASK(phy->instance)); cal_write(phy->cal, CAL_CSI2_COMPLEXIO_IRQENABLE(phy->instance), 0); + cal_write(phy->cal, CAL_CSI2_VC_IRQENABLE(phy->instance), 0); } static void cal_camerarx_ppi_enable(struct cal_camerarx *phy) { + cal_write_field(phy->cal, CAL_CSI2_PPI_CTRL(phy->instance), + 1, CAL_CSI2_PPI_CTRL_ECC_EN_MASK); + cal_write_field(phy->cal, CAL_CSI2_PPI_CTRL(phy->instance), 1, CAL_CSI2_PPI_CTRL_IF_EN_MASK); } diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index 0abcc83841c6..092041ddbcfb 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -577,6 +577,15 @@ static irqreturn_t cal_irq(int irq_cal, void *data) cal_write(cal, CAL_CSI2_COMPLEXIO_IRQSTATUS(i), cio_stat); } + + 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); + } } }
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(-)