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
