[PATCH 00/12] PHY: MediaTek DP PHY refactor and MT8196 eDP

[PATCH 00/12] PHY: MediaTek DP PHY refactor and MT8196 eDP

[AngeloGioacchino Del Regno]

This series performs major refactoring on the MediaTek DisplayPort PHY
driver, makes it probe with devicetree instead of getting registered
by the mtk_dp DRM driver, adds power_on/off() callbacks, and honors
the phy configure opts' set_lanes and set_voltages for, respectively,
varying the number of lanes and setting the voltage pre-emphasis and
swing on the PHY, for each lane.

This driver now also properly gets the PHY (EYE) Calibration Data from
NVMEM (eFuse array) if provided, instead of getting it (improperly)
injected by the mtk_dp driver.

Additionally, all of the driving parameters calculations and most of
the other register definitions were refactored to greatly enhance the
human readability of this code.

As a last step, this also transfers the register offsets for both the
digital and analog phy registers in arrays assigned to soc specific
data, in an effort to both introduce support for new minor revisions
of the MediaTek DisplayPort PHY and to have a clearer view of the
register related differences between those (for example, it is easily
understandable that the analog part remained exactly the same between
MT8195 and MT8196, but the digital part gets a slight update).

Speaking of which, as a last step, this also adds support for the
MT8196 SoC (and its derivatives), which uses this PHY only for its
Embedded DisplayPort (eDP) IP (spoiler: the DP one seems to be way
too different and requiring an entirely new PHY driver).

In this state, this driver can also easily support the MT8189 SoC
with a few lines of code: even though I do have clean code to add
support for this one, I was not (*yet*) able to test it on upstream
based kernels, and for this reason I decided to leave that one out
for now (but it's coming later for sure).

NOTE!
Despite all the apparently breaking changes in the refactoring, full
compatibility with older MTK_DP driver and with old devicetrees was
retained and carefully tested on multiple platforms!

P.S.: I am aware of the BUILD_DRIVING_PARAM_0( 0, 2, 4, 7) checkpatch
warning and I didn't fix it in bigger favor of human readability.

AngeloGioacchino Del Regno (12):
  dt-bindings: phy: Document MT8195 and MT8196 DisplayPort PHYs
  phy: phy-mtk-dp: Rename regs to regmap in struct mtk_dp_phy
  phy: phy-mtk-dp: Allow probing with devicetree match
  phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata
  phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks
  phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup
  phy: phy-mtk-dp: Support setting volt swing and preemphasis values
  phy: phy-mtk-dp: Add support for digital and analog calibration
  phy: phy-mtk-dp: Rewrite and document default driving param macros
  phy: phy-mtk-dp: Add bitrate register val definitions to SoC data
  phy: phy-mtk-dp: Add PHYD Lane EN register mask to SoC data
  phy: phy-mtk-dp: Add support for MT8196 eDP PHY

 .../bindings/phy/mediatek,mt8195-dp-phy.yaml  |  77 ++
 drivers/phy/mediatek/phy-mtk-dp.c             | 828 ++++++++++++++++--
 2 files changed, 808 insertions(+), 97 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml

-- 
2.54.0

[PATCH 01/12] dt-bindings: phy: Document MT8195 and MT8196 DisplayPort PHYs

[AngeloGioacchino Del Regno]

This adds bindings for the DisplayPort and Embedded DisplayPort
PHYs found in the MediaTek MT8195 SoC (and variants of) and for
the Embedded DisplayPort found in the MT8196 SoC (and variants).

This PHY supports varying impedance calibrations for the various
signals to reach an optimal EYE signal pattern for any specific
board(s), especially useful for very high bitrates such as HBR3
and higher, depending on board design.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../bindings/phy/mediatek,mt8195-dp-phy.yaml  | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml
new file mode 100644
index 000000000000..5847963a7085
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/mediatek,mt8195-dp-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek SoC DisplayPort Transmitter PHY
+
+maintainers:
+  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt8195-dp-phy
+      - mediatek,mt8196-edp-phy
+
+  reg:
+    maxItems: 1
+
+  "#phy-cells":
+    const: 0
+
+  nvmem-cells:
+    description: PHY calibrations from eFuse for optimal EYE signal pattern
+    items:
+      - description: PHY-Global Reference Bias trim
+      - description: PHY-Global AUX Transmitter clock impedance adjustment
+      - description: Lane 0 Transmitter impedance selection (P-MOSFET)
+      - description: Lane 0 Transmitter impedance selection (N-MOSFET)
+      - description: Lane 1 Transmitter impedance selection (P-MOSFET)
+      - description: Lane 1 Transmitter impedance selection (N-MOSFET)
+      - description: Lane 2 Transmitter impedance selection (P-MOSFET)
+      - description: Lane 2 Transmitter impedance selection (N-MOSFET)
+      - description: Lane 3 Transmitter impedance selection (P-MOSFET)
+      - description: Lane 3 Transmitter impedance selection (N-MOSFET)
+
+  nvmem-cell-names:
+    items:
+      - const: rbias-trim
+      - const: impedance-txclk
+      - const: impedance-lane0p
+      - const: impedance-lane0n
+      - const: impedance-lane1p
+      - const: impedance-lane1n
+      - const: impedance-lane2p
+      - const: impedance-lane2n
+      - const: impedance-lane3p
+      - const: impedance-lane3n
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    phy@1c500000 {
+        compatible = "mediatek,mt8195-dp-phy";
+        reg = <0 0x1c500000 0 0x2000>;
+        #phy-cells = <0>;
+        nvmem-cells = <&edp_glb_bias_trim>, <&edp_clktx_impsel>,
+                      <&edp_imp_ln0_pmos>, <&edp_imp_ln0_nmos>,
+                      <&edp_imp_ln1_pmos>, <&edp_imp_ln1_nmos>,
+                      <&edp_imp_ln2_pmos>, <&edp_imp_ln2_nmos>,
+                      <&edp_imp_ln3_pmos>, <&edp_imp_ln3_nmos>;
+        nvmem-cell-names = "rbias-trim", "impedance-txclk",
+                           "impedance-lane0p", "impedance-lane0n",
+                           "impedance-lane1p", "impedance-lane1n",
+                           "impedance-lane2p", "impedance-lane2n",
+                           "impedance-lane3p", "impedance-lane3n";
+    };
-- 
2.54.0

Re: [PATCH 01/12] dt-bindings: phy: Document MT8195 and MT8196 DisplayPort PHYs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The device tree example will fail `make dt_binding_check` validation due to a 64-bit `reg` property at the root level without `#address-cells = <2>` and `#size-cells = <2>`.
--

commit c7b476cde050e045aaa56bae9dd7ecc7b3d34ced
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
dt-bindings: phy: Document MT8195 and MT8196 DisplayPort PHYs

This adds bindings for the DisplayPort and Embedded DisplayPort PHYs found in
the MediaTek MT8195 and MT8196 SoCs. The PHY supports varying impedance
calibrations for the signals to reach an optimal EYE signal pattern.

> diff --git a/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml
> new file mode 100644
> index 0000000000000..5847963a7085e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml

[ ... ]

> +examples:
> +  - |
> +    phy@1c500000 {
> +        compatible = "mediatek,mt8195-dp-phy";
> +        reg = <0 0x1c500000 0 0x2000>;

[Severity: Low]
Looking at the examples section in mediatek,mt8195-dp-phy.yaml, will this
example pass make dt_binding_check validation? The reg property contains 4
cells, but since the example node is at the root level without a parent node
explicitly defining #address-cells = <2> and #size-cells = <2>, the dummy
root node defaults to 1 for both. This causes the 4 cells to be parsed as 2
separate items, which violates the schema's maxItems: 1 constraint.

> +        #phy-cells = <0>;
> +        nvmem-cells = <&edp_glb_bias_trim>, <&edp_clktx_impsel>,
> +                      <&edp_imp_ln0_pmos>, <&edp_imp_ln0_nmos>,
> +                      <&edp_imp_ln1_pmos>, <&edp_imp_ln1_nmos>,
> +                      <&edp_imp_ln2_pmos>, <&edp_imp_ln2_nmos>,
> +                      <&edp_imp_ln3_pmos>, <&edp_imp_ln3_nmos>;
> +        nvmem-cell-names = "rbias-trim", "impedance-txclk",
> +                           "impedance-lane0p", "impedance-lane0n",
> +                           "impedance-lane1p", "impedance-lane1n",
> +                           "impedance-lane2p", "impedance-lane2n",
> +                           "impedance-lane3p", "impedance-lane3n";
> +    };

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

Re: [PATCH 01/12] dt-bindings: phy: Document MT8195 and MT8196 DisplayPort PHYs

[AngeloGioacchino Del Regno]

On 7/1/26 14:27, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The device tree example will fail `make dt_binding_check` validation due to a 64-bit `reg` property at the root level without `#address-cells = <2>` and `#size-cells = <2>`.

That's not "Low", that should be "High" - failing dt_binding_check validation is
critical enough here.

Anyway, will allow some time for reviews before fixing that.

> --
> 
> commit c7b476cde050e045aaa56bae9dd7ecc7b3d34ced
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> dt-bindings: phy: Document MT8195 and MT8196 DisplayPort PHYs
> 
> This adds bindings for the DisplayPort and Embedded DisplayPort PHYs found in
> the MediaTek MT8195 and MT8196 SoCs. The PHY supports varying impedance
> calibrations for the signals to reach an optimal EYE signal pattern.
> 
>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml
>> new file mode 100644
>> index 0000000000000..5847963a7085e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml
> 
> [ ... ]
> 
>> +examples:
>> +  - |
>> +    phy@1c500000 {
>> +        compatible = "mediatek,mt8195-dp-phy";
>> +        reg = <0 0x1c500000 0 0x2000>;
> 
> [Severity: Low]
> Looking at the examples section in mediatek,mt8195-dp-phy.yaml, will this
> example pass make dt_binding_check validation? The reg property contains 4
> cells, but since the example node is at the root level without a parent node
> explicitly defining #address-cells = <2> and #size-cells = <2>, the dummy
> root node defaults to 1 for both. This causes the 4 cells to be parsed as 2
> separate items, which violates the schema's maxItems: 1 constraint.
> 
>> +        #phy-cells = <0>;
>> +        nvmem-cells = <&edp_glb_bias_trim>, <&edp_clktx_impsel>,
>> +                      <&edp_imp_ln0_pmos>, <&edp_imp_ln0_nmos>,
>> +                      <&edp_imp_ln1_pmos>, <&edp_imp_ln1_nmos>,
>> +                      <&edp_imp_ln2_pmos>, <&edp_imp_ln2_nmos>,
>> +                      <&edp_imp_ln3_pmos>, <&edp_imp_ln3_nmos>;
>> +        nvmem-cell-names = "rbias-trim", "impedance-txclk",
>> +                           "impedance-lane0p", "impedance-lane0n",
>> +                           "impedance-lane1p", "impedance-lane1n",
>> +                           "impedance-lane2p", "impedance-lane2n",
>> +                           "impedance-lane3p", "impedance-lane3n";
>> +    };
> 

Re: [PATCH 01/12] dt-bindings: phy: Document MT8195 and MT8196 DisplayPort PHYs

[Rob Herring (Arm)]

On Wed, 01 Jul 2026 14:19:57 +0200, AngeloGioacchino Del Regno wrote:
> This adds bindings for the DisplayPort and Embedded DisplayPort
> PHYs found in the MediaTek MT8195 SoC (and variants of) and for
> the Embedded DisplayPort found in the MT8196 SoC (and variants).
> 
> This PHY supports varying impedance calibrations for the various
> signals to reach an optimal EYE signal pattern for any specific
> board(s), especially useful for very high bitrates such as HBR3
> and higher, depending on board design.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../bindings/phy/mediatek,mt8195-dp-phy.yaml  | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/mediatek,mt8195-dp-phy.example.dtb: phy@1c500000 (mediatek,mt8195-dp-phy): reg: [[0, 475004928], [0, 8192]] is too long
	from schema $id: http://devicetree.org/schemas/phy/mediatek,mt8195-dp-phy.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20260701122008.19509-2-angelogioacchino.delregno@collabora.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH 02/12] phy: phy-mtk-dp: Rename regs to regmap in struct mtk_dp_phy

[AngeloGioacchino Del Regno]

In preparation to perform further cleanups and to extend the
driver to support more SoCs, rename the `regs` member to `regmap`
to improve readability, as this is a common name across many
kernel drivers for a struct regmap.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index d7024a144335..bf7b3a95e72d 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -79,7 +79,7 @@
 				 XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT)
 
 struct mtk_dp_phy {
-	struct regmap *regs;
+	struct regmap *regmap;
 };
 
 static int mtk_dp_phy_init(struct phy *phy)
@@ -94,13 +94,13 @@ static int mtk_dp_phy_init(struct phy *phy)
 		DRIVING_PARAM_8_DEFAULT
 	};
 
-	regmap_bulk_write(dp_phy->regs, MTK_DP_LANE0_DRIVING_PARAM_3,
+	regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE0_DRIVING_PARAM_3,
 			  driving_params, ARRAY_SIZE(driving_params));
-	regmap_bulk_write(dp_phy->regs, MTK_DP_LANE1_DRIVING_PARAM_3,
+	regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE1_DRIVING_PARAM_3,
 			  driving_params, ARRAY_SIZE(driving_params));
-	regmap_bulk_write(dp_phy->regs, MTK_DP_LANE2_DRIVING_PARAM_3,
+	regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE2_DRIVING_PARAM_3,
 			  driving_params, ARRAY_SIZE(driving_params));
-	regmap_bulk_write(dp_phy->regs, MTK_DP_LANE3_DRIVING_PARAM_3,
+	regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE3_DRIVING_PARAM_3,
 			  driving_params, ARRAY_SIZE(driving_params));
 
 	return 0;
@@ -131,10 +131,10 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 			val = BIT_RATE_HBR3;
 			break;
 		}
-		regmap_write(dp_phy->regs, MTK_DP_PHY_DIG_BIT_RATE, val);
+		regmap_write(dp_phy->regmap, MTK_DP_PHY_DIG_BIT_RATE, val);
 	}
 
-	regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_PLL_CTL_1,
+	regmap_update_bits(dp_phy->regmap, MTK_DP_PHY_DIG_PLL_CTL_1,
 			   TPLL_SSC_EN, opts->dp.ssc ? TPLL_SSC_EN : 0);
 
 	return 0;
@@ -144,10 +144,10 @@ static int mtk_dp_phy_reset(struct phy *phy)
 {
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
 
-	regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_SW_RST,
+	regmap_update_bits(dp_phy->regmap, MTK_DP_PHY_DIG_SW_RST,
 			   DP_GLB_SW_RST_PHYD, 0);
 	usleep_range(50, 200);
-	regmap_update_bits(dp_phy->regs, MTK_DP_PHY_DIG_SW_RST,
+	regmap_update_bits(dp_phy->regmap, MTK_DP_PHY_DIG_SW_RST,
 			   DP_GLB_SW_RST_PHYD, 1);
 
 	return 0;
@@ -176,7 +176,7 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
 	if (!dp_phy)
 		return -ENOMEM;
 
-	dp_phy->regs = regs;
+	dp_phy->regmap = regs;
 	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
 	if (IS_ERR(phy))
 		return dev_err_probe(dev, PTR_ERR(phy),
-- 
2.54.0

Re: [PATCH 02/12] phy: phy-mtk-dp: Rename regs to regmap in struct mtk_dp_phy

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing NULL check for `dev->platform_data` before dereferencing.
- [High] Missing `remove` callback to clean up PHY lookup leads to a memory leak and use-after-free.
--

commit a1a96e429cf3345472aae1b06f6ffe5b31edee81
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Rename regs to regmap in struct mtk_dp_phy

In preparation to perform further cleanups and to extend the driver to
support more SoCs, rename the regs member to regmap to improve readability,
as this is a common name across many kernel drivers for a struct regmap.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index d7024a1443358..bf7b3a95e72d2 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> @@ -176,7 +176,7 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)

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

Earlier in mtk_dp_phy_probe(), dev->platform_data is dereferenced directly
without a check:

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
	...
	struct regmap *regs;

	regs = *(struct regmap **)dev->platform_data;
	if (!regs)
	...
}

If the driver is manually bound via sysfs (or otherwise instantiated without
platform data), dev->platform_data will be NULL.

Should the code validate that dev->platform_data is not NULL before
dereferencing it to avoid a kernel panic?

[Severity: High]
This isn't a bug introduced by this patch, but does this driver leak the
phy_lookup struct and leave a dangling pointer if it gets unbound?

In mtk_dp_phy_probe(), when !dev->of_node is true, a lookup entry is
registered:

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
	...
	if (!dev->of_node)
		phy_create_lookup(phy, "dp", dev_name(dev));

	return 0;
}

