Hi Mark,
On Fri, Nov 04, 2022 at 03:44:53PM +0000, Mark Brown wrote:
On Fri, Nov 04, 2022 at 02:18:00PM +0100, Maxime Ripard wrote:
So, the set_parent hook is effectively unused, possibly because of an oversight. However, it could also be an explicit decision by the original author to avoid any reparenting but through an explicit call to clk_set_parent().
The latter case would be equivalent to setting the flag CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook to __clk_mux_determine_rate(). Indeed, if no determine_rate implementation is provided, clk_round_rate() (through clk_core_round_rate_nolock()) will call itself on the parent if CLK_SET_RATE_PARENT is set, and will not change the clock rate otherwise. __clk_mux_determine_rate() has the exact same behavior when CLK_SET_RATE_NO_REPARENT is set.
And if it was an oversight, then we are at least explicit about our behavior now and it can be further refined down the line.
Given that the current approach involves patching every single user to set a default implementation it feels like it might be more straightforward to just have the clock API use that implementation if none is defined - as you say there's already a flag to indicate the unusual case where there's a solid reason to prevent reparenting. It feels like the resulting API is more straightforward.
That would be another solution indeed. The thing is, most places where determine_rate is missing seems to be oversight, and the flag is missing as well.
Just filling determine_rate if it's missing with __clk_mux_determine_rate will possibly pick different parents, and I'm fairly certain that this have never been tested on most platforms, and will be completely broken. And I don't really want to play a game of whack-a-mole adding that flag everywhere it turns out it's broken.
Maxime