Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752873AbdLEJOa (ORCPT ); Tue, 5 Dec 2017 04:14:30 -0500 Received: from regular1.263xmail.com ([211.150.99.132]:42078 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752783AbdLEJOX (ORCPT ); Tue, 5 Dec 2017 04:14:23 -0500 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-RL-SENDER: nickey.yang@rock-chips.com X-FST-TO: xbl@rock-chips.com X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: nickey.yang@rock-chips.com X-UNIQUE-TAG: <6f87eaedc28424b1a891b3c880e6a789> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v4 1/3] drm/bridge/synopsys: dsi: stop clobbering drvdata To: Philippe CORNU , "robh+dt@kernel.org" , "heiko@sntech.de" , "mark.rutland@arm.com" , "airlied@linux.ie" Cc: "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linux-rockchip@lists.infradead.org" , "laurent.pinchart@ideasonboard.com" , "seanpaul@chromium.org" , "briannorris@chromium.org" , "mka@chromium.org" , "hoegsberg@gmail.com" , "architt@codeaurora.org" , Yannick FERTRE , "hl@rock-chips.com" , "zyw@rock-chips.com" , "xbl@rock-chips.com" References: <1512100685-4015-1-git-send-email-nickey.yang@rock-chips.com> <1512100685-4015-2-git-send-email-nickey.yang@rock-chips.com> <5605c3c2-56ad-2289-c3c0-78cde1152123@st.com> <1700743e-04b0-90ea-d393-dbfebf93742f@st.com> From: Nickey Yang Message-ID: <206b071f-2b57-c4a2-31b3-2f469091e5e2@rock-chips.com> Date: Tue, 5 Dec 2017 17:14:11 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <1700743e-04b0-90ea-d393-dbfebf93742f@st.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8491 Lines: 224 Hi Philippe, On 2017年12月01日 18:07, Philippe CORNU wrote: > Hi Nickey, > > On 12/01/2017 10:11 AM, Nickey Yang wrote: >> Hi Philippe, >> >> >> On 2017年12月01日 16:32, Philippe CORNU wrote: >>> Dear Nickey, >>> >>> Many thanks for your patch. >>> >>> I am sorry to say that but you can not add my "Acked-by" to this patch >>> because this code is different from the "original" one from Brian (which >>> got my "Acked-by"). >> I'm sorry I didn't think much about it, Thank you for correcting me. >>> Sometimes it is not an issue because differences are not important but >>> in this particular case, the code is really different: you have remove >>> platform_set_drvdata() & platform_get_drvdata() in the stm part. >>> >>> Could you please go back to the original code or propose me an updated >>> version of this code. >> Could you help update new version of this code(stm part) and then test on >> stm platform? > I think you can simply goes back to the original version from Brian (see > the discussion thread in https://patchwork.kernel.org/patch/10078493/) > unless you have specific/good reasons for modifying the code as you did. mmm,I'm sorry, I feel a little puzzled. Do you means we should abandon Brian's patch (https://patchwork.kernel.org/patch/10078493/)? I think we need to adjust stm part because  dw_mipi_dsi_stm.c calls bridge's drivers if we want merge Brian's patch. Thanks > Many thanks, > Philippe :) > >> Thanks, >> Nickey. >>> Many thanks, >>> >>> Philippe :-) >>> >>> >>> On 12/01/2017 04:58 AM, Nickey Yang wrote: >>>> From: Brian Norris >>>> >>>> Bridge drivers/helpers shouldn't be clobbering the drvdata, since a >>>> parent driver might need to own this. Instead, let's return our >>>> 'dw_mipi_dsi' object and have callers pass that back to us for removal. >>>> >>>> Signed-off-by: Brian Norris >>>> Signed-off-by: Nickey Yang >>>> Reviewed-by: Matthias Kaehlcke >>>> Reviewed-by: Archit Taneja >>>> Acked-by: Philippe Cornu >>>> Link:https://patchwork.kernel.org/patch/10078493/ >>>> >>>> --- >>>> Changes in v4: >>>> - Add From tag,update subject line >>>> - keep patch "drm/stm: dsi: Adjust dw_mipi_dsi_probe and remove" >>>> in this piece together. >>>> >>>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 36 ++++++++++----------------- >>>> drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 8 +++--- >>>> include/drm/bridge/dw_mipi_dsi.h | 17 ++++++++----- >>>> 3 files changed, 29 insertions(+), 32 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>> index d9cca4f..c39c7dc 100644 >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c >>>> @@ -922,8 +922,6 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge) >>>> dsi->bridge.of_node = pdev->dev.of_node; >>>> #endif >>>> >>>> - dev_set_drvdata(dev, dsi); >>>> - >>>> return dsi; >>>> } >>>> >>>> @@ -935,23 +933,16 @@ static void __dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi) >>>> /* >>>> * Probe/remove API, used from platforms based on the DRM bridge API. >>>> */ >>>> -int dw_mipi_dsi_probe(struct platform_device *pdev, >>>> - const struct dw_mipi_dsi_plat_data *plat_data) >>>> +struct dw_mipi_dsi * >>>> +dw_mipi_dsi_probe(struct platform_device *pdev, >>>> + const struct dw_mipi_dsi_plat_data *plat_data) >>>> { >>>> - struct dw_mipi_dsi *dsi; >>>> - >>>> - dsi = __dw_mipi_dsi_probe(pdev, plat_data); >>>> - if (IS_ERR(dsi)) >>>> - return PTR_ERR(dsi); >>>> - >>>> - return 0; >>>> + return __dw_mipi_dsi_probe(pdev, plat_data); >>>> } >>>> EXPORT_SYMBOL_GPL(dw_mipi_dsi_probe); >>>> >>>> -void dw_mipi_dsi_remove(struct platform_device *pdev) >>>> +void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi) >>>> { >>>> - struct dw_mipi_dsi *dsi = platform_get_drvdata(pdev); >>>> - >>>> mipi_dsi_host_unregister(&dsi->dsi_host); >>>> >>>> __dw_mipi_dsi_remove(dsi); >>>> @@ -961,31 +952,30 @@ void dw_mipi_dsi_remove(struct platform_device *pdev) >>>> /* >>>> * Bind/unbind API, used from platforms based on the component framework. >>>> */ >>>> -int dw_mipi_dsi_bind(struct platform_device *pdev, struct drm_encoder *encoder, >>>> - const struct dw_mipi_dsi_plat_data *plat_data) >>>> +struct dw_mipi_dsi * >>>> +dw_mipi_dsi_bind(struct platform_device *pdev, struct drm_encoder *encoder, >>>> + const struct dw_mipi_dsi_plat_data *plat_data) >>>> { >>>> struct dw_mipi_dsi *dsi; >>>> int ret; >>>> >>>> dsi = __dw_mipi_dsi_probe(pdev, plat_data); >>>> if (IS_ERR(dsi)) >>>> - return PTR_ERR(dsi); >>>> + return dsi; >>>> >>>> ret = drm_bridge_attach(encoder, &dsi->bridge, NULL); >>>> if (ret) { >>>> - dw_mipi_dsi_remove(pdev); >>>> + dw_mipi_dsi_remove(dsi); >>>> DRM_ERROR("Failed to initialize bridge with drm\n"); >>>> - return ret; >>>> + return ERR_PTR(ret); >>>> } >>>> >>>> - return 0; >>>> + return dsi; >>>> } >>>> EXPORT_SYMBOL_GPL(dw_mipi_dsi_bind); >>>> >>>> -void dw_mipi_dsi_unbind(struct device *dev) >>>> +void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi) >>>> { >>>> - struct dw_mipi_dsi *dsi = dev_get_drvdata(dev); >>>> - >>>> __dw_mipi_dsi_remove(dsi); >>>> } >>>> EXPORT_SYMBOL_GPL(dw_mipi_dsi_unbind); >>>> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c >>>> index e5b6310..80f9950 100644 >>>> --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c >>>> +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c >>>> @@ -66,6 +66,7 @@ enum dsi_color { >>>> struct dw_mipi_dsi_stm { >>>> void __iomem *base; >>>> struct clk *pllref_clk; >>>> + struct dw_mipi_dsi *dmd; >>>> }; >>>> >>>> static inline void dsi_write(struct dw_mipi_dsi_stm *dsi, u32 reg, u32 val) >>>> @@ -318,10 +319,11 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev) >>>> dw_mipi_dsi_stm_plat_data.base = dsi->base; >>>> dw_mipi_dsi_stm_plat_data.priv_data = dsi; >>>> >>>> - ret = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data); >>>> - if (ret) { >>>> + dsi->dmd = dw_mipi_dsi_probe(pdev, &dw_mipi_dsi_stm_plat_data); >>>> + if (IS_ERR(dsi->dmd)) { >>>> DRM_ERROR("Failed to initialize mipi dsi host\n"); >>>> clk_disable_unprepare(dsi->pllref_clk); >>>> + return PTR_ERR(dsi->dmd); >>>> } >>>> >>>> return ret; >>>> @@ -332,7 +334,7 @@ static int dw_mipi_dsi_stm_remove(struct platform_device *pdev) >>>> struct dw_mipi_dsi_stm *dsi = dw_mipi_dsi_stm_plat_data.priv_data; >>>> >>>> clk_disable_unprepare(dsi->pllref_clk); >>>> - dw_mipi_dsi_remove(pdev); >>>> + dw_mipi_dsi_remove(dsi->dmd); >>>> >>>> return 0; >>>> } >>>> diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h >>>> index 9b30fec..d9c6d54 100644 >>>> --- a/include/drm/bridge/dw_mipi_dsi.h >>>> +++ b/include/drm/bridge/dw_mipi_dsi.h >>>> @@ -10,6 +10,8 @@ >>>> #ifndef __DW_MIPI_DSI__ >>>> #define __DW_MIPI_DSI__ >>>> >>>> +struct dw_mipi_dsi; >>>> + >>>> struct dw_mipi_dsi_phy_ops { >>>> int (*init)(void *priv_data); >>>> int (*get_lane_mbps)(void *priv_data, struct drm_display_mode *mode, >>>> @@ -29,11 +31,14 @@ struct dw_mipi_dsi_plat_data { >>>> void *priv_data; >>>> }; >>>> >>>> -int dw_mipi_dsi_probe(struct platform_device *pdev, >>>> - const struct dw_mipi_dsi_plat_data *plat_data); >>>> -void dw_mipi_dsi_remove(struct platform_device *pdev); >>>> -int dw_mipi_dsi_bind(struct platform_device *pdev, struct drm_encoder *encoder, >>>> - const struct dw_mipi_dsi_plat_data *plat_data); >>>> -void dw_mipi_dsi_unbind(struct device *dev); >>>> +struct dw_mipi_dsi *dw_mipi_dsi_probe(struct platform_device *pdev, >>>> + const struct dw_mipi_dsi_plat_data >>>> + *plat_data); >>>> +void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi); >>>> +struct dw_mipi_dsi *dw_mipi_dsi_bind(struct platform_device *pdev, >>>> + struct drm_encoder *encoder, >>>> + const struct dw_mipi_dsi_plat_data >>>> + *plat_data); >>>> +void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi); >>>> >>>> #endif /* __DW_MIPI_DSI__ */