Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570031 > unrolled thread

Re: [PATCH v2] clk: add more managed APIs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] clk: add more managed APIs Stephen Boyd <sboyd@codeaurora.org> - 2017-01-30 20:10 +0100
    Re: [PATCH v2] clk: add more managed APIs Guenter Roeck <linux@roeck-us.net> - 2017-01-30 20:30 +0100
      Re: [PATCH v2] clk: add more managed APIs Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-01-30 22:50 +0100
        Re: [PATCH v2] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-30 23:00 +0100
          Re: [PATCH v2] clk: add more managed APIs Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-01-30 23:30 +0100
        Re: [PATCH v2] clk: add more managed APIs Guenter Roeck <linux@roeck-us.net> - 2017-01-31 00:00 +0100
          Re: [PATCH v2] clk: add more managed APIs Geert Uytterhoeven <geert@linux-m68k.org> - 2017-01-31 10:00 +0100
      Re: [PATCH v2] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-31 02:10 +0100
        Re: [PATCH v2] clk: add more managed APIs Guenter Roeck <linux@roeck-us.net> - 2017-01-31 18:30 +0100
          Re: [PATCH v2] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-31 19:30 +0100
            Re: [PATCH v2] clk: add more managed APIs Guenter Roeck <linux@roeck-us.net> - 2017-01-31 22:30 +0100
    [PATCH v3] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-01-31 02:00 +0100
      Re: [PATCH v3] clk: add more managed APIs Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-07 05:00 +0100

#1570031 — Re: [PATCH v2] clk: add more managed APIs

On 01/29, Dmitry Torokhov wrote:
> When converting a driver to managed resources it is desirable to be able to
> manage all resources in the same fashion. This change allows managing
> clocks in the same way we manage many other resources.

Can you please add 'managing clock prepared and enabled state in
the same way'?

The current wording makes it sound like we don't have
devm_clk_get() when we do.

> 
> This adds the following managed APIs:
> 
> - devm_clk_prepare()/devm_clk_unprepare();
> - devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
be even shorter to have the APIs

  devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
  devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()

instead?

Is there any other subsystem that has similar functionality?
Regulators? GPIOs? Resets? I'm just curious if those subsystems
also need similar changes.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1570052

On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
> On 01/29, Dmitry Torokhov wrote:
> > When converting a driver to managed resources it is desirable to be able to
> > manage all resources in the same fashion. This change allows managing
> > clocks in the same way we manage many other resources.
> 
> Can you please add 'managing clock prepared and enabled state in
> the same way'?
> 
> The current wording makes it sound like we don't have
> devm_clk_get() when we do.
> 
> > 
> > This adds the following managed APIs:
> > 
> > - devm_clk_prepare()/devm_clk_unprepare();
> > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> 
> Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
> be even shorter to have the APIs
> 
>   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
>   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
> 
> instead?
> 
In many cases I see

	devm_clk_get(clk1);
	devm_clk_get(clk2);
	clk_prepare_enable(clk1);
	clk_prepare_enable(clk2);

Sometimes the calls are intertwined with setting the clock rates.

	devm_clk_get(clk);
	clk_set_rate(clk, rate);
	clk_prepare_enable(clk);

Maybe the additional calls make sense; I can imagine they would.
However, I personally would be a bit wary of changing the initialization
order of multi-clock initializations, and I am not sure how a single call
could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
seems like a bit too much).

[ On a side note, why is there no clk_get_prepare_enable() and
  clk_get_prepare() ? Maybe it would be better to introduce those
  together with the matching devm_ functions in a separate patch
  if they are useful. ]

> Is there any other subsystem that has similar functionality?
> Regulators? GPIOs? Resets? I'm just curious if those subsystems
> also need similar changes.
> 
Ultimately yes, and most already do. If I recall correctly, I tried to
introduce devm_ functions for regulators some time ago, but that was
rejected with the comment that it would invite misuse.  At the time
I accepted that; today my reaction would be to counter that pretty much
everything can be misused, and that the potential for misuse should not
penaltize all the valid use cases.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1570127

On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> Maybe the additional calls make sense; I can imagine they would.
> However, I personally would be a bit wary of changing the initialization
> order of multi-clock initializations, and I am not sure how a single call
> could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> seems like a bit too much).
> 
> [ On a side note, why is there no clk_get_prepare_enable() and
>   clk_get_prepare() ? Maybe it would be better to introduce those
>   together with the matching devm_ functions in a separate patch
>   if they are useful. ]

If you take the view that trying to keep clocks disabled is a good way
to save power, then you'd have the clk_prepare() or maybe
clk_prepare_enable() in your runtime PM resume handler, or maybe even
deeper in the driver... the original design goal of the clk API was to
allow power saving and clock control.

With that in mind, getting and enabling the clock together in the
probe function didn't make sense.

