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 <[email protected]> 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/[email protected]?part=8
