Date:   Tue, 24 Jan 2023 01:02:51 +0200
From:   Sakari Ailus <sakari.ailus@iki.fi>
To:     shravan kumar <shravan.chippa@microchip.com>
Cc:     paul.j.murphy@intel.com, daniele.alessandrelli@intel.com,
        mchehab@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com,
        kernel@pengutronix.de, linux-imx@nxp.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
 frequency
Message-ID: <Y88SG5ndXjQ9AjZg@valkosipuli.retiisi.eu>
References: <20230121033713.3535351-1-shravan.chippa@microchip.com>
 <20230121033713.3535351-6-shravan.chippa@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230121033713.3535351-6-shravan.chippa@microchip.com>
Precedence: bulk

Hi Shravan,

On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> Update pixel_rate and link frequency for 1920x1080@30
> while changing mode.
> 
> Add dummy ctrl cases for pixel_rate and link frequency
> to avoid error while changing the modes dynamically.
> 
> Add support to handle multiple link frequencies.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 41 ++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 309c706114d2..62b104eaa437 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -49,7 +49,8 @@
>  #define IMX334_INCLK_RATE	24000000
>  
>  /* CSI2 HW configuration */
> -#define IMX334_LINK_FREQ	891000000
> +#define IMX334_LINK_FREQ_891M	891000000
> +#define IMX334_LINK_FREQ_445M	445500000
>  #define IMX334_NUM_DATA_LANES	4
>  
>  #define IMX334_REG_MIN		0x00
> @@ -139,12 +140,14 @@ struct imx334 {
>  	u32 vblank;
>  	const struct imx334_mode *cur_mode;
>  	struct mutex mutex;
> +	unsigned long menu_skip_mask;
>  	u32 cur_code;
>  	bool streaming;
>  };
>  
>  static const s64 link_freq[] = {
> -	IMX334_LINK_FREQ,
> +	IMX334_LINK_FREQ_891M,
> +	IMX334_LINK_FREQ_445M,
>  };
>  
>  /* Sensor mode registers for 1920x1080@30fps */
> @@ -468,7 +471,7 @@ static const struct imx334_mode supported_modes[] = {
>  		.vblank_min = 45,
>  		.vblank_max = 132840,
>  		.pclk = 297000000,
> -		.link_freq_idx = 0,
> +		.link_freq_idx = 1,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
>  			.regs = mode_1920x1080_regs,
> @@ -598,6 +601,11 @@ static int imx334_update_controls(struct imx334 *imx334,
>  	if (ret)
>  		return ret;
>  
> +	ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
> +				       mode->pclk, 1, mode->pclk);
> +	if (ret)
> +		return ret;
> +
>  	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
>  				       mode->hblank, 1, mode->hblank);
>  	if (ret)
> @@ -698,6 +706,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
>  		pm_runtime_put(imx334->dev);
>  
>  		break;
> +	case V4L2_CID_PIXEL_RATE:
> +	case V4L2_CID_LINK_FREQ:
>  	case V4L2_CID_HBLANK:
>  		ret = 0;
>  		break;
> @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
>  	struct fwnode_handle *ep;
>  	unsigned long rate;
>  	int ret;
> -	int i;
> +	int i, j;

unsigned int would be nicer.

>  
>  	if (!fwnode)
>  		return -ENXIO;
> @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
>  		goto done_endpoint_free;
>  	}
>  
> -	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> -		if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> +	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> +		for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> +			if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> +				set_bit(j, &imx334->menu_skip_mask);

Is there a guarantee that you'll only be using the modes with the listed
frequencies? I don't see one but I might have missed it.

> +				break;
> +			}
> +		}
> +
> +		if (j == ARRAY_SIZE(link_freq)) {
> +			ret = dev_err_probe(imx334->dev, -EINVAL,
> +					    "no supported link freq found\n");
>  			goto done_endpoint_free;
> -
> -	ret = -EINVAL;
> +		}
> +	}
>  
>  done_endpoint_free:
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
> @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct imx334 *imx334)
>  	imx334->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  							&imx334_ctrl_ops,
>  							V4L2_CID_LINK_FREQ,
> -							ARRAY_SIZE(link_freq) -
> -							1,
> -							mode->link_freq_idx,
> +							__fls(imx334->menu_skip_mask),
> +							__ffs(imx334->menu_skip_mask),
>  							link_freq);
> +
>  	if (imx334->link_freq_ctrl)
>  		imx334->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  

-- 
Kind regards,

Sakari Ailus