Re: [PATCH v4 01/29] media: imx219: Rename "PIXEL_ARRAY" as "VISIBLE"

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Apr 08, 2026 at 06:39:10PM +0300, Sakari Ailus wrote:
> The imx219 driver uses macros for denoting the size of the pixel array.
> The values reflect the area of manufacturer-designated visible pixels,
> reflect this in the naming by calling it "VISIBLE" instead of
> "PIXEL_ARRAY".

The name "pixel array" is indeed bad. I'm not sure if "visible" is the
best name though. The datasheet documents those pixels as "active area".
The same name seems to be used in the documentation of other Sony
sensors, so we could standardize on that. How about naming the macros
IMX219_ACTIVE_AREA_* ?

If you agree,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/imx219.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 7da02ce5da15..cbd151d4af5f 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -142,10 +142,10 @@
>  /* IMX219 native and active pixel array size. */
>  #define IMX219_NATIVE_WIDTH		3296U
>  #define IMX219_NATIVE_HEIGHT		2480U
> -#define IMX219_PIXEL_ARRAY_LEFT		8U
> -#define IMX219_PIXEL_ARRAY_TOP		8U
> -#define IMX219_PIXEL_ARRAY_WIDTH	3280U
> -#define IMX219_PIXEL_ARRAY_HEIGHT	2464U
> +#define IMX219_VISIBLE_LEFT		8U
> +#define IMX219_VISIBLE_TOP		8U
> +#define IMX219_VISIBLE_WIDTH		3280U
> +#define IMX219_VISIBLE_HEIGHT		2464U
>  
>  /* Mode : resolution and related config&values */
>  struct imx219_mode {
> @@ -675,13 +675,13 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>  	bpp = imx219_get_format_bpp(format);
>  
>  	cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A,
> -		  crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret);
> +		  crop->left - IMX219_VISIBLE_LEFT, &ret);
>  	cci_write(imx219->regmap, IMX219_REG_X_ADD_END_A,
> -		  crop->left - IMX219_PIXEL_ARRAY_LEFT + crop->width - 1, &ret);
> +		  crop->left - IMX219_VISIBLE_LEFT + crop->width - 1, &ret);
>  	cci_write(imx219->regmap, IMX219_REG_Y_ADD_STA_A,
> -		  crop->top - IMX219_PIXEL_ARRAY_TOP, &ret);
> +		  crop->top - IMX219_VISIBLE_TOP, &ret);
>  	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
> -		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
> +		  crop->top - IMX219_VISIBLE_TOP + crop->height - 1, &ret);
>  
>  	imx219_get_binning(state, &bin_h, &bin_v);
>  	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> @@ -867,8 +867,8 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	 * Use binning to maximize the crop rectangle size, and centre it in the
>  	 * sensor.
>  	 */
> -	bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U);
> -	bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);
> +	bin_h = min(IMX219_VISIBLE_WIDTH / format->width, 2U);
> +	bin_v = min(IMX219_VISIBLE_HEIGHT / format->height, 2U);
>  
>  	/* Ensure bin_h and bin_v are same to avoid 1:2 or 2:1 stretching */
>  	binning = min(bin_h, bin_v);
> @@ -967,10 +967,10 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
>  
>  	case V4L2_SEL_TGT_CROP_DEFAULT:
>  	case V4L2_SEL_TGT_CROP_BOUNDS:
> -		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
> -		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
> -		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
> -		sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT;
> +		sel->r.top = IMX219_VISIBLE_TOP;
> +		sel->r.left = IMX219_VISIBLE_LEFT;
> +		sel->r.width = IMX219_VISIBLE_WIDTH;
> +		sel->r.height = IMX219_VISIBLE_HEIGHT;
>  
>  		return 0;
>  	}

-- 
Regards,

Laurent Pinchart