lkml.org 
[lkml]   [2017]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets
    On Tue 20 Dec 09:03 PST 2016, Vivek Gautam wrote:

    > diff --git a/drivers/phy/phy-qcom-qmp.c b/drivers/phy/phy-qcom-qmp.c
    [..]
    > +static int qcom_qmp_phy_poweron(struct phy *phy)
    [..]
    > +
    > +err3:

    Rather than naming your labels errX, it's idiomatic to give them
    descriptive names, e.g. "disable_ref_clk" here.

    > + clk_disable_unprepare(qphy->ref_clk);
    > +err2:
    > + regulator_disable(qphy->vddp_ref_clk);
    > +err1:
    > + regulator_disable(qphy->vdda_pll);
    > +err:
    > + regulator_disable(qphy->vdda_phy);
    > + return ret;
    > +}
    [..]
    > +static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
    > +{
    > + const struct qmp_phy_cfg *cfg = qphy->cfg;
    > + void __iomem *serdes = qphy->serdes;
    > + int ret;
    > +
    > + mutex_lock(&qphy->phy_mutex);
    > + if (qphy->init_count++) {
    > + mutex_unlock(&qphy->phy_mutex);
    > + return 0;
    > + }

    As far as I can see phy_init() and phy_exit() already keep reference
    count on the initialization and you only call this function from
    phy_ops->init, so you should be able to drop this.

    [..]
    > +
    > +/* PHY Initialization */
    > +static int qcom_qmp_phy_init(struct phy *phy)
    > +{
    [..]
    > + /* phy power down delay; given in PCIE phy programming guide only */
    > + if (qphy->cfg->type == PHY_TYPE_PCIE)

    Rather than basing this off "is this pcie" it's preferred if you have a
    bool power_down_delay; (or optionally carrying the timeout value) to
    control this.

    > + usleep_range(POWER_DOWN_DELAY_US_MIN, POWER_DOWN_DELAY_US_MAX);
    > +
    > + /* start SerDes and Phy-Coding-Sublayer */
    > + qphy_setbits(pcs + QPHY_START_CTRL, cfg->start_ctrl);
    > +
    > + /* Pull PHY out of reset state */
    > + qphy_clrbits(pcs + QPHY_SW_RESET, SW_RESET);
    > +
    > + status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
    > + mask = cfg->mask_pcs_ready;
    > +
    > + ret = !mask ? 0 : readl_poll_timeout(status, val, !(val & mask), 1,
    > + PHY_INIT_COMPLETE_TIMEOUT);

    I presume it's not okay to read the status register even once for pcie?
    If it is you can skip the conditional as !(val & 0) will break the poll
    after the first read.

    [..]
    > +static int qcom_qmp_phy_exit(struct phy *phy)
    > +{
    [..]
    > +}
    > +
    > +

    Extra blank line

    > +static int qcom_qmp_phy_regulator_init(struct device *dev)
    [..]
    > +static
    > +struct qmp_phy_desc *qcom_qmp_phy_create(struct platform_device *pdev, int id)
    > +{
    [..]
    > + phy_desc->pipe_clk = devm_clk_get(dev, prop_name);
    > + if (IS_ERR(phy_desc->pipe_clk)) {
    > + if (qphy->cfg->type == PHY_TYPE_PCIE ||
    > + qphy->cfg->type == PHY_TYPE_USB3) {

    Is this check adding any value?

    > + ret = PTR_ERR(phy_desc->pipe_clk);
    > + if (ret != -EPROBE_DEFER)
    > + dev_err(dev,
    > + "failed to get lane%d pipe_clk, %d\n",
    > + id, ret);
    > + return ERR_PTR(ret);
    > + }
    > + phy_desc->pipe_clk = NULL;

    Regards,
    Bjorn

    \
     
     \ /
      Last update: 2017-01-06 08:19    [W:4.047 / U:0.864 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site