On Tue, Jan 21, 2025 at 05:38:53PM +0000, Conor Dooley wrote:

Hey Stephen,

Any thoughts on the example I gave below?

I'll give you a few more days to comment, and then I'll just send a
fresh revision, implemented as below. The links seem to have expired in
a rebase along the way, so I've provided some fresh links.

Cheers,
Conor.

On Fri, Dec 06, 2024 at 01:56:08PM +0000, Conor Dooley wrote:

quoted

On Tue, Dec 03, 2024 at 02:50:31PM -0800, Stephen Boyd wrote:

quoted

Quoting Conor Dooley (2024-11-28 02:36:16)

quoted

On Thu, Nov 14, 2024 at 05:29:54PM -0800, Stephen Boyd wrote:

quoted

Quoting Conor Dooley (2024-11-06 04:56:25)

quoted

My use case doesn't
actually need the registration code changes either as, currently, only reg
gets set at runtime, but leaving that out is a level of incomplete I'd not
let myself away with.
Obviously shoving the extra members into the clk structs has the downside
of taking up a pointer and a offset worth of memory for each clock of
that type registered, but it is substantially easier to support devices
with multiple regmaps that way. Probably moot though since the approach you
suggested in the thread linked above that implements a clk_hw_get_regmap()
has to store a pointer to the regmap's identifier which would take up an
identical amount of memory.

We don't need to store the regmap identifier in the struct clk. We can
store it in the 'struct clk_init_data' with some new field, and only do
that when/if we actually need to. We would need to pass the init data to
the clk_ops::init() callback though. We currently knock that out during
registration so that clk_hw->init is NULL. Probably we can just set that
to NULL after the init routine runs in __clk_core_init().

Long story short, don't add something to 'struct clk_core', 'struct
clk', or 'struct clk_hw' for these details. We can have a 'struct
clk_regmap_hw' that everyone else can build upon:

  struct clk_regmap_hw {
        struct regmap *regmap;
        struct clk_hw hw;
  };

What's the point of this? I don't understand why you want to do this over
what clk_divider et al already do, where clk_hw and the iomem pointer
are in the struct itself.

Can you give an example? I don't understand what you're suggesting. I
prefer a struct clk_regmap_hw like above so that the existing struct
clk_hw in the kernel aren't increased by a pointer. SoC drivers can use
the same struct as a replacement for their struct clk_hw member today.

Best example I guess is to link what I did? This one is the core
changes:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=35904222355e971c24b3eb9b9fad3dd0c38d1393

https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=435c8eb223ee804297a0491fae2b00d3d5a9c773

quoted

clk-gate has my original hack that I did while trying to figure out
what you wanted, clk-divider-regmap is a 99% copy of clk-divider with
the types, function names and readl()/writel() implementations modified.
Before your last set of comments I was doing something identical to the
clk-gate change for clk-divider also.
Here's the changes required to my driver to make it work with the
updated:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=ea40211fe20f8bc6ef0320b93e1baa5b3f244601

https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=syscon-rework-2&id=f55e907e93c55c943725dd62c2fc7dc76cdbd8d5

quoted

It's pretty much a drop in replacement, other than the additional
complexity in probe.

Hopefully that either gets my point across or lets you spot why I don't
understand the benefit of a wrapper around clk_hw.

Cheers,
Conor.