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 at samsung.com>
>>>> Acked-by: Inki Dae <inki.dae at samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park at 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 at vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>
