Re: [PATCH v6 3/5] drm/bridge: analogix_dp: Restore mandatory samsung DP DT properties

[Damon Ding <damon.ding@rock-chips.com>]

Hi Luca,

On 8/19/2026 3:32 PM, Luca Ceresoli wrote:
> Hello Damon,
> 
> On Wed Jul 15, 2026 at 10:32 AM CEST, Damon Ding wrote:
>> On 7/7/2026 12:17 PM, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>> - [Critical] Failing probe when deprecated DT properties are missing breaks DT ABI and prevents their deprecation.
>>> --
>>>
>>> --- Patch [3]: [PATCH v6 3/5] drm/bridge: analogix_dp: Restore mandatory samsung DP DT properties ---
>>> commit b573dba50acc6087122ffa4a65680322f4d004f8
>>> Author: Damon Ding <damon.ding@rock-chips.com>
>>>
>>> drm/bridge: analogix_dp: Restore mandatory samsung DP DT properties
>>>
>>> This commit reverts a change that made the samsung,link-rate and
>>> samsung,lane-count properties optional for Exynos DP. It adds error checking
>>> to fail the probe early if these properties are missing, aiming to prevent
>>> link training failures caused by zero values.
>>>
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> index 8cf6b73bceac0..293d8adfe9856 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> [ ... ]
>>>> @@ -1258,10 +1259,14 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
>>>>    		 * NOTE: those property parseing code is used for
>>>>    		 * providing backward compatibility for samsung platform.
>>>>    		 */
>>>> -		of_property_read_u32(dp_node, "samsung,link-rate",
>>>> -				     &video_info->max_link_rate);
>>>> -		of_property_read_u32(dp_node, "samsung,lane-count",
>>>> -				     &video_info->max_lane_count);
>>>> +		if (of_property_read_u32(dp_node, "samsung,link-rate", &val))
>>>> +			return dev_err_probe(dp->dev, -EINVAL,
>>>> +					     "Failed to get samsung,link-rate\n");
>>>
>>> [Severity: Critical]
>>> Does failing the probe when this property is missing break the DT ABI?
>>>
>>> Since samsung,link-rate is a deprecated property, a modern schema-compliant
>>> Device Tree might intentionally omit it. If a system boots with such a DT,
>>> this check will fail the probe with -EINVAL and disable the display.
>>>
>>> Could the driver instead fall back to hardware maximums or sensible defaults
>>> when the deprecated property is absent, rather than returning an error?
>>>
>>
>> I'm afraid it's difficult to provide a universal sensible default value
>> here.
>>
>> The samsung,lane-count and samsung,link-rate values vary significantly
>> across Exynos DP device trees, so a generic fallback to hardware
>> maximums cannot guarantee stable link training.
>>
>> Even though these two properties are marked deprecated, all existing DTS
>> files in our projects have already populated them completely. Moreover,
>> the current link training logic relies on these two parameters; omitting
>> either will directly cause link training failures at runtime.
>>
>> Therefore, retaining the mandatory property check in probe is a
>> reasonable approach for now.
> 
> I'm still not convinced we should turn an optional property into mandatory,
> after having been optional since 2016 (based on the commit in the Fixes:
> tag).
> 
> The sure thing is we cannot break existing devices which upgrade to a new
> kernel.
> 
> Se the question is: if a device is missing one of these properties, or
> both, what happens? The video_info->max_link_rate and/or
> video_info->max_lane_count will be zero, correct? And if they are zero, is
> there even a remote possibility that the device will work somehow, maybe
> only with some rare low resolution or whatever?
> 
> If the answer is "yes, there is a remote possibility that one sich device,
> with some maybe rare configuration, will work", then no, we cannot make
> this property mandatory now. There can be devices out there working without
> these proberties, and they would be broken.
> 
> If the answer is "there is no way at all a device can work without one or
> both properties", with a good explanation based on the code flow and
> hardware docs, then we can consider this change.
> 

Sorry for the confusion, I just submitted the v7 series which crossed 
with your reply.

To answer your question: there is no way at all a device can work
without these properties. Here is the code flow when either
max_link_rate or max_lane_count is 0 (helped by AI):

   analogix_dp_commit()
     -> analogix_dp_full_link_train(dp, max_lanes = 0, max_rate = 0)

   analogix_dp_full_link_train(max_lanes, max_rate):
       // Read sink capabilities via DPCD and sanitize them
       link_rate   = read_dpcd(DP_MAX_LINK_RATE); // >= 0x06 after fixup
       lane_count  = read_dpcd(DP_MAX_LANE_COUNT);// >= 1 after fixup

       // Clamp by the limits from DT
       if (link_rate > max_rate)                // 0x06 > 0, always true
           link_rate = max_rate;                // link_rate = 0
       if (lane_count > max_lanes)              // 1 > 0, always true
           lane_count = max_lanes;              // lane_count = 0

       // Configure TX with the zeroed values
       set_link_bandwidth(link_rate = 0)
           // writel() is only executed for bwtype == 0x06/0x0a,
           // so LINK_BW_SET is never written and stays at reset value;
           // phy_configure() is called with link_rate = 0.

       set_lane_count(lane_count = 0)
           // writel(0, ANALOGIX_DP_LANE_COUNT_SET) enables 0 lanes;
           // phy_configure() is called with lanes = 0.

       // Program sink for link training
       drm_dp_dpcd_write(DP_LINK_BW_SET, {link_rate = 0, lane_count = 0})
           // DP spec requires link rate in {0x06, 0x0a, 0x14} and
           // lane count in {1, 2, 4}. Writing zeros is illegal, so the
           // sink cannot enter the training state.

       // Training loop
       for (lane = 0; lane < lane_count /* 0 */; lane++)
           // loop body never executes; training_lane[] stays
           // uninitialized and no training register is ever programmed

Since the sanitized sink values are always non-zero (link_rate >= 0x06,
lane_count >= 1), the clamping with a zero maximum unconditionally
forces the training parameters to zero. Clock recovery can never be 
achieved, so link training fails deterministically.

Best regards,
Damon