On Fri 20 Dec 2024 at 16:12, Stephen Boyd [off-list ref] wrote:

Quoting Jerome Brunet (2024-12-20 09:17:43)

quoted

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

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.

quoted

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.

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.

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

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 ...)

quoted

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/meson/a1-peripherals.c | 165 +-------------
 drivers/clk/meson/a1-pll.c         |  18 +-
 drivers/clk/meson/axg-aoclk.c      |  22 --
 drivers/clk/meson/axg-audio.c      | 435 +------------------------------------
 drivers/clk/meson/axg.c            | 131 -----------
 drivers/clk/meson/c3-peripherals.c | 212 +-----------------
 drivers/clk/meson/c3-pll.c         |  34 +--
 drivers/clk/meson/clk-cpu-dyndiv.c |   1 +
 drivers/clk/meson/clk-dualdiv.c    |   2 +
 drivers/clk/meson/clk-mpll.c       |   6 +
 drivers/clk/meson/clk-phase.c      |  11 +
 drivers/clk/meson/clk-pll.c        |   7 +
 drivers/clk/meson/clk-regmap.c     |  88 ++++++++
 drivers/clk/meson/clk-regmap.h     |   7 +
 drivers/clk/meson/g12a-aoclk.c     |  34 ---
 drivers/clk/meson/g12a.c           | 261 ----------------------
 drivers/clk/meson/gxbb-aoclk.c     |  19 --
 drivers/clk/meson/gxbb.c           | 393 ---------------------------------
 drivers/clk/meson/meson-aoclk.c    |   7 +-
 drivers/clk/meson/meson-aoclk.h    |   2 -
 drivers/clk/meson/meson-eeclk.c    |   6 +-
 drivers/clk/meson/meson-eeclk.h    |   2 -
 drivers/clk/meson/meson8-ddr.c     |  11 +-
 drivers/clk/meson/s4-peripherals.c | 231 +-------------------
 drivers/clk/meson/s4-pll.c         |  33 +--
 drivers/clk/meson/sclk-div.c       |   5 +
 drivers/clk/meson/vclk.c           |   2 +
 drivers/clk/meson/vid-pll-div.c    |   1 +
 28 files changed, 165 insertions(+), 1981 deletions(-)

It would be more focused if you split the patch into two. One that
installs the init clk_op and implements the logic to hook up the regmap
and one that removes the arrays that are no longer used.

I should have done so indeed.


-- 
Jerome