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

[YoungJun Cho <yj44.cho@samsung.com>]

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
>>>
>>
>
>

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