Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to get ldo of RTQ2208 is adjustable or not

[Alina Yu <alina_yu@richtek.com>]

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