Tomi,

Thanks for the review.

Tomi Valkeinen tomi.valkeinen@ti.com wrote on Wed [2020-Mar-04 10:49:55 +0200]:

On 02/03/2020 15:56, Benoit Parrot wrote:

...

disable_irqs() was mistakenly disabling all interrupts when called. This cause all port stream to stop even if only stopping one of them.

Cc: stable stable@vger.kernel.org Signed-off-by: Benoit Parrot bparrot@ti.com

---

drivers/media/platform/ti-vpe/cal.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index 6e009e479be3..6d4cbb8782ed 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -722,16 +722,16 @@ static void enable_irqs(struct cal_ctx *ctx) static void disable_irqs(struct cal_ctx *ctx) {

• u32 val;
• /* Disable IRQ_WDMA_END 0/1 */

• reg_write_field(ctx->dev,

• 	CAL_HL_IRQENABLE_CLR(2),
  

• 	CAL_HL_IRQ_CLEAR,
  

• 	CAL_HL_IRQ_MASK(ctx->csi2_port));
  

• val = 0;
• set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
• reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), val); /* Disable IRQ_WDMA_START 0/1 */

• reg_write_field(ctx->dev,

• 	CAL_HL_IRQENABLE_CLR(3),
  

• 	CAL_HL_IRQ_CLEAR,
  

• 	CAL_HL_IRQ_MASK(ctx->csi2_port));
  

• val = 0;
• set_field(&val, CAL_HL_IRQ_CLEAR, CAL_HL_IRQ_MASK(ctx->csi2_port));
• reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(3), val); /* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */ reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0); }

I think the above works. But the enable_irqs is broken too, even if it doesn't cause any issues. Both IRQ_SET and IRQ_CLR are not supposed to be "modified" by a read-mod-write operation, but just written to.

Well maybe not consistent now, that disable_irqs has been modified but not broken per se.

The macros used also make the code very difficult to read. Something like this fixes both irq enable and disable, and makes it readable:

Ah but the mask macro are all consistent with each other, whether that create one too many level of abstraction is a different subject. This setup was oroginally requested by the maintainer. So I'll "fix" enable_irqs also but I'll keep the macro as is.

Benoit

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index c8b1290c9e2b..660653031a0b 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -707,15 +707,9 @@ static void cal_quickdump_regs(struct cal_dev *dev) static void enable_irqs(struct cal_ctx *ctx) { /* Enable IRQ_WDMA_END 0/1 */

• reg_write_field(ctx->dev,

• 	CAL_HL_IRQENABLE_SET(2),
  

• 	CAL_HL_IRQ_ENABLE,
  

• 	CAL_HL_IRQ_MASK(ctx->csi2_port));
  

• reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(2), 1 << (ctx->csi2_port - 1)); /* Enable IRQ_WDMA_START 0/1 */

• reg_write_field(ctx->dev,

• 	CAL_HL_IRQENABLE_SET(3),
  

• 	CAL_HL_IRQ_ENABLE,
  

• 	CAL_HL_IRQ_MASK(ctx->csi2_port));
  

• reg_write(ctx->dev, CAL_HL_IRQENABLE_SET(3), 1 << (ctx->csi2_port - 1)); /* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */ reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0xFF000000);

} @@ -723,15 +717,9 @@ static void enable_irqs(struct cal_ctx *ctx) static void disable_irqs(struct cal_ctx *ctx) { /* Disable IRQ_WDMA_END 0/1 */

• reg_write_field(ctx->dev,

• 	CAL_HL_IRQENABLE_CLR(2),
  

• 	CAL_HL_IRQ_CLEAR,
  

• 	CAL_HL_IRQ_MASK(ctx->csi2_port));
  

• reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(2), 1 << (ctx->csi2_port - 1)); /* Disable IRQ_WDMA_START 0/1 */

• reg_write_field(ctx->dev,

• 	CAL_HL_IRQENABLE_CLR(3),
  

• 	CAL_HL_IRQ_CLEAR,
  

• 	CAL_HL_IRQ_MASK(ctx->csi2_port));
  

• reg_write(ctx->dev, CAL_HL_IRQENABLE_CLR(3), 1 << (ctx->csi2_port - 1)); /* Todo: Add VC_IRQ and CSI2_COMPLEXIO_IRQ handling */ reg_write(ctx->dev, CAL_CSI2_VC_IRQENABLE(1), 0);

}

Tomi

-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki