Re: [PATCH v3] media: atomisp: replace ia_css_region with v4l2_rect

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Jan 15, 2026 at 12:06:28AM +0530, Karthikey Kadati wrote:

> The struct ia_css_region definition is redundant as struct v4l2_rect
> provides the same functionality (left, top, width, height) and is the
> standard V4L2 type.
> 
> Replace usage of ia_css_region with v4l2_rect in ia_css_dz_config
> and remove the definition of ia_css_region from ia_css_types.h.
> 
> Also remove historical comments referencing the addition of zoom_region
> and include <linux/videodev2.h> to support the v4l2_rect type.

Thanks for the update, my comments below.

...

> int atomisp_reset(struct atomisp_device *isp)

>  	/* Reset ISP by power-cycling it */
>  	int ret = 0;
>  
> -	dev_dbg(isp->dev, "%s\n", __func__);
>  
>  	ret = atomisp_power_off(isp->dev);
>  	if (ret < 0)

Commit message doesn't say anything about this kind of changes.

...

>  static void atomisp_3a_stats_ready_event(struct atomisp_sub_device *asd,
> -	uint8_t exp_id)
> +					 uint8_t exp_id)

Ditto (and if you go this way, the change actually should move from uint8_t to
u8 (or __u8 if it's the part of ABI).

...

With that said, this change has to be split to at least 3 patches.

...

> -	if (dz_config->zoom_region.resolution.width
> -	    == asd->sensor_array_res.width
> -	    || dz_config->zoom_region.resolution.height
> +	if (dz_config->zoom_region.width
> +	    == asd->sensor_array_res.width ||
> +	    dz_config->zoom_region.height
>  	    == asd->sensor_array_res.height) {

Make sure the == is on the previous line in both cases. I believe it's now
enough room to make it joined.

...

> -		dz_config->zoom_region.origin.x = dz_config->zoom_region.origin.x
> +		dz_config->zoom_region.left = dz_config->zoom_region.left
>  						  * eff_res.width
>  						  / (asd->sensor_array_res.width - 2 * w_offset);

Should be now

		dz_config->zoom_region.left = dz_config->zoom_region.left *
			eff_res.width / (asd->sensor_array_res.width - 2 * w_offset);

as it fits closer to 80 with gain in readability.

...

Same comments (as above and here) are applied to other similar cases.


...

> +#include <linux/videodev2.h>

+ blank line as the below is a private header for the driver.

>  #include <type_support.h>

>  #include "ia_css_frac.h"

...

>  struct ia_css_dz_config {
>  	u32 dx; /** Horizontal zoom factor */
>  	u32 dy; /** Vertical zoom factor */
> -	struct ia_css_region zoom_region; /** region for zoom */
> +	struct v4l2_rect zoom_region; /** region for zoom */

Precursor patch to convert this to kernel-doc?

>  };

-- 
With Best Regards,
Andy Shevchenko