I feel that aspect has been somewhat lost, and people now regard much
of the clk API as a bit of a probe-time nusience.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1570134

On Mon, Jan 30, 2017 at 1:42 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
>> Maybe the additional calls make sense; I can imagine they would.
>> However, I personally would be a bit wary of changing the initialization
>> order of multi-clock initializations, and I am not sure how a single call
>> could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
>> seems like a bit too much).
>>
>> [ On a side note, why is there no clk_get_prepare_enable() and
>>   clk_get_prepare() ? Maybe it would be better to introduce those
>>   together with the matching devm_ functions in a separate patch
>>   if they are useful. ]
>
> If you take the view that trying to keep clocks disabled is a good way
> to save power, then you'd have the clk_prepare() or maybe
> clk_prepare_enable() in your runtime PM resume handler, or maybe even
> deeper in the driver... the original design goal of the clk API was to
> allow power saving and clock control.
>
> With that in mind, getting and enabling the clock together in the
> probe function didn't make sense.
>
> I feel that aspect has been somewhat lost, and people now regard much
> of the clk API as a bit of a probe-time nusience.

It really depends on the driver. Devices that are expected to be used
"all the time", like keyboards, that get "opened" very early in the
boot process, maybe even by the kernel itself, usually enable hardware
in probe as doing it later just makes code more complex. Devices that
are "less used" and could provide more power savings (like GPU) may
have more elaborate power management with clocks being turned on and
off as needed. I would not paint clk API as "probe-time nuisance", but
for some class of devices devm CLK API is really helpful.

FWIW I do not think that kitchen sink calls, like
"devm_clk_get_prepare_set_rate_enable" are helpful. Resource
acquisition and use of said resource are logically separate.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1570155

On Mon, Jan 30, 2017 at 01:58:05PM -0800, Dmitry Torokhov wrote:
> FWIW I do not think that kitchen sink calls, like
> "devm_clk_get_prepare_set_rate_enable" are helpful. Resource
> acquisition and use of said resource are logically separate.

That alone is an argument against devm_clk_get_prepare_enable() in
so far that drivers should really get all their resources before
they start to enable anything.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1570172

On Mon, Jan 30, 2017 at 09:42:28PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> > Maybe the additional calls make sense; I can imagine they would.
> > However, I personally would be a bit wary of changing the initialization
> > order of multi-clock initializations, and I am not sure how a single call
> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> > seems like a bit too much).
> > 
> > [ On a side note, why is there no clk_get_prepare_enable() and
> >   clk_get_prepare() ? Maybe it would be better to introduce those
> >   together with the matching devm_ functions in a separate patch
> >   if they are useful. ]
> 
> If you take the view that trying to keep clocks disabled is a good way
> to save power, then you'd have the clk_prepare() or maybe
> clk_prepare_enable() in your runtime PM resume handler, or maybe even
> deeper in the driver... the original design goal of the clk API was to
> allow power saving and clock control.
> 
> With that in mind, getting and enabling the clock together in the
> probe function didn't make sense.
> 
> I feel that aspect has been somewhat lost, and people now regard much
> of the clk API as a bit of a probe-time nusience.
> 

While I understand what you are saying, I think just focusing on power
savings paints a bit of a simplistic view of the clock API and its use.
Power saving is not its only use case. In a system where power saving isn't
the highest priority (say, in a high end switch), it is still extremely
valuable, providing a unified API to the clocks in the system (and there
are lots of clocks in a high end switch). Having seen what happens if there
is _no_ unified API (ie a complete mess of per-clock-driver calls all over
the place), I consider this as at least as or even more important than its
power savings potential. In this use case, one would normally both get and
enable the clock (or, much more likely, clocks) in the probe function.
One would also need remove functions, since interfaces in a high end switch
are typically OIR capable.

For my part, I stumbled over the lack of devm functions for clock APIs recently
when trying to convert watchdog drivers to use devm functions where possible.
Many watchdog drivers do use the clock API to only enable the clock when the
watchdog is actually running. However, there are several which prepare and
enable the clock at probe time, and disable/unprepare on remove. Would it be
possible to convert those to only prepare/enable the clocks if the watchdog
is actually enabled ? Possibly, but I am sure that there are cases where that
is not possible, or feasible. Either way, watchdog drivers are usually only
loaded when actually used, so trying to optimize clock usage would often be
more pain than it is worth.

When I did that conversion, I started out using devm_add_action_or_reset().
While that does work, it was pointed out that using devm functions for the
clock APIs would be a much better solution. As it turns out, devm_add_action()
and devm_add_action_or_reset() is already being used by various drivers to
work around the lack of devm functions for the clock API. With that in mind,
have a choice to make - we can continue forcing people to do that, or we can
introduce devm functions. My vote is for the latter.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1570494

Hi Günter,

