Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 28 Nov 2017 21:02:27 -0500
From:   Sean Paul <seanpaul@chromium.org>
To:     Brian Norris <briannorris@chromium.org>
Cc:     Matthias Kaehlcke <mka@chromium.org>,
        Nickey Yang <nickey.yang@rock-chips.com>, robh+dt@kernel.org,
        heiko@sntech.de, mark.rutland@arm.com, airlied@linux.ie,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linux-rockchip@lists.infradead.org, seanpaul@chromium.org,
        hoegsberg@gmail.com, architt@codeaurora.org, philippe.cornu@st.com,
        yannick.fertre@st.com, hl@rock-chips.com, zyw@rock-chips.com,
        xbl@rock-chips.com, hjc@rock-chips.com
Subject: Re: [PATCH v3 4/5] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller
 driver
Message-ID: <20171129020227.ydqu7vxqw6mqond3@art_vandelay>
References: <1511868006-27130-1-git-send-email-nickey.yang@rock-chips.com>
 <1511868006-27130-5-git-send-email-nickey.yang@rock-chips.com>
 <20171128204843.GB58379@google.com>
 <20171128225540.GA23117@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20171128225540.GA23117@google.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 28, 2017 at 02:55:41PM -0800, Brian Norris wrote:
> 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.
> 

We're kind of different in drm land. Some prefer to keep the changelog above the
fold and attribute the changes to reviewers so they get recognition for their
efforts.

At any rate, I'm just happy to see changelogs.

> > > 
> > > 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.
> 

Yeah, not a showstopper, but it'd be nice to know what's going on here.

> ...
> 
> > > +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?

+1
> 
> > > +	}
> > > +
> > > +	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>

I certainly think this moves the needle in a positive direction. So with the
nits addressed:

Reviewed-by: Sean Paul <seanpaul@chromium.org>

-- 
Sean Paul, Software Engineer, Google / Chromium OS

From 1585352342722783645@xxx Tue Nov 28 22:56:51 +0000 2017
X-GM-THRID: 1585308635247124425
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread