Re: [PATCH v1 08/11] clk: move meson clk-regmap implementation to common code

[Conor Dooley]

On Wed, Oct 02, 2024 at 03:21:16PM +0200, Jerome Brunet wrote:
> On Wed 02 Oct 2024 at 13:20, Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:
> 
> > On 02/10/2024 12:48, Conor Dooley wrote:
> >> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> >> I like this one better than qualcomms and wish to use it for the
> >> PolarFire SoC clock drivers.
> >> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> >> ---
> >>   drivers/clk/Kconfig                           |  4 ++
> >>   drivers/clk/Makefile                          |  1 +
> >>   drivers/clk/{meson => }/clk-regmap.c          |  2 +-
> >>   drivers/clk/meson/Kconfig                     | 46 +++++++++----------
> >>   drivers/clk/meson/Makefile                    |  1 -
> >>   drivers/clk/meson/a1-peripherals.c            |  2 +-
> >>   drivers/clk/meson/a1-pll.c                    |  2 +-
> >>   drivers/clk/meson/axg-aoclk.c                 |  2 +-
> >>   drivers/clk/meson/axg-audio.c                 |  2 +-
> >>   drivers/clk/meson/axg.c                       |  2 +-
> >>   drivers/clk/meson/c3-peripherals.c            |  2 +-
> >>   drivers/clk/meson/c3-pll.c                    |  2 +-
> >>   drivers/clk/meson/clk-cpu-dyndiv.c            |  2 +-
> >>   drivers/clk/meson/clk-dualdiv.c               |  2 +-
> >>   drivers/clk/meson/clk-mpll.c                  |  2 +-
> >>   drivers/clk/meson/clk-phase.c                 |  2 +-
> >>   drivers/clk/meson/clk-pll.c                   |  2 +-
> >>   drivers/clk/meson/g12a-aoclk.c                |  2 +-
> >>   drivers/clk/meson/g12a.c                      |  2 +-
> >>   drivers/clk/meson/gxbb-aoclk.c                |  2 +-
> >>   drivers/clk/meson/gxbb.c                      |  2 +-
> >>   drivers/clk/meson/meson-aoclk.h               |  2 +-
> >>   drivers/clk/meson/meson-eeclk.c               |  2 +-
> >>   drivers/clk/meson/meson-eeclk.h               |  2 +-
> >>   drivers/clk/meson/meson8-ddr.c                |  2 +-
> >>   drivers/clk/meson/meson8b.c                   |  2 +-
> >>   drivers/clk/meson/s4-peripherals.c            |  2 +-
> >>   drivers/clk/meson/s4-pll.c                    |  2 +-
> >>   drivers/clk/meson/sclk-div.c                  |  2 +-
> >>   drivers/clk/meson/vclk.h                      |  2 +-
> >>   drivers/clk/meson/vid-pll-div.c               |  2 +-
> >>   .../meson => include/linux/clk}/clk-regmap.h  |  0
> >>   32 files changed, 53 insertions(+), 53 deletions(-)
> >>   rename drivers/clk/{meson => }/clk-regmap.c (99%)
> >>   rename {drivers/clk/meson => include/linux/clk}/clk-regmap.h (100%)
> >> 
> > <snip>
> >
> > I don't have objections, but I think Stephen didn't like the idea
> > a few years ago, but perhaps it has changed...
> >
> > Anyway, take my:
> > Acked-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> 
> We had a similar discussion 3y ago indeed:
> https://lore.kernel.org/linux-clk/162734682512.2368309.12015873010777083014@xxxxxxxxxxxxxxxxxxxxxxxxxx/
> 
> There are needs for a common regmap backed clocks indeed, but allowing
> meson flavored regmap clocks to spread in the kernel was not really the
> prefered way to do it. 

Cool, thanks for that link.

> IIRC, Stephen's idea was more the bring regmap support in clk-gate.c,
> clk-mux, etc ... I'm not quite sure how make iomem and regmap co-exist
> in a manageable/maintainable way within those drivers (without adding yet
> another level of abstraction I mean) ? Silently creating a regmap maybe
> ? but that's probably a bit heavy. I did not really had time to dig more
> on this, I guess no one did.

I guess I have some motivation to looking into it at the moment. I had
my reservations about the Meson approach too, liking it more than
Qualcomm's didn't mean I completely liked it.
It was already my intention to implement point b of your mail, had the
general idea here been acceptable, cos that's a divergence from how the
generic clock types (that the driver in question currently uses) work.
And on that note, I just noticed I left the mild-annoyance variable name
"sigh" in the submitted driver changes, which I had used for the
clk_regmap struct that your point b in the link relates to.

> I don't really have a preference one way or the other but if it is going
> to be exposed in 'include/linux', we need to be sure that's how we want
> to do it. With clocks poping in many driver subsystems, it will
> difficult to change afterward. 

Yeah, I agree. I didn't expect this to go in right away, and I also
didn't want to surge ahead on some rework of the clock types, were
people to hate even the reuse.

Attachment: signature.asc
Description: PGP signature