diff mbox

[v6,05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

Message ID 1405587689-1466-6-git-send-email-yj44.cho@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

YoungJun Cho July 17, 2014, 9:01 a.m. UTC
To support LCD I80 interface, the DSI host should register
TE interrupt handler from the TE GPIO of attached panel.
So the panel generates a tearing effect synchronization signal
then the DSI host calls the CRTC device manager to trigger
to transfer video image.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 2 deletions(-)

Comments

Andrzej Hajda July 21, 2014, 2:01 p.m. UTC | #1
On 07/17/2014 11:01 AM, YoungJun Cho wrote:
> To support LCD I80 interface, the DSI host should register
> TE interrupt handler from the TE GPIO of attached panel.
> So the panel generates a tearing effect synchronization signal
> then the DSI host calls the CRTC device manager to trigger
> to transfer video image.
> 

This whole patch seems to be a hack.

I think it is not a good idea to parse property of one device by another
device's driver.

Especially here TE GPIO belongs to the panel. The panel driver should
know how to configure it and how to use it or not. The panel driver will
generate TE signal based on this GPIO, DSI/BTA mechanism or any other
mechanism provided by the panel.
TE signal should be delivered to the display controller. The only role
of DSIM here is that it is between the panel and the display controller
so it should be used to route this signal to DC.

According to specs TE signal should/can be generated by DBI and DSI
command mode panels, so its handling should be more generic, not Exynos
specific.

Regards
Andrzej

> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
> Acked-by: Inki Dae <inki.dae@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 58bfb2a..4997bfe 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -16,7 +16,9 @@
>  #include <drm/drm_panel.h>
>  
>  #include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/irq.h>
> +#include <linux/of_gpio.h>
>  #include <linux/phy/phy.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/component.h>
> @@ -24,6 +26,7 @@
>  #include <video/mipi_display.h>
>  #include <video/videomode.h>
>  
> +#include "exynos_drm_crtc.h"
>  #include "exynos_drm_drv.h"
>  
>  /* returns true iff both arguments logically differs */
> @@ -247,6 +250,7 @@ struct exynos_dsi {
>  	struct clk *bus_clk;
>  	struct regulator_bulk_data supplies[2];
>  	int irq;
> +	int te_gpio;
>  
>  	u32 pll_clk_rate;
>  	u32 burst_clk_rate;
> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
> +{
> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
> +	struct drm_encoder *encoder = dsi->encoder;
> +
> +	if (dsi->state & DSIM_STATE_ENABLED)
> +		exynos_drm_crtc_te_handler(encoder->crtc);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
> +{
> +	enable_irq(dsi->irq);
> +
> +	if (gpio_is_valid(dsi->te_gpio))
> +		enable_irq(gpio_to_irq(dsi->te_gpio));
> +}
> +
> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
> +{
> +	if (gpio_is_valid(dsi->te_gpio))
> +		disable_irq(gpio_to_irq(dsi->te_gpio));
> +
> +	disable_irq(dsi->irq);
> +}
> +
>  static int exynos_dsi_init(struct exynos_dsi *dsi)
>  {
>  	exynos_dsi_enable_clock(dsi);
>  	exynos_dsi_reset(dsi);
> -	enable_irq(dsi->irq);
> +	exynos_dsi_enable_irq(dsi);
>  	exynos_dsi_wait_for_reset(dsi);
>  	exynos_dsi_init_link(dsi);
>  
>  	return 0;
>  }
>  
> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
> +{
> +	int ret;
> +
> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
> +	if (!gpio_is_valid(dsi->te_gpio)) {
> +		dev_err(dsi->dev, "no te-gpios specified\n");
> +		ret = dsi->te_gpio;
> +		goto out;
> +	}
> +
> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
> +	if (ret) {
> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
> +		goto out;
> +	}
> +
> +	/*
> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
> +	 * called by drm_panel_init() before panel is attached.
> +	 */
> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
> +					exynos_dsi_te_irq_handler, NULL,
> +					IRQF_TRIGGER_RISING, "TE", dsi);
> +	if (ret) {
> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
> +		gpio_free(dsi->te_gpio);
> +		goto out;
> +	}
> +
> +out:
> +	return ret;
> +}
> +
> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
> +{
> +	if (gpio_is_valid(dsi->te_gpio)) {
> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
> +		gpio_free(dsi->te_gpio);
> +		dsi->te_gpio = -ENOENT;
> +	}
> +}
> +
>  static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>  				  struct mipi_dsi_device *device)
>  {
> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>  	if (dsi->connector.dev)
>  		drm_helper_hpd_irq_event(dsi->connector.dev);
>  
> +	/*
> +	 * If attached panel device is for command mode one, dsi should
> +	 * register TE interrupt handler.
> +	 */
> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
> +		int ret = exynos_dsi_register_te_irq(dsi);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>  {
>  	struct exynos_dsi *dsi = host_to_dsi(host);
>  
> +	exynos_dsi_unregister_te_irq(dsi);
> +
>  	dsi->panel_node = NULL;
>  
>  	if (dsi->connector.dev)
> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>  
>  		exynos_dsi_disable_clock(dsi);
>  
> -		disable_irq(dsi->irq);
> +		exynos_dsi_disable_irq(dsi);
>  	}
>  
>  	dsi->state &= ~DSIM_STATE_CMD_LPM;
> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  		goto err_del_component;
>  	}
>  
> +	/* To be checked as invalid one */
> +	dsi->te_gpio = -ENOENT;
> +
>  	init_completion(&dsi->completed);
>  	spin_lock_init(&dsi->transfer_lock);
>  	INIT_LIST_HEAD(&dsi->transfer_list);
>
Inki Dae July 22, 2014, 1:23 a.m. UTC | #2
On 2014? 07? 21? 23:01, Andrzej Hajda wrote:
> On 07/17/2014 11:01 AM, YoungJun Cho wrote:
>> To support LCD I80 interface, the DSI host should register
>> TE interrupt handler from the TE GPIO of attached panel.
>> So the panel generates a tearing effect synchronization signal
>> then the DSI host calls the CRTC device manager to trigger
>> to transfer video image.
>>
> 
> This whole patch seems to be a hack.
> 
> I think it is not a good idea to parse property of one device by another
> device's driver.
> 
> Especially here TE GPIO belongs to the panel. The panel driver should
> know how to configure it and how to use it or not. The panel driver will
> generate TE signal based on this GPIO, DSI/BTA mechanism or any other
> mechanism provided by the panel.
> TE signal should be delivered to the display controller. The only role
> of DSIM here is that it is between the panel and the display controller
> so it should be used to route this signal to DC.
> 
> According to specs TE signal should/can be generated by DBI and DSI
> command mode panels, so its handling should be more generic, not Exynos
> specific.
> 

Right. However, it seems that there are no much users using command mode
panel so we would need more times to discuss the generic way. I think we
can have this feature in specific to Exynos drm - it doesn't affect
other SoC -.  Actually, I know OMAP people handle this feature in a way
specific to OMAP SoC. If other users need more generic way to this
feature then we could have a discussion about the generic way at that time.

That is why Mr. Cho implemented TE feature like this. Do you have more
good idea? I wish the idea would be specific so that Mr. Cho can
implement it.

P.s. Thierry already opposed the use of mipi_dsi_host_ops, I agree with
him. And also it seems not good to use crtc and encoder/connector
because these frameworks are common to all architecture including x86 so
other architectures wouldn't need TE feature.

Thanks,
Inki Dae

> Regards
> Andrzej
> 
>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>> Acked-by: Inki Dae <inki.dae@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 93 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> index 58bfb2a..4997bfe 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> @@ -16,7 +16,9 @@
>>  #include <drm/drm_panel.h>
>>  
>>  #include <linux/clk.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/irq.h>
>> +#include <linux/of_gpio.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/component.h>
>> @@ -24,6 +26,7 @@
>>  #include <video/mipi_display.h>
>>  #include <video/videomode.h>
>>  
>> +#include "exynos_drm_crtc.h"
>>  #include "exynos_drm_drv.h"
>>  
>>  /* returns true iff both arguments logically differs */
>> @@ -247,6 +250,7 @@ struct exynos_dsi {
>>  	struct clk *bus_clk;
>>  	struct regulator_bulk_data supplies[2];
>>  	int irq;
>> +	int te_gpio;
>>  
>>  	u32 pll_clk_rate;
>>  	u32 burst_clk_rate;
>> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
>> +	struct drm_encoder *encoder = dsi->encoder;
>> +
>> +	if (dsi->state & DSIM_STATE_ENABLED)
>> +		exynos_drm_crtc_te_handler(encoder->crtc);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
>> +{
>> +	enable_irq(dsi->irq);
>> +
>> +	if (gpio_is_valid(dsi->te_gpio))
>> +		enable_irq(gpio_to_irq(dsi->te_gpio));
>> +}
>> +
>> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
>> +{
>> +	if (gpio_is_valid(dsi->te_gpio))
>> +		disable_irq(gpio_to_irq(dsi->te_gpio));
>> +
>> +	disable_irq(dsi->irq);
>> +}
>> +
>>  static int exynos_dsi_init(struct exynos_dsi *dsi)
>>  {
>>  	exynos_dsi_enable_clock(dsi);
>>  	exynos_dsi_reset(dsi);
>> -	enable_irq(dsi->irq);
>> +	exynos_dsi_enable_irq(dsi);
>>  	exynos_dsi_wait_for_reset(dsi);
>>  	exynos_dsi_init_link(dsi);
>>  
>>  	return 0;
>>  }
>>  
>> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
>> +{
>> +	int ret;
>> +
>> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
>> +	if (!gpio_is_valid(dsi->te_gpio)) {
>> +		dev_err(dsi->dev, "no te-gpios specified\n");
>> +		ret = dsi->te_gpio;
>> +		goto out;
>> +	}
>> +
>> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>> +	if (ret) {
>> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
>> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>> +	 * called by drm_panel_init() before panel is attached.
>> +	 */
>> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>> +					exynos_dsi_te_irq_handler, NULL,
>> +					IRQF_TRIGGER_RISING, "TE", dsi);
>> +	if (ret) {
>> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
>> +		gpio_free(dsi->te_gpio);
>> +		goto out;
>> +	}
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>> +{
>> +	if (gpio_is_valid(dsi->te_gpio)) {
>> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
>> +		gpio_free(dsi->te_gpio);
>> +		dsi->te_gpio = -ENOENT;
>> +	}
>> +}
>> +
>>  static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>  				  struct mipi_dsi_device *device)
>>  {
>> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>  	if (dsi->connector.dev)
>>  		drm_helper_hpd_irq_event(dsi->connector.dev);
>>  
>> +	/*
>> +	 * If attached panel device is for command mode one, dsi should
>> +	 * register TE interrupt handler.
>> +	 */
>> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>> +		int ret = exynos_dsi_register_te_irq(dsi);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>  {
>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>  
>> +	exynos_dsi_unregister_te_irq(dsi);
>> +
>>  	dsi->panel_node = NULL;
>>  
>>  	if (dsi->connector.dev)
>> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>>  
>>  		exynos_dsi_disable_clock(dsi);
>>  
>> -		disable_irq(dsi->irq);
>> +		exynos_dsi_disable_irq(dsi);
>>  	}
>>  
>>  	dsi->state &= ~DSIM_STATE_CMD_LPM;
>> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>  		goto err_del_component;
>>  	}
>>  
>> +	/* To be checked as invalid one */
>> +	dsi->te_gpio = -ENOENT;
>> +
>>  	init_completion(&dsi->completed);
>>  	spin_lock_init(&dsi->transfer_lock);
>>  	INIT_LIST_HEAD(&dsi->transfer_list);
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
YoungJun Cho July 22, 2014, 2:15 a.m. UTC | #3
Hi,

On 07/22/2014 10:23 AM, Inki Dae wrote:
> On 2014? 07? 21? 23:01, Andrzej Hajda wrote:
>> On 07/17/2014 11:01 AM, YoungJun Cho wrote:
>>> To support LCD I80 interface, the DSI host should register
>>> TE interrupt handler from the TE GPIO of attached panel.
>>> So the panel generates a tearing effect synchronization signal
>>> then the DSI host calls the CRTC device manager to trigger
>>> to transfer video image.
>>>
>>
>> This whole patch seems to be a hack.
>>
>> I think it is not a good idea to parse property of one device by another
>> device's driver.
>>
>> Especially here TE GPIO belongs to the panel. The panel driver should
>> know how to configure it and how to use it or not. The panel driver will
>> generate TE signal based on this GPIO, DSI/BTA mechanism or any other
>> mechanism provided by the panel.
>> TE signal should be delivered to the display controller. The only role
>> of DSIM here is that it is between the panel and the display controller
>> so it should be used to route this signal to DC.

It looks like DSIM transfers only TE signal to FIMD, but there is one 
thing important role, DSIM transfers TE signal only it is activated.
Without this thing, a broken screen would be showed at booting time.

>>
>> According to specs TE signal should/can be generated by DBI and DSI
>> command mode panels, so its handling should be more generic, not Exynos
>> specific.
>>
>
> Right. However, it seems that there are no much users using command mode
> panel so we would need more times to discuss the generic way. I think we
> can have this feature in specific to Exynos drm - it doesn't affect
> other SoC -.  Actually, I know OMAP people handle this feature in a way

For your information, there is a dsicm_te_isr() in 
drivers/video/fbdev/omap2/displays-new.
It seems like that panel -> dsi -> display controller.

Thank you.
Best regards YJ

> specific to OMAP SoC. If other users need more generic way to this
> feature then we could have a discussion about the generic way at that time.
>
> That is why Mr. Cho implemented TE feature like this. Do you have more
> good idea? I wish the idea would be specific so that Mr. Cho can
> implement it.
>
> P.s. Thierry already opposed the use of mipi_dsi_host_ops, I agree with
> him. And also it seems not good to use crtc and encoder/connector
> because these frameworks are common to all architecture including x86 so
> other architectures wouldn't need TE feature.
>
> Thanks,
> Inki Dae
>
>> Regards
>> Andrzej
>>
>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>>>   1 file changed, 93 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index 58bfb2a..4997bfe 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -16,7 +16,9 @@
>>>   #include <drm/drm_panel.h>
>>>
>>>   #include <linux/clk.h>
>>> +#include <linux/gpio/consumer.h>
>>>   #include <linux/irq.h>
>>> +#include <linux/of_gpio.h>
>>>   #include <linux/phy/phy.h>
>>>   #include <linux/regulator/consumer.h>
>>>   #include <linux/component.h>
>>> @@ -24,6 +26,7 @@
>>>   #include <video/mipi_display.h>
>>>   #include <video/videomode.h>
>>>
>>> +#include "exynos_drm_crtc.h"
>>>   #include "exynos_drm_drv.h"
>>>
>>>   /* returns true iff both arguments logically differs */
>>> @@ -247,6 +250,7 @@ struct exynos_dsi {
>>>   	struct clk *bus_clk;
>>>   	struct regulator_bulk_data supplies[2];
>>>   	int irq;
>>> +	int te_gpio;
>>>
>>>   	u32 pll_clk_rate;
>>>   	u32 burst_clk_rate;
>>> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>>>   	return IRQ_HANDLED;
>>>   }
>>>
>>> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>>> +{
>>> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
>>> +	struct drm_encoder *encoder = dsi->encoder;
>>> +
>>> +	if (dsi->state & DSIM_STATE_ENABLED)
>>> +		exynos_drm_crtc_te_handler(encoder->crtc);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	enable_irq(dsi->irq);
>>> +
>>> +	if (gpio_is_valid(dsi->te_gpio))
>>> +		enable_irq(gpio_to_irq(dsi->te_gpio));
>>> +}
>>> +
>>> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	if (gpio_is_valid(dsi->te_gpio))
>>> +		disable_irq(gpio_to_irq(dsi->te_gpio));
>>> +
>>> +	disable_irq(dsi->irq);
>>> +}
>>> +
>>>   static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>   {
>>>   	exynos_dsi_enable_clock(dsi);
>>>   	exynos_dsi_reset(dsi);
>>> -	enable_irq(dsi->irq);
>>> +	exynos_dsi_enable_irq(dsi);
>>>   	exynos_dsi_wait_for_reset(dsi);
>>>   	exynos_dsi_init_link(dsi);
>>>
>>>   	return 0;
>>>   }
>>>
>>> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	int ret;
>>> +
>>> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
>>> +	if (!gpio_is_valid(dsi->te_gpio)) {
>>> +		dev_err(dsi->dev, "no te-gpios specified\n");
>>> +		ret = dsi->te_gpio;
>>> +		goto out;
>>> +	}
>>> +
>>> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/*
>>> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>>> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
>>> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>>> +	 * called by drm_panel_init() before panel is attached.
>>> +	 */
>>> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>>> +					exynos_dsi_te_irq_handler, NULL,
>>> +					IRQF_TRIGGER_RISING, "TE", dsi);
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
>>> +		gpio_free(dsi->te_gpio);
>>> +		goto out;
>>> +	}
>>> +
>>> +out:
>>> +	return ret;
>>> +}
>>> +
>>> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	if (gpio_is_valid(dsi->te_gpio)) {
>>> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
>>> +		gpio_free(dsi->te_gpio);
>>> +		dsi->te_gpio = -ENOENT;
>>> +	}
>>> +}
>>> +
>>>   static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>   				  struct mipi_dsi_device *device)
>>>   {
>>> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>   	if (dsi->connector.dev)
>>>   		drm_helper_hpd_irq_event(dsi->connector.dev);
>>>
>>> +	/*
>>> +	 * If attached panel device is for command mode one, dsi should
>>> +	 * register TE interrupt handler.
>>> +	 */
>>> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>>> +		int ret = exynos_dsi_register_te_irq(dsi);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>>   	return 0;
>>>   }
>>>
>>> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>   {
>>>   	struct exynos_dsi *dsi = host_to_dsi(host);
>>>
>>> +	exynos_dsi_unregister_te_irq(dsi);
>>> +
>>>   	dsi->panel_node = NULL;
>>>
>>>   	if (dsi->connector.dev)
>>> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>>>
>>>   		exynos_dsi_disable_clock(dsi);
>>>
>>> -		disable_irq(dsi->irq);
>>> +		exynos_dsi_disable_irq(dsi);
>>>   	}
>>>
>>>   	dsi->state &= ~DSIM_STATE_CMD_LPM;
>>> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>>   		goto err_del_component;
>>>   	}
>>>
>>> +	/* To be checked as invalid one */
>>> +	dsi->te_gpio = -ENOENT;
>>> +
>>>   	init_completion(&dsi->completed);
>>>   	spin_lock_init(&dsi->transfer_lock);
>>>   	INIT_LIST_HEAD(&dsi->transfer_list);
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
Andrzej Hajda July 22, 2014, 10:12 a.m. UTC | #4
On 07/22/2014 03:23 AM, Inki Dae wrote:
> On 2014? 07? 21? 23:01, Andrzej Hajda wrote:
>> On 07/17/2014 11:01 AM, YoungJun Cho wrote:
>>> To support LCD I80 interface, the DSI host should register
>>> TE interrupt handler from the TE GPIO of attached panel.
>>> So the panel generates a tearing effect synchronization signal
>>> then the DSI host calls the CRTC device manager to trigger
>>> to transfer video image.
>>>
>> This whole patch seems to be a hack.
>>
>> I think it is not a good idea to parse property of one device by another
>> device's driver.
>>
>> Especially here TE GPIO belongs to the panel. The panel driver should
>> know how to configure it and how to use it or not. The panel driver will
>> generate TE signal based on this GPIO, DSI/BTA mechanism or any other
>> mechanism provided by the panel.
>> TE signal should be delivered to the display controller. The only role
>> of DSIM here is that it is between the panel and the display controller
>> so it should be used to route this signal to DC.
>>
>> According to specs TE signal should/can be generated by DBI and DSI
>> command mode panels, so its handling should be more generic, not Exynos
>> specific.
>>
> Right. However, it seems that there are no much users using command mode
> panel so we would need more times to discuss the generic way. I think we
> can have this feature in specific to Exynos drm - it doesn't affect
> other SoC -.  Actually, I know OMAP people handle this feature in a way
> specific to OMAP SoC. If other users need more generic way to this
> feature then we could have a discussion about the generic way at that time.
>
> That is why Mr. Cho implemented TE feature like this. Do you have more
> good idea? I wish the idea would be specific so that Mr. Cho can
> implement it.
>
> P.s. Thierry already opposed the use of mipi_dsi_host_ops, I agree with
> him. And also it seems not good to use crtc and encoder/connector
> because these frameworks are common to all architecture including x86 so
> other architectures wouldn't need TE feature.

