Re: [PATCH v4 13/15] media: atomisp: Use struct v4l2_area for padding

[Andy Shevchenko]

On Fri, Aug 28, 2026 at 06:14:55PM +0200, Maurizio Casciano wrote:
> The padding helper passes width and height as four separate scalar
> arguments even though they form two logical dimensions.
> 
> Pass the requested size and returned padding as struct v4l2_area values.
> This makes the dimensions explicit and simplifies all three callers.

> Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>

This is fine...

> Link: https://lore.kernel.org/linux-media/apCc_pt5dDxGJrei@ashevche-desk.local/

...but drop this one. No need to have it in the commit message even if
there is an accompanying Suggested-by tag.

...

> int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,

>  	const struct atomisp_format_bridge *fmt, *snr_fmt;
>  	struct atomisp_sub_device *asd = &isp->asd;
>  	struct v4l2_mbus_framefmt ffmt = { };
> -	u32 padding_w, padding_h;
> +	struct v4l2_area padding;
> +	struct v4l2_area size;
>  	int ret;
>  
>  	fmt = atomisp_get_format_bridge(f->pixelformat);

>  	 * resolution + padding. Add padding here and remove it again after
>  	 * the set_fmt call, like atomisp_set_fmt_to_snr() does.
>  	 */
> -	atomisp_get_padding(isp, f->width, f->height, &padding_w, &padding_h);
> +	size.width = f->width;
> +	size.height = f->height;
> +	atomisp_get_padding(isp, size, &padding);
>  	v4l2_fill_mbus_format(&ffmt, f, fmt->mbus_code);
> -	ffmt.width += padding_w;
> -	ffmt.height += padding_h;
> +	ffmt.width += padding.width;
> +	ffmt.height += padding.height;

Add a wrapper that takes struct v4l2_pix_format

static inline void atomisp_get_pix_padding(struct atomisp_device *isp,
					   struct v4l2_pix_format *f,
					   struct v4l2_area *pad)
{
	struct v4l2_area size = { .width = f->width, .height = f->height };

	atomisp_get_padding(isp, size, pad);
}

So it will become here as

	atomisp_get_pix_padding(isp, f, &padding);

...

>  	if (atomisp_subdev_format_conversion(asd)) {
> -		atomisp_get_padding(isp, f->fmt.pix.width, f->fmt.pix.height,
> -				    &asd->sink_pad_padding_w, &asd->sink_pad_padding_h);
> +		size.width = f->fmt.pix.width;
> +		size.height = f->fmt.pix.height;
> +		atomisp_get_padding(isp, size, &padding);
> +		asd->sink_pad_padding_w = padding.width;
> +		asd->sink_pad_padding_h = padding.height;

And respectively here

		atomisp_get_pix_padding(isp, &f->fmt.pix, &padding);

>  	} else {
>  		asd->sink_pad_padding_w = 0;
>  		asd->sink_pad_padding_h = 0;

Looking at this, I would also replace these two in asd to be struct v4l2_area.
There are only three users.

With that this becomes

	if (atomisp_subdev_format_conversion(asd))
		atomisp_get_pix_padding(isp, &f->fmt.pix, &asd->sink_pad_padding);
	else
		asd->sink_pad_padding = {};


...

> static int atomisp_enum_framesizes_crop_inner(struct atomisp_device *isp,

>  		{  800,  600 },
>  		{  640,  480 },
>  	};
> -	u32 padding_w, padding_h;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(frame_sizes); i++) {
> -		atomisp_get_padding(isp, frame_sizes[i].width, frame_sizes[i].height,
> -				    &padding_w, &padding_h);
> +		struct v4l2_area size = {
> +			.width = frame_sizes[i].width,
> +			.height = frame_sizes[i].height,
> +		};

Just convert frame_sizes to be of struct v4l2_area type.

> +		struct v4l2_area padding;
>  
> -		if ((frame_sizes[i].width + padding_w) > native->width ||
> -		    (frame_sizes[i].height + padding_h) > native->height)
> +		atomisp_get_padding(isp, size, &padding);
> +
> +		if ((frame_sizes[i].width + padding.width) > native->width ||
> +		    (frame_sizes[i].height + padding.height) > native->height)
>  			continue;


Then only fsize->discrete = frame_sizes[i]; will require the member-to-member
assignment.

-- 
With Best Regards,
Andy Shevchenko