However, mtk_dp_phy_driver does not have a .remove callback to call
phy_remove_lookup():

drivers/phy/mediatek/phy-mtk-dp.c:
static struct platform_driver mtk_dp_phy_driver = {
	.probe = mtk_dp_phy_probe,
	.driver = {
		.name = "mediatek-dp-phy",
	},
};

If the driver is unbound, the phy_lookup struct is leaked and remains linked
in the global phys list. Since the phy struct gets freed by the
device-managed subsystem, subsequent device probes iterating the PHY lookup
list would trigger a use-after-free.

Could a .remove callback be added to unregister the lookup and prevent this?

>  	if (!dp_phy)
>  		return -ENOMEM;
>  
> -	dp_phy->regs = regs;
> +	dp_phy->regmap = regs;
>  	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
>  	if (IS_ERR(phy))
>  		return dev_err_probe(dev, PTR_ERR(phy),

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

Re: [PATCH 02/12] phy: phy-mtk-dp: Rename regs to regmap in struct mtk_dp_phy

[AngeloGioacchino Del Regno]

On 7/1/26 14:26, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing NULL check for `dev->platform_data` before dereferencing.
> - [High] Missing `remove` callback to clean up PHY lookup leads to a memory leak and use-after-free.
> --
> 
> commit a1a96e429cf3345472aae1b06f6ffe5b31edee81
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> phy: phy-mtk-dp: Rename regs to regmap in struct mtk_dp_phy
> 
> In preparation to perform further cleanups and to extend the driver to
> support more SoCs, rename the regs member to regmap to improve readability,
> as this is a common name across many kernel drivers for a struct regmap.
> 
>> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
>> index d7024a1443358..bf7b3a95e72d2 100644
>> --- a/drivers/phy/mediatek/phy-mtk-dp.c
>> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
> [ ... ]
>> @@ -176,7 +176,7 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for dev->platform_data to be
> NULL here?
> 
> Earlier in mtk_dp_phy_probe(), dev->platform_data is dereferenced directly
> without a check:
> 
> drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
> 	...
> 	struct regmap *regs;
> 
> 	regs = *(struct regmap **)dev->platform_data;
> 	if (!regs)
> 	...
> }
> 
> If the driver is manually bound via sysfs (or otherwise instantiated without
> platform data), dev->platform_data will be NULL.
> 
> Should the code validate that dev->platform_data is not NULL before
> dereferencing it to avoid a kernel panic?
> 

No, that's not crashing it, because there's a NULL check anyway just after that.

> [Severity: High]
> This isn't a bug introduced by this patch, but does this driver leak the
> phy_lookup struct and leave a dangling pointer if it gets unbound?
> 
> In mtk_dp_phy_probe(), when !dev->of_node is true, a lookup entry is
> registered:
> 
> drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
> 	...
> 	if (!dev->of_node)
> 		phy_create_lookup(phy, "dp", dev_name(dev));
> 
> 	return 0;
> }
> 
> However, mtk_dp_phy_driver does not have a .remove callback to call
> phy_remove_lookup():
> 
> drivers/phy/mediatek/phy-mtk-dp.c:
> static struct platform_driver mtk_dp_phy_driver = {
> 	.probe = mtk_dp_phy_probe,
> 	.driver = {
> 		.name = "mediatek-dp-phy",
> 	},
> };
> 
> If the driver is unbound, the phy_lookup struct is leaked and remains linked
> in the global phys list. Since the phy struct gets freed by the
> device-managed subsystem, subsequent device probes iterating the PHY lookup
> list would trigger a use-after-free.
> 
> Could a .remove callback be added to unregister the lookup and prevent this?
> 

That's right, but I didn't create this issue. I can fix that in a later commit
just for that, even though this code path is dead when using new style devicetree.

>>   	if (!dp_phy)
>>   		return -ENOMEM;
>>   
>> -	dp_phy->regs = regs;
>> +	dp_phy->regmap = regs;
>>   	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
>>   	if (IS_ERR(phy))
>>   		return dev_err_probe(dev, PTR_ERR(phy),
> 

[PATCH 03/12] phy: phy-mtk-dp: Allow probing with devicetree match

[AngeloGioacchino Del Regno]

Make it possible to decouple the registration of the DisplayPort
PHY driver from the DisplayPort IP driver by adding a devicetree
match to probe the PHY, registering an OF PHY provider and this
device's own MMIO regmap - if, and only if, this PHY driver was
registered with an OF match.

In order to retain compatibility with older devicetrees that are
not declaring the DisplayPort PHY as a separate node, the legacy
code was moved in a `mtk_dp_phy_legacy_probe()` function, which
gets called if the driver was registered by the DisplayPort one.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 70 ++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index bf7b3a95e72d..98e05fe05ce3 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -79,6 +79,7 @@
 				 XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT)
 
 struct mtk_dp_phy {
+	struct device *dev;
 	struct regmap *regmap;
 };
 
@@ -160,43 +161,92 @@ static const struct phy_ops mtk_dp_phy_dev_ops = {
 	.owner = THIS_MODULE,
 };
 
+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;
+
+	dp_phy->regmap = *(struct regmap **)dev->platform_data;
+	if (!dp_phy->regmap)
+		return dev_err_probe(dev, -EINVAL, "No platform data available\n");
+
+	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
+	if (IS_ERR(phy))
+		return dev_err_probe(dev, PTR_ERR(phy),
+				     "Failed to create DP PHY\n");
+
+	phy_set_drvdata(phy, dp_phy);
+	phy_create_lookup(phy, "dp", dev_name(dev));
+
+	return 0;
+}
+
+static const struct regmap_config mtk_dp_phy_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.disable_locking = true,
+};
+
 static int mtk_dp_phy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct phy_provider *provider;
 	struct mtk_dp_phy *dp_phy;
+	void __iomem *base;
 	struct phy *phy;
-	struct regmap *regs;
-
-	regs = *(struct regmap **)dev->platform_data;
-	if (!regs)
-		return dev_err_probe(dev, -EINVAL,
-				     "No data passed, requires struct regmap**\n");
 
 	dp_phy = devm_kzalloc(dev, sizeof(*dp_phy), GFP_KERNEL);
 	if (!dp_phy)
 		return -ENOMEM;
 
-	dp_phy->regmap = regs;
+	dp_phy->dev = dev;
+
+	/* If there's no devicetree, go for legacy pdev probe */
+	if (!dev->of_node)
+		return mtk_dp_phy_legacy_probe(pdev, dp_phy);
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	dp_phy->regmap = devm_regmap_init_mmio(dev, base, &mtk_dp_phy_regmap_cfg);
+	if (IS_ERR(dp_phy->regmap))
+		return PTR_ERR(dp_phy->regmap);
+
 	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
 	if (IS_ERR(phy))
 		return dev_err_probe(dev, PTR_ERR(phy),
 				     "Failed to create DP PHY\n");
 
 	phy_set_drvdata(phy, dp_phy);
-	if (!dev->of_node)
-		phy_create_lookup(phy, "dp", dev_name(dev));
+
+	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(provider))
+		return PTR_ERR(provider);
+
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 
 	return 0;
 }
 
+static const struct of_device_id mtk_dp_phy_of_match[] = {
+	{ .compatible = "mediatek,mt8195-dp-phy" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mtk_dp_phy_of_match);
+
 static struct platform_driver mtk_dp_phy_driver = {
 	.probe = mtk_dp_phy_probe,
 	.driver = {
 		.name = "mediatek-dp-phy",
+		.of_match_table = mtk_dp_phy_of_match,
 	},
 };
 module_platform_driver(mtk_dp_phy_driver);
 
+MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>");
 MODULE_AUTHOR("Markus Schneider-Pargmann <msp@baylibre.com>");
-MODULE_DESCRIPTION("MediaTek DP PHY Driver");
+MODULE_DESCRIPTION("MediaTek DisplayPort PHY Driver");
 MODULE_LICENSE("GPL");
-- 
2.54.0

Re: [PATCH 03/12] phy: phy-mtk-dp: Allow probing with devicetree match

[sashiko-bot]

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

New issues:
- [High] The return value of `pm_runtime_get_sync()` is ignored, which can lead to hardware register accesses on an unpowered device.
- [Medium] Missing `.remove` callback leaves PM runtime enabled and usage counter unbalanced on driver unbind.

Pre-existing issues:
- [High] Missing `phy_remove_lookup()` causes a memory leak and a dangling pointer in the global PHY lookup list.
- [High] Unconditional dereference of `dev->platform_data` before validating it.
--

commit e3a3844cc6f977665174375e23735f789c04a772
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Allow probing with devicetree match

This commit introduces a devicetree match table to decouple the DP PHY driver
from the DP IP driver. It adds an OF PHY provider and MMIO regmap registration
for the devicetree case, while maintaining a legacy probe path for older
devicetrees without a separate PHY node.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index bf7b3a95e72d2..98e05fe05ce3b 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -160,43 +161,92 @@ static const struct phy_ops mtk_dp_phy_dev_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +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;
> +
> +	dp_phy->regmap = *(struct regmap **)dev->platform_data;

[Severity: High]
This is a pre-existing issue, but does this code unconditionally dereference
dev->platform_data before verifying it is not NULL?

If the platform device is instantiated without platform data, would this
dereference cause a kernel panic before the subsequent check for
!dp_phy->regmap can execute?

> +	if (!dp_phy->regmap)
> +		return dev_err_probe(dev, -EINVAL, "No platform data available\n");
> +
> +	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
> +	if (IS_ERR(phy))
> +		return dev_err_probe(dev, PTR_ERR(phy),
> +				     "Failed to create DP PHY\n");
> +
> +	phy_set_drvdata(phy, dp_phy);
> +	phy_create_lookup(phy, "dp", dev_name(dev));

[Severity: High]
This isn't a bug introduced by this patch, but does this leak the lookup
entry and create a dangling pointer?

phy_create_lookup() dynamically allocates a phy_lookup structure and links it
into the global phys list. Since there is no driver .remove callback calling
phy_remove_lookup(), when the driver unbinds, the devres-managed phy is freed
while still in the global list, which could cause a use-after-free when other
drivers call phy_find().

> +
> +	return 0;
> +}
> +

[ ... ]

>  static int mtk_dp_phy_probe(struct platform_device *pdev)
>  {

[ ... ]

> +	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(provider))
> +		return PTR_ERR(provider);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

[Severity: High]
Is it safe to ignore the return value of pm_runtime_get_sync() here?

If the generic power domain or clock fails to enable, pm_runtime_get_sync()
will return a negative error code. Without checking this, the PHY provider is
registered even if the hardware is off, which could lead to a synchronous
external abort when attempting to write to the MMIO registers later.

>  
>  	return 0;
>  }
>  
> +static const struct of_device_id mtk_dp_phy_of_match[] = {
> +	{ .compatible = "mediatek,mt8195-dp-phy" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mtk_dp_phy_of_match);
> +
>  static struct platform_driver mtk_dp_phy_driver = {
>  	.probe = mtk_dp_phy_probe,
>  	.driver = {
>  		.name = "mediatek-dp-phy",
> +		.of_match_table = mtk_dp_phy_of_match,
>  	},
>  };

[Severity: Medium]
Does this driver need a .remove callback to clean up the PM runtime state?

Since pm_runtime_enable() and pm_runtime_get_sync() are called in probe
without devm_ handlers or registered teardown actions, unbinding the driver
appears to leak the usage counter and leave PM runtime permanently enabled.

>  module_platform_driver(mtk_dp_phy_driver);
>

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

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

[AngeloGioacchino Del Regno]

In preparation for adding support for newer SoCs and for adding
more capabilities to this driver in an efficient manner, migrate
all of the hardcoded register offsets to SoC specific pdata and
assign that for both DT and platform probing.

While at it also cleanup writing the driving parameters to the PHY
by iterating through all lanes with a loop instead.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 138 +++++++++++++++++++++++-------
 1 file changed, 109 insertions(+), 29 deletions(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index 98e05fe05ce3..ce33f6812bae 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -4,6 +4,10 @@
  *
  * Copyright (c) 2022, BayLibre Inc.
  * Copyright (c) 2022, MediaTek Inc.
+ *
+ * Major refactoring
+ * Copyright (c) 2026, Collabora Ltd.
+ *                     AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
  */
 
 #include <linux/delay.h>
@@ -14,24 +18,25 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
-#define PHY_OFFSET			0x1000
+#define MTK_DP_PHY_MAX_LANES		4
 
-#define MTK_DP_PHY_DIG_PLL_CTL_1	(PHY_OFFSET + 0x14)
+/* DP_PHYD_PLL_CTL_1 */
 #define TPLL_SSC_EN			BIT(3)
 
-#define MTK_DP_PHY_DIG_BIT_RATE		(PHY_OFFSET + 0x3C)
-#define BIT_RATE_RBR			0
-#define BIT_RATE_HBR			1
-#define BIT_RATE_HBR2			2
-#define BIT_RATE_HBR3			3
+/* DP_PHYD_BIT_RATE */
+#define PHYD_DIG_RG_BIT_RATE		GENMASK(1, 0)
+#  define BIT_RATE_RBR			0
+#  define BIT_RATE_HBR			1
+#  define BIT_RATE_HBR2			2
+#  define BIT_RATE_HBR3			3
 
-#define MTK_DP_PHY_DIG_SW_RST		(PHY_OFFSET + 0x38)
-#define DP_GLB_SW_RST_PHYD		BIT(0)
+/* DP_PHYD_SW_RST */
+#define PHYD_DIG_GLB_SW_RST_B		GENMASK(7, 0)
+#  define DP_GLB_SW_RST_PHYD		BIT(0)
+#  define DP_GLB_SW_RST_TFIFO_ANA	BIT(1)
+#  define DP_GLB_SW_RST_XTAL_CLK	BIT(2)
+#  define DP_GLB_SW_RST_MAIN_LINK	BIT(3)
 
-#define MTK_DP_LANE0_DRIVING_PARAM_3		(PHY_OFFSET + 0x138)
-#define MTK_DP_LANE1_DRIVING_PARAM_3		(PHY_OFFSET + 0x238)
-#define MTK_DP_LANE2_DRIVING_PARAM_3		(PHY_OFFSET + 0x338)
-#define MTK_DP_LANE3_DRIVING_PARAM_3		(PHY_OFFSET + 0x438)
 #define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT	BIT(4)
 #define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT	(BIT(10) | BIT(12))
 #define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT	GENMASK(20, 19)
@@ -78,14 +83,58 @@
 #define DRIVING_PARAM_8_DEFAULT	(XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT)
 
+enum mtk_dp_phyd_dig_lane_regidx {
+	DP_PHYD_LAN_DRIVING_PARAM_0,
+	DP_PHYD_LAN_MAX
+};
+
+enum mtk_dp_phyd_dig_glb_regidx {
+	DP_PHYD_PLL_CTL_0,
+	DP_PHYD_PLL_CTL_1,
+	DP_PHYD_SW_RST,
+	DP_PHYD_BIT_RATE,
+	DP_PHYD_GLOBAL_MAX
+};
+
+static const u8 mt8195_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
+	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
+};
+
+static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {
+	[DP_PHYD_PLL_CTL_0] = 0x10,
+	[DP_PHYD_PLL_CTL_1] = 0x14,
+	[DP_PHYD_SW_RST] = 0x38,
+	[DP_PHYD_BIT_RATE] = 0x3c,
+};
+
+/**
+ * struct mtk_dp_phy_pdata - Platform data and defaults for MediaTek DP/eDP PHY
+ * @off_dig_glb:    Base offset for dptx_phyd_sifslv_dig_glb
+ * @off_dig_lane:   Base offsets for dptx_phyd_sifslv_dig_lan (for each lane)
+ * @regs_dig_glb:   Register (layout) offsets for dig_glb
+ * @regs_dig_lane:  Register (layout) offsets for dig_lan
+ */
+struct mtk_dp_phy_pdata {
+	/* Register offsets */
+	u16 off_dig_glb;
+	u16 off_dig_lane[MTK_DP_PHY_MAX_LANES];
+
+	/* Register maps */
+	const u8 *regs_dig_glb;
+	const u8 *regs_dig_lane;
+};
+
 struct mtk_dp_phy {
 	struct device *dev;
 	struct regmap *regmap;
+	const struct mtk_dp_phy_pdata *pdata;
 };
 
 static int mtk_dp_phy_init(struct phy *phy)
 {
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
+	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
+	const u32 reg = pdata->regs_dig_lane[DP_PHYD_LAN_DRIVING_PARAM_0];
 	static const u32 driving_params[] = {
 		DRIVING_PARAM_3_DEFAULT,
 		DRIVING_PARAM_4_DEFAULT,
@@ -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));
-	regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE1_DRIVING_PARAM_3,
-			  driving_params, ARRAY_SIZE(driving_params));
-	regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE2_DRIVING_PARAM_3,
-			  driving_params, ARRAY_SIZE(driving_params));
-	regmap_bulk_write(dp_phy->regmap, MTK_DP_LANE3_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));
+		if (ret)
+			return ret;
+	};
 
 	return 0;
 }
