Re: [PATCH 18/22] phy: qcom-qmp-combo: merge driver data

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 11/11/2022 11:56, Johan Hovold wrote:
> The QMP combo driver manages a single PHY (even if it provides two
> interfaces for USB and DP, respectively) so merge the old qcom_qmp and
> qmp_phy structures and drop the PHY array.
>
> Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 690 ++++++++++------------
>   1 file changed, 313 insertions(+), 377 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 249912b75964..43b7ea5d2edc 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -796,7 +796,7 @@ static const u8 qmp_dp_v5_voltage_swing_hbr_rbr[4][4] = {
>   	{ 0x3f, 0xff, 0xff, 0xff }
>   };
>   
> -struct qmp_phy;
> +struct qmp_combo;
>   
>   struct qmp_phy_cfg {
>   	int lanes;
> @@ -835,10 +835,10 @@ struct qmp_phy_cfg {
>   	const u8 (*pre_emphasis_hbr3_hbr2)[4][4];
>   
>   	/* DP PHY callbacks */
> -	int (*configure_dp_phy)(struct qmp_phy *qphy);
> -	void (*configure_dp_tx)(struct qmp_phy *qphy);
> -	int (*calibrate_dp_phy)(struct qmp_phy *qphy);
> -	void (*dp_aux_init)(struct qmp_phy *qphy);
> +	int (*configure_dp_phy)(struct qmp_combo *qmp);
> +	void (*configure_dp_tx)(struct qmp_combo *qmp);
> +	int (*calibrate_dp_phy)(struct qmp_combo *qmp);
> +	void (*dp_aux_init)(struct qmp_combo *qmp);
>   
>   	/* clock ids to be requested */
>   	const char * const *clk_list;
> @@ -861,29 +861,19 @@ struct qmp_phy_cfg {
>   
>   };
>   
> -/**
> - * struct qmp_phy - per-lane phy descriptor
> - *
> - * @phy: generic phy
> - * @cfg: phy specific configuration
> - * @serdes: iomapped memory space for phy's serdes (i.e. PLL)
> - * @tx: iomapped memory space for lane's tx
> - * @rx: iomapped memory space for lane's rx
> - * @pcs: iomapped memory space for lane's pcs
> - * @tx2: iomapped memory space for second lane's tx (in dual lane PHYs)
> - * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs)
> - * @pcs_misc: iomapped memory space for lane's pcs_misc
> - * @pcs_usb: iomapped memory space for lane's pcs_usb
> - * @pipe_clk: pipe clock
> - * @qmp: QMP phy to which this lane belongs
> - * @mode: current PHY mode
> - * @dp_aux_cfg: Display port aux config
> - * @dp_opts: Display port optional config
> - * @dp_clks: Display port clocks
> - */
> -struct qmp_phy {
> -	struct phy *phy;
> +struct qmp_phy_dp_clks {
> +	struct qmp_combo *qmp;
> +	struct clk_hw dp_link_hw;
> +	struct clk_hw dp_pixel_hw;
> +};
> +

It would make sense to keep the kerneldoc here.

> +struct qmp_combo {
> +	struct device *dev;
> +
>   	const struct qmp_phy_cfg *cfg;
> +
> +	void __iomem *dp_com;
> +
>   	void __iomem *serdes;
>   	void __iomem *tx;
>   	void __iomem *rx;
> @@ -899,59 +889,33 @@ struct qmp_phy {
>   	void __iomem *dp_pcs;
>   
>   	struct clk *pipe_clk;
> -	struct qcom_qmp *qmp;
> -	enum phy_mode mode;
> -	unsigned int dp_aux_cfg;
> -	struct phy_configure_opts_dp dp_opts;
> -	struct qmp_phy_dp_clks *dp_clks;
> -};
> -
> -struct qmp_phy_dp_clks {
> -	struct qmp_phy *qphy;
> -	struct clk_hw dp_link_hw;
> -	struct clk_hw dp_pixel_hw;
> -};
> -
> -/**
> - * struct qcom_qmp - structure holding QMP phy block attributes
> - *
> - * @dev: device
> - * @dp_com: iomapped memory space for phy's dp_com control block
> - *
> - * @clks: array of clocks required by phy
> - * @resets: array of resets required by phy
> - * @vregs: regulator supplies bulk data
> - *
> - * @phys: array of per-lane phy descriptors
> - * @phy_mutex: mutex lock for PHY common block initialization
> - * @init_count: phy common block initialization count
> - */
> -struct qcom_qmp {
> -	struct device *dev;
> -	void __iomem *dp_com;
> -
>   	struct clk_bulk_data *clks;
>   	struct reset_control_bulk_data *resets;
>   	struct regulator_bulk_data *vregs;
>   
> -	struct qmp_phy **phys;
> -	struct qmp_phy *usb_phy;
> -
>   	struct mutex phy_mutex;
>   	int init_count;
> +
> +	struct phy *usb_phy;
> +	enum phy_mode mode;
> +
> +	struct phy *dp_phy;
> +	unsigned int dp_aux_cfg;
> +	struct phy_configure_opts_dp dp_opts;
> +	struct qmp_phy_dp_clks *dp_clks;
>   };
>   
> -static void qcom_qmp_v3_phy_dp_aux_init(struct qmp_phy *qphy);
> -static void qcom_qmp_v3_phy_configure_dp_tx(struct qmp_phy *qphy);
> -static int qcom_qmp_v3_phy_configure_dp_phy(struct qmp_phy *qphy);
> -static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy);
> +static void qcom_qmp_v3_phy_dp_aux_init(struct qmp_combo *qmp);
> +static void qcom_qmp_v3_phy_configure_dp_tx(struct qmp_combo *qmp);
> +static int qcom_qmp_v3_phy_configure_dp_phy(struct qmp_combo *qmp);
> +static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_combo *qmp);
>   
> -static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_phy *qphy);
> -static void qcom_qmp_v4_phy_configure_dp_tx(struct qmp_phy *qphy);
> -static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy);
> -static int qcom_qmp_v4_dp_phy_calibrate(struct qmp_phy *qphy);
> +static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_combo *qmp);
> +static void qcom_qmp_v4_phy_configure_dp_tx(struct qmp_combo *qmp);
> +static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_combo *qmp);
> +static int qcom_qmp_v4_dp_phy_calibrate(struct qmp_combo *qmp);
>   
> -static int qcom_qmp_v5_phy_configure_dp_phy(struct qmp_phy *qphy);
> +static int qcom_qmp_v5_phy_configure_dp_phy(struct qmp_combo *qmp);


As you are doing the cleanup anyway, would it make sense to move these 
functions up to be able to drop these prototypes?

>   
>   static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
>   {
> @@ -1265,11 +1229,11 @@ static void qmp_combo_configure(void __iomem *base,


The rest LGTM

--
With best wishes
Dmitry