Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat <quic_nitirawa@quicinc.com>]

On 8/7/2025 11:13 PM, Konrad Dybcio wrote:
> On 8/7/25 7:26 PM, Mark Brown wrote:
>> 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...
> 
> The microamp properties are in the top-level, not under OPP if
> that's what you meant

Thanks for pointing that out, Konrad


> 
> Or are you perhaps suggesting that any device requiring explicit
> current requirement settings, should do so through an OPP table
> (perhaps a degenerated one with just a single entry detailling
> the single requirement most of the time)?
> 
>>> 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.
> 
> Yeah, that's what I had in mind
> 
> I was never able to get a reliable source for those numbers myselfe
> either.. At least some of them are prooooobably? chosen based on the
> used regulator type, to ensure it's always in HPM..
> 
> That said, our drivers cover a wide variety of hardware, built on a
> wide variety of process nodes, with different configurations, etc.,
> so it's either polluting the DT, or polluting the driver with
> per-compatible hardcoded data (and additional compatibles because
> fallbacks wouldn't work most of the time)

I missed to read this before replying to my last reply.
Thanks for the explaining this. I too had mention similar thing in reply 
to mark's query in my last reply.



> 
> Konrad