Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Stephen Boyd <sboyd@kernel.org>]

Quoting Maxime Ripard (2023-03-29 12:50:49)
> On Wed, Mar 22, 2023 at 04:31:04PM -0700, Stephen Boyd wrote:
> 
> > The clk_set_parent() path is valid for those cases. Probably nobody
> > cares about determine_rate because they don't set rates on these clks.
> > Some drivers even explicitly left out determine_rate()/round_rate()
> > because they didn't want to have some other clk round up to the mux
> > and change the parent.
> > 
> > Eventually we want drivers to migrate to determine_rate op so we can get
> > rid of the round_rate op and save a pointer (we're so greedy). It's been
> > 10 years though, and that hasn't been done. Sigh! I can see value in
> > this series from the angle of migrating, but adding a determine_rate op
> > when there isn't a round_rate op makes it hard to reason about. What if
> > something copies the clk_ops or sets a different flag? Now we've just
> > added parent changing support to clk_set_rate(). What if the clk has
> > CLK_SET_RATE_PARENT flag set? Now we're going to ask the parent clk to
> > change rate. Fun bugs.
> > 
> > TL;DR: If the set_parent op exists but determine_rate/round_rate doesn't
> > then the clk is a mux that doesn't want to support clk_set_rate(). Make
> > a new mux function that's the contents of the CLK_SET_RATE_NO_REPARENT
> > branch in clk_mux_determine_rate_flags() and call that directly from the
> > clk_ops so it is clear what's happening,
> > clk_hw_mux_same_parent_determine_rate() or something with a better name.
> > Otherwise migrate the explicit determine_rate op to this new function
> > and don't set the flag.
> > 
> > It may be possible to entirely remove the CLK_SET_RATE_NO_REPARENT flag
> > with this design, if the determine_rate clk_op can call the inner
> > wrapper function instead of __clk_mux_determine_rate*() (those
> > underscores are awful, we should just prefix them with clk_hw_mux_*()
> > and live happier). That should be another patch series.
> 
> Sorry but it's not really clear to me what you expect in the v2 of this
> series (if you even expect one). It looks that you don't like the
> assignment-if-missing idea Mark suggested, but should I just
> rebase/resend or did you expect something else?
> 

Yes, we want explicit code. Just rebase & resend. Don't add a
determine_rate if there isn't a round_rate. Don't add more users of
CLK_SET_RATE_NO_REPARENT. Instead, make an explicit determine_rate
function for that. If you want to work on the removal of
CLK_SET_RATE_NO_REPARENT go for it. Otherwise I'll take care of it after
this series.