Message-ID: <Y3ID+jTI4Bf6wvh2@hovoldconsulting.com>
Date:   Mon, 14 Nov 2022 10:01:46 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Johan Hovold <johan+linaro@...nel.org>,
        Vinod Koul <vkoul@...nel.org>, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/22] phy: qcom-qmp-combo: merge driver data

On Sat, Nov 12, 2022 at 10:46:53AM +0300, Dmitry Baryshkov wrote:
> 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@...nel.org>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 690 ++++++++++------------
> >   1 file changed, 313 insertions(+), 377 deletions(-)
> > 

> > -/**
> > - * 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.

I disagree. The above kernel doc is at best pointless and mostly just
restates what can be understood from the field names.

Note how it also incorrect by referring to "memory space for *lane's*
...".

> > +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?

Nah, we want to keep the DP implementation together and for now the
configuration structs live at the top of the file.

> >   
> >   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

Thanks for reviewing all of these these.

Johan

Powered by blists - more mailing lists