@@ -110,9 +165,12 @@ static int mtk_dp_phy_init(struct phy *phy)
 static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 {
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
+	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
 	u32 val;
 
 	if (opts->dp.set_rate) {
+		const u32 reg_bit_rate = pdata->regs_dig_glb[DP_PHYD_BIT_RATE];
+
 		switch (opts->dp.link_rate) {
 		default:
 			dev_err(&phy->dev,
@@ -132,10 +190,11 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 			val = BIT_RATE_HBR3;
 			break;
 		}
-		regmap_write(dp_phy->regmap, MTK_DP_PHY_DIG_BIT_RATE, val);
+		regmap_write(dp_phy->regmap, pdata->off_dig_glb + reg_bit_rate, val);
 	}
 
-	regmap_update_bits(dp_phy->regmap, MTK_DP_PHY_DIG_PLL_CTL_1,
+	regmap_update_bits(dp_phy->regmap,
+			   pdata->off_dig_glb + pdata->regs_dig_glb[DP_PHYD_PLL_CTL_1],
 			   TPLL_SSC_EN, opts->dp.ssc ? TPLL_SSC_EN : 0);
 
 	return 0;
@@ -144,12 +203,17 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 static int mtk_dp_phy_reset(struct phy *phy)
 {
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
+	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
+	const u32 reg_rst = pdata->regs_dig_glb[DP_PHYD_SW_RST];
+
+	/* Clearing bits sets reset state */
+	regmap_clear_bits(dp_phy->regmap, pdata->off_dig_glb + reg_rst, DP_GLB_SW_RST_PHYD);
 
-	regmap_update_bits(dp_phy->regmap, MTK_DP_PHY_DIG_SW_RST,
-			   DP_GLB_SW_RST_PHYD, 0);
+	/* PHYD needs 50uS to guarantee reset done */
 	usleep_range(50, 200);
-	regmap_update_bits(dp_phy->regmap, MTK_DP_PHY_DIG_SW_RST,
-			   DP_GLB_SW_RST_PHYD, 1);
+
+	/* Setting bits means go out of reset */
+	regmap_set_bits(dp_phy->regmap, pdata->off_dig_glb + reg_rst, DP_GLB_SW_RST_PHYD);
 
 	return 0;
 }
@@ -161,11 +225,18 @@ static const struct phy_ops mtk_dp_phy_dev_ops = {
 	.owner = THIS_MODULE,
 };
 
+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;
 
+	/*
+	 * 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;
 	if (!dp_phy->regmap)
 		return dev_err_probe(dev, -EINVAL, "No platform data available\n");
@@ -214,6 +285,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);
+
 	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
 	if (IS_ERR(phy))
 		return dev_err_probe(dev, PTR_ERR(phy),
@@ -231,8 +304,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 },
+	.regs_dig_glb = mt8195_phy_dig_glb_regs,
+	.regs_dig_lane = mt8195_phy_dig_lane_regs,
+};
+
 static const struct of_device_id mtk_dp_phy_of_match[] = {
-	{ .compatible = "mediatek,mt8195-dp-phy" },
+	{ .compatible = "mediatek,mt8195-dp-phy", .data = &mt8195_dp_phy_data },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mtk_dp_phy_of_match);
-- 
2.54.0

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

[sashiko-bot]

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

New issues:
- [High] Missing NULL pointer check for `device_get_match_data()` return value leads to kernel panic.
- [Low] Build failure due to invalid initialization of an array field with a compound literal.

Pre-existing issues:
- [High] Missing driver `.remove` callback leads to unbalanced runtime PM usage counter and resource leak.
- [High] Dangling `phy_lookup` entry in the global list causes a Use-After-Free upon driver unbind.
- [High] Unconditional dereference of `dev->platform_data` before validating it for NULL.
--

commit 2a851e9d237369ac4340bdce02183defb68c9865
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata

This patch migrates the hardcoded register offsets in the MediaTek DP PHY driver
to SoC-specific platform data. It assigns this platform data for both DeviceTree
and platform probing. It also loops through lanes to clean up the writing of
driving parameters.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 98e05fe05ce3b..ce33f6812bae6 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> @@ -161,11 +225,18 @@ static const struct phy_ops mtk_dp_phy_dev_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +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;
>  
> +	/*
> +	 * 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 does this code unconditionally dereference
dev->platform_data before checking if it is NULL?

If the platform device is probed through the legacy path without an of_node and
without platform_data provided (which can happen by manually binding an
arbitrary compatible device via sysfs), could this lead to a kernel panic?

>  	if (!dp_phy->regmap)
>  		return dev_err_probe(dev, -EINVAL, "No platform data available\n");
[ ... ]
> @@ -214,6 +285,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]
Can device_get_match_data() return NULL here?

If a user with privileges to write to sysfs binds the device manually using
/sys/bus/platform/drivers/.../bind, or the device is instantiated via platform
name matching while having an of_node, device_get_match_data() will return
NULL.

Could this lead to a NULL pointer dereference resulting in a kernel panic when
dp_phy->pdata is accessed later?

>  	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
>  	if (IS_ERR(phy))
>  		return dev_err_probe(dev, PTR_ERR(phy),
[ ... ]
> @@ -231,8 +304,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 fail to compile under standard C compilers?

The off_dig_lane field is declared as an array
u16 off_dig_lane[MTK_DP_PHY_MAX_LANES], but it is being initialized using a
pointer from a compound literal cast (const u16[]) { ... }.

Assigning an explicitly cast compound literal to an array field is
syntactically invalid and could prevent the driver from building.

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

[Severity: High]
This is a pre-existing issue, but does the mtk_dp_phy_driver missing a .remove
callback lead to unbalanced runtime PM usage?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
    ...
    pm_runtime_enable(dev);
    pm_runtime_get_sync(dev);

    return 0;
}

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_driver {
    .probe = mtk_dp_phy_probe,
    .driver = {
        .name = "mediatek-dp-phy",
        .of_match_table = mtk_dp_phy_of_match,
    },
};

Without a .remove implementation to handle unbinding the driver, could the
device remain permanently powered on and fail to cleanly suspend or unbind?

[Severity: High]
This is also a pre-existing issue, but is the phy_lookup entry left dangling in
the global list when the driver is unbound?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_legacy_probe() {
    ...
    phy_create_lookup(phy, "dp", dev_name(dev));
    ...
}

Because there is no .remove callback, phy_remove_lookup() is never called.

Could a subsequent phy_get() call traversing the list access the freed
pointer, resulting in a Use-After-Free since devres automatically frees the
struct phy?

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

[PATCH 05/12] phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks

[AngeloGioacchino Del Regno]

Add .power_on() and .power_off() callbacks to mtk_dp_phy_dev_ops
to be able to call those with phy_power_on() and phy_power_off()
API in the DisplayPort driver to be able to stop using all of
those hardcoded register writes in that external driver.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 87 +++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index ce33f6812bae..586e72795633 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -20,6 +20,9 @@
 
 #define MTK_DP_PHY_MAX_LANES		4
 
+/* DP_PHYA_GLB_FORCE_CTRL_1 */
+#define CKM_CKTX0_EN_FORCE_MODE		BIT(10)
+
 /* DP_PHYD_PLL_CTL_1 */
 #define TPLL_SSC_EN			BIT(3)
 
@@ -37,6 +40,11 @@
 #  define DP_GLB_SW_RST_XTAL_CLK	BIT(2)
 #  define DP_GLB_SW_RST_MAIN_LINK	BIT(3)
 
+/* DP_PHYD_AUX_RX_CTL */
+#define PHYD_DIG_DPAUX_RX_EN		BIT(0)
+#define PHYD_DIG_XTP_GLB_CKDET_EN	BIT(1)
+#define PHYD_DIG_DPAUX_RX_DEGLITCH_EN	BIT(2)
+
 #define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT	BIT(4)
 #define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT	(BIT(10) | BIT(12))
 #define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT	GENMASK(20, 19)
@@ -83,6 +91,12 @@
 #define DRIVING_PARAM_8_DEFAULT	(XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT)
 
+enum mtk_dp_phya_ana_glb_regidx {
+	DP_PHYA_GLB_FORCE_CTRL_0,
+	DP_PHYA_GLB_FORCE_CTRL_1,
+	DP_PHYA_GLOBAL_MAX
+};
+
 enum mtk_dp_phyd_dig_lane_regidx {
 	DP_PHYD_LAN_DRIVING_PARAM_0,
 	DP_PHYD_LAN_MAX
@@ -93,9 +107,15 @@ enum mtk_dp_phyd_dig_glb_regidx {
 	DP_PHYD_PLL_CTL_1,
 	DP_PHYD_SW_RST,
 	DP_PHYD_BIT_RATE,
+	DP_PHYD_AUX_RX_CTL,
 	DP_PHYD_GLOBAL_MAX
 };
 
+static const u8 mt8195_phy_ana_glb_regs[DP_PHYA_GLOBAL_MAX] = {
+	[DP_PHYA_GLB_FORCE_CTRL_0] = 0x30,
+	[DP_PHYA_GLB_FORCE_CTRL_1] = 0x34,
+};
+
 static const u8 mt8195_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
 	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
 };
@@ -105,21 +125,26 @@ static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {
 	[DP_PHYD_PLL_CTL_1] = 0x14,
 	[DP_PHYD_SW_RST] = 0x38,
 	[DP_PHYD_BIT_RATE] = 0x3c,
+	[DP_PHYD_AUX_RX_CTL] = 0x40,
 };
 
 /**
  * struct mtk_dp_phy_pdata - Platform data and defaults for MediaTek DP/eDP PHY
+ * @off_ana_glb:    Base offset for dptx_phyd_sifslv_ana_glb
  * @off_dig_glb:    Base offset for dptx_phyd_sifslv_dig_glb
  * @off_dig_lane:   Base offsets for dptx_phyd_sifslv_dig_lan (for each lane)
+ * @regs_ana_glb:   Register (layout) offsets for ana_glb
  * @regs_dig_glb:   Register (layout) offsets for dig_glb
  * @regs_dig_lane:  Register (layout) offsets for dig_lan
  */
 struct mtk_dp_phy_pdata {
 	/* Register offsets */
+	u16 off_ana_glb;
 	u16 off_dig_glb;
 	u16 off_dig_lane[MTK_DP_PHY_MAX_LANES];
 
 	/* Register maps */
+	const u8 *regs_ana_glb;
 	const u8 *regs_dig_glb;
 	const u8 *regs_dig_lane;
 };
@@ -193,6 +218,17 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 		regmap_write(dp_phy->regmap, pdata->off_dig_glb + reg_bit_rate, val);
 	}
 
+	if (opts->dp.set_lanes) {
+		const u32 reg_dig_tx_ctl = pdata->regs_dig_glb[DP_PHYD_TX_CTL_0];
+
+		val = 0;
+		for (i = 0; i < opts->dp.lanes; i++)
+			val |= FIELD_PREP(PHYD_TX_LN_EN, i);
+
+		regmap_update_bits(dp_phy->regmap, pdata->off_dig_glb + reg_dig_tx_ctl,
+				   PHYD_TX_LN_EN, val);
+	}
+
 	regmap_update_bits(dp_phy->regmap,
 			   pdata->off_dig_glb + pdata->regs_dig_glb[DP_PHYD_PLL_CTL_1],
 			   TPLL_SSC_EN, opts->dp.ssc ? TPLL_SSC_EN : 0);
@@ -200,6 +236,53 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 	return 0;
 }
 
+static int mtk_dp_phy_power_on(struct phy *phy)
+{
+	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
+	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
+	const u8 *regs = pdata->regs_dig_glb;
+	int ret;
+
+	/* Enable AUX Channel with RX De-Glitch and input clock detection */
+	ret = regmap_write(dp_phy->regmap,
+			   pdata->off_dig_glb + regs[DP_PHYD_AUX_RX_CTL],
+			   PHYD_DIG_DPAUX_RX_EN |
+			   PHYD_DIG_XTP_GLB_CKDET_EN |
+			   PHYD_DIG_DPAUX_RX_DEGLITCH_EN);
+	if (ret)
+		return ret;
+
+	ret = regmap_clear_bits(dp_phy->regmap,
+				pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],
+				CKM_CKTX0_EN_FORCE_MODE);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mtk_dp_phy_power_off(struct phy *phy)
+{
+	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
+	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
+	const u8 *regs = pdata->regs_dig_glb;
+	int ret;
+
+	ret = regmap_set_bits(dp_phy->regmap,
+				pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],
+				CKM_CKTX0_EN_FORCE_MODE);
+	if (ret)
+		return ret;
+
+	/* Disable RX */
+	ret = regmap_write(dp_phy->regmap,
+			   pdata->off_dig_glb + regs[DP_PHYD_AUX_RX_CTL], 0);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int mtk_dp_phy_reset(struct phy *phy)
 {
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
@@ -220,6 +303,8 @@ static int mtk_dp_phy_reset(struct phy *phy)
 
 static const struct phy_ops mtk_dp_phy_dev_ops = {
 	.init = mtk_dp_phy_init,
+	.power_on = mtk_dp_phy_power_on,
+	.power_off = mtk_dp_phy_power_off,
 	.configure = mtk_dp_phy_configure,
 	.reset = mtk_dp_phy_reset,
 	.owner = THIS_MODULE,
@@ -305,8 +390,10 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
 }
 
 static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
+	.off_ana_glb = 0x0,
 	.off_dig_glb = 0x1000,
 	.off_dig_lane = (const u16[]) { 0x1100, 0x1200, 0x1300, 0x1400 },
+	.regs_ana_glb = mt8195_phy_ana_glb_regs,
 	.regs_dig_glb = mt8195_phy_dig_glb_regs,
 	.regs_dig_lane = mt8195_phy_dig_lane_regs,
 };
-- 
2.54.0

Re: [PATCH 05/12] phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The power_on and power_off callbacks use the digital register layout array to look up an analog register offset, causing register operations to modify the wrong hardware offset.
- [Medium] The error path in `mtk_dp_phy_power_on()` fails to unwind previously applied hardware state if a subsequent step fails, leaking power state.
--

commit db1c6c4d46cae7dc24770e2f52553b8292fbfbc9
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks

Add .power_on() and .power_off() callbacks to mtk_dp_phy_dev_ops
to be able to call those with phy_power_on() and phy_power_off()
API in the DisplayPort driver to be able to stop using all of
those hardcoded register writes in that external driver.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index ce33f6812bae6..586e727956332 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -200,6 +236,53 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	return 0;
>  }
>  
> +static int mtk_dp_phy_power_on(struct phy *phy)
> +{
> +	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs = pdata->regs_dig_glb;
> +	int ret;
> +
> +	/* Enable AUX Channel with RX De-Glitch and input clock detection */
> +	ret = regmap_write(dp_phy->regmap,
> +			   pdata->off_dig_glb + regs[DP_PHYD_AUX_RX_CTL],
> +			   PHYD_DIG_DPAUX_RX_EN |
> +			   PHYD_DIG_XTP_GLB_CKDET_EN |
> +			   PHYD_DIG_DPAUX_RX_DEGLITCH_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(dp_phy->regmap,
> +				pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],

[Severity: High]
Here regs points to the digital register array pdata->regs_dig_glb, but it is
being indexed with DP_PHYA_GLB_FORCE_CTRL_1, which is an analog register
enum. Does this result in reading the wrong offset (fetching the offset for
DP_PHYD_BIT_RATE instead of DP_PHYA_GLB_FORCE_CTRL_1) and corrupting the
bitrate configuration?

> +				CKM_CKTX0_EN_FORCE_MODE);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If regmap_clear_bits() fails here, the function returns the error without
unwinding the previous successful write to DP_PHYD_AUX_RX_CTL. Since the PHY
core will not call phy_power_off() if phy_power_on() returns an error, does
this leave the RX channel and clock detection permanently enabled?

> +
> +	return 0;
> +}
> +
> +static int mtk_dp_phy_power_off(struct phy *phy)
> +{
> +	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs = pdata->regs_dig_glb;
> +	int ret;
> +
> +	ret = regmap_set_bits(dp_phy->regmap,
> +				pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],

[Severity: High]
Similarly to mtk_dp_phy_power_on(), this uses the digital register array
regs indexed with the analog enum DP_PHYA_GLB_FORCE_CTRL_1. Should this use
pdata->regs_ana_glb instead to avoid writing to the wrong hardware offset?

> +				CKM_CKTX0_EN_FORCE_MODE);
> +	if (ret)
> +		return ret;

