Re: [PATCH 2/7] ARM: dts: s5pv210: Add initial DTS for Samsung Aries based phones.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Friday, June 22, 2018 9:41:02 AM CEST Krzysztof Kozlowski wrote:
> On 21 June 2018 at 21:09, Paweł Chmiel <pawel.mikolaj.chmiel@xxxxxxxxx> wrote:
> > This DTS file have initial support Samsung Aries based phones.
> > Initial version have support for:
> > - sdcard
> > - internal memory (present only on non 4g variant)
> > - max8998 pmic and rtc
> > - max17040 fuel gauge
> > - gpio keys
> > - fimd (no panel driver yet)
> > - usb (peripherial mode)
> > - wifi
> >
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@xxxxxxxxx>
> 
> Hi Pawel,
> 
> Nice job in mainlining!
> 
> Below you'll find some comments for improvement.
Thanks for all comments. I'll prepare v2 version with all issues fixed.

> 
> > ---
> >  arch/arm/boot/dts/s5pv210-aries.dtsi | 397 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 397 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/s5pv210-aries.dtsi
> >
> > diff --git a/arch/arm/boot/dts/s5pv210-aries.dtsi b/arch/arm/boot/dts/s5pv210-aries.dtsi
> > new file mode 100644
> > index 000000000000..6e8ac3615765
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/s5pv210-aries.dtsi
> > @@ -0,0 +1,397 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Samsung's S5PV210 based Galaxy Aries board device tree source
> > + */
> > +
> > +/dts-v1/;
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/input/input.h>
> 
> Is the input header used here?
> 
> > +#include <dt-bindings/interrupt-controller/irq.h>
> 
> Duplicated inclusion.
> 
> > +#include "s5pv210.dtsi"
> > +
> > +/ {
> > +       compatible = "samsung,aries", "samsung,s5pv210";
> > +
> > +       aliases {
> > +               i2c6 = &i2c_pmic;
> > +               i2c9 = &i2c_fuel;
> > +       };
> > +
> > +       memory@30000000 {
> > +               device_type = "memory";
> > +               reg = <0x30000000 0x05000000
> > +                       0x40000000 0x10000000
> > +                       0x50000000 0x08000000>;
> > +       };
> > +
> > +       wifi_pwrseq: wifi-pwrseq {
> > +               compatible = "mmc-pwrseq-simple";
> > +               reset-gpios = <&gpg1 2 GPIO_ACTIVE_LOW>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&wlan_gpio_rst>;
> > +               post-power-on-delay-ms = <500>;
> > +               power-off-delay-us = <500>;
> > +       };
> > +
> > +       i2c_pmic: i2c-pmic {
> 
> s/i2c-pmic/ to /i2c-gpio-0/
> to reflect generic class of this node. Change only the node name. The
> label can stay as is.
> 
> > +               compatible = "i2c-gpio";
> > +               sda-gpios = <&gpj4 0 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> > +               scl-gpios = <&gpj4 3 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> 
> Spaces around pipe |.
> 
> > +               i2c-gpio,delay-us = <2>;
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               pmic@66 {
> > +                       compatible = "maxim,max8998";
> > +                       reg = <0x66>;
> > +                       interrupt-parent = <&gph0>;
> > +                       interrupts = <7 0>;
> 
> If you really wanted 0 then IRQ_TYPE_NONE... but this should be a
> proper interrupt type.
> 
> > +
> > +                       max8998,pmic-buck1-default-dvs-idx = <1>;
> > +                       max8998,pmic-buck1-dvs-gpios = <&gph0 3 GPIO_ACTIVE_HIGH>,
> > +                                                       <&gph0 4 GPIO_ACTIVE_HIGH>;
> > +                       max8998,pmic-buck1-dvs-voltage = <1275000>, <1200000>,
> > +                                                       <1050000>, <950000>;
> > +
> > +                       max8998,pmic-buck2-default-dvs-idx = <0>;
> > +                       max8998,pmic-buck2-dvs-gpio = <&gph0 5 GPIO_ACTIVE_HIGH>;
> > +                       max8998,pmic-buck2-dvs-voltage = <1100000>, <1000000>;
> > +
> > +                       regulators {
> > +                               ldo2_reg: LDO2 {
> > +                                       regulator-name = "VALIVE_1.2V";
> > +                                       regulator-min-microvolt = <1200000>;
> > +                                       regulator-max-microvolt = <1200000>;
> > +                                       regulator-always-on;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-on-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo3_reg: LDO3 {
> > +                                       regulator-name = "VUSB_1.1V";
> > +                                       regulator-min-microvolt = <1100000>;
> > +                                       regulator-max-microvolt = <1100000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo4_reg: LDO4 {
> > +                                       regulator-name = "VADC_3.3V";
> > +                                       regulator-min-microvolt = <3300000>;
> > +                                       regulator-max-microvolt = <3300000>;
> > +                                       regulator-always-on;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo5_reg: LDO5 {
> > +                                       regulator-name = "VTF_2.8V";
> > +                                       regulator-min-microvolt = <2800000>;
> > +                                       regulator-max-microvolt = <2800000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> 
> LDO6?
> 
> All regulators should be defined in general. Most of the drivers would
> anyway register all of them and use driver-specific defaults for the
> ones which are missing in DT. I see that max8998 behaves differently
> and silently ignores missing regulators leaving them at bootloader
> settings. That's also not good because their initial settings might
> not be correct for current load and kernel cannot turn them off if
> needed. Also we want in general to have full description of HW in DT.
> 
> The same applies to LDO10, clocks and ESAFEOUT2 later.
> 
> 
> > +
> > +                               ldo7_reg: LDO7 {
> > +                                       regulator-name = "VLCD_1.8V";
> > +                                       regulator-min-microvolt = <1800000>;
> > +                                       regulator-max-microvolt = <1800000>;
> > +                                       /* Till we get panel driver */
> > +                                       regulator-always-on;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo8_reg: LDO8 {
> > +                                       regulator-name = "VUSB_3.3V";
> > +                                       regulator-min-microvolt = <3300000>;
> > +                                       regulator-max-microvolt = <3300000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo9_reg: LDO9 {
> > +                                       regulator-name = "VCC_2.8V_PDA";
> > +                                       regulator-min-microvolt = <2800000>;
> > +                                       regulator-max-microvolt = <2800000>;
> > +                                       regulator-always-on;
> > +                               };
> > +
> > +                               ldo11_reg: LDO11 {
> > +                                       regulator-name = "CAM_AF_3.0V";
> > +                                       regulator-min-microvolt = <3000000>;
> > +                                       regulator-max-microvolt = <3000000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo12_reg: LDO12 {
> > +                                       regulator-name = "CAM_SENSOR_CORE_1.2V";
> > +                                       regulator-min-microvolt = <1200000>;
> > +                                       regulator-max-microvolt = <1200000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo13_reg: LDO13 {
> > +                                       regulator-name = "VGA_VDDIO_2.8V";
> > +                                       regulator-min-microvolt = <2800000>;
> > +                                       regulator-max-microvolt = <2800000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo14_reg: LDO14 {
> > +                                       regulator-name = "VGA_DVDD_1.8V";
> > +                                       regulator-min-microvolt = <1800000>;
> > +                                       regulator-max-microvolt = <1800000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo15_reg: LDO15 {
> > +                                       regulator-name = "CAM_ISP_HOST_2.8V";
> > +                                       regulator-min-microvolt = <2800000>;
> > +                                       regulator-max-microvolt = <2800000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo16_reg: LDO16 {
> > +                                       regulator-name = "VGA_AVDD_2.8V";
> > +                                       regulator-min-microvolt = <2800000>;
> > +                                       regulator-max-microvolt = <2800000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               ldo17_reg: LDO17 {
> > +                                       regulator-name = "VCC_3.0V_LCD";
> > +                                       regulator-min-microvolt = <3000000>;
> > +                                       regulator-max-microvolt = <3000000>;
> > +                                       /* Till we get panel driver */
> > +                                       regulator-always-on;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               buck1_reg: BUCK1 {
> > +                                       regulator-name = "vddarm";
> > +                                       regulator-min-microvolt = <750000>;
> > +                                       regulator-max-microvolt = <1500000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                               regulator-suspend-microvolt = <1250000>;
> > +                                       };
> > +                               };
> > +
> > +                               buck2_reg: BUCK2 {
> > +                                       regulator-name = "vddint";
> > +                                       regulator-min-microvolt = <750000>;
> > +                                       regulator-max-microvolt = <1500000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                               regulator-suspend-microvolt = <1100000>;
> > +                                       };
> > +                               };
> > +
> > +                               buck3_reg: BUCK3 {
> > +                                       regulator-name = "VCC_1.8V";
> > +                                       regulator-min-microvolt = <1800000>;
> > +                                       regulator-max-microvolt = <1800000>;
> > +                                       regulator-always-on;
> > +                               };
> > +
> > +                               buck4_reg: BUCK4 {
> > +                                       regulator-name = "CAM_ISP_CORE_1.2V";
> > +                                       regulator-min-microvolt = <1200000>;
> > +                                       regulator-max-microvolt = <1200000>;
> > +
> > +                                       regulator-state-mem {
> > +                                               regulator-off-in-suspend;
> > +                                       };
> > +                               };
> > +
> > +                               safe1_sreg: ESAFEOUT1 {
> > +                                       regulator-name = "SAFEOUT1";
> > +                               };
> > +                       };
> > +               };
> > +       };
> > +
> > +       i2c_fuel: i2c-fuel {
> 
> s/i2c-fuel/ to /i2c-gpio-1/
> 
> 
> > +               compatible = "i2c-gpio";
> > +               sda-gpios = <&mp05 1 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> > +               scl-gpios = <&mp05 0 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> 
> Spaces around | please.
> 
> > +               i2c-gpio,delay-us = <2>;
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               fuel@36 {
> 
> Name: fuelgauge
> 
> > +                       compatible = "maxim,max17040";
> > +                       interrupt-parent = <&vic0>;
> > +                       interrupts = <7>;
> > +                       reg = <0x36>;
> > +               };
> > +       };
> > +};
> > +
> > +&xusbxti {
> > +       clock-frequency = <24000000>;
> > +};
> > +
> > +&pinctrl0 {
> 
> Please order all labels alphabetically. It reduces conflicts later
> with multiple changes.
> 
> > +       wlan_bt_en: wlan-bt-en {
> > +               samsung,pins = "gpb-5";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_OUTPUT>;
> > +               samsung,pin-pud = <S3C64XX_PIN_PULL_NONE>;
> > +               samsung,pin-val = <1>;
> > +       };
> > +
> > +       wlan_gpio_rst: wlan-gpio-rst {
> > +               samsung,pins = "gpg1-2";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_OUTPUT>;
> > +               samsung,pin-pud = <S3C64XX_PIN_PULL_NONE>;
> > +       };
> > +
> > +       wifi_host_wake: wifi-host-wake {
> > +               samsung,pins = "gph2-4";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> > +               samsung,pin-pud = <S3C64XX_PIN_PULL_DOWN>;
> > +               samsung,pin-drv = <EXYNOS4_PIN_DRV_LV1>;
> > +       };
> > +
> > +       tf_detect: tf-detect {
> > +               samsung,pins = "gph3-4";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> > +               samsung,pin-pud = <S3C64XX_PIN_PULL_DOWN>;
> > +               samsung,pin-drv = <EXYNOS4_PIN_DRV_LV1>;
> > +       };
> > +
> > +       wifi_wake: wifi-wake {
> > +               samsung,pins = "gph3-5";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_OUTPUT>;
> > +               samsung,pin-pud = <S3C64XX_PIN_PULL_NONE>;
> > +       };
> > +
> > +       massmemory_en: massmemory-en {
> > +               samsung,pins = "gpj2-7";
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_OUTPUT>;
> > +               samsung,pin-pud = <S3C64XX_PIN_PULL_NONE>;
> > +               samsung,pin-drv = <EXYNOS4_PIN_DRV_LV1>;
> > +       };
> > +};
> > +
> > +&uart0 {
> > +       status = "okay";
> > +};
> > +
> > +&uart1 {
> > +       status = "okay";
> > +};
> > +
> > +&uart2 {
> > +       status = "okay";
> > +};
> > +
> > +&sdhci1 {
> > +       #address-cells = <1>;
> > +       #size-cells = <0>;
> > +
> > +       bus-width = <4>;
> > +       max-frequency = <38400000>;
> > +       pinctrl-0 = <&sd1_clk &sd1_cmd &sd1_bus4 &wifi_wake &wifi_host_wake &wlan_bt_en>;
> > +       pinctrl-names = "default";
> > +       cap-sd-highspeed;
> > +       cap-mmc-highspeed;
> > +
> > +       mmc-pwrseq = <&wifi_pwrseq>;
> > +       non-removable;
> > +       status = "okay";
> > +
> > +       brcmf: bcrmf@1 {
> 
> Name of node should describe generic class of the device so maybe
> "wlan" or "wlanbt"? Label is okay.
> 
> Best regards,
> Krzysztof
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux