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