<div dir="ltr">Hi Jingoo,<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 22, 2014 at 2:13 PM, Jingoo Han <span dir="ltr"><<a href="mailto:jg1.han@samsung.com" target="_blank">jg1.han@samsung.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tuesday, April 22, 2014 7:39 AM, Ajay Kumar wrote:<br>
><br>
> This patch adds a drm_bridge driver for the PS8622 DisplayPort to LVDS<br>
> bridge chip.<br>
><br>
> Signed-off-by: Andrew Bresticker <<a href="mailto:abrestic@chromium.org">abrestic@chromium.org</a>><br>
> Signed-off-by: Sean Paul <<a href="mailto:seanpaul@chromium.org">seanpaul@chromium.org</a>><br>
> Signed-off-by: Rahul Sharma <<a href="mailto:rahul.sharma@samsung.com">rahul.sharma@samsung.com</a>><br>
> Signed-off-by: Ajay Kumar <<a href="mailto:ajaykumar.rs@samsung.com">ajaykumar.rs@samsung.com</a>><br>
> ---<br>
> Changes since V1:<br>
> Pushing V1 for this as V2 because this patch holds good in this series.<br>
><br>
> drivers/gpu/drm/bridge/Kconfig | 7 +<br>
> drivers/gpu/drm/bridge/Makefile | 1 +<br>
> drivers/gpu/drm/bridge/ps8622.c | 566 +++++++++++++++++++++++++++++++++++++++<br>
> include/drm/bridge/ps8622.h | 42 +++<br>
> 4 files changed, 616 insertions(+)<br>
> create mode 100644 drivers/gpu/drm/bridge/ps8622.c<br>
> create mode 100644 include/drm/bridge/ps8622.h<br>
><br>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig<br>
> index 3bc6845..aba21fc 100644<br>
> --- a/drivers/gpu/drm/bridge/Kconfig<br>
> +++ b/drivers/gpu/drm/bridge/Kconfig<br>
> @@ -4,3 +4,10 @@ config DRM_PTN3460<br>
> select DRM_KMS_HELPER<br>
> select DRM_PANEL<br>
> ---help---<br>
> +<br>
> +config DRM_PS8622<br>
> + tristate "Parade eDP/LVDS bridge"<br>
> + depends on DRM<br>
> + select DRM_KMS_HELPER<br>
> + select DRM_PANEL<br>
<br>
</div></div>Please add the following select.<br>
<br>
+ select BACKLIGHT_LCD_SUPPORT<br>
+ select BACKLIGHT_CLASS_DEVICE<br>
<br>
Without this, the following build issues happen.<br>
<br>
drivers/gpu/drm/bridge/ps8622.c:353: undefined reference to `backlight_device_unregister'<br>
drivers/built-in.o: In function `ps8622_init':<br>
drivers/gpu/drm/bridge/ps8622.c:559: undefined reference to `backlight_device_unregister'<br>
drivers/gpu/drm/bridge/ps8622.c:507: undefined reference to `backlight_device_register'<br>
<div class=""><br></div></blockquote><div>Right. I will add it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> + ---help---<br>
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile<br>
> index b4733e1..d1b5daa 100644<br>
> --- a/drivers/gpu/drm/bridge/Makefile<br>
> +++ b/drivers/gpu/drm/bridge/Makefile<br>
> @@ -1,3 +1,4 @@<br>
> ccflags-y := -Iinclude/drm<br>
><br>
> obj-$(CONFIG_DRM_PTN3460) += ptn3460.o<br>
> +obj-$(CONFIG_DRM_PS8622) += ps8622.o<br>
> diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c<br>
> new file mode 100644<br>
> index 0000000..1e6b3ca<br>
> --- /dev/null<br>
> +++ b/drivers/gpu/drm/bridge/ps8622.c<br>
<br>
</div>[.....]<br>
<div class=""><br>
> +static int ps8622_send_config(struct ps8622_bridge *ps_bridge)<br>
> +{<br>
> + struct i2c_client *cl = ps_bridge->client;<br>
> + int err = 0;<br>
> +<br>
> + /* wait 20ms after power ON */<br>
> + usleep_range(20000, 30000);<br>
> +<br>
> + err |= ps8622_set(cl, 0x02, 0xa1, 0x01); /* HPD low */<br>
> + /* SW setting */<br>
> + err |= ps8622_set(cl, 0x04, 0x14, 0x01); /* [1:0] SW output 1.2V voltage<br>
> + * is lower to 96% */<br>
<br>
</div>The comment style looks awkward.<br>
Please choose one of two options. And change all comments in this file.<br>
<br>
1.<br>
<div class=""><br>
+ /* SW setting */<br>
</div>+ err |= ps8622_set(cl, 0x04, 0x14, 0x01); /* [1:0] SW output 1.2V voltage is lower to 96% */<br>
<br>
2.<br>
<div class=""><br>
+ /* SW setting */<br>
</div>+ /* [1:0] SW output 1.2V voltage is lower to 96% */<br>
<div class="">+ err |= ps8622_set(cl, 0x04, 0x14, 0x01);<br>
<br></div></blockquote><div>Will use this. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
<br>
</div><div><div class="h5">> + /* RCO SS setting */<br>
> + err |= ps8622_set(cl, 0x04, 0xe3, 0x20); /* [5:4] = b01 0.5%, b10 1%,<br>
> + * b11 1.5% */<br>
> + err |= ps8622_set(cl, 0x04, 0xe2, 0x80); /* [7] RCO SS enable */<br>
> + /* RPHY Setting */<br>
> + err |= ps8622_set(cl, 0x04, 0x8a, 0x0c); /* [3:2] CDR tune wait cycle<br>
> + * before measure for fine tune<br>
> + * b00: 1us b01: 0.5us b10:2us<br>
> + * b11: 4us */<br>
> + err |= ps8622_set(cl, 0x04, 0x89, 0x08); /* [3] RFD always on */<br>
> + err |= ps8622_set(cl, 0x04, 0x71, 0x2d); /* CTN lock in/out:<br>
> + * 20000ppm/80000ppm.<br>
> + * Lock out 2 times. */<br>
> + /* 2.7G CDR settings */<br>
> + err |= ps8622_set(cl, 0x04, 0x7d, 0x07); /* NOF=40LSB for HBR CDR<br>
> + * setting */<br>
> + err |= ps8622_set(cl, 0x04, 0x7b, 0x00); /* [1:0] Fmin=+4bands */<br>
> + err |= ps8622_set(cl, 0x04, 0x7a, 0xfd); /* [7:5] DCO_FTRNG=+-40% */<br>
> + /* 1.62G CDR settings */<br>
> + err |= ps8622_set(cl, 0x04, 0xc0, 0x12); /* [5:2]NOF=64LSB [1:0]DCO<br>
> + * scale is 2/5 */<br>
> + err |= ps8622_set(cl, 0x04, 0xc1, 0x92); /* Gitune=-37% */<br>
> + err |= ps8622_set(cl, 0x04, 0xc2, 0x1c); /* Fbstep=100% */<br>
> + err |= ps8622_set(cl, 0x04, 0x32, 0x80); /* [7] LOS signal disable */<br>
> + /* RPIO Setting */<br>
> + err |= ps8622_set(cl, 0x04, 0x00, 0xb0); /* [7:4] LVDS driver bias<br>
> + * current : 75% (250mV swing)<br>
> + * */<br>
> + err |= ps8622_set(cl, 0x04, 0x15, 0x40); /* [7:6] Right-bar GPIO output<br>
> + * strength is 8mA */<br>
> + /* EQ Training State Machine Setting */<br>
> + err |= ps8622_set(cl, 0x04, 0x54, 0x10); /* RCO calibration start */<br>
> + /* Logic, needs more than 10 I2C command */<br>
> + err |= ps8622_set(cl, 0x01, 0x02, 0x80 | ps_bridge->max_lane_count);<br>
> + /* [4:0] MAX_LANE_COUNT set to<br>
> + * max supported lanes */<br>
> + err |= ps8622_set(cl, 0x01, 0x21, 0x80 | ps_bridge->lane_count);<br>
> + /* [4:0] LANE_COUNT_SET set to<br>
> + * chosen lane count */<br>
> + err |= ps8622_set(cl, 0x00, 0x52, 0x20);<br>
> + err |= ps8622_set(cl, 0x00, 0xf1, 0x03); /* HPD CP toggle enable */<br>
> + err |= ps8622_set(cl, 0x00, 0x62, 0x41);<br>
> + err |= ps8622_set(cl, 0x00, 0xf6, 0x01); /* Counter number, add 1ms<br>
> + * counter delay */<br>
> + err |= ps8622_set(cl, 0x00, 0x77, 0x06); /* [6]PWM function control by<br>
> + * DPCD0040f[7], default is PWM<br>
> + * block always works. */<br>
> + err |= ps8622_set(cl, 0x00, 0x4c, 0x04); /* 04h Adjust VTotal tolerance<br>
> + * to fix the 30Hz no display<br>
> + * issue */<br>
> + err |= ps8622_set(cl, 0x01, 0xc0, 0x00); /* DPCD00400='h00, Parade OUI =<br>
> + * 'h001cf8 */<br>
> + err |= ps8622_set(cl, 0x01, 0xc1, 0x1c); /* DPCD00401='h1c */<br>
> + err |= ps8622_set(cl, 0x01, 0xc2, 0xf8); /* DPCD00402='hf8 */<br>
> + err |= ps8622_set(cl, 0x01, 0xc3, 0x44); /* DPCD403~408 = ASCII code<br>
> + * D2SLV5='h4432534c5635 */<br>
> + err |= ps8622_set(cl, 0x01, 0xc4, 0x32); /* DPCD404 */<br>
> + err |= ps8622_set(cl, 0x01, 0xc5, 0x53); /* DPCD405 */<br>
> + err |= ps8622_set(cl, 0x01, 0xc6, 0x4c); /* DPCD406 */<br>
> + err |= ps8622_set(cl, 0x01, 0xc7, 0x56); /* DPCD407 */<br>
> + err |= ps8622_set(cl, 0x01, 0xc8, 0x35); /* DPCD408 */<br>
> + err |= ps8622_set(cl, 0x01, 0xca, 0x01); /* DPCD40A, Initial Code major<br>
> + * revision '01' */<br>
> + err |= ps8622_set(cl, 0x01, 0xcb, 0x05); /* DPCD40B, Initial Code minor<br>
> + * revision '05' */<br>
> + if (ps_bridge->bl) {<br>
> + err |= ps8622_set(cl, 0x01, 0xa5, 0xa0);<br>
> + /* DPCD720, internal PWM */<br>
> + err |= ps8622_set(cl, 0x01, 0xa7,<br>
> + ps_bridge->bl->props.brightness);<br>
> + /* FFh for 100% brightness,<br>
> + * 0h for 0% brightness */<br>
> + } else {<br>
> + err |= ps8622_set(cl, 0x01, 0xa5, 0x80);<br>
> + /* DPCD720, external PWM */<br>
> + }<br>
> + err |= ps8622_set(cl, 0x01, 0xcc, 0x13); /* Set LVDS output as 6bit-VESA<br>
> + * mapping, single LVDS channel<br>
> + * */<br>
> + err |= ps8622_set(cl, 0x02, 0xb1, 0x20); /* Enable SSC set by register<br>
> + * */<br>
> + err |= ps8622_set(cl, 0x04, 0x10, 0x16); /* Set SSC enabled and +/-1%<br>
> + * central spreading */<br>
> + /* Logic end */<br>
> + err |= ps8622_set(cl, 0x04, 0x59, 0x60); /* MPU Clock source: LC => RCO<br>
> + * */<br>
> + err |= ps8622_set(cl, 0x04, 0x54, 0x14); /* LC -> RCO */<br>
> + err |= ps8622_set(cl, 0x02, 0xa1, 0x91); /* HPD high */<br>
> +<br>
> + return err ? -EIO : 0;<br>
> +}<br>
> +<br>
<br>
</div></div>[.....]<br>
<div><div class="h5"><br>
> +<br>
> +static void ps8622_pre_enable(struct drm_bridge *bridge)<br>
> +{<br>
> + struct ps8622_bridge *ps_bridge = bridge->driver_private;<br>
> + int ret;<br>
> +<br>
> + mutex_lock(&ps_bridge->enable_mutex);<br>
> + if (ps_bridge->enabled)<br>
> + goto out;<br>
> +<br>
> + if (gpio_is_valid(ps_bridge->gpio_rst_n))<br>
> + gpio_set_value(ps_bridge->gpio_rst_n, 0);<br>
> +<br>
> + if (ps_bridge->v12) {<br>
> + ret = regulator_enable(ps_bridge->v12);<br>
> + if (ret)<br>
> + DRM_ERROR("fails to enable ps_bridge->v12");<br>
> + }<br>
> +<br>
> + drm_panel_pre_enable(ps_bridge->panel);<br>
> +<br>
> + if (gpio_is_valid(ps_bridge->gpio_slp_n))<br>
> + gpio_set_value(ps_bridge->gpio_slp_n, 1);<br>
> +<br>
> + /*<br>
> + * T1 is the range of time that it takes for the power to rise after we<br>
> + * enable the lcd fet. T2 is the range of time in which the data sheet<br>
> + * specifies we should deassert the reset pin.<br>
> + *<br>
> + * If it takes T1.max for the power to rise, we need to wait atleast<br>
> + * T2.min before deasserting the reset pin. If it takes T1.min for the<br>
> + * power to rise, we need to wait at most T2.max before deasserting the<br>
> + * reset pin.<br>
> + */<br>
> + usleep_range(PS8622_RST_HIGH_T2_MIN_US + PS8622_POWER_RISE_T1_MAX_US,<br>
> + PS8622_RST_HIGH_T2_MAX_US + PS8622_POWER_RISE_T1_MIN_US);<br>
> +<br>
> + if (gpio_is_valid(ps_bridge->gpio_rst_n))<br>
> + gpio_set_value(ps_bridge->gpio_rst_n, 1);<br>
> +<br>
> + ret = ps8622_send_config(ps_bridge);<br>
> + if (ret)<br>
> + DRM_ERROR("Failed to send config to bridge (%d)\n", ret);<br>
> +<br>
> + ps_bridge->enabled = true;<br>
> +<br>
> + mutex_unlock(&ps_bridge->enable_mutex);<br>
> + return;<br>
> +<br>
> +out:<br>
> +<br>
> + mutex_unlock(&ps_bridge->enable_mutex);<br>
> +}<br>
<br>
</div></div>mutex_unlock() is duplicated. Also, 'return' is unnecessary.<br>
Please fix it as below.<br>
<div class=""><br>
+ ps_bridge->enabled = true;<br>
+<br>
</div><div><div class="h5">+out:<br>
+ mutex_unlock(&ps_bridge->enable_mutex);<br>
+}<br></div></div></blockquote><div>Right. Will change it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
<br>
> +<br>
> +static void ps8622_enable(struct drm_bridge *bridge)<br>
> +{<br>
> + struct ps8622_bridge *ps_bridge = bridge->driver_private;<br>
> +<br>
> + mutex_lock(&ps_bridge->enable_mutex);<br>
> + if (ps_bridge->enabled)<br>
> + drm_panel_enable(ps_bridge->panel);<br>
> + mutex_unlock(&ps_bridge->enable_mutex);<br>
> +}<br>
> +<br>
> +static void ps8622_disable(struct drm_bridge *bridge)<br>
> +{<br>
> + struct ps8622_bridge *ps_bridge = bridge->driver_private;<br>
> +<br>
> + mutex_lock(&ps_bridge->enable_mutex);<br>
> +<br>
> + if (!ps_bridge->enabled)<br>
> + goto out;<br>
> +<br>
> + ps_bridge->enabled = false;<br>
> +<br>
> + drm_panel_disable(ps_bridge->panel);<br>
> + msleep(PS8622_PWMO_END_T12_MS);<br>
> +<br>
> + /*<br>
> + * This doesn't matter if the regulators are turned off, but something<br>
> + * else might keep them on. In that case, we want to assert the slp gpio<br>
> + * to lower power.<br>
> + */<br>
> + if (gpio_is_valid(ps_bridge->gpio_slp_n))<br>
> + gpio_set_value(ps_bridge->gpio_slp_n, 0);<br>
> +<br>
> + drm_panel_post_disable(ps_bridge->panel);<br>
> + if (ps_bridge->v12)<br>
> + regulator_disable(ps_bridge->v12);<br>
> +<br>
> + /*<br>
> + * Sleep for at least the amount of time that it takes the power rail to<br>
> + * fall to prevent asserting the rst gpio from doing anything.<br>
> + */<br>
> + usleep_range(PS8622_POWER_FALL_T16_MAX_US,<br>
> + 2 * PS8622_POWER_FALL_T16_MAX_US);<br>
> + if (gpio_is_valid(ps_bridge->gpio_rst_n))<br>
> + gpio_set_value(ps_bridge->gpio_rst_n, 0);<br>
> +<br>
> + msleep(PS8622_POWER_OFF_T17_MS);<br>
> +<br>
> +out:<br>
> + mutex_unlock(&ps_bridge->enable_mutex);<br>
> +}<br>
> +<br>
> +static void ps8622_post_disable(struct drm_bridge *bridge)<br>
> +{<br>
> +}<br>
<br>
</div></div>How about just removing this empty function?</blockquote><div>That is not possible. Because the bridge callbacks are called in this fashion:</div><div> bridge->funcs->post_disable(bridge);</div><div>
So, if that particular callback turns NULL, it will crash!</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
[.....]<br>
<br>
Best regards,<br>
Jingoo Han<br>
<br>
</blockquote></div><br></div><div class="gmail_extra">Regards,</div><div class="gmail_extra">Ajay Kumar</div></div>