Re: [PATCH v2 4/4] arm64: dts: mediatek: mt8195-demo: simplify mt6360 nodes

[Macpaul Lin]

On 8/30/23 19:30, Krzysztof Kozlowski wrote:
> 	
>
> External email : Please do not click links or open attachments until you 
> have verified the sender or the content.
>
> On 30/08/2023 13:15, Macpaul Lin wrote:
> > The dts file for the MediaTek MT8195 demo board has been refactored
> > to improve the configuration of the MT6360 multi-function PMIC.
> >
> > The changes include:
> >  - Addition of the mt6360.dtsi include file, which contains the common
> >    configuration of the MT6360 for all mt8195 boards.
> >  - Removal of the direct inclusion of the mt6360-regulator.h file.
> >  - Removal of the common configuration of the MT6360 PMIC since
> >    the included mt6360.dtsi is used.
> >  - Add names according to the schematic of mt8195-demo board for
> >    mt6360 nodes.
> >
> > Signed-off-by: Macpaul Lin <macpaul.lin@xxxxxxxxxxxx>
> > ---
> >  arch/arm64/boot/dts/mediatek/mt8195-demo.dts | 173 ++++++++-----------
> >  1 file changed, 74 insertions(+), 99 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8195-demo.dts b/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
> > index 8aea6f5d72b3..d082d679dbbe 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8195-demo.dts
> > @@ -11,7 +11,6 @@
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/input/input.h>
> >  #include <dt-bindings/pinctrl/mt8195-pinfunc.h>
> > -#include <dt-bindings/regulator/mediatek,mt6360-regulator.h>
> >  
> >  / {
> >  model = "MediaTek MT8195 demo board";
> > @@ -130,103 +129,9 @@
> >  mt6360: pmic@34 {
> >  compatible = "mediatek,mt6360";
> >  reg = <0x34>;
> > -interrupt-controller;
> > +pinctrl-0 = <&mt6360_pins>;
> > +pinctrl-names = "default";
> >  interrupts-extended = <&pio 101 IRQ_TYPE_EDGE_FALLING>;
> > -interrupt-names = "IRQB";
> > -
> > -charger {
> > -compatible = "mediatek,mt6360-chg";
> > -richtek,vinovp-microvolt = <14500000>;
> > -
> > -otg_vbus_regulator: usb-otg-vbus-regulator {
> > -regulator-compatible = "usb-otg-vbus";
> > -regulator-name = "usb-otg-vbus";
> > -regulator-min-microvolt = <4425000>;
> > -regulator-max-microvolt = <5825000>;
> > -};
> > -};
> > -
> > -regulator {
> > -compatible = "mediatek,mt6360-regulator";
> > -LDO_VIN3-supply = <&mt6360_buck2>;
> > -
> > -mt6360_buck1: buck1 {
>
> Your patchset does not look bisectable. Does not look tested, either...
> Why having duplicated regulators (?) and then removing correct
> regulators and keeping wrong ones?
>
> > -regulator-compatible = "BUCK1";
> > -regulator-name = "mt6360,buck1";
> > -regulator-min-microvolt = <300000>;
> > -regulator-max-microvolt = <1300000>;
> > -regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> > -   MT6360_OPMODE_LP
> > -   MT6360_OPMODE_ULP>;
> > -regulator-always-on;
> > -};
>
> ...
>
> >  };
> >  };
> >  
> > @@ -259,8 +164,8 @@
> >  cap-sd-highspeed;
> >  sd-uhs-sdr50;
> >  sd-uhs-sdr104;
> > -vmmc-supply = <&mt6360_ldo5>;
> > -vqmmc-supply = <&mt6360_ldo3>;
> > +vmmc-supply = <&mt6360_vmch_pmu_ldo5_reg>;
> > +vqmmc-supply = <&mt6360_vmc_pmu_ldo3_reg>;
> >  status = "okay";
> >  };
> >  
> > @@ -300,6 +205,67 @@
> >  regulator-always-on;
> >  };
> >  
> > +#include "mt6360.dtsi"
>
> Includes are in the top part of the DTS. Sometimes bottom, but never in
> the middle.
>
>
> Best regards,
> Krzysztof

MT6360 is an external component controlled by I2C.
On some mt8195 boards, it is connected to I2C6.
But on some smart phone boards, they are connected to I2C5.
I agree to put the includes in the top or in the bottom,
but it to include I2C node together in mt6360.dtsi will be necessary
to avoid build error. It might introduce mt6360-i2c5.dtsi
or mt6360-i2c6.dtsi in the future.

I think the better approach is to expand the common nodes in the board's 
dts, rather than organizing them into dtsi.

Please drop these 2 patches for adding mt6360.dtsi and modification for 
mt8195-demo.dts (patch 3/4 and 4/4) for the patch set.

Thanks for the review. :)
Macpaul Lin