On Mon, Jan 30, 2017 at 11:51 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Jan 30, 2017 at 09:42:28PM +0000, Russell King - ARM Linux wrote:
>> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
>> > Maybe the additional calls make sense; I can imagine they would.
>> > However, I personally would be a bit wary of changing the initialization
>> > order of multi-clock initializations, and I am not sure how a single call
>> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
>> > seems like a bit too much).
>> >
>> > [ On a side note, why is there no clk_get_prepare_enable() and
>> >   clk_get_prepare() ? Maybe it would be better to introduce those
>> >   together with the matching devm_ functions in a separate patch
>> >   if they are useful. ]
>>
>> If you take the view that trying to keep clocks disabled is a good way
>> to save power, then you'd have the clk_prepare() or maybe
>> clk_prepare_enable() in your runtime PM resume handler, or maybe even
>> deeper in the driver... the original design goal of the clk API was to
>> allow power saving and clock control.
>>
>> With that in mind, getting and enabling the clock together in the
>> probe function didn't make sense.
>>
>> I feel that aspect has been somewhat lost, and people now regard much
>> of the clk API as a bit of a probe-time nusience.
>
> While I understand what you are saying, I think just focusing on power
> savings paints a bit of a simplistic view of the clock API and its use.
> Power saving is not its only use case. In a system where power saving isn't
> the highest priority (say, in a high end switch), it is still extremely
> valuable, providing a unified API to the clocks in the system (and there
> are lots of clocks in a high end switch). Having seen what happens if there
> is _no_ unified API (ie a complete mess of per-clock-driver calls all over
> the place), I consider this as at least as or even more important than its
> power savings potential. In this use case, one would normally both get and
> enable the clock (or, much more likely, clocks) in the probe function.
> One would also need remove functions, since interfaces in a high end switch
> are typically OIR capable.
>
> For my part, I stumbled over the lack of devm functions for clock APIs recently
> when trying to convert watchdog drivers to use devm functions where possible.
> Many watchdog drivers do use the clock API to only enable the clock when the
> watchdog is actually running. However, there are several which prepare and
> enable the clock at probe time, and disable/unprepare on remove. Would it be
> possible to convert those to only prepare/enable the clocks if the watchdog
> is actually enabled ? Possibly, but I am sure that there are cases where that
> is not possible, or feasible. Either way, watchdog drivers are usually only
> loaded when actually used, so trying to optimize clock usage would often be
> more pain than it is worth.
>
> When I did that conversion, I started out using devm_add_action_or_reset().
> While that does work, it was pointed out that using devm functions for the
> clock APIs would be a much better solution. As it turns out, devm_add_action()
> and devm_add_action_or_reset() is already being used by various drivers to
> work around the lack of devm functions for the clock API. With that in mind,
> have a choice to make - we can continue forcing people to do that, or we can
> introduce devm functions. My vote is for the latter.

W.r.t. clocks, there may be multiple reasons why you care about them:
  1. You need to control a clock output to a slave device (e.g. SPI, i2c,
     audio, ...).
     Here your driver cares about both enabling/disabling the clock, and
     programming its clock rate.
  2. You need to control a clock because synchronous hardware needs a running
     clock to operate.
     Here you only care about enabling/disabling the clock, and this falls
     under the "power saving" umbrella.
     These days it's typically handled by a clock domain driver implementing
     a PM Domain using the genpd framework.  Hence your driver doesn't need
     to manage the clock explicitly (no clk_get()/clk_prepare_enable() etc.),
     but just calls the pm_runtime_*() functions.
     Using pm_runtime_*() is always a good idea, even if currently there is
     no clock to control, as your device may end up in a future SoC that does
     have a clock (or power domains).
  3. Combinations of the above.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1570233

On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
> > On 01/29, Dmitry Torokhov wrote:
> > > When converting a driver to managed resources it is desirable to be able to
> > > manage all resources in the same fashion. This change allows managing
> > > clocks in the same way we manage many other resources.
> > 
> > Can you please add 'managing clock prepared and enabled state in
> > the same way'?
> > 
> > The current wording makes it sound like we don't have
> > devm_clk_get() when we do.
> > 
> > > 
> > > This adds the following managed APIs:
> > > 
> > > - devm_clk_prepare()/devm_clk_unprepare();
> > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> > 
> > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
> > be even shorter to have the APIs
> > 
> >   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
> >   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
> > 
> > instead?
> > 
> In many cases I see
> 
> 	devm_clk_get(clk1);
> 	devm_clk_get(clk2);
> 	clk_prepare_enable(clk1);
> 	clk_prepare_enable(clk2);
> 
> Sometimes the calls are intertwined with setting the clock rates.
> 
> 	devm_clk_get(clk);
> 	clk_set_rate(clk, rate);
> 	clk_prepare_enable(clk);
> 
> Maybe the additional calls make sense; I can imagine they would.
> However, I personally would be a bit wary of changing the initialization
> order of multi-clock initializations, and I am not sure how a single call
> could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> seems like a bit too much).
> 
> [ On a side note, why is there no clk_get_prepare_enable() and
>   clk_get_prepare() ? Maybe it would be better to introduce those
>   together with the matching devm_ functions in a separate patch
>   if they are useful. ]
> 
> > Is there any other subsystem that has similar functionality?
> > Regulators? GPIOs? Resets? I'm just curious if those subsystems
> > also need similar changes.
> > 
> Ultimately yes, and most already do. If I recall correctly, I tried to
> introduce devm_ functions for regulators some time ago, but that was
> rejected with the comment that it would invite misuse.  At the time
> I accepted that; today my reaction would be to counter that pretty much
> everything can be misused, and that the potential for misuse should not
> penaltize all the valid use cases.

