From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753267AbbIGAXH (ORCPT ); Sun, 6 Sep 2015 20:23:07 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:49742 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752922AbbIGAXA (ORCPT ); Sun, 6 Sep 2015 20:23:00 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: cbfec7f4-f79c56d0000012ee-88-55ecd8e016a7 Content-transfer-encoding: 8BIT Subject: Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir To: Yakir Yang , Heiko Stuebner , Thierry Reding , Jingoo Han , Inki Dae , joe@perches.com, Kukjin Kim , Mark Yao References: <1441086371-24838-1-git-send-email-ykk@rock-chips.com> <1441086598-24995-1-git-send-email-ykk@rock-chips.com> <55E79B41.6010301@samsung.com> <55E7DAE1.6070308@rock-chips.com> <55E8E8BE.8010300@samsung.com> <55EBF024.3070304@rock-chips.com> Cc: Russell King , djkurtz@chromium.com, dianders@chromium.com, seanpaul@chromium.com, ajaynumb@gmail.com, Andrzej Hajda , Kyungmin Park , David Airlie , Gustavo Padovan , Andy Yan , Kumar Gala , Ian Campbell , Rob Herring , Pawel Moll , Kishon Vijay Abraham I , architt@codeaurora.org, robherring2@gmail.com, dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org From: Krzysztof Kozlowski Message-id: <55ECD8CB.7060408@samsung.com> Date: Mon, 07 Sep 2015 09:22:35 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 In-reply-to: <55EBF024.3070304@rock-chips.com> X-Brightmail-Tracker: H4sIAAAAAAAAA02SbUhTURjHOfd9w9VtOTtUUKwsMjIlg0OERUTe+lARChVR3fQypU1tc7Ik yEzBZmtSRm0607S0uZpNo7LSHKK9sGWttBKdaC+aM6XWNMRe5or69nvO/8dz/h8eBpcWkfOZ tPQsQZ3OK+WUmHj6o8Ozqv+1LylmrHQ5envDTSKD+zGG6n2VODI+ewNQXuEoiS61/Q5ah/Ip VDhhxNDLb2MUMvoqSfSlroBGPwdGSOT+dA2gs95iAtX6TTQq9Q4SaGQoFhkHR3DU+f40hVx5 Pho5BrtI5Gkqo9CX/p84uvisGUN3Sh5iqPjCdQI1jA9TqOBBG40mAgEKPa/y0qhnahZy556j Ny7mbOU2wOWfPE1xH5tGMM5zxoBx0x+7Ce6uuZfmrtX4Kc5hPUVxtwP9JOct6sC4hurjnOHk Z4rzW7twbtrcQnBnGq2Au91Vju+U7hWvTxGUadmCenX8QXHqu1ZdZlmMzn/TQ+eCy0v1QMRA Ng7mOgvoEEfAzj47pQdiRspeAdDkGKaCgYSdAyfP9RF6wDA4uwi2vTgcwuWwpEQV0v0AFtmL Z/bMZfdDr70ZBDmcLcTgve6EkHQCg3X6chAccNZEwe913TMWxa6BDTXVfz6LguP3B/AgE2wk nKqvJIIsY3fDJ67RGUfERkOn9yZZDFjzf/3M//qZ//WrALgVyARtcqbmkEIVG63hVRptuiI6 OUPlAKET8d8BVe3rnIBlgDxMEmPwJUlJPltzVOUEkMHl4ZK4xt9PkhT+aI6gzjig1ioFjRMs YAj5PImlaSxRyir4LOGwIGQK6r8pxojm5wIq+0DlslU9yuTpRIPv65L6U1+nZx970J+QZrNd 3TKlNfMWmb15KDLOoHDte5SwfV/ZESyrXfkqpZf9PLqr1vKqVdRZcWtZ/IoUf/dbt6d6rWUR vyfD1rl54EOgQpedNLghesdC7daWwjU5RxQR44QpLGKTS7Z+m2myffi8zr7SICc0qXxsFK7W 8L8AP7OChx4DAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06.09.2015 16:49, Yakir Yang wrote: > Hi Krzysztof, > > 在 09/04/2015 08:41 AM, Krzysztof Kozlowski 写道: >> On 03.09.2015 14:30, Yakir Yang wrote: >>> Hi Krzysztof, >>> >>> 在 09/03/2015 08:58 AM, Krzysztof Kozlowski 写道: >>>> On 01.09.2015 14:49, Yakir Yang wrote: >>>>> Split the dp core driver from exynos directory to bridge >>>>> directory, and rename the core driver to analogix_dp_*, >>>>> leave the platform code to analogix_dp-exynos. >>>>> >>>>> Signed-off-by: Yakir Yang >>>>> --- >>>>> Changes in v4: >>>>> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT >>>>> propery. >>>>> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to >>>>> "exynos_dp.c" >>>>> - Take Archit suggest, create a separate folder for analogix code in >>>>> bridge/ >>>>> >>>>> Changes in v3: >>>>> - Take Thierry Reding suggest, move exynos's video_timing code >>>>> to analogix_dp-exynos platform driver, add get_modes method >>>>> to struct analogix_dp_plat_data. >>>>> - Take Heiko suggest, rename some "samsung*" dts propery to >>>>> "analogix*". >>>>> >>>>> Changes in v2: >>>>> - Take Jingoo Han suggest, remove new copyright >>>>> - Fix compiled failed dut to analogix_dp_device misspell > > [.....] > >>>>> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge) >>>>> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge) >>>>> { >>>>> - struct exynos_dp_device *dp = bridge->driver_private; >>>>> - struct drm_encoder *encoder = &dp->encoder; >>>>> + struct analogix_dp_device *dp = bridge->driver_private; >>>>> + struct drm_encoder *encoder = dp->encoder; >>>>> struct drm_connector *connector = &dp->connector; >>>>> int ret; >>>>> - /* Pre-empt DP connector creation if there's a bridge */ >>>>> - if (dp->ptn_bridge) { >>>>> - ret = exynos_drm_attach_lcd_bridge(dp, encoder); >>>>> - if (!ret) >>>>> - return 0; >>>>> + if (!bridge->encoder) { >>>>> + DRM_ERROR("Parent encoder object not found"); >>>>> + return -ENODEV; >>>>> } >>>>> + encoder->bridge = bridge; >>>>> + >>>>> connector->polled = DRM_CONNECTOR_POLL_HPD; >>>>> ret = drm_connector_init(dp->drm_dev, connector, >>>>> - &exynos_dp_connector_funcs, >>>>> + &analogix_dp_connector_funcs, >>>>> DRM_MODE_CONNECTOR_eDP); >>>>> if (ret) { >>>>> DRM_ERROR("Failed to initialize connector with drm\n"); >>>>> return ret; >>>>> } >>>>> - drm_connector_helper_add(connector, >>>>> &exynos_dp_connector_helper_funcs); >>>>> + drm_connector_helper_add(connector, >>>>> + &analogix_dp_connector_helper_funcs); >>>>> drm_connector_register(connector); >>>>> drm_mode_connector_attach_encoder(connector, encoder); >>>>> - if (dp->panel) >>>>> - ret = drm_panel_attach(dp->panel, &dp->connector); >>>>> + if (dp->plat_data && dp->plat_data->panel) { >>>>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector); >>>>> + if (ret) { >>>>> + DRM_ERROR("Failed to attach panel\n"); >>>>> + return ret; >>>>> + } >>>>> + } >>>>> + >>>>> + /* >>>>> + * This should be the end of attach function, caused >>>>> + * we should ensure dp bridge could attach first. >>>>> + */ >>>>> + if (dp->plat_data && dp->plat_data->attach) { >>>>> + ret = dp->plat_data->attach(dp->plat_data, bridge); >>>>> + if (ret) { >>>>> + DRM_ERROR("Failed at platform attch func\n"); >>>> Two new error paths appeared here and above. Don't you have to >>>> cleanup something? I don't know, just wondering... >>> Hmm... I think both panel & platform_attch need ERROR remind when >>> it failed. But if it still need clean, I though it should clean the >>> platform attch >>> error, >>> this is not relate to DRM directly, just analogix driver logic, so >>> code would like, >>> >>> - if (dp->panel) >>> - ret = drm_panel_attach(dp->panel, &dp->connector); >>> + if (dp->plat_data && dp->plat_data->panel) { >>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector); >>> + if (ret) { >>> + DRM_ERROR("Failed to attach panel\n"); >>> + return ret; >>> + } >>> + } >>> >>> + /* >>> + * This should be the end of attach function, caused >>> + * we should ensure dp bridge could attach first. >>> + */ >>> + if (dp->plat_data && dp->plat_data->attach) { >>> + ret = dp->plat_data->attach(dp->plat_data, bridge); >>> >>> return ret; >> I am lost... the code looks the same. What did you change? > > I just remove the DRM_ERROR after dp->plat_data->attach(), > maybe I should paste the change that rebase on this patch, > here are they, > > /* > * This should be the end of attach function, caused > * we should ensure dp bridge could attach first. > */ > - if (dp->plat_data && dp->plat_data->attach) { > + if (dp->plat_data && dp->plat_data->attach) > ret = dp->plat_data->attach(dp->plat_data, bridge); > - if (ret) { > - DRM_ERROR("Failed at platform attch func\n"); > - return ret; > - } > - } > > - return 0; > + return ret; > > > If this haven't meet your comment, I maybe start to think that > your comment "Two new error paths appeared here and above" > indicated that those two function is the same. > "dp->plat_data->attach(dp->plat_data, bridge); " > "drm_panel_attach(dp->plat_data->panel, &dp->connector); " I wasn't talking about error message but rather about possible need of clean up in error path. Previously there was only drm_panel_attach(). Now you have two of them (drm_panel_attach() and dp->plat_data->attach()). If the second fails don't you have to clean up before exit? I don't know, just asking. Best regards, Krzysztof