Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Chanwoo Choi <cw00.choi@samsung.com>
To: Kamil Konieczny <k.konieczny@partner.samsung.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Kukjin Kim <kgene@kernel.org>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Mark Rutland <mark.rutland@arm.com>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	Nishanth Menon <nm@ti.com>, Rob Herring <robh+dt@kernel.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Viresh Kumar <vireshk@kernel.org>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-samsung-soc@vger.kernel.org
Subject: Re: [PATCH v2 1/4] opp: core: add regulators enable and disable
Date: Tue, 16 Jul 2019 13:03:06 +0900
Message-ID: <9acc7dd0-614b-ccd3-a485-eeca3dab494b@samsung.com> (raw)
In-Reply-To: <20190715120416.3561-2-k.konieczny@partner.samsung.com>

Hi Kamil,

On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().

IMHO, it is not proper to mention the specific driver name.
If you explain the reason why enable the regulator before using it,
it is enough description.

> 
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> --
> Changes in v2:
> 
> - move regulator enable and disable into loop
> 
> ---
>  drivers/opp/core.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..069c5cf8827e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>  			goto free_regulators;
>  		}
>  
> +		ret = regulator_enable(reg);
> +		if (ret < 0)
> +			goto disable;
> +
>  		opp_table->regulators[i] = reg;
>  	}
>  
> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>  
>  	return opp_table;
>  
> +disable:
> +	regulator_put(reg);
> +	--i;
> +
>  free_regulators:
> -	while (i != 0)
> -		regulator_put(opp_table->regulators[--i]);
> +	for (; i >= 0; --i) {
> +		regulator_disable(opp_table->regulators[i]);
> +		regulator_put(opp_table->regulators[i]);
> +	}
>  
>  	kfree(opp_table->regulators);
>  	opp_table->regulators = NULL;
> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>  	/* Make sure there are no concurrent readers while updating opp_table */
>  	WARN_ON(!list_empty(&opp_table->opp_list));
>  
> -	for (i = opp_table->regulator_count - 1; i >= 0; i--)
> +	for (i = opp_table->regulator_count - 1; i >= 0; i--) {
> +		regulator_disable(opp_table->regulators[i]);
>  		regulator_put(opp_table->regulators[i]);
> +	}
>  
>  	_free_set_opp_data(opp_table);
>  
> 

I agree to enable the regulator before using it.
The bootloader might not enable the regulators
and the kernel need to enable regulator in order to increase
the reference count explicitly event if bootloader enables it.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20190715120430eucas1p1dd216e552403899e614845295373e467@eucas1p1.samsung.com>
2019-07-15 12:04 ` [PATCH v2 0/4] add coupled regulators for Exynos5422/5800 Kamil Konieczny
     [not found]   ` <CGME20190715120430eucas1p19dddcc93756e6a110d3476229f9428b3@eucas1p1.samsung.com>
2019-07-15 12:04     ` [PATCH v2 1/4] opp: core: add regulators enable and disable Kamil Konieczny
2019-07-16  4:03       ` Chanwoo Choi [this message]
2019-07-17 14:12         ` Kamil Konieczny
2019-07-16 10:05       ` Viresh Kumar
2019-07-17 14:14         ` Kamil Konieczny
     [not found]   ` <CGME20190715120431eucas1p215eae81d0ca772d7e2a22a803669068a@eucas1p2.samsung.com>
2019-07-15 12:04     ` [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate() Kamil Konieczny
2019-07-16  3:56       ` Chanwoo Choi
2019-07-16 10:13         ` Bartlomiej Zolnierkiewicz
2019-07-16 10:33           ` Chanwoo Choi
2019-07-16 10:59             ` Bartlomiej Zolnierkiewicz
2019-07-16 11:26               ` Chanwoo Choi
2019-07-16 11:39                 ` Bartlomiej Zolnierkiewicz
2019-07-16 11:56                   ` Bartlomiej Zolnierkiewicz
     [not found]   ` <CGME20190715120432eucas1p1b32d72d239420b861bf8596d4e8a053d@eucas1p1.samsung.com>
2019-07-15 12:04     ` [PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800 Kamil Konieczny
2019-07-16  9:00       ` Chanwoo Choi
2019-07-16  9:22       ` Krzysztof Kozlowski
2019-07-16 10:30         ` Bartlomiej Zolnierkiewicz
     [not found]   ` <CGME20190715120433eucas1p26681c5c2d87423253b651d88446c538c@eucas1p2.samsung.com>
2019-07-15 12:04     ` [PATCH v2 4/4] dt-bindings: devfreq: exynos-bus: remove unused property Kamil Konieczny
2019-07-16  8:54       ` Chanwoo Choi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9acc7dd0-614b-ccd3-a485-eeca3dab494b@samsung.com \
    --to=cw00.choi@samsung.com \
    --cc=b.zolnierkie@samsung.com \
    --cc=devicetree@vger.kernel.org \
    --cc=k.konieczny@partner.samsung.com \
    --cc=kgene@kernel.org \
    --cc=krzk@kernel.org \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=m.szyprowski@samsung.com \
    --cc=mark.rutland@arm.com \
    --cc=myungjoo.ham@samsung.com \
    --cc=nm@ti.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=vireshk@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox