| Message ID | 200902231252.01980.david-b@pacbell.net (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Mon, Feb 23, 2009 at 12:52:01PM -0800, David Brownell wrote: > Use those methods to force machine-level constraints into bounds. > (Example: regulator supports 1.8V, 2.4V, 2.6V, 3.3V, and board > constraints for that rail are 2.0V to 3.6V ... so the range of > voltages is then 2.4V to 3.3V on this board.) Being able to support this is definitely a win, thanks for looking at this. > + /* maybe force machine-wide voltage constraints to match the > + * voltages supported by this regulator. use the regulator's > + * entire range for boards with no particular constraints. > + */ I'd really rather the second bit weren't here. I'd like to see a warning for the first part. > + * @count_voltages: Return the number of supported voltage indices which > + * may be passed to @list_voltage(). Some indices may correspond to > + * voltages that are not usable on this system. > + * @list_voltage: Return one of the supported voltages, in microvolts; > + * or negative errno. Indices range from zero to one less than > + * @count_voltages(). Voltages may be reported in any order. I'm having a hard time loving this interface for the driver. It feels fairly cumbersome to have to provide two functions to look up what I'd expect to be static data - I'd be fairly surprised to see it change at runtime. I think I'd expect to see something more like the way ALSA represents dB scales where the driver supplies a list of ranges that can either be simple values or value ranges expressed as (start, step, count). That would cover both complicated cases like the TWL4030 and the other common case with large regular ranges of voltages. This mostly applies to the driver interface - on the consumer side it feels a bit cumbersome to use but I can't think of anything that's particularly better. An interface that also allows consumers to ask if particular ranges can be satisfied might help here - it'd be nice for things like MMC that want to check for a relatively small set of voltages or voltage ranges (which I'd expect is the common case). -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 24 February 2009, Mark Brown wrote: > On Mon, Feb 23, 2009 at 12:52:01PM -0800, David Brownell wrote: > > > Use those methods to force machine-level constraints into bounds. > > (Example: regulator supports 1.8V, 2.4V, 2.6V, 3.3V, and board > > constraints for that rail are 2.0V to 3.6V ... so the range of > > voltages is then 2.4V to 3.3V on this board.) > > Being able to support this is definitely a win, thanks for looking at > this. Likewise consumers being able to see what voltages are available... > > + /* maybe force machine-wide voltage constraints to match the > > + * voltages supported by this regulator. use the regulator's > > + * entire range for boards with no particular constraints. > > + */ > > I'd really rather the second bit weren't here. I'd like to see a > warning for the first part. Fixable in an update. I still think it's better to require less manual configuration, not more ... manual configuration is by definition error prone; requiring more manual configuration makes systems be more fragile. Which is why I wouldn't naturally want to see a warning: it implies manual configuration is desirable. > > + * @count_voltages: Return the number of supported voltage indices which > > + * may be passed to @list_voltage(). Some indices may correspond to > > + * voltages that are not usable on this system. > > + * @list_voltage: Return one of the supported voltages, in microvolts; > > + * or negative errno. Indices range from zero to one less than > > + * @count_voltages(). Voltages may be reported in any order. > > I'm having a hard time loving this interface for the driver. It feels > fairly cumbersome to have to provide two functions to look up what I'd > expect to be static data - I'd be fairly surprised to see it change at > runtime. It can be a function of configuration, and I didn't want to force such seldom-used data into wasteful standardized-format tables. Notice that with the twl4030 code, a functional interface takes less space ... and is more flexible, since it copes with the voltage options that are defined as "not supported". Function accessors can use static const data when that's appropriate. But going the other way around isn't very straightforward... Consider also the TPS6235x regulators: the voltages they support are a simple linear function of an index 0..63, and that driver handles seven such chips. 64 values * 4 bytes * 7 chips == about 2KB of table data ... vs a few dozen bytes of function code. Again, the tradeoff looks to me like a clear win: this functional interface is small, simple, clear, flexible. > I think I'd expect to see something more like the way ALSA > represents dB scales where the driver supplies a list of ranges that can > either be simple values or value ranges expressed as (start, step, > count). That would cover both complicated cases like the TWL4030 and > the other common case with large regular ranges of voltages. Another virtue of functional interfaces for enumeration is they avoid the need for callers to see and handle a variety of different models, like that!! Do you really think it's easier to have to write code like if (regulator uses table model) { table iteration code { check(get(i)) } else if (regulator uses start/step/count model) { start/step iteration { check(something) } else if ... another model-du-jour ... { ... } else error Instead of a one simple flexible model? limit = get_limit(); for (i = 0; i < limit; i++) check(get(i)); > This mostly applies to the driver interface - on the consumer side it > feels a bit cumbersome to use but I can't think of anything that's > particularly better. There's a LONG history of functional iterator models, such as the one I used. I think they are clearly better, and don't understand why you'd prefer that more cumbersome approach. > An interface that also allows consumers to ask if > particular ranges can be satisfied might help here - it'd be nice for > things like MMC that want to check for a relatively small set of > voltages or voltage ranges (which I'd expect is the common case). Umm, did you look at the MMC patch I sent? It shows how the $SUBJECT interface fits nicely with what the MMC framework needs. mmc_regulator_get_ocrmask() took just thirty (ARM) instructions. MMC doesn't want to "check" like that. It wants to get a mask of supported voltages, then AND it with the mask reported by the MMC/SD/SDIO/eMMC/CE-ATA/... device to pick a voltage supported by both host and card. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 24, 2009 at 04:17:22PM -0800, David Brownell wrote: > On Tuesday 24 February 2009, Mark Brown wrote: > > > + /* maybe force machine-wide voltage constraints to match the > > > + * voltages supported by this regulator. use the regulator's > > > + * entire range for boards with no particular constraints. > > > + */ > > I'd really rather the second bit weren't here. I'd like to see a > > warning for the first part. > Fixable in an update. I still think it's better to require less > manual configuration, not more ... manual configuration is by > definition error prone; requiring more manual configuration makes This whole interface is structured around the idea that the consequences of getting this wrong include things like lasting hardware damage. This hardware damage may not be immediately obvious but may develop over time if components are kept running out of spec, either way it's not likely to make users happy. > systems be more fragile. Which is why I wouldn't naturally want > to see a warning: it implies manual configuration is desirable. If we weren't so reliant on the machine driver getting things right I'd agree with you. My thought here is that because there is room for human error here it'd be good to use the information we do have to flag potential problems to people, helping catch any mistakes that they make. > > I'm having a hard time loving this interface for the driver. It feels > > fairly cumbersome to have to provide two functions to look up what I'd > > expect to be static data - I'd be fairly surprised to see it change at > > runtime. > It can be a function of configuration, and I didn't want to force > such seldom-used data into wasteful standardized-format tables. > Notice that with the twl4030 code, a functional interface takes > less space ... and is more flexible, since it copes with the > voltage options that are defined as "not supported". Yeah, TWL4030 is pretty special here :/ . The only gotcha I can see is having to have a second table with only supported values in it for the case where you're not allowing the out of spec values but TBH I don't see that as a big deal. > Consider also the TPS6235x regulators: the voltages they support are > a simple linear function of an index 0..63, and that driver handles > seven such chips. 64 values * 4 bytes * 7 chips == about 2KB of > table data ... vs a few dozen bytes of function code. That's actually the most common case in the regulators I'd seen and is why I'd suggested doing something like the ALSA dB scales which handle this nicely - you don't need to actually have a table with all the values, you just provide parmeters that expresses each sub range. You say things like DECLARE_TLV_DB_SCALE(mix_tlv, -1500, 300, 0); As far as hardware requirements go I've seen regulators which provide: - A set of irregularly distributed values (usually fairly small). - A range of regularly distributed values. - A large range of values with several regular ranges in it (usually you get higher resolution at the low end). Either way can be made to work for all of these, the concerns I have are that the fact that it's a function based interface makes it look like this might be dynamic data and that it's exposing a bit too much of the implementation details (see below) which made that suggestion seem even stronger. [ALSA dB scale style] > Another virtue of functional interfaces for enumeration is > they avoid the need for callers to see and handle a variety > of different models, like that!! I'd expect the core to deal with unrolling the data rather than the consumers, this is why... > > This mostly applies to the driver interface - on the consumer side it > > feels a bit cumbersome to use but I can't think of anything that's > > particularly better. > There's a LONG history of functional iterator models, such as > the one I used. I think they are clearly better, and don't > understand why you'd prefer that more cumbersome approach. ...I am, as I say, reasonably OK with it on the consumer side. The only issue I have with it on the consumer side is that the invalid slots in the array are visible to users since it feels icky to have error conditions be part of the normal iteration process for what should be well known constant data. I worry that it's going to catch people out since relatively few regulator drivers do that (the fact that it's there is an implementation detail for drivers which have holes in the range of register values they can set). Thinking about it that could be hidden by mapping the invalid values to zero or some value that is actually valid instead of returning an error - not entirely nice but it keeps the pain away from the consumers. > > An interface that also allows consumers to ask if > > particular ranges can be satisfied might help here - it'd be nice for > > things like MMC that want to check for a relatively small set of > > voltages or voltage ranges (which I'd expect is the common case). > Umm, did you look at the MMC patch I sent? It shows > how the $SUBJECT interface fits nicely with what the > MMC framework needs. mmc_regulator_get_ocrmask() took > just thirty (ARM) instructions. > MMC doesn't want to "check" like that. It wants to get > a mask of supported voltages, then AND it with the mask > reported by the MMC/SD/SDIO/eMMC/CE-ATA/... device to > pick a voltage supported by both host and card. You can either write the loop the way you have by iterating over the voltages offered or you can write it by asking for voltage ranges that the device might want. It seems like it'd be useful for driver authors if the core allowed either method so they can use whichever idiom fits more comfortably with their needs. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 25 February 2009, Mark Brown wrote: > On Tue, Feb 24, 2009 at 04:17:22PM -0800, David Brownell wrote: > > > Fixable in an update. I still think it's better to require less > > manual configuration, not more ... manual configuration is by > > definition error prone; requiring more manual configuration makes > > This whole interface is structured around the idea that the consequences > of getting this wrong include things like lasting hardware damage. This > hardware damage may not be immediately obvious but may develop over time > if components are kept running out of spec, either way it's not likely > to make users happy. And as I noted: *hardware* designers don't like to make such goofage possible. So that's not a common scenario. I'll update the patch to be noisier about such situations, since you insist. And since braindamaged hardware designers have actually been observed in the wild. ;) > > > It feels > > > fairly cumbersome to have to provide two functions to look up what I'd > > > expect to be static data - I'd be fairly surprised to see it change at > > > runtime. > > > It can be a function of configuration, and I didn't want to force > > such seldom-used data into wasteful standardized-format tables. > > Notice that with the twl4030 code, a functional interface takes > > less space ... and is more flexible, since it copes with the > > voltage options that are defined as "not supported". > > Yeah, TWL4030 is pretty special here :/ . Those "not supported" cases seem to be mostly fallout from using four bit voltage selectors, and wanting future family members to be able to add new "supported" cases. A slightly more typical model would be to have those bit values be "undefined". Part of the space savings comes of course from tables being able to use milliVolts, not microVolts ... two bytes for each entry instead of four. :) > > Consider also the TPS6235x regulators: the voltages they support are > > a simple linear function of an index 0..63, and that driver handles > > seven such chips. 64 values * 4 bytes * 7 chips == about 2KB of > > table data ... vs a few dozen bytes of function code. > > That's actually the most common case in the regulators I'd seen Not for me. I had seen two and three bit voltage selector fields, defining fairly irregular sets of voltages. I think the rationale has to do with getting better system-wide efficiency, to stretch battery life. Circuits generating the reference voltages can be more efficient if they don't need to be continually adjustable over some range(s). > As far as hardware requirements go I've seen regulators which provide: > > - A set of irregularly distributed values (usually fairly small). > - A range of regularly distributed values. > - A large range of values with several regular ranges in it (usually > you get higher resolution at the low end). All of which model nicely as a mapping { selector --> voltage }. Hardware probably even has a register bitfield holding selector values. Maybe in that third case there's a second bitfield to hold selector bits which specify the range. > Either way can be made to work for all of these, the concerns I have are > that the fact that it's a function based interface makes it look like > this might be dynamic data and that it's exposing a bit too much of the > implementation details (see below) which made that suggestion seem even > stronger. That still doesn't make sense to me. It doesn't say a thing about what it *is* ... just how to find the voltage associated with a given index/selector. > [ALSA dB scale style] > > Another virtue of functional interfaces for enumeration is > > they avoid the need for callers to see and handle a variety > > of different models, like that!! > > I'd expect the core to deal with unrolling the data rather than the > consumers, this is why... I don't see why the core should "unroll" anything at all! The regulator driver is already doing that for get_voltage: get_voltage() { read selector from hardware map selector to voltage return that voltage } So it's trivial for similar code to take the selector as a function parameter, and do the same thing. Repackage the existing code a bit; bzzt, done! > > > This mostly applies to the driver interface - on the consumer side it > > > feels a bit cumbersome to use but I can't think of anything that's > > > particularly better. > > > There's a LONG history of functional iterator models, such as > > the one I used. I think they are clearly better, and don't > > understand why you'd prefer that more cumbersome approach. > > ...I am, as I say, reasonably OK with it on the consumer side. > > The only issue I have with it on the consumer side is that the invalid > slots in the array are visible to users since it feels icky to have > error conditions be part of the normal iteration process for what should > be well known constant data. They are "fault" conditions (like "page fault"), not errors. ;) > I worry that it's going to catch people > out since relatively few regulator drivers do that (the fact that it's > there is an implementation detail for drivers which have holes in the > range of register values they can set). It will be fairly common for the regulator to support voltages that are disallowed by the machine constraints, though. That can produce "holes" too; and not necessarily only for the lowest or highest selector codes. IMO it's reasonable to expect users of a programming interface to handle common cases like that. :) > Thinking about it that could be hidden by mapping the invalid values to > zero or some value that is actually valid instead of returning an error > - not entirely nice but it keeps the pain away from the consumers. The test for an invalid voltage is "v <= 0" regardless. I'll switch the return code for those out-of-range cases to use zero, and update the spec for regulator_list_voltage() to include that third outcome. > > MMC doesn't want to "check" like that. It wants to get > > a mask of supported voltages, then AND it with the mask > > reported by the MMC/SD/SDIO/eMMC/CE-ATA/... device to > > pick a voltage supported by both host and card. > > You can either write the loop the way you have by iterating over the > voltages offered or you can write it by asking for voltage ranges that > the device might want. The MMC stack is written to work the way I described. I don't see that model changing any time soon; most MMC adapters don't seem to have a switchable Vdd rail, so they've got a fixed mask reporting 3.2-3.4V support and won't have any reason to use (or depend on) the regulator framework. True, *other* stacks might want something else: > It seems like it'd be useful for driver authors > if the core allowed either method so they can use whichever idiom fits > more comfortably with their needs. A patch could be added later, when some system needs some other model. I'm not exactly sure what would be returned by "asking for a voltage range". That sounds like it might be a different regulator capability model than the "discrete voltage options" one. My focus this time around was on exposing the regulator capabilities to the core to support two simple use cases: (a) validating machine constraints, (b) supporting MMC/SD usage idiom, so the omap_hsmmc driver can fully decouple from the twl4030-family regulators. I'll send an updated version of this patch in a bit. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 25, 2009 at 02:12:26PM -0800, David Brownell wrote: > On Wednesday 25 February 2009, Mark Brown wrote: > > This whole interface is structured around the idea that the consequences > > of getting this wrong include things like lasting hardware damage. This > > hardware damage may not be immediately obvious but may develop over time > > if components are kept running out of spec, either way it's not likely > > to make users happy. > And as I noted: *hardware* designers don't like to make > such goofage possible. So that's not a common scenario. That's often not possible with software controllable regulators - it's usually hard to stop them being set to any value they support and the desire for any additional protection needs to be traded off against power consumption and space and BoM costs. > Not for me. I had seen two and three bit voltage selector fields, > defining fairly irregular sets of voltages. > I think the rationale has to do with getting better system-wide > efficiency, to stretch battery life. Circuits generating the > reference voltages can be more efficient if they don't need to > be continually adjustable over some range(s). It's certainly not the common case for regulators that people are contributing to the kernel (most of which seem to be intended to be primary PMICs). Make of that what you will :) > > As far as hardware requirements go I've seen regulators which provide: > > - A set of irregularly distributed values (usually fairly small). > > - A range of regularly distributed values. > > - A large range of values with several regular ranges in it (usually > > you get higher resolution at the low end). > All of which model nicely as a mapping { selector --> voltage }. > Hardware probably even has a register bitfield holding selector > values. Maybe in that third case there's a second bitfield to > hold selector bits which specify the range. Yes, you can clearly always do selector->voltage since there's going to be a finite number of register bits that it'll be possible to set. > > Either way can be made to work for all of these, the concerns I have are > > that the fact that it's a function based interface makes it look like > > this might be dynamic data and that it's exposing a bit too much of the > > implementation details (see below) which made that suggestion seem even > > stronger. > That still doesn't make sense to me. It doesn't say a thing > about what it *is* ... just how to find the voltage associated > with a given index/selector. A function that return errors suggests something non-static to me. > > I'd expect the core to deal with unrolling the data rather than the > > consumers, this is why... > I don't see why the core should "unroll" anything at all! > The regulator driver is already doing that for get_voltage: > get_voltage() { > read selector from hardware > map selector to voltage > return that voltage > } > So it's trivial for similar code to take the selector as > a function parameter, and do the same thing. Repackage > the existing code a bit; bzzt, done! Yes, that's a reasonable point (though I'd still like to see the maximum turn into a static value now I think about it). > > I worry that it's going to catch people > > out since relatively few regulator drivers do that (the fact that it's > > there is an implementation detail for drivers which have holes in the > > range of register values they can set). > It will be fairly common for the regulator to support voltages > that are disallowed by the machine constraints, though. That > can produce "holes" too; and not necessarily only for the lowest > or highest selector codes. At present only continous ranges are possible, though. I can't think of any systems I've seen that'd want discontinous constraints, though I'm sure there are some. > > Thinking about it that could be hidden by mapping the invalid values to > > zero or some value that is actually valid instead of returning an error > > - not entirely nice but it keeps the pain away from the consumers. > The test for an invalid voltage is "v <= 0" regardless. If you're looking for a bound you'd just check for things within that bound anyway. It's if there's explicit "this is an error" return that people start wanting to do something with it rather than silently ignore it (we hope, anyway). > > You can either write the loop the way you have by iterating over the > > voltages offered or you can write it by asking for voltage ranges that > > the device might want. > The MMC stack is written to work the way I described. ... > True, *other* stacks might want something else: Indeed, I'm not talking about MMC in particular here - other things are going to want to ask the same questions. > > It seems like it'd be useful for driver authors > > if the core allowed either method so they can use whichever idiom fits > > more comfortably with their needs. > A patch could be added later, when some system needs > some other model. I'm not exactly sure what would be > returned by "asking for a voltage range". That sounds > like it might be a different regulator capability model > than the "discrete voltage options" one. Yes, I'm suggesting something like: int regulator_available_voltage(min, max); It's wrapping the discrete values for consumers that find that idiom easier. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 25 February 2009, Mark Brown wrote: > > Â Â Â Â Â Â get_voltage() { > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â read selector from hardware > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â map selector to voltage > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â return that voltage > > Â Â Â Â Â Â } > > > So it's trivial for similar code to take the selector as > > a function parameter, and do the same thing. Â Repackage > > the existing code a bit; bzzt, done! > > Yes, that's a reasonable point (though I'd still like to see the maximum > turn into a static value now I think about it). At the regulator_desc level, that's trivial; I'll do that in the patch you'll see. In terms of the consumer interface, not -- "struct regulator" is opaque to consumers, and everything is a functional accessor. So I'll leave that as-is. > > It will be fairly common for the regulator to support voltages > > that are disallowed by the machine constraints, though. Â That > > can produce "holes" too; and not necessarily only for the lowest > > or highest selector codes. > > At present only continous ranges are possible, though. Â I can't think of > any systems I've seen that'd want discontinous constraints, though I'm > sure there are some. Consider a regulator where voltage selectors 0..3 correspond to voltages { 3.3V, 1.8V, 4.2V, 5.0V } With machine constraints that say voltages go from 3V to 4.5V ... - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 25 February 2009, Mark Brown wrote: > > Fixable in an update. Â I still think it's better to require less > > manual configuration, not more ... manual configuration is by > > definition error prone; requiring more manual configuration makes > > This whole interface is structured around the idea that the consequences > of getting this wrong include things like lasting hardware damage. Oh, one more comment. Requiring manual configuration of fixed-voltage regulators is pure time-wastage. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 25, 2009 at 05:02:03PM -0800, David Brownell wrote: > Oh, one more comment. Requiring manual configuration > of fixed-voltage regulators is pure time-wastage. Unless, of course, you happen to be using one of those consumers that wants to know the voltage it's running at to configure itself. Examples I've seen include sensors and analogue components which can be configured for better performance if they know the voltage they run at - and yes, people do run these things off regulators that they vary at runtime. If you want to submit a patch making it optional that's fine. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 25, 2009 at 03:47:52PM -0800, David Brownell wrote: > On Wednesday 25 February 2009, Mark Brown wrote: > In terms of the consumer interface, not -- "struct regulator" > is opaque to consumers, and everything is a functional accessor. > So I'll leave that as-is. Yes, obviously. > > At present only continous ranges are possible, though. ?I can't think of > > any systems I've seen that'd want discontinous constraints, though I'm > > sure there are some. > Consider a regulator where voltage selectors 0..3 correspond to > voltages > { 3.3V, 1.8V, 4.2V, 5.0V } > With machine constraints that say voltages go from 3V to 4.5V ... Continuous ranges of voltages, not continous indexes. The indexes are meaningless. At the minute consumers and machines always supply constraints as min,max pairs. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 26 February 2009, Mark Brown wrote: > On Wed, Feb 25, 2009 at 05:02:03PM -0800, David Brownell wrote: > > > Oh, one more comment. Requiring manual configuration > > of fixed-voltage regulators is pure time-wastage. > > Unless, of course, you happen to be using one of those consumers that > wants to know the voltage it's running at to configure itself. In which case it can ask the regulator what voltage it's using. :) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 26, 2009 at 10:56:05AM -0800, David Brownell wrote: > On Thursday 26 February 2009, Mark Brown wrote: > > Unless, of course, you happen to be using one of those consumers that > > wants to know the voltage it's running at to configure itself. > In which case it can ask the regulator what voltage it's using. :) I suspect we're talking at cross purposes here - I was talking about the fixed voltage regulator driver. I suspect you were talking about something else? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 26 February 2009, Mark Brown wrote: > On Thu, Feb 26, 2009 at 10:56:05AM -0800, David Brownell wrote: > > On Thursday 26 February 2009, Mark Brown wrote: > > > > Unless, of course, you happen to be using one of those consumers that > > > wants to know the voltage it's running at to configure itself. > > > In which case it can ask the regulator what voltage it's using. :) > > I suspect we're talking at cross purposes here - I was talking about the > fixed voltage regulator driver. I suspect you were talking about > something else? Yes ... e.g. the USB1V5, USB1V, and USB3V1 regulators exported through the twl4030 regulator driver. Semi-related: someone with time to spend on it might find and fix the bug causing the regulator framework to oops when regulator/core.c::set_machine_constraints() returns an error code. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-02-26 at 11:38 -0800, David Brownell wrote: > > Semi-related: someone with time to spend on it might > find and fix the bug causing the regulator framework > to oops when regulator/core.c::set_machine_constraints() > returns an error code. Any chance you can post the oops. Is it happening within the set_constraints() error path or sometime later on ? Thanks Liam -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 26 February 2009, Liam Girdwood wrote: > > > > Semi-related: Â someone with time to spend on it might > > find and fix the bug causing the regulator framework > > to oops when regulator/core.c::set_machine_constraints() > > returns an error code. > > Any chance you can post the oops. Is it happening within the > set_constraints() error path or sometime later on ? Â Later, after it returns. This particular oops went away with the tweak eliminating the need for pointless init of constraints for fixed voltage (1.8V in this case) regulators. - Dave <3>set_machine_constraints: invalid 'VUSB1V8' voltage constraints <1>Unable to handle kernel NULL pointer dereference at virtual address 00000004 <1>pgd = c0004000 <1>[00000004] *pgd=00000000 Internal error: Oops: 805 [#1] PREEMPT CPU: 0 Not tainted (2.6.29-rc6-omap1 #517) PC is at kobj_kset_leave+0x34/0x5c LR is at _spin_lock+0x50/0x58 pc : [<c01748a0>] lr : [<c0336e84>] psr: 20000013 sp : c7821978 ip : c7821950 fp : c782198c r10: c785f8a8 r9 : c7888a08 r8 : c78dade0 r7 : ffffffea r6 : 00000000 r5 : c785f8a8 r4 : c785f908 r3 : 00000000 r2 : c7888a6c r1 : c785f90c r0 : c7815308 Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel Control: 10c5387f Table: 80004019 DAC: 00000017 Process swapper (pid: 1, stack limit = 0xc78202e8) Stack: (0xc7821978 to 0xc7822000) 1960: c785f908 c785f8a8 1980: ... deletia ... 1fe0: 00000000 00000000 00000000 c7821ff8 c004d6c8 c0008668 8b77de58 01fbdd0a Backtrace: [<c017486c>] (kobj_kset_leave+0x0/0x5c) from [<c01748f4>] (kobject_del+0x2c/0x40) r5:c785f8a8 r4:c785f908 [<c01748c8>] (kobject_del+0x0/0x40) from [<c01a94fc>] (device_del+0x158/0x170) r5:c785f8a8 r4:c785f908 [<c01a93a4>] (device_del+0x0/0x170) from [<c01a9528>] (device_unregister+0x14/0x20) r7:ffffffea r6:c785f808 r5:c0437150 r4:c785f8a8 [<c01a9514>] (device_unregister+0x0/0x20) from [<c0188614>] (regulator_register+0x208/0x248) r5:c0437150 r4:c785f800 [<c018840c>] (regulator_register+0x0/0x248) from [<c0189894>] (twl4030reg_probe+0xa4/0x108) [<c01897f0>] (twl4030reg_probe+0x0/0x108) from [<c01ac778>] (platform_drv_probe+0x20/0x24) r6:c04371b0 r5:c7888a08 r4:c04371b0 [<c01ac758>] (platform_drv_probe+0x0/0x24) from [<c01ab920>] (really_probe+0x9c/0x148) [<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58) r7:c7888a68 r6:c7888a08 r5:c7888a08 r4:c04371b0 [<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01abac0>] (__device_attach+0x10/0x14) r5:c01abab0 r4:00000000 [<c01abab0>] (__device_attach+0x0/0x14) from [<c01aae9c>] (bus_for_each_drv+0x50/0x84) [<c01aae4c>] (bus_for_each_drv+0x0/0x84) from [<c01abb48>] (device_attach+0x58/0x70) r6:00000000 r5:c7888aac r4:c7888a08 [<c01abaf0>] (device_attach+0x0/0x70) from [<c01aacb8>] (bus_attach_device+0x30/0x64) r5:c7888a08 r4:c043b3f8 [<c01aac88>] (bus_attach_device+0x0/0x64) from [<c01a9830>] (device_add+0x148/0x284) r5:c7886820 r4:c7888a08 [<c01a96e8>] (device_add+0x0/0x284) from [<c01ace24>] (platform_device_add+0xf8/0x150) r7:c7888a8c r6:c7888a00 r5:00000000 r4:00000000 [<c01acd2c>] (platform_device_add+0x0/0x150) from [<c01b0714>] (add_numbered_child+0xe4/0x130) r7:00000120 r6:c7821bc0 r5:c7888a00 r4:00000000 [<c01b0630>] (add_numbered_child+0x0/0x130) from [<c01b07b0>] (add_regulator_linked+0x50/0x5c) [<c01b0760>] (add_regulator_linked+0x0/0x5c) from [<c01b09f8>] (add_children+0x220/0x2d4) [<c01b07d8>] (add_children+0x0/0x2d4) from [<c01b0eac>] (twl4030_probe+0x148/0x174) r8:c08010f4 r7:c041e848 r6:c7886200 r5:00000004 r4:00000178 [<c01b0d64>] (twl4030_probe+0x0/0x174) from [<c01cf6dc>] (i2c_device_probe+0x70/0x88) [<c01cf66c>] (i2c_device_probe+0x0/0x88) from [<c01ab920>] (really_probe+0x9c/0x148) r7:c7886280 r6:c043b6f8 r5:c7886220 r4:c043b6f8 [<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58) r7:c7886280 r6:c7886220 r5:c7886220 r4:c043b6f8 [<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01abac0>] (__device_attach+0x10/0x14) r5:c01abab0 r4:00000000 [<c01abab0>] (__device_attach+0x0/0x14) from [<c01aae9c>] (bus_for_each_drv+0x50/0x84) [<c01aae4c>] (bus_for_each_drv+0x0/0x84) from [<c01abb48>] (device_attach+0x58/0x70) r6:00000000 r5:c78862c4 r4:c7886220 [<c01abaf0>] (device_attach+0x0/0x70) from [<c01aacb8>] (bus_attach_device+0x30/0x64) r5:c7886220 r4:c043d558 [<c01aac88>] (bus_attach_device+0x0/0x64) from [<c01a9830>] (device_add+0x148/0x284) r5:c785ecf8 r4:c7886220 [<c01a96e8>] (device_add+0x0/0x284) from [<c01a9988>] (device_register+0x1c/0x20) r7:00000000 r6:c785ec50 r5:c7886200 r4:c7886220 [<c01a996c>] (device_register+0x0/0x20) from [<c01d069c>] (i2c_attach_client+0xc0/0x150) r5:c7886200 r4:c7886220 [<c01d05dc>] (i2c_attach_client+0x0/0x150) from [<c01d0d48>] (i2c_new_device+0x64/0x84) r7:c0420c90 r6:c785ec50 r5:c78064cc r4:c7886200 [<c01d0ce4>] (i2c_new_device+0x0/0x84) from [<c01d1388>] (i2c_scan_static_board_info+0x44/0x8c) r7:c0420c90 r6:00000000 r5:c785ec50 r4:c78064c0 [<c01d1344>] (i2c_scan_static_board_info+0x0/0x8c) from [<c01d145c>] (i2c_register_adapter+0x8c/0x144) r5:c785ec50 r4:00000000 [<c01d13d0>] (i2c_register_adapter+0x0/0x144) from [<c01d15b4>] (i2c_add_numbered_adapter+0xa0/0xb8) r5:c785ec50 r4:00000000 [<c01d1514>] (i2c_add_numbered_adapter+0x0/0xb8) from [<c0018c60>] (omap_i2c_probe+0x254/0x304) r5:c0420858 r4:c785ec00 [<c0018a0c>] (omap_i2c_probe+0x0/0x304) from [<c01ac778>] (platform_drv_probe+0x20/0x24) r8:00000001 r7:c043b3f8 r6:c043d7c8 r5:c0420860 r4:c043d7c8 [<c01ac758>] (platform_drv_probe+0x0/0x24) from [<c01ab920>] (really_probe+0x9c/0x148) [<c01ab884>] (really_probe+0x0/0x148) from [<c01aba20>] (driver_probe_device+0x54/0x58) r7:c043b3f8 r6:c043d7c8 r5:c0420860 r4:c043d7c8 [<c01ab9cc>] (driver_probe_device+0x0/0x58) from [<c01aba8c>] (__driver_attach+0x68/0x8c) r5:c0420904 r4:c0420860 [<c01aba24>] (__driver_attach+0x0/0x8c) from [<c01ab170>] (bus_for_each_dev+0x4c/0x80) r7:c043b3f8 r6:c043d7c8 r5:c01aba24 r4:00000000 [<c01ab124>] (bus_for_each_dev+0x0/0x80) from [<c01ab79c>] (driver_attach+0x20/0x28) r6:c7887300 r5:00000000 r4:c043d7c8 [<c01ab77c>] (driver_attach+0x0/0x28) from [<c01aaaec>] (bus_add_driver+0xa4/0x170) [<c01aaa48>] (bus_add_driver+0x0/0x170) from [<c01abd40>] (driver_register+0x98/0xcc) r7:c0018970 r6:00000000 r5:00000000 r4:c043d7c8 [<c01abca8>] (driver_register+0x0/0xcc) from [<c01accb0>] (platform_driver_register+0x6c/0x88) r5:00000000 r4:c0023fd8 [<c01acc44>] (platform_driver_register+0x0/0x88) from [<c0018984>] (omap_i2c_init_driver+0x14/0x1c) [<c0018970>] (omap_i2c_init_driver+0x0/0x1c) from [<c002923c>] (__exception_text_end+0x5c/0x1a0) [<c00291e0>] (__exception_text_end+0x0/0x1a0) from [<c000861c>] (do_initcalls+0x1c/0x38) r8:00000000 r7:00000000 r6:00000000 r5:00000000 r4:c0023fd8 [<c0008600>] (do_initcalls+0x0/0x38) from [<c0008658>] (do_basic_setup+0x20/0x24) r5:00000000 r4:c044b640 [<c0008638>] (do_basic_setup+0x0/0x24) from [<c00086b0>] (kernel_init+0x54/0xa8) [<c000865c>] (kernel_init+0x0/0xa8) from [<c004d6c8>] (do_exit+0x0/0x254) r4:00000000 Code: e5942008 e5943004 e2841004 e5823000 (e5832004) <4>---[ end trace 1b75b31a2719ed1c ]--- <6>note: swapper[1] exited with preempt_count 2 <0>Kernel panic - not syncing: Attempted to kill init! -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -719,6 +719,44 @@ static int set_machine_constraints(struc else name = "regulator"; + /* maybe force machine-wide voltage constraints to match the + * voltages supported by this regulator. use the regulator's + * entire range for boards with no particular constraints. + */ + if (ops->list_voltage) { + int count = ops->count_voltages(rdev); + int i; + int min_uV = INT_MAX; + int max_uV = INT_MIN; + int cmin = constraints->min_uV ? : INT_MIN; + int cmax = constraints->max_uV ? : INT_MAX; + + /* initial: [cmin..cmax] valid, [min_uV..max_uV] not */ + for (i = 0; i < count; i++) { + int value; + + value = ops->list_voltage(rdev, i); + if (value <= 0) + continue; + + /* maybe adjust [min_uV..max_uV] */ + if (value >= cmin && value < min_uV) + min_uV = value; + if (value <= cmax && value > max_uV) + max_uV = value; + } + + /* final: [min_uV..max_uV] valid iff constraints valid */ + if (max_uV < min_uV) { + pr_err("%s: bad '%s' voltage constraints\n", + __func__, name); + ret = -EINVAL; + goto out; + } + constraints->min_uV = min_uV; + constraints->max_uV = max_uV; + } + rdev->constraints = constraints; /* do we need to apply the constraint voltage */ @@ -1245,6 +1283,75 @@ int regulator_is_enabled(struct regulato EXPORT_SYMBOL_GPL(regulator_is_enabled); /** + * regulator_count_voltages - count regulator_list_voltage() indices + * @regulator: regulator source + * + * Returns number of indices, or negative errno. + */ +int regulator_count_voltages(struct regulator *regulator) +{ + struct regulator_dev *rdev = regulator->rdev; + struct regulator_ops *ops; + int ret = -EINVAL; + + mutex_lock(&rdev->mutex); + + ops = rdev->desc->ops; + if (ops->count_voltages) + ret = ops->count_voltages(rdev); + + mutex_unlock(&rdev->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(regulator_count_voltages); + +/** + * regulator_list_voltage - enumerate supported voltages + * @regulator: regulator source + * @index: identify voltage to list + * + * Returns a voltage that can be passed to @regulator_set_voltage(), + * or negative fault code. + * + * Faults include passing in invalid index, and using an index + * corresponding to a voltage that can't be used on this system. + */ +int regulator_list_voltage(struct regulator *regulator, unsigned index) +{ + struct regulator_dev *rdev = regulator->rdev; + struct regulator_ops *ops; + int ret = -EINVAL; + + mutex_lock(&rdev->mutex); + + ops = rdev->desc->ops; + if (ops->count_voltages && ops->list_voltage) + ret = ops->count_voltages(rdev); + + if (ret == 0) + ret = -EIO; + else if (ret > 0) { + if (index >= ret) + ret = -EDOM; + else + ret = ops->list_voltage(rdev, index); + } + + if (ret >= 0) { + if (ret < rdev->constraints->min_uV) + ret = -ERANGE; + else if (ret > rdev->constraints->max_uV) + ret = -ERANGE; + } + + mutex_unlock(&rdev->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(regulator_list_voltage); + +/** * regulator_set_voltage - set regulator output voltage * @regulator: regulator source * @min_uV: Minimum required voltage in uV --- a/include/linux/regulator/consumer.h +++ b/include/linux/regulator/consumer.h @@ -140,6 +140,8 @@ int regulator_bulk_disable(int num_consu void regulator_bulk_free(int num_consumers, struct regulator_bulk_data *consumers); +int regulator_count_voltages(struct regulator *regulator); +int regulator_list_voltage(struct regulator *regulator, unsigned index); int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV); int regulator_get_voltage(struct regulator *regulator); int regulator_set_current_limit(struct regulator *regulator, --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -40,6 +40,12 @@ enum regulator_status { * @set_voltage: Set the voltage for the regulator within the range specified. * The driver should select the voltage closest to min_uV. * @get_voltage: Return the currently configured voltage for the regulator. + * @count_voltages: Return the number of supported voltage indices which + * may be passed to @list_voltage(). Some indices may correspond to + * voltages that are not usable on this system. + * @list_voltage: Return one of the supported voltages, in microvolts; + * or negative errno. Indices range from zero to one less than + * @count_voltages(). Voltages may be reported in any order. * @set_current_limit: Configure a limit for a current-limited regulator. * @get_current_limit: Get the configured limit for a current-limited regulator. * @set_mode: Set the operating mode for the regulator. @@ -62,6 +68,10 @@ enum regulator_status { */ struct regulator_ops { + /* enumerate supported voltages */ + int (*count_voltages) (struct regulator_dev *); + int (*list_voltage) (struct regulator_dev *, unsigned index); + /* get/set regulator voltage */ int (*set_voltage) (struct regulator_dev *, int min_uV, int max_uV); int (*get_voltage) (struct regulator_dev *);