On Tue, Feb 14, 2017 at 11:55:20AM -0800, Dmitry Torokhov wrote: > On Tue, Feb 14, 2017 at 11:44 AM, Stephen Boyd <[email protected]> wrote: > > On 02/06, Dmitry Torokhov wrote: > >> 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 <[email protected]> > >> > Signed-off-by: Dmitry Torokhov <[email protected]> > >> > >> It would be awesome if we could get it into 4.11... > > > > I'd prefer we didn't do this. Instead, make clk_put() drop any > > prepare or enables that were done via that clk pointer. Mike > > started to do this before[1], but we have some code that assumes > > it can do: > > > > clk = clk_get(...) > > clk_prepare_enable(clk) > > clk_put(clk) > > > > and have the clk stay on. Those would need to be changed. > > That means we'd need to audit entire code base ;( > > > > > We would also need Russell's approval to update the clk_put() > > documentation to describe this change in behavior. > > > > [1] > > http://lkml.kernel.org/r/[email protected] > > > > Note that devm* APIs do not preclude from changing clk_put() behavior > down the road and it is extremely easy to go and > s/devm_clk_prepare_enable/clk_prepare_enable/ once cleanup is > automatic. > > Having devm now will help make driver code better (because right now > we either need to add wrappers so devm_add_action_or_reset() can be > used, or continue mixing devm* and goto cleanups, which are often > wrong). >
Absolutely agree. The combination of clk_prepare_enable() in probe and clk_disable_unprepare() in remove is used all over the place. Sure, we _can_ use devm_add_action_or_reset() for all those cases, and I do have a coccinelle script doing just that. While I'll have to accept going forward with that approach if needed, I have to admit that I completely fail to miss the point why that would be a good idea. Thanks, Guenter
