Date:   Wed, 30 Aug 2023 19:10:48 +0800
From:   Macpaul Lin <macpaul.lin@...iatek.com>
To:     Chen-Yu Tsai <wenst@...omium.org>
CC:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Fabien Parent <fparent@...libre.com>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        Bear Wang <bear.wang@...iatek.com>,
        Pablo Sun <pablo.sun@...iatek.com>,
        Macpaul Lin <macpaul@...il.com>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Alexandre Mergnat <amergnat@...libre.com>
Subject: Re: [PATCH 3/4] arm64: dts: mediatek: mt6360: add PMIC MT6360 related
 nodes



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@...iatek.com> 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@...iatek.com> 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@...iatek.com>
>> >> ---
>> >>  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@...libre.com>
>> > 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

Powered by blists - more mailing lists