| Message ID | 1501746323-5254-3-git-send-email-m.purski@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Aug 03, 2017 at 09:45:23AM +0200, Maciej Purski wrote: > This patch adds HDMI and Sil9234 MHL converter to Trats2 board. Just "Add HDMI...", without this patch. Except few minor nitpicks below, looks good. After fixing I will take it once bindings got accepted. > > Based on previous work by: > Tomasz Stanislawski <t.stanislaws@samsung.com> > > Signed-off-by: Maciej Purski <m.purski@samsung.com> > --- > arch/arm/boot/dts/exynos4412-trats2.dts | 93 +++++++++++++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts > index 35e9b94..39940f6 100644 > --- a/arch/arm/boot/dts/exynos4412-trats2.dts > +++ b/arch/arm/boot/dts/exynos4412-trats2.dts > @@ -97,6 +97,16 @@ > gpio = <&gpj0 5 GPIO_ACTIVE_HIGH>; > enable-active-high; > }; > + > + vsil: voltage-regulator-vsil { > + compatible = "regulator-fixed"; > + regulator-name = "HDMI_5V"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + gpio = <&gpl0 4 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + vin-supply = <&buck7_reg>; I think the supply is V_BAT, not buck7. > + }; > }; > > gpio-keys { > @@ -229,6 +239,34 @@ > }; > }; > > + i2c-mhl { > + compatible = "i2c-gpio"; > + gpios = <&gpf0 4 GPIO_ACTIVE_HIGH &gpf0 6 GPIO_ACTIVE_HIGH>; > + i2c-gpio,delay-us = <100>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + pinctrl-0 = <&i2c_mhl_bus>; > + pinctrl-names = "default"; > + status = "okay"; > + > + sii9234: sii9234@39 { The name of node should be rather generic (ePAPR: "The name of a node should be somewhat generic, reflecting the function of the device and not its precise programming model"). So maybe "sii9234: hdmi-bridge@39"? > + compatible = "sil,sii9234"; > + vcc-supply = <&vsil>; > + reset-gpios = <&gpf3 4 GPIO_ACTIVE_HIGH>; > + interrupt-parent = <&gpf3>; > + interrupts = <5 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0x39>; > + > + One empty line instead of two. > + port { > + mhl_to_hdmi: endpoint { > + remote-endpoint = <&hdmi_to_mhl>; > + }; > + }; > + }; > + }; > + > camera: camera { > pinctrl-0 = <&cam_port_a_clk_active &cam_port_b_clk_active>; > pinctrl-names = "default"; > @@ -522,6 +560,31 @@ > status = "okay"; > }; > > +&hdmi { > + hpd-gpios = <&gpx3 7 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&hdmi_hpd>; > + hdmi-en-supply = <&vsil>; > + vdd-supply = <&ldo3_reg>; > + vdd_osc-supply = <&ldo4_reg>; > + vdd_pll-supply = <&ldo3_reg>; > + ddc = <&i2c_5>; > + status = "okay"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@1 { > + reg = <1>; > + hdmi_to_mhl: endpoint { > + remote-endpoint = <&mhl_to_hdmi>; > + }; > + }; > + }; > + Unnecessary empty line. > +}; > + > &hsotg { > vusb_d-supply = <&ldo15_reg>; > vusb_a-supply = <&ldo12_reg>; > @@ -600,6 +663,11 @@ > }; > }; > > + > +&i2c_5 { > + status = "okay"; > +}; Could you describe more what is on i2c_5 and i2c_8? Is it relevant to this patch? > + > &i2c_7 { > samsung,i2c-sda-delay = <100>; > samsung,i2c-slave-addr = <0x10>; > @@ -894,12 +962,20 @@ > }; > }; > > +&i2c_8 { > + status = "okay"; > +}; > + > &i2s0 { > pinctrl-0 = <&i2s0_bus>; > pinctrl-names = "default"; > status = "okay"; > }; > > +&mixer { > + status = "okay"; > +}; > + > &mshc_0 { > num-slots = <1>; > broken-cd; > @@ -926,6 +1002,18 @@ > pinctrl-names = "default"; > pinctrl-0 = <&sleep0>; > > + mhl_int: mhl-int { > + samsung,pins = "gpf3-5"; > + samsung,pin-pud = <0>; Please use defines from dt-bindings/pinctrl/samsung.h > + }; > + > + i2c_mhl_bus: i2c-mhl-bus { > + samsung,pins = "gpf0-4", "gpf0-6"; > + samsung,pin-function = <2>; > + samsung,pin-pud = <1>; > + samsung,pin-drv = <0>; The same. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Krzysztof, On 2017-08-03 21:20, Krzysztof Kozlowski wrote: > On Thu, Aug 03, 2017 at 09:45:23AM +0200, Maciej Purski wrote: >> This patch adds HDMI and Sil9234 MHL converter to Trats2 board. > Just "Add HDMI...", without this patch. > > Except few minor nitpicks below, looks good. After fixing I will take it > once bindings got accepted. > >> Based on previous work by: >> Tomasz Stanislawski <t.stanislaws@samsung.com> >> >> Signed-off-by: Maciej Purski <m.purski@samsung.com> >> --- >> arch/arm/boot/dts/exynos4412-trats2.dts | 93 +++++++++++++++++++++++++++++++++ >> 1 file changed, 93 insertions(+) >> >> diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts >> index 35e9b94..39940f6 100644 >> --- a/arch/arm/boot/dts/exynos4412-trats2.dts >> +++ b/arch/arm/boot/dts/exynos4412-trats2.dts >> @@ -97,6 +97,16 @@ >> gpio = <&gpj0 5 GPIO_ACTIVE_HIGH>; >> enable-active-high; >> }; >> + >> + vsil: voltage-regulator-vsil { >> + compatible = "regulator-fixed"; >> + regulator-name = "HDMI_5V"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + gpio = <&gpl0 4 GPIO_ACTIVE_HIGH>; >> + enable-active-high; >> + vin-supply = <&buck7_reg>; > I think the supply is V_BAT, not buck7. Well, according to the the schematic, VSIL is derived from VCC_SUB_2.0V (buck7) by the RP114K121D-TR chip, which is controlled by gpl0-4 (HDMI_EN) pin. The only thing that has to be fixed is the voltage value for that regulator. VSIL is 1.2V and regulator-name should be adjusted too. The HDMI_V5 name and voltage value seems to be copy/paste error done long time ago... >> + }; >> }; >> >> gpio-keys { >> @@ -229,6 +239,34 @@ >> }; >> }; >> >> + i2c-mhl { >> + compatible = "i2c-gpio"; >> + gpios = <&gpf0 4 GPIO_ACTIVE_HIGH &gpf0 6 GPIO_ACTIVE_HIGH>; >> + i2c-gpio,delay-us = <100>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + pinctrl-0 = <&i2c_mhl_bus>; >> + pinctrl-names = "default"; >> + status = "okay"; >> + >> + sii9234: sii9234@39 { > The name of node should be rather generic (ePAPR: "The name of a node > should be somewhat generic, reflecting the function of the device and > not its precise programming model"). > > So maybe "sii9234: hdmi-bridge@39"? > >> + compatible = "sil,sii9234"; >> + vcc-supply = <&vsil>; >> + reset-gpios = <&gpf3 4 GPIO_ACTIVE_HIGH>; >> + interrupt-parent = <&gpf3>; >> + interrupts = <5 IRQ_TYPE_LEVEL_HIGH>; >> + reg = <0x39>; >> + >> + > One empty line instead of two. > >> + port { >> + mhl_to_hdmi: endpoint { >> + remote-endpoint = <&hdmi_to_mhl>; >> + }; >> + }; >> + }; >> + }; >> + >> camera: camera { >> pinctrl-0 = <&cam_port_a_clk_active &cam_port_b_clk_active>; >> pinctrl-names = "default"; >> @@ -522,6 +560,31 @@ >> status = "okay"; >> }; >> >> +&hdmi { >> + hpd-gpios = <&gpx3 7 GPIO_ACTIVE_HIGH>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&hdmi_hpd>; >> + hdmi-en-supply = <&vsil>; >> + vdd-supply = <&ldo3_reg>; >> + vdd_osc-supply = <&ldo4_reg>; >> + vdd_pll-supply = <&ldo3_reg>; >> + ddc = <&i2c_5>; >> + status = "okay"; >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@1 { >> + reg = <1>; >> + hdmi_to_mhl: endpoint { >> + remote-endpoint = <&mhl_to_hdmi>; >> + }; >> + }; >> + }; >> + > Unnecessary empty line. > >> +}; >> + >> &hsotg { >> vusb_d-supply = <&ldo15_reg>; >> vusb_a-supply = <&ldo12_reg>; >> @@ -600,6 +663,11 @@ >> }; >> }; >> >> + >> +&i2c_5 { >> + status = "okay"; >> +}; > Could you describe more what is on i2c_5 and i2c_8? Is it relevant to > this patch? Yes. i2c_5 is used for HDMI DDC and i2c_8 is used for HDMI_PHY. None of the other exynos*.dts, which enable HDMI has any comment on them... >> + >> &i2c_7 { >> samsung,i2c-sda-delay = <100>; >> samsung,i2c-slave-addr = <0x10>; >> @@ -894,12 +962,20 @@ >> }; >> }; >> >> +&i2c_8 { >> + status = "okay"; >> +}; >> + >> &i2s0 { >> pinctrl-0 = <&i2s0_bus>; >> pinctrl-names = "default"; >> status = "okay"; >> }; >> >> +&mixer { >> + status = "okay"; >> +}; >> + >> &mshc_0 { >> num-slots = <1>; >> broken-cd; >> @@ -926,6 +1002,18 @@ >> pinctrl-names = "default"; >> pinctrl-0 = <&sleep0>; >> >> + mhl_int: mhl-int { >> + samsung,pins = "gpf3-5"; >> + samsung,pin-pud = <0>; > Please use defines from dt-bindings/pinctrl/samsung.h > >> + }; >> + >> + i2c_mhl_bus: i2c-mhl-bus { >> + samsung,pins = "gpf0-4", "gpf0-6"; >> + samsung,pin-function = <2>; >> + samsung,pin-pud = <1>; >> + samsung,pin-drv = <0>; > The same. > Best regards
On Fri, Aug 4, 2017 at 8:32 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi Krzysztof, > > > On 2017-08-03 21:20, Krzysztof Kozlowski wrote: >> >> On Thu, Aug 03, 2017 at 09:45:23AM +0200, Maciej Purski wrote: >>> >>> This patch adds HDMI and Sil9234 MHL converter to Trats2 board. >> >> Just "Add HDMI...", without this patch. >> >> Except few minor nitpicks below, looks good. After fixing I will take it >> once bindings got accepted. >> >>> Based on previous work by: >>> Tomasz Stanislawski <t.stanislaws@samsung.com> >>> >>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>> --- >>> arch/arm/boot/dts/exynos4412-trats2.dts | 93 +++++++++++++++++++++++++++++++++ >>> 1 file changed, 93 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts >>> index 35e9b94..39940f6 100644 >>> --- a/arch/arm/boot/dts/exynos4412-trats2.dts >>> +++ b/arch/arm/boot/dts/exynos4412-trats2.dts >>> @@ -97,6 +97,16 @@ >>> gpio = <&gpj0 5 GPIO_ACTIVE_HIGH>; >>> enable-active-high; >>> }; >>> + >>> + vsil: voltage-regulator-vsil { >>> + compatible = "regulator-fixed"; >>> + regulator-name = "HDMI_5V"; >>> + regulator-min-microvolt = <5000000>; >>> + regulator-max-microvolt = <5000000>; >>> + gpio = <&gpl0 4 GPIO_ACTIVE_HIGH>; >>> + enable-active-high; >>> + vin-supply = <&buck7_reg>; >> >> I think the supply is V_BAT, not buck7. > > > Well, according to the the schematic, VSIL is derived from VCC_SUB_2.0V > (buck7) by the RP114K121D-TR chip, which is controlled by gpl0-4 (HDMI_EN) > pin. > The only thing that has to be fixed is the voltage value for that regulator. > VSIL is 1.2V and regulator-name should be adjusted too. The HDMI_V5 name > and voltage value seems to be copy/paste error done long time ago... > OK, thanks for explanation. (...) >> >> Could you describe more what is on i2c_5 and i2c_8? Is it relevant to >> this patch? > > > Yes. i2c_5 is used for HDMI DDC and i2c_8 is used for HDMI_PHY. None of > the other exynos*.dts, which enable HDMI has any comment on them... Actually now I see that this information can be get from the source (see i2c_8 in exyos4.dtsi and here i2c_5 is used in hdmi node as ddc). Still it would be nice to see this information for example in commit message. Best regards, Krzysztof > > >>> + >>> &i2c_7 { >>> samsung,i2c-sda-delay = <100>; >>> samsung,i2c-slave-addr = <0x10>; >>> @@ -894,12 +962,20 @@ >>> }; >>> }; >>> +&i2c_8 { >>> + status = "okay"; >>> +}; >>> + >>> &i2s0 { >>> pinctrl-0 = <&i2s0_bus>; >>> pinctrl-names = "default"; >>> status = "okay"; >>> }; >>> +&mixer { >>> + status = "okay"; >>> +}; >>> + >>> &mshc_0 { >>> num-slots = <1>; >>> broken-cd; >>> @@ -926,6 +1002,18 @@ >>> pinctrl-names = "default"; >>> pinctrl-0 = <&sleep0>; >>> + mhl_int: mhl-int { >>> + samsung,pins = "gpf3-5"; >>> + samsung,pin-pud = <0>; >> >> Please use defines from dt-bindings/pinctrl/samsung.h >> >>> + }; >>> + >>> + i2c_mhl_bus: i2c-mhl-bus { >>> + samsung,pins = "gpf0-4", "gpf0-6"; >>> + samsung,pin-function = <2>; >>> + samsung,pin-pud = <1>; >>> + samsung,pin-drv = <0>; >> >> The same. >> > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts index 35e9b94..39940f6 100644 --- a/arch/arm/boot/dts/exynos4412-trats2.dts +++ b/arch/arm/boot/dts/exynos4412-trats2.dts @@ -97,6 +97,16 @@ gpio = <&gpj0 5 GPIO_ACTIVE_HIGH>; enable-active-high; }; + + vsil: voltage-regulator-vsil { + compatible = "regulator-fixed"; + regulator-name = "HDMI_5V"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + gpio = <&gpl0 4 GPIO_ACTIVE_HIGH>; + enable-active-high; + vin-supply = <&buck7_reg>; + }; }; gpio-keys { @@ -229,6 +239,34 @@ }; }; + i2c-mhl { + compatible = "i2c-gpio"; + gpios = <&gpf0 4 GPIO_ACTIVE_HIGH &gpf0 6 GPIO_ACTIVE_HIGH>; + i2c-gpio,delay-us = <100>; + #address-cells = <1>; + #size-cells = <0>; + + pinctrl-0 = <&i2c_mhl_bus>; + pinctrl-names = "default"; + status = "okay"; + + sii9234: sii9234@39 { + compatible = "sil,sii9234"; + vcc-supply = <&vsil>; + reset-gpios = <&gpf3 4 GPIO_ACTIVE_HIGH>; + interrupt-parent = <&gpf3>; + interrupts = <5 IRQ_TYPE_LEVEL_HIGH>; + reg = <0x39>; + + + port { + mhl_to_hdmi: endpoint { + remote-endpoint = <&hdmi_to_mhl>; + }; + }; + }; + }; + camera: camera { pinctrl-0 = <&cam_port_a_clk_active &cam_port_b_clk_active>; pinctrl-names = "default"; @@ -522,6 +560,31 @@ status = "okay"; }; +&hdmi { + hpd-gpios = <&gpx3 7 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&hdmi_hpd>; + hdmi-en-supply = <&vsil>; + vdd-supply = <&ldo3_reg>; + vdd_osc-supply = <&ldo4_reg>; + vdd_pll-supply = <&ldo3_reg>; + ddc = <&i2c_5>; + status = "okay"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@1 { + reg = <1>; + hdmi_to_mhl: endpoint { + remote-endpoint = <&mhl_to_hdmi>; + }; + }; + }; + +}; + &hsotg { vusb_d-supply = <&ldo15_reg>; vusb_a-supply = <&ldo12_reg>; @@ -600,6 +663,11 @@ }; }; + +&i2c_5 { + status = "okay"; +}; + &i2c_7 { samsung,i2c-sda-delay = <100>; samsung,i2c-slave-addr = <0x10>; @@ -894,12 +962,20 @@ }; }; +&i2c_8 { + status = "okay"; +}; + &i2s0 { pinctrl-0 = <&i2s0_bus>; pinctrl-names = "default"; status = "okay"; }; +&mixer { + status = "okay"; +}; + &mshc_0 { num-slots = <1>; broken-cd; @@ -926,6 +1002,18 @@ pinctrl-names = "default"; pinctrl-0 = <&sleep0>; + mhl_int: mhl-int { + samsung,pins = "gpf3-5"; + samsung,pin-pud = <0>; + }; + + i2c_mhl_bus: i2c-mhl-bus { + samsung,pins = "gpf0-4", "gpf0-6"; + samsung,pin-function = <2>; + samsung,pin-pud = <1>; + samsung,pin-drv = <0>; + }; + sleep0: sleep-states { PIN_SLP(gpa0-0, INPUT, NONE); PIN_SLP(gpa0-1, OUT0, NONE); @@ -1029,6 +1117,11 @@ pinctrl-names = "default"; pinctrl-0 = <&sleep1>; + hdmi_hpd: hdmi-hpd { + samsung,pins = "gpx3-7"; + samsung,pin-pud = <EXYNOS_PIN_PULL_DOWN>; + }; + sleep1: sleep-states { PIN_SLP(gpk0-0, PREV, NONE); PIN_SLP(gpk0-1, PREV, NONE);
This patch adds HDMI and Sil9234 MHL converter to Trats2 board. Based on previous work by: Tomasz Stanislawski <t.stanislaws@samsung.com> Signed-off-by: Maciej Purski <m.purski@samsung.com> --- arch/arm/boot/dts/exynos4412-trats2.dts | 93 +++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)