Re: [PATCH] media: imx283: drop CENTERED_RECTANGLE due to clang failure

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Thu, 13 Jun 2024 16:26:43 +0100
Kieran Bingham <kieran.bingham@ideasonboard.com> escreveu:

> Quoting Hans Verkuil (2024-06-13 16:19:08)
> > The CENTERED_RECTANGLE define fails to compile on clang and old gcc
> > versions. Just drop it and fill in the crop rectangles explicitly.  
> 
> So back when I was playing around with this I thought it would have got
> dropped during review / upstreaming before now - because I couldn't find
> a way to make sure the macro args were guaranteed to be used only once,
> by putting some locals in the macro (because of the initialisation).
> 
> So I'm not surprised that it needs to be removed, but I am surprised
> that it wasn't for the reason I expected ;-)
> 
> Anyway - maybe later I'll experiement with more common helpers perhaps -
> but not if it hits compile errors..

IMO, a helper just makes it worse for humans. I mean, just looking at:

	.crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),

I can't tell what values for top/left would be used.

> I do recall using v4l2_rects to define the active area so they could be
> used collectively rather than initialising things as
> 	.width = WIDTH,
> 	.height = HEIGHT,

Using defines for the minimum/maximum visible area makes sense,
e. g. something similar to this:

               .crop = {
                      .top = MIN_VISIBLE_TOP,
                      .left = MIN_VISIBLE_LEFT,
                      .width = MAX_WIDTH,
                      .height = MAX_HEIGHT,
               },

would also be fine, as it would be clear that the crop region is
the one containing the hardware limits.

> So - perhaps this could be (if it compiles):
>	.crop = imx283_active_area,

This should equally work, but maybe you could do, instead:

	.crop = &imx283_active_area,	// e.g. using a pointer

to avoid duplicating for every supported mode.

Thanks,
Mauro