Re: [PATCH 2/3] clk: amlogic: drop clk_regmap tables

[Stephen Boyd <sboyd@kernel.org>]

Quoting Jerome Brunet (2024-12-21 03:09:28)
> On Fri 20 Dec 2024 at 16:12, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Jerome Brunet (2024-12-20 09:17:43)
> >> Remove the big clk_regmap tables that are used to populate the regmap
> >> field of clk_regmap clocks at runtime. Instead of using tables, use devres
> >> to allow the clocks to get the necessary regmap.
> >> 
> >> A simpler solution would have been to use dev_get_regmap() but this would
> >> not work with syscon based controllers.
> >
> > Why not have two init functions, one that uses the syscon approach from
> > the parent device?
> 
> That would duplicate all the ops and would not scale if anything else
> comes along. It would also tie the controller quirks with
> clock ops. I would like to keep to clock ops and controllers decoupled as
> much as possible

Hmm... Maybe the init function should be moved out of the clk_ops and
into the clk_init_data structure. It isn't used beyond registration time
anyway, so it may make sense to do that and decouple the clk_ops from
the controllers completely. Or we can have two init routines, one for
the software side and one for the hardware side, but that's probably
confusing. If anything, a clk hardware init function can be exported and
called from the clk software init function if needed.

> 
> > Is the typical path to not use a syscon anyway?
> >
> 
> I sure hope there will be no new syscon based controller but, ATM, around
> 50% are syscon based in drivers/clk/meson. Those are here to stay and I
> doubt we can do anything about it.

Ok.

> 
> >> 
> >> This rework save a bit memory and the result is less of a maintenance
> >> burden.
> >> 
> >> Unfortunately meson8b is left out for now since it is an early clock
> >> driver that does not have proper device support for now.
> >
> > We should add a clk_hw_get_of_node() function that returns
> > hw->core->of_node. Then there can be a similar function that looks at
> > the of_node of a clk registered with of_clk_hw_register() and tries to
> > find the regmap either with
> > syscon_node_to_regmap(clk_hw_get_of_node(hw)) or on the parent of the
> > node for the clk.
> 
> That's the thing. It means encoding the controller quirk of how to get
> regmap in the clock ops. I would be prefer to avoid that.

So if we moved the init function out of struct clk_ops it would work?
We could have helpers for the common paths, i.e. the device has the
regmap, or the syscon has the regmap, etc.

> 
> With what you are suggesting I could make an ops that
> * Try dev_get_regmap() first
> * Try the syscon/of_node way next
> 
> I can make this "trial an error" approach work but I think it is pretty
> nasty and encode controller stuff inside the clock driver.

I get it. The difference in driver design while sharing the same clk
hardware and clk_ops causes this tension.

> 
> >
> > TL;DR: Don't use devres.
> 
> Using it makes thing nice and tidy. clk_regmap does not care were regmap
> comes from. It just picks it up where it has been prepared

It doesn't work for early clocks that don't have a device.

> 
> That approach could be extended to support controller with multiple
> regmaps, with a name that does not depend on regmap_config and is local
> to the clock controller. This will be useful when the name if defined
> somewhere else (syscon, auxiliary device, etc ...)
> 

I think you're saying that clk_ops can be common things that aren't
device/clk controller specific, while the regmap config is usually
device/clk controller specific. Furthermore, the name of the regmap is
also usually device/clk controller specific. The regmap assignment
doesn't really fit with the clk_ops because it's not operating on the
clk hardware like the other clk_ops all do.