| Message ID | 20240315112745.63230-5-bastien.curutchet@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: ti: davinci-i2s: Add features to McBSP driver | expand |
On Fri, Mar 15, 2024 at 12:27:36PM +0100, Bastien Curutchet wrote: > - dev_err(&pdev->dev, "no mem resource?\n"); > - return -ENODEV; > + return dev_err_probe(&pdev->dev, -ENODEV, "no mem resource?\n"); > } dev_err_probe() with a fixed error code doesn't seem to make much sense, the whole point is to handle deferral but for a straight lookup like this that can't happen.
Hi Mark, On Fri, 15 Mar 2024 14:07:13 +0000 Mark Brown <broonie@kernel.org> wrote: > On Fri, Mar 15, 2024 at 12:27:36PM +0100, Bastien Curutchet wrote: > > > - dev_err(&pdev->dev, "no mem resource?\n"); > > - return -ENODEV; > > + return dev_err_probe(&pdev->dev, -ENODEV, "no mem resource?\n"); > > } > > dev_err_probe() with a fixed error code doesn't seem to make much sense, > the whole point is to handle deferral but for a straight lookup like > this that can't happen. The error code is uniformly formatted and the error path is more compact. https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963 IMHO, to benefit of these feature, it makes sense to use it even with a fixed error code. Best regards, Hervé
On Fri, Mar 15, 2024 at 03:23:32PM +0100, Herve Codina wrote: > Mark Brown <broonie@kernel.org> wrote: > > dev_err_probe() with a fixed error code doesn't seem to make much sense, > > the whole point is to handle deferral but for a straight lookup like > > this that can't happen. > The error code is uniformly formatted and the error path is more compact. > https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963 > IMHO, to benefit of these feature, it makes sense to use it even with a fixed > error code. I'm not convinced TBH, the fixed error code smells pretty bad.
Hi Mark, On 3/15/24 15:40, Mark Brown wrote: > On Fri, Mar 15, 2024 at 03:23:32PM +0100, Herve Codina wrote: >> Mark Brown <broonie@kernel.org> wrote: > >>> dev_err_probe() with a fixed error code doesn't seem to make much sense, >>> the whole point is to handle deferral but for a straight lookup like >>> this that can't happen. > >> The error code is uniformly formatted and the error path is more compact. >> https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L4963 > >> IMHO, to benefit of these feature, it makes sense to use it even with a fixed >> error code. > > I'm not convinced TBH, the fixed error code smells pretty bad. Ok. I'll keep the dev_err() for the fixed errors then, and use the dev_err_probe() for the others, would that be ok ? Best regards, Bastien
On Mon, Mar 18, 2024 at 08:40:24AM +0100, Bastien Curutchet wrote: > On 3/15/24 15:40, Mark Brown wrote: > > I'm not convinced TBH, the fixed error code smells pretty bad. > Ok. I'll keep the dev_err() for the fixed errors then, and use the > dev_err_probe() for the others, would that be ok ? Yes, when we can get a deferral it's the right thing.
diff --git a/sound/soc/ti/davinci-i2s.c b/sound/soc/ti/davinci-i2s.c index 5c906641640e..4cb3ef62db20 100644 --- a/sound/soc/ti/davinci-i2s.c +++ b/sound/soc/ti/davinci-i2s.c @@ -644,8 +644,7 @@ static int davinci_i2s_probe(struct platform_device *pdev) "\"mpu\" mem resource not found, using index 0\n"); mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!mem) { - dev_err(&pdev->dev, "no mem resource?\n"); - return -ENODEV; + return dev_err_probe(&pdev->dev, -ENODEV, "no mem resource?\n"); } } @@ -672,8 +671,7 @@ static int davinci_i2s_probe(struct platform_device *pdev) } else if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) { dma_data->filter_data = "tx"; } else { - dev_err(&pdev->dev, "Missing DMA tx resource\n"); - return -ENODEV; + return dev_err_probe(&pdev->dev, -ENODEV, "Missing DMA tx resource\n"); } dma_data = &dev->dma_data[SNDRV_PCM_STREAM_CAPTURE]; @@ -687,8 +685,7 @@ static int davinci_i2s_probe(struct platform_device *pdev) } else if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) { dma_data->filter_data = "rx"; } else { - dev_err(&pdev->dev, "Missing DMA rx resource\n"); - return -ENODEV; + return dev_err_probe(&pdev->dev, -ENODEV, "Missing DMA rx resource\n"); } dev->clk = clk_get(&pdev->dev, NULL); @@ -708,7 +705,7 @@ static int davinci_i2s_probe(struct platform_device *pdev) ret = edma_pcm_platform_register(&pdev->dev); if (ret) { - dev_err(&pdev->dev, "register PCM failed: %d\n", ret); + dev_err_probe(&pdev->dev, ret, "register PCM failed\n"); goto err_unregister_component; }
There are dev_err() in the probe() function. Replace them with dev_err_probe() Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com> --- sound/soc/ti/davinci-i2s.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)