linux-kernel - Re: [PATCH 2/3] media: i2c: imx219: Simplify binning mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <hoh2rwql6qrzpdtuvof62jzipmba77xqottwdxqd2vqitgke5a@es6m3ghbp26d>
Date: Fri, 21 Feb 2025 10:31:21 +0100
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Jai Luthra <jai.luthra@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Dave Stevenson <dave.stevenson@...pberrypi.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jacopo Mondi <jacopo.mondi@...asonboard.com>
Subject: Re: [PATCH 2/3] media: i2c: imx219: Simplify binning mode

Hi Jai

On Wed, Feb 19, 2025 at 05:16:44PM +0530, Jai Luthra wrote:
> The imx219_get_binning() function currently returns two separate pieces
> of information, the return value is the binning mode enum, and the bin_h
> and bin_v references are updated with whether to perform binning in
> horizontal and vertical dimensions.
>
> It is simpler to combine both of these pieces of information, and
> directly update the bin_h and bin_v references with the register value
> that we will write to the sensor, which includes if the binning is
> digital or analog mode, thus allowing us to remove the superfluous
> binning mode enum.
>
> This is only a style change for the driver, with no functionality
> updated.
>
> Suggested-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> Link: https://lore.kernel.org/linux-media/ubuuob7mb3o5bxoumrxv4rufutgk3lvdmdery6d3bfc6rytfti@tcchhlechzzp/
> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>

Thanks
  j

> ---
>  drivers/media/i2c/imx219.c | 55 +++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 6e51a7af5e2a05cacefb201d96a9fbdc349f17d8..f02732d8fa95de0a295f247d4f0b60017dbb2ed2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -146,12 +146,6 @@
>  #define IMX219_PIXEL_ARRAY_WIDTH	3280U
>  #define IMX219_PIXEL_ARRAY_HEIGHT	2464U
>
> -enum binning_mode {
> -	BINNING_NONE = IMX219_BINNING_NONE,
> -	BINNING_X2 = IMX219_BINNING_X2,
> -	BINNING_ANALOG_X2 = IMX219_BINNING_X2_ANALOG,
> -};
> -
>  /* Mode : resolution and related config&values */
>  struct imx219_mode {
>  	/* Frame width */
> @@ -406,39 +400,43 @@ static u32 imx219_get_format_bpp(const struct v4l2_mbus_framefmt *format)
>  	}
>  }
>
> -static enum binning_mode imx219_get_binning(struct imx219 *imx219, u8 *bin_h,
> -					    u8 *bin_v)
> +static void imx219_get_binning(struct imx219 *imx219, u8 *bin_h, u8 *bin_v)
>  {
>  	struct v4l2_subdev_state *state =
>  		v4l2_subdev_get_locked_active_state(&imx219->sd);
>  	const struct v4l2_mbus_framefmt *format =
>  		v4l2_subdev_state_get_format(state, 0);
>  	const struct v4l2_rect *crop = v4l2_subdev_state_get_crop(state, 0);
> +	u32 hbin = crop->width / format->width;
> +	u32 vbin = crop->height / format->height;
>
> -	*bin_h = crop->width / format->width;
> -	*bin_v = crop->height / format->height;
> +	*bin_h = IMX219_BINNING_NONE;
> +	*bin_v = IMX219_BINNING_NONE;
>
> -	if (*bin_h == 2 && *bin_v == 2)
> -		return BINNING_ANALOG_X2;
> -	else if (*bin_h == 2 || *bin_v == 2)
> -		/*
> -		 * Don't use analog binning if only one dimension
> -		 * is binned, as it crops the other dimension
> -		 */
> -		return BINNING_X2;
> -	else
> -		return BINNING_NONE;
> +	/*
> +	 * Use analog binning only if both dimensions are binned, as it crops
> +	 * the other dimension.
> +	 */
> +	if (hbin == 2 && vbin == 2) {
> +		*bin_h = IMX219_BINNING_X2_ANALOG;
> +		*bin_v = IMX219_BINNING_X2_ANALOG;
> +
> +		return;
> +	}
> +
> +	if (hbin == 2)
> +		*bin_h = IMX219_BINNING_X2;
> +	if (vbin == 2)
> +		*bin_v = IMX219_BINNING_X2;
>  }
>
>  static inline u32 imx219_get_rate_factor(struct imx219 *imx219)
>  {
>  	u8 bin_h, bin_v;
> -	enum binning_mode binning = imx219_get_binning(imx219, &bin_h, &bin_v);
>
> -	if (binning == BINNING_ANALOG_X2)
> -		return 2;
> +	imx219_get_binning(imx219, &bin_h, &bin_v);
>
> -	return 1;
> +	return (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? 2 : 1;
>  }
>
>  /* -----------------------------------------------------------------------------
> @@ -674,7 +672,6 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>  {
>  	const struct v4l2_mbus_framefmt *format;
>  	const struct v4l2_rect *crop;
> -	enum binning_mode binning;
>  	u8 bin_h, bin_v;
>  	u32 bpp;
>  	int ret = 0;
> @@ -692,11 +689,9 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>  	cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A,
>  		  crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret);
>
> -	binning = imx219_get_binning(imx219, &bin_h, &bin_v);
> -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H,
> -		  (bin_h == 2) ? binning : BINNING_NONE, &ret);
> -	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V,
> -		  (bin_v == 2) ? binning : BINNING_NONE, &ret);
> +	imx219_get_binning(imx219, &bin_h, &bin_v);
> +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret);
> +	cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret);
>
>  	cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
>  		  format->width, &ret);
>
> --
> 2.48.1
>