soc: mediatek: mediatek-regulator-coupler: Fix comment

Commit Message

Fei Shao Oct. 23, 2024, 10:19 a.m. UTC

Fix two minor issues in the comments.

1. We balance VSRAM voltage based on the target VGPU voltage, so the
   comment likely refers to VGPU.
2. .attach_regulator() returns 0 on success and 1 if the regulator is
   not suitable. The context suggests a successful return value (0).

Fixes: c200774a6df4 ("soc: mediatek: Introduce mediatek-regulator-coupler driver")
Signed-off-by: Fei Shao <[e-mail]>
---

 drivers/soc/mediatek/mtk-regulator-coupler.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

AngeloGioacchino Del Regno Oct. 23, 2024, 10:40 a.m. UTC | #1

Il 23/10/24 12:19, Fei Shao ha scritto:
> Fix two minor issues in the comments.
> 
> 1. We balance VSRAM voltage based on the target VGPU voltage, so the
>     comment likely refers to VGPU.

Function `mediatek_regulator_balance_voltage()` refers, as stated in the comment
located at the top of its signature, to "GPU<->SRAM" voltages relationships.

So, we're taking into consideration only two regulators:
                   VGPU and VSRAM

The first comment says:
"If we're asked to set a voltage (implicit: to VGPU) less than VSRAM min_uV[...]"

...so, I think that you've misunderstood what the comment says :-)

> 2. .attach_regulator() returns 0 on success and 1 if the regulator is
>     not suitable. The context suggests a successful return value (0).

The comment is on top of a "refuse" or "error" case - one that wants to return 1
and not zero.

Besides, it clearly states:
"The regulator core will keep walking through the list of couplers when any
  .attach_regulator() callback returns 1"

...which is definitely true.

drivers/regulator/core.c
function `regulator_find_coupler()`:

	list_for_each_entry_reverse(coupler, &regulator_coupler_list, list) {
		err = coupler->attach_regulator(coupler, rdev);
		[.....]
		if (err < 0)
			return ERR_PTR(err);

		if (err == 1)
			continue;

		break;
	}

Is that clear now?

Cheers,
Angelo

> 
> Fixes: c200774a6df4 ("soc: mediatek: Introduce mediatek-regulator-coupler driver")
> Signed-off-by: Fei Shao <[e-mail]>
> ---
> 
>   drivers/soc/mediatek/mtk-regulator-coupler.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c b/drivers/soc/mediatek/mtk-regulator-coupler.c
> index 0b6a2884145e..16df12d1c2e0 100644
> --- a/drivers/soc/mediatek/mtk-regulator-coupler.c
> +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c
> @@ -74,7 +74,7 @@ static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler,
>   		return ret;
>   
>   	/*
> -	 * If we're asked to set a voltage less than VSRAM min_uV, set
> +	 * If we're asked to set a voltage less than VGPU min_uV, set
>   	 * the minimum allowed voltage on VSRAM, as in this case it is
>   	 * safe to ignore the max_spread parameter.
>   	 */
> @@ -108,7 +108,7 @@ static int mediatek_regulator_attach(struct regulator_coupler *coupler,
>   	 * this means that this is surely not a GPU<->SRAM couple: in that
>   	 * case, we may want to use another coupler implementation, if any,
>   	 * or the generic one: the regulator core will keep walking through
> -	 * the list of couplers when any .attach_regulator() cb returns 1.
> +	 * the list of couplers when any .attach_regulator() cb returns 0.
>   	 */
>   	if (rdev->coupling_desc.n_coupled > 2)
>   		return 1;

Fei Shao Oct. 23, 2024, 12:03 p.m. UTC | #2

On Wed, Oct 23, 2024 at 6:40 PM AngeloGioacchino Del Regno
<[e-mail]> wrote:
>
> Il 23/10/24 12:19, Fei Shao ha scritto:
> > Fix two minor issues in the comments.
> >
> > 1. We balance VSRAM voltage based on the target VGPU voltage, so the
> >     comment likely refers to VGPU.
>
> Function `mediatek_regulator_balance_voltage()` refers, as stated in the comment
> located at the top of its signature, to "GPU<->SRAM" voltages relationships.
>
> So, we're taking into consideration only two regulators:
>                    VGPU and VSRAM
>
> The first comment says:
> "If we're asked to set a voltage (implicit: to VGPU) less than VSRAM min_uV[...]"
>
> ...so, I think that you've misunderstood what the comment says :-)

Let me make sure we're on the same page - VGPU never goes higher than
VSRAM (VGPU <= VSRAM), is that correct?

[ min VGPU, max VGPU ] ... spread ... [    min VSRAM  ,  max VSRAM    ]
 (min_uV)  (max_uV)                         (vsram_min_uV) (vsram_max_uV)

The longer comment is
"If we're asked to set a voltage less than VSRAM min_uV, set the
minimum allowed voltage on VSRAM, ..."
So VSRAM is the subject here I think? Because we "set voltage *on*
VSRAM" based on its own minimum allowed voltage? We never set either
VGPU min/max voltage to vsram_min_uV?
And it's attached to the line that decides vsram_target_min_uV, with
the maximum of (1) vsram_min_uV (i.e. minimum allowed VSRAM voltage)
or (2) min_uV + max_spread (min VGPU + spread)

That makes me believe VGPU is the correct candidate.
We manually configure VSRAM first (push the ceiling to higher), and
let the regulator core update VGPU afterwards.

In fact, IIUC, there should be other concerns here... it should be
"vsram_target_min_uV = max(vsram_min_uV, **max_uV** +
**min_spread**)", due to the same fact that VGPU <= VSRAM.

