Re: [PATCH 3/4] arm64: dts: mediatek: mt6360: add PMIC MT6360 related nodes

[Macpaul Lin]

On 8/28/23 18:51, Chen-Yu Tsai wrote:
> 	
>
> External email : Please do not click links or open attachments until you 
> have verified the sender or the content.
>
> On Mon, Aug 28, 2023 at 5:59 PM Macpaul Lin <macpaul.lin@xxxxxxxxxxxx> wrote:
> > On 8/28/23 12:36, Chen-Yu Tsai wrote:
> > >
> > >
> > > External email : Please do not click links or open attachments until you
> > > have verified the sender or the content.
> > >
> > > On Fri, Aug 25, 2023 at 7:46 PM Macpaul Lin <macpaul.lin@xxxxxxxxxxxx> wrote:
> > >>
> > >> MT6360 is the secondary PMIC for MT8195.
> > >> It supports USB Type-C and PD functions.
> > >> Add MT6360 related common nodes which is used for MT8195 platform, includes
> > >>  - charger
> > >>  - ADC
> > >>  - LED
> > >>  - regulators
> > >>
> > >> Signed-off-by: Macpaul Lin <macpaul.lin@xxxxxxxxxxxx>
> > >> ---
> > >>  arch/arm64/boot/dts/mediatek/mt6360.dtsi | 112 +++++++++++++++++++++++
> >
> > [snip..]
> >
> > >> +       regulator {
> > >> +               compatible = "mediatek,mt6360-regulator";
> > >> +               LDO_VIN3-supply = <&mt6360_buck2>;
> > >> +
> > >> +               mt6360_buck1: buck1 {
> > >> +                       regulator-compatible = "BUCK1";
> > >> +                       regulator-name = "mt6360,buck1";
> > >
> > > Normally there's no need to provide a default name. Any used regulator
> > > should have been named to match the power rail name from the board's
> > > schematics.
> > >
> >
> > I have 2 schematics on hand. One is mt8195-demo board and the other is
> > genio-1200-evk board. There are 2 PMIC used on these board
> > with "the same" sub power rail name for "BUCK1~BUCK4". One is mt6315,
> > and the other is mt6360.
>
> This is more of an board level integration thing. Regulator names are
> expected to be named after the actual power rail names. For example,
> take a look at Rock Pi 4 schematics [1], the power rail from BUCK1 of
> the RK808 PMIC is named "VDD_CENTER". And in the dts file [1] we can
> see the regulator is named that as well (albeit with some style changes).
>
> Now if a project really chooses meaningless names like BUCKx or LDOy
> for their power rails, then so be it. However those are board level
> decisions. The names are there to help with integration debugging, so
> it makes sense to have names that match the power rail names in the
> schematics. Default names rarely make sense.
>
> [1]https://dl.radxa.com/rockpi4/docs/hw/rockpi4/rockpi4_v13_sch_20181112.pdf  <https://urldefense.com/v3/__https://dl.radxa.com/rockpi4/docs/hw/rockpi4/rockpi4_v13_sch_20181112.pdf__;!!CTRNKA9wMg0ARbw!g4T6kWfnETA38Kc_yc6dx6gYi7zzW2m6YU0ybNN5vbTWjK5SfapEQEQMrtxg8E9xRNdpJm678Rj3uLrWHeM$>
> [2]https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi#L267  <https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi*L267__;Iw!!CTRNKA9wMg0ARbw!g4T6kWfnETA38Kc_yc6dx6gYi7zzW2m6YU0ybNN5vbTWjK5SfapEQEQMrtxg8E9xRNdpJm678Rj3hdwm0VA$>
>
> > I've also inspected other dtsi of the regulators, such as mt6357 and
> > mt6359. They have regulator nodes with named for their purpose. For the
> > schematics of mt8195-demo and genio-1200-evk boards, there are no
> > explicit usage for "BUCK1~BUCK4" for both mt6135 and mt6360. In order to
> > specify the sub power rail for mt6360, MediaTek chooses name like
> > "mt6360,buck1" instead of simple name "buck1" for distinguish with
> > "buck1" of mt6351.
> >
> > >> +                       regulator-min-microvolt = <300000>;
> > >> +                       regulator-max-microvolt = <1300000>;
> > >
> > > These values correspond to the regulator's range. They make no sense as
> > > regulator constraints. The min/max values are supposed to be the most
> > > restrictive set of voltages of the regulator consumers. If what is fed
> > > by this regulator can only take 0.7V ~ 1.1V, then it should save 0.7V
> > > and 1.1V here. If the regulator is unused, then there are no constraints,
> > > and these can be left out.
> > >
> > > Just leave them out of the file.
> > >Alexandre Mergnat <amergnat@xxxxxxxxxxxx>
> > > Both comments apply to all the regulators.
> > >
> > > ChenYu
> >
> > There are some common circuit design for these regulators like mt6359,
> > mt6360 and mt6315 used on many products. MediaTek put the most common
> > and expected default values in their dtsi. However, some changes still
> > need to be applied to derivative boards according to product's
> > requirements. The actual value be used will be applied in board's dts
> > file to override the default settings in dtsi.
>
> The values here are definitely not some product's expected values.
> They are the full range of output voltages supported, as seen in the
> driver.
>
> The regulator binding says:
>
>      regulator-min-microvolt:
>        description: smallest voltage consumers may set
>
>      regulator-max-microvolt:
>        description: largest voltage consumers may set
>
> The constraints given in the regulator node are those of the consumers,
> not the PMIC regulator itself. As you mentioned, a board may need to
> adjust the range based on its design, i.e. what the board has connected
> to the regulator.
>
> So either something is connected, and the consumer's constraints are set
> by overriding the default in the board .dts file; or, nothing is connected
> and the constraints don't matter, as nothing is going to set the voltage
> or enable the regulator. So there's no reason to give a default. For
> unused regulator outputs, their device nodes don't even have to exist.
>
> I'm trying to get people to _not_ write default values, as they don't
> make any sense. The full voltage range is already implied by the
> compatible string.
>
> ChenYu

Thanks for the explanation in detail.
I'll update the patch v2 for these modification.

Best regards,
Macpaul Lin