Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
On Thu, Mar 23, 2017 at 02:40:16PM -0700, Matthias Kaehlcke wrote: > El Mon, Mar 20, 2017 at 12:06:15PM + Mark Brown ha dit: > > On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote: > > > You are right that my case is very specialist, however I think it is > > > a general problem that a consumer can't know whether the results of > > > _list_voltage(), etc correspond to the regulator itself or to its > > > supplies. E.g. a consumer might have a continuous reg which is > > > supplied by a discrete reg, in this case _list_voltage() would return > > > the steps of the supply reg, which is probably not what most consumers > > > expect. > > No, this is doesn't make much sense! Why should we be reporting > > properties of the parent regulator when the child regulator is > > regulating away all visibility of those properties? > I am confused whether you are confirming that the current behavior > makes no sense or if you think that what I'm saying is nonsense. I'm saying that the current behaviour isn't good and that the incoherence of what you're proposing should make this clear to you. > > If it helps think of a continuous regulator as a discrete regulator with > > a base voltage of 0 and steps of 1uV. > Thanks, I understood that. What I didn't realize initially is that we > can avoid iterating through all the voltages if the regulator has > linear steps, which we can determine with regulator_get_linear_step(). > With that in mind I don't see concerns from the vctrl perspective. OK, good. I keep meaning to look into retooling all the continuous regulators to actually be linear regulators to simplify things. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
On Thu, Mar 23, 2017 at 02:40:16PM -0700, Matthias Kaehlcke wrote: > El Mon, Mar 20, 2017 at 12:06:15PM + Mark Brown ha dit: > > On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote: > > > You are right that my case is very specialist, however I think it is > > > a general problem that a consumer can't know whether the results of > > > _list_voltage(), etc correspond to the regulator itself or to its > > > supplies. E.g. a consumer might have a continuous reg which is > > > supplied by a discrete reg, in this case _list_voltage() would return > > > the steps of the supply reg, which is probably not what most consumers > > > expect. > > No, this is doesn't make much sense! Why should we be reporting > > properties of the parent regulator when the child regulator is > > regulating away all visibility of those properties? > I am confused whether you are confirming that the current behavior > makes no sense or if you think that what I'm saying is nonsense. I'm saying that the current behaviour isn't good and that the incoherence of what you're proposing should make this clear to you. > > If it helps think of a continuous regulator as a discrete regulator with > > a base voltage of 0 and steps of 1uV. > Thanks, I understood that. What I didn't realize initially is that we > can avoid iterating through all the voltages if the regulator has > linear steps, which we can determine with regulator_get_linear_step(). > With that in mind I don't see concerns from the vctrl perspective. OK, good. I keep meaning to look into retooling all the continuous regulators to actually be linear regulators to simplify things. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
El Mon, Mar 20, 2017 at 12:06:15PM + Mark Brown ha dit: > On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote: > > > In principle I totally agree with you that consumers should be able > > to enumerate the supported voltages with the existing functions. And > > they can, as long as they already know (or assume) that the regulator > > they are using actually has discrete steps, otherwise they might get > > unexpected results. > > Given the limits of number representation continuous regulators also > have discrete steps, they just have a lot of them (but so do some > regulators we currently say aren't continuous so...). > > > You are right that my case is very specialist, however I think it is > > a general problem that a consumer can't know whether the results of > > _list_voltage(), etc correspond to the regulator itself or to its > > supplies. E.g. a consumer might have a continuous reg which is > > supplied by a discrete reg, in this case _list_voltage() would return > > the steps of the supply reg, which is probably not what most consumers > > expect. > > No, this is doesn't make much sense! Why should we be reporting > properties of the parent regulator when the child regulator is > regulating away all visibility of those properties? I am confused whether you are confirming that the current behavior makes no sense or if you think that what I'm saying is nonsense. > > > > Please see my explication above on why the vctrl driver needs to know > > > > this. > > > > I'm seeing nothing in the above that addresses my question, you don't > > > even seem to have mentioned supplies. > > > Sorry, I really didn't try to evade your question. Does it make more > > sense with the example above? > > No, not at all. I take this as an indication that you don't think my description above is correct. Let's use a real world example then, tested with actual software and hardware. Our regulator is 'ppvar_bigcpu': https://chromium.googlesource.com/chromiumos/third_party/kernel/+/release-R58-9334.B-chromeos-4.4/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi#177 For our test we change the supply to 'pp1200_lpddr' to make sure the supply has at least one voltage and this voltage is within the constraints of 'ppvar_bigcpu'. 'ppvar_bigcpu' is continuous, however _count_voltages(ppvar_bigcpu) returns 1 and _list_voltage(ppvar_bigcpu, 0) returns 120, which is precisely the configuration of 'pp1200_lpddr'. I verified this with a 4.10ish kernel (no major changes, zero changes in regulators). > > > What we should be doing for continuous regulators is allowing people to > > > list the supported voltages as they would for other regulators. > > > In the overall regulator context this may make sense, at this point I > > don't really have enough background on the subsystem to have an > > informed opinion. > > > From the vctrl perspective I wouldn't be overly happy, since it > > wouldn't allow to distinguish between continuous and discrete > > supplies, and I still think that handling discrete supplies > > differently is simpler/more efficient. This doesn't mean I argue > > against your proposal if it is deemed the right thing from a subsystem > > perspective. > > If it helps think of a continuous regulator as a discrete regulator with > a base voltage of 0 and steps of 1uV. Thanks, I understood that. What I didn't realize initially is that we can avoid iterating through all the voltages if the regulator has linear steps, which we can determine with regulator_get_linear_step(). With that in mind I don't see concerns from the vctrl perspective. Matthias
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
El Mon, Mar 20, 2017 at 12:06:15PM + Mark Brown ha dit: > On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote: > > > In principle I totally agree with you that consumers should be able > > to enumerate the supported voltages with the existing functions. And > > they can, as long as they already know (or assume) that the regulator > > they are using actually has discrete steps, otherwise they might get > > unexpected results. > > Given the limits of number representation continuous regulators also > have discrete steps, they just have a lot of them (but so do some > regulators we currently say aren't continuous so...). > > > You are right that my case is very specialist, however I think it is > > a general problem that a consumer can't know whether the results of > > _list_voltage(), etc correspond to the regulator itself or to its > > supplies. E.g. a consumer might have a continuous reg which is > > supplied by a discrete reg, in this case _list_voltage() would return > > the steps of the supply reg, which is probably not what most consumers > > expect. > > No, this is doesn't make much sense! Why should we be reporting > properties of the parent regulator when the child regulator is > regulating away all visibility of those properties? I am confused whether you are confirming that the current behavior makes no sense or if you think that what I'm saying is nonsense. > > > > Please see my explication above on why the vctrl driver needs to know > > > > this. > > > > I'm seeing nothing in the above that addresses my question, you don't > > > even seem to have mentioned supplies. > > > Sorry, I really didn't try to evade your question. Does it make more > > sense with the example above? > > No, not at all. I take this as an indication that you don't think my description above is correct. Let's use a real world example then, tested with actual software and hardware. Our regulator is 'ppvar_bigcpu': https://chromium.googlesource.com/chromiumos/third_party/kernel/+/release-R58-9334.B-chromeos-4.4/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi#177 For our test we change the supply to 'pp1200_lpddr' to make sure the supply has at least one voltage and this voltage is within the constraints of 'ppvar_bigcpu'. 'ppvar_bigcpu' is continuous, however _count_voltages(ppvar_bigcpu) returns 1 and _list_voltage(ppvar_bigcpu, 0) returns 120, which is precisely the configuration of 'pp1200_lpddr'. I verified this with a 4.10ish kernel (no major changes, zero changes in regulators). > > > What we should be doing for continuous regulators is allowing people to > > > list the supported voltages as they would for other regulators. > > > In the overall regulator context this may make sense, at this point I > > don't really have enough background on the subsystem to have an > > informed opinion. > > > From the vctrl perspective I wouldn't be overly happy, since it > > wouldn't allow to distinguish between continuous and discrete > > supplies, and I still think that handling discrete supplies > > differently is simpler/more efficient. This doesn't mean I argue > > against your proposal if it is deemed the right thing from a subsystem > > perspective. > > If it helps think of a continuous regulator as a discrete regulator with > a base voltage of 0 and steps of 1uV. Thanks, I understood that. What I didn't realize initially is that we can avoid iterating through all the voltages if the regulator has linear steps, which we can determine with regulator_get_linear_step(). With that in mind I don't see concerns from the vctrl perspective. Matthias
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote: > In principle I totally agree with you that consumers should be able > to enumerate the supported voltages with the existing functions. And > they can, as long as they already know (or assume) that the regulator > they are using actually has discrete steps, otherwise they might get > unexpected results. Given the limits of number representation continuous regulators also have discrete steps, they just have a lot of them (but so do some regulators we currently say aren't continuous so...). > You are right that my case is very specialist, however I think it is > a general problem that a consumer can't know whether the results of > _list_voltage(), etc correspond to the regulator itself or to its > supplies. E.g. a consumer might have a continuous reg which is > supplied by a discrete reg, in this case _list_voltage() would return > the steps of the supply reg, which is probably not what most consumers > expect. No, this is doesn't make much sense! Why should we be reporting properties of the parent regulator when the child regulator is regulating away all visibility of those properties? > > > Please see my explication above on why the vctrl driver needs to know > > > this. > > I'm seeing nothing in the above that addresses my question, you don't > > even seem to have mentioned supplies. > Sorry, I really didn't try to evade your question. Does it make more > sense with the example above? No, not at all. > > What we should be doing for continuous regulators is allowing people to > > list the supported voltages as they would for other regulators. > In the overall regulator context this may make sense, at this point I > don't really have enough background on the subsystem to have an > informed opinion. > From the vctrl perspective I wouldn't be overly happy, since it > wouldn't allow to distinguish between continuous and discrete > supplies, and I still think that handling discrete supplies > differently is simpler/more efficient. This doesn't mean I argue > against your proposal if it is deemed the right thing from a subsystem > perspective. If it helps think of a continuous regulator as a discrete regulator with a base voltage of 0 and steps of 1uV. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
On Fri, Mar 17, 2017 at 05:03:30PM -0700, Matthias Kaehlcke wrote: > In principle I totally agree with you that consumers should be able > to enumerate the supported voltages with the existing functions. And > they can, as long as they already know (or assume) that the regulator > they are using actually has discrete steps, otherwise they might get > unexpected results. Given the limits of number representation continuous regulators also have discrete steps, they just have a lot of them (but so do some regulators we currently say aren't continuous so...). > You are right that my case is very specialist, however I think it is > a general problem that a consumer can't know whether the results of > _list_voltage(), etc correspond to the regulator itself or to its > supplies. E.g. a consumer might have a continuous reg which is > supplied by a discrete reg, in this case _list_voltage() would return > the steps of the supply reg, which is probably not what most consumers > expect. No, this is doesn't make much sense! Why should we be reporting properties of the parent regulator when the child regulator is regulating away all visibility of those properties? > > > Please see my explication above on why the vctrl driver needs to know > > > this. > > I'm seeing nothing in the above that addresses my question, you don't > > even seem to have mentioned supplies. > Sorry, I really didn't try to evade your question. Does it make more > sense with the example above? No, not at all. > > What we should be doing for continuous regulators is allowing people to > > list the supported voltages as they would for other regulators. > In the overall regulator context this may make sense, at this point I > don't really have enough background on the subsystem to have an > informed opinion. > From the vctrl perspective I wouldn't be overly happy, since it > wouldn't allow to distinguish between continuous and discrete > supplies, and I still think that handling discrete supplies > differently is simpler/more efficient. This doesn't mean I argue > against your proposal if it is deemed the right thing from a subsystem > perspective. If it helps think of a continuous regulator as a discrete regulator with a base voltage of 0 and steps of 1uV. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
Hi Mark, El Fri, Mar 17, 2017 at 09:15:47PM + Mark Brown ha dit: > On Thu, Mar 09, 2017 at 11:40:54AM -0800, Matthias Kaehlcke wrote: > > El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit: > > > On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote: > > > > > The new function allows consumers to determine if a regulator is > > > > continuous or discrete, and whether the results of > > > > Why? As we discussed at ELC this is an implementation detail of the > > > regulator and it's to an extent a taste decision if the regulator is > > > represented as a linear range or a continuous range (in fact given > > > improvements in the core we could probably just update all continuous > > > range regulators to linear ones). > > > The second patch of this series is a driver for voltage controlled > > regulators (vctrl), ie the output voltage of a vctrl regulator is > > controlled through the voltage of another regulator. The control > > regulator can be continuous or discrete and I think it makes sense for > > the vctrl regulator to mirror its control regulator in this aspect. > > Why should it pretend to have a continuous range when it is actually > > discrete due to the control regulator? > > I don't think we should be providing a consumer facing API which invites > consumers to peer into the implementation details of regulators, or > gives them the idea that these concepts exist. Consumers can already > enumerate the set of supported voltages via _list_voltage() and so on, > I'd expect consumers to be able to get what they need from those. You > have an incrediblity specialist use case here but this is just a general > consumer interface that's being added. In principle I totally agree with you that consumers should be able to enumerate the supported voltages with the existing functions. And they can, as long as they already know (or assume) that the regulator they are using actually has discrete steps, otherwise they might get unexpected results. You are right that my case is very specialist, however I think it is a general problem that a consumer can't know whether the results of _list_voltage(), etc correspond to the regulator itself or to its supplies. E.g. a consumer might have a continuous reg which is supplied by a discrete reg, in this case _list_voltage() would return the steps of the supply reg, which is probably not what most consumers expect. > > For continuous control regulators we can simply calculate a "safe" > > voltage for the next step and pass it to the control regulator. In > > case of a discrete control regulator this calculated voltage may not > > be directly available, without knowing the available steps the vctrl > > driver has to try different voltage ranges until it finds one that is > > accepted by the control regulator. Obviously this can be done but it > > adds code complexity and runtime overhead which is not necessary if we > > know the available steps (and regulator_list_voltage() is already > > there to provide them). > > I'm not seeing how knowing if a regulator is continuous is helpful for > regulators with discrete voltages? The main purpose of the function for vctrl is: Do the values reported by _count_voltages()/_list_voltage() actually describe this regulator or do I ignore them because they come from the regulators' supply? > > > > regulator_count_voltages() and regulator_list_voltage() correspond > > > > to the regulator itself or its supply. > > > > Why? > > > Please see my explication above on why the vctrl driver needs to know > > this. > > I'm seeing nothing in the above that addresses my question, you don't > even seem to have mentioned supplies. Sorry, I really didn't try to evade your question. Does it make more sense with the example above? > > In general I think the behavior of these APIs can be confusing for > > users without intimate knowledge of the regulator core. For me (as a > > possibly naive user) it isn't clear why regulator_count_voltages() of > > a continuous regulator would return the voltage count of its supply, > > instead of a value like 0 or -EINVAL that indicates that it is > > continuous. Similar for regulator_list_voltage(). I'm sure there are > > reasons for it, but it's not really intuitive. > > This is happening because continuous regulators are an infrequently used > hack and not every case where they are relevant has been caught. If the > abstractions are confusing or not working then let's improve them, not > just punch holes in the abstraction layers and make the problem worse. > What you're effectively saying is "I don't really understand what's > going on but this seems to work for me" which is a fairly big warning > sign that the solution isn't great. I'm not happy with this function either and agree that the (perceived) need for it could be an indication of an issue in the abstraction. The function was useful to unblock me on certain aspects of the vctrl driver, please interpret it
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
Hi Mark, El Fri, Mar 17, 2017 at 09:15:47PM + Mark Brown ha dit: > On Thu, Mar 09, 2017 at 11:40:54AM -0800, Matthias Kaehlcke wrote: > > El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit: > > > On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote: > > > > > The new function allows consumers to determine if a regulator is > > > > continuous or discrete, and whether the results of > > > > Why? As we discussed at ELC this is an implementation detail of the > > > regulator and it's to an extent a taste decision if the regulator is > > > represented as a linear range or a continuous range (in fact given > > > improvements in the core we could probably just update all continuous > > > range regulators to linear ones). > > > The second patch of this series is a driver for voltage controlled > > regulators (vctrl), ie the output voltage of a vctrl regulator is > > controlled through the voltage of another regulator. The control > > regulator can be continuous or discrete and I think it makes sense for > > the vctrl regulator to mirror its control regulator in this aspect. > > Why should it pretend to have a continuous range when it is actually > > discrete due to the control regulator? > > I don't think we should be providing a consumer facing API which invites > consumers to peer into the implementation details of regulators, or > gives them the idea that these concepts exist. Consumers can already > enumerate the set of supported voltages via _list_voltage() and so on, > I'd expect consumers to be able to get what they need from those. You > have an incrediblity specialist use case here but this is just a general > consumer interface that's being added. In principle I totally agree with you that consumers should be able to enumerate the supported voltages with the existing functions. And they can, as long as they already know (or assume) that the regulator they are using actually has discrete steps, otherwise they might get unexpected results. You are right that my case is very specialist, however I think it is a general problem that a consumer can't know whether the results of _list_voltage(), etc correspond to the regulator itself or to its supplies. E.g. a consumer might have a continuous reg which is supplied by a discrete reg, in this case _list_voltage() would return the steps of the supply reg, which is probably not what most consumers expect. > > For continuous control regulators we can simply calculate a "safe" > > voltage for the next step and pass it to the control regulator. In > > case of a discrete control regulator this calculated voltage may not > > be directly available, without knowing the available steps the vctrl > > driver has to try different voltage ranges until it finds one that is > > accepted by the control regulator. Obviously this can be done but it > > adds code complexity and runtime overhead which is not necessary if we > > know the available steps (and regulator_list_voltage() is already > > there to provide them). > > I'm not seeing how knowing if a regulator is continuous is helpful for > regulators with discrete voltages? The main purpose of the function for vctrl is: Do the values reported by _count_voltages()/_list_voltage() actually describe this regulator or do I ignore them because they come from the regulators' supply? > > > > regulator_count_voltages() and regulator_list_voltage() correspond > > > > to the regulator itself or its supply. > > > > Why? > > > Please see my explication above on why the vctrl driver needs to know > > this. > > I'm seeing nothing in the above that addresses my question, you don't > even seem to have mentioned supplies. Sorry, I really didn't try to evade your question. Does it make more sense with the example above? > > In general I think the behavior of these APIs can be confusing for > > users without intimate knowledge of the regulator core. For me (as a > > possibly naive user) it isn't clear why regulator_count_voltages() of > > a continuous regulator would return the voltage count of its supply, > > instead of a value like 0 or -EINVAL that indicates that it is > > continuous. Similar for regulator_list_voltage(). I'm sure there are > > reasons for it, but it's not really intuitive. > > This is happening because continuous regulators are an infrequently used > hack and not every case where they are relevant has been caught. If the > abstractions are confusing or not working then let's improve them, not > just punch holes in the abstraction layers and make the problem worse. > What you're effectively saying is "I don't really understand what's > going on but this seems to work for me" which is a fairly big warning > sign that the solution isn't great. I'm not happy with this function either and agree that the (perceived) need for it could be an indication of an issue in the abstraction. The function was useful to unblock me on certain aspects of the vctrl driver, please interpret it
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
On Thu, Mar 09, 2017 at 11:40:54AM -0800, Matthias Kaehlcke wrote: > El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit: > > On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote: > > > The new function allows consumers to determine if a regulator is > > > continuous or discrete, and whether the results of > > Why? As we discussed at ELC this is an implementation detail of the > > regulator and it's to an extent a taste decision if the regulator is > > represented as a linear range or a continuous range (in fact given > > improvements in the core we could probably just update all continuous > > range regulators to linear ones). > The second patch of this series is a driver for voltage controlled > regulators (vctrl), ie the output voltage of a vctrl regulator is > controlled through the voltage of another regulator. The control > regulator can be continuous or discrete and I think it makes sense for > the vctrl regulator to mirror its control regulator in this aspect. > Why should it pretend to have a continuous range when it is actually > discrete due to the control regulator? I don't think we should be providing a consumer facing API which invites consumers to peer into the implementation details of regulators, or gives them the idea that these concepts exist. Consumers can already enumerate the set of supported voltages via _list_voltage() and so on, I'd expect consumers to be able to get what they need from those. You have an incrediblity specialist use case here but this is just a general consumer interface that's being added. > For continuous control regulators we can simply calculate a "safe" > voltage for the next step and pass it to the control regulator. In > case of a discrete control regulator this calculated voltage may not > be directly available, without knowing the available steps the vctrl > driver has to try different voltage ranges until it finds one that is > accepted by the control regulator. Obviously this can be done but it > adds code complexity and runtime overhead which is not necessary if we > know the available steps (and regulator_list_voltage() is already > there to provide them). I'm not seeing how knowing if a regulator is continuous is helpful for regulators with discrete voltages? > > > regulator_count_voltages() and regulator_list_voltage() correspond > > > to the regulator itself or its supply. > > Why? > Please see my explication above on why the vctrl driver needs to know > this. I'm seeing nothing in the above that addresses my question, you don't even seem to have mentioned supplies. > In general I think the behavior of these APIs can be confusing for > users without intimate knowledge of the regulator core. For me (as a > possibly naive user) it isn't clear why regulator_count_voltages() of > a continuous regulator would return the voltage count of its supply, > instead of a value like 0 or -EINVAL that indicates that it is > continuous. Similar for regulator_list_voltage(). I'm sure there are > reasons for it, but it's not really intuitive. This is happening because continuous regulators are an infrequently used hack and not every case where they are relevant has been caught. If the abstractions are confusing or not working then let's improve them, not just punch holes in the abstraction layers and make the problem worse. What you're effectively saying is "I don't really understand what's going on but this seems to work for me" which is a fairly big warning sign that the solution isn't great. The reason we report properties of the parent supply if the child supply has no control is so that we can pass operations on up to the parent to implement them there, supporting things like dumb power switches. We shouldn't be leaking details of the parent regulator for anything that does actually regulate. > Above you characterize discrete vs. continuous as an implementation > detail. Aren't we already exposing large parts of it through > regulator_count_voltages() and regulator_list_voltage()? What we should be doing for continuous regulators is allowing people to list the supported voltages as they would for other regulators. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
On Thu, Mar 09, 2017 at 11:40:54AM -0800, Matthias Kaehlcke wrote: > El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit: > > On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote: > > > The new function allows consumers to determine if a regulator is > > > continuous or discrete, and whether the results of > > Why? As we discussed at ELC this is an implementation detail of the > > regulator and it's to an extent a taste decision if the regulator is > > represented as a linear range or a continuous range (in fact given > > improvements in the core we could probably just update all continuous > > range regulators to linear ones). > The second patch of this series is a driver for voltage controlled > regulators (vctrl), ie the output voltage of a vctrl regulator is > controlled through the voltage of another regulator. The control > regulator can be continuous or discrete and I think it makes sense for > the vctrl regulator to mirror its control regulator in this aspect. > Why should it pretend to have a continuous range when it is actually > discrete due to the control regulator? I don't think we should be providing a consumer facing API which invites consumers to peer into the implementation details of regulators, or gives them the idea that these concepts exist. Consumers can already enumerate the set of supported voltages via _list_voltage() and so on, I'd expect consumers to be able to get what they need from those. You have an incrediblity specialist use case here but this is just a general consumer interface that's being added. > For continuous control regulators we can simply calculate a "safe" > voltage for the next step and pass it to the control regulator. In > case of a discrete control regulator this calculated voltage may not > be directly available, without knowing the available steps the vctrl > driver has to try different voltage ranges until it finds one that is > accepted by the control regulator. Obviously this can be done but it > adds code complexity and runtime overhead which is not necessary if we > know the available steps (and regulator_list_voltage() is already > there to provide them). I'm not seeing how knowing if a regulator is continuous is helpful for regulators with discrete voltages? > > > regulator_count_voltages() and regulator_list_voltage() correspond > > > to the regulator itself or its supply. > > Why? > Please see my explication above on why the vctrl driver needs to know > this. I'm seeing nothing in the above that addresses my question, you don't even seem to have mentioned supplies. > In general I think the behavior of these APIs can be confusing for > users without intimate knowledge of the regulator core. For me (as a > possibly naive user) it isn't clear why regulator_count_voltages() of > a continuous regulator would return the voltage count of its supply, > instead of a value like 0 or -EINVAL that indicates that it is > continuous. Similar for regulator_list_voltage(). I'm sure there are > reasons for it, but it's not really intuitive. This is happening because continuous regulators are an infrequently used hack and not every case where they are relevant has been caught. If the abstractions are confusing or not working then let's improve them, not just punch holes in the abstraction layers and make the problem worse. What you're effectively saying is "I don't really understand what's going on but this seems to work for me" which is a fairly big warning sign that the solution isn't great. The reason we report properties of the parent supply if the child supply has no control is so that we can pass operations on up to the parent to implement them there, supporting things like dumb power switches. We shouldn't be leaking details of the parent regulator for anything that does actually regulate. > Above you characterize discrete vs. continuous as an implementation > detail. Aren't we already exposing large parts of it through > regulator_count_voltages() and regulator_list_voltage()? What we should be doing for continuous regulators is allowing people to list the supported voltages as they would for other regulators. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
Hi Mark, El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit: > On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote: > > > The new function allows consumers to determine if a regulator is > > continuous or discrete, and whether the results of > > Why? As we discussed at ELC this is an implementation detail of the > regulator and it's to an extent a taste decision if the regulator is > represented as a linear range or a continuous range (in fact given > improvements in the core we could probably just update all continuous > range regulators to linear ones). The second patch of this series is a driver for voltage controlled regulators (vctrl), ie the output voltage of a vctrl regulator is controlled through the voltage of another regulator. The control regulator can be continuous or discrete and I think it makes sense for the vctrl regulator to mirror its control regulator in this aspect. Why should it pretend to have a continuous range when it is actually discrete due to the control regulator? Also the vctrl driver allows to break down a voltage change into multiple steps to prevent overvoltage protection (OVP) circuitry from shutting down the regulator when a voltage change in a single step would exceed the OVP threshold. For continuous control regulators we can simply calculate a "safe" voltage for the next step and pass it to the control regulator. In case of a discrete control regulator this calculated voltage may not be directly available, without knowing the available steps the vctrl driver has to try different voltage ranges until it finds one that is accepted by the control regulator. Obviously this can be done but it adds code complexity and runtime overhead which is not necessary if we know the available steps (and regulator_list_voltage() is already there to provide them). > > regulator_count_voltages() and regulator_list_voltage() correspond > > to the regulator itself or its supply. > > Why? Please see my explication above on why the vctrl driver needs to know this. In general I think the behavior of these APIs can be confusing for users without intimate knowledge of the regulator core. For me (as a possibly naive user) it isn't clear why regulator_count_voltages() of a continuous regulator would return the voltage count of its supply, instead of a value like 0 or -EINVAL that indicates that it is continuous. Similar for regulator_list_voltage(). I'm sure there are reasons for it, but it's not really intuitive. Above you characterize discrete vs. continuous as an implementation detail. Aren't we already exposing large parts of it through regulator_count_voltages() and regulator_list_voltage()? > > Change-Id: I1198cee9fff60dc747a02860e9652034f4d5da33 > > Don't include noise like this upstream. Sorry, will remove Matthias
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
Hi Mark, El Thu, Mar 09, 2017 at 11:28:19AM +0100 Mark Brown ha dit: > On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote: > > > The new function allows consumers to determine if a regulator is > > continuous or discrete, and whether the results of > > Why? As we discussed at ELC this is an implementation detail of the > regulator and it's to an extent a taste decision if the regulator is > represented as a linear range or a continuous range (in fact given > improvements in the core we could probably just update all continuous > range regulators to linear ones). The second patch of this series is a driver for voltage controlled regulators (vctrl), ie the output voltage of a vctrl regulator is controlled through the voltage of another regulator. The control regulator can be continuous or discrete and I think it makes sense for the vctrl regulator to mirror its control regulator in this aspect. Why should it pretend to have a continuous range when it is actually discrete due to the control regulator? Also the vctrl driver allows to break down a voltage change into multiple steps to prevent overvoltage protection (OVP) circuitry from shutting down the regulator when a voltage change in a single step would exceed the OVP threshold. For continuous control regulators we can simply calculate a "safe" voltage for the next step and pass it to the control regulator. In case of a discrete control regulator this calculated voltage may not be directly available, without knowing the available steps the vctrl driver has to try different voltage ranges until it finds one that is accepted by the control regulator. Obviously this can be done but it adds code complexity and runtime overhead which is not necessary if we know the available steps (and regulator_list_voltage() is already there to provide them). > > regulator_count_voltages() and regulator_list_voltage() correspond > > to the regulator itself or its supply. > > Why? Please see my explication above on why the vctrl driver needs to know this. In general I think the behavior of these APIs can be confusing for users without intimate knowledge of the regulator core. For me (as a possibly naive user) it isn't clear why regulator_count_voltages() of a continuous regulator would return the voltage count of its supply, instead of a value like 0 or -EINVAL that indicates that it is continuous. Similar for regulator_list_voltage(). I'm sure there are reasons for it, but it's not really intuitive. Above you characterize discrete vs. continuous as an implementation detail. Aren't we already exposing large parts of it through regulator_count_voltages() and regulator_list_voltage()? > > Change-Id: I1198cee9fff60dc747a02860e9652034f4d5da33 > > Don't include noise like this upstream. Sorry, will remove Matthias
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote: > The new function allows consumers to determine if a regulator is > continuous or discrete, and whether the results of Why? As we discussed at ELC this is an implementation detail of the regulator and it's to an extent a taste decision if the regulator is represented as a linear range or a continuous range (in fact given improvements in the core we could probably just update all continuous range regulators to linear ones). > regulator_count_voltages() and regulator_list_voltage() correspond > to the regulator itself or its supply. Why? > Change-Id: I1198cee9fff60dc747a02860e9652034f4d5da33 Don't include noise like this upstream. signature.asc Description: PGP signature
Re: [PATCH v2 1/2] regulator: core: add regulator_has_continuous_voltage_range()
On Wed, Mar 08, 2017 at 12:02:45PM -0800, Matthias Kaehlcke wrote: > The new function allows consumers to determine if a regulator is > continuous or discrete, and whether the results of Why? As we discussed at ELC this is an implementation detail of the regulator and it's to an extent a taste decision if the regulator is represented as a linear range or a continuous range (in fact given improvements in the core we could probably just update all continuous range regulators to linear ones). > regulator_count_voltages() and regulator_list_voltage() correspond > to the regulator itself or its supply. Why? > Change-Id: I1198cee9fff60dc747a02860e9652034f4d5da33 Don't include noise like this upstream. signature.asc Description: PGP signature
