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