For max_uV, it's not causing any issues because min_uV == max_uV since
there's only one consumer... I can update the code in v2 if you agree
with this.

For min_spread part, the reason is that VSRAM should be somewhat ahead
of VGPU, but not way too far, i.e. VGPU + min_spread <= VSRAM <= VGPU
+ max_spread.
This is required on MT8183 and MT8186 - the spread should be at
between 0.1V to 0.25V (VGPU + 0.1V <= VSRAM <= VGPU + 0.25V).
The problem is that we don't have a "regulator-coupled-min-spread"
property, so we use the -max-spread property for minimum spread... and
without considering the real maximum spread.
But this is probably not that terrible also, currently
vsram_target_max_uV never goes too far from VGPU.

>
> > 2. .attach_regulator() returns 0 on success and 1 if the regulator is
> >     not suitable. The context suggests a successful return value (0).
>
> The comment is on top of a "refuse" or "error" case - one that wants to return 1
> and not zero.
>
> Besides, it clearly states:
> "The regulator core will keep walking through the list of couplers when any
>   .attach_regulator() callback returns 1"
>
> ...which is definitely true.

... I'm sorry, I guess I really need a dinner break - my brain
translated "when" as "until" so I read it the opposite way.
You and the original comment are both correct.

And I hope my first point still makes sense and it's not my brain
being lacking energy again...

Regards,
Fei



>
> drivers/regulator/core.c
> function `regulator_find_coupler()`:
>
>         list_for_each_entry_reverse(coupler, &regulator_coupler_list, list) {
>                 err = coupler->attach_regulator(coupler, rdev);
>                 [.....]
>                 if (err < 0)
>                         return ERR_PTR(err);
>
>                 if (err == 1)
>                         continue;
>
>                 break;
>         }
>
> Is that clear now?
>
> Cheers,
> Angelo

Fei Shao Oct. 23, 2024, 1:47 p.m. UTC | #3

On Wed, Oct 23, 2024 at 8:03 PM Fei Shao <[e-mail]> wrote:
>
> On Wed, Oct 23, 2024 at 6:40 PM AngeloGioacchino Del Regno
> <[e-mail]> wrote:
> >
> > Il 23/10/24 12:19, Fei Shao ha scritto:
> > > Fix two minor issues in the comments.
> > >
> > > 1. We balance VSRAM voltage based on the target VGPU voltage, so the
> > >     comment likely refers to VGPU.
> >
> > Function `mediatek_regulator_balance_voltage()` refers, as stated in the comment
> > located at the top of its signature, to "GPU<->SRAM" voltages relationships.
> >
> > So, we're taking into consideration only two regulators:
> >                    VGPU and VSRAM
> >
> > The first comment says:
> > "If we're asked to set a voltage (implicit: to VGPU) less than VSRAM min_uV[...]"
> >
> > ...so, I think that you've misunderstood what the comment says :-)
>
> Let me make sure we're on the same page - VGPU never goes higher than
> VSRAM (VGPU <= VSRAM), is that correct?
>
> [ min VGPU, max VGPU ] ... spread ... [    min VSRAM  ,  max VSRAM    ]
>  (min_uV)  (max_uV)                         (vsram_min_uV) (vsram_max_uV)
>
> The longer comment is
> "If we're asked to set a voltage less than VSRAM min_uV, set the
> minimum allowed voltage on VSRAM, ..."
> So VSRAM is the subject here I think? Because we "set voltage *on*
> VSRAM" based on its own minimum allowed voltage? We never set either
> VGPU min/max voltage to vsram_min_uV?
> And it's attached to the line that decides vsram_target_min_uV, with
> the maximum of (1) vsram_min_uV (i.e. minimum allowed VSRAM voltage)
> or (2) min_uV + max_spread (min VGPU + spread)

Ah, I just got your point. Both interpretations can explain..
I linked the "min_uV" in the comment to the one used in code, which
stands for VGPU min_uV, so I had the impression that it was a typo.

Regards,
Fei

Patch

diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c b/drivers/soc/mediatek/mtk-regulator-coupler.c
index 0b6a2884145e..16df12d1c2e0 100644
--- a/drivers/soc/mediatek/mtk-regulator-coupler.c
+++ b/drivers/soc/mediatek/mtk-regulator-coupler.c
@@ -74,7 +74,7 @@  static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler,
 		return ret;
 
 	/*
-	 * If we're asked to set a voltage less than VSRAM min_uV, set
+	 * If we're asked to set a voltage less than VGPU min_uV, set
 	 * the minimum allowed voltage on VSRAM, as in this case it is
 	 * safe to ignore the max_spread parameter.
 	 */
@@ -108,7 +108,7 @@  static int mediatek_regulator_attach(struct regulator_coupler *coupler,
 	 * this means that this is surely not a GPU<->SRAM couple: in that
 	 * case, we may want to use another coupler implementation, if any,
 	 * or the generic one: the regulator core will keep walking through
-	 * the list of couplers when any .attach_regulator() cb returns 1.
+	 * the list of couplers when any .attach_regulator() cb returns 0.
 	 */
 	if (rdev->coupling_desc.n_coupled > 2)
 		return 1;