I think we should ping Mark again. The only thing we are achieving is
that everyone is using devm_add_action_or_reset() with wrappers around
regulator_put().

As I said elsewhere, there are "always used" devices where it isn't
worth it to postpone enabling regulators.

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1570920

On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
> > > On 01/29, Dmitry Torokhov wrote:
> > > > When converting a driver to managed resources it is desirable to be able to
> > > > manage all resources in the same fashion. This change allows managing
> > > > clocks in the same way we manage many other resources.
> > > 
> > > Can you please add 'managing clock prepared and enabled state in
> > > the same way'?
> > > 
> > > The current wording makes it sound like we don't have
> > > devm_clk_get() when we do.
> > > 
> > > > 
> > > > This adds the following managed APIs:
> > > > 
> > > > - devm_clk_prepare()/devm_clk_unprepare();
> > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> > > 
> > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
> > > be even shorter to have the APIs
> > > 
> > >   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
> > >   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
> > > 
> > > instead?
> > > 
> > In many cases I see
> > 
> > 	devm_clk_get(clk1);
> > 	devm_clk_get(clk2);
> > 	clk_prepare_enable(clk1);
> > 	clk_prepare_enable(clk2);
> > 
> > Sometimes the calls are intertwined with setting the clock rates.
> > 
> > 	devm_clk_get(clk);
> > 	clk_set_rate(clk, rate);
> > 	clk_prepare_enable(clk);
> > 
> > Maybe the additional calls make sense; I can imagine they would.
> > However, I personally would be a bit wary of changing the initialization
> > order of multi-clock initializations, and I am not sure how a single call
> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> > seems like a bit too much).
> > 
> > [ On a side note, why is there no clk_get_prepare_enable() and
> >   clk_get_prepare() ? Maybe it would be better to introduce those
> >   together with the matching devm_ functions in a separate patch
> >   if they are useful. ]
> > 
> > > Is there any other subsystem that has similar functionality?
> > > Regulators? GPIOs? Resets? I'm just curious if those subsystems
> > > also need similar changes.
> > > 
> > Ultimately yes, and most already do. If I recall correctly, I tried to
> > introduce devm_ functions for regulators some time ago, but that was
> > rejected with the comment that it would invite misuse.  At the time
> > I accepted that; today my reaction would be to counter that pretty much
> > everything can be misused, and that the potential for misuse should not
> > penaltize all the valid use cases.
> 
> I think we should ping Mark again. The only thing we are achieving is
> that everyone is using devm_add_action_or_reset() with wrappers around
> regulator_put().
> 
regulator_get() has an equivalent devm_regulator_get(). Maybe it was since
added, or I was thinking about a different function.

Guenter

[toc] | [prev] | [next] | [standalone]

#1570970

