On 18/03/2024 20:23, Sui Jingfeng wrote:
Hi,
On 2024/3/19 02:04, Laurent Pinchart wrote:
Improving core helpers is certainly a good idea, and if we do so, we can
simplify drivers. What I'm concerned is that commit 00084f0c01bf creates
a silent probe failure path,
No, I can't agree here. It doesn't creates a silent probe failure path.
It doesn't _in debug mode_, I agree with Laurent, having a verbose probe error should be kept.
Neil
Simply because
1) It is NOT silent.
2) It should be exist at product level kernel.
which didn't exist before it.
Again, it shouldn't be exist.
Otherwise it hints us that there is ill-behavior-ed DT in the mainstream kernel
or a specific product(or development board). If I were you, I would like to fix
the boot failure first.
In the earlier stage of my attempt to contribute, I also would like to enable
debug output as much as possible. Just like you, the benefit is obvious: It really
eliminate the pain on developing stage and when bugs happens.
But I was told many many times that mainstream kernel is not for debug, it is
for sound products. I bet you have seen some product level drivers print very less.
I'm not understand why in the past, but I think I could understand something now.
Probably because professional programmers really confident about what they have
wrote. As they have been tested and/or reviewed thousands or ten thousands times.
Enable this debug output by default can only prove to the community that you are
not confident about something, either the community's reviewing power on DTS or
your debug techniques.
This is why
this patch references it in the Fixes: tag, making sure that this patch
will get backported to any stable kernel that includes commit
00084f0c01bf.
No, I keep insist on my judgement. A fixes tag is only meant for cases where your
patch fixes a bug. The bug should really be happened. All of the discussion ongoing
here are just things imaginary about the *debug* phase and development phase.
As far as I understand, this is business as usual. There's
nothing personal here, and no judgement on the quality of your code.
Please don't misunderstanding, I do cares the quality of my code.
If it is really introduce a bug, I will responsible and help to solve.
But this is not the case. Sorry.
Signed-off-by: Laurent Pinchart<laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/bridge/thc63lvd1024.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
index 5f99f9724081..674efc489e3a 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -125,8 +125,11 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
remote = of_graph_get_remote_node(thc63->dev->of_node,
THC63_RGB_OUT0, -1);
- if (!remote)
+ if (!remote) {
+ dev_err(thc63->dev, "No remote endpoint for port@%u\n",
+ THC63_RGB_OUT0);
return -ENODEV;
+ }
An side effect of this patch is that we will add one more extra error message in the console.
As the of_graph_get_remote_node() function already print one for us if I add '#define DEBUG 1'
on the top of this source file. What's worse, it does not really tell us what's really the
error is.
It could be no valid endpoint or no valid remote node because of bad coding in DT, or It is
also simply because the remove node(or device) is being disabled intentionally by adding
'status = "disabled"' clause. Therefore, the error printing code added here is very confusing
in practice. It cannot really help for locating the root cause of the problem.
After think about this more than twice, either help to improve the core of_graph_get_remote_node()
function or just to drop this. This what I can tell as a ordinary reviewer. Despite you and/or
other more advanced programmer & reviewer could override what I said though.
