Re: [PATCH 18/28] media: ti-vpe: cal: add 'use_pix_proc' field

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

Hi Tomi,

On Mon, Apr 19, 2021 at 02:53:56PM +0300, Tomi Valkeinen wrote:
> On 18/04/2021 16:00, Laurent Pinchart wrote:
> > On Mon, Apr 12, 2021 at 02:34:47PM +0300, Tomi Valkeinen wrote:
> >> We already have functions to reserve and release a pix proc unit, but we
> >> always reserve such and the code expects the pix proc unit to be used.
> >>
> >> Add a new field, 'use_pix_proc', to indicate if the pix prox unit has
> >> been reserved and should be used. Use the flag to skip programming pix
> >> proc unit when not needed.
> >>
> >> Note that we still always set the use_pix_proc flag to true.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   drivers/media/platform/ti-vpe/cal.c | 10 +++++++---
> >>   drivers/media/platform/ti-vpe/cal.h |  2 ++
> >>   2 files changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> >> index e397f59d3bbc..a1d173bd4613 100644
> >> --- a/drivers/media/platform/ti-vpe/cal.c
> >> +++ b/drivers/media/platform/ti-vpe/cal.c
> >> @@ -473,13 +473,15 @@ int cal_ctx_prepare(struct cal_ctx *ctx)
> >>   	}
> >>   
> >>   	ctx->pix_proc = ret;
> >> +	ctx->use_pix_proc = true;
> >>   
> >>   	return 0;
> >>   }
> >>   
> >>   void cal_ctx_unprepare(struct cal_ctx *ctx)
> >>   {
> >> -	cal_release_pix_proc(ctx->cal, ctx->pix_proc);
> >> +	if (ctx->use_pix_proc)
> >> +		cal_release_pix_proc(ctx->cal, ctx->pix_proc);
> >>   }
> >>   
> >>   void cal_ctx_start(struct cal_ctx *ctx)
> >> @@ -489,7 +491,8 @@ void cal_ctx_start(struct cal_ctx *ctx)
> >>   
> >>   	/* Configure the CSI-2, pixel processing and write DMA contexts. */
> >>   	cal_ctx_csi2_config(ctx);
> >> -	cal_ctx_pix_proc_config(ctx);
> >> +	if (ctx->use_pix_proc)
> >> +		cal_ctx_pix_proc_config(ctx);
> >>   	cal_ctx_wr_dma_config(ctx);
> >>   
> >>   	/* Enable IRQ_WDMA_END and IRQ_WDMA_START. */
> >> @@ -530,7 +533,8 @@ void cal_ctx_stop(struct cal_ctx *ctx)
> >>   	cal_write(ctx->cal, CAL_CSI2_CTX(ctx->phy->instance, ctx->ppi_ctx), 0);
> >>   
> >>   	/* Disable pix proc */
> >> -	cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), 0);
> >> +	if (ctx->use_pix_proc)
> >> +		cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), 0);
> >>   }
> >>   
> >>   /* ------------------------------------------------------------------
> >> diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h
> >> index 01e05e46e48d..409b7276a1fa 100644
> >> --- a/drivers/media/platform/ti-vpe/cal.h
> >> +++ b/drivers/media/platform/ti-vpe/cal.h
> >> @@ -223,6 +223,8 @@ struct cal_ctx {
> >>   	u8			cport;
> >>   	u8			ppi_ctx;
> >>   	u8			pix_proc;
> >> +
> >> +	bool use_pix_proc;
> > 
> > Indentation ?
> > 
> > How about turning pix_proc to a signed value, and using -1 to indicate
> > it's not used ?
> 
> I considered that. But then, instead of "if (ctx->use_pix_proc)" I would 
> have "if (ctx->use_pix_proc >= 0)". I think the former is better. And 
> having a non-zero default value always makes me a bit uneasy.

I personally feel more uneasy when having a bool that qualifies whether
another field is valid or not :-) Probably because I then never know
what to set the other field to when setting the bool to false. It could
just be me, it doesn't matter much.

-- 
Regards,

Laurent Pinchart