On Mon, 14 Nov 2022 at 12:02, Johan Hovold <johan@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > > ---
> > > 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.
Well, if viewed this way, 80% of kerneldocs's are pointless, as most
of the fields are self-describing.
In this case I can agree with you though. Especially since the struct
is not a part of the public API.
>
> Note how it also incorrect by referring to "memory space for *lane's*
I assumed that `lane's rx' and `lane's tx' were ugly wording. But yeah.
> ...".
>
> > > +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.
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
--
With best wishes
Dmitry