[ ... ]

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

[PATCH 06/12] phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup

[AngeloGioacchino Del Regno]

Add support for enabling a specifically requested number of lanes
in the .configure() callback and disable all lanes in power off
and reset callbacks for proper hardware cleanup.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 49 ++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index 586e72795633..a2cd22b9da06 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -45,6 +45,9 @@
 #define PHYD_DIG_XTP_GLB_CKDET_EN	BIT(1)
 #define PHYD_DIG_DPAUX_RX_DEGLITCH_EN	BIT(2)
 
+/* DP_PHYD_TX_CTL_0 */
+#define PHYD_TX_LN_EN			GENMASK(7, 4)
+
 #define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT	BIT(4)
 #define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT	(BIT(10) | BIT(12))
 #define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT	GENMASK(20, 19)
@@ -108,6 +111,7 @@ enum mtk_dp_phyd_dig_glb_regidx {
 	DP_PHYD_SW_RST,
 	DP_PHYD_BIT_RATE,
 	DP_PHYD_AUX_RX_CTL,
+	DP_PHYD_TX_CTL_0,
 	DP_PHYD_GLOBAL_MAX
 };
 
@@ -126,6 +130,7 @@ static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {
 	[DP_PHYD_SW_RST] = 0x38,
 	[DP_PHYD_BIT_RATE] = 0x3c,
 	[DP_PHYD_AUX_RX_CTL] = 0x40,
+	[DP_PHYD_TX_CTL_0] = 0x44,
 };
 
 /**
@@ -192,6 +197,7 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
 	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
 	u32 val;
+	int i;
 
 	if (opts->dp.set_rate) {
 		const u32 reg_bit_rate = pdata->regs_dig_glb[DP_PHYD_BIT_RATE];
@@ -223,7 +229,7 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 
 		val = 0;
 		for (i = 0; i < opts->dp.lanes; i++)
-			val |= FIELD_PREP(PHYD_TX_LN_EN, i);
+			val |= FIELD_PREP(PHYD_TX_LN_EN, BIT(i));
 
 		regmap_update_bits(dp_phy->regmap, pdata->off_dig_glb + reg_dig_tx_ctl,
 				   PHYD_TX_LN_EN, val);
@@ -261,6 +267,35 @@ static int mtk_dp_phy_power_on(struct phy *phy)
 	return 0;
 }
 
+static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
+{
+	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
+	const u8 *regs = pdata->regs_dig_glb;
+	int ret;
+	u32 val;
+
+	ret = regmap_read(dp_phy->regmap, pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0], &val);
+	if (ret)
+		return ret;
+
+	/* Get mask of currently enabled lane */
+	val = FIELD_GET(PHYD_TX_LN_EN, val);
+
+	/* Disable all lanes (needs to be done one by one, from last to first) */
+	do {
+		u32 lane_num = fls(val) - 1;
+		val &= ~BIT(lane_num);
+
+		ret = regmap_clear_bits(dp_phy->regmap,
+					pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
+					FIELD_PREP(PHYD_TX_LN_EN, lane_num));
+		if (ret)
+			return ret;
+	} while (val);
+
+	return 0;
+}
+
 static int mtk_dp_phy_power_off(struct phy *phy)
 {
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
@@ -280,6 +315,12 @@ static int mtk_dp_phy_power_off(struct phy *phy)
 	if (ret)
 		return ret;
 
+	ret = mtk_dp_phy_disable_all_lanes(dp_phy);
+	if (ret) {
+		dev_err(dp_phy->dev, "Could not disable lanes for poweroff!\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -288,6 +329,7 @@ static int mtk_dp_phy_reset(struct phy *phy)
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
 	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
 	const u32 reg_rst = pdata->regs_dig_glb[DP_PHYD_SW_RST];
+	int ret;
 
 	/* Clearing bits sets reset state */
 	regmap_clear_bits(dp_phy->regmap, pdata->off_dig_glb + reg_rst, DP_GLB_SW_RST_PHYD);
@@ -298,6 +340,11 @@ static int mtk_dp_phy_reset(struct phy *phy)
 	/* Setting bits means go out of reset */
 	regmap_set_bits(dp_phy->regmap, pdata->off_dig_glb + reg_rst, DP_GLB_SW_RST_PHYD);
 
+	/* Disable all lanes and continue reset even if this fails, but notify */
+	ret = mtk_dp_phy_disable_all_lanes(dp_phy);
+	if (ret)
+		dev_err(dp_phy->dev, "Could not disable lanes during reset!\n");
+
 	return 0;
 }
 
-- 
2.54.0

Re: [PATCH 06/12] phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional do-while loop in `mtk_dp_phy_disable_all_lanes` causes an out-of-bounds bitwise shift and undefined behavior when no lanes are initially enabled.
- [High] `mtk_dp_phy_disable_all_lanes` passes a bit index instead of a bitmask to `FIELD_PREP`, failing to disable the correct lanes.
--

commit 25cdd51bd1ac4d69a49e77d93a6b1374ba46dd3c
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup

Add support for enabling a specifically requested number of lanes
in the .configure() callback and disable all lanes in power off
and reset callbacks for proper hardware cleanup.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 586e727956332..a2cd22b9da066 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> @@ -261,6 +267,35 @@ static int mtk_dp_phy_power_on(struct phy *phy)
>  	return 0;
>  }
>  
> +static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
> +{
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs = pdata->regs_dig_glb;
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(dp_phy->regmap, pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0], &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Get mask of currently enabled lane */
> +	val = FIELD_GET(PHYD_TX_LN_EN, val);
> +
> +	/* Disable all lanes (needs to be done one by one, from last to first) */
> +	do {
> +		u32 lane_num = fls(val) - 1;
> +		val &= ~BIT(lane_num);
                 ^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this cause undefined behavior if val is initially 0?

If the PHY register reports 0 enabled lanes (which can happen after a
hardware reset or during initial boot), val will be 0. Since this is an
unconditional do-while loop, fls(0) will return 0, leading to lane_num
evaluating to UINT_MAX.

The subsequent BIT(lane_num) attempt would result in an out-of-bounds
bitwise shift. Could this be changed to a while loop, or check if val is 0
before entering the loop?

> +
> +		ret = regmap_clear_bits(dp_phy->regmap,
> +					pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
> +					FIELD_PREP(PHYD_TX_LN_EN, lane_num));
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this clear the correct lanes in the register?

It looks like the bit index lane_num is passed directly to FIELD_PREP here
instead of the bitmask BIT(lane_num), which was correctly used in
mtk_dp_phy_configure().

If lane_num is 3, FIELD_PREP(GENMASK(7, 4), 3) will generate 0x30, which
clears lanes 0 and 1 instead of 0x80 (which would clear lane 3). This seems
like it might repeatedly attempt to clear the wrong lanes and leave higher
lanes enabled.

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

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

[PATCH 07/12] phy: phy-mtk-dp: Support setting volt swing and preemphasis values

[AngeloGioacchino Del Regno]

Add support for honoring the set_voltages request to set the
voltage swing and preemphasis values in the .configure() callback.

For proper hardware cleanup, reset both in the .reset() callback.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 41 ++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index a2cd22b9da06..17d871530cca 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -48,6 +48,11 @@
 /* DP_PHYD_TX_CTL_0 */
 #define PHYD_TX_LN_EN			GENMASK(7, 4)
 
+/* DP_PHYD_DRIVING_FORCE */
+#define PHYD_DP_TX_FORCE_VOLT_SWING_EN	BIT(0)
+#define PHYD_DP_TX_FORCE_VOLT_SWING_VAL	GENMASK(2, 1)
+#define PHYD_DP_TX_FORCE_PRE_EMPH_VAL	GENMASK(4, 3)
+
 #define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT	BIT(4)
 #define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT	(BIT(10) | BIT(12))
 #define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT	GENMASK(20, 19)
@@ -101,6 +106,7 @@ enum mtk_dp_phya_ana_glb_regidx {
 };
 
 enum mtk_dp_phyd_dig_lane_regidx {
+	DP_PHYD_LAN_DRIVING_FORCE,
 	DP_PHYD_LAN_DRIVING_PARAM_0,
 	DP_PHYD_LAN_MAX
 };
@@ -121,6 +127,7 @@ static const u8 mt8195_phy_ana_glb_regs[DP_PHYA_GLOBAL_MAX] = {
 };
 
 static const u8 mt8195_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
+	[DP_PHYD_LAN_DRIVING_FORCE] = 0x18,
 	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
 };
 
@@ -235,6 +242,28 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 				   PHYD_TX_LN_EN, val);
 	}
 
+	if (opts->dp.set_voltages) {
+		const u32 reg_drv_force = pdata->regs_dig_lane[DP_PHYD_LAN_DRIVING_FORCE];
+
+		if (opts->dp.lanes > 4) {
+			dev_err(&phy->dev, "Wrong lanes config %u\n", opts->dp.lanes);
+			return -EINVAL;
+		}
+
+		for (i = 0; i < opts->dp.lanes; i++) {
+			const u32 off_dig_lane = pdata->off_dig_lane[i];
+			u32 val;
+
+			val = FIELD_PREP(PHYD_DP_TX_FORCE_VOLT_SWING_VAL, opts->dp.voltage[i]);
+			val |= FIELD_PREP(PHYD_DP_TX_FORCE_PRE_EMPH_VAL, opts->dp.pre[i]);
+
+			regmap_update_bits(dp_phy->regmap, off_dig_lane + reg_drv_force,
+					   PHYD_DP_TX_FORCE_VOLT_SWING_VAL |
+					   PHYD_DP_TX_FORCE_PRE_EMPH_VAL,
+					   val);
+		}
+	}
+
 	regmap_update_bits(dp_phy->regmap,
 			   pdata->off_dig_glb + pdata->regs_dig_glb[DP_PHYD_PLL_CTL_1],
 			   TPLL_SSC_EN, opts->dp.ssc ? TPLL_SSC_EN : 0);
@@ -329,7 +358,8 @@ static int mtk_dp_phy_reset(struct phy *phy)
 	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
 	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
 	const u32 reg_rst = pdata->regs_dig_glb[DP_PHYD_SW_RST];
-	int ret;
+	const u32 reg_drv_force = pdata->regs_dig_lane[DP_PHYD_LAN_DRIVING_FORCE];
+	int i, ret;
 
 	/* Clearing bits sets reset state */
 	regmap_clear_bits(dp_phy->regmap, pdata->off_dig_glb + reg_rst, DP_GLB_SW_RST_PHYD);
@@ -345,6 +375,15 @@ static int mtk_dp_phy_reset(struct phy *phy)
 	if (ret)
 		dev_err(dp_phy->dev, "Could not disable lanes during reset!\n");
 
+	/* Reset Voltage Swing and Preemphasis values */
+	for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++) {
+		const u32 off_dig_lane = pdata->off_dig_lane[i];
+
+		regmap_clear_bits(dp_phy->regmap, off_dig_lane + reg_drv_force,
+				   PHYD_DP_TX_FORCE_VOLT_SWING_VAL |
+				   PHYD_DP_TX_FORCE_PRE_EMPH_VAL);
+	}
+
 	return 0;
 }
 
-- 
2.54.0

Re: [PATCH 07/12] phy: phy-mtk-dp: Support setting volt swing and preemphasis values

[sashiko-bot]

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

New issues:
- [High] The newly added PHYD_DP_TX_FORCE_VOLT_SWING_EN bit is never set or cleared, causing the hardware to likely ignore the configured voltage swing and pre-emphasis values.
- [High] Undefined behavior in `mtk_dp_phy_disable_all_lanes` when no lanes are currently enabled.

Pre-existing issues:
- [High] Missing `.remove` callback leads to Use-After-Free and unbalanced PM runtime states on module unload.
--

commit 3443073c225a00d56dcccbe80409c8537773ab64
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Support setting volt swing and preemphasis values

Add support for honoring the set_voltages request to set the voltage swing
and preemphasis values in the .configure() callback. It also updates the
.reset() callback to clean up the newly configured values.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index a2cd22b9da066..17d871530ccaf 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
> @@ -235,6 +242,28 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
>  				   PHYD_TX_LN_EN, val);
>  	}
>  
> +	if (opts->dp.set_voltages) {
> +		const u32 reg_drv_force = pdata->regs_dig_lane[DP_PHYD_LAN_DRIVING_FORCE];
> +
> +		if (opts->dp.lanes > 4) {
> +			dev_err(&phy->dev, "Wrong lanes config %u\n", opts->dp.lanes);
> +			return -EINVAL;
> +		}
> +
> +		for (i = 0; i < opts->dp.lanes; i++) {
> +			const u32 off_dig_lane = pdata->off_dig_lane[i];
> +			u32 val;
> +
> +			val = FIELD_PREP(PHYD_DP_TX_FORCE_VOLT_SWING_VAL, opts->dp.voltage[i]);
> +			val |= FIELD_PREP(PHYD_DP_TX_FORCE_PRE_EMPH_VAL, opts->dp.pre[i]);
> +
> +			regmap_update_bits(dp_phy->regmap, off_dig_lane + reg_drv_force,
> +					   PHYD_DP_TX_FORCE_VOLT_SWING_VAL |
> +					   PHYD_DP_TX_FORCE_PRE_EMPH_VAL,
> +					   val);

[Severity: High]
Does this override hardware configuration actually take effect?

Since PHYD_DP_TX_FORCE_VOLT_SWING_EN is not included in the update mask or
set in the value here, the hardware force enable bit remains unset. Will
the hardware ignore these override values without the enable bit being
asserted?

> +		}
> +	}
> +
>  	regmap_update_bits(dp_phy->regmap,
[ ... ]
> @@ -345,6 +375,15 @@ static int mtk_dp_phy_reset(struct phy *phy)
>  	if (ret)
>  		dev_err(dp_phy->dev, "Could not disable lanes during reset!\n");
>  

[Severity: High]
Is it possible for the loop in mtk_dp_phy_disable_all_lanes() to execute
with val initially set to 0?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_disable_all_lanes() {
    ...
    /* Get mask of currently enabled lane */
    val = FIELD_GET(PHYD_TX_LN_EN, val);

    /* Disable all lanes (needs to be done one by one, from last to first) */
    do {
        u32 lane_num = fls(val) - 1;
        val &= ~BIT(lane_num);
    ...
}

If no lanes are currently enabled, fls(0) - 1 evaluates to -1, which can
cause undefined behavior when shifting ~BIT(-1). Should this be a
while (val) loop instead of do { ... } while (val)?

> +	/* Reset Voltage Swing and Preemphasis values */
> +	for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++) {
> +		const u32 off_dig_lane = pdata->off_dig_lane[i];
> +
> +		regmap_clear_bits(dp_phy->regmap, off_dig_lane + reg_drv_force,
> +				   PHYD_DP_TX_FORCE_VOLT_SWING_VAL |
> +				   PHYD_DP_TX_FORCE_PRE_EMPH_VAL);

[Severity: High]
Does this correctly clear the override state?

Similar to the configuration above, this clears the swing and pre-emphasis
values but leaves the PHYD_DP_TX_FORCE_VOLT_SWING_EN bit untouched. If
the force mode was enabled elsewhere, could it remain active after reset?

> +	}
> +
>  	return 0;
>  }


