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

[Philipp Zabel]

On Tue, 2021-11-09 at 14:59 +0800, LH.Kuo wrote:
[...]
> +static int sp_i2c_probe(struct platform_device *pdev)
> +{
> +	struct sp_i2c_dev *sp_i2c_dev_info;
> +	struct i2c_adapter *p_adap;
> +	unsigned int i2c_clk_freq;
> +	int device_id = 0;
> +	int ret = I2C_SUCCESS;
> +	struct device *dev = &pdev->dev;
> +	const struct i2c_compatible *dev_mode;
> +
> +	if (pdev->dev.of_node) {
> +		pdev->id = of_alias_get_id(pdev->dev.of_node, "i2c");
> +		dev_err(&pdev->dev, "pdev->id=%d\n", pdev->id);
> +		device_id = pdev->id;
> +	}
> +
> +	sp_i2c_dev_info = devm_kzalloc(&pdev->dev, sizeof(*sp_i2c_dev_info), GFP_KERNEL);
> +	if (!sp_i2c_dev_info)
> +		return -ENOMEM;
> +
> +	if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency", &i2c_clk_freq)) {
> +		dev_err(&pdev->dev, "clk_freq %d\n", i2c_clk_freq);
> +		sp_i2c_dev_info->i2c_clk_freq = i2c_clk_freq;
> +	} else {
> +		sp_i2c_dev_info->i2c_clk_freq = SP_I2C_FREQ*1000;
> +	}
> +
> +	sp_i2c_dev_info->dev = &pdev->dev;
> +
> +	ret = _sp_i2cm_get_resources(pdev, sp_i2c_dev_info);
> +	if (ret != I2C_SUCCESS) {
> +		dev_err(&pdev->dev, " get resources fail !\n");
> +		return ret;
> +	}
> +
> +	/* dma alloc*/
> +	sp_i2c_dev_info->dma_vir_base = dma_alloc_coherent(&pdev->dev, SP_BUFFER_SIZE,
> +					&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.

> +
> +	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().

> +	}
> +
> +	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.

> +	}
> +
> +	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-pointers

> +		goto err_clk_disable;

		return ret;

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

		goto err_clk_disable;

> +	}
> +
> +	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.

> +	}
> +
> +	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,
> +				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().

> +	}
> +
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 5000);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return ret;
> +
> +err_reset_assert:
> +	reset_control_assert(sp_i2c_dev_info->rstc);
> +err_clk_disable:
> +	clk_disable_unprepare(sp_i2c_dev_info->clk);
> +free_dma:
> +	dma_free_coherent(&pdev->dev, SP_BUFFER_SIZE,
> +			sp_i2c_dev_info->dma_vir_base, sp_i2c_dev_info->dma_phy_base);
> +
> +	return ret;
> +}

regards
Philipp