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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240515-wrinkle-engross-ab6b089baae3@wendy>
Date: Wed, 15 May 2024 09:06:06 +0100
From: Conor Dooley <conor.dooley@...rochip.com>
To: Alina Yu <alina_yu@...htek.com>
CC: Conor Dooley <conor@...nel.org>, Mark Brown <broonie@...nel.org>,
	<lgirdwood@...il.com>, <robh+dt@...nel.org>,
	<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
	<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<johnny_lai@...htek.com>, <cy_huang@...htek.com>
Subject: Re: [PATCH v3 6/6] regulator: dt-bindings: rtq2208: Add property to
 get ldo of RTQ2208 is adjustable or not

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.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)