Regulator: drivers that need to know their supply

Regulator: drivers that need to know their supply

[Martin Fuzzey]

Hi,

I am working on a driver for the tps22993 load switch.

This chip has configurable slew rates but no voltage setting
(as it's just a switch).

To implement the .enable_time() method I need the voltage (which is
the supply's voltage).

The problem is that, when my regulator is configured as always-on the
supply is not known when .enable_time() is called from regulator_register()
(it does work if the regulator is not always-on)

This is similar to the problem fixed by
     5e3ca2b349b1 regulator: Try to resolve regulators supplies on 
registration
and the follow up deadlock fix.

I tried adding a new function:

struct regulator *rdev_get_supply(struct regulator_dev *rdev)
{
     int rc;

     rc = regulator_resolve_supply(rdev);
     if (rc)
         return ERR_PTR(rc);
     return rdev->supply;
}
EXPORT_SYMBOL_GPL(rdev_get_supply);

Unfortunately that doesn't work.
First of all supply_name is not set at that point
(since it is set in regulator_register() AFTER set_machine_constraints())

Even after swapping the call order in regulator_register() it still doesn't
work for my configuration since the parent supply is probed later
so rdev_get_supply() returns -EDEFER which just results in
a warning message from _regulator_do_enable() when it propagates
back from .enable_time()

     3V3_LCD: enable_time() failed: -517

And even without that problem there is a risk of another
deadlock since the driver callbacks are called from
regulator_register() with the regulator_list mutex held
in the always_on case.

Any ideas how to do this?

Regards,

Martin

Re: Regulator: drivers that need to know their supply

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On Thu, Apr 07, 2016 at 03:25:09PM +0200, Martin Fuzzey wrote:

> To implement the .enable_time() method I need the voltage (which is
> the supply's voltage).

No, this is not sensible.  You should be telling the framework about the
slew rate and letting the framework work out how long it's going to take
to transition.  Your driver shouldn't be peering around inside other
regulators, it should be telling the framework what it does itself and
any handling of interrelationships should be in the framework.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Regulator: drivers that need to know their supply

[Martin Fuzzey]

Thanks for your reply.

On 07/04/16 19:02, Mark Brown wrote:
> On Thu, Apr 07, 2016 at 03:25:09PM +0200, Martin Fuzzey wrote:
>
>> To implement the .enable_time() method I need the voltage (which is
>> the supply's voltage).
> No, this is not sensible.  You should be telling the framework about the
> slew rate and letting the framework work out how long it's going to take
> to transition.  Your driver shouldn't be peering around inside other
> regulators, it should be telling the framework what it does itself and
> any handling of interrelationships should be in the framework.

Ok, but I fail to see any way to do that (at least with the framework as 
is).

The framework can tell the driver what slew rate to use (via 
.set_ramp_delay)
And it can ask the driver what the turn on time is (via .enable_time)
It doesn't actullay know the slew rate (as a driver may not have 
configurable
slew rates or the driver may have rounded the slew rate to something it 
supports)

Or are you saying I should extend the framework to add a .get_ramp_delay
driver callback and make the framework do the calculation itself?

Happy to do that if it's the best way but your reply makes it sound
like i'm not using an existing framework functionality and I don't think
that is the case unless I'm overlooking something.

I'm just trying to do what rc5t583 already does in 
rc5t583_regulator_enable_time()
except that in my case I don't know the voltage (since it comes from my 
supply).

While moving it to the framework will avoid the driver knowing about other
regulators it won't fix the underlying issues I mentionned.

For a regulator configured as always-on the framework doesn't lookup and
enable the parent  regulators until regulator_register_resolve_supply()
is caused right at the end of regulator_register().

This means that, when the always-on register is enabled it has no supply
assigned, even at the  framework level.

It also means that currently there is a difference in the switch on order
depending if the regulator is configured as always-on:

Suppose A is the supply for B.

If B is NOT always-on, calling regulator_enable(B) will first enable A 
then B
However if B is always-on, regulator_register(B) will first enable B then A

I'm not sure this actually matters in practice but A then B seems better
from an inrush current point of view.

Regards,

Martin

Re: Regulator: drivers that need to know their supply

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]

On Thu, Apr 07, 2016 at 08:17:25PM +0200, Martin Fuzzey wrote:
> On 07/04/16 19:02, Mark Brown wrote:

> >No, this is not sensible.  You should be telling the framework about the
> >slew rate and letting the framework work out how long it's going to take
> >to transition.  Your driver shouldn't be peering around inside other
> >regulators, it should be telling the framework what it does itself and
> >any handling of interrelationships should be in the framework.

> Ok, but I fail to see any way to do that (at least with the framework as
> is).

Then send patches for the framework.  One of the great things about
working on Linux is that the whole OS is free software so you don't have
to work around things in your driver!  I've not looked at the code for
this specific case.

> Or are you saying I should extend the framework to add a .get_ramp_delay
> driver callback and make the framework do the calculation itself?

Yes.

> While moving it to the framework will avoid the driver knowing about other
> regulators it won't fix the underlying issues I mentionned.

> For a regulator configured as always-on the framework doesn't lookup and
> enable the parent  regulators until regulator_register_resolve_supply()
> is caused right at the end of regulator_register().

> This means that, when the always-on register is enabled it has no supply
> assigned, even at the  framework level.

You might find this changing very shortly but in any case the answer is
still the same, change the framework if it needs changing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]