On Tue, Jan 31, 2017 at 9:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote:
>> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
>> > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
>> > > On 01/29, Dmitry Torokhov wrote:
>> > > > When converting a driver to managed resources it is desirable to be able to
>> > > > manage all resources in the same fashion. This change allows managing
>> > > > clocks in the same way we manage many other resources.
>> > >
>> > > Can you please add 'managing clock prepared and enabled state in
>> > > the same way'?
>> > >
>> > > The current wording makes it sound like we don't have
>> > > devm_clk_get() when we do.
>> > >
>> > > >
>> > > > This adds the following managed APIs:
>> > > >
>> > > > - devm_clk_prepare()/devm_clk_unprepare();
>> > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
>> > >
>> > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
>> > > be even shorter to have the APIs
>> > >
>> > >   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
>> > >   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
>> > >
>> > > instead?
>> > >
>> > In many cases I see
>> >
>> >     devm_clk_get(clk1);
>> >     devm_clk_get(clk2);
>> >     clk_prepare_enable(clk1);
>> >     clk_prepare_enable(clk2);
>> >
>> > Sometimes the calls are intertwined with setting the clock rates.
>> >
>> >     devm_clk_get(clk);
>> >     clk_set_rate(clk, rate);
>> >     clk_prepare_enable(clk);
>> >
>> > Maybe the additional calls make sense; I can imagine they would.
>> > However, I personally would be a bit wary of changing the initialization
>> > order of multi-clock initializations, and I am not sure how a single call
>> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
>> > seems like a bit too much).
>> >
>> > [ On a side note, why is there no clk_get_prepare_enable() and
>> >   clk_get_prepare() ? Maybe it would be better to introduce those
>> >   together with the matching devm_ functions in a separate patch
>> >   if they are useful. ]
>> >
>> > > Is there any other subsystem that has similar functionality?
>> > > Regulators? GPIOs? Resets? I'm just curious if those subsystems
>> > > also need similar changes.
>> > >
>> > Ultimately yes, and most already do. If I recall correctly, I tried to
>> > introduce devm_ functions for regulators some time ago, but that was
>> > rejected with the comment that it would invite misuse.  At the time
>> > I accepted that; today my reaction would be to counter that pretty much
>> > everything can be misused, and that the potential for misuse should not
>> > penaltize all the valid use cases.
>>
>> I think we should ping Mark again. The only thing we are achieving is
>> that everyone is using devm_add_action_or_reset() with wrappers around
>> regulator_put().
>>
> regulator_get() has an equivalent devm_regulator_get(). Maybe it was since
> added, or I was thinking about a different function.

I think we also need devm_regulator_enable().

Thanks.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1571128

On Tue, Jan 31, 2017 at 10:26:29AM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 31, 2017 at 9:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote:
> >> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> >> > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
> >> > > On 01/29, Dmitry Torokhov wrote:
> >> > > > When converting a driver to managed resources it is desirable to be able to
> >> > > > manage all resources in the same fashion. This change allows managing
> >> > > > clocks in the same way we manage many other resources.
> >> > >
> >> > > Can you please add 'managing clock prepared and enabled state in
> >> > > the same way'?
> >> > >
> >> > > The current wording makes it sound like we don't have
> >> > > devm_clk_get() when we do.
> >> > >
> >> > > >
> >> > > > This adds the following managed APIs:
> >> > > >
> >> > > > - devm_clk_prepare()/devm_clk_unprepare();
> >> > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> >> > >
> >> > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
> >> > > be even shorter to have the APIs
> >> > >
> >> > >   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
> >> > >   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
> >> > >
> >> > > instead?
> >> > >
> >> > In many cases I see
> >> >
> >> >     devm_clk_get(clk1);
> >> >     devm_clk_get(clk2);
> >> >     clk_prepare_enable(clk1);
> >> >     clk_prepare_enable(clk2);
> >> >
> >> > Sometimes the calls are intertwined with setting the clock rates.
> >> >
> >> >     devm_clk_get(clk);
> >> >     clk_set_rate(clk, rate);
> >> >     clk_prepare_enable(clk);
> >> >
> >> > Maybe the additional calls make sense; I can imagine they would.
> >> > However, I personally would be a bit wary of changing the initialization
> >> > order of multi-clock initializations, and I am not sure how a single call
> >> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> >> > seems like a bit too much).
> >> >
> >> > [ On a side note, why is there no clk_get_prepare_enable() and
> >> >   clk_get_prepare() ? Maybe it would be better to introduce those
> >> >   together with the matching devm_ functions in a separate patch
> >> >   if they are useful. ]
> >> >
> >> > > Is there any other subsystem that has similar functionality?
> >> > > Regulators? GPIOs? Resets? I'm just curious if those subsystems
> >> > > also need similar changes.
> >> > >
> >> > Ultimately yes, and most already do. If I recall correctly, I tried to
> >> > introduce devm_ functions for regulators some time ago, but that was
> >> > rejected with the comment that it would invite misuse.  At the time
> >> > I accepted that; today my reaction would be to counter that pretty much
> >> > everything can be misused, and that the potential for misuse should not
> >> > penaltize all the valid use cases.
> >>
> >> I think we should ping Mark again. The only thing we are achieving is
> >> that everyone is using devm_add_action_or_reset() with wrappers around
> >> regulator_put().
> >>
> > regulator_get() has an equivalent devm_regulator_get(). Maybe it was since
> > added, or I was thinking about a different function.
> 
> I think we also need devm_regulator_enable().
> 
Ah, yes, that was it [1]. And, yes, I do see some devm_add_action() calls
around regulator_enable().

Guenter

---
[1] https://lkml.org/lkml/2014/2/1/131

[toc] | [prev] | [next] | [standalone]

#1570220 — [PATCH v3] clk: add more managed APIs

When converting a driver to managed resources it is desirable to be able to
manage all resources in the same fashion. This change allows managing clock
prepared and enabled state in the same way we manage many other resources.

