Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 May 2023 08:02:02 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Wang Zhang <silver_code@hust.edu.cn>
Cc:     Peter Korsgaard <peter@korsgaard.com>,
        Andrew Lunn <andrew@lunn.ch>,
        hust-os-kernel-patches@googlegroups.com, linux-i2c@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] i2c: ocores: use devm_ managed clks
Message-ID: <2ca66dae-8424-4382-a0ed-ff742921c495@kili.mountain>
References: <5572a733.abc0.18846f13b0b.Coremail.m202171703@hust.edu.cn>
 <20230524154318.2259-1-silver_code@hust.edu.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230524154318.2259-1-silver_code@hust.edu.cn>
Precedence: bulk

On Wed, May 24, 2023 at 11:43:18PM +0800, Wang Zhang wrote:
> @@ -780,19 +768,18 @@ static int ocores_i2c_resume(struct device *dev)
>  {
>  	struct ocores_i2c *i2c = dev_get_drvdata(dev);
>  
> -	if (!IS_ERR(i2c->clk)) {
> -		unsigned long rate;
> -		int ret = clk_prepare_enable(i2c->clk);
> +	unsigned long rate;
> +	int ret = clk_prepare_enable(i2c->clk);

Don't put functions which can fail in the declaration block.  Generally
the declaration block is for preliminary stuff, and the important
actions should be in the code block.  There should not be a blank line
before the function call and the error checking.

>  
> -		if (ret) {
> -			dev_err(dev,
> -				"clk_prepare_enable failed: %d\n", ret);
> -			return ret;
> -		}
> -		rate = clk_get_rate(i2c->clk) / 1000;
> -		if (rate)
> -			i2c->ip_clock_khz = rate;
> +	if (ret) {
> +		dev_err(dev,
> +			"clk_prepare_enable failed: %d\n", ret);

This can fit on one line now.

	int ret;

	ret = clk_prepare_enable(i2c->clk);
	if (ret) {
		dev_err(dev, "clk_prepare_enable failed: %d\n", ret);
		return ret;
	}

regards,
dan carpenter

> +		return ret;
>  	}
> +	rate = clk_get_rate(i2c->clk) / 1000;
> +	if (rate)
> +		i2c->ip_clock_khz = rate;
> +
>  	return ocores_init(dev, i2c);