[PATCH v5 05/11] media: rockchip: add a driver for the rockchip camera interface

[Sakari Ailus]

Hi Michael,

On Tue, Apr 29, 2025 at 06:08:51PM +0200, Michael Riesch wrote:
> Hi Sakari,
> 
> Thanks for your review.
> 
> On 4/28/25 17:41, Sakari Ailus wrote:
> > Hi Michael,
> > 
> > On Thu, Mar 06, 2025 at 05:56:06PM +0100, Michael Riesch wrote:
> >> [...]
> >> +
> >> +const struct rkcif_dvp_match_data px30_vip_dvp_match_data = {
> > 
> > Can you prefix this with e.g. "rk_"? It's not static...
> 
> Will do. "rkcif_" is the prefix for the other global symbols, so I'll
> use that here as well.

Ack.

> 
> > [...]
> >> +const struct rkcif_dvp_match_data rk3568_vicap_dvp_match_data = {
> >> +	.in_fmts = rk3568_dvp_in_fmts,
> >> +	.in_fmts_num = ARRAY_SIZE(rk3568_dvp_in_fmts),
> >> +	.out_fmts = dvp_out_fmts,
> >> +	.out_fmts_num = ARRAY_SIZE(dvp_out_fmts),
> >> +	.setup = rk3568_dvp_grf_setup,
> >> +	.has_scaler = false,
> >> +	.regs = {
> >> +		[RKCIF_DVP_CTRL] = 0x00,
> >> +		[RKCIF_DVP_INTEN] = 0x04,
> >> +		[RKCIF_DVP_INTSTAT] = 0x08,
> >> +		[RKCIF_DVP_FOR] = 0x0c,
> >> +		[RKCIF_DVP_LINE_NUM_ADDR] = 0x2c,
> >> +		[RKCIF_DVP_FRM0_ADDR_Y] = 0x14,
> >> +		[RKCIF_DVP_FRM0_ADDR_UV] = 0x18,
> >> +		[RKCIF_DVP_FRM1_ADDR_Y] = 0x1c,
> >> +		[RKCIF_DVP_FRM1_ADDR_UV] = 0x20,
> >> +		[RKCIF_DVP_VIR_LINE_WIDTH] = 0x24,
> >> +		[RKCIF_DVP_SET_SIZE] = 0x28,
> >> +		[RKCIF_DVP_CROP] = 0x34,
> >> +		[RKCIF_DVP_FRAME_STATUS] = 0x3c,
> >> +		[RKCIF_DVP_LAST_LINE] = 0x44,
> >> +		[RKCIF_DVP_LAST_PIX] = 0x48,
> >> +	},
> >> +};
> > 
> > Does this belong here? Or on the user's side?
> 
> Not sure I understand your question. The *_dvp_match_data structs are
> mostly used by rkcif-capture-dvp.c, which would mean that they belong
> here. rkcif-dev.c stores the pointer to it, but I think it can be
> considered an opaque data structure.

Ack.

> 
> > 
> >> [...]
> >> +err_streams_unregister:
> >> +	for (; i >= 0; i--)
> > 
> > You can
> 
> ... use
> 
>    for (i++; i--; )
> 
> as you pointed out in your other mail? I would rather not, actually. I
> think this would require using "i - 1"...
> 
> > 
> >> +		rkcif_stream_unregister(&interface->streams[i]);
> 
> ... here:
> 
> rkcif_stream_unregister(&interface->streams[i - 1]);
> 
> which I find less readable. Or I am wrong, but then I did not understand
> the for loop definition above, which would mean that it is less readable
> than my version.

Would it? The values inside the loop are the same, it's just the loop that
is different and does not require a signed counter variable.

> 
> OK for you if I leave this as is?

Yours might be slightly easier to grasp but making the variable signed just
for that is not very pretty.

> 
> Unfortunately, your mail seems to be cut off here. Any further comments
> to the part below?

No, that was all for now.

-- 
Regards,

Sakari Ailus