This adds the following managed APIs:

- devm_clk_prepare()/devm_clk_unprepare();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

v3: adjusted commit message, added Guenter's reviewed-by
v2: dropped devm_clk_enable() and devm_clk_disable()

 drivers/clk/clk-devres.c |   98 +++++++++++++++++++++++++++++++---------------
 include/linux/clk.h      |   68 ++++++++++++++++++++++++++++++++
 2 files changed, 134 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 3a218c3a06ae..2ff94ffe11d3 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -9,30 +9,20 @@
 #include <linux/export.h>
 #include <linux/gfp.h>
 
-static void devm_clk_release(struct device *dev, void *res)
+static int devm_clk_create_devres(struct device *dev, struct clk *clk,
+				  void (*release)(struct device *, void *))
 {
-	clk_put(*(struct clk **)res);
-}
+	struct clk **ptr;
 
-struct clk *devm_clk_get(struct device *dev, const char *id)
-{
-	struct clk **ptr, *clk;
-
-	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
+	ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL);
 	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
-	clk = clk_get(dev, id);
-	if (!IS_ERR(clk)) {
-		*ptr = clk;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	*ptr = clk;
+	devres_add(dev, ptr);
 
-	return clk;
+	return 0;
 }
-EXPORT_SYMBOL(devm_clk_get);
 
 static int devm_clk_match(struct device *dev, void *res, void *data)
 {
@@ -44,31 +34,75 @@ static int devm_clk_match(struct device *dev, void *res, void *data)
 	return *c == data;
 }
 
