| Message ID | ff7a05e2-2908-4da0-817a-1d7c271e788a@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | media: imx283: drop CENTERED_RECTANGLE due to clang failure | expand |
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
diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c index 6428eb5394a9..8490618c5071 100644 --- a/drivers/media/i2c/imx283.c +++ b/drivers/media/i2c/imx283.c @@ -407,14 +407,6 @@ static const struct imx283_reg_list link_freq_reglist[] = { }, }; -#define CENTERED_RECTANGLE(rect, _width, _height) \ - { \ - .left = rect.left + ((rect.width - (_width)) / 2), \ - .top = rect.top + ((rect.height - (_height)) / 2), \ - .width = (_width), \ - .height = (_height), \ - } - /* Mode configs */ static const struct imx283_mode supported_modes_12bit[] = { { @@ -440,7 +432,12 @@ static const struct imx283_mode supported_modes_12bit[] = { .min_shr = 11, .horizontal_ob = 96, .vertical_ob = 16, - .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648), + .crop = { + .top = 40, + .left = 108, + .width = 5472, + .height = 3648, + }, }, { /* @@ -468,7 +465,12 @@ static const struct imx283_mode supported_modes_12bit[] = { .horizontal_ob = 48, .vertical_ob = 4, - .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648), + .crop = { + .top = 40, + .left = 108, + .width = 5472, + .height = 3648, + }, }, }; @@ -489,7 +491,12 @@ static const struct imx283_mode supported_modes_10bit[] = { .min_shr = 10, .horizontal_ob = 96, .vertical_ob = 16, - .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648), + .crop = { + .top = 40, + .left = 108, + .width = 5472, + .height = 3648, + }, }, };
The CENTERED_RECTANGLE define fails to compile on clang and old gcc versions. Just drop it and fill in the crop rectangles explicitly. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> ---