[PATCH 2/2] clk: mediatek: fix parent rate lookup for fixed PLL clocks

[sam.shih]

Hi David,
 
Thanks for the feedback.

We will follow your suggestion to move the grandparent_dev declaration
into the else block where it is used. This change, along with the
other minor formatting/cleanup adjustments, will be placed into the new
patch 2/3, so that patch 3/3 is limited to the functional fix for the
parent rate lookup of fixed PLL clocks.


On Fri, 2026-04-10 at 11:07 -0500, David Lechner wrote:
> On 4/9/26 9:13 PM, Weijie Gao wrote:
> > From: Sam Shih <sam.shih at mediatek.com>
> > 
> > The refactoring in commit 00d0ff7f81bf ("clk: mediatek: refactor
> > parent
> > rate lookup functions") introduced a regression where fixed PLL
> > clocks
> > using mtk_clk_fixed_pll_ops are not properly recognized as valid
> > parents
> > in the CLK_PARENT_APMIXED case.
> > 
> > Fixed PLL clocks are implemented using mtk_clk_fixed_pll_ops
> > instead of
> > mtk_clk_apmixedsys_ops, but they can also serve as parent clocks in
> > the
> > APMIXED domain. The parent lookup function needs to check for both
> > driver ops to properly resolve the parent clock device.
> > 
> > Add mtk_clk_fixed_pll_ops checks alongside mtk_clk_apmixedsys_ops
> > checks
> > in mtk_find_parent_rate() to restore support for fixed PLL parent
> > clocks.
> > 
> 
> 
> > Also refactor the grandparent lookup code to extract the repeated
> > dev_get_parent(priv->parent) call into a local variable to keep
> > lines
> > under 100 characters and pass checkpatch validation.
> 
> This sounds like an unrelated change. Let's split that out into a
> separate patch.
> 
> > 
> > Fixes: 00d0ff7f81bf ("clk: mediatek: refactor parent rate lookup
> > functions")
> > Signed-off-by: Sam Shih <sam.shih at mediatek.com>
> > Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
> > ---
> >  drivers/clk/mediatek/clk-mtk.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/clk/mediatek/clk-mtk.c
> > b/drivers/clk/mediatek/clk-mtk.c
> > index 4adec4457b4..26db60dea7f 100644
> > --- a/drivers/clk/mediatek/clk-mtk.c
> > +++ b/drivers/clk/mediatek/clk-mtk.c
> > @@ -207,20 +207,26 @@ static ulong mtk_clk_find_parent_rate(struct
> > clk *clk, int id,
> >  static ulong mtk_find_parent_rate(struct mtk_clk_priv *priv,
> > struct clk *clk,
> >  				  const int parent, u16 flags)
> >  {
> > +	struct udevice *grandparent_dev;
> 
> Putting this here is fine, but we could also declare it inside the
> else scope instead since it is only used there.
> 
> >  	struct udevice *parent_dev;
> >  
> >  	switch (flags & CLK_PARENT_MASK) {
> >  	case CLK_PARENT_APMIXED:
> >  		/* APMIXEDSYS can be parent or grandparent. */
> > -		if (dev_get_driver_ops(clk->dev) ==
> > &mtk_clk_apmixedsys_ops)
> > +		if (dev_get_driver_ops(clk->dev) ==
> > &mtk_clk_apmixedsys_ops ||
> > +		    dev_get_driver_ops(clk->dev) ==
> > &mtk_clk_fixed_pll_ops) {
> >  			parent_dev = clk->dev;
> > -		else if (dev_get_driver_ops(priv->parent) ==
> > &mtk_clk_apmixedsys_ops)
> > +		} else if (dev_get_driver_ops(priv->parent) ==
> > &mtk_clk_apmixedsys_ops ||
> > +			dev_get_driver_ops(priv->parent) ==
> > &mtk_clk_fixed_pll_ops) {
> >  			parent_dev = priv->parent;
> > -		else if (dev_get_driver_ops(dev_get_parent(priv-
> > >parent)) == &mtk_clk_apmixedsys_ops)
> > -			parent_dev = dev_get_parent(priv->parent);
> > -		else
> > -			return -EINVAL;
> > -
> > +		} else {
> > +			grandparent_dev = dev_get_parent(priv-
> > >parent);
> > +			if (dev_get_driver_ops(grandparent_dev) ==
> > &mtk_clk_apmixedsys_ops ||
> > +			    dev_get_driver_ops(grandparent_dev) ==
> > &mtk_clk_fixed_pll_ops)
> > +				parent_dev = grandparent_dev;
> > +			else
> > +				return -EINVAL;
> > +		}
> >  		break;
> >  	case CLK_PARENT_TOPCKGEN:
> >  		if (dev_get_driver_ops(clk->dev) ==
> > &mtk_clk_topckgen_ops)
> 

Best regards,
Sam