[PATCH v3 4/4] arm64: dts: rockchip: Add devicetree for Pine64 PineTab2

[Ondřej Jirman]

On Fri, Jan 05, 2024 at 05:11:03PM +0100, Manuel Traut wrote:
> On Wed, Jan 03, 2024 at 10:42:54AM +0100, Ondřej Jirman wrote:
> > Hello Manuel,
> > 
> > a few more things I noticed:
> > 
> > On Tue, Jan 02, 2024 at 05:15:47PM +0100, Manuel Traut wrote:
> > > From: Alexander Warnecke <awarnecke002 at hotmail.com>
> > > 
> > > +	leds {
> > > +		compatible = "gpio-leds";
> > > +
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&flash_led_en_h>;
> > > +
> > > +		led-0 {
> > > +			gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
> > > +			color = <LED_COLOR_ID_WHITE>;
> > > +			function = LED_FUNCTION_FLASH;
> > > +		};
> > 
> > This LED is supplied by VCC5V_MIDU, so maybe this should be a regulator-led
> > supplied by gpio (FLASH_LED_EN_H) controlled regulator-fixed named f_led which
> > is in turn supplied by VCC5V_MIDU.
> > 
> > https://megous.com/dl/tmp/9bf0d85d78946b5e.png
> 
> regulator-leds are controlled by turning on or off the regulator. However
> VCC5V_MIDU is also used by other devices (USB, HDMI, ..) so I guess this is
> not what we want. I would keep it as is.

It's used by the LED. gpio-leds will not ensure it's on when you enable the LED.

In practice this may only come up if someone tries to save power by unloading
dwc3 USB driver, when using PT2 outside of the keyboard case. Otherwise
VCC5V_MIDU will be enabled by DWC3 driver's use of PHY API.

In any case, I'm not saying you should use VCC5V_MIDU directly in regulator-led,
but as a vin-supply to a new regulator-fixed node (which would be describing
this "fixed voltage regulator" https://megous.com/dl/tmp/cc65ec81ab9af163.png ).

> > > +	};
> > > +
> > > [...]
> > >
> > > +
> > > +	speaker_amp: speaker-amplifier {
> > > +		compatible = "simple-audio-amplifier";
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&spk_ctl>;
> > > +		enable-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_HIGH>;
> > > +		sound-name-prefix = "Speaker Amplifier";
> > > +		VCC-supply = <&vcc_bat>;
> > > +	};
> > > +
> > > +	vcc_3v3: vcc-3v3 {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vcc_3v3";
> > > +		regulator-always-on;
> > > +		regulator-boot-on;
> > > +		regulator-min-microvolt = <3300000>;
> > > +		regulator-max-microvolt = <3300000>;
> > > +		vin-supply = <&vcc3v3_sys>;
> > > +	};
> > > +
> > > +	vcc3v3_minipcie: vcc3v3-minipcie {
> > > +		compatible = "regulator-fixed";
> > > +		enable-active-high;
> > > +		gpio = <&gpio4 RK_PC3 GPIO_ACTIVE_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pcie_pwren_h>;
> > > +		regulator-name = "vcc3v3_minipcie";
> > > +		regulator-min-microvolt = <3300000>;
> > > +		regulator-max-microvolt = <3300000>;
> > > +		vin-supply = <&vcc_sys>;
> > 
> > This regulator is supplied by vcc_bat: https://megous.com/dl/tmp/4ec71a4a2aea9498.png
> 
> correct, I will update this in v4.
> 
> > > +	};
> > > +
> > > +	vcc3v3_sd: vcc3v3-sd {
> > > +		compatible = "regulator-fixed";
> > > +		gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&sdmmc_pwren_l>;
> > > +		regulator-name = "vcc3v3_sd";
> > > +		regulator-min-microvolt = <3300000>;
> > > +		regulator-max-microvolt = <3300000>;
> > > +		vin-supply = <&vcc3v3_sys>;
> > > +	};
> > > +
> > > +	vcc5v0_usb_host0: vcc5v0-usb-host0 {
> > > +		compatible = "regulator-fixed";
> > > +		enable-active-high;
> > > +		gpio = <&gpio4 RK_PC4 GPIO_ACTIVE_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&usb_host_pwren1_h>;
> > > +		regulator-name = "vcc5v0_usb_host0";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +		vin-supply = <&vcc5v_midu>;
> > > +	};
> > > +
> > > +	vcc5v0_usb_host2: vcc5v0-usb-host2 {
> > > +		compatible = "regulator-fixed";
> > > +		enable-active-high;
> > > +		gpio = <&gpio4 RK_PC5 GPIO_ACTIVE_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&usb_host_pwren2_h>;
> > > +		regulator-name = "vcc5v0_usb_host2";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +		vin-supply = <&vcc5v_midu>;
> > > +	};
> > > +
> > > +	vcc_bat: vcc-bat {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vcc_bat";
> > > +		regulator-always-on;
> > > +		regulator-boot-on;
> > > +	};
> > > +
> > > +	vcc_sys: vcc-sys {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vcc_sys";
> > > +		regulator-always-on;
> > > +		regulator-boot-on;
> > > +		vin-supply = <&vcc_bat>;
> > > +	};
> > > +
> > > +	vdd1v2_dvp: vdd1v2-dvp {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vdd1v2_dvp";
> > > +		regulator-min-microvolt = <1200000>;
> > > +		regulator-max-microvolt = <1200000>;
> > > +		vin-supply = <&vcc_3v3>;
> > > +		/*enable-supply = <&vcc2v8_dvp>;*/
> > > +	};
> > 
> > There's no vdd1v2_dvp in the schematic on the camera sensor connector, or elsewhere:
> >
> >   https://megous.com/dl/tmp/fd95f003d8f3fbfb.png
> 
> It is on page 5 in the power diagram on the right top.

That (Power diagram overview) is irrelevant part of the schematic. Can be and
often is wrong. You need to use actual detailed parts of the schematic, which
is what is actually used to route the PCB traces.

kind regards,
	o.

> > So I guess, you can drop this, entirely. Maybe it's VDD1V5_DVP but I don't think
> > it needs to be described in DT, since it's pretty local to this camera sensor,
> > and nothing else uses it.
> > 
> >   https://megous.com/dl/tmp/7fc384e196c5428f.png
> 
> dvdd-supply is a required property of the ov5648 camera, so I would tend to keep
> this. But us vcc_sys for vin-supply instead of vcc_3v3.
> 
> Regards
> Manuel