All of lore.kernel.org
 help / color / mirror / Atom feed
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Jian Hu <jian.hu@amlogic.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Rob Herring <robh@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	Victor Wan <victor.wan@amlogic.com>,
	Chandle Zou <chandle.zou@amlogic.com>,
	linux-clk@vger.kernel.org, linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops
Date: Fri, 27 Dec 2019 17:53:41 +0100	[thread overview]
Message-ID: <CAFBinCC4Fgn3QQ6H-TWO_Xx+USonzMDZDyvJBfYp-_6=pmKdLQ@mail.gmail.com> (raw)
In-Reply-To: <20191227094606.143637-3-jian.hu@amlogic.com>

Hi Jian,

On Fri, Dec 27, 2019 at 10:46 AM Jian Hu <jian.hu@amlogic.com> wrote:
[...]
> @@ -294,9 +298,12 @@ static int meson_clk_pll_is_enabled(struct clk_hw *hw)
>  {
>         struct clk_regmap *clk = to_clk_regmap(hw);
>         struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> +       int ret = 0;
>
> -       if (meson_parm_read(clk->map, &pll->rst) ||
> -           !meson_parm_read(clk->map, &pll->en) ||
> +       if (MESON_PARM_APPLICABLE(&pll->rst))
> +               ret = meson_parm_read(clk->map, &pll->rst);
> +
> +       if (ret || !meson_parm_read(clk->map, &pll->en) ||
>             !meson_parm_read(clk->map, &pll->l))
>                 return 0;
I had to read this part twice to understand what it's doing because I
misunderstood what "ret" is used for (I thought that some "return ret"
is missing)
my proposal to make it easier to read:
...
if (MESON_PARM_APPLICABLE(&pll->rst) &&
    meson_parm_read(clk->map, &pll->rst))
  return 0;

if (!meson_parm_read(clk->map, &pll->en) ||
    !meson_parm_read(clk->map, &pll->l))
                 return 0;
...

please let me know what you think about this

> @@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>         /* do nothing if the PLL is already enabled */
>         if (clk_hw_is_enabled(hw))
>                 return 0;
> +       /*
> +        * Compared with the previous SoCs, self-adaption module current
> +        * is newly added for A1, keep the new power-on sequence to enable the
> +        * PLL.
> +        */
> +       if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> +               /* Enable the pll */
> +               meson_parm_write(clk->map, &pll->en, 1);
> +               udelay(10);
> +               /* Enable the pll self-adaption module current */
> +               meson_parm_write(clk->map, &pll->current_en, 1);
> +               udelay(40);
> +               /* Enable lock detect module */
> +               meson_parm_write(clk->map, &pll->l_detect, 1);
> +               meson_parm_write(clk->map, &pll->l_detect, 0);
> +               goto out;
> +       }
in all other functions you are skipping the pll->rst register by
checking for MESON_PARM_APPLICABLE(&pll->rst)
I like that because it's a pattern which is easy to follow

do you think we can make this part consistent with that?
I'm thinking of something like this (not compile-tested and I dropped
all comments, just so you get the idea):
...
if (MESON_PARM_APPLICABLE(&pll->rst)
  meson_parm_write(clk->map, &pll->rst, 1);

meson_parm_write(clk->map, &pll->en, 1);

if (MESON_PARM_APPLICABLE(&pll->rst))
  meson_parm_write(clk->map, &pll->rst, 0);

if (MESON_PARM_APPLICABLE(&pll->current_en))
  meson_parm_write(clk->map, &pll->current_en, 1);

if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
  meson_parm_write(clk->map, &pll->l_detect, 1);
  meson_parm_write(clk->map, &pll->l_detect, 0);
}

if (meson_clk_pll_wait_lock(hw))
...

I see two (and a half) benefits here:
- if there's a PLL with neither the pll->current_en nor the pll->rst
registers then you get support for this implementation for free
- the if (MESON_PARM_APPLICABLE(...)) pattern is already used in the
driver, but only for one register (in your example when
MESON_PARM_APPLICABLE(&pll->current_en) exists you also modify the
pll->l_detect register, which I did not expect)
- only counts half: no use of "goto", which in my opinion makes it
very easy to read (just read from top to bottom, checking each "if")


Martin

WARNING: multiple messages have this Message-ID
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Jian Hu <jian.hu@amlogic.com>
Cc: Rob Herring <robh@kernel.org>,
	Victor Wan <victor.wan@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Chandle Zou <chandle.zou@amlogic.com>,
	linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops
Date: Fri, 27 Dec 2019 17:53:41 +0100	[thread overview]
Message-ID: <CAFBinCC4Fgn3QQ6H-TWO_Xx+USonzMDZDyvJBfYp-_6=pmKdLQ@mail.gmail.com> (raw)
In-Reply-To: <20191227094606.143637-3-jian.hu@amlogic.com>

Hi Jian,

