Re: [PATCH 2/2] drm/bridge: tc358767: Add DPI to eDP bridge driver

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Archit,

thanks for the review!

Am Dienstag, den 05.07.2016, 10:08 +0530 schrieb Archit Taneja:
[...]
> > +#include <drm/drmP.h>
> 
> This may not be needed.

I'll check and remove it.

> > +#ifndef regmap_read_poll_timeout
> > +#define regmap_read_poll_timeout(map, addr, val, cond, sleep_us, timeout_us) \
> > +({ \
> > +	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> > +	int ret; \
> > +	might_sleep_if(sleep_us); \
> > +	for (;;) { \
> > +		ret = regmap_read((map), (addr), &(val)); \
> > +		if (ret) \
> > +			break; \
> > +		if (cond) \
> > +			break; \
> > +		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
> > +			ret = regmap_read((map), (addr), &(val)); \
> > +			break; \
> > +		} \
> > +		if (sleep_us) \
> > +			usleep_range((sleep_us >> 2) + 1, sleep_us); \
> > +	} \
> > +	ret ?: ((cond) ? 0 : -ETIMEDOUT); \
> > +})
> > +#endif
> 
> Is there a reason why this is wrapped around a #ifndef? I don't see it
> in the current regmap.h headers. It would also be nice if it were made
> into a function.

This is a macro similar to those defined in iopoll.h. It can't be a
function because the "cond"ition is specified by the caller and has to
be evaluated in the loop.
I'll submit this for regmap. If it doesn't get accepted I'll change this
into two properly named functions.

[...]
> > +static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk)
> > +{
> > +	int ret;
> > +	int i_pre, best_pre = 1;
> > +	int i_post, best_post = 1;
> > +	int div, best_div = 1;
> > +	int mul, best_mul = 1;
> > +	int delta, best_delta;
> > +	int ext_div[] = {1, 2, 3, 5, 7};
> > +	int best_pixelclock = 0;
> > +	int vco_hi = 0;
> > +	int pixelclock;
> > +
> > +	pixelclock = tc->pll_clk;
> > +
> > +	dev_dbg(tc->dev, "PLL: requested %d pixelclock, ref %d\n", pixelclock,
> > +		refclk);
> > +	best_delta = pixelclock;
> > +	/* big loops */
> 
> The above comment could be improved.

Will do.

> > +	for (i_pre = 0; i_pre < ARRAY_SIZE(ext_div); i_pre++) {
> > +		/* refclk / ext_pre_div should be in the 1 to 200 MHz range */
> > +		if ((refclk / ext_div[i_pre] > 200000000) ||
> 
> The > 200 Mhz check seems redundant, since no refclk beyond 38.4 Mhz is
> supported.

Indeed. I'll drop the check and update the comment.

> > +		    (refclk / ext_div[i_pre] < 1000000))
> > +			continue;
> > +		for (i_post = 0; i_post < ARRAY_SIZE(ext_div); i_post++) {
> > +			for (div = 1; div <= 16; div++) {
> > +				u32 clk;
> > +				u64 tmp;
> > +
> > +				tmp = pixelclock * ext_div[i_pre] *
> > +				      ext_div[i_post] * div;
> > +				do_div(tmp, refclk);
> > +				mul = tmp;
> > +
> > +				clk = refclk / ext_div[i_pre] / div * mul /
> > +				      ext_div[i_post];
> 
> Some braces for the above expression might help :)

Ok.

> > +				delta = clk - pixelclock;
> > +
> > +				/* check limits */
> > +				if ((mul < 1) ||
> > +				    (mul > 128))
> 
> minor comment: the above check could be in a single line.

Oversight, will fix.

[...]
> > +static int tc_aux_link_setup(struct tc_data *tc)
> > +{
> > +	unsigned long rate;
> > +	u32 value;
> > +	int ret;
> > +
> > +	rate = clk_get_rate(tc->refclk);
> 
> Ah, you can discard my comment on the clock binding in the DT patch.
> I guess you needed it to figure out the rate.

Alright, going back to the other mail and updating my answer.

> > +	switch (rate) {
> > +	case 38400000:
> > +		value = REF_FREQ_38M4;
> > +		break;
> > +	case 26000000:
> > +		value = REF_FREQ_26M;
> > +		break;
> > +	case 19200000:
> > +		value = REF_FREQ_19M2;
> > +		break;
> > +	case 13000000:
> > +		value = REF_FREQ_13M;
> > +		break;
> > +	default:
> > +		dev_err(tc->dev, "Invalid refclk rate: %lu Hz\n", rate);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Setup DP-PHY / PLL */
> > +	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
> > +	tc_write(SYS_PLLPARAM, value);
> > +
> > +	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | BIT(2) | PHY_A0_EN);
> 
> It seems a bit strange to have BIT(2) besides the other predefined
> macros.

I accidentally lost the comment when shortening this, BIT(2) is reserved
but set.

[...]
> > +	/* PXL PLL setup */
> > +	if (tc->test_pattern) {
> 
> I couldn't find out who is setting tc->test_pattern. Is it always
> 0?

Hm, you are right. I wonder what a good mechanism would be to enable a
test pattern for a bridge driver. Module parameters? We don't have
anyhting like V4L2_CID_TEST_PATTERN in drm. I could also drop test
pattern support from the initial patch and submit it separately.

[...]
> > +static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
> > +	.get_modes = tc_connector_get_modes,
> > +	.mode_valid = tc_connector_mode_valid,
> > +	.best_encoder = tc_connector_best_encoder,
> > +};
> > +
> > +static void tc_connector_destroy(struct drm_connector *connector)
> > +{
> > +	drm_connector_unregister(connector);
> > +	drm_connector_cleanup(connector);
> > +}
> > +
> > +static const struct drm_connector_funcs tc_connector_funcs = {
> > +	.dpms = drm_helper_connector_dpms,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.detect = tc_connector_detect,
> > +	.destroy = tc_connector_destroy,
> 
> Can we use atomic helpers where applicable?

I have worked on this on i.MX6, which doesn't have atomic support merged
yet. I'll test with Liu Ying's i.MX6 atomic modeset patches and update
to atomic helpers here.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel