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

[Archit Taneja <architt@codeaurora.org>]

On 07/05/2016 01:30 PM, Philipp Zabel wrote:
> 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.

Module parameter sounds like a good option. Although, it seems like the
pll is enabled only when test_pattern is set. How does the bridge work
if the pll isn't enabled?

Archit

>
> [...]
>>> +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
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel