drm/rockchip: Use dev_err_probe() to simplify code

[PATCH] drm/rockchip: Use dev_err_probe() to simplify code

Posted by long.yunjian@zte.com.cn 7 months, 1 week ago

From: Yumeng Fang <fang.yumeng@zte.com.cn>

In the probe path, dev_err() can be replaced with dev_err_probe()
which will check if error code is -EPROBE_DEFER and prints the
error name. It also sets the defer probe reason which can be
checked later through debugfs.

Signed-off-by: Yumeng Fang <fang.yumeng@zte.com.cn>
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index f737e7d46e66..acb59b25d928 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -213,17 +213,13 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi)

 	if (IS_ERR(hdmi->ref_clk)) {
 		ret = PTR_ERR(hdmi->ref_clk);
-		if (ret != -EPROBE_DEFER)
-			dev_err(hdmi->dev, "failed to get reference clock\n");
-		return ret;
+		return dev_err_probe(hdmi->dev, ret, "failed to get reference clock\n");
 	}

 	hdmi->grf_clk = devm_clk_get_optional(hdmi->dev, "grf");
 	if (IS_ERR(hdmi->grf_clk)) {
 		ret = PTR_ERR(hdmi->grf_clk);
-		if (ret != -EPROBE_DEFER)
-			dev_err(hdmi->dev, "failed to get grf clock\n");
-		return ret;
+		return dev_err_probe(hdmi->dev, ret, "failed to get grf clock\n");
 	}

 	ret = devm_regulator_get_enable(hdmi->dev, "avdd-0v9");
@@ -573,17 +569,13 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,

 	ret = rockchip_hdmi_parse_dt(hdmi);
 	if (ret) {
-		if (ret != -EPROBE_DEFER)
-			dev_err(hdmi->dev, "Unable to parse OF data\n");
-		return ret;
+		return dev_err_probe(hdmi->dev, ret, "Unable to parse OF data\n");
 	}

 	hdmi->phy = devm_phy_optional_get(dev, "hdmi");
 	if (IS_ERR(hdmi->phy)) {
 		ret = PTR_ERR(hdmi->phy);
-		if (ret != -EPROBE_DEFER)
-			dev_err(hdmi->dev, "failed to get phy\n");
-		return ret;
+		return dev_err_probe(hdmi->dev, ret, "failed to get phy\n");
 	}

 	if (hdmi->phy) {
-- 
2.25.1

Re: [PATCH] drm/rockchip: Use dev_err_probe() to simplify code

Posted by Heiko Stuebner 5 months, 2 weeks ago

On Thu, 15 May 2025 20:35:54 +0800, long.yunjian@zte.com.cn wrote:
> In the probe path, dev_err() can be replaced with dev_err_probe()
> which will check if error code is -EPROBE_DEFER and prints the
> error name. It also sets the defer probe reason which can be
> checked later through debugfs.
> 
> 

Applied, thanks!

[1/1] drm/rockchip: Use dev_err_probe() to simplify code
      commit: 52008d6fe7fa84ecf23864c5cc373beb085f30b9

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

Re: [PATCH] drm/rockchip: Use dev_err_probe() to simplify code

Posted by Krzysztof Kozlowski 7 months, 1 week ago

On 15/05/2025 14:35, long.yunjian@zte.com.cn wrote:
> From: Yumeng Fang <fang.yumeng@zte.com.cn>
> 
> In the probe path, dev_err() can be replaced with dev_err_probe()

That's not probe path. I am not sure if you really understand this code.

> which will check if error code is -EPROBE_DEFER and prints the
> error name. It also sets the defer probe reason which can be
> checked later through debugfs.
> 
> Signed-off-by: Yumeng Fang <fang.yumeng@zte.com.cn>

Incomplete chain.



Best regards,
Krzysztof

Re: [PATCH] drm/rockchip: Use dev_err_probe() to simplify code

Posted by Heiko Stübner 7 months, 1 week ago

Am Donnerstag, 15. Mai 2025, 17:54:20 Mitteleuropäische Sommerzeit schrieb Krzysztof Kozlowski:
> On 15/05/2025 14:35, long.yunjian@zte.com.cn wrote:
> > From: Yumeng Fang <fang.yumeng@zte.com.cn>
> > 
> > In the probe path, dev_err() can be replaced with dev_err_probe()
> 
> That's not probe path. I am not sure if you really understand this code.

I think that is somewhat debateable.

dw_hdmi_rockchip_bind() is part of the rockchip-drm component device,
so part of its probe-path. Also I think just the presence of EPROBE_DEFER
which causes the device to re-try probing later is a nice indicator that the
code in question is _a_ probe path. (and usage of EPROBE_DEFER is an
established pattern to make that component device re-try probing later)

And the parse_dt function itself is part of that path too.

> > which will check if error code is -EPROBE_DEFER and prints the
> > error name. It also sets the defer probe reason which can be
> > checked later through debugfs.
> > 
> > Signed-off-by: Yumeng Fang <fang.yumeng@zte.com.cn>
> 
> Incomplete chain.

Yep, the patch needs a 2nd Signed-off-by line from
long.yunjian@zte.com.cn please.

Heiko

Re: [PATCH] drm/rockchip: Use dev_err_probe() to simplify code

Posted by Krzysztof Kozlowski 7 months ago

On 16/05/2025 11:58, Heiko Stübner wrote:
> Am Donnerstag, 15. Mai 2025, 17:54:20 Mitteleuropäische Sommerzeit schrieb Krzysztof Kozlowski:
>> On 15/05/2025 14:35, long.yunjian@zte.com.cn wrote:
>>> From: Yumeng Fang <fang.yumeng@zte.com.cn>
>>>
>>> In the probe path, dev_err() can be replaced with dev_err_probe()
>>
>> That's not probe path. I am not sure if you really understand this code.
> 
> I think that is somewhat debateable.
> 
> dw_hdmi_rockchip_bind() is part of the rockchip-drm component device,
> so part of its probe-path. Also I think just the presence of EPROBE_DEFER
> which causes the device to re-try probing later is a nice indicator that the

No, that's not true. You can call every API like regulator_get from any
context and you will get EPROBE_DEFER. This will not be a probe path.
There are multiple cases of such drivers, I saw such patch even day ago.

> code in question is _a_ probe path. (and usage of EPROBE_DEFER is an
> established pattern to make that component device re-try probing later)
> 
> And the parse_dt function itself is part of that path too.

I quickly glanced and this was not obvious. The commit msg is poor here
and does not explain that component_bind is ALWAYS probe path (unless it
is clear for DRM folks).


Best regards,
Krzysztof

Re:Re: [PATCH] drm/rockchip: Use dev_err_probe() to simplify code

Posted by Andy Yan 7 months ago

Hi,

在 2025-05-22 16:45:08，"Krzysztof Kozlowski" <krzk@kernel.org> 写道：
>On 16/05/2025 11:58, Heiko Stübner wrote:
>> Am Donnerstag, 15. Mai 2025, 17:54:20 Mitteleuropäische Sommerzeit schrieb Krzysztof Kozlowski:
>>> On 15/05/2025 14:35, long.yunjian@zte.com.cn wrote:
>>>> From: Yumeng Fang <fang.yumeng@zte.com.cn>
>>>>
>>>> In the probe path, dev_err() can be replaced with dev_err_probe()
>>>
>>> That's not probe path. I am not sure if you really understand this code.
>> 
>> I think that is somewhat debateable.
>> 
>> dw_hdmi_rockchip_bind() is part of the rockchip-drm component device,
>> so part of its probe-path. Also I think just the presence of EPROBE_DEFER
>> which causes the device to re-try probing later is a nice indicator that the
>
>No, that's not true. You can call every API like regulator_get from any
>context and you will get EPROBE_DEFER. This will not be a probe path.
>There are multiple cases of such drivers, I saw such patch even day ago.
>
>> code in question is _a_ probe path. (and usage of EPROBE_DEFER is an
>> established pattern to make that component device re-try probing later)
>> 
>> And the parse_dt function itself is part of that path too.
>
>I quickly glanced and this was not obvious. The commit msg is poor here
>and does not explain that component_bind is ALWAYS probe path (unless it
>is clear for DRM folks).

Yes, the component_bind is called in the probe path, here is one dump stack:

[    1.113096] platform fdd90000.vop: Adding to iommu group 8
[    1.120336] rockchip-drm display-subsystem: bound fdd90000.vop (ops vop2_component_ops)
[    1.121543] rockchip-drm display-subsystem: bound fde50000.dp (ops dw_dp_rockchip_component_ops)
[    1.122834] dwhdmiqp-rockchip fde80000.hdmi: registered DesignWare HDMI QP I2C bus driver
[    1.123554] CPU: 5 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.15.0-rc5+ #667 PREEMPT 
[    1.123561] Hardware name: RK3588S CoolPi 4 Model B (DT)
[    1.123565] Call trace:
[    1.123567]  show_stack+0x18/0x24 (C)
[    1.123577]  dump_stack_lvl+0x74/0x8c
[    1.123584]  dump_stack+0x18/0x24
[    1.123589]  dw_hdmi_qp_rockchip_bind+0x2f8/0x468
[    1.123601]  component_bind_all+0x120/0x278
[    1.123611]  rockchip_drm_bind+0x9c/0x1ac
[    1.123616]  try_to_bring_up_aggregate_device+0x214/0x2d8
[    1.123625]  component_master_add_with_match+0xc4/0x104
[    1.123635]  rockchip_drm_platform_probe+0x214/0x33c
[    1.123640]  platform_probe+0x68/0xc8
[    1.123646]  really_probe+0xc0/0x390
[    1.123655]  __driver_probe_device+0x7c/0x15c
[    1.123663]  driver_probe_device+0x3c/0x110
[    1.123671]  __driver_attach+0xf0/0x1f8
[    1.123679]  bus_for_each_dev+0x7c/0xe0
[    1.123686]  driver_attach+0x24/0x30
>
>
>Best regards,
>Krzysztof