Re: [PATCH v1 10/15] media: i2c: imx290: Factor out clock initialization to separate function

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

Hello Laurent,

thanks for working on this.

Am Dienstag, 22. November 2022, 23:32:45 CET schrieb Laurent Pinchart:
> Move the external clock initialization code from probe() to a separate
> function to improve readability. No functional change intended.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx290.c | 57 +++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index d423860402fd..848de4c90d3b 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1117,6 +1117,34 @@ static int imx290_get_regulators(struct device *dev,
> struct imx290 *imx290) imx290->supplies);
>  }
> 
> +static int imx290_init_clk(struct imx290 *imx290)
> +{
> +	u32 xclk_freq;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(dev_fwnode(imx290->dev),
> +				       "clock-frequency", &xclk_freq);
> +	if (ret) {
> +		dev_err(imx290->dev, "Could not get xclk frequency\n");
> +		return ret;
> +	}
> +
> +	/* external clock must be 37.125 MHz */
> +	if (xclk_freq != 37125000) {
> +		dev_err(imx290->dev, "External clock frequency %u is not 
supported\n",
> +			xclk_freq);
> +		return -EINVAL;
> +	}
> +
> +	ret = clk_set_rate(imx290->xclk, xclk_freq);
> +	if (ret) {
> +		dev_err(imx290->dev, "Could not set xclk frequency\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Returns 0 if all link frequencies used by the driver for the given
> number * of MIPI data lanes are mentioned in the device tree, or the value
> of the @@ -1201,7 +1229,6 @@ static int imx290_probe(struct i2c_client
> *client) {
>  	struct device *dev = &client->dev;
>  	struct imx290 *imx290;
> -	u32 xclk_freq;
>  	u32 chip_id;
>  	int ret;
> 
> @@ -1220,32 +1247,12 @@ static int imx290_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
> 
> -	/* get system clock (xclk) */
> +	/* Acquire resources. */
>  	imx290->xclk = devm_clk_get(dev, "xclk");
>  	if (IS_ERR(imx290->xclk))
>  		return dev_err_probe(dev, PTR_ERR(imx290->xclk),
>  				     "Could not get xclk");
> 
> -	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> -				       &xclk_freq);
> -	if (ret) {
> -		dev_err(dev, "Could not get xclk frequency\n");
> -		return ret;
> -	}
> -
> -	/* external clock must be 37.125 MHz */
> -	if (xclk_freq != 37125000) {
> -		dev_err(dev, "External clock frequency %u is not 
supported\n",
> -			xclk_freq);
> -		return -EINVAL;
> -	}
> -
> -	ret = clk_set_rate(imx290->xclk, xclk_freq);
> -	if (ret) {
> -		dev_err(dev, "Could not set xclk frequency\n");
> -		return ret;
> -	}
> -
>  	ret = imx290_get_regulators(dev, imx290);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Cannot get regulators\n");
> @@ -1256,8 +1263,14 @@ static int imx290_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, PTR_ERR(imx290->rst_gpio),
>  				     "Cannot get reset gpio\n");
> 
> +	/* Initialize external clock frequency. */
> +	ret = imx290_init_clk(imx290);
> +	if (ret)
> +		return ret;
> +
>  	mutex_init(&imx290->lock);
> 
> +	/* Initialize and register subdev. */
>  	ret = imx290_subdev_init(imx290);
>  	if (ret)
>  		goto err_mutex;

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