On Fri, Dec 27, 2019 at 10:46 AM Jian Hu <jian.hu@amlogic.com> wrote:
[...]
> @@ -294,9 +298,12 @@ static int meson_clk_pll_is_enabled(struct clk_hw *hw)
>  {
>         struct clk_regmap *clk = to_clk_regmap(hw);
>         struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> +       int ret = 0;
>
> -       if (meson_parm_read(clk->map, &pll->rst) ||
> -           !meson_parm_read(clk->map, &pll->en) ||
> +       if (MESON_PARM_APPLICABLE(&pll->rst))
> +               ret = meson_parm_read(clk->map, &pll->rst);
> +
> +       if (ret || !meson_parm_read(clk->map, &pll->en) ||
>             !meson_parm_read(clk->map, &pll->l))
>                 return 0;
I had to read this part twice to understand what it's doing because I
misunderstood what "ret" is used for (I thought that some "return ret"
is missing)
my proposal to make it easier to read:
...
if (MESON_PARM_APPLICABLE(&pll->rst) &&
    meson_parm_read(clk->map, &pll->rst))
  return 0;

if (!meson_parm_read(clk->map, &pll->en) ||
    !meson_parm_read(clk->map, &pll->l))
                 return 0;
...

please let me know what you think about this

> @@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>         /* do nothing if the PLL is already enabled */
>         if (clk_hw_is_enabled(hw))
>                 return 0;
> +       /*
> +        * Compared with the previous SoCs, self-adaption module current
> +        * is newly added for A1, keep the new power-on sequence to enable the
> +        * PLL.
> +        */
> +       if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> +               /* Enable the pll */
> +               meson_parm_write(clk->map, &pll->en, 1);
> +               udelay(10);
> +               /* Enable the pll self-adaption module current */
> +               meson_parm_write(clk->map, &pll->current_en, 1);
> +               udelay(40);
> +               /* Enable lock detect module */
> +               meson_parm_write(clk->map, &pll->l_detect, 1);
> +               meson_parm_write(clk->map, &pll->l_detect, 0);
> +               goto out;
> +       }
in all other functions you are skipping the pll->rst register by
checking for MESON_PARM_APPLICABLE(&pll->rst)
I like that because it's a pattern which is easy to follow

do you think we can make this part consistent with that?
I'm thinking of something like this (not compile-tested and I dropped
all comments, just so you get the idea):
...
if (MESON_PARM_APPLICABLE(&pll->rst)
  meson_parm_write(clk->map, &pll->rst, 1);

meson_parm_write(clk->map, &pll->en, 1);

if (MESON_PARM_APPLICABLE(&pll->rst))
  meson_parm_write(clk->map, &pll->rst, 0);

if (MESON_PARM_APPLICABLE(&pll->current_en))
  meson_parm_write(clk->map, &pll->current_en, 1);

if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
  meson_parm_write(clk->map, &pll->l_detect, 1);
  meson_parm_write(clk->map, &pll->l_detect, 0);
}

if (meson_clk_pll_wait_lock(hw))
...

I see two (and a half) benefits here:
- if there's a PLL with neither the pll->current_en nor the pll->rst
registers then you get support for this implementation for free
- the if (MESON_PARM_APPLICABLE(...)) pattern is already used in the
driver, but only for one register (in your example when
MESON_PARM_APPLICABLE(&pll->current_en) exists you also modify the
pll->l_detect register, which I did not expect)
- only counts half: no use of "goto", which in my opinion makes it
very easy to read (just read from top to bottom, checking each "if")


Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Jian Hu <jian.hu@amlogic.com>
Cc: Rob Herring <robh@kernel.org>,
	Victor Wan <victor.wan@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Chandle Zou <chandle.zou@amlogic.com>,
	linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops
Date: Fri, 27 Dec 2019 17:53:41 +0100	[thread overview]
Message-ID: <CAFBinCC4Fgn3QQ6H-TWO_Xx+USonzMDZDyvJBfYp-_6=pmKdLQ@mail.gmail.com> (raw)
In-Reply-To: <20191227094606.143637-3-jian.hu@amlogic.com>

Hi Jian,

