Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754892Ab3JCSKf (ORCPT ); Thu, 3 Oct 2013 14:10:35 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:39374 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754720Ab3JCSKc (ORCPT ); Thu, 3 Oct 2013 14:10:32 -0400 MIME-Version: 1.0 In-Reply-To: References: <1380670860-17621-1-git-send-email-seanpaul@chromium.org> <1380670860-17621-5-git-send-email-seanpaul@chromium.org> Date: Fri, 4 Oct 2013 03:10:30 +0900 X-Google-Sender-Auth: R5XUVxlP0IESqsbdoq-eq2dLZec Message-ID: Subject: Re: [PATCH 4/5] drm/exynos: Initialize ptn3460 if present From: Inki Dae To: Sean Paul Cc: "devicetree@vger.kernel.org" , "linux-samsung-soc@vger.kernel.org" , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , DRI mailing list , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8843 Lines: 224 2013/10/4 Sean Paul : > On Thu, Oct 3, 2013 at 1:18 PM, Inki Dae wrote: >> 2013/10/4 Sean Paul : >>> On Thu, Oct 3, 2013 at 10:43 AM, Inki Dae wrote: >>>> 2013/10/2 Sean Paul : >>>>> This patch adds code to look for the ptn3460 in the device tree file on >>>>> exynos initialization. If ptn node is found, the driver will initialize >>>>> the ptn3460 driver and skip creating a DP connector (since the bridge >>>>> driver will register its own connector). >>>>> >>>>> Signed-off-by: Sean Paul >>>>> --- >>>>> drivers/gpu/drm/exynos/exynos_drm_core.c | 44 +++++++++++++++++++++++++++++++- >>>>> 1 file changed, 43 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>> index 1bef6dc..9cf4476 100644 >>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c >>>>> @@ -12,7 +12,9 @@ >>>>> * option) any later version. >>>>> */ >>>>> >>>>> +#include >>>>> #include >>>>> +#include >>>>> #include "exynos_drm_drv.h" >>>>> #include "exynos_drm_encoder.h" >>>>> #include "exynos_drm_connector.h" >>>>> @@ -20,6 +22,40 @@ >>>>> >>>>> static LIST_HEAD(exynos_drm_subdrv_list); >>>>> >>>>> +struct bridge_init { >>>>> + struct i2c_client *client; >>>>> + struct device_node *node; >>>>> +}; >>>>> + >>>>> +static bool find_bridge(const char *name, struct bridge_init *bridge) >>>>> +{ >>>>> + bridge->client = NULL; >>>>> + bridge->node = of_find_node_by_name(NULL, name); >>>> >>>> Not clear to me. Why do you try to handle device tree here, not real >>>> device driver?. How about adding a output property to board specific >>>> fimd dt node: i.e. output = <&ptn3460_bridge>? >>> >>> The problem doing something like this is that we won't have a handle >>> to drm_device if it's just a standalone driver, and so we won't be >>> able to register the bridge or connector. We need this init call one >>> way or another. >>> >> >> At least, dt binding shoul be done in real device driver so this way >> is not good. Let's find a better way. >> > > Right, so this is kind of tricky. If you do it in a "real" device > driver, you end up parsing the dt stuff in the probe, and then racing > the init callback. I figured it would be best just to do everything in > one place without races. > > Hopefully I'm just missing a good way to solve this problem, any concrete ideas? > >>> >>>> Actually, the output >>>> device of fimd hw could be one of MIPI-DSI, eDP, mDNIe, LVDS bridge, >>>> or LCD. And then, let's find ptn3460-bridge node in the fimd driver, >>>> and initialize the ptn3460 bridge driver, and get power on or off >>>> through exynos_drm_display_ops of the fimd driver. And all these >>>> codes could be hided from fimd driver by moving them into >>>> exynos_drm_display_ops. Of course, for this, you would need additional >>>> works. So let's do it if needed. >>>> >>>> The below is the outline of device tree I recommend, >>>> >>>> In board dts, >>>> i2c@I2CD000 { >>>> ptn3460_bridge: prn3460-bridge@20 { >>>> ... >>>> } >>>> } >>>> >>>> fimd@11c00000 { >>>> ... >>>> output_dev = <&ptn3460_bridge>; >>>> } >>>> >>>> With this, I believe that you can do all things you want for >>>> controlling the LVDS bridge in fimd driver. >>>> >>> >>> No, this isn't what I want to do. The bridge should not hang off fimd >>> since it's a crtc. The bridge should only be initialized by the DP >>> driver (it doesn't make sense to initialize ptn when you're using >>> MIPI/LVDS/whatever). >> >> I don't mean that the bridge device should be initialized by fimd >> directly but the fimd driver provides just interfaces abstracted to >> control the bridge device. And basically, the exynos_drm_display_ops >> shouldn't be in fimd driver but in real connector driver; i.e. lcd >> panel or LVDS driver. The reason I placed the exynos_drm_display_ops >> in fimd driver is that lcd panel driver is controlled by lcd class >> depended on Linux framebuffer, and I thought the panel driver should >> be shared with drm driver in case of ARM SoC. The >> exynos_drm_display_ops should be moved into right place if something >> better exists some time or other. >> >> So how can the DP driver control the bridge device as of now? the DP >> you mentioned would be eDP, and the eDP driver is placed in >> drivers/video/exynos/, and also MIPI-DSI driver. >> > > It can't. The DP driver just operates on its own and display either > comes up or it doesn't depending on whether fimd happens to be > initialized first. Ok, I don't know the DP hardware well. But, MIPI-DSI driver is depending on fimd on/off ordering. ie. to enable display hw pipe, the ordering should be FIMD----MIPI-DSI-----LCD because initial commands _cannot be set_ to lcd panel if fimd is off. And to disable that, the ordering should be LCD-------MIPI-DSI-------FIMD in same reason: to get lcd panel off, off commands should be set to lcd panel. In similar reason, I had posted FB_EARLY_EVENT_BLANK feature to mainline and that have been merged. > As I mentioned earlier, I have a patch set which > moves DP driver into drm/exynos and removes the display_ops from fimd. > That will fix this issue. Ah... as you may know I mentioned about this issue some time ago; moving eDP driver(maybe??) into drm/exynos. At that time, I had rejected it because I thought we should share these bus drivers with Linux framebuffer driver: actually, I planned to use CDF for this. But.... I'm not sure even CDF as of now. So let's have a discussion about this issue with other people: it doesn't mean I oppose your patch set. > > Sean > > >>> >>> Since the actual crtc/encoder drivers are abstracted through >>> exynos_drm_core/crtc/encoder, we need to initialize the ptn driver in >>> the abstraction layers in order to hook it directly into drm. >>> >>> Sean >>> >>> >>>> Thanks, >>>> Inki Dae >>>> >>>>> + if (!bridge->node) >>>>> + return false; >>>>> + >>>>> + bridge->client = of_find_i2c_device_by_node(bridge->node); >>>>> + if (!bridge->client) >>>>> + return false; >>>>> + >>>>> + return true; >>>>> +} >>>>> + >>>>> +/* returns the number of bridges attached */ >>>>> +static int exynos_drm_attach_lcd_bridge(struct drm_device *dev, >>>>> + struct drm_encoder *encoder) >>>>> +{ >>>>> + struct bridge_init bridge; >>>>> + int ret; >>>>> + >>>>> + if (find_bridge("ptn3460-bridge", &bridge)) { >>>>> + ret = ptn3460_init(dev, encoder, bridge.client, bridge.node); >>>>> + if (!ret) >>>>> + return 1; >>>>> + } >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int exynos_drm_create_enc_conn(struct drm_device *dev, >>>>> struct exynos_drm_subdrv *subdrv) >>>>> { >>>>> @@ -36,6 +72,13 @@ static int exynos_drm_create_enc_conn(struct drm_device *dev, >>>>> DRM_ERROR("failed to create encoder\n"); >>>>> return -EFAULT; >>>>> } >>>>> + subdrv->encoder = encoder; >>>>> + >>>>> + if (subdrv->manager->display_ops->type == EXYNOS_DISPLAY_TYPE_LCD) { >>>>> + ret = exynos_drm_attach_lcd_bridge(dev, encoder); >>>>> + if (ret) >>>>> + return 0; >>>>> + } >>>>> >>>>> /* >>>>> * create and initialize a connector for this sub driver and >>>>> @@ -48,7 +91,6 @@ static int exynos_drm_create_enc_conn(struct drm_device *dev, >>>>> goto err_destroy_encoder; >>>>> } >>>>> >>>>> - subdrv->encoder = encoder; >>>>> subdrv->connector = connector; >>>>> >>>>> return 0; >>>>> -- >>>>> 1.8.4 >>>>> >>>>> >>>>> _______________________________________________ >>>>> linux-arm-kernel mailing list >>>>> linux-arm-kernel@lists.infradead.org >>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/