The good thing is that DT bindings in this case are OK, TE GPIO is in
panel node.
Maybe we can leave it this way for now, but at least lets add a comment to
the code describing that it is temporary solution and should be make
more generic in the future.

Regards
Andrzej
>
> Thanks,
> Inki Dae
>
>> Regards
>> Andrzej
>>
>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>>>  1 file changed, 93 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index 58bfb2a..4997bfe 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -16,7 +16,9 @@
>>>  #include <drm/drm_panel.h>
>>>  
>>>  #include <linux/clk.h>
>>> +#include <linux/gpio/consumer.h>
>>>  #include <linux/irq.h>
>>> +#include <linux/of_gpio.h>
>>>  #include <linux/phy/phy.h>
>>>  #include <linux/regulator/consumer.h>
>>>  #include <linux/component.h>
>>> @@ -24,6 +26,7 @@
>>>  #include <video/mipi_display.h>
>>>  #include <video/videomode.h>
>>>  
>>> +#include "exynos_drm_crtc.h"
>>>  #include "exynos_drm_drv.h"
>>>  
>>>  /* returns true iff both arguments logically differs */
>>> @@ -247,6 +250,7 @@ struct exynos_dsi {
>>>  	struct clk *bus_clk;
>>>  	struct regulator_bulk_data supplies[2];
>>>  	int irq;
>>> +	int te_gpio;
>>>  
>>>  	u32 pll_clk_rate;
>>>  	u32 burst_clk_rate;
>>> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>>>  	return IRQ_HANDLED;
>>>  }
>>>  
>>> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>>> +{
>>> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
>>> +	struct drm_encoder *encoder = dsi->encoder;
>>> +
>>> +	if (dsi->state & DSIM_STATE_ENABLED)
>>> +		exynos_drm_crtc_te_handler(encoder->crtc);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	enable_irq(dsi->irq);
>>> +
>>> +	if (gpio_is_valid(dsi->te_gpio))
>>> +		enable_irq(gpio_to_irq(dsi->te_gpio));
>>> +}
>>> +
>>> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	if (gpio_is_valid(dsi->te_gpio))
>>> +		disable_irq(gpio_to_irq(dsi->te_gpio));
>>> +
>>> +	disable_irq(dsi->irq);
>>> +}
>>> +
>>>  static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>  {
>>>  	exynos_dsi_enable_clock(dsi);
>>>  	exynos_dsi_reset(dsi);
>>> -	enable_irq(dsi->irq);
>>> +	exynos_dsi_enable_irq(dsi);
>>>  	exynos_dsi_wait_for_reset(dsi);
>>>  	exynos_dsi_init_link(dsi);
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	int ret;
>>> +
>>> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
>>> +	if (!gpio_is_valid(dsi->te_gpio)) {
>>> +		dev_err(dsi->dev, "no te-gpios specified\n");
>>> +		ret = dsi->te_gpio;
>>> +		goto out;
>>> +	}
>>> +
>>> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/*
>>> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>>> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
>>> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>>> +	 * called by drm_panel_init() before panel is attached.
>>> +	 */
>>> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>>> +					exynos_dsi_te_irq_handler, NULL,
>>> +					IRQF_TRIGGER_RISING, "TE", dsi);
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
>>> +		gpio_free(dsi->te_gpio);
>>> +		goto out;
>>> +	}
>>> +
>>> +out:
>>> +	return ret;
>>> +}
>>> +
>>> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	if (gpio_is_valid(dsi->te_gpio)) {
>>> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
>>> +		gpio_free(dsi->te_gpio);
>>> +		dsi->te_gpio = -ENOENT;
>>> +	}
>>> +}
>>> +
>>>  static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>  				  struct mipi_dsi_device *device)
>>>  {
>>> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>  	if (dsi->connector.dev)
>>>  		drm_helper_hpd_irq_event(dsi->connector.dev);
>>>  
>>> +	/*
>>> +	 * If attached panel device is for command mode one, dsi should
>>> +	 * register TE interrupt handler.
>>> +	 */
>>> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>>> +		int ret = exynos_dsi_register_te_irq(dsi);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>  {
>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>  
>>> +	exynos_dsi_unregister_te_irq(dsi);
>>> +
>>>  	dsi->panel_node = NULL;
>>>  
>>>  	if (dsi->connector.dev)
>>> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>>>  
>>>  		exynos_dsi_disable_clock(dsi);
>>>  
>>> -		disable_irq(dsi->irq);
>>> +		exynos_dsi_disable_irq(dsi);
>>>  	}
>>>  
>>>  	dsi->state &= ~DSIM_STATE_CMD_LPM;
>>> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>>  		goto err_del_component;
>>>  	}
>>>  
>>> +	/* To be checked as invalid one */
>>> +	dsi->te_gpio = -ENOENT;
>>> +
>>>  	init_completion(&dsi->completed);
>>>  	spin_lock_init(&dsi->transfer_lock);
>>>  	INIT_LIST_HEAD(&dsi->transfer_list);
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
YoungJun Cho July 22, 2014, 10:26 a.m. UTC | #5
Hi Andrzej,

