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@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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
