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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA8EJpqbrefwFxsudjm=-P8xYSg2Q5tXkvjOSKCt6qqbO5ubPg@mail.gmail.com>
Date:   Mon, 14 Nov 2022 12:22:43 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Johan Hovold <johan@...nel.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 Mon, 14 Nov 2022 at 12:02, Johan Hovold <johan@...nel.org> 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@...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.

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@...aro.org>

-- 
With best wishes
Dmitry