From: "Paweł Chmiel" <pawel.mikolaj.chmiel@gmail.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: kgene@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,
linux@armlinux.org.uk, linux-arm-kernel@lists.infradead.org,
"linux-samsung-soc@vger.kernel.org"
<linux-samsung-soc@vger.kernel.org>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
xc-racer2@live.ca
Subject: Re: [PATCH 2/7] ARM: dts: s5pv210: Add initial DTS for Samsung Aries based phones.
Date: Fri, 22 Jun 2018 12:15:43 +0200 [thread overview]
Message-ID: <2053829.UjnW4DntxV@acerlaptop> (raw)
In-Reply-To: <CAJKOXPf9BVXP19Yt36uEKz=bzMh3cDd6_wdyxoRRX3itk9kuhw@mail.gmail.com>
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@gmail.com> 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@gmail.com>
>
> 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
>
WARNING: multiple messages have this Message-ID (diff)
From: pawel.mikolaj.chmiel@gmail.com (Paweł Chmiel)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 2/7] ARM: dts: s5pv210: Add initial DTS for Samsung Aries based phones.
Date: Fri, 22 Jun 2018 12:15:43 +0200 [thread overview]
Message-ID: <2053829.UjnW4DntxV@acerlaptop> (raw)
In-Reply-To: <CAJKOXPf9BVXP19Yt36uEKz=bzMh3cDd6_wdyxoRRX3itk9kuhw@mail.gmail.com>
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@gmail.com> 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@gmail.com>
>
> 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 at 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 at 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 at 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 at 1 {
>
> Name of node should describe generic class of the device so maybe
> "wlan" or "wlanbt"? Label is okay.
>
> Best regards,
> Krzysztof
>
next prev parent reply other threads:[~2018-06-22 10:15 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-21 19:09 [PATCH 0/7] Initial support for Samsung Galaxy S and Galaxy S 4G Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-21 19:09 ` [PATCH 1/7] ARM: dts: s5pv210: Add missing interrupt-controller property to gph2 Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-21 19:09 ` [PATCH 2/7] ARM: dts: s5pv210: Add initial DTS for Samsung Aries based phones Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-22 7:41 ` Krzysztof Kozlowski
2018-06-22 7:41 ` Krzysztof Kozlowski
2018-06-22 10:15 ` Paweł Chmiel [this message]
2018-06-22 10:15 ` Paweł Chmiel
2018-06-21 19:09 ` [PATCH 3/7] ARM: dts: s5pv210: Add initial DTS for Samsung Galaxy S phone Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-22 7:49 ` Krzysztof Kozlowski
2018-06-22 7:49 ` Krzysztof Kozlowski
2018-06-22 8:42 ` Paweł Chmiel
2018-06-22 8:42 ` Paweł Chmiel
2018-06-22 8:51 ` Krzysztof Kozlowski
2018-06-22 8:51 ` Krzysztof Kozlowski
2018-06-21 19:09 ` [PATCH 4/7] ARM: s5pv210_defconfig: Enable drivers for Samsung Aries based phones Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-22 7:54 ` Krzysztof Kozlowski
2018-06-22 7:54 ` Krzysztof Kozlowski
2018-06-22 7:54 ` Krzysztof Kozlowski
2018-06-21 19:09 ` [PATCH 5/7] dt-bindings: samsung: Document bindings for Samsung aries boards Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-22 7:56 ` Krzysztof Kozlowski
2018-06-22 7:56 ` Krzysztof Kozlowski
2018-06-22 7:57 ` Krzysztof Kozlowski
2018-06-22 7:57 ` Krzysztof Kozlowski
2018-06-27 17:21 ` Rob Herring
2018-06-27 17:21 ` Rob Herring
2018-06-21 19:09 ` [PATCH 6/7] ARM: dts: s5pv210: Add initial DTS config for SGH-T959P phone Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-22 7:58 ` Krzysztof Kozlowski
2018-06-22 7:58 ` Krzysztof Kozlowski
2018-06-21 19:09 ` [PATCH 7/7] dt-bindings: samsung: Document binding for SGH-T959P board Paweł Chmiel
2018-06-21 19:09 ` Paweł Chmiel
2018-06-22 7:59 ` Krzysztof Kozlowski
2018-06-22 7:59 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2053829.UjnW4DntxV@acerlaptop \
--to=pawel.mikolaj.chmiel@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=kgene@kernel.org \
--cc=krzk@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=xc-racer2@live.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.