Re: [PATCH v2 04/12] phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] PHY driving parameters are bulk written to the wrong starting register offset, corrupting hardware state.
- [High] Missing NULL check on the result of `device_get_match_data()` leads to a NULL pointer dereference.
- [Low] The array `off_dig_lane` in `mt8195_dp_phy_data` is initialized using a compound literal, causing a compilation error.
- [Low] The variable `ret` is used without being declared in `mtk_dp_phy_probe`, causing a build failure.
- [Low] Missing `#include <linux/pm_runtime.h>` for `devm_pm_runtime_enable`.

Pre-existing issues:
- [High] Dereferencing `dev->platform_data` before verifying it is not `NULL` causes a kernel panic.
--

--- Patch [4]: [PATCH v2 04/12] phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata ---
commit b50fb23a21796e73379a3d3b300932c7ffb998c8
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata

This commit migrates the hardcoded register offsets in the MediaTek DP PHY
driver to SoC-specific platform data. It also refactors the driving
parameter initialization to loop through all lanes instead of hardcoding
each lane.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index b042ab67ebaf3..915f148325c49 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -94,15 +143,21 @@ static int mtk_dp_phy_init(struct phy *phy)
>  		DRIVING_PARAM_7_DEFAULT,
>  		DRIVING_PARAM_8_DEFAULT
>  	};
> -
> -	regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE0_DRIVING_PARAM_3,
> -			  driving_params, ARRAY_SIZE(driving_params));

[ ... ]

> +	int i, ret;
> +
> +	/*
> +	 * Assume that all lanes need the same driving parameters: this
> +	 * will bulk write from DRIVING_PARAM_0 to DRIVING_PARAM_8 on
> +	 * all lanes (a grand total of [9 * num_lanes] 32-bit writes)
> +	 */
> +	for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++) {
> +		ret = regmap_bulk_write(dp_phy->regmap,
> +					pdata->off_dig_lane[i] + reg,
> +					driving_params,
> +					ARRAY_SIZE(driving_params));

[Severity: High]
Does this bulk write start at the correct register offset?

The loop writes the driving_params array starting at the base offset for
DP_PHYD_LAN_DRIVING_PARAM_0. However, the driving_params array only contains 6
elements (defaults for PARAM_3 through PARAM_8).

Will this incorrectly write parameters 3-8 into the registers for 0-5, leaving
registers 6-8 uninitialized and corrupting the hardware PHY configuration?

> +		if (ret)
> +			return ret;
> +	};
>  
>  	return 0;
>  }

[ ... ]

> @@ -169,12 +233,19 @@ static void mtk_dp_phy_legacy_remove_lookup(void *data)
>  	phy_remove_lookup(phy, "dp", dev_name(dp_phy->dev));
>  }
>  
> +static const struct mtk_dp_phy_pdata mt8195_dp_phy_data;
> +
>  static int mtk_dp_phy_legacy_probe(struct platform_device *pdev, struct mtk_dp_phy *dp_phy)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct phy *phy;
>  	int ret;
>  
> +	/*
> +	 * If legacy platform driver probe, assume this is MT8195 or compatible
> +	 * with a devicetree that was not migrated to the new, proper bindings.
> +	 */
> +	dp_phy->pdata = &mt8195_dp_phy_data;
>  	dp_phy->regmap = *(struct regmap **)dev->platform_data;

[Severity: High]
This is a pre-existing issue, but can dev->platform_data be NULL here?

The code directly dereferences dev->platform_data to assign dp_phy->regmap
before checking if it is NULL on the next line. If a device is probed without
devicetree and platform data is missing, will this trigger a kernel panic?

>  	if (!dp_phy->regmap)
>  		return dev_err_probe(dev, -EINVAL, "No platform data available\n");

[ ... ]

> @@ -227,6 +298,8 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(dp_phy->regmap))
>  		return PTR_ERR(dp_phy->regmap);
>  
> +	dp_phy->pdata = device_get_match_data(dev);

[Severity: High]
Could device_get_match_data() return NULL here?

If the driver is manually bound via sysfs without a matching OF compatible
(but the device has an of_node), device_get_match_data() will return NULL.

Since dp_phy->pdata is later unconditionally dereferenced in functions like
mtk_dp_phy_init() to look up register offsets, will this lead to a NULL pointer
dereference?

> +
>  	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);

[ ... ]

[Severity: Low]
Is the variable ret declared in the scope of mtk_dp_phy_probe?

It was noted that ret might be assigned the result of devm_pm_runtime_enable()
and evaluated:

ret = devm_pm_runtime_enable(dev);
if (ret)
    return ret;

But it appears it might not be declared within the local scope, which could
cause a compilation error.

[Severity: Low]
Should <linux/pm_runtime.h> be included in this file?

If devm_pm_runtime_enable() is used, it relies on the Power Management
API. Relying on implicit transitive inclusion for Power Management APIs can be
fragile and may cause a build failure.

> @@ -245,8 +318,15 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
> +	.off_dig_glb = 0x1000,
> +	.off_dig_lane = (const u16[]) { 0x1100, 0x1200, 0x1300, 0x1400 },

[Severity: Low]
Will this array initialization compile correctly?

In standard C, an array member inside a struct should be initialized using a
brace-enclosed list. Using a compound literal cast like (const u16[]) decays
to a pointer, which may cause a compilation error for an array type initializer.

> +	.regs_dig_glb = mt8195_phy_dig_glb_regs,
> +	.regs_dig_lane = mt8195_phy_dig_lane_regs,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707141931.191172-1-angelogioacchino.delregno@collabora.com?part=4

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy