Hi Kieran, Hans,
Thanks for working on this.
On Thu, Jun 13, 2024 at 04:26:43PM +0100, Kieran Bingham wrote:
> 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..
Or once clang before ~ 17 is deprecated? :-)
>
>
>
> >
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> > ---
> > 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,
> > + },
>
> 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,
I'd prefer this, too. Plain numbers are easier to get wrong.
>
> So - perhaps this could be (if it compiles):
> .crop = imx283_active_area,
This is my preference as well.
>
> but either way is fine with me if it unbreaks linux-next.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
Either way, as a clang compilation workaround this is ok so:
Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
> > },
> > {
> > /*
> > @@ -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,
> > + },
> > },
> > };
> >
> >
--
Kind regards,
Sakari Ailus
