Re: [PATCH v2 1/2] I2C: Add I2C driver for Sunplus SP7021

Hi,

On Wed, 2021-11-10 at 05:37 +0000, Lh Kuo 郭力豪 wrote:
[...]

quoted

quoted

+	/* dma alloc*/
+	sp_i2c_dev_info->dma_vir_base = dma_alloc_coherent(&pdev->dev,

SP_BUFFER_SIZE,

quoted

+					&sp_i2c_dev_info->dma_phy_base, GFP_ATOMIC);
+	if (!sp_i2c_dev_info->dma_vir_base)
+		goto free_dma;

Please fix your error paths, the driver shouldn't try to revert the action that
just failed.

Here you can use dmam_alloc_coherent() and just return -ENOMEM on error.

I will make change as below  is it OK ?

	/* dma alloc*/
	sp_i2c_dev_info->dma_vir_base = dmam_alloc_coherent(&pdev->dev, SP_BUFFER_SIZE,
					&sp_i2c_dev_info->dma_phy_base, GFP_ATOMIC);
	if (!sp_i2c_dev_info->dma_vir_base)
		return -ENOMEM;

Yes, this looks good to me. With this change, you can remove the
dma_free_coherent() calls below.

quoted

quoted

+
+	sp_i2c_dev_info->clk = devm_clk_get(dev, NULL);
+
+	if (IS_ERR(sp_i2c_dev_info->clk)) {
+		ret = PTR_ERR(sp_i2c_dev_info->clk);
+		dev_err(&pdev->dev, "failed to retrieve clk: %d\n", ret);
+		goto err_clk_disable;

Then this could

		return ret;

Better, use return dev_err_probe().

I will make change as below  is it OK ?

	sp_i2c_dev_info->clk = devm_clk_get(dev, NULL);

	if (IS_ERR(sp_i2c_dev_info->clk)) {
		return dev_err_probe(&pdev->dev, PTR_ERR(sp_i2c_dev_info->clk),
				     "Could not get clock\n");
	}

Yes.

quoted

quoted

+	}
+
+	sp_i2c_dev_info->rstc = devm_reset_control_get_exclusive(dev, NULL);
+
+	if (IS_ERR(sp_i2c_dev_info->rstc)) {
+		ret = PTR_ERR(sp_i2c_dev_info->rstc);
+		dev_err(&pdev->dev, "failed to retrieve reset controller: %d\n", ret);
+		goto err_clk_disable;

Same as above.

I will make change as below  is it OK ?

	sp_i2c_dev_info->rstc = devm_reset_control_get_exclusive(dev, NULL);

	if (IS_ERR(sp_i2c_dev_info->rstc)) {
		return dev_err_probe(&pdev->dev, PTR_ERR(sp_i2c_dev_info->rstc),
				     "Could not get clock\n");
	}

The error message should be "Could not get reset\n" instead.

quoted

quoted

+	}
+
+	ret = clk_prepare_enable(sp_i2c_dev_info->clk);
+
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable clk: %d\n", ret);

Consider using "%pe" and ERR_PTR(ret) to print the error name instead of a
number [1].

[1]
https://www.kernel.org/doc/html/latest/core-api/printk-formats.html?#error-p
ointers

quoted

+		goto err_clk_disable;

		return ret;

I will make change as below  is it OK ?

	ret = clk_prepare_enable(sp_i2c_dev_info->clk);

	if (ret) {
		dev_err(&pdev->dev, "failed to enable clk: %pe\n", ERR_PTR(ret));

Ok. Alternatively, you could also use dev_err_probe() as above.

		goto err_clk_disable;

Not ok. If clk_prepare_enable() did not succeed, do not call
clk_disable_unprepare(). return ret instead.

	}

quoted

quoted

+	}
+
+	ret = reset_control_deassert(sp_i2c_dev_info->rstc);
+
+	if (ret) {
+		dev_err(&pdev->dev, "failed to deassert reset line: %d\n", ret);

Consider either changing this to %pe or use dev_err_probe(), for
consistency with the above error messages.

quoted

quoted

+		goto err_reset_assert;

		goto err_clk_disable;

This is required as well.

quoted

quoted

+	}
+
+	init_waitqueue_head(&sp_i2c_dev_info->wait);
+
+	dev_mode = of_device_get_match_data(&pdev->dev);
+	sp_i2c_dev_info->mode = dev_mode->mode;
+	sp_i2c_dev_info->total_port = dev_mode->total_port;
+	p_adap = &sp_i2c_dev_info->adap;
+	sprintf(p_adap->name, "%s%d", SP_DEVICE_NAME, device_id);
+	p_adap->algo = &sp_algorithm;
+	p_adap->algo_data = sp_i2c_dev_info;
+	p_adap->nr = device_id;
+	p_adap->class = 0;
+	p_adap->retries = 5;
+	p_adap->dev.parent = &pdev->dev;
+	p_adap->dev.of_node = pdev->dev.of_node;
+
+	ret = i2c_add_numbered_adapter(p_adap);
+
+	ret = _sp_i2cm_init(device_id, sp_i2c_dev_info);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "i2c master %d init error\n", device_id);
+		goto err_reset_assert;

This one is correct, but I'd also print ret.

I will make change as below  is it OK ?

	ret = _sp_i2cm_init(device_id, sp_i2c_dev_info);
	if (ret != 0) {
		dev_err(&pdev->dev, "i2c master %d init error ret %d\n", device_id, ret);
		goto err_reset_assert;
	}

Yes, although I'd prefer a consistent style with the error messages
above. For example:

		dev_err(&pdev->dev, "i2c master %d init error: %pe\n", device_id, ERR_PTR(ret));

or

		dev_err_probe(&pdev->dev, ret, "i2c master %d init error", device_id);

quoted

quoted

+	}
+
+	if (ret < 0)
+		goto err_reset_assert;
+	else
+		platform_set_drvdata(pdev, sp_i2c_dev_info);
+
+	ret = request_irq(sp_i2c_dev_info->irq, _sp_i2cm_irqevent_handler,

IRQF_TRIGGER_HIGH,

quoted

+				p_adap->name, sp_i2c_dev_info);
+	if (ret) {
+		dev_err(&pdev->dev, "request irq fail !!\n");
+		return I2C_ERR_REQUESET_IRQ;

Don't return non-standard error codes. This should return ret instead, but not
before going through the error cleanup path below. Also, consider using
devm_request_irq().

I will make change as below  is it OK ?
	ret = devm_request_irq(&pdev->dev, sp_i2c_dev_info->irq, _sp_i2cm_irqevent_handler,
		IRQF_TRIGGER_HIGH, p_adap->name, sp_i2c_dev_info);
	if (ret) {
		dev_err(&pdev->dev, "request irq fail !!\n");
		return ret;
	}

Yes. With this, you can remove the free_irq() call below.

regards
Philipp