On 07/22/2014 07:12 PM, Andrzej Hajda wrote:
> On 07/22/2014 03:23 AM, Inki Dae wrote:
>> On 2014? 07? 21? 23:01, Andrzej Hajda wrote:
>>> On 07/17/2014 11:01 AM, YoungJun Cho wrote:
>>>> To support LCD I80 interface, the DSI host should register
>>>> TE interrupt handler from the TE GPIO of attached panel.
>>>> So the panel generates a tearing effect synchronization signal
>>>> then the DSI host calls the CRTC device manager to trigger
>>>> to transfer video image.
>>>>
>>> This whole patch seems to be a hack.
>>>
>>> I think it is not a good idea to parse property of one device by another
>>> device's driver.
>>>
>>> Especially here TE GPIO belongs to the panel. The panel driver should
>>> know how to configure it and how to use it or not. The panel driver will
>>> generate TE signal based on this GPIO, DSI/BTA mechanism or any other
>>> mechanism provided by the panel.
>>> TE signal should be delivered to the display controller. The only role
>>> of DSIM here is that it is between the panel and the display controller
>>> so it should be used to route this signal to DC.
>>>
>>> According to specs TE signal should/can be generated by DBI and DSI
>>> command mode panels, so its handling should be more generic, not Exynos
>>> specific.
>>>
>> Right. However, it seems that there are no much users using command mode
>> panel so we would need more times to discuss the generic way. I think we
>> can have this feature in specific to Exynos drm - it doesn't affect
>> other SoC -.  Actually, I know OMAP people handle this feature in a way
>> specific to OMAP SoC. If other users need more generic way to this
>> feature then we could have a discussion about the generic way at that time.
>>
>> That is why Mr. Cho implemented TE feature like this. Do you have more
>> good idea? I wish the idea would be specific so that Mr. Cho can
>> implement it.
>>
>> P.s. Thierry already opposed the use of mipi_dsi_host_ops, I agree with
>> him. And also it seems not good to use crtc and encoder/connector
>> because these frameworks are common to all architecture including x86 so
>> other architectures wouldn't need TE feature.
>
> The good thing is that DT bindings in this case are OK, TE GPIO is in
> panel node.
> Maybe we can leave it this way for now, but at least lets add a comment to
> the code describing that it is temporary solution and should be make
> more generic in the future.
>

