Re: [PATCH v4 1/3] drm/bridge/synopsys: dsi: stop clobbering drvdata

[Philippe CORNU <philippe.cornu@st.com>]

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.

Many thanks,
Philippe :)

> 
> Thanks,
> Nickey.
>>
>> Many thanks,
>>
>> Philippe :-)
>>
>>
>> On 12/01/2017 04:58 AM, Nickey Yang wrote:
>>> From: Brian Norris<briannorris@chromium.org>
>>>
>>> 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<briannorris@chromium.org>
>>> Signed-off-by: Nickey Yang<nickey.yang@rock-chips.com>
>>> Reviewed-by: Matthias Kaehlcke<mka@chromium.org>
>>> Reviewed-by: Archit Taneja<architt@codeaurora.org>
>>> Acked-by: Philippe Cornu<philippe.cornu@st.com>
>>> 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__ */
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel