Received-SPF: pass (google.com: domain of linux-kernel+bounces-179688-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 15 May 2024 17:02:29 +0800
From: Alina Yu <alina_yu@richtek.com>
To: Conor Dooley <conor.dooley@microchip.com>
CC: Conor Dooley <conor@kernel.org>, Mark Brown <broonie@kernel.org>,
	<lgirdwood@gmail.com>, <robh+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
	<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<johnny_lai@richtek.com>, <cy_huang@richtek.com>
Subject: Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to
 get ldo of RTQ2208 is adjustable or not
Message-ID: <20240515090229.GA15889@linuxcarl2.richtek.com>
References: <cover.1715340537.git.alina_yu@richtek.com>
 <6a3a90d9aa2022dfb92e124e417f3e72c2f28b0b.1715340537.git.alina_yu@richtek.com>
 <20240513-tissue-repave-13d2e3bf88fd@spud>
 <d97752ed-4032-4681-b28f-17f149fdc3d4@sirena.org.uk>
 <20240514-plunging-chair-803d9e342e6f@spud>
 <20240515073830.GA12525@linuxcarl2.richtek.com>
 <20240515-wrinkle-engross-ab6b089baae3@wendy>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240515-wrinkle-engross-ab6b089baae3@wendy>
User-Agent: Mutt/1.5.21 (2010-09-15)

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