[PATCH] drm: bridge: thc63lvd1024: Print error message when DT parsing fails

[Neil Armstrong]

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 at ideasonboard.com>
>>>> ---
>>>>    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.
>