kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Walter Harms <wharms@bfs.de>,
	Colin King <colin.king@canonical.com>,
	Sebastian Reichel <sre@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>
Cc: "kernel-janitors@vger.kernel.org"
	<kernel-janitors@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: AW: [PATCH] power: supply: axp288_fuel_gauge: remove redundant continue statement
Date: Fri, 18 Jun 2021 15:53:22 +0200	[thread overview]
Message-ID: <bac7f313-6696-7973-4261-27bf59055425@redhat.com> (raw)
In-Reply-To: <5d5dad5246f442e5aa96bdc50ac4b1f1@bfs.de>

Hi,

On 6/18/21 1:19 PM, Walter Harms wrote:
> Just a remark:
> the function fuel_gauge_reg_readb() is reporting via dev_err().
> But some callers are reporting also. Maybe someone should take
> a look.
> The valid return seems >=0 so removing the dev_err seems an option.

Actually the whole register reading code in this driver needs to
be reworked.

The AXP288 PMIC also controls voltage-planes which are used by
the CPU-cores and the i915 GPU which are part of the Intel SoCs
with which this PMIC is used.

This means that the PMU of the SoC needs to also talk to it
over the same I2C bus when CPU-cores / the GPU changes C-states
or ramp up/down their frequency.

To avoid conflicts there is a special handshake with both the
PMU itself (taking something resembling a mutex by a hw-handshake)
as well as with the i915 driver. This handshake is hidden inside
the i2c-adapter driver, so you don't see it in the code here.
This handshake is also the whole reason why the regmap_read may
return -EBUSY.

This handshake is quite expensive and currently it is done for
every single regmap_read (the handshake is many times as
expensive as the actual I2C read) and updating the fuel-gauge
status does quite a lot of reads.

A while ago I changed the underlying code so that AXP288
drivers can acquire access to the bus once; then do a bunch
of regmap accesses and then release the bus again.

A user who was having some stability issues has been working
(offlist) on a patch to use a register cache which gets updated
periodically (like how many hwmon drivers work) and then have
all the psy property accesses come from the cache. This allows
acquiring the bus once; do all the reads to fill the cache;
and then release it again.  I need to review his code; but
I've not gotten around to that yet (I really need to make
time for this).

Once we switch to this register-cache approach, then the whole
fuel_gauge_reg_readb() wrapper can go away since then we no
longer need to worry about EBUSY errors (once we have acquired
the bus these cannot happen).

TL;DR: you are right that there are some cleanups possible here,
but the entire thing is going to be rewritten soon, so it is
probably best to just leave it as is for now.

Regards,

Hans












> 
> jm2c,
>  re,
>  wh
> ________________________________________
> Von: Colin King <colin.king@canonical.com>
> Gesendet: Freitag, 18. Juni 2021 11:29:24
> An: Sebastian Reichel; Hans de Goede; Chen-Yu Tsai; linux-pm@vger.kernel.org
> Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> Betreff: [PATCH] power: supply: axp288_fuel_gauge: remove redundant continue statement
> 
> WARNUNG: Diese E-Mail kam von außerhalb der Organisation. Klicken Sie nicht auf Links oder öffnen Sie keine Anhänge, es sei denn, Sie kennen den/die Absender*in und wissen, dass der Inhalt sicher ist.
> 
> 
> From: Colin Ian King <colin.king@canonical.com>
> 
> The continue statement at the end of a for-loop has no effect,
> invert the if expression and remove the continue.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/power/supply/axp288_fuel_gauge.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
> index 39e16ecb7638..20e63609ab47 100644
> --- a/drivers/power/supply/axp288_fuel_gauge.c
> +++ b/drivers/power/supply/axp288_fuel_gauge.c
> @@ -142,9 +142,7 @@ static int fuel_gauge_reg_readb(struct axp288_fg_info *info, int reg)
> 
>         for (i = 0; i < NR_RETRY_CNT; i++) {
>                 ret = regmap_read(info->regmap, reg, &val);
> -               if (ret == -EBUSY)
> -                       continue;
> -               else
> +               if (ret != -EBUSY)
>                         break;
>         }
> 
> --
> 2.31.1
> 


  reply	other threads:[~2021-06-18 13:53 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-18  9:29 [PATCH] power: supply: axp288_fuel_gauge: remove redundant continue statement Colin King
2021-06-18 11:19 ` AW: " Walter Harms
2021-06-18 13:53   ` Hans de Goede [this message]
2021-06-18 13:36 ` Hans de Goede

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=bac7f313-6696-7973-4261-27bf59055425@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=colin.king@canonical.com \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=sre@kernel.org \
    --cc=wens@csie.org \
    --cc=wharms@bfs.de \
    /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;
as well as URLs for NNTP newsgroup(s).