[PATCH 0/2] drm/bridge/sii8620: add external connector handle

[PATCH 0/2] drm/bridge/sii8620: add external connector handle

[Maciej Purski]

Hi everyone, 

These patches add external connector handling to sii8620 driver.

The first patch documents the extcon binding and add extcon handling 
to the driver. The second adds support for this in exynos5433-tm2-common.dtsi.

All comments are welcome.

Regards,
Maciej Purski

Patch summary:

Maciej Purski (2):
  drm/bridge/sii8620: add external connector handle
  arm64: dts: exynos: add extcon phandle

 .../bindings/display/bridge/sil-sii8620.txt        |  4 ++
 .../boot/dts/exynos/exynos5433-tm2-common.dtsi     |  1 +
 drivers/gpu/drm/bridge/sil-sii8620.c               | 83 +++++++++++++++++++++-
 3 files changed, 86 insertions(+), 2 deletions(-)

-- 
2.7.4

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

[PATCH 1/2] drm/bridge/sii8620: add external connector handle

[Maciej Purski]

The driver should be switched on if an external connector is plugged and
switched off if it is unplugged. Extcon is optional. If it is not found,
the driver stays in "always-on" mode.

Signed-off-by: Maciej Purski <m.purski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../bindings/display/bridge/sil-sii8620.txt        |  4 ++
 drivers/gpu/drm/bridge/sil-sii8620.c               | 83 +++++++++++++++++++++-
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
index 9409d9c..1f230bf 100644
--- a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
+++ b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
@@ -10,6 +10,9 @@ Required properties:
 	- clocks, clock-names: specification and name of "xtal" clock
 	- video interfaces: Device node can contain video interface port
 			    node for HDMI encoder according to [1].
+Optional properties:
+	- extcon: phandle to external connector for MHL cable changes
+		  detection
 
 [1]: Documentation/devicetree/bindings/media/video-interfaces.txt
 
@@ -24,6 +27,7 @@ Example:
 		reset-gpio = <&gpv7 0 0>;
 		clocks = <&pmu_system_controller 0>;
 		clock-names = "xtal";
