Re: [PATCH 3/8] phy: qcom: qmp-usbc: Add DP phy mode support on QCS615

[Krzysztof Kozlowski <krzk@kernel.org>]

On 29/11/2024 08:57, Xiangxu Yin wrote:
> Extended DP support for QCS615 USB or DP phy. Differentiated between
> USBC and DP PHY using the match table’s type, dynamically generating
> different types of cfg and layout attributes during initialization based
> on this type. Static variables are stored in cfg, while parsed values
> are organized into the layout structure.
> 
> Signed-off-by: Xiangxu Yin <quic_xiangxuy@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-dp-phy.h |    1 +
>  drivers/phy/qualcomm/phy-qcom-qmp-usbc.c   | 1453 ++++++++++++++++++++++++----
>  2 files changed, 1254 insertions(+), 200 deletions(-)



...

> +	/* program default setting first */
> +	writel(0x2A, tx + QSERDES_V3_TX_TX_DRV_LVL);
> +	writel(0x20, tx + QSERDES_V3_TX_TX_EMP_POST1_LVL);
> +	writel(0x2A, tx2 + QSERDES_V3_TX_TX_DRV_LVL);
> +	writel(0x20, tx2 + QSERDES_V3_TX_TX_EMP_POST1_LVL);
> +
> +	writel(voltage_swing_cfg, tx + QSERDES_V3_TX_TX_DRV_LVL);
> +	writel(pre_emphasis_cfg, tx + QSERDES_V3_TX_TX_EMP_POST1_LVL);
> +	writel(voltage_swing_cfg, tx2 + QSERDES_V3_TX_TX_DRV_LVL);
> +	writel(pre_emphasis_cfg, tx2 + QSERDES_V3_TX_TX_EMP_POST1_LVL);
> +
> +	return 0;
> +}
> +
> +static int qcs615_qmp_configure_dp_phy(struct qmp_usbc *qmp)
> +{
> +	struct qmp_phy_dp_layout *layout = to_dp_layout(qmp);
> +	u32 status;
> +
> +	writel(0x01, layout->dp_phy + QSERDES_DP_PHY_CFG);
> +	writel(0x05, layout->dp_phy + QSERDES_DP_PHY_CFG);
> +	writel(0x01, layout->dp_phy + QSERDES_DP_PHY_CFG);
> +	writel(0x09, layout->dp_phy + QSERDES_DP_PHY_CFG);
> +
> +	writel(0x20, layout->dp_serdes + QSERDES_COM_RESETSM_CNTRL);
> +
> +	// C_READY

Use Linux coding style.

Anyway, drop all useless comments. Say something useful or don't say
anything.

> +	if (readl_poll_timeout(layout->dp_serdes + QSERDES_COM_C_READY_STATUS,
> +			status,
> +			((status & BIT(0)) > 0),
> +			500,
> +			10000)) {
> +		dev_err(qmp->dev, "C_READY not ready\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	// FREQ_DONE
> +	if (readl_poll_timeout(layout->dp_serdes + QSERDES_COM_CMN_STATUS,
> +			status,
> +			((status & BIT(0)) > 0),
> +			500,
> +			10000)){
> +		dev_err(qmp->dev, "FREQ_DONE not ready\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	// PLL_LOCKED
> +	if (readl_poll_timeout(layout->dp_serdes + QSERDES_COM_CMN_STATUS,
> +			status,
> +			((status & BIT(1)) > 0),
> +			500,
> +			10000)){
> +		dev_err(qmp->dev, "PLL_LOCKED not ready\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	writel(0x19, layout->dp_phy + QSERDES_DP_PHY_CFG);
> +	udelay(10);
> +
> +	// TSYNC_DONE
> +	if (readl_poll_timeout(layout->dp_phy + QSERDES_V3_DP_PHY_STATUS,
> +			status,
> +			((status & BIT(0)) > 0),
> +			500,
> +			10000)){
> +		dev_err(qmp->dev, "TSYNC_DONE not ready\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	// PHY_READY
> +	if (readl_poll_timeout(layout->dp_phy + QSERDES_V3_DP_PHY_STATUS,
> +			status,
> +			((status & BIT(1)) > 0),
> +			500,
> +			10000)){
> +		dev_err(qmp->dev, "PHY_READY not ready\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	writel(0x3f, layout->dp_tx + QSERDES_V3_TX_TRANSCEIVER_BIAS_EN);
> +	writel(0x10, layout->dp_tx + QSERDES_V3_TX_HIGHZ_DRVR_EN);
> +	writel(0x0a, layout->dp_tx + QSERDES_V3_TX_TX_POL_INV);
> +	writel(0x3f, layout->dp_tx2 + QSERDES_V3_TX_TRANSCEIVER_BIAS_EN);
> +	writel(0x10, layout->dp_tx2 + QSERDES_V3_TX_HIGHZ_DRVR_EN);
> +	writel(0x0a, layout->dp_tx2 + QSERDES_V3_TX_TX_POL_INV);
> +
> +	writel(0x18, layout->dp_phy + QSERDES_DP_PHY_CFG);
> +	writel(0x19, layout->dp_phy + QSERDES_DP_PHY_CFG);
> +
> +	if (readl_poll_timeout(layout->dp_phy + QSERDES_V3_DP_PHY_STATUS,
> +			status,
> +			((status & BIT(1)) > 0),
> +			500,
> +			10000)){
> +		dev_err(qmp->dev, "PHY_READY not ready\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcs615_qmp_calibrate_dp_phy(struct qmp_usbc *qmp)
> +{
> +	static const u8 cfg1_settings[] = {0x13, 0x23, 0x1d};
> +	struct qmp_phy_dp_layout *layout = to_dp_layout(qmp);
> +	u8 val;
> +
> +	layout->dp_aux_cfg++;
> +	layout->dp_aux_cfg %= ARRAY_SIZE(cfg1_settings);
> +	val = cfg1_settings[layout->dp_aux_cfg];
> +
> +	writel(val, layout->dp_phy + QSERDES_DP_PHY_AUX_CFG1);
> +
> +	qmp_usbc_check_dp_phy(qmp, "pos_calibrate");
> +
> +	return 0;
> +}
> +
> +static int qmp_usbc_com_init(struct phy *phy)
>  {
>  	struct qmp_usbc *qmp = phy_get_drvdata(phy);
> -	const struct qmp_phy_cfg *cfg = qmp->cfg;
> -	void __iomem *pcs = qmp->pcs;
> +	int num_vregs;
>  	u32 val = 0;
>  	int ret;
> +	unsigned int reg_pwr_dn;
>  
> -	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> +	if (qmp->type == QMP_PHY_USBC_USB) {


Sorry, all this code is unreviewable. Organize your changes in logical,
reviewable chunks.

> +		struct qmp_phy_usb_cfg *cfg = to_usb_cfg(qmp);
> +
> +		num_vregs = cfg->num_vregs;
> +		reg_pwr_dn = cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL];
> +	} else {

...

> +		.compatible = "qcom,qcs615-qmp-dp-phy",
> +		.data =  &(struct dev_cfg) {
> +			.type = QMP_PHY_USBC_DP,
> +			.cfg = &qcs615_dpphy_cfg,
> +		},
>  	}, {
>  		.compatible = "qcom,sdm660-qmp-usb3-phy",
> -		.data = &sdm660_usb3phy_cfg,
> +		.data =  &(struct dev_cfg) {
> +			.type = QMP_PHY_USBC_USB,
> +			.cfg = &sdm660_usb3phy_cfg,
> +		},
>  	}, {
>  		.compatible = "qcom,sm6115-qmp-usb3-phy",
> -		.data = &qcm2290_usb3phy_cfg,
> +		.data =  &(struct dev_cfg) {
> +			.type = QMP_PHY_USBC_USB,
> +			.cfg = &qcm2290_usb3phy_cfg,
> +		},
>  	},
>  	{ },
>  };
> +


You make some random changes all over this file. No, clean it up.

>  MODULE_DEVICE_TABLE(of, qmp_usbc_of_match_table);
>  
>  static struct platform_driver qmp_usbc_driver = {
> 


Best regards,
Krzysztof

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