linux-kernel - Re: [PATCH v2] media: qcom: camss: Add support for regulator init_load

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] [day] [month] [year] [list]

Message-ID: <hosfuvk34iolc4ylzqu2pyoozomw4nzirlfdj54x3777eyuok6@renjfbqznl4r>
Date: Tue, 12 Aug 2025 13:06:39 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Wenmeng Liu <quic_wenmliu@...cinc.com>
Cc: Robert Foss <rfoss@...nel.org>, Todor Tomov <todor.too@...il.com>,
        Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: qcom: camss: Add support for regulator
 init_load_uA in CSIPHY

On Tue, Aug 12, 2025 at 02:05:55PM +0800, Wenmeng Liu wrote:
> 
> 
> On 2025/8/11 18:39, Dmitry Baryshkov wrote:
> > On Tue, Jul 29, 2025 at 03:24:55PM +0800, Wenmeng Liu wrote:
> > > Some Qualcomm regulators are configured with initial mode as
> > > HPM (High Power Mode), which may lead to higher power consumption.
> > > To reduce power usage, it's preferable to set the initial mode
> > > to LPM (Low Power Mode).
> > > 
> > > To ensure the regulator can switch from LPM to HPM when needed,
> > > this patch adds current load configuration for CAMSS CSIPHY.
> > > This allows the regulator framework to scale the mode dynamically
> > > based on the load requirement.
> > > 
> > > The current default value for current is uninitialized or random.
> > > To address this, initial current values are added for the
> > > following platforms:
> > > SDM670, SM8250, SC7280, SM8550, and X1E80100.
> > > 
> > > For SDM670, the value is set to -1, indicating that no default
> > > current value is configured, the other values are derived
> > > from the power grid.
> > > 
> > > ---
> > > Changes in v2:
> > > - Change the source of the current value from DTS to CAMSS resource
> > > - Link to v1: https://lore.kernel.org/all/20250620040736.3032667-1-quic_wenmliu@quicinc.com/
> > > ---
> > > 
> > > Signed-off-by: Wenmeng Liu <quic_wenmliu@...cinc.com>
> > > ---
> > >   drivers/media/platform/qcom/camss/camss-csiphy.c |  4 +++-
> > >   drivers/media/platform/qcom/camss/camss.c        | 26 ++++++++++++++++++++++++
> > >   drivers/media/platform/qcom/camss/camss.h        |  1 +
> > >   3 files changed, 30 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> > > index 2de97f58f9ae4f91e8bba39dcadf92bea8cf6f73..7a2d80a03dbd0884b614451b55cd27dce94af637 100644
> > > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> > > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> > > @@ -707,8 +707,10 @@ int msm_csiphy_subdev_init(struct camss *camss,
> > >   			return -ENOMEM;
> > >   	}
> > > -	for (i = 0; i < csiphy->num_supplies; i++)
> > > +	for (i = 0; i < csiphy->num_supplies; i++) {
> > >   		csiphy->supplies[i].supply = res->regulators[i];
> > > +		csiphy->supplies[i].init_load_uA = res->regulators_current[i];
> > 
> > Could you possibly refactor to use devm_regulator_bulk_get_const()? It
> > would save you from this data moving.
> Initially, we were aiming for a minimal-change implementation.
> Consider refactor for save data moving, will be refactored in the next
> version.
> > 
> > > +	}
> > >   	ret = devm_regulator_bulk_get(camss->dev, csiphy->num_supplies,
> > >   				      csiphy->supplies);
> > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> > > index e08e70b93824baa5714b3a736bc1d05405253aaa..daf21c944c2b4818b1656efc255e817551788658 100644
> > > --- a/drivers/media/platform/qcom/camss/camss.c
> > > +++ b/drivers/media/platform/qcom/camss/camss.c
> > > @@ -750,6 +750,7 @@ static const struct camss_subdev_resources csiphy_res_670[] = {
> > >   	/* CSIPHY0 */
> > >   	{
> > >   		.regulators = { "vdda-phy", "vdda-pll" },
> > > +		.regulators_current = { -1, -1 },
> > 
> > If it's unset, it should be 0, not -1.
> I considered existing implementations as a reference:
> https://lore.kernel.org/all/20220804073608.v4.5.I55a9e65cb1c22221316629e98768ff473f47a067@changeid
> 
> but based on the implementation of regulator_bulk_get, setting it to 0 seems
> to be a better approach:

Ack. Also please try to provide a sensible value for all platforms, not
just the most recent ones.

> 
> drivers/regulator/core.c
> consumers[i].init_load_uA > 0
> 
> Thanks,
> Wenmeng
> 

-- 
With best wishes
Dmitry