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

Groups > linux.kernel > #1584835 > unrolled thread

Re: [PATCH v2] regulator: devres: introduce managed enable and disable operations

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] regulator: devres: introduce managed enable and  disable operations Mark Brown <broonie@kernel.org> - 2017-02-20 20:10 +0100
    Re: [PATCH v2] regulator: devres: introduce managed enable and  disable operations Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-21 09:40 +0100
      Re: [PATCH v2] regulator: devres: introduce managed enable and  disable operations Mark Brown <broonie@kernel.org> - 2017-02-21 20:10 +0100
        Re: [PATCH v2] regulator: devres: introduce managed enable and  disable operations Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-02-23 08:30 +0100

#1584835 — Re: [PATCH v2] regulator: devres: introduce managed enable and disable operations

[Multipart message — attachments visible in raw view] — view raw

On Mon, Feb 13, 2017 at 10:51:52AM -0800, Dmitry Torokhov wrote:

> I think it is helps if you think about devm_regulator_enable and regular
> regulator_enable as managed and unmanaged *actions*, not resources. So

That's how I see them but it's still not really helping my concern, in
general if you do a thing with devm_ you don't want to also be
interacting with the same resource in the same way with a non-managed
call.

> managed action of enabling regulator will be undone on remove() and you
> have to manually undo unmanaged regulator_disable() on resume(). It is
> not worse than having unbalanced regulator_enable/disable between
> probe()/suspend()/resume()/remove().

I find it that bit harder to think about - tracking balancing of the
same thing is a lot easier than tracking balancing of two different not
quite equivalent things.

[toc] | [next] | [standalone]

#1585126

On Mon, Feb 20, 2017 at 11:02:58AM -0800, Mark Brown wrote:
> On Mon, Feb 13, 2017 at 10:51:52AM -0800, Dmitry Torokhov wrote:
> 
> > I think it is helps if you think about devm_regulator_enable and regular
> > regulator_enable as managed and unmanaged *actions*, not resources. So
> 
> That's how I see them but it's still not really helping my concern, in
> general if you do a thing with devm_ you don't want to also be
> interacting with the same resource in the same way with a non-managed
> call.

It really depends on how you structure your API. For input, for example,
I only provide devm_input_alloc_device() and I made the rest of the
functions handle both managed and unmanaged input devices and they
internally sort it all out between themselves.

But that is what I meant here about managed action. You are not
interacting with managed regulator here, you have managed enable. There
is absolutely nothing preventing you from calling
devm_regulator_enable() on a regulator that was obtained with
regulator_get() (i.e. non-managed).

> 
> > managed action of enabling regulator will be undone on remove() and you
> > have to manually undo unmanaged regulator_disable() on resume(). It is
> > not worse than having unbalanced regulator_enable/disable between
> > probe()/suspend()/resume()/remove().
> 
> I find it that bit harder to think about - tracking balancing of the
> same thing is a lot easier than tracking balancing of two different not
> quite equivalent things.

Hmm... so what do we do (because I think this devm API is quite useful
for cleaning up probe and remove in many drivers)? Do you want it to
operate on a separate counter which we can check against underflow
separately from classic regulator_enable() and regulator_disable()?
Not sure if this will buy us much though and it will make bulk code
uglier...

Thanks.

-- 
Dmitry

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

#1585630

[Multipart message — attachments visible in raw view] — view raw

On Tue, Feb 21, 2017 at 12:30:03AM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 20, 2017 at 11:02:58AM -0800, Mark Brown wrote:
> > On Mon, Feb 13, 2017 at 10:51:52AM -0800, Dmitry Torokhov wrote:

> But that is what I meant here about managed action. You are not
> interacting with managed regulator here, you have managed enable. There
> is absolutely nothing preventing you from calling
> devm_regulator_enable() on a regulator that was obtained with
> regulator_get() (i.e. non-managed).

That's not the point, the point is using both devm_regulator_enable()
and regulator_enable() and so on.

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

#1586696

On Tue, Feb 21, 2017 at 10:56:34AM -0800, Mark Brown wrote:
> On Tue, Feb 21, 2017 at 12:30:03AM -0800, Dmitry Torokhov wrote:
> > On Mon, Feb 20, 2017 at 11:02:58AM -0800, Mark Brown wrote:
> > > On Mon, Feb 13, 2017 at 10:51:52AM -0800, Dmitry Torokhov wrote:
>
> > But that is what I meant here about managed action. You are not
> > interacting with managed regulator here, you have managed enable. There
> > is absolutely nothing preventing you from calling
> > devm_regulator_enable() on a regulator that was obtained with
> > regulator_get() (i.e. non-managed).
>
> That's not the point, the point is using both devm_regulator_enable()
> and regulator_enable() and so on.

I understand that you have objection that devm_regulator_enable() and
regulator_enable() can be used together, I just do not see it being a
problem in practice.

I still think we need a way for the drivers to "undo" the enable
automatically. Do you have some other idea how to achieve this? Do you
maybe want regulator_put() to undo all outstanding disables for the
regulator? Then drivers would not need to care about disabling
regulators in error paths/driver teardown.

Where would you want to take the API?

Thanks.

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web