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

[Inki Dae <inki.dae@samsung.com>]

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
> 

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