Received-SPF: pass (google.com: domain of linux-kernel+bounces-179626-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 15 May 2024 09:06:06 +0100
From: Conor Dooley <conor.dooley@microchip.com>
To: Alina Yu <alina_yu@richtek.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: <20240515-wrinkle-engross-ab6b089baae3@wendy>
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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="0FcHBb/wqZoBsRLq"
Content-Disposition: inline
In-Reply-To: <20240515073830.GA12525@linuxcarl2.richtek.com>

--0FcHBb/wqZoBsRLq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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:
> > >=20
> > > > > +            richtek,fixed-microvolt =3D <1200000>;
> > > > >              regulator-min-microvolt =3D <1200000>;
> > > > >              regulator-max-microvolt =3D <1200000>;
> > >=20
> > > > I'm dumb and this example seemed odd to me. Can you explain to me w=
hy
> > > > it is not sufficient to set min-microvolt =3D=3D max-microvolt to a=
chieve
> > > > the same thing?
> > >=20
> > > 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 whe=
re
> > > 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.
> >=20
> > 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 th=
at.
> > | user is supposed to know whether vout of ldo is adjustable.
> >=20
> > 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?
> >=20
> > Cheers,
> > Conor.
>=20
>=20
> Hi Conor,
>=20
>=20
> Should I update v4 to fix the commit message ?
> I will modify it as follows.
>=20
>=20
> There are two types of LDO VOUT: fixed voltage mode and adjustable voltag=
e mode.
>=20
> As the fixed voltage for the LDO is outside the range of the adjustable v=
oltage mode,
> the constraints for this scenario are not suitable to represent both mode=
s.

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.

--0FcHBb/wqZoBsRLq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZkRs7gAKCRB4tDGHoIJi
0vH7AQD9Pv88a/PkA91egE2kjvXH7h48NPsjCN8xwJ8vNsM5XgEAuD17AiUn8eu7
CNpGygjTUdxaBxaAC0nC6jR2+XMgXwg=
=ELYJ
-----END PGP SIGNATURE-----

--0FcHBb/wqZoBsRLq--