Re: [RFC PATCH 1/1] phy: mediatek: phy-mtk-hdmi: Use regmap to manipulate hdmi registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] phy: mediatek: uninitialized stack variable used if regmap_read() fails
- [Medium] phy: mediatek: uninitialized stack variable use if regmap_read() fails
- [Low] Build: missing explicit include of <linux/bitfield.h>
--

commit 0d55ccaecdee64be04bc28971e8e4aca3393e3a3
Author: Luca Leonardo Scorcia <l.scorcia@gmail.com>

phy: mediatek: phy-mtk-hdmi: Use regmap to manipulate hdmi registers

This commit transitions the Mediatek HDMI PHY driver to use the regmap API
for register manipulation, removing custom helpers. This resolves an -EBUSY
error on mt8167 devices where the HDMI PHY registers are mapped inside the
apmixedsys memory space.

> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt2701.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt2701.c
> index c9a1f20e939b9..7fcf9116a0a93 100644
> --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt2701.c
> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt2701.c
> @@ -5,7 +5,13 @@
>   */
>  
>  #include "phy-mtk-hdmi.h"
> -#include "phy-mtk-io.h"
> +
> +/* field @mask shall be constant and continuous */
> +#define mtk_phy_update_regmap_field(regmap, reg, mask, val) \
> +({ \
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not constant"); \
> +	regmap_update_bits(regmap, reg, mask, FIELD_PREP(mask, val)); \
> +})

[Severity: Low]
The code removes phy-mtk-io.h which explicitly included <linux/bitfield.h>,
but does not add <linux/bitfield.h> while still using FIELD_PREP. Relying on
implicit transitive inclusion for core bitfield macros can sometimes cause
build failures across different architectures or under varying compiler flags.
Should <linux/bitfield.h> be explicitly included in this file or the common
header?

[ ... ]

> @@ -133,7 +136,7 @@ static unsigned long mtk_hdmi_pll_recalc_rate(struct clk_hw *hw,
>  	unsigned long out_rate, val;
>  	u32 tmp;
>  
> -	tmp = readl(hdmi_phy->regs + HDMI_CON6);
> +	regmap_read(hdmi_phy->regs, HDMI_CON6, &tmp);
>  	val = FIELD_GET(RG_HTPLL_PREDIV_MASK, tmp);

[Severity: Medium]
If regmap_read() fails and returns an error code, tmp is left uninitialized.
Using the uninitialized stack variable tmp in the FIELD_GET macro leads to
undefined behavior, which could result in incorrect PLL clock rates. Should the
return value of regmap_read() be checked here?

>  	switch (val) {
>  	case 0x00:
> @@ -150,7 +153,7 @@ static unsigned long mtk_hdmi_pll_recalc_rate(struct clk_hw *hw,
>  	val = FIELD_GET(RG_HTPLL_FBKDIV_MASK, tmp);
>  	out_rate *= (val + 1) * 2;
>  
> -	tmp = readl(hdmi_phy->regs + HDMI_CON2);
> +	regmap_read(hdmi_phy->regs, HDMI_CON2, &tmp);
>  	val = FIELD_GET(RG_HDMITX_TX_POSDIV_MASK, tmp);

[Severity: Medium]
Similar to the previous read, if regmap_read() fails, tmp remains
uninitialized. Does this also need an error check?

[ ... ]

> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8173.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8173.c
> index 58c6596c8c20b..6df997ff74c97 100644
> --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8173.c
> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8173.c
> @@ -5,7 +5,13 @@
>   */
>  
>  #include "phy-mtk-hdmi.h"
> -#include "phy-mtk-io.h"
> +
> +/* field @mask shall be constant and continuous */
> +#define mtk_phy_update_regmap_field(regmap, reg, mask, val) \
> +({ \
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not constant"); \
> +	regmap_update_bits(regmap, reg, mask, FIELD_PREP(mask, val)); \
> +})

[Severity: Low]
As in the mt2701 driver, <linux/bitfield.h> is not explicitly included but
FIELD_PREP is used. Should the header be explicitly included here as well?

[ ... ]

> diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> index a4bc1268946d8..5d6122277725a 100644
> --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> @@ -15,62 +15,64 @@
>  #include <linux/units.h>
>  #include <linux/nvmem-consumer.h>
>  
> -#include "phy-mtk-io.h"
>  #include "phy-mtk-hdmi.h"
>  #include "phy-mtk-hdmi-mt8195.h"
>  
> +/* field @mask shall be constant and continuous */
> +#define mtk_phy_update_regmap_field(regmap, reg, mask, val) \
> +({ \
> +	BUILD_BUG_ON_MSG(!__builtin_constant_p(mask), "mask is not constant"); \
> +	regmap_update_bits(regmap, reg, mask, FIELD_PREP(mask, val)); \
> +})

[Severity: Low]
Missing explicit include of <linux/bitfield.h> for FIELD_PREP here too.

[ ... ]

> @@ -501,8 +497,11 @@ static int mtk_hdmi_phy_pwr5v_disable(struct regulator_dev *rdev)
>  static int mtk_hdmi_phy_pwr5v_is_enabled(struct regulator_dev *rdev)
>  {
>  	struct mtk_hdmi_phy *hdmi_phy = rdev_get_drvdata(rdev);
> +	u32 tmp;
> +
> +	regmap_read(hdmi_phy->regs, HDMI_CTL_1, &tmp);
>  
> -	return !!(readl(hdmi_phy->regs + HDMI_CTL_1) & RG_HDMITX_PWR5V_O);
> +	return !!(tmp & RG_HDMITX_PWR5V_O);
>  }

[Severity: Medium]
If regmap_read() fails, tmp is uninitialized. The function then evaluates the
regulator enabled state based on stack garbage. Does regmap_read()'s return
value need to be checked to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011082042.348422-1-l.scorcia@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy