| Message ID | 20250314-acpm-fixes-v1-3-ab03ca8e723f@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: exynos-acpm: read fix & reduce log verbosity | expand |
On 3/14/25 4:40 PM, André Draszik wrote: > dev_err_probe() exists to simplify code and unify error messages by > using its message template. > > Convert the remaining dev_err() in acpm_get_by_phandle() to > dev_err_probe(). > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- > drivers/firmware/samsung/exynos-acpm.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c > index 48f1e3cacaa709ae703115169df138b659ddae44..03f907a95c6acd66d89cd8af2f52e7c6dadf492a 100644 > --- a/drivers/firmware/samsung/exynos-acpm.c > +++ b/drivers/firmware/samsung/exynos-acpm.c > @@ -701,12 +701,14 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev, > > link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER); > if (!link) { > - dev_err(&pdev->dev, > - "Failed to create device link to consumer %s.\n", > - dev_name(dev)); > + int ret = -EINVAL; > + > + dev_err_probe(&pdev->dev, ret, > + "Failed to create device link to consumer %s.\n", > + dev_name(dev)); > platform_device_put(pdev); > module_put(pdev->dev.driver->owner); > - return ERR_PTR(-EINVAL); > + return ERR_PTR(ret); > } > > return &acpm->handle; > The clients are indeed expected to call this method in their probe method. Shall we make such assumption? I'm in the middle here, but I don't mind if this gets queued: Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
On 14/03/2025 17:40, André Draszik wrote: > dev_err_probe() exists to simplify code and unify error messages by > using its message template. > > Convert the remaining dev_err() in acpm_get_by_phandle() to > dev_err_probe(). > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- > drivers/firmware/samsung/exynos-acpm.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c > index 48f1e3cacaa709ae703115169df138b659ddae44..03f907a95c6acd66d89cd8af2f52e7c6dadf492a 100644 > --- a/drivers/firmware/samsung/exynos-acpm.c > +++ b/drivers/firmware/samsung/exynos-acpm.c > @@ -701,12 +701,14 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev, > > link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER); > if (!link) { > - dev_err(&pdev->dev, > - "Failed to create device link to consumer %s.\n", > - dev_name(dev)); > + int ret = -EINVAL; > + > + dev_err_probe(&pdev->dev, ret, > + "Failed to create device link to consumer %s.\n", > + dev_name(dev)); I do not see how it is simpler. Three lines (statement) is now 5 lines with two statements. What's more important, dev_err_probe is supposed to be used only in probe context, while this could be called in other contexts. Best regards, Krzysztof
Hi Krzysztof, On Tue, 2025-03-18 at 20:23 +0100, Krzysztof Kozlowski wrote: > On 14/03/2025 17:40, André Draszik wrote: > > dev_err_probe() exists to simplify code and unify error messages by > > using its message template. > > > > Convert the remaining dev_err() in acpm_get_by_phandle() to > > dev_err_probe(). > > > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > --- > > drivers/firmware/samsung/exynos-acpm.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c > > index 48f1e3cacaa709ae703115169df138b659ddae44..03f907a95c6acd66d89cd8af2f52e7c6dadf492a 100644 > > --- a/drivers/firmware/samsung/exynos-acpm.c > > +++ b/drivers/firmware/samsung/exynos-acpm.c > > @@ -701,12 +701,14 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev, > > > > link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER); > > if (!link) { > > - dev_err(&pdev->dev, > > - "Failed to create device link to consumer %s.\n", > > - dev_name(dev)); > > + int ret = -EINVAL; > > + > > + dev_err_probe(&pdev->dev, ret, > > + "Failed to create device link to consumer %s.\n", > > + dev_name(dev)); > > I do not see how it is simpler. Three lines (statement) is now 5 lines > with two statements. This was part of some patches converting to scoped cleanup, and there it was shorter. Shouldn't have taken this change out of that context... > What's more important, dev_err_probe is supposed to be used only in > probe context, while this could be called in other contexts. True. dev_err_probe is nice though in that it gives us unified error messages. Happy to drop for now. Cheers, A.
diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c index 48f1e3cacaa709ae703115169df138b659ddae44..03f907a95c6acd66d89cd8af2f52e7c6dadf492a 100644 --- a/drivers/firmware/samsung/exynos-acpm.c +++ b/drivers/firmware/samsung/exynos-acpm.c @@ -701,12 +701,14 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev, link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER); if (!link) { - dev_err(&pdev->dev, - "Failed to create device link to consumer %s.\n", - dev_name(dev)); + int ret = -EINVAL; + + dev_err_probe(&pdev->dev, ret, + "Failed to create device link to consumer %s.\n", + dev_name(dev)); platform_device_put(pdev); module_put(pdev->dev.driver->owner); - return ERR_PTR(-EINVAL); + return ERR_PTR(ret); } return &acpm->handle;
dev_err_probe() exists to simplify code and unify error messages by using its message template. Convert the remaining dev_err() in acpm_get_by_phandle() to dev_err_probe(). Signed-off-by: André Draszik <andre.draszik@linaro.org> --- drivers/firmware/samsung/exynos-acpm.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)