Re: [PATCH v3 4/5] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver

[Brian Norris <briannorris@chromium.org>]

Hi Nickey,

On Tue, Nov 28, 2017 at 12:48:43PM -0800, Matthias Kaehlcke wrote:
> El Tue, Nov 28, 2017 at 07:20:05PM +0800 Nickey Yang ha dit:
> 
> > Add the ROCKCHIP DSI controller driver that uses the Synopsys DesignWare
> > MIPI DSI host controller bridge.
> > 
> > v2:
> >    add err_pllref, remove unnecessary encoder.enable & disable
> >    correct spelling mistakes
> > v3:
> >    call dw_mipi_dsi_unbind() in dw_mipi_dsi_rockchip_unbind()
> >    fix typo, use of_device_get_match_data(),
> >    change some ‘bind()’ logic into 'probe()'
> >    add 'dev_set_drvdata()'

I believe the changelog normally goes below the "---", so it gets
dropped when a maintainer applies a final version.

> > 
> > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> > ---
> >  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 |  764 +++++++++++++
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.c     |    2 +-
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 +-
> >  6 files changed, 768 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.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > deleted file mode 100644
> > index b15755b..0000000
> > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > +++ /dev/null
> > @@ -1,1349 +0,0 @@

...

> > -static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
> > -				   struct mipi_dsi_device *device)
> > -{
> > -	struct dw_mipi_dsi *dsi = host_to_dsi(host);
> > -
> > -	if (device->lanes > dsi->pdata->max_data_lanes) {
> > -		DRM_DEV_ERROR(dsi->dev,
> > -			      "the number of data lanes(%u) is too many\n",
> > -			      device->lanes);
> > -		return -EINVAL;
> > -	}
> > -
> > -	dsi->lanes = device->lanes;
> > -	dsi->channel = device->channel;
> > -	dsi->format = device->format;
> > -	dsi->mode_flags = device->mode_flags;
> > -	dsi->panel = of_drm_find_panel(device->dev.of_node);

IIUC, you're implicitly making a device tree binding change, because the
original driver uses just of_drm_find_panel(), as above, but the common
bridge driver is using drm_of_find_panel_or_bridge(), which puts a
little more stringent requirements on the device tree.

I don't think that's necessarily a bad thing, and there isn't much in
the way of "real" device trees that actually used the existing driver
and binding (probably mostly test devices and prototypes), so maybe it's
better to just make the switch and not worry about compatibility. But I
just wanted to point that out, in case anyone else was interested or
concerned.

> > -	if (dsi->panel)
> > -		return drm_panel_attach(dsi->panel, &dsi->connector);
> > -
> > -	return -EINVAL;
> > -}
> > -

...

> > 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..c682ed2
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> > @@ -0,0 +1,764 @@

...

> > +static int dw_mipi_dsi_phy_init(void *priv_data)
> > +{
> > +	struct dw_mipi_dsi_rockchip *dsi = priv_data;
> > +	int ret, i, vco;
> > +
> > +	vco = (dsi->lane_mbps < 200) ? 0 : (dsi->lane_mbps + 100) / 200;
> 
> Please add a clarifying comment as requested by Sean on
> https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/780120/

FWIW, that code was already in the existing driver. Would be nice to
improve anyway, of course.

...

> > +static int
> > +dw_mipi_dsi_get_lane_mbps(void *priv_data, struct drm_display_mode *mode,
> > +			  unsigned long mode_flags, u32 lanes, u32 format,
> > +			  unsigned int *lane_mbps)
> > +{
> > +	struct dw_mipi_dsi_rockchip *dsi = priv_data;
> > +	int bpp;
> > +	unsigned long mpclk, tmp;
> > +	unsigned int target_mbps = 1000;
> > +	unsigned int max_mbps = dppa_map[ARRAY_SIZE(dppa_map) - 1].max_mbps;
> > +	unsigned long best_freq = 0;
> > +	unsigned long fvco_min, fvco_max, fin, fout;
> > +	unsigned int min_prediv, max_prediv;
> > +	unsigned int _prediv, uninitialized_var(best_prediv);
> > +	unsigned long _fbdiv, uninitialized_var(best_fbdiv);
> > +	unsigned long min_delta = ULONG_MAX;
> > +
> > +	dsi->format = format;
> > +	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +	if (bpp < 0) {
> > +		DRM_DEV_ERROR(dsi->dev,
> > +			      "failed to get bpp for pixel format %d\n",
> > +			      dsi->format);
> > +		return bpp;
> > +	}
> > +
> > +	mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC);
> > +	if (mpclk) {
> > +		/* take 1 / 0.8, since mbps must big than bandwidth of RGB */
> > +		tmp = mpclk * (bpp / lanes) * 10 / 8;
> > +		if (tmp < max_mbps)
> > +			target_mbps = tmp;
> > +		else
> > +			DRM_DEV_ERROR(dsi->dev,
> > +				      "DPHY clock frequency is out of range\n");
> > +	}
> > +
> > +	fin = clk_get_rate(dsi->pllref_clk);
> > +	fout = target_mbps * USEC_PER_SEC;
> > +
> > +	/* constraint: 5Mhz <= Fref / N <= 40MHz */
> > +	min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
> > +	max_prediv = fin / (5 * USEC_PER_SEC);
> > +
> > +	/* constraint: 80MHz <= Fvco <= 1500Mhz */
> > +	fvco_min = 80 * USEC_PER_SEC;
> > +	fvco_max = 1500 * USEC_PER_SEC;
> > +
> > +	for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
> > +		u64 tmp;
> > +		u32 delta;
> > +		/* Fvco = Fref * M / N */
> > +		tmp = (u64)fout * _prediv;
> > +		do_div(tmp, fin);
> > +		_fbdiv = tmp;
> > +		/*
> > +		 * Due to the use of a "by 2 pre-scaler," the range of the
> > +		 * feedback multiplication value M is limited to even division
> > +		 * numbers, and m must be greater than 6, less than 512.
> > +		 */
> 
> It seems this should be "not bigger than 512" or something similar.
> 
> > +		if (_fbdiv < 6 || _fbdiv > 512)
> > +			continue;
> > +
> > +		_fbdiv += _fbdiv % 2;
> > +
> > +		tmp = (u64)_fbdiv * fin;
> > +		do_div(tmp, _prediv);
> 
> Should we bail out early if min_prediv == 0 due to some bogus
> configuration of pllref_clk?
> 
> > +		if (tmp < fvco_min || tmp > fvco_max)
> > +			continue;
> > +
> > +		delta = abs(fout - tmp);
> > +		if (delta < min_delta) {
> > +			best_prediv = _prediv;
> > +			best_fbdiv = _fbdiv;
> > +			min_delta = delta;
> > +			best_freq = tmp;
> > +		}
> > +	}
> > +
> > +	if (best_freq) {
> > +		dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);
> > +		*lane_mbps = dsi->lane_mbps;
> > +		dsi->input_div = best_prediv;
> > +		dsi->feedback_div = best_fbdiv;
> > +	} else {
> > +		DRM_DEV_ERROR(dsi->dev, "Can not find best_freq for DPHY\n");
> 
> return -1;

Or a real error code would be nicer. -EINVAL?

> > +	}
> > +
> > +	return 0;
> > +}
> > +

...

Other than these relatively small things, this is looking pretty good to
my (not-well-versed-in-drm) eye:

Reviewed-by: Brian Norris <briannorris@chromium.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel