From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Joao Paulo Menezes Linaris <jplinaris@usp.br>
Cc: wbg@kernel.org, Guilherme Dias <guilhermeabreu200105@usp.br>,
linux-iio@vger.kernel.org
Subject: Re: [PATCH] counter: intel-qep: Replace manual mutex logic with lock guards
Date: Thu, 23 Apr 2026 14:42:13 +0300 (EEST) [thread overview]
Message-ID: <3ab73ef5-0a3a-66b4-a763-398f0921c289@linux.intel.com> (raw)
In-Reply-To: <20260422210537.12127-1-jplinaris@usp.br>
On Wed, 22 Apr 2026, Joao Paulo Menezes Linaris wrote:
> Use scoped_guard() for handling mutex lock instead of locking and unlocking mutex explicitly. This improves readability by eliminating the need for gotos and by clearly indicating mutex will be locked only when execution is in scoped_guard() scope.
Please fold the changelog text properly.
>
> Signed-off-by: Joao Paulo Menezes Linaris <jplinaris@usp.br>
> Co-developed-by: Guilherme Dias <guilhermeabreu200105@usp.br>
> Signed-off-by: Guilherme Dias <guilhermeabreu200105@usp.br>
> ---
> drivers/counter/intel-qep.c | 126 +++++++++++++++++-------------------
> 1 file changed, 58 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> index c49c17805..199a8d562 100644
> --- a/drivers/counter/intel-qep.c
> +++ b/drivers/counter/intel-qep.c
> @@ -188,25 +188,22 @@ static int intel_qep_ceiling_write(struct counter_device *counter,
> struct counter_count *count, u64 max)
> {
> struct intel_qep *qep = counter_priv(counter);
> - int ret = 0;
>
> /* Intel QEP ceiling configuration only supports 32-bit values */
> if (max != (u32)max)
> return -ERANGE;
>
> - mutex_lock(&qep->lock);
> - if (qep->enabled) {
> - ret = -EBUSY;
> - goto out;
> - }
> + /* Lock mutex while execution in scoped_guard() scope */
Why add dead obvious comments like this. Please don't do it.
> + scoped_guard(mutex, &qep->lock){
> + if (qep->enabled)
> + return -EBUSY;
>
> - pm_runtime_get_sync(qep->dev);
> - intel_qep_writel(qep, INTEL_QEPMAX, max);
> - pm_runtime_put(qep->dev);
> + pm_runtime_get_sync(qep->dev);
> + intel_qep_writel(qep, INTEL_QEPMAX, max);
> + pm_runtime_put(qep->dev);
> + }
>
> -out:
> - mutex_unlock(&qep->lock);
> - return ret;
> + return 0;
Why is using scoped variable of guard necessary in these cases??? Are
you just making these changes using some tool without inspecting if the
end result makes sense or not??
Also, runtime PM also has guards support added (admittedly changing them
doesn't remove gotos). But one of the cases seems non-pairing so that
seems not convertable to guard.
> }
>
> static int intel_qep_enable_read(struct counter_device *counter,
> @@ -226,28 +223,28 @@ static int intel_qep_enable_write(struct counter_device *counter,
> u32 reg;
> bool changed;
>
> - mutex_lock(&qep->lock);
> - changed = val ^ qep->enabled;
> - if (!changed)
> - goto out;
> -
> - pm_runtime_get_sync(qep->dev);
> - reg = intel_qep_readl(qep, INTEL_QEPCON);
> - if (val) {
> - /* Enable peripheral and keep runtime PM always on */
> - reg |= INTEL_QEPCON_EN;
> - pm_runtime_get_noresume(qep->dev);
> - } else {
> - /* Let runtime PM be idle and disable peripheral */
> - pm_runtime_put_noidle(qep->dev);
> - reg &= ~INTEL_QEPCON_EN;
> + /* Lock mutex while execution in scoped_guard() scope */
> + scoped_guard(mutex, &qep->lock){
> + changed = val ^ qep->enabled;
> + if (!changed)
> + return 0;
> +
> + pm_runtime_get_sync(qep->dev);
> + reg = intel_qep_readl(qep, INTEL_QEPCON);
> + if (val) {
> + /* Enable peripheral and keep runtime PM always on */
> + reg |= INTEL_QEPCON_EN;
> + pm_runtime_get_noresume(qep->dev);
> + } else {
> + /* Let runtime PM be idle and disable peripheral */
> + pm_runtime_put_noidle(qep->dev);
> + reg &= ~INTEL_QEPCON_EN;
> + }
> + intel_qep_writel(qep, INTEL_QEPCON, reg);
> + pm_runtime_put(qep->dev);
> + qep->enabled = val;
> }
> - intel_qep_writel(qep, INTEL_QEPCON, reg);
> - pm_runtime_put(qep->dev);
> - qep->enabled = val;
>
> -out:
> - mutex_unlock(&qep->lock);
> return 0;
> }
>
> @@ -279,7 +276,6 @@ static int intel_qep_spike_filter_ns_write(struct counter_device *counter,
> struct intel_qep *qep = counter_priv(counter);
> u32 reg;
> bool enable;
> - int ret = 0;
>
> /*
> * Spike filter length is (MAX_COUNT + 2) clock periods.
> @@ -300,25 +296,23 @@ static int intel_qep_spike_filter_ns_write(struct counter_device *counter,
> if (length > INTEL_QEPFLT_MAX_COUNT(length))
> return -ERANGE;
>
> - mutex_lock(&qep->lock);
> - if (qep->enabled) {
> - ret = -EBUSY;
> - goto out;
> + /* Lock mutex while execution in scoped_guard() scope */
> + scoped_guard(mutex, &qep->lock){
> + if (qep->enabled)
> + return -EBUSY;
> +
> + pm_runtime_get_sync(qep->dev);
> + reg = intel_qep_readl(qep, INTEL_QEPCON);
> + if (enable)
> + reg |= INTEL_QEPCON_FLT_EN;
> + else
> + reg &= ~INTEL_QEPCON_FLT_EN;
> + intel_qep_writel(qep, INTEL_QEPFLT, length);
> + intel_qep_writel(qep, INTEL_QEPCON, reg);
> + pm_runtime_put(qep->dev);
> }
>
> - pm_runtime_get_sync(qep->dev);
> - reg = intel_qep_readl(qep, INTEL_QEPCON);
> - if (enable)
> - reg |= INTEL_QEPCON_FLT_EN;
> - else
> - reg &= ~INTEL_QEPCON_FLT_EN;
> - intel_qep_writel(qep, INTEL_QEPFLT, length);
> - intel_qep_writel(qep, INTEL_QEPCON, reg);
> - pm_runtime_put(qep->dev);
> -
> -out:
> - mutex_unlock(&qep->lock);
> - return ret;
> + return 0;
> }
>
> static int intel_qep_preset_enable_read(struct counter_device *counter,
> @@ -342,28 +336,24 @@ static int intel_qep_preset_enable_write(struct counter_device *counter,
> {
> struct intel_qep *qep = counter_priv(counter);
> u32 reg;
> - int ret = 0;
> -
> - mutex_lock(&qep->lock);
> - if (qep->enabled) {
> - ret = -EBUSY;
> - goto out;
> - }
>
> - pm_runtime_get_sync(qep->dev);
> - reg = intel_qep_readl(qep, INTEL_QEPCON);
> - if (val)
> - reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
> - else
> - reg |= INTEL_QEPCON_COUNT_RST_MODE;
> + /* Lock mutex while execution in scoped_guard() scope */
> + scoped_guard(mutex, &qep->lock){
> + if (qep->enabled)
> + return -EBUSY;
>
> - intel_qep_writel(qep, INTEL_QEPCON, reg);
> - pm_runtime_put(qep->dev);
> + pm_runtime_get_sync(qep->dev);
> + reg = intel_qep_readl(qep, INTEL_QEPCON);
> + if (val)
> + reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
> + else
> + reg |= INTEL_QEPCON_COUNT_RST_MODE;
>
> -out:
> - mutex_unlock(&qep->lock);
> + intel_qep_writel(qep, INTEL_QEPCON, reg);
> + pm_runtime_put(qep->dev);
> + }
>
> - return ret;
> + return 0;
> }
>
> static struct counter_comp intel_qep_count_ext[] = {
>
--
i.
next prev parent reply other threads:[~2026-04-23 11:42 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-22 21:05 [PATCH] counter: intel-qep: Replace manual mutex logic with lock guards Joao Paulo Menezes Linaris
2026-04-23 11:42 ` Ilpo Järvinen [this message]
2026-04-30 22:03 ` Joao Paulo Menezes Linaris
-- strict thread matches above, loose matches on Subject: below --
2026-05-08 3:40 Joao Paulo Menezes Linaris
2026-05-08 5:18 ` Maxwell Doose
Reply instructions:
You may reply publicly 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=3ab73ef5-0a3a-66b4-a763-398f0921c289@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=guilhermeabreu200105@usp.br \
--cc=jplinaris@usp.br \
--cc=linux-iio@vger.kernel.org \
--cc=wbg@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox