[bug report] drm/msm/hdmi: HDMI 8996 PHY/PLL support

[bug report] drm/msm/hdmi: HDMI 8996 PHY/PLL support

[Dan Carpenter]

Hello Archit Taneja,

The patch e17afdceb4f2: "drm/msm/hdmi: HDMI 8996 PHY/PLL support"
from Feb 25, 2016, leads to the following
Smatch static checker warning:

   drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c:606 hdmi_8996_pll_prepare()
   info: return a literal instead of 'ret'

   drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c:622 hdmi_8996_pll_prepare()
   info: return a literal instead of 'ret'

drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c
    592 static int hdmi_8996_pll_prepare(struct clk_hw *hw)
    593 {
    594         struct hdmi_pll_8996 *pll = hw_clk_to_pll(hw);
    595         struct hdmi_phy *phy = pll_get_phy(pll);
    596         int i, ret = 0;
    597 
    598         hdmi_phy_write(phy, REG_HDMI_8996_PHY_CFG, 0x1);
    599         udelay(100);
    600 
    601         hdmi_phy_write(phy, REG_HDMI_8996_PHY_CFG, 0x19);
    602         udelay(100);
    603 
    604         ret = hdmi_8996_pll_lock_status(pll);
    605         if (!ret)
--> 606                 return ret;

If this is not locked it returns success?  That's strange.

    607 
    608         for (i = 0; i < HDMI_NUM_TX_CHANNEL; i++)
    609                 hdmi_tx_chan_write(pll, i,
    610                         REG_HDMI_PHY_QSERDES_TX_LX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN,
    611                         0x6F);
    612 
    613         /* Disable SSC */
    614         hdmi_pll_write(pll, REG_HDMI_PHY_QSERDES_COM_SSC_PER1, 0x0);
    615         hdmi_pll_write(pll, REG_HDMI_PHY_QSERDES_COM_SSC_PER2, 0x0);
    616         hdmi_pll_write(pll, REG_HDMI_PHY_QSERDES_COM_SSC_STEP_SIZE1, 0x0);
    617         hdmi_pll_write(pll, REG_HDMI_PHY_QSERDES_COM_SSC_STEP_SIZE2, 0x0);
    618         hdmi_pll_write(pll, REG_HDMI_PHY_QSERDES_COM_SSC_EN_CENTER, 0x2);
    619 
    620         ret = hdmi_8996_phy_ready_status(phy);
    621         if (!ret)
    622                 return ret;

If it's not ready then this returns success.  That seems very wrong.  I
would have expected -EIO or something.

    623 
    624         /* Restart the retiming buffer */
    625         hdmi_phy_write(phy, REG_HDMI_8996_PHY_CFG, 0x18);
    626         udelay(1);
    627         hdmi_phy_write(phy, REG_HDMI_8996_PHY_CFG, 0x19);
    628 
    629         return 0;
    630 }

regards,
dan carpenter

Re: Fwd: [bug report] drm/msm/hdmi: HDMI 8996 PHY/PLL support

[jesszhan]

Hey Dan,

On 2021-10-04 16:26, abhinavk@codeaurora.org wrote:
> -------- Original Message --------
> Subject: [bug report] drm/msm/hdmi: HDMI 8996 PHY/PLL support
> Date: 2021-10-04 04:59
> From: Dan Carpenter <dan.carpenter@oracle.com>
> To: architt@codeaurora.org
> Cc: linux-arm-msm@vger.kernel.org
> 
> Hello Archit Taneja,
> 
> The patch e17afdceb4f2: "drm/msm/hdmi: HDMI 8996 PHY/PLL support"
> from Feb 25, 2016, leads to the following
> Smatch static checker warning:
> 
>    drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c:606 hdmi_8996_pll_prepare()
>    info: return a literal instead of 'ret'
> 
>    drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c:622 hdmi_8996_pll_prepare()
>    info: return a literal instead of 'ret'
> 
> drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c
>     592 static int hdmi_8996_pll_prepare(struct clk_hw *hw)
>     593 {
>     594         struct hdmi_pll_8996 *pll = hw_clk_to_pll(hw);
>     595         struct hdmi_phy *phy = pll_get_phy(pll);
>     596         int i, ret = 0;
>     597
>     598         hdmi_phy_write(phy, REG_HDMI_8996_PHY_CFG, 0x1);
>     599         udelay(100);
>     600
>     601         hdmi_phy_write(phy, REG_HDMI_8996_PHY_CFG, 0x19);
>     602         udelay(100);
>     603
>     604         ret = hdmi_8996_pll_lock_status(pll);
>     605         if (!ret)
> --> 606                 return ret;
> 
> If this is not locked it returns success?  That's strange.
> 
>     607
>     608         for (i = 0; i < HDMI_NUM_TX_CHANNEL; i++)
>     609                 hdmi_tx_chan_write(pll, i,
>     610
> REG_HDMI_PHY_QSERDES_TX_LX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN,
>     611                         0x6F);
>     612
>     613         /* Disable SSC */
>     614         hdmi_pll_write(pll, REG_HDMI_PHY_QSERDES_COM_SSC_PER1, 
> 0x0);
>     615         hdmi_pll_write(pll, REG_HDMI_PHY_QSERDES_COM_SSC_PER2, 
> 0x0);
>     616         hdmi_pll_write(pll,
> REG_HDMI_PHY_QSERDES_COM_SSC_STEP_SIZE1, 0x0);
>     617         hdmi_pll_write(pll,
> REG_HDMI_PHY_QSERDES_COM_SSC_STEP_SIZE2, 0x0);
>     618         hdmi_pll_write(pll,
> REG_HDMI_PHY_QSERDES_COM_SSC_EN_CENTER, 0x2);
>     619
>     620         ret = hdmi_8996_phy_ready_status(phy);
>     621         if (!ret)
>     622                 return ret;
> 
> If it's not ready then this returns success.  That seems very wrong.  I
> would have expected -EIO or something.
> 
>     623
>     624         /* Restart the retiming buffer */
>     625         hdmi_phy_write(phy, REG_HDMI_8996_PHY_CFG, 0x18);
>     626         udelay(1);
>     627         hdmi_phy_write(phy, REG_HDMI_8996_PHY_CFG, 0x19);
>     628
>     629         return 0;
>     630 }
> 
> regards,
> dan carpenter

Thanks for the report, will work on it.

Best,
Jessica Zhang