[Severity: High]
This is a pre-existing issue, but I noticed the platform driver lacks a
.remove callback:

static struct platform_driver mtk_dp_phy_driver = {
	.probe = mtk_dp_phy_probe,
	.driver = { ... },
};

During probe, pm_runtime_enable() and pm_runtime_get_sync() are called, and
a phy_lookup structure is dynamically allocated via phy_create_lookup().

Because there is no .remove callback, unloading the module leaves PM runtime
unbalanced and leaks the phy_lookup struct, which remains on the global phys
list while pointing to a devres-freed phy device. Could this lead to a
use-after-free on subsequent lookups?

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

[PATCH 08/12] phy: phy-mtk-dp: Add support for digital and analog calibration

[AngeloGioacchino Del Regno]

Add support for reading the calibration values from eFuse: if
present, write those - otherwise, rely on the defaults from
SoC-specific data.
This also adds support for writing the calibration values for
the analog part of the PHY.

Note that before this change, only default hardcoded calibration
values were supported for the digital driving parameters.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 286 ++++++++++++++++++++++++++++--
 1 file changed, 267 insertions(+), 19 deletions(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index 17d871530cca..b1b526ee44eb 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -20,6 +21,19 @@
 
 #define MTK_DP_PHY_MAX_LANES		4
 
+/* DP_PHYA_GLB_BIAS_GEN_0 (PHYA - Analog) */
+#define XTP_GLB_BIAS_INT_R_CTRL		GENMASK(20, 16)
+
+/* DP_PHYA_GLB_FORCE_CTRL_1 */
+#define CKM_CKTX0_EN_FORCE_MODE		BIT(10)
+
+/* DP_PHYA_GLB_DPAUX_TX */
+#define CKM_PT0_CKTX_IMPSEL		GENMASK(23, 20)
+
+/* DP_PHYA_LAN_LANE_TX_0 */
+#define XTP_LN_TX_IMPSEL_PMOS		GENMASK(15, 12)
+#define XTP_LN_TX_IMPSEL_NMOS		GENMASK(19, 16)
+
 /* DP_PHYA_GLB_FORCE_CTRL_1 */
 #define CKM_CKTX0_EN_FORCE_MODE		BIT(10)
 
@@ -53,11 +67,29 @@
 #define PHYD_DP_TX_FORCE_VOLT_SWING_VAL	GENMASK(2, 1)
 #define PHYD_DP_TX_FORCE_PRE_EMPH_VAL	GENMASK(4, 3)
 
+/*
+ * DRIVING_PARAM_X (PHYD - Digital)
+ *
+ * Driving param registers are split in three sets, all containing settings
+ * for Voltage Swing and Pre-Emphasis for each lane's differential pair.
+ *
+ * All three sets share the same layout, but for different physical signals;
+ * In particular:
+ * [0-2]: LC TX CM (Minus / Negative Edge)
+ * [3-5]: LC TX C  (Logic State Change Point)
+ * [6-8]: LC TX CP (Plus / Positive Edge)
+ *
+ * And they contain values for:
+ * [0,3,6]: Swing 0 Pre[0-3]
+ * [1,4,7]: Swing 1 Pre[0-2] and Swing 2 Pre0
+ * [2,5,8]: Swing 2 Pre1 and Swing 3 Pre0
+ */
+#define PHYD_DIG_NUM_DRV_PARA_REGS	9
 #define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT	BIT(4)
 #define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT	(BIT(10) | BIT(12))
 #define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT	GENMASK(20, 19)
 #define XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT	GENMASK(29, 29)
-#define DRIVING_PARAM_3_DEFAULT	(XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT | \
+#define MT8195_DRIVING_PARAM_3_DEFAULT	(XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT | \
 				 XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT | \
 				 XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT | \
 				 XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT)
@@ -66,21 +98,21 @@
 #define XTP_LN_TX_LCTXC0_SW1_PRE1_DEFAULT	GENMASK(12, 9)
 #define XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT	(BIT(18) | BIT(21))
 #define XTP_LN_TX_LCTXC0_SW2_PRE0_DEFAULT	GENMASK(29, 29)
-#define DRIVING_PARAM_4_DEFAULT	(XTP_LN_TX_LCTXC0_SW1_PRE0_DEFAULT | \
+#define MT8195_DRIVING_PARAM_4_DEFAULT	(XTP_LN_TX_LCTXC0_SW1_PRE0_DEFAULT | \
 				 XTP_LN_TX_LCTXC0_SW1_PRE1_DEFAULT | \
 				 XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT | \
 				 XTP_LN_TX_LCTXC0_SW2_PRE0_DEFAULT)
 
 #define XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT	(BIT(3) | BIT(5))
 #define XTP_LN_TX_LCTXC0_SW3_PRE0_DEFAULT	GENMASK(13, 12)
-#define DRIVING_PARAM_5_DEFAULT	(XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT | \
+#define MT8195_DRIVING_PARAM_5_DEFAULT	(XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT | \
 				 XTP_LN_TX_LCTXC0_SW3_PRE0_DEFAULT)
 
 #define XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT	0
 #define XTP_LN_TX_LCTXCP1_SW0_PRE1_DEFAULT	GENMASK(10, 10)
 #define XTP_LN_TX_LCTXCP1_SW0_PRE2_DEFAULT	GENMASK(19, 19)
 #define XTP_LN_TX_LCTXCP1_SW0_PRE3_DEFAULT	GENMASK(28, 28)
-#define DRIVING_PARAM_6_DEFAULT	(XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT | \
+#define MT8195_DRIVING_PARAM_6_DEFAULT	(XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW0_PRE1_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW0_PRE2_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW0_PRE3_DEFAULT)
@@ -89,22 +121,30 @@
 #define XTP_LN_TX_LCTXCP1_SW1_PRE1_DEFAULT	GENMASK(10, 9)
 #define XTP_LN_TX_LCTXCP1_SW1_PRE2_DEFAULT	GENMASK(19, 18)
 #define XTP_LN_TX_LCTXCP1_SW2_PRE0_DEFAULT	0
-#define DRIVING_PARAM_7_DEFAULT	(XTP_LN_TX_LCTXCP1_SW1_PRE0_DEFAULT | \
+#define MT8195_DRIVING_PARAM_7_DEFAULT	(XTP_LN_TX_LCTXCP1_SW1_PRE0_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW1_PRE1_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW1_PRE2_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW2_PRE0_DEFAULT)
 
 #define XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT	GENMASK(3, 3)
 #define XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT	0
-#define DRIVING_PARAM_8_DEFAULT	(XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT | \
+#define MT8195_DRIVING_PARAM_8_DEFAULT	(XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT | \
 				 XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT)
 
 enum mtk_dp_phya_ana_glb_regidx {
+	DP_PHYA_GLB_BIAS_GEN_0,
+	DP_PHYA_GLB_BIAS_GEN_1,
+	DP_PHYA_GLB_DPAUX_TX,
 	DP_PHYA_GLB_FORCE_CTRL_0,
 	DP_PHYA_GLB_FORCE_CTRL_1,
 	DP_PHYA_GLOBAL_MAX
 };
 
+enum mtk_dp_phya_ana_lane_regidx {
+	DP_PHYA_LAN_LANE_TX_0,
+	DP_PHYA_LAN_MAX
+};
+
 enum mtk_dp_phyd_dig_lane_regidx {
 	DP_PHYD_LAN_DRIVING_FORCE,
 	DP_PHYD_LAN_DRIVING_PARAM_0,
@@ -122,10 +162,17 @@ enum mtk_dp_phyd_dig_glb_regidx {
 };
 
 static const u8 mt8195_phy_ana_glb_regs[DP_PHYA_GLOBAL_MAX] = {
+	[DP_PHYA_GLB_BIAS_GEN_0] = 0x0,
+	[DP_PHYA_GLB_BIAS_GEN_1] = 0x4,
+	[DP_PHYA_GLB_DPAUX_TX] = 0x8,
 	[DP_PHYA_GLB_FORCE_CTRL_0] = 0x30,
 	[DP_PHYA_GLB_FORCE_CTRL_1] = 0x34,
 };
 
+static const u8 mt8195_phy_ana_lane_regs[DP_PHYA_LAN_MAX] = {
+	[DP_PHYA_LAN_LANE_TX_0] = 0x4,
+};
+
 static const u8 mt8195_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
 	[DP_PHYD_LAN_DRIVING_FORCE] = 0x18,
 	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
@@ -140,46 +187,100 @@ static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {
 	[DP_PHYD_TX_CTL_0] = 0x44,
 };
 
+/**
+ * struct mtk_dp_phya_imp_sel - Per-Lane Impedance Selection
+ * @pmos: Impedance selection for P-Channel MOSFET
+ * @nmos: Impedance selection for N-Channel MOSFET
+ */
+struct mtk_dp_phya_imp_sel {
+	u8 pmos : 4;
+	u8 nmos : 4;
+};
+
 /**
  * struct mtk_dp_phy_pdata - Platform data and defaults for MediaTek DP/eDP PHY
  * @off_ana_glb:    Base offset for dptx_phyd_sifslv_ana_glb
+ * @off_ana_lane:   Base offsets for dptx_phyd_sifslv_ana_lan (for each lane)
  * @off_dig_glb:    Base offset for dptx_phyd_sifslv_dig_glb
  * @off_dig_lane:   Base offsets for dptx_phyd_sifslv_dig_lan (for each lane)
  * @regs_ana_glb:   Register (layout) offsets for ana_glb
+ * @regs_ana_lane:  Register (layout) offsets for ana_lan
  * @regs_dig_glb:   Register (layout) offsets for dig_glb
  * @regs_dig_lane:  Register (layout) offsets for dig_lan
+ * @ana_bias_r:     Internal resistance "R" Selection Settings (global)
+ * @ana_cktx_imp:   TX Clock Impedance Selection Settings (global)
+ * @ana_lanes_imp:  TX Impedance Selection Settings (for all lanes)
+ * @driving_params: Voltage Swing and Pre-Emphasis settings (for all lanes)
  */
 struct mtk_dp_phy_pdata {
 	/* Register offsets */
 	u16 off_ana_glb;
+	u16 off_ana_lane[MTK_DP_PHY_MAX_LANES];
 	u16 off_dig_glb;
 	u16 off_dig_lane[MTK_DP_PHY_MAX_LANES];
 
 	/* Register maps */
 	const u8 *regs_ana_glb;
+	const u8 *regs_ana_lane;
 	const u8 *regs_dig_glb;
 	const u8 *regs_dig_lane;
+
+	/* Calibration defaults */
+	u8 ana_bias_r;
+	u8 ana_cktx_imp;
+	struct mtk_dp_phya_imp_sel ana_lanes_imp;
+	u32 driving_params[PHYD_DIG_NUM_DRV_PARA_REGS];
 };
 
 struct mtk_dp_phy {
 	struct device *dev;
 	struct regmap *regmap;
 	const struct mtk_dp_phy_pdata *pdata;
+
+	u8 ana_bias_r;
+	u8 ana_cktx_imp;
+	struct mtk_dp_phya_imp_sel ana_impsel[MTK_DP_PHY_MAX_LANES];
+	bool efuse_cal_present;
 };
 
-static int mtk_dp_phy_init(struct phy *phy)
+static int mtk_dp_phy_set_analog_calibration_params(struct mtk_dp_phy *dp_phy)
+{
+	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
+	const u8 *regs_ana_glb = pdata->regs_ana_glb;
+	const u8 *regs_ana_lane = pdata->regs_ana_lane;
+	int i, ret;
+
+	ret = regmap_update_bits(dp_phy->regmap,
+				 pdata->off_ana_glb + regs_ana_glb[DP_PHYA_GLB_BIAS_GEN_0],
+				 XTP_GLB_BIAS_INT_R_CTRL, pdata->ana_bias_r);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(dp_phy->regmap,
+				 pdata->off_ana_glb + regs_ana_glb[DP_PHYA_GLB_DPAUX_TX],
+				 CKM_PT0_CKTX_IMPSEL, pdata->ana_cktx_imp);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++) {
+		struct mtk_dp_phya_imp_sel *ana_imp = &dp_phy->ana_impsel[i];
+		u32 val = FIELD_PREP(XTP_LN_TX_IMPSEL_PMOS, ana_imp->pmos) |
+			  FIELD_PREP(XTP_LN_TX_IMPSEL_NMOS, ana_imp->nmos);
+		u32 off_ana_lane = pdata->off_ana_lane[i];
+
+		ret = regmap_update_bits(dp_phy->regmap,
+					 off_ana_lane + regs_ana_lane[DP_PHYA_LAN_LANE_TX_0],
+					 XTP_LN_TX_IMPSEL_PMOS | XTP_LN_TX_IMPSEL_NMOS, val);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int mtk_dp_phy_set_digital_drv_params(struct mtk_dp_phy *dp_phy)
 {
-	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
 	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
 	const u32 reg = pdata->regs_dig_lane[DP_PHYD_LAN_DRIVING_PARAM_0];
-	static const u32 driving_params[] = {
-		DRIVING_PARAM_3_DEFAULT,
-		DRIVING_PARAM_4_DEFAULT,
-		DRIVING_PARAM_5_DEFAULT,
-		DRIVING_PARAM_6_DEFAULT,
-		DRIVING_PARAM_7_DEFAULT,
-		DRIVING_PARAM_8_DEFAULT
-	};
 	int i, ret;
 
 	/*
@@ -190,11 +291,31 @@ static int mtk_dp_phy_init(struct phy *phy)
 	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));
+					pdata->driving_params,
+					ARRAY_SIZE(pdata->driving_params));
 		if (ret)
 			return ret;
 	};
+	return 0;
+}
+
+static int mtk_dp_phy_init(struct phy *phy)
+{
+	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
+	struct device *dev = &phy->dev;
+	int ret;
+
+	ret = mtk_dp_phy_set_digital_drv_params(dp_phy);
+	if (ret) {
+		dev_err(dev, "Cannot set driving params\n");
+		return ret;
+	}
+
+	ret = mtk_dp_phy_set_analog_calibration_params(dp_phy);
+	if (ret) {
+		dev_err(dev, "Cannot set analog calibration\n");
+		return ret;
+	}
 
 	return 0;
 }
@@ -396,6 +517,109 @@ static const struct phy_ops mtk_dp_phy_dev_ops = {
 	.owner = THIS_MODULE,
 };
 
+static void mtk_dp_phy_get_default_cal_data(struct mtk_dp_phy *dp_phy)
+{
+	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
+	int i;
+
+	dp_phy->ana_bias_r = pdata->ana_bias_r;
+	dp_phy->ana_cktx_imp = pdata->ana_cktx_imp;
+
+	/* Copy the default lane impedance settings to all lanes */
+	for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++)
+		memcpy(&dp_phy->ana_impsel[i], &pdata->ana_lanes_imp,
+		       sizeof(dp_phy->ana_impsel[0]));
+
+	return;
+}
+
+static int mtk_dp_phy_get_one_cal_para(struct device *dev, const char *name, u8 max_val)
+{
+	u16 buf = 0;
+	int ret;
+
+	/*
+	 * All of the calibrations are always max 8 bits long, but some may
+	 * be split between two different 8-bits cells: handle this corner
+	 * case by retrying reading as u16.
+	 */
+	ret = nvmem_cell_read_u8(dev, name, (u8 *)&buf);
+	if (ret)
+		ret = nvmem_cell_read_u16(dev, name, &buf);
+
+	if (ret) {
+		dev_err(dev, "Cannot get calibration data for %s: %d\n", name, ret);
+		return ret;
+	};
+
+	if (buf == 0) {
+		dev_warn(dev, "No calibration for %s. Using defaults\n", name);
+		return -ENOENT;
+	}
+
+	if (buf > max_val) {
+		dev_err(dev, "Bad value %u retrieved for %s. Returning.\n", buf, name);
+		return -ERANGE;
+	};
+
+	return buf;
+}
+
+static int mtk_dp_phy_get_calibration_data(struct mtk_dp_phy *dp_phy)
+{
+	char mtk_dp_cal_lane_imp_name[] = "impedance-laneXM";
+	struct device *dev = dp_phy->dev;
+	int i, ret;
+
+	ret = mtk_dp_phy_get_one_cal_para(dev, "rbias-trim", FIELD_MAX(XTP_GLB_BIAS_INT_R_CTRL));
+	if (ret < 0)
+		goto end;
+	dp_phy->ana_bias_r = ret;
+
+	ret = mtk_dp_phy_get_one_cal_para(dev, "impedance-txclk", FIELD_MAX(CKM_PT0_CKTX_IMPSEL));
+	if (ret < 0)
+		goto end;
+	dp_phy->ana_cktx_imp = ret;
+
+	/* Get impedance params for each lane */
+	for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++) {
+		/* P-MOSFET first */
+		snprintf(mtk_dp_cal_lane_imp_name, ARRAY_SIZE(mtk_dp_cal_lane_imp_name),
+			 "impedance-lane%dp", i);
+		ret = mtk_dp_phy_get_one_cal_para(dev, mtk_dp_cal_lane_imp_name,
+						  FIELD_MAX(XTP_LN_TX_IMPSEL_PMOS));
+		if (ret < 0)
+			goto end;
+		dp_phy->ana_impsel[i].pmos = ret;
+
+		/* ...and then N-MOSFET too */
+		snprintf(mtk_dp_cal_lane_imp_name, ARRAY_SIZE(mtk_dp_cal_lane_imp_name),
+			 "impedance-lane%dn", i);
+		ret = mtk_dp_phy_get_one_cal_para(dev, mtk_dp_cal_lane_imp_name,
+						  FIELD_MAX(XTP_LN_TX_IMPSEL_PMOS));
+		if (ret < 0)
+			goto end;
+		dp_phy->ana_impsel[i].nmos = ret;
+	}
+end:
+	if (ret < 0) {
+		/*
+		 * If any of the calibration values is missing, or if there
+		 * is no calibration at all in the eFuses, copy the default
+		 * one entirely (as partial values shall not be mixed!)
+		 */
+		if (ret == -ENOENT) {
+			dev_info(dev, "Using calibration default values\n");
+			mtk_dp_phy_get_default_cal_data(dp_phy);
+			return 0;
+		}
+		return ret;
+	};
+	dp_phy->efuse_cal_present = true;
+
+	return 0;
+}
+
 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)
