Message-ID: <3aa82f65-4812-4bf0-9323-96f40824a004@sirena.org.uk>
Date: Thu, 7 Aug 2025 18:26:25 +0100
From: Mark Brown <broonie@...nel.org>
To: Nitin Rawat <quic_nitirawa@...cinc.com>
Cc: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, vkoul@...nel.org,
	kishon@...nel.org, mani@...nel.org, conor+dt@...nel.org,
	bvanassche@....org, andersson@...nel.org, neil.armstrong@...aro.org,
	dmitry.baryshkov@....qualcomm.com, konradybcio@...nel.org,
	krzk+dt@...nel.org, linux-arm-msm@...r.kernel.org,
	linux-phy@...ts.infradead.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from
 device tree

On Thu, Aug 07, 2025 at 09:12:53PM +0530, Nitin Rawat wrote:
> On 8/7/2025 7:14 PM, Mark Brown wrote:

> > > The intended use is to set the load requirement and then only en/disable
> > > the consumer, so that the current load is updated in core (like in the
> > > kerneldoc of _regulator_handle_consumer_enable())

> > > My question was about moving the custom parsing of
> > > $supplyname-max-micromap introduced in this patch into the regulator
> > > core, as this seems like a rather common problem.

> > Wait, is this supposed to be some new property that you want to
> > standardise?  I didn't see a proposal for that, it's not something that
> > currently exists - the only standard properties that currently exist are
> > for the regulator as a whole.

> The UFS QMP PHY driver is not the first client to use regulator_set_load or
> alternatively set load requirements and invoke enable/disable or
> alternatively

The issue isn't using regulator_set_load(), that's perfectly fine and
expected.  The issue is either reading the value to use from the
constraint information (which is just buggy) or adding a generic
property for this (which I'm not convinced is a good idea, I'd expect a
large propoprtion of drivers should just know what their requirements
are and that a generic property would just get abused).

> These drivers also define corresponding binding properties, as seen in the
> UFS instances documented here:

> UFS Common DT Binding ((link - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ufs/ufs-common.yaml?h=next-20250807)

Note that that's specifying OPPs which is different...

> There was a previous effort to introduce similar properties
> (vdda-phy-max-microamp and vdda-pll-max-microamp) in the device tree
> bindings.
> Link - (link- https://patchwork.kernel.org/project/linux-arm-msm/patch/20220418205509.1102109-3-bhupesh.sharma@linaro.org/#24820481)

That patch also fails to supply any rationale for making this board
specific or generally putting them in the DT, AFAICT it's one of these
things just pulled out of the vendor tree without really thinking about
it.  The changelog just says the properties are in downstream DTs.

> Currently, the regulator framework does support automatic aggregation of
> load requests from multiple client drivers. Therefore, it is reasonable and
> necessary for each client to individually communicate its expected runtime
> load to the regulator framework to put the regulators in current
> operation mode.

That doesn't mean that it's a good idea to put that information in the
DT, nor if it is sensible to put in DT does it mean that it's a good
idea to define a generic property that applies to all regulator
consumers which is what I now think Konrad is proposing.

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

Powered by blists - more mailing lists