Messages in this thread | | | Subject | Re: [PATCH v2 5/9] ASoC: tegra: add Tegra210 based AHUB driver | From | Sameer Pujar <> | Date | Tue, 4 Feb 2020 10:04:37 +0530 |
| |
On 1/30/2020 10:39 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 30.01.2020 13:33, Sameer Pujar пишет: > ... >> + ret = devm_snd_soc_register_component(&pdev->dev, >> + ahub->soc_data->cmpnt_drv, >> + ahub->soc_data->dai_drv, >> + ahub->soc_data->num_dais); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "failed to register component, err: %d\n", >> + ret); >> + return ret; >> + } > In the the patch #4 ("ASoC: tegra: add Tegra210 based I2S driver") I see > the following: > > ret = devm_snd_soc_register_component(dev, &tegra210_i2s_cmpnt, > tegra210_i2s_dais, > ARRAY_SIZE(tegra210_i2s_dais)); > if (ret != 0) { > dev_err(dev, "can't register I2S component, err: %d\n", ret); > return ret; > } > > Please be consistent in regards to errors checking. The correct variant > should be: if (ret != 0). Usually error codes are a negative value, but > it is much safer to check whether value isn't 0 in all cases where > positive value isn't expected to happen.
yes "if (ret)" is good enough in such cases. > > I'd also recommend to rename all "ret" variables to "err" everywhere in > the code where returned value is used only for errors checking. This > will make code more explicit, and hence, easier to read and follow.
OK, I will update it to 'err' for better. > > So, it will be nicer to write it as: > > err = devm_snd_soc_register_component(&pdev->dev, > ahub->soc_data->cmpnt_drv, > ahub->soc_data->dai_drv, > ahub->soc_data->num_dais); > if (err) { > dev_err(&pdev->dev, "failed to register component: %d\n", err); > return err; > }
| |