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

[Philipp Zabel]

Hi,

On Wed, 2021-11-10 at 05:37 +0000, Lh Kuo 郭力豪 wrote:
[...]
> > > +	/* 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.
> > 
> 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.

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

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

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

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

> > > +		goto err_reset_assert;
> > 
> > 		goto err_clk_disable;

This is required as well.

> > 
> > > +	}
> > > +
> > > +	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);

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