-void devm_clk_put(struct device *dev, struct clk *clk)
+#define DEFINE_DEVM_CLK_DESTROY_OP(destroy_op)				\
+static void devm_##destroy_op##_release(struct device *dev, void *res)	\
+{									\
+	destroy_op(*(struct clk **)res);				\
+}									\
+									\
+void devm_##destroy_op(struct device *dev, struct clk *clk)		\
+{									\
+	WARN_ON(devres_release(dev, devm_##destroy_op##_release,	\
+				devm_clk_match, clk));			\
+}									\
+EXPORT_SYMBOL(devm_##destroy_op)
+
+#define DEFINE_DEVM_CLK_OP(create_op, destroy_op)			\
+DEFINE_DEVM_CLK_DESTROY_OP(destroy_op);					\
+int devm_##create_op(struct device *dev, struct clk *clk)		\
+{									\
+	int error;							\
+									\
+	error = create_op(clk);						\
+	if (error)							\
+		return error;						\
+									\
+	error = devm_clk_create_devres(dev, clk,			\
+					devm_##destroy_op##_release);	\
+	if (error) {							\
+		destroy_op(clk);					\
+		return error;						\
+	}								\
+									\
+	return 0;							\
+}									\
+EXPORT_SYMBOL(devm_##create_op)
+
+DEFINE_DEVM_CLK_DESTROY_OP(clk_put);
+DEFINE_DEVM_CLK_OP(clk_prepare, clk_unprepare);
+DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare);
+
+struct clk *devm_clk_get(struct device *dev, const char *id)
 {
-	int ret;
+	struct clk *clk;
+	int error;
 
-	ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
+	clk = clk_get(dev, id);
+	if (!IS_ERR(clk)) {
+		error = devm_clk_create_devres(dev, clk, devm_clk_put_release);
+		if (error) {
+			clk_put(clk);
+			return ERR_PTR(error);
+		}
+	}
 
-	WARN_ON(ret);
+	return clk;
 }
-EXPORT_SYMBOL(devm_clk_put);
+EXPORT_SYMBOL(devm_clk_get);
 
 struct clk *devm_get_clk_from_child(struct device *dev,
 				    struct device_node *np, const char *con_id)
 {
-	struct clk **ptr, *clk;
-
-	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct clk *clk;
+	int error;
 
 	clk = of_clk_get_by_name(np, con_id);
 	if (!IS_ERR(clk)) {
-		*ptr = clk;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
+		error = devm_clk_create_devres(dev, clk, devm_clk_put_release);
+		if (error) {
+			clk_put(clk);
+			return ERR_PTR(error);
+		}
 	}
 
 	return clk;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index e9d36b3e49de..413dc8f636bd 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -267,6 +267,29 @@ struct clk *devm_get_clk_from_child(struct device *dev,
 				    struct device_node *np, const char *con_id);
 
 /**
+ * devm_clk_prepare - prepare clock source as a managed resource
+ * @dev: device owning the resource
+ * @clk: clock source
+ *
+ * This prepares the clock source for use.
+ *
+ * Must not be called from within atomic context.
+ */
+int devm_clk_prepare(struct device *dev, struct clk *clk);
+
+/**
+ * devm_clk_unprepare - undo preparation of a managed clock source
+ * @dev: device used to prepare the clock
+ * @clk: clock source
+ *
+ * This undoes preparation of a clock, previously prepared with a call
+ * to devm_clk_pepare().
+ *
+ * Must not be called from within atomic context.
+ */
+void devm_clk_unprepare(struct device *dev, struct clk *clk);
+
+/**
  * clk_enable - inform the system when the clock source should be running.
  * @clk: clock source
  *
@@ -295,6 +318,28 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * devm_clk_prepare_enable - prepare and enable a managed clock source
+ * @dev: device owning the clock source
+ * @clk: clock source
+ *
+ * This prepares the clock source for use and enables it.
+ *
+ * Must not be called from within atomic context.
+ */
+int devm_clk_prepare_enable(struct device *dev, struct clk *clk);
+
+/**
+ * devm_clk_disable_unprepare - disable and undo preparation of a managed clock
+ * @dev: device used to prepare and enable the clock
+ * @clk: clock source
+ *
+ * This disables and undoes a previously prepared clock.
+ *
+ * Must not be called from within atomic context.
+ */
+void devm_clk_disable_unprepare(struct device *dev, struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
@@ -460,6 +505,17 @@ static inline void clk_put(struct clk *clk) {}
 
 static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
 
+static inline int devm_clk_prepare(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void devm_clk_unprepare(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;
@@ -467,6 +523,18 @@ static inline int clk_enable(struct clk *clk)
 
 static inline void clk_disable(struct clk *clk) {}
 
+static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void devm_clk_disable_unprepare(struct device *dev,
+					      struct clk *clk)
+{
+	might_sleep();
+}
+
 static inline unsigned long clk_get_rate(struct clk *clk)
 {
 	return 0;

[toc] | [prev] | [next] | [standalone]

#1575363 — Re: [PATCH v3] clk: add more managed APIs

On Mon, Jan 30, 2017 at 04:57:13PM -0800, Dmitry Torokhov wrote:
> When converting a driver to managed resources it is desirable to be able to
> manage all resources in the same fashion. This change allows managing clock
> prepared and enabled state in the same way we manage many other resources.
> 
> This adds the following managed APIs:
> 
> - devm_clk_prepare()/devm_clk_unprepare();
> - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

It would be awesome if we could get it into 4.11...

> ---
> 
> v3: adjusted commit message, added Guenter's reviewed-by
> v2: dropped devm_clk_enable() and devm_clk_disable()
> 
>  drivers/clk/clk-devres.c |   98 +++++++++++++++++++++++++++++++---------------
>  include/linux/clk.h      |   68 ++++++++++++++++++++++++++++++++
>  2 files changed, 134 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 3a218c3a06ae..2ff94ffe11d3 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -9,30 +9,20 @@
>  #include <linux/export.h>
>  #include <linux/gfp.h>
>  
> -static void devm_clk_release(struct device *dev, void *res)
> +static int devm_clk_create_devres(struct device *dev, struct clk *clk,
> +				  void (*release)(struct device *, void *))
>  {
> -	clk_put(*(struct clk **)res);
> -}
> +	struct clk **ptr;
>  
> -struct clk *devm_clk_get(struct device *dev, const char *id)
> -{
> -	struct clk **ptr, *clk;
> -
> -	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
> +	ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL);
>  	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +		return -ENOMEM;
>  
> -	clk = clk_get(dev, id);
> -	if (!IS_ERR(clk)) {
> -		*ptr = clk;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> -	}
> +	*ptr = clk;
> +	devres_add(dev, ptr);
>  
> -	return clk;
> +	return 0;
>  }
> -EXPORT_SYMBOL(devm_clk_get);
>  
>  static int devm_clk_match(struct device *dev, void *res, void *data)
>  {
> @@ -44,31 +34,75 @@ static int devm_clk_match(struct device *dev, void *res, void *data)
>  	return *c == data;
>  }
>  
> -void devm_clk_put(struct device *dev, struct clk *clk)
> +#define DEFINE_DEVM_CLK_DESTROY_OP(destroy_op)				\
> +static void devm_##destroy_op##_release(struct device *dev, void *res)	\
> +{									\
> +	destroy_op(*(struct clk **)res);				\
> +}									\
> +									\
> +void devm_##destroy_op(struct device *dev, struct clk *clk)		\
> +{									\
> +	WARN_ON(devres_release(dev, devm_##destroy_op##_release,	\
> +				devm_clk_match, clk));			\
> +}									\
> +EXPORT_SYMBOL(devm_##destroy_op)
> +
> +#define DEFINE_DEVM_CLK_OP(create_op, destroy_op)			\
> +DEFINE_DEVM_CLK_DESTROY_OP(destroy_op);					\
> +int devm_##create_op(struct device *dev, struct clk *clk)		\
> +{									\
> +	int error;							\
> +									\
> +	error = create_op(clk);						\
> +	if (error)							\
> +		return error;						\
> +									\
> +	error = devm_clk_create_devres(dev, clk,			\
> +					devm_##destroy_op##_release);	\
> +	if (error) {							\
> +		destroy_op(clk);					\
> +		return error;						\
> +	}								\
> +									\
> +	return 0;							\
> +}									\
> +EXPORT_SYMBOL(devm_##create_op)
> +
> +DEFINE_DEVM_CLK_DESTROY_OP(clk_put);
> +DEFINE_DEVM_CLK_OP(clk_prepare, clk_unprepare);
> +DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare);
> +
> +struct clk *devm_clk_get(struct device *dev, const char *id)
>  {
> -	int ret;
> +	struct clk *clk;
> +	int error;
>  
> -	ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
> +	clk = clk_get(dev, id);
> +	if (!IS_ERR(clk)) {
> +		error = devm_clk_create_devres(dev, clk, devm_clk_put_release);
> +		if (error) {
> +			clk_put(clk);
> +			return ERR_PTR(error);
> +		}
> +	}
>  
> -	WARN_ON(ret);
> +	return clk;
>  }
> -EXPORT_SYMBOL(devm_clk_put);
> +EXPORT_SYMBOL(devm_clk_get);
>  
>  struct clk *devm_get_clk_from_child(struct device *dev,
>  				    struct device_node *np, const char *con_id)
>  {
> -	struct clk **ptr, *clk;
> -
> -	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	struct clk *clk;
> +	int error;
>  
>  	clk = of_clk_get_by_name(np, con_id);
>  	if (!IS_ERR(clk)) {
> -		*ptr = clk;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> +		error = devm_clk_create_devres(dev, clk, devm_clk_put_release);
> +		if (error) {
> +			clk_put(clk);
> +			return ERR_PTR(error);
> +		}
>  	}
>  
>  	return clk;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index e9d36b3e49de..413dc8f636bd 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -267,6 +267,29 @@ struct clk *devm_get_clk_from_child(struct device *dev,
>  				    struct device_node *np, const char *con_id);
>  
>  /**
> + * devm_clk_prepare - prepare clock source as a managed resource
> + * @dev: device owning the resource
> + * @clk: clock source
> + *
> + * This prepares the clock source for use.
> + *
> + * Must not be called from within atomic context.
> + */
> +int devm_clk_prepare(struct device *dev, struct clk *clk);
> +
> +/**
> + * devm_clk_unprepare - undo preparation of a managed clock source
> + * @dev: device used to prepare the clock
> + * @clk: clock source
> + *
> + * This undoes preparation of a clock, previously prepared with a call
> + * to devm_clk_pepare().
> + *
> + * Must not be called from within atomic context.
> + */
> +void devm_clk_unprepare(struct device *dev, struct clk *clk);
> +
> +/**
>   * clk_enable - inform the system when the clock source should be running.
>   * @clk: clock source
>   *
> @@ -295,6 +318,28 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * devm_clk_prepare_enable - prepare and enable a managed clock source
> + * @dev: device owning the clock source
> + * @clk: clock source
> + *
> + * This prepares the clock source for use and enables it.
> + *
> + * Must not be called from within atomic context.
> + */
> +int devm_clk_prepare_enable(struct device *dev, struct clk *clk);
> +
> +/**
> + * devm_clk_disable_unprepare - disable and undo preparation of a managed clock
> + * @dev: device used to prepare and enable the clock
> + * @clk: clock source
> + *
> + * This disables and undoes a previously prepared clock.
> + *
> + * Must not be called from within atomic context.
> + */
> +void devm_clk_disable_unprepare(struct device *dev, struct clk *clk);
> +
> +/**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>   *		  This is only valid once the clock source has been enabled.
>   * @clk: clock source
> @@ -460,6 +505,17 @@ static inline void clk_put(struct clk *clk) {}
>  
>  static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
>  
> +static inline int devm_clk_prepare(struct device *dev, struct clk *clk)
> +{
> +	might_sleep();
> +	return 0;
> +}
> +
> +static inline void devm_clk_unprepare(struct device *dev, struct clk *clk)
> +{
> +	might_sleep();
> +}
> +
>  static inline int clk_enable(struct clk *clk)
>  {
>  	return 0;
> @@ -467,6 +523,18 @@ static inline int clk_enable(struct clk *clk)
>  
>  static inline void clk_disable(struct clk *clk) {}
>  
> +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk)
> +{
> +	might_sleep();
> +	return 0;
> +}
> +
> +static inline void devm_clk_disable_unprepare(struct device *dev,
> +					      struct clk *clk)
> +{
> +	might_sleep();
> +}
> +
>  static inline unsigned long clk_get_rate(struct clk *clk)
>  {
>  	return 0;

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web