* Re: Expose regulator:set_consumer_device_supply()? [not found] <BANLkTi=dCHXEtv+=+sRaDp_Pt3zQy5o7ug@mail.gmail.com> @ 2011-04-26 8:33 ` Mark Brown 2011-04-26 15:33 ` Bill Gatliff 0 siblings, 1 reply; 4+ messages in thread From: Mark Brown @ 2011-04-26 8:33 UTC (permalink / raw) To: Bill Gatliff; +Cc: linux-embedded, linux-kernel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Expose regulator:set_consumer_device_supply()? 2011-04-26 8:33 ` Expose regulator:set_consumer_device_supply()? Mark Brown @ 2011-04-26 15:33 ` Bill Gatliff 2011-04-26 16:15 ` Mark Brown 0 siblings, 1 reply; 4+ messages in thread From: Bill Gatliff @ 2011-04-26 15:33 UTC (permalink / raw) To: Mark Brown; +Cc: linux-embedded, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Expose regulator:set_consumer_device_supply()? 2011-04-26 15:33 ` Bill Gatliff @ 2011-04-26 16:15 ` Mark Brown 2011-04-26 21:15 ` Bill Gatliff 0 siblings, 1 reply; 4+ messages in thread From: Mark Brown @ 2011-04-26 16:15 UTC (permalink / raw) To: Bill Gatliff; +Cc: linux-embedded, linux-kernel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Expose regulator:set_consumer_device_supply()? 2011-04-26 16:15 ` Mark Brown @ 2011-04-26 21:15 ` Bill Gatliff 0 siblings, 0 replies; 4+ messages in thread From: Bill Gatliff @ 2011-04-26 21:15 UTC (permalink / raw) To: Mark Brown; +Cc: linux-embedded, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-04-26 21:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <BANLkTi=dCHXEtv+=+sRaDp_Pt3zQy5o7ug@mail.gmail.com>
2011-04-26 8:33 ` Expose regulator:set_consumer_device_supply()? Mark Brown
2011-04-26 15:33 ` Bill Gatliff
2011-04-26 16:15 ` Mark Brown
2011-04-26 21:15 ` Bill Gatliff
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).