Re: [PATCH 07/19] clk: fixed-factor: add clk_hw_register_fixed_factor_with_accuracy

["Benoît Monin" <benoit.monin@bootlin.com>]

On Saturday, 20 September 2025 at 07:14:58 CEST, Stephen Boyd wrote:
> Quoting Benoît Monin (2025-09-03 05:47:14)
> > diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
> > index e62ae8794d445f685156276d5135448f340fca3f..7c76658a725f9b268da2485769979e5ba213d25b 100644
> > --- a/drivers/clk/clk-fixed-factor.c
> > +++ b/drivers/clk/clk-fixed-factor.c
> > @@ -217,6 +217,18 @@ struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
> >  }
> >  EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor);
> >  
> > +struct clk_hw *clk_hw_register_fixed_factor_with_accuracy(struct device *dev,
> > +               const char *name, const char *parent_name, unsigned long flags,
> > +               unsigned int mult, unsigned int div, unsigned long acc)
> > +{
> > +       const struct clk_parent_data pdata = { .index = -1 };
> 
> This is wrong. We're passing parent data and also setting the parent
> name with a string the original way. Do you have a parent?
> 
This is caused by the handling of the init struct in
__clk_hw_register_fixed_factor(). In particular num_parents
is always set to 1:
	...
	if (parent_name)
		init.parent_names = &parent_name;
	else if (parent_hw)
		init.parent_hws = &parent_hw;
	else
		init.parent_data = pdata;
	init.num_parents = 1;

We could adopt the same approach as in __clk_hw_register_divider() to set
num_parents only when one is passed-in, so the invalid pdata could be
dropped:
	...
	init.parent_names = parent_name ? &parent_name : NULL;
	init.parent_hws = parent_hw ? &parent_hw : NULL;
	init.parent_data = parent_data;
	if (parent_name || parent_hw || parent_data)
		init.num_parents = 1;
	else
		init.num_parents = 0;

But doing so would change what happens when a fixed factor is registered
with an invalid parent, for example calling clk_hw_register_fixed_factor()
with a NULL parent_name. Currently we get an orphaned clock (seen in
clk_orphan_summary) since it is created with an invalid parent. With the
change it would register without zero parent but not be considered orphaned
(a "root" clock).

Do you think it is okay to make this change?

> > +
> > +       return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, NULL,
> > +                                             &pdata, flags, mult, div, acc,
> > +                                             CLK_FIXED_FACTOR_FIXED_ACCURACY, false);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor_with_accuracy);
> 
> There are a handful of these wrappers now. Can we have one function that
> takes all possibilities and then static inline functions that call one
> exported function?  We can pass some structs to that function to keep
> the argument count "low", so those structs live on the stack for a short
> time.
> 
Yes, we could use the same approach as clk-divider, clk-gate and others.
Export the __clk_hw_register_fixed_factor() symbol in the header and have
macros / inline functions.

> > +
> >  struct clk_hw *clk_hw_register_fixed_factor_fwname(struct device *dev,
> >                 struct device_node *np, const char *name, const char *fw_name,
> >                 unsigned long flags, unsigned int mult, unsigned int div)
> 


Best regards,
-- 
Benoît Monin, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com