@@ -437,6 +661,7 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
 	struct mtk_dp_phy *dp_phy;
 	void __iomem *base;
 	struct phy *phy;
+	int ret;
 
 	dp_phy = devm_kzalloc(dev, sizeof(*dp_phy), GFP_KERNEL);
 	if (!dp_phy)
@@ -458,6 +683,10 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
 
 	dp_phy->pdata = device_get_match_data(dev);
 
+	ret = mtk_dp_phy_get_calibration_data(dp_phy);
+	if (ret)
+		return ret;
+
 	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
 	if (IS_ERR(phy))
 		return dev_err_probe(dev, PTR_ERR(phy),
@@ -476,12 +705,31 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
 }
 
 static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
-	.off_ana_glb = 0x0,
+	.off_ana_glb = 0,
+	.off_ana_lane = (const u16[]) { 0x100, 0x200, 0x300, 0x400 },
 	.off_dig_glb = 0x1000,
 	.off_dig_lane = (const u16[]) { 0x1100, 0x1200, 0x1300, 0x1400 },
 	.regs_ana_glb = mt8195_phy_ana_glb_regs,
+	.regs_ana_lane = mt8195_phy_ana_lane_regs,
 	.regs_dig_glb = mt8195_phy_dig_glb_regs,
 	.regs_dig_lane = mt8195_phy_dig_lane_regs,
+	.ana_bias_r = 15,
+	.ana_cktx_imp = 8,
+	.ana_lanes_imp = {
+		.pmos = 8,
+		.nmos = 8,
+	},
+	.driving_params = (const u32[]) {
+		[0] = 0,
+		[1] = 0,
+		[2] = 0,
+		[3] = MT8195_DRIVING_PARAM_3_DEFAULT,
+		[4] = MT8195_DRIVING_PARAM_4_DEFAULT,
+		[5] = MT8195_DRIVING_PARAM_5_DEFAULT,
+		[6] = MT8195_DRIVING_PARAM_6_DEFAULT,
+		[7] = MT8195_DRIVING_PARAM_7_DEFAULT,
+		[8] = MT8195_DRIVING_PARAM_8_DEFAULT
+	},
 };
 
 static const struct of_device_id mtk_dp_phy_of_match[] = {
-- 
2.54.0

Re: [PATCH 08/12] phy: phy-mtk-dp: Add support for digital and analog calibration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Pointer casting a 16-bit variable to a `u8 *` in `nvmem_cell_read_u8()` causes incorrect value retrieval on big-endian architectures.
- [High] `mtk_dp_phy_set_analog_calibration_params()` clears hardware registers due to missing bitwise shifts and ignores eFuse calibration.
- [High] Legacy probe path fails to initialize analog calibration parameters, breaking PHY transmission.
- [High] Driver probe fails unconditionally on platforms without NVMEM support due to unhandled `-EOPNOTSUPP`.
- [High] Missing `pm_runtime_put_sync()`, `pm_runtime_disable()`, and `phy_remove_lookup()` in driver removal, with unchecked `pm_runtime_get_sync()`.
- [High] Incorrect indexing of digital register array using an analog enum causes writes to the wrong offsets during power off.
- [Medium] Spams `dev_err` on boot when expected eFuse calibration data is not present.
- [Medium] Incorrect field mask used when validating N-MOSFET impedance values.
- [Low] The field `efuse_cal_present` is set but never read.
--

commit d7c70553eca35e13b77a951e8cbb9167598a5701
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Add support for digital and analog calibration

This commit adds support for reading digital and analog calibration
values from eFuse for the MediaTek DP PHY. If calibration values are
present, they are used; otherwise, the driver falls back to SoC-specific
defaults. It also implements hardware writes for the analog calibration.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 17d871530ccaf..cdcae861012c7 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> @@ -257,21 +257,21 @@
> +static int mtk_dp_phy_set_analog_calibration_params(struct mtk_dp_phy *dp_phy)
> +{
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs_ana_glb = pdata->regs_ana_glb;
> +	const u8 *regs_ana_lane = pdata->regs_ana_lane;
> +	int i, ret;
> +
> +	ret = regmap_update_bits(dp_phy->regmap,
> +				 pdata->off_ana_glb + regs_ana_glb[DP_PHYA_GLB_BIAS_GEN_0],
> +				 XTP_GLB_BIAS_INT_R_CTRL, pdata->ana_bias_r);

[Severity: High]
Does this clear the hardware registers instead of applying the calibrated
values?

The regmap_update_bits() call uses the static pdata->ana_bias_r defaults
instead of the calibrated dp_phy->ana_bias_r. Additionally, passing unshifted
values with the high-mask field XTP_GLB_BIAS_INT_R_CTRL results in a bitwise
AND that yields zero.

[ ... ]
> @@ -517,109 +517,109 @@
> +static int mtk_dp_phy_get_one_cal_para(struct device *dev, const char *name, u8 max_val)
> +{
> +	u16 buf = 0;
> +	int ret;
> +
> +	/*
> +	 * All of the calibrations are always max 8 bits long, but some may
> +	 * be split between two different 8-bits cells: handle this corner
> +	 * case by retrying reading as u16.
> +	 */
> +	ret = nvmem_cell_read_u8(dev, name, (u8 *)&buf);
> +	if (ret)
> +		ret = nvmem_cell_read_u16(dev, name, &buf);

[Severity: High]
Can this pointer cast corrupt the read value on big-endian architectures?

Casting the 16-bit buf variable to (u8 *) will write to the most significant
byte on big-endian systems. This shifts the 8-bit value left by 8 bits, which
will exceed max_val and cause an -ERANGE error.

> +
> +	if (ret) {
> +		dev_err(dev, "Cannot get calibration data for %s: %d\n", name, ret);
> +		return ret;
> +	};

[Severity: Medium]
Will this spam dev_err on boot when the eFuse calibration data is missing?

If the NVMEM cells are absent, this unconditionally prints an error before the
fallback logic can handle the -ENOENT return code.

[ ... ]
> +static int mtk_dp_phy_get_calibration_data(struct mtk_dp_phy *dp_phy)
> +{
> +	char mtk_dp_cal_lane_imp_name[] = "impedance-laneXM";
> +	struct device *dev = dp_phy->dev;
> +	int i, ret;
[ ... ]
> +	/* Get impedance params for each lane */
> +	for (i = 0; i < MTK_DP_PHY_MAX_LANES; i++) {
> +		/* P-MOSFET first */
[ ... ]
> +		/* ...and then N-MOSFET too */
> +		snprintf(mtk_dp_cal_lane_imp_name, ARRAY_SIZE(mtk_dp_cal_lane_imp_name),
> +			 "impedance-lane%dn", i);
> +		ret = mtk_dp_phy_get_one_cal_para(dev, mtk_dp_cal_lane_imp_name,
> +						  FIELD_MAX(XTP_LN_TX_IMPSEL_PMOS));

[Severity: Medium]
Is this using the wrong mask for the N-MOSFET impedance values?

It passes FIELD_MAX(XTP_LN_TX_IMPSEL_PMOS) instead of NMOS. While they both
share a 4-bit width currently, this might cause semantic errors if the bit
widths change.

> +		if (ret < 0)
> +			goto end;
> +		dp_phy->ana_impsel[i].nmos = ret;
> +	}
> +end:
> +	if (ret < 0) {
> +		/*
> +		 * If any of the calibration values is missing, or if there
> +		 * is no calibration at all in the eFuses, copy the default
> +		 * one entirely (as partial values shall not be mixed!)
> +		 */
> +		if (ret == -ENOENT) {
> +			dev_info(dev, "Using calibration default values\n");
> +			mtk_dp_phy_get_default_cal_data(dp_phy);
> +			return 0;
> +		}
> +		return ret;
> +	};

[Severity: High]
Will the driver probe fail unconditionally on platforms without NVMEM support?

If NVMEM is disabled, nvmem_cell_read_u8() returns -EOPNOTSUPP. The strict
check for -ENOENT means the default fallback is skipped and the driver probe
aborts.

> +	dp_phy->efuse_cal_present = true;

[Severity: Low]
Is this flag actually used anywhere?

The efuse_cal_present field is populated here but does not seem to be
evaluated anywhere else in the code.

> +
> +	return 0;
> +}
[ ... ]
> @@ -437,6 +661,7 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  	struct mtk_dp_phy *dp_phy;
>  	void __iomem *base;
>  	struct phy *phy;
> +	int ret;
>  
>  	dp_phy = devm_kzalloc(dev, sizeof(*dp_phy), GFP_KERNEL);
>  	if (!dp_phy)
> @@ -458,6 +683,10 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  
>  	dp_phy->pdata = device_get_match_data(dev);
>  
> +	ret = mtk_dp_phy_get_calibration_data(dp_phy);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
Does the legacy probe path also need to initialize the analog calibration
parameters?

If mtk_dp_phy_legacy_probe() bypasses calibration data loading,
dp_phy->ana_impsel is left entirely zeroed. These uninitialized zeroes will
then be written directly to the PHY registers, which breaks transmission on
legacy platforms.

[Severity: High]
Does the driver properly clean up runtime PM resources during removal?

The probe function acquires PM resources without verifying the result:

pm_runtime_enable(dev);
pm_runtime_get_sync(dev);

But there appears to be no corresponding .remove callback providing
pm_runtime_put_sync(), pm_runtime_disable(), or phy_remove_lookup().

[Severity: High]
Does mtk_dp_phy_power_off() access the wrong offset by indexing a digital
register array with an analog enum?

Looking at mtk_dp_phy_power_off() in phy-mtk-dp.c:

static int mtk_dp_phy_power_off(struct phy *phy)
{
        ...
        const u8 *regs = pdata->regs_dig_glb;
        ...
        ret = regmap_set_bits(dp_phy->regmap,
                        pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],

Using DP_PHYA_GLB_FORCE_CTRL_1 (which is an analog enum) to index regs
(which points to regs_dig_glb) writes to the wrong offsets during power off.

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

[PATCH 09/12] phy: phy-mtk-dp: Rewrite and document default driving param macros

[AngeloGioacchino Del Regno]

Use FIELD_PREP_CONST and add nicer definitions/macros to build the
default driving parameters for the PHY and, while at it, also add
comments explaining what they are supposed to set in the PHY.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 86 +++++++++++++++----------------
 1 file changed, 41 insertions(+), 45 deletions(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index b1b526ee44eb..bda262d437ed 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -85,51 +85,47 @@
  * [2,5,8]: Swing 2 Pre1 and Swing 3 Pre0
  */
 #define PHYD_DIG_NUM_DRV_PARA_REGS	9
-#define XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT	BIT(4)
-#define XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT	(BIT(10) | BIT(12))
-#define XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT	GENMASK(20, 19)
-#define XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT	GENMASK(29, 29)
-#define MT8195_DRIVING_PARAM_3_DEFAULT	(XTP_LN_TX_LCTXC0_SW0_PRE0_DEFAULT | \
-				 XTP_LN_TX_LCTXC0_SW0_PRE1_DEFAULT | \
-				 XTP_LN_TX_LCTXC0_SW0_PRE2_DEFAULT | \
-				 XTP_LN_TX_LCTXC0_SW0_PRE3_DEFAULT)
-
-#define XTP_LN_TX_LCTXC0_SW1_PRE0_DEFAULT	GENMASK(4, 3)
-#define XTP_LN_TX_LCTXC0_SW1_PRE1_DEFAULT	GENMASK(12, 9)
-#define XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT	(BIT(18) | BIT(21))
-#define XTP_LN_TX_LCTXC0_SW2_PRE0_DEFAULT	GENMASK(29, 29)
-#define MT8195_DRIVING_PARAM_4_DEFAULT	(XTP_LN_TX_LCTXC0_SW1_PRE0_DEFAULT | \
-				 XTP_LN_TX_LCTXC0_SW1_PRE1_DEFAULT | \
-				 XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT | \
-				 XTP_LN_TX_LCTXC0_SW2_PRE0_DEFAULT)
-
-#define XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT	(BIT(3) | BIT(5))
-#define XTP_LN_TX_LCTXC0_SW3_PRE0_DEFAULT	GENMASK(13, 12)
-#define MT8195_DRIVING_PARAM_5_DEFAULT	(XTP_LN_TX_LCTXC0_SW2_PRE1_DEFAULT | \
-				 XTP_LN_TX_LCTXC0_SW3_PRE0_DEFAULT)
-
-#define XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT	0
-#define XTP_LN_TX_LCTXCP1_SW0_PRE1_DEFAULT	GENMASK(10, 10)
-#define XTP_LN_TX_LCTXCP1_SW0_PRE2_DEFAULT	GENMASK(19, 19)
-#define XTP_LN_TX_LCTXCP1_SW0_PRE3_DEFAULT	GENMASK(28, 28)
-#define MT8195_DRIVING_PARAM_6_DEFAULT	(XTP_LN_TX_LCTXCP1_SW0_PRE0_DEFAULT | \
-				 XTP_LN_TX_LCTXCP1_SW0_PRE1_DEFAULT | \
-				 XTP_LN_TX_LCTXCP1_SW0_PRE2_DEFAULT | \
-				 XTP_LN_TX_LCTXCP1_SW0_PRE3_DEFAULT)
-
-#define XTP_LN_TX_LCTXCP1_SW1_PRE0_DEFAULT	0
-#define XTP_LN_TX_LCTXCP1_SW1_PRE1_DEFAULT	GENMASK(10, 9)
-#define XTP_LN_TX_LCTXCP1_SW1_PRE2_DEFAULT	GENMASK(19, 18)
-#define XTP_LN_TX_LCTXCP1_SW2_PRE0_DEFAULT	0
-#define MT8195_DRIVING_PARAM_7_DEFAULT	(XTP_LN_TX_LCTXCP1_SW1_PRE0_DEFAULT | \
-				 XTP_LN_TX_LCTXCP1_SW1_PRE1_DEFAULT | \
-				 XTP_LN_TX_LCTXCP1_SW1_PRE2_DEFAULT | \
-				 XTP_LN_TX_LCTXCP1_SW2_PRE0_DEFAULT)
-
-#define XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT	GENMASK(3, 3)
-#define XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT	0
-#define MT8195_DRIVING_PARAM_8_DEFAULT	(XTP_LN_TX_LCTXCP1_SW2_PRE1_DEFAULT | \
-				 XTP_LN_TX_LCTXCP1_SW3_PRE0_DEFAULT)
+#define XTP_LN_TX_LCTXC_SW0_PRE0	GENMASK(5, 0)
+#define XTP_LN_TX_LCTXC_SW0_PRE1	GENMASK(13, 8)
+#define XTP_LN_TX_LCTXC_SW0_PRE2	GENMASK(21, 16)
+#define XTP_LN_TX_LCTXC_SW0_PRE3	GENMASK(29, 24)
+
+#define XTP_LN_TX_LCTXC_SW1_PRE0	GENMASK(5, 0)
+#define XTP_LN_TX_LCTXC_SW1_PRE1	GENMASK(13, 8)
+#define XTP_LN_TX_LCTXC_SW1_PRE2	GENMASK(21, 16)
+#define XTP_LN_TX_LCTXC_SW2_PRE0	GENMASK(29, 24)
+
+#define XTP_LN_TX_LCTXC_SW2_PRE1	GENMASK(5, 0)
+#define XTP_LN_TX_LCTXC_SW3_PRE0	GENMASK(13, 8)
+
+#define BUILD_DRIVING_PARAM_0(sw0_pre0, sw0_pre1, sw0_pre2, sw0_pre3) (	\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW0_PRE0, sw0_pre0) |		\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW0_PRE1, sw0_pre1) |		\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW0_PRE2, sw0_pre2) |		\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW0_PRE3, sw0_pre3)		\
+)
+
+#define BUILD_DRIVING_PARAM_12(sw1_pre0, sw1_pre1, sw1_pre2, sw2_pre0) (\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW1_PRE0, sw1_pre0) |		\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW1_PRE1, sw1_pre1) |		\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW1_PRE2, sw1_pre2) |		\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW2_PRE0, sw2_pre0)		\
+)
+
+#define BUILD_DRIVING_PARAM_23(sw2_pre1, sw3_pre0) (			\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW2_PRE1, sw2_pre1) |		\
+	FIELD_PREP_CONST(XTP_LN_TX_LCTXC_SW3_PRE0, sw3_pre0)		\
+)
+
+/* MT8195: Logic State Change Point (LC TX C) */
+#define MT8195_DRIVING_PARAM_3_DEFAULT	BUILD_DRIVING_PARAM_0( 16, 20, 24, 32)
+#define MT8195_DRIVING_PARAM_4_DEFAULT	BUILD_DRIVING_PARAM_12(24, 30, 60, 32)
+#define MT8195_DRIVING_PARAM_5_DEFAULT	BUILD_DRIVING_PARAM_23(8, 48)
+
+/* MT8195: Positive Edge (LC TX CP) */
+#define MT8195_DRIVING_PARAM_6_DEFAULT	BUILD_DRIVING_PARAM_0( 0, 4, 8, 16)
+#define MT8195_DRIVING_PARAM_7_DEFAULT	BUILD_DRIVING_PARAM_12(0, 6, 12, 0)
+#define MT8195_DRIVING_PARAM_8_DEFAULT	BUILD_DRIVING_PARAM_23(8, 0)
 
 enum mtk_dp_phya_ana_glb_regidx {
 	DP_PHYA_GLB_BIAS_GEN_0,
-- 
2.54.0

Re: [PATCH 09/12] phy: phy-mtk-dp: Rewrite and document default driving param macros

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The refactoring introduces incorrect integer values for two DisplayPort PHY driving parameters, silently altering the underlying hardware configuration.
- [High] Integer underflow and undefined behavior in mtk_dp_phy_disable_all_lanes when no lanes are enabled.
- [High] Flawed PM runtime initialization, race condition, and unhandled errors in probe.
- [Medium] Incorrect endianness conversion by pointer aliasing a u16 as a u8*.
--

commit 3ae397642076d659c70475bdbefbc3286e0e7c1f
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: phy: phy-mtk-dp: Rewrite and document default driving param macros

This patch uses FIELD_PREP_CONST and adds definitions and macros to build
the default driving parameters for the PHY. It also adds comments explaining
what they are supposed to set in the PHY.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index d7024a1443358..1d73a622ef322 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> +/* MT8195: Logic State Change Point (LC TX C) */
> +#define MT8195_DRIVING_PARAM_3_DEFAULT	BUILD_DRIVING_PARAM_0( 16, 20, 24, 32)
> +#define MT8195_DRIVING_PARAM_4_DEFAULT	BUILD_DRIVING_PARAM_12(24, 30, 60, 32)

[Severity: High]
Does this unintentionally change the default for SW1_PRE2?

The conversion to macros incorrectly alters the sw1_pre2 value. The
original macro XTP_LN_TX_LCTXC0_SW1_PRE2_DEFAULT was (BIT(18) | BIT(21)),
which evaluates to 36 (0x24) when shifted. This macro now sets it to
60 (0x3c).

> +#define MT8195_DRIVING_PARAM_5_DEFAULT	BUILD_DRIVING_PARAM_23(8, 48)

[Severity: High]
Is there a similar issue with the default for SW2_PRE1?

The original value was 40 (0x28), but this is now passing 8 (0x08), which
drops BIT(5). These altered driving parameters can degrade signal integrity
and cause connection instability.

[ ... ]
> +static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
> +{
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs = pdata->regs_dig_glb;
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(dp_phy->regmap, pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0], &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Get mask of currently enabled lane */
> +	val = FIELD_GET(PHYD_TX_LN_EN, val);
> +
> +	/* Disable all lanes (needs to be done one by one, from last to first) */
> +	do {
> +		u32 lane_num = fls(val) - 1;
> +		val &= ~BIT(lane_num);
> +
> +		ret = regmap_clear_bits(dp_phy->regmap,
> +					pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
> +					FIELD_PREP(PHYD_TX_LN_EN, lane_num));
> +		if (ret)
> +			return ret;
> +	} while (val);

[Severity: High]
Can this do-while loop execute with val initially at 0?

If the PHY is already fully disabled when this is called, val will be 0.
The loop blindly executes once, fls(0) returns 0, and lane_num underflows
to 0xffffffff.

Using ~BIT(0xffffffff) invokes undefined behavior, and passing 0xffffffff
to FIELD_PREP() extracts unpredictable bits, causing a spurious out-of-bounds
register write when clearing the PHY control registers.

> +
> +	return 0;
> +}
[ ... ]
> +static int mtk_dp_phy_get_one_cal_para(struct device *dev, const char *name, u8 max_val)
> +{
> +	u16 buf = 0;
> +	int ret;
> +
> +	/*
> +	 * All of the calibrations are always max 8 bits long, but some may
> +	 * be split between two different 8-bits cells: handle this corner
> +	 * case by retrying reading as u16.
> +	 */
> +	ret = nvmem_cell_read_u8(dev, name, (u8 *)&buf);

[Severity: Medium]
Will this lead to an endianness violation on Big Endian platforms?

Casting a u16 buffer pointer to (u8 *) will cause the single byte to be
written into the most significant byte of buf instead of the least
significant byte on Big Endian architectures.

For instance, reading 0x0F would result in buf holding 0x0F00 (3840), which
would falsely trigger the subsequent if (buf > max_val) validation check.

> +	if (ret)
> +		ret = nvmem_cell_read_u16(dev, name, &buf);
[ ... ]
> +static int mtk_dp_phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	phy_set_drvdata(phy, dp_phy);
> +
> +	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(provider))
> +		return PTR_ERR(provider);

[Severity: High]
Could registering the provider before enabling PM runtime cause a race condition?

Exposing the PHY to consumers here means a consumer could claim the PHY and
attempt to power it on before pm_runtime_enable() is called below. This
would return -EACCES and break the consumer initialization.

> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

[Severity: High]
Should the return value of pm_runtime_get_sync(dev) be checked?

Ignoring the return value means the probe silently succeeds even if the
hardware cannot be powered.

Additionally, this driver lacks a .remove callback. Since
pm_runtime_get_sync() increases the usage counter without it ever being
released, and pm_runtime_disable() is never called, unloading the module
permanently leaks the PM reference and breaks PM runtime for this device.

> +
> +	return 0;
> +}

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

