Hi, Ondřej
On 17.08.2023 16:57, Ondřej Jirman wrote:
> Hi Muhammed,
>
> On Thu, Aug 17, 2023 at 04:33:55PM +0300, Muhammed Efe Cetin wrote:
>>
>> Hi, Ondřej
>>
>> Thanks for reviews, i'll fix them soon.
>>
>> On 15.08.2023 19:39, Ondřej Jirman wrote:
>>> Hello Muhammed,
>>>
>>>> [...]
>>>> +
>>>> + adc-keys {
>>>> + compatible = "adc-keys";
>>>> + io-channels = <&saradc 1>;
>>>> + io-channel-names = "buttons";
>>>> + keyup-threshold-microvolt = <1800000>;
>>>> + poll-interval = <100>;
>>>> +
>>>> + vol-up-key {
>>>> + label = "volume up";
>>>> + linux,code = <KEY_VOLUMEUP>;
>>>> + press-threshold-microvolt = <17000>;
>>>> + };
>>>> +
>>>> + vol-down-key {
>>>> + label = "volume down";
>>>> + linux,code = <KEY_VOLUMEDOWN>;
>>>> + press-threshold-microvolt = <417000>;
>>>> + };
>>>> +
>>>> + menu-key {
>>>> + label = "menu";
>>>> + linux,code = <KEY_MENU>;
>>>> + press-threshold-microvolt = <890000>;
>>>> + };
>>>> +
>>>> + back-key {
>>>> + label = "back";
>>>> + linux,code = <KEY_BACK>;
>>>> + press-threshold-microvolt = <1235000>;
>>>
>>> None of these 4 buttons are in the scehmatic. There's one recovery button hooked
>>> to saradc 0, instead.
>>
>> Will remove these too. I won't add recovery button because of it's mostly used for rockusb.
>
> Recovery button is useful from userspace, too. Please keep it. You already have
> the DT nodes, please just reduce them to one node for the recovery key.
Ok i will add it as KEY_VENDOR button.
>
>>>> [...]
>>>
>>>> + vcc_1v1_nldo_s3: vcc-1v1-nldo-s3-regulator {
>>>> + compatible = "regulator-fixed";
>>>> + regulator-name = "vcc_1v1_nldo_s3";
>>>> + regulator-always-on;
>>>> + regulator-boot-on;
>>>> + regulator-min-microvolt = <1100000>;
>>>> + regulator-max-microvolt = <1100000>;
>>>> + vin-supply = <&vcc5v0_sys>;
>>>> + };
>>>
>>> There's no such regulator on the board.
>>
>> It's connected to PMIC https://i.imgur.com/sVJdC5K.png
>
> It's not a separate fixed regulator. It's a PMIC output from buck6 https://megous.com/dl/tmp/8630fa17407c75b9.png
>
I think it should be fixed regulator. It's used as vcc13-supply and vcc14-supply regulator on PMIC and it's same as other rk3588 boards.
> So this is VDD2_DDR_S3. If you want to keep the alias, just add extra alias to
> dcdc-reg6 like this:
>
> ...
> vcc_1v1_nldo_s3: vdd2_ddr_s3: dcdc-reg6 {
> ...
>
>>>
>>>> + vbus5v0_typec: vbus5v0-typec-regulator {
>>>> + compatible = "regulator-fixed";
>>>> + regulator-name = "vbus5v0_typec";
>>>> + regulator-min-microvolt = <5000000>;
>>>> + regulator-max-microvolt = <5000000>;
>>>> + enable-active-high;
>>>> + gpio = <&gpio3 RK_PC0 GPIO_ACTIVE_HIGH>;
>>>> + vin-supply = <&vcc5v0_usb>;
>>>> + pinctrl-names = "default";
>>>> + pinctrl-0 = <&typec5v_pwren>;
>>>> + };
>>>
>>> No such power rail on the board. This should be VBUS_TYPEC power rail.
>>> Please name regulators/power rails as they are named in the schematic.
>>> Otherwise it's impossible to cross-check DT against schematic.
>>
>> Perhaps i can add it as a comment line. Otherwise the current name seems OK to
>> me but still open to suggestions.
>
> Please just use the name that's in the schematic, even if it's subotpimal. Your
> comment will not be seen in debugfs/sysfs and other places, where it's also
> useful for debugging.
Ok i will make this one "vbus_typec"
>
>>>
>>> No such regulator on the board. You might have meant VCC3V3_PCIE30,
>>> or not?
>>
>> Yes. However their naming in schematics is pretty nonsense. The board doesn't
>> have PCIe3, i think current naming is more appropriate.
>
> Same as above. It's more important to be able to match DT/runtime debug outputs
> and schematic than having a subjectively more meaningful name which is not used
> anywhere in the documentation.
I agree with you but VCC3V3_PCIE30 is really confusing regulator name. I guess Xunlong made a mistake about it. I think we can name it as "vcc3v3_pcie20"
>
>>>> + sata {
>>>> + sata_reset:sata-reset{
>>>> + rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_up>;
>>>
>>> Not what you think: https://megous.com/dl/tmp/f135580b68cb5dfe.png
>>>
>>>> + };
>>>> + };
>>>> +};
>>>> +
>>>> +&pwm2 {
>>>> + pinctrl-names = "active";
>>>> + pinctrl-0 = <&pwm2m0_pins>;
>>>> + status = "okay";
>>>> +};
>>>> +
>>>> +&pwm6 {
>>>> + pinctrl-names = "active";
>>>> + pinctrl-0 = <&pwm6m0_pins>;
>>>> + status = "okay";
>>>> +};
>>>> +
>>>> +&saradc {
>>>> + vref-supply = <&avcc_1v8_s0>;
>>>> + status = "okay";
>>>> +};
>>>> +
>>>> +&sata0 {
>>>> + pinctrl-names = "default";
>>>> + pinctrl-0 = <&sata_reset>;
>>>> + status = "disabled";
>>>> +};
>>>
>>> Where do you see a SATA port? http://www.orangepi.org/html/hardWare/computerAndMicrocontrollers/details/Orange-Pi-5.html
>>>
>>> Or a reset signal in the schematic?
>>
>> It's needed to use sata node once you want to use mSata SSD. I don't know if
>> it works without sata_reset pinctrl. Don't have mSata SSD to try.
>
> Ok, that makes sense. It would be better to add this once you can test it, though.
> And if there's no way to switch between SATA and PCIe at runtime, it's probably
> just another thing for an overlay which would enable/disable/add all the appropriate
> DT nodes.
Yeah you are right.
>
> thank you,
> o.
Regards,
Efe
