Re: [PATCH v3 4/7] media: i2c: imx290: Introduce initial "off" mode & link freq

[Laurent Pinchart]

Hi Benjamin,

Thank you for the patch.

On Mon, Sep 02, 2024 at 05:57:29PM +0200, bbara93@xxxxxxxxx wrote:
> From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> 
> To be compliant to the V4L2 API, the driver currently "randomly" decides
> on one of the two supported modes which also implies a link frequency.
> 
> Add a new mode and frequency which symbolize that the sensor is not in
> use. This can be used as a default value during probe() and enables us
> to avoid communication with the sensor.

I really doin't like this change. I would like to instead move away from
modes and make the driver freely configurable. Furthermore, the concept
of an initial unconfigured state isn't valid in V4L2. The driver must
fully initialize the whole device state at probe time.

> Signed-off-by: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> ---
> Changes since v2:
> - new
> ---
>  drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 6812e7cb9e23..ece4d66001f5 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = {
>  /* supported link frequencies */
>  #define FREQ_INDEX_1080P	0
>  #define FREQ_INDEX_720P		1
> +#define FREQ_INDEX_OFF		2
>  static const s64 imx290_link_freq_2lanes[] = {
>  	[FREQ_INDEX_1080P] = 445500000,
>  	[FREQ_INDEX_720P] = 297000000,
> +	[FREQ_INDEX_OFF] = 0,
>  };
>  
>  static const s64 imx290_link_freq_4lanes[] = {
>  	[FREQ_INDEX_1080P] = 222750000,
>  	[FREQ_INDEX_720P] = 148500000,
> +	[FREQ_INDEX_OFF] = 0,
>  };
>  
>  /*
> @@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
>  	},
>  };
>  
> +static const struct imx290_mode imx290_mode_off = {
> +	.link_freq_index = FREQ_INDEX_OFF,
> +};
> +
>  static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290)
>  {
>  	if (imx290->nlanes == 2)
> @@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
>  static void imx290_ctrl_update(struct imx290 *imx290,
>  			       const struct imx290_mode *mode)
>  {
> -	unsigned int hblank_min = mode->hmax_min - mode->width;
> -	unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> -	unsigned int vblank_min = mode->vmax_min - mode->height;
> -	unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> +	unsigned int hblank_min, hblank_max, vblank_min, vblank_max;
> +
> +	if (mode == &imx290_mode_off) {
> +		hblank_min = imx290_get_blank_min(imx290, false);
> +		hblank_max = HBLANK_MAX;
> +		vblank_min = imx290_get_blank_min(imx290, true);
> +		vblank_max = VBLANK_MAX;
> +	} else {
> +		hblank_min = mode->hmax_min - mode->width;
> +		hblank_max = IMX290_HMAX_MAX - mode->width;
> +		vblank_min = mode->vmax_min - mode->height;
> +		vblank_max = IMX290_VMAX_MAX - mode->height;
> +	}
>  
>  	__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
>  
> @@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290)
>  	imx290->link_freq =
>  		v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
>  				       V4L2_CID_LINK_FREQ,
> -				       imx290_link_freqs_num(imx290) - 1, 0,
> +				       imx290_link_freqs_num(imx290) - 1,
> +				       FREQ_INDEX_OFF,
>  				       imx290_link_freqs_ptr(imx290));
>  	if (imx290->link_freq)
>  		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> @@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290)
>  	struct v4l2_subdev_state *state;
>  	int ret;
>  
> -	imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> +	imx290->current_mode = &imx290_mode_off;
>  
>  	v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
>  	imx290->sd.internal_ops = &imx290_internal_ops;
> 

-- 
Regards,

Laurent Pinchart