Re: [PATCH 10/28] media: ti-vpe: cal: Add pixel processing context

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 18/04/2021 15:20, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Mon, Apr 12, 2021 at 02:34:39PM +0300, Tomi Valkeinen wrote:
> > CAL has 4 pixel processing contexts (PIX PROC) of which the driver
> > currently uses pix proc 0 for PHY0, and pix proc 1 for PHY1 (as the
> > driver supports only a single source per PHY).
> >
> > Add a pix_proc field to cal_ctx to allow us to use different pix proc
> > contexts in the future
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/media/platform/ti-vpe/cal.c | 9 +++++----
> >   drivers/media/platform/ti-vpe/cal.h | 1 +
> >   2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index c550eeb27e79..3dc83c66fd96 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -355,16 +355,16 @@ static void cal_ctx_pix_proc_config(struct cal_ctx *ctx)
> >   		break;
> >   	}
> >   
> > -	val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->index));
> > +	val = cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc));
> >   	cal_set_field(&val, extract, CAL_PIX_PROC_EXTRACT_MASK);
> >   	cal_set_field(&val, CAL_PIX_PROC_DPCMD_BYPASS, CAL_PIX_PROC_DPCMD_MASK);
> >   	cal_set_field(&val, CAL_PIX_PROC_DPCME_BYPASS, CAL_PIX_PROC_DPCME_MASK);
> >   	cal_set_field(&val, pack, CAL_PIX_PROC_PACK_MASK);
> >   	cal_set_field(&val, ctx->cport, CAL_PIX_PROC_CPORT_MASK);
> >   	cal_set_field(&val, 1, CAL_PIX_PROC_EN_MASK);
> > -	cal_write(ctx->cal, CAL_PIX_PROC(ctx->index), val);
> > -	ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->index,
> > -		cal_read(ctx->cal, CAL_PIX_PROC(ctx->index)));
> > +	cal_write(ctx->cal, CAL_PIX_PROC(ctx->pix_proc), val);
> > +	ctx_dbg(3, ctx, "CAL_PIX_PROC(%d) = 0x%08x\n", ctx->pix_proc,
>
> While at it, you could s/%d/%u/
>
> > +		cal_read(ctx->cal, CAL_PIX_PROC(ctx->pix_proc)));
>
> And should we use val here instead of reading the value back ?

I think it's usually good to read the value from the register. It'll 
show what the register value really is, not what we thought we wrote. 
Usually those two should be the same, but in some cases there are 
read-only fields or non-writeable areas.

That said, I'm not sure if any of the registers printed in cal.c have 
any of those fields, so... I don't feel strongly either way, but I also 
don't see benefit in doing the change (verifying there are no read-only 
fields, and fixing the conflicts).

I can change this and the other similar cases if you have a reason why 
it's better =).

 Tomi