Okay, I'll leave this comment at exynos_dsi_host_attach() before current 
exynos_dsi_register_te_irq() relevant comment.

Thank you.
Best regards YJ

> Regards
> Andrzej
>>
>> Thanks,
>> Inki Dae
>>
>>> Regards
>>> Andrzej
>>>
>>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 93 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> index 58bfb2a..4997bfe 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> @@ -16,7 +16,9 @@
>>>>   #include <drm/drm_panel.h>
>>>>
>>>>   #include <linux/clk.h>
>>>> +#include <linux/gpio/consumer.h>
>>>>   #include <linux/irq.h>
>>>> +#include <linux/of_gpio.h>
>>>>   #include <linux/phy/phy.h>
>>>>   #include <linux/regulator/consumer.h>
>>>>   #include <linux/component.h>
>>>> @@ -24,6 +26,7 @@
>>>>   #include <video/mipi_display.h>
>>>>   #include <video/videomode.h>
>>>>
>>>> +#include "exynos_drm_crtc.h"
>>>>   #include "exynos_drm_drv.h"
>>>>
>>>>   /* returns true iff both arguments logically differs */
>>>> @@ -247,6 +250,7 @@ struct exynos_dsi {
>>>>   	struct clk *bus_clk;
>>>>   	struct regulator_bulk_data supplies[2];
>>>>   	int irq;
>>>> +	int te_gpio;
>>>>
>>>>   	u32 pll_clk_rate;
>>>>   	u32 burst_clk_rate;
>>>> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>>>>   	return IRQ_HANDLED;
>>>>   }
>>>>
>>>> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>>>> +{
>>>> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
>>>> +	struct drm_encoder *encoder = dsi->encoder;
>>>> +
>>>> +	if (dsi->state & DSIM_STATE_ENABLED)
>>>> +		exynos_drm_crtc_te_handler(encoder->crtc);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
>>>> +{
>>>> +	enable_irq(dsi->irq);
>>>> +
>>>> +	if (gpio_is_valid(dsi->te_gpio))
>>>> +		enable_irq(gpio_to_irq(dsi->te_gpio));
>>>> +}
>>>> +
>>>> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
>>>> +{
>>>> +	if (gpio_is_valid(dsi->te_gpio))
>>>> +		disable_irq(gpio_to_irq(dsi->te_gpio));
>>>> +
>>>> +	disable_irq(dsi->irq);
>>>> +}
>>>> +
>>>>   static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>>   {
>>>>   	exynos_dsi_enable_clock(dsi);
>>>>   	exynos_dsi_reset(dsi);
>>>> -	enable_irq(dsi->irq);
>>>> +	exynos_dsi_enable_irq(dsi);
>>>>   	exynos_dsi_wait_for_reset(dsi);
>>>>   	exynos_dsi_init_link(dsi);
>>>>
>>>>   	return 0;
>>>>   }
>>>>
>>>> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
>>>> +	if (!gpio_is_valid(dsi->te_gpio)) {
>>>> +		dev_err(dsi->dev, "no te-gpios specified\n");
>>>> +		ret = dsi->te_gpio;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>>>> +	if (ret) {
>>>> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>>>> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
>>>> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>>>> +	 * called by drm_panel_init() before panel is attached.
>>>> +	 */
>>>> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>>>> +					exynos_dsi_te_irq_handler, NULL,
>>>> +					IRQF_TRIGGER_RISING, "TE", dsi);
>>>> +	if (ret) {
>>>> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
>>>> +		gpio_free(dsi->te_gpio);
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +out:
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>>>> +{
>>>> +	if (gpio_is_valid(dsi->te_gpio)) {
>>>> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
>>>> +		gpio_free(dsi->te_gpio);
>>>> +		dsi->te_gpio = -ENOENT;
>>>> +	}
>>>> +}
>>>> +
>>>>   static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>   				  struct mipi_dsi_device *device)
>>>>   {
>>>> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>   	if (dsi->connector.dev)
>>>>   		drm_helper_hpd_irq_event(dsi->connector.dev);
>>>>
>>>> +	/*
>>>> +	 * If attached panel device is for command mode one, dsi should
>>>> +	 * register TE interrupt handler.
>>>> +	 */
>>>> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>>>> +		int ret = exynos_dsi_register_te_irq(dsi);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>   	return 0;
>>>>   }
>>>>
>>>> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>>   {
>>>>   	struct exynos_dsi *dsi = host_to_dsi(host);
>>>>
>>>> +	exynos_dsi_unregister_te_irq(dsi);
>>>> +
>>>>   	dsi->panel_node = NULL;
>>>>
>>>>   	if (dsi->connector.dev)
>>>> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>>>>
>>>>   		exynos_dsi_disable_clock(dsi);
>>>>
>>>> -		disable_irq(dsi->irq);
>>>> +		exynos_dsi_disable_irq(dsi);
>>>>   	}
>>>>
>>>>   	dsi->state &= ~DSIM_STATE_CMD_LPM;
>>>> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>>>   		goto err_del_component;
>>>>   	}
>>>>
>>>> +	/* To be checked as invalid one */
>>>> +	dsi->te_gpio = -ENOENT;
>>>> +
>>>>   	init_completion(&dsi->completed);
>>>>   	spin_lock_init(&dsi->transfer_lock);
>>>>   	INIT_LIST_HEAD(&dsi->transfer_list);
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 58bfb2a..4997bfe 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -16,7 +16,9 @@ 
 #include <drm/drm_panel.h>
 
 #include <linux/clk.h>
+#include <linux/gpio/consumer.h>
 #include <linux/irq.h>
+#include <linux/of_gpio.h>
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
@@ -24,6 +26,7 @@ 
 #include <video/mipi_display.h>
 #include <video/videomode.h>
 
+#include "exynos_drm_crtc.h"
 #include "exynos_drm_drv.h"
 
 /* returns true iff both arguments logically differs */
@@ -247,6 +250,7 @@  struct exynos_dsi {
 	struct clk *bus_clk;
 	struct regulator_bulk_data supplies[2];
 	int irq;
+	int te_gpio;
 
 	u32 pll_clk_rate;
 	u32 burst_clk_rate;
@@ -954,17 +958,89 @@  static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
+{
+	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
+	struct drm_encoder *encoder = dsi->encoder;
+
+	if (dsi->state & DSIM_STATE_ENABLED)
+		exynos_drm_crtc_te_handler(encoder->crtc);
+
+	return IRQ_HANDLED;
+}
+
+static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
+{
+	enable_irq(dsi->irq);
+
+	if (gpio_is_valid(dsi->te_gpio))
+		enable_irq(gpio_to_irq(dsi->te_gpio));
+}
+
+static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
+{
+	if (gpio_is_valid(dsi->te_gpio))
+		disable_irq(gpio_to_irq(dsi->te_gpio));
+
+	disable_irq(dsi->irq);
+}
+
 static int exynos_dsi_init(struct exynos_dsi *dsi)
 {
 	exynos_dsi_enable_clock(dsi);
 	exynos_dsi_reset(dsi);
-	enable_irq(dsi->irq);
+	exynos_dsi_enable_irq(dsi);
 	exynos_dsi_wait_for_reset(dsi);
 	exynos_dsi_init_link(dsi);
 
 	return 0;
 }
 
+static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
+{
+	int ret;
+
+	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
+	if (!gpio_is_valid(dsi->te_gpio)) {
+		dev_err(dsi->dev, "no te-gpios specified\n");
+		ret = dsi->te_gpio;
+		goto out;
+	}
+
+	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
+	if (ret) {
+		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
+		goto out;
+	}
+
+	/*
+	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
+	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
+	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
+	 * called by drm_panel_init() before panel is attached.
+	 */
+	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
+					exynos_dsi_te_irq_handler, NULL,
+					IRQF_TRIGGER_RISING, "TE", dsi);
+	if (ret) {
+		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
+		gpio_free(dsi->te_gpio);
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
+{
+	if (gpio_is_valid(dsi->te_gpio)) {
+		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
+		gpio_free(dsi->te_gpio);
+		dsi->te_gpio = -ENOENT;
+	}
+}
+
 static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 				  struct mipi_dsi_device *device)
 {
@@ -978,6 +1054,16 @@  static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	if (dsi->connector.dev)
 		drm_helper_hpd_irq_event(dsi->connector.dev);
 
+	/*
+	 * If attached panel device is for command mode one, dsi should
+	 * register TE interrupt handler.
+	 */
+	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+		int ret = exynos_dsi_register_te_irq(dsi);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -986,6 +1072,8 @@  static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
 
+	exynos_dsi_unregister_te_irq(dsi);
+
 	dsi->panel_node = NULL;
 
 	if (dsi->connector.dev)
@@ -1099,7 +1187,7 @@  static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
 
 		exynos_dsi_disable_clock(dsi);
 
-		disable_irq(dsi->irq);
+		exynos_dsi_disable_irq(dsi);
 	}
 
 	dsi->state &= ~DSIM_STATE_CMD_LPM;
@@ -1445,6 +1533,9 @@  static int exynos_dsi_probe(struct platform_device *pdev)
 		goto err_del_component;
 	}
 
+	/* To be checked as invalid one */
+	dsi->te_gpio = -ENOENT;
+
 	init_completion(&dsi->completed);
 	spin_lock_init(&dsi->transfer_lock);
 	INIT_LIST_HEAD(&dsi->transfer_list);