Re: [PATCH v4 3/6] media: mt9m114: Factor out mt9m114_configure_pa

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

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:37AM +0100, Mathis Foerst wrote:
> The function mt9m114_configure writes the configuration registers of both,
> the pixel array (pa) and the image flow processor (ifp).
> This is undesirable if only the config of the pa should be changed without
> affecting the ifp.

I assume I'll see in a later patch why this is needed.

> Factor out the function mt9m114_configure_pa() that just writes the
> pa-configuration.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  drivers/media/i2c/mt9m114.c | 49 +++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index b06003b69f6f..9a49dab65180 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -789,39 +789,22 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>  	return 0;
>  }
>  
> -static int mt9m114_configure(struct mt9m114 *sensor,
> -			     struct v4l2_subdev_state *pa_state,
> -			     struct v4l2_subdev_state *ifp_state)
> +static int mt9m114_configure_pa(struct mt9m114 *sensor, struct v4l2_subdev_state *pa_state)

You can name the variable 'state' now that there's a single one.

static int mt9m114_configure_pa(struct mt9m114 *sensor,
				struct v4l2_subdev_state *state)

>  {
>  	const struct v4l2_mbus_framefmt *pa_format;
>  	const struct v4l2_rect *pa_crop;

Similarly, these can be renamed to format and crop.

> -	const struct mt9m114_format_info *ifp_info;
> -	const struct v4l2_mbus_framefmt *ifp_format;
> -	const struct v4l2_rect *ifp_crop;
> -	const struct v4l2_rect *ifp_compose;
> -	unsigned int hratio, vratio;
> -	u64 output_format;
>  	u64 read_mode;
> +	unsigned int hratio, vratio;

You can keep the order of the variables unchanged.

>  	int ret = 0;
>  
>  	pa_format = v4l2_subdev_state_get_format(pa_state, 0);
>  	pa_crop = v4l2_subdev_state_get_crop(pa_state, 0);
>  
> -	ifp_format = v4l2_subdev_state_get_format(ifp_state, 1);
> -	ifp_info = mt9m114_format_info(sensor, 1, ifp_format->code);
> -	ifp_crop = v4l2_subdev_state_get_crop(ifp_state, 0);
> -	ifp_compose = v4l2_subdev_state_get_compose(ifp_state, 0);
> -
>  	ret = cci_read(sensor->regmap, MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
>  		       &read_mode, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = cci_read(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
> -		       &output_format, NULL);
> -	if (ret < 0)
> -		return ret;
> -
>  	hratio = pa_crop->width / pa_format->width;
>  	vratio = pa_crop->height / pa_format->height;
>  
> @@ -853,6 +836,34 @@ static int mt9m114_configure(struct mt9m114 *sensor,
>  	cci_write(sensor->regmap, MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
>  		  read_mode, &ret);
>  
> +	return ret;
> +}
> +
> +static int mt9m114_configure(struct mt9m114 *sensor,
> +			     struct v4l2_subdev_state *pa_state,
> +			     struct v4l2_subdev_state *ifp_state)
> +{
> +	const struct mt9m114_format_info *ifp_info;
> +	const struct v4l2_mbus_framefmt *ifp_format;
> +	const struct v4l2_rect *ifp_crop;
> +	const struct v4l2_rect *ifp_compose;

And here you can also drop the ifp_ prefix.

> +	u64 output_format;
> +	int ret = 0;

No need to initialize ret to 0.

> +
> +	ifp_format = v4l2_subdev_state_get_format(ifp_state, 1);
> +	ifp_info = mt9m114_format_info(sensor, 1, ifp_format->code);
> +	ifp_crop = v4l2_subdev_state_get_crop(ifp_state, 0);
> +	ifp_compose = v4l2_subdev_state_get_compose(ifp_state, 0);
> +
> +	ret = cci_read(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
> +		       &output_format, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mt9m114_configure_pa(sensor, pa_state);
> +	if (ret < 0)
> +		return ret;
> +
>  	/*
>  	 * Color pipeline (IFP) cropping and scaling. Subtract 4 from the left
>  	 * and top coordinates to compensate for the lines and columns removed

For symmetry, could you call this mt9m114_configure_ifp() and move the
call to mt9m114_configure_pa() to mt9m114_start_streaming() ?

With those issues addressed, and assuming that the rationale for this
patch is good,

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

-- 
Regards,

Laurent Pinchart