On Fri, Dec 27, 2019 at 10:46 AM Jian Hu <jian.hu@amlogic.com> wrote:
[...]
> @@ -294,9 +298,12 @@ static int meson_clk_pll_is_enabled(struct clk_hw *hw)
>  {
>         struct clk_regmap *clk = to_clk_regmap(hw);
>         struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> +       int ret = 0;
>
> -       if (meson_parm_read(clk->map, &pll->rst) ||
> -           !meson_parm_read(clk->map, &pll->en) ||
> +       if (MESON_PARM_APPLICABLE(&pll->rst))
> +               ret = meson_parm_read(clk->map, &pll->rst);
> +
> +       if (ret || !meson_parm_read(clk->map, &pll->en) ||
>             !meson_parm_read(clk->map, &pll->l))
>                 return 0;
I had to read this part twice to understand what it's doing because I
misunderstood what "ret" is used for (I thought that some "return ret"
is missing)
my proposal to make it easier to read:
...
if (MESON_PARM_APPLICABLE(&pll->rst) &&
    meson_parm_read(clk->map, &pll->rst))
  return 0;

if (!meson_parm_read(clk->map, &pll->en) ||
    !meson_parm_read(clk->map, &pll->l))
                 return 0;
...

please let me know what you think about this

> @@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>         /* do nothing if the PLL is already enabled */
>         if (clk_hw_is_enabled(hw))
>                 return 0;
> +       /*
> +        * Compared with the previous SoCs, self-adaption module current
> +        * is newly added for A1, keep the new power-on sequence to enable the
> +        * PLL.
> +        */
> +       if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> +               /* Enable the pll */
> +               meson_parm_write(clk->map, &pll->en, 1);
> +               udelay(10);
> +               /* Enable the pll self-adaption module current */
> +               meson_parm_write(clk->map, &pll->current_en, 1);
> +               udelay(40);
> +               /* Enable lock detect module */
> +               meson_parm_write(clk->map, &pll->l_detect, 1);
> +               meson_parm_write(clk->map, &pll->l_detect, 0);
> +               goto out;
> +       }
in all other functions you are skipping the pll->rst register by
checking for MESON_PARM_APPLICABLE(&pll->rst)
I like that because it's a pattern which is easy to follow

do you think we can make this part consistent with that?
I'm thinking of something like this (not compile-tested and I dropped
all comments, just so you get the idea):
...
if (MESON_PARM_APPLICABLE(&pll->rst)
  meson_parm_write(clk->map, &pll->rst, 1);

meson_parm_write(clk->map, &pll->en, 1);

if (MESON_PARM_APPLICABLE(&pll->rst))
  meson_parm_write(clk->map, &pll->rst, 0);

if (MESON_PARM_APPLICABLE(&pll->current_en))
  meson_parm_write(clk->map, &pll->current_en, 1);

if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
  meson_parm_write(clk->map, &pll->l_detect, 1);
  meson_parm_write(clk->map, &pll->l_detect, 0);
}

if (meson_clk_pll_wait_lock(hw))
...

I see two (and a half) benefits here:
- if there's a PLL with neither the pll->current_en nor the pll->rst
registers then you get support for this implementation for free
- the if (MESON_PARM_APPLICABLE(...)) pattern is already used in the
driver, but only for one register (in your example when
MESON_PARM_APPLICABLE(&pll->current_en) exists you also modify the
pll->l_detect register, which I did not expect)
- only counts half: no use of "goto", which in my opinion makes it
very easy to read (just read from top to bottom, checking each "if")


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

  reply	other threads:[~2019-12-27 16:53 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-27  9:46 [PATCH v5 0/5] add Amlogic A1 clock controller driver Jian Hu
2019-12-27  9:46 ` Jian Hu
2019-12-27  9:46 ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2020-01-03 22:29   ` Rob Herring
2020-01-03 22:29     ` Rob Herring
2020-01-03 22:29     ` Rob Herring
2020-01-10 15:36   ` Jerome Brunet
2020-01-10 15:36     ` Jerome Brunet
2020-01-10 15:36     ` Jerome Brunet
2020-01-16  5:50     ` Jian Hu
2020-01-16  5:50       ` Jian Hu
2020-01-16  5:50       ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27 16:53   ` Martin Blumenstingl [this message]
2019-12-27 16:53     ` Martin Blumenstingl
2019-12-27 16:53     ` Martin Blumenstingl
2020-01-09  6:55     ` Jian Hu
2020-01-09  6:55       ` Jian Hu
2020-01-09  6:55       ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 3/5] clk: meson: a1: add support for Amlogic A1 PLL clock driver Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27 17:04   ` Martin Blumenstingl
2019-12-27 17:04     ` Martin Blumenstingl
2019-12-27 17:04     ` Martin Blumenstingl
2020-01-09  7:55     ` Jian Hu
2020-01-09  7:55       ` Jian Hu
2020-01-09  7:55       ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 4/5] dt-bindings: clock: meson: add A1 peripheral clock controller bindings Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2020-01-10 15:38   ` Jerome Brunet
2020-01-10 15:38     ` Jerome Brunet
2020-01-10 15:38     ` Jerome Brunet
2020-01-16  6:19     ` Jian Hu
2020-01-16  6:19       ` Jian Hu
2020-01-16  6:19       ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 5/5] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27 17:22   ` Martin Blumenstingl
2019-12-27 17:22     ` Martin Blumenstingl
2019-12-27 17:22     ` Martin Blumenstingl
2020-01-09 11:11     ` Jian Hu
2020-01-09 11:11       ` Jian Hu
2020-01-09 11:11       ` Jian Hu

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='CAFBinCC4Fgn3QQ6H-TWO_Xx+USonzMDZDyvJBfYp-_6=pmKdLQ@mail.gmail.com' \
    --to=martin.blumenstingl@googlemail.com \
    --cc=chandle.zou@amlogic.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jbrunet@baylibre.com \
    --cc=jian.hu@amlogic.com \
    --cc=jianxin.pan@amlogic.com \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=narmstrong@baylibre.com \
    --cc=qiufang.dai@amlogic.com \
    --cc=robh@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=victor.wan@amlogic.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.