Re: [PATCH v2 2/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver

[Brian Norris]

Hi Nickey,

Thanks for the quick version 2. You've fixed most of the the problems I
pointed out, but you've missed at least one.

On Tue, Nov 28, 2017 at 09:55:24AM +0800, Nickey Yang wrote:
> Add the ROCKCHIP DSI controller driver that uses the Synopsys DesignWare
> MIPI DSI host controller bridge.
> 
> Signed-off-by: Nickey Yang <nickey.yang@xxxxxxxxxxxxxx>
> ---

I suppose you've included a changelog in the cover letter (thanks!), but
sometimes it helps to go here too, to give an individual history of each
patch.

>  drivers/gpu/drm/rockchip/Kconfig                |    2 +-
>  drivers/gpu/drm/rockchip/Makefile               |    2 +-
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c          | 1349 -----------------------
>  drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c |  756 +++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c     |    2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 +-
>  6 files changed, 760 insertions(+), 1353 deletions(-)
>  delete mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>  create mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> 


> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> new file mode 100644
> index 0000000..c919d4f
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> @@ -0,0 +1,756 @@
[...]
> +static int dw_mipi_dsi_rockchip_bind(struct device *dev,
> +				     struct device *master,
> +				     void *data)
> +{
> +	const struct of_device_id *of_id =
> +			of_match_device(dw_mipi_dsi_rockchip_dt_ids, dev);
> +	const struct rockchip_dw_dsi_chip_data *cdata = of_id->data;
> +	struct dw_mipi_dsi_rockchip *dsi = dev_get_drvdata(dev);

Hmm, so I see 2 instances of 'dev_get_drvdata()' in this driver, and one
of them is here. I never see a 'dev_set_drvdata()', so that can't
possibly be useful.

In this case...you've actually moved all the 'probe()' logic into
bind(), so there's no driver data to carry over here. And instead, you
just clobber this 'dsi = ...' assignment a few lines below:

> +	struct resource *res;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm_dev = data;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);

^^^ See here.

So if you don't actually need to stash anything in drvdata between
probe() and bind(), then you might get away without my patch:

[PATCH] drm/bridge/synopsis: stop clobbering drvdata
https://patchwork.kernel.org/patch/10078493/

There's one other instance to review though:

> +	if (!dsi)
> +		return -ENOMEM;
> +
[...]

> +static void dw_mipi_dsi_rockchip_unbind(struct device *dev,
> +					struct device *master,
> +					void *data)
> +{
> +	struct dw_mipi_dsi_rockchip *dsi = dev_get_drvdata(dev);

^^ That's wrong. You either want to do your own 'dev_set_drvdata()'
somewhere (and make sure the bridge driver doesn't clobber it, e.g.,
with my patch above), or else you need to convince the bridge driver to
give us back something like its 'priv_data' pointer
(dsi->plat_data->priv_data).

Also, don't you need to call dw_mipi_dsi_unbind() here?

> +	clk_disable_unprepare(dsi->pllref_clk);
> +}
> +
> +static const struct component_ops dw_mipi_dsi_rockchip_ops = {
> +	.bind	= dw_mipi_dsi_rockchip_bind,
> +	.unbind	= dw_mipi_dsi_rockchip_unbind,
> +};

...

Brian