Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190214013042.254790-1-mka@chromium.org> <20190214013042.254790-4-mka@chromium.org>
In-Reply-To: <20190214013042.254790-4-mka@chromium.org>
Reply-To: cwchoi00@gmail.com
From:   Chanwoo Choi <cwchoi00@gmail.com>
Date:   Thu, 14 Feb 2019 23:12:55 +0900
Message-ID: <CAGTfZH0cQhBLDx91VArMQ52z+iJAsfb2Zvk5nJSCqSM=aPhK0A@mail.gmail.com>
Subject: Re: [PATCH 3/4] PM / devfreq: Add devfreq_governor_start/stop()
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     MyungJoo Ham <myungjoo.ham@samsung.com>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Linux PM list <linux-pm@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-tegra@vger.kernel.org,
        Lukasz Luba <l.luba@partner.samsung.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Matthias,

Looks good to me for making the function to remove the duplicate code.
But,  When I just tested the kernel build, following warnings occur
about devfreq_governor_stop().

In file included from ./include/linux/devfreq.h:16:0,
                 from drivers/devfreq/devfreq.c:23:
drivers/devfreq/devfreq.c: In function =E2=80=98devfreq_governor_stop=E2=80=
=99:
drivers/devfreq/devfreq.c:619:17: warning: format =E2=80=98%s=E2=80=99 expe=
cts
argument of type =E2=80=98char *=E2=80=99, but argument 4 has type =E2=80=
=98int=E2=80=99 [-Wformat=3D]
   dev_warn(dev, "%s: Governor %s not stopped: %d\n",
                 ^
./include/linux/device.h:1380:22: note: in definition of macro =E2=80=98dev=
_fmt=E2=80=99
 #define dev_fmt(fmt) fmt
                      ^
drivers/devfreq/devfreq.c:619:3: note: in expansion of macro =E2=80=98dev_w=
arn=E2=80=99
   dev_warn(dev, "%s: Governor %s not stopped: %d\n",
   ^
drivers/devfreq/devfreq.c:619:17: warning: format =E2=80=98%d=E2=80=99 expe=
cts a
matching =E2=80=98int=E2=80=99 argument [-Wformat=3D]
   dev_warn(dev, "%s: Governor %s not stopped: %d\n",
                 ^
./include/linux/device.h:1380:22: note: in definition of macro =E2=80=98dev=
_fmt=E2=80=99
 #define dev_fmt(fmt) fmt
                      ^
drivers/devfreq/devfreq.c:619:3: note: in expansion of macro =E2=80=98dev_w=
arn=E2=80=99
   dev_warn(dev, "%s: Governor %s not stopped: %d\n",

2019=EB=85=84 2=EC=9B=94 14=EC=9D=BC (=EB=AA=A9) =EC=98=A4=ED=9B=84 7:16, M=
atthias Kaehlcke <mka@chromium.org>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
>
> The following pattern is repeated multiple times:
>
> - call governor event handler with DEVFREQ_GOV_START/STOP
> - check return value
> - in case of error log a message
>
> Add devfreq_governor_start/stop() helper functions with this code
> and call them instead.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/devfreq/devfreq.c | 96 +++++++++++++++++++++++----------------
>  1 file changed, 58 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7fab6c4cf719b..eb86461648d74 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -580,6 +580,53 @@ static int devfreq_notifier_call(struct notifier_blo=
ck *nb, unsigned long type,
>         return ret;
>  }
>
> +/**
> + * devfreq_governor_start() - Start the devfreq governor of the device.
> + * @devfreq:   the devfreq instance
> + */
> +static int devfreq_governor_start(struct devfreq *devfreq)
> +{
> +       struct device *dev =3D devfreq->dev.parent;
> +       int err;
> +
> +       if (WARN(mutex_is_locked(&devfreq->lock),
> +                "mutex must *not* be held by the caller\n"))
> +               return -EINVAL;
> +
> +       err =3D devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_STA=
RT,
> +                                              NULL);
> +       if (err) {
> +               dev_err(dev, "Governor %s not started: %d\n",
> +                       devfreq->governor->name, err);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * devfreq_governor_stop() - Stop the devfreq governor of the device.
> + * @devfreq:   the devfreq instance
> + */
> +static int devfreq_governor_stop(struct devfreq *devfreq)
> +{
> +       struct device *dev =3D devfreq->dev.parent;
> +       int err;
> +
> +       if (WARN(mutex_is_locked(&devfreq->lock),
> +                "mutex must *not* be held by the caller\n"))
> +               return -EINVAL;
> +
> +       err =3D devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_STO=
P, NULL);
> +       if (err) {
> +               dev_warn(dev, "%s: Governor %s not stopped: %d\n",
> +                        devfreq->governor->name, err);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
>  /**
>   * devfreq_dev_release() - Callback for struct device to release the dev=
ice.
>   * @dev:       the devfreq device
> @@ -720,13 +767,9 @@ struct devfreq *devfreq_add_device(struct device *de=
v,
>         }
>
>         devfreq->governor =3D governor;
> -       err =3D devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_STA=
RT,
> -                                               NULL);
> -       if (err) {
> -               dev_err(dev, "%s: Unable to start governor for the device=
\n",
> -                       __func__);
> +       err =3D devfreq_governor_start(devfreq);
> +       if (err)
>                 goto err_init;
> -       }
>
>         list_add(&devfreq->node, &devfreq_list);
>
> @@ -1029,24 +1072,13 @@ int devfreq_add_governor(struct devfreq_governor =
*governor)
>                                 dev_warn(dev,
>                                          "%s: Governor %s already present=
\n",
>                                          __func__, devfreq->governor->nam=
e);
> -                               ret =3D devfreq->governor->event_handler(=
devfreq,
> -                                                       DEVFREQ_GOV_STOP,=
 NULL);
> -                               if (ret) {
> -                                       dev_warn(dev,
> -                                                "%s: Governor %s stop =
=3D %d\n",
> -                                                __func__,
> -                                                devfreq->governor->name,=
 ret);
> -                               }
> +                               ret =3D devfreq_governor_stop(devfreq);
> +
>                                 /* Fall through */
>                         }
> +
>                         devfreq->governor =3D governor;
> -                       ret =3D devfreq->governor->event_handler(devfreq,
> -                                               DEVFREQ_GOV_START, NULL);
> -                       if (ret) {
> -                               dev_warn(dev, "%s: Governor %s start=3D%d=
\n",
> -                                        __func__, devfreq->governor->nam=
e,
> -                                        ret);
> -                       }
> +                       devfreq_governor_start(devfreq);
>                 }
>         }
>
> @@ -1081,7 +1113,6 @@ int devfreq_remove_governor(struct devfreq_governor=
 *governor)
>                 goto err_out;
>         }
>         list_for_each_entry(devfreq, &devfreq_list, node) {
> -               int ret;
>                 struct device *dev =3D devfreq->dev.parent;
>
>                 if (!strncmp(devfreq->governor_name, governor->name,
> @@ -1093,13 +1124,8 @@ int devfreq_remove_governor(struct devfreq_governo=
r *governor)
>                                 continue;
>                                 /* Fall through */
>                         }
> -                       ret =3D devfreq->governor->event_handler(devfreq,
> -                                               DEVFREQ_GOV_STOP, NULL);
> -                       if (ret) {
> -                               dev_warn(dev, "%s: Governor %s stop=3D%d\=
n",
> -                                        __func__, devfreq->governor->nam=
e,
> -                                        ret);
> -                       }
> +
> +                       devfreq_governor_stop(devfreq);
>                         devfreq->governor =3D NULL;
>                 }
>         }
> @@ -1149,19 +1175,13 @@ static ssize_t governor_store(struct device *dev,=
 struct device_attribute *attr,
>         }
>
>         if (df->governor) {
> -               ret =3D df->governor->event_handler(df, DEVFREQ_GOV_STOP,=
 NULL);
> -               if (ret) {
> -                       dev_warn(dev, "%s: Governor %s not stopped(%d)\n"=
,
> -                                __func__, df->governor->name, ret);
> +               ret =3D devfreq_governor_stop(df);
> +               if (ret)
>                         goto out;
> -               }
>         }
>         df->governor =3D governor;
>         strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
> -       ret =3D df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> -       if (ret)
> -               dev_warn(dev, "%s: Governor %s not started(%d)\n",
> -                        __func__, df->governor->name, ret);
> +       ret =3D devfreq_governor_start(df);
>  out:
>         mutex_unlock(&devfreq_list_lock);
>
> --
> 2.20.1.791.gb4d0f1c61a-goog
>


--=20
Best Regards,
Chanwoo Choi