+		extcon = <&muic>;
 
 		port {
 			mhl_to_hdmi: endpoint {
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 2d51a22..5002654 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -17,6 +17,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/extcon.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -78,6 +79,10 @@ struct sii8620 {
 	struct edid *edid;
 	unsigned int gen2_write_burst:1;
 	enum sii8620_mt_state mt_state;
+	struct extcon_dev *extcon;
+	struct notifier_block extcon_nb;
+	struct work_struct extcon_wq;
+	bool extcon_attached;
 	struct list_head mt_queue;
 	struct {
 		int r_size;
@@ -2102,6 +2107,72 @@ static void sii8620_cable_in(struct sii8620 *ctx)
 	enable_irq(to_i2c_client(ctx->dev)->irq);
 }
 
+static void sii8620_cable_out(struct sii8620 *ctx)
+{
+	disable_irq(to_i2c_client(ctx->dev)->irq);
+	sii8620_hw_off(ctx);
+}
+
+static void sii8620_extcon_work(struct work_struct *work)
+{
+	struct sii8620 *ctx =
+		container_of(work, struct sii8620, extcon_wq);
+
+	if (ctx->extcon_attached)
+		sii8620_cable_in(ctx);
+	else
+		sii8620_cable_out(ctx);
+}
+
+static int sii8620_extcon_notifier(struct notifier_block *self,
+			unsigned long event, void *ptr)
+{
+	struct sii8620 *ctx =
+		container_of(self, struct sii8620, extcon_nb);
+
+	ctx->extcon_attached = event;
+	schedule_work(&ctx->extcon_wq);
+
+	return NOTIFY_DONE;
+}
+
+static int sii8620_extcon_init(struct sii8620 *ctx)
+{
+	struct extcon_dev *edev;
+	int ret;
+
+	INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
+
+	if (!of_property_read_bool(ctx->dev->of_node, "extcon")) {
+		dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n");
+		ctx->extcon_attached = true;
+		return 0;
+	}
+
+	edev = extcon_get_edev_by_phandle(ctx->dev, 0);
+	if (IS_ERR(edev)) {
+		if (PTR_ERR(edev) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_err(ctx->dev, "Invalid or missing extcon\n");
+		return PTR_ERR(edev);
+	}
+
+	ctx->extcon_attached = extcon_get_cable_state_(edev, EXTCON_DISP_MHL);
+	dev_info(ctx->dev, "extcon(MHL) = %d\n", ctx->extcon_attached);
+
+	ctx->extcon = edev;
+	ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
+	ret = devm_extcon_register_notifier(ctx->dev, edev,
+					EXTCON_DISP_MHL, &ctx->extcon_nb);
+
+	if (ret) {
+		dev_err(ctx->dev, "failed to register notifier for MHL\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct sii8620, bridge);
@@ -2201,13 +2272,20 @@ static int sii8620_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	ret = sii8620_extcon_init(ctx);
+	if (ret < 0) {
+		dev_err(ctx->dev, "failed to initialize EXTCON\n");
+		return ret;
+	}
+
 	i2c_set_clientdata(client, ctx);
 
 	ctx->bridge.funcs = &sii8620_bridge_funcs;
 	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
-	sii8620_cable_in(ctx);
+	if (ctx->extcon_attached)
+		sii8620_cable_in(ctx);
 
 	return 0;
 }
@@ -2216,7 +2294,8 @@ static int sii8620_remove(struct i2c_client *client)
 {
 	struct sii8620 *ctx = i2c_get_clientdata(client);
 
-	disable_irq(to_i2c_client(ctx->dev)->irq);
+	if (!ctx->extcon_attached)
+		sii8620_cable_out(ctx);
 	drm_bridge_remove(&ctx->bridge);
 	sii8620_hw_off(ctx);
 
-- 
2.7.4

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

[PATCH 2/2] arm64: dts: exynos: add extcon phandle

[Maciej Purski]

Sii8620 driver can now use extcon in order to switch on and off
the device.

Signed-off-by: Maciej Purski <m.purski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
index 0943dec..ed991b3 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
@@ -784,6 +784,7 @@
 		reset-gpios = <&gpv7 0 GPIO_ACTIVE_LOW>;
 		clocks = <&pmu_system_controller 0>;
 		clock-names = "xtal";
+		extcon = <&muic>;
 
 		port {
 			mhl_to_hdmi: endpoint {
-- 
2.7.4

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

Re: [PATCH 1/2] drm/bridge/sii8620: add external connector handle

[Laurent Pinchart]

Hi Maciej,

Thank you for the patch.

On Thursday 10 Aug 2017 15:25:29 Maciej Purski wrote:
> The driver should be switched on if an external connector is plugged and
> switched off if it is unplugged. Extcon is optional. If it is not found,
> the driver stays in "always-on" mode.
> 
> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> ---
>  .../bindings/display/bridge/sil-sii8620.txt        |  4 ++
>  drivers/gpu/drm/bridge/sil-sii8620.c               | 83 ++++++++++++++++++-
>  2 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt index
> 9409d9c..1f230bf 100644
> --- a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> @@ -10,6 +10,9 @@ Required properties:
>  	- clocks, clock-names: specification and name of "xtal" clock
>  	- video interfaces: Device node can contain video interface port
>  			    node for HDMI encoder according to [1].
> +Optional properties:
> +	- extcon: phandle to external connector for MHL cable changes
> +		  detection

The sii8620 DT node should model its connection to the MHL connector using OF 
graph, connecting a port to the MHL connector DT node through endpoints. I 
believe the extcon property should then be added to the MHL connector DT node, 
not the bridge.

>  [1]: Documentation/devicetree/bindings/media/video-interfaces.txt
> 
> @@ -24,6 +27,7 @@ Example:
>  		reset-gpio = <&gpv7 0 0>;
>  		clocks = <&pmu_system_controller 0>;
>  		clock-names = "xtal";
> +		extcon = <&muic>;
> 
>  		port {
>  			mhl_to_hdmi: endpoint {
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> b/drivers/gpu/drm/bridge/sil-sii8620.c index 2d51a22..5002654 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -17,6 +17,7 @@
> 
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/extcon.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> @@ -78,6 +79,10 @@ struct sii8620 {
>  	struct edid *edid;
>  	unsigned int gen2_write_burst:1;
>  	enum sii8620_mt_state mt_state;
> +	struct extcon_dev *extcon;
> +	struct notifier_block extcon_nb;
> +	struct work_struct extcon_wq;
> +	bool extcon_attached;
>  	struct list_head mt_queue;
>  	struct {
>  		int r_size;
> @@ -2102,6 +2107,72 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>  	enable_irq(to_i2c_client(ctx->dev)->irq);
>  }
> 
> +static void sii8620_cable_out(struct sii8620 *ctx)
> +{
> +	disable_irq(to_i2c_client(ctx->dev)->irq);
> +	sii8620_hw_off(ctx);
> +}
> +
> +static void sii8620_extcon_work(struct work_struct *work)
> +{
> +	struct sii8620 *ctx =
> +		container_of(work, struct sii8620, extcon_wq);
> +
> +	if (ctx->extcon_attached)
> +		sii8620_cable_in(ctx);
> +	else
> +		sii8620_cable_out(ctx);
> +}
> +
> +static int sii8620_extcon_notifier(struct notifier_block *self,
> +			unsigned long event, void *ptr)
> +{
> +	struct sii8620 *ctx =
> +		container_of(self, struct sii8620, extcon_nb);
> +
> +	ctx->extcon_attached = event;
> +	schedule_work(&ctx->extcon_wq);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int sii8620_extcon_init(struct sii8620 *ctx)
> +{
> +	struct extcon_dev *edev;
> +	int ret;
> +
> +	INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
> +
> +	if (!of_property_read_bool(ctx->dev->of_node, "extcon")) {
> +		dev_info(ctx->dev, "no extcon found, switching to 'always on' 
mode\n");
> +		ctx->extcon_attached = true;
> +		return 0;
> +	}
> +
> +	edev = extcon_get_edev_by_phandle(ctx->dev, 0);
> +	if (IS_ERR(edev)) {
> +		if (PTR_ERR(edev) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		dev_err(ctx->dev, "Invalid or missing extcon\n");
> +		return PTR_ERR(edev);
> +	}
> +
> +	ctx->extcon_attached = extcon_get_cable_state_(edev, EXTCON_DISP_MHL);
> +	dev_info(ctx->dev, "extcon(MHL) = %d\n", ctx->extcon_attached);
> +
> +	ctx->extcon = edev;
> +	ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
> +	ret = devm_extcon_register_notifier(ctx->dev, edev,
> +					EXTCON_DISP_MHL, &ctx->extcon_nb);
> +
> +	if (ret) {
> +		dev_err(ctx->dev, "failed to register notifier for MHL\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sii8620, bridge);
> @@ -2201,13 +2272,20 @@ static int sii8620_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
> 
> +	ret = sii8620_extcon_init(ctx);
> +	if (ret < 0) {
> +		dev_err(ctx->dev, "failed to initialize EXTCON\n");
> +		return ret;
> +	}
> +
>  	i2c_set_clientdata(client, ctx);
> 
>  	ctx->bridge.funcs = &sii8620_bridge_funcs;
>  	ctx->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&ctx->bridge);
> 
> -	sii8620_cable_in(ctx);
> +	if (ctx->extcon_attached)
> +		sii8620_cable_in(ctx);
> 
>  	return 0;
>  }
> @@ -2216,7 +2294,8 @@ static int sii8620_remove(struct i2c_client *client)
>  {
>  	struct sii8620 *ctx = i2c_get_clientdata(client);
> 
> -	disable_irq(to_i2c_client(ctx->dev)->irq);
> +	if (!ctx->extcon_attached)
> +		sii8620_cable_out(ctx);
>  	drm_bridge_remove(&ctx->bridge);
>  	sii8620_hw_off(ctx);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] drm/bridge/sii8620: add external connector handle

[Marek Szyprowski]

Hi Laurent,

On 2017-08-10 15:39, Laurent Pinchart wrote:
> Hi Maciej,
>
> Thank you for the patch.
>
> On Thursday 10 Aug 2017 15:25:29 Maciej Purski wrote:
>> The driver should be switched on if an external connector is plugged and
>> switched off if it is unplugged. Extcon is optional. If it is not found,
>> the driver stays in "always-on" mode.
>>
>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>> ---
>>   .../bindings/display/bridge/sil-sii8620.txt        |  4 ++
>>   drivers/gpu/drm/bridge/sil-sii8620.c               | 83 ++++++++++++++++++-
>>   2 files changed, 85 insertions(+), 2 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
>> b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt index
>> 9409d9c..1f230bf 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
>> @@ -10,6 +10,9 @@ Required properties:
>>   	- clocks, clock-names: specification and name of "xtal" clock
>>   	- video interfaces: Device node can contain video interface port
>>   			    node for HDMI encoder according to [1].
>> +Optional properties:
>> +	- extcon: phandle to external connector for MHL cable changes
>> +		  detection
> The sii8620 DT node should model its connection to the MHL connector using OF
> graph, connecting a port to the MHL connector DT node through endpoints. I
> believe the extcon property should then be added to the MHL connector DT node,
> not the bridge.

I don't get this. Generic extcon bindings doesn't define connector DT 
node, so
I don't see how could we use endpoint DT approach here. Could you give the
example?

>>   [1]: Documentation/devicetree/bindings/media/video-interfaces.txt
>>
>> @@ -24,6 +27,7 @@ Example:
>>   		reset-gpio = <&gpv7 0 0>;
>>   		clocks = <&pmu_system_controller 0>;
>>   		clock-names = "xtal";
>> +		extcon = <&muic>;
>>
>>   		port {
>>   			mhl_to_hdmi: endpoint {
>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
>> b/drivers/gpu/drm/bridge/sil-sii8620.c index 2d51a22..5002654 100644
>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
>> @@ -17,6 +17,7 @@
>>
>>   #include <linux/clk.h>
>>   #include <linux/delay.h>
>> +#include <linux/extcon.h>
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/i2c.h>
>>   #include <linux/interrupt.h>
>> @@ -78,6 +79,10 @@ struct sii8620 {
>>   	struct edid *edid;
>>   	unsigned int gen2_write_burst:1;
>>   	enum sii8620_mt_state mt_state;
>> +	struct extcon_dev *extcon;
>> +	struct notifier_block extcon_nb;
>> +	struct work_struct extcon_wq;
>> +	bool extcon_attached;
>>   	struct list_head mt_queue;
>>   	struct {
>>   		int r_size;
>> @@ -2102,6 +2107,72 @@ static void sii8620_cable_in(struct sii8620 *ctx)
>>   	enable_irq(to_i2c_client(ctx->dev)->irq);
>>   }
>>
>> +static void sii8620_cable_out(struct sii8620 *ctx)
>> +{
>> +	disable_irq(to_i2c_client(ctx->dev)->irq);
>> +	sii8620_hw_off(ctx);
>> +}
>> +
>> +static void sii8620_extcon_work(struct work_struct *work)
>> +{
>> +	struct sii8620 *ctx =
>> +		container_of(work, struct sii8620, extcon_wq);
>> +
>> +	if (ctx->extcon_attached)
>> +		sii8620_cable_in(ctx);
>> +	else
>> +		sii8620_cable_out(ctx);
>> +}
>> +
>> +static int sii8620_extcon_notifier(struct notifier_block *self,
>> +			unsigned long event, void *ptr)
>> +{
>> +	struct sii8620 *ctx =
>> +		container_of(self, struct sii8620, extcon_nb);
>> +
>> +	ctx->extcon_attached = event;
>> +	schedule_work(&ctx->extcon_wq);
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static int sii8620_extcon_init(struct sii8620 *ctx)
>> +{
>> +	struct extcon_dev *edev;
>> +	int ret;
>> +
>> +	INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
>> +
>> +	if (!of_property_read_bool(ctx->dev->of_node, "extcon")) {
>> +		dev_info(ctx->dev, "no extcon found, switching to 'always on'
> mode\n");
>> +		ctx->extcon_attached = true;
>> +		return 0;
>> +	}
>> +
>> +	edev = extcon_get_edev_by_phandle(ctx->dev, 0);
>> +	if (IS_ERR(edev)) {
>> +		if (PTR_ERR(edev) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;
>> +		dev_err(ctx->dev, "Invalid or missing extcon\n");
>> +		return PTR_ERR(edev);
>> +	}
>> +
>> +	ctx->extcon_attached = extcon_get_cable_state_(edev, EXTCON_DISP_MHL);
>> +	dev_info(ctx->dev, "extcon(MHL) = %d\n", ctx->extcon_attached);
>> +
>> +	ctx->extcon = edev;
>> +	ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
>> +	ret = devm_extcon_register_notifier(ctx->dev, edev,
>> +					EXTCON_DISP_MHL, &ctx->extcon_nb);
>> +
>> +	if (ret) {
>> +		dev_err(ctx->dev, "failed to register notifier for MHL\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
>>   {
>>   	return container_of(bridge, struct sii8620, bridge);
>> @@ -2201,13 +2272,20 @@ static int sii8620_probe(struct i2c_client *client,
>>   	if (ret)
>>   		return ret;
>>
>> +	ret = sii8620_extcon_init(ctx);
>> +	if (ret < 0) {
>> +		dev_err(ctx->dev, "failed to initialize EXTCON\n");
>> +		return ret;
>> +	}
>> +
>>   	i2c_set_clientdata(client, ctx);
>>
>>   	ctx->bridge.funcs = &sii8620_bridge_funcs;
>>   	ctx->bridge.of_node = dev->of_node;
>>   	drm_bridge_add(&ctx->bridge);
>>
>> -	sii8620_cable_in(ctx);
>> +	if (ctx->extcon_attached)
>> +		sii8620_cable_in(ctx);
>>
>>   	return 0;
>>   }
>> @@ -2216,7 +2294,8 @@ static int sii8620_remove(struct i2c_client *client)
>>   {
>>   	struct sii8620 *ctx = i2c_get_clientdata(client);
>>
>> -	disable_irq(to_i2c_client(ctx->dev)->irq);
>> +	if (!ctx->extcon_attached)
>> +		sii8620_cable_out(ctx);
>>   	drm_bridge_remove(&ctx->bridge);
>>   	sii8620_hw_off(ctx);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/2] drm/bridge/sii8620: add external connector handle

[Laurent Pinchart]

Hi Marek,

On Friday 11 Aug 2017 08:39:31 Marek Szyprowski wrote:
> On 2017-08-10 15:39, Laurent Pinchart wrote:
> > On Thursday 10 Aug 2017 15:25:29 Maciej Purski wrote:
> >> The driver should be switched on if an external connector is plugged and
> >> switched off if it is unplugged. Extcon is optional. If it is not found,
> >> the driver stays in "always-on" mode.
> >> 
> >> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> >> ---
> >> 
> >>   .../bindings/display/bridge/sil-sii8620.txt        |  4 ++
> >>   drivers/gpu/drm/bridge/sil-sii8620.c               | 83 ++++++++++++++-
> >>   2 files changed, 85 insertions(+), 2 deletions(-)
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> >> b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt index
> >> 9409d9c..1f230bf 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> >> +++ b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> >> @@ -10,6 +10,9 @@ Required properties:
> >>   	- clocks, clock-names: specification and name of "xtal" clock
> >>   	- video interfaces: Device node can contain video interface port
> >>   			    node for HDMI encoder according to [1].
> >> 
> >> +Optional properties:
> >> +	- extcon: phandle to external connector for MHL cable changes
> >> +		  detection
> > 
> > The sii8620 DT node should model its connection to the MHL connector using
> > OF graph, connecting a port to the MHL connector DT node through
> > endpoints. I believe the extcon property should then be added to the MHL
> > connector DT node, not the bridge.
> 
> I don't get this. Generic extcon bindings doesn't define connector DT
> node, so I don't see how could we use endpoint DT approach here. Could you
> give the example?

Sure, here you are.

&hsi2c_7 {
	...

        sii8620@39 {
                reg = <0x39>;
                compatible = "sil,sii8620";
                cvcc10-supply = <&ldo36_reg>;
                iovcc18-supply = <&ldo34_reg>;
                interrupt-parent = <&gpf0>;
                interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
                reset-gpios = <&gpv7 0 GPIO_ACTIVE_LOW>;
                clocks = <&pmu_system_controller 0>;
                clock-names = "xtal";

		ports {
			#address-cells = <1>;
			#size-cells = <0>;

	                port@0 {
				reg = <0>;
        	                mhl_to_hdmi: endpoint {
                	                remote-endpoint = <&hdmi_to_mhl>;
	                        };
        	        };
	                port@1 {
				reg = <1>;
        	                mhl_bridge_out: endpoint {
                	                remote-endpoint = <&mhl_connector_in>;
	                        };
        	        };
		};
        };
};

/ {
	mhl-connector {
		compatible = "mhl-connector";
		extcon = <&muic>;

	        port {
        	        mhl_connector_in: endpoint {
                	        remote-endpoint = <&mhl_bridge_out>;
	                };
        	};
	};
};

Note that there's currently no DT binding for MHL connectors. One needs to be 
developed, the HDMI connector DT binding can be used as a source of 
inspiration.

> >>   [1]: Documentation/devicetree/bindings/media/video-interfaces.txt

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] drm/bridge/sii8620: add external connector handle

[Marek Szyprowski]

Hi Laurent,

On 2017-08-11 11:40, Laurent Pinchart wrote:
> Hi Marek,
>
> On Friday 11 Aug 2017 08:39:31 Marek Szyprowski wrote:
>> On 2017-08-10 15:39, Laurent Pinchart wrote:
>>> On Thursday 10 Aug 2017 15:25:29 Maciej Purski wrote:
>>>> The driver should be switched on if an external connector is plugged and
>>>> switched off if it is unplugged. Extcon is optional. If it is not found,
>>>> the driver stays in "always-on" mode.
>>>>
>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
>>>> ---
>>>>
>>>>    .../bindings/display/bridge/sil-sii8620.txt        |  4 ++
>>>>    drivers/gpu/drm/bridge/sil-sii8620.c               | 83 ++++++++++++++-
>>>>    2 files changed, 85 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
>>>> b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt index
>>>> 9409d9c..1f230bf 100644
>>>> --- a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
>>>> +++ b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
>>>> @@ -10,6 +10,9 @@ Required properties:
>>>>    	- clocks, clock-names: specification and name of "xtal" clock
>>>>    	- video interfaces: Device node can contain video interface port
>>>>    			    node for HDMI encoder according to [1].
>>>>
>>>> +Optional properties:
>>>> +	- extcon: phandle to external connector for MHL cable changes
>>>> +		  detection
>>> The sii8620 DT node should model its connection to the MHL connector using
>>> OF graph, connecting a port to the MHL connector DT node through
>>> endpoints. I believe the extcon property should then be added to the MHL
>>> connector DT node, not the bridge.
>> I don't get this. Generic extcon bindings doesn't define connector DT
>> node, so I don't see how could we use endpoint DT approach here. Could you
>> give the example?
> Sure, here you are.
>
> &hsi2c_7 {
> 	...
>
>          sii8620@39 {
>                  reg = <0x39>;
>                  compatible = "sil,sii8620";
>                  cvcc10-supply = <&ldo36_reg>;
>                  iovcc18-supply = <&ldo34_reg>;
>                  interrupt-parent = <&gpf0>;
>                  interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
>                  reset-gpios = <&gpv7 0 GPIO_ACTIVE_LOW>;
>                  clocks = <&pmu_system_controller 0>;
>                  clock-names = "xtal";
>
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
>
> 	                port@0 {
> 				reg = <0>;
>          	                mhl_to_hdmi: endpoint {
>                  	                remote-endpoint = <&hdmi_to_mhl>;
> 	                        };
>          	        };
> 	                port@1 {
> 				reg = <1>;
>          	                mhl_bridge_out: endpoint {
>                  	                remote-endpoint = <&mhl_connector_in>;
> 	                        };
>          	        };
> 		};
>          };
> };
>
> / {
> 	mhl-connector {
> 		compatible = "mhl-connector";
> 		extcon = <&muic>;
>
> 	        port {
>          	        mhl_connector_in: endpoint {
>                  	        remote-endpoint = <&mhl_bridge_out>;
> 	                };
>          	};
> 	};
> };
>
> Note that there's currently no DT binding for MHL connectors. One needs to be
> developed, the HDMI connector DT binding can be used as a source of
> inspiration.

Ah, I see. I wasn't aware that display subsystem has bindings even for 
the connectors.
Other types of connectors already used together with extcon in device 
tree (like usb
and chargers) don't have dedicated bindings.

Do we need to add hdmi-connectors to all existing boards that have HDMI 
module?

>>>>    [1]: Documentation/devicetree/bindings/media/video-interfaces.txt
>>>>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/2] drm/bridge/sii8620: add external connector handle

[Rob Herring]

On Fri, Aug 11, 2017 at 02:49:35PM +0200, Marek Szyprowski wrote:
> Hi Laurent,
> 
> On 2017-08-11 11:40, Laurent Pinchart wrote:
> > Hi Marek,
> > 
> > On Friday 11 Aug 2017 08:39:31 Marek Szyprowski wrote:
> > > On 2017-08-10 15:39, Laurent Pinchart wrote:
> > > > On Thursday 10 Aug 2017 15:25:29 Maciej Purski wrote:
> > > > > The driver should be switched on if an external connector is plugged and
> > > > > switched off if it is unplugged. Extcon is optional. If it is not found,
> > > > > the driver stays in "always-on" mode.
> > > > > 
> > > > > Signed-off-by: Maciej Purski <m.purski@samsung.com>
> > > > > ---
> > > > > 
> > > > >    .../bindings/display/bridge/sil-sii8620.txt        |  4 ++
> > > > >    drivers/gpu/drm/bridge/sil-sii8620.c               | 83 ++++++++++++++-
> > > > >    2 files changed, 85 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> > > > > b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt index
> > > > > 9409d9c..1f230bf 100644
> > > > > --- a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> > > > > +++ b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> > > > > @@ -10,6 +10,9 @@ Required properties:
> > > > >    	- clocks, clock-names: specification and name of "xtal" clock
> > > > >    	- video interfaces: Device node can contain video interface port
> > > > >    			    node for HDMI encoder according to [1].
> > > > > 
> > > > > +Optional properties:
> > > > > +	- extcon: phandle to external connector for MHL cable changes
> > > > > +		  detection
> > > > The sii8620 DT node should model its connection to the MHL connector using
> > > > OF graph, connecting a port to the MHL connector DT node through
> > > > endpoints. I believe the extcon property should then be added to the MHL
> > > > connector DT node, not the bridge.
> > > I don't get this. Generic extcon bindings doesn't define connector DT
> > > node, so I don't see how could we use endpoint DT approach here. Could you
> > > give the example?
> > Sure, here you are.
> > 
> > &hsi2c_7 {
> > 	...
> > 
> >          sii8620@39 {
> >                  reg = <0x39>;
> >                  compatible = "sil,sii8620";
> >                  cvcc10-supply = <&ldo36_reg>;
> >                  iovcc18-supply = <&ldo34_reg>;
> >                  interrupt-parent = <&gpf0>;
> >                  interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> >                  reset-gpios = <&gpv7 0 GPIO_ACTIVE_LOW>;
> >                  clocks = <&pmu_system_controller 0>;
> >                  clock-names = "xtal";
> > 
> > 		ports {
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 	                port@0 {
> > 				reg = <0>;
> >          	                mhl_to_hdmi: endpoint {
> >                  	                remote-endpoint = <&hdmi_to_mhl>;
> > 	                        };
> >          	        };
> > 	                port@1 {
> > 				reg = <1>;
> >          	                mhl_bridge_out: endpoint {
> >                  	                remote-endpoint = <&mhl_connector_in>;
> > 	                        };
> >          	        };
> > 		};
> >          };
> > };
> > 
> > / {
> > 	mhl-connector {
> > 		compatible = "mhl-connector";
> > 		extcon = <&muic>;
> > 
> > 	        port {
> >          	        mhl_connector_in: endpoint {
> >                  	        remote-endpoint = <&mhl_bridge_out>;
> > 	                };
> >          	};
> > 	};
> > };
> > 
> > Note that there's currently no DT binding for MHL connectors. One needs to be
> > developed, the HDMI connector DT binding can be used as a source of
> > inspiration.
> 
> Ah, I see. I wasn't aware that display subsystem has bindings even for the
> connectors.
> Other types of connectors already used together with extcon in device tree
> (like usb
> and chargers) don't have dedicated bindings.

I NAK any extcon binding additions because it is poorly designed, so 
don't use extcon. It is full of Linuxisms and needs work. USB 
connectors need to be described like what we have for display related 
connectors especially since they are merging.

> 
> Do we need to add hdmi-connectors to all existing boards that have HDMI
> module?

Ideally, yes.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/bridge/sii8620: add external connector handle

[Laurent Pinchart]

Hi Marek,

On Thursday 17 Aug 2017 10:11:02 Rob Herring wrote:
> On Fri, Aug 11, 2017 at 02:49:35PM +0200, Marek Szyprowski wrote:
> > On 2017-08-11 11:40, Laurent Pinchart wrote:
> >> On Friday 11 Aug 2017 08:39:31 Marek Szyprowski wrote:
> >>> On 2017-08-10 15:39, Laurent Pinchart wrote:
> >>>> On Thursday 10 Aug 2017 15:25:29 Maciej Purski wrote:
> >>>>> The driver should be switched on if an external connector is
> >>>>> plugged and switched off if it is unplugged. Extcon is optional.
> >>>>> If it is not found, the driver stays in "always-on" mode.
> >>>>> 
> >>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> >>>>> ---
> >>>>> 
> >>>>> .../bindings/display/bridge/sil-sii8620.txt        |  4 ++
> >>>>> drivers/gpu/drm/bridge/sil-sii8620.c               | 83 ++++++++++++-
> >>>>> 2 files changed, 85 insertions(+), 2 deletions(-)
> >>>>> 
> >>>>> diff --git
> >>>>> a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> >>>>> b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> >>>>> index
> >>>>> 9409d9c..1f230bf 100644
> >>>>> --- a/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt
> >>>>> +++ b/Documentation/devicetree/bindings/display/bridge/sil-sii8620.txt 
> >>>>> @@ -10,6 +10,9 @@ Required properties:
> >>>>>    	- clocks, clock-names: specification and name of "xtal" clock
> >>>>>    	- video interfaces: Device node can contain video interface
> >>>>>    	  port node for HDMI encoder according to [1].
> >>>>> 
> >>>>> +Optional properties:
> >>>>> +	- extcon: phandle to external connector for MHL cable changes
> >>>>> +		  detection
> >>>> 
> >>>> The sii8620 DT node should model its connection to the MHL connector
> >>>> using OF graph, connecting a port to the MHL connector DT node through
> >>>> endpoints. I believe the extcon property should then be added to the
> >>>> MHL connector DT node, not the bridge.
> >>> 
> >>> I don't get this. Generic extcon bindings doesn't define connector DT
> >>> node, so I don't see how could we use endpoint DT approach here. Could
> >>> you give the example?
> >> 
> >> Sure, here you are.
> >> 
> >> &hsi2c_7 {
> >> 	...
> >>          sii8620@39 {
> >>                  reg = <0x39>;
> >>                  compatible = "sil,sii8620";
> >>                  cvcc10-supply = <&ldo36_reg>;
> >>                  iovcc18-supply = <&ldo34_reg>;
> >>                  interrupt-parent = <&gpf0>;
> >>                  interrupts = <2 IRQ_TYPE_LEVEL_HIGH>;
> >>                  reset-gpios = <&gpv7 0 GPIO_ACTIVE_LOW>;
> >>                  clocks = <&pmu_system_controller 0>;
> >>                  clock-names = "xtal";
> >> 		
> >> 		ports {
> >> 			#address-cells = <1>;
> >> 			#size-cells = <0>;
> >> 			
> >> 	                port@0 {
> >> 				reg = <0>;
> >>          	                mhl_to_hdmi: endpoint {
> >>                  	                remote-endpoint = <&hdmi_to_mhl>;
> >> 	                        };
> >>          	        };
> >> 	                port@1 {
> >> 				reg = <1>;
> >>          	                mhl_bridge_out: endpoint {
> >>                  	                remote-endpoint = <&mhl_connector_in>;
> >> 	                        };
> >>          	        };
> >> 		};
> >>          };
> >> };
> >> 
> >> / {
> >> 	mhl-connector {
> >> 		compatible = "mhl-connector";
> >> 		extcon = <&muic>;
> >> 		
> >> 	        port {
> >>          	        mhl_connector_in: endpoint {
> >>                  	        remote-endpoint = <&mhl_bridge_out>;
> >> 	                };
> >>          	};
> >> 	};
> >> };
> >> 
> >> Note that there's currently no DT binding for MHL connectors. One needs
> >> to be developed, the HDMI connector DT binding can be used as a source
> >> of inspiration.
> > 
> > Ah, I see. I wasn't aware that display subsystem has bindings even for the
> > connectors. Other types of connectors already used together with extcon in
> > device tree (like usb and chargers) don't have dedicated bindings.
> 
> I NAK any extcon binding additions because it is poorly designed, so
> don't use extcon. It is full of Linuxisms and needs work. USB
> connectors need to be described like what we have for display related
> connectors especially since they are merging.
> 
> > Do we need to add hdmi-connectors to all existing boards that have HDMI
> > module?
> 
> Ideally, yes.

I agree. In practice "details" such as backward compatibility need to be taken 
into account, but it's a good practice to update the mainline device tree 
sources to the recommended bindings.

-- 
Regards,

Laurent Pinchart