[PATCH 10/12] phy: phy-mtk-dp: Add bitrate register val definitions to SoC data

[AngeloGioacchino Del Regno]

In preparation for adding support for the eDP PHY found in newer
SoCs, transfer the bitrate register value definitions to SoC
specific data.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 36 +++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index bda262d437ed..73fc724e0ecc 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -42,10 +42,6 @@
 
 /* DP_PHYD_BIT_RATE */
 #define PHYD_DIG_RG_BIT_RATE		GENMASK(1, 0)
-#  define BIT_RATE_RBR			0
-#  define BIT_RATE_HBR			1
-#  define BIT_RATE_HBR2			2
-#  define BIT_RATE_HBR3			3
 
 /* DP_PHYD_SW_RST */
 #define PHYD_DIG_GLB_SW_RST_B		GENMASK(7, 0)
@@ -157,6 +153,14 @@ enum mtk_dp_phyd_dig_glb_regidx {
 	DP_PHYD_GLOBAL_MAX
 };
 
+enum mtk_dp_phyd_bit_rate_regval {
+	DP_PHYD_BIT_RATE_RBR,
+	DP_PHYD_BIT_RATE_HBR,
+	DP_PHYD_BIT_RATE_HBR2,
+	DP_PHYD_BIT_RATE_HBR3,
+	DP_PHYD_BIT_RATE_MAX,
+};
+
 static const u8 mt8195_phy_ana_glb_regs[DP_PHYA_GLOBAL_MAX] = {
 	[DP_PHYA_GLB_BIAS_GEN_0] = 0x0,
 	[DP_PHYA_GLB_BIAS_GEN_1] = 0x4,
@@ -183,6 +187,13 @@ static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {
 	[DP_PHYD_TX_CTL_0] = 0x44,
 };
 
+static const u8 mt8195_phy_dig_bitrate_val[DP_PHYD_BIT_RATE_MAX] = {
+	[DP_PHYD_BIT_RATE_RBR] = 0,
+	[DP_PHYD_BIT_RATE_HBR] = 1,
+	[DP_PHYD_BIT_RATE_HBR2] = 2,
+	[DP_PHYD_BIT_RATE_HBR3] = 3
+};
+
 /**
  * struct mtk_dp_phya_imp_sel - Per-Lane Impedance Selection
  * @pmos: Impedance selection for P-Channel MOSFET
@@ -203,6 +214,7 @@ struct mtk_dp_phya_imp_sel {
  * @regs_ana_lane:  Register (layout) offsets for ana_lan
  * @regs_dig_glb:   Register (layout) offsets for dig_glb
  * @regs_dig_lane:  Register (layout) offsets for dig_lan
+ * @val_dig_bitrate:IP Version specific register values for Bit Rate setting
  * @ana_bias_r:     Internal resistance "R" Selection Settings (global)
  * @ana_cktx_imp:   TX Clock Impedance Selection Settings (global)
  * @ana_lanes_imp:  TX Impedance Selection Settings (for all lanes)
@@ -221,6 +233,9 @@ struct mtk_dp_phy_pdata {
 	const u8 *regs_dig_glb;
 	const u8 *regs_dig_lane;
 
+	/* IP-Version specific register value arrays */
+	const u8 *val_dig_bitrate;
+
 	/* Calibration defaults */
 	u8 ana_bias_r;
 	u8 ana_cktx_imp;
@@ -325,6 +340,7 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 
 	if (opts->dp.set_rate) {
 		const u32 reg_bit_rate = pdata->regs_dig_glb[DP_PHYD_BIT_RATE];
+		enum mtk_dp_phyd_bit_rate_regval regval_idx;
 
 		switch (opts->dp.link_rate) {
 		default:
@@ -333,19 +349,20 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 				opts->dp.link_rate);
 			return -EINVAL;
 		case 1620:
-			val = BIT_RATE_RBR;
+			regval_idx = DP_PHYD_BIT_RATE_RBR;
 			break;
 		case 2700:
-			val = BIT_RATE_HBR;
+			regval_idx = DP_PHYD_BIT_RATE_HBR;
 			break;
 		case 5400:
-			val = BIT_RATE_HBR2;
+			regval_idx = DP_PHYD_BIT_RATE_HBR2;
 			break;
 		case 8100:
-			val = BIT_RATE_HBR3;
+			regval_idx = DP_PHYD_BIT_RATE_HBR3;
 			break;
 		}
-		regmap_write(dp_phy->regmap, pdata->off_dig_glb + reg_bit_rate, val);
+		regmap_write(dp_phy->regmap, pdata->off_dig_glb + reg_bit_rate,
+			     pdata->val_dig_bitrate[regval_idx]);
 	}
 
 	if (opts->dp.set_lanes) {
@@ -709,6 +726,7 @@ static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
 	.regs_ana_lane = mt8195_phy_ana_lane_regs,
 	.regs_dig_glb = mt8195_phy_dig_glb_regs,
 	.regs_dig_lane = mt8195_phy_dig_lane_regs,
+	.val_dig_bitrate = mt8195_phy_dig_bitrate_val,
 	.ana_bias_r = 15,
 	.ana_cktx_imp = 8,
 	.ana_lanes_imp = {
-- 
2.54.0

Re: [PATCH 10/12] phy: phy-mtk-dp: Add bitrate register val definitions to SoC data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtk_dp_phy_disable_all_lanes loop executes undefined behavior when val is 0, and incorrectly clears lane bits using the lane index instead of a bitmask.
--

commit 25cdd51bd1ac4d69a49e77d93a6b1374ba46dd3c
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Support set_lanes in configure and properly cleanup

Add support for enabling a specifically requested number of lanes in the
configure callback, and disable all lanes in power off and reset
callbacks for proper hardware cleanup.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 586e727956332..a2cd22b9da066 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> @@ -261,6 +267,35 @@ static int mtk_dp_phy_power_on(struct phy *phy)
>  	return 0;
>  }
>  
> +static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
> +{
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs = pdata->regs_dig_glb;
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(dp_phy->regmap, pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0], &val);
> +	if (ret)
> +		return ret;
> +
> +	/* Get mask of currently enabled lane */
> +	val = FIELD_GET(PHYD_TX_LN_EN, val);
> +
> +	/* Disable all lanes (needs to be done one by one, from last to first) */
> +	do {
> +		u32 lane_num = fls(val) - 1;

[Severity: High]
If no lanes are currently enabled, val will be 0. Does this cause fls(0) - 1
to underflow, leading to undefined behavior when computing BIT(lane_num)?

> +		val &= ~BIT(lane_num);
> +
> +		ret = regmap_clear_bits(dp_phy->regmap,
> +					pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
> +					FIELD_PREP(PHYD_TX_LN_EN, lane_num));

[Severity: High]
Does this incorrectly clear bits using the lane index instead of a bitmask?

For example, if lane 3 is being disabled (lane_num == 3), this passes 3 to
FIELD_PREP instead of BIT(3). This would clear lanes 0 and 1 (3 << 4) while
leaving lane 3 enabled. Should this use BIT(lane_num) instead?

> +		if (ret)
> +			return ret;
> +	} while (val);
> +
> +	return 0;
> +}
[ ... ]

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

[PATCH 11/12] phy: phy-mtk-dp: Add PHYD Lane EN register mask to SoC data

[AngeloGioacchino Del Regno]

In preparation for adding support for the eDP PHY found in newer
SoCs, transfer the register mask for PHYD_TX_LN_EN to SoC specific
data.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index 73fc724e0ecc..2c402b416683 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -214,6 +214,7 @@ struct mtk_dp_phya_imp_sel {
  * @regs_ana_lane:  Register (layout) offsets for ana_lan
  * @regs_dig_glb:   Register (layout) offsets for dig_glb
  * @regs_dig_lane:  Register (layout) offsets for dig_lan
+ * @mask_dig_tx_ln: Register mask for PHYD_TX_LN_EN field
  * @val_dig_bitrate:IP Version specific register values for Bit Rate setting
  * @ana_bias_r:     Internal resistance "R" Selection Settings (global)
  * @ana_cktx_imp:   TX Clock Impedance Selection Settings (global)
@@ -233,6 +234,9 @@ struct mtk_dp_phy_pdata {
 	const u8 *regs_dig_glb;
 	const u8 *regs_dig_lane;
 
+	/* Register masks */
+	u32 mask_dig_tx_ln;
+
 	/* IP-Version specific register value arrays */
 	const u8 *val_dig_bitrate;
 
@@ -370,10 +374,10 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
 
 		val = 0;
 		for (i = 0; i < opts->dp.lanes; i++)
-			val |= FIELD_PREP(PHYD_TX_LN_EN, BIT(i));
+			val |= field_prep(pdata->mask_dig_tx_ln, BIT(i));
 
 		regmap_update_bits(dp_phy->regmap, pdata->off_dig_glb + reg_dig_tx_ctl,
-				   PHYD_TX_LN_EN, val);
+				   pdata->mask_dig_tx_ln, val);
 	}
 
 	if (opts->dp.set_voltages) {
@@ -442,7 +446,7 @@ static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
 		return ret;
 
 	/* Get mask of currently enabled lane */
-	val = FIELD_GET(PHYD_TX_LN_EN, val);
+	val = field_get(pdata->mask_dig_tx_ln, val);
 
 	/* Disable all lanes (needs to be done one by one, from last to first) */
 	do {
@@ -451,7 +455,7 @@ static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
 
 		ret = regmap_clear_bits(dp_phy->regmap,
 					pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
-					FIELD_PREP(PHYD_TX_LN_EN, lane_num));
+					field_prep(pdata->mask_dig_tx_ln, lane_num));
 		if (ret)
 			return ret;
 	} while (val);
@@ -726,6 +730,7 @@ static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
 	.regs_ana_lane = mt8195_phy_ana_lane_regs,
 	.regs_dig_glb = mt8195_phy_dig_glb_regs,
 	.regs_dig_lane = mt8195_phy_dig_lane_regs,
+	.mask_dig_tx_ln = PHYD_TX_LN_EN,
 	.val_dig_bitrate = mt8195_phy_dig_bitrate_val,
 	.ana_bias_r = 15,
 	.ana_cktx_imp = 8,
-- 
2.54.0

Re: [PATCH 11/12] phy: phy-mtk-dp: Add PHYD Lane EN register mask to SoC data

[sashiko-bot]

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

Pre-existing issues:
- [High] mtk_dp_phy_disable_all_lanes() executes undefined behavior if initially zero, and incorrectly clears lanes by shifting the lane index instead of a bitmask.
--

commit d17ede4ab4b1189e133fc8f8e84c59aaf2d2590e
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Add PHYD Lane EN register mask to SoC data

This commit prepares for supporting newer SoCs by moving the PHYD_TX_LN_EN
register mask into the SoC-specific data structure.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 7c8c04787b78c..f067acef222f7 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -442,7 +446,7 @@ static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
>  		return ret;
>  
>  	/* Get mask of currently enabled lane */
> -	val = FIELD_GET(PHYD_TX_LN_EN, val);
> +	val = field_get(pdata->mask_dig_tx_ln, val);
>  
>  	/* Disable all lanes (needs to be done one by one, from last to first) */
>  	do {
> @@ -451,7 +455,7 @@ static int mtk_dp_phy_disable_all_lanes(struct mtk_dp_phy *dp_phy)
>  
>  		ret = regmap_clear_bits(dp_phy->regmap,
>  					pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
> -					FIELD_PREP(PHYD_TX_LN_EN, lane_num));
> +					field_prep(pdata->mask_dig_tx_ln, lane_num));

[Severity: High]
This is a pre-existing issue, but does this loop safely handle the case
where val is initially 0?

Since this is a do-while loop, if no lanes are currently enabled, fls(0)
will evaluate to 0, which leaves lane_num as -1. This would then result
in an out-of-bounds shift during the BIT(lane_num) calculation.

Additionally, should lane_num be passed into field_prep() as a bitmask
(e.g., BIT(lane_num)) rather than the raw integer index? 

Passing the integer index directly (like 2 or 3) instead of a bitmask
appears to generate an incorrect value, which could cause regmap_clear_bits()
to clear the wrong bits in the register and leave lanes incorrectly powered on.

>  		if (ret)
>  			return ret;
>  	} while (val);

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

[PATCH 12/12] phy: phy-mtk-dp: Add support for MT8196 eDP PHY

[AngeloGioacchino Del Regno]

The MT8196 SoC features an updated PHY IP compared to the older
ones, and there is one that is specific to Embedded DisplayPort.

Add support for the eDP PHY found in the MediaTek MT8196 SoC and
all of its variants.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/mediatek/phy-mtk-dp.c | 66 ++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
index 2c402b416683..c8abc4a2af0a 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
@@ -5,7 +5,7 @@
  * Copyright (c) 2022, BayLibre Inc.
  * Copyright (c) 2022, MediaTek Inc.
  *
- * Major refactoring
+ * Major refactoring and new SoCs support
  * Copyright (c) 2026, Collabora Ltd.
  *                     AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
  */
@@ -41,6 +41,7 @@
 #define TPLL_SSC_EN			BIT(3)
 
 /* DP_PHYD_BIT_RATE */
+#define PHYD_DIG_RG_BIT_RATE_V2		GENMASK(3, 0)
 #define PHYD_DIG_RG_BIT_RATE		GENMASK(1, 0)
 
 /* DP_PHYD_SW_RST */
@@ -57,6 +58,7 @@
 
 /* DP_PHYD_TX_CTL_0 */
 #define PHYD_TX_LN_EN			GENMASK(7, 4)
+#define PHYD_TX_LN_EN_V2		GENMASK(3, 0)
 
 /* DP_PHYD_DRIVING_FORCE */
 #define PHYD_DP_TX_FORCE_VOLT_SWING_EN	BIT(0)
@@ -123,6 +125,16 @@
 #define MT8195_DRIVING_PARAM_7_DEFAULT	BUILD_DRIVING_PARAM_12(0, 6, 12, 0)
 #define MT8195_DRIVING_PARAM_8_DEFAULT	BUILD_DRIVING_PARAM_23(8, 0)
 
+/* MT8196/MT6991: Logic State Change Point (LC TX C) */
+#define MT8196_DRIVING_PARAM_3_DEFAULT	BUILD_DRIVING_PARAM_0( 10, 12, 14, 17)
+#define MT8196_DRIVING_PARAM_4_DEFAULT	BUILD_DRIVING_PARAM_12(14, 17, 18, 18)
+#define MT8196_DRIVING_PARAM_5_DEFAULT	BUILD_DRIVING_PARAM_23(21, 24)
+
+/* MT8196/MT6991: Positive Edge (LC TX CP) */
+#define MT8196_DRIVING_PARAM_6_DEFAULT	BUILD_DRIVING_PARAM_0( 0, 2, 4, 7)
+#define MT8196_DRIVING_PARAM_7_DEFAULT	BUILD_DRIVING_PARAM_12(0, 3, 6, 0)
+#define MT8196_DRIVING_PARAM_8_DEFAULT	BUILD_DRIVING_PARAM_23(3, 0)
+
 enum mtk_dp_phya_ana_glb_regidx {
 	DP_PHYA_GLB_BIAS_GEN_0,
 	DP_PHYA_GLB_BIAS_GEN_1,
@@ -178,6 +190,11 @@ static const u8 mt8195_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
 	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
 };
 
+static const u8 mt8196_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
+	[DP_PHYD_LAN_DRIVING_FORCE] = 0x30,
+	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
+};
+
 static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {
 	[DP_PHYD_PLL_CTL_0] = 0x10,
 	[DP_PHYD_PLL_CTL_1] = 0x14,
@@ -187,6 +204,15 @@ static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {
 	[DP_PHYD_TX_CTL_0] = 0x44,
 };
 
+static const u8 mt8196_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {
+	[DP_PHYD_PLL_CTL_0] = 0x10,
+	[DP_PHYD_PLL_CTL_1] = 0x14,
+	[DP_PHYD_SW_RST] = 0x38,
+	[DP_PHYD_BIT_RATE] = 0x3c,
+	[DP_PHYD_AUX_RX_CTL] = 0x40,
+	[DP_PHYD_TX_CTL_0] = 0x74,
+};
+
 static const u8 mt8195_phy_dig_bitrate_val[DP_PHYD_BIT_RATE_MAX] = {
 	[DP_PHYD_BIT_RATE_RBR] = 0,
 	[DP_PHYD_BIT_RATE_HBR] = 1,
@@ -194,6 +220,13 @@ static const u8 mt8195_phy_dig_bitrate_val[DP_PHYD_BIT_RATE_MAX] = {
 	[DP_PHYD_BIT_RATE_HBR3] = 3
 };
 
+static const u8 mt8196_edp_phy_dig_bitrate_val[DP_PHYD_BIT_RATE_MAX] = {
+	[DP_PHYD_BIT_RATE_RBR] = 1,
+	[DP_PHYD_BIT_RATE_HBR] = 4,
+	[DP_PHYD_BIT_RATE_HBR2] = 7,
+	[DP_PHYD_BIT_RATE_HBR3] = 9
+};
+
 /**
  * struct mtk_dp_phya_imp_sel - Per-Lane Impedance Selection
  * @pmos: Impedance selection for P-Channel MOSFET
@@ -751,8 +784,39 @@ static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
 	},
 };
 
+static const struct mtk_dp_phy_pdata mt8196_edp_phy_data = {
+	.off_ana_glb = 0x400,
+	.off_ana_lane = (const u16[]) { 0x0, 0x100, 0x200, 0x300 },
+	.off_dig_glb = 0x1400,
+	.off_dig_lane = (const u16[]) { 0x1000, 0x1100, 0x1200, 0x1300 },
+	.regs_ana_glb = mt8195_phy_ana_glb_regs,
+	.regs_ana_lane = mt8195_phy_ana_lane_regs,
+	.regs_dig_glb = mt8196_phy_dig_glb_regs,
+	.regs_dig_lane = mt8196_phy_dig_lane_regs,
+	.mask_dig_tx_ln = PHYD_TX_LN_EN_V2,
+	.val_dig_bitrate = mt8196_edp_phy_dig_bitrate_val,
+	.ana_bias_r = 15,
+	.ana_cktx_imp = 8,
+	.ana_lanes_imp = {
+		.pmos = 8,
+		.nmos = 8,
+	},
+	.driving_params = (const u32[]) {
+		[0] = 0,
+		[1] = 0,
+		[2] = 0,
+		[3] = MT8196_DRIVING_PARAM_3_DEFAULT,
+		[4] = MT8196_DRIVING_PARAM_4_DEFAULT,
+		[5] = MT8196_DRIVING_PARAM_5_DEFAULT,
+		[6] = MT8196_DRIVING_PARAM_6_DEFAULT,
+		[7] = MT8196_DRIVING_PARAM_7_DEFAULT,
+		[8] = MT8196_DRIVING_PARAM_8_DEFAULT
+	},
+};
+
 static const struct of_device_id mtk_dp_phy_of_match[] = {
 	{ .compatible = "mediatek,mt8195-dp-phy", .data = &mt8195_dp_phy_data },
+	{ .compatible = "mediatek,mt8196-edp-phy", .data = &mt8196_edp_phy_data },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mtk_dp_phy_of_match);
-- 
2.54.0

Re: [PATCH 12/12] phy: phy-mtk-dp: Add support for MT8196 eDP PHY

[sashiko-bot]

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

New issues:
- [High] The MT8196 layout places DP_PHYD_LAN_DRIVING_FORCE at an offset that is overwritten by a contiguous bulk write to the driving parameters.

Pre-existing issues:
- [High] Lane disabling logic incorrectly clears the wrong register bits, leaving upper lanes powered on and corrupting adjacent lanes.
- [High] Analog register offsets are mistakenly fetched from the digital register array, causing writes to the wrong address.
- [High] Missing shift for analog calibration parameters passed to regmap_update_bits() zeroes out hardware fields.
--

commit 25701726cdcf8525c87349866f69aa298517187f
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: phy: phy-mtk-dp: Add support for MT8196 eDP PHY

The MT8196 SoC features an updated PHY IP compared to the older ones, and
there is one that is specific to Embedded DisplayPort. This patch adds
support for the eDP PHY found in the MediaTek MT8196 SoC and all of its
variants.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index f067acef222f7..998ac34466678 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -178,6 +190,11 @@ static const u8 mt8195_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
>  	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
>  };
>  
> +static const u8 mt8196_phy_dig_lane_regs[DP_PHYD_LAN_MAX] = {
> +	[DP_PHYD_LAN_DRIVING_FORCE] = 0x30,
> +	[DP_PHYD_LAN_DRIVING_PARAM_0] = 0x2c,
> +};

[Severity: High]
Could this regression corrupt the driving force configuration?

In mtk_dp_phy_set_digital_drv_params(), there is a bulk write of 9 contiguous
32-bit registers starting at DP_PHYD_LAN_DRIVING_PARAM_0:

    ret = regmap_bulk_write(dp_phy->regmap,
                            pdata->off_dig_lane[i] + reg,
                            pdata->driving_params,
                            ARRAY_SIZE(pdata->driving_params));

Since the bulk write starts at 0x2c, driving_params[1] will be written to 0x30,
which overlaps with DP_PHYD_LAN_DRIVING_FORCE.

> +
>  static const u8 mt8195_phy_dig_glb_regs[DP_PHYD_GLOBAL_MAX] = {

[ ... ]

> @@ -751,8 +784,39 @@ static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
>  	},
>  };
>  
> +static const struct mtk_dp_phy_pdata mt8196_edp_phy_data = {
> +	.off_ana_glb = 0x400,
> +	.off_ana_lane = (const u16[]) { 0x0, 0x100, 0x200, 0x300 },
> +	.off_dig_glb = 0x1400,
> +	.off_dig_lane = (const u16[]) { 0x1000, 0x1100, 0x1200, 0x1300 },
> +	.regs_ana_glb = mt8195_phy_ana_glb_regs,
> +	.regs_ana_lane = mt8195_phy_ana_lane_regs,
> +	.regs_dig_glb = mt8196_phy_dig_glb_regs,

[Severity: High]
This is a pre-existing issue, but do mtk_dp_phy_power_on() and
mtk_dp_phy_power_off() fetch analog register offsets using this digital register
array?

Both functions use the analog enum DP_PHYA_GLB_FORCE_CTRL_1 to access the
digital array:

    const u8 *regs = pdata->regs_dig_glb;
    ...
    ret = regmap_set_bits(dp_phy->regmap,
                          pdata->off_ana_glb + regs[DP_PHYA_GLB_FORCE_CTRL_1],
                          CKM_CKTX0_EN_FORCE_MODE);

Accessing index 4 (DP_PHYA_GLB_FORCE_CTRL_1) of the digital array returns
DP_PHYD_AUX_RX_CTL (0x40) instead of the intended analog offset (0x34).

Does this overwrite incorrect memory mapped register offsets?

> +	.regs_dig_lane = mt8196_phy_dig_lane_regs,
> +	.mask_dig_tx_ln = PHYD_TX_LN_EN_V2,

[Severity: High]
This isn't a bug introduced by this patch, but does the lane disabling logic in
mtk_dp_phy_disable_all_lanes() use this mask incorrectly?

It computes lane_num as a raw index and passes it to field_prep():

    ret = regmap_clear_bits(dp_phy->regmap,
                            pdata->off_dig_glb + regs[DP_PHYD_TX_CTL_0],
                            field_prep(pdata->mask_dig_tx_ln, lane_num));

Because field_prep() shifts the raw value, passing 3 instead of BIT(3) will
shift 0b11 into the mask, clearing bits 4 and 5 instead of bit 7 (or bits 0 and
1 instead of bit 3 for V2).

Should this be using BIT(lane_num) instead?

> +	.val_dig_bitrate = mt8196_edp_phy_dig_bitrate_val,
> +	.ana_bias_r = 15,
> +	.ana_cktx_imp = 8,

[Severity: High]
This isn't a bug introduced by this patch, but are these calibration parameters
unconditionally cleared to zero in the hardware?

In mtk_dp_phy_set_analog_calibration_params(), these values are passed
directly to regmap_update_bits():

    ret = regmap_update_bits(dp_phy->regmap,
                             pdata->off_ana_glb + regs_ana_glb[DP_PHYA_GLB_BIAS_GEN_0],
                             XTP_GLB_BIAS_INT_R_CTRL, pdata->ana_bias_r);

Because regmap_update_bits() expects a pre-shifted value and applies a mask
(e.g., GENMASK(20, 16)), an unshifted raw value like 15 masked against the upper
bits yields 0.

Should a macro like FIELD_PREP() be used to properly shift these values into
place?

> +	.ana_lanes_imp = {
> +		.pmos = 8,
> +		.nmos = 8,
> +	},

[ ... ]

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