Re: [PATCH v3 08/15] drm/tidss: Add mechanism to detect DPI output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] DPI output detection misidentifies AM65x OLDI ports as DPI, causing corruption of DPI clock configuration.
- [Low] Manual parsing of device tree 'compatible' property instead of using the standard `of_device_is_compatible()` API.
--

commit b4b73a906f813456b4eaed059b154ca4c691e76b
Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

drm/tidss: Add mechanism to detect DPI output

This commit adds a mechanism to determine if an output is going to a DPI
output or not, by checking if it is not DSI or OLDI. It uses a helper to
check bridge compatible strings and parent nodes to make this determination.

> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 8bb93194e5ac6..bc8b10af9a48e 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c

[ ... ]

> +static bool tidss_is_bridge_internal(struct tidss_device *tidss,
> +				     struct drm_bridge *bridge)
> +{
> +	struct device_node *parent, *grand_parent;
> +	struct property *prop;
> +	bool is_internal;
> +
> +	if (WARN_ON(!bridge->of_node))
> +		return false;
> +
> +	prop = of_find_property(bridge->of_node, "compatible", NULL);
> +	for (const char *cp = of_prop_next_string(prop, NULL); cp;
> +	     cp = of_prop_next_string(prop, cp)) {
> +		for (unsigned int i = 0;
> +		     i < ARRAY_SIZE(tidss_internal_bridge_compatibles); ++i) {
> +			if (strcmp(cp, tidss_internal_bridge_compatibles[i]) == 0)
> +				return true;
> +		}
> +	}

[Severity: Low]
Is it necessary to manually parse the compatible property string here?
Using the standard of_device_is_compatible() API would be safer and
avoid reinventing the wheel for property iteration.

> +	parent = of_get_parent(bridge->of_node);
> +	grand_parent = of_get_parent(parent);
> +
> +	is_internal = parent && grand_parent &&
> +		      tidss->dev->of_node == grand_parent &&
> +		      of_node_name_eq(parent, "oldi-transmitters");
> +
> +	of_node_put(grand_parent);
> +	of_node_put(parent);
> +
> +	return is_internal;
> +}

[ ... ]

> @@ -160,6 +206,11 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
>  			return dev_err_probe(dev, ret, "port %d probe failed\n", i);
>  		}
>  
> +		if (bridge)
> +			dpi_output = !tidss_is_bridge_internal(tidss, bridge);
> +		else
> +			dpi_output = true;
> +

[Severity: High]
Could this logic misidentify AM65x OLDI ports as DPI?

On the AM65x SoC, the OLDI transmitter is integrated directly into the DSS
without a separate oldi-transmitters node in the device tree.

If a panel is directly connected to the AM65x OLDI port, bridge will be NULL.
If an external LVDS bridge is connected, it will not be recognized as an
internal bridge. In both cases, the OLDI port seems like it would be
unconditionally marked as dpi_output = true.

Since dpi_output is used to configure the shared DPI clock control register,
would preparing the OLDI port incorrectly write its own polarity
configuration and corrupt the actual DPI port's configuration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-beagley-ai-display-v3-0-7fefdc5d1adf@ideasonboard.com?part=8