Re: About clk maintainership [Was: Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks]

[Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>]

On Sat, 31 Jul 2021 14:00:04 +0200
Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:

> Hi Russell, hi Stephen,
> 
> On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote:
> > Quoting Russell King (Oracle) (2021-07-28 13:40:34)  
> > > > I adapted the Subject in the hope to catch Stephen's and Michael's
> > > > attention. My impression is that this thread isn't on their radar yet,
> > > > but the topic here seems important enough to get a matching Subject.  
> > 
> > The thread is on my radar but...
> >   
> > > 
> > > Have you thought about sending your pull request to the clk API
> > > maintainer (iow, me) ?  
> 
> I wasn't really aware that Russell has the clk API hat (or that this
> separate hat actually exists and this isn't purely a CCF topic). I
> assume I only did
> 
> 	$ scripts/get_maintainer.pl -f drivers/clk/clk-devres.c
> 
> which is where the current and new code implementing devm_clk_get et al
> lives.
> 
> @Russell: What is your position here, do you like the approach of
> devm_clk_get_enabled? I can send a new pull request in your direction if
> you like it and are willing to take it.
> 
> > +1 This patch doesn't fall under CCF maintainer.  
> 
> Given that CCF is the only implementer of devm_clk_get at least an Ack
> from your side would still be good I guess?
> 
> > Finally, this sort of patch has been discussed for years and I didn't
> > see any mention of those previous attempts in the patch series. Has
> > something changed since that time? I think we've got a bunch of hand
> > rolled devm things in the meantime but not much else.   
> 
> I found a patch set adding devm variants of clk_enable (e.g.
> https://lore.kernel.org/patchwork/patch/755667/) but this approach is
> different as it also contains clk_get which IMHO makes more sense 
> The discussion considered wrapping get+enable at one point, but I didn't
> find a followup.
> 
> > I still wonder if it would be better if we had some sort of DT binding
> > that said "turn this clk on when the driver probes this device and keep
> > it on until the driver is unbound".  
> 
> This doesn't sound like a hardware property and so I don't think this
> belongs into DT and I would be surprised if the dt maintainers would be
> willing to accept an idea with this semantic.

Agreed. It's not unheard of to have a driver start out just enabing
clock at probe and dropping it at remove. When the driver gets more
sophisticated it will then manage the clock more frequently.
Whilst that's often tied to runtime_pm I'm not sure it always is.

Given the mess that would be involved in having a property that we
need to later ignore for particular drivers, I'd keep this management
explicit in the driver. This series makes that trivial to do for these
easy cases.

Jonathan

> 
> > That would probably work well for quite a few drivers that don't want
> > to ever call clk_get() or clk_prepare_enable() and could tie into the
> > assigned-clock-rates property in a way that let us keep the platform
> > details out of the drivers. We could also go one step further and make
> > a clk pm domain when this new property is present and then have the
> > clk be enabled based on runtime PM of the device (and if runtime PM is
> > disabled then just enable it at driver probe time).  
> 
> clk pm domain sounds good, but introducing devm_clk_get_enabled() is
> much easier and converting to it can be done without dt changes and more
> or less mechanically. So I consider the cost-usage-value of
> devm_clk_get_enabled() much better.
> 
> Best regards
> Uwe
>