Re: [PATCH v1 12/15] media: i2c: imx290: Rename, extend and expand usage of imx290_pixfmt

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello Laurent,

thanks for the patch.

Am Dienstag, 22. November 2022, 23:32:47 CET schrieb Laurent Pinchart:
> The imx290_pixfmt structure contains information about formats,
> currently limited to the bpp value. Extend it with the register settings
> for each format, and rename it to imx290_format_info to make its purpose
> clearer. Add a function named imx290_format_info() to look up format
> info for a media bus code, and use it through the code. This allows
> dropping the imx290 bpp field as the value is now looked up dynamically.
> 
> The error handling in imx290_setup_format() can also be dropped, as the
> format is guaranteed by imx290_set_fmt() to be valid.

This looks a bit weird accessing pointers returned by imx290_format_info() 
unconditionally at some places, while imx290_format_info() might return NULL.
But AFAICS the cases where this might happen, due to userspace interactions, 
are checked.

Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx290.c | 87 ++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index cd2154983341..d279d6dd0cb4 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -182,7 +182,6 @@ struct imx290 {
>  	struct media_pad pad;
> 
>  	const struct imx290_mode *current_mode;
> -	u8 bpp;
> 
>  	struct regulator_bulk_data supplies[IMX290_NUM_SUPPLIES];
>  	struct gpio_desc *rst_gpio;
> @@ -414,16 +413,41 @@ static inline int imx290_modes_num(const struct imx290
> *imx290) return ARRAY_SIZE(imx290_modes_4lanes);
>  }
> 
> -struct imx290_pixfmt {
> +struct imx290_format_info {
>  	u32 code;
>  	u8 bpp;
> +	const struct imx290_regval *regs;
> +	unsigned int num_regs;
>  };
> 
> -static const struct imx290_pixfmt imx290_formats[] = {
> -	{ MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
> -	{ MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
> +static const struct imx290_format_info imx290_formats[] = {
> +	{
> +		.code = MEDIA_BUS_FMT_SRGGB10_1X10,
> +		.bpp = 10,
> +		.regs = imx290_10bit_settings,
> +		.num_regs = ARRAY_SIZE(imx290_10bit_settings),
> +	}, {
> +		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> +		.bpp = 12,
> +		.regs = imx290_12bit_settings,
> +		.num_regs = ARRAY_SIZE(imx290_12bit_settings),
> +	}
>  };
> 
> +static const struct imx290_format_info *imx290_format_info(u32 code)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(imx290_formats); ++i) {
> +		const struct imx290_format_info *info = 
&imx290_formats[i];
> +
> +		if (info->code == code)
> +			return info;
> +	}
> +
> +	return NULL;
> +}
> +
>  /*
> ---------------------------------------------------------------------------
> -- * Register access
>   */
> @@ -516,43 +540,31 @@ static int imx290_set_data_lanes(struct imx290
> *imx290) }
> 
>  static int imx290_set_black_level(struct imx290 *imx290,
> +				  const struct v4l2_mbus_framefmt 
*format,
>  				  unsigned int black_level, int *err)
>  {
> +	unsigned int bpp = imx290_format_info(format->code)->bpp;
> +
>  	return imx290_write(imx290, IMX290_BLKLEVEL,
> -			    black_level >> (16 - imx290->bpp), err);
> +			    black_level >> (16 - bpp), err);
>  }
> 
>  static int imx290_setup_format(struct imx290 *imx290,
>  			       const struct v4l2_mbus_framefmt *format)
>  {
> -	const struct imx290_regval *regs;
> -	unsigned int num_regs;
> -	unsigned int bpp;
> +	const struct imx290_format_info *info;
>  	int ret;
> 
> -	switch (format->code) {
> -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> -		regs = imx290_10bit_settings;
> -		num_regs = ARRAY_SIZE(imx290_10bit_settings);
> -		bpp = 10;
> -		break;
> -	case MEDIA_BUS_FMT_SRGGB12_1X12:
> -		regs = imx290_12bit_settings;
> -		num_regs = ARRAY_SIZE(imx290_12bit_settings);
> -		bpp = 12;
> -		break;
> -	default:
> -		dev_err(imx290->dev, "Unknown pixel format\n");
> -		return -EINVAL;
> -	}
> +	info = imx290_format_info(format->code);
> 
> -	ret = imx290_set_register_array(imx290, regs, num_regs);
> +	ret = imx290_set_register_array(imx290, info->regs, info->num_regs);
>  	if (ret < 0) {
>  		dev_err(imx290->dev, "Could not set format registers\n");
>  		return ret;
>  	}
> 
> -	return imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT, 
&ret);
> +	return imx290_set_black_level(imx290, format,
> +				      IMX290_BLACK_LEVEL_DEFAULT, 
&ret);
>  }
> 
>  /*
> ---------------------------------------------------------------------------
> - @@ -586,7 +598,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> 
>  	case V4L2_CID_TEST_PATTERN:
>  		if (ctrl->val) {
> -			imx290_set_black_level(imx290, 0, &ret);
> +			imx290_set_black_level(imx290, format, 0, &ret);
>  			usleep_range(10000, 11000);
>  			imx290_write(imx290, IMX290_PGCTRL,
>  				     (u8)(IMX290_PGCTRL_REGEN |
> @@ -595,8 +607,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  		} else {
>  			imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
>  			usleep_range(10000, 11000);
> -			imx290_set_black_level(imx290, 
IMX290_BLACK_LEVEL_DEFAULT,
> -					       &ret);
> +			imx290_set_black_level(imx290, format,
> +					       
IMX290_BLACK_LEVEL_DEFAULT, &ret);
>  		}
>  		break;
>  	default:
> @@ -635,7 +647,7 @@ static void imx290_ctrl_update(struct imx290 *imx290,
> 
>  	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
>  	pixel_rate = link_freq * 2 * imx290->nlanes;
> -	do_div(pixel_rate, imx290->bpp);
> +	do_div(pixel_rate, imx290_format_info(format->code)->bpp);
> 
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
>  	__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, pixel_rate);
> @@ -837,8 +849,7 @@ static int imx290_enum_frame_size(struct v4l2_subdev
> *sd, const struct imx290 *imx290 = to_imx290(sd);
>  	const struct imx290_mode *imx290_modes = imx290_modes_ptr(imx290);
> 
> -	if ((fse->code != imx290_formats[0].code) &&
> -	    (fse->code != imx290_formats[1].code))
> +	if (!imx290_format_info(fse->code))
>  		return -EINVAL;
> 
>  	if (fse->index >= imx290_modes_num(imx290))
> @@ -859,7 +870,6 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  	struct imx290 *imx290 = to_imx290(sd);
>  	const struct imx290_mode *mode;
>  	struct v4l2_mbus_framefmt *format;
> -	unsigned int i;
> 
>  	mode = v4l2_find_nearest_size(imx290_modes_ptr(imx290),
>  				      imx290_modes_num(imx290), width, 
height,
> @@ -868,21 +878,15 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> 
> -	for (i = 0; i < ARRAY_SIZE(imx290_formats); i++)
> -		if (imx290_formats[i].code == fmt->format.code)
> -			break;
> +	if (!imx290_format_info(fmt->format.code))
> +		fmt->format.code = imx290_formats[0].code;
> 
> -	if (i >= ARRAY_SIZE(imx290_formats))
> -		i = 0;
> -
> -	fmt->format.code = imx290_formats[i].code;
>  	fmt->format.field = V4L2_FIELD_NONE;
> 
>  	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> 
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		imx290->current_mode = mode;
> -		imx290->bpp = imx290_formats[i].bpp;
> 
>  		imx290_ctrl_update(imx290, &fmt->format, mode);
>  	}
> @@ -980,7 +984,6 @@ static int imx290_subdev_init(struct imx290 *imx290)
>  	int ret;
> 
>  	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> -	imx290->bpp = imx290_formats[0].bpp;
> 
>  	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
>  	imx290->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;