Re: [PATCH v5 2/5] phy: qcom-qmp-pcie: support separate tables for EP mode

[Johan Hovold <johan@kernel.org>]

On Mon, Sep 26, 2022 at 08:34:32PM +0300, Dmitry Baryshkov wrote:
> The PCIe QMP PHY requires different programming sequences when being
> used for the RC (Root Complex) or for the EP (End Point) modes. Allow
> selecting the submode and thus selecting a set of PHY programming
> tables.
> 
> Since the RC and EP modes share common some common init sequence, the
> common sequence is kept in the main table and the sequence differences
> are pushed to the extra tables.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 47 +++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index dc8f0f236212..dd7911879b10 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -14,6 +14,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/of_address.h>
> +#include <linux/phy/pcie.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
> @@ -1320,10 +1321,14 @@ struct qmp_phy_cfg {
>  	/* Main init sequence for PHY blocks - serdes, tx, rx, pcs */
>  	const struct qmp_phy_cfg_tables tables;
>  	/*
> -	 * Additional init sequence for PHY blocks, providing additional
> -	 * register programming. Unless required it can be left omitted.
> +	 * Additional init sequences for PHY blocks, providing additional
> +	 * register programming. They are used for providing separate sequences
> +	 * for the Root Complex and for the End Point usecases.

"use cases", drop the second "for the".


> +	 *
> +	 * If EP mode is not supported, both tables can be left empty.

s/empty/unset/

>  	 */
>  	const struct qmp_phy_cfg_tables *tables_rc;
> +	const struct qmp_phy_cfg_tables *tables_ep;
>  
>  	/* clock ids to be requested */
>  	const char * const *clk_list;

> +static int qmp_pcie_set_mode(struct phy *phy,
> +				 enum phy_mode mode, int submode)

No need for line break.

> +{
> +	struct qmp_phy *qphy = phy_get_drvdata(phy);
> +
> +	switch (submode) {
> +	case PHY_MODE_PCIE_RC:
> +	case PHY_MODE_PCIE_EP:
> +		qphy->mode = submode;
> +		break;
> +	default:
> +		dev_err(&phy->dev, "Unuspported submode %d\n", submode);

You forgot to fix the "unsupported" typo.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

Looks good otherwise:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Johan