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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aBPvnkdakh9AtAjv@kekkonen.localdomain>
Date: Thu, 1 May 2025 22:03:10 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Michael Riesch <michael.riesch@...labora.com>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Théo Lebrun <theo.lebrun@...tlin.com>,
	Gerald Loacker <gerald.loacker@...fvision.net>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Kever Yang <kever.yang@...k-chips.com>,
	Nicolas Dufresne <nicolas.dufresne@...labora.com>,
	Sebastian Fricke <sebastian.fricke@...labora.com>,
	Sebastian Reichel <sebastian.reichel@...labora.com>,
	Paul Kocialkowski <paulk@...-base.io>,
	Alexander Shiyan <eagle.alexander923@...il.com>,
	Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org,
	Mehdi Djait <mehdi.djait@...tlin.com>
Subject: Re: [PATCH v5 05/11] media: rockchip: add a driver for the rockchip
 camera interface

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