On Fri, 14 Apr 2023 12:31, AngeloGioacchino Del Regno <angelogioacchino.delre...@collabora.com> wrote: >Il 13/04/23 14:46, Guillaume Ranquet ha scritto: >> The ret variable in mtk_hdmi_pll_calc() was used unitialized as reported >> by the kernel test robot. >> >> Fix the issue by removing the variable altogether and testing out the >> return value of mtk_hdmi_pll_set_hw() >> >> Fixes: 45810d486bb44 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195") >> Reported-by: kernel test robot <l...@intel.com> >> Signed-off-by: Guillaume Ranquet <granq...@baylibre.com> >> --- >> drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c >> b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c >> index abfc077fb0a8..e10da6c4147e 100644 >> --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c >> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c >> @@ -213,7 +213,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy >> *hdmi_phy, struct clk_hw *hw, >> u64 tmds_clk, pixel_clk, da_hdmitx21_ref_ck, ns_hdmipll_ck, pcw; >> u8 txpredivs[4] = { 2, 4, 6, 12 }; >> u32 fbkdiv_low; >> - int i, ret; >> + int i; >> >> pixel_clk = rate; >> tmds_clk = pixel_clk; >> @@ -292,10 +292,9 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy >> *hdmi_phy, struct clk_hw *hw, >> if (!(digital_div <= 32 && digital_div >= 1)) >> return -EINVAL; >> >> - mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low, >> + if (mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low, >> PLL_FBKDIV_HS3, posdiv1, posdiv2, txprediv, >> - txposdiv, digital_div); >> - if (ret) >> + txposdiv, digital_div)) >> return -EINVAL; >> > >I don't get why we're returning -EINVAL unconditionally in the first place, >here. > >Function mtk_hdmi_pll_set_hw() should return zero or a negative error number: >in >that case, the previous *intention* was fine, so this should be >
Hi Angelo, I was maybe a bit too quick on fixing this that way. Anyway it doesn't change a thing as mtk_hdmi_pll_set_hw() eitheir returns 0 or -EINVAL. But I agree that the logic is dubious and propagating the return value is the right thing to do. I see that Arnd and Tom posted versions that you might prefer: https://lore.kernel.org/linux-phy/20230414075842.4006164-1-a...@kernel.org/ https://lore.kernel.org/linux-phy/20230414122253.3171524-1-t...@redhat.com/ Thx, Guillaume. > ret = mtk_hdmi_pll_set_hw(....) > if (ret) > return ret; > > return 0; > > >Regards, >Angelo