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

Sakari Ailus
2025-05-01T22:05:31Z

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