Re: [PATCH v3 2/2] dt-bindings: display: sn65dsi83: Add output data-lanes

Re: [PATCH v3 2/2] dt-bindings: display: sn65dsi83: Add output data-lanes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mismatch between device tree schema and driver implementation for the `data-lanes` property.
--

--- Patch [2]: [PATCH v3 2/2] dt-bindings: display: sn65dsi83: Add output data-lanes ---
commit 98aea58cd5128c6a4b72efd4a5f9ec088fd30dca
Author: Wojciech Dubowik <Wojciech.Dubowik@mt.com>

dt-bindings: display: sn65dsi83: Add output data-lanes

This commit adds an optional output LVDS data lanes property with two allowed
values. It supports the standard layout <1 2 3 4> and reversed layout <4 3 2 1>
for output channels.

> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> index e69b6343a8ebb..4431f113bb9cd 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> @@ -86,10 +86,52 @@ properties:
>          description: Video port for LVDS Channel-A output (panel or bridge).
>          $ref: '#/$defs/lvds-port'
>  
> +        properties:
> +          data-lanes:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array

[Severity: Medium]
Does this schema change place data-lanes at the correct level in the
device tree hierarchy?

The schema defines data-lanes as a direct property of the port node,
but standard OF graph rules and the driver implementation parse it from the
endpoint node:

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_parse_lvds_endpoint() {
    ...
    ret_data = of_property_read_u32_array(endpoint, "data-lanes", data_lanes,
                    ARRAY_SIZE(data_lanes));
    ...
}

Because the driver reads from the endpoint node rather than the port node,
will it silently fail to find the configuration and fall back to the default
layout on affected hardware?

> +            description: |
> +              Array of physical LVDS data lane indexes
> +            oneOf:

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710082630.394040-1-wojciech.dubowik@mt.com?part=2

Re: [PATCH v3 1/2] drm/bridge: ti-sn65dsi83: Add reversed lvds lanes support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Probe definitively fails on valid legacy 3-lane or 0-indexed device trees due to rigid array bounds.
--

--- Patch [1]: [PATCH v3 1/2] drm/bridge: ti-sn65dsi83: Add reversed lvds lanes support ---
commit 47fc41c5744e78e6f8072b1226e9548c69361da5
Author: Wojciech Dubowik <Wojciech.Dubowik@mt.com>

drm/bridge: ti-sn65dsi83: Add reversed lvds lanes support

The chip supports output lvds lanes in two orders, default <1 2 3 4>
and <4 3 2 1>. Add parsing of an optional output lvds data-lanes
property so we can inform chip that the lanes have been reversed.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
[ ... ]
> @@ -854,6 +873,37 @@ static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel)
>  		goto exit;
>  	}
>  
> +	ret_data = of_property_read_u32_array(endpoint, "data-lanes", data_lanes,
> +			ARRAY_SIZE(data_lanes));
> +	if (ret_data != 0 && ret_data != -EINVAL) {

[Severity: High]
Does this code break valid legacy device trees that define fewer than 4 lanes?

If a device tree has a data-lanes property on the LVDS endpoint with fewer
than 4 lanes (e.g., <1 2 3>), of_property_read_u32_array() will return
-EOVERFLOW because it rigidly expects exactly ARRAY_SIZE(data_lanes) elements.
This will immediately abort the probe, breaking previously working displays.

> +		ret = ret_data;
> +		goto exit;
> +	}
> +
> +	if (!ret_data) {
> +		for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
> +			for (j = 0; j < DATA_LANES_COUNT; j++) {
> +				if (data_lanes[j] != supported_data_lane_mapping[i][j])
> +					break;
> +			}
> +
> +			if (j == DATA_LANES_COUNT)
> +				break;
> +		}
> +
> +		switch (i) {
> +		case NORMAL_LANE_MAPPING:
> +			break;
> +		case REVERSE_LANE_MAPPING:
> +			ctx->lvds_reverse_lanes_conf[channel] = true;
> +			break;
> +		default:
> +			dev_err(dev, "invalid data lanes mapping\n");
> +			ret = -EINVAL;

[Severity: High]
Will this cause the probe to fail on 0-indexed device tree configurations?

The array comparison logic above this switch strictly checks for 1-indexed
mappings. If the device tree uses a valid 0-indexed scheme (e.g., <0 1 2 3>),
it falls through to this default case, causing the probe to fail with -EINVAL.

> +			goto exit;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710082630.394040-1-wojciech.dubowik@mt.com?part=1

Re: [PATCH v2] drm/bridge: ti-sn65dsi83: Add reversed lvds lanes support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Strict parsing of the `data-lanes` property breaks device probe for valid configurations with fewer than 4 lanes or non-matching lane orderings.
--

--- Patch [1]: [PATCH v2] drm/bridge: ti-sn65dsi83: Add reversed lvds lanes support ---
commit 33b38b449204d2ada7177a7a3ded61d90bf26441
Author: Wojciech Dubowik <Wojciech.Dubowik@mt.com>

drm/bridge: ti-sn65dsi83: Add reversed lvds lanes support

This commit parses an optional data-lanes property on the LVDS endpoint to
inform the chip if the lanes have been reversed.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index e6dbe51d0dba9..d676ea3b40e87 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c

[ ... ]

> @@ -854,6 +873,37 @@ static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel)
>  		goto exit;
>  	}
>  
> +	ret_data = of_property_read_u32_array(endpoint, "data-lanes", data_lanes,
> +			ARRAY_SIZE(data_lanes));
> +	if (ret_data != 0 && ret_data != -EINVAL) {
> +		ret = ret_data;
> +		goto exit;
> +	}

[Severity: Medium]
Will this break the device probe for valid configurations with fewer than 4
lanes?

If a device tree specifies a valid data-lanes property on the LVDS endpoint
that has fewer than 4 elements (such as <1 2 3> for an 18-bpp display using
3 lanes), of_property_read_u32_array() will return -EOVERFLOW.

Since -EOVERFLOW is not -EINVAL, the probe will be aborted here.

> +
> +	if (!ret_data) {
> +		for (i = 0; i < ARRAY_SIZE(supported_data_lane_mapping); i++) {
> +			for (j = 0; j < DATA_LANES_COUNT; j++) {
> +				if (data_lanes[j] != supported_data_lane_mapping[i][j])
> +					break;
> +			}
> +
> +			if (j == DATA_LANES_COUNT)
> +				break;
> +		}
> +
> +		switch (i) {
> +		case NORMAL_LANE_MAPPING:
> +			break;
> +		case REVERSE_LANE_MAPPING:
> +			ctx->lvds_reverse_lanes_conf[channel] = true;
> +			break;
> +		default:
> +			dev_err(dev, "invalid data lanes mapping\n");
> +			ret = -EINVAL;
> +			goto exit;

[Severity: Medium]
Does this code also incorrectly reject valid 4-element arrays with alternative
numbering?

If the mapping doesn't exactly match the two hardcoded 1-based arrays (for
example, a 0-indexed mapping like <0 1 2 3>), it falls through to this
default case, returning -EINVAL and aborting the probe.

Could we make the parsing more lenient to avoid breaking existing setups
that specify valid but non-matching lane orderings?

> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710082630.394040-1-wojciech.dubowik@mt.com?part=1