All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marek Szyprowski <m.szyprowski@samsung.com>
To: Krzysztof Kozlowski <krzk@kernel.org>,
	Maciej Purski <m.purski@samsung.com>
Cc: dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
	linux-samsung-soc@vger.kernel.org, airlied@linux.ie,
	robh+dt@kernel.org, mark.rutland@arm.com, architt@codeaurora.org,
	a.hajda@samsung.com, Laurent.pinchart@ideasonboard.com,
	b.zolnierkie@samsung.com
Subject: Re: [PATCH 2/2] ARM: dts: exynos: Add HDMI and Sil9234 to Trats2 board
Date: Fri, 04 Aug 2017 08:32:25 +0200	[thread overview]
Message-ID: <aa50cb82-29be-acfc-32b4-7f2a64266d8a@samsung.com> (raw)
In-Reply-To: <20170803192000.trgfndzj4b3bnisc@kozik-lap>

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

  reply	other threads:[~2017-08-04  6:32 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20170803074535eucas1p107414859fa54647d1f623c73d0fc17f2@eucas1p1.samsung.com>
2017-08-03  7:45 ` [PATCH 0/2] add Silicon Image SiI9234 driver Maciej Purski
     [not found]   ` <CGME20170803074538eucas1p1fec88e4f2c3ebc00054fd362a504c03e@eucas1p1.samsung.com>
2017-08-03  7:45     ` [PATCH 1/2] drm/bridge: " Maciej Purski
2017-08-03 10:24       ` Laurent Pinchart
2017-08-04  6:55         ` Marek Szyprowski
2017-08-10 14:51           ` Laurent Pinchart
2017-08-11  7:00             ` Marek Szyprowski
2017-08-11  8:59               ` Laurent Pinchart
2017-08-11  9:00               ` Laurent Pinchart
2017-08-14 16:35                 ` Mark Brown
2017-08-03 19:36       ` kbuild test robot
     [not found]         ` <1501746323-5254-2-git-send-email-m.purski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2017-08-03 19:36           ` [PATCH] drm/bridge: fix platform_no_drv_owner.cocci warnings kbuild test robot
2017-08-12 22:43           ` [PATCH 1/2] drm/bridge: add Silicon Image SiI9234 driver kbuild test robot
     [not found]   ` <CGME20170803074541eucas1p2b054d4853a98819fc42f19f7cae7f419@eucas1p2.samsung.com>
2017-08-03  7:45     ` [PATCH 2/2] ARM: dts: exynos: Add HDMI and Sil9234 to Trats2 board Maciej Purski
2017-08-03 19:20       ` Krzysztof Kozlowski
2017-08-04  6:32         ` Marek Szyprowski [this message]
2017-08-04  6:44           ` 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=aa50cb82-29be-acfc-32b4-7f2a64266d8a@samsung.com \
    --to=m.szyprowski@samsung.com \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=architt@codeaurora.org \
    --cc=b.zolnierkie@samsung.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=krzk@kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=m.purski@samsung.com \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    /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.