Re: Expose regulator:set_consumer_device_supply()?

Re: Expose regulator:set_consumer_device_supply()?

[Mark Brown]

On Mon, Apr 25, 2011 at 09:16:55PM -0500, Bill Gatliff wrote:
> Guys:

Please always:
- CC maintainers on mails (in this case myself and Liam).
- CC the relevant list (in this case linux-kernel).

> In a nutshell, I have a lot of i2c chips, each of which is powered by
> its own voltage regulator.  Among other things, I'm trying to write
> the i2c drivers so that they are as platform-agnostic as possible,
> because I intend for these drivers to be reused on future platforms
> with different voltage regulator layouts.  On future platforms
> regulators might be shared with multiple i2c chips, for example.

OK, this is absolutely normal for the regulator API.

> What I'm hoping for is i2c driver code that asks for a regulator based
> on the name of the pin to which the regulator is connected.  So
> instead of doing this:

> ... I can do this:
> 
> i2c_chip_probe(i2c_client *this, ...)
> {
> ...
>    /* ask for the regulator connected to our VDD pin */
>    reg = regulator_get(this, "VDD");
> ...
> }

This is exactly what you're supposed to do with the regulator API now.
If you're not doing that the consumer driver is buggy and should be
fixed, it should be written without reference to the board it is running
on.

> The problem with i2c devices is that if you register them with
> i2c_register_board_info(), you don't have a device pointer that you
> can associate a regulator with.  So you have to register the regulator
> after the i2c chip gets registered, which means doing it in the i2c
> chip's probe method.  Ugly, and it won't work when regulators are
> shared between devices.

You can specify the device by either dev_name() or a dev pointer.  You
can use dev_name() at any time without the device having been
instantiated, it would be unusal to use a struct device.

> Any reason why we couldn't expose set_consumer_device_supply(), so
> that I can add a device as a regulator consumer after a regulator is
> already registered?

This would facilitate abuse of the API, we already have enough problems
with people trying to pass regulators as platform data.

Re: Expose regulator:set_consumer_device_supply()?

[Bill Gatliff]

Mark:


Thanks for the response.  But now I'm even more confused than I was before!

On Tue, Apr 26, 2011 at 3:33 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> You can specify the device by either dev_name() or a dev pointer.  You
> can use dev_name() at any time without the device having been
> instantiated, it would be unusal to use a struct device.

When a consumer e.g. i2c chip driver is trying to get a handle for its
own regulator, the only function I see is:

struct regulator* regulator_get(struct device *dev, const char *id)

My understanding, even after looking at the implementation of
regulator_get(), is that the dev pointer refers to the device
structure of the consumer itself.  The regulator returned is one that
matches the combination of that device structure pointer plus the name
of the regulator.  In other words, I get back the regulator that is
unambiguously associated with the consumer.

In order for there to be a regulator with a matching device:id
combination, someone must have previously provided a struct
regulator_consumer_supply with the identical device pointer to
regulator_register().  That means that I have to call
regulator_register() AFTER I register the i2c chip driver, so that I
have a struct device pointer to place in the regulator_consumer_supply
list.  Right?

The alternative is to not indicate any regulator_consumer_supply
devices when I register a regulator, and then do the regulator_get()
by matching on name alone.  But then I have to pass the regulator name
as platform data to the i2c chip driver, because that regulator's name
is bound to change across boards.  And I also lose the strong
relationship between regulators and their downstream devices that
regulator_consumer_supply offers.  If I can keep that association,
then consumers need only ask for "the regulator tied to my VCC pin,
whatever regulator that is".

Maybe I'm misunderstanding the utility of the dev pointer in regulator_get()?

>> Any reason why we couldn't expose set_consumer_device_supply(), so
>> that I can add a device as a regulator consumer after a regulator is
>> already registered?
>
> This would facilitate abuse of the API, we already have enough problems
> with people trying to pass regulators as platform data.

But I think you'll agree that regulators are pretty important platform data, no?

What specifically is the breakage that comes from allowing consumers
to add themselves to regulator consumer lists at a time after
regulator_register() is complete?  Why is passing a regulator pointer
as platform data such a problem?


Confused,


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: Expose regulator:set_consumer_device_supply()?

[Mark Brown]

On Tue, Apr 26, 2011 at 10:33:29AM -0500, Bill Gatliff wrote:
> On Tue, Apr 26, 2011 at 3:33 AM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:

> > You can specify the device by either dev_name() or a dev pointer.  You
> > can use dev_name() at any time without the device having been
> > instantiated, it would be unusal to use a struct device.

> When a consumer e.g. i2c chip driver is trying to get a handle for its
> own regulator, the only function I see is:
> 
> struct regulator* regulator_get(struct device *dev, const char *id)

There's also regulator_get_exclusive() but it's almost exactly the same
thing.

> In order for there to be a regulator with a matching device:id
> combination, someone must have previously provided a struct
> regulator_consumer_supply with the identical device pointer to
> regulator_register().  That means that I have to call
> regulator_register() AFTER I register the i2c chip driver, so that I
> have a struct device pointer to place in the regulator_consumer_supply
> list.  Right?

No.  As I said in the text you've quoted above you can also specify the
device mapping using the dev_name() of the device.  As you will have
seen when looking through the regulator_get() implementation the
matching is actually done on the dev_name(), if the mapping is set up
with a struct device we always do matches based on the dev_name()
string, not by comparing pointers.
  
> > This would facilitate abuse of the API, we already have enough problems
> > with people trying to pass regulators as platform data.

> But I think you'll agree that regulators are pretty important platform data, no?

No, the set of power supplies the device has is not platform data, it's
a physical property of the device.

> What specifically is the breakage that comes from allowing consumers
> to add themselves to regulator consumer lists at a time after
> regulator_register() is complete?  Why is passing a regulator pointer
> as platform data such a problem?

It means you get reams of code in drivers conditionally using the
regulator API, all of which adds needless complexity all over the tree
as people invariably make everything conditional on the regulator not
being there when they shouldn't.  This then means you also end up with
no meaningful error handling, all errors just get silently eaten.

Re: Expose regulator:set_consumer_device_supply()?

[Bill Gatliff]

Mark:

On Tue, Apr 26, 2011 at 11:15 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>
> No.  As I said in the text you've quoted above you can also specify the
> device mapping using the dev_name() of the device.

Aah!  I see it now--- the regulator_consumer_supply structure will
take either the consumer's struct device pointer, or the device name
of the consumer's struct device.  The device name can easily be
predicted (controlled, in fact) before the consumer itself is
registered; in the case of i2c devices, it's the bus-id, i.e.
"0-0038".

Now it all fits together for me.  Thanks for your patience!

> It means you get reams of code in drivers conditionally using the
> regulator API, all of which adds needless complexity all over the tree
> as people invariably make everything conditional on the regulator not
> being there when they shouldn't.  This then means you also end up with
> no meaningful error handling, all errors just get silently eaten.

Now I think I see your point: better to have drivers check the result
of regulator_get() themselves, rather than test a pointer coming in
with the platform data.  And since regulators are often registered as
platform devices themselves, there is no way to get a valid result
from regulator_get() in early-init board code anyway.


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com