| Message ID | 6a3a90d9aa2022dfb92e124e417f3e72c2f28b0b.1715340537.git.alina_yu@richtek.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix rtq2208 BUCK ramp_delay and LDO dvs setting | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote: > Since there is no way to check is ldo is adjustable or not. > As discussing in v2 series, 'richtek,fixed-microvolt' is added for that. > user is supposed to know whether vout of ldo is adjustable. > > Signed-off-by: Alina Yu <alina_yu@richtek.com> > --- > Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml > index 609c066..6212f44 100644 > --- a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml > +++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml > @@ -75,6 +75,13 @@ properties: > description: > regulator description for ldo[1-2]. > > + properties: > + richtek,fixed-microvolt: > + description: | > + If it exists, the voltage is unadjustable. > + There is no risk-free method for software to determine whether the ldo vout is fixed or not. > + Therefore, it can only be done in this way. > + > required: > - compatible > - reg > @@ -177,6 +184,7 @@ examples: > }; > }; > ldo1 { > + richtek,fixed-microvolt = <1200000>; > regulator-min-microvolt = <1200000>; > regulator-max-microvolt = <1200000>; I'm dumb and this example seemed odd to me. Can you explain to me why it is not sufficient to set min-microvolt == max-microvolt to achieve the same thing? Cheers, Conor.
On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote: > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote: > > + richtek,fixed-microvolt = <1200000>; > > regulator-min-microvolt = <1200000>; > > regulator-max-microvolt = <1200000>; > I'm dumb and this example seemed odd to me. Can you explain to me why > it is not sufficient to set min-microvolt == max-microvolt to achieve > the same thing? This is for a special mode where the voltage being configured is out of the range usually supported by the regulator, requiring a hardware design change to achieve. The separate property is because otherwise we can't distinguish the case where the mode is in use from the case where the constraints are nonsense, and we need to handle setting a fixed voltage on a configurable regulator differently to there being a hardware fixed voltage on a normally configurable regulator.
On Tue, May 14, 2024 at 11:34:29AM +0100, Mark Brown wrote: > On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote: > > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote: > > > > + richtek,fixed-microvolt = <1200000>; > > > regulator-min-microvolt = <1200000>; > > > regulator-max-microvolt = <1200000>; > > > I'm dumb and this example seemed odd to me. Can you explain to me why > > it is not sufficient to set min-microvolt == max-microvolt to achieve > > the same thing? > > This is for a special mode where the voltage being configured is out of > the range usually supported by the regulator, requiring a hardware > design change to achieve. The separate property is because otherwise we > can't distinguish the case where the mode is in use from the case where > the constraints are nonsense, and we need to handle setting a fixed > voltage on a configurable regulator differently to there being a > hardware fixed voltage on a normally configurable regulator. Cool, I think an improved comment message and description would be helpful then to describe the desired behaviour that you mention here. The commit message in particular isn't great: | Since there is no way to check is ldo is adjustable or not. | As discussing in v2 series, 'richtek,fixed-microvolt' is added for that. | user is supposed to know whether vout of ldo is adjustable. It also doesn't seem like this sort of behaviour would be limited to Richtek either, should this actually be a common property in regulator.yaml w/o the vendor prefix? Cheers, Conor.
On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote: > On Tue, May 14, 2024 at 11:34:29AM +0100, Mark Brown wrote: > > On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote: > > > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote: > > > > > > + richtek,fixed-microvolt = <1200000>; > > > > regulator-min-microvolt = <1200000>; > > > > regulator-max-microvolt = <1200000>; > > > > > I'm dumb and this example seemed odd to me. Can you explain to me why > > > it is not sufficient to set min-microvolt == max-microvolt to achieve > > > the same thing? > > > > This is for a special mode where the voltage being configured is out of > > the range usually supported by the regulator, requiring a hardware > > design change to achieve. The separate property is because otherwise we > > can't distinguish the case where the mode is in use from the case where > > the constraints are nonsense, and we need to handle setting a fixed > > voltage on a configurable regulator differently to there being a > > hardware fixed voltage on a normally configurable regulator. > > Cool, I think an improved comment message and description would be > helpful then to describe the desired behaviour that you mention here. > The commit message in particular isn't great: > | Since there is no way to check is ldo is adjustable or not. > | As discussing in v2 series, 'richtek,fixed-microvolt' is added for that. > | user is supposed to know whether vout of ldo is adjustable. > > It also doesn't seem like this sort of behaviour would be limited to > Richtek either, should this actually be a common property in > regulator.yaml w/o the vendor prefix? > > Cheers, > Conor. Hi Conor, Should I update v4 to fix the commit message ? I will modify it as follows. There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode. As the fixed voltage for the LDO is outside the range of the adjustable voltage mode, the constraints for this scenario are not suitable to represent both modes. In version 3, a property has been added to specify the fixed voltage. Thanks, Alina
On Wed, May 15, 2024 at 03:38:30PM +0800, Alina Yu wrote: > On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote: > > On Tue, May 14, 2024 at 11:34:29AM +0100, Mark Brown wrote: > > > On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote: > > > > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote: > > > > > > > > + richtek,fixed-microvolt = <1200000>; > > > > > regulator-min-microvolt = <1200000>; > > > > > regulator-max-microvolt = <1200000>; > > > > > > > I'm dumb and this example seemed odd to me. Can you explain to me why > > > > it is not sufficient to set min-microvolt == max-microvolt to achieve > > > > the same thing? > > > > > > This is for a special mode where the voltage being configured is out of > > > the range usually supported by the regulator, requiring a hardware > > > design change to achieve. The separate property is because otherwise we > > > can't distinguish the case where the mode is in use from the case where > > > the constraints are nonsense, and we need to handle setting a fixed > > > voltage on a configurable regulator differently to there being a > > > hardware fixed voltage on a normally configurable regulator. > > > > Cool, I think an improved comment message and description would be > > helpful then to describe the desired behaviour that you mention here. > > The commit message in particular isn't great: > > | Since there is no way to check is ldo is adjustable or not. > > | As discussing in v2 series, 'richtek,fixed-microvolt' is added for that. > > | user is supposed to know whether vout of ldo is adjustable. > > > > It also doesn't seem like this sort of behaviour would be limited to > > Richtek either, should this actually be a common property in > > regulator.yaml w/o the vendor prefix? > > > > Cheers, > > Conor. > > > Hi Conor, > > > Should I update v4 to fix the commit message ? > I will modify it as follows. > > > There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode. > > As the fixed voltage for the LDO is outside the range of the adjustable voltage mode, > the constraints for this scenario are not suitable to represent both modes. That's definitely an improvement, yes. The property description could also do with an update to explain that this is for a situation where the fixed voltage is out of the adjustable range, it doesn't mention that at all right now. > In version 3, a property has been added to specify the fixed voltage. Don't refer to previous versions of the patchset in your commit message, that doesn't help people reading a commit log in the future etc. If there's some relevant information in a previous version patchset, put it in the commit message directly. Cheers, Conor.
On Wed, May 15, 2024 at 09:06:06AM +0100, Conor Dooley wrote: > On Wed, May 15, 2024 at 03:38:30PM +0800, Alina Yu wrote: > > On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote: > > > On Tue, May 14, 2024 at 11:34:29AM +0100, Mark Brown wrote: > > > > On Mon, May 13, 2024 at 05:22:54PM +0100, Conor Dooley wrote: > > > > > On Fri, May 10, 2024 at 08:06:25PM +0800, Alina Yu wrote: > > > > > > > > > > + richtek,fixed-microvolt = <1200000>; > > > > > > regulator-min-microvolt = <1200000>; > > > > > > regulator-max-microvolt = <1200000>; > > > > > > > > > I'm dumb and this example seemed odd to me. Can you explain to me why > > > > > it is not sufficient to set min-microvolt == max-microvolt to achieve > > > > > the same thing? > > > > > > > > This is for a special mode where the voltage being configured is out of > > > > the range usually supported by the regulator, requiring a hardware > > > > design change to achieve. The separate property is because otherwise we > > > > can't distinguish the case where the mode is in use from the case where > > > > the constraints are nonsense, and we need to handle setting a fixed > > > > voltage on a configurable regulator differently to there being a > > > > hardware fixed voltage on a normally configurable regulator. > > > > > > Cool, I think an improved comment message and description would be > > > helpful then to describe the desired behaviour that you mention here. > > > The commit message in particular isn't great: > > > | Since there is no way to check is ldo is adjustable or not. > > > | As discussing in v2 series, 'richtek,fixed-microvolt' is added for that. > > > | user is supposed to know whether vout of ldo is adjustable. > > > > > > It also doesn't seem like this sort of behaviour would be limited to > > > Richtek either, should this actually be a common property in > > > regulator.yaml w/o the vendor prefix? > > > > > > Cheers, > > > Conor. > > > > > > Hi Conor, > > > > > > Should I update v4 to fix the commit message ? > > I will modify it as follows. > > > > > > There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode. > > > > As the fixed voltage for the LDO is outside the range of the adjustable voltage mode, > > the constraints for this scenario are not suitable to represent both modes. > > That's definitely an improvement, yes. The property description could > also do with an update to explain that this is for a situation where the > fixed voltage is out of the adjustable range, it doesn't mention that at > all right now. > > > In version 3, a property has been added to specify the fixed voltage. > > Don't refer to previous versions of the patchset in your commit message, > that doesn't help people reading a commit log in the future etc. If > there's some relevant information in a previous version patchset, put it > in the commit message directly. > > Cheers, > Conor. Hi Conor, May I modify the description as follows ? properties: richtek,fixed-microvolt: description: | - If it exists, the voltage is unadjustable. - There is no risk-free method for software to determine whether the ldo vout is fixed or not. - Therefore, it can only be done in this way. + + There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode. + For fixed voltage mode, the working voltage is out side the range of the adjustable mode. + The constraints are unsuitable to describe both modes. + Therefore, this property is added to specify the fixed LDO VOUT. Thanks, Alina
On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote: > It also doesn't seem like this sort of behaviour would be limited to > Richtek either, should this actually be a common property in > regulator.yaml w/o the vendor prefix? It's a pretty weird thing for hardware to do - usually if the regulator is controllable it'll be qualified for use within whatever range it's variable over and not some other completely disjoint value.
On Wed, May 15, 2024 at 01:10:31PM +0100, Mark Brown wrote: > On Tue, May 14, 2024 at 07:01:21PM +0100, Conor Dooley wrote: > > > It also doesn't seem like this sort of behaviour would be limited to > > Richtek either, should this actually be a common property in > > regulator.yaml w/o the vendor prefix? > > It's a pretty weird thing for hardware to do - usually if the regulator > is controllable it'll be qualified for use within whatever range it's > variable over and not some other completely disjoint value. Okay cool, just the commit message/description to be changed then. Thanks for the info
On Wed, May 15, 2024 at 05:02:29PM +0800, Alina Yu wrote: > properties: > richtek,fixed-microvolt: > description: | > - If it exists, the voltage is unadjustable. > - There is no risk-free method for software to determine whether the ldo vout is fixed or not. > - Therefore, it can only be done in this way. > + > + There are two types of LDO VOUT: fixed voltage mode and adjustable voltage mode. > + For fixed voltage mode, the working voltage is out side the range of the adjustable mode. > + The constraints are unsuitable to describe both modes. > + Therefore, this property is added to specify the fixed LDO VOUT. I think you could do something as simple as: This property can be used to set a fixed operating voltage that lies outside of the range of the regulator's adjustable mode. BTW, you should probably change the example so that the voltage you add there is actually outside of the range, rather than identical to one of the range's constraints :)
On Wed, May 15, 2024 at 04:51:30PM +0100, Conor Dooley wrote: > BTW, you should probably change the example so that the voltage you add > there is actually outside of the range, rather than identical to one of > the range's constraints :) No, that'd be invalid - the constraints need to include a value offered by the regulator, in this case the one fixed voltage. For a fixed voltage regulator it's probably better to just not specify a voltage range since it can't be changed anyway.
On Wed, May 15, 2024 at 05:04:32PM +0100, Mark Brown wrote: > On Wed, May 15, 2024 at 04:51:30PM +0100, Conor Dooley wrote: > > > BTW, you should probably change the example so that the voltage you add > > there is actually outside of the range, rather than identical to one of > > the range's constraints :) > > No, that'd be invalid - the constraints need to include a value offered > by the regulator, in this case the one fixed voltage. For a fixed > voltage regulator it's probably better to just not specify a voltage > range since it can't be changed anyway. I see. Clearly I got your previous explanation of why the property was needed arseways.
diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml index 609c066..6212f44 100644 --- a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml +++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml @@ -75,6 +75,13 @@ properties: description: regulator description for ldo[1-2]. + properties: + richtek,fixed-microvolt: + description: | + If it exists, the voltage is unadjustable. + There is no risk-free method for software to determine whether the ldo vout is fixed or not. + Therefore, it can only be done in this way. + required: - compatible - reg @@ -177,6 +184,7 @@ examples: }; }; ldo1 { + richtek,fixed-microvolt = <1200000>; regulator-min-microvolt = <1200000>; regulator-max-microvolt = <1200000>; regulator-always-on;
Since there is no way to check is ldo is adjustable or not. As discussing in v2 series, 'richtek,fixed-microvolt' is added for that. user is supposed to know whether vout of ldo is adjustable. Signed-off-by: Alina Yu <alina_yu@richtek.com> --- Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)