Received-SPF: pass (google.com: domain of linux-kernel+bounces-210327-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 11 Jun 2024 10:26:02 -0700
From: Nathan Chancellor <nathan@kernel.org>
To: Umang Jain <umang.jain@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>, Shawn Guo <shawnguo@kernel.org>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev
Subject: Re: [PATCH v4 2/2] media: i2c: Add imx283 camera sensor driver
Message-ID: <20240611172602.GA2226028@thelio-3990X>
References: <20240402-kernel-name-extraversion-v4-0-fb776893e4ec@ideasonboard.com>
 <20240402-kernel-name-extraversion-v4-2-fb776893e4ec@ideasonboard.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240402-kernel-name-extraversion-v4-2-fb776893e4ec@ideasonboard.com>

Hi Umang,

On Tue, Apr 02, 2024 at 03:37:51PM +0530, Umang Jain wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Add a v4l2 subdevice driver for the Sony IMX283 image sensor.
> 
> The IMX283 is a 20MP Diagonal 15.86 mm (Type 1) CMOS Image Sensor with
> Square Pixel for Color Cameras.
> 
> The following features are supported:
> - Manual exposure an gain control support
> - vblank/hblank/link freq control support
> - Test pattern support control
> - Arbitrary horizontal and vertical cropping
> - Supported resolution:
>   - 5472x3648 @ 20fps (SRGGB12)
>   - 5472x3648 @ 25fps (SRGGB10)
>   - 2736x1824 @ 50fps (SRGGB12)
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

This change is now in -next as commit ccb4eb4496fa ("media: i2c: Add
imx283 camera sensor driver").

> +++ b/drivers/media/i2c/imx283.c
...
> +/* IMX283 native and active pixel array size. */
> +static const struct v4l2_rect imx283_native_area = {
> +	.top = 0,
> +	.left = 0,
> +	.width = 5592,
> +	.height = 3710,
> +};
> +
> +static const struct v4l2_rect imx283_active_area = {
> +	.top = 40,
> +	.left = 108,
> +	.width = 5472,
> +	.height = 3648,
> +};
...
> +#define CENTERED_RECTANGLE(rect, _width, _height)			\
> +	{								\
> +		.left = rect.left + ((rect.width - (_width)) / 2),	\
> +		.top = rect.top + ((rect.height - (_height)) / 2),	\
> +		.width = (_width),					\
> +		.height = (_height),					\
> +	}
...
> +		.crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),

This construct does not work with GCC prior to 7 and Clang prior to 17
(where certain const structures and variables will be considered
constant expressions for the sake of initializers and such), resulting
in:

  drivers/media/i2c/imx283.c:443:30: error: initializer element is not constant
     .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
                                ^
  drivers/media/i2c/imx283.c:412:11: note: in definition of macro 'CENTERED_RECTANGLE'
     .left = rect.left + ((rect.width - (_width)) / 2), \
             ^~~~
  drivers/media/i2c/imx283.c:443:30: note: (near initialization for 'supported_modes_12bit[0].crop.left')
     .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
                                ^
  drivers/media/i2c/imx283.c:412:11: note: in definition of macro 'CENTERED_RECTANGLE'
     .left = rect.left + ((rect.width - (_width)) / 2), \
             ^~~~
  drivers/media/i2c/imx283.c:443:30: error: initializer element is not constant
     .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
                                ^
  drivers/media/i2c/imx283.c:413:10: note: in definition of macro 'CENTERED_RECTANGLE'
     .top = rect.top + ((rect.height - (_height)) / 2), \
            ^~~~
  drivers/media/i2c/imx283.c:443:30: note: (near initialization for 'supported_modes_12bit[0].crop.top')
     .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
                                ^
  drivers/media/i2c/imx283.c:413:10: note: in definition of macro 'CENTERED_RECTANGLE'
     .top = rect.top + ((rect.height - (_height)) / 2), \
            ^~~~

  drivers/media/i2c/imx283.c:443:30: error: initializer element is not a compile-time constant
                  .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
                          ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/media/i2c/imx283.c:412:11: note: expanded from macro 'CENTERED_RECTANGLE'
                  .left = rect.left + ((rect.width - (_width)) / 2),      \
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/media/i2c/imx283.c:492:30: error: initializer element is not a compile-time constant
                  .crop = CENTERED_RECTANGLE(imx283_active_area, 5472, 3648),
                          ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/media/i2c/imx283.c:412:11: note: expanded from macro 'CENTERED_RECTANGLE'
                  .left = rect.left + ((rect.width - (_width)) / 2),      \
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  2 errors generated.

with these compiler versions. Usually, the values are just refactored
with #define macros.

Cheers,
Nathan